[codex] Upstream hosted review dogfood stack

[romgenie]

Summary

• Upstreams the hosted review hardening stack for Coven Code as one integrated PR.
• Adds hosted review runtime/memory isolation, tenant/install/repo/domain scoping, trust classification, provenance, secret scanning, sync conflict handling, retention/redaction controls, structured review output, read tracing, and substantive-review enforcement.
• Adds follow-up review fixes for supporting-evidence reporting, hosted settings-sync isolation, memory candidate ID validation, and transcript session ID validation.

Issue tags

• Primary hosted-review hardening track: Hosted Review Mode — security & memory hardening track #119
• Hosted-mode foundation: Add a hosted-mode memory isolation model #97, Disable global user memory in hosted review mode #100
• Identity and scoping: Scope hosted memory by GitHub App installation or tenant #98, Scope hosted memory by canonical repo identity instead of local path #99, Derive and verify project_id from canonical GitHub repo identity #104, Split hosted memory by branch and security domain #110
• Trust and poisoning defense: Block untrusted PR and fork input from durable memory writes #101, Add trust classification for memory sources #107, Add an approval gate for auto-extracted memories #108
• Metadata and provenance: Make memory frontmatter scope and trust enforceable #105, Add provenance metadata to every memory entry #106, Require review output to cite memory entries used #112
• Secrets, sync, retention: Enforce secret scanning before memory write or sync #102, Harden team-memory sync auth and keying for tenant and repo isolation #103, Add memory retention, deletion, and redaction controls #109, Replace server-wins team-memory pull with conflict handling #111

Refs #119
Related to #97, #98, #99, #100, #101, #102, #103, #104, #105, #106, #107, #108, #109, #110, #111, #112

Context

• Related issue/user request: Hosted Review Mode — security & memory hardening track #119, including the hosted-mode security and memory hardening sub-track issues Add a hosted-mode memory isolation model #97 through Require review output to cite memory entries used #112.
• Scope: Rust runtime, CLI/headless contract, hosted-review memory/session/sync behavior, CI coverage, and docs for the hosted review contract.
• Non-goals: Merging this PR, enabling Vercel deployment authorization, or changing unrelated provider/runtime behavior.

Changes

• Adds hosted-review mode configuration and stricter defaults for memory, MCP/plugin/tool exposure, and write behavior.
• Keys hosted memory, transcripts, settings sync, and team-memory sync by scoped tenant, installation, repository, and memory domain identity.
• Blocks untrusted hosted memory writes by default, routes unapproved extractions to reviewable candidates, and validates candidate/session identifiers before filesystem path construction.
• Adds memory provenance/trust metadata, public-review filtering, secret scanning, retention/deletion/redaction helpers, and team-memory conflict records.
• Emits structured headless review results with review evidence, supporting-file tracing, findings/tests/no-findings parsing, and substantive-review enforcement.
• Adds Rust CI and interactive terminal suite workflows for PR validation.

Validation

Record exact commands and outcomes. Mark items N/A with a reason when they do not apply.

• git diff --check - passed locally; only Windows line-ending warnings were reported.
• cargo fmt --all - passed locally.
• cargo check --workspace - passed locally.
• cargo clippy --workspace --all-targets -- -D warnings - passed locally.
• cargo test --workspace - covered locally with cargo test --workspace -- --test-threads=1; passed. Serial execution was used because the TUI OSC8 tests are order-sensitive under parallel Windows execution.
• Targeted/manual checks: cargo test -p claurst-query candidate_store_rejects_traversal_candidate_ids passed; cargo test -p claurst-core transcript_paths_reject_traversal_session_ids passed; GitHub Actions Rust CI passed on a94c1b7; GitHub Actions TUI Tests passed on a94c1b7.
• Not run: Vercel preview could not be completed from code because the status requires team authorization in Vercel.

PR Readiness

• Diff is limited to the intended files.
• Generated files were not edited by hand.
• User-facing docs/help were updated or are not needed.
• No secrets, credentials, local paths, or unrelated logs are included.
• Remaining risks or follow-up work are listed above.

Remaining external blocker: the only failing status is Vercel authorization required for team 0xBuns; Rust CI and TUI checks are passing.

[romgenie]

While re-checking the hosted persistence paths, I found one more path-shaped input that should be constrained in this PR. transcript_path_for_mode appends caller-controlled session_id directly as {session_id}.jsonl; generated IDs are UUIDs, but --session-id can provide arbitrary text before it reaches transcript persistence. In hosted-review mode that means a separator-bearing ID could escape the scoped transcript directory. We should validate session IDs as single safe filename components before building either local or hosted transcript paths, and add a regression for traversal-style IDs.

[romgenie]

Follow-up: fixed in a94c1b7 by validating transcript session IDs as safe filename components before local or hosted transcript path construction. Verified locally with:

• cargo test -p claurst-core transcript_paths_reject_traversal_session_ids
• cargo check --workspace
• cargo clippy --workspace --all-targets -- -D warnings
• cargo test --workspace -- --test-threads=1

GitHub Actions Rust CI and TUI Tests are also passing on a94c1b7.

[BunsDev]

The architecture here is sound — tenant/install/repo/domain scoping, candidate gating, conflict-aware sync, and the CI/docs are solid. But two High issues undermine the security model this PR exists to provide, and two more break the locked contract. Requesting changes.

High

1. safe_component passes .. through, defeating the traversal defense for scope components
src-rust/crates/core/src/hosted_review.rs:366 — safe_component keeps ., so "." and ".." pass unchanged. tenant_id, installation_id, and repo_id are external control-plane inputs joined bare into hosted_memory_path(), transcript_dir_for_mode(), and delete_hosted_memory_for_scope() — the last one calls remove_dir_all, so tenant_id = ".." recursively deletes outside the tenant namespace. The mapping is also non-injective: tenants a/b and a_b both sanitize to a_b and collide, which is exactly the cross-tenant leakage this PR targets. This PR's own validate_session_id_component (session_storage.rs:294) rejects ./.. — scope fields need the same check, or hash the raw IDs like local_project_id_from_identity does.

2. Hosted trust gate is self-attested by the file it gates
src-rust/crates/core/src/claudemd.rs — memory_file_allowed_for_options decides admission from the memory file's own frontmatter trust:. In the hosted PR-review threat model, project AGENTS.md comes from the PR checkout and is attacker-writable: a fork PR adds trust: maintainer_approved (or system_policy) and sails through the threshold, injecting instructions into the reviewer's system prompt. Same for visibility, expires_at, deleted_at. Trust must be assigned by the loader from provenance (which ref/directory the file came from); at minimum, cap self-declared trust of review-checkout files at contributor_input/fork_input.

3. Compliant review-only address_review_comment runs are classified as failure (exit 1)
src-rust/crates/cli/src/headless.rs — every AddressReviewComment brief is now review-mode and its prompt says "Do not modify files … unless the review comment explicitly asks for code changes", but is_comment_only() still only covers RespondToMention, and classify(Completed, has_commits=false, comment_only=false) returns (Failure, AmbiguousSpec, 1). A model that follows the new prompt exactly is reported to the adapter as a non-retryable failure. classify should treat review-mode runs as comment-only.

4. v2 contract schemas reject the runtime's own I/O

• session-brief.schema.json only had its const bumped to "2"; it keeps additionalProperties: false and defines neither review_context nor audit_instruction, so a validating adapter can never emit a brief that triggers review mode (verified with ajv against sample_review_brief()).
• result.schema.json's review oneOf requires findings (minItems 1) or a non-null no_findings_reason, but the degraded-review path deliberately emits findings: [] + no_findings_reason: null — asserted by this PR's own tests (review_context_produces_structured_pr_review_evidence, generic_review_output_is_marked_partial). An envelope with both findings and a reason also fails oneOf since both branches match. Verified with ajv.

Medium

5. Review-only enforcement is split across two signals
The 40-turn cap keys off the brief's review_context, but the write/exec tool filter and MCP/plugin lockdown key only off --hosted-review / COVEN_CODE_HOSTED_REVIEW (main.rs:1528-1580, filter_tools_for_hosted_review). The documented contract invocation has no flag, so a brief-driven review session runs with the full toolset and PermissionMode::BypassPermissions — the only "no writes" enforcement is a prompt sentence. Brief review-mode should imply at least the read-only tool filter.

6. Supporting-evidence trace counts failed reads and grep match text
headless.rs — Read paths are recorded at ToolStart and never revisited on failure; normalize_path only rejects paths that exist and are not files, so a Read of a nonexistent path counts as evidence and can flip evidence_status to Complete without any file being read. record_tool_end also mints supporting_files from any grep output line whose prefix contains a dot before a colon, including matched source text. Record Reads at ToolEnd on success (or require is_file()), and restrict grep parsing to the filename-prefix format.

7. Legitimate short no-findings justifications rejected as generic
parse_no_findings_reason drops any justification under 120 chars containing "no findings"/"no issues found" — phrases the prompt itself elicits. A concise file-backed justification gets nulled, has_substantive_review flips false, and a compliant clean review is downgraded to Partial. The file-reference requirement already guards substance; drop the generic-needle check for this section.

8. Memory body unescaped inside <memory> wrapper — trust-label forgery
claudemd.rs format_memory_file_for_prompt escapes attributes but injects the body verbatim, so a body containing </memory><memory trust="system-policy" …> forges a trusted block. The hosted-vs-local wrapper choice in build_system_prompt is also a contains("<memory id=") substring probe; derive it from MemoryLoadOptions.mode instead.

Coordination

The headless contract is bumped v1 -> v2 and declared locked/normative in this repo, so coven-github needs matching v2 support before merge — and fixing item 4 will touch the schemas again, so worth landing those together.

CI workflow itself looks good: --locked builds, minimal permissions, cache, 45-minute timeout.

[romgenie]

BunsDev requested changes have been addressed in 8b62b4a on this PR, with the matching v2 contract alignment pushed to OpenCoven/coven-github PR #31.

Resolved items:

• Hardened hosted scope components with collision-proof encoding for ., .., separators, and non-injective inputs.
• Capped hosted project/local memory trust by loader provenance so PR-controlled memory cannot self-attest elevated trust.
• Treated review-mode AddressReviewComment no-commit completions as successful review output.
• Updated v2 session/result schemas and docs for review_context, audit_instruction, degraded/partial review output, and no oneOf ambiguity.
• Made brief-driven review mode enable hosted read-only restrictions without requiring --hosted-review.
• Changed evidence tracing to count only successful reads/search results against existing files, not failed reads or arbitrary grep match text.
• Allowed concise file-backed no-findings justifications.
• Escaped hosted memory bodies and derived hosted memory wrapping from MemoryLoadOptions.mode.

Verification on PR #132 head 8b62b4a:

• cargo fmt --all
• cargo check --workspace
• cargo clippy --workspace --all-targets -- -D warnings
• cargo test --workspace
• GitHub Rust CI: passing
• GitHub TUI Tests: passing

Remaining non-code status: Vercel still requires external authorization. Ready for re-review.

[BunsDev]

Verified all 8 items from my previous review at 8b62b4a, plus a full local run of cargo fmt --all -- --check, cargo check --workspace, cargo clippy --workspace --all-targets -- -D warnings, and cargo test --workspace — all pass.

High

1. Fixed. safe_component now injectively encodes non-[A-Za-z0-9_-] bytes (. -> ~2E, / -> ~2F, empty -> ~) at hosted_review.rs:374-391; .., ., empty, and the a/b vs a_b collision are all covered by tests, and every hosted path join (memdir, transcripts, delete) goes through it.
2. Fixed. Hosted mode caps self-declared trust of project/local/user files at ContributorInput (claudemd.rs:387-399), below the MaintainerApproved threshold; frontmatter can now only further exclude, never elevate. Tests cover the fork-PR self-attestation case.
3. Fixed. AddressReviewComment maps to ReviewMode::ReviewComment and is_comment_only covers all review modes; classify(Completed, false, true) returns exit 0 with a regression test.
4. Fixed. session-brief.schema.json defines review_context/audit_instruction matching the Rust structs; result.schema.json dropped the broken oneOf. Re-validated the checked-in examples and the degraded findings: [] + no_findings_reason: null case with ajv (2020 spec) — all pass.

Medium

5–8 fixed as described in the PR comments. One residual on item 6: single-file grep output has no filename prefix, so matched source text shaped like foo.rs: ... can still mint a supporting-file entry — but normalize_path now requires an existing regular file inside the workspace, so the worst case is over-attributing a real file, not fabricating evidence. Fine as a follow-up.

Coordination

coven-github #31 (v2 contract) is merged, so the cross-repo blocker is cleared.

Issue closure scope

Cross-checked #97–#112 against the diff. This PR fully closes #97, #100, #101, #102, #105, #108. The rest land substantial groundwork but each has a remaining gap (mostly missing acceptance-criteria tests or unwired pieces, e.g. #112's memory-ref validator not called in the live /review path); I'll leave those open with scoping comments rather than close them on this merge. #119 stays open as the tracker.

Approving. Will squash-merge with the closes list above.