fix(ci): show a fork review body whose verdict stamp is missing by chenmingwei23 · Pull Request #8482 · kirodotdev/KiroCrew

chenmingwei23 · 2026-09-04T15:11:03Z

This PR changes the AI review gate that judges pull requests in this repo, including its own. It does not alter any check-run conclusion, does not touch /ai-review override semantics, and nothing that blocked before stops blocking -- the "Tests" section locks that property down explicitly.

It is not purely a reporting change, and that exception is the substance of the last review round. Surfacing a body that was previously log-only is itself the new exposure, so the GPT lane now also keeps the AWS credentials out of the environment its model's shell can read. That is a real change to how the reviewer runs, and it is described under "What changed".

Rebased onto main (the branch was 407 commits behind and conflicting). The conflict was in test/test_ai_review_workflows.py: 8b166f7d5 renamed TestForkGptVerdictVisibility to TestReviewLaneVerdictVisibility and rewrote it. This PR's test code is module-level and independent of that class, so it re-applies onto the rewrite untouched. One hunk was dropped as redundant: the bash -c to script-file harness swap is now how main's own helper already runs its step.

Problem / Motivation

Both fork reviewer lanes decide what to post from one test -- whether the captured output contains [<NAME>-REVIEWED] <head>:

if [ -s claude-review-output.md ] && grep -Fq "[OPUS-REVIEWED] $HEAD" claude-review-output.md; then

When a review ran to completion but that stamp was absent or SHA-corrupted, kind fell through to incomplete and the entire body was dropped. The lane posted one line:

No completed Opus verdict for this commit; see the Fork Opus 4.8 Review job logs.

The findings the model did produce survived only in the job log, which a fork contributor cannot open (403). Two very different situations rendered identically: a clean, complete review whose stamp got mangled, and a review that produced nothing at all.

This is not hypothetical. The Opus lane has been observed completing a review cleanly while corrupting the head SHA in its own marker (a hex-prefix duplication, and a 25-character truncation of the 40-character SHA), five consecutive attempts on one PR. In every one of those attempts the review text existed and was unreadable on the PR.

Why it matters

The reader cannot act, because the two cases need opposite responses. A mangled stamp on a clean review means re-run the lane. No output at all means investigate why the model produced nothing. Told only "no verdict", the contributor cannot tell which, and the person best placed to judge the review is the one denied it.

It also lands hardest on exactly the contributor with the fewest options: these are the fork lanes, so the reader is usually an external contributor who cannot open the job log and cannot re-run the workflow.

What changed (motivation -> approach -> change)

Symptom: a completed review reports "no verdict" and shows nothing.

Root cause: kind=incomplete is a single bucket holding two distinct states -- "no output was captured" and "output was captured but carries no stamp for this head" -- and the branch prints neither.

The change: in that branch, when the captured file is non-empty, print it in a collapsed <details> block labelled as not a verdict.

The one thing that makes this safe is the marker handling. pr_status.py reads reviewer markers straight out of comment text:

REVIEWED_STAMP_RE = re.compile(r"\[([A-Z][A-Z0-9_-]*)-REVIEWED\]\s+([0-9a-f]{7,40})\b")
BLOCK_MERGE_RE = re.compile(r"\[BLOCK-MERGE\]\s+([0-9a-f]{7,40})\b")

Printing a raw body would therefore publish a [GPT-REVIEWED] <head> freshness stamp for a review that never completed, and an unstamped review would start reading as freshly reviewed. That would reduce what blocks, so it is not acceptable.

Both patterns anchor on a literal [. So every marker is de-bracketed on the way out -- [GPT-REVIEWED] becomes (GPT-REVIEWED-UNSTAMPED) -- which makes the surfaced body inert to both parsers in exactly the way an absent body is.

The pattern matches the bracketed token alone and deliberately does not require a following SHA. Requiring one would leave open the very hole this branch exists to avoid: Python's \s+ spans newlines, but line-oriented sed cannot, so a body emitting [GPT-REVIEWED]\n<sha> would slip past a SHA-anchored pattern while pr_status.py still read it as a live stamp. That spelling lands in this branch precisely because the lane's own grep -Fq "[GPT-REVIEWED] $HEAD" is single-line too, so it fails to find the marker and routes there. Both parser patterns need the bracketed token contiguous on one line (their character classes exclude newline), so breaking the bracket cannot be bypassed by any whitespace arrangement. That is the invariant: before this change the branch posted no body and pr_status.py saw no markers; after it, pr_status.py still sees none. The downstream view is unchanged; only a human gains something.

This mirrors an existing precedent in the same file, which already de-fangs [BLOCK-MERGE] when adjudication downgrades a verdict.

The check-run is untouched. Finalize check-run (fail closed) re-derives its conclusion from the same missing stamp, independently of the comment step, and still resolves failure.

Keeping the credentials out of the model's shell

The GPT lane's reviewer blocked this PR for a residual security finding, and it was correct. Publishing the unstamped body is the new exposure -- nothing was posted in this branch before -- and codex exec hands the model a shell, on a fork's untrusted diff. So a prompt-injected reviewer can be told to print its environment, which holds the Bedrock credentials minted immediately before the call.

Redaction cannot close that on its own. This PR already redacts the credentials' shapes and their verbatim values, but a value printed in chunks, with delimiters, or re-encoded defeats both, and no pattern can catch every encoding. The fix therefore removes the value rather than trying to match it: the lane's config.toml now excludes the AWS credential variables from the environment the shell tool hands to subprocesses.

[shell_environment_policy]
ignore_default_excludes = false
filters = { "AWS_ACCESS_KEY_ID" = "exclude", "AWS_SECRET_ACCESS_KEY" = "exclude", "AWS_SESSION_TOKEN" = "exclude", "AWS_*" = "exclude" }

Two things about this are worth stating because both read backwards:

It does not cost the lane its model. The Bedrock provider resolves credentials in the codex process environment; shell_environment_policy governs only the env passed to subprocesses the shell tool spawns. A test asserts the provider config still resolves, so the exclusion cannot pass by simply breaking the reviewer.
ignore_default_excludes defaults to true, and true KEEPS variables whose names contain KEY, SECRET or TOKEN. Setting it false restores those built-in exclusions, so GH_TOKEN and any future secret-shaped variable drop out without anyone having to name them here.

A second review round showed that reasoning was wrong about the Opus lane, and the correction is worth stating plainly because it is the more interesting half.

I had argued the Opus lane needed no fence because it runs with --allowedTools "Read,Grep,Glob" and therefore has no shell. No shell is not no environment access. Read is path-unscoped and /proc/self/environ is an ordinary file, so an injected prompt can read the same Bedrock credentials through Read alone. The reviewer caught this; the "it has no Bash" comment in that workflow had been asserting a safety property it did not have.

Both Opus model steps now deny that path:

--allowedTools "Read,Grep,Glob"
--disallowedTools "Read(//proc/**),Read(//sys/**)"

A deny rule is the right shape rather than a narrowed allow list: deny outranks every allow rule and CLI flag and cannot be re-widened, Read denials also cover Grep and Glob, and an allow rule that fails to match falls back to prompting -- which guarantees nothing in a non-interactive run. It also costs the lane nothing it legitimately reads: the checkout and the pre-fetched patch are untouched.

Neither lane could solve this the other's way. The GPT lane can drop the credentials from the shell's environment because the codex process keeps its own copy for Bedrock auth. The Opus lane cannot: the environment being read is the action process's own, which must hold the credentials, so the only move is to block the path to it.

Scoped out deliberately: the pass-failure stub is NOT surfaced. If GPT's pass 1 fails, pass 2 still runs but against an empty discovery block, so its output reviewed nothing; publishing that as findings would misrepresent it, and the existing refusal to publish it is correct.

Findings on issue #8445 that a reviewer should know

I could not reproduce the issue's headline claim, and I am reporting that rather than building on it.

#8445 states that a blocking AUTOSDE rule "masks the entire AI review", so a policy-blocked fork PR "gets no usable code-review signal". On the PR cited as its evidence (#5890, head 07bdb0121), the GPT lane posted three blocking findings, not one:

#	Anchor rule	Site
F1	`no-new-builtin-apps`	`apps/builtins/project_scaffolder/app.json:2`
F2	`no-blocking-call-on-event-loop`	`dashboard/chat_folders.py:618`
F3	`errors-use-error-notice`	`ProjectScaffolderPage.tsx:382`

F2 and F3 are precisely the code-review signal the issue says is erased, delivered in the same comment as the policy block, each with file:line, mechanism and fix, plus an adjudication block upholding all three individually.

Reading the code agrees: no fork lane truncates or verdict-filters its findings during output assembly. The blocked branch prints the whole body. Every finding-count constraint ("at most 1 BLOCK per review", "Blockers only when the verdict is BLOCK") lives in the model prompt, not the shell -- so a single-finding body means the model emitted one finding, which is output never generated, not output truncated. The Opus lane's own job log confirms this directly for #5890: its discovery stage emitted two candidates, the second anchored to no rule and left unelevated by the validator.

So the real defect in this area is not truncation by a blocking rule. It is the unstamped branch fixed here -- output that was generated, then dropped. The two are easy to confuse downstream and need opposite fixes, which is why this PR addresses only the one it can demonstrate.

The issue's remaining asks are left alone on purpose, as maintainer decisions rather than bugs: splitting AUTOSDE enforcement into its own check-run changes review gating topology and what readiness reports for every PR; making fork lanes consume an override marker is a trust-boundary call on untrusted-fork code and is contradicted by a deliberate written position in pr-readiness.yml; and the override workflow's concurrency behaviour sits on the override surface.

Tests

Added TestForkLaneSurfacesAnUnstampedReviewBody (10 cases, both lanes), which executes the lanes' real bash step bodies with a stubbed gh rather than asserting on YAML text:

test_a_completed_review_with_no_stamp_is_surfaced_not_dropped -- an unstamped two-finding review now has both findings readable on the PR, and the lane still says it has no verdict.
test_the_surfaced_body_is_inert_to_the_real_marker_parsers -- imports the real _review_contract module and asserts REVIEWED_STAMP_RE and BLOCK_MERGE_RE both find zero matches in the posted body. Binding to the real regexes means a future drift in the parser fails this test instead of silently re-forging a stamp.
test_the_check_run_still_fails_closed_when_the_stamp_is_missing -- runs the finalize step and asserts conclusion=failure, never success. This is the "nothing blocks less" assertion.
test_a_marker_split_across_lines_is_still_neutralized -- pins the multiline case above. It first asserts the parser really does read [<NAME>-REVIEWED]\n<sha> as live, so the test cannot pass for the wrong reason, then asserts the posted body carries no readable marker.
test_a_stamped_review_keeps_its_markers_intact -- a properly stamped blocking review still publishes live markers, so the neutralization cannot leak out of the unstamped branch.

Result: 483 passed in test/test_ai_review_workflows.py. The parser module is loaded through the repo's load_skill_script helper so the import writes no bytecode into the checked-in scripts directory (verified: zero __pycache__ entries after a run that loads it). check_black_formatting, isort, flake8, check_changelog_history, check_testpaths_coverage and check_per_file_coverage all pass.

Added TestForkGptLaneKeepsCredentialsOutOfTheModelShell (3 cases) for the credential exclusion. It runs the real config step with HOME redirected and parses the config.toml it writes with tomllib, because a heredoc emitting invalid TOML would satisfy a substring assertion while codex discarded the policy entirely. One case asserts the Bedrock provider still resolves, so the exclusion cannot pass by breaking the lane's model. Mutation-verified: removing the [shell_environment_policy] block fails 2 of the 3.

One test-harness detail worth flagging, because it used to be a real trap: each lane assembles its comment under ${RUNNER_TEMP:-/tmp}/fork-*-comment.md, and the harness points RUNNER_TEMP at a per-test directory. That is what stops two parametrized cases from racing on one path -- which is how a stamped case's live markers first appeared in the unstamped case's assertions. The harness asserts the path stays RUNNER_TEMP-scoped, so a regression to a bare /tmp (shared between xdist workers, and the operator's own machine when the suite runs locally) fails the test instead of silently restoring the race.

Manual verification

Not possible before merge, and I want to be exact about why rather than present a green as evidence. These lanes are workflow_run-triggered, and as the file's own header states, workflow_run always runs the workflow definition from the default branch. This is also a same-repo PR, so it is reviewed by codex-review.yml and claude-review.yml; fork-gpt-review.yml and fork-opus-review.yml will not execute on it at all. No lane on this PR can exercise this change, and any green here says nothing about it. The behavioural tests above run the real step bodies precisely because that is the only pre-merge proof available.

Verified out of band instead: the neutralizing sed was run over a synthetic body carrying [BLOCK-MERGE], [GPT-REVIEWED], [OPUS-REVIEWED] and [DESIGN-REVIEWED] stamps, and the real parser regexes were run over the result -- two stamps and one blocking marker before, zero of each after. Both workflow files parse as YAML.

Also unrun, and stated rather than skipped silently: run_scoped_tests.py classifies .github/workflows/** as a broad-impact path and would run the full backend suite (62,108 tests). That run measures 21-32 GB on the host this was prepared on, which cannot host it safely alongside other work, so it was deliberately not run locally and is left to CI.

Pattern harvest

Rule candidate: a neutralizer or guard written as a single-line sed/grep must not be relied on to sanitize text for a parser whose whitespace class spans newlines. Match the smallest token the parser cannot split (here the bracketed marker) rather than the token plus its argument, and pin it with a test that loads the real parser. This PR contains a live instance in both directions: the lane's grep -Fq and the neutralizing sed are both single-line while pr_status.py uses \s+, and that mismatch is what decides whether an unstamped review can forge a freshness stamp.

Secondary observation, not proposed as a rule because it is already covered: AUTOSDE.yaml's feature-map-correctness rule expresses the same "the artifact must be true, not merely present" shape that this defect is an instance of -- a marker was present-looking and unusable.

Recording the pattern for the reviewer rather than the rule file, because its scope is one shell idiom: when a gate keys on a marker it did not itself construct, the failure path must still surface the content the marker was supposed to describe. Fail-closed on the marker is right; discarding the payload is a separate decision that reads as the same thing. The generalizable half is the neutralization technique -- when reporting content that a downstream parser also reads, break the parser's anchor (here a literal [) instead of trusting the content to be marker-free, and pin it with a test that imports the real parser so the two cannot drift.

Refs #8445

github-actions · 2026-09-04T15:19:19Z

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 28e2eceec7bb4eb36c6e36884eb29cdb0b100503 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All evidence gathered. Composing the review.

First-Principles-Verdict: CONCERNS

The GPT-lane credential fence rests on unverifiable codex config semantics, and the /proc + set_env fences skip three sibling fork lanes that always post their bodies.

What this change ships

Intent: let a fork contributor read a review that completed but lost its verdict stamp, without weakening the credential or freshness boundaries — a FIX (provenance: the SHA-corruption reproduction in the description, and added tests that fail on base).

Inventory (7 items)

A completed-but-unstamped fork review body is now posted collapsed, labelled "NOT a verdict", markers de-bracketed (both fork lanes) — justified
That body is withheld entirely if any credential shape survives redaction — justified
GPT lane's model shell no longer inherits the AWS credentials (codex shell_environment_policy) — justified
All three Opus model steps in the two edited lanes are denied Read of /proc and /sys — justified
A scrub truncates the runner's on-disk set_env_* credential copies before each of the 5 model calls — justified
Both lanes' redaction now erases the literal AWS credential values, not just shapes — justified
Seven pre-existing test comments rephrased and the comment-history baseline lowered by one — rides along

Watch

Item 3 rests on three vendored-tool claims nothing in-repo can verify: that shell_environment_policy never touches the provider's own credential resolution, that filters = { name = "exclude" } is a key this codex build recognizes at all (an ignored key makes the fence silently inert), and that ignore_default_excludes "defaults to TRUE and true KEEPS" secret-named vars. The added test only parses the TOML it wrote; it never runs codex — and under pull_request_target this PR's own review runs used the BASE workflow, so the new config has never executed. Wrong in one direction the lane loses its model on every fork PR; wrong in the other the fence does nothing.
Clears when: one live codex run in this lane shows the model resolving AND its shell tool seeing no AWS_* variables.
Items 4–5 are a point patch: I grepped configure-aws-credentials and claude-code-action across .github/workflows/ — fork-first-principles-review.yml, fork-ux-review.yml and fork-design-review.yml all mint the same Bedrock credentials, run the same action with unscoped Read on fork diffs, and post their bodies on every run (not just the unstamped branch), guarded only by the shape-based withhold grep this PR's own description calls defeatable by chunking or re-encoding. --disallowedTools "Read(//proc/**)" and the scrub appear in none of the three.
Clears when: the same two fences land in those three lanes, or a linked issue tracks them.

[FIRST-PRINCIPLES-REVIEWED] 28e2ece

github-actions · 2026-09-04T15:20:08Z

Design Review (Fable 5) — ✅ PASS

Design-level review of 28e2eceec7bb4eb36c6e36884eb29cdb0b100503 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

A real, observed failure fixed at the right layer, with the marker-forgery and credential-exfil consequences of the new exposure closed and pinned by behavioral tests.

Suggestions

The lane's completeness check (grep -Fq "[OPUS-REVIEWED] $HEAD") demands the full 40-char SHA while REVIEWED_STAMP_RE accepts {7,40}; accepting a unique prefix (≥7 hex) of $HEAD would turn the observed truncation case into a real verdict instead of a labeled non-verdict that still needs a maintainer re-run — worth a follow-up.
The "Scrub persisted credential files" step is copy-pasted verbatim five times across two workflows and is absent from the PR description; extract it to one shared script (repo precedent: .github/review-prompts/) and add a line to the description so the runner-internals assumption ("set_env files are already consumed by step start") is stated where a reviewer of either file will see it once, not maintained in five places.

[DESIGN-REVIEWED] 28e2ece

github-actions · 2026-09-04T15:20:38Z

Opus 4.8 Review — ✅ no blocking findings

Reviewed 28e2eceec7bb4eb36c6e36884eb29cdb0b100503 — this comment is updated in place on each push.

Review details

Both candidates fail the grounding bar. Candidate 1 depends entirely on claude-code-action's permission semantics — whether a Read(//proc/**) deny governs the separate Grep/Glob tools — which lives outside this repo; ux-review.yml shows a different design (narrowed Read allow + explicit Grep,Glob deny) but does not establish the deny-coverage semantics either way, and it's self-scored medium. I cannot re-derive an observable wrong outcome from code I opened, and downstream the verbatim-value redaction and shape-withhold gate further muddy reachability. Candidate 2 is self-admittedly unconstructible ("could not construct a path where that reliably happens") — each pass writes a fresh output file matched against its own live session env, so a prior session's bare value has no established route into the posted body. Neither reaches 80. No Step 2 finding grounds to the bar.

No findings.

[OPUS-REVIEWED] 28e2ece

Verdict parsed from the review's SHA-scoped output markers for commit 28e2eceec7bb4eb36c6e36884eb29cdb0b100503.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 28e2eceec7bb4eb36c6e36884eb29cdb0b100503: <one-sentence reason>

github-actions · 2026-09-04T15:21:00Z

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 28e2eceec7bb4eb36c6e36884eb29cdb0b100503 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 28e2ece

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 28e2eceec7bb4eb36c6e36884eb29cdb0b100503: <one-sentence reason>

chenmingwei23 · 2026-09-04T20:13:32Z

Disposition: First Principles CONCERNS -- "2 of 5 fork lanes", deferral not stated

Accepted, and the finding is correct: the same drop-a-completed-review behaviour does live in fork-first-principles-review.yml, fork-ux-review.yml and fork-design-review.yml. The deferral was real and undocumented. Stating it here rather than editing the description, because a body edit re-triggers the GPT lane, which is currently clean on this head, and this concern is advisory.

Why these two lanes and not five.

GPT 5.6 and Opus 4.8 are the required gates -- their failure blocks merge. For the other three, only a BLOCK fails the check and PASS/CONCERNS are advisory. So the harm being fixed (a fork contributor cannot read a completed review that is gating their merge, and cannot open the job log -- 403) bites hardest exactly where this PR fixes it.
The three advisory lanes deliberately do something different: when no verdict header parses they substitute a generic notice instead of the captured text. fork-design-review.yml states the reason in its own comment -- "raw model/error text can carry internal detail". Extending this change there overrides a deliberate leak-safety decision in three lanes at once, which is a different proposal from fixing an oversight in two.
There is a concrete prerequisite, not a preference. Of the three, only fork-first-principles-review.yml carries the credential-shape withhold gate; fork-ux-review.yml and fork-design-review.yml have redaction but no withhold. Surfacing their captured bodies without first adding that gate would reintroduce precisely the exposure the GPT lane blocked this PR for one round ago (a malicious fork can make the reviewer dump its credential-bearing environment and omit the stamp, which routes into the unstamped branch by construction). So the ordering is: add the withhold gate to UX and Design, then extend the surfacing.

Follow-up worth doing instead of, or before, widening this. The Design lane's suggestion attacks the root cause rather than the symptom: have the lane's stamp check accept a sufficiently long SHA prefix, so a clean review whose stamp got truncated (observed as a 25-char prefix of the 40-char head) counts as a verdict at all. That would remove most traffic from the unstamped branch in every lane at once, rather than improving what the unstamped branch prints in each. This PR does not attempt it: it changes what a verdict is, which is gate semantics rather than reporting, and that is a maintainer call.

Filing the three-lane extension and the prefix-tolerant stamp as follow-ups rather than growing this diff, which has already converged twice.

chenmingwei23 · 2026-09-08T02:29:20Z

Disposition 1 of 2: unfenced `Read` in sibling model steps

The in-file sibling is FIXED; the three advisory lanes are out of scope for this PR. fork-gpt-review.yml's own Opus adjudication step is a claude-code-action step too, and the body it produces is posted by the same comment step this PR modifies -- so leaving it unfenced kept the exposure reachable through the very publication path this PR opens, in a file the diff already edits. It now carries the same --disallowedTools "Read(//proc/**),Read(//sys/**)" as the two fork Opus review steps.

The test blind spot named alongside it is closed. TestForkOpusLaneDeniesReadingTheEnvironment did enumerate only fork-opus-review.yml, so the in-file sibling was invisible to it -- a fair hit. It is now TestForkModelStepsDenyReadingTheEnvironment and enumerates every claude-code-action step across both edited workflows. Mutation-verified: unfencing only the adjudication step fails the test and names that step, which is exactly the case the old shape missed.

fork-first-principles-review.yml:452, fork-ux-review.yml:341 and fork-design-review.yml:287 do have unfenced Read, and that is a real pre-existing gap on main. It is not reachable through this change: those lanes never surface a captured unstamped body -- they substitute a generic notice -- so no publication path this PR opens carries their output. Fencing them is worth doing and I am not arguing otherwise; it is simply not this PR's exposure, and folding it in would grow a diff that has converged three times without reducing the risk this PR introduces. Same reasoning for the bare-value redaction gap in those lanes' shape-only perl (fork-ux-review.yml:962, fork-first-principles-review.yml:566).

chenmingwei23 · 2026-09-08T02:32:24Z

Disposition 2 of 2: the unstamped-body cause is fixed in 2 of 5 fork lanes

Accepted and deferred, with an ordering constraint that makes it a separate change rather than a wider version of this one. Of the three advisory lanes, only fork-first-principles-review.yml carries the credential-shape withhold gate; fork-ux-review.yml and fork-design-review.yml have redaction but no withhold. Surfacing their captured bodies without adding that gate first would reintroduce exactly the exposure that blocked this PR twice -- a malicious fork can make a reviewer dump its credential-bearing environment and omit the stamp, which routes into the unstamped branch by construction. So the ordering is: add the withhold gate to UX and Design, then extend the surfacing.

Two further reasons the split is at two lanes and not five. GPT 5.6 and Opus 4.8 are the required gates, so the harm being fixed -- a fork contributor cannot read a completed review that is gating their merge, and gets a 403 on the job log -- bites hardest exactly where this PR fixes it. And the three advisory lanes deliberately do something different: when no verdict header parses they substitute a generic notice instead of the captured text, and fork-design-review.yml states the reason in its own comment ("raw model/error text can carry internal detail"). Extending this change there overrides a deliberate leak-safety decision in three lanes at once.

The review is also right that the description had claimed more than the diff delivered. Both the PR body and the commit message now say which lanes are fixed and which are not, and the body's earlier "REPORTING change only" framing is gone, because the credential fences are a real behaviour change to how the reviewers run.

The better root fix, which I am deliberately not attempting here: make a lane's stamp check accept a sufficiently long SHA prefix, so a truncated-but-clean stamp stops routing into the unstamped branch at all, in every lane at once. That changes what counts as a verdict -- gate semantics, not reporting -- so it is a maintainer call and belongs in its own PR.

Both fork reviewer lanes decide `kind` from the presence of `[<NAME>-REVIEWED] <head>` in the captured output. When a review ran to completion but that stamp was absent or SHA-corrupted, `kind` fell to `incomplete` and the whole body was dropped to the job logs, leaving a one-line "no verdict" notice. A reader could not distinguish a clean review carrying a mangled stamp from a review that produced nothing, and those need opposite responses. A fork contributor cannot open the job log, so the findings gating their merge were unreachable. Print the captured body in that branch, behind four guards. CUT THE MODEL OFF FROM THE CREDENTIALS. Publishing the body is the new exposure -- nothing was posted in this branch before -- and both lanes run agentically on a fork's UNTRUSTED diff with Bedrock credentials in the job environment. Redaction cannot close that: it matches a credential's shapes and its verbatim value, and a value that is chunked, delimited or re-encoded defeats both. Pattern-matching the output is the wrong layer, so each lane instead loses its route to the value. The two lanes need different fences because they expose different things. The GPT lane runs `codex exec`, which hands the model a SHELL, so its `config.toml` now excludes the AWS credential variables from the environment the shell tool passes to subprocesses. That does not cost the lane its model: the Bedrock provider resolves credentials in the codex PROCESS environment, which the policy does not touch. `ignore_default_excludes` is set false as well -- it defaults to TRUE, which KEEPS names containing KEY, SECRET or TOKEN -- so GH_TOKEN and future secret-shaped variables drop out without being named. Having no shell is NOT sufficient, which is where the first attempt at this was wrong. `Read` is path-unscoped and `/proc/self/environ` is an ordinary file, so an injected prompt reads the same credentials through `Read` alone. Every `claude-code-action` model step in both edited lanes now denies that path -- the fork Opus lane's two review steps AND the GPT lane's own Opus ADJUDICATION step, which is easy to miss because it sits in the file the shell fix above already edits, and the body it produces is posted by the SAME comment step that surfaces an unstamped review. A deny rule is the right shape rather than a narrowed allow list: deny outranks every allow rule and CLI flag and cannot be re-widened, `Read` denials also cover `Grep` and `Glob`, and an allow rule that fails to match falls back to prompting, which guarantees nothing in a non-interactive run. It costs the lanes nothing they legitimately read. Both of those fences guard the ENVIRONMENT, and that is not the whole surface. `configure-aws-credentials` exports through `core.exportVariable`, which WRITES each value into `$RUNNER_TEMP/_runner_file_commands/set_env_*`. That is an ordinary FILE, under a directory these lanes legitimately read, so the shell-env exclusion does not cover it and neither does the `/proc` deny: a model told to open it can emit the value in chunks no shape rule matches. Every one of the five model invocations across the two lanes is therefore preceded by a step that truncates those files. The runner has already consumed them by then, so no variable changes -- what goes away is the only on-disk copy. A missing directory is a pass, so the scrub cannot fail a lane before its review starts. REDACT CREDENTIAL VALUES, not only their shapes. A bare AWS secret access key is 40 chars of [A-Za-z0-9/+=] with no distinctive prefix, so it survives every existing rule: it is not AKIA/ASIA (that is the key ID), it carries no `name=` for the named-pair rule, and it is far short of the 200+ char base64 run the withhold filter looks for. The lanes now also redact the literal values of the credentials they inherit. A length guard skips the unset case, because \Q\E on an empty string matches at every position. This sits in the existing redaction step, so it protects the blocked and clear paths too, not just the branch added here. WITHHOLD what still looks credential-shaped. Any surviving shape withholds the whole body. Same pattern as the fork first-principles and UX lanes. DE-BRACKET EVERY MARKER. REVIEWED_STAMP_RE and BLOCK_MERGE_RE both anchor on a literal "[", so the surfaced body is inert to pr_status.py exactly as an absent body is. The pattern matches the bracketed TOKEN ALONE: Python's `\s+` spans newlines while line-oriented sed cannot, so `[GPT-REVIEWED]\n<sha>` would slip past a sha-anchored pattern while still reading as live. The check-run conclusion is re-derived independently and still fails closed on the missing stamp. Reporting only; nothing that blocked before stops blocking. The surfaced body carries no fresh stamp, so the shared `guarded_comment_upsert` can only CREATE with it and never PATCHes over an existing verdict. The added harnesses deliver each step as a FILE rather than a `bash -c` argument, matching the idiom this file already uses: Git-bash on Windows truncates that argument near 8 KiB and these steps exceed it, so the tail would be silently cut and an `if` would never reach its `fi`. The GPT config test PARSES the emitted TOML rather than grepping it, because a heredoc emitting invalid TOML would satisfy a substring assertion while codex discarded the policy. The Opus fence test ENUMERATES the lane's model steps rather than grepping the file, so a new model step added without the fence fails instead of hiding behind a sibling. Two changes here are housekeeping the gates asked for, called out so they are not mistaken for scope creep. Seven comments and docstrings in `test_ai_review_workflows.py` are restated in present tense: they narrated change history, which `docs/system-specs/common/code-style.md` puts in git rather than in a comment. None of them are lines this change introduced -- the added tests carry no such narration -- and the file's `comment-history-baseline.json` entry is RATCHETED DOWN from 49 to 48 to match, never raised, which is the only direction that file permits. Refs #8445

chenmingwei23 requested a review from a team as a code owner September 4, 2026 15:11

chenmingwei23 requested a review from Zedmor September 4, 2026 15:11

github-actions Bot added the readiness: action required A blocking check or review needs attention label Sep 4, 2026

chenmingwei23 force-pushed the fix/fork-review-surface-unstamped-body-8445 branch from 621a709 to 82f0c3f Compare September 4, 2026 15:37

chenmingwei23 force-pushed the fix/fork-review-surface-unstamped-body-8445 branch from 82f0c3f to ccbd41d Compare September 4, 2026 18:27

chenmingwei23 mentioned this pull request Sep 4, 2026

Backend Tests (Windows) (4) fails on six unrelated PRs in one window, passes on a seventh #8517

Closed

chenmingwei23 force-pushed the fix/fork-review-surface-unstamped-body-8445 branch from ccbd41d to 8320589 Compare September 4, 2026 19:37

github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 4, 2026

github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 4, 2026

github-actions Bot added readiness: checking Automated validation is still running merge conflict Branch has merge conflicts with its base — author must resolve before merge labels Sep 8, 2026

chenmingwei23 force-pushed the fix/fork-review-surface-unstamped-body-8445 branch from 0c75b29 to a6f574d Compare September 8, 2026 02:24

github-actions Bot removed the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Sep 8, 2026

chenmingwei23 force-pushed the fix/fork-review-surface-unstamped-body-8445 branch from a6f574d to d2d4e46 Compare September 8, 2026 02:28

github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 8, 2026

github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 8, 2026

chenmingwei23 force-pushed the fix/fork-review-surface-unstamped-body-8445 branch from d2d4e46 to d3b50ed Compare September 8, 2026 03:02

github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 8, 2026

chenmingwei23 force-pushed the fix/fork-review-surface-unstamped-body-8445 branch from d3b50ed to 0db87fd Compare September 8, 2026 03:33

chenmingwei23 mentioned this pull request Sep 8, 2026

test(security-conductor): stale staging guard fails now that the ledger script ships #9363

Closed

chenmingwei23 force-pushed the fix/fork-review-surface-unstamped-body-8445 branch from 0db87fd to 28e2ece Compare September 8, 2026 04:32

bolichen97 approved these changes Sep 8, 2026

View reviewed changes

bolichen97 merged commit f76a5b0 into main Sep 8, 2026
65 checks passed

bolichen97 deleted the fix/fork-review-surface-unstamped-body-8445 branch September 8, 2026 06:39

github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Sep 8, 2026

Navigation Menu

Uh oh!

fix(ci): show a fork review body whose verdict stamp is missing - #8482

fix(ci): show a fork review body whose verdict stamp is missing#8482
bolichen97 merged 1 commit into
mainfrom
fix/fork-review-surface-unstamped-body-8445

chenmingwei23 commented Sep 4, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 4, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 4, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 4, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 4, 2026 •

edited

Loading

Uh oh!

chenmingwei23 commented Sep 4, 2026

Uh oh!

chenmingwei23 commented Sep 8, 2026 •

edited

Loading

Uh oh!

chenmingwei23 commented Sep 8, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

chenmingwei23 commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem / Motivation

Why it matters

What changed (motivation -> approach -> change)

Keeping the credentials out of the model's shell

Findings on issue #8445 that a reviewer should know

Tests

Manual verification

Pattern harvest

Uh oh!

github-actions Bot commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

First Principles Review (Fable 5) — 🟡 CONCERNS

What this change ships

Watch

Uh oh!

github-actions Bot commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Design Review (Fable 5) — ✅ PASS

Suggestions

Uh oh!

github-actions Bot commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Opus 4.8 Review — ✅ no blocking findings

Uh oh!

github-actions Bot commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

GPT 5.6 Review — ✅ no blocking findings

Uh oh!

chenmingwei23 commented Sep 4, 2026

Disposition: First Principles CONCERNS -- "2 of 5 fork lanes", deferral not stated

Uh oh!

chenmingwei23 commented Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Disposition 1 of 2: unfenced Read in sibling model steps

Uh oh!

chenmingwei23 commented Sep 8, 2026

Disposition 2 of 2: the unstamped-body cause is fixed in 2 of 5 fork lanes

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

chenmingwei23 commented Sep 4, 2026 •

edited

Loading

github-actions Bot commented Sep 4, 2026 •

edited

Loading

github-actions Bot commented Sep 4, 2026 •

edited

Loading

github-actions Bot commented Sep 4, 2026 •

edited

Loading

github-actions Bot commented Sep 4, 2026 •

edited

Loading

chenmingwei23 commented Sep 8, 2026 •

edited

Loading

Disposition 1 of 2: unfenced `Read` in sibling model steps