fix(ci): relocate Bedrock credentials out of the reviewer's env

[chenmingwei23]

Problem / Motivation

The fork GPT reviewer holds Bedrock credentials in the process that gives an
untrusted-prompt-driven model a shell, and then publishes that model's output to
a public PR comment.

This is demonstrated, not inferred. Earlier revisions argued it from the YAML;
the job log says it outright. From run 33970715459:

OpenAI Codex v0.150.1
...
sandbox: read-only

and the model step's own env: block, in the same log:

AWS_DEFAULT_REGION: us-east-1
AWS_REGION: us-east-1
AWS_ACCESS_KEY_ID: ***
AWS_SECRET_ACCESS_KEY: ***
AWS_SESSION_TOKEN: ***

A fork PR author controls the diff the reviewer reads, so they control the prompt.
The read-only sandbox restricts writes; it does not restrict reads.

Why it matters

The lane already redacts credential shapes and literal values before publishing,
and those filters are correct as far as they go, but an attacker who controls the
prompt controls the output encoding -- base64, reversed, interleaved -- and no
shape or literal matcher sees it. The value has to be absent, not filtered.

What this PR claims, stated before the diff

This PR does not close #8671, and it is not hardening. It relocates the
credential from the process environment to a profile file, and deletes a config
block that claimed an authority it did not have.

On an ephemeral runner both the environment and the file die with the runner, and
both are reachable by any same-UID sibling the model controls. So this is a change
of shape, not a reduction in exposure. Its value is as a prerequisite: the
credential has to leave codex's environment before it can be moved behind a
privilege boundary, which is the only thing that closes this. That work is
#8789.

If you are reviewing this for security value, review it as plumbing.

Three attempts, three defeats, one unmoved invariant

The invariant: a secret reachable by a process is reachable by any same-UID
sibling the model controls.

Attempt	What it did	How it was defeated
1. Environment filter	shell_environment_policy withheld the credentials from the model's shell	Same UID, so /proc/<ppid>/environ returns what the parent holds
2. Environment removal	Blank the credential variables in both codex steps' env:; authenticate from a profile file	The file is readable by the same UID
3. File deny	default_permissions + a [permissions.*] deny on that file	The read-only sandbox flag on the command line selects the legacy sandbox instead of a named profile, so the deny is inert

Attempts 1 and 3 were each caught by this lane's own GPT reviewer, on the PR that
introduced them. This PR keeps attempt 2 and deletes attempt 3.

What changed

One workflow file. configure-aws-credentials is untouched -- all three steps
and their least-privilege roles are byte-identical, asserted by test. How the job
authenticates does not change; what the model step holds does.

• Both codex steps blank AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY and
  AWS_SESSION_TOKEN in their own env: blocks. Per-step, so later steps that
  legitimately need them -- the Opus adjudication -- still see them.
• Authentication moves to a profile file, staged once per pass because each
  pass re-assumes the role and the prior session has expired. Staged under
  $RUNNER_TEMP and selected with AWS_SHARED_CREDENTIALS_FILE rather than
  written to the SDK's global default path: reachability is identical -- same UID
  either way -- but writing the default would clobber an existing credentials file
  on a non-ephemeral or self-hosted runner. Raised as an advisory by this repo's
  Design Review lane.
• The default_permissions / [permissions.review-readonly] block is deleted.
  A comment in its place records why, so nobody re-adds it.
• shell_environment_policy is kept as defence in depth.

Why deleting the deny, rather than deleting the flag

The reviewer's prescribed fix was to remove the sandbox flag so the config would
govern. This does the inverse, deliberately: the flag governs today, so deleting
the config changes the effective sandbox by exactly nothing and requires no
fact about the pinned CLI version. Removing the flag instead would hand control to
whatever 0.150.1 falls back to when it is absent -- which I cannot verify, and
whose failure direction is more permissive than read-only.

The reviewer keeps its shell, and that was measured

Removing the shell was considered and refuted by measurement. One real review
makes 53 bash -lc calls across 16 paths: 19 paging this repo's own review
rulebooks in chunks, 14 cross-referencing changed symbols against surrounding
code, 10 reading CLAUDE.md and the convention docs, 6 running git apply on the
patch, 3 reading the patch itself. The patch reaches the model through the shell.

The lane's own prompt settles it: "A finding is grounded in what the code DOES
when executed, never in what text in the diff says about it." A shell-less
reviewer could ground findings only in diff text, which its instructions forbid.
The claude-code-action lanes need no shell because they have Read, Grep and
Glob natively; codex has no native file tools here.

The four publishing paths

Path	Line on c791f0f1d	Status
GPT blocked	fork-gpt-review.yml:973, body at :974	still exposed -- see #8789
GPT clear	fork-gpt-review.yml:975	still exposed -- see #8789
Opus blocked	fork-opus-review.yml:465, body at :466	not reachable by this mechanism
Opus clear	fork-opus-review.yml:467, body at :471	not reachable by this mechanism

Refs, not Closes. The Opus paths are out of reach only because those three
claude-code-action calls grant no shell -- a premise this PR asserts in a test
rather than assumes, so granting Bash to any of them fails CI.

Tests

test/test_ai_review_workflows.py::TestModelShellDoesNotInheritCredentials,
13 cases. The load-bearing one is provable by reading this repository: the
credential variables are blank in both model steps, a profile is named, the region
survives. Others assert the provider resolves from a profile and the name matches
both steps; the credentials are re-staged once per pass; all three
credential-minting steps are unchanged with roles intact; the config write precedes
both exec boundaries; and no claude-code-action call grants a shell.

One test is the inverse of a mistake this PR made: it fails if anyone re-adds a
permissions profile while the sandbox flag is still passed, because that
combination produces a deny that parses fine and enforces nothing. An earlier
revision also had default_permissions land inside a table, silently becoming
model_providers.amazon-bedrock.aws.default_permissions -- same failure shape,
different cause.

Not vacuous, measured: re-adding the inert default_permissions makes that guard
fail with config.toml selects a permissions profile while --sandbox is passed on the command line, which overrides it -- the deny is inert. Whole touched file
passes 360.

isort and flake8 clean on the changed file. black --check reports the test
file unformatted both at origin/main and here (exit 1 either way), so it is
in the gate's shrinking baseline, not introduced by this diff.

Manual verification

Verified by consequence. model_providers.amazon-bedrock.aws.profile is
honoured by 0.150.1: a verdict landed with the credential variables blanked and
zero auth-failure lines in the log, leaving the profile file as the only
possible credential source.

Verified from primary source, at the pinned tag. An earlier revision of this
body listed shell_environment_policy.filters and
ignore_default_excludes as unverifiable because npm install on my host fails
E401 against an authenticated registry. That conflated cannot install the
package with cannot verify, and the second does not follow: the CLI is open
source. codex-rs/config/src/shell_environment_policy.rs at tag
rust-v0.150.1 defines ShellEnvironmentPolicyToml with inherit,
ignore_default_excludes, exclude, set, include_only, and filters as
BTreeMap<String, ShellEnvironmentPolicyFilter> -- a map, exactly the form
used here -- alongside validate_shell_environment_policy_filter_config, so the
field is actively validated rather than vestigial. Every key this PR writes
exists on the pinned version.

That also refutes a concern worth recording rather than quietly dropping:
Opus 4.8 reasoned that on 0.150.1 the exclusion field might be exclude (a glob
array) rather than the filters map, which would have left two mechanisms inert.
It was a reasonable inference and it is wrong. Two practical notes for anyone
repeating the check: the tag is rust-v0.150.1, not v0.150.1, and
unauthenticated code search returns 401 while an authenticated
gh api search/code works.

Proven inert, and therefore deleted: the permissions.* deny-read profile.
The read-only sandbox flag on the command line selects the legacy sandbox instead
of a named profile, so the deny never applied.

Acceptance evidence from the lanes on this PR, which are the change under test.
Real verdicts landed bound to the head, and the shell still works: 83 bash -lc
calls against a 53-call baseline on the revision that introduced the profile
file, still reading the rulebooks, CLAUDE.md, src/, website/src and docs/,
with zero sandbox denials. authentic.patch reads dropped to zero, which is
what a denial looks like -- but git calls rose from 6 to 29, so it reads the diff
through git now. A count falling to zero was checked, not assumed. The 53-call
baseline came from a run reviewing a different PR, so cross-revision counts are
not strictly comparable; the denial check is the load-bearing evidence.

Pattern harvest

Rule candidate: when three sound fixes are each routed around, stop patching and
name the invariant they all sit below. Here it is same-UID reachability --
environment, procfs and filesystem are three doors to one secret, and closing them
one at a time is not progress.

Rule candidate: a config that asserts an authority it does not have is worse than
no config, because it reads to a human as a closed hole. Prefer deleting the false
claim over trusting an unverified fallback that would make it true.

Not generalizable: the sandbox-flag-versus-permissions-profile precedence, and
Codex's default of keeping KEY/SECRET/TOKEN variables rather than dropping them.
Both are properties of this CLI.

Refs #8671
Refs #8789

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

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

Design-Verdict: CONCERNS

Sound staged plumbing, honestly scoped to #8789; but the shipped comment contradicts the PR body on whether the defence-in-depth layer is verified.

Watch

• Stale comment vs. body: the config comment says filters/ignore_default_excludes "could not be verified against 0.150.1... may be exclude... Do not copy filters", while the body says both were verified from source at rust-v0.150.1 and the exclude concern refuted. The file is the durable record, so a future maintainer will treat a verified layer as possibly inert — update the comment before merge.
• Value is contingent: with the GPT publishing paths still exposed (body's own table) and the title reading fix(ci), this ships extra moving parts (two staging steps, a profile file with no cleanup) whose only payoff is fork AI reviewers: move Bedrock credentials behind a UID boundary #8789 landing. If that stalls, revert this rather than leave the extra credential copy on disk.

[DESIGN-REVIEWED] ca7ef90

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of ca7ef9081a4b70d4594249f0fe27fea11eb9404e — 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.

I have everything I need: the contract, the intent, the patch, the resulting workflow file, the sibling trusted-PR lane, and the test helpers. The evidence is consistent — the diff is purely additive against base (the "deleted" default_permissions block never existed on main), and codex-review.yml carries the same credentials-in-env shape unfixed. Here is the review.

First-Principles-Verdict: CONCERNS

The core plumbing is honest and earns its place; what rides along is a possibly-inert env-filter block pinned by four tests, plus a history-narrating tombstone.

What this change ships

Intent: keep Bedrock credentials out of the process an attacker-prompted model drives, as staging for the real UID-boundary fix (#8789). A FIX by type, declared plumbing by the author.

1. Both GPT model steps run without the credential variables — justified; level honestly declared.
2. A credentials file is written to disk before each pass — justified; item 1 needs auth to survive.
3. CLI config gains a profile/region provider block — justified.
4. CLI config gains shell_environment_policy env filtering — rides along; author-admitted unverified keys.
5. A 14-line comment documents a deny that never existed on main — oversized.
6. CI fails if a permissions profile is re-added while --sandbox is passed — justified ratchet.
7. CI fails if any claude-code-action model call gains a shell — justified; pins the premise the fix stands on.
8. CI pins the three credential-minting steps and their roles — justified ratchet.

Watch

• Item 4 is an ADDITION relative to main (the diff has zero removed lines; "kept" is true only against earlier PR revisions), and its own comment says filters / ignore_default_excludes "could not be verified against 0.150.1... may be inert" — yet 4 new tests (test_the_policy_excludes_every_credential_variable etc.) pin those exact keys. It guards a named boundary, so it may stay, but if the keys are inert the tests freeze an incantation, not a behavior, and item 1 already empties every variable they would filter.
• Counted sibling: codex-review.yml runs the same codex exec --sandbox read-only (2 calls, codex-review.yml:516, :606) with Bedrock credentials in the job env and no blanking — grepped codex exec|AWS_ across .github/workflows. Defensible scope cut (same-repo authors already sit inside the secrets boundary), but it is the unfixed instance of the identical mechanism.

Subtractions

• Shrink the tombstone at fork-gpt-review.yml:381-394 to the one invariant sentence (the command-line sandbox flag overrides named permission profiles): "it was written, it was inert, and this lane's own GPT reviewer caught it" is the review-round narration AGENTS.md's comment rule forbids, and test_the_config_claims_no_authority_the_sandbox_flag_overrides already ratchets the mistake.
• Drop the three explicit AWS_SESSION_TOKEN / AWS_SECRET_ACCESS_KEY / AWS_ACCESS_KEY_ID entries in filters (fork-gpt-review.yml:420) — the AWS_* glob beside them already covers all three, and the new test accepts glob coverage.

[FIRST-PRINCIPLES-REVIEWED] ca7ef90

[github-actions]

GPT 5.6 Review — 🔴 changes requested (blocking)

GPT 5.6 found at least one blocking issue that must be resolved before merging ca7ef9081a4b70d4594249f0fe27fea11eb9404e. 1 of 1 blocking finding(s) are security-class and were withheld from adjudication, so the blocking verdict stands.

This comment is updated in place on each push.

BLOCKING -- .github/workflows/fork-gpt-review.yml:453 -- Credential file remains reachable by the untrusted reviewer
printf 'aws_access_key_id = %s\n' "$AWS_ACCESS_KEY_ID"
Malicious fork diff -> Codex shell reads the parent’s profile path and file -> encoded credentials bypass redaction and reach the public comment.
Anchor: residual/security
Fix: Place credentials behind a privilege boundary inaccessible to model-spawned commands.
[BLOCK-MERGE] ca7ef90
[GPT-REVIEWED] ca7ef90

Adjudication (Opus 4.8) — is blocking on each finding proportionate?

The adjudicable block is empty; ruling only on the one fenced finding F1.

F1 — unbounded (credential exposure). Confirmed conditions from code opened this run: the minted Bedrock credentials are written to $RUNNER_TEMP/review-aws/credentials (.github/workflows/fork-gpt-review.yml:453-458); the codex model step authenticates against that same file via AWS_SHARED_CREDENTIALS_FILE (line 477) while running --sandbox read-only (line 699) — a read-only sandbox still permits reads, so the file the CLI must read to authenticate is reachable by any shell the model drives. Blanking the env vars (lines 473-475) relocates but does not remove reach, which the PR's own comments state at lines 400-403 and 469-472. The only barriers to public exposure are prompt-injection defenses (not absolute against an untrusted fork diff) and shape-based redaction (lines 823-828), which the finding's base64-encoding vector defeats. This is precisely the fork threat model this lane exists for, and the author concedes the exposure is not reduced — I cannot complete the rarity record FLAG requires, so the residual risk is not one a human should be pre-drafted to accept.

[ADJUDICATION] ca7ef90 total=0 uphold=0 downgrade=0
[GPT-ADJUDICATED] ca7ef90
[ADJUDICATION-FENCED] ca7ef90 fenced=1 flagged=0
UPHOLD-FENCED F1 .github/workflows/fork-gpt-review.yml:453 -- The credential file is read-reachable by the model's own sandbox and the PR concedes exposure is unchanged, so the injection-plus-encoding path is plausible, not extreme.
[GPT-ADJUDICATED-FENCED] ca7ef90

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

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

I've validated the discovery pass's conclusion against the diff and the changed files directly.

The change is a defensive CI hardening: it relocates minted Bedrock credentials into a chmod 600 profile file, blanks the credential env vars in the two step-scoped codex exec steps (later Opus steps keep them), points the CLI at the profile, and adds a defence-in-depth [shell_environment_policy] block plus read-only tests.

Checking for a groundable defect on the changed lines:

• Step-level env: AWS_ACCESS_KEY_ID: "" overrides the $GITHUB_ENV-exported values, so the codex process env is blank — the /proc/<ppid>/environ bypass property holds. Later credential-needing steps are unaffected (step-scoped). No observable wrong outcome.
• Staging runs before each codex exec, re-stages after each re-mint, uses umask 077 + chmod 600. set -uo pipefail (no -e) can only yield an auth failure, never a leak. The dir-created-before-umask detail is a same-UID lateral matter, explicitly scoped as "relocates, does not reduce" — no named trigger past what's disclosed.
• Provider config sets profile = "review", matching the file's [review] section and AWS_PROFILE/AWS_SHARED_CREDENTIALS_FILE; runner.temp and $RUNNER_TEMP are the same path. Consistent, no crash/auth-loss on the changed path.
• filters/ignore_default_excludes uncertainty is self-flagged and only defence-in-depth behind the primary env-blanking.
• No hardcoded model id, no harness-parity negative test, no blocking: true rule weakened, no changelog touch.

Nothing survives falsification, and I could ground no new Step-2 finding at 80+.

No findings.

[OPUS-REVIEWED] ca7ef90

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

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

[github-actions]

UX Review (Fable 5) — ⏭️ skipped

Revision ca7ef9081a4b70d4594249f0fe27fea11eb9404e touches no user-facing surface (no changes under website/ or committed screenshots), so the UX review was skipped. Advisory — does not block merge.

[chenmingwei23]

Closing as folded into #8789, not abandoned

The diff is preserved on branch fix/scope-model-shell-env-8671 (head
ca7ef9081, 2 commits, +313/-0 against 45a706542) and will be carried into the
privilege-boundary work in #8789 rather than reworked from scratch.

Why fold rather than land. This PR's own body claims relocation and
prerequisite, never hardening. GPT 5.6 blocks it for precisely that -- "credential
file remains reachable by the untrusted reviewer." The body and the blocking
verdict agree, which means the PR is not being obstructed, it is being read
correctly. The same diff is unreviewable as a standalone relocation and reviewable
as part of a boundary: the proxy in #8789 needs the credential out of the codex
process's environment before it can hold it under another UID. Alone, the change
is a change of shape a reviewer must take on faith.

The invariant, and three sound fixes defeated by it

A secret reachable by a process is reachable by any same-UID sibling the model
controls.

Attempt	What it did	How it was defeated
1. Environment filter	shell_environment_policy withheld the credentials from the model's shell	Same UID, so /proc/<ppid>/environ returns what the parent holds
2. Environment removal	Blanked the credential variables in both codex steps' env:; authentication via a profile file	The file is readable by the same UID
3. File deny	default_permissions + a [permissions.*] deny on that file	The read-only sandbox flag on the command line selects the legacy sandbox instead of a named profile, so the deny is inert

Attempts 1 and 3 were each caught by this repository's own fork GPT reviewer, on
the PR that introduced them. Two of the three read to a human as a closed hole
while the credential stayed reachable. A fourth patch of the same class is
expected to fail the same way; the class is the problem, not the individual fix.

What was measured, and is worth keeping

The relocation neither broke authentication nor narrowed the reviewer. On the
final head: 48 bash -lc calls, zero permission-denied lines, zero
auth-failure lines, still reading .review-base-rules (7), CLAUDE.md (2),
src/kiro_crew (1) and docs/ (3). A verdict landed with the credential
variables blanked in the step env, leaving the profile file as the only possible
credential source -- which is how model_providers.amazon-bedrock.aws.profile was
confirmed to work on the pinned CLI, by consequence rather than citation.

The reviewer needs its shell, and that was measured rather than assumed. One
real review makes 53 bash -lc calls across 16 paths: 19 paging this repo's own
review rulebooks in chunks, 14 cross-referencing changed symbols against
surrounding code, 10 reading the conventions docs, 6 running git apply on the
patch, 3 reading the patch itself. The patch reaches the model through that shell,
and the lane's own prompt requires findings to be grounded in what the code does
when executed. Removing the shell would leave a reviewer able to ground findings
only in diff text, which its instructions forbid. So no remedy here may narrow the
reviewer's read path.

Schema verification, at the pinned tag.
codex-rs/config/src/shell_environment_policy.rs at tag rust-v0.150.1 defines
ShellEnvironmentPolicyToml with inherit, ignore_default_excludes, exclude,
set, include_only, and filters as
BTreeMap<String, ShellEnvironmentPolicyFilter>, alongside
validate_shell_environment_policy_filter_config. So filters is a map on that
version and is actively validated. That refutes a reasonable concern raised during
review, that on 0.150.1 the exclusion field might be exclude (a glob array),
which would have left two mechanisms inert. Two practical notes: the tag is
rust-v0.150.1, not v0.150.1, and unauthenticated code search returns 401 while
an authenticated client works.

A correction worth recording

Earlier revisions of this PR body listed four config keys as "statically
unverifiable" because installing the pinned CLI failed with an authentication
error against a package registry. That conflated a blocked tool with an absent
fact: the CLI is open source and the struct was one fetch away. This
repository's own Design Review lane caught it and pointed out that the schema is
readable in public source at the pinned tag without installing anything. That is a
stronger argument for the review lanes' value than any advocacy: a passing
advisory verdict corrected an author's reasoning about their own change.

The acceptance criterion, and why it was the right one

The bar was not a green board: a real verdict had to LAND and the shell had to
still work. Both held. The worst available outcome was a reviewer narrowed into
agreement, which would have looked exactly like success from the board -- and the
only reason we can rule it out is that the reviewer was measured rather than its
verdict read.

Refs #8671
Refs #8789