Skip to content

fix(cli): give the comment page cursor a consumer on show and inspect - #200

Open
andrei-hasna wants to merge 1 commit into
mainfrom
65b80fa4
Open

fix(cli): give the comment page cursor a consumer on show and inspect#200
andrei-hasna wants to merge 1 commit into
mainfrom
65b80fa4

Conversation

@andrei-hasna

@andrei-hasna andrei-hasna commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Fixes the half of todos task 65b80fa4 that survives measurement. Part of the reported premise is refuted below — please read that section.

The defect

show and inspect emit:

"comments_page": {"count":100,"limit":100,"has_more":true,
                  "next_cursor":"eyJjcmVhdGVkX2F0Ijoi...","pagination_supported":true}

and both called cloudListComments(cloud, id) with no options — while that reader has accepted { limit, cursor } all along and is unit-tested for it (cloud-router.test.ts:619). Nothing anywhere in the CLI could spend the cursor:

  • todos show --help lists no options beyond -h
  • --cursor / --comments-cursor rejected as unknown options
  • no comments verb; comment is write-only
  • history carries only field changes; timeline --task <id> returns 0 rows

So every comment older than the newest page was unreachable from the CLI.

Measured, on a live 125-comment task (0ba8ff46), before the fix

default, no flags   -> count 100  has_more true
                       FIRST 2026-08-01T16:41:26.635Z   LAST 2026-08-02T02:58:00.964Z

25 comments existed and no command could read them.

After the fix, same live task, CLI built from this branch

--comments-limit 1  -> count 1   ONLY 2026-08-02T02:58:00.964Z 281a1b1e

--comments-cursor <next_cursor from page 1>:
    page2 count 25   {"count":25,"limit":100,"has_more":false,"next_cursor":null,"pagination_supported":true}
    page2 FIRST 2026-08-01T16:02:58.282Z   page2 LAST 2026-08-01T16:37:01.143Z
    overlap between pages : 0
    total distinct walked : 125
    ALL page2 strictly older than page1 FIRST : True

--comments-limit 501     -> rc=1  --comments-limit must be an integer between 1 and 500
--comments-cursor '!!bad!!' -> rc=1  --comments-cursor is not a valid comment cursor; pass the value from comments_page.next_cursor

PARTIAL REFUTATION of the reported premise — the direction was backwards

The task title says "comments cap at 100 oldest-first … so the newest comment on a busy task is unreachable." The unreachability claim is false, and the fix would have been wrong if built to that description.

A page is the NEWEST limit comments in ASCENDING display order. The newest comment is the last array element and was always reachable. next_cursor walks toward older history — that is the direction that was blocked.

The discriminating probe is --comments-limit 1: if the page were the oldest 100, a one-row page would return the oldest comment. It returns the newest (281a1b1e, above).

Positive control on a row whose newest comment was known independently (written minutes before the probe):

page1 LAST      : 2026-08-05T14:56:57.183Z  Silvanus  Picked up 65b80fa4...
limit=1 returns : 2026-08-05T14:56:57.183Z  Silvanus  Picked up 65b80fa4...
limit=1 == page1 LAST ? true
limit=1 == page1 FIRST? false

The probe can fail: an earlier run against a mistyped UUID returned HasnaHttpError … -> 404 {"error":"task not found"} and produced no rows.

Source agrees, independently of the live read: interfaces.ts:371 documents before as "strictly older than"; postgres-adapter.ts:238 sorts ascending; v1.ts over-fetches limit + 1 and slices from the front, which only works if the store returns the newest limit + 1 ascending. The human render already said it correctly — task-commands.ts prints "newer page shown; older comments available".

Consequence for anyone triaging on this: a task reading as stale from its newest comment is genuinely stale; that reading was never a cap artefact.

What is in this PR

  • --comments-limit <n> (1–500) and --comments-cursor <cursor> on show and inspect.
  • Cloud path passes them to the already-capable cloudListComments.
  • Local SQLite path gets the same semantics via a shared pure pager. Without a flag the local shape is unchanged — complete history, no comments_page — so existing local consumers see nothing new.
  • Cursor codec moved from src/server/v1.ts to src/lib/comment-cursor.ts so the CLI decodes the cursors the server mints. Server behaviour unchanged; one definition of the keyset instead of two.

What is deliberately NOT in this PR

last_activity_at. It is the fix for the other half of the original question — telling a live row from a stale one — and it is filed separately as 01640a5e with the evidence. It belongs on list (the sweep surface, which carries no comment data), so it needs a server + storage change across both backends and a design call on cost. On show the caller already holds both signals.

Tests

src/cli/comment-page-cursor.test.ts — 5 cases, spawning the real CLI against a mock /v1 that reproduces the measured keyset semantics, over a 125-comment fixture:

  1. show walks the entire history through next_cursor (100 + 25 = 125 distinct, zero overlap)
  2. --comments-limit 1 returns the newest comment and sends limit=1
  3. inspect accepts the same flags
  4. out-of-range limit and malformed cursor fail with actionable messages
  5. local store pages and emits a walkable cursor; unflagged output stays complete

Written red-first. One arm initially passed vacuouslyexpect(stderr).toMatch(/comments-limit|between 1 and 500/) is satisfied by error: unknown option '--comments-limit' before any fix exists — so it now also asserts not.toMatch(/unknown option/i). After tightening: 0 pass, 5 fail, failing with error: unknown option '--comments-cursor'. After the fix: 5 pass, 0 fail, 57 expect().

bun run typecheck → rc=0.

Reviewer notes — two things to attack

  1. Ordering under identical created_at. getTaskWithRelations returns local comments ORDER BY created_at with no tiebreak, while the pager sorts by (created_at, id) — the portable keyset the cursor requires, matching what local-sqlite.ts already does. For comments sharing a timestamp the paged order can differ from the unpaged order.
  2. The local cap is 500, matching the server and cloudListComments, while the local-sqlite adapter internally permits up to 1001. Deliberate, for one consistent documented bound.

Agent: Silvanus


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

`show` and `inspect` emit `comments_page` with `has_more: true`, a
`next_cursor` and `pagination_supported: true`, and both called
`cloudListComments(cloud, id)` with no options — while that reader has
accepted `{ limit, cursor }` all along and is unit-tested for it. No verb
could spend the cursor: `show --help` listed nothing beyond `-h`,
`--comments-cursor` was rejected as unknown, and there is no `comments`
verb (`comment` is write-only). Every comment older than the newest page
was therefore unreachable from the CLI.

Measured on a live 125-comment task: the newest 100 were returned and the
remaining 25 could not be read by any command. With `--comments-cursor`
the walk now yields 100 + 25 = 125 distinct comments, zero overlap,
terminating at `has_more: false`.

Ordering, measured rather than assumed, because the reported symptom
pointed the other way: a page is the NEWEST `limit` comments in ASCENDING
display order, so the newest comment is the LAST element and was always
reachable. `next_cursor` walks toward OLDER history, which is the
direction that was blocked. `--comments-limit 1` returns the single newest
comment, which is the probe that distinguishes the two readings.

The local SQLite path accepts the same flags with the same semantics via a
shared pager. Without a flag its output is unchanged — complete history,
no `comments_page` — so existing local consumers are unaffected.

The cursor codec moves from src/server/v1.ts to src/lib/comment-cursor.ts
so the CLI can decode the cursors the server mints; server behaviour is
unchanged. A second copy of keyset logic is how the two ends drift into
disagreeing about what a cursor means.

Agent: Silvanus
@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] GO — #200 @ 9e5293e — lens: correctness+isolation+wiring, reviewer codewith-sol-reviewer (1 of 1)

P0: None.

P1: None. src/cli/commands/task-commands.ts validates limits/cursors before either routing path, forwards the same bounded request to cloudListComments, and applies matching newest-page/older-cursor semantics locally. src/lib/comment-cursor.ts preserves the server codec and strict (created_at, id) keyset behavior; src/server/v1.ts only relocates that codec. I found no reachable correctness, isolation, or wiring regression.

P2: None.

P3: None.

Evidence: exact-head bun test src/cli/comment-page-cursor.test.ts passed 5/5; existing cloud/router/OpenAPI suites passed 106/106; bun run typecheck passed. GitHub test and dashboard checks pass. PR base main is current at f2cba9a1a2f689b5f657a9e4b0343c06bb5a9053, which is also the head's merge base, so the reviewed diff is the current merge candidate.

Could not verify: I did not exercise the cited live 125-comment production task or a published-package installation. The hermetic cloud-server walk and local SQLite walk cover the changed paths and both pass.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant