feat(autonomous-dispatcher): two-token split + agent env scrubbing — agents get a scoped token that cannot approve/merge; wrapper keeps full-write (INV-77, closes #234)#261
Open
kane-coding-agent[bot] wants to merge 4 commits into
Conversation
…agents get a scoped token that cannot approve/merge; wrapper keeps full-write (INV-77, closes #234) In App auth mode the wrapper now mints a SECOND, scoped installation token (contents:write, issues:write, pull_requests:read) for the agent subprocess and scrubs the wrapper's full-write credential from the agent subtree, so an agent that runs `gh pr review --approve` / `gh pr merge` gets a deterministic 403 — turning the #191/#193 self-merge incident class from "agent CAN merge" into "agent's token CANNOT merge", independent of the (claude-only) PreToolUse hooks. - gh-app-token.sh: get_gh_app_token takes an optional permissions JSON (5th arg); _build_access_token_body validates it via python3 (injection-safe) and embeds it; get_gh_app_scoped_token convenience wrapper. - gh-token-refresh-daemon.sh: optional 6th permissions arg so the same daemon keeps the scoped token fresh (5-arg callers unchanged → full-grant). - lib-auth.sh: setup_agent_token (second scoped daemon, app mode; one-time WARN in PAT mode), build_agent_env_argv (CLI-agnostic env-scrub prefix), _strip_path_entry, drain_agent_pr_create (narrow PR-create broker with explicit --head), cleanup reaps the second daemon. - lib-agent.sh: _run_with_timeout prepends the scrub prefix to EVERY adapter launch (uniform across claude/codex/gemini/kiro/opencode/agy/generic). Empty prefix when no scoped token is armed → byte-identical no-regression in PAT mode. - lib-review-e2e.sh: E2E report broker (_post_brokered_e2e_report + prompt) so report delivery doesn't depend on the agent's write capability; dedup-guarded. - autonomous-dev.sh / autonomous-review.sh: call setup_agent_token; dev drains the PR-create broker; review posts the brokered E2E report. PAT mode (GH_AUTH_MODE=token) degrades to convention with a one-time WARN — no second token possible; behavior byte-identical to before. INV-77 (renumbered from INV-76 at rebase: PR #258's smoke no-response fix landed INV-76 on main first; this section took the next free number, disambiguated by #234). Docs (same PR, pipeline-docs-authority): INV-77 in invariants.md; github-app-setup.md scope set + attack-surface note; dev-agent-flow.md + review-agent-flow.md env tables; design + TC-TOKEN-SPLIT test cases. Tests: tests/unit/test-token-split-234.sh (31), conformance 15/15, ShellCheck -S error clean.
15 tasks
![kane-review-agent[bot]](https://avatars.githubusercontent.com/in/3020954?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review reached a blocking FAIL verdict — see the Review findings: comment on issue #234 for the full list of blocking findings and remediation steps. This PR is sent back to development; reviewDecision is set to CHANGES_REQUESTED until the findings are addressed and a new review passes (INV-52).
…efore launcher; open-PR fast path routes through broker under scoping (#234) Two [P1] findings from the post-approval review (kane-review-agent / codex): 1. [P1] _run_with_timeout appended the scoped-token `env …` scrub AFTER AGENT_LAUNCHER_ARGV. A launcher (`cc "$@"` / `bash -c '… claude "$@"' --`) execs the real CLI with its trailing args, so the `env …` was forwarded to claude as LITERAL positional args — the scrub silently no-op'd (the agent kept the wrapper's full-write credential) and could fail the launch. Fixed: the scrub prefix now precedes the launcher, so `env …` runs the launcher (and the agent it execs) under the scrubbed environment. TC-TOKEN-SPLIT-090 pins it (behavioral env-dump with a real fixed-command launcher + source-order lockdown; verified to FAIL on the pre-fix order). 2. [P1] emit_open_pr_fast_path_block ([INV-45]) still told the agent to run `gh pr create` directly, which 403s under the scoped token (pull_requests:read). Fixed: the fast-path open-PR step now routes through the AGENT_PR_CREATE_FILE broker when scoping is armed (agent writes branch:+title+body, wrapper opens the PR), exactly like the normal scoped-token path; PAT/no-scope keeps the direct create. TC-TOKEN-SPLIT-091 pins both branches. Docs (same PR): INV-77 Consumer note + dev-agent-flow.md updated for the launcher ordering and the fast-path broker routing. Tests: 35/35, conformance 15/15, ShellCheck -S error clean.
There was a problem hiding this comment.
Review reached a blocking FAIL verdict — see the Review findings: comment on issue #234 for the full list of blocking findings and remediation steps. This PR is sent back to development; reviewDecision is set to CHANGES_REQUESTED until the findings are addressed and a new review passes (INV-52).
…t strip GH_WRAPPER_DIR (INV-77, #234 review [P1]) The scoped-token env scrub (build_agent_env_argv) rewrote the agent PATH with the per-run GH_WRAPPER_DIR shim dir removed. But the agent (and vendored helpers) call BARE `gh` — the review prompt's `gh issue view`/`gh pr checks`, and scripts/mark-issue-checkbox.sh — which rely on that shim being on PATH. On `REAL_GH`/non-interactive-PATH hosts (#92) the shim is the agent's ONLY resolvable `gh`, so stripping it broke checkbox-ticking and E2E evidence with `gh: command not found`. Fix: leave PATH intact. The scrub still unsets GH_TOKEN_FILE / GITHUB_PERSONAL_ACCESS_TOKEN / GH_USER_PAT and sets GH_TOKEN=<scoped>. With GH_TOKEN_FILE unset, gh-with-token-refresh.sh falls through to `exec gh` inheriting the scoped GH_TOKEN — so the bare-`gh` path keeps working AND authenticates with the SCOPED token (`gh pr review --approve`/`gh pr merge` still 403). Removed the now-unused _strip_path_entry helper. Tests: TC-TOKEN-SPLIT-032 flipped to assert NO PATH= override + source lockdown (_strip_path_entry gone, build_agent_env_argv emits no PATH=); TC-040 PATH assertion flipped (shim dir now KEPT); new TC-TOKEN-SPLIT-092 functionally proves a bare `gh` under the scrub resolves the shim → real gh with the scoped token on a #92 REAL_GH host. Verified the new tests FAIL on the pre-fix PATH-strip. 36/36, conformance 15/15, ShellCheck -S error clean. Docs (same PR): INV-77, dev-agent-flow.md, github-app-setup.md, design doc all updated — PATH is kept, not stripped.
There was a problem hiding this comment.
Review reached a blocking FAIL verdict — see the Review findings: comment on issue #234 for the full list of blocking findings and remediation steps. This PR is sent back to development; reviewDecision is set to CHANGES_REQUESTED until the findings are addressed and a new review passes (INV-52).
…— point GH_TOKEN_FILE at the scoped file, not a one-time snapshot (INV-77, #234 review [P1]) build_agent_env_argv set GH_TOKEN=<scoped> ONCE at launch and UNSET GH_TOKEN_FILE, so the gh-with-token-refresh.sh shim had no file to re-read — the agent kept using the launch-time token snapshot. App-mode dev/review/E2E runs longer than the 1h GitHub-App token TTL then used a STALE/expired token and started failing branch pushes, progress comments, checkbox ticks, and PR reads. Fix: point the agent's GH_TOKEN_FILE at AGENT_GH_TOKEN_FILE (the SCOPED token file, kept fresh by the scoped refresh daemon) instead of unsetting it. The shim re-reads that file on every gh call, so the daemon's refreshes are honored — the agent's gh stays valid for the whole run AND remains scoped (pull_requests:read → approve/merge still 403). GH_TOKEN=<scoped> is kept as a snapshot fallback for any direct gh resolution that bypasses the shim (the shim re-reads the file and overrides it, so the fresh file always wins). GITHUB_PERSONAL_ACCESS_TOKEN / GH_USER_PAT stay unset. SECURITY: GH_TOKEN_FILE points at the SCOPED file only; the wrapper's full-write token file (a different path) is never exposed to the agent subtree. Tests: TC-TOKEN-SPLIT-031 now asserts GH_TOKEN_FILE=<scoped> (not -u); TC-040 asserts the dumped GH_TOKEN_FILE is the scoped file (not the wrapper's full-write file); TC-092 asserts the bare-gh shim reads the scoped file; NEW TC-TOKEN-SPLIT-093 functionally proves refresh-awareness — a scoped-file rewrite between two bare-gh calls is picked up (call 1 = initial token, call 2 = refreshed token). Verified all four FAIL on the pre-fix snapshot-only form (TC-093 saw the stale token on call 2). 38/38, conformance 15/15, ShellCheck -S error clean. Docs (same PR): INV-77, dev-agent-flow.md, github-app-setup.md, design doc, test-cases all updated — GH_TOKEN_FILE points at the scoped file (refresh-aware), not unset.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #234.
In App auth mode the wrapper now mints a second, scoped GitHub-App installation token (
contents:write,issues:write,pull_requests:read) for the agent subprocess and scrubs the wrapper's full-write credential from the agent subtree. An agent that runsgh pr review --approve/gh pr mergenow gets a deterministic 403 from its own token — turning the self-merge incident class (#191/#193) from "agent CAN merge" into "agent's token CANNOT merge", independent of the (claude-only) PreToolUse hook layer (which missesgh apiand non-claude CLIs). The wrapper keeps the full-write token and is the sole approve/merge/label/PR-create/verdict path.This is the new INV-77 (credential split contract). It was authored as INV-76; PR #258's smoke
no-responsefix landed INV-76 onmainfirst during review, so this section took the next free number (renumbered at rebase, disambiguated by #234).How it works
lib-agent.sh::_run_with_timeoutprepends anenv-prefix (build_agent_env_argv) to every adapter launch —GH_TOKEN=scoped,GH_TOKEN_FILE/GITHUB_PERSONAL_ACCESS_TOKEN/GH_USER_PATunset, the per-runGH_WRAPPER_DIRshim stripped fromPATH.pull_requests:readalso blocksgh pr create, so the dev agent writesbranch:+title+body to a broker file and the wrapper opens the PR with an explicit--head(drain_agent_pr_create). The browser-E2E report is brokered the same way (_post_brokered_e2e_report).GH_AUTH_MODE=token): a PAT cannot be down-scoped, so there is no second token —setup_agent_tokenlogs a one-time WARN ("enforcement degraded to convention in PAT mode"), the scrub prefix is empty, and behavior is byte-identical to before.ghuses cannot approve/merge. OS-user/container isolation is out of scope (feat(autonomous-dispatcher): two-token split + agent env scrubbing — agents get a scoped token; wrapper keeps full-write; E2E report posting brokered #234).Design
docs/designs/token-split-234.md)Test Plan
docs/test-cases/token-split-234.md,TC-TOKEN-SPLIT-NNN)tests/unit/test-token-split-234.sh— 31/31 (scoped-mint body + injection rejection, daemon--permissionsforwarding,setup_agent_tokenapp/PAT paths, one-time PAT WARN,build_agent_env_argvscrub assembly + empty-prefix no-regression, env-dump verify-by-construction gate via the real_run_with_timeout(no full-write credential, no shim PATH in the agent env),pull_requests:readscope pin, PR-create broker--head+ skip-when-no-branch, E2E report broker)_run_with_timeoutchange is transparent when no scoped token is armed)-S errorclean (CI list + the two touched non-listed files)gh pr createhad no--head→ would fail from the wrapper's base-branch cwd) and the report-double-post nit were fixedE2E / soak
GH_AUTH_MODE=appfor this project and watch one dev+review cycle complete with the scoped token (branch push, brokered PR create, progress comments, review, E2E report) before flipping other projects.Acceptance Criteria (issue #234)
envdump in a stub agent shows no full-write credential and no wrapper gh shim → TC-TOKEN-SPLIT-040, env-dump gate.pull_requests:read) → TC-TOKEN-SPLIT-060.Pipeline docs (same PR — pipeline-docs-authority)
docs/pipeline/invariants.md— INV-77docs/github-app-setup.md— exact scope set + attack-surface note + PAT degradationdocs/pipeline/dev-agent-flow.md+review-agent-flow.md— agent env tables + brokersscripts/autonomous.conf.example—AGENT_TOKEN_PERMISSIONSknobPost-install / upgrade
This PR edits existing dispatcher scripts/libs only (no new
scripts/*.shorlib-*.shfiles are added), so it does not require aninstall-project-hooks.shre-run afternpx skills update. (The new files are a test + docs, neither of which is symlinked into projectscripts/.)