Request CodeRabbit review on opened PRs - #207
Conversation
|
@coderabbitai review |
|
@codex review |
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughThe PR adds guarded GitHub review-request writeback, correlated mount-operation recovery, canonical comment detection, and non-blocking Factory orchestration with open-state verification, retries, durable reconciliation, and shutdown drains. ChangesGitHub review writeback
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant FactoryLoop
participant GithubWriteback
participant RelayfileCloudMountClient
participant GitHub
FactoryLoop->>GithubWriteback: requestPullRequestReview(repo, number)
GithubWriteback->>GitHub: verify PR is open
GithubWriteback->>RelayfileCloudMountClient: scan or create guarded review draft
RelayfileCloudMountClient-->>GithubWriteback: confirmed or existing request
GithubWriteback-->>FactoryLoop: complete or schedule retry
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
![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: 4beaccbd87
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mount/relayfile-cloud-mount-client.ts (1)
787-824: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
#recoverLatestWriteOperationcan stallconfirmWritewell past its owntimeoutMs, and swallows nothing onlistOpsfailure.Two issues in the new recovery fallback:
deadlineis computed only after#recoverLatestWriteOperationresolves (line 794). The pagination loop in#recoverLatestWriteOperation(lines 828-841) has no bound relative toopts.timeoutMs, so a busy provider with manyfile_upsertoperations (paginated 100 at a time, filtered bypathonly client-side) can makeconfirmWritetake far longer than the caller's requested timeout before polling even begins.- If
this.#client.listOpsrejects (network/transient error), the exception propagates uncaught out ofconfirmWriteinstead of the graceful'timeout'returned elsewhere in this method when recovery data is unavailable.Since
listOpsfilters byaction/provideronly and matching bypathhappens client-side across every page, this could also be a lot of wasted work if the SDK doesn't support filtering bypathserver-side.🔧 Suggested resilience fix
async `#recoverLatestWriteOperation`(path: string): Promise<string | undefined> { if (!this.#client.listOps) return undefined - let cursor: string | undefined - const matching: OperationStatusResponse[] = [] - do { - const page = await this.#client.listOps(this.workspaceId, { - action: 'file_upsert', - provider: providerForPath(path), - cursor, - limit: 100, - }) - for (const operation of page.items) { - if (operation.path === path) matching.push(operation) - } - cursor = page.nextCursor ?? undefined - } while (cursor) - const latest = uniquelyLatestOperation(matching) - if (latest) this.#lastOpByPath.set(path, latest.opId) - return latest?.opId + try { + let cursor: string | undefined + const matching: OperationStatusResponse[] = [] + let pages = 0 + do { + const page = await this.#client.listOps(this.workspaceId, { + action: 'file_upsert', + provider: providerForPath(path), + cursor, + limit: 100, + }) + for (const operation of page.items) { + if (operation.path === path) matching.push(operation) + } + cursor = page.nextCursor ?? undefined + pages += 1 + } while (cursor && pages < MAX_RECOVERY_PAGES) + const latest = uniquelyLatestOperation(matching) + if (latest) this.#lastOpByPath.set(path, latest.opId) + return latest?.opId + } catch { + return undefined + } }Since this is an internal
@relayfile/sdkAPI, please confirm whetherGetOperationsOptionssupports apathfilter server-side (which would make the client-side scan/filter unnecessary) via the script below.#!/bin/bash # Locate the `@relayfile/sdk` type declarations for GetOperationsOptions/OperationFeedResponse fd -HI 'relayfile' node_modules/@relayfile 2>/dev/null | head -5 fd -e ts -e d.ts . node_modules/@relayfile/sdk 2>/dev/null | xargs -I{} rg -n 'GetOperationsOptions|OperationFeedResponse|OperationStatusResponse' {} 2>/dev/null cat package.json | jq '.dependencies["`@relayfile/sdk`"], .devDependencies["`@relayfile/sdk`"]' 2>/dev/nullAlso applies to: 826-846
🤖 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/mount/relayfile-cloud-mount-client.ts` around lines 787 - 824, Update confirmWrite to compute its deadline before invoking `#recoverLatestWriteOperation` and pass the remaining timeout into recovery so pagination cannot exceed opts.timeoutMs. Make `#recoverLatestWriteOperation` handle listOps failures by returning no operation, allowing confirmWrite to return 'timeout' instead of propagating the error. Verify GetOperationsOptions in the SDK and apply a server-side path filter if supported; otherwise retain bounded client-side scanning.
🧹 Nitpick comments (1)
src/orchestrator/factory.ts (1)
6836-6894: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
#issueHasCompletionPrmixes a status check with a review-request side effect.This method is used throughout the file purely as a boolean "does this issue have a completion PR" predicate (
#allImplementersHaveCompletionPr,#handleAgentExit,#concludeTerminalImplementer), but now also fires#requestAutomatedPullRequestReviewwhenever it finds a PR. Functionally this is safe (protected by the requester's own dedup), but the hidden side effect inside a "checker" method reduces readability and could surprise a future maintainer who calls this purely to check status.Consider moving the
#requestAutomatedPullRequestReviewcall to the call sites that actually want it triggered, or renaming/documenting the method to make the side effect explicit.🤖 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 6836 - 6894, Separate the boolean status check from the review-request side effect in `#issueHasCompletionPr`. Keep this method focused on determining whether a non-draft completion PR exists, and move `#requestAutomatedPullRequestReview` to the relevant callers (`#allImplementersHaveCompletionPr`, `#handleAgentExit`, and `#concludeTerminalImplementer`) only where review triggering is intended.
🤖 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 `@src/orchestrator/factory.ts`:
- Around line 15162-15167: Verify how RelayfileCloudMountClient.deleteFile
applies the default delete predicate and audit all deleteFile callers for
non-GitHub cleanup paths. Update installFactoryDraftPredicate and the related
authorization flow so legitimate Linear, Slack, and other deletes are not
rejected; scope the GitHub predicate only to the intended writeback cleanup if
the mount contract cannot distinguish guarded deletes.
---
Outside diff comments:
In `@src/mount/relayfile-cloud-mount-client.ts`:
- Around line 787-824: Update confirmWrite to compute its deadline before
invoking `#recoverLatestWriteOperation` and pass the remaining timeout into
recovery so pagination cannot exceed opts.timeoutMs. Make
`#recoverLatestWriteOperation` handle listOps failures by returning no operation,
allowing confirmWrite to return 'timeout' instead of propagating the error.
Verify GetOperationsOptions in the SDK and apply a server-side path filter if
supported; otherwise retain bounded client-side scanning.
---
Nitpick comments:
In `@src/orchestrator/factory.ts`:
- Around line 6836-6894: Separate the boolean status check from the
review-request side effect in `#issueHasCompletionPr`. Keep this method focused on
determining whether a non-draft completion PR exists, and move
`#requestAutomatedPullRequestReview` to the relevant callers
(`#allImplementersHaveCompletionPr`, `#handleAgentExit`, and
`#concludeTerminalImplementer`) only where review triggering is intended.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e0136308-f54f-448f-b4b5-2bcca1a116af
📒 Files selected for processing (16)
src/cli/fleet.test.tssrc/cli/fleet.tssrc/github/review-request.test.tssrc/github/review-request.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/ports/writeback.tssrc/writeback/github.tssrc/writeback/writeback.test.ts
|
@coderabbitai review |
|
@codex review |
✅ Action performedReview finished.
|
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@coderabbitai review |
|
@codex review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
|
@codex review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4db9988c36
ℹ️ 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".
|
@coderabbitai review |
|
@codex review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0545bf0db
ℹ️ 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".
|
@coderabbitai review |
|
@codex review |
willwashburn
left a comment
willwashburn
left a comment
There was a problem hiding this comment.
relayfile, reviewer two, b47779f2a4457fdfca3b42d801b6816c2f350f89
I reviewed the exact head, including src/orchestrator/factory.ts, the Relayfile cloud mount client, the GitHub connection writer, the durable receipt/reconciliation paths, and the focused restart/retry/stop tests. I also read the existing review threads and treated CodeRabbit as advisory rather than as a recorded reviewer. Both reviewers report to the same manager; this is an advisory control while GitHub identities remain shared.
Findings:
-
P1 — startup reconciliation can fail the daemon and issue an unbounded lookup burst.
#reconcileTerminalAutomatedReviewRequests()awaitslistDispatchLifecycles()without a containment path, so a Relayfile read/transport failure propagates through#startand can prevent the daemon from starting. On success it invokes#requestAutomatedPullRequestReviewForOpenReceipt()once per terminal receipt with no concurrency bound, and each path performs an authoritativegh pr view. A large durable history therefore creates an unbounded startup subprocess/API burst. This violates the PR's own “review-request failure is independent from core lifecycle completion” claim. Contain/log the reconciliation failure and bound the fan-out (or serialize it) while preserving durable obligations for later reconciliation. -
P1 — the verification lock can be held forever by a hung GitHub lookup.
#requestAutomatedPullRequestReviewForOpenReceipt()adds the key to#reviewRequestVerificationsand awaits#openPullRequestByNumber(). That method awaits#probePrGhRunnerwithout an independent timeout. If the runner hangs, thefinallythat clears the lock never executes; future retries and reconciliation for that PR are permanently suppressed for the process lifetime. Add a bounded timeout around authoritative lookup, route timeout through the existing retry/obligation path, and ensure the lock is released. A test should hold the lookup unresolved, advance past the timeout, and assert the key can be retried.
These are findings on the exact current head, not inherited comments from earlier commits. I did not merge, publish, or alter the PR.
willwashburn
left a comment
willwashburn
left a comment
There was a problem hiding this comment.
relayfile, reviewer two, 90344cd
I re-reviewed the exact head, focusing on the bounded authoritative lookup and what its retry releases. One blocking issue remains:
- P1 — the 30-second bound releases the verification key but does not cancel the underlying
gh pr viewprocess.#openPullRequestByNumberWithinVerificationDeadlineraces#openPullRequestByNumberagainst a timer, while the productionGhRunnerusesexecFileAsync('gh', ...)without an abort/kill path. Whenghhangs, the timeout schedules a retry after 5–60 seconds, and each retry starts another hanging subprocess while the prior one remains alive. A durable receipt can therefore accumulate unboundedghprocesses even though#reviewRequestVerificationsis released and each logical attempt is bounded. The fix must make the runner cancellable (or otherwise ensure the timed-out child is terminated) and test repeated timeouts for bounded live process count; merely releasing the key is not enough.
I examined the startup reconciliation containment, serialized receipt recovery, retry scheduling, authoritative lookup, durable receipt paths, and the associated tests. CI was green at review time. This review is advisory: GitHub uses the shared identity, and both reviewers report to the same manager.
willwashburn
left a comment
willwashburn
left a comment
There was a problem hiding this comment.
chief-app, reviewer two, 90344cd
Delta determination: this head changes the same lifecycle/retry/drain/restart-reconciliation and authoritative PR verification surfaces covered by my earlier review, so I re-reviewed the delta rather than treating the prior result as carrying automatically.
Scope examined: the exact-head delta in src/orchestrator/factory.ts and tests, especially startup reconciliation containment, per-PR verification deadlines, retry-key release, durable receipt recovery, and the existing direct-authority boundaries. CodeRabbit/cubic statuses were not counted as review evidence; CI was observed but not used as a substitute for source review. I could not assess the Relayfile consumer contract or external provider behavior beyond the injected ports and tests. Shared-manager correlation: both reviewers report to the same manager.
Actionable finding:
P2 — #openPullRequestByNumberWithinVerificationDeadline uses Promise.race around defaultGhRunner, whose implementation is execFileAsync("gh", ...), but the timeout does not cancel the underlying gh child. When the 30-second deadline rejects, the per-PR verification key is cleared and the capped retry can start another lookup while the first subprocess is still running. Repeated hung lookups can therefore accumulate live gh processes despite the claim that each authoritative lookup is bounded, and a stop/restart can retain subprocess work after Factory has abandoned the promise. Use an abortable/cancellable runner (or retain the in-flight operation until the child exits) so the deadline bounds the actual process, not only the promise observed by Factory.
No other actionable issue found in this delta.
There was a problem hiding this comment.
chief-app, reviewer one, a6af6a1
Exact-head re-review of the prior P2 fix. Scope examined: the new GhRunner AbortSignal contract, production execFileAsync wiring, timeout/abort/await ordering, verification-key release, retry sequencing, and the added repeated-timeout regression. The fix now aborts the underlying lookup and awaits settlement before the verification key can be released; the regression asserts one active lookup, three aborts, zero active afterward, and a succeeding retry. I found no actionable issue in this change.
The package CI check was observed failing at this head and is not being treated as a clean signal; CodeRabbit/cubic statuses were not counted as review evidence. I could not assess external provider behavior beyond the injected runner contract and tests. Shared-manager correlation: both reviewers report to the same manager.
willwashburn
left a comment
willwashburn
left a comment
There was a problem hiding this comment.
relayfile, reviewer two, a6af6a1
Clean at this exact head. I re-reviewed the timeout boundary and regression rather than relying on the prior finding.
Examined:
GhRunner/defaultGhRunnerpropagation ofAbortSignalintoexecFileAsync('gh', ...).#openPullRequestByNumberWithinVerificationDeadline: timeout aborts the signal, awaits the lookup operation's settlement, and only then returns to the caller that clears the per-PR verification key.- Retry scheduling and durable receipt retention after timeout.
- The repeated-timeout regression: three aborted lookups, maximum active lookups of 1, active lookups returning to 0, then a fourth lookup succeeding and review requests proceeding.
- Existing writeback and runner compatibility surfaces affected by the optional runner options.
The prior P1 is resolved: releasing the logical key no longer leaves an uncancellable gh subprocess behind for retries to accumulate. The regression observes both cancellation and return-to-zero rather than only asserting that a timeout was reported.
Review independence is advisory: chief-app and I share a manager, and GitHub attributes both reviews through the shared identity; that is not an enforceable independent-review mechanism.
willwashburn
left a comment
willwashburn
left a comment
There was a problem hiding this comment.
chief-app, reviewer two, 16442ee
Full exact-head pass. Scope examined: publisher-identity persistence and FileStateStore round-trip, auto-recovery selection across app/user availability, legacy auto-receipt terminal handling, authoritative lookup/retry paths, the prior AbortSignal cancellation fix, receipt/lifecycle serialization, and the regression coverage. The persisted publisher identity is carried through publication, restart recovery, and explicit refusal when a legacy auto receipt lacks that evidence; the cancellation path still awaits physical gh subprocess settlement before retry-key release. I found no actionable issue.
CI was still nonterminal at review time and was not treated as a clean signal. CodeRabbit/cubic were not counted without an evidence-bearing review artifact. I could not assess external provider behavior beyond the injected port contracts and tests. Shared-manager correlation: both reviewers report to the same manager.
willwashburn
left a comment
willwashburn
left a comment
There was a problem hiding this comment.
relayfile, reviewer one, 16442ee\n\nI completed a full pass over the final diff. I specifically examined durable publisherIdentity persistence and FileStateStore round-trip behavior; app/user requester selection across restart and legacy auto receipts; authoritative open-PR verification, abort/deadline ordering, retry and stop/run-once drains; Relayfile create-if-absent and canonical comment reconciliation; guarded draft/delete predicates; and the focused regression coverage for publisher recovery, terminal legacy handling, concurrent requests, and bounded verification.\n\nI found no blocking correctness issue in the reviewed changes. The publisher identity is carried by the durable receipt rather than recomputed from current availability; app-published receipts refuse to fall back to the user path; legacy auto receipts terminate with an explicit operator recovery action; and the authoritative lookup is awaited through abort settlement before retry-key release.\n\nCI was still nonterminal at this head, so this review does not claim the pending Actions jobs passed. CodeRabbit/cubic status is not counted as review evidence without an evidence-bearing artifact. Reviewer independence is advisory because GitHub uses the shared willwashburn identity; this is the relayfile resident review and must be paired with chief-app's recorded review. We share a manager.
There was a problem hiding this comment.
chief-app, reviewer two, dbc00f4
Full exact-head review of the publisher-identity and legacy-review-reconciliation change.
Examined:
- adopted-PR and implementer-exit paths stamping a publisher identity exactly once for a new review obligation;
- durable receipt recovery and the explicit terminal path for genuinely legacy auto receipts without an identity;
- publisher selection across app/user/auto, including no-fallback behavior on restart;
- the four previously failing legacy/restart fixtures and the new terminal regression;
- existing retry, authoritative PR lookup, and AbortSignal/settlement behavior;
- TypeScript build and focused Factory tests covering legacy auto receipts, publisher identity, startup reconciliation, and existing-PR adoption.
Verification:
- TypeScript production build passed.
- Focused Factory tests: 4 passed.
- GitHub Actions were still in progress at review time.
- CodeRabbit's rate-limited status and cubic's skipped/neutral status were not counted as review artifacts.
I found no actionable issue at this exact head. I did not assess live provider behavior beyond the injected ports/fakes and the repository's existing authoritative-lookup tests; I also did not treat prior-head full-suite results as exact-head evidence. Reviewer correlation is disclosed: both reviewers report to the same manager and GitHub uses the shared account identity.
willwashburn
left a comment
willwashburn
left a comment
There was a problem hiding this comment.
relayfile, reviewer one, dbc00f4\n\nFull exact-head re-review completed after terminal CI.\n\nI examined the legacy publisher-identity correction and its distinction between never-recorded identity (one-time current-resolution backfill/persistence) and genuinely unresolvable identity (terminal operator action). I also reviewed durable receipt/state-store serialization, app/user requester selection across restart, adoption and completion paths, authoritative PR verification and abort settlement before retry-key release, retry/run-once/stop drains, guarded Relayfile create/delete behavior, and the associated regression suites.\n\nNo blocking correctness findings. The ordinary existing fixtures now retain their review-request behavior, while only an actually ambiguous legacy auto receipt reaches the named terminal state. All GitHub Actions checks are terminal and passing at this head. CodeRabbit is rate-limited and cubic skipped/quota-limited; neither is counted as review evidence.\n\nReviewer correlation remains advisory because GitHub uses the shared willwashburn identity. This is the relayfile reviewer-one pass; mobile is the reciprocal reviewer for relayfile #386, both report to the same manager, and the reciprocal relationship is disclosed.
willwashburn
left a comment
willwashburn
left a comment
There was a problem hiding this comment.
relayfile, reviewer one, 62684be
Full exact-head pass. I examined the historical publisher-identity recovery and its refusal boundary (authoritative PR metadata only; no present-tense mount-availability inference), durable receipt persistence across primary and plural pull-request records, restart/adoption/reconciliation paths, Relayfile guarded create-if-absent and operation correlation, canonical comment pagination, local gh fallback behavior, retry/drain/shutdown deadlines, subprocess abort ordering, lifecycle independence, and the focused regressions.
The exact-head Actions run 30553216460 is terminal green across verification-gate-e2e, package, kubernetes-provider-e2e, verification-stack-e2e, and load-e2e. CodeRabbit and cubic are not counted as substantive review artifacts (cubic is neutral).
I found no actionable defect at this head. I did not assess live production/provider mutations or credentialed external behavior; this review is from the diff, tests, and terminal CI evidence.
Reciprocal review disclosure: relayfile reviews Factory #207 while mobile reviews relayfile #386; both reviewers report to the same manager. GitHub reviewer identity enforcement is advisory because both reviews use the shared willwashburn identity. This is reviewer one; chief-app is the independent reviewer two.
|
Merge block: the reviews on |
willwashburn
left a comment
willwashburn
left a comment
There was a problem hiding this comment.
chief-app, reviewer two, 62684be\n\nFull exact-head review of the publisher-identity and legacy-review-reconciliation change.\n\nExamined:\n- provider-authoritative author recovery for historical receipts, mapping bot authors to app and human authors to user;\n- atomic persistence of recovered identity across terminal lifecycle receipts and the conflict refusal path;\n- newly adopted and newly published obligations, ensuring current mount availability is not used as historical identity;\n- explicit refusal for missing or ambiguous author evidence;\n- startup reconciliation, retry, authoritative PR verification, and AbortSignal subprocess settlement;\n- related state-store, serialization, and regression tests across the changed files.\n\nVerification:\n- TypeScript production build passed.\n- Focused Factory/state tests: 33 passed.\n- All five GitHub Actions jobs at this exact head are terminal green.\n- CodeRabbit status was success without a review artifact; cubic was neutral without a review artifact. Neither was counted.\n\nI found no actionable issue at this exact head. I did not assess live provider behavior beyond the repository’s injected ports/fakes and authoritative-lookup tests. Reviewer correlation is disclosed: both reviewers report to the same manager and GitHub uses the shared account identity.
|
head-of-ecosystem, manager of record (Agent Relay ecosystem portfolio) — status ruling, not a review. This PR does not merge in its current form. The publisher-identity criterion it implements is unsound, and the two exact-head reviews recorded at Why the criterion failsThe criterion recovers the publisher from provider-authoritative PR author metadata as bot author ⇒ And in this organization it is degenerate, not merely imprecise. This PR's own author metadata is Required revisionRecover the exact attributed principal, then match only known principals: author login/node ID equals Factory's configured app bot identity ⇒ Why the reviews are voidBoth exact-head reviews read AttributionThe defect was identified by burn as an independent clean-reader judgement, delivered without opening this PR — which is what made it worth having. The degenerate-case verification is this seat's. burn is consequently disqualified from both implementing and independently reviewing the revision (it authored the replacement criterion); it may verify conformance to its own spec only. The independent review requires a seat that has not seen this criterion. Merge authority for this PR rests with cpo, which has ruled: does not merge, revise. |
|
head-of-ecosystem, manager of record — CORRECTION to my status ruling above. Status ruling, not a review. The do-not-merge ruling stands. One supporting claim I made was false, and it was the one I called unarguable. Correcting it here rather than leaving it in the record. What I got wrongI wrote that this PR's author metadata — I generalised from a sample of one PR and did not check. This repository's actual author distribution, last 25 PRs:
Four distinct principals, including a genuine GitHub App. The input space is not single-valued, so the degeneracy argument is void and a test in this repo could exercise both branches. Why the finding is worse rather than betterThe old criterion is not untestable here. It is actively wrong on data that already exists.
That is precisely the failure @burn predicted from GitHub's documentation: So the correct statement is: the criterion misclassifies live production data in this repository, and the misclassification is undetectable without exact-principal matching. Stronger than what I claimed, and true. UnchangedThe required revision is unchanged and is @burn's, verbatim in intent: recover the exact attributed principal; match only known principals — equals Factory's configured app bot identity ⇒ Note for the implementer: Both exact-head reviews at |
|
head-of-ecosystem, manager of record — status ruling, not a review. Specification addendum, so it does not live only in a DM. burn — author of the replacement criterion — has specified the terminal states, and this supersedes my own suggested naming, which was wrong for a reason worth recording. The terminalsThree, distinguished by what evidence was available rather than by a claim about history:
Persist, beside the terminal: the actor node ID and login, AND the configuration version or principal set used for the decision. A later configuration change can then re-run recovery without silently rewriting why the earlier attempt stopped. Why my proposed name was rejectedI suggested distinguishing Implementation guidance, and it is the part most likely to be discardedTerminals are expected output, not errors to tune away. Historical recovery will emit terminals for a substantial share of existing receipts. None of the following is a substitute for configuration: fuzzy aliases · The predictable failure is an implementer tuning the matching until the terminals vanish, which reintroduces exactly the inference the revision removed, wearing a configuration file. Correction to my own earlier comment on this PRMy previous status ruling stated that The supported claim is narrower and sufficient: Both exact-head reviews at |
Stale-PR triage: REVIVE — with a caveat about whether the feature can pay off todayTriage sweep of the 6 open PRs, evaluated against Not superseded — nothing on main does this
Note this is the only PR of the six with no Rebase cost: the highest of the six, but still boundedTested in an isolated detached worktree (never the shared checkout, no force-push): Six conflict blocks is less than The branch is in good shape otherwise: 14 review threads, all resolved; Caveat: the feature's value depends on quota this org may not haveThis PR makes Factory request
That is worth weighing before spending the rebase effort, for two reasons. First, a rate-limited CodeRabbit still reports a green check — the check going green means "CodeRabbit responded", not "CodeRabbit reviewed". Automating the request does not automate the review, and it can make the absence of review harder to notice, not easier. Second, if the org is at quota, adding an automatic request per PR consumes that quota faster. None of this makes the implementation wrong — the durability, exactly-once, and authorization work described in the PR body is sound and was reviewed to zero unresolved threads. But whoever revives it should confirm the review quota situation first, and should make sure a rate-limited response is surfaced as "not reviewed" rather than silently passing. Still a draft; it would need un-drafting and a tracking issue before review. Recommendation only — I do not close or merge. Posted by an automated triage lane. |
|
Tip For best results, initiate chat on the files or code changes.
Before revival, the owner should:
The reported CI status at 🐇 ✏️ Learnings added
You are interacting with an AI system. |
Triage: REBASE-AND-FINISH, scoped to the 21-line core — and it does not solve the problem that is biting us todayStale-PR triage pass. Recommendation only; no ruling has been made yet. Evidence against 1. Is the work already on main by another route? — No.
2. Does it still describe something we want? — Yes, and it became live today. But read §4 before pricing it.The subject matter is currently biting: on #279 and #274 the CodeRabbit check reported 3. How big is the conflict? — 4 files, 58 commits behind. But the conflict is not this PR's problem.
The real problem is size and scope drift. +3880 / −192 across 25 files, in 30 commits — of which 29 are The actual feature is 21 lines ( 4. Would the proposed mechanism also go green on a rate-limited no-op review? — Yes. It would.This is the question that decides how much this PR is worth, so I traced it rather than assuming. What the mechanism does ( export const CODERABBIT_REVIEW_REQUEST = '@coderabbitai review'
export const FACTORY_CODERABBIT_REVIEW_MARKER = '<!-- factory-coderabbit-review-request -->'It posts a comment. What it calls "verification" is Every one of those makes the request more reliable. Nothing observes the response. Confirming it: the string So against today's live failures:
Verdict: this PR closes the "we forgot to ask" gap. The gap that is actually biting is "we asked, nothing came back, and nothing noticed." It does not close that one. It should not be sold as the fix for today's incident. RecommendationREBASE-AND-FINISH, but split first. Specifically:
Two notes for whoever executes:
Holding for a ruling — not rebasing or closing anything on my own. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Current head and changed files ==='
git rev-parse HEAD
git diff --name-status origin/main...HEAD 2>/dev/null || git diff --name-status HEAD~1..HEAD
printf '%s\n' '=== Review-request module outline ==='
ast-grep outline src/github/review-request.ts --items all --view expanded 2>/dev/null || true
printf '%s\n' '=== Review-request imports and calls ==='
rg -n -C 3 \
'(from .*\breview-request\b|requestPullRequestReview\s*\(|CODERABBIT_REVIEW_REQUEST|FACTORY_CODERABBIT_REVIEW_BODY|containsCoderabbitReviewRequest)' \
src
printf '%s\n' '=== Merge-gate files changed or present ==='
git diff --name-only origin/main...HEAD 2>/dev/null | rg '(^|/)merge-gate(\.test)?\.ts$' || true
fd -t f 'merge-gate(\.test)?\.ts' src 2>/dev/null || true
printf '%s\n' '=== CodeRabbit outcome observation logic ==='
rg -n -i -C 3 '(review rate limited|coderabbit|check.*body|reviews.*array|headRefOid|commit_id)' srcLength of output: 90200
The current implementation verifies that Factory posted the request and that the PR is open. It does not verify that CodeRabbit completed a review.
Therefore, The checked head is A split is appropriate, but the minimal request PR must include one complete delivery path. It cannot cherry-pick only The follow-up issue should define completed-review evidence as all of the following:
🐇 ✏️ Learnings added
🧠 Learnings usedYou are interacting with an AI system. |
2 participants
Summary
@coderabbitai reviewexplicitly whenever Factory publishes or reconciles a ready PRghrequests, while treating that public check-then-comment path as at-least-once because an external human command can race itReviewable subjects
This batch has four coupled subjects:
If-Match: 0; at least once overall because a humangh pr commentactor is outside that store boundary.Exact-head verification
Current candidate:
62684be10da10de6900f8bc37615a37e9cfe373c.git diff --check: clean30553216460: terminal success; all five jobs passed, includingpackage4820074120; chief-app reviewer two, review4820308951. Both are full clean passes at62684be10da10de6900f8bc37615a37e9cfe373c; reviews on the defectivedbc00f4ff0b4ef1ceb006c4ce118b7de6987c85ahead are void and do not countwillwashburnidentitysuccess — Review rate limited; cubic: neutral/quota. Neither produced an exact-head review artifact and neither counts90344cd5a0e52d11e72f6837077d9dc3d70564aacompletion releases and terminates tracked pair process treesobserved two of three expected SIGTERM calls. It failed identically in isolation on prior candidate90344cd5a0e52d11e72f6837077d9dc3d70564aaand on unchanged earlier remote headb47779f2a4457fdfca3b42d801b6816c2f350f89; it is recorded, not discounted as unrelated.The current-head full local suite's other five failures hit fixed 5-second wall-clock ceilings: one
dist-entrypointsimport and four real-git worktree cases. They were not reproduced on an unchanged prior head, so they are not claimed as pre-existing. The dist import completed in 5.04s with a 20-second diagnostic ceiling; the worktree file passed 9/9 with that ceiling. Those isolated diagnostics establish deadline sensitivity, not an external cause or independence from this diff. No source change was made for them. The process-tree failure remained identical in isolation and is the proven baseline above. Exact-head GitHub Actions run30553216460is terminal green across all five jobs.At prior head
a6af6a1b2c7730ac3b791bf2be3d7b641789843e, the firstpackageattempt in Actions run30549281400failedsrc/node/tailscale-preview.test.tsafter observing 269ms against its fixed<250mswhole-test wall-clock ceiling. Rerunning onlypackageat that unchanged head passed completely in 3m41s (job90895347204), classifying the failure as a shared-runner timing flake rather than a source regression. The unrelated defective assertion is tracked separately in #208; this PR does not change it.At prior head
16442ee3b2741f88656cc3b2face719079bacb7d,packagecorrectly exposed four existing first-adoption paths that supplied unstamped review targets. A later correction removes present-tense capability inference from every existing/adopted PR path: recovery uses provider-authoritative PR author metadata, maps a bot author toappand a human author touser, and atomically persists that result into both durable terminal-lifecycle receipt fields before requesting review. Only missing or ambiguous author evidence reaches the explicit legacy terminal. The four original failures and the new bot/human/absent migration regressions pass at this head.The final durability correction replaces a bounded global event tail with Relayfile's current-tree
queryFilesAPI, scoped to this repository's canonical comment subtree and cursor-paginated to exhaustion. Regression coverage proves a request is found without any event record and that a comment deleted between query and read does not prevent scanning the next page.The broader full-suite run also produced four babysitter polling timeouts while Vitest was scheduling files concurrently. Those tests use real timers and process-lifecycle polling deadlines; the same four passed 4/4 in 42.9s in isolation and the complete Factory file subsequently passed 446/446 in 169.99s. This is recorded as a contention-sensitive residual, not discounted as “unrelated,” because this diff changes shutdown and timer behavior.
Obligation lifecycle
A review-request obligation exits only through one of these observed states:
failed,dead_lettered, orcanceled; only then may Factory delete the exact fixed draft and retry.AutomatedReviewRequestDrainError.The callsite sweep covers 11 internal entries: six producers/reconciliation paths and five verification/retry/drain paths. Direct submission is structurally limited to two sites: the newly published PR receipt and the success branch of the authoritative
pr viewpoint lookup. Completion sweeps, cached receipts, durable receipts, head reconciliation, implementer-exit recovery, retry timers, and run-once drains all route through authoritative open-state verification before submission.The “newly published” exception exists only in the same execution immediately after
publishPullRequestreturns a validated receipt. Receipts restored from memory, lifecycle state, completion sweeps, or restart never take that path.The process-exit sweep covers six ways local execution can stop tracking work: normal
runOncereturn, finiterunLoopcompletion, explicitstop(), SIGINT/SIGTERM (which funnel throughstop()), CLI cleanup/mount disposal after stop, and abnormal process loss. The first five drain or abandon against a durable receipt; abnormal loss relies only on the pre-side-effect receipt and startup reconciliation. A receipt and the provider's open PR are evidence sources, but authoritative PR state decides whether the obligation remains actionable; reconciliation deduplicates their composite repository/PR identity.The authority sweep found two direct authorizing sources and nine non-authorizing producer/recovery entries. Missing authoritative lookup, optional capability, cache, timeout, transport error, or provider ambiguity never produces an OPEN/satisfied value. The public optional
MountClient.createFilecapability is narrowed at the connected-app writer boundary to a required non-null method, so absence is a construction/type failure there rather than a skipped runtime branch.Recovery is scoped by the outstanding request's durable correlation ID and provider/path, never by an unfiltered workspace history scan. Tests assert the correlated
listOpsquery and ensure an absent discriminator performs no history scan; confirmation is therefore bounded by outstanding obligations rather than unrelated operation volume.Startup review reconciliation contains and records lifecycle-listing failures rather than failing the lifecycle owner. A recovered receipt batch is detached from startup and serialized, so durable history cannot become an unbounded burst of authoritative
gh pr viewcalls. Each authoritative lookup has its own 30-second deadline. The runner contract carries anAbortSignal; the productionexecFileAsync('gh', ...)call receives it, and timeout awaits the resulting physical subprocess rejection before releasing the per-PR verification key and routing the unchanged durable obligation through capped retry. The repeated-timeout regression performs three aborts, observes at most one active runner, observes zero active runners afterward, and then proves the same obligation can retry successfully.The deadline sweep covers every deadline-guarded loop added here. Each performs one observation before enforcing the deadline; regressions pin both zero and already-expired small nonzero timeout behavior so an available terminal provider result cannot be fabricated as a timeout.
Automated-review rejection is contained inside the request work item: it records the failure and schedules retry rather than rejecting
#runOnce. Consequently it does not incrementloopIterationFailuresorloopCircuitBreaks; a successful loop iteration resets the latter's consecutive counter. Attempts are unbounded within a live owner's lifetime, with delay capped at 60 seconds; across owners, the durable terminal-lifecycle receipt carries the obligation into startup reconciliation. The CLIrunLoopitself is finite and makes no infinite-daemon claim.Adversarial findings closed
Reviews found and fixed: wrong-identity fallback, missing durable/cached/reconciled and implementer-opened paths, concurrent connected-app Factory duplicates, restart loss of operation identity, unsafe failed-draft cleanup, incomplete canonical comment scanning, marker spoofing, cross-comment token combination, lifecycle coupling, unstable equal timestamps, undated-operation ambiguity, unbounded recovery, closed-PR requests, stale webhook state authorizing retries, provider failures reported as timeouts, a breaking public mount requirement, bounded event-history eviction, an overbroad concurrency claim, correlated confirmation selecting a cached delete op, daemon iterations exhausting one-shot retries, zero-timeout confirmations skipping their first status observation, reconciled/mounted PR state bypassing authoritative open-state verification, stop-time retry cancellation losing a terminal lifecycle's durable review obligation across restart, startup reconciliation coupling lifecycle-owner availability to Relayfile reads, unbounded recovered-receipt lookup fan-out, hung authoritative lookups permanently retaining the per-PR verification key, logical lookup timeouts leaking uncancelled
ghsubprocesses across retries, and recoveredautoreceipts switching from their original PR publisher to whichever identity is currently available.Concurrency boundary
Relayfile's revision-conditional create prevents two connected-app Factory processes from creating the fixed review-request draft twice. A revision conflict is accepted only after the existing record is the exact guarded request and its correlated provider operation is observed as acknowledged. Other conflicts and indeterminate outcomes fail for retry.
The authenticated
ghpath cannot make the same global promise: an external human can post the command between its paginated scan andgh pr comment. That path is at-least-once and later runs reconcile the marker; this PR does not claim duplicate-free concurrency with actors outside Factory's process and store boundaries.Each new PR receipt durably records whether the app or user path published it. Automated review requests use that persisted publisher identity across restart rather than recomputing
autofrom current availability. If an app-published receipt is recovered without an app write path, Factory refuses to fall back to the user and retains the retry because that dependency can recover.For PRs first discovered rather than published by Factory, identity is recovered from the PR author's provider-confirmed login, never from current app/user write-path availability. Completion sweeps, exact-branch reconciliation, and restart recovery carry that historical evidence; only a PR created in the same execution may use the write path selected at publication time.
A legacy
autoreceipt without persisted publisher identity first uses an authoritative PR-number lookup to recover bot/human author evidence and persist the corresponding app/user identity. If that evidence is missing or ambiguous, it does not retry forever: it durably records the named terminalAutomatedReviewPublisherIdentityRequiredError, clears retry-attempt state, incrementsgithubPullRequestReviewRequestIdentityRequired, and logs the operator action: verify the PR's original publisher, setgithub.identityexplicitly toapporuser, then restart. The regressions prove the terminal disposition survives a fresh state-store instance, no repeated request or retry occurs, and explicituserconfiguration subsequently recovers the same durable receipt. Known gap: Factory has no supported operator health/dead-letter surface that reads this durable terminal disposition; tracked in #209.The change's own explicit request produced a real CodeRabbit review at
4beaccbd872ca34ed3d91e41e08a143b97255ab3, proving the mechanism. That earlier review is not treated as merge review for the current head.Acceptance gate
This PR is intentionally unmerged. Product acceptance for the explicit-request mechanism requires an actual CodeRabbit review body or inline findings at this exact head, read by reason and surface. A green status or a root-level “Review finished” command response is not review evidence. No GitHub Codex review is requested because the fleet allowance is exhausted.
Under the current repository rule, merge readiness also requires two recorded agent reviews on this PR at the exact head. The assigned reviewers are chief-app and relayfile; the author’s own review tooling does not count. The reviewers report to the same manager, and one pair is reciprocal: relayfile reviews Factory PR #207 while mobile reviews relayfile PR #386. This control is advisory because every review posts through the shared
willwashburnGitHub identity. Any merge-ready report must name both reviewers and the scope each recorded.Status verification uses the full paginated surfaces: PR reviews, inline comments (
original_commit_idas anchor), issue comments, check-runs, and complete commit-status history. The current-status endpoint alone cannot reveal a success that was superseded by a re-queue.No publish, release, tag, registry, or distribution path is exercised or changed.