fix(github): refuse local-gh GitHub mutations under identity "app" - #388
Conversation
Factory had two GitHub write identities. The lifecycle writeback honours `github.identity` and, on exact `app`, is performed server-side by the connected workspace GitHub App. Two mutations were never gated and still shelled out to `gh`, which authenticates as whichever human is logged in locally: - the guarded squash merge (`src/github/merge-gate.ts`), and - Notion intake issue create/edit (`src/intake/notion.ts`). Neither can be re-routed today: the connected `GithubConnectionWrite` surface exposes no `mergePullRequest` and no `createIssue`, and Factory must never receive or invoke a GitHub credential of its own. So under an explicit `app` identity both now refuse, naming the missing server-side capability and the operator's recovery path, instead of silently writing as the operator. `auto` and `user` are unchanged. Also: - `defaultMergeGate(config)` makes the FactoryLoop selection testable, the same shape as the existing `defaultGithubWriteback`. - `GhCliIssuePublisher` takes an explicit identity, so Notion intake's operator attribution is a decision on the record rather than an unnoticed default, and an injectable `gh` runner, because the class previously had no test seam at all and could only be exercised against live GitHub. - `StandalonePullRequest.source` keeps its `'gh'` member with the rationale written down: it is READ provenance, and this module performs no writes. - The stale `TODO(issue-52)` marker is rewritten; issue 52 is closed as completed and no longer owns the work. Reads are deliberately untouched: `gh pr view` carries no authorship. Session-Id: f298a3ee-c3f6-4f98-ae86-610a2d044e22
|
@coderabbitai review Requested for exact head |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change adds a GitHub write-identity policy. Local ChangesGitHub identity policy
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The PR prevents local GitHub mutations when using the app identity while preserving reads and existing user or auto behavior. No actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 7 files. (2 skipped: 1 unsupported, 1 too large.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 624e05a0ad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 9 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
CI's `package` job timed out `src/cli/teammate-mcp.test.ts` twice on this branch while passing on the identical base commit. The cause was load, not logic: `gh-identity.test.ts` became the third test file to import the 23.5k-line `src/orchestrator/factory.ts`, and the MCP test has a 5s timeout over a microtask-driven in-memory transport. Move the FactoryLoop selector's must-fire/must-not-fire pair into `factory.test.ts`, which already imports that module. The proof is unchanged and it now lives with the loop it selects for. `defaultMergeGate` gains a test-only runner seam so the pair can assert the `user`/`auto` arms actually merge. Without it those arms would have spawned a real `gh pr merge`; production still passes nothing and gets `defaultGhRunner`. Session-Id: f298a3ee-c3f6-4f98-ae86-610a2d044e22
|
@coderabbitai review Requested for exact head |
…e claiming Addresses all four review threads on #388. P1 (codex, cubic) — the identity refusal was raised inside `createIssue`, which `publishRepoTask` reaches only after `claimNotionDelivery` has reserved the durable exactly-once claim. An `app`-configured run therefore consumed the claim and returned blocked without a receipt, so the operator's retry under `user`/`auto` hit "durable Notion claim already exists" permanently — the gate took a hostage with no recovery path. `GithubIssuePublisher` gains an optional `assertWritable()`, called at the top of `publishRepoTask` before any claim, receipt or network call. The in-method asserts remain as a backstop. P2 (cubic) — `new GhCliIssuePublisher('user')` hardcoded the identity at the only production caller, so the refusal could never fire and an operator with `identity: "app"` would still get issues authored by their local `gh` user. That is the same "gate nobody invokes" defect this PR fixes for the merge gate, reintroduced one file over. The CLI now resolves `github.identity` from the contract. An absent contract is `auto` (Notion intake does not otherwise require one, and the schema synthesises absent to `auto`); a contract that exists but cannot be parsed is an error rather than a silent downgrade to the permissive value. `githubIdentitySchema` is exported so both readers share one declaration. P3 (cubic) — the README table listed `createIssue` for a combined create/edit row while an edit refusal names `updateIssue`. Split into two rows. Each fix has a must-fail/must-pass pair: the claim is not consumed and a permitted retry succeeds; the CLI blocks under a config-declared `app`; and `assertWritable` refuses without touching gh. Session-Id: f298a3ee-c3f6-4f98-ae86-610a2d044e22
|
@coderabbitai review Requested for exact head |
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…the task Second review round on #388; all three threads valid. P1 — `resolveNotionIntakeIdentity` read only a flat `github` key, so a split `workspaceConfig`/`nodeConfig` contract declaring `identity: "app"` resolved to `auto` and silently permitted the local-user write. It now reads both halves, node winning, matching `combineSplitConfigInput`'s spread order. P2 — the guard ran at the top of `publishRepoTask`, ahead of `findBySource`, `repositoryVisibility` and `missingLabels`, so read-only reconciliation and already-dispatched tasks were blocked under an app identity even though they write nothing. The refusal now sits immediately before each mutation: before `ensureNotionWorkUnitClaim` on the create path, so it still precedes the durable claim, and immediately before `updateIssue` on the reconciliation path. P3 — the README paragraph still described the hardcoded `"user"` this PR removed. Rewritten to describe the resolved identity, the split-contract rule, and the fact that only mutations refuse. `FleetCliDeps.notionGithub` is a test seam that receives the resolved identity, matching the existing `notionClaims`/`notionContracts` pattern, so the CLI test proves the wiring without invoking gh. Red/green for each: the flat-only reader resolves `auto` for a split contract; a top-of-task guard blocks reconciliation; the claim is not consumed. Session-Id: f298a3ee-c3f6-4f98-ae86-610a2d044e22
|
@coderabbitai review Requested for exact head |
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Session-Id: 01a03d7c-82f6-7b60-9500-45a3f158870b
|
@coderabbitai review Requested for exact head |
2 participants
Closes the un-gated half of #221.
What was actually wrong
#283 (shipped in 0.1.75) routed the Factory lifecycle writeback through the connected GitHub App under
github.identity: "app". It did not touch every GitHub write. Two mutations were still unconditional shell-outs togh, which authenticates as whichever human is logged in locally — so an operator who deliberately selectedappcould still produce human-authored writes without any signal.One correction to the issue's premise, verified on
origin/main:standalone-babysitter.tsperforms no GitHub writes at all. Its onlyghcall isgh pr view --json(a read), andsource: 'mount' | 'gh' | 'mount+gh'is read provenance. The babysitter's review replies and pushes are performed by the dispatched agent under the agent's own credential, not by the Factory process. The issue mislocated this write; the audit below locates it.Complete sink audit
Search that produced it (non-test
src/):Identity-bearing writes — all 8:
github.identitybeforewriteback/github.ts:341(gh) /mount/relayfile-github-connection-write.ts:84(app)factory.ts:11214git pushof a recovery branchwriteback/github.ts:327writeback/github.ts:412(gh) /writeback/github.ts:139(app)defaultGithubWritebackwriteback/github.ts:445-500(gh) /:213(app)writeback/github.ts:633(gh) /:272(app)github/merge-gate.ts:82viadefaultGhRunner:175appintake/notion.ts:407appintake/notion.ts:416appReads — untouched by design (
gh pr viewcarries no authorship, so gating them would break Factory without removing any attribution):github/standalone-babysitter.ts:86,github/merge-gate.ts:59,writeback/github.ts:360,389,405,425,545,570,614,656,674,702,intake/notion.ts:382,390,396.Not a Factory sink: agent-side review replies and PR pushes, instructed at
dispatch/templates.ts:282,286. These run under the dispatched agent's credential in its own checkout. Genuinely out of scope for a change to Factory's own write paths, and worth its own issue if the agent's attribution matters.Why refuse instead of route
The connected
GithubConnectionWritesurface exposespublishPullRequest,closePullRequest,postIssueComment,ensureRepositoryLabel,mutateIssueLabel,updateIssue— and no merge and no issue-create. Per the Axis B ruling on design-note #228, the no-GitHub-token constraint is not relaxed: Factory holds no PAT, noGITHUB_TOKEN, no App private key, and the invariant inmount/github-api-issue-read.tsthat Factory never receives or invokes a GitHub credential stands. Faking the capability would break it. So the honest behavior is a documented refusal, per DoD item 4.Neither refusal is reachable in the default cloud deployment (
mergePolicy: "never", no Notion intake), and each names the recovery path — setidentityto"user"or"auto"— so the gate does not take an honest caller hostage.Must-fail / must-pass evidence
Red-check by neutering
localGhMutationAllowedto its pre-change behavior (permit everything), which is exactly the defect:All three MUST-FIRE cases fail red and pass green; all six MUST-NOT-FIRE cases pass in both directions, so the test discriminates the fix from a change that simply broke local runs. The selector case matters on its own: without it the guard would be a gate nobody invokes — the class refusing correctly while
FactoryLoopkept constructing it with the default.Other suites:
buildexit 0;featuremap:checkexit 0;src/github src/intake src/config175 passed;src/cli/fleet.test.ts src/orchestrator/factory.test.ts src/writeback906 passed.A defect the red-check exposed
The first draft of this test used the real
ghrunner, becauseGhCliIssuePublisherhad no test seam — every method went straight to a module-levelrunGh. Running it created a junk issue (#385, since closed) and overwrote a merged PR's body (#7, restored byte-exact from itsuserContentEditshistory, 3056 chars). A class whose every method mutates production and which cannot be constructed with a fake is a defect in its own right, so this PR gives it an injectable runner and the test now assertscallsis empty on the refusal path — proving the write is declined before the process boundary rather than after something already landed.Review round (f315498)
Four threads, all valid, all fixed:
createIssue, whichpublishRepoTaskreaches only afterclaimNotionDelivery. Anapprun therefore burned the claim and returned blocked with no receipt, so the operator's retry underuser/autowas permanently blocked by its own aborted run.GithubIssuePublishergainedassertWritable(), called before any claim, receipt or network call.new GhCliIssuePublisher('user')at the only production caller, so the Notion gate could never fire and the README claim was false. This was the merge-gate defect this PR fixes, reintroduced one file over. The CLI now resolvesgithub.identityfrom the contract; an absent contract isauto(matching schema synthesis), while an unparseable one is an error rather than a silent downgrade to the permissive value.createIssuefor a combined create/edit row while the edit refusal namesupdateIssue. Split.Each has its own red/green pair, including a CLI-level test that drives the real
runFleetCliwith a config declaringidentity: "app"and asserts exit 1, a blocked result, and an unconsumed claim.Blocker
Two server-side capabilities are missing before merge and Notion intake can be app-authored rather than refused. Both belong behind the existing App credential boundary in Relayfile Cloud — Factory must still hold no credential — and follow the confirmed-write contract the existing methods use (
confirmWritemust acknowledge a real provider outcome; pending/failed/timeout must reject).Tracking for the read half remains relayfile-cloud#159. This is a blocker on making those two paths app-authored, not on this PR — the split audit trail is closed either way, because an explicit
appidentity can no longer produce a human-attributed write.Also decided by this PR
source: 'gh'is retained with a written rationale (DoD item 2) — it is read provenance and the mounted projection can be stale, so callers need it.TODO(issue-52)marker inmerge-gate.tsis rewritten. Don't assume localgh: create PRs + drive the merge gate via the authenticated workspace GitHub connection #52 is closed as completed and no longer owns that work.Still open, deliberately not in this PR
R1 — the laptop contract resolves to
auto.config/schema.ts:423synthesises an absentgithubkey toauto, so a local Factory still writes lifecycle comments as the operator. That is a config decision for the principal (cut laptop over toapp, or state thatautois intended), not a code change — and after this PR, choosingapplocally is now safe in the sense that nothing will silently fall back.Do not merge — the merge gate is the principal's.