Fix SAST findings, enhance error handling, and improve diagnostics - #15
Merged
Conversation
npx was on the default allow-list, NON_INTERACTIVE_ENV set npm_config_yes so its own "Ok to proceed?" never appeared, and it was absent from ALWAYS_CONFIRM — so under auto-approve it fetched and ran arbitrary remote code with no click, in an extension whose headline claim is that it works fully offline. A live 0.6.0 run has it in the audit log, auto-approved and recorded as routine. All six spellings now confirm: npx, npm exec/x/create, yarn dlx/create, pnpm dlx/create. Bare `npm init` is left alone because it writes a manifest and touches no network, and a click for nothing is a click trained away. workspaceBootstrap wrote .gitignore and environment.json straight through path.join, the only two writes in the extension that skipped pathGuard and therefore its symlink check. Both now resolve through it. The guard needed a synchronous twin to get there, since activation reads the profile immediately; the containment half is shared so the two cannot drift on the part that decides. mocha 11.8.0 with overrides pinning diff@9 and serialize-javascript@7.1.0, which is what actually clears the advisories — upstream mocha still asks for diff@^7 and serialize-javascript@^6, and `npm audit fix --force` would have moved sideways to 11.3.0 without fixing either. npm audit now reports zero across the full tree rather than only under --omit=dev. Suite verified serially, in parallel mode (the serialize-javascript path), and against a failing assertion (the diff reporter). 1222 unit tests passing, 0 lint errors.
Four changes that share one premise: a 1B model that is stuck does not know it, and everything it does next makes the run worse. From the 0.5.3 and 0.6.0 rounds, the three endings were resend the failing action until the budget is gone, report success it did not achieve, or abandon a task it was one step from finishing. Undefined symbols now have diagnosis rules. ReferenceError, NameError, javac's cannot find symbol, reading a property of undefined or null, AttributeError, x is not a function, and NullPointerException — seven rules across JS, Python and Java, and each sentence names the symbol the error named, because "something is undefined" is not actionable and "`addTodo` is used but never imported there" is. `scriptDiagnosis` rules may now compute their text from the match, which is what makes that possible; the matcher moved from test to exec and gained a test pinning that repeated calls agree. errorRecovery covers what no rule matches, and what a rule matched twice. The ladder is guidance on the first failure, ask the user on the second — set against reactLoop.REPEAT_LIMIT rather than a round number, because the loop ends a run after two identical actions and a ladder waiting for a third would never reach its top rung. What kept failing goes into memory, composed rather than model-written, since a failed step is exactly what the translator has nothing to narrate about and exactly what the next run needs. commonSense reads the request before the model does. A name one edit from a real file is repaired with both names stated; two candidates ask; no candidate says nothing, because that is what creating a file looks like. The verb governing the filename is read from the six words in front of it, so "update mian.js to add a header" is a typo rather than a creation. Damerau-Levenshtein, not memoryStore.similarity — that one is Jaccard over whole words and scores mian.js against main.js at zero. clarification is the shared shape for asking: two to four options, exactly one recommended, every option stating its effect on the queue, and free text always available. The TodoList can now be added to, reworded, and skipped mid-run, and it keeps a change log so the summary can say the checklist changed and why. The property most worth keeping is the negative one, and it is tested from both sides: a session with no onClarify never blocks. Closing a tab, pressing Stop, and a turn that throws all settle an outstanding question, so a background run cannot hold its lane in the turn queue on a card nobody can see. Memory recall is now selected by relevance on every turn rather than only in experimental step mode, and ranked by how much of the subject each note shares instead of by any-token-matches. Rebuilding a route per item exposed that _routeForStep dropped readOnlyTurn; carried through now, since that would otherwise hand the mutating tools back to a look-only request on the default path. 1318 unit tests passing, 0 lint errors.
An answer that only reaches the model is half-applied. The item's text is what a retry is briefed on, what stepGuard checks the changed files against, and what the summary reads back — so a free-text answer now rewords the running item, and a skip closes it as the user's decision rather than as a failure. The distinction matters twice over: a failed item puts a [!] against a row the user themselves closed, and it feeds "an earlier step failed" to every item after it. The checklist keeps a change log, so the summary can say it changed and why. insertAfterCurrent is gone. It was written for a user path that does not exist — nothing offers "add a step" — and an unreachable method that mutates the one structure both the user and the model treat as settled is worse than a slightly narrower feature. Two regexes in commonSense were flagged by eslint-plugin-security and both were real rather than noise, which is worth saying because the last two passes reviewed and dismissed every one of these: - DANGLING_REFERENCE had `^\s*` and `\s*$` around an optional character, the classic ambiguous shape. 68ms at 10,000 trailing spaces, 1,660ms at 50,000. The caller trims and the anchors are gone: 0ms. - PATH_TOKEN matches linearly but is scanned with /g, so a string with no match gets one scan per start position — 3,089ms on 50,000 characters of `a/a/a/…`. Input is now bounded to 4,000 characters, the same discipline the TODO list uses on its items. Neither is an attack — it is the user's own composer — but three seconds of frozen extension host is a bug either way. Both are pinned by tests with a 250ms budget. Also removed two eslint-disable directives added earlier in this branch that suppressed nothing. Warnings are back to 25 against a 23 baseline; the two additions are the regexes above, measured and documented in place rather than waved through. 1320 unit tests and 16 integration tests passing, 0 lint errors.
security/sast-report-2026-08-14-0.7.0.md supersedes doc/SAST-0.6.1.md. First pass where no Critical or High is carried forward with a standing risk: npx now confirms, both pathGuard bypasses are closed, and the mocha advisories are fixed rather than deferred — npm audit reports zero across the full tree, not only under --omit=dev. Two findings were opened and closed inside the pass, and both deserve saying out loud because the 0.1.0 and 0.6.1 passes each reviewed this warning class and dismissed all of it. DANGLING_REFERENCE and PATH_TOKEN were genuinely super-linear — 1,660ms and 3,089ms on 50,000 characters. "Reviewed and dismissed" had become the default answer for detect-unsafe-regex, and these two were the cases where it was wrong. A flagged pattern is now measured on adversarial input before it is accepted, and the measurement is written beside the pattern rather than only in the report. retire.js is recorded as NOT run, with the reason and the command to run it. Fetching it would have meant `npx retire` — downloading and executing a package from the registry, which is the exact behaviour the headline finding of this pass exists to require a decision about. Running it silently inside the pass that closed that finding would have been the wrong way to close it. Semgrep's `python -m semgrep` shim prints a deprecation notice and exits without scanning, which reads exactly like a clean run. The report says to invoke the executable directly. 91 rules over 77 targets, 2 findings, both the known child_process call. Both marketing images regenerated at v0.7.0 and checked visually rather than assumed correct. The "New in" tag moved from "Knows your machine" to "Big requests become a checklist" — the card 0.7.0 actually changed — and the source README now records that the tag moves with the release and that only one card ever carries it. ARCHITECTURE.md had 46 modules and 573 unit tests against a real 73 and 1323, and said nothing about how a failing step is handled. It now covers why diagnosis, recovery, and clarification are three modules, and why the ladder is two rungs rather than a round number. 1323 unit tests, 16 integration tests, 0 lint errors, 0 vulnerabilities.
jaymar921
added
documentation
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
1 participant
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.
This pull request introduces HirayaCoder v0.7.0, focusing on making the agent more robust when it encounters errors or ambiguous situations. The main improvements are a new escalation ladder for error handling, better undefined symbol diagnosis, and a structured way for the agent to ask the user for clarification with actionable options. Several security and usability fixes are also included.
Error handling and escalation improvements:
agent/errorRecovery, allowing the agent to diagnose unknown failures, escalate when repeated, and ask the user for guidance with clear options if needed. Asking the user is now the last resort after two failed attempts, preventing the agent from getting stuck or making poor guesses.app/agent/clarification.js, defining how the agent builds, validates, and resolves questions to the user, ensuring every question is actionable, concise, and always includes a recommended option and free-text input.Diagnosis and feedback improvements:
app/agent/scriptDiagnosis.jswith new rules for undefined symbols across JavaScript, Python, and Java, making error messages actionable by naming the missing symbol and suggesting concrete next steps. Thefixfield can now be a function to generate context-specific advice. [1] [2]Security and performance fixes:
core/commonSensethat could freeze the extension host on large input; both are now bounded and tested.Documentation and UI updates:
README.mdto reflect v0.7.0 and the new agent capabilities.CHANGELOG.md, including new features, fixes, and removed unused code.