mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-10-25 14:00:40 +00:00
fix(items): eleven hand-built doors carry the stale-body marker (BUG-3033) (#1344)
* docs: make test-pg is not safe against itself in one worktree (BUG-3033) CLAUDE.md said the target is safe across worktrees, which is true and incomplete. TEST_PG_PROJECT is basename + cksum of the absolute path, so two overlapping runs in ONE worktree share a compose project: the second `up -d --wait` attaches to the first run's container, and whichever run finishes first executes the unconditional `down -v` and destroys the other's database mid-test. Measured on the BUG-3000 trail, where the survivor's connection-refused failures were first read as evidence about the code. The note also records that a PASSING run carries ~41 redis connection-refused lines, so that noise is not the diagnostic — the failure text is. Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA * fix(items): six hand-built doors carry the stale-body marker (BUG-3033) BUG-3000 put content_state on models.Item, so every door that serialises the struct inherits it. These build their own shape and inherit nothing: - PlaybookRunResponse.Body (handlers_playbooks.go) - AgentBootstrapConvention.Content (handlers_bootstrap.go) - BootstrapIncludeItem.Content, bodies mode (handlers_bootstrap.go) - the artifact export's markdown+YAML (handlers_artifact_export.go) - the MCP item resource's composed markdown (mcp/resources.go) - `pad playbook show` / `pad playbook run`, which decode narrow structs of their own and print the body The last two are in the unit because an agent reads what the CLI prints, not the wire: a marker the server sets and the CLI drops is a fix its reader never sees — the same gap this bug describes for the MCP resource, whose fetcher reads stdout and cannot surface a successful command's stderr. BootstrapIncludeItem is a fifth server door the bug does not name, found by this unit's sweep and folded in under CONVE-18 rather than filed: same file, same handler, same shape as the conventions door beside it. The artifact export is a portable format, so the marker goes into the existing provenance block as an additive omitempty key with NO format_version bump. Decode uses a plain yaml.Unmarshal with no KnownFields, so an older binary ignores the key; format_version is compared with exact equality, so a bump would reject artifacts in both directions for an added optional key. A current item's bytes are unchanged. It signals rather than refuses; BUG-3032 owns the signal/refuse/accept decision for the bundle format, and an additive key forecloses neither of the other two. Every marker is emitted only where a real body is — the includes site sets it inside the same wantBodies guard that sets Content — so a metadata-only projection cannot claim its absent body is stale. Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA * test(items): both-directions coverage for the six BUG-3033 doors One test per door, both directions on one item, ABSENCE ASSERTED FIRST. The order is the instrument: a marker stamped unconditionally satisfies every positive assertion in these files, so a presence leg proves something only after the same door has been shown to stay silent about a current row. Assertions are on the raw wire form — a decoded map, the exported bytes, the rendered markdown — rather than on a Go struct, because a struct round-trip passes even when the field is never serialised. That is exactly the failure this bug is: a value computed correctly in the store and dropped by a projection on the way out. Four legs beyond the plain both-directions pair: - PlaybookShowResponse inherits the marker through an embed nobody tested. Pinned, because a refactor to an explicit projection — the shape BootstrapCollection and BootstrapRole already took for size — would drop it silently. - A metadata-mode include must carry NO marker even when its item is stale, since it serves no body; the premise (that the group really withheld the body) is asserted before the conclusion. - A current artifact must not merely lack the value but lack the KEY, and a marked one must differ by exactly one line and still decode with body and fields intact. Those are the compatibility claims made testable rather than asserted in a comment. - An UNRECOGNISED content_state must not render as this unit's specific claim about a live editor. The vocabulary is shared with BUG-2995's content_outcome and is expected to grow. The CLI and MCP legs drive the command and the resource rather than the helper and the formatter (CONVE-19): a direct call vouches for the function while leaving the narrow decode struct — where the defect actually lives — free to omit the key. Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA * docs: the read-side content_state marker (BUG-3033, CONVE-23 sweep) Three consumer-facing artifacts described the write-side warning and said nothing about its read-side counterpart, so an agent reading them would still conclude a stale read is silent. That was already true before this change — BUG-3000 shipped the marker without documenting it outside the field's own doc comment — and adding six more doors is what makes it worth closing rather than inheriting. - CLAUDE.md gains a read-responses bullet beside the three write warnings: what the value means, that it is emitted only where a real body is, that the window is UNBOUNDED because nothing server-side flushes, and that the correct response is re-read later, never re-send. - internal/mcp/instructions.md had a sentence saying a get before the flush "answers with the PREVIOUS content" — true and now incomplete, since the get says so itself. It also gains the case an agent cannot infer from its own write: someone else's tab holds the edits and you wrote nothing. pad_playbook's entry says a marked body may be superseded, and that this is a reason to re-read, not to refuse to run. - internal/mcp/version.go gains a POST-0.31 NO BUMP entry. The disposition is BUG-2304's, argued at length in the BUG-2995 entry above it: nothing is dropped or retyped, and a current item renders exactly as before. The item resource is the half worth naming — a text resource has no additive key, so the marker is a line of markdown, and a consumer diffing that text sees a new block when and only when the body it is about is stale. Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA * docs: contract comments name the marker they now emit (BUG-3033) formatItemAsMarkdown's doc listed the blocks it composes and this change added one; the artifact's compatibility claim now cites the instrument that proves it rather than asserting it (testdata/*.golden.md pin the encoded bytes exactly and were not touched to land the key). Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA * fix(cli): pad item edit warns before seeding $EDITOR from a stale body (BUG-3033) A seventh door, found by re-sweeping the population, and the only one whose consequence is not a confusing read. `pad item edit` is a read-modify-write over the WHOLE body: it seeds the editor from the row and PATCHes the entire edited text back with no concurrency token. When the row is behind the live collaborative document, the editor is seeded with content that predates edits which exist and are durable in the op-log, and the save replaces them with a version derived from a state before them — visible to neither the person editing nor the person whose tab holds them. Unlike an ordinary lost update this needs only ONE writer plus an open tab; the other side is the unflushed state of a document nobody is typing in. The warning is printed BEFORE the editor launches, which is the only point at which it can still change what the user does — afterwards it would be read beside an "Updated TASK-5" line. Its wording differs from its two siblings for the reason a third one exists at all: the consequence is that saving destroys something, not that the text is old. It warns and proceeds. Whether `edit` should refuse, prompt, or send a DIFF instead of the whole body is a behaviour decision with its own item (BUG-3035, which also asks whether `item update --stdin` joins any gate `edit` grows), not something to settle inside a sweep for missing markers. Two tests, and the second is the load-bearing one: the gating table vouches for the helper, and the end-to-end leg vouches for the BINDING — a fake editor announces itself on its own stderr, which OpenInEditor wires to os.Stderr, so the ordering claim is an assertion about one stream rather than about presence. Both mutants (drop the call; move it after the editor) are killed by that leg alone. Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA * fix(items): playbook summaries carry the marker; two tests stop passing wrongly Codex round 1, three findings, all valid. P2 — an eighth door, and this unit's population table missed it with the precedent already in the tree. A playbook `summary` is the body's first paragraph truncated, so a stale body makes a stale summary, and an agent ROUTES on that description. BUG-3000 put the marker on cli.ItemSummary.ContentPreview on exactly this reasoning ("a content_preview is content"); calling the same bytes a summary does not change where they came from. Carried on AgentBootstrapPlaybookMeta, which both the bootstrap blob and the /playbooks endpoint render, and through both CLI renderers — one stderr line naming the stale refs rather than a per-row marker, since a per-row marker on a long list is noise and a bare count sends the reader back to diff it by hand. Set only when a summary was actually emitted: no summary, no claim. It also does not mark the other metadata, which comes from the fields blob that the collaborative document does not hold. P3 — the edit tests accepted an always-on warning. They supplied only a stale response, so a call site forced to warn unconditionally passed them, and the helper's gating table cannot see a wiring error. Now both directions at the COMMAND boundary, with two premises asserted before any conclusion: the fake editor ran, and the edited body was really PATCHed. P3 — the artifact test accepted a corrupted body. It compared line counts and asserted only that the decoded body was non-empty, so an export that replaced the body with unrelated text passed. Now the EXACT body on both exports, plus byte equality after removing the marker line and normalising exported_at — with a check that the normalisation removed something, so the comparison cannot pass by stripping both sides. Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA * fix(items): backlink + search snippets carry the marker; add the class guard Codex round 2: two more doors and two test gaps, all valid. P2 — backlink snippets. models.Backlink.Snippet is a window cut from the SOURCE item's body, so a stale body makes a stale snippet. This unit's sweep could not see it: its vantage point was internal/server, internal/mcp and internal/cli, and this shape is built in the STORE. The predicate needed a non-default table alias (the backlink queries call items `s`), so contentStateSQL became contentStateSQLFor(alias) with the old constant as its "i" case — a function rather than a second copy, since two hand-maintained spellings of this predicate would be two chances to drift. The marker describes the SOURCE of each link, never the target the caller asked about, and a test asserts marking the target changes nothing. P2 — `pad item search` and `pad item backlinks` printed body-derived snippets and dropped the marker the wire already carried. Both now use one shared renderer with the playbook summaries, so the surfaces cannot word the same fact three ways. P3 — the edit test asserted only that a PATCH occurred, though it claimed to exercise saving; it now asserts the exact content that reached the server. P3 — the summary test had one stale row, which cannot tell a correct renderer from one naming every ref or only the first; it now interleaves stale and current entries, asserts the complete set and EXACTLY ONE warning line, and covers the bootstrap renderer that had no test at all. CONVE-35 — three rounds each found another member of this class, which means the reviews were measuring my enumeration rather than the code. So the table is now a test: every site turning an item body into text under another name is listed with its ruling, keyed on the exact call LINES with multiplicity, and anything else fails closed. Its first draft keyed on file+helper and a negative control caught the hole: a SECOND call site in an already-listed file passed silently — and handlers_bootstrap.go, which is listed, is exactly where a new projection would be written. Four controls now run: a new compiling call site goes red, a broken walk root goes red, a removed site goes red, and comments mentioning a helper stay green. The guard states what it cannot see — a door built on a helper it does not know — and its failure message asks for the helper, not just the site. Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA * fix(items): title-only search results drop the body marker; guard parses Go Codex round 3, six findings, all valid. P2, and a defect this unit INTRODUCED: Store.Search's direct-ref and numeric paths set Snippet = Item.Title and clear the body, but kept the BODY's content_state — so the new CLI renderer announced that a TITLE was derived from a stale body. Cleared at the source rather than patched in the renderer, which restores models.Item.ContentState's documented invariant and fixes every other consumer of those results at the same time. The FTS path beside it keeps the marker deliberately: there the snippet IS cut from the body, so the marker still qualifies text the result serves. A test asserts both directions, because "clear it everywhere" would delete the signal this unit exists for. P2 — the enumeration guard was itself fail-open. It passed a one-line function calling a helper, a helper assigned to a local and called through it, a comment inside the call expression, and an argument changed below the opening line of a multiline call, while FAILING on prose and strings that merely mentioned a helper. Wrong in both directions is the signature of a scanner not reading the language it claims to read, so it now parses Go: call expressions matched on the AST, where comments and line breaks are not facts. A helper NAME used as a value is reported too and fails closed, since the call through an alias is invisible. All four of codex's bypasses now go red and both false-positive cases stay green. P2 — FTSSnippet was missing from the helper list entirely, so four existing derivation sites were outside the table. Added and ruled; the stated boundary was understating itself and now says so. P2 — browser renderers (BacklinksPanel, CommandPalette) drop the marker. The wire and the TypeScript type now carry it; where a stale badge belongs in a panel or a palette is a UI decision, filed as BUG-3050 on the same grounds BUG-3033 was split from BUG-3000. P3 — cross-workspace backlinks had no coverage: clearing ContentState on every result of that query left the same-workspace test and the existing cross-workspace tests all green. P3 — the snippet tests had ONE stale row while their comment claimed interleaving, the same gap fixed for summaries one test earlier; a renderer reporting only the first stale source passed them. Both now use multiple stale rows separated by current ones, and that mutant dies on all four legs. Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA * fix(items): close the guard's helper-named bypass; seven tests stop passing wrongly Codex round 4, enumeration-prompted. No new Go door — its census of 434 non-test files found the two remaining Go omissions are already filed (workspace export on BUG-3032, cross-workspace copy attached to it) and the browser set belongs to BUG-3050. Everything below is in-class. The guard had a real bypass. A declaration whose own NAME was a helper got a special traversal that checked CallExprs and returned false, so the value-use arm never ran inside it: a compiling, callable `func (p ReviewProjection) PlaybookSummary() string { render := contentPreview; return render(p.Content) }` served unmarked stale prose while the guard passed, and renaming the method made it fail. The arm was redundant as well as wrong — notAValueUse already marks every FuncDecl.Name — so it is gone. It also skipped a `build` directory it never disclosed, which hid a real file at internal/server/build/. Three doc claims it made were false and are corrected rather than softened: "anywhere under internal/ or cmd/" (it skips four directories, now named), "helper values fail closed" (they did not), and that the browser surface is a pure rendering choice (the share paths do field copies that discard the marker before any rendering decision). It now states plainly that its claim is narrow — no UNRULED USE of four helpers — and must not be cited as proof that every door carries the marker. Seven mutations survived the tests, all the same species: they asserted the MARKER without asserting the BODY it qualifies. Replacing a served body with "WRONG BODY", dropping it entirely, or suppressing stale summaries and snippets while keeping the warnings all passed. Every door test now pins the exact text, and the backlink premise finally sees the snippet it is about — cli.Dim writes to color.Output, so an os.Stdout swap missed it and an earlier draft recorded that as a limit and asserted row headings instead, which is precisely what let that mutation live. Also: the numeric direct-ref path had no coverage (the test queried a prefixed ref only), backlink source-correlation passed when replaced with "any op-log row exists anywhere", and the FTS leg never asserted a snippet was produced. Search's surviving-marker comment claimed it always qualifies served text; an FTS title match on an empty-bodied item is the counter-example, and the comment now says so. Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA
This commit is contained in:
xarmian
@@ -43,7 +43,7 @@ Agent sessions take a `git worktree` per task rather than sharing the main check
|
||||
|
||||
- **Never run `npm ci` in a worktree whose `web/node_modules` is a symlink — including via make.** `npm ci` lives in the `web` target, so every target whose dependency chain reaches it is off-limits too: currently `web`, `build`, `install`, `serve`, `web-check`, and `check` (via `web-check`). Everything else — `build-go`, `dev`, `restart`, `test`, `test-pg`, `lint`, `vuln`, `web-test`, `web-audit`, `dev-web`, `clean` — never reaches `npm ci`. `npm ci` deletes through the symlink into the shared tree, breaking every other worktree and session at once with a confusing `vitest: not found`. If you want a real, isolated `node_modules` instead of a symlink, `npm ci` in an un-symlinked `web/` is ~5s on a warm cache and regenerates `.svelte-kit` as a side effect.
|
||||
|
||||
- **`make test-pg` is safe to run from several worktrees at once** (TASK-2708). It used to bind the Postgres test container to a fixed host port, so a second worktree failed with `port is already allocated` and a stack orphaned by a removed worktree blocked the port for everyone. Docker now assigns the port and the Makefile reads it back, so each worktree gets its own container on its own port under its own compose project. If you have been starting a private container by hand to avoid the collision, you no longer need to. Two other things that target now does: it REFUSES to run, with a `NO TESTS EXECUTED` banner, when the database is unreachable — `go test` exiting 2 with zero FAIL lines had already been mistaken for a pass once — and it says so explicitly when the database dies mid-run, so the failures read as infrastructure rather than as evidence about the code. To reap a stack whose worktree was deleted before teardown: `docker ps --filter name=padtest-` lists them and the container name carries the compose project, so `docker compose -p <that-project> down -v` reaps it from anywhere. From inside the worktree, `make test-pg-project` prints the name and `make test-pg-down` does it for you. (The project is `padtest-<basename>-<checksum-of-the-absolute-path>` — NOT the bare directory name, which would collide between two checkouts sharing a basename.)
|
||||
- **`make test-pg` is safe to run from several worktrees at once, but NOT against itself in one worktree** (TASK-2708; the second half measured on BUG-3000). `TEST_PG_PROJECT` is derived from the worktree's basename and a checksum of its absolute path, so two overlapping runs in the SAME worktree resolve to the SAME compose project: the second `up -d --wait` attaches to the first run's container instead of creating its own, and the first run to finish executes the target's unconditional `down -v` and tears the database out from under the other. The survivor fails with `dial tcp 127.0.0.1:<port>: connect: connection refused` while cloning the test database — which the `THE DATABASE DIED DURING THE RUN` banner reports correctly, but reads as evidence about the code to anyone who does not know a sibling run was up. Run it alone per worktree, and do not diagnose such a failure from surrounding connection-refused noise (a passing run carries ~41 redis connection-refused lines of its own); the failure TEXT is the evidence. It used to bind the Postgres test container to a fixed host port, so a second worktree failed with `port is already allocated` and a stack orphaned by a removed worktree blocked the port for everyone. Docker now assigns the port and the Makefile reads it back, so each worktree gets its own container on its own port under its own compose project. If you have been starting a private container by hand to avoid the collision, you no longer need to. Two other things that target now does: it REFUSES to run, with a `NO TESTS EXECUTED` banner, when the database is unreachable — `go test` exiting 2 with zero FAIL lines had already been mistaken for a pass once — and it says so explicitly when the database dies mid-run, so the failures read as infrastructure rather than as evidence about the code. To reap a stack whose worktree was deleted before teardown: `docker ps --filter name=padtest-` lists them and the container name carries the compose project, so `docker compose -p <that-project> down -v` reaps it from anywhere. From inside the worktree, `make test-pg-project` prints the name and `make test-pg-down` does it for you. (The project is `padtest-<basename>-<checksum-of-the-absolute-path>` — NOT the bare directory name, which would collide between two checkouts sharing a basename.)
|
||||
|
||||
`web/vitest.config.ts`'s `server.fs.allow` note covers the other worktree wrinkle (symlink realpaths vs the dev-server file-serving guard) and points back at this section.
|
||||
|
||||
@@ -82,6 +82,7 @@ REST API at `/api/v1/`. Key endpoints:
|
||||
- `GET/PATCH/DELETE /workspaces/{ws}/items/{slug}` — item by slug
|
||||
- **Write responses (create/update) may carry `warnings.undeclared_fields`** (BUG-2850) — field keys stored in the item's `fields` blob that the collection's schema does not declare. They are ACCEPTED, not refused: a census found 168 live values under 14 such keys, and refusing them would break read-modify-write on items nobody edited wrongly. The element is additive and `omitempty`, so a clean write is byte-identical to before; system-written metadata (`implementation_notes`, `decision_log`, `github_pr`, `convention`) is excluded. The CLI prints the same list to **stderr**, never stdout, so `--format json` stays parseable
|
||||
- **Write responses may also carry `warnings.content_outcome`** (BUG-2995) — on a 200 it takes exactly one value, `applied_pending_flush`: the content went to the live collaborative document (a browser tab has the item open) rather than to `items.content`. It describes the WRITE, not the row's state at response time — a concurrent flush may already have updated the row, and the server does not re-read to find out. The response's `content` is then the markdown as **sent**, not as stored, and a read before a later `?source=collab-snapshot` flush lands answers with the PREVIOUS content — the lag, not a failed write. The stored form may also differ from the sent form: the markdown round-trips through the editor, so bullet markers, emphasis characters, list numbering and blank-line runs normalise, while already-canonical markdown survives unchanged (measured — `web/src/lib/collab/bug2995Roundtrip.svelte.test.ts`). No duration is claimed anywhere, and the flush is NOT guaranteed to happen at all (BUG-3000). The CLI prints this to **stderr**, like the other write warnings
|
||||
- **READ responses may carry `content_state`** (BUG-3000 / BUG-3033) — the read-side counterpart to `warnings.content_outcome` above, and it takes the same one value, `applied_pending_flush`. A write warning says what a REQUEST did; this says what the ROW is: the item's op-log holds updates above `items.content_flushed_op_log_id`, so the body being served is BEHIND the live collaborative document. Additive and `omitempty`, and emitted only where a real body is — a summary or metadata projection that serves no content never claims its absent body is stale. The window is **unbounded**: nothing server-side moves that content into `items.content` (the op-log GC's flush-coverage guard deliberately RETAINS unflushed rows rather than flushing them), so the row catches up only when a browser tab next opens the item and flushes, which may never happen. The content is not lost — the op-log is the durability layer — but the marker is a signal to RE-READ LATER, never to re-send. The predicate is broader than "an API write is pending flush" and deliberately so: a user typing in an open tab produces the same stale body, and the row does not record WHY the op-log is ahead. Carried by every door that serialises `models.Item`, and by the hand-built ones too — the playbook `run` response, bootstrap convention and bodies-mode include payloads, the exported artifact's `provenance` block, and a line above the body in the MCP item resource. The CLI prints it to **stderr** (`pad item show`, `pad playbook show`, `pad playbook run`), like the write warnings
|
||||
- **Write responses may also carry `warnings.dropped_fields`** (TASK-2878) — schema-declared keys the write DISCARDED. Today one case: a `relation` field whose schema DEFAULT is not a reference. `ValidateFields` assigns a default and skips its own type check, so an injected default is the only route by which a non-string reaches a relation field; a caller-supplied one is type-checked and refused. Dropped rather than refused because nobody in the request typed it. Additive and `omitempty`
|
||||
- `POST /workspaces/{ws}/items/{slug}/copy/preflight` — cross-workspace copy dry run: what would carry / drop / need a value, plus the full warning set. Read-only and safe to call repeatedly (PLAN-2357)
|
||||
- `POST /workspaces/{ws}/items/{slug}/copy` — cross-workspace copy; with `archive_source` it is the move. Same request shape as the preflight. **Never retry it automatically** — there is no idempotency key, so a retry duplicates the item
|
||||
|
||||
@@ -8,6 +8,7 @@ import (
|
||||
"github.com/spf13/cobra"
|
||||
|
||||
"github.com/PerpetualSoftware/pad/internal/cli"
|
||||
"github.com/PerpetualSoftware/pad/internal/models"
|
||||
)
|
||||
|
||||
// bootstrapCmd returns the agent bootstrap blob — the consolidated
|
||||
@@ -100,6 +101,7 @@ func printBootstrapMarkdown(raw []byte) error {
|
||||
InvocationSlug string `json:"invocation_slug"`
|
||||
Trigger string `json:"trigger"`
|
||||
Summary string `json:"summary"`
|
||||
ContentState string `json:"content_state"`
|
||||
} `json:"playbooks"`
|
||||
}
|
||||
if err := json.Unmarshal(raw, &b); err != nil {
|
||||
@@ -158,7 +160,11 @@ func printBootstrapMarkdown(raw []byte) error {
|
||||
fmt.Println()
|
||||
|
||||
fmt.Printf("## Playbooks (%d)\n", len(b.Playbooks))
|
||||
var stalePlaybookSummaries []string
|
||||
for _, p := range b.Playbooks {
|
||||
if p.ContentState == models.ContentOutcomeAppliedPendingFlush {
|
||||
stalePlaybookSummaries = append(stalePlaybookSummaries, p.Ref)
|
||||
}
|
||||
invocation := "—"
|
||||
if p.InvocationSlug != "" {
|
||||
invocation = "/pad " + p.InvocationSlug
|
||||
@@ -168,6 +174,9 @@ func printBootstrapMarkdown(raw []byte) error {
|
||||
fmt.Printf(" %s\n", p.Summary)
|
||||
}
|
||||
}
|
||||
// BUG-3033 — same helper the `playbook list` renderer uses, so the two
|
||||
// surfaces cannot word this differently.
|
||||
warnStalePlaybookSummaries(stalePlaybookSummaries)
|
||||
return nil
|
||||
}
|
||||
|
||||
|
||||
@@ -3126,6 +3126,7 @@ Examples:
|
||||
return nil
|
||||
}
|
||||
fmt.Printf("Backlinks to %s\n\n", cli.Bold.Sprint(label))
|
||||
var staleBacklinks []string
|
||||
for _, bl := range backlinks {
|
||||
header := bl.SourceRef + " " + bl.SourceTitle
|
||||
if bl.SourceCollectionIcon != "" {
|
||||
@@ -3134,6 +3135,12 @@ Examples:
|
||||
fmt.Printf("%s\n", cli.Bold.Sprint(header))
|
||||
if bl.Snippet != "" {
|
||||
cli.Dim.Printf(" %s\n", bl.Snippet)
|
||||
// BUG-3033 — the marker describes the SOURCE item of each
|
||||
// link, so the refs named are source refs, not the target
|
||||
// the caller asked about.
|
||||
if bl.ContentState == models.ContentOutcomeAppliedPendingFlush {
|
||||
staleBacklinks = append(staleBacklinks, bl.SourceRef)
|
||||
}
|
||||
}
|
||||
if bl.DisplayText != nil {
|
||||
// nil = no `|` in body; non-nil = `[[X|...]]`,
|
||||
@@ -3144,6 +3151,7 @@ Examples:
|
||||
}
|
||||
fmt.Println()
|
||||
}
|
||||
warnStaleDerivedText("snippet", staleBacklinks, "`pad item show <ref>`")
|
||||
return nil
|
||||
},
|
||||
}
|
||||
@@ -3425,6 +3433,7 @@ func searchCmd() *cobra.Command {
|
||||
return nil
|
||||
}
|
||||
|
||||
var staleSnippets []string
|
||||
for _, r := range searchResp.Results {
|
||||
icon := r.Item.CollectionIcon
|
||||
if icon == "" {
|
||||
@@ -3433,9 +3442,27 @@ func searchCmd() *cobra.Command {
|
||||
fmt.Printf("%s %s (%s)\n", icon, r.Item.Title, r.Item.CollectionName)
|
||||
if r.Snippet != "" {
|
||||
fmt.Printf(" %s\n", r.Snippet)
|
||||
// BUG-3033. The JSON form carries the marker on the embedded
|
||||
// item; this renderer decodes that item and prints only the
|
||||
// snippet, so without this the signal stops here.
|
||||
if r.Item.ContentState == models.ContentOutcomeAppliedPendingFlush {
|
||||
// Ref first, ItemRef as the fallback: the server computes
|
||||
// `ref` on the item it returns, but this renderer must name
|
||||
// something actionable even from a payload that omits it,
|
||||
// and a warning naming "" would be worse than none.
|
||||
name := r.Item.Ref
|
||||
if name == "" {
|
||||
name = cli.ItemRef(r.Item)
|
||||
}
|
||||
if name == "" {
|
||||
name = r.Item.Title
|
||||
}
|
||||
staleSnippets = append(staleSnippets, name)
|
||||
}
|
||||
}
|
||||
fmt.Println()
|
||||
}
|
||||
warnStaleDerivedText("snippet", staleSnippets, "`pad item show <ref>`")
|
||||
|
||||
showing := len(searchResp.Results)
|
||||
if showing == 0 && searchResp.Total > 0 {
|
||||
@@ -3498,6 +3525,10 @@ Set EDITOR or VISUAL env var to choose your editor (default: vi).`,
|
||||
return err
|
||||
}
|
||||
|
||||
// BEFORE the editor opens, not after: this is the only point at which
|
||||
// the warning can still change what the user does (BUG-3033).
|
||||
warnStaleEditSeed(item)
|
||||
|
||||
edited, err := cli.OpenInEditor(cfg, item.Content, ".md")
|
||||
if err != nil {
|
||||
return err
|
||||
@@ -3861,6 +3892,64 @@ func warnContentStale(item *models.Item) {
|
||||
"server forces that to happen.")
|
||||
}
|
||||
|
||||
// warnStaleDerivedText prints ONE line to STDERR naming the items whose
|
||||
// body-DERIVED text in a listing came from a body the server knows is behind its
|
||||
// live collaborative document (BUG-3033).
|
||||
//
|
||||
// Snippets, previews and summaries are all windows onto the body rather than
|
||||
// metadata about it, so a stale body makes every one of them stale. They share
|
||||
// this renderer so the surfaces cannot word the same fact differently — three
|
||||
// listings drifting into three phrasings is how a reader learns to ignore all
|
||||
// three.
|
||||
//
|
||||
// One line for the whole listing rather than one per row, and it NAMES the
|
||||
// refs: a per-row marker on a long list is noise, while a bare count sends the
|
||||
// reader back to diff it by hand. `what` names the kind of text ("snippet",
|
||||
// "summary") and `howToSee` is the command that shows what is actually stored.
|
||||
//
|
||||
// Empty is the common case and prints nothing, so a current listing is
|
||||
// byte-identical on both streams.
|
||||
func warnStaleDerivedText(what string, refs []string, howToSee string) {
|
||||
if len(refs) == 0 {
|
||||
return
|
||||
}
|
||||
fmt.Fprintf(os.Stderr, "warning: the %s shown for %s is derived from a body that is behind its "+
|
||||
"live collaborative document — an editor holds edits that have not been written back yet, so "+
|
||||
"the text may be out of date. Run %s to see what is stored.\n",
|
||||
what, strings.Join(refs, ", "), howToSee)
|
||||
}
|
||||
|
||||
// warnStaleEditSeed prints one line to STDERR before `pad item edit` opens the
|
||||
// editor on a body the server knows is BEHIND the item's live collaborative
|
||||
// document (BUG-3033).
|
||||
//
|
||||
// A third wording rather than reusing warnContentStale, for the reason that one
|
||||
// exists separately from the playbook wording: the consequence differs, and the
|
||||
// consequence is the whole content of a warning. A reader of `item show` is told
|
||||
// the text may be old. An EDITOR is told something worse — this command is a
|
||||
// read-modify-write, and the body it is seeding the editor with predates edits
|
||||
// that exist. Saving sends the whole body back, so those edits are replaced by a
|
||||
// version derived from a state before them, and neither the person editing nor
|
||||
// the person whose tab holds them sees it happen.
|
||||
//
|
||||
// What this line does NOT do is stop that: it warns and proceeds. Whether the
|
||||
// command should refuse, or offer to merge, is a behaviour decision with its own
|
||||
// item (BUG-3035) rather than something to settle inside a sweep for missing
|
||||
// markers.
|
||||
//
|
||||
// It is printed BEFORE the editor is launched. Printed afterwards it would be
|
||||
// read, at best, next to a "Updated TASK-5" line — after the damage.
|
||||
func warnStaleEditSeed(item *models.Item) {
|
||||
if item == nil || item.ContentState != models.ContentOutcomeAppliedPendingFlush {
|
||||
return
|
||||
}
|
||||
fmt.Fprintln(os.Stderr, "warning: this item's stored content is behind its live collaborative "+
|
||||
"document — an editor holds edits that have not been written back yet. You are about to edit "+
|
||||
"the PREVIOUS content, and saving will send your whole version back, replacing those edits "+
|
||||
"with one derived from a state before them. Open the item in a browser tab first if you need "+
|
||||
"them.")
|
||||
}
|
||||
|
||||
// warnContentPendingFlush prints one line to STDERR when a content write reached
|
||||
// the collaborative document but not yet items.content (BUG-2995).
|
||||
//
|
||||
|
||||
@@ -3,10 +3,12 @@ package main
|
||||
import (
|
||||
"encoding/json"
|
||||
"fmt"
|
||||
"os"
|
||||
|
||||
"github.com/spf13/cobra"
|
||||
|
||||
"github.com/PerpetualSoftware/pad/internal/cli"
|
||||
"github.com/PerpetualSoftware/pad/internal/models"
|
||||
)
|
||||
|
||||
// playbookCmd is the `pad playbook` command group for the first-class
|
||||
@@ -55,6 +57,7 @@ func playbookListCmd() *cobra.Command {
|
||||
Status string `json:"status"`
|
||||
HasArguments bool `json:"has_arguments"`
|
||||
Summary string `json:"summary"`
|
||||
ContentState string `json:"content_state"`
|
||||
}
|
||||
if err := json.Unmarshal(raw, &list); err != nil {
|
||||
return fmt.Errorf("decode playbooks: %w", err)
|
||||
@@ -63,7 +66,11 @@ func playbookListCmd() *cobra.Command {
|
||||
fmt.Println("No playbooks in this workspace yet.")
|
||||
return nil
|
||||
}
|
||||
var staleSummaries []string
|
||||
for _, p := range list {
|
||||
if p.ContentState == models.ContentOutcomeAppliedPendingFlush {
|
||||
staleSummaries = append(staleSummaries, p.Ref)
|
||||
}
|
||||
slug := "—"
|
||||
if p.InvocationSlug != "" {
|
||||
slug = "/pad " + p.InvocationSlug
|
||||
@@ -80,6 +87,7 @@ func playbookListCmd() *cobra.Command {
|
||||
}
|
||||
fmt.Println()
|
||||
}
|
||||
warnStalePlaybookSummaries(staleSummaries)
|
||||
return nil
|
||||
},
|
||||
}
|
||||
@@ -112,10 +120,23 @@ func playbookShowCmd() *cobra.Command {
|
||||
Title string `json:"title"`
|
||||
Content string `json:"content"`
|
||||
Fields string `json:"fields"`
|
||||
// BUG-3033. The server marks a body it knows is behind the item's
|
||||
// live collaborative document, but this command decodes a narrow
|
||||
// struct of its own and prints only the body — so without this key
|
||||
// the marker reaches the wire and never reaches the reader. That is
|
||||
// the same gap BUG-3033 describes for the MCP item resource, which
|
||||
// re-renders `item show --format json` through a formatter that
|
||||
// drops the field.
|
||||
//
|
||||
// It matters most here: an agent loads a playbook body in order to
|
||||
// EXECUTE it, so a stale one is superseded steps being run, not a
|
||||
// stale page being read.
|
||||
ContentState string `json:"content_state"`
|
||||
}
|
||||
if err := json.Unmarshal(raw, &item); err != nil {
|
||||
return fmt.Errorf("decode playbook: %w", err)
|
||||
}
|
||||
warnPlaybookBodyStale(item.ContentState)
|
||||
fmt.Printf("# %s: %s\n\n", item.Ref, item.Title)
|
||||
if item.Content != "" {
|
||||
fmt.Println(item.Content)
|
||||
@@ -182,10 +203,14 @@ and run it anyway.`,
|
||||
Unbound []struct {
|
||||
Name string `json:"name"`
|
||||
} `json:"unbound"`
|
||||
// BUG-3033 — see the sibling in playbookShowCmd. Same reason, and
|
||||
// this is the door an agent actually invokes to run something.
|
||||
ContentState string `json:"content_state"`
|
||||
}
|
||||
if err := json.Unmarshal(raw, &resp); err != nil {
|
||||
return fmt.Errorf("decode run response: %w", err)
|
||||
}
|
||||
warnPlaybookBodyStale(resp.ContentState)
|
||||
fmt.Printf("# %s: %s\n\n", resp.Ref, resp.Title)
|
||||
if len(resp.BoundArgs) > 0 {
|
||||
fmt.Println("## Bound arguments")
|
||||
@@ -211,4 +236,45 @@ and run it anyway.`,
|
||||
return cmd
|
||||
}
|
||||
|
||||
// warnPlaybookBodyStale prints one line to STDERR when a playbook body this
|
||||
// command is about to print is one the server knows is BEHIND the item's live
|
||||
// collaborative document (BUG-3033).
|
||||
//
|
||||
// It takes the raw state string rather than a *models.Item because both callers
|
||||
// decode narrow anonymous structs of their own — `playbook show` reads an item
|
||||
// shape, `playbook run` reads a run-response shape — and neither has an item to
|
||||
// hand. The string is the whole contract, so one helper serves both and the two
|
||||
// doors cannot drift into different wording.
|
||||
//
|
||||
// Why the wording differs from cmd_item.go's warnContentStale, which says the
|
||||
// same thing about an ordinary item: this body is about to be EXECUTED. The
|
||||
// consequence a reader needs is "the steps below may be superseded", not "the
|
||||
// text below may be old", and the line says so.
|
||||
//
|
||||
// Stderr for the reason every warning in this CLI is on stderr: `--format json`
|
||||
// is piped into scripts, and the markdown form of both commands exists to be
|
||||
// redirected into a file. This one is never in that stream.
|
||||
func warnPlaybookBodyStale(contentState string) {
|
||||
if contentState != models.ContentOutcomeAppliedPendingFlush {
|
||||
return
|
||||
}
|
||||
fmt.Fprintln(os.Stderr, "warning: this playbook's stored body is behind its live collaborative "+
|
||||
"document — an editor holds edits that have not been written back yet, so the steps below "+
|
||||
"may be superseded. The body catches up when a tab next flushes the item, and nothing on "+
|
||||
"the server forces that to happen.")
|
||||
}
|
||||
|
||||
// warnStalePlaybookSummaries reports the listed playbooks whose summary is
|
||||
// derived from a body the server knows is behind its live collaborative
|
||||
// document (BUG-3033).
|
||||
//
|
||||
// A thin wrapper over the shared warnStaleDerivedText rather than its own
|
||||
// wording: a summary and a search snippet are the same kind of thing (a window
|
||||
// onto the body), and three listings phrasing that fact three ways is how a
|
||||
// reader learns to skip all three. Only the noun and the see-what-is-stored
|
||||
// command differ.
|
||||
func warnStalePlaybookSummaries(refs []string) {
|
||||
warnStaleDerivedText("summary", refs, "`pad playbook show <ref>`")
|
||||
}
|
||||
|
||||
// --- bootstrap ---
|
||||
|
||||
@@ -0,0 +1,682 @@
|
||||
package main
|
||||
|
||||
import (
|
||||
"encoding/json"
|
||||
"fmt"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
"github.com/fatih/color"
|
||||
"github.com/spf13/cobra"
|
||||
|
||||
"github.com/PerpetualSoftware/pad/internal/models"
|
||||
)
|
||||
|
||||
// BUG-3033, the CLI half — and the half the server-side fix is useless without.
|
||||
//
|
||||
// An agent does not read the wire; it reads what the CLI prints. `pad playbook
|
||||
// show` and `pad playbook run` each decode a NARROW anonymous struct of their
|
||||
// own and print the body, so a marker the server sets and these commands drop
|
||||
// is a fix its intended reader never sees. That is the same gap this bug
|
||||
// describes for the MCP item resource.
|
||||
//
|
||||
// Driven through the cobra commands rather than by calling warnPlaybookBodyStale
|
||||
// directly (CONVE-19): a direct-call test vouches for the helper while leaving
|
||||
// the decode struct — which is where the defect actually lives — free to omit
|
||||
// the key. The helper's own gating is covered by the table at the bottom.
|
||||
//
|
||||
// Both directions on both commands, ABSENCE FIRST: a command that warned on
|
||||
// every body would satisfy every presence leg and be worthless, since an agent
|
||||
// would learn to ignore it.
|
||||
func TestPlaybookCommandsWarnOnAStaleBody(t *testing.T) {
|
||||
serve := func(t *testing.T, contentState string) {
|
||||
t.Helper()
|
||||
mux := http.NewServeMux()
|
||||
mux.HandleFunc("/api/v1/workspaces/ws/playbooks/ship", func(w http.ResponseWriter, r *http.Request) {
|
||||
body := map[string]any{
|
||||
"ref": "PLAYB-1", "title": "Ship", "slug": "ship",
|
||||
"content": "Step 1: do the thing", "fields": `{"status":"active"}`,
|
||||
"status": "active",
|
||||
}
|
||||
if contentState != "" {
|
||||
body["content_state"] = contentState
|
||||
}
|
||||
_ = json.NewEncoder(w).Encode(body)
|
||||
})
|
||||
mux.HandleFunc("/api/v1/workspaces/ws/playbooks/ship/run", func(w http.ResponseWriter, r *http.Request) {
|
||||
body := map[string]any{
|
||||
"ref": "PLAYB-1", "title": "Ship", "slug": "ship", "status": "active",
|
||||
"body": "Step 1: do the thing", "arguments": []any{}, "bound_args": map[string]any{},
|
||||
}
|
||||
if contentState != "" {
|
||||
body["content_state"] = contentState
|
||||
}
|
||||
_ = json.NewEncoder(w).Encode(body)
|
||||
})
|
||||
srv := httptest.NewServer(mux)
|
||||
t.Cleanup(srv.Close)
|
||||
setTempHomeMain(t)
|
||||
t.Setenv("PAD_URL", srv.URL)
|
||||
t.Setenv("PAD_TOKEN", "pad_testtoken")
|
||||
}
|
||||
|
||||
// The commands read package-level flag vars; restore them so test order
|
||||
// cannot leak a format through to an unrelated test.
|
||||
origWS, origFormat := workspaceFlag, formatFlag
|
||||
t.Cleanup(func() { workspaceFlag, formatFlag = origWS, origFormat })
|
||||
workspaceFlag, formatFlag = "ws", "markdown"
|
||||
|
||||
type door struct {
|
||||
name string
|
||||
run func(t *testing.T) (stdout, stderr string)
|
||||
}
|
||||
doors := []door{
|
||||
{"playbook show", func(t *testing.T) (string, string) {
|
||||
t.Helper()
|
||||
cmd := playbookShowCmd()
|
||||
cmd.SetArgs([]string{"ship"})
|
||||
var out string
|
||||
err := captureStderr(t, func() {
|
||||
out = captureStdout(t, func() {
|
||||
if e := cmd.Execute(); e != nil {
|
||||
t.Fatalf("playbook show: %v", e)
|
||||
}
|
||||
})
|
||||
})
|
||||
return out, err
|
||||
}},
|
||||
{"playbook run", func(t *testing.T) (string, string) {
|
||||
t.Helper()
|
||||
cmd := playbookRunCmd()
|
||||
cmd.SetArgs([]string{"ship"})
|
||||
var out string
|
||||
err := captureStderr(t, func() {
|
||||
out = captureStdout(t, func() {
|
||||
if e := cmd.Execute(); e != nil {
|
||||
t.Fatalf("playbook run: %v", e)
|
||||
}
|
||||
})
|
||||
})
|
||||
return out, err
|
||||
}},
|
||||
}
|
||||
|
||||
for _, d := range doors {
|
||||
t.Run(d.name, func(t *testing.T) {
|
||||
// ABSENCE FIRST.
|
||||
serve(t, "")
|
||||
stdout, stderr := d.run(t)
|
||||
if stdout == "" {
|
||||
t.Fatal("the command printed no body; nothing below would measure anything")
|
||||
}
|
||||
if strings.Contains(strings.ToLower(stderr), "behind its live collaborative") {
|
||||
t.Fatalf("a current body warned:\nstderr: %s", stderr)
|
||||
}
|
||||
|
||||
serve(t, models.ContentOutcomeAppliedPendingFlush)
|
||||
stdout, stderr = d.run(t)
|
||||
if !strings.Contains(strings.ToLower(stderr), "behind its live collaborative") {
|
||||
t.Errorf("a stale body printed no warning:\nstderr: %q", stderr)
|
||||
}
|
||||
// STDOUT must stay clean: `--format json` is piped into scripts and
|
||||
// the markdown form exists to be redirected into a file.
|
||||
if strings.Contains(strings.ToLower(stdout), "behind its live collaborative") {
|
||||
t.Errorf("the warning landed on STDOUT, corrupting a redirected body:\n%s", stdout)
|
||||
}
|
||||
// The body is still printed. The marker qualifies it; a command that
|
||||
// withheld the body would pass a warning-only assertion.
|
||||
if !strings.Contains(stdout, "Step 1: do the thing") {
|
||||
t.Errorf("the body is missing from a marked response:\n%s", stdout)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// TestWarnPlaybookBodyStaleGating pins the helper's own contract: it fires for
|
||||
// the one value the vocabulary defines and is silent for everything else.
|
||||
//
|
||||
// The value cannot drift between the server that writes it and this renderer —
|
||||
// both read models.ContentOutcomeAppliedPendingFlush — so what this guards is
|
||||
// the gate, not the spelling. The "some future value" row is the load-bearing
|
||||
// one: content_state shares its vocabulary with BUG-2995's content_outcome
|
||||
// deliberately, so that vocabulary is expected to grow, and a helper that fired
|
||||
// on any non-empty string would turn a value it has never heard of into this
|
||||
// specific claim about a live editor.
|
||||
func TestWarnPlaybookBodyStaleGating(t *testing.T) {
|
||||
cases := []struct {
|
||||
name string
|
||||
state string
|
||||
want bool
|
||||
}{
|
||||
{"empty — the common case", "", false},
|
||||
{"some future value", "some_future_value", false},
|
||||
{"the write-side sibling's other value", "content_not_applied", false},
|
||||
{"the value the server sends", models.ContentOutcomeAppliedPendingFlush, true},
|
||||
}
|
||||
for _, tc := range cases {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
out := captureStderr(t, func() { warnPlaybookBodyStale(tc.state) })
|
||||
if got := out != ""; got != tc.want {
|
||||
t.Fatalf("warned=%v want=%v (stderr: %q)", got, tc.want, out)
|
||||
}
|
||||
if !tc.want {
|
||||
return
|
||||
}
|
||||
// The consequence a reader of a PLAYBOOK needs is that the steps may
|
||||
// be superseded, not merely that the text may be old. That is the one
|
||||
// thing distinguishing this wording from cmd_item.go's sibling, and
|
||||
// it is the reason a second helper exists at all.
|
||||
if !strings.Contains(out, "superseded") {
|
||||
t.Errorf("the warning does not say the steps may be superseded: %q", out)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// TestWarnStaleEditSeedGating pins the seventh door BUG-3033's sweep found, and
|
||||
// the one whose consequence is not merely a confusing read.
|
||||
//
|
||||
// `pad item edit` is a read-modify-write over the WHOLE body: it seeds $EDITOR
|
||||
// from the row and PATCHes the entire edited text back with no concurrency
|
||||
// token. When the row is behind the live document, saving replaces edits that
|
||||
// exist and are durable with a version derived from a state before them. The
|
||||
// warning does not prevent that — BUG-3035 owns the refuse/prompt/diff decision —
|
||||
// so what this test guards is that the signal exists, fires only for the defined
|
||||
// value, and says the thing that distinguishes it from an ordinary stale read.
|
||||
func TestWarnStaleEditSeedGating(t *testing.T) {
|
||||
cases := []struct {
|
||||
name string
|
||||
item *models.Item
|
||||
want bool
|
||||
}{
|
||||
{"nil item", nil, false},
|
||||
{"a current row — the common case", &models.Item{Content: "body"}, false},
|
||||
{"some future value", &models.Item{ContentState: "some_future_value"}, false},
|
||||
{"the value the server sends", &models.Item{
|
||||
ContentState: models.ContentOutcomeAppliedPendingFlush,
|
||||
}, true},
|
||||
}
|
||||
for _, tc := range cases {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
out := captureStderr(t, func() { warnStaleEditSeed(tc.item) })
|
||||
if got := out != ""; got != tc.want {
|
||||
t.Fatalf("warned=%v want=%v (stderr: %q)", got, tc.want, out)
|
||||
}
|
||||
if !tc.want {
|
||||
return
|
||||
}
|
||||
// The consequence an EDITOR needs is that their save destroys
|
||||
// something, not that their text is old. A line that only said
|
||||
// "previous content" would read as a display quirk.
|
||||
lower := strings.ToLower(out)
|
||||
if !strings.Contains(lower, "saving") || !strings.Contains(lower, "replac") {
|
||||
t.Errorf("the warning does not say a save will replace the unseen edits: %q", out)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// TestEditWarnsBeforeLaunchingTheEditor is the BINDING half of the test above
|
||||
// (CONVE-19), and it guards the one claim the gating table cannot: the warning
|
||||
// is printed BEFORE $EDITOR opens.
|
||||
//
|
||||
// Ordering is the whole value here. Printed afterwards the line would be read,
|
||||
// at best, beside a "Updated TASK-5" confirmation — after the user has already
|
||||
// edited a stale body and saved it over edits they could not see. So the
|
||||
// assertion is on ORDER within one stream, not on presence: the fake editor
|
||||
// writes a sentinel to its own stderr, which OpenInEditor wires to os.Stderr
|
||||
// (read at call time, so captureStderr's swap catches both), and the warning
|
||||
// must appear before it.
|
||||
func TestEditWarnsBeforeLaunchingTheEditor(t *testing.T) {
|
||||
const sentinel = "EDITOR-RAN-HERE"
|
||||
|
||||
// Both directions at the COMMAND boundary, because the helper's own gating
|
||||
// table cannot see a wiring error: with the call site forced to warn
|
||||
// unconditionally, a stale-only test still passes and the marker becomes
|
||||
// noise on every edit. Codex round 1 P3.
|
||||
for _, tc := range []struct {
|
||||
name string
|
||||
contentState string
|
||||
wantWarning bool
|
||||
}{
|
||||
{"a current row — the common case", "", false},
|
||||
{"the value the server sends", models.ContentOutcomeAppliedPendingFlush, true},
|
||||
} {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
var patchedContent *string
|
||||
mux := http.NewServeMux()
|
||||
mux.HandleFunc("/api/v1/workspaces/ws/items/TASK-5", func(w http.ResponseWriter, r *http.Request) {
|
||||
if r.Method == http.MethodPatch {
|
||||
var body struct {
|
||||
Content *string `json:"content"`
|
||||
}
|
||||
_ = json.NewDecoder(r.Body).Decode(&body)
|
||||
patchedContent = body.Content
|
||||
_ = json.NewEncoder(w).Encode(map[string]any{
|
||||
"id": "i1", "ref": "TASK-5", "title": "T", "slug": "t", "content": "new",
|
||||
})
|
||||
return
|
||||
}
|
||||
body := map[string]any{
|
||||
"id": "i1", "ref": "TASK-5", "title": "T", "slug": "t",
|
||||
"content": "the previous content",
|
||||
}
|
||||
if tc.contentState != "" {
|
||||
body["content_state"] = tc.contentState
|
||||
}
|
||||
_ = json.NewEncoder(w).Encode(body)
|
||||
})
|
||||
srv := httptest.NewServer(mux)
|
||||
t.Cleanup(srv.Close)
|
||||
|
||||
setTempHomeMain(t)
|
||||
t.Setenv("PAD_URL", srv.URL)
|
||||
t.Setenv("PAD_TOKEN", "pad_testtoken")
|
||||
|
||||
// An "editor" that announces itself on stderr and edits the file, so
|
||||
// the command really reaches the write path rather than bailing on
|
||||
// "No changes."
|
||||
//
|
||||
// A script FILE rather than an inline `sh -c ...`: OpenInEditor splits
|
||||
// the EDITOR value on strings.Fields, which is whitespace-naive and
|
||||
// does not honour quoting, so an inline command is torn into words and
|
||||
// the editor exits 2. (That splitting is deliberate — it is what makes
|
||||
// `code --wait` work — and this test is not the place to change it.)
|
||||
editorPath := filepath.Join(t.TempDir(), "fake-editor")
|
||||
script := "#!/bin/sh\necho " + sentinel + " >&2\necho edited >> \"$1\"\n"
|
||||
if err := os.WriteFile(editorPath, []byte(script), 0o755); err != nil {
|
||||
t.Fatalf("write fake editor: %v", err)
|
||||
}
|
||||
t.Setenv("EDITOR", editorPath)
|
||||
|
||||
origWS, origFormat := workspaceFlag, formatFlag
|
||||
t.Cleanup(func() { workspaceFlag, formatFlag = origWS, origFormat })
|
||||
workspaceFlag, formatFlag = "ws", ""
|
||||
|
||||
cmd := editCmd()
|
||||
cmd.SetArgs([]string{"TASK-5"})
|
||||
stderr := captureStderr(t, func() {
|
||||
_ = captureStdout(t, func() {
|
||||
if err := cmd.Execute(); err != nil {
|
||||
t.Fatalf("item edit: %v", err)
|
||||
}
|
||||
})
|
||||
})
|
||||
|
||||
// PREMISES, asserted before any conclusion is drawn from them. An
|
||||
// absence assertion is vacuous if the command never got this far: the
|
||||
// editor must have run, and the edited body must have been sent.
|
||||
editorAt := strings.Index(stderr, sentinel)
|
||||
if editorAt < 0 {
|
||||
t.Fatalf("the fake editor never ran, so this case measured nothing:\n%s", stderr)
|
||||
}
|
||||
// The exact CONTENT, not merely that a PATCH occurred: an update
|
||||
// payload stripped of its content still produces a PATCH, and this
|
||||
// case claims to exercise saving the edited body (codex round 2 P3).
|
||||
wantContent := "the previous content" + "edited\n"
|
||||
if patchedContent == nil {
|
||||
t.Fatalf("the PATCH carried no content, so this case did not exercise saving:\n%s", stderr)
|
||||
}
|
||||
if *patchedContent != wantContent {
|
||||
t.Fatalf("the PATCH sent %q, want %q — the edited body is not what reached the server",
|
||||
*patchedContent, wantContent)
|
||||
}
|
||||
|
||||
warnAt := strings.Index(stderr, "behind its live collaborative")
|
||||
if got := warnAt >= 0; got != tc.wantWarning {
|
||||
t.Fatalf("warned=%v want=%v (stderr: %q)", got, tc.wantWarning, stderr)
|
||||
}
|
||||
if !tc.wantWarning {
|
||||
return
|
||||
}
|
||||
// Ordering is the whole value: printed after the editor opened, the
|
||||
// line would be read beside an "Updated TASK-5" confirmation — after
|
||||
// the user has already edited a stale body and saved it over edits
|
||||
// they could not see. The fake editor's stderr is wired to os.Stderr
|
||||
// by OpenInEditor (read at call time, so captureStderr's swap catches
|
||||
// both), which makes this an assertion about ORDER in one stream.
|
||||
if warnAt > editorAt {
|
||||
t.Errorf("the warning was printed AFTER the editor opened, where it cannot change anything:\n%s", stderr)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// TestPlaybookListWarnsOnlyAboutStaleSummaries covers the CLI half of the
|
||||
// summary door (BUG-3033, codex round 1 P2) through BOTH renderers.
|
||||
//
|
||||
// A `summary` is the body's first paragraph, truncated, so a stale body makes a
|
||||
// stale summary — and an agent routes on that description. Each renderer decodes
|
||||
// its own narrow struct, so neither inherits anything.
|
||||
//
|
||||
// Three stale entries INTERLEAVED with current ones, because a single stale row
|
||||
// cannot distinguish three different renderers (codex round 2 P3): one that
|
||||
// names every ref, one that names only the first, and the correct one all agree
|
||||
// when exactly one row is stale. The assertions are therefore the COMPLETE ref
|
||||
// set, the absence of the current ones, and EXACTLY ONE warning line.
|
||||
func TestPlaybookListWarnsOnlyAboutStaleSummaries(t *testing.T) {
|
||||
// PLAYB-2 and PLAYB-4 stale, PLAYB-1/3/5 current — stale entries neither
|
||||
// first nor last nor contiguous, so an off-by-one or a first-only renderer
|
||||
// cannot pass by accident.
|
||||
all := []string{"PLAYB-1", "PLAYB-2", "PLAYB-3", "PLAYB-4", "PLAYB-5"}
|
||||
stale := map[string]bool{"PLAYB-2": true, "PLAYB-4": true}
|
||||
|
||||
serve := func(t *testing.T, markStale bool) {
|
||||
t.Helper()
|
||||
payload := func() []map[string]any {
|
||||
out := make([]map[string]any, 0, len(all))
|
||||
for i, ref := range all {
|
||||
e := map[string]any{
|
||||
"ref": ref, "title": fmt.Sprintf("P%d", i+1), "slug": fmt.Sprintf("p%d", i+1),
|
||||
"invocation_slug": fmt.Sprintf("p%d", i+1), "status": "active",
|
||||
"summary": fmt.Sprintf("Does the P%d thing.", i+1),
|
||||
}
|
||||
if markStale && stale[ref] {
|
||||
e["content_state"] = models.ContentOutcomeAppliedPendingFlush
|
||||
}
|
||||
out = append(out, e)
|
||||
}
|
||||
return out
|
||||
}
|
||||
mux := http.NewServeMux()
|
||||
mux.HandleFunc("/api/v1/workspaces/ws/playbooks", func(w http.ResponseWriter, r *http.Request) {
|
||||
_ = json.NewEncoder(w).Encode(payload())
|
||||
})
|
||||
mux.HandleFunc("/api/v1/workspaces/ws/agent/bootstrap", func(w http.ResponseWriter, r *http.Request) {
|
||||
_ = json.NewEncoder(w).Encode(map[string]any{
|
||||
"workspace": map[string]any{"slug": "ws", "name": "WS"},
|
||||
"user": map[string]any{"name": "T", "email": "t@t"},
|
||||
"playbooks": payload(),
|
||||
})
|
||||
})
|
||||
srv := httptest.NewServer(mux)
|
||||
t.Cleanup(srv.Close)
|
||||
setTempHomeMain(t)
|
||||
t.Setenv("PAD_URL", srv.URL)
|
||||
t.Setenv("PAD_TOKEN", "pad_testtoken")
|
||||
}
|
||||
|
||||
origWS, origFormat := workspaceFlag, formatFlag
|
||||
t.Cleanup(func() { workspaceFlag, formatFlag = origWS, origFormat })
|
||||
workspaceFlag, formatFlag = "ws", "markdown"
|
||||
|
||||
// Both renderers, because the bootstrap one had no coverage at all and the
|
||||
// two are separate decode structs in separate files.
|
||||
renderers := map[string]func() *cobra.Command{
|
||||
"playbook list": playbookListCmd,
|
||||
"bootstrap": bootstrapCmd,
|
||||
}
|
||||
|
||||
run := func(t *testing.T, mk func() *cobra.Command) (stdout, stderr string) {
|
||||
t.Helper()
|
||||
cmd := mk()
|
||||
cmd.SetArgs(nil)
|
||||
var out string
|
||||
errOut := captureStderr(t, func() {
|
||||
out = captureStdout(t, func() {
|
||||
if e := cmd.Execute(); e != nil {
|
||||
t.Fatalf("execute: %v", e)
|
||||
}
|
||||
})
|
||||
})
|
||||
return out, errOut
|
||||
}
|
||||
|
||||
for name, mk := range renderers {
|
||||
t.Run(name, func(t *testing.T) {
|
||||
// ABSENCE FIRST, with the premise asserted so the silence below is
|
||||
// not the silence of a command that rendered nothing.
|
||||
serve(t, false)
|
||||
stdout, stderr := run(t, mk)
|
||||
for _, ref := range all {
|
||||
if !strings.Contains(stdout, ref) {
|
||||
t.Fatalf("the listing did not render %s, so the absence assertion is vacuous:\n%s", ref, stdout)
|
||||
}
|
||||
}
|
||||
if strings.Contains(stdout, "Does the P2 thing.") == false {
|
||||
t.Fatalf("the listing rendered no summaries, so this measures nothing:\n%s", stdout)
|
||||
}
|
||||
if strings.Contains(stderr, "behind its live collaborative") {
|
||||
t.Fatalf("a listing with no stale summaries warned:\n%s", stderr)
|
||||
}
|
||||
|
||||
serve(t, true)
|
||||
stdout, stderr = run(t, mk)
|
||||
|
||||
// The summaries must STILL BE RENDERED. Presence was asserted only
|
||||
// in the absence leg, so suppressing stale summary text in both
|
||||
// renderers while keeping the warning refs left this green (codex
|
||||
// round 4). The warning qualifies the text; it is not a substitute.
|
||||
for ref := range stale {
|
||||
if !strings.Contains(stdout, ref) {
|
||||
t.Errorf("stale %s vanished from the listing; the warning is a qualifier, not a replacement:\n%s", ref, stdout)
|
||||
}
|
||||
}
|
||||
if !strings.Contains(stdout, "Does the P2 thing.") {
|
||||
t.Errorf("the stale entry's summary text is gone from the listing:\n%s", stdout)
|
||||
}
|
||||
|
||||
// EXACTLY ONE warning line. A renderer emitting one per stale row
|
||||
// satisfies every "contains" assertion and is the thing that makes a
|
||||
// long listing unreadable.
|
||||
lines := 0
|
||||
for _, ln := range strings.Split(stderr, "\n") {
|
||||
if strings.Contains(ln, "behind its live collaborative") {
|
||||
lines++
|
||||
}
|
||||
}
|
||||
if lines != 1 {
|
||||
t.Errorf("got %d warning lines, want exactly 1:\n%s", lines, stderr)
|
||||
}
|
||||
// The COMPLETE stale set — a renderer naming only the first would
|
||||
// pass a single-ref assertion.
|
||||
for ref := range stale {
|
||||
if !strings.Contains(stderr, ref) {
|
||||
t.Errorf("the warning omits stale %s:\n%s", ref, stderr)
|
||||
}
|
||||
}
|
||||
// And only those.
|
||||
for _, ref := range all {
|
||||
if stale[ref] {
|
||||
continue
|
||||
}
|
||||
if strings.Contains(stderr, ref) {
|
||||
t.Errorf("the warning names %s, whose summary is current:\n%s", ref, stderr)
|
||||
}
|
||||
}
|
||||
// STDOUT stays clean: the listing is piped.
|
||||
if strings.Contains(stdout, "behind its live collaborative") {
|
||||
t.Errorf("the warning landed on STDOUT, corrupting the listing:\n%s", stdout)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// TestSnippetRenderersWarnAboutStaleSources covers the two CLI renderers that
|
||||
// print body-derived SNIPPETS (BUG-3033, codex round 2 P2).
|
||||
//
|
||||
// `pad item search` decodes the embedded models.Item and prints only the
|
||||
// snippet; `pad item backlinks` decodes models.Backlink and prints only the
|
||||
// snippet. Both carry the marker on the wire and both dropped it here, so the
|
||||
// signal stopped at the renderer — the same shape as the playbook commands.
|
||||
//
|
||||
// Interleaved stale and current rows for the reason the summary test uses them:
|
||||
// a single stale row cannot tell a correct renderer from one that names every
|
||||
// ref or only the first.
|
||||
func TestSnippetRenderersWarnAboutStaleSources(t *testing.T) {
|
||||
// THREE stale rows separated by current ones. An earlier draft had one
|
||||
// stale row while its comment claimed interleaving — the same gap this unit
|
||||
// had already fixed for summaries, reintroduced one test later, and a
|
||||
// renderer reporting only the first stale source passed it (codex round 3).
|
||||
staleRefs := []string{"TASK-2", "TASK-4"}
|
||||
currentRefs := []string{"TASK-1", "TASK-3", "TASK-5"}
|
||||
isStale := map[string]bool{}
|
||||
for _, r := range staleRefs {
|
||||
isStale[r] = true
|
||||
}
|
||||
allRefs := []string{"TASK-1", "TASK-2", "TASK-3", "TASK-4", "TASK-5"}
|
||||
|
||||
serve := func(t *testing.T, markStale bool) {
|
||||
t.Helper()
|
||||
item := func(ref, title string) map[string]any {
|
||||
m := map[string]any{
|
||||
"id": ref, "ref": ref, "title": title, "slug": strings.ToLower(ref),
|
||||
"collection_name": "Tasks", "collection_icon": "✓", "content": "body",
|
||||
}
|
||||
if markStale && isStale[ref] {
|
||||
m["content_state"] = models.ContentOutcomeAppliedPendingFlush
|
||||
}
|
||||
return m
|
||||
}
|
||||
backlink := func(ref, title string) map[string]any {
|
||||
m := map[string]any{
|
||||
"source_item_id": ref, "source_ref": ref, "source_title": title,
|
||||
"source_collection_slug": "tasks", "snippet": "…a snippet from " + ref + "…",
|
||||
}
|
||||
if markStale && isStale[ref] {
|
||||
m["content_state"] = models.ContentOutcomeAppliedPendingFlush
|
||||
}
|
||||
return m
|
||||
}
|
||||
mux := http.NewServeMux()
|
||||
mux.HandleFunc("/api/v1/search", func(w http.ResponseWriter, r *http.Request) {
|
||||
results := make([]map[string]any, 0, len(allRefs))
|
||||
for _, ref := range allRefs {
|
||||
results = append(results, map[string]any{
|
||||
"item": item(ref, ref),
|
||||
"snippet": "…a snippet from " + ref + "…",
|
||||
})
|
||||
}
|
||||
_ = json.NewEncoder(w).Encode(map[string]any{
|
||||
"results": results, "total": len(results), "limit": 20, "offset": 0,
|
||||
})
|
||||
})
|
||||
mux.HandleFunc("/api/v1/workspaces/ws/items/TASK-9", func(w http.ResponseWriter, r *http.Request) {
|
||||
_ = json.NewEncoder(w).Encode(item("TASK-9", "Target"))
|
||||
})
|
||||
// The CLI resolves the ref to an item first and builds the backlinks URL
|
||||
// from its SLUG, so the fixture has to serve the slug path.
|
||||
mux.HandleFunc("/api/v1/workspaces/ws/items/task-9/backlinks", func(w http.ResponseWriter, r *http.Request) {
|
||||
bls := make([]map[string]any, 0, len(allRefs))
|
||||
for _, ref := range allRefs {
|
||||
bls = append(bls, backlink(ref, ref))
|
||||
}
|
||||
_ = json.NewEncoder(w).Encode(bls)
|
||||
})
|
||||
srv := httptest.NewServer(mux)
|
||||
t.Cleanup(srv.Close)
|
||||
setTempHomeMain(t)
|
||||
t.Setenv("PAD_URL", srv.URL)
|
||||
t.Setenv("PAD_TOKEN", "pad_testtoken")
|
||||
}
|
||||
|
||||
origWS, origFormat := workspaceFlag, formatFlag
|
||||
t.Cleanup(func() { workspaceFlag, formatFlag = origWS, origFormat })
|
||||
workspaceFlag, formatFlag = "ws", ""
|
||||
|
||||
// Each door names the tokens whose presence proves the listing really
|
||||
// rendered, so an absence assertion cannot pass vacuously — and for both
|
||||
// doors those tokens are SNIPPET text, because the mutation that matters is
|
||||
// "suppress the snippets, keep the warning" (codex round 4 ran exactly that
|
||||
// and an earlier draft of this test stayed green).
|
||||
//
|
||||
// `item backlinks` prints its snippet through cli.Dim, and fatih/color's
|
||||
// Printf writes to the package-level color.Output rather than os.Stdout, so
|
||||
// a plain os.Stdout swap does not intercept it. An earlier draft recorded
|
||||
// that as a limit and asserted row headings instead — which is precisely
|
||||
// what let the suppress-snippets mutation live. The run helper below points
|
||||
// color.Output at the swapped os.Stdout for the duration instead, so the
|
||||
// dimmed text is captured and the premise is about the thing under test.
|
||||
doors := map[string]struct {
|
||||
mk func() (*cobra.Command, []string)
|
||||
present []string
|
||||
}{
|
||||
"item search": {
|
||||
mk: func() (*cobra.Command, []string) { return searchCmd(), []string{"snippet"} },
|
||||
present: []string{"a snippet from TASK-1", "a snippet from TASK-4"},
|
||||
},
|
||||
"item backlinks": {
|
||||
mk: func() (*cobra.Command, []string) { return backlinksCmd(), []string{"TASK-9"} },
|
||||
present: []string{"a snippet from TASK-1", "a snippet from TASK-4"},
|
||||
},
|
||||
}
|
||||
|
||||
for name, door := range doors {
|
||||
t.Run(name, func(t *testing.T) {
|
||||
run := func(t *testing.T) (stdout, stderr string) {
|
||||
t.Helper()
|
||||
cmd, args := door.mk()
|
||||
cmd.SetArgs(args)
|
||||
var out string
|
||||
errOut := captureStderr(t, func() {
|
||||
out = captureStdout(t, func() {
|
||||
// color.Output is read at Printf time, so pointing it at
|
||||
// the already-swapped os.Stdout captures cli.Dim output
|
||||
// too. Restored immediately after.
|
||||
origColorOut := color.Output
|
||||
color.Output = os.Stdout
|
||||
defer func() { color.Output = origColorOut }()
|
||||
if e := cmd.Execute(); e != nil {
|
||||
t.Fatalf("%s: %v", name, e)
|
||||
}
|
||||
})
|
||||
})
|
||||
return out, errOut
|
||||
}
|
||||
|
||||
// ABSENCE FIRST, premise asserted: both snippets really rendered.
|
||||
serve(t, false)
|
||||
stdout, stderr := run(t)
|
||||
for _, token := range door.present {
|
||||
if !strings.Contains(stdout, token) {
|
||||
t.Fatalf("the listing did not render %q, so the absence assertion is vacuous:\n%s", token, stdout)
|
||||
}
|
||||
}
|
||||
if strings.Contains(stderr, "behind its live collaborative") {
|
||||
t.Fatalf("a listing with no stale sources warned:\n%s", stderr)
|
||||
}
|
||||
|
||||
serve(t, true)
|
||||
stdout, stderr = run(t)
|
||||
|
||||
// The rendered text must SURVIVE the stale leg. Suppressing stale
|
||||
// search snippets and ALL backlink snippets while keeping the
|
||||
// warnings left this green (codex round 4), because the backlink
|
||||
// premise checked row headings rather than snippets.
|
||||
for _, token := range door.present {
|
||||
if !strings.Contains(stdout, token) {
|
||||
t.Errorf("%q vanished from the stale listing; the warning qualifies the text, it does not replace it:\n%s", token, stdout)
|
||||
}
|
||||
}
|
||||
|
||||
lines := 0
|
||||
for _, ln := range strings.Split(stderr, "\n") {
|
||||
if strings.Contains(ln, "behind its live collaborative") {
|
||||
lines++
|
||||
}
|
||||
}
|
||||
if lines != 1 {
|
||||
t.Errorf("got %d warning lines, want exactly 1:\n%s", lines, stderr)
|
||||
}
|
||||
// The COMPLETE stale set — a renderer naming only the first passes
|
||||
// any single-ref assertion.
|
||||
for _, ref := range staleRefs {
|
||||
if !strings.Contains(stderr, ref) {
|
||||
t.Errorf("the warning omits stale source %s:\n%s", ref, stderr)
|
||||
}
|
||||
}
|
||||
for _, ref := range currentRefs {
|
||||
if strings.Contains(stderr, ref) {
|
||||
t.Errorf("the warning names %s, whose body is current:\n%s", ref, stderr)
|
||||
}
|
||||
}
|
||||
if strings.Contains(stdout, "behind its live collaborative") {
|
||||
t.Errorf("the warning landed on STDOUT, corrupting a piped listing:\n%s", stdout)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
@@ -39,6 +39,36 @@ type Provenance struct {
|
||||
ExportedAt string `yaml:"exported_at"`
|
||||
Author string `yaml:"author"`
|
||||
FormatVersion int `yaml:"format_version"`
|
||||
// ContentState records that the BODY of this artifact was one the server
|
||||
// knew to be BEHIND the item's live collaborative document at export time
|
||||
// (BUG-3033 / BUG-3000). Same values as models.Item.ContentState, named the
|
||||
// same on purpose: a reader who learned the word from an API response should
|
||||
// not need a second lesson to read it here.
|
||||
//
|
||||
// It belongs in provenance rather than beside `title`, because it is a fact
|
||||
// about THIS EXPORT and not about the item — re-export the same item after a
|
||||
// flush and the key is gone, while nothing about the playbook or convention
|
||||
// itself changed.
|
||||
//
|
||||
// Additive and omitempty, with NO format_version bump, and the two lines
|
||||
// that make that safe are worth naming since a future key will face the same
|
||||
// question. Decode uses a plain yaml.Unmarshal with no KnownFields, so an
|
||||
// older binary reading a newer artifact IGNORES this key rather than
|
||||
// failing; and decode.go compares format_version with EXACT equality against
|
||||
// the constant, so bumping it would reject artifacts in BOTH directions for
|
||||
// what is only an added optional key. A current item's bytes are therefore
|
||||
// byte-identical to what this package emitted before — and that claim has an
|
||||
// instrument older than this change: testdata/*.golden.md pin the encoded
|
||||
// bytes exactly, and they were not touched to land this key.
|
||||
//
|
||||
// What it does NOT do: stop the import. A stale body imported into another
|
||||
// workspace becomes canonical there, in a workspace whose op-log never held
|
||||
// the real content and so can never catch up — that is BUG-3032's open
|
||||
// decision (signal / refuse / accept) for the bundle format. This key is the
|
||||
// cheapest of those three on this format, and forecloses neither of the
|
||||
// others: if that unit rules refuse-or-warn for portable formats, this door
|
||||
// joins the ruling and this marker is what makes the check possible.
|
||||
ContentState string `yaml:"content_state,omitempty"`
|
||||
}
|
||||
|
||||
// Artifact is a decoded export of one playbook or convention item.
|
||||
|
||||
File diff suppressed because one or more lines are too long
@@ -13,6 +13,8 @@ import (
|
||||
|
||||
"github.com/mark3labs/mcp-go/mcp"
|
||||
"github.com/mark3labs/mcp-go/server"
|
||||
|
||||
"github.com/PerpetualSoftware/pad/internal/models"
|
||||
)
|
||||
|
||||
// MIME types reported in the read response. Stable across versions —
|
||||
@@ -342,7 +344,15 @@ func (r *resources) readItem(ctx context.Context, req mcp.ReadResourceRequest) (
|
||||
// formatItemAsMarkdown turns the JSON body returned by
|
||||
// `pad item show --format json` into a self-contained markdown
|
||||
// document: heading with ref + title, sorted metadata fields,
|
||||
// optional parent link, then the item's body content.
|
||||
// optional parent link, a staleness marker when the server reports one,
|
||||
// then the item's body content.
|
||||
//
|
||||
// The marker (BUG-3033) appears ONLY for content_state's one defined
|
||||
// value, and only immediately above the body it qualifies. An
|
||||
// unrecognised value renders nothing: the vocabulary is shared with the
|
||||
// write side's content_outcome and is expected to grow, and rendering
|
||||
// an unknown value as this specific claim about a live editor would be
|
||||
// worse than staying silent.
|
||||
//
|
||||
// Map-key iteration is sorted so the output is stable for tests.
|
||||
func formatItemAsMarkdown(jsonBlob string) (string, error) {
|
||||
@@ -391,6 +401,27 @@ func formatItemAsMarkdown(jsonBlob string) (string, error) {
|
||||
}
|
||||
}
|
||||
|
||||
// Staleness marker (BUG-3033). `item show --format json` carries
|
||||
// content_state since BUG-3000, and this formatter composes its own
|
||||
// markdown, so without this the field is fetched and then dropped on
|
||||
// the way out — the resource hands an agent a stale body with the one
|
||||
// signal that says so removed. The CLI's stderr warning does not cover
|
||||
// it either: ExecResourceFetcher reads stdout and does not surface
|
||||
// stderr from a command that SUCCEEDED.
|
||||
//
|
||||
// Placed here, immediately before the body, rather than with the
|
||||
// metadata above: it qualifies the body specifically, and an agent
|
||||
// reading top-down meets it in the sentence before the text it is
|
||||
// about. The literal state token is included so the line is greppable
|
||||
// by the same name the JSON field uses, not only readable as prose.
|
||||
if state, _ := item["content_state"].(string); state == models.ContentOutcomeAppliedPendingFlush {
|
||||
fmt.Fprintf(&b, "> **Stale body** (`content_state: %s`) — this item's stored content is "+
|
||||
"behind its live collaborative document. An editor holds edits that have not been "+
|
||||
"written back, so the body below is the previous content. It catches up when a tab "+
|
||||
"next flushes the item, and nothing on the server forces that to happen.\n\n",
|
||||
models.ContentOutcomeAppliedPendingFlush)
|
||||
}
|
||||
|
||||
// Body. Pad items often have rich markdown here already; pass
|
||||
// through verbatim.
|
||||
if content, _ := item["content"].(string); content != "" {
|
||||
|
||||
@@ -0,0 +1,108 @@
|
||||
package mcp
|
||||
|
||||
import (
|
||||
"context"
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
"github.com/mark3labs/mcp-go/mcp"
|
||||
|
||||
"github.com/PerpetualSoftware/pad/internal/models"
|
||||
)
|
||||
|
||||
// BUG-3033 — the MCP item resource.
|
||||
//
|
||||
// This door is the subtlest of the family: the marker is FETCHED and then
|
||||
// dropped. `item show --format json` has carried content_state since BUG-3000,
|
||||
// and readItem re-renders that JSON through formatItemAsMarkdown, which composes
|
||||
// its own document — so the field arrives and is discarded on the way out. The
|
||||
// CLI's stderr warning does not rescue it either: ExecResourceFetcher reads
|
||||
// stdout and never surfaces stderr from a command that SUCCEEDED.
|
||||
//
|
||||
// Driven through readItem rather than by calling the formatter directly (CONVE-19):
|
||||
// a direct-call test would vouch for the formatter while leaving the resource
|
||||
// free to stop calling it. The absence leg runs first for the reason it does
|
||||
// everywhere in this unit.
|
||||
func TestReadItemCarriesTheStaleBodyMarkerBothWays(t *testing.T) {
|
||||
read := func(t *testing.T, itemJSON string) string {
|
||||
t.Helper()
|
||||
r := &resources{fetcher: &fakeFetcher{stdout: itemJSON}}
|
||||
req := mcp.ReadResourceRequest{}
|
||||
req.Params.URI = "pad://workspace/docapp/items/TASK-5"
|
||||
contents, err := r.readItem(context.Background(), req)
|
||||
if err != nil {
|
||||
t.Fatalf("readItem: %v", err)
|
||||
}
|
||||
tc, ok := contents[0].(mcp.TextResourceContents)
|
||||
if !ok {
|
||||
t.Fatalf("expected TextResourceContents, got %T", contents[0])
|
||||
}
|
||||
return tc.Text
|
||||
}
|
||||
|
||||
const current = `{
|
||||
"ref": "TASK-5",
|
||||
"title": "Fix OAuth",
|
||||
"fields": "{\"priority\":\"high\"}",
|
||||
"content": "Detailed plan goes here."
|
||||
}`
|
||||
const stale = `{
|
||||
"ref": "TASK-5",
|
||||
"title": "Fix OAuth",
|
||||
"fields": "{\"priority\":\"high\"}",
|
||||
"content": "Detailed plan goes here.",
|
||||
"content_state": "applied_pending_flush"
|
||||
}`
|
||||
|
||||
// ABSENCE FIRST. A resource that always warned would satisfy the leg below
|
||||
// and be useless — every body would read as suspect.
|
||||
if got := read(t, current); strings.Contains(got, "content_state") || strings.Contains(got, "Stale body") {
|
||||
t.Fatalf("a current item's resource warns about staleness:\n%s", got)
|
||||
}
|
||||
|
||||
got := read(t, stale)
|
||||
if !strings.Contains(got, "Stale body") {
|
||||
t.Errorf("a stale item's resource carries no warning:\n%s", got)
|
||||
}
|
||||
// The literal state token, so the marker is greppable by the same name the
|
||||
// JSON field uses rather than only readable as prose.
|
||||
if !strings.Contains(got, models.ContentOutcomeAppliedPendingFlush) {
|
||||
t.Errorf("the warning omits the %q token:\n%s", models.ContentOutcomeAppliedPendingFlush, got)
|
||||
}
|
||||
// It must precede the body it qualifies — an agent reading top-down has to
|
||||
// meet the caveat before the text it is about, not after.
|
||||
markerAt := strings.Index(got, "Stale body")
|
||||
bodyAt := strings.Index(got, "Detailed plan goes here.")
|
||||
if bodyAt < 0 {
|
||||
t.Fatal("the body is missing from the marked resource; the marker is a qualifier, not a substitute")
|
||||
}
|
||||
if markerAt > bodyAt {
|
||||
t.Errorf("the marker follows the body it qualifies:\n%s", got)
|
||||
}
|
||||
}
|
||||
|
||||
// TestFormatItemAsMarkdownIgnoresAnUnknownContentState pins the other direction
|
||||
// of the same key: only the ONE value this vocabulary defines produces a
|
||||
// warning.
|
||||
//
|
||||
// Not a hypothetical. content_state shares its value vocabulary with BUG-2995's
|
||||
// write-side content_outcome deliberately, so that vocabulary is expected to
|
||||
// grow — and a formatter that warned on any non-empty string would turn a future
|
||||
// value it has never heard of into this specific claim about a live editor.
|
||||
func TestFormatItemAsMarkdownIgnoresAnUnknownContentState(t *testing.T) {
|
||||
got, err := formatItemAsMarkdown(`{
|
||||
"ref": "TASK-5",
|
||||
"title": "Fix OAuth",
|
||||
"content": "Body.",
|
||||
"content_state": "some_future_value"
|
||||
}`)
|
||||
if err != nil {
|
||||
t.Fatalf("formatItemAsMarkdown: %v", err)
|
||||
}
|
||||
if strings.Contains(got, "Stale body") {
|
||||
t.Errorf("an unrecognised content_state was rendered as this unit's specific claim:\n%s", got)
|
||||
}
|
||||
if !strings.Contains(got, "Body.") {
|
||||
t.Errorf("the body was lost:\n%s", got)
|
||||
}
|
||||
}
|
||||
@@ -1081,6 +1081,48 @@ const CmdhelpVersion = "0.1"
|
||||
// partially, and reports it once through the same
|
||||
// `warnings.dropped_fields` channel BUG-2674 established — so an
|
||||
// import can never change an element count.
|
||||
//
|
||||
// - POST-0.33, NO BUMP — BUG-3000 / BUG-3033, the READ half of the
|
||||
// BUG-2995 entry above.
|
||||
//
|
||||
// Authored against 0.31 and rebased over U4's 0.33 bump, which is
|
||||
// why it is filed here rather than beside its sibling: it needed no
|
||||
// bump then and needs none now, and nothing in 0.33 touches it. The
|
||||
// two changes are independent — `multi_relation` is a field TYPE,
|
||||
// this is a row PROPERTY on every item.
|
||||
//
|
||||
// A read that serves a body the server knows is
|
||||
// behind the item's live collaborative document now says so, via
|
||||
// an additive omitempty `content_state` carrying the SAME value
|
||||
// vocabulary as `warnings.content_outcome`. A distinct name
|
||||
// because a write warning describes what a REQUEST did and this
|
||||
// describes what the ROW is; the shared vocabulary so an agent
|
||||
// that learned the word from a write response recognises it on a
|
||||
// read without a second lesson.
|
||||
//
|
||||
// BUG-3000 put it on models.Item, so every MCP surface that
|
||||
// serialises the struct — `pad_item` get, `pad_item` list with
|
||||
// full=true, the summary projection's preview — inherited it and
|
||||
// needed no entry here. BUG-3033 is why there is one: the doors
|
||||
// that build their OWN shape inherited nothing, and two of them
|
||||
// are on this surface. `pad_playbook` run responses gain the
|
||||
// field, and the `pad://workspace/{ws}/items/{ref}` RESOURCE gains
|
||||
// a line of markdown above the body, because a text resource has
|
||||
// no additive key to carry.
|
||||
//
|
||||
// The resource line is the one an integrator should read twice: a
|
||||
// consumer diffing that markdown byte-for-byte sees a new block
|
||||
// when — and only when — the body it is about is stale. That is a
|
||||
// changed VALUE on one path, the BUG-2304 disposition the entry
|
||||
// above argues at length, not 0.9's field removal. Nothing is
|
||||
// dropped or retyped, and a current item renders exactly as before.
|
||||
//
|
||||
// What a consumer should do with it is the same on both halves:
|
||||
// re-read later. The marker means the op-log is ahead of the row,
|
||||
// not that a write failed, and nothing server-side ever moves that
|
||||
// content into the row — so no surface here states a duration for
|
||||
// this one either, and it is not a promise that the row will catch
|
||||
// up at all.
|
||||
const ToolSurfaceVersion = "0.33"
|
||||
|
||||
// MetaVersionURI is the canonical URI of the queryable version document.
|
||||
|
||||
@@ -35,6 +35,22 @@ type Backlink struct {
|
||||
// spaces. Empty when the source item has no body.
|
||||
Snippet string `json:"snippet"`
|
||||
|
||||
// ContentState marks `Snippet` above as taken from a body the server knows
|
||||
// is BEHIND the source item's live collaborative document (BUG-3033). Same
|
||||
// values as models.Item.ContentState, omitted when the source row is
|
||||
// current.
|
||||
//
|
||||
// A snippet is a window onto the body, not metadata about it, so a stale
|
||||
// body makes a stale snippet — the same reasoning that put the marker on
|
||||
// cli.ItemSummary.ContentPreview and on a playbook's summary. It is set
|
||||
// only when a snippet was actually produced: an empty body yields no
|
||||
// snippet and therefore no claim about one.
|
||||
//
|
||||
// It describes the SOURCE item of the link, never the target the caller
|
||||
// asked about. A backlink list can mix marked and unmarked rows for that
|
||||
// reason, and each row's marker is about the row.
|
||||
ContentState string `json:"content_state,omitempty"`
|
||||
|
||||
// DisplayText is the [[X|Display]] override the link author
|
||||
// supplied, nil when the link was a bare `[[X]]` (no pipe).
|
||||
// A non-nil pointer to "" represents the editor-distinct case
|
||||
|
||||
@@ -0,0 +1,304 @@
|
||||
package server
|
||||
|
||||
import (
|
||||
"fmt"
|
||||
"go/ast"
|
||||
"go/parser"
|
||||
"go/token"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"sort"
|
||||
"strings"
|
||||
"testing"
|
||||
)
|
||||
|
||||
// BUG-3033, CONVE-35 — the ENUMERATION guard.
|
||||
//
|
||||
// Three review rounds of this unit each found another door of the same class,
|
||||
// and one had its precedent already in the tree. A loop that keeps finding new
|
||||
// members is measuring the enumeration, not the code, so this test IS the
|
||||
// table: every site that turns an item body into text under another name is
|
||||
// listed with why it is safe, and anything else fails.
|
||||
//
|
||||
// It parses Go rather than grepping. The grep draft passed four controls and
|
||||
// codex round 3 then walked through it in four ways — a one-line function, a
|
||||
// helper assigned to a local and called through it, a comment inside the call
|
||||
// (`PlaybookSummary /* x */ (body)`), and an argument changed below the opening
|
||||
// line of a multiline call — while failing on harmless prose that merely
|
||||
// mentioned a helper. Both directions wrong is the signature of a scanner that
|
||||
// is not reading the language it claims to be reading.
|
||||
//
|
||||
// What it sees and does not, stated because a guard whose boundary is unstated
|
||||
// gets trusted past it:
|
||||
//
|
||||
// - CALL EXPRESSIONS whose callee names one of the derivation helpers, by
|
||||
// bare identifier or through a package selector, in a non-test .go file
|
||||
// under internal/ or cmd/ — EXCEPT under node_modules, vendor, .git and
|
||||
// testdata, which are named here because an earlier version of this comment
|
||||
// said "anywhere" while the code skipped directories it never disclosed.
|
||||
// Position, comments and line breaks inside the call are irrelevant to the
|
||||
// AST, which is the point.
|
||||
//
|
||||
// - A helper NAME used as a value rather than called — assigned to a
|
||||
// variable, passed as an argument — is reported too, and fails closed: the
|
||||
// call through that alias is invisible, so the reference itself is the last
|
||||
// honest place to stop. That claim was FALSE until codex round 4: a
|
||||
// declaration whose own name was a helper had a special traversal that
|
||||
// skipped this check entirely.
|
||||
//
|
||||
// - It does NOT prove a listed site carries the marker. That is each door's
|
||||
// own both-directions test. This answers a different question: has a site
|
||||
// appeared that nobody has ruled on.
|
||||
//
|
||||
// - It cannot see a door built on a helper it does not know, one that inlines
|
||||
// the derivation without any helper at all, or a DTO that simply copies
|
||||
// Content into a field under another name. The helper list is the grammar
|
||||
// it reads; the failure message therefore asks for the helper too.
|
||||
//
|
||||
// This is a real and load-bearing limit, not a formality. Codex round 4
|
||||
// censused 434 non-test Go files and found the marker's population is
|
||||
// WIDER than these four helpers: the workspace export's ItemExport.Content
|
||||
// (BUG-3032) and the cross-workspace copy's body remap are field copies
|
||||
// this guard cannot see by construction. Counts also cannot distinguish a
|
||||
// count-preserving move of a call to an unreviewed caller.
|
||||
//
|
||||
// So this test's honest claim is narrow: no UNRULED USE of these four
|
||||
// helpers. It is not a proof that every door carries the marker, and it
|
||||
// must not be cited as one.
|
||||
//
|
||||
// - It scans GO only. The browser surface is BUG-3050, and that item's scope
|
||||
// is wider than "choose a badge": codex round 4 found the share-view
|
||||
// projection and the individual-share page performing actual FIELD COPIES
|
||||
// that DISCARD the marker before any rendering decision arises, alongside
|
||||
// quick-action prompts, the conventions and playbook editors, and
|
||||
// ItemDetail's editor-seed and timeline-diff strings. An earlier version of
|
||||
// this comment described that surface as a pure rendering choice, which was
|
||||
// inaccurate.
|
||||
func TestBodyDerivedTextSitesAreAllRuledOn(t *testing.T) {
|
||||
// The helpers whose output is item-body text under another name. Adding one
|
||||
// here is how this guard learns about a new shape.
|
||||
helpers := map[string]bool{
|
||||
"snippetAround": true,
|
||||
"PlaybookSummary": true,
|
||||
"contentPreview": true,
|
||||
"FTSSnippet": true,
|
||||
}
|
||||
|
||||
type ruling struct {
|
||||
// sites are "file:line-ish" free of line numbers on purpose: a key that
|
||||
// moved with an unrelated edit above it would be re-ruled every time.
|
||||
// The COUNT is the load-bearing half — an added call in an already
|
||||
// listed file is the hole the grep draft had.
|
||||
count int
|
||||
reason string
|
||||
}
|
||||
allowed := map[string]ruling{
|
||||
"internal/store/wiki_links.go:snippetAround": {
|
||||
count: 2,
|
||||
reason: "backlink snippets (both query sites) — models.Backlink.ContentState carries the marker, set only when a snippet was produced",
|
||||
},
|
||||
"internal/cli/item_summary.go:contentPreview": {
|
||||
count: 1,
|
||||
reason: "list previews — cli.ItemSummary.ContentState carries the marker (BUG-3000)",
|
||||
},
|
||||
"internal/server/handlers_bootstrap.go:PlaybookSummary": {
|
||||
count: 1,
|
||||
reason: "playbook summaries — AgentBootstrapPlaybookMeta.ContentState carries the marker, set only when a summary was produced",
|
||||
},
|
||||
"internal/server/handlers_playbook_library.go:PlaybookSummary": {
|
||||
count: 1,
|
||||
reason: "LIBRARY playbooks are static Go-defined entries, not item rows: no op-log can be ahead of them, so there is nothing to mark",
|
||||
},
|
||||
"internal/store/search.go:FTSSnippet": {
|
||||
count: 2,
|
||||
reason: "FTS search snippets — the body is dropped from the result but the snippet is cut from it, so models.Item.ContentState survives on that path deliberately (and is cleared on the title-only direct-ref paths beside it)",
|
||||
},
|
||||
"internal/store/items.go:FTSSnippet": {
|
||||
count: 2,
|
||||
reason: "item-list FTS snippets — same path and same reasoning as search.go's, over the shared item queries that already splice contentStateSQL",
|
||||
},
|
||||
}
|
||||
|
||||
root := repoRoot(t)
|
||||
found := map[string]int{}
|
||||
where := map[string][]string{}
|
||||
visited := map[string]int{}
|
||||
|
||||
for _, dir := range []string{"internal", "cmd"} {
|
||||
base := filepath.Join(root, dir)
|
||||
err := filepath.Walk(base, func(path string, info os.FileInfo, err error) error {
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
if info.IsDir() {
|
||||
// Skipped, and named in the doc comment above rather than left
|
||||
// silent: vendored/third-party trees we do not own, and
|
||||
// `testdata`, which the go tool itself excludes from builds so
|
||||
// nothing under it is production code. `build` was skipped too
|
||||
// until codex round 4 reproduced a green with a real file at
|
||||
// internal/server/build/ — a package may legitimately be named
|
||||
// that, and the exclusion was inherited from a web/build path
|
||||
// that is not under these roots at all.
|
||||
if b := info.Name(); b == "node_modules" || b == "vendor" || b == ".git" || b == "testdata" {
|
||||
return filepath.SkipDir
|
||||
}
|
||||
return nil
|
||||
}
|
||||
if !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") {
|
||||
return nil
|
||||
}
|
||||
visited[dir]++
|
||||
|
||||
fset := token.NewFileSet()
|
||||
file, err := parser.ParseFile(fset, path, nil, 0)
|
||||
if err != nil {
|
||||
// A file this guard cannot parse is a file it cannot rule on.
|
||||
return fmt.Errorf("parse %s: %w", path, err)
|
||||
}
|
||||
rel, err := filepath.Rel(root, path)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
rel = filepath.ToSlash(rel)
|
||||
|
||||
// nameOf reports the helper a callee or value expression names.
|
||||
nameOf := func(e ast.Expr) string {
|
||||
switch v := e.(type) {
|
||||
case *ast.Ident:
|
||||
if helpers[v.Name] {
|
||||
return v.Name
|
||||
}
|
||||
case *ast.SelectorExpr:
|
||||
if helpers[v.Sel.Name] {
|
||||
return v.Sel.Name
|
||||
}
|
||||
}
|
||||
return ""
|
||||
}
|
||||
|
||||
// Positions that are NOT a value-use of a helper: the callee of a
|
||||
// call, and any DECLARATION of the name (a func, or an interface
|
||||
// method / struct field spec). Collected first so the value-use arm
|
||||
// below cannot fire on a call's own callee — which it did on its
|
||||
// first run, reporting every legitimate call twice.
|
||||
notAValueUse := map[token.Pos]bool{}
|
||||
ast.Inspect(file, func(n ast.Node) bool {
|
||||
switch v := n.(type) {
|
||||
case *ast.CallExpr:
|
||||
switch fn := v.Fun.(type) {
|
||||
case *ast.Ident:
|
||||
notAValueUse[fn.Pos()] = true
|
||||
case *ast.SelectorExpr:
|
||||
notAValueUse[fn.Sel.Pos()] = true
|
||||
}
|
||||
case *ast.FuncDecl:
|
||||
notAValueUse[v.Name.Pos()] = true
|
||||
case *ast.Field:
|
||||
for _, nm := range v.Names {
|
||||
notAValueUse[nm.Pos()] = true
|
||||
}
|
||||
}
|
||||
return true
|
||||
})
|
||||
|
||||
// No special case for a declaration whose NAME is a helper. An
|
||||
// earlier draft had one — it walked such a body for CallExprs and
|
||||
// then returned false — and that shortcut was a bypass: the
|
||||
// value-use arm below never ran inside it, so a compiling, callable
|
||||
// `func (p ReviewProjection) PlaybookSummary() string { render :=
|
||||
// contentPreview; return render(p.Content) }` served unmarked stale
|
||||
// prose while this guard passed. Renaming the method made it fail,
|
||||
// which is the tell that the guard was keying on the wrong thing.
|
||||
//
|
||||
// It was also redundant: notAValueUse already marks every
|
||||
// FuncDecl.Name, so the definition's own name is not counted as a
|
||||
// use without any special traversal.
|
||||
ast.Inspect(file, func(n ast.Node) bool {
|
||||
switch v := n.(type) {
|
||||
case *ast.CallExpr:
|
||||
if name := nameOf(v.Fun); name != "" {
|
||||
key := rel + ":" + name
|
||||
found[key]++
|
||||
where[key] = append(where[key], fmt.Sprintf("%s:%d", rel, fset.Position(v.Lparen).Line))
|
||||
}
|
||||
case *ast.Ident:
|
||||
// A helper NAME that is not the callee of a call — assigned
|
||||
// to a variable, passed along — makes the eventual call
|
||||
// invisible to this guard. Fail closed on the reference.
|
||||
if helpers[v.Name] && !notAValueUse[v.Pos()] {
|
||||
key := rel + ":" + v.Name + " (used as a value, not called)"
|
||||
found[key]++
|
||||
where[key] = append(where[key], fmt.Sprintf("%s:%d", rel, fset.Position(v.Pos()).Line))
|
||||
}
|
||||
}
|
||||
return true
|
||||
})
|
||||
return nil
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("walk %s: %v", base, err)
|
||||
}
|
||||
}
|
||||
|
||||
// POSITIVE CONTROLS. This guard's failure mode is silence, and silence reads
|
||||
// as a clean tree.
|
||||
for _, dir := range []string{"internal", "cmd"} {
|
||||
if visited[dir] == 0 {
|
||||
t.Fatalf("walk root %q contributed no .go files; every door under it is invisible "+
|
||||
"to this guard and it would still be green", dir)
|
||||
}
|
||||
}
|
||||
if len(found) == 0 {
|
||||
t.Fatal("the scan found no body-derivation call sites anywhere; it is broken, " +
|
||||
"and a broken scan looks exactly like a clean tree")
|
||||
}
|
||||
|
||||
var problems []string
|
||||
for site, n := range found {
|
||||
rule, ok := allowed[site]
|
||||
if !ok {
|
||||
problems = append(problems, fmt.Sprintf("UNRULED %s (%d)\n %s",
|
||||
site, n, strings.Join(where[site], "\n ")))
|
||||
continue
|
||||
}
|
||||
if n != rule.count {
|
||||
problems = append(problems, fmt.Sprintf("COUNT CHANGED %s: ruled on %d, found %d\n %s\n reason on file: %s",
|
||||
site, rule.count, n, strings.Join(where[site], "\n "), rule.reason))
|
||||
}
|
||||
}
|
||||
for site := range allowed {
|
||||
if _, ok := found[site]; !ok {
|
||||
problems = append(problems, fmt.Sprintf("STALE RULING %s matches nothing any more — the site moved or was renamed; re-rule it rather than leaving an exemption that guards nothing", site))
|
||||
}
|
||||
}
|
||||
sort.Strings(problems)
|
||||
if len(problems) > 0 {
|
||||
t.Errorf("body-derived text is produced at %d site(s) that do not match what was ruled on (BUG-3033):\n %s\n\n"+
|
||||
"Each turns an item's body into text under another name, so a stale body makes it stale. Either carry "+
|
||||
"content_state there (set only when the text was actually produced) or update this test's table with "+
|
||||
"the new count and the reason it needs no marker. If the new site uses a derivation helper this guard "+
|
||||
"does not know, add the helper too — the helper list is the grammar this test reads, and a door built "+
|
||||
"outside it is invisible.",
|
||||
len(problems), strings.Join(problems, "\n "))
|
||||
}
|
||||
}
|
||||
|
||||
// repoRoot finds the directory holding go.mod, so the table's keys read the same
|
||||
// regardless of the checkout's own name (which differs per worktree).
|
||||
func repoRoot(t *testing.T) string {
|
||||
t.Helper()
|
||||
dir, err := os.Getwd()
|
||||
if err != nil {
|
||||
t.Fatalf("getwd: %v", err)
|
||||
}
|
||||
for {
|
||||
if _, err := os.Stat(filepath.Join(dir, "go.mod")); err == nil {
|
||||
return dir
|
||||
}
|
||||
parent := filepath.Dir(dir)
|
||||
if parent == dir {
|
||||
t.Fatal("no go.mod above the test's working directory; the scan cannot anchor its paths")
|
||||
}
|
||||
dir = parent
|
||||
}
|
||||
}
|
||||
@@ -119,9 +119,14 @@ func (s *Server) handleExportItemArtifact(w http.ResponseWriter, r *http.Request
|
||||
Fields: fields,
|
||||
Body: item.Content,
|
||||
Provenance: artifact.Provenance{
|
||||
Workspace: ws.Slug,
|
||||
ExportedAt: time.Now().UTC().Format(time.RFC3339),
|
||||
Author: author,
|
||||
Workspace: ws.Slug,
|
||||
ExportedAt: time.Now().UTC().Format(time.RFC3339),
|
||||
Author: author,
|
||||
// BUG-3033. The artifact is TEXT, so unlike a JSON door there is no
|
||||
// struct to inherit models.Item's marker from — it has to be written
|
||||
// into the format. See Provenance.ContentState for why this block and
|
||||
// why no format_version bump.
|
||||
ContentState: item.ContentState,
|
||||
FormatVersion: artifact.FormatVersion,
|
||||
},
|
||||
}
|
||||
|
||||
@@ -288,12 +288,27 @@ type AgentBootstrapUser struct {
|
||||
// `slug` was dropped in PLAN-1410 / TASK-1413: the agent addresses
|
||||
// items by `ref` (CONVE-N) — slug was dead weight.
|
||||
type AgentBootstrapConvention struct {
|
||||
Ref string `json:"ref"`
|
||||
Title string `json:"title"`
|
||||
Content string `json:"content"`
|
||||
Priority string `json:"priority,omitempty"`
|
||||
Scope string `json:"scope,omitempty"`
|
||||
Trigger string `json:"trigger,omitempty"`
|
||||
Ref string `json:"ref"`
|
||||
Title string `json:"title"`
|
||||
Content string `json:"content"`
|
||||
// ContentState marks `Content` above as a body the server knows is BEHIND the
|
||||
// convention item's live collaborative document (BUG-3033, the read-door half
|
||||
// of BUG-3000). Same values as models.Item.ContentState; omitted when the row
|
||||
// is current.
|
||||
//
|
||||
// This projection is hand-built rather than a serialised models.Item, so it
|
||||
// inherits nothing — the field is here because the payload it belongs to is
|
||||
// the one an agent loads its MUST-FOLLOW RULES from. A stale convention is a
|
||||
// rule an agent obeys after it changed, which is the same class of harm as a
|
||||
// stale playbook body and unlike a stale document nobody acts on.
|
||||
//
|
||||
// omitempty is doing real work here beyond wire compatibility: PLAN-1410 cut
|
||||
// this blob by ~40% on purpose, and a key that appears only when a body is
|
||||
// actually stale costs that effort nothing in the common case.
|
||||
ContentState string `json:"content_state,omitempty"`
|
||||
Priority string `json:"priority,omitempty"`
|
||||
Scope string `json:"scope,omitempty"`
|
||||
Trigger string `json:"trigger,omitempty"`
|
||||
}
|
||||
|
||||
// AgentBootstrapConventionMeta is the lightweight, body-less convention
|
||||
@@ -343,6 +358,23 @@ type AgentBootstrapPlaybookMeta struct {
|
||||
// from the first non-heading non-empty paragraph of the body. Capped
|
||||
// at ~240 chars so the bootstrap stays small.
|
||||
Summary string `json:"summary,omitempty"`
|
||||
// ContentState marks `Summary` above as DERIVED FROM a body the server
|
||||
// knows is behind the item's live collaborative document (BUG-3033).
|
||||
//
|
||||
// A summary is not metadata about the item; it is the item's first
|
||||
// paragraph, truncated. Calling it a summary does not change where the
|
||||
// bytes came from, so a stale body makes a stale summary — and this is the
|
||||
// same reasoning by which BUG-3000 put the marker on
|
||||
// cli.ItemSummary.ContentPreview, which is the identical shape.
|
||||
//
|
||||
// Set only when a summary was actually emitted from a real body: no
|
||||
// summary, no claim. Emitted alongside Summary for that reason rather than
|
||||
// unconditionally from the row.
|
||||
//
|
||||
// It does NOT mean the playbook's other metadata here is stale. Trigger,
|
||||
// status and invocation_slug come from the fields blob, which the
|
||||
// collaborative document does not hold; only the prose can be behind.
|
||||
ContentState string `json:"content_state,omitempty"`
|
||||
}
|
||||
|
||||
// BootstrapDashboard is the bootstrap-side dashboard projection. It
|
||||
@@ -733,12 +765,13 @@ func projectBootstrapConventions(items []models.Item) []AgentBootstrapConvention
|
||||
return ""
|
||||
}
|
||||
out = append(out, AgentBootstrapConvention{
|
||||
Ref: it.Ref,
|
||||
Title: it.Title,
|
||||
Content: it.Content,
|
||||
Priority: strField("priority"),
|
||||
Scope: strField("scope"),
|
||||
Trigger: strField("trigger"),
|
||||
Ref: it.Ref,
|
||||
Title: it.Title,
|
||||
Content: it.Content,
|
||||
ContentState: it.ContentState,
|
||||
Priority: strField("priority"),
|
||||
Scope: strField("scope"),
|
||||
Trigger: strField("trigger"),
|
||||
})
|
||||
}
|
||||
sort.SliceStable(out, func(i, j int) bool {
|
||||
@@ -841,10 +874,23 @@ type BootstrapIncludeGroup struct {
|
||||
// BootstrapIncludeItem is the generic item projection for a declared payload.
|
||||
// Content is present only for a bodies-mode declaration.
|
||||
type BootstrapIncludeItem struct {
|
||||
Ref string `json:"ref"`
|
||||
Title string `json:"title"`
|
||||
Content string `json:"content,omitempty"`
|
||||
Fields map[string]string `json:"fields,omitempty"`
|
||||
Ref string `json:"ref"`
|
||||
Title string `json:"title"`
|
||||
Content string `json:"content,omitempty"`
|
||||
// ContentState marks `Content` above as a body the server knows is BEHIND the
|
||||
// item's live collaborative document (BUG-3033). Set only in bodies mode,
|
||||
// alongside Content and under the same condition — a metadata-mode group
|
||||
// carries no body, and a marker on an absent body would be a claim about
|
||||
// something this group does not serve.
|
||||
//
|
||||
// This door is not one of the four BUG-3033 names; it was found by that
|
||||
// unit's sweep and folded in under CONVE-18. It is the generic, declaration
|
||||
// driven half of the same handler that builds AgentBootstrapConvention: any
|
||||
// collection can declare a bodies-mode payload, so the set of item bodies
|
||||
// reaching an agent through here is open-ended in a way the conventions
|
||||
// payload is not.
|
||||
ContentState string `json:"content_state,omitempty"`
|
||||
Fields map[string]string `json:"fields,omitempty"`
|
||||
}
|
||||
|
||||
// traitedFromModelCollections wraps collections.TraitedFromCollections for
|
||||
@@ -921,6 +967,13 @@ func (s *Server) collectGenericBootstrapIncludes(workspaceID string, traited []c
|
||||
entry := BootstrapIncludeItem{Ref: it.Ref, Title: it.Title}
|
||||
if wantBodies {
|
||||
entry.Content = it.Content
|
||||
// Inside the same guard as Content, deliberately. The store
|
||||
// already leaves ContentState empty when NoContent selects an
|
||||
// empty-string literal for content, so this is belt and braces
|
||||
// — but it keeps the invariant ("the marker is emitted only
|
||||
// where a real body is") readable at THIS site rather than
|
||||
// resting on a property of a query three packages away.
|
||||
entry.ContentState = it.ContentState
|
||||
}
|
||||
// Structured fields are flattened to strings: an agent reads
|
||||
// them to route, not to compute, and a stable string map is
|
||||
@@ -982,6 +1035,14 @@ func projectPlaybookMetadata(items []models.Item) []AgentBootstrapPlaybookMeta {
|
||||
hasArgs = false
|
||||
}
|
||||
}
|
||||
// The marker rides the SUMMARY, so it is set only when there is one
|
||||
// (BUG-3033). A playbook with an empty body yields no summary and
|
||||
// therefore makes no claim about one.
|
||||
summary := collections.PlaybookSummary(it.Content)
|
||||
summaryState := ""
|
||||
if summary != "" {
|
||||
summaryState = it.ContentState
|
||||
}
|
||||
out = append(out, AgentBootstrapPlaybookMeta{
|
||||
Ref: it.Ref,
|
||||
Title: it.Title,
|
||||
@@ -991,7 +1052,8 @@ func projectPlaybookMetadata(items []models.Item) []AgentBootstrapPlaybookMeta {
|
||||
Scope: strField("scope"),
|
||||
Status: strField("status"),
|
||||
HasArguments: hasArgs,
|
||||
Summary: collections.PlaybookSummary(it.Content),
|
||||
Summary: summary,
|
||||
ContentState: summaryState,
|
||||
})
|
||||
}
|
||||
// Stable order: invocation_slug-bearing first (the user-facing,
|
||||
|
||||
@@ -0,0 +1,486 @@
|
||||
package server
|
||||
|
||||
import (
|
||||
"encoding/json"
|
||||
"net/http"
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
"github.com/PerpetualSoftware/pad/internal/artifact"
|
||||
"github.com/PerpetualSoftware/pad/internal/models"
|
||||
)
|
||||
|
||||
// BUG-3033 — the doors that do NOT serialise models.Item and therefore inherit
|
||||
// nothing from BUG-3000.
|
||||
//
|
||||
// Every test here runs BOTH directions on ONE item, and asserts ABSENCE FIRST.
|
||||
// That order is the instrument, not decoration: a marker stamped unconditionally
|
||||
// would satisfy every positive assertion in this file, so a presence leg proves
|
||||
// something only after the same door has been shown to stay silent about a
|
||||
// current row.
|
||||
//
|
||||
// Assertions are on the RAW wire form (a decoded map, or the exported bytes)
|
||||
// rather than on a Go struct. A struct round-trip passes even when the field is
|
||||
// never serialised, which is precisely the failure mode — a value computed
|
||||
// correctly in the store and dropped by a projection on the way out.
|
||||
//
|
||||
// makeStale puts the op-log ahead of the item's flush watermark, which is the
|
||||
// state an applier-path write leaves behind. It is the fixture BUG-3000
|
||||
// established; the predicate itself is proven in internal/store.
|
||||
func makeStale(t *testing.T, srv *Server, itemID string) {
|
||||
t.Helper()
|
||||
if _, err := srv.store.AppendYjsUpdate(itemID, []byte{1, 2, 3}, "1"); err != nil {
|
||||
t.Fatalf("AppendYjsUpdate: %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
// decodeMap reads a response body as a generic map so a key's ABSENCE is
|
||||
// distinguishable from its zero value.
|
||||
func decodeMap(t *testing.T, body []byte) map[string]any {
|
||||
t.Helper()
|
||||
var m map[string]any
|
||||
if err := json.Unmarshal(body, &m); err != nil {
|
||||
t.Fatalf("decode response as map: %v\nbody: %s", err, body)
|
||||
}
|
||||
return m
|
||||
}
|
||||
|
||||
// TestPlaybookRunCarriesTheMarkerBothWays covers the door BUG-3033 ranks first.
|
||||
//
|
||||
// A playbook body is EXECUTED, not merely read: a stale one is an agent running
|
||||
// superseded steps. PlaybookRunResponse hoists four values out of the item into
|
||||
// a hand-built shape, so it inherits nothing.
|
||||
func TestPlaybookRunCarriesTheMarkerBothWays(t *testing.T) {
|
||||
srv := testServerWithCollab(t)
|
||||
slug := createWSWithCollections(t, srv)
|
||||
|
||||
pb := createItem(t, srv, slug, "playbooks", map[string]interface{}{
|
||||
"title": "Ship something",
|
||||
"content": "Step 1: do the thing",
|
||||
"fields": `{"status":"active","invocation_slug":"ship"}`,
|
||||
})
|
||||
|
||||
run := func(t *testing.T) map[string]any {
|
||||
t.Helper()
|
||||
rr := doRequest(srv, "POST", "/api/v1/workspaces/"+slug+"/playbooks/ship/run", map[string]any{})
|
||||
if rr.Code != http.StatusOK {
|
||||
t.Fatalf("run playbook: %d: %s", rr.Code, rr.Body.String())
|
||||
}
|
||||
return decodeMap(t, rr.Body.Bytes())
|
||||
}
|
||||
|
||||
// ABSENCE FIRST.
|
||||
if got := run(t); got["content_state"] != nil {
|
||||
t.Fatalf("a current playbook's run response carries content_state=%v; "+
|
||||
"every assertion below would then prove nothing", got["content_state"])
|
||||
}
|
||||
|
||||
makeStale(t, srv, pb.ID)
|
||||
|
||||
got := run(t)
|
||||
if got["content_state"] != models.ContentOutcomeAppliedPendingFlush {
|
||||
t.Errorf("run content_state = %v, want %q — an agent is handed superseded steps with no signal",
|
||||
got["content_state"], models.ContentOutcomeAppliedPendingFlush)
|
||||
}
|
||||
// The body is still served. The marker qualifies the body; it does not
|
||||
// replace it, and a door that withheld the content would pass a
|
||||
// presence-only assertion.
|
||||
// The EXACT body. "Non-empty" passes for a response that replaced the body
|
||||
// with anything at all, which is the failure a marker must never hide
|
||||
// (codex round 4: substituting "WRONG BODY" kept this green).
|
||||
if body, _ := got["body"].(string); body != "Step 1: do the thing" {
|
||||
t.Errorf("run body = %q, want the playbook's own body — the marker qualifies the text, it does not substitute for it", body)
|
||||
}
|
||||
}
|
||||
|
||||
// TestPlaybookShowInheritsTheContentStateMarker pins the SIBLING door.
|
||||
//
|
||||
// PlaybookShowResponse embeds *models.Item, so it gets the marker for free —
|
||||
// which is exactly why it needs a test. Nothing in the type says the embed is
|
||||
// load-bearing, and a future refactor to an explicit projection (the shape
|
||||
// BootstrapCollection and BootstrapRole already took for size) would drop the
|
||||
// field silently. Named in PlaybookRunResponse.ContentState's doc comment as the
|
||||
// guard that keeps the two doors from diverging.
|
||||
func TestPlaybookShowInheritsTheContentStateMarker(t *testing.T) {
|
||||
srv := testServerWithCollab(t)
|
||||
slug := createWSWithCollections(t, srv)
|
||||
|
||||
pb := createItem(t, srv, slug, "playbooks", map[string]interface{}{
|
||||
"title": "Ship something",
|
||||
"content": "Step 1: do the thing",
|
||||
"fields": `{"status":"active","invocation_slug":"ship"}`,
|
||||
})
|
||||
|
||||
show := func(t *testing.T) map[string]any {
|
||||
t.Helper()
|
||||
rr := doRequest(srv, "GET", "/api/v1/workspaces/"+slug+"/playbooks/ship", nil)
|
||||
if rr.Code != http.StatusOK {
|
||||
t.Fatalf("show playbook: %d: %s", rr.Code, rr.Body.String())
|
||||
}
|
||||
return decodeMap(t, rr.Body.Bytes())
|
||||
}
|
||||
|
||||
if got := show(t); got["content_state"] != nil {
|
||||
t.Fatalf("a current playbook's show response carries content_state=%v", got["content_state"])
|
||||
}
|
||||
|
||||
makeStale(t, srv, pb.ID)
|
||||
|
||||
got := show(t)
|
||||
if got["content_state"] != models.ContentOutcomeAppliedPendingFlush {
|
||||
t.Errorf("show content_state = %v, want %q — the embed that carries it has been broken",
|
||||
got["content_state"], models.ContentOutcomeAppliedPendingFlush)
|
||||
}
|
||||
// The body the marker is ABOUT must still be served (codex round 4: removing
|
||||
// it entirely left this green, since only the marker was asserted).
|
||||
if body, _ := got["content"].(string); body != "Step 1: do the thing" {
|
||||
t.Errorf("show content = %q, want the playbook's own body", body)
|
||||
}
|
||||
}
|
||||
|
||||
// TestBootstrapConventionBodiesCarryTheMarkerBothWays covers the second door an
|
||||
// agent ACTS on: a stale convention is a rule followed after it changed.
|
||||
func TestBootstrapConventionBodiesCarryTheMarkerBothWays(t *testing.T) {
|
||||
srv := testServerWithCollab(t)
|
||||
slug := createWSWithCollections(t, srv)
|
||||
|
||||
conv := createItem(t, srv, slug, "conventions", map[string]interface{}{
|
||||
"title": "Always follow this",
|
||||
"content": "The rule an agent obeys.",
|
||||
"fields": `{"status":"active","trigger":"always","priority":"must","scope":"all"}`,
|
||||
})
|
||||
|
||||
// convention reads the bootstrap and returns the raw map for our convention,
|
||||
// so an absent key is distinguishable from an empty one.
|
||||
convention := func(t *testing.T) map[string]any {
|
||||
t.Helper()
|
||||
rr := doRequest(srv, "GET", "/api/v1/workspaces/"+slug+"/agent/bootstrap", nil)
|
||||
if rr.Code != http.StatusOK {
|
||||
t.Fatalf("bootstrap: %d: %s", rr.Code, rr.Body.String())
|
||||
}
|
||||
blob := decodeMap(t, rr.Body.Bytes())
|
||||
list, _ := blob["conventions"].([]any)
|
||||
for _, e := range list {
|
||||
entry, _ := e.(map[string]any)
|
||||
if entry["ref"] == conv.Ref {
|
||||
return entry
|
||||
}
|
||||
}
|
||||
t.Fatalf("the convention did not come back in the bootstrap blob; this leg measured nothing")
|
||||
return nil
|
||||
}
|
||||
|
||||
if got := convention(t); got["content_state"] != nil {
|
||||
t.Fatalf("a current convention carries content_state=%v in the bootstrap", got["content_state"])
|
||||
}
|
||||
|
||||
makeStale(t, srv, conv.ID)
|
||||
|
||||
got := convention(t)
|
||||
if got["content_state"] != models.ContentOutcomeAppliedPendingFlush {
|
||||
t.Errorf("bootstrap convention content_state = %v, want %q — an agent loads a superseded rule with no signal",
|
||||
got["content_state"], models.ContentOutcomeAppliedPendingFlush)
|
||||
}
|
||||
if body, _ := got["content"].(string); body != "The rule an agent obeys." {
|
||||
t.Errorf("convention content = %q, want the convention's own body — a substituted rule would pass a non-empty check", body)
|
||||
}
|
||||
}
|
||||
|
||||
// TestBootstrapGenericIncludeBodiesCarryTheMarkerBothWays covers the door this
|
||||
// unit's sweep found, which BUG-3033 does not name — the declaration-driven
|
||||
// bodies-mode include group. Any collection can declare one, so the set of item
|
||||
// bodies reaching an agent through here is open-ended in a way the fixed
|
||||
// conventions payload is not.
|
||||
//
|
||||
// The third leg is the one worth reading: a METADATA-mode group must carry no
|
||||
// marker even when the item is stale, because it carries no body. A marker on an
|
||||
// absent body is a claim about something the group does not serve.
|
||||
func TestBootstrapGenericIncludeBodiesCarryTheMarkerBothWays(t *testing.T) {
|
||||
srv := testServerWithCollab(t)
|
||||
slug := createWSWithCollections(t, srv)
|
||||
|
||||
mkColl := func(t *testing.T, name, key, mode string) {
|
||||
t.Helper()
|
||||
traits := `{"bootstrap_include":[{"mode":"` + mode + `","key":"` + key + `"}]}`
|
||||
rr := doRequest(srv, "POST", "/api/v1/workspaces/"+slug+"/collections", map[string]interface{}{
|
||||
"name": name,
|
||||
"schema": `{"fields":[{"key":"status","type":"select","options":["active"],"default":"active"}]}`,
|
||||
"traits": traits,
|
||||
})
|
||||
if rr.Code != http.StatusCreated {
|
||||
t.Fatalf("create %s collection: %d: %s", name, rr.Code, rr.Body.String())
|
||||
}
|
||||
}
|
||||
mkColl(t, "Runbooks", "runbooks", "bodies")
|
||||
mkColl(t, "Catalog", "catalog", "metadata")
|
||||
|
||||
bodied := createItem(t, srv, slug, "runbooks", map[string]interface{}{
|
||||
"title": "Restart the thing",
|
||||
"content": "Step 1: restart it",
|
||||
"fields": `{"status":"active"}`,
|
||||
})
|
||||
meta := createItem(t, srv, slug, "catalog", map[string]interface{}{
|
||||
"title": "Listed only",
|
||||
"content": "This body never ships in metadata mode.",
|
||||
"fields": `{"status":"active"}`,
|
||||
})
|
||||
|
||||
entry := func(t *testing.T, key, ref string) map[string]any {
|
||||
t.Helper()
|
||||
rr := doRequest(srv, "GET", "/api/v1/workspaces/"+slug+"/agent/bootstrap", nil)
|
||||
if rr.Code != http.StatusOK {
|
||||
t.Fatalf("bootstrap: %d: %s", rr.Code, rr.Body.String())
|
||||
}
|
||||
blob := decodeMap(t, rr.Body.Bytes())
|
||||
groups, _ := blob["bootstrap_includes"].([]any)
|
||||
for _, g := range groups {
|
||||
group, _ := g.(map[string]any)
|
||||
if group["key"] != key {
|
||||
continue
|
||||
}
|
||||
items, _ := group["items"].([]any)
|
||||
for _, i := range items {
|
||||
it, _ := i.(map[string]any)
|
||||
if it["ref"] == ref {
|
||||
return it
|
||||
}
|
||||
}
|
||||
}
|
||||
t.Fatalf("no %q include entry for %s came back; this leg measured nothing", key, ref)
|
||||
return nil
|
||||
}
|
||||
|
||||
if got := entry(t, "runbooks", bodied.Ref); got["content_state"] != nil {
|
||||
t.Fatalf("a current bodies-mode include carries content_state=%v", got["content_state"])
|
||||
}
|
||||
|
||||
makeStale(t, srv, bodied.ID)
|
||||
makeStale(t, srv, meta.ID)
|
||||
|
||||
got := entry(t, "runbooks", bodied.Ref)
|
||||
if got["content_state"] != models.ContentOutcomeAppliedPendingFlush {
|
||||
t.Errorf("bodies-mode include content_state = %v, want %q",
|
||||
got["content_state"], models.ContentOutcomeAppliedPendingFlush)
|
||||
}
|
||||
if body, _ := got["content"].(string); body != "Step 1: restart it" {
|
||||
t.Errorf("bodies-mode include content = %q, want the item's own body — removing it entirely kept this test green (codex round 4)", body)
|
||||
}
|
||||
|
||||
// METADATA mode: no body is served, so no claim about a body may be made —
|
||||
// even though this item is just as stale as the one above.
|
||||
metaEntry := entry(t, "catalog", meta.Ref)
|
||||
if metaEntry["content"] != nil {
|
||||
t.Fatalf("premise broken: the metadata-mode group served a body (%v), so the assertion below measures nothing",
|
||||
metaEntry["content"])
|
||||
}
|
||||
if metaEntry["content_state"] != nil {
|
||||
t.Errorf("a metadata-mode include marked its ABSENT body as stale (content_state=%v)",
|
||||
metaEntry["content_state"])
|
||||
}
|
||||
}
|
||||
|
||||
// TestArtifactExportCarriesTheMarkerBothWays covers the portable-format door.
|
||||
//
|
||||
// The artifact is TEXT, so there is no struct to inherit the marker from — it has
|
||||
// to be written into the format, and that is a compatibility question the JSON
|
||||
// doors do not have. The legs here are the compatibility claims, made testable:
|
||||
// a CURRENT item's bytes are byte-identical to what this format emitted before
|
||||
// (no key, no blank line, no reordering), and a MARKED artifact still decodes
|
||||
// with body and fields intact, so `pad item import` is unaffected.
|
||||
func TestArtifactExportCarriesTheMarkerBothWays(t *testing.T) {
|
||||
srv := testServerWithCollab(t)
|
||||
slug := createWSWithCollections(t, srv)
|
||||
|
||||
const body = "Step 1: do the thing\n"
|
||||
pb := createItem(t, srv, slug, "playbooks", map[string]interface{}{
|
||||
"title": "Ship something",
|
||||
"content": body,
|
||||
"fields": `{"status":"active","invocation_slug":"ship"}`,
|
||||
})
|
||||
|
||||
export := func(t *testing.T) string {
|
||||
t.Helper()
|
||||
rr := doRequest(srv, "GET", "/api/v1/workspaces/"+slug+"/items/"+pb.Slug+"/export", nil)
|
||||
if rr.Code != http.StatusOK {
|
||||
t.Fatalf("export: %d: %s", rr.Code, rr.Body.String())
|
||||
}
|
||||
return rr.Body.String()
|
||||
}
|
||||
|
||||
// ABSENCE FIRST — and on this door absence means the KEY IS NOT IN THE
|
||||
// FRONTMATTER AT ALL, which is what keeps a current export byte-identical.
|
||||
current := export(t)
|
||||
if strings.Contains(current, "content_state") {
|
||||
t.Fatalf("a current item's artifact mentions content_state; the format is no longer byte-identical:\n%s", current)
|
||||
}
|
||||
|
||||
makeStale(t, srv, pb.ID)
|
||||
|
||||
marked := export(t)
|
||||
if !strings.Contains(marked, "content_state: "+models.ContentOutcomeAppliedPendingFlush) {
|
||||
t.Errorf("a stale item's artifact carries no content_state:\n%s", marked)
|
||||
}
|
||||
|
||||
// The marker rides in provenance, not beside the item's own keys: it is a
|
||||
// fact about the EXPORT, and a reader must not mistake it for a field.
|
||||
decoded, err := artifact.Decode([]byte(marked))
|
||||
if err != nil {
|
||||
t.Fatalf("a marked artifact no longer decodes — import is broken: %v", err)
|
||||
}
|
||||
if decoded.Provenance.ContentState != models.ContentOutcomeAppliedPendingFlush {
|
||||
t.Errorf("decoded provenance.content_state = %q, want %q",
|
||||
decoded.Provenance.ContentState, models.ContentOutcomeAppliedPendingFlush)
|
||||
}
|
||||
// The EXACT body, not merely a non-empty one. "Non-empty" is satisfied by an
|
||||
// export that replaced the body with unrelated text, which is the failure a
|
||||
// marker must never be able to hide (codex round 1 P3).
|
||||
if decoded.Body != body {
|
||||
t.Errorf("the marked artifact's body is not what the item holds:\n got: %q\nwant: %q", decoded.Body, body)
|
||||
}
|
||||
if decoded.Fields["invocation_slug"] != "ship" {
|
||||
t.Errorf("a marked artifact lost a field on decode: invocation_slug = %v", decoded.Fields["invocation_slug"])
|
||||
}
|
||||
// The current export must decode to the same body, so the comparison below
|
||||
// is between two artifacts of the same item rather than two arbitrary blobs.
|
||||
decodedCurrent, err := artifact.Decode([]byte(current))
|
||||
if err != nil {
|
||||
t.Fatalf("the current artifact does not decode: %v", err)
|
||||
}
|
||||
if decodedCurrent.Body != body {
|
||||
t.Errorf("the current artifact's body is not what the item holds:\n got: %q\nwant: %q",
|
||||
decodedCurrent.Body, body)
|
||||
}
|
||||
|
||||
// Byte equality after removing the marker line and normalising the one field
|
||||
// that legitimately differs between two exports (exported_at is a
|
||||
// timestamp). A line-count check alone passes for any single-line change
|
||||
// anywhere in the document, including a rewritten body.
|
||||
normalise := func(doc string) string {
|
||||
out := make([]string, 0, 32)
|
||||
for _, line := range strings.Split(doc, "\n") {
|
||||
if strings.HasPrefix(strings.TrimSpace(line), "content_state:") {
|
||||
continue
|
||||
}
|
||||
if strings.HasPrefix(strings.TrimSpace(line), "exported_at:") {
|
||||
line = " exported_at: <normalised>"
|
||||
}
|
||||
out = append(out, line)
|
||||
}
|
||||
return strings.Join(out, "\n")
|
||||
}
|
||||
if got, want := normalise(marked), normalise(current); got != want {
|
||||
t.Errorf("the two exports differ by more than the marker line:\n marked: %q\ncurrent: %q", got, want)
|
||||
}
|
||||
// And the marker really was the only thing removed by that normalisation —
|
||||
// otherwise the comparison above could be passing because both sides were
|
||||
// stripped of something.
|
||||
if normalise(marked) == marked {
|
||||
t.Error("normalisation removed nothing from the marked export; the comparison above proved nothing")
|
||||
}
|
||||
}
|
||||
|
||||
// TestPlaybookSummariesCarryTheMarkerBothWays covers the door codex round 1
|
||||
// found, which this unit's own population table missed even though the
|
||||
// precedent was already in the tree: BUG-3000 put the marker on
|
||||
// cli.ItemSummary.ContentPreview on the grounds that "a content_preview is
|
||||
// content", and a playbook `summary` is the identical shape — the body's first
|
||||
// paragraph, truncated.
|
||||
//
|
||||
// Two doors, one projection: the bootstrap blob and the dedicated /playbooks
|
||||
// endpoint both render collectPlaybookMetadata, so both are driven here. The
|
||||
// third leg is the constraint that keeps the marker honest: a playbook with no
|
||||
// summary makes no claim about one, even while its row is stale.
|
||||
func TestPlaybookSummariesCarryTheMarkerBothWays(t *testing.T) {
|
||||
srv := testServerWithCollab(t)
|
||||
slug := createWSWithCollections(t, srv)
|
||||
|
||||
withBody := createItem(t, srv, slug, "playbooks", map[string]interface{}{
|
||||
"title": "Ship something",
|
||||
"content": "Ships a list of tasks end to end.",
|
||||
"fields": `{"status":"active","invocation_slug":"ship"}`,
|
||||
})
|
||||
noBody := createItem(t, srv, slug, "playbooks", map[string]interface{}{
|
||||
"title": "Empty one",
|
||||
"content": "",
|
||||
"fields": `{"status":"active","invocation_slug":"empty"}`,
|
||||
})
|
||||
|
||||
// entries reads BOTH doors and returns the raw map for `ref` from each,
|
||||
// keyed by a label, so a key's absence is distinguishable from its zero
|
||||
// value and neither door can ride on the other's coverage.
|
||||
entries := func(t *testing.T, ref string) map[string]map[string]any {
|
||||
t.Helper()
|
||||
out := map[string]map[string]any{}
|
||||
|
||||
rr := doRequest(srv, "GET", "/api/v1/workspaces/"+slug+"/agent/bootstrap", nil)
|
||||
if rr.Code != http.StatusOK {
|
||||
t.Fatalf("bootstrap: %d: %s", rr.Code, rr.Body.String())
|
||||
}
|
||||
blob := decodeMap(t, rr.Body.Bytes())
|
||||
list, _ := blob["playbooks"].([]any)
|
||||
for _, e := range list {
|
||||
entry, _ := e.(map[string]any)
|
||||
if entry["ref"] == ref {
|
||||
out["bootstrap"] = entry
|
||||
}
|
||||
}
|
||||
|
||||
rr = doRequest(srv, "GET", "/api/v1/workspaces/"+slug+"/playbooks", nil)
|
||||
if rr.Code != http.StatusOK {
|
||||
t.Fatalf("playbooks: %d: %s", rr.Code, rr.Body.String())
|
||||
}
|
||||
var raw []map[string]any
|
||||
if err := json.Unmarshal(rr.Body.Bytes(), &raw); err != nil {
|
||||
t.Fatalf("decode playbooks: %v", err)
|
||||
}
|
||||
for _, entry := range raw {
|
||||
if entry["ref"] == ref {
|
||||
out["playbooks endpoint"] = entry
|
||||
}
|
||||
}
|
||||
|
||||
if len(out) != 2 {
|
||||
t.Fatalf("%s did not come back from both doors (got %d); this leg measured nothing", ref, len(out))
|
||||
}
|
||||
return out
|
||||
}
|
||||
|
||||
// ABSENCE FIRST, on both doors.
|
||||
for door, entry := range entries(t, withBody.Ref) {
|
||||
if entry["summary"] == nil {
|
||||
t.Fatalf("%s: premise broken — no summary was emitted, so nothing below is about a summary", door)
|
||||
}
|
||||
if entry["content_state"] != nil {
|
||||
t.Fatalf("%s: a current playbook's summary is marked %v", door, entry["content_state"])
|
||||
}
|
||||
}
|
||||
|
||||
makeStale(t, srv, withBody.ID)
|
||||
makeStale(t, srv, noBody.ID)
|
||||
|
||||
for door, entry := range entries(t, withBody.Ref) {
|
||||
if entry["content_state"] != models.ContentOutcomeAppliedPendingFlush {
|
||||
t.Errorf("%s: summary content_state = %v, want %q — an agent routes on a description "+
|
||||
"taken from a body that has moved on", door, entry["content_state"],
|
||||
models.ContentOutcomeAppliedPendingFlush)
|
||||
}
|
||||
// The summary must still be THERE. Presence was asserted only BEFORE
|
||||
// staleness, so dropping marked summaries from both doors while keeping
|
||||
// the marker left this green (codex round 4).
|
||||
if sum, _ := entry["summary"].(string); sum == "" {
|
||||
t.Errorf("%s: the marked entry carries no summary; the marker qualifies the text, it does not replace it", door)
|
||||
}
|
||||
}
|
||||
|
||||
// No summary, no claim — even though this item's row is just as stale.
|
||||
for door, entry := range entries(t, noBody.Ref) {
|
||||
if entry["summary"] != nil {
|
||||
t.Fatalf("%s: premise broken — the empty-bodied playbook emitted a summary (%v)",
|
||||
door, entry["summary"])
|
||||
}
|
||||
if entry["content_state"] != nil {
|
||||
t.Errorf("%s: a playbook with NO summary carries content_state=%v, claiming staleness "+
|
||||
"about something it does not serve", door, entry["content_state"])
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -27,14 +27,31 @@ import (
|
||||
// didn't supply (so the agent can prompt the user instead of failing
|
||||
// the call outright).
|
||||
type PlaybookRunResponse struct {
|
||||
Ref string `json:"ref"`
|
||||
Slug string `json:"slug"`
|
||||
Title string `json:"title"`
|
||||
Status string `json:"status"`
|
||||
Body string `json:"body"`
|
||||
Arguments []PlaybookArgumentSpec `json:"arguments"`
|
||||
BoundArgs map[string]any `json:"bound_args"`
|
||||
Unbound []PlaybookUnboundArgument `json:"unbound,omitempty"`
|
||||
Ref string `json:"ref"`
|
||||
Slug string `json:"slug"`
|
||||
Title string `json:"title"`
|
||||
Status string `json:"status"`
|
||||
Body string `json:"body"`
|
||||
// ContentState marks `Body` above as one the server knows is BEHIND the
|
||||
// playbook item's live collaborative document (BUG-3033, the read-door half
|
||||
// of BUG-3000). Same values and the same meaning as models.Item.ContentState;
|
||||
// omitted whenever the row is current, so a consumer that does not know the
|
||||
// field sees byte-identical responses.
|
||||
//
|
||||
// This response does not serialise models.Item — it hoists four of its
|
||||
// values into a hand-built shape — so it inherits nothing, which is why the
|
||||
// field is repeated here rather than arriving for free. Its sibling
|
||||
// PlaybookShowResponse DOES embed *models.Item and therefore does get it for
|
||||
// free; TestPlaybookShowInheritsTheContentStateMarker pins that so the two
|
||||
// doors cannot silently diverge.
|
||||
//
|
||||
// It matters more on this door than on an ordinary read: a playbook body is
|
||||
// EXECUTED. A stale one is an agent running superseded steps, which is a
|
||||
// different kind of wrong from a stale document somebody reads.
|
||||
ContentState string `json:"content_state,omitempty"`
|
||||
Arguments []PlaybookArgumentSpec `json:"arguments"`
|
||||
BoundArgs map[string]any `json:"bound_args"`
|
||||
Unbound []PlaybookUnboundArgument `json:"unbound,omitempty"`
|
||||
}
|
||||
|
||||
// PlaybookShowResponse is the result of `GET /workspaces/{ws}/playbooks/{ref}`.
|
||||
@@ -251,14 +268,15 @@ func (s *Server) handleRunPlaybook(w http.ResponseWriter, r *http.Request) {
|
||||
bound, unbound := bindPlaybookArgs(specs, supplied)
|
||||
|
||||
writeJSON(w, http.StatusOK, PlaybookRunResponse{
|
||||
Ref: item.Ref,
|
||||
Slug: item.Slug,
|
||||
Title: item.Title,
|
||||
Status: status,
|
||||
Body: item.Content,
|
||||
Arguments: specs,
|
||||
BoundArgs: bound,
|
||||
Unbound: unbound,
|
||||
Ref: item.Ref,
|
||||
Slug: item.Slug,
|
||||
Title: item.Title,
|
||||
Status: status,
|
||||
Body: item.Content,
|
||||
ContentState: item.ContentState,
|
||||
Arguments: specs,
|
||||
BoundArgs: bound,
|
||||
Unbound: unbound,
|
||||
})
|
||||
}
|
||||
|
||||
|
||||
@@ -0,0 +1,133 @@
|
||||
package store_test
|
||||
|
||||
import (
|
||||
"testing"
|
||||
|
||||
"github.com/PerpetualSoftware/pad/internal/models"
|
||||
"github.com/PerpetualSoftware/pad/internal/store"
|
||||
"github.com/PerpetualSoftware/pad/internal/store/storetest"
|
||||
)
|
||||
|
||||
// BUG-3033 — backlink snippets, the door codex round 2 found and this unit's
|
||||
// sweep missed because its vantage point was internal/server, internal/mcp and
|
||||
// internal/cli. This shape is built in the STORE, so nothing reachable from
|
||||
// those three packages could see it.
|
||||
//
|
||||
// A snippet is a window onto the source item's body, not metadata about it, so
|
||||
// a stale body makes a stale snippet — the same reasoning that marks an
|
||||
// ItemSummary preview and a playbook summary.
|
||||
//
|
||||
// Both directions on one item, absence first, on both backends: the predicate
|
||||
// here is SQL with a non-default table alias, which is precisely where the two
|
||||
// dialects can diverge.
|
||||
func TestBacklinkSnippetsCarryTheMarkerBothWays(t *testing.T) {
|
||||
for _, backend := range []struct {
|
||||
name string
|
||||
open func(t *testing.T) *store.Store
|
||||
}{
|
||||
{"SQLite", storetest.NewSQLite},
|
||||
{"Postgres", storetest.NewPostgres}, // skips unless PAD_TEST_POSTGRES_URL is set
|
||||
} {
|
||||
t.Run(backend.name, func(t *testing.T) {
|
||||
s := backend.open(t)
|
||||
|
||||
ws, err := s.CreateWorkspace(models.WorkspaceCreate{Name: "BL", Slug: "bl"})
|
||||
if err != nil {
|
||||
t.Fatalf("CreateWorkspace: %v", err)
|
||||
}
|
||||
if err := s.SeedCollectionsFromTemplate(ws.ID, "startup"); err != nil {
|
||||
t.Fatalf("seed: %v", err)
|
||||
}
|
||||
colls, err := s.ListCollections(ws.ID)
|
||||
if err != nil {
|
||||
t.Fatalf("ListCollections: %v", err)
|
||||
}
|
||||
var tasks string
|
||||
for _, c := range colls {
|
||||
if c.Slug == "tasks" {
|
||||
tasks = c.ID
|
||||
}
|
||||
}
|
||||
if tasks == "" {
|
||||
t.Fatal("no tasks collection")
|
||||
}
|
||||
|
||||
target, err := s.CreateItem(ws.ID, tasks, models.ItemCreate{Title: "Target item"})
|
||||
if err != nil {
|
||||
t.Fatalf("create target: %v", err)
|
||||
}
|
||||
source, err := s.CreateItem(ws.ID, tasks, models.ItemCreate{
|
||||
Title: "Source item",
|
||||
Content: "Some prose that links to [[Target item]] in the middle of a body.",
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("create source: %v", err)
|
||||
}
|
||||
|
||||
read := func(t *testing.T) models.Backlink {
|
||||
t.Helper()
|
||||
bls, err := s.GetBacklinks(target.ID, ws.ID, 50, 0, store.BacklinksVisibility{Unrestricted: true})
|
||||
if err != nil {
|
||||
t.Fatalf("GetBacklinks: %v", err)
|
||||
}
|
||||
for _, bl := range bls {
|
||||
if bl.SourceItemID == source.ID {
|
||||
return bl
|
||||
}
|
||||
}
|
||||
t.Fatalf("the source item produced no backlink; this leg measured nothing (got %d)", len(bls))
|
||||
return models.Backlink{}
|
||||
}
|
||||
|
||||
// SOURCE ISOLATION, before anything else. An unrelated item in the
|
||||
// same workspace is made stale; the backlink must stay unmarked.
|
||||
// Without this leg, replacing the source correlation with "any
|
||||
// op-log row exists anywhere" passes every assertion below (codex
|
||||
// round 4 ran that mutation).
|
||||
unrelated, err := s.CreateItem(ws.ID, tasks, models.ItemCreate{Title: "Unrelated"})
|
||||
if err != nil {
|
||||
t.Fatalf("create unrelated: %v", err)
|
||||
}
|
||||
if _, err := s.AppendYjsUpdate(unrelated.ID, []byte{4, 5, 6}, "1"); err != nil {
|
||||
t.Fatalf("AppendYjsUpdate(unrelated): %v", err)
|
||||
}
|
||||
|
||||
// ABSENCE FIRST, with the premise that a snippet was actually
|
||||
// produced — a marker on an empty snippet would be a claim about
|
||||
// text nobody was served.
|
||||
bl := read(t)
|
||||
if bl.Snippet == "" {
|
||||
t.Fatal("no snippet was produced, so everything below is about nothing")
|
||||
}
|
||||
if bl.ContentState != "" {
|
||||
t.Fatalf("a current source item's snippet is marked %q", bl.ContentState)
|
||||
}
|
||||
|
||||
// Put the source's document ahead of its row — the state an
|
||||
// applier-path write leaves.
|
||||
if _, err := s.AppendYjsUpdate(source.ID, []byte{1, 2, 3}, "1"); err != nil {
|
||||
t.Fatalf("AppendYjsUpdate: %v", err)
|
||||
}
|
||||
|
||||
bl = read(t)
|
||||
if bl.ContentState != models.ContentOutcomeAppliedPendingFlush {
|
||||
t.Errorf("backlink content_state = %q, want %q — the snippet is cut from a body that has moved on",
|
||||
bl.ContentState, models.ContentOutcomeAppliedPendingFlush)
|
||||
}
|
||||
if bl.Snippet == "" {
|
||||
t.Error("the marked backlink carries no snippet; the marker qualifies the text, it does not replace it")
|
||||
}
|
||||
|
||||
// The marker describes the SOURCE item, never the target the caller
|
||||
// asked about. Marking the target's row must change nothing here,
|
||||
// which is the assertion that keeps the two from being conflated.
|
||||
if _, err := s.AppendYjsUpdate(target.ID, []byte{9}, "1"); err != nil {
|
||||
t.Fatalf("AppendYjsUpdate(target): %v", err)
|
||||
}
|
||||
if got := read(t); got.ContentState != models.ContentOutcomeAppliedPendingFlush {
|
||||
t.Errorf("after marking the TARGET too, content_state = %q — it must still describe the source",
|
||||
got.ContentState)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,199 @@
|
||||
package store
|
||||
|
||||
import (
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
"github.com/PerpetualSoftware/pad/internal/models"
|
||||
)
|
||||
|
||||
// BUG-3033 — the CROSS-WORKSPACE backlink query, which the same-workspace test
|
||||
// does not reach (codex round 3 P3: clearing ContentState on every
|
||||
// cross-workspace result left that test and the existing cross-workspace tests
|
||||
// all green).
|
||||
//
|
||||
// Two query functions build models.Backlink from a source item's body. They are
|
||||
// separate SQL with separate scans, so one carrying the marker says nothing
|
||||
// about the other — which is the shape of every miss in this unit.
|
||||
func TestCrossWorkspaceBacklinkSnippetsCarryTheMarkerBothWays(t *testing.T) {
|
||||
t.Parallel()
|
||||
s := testStore(t)
|
||||
user := createTestUser(t, s, "xws-cs@example.com", "Alice", "password123")
|
||||
|
||||
wsA := createTestWorkspace(t, s, "Workspace A")
|
||||
wsB := createTestWorkspace(t, s, "Workspace B")
|
||||
if err := s.AddWorkspaceMember(wsA.ID, user.ID, "owner"); err != nil {
|
||||
t.Fatalf("add user to wsA: %v", err)
|
||||
}
|
||||
if err := s.AddWorkspaceMember(wsB.ID, user.ID, "owner"); err != nil {
|
||||
t.Fatalf("add user to wsB: %v", err)
|
||||
}
|
||||
|
||||
colA := createTestCollection(t, s, wsA.ID, "Tasks")
|
||||
colB := createTestCollection(t, s, wsB.ID, "Notes")
|
||||
|
||||
target := createTestItem(t, s, wsA.ID, colA.ID, "Cross target", "")
|
||||
targetRef := refOf(target)
|
||||
body := "Cross-link to [[" + wsA.Slug + "::" + targetRef + "]] from B, with prose around it."
|
||||
source := createTestItem(t, s, wsB.ID, colB.ID, "Cross source", body)
|
||||
|
||||
read := func(t *testing.T) models.Backlink {
|
||||
t.Helper()
|
||||
got, err := s.GetCrossWorkspaceBacklinks(wsA.ID, targetRef, user.ID, nil, 50, 0, false)
|
||||
if err != nil {
|
||||
t.Fatalf("GetCrossWorkspaceBacklinks: %v", err)
|
||||
}
|
||||
for _, bl := range got {
|
||||
if bl.SourceItemID == source.ID {
|
||||
return bl
|
||||
}
|
||||
}
|
||||
t.Fatalf("the cross-workspace source produced no backlink; this leg measured nothing (got %d)", len(got))
|
||||
return models.Backlink{}
|
||||
}
|
||||
|
||||
// SOURCE ISOLATION first: an unrelated item in the SOURCE workspace is made
|
||||
// stale, and the backlink must stay unmarked. Replacing the source
|
||||
// correlation with "any op-log row exists anywhere" passed this test without
|
||||
// it (codex round 4).
|
||||
unrelated := createTestItem(t, s, wsB.ID, colB.ID, "Unrelated in B", "nothing to do with the link")
|
||||
if _, err := s.AppendYjsUpdate(unrelated.ID, []byte{4, 5, 6}, "1"); err != nil {
|
||||
t.Fatalf("AppendYjsUpdate(unrelated): %v", err)
|
||||
}
|
||||
|
||||
// ABSENCE FIRST, premise asserted: a snippet was really produced, so the
|
||||
// marker below is about text a caller is actually served.
|
||||
bl := read(t)
|
||||
if bl.Snippet == "" {
|
||||
t.Fatal("no snippet was produced, so everything below is about nothing")
|
||||
}
|
||||
if bl.ContentState != "" {
|
||||
t.Fatalf("a current source item's cross-workspace snippet is marked %q", bl.ContentState)
|
||||
}
|
||||
|
||||
if _, err := s.AppendYjsUpdate(source.ID, []byte{1, 2, 3}, "1"); err != nil {
|
||||
t.Fatalf("AppendYjsUpdate: %v", err)
|
||||
}
|
||||
|
||||
bl = read(t)
|
||||
if bl.ContentState != models.ContentOutcomeAppliedPendingFlush {
|
||||
t.Errorf("cross-workspace backlink content_state = %q, want %q — the snippet is cut from a body that has moved on",
|
||||
bl.ContentState, models.ContentOutcomeAppliedPendingFlush)
|
||||
}
|
||||
if bl.Snippet == "" {
|
||||
t.Error("the marked backlink carries no snippet; the marker qualifies the text, it does not replace it")
|
||||
}
|
||||
}
|
||||
|
||||
// TestDirectRefSearchResultsDoNotClaimTheirTitleIsStale covers codex round 3's
|
||||
// P2, which this unit INTRODUCED: the new CLI renderer warned that a snippet was
|
||||
// derived from a stale body on results whose snippet is the item's TITLE.
|
||||
//
|
||||
// The direct-ref and numeric search paths set Snippet = Item.Title and drop the
|
||||
// body entirely, so the body's marker has nothing to qualify there. The FTS path
|
||||
// beside them keeps the marker deliberately, because its snippet IS cut from the
|
||||
// body — so the second leg here is the one that stops the fix over-reaching.
|
||||
func TestDirectRefSearchResultsDoNotClaimTheirTitleIsStale(t *testing.T) {
|
||||
t.Parallel()
|
||||
s := testStore(t)
|
||||
ws := createTestWorkspace(t, s, "Search CS")
|
||||
col := createTestCollection(t, s, ws.ID, "Tasks")
|
||||
item := createTestItem(t, s, ws.ID, col.ID, "Findable title", "A body with the word haystack in it.")
|
||||
ref := refOf(item)
|
||||
|
||||
// Make the row stale: the op-log is ahead of the flush watermark.
|
||||
if _, err := s.AppendYjsUpdate(item.ID, []byte{1, 2, 3}, "1"); err != nil {
|
||||
t.Fatalf("AppendYjsUpdate: %v", err)
|
||||
}
|
||||
|
||||
// Store.Search, not Store.SearchItems: the title-only direct-ref path lives
|
||||
// in the former, which is also what GET /api/v1/search — and therefore the
|
||||
// CLI renderer that raised this — goes through.
|
||||
byRef, err := s.Search(SearchParams{WorkspaceIDs: []string{ws.ID}, Query: ref})
|
||||
if err != nil {
|
||||
t.Fatalf("Search(ref): %v", err)
|
||||
}
|
||||
var seen bool
|
||||
for _, r := range byRef.Results {
|
||||
if r.Item.ID != item.ID {
|
||||
continue
|
||||
}
|
||||
seen = true
|
||||
// PREMISE: this really is the title-only path.
|
||||
if r.Snippet != item.Title {
|
||||
t.Fatalf("premise broken: the direct-ref snippet is %q, not the title; this test measures nothing", r.Snippet)
|
||||
}
|
||||
if r.Item.Content != "" {
|
||||
t.Fatalf("premise broken: the direct-ref result still carries a body")
|
||||
}
|
||||
if r.Item.ContentState != "" {
|
||||
t.Errorf("a title-only result is marked %q, so a renderer describes the TITLE as derived from a stale body",
|
||||
r.Item.ContentState)
|
||||
}
|
||||
}
|
||||
if !seen {
|
||||
t.Fatal("the direct-ref search did not return the item; this leg measured nothing")
|
||||
}
|
||||
|
||||
// The NUMERIC direct-ref path, which is a SECOND code path in Store.Search
|
||||
// with its own scan and its own clearing. Removing the marker-clearing from
|
||||
// only that one left this test green (codex round 4), because it queried a
|
||||
// prefixed ref and never a bare number.
|
||||
byNumber, err := s.Search(SearchParams{WorkspaceIDs: []string{ws.ID}, Query: numericPartOf(ref)})
|
||||
if err != nil {
|
||||
t.Fatalf("Search(number): %v", err)
|
||||
}
|
||||
seen = false
|
||||
for _, r := range byNumber.Results {
|
||||
if r.Item.ID != item.ID {
|
||||
continue
|
||||
}
|
||||
seen = true
|
||||
if r.Snippet != item.Title {
|
||||
t.Fatalf("premise broken: the numeric-path snippet is %q, not the title", r.Snippet)
|
||||
}
|
||||
if r.Item.ContentState != "" {
|
||||
t.Errorf("a numeric-path title-only result is marked %q", r.Item.ContentState)
|
||||
}
|
||||
}
|
||||
if !seen {
|
||||
t.Fatal("the numeric search did not return the item; that leg measured nothing")
|
||||
}
|
||||
|
||||
// FTS lookup over the same, still-stale item: here the snippet IS cut from
|
||||
// the body, so the marker must SURVIVE. Without this leg the fix above could
|
||||
// be "clear it everywhere", which would delete the signal it exists for.
|
||||
byText, err := s.Search(SearchParams{WorkspaceIDs: []string{ws.ID}, Query: "haystack"})
|
||||
if err != nil {
|
||||
t.Fatalf("Search(text): %v", err)
|
||||
}
|
||||
seen = false
|
||||
for _, r := range byText.Results {
|
||||
if r.Item.ID != item.ID {
|
||||
continue
|
||||
}
|
||||
seen = true
|
||||
// PREMISE: a snippet was actually produced, or "the marker qualifies
|
||||
// served text" is a claim about nothing (codex round 4).
|
||||
if r.Snippet == "" {
|
||||
t.Fatal("the FTS result carries no snippet, so the surviving marker qualifies nothing here")
|
||||
}
|
||||
if r.Item.ContentState != models.ContentOutcomeAppliedPendingFlush {
|
||||
t.Errorf("an FTS result's content_state = %q, want %q — its snippet is cut from the stale body",
|
||||
r.Item.ContentState, models.ContentOutcomeAppliedPendingFlush)
|
||||
}
|
||||
}
|
||||
if !seen {
|
||||
t.Fatal("the FTS search did not return the item; the second leg measured nothing")
|
||||
}
|
||||
}
|
||||
|
||||
// numericPartOf returns the number from a "PREFIX-N" ref, which is the second
|
||||
// spelling Store.Search accepts for a direct lookup and a separate code path
|
||||
// from the prefixed one.
|
||||
func numericPartOf(ref string) string {
|
||||
if i := strings.LastIndex(ref, "-"); i >= 0 {
|
||||
return ref[i+1:]
|
||||
}
|
||||
return ref
|
||||
}
|
||||
+14
-3
@@ -693,11 +693,22 @@ func (s *Store) getItemTx(tx *sql.Tx, id string) (*models.Item, error) {
|
||||
//
|
||||
// Error direction is benign either way: the predicate is read-only and its only
|
||||
// consequence is an extra index seek per row.
|
||||
const contentStateSQL = `CASE WHEN EXISTS (
|
||||
var contentStateSQL = contentStateSQLFor("i")
|
||||
|
||||
// contentStateSQLFor is contentStateSQL for a query that aliases the items table
|
||||
// as something other than `i` — the backlink queries call it `s`, since the row
|
||||
// they describe is the SOURCE item of a link (BUG-3033).
|
||||
//
|
||||
// A function rather than a second copy: two hand-maintained spellings of this
|
||||
// predicate would be two chances to drift, and a predicate that disagreed with
|
||||
// itself between doors is the shape of the defect this whole family is about.
|
||||
func contentStateSQLFor(alias string) string {
|
||||
return `CASE WHEN EXISTS (
|
||||
SELECT 1 FROM item_yjs_updates u
|
||||
WHERE u.item_id = i.id
|
||||
AND u.id > COALESCE(i.content_flushed_op_log_id, 0)
|
||||
WHERE u.item_id = ` + alias + `.id
|
||||
AND u.id > COALESCE(` + alias + `.content_flushed_op_log_id, 0)
|
||||
) THEN 'applied_pending_flush' ELSE '' END`
|
||||
}
|
||||
|
||||
// getItemScanQ is the one item-row scan behind GetItem, getItemTx and
|
||||
// GetItemIncludeDeleted — identical SELECT and hydration, differing only in
|
||||
|
||||
@@ -197,6 +197,17 @@ func (s *Store) Search(params SearchParams) (*SearchResponse, error) {
|
||||
r.Item.UpdatedAt = parseTime(updatedAt)
|
||||
hydrateItemComputedMetadata(&r.Item)
|
||||
r.Item.Content = ""
|
||||
// ...and with it the staleness marker (BUG-3033). This result
|
||||
// carries NO body-derived text at all: the snippet below is the
|
||||
// TITLE, which the collaborative document does not hold. Keeping
|
||||
// the body's marker here would make a renderer describe a title
|
||||
// as derived from a stale body — which it did, until codex round
|
||||
// 3 reproduced it through the store and the CLI.
|
||||
//
|
||||
// The FTS path further down does the opposite and deliberately:
|
||||
// there the snippet IS cut from the body, so the marker is true
|
||||
// and load-bearing even though Content itself is cleared.
|
||||
r.Item.ContentState = ""
|
||||
r.Snippet = r.Item.Title
|
||||
r.Rank = -1000 // Best possible rank so it sorts first
|
||||
results = append(results, r)
|
||||
@@ -308,6 +319,17 @@ func (s *Store) Search(params SearchParams) (*SearchResponse, error) {
|
||||
r.Item.UpdatedAt = parseTime(updatedAt)
|
||||
hydrateItemComputedMetadata(&r.Item)
|
||||
r.Item.Content = ""
|
||||
// ...and with it the staleness marker (BUG-3033). This result
|
||||
// carries NO body-derived text at all: the snippet below is the
|
||||
// TITLE, which the collaborative document does not hold. Keeping
|
||||
// the body's marker here would make a renderer describe a title
|
||||
// as derived from a stale body — which it did, until codex round
|
||||
// 3 reproduced it through the store and the CLI.
|
||||
//
|
||||
// The FTS path further down does the opposite and deliberately:
|
||||
// there the snippet IS cut from the body, so the marker is true
|
||||
// and load-bearing even though Content itself is cleared.
|
||||
r.Item.ContentState = ""
|
||||
r.Snippet = r.Item.Title
|
||||
r.Rank = -1000 // Best possible rank so it sorts first
|
||||
results = append(results, r)
|
||||
@@ -595,6 +617,19 @@ func (s *Store) Search(params SearchParams) (*SearchResponse, error) {
|
||||
r.Item.UpdatedAt = parseTime(updatedAt)
|
||||
hydrateItemComputedMetadata(&r.Item)
|
||||
r.Item.Content = ""
|
||||
// ContentState deliberately SURVIVES here, unlike on the two direct-ref
|
||||
// paths above (BUG-3033). The body is dropped to keep the payload small,
|
||||
// but r.Snippet on this path is normally cut FROM that body by
|
||||
// FTSSnippet, so the marker qualifies text this result serves.
|
||||
//
|
||||
// NOT always, and the earlier wording claimed otherwise: an FTS TITLE
|
||||
// match on an item with an empty body returns an empty snippet and an
|
||||
// empty content while still carrying the marker (codex round 4). That
|
||||
// case predates this change and is not wrong in itself — the row really
|
||||
// is behind its live document — but it is the marker describing the ROW
|
||||
// rather than any served text, which is the one thing the sentence above
|
||||
// must not be read as promising. cli.ItemSummary has the analogous case
|
||||
// when content_preview is omitted.
|
||||
results = append(results, r)
|
||||
}
|
||||
if err := rows.Err(); err != nil {
|
||||
|
||||
@@ -1148,7 +1148,7 @@ func (s *Store) GetBacklinks(targetItemID, workspaceID string, limit, offset int
|
||||
|
||||
rows, err := s.db.Query(s.q(`
|
||||
SELECT s.id, c.prefix, s.item_number, s.title, c.slug, c.icon,
|
||||
s.content, wl.position, wl.display_text, s.updated_at
|
||||
s.content, `+contentStateSQLFor("s")+`, wl.position, wl.display_text, s.updated_at
|
||||
FROM item_wiki_links wl
|
||||
JOIN items s ON s.id = wl.source_item_id
|
||||
JOIN collections c ON c.id = s.collection_id
|
||||
@@ -1167,21 +1167,30 @@ func (s *Store) GetBacklinks(targetItemID, workspaceID string, limit, offset int
|
||||
var out []models.Backlink
|
||||
for rows.Next() {
|
||||
var (
|
||||
sourceID, prefix, title, collSlug, collIcon, content, updatedAt string
|
||||
itemNumber int
|
||||
position int
|
||||
displayText sql.NullString
|
||||
sourceID, prefix, title, collSlug, collIcon, content, contentState, updatedAt string
|
||||
itemNumber int
|
||||
position int
|
||||
displayText sql.NullString
|
||||
)
|
||||
if err := rows.Scan(&sourceID, &prefix, &itemNumber, &title, &collSlug, &collIcon, &content, &position, &displayText, &updatedAt); err != nil {
|
||||
if err := rows.Scan(&sourceID, &prefix, &itemNumber, &title, &collSlug, &collIcon, &content, &contentState, &position, &displayText, &updatedAt); err != nil {
|
||||
return nil, fmt.Errorf("scan backlink row: %w", err)
|
||||
}
|
||||
// The marker rides the SNIPPET, so it is set only when there is one
|
||||
// (BUG-3033): a source item with an empty body produces no snippet and
|
||||
// makes no claim about one.
|
||||
snippet := snippetAround(content, position)
|
||||
snippetState := ""
|
||||
if snippet != "" {
|
||||
snippetState = contentState
|
||||
}
|
||||
bl := models.Backlink{
|
||||
SourceItemID: sourceID,
|
||||
SourceRef: formatRef(prefix, itemNumber),
|
||||
SourceTitle: title,
|
||||
SourceCollectionSlug: collSlug,
|
||||
SourceCollectionIcon: collIcon,
|
||||
Snippet: snippetAround(content, position),
|
||||
Snippet: snippet,
|
||||
ContentState: snippetState,
|
||||
UpdatedAt: updatedAt,
|
||||
}
|
||||
if displayText.Valid {
|
||||
@@ -1527,7 +1536,7 @@ func (s *Store) queryCrossWorkspaceBacklinksForWorkspace(
|
||||
|
||||
rows, err := s.db.Query(s.q(`
|
||||
SELECT s.id, c.prefix, s.item_number, s.title, c.slug, c.icon,
|
||||
s.content, wl.position, wl.display_text, s.updated_at
|
||||
s.content, `+contentStateSQLFor("s")+`, wl.position, wl.display_text, s.updated_at
|
||||
FROM item_wiki_links wl
|
||||
JOIN items s ON s.id = wl.source_item_id
|
||||
JOIN collections c ON c.id = s.collection_id
|
||||
@@ -1547,21 +1556,30 @@ func (s *Store) queryCrossWorkspaceBacklinksForWorkspace(
|
||||
var out []models.Backlink
|
||||
for rows.Next() {
|
||||
var (
|
||||
sourceID, prefix, title, collSlug, collIcon, content, updatedAt string
|
||||
itemNumber int
|
||||
position int
|
||||
displayText sql.NullString
|
||||
sourceID, prefix, title, collSlug, collIcon, content, contentState, updatedAt string
|
||||
itemNumber int
|
||||
position int
|
||||
displayText sql.NullString
|
||||
)
|
||||
if err := rows.Scan(&sourceID, &prefix, &itemNumber, &title, &collSlug, &collIcon, &content, &position, &displayText, &updatedAt); err != nil {
|
||||
if err := rows.Scan(&sourceID, &prefix, &itemNumber, &title, &collSlug, &collIcon, &content, &contentState, &position, &displayText, &updatedAt); err != nil {
|
||||
return nil, fmt.Errorf("scan cross-ws backlink row: %w", err)
|
||||
}
|
||||
// The marker rides the SNIPPET, so it is set only when there is one
|
||||
// (BUG-3033): a source item with an empty body produces no snippet and
|
||||
// makes no claim about one.
|
||||
snippet := snippetAround(content, position)
|
||||
snippetState := ""
|
||||
if snippet != "" {
|
||||
snippetState = contentState
|
||||
}
|
||||
bl := models.Backlink{
|
||||
SourceItemID: sourceID,
|
||||
SourceRef: formatRef(prefix, itemNumber),
|
||||
SourceTitle: title,
|
||||
SourceCollectionSlug: collSlug,
|
||||
SourceCollectionIcon: collIcon,
|
||||
Snippet: snippetAround(content, position),
|
||||
Snippet: snippet,
|
||||
ContentState: snippetState,
|
||||
UpdatedAt: updatedAt,
|
||||
SourceWorkspaceSlug: sourceWs.Slug,
|
||||
}
|
||||
|
||||
@@ -1403,6 +1403,22 @@ export interface Backlink {
|
||||
source_collection_slug: string;
|
||||
source_collection_icon: string;
|
||||
snippet: string;
|
||||
/**
|
||||
* Set when the SOURCE item's stored body is behind its live collaborative
|
||||
* document, so `snippet` above is cut from text that has moved on
|
||||
* (BUG-3033). Same values as `Item.content_state`; omitted when the source
|
||||
* row is current, which is the common case.
|
||||
*
|
||||
* It describes the SOURCE of each link, never the item the panel is about,
|
||||
* so a list can mix marked and unmarked rows.
|
||||
*
|
||||
* NOTE: the API carries this; no browser surface RENDERS it yet. Where a
|
||||
* stale marker belongs in a backlinks panel or a command-palette result is
|
||||
* a UI decision rather than a field copy, tracked as BUG-3050. This
|
||||
* declaration exists so a consumer can see the field rather than discover
|
||||
* it in a network tab.
|
||||
*/
|
||||
content_state?: 'applied_pending_flush';
|
||||
updated_at: string;
|
||||
/**
|
||||
* Optional `[[X|display]]` override. `null` (omitted from JSON) when
|
||||
|
||||
Reference in New Issue
Block a user