[factory] Recover babysitters after PR snapshot failures - #249
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: 88 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 (5)
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: 396b9500ca
ℹ️ 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".
|
|
||
| const issue = await this.#readIssue(record.issue.path) | ||
| if (!issue || !isInFactoryScope(issue, this.#config.safety)) return | ||
| const pr = await this.#openPrForIssue(issue) |
There was a problem hiding this comment.
For a multi-repository issue with no babysitters, #openPrForIssue returns only the single globally highest-scoring PR. The first sweep adopts that PR, but every later sweep resolves the same PR and returns from #babysitterOwnerFor, so the other repositories' PRs remain permanently ownerless despite expectedRepoOwners indicating that more owners are needed. Reconciliation needs to enumerate candidates per unowned repository rather than repeatedly resolving one scalar candidate.
Useful? React with 👍 / 👎.
| const factoryTitleMatch = Boolean( | ||
| pr.headRef?.toLowerCase().startsWith('factory/') && | ||
| pr.crossRepository !== true && | ||
| (pr.matchScore ?? 0) >= 20, |
There was a problem hiding this comment.
When the PR is unavailable from the mount and #openPrForIssue falls back to resolveIssuePrFromGh, a PR whose factory/ branch carries a different work-unit number is discarded by that resolver's branch prefilter before issuePrMatchScore can award the title score. Consequently the cloud#3021/cloud#3024 identity shape that this condition explicitly accepts is recoverable only while its mounted snapshot is present; mount lookup failure makes the gh safety net return no PR. The fallback must retain same-repository Factory branches that have the required title association.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 5 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:1696">
P2: A persistently unreadable PR snapshot is re-exhausted and re-reported on every reconcile sweep with no dead-letter redrive cooldown. `#reconcileOrphanedBabysitters` calls `#handlePrChange(deadLetter.path)` each sweep, which re-runs the full 5-attempt read; when it still fails, `#readBabysitterPrSnapshotWithRetry` re-increments `exhaustions`, re-fires the `errorCode: 'snapshot_read_exhausted'` anomaly report and an error-level log, and leaves the dead letter in place. If the mount stays down, this repeats each sweep (in live mode potentially frequently), so a single stuck snapshot floods cloud anomaly reporting and error logs. Add a per-dead-letter minimum redrive interval (e.g. skip redrive if `now - failedAtMs` is below a threshold, or cap exhaustions/error reporting once a dead letter is known-stuck).</violation>
<violation number="2" location="src/orchestrator/factory.ts:1737">
P1: For a multi-repository dispatch with two open PRs, reconciliation can leave one PR ownerless forever. Resolve all open candidates, or query each expected repository rather than stopping at the single globally ranked PR.</violation>
<violation number="3" location="src/orchestrator/factory.ts:1739">
P1: When another configured repository has a matching `factory/<issue>` branch or title, reconciliation can spawn a babysitter for that unrelated PR. Require the resolved PR repository to match `record.issue` or one of its dispatch routes before accepting either match.</violation>
<violation number="4" location="src/orchestrator/factory.ts:1751">
P2: The reconcile adoption guard rejects any PR whose draft flag is absent from the mount metadata (`pr.draft !== false` is true when `draft`/`isDraft` is undefined), while the webhook path that this recovery is meant to mirror spawns whenever `snapshot.draft` is not truthy. `readProbePrCandidate` returns `draft` as `booleanValue(payload.isDraft) ?? booleanValue(payload.draft)`, so a real open PR without an explicit draft field is adopted by the live webhook path but permanently refused by this reconcile path. Since periodic reconcile exists precisely to recover PRs whose one-shot open event was missed, this contradicts the feature intent and can leave an ownerless open Factory PR unadopted (and keep escalating unowned PR events) even after the mount recovers. Use `pr.draft === true` so an absent field is treated as non-draft, matching `#handlePrChange`'s `if (snapshot.draft)` check.</violation>
<violation number="5" location="src/orchestrator/factory.ts:15037">
P2: The new `factoryTitleMatch` acceptance path in `#reconcileOrphanedBabysitterForIssue` depends on `pr.matchScore` being populated by `resolveIssuePrFromGh`, but that resolver's branch prefilter discards same-repo `factory/` branches whose work-unit number differs from the issue before `issuePrMatchScore` runs. When the mounted snapshot is unavailable, the gh fallback can't recover the cloud#3021/cloud#3024 identity shape this check was written to accept. Update the gh fallback's branch prefilter to retain same-repository Factory branches with a matching title association.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| const issue = await this.#readIssue(record.issue.path) | ||
| if (!issue || !isInFactoryScope(issue, this.#config.safety)) return | ||
| const pr = await this.#openPrForIssue(issue) |
There was a problem hiding this comment.
P1: For a multi-repository dispatch with two open PRs, reconciliation can leave one PR ownerless forever. Resolve all open candidates, or query each expected repository rather than stopping at the single globally ranked PR.
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 1737:
<comment>For a multi-repository dispatch with two open PRs, reconciliation can leave one PR ownerless forever. Resolve all open candidates, or query each expected repository rather than stopping at the single globally ranked PR.</comment>
<file context>
@@ -1662,6 +1685,147 @@ export class FactoryLoop implements Factory {
+
+ const issue = await this.#readIssue(record.issue.path)
+ if (!issue || !isInFactoryScope(issue, this.#config.safety)) return
+ const pr = await this.#openPrForIssue(issue)
+ if (!pr) return
+ const exactFactoryBranch = Boolean(pr.headRef && factoryBranchMatchesIssue(pr.headRef, issue.key))
</file context>
| if (!issue || !isInFactoryScope(issue, this.#config.safety)) return | ||
| const pr = await this.#openPrForIssue(issue) | ||
| if (!pr) return | ||
| const exactFactoryBranch = Boolean(pr.headRef && factoryBranchMatchesIssue(pr.headRef, issue.key)) |
There was a problem hiding this comment.
P1: When another configured repository has a matching factory/<issue> branch or title, reconciliation can spawn a babysitter for that unrelated PR. Require the resolved PR repository to match record.issue or one of its dispatch routes before accepting either match.
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 1739:
<comment>When another configured repository has a matching `factory/<issue>` branch or title, reconciliation can spawn a babysitter for that unrelated PR. Require the resolved PR repository to match `record.issue` or one of its dispatch routes before accepting either match.</comment>
<file context>
@@ -1662,6 +1685,147 @@ export class FactoryLoop implements Factory {
+ if (!issue || !isInFactoryScope(issue, this.#config.safety)) return
+ const pr = await this.#openPrForIssue(issue)
+ if (!pr) return
+ const exactFactoryBranch = Boolean(pr.headRef && factoryBranchMatchesIssue(pr.headRef, issue.key))
+ // Some app-created branches can carry the originating work-unit number
+ // rather than the mirrored issue number (cloud#3024 is a production
</file context>
| // independent restart-safe path if the one-shot event is gone for good. | ||
| await Promise.all([...this.#babysitterPrSnapshotDeadLetters.values()] | ||
| .slice(0, COMPLETION_SWEEP_BATCH_SIZE) | ||
| .map(async (deadLetter) => { |
There was a problem hiding this comment.
P2: A persistently unreadable PR snapshot is re-exhausted and re-reported on every reconcile sweep with no dead-letter redrive cooldown. #reconcileOrphanedBabysitters calls #handlePrChange(deadLetter.path) each sweep, which re-runs the full 5-attempt read; when it still fails, #readBabysitterPrSnapshotWithRetry re-increments exhaustions, re-fires the errorCode: 'snapshot_read_exhausted' anomaly report and an error-level log, and leaves the dead letter in place. If the mount stays down, this repeats each sweep (in live mode potentially frequently), so a single stuck snapshot floods cloud anomaly reporting and error logs. Add a per-dead-letter minimum redrive interval (e.g. skip redrive if now - failedAtMs is below a threshold, or cap exhaustions/error reporting once a dead letter is known-stuck).
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 1696:
<comment>A persistently unreadable PR snapshot is re-exhausted and re-reported on every reconcile sweep with no dead-letter redrive cooldown. `#reconcileOrphanedBabysitters` calls `#handlePrChange(deadLetter.path)` each sweep, which re-runs the full 5-attempt read; when it still fails, `#readBabysitterPrSnapshotWithRetry` re-increments `exhaustions`, re-fires the `errorCode: 'snapshot_read_exhausted'` anomaly report and an error-level log, and leaves the dead letter in place. If the mount stays down, this repeats each sweep (in live mode potentially frequently), so a single stuck snapshot floods cloud anomaly reporting and error logs. Add a per-dead-letter minimum redrive interval (e.g. skip redrive if `now - failedAtMs` is below a threshold, or cap exhaustions/error reporting once a dead letter is known-stuck).</comment>
<file context>
@@ -1662,6 +1685,147 @@ export class FactoryLoop implements Factory {
+ // independent restart-safe path if the one-shot event is gone for good.
+ await Promise.all([...this.#babysitterPrSnapshotDeadLetters.values()]
+ .slice(0, COMPLETION_SWEEP_BATCH_SIZE)
+ .map(async (deadLetter) => {
+ if (this.#stopping) return
+ this.#increment('babysitterPrSnapshotDeadLetterRedriveAttempts')
</file context>
| (pr.matchScore ?? 0) >= 20, | ||
| ) | ||
| if ( | ||
| pr.draft !== false || |
There was a problem hiding this comment.
P2: The reconcile adoption guard rejects any PR whose draft flag is absent from the mount metadata (pr.draft !== false is true when draft/isDraft is undefined), while the webhook path that this recovery is meant to mirror spawns whenever snapshot.draft is not truthy. readProbePrCandidate returns draft as booleanValue(payload.isDraft) ?? booleanValue(payload.draft), so a real open PR without an explicit draft field is adopted by the live webhook path but permanently refused by this reconcile path. Since periodic reconcile exists precisely to recover PRs whose one-shot open event was missed, this contradicts the feature intent and can leave an ownerless open Factory PR unadopted (and keep escalating unowned PR events) even after the mount recovers. Use pr.draft === true so an absent field is treated as non-draft, matching #handlePrChange's if (snapshot.draft) check.
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 1751:
<comment>The reconcile adoption guard rejects any PR whose draft flag is absent from the mount metadata (`pr.draft !== false` is true when `draft`/`isDraft` is undefined), while the webhook path that this recovery is meant to mirror spawns whenever `snapshot.draft` is not truthy. `readProbePrCandidate` returns `draft` as `booleanValue(payload.isDraft) ?? booleanValue(payload.draft)`, so a real open PR without an explicit draft field is adopted by the live webhook path but permanently refused by this reconcile path. Since periodic reconcile exists precisely to recover PRs whose one-shot open event was missed, this contradicts the feature intent and can leave an ownerless open Factory PR unadopted (and keep escalating unowned PR events) even after the mount recovers. Use `pr.draft === true` so an absent field is treated as non-draft, matching `#handlePrChange`'s `if (snapshot.draft)` check.</comment>
<file context>
@@ -1662,6 +1685,147 @@ export class FactoryLoop implements Factory {
+ (pr.matchScore ?? 0) >= 20,
+ )
+ if (
+ pr.draft !== false ||
+ pr.state?.trim().toUpperCase() !== 'OPEN' ||
+ !pr.headRef ||
</file context>
| pr.draft !== false || | |
| pr.draft === true || |
| candidates.push({ | ||
| repo, | ||
| prNumber: pr.number, | ||
| matchScore: score, |
There was a problem hiding this comment.
P2: The new factoryTitleMatch acceptance path in #reconcileOrphanedBabysitterForIssue depends on pr.matchScore being populated by resolveIssuePrFromGh, but that resolver's branch prefilter discards same-repo factory/ branches whose work-unit number differs from the issue before issuePrMatchScore runs. When the mounted snapshot is unavailable, the gh fallback can't recover the cloud#3021/cloud#3024 identity shape this check was written to accept. Update the gh fallback's branch prefilter to retain same-repository Factory branches with a matching title association.
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 15037:
<comment>The new `factoryTitleMatch` acceptance path in `#reconcileOrphanedBabysitterForIssue` depends on `pr.matchScore` being populated by `resolveIssuePrFromGh`, but that resolver's branch prefilter discards same-repo `factory/` branches whose work-unit number differs from the issue before `issuePrMatchScore` runs. When the mounted snapshot is unavailable, the gh fallback can't recover the cloud#3021/cloud#3024 identity shape this check was written to accept. Update the gh fallback's branch prefilter to retain same-repository Factory branches with a matching title association.</comment>
<file context>
@@ -14747,6 +15034,7 @@ const resolveIssuePrFromMount = async (
candidates.push({
repo,
prNumber: pr.number,
+ matchScore: score,
draft: pr.draft,
headRef: pr.headRef,
</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# #243 — an orphaned PR must be recoverable
node bin/factory.mjs babysit 3024 --config <path> # AgentWorkforce/cloud#3024
Note: 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
Verification
Fixes #243
Summary by cubic
Recovers babysitters after PR snapshot read failures and adopts ownerless Factory PRs without waiting for webhooks. Previously, a missed or unreadable PR-open snapshot dropped routing and left the PR unowned; now we retry with backoff, dead-letter on exhaustion, and a periodic reconcile redrives or adopts the PR to restore ownership. Addresses the requirements in Linear 243.
Key changes
factory/head ref that matches the issue or a strong same-repo title match; on adoption, clears unowned-event counters and dead letters.factory statusoutput; heartbeat counters take precedence when the CLI runs out-of-process.matchScoreso reconcile can safely adopt PRs when title evidence is strong even if branch numbers differ.Rollout
factory statusfor snapshot read failures, retries/dead-letters, unowned-event occurrences, and orphan adoption.Written for commit 396b950. Summary will update on new commits.