244: Enforce issue-owned implementer branches - #247
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Warning Review limit reached
Next review available in: 101 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
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: eaa412ec1e
ℹ️ 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".
| if (!normalizedHead.startsWith('factory/') || !normalizedKey) return false | ||
| return /^\d+$/u.test(normalizedKey) | ||
| ? normalizedHead === `factory/${normalizedKey}` || normalizedHead.startsWith(`factory/${normalizedKey}-`) | ||
| : containsIssueKey(normalizedHead, normalizedKey) |
There was a problem hiding this comment.
factory/
For Linear-style keys, this accepts the issue key anywhere in the branch rather than in the ownership position. For example, factory/ar-245-fix-ar-244 is considered owned by AR-244, so a stale branch primarily belonging to AR-245 can pass the new worktree and publication safety gates if it also mentions AR-244 later in its name. Apply the same factory/<issue> or factory/<issue>-... anchoring used for numeric keys.
Useful? React with 👍 / 👎.
Review — issue 244 (reviewer:
|
| Check | Result |
|---|---|
tsc -p tsconfig.build.json --noEmit |
clean |
| Full suite | 1555/1556 passed; the single failure is dist-entrypoints.test.ts, which imports dist/ — an artifact of my skipping npm run build, not a code failure. Matches the author's reported 1,556. |
Targeted tests (issue-key-match, agent-worktree, writeback, relayfile-github-connection-write) |
76/76 passed |
| Branch provenance | single commit off origin/main; b2ad725 is not an ancestor — WIP uncontaminated |
factory/3022-chief-org-live-population |
still at 2977c744, not deleted, not force-pushed |
cloud#3024 |
closed 15:58:07Z |
Guards land before the side effect
This was the thing worth checking, and it holds in all three places:
writeback/github.ts:91— theexpectedHeadRefmismatch throw sits afterheadRefderivation but beforepush HEAD:refs/heads/…. The test assertsghCallsis empty and exactly one git call was made.relayfile-github-connection-write.ts:53— throws beforeheadShaand before any mount write;mount.writesasserted empty. Moving theheadShacomputation below the guards also drops a needless git call on the failure path.agent-worktree.ts:323—assertSafeWorktreeis the first statement in#prepare, ahead ofworktree pruneandmkdir, so the regression test'sENOENTassertion on the worktree path is meaningful rather than incidental.
The regression test asked for in the DoD is present and genuine: agent-worktree.test.ts pre-creates factory/3022-chief-org-live-population, dispatches issue 3021, asserts the specific rejection message and that no worktree was created.
The root fix is the right one — implementer.spec.branch was already derived correctly by decisionWithLifecycleBranches and computed at the old :6658 as expectedHeadRef for the reconcile path, but never handed to the publisher. Threading it through and tightening published.headRef !== expectedHeadRef (previously the no-op !== (remoteBranch ?? published.headRef)) closes the hole at the boundary rather than by convention.
Non-blocking findings
1. factoryBranchBelongsToIssue is anchored for numeric keys but not for Linear-style keys (src/issue-key-match.ts:105)
The numeric path is exact (=== factory/<key> or startsWith('factory/<key>-')). The non-numeric path delegates to containsIssueKey, which is an unanchored discovery heuristic — deliberately loose for matching PRs found in the wild. As an enforcement predicate it is too permissive:
factoryBranchBelongsToIssue('factory/ar-999-fix-ar-244', 'AR-244') // => true
Confirmed by execution, not inspection. Severity is genuinely low: Factory-derived branches are factory/<key>-<repo>-<runid8>, and a repo slug will not contain another issue key, so factory/ar-999-agentworkforce-factory-abcd1234 correctly returns false. It only bites a hand-named or adopted branch reaching assertSafeWorktree. Suggest anchoring both key shapes identically.
2. The durable-receipt guard converts existing corruption into a hard stop (src/orchestrator/factory.ts:6369)
durableReceipt.headRef !== expectedHeadRef now throws where it previously returned the receipt. For lifecycles persisted before this change — whose receipts recorded the stale symbolic-ref head, i.e. precisely the corruption being fixed — this turns a silently-wrong PR into a repeating failure. Failing loudly is the asked-for behaviour so this is not wrong, but it is worth confirming an operator has a lifecycle-reset escape hatch for already-corrupted state.
I checked the obvious re-dispatch concern and it does not apply: #claimDispatchLifecycle is called with preparedRunId, so decisionWithLifecycleBranches is skipped on the seed path, and dispatchDecision is replaced by structuredClone(lifecycleClaim.lifecycle.decision) on takeover. The branch is stable across resume.
3. Scope flag: lifecycleRunId = !dryRun ? randomUUID() : undefined (:2569) now applies decisionWithLifecycleBranches to every live dispatch, including non-durable local fleets that previously received neither a lifecycle branch nor an invocationId. The spec.invocationId ?? … guard at :14255 handles re-entry correctly, but this widens the blast radius past the branch fix and deserves a line in the PR body.
On DoD item 3
Independently corroborated before the PR was opened. cloud#3024 carried exactly one commit — 2977c744, authored 06:26:24Z, ~6h before the 3021 implementer was dispatched — touching organization-hierarchy.ts, which is 3022's subject matter. It contained zero 3021 work, so the issue's premise ("pushed its work onto a branch belonging to a different issue") is not what happened: the implementer pushed nothing and opened a PR on a branch that already existed. The conflict was real but unrelated — the branch forked at b143b3b3, sat 1 ahead / 4 behind, and 43e68b5b (#3018) later touched the same file on main.
Closing as obsolete was correct; a re-cut would have produced an empty PR. I separately confirmed the author's claim that the 3021 repair landed via cloud#3017, merged 06:52:23Z — also before dispatch.
Item 4 (prune stale factory/* branches) is correctly not executed here, per the constraints.
Loose end, outside this PR
cloud#3021 is still OPEN despite its repair having merged via cloud#3017. Worth a separate disposition.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 11 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/orchestrator/factory.ts">
<violation number="1" location="src/orchestrator/factory.ts:6346">
P3: The publish legacy gate uses a loose `startsWith(`${record.issue.key.toLowerCase()}-`)`, so malformed legacy branches such as `3021-`, `3021- `, or `3021--foo` pass the ownership gate here, while the worktree safety layer (`isAuthorizedExistingPrBranch`) rejects exactly those forms with a strict `^<key>-[A-Za-z0-9][A-Za-z0-9._-]*$` regex. When an implementer goes through a worktree these gates disagree. Align the publish gate with the worktree regex so the two layers authorize the same set of legacy existing-PR branches.</violation>
<violation number="2" location="src/orchestrator/factory.ts:14255">
P1: When a caller or triage result supplies `spec.invocationId`, this preserves it across a genuine reopen despite generating a new `runId`. `BatchTracker` and the fleet then reuse the old provider idempotency key, which can deduplicate the new spawn against a prior run; always derive the ID from the new run here (takeover already reuses the persisted lifecycle decision).</violation>
</file>
<file name="src/git/agent-worktree.ts">
<violation number="1" location="src/git/agent-worktree.ts:323">
P2: For non-numeric (Linear-style `XXX-N`) issue keys, `factoryBranchBelongsToIssue` falls back to `containsIssueKey`, whose terminal-boundary rule `-(?!\d)` fails whenever the separator following the number is followed by a digit. The worktree branch is always built as `factory/<issue>-<repo>-<runId>` (see `decisionWithLifecycleBranches` in factory.ts), so if the repo slug begins with a digit — e.g. issue `AR-244`, repo `2fa-demo` → branch `factory/ar-244-2fa-demo-abc12345` — `containsIssueKey` returns false and this new check falsely rejects a legitimate Factory branch, throwing in `prepare` and blocking dispatch for that issue. The numeric issue-key path avoids this because it uses an anchored `startsWith('factory/<key>-')`. Mirror that anchored check for the non-numeric path so the separator digit is irrelevant.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| invocationId: spec.invocationId ?? | ||
| `factory:${decision.issue.key}:${runId}:${spec.role}:${sanitizeAgentSlug(spec.name)}`, |
There was a problem hiding this comment.
P1: When a caller or triage result supplies spec.invocationId, this preserves it across a genuine reopen despite generating a new runId. BatchTracker and the fleet then reuse the old provider idempotency key, which can deduplicate the new spawn against a prior run; always derive the ID from the new run here (takeover already reuses the persisted lifecycle decision).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/orchestrator/factory.ts, line 14255:
<comment>When a caller or triage result supplies `spec.invocationId`, this preserves it across a genuine reopen despite generating a new `runId`. `BatchTracker` and the fleet then reuse the old provider idempotency key, which can deduplicate the new spawn against a prior run; always derive the ID from the new run here (takeover already reuses the persisted lifecycle decision).</comment>
<file context>
@@ -14225,7 +14252,8 @@ function decisionWithLifecycleBranches(
// The same persisted lifecycle reuses this id after takeover, while a
// genuine reopen gets a new id and cannot replay an old placement ack.
- invocationId: `factory:${decision.issue.key}:${runId}:${spec.role}:${sanitizeAgentSlug(spec.name)}`,
+ invocationId: spec.invocationId ??
+ `factory:${decision.issue.key}:${runId}:${spec.role}:${sanitizeAgentSlug(spec.name)}`,
}
</file context>
| invocationId: spec.invocationId ?? | |
| `factory:${decision.issue.key}:${runId}:${spec.role}:${sanitizeAgentSlug(spec.name)}`, | |
| invocationId: `factory:${decision.issue.key}:${runId}:${spec.role}:${sanitizeAgentSlug(spec.name)}`, |
| if (target === base || !target.startsWith(`${expectedRoot}/`)) { | ||
| throw new Error(`Refusing unsafe Factory worktree path ${target}; expected a child of ${expectedRoot}`) | ||
| } | ||
| if (worktree.branch.startsWith('factory/') && !factoryBranchBelongsToIssue(worktree.branch, worktree.issueKey)) { |
There was a problem hiding this comment.
P2: For non-numeric (Linear-style XXX-N) issue keys, factoryBranchBelongsToIssue falls back to containsIssueKey, whose terminal-boundary rule -(?!\d) fails whenever the separator following the number is followed by a digit. The worktree branch is always built as factory/<issue>-<repo>-<runId> (see decisionWithLifecycleBranches in factory.ts), so if the repo slug begins with a digit — e.g. issue AR-244, repo 2fa-demo → branch factory/ar-244-2fa-demo-abc12345 — containsIssueKey returns false and this new check falsely rejects a legitimate Factory branch, throwing in prepare and blocking dispatch for that issue. The numeric issue-key path avoids this because it uses an anchored startsWith('factory/<key>-'). Mirror that anchored check for the non-numeric path so the separator digit is irrelevant.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/git/agent-worktree.ts, line 323:
<comment>For non-numeric (Linear-style `XXX-N`) issue keys, `factoryBranchBelongsToIssue` falls back to `containsIssueKey`, whose terminal-boundary rule `-(?!\d)` fails whenever the separator following the number is followed by a digit. The worktree branch is always built as `factory/<issue>-<repo>-<runId>` (see `decisionWithLifecycleBranches` in factory.ts), so if the repo slug begins with a digit — e.g. issue `AR-244`, repo `2fa-demo` → branch `factory/ar-244-2fa-demo-abc12345` — `containsIssueKey` returns false and this new check falsely rejects a legitimate Factory branch, throwing in `prepare` and blocking dispatch for that issue. The numeric issue-key path avoids this because it uses an anchored `startsWith('factory/<key>-')`. Mirror that anchored check for the non-numeric path so the separator digit is irrelevant.</comment>
<file context>
@@ -319,6 +320,11 @@ const assertSafeWorktree = (worktree: AgentWorktree): void => {
if (target === base || !target.startsWith(`${expectedRoot}/`)) {
throw new Error(`Refusing unsafe Factory worktree path ${target}; expected a child of ${expectedRoot}`)
}
+ if (worktree.branch.startsWith('factory/') && !factoryBranchBelongsToIssue(worktree.branch, worktree.issueKey)) {
+ throw new Error(
+ `Refusing Factory worktree branch ${worktree.branch}: it does not belong to dispatched issue ${worktree.issueKey}`,
</file context>
| const matchesCurrentConvention = factoryBranchBelongsToIssue(expectedHeadRef, record.issue.key) | ||
| const matchesAuthorizedLegacyBranch = implementer.spec.existingPullRequestBranch === true && | ||
| /^\d+$/u.test(record.issue.key) && | ||
| expectedHeadRef.toLowerCase().startsWith(`${record.issue.key.toLowerCase()}-`) |
There was a problem hiding this comment.
P3: The publish legacy gate uses a loose startsWith(${record.issue.key.toLowerCase()}-), so malformed legacy branches such as 3021-, 3021- , or 3021--foo pass the ownership gate here, while the worktree safety layer (isAuthorizedExistingPrBranch) rejects exactly those forms with a strict ^<key>-[A-Za-z0-9][A-Za-z0-9._-]*$ regex. When an implementer goes through a worktree these gates disagree. Align the publish gate with the worktree regex so the two layers authorize the same set of legacy existing-PR branches.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/orchestrator/factory.ts, line 6346:
<comment>The publish legacy gate uses a loose `startsWith(`${record.issue.key.toLowerCase()}-`)`, so malformed legacy branches such as `3021-`, `3021- `, or `3021--foo` pass the ownership gate here, while the worktree safety layer (`isAuthorizedExistingPrBranch`) rejects exactly those forms with a strict `^<key>-[A-Za-z0-9][A-Za-z0-9._-]*$` regex. When an implementer goes through a worktree these gates disagree. Align the publish gate with the worktree regex so the two layers authorize the same set of legacy existing-PR branches.</comment>
<file context>
@@ -6333,9 +6336,29 @@ export class FactoryLoop implements Factory {
+ const matchesCurrentConvention = factoryBranchBelongsToIssue(expectedHeadRef, record.issue.key)
+ const matchesAuthorizedLegacyBranch = implementer.spec.existingPullRequestBranch === true &&
+ /^\d+$/u.test(record.issue.key) &&
+ expectedHeadRef.toLowerCase().startsWith(`${record.issue.key.toLowerCase()}-`)
+ if (!matchesCurrentConvention && !matchesAuthorizedLegacyBranch) {
+ throw new Error(
</file context>
Required: verify against the built CLI, not just unit testsDo not mark this done on green unit tests alone. Build this repo and exercise the real Critical context firstThe Factory daemon running in production is So before writing a fix, establish that the defect actually exists on current main: npm ci && npm run build
node bin/factory.mjs --help # sanity: built CLI runs
node -e "console.log(require('./package.json').version)" # expect 0.1.57If the defect does NOT reproduce on a build of current main, stop and say so on the issue. The correct fix is then "release and deploy 0.1.57", not a code change. Reporting that is a success, not a failure — do not invent a change to justify the dispatch. Reproduce → fix → re-verify# #244 — implementer must not push to another issue-s branch
node bin/factory.mjs dispatch <ISSUE_KEY> --config <path> --dry-runProve: with a branch for a different issue key already present, dispatch refuses it rather than pushing onto it. Reference case: Attach the actual terminal output for the failing run and the passing run. A diff, a type signature, or a green test is not evidence that the CLI behaves correctly. Notes
|
1 participant
Summary
cloud#3024 diagnosis
The conflict is an artifact of branch reuse. cloud#3024 contains only 2977c744 (3022 Chief org-chart work), while the real 3021 repair is 55f1a42d on cloud#3017, which merged before cloud#3024 opened. There is no unmerged 3021 change to re-cut; creating a replacement would be empty. This change does not delete or force-push factory/3022-chief-org-live-population.
Validation
Merge policy: never. Leave open for human review.
Fixes #244
Summary by cubic
Enforces issue-owned implementer branches and blocks PR publication if the head branch does not match the expected Factory branch. Prevents cross‑issue branch reuse and stale shared-checkout heads so PRs and receipts always belong to the dispatched issue. Fixes #244.
factory/<issueKey>-<repoSlug>-<8hex>, including non‑durable local fleets. Required: implementers must checkout or push to this exact branch before publishing.factory/branches that belong to a different issue. Required: delete/rename the branch or dispatch the matching issue.expectedHeadRefto PR publish calls and aborts before any push or PR creation if the local or remote head differs; applies to bothRelayfileGithubConnectionWriteandGhCliGithubWriteback. Required: remote implementers must push to the expected branch; local publishers must checkout the expected branch.factoryBranchBelongsToIssueanchors numeric keys (3021 cannot claim 30210) and supports alphanumeric keys (e.g., AR‑244); legacy<issue>-*heads are allowed only whenexistingPullRequestBranch: trueand the issue key is numeric.Written for commit eaa412e. Summary will update on new commits.