Open
Conversation
…r without staged files
…net only work when not needed
…d return type
Replaces the old whitelist walk (which had a dead-code short-circuit making it
always return true) with a correct blacklist implementation:
- Walks depth-first, finding the innermost statement that *starts* on the target
line; separately tracks block-end lines (end if/for/while) via range check
- Rejects known non-executable statement types (comments, imports, declarations,
labels, dim, const, enum/interface members, etc.) rather than requiring every
executable type to be enumerated
- Function/sub header lines are valid breakpoints; end function/sub are not
- Returns LineValidationResult {isExecutable, correctedLine?} instead of boolean,
laying the groundwork for future breakpoint-position correction
- Rewrites tests to use an executable(...lines: [string, boolean][]) helper so
expected values sit inline next to each source line
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- end if / end for / end while: now non-executable (removed hasBlockEnd logic entirely) - dim / label / standalone end: now executable (removed from blacklist) - Simplifies the walk: any line where no statement starts is unconditionally non-executable Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Now that function headers are valid breakpoint locations, sub main() on line 1 is staged as a permanent breakpoint and correctly comes back verified:true — the original test expectations hold without the workaround. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…nguage - Rename method: isStagingLineExecutable → isValidBreakpointLine - Rename helper: isNonExecutableLine → isInvalidBreakpointLine - Rename result field: isExecutable → isValid on LineValidationResult - Update all JSDoc, comments, and section headers to use "valid/invalid breakpoint location" language instead of "executable/non-executable" - Rename test helper: executable() → checkLines() and update its JSDoc - Update all test names to use "valid/invalid breakpoint location" phrasing - Fix integration test that used sub header (now valid) as the invalid line; switch to end sub instead Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
TwitchBronBron
requested changes
Mar 31, 2026
Comment on lines
+1204
to
+1206
| await this.breakpointManager.resolveBreakpointsForProject(this.projectManager.mainProject); | ||
| } else { | ||
| await this.breakpointManager.injectBreakpointsForProject(this.projectManager.mainProject); |
Member
There was a problem hiding this comment.
Can we move this branching logic inside of a single call? Would prefer avoiding branchout out here if possible.
|
|
||
| await this.breakpointManager.writeBreakpointsForProject(this.projectManager.mainProject); | ||
| if (this.enableDebugProtocol) { | ||
| await this.breakpointManager.resolveBreakpointsForProject(this.projectManager.mainProject); |
Member
There was a problem hiding this comment.
Let's find different names for these.
"write" still feels good for when it actually is rewriting the code.
"resolve" is the ambiguous one, it feels like it's more like "scan the project and make sure that every breakpoint is allowed to be at this spot, or throw out the ones that aren't allowed or move them". So you know, a good function name for that. ;)
| * Write "stop" lines into source code for each breakpoint of each file in the given project | ||
| * Validate breakpoints against the project's staging files and fail any that cannot be placed | ||
| * (non-executable lines, unsupported file types, files not in the project). | ||
| * Called for all debugger types. Returns the resolved breakpoints keyed by staging file path. |
Member
There was a problem hiding this comment.
Suggested change
| * Called for all debugger types. Returns the resolved breakpoints keyed by staging file path. | |
| * Returns the resolved breakpoints keyed by staging file path. |
| bp.verified = false; | ||
| bp.reason = 'failed'; | ||
| //only set a message for file types we understand — for unknown file types | ||
| //(JSON, etc.) leave the message empty so other debuggers can claim the breakpoint |
Member
There was a problem hiding this comment.
comment feels incorrect, "claim the breakpoint"
| * Marks injected breakpoints as verified and registers them as permanent. | ||
| * Only called for telnet debuggers. | ||
| */ | ||
| public async injectBreakpointsForProject(project: Project) { |
Member
There was a problem hiding this comment.
This one, i liked the name "write" better than "inject".
| * Clear the script-referenced files cache. Should be called when staging directory | ||
| * contents may have changed (e.g. at the start of a new debug session). | ||
| */ | ||
| public clearScriptReferencedFilesCache() { |
Member
There was a problem hiding this comment.
combine these to a .reset() function instead of separate functions? Also, if we dispose the manager across reloads, we don't need to worry about it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
endstatements, comments, imports, etc.) as failed withNo executable code at this linewriteBreakpointsForProjectinto two clearly-scoped methods:resolveBreakpointsForProject(runs for all debuggers) andinjectBreakpointsForProject(telnet only — writes STOP statements and registers permanent breakpoints)Test plan
sub/functionheaders,end sub,end function, comments,import,librarystatements are marked failed withNo executable code at this line🤖 Generated with Claude Code