From d9bc70e91f641a7b9f756806567d1277b47466bb Mon Sep 17 00:00:00 2001 From: koydas Date: Mon, 27 Jul 2026 17:28:38 +0000 Subject: [PATCH 01/12] docs(adr): propose static verification backstop for generated code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- CHANGELOG.md | 1 + docs/adr/0019-static-verification-backstop.md | 48 +++++++++++++++++++ docs/adr/README.md | 1 + 3 files changed, 50 insertions(+) create mode 100644 docs/adr/0019-static-verification-backstop.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 43edc2c..44773a2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ Entries are grouped by date. Add new entries under `[Unreleased]`. ## [Unreleased] ### Added +- ADR-0019 (proposed): Static verification backstop for generated code — proposes an import-allowlist check and opt-in `tsc --noEmit` gate after generation/auto-fix, motivated by a benchmark session where a local coding model violated existing prompt-only guardrails (unauthorized dependency import, read-only property assignment causing a guaranteed runtime crash) and the paired PR-review prompt approved the resulting diff. - Structured end-to-end observability: `scripts/lib/observability.mjs` provides `log()` (structured JSON to stderr, per-event) and `createTracer()` (incremental per-run trace file at `observability/traces/.json`). All four pipeline stages (issue_validation, code_gen/pr_prepare, review, autofix) now emit required events with `duration_ms` on terminal events. Error-level events emit `::error::` GitHub Actions annotations automatically (ADR-0018). - Run trace artifact: each of the four main workflows uploads `run-trace-` as a GitHub Actions artifact (`if: always()`), so trace files are preserved even on failure. - `scripts/tests/observability.test.mjs` — 20 unit tests covering `log()` schema, GHA annotations, error containment, tracer happy-path and I/O failure isolation. diff --git a/docs/adr/0019-static-verification-backstop.md b/docs/adr/0019-static-verification-backstop.md new file mode 100644 index 0000000..27578f6 --- /dev/null +++ b/docs/adr/0019-static-verification-backstop.md @@ -0,0 +1,48 @@ +# ADR-0019: Static verification backstop for generated code (proposal) + +- **Date:** 2026-07-27 +- **Status:** Proposed + +## Context + +ADR-0009 addressed LLM guardrail violations (test-suite deletion, module-format corruption, unauthorized dependencies, signature changes) entirely through prompt wording, and explicitly rejected adding a pre-commit validation script as an alternative: "adds infra complexity and latency to every auto-fix run. The prompt guardrails are cheaper and address the root cause." + +A benchmark session run against a locally-hosted 7B coding model (`qwen2.5-coder:7b-instruct-q4_0`, used here as a stand-in for "a capable but not frontier-tier LLM") fed the exact production `generation-system.md`/`generation-user.md` prompts a synthetic issue requesting a React hook. The output: + +1. Imported `AbortController` from a nonexistent `abort-controller` npm package — the same class of violation ADR-0009's "never introduce external packages" guardrail was written to prevent (that ADR's motivating incident was `require('nyc')` introduced without being in `package.json`). +2. Assigned to `AbortController.prototype.signal`, a getter-only accessor. Verified empirically in a real ES-module environment (Node's native ESM loader, matching browser strict-mode semantics): this throws `TypeError` and crashes the entire React component tree on first use. + +Both violations occurred despite explicit HARD GUARDRAIL prose forbidding exactly this behavior. The generated diff was then run through the production `pr-review-system.md` prompt (same benchmark session) and returned `APPROVED`, `Issues Found: None` — the paired review step did not catch either defect, nor the complete absence of unit tests the issue had explicitly requested. + +This reopens the question ADR-0009 settled by prompt-only means: **prompt wording is not a sufficient backstop by itself for at least some classes of defect, for at least some models this pipeline might run against.** Two of the observed defects are mechanically checkable by tools that already exist and require no LLM judgment call: +- An unauthorized import is a straightforward static comparison against `package.json` (see #157, which already gives the model a concrete allowlist — but a generation-time constraint on the *prompt* doesn't guarantee compliance, as shown here). +- A read-only property assignment on a well-known built-in (`AbortController.signal`, `Response.body`, etc.) is exactly the class of error a type checker (`tsc --noEmit`) is designed to catch, for any repo already using TypeScript with the DOM lib. + +## Decision (proposed) + +Add an optional, opt-in static verification step that runs immediately after `writeGeneratedFiles()` in the code-generation stage (and equivalently after auto-fix), before a PR is opened: + +1. **Import allowlist check** (applies to any repo, no per-repo config needed beyond `package.json`): for each generated/modified JS/TS file, extract `import`/`require` specifiers and flag any bare (non-relative) specifier that is neither in `package.json`'s `dependencies`/`devDependencies` nor in a small maintained list of language/runtime built-ins. This is a regex/AST-level check, not a full build — cheap, deterministic, language-aware only for JS/TS. +2. **Type-check pass** (opt-in per target repo, since not every repo this pipeline touches is TypeScript): if the target repo has a `tsconfig.json`, run `tsc --noEmit` scoped to the generated files' containing project before opening the PR. Treat a failure as equivalent to a generation failure (per AGENTS.md's "fail fast on external API errors; never open PRs on failed generation" — extend that principle to "never open PRs on code that doesn't type-check when a type checker is available"). + +Both checks are advisory-to-blocking: a failure prevents the PR from being opened (or triggers a re-generation attempt, mirroring the existing auto-fix retry loop), rather than being left for the AI reviewer to notice — this benchmark's finding is precisely that the reviewer cannot be relied upon to notice it reliably. + +## Alternatives Considered + +**Rely solely on sharper prompt wording** (see companion PRs #155/#156): cheaper, and does measurably help — but this ADR's own motivating data was gathered *against* prompts already containing explicit, unambiguous guardrail prose for exactly these two failure modes, and the model violated them anyway. Prompt sharpening is worth doing regardless (it may reduce the rate), but this benchmark is evidence it cannot be assumed sufficient on its own for every model this pipeline might use. + +**Do nothing / accept the risk, matching ADR-0009's original latency/complexity tradeoff**: reasonable if the pipeline only ever targets frontier-tier hosted models (Groq/Anthropic, the current defaults per AGENTS.md) where this failure rate may be lower or unobserved. Worth noting this ADR's evidence comes from a local 7B model, not the pipeline's configured defaults — the proposal here is explicitly scoped as opt-in for exactly that reason, not a blanket requirement. + +**Full CI build/test suite before PR open, for every generated change**: strictly more thorough but reintroduces the latency/complexity concern ADR-0009 weighed against, and requires per-repo build tooling knowledge this pipeline doesn't currently have. The import-allowlist and `tsc --noEmit` checks proposed here are a narrower, cheaper subset chosen specifically because they map to the two concrete defects observed and require no repo-specific build knowledge beyond "does a `tsconfig.json` exist." + +## Consequences + +- ✅ Catches the two defect classes this ADR is motivated by deterministically, without depending on either the generator or the reviewer LLM noticing them. +- ✅ Consistent with the existing "fail fast" principle in AGENTS.md — extends it from "external API errors" to "generated code that provably doesn't satisfy its own stated constraints." +- ⚠️ Reopens the exact tradeoff ADR-0009 already made a call on (latency/complexity vs. prompt-only enforcement) — this ADR does not overrule ADR-0009's guardrail prose, it proposes an additional layer on top, scoped narrowly enough that the original objection (general pre-commit validation script) doesn't fully apply. +- ⚠️ The `tsc --noEmit` check only fires for TypeScript repos with a `tsconfig.json`; it is not a general-purpose correctness gate and won't catch logic bugs outside its type-checkable surface (e.g. the race-condition class of bug also observed in the same benchmark session is not something a type checker catches). +- ⚠️ Requires deciding what happens on repeated failure (retry generation, same as auto-fix's existing bounded-retry pattern per ADR-0010, or surface to a human) — left for the implementation PR to resolve, not decided here. + +## Related benchmark data + +The full benchmark session (generation + review + targeted-prompt iteration, run against `qwen2.5-coder:7b-instruct-q4_0` outside this repo) is not preserved in this repository; this ADR summarizes only the two findings directly relevant to this repo's guardrail design. diff --git a/docs/adr/README.md b/docs/adr/README.md index 7d101f7..69dd3b0 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -22,3 +22,4 @@ Architecture Decision Records (ADR) for the MVP Issue → AI → PR automation. - [ADR-0016: Changelog CI gate for entrypoint scripts and ADR files](./0016-changelog-ci-gate.md) - [ADR-0017: Configurable per-stage token budget in `config/models.yaml`](./0017-configurable-token-budget.md) - [ADR-0018: Structured observability — JSON events to stderr + per-run trace files](./0018-structured-observability.md) +- [ADR-0019: Static verification backstop for generated code (proposal)](./0019-static-verification-backstop.md) From 71bfceb9e7bd6cb6e825b9567066471e44648bc1 Mon Sep 17 00:00:00 2001 From: koydas Date: Mon, 27 Jul 2026 17:45:39 +0000 Subject: [PATCH 02/12] fix(adr): address review feedback on ADR-0019 proposal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 --- docs/adr/0019-static-verification-backstop.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/adr/0019-static-verification-backstop.md b/docs/adr/0019-static-verification-backstop.md index 27578f6..9e73069 100644 --- a/docs/adr/0019-static-verification-backstop.md +++ b/docs/adr/0019-static-verification-backstop.md @@ -12,7 +12,7 @@ A benchmark session run against a locally-hosted 7B coding model (`qwen2.5-coder 1. Imported `AbortController` from a nonexistent `abort-controller` npm package — the same class of violation ADR-0009's "never introduce external packages" guardrail was written to prevent (that ADR's motivating incident was `require('nyc')` introduced without being in `package.json`). 2. Assigned to `AbortController.prototype.signal`, a getter-only accessor. Verified empirically in a real ES-module environment (Node's native ESM loader, matching browser strict-mode semantics): this throws `TypeError` and crashes the entire React component tree on first use. -Both violations occurred despite explicit HARD GUARDRAIL prose forbidding exactly this behavior. The generated diff was then run through the production `pr-review-system.md` prompt (same benchmark session) and returned `APPROVED`, `Issues Found: None` — the paired review step did not catch either defect, nor the complete absence of unit tests the issue had explicitly requested. +Only the first of these violated an explicit guardrail already present in the prompt at benchmark time (`generation-system.md`'s HARD GUARDRAILS forbid introducing new external packages). The read-only-property assignment was not covered by any existing guardrail prose — it's a general code-correctness failure the review step should have caught on its own merits, not a case of the model ignoring an explicit instruction. (Companion PR #155 has since added an explicit self-check for this exact pattern, closing that specific gap at the prompt level — this ADR's static-verification proposal is an additional, tool-based backstop on top of that, not a substitute for it.) The generated diff was then run through the production `pr-review-system.md` prompt (same benchmark session) and returned `APPROVED`, `Issues Found: None` — the paired review step did not catch either defect, nor the complete absence of unit tests the issue had explicitly requested. This reopens the question ADR-0009 settled by prompt-only means: **prompt wording is not a sufficient backstop by itself for at least some classes of defect, for at least some models this pipeline might run against.** Two of the observed defects are mechanically checkable by tools that already exist and require no LLM judgment call: - An unauthorized import is a straightforward static comparison against `package.json` (see #157, which already gives the model a concrete allowlist — but a generation-time constraint on the *prompt* doesn't guarantee compliance, as shown here). @@ -22,8 +22,8 @@ This reopens the question ADR-0009 settled by prompt-only means: **prompt wordin Add an optional, opt-in static verification step that runs immediately after `writeGeneratedFiles()` in the code-generation stage (and equivalently after auto-fix), before a PR is opened: -1. **Import allowlist check** (applies to any repo, no per-repo config needed beyond `package.json`): for each generated/modified JS/TS file, extract `import`/`require` specifiers and flag any bare (non-relative) specifier that is neither in `package.json`'s `dependencies`/`devDependencies` nor in a small maintained list of language/runtime built-ins. This is a regex/AST-level check, not a full build — cheap, deterministic, language-aware only for JS/TS. -2. **Type-check pass** (opt-in per target repo, since not every repo this pipeline touches is TypeScript): if the target repo has a `tsconfig.json`, run `tsc --noEmit` scoped to the generated files' containing project before opening the PR. Treat a failure as equivalent to a generation failure (per AGENTS.md's "fail fast on external API errors; never open PRs on failed generation" — extend that principle to "never open PRs on code that doesn't type-check when a type checker is available"). +1. **Import allowlist check** (applies to any repo, no per-repo config needed beyond `package.json`): snapshot `package.json`'s dependency fields **before** calling the generation LLM (the same read already performed to build the "Allowed npm dependencies" context block, see #157). For each generated/modified JS/TS file, extract `import`/`require` specifiers and flag any bare (non-relative) specifier that is neither in that pre-generation snapshot nor in a small maintained list of language/runtime built-ins. Comparing against the pre-generation snapshot, not the post-write working tree, is load-bearing: since the model's own `changes` can include a rewritten `package.json` (up to 6 files per the existing HARD LIMIT), checking against the working-tree file after `writeGeneratedFiles()` would let the model add both the import and the corresponding manifest entry in the same patch and pass trivially, which defeats the check's purpose. This is a regex/AST-level check, not a full build — cheap, deterministic, language-aware only for JS/TS. +2. **Type-check pass** (opt-in per target repo, since not every repo this pipeline touches is TypeScript): if the target repo has a `tsconfig.json`, running `tsc --noEmit` isn't a drop-in solution — `tsc`'s CLI rejects combining `--project` with an explicit file list (error TS5042), so scoping to just the generated files isn't directly available, and running full project-mode `tsc` would fail on *any* pre-existing type error anywhere in the project, blocking every generated PR regardless of whether the generated files themselves are valid. A workable version needs one of: (a) a clean-baseline prerequisite (only enable this check for repos that already pass `tsc --noEmit` on `main`), (b) a baseline-error-count comparison (run `tsc --noEmit` before and after applying the generated changes, on the whole project, and fail only if the error count/set grows), or (c) a derived, isolated `tsconfig.json` (generated on the fly, `include`-scoped to just the changed files plus their transitive local imports) so project mode has a narrow enough surface. This needs to be resolved concretely in the implementation PR, not assumed away — treat "which of (a)/(b)/(c)" as an open implementation question this ADR does not settle. Both checks are advisory-to-blocking: a failure prevents the PR from being opened (or triggers a re-generation attempt, mirroring the existing auto-fix retry loop), rather than being left for the AI reviewer to notice — this benchmark's finding is precisely that the reviewer cannot be relied upon to notice it reliably. From 2208627a4093da962b0ff47bf13a920fdf2ecb01 Mon Sep 17 00:00:00 2001 From: koydas Date: Mon, 27 Jul 2026 17:58:50 +0000 Subject: [PATCH 03/12] fix(adr): resolve remaining inconsistencies in ADR-0019 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 --- docs/adr/0019-static-verification-backstop.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/adr/0019-static-verification-backstop.md b/docs/adr/0019-static-verification-backstop.md index 9e73069..16ac45e 100644 --- a/docs/adr/0019-static-verification-backstop.md +++ b/docs/adr/0019-static-verification-backstop.md @@ -29,7 +29,7 @@ Both checks are advisory-to-blocking: a failure prevents the PR from being opene ## Alternatives Considered -**Rely solely on sharper prompt wording** (see companion PRs #155/#156): cheaper, and does measurably help — but this ADR's own motivating data was gathered *against* prompts already containing explicit, unambiguous guardrail prose for exactly these two failure modes, and the model violated them anyway. Prompt sharpening is worth doing regardless (it may reduce the rate), but this benchmark is evidence it cannot be assumed sufficient on its own for every model this pipeline might use. +**Rely solely on sharper prompt wording** (see companion PRs #155/#156): cheaper, and does measurably help — but this ADR's own motivating data includes a case (the unauthorized-dependency import) where the model violated guardrail prose that was already explicit and unambiguous at benchmark time. The second defect (the read-only property assignment) wasn't covered by any guardrail at benchmark time, so it doesn't independently demonstrate prompt-wording *violation* — but it does demonstrate the paired review step failing to catch a plain correctness bug on its own merits, which is a related but distinct argument for a tool-based backstop: sharper prompts (per #155/#156) can close the first gap, but nothing in prompt wording alone guarantees the second kind of failure gets caught either. Prompt sharpening is worth doing regardless (it may reduce the rate of both), but this benchmark is evidence it cannot be assumed sufficient on its own for every model this pipeline might use. **Do nothing / accept the risk, matching ADR-0009's original latency/complexity tradeoff**: reasonable if the pipeline only ever targets frontier-tier hosted models (Groq/Anthropic, the current defaults per AGENTS.md) where this failure rate may be lower or unobserved. Worth noting this ADR's evidence comes from a local 7B model, not the pipeline's configured defaults — the proposal here is explicitly scoped as opt-in for exactly that reason, not a blanket requirement. @@ -41,7 +41,7 @@ Both checks are advisory-to-blocking: a failure prevents the PR from being opene - ✅ Consistent with the existing "fail fast" principle in AGENTS.md — extends it from "external API errors" to "generated code that provably doesn't satisfy its own stated constraints." - ⚠️ Reopens the exact tradeoff ADR-0009 already made a call on (latency/complexity vs. prompt-only enforcement) — this ADR does not overrule ADR-0009's guardrail prose, it proposes an additional layer on top, scoped narrowly enough that the original objection (general pre-commit validation script) doesn't fully apply. - ⚠️ The `tsc --noEmit` check only fires for TypeScript repos with a `tsconfig.json`; it is not a general-purpose correctness gate and won't catch logic bugs outside its type-checkable surface (e.g. the race-condition class of bug also observed in the same benchmark session is not something a type checker catches). -- ⚠️ Requires deciding what happens on repeated failure (retry generation, same as auto-fix's existing bounded-retry pattern per ADR-0010, or surface to a human) — left for the implementation PR to resolve, not decided here. +- ⚠️ Requires deciding what happens on repeated failure. Note this is a distinct problem from ADR-0010's bounded retry, which applies to *transient external API calls* (`scripts/lib/retry.mjs` re-invokes the same operation after a network/rate-limit blip) — a deterministic static-verification failure will reproduce identically on a bare retry of the same call, so ADR-0010's mechanism doesn't apply here. What's needed instead is a genuine regeneration loop: a new LLM call with the verification failure fed back as feedback (structurally closer to the existing auto-fix label-driven re-trigger cycle than to ADR-0010's retry), or surfacing to a human after some bound. Left for the implementation PR to define concretely, not decided here. ## Related benchmark data From 54447a71c6449d1faf5d02a9be29ad590d58ad40 Mon Sep 17 00:00:00 2001 From: koydas Date: Mon, 27 Jul 2026 18:17:21 +0000 Subject: [PATCH 04/12] fix(adr): scope import check to added specifiers, fix auto-fix gate boundary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 --- docs/adr/0019-static-verification-backstop.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/adr/0019-static-verification-backstop.md b/docs/adr/0019-static-verification-backstop.md index 16ac45e..19b275e 100644 --- a/docs/adr/0019-static-verification-backstop.md +++ b/docs/adr/0019-static-verification-backstop.md @@ -20,12 +20,12 @@ This reopens the question ADR-0009 settled by prompt-only means: **prompt wordin ## Decision (proposed) -Add an optional, opt-in static verification step that runs immediately after `writeGeneratedFiles()` in the code-generation stage (and equivalently after auto-fix), before a PR is opened: +Add an optional, opt-in static verification step. For the generation stage, it runs immediately after `writeGeneratedFiles()`, before a PR is opened. For auto-fix, there is no "before a PR is opened" moment — `auto-fix-pr.yml` only runs after a label is applied to an *already-open* PR, checks out its existing branch, and commits/pushes generated files to it — so the equivalent gate there runs immediately after auto-fix writes its generated files, before its commit/push step: -1. **Import allowlist check** (applies to any repo, no per-repo config needed beyond `package.json`): snapshot `package.json`'s dependency fields **before** calling the generation LLM (the same read already performed to build the "Allowed npm dependencies" context block, see #157). For each generated/modified JS/TS file, extract `import`/`require` specifiers and flag any bare (non-relative) specifier that is neither in that pre-generation snapshot nor in a small maintained list of language/runtime built-ins. Comparing against the pre-generation snapshot, not the post-write working tree, is load-bearing: since the model's own `changes` can include a rewritten `package.json` (up to 6 files per the existing HARD LIMIT), checking against the working-tree file after `writeGeneratedFiles()` would let the model add both the import and the corresponding manifest entry in the same patch and pass trivially, which defeats the check's purpose. This is a regex/AST-level check, not a full build — cheap, deterministic, language-aware only for JS/TS. +1. **Import allowlist check** (applies to any repo, no per-repo config needed beyond `package.json`): snapshot `package.json`'s dependency fields **before** calling the generation LLM (the same read already performed to build the "Allowed npm dependencies" context block, see #157), and snapshot each target file's own pre-generation import/require specifiers alongside it. After writing, extract `import`/`require` specifiers from each generated/modified JS/TS file and flag only specifiers that are newly added by this patch (not present in that file's own pre-generation snapshot) AND are neither in the pre-generation `package.json` snapshot nor in a small maintained list of language/runtime built-ins. Checking only newly-added specifiers matters: an existing file may legitimately already import a package absent from the root manifest (workspace-local dependency, or one this scan doesn't otherwise see) — the generation guardrail explicitly allows imports "already used elsewhere in the file", and scanning every specifier in the completed file rather than only what changed would reject that legitimate, unrelated-to-this-patch import. Comparing against the pre-generation `package.json` snapshot, not the post-write working tree, is separately load-bearing: since the model's own `changes` can include a rewritten `package.json` (up to 6 files per the existing HARD LIMIT), checking against the working-tree file after `writeGeneratedFiles()` would let the model add both the import and the corresponding manifest entry in the same patch and pass trivially, which defeats the check's purpose. This is a regex/AST-level check, not a full build — cheap, deterministic, language-aware only for JS/TS. 2. **Type-check pass** (opt-in per target repo, since not every repo this pipeline touches is TypeScript): if the target repo has a `tsconfig.json`, running `tsc --noEmit` isn't a drop-in solution — `tsc`'s CLI rejects combining `--project` with an explicit file list (error TS5042), so scoping to just the generated files isn't directly available, and running full project-mode `tsc` would fail on *any* pre-existing type error anywhere in the project, blocking every generated PR regardless of whether the generated files themselves are valid. A workable version needs one of: (a) a clean-baseline prerequisite (only enable this check for repos that already pass `tsc --noEmit` on `main`), (b) a baseline-error-count comparison (run `tsc --noEmit` before and after applying the generated changes, on the whole project, and fail only if the error count/set grows), or (c) a derived, isolated `tsconfig.json` (generated on the fly, `include`-scoped to just the changed files plus their transitive local imports) so project mode has a narrow enough surface. This needs to be resolved concretely in the implementation PR, not assumed away — treat "which of (a)/(b)/(c)" as an open implementation question this ADR does not settle. -Both checks are advisory-to-blocking: a failure prevents the PR from being opened (or triggers a re-generation attempt, mirroring the existing auto-fix retry loop), rather than being left for the AI reviewer to notice — this benchmark's finding is precisely that the reviewer cannot be relied upon to notice it reliably. +For the generation stage, both checks are advisory-to-blocking: a failure prevents the PR from being opened (or triggers a re-generation attempt), rather than being left for the AI reviewer to notice. For auto-fix, a failure should count against the existing 3-attempt bound (`MAX_ATTEMPTS` in `auto_fix_pr.mjs`, tracked via `auto-fix-attempt-N` labels) the same way an unhelpful fix already does, rather than opening a new, separate retry budget — and whether it also changes the `changes-requested`/`review-approved` labeling is an open implementation question, not decided here. Either way, this benchmark's finding is precisely that the reviewer cannot be relied upon to notice these defects reliably on its own. ## Alternatives Considered From e58f0de8c2a63259e795c8a596693b01a0195baf Mon Sep 17 00:00:00 2001 From: koydas Date: Mon, 27 Jul 2026 18:47:14 +0000 Subject: [PATCH 05/12] fix(adr): account for missing dependency installation before tsc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- docs/adr/0019-static-verification-backstop.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/adr/0019-static-verification-backstop.md b/docs/adr/0019-static-verification-backstop.md index 19b275e..6bebb39 100644 --- a/docs/adr/0019-static-verification-backstop.md +++ b/docs/adr/0019-static-verification-backstop.md @@ -23,7 +23,7 @@ This reopens the question ADR-0009 settled by prompt-only means: **prompt wordin Add an optional, opt-in static verification step. For the generation stage, it runs immediately after `writeGeneratedFiles()`, before a PR is opened. For auto-fix, there is no "before a PR is opened" moment — `auto-fix-pr.yml` only runs after a label is applied to an *already-open* PR, checks out its existing branch, and commits/pushes generated files to it — so the equivalent gate there runs immediately after auto-fix writes its generated files, before its commit/push step: 1. **Import allowlist check** (applies to any repo, no per-repo config needed beyond `package.json`): snapshot `package.json`'s dependency fields **before** calling the generation LLM (the same read already performed to build the "Allowed npm dependencies" context block, see #157), and snapshot each target file's own pre-generation import/require specifiers alongside it. After writing, extract `import`/`require` specifiers from each generated/modified JS/TS file and flag only specifiers that are newly added by this patch (not present in that file's own pre-generation snapshot) AND are neither in the pre-generation `package.json` snapshot nor in a small maintained list of language/runtime built-ins. Checking only newly-added specifiers matters: an existing file may legitimately already import a package absent from the root manifest (workspace-local dependency, or one this scan doesn't otherwise see) — the generation guardrail explicitly allows imports "already used elsewhere in the file", and scanning every specifier in the completed file rather than only what changed would reject that legitimate, unrelated-to-this-patch import. Comparing against the pre-generation `package.json` snapshot, not the post-write working tree, is separately load-bearing: since the model's own `changes` can include a rewritten `package.json` (up to 6 files per the existing HARD LIMIT), checking against the working-tree file after `writeGeneratedFiles()` would let the model add both the import and the corresponding manifest entry in the same patch and pass trivially, which defeats the check's purpose. This is a regex/AST-level check, not a full build — cheap, deterministic, language-aware only for JS/TS. -2. **Type-check pass** (opt-in per target repo, since not every repo this pipeline touches is TypeScript): if the target repo has a `tsconfig.json`, running `tsc --noEmit` isn't a drop-in solution — `tsc`'s CLI rejects combining `--project` with an explicit file list (error TS5042), so scoping to just the generated files isn't directly available, and running full project-mode `tsc` would fail on *any* pre-existing type error anywhere in the project, blocking every generated PR regardless of whether the generated files themselves are valid. A workable version needs one of: (a) a clean-baseline prerequisite (only enable this check for repos that already pass `tsc --noEmit` on `main`), (b) a baseline-error-count comparison (run `tsc --noEmit` before and after applying the generated changes, on the whole project, and fail only if the error count/set grows), or (c) a derived, isolated `tsconfig.json` (generated on the fly, `include`-scoped to just the changed files plus their transitive local imports) so project mode has a narrow enough surface. This needs to be resolved concretely in the implementation PR, not assumed away — treat "which of (a)/(b)/(c)" as an open implementation question this ADR does not settle. +2. **Type-check pass** (opt-in per target repo, since not every repo this pipeline touches is TypeScript): if the target repo has a `tsconfig.json`, running `tsc --noEmit` isn't a drop-in solution for several reasons. First, a prerequisite this proposal initially glossed over: `code-generation.yml` and `auto-fix-pr.yml` currently only check out the repository and set up Node for *this* repo (`autonomous-dev-loop` itself) — neither installs the *target* repo's dependencies via its package manager (npm/yarn/pnpm) and lockfile. Without that, `tsc` may not even be installed, and even a globally-available compiler would immediately report every imported module and type declaration as missing, so no baseline could ever pass. Enabling this check therefore first requires detecting the target repo's package manager and running its install step (or invoking a repo-configured validation command) before any of the following applies. Second, even with dependencies installed, `tsc`'s CLI rejects combining `--project` with an explicit file list (error TS5042), so scoping to just the generated files isn't directly available, and running full project-mode `tsc` would fail on *any* pre-existing type error anywhere in the project, blocking every generated PR regardless of whether the generated files themselves are valid. A workable version needs one of: (a) a clean-baseline prerequisite (only enable this check for repos that already pass `tsc --noEmit` on `main`, post-install), (b) a baseline-error-count comparison (run `tsc --noEmit` before and after applying the generated changes, on the whole project, and fail only if the error count/set grows), or (c) a derived, isolated `tsconfig.json` (generated on the fly, `include`-scoped to just the changed files plus their transitive local imports) so project mode has a narrow enough surface. This needs to be resolved concretely in the implementation PR, not assumed away — treat both the dependency-installation step and "which of (a)/(b)/(c)" as open implementation questions this ADR does not settle. For the generation stage, both checks are advisory-to-blocking: a failure prevents the PR from being opened (or triggers a re-generation attempt), rather than being left for the AI reviewer to notice. For auto-fix, a failure should count against the existing 3-attempt bound (`MAX_ATTEMPTS` in `auto_fix_pr.mjs`, tracked via `auto-fix-attempt-N` labels) the same way an unhelpful fix already does, rather than opening a new, separate retry budget — and whether it also changes the `changes-requested`/`review-approved` labeling is an open implementation question, not decided here. Either way, this benchmark's finding is precisely that the reviewer cannot be relied upon to notice these defects reliably on its own. From 9d8e289642a67d3f5bd935e7cbe64102c7521533 Mon Sep 17 00:00:00 2001 From: koydas Date: Mon, 27 Jul 2026 18:55:13 +0000 Subject: [PATCH 06/12] fix(adr): flag workspace-nested manifests and remove contradicted build-knowledge claim MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 --- docs/adr/0019-static-verification-backstop.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/adr/0019-static-verification-backstop.md b/docs/adr/0019-static-verification-backstop.md index 6bebb39..67c5a3c 100644 --- a/docs/adr/0019-static-verification-backstop.md +++ b/docs/adr/0019-static-verification-backstop.md @@ -22,7 +22,7 @@ This reopens the question ADR-0009 settled by prompt-only means: **prompt wordin Add an optional, opt-in static verification step. For the generation stage, it runs immediately after `writeGeneratedFiles()`, before a PR is opened. For auto-fix, there is no "before a PR is opened" moment — `auto-fix-pr.yml` only runs after a label is applied to an *already-open* PR, checks out its existing branch, and commits/pushes generated files to it — so the equivalent gate there runs immediately after auto-fix writes its generated files, before its commit/push step: -1. **Import allowlist check** (applies to any repo, no per-repo config needed beyond `package.json`): snapshot `package.json`'s dependency fields **before** calling the generation LLM (the same read already performed to build the "Allowed npm dependencies" context block, see #157), and snapshot each target file's own pre-generation import/require specifiers alongside it. After writing, extract `import`/`require` specifiers from each generated/modified JS/TS file and flag only specifiers that are newly added by this patch (not present in that file's own pre-generation snapshot) AND are neither in the pre-generation `package.json` snapshot nor in a small maintained list of language/runtime built-ins. Checking only newly-added specifiers matters: an existing file may legitimately already import a package absent from the root manifest (workspace-local dependency, or one this scan doesn't otherwise see) — the generation guardrail explicitly allows imports "already used elsewhere in the file", and scanning every specifier in the completed file rather than only what changed would reject that legitimate, unrelated-to-this-patch import. Comparing against the pre-generation `package.json` snapshot, not the post-write working tree, is separately load-bearing: since the model's own `changes` can include a rewritten `package.json` (up to 6 files per the existing HARD LIMIT), checking against the working-tree file after `writeGeneratedFiles()` would let the model add both the import and the corresponding manifest entry in the same patch and pass trivially, which defeats the check's purpose. This is a regex/AST-level check, not a full build — cheap, deterministic, language-aware only for JS/TS. +1. **Import allowlist check** (root-manifest scope; per-repo config beyond `package.json` is only needed for workspace/monorepo setups, see caveat below): snapshot `package.json`'s dependency fields **before** calling the generation LLM (the same read already performed to build the "Allowed npm dependencies" context block, see #157), and snapshot each target file's own pre-generation import/require specifiers alongside it. After writing, extract `import`/`require` specifiers from each generated/modified JS/TS file and flag only specifiers that are newly added by this patch (not present in that file's own pre-generation snapshot) AND are neither in the pre-generation `package.json` snapshot nor in a small maintained list of language/runtime built-ins. Checking only newly-added specifiers matters: an existing file may legitimately already import a package absent from the root manifest (workspace-local dependency, or one this scan doesn't otherwise see) — the generation guardrail explicitly allows imports "already used elsewhere in the file", and scanning every specifier in the completed file rather than only what changed would reject that legitimate, unrelated-to-this-patch import. Comparing against the pre-generation `package.json` snapshot, not the post-write working tree, is separately load-bearing: since the model's own `changes` can include a rewritten `package.json` (up to 6 files per the existing HARD LIMIT), checking against the working-tree file after `writeGeneratedFiles()` would let the model add both the import and the corresponding manifest entry in the same patch and pass trivially, which defeats the check's purpose. This is a regex/AST-level check, not a full build — cheap, deterministic, language-aware only for JS/TS. **Workspace/monorepo caveat:** this check only reads the repository-root manifest (matching #157's scope). A *new* file inside a workspace package (e.g. `packages/app/`) that legitimately imports a dependency declared only in that package's own nested `package.json` — not the root one — would be wrongly flagged, since it has no pre-generation snapshot of its own to fall back on (it doesn't exist yet) and the root manifest doesn't list it. Resolving this requires snapshotting the nearest governing `package.json` per generated file path, not just the root one — left as an open implementation detail, not assumed to already work for every repository layout. 2. **Type-check pass** (opt-in per target repo, since not every repo this pipeline touches is TypeScript): if the target repo has a `tsconfig.json`, running `tsc --noEmit` isn't a drop-in solution for several reasons. First, a prerequisite this proposal initially glossed over: `code-generation.yml` and `auto-fix-pr.yml` currently only check out the repository and set up Node for *this* repo (`autonomous-dev-loop` itself) — neither installs the *target* repo's dependencies via its package manager (npm/yarn/pnpm) and lockfile. Without that, `tsc` may not even be installed, and even a globally-available compiler would immediately report every imported module and type declaration as missing, so no baseline could ever pass. Enabling this check therefore first requires detecting the target repo's package manager and running its install step (or invoking a repo-configured validation command) before any of the following applies. Second, even with dependencies installed, `tsc`'s CLI rejects combining `--project` with an explicit file list (error TS5042), so scoping to just the generated files isn't directly available, and running full project-mode `tsc` would fail on *any* pre-existing type error anywhere in the project, blocking every generated PR regardless of whether the generated files themselves are valid. A workable version needs one of: (a) a clean-baseline prerequisite (only enable this check for repos that already pass `tsc --noEmit` on `main`, post-install), (b) a baseline-error-count comparison (run `tsc --noEmit` before and after applying the generated changes, on the whole project, and fail only if the error count/set grows), or (c) a derived, isolated `tsconfig.json` (generated on the fly, `include`-scoped to just the changed files plus their transitive local imports) so project mode has a narrow enough surface. This needs to be resolved concretely in the implementation PR, not assumed away — treat both the dependency-installation step and "which of (a)/(b)/(c)" as open implementation questions this ADR does not settle. For the generation stage, both checks are advisory-to-blocking: a failure prevents the PR from being opened (or triggers a re-generation attempt), rather than being left for the AI reviewer to notice. For auto-fix, a failure should count against the existing 3-attempt bound (`MAX_ATTEMPTS` in `auto_fix_pr.mjs`, tracked via `auto-fix-attempt-N` labels) the same way an unhelpful fix already does, rather than opening a new, separate retry budget — and whether it also changes the `changes-requested`/`review-approved` labeling is an open implementation question, not decided here. Either way, this benchmark's finding is precisely that the reviewer cannot be relied upon to notice these defects reliably on its own. @@ -33,7 +33,7 @@ For the generation stage, both checks are advisory-to-blocking: a failure preven **Do nothing / accept the risk, matching ADR-0009's original latency/complexity tradeoff**: reasonable if the pipeline only ever targets frontier-tier hosted models (Groq/Anthropic, the current defaults per AGENTS.md) where this failure rate may be lower or unobserved. Worth noting this ADR's evidence comes from a local 7B model, not the pipeline's configured defaults — the proposal here is explicitly scoped as opt-in for exactly that reason, not a blanket requirement. -**Full CI build/test suite before PR open, for every generated change**: strictly more thorough but reintroduces the latency/complexity concern ADR-0009 weighed against, and requires per-repo build tooling knowledge this pipeline doesn't currently have. The import-allowlist and `tsc --noEmit` checks proposed here are a narrower, cheaper subset chosen specifically because they map to the two concrete defects observed and require no repo-specific build knowledge beyond "does a `tsconfig.json` exist." +**Full CI build/test suite before PR open, for every generated change**: strictly more thorough but reintroduces the latency/complexity concern ADR-0009 weighed against, and requires per-repo build tooling knowledge this pipeline doesn't currently have. The import-allowlist check proposed here needs only `package.json`, no build step. The `tsc --noEmit` check is not as cheap by that same measure — as established above, it additionally requires detecting the target repo's package manager and running its install step before `tsc` is usable at all, which is real repo-specific build knowledge, not just "does a `tsconfig.json` exist." Both checks are still narrower than a full build/test suite (no test runner, no application build, no deployment steps), but the type-check option in particular carries more setup cost than this ADR's Decision section states outright — worth flagging honestly rather than understating it here for the sake of the comparison. ## Consequences From 15c61ebfc5dd26a8e50d8704cfc682efe17a9b6a Mon Sep 17 00:00:00 2001 From: koydas Date: Mon, 27 Jul 2026 19:04:22 +0000 Subject: [PATCH 07/12] fix(adr): require --ignore-scripts and a pre-generation snapshot for the install step MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- docs/adr/0019-static-verification-backstop.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/adr/0019-static-verification-backstop.md b/docs/adr/0019-static-verification-backstop.md index 67c5a3c..e717a20 100644 --- a/docs/adr/0019-static-verification-backstop.md +++ b/docs/adr/0019-static-verification-backstop.md @@ -23,7 +23,7 @@ This reopens the question ADR-0009 settled by prompt-only means: **prompt wordin Add an optional, opt-in static verification step. For the generation stage, it runs immediately after `writeGeneratedFiles()`, before a PR is opened. For auto-fix, there is no "before a PR is opened" moment — `auto-fix-pr.yml` only runs after a label is applied to an *already-open* PR, checks out its existing branch, and commits/pushes generated files to it — so the equivalent gate there runs immediately after auto-fix writes its generated files, before its commit/push step: 1. **Import allowlist check** (root-manifest scope; per-repo config beyond `package.json` is only needed for workspace/monorepo setups, see caveat below): snapshot `package.json`'s dependency fields **before** calling the generation LLM (the same read already performed to build the "Allowed npm dependencies" context block, see #157), and snapshot each target file's own pre-generation import/require specifiers alongside it. After writing, extract `import`/`require` specifiers from each generated/modified JS/TS file and flag only specifiers that are newly added by this patch (not present in that file's own pre-generation snapshot) AND are neither in the pre-generation `package.json` snapshot nor in a small maintained list of language/runtime built-ins. Checking only newly-added specifiers matters: an existing file may legitimately already import a package absent from the root manifest (workspace-local dependency, or one this scan doesn't otherwise see) — the generation guardrail explicitly allows imports "already used elsewhere in the file", and scanning every specifier in the completed file rather than only what changed would reject that legitimate, unrelated-to-this-patch import. Comparing against the pre-generation `package.json` snapshot, not the post-write working tree, is separately load-bearing: since the model's own `changes` can include a rewritten `package.json` (up to 6 files per the existing HARD LIMIT), checking against the working-tree file after `writeGeneratedFiles()` would let the model add both the import and the corresponding manifest entry in the same patch and pass trivially, which defeats the check's purpose. This is a regex/AST-level check, not a full build — cheap, deterministic, language-aware only for JS/TS. **Workspace/monorepo caveat:** this check only reads the repository-root manifest (matching #157's scope). A *new* file inside a workspace package (e.g. `packages/app/`) that legitimately imports a dependency declared only in that package's own nested `package.json` — not the root one — would be wrongly flagged, since it has no pre-generation snapshot of its own to fall back on (it doesn't exist yet) and the root manifest doesn't list it. Resolving this requires snapshotting the nearest governing `package.json` per generated file path, not just the root one — left as an open implementation detail, not assumed to already work for every repository layout. -2. **Type-check pass** (opt-in per target repo, since not every repo this pipeline touches is TypeScript): if the target repo has a `tsconfig.json`, running `tsc --noEmit` isn't a drop-in solution for several reasons. First, a prerequisite this proposal initially glossed over: `code-generation.yml` and `auto-fix-pr.yml` currently only check out the repository and set up Node for *this* repo (`autonomous-dev-loop` itself) — neither installs the *target* repo's dependencies via its package manager (npm/yarn/pnpm) and lockfile. Without that, `tsc` may not even be installed, and even a globally-available compiler would immediately report every imported module and type declaration as missing, so no baseline could ever pass. Enabling this check therefore first requires detecting the target repo's package manager and running its install step (or invoking a repo-configured validation command) before any of the following applies. Second, even with dependencies installed, `tsc`'s CLI rejects combining `--project` with an explicit file list (error TS5042), so scoping to just the generated files isn't directly available, and running full project-mode `tsc` would fail on *any* pre-existing type error anywhere in the project, blocking every generated PR regardless of whether the generated files themselves are valid. A workable version needs one of: (a) a clean-baseline prerequisite (only enable this check for repos that already pass `tsc --noEmit` on `main`, post-install), (b) a baseline-error-count comparison (run `tsc --noEmit` before and after applying the generated changes, on the whole project, and fail only if the error count/set grows), or (c) a derived, isolated `tsconfig.json` (generated on the fly, `include`-scoped to just the changed files plus their transitive local imports) so project mode has a narrow enough surface. This needs to be resolved concretely in the implementation PR, not assumed away — treat both the dependency-installation step and "which of (a)/(b)/(c)" as open implementation questions this ADR does not settle. +2. **Type-check pass** (opt-in per target repo, since not every repo this pipeline touches is TypeScript): if the target repo has a `tsconfig.json`, running `tsc --noEmit` isn't a drop-in solution for several reasons. First, a prerequisite this proposal initially glossed over: `code-generation.yml` and `auto-fix-pr.yml` currently only check out the repository and set up Node for *this* repo (`autonomous-dev-loop` itself) — neither installs the *target* repo's dependencies via its package manager (npm/yarn/pnpm) and lockfile. Without that, `tsc` may not even be installed, and even a globally-available compiler would immediately report every imported module and type declaration as missing, so no baseline could ever pass. Enabling this check therefore first requires detecting the target repo's package manager and running its install step (or invoking a repo-configured validation command) before any of the following applies. **This install step is itself a security boundary, not a detail to defer**: by default, npm/yarn/pnpm all execute lifecycle scripts (`preinstall`/`install`/`postinstall`) declared in `package.json` during install — npm does not set `ignore-scripts` to true by default. Since this runs inside the existing generation/auto-fix job (which holds `GITHUB_TOKEN`/`AI_PR_TOKEN` and a checkout of the repo, including whatever the generated patch or an external contributor's PR branch contains), an unverified `package.json` with a malicious or merely-compromised `postinstall` script would execute with those credentials *before* any of this ADR's verification has run — the opposite of the trust boundary this ADR is trying to establish. The install must therefore (a) run with lifecycle scripts disabled (`npm install --ignore-scripts`; yarn and pnpm have equivalent flags) and (b) be based on the pre-generation manifest/lockfile snapshot, not the post-write working tree, for the same reason snapshot-timing matters for the import-allowlist check above. Second, even with dependencies installed, `tsc`'s CLI rejects combining `--project` with an explicit file list (error TS5042), so scoping to just the generated files isn't directly available, and running full project-mode `tsc` would fail on *any* pre-existing type error anywhere in the project, blocking every generated PR regardless of whether the generated files themselves are valid. A workable version needs one of: (a) a clean-baseline prerequisite (only enable this check for repos that already pass `tsc --noEmit` on `main`, post-install), (b) a baseline-error-count comparison (run `tsc --noEmit` before and after applying the generated changes, on the whole project, and fail only if the error count/set grows), or (c) a derived, isolated `tsconfig.json` (generated on the fly, `include`-scoped to just the changed files plus their transitive local imports) so project mode has a narrow enough surface. This needs to be resolved concretely in the implementation PR, not assumed away — treat the dependency-installation step (including its `--ignore-scripts` requirement), "which of (a)/(b)/(c)", and whether `tsc` itself needs sandboxing beyond script-disabling as open implementation questions this ADR does not settle. For the generation stage, both checks are advisory-to-blocking: a failure prevents the PR from being opened (or triggers a re-generation attempt), rather than being left for the AI reviewer to notice. For auto-fix, a failure should count against the existing 3-attempt bound (`MAX_ATTEMPTS` in `auto_fix_pr.mjs`, tracked via `auto-fix-attempt-N` labels) the same way an unhelpful fix already does, rather than opening a new, separate retry budget — and whether it also changes the `changes-requested`/`review-approved` labeling is an open implementation question, not decided here. Either way, this benchmark's finding is precisely that the reviewer cannot be relied upon to notice these defects reliably on its own. From 7cb73828cb051da8163d24c71c11a212753c5820 Mon Sep 17 00:00:00 2001 From: koydas Date: Mon, 27 Jul 2026 19:43:28 +0000 Subject: [PATCH 08/12] fix(adr): correct snapshot sequencing for the import allowlist check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- docs/adr/0019-static-verification-backstop.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/adr/0019-static-verification-backstop.md b/docs/adr/0019-static-verification-backstop.md index e717a20..a5e4a1d 100644 --- a/docs/adr/0019-static-verification-backstop.md +++ b/docs/adr/0019-static-verification-backstop.md @@ -22,7 +22,7 @@ This reopens the question ADR-0009 settled by prompt-only means: **prompt wordin Add an optional, opt-in static verification step. For the generation stage, it runs immediately after `writeGeneratedFiles()`, before a PR is opened. For auto-fix, there is no "before a PR is opened" moment — `auto-fix-pr.yml` only runs after a label is applied to an *already-open* PR, checks out its existing branch, and commits/pushes generated files to it — so the equivalent gate there runs immediately after auto-fix writes its generated files, before its commit/push step: -1. **Import allowlist check** (root-manifest scope; per-repo config beyond `package.json` is only needed for workspace/monorepo setups, see caveat below): snapshot `package.json`'s dependency fields **before** calling the generation LLM (the same read already performed to build the "Allowed npm dependencies" context block, see #157), and snapshot each target file's own pre-generation import/require specifiers alongside it. After writing, extract `import`/`require` specifiers from each generated/modified JS/TS file and flag only specifiers that are newly added by this patch (not present in that file's own pre-generation snapshot) AND are neither in the pre-generation `package.json` snapshot nor in a small maintained list of language/runtime built-ins. Checking only newly-added specifiers matters: an existing file may legitimately already import a package absent from the root manifest (workspace-local dependency, or one this scan doesn't otherwise see) — the generation guardrail explicitly allows imports "already used elsewhere in the file", and scanning every specifier in the completed file rather than only what changed would reject that legitimate, unrelated-to-this-patch import. Comparing against the pre-generation `package.json` snapshot, not the post-write working tree, is separately load-bearing: since the model's own `changes` can include a rewritten `package.json` (up to 6 files per the existing HARD LIMIT), checking against the working-tree file after `writeGeneratedFiles()` would let the model add both the import and the corresponding manifest entry in the same patch and pass trivially, which defeats the check's purpose. This is a regex/AST-level check, not a full build — cheap, deterministic, language-aware only for JS/TS. **Workspace/monorepo caveat:** this check only reads the repository-root manifest (matching #157's scope). A *new* file inside a workspace package (e.g. `packages/app/`) that legitimately imports a dependency declared only in that package's own nested `package.json` — not the root one — would be wrongly flagged, since it has no pre-generation snapshot of its own to fall back on (it doesn't exist yet) and the root manifest doesn't list it. Resolving this requires snapshotting the nearest governing `package.json` per generated file path, not just the root one — left as an open implementation detail, not assumed to already work for every repository layout. +1. **Import allowlist check** (root-manifest scope; per-repo config beyond `package.json` is only needed for workspace/monorepo setups, see caveat below): the two snapshots this check needs happen at different points, not both "before the LLM call" — `target_path` values are chosen by the LLM itself, so which files even need a pre-generation import snapshot isn't knowable until its response exists. Snapshot `package.json`'s dependency fields **before** calling the generation LLM (the same read already performed to build the "Allowed npm dependencies" context block, see #157) — this one is repo-wide and doesn't depend on the response. Snapshot each target file's own pre-generation import/require specifiers (empty for a file that doesn't exist yet) **after** `validateAiOutput()` returns `changes` (so `target_path` values are known) but **before** `writeGeneratedFiles()` overwrites them — in `generate_issue_change.mjs`'s current flow, that's the narrow window between those two calls. After writing, extract `import`/`require` specifiers from each generated/modified JS/TS file and flag only specifiers that are newly added by this patch (not present in that file's own pre-generation snapshot) AND are neither in the pre-generation `package.json` snapshot nor in a small maintained list of language/runtime built-ins. Checking only newly-added specifiers matters: an existing file may legitimately already import a package absent from the root manifest (workspace-local dependency, or one this scan doesn't otherwise see) — the generation guardrail explicitly allows imports "already used elsewhere in the file", and scanning every specifier in the completed file rather than only what changed would reject that legitimate, unrelated-to-this-patch import. Comparing against the pre-generation `package.json` snapshot, not the post-write working tree, is separately load-bearing: since the model's own `changes` can include a rewritten `package.json` (up to 6 files per the existing HARD LIMIT), checking against the working-tree file after `writeGeneratedFiles()` would let the model add both the import and the corresponding manifest entry in the same patch and pass trivially, which defeats the check's purpose. This is a regex/AST-level check, not a full build — cheap, deterministic, language-aware only for JS/TS. **Workspace/monorepo caveat:** this check only reads the repository-root manifest (matching #157's scope). A *new* file inside a workspace package (e.g. `packages/app/`) that legitimately imports a dependency declared only in that package's own nested `package.json` — not the root one — would be wrongly flagged, since it has no pre-generation snapshot of its own to fall back on (it doesn't exist yet) and the root manifest doesn't list it. Resolving this requires snapshotting the nearest governing `package.json` per generated file path, not just the root one — left as an open implementation detail, not assumed to already work for every repository layout. 2. **Type-check pass** (opt-in per target repo, since not every repo this pipeline touches is TypeScript): if the target repo has a `tsconfig.json`, running `tsc --noEmit` isn't a drop-in solution for several reasons. First, a prerequisite this proposal initially glossed over: `code-generation.yml` and `auto-fix-pr.yml` currently only check out the repository and set up Node for *this* repo (`autonomous-dev-loop` itself) — neither installs the *target* repo's dependencies via its package manager (npm/yarn/pnpm) and lockfile. Without that, `tsc` may not even be installed, and even a globally-available compiler would immediately report every imported module and type declaration as missing, so no baseline could ever pass. Enabling this check therefore first requires detecting the target repo's package manager and running its install step (or invoking a repo-configured validation command) before any of the following applies. **This install step is itself a security boundary, not a detail to defer**: by default, npm/yarn/pnpm all execute lifecycle scripts (`preinstall`/`install`/`postinstall`) declared in `package.json` during install — npm does not set `ignore-scripts` to true by default. Since this runs inside the existing generation/auto-fix job (which holds `GITHUB_TOKEN`/`AI_PR_TOKEN` and a checkout of the repo, including whatever the generated patch or an external contributor's PR branch contains), an unverified `package.json` with a malicious or merely-compromised `postinstall` script would execute with those credentials *before* any of this ADR's verification has run — the opposite of the trust boundary this ADR is trying to establish. The install must therefore (a) run with lifecycle scripts disabled (`npm install --ignore-scripts`; yarn and pnpm have equivalent flags) and (b) be based on the pre-generation manifest/lockfile snapshot, not the post-write working tree, for the same reason snapshot-timing matters for the import-allowlist check above. Second, even with dependencies installed, `tsc`'s CLI rejects combining `--project` with an explicit file list (error TS5042), so scoping to just the generated files isn't directly available, and running full project-mode `tsc` would fail on *any* pre-existing type error anywhere in the project, blocking every generated PR regardless of whether the generated files themselves are valid. A workable version needs one of: (a) a clean-baseline prerequisite (only enable this check for repos that already pass `tsc --noEmit` on `main`, post-install), (b) a baseline-error-count comparison (run `tsc --noEmit` before and after applying the generated changes, on the whole project, and fail only if the error count/set grows), or (c) a derived, isolated `tsconfig.json` (generated on the fly, `include`-scoped to just the changed files plus their transitive local imports) so project mode has a narrow enough surface. This needs to be resolved concretely in the implementation PR, not assumed away — treat the dependency-installation step (including its `--ignore-scripts` requirement), "which of (a)/(b)/(c)", and whether `tsc` itself needs sandboxing beyond script-disabling as open implementation questions this ADR does not settle. For the generation stage, both checks are advisory-to-blocking: a failure prevents the PR from being opened (or triggers a re-generation attempt), rather than being left for the AI reviewer to notice. For auto-fix, a failure should count against the existing 3-attempt bound (`MAX_ATTEMPTS` in `auto_fix_pr.mjs`, tracked via `auto-fix-attempt-N` labels) the same way an unhelpful fix already does, rather than opening a new, separate retry budget — and whether it also changes the `changes-requested`/`review-approved` labeling is an open implementation question, not decided here. Either way, this benchmark's finding is precisely that the reviewer cannot be relied upon to notice these defects reliably on its own. From 866fa03303931e5ffb5a7d7055ad5c18587ac243 Mon Sep 17 00:00:00 2001 From: koydas Date: Mon, 27 Jul 2026 19:50:52 +0000 Subject: [PATCH 09/12] fix(adr): exempt relative imports and compare diagnostic identity not count MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 --- docs/adr/0019-static-verification-backstop.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/adr/0019-static-verification-backstop.md b/docs/adr/0019-static-verification-backstop.md index a5e4a1d..3ce1ac9 100644 --- a/docs/adr/0019-static-verification-backstop.md +++ b/docs/adr/0019-static-verification-backstop.md @@ -22,8 +22,8 @@ This reopens the question ADR-0009 settled by prompt-only means: **prompt wordin Add an optional, opt-in static verification step. For the generation stage, it runs immediately after `writeGeneratedFiles()`, before a PR is opened. For auto-fix, there is no "before a PR is opened" moment — `auto-fix-pr.yml` only runs after a label is applied to an *already-open* PR, checks out its existing branch, and commits/pushes generated files to it — so the equivalent gate there runs immediately after auto-fix writes its generated files, before its commit/push step: -1. **Import allowlist check** (root-manifest scope; per-repo config beyond `package.json` is only needed for workspace/monorepo setups, see caveat below): the two snapshots this check needs happen at different points, not both "before the LLM call" — `target_path` values are chosen by the LLM itself, so which files even need a pre-generation import snapshot isn't knowable until its response exists. Snapshot `package.json`'s dependency fields **before** calling the generation LLM (the same read already performed to build the "Allowed npm dependencies" context block, see #157) — this one is repo-wide and doesn't depend on the response. Snapshot each target file's own pre-generation import/require specifiers (empty for a file that doesn't exist yet) **after** `validateAiOutput()` returns `changes` (so `target_path` values are known) but **before** `writeGeneratedFiles()` overwrites them — in `generate_issue_change.mjs`'s current flow, that's the narrow window between those two calls. After writing, extract `import`/`require` specifiers from each generated/modified JS/TS file and flag only specifiers that are newly added by this patch (not present in that file's own pre-generation snapshot) AND are neither in the pre-generation `package.json` snapshot nor in a small maintained list of language/runtime built-ins. Checking only newly-added specifiers matters: an existing file may legitimately already import a package absent from the root manifest (workspace-local dependency, or one this scan doesn't otherwise see) — the generation guardrail explicitly allows imports "already used elsewhere in the file", and scanning every specifier in the completed file rather than only what changed would reject that legitimate, unrelated-to-this-patch import. Comparing against the pre-generation `package.json` snapshot, not the post-write working tree, is separately load-bearing: since the model's own `changes` can include a rewritten `package.json` (up to 6 files per the existing HARD LIMIT), checking against the working-tree file after `writeGeneratedFiles()` would let the model add both the import and the corresponding manifest entry in the same patch and pass trivially, which defeats the check's purpose. This is a regex/AST-level check, not a full build — cheap, deterministic, language-aware only for JS/TS. **Workspace/monorepo caveat:** this check only reads the repository-root manifest (matching #157's scope). A *new* file inside a workspace package (e.g. `packages/app/`) that legitimately imports a dependency declared only in that package's own nested `package.json` — not the root one — would be wrongly flagged, since it has no pre-generation snapshot of its own to fall back on (it doesn't exist yet) and the root manifest doesn't list it. Resolving this requires snapshotting the nearest governing `package.json` per generated file path, not just the root one — left as an open implementation detail, not assumed to already work for every repository layout. -2. **Type-check pass** (opt-in per target repo, since not every repo this pipeline touches is TypeScript): if the target repo has a `tsconfig.json`, running `tsc --noEmit` isn't a drop-in solution for several reasons. First, a prerequisite this proposal initially glossed over: `code-generation.yml` and `auto-fix-pr.yml` currently only check out the repository and set up Node for *this* repo (`autonomous-dev-loop` itself) — neither installs the *target* repo's dependencies via its package manager (npm/yarn/pnpm) and lockfile. Without that, `tsc` may not even be installed, and even a globally-available compiler would immediately report every imported module and type declaration as missing, so no baseline could ever pass. Enabling this check therefore first requires detecting the target repo's package manager and running its install step (or invoking a repo-configured validation command) before any of the following applies. **This install step is itself a security boundary, not a detail to defer**: by default, npm/yarn/pnpm all execute lifecycle scripts (`preinstall`/`install`/`postinstall`) declared in `package.json` during install — npm does not set `ignore-scripts` to true by default. Since this runs inside the existing generation/auto-fix job (which holds `GITHUB_TOKEN`/`AI_PR_TOKEN` and a checkout of the repo, including whatever the generated patch or an external contributor's PR branch contains), an unverified `package.json` with a malicious or merely-compromised `postinstall` script would execute with those credentials *before* any of this ADR's verification has run — the opposite of the trust boundary this ADR is trying to establish. The install must therefore (a) run with lifecycle scripts disabled (`npm install --ignore-scripts`; yarn and pnpm have equivalent flags) and (b) be based on the pre-generation manifest/lockfile snapshot, not the post-write working tree, for the same reason snapshot-timing matters for the import-allowlist check above. Second, even with dependencies installed, `tsc`'s CLI rejects combining `--project` with an explicit file list (error TS5042), so scoping to just the generated files isn't directly available, and running full project-mode `tsc` would fail on *any* pre-existing type error anywhere in the project, blocking every generated PR regardless of whether the generated files themselves are valid. A workable version needs one of: (a) a clean-baseline prerequisite (only enable this check for repos that already pass `tsc --noEmit` on `main`, post-install), (b) a baseline-error-count comparison (run `tsc --noEmit` before and after applying the generated changes, on the whole project, and fail only if the error count/set grows), or (c) a derived, isolated `tsconfig.json` (generated on the fly, `include`-scoped to just the changed files plus their transitive local imports) so project mode has a narrow enough surface. This needs to be resolved concretely in the implementation PR, not assumed away — treat the dependency-installation step (including its `--ignore-scripts` requirement), "which of (a)/(b)/(c)", and whether `tsc` itself needs sandboxing beyond script-disabling as open implementation questions this ADR does not settle. +1. **Import allowlist check** (root-manifest scope; per-repo config beyond `package.json` is only needed for workspace/monorepo setups, see caveat below): the two snapshots this check needs happen at different points, not both "before the LLM call" — `target_path` values are chosen by the LLM itself, so which files even need a pre-generation import snapshot isn't knowable until its response exists. Snapshot `package.json`'s dependency fields **before** calling the generation LLM (the same read already performed to build the "Allowed npm dependencies" context block, see #157) — this one is repo-wide and doesn't depend on the response. Snapshot each target file's own pre-generation import/require specifiers (empty for a file that doesn't exist yet) **after** `validateAiOutput()` returns `changes` (so `target_path` values are known) but **before** `writeGeneratedFiles()` overwrites them — in `generate_issue_change.mjs`'s current flow, that's the narrow window between those two calls. After writing, extract `import`/`require` specifiers from each generated/modified JS/TS file. First discard any relative or absolute-path specifier (starts with `./`, `../`, or `/`) — these are project-local file references, never package names, and `generation-user.md`'s own requirement 7 already permits them; the allowlist check applies only to bare specifiers (`react`, `@scope/pkg`, etc.), which is what "package" means in this context. Among the remaining bare specifiers, flag only those that are newly added by this patch (not present in that file's own pre-generation snapshot) AND are neither in the pre-generation `package.json` snapshot nor in a small maintained list of language/runtime built-ins. Checking only newly-added specifiers matters: an existing file may legitimately already import a package absent from the root manifest (workspace-local dependency, or one this scan doesn't otherwise see) — the generation guardrail explicitly allows imports "already used elsewhere in the file", and scanning every specifier in the completed file rather than only what changed would reject that legitimate, unrelated-to-this-patch import. Comparing against the pre-generation `package.json` snapshot, not the post-write working tree, is separately load-bearing: since the model's own `changes` can include a rewritten `package.json` (up to 6 files per the existing HARD LIMIT), checking against the working-tree file after `writeGeneratedFiles()` would let the model add both the import and the corresponding manifest entry in the same patch and pass trivially, which defeats the check's purpose. This is a regex/AST-level check, not a full build — cheap, deterministic, language-aware only for JS/TS. **Workspace/monorepo caveat:** this check only reads the repository-root manifest (matching #157's scope). A *new* file inside a workspace package (e.g. `packages/app/`) that legitimately imports a dependency declared only in that package's own nested `package.json` — not the root one — would be wrongly flagged, since it has no pre-generation snapshot of its own to fall back on (it doesn't exist yet) and the root manifest doesn't list it. Resolving this requires snapshotting the nearest governing `package.json` per generated file path, not just the root one — left as an open implementation detail, not assumed to already work for every repository layout. +2. **Type-check pass** (opt-in per target repo, since not every repo this pipeline touches is TypeScript): if the target repo has a `tsconfig.json`, running `tsc --noEmit` isn't a drop-in solution for several reasons. First, a prerequisite this proposal initially glossed over: `code-generation.yml` and `auto-fix-pr.yml` currently only check out the repository and set up Node for *this* repo (`autonomous-dev-loop` itself) — neither installs the *target* repo's dependencies via its package manager (npm/yarn/pnpm) and lockfile. Without that, `tsc` may not even be installed, and even a globally-available compiler would immediately report every imported module and type declaration as missing, so no baseline could ever pass. Enabling this check therefore first requires detecting the target repo's package manager and running its install step (or invoking a repo-configured validation command) before any of the following applies. **This install step is itself a security boundary, not a detail to defer**: by default, npm/yarn/pnpm all execute lifecycle scripts (`preinstall`/`install`/`postinstall`) declared in `package.json` during install — npm does not set `ignore-scripts` to true by default. Since this runs inside the existing generation/auto-fix job (which holds `GITHUB_TOKEN`/`AI_PR_TOKEN` and a checkout of the repo, including whatever the generated patch or an external contributor's PR branch contains), an unverified `package.json` with a malicious or merely-compromised `postinstall` script would execute with those credentials *before* any of this ADR's verification has run — the opposite of the trust boundary this ADR is trying to establish. The install must therefore (a) run with lifecycle scripts disabled (`npm install --ignore-scripts`; yarn and pnpm have equivalent flags) and (b) be based on the pre-generation manifest/lockfile snapshot, not the post-write working tree, for the same reason snapshot-timing matters for the import-allowlist check above. Second, even with dependencies installed, `tsc`'s CLI rejects combining `--project` with an explicit file list (error TS5042), so scoping to just the generated files isn't directly available, and running full project-mode `tsc` would fail on *any* pre-existing type error anywhere in the project, blocking every generated PR regardless of whether the generated files themselves are valid. A workable version needs one of: (a) a clean-baseline prerequisite (only enable this check for repos that already pass `tsc --noEmit` on `main`, post-install), (b) a baseline-diagnostics comparison (run `tsc --noEmit` before and after applying the generated changes, on the whole project, and fail if any diagnostic — identified by file + line + error code, not merely the total count — is present after that wasn't present before), or (c) a derived, isolated `tsconfig.json` (generated on the fly, `include`-scoped to just the changed files plus their transitive local imports) so project mode has a narrow enough surface. Comparing raw counts instead of diagnostic identity in (b) would be unsound: a multi-file patch that introduces one new error while incidentally fixing an unrelated pre-existing one leaves the count flat or lower, letting exactly the class of defect this ADR targets slip through a count-only gate. This needs to be resolved concretely in the implementation PR, not assumed away — treat the dependency-installation step (including its `--ignore-scripts` requirement), "which of (a)/(b)/(c)", and whether `tsc` itself needs sandboxing beyond script-disabling as open implementation questions this ADR does not settle. For the generation stage, both checks are advisory-to-blocking: a failure prevents the PR from being opened (or triggers a re-generation attempt), rather than being left for the AI reviewer to notice. For auto-fix, a failure should count against the existing 3-attempt bound (`MAX_ATTEMPTS` in `auto_fix_pr.mjs`, tracked via `auto-fix-attempt-N` labels) the same way an unhelpful fix already does, rather than opening a new, separate retry budget — and whether it also changes the `changes-requested`/`review-approved` labeling is an open implementation question, not decided here. Either way, this benchmark's finding is precisely that the reviewer cannot be relied upon to notice these defects reliably on its own. From b18b0dec5cb52c955f947b73ebd3eb1b88fe2942 Mon Sep 17 00:00:00 2001 From: koydas Date: Mon, 27 Jul 2026 19:57:36 +0000 Subject: [PATCH 10/12] fix(adr): normalize subpath imports, verify program coverage, require frozen installs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 --- docs/adr/0019-static-verification-backstop.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/adr/0019-static-verification-backstop.md b/docs/adr/0019-static-verification-backstop.md index 3ce1ac9..cc5b3b6 100644 --- a/docs/adr/0019-static-verification-backstop.md +++ b/docs/adr/0019-static-verification-backstop.md @@ -22,8 +22,8 @@ This reopens the question ADR-0009 settled by prompt-only means: **prompt wordin Add an optional, opt-in static verification step. For the generation stage, it runs immediately after `writeGeneratedFiles()`, before a PR is opened. For auto-fix, there is no "before a PR is opened" moment — `auto-fix-pr.yml` only runs after a label is applied to an *already-open* PR, checks out its existing branch, and commits/pushes generated files to it — so the equivalent gate there runs immediately after auto-fix writes its generated files, before its commit/push step: -1. **Import allowlist check** (root-manifest scope; per-repo config beyond `package.json` is only needed for workspace/monorepo setups, see caveat below): the two snapshots this check needs happen at different points, not both "before the LLM call" — `target_path` values are chosen by the LLM itself, so which files even need a pre-generation import snapshot isn't knowable until its response exists. Snapshot `package.json`'s dependency fields **before** calling the generation LLM (the same read already performed to build the "Allowed npm dependencies" context block, see #157) — this one is repo-wide and doesn't depend on the response. Snapshot each target file's own pre-generation import/require specifiers (empty for a file that doesn't exist yet) **after** `validateAiOutput()` returns `changes` (so `target_path` values are known) but **before** `writeGeneratedFiles()` overwrites them — in `generate_issue_change.mjs`'s current flow, that's the narrow window between those two calls. After writing, extract `import`/`require` specifiers from each generated/modified JS/TS file. First discard any relative or absolute-path specifier (starts with `./`, `../`, or `/`) — these are project-local file references, never package names, and `generation-user.md`'s own requirement 7 already permits them; the allowlist check applies only to bare specifiers (`react`, `@scope/pkg`, etc.), which is what "package" means in this context. Among the remaining bare specifiers, flag only those that are newly added by this patch (not present in that file's own pre-generation snapshot) AND are neither in the pre-generation `package.json` snapshot nor in a small maintained list of language/runtime built-ins. Checking only newly-added specifiers matters: an existing file may legitimately already import a package absent from the root manifest (workspace-local dependency, or one this scan doesn't otherwise see) — the generation guardrail explicitly allows imports "already used elsewhere in the file", and scanning every specifier in the completed file rather than only what changed would reject that legitimate, unrelated-to-this-patch import. Comparing against the pre-generation `package.json` snapshot, not the post-write working tree, is separately load-bearing: since the model's own `changes` can include a rewritten `package.json` (up to 6 files per the existing HARD LIMIT), checking against the working-tree file after `writeGeneratedFiles()` would let the model add both the import and the corresponding manifest entry in the same patch and pass trivially, which defeats the check's purpose. This is a regex/AST-level check, not a full build — cheap, deterministic, language-aware only for JS/TS. **Workspace/monorepo caveat:** this check only reads the repository-root manifest (matching #157's scope). A *new* file inside a workspace package (e.g. `packages/app/`) that legitimately imports a dependency declared only in that package's own nested `package.json` — not the root one — would be wrongly flagged, since it has no pre-generation snapshot of its own to fall back on (it doesn't exist yet) and the root manifest doesn't list it. Resolving this requires snapshotting the nearest governing `package.json` per generated file path, not just the root one — left as an open implementation detail, not assumed to already work for every repository layout. -2. **Type-check pass** (opt-in per target repo, since not every repo this pipeline touches is TypeScript): if the target repo has a `tsconfig.json`, running `tsc --noEmit` isn't a drop-in solution for several reasons. First, a prerequisite this proposal initially glossed over: `code-generation.yml` and `auto-fix-pr.yml` currently only check out the repository and set up Node for *this* repo (`autonomous-dev-loop` itself) — neither installs the *target* repo's dependencies via its package manager (npm/yarn/pnpm) and lockfile. Without that, `tsc` may not even be installed, and even a globally-available compiler would immediately report every imported module and type declaration as missing, so no baseline could ever pass. Enabling this check therefore first requires detecting the target repo's package manager and running its install step (or invoking a repo-configured validation command) before any of the following applies. **This install step is itself a security boundary, not a detail to defer**: by default, npm/yarn/pnpm all execute lifecycle scripts (`preinstall`/`install`/`postinstall`) declared in `package.json` during install — npm does not set `ignore-scripts` to true by default. Since this runs inside the existing generation/auto-fix job (which holds `GITHUB_TOKEN`/`AI_PR_TOKEN` and a checkout of the repo, including whatever the generated patch or an external contributor's PR branch contains), an unverified `package.json` with a malicious or merely-compromised `postinstall` script would execute with those credentials *before* any of this ADR's verification has run — the opposite of the trust boundary this ADR is trying to establish. The install must therefore (a) run with lifecycle scripts disabled (`npm install --ignore-scripts`; yarn and pnpm have equivalent flags) and (b) be based on the pre-generation manifest/lockfile snapshot, not the post-write working tree, for the same reason snapshot-timing matters for the import-allowlist check above. Second, even with dependencies installed, `tsc`'s CLI rejects combining `--project` with an explicit file list (error TS5042), so scoping to just the generated files isn't directly available, and running full project-mode `tsc` would fail on *any* pre-existing type error anywhere in the project, blocking every generated PR regardless of whether the generated files themselves are valid. A workable version needs one of: (a) a clean-baseline prerequisite (only enable this check for repos that already pass `tsc --noEmit` on `main`, post-install), (b) a baseline-diagnostics comparison (run `tsc --noEmit` before and after applying the generated changes, on the whole project, and fail if any diagnostic — identified by file + line + error code, not merely the total count — is present after that wasn't present before), or (c) a derived, isolated `tsconfig.json` (generated on the fly, `include`-scoped to just the changed files plus their transitive local imports) so project mode has a narrow enough surface. Comparing raw counts instead of diagnostic identity in (b) would be unsound: a multi-file patch that introduces one new error while incidentally fixing an unrelated pre-existing one leaves the count flat or lower, letting exactly the class of defect this ADR targets slip through a count-only gate. This needs to be resolved concretely in the implementation PR, not assumed away — treat the dependency-installation step (including its `--ignore-scripts` requirement), "which of (a)/(b)/(c)", and whether `tsc` itself needs sandboxing beyond script-disabling as open implementation questions this ADR does not settle. +1. **Import allowlist check** (root-manifest scope; per-repo config beyond `package.json` is only needed for workspace/monorepo setups, see caveat below): the two snapshots this check needs happen at different points, not both "before the LLM call" — `target_path` values are chosen by the LLM itself, so which files even need a pre-generation import snapshot isn't knowable until its response exists. Snapshot `package.json`'s dependency fields **before** calling the generation LLM (the same read already performed to build the "Allowed npm dependencies" context block, see #157) — this one is repo-wide and doesn't depend on the response. Snapshot each target file's own pre-generation import/require specifiers (empty for a file that doesn't exist yet) **after** `validateAiOutput()` returns `changes` (so `target_path` values are known) but **before** `writeGeneratedFiles()` overwrites them — in `generate_issue_change.mjs`'s current flow, that's the narrow window between those two calls. After writing, extract `import`/`require` specifiers from each generated/modified JS/TS file. First discard any relative or absolute-path specifier (starts with `./`, `../`, or `/`) — these are project-local file references, never package names, and `generation-user.md`'s own requirement 7 already permits them; the allowlist check applies only to bare specifiers (`react`, `@scope/pkg`, etc.), which is what "package" means in this context. Among the remaining bare specifiers, normalize each to its package root before comparing against the manifest — a public subpath import like `react/jsx-runtime`, `date-fns/format`, or `@scope/pkg/subpath` is authorized by the same `package.json` entry (`react`, `date-fns`, `@scope/pkg`) as a bare import of the package itself, but won't literally match that key unless the first one or two path segments (the second only for a `@scope/` specifier) are taken as the comparison key. Then flag only those normalized names that are newly added by this patch (not present in that file's own pre-generation snapshot) AND are neither in the pre-generation `package.json` snapshot nor in a small maintained list of language/runtime built-ins. Checking only newly-added specifiers matters: an existing file may legitimately already import a package absent from the root manifest (workspace-local dependency, or one this scan doesn't otherwise see) — the generation guardrail explicitly allows imports "already used elsewhere in the file", and scanning every specifier in the completed file rather than only what changed would reject that legitimate, unrelated-to-this-patch import. Comparing against the pre-generation `package.json` snapshot, not the post-write working tree, is separately load-bearing: since the model's own `changes` can include a rewritten `package.json` (up to 6 files per the existing HARD LIMIT), checking against the working-tree file after `writeGeneratedFiles()` would let the model add both the import and the corresponding manifest entry in the same patch and pass trivially, which defeats the check's purpose. This is a regex/AST-level check, not a full build — cheap, deterministic, language-aware only for JS/TS. **Workspace/monorepo caveat:** this check only reads the repository-root manifest (matching #157's scope). A *new* file inside a workspace package (e.g. `packages/app/`) that legitimately imports a dependency declared only in that package's own nested `package.json` — not the root one — would be wrongly flagged, since it has no pre-generation snapshot of its own to fall back on (it doesn't exist yet) and the root manifest doesn't list it. Resolving this requires snapshotting the nearest governing `package.json` per generated file path, not just the root one — left as an open implementation detail, not assumed to already work for every repository layout. +2. **Type-check pass** (opt-in per target repo, since not every repo this pipeline touches is TypeScript): if the target repo has a `tsconfig.json`, running `tsc --noEmit` isn't a drop-in solution for several reasons. First, a prerequisite this proposal initially glossed over: `code-generation.yml` and `auto-fix-pr.yml` currently only check out the repository and set up Node for *this* repo (`autonomous-dev-loop` itself) — neither installs the *target* repo's dependencies via its package manager (npm/yarn/pnpm) and lockfile. Without that, `tsc` may not even be installed, and even a globally-available compiler would immediately report every imported module and type declaration as missing, so no baseline could ever pass. Enabling this check therefore first requires detecting the target repo's package manager and running its install step (or invoking a repo-configured validation command) before any of the following applies. **This install step is itself a security boundary, not a detail to defer**: by default, npm/yarn/pnpm all execute lifecycle scripts (`preinstall`/`install`/`postinstall`) declared in `package.json` during install — npm does not set `ignore-scripts` to true by default. Since this runs inside the existing generation/auto-fix job (which holds `GITHUB_TOKEN`/`AI_PR_TOKEN` and a checkout of the repo, including whatever the generated patch or an external contributor's PR branch contains), an unverified `package.json` with a malicious or merely-compromised `postinstall` script would execute with those credentials *before* any of this ADR's verification has run — the opposite of the trust boundary this ADR is trying to establish. The install must therefore (a) run with lifecycle scripts disabled AND as a frozen/immutable install rather than a mutating one — `npm install --ignore-scripts` alone still permits npm to rewrite `package-lock.json` on a stale or older-format lockfile; use `npm ci --ignore-scripts` instead, which exits on a manifest/lockfile mismatch and never writes to either file (equivalently, `yarn install --immutable --ignore-scripts` or `pnpm install --frozen-lockfile --ignore-scripts`) — and (b) be based on the pre-generation manifest/lockfile snapshot, not the post-write working tree, for the same reason snapshot-timing matters for the import-allowlist check above. The frozen-install requirement also matters operationally for auto-fix specifically: `auto-fix-pr.yml`'s commit step currently stages with `git add -A`, so a mutating install that rewrote the lockfile as a side effect of *verification* would get committed as if it were part of the generated fix. Second, even with dependencies installed, `tsc`'s CLI rejects combining `--project` with an explicit file list (error TS5042), so scoping to just the generated files isn't directly available, and running full project-mode `tsc` would fail on *any* pre-existing type error anywhere in the project, blocking every generated PR regardless of whether the generated files themselves are valid. A workable version needs one of: (a) a clean-baseline prerequisite (only enable this check for repos that already pass `tsc --noEmit` on `main`, post-install), (b) a baseline-diagnostics comparison (run `tsc --noEmit` before and after applying the generated changes, on the whole project, and fail if any diagnostic — identified by file + line + error code, not merely the total count — is present after that wasn't present before), or (c) a derived, isolated `tsconfig.json` (generated on the fly, `include`-scoped to just the changed files plus their transitive local imports) so project mode has a narrow enough surface. Comparing raw counts instead of diagnostic identity in (b) would be unsound: a multi-file patch that introduces one new error while incidentally fixing an unrelated pre-existing one leaves the count flat or lower, letting exactly the class of defect this ADR targets slip through a count-only gate. Options (a) and (b) share a further gap: `tsc` only type-checks files reachable through the governing `tsconfig.json`'s `files`/`include`/`exclude` (or project-reference boundaries) — a generated file the target repo's config doesn't cover is silently absent from the compiled program, so `tsc --noEmit` reports success without ever having looked at it, missing the exact read-only-property class of bug this gate exists to catch. Whichever of (a)/(b) is chosen must therefore first confirm every changed TypeScript path is actually part of the checked program (e.g. cross-check against `tsc --listFilesOnly`) and fall back to the isolated-config approach (c) — which sidesteps this by construction, since it's built from the changed files directly — for any path that isn't. This needs to be resolved concretely in the implementation PR, not assumed away — treat the dependency-installation step (including its `--ignore-scripts`/frozen-install requirements), "which of (a)/(b)/(c)", the program-coverage check, and whether `tsc` itself needs sandboxing beyond script-disabling as open implementation questions this ADR does not settle. For the generation stage, both checks are advisory-to-blocking: a failure prevents the PR from being opened (or triggers a re-generation attempt), rather than being left for the AI reviewer to notice. For auto-fix, a failure should count against the existing 3-attempt bound (`MAX_ATTEMPTS` in `auto_fix_pr.mjs`, tracked via `auto-fix-attempt-N` labels) the same way an unhelpful fix already does, rather than opening a new, separate retry budget — and whether it also changes the `changes-requested`/`review-approved` labeling is an open implementation question, not decided here. Either way, this benchmark's finding is precisely that the reviewer cannot be relied upon to notice these defects reliably on its own. From 04190f7dfad717652d422d2fbaa06cfdfbf7bbd9 Mon Sep 17 00:00:00 2001 From: koydas Date: Mon, 27 Jul 2026 20:03:01 +0000 Subject: [PATCH 11/12] fix(adr): exempt configured import aliases, fix line-number-fragile diagnostic identity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 --- docs/adr/0019-static-verification-backstop.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/adr/0019-static-verification-backstop.md b/docs/adr/0019-static-verification-backstop.md index cc5b3b6..9aaa648 100644 --- a/docs/adr/0019-static-verification-backstop.md +++ b/docs/adr/0019-static-verification-backstop.md @@ -22,8 +22,8 @@ This reopens the question ADR-0009 settled by prompt-only means: **prompt wordin Add an optional, opt-in static verification step. For the generation stage, it runs immediately after `writeGeneratedFiles()`, before a PR is opened. For auto-fix, there is no "before a PR is opened" moment — `auto-fix-pr.yml` only runs after a label is applied to an *already-open* PR, checks out its existing branch, and commits/pushes generated files to it — so the equivalent gate there runs immediately after auto-fix writes its generated files, before its commit/push step: -1. **Import allowlist check** (root-manifest scope; per-repo config beyond `package.json` is only needed for workspace/monorepo setups, see caveat below): the two snapshots this check needs happen at different points, not both "before the LLM call" — `target_path` values are chosen by the LLM itself, so which files even need a pre-generation import snapshot isn't knowable until its response exists. Snapshot `package.json`'s dependency fields **before** calling the generation LLM (the same read already performed to build the "Allowed npm dependencies" context block, see #157) — this one is repo-wide and doesn't depend on the response. Snapshot each target file's own pre-generation import/require specifiers (empty for a file that doesn't exist yet) **after** `validateAiOutput()` returns `changes` (so `target_path` values are known) but **before** `writeGeneratedFiles()` overwrites them — in `generate_issue_change.mjs`'s current flow, that's the narrow window between those two calls. After writing, extract `import`/`require` specifiers from each generated/modified JS/TS file. First discard any relative or absolute-path specifier (starts with `./`, `../`, or `/`) — these are project-local file references, never package names, and `generation-user.md`'s own requirement 7 already permits them; the allowlist check applies only to bare specifiers (`react`, `@scope/pkg`, etc.), which is what "package" means in this context. Among the remaining bare specifiers, normalize each to its package root before comparing against the manifest — a public subpath import like `react/jsx-runtime`, `date-fns/format`, or `@scope/pkg/subpath` is authorized by the same `package.json` entry (`react`, `date-fns`, `@scope/pkg`) as a bare import of the package itself, but won't literally match that key unless the first one or two path segments (the second only for a `@scope/` specifier) are taken as the comparison key. Then flag only those normalized names that are newly added by this patch (not present in that file's own pre-generation snapshot) AND are neither in the pre-generation `package.json` snapshot nor in a small maintained list of language/runtime built-ins. Checking only newly-added specifiers matters: an existing file may legitimately already import a package absent from the root manifest (workspace-local dependency, or one this scan doesn't otherwise see) — the generation guardrail explicitly allows imports "already used elsewhere in the file", and scanning every specifier in the completed file rather than only what changed would reject that legitimate, unrelated-to-this-patch import. Comparing against the pre-generation `package.json` snapshot, not the post-write working tree, is separately load-bearing: since the model's own `changes` can include a rewritten `package.json` (up to 6 files per the existing HARD LIMIT), checking against the working-tree file after `writeGeneratedFiles()` would let the model add both the import and the corresponding manifest entry in the same patch and pass trivially, which defeats the check's purpose. This is a regex/AST-level check, not a full build — cheap, deterministic, language-aware only for JS/TS. **Workspace/monorepo caveat:** this check only reads the repository-root manifest (matching #157's scope). A *new* file inside a workspace package (e.g. `packages/app/`) that legitimately imports a dependency declared only in that package's own nested `package.json` — not the root one — would be wrongly flagged, since it has no pre-generation snapshot of its own to fall back on (it doesn't exist yet) and the root manifest doesn't list it. Resolving this requires snapshotting the nearest governing `package.json` per generated file path, not just the root one — left as an open implementation detail, not assumed to already work for every repository layout. -2. **Type-check pass** (opt-in per target repo, since not every repo this pipeline touches is TypeScript): if the target repo has a `tsconfig.json`, running `tsc --noEmit` isn't a drop-in solution for several reasons. First, a prerequisite this proposal initially glossed over: `code-generation.yml` and `auto-fix-pr.yml` currently only check out the repository and set up Node for *this* repo (`autonomous-dev-loop` itself) — neither installs the *target* repo's dependencies via its package manager (npm/yarn/pnpm) and lockfile. Without that, `tsc` may not even be installed, and even a globally-available compiler would immediately report every imported module and type declaration as missing, so no baseline could ever pass. Enabling this check therefore first requires detecting the target repo's package manager and running its install step (or invoking a repo-configured validation command) before any of the following applies. **This install step is itself a security boundary, not a detail to defer**: by default, npm/yarn/pnpm all execute lifecycle scripts (`preinstall`/`install`/`postinstall`) declared in `package.json` during install — npm does not set `ignore-scripts` to true by default. Since this runs inside the existing generation/auto-fix job (which holds `GITHUB_TOKEN`/`AI_PR_TOKEN` and a checkout of the repo, including whatever the generated patch or an external contributor's PR branch contains), an unverified `package.json` with a malicious or merely-compromised `postinstall` script would execute with those credentials *before* any of this ADR's verification has run — the opposite of the trust boundary this ADR is trying to establish. The install must therefore (a) run with lifecycle scripts disabled AND as a frozen/immutable install rather than a mutating one — `npm install --ignore-scripts` alone still permits npm to rewrite `package-lock.json` on a stale or older-format lockfile; use `npm ci --ignore-scripts` instead, which exits on a manifest/lockfile mismatch and never writes to either file (equivalently, `yarn install --immutable --ignore-scripts` or `pnpm install --frozen-lockfile --ignore-scripts`) — and (b) be based on the pre-generation manifest/lockfile snapshot, not the post-write working tree, for the same reason snapshot-timing matters for the import-allowlist check above. The frozen-install requirement also matters operationally for auto-fix specifically: `auto-fix-pr.yml`'s commit step currently stages with `git add -A`, so a mutating install that rewrote the lockfile as a side effect of *verification* would get committed as if it were part of the generated fix. Second, even with dependencies installed, `tsc`'s CLI rejects combining `--project` with an explicit file list (error TS5042), so scoping to just the generated files isn't directly available, and running full project-mode `tsc` would fail on *any* pre-existing type error anywhere in the project, blocking every generated PR regardless of whether the generated files themselves are valid. A workable version needs one of: (a) a clean-baseline prerequisite (only enable this check for repos that already pass `tsc --noEmit` on `main`, post-install), (b) a baseline-diagnostics comparison (run `tsc --noEmit` before and after applying the generated changes, on the whole project, and fail if any diagnostic — identified by file + line + error code, not merely the total count — is present after that wasn't present before), or (c) a derived, isolated `tsconfig.json` (generated on the fly, `include`-scoped to just the changed files plus their transitive local imports) so project mode has a narrow enough surface. Comparing raw counts instead of diagnostic identity in (b) would be unsound: a multi-file patch that introduces one new error while incidentally fixing an unrelated pre-existing one leaves the count flat or lower, letting exactly the class of defect this ADR targets slip through a count-only gate. Options (a) and (b) share a further gap: `tsc` only type-checks files reachable through the governing `tsconfig.json`'s `files`/`include`/`exclude` (or project-reference boundaries) — a generated file the target repo's config doesn't cover is silently absent from the compiled program, so `tsc --noEmit` reports success without ever having looked at it, missing the exact read-only-property class of bug this gate exists to catch. Whichever of (a)/(b) is chosen must therefore first confirm every changed TypeScript path is actually part of the checked program (e.g. cross-check against `tsc --listFilesOnly`) and fall back to the isolated-config approach (c) — which sidesteps this by construction, since it's built from the changed files directly — for any path that isn't. This needs to be resolved concretely in the implementation PR, not assumed away — treat the dependency-installation step (including its `--ignore-scripts`/frozen-install requirements), "which of (a)/(b)/(c)", the program-coverage check, and whether `tsc` itself needs sandboxing beyond script-disabling as open implementation questions this ADR does not settle. +1. **Import allowlist check** (root-manifest scope; per-repo config beyond `package.json` is only needed for workspace/monorepo setups, see caveat below): the two snapshots this check needs happen at different points, not both "before the LLM call" — `target_path` values are chosen by the LLM itself, so which files even need a pre-generation import snapshot isn't knowable until its response exists. Snapshot `package.json`'s dependency fields **before** calling the generation LLM (the same read already performed to build the "Allowed npm dependencies" context block, see #157) — this one is repo-wide and doesn't depend on the response. Snapshot each target file's own pre-generation import/require specifiers (empty for a file that doesn't exist yet) **after** `validateAiOutput()` returns `changes` (so `target_path` values are known) but **before** `writeGeneratedFiles()` overwrites them — in `generate_issue_change.mjs`'s current flow, that's the narrow window between those two calls. After writing, extract `import`/`require` specifiers from each generated/modified JS/TS file. First discard any relative or absolute-path specifier (starts with `./`, `../`, or `/`) — these are project-local file references, never package names, and `generation-user.md`'s own requirement 7 already permits them. Also discard any specifier matching the target repo's own configured import aliases before treating the rest as npm packages: repos commonly resolve subpath imports like `#utils` via `package.json`'s `imports` field, or aliases like `@/utils` via `tsconfig.json`'s `compilerOptions.paths` — these resolve to project-local files too, just through a different mechanism than a relative path, and would otherwise be misidentified as unauthorized bare package specifiers (a `@/`-prefixed alias in particular looks exactly like a scoped package specifier). Read both config files (when present) as part of building the per-file snapshot and exempt any specifier they map. Only what's left after both of these exemptions is "bare" in the sense that "package" means in this context (`react`, `@scope/pkg`, etc.). Among the remaining bare specifiers, normalize each to its package root before comparing against the manifest — a public subpath import like `react/jsx-runtime`, `date-fns/format`, or `@scope/pkg/subpath` is authorized by the same `package.json` entry (`react`, `date-fns`, `@scope/pkg`) as a bare import of the package itself, but won't literally match that key unless the first one or two path segments (the second only for a `@scope/` specifier) are taken as the comparison key. Then flag only those normalized names that are newly added by this patch (not present in that file's own pre-generation snapshot) AND are neither in the pre-generation `package.json` snapshot nor in a small maintained list of language/runtime built-ins. Checking only newly-added specifiers matters: an existing file may legitimately already import a package absent from the root manifest (workspace-local dependency, or one this scan doesn't otherwise see) — the generation guardrail explicitly allows imports "already used elsewhere in the file", and scanning every specifier in the completed file rather than only what changed would reject that legitimate, unrelated-to-this-patch import. Comparing against the pre-generation `package.json` snapshot, not the post-write working tree, is separately load-bearing: since the model's own `changes` can include a rewritten `package.json` (up to 6 files per the existing HARD LIMIT), checking against the working-tree file after `writeGeneratedFiles()` would let the model add both the import and the corresponding manifest entry in the same patch and pass trivially, which defeats the check's purpose. This is a regex/AST-level check, not a full build — cheap, deterministic, language-aware only for JS/TS. **Workspace/monorepo caveat:** this check only reads the repository-root manifest (matching #157's scope). A *new* file inside a workspace package (e.g. `packages/app/`) that legitimately imports a dependency declared only in that package's own nested `package.json` — not the root one — would be wrongly flagged, since it has no pre-generation snapshot of its own to fall back on (it doesn't exist yet) and the root manifest doesn't list it. Resolving this requires snapshotting the nearest governing `package.json` per generated file path, not just the root one — left as an open implementation detail, not assumed to already work for every repository layout. +2. **Type-check pass** (opt-in per target repo, since not every repo this pipeline touches is TypeScript): if the target repo has a `tsconfig.json`, running `tsc --noEmit` isn't a drop-in solution for several reasons. First, a prerequisite this proposal initially glossed over: `code-generation.yml` and `auto-fix-pr.yml` currently only check out the repository and set up Node for *this* repo (`autonomous-dev-loop` itself) — neither installs the *target* repo's dependencies via its package manager (npm/yarn/pnpm) and lockfile. Without that, `tsc` may not even be installed, and even a globally-available compiler would immediately report every imported module and type declaration as missing, so no baseline could ever pass. Enabling this check therefore first requires detecting the target repo's package manager and running its install step (or invoking a repo-configured validation command) before any of the following applies. **This install step is itself a security boundary, not a detail to defer**: by default, npm/yarn/pnpm all execute lifecycle scripts (`preinstall`/`install`/`postinstall`) declared in `package.json` during install — npm does not set `ignore-scripts` to true by default. Since this runs inside the existing generation/auto-fix job (which holds `GITHUB_TOKEN`/`AI_PR_TOKEN` and a checkout of the repo, including whatever the generated patch or an external contributor's PR branch contains), an unverified `package.json` with a malicious or merely-compromised `postinstall` script would execute with those credentials *before* any of this ADR's verification has run — the opposite of the trust boundary this ADR is trying to establish. The install must therefore (a) run with lifecycle scripts disabled AND as a frozen/immutable install rather than a mutating one — `npm install --ignore-scripts` alone still permits npm to rewrite `package-lock.json` on a stale or older-format lockfile; use `npm ci --ignore-scripts` instead, which exits on a manifest/lockfile mismatch and never writes to either file (equivalently, `yarn install --immutable --ignore-scripts` or `pnpm install --frozen-lockfile --ignore-scripts`) — and (b) be based on the pre-generation manifest/lockfile snapshot, not the post-write working tree, for the same reason snapshot-timing matters for the import-allowlist check above. The frozen-install requirement also matters operationally for auto-fix specifically: `auto-fix-pr.yml`'s commit step currently stages with `git add -A`, so a mutating install that rewrote the lockfile as a side effect of *verification* would get committed as if it were part of the generated fix. Second, even with dependencies installed, `tsc`'s CLI rejects combining `--project` with an explicit file list (error TS5042), so scoping to just the generated files isn't directly available, and running full project-mode `tsc` would fail on *any* pre-existing type error anywhere in the project, blocking every generated PR regardless of whether the generated files themselves are valid. A workable version needs one of: (a) a clean-baseline prerequisite (only enable this check for repos that already pass `tsc --noEmit` on `main`, post-install), (b) a baseline-diagnostics comparison (run `tsc --noEmit` before and after applying the generated changes, on the whole project, and fail if any diagnostic is present after that wasn't present before), or (c) a derived, isolated `tsconfig.json` (generated on the fly, `include`-scoped to just the changed files plus their transitive local imports) so project mode has a narrow enough surface. Comparing raw counts instead of diagnostic identity in (b) would be unsound: a multi-file patch that introduces one new error while incidentally fixing an unrelated pre-existing one leaves the count flat or lower, letting exactly the class of defect this ADR targets slip through a count-only gate. Naively keying that identity by file + line number + error code is its own trap, though: any generated edit that inserts or removes lines earlier in the same file shifts every unrelated pre-existing diagnostic below it to a new line, which a literal line-number comparison would misclassify as newly introduced — a false failure on an untouched error, not a real regression. A workable identity needs to either map each baseline diagnostic's line through the generated diff's hunk offsets before comparing, or use a representation less sensitive to line movement (e.g. file + error code + the enclosing declaration/symbol name). Which of these — not just whether to key by line number at all — is left to the implementation PR. Options (a) and (b) share a further gap: `tsc` only type-checks files reachable through the governing `tsconfig.json`'s `files`/`include`/`exclude` (or project-reference boundaries) — a generated file the target repo's config doesn't cover is silently absent from the compiled program, so `tsc --noEmit` reports success without ever having looked at it, missing the exact read-only-property class of bug this gate exists to catch. Whichever of (a)/(b) is chosen must therefore first confirm every changed TypeScript path is actually part of the checked program (e.g. cross-check against `tsc --listFilesOnly`) and fall back to the isolated-config approach (c) — which sidesteps this by construction, since it's built from the changed files directly — for any path that isn't. This needs to be resolved concretely in the implementation PR, not assumed away — treat the dependency-installation step (including its `--ignore-scripts`/frozen-install requirements), "which of (a)/(b)/(c)", the program-coverage check, and whether `tsc` itself needs sandboxing beyond script-disabling as open implementation questions this ADR does not settle. For the generation stage, both checks are advisory-to-blocking: a failure prevents the PR from being opened (or triggers a re-generation attempt), rather than being left for the AI reviewer to notice. For auto-fix, a failure should count against the existing 3-attempt bound (`MAX_ATTEMPTS` in `auto_fix_pr.mjs`, tracked via `auto-fix-attempt-N` labels) the same way an unhelpful fix already does, rather than opening a new, separate retry budget — and whether it also changes the `changes-requested`/`review-approved` labeling is an open implementation question, not decided here. Either way, this benchmark's finding is precisely that the reviewer cannot be relied upon to notice these defects reliably on its own. From a9153d8734246b0cbfca13b640e447e20a0bd8e3 Mon Sep 17 00:00:00 2001 From: koydas Date: Mon, 27 Jul 2026 20:07:16 +0000 Subject: [PATCH 12/12] fix(adr): correct factual claim that abort-controller doesn't exist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- docs/adr/0019-static-verification-backstop.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/adr/0019-static-verification-backstop.md b/docs/adr/0019-static-verification-backstop.md index 9aaa648..5efa652 100644 --- a/docs/adr/0019-static-verification-backstop.md +++ b/docs/adr/0019-static-verification-backstop.md @@ -9,7 +9,7 @@ ADR-0009 addressed LLM guardrail violations (test-suite deletion, module-format A benchmark session run against a locally-hosted 7B coding model (`qwen2.5-coder:7b-instruct-q4_0`, used here as a stand-in for "a capable but not frontier-tier LLM") fed the exact production `generation-system.md`/`generation-user.md` prompts a synthetic issue requesting a React hook. The output: -1. Imported `AbortController` from a nonexistent `abort-controller` npm package — the same class of violation ADR-0009's "never introduce external packages" guardrail was written to prevent (that ADR's motivating incident was `require('nyc')` introduced without being in `package.json`). +1. Imported `AbortController` from the `abort-controller` npm package — a real, published package (a pre-Node-15/older-browser polyfill), but one never declared in this project's `package.json` and unnecessary here since `AbortController` is a native global. The defect is the undeclared/unauthorized dependency, not that the package doesn't exist — the same class of violation ADR-0009's "never introduce external packages" guardrail was written to prevent (that ADR's motivating incident was `require('nyc')` introduced without being in `package.json`). 2. Assigned to `AbortController.prototype.signal`, a getter-only accessor. Verified empirically in a real ES-module environment (Node's native ESM loader, matching browser strict-mode semantics): this throws `TypeError` and crashes the entire React component tree on first use. Only the first of these violated an explicit guardrail already present in the prompt at benchmark time (`generation-system.md`'s HARD GUARDRAILS forbid introducing new external packages). The read-only-property assignment was not covered by any existing guardrail prose — it's a general code-correctness failure the review step should have caught on its own merits, not a case of the model ignoring an explicit instruction. (Companion PR #155 has since added an explicit self-check for this exact pattern, closing that specific gap at the prompt level — this ADR's static-verification proposal is an additional, tool-based backstop on top of that, not a substitute for it.) The generated diff was then run through the production `pr-review-system.md` prompt (same benchmark session) and returned `APPROVED`, `Issues Found: None` — the paired review step did not catch either defect, nor the complete absence of unit tests the issue had explicitly requested.