Reject non-positive posting IDs and drop duplicates - #155
Open
martinlaws wants to merge 3 commits into
Open
Conversation
parseIntArgs is shared by `seen` and `unseen`, and accepts anything
that parses as an int64. So `hey seen 0` and `hey seen -5` are
currently sent to the server, which can only reject them:
POST /postings/0/seen.json
POST /postings/-5/seen.json
Zero and negatives are not valid posting IDs, so this is a request the
client already knows will fail. Rejecting it locally saves a round trip
and gives a clearer message than whatever the API returns.
Duplicates are dropped, first occurrence wins. For `seen` that just
saves work. It matters more for any command whose operation is not
idempotent, where the second attempt on the same ID comes back as a
failure and turns a request the caller got right into a reported error.
Both are behaviour changes, but only for input that could not have
succeeded or that asked for the same thing twice.
Contributor
There was a problem hiding this comment.
Pull request overview
Pull request overview
Validates posting IDs locally and deduplicates them before seen/unseen SDK calls.
Changes:
- Rejects zero and negative posting IDs.
- Deduplicates IDs while preserving order.
- Adds unit tests for parsing behavior.
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.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
internal/cmd/seen.go |
Adds positive-ID validation and deduplication. |
internal/cmd/args_test.go |
Tests parsing, validation, and ordering. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Both from review feedback on basecamp#155. The doc comment described the request as hitting /postings/{id}/..., which is not what this helper feeds. MarkSeen and MarkUnseen make one bulk POST to /postings/seen(.json) with posting_ids in the body, per seen_test.go:19-32. The argument for rejecting locally is unchanged — an invalid ID still goes into that payload — but the comment now describes the request the helper actually produces. TestParseIntArgsRejectsNonPositive only checked that an error existed, so it would have passed if the clearer message regressed to a generic one. The message is the user-facing part of this change, so it is now asserted per case.
Applying the review point from basecamp#155 consistently rather than only where it was raised. TestParseIntArgsRejectsNonNumeric had the same gap — it checked that an error existed but not what it said, so it would have passed if the message regressed. Also covers empty string and a float, both of which fail ParseInt and should report the same way.
martinlaws
added a commit
to martinlaws/hey-cli
that referenced
this pull request
Aug 4, 2026
Closes basecamp#138. `hey box` hands out posting IDs and `hey seen` already acts on them in bulk, but filing what you found still meant opening the web app. This adds the missing verb: hey move <box> <posting-id>... Box first, matching `hey box <name|id>` — a variadic ID tail cannot coexist with a trailing destination. Short aliases (feed, trail, aside, later) work alongside the canonical names. The SDK's typed move methods (Postings().MoveToFeed() and its siblings) generate per-posting routes that are gone from production: every /postings/{id}/... path answers 404 against app.hey.com with a token that succeeds on /postings/seen.json for the same ID. This uses the route the web app uses instead: POST /postings/moves?box_id={id} {"posting_ids": [...]} It takes the whole batch in one request and answers 200 whatever the IDs were, confirming each posting that moved with a <turbo-stream action="remove" target="posting_{id}"> element; IDs it does not recognize are silently absent. Box IDs are per-account, so the destination kind is resolved with one GET /boxes.json first. Notes on the less obvious decisions: - `imbox` and `trash` are rejected up front with their own messages. There is no trash route to call at all, and keeping moves one-way reads as a product decision rather than a CLI one. - Every ID gets a status: moved, not_found, or failed, and would_move under --dry-run, which makes no requests at all. Exit 0 means every ID moved; any other exit means at least one did not, and the hint names those IDs grouped by status. There is deliberately no exit code meaning "your IDs were stale" — the route answers 200 either way, and a scripted caller should not lean on this command telling a batch of dead IDs apart from a response it could not read. - A 404 from the route itself is reported as an API error naming the route. Posting IDs never 404 this endpoint, so a bare "not found" would point the caller at their own IDs. - An unreadable response body reads as failed with an explicit "cannot confirm", never not_found. The moves may have happened. - Auth, permission, rate-limit and server errors fail the batch as a unit, since it is one request. The upstream exit code and its recovery hint are preserved. - The payload is a slice so --ids-only and --count work. An object payload would have been rejected by the writer *after* the postings had already moved, reporting a usage error for work that succeeded. apierr.Error carries no arbitrary fields, so on failure paths the per-ID breakdown goes into the hint rather than the data. One known gap: a repeated ID is counted once for each time it appears, so `hey move feedbox 111 111` reports two moves for one posting. The posting itself moves once. basecamp#155 is adding de-duplication to the shared parseIntArgs, which this inherits when it lands. Tests follow the seen_test.go httptest pattern and cover box-ID resolution per destination, alias resolution, imbox and trash rejection, arity, non-numeric ID rejection, unrecognized-ID reporting, a batch where nothing is confirmed, request-level failures per status code, the route-404 remap, the unreadable-response guard, prefix-ID false matches, --dry-run and --ids-only/--count. `make check` and `make race-test` pass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
parseIntArgsis shared byseenandunseen, and accepts anything that parses as anint64. Sohey seen 0andhey seen -- -5are accepted locally and go into theposting_idspayload of the bulk request:Zero and negatives are not valid posting IDs, so this asks the server to act on something the client already knows is invalid. Rejecting locally gives a clearer message than whatever comes back:
Duplicates are also dropped, first occurrence wins. For the bulk
seen/unseencalls that only trims the payload. It matters more for any caller that issues one request per ID, where a repeat can come back as a failure and turn a request the caller got right into a reported error.Both are behaviour changes, but only for input that could not have succeeded, or that asked for the same thing twice.
Four tests added in
args_test.go, asserting the messages rather than just the presence of an error. Existingseen/unseentests unchanged and passing.make checkpasses.I found this while adding a
hey movecommand locally — moves are one request per ID rather than one bulk call, which made both gaps visible. That's coming as a separate PR; this one stands on its own and touches only the shared helper.Built with Claude Code, which is also how I ran into it. The design calls and the review are mine.