diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 0342f0d10..4d34d29aa 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -170,7 +170,7 @@ server.py (Starlette+uvicorn) ← HTTP + WebSocket on configurable host:port (de ├── local_model_autostart.py ← Local model startup helper ├── deep_self_review.py ← Deep self-review: Generated Deep Self-Review Atlas repository context + full memory whitelist → 1M-context model. Guaranteed-fit assembly (v6.27.1): the in-prompt OMITTED-files section is bounded (counts per reason + capped sample; full coverage stays in the persisted atlas manifest) and reserved inside the atlas fixed budget; an atlas that did not assemble (`atlas_assembly_failed`: over hard budget, or a REQUIRED artifact omitted) retries once with the compact manifest and otherwise returns no pack at all, and a final-shrink rebuild (tighter hard budget by the measured overage) replaces the historical fatal 'Review pack too large' error — the gate remains as the fail-closed last assertion. File selection is ranked by import-graph centrality (reverse-import in-degree from code_intelligence, additive bonus ≤600, deep-review-only) ├── review.py ← Code collection, complexity metrics, pre-commit review - ├── review_execution_projection.py ← Pure read-side `executions[]` projection shared by Skill Review, plan review, and task acceptance; it admits only returned usage or a resolved delegated route and never invents execution, money, or profile facts. + ├── review_execution_projection.py ← Pure read-side presentation leaf shared by Skill Review, plan review, and task acceptance: the `executions[]` projection (admits only returned usage or a resolved delegated route; never invents execution, money, or profile facts) and the bounded owner-facing finding rows (`projected_finding_row` + `MAX_PROJECTED_ACTOR_FINDINGS` — row-count bound, generous 2000-char string bound after redaction, unknown shapes ship as a disclosed JSON row). ├── preflight_runner.py ← Hermetic reviewed-change pytest gate: disposable git worktree, ONE hardened candidate capture (`git diff --binary --no-ext-diff --no-textconv --no-color --src-prefix=a/ --dst-prefix=b/ HEAD` applied as RAW BYTES, identically for every index state including an unfinished merge — whose unmerged entries the former staged+unstaged pair could only render as contentless stubs and `--cc` hunks `git apply` rejects or silently drops; capture/apply failure is the typed hard block PREFLIGHT_CANDIDATE_ASSEMBLY, never a test verdict; the honest bound is an exact tracked projection of the live worktree plus its safe non-ignored untracked entries), temp data/settings/pycache env, and live OUROBOROS_*/secret-class scrub so review tests cannot inherit operator behavior or mutate live repo/data. Runs the node web-tests lane (`preflight_node.py`) then CI's own two-pass split in that one worktree (parallel `not serial` with `-n auto --dist loadscope --max-worker-restart=0 --timeout=300`, then a flag-free `serial` pass) under ONE total budget, with `LANE_EXCLUSION_EXPR` as the marker-lane SSOT; a dead xdist worker and a missing xdist/timeout plugin are distinct named hard blocks, never a retry and never a silent serial fallback ├── preflight_node.py ← Node lane of the hermetic commit gate (called from `preflight_runner.run_hermetic_pytest` once the candidate is assembled, as the first consumer of the shared budget): when the candidate tree carries `web/tests/*.test.js`, runs that browser-module suite once with `node --test` from the candidate's `web/` (bundled signed node first, then PATH; version floor 20.11) inside the same ProcessContainer/reap/bounded-output pattern as the pytest passes. Content-keyed: candidates without web tests never require node; while active, a missing/unusable runtime is the typed PREFLIGHT_NODE_MISSING/PREFLIGHT_NODE_TOO_OLD hard block and a red suite is NODE_TESTS_FAILED — never a silent skip. Both CI jobs run the mirrored `cd web && node --test tests/*.test.js` step ├── review_substrate.py ← Reviewer-slot coordinator used by task acceptance and planning helpers; duplicate model ids remain independent slots. Actor records keep transport status, parse status, semantic verdict, model/provider, role, coverage, quorum contribution, reason, enforcement impact, and review-binding hashes distinct; only a compact projection reaches task/event/UI records. Task acceptance enforces adaptive quorum, one substantive call and no more than two physical attempts per actor, metric-grounded criterion evidence, provenance, and a public-info-only anti-cheat boundary. Commit/triad/scope P3 orchestration remains a separate one-pass contract. (v6.87.21) Slot execution has ONE seam: `_run_slot` builds an immutable `ReviewAssignment` and binds it ONCE through `review_execution._review_route_executor` — the single place a transport is chosen (closed `ReviewRouteKind`: `api_chat` and `agent_session` — never a vendor/harness name), bound before the first send so the durable prompt record is written from the route's own lazily rendered projection; `_execute_slot_attempt` is the single physical-attempt seam that runs the already-bound executor, and the route's executor returns a typed `ReviewAttemptResult`. Attempt rails, persistence, parsing, actor projection and quorum stay above the seam and are route-agnostic; a route that cannot deliver raises the typed `ReviewRouteUnavailable` on its own slot instead of falling back to another transport. Prompt assembly lives BELOW the seam: `ApiChatReviewExecutor` renders the historical messages lazily and memoizes them, so the durable prompt record and both permitted physical sends share one byte-identical rendering (pinned by a golden digest test) and a non-API route never assembles an API pack. Everything below the seam — route vocabulary, assignment, attempt result, executors, and the api_chat prompt renderers — lives in `review_execution.py`, which never imports the coordinator back; `review_substrate` re-exports the historical renderer names for existing callers. (phase 5) `AgentSessionReviewExecutor` delivers a slot as ONE delegated read-only Claudexor session through the shared `run_delegated_review_session` nanny loop (custody, settlement, verified-cancel time cap, D7 full-artifact read; the delegated advisory rides the same loop). Its typed verdict follows D19: `outputSchema` is asked only when the route's own live manifest (`GET /v2/harnesses`) declares structured output — the agent-capability catalog's harness rows carry no such field at all, so reading it there answered False for every route — trusted only on the run's own `outputConformance == "passed"` (never run success); otherwise the strict parser first, then LIGHT-MODEL extraction canonicalizes narrative to the review's own contract — bare `[]` or a findings array — so a session's clean verdict survives `empty_array_is_verified_clean` unchanged, with every extraction-instead-of-schema landing disclosed as `capability_delta` (actor usage + durable event). Per-row delivery comes from `OUROBOROS_REVIEW_ROUTES` / `OUROBOROS_SCOPE_REVIEW_ROUTES` with the session target in `OUROBOROS_REVIEW_SESSION_ROUTE` (falling back to `OUROBOROS_SUBAGENT_HARNESS`, so an owner who configured ONE delegated route does not have to configure it twice; review/advisory sessions riding that subagent default also inherit the owner's Delegation account pin — `OUROBOROS_SUBAGENT_PROFILE` — with the same strict per-subject health on dispatch); task acceptance is pinned `api_chat` (D15); plan review follows each configured row's delivery kind (`api_chat` packet in-process, `agent_session` retrieving reviewer). The advisory row lives on the SAME shared row vocabulary (structured `OUROBOROS_REVIEWER_SLOTS.advisory`, or legacy `OUROBOROS_ADVISORY_REVIEW_ROUTE`): kind `api_chat` runs the bounded native inspection episode on the row's routed model (the retired legacy kind `api` — Claude-SDK spellings — still parses and migrates to the same model's routed id, force-disabling the row with a typed `disabled_reason` when unmappable, never silently swapping models), `agent_session` rides the delegated session executor, and a `subagent_id` reference resolves through the Available-subagents roster. Advisory availability is credentials-based on the resolved model (`advisory_model_credentials_missing` bypass), never a vendor-key special case. Scope session delivery is assembled by `tools/scope_review_session.py` from the SAME `build_scope_review_prompt` builder (retrieval pointers instead of packs, canonical docs as `generate_doc_nav_map` navigation maps); its coverage manifest is forensics, never a gate: `host_file_read_attestation: unobserved` is a non-blocking disclosed fact (the host does not see which files the session opened — a provenance limit, not a coverage finding), and the api-only ≥1M window floor does not apply to the agentic-delivery session mode, which BIBLE P3 admits as an ALTERNATE AUTHORITATIVE delivery mode once its window is sourced at ≥200K (D16). @@ -3058,7 +3058,7 @@ package. | In-flight chat activity ABI (additive-optional) — `StateResponse.active_direct_turns` (list of `ActiveDirectTurn`: `activity_id`, `chat_id`, `project_id`, `client_message_id`, `kind`, `phase`, `started_at`) snapshots the process-local `DirectActivityRegistry`; `StateResponse.active_chat_activities` (list of `ActiveChatActivity`, same field shape) unites those rows with ROOT managed queue tasks (`kind="managed_task"`, `phase` `queued`/`working`/`finalizing`, chat/project re-homed through the task binding); `TypingOutbound` gains optional `activity_id`/`client_message_id`/`phase`/`kind`, and `ChatOutbound` gains optional `task_phase` ("finalizing" on a root's early final). `kind` is stamped on registry-tracked turns and on RUNNING queue roots; a kind-less typing frame (subagents, legacy) stays exempt from the snapshot's deletion authority. Consumers: the chat status reducer (`chat_activity.computeDerivedChatStatus`) and snapshot hydration (`computeHydratedDirectActivities`). | `ouroboros/gateway/contracts.py`, `web/modules/api_types.js`, `supervisor/active_activity.py`, `ouroboros/gateway/state.py`, `web/modules/chat_activity.js` | `tests/test_gateway_parity.py` pins both mirrors (name- and field-level); `tests/test_direct_activity_registry.py` pins the registry; `tests/test_project_chat_continuity.py` pins the queue projection and finalizing seams; `web/tests/chat_inflight_indicator.test.js` + `web/tests/chat_continuity.test.js` pin the reducer and hydration authority. | | `project_thread` (additive-optional, this sprint) — boolean stamp on `ChatOutbound`/`TypingOutbound`/`PhotoOutbound`/`VideoOutbound`/`DocumentOutbound`/`LogOutbound` set at the message-bus broadcast choke when the frame's final `chat_id` is a reserved Project thread (registry membership via an mtime-cached lens, `projects_registry.project_thread_chat_ids`; the chat seam re-stamps as the LAST writer after progress-meta merge, so meta can neither spoof nor erase it). Consumer: Main's fan-out gate `chat_activity.mainThreadAccepts` — a stamped frame is never adopted by Main, even before `projectChatIds` learns the project. Absent = unchanged legacy routing (external transport ids are never stamped). Distinct from `ChatOutbound.project_id`, which rides the two Main-destined project lifecycle rows (`project_started` and `project_completion_summary`), persisted with their metadata and consumed by history replay and the Main renderer. | `supervisor/message_bus.py`, `ouroboros/projects_registry.py`, `ouroboros/gateway/contracts.py`, `web/modules/api_types.js`, `web/modules/chat_activity.js`, `web/modules/chat.js` | `tests/test_gateway_parity.py` pins all six fields in both mirrors; `tests/test_message_bus.py` pins the stamp per seam (project yes; main/legacy/transport-shaped no; meta spoof/erase; lens follows the registry file); `web/tests/chat_thread_routing.test.js` pins the gate. | | Managed update gateway ABI — the empty preflight request, exact channel-bound `UpdateMergePlan`, pinned apply request (`strategy`, base/target SHAs, recovery confirmation), typed success/error variants, and `update_status_ready` WS notice that refreshes the boot-time cache in the UI. | `ouroboros/gateway/contracts.py`, `web/modules/api_types.js` | `tests/test_gateway_parity.py` pins every field and message type in both mirrors; `tests/test_update_apply_routing.py` drives pin, strategy, recovery-confirmation, and response routing. | -| `ChatOutbound.review_projection` (v6.65.0) — optional compact panel/actor truth for Chat and Logs: transport status, parse status, semantic verdict, task-acceptance `outcome_tier`, model/provider/role, coverage, quorum/enforcement impact, the complete redacted reason, a forensic `response_ref` (flat content hashes, no host paths — v6.70.0), and exact candidate/evidence/fence binding hashes; v6.74.0 adds additive optional keys — per-actor `dialogue_status`, per-panel `dialogue` ({status, votes}) and the `single_reviewer_no_diversity` label; raw reviewer output remains in private audit storage. | `ouroboros/gateway/contracts.py`, `ouroboros/review_substrate.py` | `tests/test_contracts.py` pins the field as optional frozen ABI; `tests/test_gateway_parity.py` pins the field in both Python and JavaScript contracts; `tests/test_review_substrate_v2.py` pins the bounded actor projection including `outcome_tier`; `web/tests/review_truth.test.js` pins the shared renderer. | +| `ChatOutbound.review_projection` (v6.65.0) — optional compact panel/actor truth for Chat and Logs: transport status, parse status, semantic verdict, task-acceptance `outcome_tier`, model/provider/role, coverage, quorum/enforcement impact, the complete redacted reason, a forensic `response_ref` (flat content hashes, no host paths — v6.70.0), and exact candidate/evidence/fence binding hashes; v6.74.0 adds additive optional keys — per-actor `dialogue_status`, per-panel `dialogue` ({status, votes}) and the `single_reviewer_no_diversity` label; a later additive pair carries the reviewer's structured findings on the owner surface — `actors[].findings` (bounded disclosed rows `{id?, severity?, verdict?, item?, summary?, evidence?, reason?, recommendation?}`, redacted and string-bounded through `utils.truncate_review_artifact`, at most `review_substrate.MAX_PROJECTED_ACTOR_FINDINGS` rows) beside the exact `actors[].findings_omitted` count, emitted only when that reviewer produced a parsed response (absence is a transport/parse hole, never "zero findings"; the durable remainder stays addressable through `actors[].response_ref`); raw reviewer output remains in private audit storage. | `ouroboros/gateway/contracts.py`, `ouroboros/review_substrate.py` | `tests/test_contracts.py` pins the field as optional frozen ABI; `tests/test_gateway_parity.py` pins the field in both Python and JavaScript contracts; `tests/test_review_substrate_v2.py` pins the bounded actor projection including `outcome_tier`; `web/tests/review_truth.test.js` pins the shared renderer. | | `chat_id_policy` — SSOT for A2A/synthetic chat-id filtering across message bus, history, memory, and consolidation | `ouroboros/contracts/chat_id_policy.py` | `tests/test_chat_id_policy.py` pins boundaries and human/transport positive ids | | `task_contract` — canonical, durable normalization for objective/output, constraints, resources, disabled tools, workspace/lineage, delegation budget, deadline, answer protocol, budget profile, acceptance claims, and the optional host-verified Presence `capability_ceiling`. `effective_acceptance_claims(task, closed_plan_wave)` is the pure read-time binder: ingress claims win, otherwise the current closed plan wave's frozen claims apply; it neither mutates nor rebuilds the running contract. Child builders must restate every intentionally narrowed field after the parent spread. Presence promotion and both one-shot and recurring follow-ups copy the same ceiling and return context by value, so later work cannot widen the admitted turn. Pacing interprets the normalized budget profile separately through typed `task_pacing.CostCeiling`. | `ouroboros/contracts/task_contract.py` | Contract, delegation-budget, disabled-tool, task/outcome, acceptance-evidence, Presence-authority, promotion, and follow-up tests pin normalization, propagation, and claim provenance. | | `PluginAPI` (Phase 4, v1.4) + `ExtensionRegistrationError` + `FORBIDDEN_EXTENSION_SETTINGS` + `VALID_EXTENSION_PERMISSIONS` + `VALID_EXTENSION_ROUTE_METHODS` — the surface every `type: extension` skill's `plugin.py::register(api)` binds against (`register_tool`, `register_route`, `register_ws_handler`, `register_ui_tab`, `register_settings_section`, `register_supervised_task`, `register_companion_process`, `subscribe_event`, `get_skill_token`, `send_ws_message`, `on_unload`, `log`, `get_settings`, `get_state_dir`, `skill_job_dir`, `get_runtime_info`). `skill_job_dir(job_id)` creates isolated `jobs/-/{assets,output,tmp}` state folders so generation skills do not overwrite their own assets across jobs. `VALID_EXTENSION_PERMISSIONS` includes host-mediated permissions (`companion_process`, `supervised_task`, `subscribe_event`, `inject_chat`, `presence`) that require review/owner grants as documented in CHECKLISTS.md. The `ExecutionMode` capability matrix (`MATRIX_CAPABILITIES` / `OUT_OF_PROCESS_UNAVAILABLE_CAPABILITIES` / `capability_available` / `available_capabilities`) is the SSOT for which side-effect surfaces an out-of-process child may use and is pinned by the contract test. | `ouroboros/contracts/plugin_api.py` | `tests/test_contracts.py::test_plugin_api_surface_is_frozen` pins the frozen method set; `tests/test_contracts.py::test_extension_route_methods_contract_matches_server_dispatch` pins the route-methods tuple; `tests/test_extension_loader.py::test_plugin_api_impl_matches_protocol` asserts the concrete `PluginAPIImpl` structurally satisfies the runtime-checkable Protocol | diff --git a/ouroboros/review_execution_projection.py b/ouroboros/review_execution_projection.py index c8cf7c5e1..206aea6b9 100644 --- a/ouroboros/review_execution_projection.py +++ b/ouroboros/review_execution_projection.py @@ -1,14 +1,61 @@ -"""Tiny public execution receipts projected from review actor usage. +"""Tiny public read-side projections from review actor records. -This leaf owns the cross-surface presentation wire. It deliberately imports no -review engine: callers pass returned actor usage, and only actual receipt facts -can become an API or harness execution badge. +This leaf owns the cross-surface presentation wire: execution receipts and the +bounded finding rows. It deliberately imports no review engine: callers pass +returned actor facts, and only actual receipt/finding content is projected. """ from __future__ import annotations +import json from typing import Any, Dict, List +from ouroboros.observability import redact_projection +from ouroboros.utils import truncate_review_artifact + +# Bounded structured findings on the public actor projection: the bound is the +# ROW COUNT, never a second aggressive cut of the finding bodies — capping the +# only owner-reachable copy of reviewer text is the class v6.70.0 removed. The +# per-string bound mirrors plan review's MAX_FINDING_TEXT_CHARS (2000), not the +# 200-char path-list default; the durable remainder stays addressable through +# the actor's existing response_ref, so no full-set hash is needed. +MAX_PROJECTED_ACTOR_FINDINGS = 8 +PROJECTED_FINDING_TEXT_CHARS = 2000 +_PROJECTED_FINDING_KEYS = ( + "id", "severity", "verdict", "item", "summary", "evidence", "reason", + "recommendation", +) + + +def projected_finding_row(item: Any) -> Dict[str, str]: + """One bounded, redacted finding row for the public actor projection.""" + row: Dict[str, str] = {} + if not isinstance(item, dict): + return row + for key in _PROJECTED_FINDING_KEYS: + value = item.get(key) + if value is None or value == "": + continue + if isinstance(value, str): + rendered = str(redact_projection(value).value) + else: + # A non-string value keeps structural key-based masking: str() + # first would flatten a nested secret past the key-name redactor. + rendered = json.dumps( + redact_projection(value).value, ensure_ascii=False, default=str, + ) + row[key] = truncate_review_artifact(rendered, PROJECTED_FINDING_TEXT_CHARS) + if not row: + # An unknown finding shape still carries evidence; a silently empty row + # would destroy it without a trace. Redact the OBJECT before + # serializing: structural key-based secret masking does not survive a + # pre-serialized string. + row["item"] = truncate_review_artifact( + json.dumps(redact_projection(item).value, ensure_ascii=False, default=str), + PROJECTED_FINDING_TEXT_CHARS, + ) + return row + _API_EXECUTION_RECEIPT_KEYS = frozenset({ "resolved_model", "provider", "prompt_tokens", "completion_tokens", @@ -78,4 +125,10 @@ def review_executions_from_actor_usage(actors: Any) -> List[Dict[str, str]]: return normalize_review_executions(executions) -__all__ = ["normalize_review_executions", "review_executions_from_actor_usage"] +__all__ = [ + "MAX_PROJECTED_ACTOR_FINDINGS", + "PROJECTED_FINDING_TEXT_CHARS", + "normalize_review_executions", + "projected_finding_row", + "review_executions_from_actor_usage", +] diff --git a/ouroboros/review_substrate.py b/ouroboros/review_substrate.py index 75ce643ef..b0f69d86e 100644 --- a/ouroboros/review_substrate.py +++ b/ouroboros/review_substrate.py @@ -22,7 +22,10 @@ log = logging.getLogger("review_substrate") from ouroboros.llm import LLMClient from ouroboros.observability import new_call_id, persist_call, redact_projection from ouroboros.provider_models import provider_for_model -from ouroboros.review_execution_projection import review_executions_from_actor_usage +from ouroboros.review_execution_projection import ( + MAX_PROJECTED_ACTOR_FINDINGS, projected_finding_row, + review_executions_from_actor_usage, +) from ouroboros.task_results import review_binding_hash # Everything below the seam. Re-exported here because the substrate is the # historical import site for the api_chat prompt renderers; `review_execution` @@ -60,6 +63,7 @@ from ouroboros.usage_accounting import ( usage_scope, ) from ouroboros.utils import sanitize_tool_result_for_log, truncate_review_artifact +from ouroboros._outcome_receipts import disclosed_list_projection class _CustodyUsageContext: @@ -346,7 +350,7 @@ def _review_actor_projection(actor: Any, surface: str) -> Dict[str, Any]: ) if dialogue_vote not in DIALOGUE_STATUS_VALUES: dialogue_vote = "" - return { + projection = { "slot_id": str(row.get("slot_id") or ""), "model": model, "provider": provider, "actor_role": str(row.get("actor_role") or f"{surface} reviewer"), "transport_status": transport, @@ -369,6 +373,16 @@ def _review_actor_projection(actor: Any, surface: str) -> Dict[str, Any]: # Flat, redacted pointer to the private full response artifact. "response_ref": _response_ref_projection(row.get("response_ref")), } + # Structured rows ride only where a parsed response exists: an absent + # `findings` key is a hole, never the claim "zero findings reported". + if parsed is not None: + projection.update(disclosed_list_projection( + parsed_findings, + key="findings", + limit=MAX_PROJECTED_ACTOR_FINDINGS, + item=projected_finding_row, + )) + return projection def _response_ref_projection(ref: Any) -> Dict[str, str]: diff --git a/ouroboros/size_ratchet_manifest.py b/ouroboros/size_ratchet_manifest.py index ec2009a8b..3abaac6b2 100644 --- a/ouroboros/size_ratchet_manifest.py +++ b/ouroboros/size_ratchet_manifest.py @@ -233,5 +233,5 @@ BYTE_DEBT = { "ouroboros/loop.py": 312772, "tests/test_delegated_subagent_transport.py": 320340, "tests/test_devtools_benchmarks.py": 328116, - "web/modules/chat.js": 224315, + "web/modules/chat.js": 224244, } diff --git a/ouroboros/skill_review_runner.py b/ouroboros/skill_review_runner.py index 682a87f32..63b54ed40 100644 --- a/ouroboros/skill_review_runner.py +++ b/ouroboros/skill_review_runner.py @@ -97,7 +97,8 @@ def _file_stamp(path: pathlib.Path) -> tuple: def skill_review_ui_projection( drive_root: pathlib.Path, skill_name: str, ) -> Dict[str, Any]: - """Sanitized current run and the last ten rows in its review group.""" + """Sanitized current run, the last ten rows in its review group, and the + exact count of older group rows that ten-row window leaves out.""" cache_key = (str(drive_root), str(skill_name)) stamp = ( _file_stamp(review_job_state_path(drive_root, skill_name)), @@ -111,7 +112,8 @@ def skill_review_ui_projection( group_id = str(current.get("group_id") or "") if not group_id and all_history: group_id = str(all_history[-1].get("group_id") or "") - history = [row for row in all_history if not group_id or row.get("group_id") == group_id][-10:] + group_rows = [row for row in all_history if not group_id or row.get("group_id") == group_id] + history = group_rows[-10:] projection: Dict[str, Any] if not current and not history: projection = {} @@ -119,6 +121,9 @@ def skill_review_ui_projection( projection = { "current": _review_ui_row(current) if current else {}, "history": [_review_ui_row(row) for row in history], + # Group-scoped disclosed bound (BIBLE P1): the exact number of + # older rows the ten-row window left out, 0 included. + "history_omitted": max(0, len(group_rows) - len(history)), } _UI_PROJECTION_CACHE[cache_key] = (stamp, projection) return projection diff --git a/tests/test_review_substrate_v2.py b/tests/test_review_substrate_v2.py index c709b1dfa..45332a659 100644 --- a/tests/test_review_substrate_v2.py +++ b/tests/test_review_substrate_v2.py @@ -1054,6 +1054,150 @@ def test_compact_review_projection_redacts_public_reasons_before_truncation(): assert "***REDACTED***" in panel["reason"] +def test_actor_projection_carries_bounded_disclosed_finding_rows(): + from ouroboros.review_substrate import ( + MAX_PROJECTED_ACTOR_FINDINGS, compact_review_projection, + ) + + secret = "sk-or-" + ("FindingSecret456" * 4) + long_recommendation = "Re-run the verifier with the fixed seed. " * 120 + findings = [ + { + "severity": "critical", + "item": f"finding {index}", + "evidence": f"evidence {index}", + "recommendation": f"fix {index}", + } + for index in range(MAX_PROJECTED_ACTOR_FINDINGS + 2) + ] + findings[0]["evidence"] = "credential=" + secret + findings[1]["recommendation"] = long_recommendation + run = { + "request": {"surface": "task_acceptance", "policy": {"min_successful_slots": 1}}, + "aggregate_signal": "FAIL", + "actors": [ + { + "slot_id": "with-findings", + "model": "model-a", + "status": "ok", + "signal": "FAIL", + "parsed": {"verdict": "FAIL", "summary": "s", "findings": findings}, + "quorum_contribution": True, + }, + { + "slot_id": "clean", + "model": "model-b", + "status": "ok", + "signal": "PASS", + "parsed": {"verdict": "PASS", "summary": "ok", "findings": []}, + "quorum_contribution": True, + }, + { + "slot_id": "transport-hole", + "model": "model-c", + "status": "error", + "error": "timed out", + "parsed": None, + }, + { + "slot_id": "odd-shape", + "model": "model-d", + "status": "ok", + "signal": "FAIL", + "parsed": { + "verdict": "FAIL", + "summary": "s", + "findings": [{ + "weird_key": "the only copy of this evidence", + "password": "hunter2-odd-shape", + }], + }, + }, + { + # A non-string value under a KNOWN key keeps structural + # key-based masking: str() first would flatten the nested + # secret past the key-name redactor. + "slot_id": "nested-evidence", + "model": "model-f", + "status": "ok", + "signal": "FAIL", + "parsed": { + "verdict": "FAIL", + "summary": "s", + "findings": [{ + "severity": "high", + "item": "nested shape", + "evidence": {"password": "hunter2-nested-shape"}, + }], + }, + }, + { + # The array-ladder reviewer contract shapes findings as + # {item, verdict, severity, reason}: the substantive `reason` + # text must survive projection. + "slot_id": "triad-shape", + "model": "model-e", + "status": "ok", + "signal": "FAIL", + "parsed": [{ + "item": "missing rollback test", + "verdict": "FAIL", + "severity": "high", + "reason": "the new path has no failure-injection coverage", + }], + }, + ], + } + + panel = compact_review_projection([run])["panels"][0] + actors = {actor["slot_id"]: actor for actor in panel["actors"]} + rendered = json.dumps(panel, ensure_ascii=False) + + rows = actors["with-findings"]["findings"] + assert len(rows) == MAX_PROJECTED_ACTOR_FINDINGS + assert actors["with-findings"]["findings_omitted"] == 2 + assert rows[2] == { + "severity": "critical", "item": "finding 2", + "evidence": "evidence 2", "recommendation": "fix 2", + } + # The count stays beside the rows: coverage keeps the full total. + assert actors["with-findings"]["coverage"]["findings"] == len(findings) + # Redaction covers finding bodies exactly like reasons. + assert secret not in rendered + assert "***REDACTED***" in rows[0]["evidence"] + # A clipped string discloses its own cut instead of clipping silently. + assert "OMISSION NOTE" in rows[1]["recommendation"] + assert len(rows[1]["recommendation"]) < len(long_recommendation) + + # A reviewer that reported no findings states that as an empty disclosed + # list; a reviewer with no parsed response leaves a hole, not a zero. + assert actors["clean"]["findings"] == [] + assert actors["clean"]["findings_omitted"] == 0 + assert "findings" not in actors["transport-hole"] + assert "findings_omitted" not in actors["transport-hole"] + + # An unknown finding shape still ships its evidence as a bounded row, and + # structural key-based secret masking applies BEFORE serialization. + odd_rows = actors["odd-shape"]["findings"] + assert odd_rows and "the only copy of this evidence" in odd_rows[0]["item"] + assert "hunter2-odd-shape" not in rendered + assert "***REDACTED***" in odd_rows[0]["item"] + + nested_rows = actors["nested-evidence"]["findings"] + assert "hunter2-nested-shape" not in rendered + assert "***REDACTED***" in nested_rows[0]["evidence"] + assert nested_rows[0]["item"] == "nested shape" + + # A list-shaped parsed response (array reviewer contract) projects its + # rows too, and the substantive `reason`/`verdict` fields survive. + triad_rows = actors["triad-shape"]["findings"] + assert triad_rows == [{ + "severity": "high", "verdict": "FAIL", "item": "missing rollback test", + "reason": "the new path has no failure-injection coverage", + }] + assert actors["triad-shape"]["findings_omitted"] == 0 + + class _MixedPassPassFailLLM: def chat(self, **kwargs): if str(kwargs.get("model") or "").endswith("-2"): diff --git a/tests/test_skill_review_runner.py b/tests/test_skill_review_runner.py index 6f5659c9d..e4f8d0088 100644 --- a/tests/test_skill_review_runner.py +++ b/tests/test_skill_review_runner.py @@ -929,6 +929,9 @@ def test_skill_review_ui_projection_is_group_scoped_bounded_and_sanitized(tmp_pa assert projection["history"][0]["review_round"] == 3 assert all(row["group_id"] == "manual:alpha" for row in projection["history"]) assert all("raw_actor_records" not in row for row in projection["history"]) + # The ten-row window is a disclosed bound: 12 group rows minus 10 shown. + # The foreign-group row must not count into the omitted number. + assert projection["history_omitted"] == 2 def test_cancel_and_timeout_each_write_one_idempotent_terminal_row(tmp_path): diff --git a/tests/test_ui_smoke_review_checkpoint.py b/tests/test_ui_smoke_review_checkpoint.py index 523a8d643..f8b04d5a0 100644 --- a/tests/test_ui_smoke_review_checkpoint.py +++ b/tests/test_ui_smoke_review_checkpoint.py @@ -45,6 +45,18 @@ def test_ui_smoke_review_truth_is_visible_in_chat_and_logs(direct_server_with_da "quorum_contribution": True, "enforcement_impact": "supports_pass", "reason": "The browser evidence is incomplete.", + "coverage": {"criteria_total": 3, "findings": 3}, + "findings": [ + { + "severity": "high", + "item": "Browser evidence missing for the checkout flow", + "evidence": "no screenshot covers step 3", + "recommendation": "Capture the payment page state", + }, + {"severity": "low", "item": "Trace summary is terse"}, + ], + "findings_omitted": 1, + "response_ref": {"call_id": "call-fable-1", "sha256": "a" * 64}, }, { "slot_id": "sol", @@ -110,12 +122,46 @@ def test_ui_smoke_review_truth_is_visible_in_chat_and_logs(direct_server_with_da }) + "\n", encoding="utf-8") task_results = data_dir / "task_results" task_results.mkdir(parents=True, exist_ok=True) + plan_state = { + "schema_version": 2, + "current_attempt": {"fingerprint": "f" * 64, "status": "open"}, + "waves": [{ + "request_fingerprint": "f" * 64, + "cycle_index": 1, + "aggregate": "REVISE_PLAN", + "closed": False, + "paid": True, + "counts": {"blocking": 1, "note": 0, "need_evidence": 0}, + "findings": [{ + "finding_id": "slot_1:f1", + "id": "f1", + "class": "blocking", + "summary": "The migration step drops the audit ledger", + "breaks": "invariant_1", + "locator": "ouroboros/usage_accounting.py", + "recommendation": "Keep the ledger append-only through the migration", + "slot": "slot_1", + "model": "anthropic/claude-fable-5", + }], + "dispositions": [{ + "finding_id": "slot_1:f1", + "decision": "accept", + "rationale": "will rework the migration", + }], + "actors": [ + {"slot_id": "slot_1", "model": "anthropic/claude-fable-5", "ok": True}, + ], + "reviewed_at": "2026-07-15T09:59:58+00:00", + }], + "waves_omitted": 0, + } (task_results / "review-no-summary.json").write_text(json.dumps({ "task_id": "review-no-summary", "status": "completed", "reason_code": "acceptance_degraded", "outcome_axes": axes, "review_projection": projection, + "plan_review_state": plan_state, }) + "\n", encoding="utf-8") try: @@ -135,11 +181,30 @@ def test_ui_smoke_review_truth_is_visible_in_chat_and_logs(direct_server_with_da assert "Review panel panel_visual_truth" in chat_text assert "Reviewer fable" in chat_text assert "Reviewer sol" in chat_text + # The reviewer's actual findings are readable, not just counted. + assert "Browser evidence missing for the checkout flow" in chat_text + assert "fix: Capture the payment page state" in chat_text + assert "Reviewer fable findings omitted: 1" in chat_text + assert "observability call call-fable-1" in chat_text no_summary = page.locator('.chat-live-card[data-task-id="review-no-summary"]') no_summary.wait_for(state="attached", timeout=30_000) assert no_summary.get_attribute("data-expanded") == "0" + # This card carries BOTH groups; the helper opens the first + # (Plan review), whose finding text must be readable. _open_review_checkpoint(no_summary) assert no_summary.locator('[data-live-phase]').first.get_attribute("data-phase") == "warn" + plan_text = no_summary.inner_text() + assert "The migration step drops the audit ledger" in plan_text + assert "breaks invariant_1" in plan_text + assert "agent: accept — will rework the migration" in plan_text + acceptance_toggle = no_summary.locator( + '[data-review-group-toggle="task_acceptance:review-no-summary"]', + ) + acceptance_toggle.click() + no_summary.locator( + '[data-review-group="task_acceptance:review-no-summary"]' + ' [data-review-attempt-toggle]', + ).first.click() assert "Review panel panel_visual_truth" in no_summary.inner_text() page.wait_for_timeout(900) # cover the routine background history sync assert no_summary.locator('.chat-live-line-repeat:not([hidden])').count() == 0 @@ -157,6 +222,9 @@ def test_ui_smoke_review_truth_is_visible_in_chat_and_logs(direct_server_with_da assert "Review panel panel_visual_truth" in log_text assert "Reviewer fable" in log_text assert "Reviewer sol" in log_text + # The same formatter serves Logs: finding bodies ride along. + assert "Browser evidence missing for the checkout flow" in log_text + assert "Reviewer fable findings omitted: 1" in log_text assert log_card.locator('[data-task-phase]').inner_text() == "warn" review.scroll_into_view_if_needed() review.screenshot(path=str(data_dir.parent / "review-truth-logs.png")) diff --git a/web/modules/api_client.js b/web/modules/api_client.js index 05d8d999c..e100397de 100644 --- a/web/modules/api_client.js +++ b/web/modules/api_client.js @@ -106,6 +106,23 @@ export async function fetchTaskDetail(taskId) { return (resp && typeof resp.json === 'function' && resp.ok !== false) ? resp.json() : null; } +/** + * Strict task-detail read for consumers that must tell a genuinely absent + * record (404 → null) apart from a failed read (rejects). The lenient + * fetchTaskDetail above keeps its every-failure→null contract for reconcile + * flows that treat all misses alike. + * @param {string} taskId + * @returns {Promise} + */ +export async function fetchTaskDetailStrict(taskId) { + const resp = await apiFetch(`/api/tasks/${encodeURIComponent(taskId)}`); + if (resp && typeof resp.json === 'function') { + if (resp.ok !== false) return resp.json(); + if (resp.status === 404) return null; + } + throw new Error(`task detail read failed (HTTP ${resp?.status ?? 'no response'})`); +} + export function cleanExtensionRoute(value) { const route = String(value || '').trim().replace(/^\/+/, ''); const parts = route.split('/').filter(Boolean); diff --git a/web/modules/api_types.js b/web/modules/api_types.js index fb0d1c4ea..1bdab4c54 100644 --- a/web/modules/api_types.js +++ b/web/modules/api_types.js @@ -317,6 +317,13 @@ * v6.74.0 additive keys: panels[].dialogue ({status, votes} — the reviewer-authored * dialogue-status reduction), panels[].single_reviewer_no_diversity (boolean label), * and actors[].dialogue_status ("continue_actionable"|"unreachable_here"|"stable_disagreement"|""). + * Additive bounded-findings keys: actors[].findings (disclosed rows + * {id?, severity?, verdict?, item?, summary?, evidence?, reason?, + * recommendation?} — redacted, each string + * bounded with an explicit omission marker, at most 8 rows per actor) and + * actors[].findings_omitted (exact count, 0 included). Both are emitted only + * when that reviewer produced a parsed response; their absence is a + * transport/parse hole, never "zero findings". * @property {boolean=} worker_saturation_warning * @property {string=} source * @property {string=} sender_label @@ -601,7 +608,7 @@ * @property {boolean=} official_hub_verified * @property {boolean=} owner_attestable * @property {{visible: boolean, publication_ready: boolean, task_start_allowed: boolean, disabled: boolean, state: "ready"|"warnings"|"needs_attention"|"repairable"|"hard_block", reason: string}=} submit_hub - * @property {{current: Object, history: Object[]}=} skill_review + * @property {{current: Object, history: Object[], history_omitted: number=}=} skill_review * @property {boolean=} is_self_authored * @property {Object=} grants * @property {string[]=} permissions diff --git a/web/modules/chat.js b/web/modules/chat.js index 3c5fdf2fc..0846778d1 100644 --- a/web/modules/chat.js +++ b/web/modules/chat.js @@ -8,7 +8,7 @@ import { PAGE_ICONS } from './page_icons.js'; import { showToast } from './toast.js'; import { createSystemMessageAction, downloadViaHostBridge, openViaHostBridge } from './ui_helpers.js'; import { clientSurfaceField } from './client_surface.js'; -import { apiClient, apiFetch, fetchTaskDetail } from './api_client.js'; +import { apiClient, apiFetch, fetchTaskDetail, fetchTaskDetailStrict } from './api_client.js'; import { OWNER_STOP_DETAIL_MARKER, getLogTaskGroupId, @@ -147,7 +147,7 @@ const MAX_PENDING_ATTACHMENT_BYTES = 100 * 1024 * 1024; const shownIncidentToastKeys = new Set(); function showTaskIncidentToast(msg) { - const incident = String(msg?.task_incident || '').trim(); + const incident = taskKey(msg?.task_incident); if (!incident) return; const key = String(msg?.toast_once || `${msg?.task_id || ''}:${incident}`).trim(); if (!key || shownIncidentToastKeys.has(key)) return; @@ -164,6 +164,8 @@ export function initChat(ctx) { return createChatInstance(ctx); } +const taskKey = (value) => String(value || '').trim(); + export function createChatInstance({ ws, state, updateUnreadBadge, openSettingsTab, openDashboardTab, stateSnapshots, @@ -507,12 +509,12 @@ export function createChatInstance({ const reviewDisclosureByTask = new Map(); const skillReviewDetailStore = new Map(); const reviewHydrator = createReviewHydrator({ - fetchDetail: (taskId) => fetchTaskDetail(taskId), - applyDetail: (taskId, detail) => !destroyed && attachTaskDetailReviews(taskId, detail), + fetchDetail: fetchTaskDetailStrict, + applyDetail: (id, d) => !destroyed && attachTaskDetailReviews(id, d), + onState: (id, status) => !destroyed + && liveCardRecords.get(id)?.reviewController?.setHydrateStatus?.(status), }); - // Cluster B: a proactively-coined name (task_named) can arrive BEFORE the card's - // liveCardRecords entry exists (the namer broadcasts as the task starts). Buffer it - // here so createLiveCardRecord can apply it when the card appears (no lost title). + // A task_named frame can arrive before the card's record exists; buffer it. const pendingSuggestedNames = new Map(); const taskUiStates = new Map(); // Busy-chat decision turns reuse the normal agent/event path for ordering and @@ -545,7 +547,7 @@ export function createChatInstance({ } function recordConcludedActivity(activityId) { - const aid = String(activityId || '').trim(); + const aid = taskKey(activityId); if (!aid) return; missingManagedTaskIds.delete(aid); concludedDirectActivities.delete(aid); @@ -556,7 +558,7 @@ export function createChatInstance({ } } function recordTerminalActivity(taskId) { - const id = String(taskId || '').trim(); + const id = taskKey(taskId); if (!id) return; activeDirectActivities.delete(id); missingManagedTaskIds.delete(id); @@ -577,7 +579,7 @@ export function createChatInstance({ } function registerEphemeralDecisionFrameMutation(frame) { - const taskId = String(frame?.task_id || '').trim(); + const taskId = taskKey(frame?.task_id); if (!taskId) return false; if (frame?.ephemeral_decision) { ephemeralDecisionTaskIds.add(taskId); @@ -1136,7 +1138,7 @@ export function createChatInstance({ async function turnTaskIntoProject(record) { if (!record || record.root?.dataset?.projectCreating === '1' || record.root?.dataset?.projectCreated === '1') return; - const taskId = String(record.groupId || '').trim(); + const taskId = taskKey(record.groupId); const projectId = projectIdFromTask(taskId); record.root.dataset.projectCreating = '1'; const actions = record.turnProjectBtn?.parentElement || record.root.querySelector('.chat-live-actions'); @@ -1250,7 +1252,7 @@ export function createChatInstance({ // Pending intent stays live until settled; soft stop shows Finalizing…. function markLiveCardCancelPending(taskId = '', soft = false) { - const record = liveCardRecords.get(String(taskId || '').trim()); + const record = liveCardRecords.get(taskKey(taskId)); if (!record || record.finished || !record.phaseEl) return; record.cancelPendingPolicy = soft ? 'finalize' : 'immediate'; record.finalizingHold = false; // owner cancel outranks the hold @@ -1262,7 +1264,7 @@ export function createChatInstance({ // Early final stays live while post-task synthesis runs. function markLiveCardFinalizing(taskId = '') { - const record = liveCardRecords.get(String(taskId || '').trim()); + const record = liveCardRecords.get(taskKey(taskId)); if (!record || record.finished || !record.phaseEl) return; markReviewAnchor(record); if (record.cancelPendingPolicy) return; @@ -1294,7 +1296,7 @@ export function createChatInstance({ } async function cancelRunFromCard(record, action = '') { - const taskId = String(record?.groupId || '').trim(); + const taskId = taskKey(record?.groupId); if (!taskId || record.finished) return; // Q2: the dropdown itself is the confirmation surface — dismissing it // continued the run, so a selected action executes immediately. @@ -1352,7 +1354,7 @@ export function createChatInstance({ } function markTaskCancelable(taskId = '') { - const id = String(taskId || '').trim(); + const id = taskKey(taskId); if (!id || cancelableTaskIds.has(id)) return; cancelableTaskIds.add(id); const record = liveCardRecords.get(id); @@ -1411,12 +1413,14 @@ export function createChatInstance({ _chatFreedTimer = setTimeout(() => row.classList.remove('chat-freed'), 900); } + const reviewAnchorEligible = (id) => !liveCardRecords.has(id) + && !taskUiStates.has(id) && !activeDirectActivities.has(id); + function attachReviewGroup(group, rawTs = '') { - const ownerTaskId = String(group?.presentationOwnerTaskId || '').trim(); + const ownerTaskId = taskKey(group?.presentationOwnerTaskId); if (!ownerTaskId) return false; if (retiredTaskIds.has(ownerTaskId) && !liveCardRecords.has(ownerTaskId)) return true; - const reviewAnchor = !liveCardRecords.has(ownerTaskId) - && !taskUiStates.has(ownerTaskId) && !activeDirectActivities.has(ownerTaskId); + const reviewAnchor = reviewAnchorEligible(ownerTaskId); const ownerState = forceTaskCard(ownerTaskId, rawTs); if (!ownerState?.cardVisible) return false; const record = liveCardRecords.get(ownerTaskId); @@ -1434,7 +1438,7 @@ export function createChatInstance({ } function attachTaskDetailReviews(taskId, detail) { - const id = String(taskId || '').trim(); + const id = taskKey(taskId); const groups = reviewGroupsFromTaskDetail(detail, id); if (!id || groups.length === 0) return false; if (!liveCardRecords.has(id)) forceTaskCard(id, detail?.ts || detail?.timestamp || ''); @@ -1446,9 +1450,7 @@ export function createChatInstance({ } function hydrateCardReviews(taskId, revision = null) { - const id = String(taskId || '').trim(); - if (!id || destroyed) return Promise.resolve(false); - return reviewHydrator.hydrate(id, revision); + return destroyed ? Promise.resolve(false) : reviewHydrator.hydrate(taskId, revision); } function attachReviewFromRow(row, rawTs = '', showPointerAck = false) { @@ -1481,7 +1483,13 @@ export function createChatInstance({ function handleReviewReference(row) { const reference = reviewReferenceFromRow(row); if (!reference) return false; - hydrateCardReviews(reference.presentationOwnerTaskId, reference.stateRevision); + const owner = reference.presentationOwnerTaskId; + // A reference proves a review exists: mint an anchored card so a + // failed hydrate has a home, not fake task activity. + const anchor = reviewAnchorEligible(owner); + forceTaskCard(owner, row?.ts); + if (anchor) markReviewAnchor(liveCardRecords.get(owner), true); + hydrateCardReviews(owner, reference.stateRevision); return true; } @@ -1580,8 +1588,7 @@ export function createChatInstance({ // used to name a project on "turn into project" when the server has no // title/objective yet (P1, direct-chat conversion). One-shot handoff. objectiveHint: (isMain && !options.isSubagent) ? _pendingCardObjective : '', - // Cluster B: the proactively-coined LLM project name; when set it becomes - // the card title (the activity headline keeps rendering in the lines below). + // The proactively-coined LLM name; becomes the card title when set. suggestedName: '', // P1 (v6.82): last bounded activity projection (remembered even while // the collapsed line is suppressed on unnamed root cards) + sticky cost. @@ -1674,8 +1681,8 @@ export function createChatInstance({ } function applySuggestedNameMutation(taskId, name) { - const tid = String(taskId || '').trim(); - const nm = String(name || '').trim(); + const tid = taskKey(taskId); + const nm = taskKey(name); if (!tid || !nm) return; const record = liveCardRecords.get(tid); if (!record) { @@ -2380,17 +2387,17 @@ export function createChatInstance({ subagentChildParents.set(childId, { parentId: parentId || prev.parentId || '', role: role || prev.role || '', - model: String(model || '').trim() || prev.model || '', + model: taskKey(model) || prev.model || '', }); } function learnSubagentLineage(msg) { if (String(msg?.delegation_role || '').toLowerCase() !== 'subagent') return ''; - const parentId = String(msg.parent_task_id || '').trim(); + const parentId = taskKey(msg.parent_task_id); const childId = String(msg.subagent_task_id || msg.task_id || '').trim(); if (!parentId || !childId || parentId === childId) return ''; setSubagentParent(childId, { - parentId, role: String(msg.subagent_role || '').trim(), model: msg.model, + parentId, role: taskKey(msg.subagent_role), model: msg.model, }); const event = String(msg.subagent_event || '').toLowerCase(); const replayTerminal = msg.task_terminal_status @@ -2424,7 +2431,7 @@ export function createChatInstance({ markTaskCancelable(String(msg.task_id)); } // Child lifecycle pings must not update the parent's terminal state. - const lifecycleParent = String(msg?.parent_task_id || '').trim(); + const lifecycleParent = taskKey(msg?.parent_task_id); if ( msg?.subagent_event && lifecycleParent @@ -2489,11 +2496,11 @@ export function createChatInstance({ function updateSubagentCardFromEvent(evt, tsValue) { if (!evt || String(evt.delegation_role || '').toLowerCase() !== 'subagent') return false; - const parentId = String(evt.parent_task_id || '').trim(); + const parentId = taskKey(evt.parent_task_id); const childId = String(evt.subagent_task_id || evt.task_id || '').trim(); if (!parentId || !childId || parentId === childId) return false; const event = String(evt.subagent_event || '').toLowerCase(); - const role = String(evt.subagent_role || '').trim(); + const role = taskKey(evt.subagent_role); setSubagentParent(childId, { parentId, role, model: evt.model }); // Worker narration carries subagent_event="progress" too. It is activity, // not a lifecycle row: route it through the progress key so the later @@ -2571,7 +2578,7 @@ export function createChatInstance({ } function routeSubagentFinalMessageToCard(taskId, msg) { - const childId = String(taskId || '').trim(); + const childId = taskKey(taskId); const info = subagentChildParents.get(childId); if (!childId || !info) return false; const { parentId, role, model } = info; @@ -3984,7 +3991,7 @@ export function createChatInstance({ } function showTyping(activityId = '', meta = {}) { - const actId = String(activityId || '').trim() || ('direct-' + chatId); + const actId = taskKey(activityId) || ('direct-' + chatId); // A typing frame after its turn's keyed final must not resurrect the // concluded turn — but it still carries the activity<->cmid link, so // it settles the linked submission (broadcasts are not ordered). @@ -4050,7 +4057,7 @@ export function createChatInstance({ if (!currentRecord || currentRecord.isSubagent || subagentChildParents.has(taskId)) return; attachTaskDetailReviews(taskId, detail); const cancelPending = taskCancelPending(detail); - if (cancelPending || String(detail?.status || '').trim()) { + if (cancelPending || taskKey(detail?.status)) { markReviewAnchor(currentRecord); } if (cancelPending) { @@ -4075,7 +4082,7 @@ export function createChatInstance({ } function observeMissingManagedTask(taskId) { - const id = String(taskId || '').trim(); + const id = taskKey(taskId); if (!id || concludedDirectActivities.has(id)) return; const record = liveCardRecords.get(id); if (subagentChildParents.has(id) || record?.isSubagent) return; diff --git a/web/modules/review_dom_patch.js b/web/modules/review_dom_patch.js index f03fec428..99a02f61f 100644 --- a/web/modules/review_dom_patch.js +++ b/web/modules/review_dom_patch.js @@ -2,6 +2,7 @@ function reviewNodeKey(node) { const dataset = node?.dataset || {}; if (Object.hasOwn(dataset, 'reviewSection')) return 'section'; if (Object.hasOwn(dataset, 'reviewSectionToggle')) return 'section-toggle'; + if (Object.hasOwn(dataset, 'reviewHydrateStatus')) return 'hydrate-status'; if (dataset.reviewGroup) return `group:${dataset.reviewGroup}`; if (dataset.reviewGroupToggle) return `group-toggle:${dataset.reviewGroupToggle}`; if (dataset.reviewAttempt) return `attempt:${dataset.reviewAttempt}`; diff --git a/web/modules/review_presentation.js b/web/modules/review_presentation.js index 880596e8e..d9af10e5e 100644 --- a/web/modules/review_presentation.js +++ b/web/modules/review_presentation.js @@ -461,6 +461,66 @@ function currentPlanAttempt(current, index) { }; } +function planFindingLines(wave) { + const findings = (Array.isArray(wave.findings) ? wave.findings : []) + .filter((item) => item && typeof item === 'object'); + const dispositions = (Array.isArray(wave.dispositions) ? wave.dispositions : []) + .filter((item) => item && typeof item === 'object'); + // EVERY disposition row renders: the backend refuses duplicate rows for + // one finding as contradictory intent and keeps the finding open, so + // showing only the first would present a refused decision as operative. + const dispositionsByFinding = new Map(); + for (const disposition of dispositions) { + const key = text(disposition.finding_id); + const bucket = dispositionsByFinding.get(key) || []; + bucket.push(disposition); + dispositionsByFinding.set(key, bucket); + } + const dispositionLine = (disposition, prefix) => ( + `${prefix}${text(disposition.decision) || 'disposition'}${text(disposition.rationale) ? ` — ${text(disposition.rationale)}` : ''}` + ); + const lines = []; + for (const finding of findings) { + const head = [ + `[${text(finding.class) || 'finding'}] ${text(finding.summary) || '(no summary)'}`, + text(finding.breaks) ? `breaks ${text(finding.breaks)}` : '', + text(finding.locator) ? `at ${text(finding.locator)}` : '', + ].filter(Boolean).join(' — '); + const source = [text(finding.slot), text(finding.model)].filter(Boolean).join(' · '); + lines.push(source ? `${head} — ${source}` : head); + if (text(finding.recommendation)) lines.push(` fix: ${text(finding.recommendation)}`); + const findingId = text(finding.finding_id); + if (findingId) { + for (const disposition of dispositionsByFinding.get(findingId) || []) { + lines.push(dispositionLine(disposition, ' agent: ')); + } + dispositionsByFinding.delete(findingId); + } + } + if (dispositionsByFinding.size) { + lines.push('General dispositions:'); + for (const [findingId, bucket] of dispositionsByFinding) { + for (const disposition of bucket) { + lines.push(dispositionLine(disposition, ` ${findingId || '(no finding id)'}: `)); + } + } + } + return lines; +} + +function planActorAvailabilityLines(wave) { + // The bug report's own bar: a result that was never received must say so + // explicitly instead of contributing silently-zero findings. + const lines = []; + for (const actor of (Array.isArray(wave.actors) ? wave.actors : [])) { + if (!actor || typeof actor !== 'object' || actor.ok !== false) continue; + const identity = [text(actor.slot_id), text(actor.model)].filter(Boolean).join(' · ') || 'reviewer'; + const cause = text(actor.failure_code) || text(actor.error) || 'no parseable verdict'; + lines.push(`Reviewer unavailable: ${identity} — ${cause}`); + } + return lines; +} + function planWaveDetail(wave) { const lines = [ wave.aggregate ? `Verdict: ${wave.aggregate}` : '', @@ -468,8 +528,36 @@ function planWaveDetail(wave) { wave.paid != null ? `Reviewer panel dispatched: ${wave.paid ? 'yes' : 'no'}` : '', wave.quorum_unreachable ? 'Quorum unavailable' : '', wave.cycles_exhausted ? 'Review cycles exhausted' : '', - wave.reason ? `Reason: ${wave.reason}` : '', + wave.reason ? `Reason: ${text(wave.reason)}` : '', ]; + const counts = wave.counts && typeof wave.counts === 'object' ? wave.counts : {}; + if (wave.compact) { + // A compacted wave keeps counts while its finding bodies moved to the + // immutable wave artifact; name that remainder instead of rendering a + // bound that looks like the whole record. + const recorded = [ + finiteCount(counts.findings) != null ? `${finiteCount(counts.findings)} findings` : '', + finiteCount(counts.blocking) != null ? `${finiteCount(counts.blocking)} blocking` : '', + finiteCount(counts.dispositions) != null ? `${finiteCount(counts.dispositions)} dispositions` : '', + ].filter(Boolean).join(' · '); + if (recorded) lines.push(`Recorded: ${recorded}`); + const artifact = wave.wave_artifact && typeof wave.wave_artifact === 'object' ? wave.wave_artifact : {}; + const sha = text(artifact.sha256); + lines.push(`Finding bodies compacted${sha ? ` · artifact sha256=${sha.slice(0, 12)}…` : ''}${finiteCount(artifact.bytes) != null ? ` (${finiteCount(artifact.bytes)} bytes)` : ''}`); + return lines.filter(Boolean).join('\n'); + } + const countParts = ['blocking', 'note', 'need_evidence'] + .filter((key) => finiteCount(counts[key]) != null) + .map((key) => `${finiteCount(counts[key])} ${key}`); + if (countParts.length) lines.push(`Findings: ${countParts.join(' · ')}`); + lines.push(...planFindingLines(wave)); + lines.push(...planActorAvailabilityLines(wave)); + const findingsShown = (Array.isArray(wave.findings) ? wave.findings : []).length; + if (wave.findings_paged && finiteCount(wave.findings_total) != null) { + lines.push(`Showing ${findingsShown} of ${finiteCount(wave.findings_total)} findings (per-slot page cap)`); + } + if (wave.findings_texts_truncated) lines.push('Some finding texts were truncated at capture.'); + if (wave.spec_body_truncated) lines.push('Spec body was truncated at capture.'); return lines.filter(Boolean).join('\n'); } @@ -642,12 +730,39 @@ export function formatReviewProjection(projection) { if (binding.length) lines.push(`Panel binding: ${binding.join(' · ')}`); (Array.isArray(panel.actors) ? panel.actors : []).forEach((actor) => { if (!actor || typeof actor !== 'object') return; + const slotId = String(actor.slot_id || '?'); lines.push( - `Reviewer ${String(actor.slot_id || '?')}: role=${String(actor.actor_role || 'reviewer')} · provider=${String(actor.provider || 'unknown')} · model=${String(actor.model || 'unknown')} · transport=${String(actor.transport_status || 'unknown')} · parse=${String(actor.parse_status || 'unknown')} · verdict=${String(actor.semantic_verdict || 'none')}${actor.outcome_tier ? ` · outcome_tier=${String(actor.outcome_tier)}` : ''}${actor.dialogue_status ? ` · dialogue=${String(actor.dialogue_status)}` : ''} · quorum=${actor.quorum_contribution ? 'contributes' : 'abstains'} · enforcement=${String(actor.enforcement_impact || 'unknown')}`, + `Reviewer ${slotId}: role=${String(actor.actor_role || 'reviewer')} · provider=${String(actor.provider || 'unknown')} · model=${String(actor.model || 'unknown')} · transport=${String(actor.transport_status || 'unknown')} · parse=${String(actor.parse_status || 'unknown')} · verdict=${String(actor.semantic_verdict || 'none')}${actor.outcome_tier ? ` · outcome_tier=${String(actor.outcome_tier)}` : ''}${actor.dialogue_status ? ` · dialogue=${String(actor.dialogue_status)}` : ''} · quorum=${actor.quorum_contribution ? 'contributes' : 'abstains'} · enforcement=${String(actor.enforcement_impact || 'unknown')}`, ); const actorCoverage = compactCoverage(actor.coverage); - if (actorCoverage) lines.push(`Reviewer ${String(actor.slot_id || '?')} coverage: ${actorCoverage}`); - if (actor.reason) lines.push(`Reviewer ${String(actor.slot_id || '?')} reason: ${String(actor.reason)}`); + if (actorCoverage) lines.push(`Reviewer ${slotId} coverage: ${actorCoverage}`); + if (actor.reason) lines.push(`Reviewer ${slotId} reason: ${String(actor.reason)}`); + if (Array.isArray(actor.findings)) { + for (const finding of actor.findings) { + if (!finding || typeof finding !== 'object') continue; + const label = [text(finding.severity), text(finding.verdict)] + .filter(Boolean).join(' ') || 'finding'; + const title = text(finding.item) || text(finding.summary) || '(no item)'; + const summaryText = text(finding.summary); + const body = [ + `[${label}]${text(finding.id) ? ` ${text(finding.id)}` : ''} ${title}`, + summaryText && summaryText !== title ? `summary: ${summaryText}` : '', + text(finding.reason) ? `reason: ${text(finding.reason)}` : '', + text(finding.evidence) ? `evidence: ${text(finding.evidence)}` : '', + text(finding.recommendation) ? `fix: ${text(finding.recommendation)}` : '', + ].filter(Boolean).join(' — '); + lines.push(`Reviewer ${slotId} finding: ${body}`); + } + const omitted = finiteCount(actor.findings_omitted); + if (omitted) lines.push(`Reviewer ${slotId} findings omitted: ${omitted}`); + } + // P1: name the durable full copy unconditionally — bounded rows, + // per-string truncation markers and pre-findings-era projections + // all resolve through the same observability call. + const callId = text(actor.response_ref?.call_id); + if (callId) { + lines.push(`Reviewer ${slotId} full response: observability call ${callId}`); + } }); }); return lines.join('\n'); @@ -954,20 +1069,38 @@ function reviewRevision(value) { * not counters. A distinct token arriving during a GET schedules one trailing * read; an identical applied/in-flight/pending token is a no-op/same flight. */ -export function createReviewHydrator({ fetchDetail, applyDetail } = {}) { +export function createReviewHydrator({ fetchDetail, applyDetail, onState = () => {} } = {}) { const states = new Map(); const start = (taskId, state, revision) => { const generation = ++state.generation; state.inFlightRevision = revision; + // Status is typed presentation state, not a per-attempt DOM FSM: a + // first load (or a retry after a failure) announces itself; routine + // background refreshes over already-applied content stay silent. + const notify = (status) => { + state.lastStatus = status; + onState(taskId, status); + }; + if (!state.everApplied || state.lastStatus === 'error') notify('loading'); const request = Promise.resolve() .then(() => fetchDetail(taskId)) - .then((detail) => applyDetail(taskId, detail)) + .then((detail) => { + // A strict fetch seam REJECTS on a failed read; a null detail + // means the record is genuinely absent (404) — not an error. + if (detail === null || detail === undefined) return false; + return applyDetail(taskId, detail); + }) .then((applied) => { if (applied !== false && revision !== null) state.appliedRevision = revision; + state.everApplied = true; + notify('idle'); return applied; }) - .catch(() => false) + .catch(() => { + notify('error'); + return false; + }) .finally(() => { if (state.inFlight !== request || state.generation !== generation) return; state.inFlight = null; @@ -993,6 +1126,8 @@ export function createReviewHydrator({ fetchDetail, applyDetail } = {}) { pendingRevision: null, inFlight: null, generation: 0, + everApplied: false, + lastStatus: 'idle', }; states.set(taskId, state); } @@ -1108,12 +1243,24 @@ export function reviewReferenceFromRow(row) { export function renderReviewsSection(groupsInput, disclosure = {}) { const groups = orderedReviewGroups(groupsInput); - if (!groups.length) return ''; + // A failed FIRST hydration has no groups to hang the error on: the shell + // renders anyway and stays mounted through the retry's own loading pass + // (hadHydrateError) so the recovery control cannot unmount mid-flight. A + // quiet first-load zero-group loading pass stays invisible (every card + // expand hydrates, most tasks have no reviews). + const hydrateStatus = text(disclosure.hydrateStatus); + const emptyShell = !groups.length && ( + hydrateStatus === 'error' + || (hydrateStatus === 'loading' && disclosure.hadHydrateError === true) + ); + if (!groups.length && !emptyShell) return ''; const expandedGroups = disclosure.expandedGroups instanceof Set ? disclosure.expandedGroups : new Set(); const expandedAttempts = disclosure.expandedAttempts instanceof Set ? disclosure.expandedAttempts : new Set(); const sectionExpanded = disclosure.sectionExpanded === true; const { groupCount, activeCount } = reviewGroupCounts(groups); - const countText = `${groupCount}${activeCount ? ` · ${activeCount} active` : ''}`; + const countText = groupCount + ? `${groupCount}${activeCount ? ` · ${activeCount} active` : ''}` + : '—'; const groupHtml = groups.map((group) => { const groupExpanded = expandedGroups.has(group.id); const shown = group.countIsAuthoritative ? `${group.attemptCount}` : `${group.attempts.length} shown`; @@ -1174,12 +1321,27 @@ export function renderReviewsSection(groupsInput, disclosure = {}) { `; }).join(''); + // Section-level hydration truth (typed controller state, no per-attempt + // FSM): a first load announces itself, a failed refresh names itself and + // offers Retry instead of silently presenting stale-or-missing detail. + // The message rides inside a : the keyed status node is patched in + // place across loading↔error, and the DOM patcher syncs text only through + // childless-element innerHTML — a bare text node beside the Retry button + // would survive the transition stale. + const hydrateHtml = hydrateStatus === 'loading' + ? '
Loading review details…
' + : (hydrateStatus === 'error' + ? '' + : ''); + // The status node sits OUTSIDE the collapsible groups container: a failed + // refresh stays visible on a collapsed section and the keyed node survives + // loading↔error transitions. Disclosure stays user-owned — nothing expands. return `
-
${groupHtml}
+ ${hydrateHtml}
${groupHtml}
`; } @@ -1233,10 +1395,14 @@ export function createReviewPresentationController({ if (!host || !summary) return; const focused = focusedControl(); const { groupCount, activeCount } = reviewGroupCounts(groups); - summary.hidden = groupCount === 0; + const failedEmpty = groupCount === 0 && ( + state.hydrateStatus === 'error' + || (state.hydrateStatus === 'loading' && state.hadHydrateError === true) + ); + summary.hidden = groupCount === 0 && !failedEmpty; summary.textContent = groupCount ? `Reviews ${groupCount}${activeCount ? ` · ${activeCount} active` : ''}` - : ''; + : (failedEmpty ? 'Reviews' : ''); const reconciled = reconcileReviewMarkup(host, renderReviewsSection(groups, state)); const active = host?.ownerDocument?.activeElement; if (!reconciled || !active || !host.contains?.(active)) restoreFocus(focused); @@ -1250,6 +1416,19 @@ export function createReviewPresentationController({ }; host?.addEventListener('click', (event) => { + const hydrateRetry = event.target?.closest?.('[data-review-hydrate-retry]'); + if (hydrateRetry) { + // A failed pass never records an applied revision, so a plain + // revision-less re-hydrate always re-issues the physical GET. + onHydrate(); + const status = host.querySelector?.('[data-review-hydrate-status]'); + if (status) { + status.setAttribute?.('tabindex', '-1'); + status.focus?.(); + } + onLayout(); + return; + } const retry = event.target?.closest?.('[data-skill-review-retry]'); if (retry) { const detail = retry.closest?.('[data-review-attempt-detail]'); @@ -1307,5 +1486,15 @@ export function createReviewPresentationController({ if (changed) render(); return changed; }, + setHydrateStatus(statusValue) { + const status = text(statusValue); + if (state.hydrateStatus === status) return; + // Remember a failure across the retry's loading pass so the + // zero-group shell stays mounted until the retry settles. + if (status === 'error') state.hadHydrateError = true; + else if (status === 'idle') state.hadHydrateError = false; + state.hydrateStatus = status; + render(); + }, }; } diff --git a/web/modules/skill_card_renderer.js b/web/modules/skill_card_renderer.js index 143e4611f..acb4f994a 100644 --- a/web/modules/skill_card_renderer.js +++ b/web/modules/skill_card_renderer.js @@ -177,7 +177,10 @@ function reviewRunTitle(run) { function reviewHistory(skill) { const review = skill.skill_review && typeof skill.skill_review === 'object' ? skill.skill_review : {}; - const history = Array.isArray(review.history) ? review.history.slice(-10) : []; + // The backend projection already bounds history to its ten-row window and + // discloses the exact omitted count; a second client-side slice would be + // an undisclosed bound on top of a disclosed one. + const history = Array.isArray(review.history) ? review.history : []; const current = review.current && Object.keys(review.current).length ? review.current : history[history.length - 1]; if (!current) return ''; @@ -186,9 +189,13 @@ function reviewHistory(skill) { const source = run.source ? ` · ${run.source}` : ''; return `
  • ${escapeHtml(reviewRunTitle(run))} · ${escapeHtml(status)}${escapeHtml(source)}
  • `; }).join(''); + const omitted = Number(review.history_omitted); + const historyLabel = Number.isFinite(omitted) && omitted > 0 + ? `${history.length} of ${history.length + omitted}` + : `${history.length}`; const currentStatus = current.review_status || current.status || current.job_status || 'unknown'; return `
    ${escapeHtml(reviewRunTitle(current))} · ${escapeHtml(currentStatus)}
    - ${rows ? `
    Skill Review history (${history.length})
      ${rows}
    ` : ''}`; + ${rows ? `
    Skill Review history (${historyLabel})
      ${rows}
    ` : ''}`; } function grantBlock(skill) { diff --git a/web/modules/utils.js b/web/modules/utils.js index 485eecd0a..4d2844a29 100644 --- a/web/modules/utils.js +++ b/web/modules/utils.js @@ -99,7 +99,8 @@ export function topReviewFinding(entity) { const label = first.item || first.check || first.title || 'finding'; const verdict = first.verdict || first.severity || ''; const reason = first.reason || first.message || ''; - return `${verdict ? `${verdict} ` : ''}${label}: ${reason}`.trim(); + const more = findings.length > 1 ? ` (+${findings.length - 1} more)` : ''; + return `${`${verdict ? `${verdict} ` : ''}${label}: ${reason}`.trim()}${more}`; } export function renderHubCard(item, { diff --git a/web/tests/marketplace_review_hint.test.js b/web/tests/marketplace_review_hint.test.js new file mode 100644 index 000000000..965195399 --- /dev/null +++ b/web/tests/marketplace_review_hint.test.js @@ -0,0 +1,20 @@ +import assert from 'node:assert/strict'; +import test from 'node:test'; + +import { topReviewFinding } from '../modules/utils.js'; + +test('the compact review hint discloses how many further findings it hides', () => { + const one = topReviewFinding({ review_findings: [ + { verdict: 'FAIL', item: 'writes outside payload', reason: 'escapes the bucket' }, + ] }); + assert.equal(one, 'FAIL writes outside payload: escapes the bucket'); + + const three = topReviewFinding({ review_findings: [ + { verdict: 'FAIL', item: 'writes outside payload', reason: 'escapes the bucket' }, + { verdict: 'WARN', item: 'b', reason: 'r' }, + { verdict: 'WARN', item: 'c', reason: 'r' }, + ] }); + assert.equal(three, 'FAIL writes outside payload: escapes the bucket (+2 more)'); + + assert.equal(topReviewFinding({ review_findings: [] }), ''); +}); diff --git a/web/tests/review_findings_detail.test.js b/web/tests/review_findings_detail.test.js new file mode 100644 index 000000000..9e4500611 --- /dev/null +++ b/web/tests/review_findings_detail.test.js @@ -0,0 +1,290 @@ +import assert from 'node:assert/strict'; +import test from 'node:test'; + +import { + createReviewHydrator, + createReviewPresentationController, + planReviewGroupFromTaskDetail, + renderReviewsSection, + taskAcceptanceGroupFromTaskDetail, +} from '../modules/review_presentation.js'; +import { reconcileReviewElementTree } from '../modules/review_dom_patch.js'; + +test('a full Plan wave renders findings, mapped dispositions, degraded reviewers and honesty stamps', () => { + const fingerprint = 'd'.repeat(64); + const group = planReviewGroupFromTaskDetail({ + task_id: 'root', + plan_review_state: { + schema_version: 2, + current_attempt: { fingerprint, status: 'open' }, + waves: [{ + request_fingerprint: fingerprint, + cycle_index: 2, + aggregate: 'REVISE_PLAN', + closed: false, + paid: true, + reason: 'blocking_slots_at_quorum:2/3', + counts: { blocking: 1, note: 1, need_evidence: 0 }, + findings: [ + { + finding_id: 'slot_1:f1', id: 'f1', class: 'blocking', + summary: 'The rollback path loses the stash', breaks: 'invariant_2', + locator: 'supervisor/update_merge.py', + recommendation: 'Restore the stash before reset', + slot: 'slot_1', model: 'anthropic/claude-opus-5', + }, + { + finding_id: 'slot_2:f1', id: 'f1', class: 'note', + summary: 'Naming could be clearer', slot: 'slot_2', + }, + ], + dispositions: [ + { finding_id: 'slot_1:f1', decision: 'reject', rationale: 'stash is restored by boot finalize' }, + { finding_id: 'slot_1:f1', decision: 'accept', rationale: 'second thoughts after re-reading' }, + { finding_id: 'slot_9:gone', decision: 'accept', rationale: 'will fold into phase 2' }, + ], + actors: [ + { slot_id: 'slot_1', model: 'anthropic/claude-opus-5', ok: true }, + { slot_id: 'slot_3', model: 'openai/gpt-5.6-sol', ok: false, failure_code: 'window_exhausted' }, + ], + findings_paged: true, + findings_total: 40, + findings_texts_truncated: true, + spec_body_truncated: true, + }], + waves_omitted: 0, + }, + }); + + const detail = group.attempts[0].detailText; + assert.match(detail, /Findings: 1 blocking · 1 note · 0 need_evidence/); + assert.match(detail, /\[blocking\] The rollback path loses the stash — breaks invariant_2 — at supervisor\/update_merge\.py — slot_1 · anthropic\/claude-opus-5/); + assert.match(detail, / fix: Restore the stash before reset/); + assert.match(detail, / agent: reject — stash is restored by boot finalize/); + // Contradictory duplicate dispositions both render: the backend refuses + // the pair and keeps the finding open, so hiding one would present the + // other as operative. + assert.match(detail, / agent: accept — second thoughts after re-reading/); + assert.match(detail, /\[note\] Naming could be clearer — slot_2/); + assert.match(detail, /General dispositions:\n slot_9:gone: accept — will fold into phase 2/); + assert.match(detail, /Reviewer unavailable: slot_3 · openai\/gpt-5\.6-sol — window_exhausted/); + assert.match(detail, /Showing 2 of 40 findings \(per-slot page cap\)/); + assert.match(detail, /Some finding texts were truncated at capture\./); + assert.match(detail, /Spec body was truncated at capture\./); + assert.match(detail, /Cost unavailable/); + // The rendered section carries the finding text to the reader. + const html = renderReviewsSection([group], { + sectionExpanded: true, + expandedGroups: new Set(['plan:root']), + expandedAttempts: new Set([`plan:root:${group.attempts[0].id}`]), + }); + assert.match(html, /The rollback path loses the stash/); +}); + +test('a compact Plan wave names its recorded counts and the immutable artifact remainder', () => { + const fingerprint = 'e'.repeat(64); + const group = planReviewGroupFromTaskDetail({ + task_id: 'root', + plan_review_state: { + schema_version: 2, + current_attempt: {}, + waves: [{ + compact: true, + request_fingerprint: fingerprint, + cycle_index: 1, + aggregate: 'GREEN', + closed: true, + counts: { findings: 5, dispositions: 3, blocking: 2 }, + wave_artifact: { root: 'artifact_store', path: 'w.json', sha256: 'abc123def4567890', bytes: 321 }, + }], + waves_omitted: 0, + }, + }); + const detail = group.attempts[0].detailText; + assert.match(detail, /Recorded: 5 findings · 2 blocking · 3 dispositions/); + assert.match(detail, /Finding bodies compacted · artifact sha256=abc123def456… \(321 bytes\)/); + assert.doesNotMatch(detail, /w\.json/); +}); + +test('the hydrator announces first load, failure and retry without narrating background refreshes', async () => { + const events = []; + let mode = 'ok'; + const hydrator = createReviewHydrator({ + fetchDetail: async () => { + if (mode === 'reject') throw new Error('transport failed'); + return mode === 'missing' ? null : { ok: true }; + }, + applyDetail: () => true, + onState: (taskId, status) => events.push(`${taskId}:${status}`), + }); + + mode = 'reject'; + assert.equal(await hydrator.hydrate('root', 'a'.repeat(64)), false); + assert.deepEqual(events, ['root:loading', 'root:error']); + + events.length = 0; + mode = 'ok'; + await hydrator.hydrate('root', 'a'.repeat(64)); + assert.deepEqual( + events, ['root:loading', 'root:idle'], + 'a plain re-hydrate after failure re-fetches (no applied receipt was recorded) and announces', + ); + + events.length = 0; + await hydrator.hydrate('root', 'b'.repeat(64)); + assert.deepEqual(events, ['root:idle'], 'a background refresh over applied content stays silent'); + + events.length = 0; + mode = 'missing'; + await hydrator.hydrate('gone', 'c'.repeat(64)); + assert.deepEqual( + events, ['gone:loading', 'gone:idle'], + 'a genuinely absent record (404 → null) is not an error', + ); +}); + +test('the section renders hydration truth and the controller retries through onHydrate', () => { + const errorHtml = renderReviewsSection([ + taskAcceptanceGroupFromTaskDetail({ + task_id: 'root', + review_projection: { panels: [{ surface: 'task_acceptance', panel_id: 'p1', aggregate_signal: 'PASS' }] }, + }, 'root'), + ], { sectionExpanded: true, hydrateStatus: 'error' }); + assert.match(errorHtml, /data-review-hydrate-status/); + assert.match(errorHtml, /role="alert"/); + assert.match(errorHtml, /Review details failed to refresh/); + assert.match(errorHtml, /data-review-hydrate-retry/); + + const loadingHtml = renderReviewsSection([ + taskAcceptanceGroupFromTaskDetail({ + task_id: 'root', + review_projection: { panels: [{ surface: 'task_acceptance', panel_id: 'p1', aggregate_signal: 'PASS' }] }, + }, 'root'), + ], { sectionExpanded: true, hydrateStatus: 'loading' }); + assert.match(loadingHtml, /Loading review details…/); + + const hydrated = []; + let clickHandler = null; + const statusNode = { + setAttribute(key, value) { this[key] = value; }, + focus() { this.focused = true; }, + }; + const controller = createReviewPresentationController({ + host: { + addEventListener: (_type, handler) => { clickHandler = handler; }, + querySelector: (selector) => (selector === '[data-review-hydrate-status]' ? statusNode : null), + }, + summary: null, + onHydrate: (...args) => hydrated.push(args), + }); + controller.setHydrateStatus('error'); + clickHandler({ + target: { + closest: (selector) => (selector === '[data-review-hydrate-retry]' ? {} : null), + }, + }); + assert.deepEqual(hydrated, [[]]); + assert.equal(statusNode.tabindex, '-1'); + assert.equal(statusNode.focused, true); +}); + +test('a failed first hydration renders the section shell with no groups', () => { + assert.equal(renderReviewsSection([], {}), ''); + assert.equal(renderReviewsSection([], { hydrateStatus: 'loading' }), '', + 'a quiet first-load zero-group loading pass stays invisible (every card expand hydrates)'); + const errorShell = renderReviewsSection([], { sectionExpanded: true, hydrateStatus: 'error' }); + assert.match(errorShell, /data-review-section/); + assert.match(errorShell, /Review details failed to refresh/); + assert.match(errorShell, /data-review-hydrate-retry/); + assert.match(errorShell, /chat-review-section-count">—= 0 && statusIndex < groupsIndex, + 'status node renders before (outside) the collapsible groups container'); + assert.match(collapsedShell, /Review details failed to refresh/); + + // A retry's own loading pass keeps the shell mounted (hadHydrateError), + // so the recovery control cannot unmount mid-flight. + const retryLoading = renderReviewsSection([], { hydrateStatus: 'loading', hadHydrateError: true }); + assert.match(retryLoading, /Loading review details…/); + assert.equal(renderReviewsSection([], { hydrateStatus: 'loading', hadHydrateError: false }), ''); +}); + +test('the hydrate status node swaps its message text across the loading→error patch', () => { + class Node { + constructor({ tag = 'div', dataset = {}, classes = [], attrs = {}, html = '', children = [] } = {}) { + this.tagName = tag.toUpperCase(); + this.dataset = { ...dataset }; + this._attrs = new Map(Object.entries(attrs)); + for (const [key, value] of Object.entries(dataset)) { + this._attrs.set(`data-${key.replace(/[A-Z]/g, (c) => `-${c.toLowerCase()}`)}`, String(value)); + } + this._classes = new Set(classes); + if (classes.length) this._attrs.set('class', classes.join(' ')); + this.classList = { contains: (name) => this._classes.has(name) }; + this._innerHTML = html; + this.children = []; + children.forEach((child) => this.insertBefore(child, null)); + } + get attributes() { return [...this._attrs].map(([name, value]) => ({ name, value })); } + get innerHTML() { return this._innerHTML; } + set innerHTML(value) { this._innerHTML = String(value); this.children = []; } + hasAttribute(name) { return this._attrs.has(name); } + setAttribute(name, value) { + this._attrs.set(name, String(value)); + if (name.startsWith('data-')) { + this.dataset[name.slice(5).replace(/-([a-z])/g, (_a, c) => c.toUpperCase())] = String(value); + } + } + removeAttribute(name) { this._attrs.delete(name); } + insertBefore(child, before) { + child.remove(); + const index = before ? this.children.indexOf(before) : -1; + if (index >= 0) this.children.splice(index, 0, child); else this.children.push(child); + child.parentElement = this; + return child; + } + remove() { + if (!this.parentElement) return; + const index = this.parentElement.children.indexOf(this); + if (index >= 0) this.parentElement.children.splice(index, 1); + this.parentElement = null; + } + cloneNode(deep) { + return new Node({ + tag: this.tagName, + dataset: this.dataset, + classes: [...this._classes], + attrs: Object.fromEntries(this._attrs), + html: this._innerHTML, + children: deep ? this.children.map((child) => child.cloneNode(true)) : [], + }); + } + } + const statusNode = (state) => new Node({ + dataset: { reviewHydrateStatus: '' }, + classes: [state === 'error' ? 'skill-review-error' : 'skill-review-loading'], + children: state === 'error' + ? [ + new Node({ tag: 'span', html: 'Review details failed to refresh — shown data may be incomplete. ' }), + new Node({ tag: 'button', dataset: { reviewHydrateRetry: '' }, html: 'Retry' }), + ] + : [new Node({ tag: 'span', html: 'Loading review details…' })], + }); + + const current = statusNode('loading'); + assert.equal(reconcileReviewElementTree(current, statusNode('error')), true); + assert.match(current.children[0].innerHTML, /failed to refresh/); + assert.doesNotMatch(current.children[0].innerHTML, /Loading review details/); + assert.equal(current.children.length, 2); + assert.equal(current.children[1].innerHTML, 'Retry'); + + assert.equal(reconcileReviewElementTree(current, statusNode('loading')), true); + assert.match(current.children[0].innerHTML, /Loading review details/); + assert.equal(current.children.length, 1); +}); diff --git a/web/tests/review_truth.test.js b/web/tests/review_truth.test.js index 86dec356e..e9a2d2777 100644 --- a/web/tests/review_truth.test.js +++ b/web/tests/review_truth.test.js @@ -448,3 +448,50 @@ test('an evidence-bearing chip is marked so sticky rendering can refuse downgrad execution_evidence: { delegated_runs_started: 1, delegated_runs_settled: 1, delegated_runs_failed: 0, subscription_cost_usd: null }, }).hasEvidence, true); }); + +test('actor findings rows, omitted counts and the durable pointer ride the shared formatter', () => { + const text = formatReviewProjection({ panels: [{ + panel_id: 'panel_f', surface: 'task_acceptance', aggregate_signal: 'FAIL', + quorum: { required: 2, contributed: 2, configured: 3 }, + actors: [ + { + slot_id: 'slot_1', model: 'm1', reason: 'summary text', + coverage: { criteria_total: 3, findings: 10 }, + findings: [ + { + id: 'f1', severity: 'critical', item: 'Preflop sizing is wrong', + summary: 'Sizing deviates from the baseline in early position', + evidence: 'raises 2bb from UTG', recommendation: 'raise 2.5bb', + }, + { severity: 'low', verdict: 'FAIL', item: 'Style nit', reason: 'inconsistent spacing' }, + ], + findings_omitted: 8, + response_ref: { call_id: 'review_task_acceptance_slot_1_resp', sha256: 'a'.repeat(64) }, + }, + { + slot_id: 'slot_2', model: 'm2', + coverage: { criteria_total: 3, findings: 0 }, + findings: [], findings_omitted: 0, + response_ref: { call_id: 'c2' }, + }, + { + // A pre-findings-era projection: count says findings existed, + // rows are absent — the pointer is the only resolvable source. + slot_id: 'slot_3', model: 'm3', + coverage: { criteria_total: 3, findings: 4 }, + response_ref: { call_id: 'c3' }, + }, + ], + }] }); + + assert.match(text, /Reviewer slot_1 finding: \[critical\] f1 Preflop sizing is wrong — summary: Sizing deviates from the baseline in early position — evidence: raises 2bb from UTG — fix: raise 2\.5bb/); + assert.match(text, /Reviewer slot_1 finding: \[low FAIL\] Style nit — reason: inconsistent spacing/); + assert.match(text, /Reviewer slot_1 findings omitted: 8/); + assert.match(text, /Reviewer slot_1 full response: observability call review_task_acceptance_slot_1_resp/); + assert.doesNotMatch(text, /Reviewer slot_2 finding:/); + // The durable pointer is unconditional: bounded rows, per-string + // truncation markers and pre-findings-era projections all resolve there. + assert.match(text, /Reviewer slot_2 full response: observability call c2/); + assert.match(text, /Reviewer slot_3 full response: observability call c3/); + assert.doesNotMatch(text, /Reviewer slot_3 finding:/); +}); diff --git a/web/tests/skill_review_history.test.js b/web/tests/skill_review_history.test.js index e0f8fca56..dcc4abc4f 100644 --- a/web/tests/skill_review_history.test.js +++ b/web/tests/skill_review_history.test.js @@ -3,7 +3,7 @@ import test from 'node:test'; import { renderInstalledSkillCard } from '../modules/skill_card_renderer.js'; -test('skill card shows current review round and collapses only the last ten group rows', () => { +test('skill card shows the current review round over the runner-bounded ten-row window', () => { const history = Array.from({ length: 10 }, (_, idx) => ({ status: 'clean', content_hash: `snapshot-${idx}`, @@ -38,3 +38,34 @@ test('skill card shows current review round and collapses only the last ten grou assert.match(html, /Skill Review history \(10\)/); assert.doesNotMatch(html, /must stay private/); }); + +test('a bounded history window names the exact omitted remainder in its label', () => { + const history = Array.from({ length: 10 }, (_, idx) => ({ + status: 'clean', + content_hash: `snapshot-${idx}`, + group_id: 'manual:alpha', + review_round: idx + 15, + snapshot_attempt: 1, + })); + const skill = { + name: 'alpha', + type: 'instruction', + version: '1.0.0', + description: 'test', + source: 'external', + enabled: false, + review_status: 'clean', + review_gate: { executable_review: true }, + review_findings: [], + permissions: [], + grants: {}, + skill_review: { + current: history[history.length - 1], + history, + history_omitted: 14, + }, + }; + + const html = renderInstalledSkillCard(skill); + assert.match(html, /Skill Review history \(10 of 24\)/); +});