From a247e8954028f0b3242209f8a2a0ba0f9985bf02 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 11 Jul 2026 22:47:51 +0000 Subject: [PATCH 1/8] docs(references): add PRD-0017 and ADR-0018 for interactive review sessions PRD-0017 specifies the agent<->operator feedback loop on a wiki page, ported from the lavish-axi local-artifact review pattern but keyed to (wiki_id, title) targets instead of file paths. ADR-0018 records the structural decisions: pure session core in sp42-platform, in-memory store with Notify wake-ups in sp42-server, /dev/review/* routes gated by the bridge session, and Parsoid block/cite anchors in place of CSS selectors. Regenerates the architecture map for the new docs. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012jVtT6gPkHFvpiuDLS5rVR --- docs/domains/references/README.md | 2 + .../0018-review-session-bridge-contract.md | 96 ++++++++++++ .../prd/0017-interactive-review-sessions.md | 143 ++++++++++++++++++ docs/platform/architecture.md | 16 +- 4 files changed, 250 insertions(+), 7 deletions(-) create mode 100644 docs/domains/references/adr/0018-review-session-bridge-contract.md create mode 100644 docs/domains/references/prd/0017-interactive-review-sessions.md diff --git a/docs/domains/references/README.md b/docs/domains/references/README.md index d7680b7a..8e2fd884 100644 --- a/docs/domains/references/README.md +++ b/docs/domains/references/README.md @@ -21,6 +21,7 @@ Operational guidance for the model panel is in [model-panel.md](model-panel.md). - [PRD-0010 — Citation-verification agent surface (MCP)](prd/0010-citation-verification-mcp-surface.md) — expose probe/verify verbs to agent clients via the `sp42-mcp` shell - [PRD-0012 — Citation insertion for unsourced claims](prd/0012-citation-insertion.md) — ground a candidate source against a sentence's atomic claims, then propose a `` insert - [PRD-0014 — Citation repair and insertion, browser surface](prd/0014-citation-repair-insertion-browser-surface.md) — per-finding action row in the Citations tab routing to text-edit or citation-fix, operator always chooses +- [PRD-0017 — Interactive review sessions](prd/0017-interactive-review-sessions.md) — agent opens a session on a page, the operator queues anchored feedback, the agent polls for it (loop ported from lavish-axi, keyed to wiki pages) ## Architecture Decision Records @@ -29,6 +30,7 @@ Operational guidance for the model panel is in [model-panel.md](model-panel.md). - [ADR-0009 — Source-snapshot storage for reproducibility](adr/0009-citation-source-snapshot-storage.md) - [ADR-0011 — Article-level citation verification (the review path)](adr/0011-article-citation-verification.md) - [ADR-0015 — Rules-compliant read-only fetch edge](adr/0015-rules-compliant-read-only-fetch-edge.md) +- [ADR-0018 — Review-session bridge contract and store placement](adr/0018-review-session-bridge-contract.md) The shared LLM posture these depend on (model panel, measured agreement, keyless inference endpoint) is a platform decision, diff --git a/docs/domains/references/adr/0018-review-session-bridge-contract.md b/docs/domains/references/adr/0018-review-session-bridge-contract.md new file mode 100644 index 00000000..36df955c --- /dev/null +++ b/docs/domains/references/adr/0018-review-session-bridge-contract.md @@ -0,0 +1,96 @@ +# ADR-0018: Review-session bridge contract and store placement + +**Status:** Proposed +**Date:** 2026-07-11 +**Author:** Claude Code (Fable), for Luis Villa + +PRD-0017 ports the agent↔human artifact-review loop popularized by +[lavish-axi](https://github.com/kunchenguid/lavish-axi) from local HTML files +to wiki pages. This ADR records the structural decisions: where the contract +lives, how sessions are keyed, how the long-poll is implemented, and how the +routes are gated. + +## Context + +The ported loop has three parties — an agent (CLI/MCP), an operator surface +(browser), and a rendezvous server — and one contract: session state plus a +queued-feedback drain with strict etiquette (deliver-before-ended, operator +ends gate reopens, `next_step` guidance in every response). lavish-axi keys +sessions by canonical file path, persists them in `~/.lavish-axi/state.json`, +watches the file for live reload, and long-polls one indefinite HTTP request +with whitespace heartbeats. None of the file-specific machinery maps to a +remote wiki page; the loop semantics map cleanly. + +SP42 constraints that shaped the port: contracts shared by more than one crate +live in `sp42-platform` (ADR-0013); business logic is pure and tested with no +I/O (Constitution Art. 1–2); the localhost server's mutable stores are +`Arc>` fields on `AppState` with session+CSRF gating on +mutating routes (ADR-0002); external interfaces carry versioned serde +contracts (Art. 9.1). + +## Decision + +1. **Pure core in `sp42-platform::review_session`.** `ReviewSession` owns + every transition — `queue_prompts`, `take_feedback` (deliver-before-ended, + drain-exactly-once), `gate_reopen` (operator-ended refuses a plain + reopen), `resume`, `end`, `agent_reply` — as pure methods over injected + `now_ms`. The wire types (`ReviewOpenRequest/Response`, `ReviewPollRequest/ + Response`, `ReviewQueueRequest/Response`, ack/list shapes) live beside + them, all embedding `REVIEW_SESSION_CONTRACT_VERSION`. The `next_step` + strings are pure functions too, so the loop etiquette is unit-tested. + +2. **Session identity is the parsed `(wiki_id, title)` pair.** The open + route accepts a bare title or a pasted wiki URL and unwraps it with the + existing `parse_page_target` (same as verify-page), so every URL spelling + of a page collapses to one session — the analog of lavish-axi's + canonical-path key, with MediaWiki title normalization playing the role of + `realpath`. The store key is `wiki_id ␟ title` (unit-separator joined, so + the pair is unambiguous). The revision is session *state* (pinned at open, + re-pinned on resume), not identity. + +3. **Server store: `AppState.review_sessions`, in-memory, with a per-session + `tokio::sync::Notify`.** Each entry pairs the pure `ReviewSession` with a + `Notify`; queue/end call `notify_one`, whose stored permit covers feedback + queued while no poll is waiting. The poll handler drains, waits + (`tokio::time::timeout` over `notified()`, clamped to 55 s), and drains + once more. Bounded server-side waits with a re-arming CLI loop replace + lavish-axi's single indefinite request — no heartbeat protocol, no + proxy-timeout exposure, identical agent ergonomics. No cross-restart + persistence: this is a localhost dev-bridge surface (PRD-0017 Risks). + +4. **Routes under `/dev/review/*`, POSTs session+CSRF gated.** Path constants + in `sp42_platform::routes` like every other route contract. All five POST + routes (`open`, `prompts`, `poll`, `agent-reply`, `end`) require the + bridge session cookie plus the CSRF header (ADR-0002) — open reads through + the caller's wiki identity to pin the latest revision, and the rest mutate + session state. `GET /dev/review/sessions` is an ungated read-only + inspection surface, listed in the operator endpoint manifest. + +5. **The article outline replaces the DOM snapshot.** Where lavish-axi ships + a walked-DOM outline with synthetic uids, the open response ships + `build_article_outline` over the Parsoid blocks the editor already + extracts (`extract_blocks`, ADR-0011): block ordinal, kind, truncated + text, cite ids. Prompt anchors reuse the same coordinates + (`block_ordinal` + optional `ref_id` + optional verbatim `selected_text`), + which map directly onto citation use-sites — no CSS selectors. + +6. **Agent surface is `sp42-cli review …` over the bridge-bootstrap pattern.** + Open/poll/queue/reply/end/sessions subcommands authenticate exactly like + `verify-page` (bootstrap → cookie + CSRF token). The poll loop keeps + stdout reserved for one structured response and narrates waiting on + stderr, mirroring the ported tool's agent ergonomics. MCP verbs are + deferred (PRD-0017 open question 3) because `sp42-mcp` has no + localhost-server client today. + +## Consequences + +- The loop's semantics are testable without a server, and the server tests + exercise only glue (gating, wake-ups, status codes). +- A browser Review panel needs no new contract — it consumes the same routes + the CLI queueing surface uses. +- Session state dies with the server process; if review sessions outgrow the + dev bridge (multi-operator, cross-restart), persistence and eviction become + a follow-up ADR alongside the deployment-mode gating question. +- The bounded-wait poll means worst-case feedback latency for a poll that + raced a notification is one wait window; the permit semantics make that + race a non-event in the single-agent case. diff --git a/docs/domains/references/prd/0017-interactive-review-sessions.md b/docs/domains/references/prd/0017-interactive-review-sessions.md new file mode 100644 index 00000000..5182aeff --- /dev/null +++ b/docs/domains/references/prd/0017-interactive-review-sessions.md @@ -0,0 +1,143 @@ +# PRD-0017: Interactive review sessions — agent↔operator feedback loop on a page + +**Drafter:** Claude Code (Fable) +**Editor:** Luis Villa +**Date:** 2026-07-11 +**State:** Draft +**Discussion:** +**Spawned ADRs:** [ADR-0018](../adr/0018-review-session-bridge-contract.md) +(review-session bridge contract and store placement) + +## Problem + +An agent working on citations (via `sp42-cli` or `sp42-mcp`) and an operator +reviewing an article currently have no shared, structured feedback loop. The +operator's judgment reaches the agent as free-form chat: "the third paragraph's +quote isn't in the source" — no page identity, no revision, no block, no cite +id. The agent guesses at anchors, and the operator repeats themselves. + +Local-first agent review tools solved this loop for *files*: +[lavish-axi](https://github.com/kunchenguid/lavish-axi) keys a session to an +HTML artifact's canonical path, lets the human annotate elements and text +ranges in a browser, and delivers the queued, anchored prompts to whichever +agent polls the session. The loop etiquette it ships — deliver queued feedback +before reporting an ended session, refuse to reopen a session the *human* +ended unless explicitly asked, tell the agent its next step in every response +— is what makes the loop usable by agents without bespoke prompting. + +SP42 wants that loop, keyed to what SP42 reviews: a **wiki page**, identified +by `(wiki_id, title)` and pinned to a revision — passing a Wikipedia URL where +lavish-axi passes a file path. + +## Proposal + +A localhost **review-session bridge**: the agent opens a session on a page +target (bare title or pasted wiki URL, including `oldid` URLs), the operator +queues anchored feedback prompts against that page, and the agent polls the +session to collect them. + +User-facing behavior: + +- **Open**: `sp42-cli review open ` opens (or resumes) the + session and returns a compact **article outline** — block ordinals, block + kinds, truncated text, and cite ids from the Parsoid decomposition — the + agent's map for resolving anchors, plus a `next_step` telling it to poll. +- **Annotate**: the operator queues prompts anchored to a block ordinal, + optionally narrowed to a cite id (`cite_ref-…`) or a verbatim selected-text + range, or free-form messages. Anchors use Parsoid structure, not CSS + selectors, so they survive re-rendering and map directly onto the + use-site anchors the verification domain already uses. The MVP queueing + surface is `sp42-cli review queue` (dev/test) and the gated + `/dev/review/prompts` route; the browser annotation panel is a committed + follow-up (see Open questions). +- **Poll**: `sp42-cli review poll ` waits for feedback. Each + server request is a bounded wait; the CLI re-arms until feedback, an end, + or a missing session, narrating the wait on stderr while stdout stays a + single structured response. `--agent-reply ""` posts a chat line + to the operator surface before the wait starts. +- **Session-end etiquette** (ported): feedback queued before an end still + delivers first, flagged `session_ended`; only a later poll reports `ended`. + An **operator**-ended session refuses a plain reopen (HTTP 409 with + guidance) unless `--reopen` is passed; an **agent**-ended session + (`sp42-cli review end`) reopens freely. Every response carries `next_step` + prose telling the agent what to do — poll again, apply-and-reply, or stop + without reopening. +- **Inventory**: `sp42-cli review sessions` and `GET /dev/review/sessions` + list open sessions with pending-prompt counts. + +Everything stays read-only with respect to the wiki: a review session never +edits anything. Acting on feedback rides the existing operator-confirmed +lanes (re-verify, bare-URL repair, inline edit). + +## Definition of Done + +- [ ] Opening with a pasted wiki URL unwraps the title and records the pinned + revision, verified by `review_session_loop_delivers_operator_feedback_to_the_agent` + (`sp42-server`) +- [ ] Prompts queued before an operator "send & end" deliver on the next poll + flagged `session_ended`, and only the following poll reports `ended`, + verified by `feedback_queued_before_an_end_delivers_before_ended` + (`sp42-platform`) and the server loop test +- [ ] A plain open of an operator-ended session refuses with + `review-session-operator-ended` and an explicit `reopen` resumes it, + verified by `review_reopen_gate_requires_explicit_reopen_after_operator_end` + (`sp42-server`) +- [ ] Feedback drains exactly once — a second poll after delivery reports + `waiting`, verified by `take_feedback_drains_once_and_reopens` + (`sp42-platform`) +- [ ] Queueing wakes a concurrently waiting poll without a full timeout wait, + verified by `queued_feedback_wakes_a_waiting_poll` (`sp42-server`) +- [ ] All review POST routes require an authenticated bridge session and CSRF + header, verified by `review_routes_require_an_authenticated_bridge_session` + (`sp42-server`) +- [ ] Every response embeds `contract_version` and a `next_step`, and the + ended `next_step` distinguishes operator-ended (do not reopen) from + agent-ended, verified by `next_step_matches_the_loop_etiquette` + (`sp42-platform`) +- [ ] Session opens, feedback queueing, deliveries, and ends emit `tracing` + events, checkable in server logs + +## Alternatives + +- **Adopt lavish-axi itself** (open the article as a local HTML artifact): + rejected — its identity model (canonical file path, chokidar file watching, + sibling-asset serving) does not map to remote wiki URLs; its annotation + anchors (truncated CSS selectors + DOM ranges) are weaker than Parsoid + block/cite anchors for wiki work; and SP42 already owns the localhost + server, session/CSRF runtime, and Parsoid read path the loop needs. +- **WebSocket feedback channel** (reuse the coordination-room substrate): + deferred — a long-poll matches the agent's blocking-command ergonomics + (one CLI invocation = one rendezvous) and needs no client protocol. The + browser annotation panel may later ride the existing WebSocket/SSE paths. +- **Key sessions by URL string** (hash the raw URL like lavish-axi hashes + paths): rejected — two URLs for the same page (percent-encoding, + underscores, `oldid` forms) must resolve to one session, so the canonical + key is the parsed `(wiki_id, title)` pair. + +## Risks + +- **The MVP has no browser annotation surface**, so operator queueing runs + through the CLI/route until the panel ships — the loop is real but the + operator ergonomics are not yet. Mitigation: the wire contract is versioned + and panel-agnostic; the browser panel consumes the same routes. +- **Feedback is held in server memory only**; a server restart drops queued + prompts, unlike lavish-axi's `state.json`. Acceptable for a localhost dev + bridge; revisit persistence when multi-operator review sessions arrive. +- **An agent could ignore the reopen etiquette** and hammer `reopen: true`. + The gate is advisory by design (same as the ported tool); the operator + surface always shows who reopened. + +## Open questions + +1. **Where does the operator annotate?** Proposed: a Review panel in the + browser shell's article surface, listing outline blocks with per-block + queue actions and a session composer, reusing the `/dev/review` routes. + Until then the CLI `review queue` command is the queueing surface. +2. **Should the outline include verification findings** (merge + `PageVerificationReport` findings into the open response) so the operator + annotates findings rather than raw blocks? Proposed: yes, as an opt-in + flag once the browser panel exists; verification spends inference budget, + so it must stay operator-triggered (PRD-0014 posture). +3. **MCP surface**: should `sp42-mcp` grow `review_open`/`review_poll` verbs? + Proposed: yes, but the MCP server is standalone today (no localhost-server + client); bridging it is its own small ADR. diff --git a/docs/platform/architecture.md b/docs/platform/architecture.md index 72ecb07b..d0236702 100644 --- a/docs/platform/architecture.md +++ b/docs/platform/architecture.md @@ -47,10 +47,10 @@ flowchart LR flowchart TB subgraph SHELLS["Shells — composition roots"] sp42_app["sp42-app
ADR-0004 … +5 more ADRs"]:::shell - sp42_cli["sp42-cli
ADR-0004 … +3 more ADRs"]:::shell + sp42_cli["sp42-cli
ADR-0004 … +4 more ADRs"]:::shell sp42_desktop["sp42-desktop
ADR-0004, ADR-0005, ADR-0013"]:::shell sp42_devtools["sp42-devtools
ADR-0004, ADR-0013"]:::shell - sp42_mcp["sp42-mcp
ADR-0016"]:::shell + sp42_mcp["sp42-mcp
ADR-0016, ADR-0018"]:::shell sp42_server["sp42-server
ADR-0003 … +6 more ADRs"]:::shell sp42_ui["sp42-ui
ADR-0005"]:::shell end @@ -71,7 +71,7 @@ flowchart TB sp42_inference["sp42-inference
ADR-0011, ADR-0013, ADR-0015"]:::platform sp42_live["sp42-live
ADR-0004, ADR-0013"]:::platform sp42_parsoid["sp42-parsoid"]:::platform - sp42_platform["sp42-platform
ADR-0013, ADR-0016, ADR-0017"]:::platform + sp42_platform["sp42-platform
ADR-0013 … +3 more ADRs"]:::platform sp42_reporting["sp42-reporting
ADR-0004 … +4 more ADRs"]:::platform sp42_types["sp42-types
ADR-0004 … +8 more ADRs"]:::platform sp42_wiki["sp42-wiki
ADR-0004 … +3 more ADRs"]:::platform @@ -151,7 +151,7 @@ Reading notes: |---|---|---|---|---| | `sp42-app` | shell | [ADR-0004](adr/0004-crate-boundary-collaboration-model.md), [ADR-0005](adr/0005-design-system-shared-component-layer.md), [ADR-0008](../domains/references/adr/0008-citation-verification-contract.md), [ADR-0010](adr/0010-operator-confirmed-content-proposals.md), [ADR-0012](adr/0012-frontend-e2e-testing-approach.md), [ADR-0013](adr/0013-layered-platform-domain-architecture.md) | [PRD-0002](../domains/patrolling/prd/0002-patrol-review-workflow.md), [PRD-0006](../domains/patrolling/prd/0006-multi-operator-coordination.md), [PRD-0010](../domains/references/prd/0010-citation-verification-mcp-surface.md), [PRD-0014](../domains/references/prd/0014-citation-repair-insertion-browser-surface.md) | | | `sp42-citation` | domain | [ADR-0013](adr/0013-layered-platform-domain-architecture.md) | [PRD-0001](../domains/references/prd/0001-citation-verification.md), [PRD-0008](../domains/references/prd/0008-bare-url-repair.md), [PRD-0010](../domains/references/prd/0010-citation-verification-mcp-surface.md), [PRD-0014](../domains/references/prd/0014-citation-repair-insertion-browser-surface.md), [PRD-0016](../domains/assessment/prd/0016-ga-evidence-appendix-renderer.md) | | -| `sp42-cli` | shell | [ADR-0004](adr/0004-crate-boundary-collaboration-model.md), [ADR-0005](adr/0005-design-system-shared-component-layer.md), [ADR-0013](adr/0013-layered-platform-domain-architecture.md), [ADR-0015](../domains/references/adr/0015-rules-compliant-read-only-fetch-edge.md) | [PRD-0001](../domains/references/prd/0001-citation-verification.md) | | +| `sp42-cli` | shell | [ADR-0004](adr/0004-crate-boundary-collaboration-model.md), [ADR-0005](adr/0005-design-system-shared-component-layer.md), [ADR-0013](adr/0013-layered-platform-domain-architecture.md), [ADR-0015](../domains/references/adr/0015-rules-compliant-read-only-fetch-edge.md), [ADR-0018](../domains/references/adr/0018-review-session-bridge-contract.md) | [PRD-0001](../domains/references/prd/0001-citation-verification.md), [PRD-0017](../domains/references/prd/0017-interactive-review-sessions.md) | | | `sp42-coordination` | platform | [ADR-0004](adr/0004-crate-boundary-collaboration-model.md), [ADR-0013](adr/0013-layered-platform-domain-architecture.md) | [PRD-0006](../domains/patrolling/prd/0006-multi-operator-coordination.md) | | | `sp42-core` | hybrid | [ADR-0001](adr/0001-foundational-decisions.md), [ADR-0003](adr/0003-node-anchored-wikitext-editing.md), [ADR-0004](adr/0004-crate-boundary-collaboration-model.md), [ADR-0005](adr/0005-design-system-shared-component-layer.md), [ADR-0006](adr/0006-using-llms.md), [ADR-0007](../domains/references/adr/0007-citation-verification-semantics.md), [ADR-0008](../domains/references/adr/0008-citation-verification-contract.md), [ADR-0009](../domains/references/adr/0009-citation-source-snapshot-storage.md), [ADR-0010](adr/0010-operator-confirmed-content-proposals.md), [ADR-0011](../domains/references/adr/0011-article-citation-verification.md), [ADR-0013](adr/0013-layered-platform-domain-architecture.md), [ADR-0015](../domains/references/adr/0015-rules-compliant-read-only-fetch-edge.md) | [PRD-0001](../domains/references/prd/0001-citation-verification.md), [PRD-0002](../domains/patrolling/prd/0002-patrol-review-workflow.md), [PRD-0004](../domains/patrolling/prd/0004-reviewer-actions-on-wikimedia.md), [PRD-0005](../domains/patrolling/prd/0005-operator-identity-and-session.md), [PRD-0007](../domains/references/prd/0007-llm-output-benchmarking.md), [PRD-0015](../domains/assessment/prd/0015-article-stability-signal.md), [PRD-0016](../domains/assessment/prd/0016-ga-evidence-appendix-renderer.md) | Hybrid exemption — re-export facade being split into platform/domain crates and retired ([ADR-0013](adr/0013-layered-platform-domain-architecture.md)) | | `sp42-desktop` | shell | [ADR-0004](adr/0004-crate-boundary-collaboration-model.md), [ADR-0005](adr/0005-design-system-shared-component-layer.md), [ADR-0013](adr/0013-layered-platform-domain-architecture.md) | — | | @@ -159,12 +159,12 @@ Reading notes: | `sp42-fetch` | platform | [ADR-0013](adr/0013-layered-platform-domain-architecture.md), [ADR-0015](../domains/references/adr/0015-rules-compliant-read-only-fetch-edge.md) | — | | | `sp42-inference` | platform | [ADR-0011](../domains/references/adr/0011-article-citation-verification.md), [ADR-0013](adr/0013-layered-platform-domain-architecture.md), [ADR-0015](../domains/references/adr/0015-rules-compliant-read-only-fetch-edge.md) | — | Still depends on `sp42-core`; edge disappears when the facade is retired ([ADR-0013](adr/0013-layered-platform-domain-architecture.md)) | | `sp42-live` | platform | [ADR-0004](adr/0004-crate-boundary-collaboration-model.md), [ADR-0013](adr/0013-layered-platform-domain-architecture.md) | [PRD-0002](../domains/patrolling/prd/0002-patrol-review-workflow.md), [PRD-0004](../domains/patrolling/prd/0004-reviewer-actions-on-wikimedia.md) | | -| `sp42-mcp` | shell | [ADR-0016](adr/0016-wikidata-entity-content-model.md) | [PRD-0001](../domains/references/prd/0001-citation-verification.md), [PRD-0010](../domains/references/prd/0010-citation-verification-mcp-surface.md) | | +| `sp42-mcp` | shell | [ADR-0016](adr/0016-wikidata-entity-content-model.md), [ADR-0018](../domains/references/adr/0018-review-session-bridge-contract.md) | [PRD-0001](../domains/references/prd/0001-citation-verification.md), [PRD-0010](../domains/references/prd/0010-citation-verification-mcp-surface.md), [PRD-0017](../domains/references/prd/0017-interactive-review-sessions.md) | | | `sp42-parsoid` | platform | — | — | | | `sp42-patrol` | domain | [ADR-0013](adr/0013-layered-platform-domain-architecture.md) | — | | -| `sp42-platform` | platform | [ADR-0013](adr/0013-layered-platform-domain-architecture.md), [ADR-0016](adr/0016-wikidata-entity-content-model.md), [ADR-0017](adr/0017-wikidata-statement-proposal-write-contract.md) | — | | +| `sp42-platform` | platform | [ADR-0013](adr/0013-layered-platform-domain-architecture.md), [ADR-0016](adr/0016-wikidata-entity-content-model.md), [ADR-0017](adr/0017-wikidata-statement-proposal-write-contract.md), [ADR-0018](../domains/references/adr/0018-review-session-bridge-contract.md) | [PRD-0017](../domains/references/prd/0017-interactive-review-sessions.md) | | | `sp42-reporting` | platform | [ADR-0004](adr/0004-crate-boundary-collaboration-model.md), [ADR-0008](../domains/references/adr/0008-citation-verification-contract.md), [ADR-0009](../domains/references/adr/0009-citation-source-snapshot-storage.md), [ADR-0013](adr/0013-layered-platform-domain-architecture.md), [ADR-0016](adr/0016-wikidata-entity-content-model.md) | [PRD-0002](../domains/patrolling/prd/0002-patrol-review-workflow.md), [PRD-0016](../domains/assessment/prd/0016-ga-evidence-appendix-renderer.md) | | -| `sp42-server` | shell | [ADR-0003](adr/0003-node-anchored-wikitext-editing.md), [ADR-0004](adr/0004-crate-boundary-collaboration-model.md), [ADR-0005](adr/0005-design-system-shared-component-layer.md), [ADR-0008](../domains/references/adr/0008-citation-verification-contract.md), [ADR-0009](../domains/references/adr/0009-citation-source-snapshot-storage.md), [ADR-0013](adr/0013-layered-platform-domain-architecture.md), [ADR-0015](../domains/references/adr/0015-rules-compliant-read-only-fetch-edge.md) | [PRD-0001](../domains/references/prd/0001-citation-verification.md), [PRD-0002](../domains/patrolling/prd/0002-patrol-review-workflow.md), [PRD-0003](../domains/patrolling/prd/0003-edit-scoring-and-queue-ranking.md), [PRD-0004](../domains/patrolling/prd/0004-reviewer-actions-on-wikimedia.md), [PRD-0005](../domains/patrolling/prd/0005-operator-identity-and-session.md), [PRD-0006](../domains/patrolling/prd/0006-multi-operator-coordination.md), [PRD-0008](../domains/references/prd/0008-bare-url-repair.md), [PRD-0014](../domains/references/prd/0014-citation-repair-insertion-browser-surface.md) | | +| `sp42-server` | shell | [ADR-0003](adr/0003-node-anchored-wikitext-editing.md), [ADR-0004](adr/0004-crate-boundary-collaboration-model.md), [ADR-0005](adr/0005-design-system-shared-component-layer.md), [ADR-0008](../domains/references/adr/0008-citation-verification-contract.md), [ADR-0009](../domains/references/adr/0009-citation-source-snapshot-storage.md), [ADR-0013](adr/0013-layered-platform-domain-architecture.md), [ADR-0015](../domains/references/adr/0015-rules-compliant-read-only-fetch-edge.md) | [PRD-0001](../domains/references/prd/0001-citation-verification.md), [PRD-0002](../domains/patrolling/prd/0002-patrol-review-workflow.md), [PRD-0003](../domains/patrolling/prd/0003-edit-scoring-and-queue-ranking.md), [PRD-0004](../domains/patrolling/prd/0004-reviewer-actions-on-wikimedia.md), [PRD-0005](../domains/patrolling/prd/0005-operator-identity-and-session.md), [PRD-0006](../domains/patrolling/prd/0006-multi-operator-coordination.md), [PRD-0008](../domains/references/prd/0008-bare-url-repair.md), [PRD-0014](../domains/references/prd/0014-citation-repair-insertion-browser-surface.md), [PRD-0017](../domains/references/prd/0017-interactive-review-sessions.md) | | | `sp42-types` | platform | [ADR-0004](adr/0004-crate-boundary-collaboration-model.md), [ADR-0005](adr/0005-design-system-shared-component-layer.md), [ADR-0008](../domains/references/adr/0008-citation-verification-contract.md), [ADR-0009](../domains/references/adr/0009-citation-source-snapshot-storage.md), [ADR-0011](../domains/references/adr/0011-article-citation-verification.md), [ADR-0013](adr/0013-layered-platform-domain-architecture.md), [ADR-0015](../domains/references/adr/0015-rules-compliant-read-only-fetch-edge.md), [ADR-0016](adr/0016-wikidata-entity-content-model.md), [ADR-0017](adr/0017-wikidata-statement-proposal-write-contract.md) | — | | | `sp42-ui` | shell | [ADR-0005](adr/0005-design-system-shared-component-layer.md) | — | | | `sp42-wiki` | platform | [ADR-0004](adr/0004-crate-boundary-collaboration-model.md), [ADR-0005](adr/0005-design-system-shared-component-layer.md), [ADR-0013](adr/0013-layered-platform-domain-architecture.md), [ADR-0014](adr/0014-wikimedia-oauth-and-any-project.md) | [PRD-0005](../domains/patrolling/prd/0005-operator-identity-and-session.md) | Still depends on `sp42-core`; edge disappears when the facade is retired ([ADR-0013](adr/0013-layered-platform-domain-architecture.md)) | @@ -190,6 +190,7 @@ Reading notes: | [ADR-0015](../domains/references/adr/0015-rules-compliant-read-only-fetch-edge.md) | Rules-compliant read-only fetch edge | Accepted | references | `sp42-cli`, `sp42-core`, `sp42-fetch`, `sp42-inference`, `sp42-server`, `sp42-types` | | [ADR-0016](adr/0016-wikidata-entity-content-model.md) | Wikidata entity content-model — revision read, `EntityDiff`, and content-model routing | Proposed | platform | `sp42-mcp`, `sp42-platform`, `sp42-reporting`, `sp42-types` | | [ADR-0017](adr/0017-wikidata-statement-proposal-write-contract.md) | Wikidata statement-proposal write contract | Proposed | platform | `sp42-platform`, `sp42-types` | +| [ADR-0018](../domains/references/adr/0018-review-session-bridge-contract.md) | Review-session bridge contract and store placement | Proposed | references | `sp42-cli`, `sp42-mcp`, `sp42-platform` | ## PRD index @@ -210,3 +211,4 @@ Reading notes: | [PRD-0014](../domains/references/prd/0014-citation-repair-insertion-browser-surface.md) | Citation repair and insertion — browser surface | Accepted | references | ADR-0003, ADR-0006, ADR-0010 | | [PRD-0015](../domains/assessment/prd/0015-article-stability-signal.md) | Article stability signal | Discussion | assessment | ADR-0006, ADR-0007, ADR-0009, ADR-0011, ADR-0014 | | [PRD-0016](../domains/assessment/prd/0016-ga-evidence-appendix-renderer.md) | GA evidence appendix renderer | Discussion | assessment | ADR-0003, ADR-0007, ADR-0010, ADR-0011, ADR-0013, ADR-0018 | +| [PRD-0017](../domains/references/prd/0017-interactive-review-sessions.md) | Interactive review sessions — agent↔operator feedback loop on a page | Draft | references | ADR-0018 | From 31f8f12a7d6d89a2f33cab36641ed9233f0acf89 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 11 Jul 2026 22:48:03 +0000 Subject: [PATCH 2/8] feat(review): port the lavish-axi review loop to wiki-page sessions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements PRD-0017 / ADR-0018: an interactive review-session bridge where an agent opens a session on a page (bare title or pasted wiki URL), the operator queues feedback prompts anchored to Parsoid blocks and cite ids, and the agent long-polls the session to collect them. - sp42-platform: pure review_session core — versioned wire contracts, drain-before-ended feedback semantics, the operator-ended reopen gate, next_step loop etiquette, and the article outline built from Parsoid blocks (the agent's anchor map). - sp42-server: /dev/review/* route group over an in-memory session store with per-session Notify wake-ups; bounded poll waits; all POST routes session+CSRF gated; read-only session inventory listed in the operator endpoint manifest. - sp42-cli: review open/poll/queue/reply/end/sessions subcommands over the bridge-bootstrap pattern, with a re-arming poll loop that keeps stdout reserved for the structured response and narrates waiting on stderr. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012jVtT6gPkHFvpiuDLS5rVR --- crates/sp42-cli/src/main.rs | 640 +++++++++++++++++- crates/sp42-platform/src/lib.rs | 9 + crates/sp42-platform/src/review_session.rs | 720 +++++++++++++++++++++ crates/sp42-platform/src/routes.rs | 16 + crates/sp42-server/src/citation_routes.rs | 2 +- crates/sp42-server/src/main.rs | 2 + crates/sp42-server/src/review_routes.rs | 462 +++++++++++++ crates/sp42-server/src/routes.rs | 35 + crates/sp42-server/src/state.rs | 2 + crates/sp42-server/src/tests.rs | 261 ++++++++ docs/platform/CLI.md | 30 + 11 files changed, 2169 insertions(+), 10 deletions(-) create mode 100644 crates/sp42-platform/src/review_session.rs create mode 100644 crates/sp42-server/src/review_routes.rs diff --git a/crates/sp42-cli/src/main.rs b/crates/sp42-cli/src/main.rs index 984a7a21..9b4ddd81 100644 --- a/crates/sp42-cli/src/main.rs +++ b/crates/sp42-cli/src/main.rs @@ -11,10 +11,13 @@ use sp42_core::routes as route_contracts; use sp42_core::{ CitationFinding, CitationVerificationRequest, CitoidMetadata, ClaimContext, DevAuthBootstrapRequest, DevAuthSessionStatus, FetchedSource, GroundingStatus, - PageVerificationReport, PageVerificationRequest, QueuedEdit, SessionActionExecutionRequest, - SessionActionExecutionResponse, SessionActionKind, SystemClock, VerificationOutcome, - VerifyOptions as CoreVerifyOptions, build_dev_auth_bootstrap_request, locate_quote, - locate_quote_fuzzy, parse_dev_auth_status, verify_citation_use_site, + PageVerificationReport, PageVerificationRequest, QueuedEdit, ReviewAckResponse, ReviewAnchor, + ReviewEndRequest, ReviewOpenRequest, ReviewOpenResponse, ReviewPollRequest, ReviewPollResponse, + ReviewPollStatus, ReviewPrompt, ReviewPromptKind, ReviewQueueRequest, ReviewQueueResponse, + ReviewReplyRequest, ReviewSessionSnapshot, ReviewSessionsResponse, + SessionActionExecutionRequest, SessionActionExecutionResponse, SessionActionKind, SystemClock, + VerificationOutcome, VerifyOptions as CoreVerifyOptions, build_dev_auth_bootstrap_request, + locate_quote, locate_quote_fuzzy, parse_dev_auth_status, verify_citation_use_site, }; use sp42_devtools::{ DEV_PREVIEW_SAMPLE_EVENTS, DEV_PREVIEW_WIKI_ID, DevContextOptions, DevWorkbenchOptions, @@ -235,6 +238,8 @@ enum Command { Batch(BatchArgs), /// Preview or apply bare-URL citation repairs (PRD-0008). BareUrl(BareUrlArgs), + /// Interactive review sessions: open a page, wait for operator feedback (PRD-0017). + Review(ReviewArgs), /// Dev / operator queue views; reads the event payload from STDIN. Preview(PreviewArgs), } @@ -335,6 +340,137 @@ struct VerifyPageArgs { fmt: FormatArg, } +#[derive(Debug, Args)] +struct ReviewArgs { + #[command(subcommand)] + action: ReviewAction, +} + +#[derive(Debug, Subcommand)] +enum ReviewAction { + /// Open or resume a review session on a page (bare title or pasted wiki URL). + Open(ReviewOpenArgs), + /// Wait for operator feedback on an open session; re-arms until feedback or end. + Poll(ReviewPollArgs), + /// Queue operator feedback from the command line (dev/test surface). + Queue(ReviewQueueArgs), + /// Send an agent chat reply to the operator surface. + Reply(ReviewReplyArgs), + /// End a session as the agent (a plain reopen stays allowed). + End(ReviewEndArgs), + /// List review sessions on the local server. + Sessions(ReviewSessionsArgs), +} + +#[derive(Debug, Args)] +struct ReviewOpenArgs { + /// Page target: bare title or pasted wiki URL (oldid URLs pin the revision). + target: String, + /// Target wiki id. + #[arg(long, default_value = BARE_URL_DEFAULT_WIKI)] + wiki: String, + /// Revision id. Omit for the latest revision (the server resolves it). + #[arg(long)] + rev: Option, + /// Resume a session the operator explicitly ended (only when they ask). + #[arg(long)] + reopen: bool, + /// Local server base URL. + #[arg(long, default_value = LOCAL_SERVER_BASE_URL)] + bridge_base_url: String, + #[command(flatten)] + fmt: FormatArg, +} + +#[derive(Debug, Args)] +struct ReviewPollArgs { + /// Page target: bare title or pasted wiki URL. + target: String, + /// Target wiki id. + #[arg(long, default_value = BARE_URL_DEFAULT_WIKI)] + wiki: String, + /// Chat reply shown to the operator before the wait starts. + #[arg(long)] + agent_reply: Option, + /// Return after one bounded server wait instead of re-arming until feedback. + #[arg(long)] + once: bool, + /// Local server base URL. + #[arg(long, default_value = LOCAL_SERVER_BASE_URL)] + bridge_base_url: String, + #[command(flatten)] + fmt: FormatArg, +} + +#[derive(Debug, Args)] +struct ReviewQueueArgs { + /// Page target: bare title or pasted wiki URL. + target: String, + /// The feedback prompt to queue. + #[arg(long)] + message: String, + /// Target wiki id. + #[arg(long, default_value = BARE_URL_DEFAULT_WIKI)] + wiki: String, + /// Anchor the prompt to this block ordinal. + #[arg(long)] + block: Option, + /// Anchor the prompt to this cite id (e.g. cite_ref-x_2-0). + #[arg(long)] + ref_id: Option, + /// Anchor the prompt to this verbatim selected text. + #[arg(long)] + selected_text: Option, + /// Queue and end the session in one action ("send & end"). + #[arg(long)] + end: bool, + /// Local server base URL. + #[arg(long, default_value = LOCAL_SERVER_BASE_URL)] + bridge_base_url: String, + #[command(flatten)] + fmt: FormatArg, +} + +#[derive(Debug, Args)] +struct ReviewReplyArgs { + /// Page target: bare title or pasted wiki URL. + target: String, + /// The reply text. + #[arg(long)] + message: String, + /// Target wiki id. + #[arg(long, default_value = BARE_URL_DEFAULT_WIKI)] + wiki: String, + /// Local server base URL. + #[arg(long, default_value = LOCAL_SERVER_BASE_URL)] + bridge_base_url: String, + #[command(flatten)] + fmt: FormatArg, +} + +#[derive(Debug, Args)] +struct ReviewEndArgs { + /// Page target: bare title or pasted wiki URL. + target: String, + /// Target wiki id. + #[arg(long, default_value = BARE_URL_DEFAULT_WIKI)] + wiki: String, + /// Local server base URL. + #[arg(long, default_value = LOCAL_SERVER_BASE_URL)] + bridge_base_url: String, + #[command(flatten)] + fmt: FormatArg, +} + +#[derive(Debug, Args)] +struct ReviewSessionsArgs { + /// Local server base URL. + #[arg(long, default_value = LOCAL_SERVER_BASE_URL)] + bridge_base_url: String, + #[command(flatten)] + fmt: FormatArg, +} + #[derive(Debug, Args)] struct LocateProbeArgs { /// The quote to locate within the STDIN source body. @@ -703,6 +839,7 @@ fn dispatch(command: Command) -> Result { models: args.models, }), Command::BareUrl(args) => dispatch_bare_url(args.action), + Command::Review(args) => dispatch_review(args.action), Command::Preview(args) => dispatch_preview(args), } } @@ -3126,6 +3263,372 @@ async fn execute_bare_url_via_bridge( }) } +/// An authenticated bridge session: the reqwest client plus the cookie and +/// CSRF token every gated review request must carry (ADR-0002). +struct BridgeSession { + client: reqwest::Client, + cookie: String, + csrf_token: String, +} + +/// Bootstrap the local dev-auth bridge and capture the session cookie and +/// CSRF token — the shared preamble of every gated bridge call. +async fn bootstrap_bridge_session(base_url: &str) -> Result { + let client = reqwest::Client::builder() + .user_agent(sp42_core::branding::USER_AGENT) + .build() + .map_err(|error| format!("bridge client failed to build: {error}"))?; + let bootstrap_request = + build_dev_auth_bootstrap_request(base_url, &DevAuthBootstrapRequest::default()) + .map_err(|error| error.to_string())?; + let bootstrap_response = execute_local_http_request(&client, bootstrap_request).await?; + let bootstrap = + parse_dev_auth_status(&bootstrap_response.body).map_err(|error| error.to_string())?; + if !bootstrap.authenticated { + return Err("bridge bootstrap did not produce an authenticated session".to_string()); + } + let cookie = session_cookie_from_headers(&bootstrap_response.headers) + .ok_or_else(|| "bridge bootstrap did not set a session cookie".to_string())?; + let csrf_token = bootstrap + .csrf_token + .ok_or_else(|| "bridge bootstrap did not return a CSRF token".to_string())?; + Ok(BridgeSession { + client, + cookie, + csrf_token, + }) +} + +/// POST a JSON body to a gated review route and parse the JSON response. +async fn post_review_route( + bridge: &BridgeSession, + base_url: &str, + path: &str, + request: &Request, +) -> Result +where + Request: serde::Serialize, + Response: serde::de::DeserializeOwned, +{ + let response = bridge + .client + .post(format!("{base_url}{path}")) + .header(COOKIE, bridge.cookie.as_str()) + .header( + route_contracts::CSRF_HEADER_NAME, + bridge.csrf_token.as_str(), + ) + .json(request) + .send() + .await + .map_err(|error| format!("review request to {path} failed: {error}"))?; + let status = response.status(); + if !status.is_success() { + // Keep the server's JSON detail: refusals like the operator-ended + // reopen gate carry their etiquette guidance in the body. + let body = response.text().await.unwrap_or_default(); + return Err(format!( + "review request to {path} failed: HTTP {status}: {body}" + )); + } + response + .json::() + .await + .map_err(|error| format!("review response from {path} was invalid: {error}")) +} + +/// Route a `review` subcommand to its runner on a fresh Tokio runtime +/// (`reqwest` needs a reactor; mirrors `render_verify_page`). +fn dispatch_review(action: ReviewAction) -> Result { + let runtime = tokio::runtime::Runtime::new() + .map_err(|error| format!("failed to start async runtime: {error}"))?; + match action { + ReviewAction::Open(args) => { + let format = args.fmt.format; + let response = runtime.block_on(run_review_open(&args))?; + render_review_output(format, &response, render_review_open_text(&response)) + } + ReviewAction::Poll(args) => { + let format = args.fmt.format; + let response = runtime.block_on(run_review_poll(&args))?; + render_review_output(format, &response, render_review_poll_text(&response)) + } + ReviewAction::Queue(args) => { + let format = args.fmt.format; + let response = runtime.block_on(run_review_queue(&args))?; + let text = format!( + "queued {} prompt(s)\n{}", + response.queued, + render_review_snapshot_line(&response.session) + ); + render_review_output(format, &response, text) + } + ReviewAction::Reply(args) => { + let format = args.fmt.format; + let response = runtime.block_on(run_review_reply(&args))?; + let text = format!( + "reply delivered\n{}", + render_review_snapshot_line(&response.session) + ); + render_review_output(format, &response, text) + } + ReviewAction::End(args) => { + let format = args.fmt.format; + let response = runtime.block_on(run_review_end(&args))?; + let text = format!( + "session ended by agent\n{}", + render_review_snapshot_line(&response.session) + ); + render_review_output(format, &response, text) + } + ReviewAction::Sessions(args) => { + let format = args.fmt.format; + let response = runtime.block_on(run_review_sessions(&args))?; + render_review_output(format, &response, render_review_sessions_text(&response)) + } + } +} + +async fn run_review_open(args: &ReviewOpenArgs) -> Result { + let bridge = bootstrap_bridge_session(&args.bridge_base_url).await?; + let request = ReviewOpenRequest { + wiki_id: args.wiki.clone(), + target: args.target.clone(), + rev_id: args.rev.unwrap_or(0), + reopen: args.reopen, + }; + post_review_route( + &bridge, + &args.bridge_base_url, + route_contracts::DEV_REVIEW_OPEN_PATH, + &request, + ) + .await +} + +/// Wait for operator feedback. Each request is a bounded server-side wait; +/// the loop re-arms until feedback, an end, or a missing session, so from +/// the agent's side this behaves like one long poll. Waiting narration goes +/// to stderr; stdout stays reserved for the final structured response. +async fn run_review_poll(args: &ReviewPollArgs) -> Result { + let bridge = bootstrap_bridge_session(&args.bridge_base_url).await?; + let title = sp42_core::parse_page_target(&args.target).title; + if let Some(reply) = &args.agent_reply { + let request = ReviewReplyRequest { + wiki_id: args.wiki.clone(), + title: title.clone(), + text: reply.clone(), + }; + let _: ReviewAckResponse = post_review_route( + &bridge, + &args.bridge_base_url, + route_contracts::DEV_REVIEW_REPLY_PATH, + &request, + ) + .await?; + } + let request = ReviewPollRequest { + wiki_id: args.wiki.clone(), + title: title.clone(), + wait_ms: 0, + }; + loop { + let response: ReviewPollResponse = post_review_route( + &bridge, + &args.bridge_base_url, + route_contracts::DEV_REVIEW_POLL_PATH, + &request, + ) + .await?; + if args.once || response.status != ReviewPollStatus::Waiting { + return Ok(response); + } + eprintln!( + "review poll: still waiting for operator feedback on {title} \ + (re-arming; queued feedback is never lost)" + ); + } +} + +/// Build the queue prompt from CLI anchor flags: `--selected-text` makes a +/// text prompt, `--block`/`--ref-id` a block prompt, neither a free-form +/// message. Anchored prompts require `--block` so the anchor is complete. +fn build_review_queue_prompt(args: &ReviewQueueArgs) -> Result { + let anchored = args.ref_id.is_some() || args.selected_text.is_some(); + let Some(block_ordinal) = args.block else { + if anchored { + return Err("--ref-id/--selected-text need --block to anchor the prompt".to_string()); + } + return Ok(ReviewPrompt { + kind: ReviewPromptKind::Message, + prompt: args.message.clone(), + anchor: None, + }); + }; + let kind = if args.selected_text.is_some() { + ReviewPromptKind::Text + } else { + ReviewPromptKind::Block + }; + Ok(ReviewPrompt { + kind, + prompt: args.message.clone(), + anchor: Some(ReviewAnchor { + block_ordinal, + ref_id: args.ref_id.clone(), + selected_text: args.selected_text.clone(), + }), + }) +} + +async fn run_review_queue(args: &ReviewQueueArgs) -> Result { + let bridge = bootstrap_bridge_session(&args.bridge_base_url).await?; + let request = ReviewQueueRequest { + wiki_id: args.wiki.clone(), + title: sp42_core::parse_page_target(&args.target).title, + prompts: vec![build_review_queue_prompt(args)?], + end_session: args.end, + }; + post_review_route( + &bridge, + &args.bridge_base_url, + route_contracts::DEV_REVIEW_PROMPTS_PATH, + &request, + ) + .await +} + +async fn run_review_reply(args: &ReviewReplyArgs) -> Result { + let bridge = bootstrap_bridge_session(&args.bridge_base_url).await?; + let request = ReviewReplyRequest { + wiki_id: args.wiki.clone(), + title: sp42_core::parse_page_target(&args.target).title, + text: args.message.clone(), + }; + post_review_route( + &bridge, + &args.bridge_base_url, + route_contracts::DEV_REVIEW_REPLY_PATH, + &request, + ) + .await +} + +async fn run_review_end(args: &ReviewEndArgs) -> Result { + let bridge = bootstrap_bridge_session(&args.bridge_base_url).await?; + let request = ReviewEndRequest { + wiki_id: args.wiki.clone(), + title: sp42_core::parse_page_target(&args.target).title, + }; + post_review_route( + &bridge, + &args.bridge_base_url, + route_contracts::DEV_REVIEW_END_PATH, + &request, + ) + .await +} + +async fn run_review_sessions(args: &ReviewSessionsArgs) -> Result { + let client = reqwest::Client::builder() + .user_agent(sp42_core::branding::USER_AGENT) + .build() + .map_err(|error| format!("bridge client failed to build: {error}"))?; + let url = format!( + "{}{}", + args.bridge_base_url, + route_contracts::DEV_REVIEW_SESSIONS_PATH + ); + let response = client + .get(&url) + .send() + .await + .map_err(|error| format!("review sessions request failed: {error}"))? + .error_for_status() + .map_err(|error| format!("review sessions request failed: {error}"))?; + response + .json::() + .await + .map_err(|error| format!("review sessions payload was invalid: {error}")) +} + +/// Shared review output shaping: JSON is the machine contract; text and +/// markdown share the same line-oriented rendering. +fn render_review_output( + format: OutputFormat, + value: &T, + text: String, +) -> Result { + match format { + OutputFormat::Json => { + serde_json::to_string_pretty(value).map_err(|error| error.to_string()) + } + OutputFormat::Text => Ok(text), + OutputFormat::Markdown => Ok(format!("## Review session\n\n```\n{text}\n```")), + } +} + +fn render_review_snapshot_line(session: &ReviewSessionSnapshot) -> String { + let ended = session.ended_by.map_or(String::new(), |by| { + format!(", ended by {by:?}").to_lowercase() + }); + format!( + "session: {} ({}) rev {} — {:?}, {} pending prompt(s){}", + session.title, + session.wiki_id, + session.rev_id, + session.status, + session.pending_prompts, + ended + ) +} + +fn render_review_open_text(response: &ReviewOpenResponse) -> String { + format!( + "{}\noutline: {} block(s)\nnext: {}", + render_review_snapshot_line(&response.session), + response.outline.len(), + response.next_step + ) +} + +fn render_review_prompt_line(prompt: &ReviewPrompt) -> String { + let anchor = prompt.anchor.as_ref().map_or(String::new(), |anchor| { + let ref_part = anchor + .ref_id + .as_deref() + .map_or(String::new(), |ref_id| format!(", ref {ref_id}")); + let text_part = anchor + .selected_text + .as_deref() + .map_or(String::new(), |text| format!(", text \"{text}\"")); + format!(" (block {}{ref_part}{text_part})", anchor.block_ordinal) + }); + format!("- [{:?}] {}{anchor}", prompt.kind, prompt.prompt).to_string() +} + +fn render_review_poll_text(response: &ReviewPollResponse) -> String { + let mut lines = vec![format!("status: {:?}", response.status).to_lowercase()]; + lines.extend(response.prompts.iter().map(render_review_prompt_line)); + if let Some(ended_by) = response.ended_by { + lines.push(format!("ended by: {ended_by:?}").to_lowercase()); + } + lines.push(format!("next: {}", response.next_step)); + lines.join("\n") +} + +fn render_review_sessions_text(response: &ReviewSessionsResponse) -> String { + if response.sessions.is_empty() { + return "no review sessions".to_string(); + } + response + .sessions + .iter() + .map(render_review_snapshot_line) + .collect::>() + .join("\n") +} + async fn execute_local_http_request( client: &reqwest::Client, request: HttpRequest, @@ -3466,12 +3969,14 @@ mod tests { use super::{ ActionKind, BareUrlAction, BareUrlCliMode, BareUrlCliOptions, BareUrlExecuteReport, Cli, - CliOptions, Command, ContextPreviewOptions, LOCAL_SERVER_BASE_URL, OutputFormat, ShellMode, - VerifyPageCliOptions, WorkbenchOptions, render_action_preview, render_backlog_preview, + CliOptions, Command, ContextPreviewOptions, FormatArg, LOCAL_SERVER_BASE_URL, OutputFormat, + ReviewAction, ReviewAnchor, ReviewPollResponse, ReviewPollStatus, ReviewPrompt, + ReviewPromptKind, ReviewQueueArgs, ShellMode, VerifyPageCliOptions, WorkbenchOptions, + build_review_queue_prompt, render_action_preview, render_backlog_preview, render_bare_url_execute, render_bare_url_proposals, render_context_preview, - render_coordination_preview, render_parity_report, render_queue, render_scenario_report, - render_session_digest, render_stream_preview, render_workbench, select_bare_url_proposal, - server_report_lines, + render_coordination_preview, render_parity_report, render_queue, render_review_poll_text, + render_scenario_report, render_session_digest, render_stream_preview, render_workbench, + select_bare_url_proposal, server_report_lines, }; use clap::Parser; use serde_json::json; @@ -4450,4 +4955,121 @@ mod tests { assert!(error.contains("no bare-URL proposal for ordinal 1")); assert!(error.contains("declined: []")); } + + #[test] + fn review_open_args_parse_target_wiki_rev_and_reopen() { + let cli = Cli::parse_from([ + "sp42-cli", + "review", + "open", + "https://en.wikipedia.org/wiki/Example", + "--wiki", + "enwiki", + "--rev", + "5", + "--reopen", + ]); + let Command::Review(args) = cli.command else { + panic!("expected review command"); + }; + let ReviewAction::Open(open) = args.action else { + panic!("expected open action"); + }; + assert_eq!(open.target, "https://en.wikipedia.org/wiki/Example"); + assert_eq!(open.wiki, "enwiki"); + assert_eq!(open.rev, Some(5)); + assert!(open.reopen); + assert_eq!(open.bridge_base_url, LOCAL_SERVER_BASE_URL); + } + + #[test] + fn review_queue_prompt_builds_each_anchor_shape() { + let base = ReviewQueueArgs { + target: "Exemple".to_string(), + message: "tighten this".to_string(), + wiki: "frwiki".to_string(), + block: None, + ref_id: None, + selected_text: None, + end: false, + bridge_base_url: LOCAL_SERVER_BASE_URL.to_string(), + fmt: FormatArg { + format: OutputFormat::Text, + }, + }; + + let message = build_review_queue_prompt(&base).expect("message prompt should build"); + assert_eq!(message.kind, ReviewPromptKind::Message); + assert!(message.anchor.is_none()); + + let block = build_review_queue_prompt(&ReviewQueueArgs { + block: Some(3), + ref_id: Some("cite_ref-a_1-0".to_string()), + ..clone_review_queue_args(&base) + }) + .expect("block prompt should build"); + assert_eq!(block.kind, ReviewPromptKind::Block); + let anchor = block.anchor.expect("block prompt should carry an anchor"); + assert_eq!(anchor.block_ordinal, 3); + assert_eq!(anchor.ref_id.as_deref(), Some("cite_ref-a_1-0")); + + let text = build_review_queue_prompt(&ReviewQueueArgs { + block: Some(2), + selected_text: Some("the quote".to_string()), + ..clone_review_queue_args(&base) + }) + .expect("text prompt should build"); + assert_eq!(text.kind, ReviewPromptKind::Text); + + let missing_block = build_review_queue_prompt(&ReviewQueueArgs { + ref_id: Some("cite_ref-a_1-0".to_string()), + ..clone_review_queue_args(&base) + }) + .expect_err("an anchored prompt without --block must refuse"); + assert!(missing_block.contains("--block")); + } + + fn clone_review_queue_args(args: &ReviewQueueArgs) -> ReviewQueueArgs { + ReviewQueueArgs { + target: args.target.clone(), + message: args.message.clone(), + wiki: args.wiki.clone(), + block: args.block, + ref_id: args.ref_id.clone(), + selected_text: args.selected_text.clone(), + end: args.end, + bridge_base_url: args.bridge_base_url.clone(), + fmt: FormatArg { + format: OutputFormat::Text, + }, + } + } + + #[test] + fn review_poll_text_lists_prompts_and_next_step() { + let response = ReviewPollResponse { + contract_version: 1, + status: ReviewPollStatus::Feedback, + prompts: vec![ReviewPrompt { + kind: ReviewPromptKind::Text, + prompt: "check this quote".to_string(), + anchor: Some(ReviewAnchor { + block_ordinal: 3, + ref_id: Some("cite_ref-x_2-0".to_string()), + selected_text: Some("the quote".to_string()), + }), + }], + session_ended: false, + ended_by: None, + next_step: "apply the feedback".to_string(), + }; + + let text = render_review_poll_text(&response); + + assert!(text.contains("status: feedback")); + assert!(text.contains("check this quote")); + assert!(text.contains("block 3")); + assert!(text.contains("cite_ref-x_2-0")); + assert!(text.contains("next: apply the feedback")); + } } diff --git a/crates/sp42-platform/src/lib.rs b/crates/sp42-platform/src/lib.rs index d2620b2d..fdb21cc8 100644 --- a/crates/sp42-platform/src/lib.rs +++ b/crates/sp42-platform/src/lib.rs @@ -48,6 +48,7 @@ pub mod origin; pub mod priority_queue; pub mod public_documents; pub mod queue_builder; +pub mod review_session; pub mod review_workbench; pub mod routes; pub mod score_tier; @@ -126,6 +127,14 @@ pub use public_documents::{ pub use queue_builder::{ build_ranked_queue, build_ranked_queue_with_contexts, build_ranked_queue_with_policy, }; +pub use review_session::{ + REVIEW_SESSION_CONTRACT_VERSION, ReviewAckResponse, ReviewAnchor, ReviewBlockOutline, + ReviewChatEntry, ReviewChatRole, ReviewEndRequest, ReviewEndedBy, ReviewFeedbackTake, + ReviewOpenRequest, ReviewOpenResponse, ReviewPollRequest, ReviewPollResponse, ReviewPollStatus, + ReviewPrompt, ReviewPromptKind, ReviewQueueRequest, ReviewQueueResponse, ReviewReplyRequest, + ReviewSession, ReviewSessionSnapshot, ReviewSessionStatus, ReviewSessionsResponse, + build_article_outline, open_next_step, poll_next_step, poll_status, +}; pub use review_workbench::{ PreparedRequestPreview, ReviewWorkbench, build_review_workbench, build_session_action_execution_requests, diff --git a/crates/sp42-platform/src/review_session.rs b/crates/sp42-platform/src/review_session.rs new file mode 100644 index 00000000..33381d61 --- /dev/null +++ b/crates/sp42-platform/src/review_session.rs @@ -0,0 +1,720 @@ +//! Interactive review-session contracts (PRD-0017). +//! +//! An agent opens a review session on a wiki page, the operator queues +//! anchored feedback prompts against the rendered article, and the agent +//! polls the session to collect that feedback. The loop is ported from the +//! local-first artifact-review pattern popularized by `lavish-axi` +//! (), re-keyed from local file +//! paths to `(wiki_id, title)` targets. +//! +//! This module is the pure core: session state transitions, the +//! drain-before-ended feedback semantics, the operator-ended reopen gate, +//! and the `next_step` guidance strings. The imperative edges — route glue, +//! waiting, notification — live in `sp42-server`; the agent loop lives in +//! `sp42-cli`. + +use serde::{Deserialize, Serialize}; + +use crate::wikitext_editor::{BlockKind, ParsoidBlock}; + +/// Version tag embedded in every review-session response so agents can +/// detect contract drift (Constitution Art. 9.1). +pub const REVIEW_SESSION_CONTRACT_VERSION: u32 = 1; + +/// Outline text is truncated to this many characters so a full-article +/// outline stays a compact, token-friendly snapshot. +pub const REVIEW_OUTLINE_TEXT_LIMIT: usize = 160; + +/// Who closed a review session. Mirrors the `ended_by` etiquette of the +/// ported loop: an operator-ended session refuses a plain reopen, while an +/// agent-ended session may be reopened freely. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum ReviewEndedBy { + Operator, + Agent, +} + +/// Lifecycle of a review session. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum ReviewSessionStatus { + /// Open with no undelivered feedback. + Open, + /// Open with queued prompts awaiting an agent poll. + Feedback, + /// Closed; `ended_by` records who closed it. + Ended, +} + +/// What a queued prompt is anchored to. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum ReviewPromptKind { + /// Anchored to one article block (paragraph, list item, table cell). + Block, + /// Anchored to selected text inside a block. + Text, + /// Free-form message with no article anchor. + Message, +} + +/// Anchor tying a prompt to article structure. Block ordinals and cite ids +/// come from the Parsoid block decomposition, so anchors survive rendering +/// differences that CSS selectors would not. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct ReviewAnchor { + /// Document-order index of the anchored block. + pub block_ordinal: usize, + /// Stable cite id (e.g. `"cite_ref-smith_3-0"`) when the prompt targets + /// a specific reference in the block. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub ref_id: Option, + /// Verbatim selected text when the prompt targets a text range. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub selected_text: Option, +} + +/// One operator feedback item queued for the agent. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct ReviewPrompt { + pub kind: ReviewPromptKind, + /// The operator's instruction or question. + pub prompt: String, + /// Article anchor; `None` for [`ReviewPromptKind::Message`]. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub anchor: Option, +} + +/// Who wrote a chat entry. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum ReviewChatRole { + Operator, + Agent, +} + +/// One line of the session's operator/agent conversation. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct ReviewChatEntry { + pub role: ReviewChatRole, + pub text: String, + pub at_ms: i64, +} + +/// Pure state of one review session. The server wraps this in its shared +/// store; every transition is a method here so the semantics stay testable +/// without I/O. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct ReviewSession { + pub wiki_id: String, + /// Namespace-qualified page title (spaces, not underscores). + pub title: String, + /// Revision the session was opened against (`0` = latest at open time). + pub rev_id: u64, + pub status: ReviewSessionStatus, + /// Prompts queued since the last successful poll delivery. + pub queued_prompts: Vec, + pub chat: Vec, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub ended_by: Option, + pub created_at_ms: i64, + pub updated_at_ms: i64, +} + +/// Result of draining a session's queued feedback. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum ReviewFeedbackTake { + /// Nothing queued and the session is open. + Waiting, + /// Queued prompts, delivered exactly once. `session_ended` is `true` + /// when this is the final batch of a session that has already closed — + /// queued feedback always delivers before the `Ended` status does. + Feedback { + prompts: Vec, + session_ended: bool, + ended_by: Option, + }, + /// Session closed with nothing left to deliver. + Ended { ended_by: ReviewEndedBy }, +} + +/// Refusal reason when an open request hits the reopen gate. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct ReopenRefused; + +impl ReviewSession { + /// A fresh open session on `(wiki_id, title)` at `rev_id`. + #[must_use] + pub fn open(wiki_id: &str, title: &str, rev_id: u64, now_ms: i64) -> Self { + Self { + wiki_id: wiki_id.to_string(), + title: title.to_string(), + rev_id, + status: ReviewSessionStatus::Open, + queued_prompts: Vec::new(), + chat: Vec::new(), + ended_by: None, + created_at_ms: now_ms, + updated_at_ms: now_ms, + } + } + + /// Canonical store key for a target. The pair is the identity — the + /// analog of the ported loop's canonical-file-path session key. + #[must_use] + pub fn canonical_key(wiki_id: &str, title: &str) -> String { + format!("{wiki_id}\u{1f}{title}") + } + + /// Gate a reopen attempt: a session the *operator* ended refuses a plain + /// reopen so the agent does not reopen the review surface uninvited; an + /// agent-ended (or still-open) session resumes freely. + /// + /// # Errors + /// + /// [`ReopenRefused`] when the operator ended the session and the caller + /// did not explicitly request a reopen. + pub fn gate_reopen(&self, reopen_requested: bool) -> Result<(), ReopenRefused> { + if self.ended_by == Some(ReviewEndedBy::Operator) && !reopen_requested { + return Err(ReopenRefused); + } + Ok(()) + } + + /// Resume an ended session as open again (after [`Self::gate_reopen`]). + pub fn resume(&mut self, rev_id: u64, now_ms: i64) { + self.status = if self.queued_prompts.is_empty() { + ReviewSessionStatus::Open + } else { + ReviewSessionStatus::Feedback + }; + self.ended_by = None; + self.rev_id = rev_id; + self.updated_at_ms = now_ms; + } + + /// Queue operator prompts; optionally close the session in the same + /// action ("send & end"). Queued prompts survive the end and deliver on + /// the next poll. + pub fn queue_prompts(&mut self, prompts: Vec, end_session: bool, now_ms: i64) { + self.queued_prompts.extend(prompts); + if end_session { + self.status = ReviewSessionStatus::Ended; + self.ended_by = Some(ReviewEndedBy::Operator); + } else if !self.queued_prompts.is_empty() { + self.status = ReviewSessionStatus::Feedback; + } + self.updated_at_ms = now_ms; + } + + /// Drain queued feedback with deliver-before-ended semantics: prompts + /// queued before an end still deliver first (marked `session_ended`); + /// only a later poll reports `Ended`. + pub fn take_feedback(&mut self, now_ms: i64) -> ReviewFeedbackTake { + if !self.queued_prompts.is_empty() { + let prompts = std::mem::take(&mut self.queued_prompts); + let session_ended = self.status == ReviewSessionStatus::Ended; + if !session_ended { + self.status = ReviewSessionStatus::Open; + } + self.updated_at_ms = now_ms; + return ReviewFeedbackTake::Feedback { + prompts, + session_ended, + ended_by: self.ended_by, + }; + } + match self.ended_by { + Some(ended_by) if self.status == ReviewSessionStatus::Ended => { + ReviewFeedbackTake::Ended { ended_by } + } + _ => ReviewFeedbackTake::Waiting, + } + } + + /// Append an agent chat reply (shown on the operator surface). + pub fn agent_reply(&mut self, text: &str, now_ms: i64) { + self.chat.push(ReviewChatEntry { + role: ReviewChatRole::Agent, + text: text.to_string(), + at_ms: now_ms, + }); + self.updated_at_ms = now_ms; + } + + /// Close the session, recording who closed it. + pub fn end(&mut self, by: ReviewEndedBy, now_ms: i64) { + self.status = ReviewSessionStatus::Ended; + self.ended_by = Some(by); + self.updated_at_ms = now_ms; + } + + /// Wire-facing snapshot of this session. + #[must_use] + pub fn snapshot(&self) -> ReviewSessionSnapshot { + ReviewSessionSnapshot { + wiki_id: self.wiki_id.clone(), + title: self.title.clone(), + rev_id: self.rev_id, + status: self.status, + pending_prompts: self.queued_prompts.len(), + ended_by: self.ended_by, + updated_at_ms: self.updated_at_ms, + } + } +} + +/// Wire-facing summary of one session for listings and command output. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct ReviewSessionSnapshot { + pub wiki_id: String, + pub title: String, + pub rev_id: u64, + pub status: ReviewSessionStatus, + pub pending_prompts: usize, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub ended_by: Option, + pub updated_at_ms: i64, +} + +/// One block of the article outline — the compact structure snapshot an +/// agent uses to resolve prompt anchors (the analog of the ported loop's +/// DOM snapshot, built from Parsoid blocks instead of live DOM). +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct ReviewBlockOutline { + pub block_ordinal: usize, + pub kind: String, + /// Block text truncated to [`REVIEW_OUTLINE_TEXT_LIMIT`] characters. + pub text: String, + /// Stable cite ids of the block's inline references, in order. + pub ref_ids: Vec, +} + +/// Build the outline for a decomposed page. +#[must_use] +pub fn build_article_outline(blocks: &[ParsoidBlock]) -> Vec { + blocks + .iter() + .map(|block| ReviewBlockOutline { + block_ordinal: block.block_ordinal, + kind: block_kind_label(block.block_kind).to_string(), + text: truncate_chars(&block.text, REVIEW_OUTLINE_TEXT_LIMIT), + ref_ids: block + .refs + .iter() + .map(|block_ref| block_ref.ref_id.clone()) + .collect(), + }) + .collect() +} + +const fn block_kind_label(kind: BlockKind) -> &'static str { + match kind { + BlockKind::Paragraph => "paragraph", + BlockKind::ListItem => "list-item", + BlockKind::TableCell => "table-cell", + BlockKind::Other => "other", + } +} + +/// Truncate on a character boundary, appending an ellipsis when shortened. +fn truncate_chars(text: &str, limit: usize) -> String { + if text.chars().count() <= limit { + return text.to_string(); + } + let mut shortened: String = text.chars().take(limit).collect(); + shortened.push('…'); + shortened +} + +/// Open (or resume) a review session on a page target. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct ReviewOpenRequest { + pub wiki_id: String, + /// Bare title or pasted wiki URL; the server unwraps URLs the same way + /// verify-page does. + pub target: String, + /// Pinned revision; `0` means latest. + #[serde(default)] + pub rev_id: u64, + /// Required to resume a session the operator explicitly ended. + #[serde(default)] + pub reopen: bool, +} + +/// Open/resume response: session snapshot plus the article outline. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct ReviewOpenResponse { + pub contract_version: u32, + pub session: ReviewSessionSnapshot, + pub outline: Vec, + pub next_step: String, +} + +/// Operator queues prompts (the browser surface's send action). +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct ReviewQueueRequest { + pub wiki_id: String, + pub title: String, + pub prompts: Vec, + /// "Send & end": queue and close in one action. + #[serde(default)] + pub end_session: bool, +} + +/// Queue acknowledgement. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct ReviewQueueResponse { + pub contract_version: u32, + pub session: ReviewSessionSnapshot, + pub queued: usize, +} + +/// Agent polls for feedback, optionally bounding the server-side wait. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct ReviewPollRequest { + pub wiki_id: String, + pub title: String, + /// Server-side wait bound in milliseconds; `0` returns immediately. + /// The server clamps this to its own maximum. + #[serde(default)] + pub wait_ms: u64, +} + +/// Poll outcome status on the wire. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum ReviewPollStatus { + Waiting, + Feedback, + Ended, + Missing, +} + +/// Poll response. `next_step` carries the loop etiquette so agents do not +/// have to invent it: keep polling on `waiting`, apply feedback then poll +/// again with a reply on `feedback`, and stop (without reopening uninvited +/// when the operator ended) on `ended`. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct ReviewPollResponse { + pub contract_version: u32, + pub status: ReviewPollStatus, + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub prompts: Vec, + #[serde(default)] + pub session_ended: bool, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub ended_by: Option, + pub next_step: String, +} + +/// Agent chat reply shown on the operator surface. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct ReviewReplyRequest { + pub wiki_id: String, + pub title: String, + pub text: String, +} + +/// Agent-initiated session end. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct ReviewEndRequest { + pub wiki_id: String, + pub title: String, +} + +/// Generic acknowledgement for reply/end. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct ReviewAckResponse { + pub contract_version: u32, + pub session: ReviewSessionSnapshot, +} + +/// Session listing. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct ReviewSessionsResponse { + pub contract_version: u32, + pub sessions: Vec, +} + +/// `next_step` for a fresh or resumed open. +#[must_use] +pub fn open_next_step(title: &str) -> String { + format!( + "Session open on \"{title}\". Ask the operator to review it in the SP42 browser shell, \ + then run `sp42-cli review poll` and wait for their feedback before responding further." + ) +} + +/// `next_step` for a poll outcome. +#[must_use] +pub fn poll_next_step(take: &ReviewFeedbackTake, title: &str) -> String { + match take { + ReviewFeedbackTake::Waiting => format!( + "No feedback on \"{title}\" yet. Poll again (the wait is safe to re-run; queued \ + feedback is never lost)." + ), + ReviewFeedbackTake::Feedback { + session_ended: false, + .. + } => format!( + "Apply the operator's feedback on \"{title}\", then poll again with an agent reply \ + summarizing what changed." + ), + ReviewFeedbackTake::Feedback { + session_ended: true, + .. + } => format!( + "Final feedback batch: the operator ended the review of \"{title}\". Apply the \ + feedback and deliver remaining updates in chat; do not reopen unless asked." + ), + ReviewFeedbackTake::Ended { + ended_by: ReviewEndedBy::Operator, + } => format!( + "The operator ended the review of \"{title}\". Stop polling and do not reopen \ + uninvited; deliver any remaining updates in chat." + ), + ReviewFeedbackTake::Ended { + ended_by: ReviewEndedBy::Agent, + } => format!( + "Review of \"{title}\" was ended by the agent. Stop polling; open a new session if \ + further review is needed." + ), + } +} + +/// The wire status for a drained take. +#[must_use] +pub fn poll_status(take: &ReviewFeedbackTake) -> ReviewPollStatus { + match take { + ReviewFeedbackTake::Waiting => ReviewPollStatus::Waiting, + ReviewFeedbackTake::Feedback { .. } => ReviewPollStatus::Feedback, + ReviewFeedbackTake::Ended { .. } => ReviewPollStatus::Ended, + } +} + +#[cfg(test)] +mod tests { + use super::{ + REVIEW_OUTLINE_TEXT_LIMIT, ReopenRefused, ReviewEndedBy, ReviewFeedbackTake, ReviewPrompt, + ReviewPromptKind, ReviewSession, ReviewSessionStatus, build_article_outline, + poll_next_step, poll_status, + }; + use crate::wikitext_editor::{BlockKind, BlockRef, ParsoidBlock}; + + fn message_prompt(text: &str) -> ReviewPrompt { + ReviewPrompt { + kind: ReviewPromptKind::Message, + prompt: text.to_string(), + anchor: None, + } + } + + #[test] + fn queueing_prompts_moves_the_session_to_feedback() { + let mut session = ReviewSession::open("frwiki", "Exemple", 42, 1_000); + + session.queue_prompts(vec![message_prompt("tighten the lede")], false, 2_000); + + assert_eq!(session.status, ReviewSessionStatus::Feedback); + assert_eq!(session.queued_prompts.len(), 1); + assert_eq!(session.updated_at_ms, 2_000); + } + + #[test] + fn take_feedback_drains_once_and_reopens() { + let mut session = ReviewSession::open("frwiki", "Exemple", 42, 1_000); + session.queue_prompts(vec![message_prompt("first")], false, 2_000); + + let take = session.take_feedback(3_000); + let ReviewFeedbackTake::Feedback { + prompts, + session_ended, + .. + } = take + else { + panic!("expected feedback"); + }; + assert_eq!(prompts.len(), 1); + assert!(!session_ended); + assert_eq!(session.status, ReviewSessionStatus::Open); + assert_eq!(session.take_feedback(4_000), ReviewFeedbackTake::Waiting); + } + + #[test] + fn feedback_queued_before_an_end_delivers_before_ended() { + let mut session = ReviewSession::open("frwiki", "Exemple", 42, 1_000); + session.queue_prompts(vec![message_prompt("last words")], true, 2_000); + + let take = session.take_feedback(3_000); + let ReviewFeedbackTake::Feedback { + session_ended, + ended_by, + .. + } = take + else { + panic!("expected the final feedback batch"); + }; + assert!(session_ended); + assert_eq!(ended_by, Some(ReviewEndedBy::Operator)); + + assert_eq!( + session.take_feedback(4_000), + ReviewFeedbackTake::Ended { + ended_by: ReviewEndedBy::Operator + } + ); + } + + #[test] + fn operator_end_gates_a_plain_reopen_but_agent_end_does_not() { + let mut operator_ended = ReviewSession::open("frwiki", "Exemple", 42, 1_000); + operator_ended.end(ReviewEndedBy::Operator, 2_000); + assert_eq!(operator_ended.gate_reopen(false), Err(ReopenRefused)); + assert_eq!(operator_ended.gate_reopen(true), Ok(())); + + let mut agent_ended = ReviewSession::open("frwiki", "Exemple", 42, 1_000); + agent_ended.end(ReviewEndedBy::Agent, 2_000); + assert_eq!(agent_ended.gate_reopen(false), Ok(())); + } + + #[test] + fn resume_clears_the_end_and_keeps_queued_feedback() { + let mut session = ReviewSession::open("frwiki", "Exemple", 42, 1_000); + session.queue_prompts(vec![message_prompt("pending")], true, 2_000); + + session.resume(43, 3_000); + + assert_eq!(session.status, ReviewSessionStatus::Feedback); + assert_eq!(session.ended_by, None); + assert_eq!(session.rev_id, 43); + assert_eq!(session.queued_prompts.len(), 1); + } + + #[test] + fn canonical_key_separates_wiki_and_title_unambiguously() { + assert_ne!( + ReviewSession::canonical_key("a", "b:c"), + ReviewSession::canonical_key("a:b", "c") + ); + } + + #[test] + fn outline_truncates_on_character_boundaries() { + let long_text = "é".repeat(REVIEW_OUTLINE_TEXT_LIMIT + 5); + let blocks = vec![ParsoidBlock { + text: long_text, + refs: vec![BlockRef { + offset: 3, + ref_id: "cite_ref-a_1-0".to_string(), + sources: Vec::new(), + ref_text: "[1]".to_string(), + named: false, + is_bare_url_ref: false, + }], + block_kind: BlockKind::Paragraph, + block_ordinal: 7, + }]; + + let outline = build_article_outline(&blocks); + + assert_eq!(outline.len(), 1); + assert_eq!(outline[0].block_ordinal, 7); + assert_eq!(outline[0].kind, "paragraph"); + assert_eq!( + outline[0].text.chars().count(), + REVIEW_OUTLINE_TEXT_LIMIT + 1, + "limit characters plus the ellipsis" + ); + assert!(outline[0].text.ends_with('…')); + assert_eq!(outline[0].ref_ids, vec!["cite_ref-a_1-0".to_string()]); + } + + #[test] + fn outline_passes_short_text_through_untruncated() { + let blocks = vec![ParsoidBlock { + text: "short".to_string(), + refs: Vec::new(), + block_kind: BlockKind::ListItem, + block_ordinal: 0, + }]; + + let outline = build_article_outline(&blocks); + + assert_eq!(outline[0].text, "short"); + assert_eq!(outline[0].kind, "list-item"); + } + + #[test] + fn next_step_matches_the_loop_etiquette() { + let waiting = poll_next_step(&ReviewFeedbackTake::Waiting, "Exemple"); + assert!(waiting.contains("Poll again")); + + let operator_ended = poll_next_step( + &ReviewFeedbackTake::Ended { + ended_by: ReviewEndedBy::Operator, + }, + "Exemple", + ); + assert!(operator_ended.contains("do not reopen")); + + let agent_ended = poll_next_step( + &ReviewFeedbackTake::Ended { + ended_by: ReviewEndedBy::Agent, + }, + "Exemple", + ); + assert!(agent_ended.contains("new session")); + } + + #[test] + fn poll_status_maps_every_take() { + use super::ReviewPollStatus; + assert_eq!( + poll_status(&ReviewFeedbackTake::Waiting), + ReviewPollStatus::Waiting + ); + assert_eq!( + poll_status(&ReviewFeedbackTake::Feedback { + prompts: Vec::new(), + session_ended: false, + ended_by: None, + }), + ReviewPollStatus::Feedback + ); + assert_eq!( + poll_status(&ReviewFeedbackTake::Ended { + ended_by: ReviewEndedBy::Agent + }), + ReviewPollStatus::Ended + ); + } + + #[test] + fn wire_types_round_trip_with_the_contract_version() { + let response = super::ReviewPollResponse { + contract_version: super::REVIEW_SESSION_CONTRACT_VERSION, + status: super::ReviewPollStatus::Feedback, + prompts: vec![ReviewPrompt { + kind: ReviewPromptKind::Text, + prompt: "check this quote".to_string(), + anchor: Some(super::ReviewAnchor { + block_ordinal: 3, + ref_id: Some("cite_ref-x_2-0".to_string()), + selected_text: Some("the quote".to_string()), + }), + }], + session_ended: false, + ended_by: None, + next_step: "apply".to_string(), + }; + + let json = serde_json::to_string(&response).expect("poll response should serialize"); + let parsed: super::ReviewPollResponse = + serde_json::from_str(&json).expect("poll response should deserialize"); + + assert_eq!(parsed, response); + assert_eq!(parsed.contract_version, 1); + } +} diff --git a/crates/sp42-platform/src/routes.rs b/crates/sp42-platform/src/routes.rs index fb2abdb1..1b80b215 100644 --- a/crates/sp42-platform/src/routes.rs +++ b/crates/sp42-platform/src/routes.rs @@ -97,6 +97,17 @@ pub const DEV_CITATION_VERIFY_PAGE_PATH: &str = "/dev/citation/verify-page"; /// cost is spent. pub const DEV_CITATION_REVERIFY_PATH: &str = "/dev/citation/reverify"; +/// Interactive review-session bridge (PRD-0017): an agent opens a session on +/// a page, the operator queues anchored feedback, and the agent polls for it. +/// All POST routes are session+CSRF gated; the listing is a read-only +/// inspection surface. +pub const DEV_REVIEW_OPEN_PATH: &str = "/dev/review/open"; +pub const DEV_REVIEW_SESSIONS_PATH: &str = "/dev/review/sessions"; +pub const DEV_REVIEW_PROMPTS_PATH: &str = "/dev/review/prompts"; +pub const DEV_REVIEW_POLL_PATH: &str = "/dev/review/poll"; +pub const DEV_REVIEW_REPLY_PATH: &str = "/dev/review/agent-reply"; +pub const DEV_REVIEW_END_PATH: &str = "/dev/review/end"; + /// Header carrying the bridge session's CSRF token on state-changing routes. pub const CSRF_HEADER_NAME: &str = "x-sp42-csrf-token"; @@ -329,6 +340,11 @@ fn operator_dev_endpoints(default_wiki_id: &str) -> Vec { COORDINATION_INSPECTIONS_PATH, "Room-by-room coordination inspection collection.", ), + RouteDefinition::new( + RouteMethod::Get, + DEV_REVIEW_SESSIONS_PATH, + "Interactive review-session inventory and pending-feedback summaries.", + ), ] } diff --git a/crates/sp42-server/src/citation_routes.rs b/crates/sp42-server/src/citation_routes.rs index 1f627bb1..87910cc3 100644 --- a/crates/sp42-server/src/citation_routes.rs +++ b/crates/sp42-server/src/citation_routes.rs @@ -212,7 +212,7 @@ pub(crate) async fn post_bare_url_proposals( /// Resolve a page's current revision id via the wiki action API (a public read). /// Used when a verify-page request leaves `rev_id` at `0` ("latest"). -async fn fetch_latest_revid( +pub(crate) async fn fetch_latest_revid( client: &(impl HttpClient + ?Sized), config: &sp42_core::WikiConfig, title: &str, diff --git a/crates/sp42-server/src/main.rs b/crates/sp42-server/src/main.rs index 876ae7dd..c0f7dafb 100644 --- a/crates/sp42-server/src/main.rs +++ b/crates/sp42-server/src/main.rs @@ -11,6 +11,7 @@ mod local_env; mod oauth_runtime; mod operator_live; mod parsoid_editor; +mod review_routes; mod revision_artifacts; mod routes; pub(crate) mod runtime_adapters; @@ -322,6 +323,7 @@ async fn main() -> Result<(), std::io::Error> { capability_targets: CapabilityProbeTargets::default(), clock: clock.clone(), coordination: CoordinationRegistry::new(clock), + review_sessions: review_routes::new_review_session_store(), deployment, wiki_registry, wikitext_editor: Arc::new(parsoid_editor::ParsoidWikitextEditor::new()), diff --git a/crates/sp42-server/src/review_routes.rs b/crates/sp42-server/src/review_routes.rs new file mode 100644 index 00000000..1dcce631 --- /dev/null +++ b/crates/sp42-server/src/review_routes.rs @@ -0,0 +1,462 @@ +//! Interactive review-session bridge routes (PRD-0017). +//! +//! FCIS: every session transition (queue, drain-before-ended, reopen gate, +//! end etiquette) is a pure `sp42_core::ReviewSession` method; this module +//! owns the imperative edges — the shared store, the bounded poll wait, the +//! wake-up notification, and the route glue. All POST routes are +//! session+CSRF gated (ADR-0002): the agent side rides the CLI's bridge +//! bootstrap, the operator side rides the browser session cookie. + +use std::collections::HashMap; +use std::sync::Arc; +use std::time::Duration; + +use axum::{ + Json, + extract::State, + http::{HeaderMap, StatusCode}, + response::IntoResponse, +}; +use sp42_core::{ + REVIEW_SESSION_CONTRACT_VERSION, ReviewAckResponse, ReviewBlockOutline, ReviewEndRequest, + ReviewEndedBy, ReviewFeedbackTake, ReviewOpenRequest, ReviewOpenResponse, ReviewPollRequest, + ReviewPollResponse, ReviewPollStatus, ReviewQueueRequest, ReviewQueueResponse, ReviewSession, + ReviewSessionSnapshot, ReviewSessionsResponse, WikitextPageRef, build_article_outline, + open_next_step, poll_next_step, poll_status, +}; +use tokio::sync::{Notify, RwLock}; +use tracing::info; + +use crate::citation_routes::fetch_latest_revid; +use crate::config_for_state_wiki; +use crate::http_errors::unauthorized_error; +use crate::runtime_adapters::BearerHttpClient; +use crate::session_runtime::{current_session_snapshot, validate_csrf_header}; +use crate::state::AppState; + +/// Default server-side wait when a poll request does not bound it. +pub(crate) const POLL_WAIT_DEFAULT_MS: u64 = 25_000; + +/// Upper clamp on the server-side poll wait: long enough that an agent loop +/// is quiet, short enough to stay under common client/proxy request +/// timeouts. The CLI re-arms between waits, so the loop is unbounded even +/// though each request is not. +pub(crate) const POLL_WAIT_MAX_MS: u64 = 55_000; + +/// One stored session plus its poll wake-up channel. `notify_one` stores a +/// permit when no poll is waiting, so feedback queued between an empty drain +/// and the wait registration still wakes the next `notified().await`. +#[derive(Debug)] +pub(crate) struct ReviewSessionEntry { + pub(crate) session: ReviewSession, + pub(crate) notify: Arc, +} + +pub(crate) type SharedReviewSessions = Arc>>; + +/// Fresh empty review-session store for state construction. +pub(crate) fn new_review_session_store() -> SharedReviewSessions { + Arc::new(RwLock::new(HashMap::new())) +} + +type RouteError = (StatusCode, Json); + +/// Require an authenticated bridge session with a valid CSRF header — +/// the shared gate for every review POST route. +async fn require_review_session( + state: &AppState, + headers: &HeaderMap, +) -> Result { + let Some(session) = current_session_snapshot(state, headers, true).await else { + return Err(unauthorized_error( + "No authenticated bridge session is active.", + )); + }; + validate_csrf_header(headers, &session)?; + Ok(session) +} + +fn missing_session_error(wiki_id: &str, title: &str) -> RouteError { + ( + StatusCode::NOT_FOUND, + Json(serde_json::json!({ + "error": format!("no review session is open for {title} on {wiki_id}"), + "code": "review-session-not-found", + })), + ) +} + +/// Resolve the open target: unwrap pasted URLs, pin `rev_id == 0` to the +/// current revision (so the outline and the session record agree on what +/// was reviewed), and decompose the page into outline blocks. +async fn resolve_open_target( + state: &AppState, + session: &crate::state::SessionSnapshot, + payload: &ReviewOpenRequest, +) -> Result<(String, u64, Vec), RouteError> { + let config = config_for_state_wiki(state, &payload.wiki_id)?; + let target = sp42_core::parse_page_target(&payload.target); + let mut rev_id = if payload.rev_id == 0 { + target.rev_id + } else { + payload.rev_id + }; + if rev_id == 0 { + let wiki_client = + BearerHttpClient::new(state.http_client.clone(), session.access_token.clone()); + rev_id = fetch_latest_revid(&wiki_client, &config, &target.title) + .await + .map_err(|error| crate::action_routes::action_error_response(&error))?; + } + let page_ref = WikitextPageRef { + title: target.title.clone(), + rev_id, + }; + let blocks = state + .wikitext_editor + .extract_blocks(&config, &page_ref) + .await + .map_err(|error| { + crate::action_routes::action_error_response( + &crate::action_routes::action_error_from_editor(&error), + ) + })?; + Ok((target.title, rev_id, build_article_outline(&blocks))) +} + +/// Open a new session or resume an existing one, honoring the reopen gate: +/// a session the operator explicitly ended refuses a plain reopen. +fn open_or_resume( + entry: Option<&mut ReviewSessionEntry>, + reopen_requested: bool, + title: &str, + rev_id: u64, + now_ms: i64, +) -> Result, RouteError> { + let Some(entry) = entry else { + return Ok(None); + }; + if entry.session.gate_reopen(reopen_requested).is_err() { + return Err(( + StatusCode::CONFLICT, + Json(serde_json::json!({ + "error": format!( + "the operator ended the review of {title}; do not reopen uninvited — \ + pass reopen=true only when the operator asks for further review" + ), + "code": "review-session-operator-ended", + })), + )); + } + entry.session.resume(rev_id, now_ms); + Ok(Some(entry.session.snapshot())) +} + +/// `POST /dev/review/open` — open or resume a review session on a page. +/// +/// Session + CSRF gated: the route mutates server state and (for a latest- +/// revision open) reads through the caller's wiki identity. +pub(crate) async fn post_review_open( + State(state): State, + headers: HeaderMap, + Json(payload): Json, +) -> Result { + let session = require_review_session(&state, &headers).await?; + let (title, rev_id, outline) = resolve_open_target(&state, &session, &payload).await?; + let key = ReviewSession::canonical_key(&payload.wiki_id, &title); + let now_ms = state.clock.now_ms(); + + let mut store = state.review_sessions.write().await; + let resumed = open_or_resume(store.get_mut(&key), payload.reopen, &title, rev_id, now_ms)?; + let session = if let Some(snapshot) = resumed { + snapshot + } else { + let session = ReviewSession::open(&payload.wiki_id, &title, rev_id, now_ms); + let snapshot = session.snapshot(); + store.insert( + key, + ReviewSessionEntry { + session, + notify: Arc::new(Notify::new()), + }, + ); + snapshot + }; + info!( + wiki_id = %session.wiki_id, + title = %session.title, + rev_id = session.rev_id, + "review session opened" + ); + Ok(Json(ReviewOpenResponse { + contract_version: REVIEW_SESSION_CONTRACT_VERSION, + next_step: open_next_step(&session.title), + session, + outline, + })) +} + +/// `GET /dev/review/sessions` — read-only session inventory. +pub(crate) async fn get_review_sessions(State(state): State) -> impl IntoResponse { + let store = state.review_sessions.read().await; + let mut sessions: Vec = store + .values() + .map(|entry| entry.session.snapshot()) + .collect(); + sessions.sort_by_key(|session| std::cmp::Reverse(session.updated_at_ms)); + Json(ReviewSessionsResponse { + contract_version: REVIEW_SESSION_CONTRACT_VERSION, + sessions, + }) +} + +/// `POST /dev/review/prompts` — the operator queues feedback (optionally +/// ending the session in the same action) and wakes any waiting poll. +pub(crate) async fn post_review_prompts( + State(state): State, + headers: HeaderMap, + Json(payload): Json, +) -> Result { + require_review_session(&state, &headers).await?; + let key = ReviewSession::canonical_key(&payload.wiki_id, &payload.title); + let now_ms = state.clock.now_ms(); + + let mut store = state.review_sessions.write().await; + let Some(entry) = store.get_mut(&key) else { + return Err(missing_session_error(&payload.wiki_id, &payload.title)); + }; + let queued = payload.prompts.len(); + entry + .session + .queue_prompts(payload.prompts, payload.end_session, now_ms); + entry.notify.notify_one(); + info!( + wiki_id = %payload.wiki_id, + title = %payload.title, + queued, + end_session = payload.end_session, + "review feedback queued" + ); + Ok(Json(ReviewQueueResponse { + contract_version: REVIEW_SESSION_CONTRACT_VERSION, + session: entry.session.snapshot(), + queued, + })) +} + +/// Drain one session's feedback; `None` when the session does not exist. +async fn drain_feedback( + state: &AppState, + key: &str, +) -> Option<(ReviewFeedbackTake, Arc, String)> { + let mut store = state.review_sessions.write().await; + let entry = store.get_mut(key)?; + let take = entry.session.take_feedback(state.clock.now_ms()); + Some((take, entry.notify.clone(), entry.session.title.clone())) +} + +fn poll_response(take: ReviewFeedbackTake, title: &str) -> ReviewPollResponse { + let status = poll_status(&take); + let next_step = poll_next_step(&take, title); + let (prompts, session_ended, ended_by) = match take { + ReviewFeedbackTake::Waiting => (Vec::new(), false, None), + ReviewFeedbackTake::Feedback { + prompts, + session_ended, + ended_by, + } => (prompts, session_ended, ended_by), + ReviewFeedbackTake::Ended { ended_by } => (Vec::new(), true, Some(ended_by)), + }; + ReviewPollResponse { + contract_version: REVIEW_SESSION_CONTRACT_VERSION, + status, + prompts, + session_ended, + ended_by, + next_step, + } +} + +fn missing_poll_response() -> ReviewPollResponse { + ReviewPollResponse { + contract_version: REVIEW_SESSION_CONTRACT_VERSION, + status: ReviewPollStatus::Missing, + prompts: Vec::new(), + session_ended: false, + ended_by: None, + next_step: "No review session is open for this page. Run `sp42-cli review open` first." + .to_string(), + } +} + +/// `POST /dev/review/poll` — the agent's bounded feedback wait. +/// +/// Drains immediately when feedback is queued; otherwise waits (clamped to +/// [`POLL_WAIT_MAX_MS`]) for a queue/end wake-up and drains once more. The +/// wake-up uses `notify_one` permits, so feedback queued while no poll is +/// waiting is picked up by the next poll without racing. +pub(crate) async fn post_review_poll( + State(state): State, + headers: HeaderMap, + Json(payload): Json, +) -> Result { + require_review_session(&state, &headers).await?; + let key = ReviewSession::canonical_key(&payload.wiki_id, &payload.title); + let Some((take, notify, title)) = drain_feedback(&state, &key).await else { + return Ok(Json(missing_poll_response())); + }; + + let wait_ms = if payload.wait_ms == 0 { + POLL_WAIT_DEFAULT_MS + } else { + payload.wait_ms.min(POLL_WAIT_MAX_MS) + }; + let take = if take == ReviewFeedbackTake::Waiting { + let _ = tokio::time::timeout(Duration::from_millis(wait_ms), notify.notified()).await; + match drain_feedback(&state, &key).await { + Some((take, _, _)) => take, + None => return Ok(Json(missing_poll_response())), + } + } else { + take + }; + + if !matches!(take, ReviewFeedbackTake::Waiting) { + info!( + wiki_id = %payload.wiki_id, + title = %title, + status = ?poll_status(&take), + "review poll delivered" + ); + } + Ok(Json(poll_response(take, &title))) +} + +/// `POST /dev/review/agent-reply` — the agent's chat message to the +/// operator surface. +pub(crate) async fn post_review_reply( + State(state): State, + headers: HeaderMap, + Json(payload): Json, +) -> Result { + require_review_session(&state, &headers).await?; + let key = ReviewSession::canonical_key(&payload.wiki_id, &payload.title); + let now_ms = state.clock.now_ms(); + let mut store = state.review_sessions.write().await; + let Some(entry) = store.get_mut(&key) else { + return Err(missing_session_error(&payload.wiki_id, &payload.title)); + }; + entry.session.agent_reply(&payload.text, now_ms); + Ok(Json(ReviewAckResponse { + contract_version: REVIEW_SESSION_CONTRACT_VERSION, + session: entry.session.snapshot(), + })) +} + +/// `POST /dev/review/end` — agent-initiated end. Unlike an operator end, a +/// plain reopen stays allowed afterwards. +pub(crate) async fn post_review_end( + State(state): State, + headers: HeaderMap, + Json(payload): Json, +) -> Result { + require_review_session(&state, &headers).await?; + let key = ReviewSession::canonical_key(&payload.wiki_id, &payload.title); + let now_ms = state.clock.now_ms(); + let mut store = state.review_sessions.write().await; + let Some(entry) = store.get_mut(&key) else { + return Err(missing_session_error(&payload.wiki_id, &payload.title)); + }; + entry.session.end(ReviewEndedBy::Agent, now_ms); + entry.notify.notify_one(); + info!( + wiki_id = %payload.wiki_id, + title = %payload.title, + "review session ended by agent" + ); + Ok(Json(ReviewAckResponse { + contract_version: REVIEW_SESSION_CONTRACT_VERSION, + session: entry.session.snapshot(), + })) +} + +#[cfg(test)] +mod tests { + use std::sync::Arc; + use std::time::Duration; + + use sp42_core::{ReviewFeedbackTake, ReviewPrompt, ReviewPromptKind, ReviewSession}; + use tokio::sync::Notify; + + use super::{ReviewSessionEntry, new_review_session_store}; + + fn message_prompt(text: &str) -> ReviewPrompt { + ReviewPrompt { + kind: ReviewPromptKind::Message, + prompt: text.to_string(), + anchor: None, + } + } + + #[tokio::test] + async fn queued_feedback_wakes_a_waiting_poll() { + let store = new_review_session_store(); + let key = ReviewSession::canonical_key("frwiki", "Exemple"); + store.write().await.insert( + key.clone(), + ReviewSessionEntry { + session: ReviewSession::open("frwiki", "Exemple", 42, 1_000), + notify: Arc::new(Notify::new()), + }, + ); + + let waiter_store = store.clone(); + let waiter_key = key.clone(); + let waiter = tokio::spawn(async move { + let notify = waiter_store + .read() + .await + .get(&waiter_key) + .expect("session should exist") + .notify + .clone(); + tokio::time::timeout(Duration::from_secs(5), notify.notified()) + .await + .expect("queueing should wake the waiter"); + let mut sessions = waiter_store.write().await; + let entry = sessions + .get_mut(&waiter_key) + .expect("session should still exist"); + entry.session.take_feedback(2_000) + }); + + // Let the waiter register before queueing. + tokio::time::sleep(Duration::from_millis(50)).await; + { + let mut sessions = store.write().await; + let entry = sessions.get_mut(&key).expect("session should exist"); + entry + .session + .queue_prompts(vec![message_prompt("wake up")], false, 1_500); + entry.notify.notify_one(); + } + + let take = waiter.await.expect("waiter should finish"); + let ReviewFeedbackTake::Feedback { prompts, .. } = take else { + panic!("expected delivered feedback"); + }; + assert_eq!(prompts.len(), 1); + assert_eq!(prompts[0].prompt, "wake up"); + } + + #[tokio::test] + async fn a_permit_covers_feedback_queued_before_the_wait_registers() { + let notify = Arc::new(Notify::new()); + // Feedback arrives while no poll is waiting: the permit is stored... + notify.notify_one(); + // ...and the next wait completes immediately instead of blocking. + tokio::time::timeout(Duration::from_millis(100), notify.notified()) + .await + .expect("stored permit should complete the wait immediately"); + } +} diff --git a/crates/sp42-server/src/routes.rs b/crates/sp42-server/src/routes.rs index 25be5fb7..ad7ceae6 100644 --- a/crates/sp42-server/src/routes.rs +++ b/crates/sp42-server/src/routes.rs @@ -18,6 +18,10 @@ use crate::citation_routes::{ post_bare_url_apply, post_bare_url_proposals, post_citation_reverify, post_verify_page, }; use crate::operator_live::get_live_operator_view; +use crate::review_routes::{ + get_review_sessions, post_review_end, post_review_open, post_review_poll, post_review_prompts, + post_review_reply, +}; use crate::revision_artifacts::{ get_rendered_hunk_preview, get_revision_diff, get_revision_media_diff, }; @@ -84,9 +88,40 @@ fn operator_routes(router: Router) -> Router { let router = operator_api_routes(router); let router = operator_storage_routes(router); let router = dev_bridge_routes(router); + let router = review_session_routes(router); static_asset_routes(router) } +/// Interactive review-session bridge (PRD-0017): agent opens/polls, the +/// operator queues feedback, both through the same session-gated bridge. +fn review_session_routes(router: Router) -> Router { + router + .route( + route_contracts::DEV_REVIEW_OPEN_PATH, + axum::routing::post(post_review_open), + ) + .route( + route_contracts::DEV_REVIEW_SESSIONS_PATH, + get(get_review_sessions), + ) + .route( + route_contracts::DEV_REVIEW_PROMPTS_PATH, + axum::routing::post(post_review_prompts), + ) + .route( + route_contracts::DEV_REVIEW_POLL_PATH, + axum::routing::post(post_review_poll), + ) + .route( + route_contracts::DEV_REVIEW_REPLY_PATH, + axum::routing::post(post_review_reply), + ) + .route( + route_contracts::DEV_REVIEW_END_PATH, + axum::routing::post(post_review_end), + ) +} + fn coordination_routes(router: Router) -> Router { router .route( diff --git a/crates/sp42-server/src/state.rs b/crates/sp42-server/src/state.rs index 90629894..0d8c85e6 100644 --- a/crates/sp42-server/src/state.rs +++ b/crates/sp42-server/src/state.rs @@ -13,6 +13,7 @@ use crate::coordination::CoordinationRegistry; use crate::deployment::DeploymentConfig; use crate::live_queue::IngestionSupervisorSnapshot; use crate::local_env::LocalOAuthConfig; +use crate::review_routes::SharedReviewSessions; use crate::revision_artifacts::{CachedRenderedHunkPreview, CachedRevisionArtifacts}; use crate::wikimedia_capabilities::CapabilityProbeTargets; @@ -38,6 +39,7 @@ pub(crate) struct AppState { pub(crate) capability_targets: CapabilityProbeTargets, pub(crate) clock: Arc, pub(crate) coordination: CoordinationRegistry, + pub(crate) review_sessions: SharedReviewSessions, pub(crate) deployment: DeploymentConfig, pub(crate) wiki_registry: WikiRegistry, pub(crate) wikitext_editor: Arc, diff --git a/crates/sp42-server/src/tests.rs b/crates/sp42-server/src/tests.rs index 7476a307..39f42721 100644 --- a/crates/sp42-server/src/tests.rs +++ b/crates/sp42-server/src/tests.rs @@ -100,6 +100,7 @@ fn test_state() -> AppState { capability_targets: CapabilityProbeTargets::default(), clock: clock.clone(), coordination: CoordinationRegistry::new(clock), + review_sessions: crate::review_routes::new_review_session_store(), deployment: test_deployment(), wiki_registry: test_wiki_registry(), wikitext_editor: test_wikitext_editor(), @@ -1177,6 +1178,7 @@ async fn healthz_reports_ready_when_local_token_is_loaded() { }, clock: clock.clone(), coordination: CoordinationRegistry::new(clock), + review_sessions: crate::review_routes::new_review_session_store(), deployment: test_deployment(), wiki_registry: test_wiki_registry(), wikitext_editor: test_wikitext_editor(), @@ -1686,6 +1688,7 @@ async fn capability_route_uses_injected_targets() { }, clock: clock.clone(), coordination: CoordinationRegistry::new(clock), + review_sessions: crate::review_routes::new_review_session_store(), deployment: test_deployment(), wiki_registry: test_wiki_registry(), wikitext_editor: test_wikitext_editor(), @@ -1754,6 +1757,7 @@ async fn live_operator_route_returns_canonical_operator_contract() { }, clock: clock.clone(), coordination: CoordinationRegistry::new(clock), + review_sessions: crate::review_routes::new_review_session_store(), deployment: test_deployment(), wiki_registry: test_wiki_registry(), wikitext_editor: test_wikitext_editor(), @@ -1845,6 +1849,7 @@ async fn live_operator_route_surfaces_cached_backlog_state() { }, clock: clock.clone(), coordination: CoordinationRegistry::new(clock), + review_sessions: crate::review_routes::new_review_session_store(), deployment: test_deployment(), wiki_registry: test_wiki_registry(), wikitext_editor: test_wikitext_editor(), @@ -2021,6 +2026,7 @@ async fn logical_storage_document_route_resolves_profile_page() { }, clock: clock.clone(), coordination: CoordinationRegistry::new(clock), + review_sessions: crate::review_routes::new_review_session_store(), deployment: test_deployment(), wiki_registry: test_wiki_registry(), wikitext_editor: test_wikitext_editor(), @@ -2104,6 +2110,7 @@ async fn public_storage_document_route_returns_typed_preferences() { }, clock: clock.clone(), coordination: CoordinationRegistry::new(clock), + review_sessions: crate::review_routes::new_review_session_store(), deployment: test_deployment(), wiki_registry: test_wiki_registry(), wikitext_editor: test_wikitext_editor(), @@ -2193,6 +2200,7 @@ async fn bootstrap_derives_username_and_scopes_from_validated_token() { }, clock: clock.clone(), coordination: CoordinationRegistry::new(clock), + review_sessions: crate::review_routes::new_review_session_store(), deployment: test_deployment(), wiki_registry: test_wiki_registry(), wikitext_editor: test_wikitext_editor(), @@ -3997,3 +4005,256 @@ fn action_error_response_preserves_carried_status() { axum::http::StatusCode::BAD_GATEWAY ); } + +/// POST a JSON body to a review route with the bridge cookie + CSRF header +/// and return the status plus parsed JSON body. +async fn post_review_json( + router: Router, + cookie: &str, + path: &str, + body: &serde_json::Value, +) -> (StatusCode, serde_json::Value) { + let response = router + .oneshot( + Request::builder() + .method(Method::POST) + .uri(path) + .header(axum::http::header::COOKIE, cookie) + .header(CSRF_HEADER_NAME, "csrf-token") + .header(axum::http::header::CONTENT_TYPE, "application/json") + .body(Body::from(body.to_string())) + .expect("request should build"), + ) + .await + .expect("review request should succeed"); + let status = response.status(); + let bytes = to_bytes(response.into_body(), usize::MAX) + .await + .expect("review response body should read"); + let value = serde_json::from_slice(&bytes).unwrap_or(serde_json::Value::Null); + (status, value) +} + +#[tokio::test] +async fn review_routes_require_an_authenticated_bridge_session() { + let state = test_state(); + let router = build_router(state); + + let response = router + .oneshot( + Request::builder() + .method(Method::POST) + .uri(sp42_core::routes::DEV_REVIEW_POLL_PATH) + .header(axum::http::header::CONTENT_TYPE, "application/json") + .body(Body::from( + serde_json::json!({"wiki_id": "frwiki", "title": "Exemple", "wait_ms": 1}) + .to_string(), + )) + .expect("request should build"), + ) + .await + .expect("poll request should succeed"); + + assert_eq!(response.status(), StatusCode::UNAUTHORIZED); +} + +/// A router with an installed bridge session, ready for review requests. +async fn review_test_router() -> (Router, String) { + let state = test_state(); + let created_at_ms = now_ms(); + state.sessions.write().await.insert( + "review-agent".to_string(), + test_session("Reviewer", "secret-token", created_at_ms), + ); + ( + build_router(state), + format!("{SESSION_COOKIE_NAME}=review-agent"), + ) +} + +#[tokio::test] +async fn review_session_loop_delivers_operator_feedback_to_the_agent() { + let (router, cookie) = review_test_router().await; + + // Open on a pasted URL: the title unwraps and the pinned rev is recorded. + let (status, open) = post_review_json( + router.clone(), + &cookie, + sp42_core::routes::DEV_REVIEW_OPEN_PATH, + &serde_json::json!({ + "wiki_id": "frwiki", + "target": "https://fr.wikipedia.org/wiki/Exemple", + "rev_id": 42, + }), + ) + .await; + assert_eq!(status, StatusCode::OK); + assert_eq!(open["session"]["title"], "Exemple"); + assert_eq!(open["session"]["rev_id"], 42); + assert_eq!(open["session"]["status"], "open"); + assert_eq!(open["contract_version"], 1); + + // No feedback yet: a bounded poll reports waiting. + let poll_body = serde_json::json!({"wiki_id": "frwiki", "title": "Exemple", "wait_ms": 1}); + let (status, poll) = post_review_json( + router.clone(), + &cookie, + sp42_core::routes::DEV_REVIEW_POLL_PATH, + &poll_body, + ) + .await; + assert_eq!(status, StatusCode::OK); + assert_eq!(poll["status"], "waiting"); + + // The operator queues an anchored prompt and ends the session in one action. + let (status, queued) = post_review_json( + router.clone(), + &cookie, + sp42_core::routes::DEV_REVIEW_PROMPTS_PATH, + &serde_json::json!({ + "wiki_id": "frwiki", + "title": "Exemple", + "prompts": [{ + "kind": "text", + "prompt": "this quote is not in the source", + "anchor": {"block_ordinal": 3, "ref_id": "cite_ref-a_1-0", "selected_text": "quote"}, + }], + "end_session": true, + }), + ) + .await; + assert_eq!(status, StatusCode::OK); + assert_eq!(queued["queued"], 1); + + // Feedback queued before the end still delivers, flagged as the final batch. + let (status, poll) = post_review_json( + router.clone(), + &cookie, + sp42_core::routes::DEV_REVIEW_POLL_PATH, + &poll_body, + ) + .await; + assert_eq!(status, StatusCode::OK); + assert_eq!(poll["status"], "feedback"); + assert_eq!(poll["session_ended"], true); + assert_eq!(poll["ended_by"], "operator"); + assert_eq!(poll["prompts"][0]["anchor"]["block_ordinal"], 3); + + // A later poll reports the operator end with reopen etiquette. + let (status, poll) = post_review_json( + router.clone(), + &cookie, + sp42_core::routes::DEV_REVIEW_POLL_PATH, + &poll_body, + ) + .await; + assert_eq!(status, StatusCode::OK); + assert_eq!(poll["status"], "ended"); + assert!( + poll["next_step"] + .as_str() + .expect("next_step should be a string") + .contains("do not reopen"), + ); +} + +#[tokio::test] +async fn review_reopen_gate_requires_explicit_reopen_after_operator_end() { + let (router, cookie) = review_test_router().await; + + // Open, then let the operator end the session with no further feedback. + let (status, _) = post_review_json( + router.clone(), + &cookie, + sp42_core::routes::DEV_REVIEW_OPEN_PATH, + &serde_json::json!({"wiki_id": "frwiki", "target": "Exemple", "rev_id": 42}), + ) + .await; + assert_eq!(status, StatusCode::OK); + let (status, _) = post_review_json( + router.clone(), + &cookie, + sp42_core::routes::DEV_REVIEW_PROMPTS_PATH, + &serde_json::json!({ + "wiki_id": "frwiki", + "title": "Exemple", + "prompts": [], + "end_session": true, + }), + ) + .await; + assert_eq!(status, StatusCode::OK); + + // A plain reopen refuses; an explicit reopen resumes. + let open_body = serde_json::json!({ + "wiki_id": "frwiki", + "target": "Exemple", + "rev_id": 42, + }); + let (status, refused) = post_review_json( + router.clone(), + &cookie, + sp42_core::routes::DEV_REVIEW_OPEN_PATH, + &open_body, + ) + .await; + assert_eq!(status, StatusCode::CONFLICT); + assert_eq!(refused["code"], "review-session-operator-ended"); + + let (status, reopened) = post_review_json( + router.clone(), + &cookie, + sp42_core::routes::DEV_REVIEW_OPEN_PATH, + &serde_json::json!({ + "wiki_id": "frwiki", + "target": "Exemple", + "rev_id": 42, + "reopen": true, + }), + ) + .await; + assert_eq!(status, StatusCode::OK); + assert_eq!(reopened["session"]["status"], "open"); + + // The listing shows the resumed session. + let response = router + .oneshot( + Request::builder() + .method(Method::GET) + .uri(sp42_core::routes::DEV_REVIEW_SESSIONS_PATH) + .body(Body::empty()) + .expect("request should build"), + ) + .await + .expect("sessions request should succeed"); + assert_eq!(response.status(), StatusCode::OK); + let bytes = to_bytes(response.into_body(), usize::MAX) + .await + .expect("sessions body should read"); + let sessions: serde_json::Value = + serde_json::from_slice(&bytes).expect("sessions body should parse"); + assert_eq!(sessions["sessions"][0]["title"], "Exemple"); +} + +#[tokio::test] +async fn review_poll_reports_missing_for_an_unopened_page() { + let state = test_state(); + let created_at_ms = now_ms(); + state.sessions.write().await.insert( + "review-agent".to_string(), + test_session("Reviewer", "secret-token", created_at_ms), + ); + let router = build_router(state); + let cookie = format!("{SESSION_COOKIE_NAME}=review-agent"); + + let (status, poll) = post_review_json( + router, + &cookie, + sp42_core::routes::DEV_REVIEW_POLL_PATH, + &serde_json::json!({"wiki_id": "frwiki", "title": "Nowhere", "wait_ms": 1}), + ) + .await; + + assert_eq!(status, StatusCode::OK); + assert_eq!(poll["status"], "missing"); +} diff --git a/docs/platform/CLI.md b/docs/platform/CLI.md index 3732e8cf..541c52b6 100644 --- a/docs/platform/CLI.md +++ b/docs/platform/CLI.md @@ -32,6 +32,7 @@ quick one. | `batch` | Verify a JSONL batch (one case per line), one result line out. | JSONL cases (or `--file`) | | `locate-probe` | Offline check whether a quote locates in a source body. | source body | | `bare-url preview` / `bare-url execute` | Preview or apply bare-URL repairs. | no | +| `review ` | Interactive review sessions: open a page, wait for operator feedback. | no | | `preview [mode]` | Dev / operator queue views; default is the ranked queue. | event payload | Output goes to **stdout** on success (exit `0`); errors print to **stderr** @@ -188,6 +189,35 @@ sp42-cli bare-url execute --title "Museum" --rev 12345 --ordinal 0 | `--action-note ` | Edit summary attached to the applied repair. | | `--bridge-base-url ` | Local server base URL. | +## `review` — interactive review sessions (PRD-0017) + +Agent↔operator feedback loop on a page: the agent opens a session on a page +target (bare title or pasted wiki URL, including `oldid` URLs), the operator +queues anchored feedback prompts, and the agent polls until they arrive. All +actions ride the local bridge session (bootstrap → cookie + CSRF). + +```sh +sp42-cli review open "https://fr.wikipedia.org/wiki/Exemple" --wiki frwiki +sp42-cli review poll "Exemple" --wiki frwiki --agent-reply "opened — start with the lede" +sp42-cli review queue "Exemple" --wiki frwiki --message "check this ref" --block 3 --ref-id cite_ref-a_1-0 +sp42-cli review end "Exemple" --wiki frwiki +sp42-cli review sessions +``` + +| Action | What it does | +| --- | --- | +| `open ` | Open or resume a session; returns the article outline plus `next_step`. `--rev ` pins a revision (default: latest), `--reopen` resumes a session the operator explicitly ended. | +| `poll ` | Wait for feedback; re-arms bounded server waits until feedback, an end, or a missing session (stderr narrates the wait). `--agent-reply ` posts a chat line first, `--once` returns after a single wait. | +| `queue ` | Queue one operator prompt (dev/test surface): `--message ` plus optional anchors `--block `, `--ref-id `, `--selected-text `; `--end` sends and ends in one action. | +| `reply ` | Send an agent chat reply (`--message `). | +| `end ` | End the session as the agent; a plain reopen stays allowed. | +| `sessions` | List review sessions on the local server. | + +All actions accept `--wiki ` (default `testwiki`), `--bridge-base-url +`, and `--format `. An operator-ended session refuses a plain +`open` with `review-session-operator-ended`; pass `--reopen` only when the +operator asks for further review. + ## `preview` — dev / operator views Consumes an event payload from STDIN (or a built-in sample when STDIN is empty) From c2845b00a295b1f3c955b95867830e95953197dc Mon Sep 17 00:00:00 2001 From: Luis Villa Date: Mon, 13 Jul 2026 18:20:38 -0700 Subject: [PATCH 3/8] feat(review): overlay verify-page findings on the review outline The review session becomes the in-article frontend for the verification report: `sp42-cli review findings` attaches a verify-page report to the session, its findings project onto the outline blocks by cite id (verdict, truncated claim, grounding/unavailable caveat), and the next open returns the annotated outline instead of leaving the operator with detached report text. Revision-gated (409 on mismatch, stale overlay dropped on re-pin), replace-all semantics, unmatched markers surfaced as unanchored_findings. Promotes PRD-0017 open question 2 into the proposal; ADR-0018 gains the marker-projection decision (platform stays citation-agnostic). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01QB2VfS9GXyTzVw3FayQZxf --- crates/sp42-citation/src/citation/page.rs | 91 ++++++ crates/sp42-citation/src/lib.rs | 4 +- crates/sp42-cli/src/main.rs | 122 +++++++- crates/sp42-platform/src/lib.rs | 8 +- crates/sp42-platform/src/review_session.rs | 266 +++++++++++++++++- crates/sp42-platform/src/routes.rs | 1 + crates/sp42-server/src/review_routes.rs | 71 ++++- crates/sp42-server/src/routes.rs | 8 +- crates/sp42-server/src/tests.rs | 80 ++++++ docs/domains/references/README.md | 2 +- .../0018-review-session-bridge-contract.md | 26 +- .../prd/0017-interactive-review-sessions.md | 43 ++- docs/platform/CLI.md | 3 + docs/platform/architecture.md | 2 +- 14 files changed, 688 insertions(+), 39 deletions(-) diff --git a/crates/sp42-citation/src/citation/page.rs b/crates/sp42-citation/src/citation/page.rs index fa7a03d0..9d00d72c 100644 --- a/crates/sp42-citation/src/citation/page.rs +++ b/crates/sp42-citation/src/citation/page.rs @@ -175,6 +175,52 @@ pub struct PageVerificationReport { pub stats: PageVerificationStats, } +/// Project a page report's findings into review-session overlay markers +/// (PRD-0017): one marker per finding, joined to the article by the +/// finding's `ref_id` — the same anchor coordinate review prompts use — so +/// a review surface can show the report's problems in the context of the +/// article instead of as detached text. Verdicts keep their wire labels; +/// claims are truncated to the outline display limit; grounding caveats and +/// unavailable reasons ride along as `detail`. Findings without a `ref_id` +/// (the standalone single-claim path) cannot anchor and are skipped. +#[must_use] +pub fn review_finding_markers( + report: &PageVerificationReport, +) -> Vec { + report + .findings + .iter() + .filter(|finding| !finding.ref_id.is_empty()) + .map(|finding| sp42_platform::ReviewFindingMarker { + ref_id: finding.ref_id.clone(), + verdict: finding.verdict.as_wire().to_string(), + claim: sp42_platform::truncate_outline_text(&finding.claim), + detail: finding_marker_detail(finding), + }) + .collect() +} + +/// The marker's short qualifier: the unavailable reason for an abstention, +/// or the grounding caveat for a support judgment that did not locate +/// verbatim (SP42#25 layer 6 — surfaced, never silently upgraded). +fn finding_marker_detail(finding: &CitationFinding) -> Option { + if finding.verdict == CitationVerdict::SourceUnavailable { + return finding + .source_unavailable_reason + .map(|reason| format!("source {}", reason.as_str())); + } + match finding.grounding_status { + crate::citation::verify::GroundingStatus::Located + | crate::citation::verify::GroundingStatus::NotApplicable => None, + crate::citation::verify::GroundingStatus::LocatedFuzzy => { + Some("support located by fuzzy match only".to_string()) + } + crate::citation::verify::GroundingStatus::Unlocated => { + Some("support quote not located in source (unverified)".to_string()) + } + } +} + /// Try archive fallbacks if the primary URL came back `SourceUnavailable`. /// Returns the original outcome if no archives work or if no archives exist. async fn try_archive_fallback( @@ -540,6 +586,51 @@ mod orchestrator_tests { } } + #[test] + fn review_finding_markers_project_the_report_onto_review_anchors() { + let judged = |level| CitationVerdict::Judged(level); + let mut unlocated = mk_finding("r_unlocated", 1, judged(SupportLevel::Supported)); + unlocated.grounding_status = GroundingStatus::Unlocated; + let mut dead = mk_finding("r_dead", 2, CitationVerdict::SourceUnavailable); + dead.source_unavailable_reason = Some(SourceUnavailableReason::Unreachable); + let mut long_claim = mk_finding("r_long", 3, judged(SupportLevel::Partial)); + long_claim.claim = "x".repeat(500); + // The standalone single-claim path has no page ref: nothing to anchor to. + let standalone = mk_finding("", 4, judged(SupportLevel::NotSupported)); + let report = PageVerificationReport { + wiki_id: "enwiki".into(), + title: "Cats".into(), + rev_id: 7, + findings: vec![ + mk_finding("r_ok", 0, judged(SupportLevel::Supported)), + unlocated, + dead, + long_claim, + standalone, + ], + skipped: Vec::new(), + extraction_failures: Vec::new(), + stats: PageVerificationStats::default(), + }; + + let markers = super::review_finding_markers(&report); + + assert_eq!(markers.len(), 4, "ref-less standalone finding is skipped"); + assert_eq!(markers[0].ref_id, "r_ok"); + assert_eq!(markers[0].verdict, "supported"); + assert_eq!(markers[0].detail, None); + assert_eq!( + markers[1].detail.as_deref(), + Some("support quote not located in source (unverified)") + ); + assert_eq!(markers[2].verdict, "source_unavailable"); + assert_eq!(markers[2].detail.as_deref(), Some("source unreachable")); + assert!( + markers[3].claim.chars().count() <= 200, + "claims are truncated to the outline display limit" + ); + } + #[test] fn apply_reverified_finding_moves_verdict_and_updates_tallies() { let judged = |level| CitationVerdict::Judged(level); diff --git a/crates/sp42-citation/src/lib.rs b/crates/sp42-citation/src/lib.rs index 3aefff03..2264e72c 100644 --- a/crates/sp42-citation/src/lib.rs +++ b/crates/sp42-citation/src/lib.rs @@ -40,8 +40,8 @@ pub use citation::extract::{ pub use citation::locate_quote::{FuzzyLocate, locate_quote, locate_quote_fuzzy}; pub use citation::page::{ PageVerificationReport, PageVerificationRequest, PageVerificationStats, ReverifyFindingRequest, - ReverifyFindingResponse, apply_reverified_finding, select_reverify_use_site, - verify_one_use_site, verify_page, + ReverifyFindingResponse, apply_reverified_finding, review_finding_markers, + select_reverify_use_site, verify_one_use_site, verify_page, }; pub use citation::parsing::{ ParsedVerdict, canonicalize_verdict, parse_repair_response, parse_verdict_response, diff --git a/crates/sp42-cli/src/main.rs b/crates/sp42-cli/src/main.rs index 9b4ddd81..c1b6e287 100644 --- a/crates/sp42-cli/src/main.rs +++ b/crates/sp42-cli/src/main.rs @@ -14,7 +14,8 @@ use sp42_core::{ PageVerificationReport, PageVerificationRequest, QueuedEdit, ReviewAckResponse, ReviewAnchor, ReviewEndRequest, ReviewOpenRequest, ReviewOpenResponse, ReviewPollRequest, ReviewPollResponse, ReviewPollStatus, ReviewPrompt, ReviewPromptKind, ReviewQueueRequest, ReviewQueueResponse, - ReviewReplyRequest, ReviewSessionSnapshot, ReviewSessionsResponse, + ReviewFindingsRequest, ReviewFindingsResponse, ReviewReplyRequest, ReviewSessionSnapshot, + ReviewSessionsResponse, review_finding_markers, SessionActionExecutionRequest, SessionActionExecutionResponse, SessionActionKind, SystemClock, VerificationOutcome, VerifyOptions as CoreVerifyOptions, build_dev_auth_bootstrap_request, locate_quote, locate_quote_fuzzy, parse_dev_auth_status, verify_citation_use_site, @@ -354,6 +355,9 @@ enum ReviewAction { Poll(ReviewPollArgs), /// Queue operator feedback from the command line (dev/test surface). Queue(ReviewQueueArgs), + /// Attach a verify-page report to the session so its findings overlay + /// the article outline (the report's in-article frontend). + Findings(ReviewFindingsArgs), /// Send an agent chat reply to the operator surface. Reply(ReviewReplyArgs), /// End a session as the agent (a plain reopen stays allowed). @@ -431,6 +435,23 @@ struct ReviewQueueArgs { fmt: FormatArg, } +#[derive(Debug, Args)] +struct ReviewFindingsArgs { + /// Page target: bare title or pasted wiki URL. + target: String, + /// Path to a `verify-page --format json` report, or `-` for stdin. + #[arg(long)] + report: String, + /// Target wiki id. + #[arg(long, default_value = BARE_URL_DEFAULT_WIKI)] + wiki: String, + /// Local server base URL. + #[arg(long, default_value = LOCAL_SERVER_BASE_URL)] + bridge_base_url: String, + #[command(flatten)] + fmt: FormatArg, +} + #[derive(Debug, Args)] struct ReviewReplyArgs { /// Page target: bare title or pasted wiki URL. @@ -3363,6 +3384,16 @@ fn dispatch_review(action: ReviewAction) -> Result { ); render_review_output(format, &response, text) } + ReviewAction::Findings(args) => { + let format = args.fmt.format; + let response = runtime.block_on(run_review_findings(&args))?; + let text = format!( + "attached {} finding marker(s)\n{}", + response.attached, + render_review_snapshot_line(&response.session) + ); + render_review_output(format, &response, text) + } ReviewAction::Reply(args) => { let format = args.fmt.format; let response = runtime.block_on(run_review_reply(&args))?; @@ -3498,6 +3529,42 @@ async fn run_review_queue(args: &ReviewQueueArgs) -> Result Result { + let raw = if path == "-" { + let mut buffer = String::new(); + std::io::Read::read_to_string(&mut std::io::stdin(), &mut buffer) + .map_err(|error| format!("failed to read report from stdin: {error}"))?; + buffer + } else { + std::fs::read_to_string(path) + .map_err(|error| format!("failed to read report {path}: {error}"))? + }; + serde_json::from_str(&raw) + .map_err(|error| format!("report is not a verify-page JSON report: {error}")) +} + +/// Attach a verify-page report to the review session: project its findings +/// onto review anchors (ref ids) and post them, pinned to the report's +/// revision so a stale report cannot overlay a newer session. +async fn run_review_findings(args: &ReviewFindingsArgs) -> Result { + let report = read_verification_report(&args.report)?; + let bridge = bootstrap_bridge_session(&args.bridge_base_url).await?; + let request = ReviewFindingsRequest { + wiki_id: args.wiki.clone(), + title: sp42_core::parse_page_target(&args.target).title, + rev_id: report.rev_id, + findings: review_finding_markers(&report), + }; + post_review_route( + &bridge, + &args.bridge_base_url, + route_contracts::DEV_REVIEW_FINDINGS_PATH, + &request, + ) + .await +} + async fn run_review_reply(args: &ReviewReplyArgs) -> Result { let bridge = bootstrap_bridge_session(&args.bridge_base_url).await?; let request = ReviewReplyRequest { @@ -3584,10 +3651,20 @@ fn render_review_snapshot_line(session: &ReviewSessionSnapshot) -> String { } fn render_review_open_text(response: &ReviewOpenResponse) -> String { + let unanchored = if response.unanchored_findings.is_empty() { + String::new() + } else { + format!( + "\nunanchored findings: {} (attached but matching no outline block)", + response.unanchored_findings.len() + ) + }; format!( - "{}\noutline: {} block(s)\nnext: {}", + "{}\noutline: {} block(s), {} finding(s) attached{}\nnext: {}", render_review_snapshot_line(&response.session), response.outline.len(), + response.session.findings, + unanchored, response.next_step ) } @@ -3972,7 +4049,8 @@ mod tests { CliOptions, Command, ContextPreviewOptions, FormatArg, LOCAL_SERVER_BASE_URL, OutputFormat, ReviewAction, ReviewAnchor, ReviewPollResponse, ReviewPollStatus, ReviewPrompt, ReviewPromptKind, ReviewQueueArgs, ShellMode, VerifyPageCliOptions, WorkbenchOptions, - build_review_queue_prompt, render_action_preview, render_backlog_preview, + build_review_queue_prompt, read_verification_report, render_action_preview, + render_backlog_preview, render_bare_url_execute, render_bare_url_proposals, render_context_preview, render_coordination_preview, render_parity_report, render_queue, render_review_poll_text, render_scenario_report, render_session_digest, render_stream_preview, render_workbench, @@ -4982,6 +5060,44 @@ mod tests { assert_eq!(open.bridge_base_url, LOCAL_SERVER_BASE_URL); } + #[test] + fn parses_review_findings_action() { + let cli = Cli::parse_from([ + "sp42-cli", + "review", + "findings", + "Exemple", + "--report", + "report.json", + "--wiki", + "frwiki", + ]); + let Command::Review(args) = cli.command else { + panic!("expected review command"); + }; + let ReviewAction::Findings(findings) = args.action else { + panic!("expected findings action"); + }; + assert_eq!(findings.target, "Exemple"); + assert_eq!(findings.report, "report.json"); + assert_eq!(findings.wiki, "frwiki"); + } + + #[test] + fn read_verification_report_rejects_non_report_json() { + let dir = std::env::temp_dir().join(format!( + "sp42-cli-review-findings-{}", + std::process::id() + )); + std::fs::create_dir_all(&dir).expect("temp dir should create"); + let path = dir.join("not-a-report.json"); + std::fs::write(&path, "{\"outcome\": true}").expect("temp file should write"); + + let error = read_verification_report(path.to_str().expect("utf-8 path")) + .expect_err("non-report JSON should be rejected"); + assert!(error.contains("not a verify-page JSON report")); + } + #[test] fn review_queue_prompt_builds_each_anchor_shape() { let base = ReviewQueueArgs { diff --git a/crates/sp42-platform/src/lib.rs b/crates/sp42-platform/src/lib.rs index fdb21cc8..7447c59c 100644 --- a/crates/sp42-platform/src/lib.rs +++ b/crates/sp42-platform/src/lib.rs @@ -128,12 +128,14 @@ pub use queue_builder::{ build_ranked_queue, build_ranked_queue_with_contexts, build_ranked_queue_with_policy, }; pub use review_session::{ - REVIEW_SESSION_CONTRACT_VERSION, ReviewAckResponse, ReviewAnchor, ReviewBlockOutline, - ReviewChatEntry, ReviewChatRole, ReviewEndRequest, ReviewEndedBy, ReviewFeedbackTake, + FindingsRevisionMismatch, REVIEW_SESSION_CONTRACT_VERSION, ReviewAckResponse, ReviewAnchor, + ReviewBlockOutline, ReviewChatEntry, ReviewChatRole, ReviewEndRequest, ReviewEndedBy, + ReviewFeedbackTake, ReviewFindingMarker, ReviewFindingsRequest, ReviewFindingsResponse, ReviewOpenRequest, ReviewOpenResponse, ReviewPollRequest, ReviewPollResponse, ReviewPollStatus, ReviewPrompt, ReviewPromptKind, ReviewQueueRequest, ReviewQueueResponse, ReviewReplyRequest, ReviewSession, ReviewSessionSnapshot, ReviewSessionStatus, ReviewSessionsResponse, - build_article_outline, open_next_step, poll_next_step, poll_status, + annotate_outline, build_article_outline, open_next_step, poll_next_step, poll_status, + truncate_outline_text, }; pub use review_workbench::{ PreparedRequestPreview, ReviewWorkbench, build_review_workbench, diff --git a/crates/sp42-platform/src/review_session.rs b/crates/sp42-platform/src/review_session.rs index 33381d61..d5091b6c 100644 --- a/crates/sp42-platform/src/review_session.rs +++ b/crates/sp42-platform/src/review_session.rs @@ -75,6 +75,35 @@ pub struct ReviewAnchor { pub selected_text: Option, } +/// One verification finding projected onto review-session coordinates — the +/// overlay marker that lets a review surface show a page verification +/// report's problems *in the context of the article* instead of as detached +/// report text. Markers are citation-agnostic here: the citation domain +/// projects its report into this shape at the edges (`ref_id` is the join +/// key onto outline blocks), so this crate never depends on report types. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct ReviewFindingMarker { + /// The originating ref's marker id (e.g. `cite_ref-smith_3-0`) — the + /// same anchor coordinate prompts use. + pub ref_id: String, + /// Verdict wire label (e.g. `"unsupported"`, `"source_unavailable"`). + pub verdict: String, + /// The judged claim sentence, truncated like outline text. + pub claim: String, + /// Optional short qualifier — a grounding caveat or unavailable reason. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub detail: Option, +} + +/// Refusal reason when attached findings were produced against a different +/// revision than the session is pinned to — a stale overlay would point at +/// blocks that may no longer exist. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct FindingsRevisionMismatch { + /// The revision the session is pinned to. + pub session_rev: u64, +} + /// One operator feedback item queued for the agent. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct ReviewPrompt { @@ -115,6 +144,10 @@ pub struct ReviewSession { pub status: ReviewSessionStatus, /// Prompts queued since the last successful poll delivery. pub queued_prompts: Vec, + /// Verification findings attached to this session's pinned revision — + /// the report overlay the open response joins onto the outline. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub findings: Vec, pub chat: Vec, #[serde(default, skip_serializing_if = "Option::is_none")] pub ended_by: Option, @@ -153,6 +186,7 @@ impl ReviewSession { rev_id, status: ReviewSessionStatus::Open, queued_prompts: Vec::new(), + findings: Vec::new(), chat: Vec::new(), ended_by: None, created_at_ms: now_ms, @@ -183,6 +217,8 @@ impl ReviewSession { } /// Resume an ended session as open again (after [`Self::gate_reopen`]). + /// Re-pinning to a *different* revision drops the findings overlay: the + /// markers describe the old revision's blocks. pub fn resume(&mut self, rev_id: u64, now_ms: i64) { self.status = if self.queued_prompts.is_empty() { ReviewSessionStatus::Open @@ -190,10 +226,39 @@ impl ReviewSession { ReviewSessionStatus::Feedback }; self.ended_by = None; + if self.rev_id != rev_id { + self.findings.clear(); + } self.rev_id = rev_id; self.updated_at_ms = now_ms; } + /// Replace the session's findings overlay with one report's markers. + /// Findings do not accumulate across attaches — each attach describes + /// one verification run of the pinned revision. + /// + /// # Errors + /// + /// [`FindingsRevisionMismatch`] when `report_rev_id` differs from the + /// session's pinned revision — a stale overlay must not annotate blocks + /// it was not produced against. + pub fn attach_findings( + &mut self, + report_rev_id: u64, + findings: Vec, + now_ms: i64, + ) -> Result { + if report_rev_id != self.rev_id { + return Err(FindingsRevisionMismatch { + session_rev: self.rev_id, + }); + } + let attached = findings.len(); + self.findings = findings; + self.updated_at_ms = now_ms; + Ok(attached) + } + /// Queue operator prompts; optionally close the session in the same /// action ("send & end"). Queued prompts survive the end and deliver on /// the next poll. @@ -259,6 +324,7 @@ impl ReviewSession { rev_id: self.rev_id, status: self.status, pending_prompts: self.queued_prompts.len(), + findings: self.findings.len(), ended_by: self.ended_by, updated_at_ms: self.updated_at_ms, } @@ -273,6 +339,9 @@ pub struct ReviewSessionSnapshot { pub rev_id: u64, pub status: ReviewSessionStatus, pub pending_prompts: usize, + /// Attached verification-finding markers (the report overlay). + #[serde(default)] + pub findings: usize, #[serde(default, skip_serializing_if = "Option::is_none")] pub ended_by: Option, pub updated_at_ms: i64, @@ -289,6 +358,10 @@ pub struct ReviewBlockOutline { pub text: String, /// Stable cite ids of the block's inline references, in order. pub ref_ids: Vec, + /// Verification findings anchored to this block (the report overlay), + /// joined by `ref_id`. Empty when no report is attached. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub findings: Vec, } /// Build the outline for a decomposed page. @@ -305,10 +378,41 @@ pub fn build_article_outline(blocks: &[ParsoidBlock]) -> Vec .iter() .map(|block_ref| block_ref.ref_id.clone()) .collect(), + findings: Vec::new(), }) .collect() } +/// Join finding markers onto outline blocks by `ref_id` — the projection +/// that turns the verification report into an in-article overlay. Markers +/// whose `ref_id` matches no block are returned, not dropped: the surface +/// must be able to say "these findings could not be placed" rather than +/// silently under-report. +#[must_use] +pub fn annotate_outline( + mut outline: Vec, + findings: &[ReviewFindingMarker], +) -> (Vec, Vec) { + let mut unanchored = Vec::new(); + for marker in findings { + match outline + .iter_mut() + .find(|block| block.ref_ids.iter().any(|ref_id| ref_id == &marker.ref_id)) + { + Some(block) => block.findings.push(marker.clone()), + None => unanchored.push(marker.clone()), + } + } + (outline, unanchored) +} + +/// Truncate text to the outline display limit — shared by outline blocks +/// and by edge crates projecting report claims into [`ReviewFindingMarker`]s. +#[must_use] +pub fn truncate_outline_text(text: &str) -> String { + truncate_chars(text, REVIEW_OUTLINE_TEXT_LIMIT) +} + const fn block_kind_label(kind: BlockKind) -> &'static str { match kind { BlockKind::Paragraph => "paragraph", @@ -343,15 +447,41 @@ pub struct ReviewOpenRequest { pub reopen: bool, } -/// Open/resume response: session snapshot plus the article outline. +/// Open/resume response: session snapshot plus the article outline. When a +/// verification report is attached, outline blocks carry their findings and +/// markers that matched no block surface in `unanchored_findings`. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct ReviewOpenResponse { pub contract_version: u32, pub session: ReviewSessionSnapshot, pub outline: Vec, + /// Attached findings whose `ref_id` matched no outline block. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub unanchored_findings: Vec, pub next_step: String, } +/// Agent attaches a verification report's findings to the session (the +/// report becomes an in-article overlay for the operator surface). +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct ReviewFindingsRequest { + pub wiki_id: String, + pub title: String, + /// Revision the findings were produced against; must match the + /// session's pinned revision. + #[serde(default)] + pub rev_id: u64, + pub findings: Vec, +} + +/// Findings-attach acknowledgement. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct ReviewFindingsResponse { + pub contract_version: u32, + pub session: ReviewSessionSnapshot, + pub attached: usize, +} + /// Operator queues prompts (the browser surface's send action). #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct ReviewQueueRequest { @@ -438,12 +568,19 @@ pub struct ReviewSessionsResponse { pub sessions: Vec, } -/// `next_step` for a fresh or resumed open. +/// `next_step` for a fresh or resumed open. Mentions the findings overlay +/// when one is attached so the agent tells the operator what to look at. #[must_use] -pub fn open_next_step(title: &str) -> String { +pub fn open_next_step(title: &str, findings: usize) -> String { + let overlay = if findings == 0 { + String::new() + } else { + format!(" The outline carries {findings} verification finding(s) anchored to blocks.") + }; format!( - "Session open on \"{title}\". Ask the operator to review it in the SP42 browser shell, \ - then run `sp42-cli review poll` and wait for their feedback before responding further." + "Session open on \"{title}\".{overlay} Ask the operator to review it in the SP42 browser \ + shell, then run `sp42-cli review poll` and wait for their feedback before responding \ + further." ) } @@ -498,8 +635,8 @@ pub fn poll_status(take: &ReviewFeedbackTake) -> ReviewPollStatus { mod tests { use super::{ REVIEW_OUTLINE_TEXT_LIMIT, ReopenRefused, ReviewEndedBy, ReviewFeedbackTake, ReviewPrompt, - ReviewPromptKind, ReviewSession, ReviewSessionStatus, build_article_outline, - poll_next_step, poll_status, + ReviewPromptKind, ReviewSession, ReviewSessionStatus, annotate_outline, + build_article_outline, open_next_step, poll_next_step, poll_status, }; use crate::wikitext_editor::{BlockKind, BlockRef, ParsoidBlock}; @@ -691,6 +828,121 @@ mod tests { ); } + #[test] + fn attach_findings_replaces_the_overlay_and_counts() { + let mut session = ReviewSession::open("frwiki", "Exemple", 42, 1_000); + + let attached = session + .attach_findings(42, vec![marker("cite_ref-a_1-0", "unsupported")], 2_000) + .expect("matching revision should attach"); + assert_eq!(attached, 1); + assert_eq!(session.findings.len(), 1); + assert_eq!(session.updated_at_ms, 2_000); + + // A later attach replaces the overlay wholesale — findings describe + // one report, they do not accumulate across runs. + let attached = session + .attach_findings( + 42, + vec![ + marker("cite_ref-b_2-0", "partial"), + marker("cite_ref-c_3-0", "source_unavailable"), + ], + 3_000, + ) + .expect("matching revision should attach"); + assert_eq!(attached, 2); + assert_eq!(session.findings.len(), 2); + assert_eq!(session.findings[0].ref_id, "cite_ref-b_2-0"); + } + + #[test] + fn attach_findings_refuses_a_revision_mismatch() { + let mut session = ReviewSession::open("frwiki", "Exemple", 42, 1_000); + + let refused = session + .attach_findings(41, vec![marker("cite_ref-a_1-0", "unsupported")], 2_000) + .expect_err("stale-revision findings must not overlay the outline"); + assert_eq!(refused.session_rev, 42); + assert!(session.findings.is_empty()); + } + + #[test] + fn resume_to_a_new_revision_drops_stale_findings() { + let mut session = ReviewSession::open("frwiki", "Exemple", 42, 1_000); + session + .attach_findings(42, vec![marker("cite_ref-a_1-0", "unsupported")], 2_000) + .expect("attach"); + session.end(ReviewEndedBy::Agent, 3_000); + + session.resume(43, 4_000); + assert!( + session.findings.is_empty(), + "findings for revision 42 must not overlay revision 43" + ); + + let mut same_rev = ReviewSession::open("frwiki", "Exemple", 42, 1_000); + same_rev + .attach_findings(42, vec![marker("cite_ref-a_1-0", "unsupported")], 2_000) + .expect("attach"); + same_rev.resume(42, 3_000); + assert_eq!(same_rev.findings.len(), 1, "same-revision resume keeps the overlay"); + } + + #[test] + fn annotate_outline_joins_findings_onto_blocks_by_ref_id() { + let outline = vec![ + outline_block(0, vec!["cite_ref-a_1-0"]), + outline_block(1, vec!["cite_ref-b_2-0", "cite_ref-c_3-0"]), + ]; + let findings = vec![ + marker("cite_ref-c_3-0", "unsupported"), + marker("cite_ref-ghost_9-0", "partial"), + ]; + + let (annotated, unanchored) = annotate_outline(outline, &findings); + + assert!(annotated[0].findings.is_empty()); + assert_eq!(annotated[1].findings.len(), 1); + assert_eq!(annotated[1].findings[0].verdict, "unsupported"); + assert_eq!(unanchored.len(), 1, "unmatched markers surface, never drop"); + assert_eq!(unanchored[0].ref_id, "cite_ref-ghost_9-0"); + } + + #[test] + fn snapshot_counts_attached_findings() { + let mut session = ReviewSession::open("frwiki", "Exemple", 42, 1_000); + session + .attach_findings(42, vec![marker("cite_ref-a_1-0", "unsupported")], 2_000) + .expect("attach"); + assert_eq!(session.snapshot().findings, 1); + } + + #[test] + fn open_next_step_mentions_an_attached_overlay() { + assert!(!open_next_step("Exemple", 0).contains("finding")); + assert!(open_next_step("Exemple", 3).contains("3 verification finding")); + } + + fn marker(ref_id: &str, verdict: &str) -> super::ReviewFindingMarker { + super::ReviewFindingMarker { + ref_id: ref_id.to_string(), + verdict: verdict.to_string(), + claim: "a claim".to_string(), + detail: None, + } + } + + fn outline_block(block_ordinal: usize, ref_ids: Vec<&str>) -> super::ReviewBlockOutline { + super::ReviewBlockOutline { + block_ordinal, + kind: "paragraph".to_string(), + text: "text".to_string(), + ref_ids: ref_ids.into_iter().map(str::to_string).collect(), + findings: Vec::new(), + } + } + #[test] fn wire_types_round_trip_with_the_contract_version() { let response = super::ReviewPollResponse { diff --git a/crates/sp42-platform/src/routes.rs b/crates/sp42-platform/src/routes.rs index 1b80b215..6de183b8 100644 --- a/crates/sp42-platform/src/routes.rs +++ b/crates/sp42-platform/src/routes.rs @@ -105,6 +105,7 @@ pub const DEV_REVIEW_OPEN_PATH: &str = "/dev/review/open"; pub const DEV_REVIEW_SESSIONS_PATH: &str = "/dev/review/sessions"; pub const DEV_REVIEW_PROMPTS_PATH: &str = "/dev/review/prompts"; pub const DEV_REVIEW_POLL_PATH: &str = "/dev/review/poll"; +pub const DEV_REVIEW_FINDINGS_PATH: &str = "/dev/review/findings"; pub const DEV_REVIEW_REPLY_PATH: &str = "/dev/review/agent-reply"; pub const DEV_REVIEW_END_PATH: &str = "/dev/review/end"; diff --git a/crates/sp42-server/src/review_routes.rs b/crates/sp42-server/src/review_routes.rs index 1dcce631..0d7e8e30 100644 --- a/crates/sp42-server/src/review_routes.rs +++ b/crates/sp42-server/src/review_routes.rs @@ -19,9 +19,10 @@ use axum::{ }; use sp42_core::{ REVIEW_SESSION_CONTRACT_VERSION, ReviewAckResponse, ReviewBlockOutline, ReviewEndRequest, - ReviewEndedBy, ReviewFeedbackTake, ReviewOpenRequest, ReviewOpenResponse, ReviewPollRequest, - ReviewPollResponse, ReviewPollStatus, ReviewQueueRequest, ReviewQueueResponse, ReviewSession, - ReviewSessionSnapshot, ReviewSessionsResponse, WikitextPageRef, build_article_outline, + ReviewEndedBy, ReviewFeedbackTake, ReviewFindingsRequest, ReviewFindingsResponse, + ReviewOpenRequest, ReviewOpenResponse, ReviewPollRequest, ReviewPollResponse, ReviewPollStatus, + ReviewQueueRequest, ReviewQueueResponse, ReviewSession, ReviewSessionSnapshot, + ReviewSessionsResponse, WikitextPageRef, annotate_outline, build_article_outline, open_next_step, poll_next_step, poll_status, }; use tokio::sync::{Notify, RwLock}; @@ -168,8 +169,12 @@ pub(crate) async fn post_review_open( let mut store = state.review_sessions.write().await; let resumed = open_or_resume(store.get_mut(&key), payload.reopen, &title, rev_id, now_ms)?; - let session = if let Some(snapshot) = resumed { - snapshot + let (session, findings) = if let Some(snapshot) = resumed { + let findings = store + .get(&key) + .map(|entry| entry.session.findings.clone()) + .unwrap_or_default(); + (snapshot, findings) } else { let session = ReviewSession::open(&payload.wiki_id, &title, rev_id, now_ms); let snapshot = session.snapshot(); @@ -180,19 +185,25 @@ pub(crate) async fn post_review_open( notify: Arc::new(Notify::new()), }, ); - snapshot + (snapshot, Vec::new()) }; + // Attached verification findings overlay the outline: each marker joins + // its block by ref_id, so the operator sees report problems in article + // context rather than as detached text. + let (outline, unanchored_findings) = annotate_outline(outline, &findings); info!( wiki_id = %session.wiki_id, title = %session.title, rev_id = session.rev_id, + findings = findings.len(), "review session opened" ); Ok(Json(ReviewOpenResponse { contract_version: REVIEW_SESSION_CONTRACT_VERSION, - next_step: open_next_step(&session.title), + next_step: open_next_step(&session.title, findings.len()), session, outline, + unanchored_findings, })) } @@ -244,6 +255,52 @@ pub(crate) async fn post_review_prompts( })) } +/// `POST /dev/review/findings` — the agent attaches a verification report's +/// finding markers to the session (replace-all; a stale-revision report is +/// refused). The markers overlay the outline on the next open and feed the +/// operator surface; they are not agent feedback, so no poll is woken. +pub(crate) async fn post_review_findings( + State(state): State, + headers: HeaderMap, + Json(payload): Json, +) -> Result { + require_review_session(&state, &headers).await?; + let key = ReviewSession::canonical_key(&payload.wiki_id, &payload.title); + let now_ms = state.clock.now_ms(); + let mut store = state.review_sessions.write().await; + let Some(entry) = store.get_mut(&key) else { + return Err(missing_session_error(&payload.wiki_id, &payload.title)); + }; + let attached = entry + .session + .attach_findings(payload.rev_id, payload.findings, now_ms) + .map_err(|mismatch| { + ( + StatusCode::CONFLICT, + Json(serde_json::json!({ + "error": format!( + "the findings were produced against revision {} but the review \ + session is pinned to revision {}; re-run verification (or reopen \ + the session) on one revision", + payload.rev_id, mismatch.session_rev + ), + "code": "review-findings-revision-mismatch", + })), + ) + })?; + info!( + wiki_id = %payload.wiki_id, + title = %payload.title, + attached, + "review findings attached" + ); + Ok(Json(ReviewFindingsResponse { + contract_version: REVIEW_SESSION_CONTRACT_VERSION, + session: entry.session.snapshot(), + attached, + })) +} + /// Drain one session's feedback; `None` when the session does not exist. async fn drain_feedback( state: &AppState, diff --git a/crates/sp42-server/src/routes.rs b/crates/sp42-server/src/routes.rs index ad7ceae6..0b6a71df 100644 --- a/crates/sp42-server/src/routes.rs +++ b/crates/sp42-server/src/routes.rs @@ -19,8 +19,8 @@ use crate::citation_routes::{ }; use crate::operator_live::get_live_operator_view; use crate::review_routes::{ - get_review_sessions, post_review_end, post_review_open, post_review_poll, post_review_prompts, - post_review_reply, + get_review_sessions, post_review_end, post_review_findings, post_review_open, + post_review_poll, post_review_prompts, post_review_reply, }; use crate::revision_artifacts::{ get_rendered_hunk_preview, get_revision_diff, get_revision_media_diff, @@ -112,6 +112,10 @@ fn review_session_routes(router: Router) -> Router { route_contracts::DEV_REVIEW_POLL_PATH, axum::routing::post(post_review_poll), ) + .route( + route_contracts::DEV_REVIEW_FINDINGS_PATH, + axum::routing::post(post_review_findings), + ) .route( route_contracts::DEV_REVIEW_REPLY_PATH, axum::routing::post(post_review_reply), diff --git a/crates/sp42-server/src/tests.rs b/crates/sp42-server/src/tests.rs index 39f42721..eda86a27 100644 --- a/crates/sp42-server/src/tests.rs +++ b/crates/sp42-server/src/tests.rs @@ -4158,6 +4158,86 @@ async fn review_session_loop_delivers_operator_feedback_to_the_agent() { ); } +#[tokio::test] +async fn review_findings_attach_and_overlay_the_next_open() { + let (router, cookie) = review_test_router().await; + + // Attaching to a page with no session refuses. + let marker = serde_json::json!({ + "ref_id": "cite_ref-a_1-0", + "verdict": "not_supported", + "claim": "Cats bark.", + }); + let attach_body = serde_json::json!({ + "wiki_id": "frwiki", + "title": "Exemple", + "rev_id": 42, + "findings": [marker], + }); + let (status, refused) = post_review_json( + router.clone(), + &cookie, + sp42_core::routes::DEV_REVIEW_FINDINGS_PATH, + &attach_body, + ) + .await; + assert_eq!(status, StatusCode::NOT_FOUND); + assert_eq!(refused["code"], "review-session-not-found"); + + let (status, _) = post_review_json( + router.clone(), + &cookie, + sp42_core::routes::DEV_REVIEW_OPEN_PATH, + &serde_json::json!({"wiki_id": "frwiki", "target": "Exemple", "rev_id": 42}), + ) + .await; + assert_eq!(status, StatusCode::OK); + + // A report produced against another revision must not overlay this one. + let mut stale = attach_body.clone(); + stale["rev_id"] = serde_json::json!(41); + let (status, refused) = post_review_json( + router.clone(), + &cookie, + sp42_core::routes::DEV_REVIEW_FINDINGS_PATH, + &stale, + ) + .await; + assert_eq!(status, StatusCode::CONFLICT); + assert_eq!(refused["code"], "review-findings-revision-mismatch"); + + let (status, attached) = post_review_json( + router.clone(), + &cookie, + sp42_core::routes::DEV_REVIEW_FINDINGS_PATH, + &attach_body, + ) + .await; + assert_eq!(status, StatusCode::OK); + assert_eq!(attached["attached"], 1); + assert_eq!(attached["session"]["findings"], 1); + + // The next open carries the overlay. The scripted editor exposes no + // blocks, so the marker surfaces as unanchored rather than dropping; + // block-level joins are covered by the platform unit tests. + let (status, open) = post_review_json( + router.clone(), + &cookie, + sp42_core::routes::DEV_REVIEW_OPEN_PATH, + &serde_json::json!({"wiki_id": "frwiki", "target": "Exemple", "rev_id": 42}), + ) + .await; + assert_eq!(status, StatusCode::OK); + assert_eq!(open["unanchored_findings"][0]["ref_id"], "cite_ref-a_1-0"); + assert_eq!(open["unanchored_findings"][0]["verdict"], "not_supported"); + assert!( + open["next_step"] + .as_str() + .expect("next_step should be a string") + .contains("1 verification finding"), + ); +} + #[tokio::test] async fn review_reopen_gate_requires_explicit_reopen_after_operator_end() { let (router, cookie) = review_test_router().await; diff --git a/docs/domains/references/README.md b/docs/domains/references/README.md index 8e2fd884..f2f30014 100644 --- a/docs/domains/references/README.md +++ b/docs/domains/references/README.md @@ -21,7 +21,7 @@ Operational guidance for the model panel is in [model-panel.md](model-panel.md). - [PRD-0010 — Citation-verification agent surface (MCP)](prd/0010-citation-verification-mcp-surface.md) — expose probe/verify verbs to agent clients via the `sp42-mcp` shell - [PRD-0012 — Citation insertion for unsourced claims](prd/0012-citation-insertion.md) — ground a candidate source against a sentence's atomic claims, then propose a `` insert - [PRD-0014 — Citation repair and insertion, browser surface](prd/0014-citation-repair-insertion-browser-surface.md) — per-finding action row in the Citations tab routing to text-edit or citation-fix, operator always chooses -- [PRD-0017 — Interactive review sessions](prd/0017-interactive-review-sessions.md) — agent opens a session on a page, the operator queues anchored feedback, the agent polls for it (loop ported from lavish-axi, keyed to wiki pages) +- [PRD-0017 — Interactive review sessions](prd/0017-interactive-review-sessions.md) — agent opens a session on a page, the operator queues anchored feedback, the agent polls for it (loop ported from lavish-axi, keyed to wiki pages); attached verify-page findings overlay the outline as the report's in-article view ## Architecture Decision Records diff --git a/docs/domains/references/adr/0018-review-session-bridge-contract.md b/docs/domains/references/adr/0018-review-session-bridge-contract.md index 36df955c..8555c5f8 100644 --- a/docs/domains/references/adr/0018-review-session-bridge-contract.md +++ b/docs/domains/references/adr/0018-review-session-bridge-contract.md @@ -59,8 +59,8 @@ contracts (Art. 9.1). persistence: this is a localhost dev-bridge surface (PRD-0017 Risks). 4. **Routes under `/dev/review/*`, POSTs session+CSRF gated.** Path constants - in `sp42_platform::routes` like every other route contract. All five POST - routes (`open`, `prompts`, `poll`, `agent-reply`, `end`) require the + in `sp42_platform::routes` like every other route contract. All six POST + routes (`open`, `prompts`, `poll`, `findings`, `agent-reply`, `end`) require the bridge session cookie plus the CSRF header (ADR-0002) — open reads through the caller's wiki identity to pin the latest revision, and the rest mutate session state. `GET /dev/review/sessions` is an ungated read-only @@ -79,15 +79,35 @@ contracts (Art. 9.1). `verify-page` (bootstrap → cookie + CSRF token). The poll loop keeps stdout reserved for one structured response and narrates waiting on stderr, mirroring the ported tool's agent ergonomics. MCP verbs are - deferred (PRD-0017 open question 3) because `sp42-mcp` has no + deferred (PRD-0017 open question 2) because `sp42-mcp` has no localhost-server client today. +7. **The verification report overlays the outline as citation-agnostic + markers.** `sp42-platform` gains `ReviewFindingMarker` — `ref_id`, + verdict wire label, truncated claim, optional caveat — deliberately *not* + the citation domain's `CitationFinding`: the platform layer never depends + on report types (ADR-0013 layering), so the citation crate projects its + report into markers at the edge (`review_finding_markers`, which also + translates grounding caveats and unavailable reasons into prose and skips + ref-less standalone findings). Markers attach via `POST + /dev/review/findings` with replace-all semantics (one attach = one + verification run) and a revision gate — a report pinned to a different + revision returns 409 `review-findings-revision-mismatch`, and resuming a + session onto a new revision drops the stale overlay. On open, the pure + `annotate_outline` joins markers onto blocks by `ref_id` (the same + coordinate prompts anchor to); unmatched markers return as + `unanchored_findings` rather than dropping. Attaching wakes no poll — + findings are operator-surface input, not agent feedback. + ## Consequences - The loop's semantics are testable without a server, and the server tests exercise only glue (gating, wake-ups, status codes). - A browser Review panel needs no new contract — it consumes the same routes the CLI queueing surface uses. +- The text report is no longer the only operator surface for verification + results: any consumer of the open response gets the report as an + in-article, per-block overlay for free. - Session state dies with the server process; if review sessions outgrow the dev bridge (multi-operator, cross-restart), persistence and eviction become a follow-up ADR alongside the deployment-mode gating question. diff --git a/docs/domains/references/prd/0017-interactive-review-sessions.md b/docs/domains/references/prd/0017-interactive-review-sessions.md index 5182aeff..57449f95 100644 --- a/docs/domains/references/prd/0017-interactive-review-sessions.md +++ b/docs/domains/references/prd/0017-interactive-review-sessions.md @@ -55,6 +55,20 @@ User-facing behavior: or a missing session, narrating the wait on stderr while stdout stays a single structured response. `--agent-reply ""` posts a chat line to the operator surface before the wait starts. +- **Findings overlay**: the session doubles as the *in-article frontend for + the verification report*. `sp42-cli review findings + --report ` attaches a `PageVerificationReport`'s + findings to the session, projected onto the same anchors prompts use — + each finding joins its outline block by cite id, carrying the verdict, a + truncated claim, and a caveat (dead source, ungrounded support). The next + open returns the annotated outline, so the operator sees where the + problems are in the article instead of reading a detached text report, + and can queue prompts directly against them. Markers matching no block + surface as `unanchored_findings`, never silently dropped; a report + produced against a different revision than the session's pin is refused + (`review-findings-revision-mismatch`). Attaching spends no inference — + verification stays operator-triggered (PRD-0014 posture); the agent runs + `verify-page` explicitly and hands the result over. - **Session-end etiquette** (ported): feedback queued before an end still delivers first, flagged `session_ended`; only a later poll reports `ended`. An **operator**-ended session refuses a plain reopen (HTTP 409 with @@ -94,8 +108,21 @@ lanes (re-verify, bare-URL repair, inline edit). ended `next_step` distinguishes operator-ended (do not reopen) from agent-ended, verified by `next_step_matches_the_loop_etiquette` (`sp42-platform`) -- [ ] Session opens, feedback queueing, deliveries, and ends emit `tracing` - events, checkable in server logs +- [ ] Attached findings overlay the next open's outline joined by cite id, + with unmatched markers surfaced (not dropped), verified by + `annotate_outline_joins_findings_onto_blocks_by_ref_id` (`sp42-platform`) + and `review_findings_attach_and_overlay_the_next_open` (`sp42-server`) +- [ ] A report for a different revision than the session's pin refuses with + `review-findings-revision-mismatch`, and resuming to a new revision drops + the stale overlay, verified by `attach_findings_refuses_a_revision_mismatch` + and `resume_to_a_new_revision_drops_stale_findings` (`sp42-platform`) +- [ ] Report findings project onto review anchors — verdict wire labels, + truncated claims, grounding/unavailable caveats, ref-less standalone + findings skipped — verified by + `review_finding_markers_project_the_report_onto_review_anchors` + (`sp42-citation`) +- [ ] Session opens, feedback queueing, findings attaches, deliveries, and + ends emit `tracing` events, checkable in server logs ## Alternatives @@ -130,14 +157,10 @@ lanes (re-verify, bare-URL repair, inline edit). ## Open questions 1. **Where does the operator annotate?** Proposed: a Review panel in the - browser shell's article surface, listing outline blocks with per-block - queue actions and a session composer, reusing the `/dev/review` routes. + browser shell's article surface, listing outline blocks — with attached + findings rendered inline as per-block badges — plus per-block queue + actions and a session composer, reusing the `/dev/review` routes. Until then the CLI `review queue` command is the queueing surface. -2. **Should the outline include verification findings** (merge - `PageVerificationReport` findings into the open response) so the operator - annotates findings rather than raw blocks? Proposed: yes, as an opt-in - flag once the browser panel exists; verification spends inference budget, - so it must stay operator-triggered (PRD-0014 posture). -3. **MCP surface**: should `sp42-mcp` grow `review_open`/`review_poll` verbs? +2. **MCP surface**: should `sp42-mcp` grow `review_open`/`review_poll` verbs? Proposed: yes, but the MCP server is standalone today (no localhost-server client); bridging it is its own small ADR. diff --git a/docs/platform/CLI.md b/docs/platform/CLI.md index 541c52b6..91c91d5d 100644 --- a/docs/platform/CLI.md +++ b/docs/platform/CLI.md @@ -200,6 +200,8 @@ actions ride the local bridge session (bootstrap → cookie + CSRF). sp42-cli review open "https://fr.wikipedia.org/wiki/Exemple" --wiki frwiki sp42-cli review poll "Exemple" --wiki frwiki --agent-reply "opened — start with the lede" sp42-cli review queue "Exemple" --wiki frwiki --message "check this ref" --block 3 --ref-id cite_ref-a_1-0 +sp42-cli verify-page --title "Exemple" --wiki frwiki --format json > report.json +sp42-cli review findings "Exemple" --wiki frwiki --report report.json sp42-cli review end "Exemple" --wiki frwiki sp42-cli review sessions ``` @@ -209,6 +211,7 @@ sp42-cli review sessions | `open ` | Open or resume a session; returns the article outline plus `next_step`. `--rev ` pins a revision (default: latest), `--reopen` resumes a session the operator explicitly ended. | | `poll ` | Wait for feedback; re-arms bounded server waits until feedback, an end, or a missing session (stderr narrates the wait). `--agent-reply ` posts a chat line first, `--once` returns after a single wait. | | `queue ` | Queue one operator prompt (dev/test surface): `--message ` plus optional anchors `--block `, `--ref-id `, `--selected-text `; `--end` sends and ends in one action. | +| `findings ` | Attach a `verify-page --format json` report (`--report `, `-` for stdin) so its findings overlay the outline by cite id — the report's in-article view. Refused when the report's revision differs from the session's (`review-findings-revision-mismatch`). | | `reply ` | Send an agent chat reply (`--message `). | | `end ` | End the session as the agent; a plain reopen stays allowed. | | `sessions` | List review sessions on the local server. | diff --git a/docs/platform/architecture.md b/docs/platform/architecture.md index d0236702..184cfa4f 100644 --- a/docs/platform/architecture.md +++ b/docs/platform/architecture.md @@ -150,7 +150,7 @@ Reading notes: | Crate | Layer | ADRs that name it | PRDs that name it | Notes | |---|---|---|---|---| | `sp42-app` | shell | [ADR-0004](adr/0004-crate-boundary-collaboration-model.md), [ADR-0005](adr/0005-design-system-shared-component-layer.md), [ADR-0008](../domains/references/adr/0008-citation-verification-contract.md), [ADR-0010](adr/0010-operator-confirmed-content-proposals.md), [ADR-0012](adr/0012-frontend-e2e-testing-approach.md), [ADR-0013](adr/0013-layered-platform-domain-architecture.md) | [PRD-0002](../domains/patrolling/prd/0002-patrol-review-workflow.md), [PRD-0006](../domains/patrolling/prd/0006-multi-operator-coordination.md), [PRD-0010](../domains/references/prd/0010-citation-verification-mcp-surface.md), [PRD-0014](../domains/references/prd/0014-citation-repair-insertion-browser-surface.md) | | -| `sp42-citation` | domain | [ADR-0013](adr/0013-layered-platform-domain-architecture.md) | [PRD-0001](../domains/references/prd/0001-citation-verification.md), [PRD-0008](../domains/references/prd/0008-bare-url-repair.md), [PRD-0010](../domains/references/prd/0010-citation-verification-mcp-surface.md), [PRD-0014](../domains/references/prd/0014-citation-repair-insertion-browser-surface.md), [PRD-0016](../domains/assessment/prd/0016-ga-evidence-appendix-renderer.md) | | +| `sp42-citation` | domain | [ADR-0013](adr/0013-layered-platform-domain-architecture.md) | [PRD-0001](../domains/references/prd/0001-citation-verification.md), [PRD-0008](../domains/references/prd/0008-bare-url-repair.md), [PRD-0010](../domains/references/prd/0010-citation-verification-mcp-surface.md), [PRD-0014](../domains/references/prd/0014-citation-repair-insertion-browser-surface.md), [PRD-0016](../domains/assessment/prd/0016-ga-evidence-appendix-renderer.md), [PRD-0017](../domains/references/prd/0017-interactive-review-sessions.md) | | | `sp42-cli` | shell | [ADR-0004](adr/0004-crate-boundary-collaboration-model.md), [ADR-0005](adr/0005-design-system-shared-component-layer.md), [ADR-0013](adr/0013-layered-platform-domain-architecture.md), [ADR-0015](../domains/references/adr/0015-rules-compliant-read-only-fetch-edge.md), [ADR-0018](../domains/references/adr/0018-review-session-bridge-contract.md) | [PRD-0001](../domains/references/prd/0001-citation-verification.md), [PRD-0017](../domains/references/prd/0017-interactive-review-sessions.md) | | | `sp42-coordination` | platform | [ADR-0004](adr/0004-crate-boundary-collaboration-model.md), [ADR-0013](adr/0013-layered-platform-domain-architecture.md) | [PRD-0006](../domains/patrolling/prd/0006-multi-operator-coordination.md) | | | `sp42-core` | hybrid | [ADR-0001](adr/0001-foundational-decisions.md), [ADR-0003](adr/0003-node-anchored-wikitext-editing.md), [ADR-0004](adr/0004-crate-boundary-collaboration-model.md), [ADR-0005](adr/0005-design-system-shared-component-layer.md), [ADR-0006](adr/0006-using-llms.md), [ADR-0007](../domains/references/adr/0007-citation-verification-semantics.md), [ADR-0008](../domains/references/adr/0008-citation-verification-contract.md), [ADR-0009](../domains/references/adr/0009-citation-source-snapshot-storage.md), [ADR-0010](adr/0010-operator-confirmed-content-proposals.md), [ADR-0011](../domains/references/adr/0011-article-citation-verification.md), [ADR-0013](adr/0013-layered-platform-domain-architecture.md), [ADR-0015](../domains/references/adr/0015-rules-compliant-read-only-fetch-edge.md) | [PRD-0001](../domains/references/prd/0001-citation-verification.md), [PRD-0002](../domains/patrolling/prd/0002-patrol-review-workflow.md), [PRD-0004](../domains/patrolling/prd/0004-reviewer-actions-on-wikimedia.md), [PRD-0005](../domains/patrolling/prd/0005-operator-identity-and-session.md), [PRD-0007](../domains/references/prd/0007-llm-output-benchmarking.md), [PRD-0015](../domains/assessment/prd/0015-article-stability-signal.md), [PRD-0016](../domains/assessment/prd/0016-ga-evidence-appendix-renderer.md) | Hybrid exemption — re-export facade being split into platform/domain crates and retired ([ADR-0013](adr/0013-layered-platform-domain-architecture.md)) | From 21021a4c8aac8b1980d6d359e676aa8515a29258 Mon Sep 17 00:00:00 2001 From: Luis Villa Date: Mon, 13 Jul 2026 19:37:27 -0700 Subject: [PATCH 4/8] fix(review): address Codex review of the session loop and findings overlay - An ended session refuses new prompts (409 review-session-ended) instead of silently flipping back to feedback around the reopen gate; queued pre-end feedback still delivers with its end flag intact. - The open route checks the reopen gate against the stored session before any remote revision/Parsoid read, so a refused reopen cannot hang or 5xx on slow lookups. - ReviewPollRequest.wait_ms is now optional: omitted means the server default, an explicit 0 is the documented nonblocking status check. - `review findings` validates the report's wiki/title against the target session, so a reused report file cannot overlay another page's outline when revision ids collide. - Agent chat replies surface on the open response (`chat`), so a posted summary is readable by the operator surface instead of write-only. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01QB2VfS9GXyTzVw3FayQZxf --- crates/sp42-cli/src/main.rs | 77 +++++++++++++-- crates/sp42-platform/src/lib.rs | 4 +- crates/sp42-platform/src/review_session.rs | 88 +++++++++++++++-- crates/sp42-server/src/review_routes.rs | 97 ++++++++++++++----- crates/sp42-server/src/tests.rs | 80 +++++++++++++++ .../0018-review-session-bridge-contract.md | 14 ++- .../prd/0017-interactive-review-sessions.md | 13 ++- docs/platform/CLI.md | 2 +- 8 files changed, 321 insertions(+), 54 deletions(-) diff --git a/crates/sp42-cli/src/main.rs b/crates/sp42-cli/src/main.rs index acc3e641..09edf4a5 100644 --- a/crates/sp42-cli/src/main.rs +++ b/crates/sp42-cli/src/main.rs @@ -3463,7 +3463,9 @@ async fn run_review_poll(args: &ReviewPollArgs) -> Result Result Result<(), String> { + // MediaWiki treats underscores and spaces in titles as the same page. + let canonical = |t: &str| t.replace('_', " "); + if report.wiki_id != wiki || canonical(&report.title) != canonical(title) { + return Err(format!( + "the report describes {}:{} but the target session is {wiki}:{title}; \ + pass the report produced for this page", + report.wiki_id, report.title + )); + } + Ok(()) +} + /// Attach a verify-page report to the review session: project its findings /// onto review anchors (ref ids) and post them, pinned to the report's /// revision so a stale report cannot overlay a newer session. async fn run_review_findings(args: &ReviewFindingsArgs) -> Result { let report = read_verification_report(&args.report)?; + let title = sp42_core::parse_page_target(&args.target).title; + check_report_identity(&report, &args.wiki, &title)?; let bridge = bootstrap_bridge_session(&args.bridge_base_url).await?; let request = ReviewFindingsRequest { wiki_id: args.wiki.clone(), - title: sp42_core::parse_page_target(&args.target).title, + title, rev_id: report.rev_id, findings: review_finding_markers(&report), }; @@ -4049,13 +4074,14 @@ mod tests { use super::{ ActionKind, BareUrlAction, BareUrlCliMode, BareUrlCliOptions, BareUrlExecuteReport, Cli, CliOptions, Command, ContextPreviewOptions, FormatArg, LOCAL_SERVER_BASE_URL, OutputFormat, - ReviewAction, ReviewAnchor, ReviewPollResponse, ReviewPollStatus, ReviewPrompt, - ReviewPromptKind, ReviewQueueArgs, ShellMode, VerifyPageCliOptions, WorkbenchOptions, - build_review_queue_prompt, read_verification_report, render_action_preview, - render_backlog_preview, render_bare_url_execute, render_bare_url_proposals, - render_context_preview, render_coordination_preview, render_parity_report, render_queue, - render_review_poll_text, render_scenario_report, render_session_digest, - render_stream_preview, render_workbench, select_bare_url_proposal, server_report_lines, + PageVerificationReport, ReviewAction, ReviewAnchor, ReviewPollResponse, ReviewPollStatus, + ReviewPrompt, ReviewPromptKind, ReviewQueueArgs, ShellMode, VerifyPageCliOptions, + WorkbenchOptions, build_review_queue_prompt, check_report_identity, + read_verification_report, render_action_preview, render_backlog_preview, + render_bare_url_execute, render_bare_url_proposals, render_context_preview, + render_coordination_preview, render_parity_report, render_queue, render_review_poll_text, + render_scenario_report, render_session_digest, render_stream_preview, render_workbench, + select_bare_url_proposal, server_report_lines, }; use clap::Parser; use serde_json::json; @@ -5097,6 +5123,39 @@ mod tests { assert!(error.contains("not a verify-page JSON report")); } + #[test] + fn report_identity_check_matches_page_not_just_revision() { + let report = |wiki: &str, title: &str| PageVerificationReport { + wiki_id: wiki.to_string(), + title: title.to_string(), + rev_id: 42, + findings: Vec::new(), + skipped: Vec::new(), + extraction_failures: Vec::new(), + book_resolutions: Vec::new(), + stats: sp42_core::PageVerificationStats::default(), + }; + + assert!(check_report_identity(&report("frwiki", "Exemple"), "frwiki", "Exemple").is_ok()); + // Underscore and space spellings are the same MediaWiki title. + assert!( + check_report_identity( + &report("frwiki", "Grand_Exemple"), + "frwiki", + "Grand Exemple" + ) + .is_ok() + ); + + let wrong_wiki = check_report_identity(&report("enwiki", "Exemple"), "frwiki", "Exemple") + .expect_err("a report for another wiki must not attach"); + assert!(wrong_wiki.contains("enwiki:Exemple")); + assert!( + check_report_identity(&report("frwiki", "Autre"), "frwiki", "Exemple").is_err(), + "a report for another page must not attach" + ); + } + #[test] fn review_queue_prompt_builds_each_anchor_shape() { let base = ReviewQueueArgs { diff --git a/crates/sp42-platform/src/lib.rs b/crates/sp42-platform/src/lib.rs index 6eb12fbc..e1f06835 100644 --- a/crates/sp42-platform/src/lib.rs +++ b/crates/sp42-platform/src/lib.rs @@ -135,8 +135,8 @@ pub use review_session::{ ReviewOpenRequest, ReviewOpenResponse, ReviewPollRequest, ReviewPollResponse, ReviewPollStatus, ReviewPrompt, ReviewPromptKind, ReviewQueueRequest, ReviewQueueResponse, ReviewReplyRequest, ReviewSession, ReviewSessionSnapshot, ReviewSessionStatus, ReviewSessionsResponse, - annotate_outline, build_article_outline, open_next_step, poll_next_step, poll_status, - truncate_outline_text, + SessionEnded, annotate_outline, build_article_outline, open_next_step, poll_next_step, + poll_status, truncate_outline_text, }; pub use review_workbench::{ PreparedRequestPreview, ReviewWorkbench, build_review_workbench, diff --git a/crates/sp42-platform/src/review_session.rs b/crates/sp42-platform/src/review_session.rs index 6ccab0b8..97967749 100644 --- a/crates/sp42-platform/src/review_session.rs +++ b/crates/sp42-platform/src/review_session.rs @@ -176,6 +176,13 @@ pub enum ReviewFeedbackTake { #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub struct ReopenRefused; +/// Refusal reason when prompts are queued against an already-ended session. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct SessionEnded { + /// Who closed the session. + pub ended_by: ReviewEndedBy, +} + impl ReviewSession { /// A fresh open session on `(wiki_id, title)` at `rev_id`. #[must_use] @@ -262,7 +269,27 @@ impl ReviewSession { /// Queue operator prompts; optionally close the session in the same /// action ("send & end"). Queued prompts survive the end and deliver on /// the next poll. - pub fn queue_prompts(&mut self, prompts: Vec, end_session: bool, now_ms: i64) { + /// + /// # Errors + /// + /// [`SessionEnded`] when the session is already closed: queueing then + /// would silently flip an ended session back to `Feedback` while + /// `ended_by` still says who closed it, so the next poll would deliver + /// with `session_ended: false` and reopen the session around the reopen + /// gate. An ended session takes new prompts only after an explicit open. + pub fn queue_prompts( + &mut self, + prompts: Vec, + end_session: bool, + now_ms: i64, + ) -> Result<(), SessionEnded> { + if self.status == ReviewSessionStatus::Ended { + return Err(SessionEnded { + // An ended session always records who ended it; fall back to + // the operator (the conservative gate) for hand-built state. + ended_by: self.ended_by.unwrap_or(ReviewEndedBy::Operator), + }); + } self.queued_prompts.extend(prompts); if end_session { self.status = ReviewSessionStatus::Ended; @@ -271,6 +298,7 @@ impl ReviewSession { self.status = ReviewSessionStatus::Feedback; } self.updated_at_ms = now_ms; + Ok(()) } /// Drain queued feedback with deliver-before-ended semantics: prompts @@ -458,6 +486,11 @@ pub struct ReviewOpenResponse { /// Attached findings whose `ref_id` matched no outline block. #[serde(default, skip_serializing_if = "Vec::is_empty")] pub unanchored_findings: Vec, + /// The session's operator/agent conversation so far — the surface the + /// agent's `--agent-reply` summaries land on; without it a stored reply + /// would be write-only. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub chat: Vec, pub next_step: String, } @@ -506,10 +539,11 @@ pub struct ReviewQueueResponse { pub struct ReviewPollRequest { pub wiki_id: String, pub title: String, - /// Server-side wait bound in milliseconds; `0` returns immediately. - /// The server clamps this to its own maximum. - #[serde(default)] - pub wait_ms: u64, + /// Server-side wait bound in milliseconds. Omitted (`None`) means the + /// server's default wait; an explicit `0` returns immediately (a + /// nonblocking status check). The server clamps this to its own maximum. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub wait_ms: Option, } /// Poll outcome status on the wire. @@ -652,7 +686,9 @@ mod tests { fn queueing_prompts_moves_the_session_to_feedback() { let mut session = ReviewSession::open("frwiki", "Exemple", 42, 1_000); - session.queue_prompts(vec![message_prompt("tighten the lede")], false, 2_000); + session + .queue_prompts(vec![message_prompt("tighten the lede")], false, 2_000) + .expect("open session should accept prompts"); assert_eq!(session.status, ReviewSessionStatus::Feedback); assert_eq!(session.queued_prompts.len(), 1); @@ -662,7 +698,9 @@ mod tests { #[test] fn take_feedback_drains_once_and_reopens() { let mut session = ReviewSession::open("frwiki", "Exemple", 42, 1_000); - session.queue_prompts(vec![message_prompt("first")], false, 2_000); + session + .queue_prompts(vec![message_prompt("first")], false, 2_000) + .expect("open session should accept prompts"); let take = session.take_feedback(3_000); let ReviewFeedbackTake::Feedback { @@ -682,7 +720,9 @@ mod tests { #[test] fn feedback_queued_before_an_end_delivers_before_ended() { let mut session = ReviewSession::open("frwiki", "Exemple", 42, 1_000); - session.queue_prompts(vec![message_prompt("last words")], true, 2_000); + session + .queue_prompts(vec![message_prompt("last words")], true, 2_000) + .expect("open session should accept prompts"); let take = session.take_feedback(3_000); let ReviewFeedbackTake::Feedback { @@ -719,7 +759,9 @@ mod tests { #[test] fn resume_clears_the_end_and_keeps_queued_feedback() { let mut session = ReviewSession::open("frwiki", "Exemple", 42, 1_000); - session.queue_prompts(vec![message_prompt("pending")], true, 2_000); + session + .queue_prompts(vec![message_prompt("pending")], true, 2_000) + .expect("open session should accept prompts"); session.resume(43, 3_000); @@ -829,6 +871,34 @@ mod tests { ); } + #[test] + fn queueing_to_an_ended_session_is_refused() { + let mut session = ReviewSession::open("frwiki", "Exemple", 42, 1_000); + session + .queue_prompts(vec![message_prompt("last words")], true, 2_000) + .expect("open session should accept the send-and-end"); + + // A late queue (stale panel, plain CLI queue) must not flip the + // ended session back to feedback around the reopen gate. + let refused = session + .queue_prompts(vec![message_prompt("too late")], false, 3_000) + .expect_err("ended session must refuse new prompts"); + assert_eq!(refused.ended_by, ReviewEndedBy::Operator); + assert_eq!(session.status, ReviewSessionStatus::Ended); + assert_eq!( + session.queued_prompts.len(), + 1, + "only the pre-end prompt stays queued" + ); + + // The pre-end feedback still delivers with the end flag intact. + let take = session.take_feedback(4_000); + let ReviewFeedbackTake::Feedback { session_ended, .. } = take else { + panic!("expected the final feedback batch"); + }; + assert!(session_ended); + } + #[test] fn attach_findings_replaces_the_overlay_and_counts() { let mut session = ReviewSession::open("frwiki", "Exemple", 42, 1_000); diff --git a/crates/sp42-server/src/review_routes.rs b/crates/sp42-server/src/review_routes.rs index 0d7e8e30..54127864 100644 --- a/crates/sp42-server/src/review_routes.rs +++ b/crates/sp42-server/src/review_routes.rs @@ -125,19 +125,14 @@ async fn resolve_open_target( Ok((target.title, rev_id, build_article_outline(&blocks))) } -/// Open a new session or resume an existing one, honoring the reopen gate: -/// a session the operator explicitly ended refuses a plain reopen. -fn open_or_resume( - entry: Option<&mut ReviewSessionEntry>, +/// The 409 etiquette response when a plain open hits a session the operator +/// explicitly ended. +fn reopen_gate_error( + session: &ReviewSession, reopen_requested: bool, title: &str, - rev_id: u64, - now_ms: i64, -) -> Result, RouteError> { - let Some(entry) = entry else { - return Ok(None); - }; - if entry.session.gate_reopen(reopen_requested).is_err() { +) -> Result<(), RouteError> { + if session.gate_reopen(reopen_requested).is_err() { return Err(( StatusCode::CONFLICT, Json(serde_json::json!({ @@ -149,6 +144,22 @@ fn open_or_resume( })), )); } + Ok(()) +} + +/// Open a new session or resume an existing one, honoring the reopen gate: +/// a session the operator explicitly ended refuses a plain reopen. +fn open_or_resume( + entry: Option<&mut ReviewSessionEntry>, + reopen_requested: bool, + title: &str, + rev_id: u64, + now_ms: i64, +) -> Result, RouteError> { + let Some(entry) = entry else { + return Ok(None); + }; + reopen_gate_error(&entry.session, reopen_requested, title)?; entry.session.resume(rev_id, now_ms); Ok(Some(entry.session.snapshot())) } @@ -163,18 +174,33 @@ pub(crate) async fn post_review_open( Json(payload): Json, ) -> Result { let session = require_review_session(&state, &headers).await?; + + // Check the reopen gate before any remote read: a plain reopen of an + // operator-ended session must get the 409 etiquette response from the + // stored session alone, not depend on (or pay for) revision/Parsoid + // lookups. The key from the locally parsed title matches the one built + // after resolution. Re-gated under the write lock below for the race. + let parsed_title = sp42_core::parse_page_target(&payload.target).title; + { + let store = state.review_sessions.read().await; + let key = ReviewSession::canonical_key(&payload.wiki_id, &parsed_title); + if let Some(entry) = store.get(&key) { + reopen_gate_error(&entry.session, payload.reopen, &parsed_title)?; + } + } + let (title, rev_id, outline) = resolve_open_target(&state, &session, &payload).await?; let key = ReviewSession::canonical_key(&payload.wiki_id, &title); let now_ms = state.clock.now_ms(); let mut store = state.review_sessions.write().await; let resumed = open_or_resume(store.get_mut(&key), payload.reopen, &title, rev_id, now_ms)?; - let (session, findings) = if let Some(snapshot) = resumed { - let findings = store + let (session, findings, chat) = if let Some(snapshot) = resumed { + let (findings, chat) = store .get(&key) - .map(|entry| entry.session.findings.clone()) + .map(|entry| (entry.session.findings.clone(), entry.session.chat.clone())) .unwrap_or_default(); - (snapshot, findings) + (snapshot, findings, chat) } else { let session = ReviewSession::open(&payload.wiki_id, &title, rev_id, now_ms); let snapshot = session.snapshot(); @@ -185,7 +211,7 @@ pub(crate) async fn post_review_open( notify: Arc::new(Notify::new()), }, ); - (snapshot, Vec::new()) + (snapshot, Vec::new(), Vec::new()) }; // Attached verification findings overlay the outline: each marker joins // its block by ref_id, so the operator sees report problems in article @@ -204,6 +230,7 @@ pub(crate) async fn post_review_open( session, outline, unanchored_findings, + chat, })) } @@ -239,7 +266,24 @@ pub(crate) async fn post_review_prompts( let queued = payload.prompts.len(); entry .session - .queue_prompts(payload.prompts, payload.end_session, now_ms); + .queue_prompts(payload.prompts, payload.end_session, now_ms) + .map_err(|ended| { + let by = match ended.ended_by { + ReviewEndedBy::Operator => "operator", + ReviewEndedBy::Agent => "agent", + }; + ( + StatusCode::CONFLICT, + Json(serde_json::json!({ + "error": format!( + "the review of {} was already ended by the {by}; open the session \ + again before queueing more feedback", + payload.title + ), + "code": "review-session-ended", + })), + ) + })?; entry.notify.notify_one(); info!( wiki_id = %payload.wiki_id, @@ -349,7 +393,9 @@ fn missing_poll_response() -> ReviewPollResponse { /// `POST /dev/review/poll` — the agent's bounded feedback wait. /// /// Drains immediately when feedback is queued; otherwise waits (clamped to -/// [`POLL_WAIT_MAX_MS`]) for a queue/end wake-up and drains once more. The +/// [`POLL_WAIT_MAX_MS`]; an explicit `wait_ms: 0` skips the wait entirely +/// for a nonblocking status check) for a queue/end wake-up and drains once +/// more. The /// wake-up uses `notify_one` permits, so feedback queued while no poll is /// waiting is picked up by the next poll without racing. pub(crate) async fn post_review_poll( @@ -363,12 +409,12 @@ pub(crate) async fn post_review_poll( return Ok(Json(missing_poll_response())); }; - let wait_ms = if payload.wait_ms == 0 { - POLL_WAIT_DEFAULT_MS - } else { - payload.wait_ms.min(POLL_WAIT_MAX_MS) - }; - let take = if take == ReviewFeedbackTake::Waiting { + // Omitted wait = the server default; an explicit 0 is the contract's + // nonblocking status check and must return the first drain immediately. + let wait_ms = payload + .wait_ms + .map_or(POLL_WAIT_DEFAULT_MS, |ms| ms.min(POLL_WAIT_MAX_MS)); + let take = if take == ReviewFeedbackTake::Waiting && wait_ms > 0 { let _ = tokio::time::timeout(Duration::from_millis(wait_ms), notify.notified()).await; match drain_feedback(&state, &key).await { Some((take, _, _)) => take, @@ -494,7 +540,8 @@ mod tests { let entry = sessions.get_mut(&key).expect("session should exist"); entry .session - .queue_prompts(vec![message_prompt("wake up")], false, 1_500); + .queue_prompts(vec![message_prompt("wake up")], false, 1_500) + .expect("open session should accept prompts"); entry.notify.notify_one(); } diff --git a/crates/sp42-server/src/tests.rs b/crates/sp42-server/src/tests.rs index b48dbcc2..6a3f450f 100644 --- a/crates/sp42-server/src/tests.rs +++ b/crates/sp42-server/src/tests.rs @@ -4145,6 +4145,86 @@ async fn review_session_loop_delivers_operator_feedback_to_the_agent() { ); } +#[tokio::test] +async fn review_queue_refuses_an_ended_session_and_replies_surface_in_open() { + let (router, cookie) = review_test_router().await; + + let (status, _) = post_review_json( + router.clone(), + &cookie, + sp42_core::routes::DEV_REVIEW_OPEN_PATH, + &serde_json::json!({"wiki_id": "frwiki", "target": "Exemple", "rev_id": 42}), + ) + .await; + assert_eq!(status, StatusCode::OK); + + // The agent posts a summary; the operator surface must be able to read + // it back (an invisible reply would be write-only). + let (status, _) = post_review_json( + router.clone(), + &cookie, + sp42_core::routes::DEV_REVIEW_REPLY_PATH, + &serde_json::json!({ + "wiki_id": "frwiki", + "title": "Exemple", + "text": "opened — start with the lede", + }), + ) + .await; + assert_eq!(status, StatusCode::OK); + let (status, open) = post_review_json( + router.clone(), + &cookie, + sp42_core::routes::DEV_REVIEW_OPEN_PATH, + &serde_json::json!({"wiki_id": "frwiki", "target": "Exemple", "rev_id": 42}), + ) + .await; + assert_eq!(status, StatusCode::OK); + assert_eq!(open["chat"][0]["role"], "agent"); + assert_eq!(open["chat"][0]["text"], "opened — start with the lede"); + + // The operator ends the session; a later plain queue must not silently + // flip it back to feedback around the reopen gate. + let (status, _) = post_review_json( + router.clone(), + &cookie, + sp42_core::routes::DEV_REVIEW_PROMPTS_PATH, + &serde_json::json!({ + "wiki_id": "frwiki", + "title": "Exemple", + "prompts": [], + "end_session": true, + }), + ) + .await; + assert_eq!(status, StatusCode::OK); + let (status, refused) = post_review_json( + router.clone(), + &cookie, + sp42_core::routes::DEV_REVIEW_PROMPTS_PATH, + &serde_json::json!({ + "wiki_id": "frwiki", + "title": "Exemple", + "prompts": [{"kind": "message", "prompt": "too late"}], + "end_session": false, + }), + ) + .await; + assert_eq!(status, StatusCode::CONFLICT); + assert_eq!(refused["code"], "review-session-ended"); + + // A nonblocking poll (explicit wait_ms: 0) reports the end immediately. + let (status, poll) = post_review_json( + router.clone(), + &cookie, + sp42_core::routes::DEV_REVIEW_POLL_PATH, + &serde_json::json!({"wiki_id": "frwiki", "title": "Exemple", "wait_ms": 0}), + ) + .await; + assert_eq!(status, StatusCode::OK); + assert_eq!(poll["status"], "ended"); +} + #[tokio::test] async fn review_findings_attach_and_overlay_the_next_open() { let (router, cookie) = review_test_router().await; diff --git a/docs/domains/references/adr/0018-review-session-bridge-contract.md b/docs/domains/references/adr/0018-review-session-bridge-contract.md index 8555c5f8..c3ade2cd 100644 --- a/docs/domains/references/adr/0018-review-session-bridge-contract.md +++ b/docs/domains/references/adr/0018-review-session-bridge-contract.md @@ -31,10 +31,13 @@ contracts (Art. 9.1). ## Decision 1. **Pure core in `sp42-platform::review_session`.** `ReviewSession` owns - every transition — `queue_prompts`, `take_feedback` (deliver-before-ended, - drain-exactly-once), `gate_reopen` (operator-ended refuses a plain - reopen), `resume`, `end`, `agent_reply` — as pure methods over injected - `now_ms`. The wire types (`ReviewOpenRequest/Response`, `ReviewPollRequest/ + every transition — `queue_prompts` (which refuses an ended session so a + stale queue cannot flip it back to feedback around the reopen gate), + `take_feedback` (deliver-before-ended, drain-exactly-once), `gate_reopen` + (operator-ended refuses a plain reopen; the route checks this against + the stored session *before* any remote revision/Parsoid read), `resume`, + `end`, `agent_reply` (chat is echoed back on the open response so + replies are not write-only) — as pure methods over injected `now_ms`. The wire types (`ReviewOpenRequest/Response`, `ReviewPollRequest/ Response`, `ReviewQueueRequest/Response`, ack/list shapes) live beside them, all embedding `REVIEW_SESSION_CONTRACT_VERSION`. The `next_step` strings are pure functions too, so the loop etiquette is unit-tested. @@ -53,7 +56,8 @@ contracts (Art. 9.1). `Notify`; queue/end call `notify_one`, whose stored permit covers feedback queued while no poll is waiting. The poll handler drains, waits (`tokio::time::timeout` over `notified()`, clamped to 55 s), and drains - once more. Bounded server-side waits with a re-arming CLI loop replace + once more; an omitted `wait_ms` uses the server default, while an + explicit `wait_ms: 0` skips the wait — a nonblocking status check. Bounded server-side waits with a re-arming CLI loop replace lavish-axi's single indefinite request — no heartbeat protocol, no proxy-timeout exposure, identical agent ergonomics. No cross-restart persistence: this is a localhost dev-bridge surface (PRD-0017 Risks). diff --git a/docs/domains/references/prd/0017-interactive-review-sessions.md b/docs/domains/references/prd/0017-interactive-review-sessions.md index 57449f95..c1a5a8e7 100644 --- a/docs/domains/references/prd/0017-interactive-review-sessions.md +++ b/docs/domains/references/prd/0017-interactive-review-sessions.md @@ -73,9 +73,11 @@ User-facing behavior: delivers first, flagged `session_ended`; only a later poll reports `ended`. An **operator**-ended session refuses a plain reopen (HTTP 409 with guidance) unless `--reopen` is passed; an **agent**-ended session - (`sp42-cli review end`) reopens freely. Every response carries `next_step` - prose telling the agent what to do — poll again, apply-and-reply, or stop - without reopening. + (`sp42-cli review end`) reopens freely. An ended session also refuses new + prompts (`review-session-ended`) until it is explicitly opened again — a + stale queue must not flip a closed session back to feedback around the + gate. Every response carries `next_step` prose telling the agent what to + do — poll again, apply-and-reply, or stop without reopening. - **Inventory**: `sp42-cli review sessions` and `GET /dev/review/sessions` list open sessions with pending-prompt counts. @@ -121,6 +123,11 @@ lanes (re-verify, bare-URL repair, inline edit). findings skipped — verified by `review_finding_markers_project_the_report_onto_review_anchors` (`sp42-citation`) +- [ ] Queueing to an ended session refuses with `review-session-ended`, and + agent chat replies are readable back on the open response, verified by + `queueing_to_an_ended_session_is_refused` (`sp42-platform`) and + `review_queue_refuses_an_ended_session_and_replies_surface_in_open` + (`sp42-server`) - [ ] Session opens, feedback queueing, findings attaches, deliveries, and ends emit `tracing` events, checkable in server logs diff --git a/docs/platform/CLI.md b/docs/platform/CLI.md index 91c91d5d..965951c2 100644 --- a/docs/platform/CLI.md +++ b/docs/platform/CLI.md @@ -211,7 +211,7 @@ sp42-cli review sessions | `open ` | Open or resume a session; returns the article outline plus `next_step`. `--rev ` pins a revision (default: latest), `--reopen` resumes a session the operator explicitly ended. | | `poll ` | Wait for feedback; re-arms bounded server waits until feedback, an end, or a missing session (stderr narrates the wait). `--agent-reply ` posts a chat line first, `--once` returns after a single wait. | | `queue ` | Queue one operator prompt (dev/test surface): `--message ` plus optional anchors `--block `, `--ref-id `, `--selected-text `; `--end` sends and ends in one action. | -| `findings ` | Attach a `verify-page --format json` report (`--report `, `-` for stdin) so its findings overlay the outline by cite id — the report's in-article view. Refused when the report's revision differs from the session's (`review-findings-revision-mismatch`). | +| `findings ` | Attach a `verify-page --format json` report (`--report `, `-` for stdin) so its findings overlay the outline by cite id — the report's in-article view. The report must describe the target page/wiki, and is refused when its revision differs from the session's (`review-findings-revision-mismatch`). | | `reply ` | Send an agent chat reply (`--message `). | | `end ` | End the session as the agent; a plain reopen stays allowed. | | `sessions` | List review sessions on the local server. | From 30bbfb022d6c81720ddbf982e8eacb067952cd5f Mon Sep 17 00:00:00 2001 From: Luis Villa Date: Mon, 13 Jul 2026 19:59:15 -0700 Subject: [PATCH 5/8] fix(review): normalize title spacing in page targets and session keys MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bare titles previously passed through parse_page_target verbatim, so "Grand_Exemple", "Grand Exemple", and a pasted URL spelled three distinct review sessions for one page. Bare titles now get the wiki-independent half of MediaWiki normalization (underscores are spaces, whitespace runs collapse) — shared as sp42_platform::normalize_title_spacing — and ReviewSession::canonical_key applies the same rule, covering direct API callers. First-letter case is deliberately not folded: case rules are wiki-dependent (Wiktionary is case-sensitive), and merging two distinct pages would be worse than splitting one. ADR-0018 §2 now states the actual normalization instead of the realpath analogy it overstated. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01QB2VfS9GXyTzVw3FayQZxf --- crates/sp42-citation/src/citation/urls.rs | 40 +++++++++++++------ crates/sp42-platform/src/lib.rs | 1 + crates/sp42-platform/src/review_session.rs | 27 ++++++++++++- crates/sp42-platform/src/wikitext_editor.rs | 13 ++++++ .../0018-review-session-bridge-contract.md | 17 ++++++-- 5 files changed, 80 insertions(+), 18 deletions(-) diff --git a/crates/sp42-citation/src/citation/urls.rs b/crates/sp42-citation/src/citation/urls.rs index 61f3ad47..430f3a91 100644 --- a/crates/sp42-citation/src/citation/urls.rs +++ b/crates/sp42-citation/src/citation/urls.rs @@ -12,6 +12,7 @@ use regex::Regex; use url::Url; use crate::errors::CitationVerificationError; +use sp42_platform::wikitext_editor::normalize_title_spacing; /// Accepted wiki language/site codes — the SSRF guard before host interpolation. static WIKI_CODE: LazyLock = LazyLock::new(|| { @@ -166,9 +167,14 @@ pub struct PageTarget { /// - `https://…/w/index.php?title=Foo&oldid=123` → title `Foo`, `rev_id` `123`. /// /// Underscores become spaces and percent-escapes are decoded, matching `MediaWiki` -/// title normalization. Anything that is not an `http(s)` URL is treated as a -/// bare title verbatim. The wiki the URL points at is *not* inspected — the caller -/// chooses the wiki separately, and a host mismatch is theirs to reconcile. +/// title normalization. A bare (non-URL) title gets the same spacing +/// normalization — underscores to spaces, whitespace runs collapsed — but no +/// percent-decoding, since a literal `%` is valid title text. First-letter +/// case is deliberately *not* folded: case rules are wiki-dependent +/// (Wiktionary titles are case-sensitive), and conflating two distinct pages +/// would be worse than treating one page as two. The wiki the URL points at +/// is *not* inspected — the caller chooses the wiki separately, and a host +/// mismatch is theirs to reconcile. #[must_use] pub fn parse_page_target(input: &str) -> PageTarget { let trimmed = input.trim(); @@ -207,20 +213,15 @@ pub fn parse_page_target(input: &str) -> PageTarget { } PageTarget { - title: trimmed.to_string(), + title: normalize_title_spacing(trimmed), rev_id: 0, } } -/// Normalize a title extracted from a URL: percent-decode, then map underscores -/// to spaces (`MediaWiki` treats them as equivalent and the action API expects -/// spaces). +/// Normalize a title extracted from a URL: percent-decode, then apply the +/// spacing normalization. fn normalize_title(raw: &str) -> String { - percent_decode_str(raw) - .decode_utf8_lossy() - .replace('_', " ") - .trim() - .to_string() + normalize_title_spacing(&percent_decode_str(raw).decode_utf8_lossy()) } #[cfg(test)] @@ -237,6 +238,21 @@ mod tests { assert_eq!(target.rev_id, 0); } + #[test] + fn parse_page_target_normalizes_bare_title_spacing() { + // Underscores are spaces in MediaWiki, and runs of either collapse + // to one — a bare title must not spell a different page (or review + // session) than the same title pasted as a URL. + assert_eq!( + parse_page_target("Grand_Exemple").title, + "Grand Exemple", + "underscore spelling matches the URL spelling" + ); + assert_eq!(parse_page_target("A __ B C").title, "A B C"); + // No percent-decoding for bare titles: a literal `%` is valid text. + assert_eq!(parse_page_target("100%_Design").title, "100% Design"); + } + #[test] fn parse_page_target_unwraps_a_wiki_url() { // The reported failure: a pasted /wiki/ URL with an underscore subpage. diff --git a/crates/sp42-platform/src/lib.rs b/crates/sp42-platform/src/lib.rs index e1f06835..bc93a355 100644 --- a/crates/sp42-platform/src/lib.rs +++ b/crates/sp42-platform/src/lib.rs @@ -203,4 +203,5 @@ pub use wikitext_editor::{ ScriptedEditorInvocation, ScriptedWikitextEditor, ScriptedWikitextNode, WikitextEditOutcome, WikitextEditRefusal, WikitextEditor, WikitextEditorError, WikitextNodeDescriptor, WikitextNodeKind, WikitextNodeLocator, WikitextPageRef, normalize_anchor_text, + normalize_title_spacing, }; diff --git a/crates/sp42-platform/src/review_session.rs b/crates/sp42-platform/src/review_session.rs index 97967749..43c61c53 100644 --- a/crates/sp42-platform/src/review_session.rs +++ b/crates/sp42-platform/src/review_session.rs @@ -202,10 +202,19 @@ impl ReviewSession { } /// Canonical store key for a target. The pair is the identity — the - /// analog of the ported loop's canonical-file-path session key. + /// analog of the ported loop's canonical-file-path session key. The + /// title component gets the wiki-independent spacing normalization + /// (underscores are spaces, whitespace runs collapse), so every + /// spelling of one page keys one session even for callers that bypass + /// the CLI's target parsing. First-letter case is not folded — case + /// rules are wiki-dependent, and merging two distinct pages would be + /// worse than splitting one. #[must_use] pub fn canonical_key(wiki_id: &str, title: &str) -> String { - format!("{wiki_id}\u{1f}{title}") + format!( + "{wiki_id}\u{1f}{}", + crate::wikitext_editor::normalize_title_spacing(title) + ) } /// Gate a reopen attempt: a session the *operator* ended refuses a plain @@ -779,6 +788,20 @@ mod tests { ); } + #[test] + fn canonical_key_collapses_title_spellings_but_not_case() { + // Underscore/space/whitespace spellings of one page → one session. + assert_eq!( + ReviewSession::canonical_key("frwiki", "Grand_Exemple"), + ReviewSession::canonical_key("frwiki", "Grand Exemple") + ); + // Case rules are wiki-dependent; never merge across case. + assert_ne!( + ReviewSession::canonical_key("frwiki", "exemple"), + ReviewSession::canonical_key("frwiki", "Exemple") + ); + } + #[test] fn outline_truncates_on_character_boundaries() { let long_text = "é".repeat(REVIEW_OUTLINE_TEXT_LIMIT + 5); diff --git a/crates/sp42-platform/src/wikitext_editor.rs b/crates/sp42-platform/src/wikitext_editor.rs index 1e8cc5e9..7ae802b5 100644 --- a/crates/sp42-platform/src/wikitext_editor.rs +++ b/crates/sp42-platform/src/wikitext_editor.rs @@ -52,6 +52,19 @@ pub struct WikitextNodeLocator { pub expected_text: String, } +/// The spacing half of `MediaWiki` title normalization, valid on every +/// wiki: underscores are spaces, and runs of whitespace collapse to one +/// space. First-letter case is deliberately *not* folded — case rules are +/// wiki-dependent (Wiktionary titles are case-sensitive), and conflating +/// two distinct pages would be worse than treating one page as two. +#[must_use] +pub fn normalize_title_spacing(raw: &str) -> String { + raw.replace('_', " ") + .split_whitespace() + .collect::>() + .join(" ") +} + /// The page revision a node-anchored operation grounds on. #[derive(Debug, Clone, PartialEq, Eq)] pub struct WikitextPageRef { diff --git a/docs/domains/references/adr/0018-review-session-bridge-contract.md b/docs/domains/references/adr/0018-review-session-bridge-contract.md index c3ade2cd..8d2d8f35 100644 --- a/docs/domains/references/adr/0018-review-session-bridge-contract.md +++ b/docs/domains/references/adr/0018-review-session-bridge-contract.md @@ -46,10 +46,19 @@ contracts (Art. 9.1). route accepts a bare title or a pasted wiki URL and unwraps it with the existing `parse_page_target` (same as verify-page), so every URL spelling of a page collapses to one session — the analog of lavish-axi's - canonical-path key, with MediaWiki title normalization playing the role of - `realpath`. The store key is `wiki_id ␟ title` (unit-separator joined, so - the pair is unambiguous). The revision is session *state* (pinned at open, - re-pinned on resume), not identity. + canonical-path key. The store key is `wiki_id ␟ title` (unit-separator + joined, so the pair is unambiguous), and `canonical_key` applies the + wiki-independent half of MediaWiki title normalization to the title + component — underscores are spaces, whitespace runs collapse — so bare + titles, URL spellings, and direct API callers all key one session. + First-letter case is deliberately *not* folded: case rules are + wiki-dependent (Wiktionary titles are case-sensitive), so folding could + merge two genuinely distinct pages; the residual cost is that a + lowercase-typed title opens a separate session on case-insensitive + wikis. Full API-side normalization (case, namespace aliases) would need + a wiki round-trip per request and is out of scope for the dev bridge. + The revision is session *state* (pinned at open, re-pinned on resume), + not identity. 3. **Server store: `AppState.review_sessions`, in-memory, with a per-session `tokio::sync::Notify`.** Each entry pairs the pure `ReviewSession` with a From b7a023fd6b6e59f1a7714ad922648f7f7a2443da Mon Sep 17 00:00:00 2001 From: Luis Villa Date: Mon, 13 Jul 2026 20:25:01 -0700 Subject: [PATCH 6/8] feat(review): ring the coordination room when a review session changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Option C of the two-channels question: the substrates stay separate — the review store keeps its durable drain-exactly-once queue, coordination keeps its live-only no-backfill sync — and they converge at the notification layer only. Every review mutation (open, queue, findings, reply, end) publishes a ReviewSignal, a seventh CoordinationMessage kind carrying just the session snapshot, to the wiki's room with the server sender id (0, never echo-suppressed). The reducer relays it without folding room state: the signal is a doorbell, not the data — panels re-fetch through the gated review routes, so a client-forged signal can only cause a harmless re-fetch, and a room with no operators drops it while late joiners recover over REST. Gives the future browser Review panel live push over the existing WebSocket with no second transport and no WS client in the agent shells. ADR-0018 §8 records the decision; PRD-0017 alternative updated from deferred to rejected-with-convergence. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01QB2VfS9GXyTzVw3FayQZxf --- crates/sp42-coordination/src/codec.rs | 13 ++++ crates/sp42-coordination/src/lib.rs | 2 +- crates/sp42-coordination/src/messages.rs | 17 ++++- crates/sp42-coordination/src/state.rs | 32 +++++++++- crates/sp42-devtools/src/preview.rs | 1 + crates/sp42-server/src/review_routes.rs | 57 +++++++++++++++-- crates/sp42-server/src/tests.rs | 62 +++++++++++++++++++ .../0018-review-session-bridge-contract.md | 20 ++++++ .../prd/0017-interactive-review-sessions.md | 22 +++++-- docs/platform/architecture.md | 4 +- 10 files changed, 216 insertions(+), 14 deletions(-) diff --git a/crates/sp42-coordination/src/codec.rs b/crates/sp42-coordination/src/codec.rs index 9c743e59..d3dba8e2 100644 --- a/crates/sp42-coordination/src/codec.rs +++ b/crates/sp42-coordination/src/codec.rs @@ -46,6 +46,19 @@ mod tests { assert_eq!(decoded, message); } + #[test] + fn review_signal_round_trips() { + let message = CoordinationMessage::ReviewSignal(crate::messages::ReviewSignal { + wiki_id: "frwiki".to_string(), + session: sp42_platform::ReviewSession::open("frwiki", "Exemple", 42, 1_000).snapshot(), + }); + + let bytes = encode_message(&message).expect("encoding should succeed"); + let decoded = decode_message(&bytes).expect("decoding should succeed"); + + assert_eq!(decoded, message); + } + fn action_strategy() -> impl Strategy { prop_oneof![ Just(Action::Rollback), diff --git a/crates/sp42-coordination/src/lib.rs b/crates/sp42-coordination/src/lib.rs index 1277e90c..8e541fd4 100644 --- a/crates/sp42-coordination/src/lib.rs +++ b/crates/sp42-coordination/src/lib.rs @@ -22,7 +22,7 @@ pub use codec::{decode_message, encode_message}; pub use errors::{CodecError, CoordinationError}; pub use messages::{ ActionBroadcast, CoordinationMessage, CoordinationRoomSummary, CoordinationSnapshot, EditClaim, - FlaggedEdit, PresenceHeartbeat, RaceResolution, ScoreDelta, + FlaggedEdit, PresenceHeartbeat, RaceResolution, ReviewSignal, ScoreDelta, }; pub use runtime::{CoordinationRuntime, CoordinationRuntimeStatus}; pub use state::{CoordinationState, CoordinationStateSummary}; diff --git a/crates/sp42-coordination/src/messages.rs b/crates/sp42-coordination/src/messages.rs index 099eadb9..dbdb30ee 100644 --- a/crates/sp42-coordination/src/messages.rs +++ b/crates/sp42-coordination/src/messages.rs @@ -1,7 +1,7 @@ //! Coordination protocol payloads and room summary contracts. use serde::{Deserialize, Serialize}; -use sp42_platform::Action; +use sp42_platform::{Action, ReviewSessionSnapshot}; #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub enum CoordinationMessage { @@ -11,6 +11,21 @@ pub enum CoordinationMessage { PresenceHeartbeat(PresenceHeartbeat), FlaggedEdit(FlaggedEdit), RaceResolution(RaceResolution), + ReviewSignal(ReviewSignal), +} + +/// Server-originated hint that a review session changed (PRD-0017, +/// ADR-0018 §8): a live re-fetch cue for operator panels watching the +/// wiki's room. Advisory by design — the review-session store behind the +/// gated `/dev/review` routes stays the source of truth, the reducer +/// relays this kind without folding any room state, and a spoofed signal +/// can only cause a harmless re-fetch. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct ReviewSignal { + pub wiki_id: String, + /// Session summary so a panel can badge (status, pending prompts, + /// findings count) without a round-trip. Never authoritative. + pub session: ReviewSessionSnapshot, } #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] diff --git a/crates/sp42-coordination/src/state.rs b/crates/sp42-coordination/src/state.rs index b1d48ed4..a8f8633e 100644 --- a/crates/sp42-coordination/src/state.rs +++ b/crates/sp42-coordination/src/state.rs @@ -136,6 +136,13 @@ impl CoordinationState { self.recent_actions.drain(0..overflow); } } + CoordinationMessage::ReviewSignal(signal) => { + // Deliberately stateless: the signal is a live re-fetch hint + // for panels, and the review-session store behind the gated + // review routes stays the source of truth (ADR-0018 §8). + // Folding it here would create a second, spoofable copy. + drop(signal); + } } true @@ -202,6 +209,7 @@ fn message_wiki_id(message: &CoordinationMessage) -> &str { CoordinationMessage::PresenceHeartbeat(heartbeat) => &heartbeat.wiki_id, CoordinationMessage::FlaggedEdit(flagged) => &flagged.wiki_id, CoordinationMessage::RaceResolution(resolution) => &resolution.wiki_id, + CoordinationMessage::ReviewSignal(signal) => &signal.wiki_id, } } @@ -209,12 +217,34 @@ fn message_wiki_id(message: &CoordinationMessage) -> &str { mod tests { use crate::messages::{ ActionBroadcast, CoordinationMessage, EditClaim, FlaggedEdit, PresenceHeartbeat, - RaceResolution, ScoreDelta, + RaceResolution, ReviewSignal, ScoreDelta, }; use sp42_platform::Action; use super::CoordinationState; + fn review_signal(wiki_id: &str) -> ReviewSignal { + ReviewSignal { + wiki_id: wiki_id.to_string(), + session: sp42_platform::ReviewSession::open(wiki_id, "Exemple", 42, 1_000).snapshot(), + } + } + + #[test] + fn review_signal_relays_without_folding_room_state() { + let mut state = CoordinationState::new("frwiki"); + let before = state.summary(); + + // Same wiki: accepted (so the relay fans it out to panels)... + assert!(state.apply(CoordinationMessage::ReviewSignal(review_signal("frwiki")))); + // ...but deliberately stateless — the review-session store behind + // the gated review routes is the source of truth (ADR-0018 §8). + assert_eq!(state.summary(), before); + + // Other wiki: rejected like every other kind. + assert!(!state.apply(CoordinationMessage::ReviewSignal(review_signal("enwiki")))); + } + #[test] fn ignores_messages_for_other_wikis() { let mut state = CoordinationState::new("frwiki"); diff --git a/crates/sp42-devtools/src/preview.rs b/crates/sp42-devtools/src/preview.rs index 0b564ea2..fa4254af 100644 --- a/crates/sp42-devtools/src/preview.rs +++ b/crates/sp42-devtools/src/preview.rs @@ -211,6 +211,7 @@ pub fn dev_coordination_message_label(message: &CoordinationMessage) -> &'static CoordinationMessage::PresenceHeartbeat(_) => "PresenceHeartbeat", CoordinationMessage::FlaggedEdit(_) => "FlaggedEdit", CoordinationMessage::RaceResolution(_) => "RaceResolution", + CoordinationMessage::ReviewSignal(_) => "ReviewSignal", } } diff --git a/crates/sp42-server/src/review_routes.rs b/crates/sp42-server/src/review_routes.rs index 54127864..12e85214 100644 --- a/crates/sp42-server/src/review_routes.rs +++ b/crates/sp42-server/src/review_routes.rs @@ -17,6 +17,7 @@ use axum::{ http::{HeaderMap, StatusCode}, response::IntoResponse, }; +use sp42_coordination::{CoordinationMessage, ReviewSignal}; use sp42_core::{ REVIEW_SESSION_CONTRACT_VERSION, ReviewAckResponse, ReviewBlockOutline, ReviewEndRequest, ReviewEndedBy, ReviewFeedbackTake, ReviewFindingsRequest, ReviewFindingsResponse, @@ -26,7 +27,9 @@ use sp42_core::{ open_next_step, poll_next_step, poll_status, }; use tokio::sync::{Notify, RwLock}; -use tracing::info; +use tracing::{info, warn}; + +use crate::coordination::CoordinationEnvelope; use crate::citation_routes::fetch_latest_revid; use crate::config_for_state_wiki; @@ -62,6 +65,36 @@ pub(crate) fn new_review_session_store() -> SharedReviewSessions { type RouteError = (StatusCode, Json); +/// Sender id for server-originated room messages. Client ids are handed out +/// from 1, so 0 is never echo-suppressed by the relay. +pub(crate) const SERVER_SENDER_ID: u64 = 0; + +/// Publish the live re-fetch hint for a changed review session to the +/// wiki's coordination room (ADR-0018 §8). Best-effort by design: a room no +/// operator has joined drops the signal, and panels recover through the +/// review routes — the signal is a doorbell, never the data. +async fn publish_review_signal(state: &AppState, session: &sp42_core::ReviewSessionSnapshot) { + let message = CoordinationMessage::ReviewSignal(ReviewSignal { + wiki_id: session.wiki_id.clone(), + session: session.clone(), + }); + match sp42_coordination::encode_message(&message) { + Ok(payload) => { + let _subscribers = state + .coordination + .publish( + &session.wiki_id, + CoordinationEnvelope { + sender_id: SERVER_SENDER_ID, + payload, + }, + ) + .await; + } + Err(error) => warn!(%error, "review signal failed to encode"), + } +} + /// Require an authenticated bridge session with a valid CSRF header — /// the shared gate for every review POST route. async fn require_review_session( @@ -213,6 +246,8 @@ pub(crate) async fn post_review_open( ); (snapshot, Vec::new(), Vec::new()) }; + drop(store); + publish_review_signal(&state, &session).await; // Attached verification findings overlay the outline: each marker joins // its block by ref_id, so the operator sees report problems in article // context rather than as detached text. @@ -285,6 +320,9 @@ pub(crate) async fn post_review_prompts( ) })?; entry.notify.notify_one(); + let snapshot = entry.session.snapshot(); + drop(store); + publish_review_signal(&state, &snapshot).await; info!( wiki_id = %payload.wiki_id, title = %payload.title, @@ -294,7 +332,7 @@ pub(crate) async fn post_review_prompts( ); Ok(Json(ReviewQueueResponse { contract_version: REVIEW_SESSION_CONTRACT_VERSION, - session: entry.session.snapshot(), + session: snapshot, queued, })) } @@ -332,6 +370,9 @@ pub(crate) async fn post_review_findings( })), ) })?; + let snapshot = entry.session.snapshot(); + drop(store); + publish_review_signal(&state, &snapshot).await; info!( wiki_id = %payload.wiki_id, title = %payload.title, @@ -340,7 +381,7 @@ pub(crate) async fn post_review_findings( ); Ok(Json(ReviewFindingsResponse { contract_version: REVIEW_SESSION_CONTRACT_VERSION, - session: entry.session.snapshot(), + session: snapshot, attached, })) } @@ -450,9 +491,12 @@ pub(crate) async fn post_review_reply( return Err(missing_session_error(&payload.wiki_id, &payload.title)); }; entry.session.agent_reply(&payload.text, now_ms); + let snapshot = entry.session.snapshot(); + drop(store); + publish_review_signal(&state, &snapshot).await; Ok(Json(ReviewAckResponse { contract_version: REVIEW_SESSION_CONTRACT_VERSION, - session: entry.session.snapshot(), + session: snapshot, })) } @@ -472,6 +516,9 @@ pub(crate) async fn post_review_end( }; entry.session.end(ReviewEndedBy::Agent, now_ms); entry.notify.notify_one(); + let snapshot = entry.session.snapshot(); + drop(store); + publish_review_signal(&state, &snapshot).await; info!( wiki_id = %payload.wiki_id, title = %payload.title, @@ -479,7 +526,7 @@ pub(crate) async fn post_review_end( ); Ok(Json(ReviewAckResponse { contract_version: REVIEW_SESSION_CONTRACT_VERSION, - session: entry.session.snapshot(), + session: snapshot, })) } diff --git a/crates/sp42-server/src/tests.rs b/crates/sp42-server/src/tests.rs index 6a3f450f..87b005c7 100644 --- a/crates/sp42-server/src/tests.rs +++ b/crates/sp42-server/src/tests.rs @@ -4145,6 +4145,68 @@ async fn review_session_loop_delivers_operator_feedback_to_the_agent() { ); } +#[tokio::test] +async fn review_mutations_ring_the_coordination_room() { + let state = test_state(); + state.sessions.write().await.insert( + "review-agent".to_string(), + test_session("Reviewer", "secret-token", now_ms()), + ); + // A panel is listening on the wiki's room before the agent acts. + let mut room = state.coordination.subscribe("frwiki").await; + let router = build_router(state); + let cookie = format!("{SESSION_COOKIE_NAME}=review-agent"); + + let (status, _) = post_review_json( + router.clone(), + &cookie, + sp42_core::routes::DEV_REVIEW_OPEN_PATH, + &serde_json::json!({"wiki_id": "frwiki", "target": "Exemple", "rev_id": 42}), + ) + .await; + assert_eq!(status, StatusCode::OK); + + let envelope = tokio::time::timeout(Duration::from_secs(5), room.recv()) + .await + .expect("open should ring the room promptly") + .expect("room channel should stay open"); + assert_eq!(envelope.sender_id, crate::review_routes::SERVER_SENDER_ID); + let message = + sp42_coordination::decode_message(&envelope.payload).expect("signal should decode"); + let sp42_coordination::CoordinationMessage::ReviewSignal(signal) = message else { + panic!("expected a review signal, got {message:?}"); + }; + assert_eq!(signal.wiki_id, "frwiki"); + assert_eq!(signal.session.title, "Exemple"); + assert_eq!(signal.session.rev_id, 42); + + // Queueing feedback rings again, carrying the pending count so a panel + // can badge without a round-trip. + let (status, _) = post_review_json( + router.clone(), + &cookie, + sp42_core::routes::DEV_REVIEW_PROMPTS_PATH, + &serde_json::json!({ + "wiki_id": "frwiki", + "title": "Exemple", + "prompts": [{"kind": "message", "prompt": "check the lede"}], + "end_session": false, + }), + ) + .await; + assert_eq!(status, StatusCode::OK); + let envelope = tokio::time::timeout(Duration::from_secs(5), room.recv()) + .await + .expect("queue should ring the room promptly") + .expect("room channel should stay open"); + let message = + sp42_coordination::decode_message(&envelope.payload).expect("signal should decode"); + let sp42_coordination::CoordinationMessage::ReviewSignal(signal) = message else { + panic!("expected a review signal, got {message:?}"); + }; + assert_eq!(signal.session.pending_prompts, 1); +} + #[tokio::test] async fn review_queue_refuses_an_ended_session_and_replies_surface_in_open() { let (router, cookie) = review_test_router().await; diff --git a/docs/domains/references/adr/0018-review-session-bridge-contract.md b/docs/domains/references/adr/0018-review-session-bridge-contract.md index 8d2d8f35..a1f3e26f 100644 --- a/docs/domains/references/adr/0018-review-session-bridge-contract.md +++ b/docs/domains/references/adr/0018-review-session-bridge-contract.md @@ -112,6 +112,26 @@ contracts (Art. 9.1). `unanchored_findings` rather than dropping. Attaching wakes no poll — findings are operator-surface input, not agent feedback. +8. **Live panel updates ride the coordination room as an advisory signal — + the substrates stay separate.** The review store and the coordination + substrate make opposite delivery guarantees: review feedback is a + durable, drain-exactly-once queue keyed per page; coordination is + live-only state sync keyed per wiki with no backfill (ADR-0023 §2). + Folding one into the other would break one contract or the other, so + they converge only at the notification layer: every review mutation + (open, queue, findings, reply, end) publishes a `ReviewSignal` — a + seventh `CoordinationMessage` kind extending ADR-0023's registry — to + the wiki's room with the server sender id (`0`, never a client id, so + it is never echo-suppressed). The signal is a doorbell, not the data: + it carries only the session snapshot for badging, the reducer relays + it without folding any room state, and panels re-fetch through the + gated review routes, which remain the sole source of truth. This makes + spoofing harmless (a client-forged signal can only cause a re-fetch), + keeps the agent side on the long-poll (no WebSocket client in + `sp42-cli`/`sp42-mcp`), and loses nothing when no operator is in the + room — an unheard doorbell has no one to notify, and a late-joining + panel lists sessions over REST. + ## Consequences - The loop's semantics are testable without a server, and the server tests diff --git a/docs/domains/references/prd/0017-interactive-review-sessions.md b/docs/domains/references/prd/0017-interactive-review-sessions.md index c1a5a8e7..658065ea 100644 --- a/docs/domains/references/prd/0017-interactive-review-sessions.md +++ b/docs/domains/references/prd/0017-interactive-review-sessions.md @@ -80,6 +80,11 @@ User-facing behavior: do — poll again, apply-and-reply, or stop without reopening. - **Inventory**: `sp42-cli review sessions` and `GET /dev/review/sessions` list open sessions with pending-prompt counts. +- **Live signal**: every session mutation rings the wiki's coordination + room with an advisory `ReviewSignal` (session snapshot only — a doorbell, + not the data), so a browser panel gets push updates over the existing + WebSocket without a second push transport; it re-fetches the review + routes, which stay the source of truth (ADR-0018 §8). Everything stays read-only with respect to the wiki: a review session never edits anything. Acting on feedback rides the existing operator-confirmed @@ -128,6 +133,10 @@ lanes (re-verify, bare-URL repair, inline edit). `queueing_to_an_ended_session_is_refused` (`sp42-platform`) and `review_queue_refuses_an_ended_session_and_replies_surface_in_open` (`sp42-server`) +- [ ] Every review mutation publishes an advisory `ReviewSignal` to the + wiki's coordination room, relayed without folding room state, verified by + `review_mutations_ring_the_coordination_room` (`sp42-server`) and + `review_signal_relays_without_folding_room_state` (`sp42-coordination`) - [ ] Session opens, feedback queueing, findings attaches, deliveries, and ends emit `tracing` events, checkable in server logs @@ -139,10 +148,15 @@ lanes (re-verify, bare-URL repair, inline edit). anchors (truncated CSS selectors + DOM ranges) are weaker than Parsoid block/cite anchors for wiki work; and SP42 already owns the localhost server, session/CSRF runtime, and Parsoid read path the loop needs. -- **WebSocket feedback channel** (reuse the coordination-room substrate): - deferred — a long-poll matches the agent's blocking-command ergonomics - (one CLI invocation = one rendezvous) and needs no client protocol. The - browser annotation panel may later ride the existing WebSocket/SSE paths. +- **WebSocket feedback channel** (move the whole loop onto the + coordination-room substrate): rejected — a long-poll matches the agent's + blocking-command ergonomics (one CLI invocation = one rendezvous) and + needs no client protocol, and the substrates' delivery guarantees are + opposites (durable exactly-once feedback queue vs. live-only no-backfill + state sync, ADR-0023). Instead the browser side converges at the + notification layer only: every review mutation rings the wiki's + coordination room with an advisory `ReviewSignal` the panel answers by + re-fetching the review routes (ADR-0018 §8). - **Key sessions by URL string** (hash the raw URL like lavish-axi hashes paths): rejected — two URLs for the same page (percent-encoding, underscores, `oldid` forms) must resolve to one session, so the canonical diff --git a/docs/platform/architecture.md b/docs/platform/architecture.md index 5fd1c1eb..49566490 100644 --- a/docs/platform/architecture.md +++ b/docs/platform/architecture.md @@ -152,7 +152,7 @@ Reading notes: | `sp42-app` | shell | [ADR-0004](adr/0004-crate-boundary-collaboration-model.md), [ADR-0005](adr/0005-design-system-shared-component-layer.md), [ADR-0008](../domains/references/adr/0008-citation-verification-contract.md), [ADR-0010](adr/0010-operator-confirmed-content-proposals.md), [ADR-0012](adr/0012-frontend-e2e-testing-approach.md), [ADR-0013](adr/0013-layered-platform-domain-architecture.md) | [PRD-0002](../domains/patrolling/prd/0002-patrol-review-workflow.md), [PRD-0006](../domains/patrolling/prd/0006-multi-operator-coordination.md), [PRD-0010](../domains/references/prd/0010-citation-verification-mcp-surface.md), [PRD-0014](../domains/references/prd/0014-citation-repair-insertion-browser-surface.md) | | | `sp42-citation` | domain | [ADR-0013](adr/0013-layered-platform-domain-architecture.md) | [PRD-0001](../domains/references/prd/0001-citation-verification.md), [PRD-0008](../domains/references/prd/0008-bare-url-repair.md), [PRD-0010](../domains/references/prd/0010-citation-verification-mcp-surface.md), [PRD-0014](../domains/references/prd/0014-citation-repair-insertion-browser-surface.md), [PRD-0016](../domains/assessment/prd/0016-ga-evidence-appendix-renderer.md), [PRD-0017](../domains/references/prd/0017-interactive-review-sessions.md) | | | `sp42-cli` | shell | [ADR-0004](adr/0004-crate-boundary-collaboration-model.md), [ADR-0005](adr/0005-design-system-shared-component-layer.md), [ADR-0013](adr/0013-layered-platform-domain-architecture.md), [ADR-0015](../domains/references/adr/0015-rules-compliant-read-only-fetch-edge.md), [ADR-0018](../domains/references/adr/0018-review-session-bridge-contract.md) | [PRD-0001](../domains/references/prd/0001-citation-verification.md), [PRD-0017](../domains/references/prd/0017-interactive-review-sessions.md) | | -| `sp42-coordination` | platform | [ADR-0004](adr/0004-crate-boundary-collaboration-model.md), [ADR-0013](adr/0013-layered-platform-domain-architecture.md), [ADR-0023](adr/0023-coordination-contract.md) | [PRD-0006](../domains/patrolling/prd/0006-multi-operator-coordination.md) | | +| `sp42-coordination` | platform | [ADR-0004](adr/0004-crate-boundary-collaboration-model.md), [ADR-0013](adr/0013-layered-platform-domain-architecture.md), [ADR-0023](adr/0023-coordination-contract.md) | [PRD-0006](../domains/patrolling/prd/0006-multi-operator-coordination.md), [PRD-0017](../domains/references/prd/0017-interactive-review-sessions.md) | | | `sp42-core` | hybrid | [ADR-0001](adr/0001-foundational-decisions.md), [ADR-0003](adr/0003-node-anchored-wikitext-editing.md), [ADR-0004](adr/0004-crate-boundary-collaboration-model.md), [ADR-0005](adr/0005-design-system-shared-component-layer.md), [ADR-0006](adr/0006-using-llms.md), [ADR-0007](../domains/references/adr/0007-citation-verification-semantics.md), [ADR-0008](../domains/references/adr/0008-citation-verification-contract.md), [ADR-0009](../domains/references/adr/0009-citation-source-snapshot-storage.md), [ADR-0010](adr/0010-operator-confirmed-content-proposals.md), [ADR-0011](../domains/references/adr/0011-article-citation-verification.md), [ADR-0013](adr/0013-layered-platform-domain-architecture.md), [ADR-0015](../domains/references/adr/0015-rules-compliant-read-only-fetch-edge.md), [ADR-0021](adr/0021-scoring-and-ranking-contract.md) | [PRD-0001](../domains/references/prd/0001-citation-verification.md), [PRD-0002](../domains/patrolling/prd/0002-patrol-review-workflow.md), [PRD-0004](../domains/patrolling/prd/0004-reviewer-actions-on-wikimedia.md), [PRD-0005](../domains/patrolling/prd/0005-operator-identity-and-session.md), [PRD-0007](../domains/references/prd/0007-llm-output-benchmarking.md), [PRD-0015](../domains/assessment/prd/0015-article-stability-signal.md), [PRD-0016](../domains/assessment/prd/0016-ga-evidence-appendix-renderer.md) | Hybrid exemption — re-export facade being split into platform/domain crates and retired ([ADR-0013](adr/0013-layered-platform-domain-architecture.md)) | | `sp42-desktop` | shell | [ADR-0004](adr/0004-crate-boundary-collaboration-model.md), [ADR-0005](adr/0005-design-system-shared-component-layer.md), [ADR-0013](adr/0013-layered-platform-domain-architecture.md) | — | | | `sp42-devtools` | shell | [ADR-0004](adr/0004-crate-boundary-collaboration-model.md), [ADR-0013](adr/0013-layered-platform-domain-architecture.md) | [PRD-0006](../domains/patrolling/prd/0006-multi-operator-coordination.md) | | @@ -218,4 +218,4 @@ Reading notes: | [PRD-0014](../domains/references/prd/0014-citation-repair-insertion-browser-surface.md) | Citation repair and insertion — browser surface | Accepted | references | ADR-0003, ADR-0006, ADR-0009, ADR-0010, ADR-0019 | | [PRD-0015](../domains/assessment/prd/0015-article-stability-signal.md) | Article stability signal | Discussion | assessment | ADR-0006, ADR-0007, ADR-0009, ADR-0011, ADR-0014 | | [PRD-0016](../domains/assessment/prd/0016-ga-evidence-appendix-renderer.md) | GA evidence appendix renderer | Discussion | assessment | ADR-0003, ADR-0007, ADR-0010, ADR-0011, ADR-0013, ADR-0018 | -| [PRD-0017](../domains/references/prd/0017-interactive-review-sessions.md) | Interactive review sessions — agent↔operator feedback loop on a page | Draft | references | ADR-0018 | +| [PRD-0017](../domains/references/prd/0017-interactive-review-sessions.md) | Interactive review sessions — agent↔operator feedback loop on a page | Draft | references | ADR-0018, ADR-0023 | From 4a4d6cbc1d84469dbf6b1d19b282a51d5ffdc2b7 Mon Sep 17 00:00:00 2001 From: Luis Villa Date: Mon, 13 Jul 2026 23:21:34 -0700 Subject: [PATCH 7/8] fix(review): address Codex round two (end attribution, drain signal) - Ending an already-ended session is a no-op: an agent cleanup `review end` after an operator send-and-end kept flipping ended_by to Agent, silently lifting the operator-ended reopen gate. - A poll that delivers feedback now rings the coordination room with the post-drain snapshot, so a panel's pending badge does not sit on the queueing signal's stale count until some later mutation. Codex's third comment (canonicalize titles before keying) reviewed the pre-30bbfb0 code and was already fixed by that commit. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01QB2VfS9GXyTzVw3FayQZxf --- crates/sp42-platform/src/review_session.rs | 26 +++++++++++++++++++++- crates/sp42-server/src/review_routes.rs | 14 ++++++++++++ crates/sp42-server/src/tests.rs | 23 +++++++++++++++++++ 3 files changed, 62 insertions(+), 1 deletion(-) diff --git a/crates/sp42-platform/src/review_session.rs b/crates/sp42-platform/src/review_session.rs index 43c61c53..af1cdd44 100644 --- a/crates/sp42-platform/src/review_session.rs +++ b/crates/sp42-platform/src/review_session.rs @@ -345,8 +345,15 @@ impl ReviewSession { self.updated_at_ms = now_ms; } - /// Close the session, recording who closed it. + /// Close the session, recording who closed it. Ending an already-ended + /// session is a no-op: the first closer keeps the attribution, so an + /// agent's cleanup `end` after an operator's send-and-end cannot + /// downgrade the operator-ended reopen gate to a freely-reopenable + /// agent end. pub fn end(&mut self, by: ReviewEndedBy, now_ms: i64) { + if self.status == ReviewSessionStatus::Ended { + return; + } self.status = ReviewSessionStatus::Ended; self.ended_by = Some(by); self.updated_at_ms = now_ms; @@ -894,6 +901,23 @@ mod tests { ); } + #[test] + fn agent_end_does_not_downgrade_an_operator_end() { + let mut session = ReviewSession::open("frwiki", "Exemple", 42, 1_000); + session.end(ReviewEndedBy::Operator, 2_000); + + // Agent cleanup after the operator already closed the review must + // not flip the attribution and lift the reopen gate. + session.end(ReviewEndedBy::Agent, 3_000); + + assert_eq!(session.ended_by, Some(ReviewEndedBy::Operator)); + assert_eq!(session.gate_reopen(false), Err(ReopenRefused)); + assert_eq!( + session.updated_at_ms, 2_000, + "a no-op end leaves the session untouched" + ); + } + #[test] fn queueing_to_an_ended_session_is_refused() { let mut session = ReviewSession::open("frwiki", "Exemple", 42, 1_000); diff --git a/crates/sp42-server/src/review_routes.rs b/crates/sp42-server/src/review_routes.rs index 12e85214..ade9b129 100644 --- a/crates/sp42-server/src/review_routes.rs +++ b/crates/sp42-server/src/review_routes.rs @@ -473,6 +473,20 @@ pub(crate) async fn post_review_poll( "review poll delivered" ); } + // A delivered batch changed the session (pending count, maybe status) — + // ring the room so a panel's badge does not sit on the stale count from + // the queueing signal until some later mutation. + if matches!(take, ReviewFeedbackTake::Feedback { .. }) { + let snapshot = state + .review_sessions + .read() + .await + .get(&key) + .map(|entry| entry.session.snapshot()); + if let Some(snapshot) = snapshot { + publish_review_signal(&state, &snapshot).await; + } + } Ok(Json(poll_response(take, &title))) } diff --git a/crates/sp42-server/src/tests.rs b/crates/sp42-server/src/tests.rs index 87b005c7..53768ace 100644 --- a/crates/sp42-server/src/tests.rs +++ b/crates/sp42-server/src/tests.rs @@ -4205,6 +4205,29 @@ async fn review_mutations_ring_the_coordination_room() { panic!("expected a review signal, got {message:?}"); }; assert_eq!(signal.session.pending_prompts, 1); + + // Draining the feedback rings once more so the panel's badge does not + // sit on the stale pending count. + let (status, poll) = post_review_json( + router.clone(), + &cookie, + sp42_core::routes::DEV_REVIEW_POLL_PATH, + &serde_json::json!({"wiki_id": "frwiki", "title": "Exemple", "wait_ms": 1}), + ) + .await; + assert_eq!(status, StatusCode::OK); + assert_eq!(poll["status"], "feedback"); + let envelope = tokio::time::timeout(Duration::from_secs(5), room.recv()) + .await + .expect("a drained poll should ring the room promptly") + .expect("room channel should stay open"); + let message = + sp42_coordination::decode_message(&envelope.payload).expect("signal should decode"); + let sp42_coordination::CoordinationMessage::ReviewSignal(signal) = message else { + panic!("expected a review signal, got {message:?}"); + }; + assert_eq!(signal.session.pending_prompts, 0); + assert_eq!(signal.session.status, sp42_core::ReviewSessionStatus::Open); } #[tokio::test] From 3acb9b130575b641dc687a9edac4ff2371b4c202 Mon Sep 17 00:00:00 2001 From: Luis Villa Date: Tue, 14 Jul 2026 08:11:01 -0700 Subject: [PATCH 8/8] fix(assessment): cover UnresolvedShortCite in the GA appendix skip copy origin/main does not compile: PR #153 added SkippedReason::UnresolvedShortCite while PR #154 added an exhaustive match over SkippedReason in ga_appendix.rs; they merged independently and the semantic conflict landed on main. This adds the missing arm (with disclosed-not-guessed copy) so this branch builds; main needs the same one-arm fix. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01QB2VfS9GXyTzVw3FayQZxf --- crates/sp42-assessment/src/copy.rs | 5 +++++ crates/sp42-assessment/src/ga_appendix.rs | 3 +++ crates/sp42-platform/src/review_session.rs | 1 + 3 files changed, 9 insertions(+) diff --git a/crates/sp42-assessment/src/copy.rs b/crates/sp42-assessment/src/copy.rs index b82715ba..ca4b9a22 100644 --- a/crates/sp42-assessment/src/copy.rs +++ b/crates/sp42-assessment/src/copy.rs @@ -114,6 +114,11 @@ pub const SKIPPED_BOOK_UNRESOLVED: &str = /// problem) — the book may well have a record; nothing is implied about it. pub const SKIPPED_BOOK_LOOKUP_FAILED: &str = "cites a book whose catalog lookup did not complete — the tool could not reach the catalog; nothing is implied about the book"; +/// Skip reason: a short cite ({{sfn}}-style) whose bibliography entry could +/// not be resolved to a usable identifier — never guessed, disclosed. +pub const SKIPPED_SHORT_CITE_UNRESOLVED: &str = + "is a short cite the tool could not resolve to a bibliography entry with a usable identifier"; + /// Books consulted section heading. pub const BUCKET_BOOKS_CONSULTED: &str = "==== Books consulted ===="; diff --git a/crates/sp42-assessment/src/ga_appendix.rs b/crates/sp42-assessment/src/ga_appendix.rs index beed90c0..a244f550 100644 --- a/crates/sp42-assessment/src/ga_appendix.rs +++ b/crates/sp42-assessment/src/ga_appendix.rs @@ -321,6 +321,9 @@ fn render_skipped_section(output: &mut String, report: &sp42_citation::PageVerif crate::copy::SKIPPED_BOOK_UNRESOLVED } } + sp42_citation::SkippedReason::UnresolvedShortCite => { + crate::copy::SKIPPED_SHORT_CITE_UNRESOLVED + } }); // Which book: identifiers (and cited page) make multiple books // under one ref distinguishable (Codex round 4, PR 154). diff --git a/crates/sp42-platform/src/review_session.rs b/crates/sp42-platform/src/review_session.rs index af1cdd44..70698c9c 100644 --- a/crates/sp42-platform/src/review_session.rs +++ b/crates/sp42-platform/src/review_session.rs @@ -822,6 +822,7 @@ mod tests { ref_text: "[1]".to_string(), named: false, is_bare_url_ref: false, + short_cite_unresolved: false, }], block_kind: BlockKind::Paragraph, block_ordinal: 7,