From 8f207ab4969f3dc8c51c3e51abcfedeac8ecf065 Mon Sep 17 00:00:00 2001 From: Martin Laws Date: Mon, 3 Aug 2026 14:00:55 -0400 Subject: [PATCH 1/3] Reject non-positive posting IDs and drop duplicates 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. --- internal/cmd/args_test.go | 40 +++++++++++++++++++++++++++++++++++++++ internal/cmd/seen.go | 16 ++++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/internal/cmd/args_test.go b/internal/cmd/args_test.go index bd51c5c3..d3a4a100 100644 --- a/internal/cmd/args_test.go +++ b/internal/cmd/args_test.go @@ -41,3 +41,43 @@ func TestCleanUseLineStripsFlagsSuffix(t *testing.T) { t.Fatalf("cleanUseLine() = %q", line) } } + +func TestParseIntArgsRejectsNonPositive(t *testing.T) { + for _, arg := range []string{"0", "-1", "-99999"} { + if _, err := parseIntArgs([]string{arg}); err == nil { + t.Errorf("parseIntArgs(%q): expected an error, got nil", arg) + } + } +} + +func TestParseIntArgsRejectsNonNumeric(t *testing.T) { + if _, err := parseIntArgs([]string{"abc"}); err == nil { + t.Error("parseIntArgs(\"abc\"): expected an error, got nil") + } +} + +func TestParseIntArgsDeduplicatesPreservingOrder(t *testing.T) { + got, err := parseIntArgs([]string{"3", "1", "3", "2", "1"}) + if err != nil { + t.Fatalf("parseIntArgs: %v", err) + } + want := []int64{3, 1, 2} + if len(got) != len(want) { + t.Fatalf("got %v, want %v", got, want) + } + for i := range want { + if got[i] != want[i] { + t.Fatalf("got %v, want %v (first occurrence order preserved)", got, want) + } + } +} + +func TestParseIntArgsAcceptsValidIDs(t *testing.T) { + got, err := parseIntArgs([]string{"12345", "67890"}) + if err != nil { + t.Fatalf("parseIntArgs: %v", err) + } + if len(got) != 2 || got[0] != 12345 || got[1] != 67890 { + t.Errorf("got %v, want [12345 67890]", got) + } +} diff --git a/internal/cmd/seen.go b/internal/cmd/seen.go index a40866c4..545fae0a 100644 --- a/internal/cmd/seen.go +++ b/internal/cmd/seen.go @@ -101,14 +101,30 @@ func (c *unseenCommand) run(cmd *cobra.Command, args []string) error { return writeOK(nil, output.WithSummary(summary)) } +// parseIntArgs parses posting IDs, rejecting non-positive values and dropping +// duplicates. Zero and negatives are not valid posting IDs, and passing one +// through produces a request against a nonsense path (/postings/0/...) that the +// server can only reject. Duplicates cost a round trip and, for commands where +// the operation is not idempotent, can turn a request the caller got right into +// a reported failure. func parseIntArgs(args []string) ([]int64, error) { ids := make([]int64, 0, len(args)) + seen := make(map[int64]bool, len(args)) + for _, arg := range args { id, err := strconv.ParseInt(arg, 10, 64) if err != nil { return nil, output.ErrUsage(fmt.Sprintf("invalid posting ID: %s", arg)) } + if id <= 0 { + return nil, output.ErrUsage(fmt.Sprintf("invalid posting ID: %d (must be positive)", id)) + } + if seen[id] { + continue + } + seen[id] = true ids = append(ids, id) } + return ids, nil } From b1e1da3ee74561b3f5789568aae8f7fae3dc8afe Mon Sep 17 00:00:00 2001 From: Martin Laws Date: Mon, 3 Aug 2026 14:15:48 -0400 Subject: [PATCH 2/3] Correct the endpoint claim and assert the error message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both from review feedback on #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. --- internal/cmd/args_test.go | 17 ++++++++++++++--- internal/cmd/seen.go | 12 +++++++----- 2 files changed, 21 insertions(+), 8 deletions(-) diff --git a/internal/cmd/args_test.go b/internal/cmd/args_test.go index d3a4a100..3ed4cf73 100644 --- a/internal/cmd/args_test.go +++ b/internal/cmd/args_test.go @@ -43,9 +43,20 @@ func TestCleanUseLineStripsFlagsSuffix(t *testing.T) { } func TestParseIntArgsRejectsNonPositive(t *testing.T) { - for _, arg := range []string{"0", "-1", "-99999"} { - if _, err := parseIntArgs([]string{arg}); err == nil { - t.Errorf("parseIntArgs(%q): expected an error, got nil", arg) + for _, tc := range []struct{ arg, want string }{ + {"0", "invalid posting ID: 0 (must be positive)"}, + {"-1", "invalid posting ID: -1 (must be positive)"}, + {"-99999", "invalid posting ID: -99999 (must be positive)"}, + } { + _, err := parseIntArgs([]string{tc.arg}) + if err == nil { + t.Errorf("parseIntArgs(%q): expected an error, got nil", tc.arg) + continue + } + // The clearer message is the point of the change, so assert it rather + // than just the presence of an error. + if err.Error() != tc.want { + t.Errorf("parseIntArgs(%q) = %q, want %q", tc.arg, err.Error(), tc.want) } } } diff --git a/internal/cmd/seen.go b/internal/cmd/seen.go index 545fae0a..905cf5c3 100644 --- a/internal/cmd/seen.go +++ b/internal/cmd/seen.go @@ -102,11 +102,13 @@ func (c *unseenCommand) run(cmd *cobra.Command, args []string) error { } // parseIntArgs parses posting IDs, rejecting non-positive values and dropping -// duplicates. Zero and negatives are not valid posting IDs, and passing one -// through produces a request against a nonsense path (/postings/0/...) that the -// server can only reject. Duplicates cost a round trip and, for commands where -// the operation is not idempotent, can turn a request the caller got right into -// a reported failure. +// duplicates. Zero and negatives are not valid posting IDs, so including one in +// the posting_ids payload 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 dropped, first occurrence wins — for the bulk +// seen/unseen calls that only trims the payload, but it matters more for any +// caller that issues one request per ID, where a repeat can come back as a +// failure. func parseIntArgs(args []string) ([]int64, error) { ids := make([]int64, 0, len(args)) seen := make(map[int64]bool, len(args)) From 891e3abf53378c21c513ea093f25b9f25ec4fed5 Mon Sep 17 00:00:00 2001 From: Martin Laws Date: Mon, 3 Aug 2026 14:27:14 -0400 Subject: [PATCH 3/3] Assert the message in the non-numeric case too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Applying the review point from #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. --- internal/cmd/args_test.go | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/internal/cmd/args_test.go b/internal/cmd/args_test.go index 3ed4cf73..9a0e8685 100644 --- a/internal/cmd/args_test.go +++ b/internal/cmd/args_test.go @@ -62,8 +62,19 @@ func TestParseIntArgsRejectsNonPositive(t *testing.T) { } func TestParseIntArgsRejectsNonNumeric(t *testing.T) { - if _, err := parseIntArgs([]string{"abc"}); err == nil { - t.Error("parseIntArgs(\"abc\"): expected an error, got nil") + for _, tc := range []struct{ arg, want string }{ + {"abc", "invalid posting ID: abc"}, + {"", "invalid posting ID: "}, + {"1.5", "invalid posting ID: 1.5"}, + } { + _, err := parseIntArgs([]string{tc.arg}) + if err == nil { + t.Errorf("parseIntArgs(%q): expected an error, got nil", tc.arg) + continue + } + if err.Error() != tc.want { + t.Errorf("parseIntArgs(%q) = %q, want %q", tc.arg, err.Error(), tc.want) + } } }