Add factory orchestrator and live merge gate#235
Merged
Conversation
📝 WalkthroughWalkthroughThis PR introduces a complete factory orchestration system with issue triage, agent dispatch, recovery workflows, and GitHub merge-gate evaluation. It extends fleet client capabilities for per-agent resumption, refactors GitHub PR data parsing with new metadata fields, and reorganizes the public API as barrel exports. ChangesFleet Resume Capability Enhancement
GitHub Merge Gate Implementation
Batch Tracking Infrastructure
GitHub PR Data Schema and Parsing
Factory Orchestrator Loop
Public API Consolidation
Sequence Diagram(s)sequenceDiagram
participant Client
participant FactoryLoop
participant LinearMount
participant TriageEngine
participant FleetClient
participant LinearWriteback
participant MergeGate
Client->>FactoryLoop: start()
FactoryLoop->>LinearMount: mount issue root
FactoryLoop->>LinearMount: subscribe to ready issues
LinearMount->>FactoryLoop: issue-ready event
FactoryLoop->>FactoryLoop: readIssue(path)
FactoryLoop->>TriageEngine: triageIssue(issue)
TriageEngine->>FactoryLoop: TriageDecision
FactoryLoop->>FactoryLoop: dispatch(decision)
FactoryLoop->>FleetClient: spawn(implementer)
FactoryLoop->>FleetClient: spawn(reviewer)
FactoryLoop->>LinearWriteback: set state to "agent implementing"
FactoryLoop->>FactoryLoop: emit writeback-verified
FleetClient->>FactoryLoop: agent-exit event
FactoryLoop->>FleetClient: resume(capability, sessionRef)
FactoryLoop->>FactoryLoop: handleAgentExit
FleetClient->>FactoryLoop: agent-exit with completion
FactoryLoop->>FactoryLoop: completeIssue
FactoryLoop->>LinearWriteback: set state to "done"
FactoryLoop->>MergeGate: check() for PR merge readiness
MergeGate->>FactoryLoop: GithubMergeGateVerdict
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
Contributor
|
Implemented two PR-scoped fixes:
Added regressions for both:
Addressed comments
VerificationPassed locally with final edits in place:
I did not run the macOS |
kjgbot
force-pushed
the
factory-sdk/w6-orchestrator
branch
from
1ada2b9 to
96363e8
Compare
Contributor
|
Implemented one PR-scoped fix in the GitHub merge gate: it no longer treats a check run with only Changed:
Addressed comments
Advisory NotesNone. VerificationLocal verification passed:
I did not run the macOS-only packaged smoke job ( |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Contributor
|
Implemented one PR-scoped fix: the GitHub merge gate no longer treats a bare Changed: Addressed comments
Advisory NotesNone. VerificationPassed locally with final edits:
Remote GitHub state prevents |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/factory-sdk/src/writeback/writeback.test.ts (1)
187-218: ⚡ Quick winAdd a
payload.user.logincompatibility test case.This test currently validates only the
payload.authorpath. Add one case withpayload.user: { login: ... }to lock in backward-compatible author extraction.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/factory-sdk/src/writeback/writeback.test.ts` around lines 187 - 218, Update the test that uses FakeMountClient + MountGithubRead to also cover the legacy payload.user.login shape: add a second mounted record (or extend the existing fixture) for '/github/repos/AgentWorkforce__cloud/pulls/by-id/2086.json' (or a new pull id) whose payload includes user: { login: 'legacy-bot' } and assert via github.getPr(...) that the returned author is 'legacy-bot' (similar to the existing assertion for payload.author). Ensure the test still asserts the original payload.author case so both paths (payload.author and payload.user.login) are validated by the getPr implementation.packages/factory-sdk/src/orchestrator/factory.ts (1)
67-67: ⚡ Quick winRemove wasteful GithubRead instantiation.
This line creates a
MountGithubReadinstance via the nullish coalescing operator but immediately discards it withvoid. If the intent is to ensure the mount is configured, this side effect is not clear and wastes resources. IfGithubReadis not needed byFactoryLoop, remove this line entirely; otherwise, store the result in#githuband use it.♻️ Proposed fix to remove wasteful instantiation
- void (ports.github ?? MountGithubRead(ports.mount))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/factory-sdk/src/orchestrator/factory.ts` at line 67, The line using void (ports.github ?? MountGithubRead(ports.mount)) causes a throwaway instantiation of MountGithubRead; either remove the line entirely if FactoryLoop doesn't require GithubRead, or assign and retain the instance (e.g., store MountGithubRead(ports.mount) into the orchestrator's github field like `#github` or into ports.github) and ensure FactoryLoop uses that stored reference; locate the expression involving ports.github and MountGithubRead in factory.ts and replace the void usage with a proper assignment or delete it depending on whether GithubRead is required.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/factory-sdk/src/github/merge-gate.ts`:
- Around line 126-128: The gh subprocess call in defaultGhRunner lacks a timeout
and can hang; update the execFileAsync invocation inside defaultGhRunner to
include a timeout option (e.g. { timeout: 30_000, maxBuffer: 1024 * 1024 }) or
wire in a configurable constant (e.g. DEFAULT_GH_TIMEOUT) and pass it as {
timeout: DEFAULT_GH_TIMEOUT, maxBuffer: ... } so the child process is killed
after the timeout; keep the returned shape ({ stdout, stderr }) unchanged and
surface the timeout error as-is.
In `@packages/factory-sdk/src/orchestrator/factory.ts`:
- Line 487: `#completeIssue` currently ends with a no-op "void this.#mergeGate" so
the merge gate is never executed; replace that with a call to the merge gate's
check method (this.#mergeGate.check({...})) using a GithubMergeGateInput object
and ensure the check receives { repo, number, expectedHeadSha } (plumb these
into `#completeIssue`'s parameters or derive them before the call). Update any
callers of `#completeIssue` to pass the repo, number and expectedHeadSha values if
needed, and handle the returned Promise (await or explicitly ignore with void)
so GithubMergeGate.check(...) is actually invoked.
In `@packages/factory-sdk/src/writeback/github.ts`:
- Line 34: The current assignment author: refName(payload.author) ??
stringValue(payload.user) can drop author when payload.user is an object (e.g. {
login: ... }); update it to preserve author by checking for payload.user.login
when payload.user is an object. Replace the fallback with something like:
refName(payload.author) ?? (typeof payload.user === "string" ?
stringValue(payload.user) : stringValue(payload.user?.login)), using the
existing refName and stringValue helpers so both string and object-shaped
payload.user are handled.
---
Nitpick comments:
In `@packages/factory-sdk/src/orchestrator/factory.ts`:
- Line 67: The line using void (ports.github ?? MountGithubRead(ports.mount))
causes a throwaway instantiation of MountGithubRead; either remove the line
entirely if FactoryLoop doesn't require GithubRead, or assign and retain the
instance (e.g., store MountGithubRead(ports.mount) into the orchestrator's
github field like `#github` or into ports.github) and ensure FactoryLoop uses that
stored reference; locate the expression involving ports.github and
MountGithubRead in factory.ts and replace the void usage with a proper
assignment or delete it depending on whether GithubRead is required.
In `@packages/factory-sdk/src/writeback/writeback.test.ts`:
- Around line 187-218: Update the test that uses FakeMountClient +
MountGithubRead to also cover the legacy payload.user.login shape: add a second
mounted record (or extend the existing fixture) for
'/github/repos/AgentWorkforce__cloud/pulls/by-id/2086.json' (or a new pull id)
whose payload includes user: { login: 'legacy-bot' } and assert via
github.getPr(...) that the returned author is 'legacy-bot' (similar to the
existing assertion for payload.author). Ensure the test still asserts the
original payload.author case so both paths (payload.author and
payload.user.login) are validated by the getPr implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: aad4f457-34a4-4430-86d6-441100dee24f
📒 Files selected for processing (16)
packages/factory-sdk/src/fleet/internal-fleet-client.test.tspackages/factory-sdk/src/fleet/internal-fleet-client.tspackages/factory-sdk/src/fleet/relay-fleet-client.tspackages/factory-sdk/src/github/index.tspackages/factory-sdk/src/github/merge-gate.test.tspackages/factory-sdk/src/github/merge-gate.tspackages/factory-sdk/src/index.tspackages/factory-sdk/src/orchestrator/batch-tracker.tspackages/factory-sdk/src/orchestrator/factory.test.tspackages/factory-sdk/src/orchestrator/factory.tspackages/factory-sdk/src/orchestrator/index.tspackages/factory-sdk/src/ports/fleet.tspackages/factory-sdk/src/testing/fakes.tspackages/factory-sdk/src/types.tspackages/factory-sdk/src/writeback/github.tspackages/factory-sdk/src/writeback/writeback.test.ts
Comment on lines
+126
to
+128
| const defaultGhRunner: GhRunner = async (args) => { | ||
| const { stdout, stderr } = await execFileAsync('gh', args, { maxBuffer: 1024 * 1024 }) | ||
| return { stdout, stderr } |
There was a problem hiding this comment.
Add a timeout to the gh subprocess call.
Line 127 runs an external CLI without a timeout, so a stuck gh process can block merge-gate evaluation indefinitely and stall the loop.
Suggested fix
const defaultGhRunner: GhRunner = async (args) => {
- const { stdout, stderr } = await execFileAsync('gh', args, { maxBuffer: 1024 * 1024 })
+ const { stdout, stderr } = await execFileAsync('gh', args, {
+ maxBuffer: 1024 * 1024,
+ timeout: 30_000,
+ })
return { stdout, stderr }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const defaultGhRunner: GhRunner = async (args) => { | |
| const { stdout, stderr } = await execFileAsync('gh', args, { maxBuffer: 1024 * 1024 }) | |
| return { stdout, stderr } | |
| const defaultGhRunner: GhRunner = async (args) => { | |
| const { stdout, stderr } = await execFileAsync('gh', args, { | |
| maxBuffer: 1024 * 1024, | |
| timeout: 30_000, | |
| }) | |
| return { stdout, stderr } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/factory-sdk/src/github/merge-gate.ts` around lines 126 - 128, The gh
subprocess call in defaultGhRunner lacks a timeout and can hang; update the
execFileAsync invocation inside defaultGhRunner to include a timeout option
(e.g. { timeout: 30_000, maxBuffer: 1024 * 1024 }) or wire in a configurable
constant (e.g. DEFAULT_GH_TIMEOUT) and pass it as { timeout: DEFAULT_GH_TIMEOUT,
maxBuffer: ... } so the child process is killed after the timeout; keep the
returned shape ({ stdout, stderr }) unchanged and surface the timeout error
as-is.
| }) | ||
| await this.#slack.reply(root.threadId, `${record.issue.key}: Linear state set to done.`) | ||
| } | ||
| void this.#mergeGate |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for merge-gate invocations to confirm the bug scope.
# Search for calls to mergeGate.check or mergeGate methods
rg -nP --type=ts '\bmergeGate\s*\.\s*check\s*\(' packages/factory-sdk/src/orchestrator/
# Also check for any usage of the mergeGate field beyond the void statement
rg -nP --type=ts -C2 '\bthis\.#mergeGate\b' packages/factory-sdk/src/orchestrator/factory.tsRepository: AgentWorkforce/pear
Length of output: 598
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Find GithubMergeGate implementation =="
rg -n --hidden --no-ignore-vcs --type=ts 'class\s+GithubMergeGate\b|interface\s+GithubMergeGateInput\b|type\s+GithubMergeGateInput\b' .
echo
echo "== Find calls to any merge-gate methods =="
rg -n --hidden --no-ignore-vcs --type=ts '\bmergeGate\s*.\s*[A-Za-z0-9_]+\s*\(' .
echo
echo "== Find any usage of the private field itself =="
rg -n --hidden --no-ignore-vcs --type=ts '\bthis\.#mergeGate\b' .
echo
echo "== Look for void this.#mergeGate =="
rg -n --hidden --no-ignore-vcs --type=ts 'void\s+this\.#mergeGate\b' packages/factory-sdk/src/orchestrator/factory.ts
echo
echo "== Check what check(...) actually is on GithubMergeGate (if found) =="
# Best-effort: show method definitions around "check"
rg -n --hidden --no-ignore-vcs --type=ts 'GithubMergeGate[\s\S]{0,200}\bcheck\s*\(' -S . || trueRepository: AgentWorkforce/pear
Length of output: 240
🏁 Script executed:
#!/bin/bash
set -u
# Do not use `set -e` so "no matches" doesn't abort the script.
echo "== Find GithubMergeGate implementation (and input type) =="
rg -n --hidden --no-ignore-vcs --type=ts 'class\s+GithubMergeGate\b|export\s+interface\s+GithubMergeGateInput\b|export\s+class\s+GithubMergeGate\b' packages/factory-sdk/src
echo
echo "== Show GithubMergeGateInput definition =="
sed -n '1,200p' packages/factory-sdk/src/github/merge-gate.ts
echo
echo
echo "== Find calls to any merge-gate methods =="
rg -n --hidden --no-ignore-vcs --type=ts '\bmergeGate\s*.\s*[A-Za-z0-9_]+\s*\(' packages/factory-sdk/src || true
echo
echo "== Find any usage of the private field itself =="
rg -n --hidden --no-ignore-vcs --type=ts '\bthis\.#mergeGate\b' packages/factory-sdk/src || true
echo
echo "== Look for void this.#mergeGate =="
rg -n --hidden --no-ignore-vcs --type=ts 'void\s+this\.#mergeGate\b' packages/factory-sdk/src/orchestrator/factory.ts || true
echo
echo "== Find any `.check(` usage on GithubMergeGate (best-effort) =="
rg -n --hidden --no-ignore-vcs --type=ts '\bGithubMergeGate\b|\.check\s*\(' packages/factory-sdk/src || trueRepository: AgentWorkforce/pear
Length of output: 7349
🏁 Script executed:
#!/bin/bash
set -u
echo "== Context around void this.#mergeGate in factory.ts =="
sed -n '430,520p' packages/factory-sdk/src/orchestrator/factory.ts
echo
echo "== Search for any mergeGate usage elsewhere in packages/factory-sdk/src =="
rg -n --hidden --no-ignore-vcs --type=ts '\bmergeGate\b' packages/factory-sdk/srcRepository: AgentWorkforce/pear
Length of output: 3703
🏁 Script executed:
#!/bin/bash
set -u
echo "== Search for expectedHeadSha usage =="
rg -n --hidden --no-ignore-vcs --type=ts '\bexpectedHeadSha\b' packages/factory-sdk/src || true
echo
echo "== Search for head sha / headRefOid / expected head in factory.ts =="
rg -n --hidden --no-ignore-vcs --type=ts '\b(headRefOid|headSha|head\s*sha|expected\s*head)\b' packages/factory-sdk/src/orchestrator/factory.ts || true
echo
echo "== Inspect factory.ts for `#completeIssue-related` data fetching =="
rg -n --hidden --no-ignore-vcs --type=ts '`#completeIssue`\(|`#readIssue`\(|mergePolicy|merge' packages/factory-sdk/src/orchestrator/factory.ts -nRepository: AgentWorkforce/pear
Length of output: 1436
🏁 Script executed:
#!/bin/bash
set -u
echo "== Find InFlightIssue type in factory.ts =="
rg -n --hidden --no-ignore-vcs --type=ts 'type\s+InFlightIssue\b|interface\s+InFlightIssue\b|class\s+InFlightIssue\b' packages/factory-sdk/src/orchestrator/factory.ts
echo
echo "== Show top portion of factory.ts (imports/types) =="
sed -n '1,220p' packages/factory-sdk/src/orchestrator/factory.ts
echo
echo
echo "== Show InFlightIssue and related record type usage around `#completeIssue` call site =="
sed -n '220,420p' packages/factory-sdk/src/orchestrator/factory.tsRepository: AgentWorkforce/pear
Length of output: 14043
🏁 Script executed:
#!/bin/bash
set -u
echo "== Locate BatchTracker / InFlightIssue definitions =="
rg -n --hidden --no-ignore-vcs --type=ts 'export\s+type\s+InFlightIssue\b|export\s+interface\s+InFlightIssue\b|export\s+type\s+.*InFlightIssue\b|type\s+InFlightIssue\b' packages/factory-sdk/src/orchestrator
echo
echo "== Show batch-tracker.ts (first 260 lines) =="
wc -l packages/factory-sdk/src/orchestrator/batch-tracker.ts
sed -n '1,260p' packages/factory-sdk/src/orchestrator/batch-tracker.ts
echo
echo "== Search in batch-tracker.ts for GitHub PR metadata =="
rg -n --hidden --no-ignore-vcs --type=ts '\brepo\b|\bnumber\b|\bexpectedHeadSha\b|\bhead\b|GitHub' packages/factory-sdk/src/orchestrator/batch-tracker.tsRepository: AgentWorkforce/pear
Length of output: 5166
🏁 Script executed:
#!/bin/bash
set -u
echo "== Inspect IssueRef and related types =="
rg -n --hidden --no-ignore-vcs --type=ts 'export\s+interface\s+IssueRef|export\s+type\s+IssueRef|interface\s+IssueRef|type\s+IssueRef' packages/factory-sdk/src/types.ts
sed -n '1,220p' packages/factory-sdk/src/types.ts
echo
echo "== Find fields repo/number/headSha in types.ts =="
rg -n --hidden --no-ignore-vcs --type=ts '\b(repo|number|headSha|headRef|expectedHeadSha|pr)\b' packages/factory-sdk/src/types.ts
echo
echo "== Inspect TriageDecision type =="
rg -n --hidden --no-ignore-vcs --type=ts 'export\s+interface\s+TriageDecision|export\s+type\s+TriageDecision|interface\s+TriageDecision|type\s+TriageDecision' packages/factory-sdk/src/types.ts
rg -n --hidden --no-ignore-vcs --type=ts 'export\s+type\s+TriageDecision|TriageDecision\s*=' packages/factory-sdk/src/types.ts -n
sed -n '220,420p' packages/factory-sdk/src/types.tsRepository: AgentWorkforce/pear
Length of output: 3362
🏁 Script executed:
#!/bin/bash
set -u
echo "== Search for parseLinearIssue implementation =="
rg -n --hidden --no-ignore-vcs --type=ts 'function\s+parseLinearIssue\b|parseLinearIssue\s*\(' packages/factory-sdk/src
echo
echo "== Search for linear issue raw GitHub/PR metadata keys =="
# Look for likely fields in JSON/raw usage.
rg -n --hidden --no-ignore-vcs --type=ts '\b(pr|pullRequest|merge|mergeable|headSha|headRef|expectedHeadSha|baseRef|repo|number)\b' packages/factory-sdk/src/orchestrator packages/factory-sdk/src/triage packages/factory-sdk/src/writeback packages/factory-sdk/src || true
echo
echo "== Search for mergePolicy usage across the SDK =="
rg -n --hidden --no-ignore-vcs --type=ts '\bmergePolicy\b' packages/factory-sdk/src || true
echo
echo "== Search for GithubMergeGateInput assembly =="
rg -n --hidden --no-ignore-vcs --type=ts 'GithubMergeGateInput|expectedHeadSha|merge-gate' packages/factory-sdk/src || trueRepository: AgentWorkforce/pear
Length of output: 38912
🏁 Script executed:
#!/bin/bash
set -u
rg -n --hidden --no-ignore-vcs --type=ts '\bmergeGate\b' . || true
rg -n --hidden --no-ignore-vcs --type=ts '\bmergeGate\s*.\s*check\s*\(' . || true
rg -n --hidden --no-ignore-vcs --type=ts '\bthis\.#mergeGate\b' . || trueRepository: AgentWorkforce/pear
Length of output: 627
🏁 Script executed:
#!/bin/bash
set -u
echo "== Show parseLinearIssue implementation =="
sed -n '500,720p' packages/factory-sdk/src/orchestrator/factory.ts
echo
echo "== Inspect raw usage of PR metadata around writeback-ready paths =="
rg -n --hidden --no-ignore-vcs --type=ts '\braw\b' packages/factory-sdk/src/orchestrator/factory.ts
echo
echo "== Show LinearIssue parsing test expectations (if any mention PR fields) =="
rg -n --hidden --no-ignore-vcs --type=ts 'parseLinearIssue|LinearIssue|raw|pr|number|head|merge' packages/factory-sdk/src/orchestrator/factory.test.tsRepository: AgentWorkforce/pear
Length of output: 5732
Critical: merge gate is never executed (packages/factory-sdk/src/orchestrator/factory.ts:487)
#completeIssue ends with void this.#mergeGate, which is a no-op and never calls GithubMergeGate.check(...). A repo-wide search finds no mergeGate.check( usage anywhere, so merge-readiness is never evaluated. Replace the no-op with this.#mergeGate.check({ repo, number, expectedHeadSha }) (i.e., GithubMergeGateInput) and plumb those values into #completeIssue.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/factory-sdk/src/orchestrator/factory.ts` at line 487, `#completeIssue`
currently ends with a no-op "void this.#mergeGate" so the merge gate is never
executed; replace that with a call to the merge gate's check method
(this.#mergeGate.check({...})) using a GithubMergeGateInput object and ensure
the check receives { repo, number, expectedHeadSha } (plumb these into
`#completeIssue`'s parameters or derive them before the call). Update any callers
of `#completeIssue` to pass the repo, number and expectedHeadSha values if needed,
and handle the returned Promise (await or explicitly ignore with void) so
GithubMergeGate.check(...) is actually invoked.
| state: typeof payload.state === 'string' ? payload.state : undefined, | ||
| headRef: refName(payload.headRef) ?? refName(payload.head) ?? stringValue(payload.head_ref), | ||
| baseRef: refName(payload.baseRef) ?? refName(payload.base) ?? stringValue(payload.base_ref), | ||
| author: refName(payload.author) ?? stringValue(payload.user), |
There was a problem hiding this comment.
Preserve author when payload uses user.login object shape.
Line 34 can lose author metadata when payload.user is an object (e.g. { login: ... }), because the fallback only accepts a string.
Suggested fix
- author: refName(payload.author) ?? stringValue(payload.user),
+ author: refName(payload.author) ?? refName(payload.user) ?? stringValue(payload.user),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| author: refName(payload.author) ?? stringValue(payload.user), | |
| author: refName(payload.author) ?? refName(payload.user) ?? stringValue(payload.user), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/factory-sdk/src/writeback/github.ts` at line 34, The current
assignment author: refName(payload.author) ?? stringValue(payload.user) can drop
author when payload.user is an object (e.g. { login: ... }); update it to
preserve author by checking for payload.user.login when payload.user is an
object. Replace the fallback with something like: refName(payload.author) ??
(typeof payload.user === "string" ? stringValue(payload.user) :
stringValue(payload.user?.login)), using the existing refName and stringValue
helpers so both string and object-shaped payload.user are handled.
Contributor
|
Fixed one PR-scoped issue in the new GitHub merge gate: Addressed comments
Changes
Local validation
I did not check live GitHub mergeability or cloud CI state because the task prohibited |
This was referenced Jun 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
packages/factory-sdk/src/orchestrator/withcreateFactory,FactoryLoop, andBatchTracker.packages/factory-sdk/src/github/merge-gate.tslivegh pr viewmerge gate. READY is returned only for positively observedMERGEABLE + CLEAN + expected head SHA + all checks SUCCESS; all uncertainty fails closed.MountGithubRead.getPrto static mount metadata only and unwrap{ provider, payload }; mount state remains advisory and no longer surfaces status/checks/mergeable.1440608and patchedInternalFleetClient.resumeto honorinput.capabilityfor W6 reviewer resumes (// followups [fleet→W6]).Verification
npx vitest run packages/factory-sdknpx tsc --noEmit -p tsconfig.node.jsonNon-vacuity proof points for review
packages/factory-sdk/src/orchestrator/factory.test.ts—it('start backfills ready issues and dispatches when capacity is available', ...). Reverting#handleChangeto queue-only/no-op when capacity is free makes this fail.packages/factory-sdk/src/orchestrator/factory.test.ts—it('start queues and emits issue-queued when backfill exceeds batch capacity', ...). Reverting the full-batch queue branch or event emit makes this fail.packages/factory-sdk/src/orchestrator/factory.test.ts—it('coalesces concurrent starts into one subscription and dispatch pass', ...). Removing#startingmakes concurrentstart()calls create multiple subscriptions or dispatch passes.packages/factory-sdk/src/orchestrator/factory.test.ts—it('dedupes duplicate subscribe events for an already tracked issue', ...). Removing the tracked issue guard causes duplicate subscribe callbacks to double-dispatch.packages/factory-sdk/src/orchestrator/factory.test.ts—it('BatchTracker blocks duplicate invocation ids within and across issue records', ...). The same-record assertions pinrecord.invocationIds; the cross-record assertion pins the global#invocationIdsset. RevertingshouldSpawntotrueor removing the global set makes this fail.packages/factory-sdk/src/orchestrator/factory.test.ts—it('dedupes dispatch spawns that retry the same invocation id under different agent names', ...). This reaches#spawnAgentwithout therecord.resultshort-circuit by using two different agent names with the same invocationId; reverting the invocation guard produces an extra spawn and fails.packages/factory-sdk/src/orchestrator/factory.test.ts—it('dedupes repeated dispatch by stable invocation id', ...). This remains a higher-level regression for retrying the same decision after dispatch result capture.packages/factory-sdk/src/orchestrator/factory.test.ts—it('resumes exited open agents by sessionRef with the original capability', ...). Reverting resume to fresh spawn, omittingsessionRef, or omitting per-agentcapabilitymakes the test fail.packages/factory-sdk/src/orchestrator/factory.test.ts—it('coalesces duplicate exit callbacks for the same open issue, agent, and sessionRef', ...). Reverting the issue+agent+sessionRef resume dedup makes duplicate exit callbacks double-resume and the test fails.packages/factory-sdk/src/orchestrator/factory.test.ts—it('fresh-spawns on exit only when sessionRef is absent', ...). Removing the explicit fallback or trying to resume without a sessionRef makes this fail.packages/factory-sdk/src/fleet/internal-fleet-client.test.ts—it('resumes with the per-agent capability when provided', ...). Revertingcli: capabilityCli[input.capability ?? this.#resumeCapability]back to the fixed resume capability makes reviewer resume usecodexand this fails.packages/factory-sdk/src/orchestrator/factory.test.ts—it('emits an escalation on delivery_failed for an in-flight agent', ...). Removing theonDeliveryFailedhandling makes the error/escalation assertion fail.packages/factory-sdk/src/orchestrator/factory.test.ts—it('emits error and rejects when writeback verification fails', ...). SwallowingconfirmWritefailure or skipping error emit makes this fail.packages/factory-sdk/src/github/merge-gate.test.ts—it('refuses stale mount-clean snapshots when live GitHub contradicts readiness', ...). Defaulting open from stale mount-like data makes this fail.packages/factory-sdk/src/github/merge-gate.test.ts—it('refuses when the live head differs from the expected head sha', ...). Reverting only theheadRefOid !== expectedHeadShaguard makes this fail.packages/factory-sdk/src/github/merge-gate.test.ts—it('returns READY for MERGEABLE+CLEAN with neutral, skipped, or expected advisory checks', ...). This provesNEUTRAL,SKIPPED, andEXPECTEDare non-blocking underMERGEABLE+CLEANwith matching head.packages/factory-sdk/src/github/merge-gate.test.ts—it('refuses missing, blocking, pending, or unknown status checks', ...). This provesFAILURE, pending/in-progress,UNKNOWN, empty rollups, and bare status without an allowed conclusion/state refuse.packages/factory-sdk/src/github/merge-gate.test.ts—it('fails closed when gh returns UNKNOWN, errors, or partial output', ...). TreatingUNKNOWN, runner errors, empty/partial JSON, or missing fields as ready makes this fail.Notes
FleetClientonly; no concrete fleet implementation is imported by the orchestrator.it('runOnce caps active issues, skips stale state, and pulls queued work after completion', ...)dispatches two issues at batch size 2, queues the third, and pulls it when one issue completes.FleetClient.onAgentExit(listener: (name: string, reason?: string) => void)shape. The implementation intentionally does not key onreason, because W5 can emit duplicate callbacks from typed and broker-event paths with different reason strings.mergePolicyremainsneverby default.