Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions src/git/agent-worktree.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,36 @@ describe('GitAgentWorktreeManager', () => {
}
})

it('refuses a pre-created branch owned by a different issue', async () => {
const root = await mkdtemp(join(tmpdir(), 'factory-agent-worktree-issue-collision-'))
const base = join(root, 'CloudCheckout')
try {
await mkdir(base)
await git(base, ['init', '-b', 'main'])
await git(base, ['config', 'user.email', 'factory@example.test'])
await git(base, ['config', 'user.name', 'Factory Test'])
await writeFile(join(base, 'README.md'), '# cloud\n', 'utf8')
await git(base, ['add', 'README.md'])
await git(base, ['commit', '-m', 'initial'])
await git(base, ['branch', 'factory/3022-chief-org-live-population'])

const manager = new GitAgentWorktreeManager()
const worktreePath = factoryWorktreePath(base, '3021', 'AgentWorkforce/cloud', 'collision')
await expect(manager.prepare({
repo: 'AgentWorkforce/cloud',
issueKey: '3021',
baseClonePath: base,
worktreePath,
branch: 'factory/3022-chief-org-live-population',
})).rejects.toThrow(
'Refusing Factory worktree branch factory/3022-chief-org-live-population: it does not belong to dispatched issue 3021',
)
await expect(stat(worktreePath)).rejects.toMatchObject({ code: 'ENOENT' })
} finally {
await rm(root, { recursive: true, force: true })
}
})

it('refuses cleanup outside the Factory worktree root', async () => {
const manager = new GitAgentWorktreeManager()
const unsafe = {
Expand Down
6 changes: 6 additions & 0 deletions src/git/agent-worktree.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { lstat, mkdir, readdir, realpath, rmdir, stat } from 'node:fs/promises'
import { basename, dirname, join, resolve } from 'node:path'
import { promisify } from 'node:util'

import { factoryBranchBelongsToIssue } from '../issue-key-match'
import type {
AgentWorktree,
AgentWorktreeCleanupInspection,
Expand Down Expand Up @@ -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)) {

Copy link
Copy Markdown

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, 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-abc12345containsIssueKey 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>

throw new Error(
`Refusing Factory worktree branch ${worktree.branch}: it does not belong to dispatched issue ${worktree.issueKey}`,
)
}
if (!worktree.branch.startsWith('factory/') && !isAuthorizedExistingPrBranch(worktree)) {
throw new Error(`Refusing unsafe Factory worktree branch ${worktree.branch}`)
}
Expand Down
10 changes: 9 additions & 1 deletion src/issue-key-match.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { describe, expect, it } from 'vitest'

import { containsExplicitIssueReference, containsIssueKey } from './issue-key-match'
import { containsExplicitIssueReference, containsIssueKey, factoryBranchBelongsToIssue } from './issue-key-match'

describe('issue key matching', () => {
it('matches dispatch branch conventions without numeric-prefix collisions', () => {
Expand All @@ -23,4 +23,12 @@ describe('issue key matching', () => {
expect(containsExplicitIssueReference('Issue: 52', '52')).toBe(true)
expect(containsExplicitIssueReference('https://github.com/AgentWorkforce/hoopsheet/issues/52', '52')).toBe(true)
})

it('matches Factory branches only to their dispatched issue key', () => {
expect(factoryBranchBelongsToIssue('factory/3021-cloud-deployment-fix', '3021')).toBe(true)
expect(factoryBranchBelongsToIssue('factory/3022-chief-org-live-population', '3021')).toBe(false)
expect(factoryBranchBelongsToIssue('factory/30210-not-3021', '3021')).toBe(false)
expect(factoryBranchBelongsToIssue('factory/ar-244-agentworkforce-factory', 'AR-244')).toBe(true)
expect(factoryBranchBelongsToIssue('feature/ar-244-agentworkforce-factory', 'AR-244')).toBe(false)
})
})
14 changes: 14 additions & 0 deletions src/issue-key-match.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Anchor alphanumeric issue keys after 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 👍 / 👎.

}
19 changes: 19 additions & 0 deletions src/mount/relayfile-github-connection-write.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,25 @@ describe('RelayfileGithubConnectionWrite', () => {
expect(mount.writes).toEqual([])
})

it('refuses a stale local branch before pushing or opening a pull request', async () => {
const mount = new FakeMountClient()
const git = gitRunnerForBranch('factory/3022-chief-org-live-population')
const write = new RelayfileGithubConnectionWrite({ mount, gitRunner: git })

await expect(write.publishPullRequest({
repo: 'AgentWorkforce/cloud',
clonePath: '/work/cloud',
expectedHeadRef: 'factory/3021-agentworkforce-cloud-12345678',
baseRef: 'main',
title: '3021: repair deployment objective CI',
body: 'Fixes #3021',
})).rejects.toThrow(
'Refusing to publish GitHub PR: expected head branch factory/3021-agentworkforce-cloud-12345678, found factory/3022-chief-org-live-population',
)
expect(git).toHaveBeenCalledTimes(1)
expect(mount.writes).toEqual([])
})

it('retries until the created pull request receipt is visible', async () => {
const draft = 'factory-fix-issue-52-1234567890ab'
const pullRequestPath = `/github/repos/AgentWorkforce/factory/pull-requests/${draft}.json`
Expand Down
11 changes: 8 additions & 3 deletions src/mount/relayfile-github-connection-write.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,15 +46,20 @@ export class RelayfileGithubConnectionWrite implements GithubConnectionWrite {
const headRef = input.headRef ?? (input.clonePath
? await this.#gitValue(['-C', input.clonePath, 'symbolic-ref', '--short', 'HEAD'], 'current branch')
: undefined)
const headSha = input.headSha ?? (input.clonePath && !input.headRef
? await this.#gitValue(['-C', input.clonePath, 'rev-parse', 'HEAD'], 'HEAD commit')
: undefined)
if (!headRef) {
throw new Error('GitHub PR publication requires headRef or clonePath')
}
if (input.expectedHeadRef && headRef !== input.expectedHeadRef) {
throw new Error(
`Refusing to publish GitHub PR: expected head branch ${input.expectedHeadRef}, found ${headRef}`,
)
}
if (headRef === input.baseRef) {
throw new Error(`Refusing to publish GitHub PR with head equal to base branch: ${headRef}`)
}
const headSha = input.headSha ?? (input.clonePath && !input.headRef
? await this.#gitValue(['-C', input.clonePath, 'rev-parse', 'HEAD'], 'HEAD commit')
: undefined)
const draftName = githubDraftName(headRef, headSha)
const repoRoot = `/github/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}`
const fullHeadRef = `refs/heads/${headRef}`
Expand Down
30 changes: 18 additions & 12 deletions src/orchestrator/factory.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,7 @@ class PublishingGithubWriteback extends RecordingGithubWriteback {
repo: input.repo,
number: this.receipt.number,
url: `https://github.com/${input.repo}/pull/${this.receipt.number}`,
headRef: input.headRef ?? `factory/${this.receipt.number}-user`,
headRef: input.headRef ?? input.expectedHeadRef ?? `factory/${this.receipt.number}-user`,
author: this.receipt.author,
}
}
Expand Down Expand Up @@ -10641,6 +10641,9 @@ describe('FactoryLoop', () => {
expect.stringMatching(/^factory:AR-14:/u),
'reviewer-invocation',
])
expect(fleet.spawns[0]?.task).toMatch(
/Create or reset the exact branch `factory\/ar-14-agentworkforce-pear-[0-9a-f]{8}`/u,
)
})

it('resumes exited open agents by sessionRef with the original capability', async () => {
Expand Down Expand Up @@ -10679,10 +10682,11 @@ describe('FactoryLoop', () => {
publishInputs.push(input)
const repoSlug = input.repo.split('/').at(-1)!
const prNumber = repoSlug === 'pear' ? 126 : 127
const headRef = input.headRef ?? input.expectedHeadRef!
mount.files.set(`/github/repos/${input.repo}/pulls/${prNumber}/metadata.json`, {
content: {
number: prNumber,
head_ref: `factory/${number}-${repoSlug}`,
head_ref: headRef,
url: `https://github.com/${input.repo}/pull/${prNumber}`,
state: 'open',
draft: false,
Expand All @@ -10692,7 +10696,7 @@ describe('FactoryLoop', () => {
repo: input.repo,
number: prNumber,
url: `https://github.com/${input.repo}/pull/${prNumber}`,
headRef: `factory/${number}-${repoSlug}`,
headRef,
}
},
closePullRequest: async () => undefined,
Expand Down Expand Up @@ -10772,7 +10776,7 @@ describe('FactoryLoop', () => {
repo: input.repo,
number: input.repo.endsWith('/pear') ? 128 : 129,
url: `https://github.com/${input.repo}/pull/${input.repo.endsWith('/pear') ? 128 : 129}`,
headRef: input.headRef ?? `factory/${number}-${input.repo.split('/').at(-1)}`,
headRef: input.headRef ?? input.expectedHeadRef!,
}
},
closePullRequest: async () => undefined,
Expand Down Expand Up @@ -10825,7 +10829,7 @@ describe('FactoryLoop', () => {
repo: input.repo,
number,
url: `https://github.com/${input.repo}/pull/${number}`,
headRef: `factory/${number}-app`,
headRef: input.headRef ?? input.expectedHeadRef!,
author: 'relayfile[bot]',
}
},
Expand Down Expand Up @@ -10891,7 +10895,7 @@ describe('FactoryLoop', () => {
repo: input.repo,
number: 52,
url: 'https://github.com/AgentWorkforce/pear/pull/52',
headRef: 'fix/ar-52',
headRef: input.headRef ?? input.expectedHeadRef!,
headSha: 'sha-52',
}
},
Expand Down Expand Up @@ -10953,6 +10957,7 @@ describe('FactoryLoop', () => {
expect(publishInputs).toEqual([{
repo: 'AgentWorkforce/pear',
clonePath: '/work/pear',
expectedHeadRef: expect.stringMatching(/^factory\/ar-52-agentworkforce-pear-[0-9a-f]{8}$/u),
baseRef: 'main',
title: 'AR-52: [factory-e2e] Fix factory issue 52',
body: expect.stringContaining('Live preview: https://factory-node.tailnet.ts.net:10052/'),
Expand Down Expand Up @@ -11048,7 +11053,7 @@ describe('FactoryLoop', () => {
repo: input.repo,
number: 54,
url: 'https://github.com/acme/pear/pull/54',
headRef: 'fix/ar-54',
headRef: input.headRef ?? input.expectedHeadRef!,
}
},
closePullRequest: async () => undefined,
Expand Down Expand Up @@ -11678,7 +11683,7 @@ describe('FactoryLoop', () => {
repo: input.repo,
number: 92,
url: 'https://github.com/AgentWorkforce/pear/pull/92',
headRef: 'ar-92-impl-pear',
headRef: input.headRef ?? input.expectedHeadRef!,
headSha: 'sha-92',
}
},
Expand Down Expand Up @@ -14338,7 +14343,7 @@ describe('FactoryLoop', () => {
repo: input.repo,
number: 158,
url: 'https://github.com/AgentWorkforce/pear/pull/158',
headRef: 'factory/58-agentworkforce-pear',
headRef: input.headRef ?? input.expectedHeadRef!,
headSha: 'sha-58',
}
},
Expand Down Expand Up @@ -17822,7 +17827,7 @@ describe('FactoryLoop PR babysitter', () => {
repo: input.repo,
number,
url: `https://github.com/${input.repo}/pull/${number}`,
headRef: `ar-${number}-impl-${input.repo.split('/').at(-1)}`,
headRef: input.headRef ?? input.expectedHeadRef!,
}
},
closePullRequest: async () => undefined,
Expand Down Expand Up @@ -18443,12 +18448,13 @@ describe('FactoryLoop PR babysitter', () => {
const githubWrite: GithubConnectionWrite = {
publishPullRequest: async (input) => {
publishInputs.push(input)
seedPrMeta(mount, input.repo, 402, { state: 'open', draft: false })
const headRef = input.headRef ?? input.expectedHeadRef!
seedPrMeta(mount, input.repo, 402, { state: 'open', draft: false, head_ref: headRef })
return {
repo: input.repo,
number: 402,
url: `https://github.com/${input.repo}/pull/402`,
headRef: 'factory/ar-402-agentworkforce-pear',
headRef,
}
},
closePullRequest: async () => undefined,
Expand Down
53 changes: 40 additions & 13 deletions src/orchestrator/factory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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()}-`)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

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
Expand All @@ -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)
Expand All @@ -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),
Expand All @@ -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
Expand Down Expand Up @@ -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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
Suggested change
invocationId: spec.invocationId ??
`factory:${decision.issue.key}:${runId}:${spec.role}:${sanitizeAgentSlug(spec.name)}`,
invocationId: `factory:${decision.issue.key}:${runId}:${spec.role}:${sanitizeAgentSlug(spec.name)}`,

}
return branch ? { ...lifecycleSpec, branch } : lifecycleSpec
}
Expand Down Expand Up @@ -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>-*`
Expand Down
2 changes: 2 additions & 0 deletions src/ports/mount.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ export interface GithubPublishPullRequestInput {
clonePath?: string
/** Exact branch already pushed by a remote implementer. Avoids reading its node-local clone. */
headRef?: string
/** Factory-derived branch that must be the PR head before any push or PR mutation occurs. */
expectedHeadRef?: string
headSha?: string
baseRef: string
title: string
Expand Down
Loading