ADR-0019 (proposal): static verification backstop for generated code - #158
Merged
Conversation
Benchmarking the local coding model against this repo's own production prompts (generation + pr-review) found the model violated existing HARD GUARDRAIL prose (unauthorized external dependency, same class of violation ADR-0009 was written to prevent) and separately produced a guaranteed runtime crash (assignment to a read-only property) that the paired pr-review prompt then approved with zero findings. ADR-0009 previously rejected a pre-commit validation script in favor of prompt-only guardrails, reasoning they were cheaper and addressed the root cause. This new evidence — gathered against a local 7B model rather than the pipeline's configured Groq/Anthropic defaults — shows prompt wording alone isn't sufficient for at least these two defect classes, both of which are mechanically checkable (import allowlist comparison; tsc --noEmit for read-only property misuse) without requiring LLM judgment. This ADR proposes an additive, narrowly-scoped opt-in check rather than overruling ADR-0009 — status is "Proposed", not "Accepted"; a follow-up implementation PR would still be needed if adopted. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9bc70e91f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Correct an overstated claim: only the unauthorized-dependency defect violated an explicit guardrail present at benchmark time. The read-only property assignment was a general correctness failure, not a case of the model ignoring existing guardrail prose — the self-check for that exact pattern was added afterward, in companion PR #155. Clarified that this ADR's proposal is an additional tool-based backstop on top of that prompt-level fix, not a substitute for it. - Fix a real design flaw in the import-allowlist proposal: it must compare against a package.json snapshot taken BEFORE calling the generation LLM, not the post-write working tree — otherwise a single patch that adds both the unauthorized import and a matching package.json entry would pass trivially, defeating the check. - Replace the naive "run tsc --noEmit" type-check proposal with an accurate account of why it doesn't work as stated: tsc rejects combining --project with an explicit file list (TS5042), and project-mode tsc would fail on any pre-existing type error anywhere in the repo, blocking every generated PR regardless of the generated files' own validity. Lists three concrete resolution paths (clean-baseline prerequisite, baseline-error comparison, or a derived isolated tsconfig) as an open implementation question rather than asserting a solution that doesn't hold up. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 71bfceb9e7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- The Context section's fix (only the dependency import violated an explicit guardrail at benchmark time) wasn't propagated to the Alternatives Considered section, which still said the model "violated guardrail prose for exactly these two failure modes" — a leftover contradiction of the same overstated claim. Corrected to distinguish the guardrail-violation case from the review/correctness-failure case explicitly, rather than treating both as ignored explicit instructions. - Removed the ADR-0010 bounded-retry comparison for handling repeated static-verification failure: ADR-0010's retry applies to transient external API errors and simply re-invokes the same call, which would reproduce a deterministic verification failure identically rather than fix it. Replaced with an accurate framing — a genuine regeneration loop (new LLM call with the failure fed back), structurally closer to the existing auto-fix re-trigger cycle than to ADR-0010's mechanism. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Owner
Author
|
@codex review |
koydas
added a commit
that referenced
this pull request
Jul 27, 2026
docs/code-generation.md described the dependency-allowlist injection as active pipeline behavior and linked to docs/adr/0019-static-verification-backstop.md, but neither the injection code (scripts/lib/file_injector.mjs) nor the ADR file exist in this branch — they're delivered by companion PRs #157 and #158, which are separate, not-yet-merged branches. Reworded to describe what THIS prompt change expects/consumes, attribute the actual implementation to PR #157 by reference, and point the ADR mention at PR #158's URL instead of a dead relative path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2208627a40
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
koydas
added a commit
that referenced
this pull request
Jul 27, 2026
…rty crashes (#155) * feat(prompts): sharpen generation guardrails against native-API imports and read-only property crashes Benchmarking the local coding model against these exact prompts surfaced two concrete failure modes: it imported a package (`abort-controller`) for functionality (`AbortController`) that's a native global, and it assigned to `AbortController.signal`, a getter-only property, causing a guaranteed runtime TypeError in strict-mode ES modules. Neither was caught by the existing "never introduce a new external package" guardrail because it had no concrete allowlist to check against, and there was no explicit self-verification step for read-only property assignment. Also generalizes the test-writing requirement (previously scoped only to scripts/, prompts/, .github/workflows/) to apply whenever the issue requests tests or new non-trivial logic is introduced, regardless of path — the narrower scoping let a requested test file for a new hook get silently dropped. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(prompts): address review feedback on generation guardrails - Correct an accuracy bug in the guardrail wording itself: Node built-in modules (fs, path, node:*) still require an explicit import/require statement — they were incorrectly grouped with true no-import globals (AbortController, fetch). Distinguish the two categories explicitly. - Add 7 targeted prompt-regression tests covering the new dependency allowlist reference, the no-import-vs-importable-built-in distinction, the path-independent test-writing requirement, and both self-check clauses (read-only property, cross-render persistence) — previously only covered by generic non-empty/schema-key assertions that couldn't catch removal of these specific guarantees. - Document the expanded generation guardrails in docs/code-generation.md (new "Generation Guardrails" section), since this changes the operator- visible contract of what the pipeline generates. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(docs): stop asserting cross-PR behavior not present in this tree docs/code-generation.md described the dependency-allowlist injection as active pipeline behavior and linked to docs/adr/0019-static-verification-backstop.md, but neither the injection code (scripts/lib/file_injector.mjs) nor the ADR file exist in this branch — they're delivered by companion PRs #157 and #158, which are separate, not-yet-merged branches. Reworded to describe what THIS prompt change expects/consumes, attribute the actual implementation to PR #157 by reference, and point the ADR mention at PR #158's URL instead of a dead relative path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…ification-backstop
…oundary
- The import-allowlist check as written would scan every specifier in the
completed file, which would reject an unrelated, pre-existing import the
generation guardrail already explicitly allows ("already used elsewhere
in the file"). Now snapshots each target file's own pre-generation
imports alongside package.json, and only flags specifiers newly added by
the patch that are neither in the file's own prior imports nor the
package.json snapshot.
- "Runs... before a PR is opened" doesn't describe auto-fix: auto-fix-pr.yml
only runs after a label is applied to an already-open PR and commits/
pushes to its existing branch — there is no "before PR opened" moment on
that path. Reworded to define the auto-fix gate at its actual boundary
(before the commit/push step) and to specify that a failure counts
against the existing 3-attempt bound (MAX_ATTEMPTS in auto_fix_pr.mjs)
rather than opening an unbounded separate retry loop.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54447a71c6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Verified against the actual workflows: code-generation.yml and auto-fix-pr.yml only check out this repo and set up Node — neither installs the target repo's own dependencies via its package manager. Without that, tsc may not be installed at all, and even a global compiler would report every imported module/type declaration as missing, so no clean baseline could ever be established. Added this as an explicit prerequisite (detect package manager, run its install step, before any of the tsc-invocation modes apply) rather than assuming tsconfig.json presence is the only repo-specific knowledge needed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e58f0de8c2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ld-knowledge claim - Added a workspace/monorepo caveat to the import-allowlist check: it only reads the repository-root package.json, so a new file inside a workspace package (e.g. packages/app/) legitimately importing a dependency declared only in that package's own nested package.json would be wrongly flagged. Left as an open implementation detail (nearest-governing-manifest resolution per generated file path) rather than assumed to work. - The Alternatives Considered section still claimed both proposed checks "require no repo-specific build knowledge beyond does a tsconfig.json exist" — contradicted by the earlier fix establishing that tsc also needs the target repo's package manager detected and its install step run first. Reworded to state that plainly rather than understating the tradeoff for the sake of the comparison. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d8e289642
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…the install step The dependency-install prerequisite for the tsc check was described as just "run the package manager's install step" — but npm/yarn/pnpm all run package.json lifecycle scripts (preinstall/install/postinstall) by default, and npm does not set ignore-scripts to true. Running that inside the existing generation/auto-fix job means an unverified package.json (from a malicious or compromised contributor's PR, or the generated patch itself) could execute arbitrary code with the job's GITHUB_TOKEN/AI_PR_TOKEN before any of this ADR's verification runs — defeating the trust boundary the ADR is meant to establish. Now explicitly requires --ignore-scripts (and its yarn/pnpm equivalents) and installing from the pre-generation manifest/ lockfile snapshot, for the same snapshot-timing reason already established for the import-allowlist check. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15c61ebfc5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ification-backstop
The decision described snapshotting "each target file's" pre-generation imports before calling the generation LLM — but target_path values are chosen by the LLM in its response, so which files even need a snapshot isn't knowable at that point without scanning every JS/TS file in the repo. Split the two snapshots by their actual dependency: package.json (repo-wide, response-independent) stays pre-LLM-call; per-target-file import snapshots must happen after validateAiOutput() returns changes (target_path values known) and before writeGeneratedFiles() overwrites them — the actual window available in generate_issue_change.mjs's current flow. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7cb73828cb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… count - The import-allowlist predicate as written would reject a normal relative import (./useThing.js) since it's newly added, absent from package.json, and not a runtime built-in — contradicting generation-user.md's own requirement 7, which explicitly permits relative imports, and blocking routine generated changes whenever this check is enabled. Now explicitly discards relative/absolute-path specifiers before applying the allowlist, which only ever applied to bare package specifiers in intent. - Option (b) for the type-check pass compared raw tsc error counts before/ after the patch. That's unsound: a multi-file patch that introduces one new type error while incidentally fixing an unrelated pre-existing one leaves the count flat or lower, letting exactly the class of defect this ADR targets (the read-only-property crash) slip through undetected. Reworded to compare diagnostic identity (file + line + error code), not count. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 866fa03303
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… frozen installs - The import-allowlist algorithm compared bare specifiers literally against package.json keys, but a public subpath import (react/jsx-runtime, date-fns/format, @scope/pkg/subpath) is authorized by the same manifest entry as the bare package import and won't match without normalizing to the package root first (first path segment, or first two for a @scope/ specifier). Added that normalization step before the comparison. - Options (a) and (b) for the tsc check only see files tsc's own tsconfig.json include/exclude/files/project-references actually cover — a generated file outside that scope is silently absent from the compiled program, so tsc reports success without ever inspecting it, missing the exact defect class this gate exists to catch. Now requires confirming every changed TS path is part of the checked program (cross-check against tsc --listFilesOnly) and falling back to the isolated-config option (c) for any path that isn't. - npm install --ignore-scripts alone still permits rewriting package-lock.json on a stale/older-format lockfile. Since auto-fix-pr.yml stages with git add -A, a lockfile rewrite as a side effect of verification would get committed as if part of the fix. Now requires a frozen/immutable install (npm ci, yarn --immutable, pnpm --frozen-lockfile) combined with --ignore-scripts, not a mutating one. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b18b0dec5c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…iagnostic identity - The specifier classification only exempted relative/absolute paths, but repos commonly resolve project-local imports through package.json's imports field (#alias) or tsconfig.json's compilerOptions.paths (@/utils) — these look like bare package specifiers (a @/-prefixed alias especially resembles a scoped package) and would be wrongly rejected as unauthorized. Now reads both config files as part of the per-file snapshot and exempts any specifier they map, before the bare-package allowlist check applies. - The diagnostic-identity fix from the previous round (file + line + error code) was itself fragile: a generated edit that inserts/removes lines earlier in a file shifts every unrelated pre-existing diagnostic below it to a new line number, which literal line-number comparison would misclassify as a newly introduced error — a false failure on an untouched diagnostic. Reworded to require either mapping baseline diagnostics through the diff's hunk offsets, or a representation less sensitive to line movement (file + error code + enclosing symbol), leaving the choice to the implementation PR rather than asserting a specific fragile scheme. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 04190f7dfa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
abort-controller is a real, published npm package (a pre-Node-15/older- browser polyfill for AbortController) — it just wasn't declared in this project's package.json and wasn't needed since AbortController is a native global. The defect this ADR's evidence rests on is the undeclared/ unauthorized dependency, not a nonexistent package; corrected the claim so the ADR's motivating benchmark data is described accurately. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
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.
Summary
qwen2.5-coder:7b-instruct-q4_0) against this repo's actual production prompts (see Sharpen generation guardrails: native-API imports and read-only property crashes #155, Generalize test-coverage gate and add named defect checklist to review/auto-fix prompts #156, Inject package.json dependency allowlist into generation context #157): the model violated existing HARD GUARDRAIL prose (unauthorized dependency — the same failure class ADR-0009 was written to prevent) and produced a guaranteed-crash bug (assignment to a read-only property) that the pairedpr-review-system.mdprompt then approved with zero findings.tsc --noEmitwhen atsconfig.jsonexists) for the two specific defect classes shown to survive prompt-only enforcement, scoped explicitly to acknowledge the evidence comes from a local model rather than this pipeline's configured Groq/Anthropic defaults.Test plan
node --test scripts/tests/*.test.mjspasses (534/534) — docs-only change, no code touched.Companion to #155, #156, #157 (same benchmark session).
🤖 Generated with Claude Code