Skip to content

Add opt-in conflict checks before body replacements - #1020

Open
robzolkos wants to merge 2 commits into
mainfrom
cli-cleanup/body-guard-10292437212
Open

robzolkos wants to merge 2 commits into
mainfrom
cli-cleanup/body-guard-10292437212

Conversation

@robzolkos

@robzolkos robzolkos commented Nov 9, 2026 •

Copy link
Copy Markdown
Collaborator

What

Adds optional --if-unmodified-since <updated_at> and explicit --force conflict override to the supported typed replacement commands. The caller must retain the timestamp from the representation used to prepare the edit. A server-revalidated GET requires equal parsed timestamp instants before uploads or writes; failed reads and missing, zero or invalid timestamps refuse even with force.

Coverage: comments, messages, to-dos, cards, to-do lists, schedule entries, check-in answers, chat lines, documents/uploads/vaults, file replacement, Doors, projects, templates, notes, gauges and timesheet entries. See exact coverage and limitations.

Boost editing is explicitly excluded and unchanged. Its supported API/SDK representation has no updated_at baseline. Creation time, a parent timestamp and ETags are not substituted.

Limits

  • Opt-in only: no baseline, including first use, remains unprotected. A caller-supplied timestamp is not a remembered read baseline.
  • Raw API writes, MCP/TUI/SDK callers, creation commands and other update paths are not globally guarded.
  • GET then PUT is not atomic: an intervening edit after the check can still be lost. Whole-record comparison can conservatively refuse unrelated field edits.
  • No local authored-body ledger, append mode, server conditional writes or automatic baseline store is implemented.

Verification

  • Full local mise exec golangci-lint@2.15.0 ruby@3.4.12 -- bin/ci, including Go regression and BATS integration suites. Zizmor ran in its default offline mode; platform/credential helper skips are not live-platform coverage.
  • Focused guard regressions include stale/future/fractional baselines, equivalent offsets, missing/malformed live versions, failed GETs, force prerequisites, explicit clears and attachment/file-only updates refusing before side effects. Boost exclusion has a regression test.
  • Real isolated development runtime, using synthetic fixtures and the supported flat Doors JSON API: comments stale refusal with exact unchanged readback, matching success, override, no-baseline and first-use behavior; Doors stale refusal, matching update/clear success and override; file replacement stale description/clear/file-only refusal and matching clear; malformed/missing-target prerequisites; raw API remaining unguarded.
  • The added comments smoke test ran unchanged in an isolated retained harness and passed. Source review found no actionable defects; it is separate from runtime verification.

Safe reproduction

On a disposable record you own, read and keep its updated_at, make an intervening edit, then try a replacement with the older timestamp:

basecamp comments show <synthetic-comment-id> --json
basecamp comments update <synthetic-comment-id> 'replacement' \
  --if-unmodified-since '<saved-updated_at>' --json

The command must refuse with a conflict and leave the record unchanged. Re-fetch, reconcile and use the timestamp belonging to the reconciled edit for a matching update. Only use --force deliberately to overwrite a mismatch; it does not waive prerequisites.

No production/beta validation, merge, release or deployment is claimed.


Summary by cubic

Adds an opt-in conflict guard to typed replacement commands so callers can refuse to overwrite records edited since their last read. When --if-unmodified-since <updated_at> is set, the CLI runs a server-revalidated GET and requires the live updated_at to parse to the same instant before writing; --force overrides a version mismatch, while failed reads and missing, zero, or invalid timestamps refuse even with force.

The guard covers comments, messages, todos and to-do lists, cards, schedule entries, check-in answers, chat lines, files/documents/uploads/vaults (including the file, docs, documents, vault, and folders aliases), Doors, projects, templates, notes, gauges, and timesheet entries, and checks the whole record rather than only body edits. User and agent guidance now instructs reading the record's updated_at and passing it unchanged as the baseline.

Limits

  • No baseline, including first use, leaves writes unprotected; a supplied timestamp is not remembered.
  • GET then PUT is not atomic, so an edit after the check can still be lost.
  • --force requires a baseline and does not waive invalid input or unavailable live state.
  • Boost editing is explicitly excluded and unchanged because its API/SDK representation has no updated_at.
  • Raw API, MCP/TUI/SDK callers, creation commands, and other update paths stay unguarded.

Written for commit 36d89ea. Summary will update on new commits.

View guided diff Turn on auto-fix

@robzolkos
robzolkos requested a review from a team as a code owner November 9, 2026 02:07
Copilot AI balanced review requested due to automatic review settings November 9, 2026 02:07
@github-actions github-actions Bot added commands CLI command implementations tests Tests (unit and e2e) skills Agent skills docs labels Nov 9, 2026

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 21 files

Reply with feedback, questions, or to request a fix.

View guided diff | Turn on auto-fix | Re-trigger cubic

Comment thread internal/commands/comment.go Resolved
Comment thread skills/basecamp/SKILL.md Resolved
Comment thread e2e/smoke/smoke_comments.bats Resolved
Comment thread e2e/smoke/smoke_comments.bats Resolved
Comment thread skills/basecamp/SKILL.md Resolved
Comment thread internal/commands/chat.go Resolved
Comment thread docs/body-conflict-guard.md Resolved
Comment thread internal/commands/projects.go Resolved
Comment thread skills/basecamp/SKILL.md Resolved
Copilot AI previously approved these changes Nov 9, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approved

No active review issues block the opt-in conflict-check changes.

No open issues 🎉

What changed in this PR

Adds opt-in conflict checks to supported Basecamp CLI replacement commands, using the caller’s saved updated_at timestamp to detect intervening edits before uploads or writes. Protection remains optional and non-atomic; Boost editing is unchanged.

Changes:

  • Adds shared --if-unmodified-since validation and a --force override for timestamp mismatches only.
  • Integrates checks across supported update commands, with regression tests and a comments smoke test.
  • Documents coverage and limitations, updates agent guidance, and refreshes the CLI surface snapshot.

[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.

File Description
skills/​basecamp/​SKILL.md Guides agents to retain timestamps and use guarded replacements.
internal/​commands/​todos.go Guards to-do updates before edit processing.
internal/​commands/​todolists.go Adds version checks to to-do list updates.
internal/​commands/​timesheet_entries.go Guards timesheet entry updates.
internal/​commands/​templates.go Adds version checks to template updates.
internal/​commands/​schedule.go Guards schedule updates before attachment processing.
internal/​commands/​projects.go Adds version checks to project updates.
internal/​commands/​notes.go Guards note replacement, including missing-version handling.
internal/​commands/​messages.go Adds version checks to message updates.
internal/​commands/​gauges.go Guards gauge needle description updates.
internal/​commands/​files.go Guards file replacement and document, upload, and vault updates.
internal/​commands/​doors.go Adds version checks to Door updates.
internal/​commands/​comment.go Guards comment replacement before content processing.
internal/​commands/​checkins.go Guards check-in answer updates before image processing.
internal/​commands/​chat.go Adds version checks to chat line updates.
internal/​commands/​cards.go Guards card updates before mutation preparation.
internal/​commands/​body_guard.go Implements shared flags, timestamp validation, and conflict checks.
internal/​commands/​body_guard_test.go Covers refusals, overrides, matching timestamps, and exclusions.
e2e/​smoke/​smoke_comments.bats Exercises matching updates, stale refusal, and explicit override.
docs/​body-conflict-guard.md Documents usage, exact coverage, and safety limitations.
.surface Records guard flags on supported commands and aliases.

Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings November 9, 2026 02:19
Copilot AI dismissed their stale review, a newer Copilot review was requested November 9, 2026 02:19
@robzolkos

Copy link
Copy Markdown
Collaborator Author

🤖 Review follow-up on 36d89eafb0d6abc8f1d87f484c152e014c7ee38d:

  • Tightened smoke read-success/structured JSON assertions, restored target and notification guidance, broadened the skill wording to guarded updates, removed its unshipped documentation reference, and documented file aliases.
  • Declined the comment nil-response and chat nil-response claims: the pinned SDK returns an error for absent JSON200. Added null-record/HTTP204 regressions across guarded variants to verify refusal without panic or writes.
  • Left project-read reuse as a performance optimization outside this slice; either read failing already refuses the update.

Full local bin/ci passed on this committed head. Its rebuilt executable passed 18 real isolated synthetic runtime cases, including Doors stale refusal, matching update/clear and override, plus the strengthened unchanged-byte smoke test. Run-owned development grants were revoked and own servers stopped. These are separate from the live GitHub checks and automatic reviews. Boost is excluded; absent baselines stay unprotected; GET then PUT remains non-atomic.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approved

No active review issues are reported that would block approval.

No open issues 🎉

Review effort: Balanced

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

commands CLI command implementations docs skills Agent skills tests Tests (unit and e2e)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants