Identity separation: per-role and per-mix-bucket session env (#304)

[polymath-orchestrator]

Refs #304 — deliberately does NOT close it. The mechanism ships here; acceptance criterion 2 is unmet by operator decision and the config cutover is blocked on credentials (both scored honestly below). An independent reviewer flagged auto-closing this as its top finding, and it was right: merging must not mark #304 done.

Workers push and call gh under the orchestrator's own login, so author and reviewer collapse to one identity and GitHub refuses a blocking review (Can not request changes on your own pull request). This gives a session a GitHub identity resolved from its role and its resolved harness, so a worker can author a PR under a login the reviewing orchestrator can actually block.

Design: docs/design/304-identity-separation.md.

Three findings that reshaped the issue

1. The bot accounts already exist. nhod-claude and nhod-codex are already org members with write on this repo and on agent-vault, and already author PRs elsewhere (nhod-codex → Consolidate runtime architecture and delete dead generations #271 here). The provisioning half was largely done — it had just never reached the orchestrator box, where one gh config and one SSH key collapse every pane to polymath-orchestrator.
2. ⚠️ mainprotect has no pull_request rule — only deletion, non_fast_forward, required_status_checks, merge_queue. GitHub enforces CHANGES_REQUESTED as a merge blocker only when a review rule exists. So identity separation alone satisfies AC1 but not AC2 — see the honest scoring below.
3. Seats are full (5/5, Team, agent-vault private), so reusing the existing bots costs nothing while a new one costs a seat.

What this ships

Env on RoleOverride and on each WorkerMix bucket, resolved by ProjectConfig.ResolveEnv:

projectEnv  <  RoleOverride(role).Env  <  mixBucket(resolvedHarness).Env  <  AO_* internals

Identity is a path, never a secret. The committed spec carries only GH_CONFIG_DIR and GIT_CONFIG_GLOBAL paths into ~/.ao/identities/<bot>/; tokens live on disk, outside git. A url.insteadOf rewrite (SSH → HTTPS) routes push auth through gh auth git-credential, which resolves against each pane's own GH_CONFIG_DIR — so one global credential helper yields a different identity per pane, and zero SSH keys need provisioning.

Two things worth reviewing closely

The POLYPOWERS_AUTOMERGE escalation. projectRuntimeEnv strips that key from project env precisely so a project cannot self-grant autonomous merge. A naive role-env merge would have let worker.env: {POLYPOWERS_AUTOMERGE: "1"} walk straight around the merge gate. The strip now applies to the merged map. Tested.

The switch trap. switchLiveHarness and relaunchTerminatedWithHarness receive the new harness as a parameter while the session record still holds the old one. Resolving identity from the record compiles, passes every spawn test, and silently leaves a switched session pushing under the previous bot's login. The harness is therefore a parameter, not a lookup — and TestSwitchHarness_IdentityEnvFollowsTheNewHarness was verified to fail against exactly that mistake.

Reviewer panes are deliberately not given a worker identity (review/launcher.go never calls projectRuntimeEnv), which is what keeps author and reviewer distinct. A future refactor that "unifies" the two env paths would re-collapse them — commented in code.

Two deliberate departures from the plan

• No CDC redaction migration. SQLite json_remove has no array wildcard, so a trigger could redact the three role paths but never workerMix[i].env — partial coverage that reads as complete. Instead Validate() refuses credential-shaped keys in any role/bucket env and SetConfig calls it on every write, which is total. Also avoids burning a migration version (a recurring collision point between parallel PRs) on a weaker guard.
• No ao doctor check. "An identity that collapses to the reviewing login is a failure" is Polymath review policy, not generic daemon behavior, and this tree is under the vanilla rule. The backend diff stays minimal and upstream-shaped; the policy check lives in ops/identity.mjs check --all.

The silent-corruption class this closes

The load-bearing surface was the read path, not set-config: ao project get --json re-marshals through the CLI's own mirror structs, and a field missing there is silently dropped — and that read path is what ops/project-config check/capture consume. Unfixed, worker.env would have been permanent phantom drift and capture would have erased the identity config apply had just installed. The frontend had the mirror-image bug: ProjectSettingsForm rebuilt mix rows as {agent, model, weight}, wiping bucket env on any settings save. Both are now regression-tested against the lossy versions (both tests verified to fail against them).

Acceptance criteria, scored honestly

#	Criterion	Status
1	Workers author PRs under a distinct login	Mechanism shipped. Live once credentials exist and the spec is cut over.
2	A blocking --request-changes actually blocks the merge	Not met, by decision — branch protection stays as-is per operator call. Proposed follow-up: teach ops/final-review-status.mjs check --mode autonomous to refuse a current-head CHANGES_REQUESTED from a non-author, which closes it on the path these PRs actually take.
3	BEADS_TO_GH_ASSIGNEE_MAP / claim+mirror still resolve	Unaffected: no .beads/ here, and skills resolve the login from gh api user, which now returns the bot.

⚠️ Sequencing — the cutover is NOT in this PR, on purpose

GH_CONFIG_DIR pointing at a directory that does not exist makes gh unauthenticated. Landing identity env in the committed spec before the bots are authenticated on the box would break gh for every worker on the next project-config apply. Order is fixed: identity.mjs create → operator runs gh auth login per bot (the one step an agent cannot do) → identity.mjs check --all passes → then cut the spec over.

Testing

• npm run ci:backend green (build, vet, -race, golangci-lint).
• Frontend: 614/614.
• npm run format:check green.
• The two regression tests and the switch-trap test were each mutation-verified: reverted to the buggy version, confirmed red, restored, confirmed green.

Notes for the reviewer

• Backend change → vanilla rule: upstream-shaped, upstream PR to follow regardless.
• Touches backend/internal/session_manager/** (sensitive path) → autonomous merge parks for a human.
• Residual risk, documented not hidden: separation is cooperative, not sandboxed. Panes still share $HOME, so a worker that deliberately unset GH_CONFIG_DIR could still act as the orchestrator. Strictly weaker threat model than today's structural single-identity, but not isolation.

🤖 Generated with Claude Code

https://claude.ai/code/session_01RP5jQC1fNo5ak4oyogD3ay

---

^{Need help on this PR? Tag /codesmith with what you need. Autofix is disabled.}

[Copilot]

Pull request overview

Implements per-session GitHub identity separation by introducing env layering at the project-config level (project env < per-role env < per-workerMix-bucket env keyed by resolved harness), plus adds ops tooling to provision/check per-bot gh/git identity directories so workers can author PRs under a login distinct from the reviewing orchestrator.

Changes:

• Backend: add env to RoleOverride and WorkerMixEntry, implement ProjectConfig.ResolveEnv(kind,harness) and wire session_manager runtime env resolution to the resolved harness (including switch/relaunch paths), while stripping POLYPOWERS_AUTOMERGE post-merge.
• Surfaces: update CLI JSON mirror structs, OpenAPI + generated frontend schema, and frontend project settings form to round-trip bucket env without wiping it.
• Ops: expand assertNoSecretEnv to cover role/bucket env maps and add ops/identity.mjs (+ core + tests) for identity directory creation and validation.

Reviewed changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
ops/project-config-core.mjs	Extends secret-shaped env key guard to role/bucket env maps.
ops/project-config-core.test.mjs	Adds regression tests ensuring the secret guard reaches role/bucket env and allows identity path keys.
ops/identity.mjs	New CLI to create/check/list per-bot identity directories and validate separation from orchestrator login.
ops/identity.test.mjs	End-to-end tests for ops/identity.mjs using a fake gh and temp HOME.
ops/identity-core.mjs	Pure helpers for identity path layout, gitconfig rendering, and validation rules.
ops/identity-core.test.mjs	Unit tests for identity-core helpers and validation failure modes.
backend/internal/domain/projectconfig.go	Adds per-role env + env resolution (ResolveEnv) and validates role/bucket env against secret-shaped keys.
backend/internal/domain/workermix.go	Adds env to worker-mix entries and helper to pick env by harness (first match).
backend/internal/domain/projectconfig_env_test.go	Domain-level tests for env layering precedence and worker-only bucket application.
backend/internal/session_manager/manager.go	Threads harness into runtime env resolution; uses resolved harness on switch paths; strips automerge key post-merge.
backend/internal/session_manager/provision_test.go	Updates tests for new projectRuntimeEnv(kind,harness,cfg) signature.
backend/internal/session_manager/identity_env_test.go	Session-manager seam tests ensuring identity env follows resolved harness (including switch trap).
backend/internal/service/project/service_test.go	Verifies config writes reject secret-shaped keys in role/bucket env.
backend/internal/cli/project.go	CLI JSON mirror updated to round-trip role/bucket env fields.
backend/internal/httpd/apispec/openapi.yaml	OpenAPI schema updated to include env on role override + worker mix entries.
frontend/src/api/schema.ts	Regenerated client schema reflecting OpenAPI env additions.
frontend/src/renderer/components/ProjectSettingsForm.tsx	Preserves workerMix[i].env through load/edit/save so settings edits don’t wipe identity env.
frontend/src/renderer/components/ProjectSettingsForm.test.tsx	Regression test ensuring role/bucket env survives a save even though the form doesn’t expose it.
docs/design/304-identity-separation.md	Design doc capturing rationale, env layering rules, sequencing, and residual risk.

Comments suppressed due to low confidence (1)

ops/project-config-core.mjs:98

• The error message still says “Move secrets out of project env…”, but assertNoSecretEnv now scans multiple env maps (env, worker.env, orchestrator.env, prime.env, workerMix[i].env). When the offender is under a role/bucket env path, the current message is misleading for operators trying to fix the config.

		throw new Error(
			`refusing to write secret-like env key(s) to a committed spec: ${offenders.join(", ")}. ` +
				`Move secrets out of project env, or exempt specific non-secret keys via ` +
				`AO_PROJECT_CONFIG_ALLOW_ENV_KEYS=<comma-separated key names>.`,
		);

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[polymath-orchestrator]

final-review — verdict: clean · PARKED for human merge

Head reviewed: cfe1b390f1d0888beb0478cb81d7fd8f0c1afb33
Independent reviewer: Codex (gpt-5.5) — different model family from the implementer (Claude). PR-integrated reviewer: Copilot (arrived cycle 1).
Cycles: 6 · Findings fixed: 8 HIGH, 20 MEDIUM, 4 LOW/nit · Unresolved current-head threads: 0
CI: all 13 GitHub checks green on this SHA. Local: backend ci:backend green, frontend 615/615, ops 407/407, format:check green.

Reviewer's closing verdict: "READY. No CRITICAL or HIGH issue remains that makes merging unsafe. ResolveEnv layering, token shadowing, AO-owned precedence, spawn/switch/restore selection, and reviewer-pane isolation are sound."

⛔ PARKED — sensitive path, human merge required

The diff touches backend/internal/session_manager/**. Per the repo's sensitive-path rule, autonomous merge parks for a human regardless of how clean the review is. merge-park status set with reason=human-required.

The review earned its keep — 3 bugs that would have shipped a silently broken feature

Each looks correctly configured and fails silently, which is the exact failure mode #304 exists to eliminate.

1. GH_TOKEN overrides GH_CONFIG_DIR (HIGH). gh resolves GH_TOKEN > GITHUB_TOKEN > hosts.yml, and a pane inherits the daemon's whole environment. An inherited token overrides the bot's stored credentials — and this is reachable in supported config, since ProjectConfig.Env may legitimately carry a GITHUB_TOKEN (that's why the CDC triggers redact $.env at all). Every worker would have kept authenticating as the orchestrator while looking perfectly configured. Verified live against gh. Now shadowed to empty (not deleted — the pane must override an inherited value) whenever a session selects an identity by path.

2. Inherited git credential helpers answer first (HIGH). git collects helpers into a list and takes the first that answers. The identity gitconfig [include]s the real ~/.gitconfig above its own settings, so a store/cache/keychain helper there is asked first and hands back the orchestrator's credentials. Proven with git credential fill: without an empty helper = reset the ORCHESTRATOR answers; with it, the bot does. Validation now checks the list git will actually use — includes followed, resets applied, and covering generic, host-scoped and path-scoped credential sections.

3. AO_RUN_FILE was forgeable (security). Every other AO-owned key is unconditionally assigned; AO_RUN_FILE was only set when the daemon has a run file, so otherwise a project-supplied value survived and could forge the path ao hooks callbacks resolve against. Found by the table-test the reviewer asked for — the original test only proved AO_SESSION_ID, which is precisely why it hid.

Also caught and fixed: a regression I introduced (check --all crashed with [object Object] exactly once a spec configured an identity — i.e. at cutover, the one moment the gate exists for), the settings form silently carrying a bucket's identity across an agent change, and a privilege-escalation path where worker.env could have self-granted POLYPOWERS_AUTOMERGE.

Where I pushed back

• No CDC redaction migration. SQLite json_remove has no array wildcard, so a trigger could cover the three role paths but never workerMix[i].env — partial coverage that reads as complete. Validate() on the write path is total.
• No ao doctor check. "An identity that collapses to the reviewing login is a failure" is Polymath review policy, not generic daemon behavior, and this tree is vanilla-rule. The check lives in ops/identity.mjs.
• Copilot validation stays scoped to configured harnesses. The reviewer wanted it extended to every harness that exists; that would force COPILOT_GITHUB_TOKEN on every identity-using project. An explicit --agent copilot override fails loudly at runtime — an error, not a silent collapse. The reviewer accepted this.

⚠️ Two things this PR does NOT do

• AC2 is unmet, by operator decision. mainprotect carries no pull_request rule, so a --request-changes will be visible but will not block the merge queue. Proposed follow-up: teach ops/final-review-status.mjs check --mode autonomous to refuse a current-head CHANGES_REQUESTED from a non-author.
• The config cutover is deliberately excluded. GH_CONFIG_DIR pointing at a nonexistent directory makes gh unauthenticated, so landing identity env in the spec before the bots are authenticated would break gh for every worker on the next apply. Sequence: identity.mjs create → operator runs gh auth login per bot → check --all → then cut over.

This PR therefore refs #304 rather than closing it.

[polymath-orchestrator]

review requested (SDLC marker)\n\nReviewer: Claude (independent local reviewer)\nHead: e0901faad2aec2b3d7f6099a4c46fc6df560e60f\nCycle: final-review rebase pass

[polymath-orchestrator]

review verdict (SDLC marker)\n\nCycle: 1\nHead reviewed: e0901faad2aec2b3d7f6099a4c46fc6df560e60f\nReviewer: Claude (independent local reviewer)\nVerdict: fixes required\nFindings fixed: 4 medium/low ops identity/config validation findings plus stale nopool rebase concern verified fixed.\nFix head: 785e116464faf55143d17c6c0d83f53573e6a97b

[polymath-orchestrator]

final-review cycle 2 verdict

2a. Reviewer/model family: Claude independent foreground review.

2b. Reviewed head: 785e116.

2c. Verdict: fixes required; 2 findings, both fixed.

2d. Fix head: 9b0744b.

2e. Fixes: removed the retired nopool helper/test restored during rebase, and updated docs/design/304-identity-separation.md so the phase and acceptance-status notes match the shipped PR #311/autonomous-merge path.

2f. Local verification after fixes: npm run ci:backend, npm run test:ops, npm run format:check, npm test --prefix frontend, npm run typecheck --prefix frontend, npm run build:web --prefix frontend.

[polymath-orchestrator]

final-review cycle 3 verdict

3a. Reviewer/model family: Claude independent foreground recheck.

3b. Reviewed head: 9b0744b.

3c. Verdict: clean for the cycle-2 fixes.

3d. Findings: none. The retired nopool helper/symbols are absent from backend/internal/domain/workermix.go, remaining nopool occurrences are tests/docs asserting the legacy label is ignored, and docs/design/304-identity-separation.md now marks phase 4 shipped via #311 and AC2 met on the autonomous-merge path.

3e. Residual risk noted by reviewer: envForHarness depends on validateMixEnvConsistentPerHarness preserving same-harness env consistency; existing implementation/tests cover that invariant.

[polymath-orchestrator]

final-review — verdict: clean · PARKED for human merge

1a. Head reviewed: 9b0744b.

1b. Independent reviewer: Claude foreground recheck after rebase and review fixes.

1c. Review result: clean. Cycle 1 findings fixed at 785e116; cycle 2 findings fixed at 9b0744b; cycle 3 recheck found no remaining findings.

1d. Review threads: 0 unresolved current-head threads. The only Copilot thread is outdated and resolved.

1e. Local CI: npm run ci:backend; npm run test:ops; npm run format:check; npm test --prefix frontend; npm run typecheck --prefix frontend; npm run build:web --prefix frontend.

1f. Remote CI: all GitHub checks are green on this SHA, including Go, Frontend, Prettier, gitleaks, and CLI E2E.

1g. Statuses set on this SHA: final-review=success, review-passed=success, merge-park=success with reason=human-required.

1h. Park reason: the diff touches backend/internal/session_manager/**, which is a sensitive path under this repo's policy, so autonomous merge must park for a human even with a clean gate.

[polymath-orchestrator]

Closing without merge, per the #313 harmony plan (operator-approved implementation direction, 2026-07-13/14): separate GitHub bot identities, credential routing, and per-role/mix-bucket environment layering add a large subsystem that does not solve a current need of this single-operator local product. This is a rejection on product-scope grounds, not a deferral — see the 'Close without implementing' section of the plan comment on #313. The identity mechanism here is not security isolation; cross-family independent review plus the current-head final-review status remains the review control.