Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends basecamp todos update to support clearing nullable todo fields (due date, start date, description) via dedicated --no-* flags and by treating explicitly empty flag values (e.g. --due "") as “clear”.
Changes:
- Add
--no-due,--no-starts-on,--no-descriptionflags and document clearing examples in help text. - Detect and error on conflicting clear/set usage (e.g.
--no-duewith a non-empty--due). - When clearing is requested, bypass the SDK typed request and issue a raw PUT with
nullvalues.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8e9523169
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
8325fdb to
e5d3707
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8325fdb7c6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="e2e/smoke/smoke_todos_write.bats">
<violation number="1" location="e2e/smoke/smoke_todos_write.bats:119">
P2: The new preservation smoke tests do not assert all fields they claim to preserve, so starts_on/content regressions can pass unnoticed.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ed8d807a6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
5ed8d80 to
9637ea2
Compare
Review carefully before merging. Consider a major version bump. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 22 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 22 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b67390e749
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
6ea302a to
d2df13a
Compare
b67390e to
b824e1c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b824e1c051
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…scription) The SDK's UpdateTodoRequest uses omitempty on value types, making it impossible to send JSON null to clear a field. When clearing is needed, bypass the typed SDK path and issue a raw PUT with nil map values — the same pattern the TUI uses in hub.go. Supports both explicit flags (--no-due) and empty values (--due ""). Conflicting usage (--no-due --due "friday") returns a clear error. Closes #293
Guard against nil Bucket in the clearing path. Add tests covering: null emission for each --no-* flag, empty-value clearing (--due ""), conflicting flag combinations, and combined clear+set operations.
The BC3 API clears fields by omission, not by sending null. Rebuild the PUT body with all existing values preserved, omitting only the fields being cleared. Clearing due also clears starts (Basecamp enforces starts <= due). Updates mock GET responses to include realistic todo data so clear-path tests can verify field preservation.
Basecamp requires a due date when a start date is set, so clearing the due while setting a start is contradictory. Return a clear usage error instead of silently dropping the starts-on value.
Two new live-API smoke tests resolve the contract dispute between the SDK typed-update path and the raw-PUT clearing path: 1. Update title only → verify due_on, starts_on, description survive 2. Clear due via --no-due → verify due/starts cleared, description preserved Uses raw API GET to check null fields that the SDK omits via omitempty.
Move the clearDue+startsOn conflict check after clear detection so --due "" --starts-on "value" is caught alongside --no-due --starts-on. Add missing starts_on assertion to the field preservation smoke test.
Smoke tests: use todos update for --starts-on (create lacks it), match description as HTML substring (API wraps in <p> tags). Unit tests: add --starts-on "" and --description "" empty-value clearing coverage alongside the existing --due "" test.
b824e1c to
e397f06
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
--no-due,--no-starts-on,--no-descriptionflags tobasecamp todos updatefor clearing fields--due "",--starts-on "",--description ""--no-due+--due "friday") and returns a clear errorBucketin the clearing pathTest plan
basecamp todos update <id> --no-dueclears the due datebasecamp todos update <id> --due ""also clearsbasecamp todos update <id> --no-due --title "New"clears date + sets title in one callbasecamp todos update <id> --no-due --due "friday"returns usage errorbasecamp todos update --helpshows the clearing flags--due "friday",--title "x") is unchangedCI note
TestSurfaceSnapshotfailure is pre-existing on main — the unit snapshot baseline has accumulated stale ARG removals unrelated to this PR. The merge-gating "CLI Surface Check" job (which runscheck-cli-surface.shseparately) passes green.Closes #293