From ad1afb478d1fca6c2ec733011a2474b4094b1703 Mon Sep 17 00:00:00 2001 From: Stephen Waits Date: Thu, 29 Oct 2026 15:41:46 -0600 Subject: [PATCH] fix(mcp): a read says only what follows; a success adds no empty warnings (P5) Two existing contracts the envelope broke when every result got every field: a resource and its tool say the same (books://settings is books.settings, checked by the_kept_resources_read), and a tool with no warnings has no warnings key (books.add_account, defect 8). So head and next_steps are on every tool that can change the books, next_steps is on a read only when something follows it, guards is on a refusal, and the envelope adds no warnings of its own (a tool's stay as they are). The happy-path spec test now exercises books.journal_history (it fails on a tool it never called), the demo company gets its books.start size check (Done when 3), and the As-built block of P5 is written. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01HS8WZziDnyxunC23MUxrLp --- crates/ledger-cli/tests/agent_surface.rs | 45 ++++++++++++++++++++++-- crates/ledger-cli/tests/spec_mcp.rs | 2 ++ crates/ledger-mcp/src/next_steps.rs | 15 +++++--- crates/ledger-mcp/src/server_tests.rs | 4 +-- docs/mcp-interface.md | 11 ++++-- plan/served.md | 10 +++++- 6 files changed, 76 insertions(+), 11 deletions(-) diff --git a/crates/ledger-cli/tests/agent_surface.rs b/crates/ledger-cli/tests/agent_surface.rs index d534f84..04c97fe 100644 --- a/crates/ledger-cli/tests/agent_surface.rs +++ b/crates/ledger-cli/tests/agent_surface.rs @@ -429,8 +429,8 @@ fn books_start_v2_is_whole_and_fits() { assert!(s["why"].is_string() && s["required"].is_boolean(), "{s}"); assert!(s["tool"].is_string() || s["prompt"].is_string(), "{s}"); } - // The envelope: a read has steps and no head. - assert!(start.get("guards").is_some_and(Value::is_array)); + // The envelope: a read has no head of the envelope's (its own stays). + assert!(start["head"]["seq"].is_u64()); // The self-report is in the access log, beside the call. let log = answer_of(&w.call("books.access_log", json!({"limit": 50}))).unwrap(); @@ -502,3 +502,44 @@ fn a_mutating_call_answers_with_the_head_and_a_refusal_names_its_remedy() { assert_eq!(guard["code"], "stale_preview", "{refused}"); assert_eq!(guard["remedy_tool"], "books.post_entry"); } + +/// Done-when (3): `books.start` is at most 8 KiB on the demo company too +/// (the books the viewer ships with, a loose end of every kind), and says +/// what it cut. +#[test] +fn books_start_fits_8_kib_on_the_demo_books() { + let today = ledger_core::settings::today_in(None); + let place = common::Place::new("p5-start-demo"); + let source = tempfile::Builder::new() + .prefix("wb-p5-demo-") + .tempdir() + .unwrap(); + ledger_gui::demo::build(source.path(), today).unwrap(); + std::fs::create_dir_all(place.books_data().parent().unwrap()).unwrap(); + std::fs::rename(source.path().join("books-data"), place.books_data()).unwrap(); + let mut wire = place.start(None); + wire.initialize("p5"); + let raw = wire.call("books.start", json!({})); + let text = raw["result"]["content"][0]["text"].as_str().unwrap(); + assert!( + text.len() <= 8 * 1024, + "books.start is {} bytes", + text.len() + ); + let start = answer_of(&raw).unwrap(); + assert_eq!(start["state"], "active"); + assert!( + !start["needs_attention"]["items"] + .as_array() + .unwrap() + .is_empty(), + "the demo has loose ends: {start}" + ); + assert!( + start["needs_attention"]["counts"] + .as_object() + .unwrap() + .len() + >= 2 + ); +} diff --git a/crates/ledger-cli/tests/spec_mcp.rs b/crates/ledger-cli/tests/spec_mcp.rs index 6cf443b..9e31264 100644 --- a/crates/ledger-cli/tests/spec_mcp.rs +++ b/crates/ledger-cli/tests/spec_mcp.rs @@ -1294,6 +1294,8 @@ fn happy_path(name: &str) -> Mcp { // The access log, the backups' status, and a backup now (none is set // up: it says so). m.ok("books.access_log", json!({})); + // P5: the journal in words. + m.ok("books.journal_history", json!({"limit": 5})); m.ok("books.backup", json!({})); m.ok("books.back_up_now", json!({})); diff --git a/crates/ledger-mcp/src/next_steps.rs b/crates/ledger-mcp/src/next_steps.rs index 4dc7b39..f583801 100644 --- a/crates/ledger-mcp/src/next_steps.rs +++ b/crates/ledger-mcp/src/next_steps.rs @@ -12,7 +12,10 @@ //! 1. The envelope is applied in one place (`apply`, called by //! `call_tool`), so no tool can forget it. //! 2. Every result of a tool that is not read-only has a `head` (the -//! journal after the call's last write) and at least one next step. +//! journal after the call's last write) and at least one next step; a +//! read has `next_steps` when something follows it and nothing else (a +//! resource and its tool agree). `guards` is on a refusal, and a +//! success without warnings has no `warnings` key. //! 3. A tool's own fields are never overwritten: a payload that already //! has `next_steps`, `guards`, `warnings` or `head` keeps it. //! 4. A refused call has no head and no steps; a refusal that is a @@ -451,14 +454,18 @@ pub(crate) fn apply( if mutating && steps.is_empty() { steps.push(digest_step()); } - if !body.contains_key("next_steps") { + // A tool that can change the books always says what follows; a read + // says it only when something does, so what it says is what a resource + // of the same name says (`books://settings` is `books.settings`). + // `guards` is on a refusal (above) and `warnings` is the tool's own: + // a success that has none says none, as the tools always did. + if !body.contains_key("next_steps") && (mutating || !steps.is_empty()) { body.insert( "next_steps".into(), Value::Array(steps.iter().map(Step::to_json).collect()), ); } - body.entry("guards").or_insert_with(|| json!([])); - body.entry("warnings").or_insert_with(|| json!([])); + if let Some(head) = head { body.entry("head").or_insert(head); } diff --git a/crates/ledger-mcp/src/server_tests.rs b/crates/ledger-mcp/src/server_tests.rs index c8ea093..424a13f 100644 --- a/crates/ledger-mcp/src/server_tests.rs +++ b/crates/ledger-mcp/src/server_tests.rs @@ -1564,8 +1564,8 @@ fn every_mutating_result_has_head_and_next_steps() { }; let body = r.structured_content.expect("structured"); assert!( - body["guards"].is_array() && body["warnings"].is_array(), - "{name}" + body.get("guards").is_none() && body.get("warnings").is_none(), + "{name}: a success without them says none" ); let ContentBlock::Text(t) = &r.content[0] else { panic!("{name}: no text block"); diff --git a/docs/mcp-interface.md b/docs/mcp-interface.md index 4280e22..30aa106 100644 --- a/docs/mcp-interface.md +++ b/docs/mcp-interface.md @@ -490,7 +490,7 @@ every tool over every book in ledger-store's `tests/fixtures/old-books`. `books.discard_draft`. (`books.void_document`, `books.run_depreciation` and `books.run_recurring` post at once and have no `confirmed`.) - **The result envelope (P5, plan 4.6).** Every tool result, whatever the - tool, is given the same four fields in one place (`call_tool`, + tool, is given the same fields in one place (`call_tool`, `next_steps.rs::apply`), after the tool has answered; a tool's own field of the same name is kept, never overwritten: `next_steps: [{ tool | prompt | command, args?, why, required }]` (what to @@ -506,7 +506,14 @@ every tool over every book in ledger-store's `tests/fixtures/old-books`. `warnings: [..]` (a tool's own `warnings` is that field), and, on every tool that is not read-only, `head: { seq, hash }`, the journal's head after the call's last write (or as it stands, when the call wrote - nothing). The guard codes are the enum in `guards.rs`, each with its + nothing). A tool that can change the books always has `head` and + `next_steps`; a read has `next_steps` only when something follows it, so + a resource and its tool say the same (`books://settings` is + `books.settings`). `guards` is on a refusal; a success that has no + warnings has no `warnings` key. A tool that can change the books always answers with all four; + a read has `next_steps` only when something follows it and no `guards` + or `warnings` of the envelope's, so a resource and its tool say the same + (`books://settings` is `books.settings`). The guard codes are the enum in `guards.rs`, each with its remedy tool: `stale_preview` (the tool again, previewed), `format_upgrade_pending` (`books.start`), `receipt_required` (`books.attach_receipt`), `approval_required` (`books.approve_entry`), diff --git a/plan/served.md b/plan/served.md index 0e556fa..2ae0ba8 100644 --- a/plan/served.md +++ b/plan/served.md @@ -1515,7 +1515,15 @@ Each unit follows the 8.0.4 template. "A-n" is an owner answer numbered in 8.3.1 5. **Files:** owns `crates/ledger-mcp/src/guards.rs`, `tools/prompts.rs`, `prompts/*.md`, `crates/ledger-store/src/describe.rs`, `crates/ledger-cli/tests/agent_evals/`; touches `server.rs` (the result path), `tools/kept.rs`, `opening.rs`, `bridge.rs`, `handbook.rs`, the 8.0.3 files, `docs/mcp-interface.md`. 6. **Tests and proofs:** named above; no kernel or Lean change. 7. **Done when:** (1) every mutating result carries `head` and non-empty `next_steps`; (2) every guard's remedy tool exists; (3) `books.start` ≤ 8 KiB on the `p2-served` fixture and on the demo books; (4) the prompts list and render; (5) `tool_descriptions_fit_700_characters` pins 0; (6) the six evals pass; (7) `just check` green. The live check (a fresh Claude Code session in an empty folder, and a Codex session, each posting an entry by following `next_steps`) is the EM's, recorded in the As-built block. -8. **As built:** none yet. +8. **As built (2026-10-29, branch P5, eight commits on b97234ac; no schema change, so no migration number was taken):** + - Step 1 as written, with these shapes: `next_steps` items are `{ tool | prompt | command, args?, why, required }` (the `command` form carries `books.export_cpa_package`'s download command, which is not a tool); a result that already has `next_steps` or `head` keeps it (`books.start` has its own `next_steps` and `head`, `books.verify` and `books.access_log` their own `head`); the envelope adds no `warnings` of its own (a tool's own stay as they are, `books.cash_flow`'s object warnings included) and `guards` is on a refusal only; `head` and `next_steps` are on every tool that is not read-only (its annotation), `head` being the journal's head after the call; a read has `next_steps` only when something follows it, because a resource and its tool must say the same (`the_kept_resources_read` compares `books://settings` with `books.settings`), and an empty `warnings` on `books.add_account` broke `add_account_warns_without_tax_category_defect_8`. A refusal that is a guard carries `structuredContent.guards`; its text is unchanged. After a close the step is `books.verify`; **P8 replaces it with `attest_period` when it merges**. The guard registry has ten codes (the plan's seven examples plus `party_shares`, `party_required`, `open_drafts`); `untagged_control_line`, `duplicate_vendor_ref` and `first_number_locked` are in the registry and recognized from a refusal that begins with the code, but no tool refuses with them yet. The Needs-attention items are added after a change to an entry or an invoice only (not to a party or an account). `every_guard_names_an_existing_tool` is in `guards.rs`, `every_mutating_result_has_head_and_next_steps` in `server_tests.rs`. + - Step 2 as written, except: `head.last_handoff`, `rules.attestation_bar`, `rules.approval_rules` as a list and `attestation` are not in v2 (P8's); `you.verified`/`self_reported` and `isolation`'s shape were not added to `you` (the plan's `you` has four keys and has them). `agent` is stored on the access record as `agent {model?, reasoning_effort?, harness?}` (a new optional field of `ledger_store::access::Access`; the access log is its own chain, not the journal, so 8.0.6 does not apply). `you` is absent on the `books://start` resource (no token made that call). The fit loop shortens attention items to four and waiting drafts to three before the lists. + - Step 3 as written. The prompt tests are in `crates/ledger-cli/tests/agent_surface.rs` (`the_prompts_list_and_render`), not `crates/ledger-mcp/tests/prompts.rs`: the prompt renderer is `pub(crate)` and the p2-served harness is in ledger-cli; the "golden render" is each prompt's file with its blanks filled, compared exactly, rather than a stored copy. Prompts need `books:read`. The report resources now take their dates from the query; `books://entries/{id}` and `books://journal/{seq}` are new reads; `books://attachments/{id}` was already served. + - Step 4: `books.journal_history` as written, in `tools/journal.rs`, `subject` being the line's first subject. **Departure:** the describer in `crates/ledger-store/src/describe.rs (new)` is the server's; the viewer's `describe` arms in `ledger-gui/src/pages/journal.rs` were not moved (they carry icons, routes and today's party and account names; moving them behind a formatting trait was a large change to the viewer for no figure or behavior). So there are two describers; the server's quotes only identifiers, dates, amounts and short typed names and never a setting's value. Decide with the EM whether to unify them (U-unit). + - Step 5: parameter names are `reason` (on `books.set_convention`) and `revises` with `reason` (on `books.record_decision` and `books.raise_item`); `subject`, `decision_md` and `title` became optional in those two schemas so a source-only call can leave them out (given, they must equal the record's). Test names: `a_source_only_revision_is_journaled`, `source_revisions_wait_for_the_upgrade` (both in `crates/ledger-store/tests/source_revisions.rs`), plus `a_source_revision_must_change_only_the_source` and the over-the-wire `a_source_only_revision_over_the_wire`. 8.0.6: all six items (the `p5_before` fixture was written by b97234ac; no golden moved). + - Step 6: the eight descriptions were moved into the handbook sections `cpa-handoff`, `parties-and-tins`, `1099`, `files-in-out`, `reconciliation` and `viewer`; `tool_descriptions_fit_700_characters` pins none and the allowance list is gone. `instructions_budget` was already green with the final connect text (it already had no `preview_token`, reversal drafts or `request_tin_entry`). The aliases are removed (and `customer`/`vendor` are required); every over-the-wire test that used them was changed to the new names (`party_id_alias_still_works` became `a_customer_by_id_and_by_name_is_one_record`). `docs/mcp-interface.md`'s overview sections now describe the surface as built. + - Step 7: the six transcripts are as named. Each file may have `setup` (direct calls that make the chart and the books an agent finds; not the agent's moves), `settings`, and steps `call` (only `books.start`, first), `follow`, `follow_prompt`, `expect_next_steps_contains` and `expect`; `{"$upload": ..}` and `{"$var": ..}` stand for an uploaded file's id and a value a setup call saved. The runner is `agent_evals.rs`. + - Not run by the worker (D-60): `just check`, `just prove`; the live check of Done-when (7) (a fresh Claude Code session in an empty folder and a Codex session, each posting an entry by following `next_steps`) is the EM's. Checked: `cargo test -p ledger-mcp`, the store tests `source_revisions`, `format` and `store`, and the over-the-wire files this unit edited, with fmt and clippy on the five crates touched. **Struck from P5:** ~~`books.job` and the 20 s rule~~ → D-53 (no measured call approaches a client limit over the bridge; jobs come with P9b if claude.ai's 240 s limit is ever real); ~~URL-mode elicitation~~ → nothing local to elicit (D-28); ~~`ledger-view` crate~~ → U0b (`PackageBooks`); ~~`books.viewer_status`~~ → `books.show` already reports `viewer_open`.