Repository navigation
fix(pr): settle a PR-review plan when Publish to PR is verified - #293
Conversation
Publish to PR pushes the commit that carries a review fix, but only Apply and Sync replies ever cleared the plan's "not yet on the remote" marker (fix_uncommitted). After a verified publication the plan kept calling the fix undelivered and Sync replies stayed unavailable until Apply ran again. When a publication is recorded as Published, including one settled by reconciling an earlier pending push, the remote branch it just read now settles the plan with the same per-item rule Apply and Sync use: the fix's commit must be in that branch and its recorded effect still in the tip. The marker is cleared in the same board write as the Published record, so a restart finds both. Refused, pending and unknown outcomes settle nothing, a repeat answered from an existing record reads nothing, and no reply is posted. The per-item delivery rule is split out of settle_delivery so both callers share it.
… pull request When a publication request finds an earlier Pending or Unknown record and settles it against the remote, the remote branch it read is the record's. The review plan belongs to the Task's own branch and pull request, so the plan is now settled from that read only when the record names the same branch and pull request as the confirmed request. For any other record the reconciled record is written as before and the plan is left alone.
|
No actionable comments were generated in the recent review.
 |
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly describes the primary correctness change: settling a PR-review plan when Publish to PR verification succeeds. |
| Description check | |
The description directly explains the changes to publication verification, branch and pull-request identity checks, reconciliation, and clearing fix_uncommitted. |
| Persistent State Integrity | |
PASS. Changed publication writes use lifecycle::record_typed through write_publication_settling. The lifecycle stages changes, saves the board, and updates shared memory only when the save succeed… |
| Recoverable Resource Lifecycle | |
PASS. The pull request does not change worktree, migration, or cleanup lifecycle implementation. It only clears fix_uncommitted after verified remote delivery, while retaining fix_commit and `fix_… |
| Platform Boundary Safety | |
The changed production paths use platform-neutral Rust APIs and existing repository abstractions. The added Unix-specific test hook uses std::os::unix::fs::PermissionsExt, a Unix shell hook, and is … |
Full details: Persistent State Integrity
-
Explanation
PASS. Changed publication writes use
lifecycle::record_typedthroughwrite_publication_settling. The lifecycle stages changes, saves the board, and updates shared memory only when the save succeeded or the file replacement was published; all callers propagate write errors instead of reporting success. The plan settlement uses the current task staged under the write lock and checks the expected publication record plus item identity before clearingfix_uncommitted, so it does not replace a newer task snapshot with the stale read. The changed paths therefore show no explicit persistent-state integrity failure.
Full details: Recoverable Resource Lifecycle
-
Explanation
PASS. The pull request does not change worktree, migration, or cleanup lifecycle implementation. It only clears
fix_uncommittedafter verified remote delivery, while retainingfix_commitandfix_effect. The publication record and marker update use one compare-and-set board write. A failed write leaves the prior state unchanged, or reportsPublishedNotDurable; the next request reconciles the pending record. Existing tests cover this retry path.
Full details: Platform Boundary Safety
-
Explanation
The changed production paths use platform-neutral Rust APIs and existing repository abstractions. The added Unix-specific test hook uses
std::os::unix::fs::PermissionsExt, a Unix shell hook, and is gated with#[cfg(unix)]; its only test caller is also#[cfg(unix)]. The added Git/filesystem test setup uses portablestd::process::Commandandstd::fsAPIs. No changed code introduces an ungated Windows- or Unix-specific API or an evident cross-platform compile failure.
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.
… branch A published record answered a repeated request, and a settled earlier record answered a request that had reconciled it, when only the commit and the pull request matched. The same commit and pull request published from another branch was then reported as the branch the person confirmed, and the panel accepted it. Replay and the post-reconcile answer check now compare the branch as well. A record for another branch is still settled against its own remote, but it neither answers nor publishes the current request, which is refused so the person can confirm it again. The panel applies the same identity to the answer it receives.
The replay and publish documentation still described a record as matching on the commit and pull request alone. Say that the branch is part of the identity too.
Summary
fix_uncommitted). After a verified publication the plan kept reporting the fix as undelivered and Sync replies stayed unavailable until Apply ran again.Published(including one settled by reconciling an earlier pending push), the remote branch it just read now settles the plan with the same per-item rule Apply and Sync use: the fix commit must be in that branch and its recorded effect still in the tip. The marker is cleared in the same board write as thePublishedrecord, as a compare-and-set on the item, so a restart finds both or neither.settle_deliveryso both callers share it. Its behavior is unchanged.Testing
Run locally with
GIT_CONFIG_GLOBAL=/dev/null, against real local Git repositories and a bare remote (only GitHub's answer is the existing test stand-in):cargo test -p slashit-ui --lib commands::pr: 405 passed (the new reconcile and branch-identity tests failed before their changes)cargo test -p slashit-ui --test pr_review_integration: 27 passedcargo test -p slashit-frontend: 296 + 3 passedcargo clippyin the CI form (-D warnings)cargo fmt --check: not run;publication.rsandpr.rsare already hand-formatted differently from rustfmt onmaintrunk build: not run locally; left to hosted CINotes
git push, or aPublishedrecord written before this change) are not repaired here; their plans settle on the next Apply or Sync. Whether Sync should also be enabled for items that have a fix commit but are not yet marked delivered is a UI policy question and is not changed here.Publishedrecord for the same commit and pull request, next to the refusal. The backend answer is correct; making the panel's status line branch-aware is left out of this change.