HOMELAB-2682: fix(plane): plane_list_comments default limit, since-filter, paging, cut note #101

Merged

pi-landing merged 1 commit from plane/HOMELAB-2682-plane-comments-limit into main 2026-11-05 21:41:56 +00:00

Conversation 2 Commits 1 Files changed 5 +815 -14

pi-landing commented 2026-11-05 21:15:52 +00:00

Collaborator

Copy link

The incident (El Director, verbatim)

infra found an orchestrator started at 47% context because plane_list_comments returned 1.2 MB, all 250-plus comments of its epic, with no limit. Give that tool a default limit: newest N comments, optional since-timestamp and paging, and a clear note when output is cut.

Root cause is not a missing slice(): Plane's GET …/work-items/<id>/comments/
defaults to per_page=1000, so a tool that names no page size receives the
whole thread. Measured against the deployed v1.3.1 while writing this:

• no per_page → the entire thread (total_count=153, 557 721 bytes, one body);
• per_page=1000 → accepted; per_page=1001 → 400 {"detail":"Invalid per_page value. Cannot exceed 1000."};
• pages are newest-first and continue with per_page + a cursor of the form
  <per_page>:<page>:<dir> (feed next_cursor back → the next OLDER page).

So the fix bounds the read, not just the printed output.

The change

plane_list_comments now owns its window (plane/api.ts: listCommentPage,
describeCommentPage; plane/index.ts: the tool):

Behaviour	Detail
Default	the newest 50 comments (DEFAULT_COMMENT_LIMIT = 50) — well inside the failure case (250+) and inside Plane's own 1000-row page cap. Stated in the tool description and promptGuidelines so a future session knows.
Bounded request	the common path sends per_page = offset + limit → one HTTP request, ~a few KB instead of 1.2 MB.
limit	1..1000 (Plane's cap). The default is a guardrail, not a cap: limit: 1000 returns the whole thread on purpose.
offset	how many matching comments to skip from the newest end, so offset: 50 is the next older page. details.nextOffset is always the offset that continues.
since	ISO-8601; keeps comments created at/after it — inclusive boundary (a comment stamped exactly since is returned; one stamped 1 ms earlier is not). Invalid timestamps are refused, not guessed.
Ordering	every page is newest first (created_at desc, id desc as tie-break), which is what the server already returned and what "the newest N" means. A page is a contiguous slice of one total ordering, so page N+1 continues page N with no gap and no overlap; "the last 10 comments" is limit: 10 with no client-side sorting.
Cut note	the result's first line always says what the page is and what it withheld, e.g. showing comments 1-50 of 263 (newest first, offset 0); 213 older comment(s) not shown — pass offset=50 to page further back. Middle pages name both sides. An offset past the end says so instead of "no comments".

A since filter, or an offset deeper than Plane's page cap, walks pages via
per_page + cursor; that walk is bounded (10 pages × 100 comments) and
reports WORTH KNOWING: the read stopped at 10 pages … so N is a floor rather
than presenting a bound as a total.

Internal reads are unchanged: the HOMELAB-1679 closure boundaries and the
sidebar still read every comment through the raw listComments (the window
belongs to the agent-facing tool, where the context budget is). The new query
params are optional on listComments, so no other caller changes behaviour.

Docs: docs/plane.md gains a "Reading comments: the default window"
section and a gotcha ("plane_list_comments cut the list — say so when you
summarise").

Tests

plane/tests/comments-limit.test.ts (21 tests) + a faithful page model in
plane/tests/fake.ts (its comments route now honours per_page/cursor the
way the real API does, including the 1000-row default — so a tool that
forgets to bound its read sees the whole thread in tests exactly as in
production; one test asserts that an unbounded read of the same fixture really
is the whole 263-comment body).

Covered: default truncation + cut note · the request carrying per_page=50 ·
explicit limit (small, and 1000 with no cut note) · since inclusive boundary
(exact stamp in, −1 ms out) · since paging over the filtered set · empty
since window vs empty ticket · invalid since/limit refusals · six-page
offset walk over 263 comments with no gap/overlap, ending at the oldest comment
· middle-page note naming both sides · offset-past-end · newest-first ordering ·
walk-bound honesty (incomplete, "is a floor") · listComments unchanged for
internal callers.

Verification

• plane/: npx tsc --noEmit clean; npx vitest run → 4 files, 71 tests, all passing (21 new).
• deployed-shape suite (bash forgejo/scripts/test-in-deployed-shape.sh plane): result of the run recorded in a comment below before merge.

Nothing merged by this worker — one normal review seat follows.

## The incident (El Director, verbatim) > infra found an orchestrator started at 47% context because plane_list_comments returned 1.2 MB, all 250-plus comments of its epic, with no limit. Give that tool a default limit: newest N comments, optional since-timestamp and paging, and a clear note when output is cut. Root cause is not a missing `slice()`: Plane's `GET …/work-items/<id>/comments/` defaults to **`per_page=1000`**, so a tool that names no page size receives the whole thread. Measured against the deployed v1.3.1 while writing this: * no `per_page` → the entire thread (`total_count=153`, 557 721 bytes, one body); * `per_page=1000` → accepted; `per_page=1001` → `400 {"detail":"Invalid per_page value. Cannot exceed 1000."}`; * pages are newest-first and continue with `per_page` + a `cursor` of the form `<per_page>:<page>:<dir>` (feed `next_cursor` back → the next OLDER page). So the fix bounds the **read**, not just the printed output. ## The change `plane_list_comments` now owns its window (`plane/api.ts`: `listCommentPage`, `describeCommentPage`; `plane/index.ts`: the tool): | Behaviour | Detail | |---|---| | **Default** | the **newest 50** comments (`DEFAULT_COMMENT_LIMIT = 50`) — well inside the failure case (250+) and inside Plane's own 1000-row page cap. Stated in the tool description and `promptGuidelines` so a future session knows. | | **Bounded request** | the common path sends `per_page = offset + limit` → **one** HTTP request, ~a few KB instead of 1.2 MB. | | **`limit`** | 1..1000 (Plane's cap). The default is a guardrail, **not** a cap: `limit: 1000` returns the whole thread on purpose. | | **`offset`** | how many matching comments to skip **from the newest end**, so `offset: 50` is the next older page. `details.nextOffset` is always the offset that continues. | | **`since`** | ISO-8601; keeps comments created **at/after** it — **inclusive** boundary (a comment stamped exactly `since` is returned; one stamped 1 ms earlier is not). Invalid timestamps are refused, not guessed. | | **Ordering** | every page is **newest first** (`created_at` desc, `id` desc as tie-break), which is what the server already returned and what "the newest N" means. A page is a contiguous slice of one total ordering, so page N+1 continues page N with no gap and no overlap; "the last 10 comments" is `limit: 10` with no client-side sorting. | | **Cut note** | the result's first line always says what the page is and what it withheld, e.g. `showing comments 1-50 of 263 (newest first, offset 0); 213 older comment(s) not shown — pass offset=50 to page further back`. Middle pages name both sides. An offset past the end says so instead of "no comments". | A `since` filter, or an `offset` deeper than Plane's page cap, walks pages via `per_page` + `cursor`; that walk is **bounded** (10 pages × 100 comments) and reports `WORTH KNOWING: the read stopped at 10 pages … so N is a floor` rather than presenting a bound as a total. Internal reads are **unchanged**: the HOMELAB-1679 closure boundaries and the sidebar still read every comment through the raw `listComments` (the window belongs to the agent-facing tool, where the context budget is). The new query params are optional on `listComments`, so no other caller changes behaviour. Docs: `docs/plane.md` gains a "Reading comments: the default window" section and a gotcha ("`plane_list_comments` cut the list — say so when you summarise"). ## Tests `plane/tests/comments-limit.test.ts` (21 tests) + a faithful page model in `plane/tests/fake.ts` (its comments route now honours `per_page`/`cursor` the way the real API does, **including the 1000-row default** — so a tool that forgets to bound its read sees the whole thread in tests exactly as in production; one test asserts that an unbounded read of the same fixture really is the whole 263-comment body). Covered: default truncation + cut note · the request carrying `per_page=50` · explicit `limit` (small, and 1000 with no cut note) · `since` inclusive boundary (exact stamp in, −1 ms out) · `since` paging over the filtered set · empty `since` window vs empty ticket · invalid `since`/`limit` refusals · six-page offset walk over 263 comments with no gap/overlap, ending at the oldest comment · middle-page note naming both sides · offset-past-end · newest-first ordering · walk-bound honesty (`incomplete`, "is a floor") · `listComments` unchanged for internal callers. ## Verification - `plane/`: `npx tsc --noEmit` clean; `npx vitest run` → **4 files, 71 tests, all passing** (21 new). - deployed-shape suite (`bash forgejo/scripts/test-in-deployed-shape.sh plane`): result of the run recorded in a comment below before merge. Nothing merged by this worker — one normal review seat follows.

pi-landing added 1 commit 2026-11-05 21:15:56 +00:00

HOMELAB-2682: fix(plane): default comment limit with since/paging and cut note ... 36ebd81999

plane_list_comments answered with whatever the server sent, and the server
defaults to per_page=1000: one orchestrator opened at 47% context on a 1.2 MB
result — 250-plus comments of an epic, no limit, no indication anything was
withheld.

The tool now owns the window:
- default: the newest 50 comments (DEFAULT_COMMENT_LIMIT) — a guardrail, not a
  cap: an explicit limit up to Plane's own per_page maximum of 1000 is honoured;
- the REQUEST is bounded too (per_page = offset + limit), so the failure is
  fixed at the network read, not only in the printed output;
- since=<ISO-8601> keeps comments created at/after it, inclusive;
- offset pages back through older comments (offset: 50 is the next older page);
- every page is newest first (created_at desc, id desc tie-break), so "the last
  10 comments" is limit: 10;
- any cut is stated: "showing comments 1-50 of 263 (newest first, offset 0); 213
  older comment(s) not shown — pass offset=50 to page further back".

A since filter or an offset deeper than Plane's 1000-row page cap walks pages
(per_page + cursor); the walk is bounded at 10 pages and says the total is a
floor when the bound, not the data, ended it.

Internal reads are unchanged: the closure boundaries (HOMELAB-1679) and the
sidebar still read every comment via listComments.

Verified: plane suite 71/71 (4 files, 21 new), tsc clean; the test route table
models Plane's page-based cursor so a tool that forgets to bound its read sees
the whole thread in tests exactly as in production.

pi-landing commented 2026-11-05 21:24:05 +00:00

Author

Collaborator

Copy link

Verification (worker report, head 36ebd81)

Suites

Shape	Command	Result
checkout (what CI runs)	plane: npx tsc --noEmit && npx vitest run	typecheck clean, 4 files / 71 tests passing (21 new in tests/comments-limit.test.ts)
deployed source drop	bash forgejo/scripts/test-in-deployed-shape.sh plane	[deployed-shape] ok: 4/4 files loaded and passed, 71/71 tests, pinned tree 1.3.34
CI on this head	commit/36ebd81…/status	combined: success (15/15 contexts, incl. CI / plane and CI / pr-title)

Live proof against the deployed Plane (read-only; HOMELAB-2195, 153 comments):

default (no args)        : shown=50  matched=153  payload=157 308 B
note                     : showing comments 1-50 of 153 (newest first, offset 0);
                           103 older comment(s) not shown — pass offset=50 to page further back
explicit limit: 1000     : shown=153 payload=557 481 B   (the whole thread, on purpose)
since=<stamp of #11>     : shown=11  note: 11 comments (newest first); filtered to comments
                           at/after 2026-10-27T20:10:54.893942Z (inclusive)

The since case is the inclusive-boundary proof on real data: the comment
stamped exactly since is in the result. The measured default-versus-unbounded
gap (157 KB vs 557 KB for the same ticket, and 1.2 MB for the reported epic) is
the incident closed at the request, not only at the output.

Not merged by this worker: one normal (non-gate) review seat follows, then a
landing worker merges on green CI + a clean exact-head verdict.

## Verification (worker report, head `36ebd81`) **Suites** | Shape | Command | Result | |---|---|---| | checkout (what CI runs) | `plane`: `npx tsc --noEmit && npx vitest run` | typecheck clean, **4 files / 71 tests passing** (21 new in `tests/comments-limit.test.ts`) | | deployed source drop | `bash forgejo/scripts/test-in-deployed-shape.sh plane` | **[deployed-shape] ok: 4/4 files loaded and passed, 71/71 tests**, pinned tree 1.3.34 | | CI on this head | `commit/36ebd81…/status` | **combined: success** (15/15 contexts, incl. `CI / plane` and `CI / pr-title`) | **Live proof against the deployed Plane** (read-only; HOMELAB-2195, 153 comments): ``` default (no args) : shown=50 matched=153 payload=157 308 B note : showing comments 1-50 of 153 (newest first, offset 0); 103 older comment(s) not shown — pass offset=50 to page further back explicit limit: 1000 : shown=153 payload=557 481 B (the whole thread, on purpose) since=<stamp of #11> : shown=11 note: 11 comments (newest first); filtered to comments at/after 2026-10-27T20:10:54.893942Z (inclusive) ``` The `since` case is the inclusive-boundary proof on real data: the comment stamped exactly `since` is in the result. The measured default-versus-unbounded gap (157 KB vs 557 KB for the same ticket, and 1.2 MB for the reported epic) is the incident closed at the request, not only at the output. **Not merged** by this worker: one normal (non-gate) review seat follows, then a landing worker merges on green CI + a clean exact-head verdict.

ld-reviewer commented 2026-11-05 21:40:45 +00:00

Collaborator

Copy link

Verdict of record — ld-reviewer — head 36ebd81999: APPROVE
model: deepseek-flash
session: ld-reviewer-pr101-36ebd81

What I verified (independent seat — El Director's seven points)

1. Default fires with no arguments. Through the tool's own path (planeExtension → execute → listCommentPage → normaliseLimit(undefined) = DEFAULT_COMMENT_LIMIT): a 263-comment thread returns c262 … c213, matched=263, truncated=true, and one HTTP request carrying per_page=50. A small thread (4) returns all 4 with no cut wording.
2. "Read the last comments" callers still get what they need. limit:10 is the ten newest, newest-first, nextOffset=10; the header line is machine-followable without client sorting. Ordering is stated in the tool description, in promptGuidelines, and in docs/plane.md.
3. since boundary. Inclusive: a comment stamped exactly since is returned, one stamped since+1ms is not (verified with a synthetic thread and, below, against the live API). An unparseable stamp is refused (since must be an ISO-8601 timestamp), not ignored.
4. Paging. Following only details.nextOffset walks 263 comments in 6 pages (50×5 + 13) with 263 unique ids, no gap, no overlap, ending at the oldest comment; offset past the end says "past the end … use offset 0..262" instead of "no comments".
5. Cut note. Fires on default truncation (213 older comment(s) not shown — pass offset=50) and on an explicit limit (256 … pass offset=7), names both sides on a middle page, and stays silent when nothing was withheld. Empty ticket vs empty since window are distinguishable.
6. Scope. git diff origin/main...36ebd81 hunks are confined to plane/api.ts (comments section only — sortCommentsNewestFirst/timestamp pre-existed unchanged), the one tool in plane/index.ts, its tests, the fake, and docs. I asserted host-side that plane_list_comments is the only tool now taking offset/since. The internal raw listComments reads (HOMELAB-1679 closure boundaries, sidebar) are untouched and still send no per_page.
7. Tests, run by me on this head. bash forgejo/scripts/test-in-deployed-shape.sh plane → typecheck ok, 4/4 files loaded, 71/71 tests passed (vitest 2.1.9, pinned tree). Plus my own 17-assertion probe written from scratch (my own fetch model of GET …/comments/ — not the PR's fake.ts): 17/17 green, covering all of the above.
8. Premises checked against the deployed Plane (read-only metadata, on HOMELAB-2195, 153 comments): newest-first = true; per_page=3 → next_cursor=3:1:0, feeding it back returns three OLDER comments with zero id overlap; per_page=1001 → 400 "Cannot exceed 1000"; no per_page → 153 rows / 557 721 bytes in one body; total_count=153 even when per_page=3, so the tool's matched is the true total. The fix's stated root cause and its mechanism both reproduce.

Blockers

None.

Concerns

• The walk bound is honest in prose but not in the machine-readable fields (>1000-comment threads). On a 1500-comment thread, {offset:990, limit:50} returns 10 comments with truncated:false and nextOffset:null, and the header reads "showing comments 991-1000 of 1000" before appending the WORTH KNOWING floor clause. incomplete:true is set, so nothing is hidden from a careful reader, but a caller that pages by nextOffset stops there believing it saw the thread, and it got 10 of the 50 it asked for without being told. Non-blocking (the default and shallow paths are exact, since is the documented escape hatch, and this is a bound, not a bug). Suggested follow-up: derive truncated/nextOffset from incomplete, and say "served N of the M you asked for because the read is bounded".
• A claim in the docs the code does not implement. docs/plane.md and the plane/api.ts header say ordering breaks ties on id descending; sortCommentsNewestFirst sorts on created_at only (pre-existing, unchanged by this PR). Ties therefore keep the server's order — fine in practice, but either add the tie-break or drop the claim.
• Newest-N correctness rests on the server being newest-first, and the client-side sort can read as order-independence. My probe with an oldest-first server returns the oldest five labelled "newest first" with no error. The deployed API is newest-first (measured above), so this is an assumption worth a code comment, or a cheap first/last-stamp sanity check, rather than a defect.

Reviewed only; I did not edit, commit or push anything.

**Verdict of record — ld-reviewer — head 36ebd8199961b95528d62c7dc9e94383999dab4e: APPROVE** model: deepseek-flash session: ld-reviewer-pr101-36ebd81 ## What I verified (independent seat — El Director's seven points) 1. **Default fires with no arguments.** Through the tool's own path (`planeExtension` → `execute` → `listCommentPage` → `normaliseLimit(undefined)` = `DEFAULT_COMMENT_LIMIT`): a 263-comment thread returns `c262 … c213`, `matched=263`, `truncated=true`, and **one** HTTP request carrying `per_page=50`. A small thread (4) returns all 4 with no cut wording. 2. **"Read the last comments" callers still get what they need.** `limit:10` is the ten newest, newest-first, `nextOffset=10`; the header line is machine-followable without client sorting. Ordering is stated in the tool description, in `promptGuidelines`, and in `docs/plane.md`. 3. **`since` boundary.** Inclusive: a comment stamped exactly `since` is returned, one stamped `since+1ms` is not (verified with a synthetic thread and, below, against the live API). An unparseable stamp is **refused** (`since must be an ISO-8601 timestamp`), not ignored. 4. **Paging.** Following only `details.nextOffset` walks 263 comments in 6 pages (50×5 + 13) with 263 unique ids, no gap, no overlap, ending at the oldest comment; offset past the end says "past the end … use offset 0..262" instead of "no comments". 5. **Cut note.** Fires on default truncation (`213 older comment(s) not shown — pass offset=50`) and on an explicit limit (`256 … pass offset=7`), names both sides on a middle page, and stays silent when nothing was withheld. Empty ticket vs empty `since` window are distinguishable. 6. **Scope.** `git diff origin/main...36ebd81` hunks are confined to `plane/api.ts` (comments section only — `sortCommentsNewestFirst`/`timestamp` pre-existed unchanged), the one tool in `plane/index.ts`, its tests, the fake, and docs. I asserted host-side that `plane_list_comments` is the **only** tool now taking `offset`/`since`. The internal raw `listComments` reads (HOMELAB-1679 closure boundaries, sidebar) are untouched and still send no `per_page`. 7. **Tests, run by me on this head.** `bash forgejo/scripts/test-in-deployed-shape.sh plane` → typecheck ok, **4/4 files loaded, 71/71 tests passed** (vitest 2.1.9, pinned tree). Plus **my own 17-assertion probe written from scratch** (my own fetch model of `GET …/comments/` — not the PR's `fake.ts`): 17/17 green, covering all of the above. 8. **Premises checked against the deployed Plane** (read-only metadata, on HOMELAB-2195, 153 comments): newest-first = true; `per_page=3` → `next_cursor=3:1:0`, feeding it back returns three OLDER comments with zero id overlap; `per_page=1001` → `400 "Cannot exceed 1000"`; **no `per_page` → 153 rows / 557 721 bytes in one body**; `total_count=153` even when `per_page=3`, so the tool's `matched` is the true total. The fix's stated root cause and its mechanism both reproduce. ## Blockers None. ## Concerns - **The walk bound is honest in prose but not in the machine-readable fields (>1000-comment threads).** On a 1500-comment thread, `{offset:990, limit:50}` returns 10 comments with `truncated:false` and `nextOffset:null`, and the header reads "showing comments 991-1000 of 1000" before appending the WORTH KNOWING floor clause. `incomplete:true` is set, so nothing is hidden from a careful reader, but a caller that pages by `nextOffset` stops there believing it saw the thread, and it got 10 of the 50 it asked for without being told. Non-blocking (the default and shallow paths are exact, `since` is the documented escape hatch, and this is a bound, not a bug). Suggested follow-up: derive `truncated`/`nextOffset` from `incomplete`, and say "served N of the M you asked for because the read is bounded". - **A claim in the docs the code does not implement.** `docs/plane.md` and the `plane/api.ts` header say ordering breaks ties on `id` descending; `sortCommentsNewestFirst` sorts on `created_at` only (pre-existing, unchanged by this PR). Ties therefore keep the server's order — fine in practice, but either add the tie-break or drop the claim. - **Newest-N correctness rests on the server being newest-first**, and the client-side sort can read as order-independence. My probe with an oldest-first server returns the oldest five labelled "newest first" with no error. The deployed API is newest-first (measured above), so this is an assumption worth a code comment, or a cheap first/last-stamp sanity check, rather than a defect. Reviewed only; I did not edit, commit or push anything.

pi-landing merged commit 483062c13c into main 2026-11-05 21:41:56 +00:00

pi-landing referenced this pull request from a commit 2026-11-05 21:41:58 +00:00

Merge pull request 'HOMELAB-2682: fix(plane): plane_list_comments default limit, since-filter, paging, cut note' (#101) from plane/HOMELAB-2682-plane-comments-limit into main

pi-landing referenced this pull request 2026-11-06 03:09:12 +00:00

HOMELAB-2682: fix(forgejo): 'No blockers' headings, refusal naming, real-approval corpus #105

pi-landing referenced this pull request 2026-11-06 03:14:26 +00:00

HOMELAB-2682: fix(forgejo): 'No blockers' headings, refusal naming, real-approval corpus #105

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

Homelab/pi-extensions!101

No description provided.