diff --git a/.changeset/eval-json-extraction-hardening.md b/.changeset/eval-json-extraction-hardening.md deleted file mode 100644 index 51948574..00000000 --- a/.changeset/eval-json-extraction-hardening.md +++ /dev/null @@ -1,7 +0,0 @@ ---- -"review": patch ---- - -review: eval-harness JSON extraction survives prose braces and invalid string escapes - -Every live seam (finder and validator output in the producer, judge scoring, the match arbiter) sliced model output with `/\{[\s\S]*\}/` (first `{` through last `}`) plus a strict `JSON.parse`. Two recurring live failures follow from that rule: an agent that quotes a template literal from the diff (`` `user-profile:${tenantId}` ``) before its JSON payload fails with "Expected property name or '}'" (the standing incident-cache-missing-key agent failures; the retry re-quotes the same snippet, so it never recovers), and one invalid string escape from the judge (`\'`) kills a whole arm's scoring ("Bad escaped character in JSON"). The new shared `extractJsonObject` walks balanced brace candidates left to right, retries a failed slice with invalid string escapes repaired, and returns the last top-level object that parses. The match arbiter also stops silently reporting "no match" when its yes verdict rides alongside prose braces. diff --git a/.changeset/gh-aw-v0834.md b/.changeset/gh-aw-v0834.md deleted file mode 100644 index 56574213..00000000 --- a/.changeset/gh-aw-v0834.md +++ /dev/null @@ -1,13 +0,0 @@ ---- -"review": patch ---- - -Move the reviewer onto gh-aw v0.83.4 (from v0.81.6) and retire the firewall workaround that release makes obsolete. Three parts. - -(1) The toolchain: this repo's installed reviewer is recompiled with v0.83.4, which bumps the pinned setup action, the MCP gateway (v0.3.30 to v0.4.6), the GitHub MCP server (v1.4.0 to v1.7.0), the Claude Code CLI (2.1.191 to 2.1.220), and the `actions/*` pins. Two execution-model changes ride along and are the reason this lands as its own change rather than inside a feature PR: the awf sandbox now runs **rootless** (no `sudo`, `install_awf_binary.sh --rootless`) and the MCP gateway moves from host networking to a bridge network the sandbox attaches to (`isolation: true`, `topologyAttach: [awmg-mcpg]`, `MCP_GATEWAY_DOMAIN: awmg-mcpg`). The compiled invariants the reviewer depends on were re-verified under v0.83.4 on a scratch consumer layout: the dispatch-conformance gate still compiles between "Ingest agent output" and "Upload agent artifacts", the `engine.env` Bash-timeout override still replaces the generated 60s defaults on the engine step (`BASH_MAX_TIMEOUT_MS: 1200000`), inline sub-agents still extract to `.claude/agents` (what the dispatcher reads), the safe-output handler set and `upload_artifact` `allowed-paths` are byte-identical, and no step was dropped. - -(2) The firewall pin and the `models:` pricing override are removed, from the shared frontmatter and from this repo's installed copy (the latter as a marked `KHAN/ACTIONS LOCAL OVERRIDE`, since the install stays on `review-v1.7.0` until its next bump). **Do not re-pin `sandbox.agent.version` below v0.27.42 while on gh-aw >= v0.83:** v0.83.4 compiles the agent to reach the MCP gateway over a bridge network (`MCP_GATEWAY_DOMAIN: awmg-mcpg`, `network.isolation`, `network.topologyAttach`), and firewall v0.27.27 implements none of those keys — they are absent from its resolved config, and its squid allowlist carries `.host.docker.internal` with no route to `awmg-mcpg`. Measured on two runs of Khan/actions#296: with the pin (run 30290472047), 3 `TCP_DENIED` 403s on `POST awmg-mcpg:8080/mcp/github` and `/mcp/safeoutputs` — a gateway path the lock expects to work, rejected — though MCP still functioned over the path that bypasses squid (10 `tools/call`) and the run's failure was the pre-existing 20-minute step timeout, not the denials; without the pin (run 30292838824, firewall v0.27.42), zero firewall denials, 16 `tools/call` including the safe-output posts, and a posted review. The removal is also correct on its own terms: the pin existed only because claude-fable-5 was absent from the AI-credits pricing table of the firewall the old gh-aw defaulted to (v0.27.11), which 400s an un-priced model, and v0.27.42 prices it while pinning every container by digest. `sandbox.agent.id: awf` stays declared, since the api-proxy is what meters AI credits and caps a runaway fan-out. - -(3) Sub-agent retries are restored in code. v0.83 sets `ANTHROPIC_MAX_RETRIES=0` on the engine step so a terminal error (403 `ai_credits_limit_exceeded`) reaches the harness that owns retry/backoff for 429/529 — that harness wraps the orchestrator process only. The dispatcher's sub-agents are spawned inside it with no such wrapper, so they would have inherited 0 and turned any transient overload into a shed lens on every scripted run. `dispatch-runner.ts` now passes the SDK's `env` option with `ANTHROPIC_MAX_RETRIES` back at the SDK default for the sub-agent subprocesses alone (spreading `process.env`, since that option replaces the environment rather than merging it). - -Also disables gh-aw's newly generated `agentics-maintenance.yml` (`.github/workflows/aw.json`: `{"maintenance": false}`) in this repo rather than adopting a daily scheduled cleanup workflow as a side effect of a version bump; adopt it deliberately if wanted. diff --git a/.changeset/review-decision-chokepoints.md b/.changeset/review-decision-chokepoints.md deleted file mode 100644 index 3e42decf..00000000 --- a/.changeset/review-decision-chokepoints.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -"review": minor ---- - -The decision chokepoints become code (deterministic-orchestrator slice 3, the ledger deferred from the 07-13 bot-feedback pass). Three additions. First, the dispatch-conformance gate gains three rules at the submit chokepoint: an APPROVE cannot be queued alongside a blocking-labeled inline comment (Step 4's verdict is a mechanical function of the labels; the #40561 miscalibration direction), the reduced-depth flip veto is enforced in code (at flip-gated/fast depth over a prior REQUEST_CHANGES stamp, APPROVE requires `rereview.json` `keptBlockingCount: 0`; the #246 flip-gate chokepoint, previously prompt-trusted), and every queued thread resolution must be one the reconciler decided (`out/thread-reconciler.json` `resolve`; resolving a kept thread, or resolving with no reconciler run, blocks), with the deficit direction reported as executed-vs-decided accounting in the gate report (#244's ledger), never blocked. Second, the disciplines extraction becomes a pre-step and its verify becomes code (#247): the staging CLI extracts the marker-delimited REVIEW DISCIPLINES section from the rendered prompt and verifies the schema heading before writing `disciplines.md`; the prompt keeps only the byte-for-byte heredoc fallback, and a parity test proves the code extraction byte-identical to the old sed command over the real review.md. Third, the remaining #245 ledger items (out-of-lane handoff shape enforcement, cross-source same-line dedup) are code in the scripted dispatch path (slice 2) and stay prompt-trusted in task mode until the dispatch dial flips; the gate and README document that boundary. diff --git a/.changeset/review-dispatch-gate.md b/.changeset/review-dispatch-gate.md deleted file mode 100644 index 83fe5326..00000000 --- a/.changeset/review-dispatch-gate.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -"review": minor ---- - -The dispatch-conformance gate: a review verdict can no longer be submitted unless the sub-agent outputs it is supposed to summarize actually exist. On the v1.7.0 acceptance trial (Khan/webapp#40992, run 29865480728) the orchestrator skipped its own protocol in production: it ran no router, dispatched zero sub-agents, did no claim validation, reviewed the diff itself, labeled its audit record "streamlined direct review", and submitted a REQUEST_CHANGES that disclosed none of it; the previous day's review of Khan/actions#272 dispatched correctly and disclosed its sheds, so this is stochastic non-conformance the eval suite cannot see by construction (the harness dispatches sub-agents from a script). The gate is code at the submit chokepoint, same family as v1.6.1's non-empty-body rule: a new `post-steps:` step in the agent job (`lib/dispatch-gate.ts`) runs after gh-aw finalizes the safe-output queue and before the queue ships to the `safe_outputs` job that calls the GitHub API. It checks the queued verdict and findings against the staged `out/` files per re-review depth (the correctness pass wherever the depth dispatches one, with the pattern-triage empty-`reviewFiles` waiver; a parseable `claim-validator.json` or its disclosed skipped-dimension note whenever inline comments post; a disclosure note for every reviewer routing planned that never dispatched) and, on violation, strips every posting item from the queue and fails the job: the submission is blocked rather than detected, the run goes red, and the original queue plus the gate report ride the agent artifact for diagnosis. Fail-open only for the gate's own bugs (loud warning, review unblocked); a detected violation never passes silently. diff --git a/.changeset/review-lens-payload-seam.md b/.changeset/review-lens-payload-seam.md deleted file mode 100644 index 1f994464..00000000 --- a/.changeset/review-lens-payload-seam.md +++ /dev/null @@ -1,30 +0,0 @@ ---- -"review": minor ---- - -Add the per-lens consumer payload seam. A consuming repo may now define -`.github/aw/review/lenses/.md` for any of the eleven specialist lenses, plus -`lenses/correctness.md` for the always-on `correctness-reviewer`; each file is -runtime-imported (optional form) into a new "Repo-specific rules and hunts" section -of the matching reviewer prompt, carrying that repo's surface-specific rules and -extra tri-state hunts. Lens names stay generic and shared; only payloads vary per -repo. Behavior-neutral for every current consumer: no consumer carries a payload -file yet, and a missing optional import inlines nothing at runtime. -`correctness-checks.md` remains imported as a deprecated alias for -`lenses/correctness.md` (frontend carries one today); repos should migrate the file -and carry at most one of the two, and the alias is removed in the next major -release. Payloads are additive by contract: the lens prompts state that payload -rules never relax or override the shared rules, which win on any conflict. The -router now warns (through `routingConfig.warnings`, surfaced in the review body's -note lines) when a payload would be silently inert: a filename matching no imported -payload, a specialist payload no ROUTING rule routes, the correctness alias carried -alongside its replacement, the alias carried at all (a deprecation nudge ahead of -its removal), or a `lenses` path that is not a readable directory (which degrades -to a warning instead of crashing the router CLI). The eval's import resolution now matches -production for the optional form (missing resolves to empty, not the "(not -configured for this eval case)" note), so corpus case trees can carry payloads; the -required-form fallback note is unchanged. README documents the new surface and the -three-way contribution rule (shared skeleton vs lens payload vs skills) and fixes -two stale claims in the consumer-config section (the undocumented -`correctness-checks.md`, and the assertion that the optional import form was -dropped). diff --git a/.changeset/review-notified.md b/.changeset/review-notified.md deleted file mode 100644 index 2e73dd3c..00000000 --- a/.changeset/review-notified.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -"review": minor ---- - -Honour Gerald `.github/NOTIFIED` in the reviewer. New `lib/notified.ts` parses the file's `[ON PULL REQUEST]` notify rules (ignoring the documentation preamble above the `----…ignored!----` marker and the `[ON PUSH WITHOUT PULL REQUEST]` section) and matches them against the PR: path globs against the changed-file list, quoted `"/body/flags"` diff regexes against each file's staged unified diff. It groups the matches by `@mention`, carries each rule's label, and renders a ready-to-insert `### Notified` block. On approval, review.md Step 7 runs the CLI (`REVIEW_REPO_ROOT=$GITHUB_WORKSPACE npx -y tsx workflows/review/lib/notified.ts`) and pastes that block into the Review Guidance comment, so a matched person/team is pinged and told the label and which changed files matched them; the notification set feeds the comment's post trigger and its `risksPatternsKey` idempotency signature so a repost re-pings only when the set changes. The matching is a self-contained, practical subset of Gerald's micromatch (`**`, `*`, `?`, `{a,b,c}`, `[…]`, `(a|b)`, `?(…) *(…) +(…) @(…)`) anchored at the repo root, so the lib stays dependency-free (runs under `npx tsx` with no install); it is not a faithful micromatch reimplementation (wildcards match dotfiles, `!(…)` is unsupported — documented in the README). An unsupported glob construct degrades to no match; a malformed rule (bad regex body, unterminated quote) is dropped and surfaces a `Note:` on the PR — never a crashed review. The base-branch copy of `.github/NOTIFIED` is read (like `.github/REVIEWERS`), so a PR cannot inject notify rules that fire before it merges. Delivery of the pings still depends on the workflow's gh-aw `mentions:` allow-list (repository collaborators are allowed by default); the README documents how to widen it for teams. No existing review semantics change; a repo with no `.github/NOTIFIED` gets no section. diff --git a/.changeset/review-pre-agent-staging.md b/.changeset/review-pre-agent-staging.md deleted file mode 100644 index 0b2ed138..00000000 --- a/.changeset/review-pre-agent-staging.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -"review": minor ---- - -Staging becomes a pre-agent step (deterministic-orchestrator slice 1, scoped 07-13 out of the #246 review). A new `lib/stage-pr.ts` runs in `pre-agent-steps:` before the agent starts: it fetches the PR metadata, changed files, and prior bot reviews from the GitHub API, rebuilds the unified diff in the exact header format the provenance parser requires, computes the diff facts (per-file fingerprint plus added-lines hunk signature, staged as `diff-facts.json` so Step 2 compares and Step 9 saves code-computed values) and the newly-changed-code scope against cache memory's `reviewedHunks`, and then runs the deterministic CLI chain the orchestrator previously invoked mid-conversation (router first pass, provenance staging, re-review plan including the `out/` copy, and the scoped swap; at flip-gated depth `pr.diff` / `pr-annotated.diff` / `review-files.json` are staged from `scoped.diff` since no triage runs there). Steps 1 and 3 of the prompt now read the staged files instead of fetching and invoking; #246's interim prompt wiring is deleted. The one model touch stays mid-run by design: when the staged `routing.json` carries `pendingRiskQuestions`, the orchestrator still answers them and runs the router's second pass, which changes only tiers and budget. Every staging failure degrades toward more review, never less (missing cache puts the whole diff in scope, a failed reviews fetch forces a full review), and a hard fetch failure fails the step before any AI spend. Behavior-neutral by construction: the eval producer stages cases through the same lib functions, so A/Bs keep measuring production. diff --git a/.changeset/review-scripted-dispatch.md b/.changeset/review-scripted-dispatch.md deleted file mode 100644 index d06d0d97..00000000 --- a/.changeset/review-scripted-dispatch.md +++ /dev/null @@ -1,9 +0,0 @@ ---- -"review": minor ---- - -Script-driven dispatch and collection (deterministic-orchestrator slice 2), opt-in per repo via the new ROUTING `dispatch scripted` line (default `task` keeps today's Task-tool path). In scripted mode the orchestrator invokes `lib/dispatch.ts` once as a single blocking Bash call; the CLI runs Step 3's phases as code: pattern-triage and the pr.diff staging, the reviewer fan-out with the roster, invocation cap, and planned sheds computed deterministically from `routing.json` (defaults, then matched lenses, then opt-ins by inverse shed ranking; every shed recorded and its Step 6 note line code-rendered), per-dispatch `out/.json` staging (the same evidence the dispatch-conformance gate reads in either mode), output parsing per reviewer contract (label-shape reviewers, structured-schema lenses, out-of-lane handoffs code-labeled `question (non-blocking)`), the change-provenance gate with anchor-snap plus the `snapped.json`/`pre-existing.json` artifacts, the newly-changed-code scope filter with the plain-blocking exception, `claims.json`, the claim-validator dispatch, and the mechanical verification rules (refuted drops, plausible never blocks, confirmed corrections apply, an author-disputed claim caps at a question unless confirmed). Sub-agents are spawned through the Claude Agent SDK against the gh-aw inline agent definitions the activation job extracts to `.claude/agents/`, inside the same firewall sandbox, so the api-proxy meters and caps script-spawned sub-agents exactly like Task-spawned ones (verified from a production run's awf config). Supporting changes: the engine env raises the Bash tool ceiling for the one blocking dispatcher call, a conditional pre-agent `npm ci` installs the pinned SDK only for repos that opted in, and `workflows/review/package.json` pins `@anthropic-ai/claude-agent-sdk`. The scripted path is live-trial-gated before any consumer flips the line: the script-driven eval cannot measure task-vs-scripted dispatch by construction (both of its arms already dispatch from a script), so a seeded live trial per the review-trial protocol is the acceptance instrument. - -Hardening from the 2026-07-22 seeded live trial (Khan/actions#290 comment 5049640985, run 29943085279) and the follow-up degradation audit, applied to the dispatcher this changeset introduces. (1) The cross-source duplicate merge drops its two-line window for same-path pairs (the trial posted the missing-deletion-test defect four times, two copies 43 lines apart in one file, and nothing merged); the calibrated similarity floor carries the precision alone, and the collapse is star-guarded so a bridging claim that bundles two defects can never chain two distinct findings into one silent drop; cross-file merging stays out of scope pending a strictly higher calibration. (2) The label-shape salvage accepts `suggested_patch` as the alternate suggestion key when `suggestion` is absent (`suggestion` wins when both are present), so a ReportFindings-style drift keeps its one-click fix: the trial's AddDate catch carried a committable one-liner that the salvage dropped. (3) `runDispatch` stages `dispatch-result.json` under `out/` so the run artifact carries the per-agent accounting, merges, and suppressions; the trial's post-hoc could not tell structured finals from text fallbacks. (4) A triage `reviewFiles` array with zero string entries reads as triage-unavailable (review everything, disclosed) instead of an empty review: the gate's waiver reads the raw staged array, so filtering to empty was a guaranteed false block. (5) The correctness reviewer's output contract gains one complete example finding naming the required keys and the drift keys not to substitute (format emphasis only). - -Recalibration of that duplicate merge against the 2026-07-26 seeded live trial (Khan/webapp#41118, run 30301235749), where five sources flagged one TTL-unit defect at `expiration.go:38`, none of them merged, and the run posted three blocking comments plus a note for a single defect. The cause was not the floor but the text the floor scored: a label-shape reviewer that omits `failure_scenario` is handed its own subject back as one (the salvage that keeps a missing field from voiding a whole dimension), so the default correctness pass (a one-line title with all of its evidence in `discussion`) reached the comparison as eleven content tokens repeated twice, and scored 0.13-0.23 Jaccard against the four discursive copies on a 0.20 floor. A claim whose failure scenario only restates its subject is now compared on its discussion instead, which is the evidence the reviewer actually wrote; every other claim's compared text is unchanged (feeding the discussion to all claims was measured and rejected: it lifts run 29943085279's distinct same-line issue/thought pair to ten shared bigrams, above every real duplicate). The floors then split in two tiers, since an identical `(path, line)` from two sources is itself evidence of one defect while a differing line is weak evidence of two: an exact anchor clears at 0.14 Jaccard / 0.34 overlap / 4 shared bigrams, any other line on the same path at 0.20 / 0.35 / 6. The tighter other-line tier fixes the same run's false merge, which folded the skill-auditor's AddDate handoff at `:38` into the unrelated missing-test todo at `:62` on five shared bigrams; both real different-line duplicates share six or more. Replayed over the two runs whose real pre-validation claim sets survive as artifacts, all 58 same-path cross-source pairs now land on their ground truth: run 30301235749 collapses its five `:38` copies into the correctness issue and keeps the `:62` todo, run 29897276810's three-way `:38` group merges exactly as before, and no other pair moves. Run 29943085279 predates the scripted dispatcher and staged no `claims.json`, so its pairs are held by fixtures checked against the comments it actually posted. Both floors are minima over real claims, so the margins are thin by construction (the exact-anchor tier separates on 4 shared bigrams against 3, the other-line tier on 6 against 5); `dedup.test.ts` carries the run's real claim texts as fixtures so the next recalibration re-derives them rather than nudging them. diff --git a/.changeset/review-scripted-only.md b/.changeset/review-scripted-only.md deleted file mode 100644 index 92896467..00000000 --- a/.changeset/review-scripted-only.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -"review": minor ---- - -Scripted dispatch becomes the only mode: task mode is removed. The lifecycle trial (webapp#41010 vs #41013) was the acceptance instrument the scripted probe was gated on; it matched task mode's verdicts on all three rounds while running 8-17% cheaper with a tighter, deduplicated comment set, so the migration plan's slice-4 shape is now the pipeline, not the probe. The orchestrator prompt drops roughly 780 lines of turn-by-turn protocol (Phases 1-3, the task-mode Steps 4-6 and Step 9 record spec, and the budget-guardrail choreography whose shed arithmetic the dispatcher already owns); Steps 4-6 remain as short stubs naming the plan CLI as their owner so cross-references stay stable, and the thread-staging spec moves inline into the Step 3 pipeline. The ROUTING `dispatch` dial is retired: a leftover line warns and is ignored (`routing.json` always carries `dispatchMode: "scripted"`), and the Agent SDK install pre-step is unconditional. Consumers adopt the flip with their next release bump; rollback is release-level pinning. The next seeded lifecycle trial on this pin doubles as the live validation of the structured-final, open-thread-suppression, and deterministic-cache-write follow-ups, which have not yet run live together. diff --git a/.changeset/review-submission-plan.md b/.changeset/review-submission-plan.md deleted file mode 100644 index c6fde3e4..00000000 --- a/.changeset/review-submission-plan.md +++ /dev/null @@ -1,7 +0,0 @@ ---- -"review": minor ---- - -The submission plan (deterministic-orchestrator slice 4, the production probe of the end state): in scripted dispatch mode, Steps 4-6 become code. After the dispatcher returns, the orchestrator runs `lib/submission.ts` once; it drops any claim anchored on a line the thread-reconciler flagged in `skipLines` (Step 5's defer-to-open-human-threads rule, applied before the verdict so a deferred claim neither posts nor counts), computes the verdict mechanically (Step 4's blocking-label rule via `computeVerdict`, plus the reduced-depth flip floor from `rereview.json`'s `keptBlockingCount` over a prior REQUEST_CHANGES stamp), renders every inline comment from the surviving claims (post-validation labels, rule quotes, suggestion blocks), renders the full review body (verdict head, the code-rendered accountability section, the dispatcher's note lines, the depth/tripwire notes, the fingerprint stamp as the final line, pr-level claims folded in), and stages `submission-plan.json`. The orchestrator's remaining model role is emitting safe outputs that match the plan verbatim, and the dispatch-conformance gate gains the plan-match rule: when a plan is staged, the queued event, body, and inline comments must match it under a sanitizer-tolerant normalization, so any splice, omission, or "improvement" blocks the submission and fails the run (the #244 accountability-splice check, as code). Queueing nothing is permitted only for the exact Step 6 redundant-approval shape: an APPROVE plan with no comments whose body is the bare comment-less approve line, so an approval carrying a shed disclosure can never be dropped on the floor silently. The safe-output emission itself is the remaining seam code cannot own under gh-aw today: the queue is a run-local JSONL append that needs no credentials, but the agent sandbox mounts `${RUNNER_TEMP}/gh-aw` read-only, so only the safeoutputs MCP container can write it; removing the seam wants a writable path into the queue (an upstream mount change, or a post-agent step on the host), neither tested yet. Rides the same live-trial gate as scripted dispatch; nothing changes in task mode. - -Audit hardening for the plan-match rule this changeset introduces: an audit of gh-aw v0.81.6's ingest sanitizer against the rule-7 normalization found unabsorbed transforms that false-block a byte-faithful submission. The deployed URL policy is allowed-only and runs inside code regions, so any cited non-allowlisted https URL (MDN, StackOverflow) came back "(host/redacted)" against the plan's `` placeholder: a guaranteed mismatch on ordinary review prose. The normalization (now its own module, `lib/sanitizer-normalize.ts`) folds https URLs and the domain-redaction token to a host-bearing placeholder, so the sanitizer's own rewrites pass while a link splice that swaps the host still blocks; it also absorbs neutralizeTemplateDelimiters' escaping backslashes (unbackticked `${{ }}` in prose), the bidi-control strip, and the C0/DEL control strip. Every fold applies to both sides, so splice detection loses nothing. diff --git a/.changeset/review-timeout-forty.md b/.changeset/review-timeout-forty.md deleted file mode 100644 index 1f54222f..00000000 --- a/.changeset/review-timeout-forty.md +++ /dev/null @@ -1,15 +0,0 @@ ---- -"review": minor ---- - -Raise the agent job's `timeout-minutes` from 20 to 40. The high-tier -`runBudget.maxWallClockMinutes` soft target is 20, which sat exactly on the old -hard ceiling: a high-tier run's "start landing" signal and its kill point were -the same minute, so the graceful-shed logic could never actually save a heavy -run. Four high-tier runs on Khan/actions died at the ceiling in one evening -(2026-07-21), each after emitting its review outputs but before the -cache-memory update, which made the next re-review start cold and run even -longer. The budget table's soft targets are deliberately unchanged: shedding -starts at the same points as before, and the wider ceiling restores real -headroom between "start landing" and "killed". Consumers pick this up at their -next installed-reviewer bump. diff --git a/.changeset/review-trial-followups.md b/.changeset/review-trial-followups.md deleted file mode 100644 index 19287392..00000000 --- a/.changeset/review-trial-followups.md +++ /dev/null @@ -1,7 +0,0 @@ ---- -"review": minor ---- - -Post-trial follow-ups to the scripted pipeline, the three highest-value suggestions from the lifecycle trial's consolidated report (Khan/actions#284 discussion). (1) Structured sub-agent finals: the scripted dispatcher exposes an in-process `submit_result` MCP tool to every sub-agent whose input is validated against the agent's exact output contract at the tool boundary: a drifted shape (the trial's defect 13: three distinct correctness-reviewer contract drifts in five runs, two of which voided the whole correctness dimension) is rejected back to the model in-session with the precise contract error instead of costing a $2-3 corrective re-dispatch or the dimension; free-text finals remain the fallback, and a payload the tool accepted is salvaged even if the session then dies. (2) Open-thread suppression at dispatch: a candidate claim that describes a defect an open bot thread already tracks (same path plus the #245 dedup's calibrated text-similarity floor; threads the reconciler resolves this run are exempt) is neither re-validated nor re-posted, closing the observed duplicate-open-threads defect (trial S4 r2: the same missing-test defect open at two anchors) and implementing the keep-thread dedup scripted mode previously had no code for; a suppressed blocking candidate still floors the verdict at REQUEST_CHANGES via the kept-blocking rule, so suppression can never flip a run to APPROVE over an unfixed blocking objection. (3) The Step 9 cache write moves into the deterministic layer: in scripted mode the orchestrator invokes `lib/cache-record.ts` once after emitting safe outputs (it cannot be a post-step; the compiled agent job commits and uploads cache-memory before post-steps run), and the CLI writes the cache-memory record (the divergence tripwire's fingerprint carrier) by copying `diffFingerprint`/`reviewedHunks`/`stampHunks` verbatim from staged files and corroborating the verdict against the in-run safe-output queue (`GH_AW_SAFE_OUTPUTS`), refusing to write when they disagree or when no queue is readable at all (the prior record then stands, degrading toward a fuller review, never a cheaper one; the one shape that legitimately queues nothing, the Step 6 redundant-approval skip, still records); `risksPatternsKey` becomes code-owned end to end (the plan CLI stages the canonical signature Step 7 compares, the writer records it when the guidance comment queues), and `requestedTeams` accumulates from the queued `add_reviewer` items. Task mode is untouched by all three (the writer no-ops without a staged plan; the structured-final tool exists only in the scripted dispatcher). - -Audit hardening for the open-thread suppression above: threads.json staging is prompt-executed (review.md asks the orchestrator for the unresolved github-actions[bot] threads), and nothing in code verified either property suppression depends on, so a mis-staged human thread could silently suppress a bot candidate while its free-text opener read as non-blocking and skipped the verdict floor, and a mis-staged already-resolved thread could suppress a genuine regression re-flag. The dispatcher now builds its suppression inputs through `openThreadsFromStaged`, which admits only threads whose opening comment the bot authored and whose staged `resolved` flag (copied from the tool's own `is_resolved`, now part of the threads.json contract) says the thread is still open; fails closed on both (worst case is a duplicate comment). The suppression's blocking-ness read is the label taxonomy's own (`isBlockingLabel`), so `issue (blocking, best-practice)` counts toward the verdict floor like any other blocking label. diff --git a/.changeset/review-trial-skill-lessons.md b/.changeset/review-trial-skill-lessons.md deleted file mode 100644 index 98d9e9b7..00000000 --- a/.changeset/review-trial-skill-lessons.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -"review": patch ---- - -Skill-doc only: fold four operational lessons from the 2026-07-22 sonnet-low trial into the `review-trial` skill. (1) Cost projections and cap sizing now use the enforcement unit, the firewall api-proxy's credit meter (the sum of `ai_credits_this_response` in the run's token-usage log), rather than the run-summary AI-credits figure, which reads ~2.5x lower; the measured basis is restated as 1,050-1,077 proxy credits for a full-depth, full-roster run of the memory-expiration shape. (2) Arm workflows must carry the 2500-credit trial cap (`max-ai-credits` plus the `REVIEW_MAX_AI_CREDITS` mirror), because the shared default of 1000 sits below the metered cost of exactly the runs this skill choreographs and a capped run dies mid-tail with the review emitted but the cache record and artifact upload unfinished. (3) Never label the arm PR with the repo's opt-out: the trial workflow ships the same `skip-ai-review` gate as the installed reviewer, so the label skips the trial arm itself; suppression moves to the scaffolding branch (remove the installed reviewer files and any pull_request-triggered slash-command shim). (4) The arm PR must present a realistic title and body, since the reviewer reads both via `pr-context.json`; the trial disclosure, the label, and the `[reviewer-trial]` prefix move to the scaffolding PR, whose branch the arm workflow's `if:` now excludes so it burns no credits. No change to the shipped review workflow. diff --git a/.changeset/stamp-carrier-cache-memory.md b/.changeset/stamp-carrier-cache-memory.md deleted file mode 100644 index 703450dc..00000000 --- a/.changeset/stamp-carrier-cache-memory.md +++ /dev/null @@ -1,7 +0,0 @@ ---- -"review": patch ---- - -review: the re-review fingerprint anchors on cache memory; the body stamp never survives gh-aw ingest - -gh-aw's safe-output sanitizer strips all XML/HTML comments (`removeXmlComments`), so the hidden fingerprint stamp a review body carries never reaches the PR: every production re-review planned `no-prior-fingerprint` and silently escalated to full depth, making the `re-review` ROUTING dial (scoped/flip-gated/fast) inert. The plan CLI now falls back to the Step 9 cache-memory record (`verdict`, `stampHunks`/`reviewedHunks`, `wasDraft`) when no prior-review body carries a stamp, and records which carrier anchored the plan as `stampSource` in `rereview-plan.json`. Step 9 gains a `stampHunks` field copied verbatim from the plan CLI's own hash computation so hash regimes are never mixed. Cache eviction still degrades to a full review, never a cheaper one. diff --git a/workflows/review/CHANGELOG.md b/workflows/review/CHANGELOG.md index 778c27f4..2159ba02 100644 --- a/workflows/review/CHANGELOG.md +++ b/workflows/review/CHANGELOG.md @@ -1,5 +1,86 @@ # review +## 1.8.0 + +### Minor Changes + +- 2b958df: The decision chokepoints become code (deterministic-orchestrator slice 3, the ledger deferred from the 07-13 bot-feedback pass). Three additions. First, the dispatch-conformance gate gains three rules at the submit chokepoint: an APPROVE cannot be queued alongside a blocking-labeled inline comment (Step 4's verdict is a mechanical function of the labels; the #40561 miscalibration direction), the reduced-depth flip veto is enforced in code (at flip-gated/fast depth over a prior REQUEST_CHANGES stamp, APPROVE requires `rereview.json` `keptBlockingCount: 0`; the #246 flip-gate chokepoint, previously prompt-trusted), and every queued thread resolution must be one the reconciler decided (`out/thread-reconciler.json` `resolve`; resolving a kept thread, or resolving with no reconciler run, blocks), with the deficit direction reported as executed-vs-decided accounting in the gate report (#244's ledger), never blocked. Second, the disciplines extraction becomes a pre-step and its verify becomes code (#247): the staging CLI extracts the marker-delimited REVIEW DISCIPLINES section from the rendered prompt and verifies the schema heading before writing `disciplines.md`; the prompt keeps only the byte-for-byte heredoc fallback, and a parity test proves the code extraction byte-identical to the old sed command over the real review.md. Third, the remaining #245 ledger items (out-of-lane handoff shape enforcement, cross-source same-line dedup) are code in the scripted dispatch path (slice 2) and stay prompt-trusted in task mode until the dispatch dial flips; the gate and README document that boundary. +- 034181f: The dispatch-conformance gate: a review verdict can no longer be submitted unless the sub-agent outputs it is supposed to summarize actually exist. On the v1.7.0 acceptance trial (Khan/webapp#40992, run 29865480728) the orchestrator skipped its own protocol in production: it ran no router, dispatched zero sub-agents, did no claim validation, reviewed the diff itself, labeled its audit record "streamlined direct review", and submitted a REQUEST_CHANGES that disclosed none of it; the previous day's review of Khan/actions#272 dispatched correctly and disclosed its sheds, so this is stochastic non-conformance the eval suite cannot see by construction (the harness dispatches sub-agents from a script). The gate is code at the submit chokepoint, same family as v1.6.1's non-empty-body rule: a new `post-steps:` step in the agent job (`lib/dispatch-gate.ts`) runs after gh-aw finalizes the safe-output queue and before the queue ships to the `safe_outputs` job that calls the GitHub API. It checks the queued verdict and findings against the staged `out/` files per re-review depth (the correctness pass wherever the depth dispatches one, with the pattern-triage empty-`reviewFiles` waiver; a parseable `claim-validator.json` or its disclosed skipped-dimension note whenever inline comments post; a disclosure note for every reviewer routing planned that never dispatched) and, on violation, strips every posting item from the queue and fails the job: the submission is blocked rather than detected, the run goes red, and the original queue plus the gate report ride the agent artifact for diagnosis. Fail-open only for the gate's own bugs (loud warning, review unblocked); a detected violation never passes silently. +- 8d9fc12: Add the per-lens consumer payload seam. A consuming repo may now define + `.github/aw/review/lenses/.md` for any of the eleven specialist lenses, plus + `lenses/correctness.md` for the always-on `correctness-reviewer`; each file is + runtime-imported (optional form) into a new "Repo-specific rules and hunts" section + of the matching reviewer prompt, carrying that repo's surface-specific rules and + extra tri-state hunts. Lens names stay generic and shared; only payloads vary per + repo. Behavior-neutral for every current consumer: no consumer carries a payload + file yet, and a missing optional import inlines nothing at runtime. + `correctness-checks.md` remains imported as a deprecated alias for + `lenses/correctness.md` (frontend carries one today); repos should migrate the file + and carry at most one of the two, and the alias is removed in the next major + release. Payloads are additive by contract: the lens prompts state that payload + rules never relax or override the shared rules, which win on any conflict. The + router now warns (through `routingConfig.warnings`, surfaced in the review body's + note lines) when a payload would be silently inert: a filename matching no imported + payload, a specialist payload no ROUTING rule routes, the correctness alias carried + alongside its replacement, the alias carried at all (a deprecation nudge ahead of + its removal), or a `lenses` path that is not a readable directory (which degrades + to a warning instead of crashing the router CLI). The eval's import resolution now matches + production for the optional form (missing resolves to empty, not the "(not + configured for this eval case)" note), so corpus case trees can carry payloads; the + required-form fallback note is unchanged. README documents the new surface and the + three-way contribution rule (shared skeleton vs lens payload vs skills) and fixes + two stale claims in the consumer-config section (the undocumented + `correctness-checks.md`, and the assertion that the optional import form was + dropped). +- 8b4e56d: Honour Gerald `.github/NOTIFIED` in the reviewer. New `lib/notified.ts` parses the file's `[ON PULL REQUEST]` notify rules (ignoring the documentation preamble above the `----…ignored!----` marker and the `[ON PUSH WITHOUT PULL REQUEST]` section) and matches them against the PR: path globs against the changed-file list, quoted `"/body/flags"` diff regexes against each file's staged unified diff. It groups the matches by `@mention`, carries each rule's label, and renders a ready-to-insert `### Notified` block. On approval, review.md Step 7 runs the CLI (`REVIEW_REPO_ROOT=$GITHUB_WORKSPACE npx -y tsx workflows/review/lib/notified.ts`) and pastes that block into the Review Guidance comment, so a matched person/team is pinged and told the label and which changed files matched them; the notification set feeds the comment's post trigger and its `risksPatternsKey` idempotency signature so a repost re-pings only when the set changes. The matching is a self-contained, practical subset of Gerald's micromatch (`**`, `*`, `?`, `{a,b,c}`, `[…]`, `(a|b)`, `?(…) *(…) +(…) @(…)`) anchored at the repo root, so the lib stays dependency-free (runs under `npx tsx` with no install); it is not a faithful micromatch reimplementation (wildcards match dotfiles, `!(…)` is unsupported — documented in the README). An unsupported glob construct degrades to no match; a malformed rule (bad regex body, unterminated quote) is dropped and surfaces a `Note:` on the PR — never a crashed review. The base-branch copy of `.github/NOTIFIED` is read (like `.github/REVIEWERS`), so a PR cannot inject notify rules that fire before it merges. Delivery of the pings still depends on the workflow's gh-aw `mentions:` allow-list (repository collaborators are allowed by default); the README documents how to widen it for teams. No existing review semantics change; a repo with no `.github/NOTIFIED` gets no section. +- 826e483: Staging becomes a pre-agent step (deterministic-orchestrator slice 1, scoped 07-13 out of the #246 review). A new `lib/stage-pr.ts` runs in `pre-agent-steps:` before the agent starts: it fetches the PR metadata, changed files, and prior bot reviews from the GitHub API, rebuilds the unified diff in the exact header format the provenance parser requires, computes the diff facts (per-file fingerprint plus added-lines hunk signature, staged as `diff-facts.json` so Step 2 compares and Step 9 saves code-computed values) and the newly-changed-code scope against cache memory's `reviewedHunks`, and then runs the deterministic CLI chain the orchestrator previously invoked mid-conversation (router first pass, provenance staging, re-review plan including the `out/` copy, and the scoped swap; at flip-gated depth `pr.diff` / `pr-annotated.diff` / `review-files.json` are staged from `scoped.diff` since no triage runs there). Steps 1 and 3 of the prompt now read the staged files instead of fetching and invoking; #246's interim prompt wiring is deleted. The one model touch stays mid-run by design: when the staged `routing.json` carries `pendingRiskQuestions`, the orchestrator still answers them and runs the router's second pass, which changes only tiers and budget. Every staging failure degrades toward more review, never less (missing cache puts the whole diff in scope, a failed reviews fetch forces a full review), and a hard fetch failure fails the step before any AI spend. Behavior-neutral by construction: the eval producer stages cases through the same lib functions, so A/Bs keep measuring production. +- 5169d0a: Script-driven dispatch and collection (deterministic-orchestrator slice 2), opt-in per repo via the new ROUTING `dispatch scripted` line (default `task` keeps today's Task-tool path). In scripted mode the orchestrator invokes `lib/dispatch.ts` once as a single blocking Bash call; the CLI runs Step 3's phases as code: pattern-triage and the pr.diff staging, the reviewer fan-out with the roster, invocation cap, and planned sheds computed deterministically from `routing.json` (defaults, then matched lenses, then opt-ins by inverse shed ranking; every shed recorded and its Step 6 note line code-rendered), per-dispatch `out/.json` staging (the same evidence the dispatch-conformance gate reads in either mode), output parsing per reviewer contract (label-shape reviewers, structured-schema lenses, out-of-lane handoffs code-labeled `question (non-blocking)`), the change-provenance gate with anchor-snap plus the `snapped.json`/`pre-existing.json` artifacts, the newly-changed-code scope filter with the plain-blocking exception, `claims.json`, the claim-validator dispatch, and the mechanical verification rules (refuted drops, plausible never blocks, confirmed corrections apply, an author-disputed claim caps at a question unless confirmed). Sub-agents are spawned through the Claude Agent SDK against the gh-aw inline agent definitions the activation job extracts to `.claude/agents/`, inside the same firewall sandbox, so the api-proxy meters and caps script-spawned sub-agents exactly like Task-spawned ones (verified from a production run's awf config). Supporting changes: the engine env raises the Bash tool ceiling for the one blocking dispatcher call, a conditional pre-agent `npm ci` installs the pinned SDK only for repos that opted in, and `workflows/review/package.json` pins `@anthropic-ai/claude-agent-sdk`. The scripted path is live-trial-gated before any consumer flips the line: the script-driven eval cannot measure task-vs-scripted dispatch by construction (both of its arms already dispatch from a script), so a seeded live trial per the review-trial protocol is the acceptance instrument. + + Hardening from the 2026-07-22 seeded live trial (Khan/actions#290 comment 5049640985, run 29943085279) and the follow-up degradation audit, applied to the dispatcher this changeset introduces. (1) The cross-source duplicate merge drops its two-line window for same-path pairs (the trial posted the missing-deletion-test defect four times, two copies 43 lines apart in one file, and nothing merged); the calibrated similarity floor carries the precision alone, and the collapse is star-guarded so a bridging claim that bundles two defects can never chain two distinct findings into one silent drop; cross-file merging stays out of scope pending a strictly higher calibration. (2) The label-shape salvage accepts `suggested_patch` as the alternate suggestion key when `suggestion` is absent (`suggestion` wins when both are present), so a ReportFindings-style drift keeps its one-click fix: the trial's AddDate catch carried a committable one-liner that the salvage dropped. (3) `runDispatch` stages `dispatch-result.json` under `out/` so the run artifact carries the per-agent accounting, merges, and suppressions; the trial's post-hoc could not tell structured finals from text fallbacks. (4) A triage `reviewFiles` array with zero string entries reads as triage-unavailable (review everything, disclosed) instead of an empty review: the gate's waiver reads the raw staged array, so filtering to empty was a guaranteed false block. (5) The correctness reviewer's output contract gains one complete example finding naming the required keys and the drift keys not to substitute (format emphasis only). + + Recalibration of that duplicate merge against the 2026-07-26 seeded live trial (Khan/webapp#41118, run 30301235749), where five sources flagged one TTL-unit defect at `expiration.go:38`, none of them merged, and the run posted three blocking comments plus a note for a single defect. The cause was not the floor but the text the floor scored: a label-shape reviewer that omits `failure_scenario` is handed its own subject back as one (the salvage that keeps a missing field from voiding a whole dimension), so the default correctness pass (a one-line title with all of its evidence in `discussion`) reached the comparison as eleven content tokens repeated twice, and scored 0.13-0.23 Jaccard against the four discursive copies on a 0.20 floor. A claim whose failure scenario only restates its subject is now compared on its discussion instead, which is the evidence the reviewer actually wrote; every other claim's compared text is unchanged (feeding the discussion to all claims was measured and rejected: it lifts run 29943085279's distinct same-line issue/thought pair to ten shared bigrams, above every real duplicate). The floors then split in two tiers, since an identical `(path, line)` from two sources is itself evidence of one defect while a differing line is weak evidence of two: an exact anchor clears at 0.14 Jaccard / 0.34 overlap / 4 shared bigrams, any other line on the same path at 0.20 / 0.35 / 6. The tighter other-line tier fixes the same run's false merge, which folded the skill-auditor's AddDate handoff at `:38` into the unrelated missing-test todo at `:62` on five shared bigrams; both real different-line duplicates share six or more. Replayed over the two runs whose real pre-validation claim sets survive as artifacts, all 58 same-path cross-source pairs now land on their ground truth: run 30301235749 collapses its five `:38` copies into the correctness issue and keeps the `:62` todo, run 29897276810's three-way `:38` group merges exactly as before, and no other pair moves. Run 29943085279 predates the scripted dispatcher and staged no `claims.json`, so its pairs are held by fixtures checked against the comments it actually posted. Both floors are minima over real claims, so the margins are thin by construction (the exact-anchor tier separates on 4 shared bigrams against 3, the other-line tier on 6 against 5); `dedup.test.ts` carries the run's real claim texts as fixtures so the next recalibration re-derives them rather than nudging them. + +- 237d540: Scripted dispatch becomes the only mode: task mode is removed. The lifecycle trial (webapp#41010 vs #41013) was the acceptance instrument the scripted probe was gated on; it matched task mode's verdicts on all three rounds while running 8-17% cheaper with a tighter, deduplicated comment set, so the migration plan's slice-4 shape is now the pipeline, not the probe. The orchestrator prompt drops roughly 780 lines of turn-by-turn protocol (Phases 1-3, the task-mode Steps 4-6 and Step 9 record spec, and the budget-guardrail choreography whose shed arithmetic the dispatcher already owns); Steps 4-6 remain as short stubs naming the plan CLI as their owner so cross-references stay stable, and the thread-staging spec moves inline into the Step 3 pipeline. The ROUTING `dispatch` dial is retired: a leftover line warns and is ignored (`routing.json` always carries `dispatchMode: "scripted"`), and the Agent SDK install pre-step is unconditional. Consumers adopt the flip with their next release bump; rollback is release-level pinning. The next seeded lifecycle trial on this pin doubles as the live validation of the structured-final, open-thread-suppression, and deterministic-cache-write follow-ups, which have not yet run live together. +- 616ede7: The submission plan (deterministic-orchestrator slice 4, the production probe of the end state): in scripted dispatch mode, Steps 4-6 become code. After the dispatcher returns, the orchestrator runs `lib/submission.ts` once; it drops any claim anchored on a line the thread-reconciler flagged in `skipLines` (Step 5's defer-to-open-human-threads rule, applied before the verdict so a deferred claim neither posts nor counts), computes the verdict mechanically (Step 4's blocking-label rule via `computeVerdict`, plus the reduced-depth flip floor from `rereview.json`'s `keptBlockingCount` over a prior REQUEST_CHANGES stamp), renders every inline comment from the surviving claims (post-validation labels, rule quotes, suggestion blocks), renders the full review body (verdict head, the code-rendered accountability section, the dispatcher's note lines, the depth/tripwire notes, the fingerprint stamp as the final line, pr-level claims folded in), and stages `submission-plan.json`. The orchestrator's remaining model role is emitting safe outputs that match the plan verbatim, and the dispatch-conformance gate gains the plan-match rule: when a plan is staged, the queued event, body, and inline comments must match it under a sanitizer-tolerant normalization, so any splice, omission, or "improvement" blocks the submission and fails the run (the #244 accountability-splice check, as code). Queueing nothing is permitted only for the exact Step 6 redundant-approval shape: an APPROVE plan with no comments whose body is the bare comment-less approve line, so an approval carrying a shed disclosure can never be dropped on the floor silently. The safe-output emission itself is the remaining seam code cannot own under gh-aw today: the queue is a run-local JSONL append that needs no credentials, but the agent sandbox mounts `${RUNNER_TEMP}/gh-aw` read-only, so only the safeoutputs MCP container can write it; removing the seam wants a writable path into the queue (an upstream mount change, or a post-agent step on the host), neither tested yet. Rides the same live-trial gate as scripted dispatch; nothing changes in task mode. + + Audit hardening for the plan-match rule this changeset introduces: an audit of gh-aw v0.81.6's ingest sanitizer against the rule-7 normalization found unabsorbed transforms that false-block a byte-faithful submission. The deployed URL policy is allowed-only and runs inside code regions, so any cited non-allowlisted https URL (MDN, StackOverflow) came back "(host/redacted)" against the plan's `` placeholder: a guaranteed mismatch on ordinary review prose. The normalization (now its own module, `lib/sanitizer-normalize.ts`) folds https URLs and the domain-redaction token to a host-bearing placeholder, so the sanitizer's own rewrites pass while a link splice that swaps the host still blocks; it also absorbs neutralizeTemplateDelimiters' escaping backslashes (unbackticked `${{ }}` in prose), the bidi-control strip, and the C0/DEL control strip. Every fold applies to both sides, so splice detection loses nothing. + +- 82af000: Raise the agent job's `timeout-minutes` from 20 to 40. The high-tier + `runBudget.maxWallClockMinutes` soft target is 20, which sat exactly on the old + hard ceiling: a high-tier run's "start landing" signal and its kill point were + the same minute, so the graceful-shed logic could never actually save a heavy + run. Four high-tier runs on Khan/actions died at the ceiling in one evening + (2026-07-21), each after emitting its review outputs but before the + cache-memory update, which made the next re-review start cold and run even + longer. The budget table's soft targets are deliberately unchanged: shedding + starts at the same points as before, and the wider ceiling restores real + headroom between "start landing" and "killed". Consumers pick this up at their + next installed-reviewer bump. +- 3592b68: Post-trial follow-ups to the scripted pipeline, the three highest-value suggestions from the lifecycle trial's consolidated report (Khan/actions#284 discussion). (1) Structured sub-agent finals: the scripted dispatcher exposes an in-process `submit_result` MCP tool to every sub-agent whose input is validated against the agent's exact output contract at the tool boundary: a drifted shape (the trial's defect 13: three distinct correctness-reviewer contract drifts in five runs, two of which voided the whole correctness dimension) is rejected back to the model in-session with the precise contract error instead of costing a $2-3 corrective re-dispatch or the dimension; free-text finals remain the fallback, and a payload the tool accepted is salvaged even if the session then dies. (2) Open-thread suppression at dispatch: a candidate claim that describes a defect an open bot thread already tracks (same path plus the #245 dedup's calibrated text-similarity floor; threads the reconciler resolves this run are exempt) is neither re-validated nor re-posted, closing the observed duplicate-open-threads defect (trial S4 r2: the same missing-test defect open at two anchors) and implementing the keep-thread dedup scripted mode previously had no code for; a suppressed blocking candidate still floors the verdict at REQUEST_CHANGES via the kept-blocking rule, so suppression can never flip a run to APPROVE over an unfixed blocking objection. (3) The Step 9 cache write moves into the deterministic layer: in scripted mode the orchestrator invokes `lib/cache-record.ts` once after emitting safe outputs (it cannot be a post-step; the compiled agent job commits and uploads cache-memory before post-steps run), and the CLI writes the cache-memory record (the divergence tripwire's fingerprint carrier) by copying `diffFingerprint`/`reviewedHunks`/`stampHunks` verbatim from staged files and corroborating the verdict against the in-run safe-output queue (`GH_AW_SAFE_OUTPUTS`), refusing to write when they disagree or when no queue is readable at all (the prior record then stands, degrading toward a fuller review, never a cheaper one; the one shape that legitimately queues nothing, the Step 6 redundant-approval skip, still records); `risksPatternsKey` becomes code-owned end to end (the plan CLI stages the canonical signature Step 7 compares, the writer records it when the guidance comment queues), and `requestedTeams` accumulates from the queued `add_reviewer` items. Task mode is untouched by all three (the writer no-ops without a staged plan; the structured-final tool exists only in the scripted dispatcher). + + Audit hardening for the open-thread suppression above: threads.json staging is prompt-executed (review.md asks the orchestrator for the unresolved github-actions[bot] threads), and nothing in code verified either property suppression depends on, so a mis-staged human thread could silently suppress a bot candidate while its free-text opener read as non-blocking and skipped the verdict floor, and a mis-staged already-resolved thread could suppress a genuine regression re-flag. The dispatcher now builds its suppression inputs through `openThreadsFromStaged`, which admits only threads whose opening comment the bot authored and whose staged `resolved` flag (copied from the tool's own `is_resolved`, now part of the threads.json contract) says the thread is still open; fails closed on both (worst case is a duplicate comment). The suppression's blocking-ness read is the label taxonomy's own (`isBlockingLabel`), so `issue (blocking, best-practice)` counts toward the verdict floor like any other blocking label. + +### Patch Changes + +- cc23d7c: review: eval-harness JSON extraction survives prose braces and invalid string escapes + + Every live seam (finder and validator output in the producer, judge scoring, the match arbiter) sliced model output with `/\{[\s\S]*\}/` (first `{` through last `}`) plus a strict `JSON.parse`. Two recurring live failures follow from that rule: an agent that quotes a template literal from the diff (`` `user-profile:${tenantId}` ``) before its JSON payload fails with "Expected property name or '}'" (the standing incident-cache-missing-key agent failures; the retry re-quotes the same snippet, so it never recovers), and one invalid string escape from the judge (`\'`) kills a whole arm's scoring ("Bad escaped character in JSON"). The new shared `extractJsonObject` walks balanced brace candidates left to right, retries a failed slice with invalid string escapes repaired, and returns the last top-level object that parses. The match arbiter also stops silently reporting "no match" when its yes verdict rides alongside prose braces. + +- 98f686f: Move the reviewer onto gh-aw v0.83.4 (from v0.81.6) and retire the firewall workaround that release makes obsolete. Three parts. + + (1) The toolchain: this repo's installed reviewer is recompiled with v0.83.4, which bumps the pinned setup action, the MCP gateway (v0.3.30 to v0.4.6), the GitHub MCP server (v1.4.0 to v1.7.0), the Claude Code CLI (2.1.191 to 2.1.220), and the `actions/*` pins. Two execution-model changes ride along and are the reason this lands as its own change rather than inside a feature PR: the awf sandbox now runs **rootless** (no `sudo`, `install_awf_binary.sh --rootless`) and the MCP gateway moves from host networking to a bridge network the sandbox attaches to (`isolation: true`, `topologyAttach: [awmg-mcpg]`, `MCP_GATEWAY_DOMAIN: awmg-mcpg`). The compiled invariants the reviewer depends on were re-verified under v0.83.4 on a scratch consumer layout: the dispatch-conformance gate still compiles between "Ingest agent output" and "Upload agent artifacts", the `engine.env` Bash-timeout override still replaces the generated 60s defaults on the engine step (`BASH_MAX_TIMEOUT_MS: 1200000`), inline sub-agents still extract to `.claude/agents` (what the dispatcher reads), the safe-output handler set and `upload_artifact` `allowed-paths` are byte-identical, and no step was dropped. + + (2) The firewall pin and the `models:` pricing override are removed, from the shared frontmatter and from this repo's installed copy (the latter as a marked `KHAN/ACTIONS LOCAL OVERRIDE`, since the install stays on `review-v1.7.0` until its next bump). **Do not re-pin `sandbox.agent.version` below v0.27.42 while on gh-aw >= v0.83:** v0.83.4 compiles the agent to reach the MCP gateway over a bridge network (`MCP_GATEWAY_DOMAIN: awmg-mcpg`, `network.isolation`, `network.topologyAttach`), and firewall v0.27.27 implements none of those keys — they are absent from its resolved config, and its squid allowlist carries `.host.docker.internal` with no route to `awmg-mcpg`. Measured on two runs of Khan/actions#296: with the pin (run 30290472047), 3 `TCP_DENIED` 403s on `POST awmg-mcpg:8080/mcp/github` and `/mcp/safeoutputs` — a gateway path the lock expects to work, rejected — though MCP still functioned over the path that bypasses squid (10 `tools/call`) and the run's failure was the pre-existing 20-minute step timeout, not the denials; without the pin (run 30292838824, firewall v0.27.42), zero firewall denials, 16 `tools/call` including the safe-output posts, and a posted review. The removal is also correct on its own terms: the pin existed only because claude-fable-5 was absent from the AI-credits pricing table of the firewall the old gh-aw defaulted to (v0.27.11), which 400s an un-priced model, and v0.27.42 prices it while pinning every container by digest. `sandbox.agent.id: awf` stays declared, since the api-proxy is what meters AI credits and caps a runaway fan-out. + + (3) Sub-agent retries are restored in code. v0.83 sets `ANTHROPIC_MAX_RETRIES=0` on the engine step so a terminal error (403 `ai_credits_limit_exceeded`) reaches the harness that owns retry/backoff for 429/529 — that harness wraps the orchestrator process only. The dispatcher's sub-agents are spawned inside it with no such wrapper, so they would have inherited 0 and turned any transient overload into a shed lens on every scripted run. `dispatch-runner.ts` now passes the SDK's `env` option with `ANTHROPIC_MAX_RETRIES` back at the SDK default for the sub-agent subprocesses alone (spreading `process.env`, since that option replaces the environment rather than merging it). + + Also disables gh-aw's newly generated `agentics-maintenance.yml` (`.github/workflows/aw.json`: `{"maintenance": false}`) in this repo rather than adopting a daily scheduled cleanup workflow as a side effect of a version bump; adopt it deliberately if wanted. + +- 11213f7: Skill-doc only: fold four operational lessons from the 2026-07-22 sonnet-low trial into the `review-trial` skill. (1) Cost projections and cap sizing now use the enforcement unit, the firewall api-proxy's credit meter (the sum of `ai_credits_this_response` in the run's token-usage log), rather than the run-summary AI-credits figure, which reads ~2.5x lower; the measured basis is restated as 1,050-1,077 proxy credits for a full-depth, full-roster run of the memory-expiration shape. (2) Arm workflows must carry the 2500-credit trial cap (`max-ai-credits` plus the `REVIEW_MAX_AI_CREDITS` mirror), because the shared default of 1000 sits below the metered cost of exactly the runs this skill choreographs and a capped run dies mid-tail with the review emitted but the cache record and artifact upload unfinished. (3) Never label the arm PR with the repo's opt-out: the trial workflow ships the same `skip-ai-review` gate as the installed reviewer, so the label skips the trial arm itself; suppression moves to the scaffolding branch (remove the installed reviewer files and any pull_request-triggered slash-command shim). (4) The arm PR must present a realistic title and body, since the reviewer reads both via `pr-context.json`; the trial disclosure, the label, and the `[reviewer-trial]` prefix move to the scaffolding PR, whose branch the arm workflow's `if:` now excludes so it burns no credits. No change to the shipped review workflow. +- 034181f: review: the re-review fingerprint anchors on cache memory; the body stamp never survives gh-aw ingest + + gh-aw's safe-output sanitizer strips all XML/HTML comments (`removeXmlComments`), so the hidden fingerprint stamp a review body carries never reaches the PR: every production re-review planned `no-prior-fingerprint` and silently escalated to full depth, making the `re-review` ROUTING dial (scoped/flip-gated/fast) inert. The plan CLI now falls back to the Step 9 cache-memory record (`verdict`, `stampHunks`/`reviewedHunks`, `wasDraft`) when no prior-review body carries a stamp, and records which carrier anchored the plan as `stampSource` in `rereview-plan.json`. Step 9 gains a `stampHunks` field copied verbatim from the plan CLI's own hash computation so hash regimes are never mixed. Cache eviction still degrades to a full review, never a cheaper one. + ## 1.7.0 ### Minor Changes diff --git a/workflows/review/package.json b/workflows/review/package.json index 1ca5ad2c..dcc11878 100644 --- a/workflows/review/package.json +++ b/workflows/review/package.json @@ -1,6 +1,6 @@ { "name": "review", - "version": "1.7.0", + "version": "1.8.0", "dependencies": { "octokit": "5.0.5", "@anthropic-ai/claude-agent-sdk": "0.3.205", diff --git a/workflows/review/review.md b/workflows/review/review.md index 7cbd4806..9a1b4d59 100644 --- a/workflows/review/review.md +++ b/workflows/review/review.md @@ -227,7 +227,7 @@ pre-agent-steps: uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5 with: repository: Khan/actions - ref: review-v1.7.0 + ref: review-v1.8.0 path: gh-aw-review-lib persist-credentials: false