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..5efa652 --- /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 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. + +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. 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. 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. + +## 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 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. + +**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 + +- ✅ 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. 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 + +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)