Uh oh!
There was an error while loading. Please reload this page.
feat(factory): route GitHub PR writes through the workspace connection (fixes #52) - #64
Conversation
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
📝 WalkthroughWalkthroughFactory now publishes and closes pull requests through a connected workspace GitHub write connection, backed by Relayfile storage. Orchestration, probe closure, mount permissions, tests, public exports, and setup documentation were updated accordingly. ChangesWorkspace GitHub writeback
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Implementer
participant Factory
participant GitHubWrite
participant RelayfileWorkspace
Implementer->>Factory: Complete implementation
Factory->>GitHubWrite: publishPullRequest(input)
GitHubWrite->>RelayfileWorkspace: Write ref and PR metadata
RelayfileWorkspace-->>GitHubWrite: Return acknowledged receipt
GitHubWrite-->>Factory: Return PR details
Factory->>Factory: Start or advance babysitter
Possibly related issues
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 |
There was a problem hiding this comment.
1 issue found across 22 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/github/merge-gate.ts">
<violation number="1" location="src/github/merge-gate.ts:176">
P2: The TODO correctly marks the merge-gate defaultGhRunner for retirement, but there's an identical duplicate of this runner in probe-closer.ts (line 101) that shares the same gh CLI call. When the merge-gate runner is retired, the probe-closer copy will remain as a hidden dependency on the gh binary. Consider importing the exported merge-gate defaultGhRunner from probe-closer.ts instead of duplicating it, or extend the TODO to track both copies.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| } | ||
| export const defaultGhRunner: GhRunner = async (args) => { | ||
| // TODO(issue-52): retire this compatibility runner when merge-gate reads and |
There was a problem hiding this comment.
P2: The TODO correctly marks the merge-gate defaultGhRunner for retirement, but there's an identical duplicate of this runner in probe-closer.ts (line 101) that shares the same gh CLI call. When the merge-gate runner is retired, the probe-closer copy will remain as a hidden dependency on the gh binary. Consider importing the exported merge-gate defaultGhRunner from probe-closer.ts instead of duplicating it, or extend the TODO to track both copies.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/github/merge-gate.ts, line 176:
<comment>The TODO correctly marks the merge-gate defaultGhRunner for retirement, but there's an identical duplicate of this runner in probe-closer.ts (line 101) that shares the same gh CLI call. When the merge-gate runner is retired, the probe-closer copy will remain as a hidden dependency on the gh binary. Consider importing the exported merge-gate defaultGhRunner from probe-closer.ts instead of duplicating it, or extend the TODO to track both copies.</comment>
<file context>
@@ -173,6 +173,8 @@ export function evaluateGithubMergeGate(
}
export const defaultGhRunner: GhRunner = async (args) => {
+ // TODO(issue-52): retire this compatibility runner when merge-gate reads and
+ // guarded merges are fully represented by the mounted GitHub connection.
const { stdout, stderr } = await execFileAsync('gh', args, { maxBuffer: 1024 * 1024 })
</file context>
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/orchestrator/factory.ts (1)
6134-6143: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the GitHub writeback allowlist regex to a shared module.
Both sites independently define the identical
isFactoryGithubWritebackPathregex and near-identicalisAllowedFactoryGithubDraft/inline guard logic that gate guarded writes throughRelayfileCloudMountClient. Since this predicate controls which paths are permitted for guarded GitHub writes (PR publish/close), letting the two copies drift would silently change write permissions in only one of the two call paths (daemon vs. standalone CLI close-probe).
src/orchestrator/factory.ts#L6134-L6143: keepisFactoryGithubWritebackPathhere (or move it) and have both files import from one location.src/cli/fleet.ts#L690-L704: import the sharedisFactoryGithubWritebackPath/predicate instead of redefining it locally.🤖 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 `@src/orchestrator/factory.ts` around lines 6134 - 6143, Extract the shared isFactoryGithubWritebackPath predicate from src/orchestrator/factory.ts lines 6134-6143 into a reusable module, then update the guard logic in src/orchestrator/factory.ts to use that shared export. In src/cli/fleet.ts lines 690-704, remove the local regex and duplicate predicate definition and import the shared isFactoryGithubWritebackPath instead, preserving the existing guarded GitHub write behavior in both call paths.
🤖 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 `@README.md`:
- Around line 58-63: Update the GitHub connection prerequisite text in the
README to state that local `gh` installation and authentication are unnecessary
for Factory’s branch publishing and pull-request creation, while clarifying that
probe and PR-resolution read paths still require the local CLI.
In `@src/orchestrator/factory.ts`:
- Around line 2497-2523: Update the error handling around
`#publishImplementerPullRequest` so babysitter-enabled runs do not return without
recovery when publishing fails. Preserve the existing immediate return for
non-babysitter mode, but in babysitter mode invoke the established bounded
recovery or escalation path for the affected record before exiting, ensuring the
issue is not left in-flight without retry handling.
- Around line 2613-2682: Update `#publishImplementerPullRequest` to deduplicate
concurrent publication attempts using an in-flight promise map, following the
established `#dispatchInFlight/`#resumeInFlight pattern. Register the key before
the first await, reuse the existing promise for concurrent callers, and clear
the in-flight entry when the operation settles while preserving
`#publishedPullRequests` for completed publications.
---
Nitpick comments:
In `@src/orchestrator/factory.ts`:
- Around line 6134-6143: Extract the shared isFactoryGithubWritebackPath
predicate from src/orchestrator/factory.ts lines 6134-6143 into a reusable
module, then update the guard logic in src/orchestrator/factory.ts to use that
shared export. In src/cli/fleet.ts lines 690-704, remove the local regex and
duplicate predicate definition and import the shared
isFactoryGithubWritebackPath instead, preserving the existing guarded GitHub
write behavior in both call paths.
🪄 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: abeb0e38-5e89-43ba-9469-3bda6d91a94b
📒 Files selected for processing (25)
README.mdfactory-build/README.mdfactory-build/lib/factory-build-lib.tsfactory-build/wave2-extraction/01-p4-extract-to-factory-repo.tssrc/__tests__/writefile-callsite-invariant.test.tssrc/cli/fleet.test.tssrc/cli/fleet.tssrc/config/schema.test.tssrc/config/schema.tssrc/dispatch/templates.test.tssrc/dispatch/templates.tssrc/github/merge-gate.tssrc/github/probe-closer.test.tssrc/github/probe-closer.tssrc/index.tssrc/mount/relayfile-cloud-mount-client.test.tssrc/mount/relayfile-cloud-mount-client.tssrc/mount/relayfile-github-connection-write.test.tssrc/mount/relayfile-github-connection-write.tssrc/orchestrator/factory.test.tssrc/orchestrator/factory.tssrc/ports/index.tssrc/ports/mount.tssrc/testing/fakes.tssrc/writeback/github.ts
| 1. **Connect GitHub to your relay workspace** with push access for the target | ||
| repositories. Factory uses that workspace connection to publish branches and | ||
| open pull requests; a local `gh` installation or `gh auth login` is not a | ||
| prerequisite. | ||
| 2. **Write a minimal config** (`factory.config.json`). Only `workspaceId` and a |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bashset -euo pipefail
printf'\n== README excerpt ==\n'
nl -ba README.md | sed -n '50,70p'printf'\n== Search for gh runner usage ==\n'
rg -n "defaultGhRunner|gh auth|gh " src README.md
printf'\n== Fleet file outline ==\n'
ast-grep outline src/cli/fleet.ts --view expandedRepository: AgentWorkforce/factory
Length of output: 222
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo'--- README lines 58-63 ---'
nl -ba README.md | sed -n '58,63p'echoecho'--- fleet.ts gh-related lines ---'
grep -nE 'defaultGhRunner|gh auth|gh ' -n src/cli/fleet.ts ||trueechoecho'--- surrounding fleet.ts context ---'
nl -ba src/cli/fleet.ts | sed -n '1,260p'Repository: AgentWorkforce/factory
Length of output: 227
🏁 Script executed:
#!/bin/bashset -euo pipefail
nl -ba README.md | sed -n '58,63p'Repository: AgentWorkforce/factory
Length of output: 200
🏁 Script executed:
#!/bin/bashset -euo pipefail
printf'\n== README excerpt ==\n'
sed -n '58,63p' README.md
printf'\n== gh-related search ==\n'
grep -RInE 'defaultGhRunner|gh auth|gh ' src README.md ||trueprintf'\n== fleet.ts excerpt ==\n'
sed -n '1,260p' src/cli/fleet.tsRepository: AgentWorkforce/factory
Length of output: 20444
Clarify the gh prerequisite scope. Workspace auth removes the need for local gh when Factory publishes branches and opens PRs, but probe/PR-resolution read paths still invoke the local CLI. Avoid saying it is categorically “not a prerequisite”; scope the claim to GitHub writes.
🤖 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 `@README.md` around lines 58 - 63, Update the GitHub connection prerequisite
text in the README to state that local `gh` installation and authentication are
unnecessary for Factory’s branch publishing and pull-request creation, while
clarifying that probe and PR-resolution read paths still require the local CLI.
| let publishedPr: GithubPublishPullRequestResult | undefined | ||
| if ( | ||
| exiting?.spec.role === 'implementer' && | ||
| !record.dryRun && | ||
| (this.#mount.githubWrite || this.#mount.writebackTransport === 'relayfile-cloud') | ||
| ) { | ||
| try { | ||
| publishedPr = await this.#publishImplementerPullRequest(record, exiting) | ||
| } catch (error) { | ||
| this.#increment('githubPullRequestPublishFailures') | ||
| this.#error(error, record.issue) | ||
| return | ||
| } | ||
| } | ||
| if (this.#config.babysitter.enabled && !record.dryRun) { | ||
| // Babysitter path: an implementer/reviewer finishing does NOT mark the | ||
| // issue done — it hands the open PR to the babysitter. The babysitter | ||
| // itself finishing means it believes the PR is ready, so re-check and | ||
| // advance to Human Review. | ||
| if (exiting?.spec.role === 'babysitter') { | ||
| await this.#maybeAdvanceToHumanReview(record) | ||
| } else if (publishedPr) { | ||
| await this.#ensureBabysitter(record, { | ||
| repo: publishedPr.repo, | ||
| prNumber: publishedPr.number, | ||
| url: publishedPr.url, | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bashset -euo pipefail
# Map relevant files/symbols first
git ls-files | rg '(^|/)(src/orchestrator/factory\.ts|src/.*babysitter.*|src/.*sweepPrStateCompletions.*|src/.*ensureBabysitter.*|src/.*completeIssue.*|src/.*escalateStalledIssue.*)$'||trueecho"---- outline: factory.ts ----"
ast-grep outline src/orchestrator/factory.ts --view expanded ||trueecho"---- search relevant symbols ----"
rg -n "sweepPrStateCompletions|ensureBabysitterForIssue|ensureBabysitter|escalateStalledIssue|completeIssue|publishImplementerPullRequest|babysitter.enabled|webhook-driven|load-bearing" src/orchestrator src ||trueRepository: AgentWorkforce/factory
Length of output: 37314
🏁 Script executed:
#!/bin/bashset -euo pipefail
# Read only the relevant slices around the cited lines and related methods.
sed -n '2460,2565p' src/orchestrator/factory.ts
printf'\n----\n'
sed -n '2565,2665p' src/orchestrator/factory.ts
printf'\n----\n'
sed -n '2665,2765p' src/orchestrator/factory.tsRepository: AgentWorkforce/factory
Length of output: 11971
🏁 Script executed:
#!/bin/bashset -euo pipefail
# Inspect the babysitter/no-op webhook path and issue completion/recovery helpers.
rg -n -A 6 -B 6 "sweepPrStateCompletions|ensureBabysitterForIssue|ensureBabysitter|escalateStalledIssue|completeIssue" src/orchestrator srcRepository: AgentWorkforce/factory
Length of output: 32719
🏁 Script executed:
#!/bin/bashset -euo pipefail
# Find and inspect the sweep path.
rg -n "sweepPrStateCompletions" src/orchestrator/factory.ts src ||trueprintf'\n----\n'
sed -n '1,220p' src/orchestrator/factory.ts | sed -n '/sweepPrStateCompletions/,+120p'Repository: AgentWorkforce/factory
Length of output: 994
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo"== factory.ts lines around the cited block =="
nl -ba src/orchestrator/factory.ts | sed -n '2470,2555p'echo"== sweepPrStateCompletions references =="
rg -n -A 8 -B 8 "sweepPrStateCompletions" src/orchestrator/factory.ts src ||trueecho"== babysitter-related helper references =="
rg -n -A 8 -B 8 "ensureBabysitterForIssue|escalateStalledIssue|maybeAdvanceToHumanReview|completeIssue|publishImplementerPullRequest" src/orchestrator/factory.ts src ||trueRepository: AgentWorkforce/factory
Length of output: 246
🏁 Script executed:
#!/bin/bashset -euo pipefail
python3 - <<'PY'from pathlib import Pathp = Path('src/orchestrator/factory.ts')text = p.read_text()for needle in [ 'sweepPrStateCompletions', 'ensureBabysitterForIssue', 'escalateStalledIssue', 'maybeAdvanceToHumanReview', 'completeIssue', 'publishImplementerPullRequest',]: print(f'== {needle} ==') idx = text.find(needle) if idx == -1: print('not found') continue start = max(0, idx - 900) end = min(len(text), idx + 1800) print(text[start:end]) print()PYRepository: AgentWorkforce/factory
Length of output: 16548
Handle PR publish failures in babysitter mode
If #publishImplementerPullRequest throws, this returns before any babysitter handoff or completion path runs. In babysitter mode the sweep is disabled, so the issue can stay in-flight with no retry or escalation. Keep the non-babysitter early return, but route this failure through a bounded recovery path.
🤖 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 `@src/orchestrator/factory.ts` around lines 2497 - 2523, Update the error
handling around `#publishImplementerPullRequest` so babysitter-enabled runs do not
return without recovery when publishing fails. Preserve the existing immediate
return for non-babysitter mode, but in babysitter mode invoke the established
bounded recovery or escalation path for the affected record before exiting,
ensuring the issue is not left in-flight without retry handling.
| async #publishImplementerPullRequest( | ||
| record: InFlightIssue, | ||
| implementer: TrackedAgent, | ||
| ): Promise<GithubPublishPullRequestResult | undefined> { | ||
| const key = `${issueKey(record.issue)}:${implementer.spec.repo}` | ||
| if (this.#publishedPullRequests.has(key)) return undefined | ||
| const githubWrite = this.#mount.githubWrite | ||
| if (!githubWrite) { | ||
| throw new Error('GitHub write path not available on this mount — connect GitHub to your workspace') | ||
| } | ||
| if (!implementer.spec.clonePath) { | ||
| throw new Error(`GitHub PR publication requires a configured clone path for ${implementer.spec.repo}`) | ||
| } | ||
| const issue = await this.#readIssue(record.issue.path) | ||
| if (!issue) { | ||
| throw new Error(`Unable to publish GitHub PR: issue ${record.issue.key} is no longer readable`) | ||
| } | ||
| const sourceRepo = githubMirrorRepoForIssue(issue) | ||
| const sourceRepoParts = sourceRepo ? githubRepoParts(sourceRepo) : undefined | ||
| const bareRepoName = implementer.spec.repo.includes('/') ? undefined : implementer.spec.repo | ||
| const sourceOwner = bareRepoName && sourceRepoParts?.repo === bareRepoName | ||
| ? sourceRepoParts.owner | ||
| : undefined | ||
| const repo = normalizeGithubRepo(implementer.spec.repo, this.#config.repos.org ?? sourceOwner) | ||
| const baseRef = await this.#githubDefaultBranch(repo) | ||
| const result = await githubWrite.publishPullRequest({ | ||
| repo, | ||
| clonePath: implementer.spec.clonePath, | ||
| baseRef, | ||
| title: `${issue.key}: ${issue.title}`, | ||
| body: githubPullRequestBody(issue), | ||
| }) | ||
| this.#publishedPullRequests.add(key) | ||
| this.#increment('githubPullRequestsPublished') | ||
| this.#logger.info?.('[factory] published PR through workspace GitHub connection', { | ||
| issue: issue.key, | ||
| repo: result.repo, | ||
| prNumber: result.number, | ||
| url: result.url, | ||
| }) | ||
| return result | ||
| } | ||
| async #githubDefaultBranch(repo: string): Promise<string> { | ||
| const parts = githubRepoParts(repo) | ||
| if (!parts) { | ||
| throw new Error(`GitHub repo must be owner/repo before resolving its default branch: ${repo}`) | ||
| } | ||
| const path = `/github/repos/${encodeURIComponent(parts.owner)}/${encodeURIComponent(parts.repo)}/meta.json` | ||
| let payload: Record<string, unknown> | ||
| try { | ||
| payload = wrappedPayload((await this.#mount.readFile(path)).content) | ||
| } catch (error) { | ||
| throw new Error(`Unable to resolve the default branch for ${repo} from ${path}: ${describeError(error).errorMessage}`) | ||
| } | ||
| const repository = asRecord(payload.repository) | ||
| const defaultBranch = ( | ||
| stringValue(payload.defaultBranch) ?? | ||
| stringValue(payload.default_branch) ?? | ||
| stringValue(repository?.defaultBranch) ?? | ||
| stringValue(repository?.default_branch) | ||
| )?.trim() | ||
| if (!defaultBranch) { | ||
| throw new Error(`GitHub repository metadata for ${repo} does not include a default branch`) | ||
| } | ||
| return defaultBranch | ||
| } | ||
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
printf'Files:\n'
git ls-files src/orchestrator/factory.ts templates.test.ts | sed 's#^`#-` #'printf'\nRelevant symbols:\n'
rg -n "`#publishedPullRequests`|`#dispatchInFlight`|`#resumeInFlight`|publishImplementerPullRequest|publishPullRequest\\(" src/orchestrator/factory.ts templates.test.ts
printf'\nFactory outline:\n'
ast-grep outline src/orchestrator/factory.ts --view expanded | sed -n '1,220p'Repository: AgentWorkforce/factory
Length of output: 1847
🏁 Script executed:
set -euo pipefail
rg -n "`#publishedPullRequests`|`#dispatchInFlight`|`#resumeInFlight`|publishImplementerPullRequest|publishPullRequest\\(" src/orchestrator/factory.ts templates.test.tsRepository: AgentWorkforce/factory
Length of output: 1791
🏁 Script executed:
set -euo pipefail
printf'Factory around publish method:\n'
sed -n '2460,2665p' src/orchestrator/factory.ts | cat -n
printf'\nFactory around call site:\n'
sed -n '2468,2525p' src/orchestrator/factory.ts | cat -n
printf'\nImplementer exit / completion flow:\n'
sed -n '1360,1425p' src/orchestrator/factory.ts | cat -n
printf'\nTest files containing implementerNames or shared route.repo:\n'
git ls-files | rg 'templates\.test\.ts$|test\.ts$|spec\.ts$'Repository: AgentWorkforce/factory
Length of output: 16048
🏁 Script executed:
set -euo pipefail
printf'Test-like files:\n'
git ls-files | rg '(^|/)(.*test|.*spec)\.(ts|tsx|js|jsx)$'printf'\nFactory snippet around publish flow:\n'
sed -n '2488,2655p' src/orchestrator/factory.ts | cat -n
printf'\nFactory snippet around dispatch/resume patterns:\n'
sed -n '1380,1410p' src/orchestrator/factory.ts | cat -nRepository: AgentWorkforce/factory
Length of output: 10534
🏁 Script executed:
set -euo pipefail
sed -n '2480,2525p' src/orchestrator/factory.ts | cat -n
printf'\nNearby control flow:\n'
sed -n '2420,2488p' src/orchestrator/factory.ts | cat -nRepository: AgentWorkforce/factory
Length of output: 4772
🏁 Script executed:
set -euo pipefail
sed -n '2488,2525p' src/orchestrator/factory.ts | cat -n
printf'\nOne more surrounding block:\n'
sed -n '2400,2488p' src/orchestrator/factory.ts | cat -nRepository: AgentWorkforce/factory
Length of output: 5287
Guard PR publication with an in-flight dedupe#publishImplementerPullRequest checks #publishedPullRequests before any await and only adds the key after publishPullRequest(). Concurrent exits for the same issue/repo can both reach the write path, so use the same in-flight promise pattern as #dispatchInFlight/#resumeInFlight.
🤖 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 `@src/orchestrator/factory.ts` around lines 2613 - 2682, Update
`#publishImplementerPullRequest` to deduplicate concurrent publication attempts
using an in-flight promise map, following the established
`#dispatchInFlight/`#resumeInFlight pattern. Register the key before the first
await, reuse the existing promise for concurrent callers, and clear the
in-flight entry when the operation settles while preserving
`#publishedPullRequests` for completed publications.
Summary
Closes#52
Verification
Compatibility notes