fix(sessions): close crystallize review-gate bypass via summary page

[galuis116]

Problem

sessions.crystallize writes a durable Page artifact directly via
store.put_page(page) (src/vouch/sessions.py:99-110), bypassing
the propose_page + approve review gate that guards every other
write to pages/. The page body is built by _build_summary_body
(src/vouch/sessions.py:125-139) and embeds three agent-controlled
fields verbatim into rendered markdown:

• sess.task — set by the agent at session_start(task=...)
• sess.note — set/extended by the agent at session_start / session_end
• sess.agent — taken from the agent's VOUCH_AGENT env var

There is no escape, no sanitisation, and no review step. Result:
any agent that opens a session and gets one trivial claim approved
via crystallize can land arbitrary markdown into
pages/session-<id>.md — the page is committed to git, returned by
kb.read_page / kb.list_pages / kb.context, and (once #60 is
fixed) surfaces in kb.search. The approver reviews the claim,
not the session task; their approval action causes a page with
unreviewed agent-chosen content to land.

Secondary audit gap: the session.crystallize audit event at
src/vouch/sessions.py:112-116 lists [sess.id, *approved_artifact_ids]
as object_ids — the new page id is never in the list. So
vouch audit cannot attribute the page write, defeating the
audit-truthfulness guarantee.

This breaks vouch's #1 stated guarantee (README §"Why this exists"
point 2, CONTRIBUTING §"Things we won't merge" point 1):
"Writes require approval. Agents file proposals; a human (or
trusted approving agent) explicitly accepts them."

Fix

Two changes in src/vouch/sessions.py:

1. _build_summary_body is restricted to fields the proposing agent
   cannot influence: session id (server-generated), started_at and
   ended_at (set from the server clock at session_start /
   session_end), and the list of crystallized artifact ids
   (each of which did go through the review gate). The
   agent-controlled fields (sess.task, sess.note, sess.agent)
   are no longer promoted into the durable Page body — they remain
   on the Session model and are still queryable through
   store.get_session, but no longer surface in kb.read_page /
   kb.search / kb.context.

2. The session.crystallize audit event now includes
   summary_page_id in object_ids whenever a page is written, so
   vouch audit truthfully attributes every artifact the call
   produced.

This is the strict-derivation fix (option (a) in the issue's
"Suggested fix" section); the alternative of routing the page
through propose_page + approve would materially change
crystallize's return shape and is left for a separate VEP.

No on-disk-layout, schema, or bundle-format change. Existing
sessions are unaffected: only newly-crystallized session-summary
pages take the new body shape.

Tests

Added two regression tests in tests/test_sessions.py:

• test_crystallize_summary_page_does_not_leak_agent_controlled_fields
  — calls session_start(task=<markdown payload>, note=<more>) with
  agent mallory, files a legitimate claim, runs crystallize,
  and asserts the resulting page body contains none of the
  agent-supplied strings (## DECISION, the injected note,
  mallory).
• test_crystallize_audit_event_records_summary_page_id — reads
  audit.log.jsonl, finds the most recent session.crystallize
  event, and asserts summary_page_id is in its object_ids.

All 4 session tests pass on Windows (Python 3.12.13). Full suite
shows the same pre-existing failures present on main
(#72 bundle path separator on Windows; O_NOFOLLOW use in
storage.py) — both untouched by this PR. Lint clean.

Fixes #76

Summary by CodeRabbit

• Bug Fixes

  • Session summaries no longer include agent-influenced fields; only server-controlled data is persisted.
  • Session crystallization audit events now include the summary page identifier for accurate attribution.

• Documentation

  • Changelog updated to describe the summary and audit behavior changes.

• Tests

  • Added regression tests for summary content restrictions and audit event object IDs.

[coderabbitai]

📝 Walkthrough

Walkthrough

Closes a review-gate bypass: sessions.crystallize no longer includes agent-controlled fields (task, note, agent) in durable session-summary pages; summary bodies are server-controlled only. The session.crystallize audit event now includes the written summary page id in object_ids.

Changes

Session crystallize review-gate bypass closure

Layer / File(s)	Summary
Crystallize audit event and body builder implementation src/vouch/sessions.py	crystallize now builds crystallize_object_ids including the session id, approved artifact ids, and the created summary page id for audit logging. _build_summary_body excludes agent-supplied task, note, and agent, producing a page body with only server-controlled session id, timestamps, and approved artifact ids.
Regression tests validating body content and audit attribution tests/test_sessions.py	Adds tests: one asserts the durable summary page body omits injected agent-controlled task/note and agent-identifying prose; another parses audit.log.jsonl to ensure the session.crystallize audit event includes the produced summary_page_id in object_ids. Also adds json import and minor formatting tweaks.
Changelog documentation of security fix CHANGELOG.md	Adds an Unreleased → Fixed entry documenting the review-gate bypass closure: session-summary markdown no longer embeds agent-influenced fields and audit events now include the summary page id.

sequenceDiagram
  participant crystallize
  participant _build_summary_body
  participant store
  participant audit

  crystallize->>_build_summary_body: construct summary body from session_id and approved_artifact_ids
  _build_summary_body-->>crystallize: return body (session id, timestamps, artifact ids only)
  crystallize->>store: put_page(body)
  store-->>crystallize: summary_page_id
  crystallize->>crystallize: append summary_page_id to crystallize_object_ids
  crystallize->>audit: log_event(name="session.crystallize", object_ids=crystallize_object_ids)

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• vouchdev/vouch#61: Also modifies src/vouch/sessions.py’s crystallize flow (that PR additionally indexes the summary page into FTS5).

Poem

🐰 I hopped through logs, a careful little snoop,
Found pages once wild that let mischief loop.
Now only server-truth may hop into the tree,
Audits keep the tracks so we all can see.
Hooray — no more secret carrots in the soup!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: fixing a security bypass in sessions.crystallize by restricting the summary page body to server-controlled fields.
Linked Issues check	✅ Passed	The PR fully addresses both objectives from issue #76: it restricts summary page body to server-controlled fields only (approach a) and includes summary_page_id in audit event object_ids.
Out of Scope Changes check	✅ Passed	All changes are scoped to closing the identified review-gate bypass: session.py modifications restrict page body and audit logging, tests verify the fix, and CHANGELOG documents it.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[plind-junior]

Fix MR conflict

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/test_sessions.py`:
- Line 7: Remove the unused import causing Ruff F401 by deleting "patch" from
the import statement (i.e., remove or change the line "from unittest.mock import
patch") in tests/test_sessions.py so the file no longer imports the unused
symbol; if other mocks are needed, import only the actually used names from
unittest.mock instead of patch.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 198ebfe2-fc0b-45c3-8791-23b02d05b5b1

📥 Commits

Reviewing files that changed from the base of the PR and between 0ed8a89 and e427404.

📒 Files selected for processing (1)

• tests/test_sessions.py

[coderabbitai]

🧹 Nitpick comments (1)

tests/test_sessions.py (1)

132-135: 💤 Low value

Consider refactoring to avoid double JSON parsing.

Each line is parsed twice: once in the if condition and again for the list comprehension. This is a minor inefficiency.

♻️ Proposed refactor using walrus operator

-    cryst_events = [
-        json.loads(line) for line in audit_lines
-        if json.loads(line).get("event") == "session.crystallize"
-    ]
+    cryst_events = [
+        evt for line in audit_lines
+        if (evt := json.loads(line)).get("event") == "session.crystallize"
+    ]

Alternatively, use a traditional loop:

-    cryst_events = [
-        json.loads(line) for line in audit_lines
-        if json.loads(line).get("event") == "session.crystallize"
-    ]
+    cryst_events = []
+    for line in audit_lines:
+        evt = json.loads(line)
+        if evt.get("event") == "session.crystallize":
+            cryst_events.append(evt)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_sessions.py` around lines 132 - 135, The list comprehension parses
each JSON line twice; change it to parse once by storing the parsed object and
filtering on its "event" field—e.g., use a single json.loads call per line (via
the walrus operator or a simple for-loop) so that cryst_events is built from the
already-parsed dicts; update the expression around audit_lines and json.loads to
reuse the parsed object instead of calling json.loads twice.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@tests/test_sessions.py`:
- Around line 132-135: The list comprehension parses each JSON line twice;
change it to parse once by storing the parsed object and filtering on its
"event" field—e.g., use a single json.loads call per line (via the walrus
operator or a simple for-loop) so that cryst_events is built from the
already-parsed dicts; update the expression around audit_lines and json.loads to
reuse the parsed object instead of calling json.loads twice.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: ec2ffbbc-1165-4e97-a3f2-6f9aed416a3d

📥 Commits

Reviewing files that changed from the base of the PR and between b2e3db1 and df6e9d3.

📒 Files selected for processing (3)

• CHANGELOG.md
• src/vouch/sessions.py
• tests/test_sessions.py

✅ Files skipped from review due to trivial changes (1)

• CHANGELOG.md

[plind-junior]

PR should made against test branch

[galuis116]

@plind-junior — retargeted to test. Base is now vouchdev:test; commits, CodeRabbit nitpicks, and CI history are preserved on this same PR.