mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-21 18:13:26 +00:00
xarmian
625cab9984
* 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.