Repository navigation
Conversation
|
@coderabbitai review |
sbddesign
left a comment
There was a problem hiding this comment.
Replacement diff comparison completed against master 8dfa4eedf6fc803d16f798aa3b47f838614b89c4.
For filters #102 (fe97bbe950f1e88c52cb9d0b25b1305c9bfc80d8), compared the replacement tree with git merge-tree --write-tree 8dfa4ee 25eba704. For promotion #103 (16c08ee5fc893d24a19307766653109a1f4a59b7), compared with the merge-tree of 8dfa4ee and 3b0917a. Both comparisons have no differences outside CHANGELOG.md. Each changelog retains master's entries and inserts only its feature entry. Range-diff confirms preservation of the corrected filter guidance and regression.
No new finding from this comparison. The #99 command-contract finding remains closed in #102: omit --event-type for payment entries; explicit payment is rejected. Reviewed the intervening master changes in affected CLI/HTTP paths; no new conflict with the reviewed filter or promotion behavior was identified.
This is a comparison with prior reviews, not a fresh exhaustive review or independent rerun of replacement-head tests/CRAP. Vance records fresh integrated validation in WORK_LOGS/CLI_RELEASE_SYNC_REPLACEMENTS_2026_10_22.md; my independent full-package validation applies to old #99 at 25eba704. CI completion and current-head CodeRabbit approval remain separate gates.
Docs #36 and skills #10 companion descriptions now point to #102/#103. Their released v0.2.0 fallbacks remain unchanged. Both CLI features remain unreleased.
tee8z
left a comment
There was a problem hiding this comment.
Requesting changes on 16c08ee. I found no path that sends a payment twice, sends without --execute, or sends parameters different from the check's:
execute()returnsNotExecutedbeforemutate, so the promotion read never runs without--execute.- The journal hash covers canonical JSON, the organization and the environment.
- The status and
check_onlygate matches backendcheck_payment_approved_or_not_exists, andcheck_only:falseis the right acceptance signal (send/entity_store.rs). - Every request is sent once.
Two things need a fix before merge.
P2: promoting a check whose body has flow_preference reports success, but no payment is sent. Promotion requires the identical body (api.rs:1176), so flow_preference is sent again. A check does not store it: backend origin/master c4ca979, crates/api/src/domain/payments/routes.rs (let flow_preference = if check_only { None } else { req.flow_preference };). matches_send_command compares it (send/entity.rs:186), and handle_send then logs send payment command does not match approved payment ..., ignoring command and returns Ok(()) (payment_command.rs). The API has already answered 202, and without --wait, mutate reports Outcome::Succeeded. The payment stays approved with check_only:true, and --wait completed ends in exit 5. The CLI has no flow_preference flag, so this needs a --data body, which the contract allows. Until the backend keeps the field on checks, refuse to promote a body with flow_preference, say why, and note it in the guide. A backend issue is worth filing as well.
P2: new error paths have no tests, which AGENTS.md requires. Nothing covers the promotion read failing at api.rs:1186 (a 404 for a check the API never stored, a 5xx, or a transport error), or the journal rewrite at api.rs:1227 failing before the POST. The write_faults harness can inject that second failure. Each should show that no POST was sent and what the error tells the user.
Nonblocking:
- A rerun after a promotion does not read the approval again. The journal now says
CreatePayment, soauthorize_check_promotionreturnsOk(false)and the POST goes out with no GET. The test pins that with.expect(2). The backend deduplicates, so nothing is sent twice. Still, the description's "only after the API confirms an approved send with check_only=true" holds only for the first run. Whenpromoted_checkis set, reading first would let a rerun report acceptance oncheck_only:falseinstead of submitting again. Promotion requires an approved send with check_only=true; reconcile this ID before proceedingnames no command. A check still evaluating, a failed check and an already-promoted check each need a different next step. Checks have no--waiteither.- The approval check runs only when the check is in this machine's journal. A check made elsewhere is promoted on the backend's gate alone. The guide should say so.
- The new section in
docs/guides/payments.mdcomes after theRelated:footer.safety-model.mdstill says that the CLI refuses to reuse an ID for a different request. check_onlyis read from raw JSON in three places, and the status is compared as the string"approved". Acheck_only: Option<bool>onPaymentView, and the existing status type, would be clearer. #102 addscheck_onlyto the snapshot, so the contract will cover the field once both merge.
Validation on this head, in nix develop: cargo fmt --check, cargo clippy --locked --all-targets -- -D warnings, cargo machete --with-metadata, cargo test --locked (143 unit, 127 integration) and python3 scripts/check-coverage.py passed. CRAP: 636 measured functions, none at or above 20, highest wait_for_payment at 19.60; 185 functions were unmeasured. CI is green. I did not exercise the flow_preference case against a live backend. It follows from the backend code cited above.
| } | ||
| previous.operation = OperationId::CreatePayment; | ||
| let candidate = JournalRecord::new(operation, scope, body, id)?; | ||
| if !previous.describes_same_request(&candidate) { |
There was a problem hiding this comment.
Requiring the identical body also re-sends flow_preference. The backend drops it from a check (routes.rs: let flow_preference = if check_only { None } else { req.flow_preference };), and then matches_send_command fails. The promotion is ignored after a 202, which this command reports as succeeded. Refuse promotion when the body has flow_preference.
| scope.require_org()?, | ||
| scope.single_env()? | ||
| ); | ||
| let response = api.read(&path).await?; |
There was a problem hiding this comment.
This read can fail with a 404 (the check was never stored), a 5xx, or a transport error. None of these paths has a test, and each should show that no POST was sent and what the user is told.
| return Ok(()); | ||
| if promotion { | ||
| record.promoted_check = true; | ||
| atomic_write(&path, &serde_json::to_vec(&record)?)?; |
There was a problem hiding this comment.
If this rewrite fails, the command must stop before the POST. The write_faults harness can inject the failure. Please add a test that shows no request is sent.
tee8z
left a comment
There was a problem hiding this comment.
Rechecked current head 16c08ee5fc893d24a19307766653109a1f4a59b7 (unchanged since the existing request for changes). I cannot approve yet: both existing blockers remain.
authorize_check_promotionstill permits a non-nullflow_preferencein the identical body. I reverified the backend at current masterf632c59590e71f0c3fca154007dbc113d3ac6078: the check drops the preference, send matching compares it, and a mismatch is ignored. The CLI can report success on HTTP acceptance while the payment remains a check. Reject this promotion until the backend preserves the field; explain the limitation and add a regression. The inline suggestion is a minimal guard.- The new promotion-read failure paths (404/5xx/transport) and failed journal rewrite still lack dedicated tests. Assert no payment POST is sent, and that the journal remains recoverable. The existing
write_faultsharness can cover the rewrite failure.
Independent verification on this head: formatting, full-target Clippy with warnings denied, dependency hygiene, contract/docs check, and full cargo test --locked passed (143 unit + 127 integration). Required whole-package CRAP measurement passed: 636 measured functions, 185 unmeasured, zero scores >=20; highest 19.604615 (wait_for_payment). Scope: src, build.rs, tests; cargo-llvm-cov 0.8.5 and knots 1.16.0, all targets/features, locked/offline.
The backend mismatch is a source-level conclusion, not a live payment reproduction. No staging or production payment was sent. This comment supplements the existing request for changes rather than adding a duplicate blocking review.
| return Err(Error::usage( | ||
| "A check can only be promoted with its original request and scope", | ||
| )); | ||
| } |
There was a problem hiding this comment.
Suggested guard before reading approval. The check endpoint discards a non-null flow_preference, so the same request cannot match the stored approved check when promoted. Preserve the original payload and refuse the promotion rather than silently removing its routing preference. Add regression coverage and document this limitation.
| } | |
| } | |
| if body.get("flow_preference").is_some_and(|value| !value.is_null()) { | |
| return Err(Error::usage( | |
| "Cannot promote a check with flow_preference: the service does not preserve it on checks", | |
| )); | |
| } |
The recovery journal rejects an approved check when the same payment ID is submitted through
payments createorpayments send. Allow the transition only for an identical payload and scope, after the API confirms an approved send withcheck_only=true.Persist the promotion before submission. An interrupted promotion requires an explicit send projection with
check_only=falsefor automatic reconciliation, including after restarting. The old check cannot establish send acceptance. Changed payloads and scope remain rejected; no mutation is automatically retried.Validation: formatting, Clippy with warnings denied, cargo-machete, full
cargo test --locked(143 unit and 127 integration tests), and contract coverage check pass. Tests cover preview without execution, valid promotion, changed payload/scope, incompatible projections, dropped submissions, and restart reconciliation. Whole-package CRAP passes: maximum 19.604615 (wait_for_payment); zero functions at or above 20. Measured 636 functions; 185 functions have zero instrumented lines and remain unmeasured. Scope includessrc,build.rs, andtests, using cargo-llvm-cov 0.8.5 and Knots 1.16.0.Companions: agent skills #10, docs #36. Both target released v0.2.0 and retain direct API fallbacks. These changes are unreleased; no release, merge, deployment, or live payment is included.
Replacement companion: #102. Each PR is independent.
Replaces #100 without rewriting published history. Based on master
8dfa4ee; retains master changelog entries. Original evidence:WORK_LOGS/CLI_RELEASE_SYNC_2026_10_22.md. Fresh integrated validation replaces the old-head results.Origin: Buzz engineering channel
b25e2e33-fcbe-4db7-a726-9a6f7eb4df67, thread20a1cc5bc38385cb798c5464afeb605cfabb38b8011d14f18219c9f2e42179e4.Self-review confirms the diff contains only this feature against current master. No live service/payment acceptance or local cross-platform run. CodeRabbit approval is pending; no
human review wantedlabel.