diff --git a/CHANGELOG.md b/CHANGELOG.md index 9143a918..1bf5d166 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,32 @@ All notable changes to vouch are documented here. Format follows `vouch triage [proposal-id...]` mirrors it on the CLI with `--json` and `--reverse` (#322). +### Fixed +- `vouch install-mcp ` (codex `toml_merge`): a `config.toml` the minimal + serializer couldn't faithfully re-emit (a non-BMP string value, a `nan`/`inf` + float) was bucketed as `skipped` and printed as `(already present)` with a + clean `Done`, so the user believed vouch was wired into codex when it wasn't. + serializer-failure now lands in a distinct `failed` bucket, is reported as + such, and the command exits non-zero — "already installed" and "install + failed" no longer look the same. +- `vouch install-mcp `: a manifest `dst` that escaped the target tree + (via `..` or an absolute path) is now refused with an `AdapterError` instead + of writing outside `target` (defense in depth for the manifest file writer; + shipped adapters are unaffected). +- dual-solve review-ui: the recommendation hint rendered "neither engine + produced a usable diff" for the entire duration of a still-running job — the + hint was computed over the not-yet-populated candidate list on every poll. + it is now omitted until candidates exist. +- `session.crystallize`: retrying on a session that hasn't been ended rewrote + the `session-` summary page with a fresh wall-clock `Ended:` stamp each + time (and needlessly re-embedded it), so the "idempotent retry" wasn't. an + open session now renders a stable marker, so retries produce an identical + body. +- `vouch capture ingest-codex`: rollout parsing had no size cap and could read + an oversized (or newline-free blob) rollout whole into memory. the file is + now bounded to 64 MiB up front, mirroring the byte caps on other untrusted + reads. + ## [1.2.1] — 2026-07-06 ### Fixed diff --git a/src/vouch/cli.py b/src/vouch/cli.py index 16777133..9d495337 100644 --- a/src/vouch/cli.py +++ b/src/vouch/cli.py @@ -3505,12 +3505,21 @@ def install_mcp( click.echo(f" ~ {f} (merged into existing)") for f in result.skipped: click.echo(f" · {f} (already present)") + for f in result.failed: + click.echo(f" ✗ {f} (could not install — left unchanged)") click.echo( f"Done — {len(result.written)} written, " f"{len(result.appended)} appended, {len(result.merged)} merged, " - f"{len(result.skipped)} skipped " + f"{len(result.skipped)} skipped, {len(result.failed)} failed " f"under {target}" ) + if result.failed: + # a failed install is not a no-op: exit non-zero so scripts (and the + # user) notice vouch was NOT wired into these files. + raise click.ClickException( + f"{len(result.failed)} file(s) could not be installed: " + f"{', '.join(result.failed)}" + ) # --- sync: bidirectional vouch <-> Obsidian-style vault ------------------- diff --git a/src/vouch/codex_rollout.py b/src/vouch/codex_rollout.py index c94684c7..58e8e23d 100644 --- a/src/vouch/codex_rollout.py +++ b/src/vouch/codex_rollout.py @@ -42,6 +42,11 @@ _ZSTD_MAGIC = b"\x28\xb5\x2f\xfd" _MAX_PROMPT_CHARS = 240 +# Upper bound on a rollout file we'll parse. Rollouts carry large tool +# outputs, so this is generous, but an unbounded read (a huge file, or a +# newline-free blob that the streaming reader would slurp whole) must not be +# able to exhaust memory. Mirrors the byte caps elsewhere (fetch, http_server). +_MAX_ROLLOUT_BYTES = 64 * 1024 * 1024 _PATCH_FILE_RE = re.compile(r"^\*\*\* (Add|Update|Delete) File: (.+)$", re.MULTILINE) _EXIT_CODE_RE = re.compile(r"exited with code (\d+)") @@ -151,14 +156,23 @@ def _iter_rollout_lines(path: Path) -> Iterator[str]: """Yield the rollout's lines one at a time, decoded as UTF-8. Streams from the file handle rather than slurping the whole file into - memory — codex rollouts can carry large tool outputs. The zstd magic - header is checked up front (compressed rollouts aren't parsed here). + memory — codex rollouts can carry large tool outputs. The size is checked + up front so an oversized rollout (or a newline-free blob that iterating the + handle would otherwise read whole) is refused instead of exhausting memory. + The zstd magic header is checked up front too (compressed rollouts aren't + parsed here). """ try: fh = path.open("rb") except OSError as e: raise CodexRolloutError(f"cannot read rollout file {path}: {e}") from e with fh: + size = os.fstat(fh.fileno()).st_size + if size > _MAX_ROLLOUT_BYTES: + raise CodexRolloutError( + f"{path.name} is too large to parse " + f"({size} bytes > {_MAX_ROLLOUT_BYTES} byte limit)" + ) if fh.read(4) == _ZSTD_MAGIC: raise CodexRolloutError( f"{path.name} is zstd-compressed; decompress it first " diff --git a/src/vouch/install_adapter.py b/src/vouch/install_adapter.py index 4fce5cd0..e6998c98 100644 --- a/src/vouch/install_adapter.py +++ b/src/vouch/install_adapter.py @@ -73,6 +73,11 @@ class InstallResult: appended: list[str] = field(default_factory=list) skipped: list[str] = field(default_factory=list) merged: list[str] = field(default_factory=list) + # files vouch wanted to install but could not (e.g. a config it can't + # faithfully re-serialize, or a malformed existing file). Kept distinct + # from ``skipped`` so "already installed" and "install failed" never look + # the same to the caller / CLI. + failed: list[str] = field(default_factory=list) @dataclass(frozen=True) @@ -235,6 +240,16 @@ def install(adapter: str, *, target: Path, tier: str = "T4") -> InstallResult: for entry in entries: src = src_root / entry.src dst = target / entry.dst + # Defense in depth: `dst` comes from the manifest. Adapters ship + # with vouch (trusted), but a `..`/absolute `dst` must never let a + # manifest write outside the target tree. `target` is already + # resolved above; resolve `dst` (normalizes `..`, even when the + # file doesn't exist yet) and require containment. + if not dst.resolve().is_relative_to(target): + raise AdapterError( + f"{adapter}: install.yaml dst {entry.dst!r} escapes the " + f"target tree ({target})" + ) if not src.is_file(): # Manifest declares a template that doesn't exist in the # adapter directory: a contributor-time bug, but surface it @@ -588,12 +603,17 @@ def _install_toml_merge( * dst exists, merge adds keys -> merge + rewrite (``merged``) * dst exists, nothing to add -> skip (``skipped``); already installed * dst exists, unparseable -> skip (``skipped``); never clobber the user + * dst exists, can't re-emit -> ``failed``; the minimal serializer can't + round-trip the merged data, so vouch is + left unwired — reported, not silently + bucketed as "already present" Rewriting re-serializes the whole file (comments and formatting are not preserved — same trade-off ``_install_json_merge`` already makes). The serialized result must survive a tomllib round-trip back to the merged data; anything the minimal serializer can't faithfully re-emit degrades - to ``skipped`` rather than risking the user's config. + to ``failed`` (the user's config is left untouched) rather than being + reported as a successful no-op. """ if not dst.exists(): dst.parent.mkdir(parents=True, exist_ok=True) @@ -605,7 +625,8 @@ def _install_toml_merge( dst_data = tomllib.loads(dst.read_text(encoding="utf-8")) src_data = tomllib.loads(src.read_text(encoding="utf-8")) except (OSError, UnicodeDecodeError, tomllib.TOMLDecodeError): - # Malformed or unreadable user file — leave it untouched. + # Malformed or unreadable user file — leave it untouched (existing + # "never clobber the user" contract; see the skipped test). result.skipped.append(rel_dst) return @@ -618,7 +639,10 @@ def _install_toml_merge( if tomllib.loads(text) != dst_data: raise ValueError("serializer round-trip mismatch") except (ValueError, tomllib.TOMLDecodeError): - result.skipped.append(rel_dst) + # The minimal serializer can't faithfully re-emit the merged config + # (e.g. a non-BMP string value, or a nan/inf float). Leave the user's + # file alone, but report it as a failure — not "already present". + result.failed.append(rel_dst) return dst.write_text(text, encoding="utf-8") diff --git a/src/vouch/sessions.py b/src/vouch/sessions.py index d243646b..a0079366 100644 --- a/src/vouch/sessions.py +++ b/src/vouch/sessions.py @@ -149,11 +149,16 @@ def _build_summary_body(sess: Session, ids: list[str]) -> str: # through the review gate. Anything agent-controlled (sess.task, # sess.note, sess.agent) is omitted to keep the review-gate guarantee # intact for the Page artifact kind. See #76. + # crystallize does not end the session, so ended_at is often None here. + # A wall-clock fallback would make every retry write a different body, + # breaking crystallize idempotency (#256) and re-embedding needlessly, so + # emit a stable marker instead of datetime.now() for a still-open session. + ended = sess.ended_at.isoformat() if sess.ended_at else "(session still open)" lines = [ f"# Session {sess.id}", "", f"**Started:** {sess.started_at.isoformat()}", - f"**Ended:** {(sess.ended_at or datetime.now(UTC)).isoformat()}", + f"**Ended:** {ended}", "", "## Crystallized artifacts", "", diff --git a/src/vouch/web/dual_solve_api.py b/src/vouch/web/dual_solve_api.py index c696cb94..e19db26b 100644 --- a/src/vouch/web/dual_solve_api.py +++ b/src/vouch/web/dual_solve_api.py @@ -71,7 +71,13 @@ def _serialize(job: DualSolveJob) -> dict[str, Any]: "log": c.log, "diff": c.diff} for c in job.candidates ], - "recommendation": ds.recommendation(job.candidates), + # No candidates yet (still running) -> no recommendation. Computing it + # over [] returns the "neither engine produced a usable diff" hint, + # which the SPA would otherwise render as a false failure for the whole + # duration of a run that is still in progress. + "recommendation": ( + ds.recommendation(job.candidates) if job.candidates else None + ), "proposed_ids": list(job.proposed_ids), "kept_branch": job.kept_branch, "changed_files": ds.changed_files(kept.diff) if kept is not None else [], diff --git a/tests/test_codex_rollout.py b/tests/test_codex_rollout.py index 353b6046..999ad382 100644 --- a/tests/test_codex_rollout.py +++ b/tests/test_codex_rollout.py @@ -297,3 +297,17 @@ def test_fixtures_use_placeholder_data_only() -> None: text = fixture.read_text(encoding="utf-8") assert "alice-example" in text or "session_meta" not in text assert "/home/a/" not in text + + +def test_parse_rejects_oversized_rollout( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + # The parser streams line-by-line, but a rollout with no interior newlines + # (or a huge one) would still be read whole into memory. Guard on file size + # so an oversized/blob rollout is refused up front, mirroring the byte caps + # the codebase applies to other untrusted reads. + monkeypatch.setattr(cr, "_MAX_ROLLOUT_BYTES", 64, raising=False) + big = tmp_path / "rollout-2026-07-01T09-00-00-x.jsonl" + big.write_text("x" * 512, encoding="utf-8") + with pytest.raises(cr.CodexRolloutError, match="too large"): + cr.parse_rollout(big) diff --git a/tests/test_install_adapter.py b/tests/test_install_adapter.py index e4adad83..c9e35cdd 100644 --- a/tests/test_install_adapter.py +++ b/tests/test_install_adapter.py @@ -867,3 +867,75 @@ def test_installed_wheel_resolves_adapters(tmp_path: Path) -> None: assert "claude-code" in result["hosts"], ( f"installed copy can't resolve adapters: {result}" ) + + +# --- toml_merge failure is not silent success ----------------------------- + + +def test_toml_merge_unserializable_config_is_failed_not_skipped(tmp_path: Path) -> None: + # A user config.toml carrying a non-BMP char in a string value can't + # survive the minimal serializer's json.dumps -> tomllib round-trip, so + # vouch cannot install. That must be reported as a distinct failure, not + # bucketed as "already present" (which would tell the user vouch is wired + # when it is not). + from vouch.install_adapter import _install_toml_merge + + dst = tmp_path / "config.toml" + dst.write_text('model = "gpt-5"\nnote = "hi \U0001f600 there"\n', encoding="utf-8") + src = tmp_path / "src.toml" + src.write_text('[mcp_servers.vouch]\ncommand = "vouch"\n', encoding="utf-8") + + result = InstallResult() + _install_toml_merge(src, dst, result, ".codex/config.toml") + + assert result.failed == [".codex/config.toml"] + assert result.skipped == [] + assert "[mcp_servers.vouch]" not in dst.read_text(encoding="utf-8") + + +def test_install_mcp_reports_failure_and_exits_nonzero(tmp_path: Path) -> None: + # End-to-end: an existing codex config.toml with an emoji makes the T1 + # toml_merge fail. The CLI must NOT print "already present" / a clean + # "Done" for it, and must exit non-zero so scripts notice. + cfg = tmp_path / ".codex" / "config.toml" + cfg.parent.mkdir(parents=True) + cfg.write_text('model = "gpt-5"\nnote = "\U0001f600"\n', encoding="utf-8") + + result = CliRunner().invoke( + cli, ["install-mcp", "codex", "--tier", "T1", "--path", str(tmp_path)] + ) + + assert result.exit_code != 0, result.output + assert "already present" not in result.output + assert "[mcp_servers.vouch]" not in cfg.read_text(encoding="utf-8") + + +# --- manifest dst containment (path traversal) ---------------------------- + + +def test_install_rejects_dst_escaping_target( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + # A manifest whose `dst` escapes the target tree (via `..` or an absolute + # path) must be refused, not written outside `target`. + import vouch.install_adapter as ia + + adapters = tmp_path / "adapters" + (adapters / "evil").mkdir(parents=True) + (adapters / "evil" / "payload.txt").write_text("pwned", encoding="utf-8") + (adapters / "evil" / "install.yaml").write_text( + "host: evil\n" + "pretty: Evil\n" + "fence: { begin: x, end: y }\n" + "tiers:\n" + " T1:\n" + " - { src: payload.txt, dst: ../../escape.txt }\n", + encoding="utf-8", + ) + monkeypatch.setattr(ia, "ADAPTERS_DIR", adapters) + + target = tmp_path / "project" + target.mkdir() + with pytest.raises(AdapterError, match=r"escape|outside|traversal"): + install("evil", target=target, tier="T1") + assert not (tmp_path / "escape.txt").exists() diff --git a/tests/test_sessions.py b/tests/test_sessions.py index e87cadb1..917b5d05 100644 --- a/tests/test_sessions.py +++ b/tests/test_sessions.py @@ -204,3 +204,45 @@ def flaky_approve(*args, **kwargs): summary = store.get_page(second["summary_page_id"]) assert sorted(summary.claims) == sorted([c.id for c in store.list_claims()]) + + +def test_crystallize_open_session_summary_body_is_idempotent( + store: KBStore, monkeypatch: pytest.MonkeyPatch +) -> None: + # crystallize never ends the session, so sess.ended_at stays None. #256 + # made the retry stop crashing, but the summary body still embedded a + # wall-clock 'Ended' stamp, so each rewrite produced a different body (and + # re-embedded). Retrying on an unchanged, still-open session must yield an + # identical page body. + import datetime as _dt + + from vouch import sessions as s + + src = store.put_source(b"e") + sess = sess_mod.session_start(store, agent="a", task="t") + propose_claim( + store, text="finding", evidence=[src.id], proposed_by="a", + session_id=sess.id, + ) + + # a clock that advances on every read, so any wall-clock stamp in the body + # would differ between the two crystallize calls. + ticks = iter([ + _dt.datetime(2026, 1, 1, tzinfo=_dt.UTC), + _dt.datetime(2026, 6, 30, 12, tzinfo=_dt.UTC), + _dt.datetime(2026, 12, 31, tzinfo=_dt.UTC), + ]) + + class _Clock: + @staticmethod + def now(tz: object = None) -> _dt.datetime: + return next(ticks) + + monkeypatch.setattr(s, "datetime", _Clock) + + first = sess_mod.crystallize(store, sess.id, approver="u") + body1 = store.get_page(first["summary_page_id"]).body + second = sess_mod.crystallize(store, sess.id, approver="u") + body2 = store.get_page(second["summary_page_id"]).body + + assert body1 == body2 diff --git a/tests/test_web_dual_solve.py b/tests/test_web_dual_solve.py index 4ea528df..23db9a8f 100644 --- a/tests/test_web_dual_solve.py +++ b/tests/test_web_dual_solve.py @@ -272,3 +272,37 @@ def test_spa_assets_are_served(git_kb): assert c.get(path).status_code == 200, path page = c.get("/dual-solve").text assert "/static/dual_solve.js" in page and "createApp" in page + + +def _job(**kw): + from vouch.web.dual_solve_api import DualSolveJob + + base = dict(id="j", issue_url="o/n#1", claude_effort="high", + codex_effort="high") + base.update(kw) + return DualSolveJob(**base) + + +def test_serialize_omits_recommendation_while_running(): + # While the job is still running there are no candidates yet, so + # recommendation([]) would say "neither engine produced a usable diff". + # That false negative must not be surfaced during a running job. + from vouch.web.dual_solve_api import _serialize + + data = _serialize(_job()) # status defaults to "running", candidates=[] + assert data["status"] == "running" + assert data["recommendation"] is None + + +def test_serialize_includes_recommendation_once_candidates_exist(): + from vouch.dual_solve import Candidate + from vouch.web.dual_solve_api import _serialize + + job = _job(status="ready") + job.candidates = [ + Candidate(engine="claude", branch="ds/claude", worktree=Path("/tmp/x"), + diff="+a\n+b\n", ok=True), + ] + data = _serialize(job) + assert data["recommendation"] is not None + assert data["recommendation"]["engine"] == "claude"