-
Notifications
You must be signed in to change notification settings - Fork 0
244: Enforce issue-owned implementer branches #247
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,3 +37,17 @@ export const containsExplicitIssueReference = (value: string, issueKey: string): | |
| const issue = `${prefix}-${number}(?=$|[^A-Za-z0-9-]|-(?!\\d))` | ||
| return new RegExp(`(^|\\n)\\s*(?:linear|issue|closes|fixes|resolves)\\b[^\\n]*${issue}`, 'i').test(value) | ||
| } | ||
|
|
||
| /** | ||
| * Factory-owned implementation branches always start with `factory/` and | ||
| * carry the dispatched issue key. Numeric GitHub issue keys need an anchored | ||
| * match so issue 3021 can never claim a branch owned by 3022 (or 30210). | ||
| */ | ||
| export const factoryBranchBelongsToIssue = (headRef: string, issueKey: string): boolean => { | ||
| const normalizedHead = headRef.trim().toLowerCase() | ||
| const normalizedKey = issueKey.trim().toLowerCase() | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
For Linear-style keys, this accepts the issue key anywhere in the branch rather than in the ownership position. For example, Useful? React with 👍 / 👎. |
||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -47,7 +47,7 @@ import type { Clock, Logger } from '../ports/system' | |||||||
| import type { AgentWorktree, AgentWorktreeManager, AgentWorktreeRepository } from '../ports/worktree' | ||||||||
| import { factoryWorktreeIssueSlug, factoryWorktreePath } from '../git/agent-worktree' | ||||||||
| import { InMemoryStateStore } from '../state/in-memory-state-store' | ||||||||
| import { containsExplicitIssueReference, containsIssueKey } from '../issue-key-match' | ||||||||
| import { containsExplicitIssueReference, containsIssueKey, factoryBranchBelongsToIssue } from '../issue-key-match' | ||||||||
| import { normalizeLogger, normalizeLogValue, setSafeErrorStack, stringifyLogValue } from '../logging' | ||||||||
| import { isInFactoryScope } from '../safety/factory-scope' | ||||||||
| import { dispatchRelayflowForChangeEvent } from '../dispatch/relayflow-registry' | ||||||||
|
|
@@ -2565,7 +2565,10 @@ export class FactoryLoop implements Factory { | |||||||
| // ones. Without it, every worker starts in the configured shared checkout | ||||||||
| // and concurrent issues can switch each other back to the base branch. | ||||||||
| const isolateLocalWorktree = this.#fleet.placementLocality === 'local' && Boolean(this.#worktrees) | ||||||||
| const lifecycleRunId = !dryRun && (durableDispatch || isolateLocalWorktree) ? randomUUID() : undefined | ||||||||
| // Every live dispatch gets an issue-owned branch, including legacy local | ||||||||
| // fleets without a durable lifecycle or worktree manager. The publication | ||||||||
| // boundary uses this exact ref to reject a stale shared checkout. | ||||||||
| const lifecycleRunId = !dryRun ? randomUUID() : undefined | ||||||||
| if (lifecycleRunId) { | ||||||||
| dispatchDecision = decisionWithLifecycleBranches(dispatchDecision, lifecycleRunId, { | ||||||||
| isolateLocalWorktree, | ||||||||
|
|
@@ -6333,9 +6336,29 @@ export class FactoryLoop implements Factory { | |||||||
| opts: { reconcileExisting?: boolean } = {}, | ||||||||
| ): Promise<GithubPublishPullRequestResult | undefined> { | ||||||||
| const key = `${issueKey(record.issue)}:${implementer.spec.repo}` | ||||||||
| const expectedHeadRef = implementer.spec.branch | ||||||||
| if (!expectedHeadRef) { | ||||||||
| throw new Error(`Refusing to publish ${record.issue.key}: implementer has no Factory-derived branch`) | ||||||||
| } | ||||||||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3: The publish legacy gate uses a loose Prompt for AI agents |
||||||||
| if (!matchesCurrentConvention && !matchesAuthorizedLegacyBranch) { | ||||||||
| throw new Error( | ||||||||
| `Refusing to publish ${record.issue.key}: branch ${expectedHeadRef} belongs to a different issue`, | ||||||||
| ) | ||||||||
| } | ||||||||
| const durable = await this.#state.getDispatchLifecycle(this.#workspaceId, issueKey(record.issue)) | ||||||||
| const cached = this.#publishedPullRequests.get(key) | ||||||||
| if (cached) return cached | ||||||||
| if (cached) { | ||||||||
| if (cached.headRef !== expectedHeadRef) { | ||||||||
| throw new Error( | ||||||||
| `Refusing cached PR for ${record.issue.key}: expected head branch ${expectedHeadRef}, found ${cached.headRef}`, | ||||||||
| ) | ||||||||
| } | ||||||||
| return cached | ||||||||
| } | ||||||||
|
|
||||||||
| const { identity, publisher } = this.#githubPullRequestPublisher() | ||||||||
| const remoteBranch = implementer.result?.locality === 'remote' && implementer.spec.branch | ||||||||
|
|
@@ -6359,12 +6382,15 @@ export class FactoryLoop implements Factory { | |||||||
| const durableReceipt = publishedPullRequests(durable).find((receipt) => | ||||||||
| receipt.repo.toLowerCase() === repo.toLowerCase() | ||||||||
| ) | ||||||||
| const expectedHeadRef = implementer.spec.branch ?? remoteBranch | ||||||||
| if ( | ||||||||
| durableReceipt && | ||||||||
| (!opts.reconcileExisting || !expectedHeadRef || durableReceipt.headRef === expectedHeadRef) | ||||||||
| ) return durableReceipt | ||||||||
| if (opts.reconcileExisting && expectedHeadRef) { | ||||||||
| if (durableReceipt) { | ||||||||
| if (durableReceipt.headRef !== expectedHeadRef) { | ||||||||
| throw new Error( | ||||||||
| `Refusing durable PR receipt for ${record.issue.key}: expected head branch ${expectedHeadRef}, found ${durableReceipt.headRef}`, | ||||||||
| ) | ||||||||
| } | ||||||||
| return durableReceipt | ||||||||
| } | ||||||||
| if (opts.reconcileExisting) { | ||||||||
| const existing = await this.#openPullRequestByHead(repo, expectedHeadRef) | ||||||||
| if (existing) { | ||||||||
| this.#publishedPullRequests.set(key, existing) | ||||||||
|
|
@@ -6382,6 +6408,7 @@ export class FactoryLoop implements Factory { | |||||||
| const result = await publisher.publishPullRequest({ | ||||||||
| repo, | ||||||||
| ...(remoteBranch ? { headRef: remoteBranch } : { clonePath: implementer.spec.clonePath }), | ||||||||
| expectedHeadRef, | ||||||||
| baseRef, | ||||||||
| title: `${issue.key}: ${issue.title}`, | ||||||||
| body: githubPullRequestBody(issue, implementer.spec.preview), | ||||||||
|
|
@@ -6392,7 +6419,7 @@ export class FactoryLoop implements Factory { | |||||||
| : { ...result, author: identity } | ||||||||
| if ( | ||||||||
| published.repo.toLowerCase() !== repo.toLowerCase() || | ||||||||
| published.headRef !== (remoteBranch ?? published.headRef) || | ||||||||
| published.headRef !== expectedHeadRef || | ||||||||
| !Number.isInteger(published.number) || | ||||||||
| published.number <= 0 || | ||||||||
| !published.url | ||||||||
|
|
@@ -14225,7 +14252,8 @@ function decisionWithLifecycleBranches( | |||||||
| ...(opts.isolateLocalWorktree && baseClonePath && branch ? { baseClonePath, clonePath } : {}), | ||||||||
| // 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)}`, | ||||||||
|
Comment on lines
+14255
to
+14256
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: When a caller or triage result supplies Prompt for AI agents
Suggested change
|
||||||||
| } | ||||||||
| return branch ? { ...lifecycleSpec, branch } : lifecycleSpec | ||||||||
| } | ||||||||
|
|
@@ -15052,8 +15080,7 @@ const hasTitlePrefix = (title: string, marker: string): boolean => | |||||||
|
|
||||||||
| const factoryBranchMatchesIssue = (headRef: string, issueKey: string): boolean => | ||||||||
| /^\d+$/u.test(issueKey) | ||||||||
| ? headRef.toLowerCase() === `factory/${issueKey.toLowerCase()}` || | ||||||||
| headRef.toLowerCase().startsWith(`factory/${issueKey.toLowerCase()}-`) | ||||||||
| ? factoryBranchBelongsToIssue(headRef, issueKey) | ||||||||
| : containsIssueKey(headRef, issueKey) | ||||||||
|
|
||||||||
| // Legacy Factory runs created GitHub-native branches as `<issue-number>-*` | ||||||||
|
|
||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: For non-numeric (Linear-style
XXX-N) issue keys,factoryBranchBelongsToIssuefalls back tocontainsIssueKey, whose terminal-boundary rule-(?!\d)fails whenever the separator following the number is followed by a digit. The worktree branch is always built asfactory/<issue>-<repo>-<runId>(seedecisionWithLifecycleBranchesin factory.ts), so if the repo slug begins with a digit — e.g. issueAR-244, repo2fa-demo→ branchfactory/ar-244-2fa-demo-abc12345—containsIssueKeyreturns false and this new check falsely rejects a legitimate Factory branch, throwing inprepareand blocking dispatch for that issue. The numeric issue-key path avoids this because it uses an anchoredstartsWith('factory/<key>-'). Mirror that anchored check for the non-numeric path so the separator digit is irrelevant.Prompt for AI agents