Repository navigation
Conversation
There was a problem hiding this comment.
🟢 Approved
The narrow fix preserves error propagation and handoff behavior, with regression coverage for both save outcomes and no unresolved findings.
No open issues 🎉
What changed in this PR
This PR fixes misleading connector logs by reporting an old request as discarded only after its ledger state is saved. It retains the narrow fix from #825 without changing the handoff age limit or adding testing infrastructure.
Changes:
- Return ledger write errors before emitting the discard log.
- Add regression coverage for successful and failed saves, confirming neither case hands off the old request.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
| File | Description |
|---|---|
internal/connector/handoff.go |
Log discarded requests only after successful persistence. |
internal/connector/handoff_test.go |
Test logging and output for both save outcomes. |
Review effort: Balanced
|
Ready for review on ef7b223: local CI passes and all 31 hosted checks are accounted for (26 successful, 4 conditionally skipped, 1 neutral; none failed or pending). Copilot approved this exact head. There are 0 unresolved review threads and 0 unanswered findings. Cubic's fresh review is unavailable because its monthly quota is exhausted. Nothing merged. |
|
🤖 Local review at ef7b223: clean. CI is green on it (27 passed, 4 skipped, 1 neutral). Writing the discard first and logging after (handoff.go L246-L252) keeps the error return and prints nothing for the old request either way. I put the old order back and ran the new test: #928 also edits the age check at the top of |
…p#830) * Make checkins question notify change what it says it changes The command sent notify_on_answer and digest_include_unanswered through the SDK. Basecamp reads only responding and subscribed, ignores anything else, and answers 200 with the settings unchanged, so the command reported success and changed nothing. Pin basecamp-sdk to the commit that sends the right fields (basecamp-sdk#959) and resync the MCP model from it. --on-answer now sets subscribed, and --responding / --no-responding replace --digest-include-unanswered, which named a setting Basecamp does not have; the removed flags are recorded in .surface-breaking. The summary now states the settings Basecamp answered with, and the command fails when a requested setting did not take, rather than claiming an update. * Explain a refused change only by the rule that applies to it, and update the Nix vendor hash The agent-account hint fits only a --responding that did not take; any other mismatch gets a generic hint. The summary says the question asks you rather than that you are asked it. The SDK pin changed go.sum, so nix/package.nix needs the new vendorHash. * Reword the test comment on which keys bc3 reads * Describe a mismatch as different settings, not unchanged ones With both settings requested, Basecamp can apply one and refuse the other, so the generic hint cannot claim the old settings were kept. * Pin the SDK to v0.22.0 basecamp-sdk#959 shipped in v0.22.0. The vendored MCP model, catalog snapshot and Nix vendorHash move with it. The release also carries basecamp#960's MoveRecordingToVault, which the catalog now serves as move_recording_to_vault, so the served operation count rises to 275.
…basecamp#831) * Add files move: move a document, upload or folder into another folder bc3 #13644 documented POST /recordings/:id/filing.json, which moves a document, upload or folder into another folder in the same project, in place: the ID, URL and comments stay the same. basecamp-sdk basecamp#960 models it as Recordings().MoveToVault. basecamp files move <id|url> --to <folder-id|url> [--position N] Both the item and the folder accept an ID or a Basecamp URL. Before anything is sent, the command refuses an item URL that isn't a document, upload or folder, a --to URL that isn't a folder, two URLs in different projects, a position below 1, and --vault/--folder, which the files group reads as a listing filter and which would otherwise be ignored here. The success line names the item and the folder ("Moved Spec into Designs"), falling back to IDs when those lookups fail, since the move has already happened. An item URL names its type, so only that lookup runs. JSON data is {id, status: "moved", folder_id, position?}, the shape of cards move. Refusals say what to change. A 422 carries bc3's reason when it sends one, and the hint lists the rules for the ones it doesn't. A 403 points at the account's move restriction. A 404 says the folder must be in the item's project. The SDK is pinned to the basecamp#960 head until it merges and is released, with the vendored MCP model synced from that commit (the catalog gains move_recording_to_vault) and the Nix vendorHash recomputed. * Hold pasted URLs to the session's host and account; tighten the move's inputs The URL router is host-agnostic and keeps only the IDs, so a look-alike URL, or one from another account, would have moved same-numbered records in the configured account. files move now checks each URL the way files replace does: a trusted Basecamp host before parsing, and the session's account after it is resolved. The item and --to share one parser that also refuses collection URLs (…/vaults/456/vaults would otherwise have meant folder 456) and IDs below 1. The smoke test creates its own folder and upload, since bats -j may run the file's tests in any order. --position's help says a position past the end means last. The SDK pin moves to basecamp-sdk#960's head, whose operation summary now mentions reordering; the MCP catalog line follows. * Compare a pasted URL's account canonically, and name the v0.22.0 pin in API-COVERAGE A zero-padded spelling of the session's account is the same account, so files move now compares with accountIDsEqual instead of raw strings. API-COVERAGE still described the SDK pin as v0.21.0 plus basecamp-sdk#960; main pins the v0.22.0 release that shipped it. * Compare the two URLs' projects as numbers Two zero-padded spellings of one bucket name one project, so the same-project check parses both IDs instead of comparing strings.
Summary
Keep the narrow correctness fix from the retired restart-testing work: log that an old request was discarded only after the ledger write succeeds. A failed write previously left a success message in the log.
No new harness, timing knob, or change to the handoff age limit.
Verification
bin/cipasses.Summary by cubic
PR Description
Fixes the handoff discard logging so a completion message is only emitted after the ledger write succeeds.
Previously, a request from before the run started was logged as "not handed off" before its discarded state was saved, so a failed save left a misleading success message in the log. Now the state is written first and the log line appears only if the write succeeds. Adds a test covering both save success and save failure.
Summary
Changes the handoff discard path so the discarded state is saved before the "not handed off" log line is written. A failed save now returns the error without logging a success message, and a new test verifies both the success and failure cases.
Written for commit ef7b223. Summary will update on new commits.