mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-10-26 14:30:13 +00:00
xarmian
d201ebb87d
* fix(server): a successful applier-path PATCH stops answering with the previous content (BUG-2995) The applier path writes the row WITHOUT content — the markdown travels through a connected tab's Y.Doc and reaches items.content on the next collab-snapshot flush — and the 200 was serialized from that row, so a caller that PATCHed content and read `content` back saw its own update missing. The response now carries the content as SENT, marked `warnings.content_outcome = applied_pending_flush` in the same vocabulary `content_not_applied` uses. The marker is what makes the echo honest: the echoed value is true of the apply and not of the row, and the stored form is not byte-identical to the sent form even once the flush lands. Measured rather than assumed (BUG-2995 trail): a jsdom harness driving the real two halves — setContent() then the editor's markdown storage — found 11 of 14 StarterKit-owned constructs normalise (setext headings, bullet markers, emphasis characters, list renumbering, blank-line runs). The row deliberately still lags; this stops the response lying about it. * test(server): the direct-path leg needs a warnings object to be wrong about (BUG-2995) The leg asserted "no content_outcome when the content landed in the row" and a mutant that stamps the marker unconditionally survived it: the warnings object is only CONSTRUCTED when there is something to say, so the marker landed inside a block that path never enters. The leg was asserting a property whose mechanism it could not reach. Sending an undeclared field gives that request a warnings object, so the mutant now has somewhere to be wrong. Preconditions are asserted rather than assumed, and the reason is in a comment so it survives a cleanup. * test: say what each BUG-2995 guard actually establishes (codex round 1) Two findings, both about a test claiming more than it measures. The direct-path control leg was named DirectPath and asserted only that items.content holds the sent value — true of the direct write, and equally true of contentRouteFallThrough or an ordinary non-routed write. Renamed to NoPendingMarkerWhenTheContentIsInTheRow, which is the route-independent invariant it does pin, with the boundary in the doc comment. The round-trip guard mixed the property (the sent form is not what comes back) with the recorded serializer output. A tiptap bump could change HOW a construct normalises without touching the contract and fail the property tests. Split: per-construct inequality assertions carry the contract, one clearly-named test holds the recorded forms and says to re-record it. * docs+cli: tell callers where the content is when a write is pending flush (codex round 2) The contract was implemented and typed but invisible to the surfaces callers actually read: the MCP instructions and catalog said only that 'content' is optional, the CLI printed nothing, and the TypeScript mirror of ItemWriteWarnings had no content_outcome, so typed code could not reach a warning the server was already sending. - pad item create/update now print a stderr line when the write is pending flush, saying the content landed in the live document, that what was printed is what was sent, and that a read may still show the previous content. Stderr, never stdout: --format json is piped into scripts. - pad item update --help, internal/mcp/instructions.md and the catalog's update action say the same, including the part that matters most to an agent: a get inside the window answers with the PREVIOUS content, which is the lag rather than a failed write, so do not re-send on it. - web/src/lib/types/index.ts mirrors content_outcome. - CLAUDE.md documents it beside undeclared_fields / dropped_fields. The outcome string is now one exported models constant rather than a literal in two packages: the server writes it and the CLI compares it, and a rename on either side would have left the warning silently never firing — indistinguishable from the bug being fixed. No duration is claimed on any surface. It is not established that the flush always lands (BUG-3000), so a number would be a claim nothing here can support; a test asserts the warning carries none. * docs: stop the pending-flush prose claiming more than the code does (codex round 3) Three overclaims, in the change whose whole point is not overclaiming. - Every surface said the stored copy 'catches up afterwards'. Nothing establishes that: the flush belongs to a browser tab, and whether it ever happens is the open half filed as BUG-3000. Now 'updated only when that tab next flushes, and nothing guarantees it does'. - 'a get/list answers with the PREVIOUS content' — a default list is summary-shaped and carries no content at all, so only get, or list with full=true, can answer with anything. - 'the stored form is NOT byte-identical' — the round-trip guard's own fixtures include three constructs that survive unchanged, so the measurement contradicted the sentence. Now 'may not be', with the stable case named. Two doc comments the last two commits falsified: models.Warnings described only undeclared keys, and the TypeScript mirror said TASK-2878 'added two more members'. Both now name the KINDS rather than counting members, so the next addition does not falsify them again. Also records the version decision where the next person will look: a POST-0.30 NO BUMP entry in version.go and CLAUDE.md. v0.9 bumped because list rows LOST fields consumers read; here nothing is removed or retyped and the one field whose value moved was answering a content PATCH with content from before that PATCH, so there is no reliance to break — the BUG-2304 disposition rather than v0.9's. * docs: the same overclaim, swept as a class this time (codex round 4) Round 3 fixed the user-facing prose and left the identical claim in the code comments — the named instances rather than the class (CONVE-18). Swept the branch diff for both shapes instead of patching the four sites the review listed: - 'is not byte-identical' as an invariant: models.ItemWriteWarnings, the handler comment, the Go test comment and the round-trip guard's comment now say the stored form MAY differ, and name the case that survives unchanged. The two remaining hits are correctly quantified — CLAUDE.md's 'may also differ', and a test asserting at least ONE construct differs, which is existential and true. - 'the row is written when a tab next flushes': now 'only if and when', with the BUG-3000 pointer, in the constant's own doc comment where a reader meets it first. The NO BUMP entry claimed 'there is no reliance to break', which is not establishable. A consumer could have detected the applier path by noticing the response echoed something other than what it sent, and that stops working. Restated as no DOCUMENTED OR SUPPORTED reliance, with the counter-case named: it was never documented, a genuinely lost write produced the same mismatch, and warnings.content_outcome answers that question properly in the same change. Also asserts what round 4 asked about: content plus an undeclared field in one applier-path request must produce ONE warnings object carrying both members. Mutation-checked — a marker that replaces the object rather than building on it now fails. * docs: the flush is a collab-snapshot write, not a promise from one tab (codex round 5) Every surface said the row is updated when 'that tab/editor next flushes', which over-specifies the mechanism in a way an agent could act on. The row is updated by any later ?source=collab-snapshot write: in practice that is the tab which applied the content, but the row does not care which caller sends it, and an open tab may be non-electable, so 'that tab' is neither necessary nor sufficient. Swept as a class again rather than per site — CLI help, CLI stderr line, MCP catalog, instructions.md, CLAUDE.md, the TypeScript mirror, the models doc comment and the constant's own comment. Verified by re-running the grep that found them: no single-tab attribution remains. The review also cited README as carrying this text; it does not — the grep returns nothing there, and nothing in this branch touched it. * fix(test): the applier probe loop was flaky, and the marker is write-scoped (codex round 6) Two findings, and the first one was real in a way I had not seen. The review said the focused tests fail on a 429 from the shared waitForApplierPath probe loop. My own runs were green, so I ran it the way that would show it instead of assuming: -count=5 across both BUG-2995 tests fails with exactly that 429. One test at -count=1 stays inside the API limiter's burst; two at -count=5 do not. The helper now waits out the limiter rather than failing on it, and keeps the diagnoses apart — being rate-limited throughout is inconclusive about the route, not evidence the server wrote items.content directly. That exposed a second flake underneath: with the probe slowed, election lost the race in 2 runs of 5 against a 3s deadline. That deadline is a READINESS wait, not an assertion — every millisecond is spent only while the room is not ready — so a too-short one invents a flaky test rather than catching a product bug. Raised to 15s, which the passing runs never approach (they finish in about a second). Stable over four -count=5 runs, including alongside the BUG-2840 test that shares the helper. The second finding: the marker could be stale, since a concurrent collab-snapshot flush can update the row between the apply and the response. Rather than synchronise, every surface now says what the value actually means — it describes THIS WRITE, that the content went to the document rather than the row, and is not a live reading of the row. The caller's action is unchanged either way: re-read rather than re-send. * docs: the CLI line said the stored copy has not been updated (codex round 7) Last surface still phrased as a live reading of the row. A concurrent collab-snapshot flush may already have updated it, so the honest form is what this request did: it applied the content to the document and did not write the stored copy itself. Also sharpens the probe loop's rate-limited failure message. The flag is sticky, so some probes did complete — the branch does not suppress a real failure, it declines to name a cause the run did not measure. * fix(test): the applier readiness helper stops paying the rate limiter (BUG-2995, CI) CI's Go job failed on TestApplierPathRefusalLeavesNoOpLogRow_OpenChildren — a test green on main, untouched by this branch, and a THIRD caller of waitForApplierPath I had not accounted for. It spent its whole budget being answered 429. The helper inferred the content route by PATCHing every few milliseconds and watching whether items.content changed, with a comment saying no exported accessor for electable conns existed. That was true when it was written and stopped being true at TASK-2987, which added RoomManager.HasElectableApplier so the handler could learn the route without taking it. The helper now asks that predicate directly. Leaving it as a write loop was not just wasteful: those writes go through the ordinary API limiter (600/min, burst 60) and the bucket is shared across the package, so every caller drains it for the tests that follow. One caller stayed under the burst. Adding a second pushed a third, unrelated test over the edge — a helper that consumes a shared exhaustible resource makes other tests fail as a function of how many ran first, which is the worst kind of flake to read from a log. Asking the predicate costs nothing, cannot be rate-limited, and answers the exact question rather than inferring it from a side effect. The three callers now run in about a second at -count=3 where the probe loop took multiple seconds and could fail outright.