fix(gui): stop the fork dialog waiting forever, and block a doomed submit - #1230
Conversation
…bmit Two client-side halves of a cross-host fork review, plus the protocol field they read. `epic.chatPublicationState` gains `definitive`, set when waiting CANNOT change the answer. Three host states needed it and none could be expressed before: a deleted source chat, a chat whose lineage lost an arbitrated fork, and a publication halted for a reason the sweep does not retry within the process lifetime. All three reported byte-identically to "the sweep has not reached it yet", so the condition poll re-asked every 30s forever while the dialog told the user "it backs up automatically - try again shortly" and kept submit blocked. The poll now terminates and each reason gets copy that is true for it. An UNRECOGNISED reason is treated as terminal-but-unexplained rather than as null, so a newer host cannot reintroduce the infinite wait. An ABSENT one is NOT: the field was added to an unreleased method in place, so a host built before it takes the un-parsed same-version path and the key genuinely arrives missing - reading that as a reason would mark every older host permanently halted, which is the same hang inverted. Separately, submit no longer stays enabled while `boundaryCovered === false`. That behaviour was documented as safe because "Layer 2 - the host's typed refusal - is the authority", and that premise was false: the host's check was presence-only, so a boundary published mid-stream is present at its partial version, the fork returns 200 OK, and the user gets a silently truncated turn. The host-side currency check lands alongside this; both halves are wanted, because the hook reports UNKNOWN during background refetches and the gate therefore blinks open for one RTT per poll - the host refusal is what makes that blink harmless rather than a hole. `boundaryCovered: null` is still never collapsed to `false`, and the permissive unknown path for hosts that predate the method stays permissive. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Hardik Shingala <hardik@traycer.ai>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 36 minutes Limit details: You’ve used all 1 included review currently available under your plan. You completed 122 included PR reviews in the past 7 days; at that activity level, included reviews refill at 1 review per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
Included review availability: 0 reviews are currently available. Based on recent review activity, included reviews refill at 1 per hour. Summary by CodeRabbit
WalkthroughThe PR adds definitive chat publication outcomes to the protocol, shared parsing, polling policy, and chat-fork eligibility. Definitive failures make rows unreachable; transient syncing blocks submission while remaining selectable. ChangesChat publication and forking
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The PR is merge-ready after normal checks and review; no actionable merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant Host
participant PublicationQuery
participant ForkTarget
participant ForkDialog
Host->>PublicationQuery: return publication state and definitive reason
PublicationQuery->>ForkTarget: classify publication status
ForkTarget->>ForkDialog: provide verdict and notice
ForkDialog->>ForkDialog: enable submission only for allowed
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…usals Three CI jobs on #1230 failed on tests that still describe the behaviour this branch deliberately changed. `definitive` is `.default(null)`, so a wire payload that omits it - which is exactly what a host predating the field sends - no longer parses back to itself. Both round-trips now assert the defaulted shape and say why the two sides differ. The field also had no coverage at all, so this adds it: each reason round-trips, an explicit null stays null, and an unrecognised reason is rejected rather than silently read as "no terminal cause", which would put the infinite wait back. The two `boundarySyncing` dialog tests were stale in OPPOSITE directions, and updating them the same way would have hidden the rule. The one that selects a remote host is cross-host, so it now asserts the submit is BLOCKED - the reversal this branch made after finding the host's coverage check is presence-only. The first-paint one selects nothing, so it is a same-host fork that `chatForkTargetVerdict` short-circuits to `allowed` before it ever reads publication state; its button correctly stays enabled. The pair is what pins that exemption, so both now say so. Signed-off-by: Hardik Shingala <hardik@traycer.ai>
|
@coderabbitai approve |
✅ Action performedComments resolved and changes approved. |
1 participant
Client half of a cold review of the cross-host chat fork feature. The host half is internal #5014; this PR carries the protocol field it answers with and the two client behaviours that read it.
Follows #1227.
definitive— a way to say "never"epic.chatPublicationStategainsdefinitive, set when waiting cannot change the answer. Three host states needed it and none could be expressed before:chat-deletedlineage-supersededbackup-haltedAll three previously reported byte-identically to "the sweep has not reached it yet", so the condition poll re-asked every 30s forever while the dialog said "It backs up automatically — try again shortly." and kept submit blocked. The poll now terminates, and each reason gets copy that is true for it.
Two cases that look symmetric and are not:
Submit no longer stays enabled on an uncovered boundary
verdictAllowsSubmitpreviously allowed submit whileboundaryCovered === false, documented as safe because "Layer 2 — the host's typed refusal — is the authority".That premise was false. The host's check was presence-only, so a boundary turn published mid-stream is present in the head at its partial version: the fork returned 200 OK and seeded a silently truncated turn.
The host-side currency check lands alongside this in #5014. Both halves are wanted rather than either alone — the publication hook reports
UNKNOWNduring background refetches, so the gate blinks open for one RTT per poll cycle, and the host refusal is what makes that blink a wasted round trip instead of a hole.Preserved deliberately
boundaryCovered: nullis still never collapsed tofalse—nullmeans the question was not answered, and blocking on it would refuse a fork on a question nobody asked.unknownpath for hosts that predate the method stays permissive; an unsupported or unreachable host does not start blocking.Tests
84 tests across three suites, including four mutation probes — disabling the
definitive-first check, treatingundefinedas a reason, disabling the invalidates-published-head gate, and reverting the submit block — each confirmed caught, with production restored anddiff-verified byte-identical afterward.🤖 Generated with Claude Code