feat(messages): add --since flag to thread command#135
Conversation
Maps to Slack's conversations.replies `oldest` parameter, allowing callers to fetch only messages newer than a given timestamp. Useful for polling threads without refetching the entire history. Closes #127
TDD Assessment:
|
| Scenario | Covered? |
|---|---|
oldest sent correctly when non-empty |
Yes — TestClient_GetThreadReplies_WithOldest asserts oldest=1700000050.000000 via the server handler |
Empty oldest not sent |
Partial — TestClient_GetThreadReplies_Success passes "" but never asserts the oldest query param is absent from the request |
Pagination with oldest set |
Not tested — only a single page (empty cursor) is exercised in WithOldest |
The missing "empty oldest not sent" assertion is a minor gap. The current test would pass even if the code unconditionally sent oldest=, because the handler only checks that oldest == "1700000050.000000" in the positive case, not that it is absent in the base case.
Command layer (messages_test.go) — gap
None of the three existing TestRunThread_* tests exercise the --since flag at all. opts.since is left at its zero value ("") in every case. There is no test that:
- Passes a non-empty
sincevalue and verifiesoldestreaches the API call. - Verifies the flag default is
""(i.e. omitted from the request by default).
The command layer is the integration point between flag parsing and the client call. Without a test here, a regression that strips or ignores opts.since before calling GetThreadReplies would go undetected.
Other observations
- No validation of the
--sincetimestamp format. The flag is a raw string passed directly to the API. An obviously invalid value (e.g.--since foo) will be forwarded without error. The history command (history.go) has comparable behaviour, so this is consistent, but worth a note if strict UX is desired. sincevsoldestnaming mismatch — the flag is--sincebut the underlying parameter isoldest. This is fine UX-wise, but a comment in the code or a test name making the mapping explicit would help future readers.
Summary
Coverage is adequate at the client layer for the happy path, but has two concrete gaps:
- Client test: No assertion that
oldestis absent when""is passed. - Command test: No
runThreadtest exercisesopts.since != ""end-to-end.
Recommend adding one TestRunThread_WithSince test in messages_test.go that sets opts.since and asserts the oldest query param reaches the mock server, and tightening TestClient_GetThreadReplies_Success to assert r.URL.Query().Get("oldest") == "".
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: c2cb55d
Approved with 1 non-blocking suggestion below. Address at your discretion.
Summary
| Reviewer | Findings |
|---|---|
| security:code-auditor | 1 |
security:code-auditor (1 findings)
💡 Suggestion - internal/cmd/messages/thread.go:29
The flag is named
--sincebutmessages historyuses--oldestfor the same concept (Slack'soldestparameter). Using a different name for the same underlying behavior adds cognitive overhead for users switching between subcommands. Consider using--oldestfor consistency, or document the mapping in the help text.
2 info-level observations excluded. Run with --verbose to include.
Completed in 3m 55s | $0.54
| Field | Value |
|---|---|
| Reviewers | hybrid-synthesis, database:sql-reviewer, security:code-auditor, harness-engineering:knowledge-reviewer, harness-engineering:enforcement-reviewer, harness-engineering:architecture-reviewer, harness-engineering:legibility-reviewer |
| Reviewed by | pr-review-daemon · monit-pr-reviewer |
| Duration | 3m 55s (Reviewers: 52s · Synthesis: 45s) |
| Cost | $0.54 |
| Tokens | 245.9k in / 17.5k out |
| Turns | 23 |
| } | ||
|
|
||
| cmd.Flags().IntVar(&opts.limit, "limit", 100, "Maximum replies to return") | ||
| cmd.Flags().StringVar(&opts.since, "since", "", "Only return messages after this timestamp") |
There was a problem hiding this comment.
🔵 Low (security:code-auditor): The flag is named --since but messages history uses --oldest for the same concept (Slack's oldest parameter). Using a different name for the same underlying behavior adds cognitive overhead for users switching between subcommands. Consider using --oldest for consistency, or document the mapping in the help text.
There was a problem hiding this comment.
--since was the name requested in #127. It is also more intuitive for the polling use case (matches docker logs --since, git log --since). The --oldest name on history mirrors Slack API parameter naming, a different convention.
…138) ## Summary - **README**: Added canvas command section, `--since` flag on thread, canvas alias in aliases table - **CHANGELOG**: Added entries for canvas (#137), `--since` (#135), edited metadata (#136), truncation fix (#134) - **integration-tests.md**: Added Part 10 (Canvas Tests), enhanced Part 3.4 with `--since` and full-text verification, enhanced Part 3.5 with `[edited]` indicator checks ## Test plan - [x] Docs-only change, no code modified
Summary
--since <timestamp>flag toslck messages threadthat maps to Slack'soldestparameter onconversations.repliestsgreater than the given value — useful for polling threads without refetching all history--oldestflag onmessages historyCloses #127
Test plan
TestClient_GetThreadReplies_WithOldest— verifiesoldestparam is sent to APIGetThreadRepliestests updated and passingmake lintclean