fix: bound item history and stop resolving bodies nobody reads (BUG-2608) (#1147)

* fix: bound item history and stop resolving bodies nobody reads (BUG-2608)

Item history was unbounded on every surface, and summary mode paid for what it
discarded: the endpoint resolved EVERY version by walking the item's whole
reverse-patch chain, and both the CLI and the MCP dispatcher then projected
that away to metadata. An item edited under collab records a version every few
seconds while someone types, so this is routinely hundreds of full-content
reconstructions per history call, for output that shows none of them.

Two independent fixes, because they address different costs.

SUMMARY SKIPS THE WALK. `?summary=true` returns metadata from the raw rows and
never resolves a patch. That is the dominant win: the resolution was pure waste
for every caller except --full. Content and is_diff are cleared TOGETHER — an
empty body still claiming to be a reverse patch would tell a consumer to
resolve something that is not there.

LIMIT BOUNDS THE WINDOW, newest-first. That direction is not a preference: with
reverse patches, reconstructing any version means walking back from current
content, so a newest-end window is the cheap prefix of that walk while an older
one still pays for everything above it. That is also why there is deliberately
no offset — it would advertise a pagination whose later pages cost the same as
no bound at all.

Absent limit stays UNBOUNDED on the endpoint, following the item-list
precedent (maxItemListQueryLimit: "a zero/absent limit is left unbounded — this
only clamps an explicit oversized request"). The defaults live on the CLIENTS,
where a token budget is actually known: `pad item history` defaults to 50 with
--limit to change it, and the MCP catalog action injects 50 (max 300, the same
pair list and backlinks already use). A server that truncates a request nobody
bounded is a silent-truncation trap for third-party API consumers.

The MCP default goes in the CATALOG action rather than either dispatcher, so it
reaches BOTH transports — HTTP reads it off the input, and stdio receives it as
the CLI's new --limit through BuildCLIArgs. ToolSurfaceVersion 0.20 -> 0.21
with a changelog entry, plus instructions.md and README, per the 2304-family
contract discipline. Additive param bump: `limit` already existed, nothing
changed shape, and a v0.20 consumer that sends no limit now gets the newest 50
instead of all — which is the fix, not a break in it.

The restore and single-version-expand paths still resolve the FULL chain, and a
test pins that: bounding their walk would strand exactly the old versions those
paths exist to reach.

Eight mutations, each failing only the leg it targets. Three fixture problems
surfaced that way and are worth naming, because each made a test that could not
fail:
  - force_version in a PATCH body does nothing (`json:"-"` on ItemUpdate), so
    the throttle collapsed six edits into one version; varying the source per
    edit is what actually records them.
  - an 8-byte body is cheaper stored whole than as a patch, so no version was
    ever is_diff=true and the is_diff assertion was inert. The fixture now uses
    a body large enough that the store really stores patches.
  - the cmdhelp test fixture lacked the new --limit flag, so BuildCLIArgs
    silently dropped it. Verified against the REAL cmdhelp tree that the flag
    is present and typed int, so the fixture mirrors the CLI rather than
    flattering it.

* docs: bring CLAUDE.md to v0.21 and name why the two result caps differ (BUG-2608)

Codex round 1, both findings.

CLAUDE.md still described the MCP surface as v0.20 — stale because of my own
bump, in the document every agent working this repo reads first. README and
instructions.md are held to the version by a test; CLAUDE.md is not, which is
exactly why it drifts.

The cap "mismatch" (MCP max 300, endpoint clamp 500) is deliberate layering,
not an oversight — item lists have the identical split (300 in the catalog,
1000 at the endpoint) because the two answer different questions: an agent
token budget is only knowable in the catalog, while the endpoint's clamp is a
server-resource ceiling on what any caller may ask for. But nothing said so
anywhere, so a reader comparing the two numbers had no way to tell design from
accident — which is precisely the report Codex filed. Now stated at the
constant and in CLAUDE.md, including why the versions ceiling is LOWER than the
list one (resolving a version can cost a patch application per row, not just a
row read) and why an absent limit is left unbounded at the endpoint.

* fix+test: honest truncation notice, armed fixtures, and the residual named (BUG-2608)

Codex round 2, both findings, and the second is the more useful one.

CLI TRUNCATION NOTICE was wrong in both directions: it compared the response
length against the requested limit, so an item with exactly N versions was
reported as truncated, and a --limit above the server's ceiling was clamped
there and reported as complete. It now asks for ONE MORE row than it shows and
reports truncation only when that extra row comes back. The one case this still
cannot detect — an ask above the server's own ceiling, where the probe row is
clamped away with everything else — is stated in the code rather than papered
over by hardcoding the server's constant in the CLI.

UNDER-ARMED FIXTURES. The unbounded test seeded 5 versions, so a server quietly
defaulting to 50 would have passed the assertion that denies exactly that; it
now seeds 60. The clamp test seeded 2 and could not observe a clamp at all;
the clamp is now asserted directly against a parseItemVersionsLimit function
extracted for the purpose, over the inputs a URL can really carry (absent, 0,
negative, unparseable, either side of the ceiling).

That extraction replaced my own first attempt, which was worse than no test: it
re-implemented the clamp arithmetic in the test body and asserted the result
against itself. It could not have failed.

THE RESIDUAL, NAMED RATHER THAN IMPLIED. Codex's sharpest point is that the
summary tests cannot detect "resolve everything, then clear the fields" —
verified by mutation: pointing the summary branch at the resolving reader
leaves every handler test green, because the response is byte-identical either
way. So the performance claim does not rest on them. It rests on the handler's
summary branch calling ListItemVersionsPage (one reviewable line) plus a new
store test proving that reader really returns unresolved rows rather than
quietly resolving them — mutation-verified from the other side by making the
resolver a passthrough. The test file says all of this, including that an
end-to-end assertion would need a patch-application counter in the production
path, and why that is not worth it when the cost of being wrong is performance
rather than correctness.

* fix(cli): don't resolve for table output, guard the probe overflow, finish the CLAUDE.md bump (BUG-2608)

Codex round 3, four findings.

--full was treated as "content needed" regardless of output format, but the
table path prints no bodies at any setting — so `pad item history --full`
without --format json made the server walk the entire patch chain to build
content the CLI then dropped. That is the exact waste this bug is about,
reintroduced through the flag meant to opt into it. Content is now resolved
only when it will actually be shown.

The limit+1 probe overflowed at MaxInt: it wrapped negative, the client omitted
the parameter, and a request the user bounded came back unbounded — the
opposite of the ask. Guarded.

The truncation notice's documented blind spot was understated: it is AT the
server ceiling as well as above it, since the probe row is clamped away with
everything else. Wording corrected rather than resolved — the CLI still does
not duplicate the server's constant, because a copied ceiling goes stale
silently and asking for hundreds of versions is already opting out of a bound.

Two more CLAUDE.md sites still called v0.19 current; I fixed only the first on
the previous pass. That document describes the contract in three places and I
updated one, which is its own small lesson about grepping for every instance of
a claim rather than the first.

Live-verified against a real server: --limit 2 truncates and says so, --limit 4
on a 4-version item says nothing (the false positive Codex found), --limit 0
returns everything, --full --format json carries bodies, and the default JSON
shape carries metadata only.

* fix(server): clamp an out-of-range limit instead of treating it as unbounded (BUG-2608)

Codex round 4. `?limit=9223372036854775808` made strconv.Atoi return ErrRange,
which my parser lumped in with 'unparseable' and mapped to 0 — unbounded. So an
absurdly large number defeated the very ceiling the function exists to impose,
which is worse than no clamp at all: the bound looked enforced and was not.

Atoi hands back the saturated value alongside ErrRange, so a range-positive
input is now clamped to the ceiling — it is an oversized ASK, not a malformed
one. Range-negative still falls through to unbounded, matching a plain
negative.

Regression cases cover both overflow directions, and the mutation that
reinstates the old behaviour fails on the exact input.

This is the input-domain lesson again: I enumerated absent, zero, negative and
non-numeric, and stopped at what a person would plausibly type rather than at
what the TYPE admits.

* docs(cli): say in --help that a large --limit is capped server-side (BUG-2608)

Codex round 6, and the third time this ceiling came up — which is the tell that
the disposition was in the wrong place. I had documented it twice in CODE
comments, where the person affected never looks. `--help` says what the flag
does, and it said 'show only the newest N versions (0 = all)', which is false
for an N above the server's cap.

Now stated in both the flag help and the command's long description, including
that the truncation notice cannot detect that case and that --limit 0 is the
way to ask for a complete history.

Deliberately not naming the number: a constant duplicated into help text goes
stale silently, and 'capped server-side' is the part a user needs in order to
act. Behaviour unchanged — this is the artifact catching up with it, which is
the same correction the BUG-2301 sentinel comment and the instructions.md
overclaim both came down to.

* test: seed versions through the store so the fixture stops tripping the rate limiter (BUG-2608)

CI caught this and it is my defect, not a flake. Arming the unbounded test
above the plausible default meant seeding 60 versions, and the fixture did that
with 60 HTTP PATCHes in a burst — which trips the server's rate limiter. It
passed locally and in the Go job and failed under Nix, which is the signature
of a test that was always going to fail intermittently rather than one that
broke.

Seeding now goes through the store. That is not a weakening: versions are
recorded by the store on either path, and the endpoint under test is the READ
side, so seeding underneath the transport costs the assertions nothing while
removing a burst the server is entitled to refuse.

The three things that make this fixture work are now stated where someone would
otherwise undo them by accident — the large body (a small one is stored whole,
so no version is ever is_diff and every diff assertion goes vacuous), the
rotating source (the throttle collapses same-(actor, source) bursts into one
version), and the store-not-HTTP seeding with the rate-limit reason attached.

Re-verified after the change: the fixture still records more than 50 versions
and still produces reverse-patch rows, and the default-cap mutation now uses
the REALISTIC default of 50 rather than the 3 I first tested with — the old
5-version fixture could only have caught an implausibly small cap.
This commit is contained in:
xarmian
2026-08-17 19:02:53 -04:00
committed by GitHub
parent 50a442d048
commit 625cab9984
14 changed files with 891 additions and 19 deletions
+5 -3
View File
File diff suppressed because one or more lines are too long
+2 -2
View File
@@ -349,7 +349,7 @@ directory for `claude-code`, and an `[mcp_servers.pad]` table in
project-scoped, it's install-on-request only — `--all` and `pad mcp status` cover
the per-user clients (including Codex) and skip it.
**Tool catalog (v0.20)** — ten resource × action tools plus `pad_set_workspace` (eleven total), no flat verb explosion. `pad_item.list` accepts `unparented: true` (mutually exclusive with `parent`) to select items with no parent or implements relationship, and is summary-shaped by default on both transports (`full: true` opts into complete content bodies):
**Tool catalog (v0.21)** — ten resource × action tools plus `pad_set_workspace` (eleven total), no flat verb explosion. `pad_item.list` accepts `unparented: true` (mutually exclusive with `parent`) to select items with no parent or implements relationship, and is summary-shaped by default on both transports (`full: true` opts into complete content bodies):
| Tool | Actions |
|---|---|
@@ -378,7 +378,7 @@ initialize handshake under `capabilities.experimental.padCmdhelp` and
`pad://_meta/version`):
- `cmdhelp_version: "0.1"` — CLI help-tree contract (used at dispatch time)
- `tool_surface_version: "0.20"` — MCP tool catalog contract (v0.5 added `pad_library`; v0.6 `pad_item.backlinks`; v0.7 `pad_item` `export`/`import`; v0.8 `pad_workspace` `deleted`/`restore`; v0.9 made `pad_item.list` summary-shaped by default with a default+max result cap; v0.10 enforced the draft-playbook gate server-side on `pad_playbook.run` with an `allow_draft` escape hatch; v0.11 added the read-only `pad_attachment` tool (`list`/`show`); v0.12 added `pad_project.activity` (agent-accessible non-streaming activity feed); v0.13 added `pad_project` `ready`/`stale` (agent-oriented backlog + attention queries); v0.14 added `pad_item` `history` + optimistic concurrency (TASK-2022); v0.15 added the `pad_item.list` `unparented` parameter (TASK-2096); v0.16 made an empty-string `assigned_user_id` / `agent_role_id` CLEAR the assignment instead of being silently dropped, so an agent can finally unassign an item (TASK-2571); v0.17 carried that to the LOCAL STDIO transport by teaching the CLI to lift those keys onto their columns instead of into the fields blob (BUG-2583); v0.18 added `clear_assigned_user` / `clear_agent_role` booleans — the canonical, schema-discoverable way to unassign, backed by new `--clear-assigned-user` / `--clear-agent-role` flags on `pad item update` (IDEA-2584); v0.19 added a `clear_parent` boolean — the canonical, schema-discoverable way to detach an item from its parent, backed by a new `--clear-parent` flag on `pad item update` (BUG-2078); v0.20 gave every tool an explicit annotation block derived from the catalogs read-only knowledge — fully-read-only tools advertise `readOnlyHint: true` / `destructiveHint: false`, all-additive-write tools (`pad_workspace`, `pad_library`) drop `destructiveHint`, overwrite/delete-capable tools stay conservatively destructive, `openWorldHint: false` everywhere — replacing mcp-gos defaults that marked every tool destructive (BUG-2302), and made `pad_item.list` summary-shaped on the remote HTTP transport too, with a declared `full` boolean as the opt-in for complete bodies on both transports (BUG-2305); see `internal/mcp/version.go` for the full changelog)
- `tool_surface_version: "0.21"` — MCP tool catalog contract (v0.5 added `pad_library`; v0.6 `pad_item.backlinks`; v0.7 `pad_item` `export`/`import`; v0.8 `pad_workspace` `deleted`/`restore`; v0.9 made `pad_item.list` summary-shaped by default with a default+max result cap; v0.10 enforced the draft-playbook gate server-side on `pad_playbook.run` with an `allow_draft` escape hatch; v0.11 added the read-only `pad_attachment` tool (`list`/`show`); v0.12 added `pad_project.activity` (agent-accessible non-streaming activity feed); v0.13 added `pad_project` `ready`/`stale` (agent-oriented backlog + attention queries); v0.14 added `pad_item` `history` + optimistic concurrency (TASK-2022); v0.15 added the `pad_item.list` `unparented` parameter (TASK-2096); v0.16 made an empty-string `assigned_user_id` / `agent_role_id` CLEAR the assignment instead of being silently dropped, so an agent can finally unassign an item (TASK-2571); v0.17 carried that to the LOCAL STDIO transport by teaching the CLI to lift those keys onto their columns instead of into the fields blob (BUG-2583); v0.18 added `clear_assigned_user` / `clear_agent_role` booleans — the canonical, schema-discoverable way to unassign, backed by new `--clear-assigned-user` / `--clear-agent-role` flags on `pad item update` (IDEA-2584); v0.19 added a `clear_parent` boolean — the canonical, schema-discoverable way to detach an item from its parent, backed by a new `--clear-parent` flag on `pad item update` (BUG-2078); v0.20 gave every tool an explicit annotation block derived from the catalogs read-only knowledge — fully-read-only tools advertise `readOnlyHint: true` / `destructiveHint: false`, all-additive-write tools (`pad_workspace`, `pad_library`) drop `destructiveHint`, overwrite/delete-capable tools stay conservatively destructive, `openWorldHint: false` everywhere — replacing mcp-gos defaults that marked every tool destructive (BUG-2302), and made `pad_item.list` summary-shaped on the remote HTTP transport too, with a declared `full` boolean as the opt-in for complete bodies on both transports (BUG-2305); v0.21 bounded `pad_item.history`, which was unbounded on every surface — `limit` now covers it (default 50, max 300, the NEWEST N; no `offset`, because reverse-patch storage makes only a newest-end window cheap), applied in the catalog action so it lands on both transports, and summary mode now asks the server to skip patch resolution rather than resolving bodies the dispatcher discards (BUG-2608); see `internal/mcp/version.go` for the full changelog)
External agents pin against these so a future rename doesn't break them
silently. Errors come back as structured envelopes (`{error: {code,
+62 -1
View File
@@ -1294,8 +1294,18 @@ type itemVersionSummary struct {
ChangeSummary string `json:"change_summary,omitempty"`
}
// defaultItemHistoryLimit bounds `pad item history` when the caller does not
// ask for a window. The endpoint itself is deliberately unbounded when no
// limit is sent (see maxItemVersionsQueryLimit) — the default belongs here,
// where a terminal and a token budget are the actual constraint, mirroring how
// TASK-2000 put the item-list default on the clients rather than the server.
// A collab-heavy item accumulates a version every few seconds while someone
// types, so "all of them" is rarely the question being asked.
const defaultItemHistoryLimit = 50
func historyCmd() *cobra.Command {
var full bool
var limit int
cmd := &cobra.Command{
Use: "history <ref>",
@@ -1307,11 +1317,21 @@ Each row is a snapshot captured when the item's content changed (edits from
the web editor, CLI, MCP, collab flushes, and version restores). This is a
READ-ONLY view use the web UI to restore a specific version.
Shows the newest 50 by default; pass --limit 0 for the whole history. A
collab-edited item records a version every few seconds while someone types, so
histories get long.
A single request is capped server-side, so a very large --limit returns the
cap rather than everything, and the "showing the newest N" notice cannot detect
that case. Use --limit 0 when you genuinely want the complete history.
Items can be referenced by issue ID (e.g. TASK-5) or slug.
Examples:
pad item history TASK-5
pad item versions TASK-5 --format json
pad item history TASK-5 --limit 10 # newest 10 only
pad item history TASK-5 --limit 0 # all versions
pad item history TASK-5 --full --format json # include resolved content`,
Args: cobra.ExactArgs(1),
RunE: func(cmd *cobra.Command, args []string) error {
@@ -1319,10 +1339,36 @@ Examples:
ws := getWorkspace()
slug := args[0]
versions, err := client.ListItemVersions(ws, slug)
if limit < 0 {
return fmt.Errorf("--limit must be zero or positive")
}
// Ask for ONE MORE than we intend to show. That extra row is the
// only honest way to tell "there are more" from "that is all
// of them" — comparing the response length against the requested
// limit calls an exactly-N history truncated, and stays silent
// when it really was cut short.
//
// Guarded against overflow: limit+1 at MaxInt wraps negative, the
// client then omits the parameter, and the "bounded" request comes
// back unbounded — the opposite of what was asked for.
ask := limit
if ask > 0 && ask < math.MaxInt {
ask = limit + 1
}
// Resolve content only when it will actually be SHOWN. --full
// alone is not enough: the table path prints no bodies at any
// setting, so pairing --full with table output would make the
// server walk the whole patch chain for output that discards it.
wantsContent := full && formatFlag == "json"
versions, err := client.ListItemVersionsPage(ws, slug, ask, !wantsContent)
if err != nil {
return err
}
truncated := limit > 0 && len(versions) > limit
if truncated {
versions = versions[:limit]
}
if formatFlag == "json" {
if full {
@@ -1360,11 +1406,26 @@ Examples:
)
}
fmt.Printf("\n%d version(s).\n", len(versions))
// Say so when the window was capped. A silent cap reads as "this
// is the whole history", which is the wrong thing to believe
// about an audit trail.
//
// One case this cannot detect: a --limit AT OR ABOVE the server's
// own ceiling is clamped there, and the probe row is clamped away
// with it, so the result looks complete. The CLI does not
// hardcode the server's ceiling to paper over that — asking for
// hundreds of versions is already opting out of a bound, and a
// duplicated constant would go stale silently.
if truncated {
fmt.Printf("Showing the newest %d — pass --limit 0 for all, or --limit N for more.\n", limit)
}
return nil
},
}
cmd.Flags().BoolVar(&full, "full", false, "include each version's resolved content body (JSON output only)")
cmd.Flags().IntVar(&limit, "limit", defaultItemHistoryLimit,
"show only the newest N versions (0 = all; very large values are capped server-side)")
return cmd
}
+25 -1
View File
@@ -286,8 +286,32 @@ func wrapItemNotFound(err error, itemSlug, wsSlug string) error {
// `pad item history` (TASK-2022). Reuses the existing read-only
// GET /items/{slug}/versions endpoint — no new store surface.
func (c *Client) ListItemVersions(wsSlug, itemSlug string) ([]models.Version, error) {
return c.ListItemVersionsPage(wsSlug, itemSlug, 0, false)
}
// ListItemVersionsPage is ListItemVersions with the BUG-2608 bounds: `limit`
// caps the newest-first window (0 = server default, i.e. unbounded), and
// `summary` asks the server to skip reverse-patch resolution and return
// metadata only.
//
// Pass summary=true whenever the caller is going to discard content. It is not
// merely a smaller response: resolving means walking the item's entire patch
// chain, so a history listing that projects to metadata was paying for bodies
// it never showed.
func (c *Client) ListItemVersionsPage(wsSlug, itemSlug string, limit int, summary bool) ([]models.Version, error) {
path := "/workspaces/" + wsSlug + "/items/" + itemSlug + "/versions"
q := url.Values{}
if limit > 0 {
q.Set("limit", strconv.Itoa(limit))
}
if summary {
q.Set("summary", "true")
}
if len(q) > 0 {
path += "?" + q.Encode()
}
var result []models.Version
return result, c.get("/workspaces/"+wsSlug+"/items/"+itemSlug+"/versions", &result)
return result, c.get(path, &result)
}
// RestoreItem un-archives a soft-deleted item via the restore endpoint, which
+41 -3
View File
@@ -82,7 +82,7 @@ var padItemTool = ToolDef{
// (newest-first), a token-light summary shape (id, created_at,
// created_by, source, change_summary) with the resolved content
// body omitted. Restoring a version stays a web-UI action.
"history": passThrough([]string{"item", "history"}),
"history": actionItemHistory,
// Bulk + notes + decisions
// bulk-update is custom because the CLI takes repeatable
@@ -208,7 +208,7 @@ var padItemSchemaParams = []ParamDef{
// ── List / starred ──
{Name: "all", Type: "bool", Description: "Include archived/done items in list responses. Optional for: list, starred."},
{Name: "limit", Type: "number", Description: "Maximum results. Optional for: list, backlinks. List defaults to 50, max 300. Backlinks defaults to 50, max 300."},
{Name: "limit", Type: "number", Description: "Maximum results. Optional for: list, backlinks, history. Each defaults to 50, max 300. For history the window is the NEWEST N versions."},
{Name: "offset", Type: "number", Description: "Skip the first N results (paging). Optional for: backlinks."},
{Name: "sort", Type: "string", Description: "Sort field. Optional for: list."},
{Name: "group_by", Type: "string", Description: "Group-by field. Optional for: list."},
@@ -309,7 +309,8 @@ Actions:
reference TASK-5?" without scanning the full content
corpus.
history Read an item's version history (newest-first, read-only).
Required: ref.
Required: ref. Optional: limit (default 50, max 300
the NEWEST N versions), full.
Returns a token-light summary per recorded version
(id, created_at, created_by, source, change_summary);
the resolved content body is omitted. Restoring a
@@ -694,6 +695,43 @@ func actionItemList(ctx context.Context, input map[string]any, env ActionEnv) (*
return env.Dispatch(ctx, []string{"item", "list"}, out)
}
// mcpItemHistoryDefaultLimit / MaxLimit bound pad_item.action=history the same
// way list and backlinks are bounded, and for a sharper reason: a
// collab-edited item records a version every few seconds while someone types,
// so "the whole history" is routinely hundreds of rows nobody asked for
// (BUG-2608). Same numbers as list, so an agent does not have to remember a
// third pair.
const (
mcpItemHistoryDefaultLimit = 50
mcpItemHistoryMaxLimit = 300
)
// actionItemHistory handles pad_item.action=history. It injects a default
// limit when the agent did not ask for one and clamps an oversized one, so
// this lands on BOTH transports: the HTTP dispatcher reads it off the input,
// and the exec path gets it as the CLI's --limit through BuildCLIArgs.
//
// The catalog is the right home for the default (rather than either
// dispatcher) for the reason actionItemList already documents — it is the one
// place both transports pass through.
func actionItemHistory(ctx context.Context, input map[string]any, env ActionEnv) (*mcp.CallToolResult, error) {
out := make(map[string]any, len(input)+1)
for k, v := range input {
out[k] = v
}
limit := mcpItemHistoryDefaultLimit
if n, ok := numericInput(input["limit"]); ok && n > 0 {
limit = int(n)
}
if limit > mcpItemHistoryMaxLimit {
limit = mcpItemHistoryMaxLimit
}
out["limit"] = limit
return env.Dispatch(ctx, []string{"item", "history"}, out)
}
// actionItemExport handles pad_item.action=export. The CLI's
// `pad item export <ref>` defaults to WRITING A FILE (<slug>.pad.md),
// which is useless to an MCP caller — the bytes have to come back as
+6
View File
@@ -687,6 +687,12 @@ func liveCmdhelpDoc(t *testing.T) *cmdhelp.Document {
Flags: func() map[string]cmdhelp.Flag {
f := mkFlags("workspace")
f["full"] = cmdhelp.Flag{Type: "bool"}
// Mirrors `pad item history --limit` (BUG-2608). This
// fixture is what BuildCLIArgs consults in tests, so a
// flag missing here is silently dropped from the emitted
// args — the catalog default would look applied and reach
// the CLI as nothing.
f["limit"] = cmdhelp.Flag{Type: "int"}
return f
}(),
},
@@ -0,0 +1,146 @@
package mcp
// BUG-2608 — `pad_item.action=history` was unbounded, and summary mode paid
// for content it discarded: the endpoint resolved every version by walking the
// item's whole reverse-patch chain, and the dispatcher then projected that
// away to metadata.
//
// Two independent claims, tested where each actually lives:
// - the DEFAULT window is injected by the catalog action, so it reaches both
// transports — asserted on the CLI args the action produces, which is also
// the stdio half (BuildCLIArgs emits `--limit`).
// - the HTTP dispatcher asks the server to SKIP resolution when the caller
// did not ask for content — asserted on the request it builds.
import (
"context"
"net/http"
"net/http/httptest"
"strconv"
"strings"
"testing"
"github.com/PerpetualSoftware/pad/internal/models"
)
func historyArgs(t *testing.T, input map[string]any) string {
t.Helper()
disp := &fakeDispatcher{}
env := ActionEnv{Doc: liveCmdhelpDoc(t), Workspace: NewWorkspaceState("docapp"), Dispatcher: disp}
res, err := actionItemHistory(context.Background(), input, env)
if err != nil {
t.Fatalf("actionItemHistory error: %v", err)
}
if res != nil && res.IsError {
t.Fatalf("error result: %s", textOf(res))
}
return strings.Join(disp.gotArgs, " ")
}
// A bare history call must be bounded. Asserting the CLI args covers the exec
// transport at the same time: the default only reaches stdio because
// BuildCLIArgs turns the param into the CLI's --limit flag.
func TestPadItemHistory_AppliesDefaultLimit(t *testing.T) {
joined := historyArgs(t, map[string]any{"ref": "TASK-5"})
want := "--limit " + strconv.Itoa(mcpItemHistoryDefaultLimit)
if !strings.Contains(joined, want) {
t.Errorf("cliArgs %q should carry the injected default %q — an unbounded "+
"history is the bug", joined, want)
}
}
func TestPadItemHistory_ClampsOversizedLimit(t *testing.T) {
joined := historyArgs(t, map[string]any{"ref": "TASK-5", "limit": float64(99999)})
want := "--limit " + strconv.Itoa(mcpItemHistoryMaxLimit)
if !strings.Contains(joined, want) {
t.Errorf("cliArgs %q should clamp to %q", joined, want)
}
}
func TestPadItemHistory_HonorsInRangeLimit(t *testing.T) {
joined := historyArgs(t, map[string]any{"ref": "TASK-5", "limit": float64(7)})
if !strings.Contains(joined, "--limit 7") {
t.Errorf("cliArgs %q should honor an in-range limit of 7", joined)
}
}
// The HTTP half of the optimization. Summary mode is not merely a smaller
// response — it tells the server not to walk the patch chain at all. If the
// dispatcher stops sending it, the projection below still looks identical
// while the server goes back to resolving bodies nobody reads, which is the
// silent half of this bug.
func TestDispatchItemHistory_RequestsSummaryUnlessFullAsked(t *testing.T) {
for _, tc := range []struct {
name string
input map[string]any
wantSummary bool
}{
{"default asks the server to skip resolution", map[string]any{
"workspace": "docapp", "ref": "TASK-5",
}, true},
{"full=true must NOT skip it", map[string]any{
"workspace": "docapp", "ref": "TASK-5", "full": true,
}, false},
} {
t.Run(tc.name, func(t *testing.T) {
rec := &queryRecordingHandler{respBody: "[]"}
d := &HTTPHandlerDispatcher{
Handler: rec,
UserResolver: fixedUserResolver(&models.User{
ID: "user-1", Name: "Dave", Email: "dave@example.com",
}),
}
ctx := WithDispatchInput(context.Background(), tc.input)
if _, err := d.Dispatch(ctx, []string{"item", "history"}, nil); err != nil {
t.Fatalf("Dispatch: %v", err)
}
got := rec.gotQuery.Get("summary") == "true"
if got != tc.wantSummary {
t.Errorf("summary=%v, want %v (query %q)", got, tc.wantSummary, rec.gotQuery.Encode())
}
})
}
}
// The limit has to survive the trip to the server too, not just reach the
// action — the action's injection is worthless if the dispatcher drops it.
func TestDispatchItemHistory_ForwardsLimitToTheEndpoint(t *testing.T) {
rec := &queryRecordingHandler{respBody: "[]"}
d := &HTTPHandlerDispatcher{
Handler: rec,
UserResolver: fixedUserResolver(&models.User{
ID: "user-1", Name: "Dave", Email: "dave@example.com",
}),
}
ctx := WithDispatchInput(context.Background(), map[string]any{
"workspace": "docapp", "ref": "TASK-5", "limit": float64(12),
})
if _, err := d.Dispatch(ctx, []string{"item", "history"}, nil); err != nil {
t.Fatalf("Dispatch: %v", err)
}
if got := rec.gotQuery.Get("limit"); got != "12" {
t.Errorf("limit reached the endpoint as %q, want \"12\" (query %q)",
got, rec.gotQuery.Encode())
}
}
// queryRecordingHandler captures the QUERY STRING, which the shared
// recordingHandler does not keep — these tests are entirely about what ends up
// in it.
type queryRecordingHandler struct {
respBody string
gotQuery interface {
Get(string) string
Encode() string
}
}
func (h *queryRecordingHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
h.gotQuery = r.URL.Query()
w.Header().Set("Content-Type", "application/json")
w.WriteHeader(http.StatusOK)
_, _ = w.Write([]byte(h.respBody))
}
var _ http.Handler = (*queryRecordingHandler)(nil)
var _ = httptest.NewRecorder
+17 -1
View File
@@ -1387,7 +1387,23 @@ func (d *HTTPHandlerDispatcher) dispatchItemHistory(
return validationFailedResult(cmdKey, "ref is required",
"Pass ref=<issue id or slug> for the item whose history you want."), nil
}
full, _ := input["full"].(bool)
urlPath := "/api/v1/workspaces/" + url.PathEscape(workspace) + "/items/" + url.PathEscape(ref) + "/versions"
q := url.Values{}
if n, ok := numericInput(input["limit"]); ok && n > 0 {
q.Set("limit", strconv.FormatInt(n, 10))
}
// Ask the server to skip reverse-patch resolution whenever the bodies are
// about to be thrown away below. Without this the projection to
// itemVersionSummary was discarding content the server had just walked the
// item's entire patch chain to build (BUG-2608).
if !full {
q.Set("summary", "true")
}
if len(q) > 0 {
urlPath += "?" + q.Encode()
}
req, err := d.buildAuthedRequest(ctx, http.MethodGet, urlPath, nil, user)
if err != nil {
return buildRequestErrorResult(cmdKey, err), nil
@@ -1410,7 +1426,7 @@ func (d *HTTPHandlerDispatcher) dispatchItemHistory(
return classifyHTTPStatusKind(req.Context(), cmdKey, urlPath,
resp.StatusCode, bodyBytes, d.Lister, ResourceItem, ref), nil
}
if full, _ := input["full"].(bool); full {
if full {
// Complete rows requested — forward the endpoint's JSON
// verbatim, the same shape stdio's --full emits.
return packageJSONResult(string(bodyBytes)), nil
+2 -2
View File
@@ -6,11 +6,11 @@ Pad is a project tracker for developers and AI agents — issues (TASK, BUG), pl
If the user is asking general code questions with no project-management thread, you don't need this server.
## Tool surface (v0.20)
## Tool surface (v0.21)
Ten resource × action tools, plus `pad_set_workspace` (which takes a `workspace` slug only — no action enum). Eleven tools total.
- `pad_item` — Items: create / update / delete / get / list / move / restore / link / unlink / deps / star / unstar / starred / comment / list-comments / backlinks / bulk-update / note / decide / export / import / history. `list` accepts `unparented: true` to keep items with no parent or implements relationship (mutually exclusive with `parent`). `list` results are SUMMARY-shaped by default on both transports — no content bodies; pass `full: true` for complete bodies (token-expensive), or prefer `get` for a single item's body. `update` field writes are a server-side field-level merge (only the keys you set change); pass `expected_updated_at` for optimistic concurrency (a stale value fails with a structured 409 `update_conflict`). `history` returns read-only item version metadata (newest-first); pass `full: true` to include each version's resolved content body (token-expensive). To UNASSIGN an item, pass `clear_assigned_user: true` (or `clear_agent_role: true`) — the canonical form, works on both transports. To DETACH an item from its parent, pass `clear_parent: true` — same canonical shape, works on both transports. Setting and clearing the same field in one call is refused, not silently resolved, so don't pair `clear_assigned_user`/`clear_agent_role`/`clear_parent` with `assign`/`role`/`parent` respectively. An empty `assign` / `role` / `parent` does NOT clear: those name a person, a slug, or a ref, so an empty value reads as "not provided", exactly like every other optional string here. (Two older forms still work and are not deprecated: `field: ["assigned_user_id="]` on either transport, and a direct `assigned_user_id: ""` param over remote `/mcp` only — prefer the boolean, which is the only one this schema advertises.)
- `pad_item` — Items: create / update / delete / get / list / move / restore / link / unlink / deps / star / unstar / starred / comment / list-comments / backlinks / bulk-update / note / decide / export / import / history. `list` accepts `unparented: true` to keep items with no parent or implements relationship (mutually exclusive with `parent`). `list` results are SUMMARY-shaped by default on both transports — no content bodies; pass `full: true` for complete bodies (token-expensive), or prefer `get` for a single item's body. `update` field writes are a server-side field-level merge (only the keys you set change); pass `expected_updated_at` for optimistic concurrency (a stale value fails with a structured 409 `update_conflict`). `history` returns read-only item version metadata (newest-first), bounded to the NEWEST 50 versions by default (max 300 — pass `limit` to change the window); pass `full: true` to include each version's resolved content body (token-expensive). There is no `offset`: versions are stored as reverse patches, so only a newest-end window is cheap to reconstruct. To UNASSIGN an item, pass `clear_assigned_user: true` (or `clear_agent_role: true`) — the canonical form, works on both transports. To DETACH an item from its parent, pass `clear_parent: true` — same canonical shape, works on both transports. Setting and clearing the same field in one call is refused, not silently resolved, so don't pair `clear_assigned_user`/`clear_agent_role`/`clear_parent` with `assign`/`role`/`parent` respectively. An empty `assign` / `role` / `parent` does NOT clear: those name a person, a slug, or a ref, so an empty value reads as "not provided", exactly like every other optional string here. (Two older forms still work and are not deprecated: `field: ["assigned_user_id="]` on either transport, and a direct `assigned_user_id: ""` param over remote `/mcp` only — prefer the boolean, which is the only one this schema advertises.)
- `pad_workspace` — Workspaces: list / members / invite / storage / audit-log / create / claim / deleted / restore.
- `pad_collection` — Collections: list / create / update / delete.
- `pad_project` — Project intelligence: dashboard / next / ready / stale / standup / changelog / report / activity. Use `ready` for the actionable backlog and `stale` for items needing attention; `activity` to catch up on what other agents/users changed since you last worked (non-streaming feed with item refs + change details).
+31 -2
View File
@@ -103,7 +103,36 @@ const CmdhelpVersion = "0.1"
// pad_item actions unchanged. Backwards-compatible for v0.6
// consumers that don't enumerate the new actions.
//
// - "0.20" — current. BUG-2302: every advertised tool now carries an
// - "0.21" — current. BUG-2608: bounds `pad_item.action=history`, which
// was unbounded on every surface. Extends the `limit` param's
// vocabulary to cover history (default 50, max 300 — the same pair
// list and backlinks already use, so an agent has no third set of
// numbers to remember) and applies it in the CATALOG action rather
// than either dispatcher, so it lands on BOTH transports: the HTTP
// path reads it off the input, the exec path receives it as the
// CLI's new --limit through BuildCLIArgs.
//
// ADDITIVE param bump on the v0.6/v0.18/v0.19 pattern: `limit`
// already existed, no tool, action enum, or param SHAPE changed,
// and a v0.20 consumer that sends no limit keeps working — it now
// receives the newest 50 versions instead of all of them, which is
// the point of the fix rather than a break in it.
//
// The window is the NEWEST N and there is deliberately no `offset`.
// Versions are stored as REVERSE patches, so reconstructing any
// version means walking back from the item's current content
// through everything newer: a newest-end window is the cheap prefix
// of that walk, while an older window would still pay for
// everything above it. Offering an offset would advertise a
// pagination whose later pages cost the same as no bound at all.
//
// Behaviour change worth stating even though the shape is stable:
// summary mode now asks the SERVER to skip patch resolution
// (`?summary=true`) instead of resolving every body and discarding
// it in the dispatcher. Same result payload, minus a full chain
// walk per call.
//
// - "0.20" — BUG-2302: every advertised tool now carries an
// EXPLICIT annotation block derived from readOnlyActions (the same
// single source the tool-surface serializer uses) instead of
// inheriting mcp-go's NewTool defaults, which stamped
@@ -491,7 +520,7 @@ const CmdhelpVersion = "0.1"
// - result.capabilities.experimental.padToolSurface.version (handshake).
// - pad://_meta/version resource (queryable JSON document).
// - pad_meta.action: tool-surface (full catalog introspection).
const ToolSurfaceVersion = "0.20"
const ToolSurfaceVersion = "0.21"
// MetaVersionURI is the canonical URI of the queryable version document.
// Lives outside the pad://workspace/{ws}/... namespace because it's a
+90 -1
View File
@@ -5,6 +5,7 @@ import (
"errors"
"fmt"
"net/http"
"strconv"
"github.com/go-chi/chi/v5"
@@ -40,7 +41,31 @@ func (s *Server) handleListItemVersions(w http.ResponseWriter, r *http.Request)
return
}
versions, err := s.store.ListItemVersionsResolved(item.ID, item.Content)
// `limit` bounds the newest-first window. Absent means UNBOUNDED, matching
// the item-list endpoints (see maxItemListQueryLimit): the server does not
// truncate a request nobody asked to have truncated, and the agent-facing
// defaults live in the CLI and the MCP catalog where a token budget is
// actually known. An explicit oversized value is clamped.
limit := parseItemVersionsLimit(r.URL.Query().Get("limit"))
// `summary` skips diff resolution entirely. Every consumer of this
// endpoint except `--full` throws the content away immediately — the CLI
// projects to metadata, and so does the MCP history dispatcher — so
// resolving it means walking the whole reverse-patch chain to build bodies
// that are discarded milliseconds later (BUG-2608). The rows themselves
// still carry patch text, so the projection below strips it rather than
// shipping patches as though they were content.
if r.URL.Query().Get("summary") == "true" {
versions, err := s.store.ListItemVersionsPage(item.ID, limit)
if err != nil {
writeInternalError(w, err)
return
}
writeJSON(w, http.StatusOK, itemVersionMetadata(versions))
return
}
versions, err := s.store.ListItemVersionsResolvedPage(item.ID, item.Content, limit)
if err != nil {
writeInternalError(w, err)
return
@@ -52,6 +77,70 @@ func (s *Server) handleListItemVersions(w http.ResponseWriter, r *http.Request)
writeJSON(w, http.StatusOK, versions)
}
// maxItemVersionsQueryLimit clamps an explicit oversized `?limit=` on the
// version-history endpoint. Mirrors maxItemListQueryLimit's role: a ceiling on
// what a caller may ASK for, not a default applied to callers who ask for
// nothing.
//
// It is deliberately NOT the same number as the MCP catalog's max (300). The
// two caps answer different questions at different layers: this one is a
// server-resource ceiling, while the catalog's is an agent token budget, which
// is only knowable there. Item lists have the same split (1000 here, 300 in
// the catalog). Lower than the list ceiling because resolving a version can
// cost a patch application per row, not just a row read.
const maxItemVersionsQueryLimit = 500
// parseItemVersionsLimit turns the raw `?limit=` value into a row bound.
// Returns 0 — unbounded — for absent, unparseable, zero and negative input,
// so only a well-formed positive value bounds anything, and clamps anything
// above the ceiling.
//
// Split out of the handler so the clamp is directly testable. Asserting it
// through the endpoint would mean seeding 500+ versions per run to prove the
// ceiling binds; asserting it by re-implementing the arithmetic in a test
// would prove only that the test can multiply.
func parseItemVersionsLimit(raw string) int {
if raw == "" {
return 0
}
n, err := strconv.Atoi(raw)
if err != nil {
// A value too large for an int is an OVERSIZED ask, not a malformed
// one, and Atoi still hands back the saturated bound alongside
// ErrRange. Treating it as unparseable would return 0 — unbounded —
// so `?limit=9223372036854775808` would defeat the very ceiling this
// function exists to impose (codex round 4). Range-negative falls
// through to the unbounded branch below, matching a plain negative.
if errors.Is(err, strconv.ErrRange) && n > 0 {
return maxItemVersionsQueryLimit
}
return 0
}
if n <= 0 {
return 0
}
if n > maxItemVersionsQueryLimit {
return maxItemVersionsQueryLimit
}
return n
}
// itemVersionMetadata strips content from raw version rows for summary mode.
//
// Both fields are cleared together on purpose. An empty Content with IsDiff
// still true would describe a version whose body is a reverse patch waiting to
// be applied — telling a consumer to resolve something that is not there. The
// pair only means anything together, so summary mode returns neither.
func itemVersionMetadata(versions []models.Version) []models.Version {
out := make([]models.Version, 0, len(versions))
for _, v := range versions {
v.Content = ""
v.IsDiff = false
out = append(out, v)
}
return out
}
// handleGetItemVersion returns a single version with its diff resolved to full
// content. The paginated timeline serves raw reverse-patch text (it can't resolve
// a partial window), so the timeline card calls this to reconstruct real content
@@ -0,0 +1,283 @@
package server
// BUG-2608 — item history was unbounded on every surface, and worse, summary
// mode paid for what it discarded: the endpoint resolved EVERY version's
// content by walking the item's whole reverse-patch chain, and both the CLI
// and the MCP dispatcher then projected that away to metadata.
//
// Two independent properties are covered here, because they fail
// independently: `limit` bounds the window, and `summary` skips the walk.
import (
"net/http"
"strconv"
"strings"
"testing"
"github.com/PerpetualSoftware/pad/internal/models"
)
// seedVersionedItem creates an item and edits it n times, producing n+1
// versions' worth of history (the create plus each content change).
//
// Edits go through the STORE, not the HTTP API. That is not laziness: one of
// these fixtures needs more than 50 versions to arm (a smaller one cannot tell
// an unbounded server from one quietly defaulting to 50), and 60-odd PATCHes
// in a burst trip the server's rate limiter — which is exactly how this test
// failed in CI after the count was raised. The store is where versions are
// recorded on either path, and the endpoint under test is the READ side, so
// seeding underneath the transport costs the test nothing.
//
// Two details are load-bearing and easy to undo by accident:
// - the body is LARGE, so the store really stores reverse patches. A small
// body is cheaper whole, no version is ever is_diff, and every assertion
// about diff handling goes quietly vacuous.
// - each edit declares a DIFFERENT source, because the version throttle
// suppresses rapid snapshots from the same (actor, source) pair and would
// otherwise collapse the whole burst into one version. ForceVersion is not
// reachable from a request — it is `json:"-"` on ItemUpdate.
func seedVersionedItem(t *testing.T, srv *Server, wsSlug string, edits int) *models.Item {
t.Helper()
base := strings.Repeat("a line of body text that makes the patch worth storing\n", 200)
rr := doRequest(srv, "POST", "/api/v1/workspaces/"+wsSlug+"/collections/tasks/items",
map[string]any{"title": "versioned", "content": base + "v0\n"})
if rr.Code != http.StatusCreated {
t.Fatalf("create item: %d %s", rr.Code, rr.Body.String())
}
var item models.Item
parseJSON(t, rr, &item)
sources := []string{"web", "cli", "mcp", "skill"}
for i := 1; i <= edits; i++ {
content := base + "v" + strconv.Itoa(i) + "\n"
if _, err := srv.store.UpdateItem(item.ID, models.ItemUpdate{
Content: &content,
Source: sources[i%len(sources)],
}); err != nil {
t.Fatalf("edit %d: %v", i, err)
}
}
return &item
}
func fetchVersions(t *testing.T, srv *Server, wsSlug, itemSlug, query string) []models.Version {
t.Helper()
path := "/api/v1/workspaces/" + wsSlug + "/items/" + itemSlug + "/versions"
if query != "" {
path += "?" + query
}
rr := doRequest(srv, "GET", path, nil)
if rr.Code != http.StatusOK {
t.Fatalf("GET versions%s = %d: %s", query, rr.Code, rr.Body.String())
}
var out []models.Version
parseJSON(t, rr, &out)
return out
}
func TestItemVersions_LimitBoundsTheWindow(t *testing.T) {
srv := testServer(t)
ws := createTestWorkspaceViaAPI(t, srv)
item := seedVersionedItem(t, srv, ws, 5)
// Armed: the unbounded response must be bigger than the window under
// test, or a limit that does nothing would still pass.
all := fetchVersions(t, srv, ws, item.Slug, "")
if len(all) < 4 {
t.Fatalf("fixture never armed: only %d versions recorded, need enough to truncate", len(all))
}
got := fetchVersions(t, srv, ws, item.Slug, "limit=2")
if len(got) != 2 {
t.Errorf("limit=2 returned %d versions, want 2", len(got))
}
// Newest-first, and the window is the NEWEST end — the only end that is
// cheap to reconstruct from reverse patches.
if len(got) == 2 && len(all) >= 2 {
if got[0].ID != all[0].ID || got[1].ID != all[1].ID {
t.Errorf("limit window = [%s %s], want the newest two [%s %s]",
got[0].ID, got[1].ID, all[0].ID, all[1].ID)
}
}
}
// Absent limit stays UNBOUNDED, matching the item-list endpoints. The default
// belongs on the clients, where a token budget is actually known; a server
// that truncates a request nobody bounded is a silent-truncation trap for
// third-party API consumers.
func TestItemVersions_AbsentLimitIsUnbounded(t *testing.T) {
srv := testServer(t)
ws := createTestWorkspaceViaAPI(t, srv)
// MORE than the default any client applies (50). A fixture below that
// number cannot tell an unbounded server from one that quietly defaults
// to 50 — which is exactly the behaviour this test denies, so the row
// count is load-bearing rather than incidental (codex round 2).
const seeded = 60
item := seedVersionedItem(t, srv, ws, seeded)
all := fetchVersions(t, srv, ws, item.Slug, "")
if len(all) <= 50 {
t.Errorf("unbounded request returned %d versions after seeding %d; the "+
"server must not apply a default cap", len(all), seeded)
}
}
func TestItemVersions_OversizedLimitIsClamped(t *testing.T) {
srv := testServer(t)
ws := createTestWorkspaceViaAPI(t, srv)
item := seedVersionedItem(t, srv, ws, 8)
// An oversized ask must be treated as the ceiling, not rejected and not
// honoured. Seeding past the clamp to prove the ceiling BINDS would mean
// 500+ versions per run, so this asserts the reachable half — the request
// succeeds and returns what exists — and the clamp arithmetic itself is
// asserted directly below, where it is cheap and exact.
got := fetchVersions(t, srv, ws, item.Slug,
"limit="+strconv.Itoa(maxItemVersionsQueryLimit*10))
if len(got) == 0 {
t.Error("oversized limit returned nothing; it should clamp, not reject")
}
all := fetchVersions(t, srv, ws, item.Slug, "")
if len(got) != len(all) {
t.Errorf("oversized limit returned %d of %d existing versions; a clamp is "+
"a ceiling on the ASK, not a truncation of the answer", len(got), len(all))
}
}
// The clamp, asserted against the REAL function rather than by re-implementing
// its arithmetic in the test (which would prove only that the test can
// multiply) or by seeding 500+ rows per run. Pairs with the request-level test
// above: that one proves an oversized ask is accepted, this one proves the
// number it is accepted AS — and covers the inputs a URL can actually carry.
func TestItemVersions_ClampArithmetic(t *testing.T) {
for _, tc := range []struct {
raw string
want int
}{
{raw: "", want: 0}, // absent -> unbounded
{raw: "0", want: 0}, // explicit zero -> unbounded
{raw: "-5", want: 0}, // negative -> unbounded, not an error
{raw: "banana", want: 0}, // unparseable -> unbounded, not a 500
{raw: "1", want: 1},
{raw: strconv.Itoa(maxItemVersionsQueryLimit - 1), want: maxItemVersionsQueryLimit - 1},
{raw: strconv.Itoa(maxItemVersionsQueryLimit), want: maxItemVersionsQueryLimit},
{raw: strconv.Itoa(maxItemVersionsQueryLimit + 1), want: maxItemVersionsQueryLimit},
{raw: strconv.Itoa(maxItemVersionsQueryLimit * 100), want: maxItemVersionsQueryLimit},
// Past MaxInt64. Atoi reports ErrRange AND returns the saturated
// bound; treating that as unparseable would return 0 — unbounded —
// letting an absurd number defeat the ceiling entirely.
{raw: "9223372036854775808", want: maxItemVersionsQueryLimit},
{raw: "99999999999999999999999999", want: maxItemVersionsQueryLimit},
// Range-NEGATIVE stays unbounded, same as a plain negative.
{raw: "-9223372036854775809", want: 0},
} {
if got := parseItemVersionsLimit(tc.raw); got != tc.want {
t.Errorf("parseItemVersionsLimit(%q) = %d, want %d", tc.raw, got, tc.want)
}
}
}
// The summary property. This covers the SHAPE — same rows, no content, no
// stale is_diff — and deliberately does not claim more than that.
//
// WHAT THIS CANNOT SEE, stated because a reader would otherwise assume it
// does: an implementation that resolved every version and THEN blanked the
// fields would pass every assertion here, because the response is byte-identical
// either way. Verified by mutation — pointing the summary branch at
// ListItemVersionsResolvedPage leaves this file green (codex round 2).
//
// So the "no walk happened" half rests on two things instead: the handler's
// summary branch calls ListItemVersionsPage, which is one line and reviewable,
// and TestListItemVersionsPage_ReturnsUnresolvedRows in internal/store proves
// that reader genuinely returns unresolved rows rather than quietly resolving
// them. An end-to-end assertion would need a patch-application counter in the
// production path; the cost of being wrong here is performance, not
// correctness, so that instrument is not built.
func TestItemVersions_SummaryOmitsContentButKeepsMetadata(t *testing.T) {
srv := testServer(t)
ws := createTestWorkspaceViaAPI(t, srv)
item := seedVersionedItem(t, srv, ws, 3)
full := fetchVersions(t, srv, ws, item.Slug, "")
summary := fetchVersions(t, srv, ws, item.Slug, "summary=true")
if len(summary) != len(full) {
t.Fatalf("summary returned %d versions, full returned %d — summary must "+
"change the SHAPE, not the row set", len(summary), len(full))
}
// Armed: the full response actually carries bodies, or "summary has no
// content" is trivially true of both.
var fullHasContent bool
for _, v := range full {
if v.Content != "" {
fullHasContent = true
break
}
}
if !fullHasContent {
t.Fatal("fixture never armed: the unbounded response carried no content, " +
"so the summary assertion below proves nothing")
}
for i, v := range summary {
if v.Content != "" {
t.Errorf("summary version %d carried content (%d bytes)", i, len(v.Content))
}
// IsDiff must be cleared with it: an empty body still claiming to be a
// reverse patch tells a consumer to resolve something that is absent.
if v.IsDiff {
t.Errorf("summary version %d still claims is_diff with no body to patch", i)
}
// Metadata is the whole point of the mode — it has to survive.
if v.ID == "" || v.CreatedAt.IsZero() || v.CreatedBy == "" {
t.Errorf("summary version %d lost metadata: %+v", i, v)
}
}
}
// summary and limit compose: the bound applies to the metadata-only path too,
// which is the combination every agent call actually uses.
func TestItemVersions_SummaryRespectsLimit(t *testing.T) {
srv := testServer(t)
ws := createTestWorkspaceViaAPI(t, srv)
item := seedVersionedItem(t, srv, ws, 5)
got := fetchVersions(t, srv, ws, item.Slug, "summary=true&limit=2")
if len(got) != 2 {
t.Errorf("summary+limit=2 returned %d versions, want 2", len(got))
}
for i, v := range got {
if v.Content != "" {
t.Errorf("summary+limit version %d carried content", i)
}
}
}
// The restore path must keep resolving the WHOLE chain. A version is
// reconstructed by walking back from current content, so bounding that walk
// would make older versions unrestorable — the one place the limit must not
// reach (BUG-1612's expand path has the same requirement).
func TestItemVersions_RestoreStillReachesOldVersions(t *testing.T) {
srv := testServer(t)
ws := createTestWorkspaceViaAPI(t, srv)
item := seedVersionedItem(t, srv, ws, 6)
all := fetchVersions(t, srv, ws, item.Slug, "")
if len(all) < 5 {
t.Fatalf("fixture never armed: %d versions", len(all))
}
// The OLDEST recorded version — past any default window a client applies.
oldest := all[len(all)-1]
rr := doRequest(srv, "POST",
"/api/v1/workspaces/"+ws+"/items/"+item.Slug+"/versions/"+oldest.ID+"/restore", nil)
if rr.Code != http.StatusOK {
t.Fatalf("restore oldest version = %d: %s — bounding the resolve walk "+
"would strand exactly these", rr.Code, rr.Body.String())
}
}
+35 -3
View File
@@ -4618,8 +4618,27 @@ func (s *Store) shouldCreateItemVersion(itemID, actor, source string) (bool, err
// ListItemVersionsResolved returns versions with full content (diffs resolved).
// Requires the current item content to reconstruct diff-based versions.
//
// Unbounded: every version is read and every reverse patch applied. Callers
// that only need the newest N should use ListItemVersionsResolvedPage, which
// bounds BOTH the read and the patch walk (BUG-2608). This form remains
// correct — and required — where an arbitrary version must be located, since
// the chain can only be walked from current content backwards.
func (s *Store) ListItemVersionsResolved(itemID, currentContent string) ([]models.Version, error) {
versions, err := s.ListItemVersions(itemID)
return s.ListItemVersionsResolvedPage(itemID, currentContent, 0)
}
// ListItemVersionsResolvedPage is ListItemVersionsResolved bounded to the
// newest `limit` versions (limit <= 0 means unbounded).
//
// The bound is cheap ONLY because it takes the newest N. Versions are stored
// as REVERSE patches, so reconstructing any version means starting from the
// item's current content and walking backwards through everything newer — a
// window at the newest end is exactly the prefix of that walk, while an older
// window would still require walking everything above it. That asymmetry is
// why this offers a limit and not an offset (BUG-2608).
func (s *Store) ListItemVersionsResolvedPage(itemID, currentContent string, limit int) ([]models.Version, error) {
versions, err := s.ListItemVersionsPage(itemID, limit)
if err != nil {
return nil, err
}
@@ -4720,12 +4739,25 @@ func (s *Store) ListItemVersionsBeforeTime(itemID string, before time.Time, befo
// ListItemVersions returns all versions for an item.
func (s *Store) ListItemVersions(itemID string) ([]models.Version, error) {
rows, err := s.db.Query(s.q(`
return s.ListItemVersionsPage(itemID, 0)
}
// ListItemVersionsPage returns an item's versions newest-first, bounded to
// `limit` rows (limit <= 0 means unbounded). Raw rows — reverse-patch versions
// still carry patch text, not content; see ListItemVersionsResolvedPage.
func (s *Store) ListItemVersionsPage(itemID string, limit int) ([]models.Version, error) {
query := `
SELECT id, item_id, content, change_summary, created_by, source, is_diff, created_at
FROM item_versions
WHERE item_id = ?
ORDER BY created_at DESC, version_seq DESC
`), itemID)
`
args := []interface{}{itemID}
if limit > 0 {
query += " LIMIT ?"
args = append(args, limit)
}
rows, err := s.db.Query(s.q(query), args...)
if err != nil {
return nil, err
}
+146
View File
@@ -0,0 +1,146 @@
package store
// BUG-2608 — the claim that summary mode SKIPS reverse-patch resolution is a
// claim about work not done, which a response-shape assertion cannot make: a
// handler that resolved everything and then blanked the fields would look
// identical from outside (codex round 2).
//
// This asserts it where it is observable — the paged raw reader returns rows
// still carrying patch text and is_diff, while the resolved reader returns
// reconstructed content. Summary mode calls the former, and that one-line
// reading is what carries the performance claim.
import (
"strings"
"testing"
"github.com/PerpetualSoftware/pad/internal/models"
)
func TestListItemVersionsPage_ReturnsUnresolvedRows(t *testing.T) {
s := testStore(t)
ws, err := s.CreateWorkspace(models.WorkspaceCreate{Name: "Versions WS"})
if err != nil {
t.Fatalf("CreateWorkspace: %v", err)
}
coll, err := s.CreateCollection(ws.ID, models.CollectionCreate{
Name: "Tasks", Slug: "tasks", Prefix: "TASK",
Schema: `{"fields":[{"key":"status","type":"select","options":["open","done"],"default":"open"}]}`,
})
if err != nil {
t.Fatalf("CreateCollection: %v", err)
}
// A body large enough that a patch really is smaller than the whole
// content — otherwise the store keeps full copies, nothing is a diff, and
// every assertion below is vacuous.
base := strings.Repeat("a line of body text that makes the patch worth storing\n", 200)
item, err := s.CreateItem(ws.ID, coll.ID, models.ItemCreate{Title: "versioned", Content: base + "v0\n"})
if err != nil {
t.Fatalf("CreateItem: %v", err)
}
sources := []string{"web", "cli", "mcp", "skill"}
for i := 1; i <= 4; i++ {
content := base + "v" + string(rune('0'+i)) + "\n"
if _, err := s.UpdateItem(item.ID, models.ItemUpdate{
Content: &content,
Source: sources[i%len(sources)],
}); err != nil {
t.Fatalf("edit %d: %v", i, err)
}
}
fresh, err := s.GetItem(item.ID)
if err != nil || fresh == nil {
t.Fatalf("reload item: %v", err)
}
raw, err := s.ListItemVersionsPage(item.ID, 0)
if err != nil {
t.Fatalf("ListItemVersionsPage: %v", err)
}
resolved, err := s.ListItemVersionsResolvedPage(item.ID, fresh.Content, 0)
if err != nil {
t.Fatalf("ListItemVersionsResolvedPage: %v", err)
}
if len(raw) != len(resolved) || len(raw) == 0 {
t.Fatalf("raw=%d resolved=%d rows; need the same non-empty set", len(raw), len(resolved))
}
var rawDiffs int
for _, v := range raw {
if v.IsDiff {
rawDiffs++
}
}
if rawDiffs == 0 {
t.Fatal("fixture never armed: the store recorded no reverse-patch versions, " +
"so 'raw rows are unresolved' is trivially true of full content too")
}
// The resolved reader must have DONE the work the raw one skips.
for i, v := range resolved {
if v.IsDiff {
t.Errorf("resolved version %d still marked is_diff — it was not resolved", i)
}
}
// And the raw reader must NOT have: at least one row differs from its
// resolved counterpart, which can only be true if no patch was applied.
var differ int
for i := range raw {
if raw[i].ID == resolved[i].ID && raw[i].Content != resolved[i].Content {
differ++
}
}
if differ == 0 {
t.Error("every raw row already equalled its resolved content — the paged " +
"reader is resolving patches, which is the work summary mode exists " +
"to skip")
}
}
func TestListItemVersionsPage_LimitTakesTheNewest(t *testing.T) {
s := testStore(t)
ws, err := s.CreateWorkspace(models.WorkspaceCreate{Name: "Versions WS 2"})
if err != nil {
t.Fatalf("CreateWorkspace: %v", err)
}
coll, err := s.CreateCollection(ws.ID, models.CollectionCreate{
Name: "Tasks", Slug: "tasks", Prefix: "TASK",
Schema: `{"fields":[{"key":"status","type":"select","options":["open","done"],"default":"open"}]}`,
})
if err != nil {
t.Fatalf("CreateCollection: %v", err)
}
item, err := s.CreateItem(ws.ID, coll.ID, models.ItemCreate{Title: "versioned", Content: "v0\n"})
if err != nil {
t.Fatalf("CreateItem: %v", err)
}
sources := []string{"web", "cli", "mcp", "skill"}
for i := 1; i <= 6; i++ {
content := "v" + string(rune('0'+i)) + "\n"
if _, err := s.UpdateItem(item.ID, models.ItemUpdate{Content: &content, Source: sources[i%len(sources)]}); err != nil {
t.Fatalf("edit %d: %v", i, err)
}
}
all, err := s.ListItemVersionsPage(item.ID, 0)
if err != nil {
t.Fatalf("unbounded: %v", err)
}
if len(all) < 4 {
t.Fatalf("fixture never armed: %d versions", len(all))
}
got, err := s.ListItemVersionsPage(item.ID, 2)
if err != nil {
t.Fatalf("limited: %v", err)
}
if len(got) != 2 {
t.Fatalf("limit=2 returned %d rows", len(got))
}
// The NEWEST two — the only window the reverse-patch chain makes cheap.
if got[0].ID != all[0].ID || got[1].ID != all[1].ID {
t.Errorf("limit window = [%s %s], want newest [%s %s]",
got[0].ID, got[1].ID, all[0].ID, all[1].ID)
}
}