fix(csa-session): recon sessions write to session-local output path (#910)

[RyderFreeman4Logos]

Summary

• Canonicalize recon pattern/skill prompts to use \$CSA_SESSION_DIR/output/...
• Add post-session audit that detects repo-tracked file mutations (warn only, no auto-revert)
• Rule 027: PATTERN.md + workflow.toml updated in lockstep (patterns/mktd)

Scope

13 files, +313 / -50 LOC. New modules: pipeline_post_exec_audit.rs (129 LOC), manager_audit.rs (75 LOC).

Test plan

• cargo test --workspace green
• weave compile green
• just pre-commit green

Closes #910.

[gemini-code-assist]

Code Review

This pull request implements a repository write audit mechanism to detect unintended modifications to tracked files during read-only or reconnaissance sessions. Key changes include a new auditing module in cli-sub-agent, logic to identify session types based on prompt keywords, and updates to the session manager to record audit warnings as artifacts. The review feedback suggests refactoring unstable Rust let_chains for better compatibility, using static constants for string marker arrays to optimize performance, and addressing potential test flakiness caused by thread::sleep when asserting on file timestamps.

[gemini-code-assist]

The if let ... && let ... syntax is part of the let_chains feature, which is currently unstable in Rust. While it might compile with a nightly toolchain, it's generally recommended to stick to stable features for better compatibility and readability.

Consider refactoring this to nested if let statements, which is idiomatic in stable Rust.

Suggested change

	if let Some(artifact_path) =
	csa_session::write_audit_warning_artifact(&ctx.session_dir, &mutated_paths)?
	&& let Ok(rel_path) = artifact_path.strip_prefix(&ctx.session_dir)
	{
	session_result.artifacts.push(SessionArtifact::new(
	rel_path.to_string_lossy().into_owned(),
	));
	}
	if let Some(artifact_path) =
	csa_session::write_audit_warning_artifact(&ctx.session_dir, &mutated_paths)?
	{
	if let Ok(rel_path) = artifact_path.strip_prefix(&ctx.session_dir) {
	session_result.artifacts.push(SessionArtifact::new(
	rel_path.to_string_lossy().into_owned(),
	));
	}
	}

[gemini-code-assist]

For performance and clarity, this array of string literals can be defined as a static constant. This avoids reallocating it on the stack every time this function is called.

Suggested change

	let explicit_readonly = [
	"read-only",
	"readonly",
	"do not edit",
	"don't edit",
	"must not edit",
	"without editing",
	"do not modify",
	"don't modify",
	];
	if explicit_readonly
	.iter()
	.any(|marker| prompt_lower.contains(marker))
	{
	static EXPLICIT_READONLY: &[&str] = &[
	"read-only",
	"readonly",
	"do not edit",
	"don't edit",
	"must not edit",
	"without editing",
	"do not modify",
	"don't modify",
	];
	if EXPLICIT_READONLY
	.iter()
	.any(|marker| prompt_lower.contains(marker))
	{

[gemini-code-assist]

Similarly, these marker arrays can also be static constants to avoid reallocating them on each function call.

    static RECON_MARKERS: &[&str] = &[
        "recon",
        "reconnaissance",
        "analyze",
        "analyse",
        "analysis",
        "summarize",
        "summary",
        "inspect",
        "investigate",
    ];
    static WRITE_MARKERS: &[&str] = &[
        "implement",
        "edit",
        "fix",
        "modify",
        "update",
        "patch",
        "write code",
        "create file",
        "commit",
        "merge",
        "refactor",
    ];
    RECON_MARKERS
        .iter()
        .any(|marker| prompt_lower.contains(marker))
        && !WRITE_MARKERS
            .iter()
            .any(|marker| prompt_lower.contains(marker))

[gemini-code-assist]

Using thread::sleep in tests that rely on file modification timestamps can lead to flakiness, especially with a short duration like 20ms. The test might fail on a slow or heavily loaded system if the file write on the next line happens within the same timestamp granularity as session_start.

A more robust approach would be to manually set the file's modification time to be explicitly after session_start. You could use a helper function to touch the file with a future timestamp. For example:

// In a test helper module
use std::time::{Duration, SystemTime};
pub fn touch_file(path: &std::path::Path, offset_secs: u64) {
    let new_time = SystemTime::now() + Duration::from_secs(offset_secs);
    filetime::set_file_mtime(path, filetime::FileTime::from_system_time(new_time)).unwrap();
}

This would require adding the filetime crate as a dev-dependency. If you prefer to avoid new dependencies, consider increasing the sleep duration to something safer, like 1 second, though it will make the test slower.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ec422fca5e

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

Keep repo-write audit best-effort

The new repo-write audit is wired as a hard failure path: process_execution_result propagates any error from maybe_record_repo_write_audit with ?, and pipeline_session_exec turns post-exec errors into terminal session failures. That means an observational audit problem (for example git invocation failure, non-UTF8 path output, or inability to write audit-warnings.md) can mark an otherwise successful run as failed, which contradicts the warning-only behavior described for this feature.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Resolve mutated paths from repository root

audit_repo_tracked_writes treats each git diff --name-only entry as relative to project_root, but Git emits repository-root-relative paths (e.g. from repo/sub, output is sub/file.rs). When project_root is a nested directory, project_root.join(rel_path) points to a non-existent path like repo/sub/sub/file.rs, so metadata lookup fails and real tracked mutations are silently skipped.

Useful? React with 👍 / 👎.

[RyderFreeman4Logos]

Merge audit trail — 9 rounds iterative fixes + design-residual circuit-break

This PR went through 9 rounds of review with codex. Each round surfaced a NEW concrete correctness bug (per CLAUDE.md lesson #821 rule of legitimate iterative quality work):

1. Round 1: output path canonicalization → output/ instead of repo-root
2. Round 2: audit gating extended beyond task_type="run" to cover plan/plan-step
3. Round 3: git diff --name-only --diff-filter=M HEAD replaced with pre/post snapshot covering A/M/D/R + commits
4. Round 4: pre-session porcelain baseline subtracted to avoid false-positive pre-existing-dirty attribution
5. Round 5: stale sidecar deletion on empty manager_fields save
6. Round 6: overcorrection of round-5 reverted via SaveOptions::clear_stale_manager_sidecar opt-in
7. Round 7: atomic sidecar→envelope write ordering + best-effort audit (swallow errors at 3 boundaries)
8. Round 8: symmetric atomic envelope→sidecar-unlink clear ordering
9. Round 9: per-execution audit baseline (refresh at turn start, not session creation)

Round-10 review findings (circuit-break rationale)

Session 01KPNHKCR6C09Y2BAW34S7S9SM returned:

• HIGH (manager_result.rs:89): preserve-by-default leaks stale sidecar across turns via load_result_in overlay.
• MEDIUM (manager_result.rs:127): clear_manager_sidecar failure is silently swallowed.

The HIGH is a same-point design flip with round-5's original finding (preserve vs overwrite sidecar semantics). Per CLAUDE.md lesson #821, same-point contradictions trigger circuit-break rather than further rounds.

Both are filed as followup issues for design decisions:

• csa-session: load_result_in unconditionally overlays sidecar regardless of envelope artifacts #956 — load_result_in sidecar overlay reconciliation (HIGH)
• csa-session: save_result_in reports Ok even when clear_manager_sidecar unlink fails #957 — silent unlink swallow (MEDIUM, blocks on csa-session: load_result_in unconditionally overlays sidecar regardless of envelope artifacts #956)

Merging at round 9 with atomicity + per-execution baseline fixes intact. The load-side reconciliation is a separate design decision requiring user input on preserve-vs-overwrite semantics.

Closes #910.