fix(cli): give the comment page cursor a consumer on show and inspect - #200
fix(cli): give the comment page cursor a consumer on show and inspect#200andrei-hasna wants to merge 1 commit into
Conversation
`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
|
[REVIEW] GO — #200 @ 9e5293e — lens: correctness+isolation+wiring, reviewer codewith-sol-reviewer (1 of 1) P0: None. P1: None. P2: None. P3: None. Evidence: exact-head 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. |
Fixes the half of todos task
65b80fa4that survives measurement. Part of the reported premise is refuted below — please read that section.The defect
showandinspectemit: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 --helplists no options beyond-h--cursor/--comments-cursorrejected as unknown optionscommentsverb;commentis write-onlyhistorycarries only field changes;timeline --task <id>returns 0 rowsSo every comment older than the newest page was unreachable from the CLI.
Measured, on a live 125-comment task (
0ba8ff46), before the fix25 comments existed and no command could read them.
After the fix, same live task, CLI built from this branch
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
limitcomments in ASCENDING display order. The newest comment is the last array element and was always reachable.next_cursorwalks 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):
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:371documentsbeforeas "strictly older than";postgres-adapter.ts:238sorts ascending;v1.tsover-fetcheslimit + 1and slices from the front, which only works if the store returns the newestlimit + 1ascending. The human render already said it correctly —task-commands.tsprints "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>onshowandinspect.cloudListComments.comments_page— so existing local consumers see nothing new.src/server/v1.tstosrc/lib/comment-cursor.tsso 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 as01640a5ewith the evidence. It belongs onlist(the sweep surface, which carries no comment data), so it needs a server + storage change across both backends and a design call on cost. Onshowthe caller already holds both signals.Tests
src/cli/comment-page-cursor.test.ts— 5 cases, spawning the real CLI against a mock/v1that reproduces the measured keyset semantics, over a 125-comment fixture:showwalks the entire history throughnext_cursor(100 + 25 = 125 distinct, zero overlap)--comments-limit 1returns the newest comment and sendslimit=1inspectaccepts the same flagsWritten red-first. One arm initially passed vacuously —
expect(stderr).toMatch(/comments-limit|between 1 and 500/)is satisfied byerror: unknown option '--comments-limit'before any fix exists — so it now also assertsnot.toMatch(/unknown option/i). After tightening:0 pass, 5 fail, failing witherror: unknown option '--comments-cursor'. After the fix:5 pass, 0 fail, 57 expect().bun run typecheck→ rc=0.Reviewer notes — two things to attack
created_at.getTaskWithRelationsreturns local commentsORDER BY created_atwith no tiebreak, while the pager sorts by(created_at, id)— the portable keyset the cursor requires, matching whatlocal-sqlite.tsalready does. For comments sharing a timestamp the paged order can differ from the unpaged order.cloudListComments, while the local-sqlite adapter internally permits up to 1001. Deliberate, for one consistent documented bound.Agent: Silvanus
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.