diff --git a/.coderabbit.yaml b/.coderabbit.yaml new file mode 100644 index 0000000..fdb9635 --- /dev/null +++ b/.coderabbit.yaml @@ -0,0 +1,119 @@ +# yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json +early_access: true +language: en-US +reviews: + request_changes_workflow: true + high_level_summary: false + high_level_summary_in_walkthrough: true + fail_commit_status: true + auto_apply_labels: true + auto_assign_reviewers: true + poem: false + slop_detection: + label: slop + auto_review: + enabled: true + pre_merge_checks: + docstrings: + mode: error + title: + mode: error + requirements: >- + Must follow conventional commit. The type should match the intent, + the description and optional scope should be concise, lowercase and + in the imperative mood. Ideally total length should be within 72 + characters. Breaking changes should be marked with a ! (bang) after + the type + description: + mode: error + issue_assessment: + mode: error + custom_checks: + - name: CLAUDE.md stays in sync + mode: error + instructions: >- + When source files under hooks/ or memory/ change (Cargo.toml, + src/bin/*, src/lib.rs, src/*.rs), verify CLAUDE.md is updated if + any of these are now stale: + Binary count matches [[bin]] entries across all Cargo.toml files. + Architecture tree lists exactly the files that exist under hooks/src/. + Dependency count matches [dependencies] entries in hooks/Cargo.toml. + All dependency crate names appear as backtick-quoted words in the + Dependencies section. + Test count in the Testing section reflects actual test suite sizes. + Flag as "must fix" if any claim is provably wrong. Ignore if the + PR doesn't touch structural files. + + - name: Panic = deny in all binaries + mode: error + instructions: >- + Every file under hooks/src/bin/*.rs must wrap its main logic in + std::panic::catch_unwind and deny (exit non-zero or return a deny + JSON response) on panic. The sole exception is persona_inject.rs, + which may fail open (persona injection is not a security gate). If + a new binary is added or an existing one is modified, verify + catch_unwind is present and the panic path produces a deny outcome. + Flag if the panic path silently allows the operation. This is a + security invariant: hooks must never fail open unless explicitly + exempted. + + - name: "H-4 purity: permissions never writes" + mode: error + instructions: >- + hooks/src/bin/permissions.rs (the PreToolUse hook) must NEVER + perform file I/O writes. It must use resolve_readonly() from + session.rs, never resolve_readwrite(). Check that no + std::fs::write, std::fs::File::create, std::fs::OpenOptions with + write/append, or any other write syscall is introduced. If the PR + adds write operations to permissions.rs, flag as "must fix". This + is a structural security boundary. + + - name: Architecture boundaries enforced + mode: error + instructions: >- + Read docs/architecture.md from the repository root. Verify these + constraints against the PR diff: + Layer direction: infrastructure modules (config.rs, output.rs, + changelog.rs, log.rs, mcp.rs) must NOT import from core modules + (sandbox, gitcheck, session, worktree). Check use/mod statements. + Crate independence: memory/ must not depend on hooks/ and vice + versa. Check Cargo.toml changes. + Forbidden deps: no async runtimes (tokio, async-std, smol), no + network clients (reqwest, hyper, ureq), no proc-macro crates + beyond serde_derive. Check Cargo.toml changes. + Denial message format: all Block() and Deny() messages in + gitcheck.rs, sandbox.rs, and lib.rs must contain WHAT:, FIX:, and + REF: tokens. Exception: the REDIRECT message for config paths in + sandbox.rs (containing "config path must persist") is exempt. + Module placement: new source files must be placed in the correct + layer (infra vs core vs binary) per the architecture doc. + Only flag issues provably violated by the PR diff. + + - name: Harness engineering principles + mode: error + instructions: >- + Read .agents/research/harness.md and GOALS.md from the repository + root for context. Evaluate the PR against these principles: + Error messages are instructions: any new error, log, or denial + message visible to the agent should be actionable with WHAT went + wrong and HOW to fix it. Cryptic messages like "operation failed" + without remediation are a flag. (harness Pillar 4a) + No silent pass-through: if a check, gate, or hook encounters an + error, it must fail closed (deny/block), not silently allow. This + applies to catch_unwind paths, file I/O errors, and regex + compilation. (harness Pillar 3d) + Change risk: flag if the PR modifies security-critical files + (sandbox.rs, gitcheck.rs, session.rs, worktree/) without + proportional test coverage. These are high-risk paths. (harness + Pillar 3a, GOALS directive 23) + Test quality: the PR should not remove tests without replacing + them. Test code must use fictional repo names (acme-api, web-app), + never real company names. Property-based tests should cover + sandbox and gitcheck changes. (GOALS directives 4, 10) + Only flag concrete violations visible in the diff. + +issue_enrichment: + auto_enrich: + enabled: true + labeling: + auto_apply_labels: true diff --git a/CLAUDE.md b/CLAUDE.md index 9760beb..9b806bb 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -92,7 +92,7 @@ After pushing, poll PR checks and review comments in a single loop for up to 10 ## Key Design Decisions - **Three-layer sandbox**: Session resolution -> context-aware path checking -> git safety regex -- **H-4 purity**: PreToolUse hook (`permissions`) NEVER writes files. Uses `resolve_readonly()` +- **H-4 purity**: PreToolUse hook (`permissions`) NEVER writes files. Uses `resolve_readonly()`. - **Lazy worktrees**: `WORKTREE_MISSING:` denials trigger `ensure-worktree` on-demand - **Config persistence**: `.agents/`, `.claude/` redirect to main checkout when gitignored; if tracked by git (dir exists in worktree), allowed in-place - **Committed repo files**: `CLAUDE.md`, `AGENTS.md` are version-controlled — allowed in worktrees @@ -177,7 +177,7 @@ All shell scripts follow the [Google Shell Style Guide](https://google.github.io ## Testing -219 tests (166 unit + 5 doc + 13 integration + 10 proptest + 25 memory) plus 4 fuzz targets. +246 tests (188 hooks unit + 5 claude_md + 17 integration + 10 proptest + 22 memory unit + 4 memory integration) plus 4 fuzz targets. Run with `make test` or `cargo test`. Test patterns: @@ -230,5 +230,5 @@ Every workflow change must pass `actionlint` + `zizmor --pedantic` in CI. ## Dependencies -5 crates: `serde`, `serde_json`, `regex`, `flate2`, `libc`. No async runtime, -no network deps, no proc macros. +5 crates: `serde`, `serde_json`, `regex`, `flate2`, `libc`. Memory crate adds +`rusqlite`. No async runtime, no network deps, no proc macros. diff --git a/greptile.json b/greptile.json new file mode 100644 index 0000000..70c1e5b --- /dev/null +++ b/greptile.json @@ -0,0 +1,108 @@ +{ + "labels": [], + "comment": "", + "enabled": true, + "fixWithAI": true, + "hideFooter": false, + "strictness": 2, + "statusCheck": true, + "commentTypes": [ + "logic", + "syntax", + "style" + ], + "instructions": "", + "disabledLabels": [], + "excludeAuthors": [ + "dependabot[bot]", + "renovate[bot]" + ], + "ignoreKeywords": "", + "ignorePatterns": "greptile.json\n", + "includeAuthors": [], + "summarySection": { + "included": true, + "collapsible": true, + "defaultOpen": false + }, + "excludeBranches": [], + "fileChangeLimit": 100, + "includeBranches": [], + "includeKeywords": "", + "triggerOnDrafts": false, + "triggerOnUpdates": false, + "updateSummaryOnly": false, + "issuesTableSection": { + "included": true, + "collapsible": true, + "defaultOpen": false + }, + "statusCommentsEnabled": true, + "confidenceScoreSection": { + "included": true, + "collapsible": false, + "defaultOpen": false + }, + "sequenceDiagramSection": { + "included": true, + "collapsible": true, + "defaultOpen": false + }, + "shouldUpdateDescription": false, + "commentsOutsideDiffSection": { + "included": true, + "collapsible": true, + "defaultOpen": true + }, + "customContext": { + "other": [ + { + "scope": [], + "content": "This is a Rust workspace providing session isolation hooks and persistent memory for AI coding agents. Read GOALS.md for fitness directives and gates. Read docs/architecture.md for layer diagram, forbidden dependencies, and key invariants. Read CLAUDE.md for build commands, commit convention, and design decisions." + } + ], + "rules": [ + { + "scope": [], + "rule": "Every hooks/src/bin/*.rs must use catch_unwind and deny on panic (fail-closed). Exception: persona_inject.rs may fail open. Flag any binary missing catch_unwind or silently passing on panic." + }, + { + "scope": [], + "rule": "hooks/src/bin/permissions.rs (PreToolUse hook) must NEVER write files. It must use resolve_readonly(), never resolve_readwrite(). No std::fs::write, File::create, or OpenOptions with write. Flag any write I/O added to permissions.rs." + }, + { + "scope": [], + "rule": "All Block() and Deny() denial messages in gitcheck.rs, sandbox.rs, and lib.rs must contain WHAT:, FIX:, and REF: tokens. Exception: the REDIRECT message for config paths in sandbox.rs (containing 'config path must persist') is exempt. Flag new denial messages missing any of the three fields." + }, + { + "scope": [], + "rule": "Infrastructure modules (config.rs, output.rs, changelog.rs, log.rs, mcp.rs) must NOT import from core modules (sandbox, gitcheck, session, worktree). memory/ must not depend on hooks/. No async runtimes, network clients, or proc-macro crates beyond serde_derive in Cargo.toml." + }, + { + "scope": [], + "rule": "When hooks/src/ structure, Cargo.toml binaries, or dependencies change, CLAUDE.md must be updated to match: binary count, architecture tree, dependency count and names, test count. PR title must follow conventional commits: type(scope): description." + }, + { + "scope": [], + "rule": "Changes to security-critical files (sandbox.rs, gitcheck.rs, session.rs, worktree/) are high-risk and need proportional test coverage. No net test removal. Test code must use fictional repo names (acme-api, web-app), never real company names. No lint suppressions (#[allow(...)], // nolint) without justification in the PR description." + } + ], + "files": [ + { + "scope": [], + "path": "GOALS.md", + "description": "Fitness goals, directives (with steer values), and gates (with check commands and weights). Higher-weight gates are more critical. Directives steered 'increase' must not regress." + }, + { + "scope": [], + "path": "docs/architecture.md", + "description": "Layer diagram (binaries → core → infra), module map, forbidden dependency directions, cross-cutting concerns, and key invariants (panic=deny, H-4 purity, lazy worktrees)." + }, + { + "scope": [], + "path": "CLAUDE.md", + "description": "Build/test/lint commands, commit convention, design decisions, testing patterns, lint suppression policy, shell style guide. Must stay in sync with codebase (enforced by tests/claude_md.rs)." + } + ] + } +} diff --git a/hooks/src/gitcheck.rs b/hooks/src/gitcheck.rs index 4de1cde..531215a 100644 --- a/hooks/src/gitcheck.rs +++ b/hooks/src/gitcheck.rs @@ -72,6 +72,9 @@ static RE_GIT_C_PATH: LazyLock = LazyLock::new(|| Regex::new(r#"\bgit\s+-C\s+("[^"]+"|'[^']+'|(\S+))"#).unwrap()); /// Run all 8 git safety checks against a Bash command. +/// +/// Denial messages use the WHAT/FIX/REF remediation format so the agent +/// can self-repair without human intervention. pub fn check_git_safety(cmd: &str) -> GitResult { // FR-GS-1: Force push without --force-with-lease if RE_GIT_PUSH.is_match(cmd) @@ -79,14 +82,19 @@ pub fn check_git_safety(cmd: &str) -> GitResult { && !RE_FORCE_WITH_LEASE.is_match(cmd) { return GitResult::Block( - "BLOCKED: Force push without --force-with-lease. Use: git push --force-with-lease origin ".into(), + "WHAT: Force push without --force-with-lease. \ + FIX: Use `git push --force-with-lease origin ` instead. \ + REF: CLAUDE.md#supply-chain-policy" + .into(), ); } // FR-GS-2: Push to main/master if RE_PUSH_TO_MAIN.is_match(cmd) { return GitResult::Block( - "BLOCKED: Direct push to main/master. Create a feature branch and open a PR instead." + "WHAT: Direct push to main/master. \ + FIX: Create a feature branch and open a PR instead. \ + REF: CLAUDE.md#commit-convention" .into(), ); } @@ -94,25 +102,37 @@ pub fn check_git_safety(cmd: &str) -> GitResult { // FR-GS-3: Refspec push to main/master if RE_REFSPEC_MAIN.is_match(cmd) { return GitResult::Block( - "BLOCKED: Push to main/master via refspec. Create a feature branch and open a PR instead." + "WHAT: Push to main/master via refspec. \ + FIX: Create a feature branch and open a PR instead. \ + REF: CLAUDE.md#commit-convention" .into(), ); } // FR-GS-4: Delete main/master if RE_DELETE_MAIN.is_match(cmd) { - return GitResult::Block("BLOCKED: Deleting main/master branch is not allowed.".into()); + return GitResult::Block( + "WHAT: Deleting main/master branch is not allowed. \ + FIX: Do not delete protected branches. \ + REF: CLAUDE.md#supply-chain-policy" + .into(), + ); } if RE_DELETE_REFSPEC.is_match(cmd) { return GitResult::Block( - "BLOCKED: Deleting main/master branch via empty refspec is not allowed.".into(), + "WHAT: Deleting main/master branch via empty refspec is not allowed. \ + FIX: Do not delete protected branches. \ + REF: CLAUDE.md#supply-chain-policy" + .into(), ); } // FR-GS-5: --no-verify if RE_NO_VERIFY.is_match(cmd) { return GitResult::Block( - "BLOCKED: git push --no-verify bypasses pre-push hooks. Fix the hook failures instead." + "WHAT: git push --no-verify bypasses pre-push hooks. \ + FIX: Fix the hook failures instead of skipping them. \ + REF: CLAUDE.md#lint-suppression-policy" .into(), ); } @@ -120,26 +140,38 @@ pub fn check_git_safety(cmd: &str) -> GitResult { // FR-GS-6: --follow-tags if RE_FOLLOW_TAGS.is_match(cmd) { return GitResult::Block( - "BLOCKED: git push --follow-tags pushes ALL matching local tags. Push tags explicitly: git push origin ".into(), + "WHAT: git push --follow-tags pushes ALL matching local tags. \ + FIX: Push tags explicitly: `git push origin `. \ + REF: CLAUDE.md#releases" + .into(), ); } // FR-GS-7: Delete semver tags (local and remote) if RE_DELETE_SEMVER_TAG.is_match(cmd) { return GitResult::Block( - "BLOCKED: Deleting semantic version tags is not allowed. Release a new patch version instead.".into(), + "WHAT: Deleting semantic version tags is not allowed. \ + FIX: Release a new patch version instead. \ + REF: CLAUDE.md#releases" + .into(), ); } if RE_DELETE_REMOTE_TAG.is_match(cmd) { return GitResult::Block( - "BLOCKED: Deleting remote semantic version tags is not allowed. Release a new patch version instead.".into(), + "WHAT: Deleting remote semantic version tags is not allowed. \ + FIX: Release a new patch version instead. \ + REF: CLAUDE.md#releases" + .into(), ); } // FR-GS-8: Hard reset to origin/main|master if RE_HARD_RESET.is_match(cmd) { return GitResult::Block( - "BLOCKED: git reset --hard origin/main|master discards all local work. Use: git stash or git reset --soft".into(), + "WHAT: git reset --hard origin/main|master discards all local work. \ + FIX: Use `git stash` or `git reset --soft` instead. \ + REF: CLAUDE.md#key-design-decisions" + .into(), ); } @@ -204,9 +236,9 @@ pub fn check_worktree_enforcement( return Some(crate::worktree_missing_msg(&repo)); } return Some(format!( - "BLOCKED: Git op on main checkout ({repo}). \ - Use worktree: {ws_str}/{repo}/.worktrees/{short_id}. \ - Tip: run git -C fetch origin before creating new branches" + "WHAT: Git operation targets main checkout ({repo}), not the session worktree. \ + FIX: Use `git -C {ws_str}/{repo}/.worktrees/{short_id}/` instead. \ + REF: docs/architecture.md#key-invariants" )); } } @@ -228,9 +260,9 @@ pub fn check_worktree_enforcement( return Some(crate::worktree_missing_msg(&repo)); } return Some(format!( - "BLOCKED: Git op on main checkout ({repo}). \ - Use worktree: {ws_str}/{repo}/.worktrees/{short_id}. \ - Tip: run git -C fetch origin before creating new branches" + "WHAT: Git operation targets main checkout ({repo}), not the session worktree. \ + FIX: Use `git -C {ws_str}/{repo}/.worktrees/{short_id}/` instead. \ + REF: docs/architecture.md#key-invariants" )); } } @@ -241,8 +273,9 @@ pub fn check_worktree_enforcement( if !RE_GIT_C.is_match(cmd) && !RE_CD_PATH.is_match(cmd) && RE_GIT_CHECKOUT_SWITCH.is_match(cmd) { return Some(format!( - "BLOCKED: Bare git checkout/switch — worktrees are active. Use: git -C /.worktrees/{}/ checkout ...", - short_id + "WHAT: Bare git checkout/switch — worktrees are active. \ + FIX: Use `git -C /.worktrees/{short_id}/ checkout ...` instead. \ + REF: docs/architecture.md#key-invariants" )); } diff --git a/hooks/src/lib.rs b/hooks/src/lib.rs index 8bc6a04..ca5d4bb 100644 --- a/hooks/src/lib.rs +++ b/hooks/src/lib.rs @@ -21,11 +21,15 @@ pub mod session; pub mod worktree; /// Format a WORKTREE_MISSING denial message for lazy worktree creation. +/// +/// Uses the WHAT/FIX/REF remediation format so the agent can self-repair. pub fn worktree_missing_msg(repo: &str) -> String { let bin = config::bin_dir().join("ensure-worktree"); format!( - "WORKTREE_MISSING:{repo} \ - — Run: {} {repo}", + "WORKTREE_MISSING:{repo} — \ + WHAT: No worktree exists for repo '{repo}' in this session. \ + FIX: Run `{} {repo}` to create one, then retry the write. \ + REF: docs/architecture.md#key-invariants", bin.display() ) } diff --git a/hooks/src/sandbox.rs b/hooks/src/sandbox.rs index bcac28a..0a85361 100644 --- a/hooks/src/sandbox.rs +++ b/hooks/src/sandbox.rs @@ -63,8 +63,9 @@ pub fn check_path_with_context( // FR-PS-1: System paths — check BEFORE resolving symlinks if is_system_path(&path_str) { return PathDecision::Deny(format!( - "BLOCKED: Cannot write to system path: {}", - raw_path + "WHAT: Write to system path {raw_path} is not allowed. \ + FIX: Write to a project or temp directory instead. \ + REF: docs/architecture.md#forbidden-dependencies" )); } @@ -74,8 +75,9 @@ pub fn check_path_with_context( // System paths check on resolved path (catches /private/etc, /private/var on macOS) if is_system_path(&resolved) || is_private_system_path(&resolved) { return PathDecision::Deny(format!( - "BLOCKED: Cannot write to system path: {}", - raw_path + "WHAT: Write to system path {raw_path} is not allowed (resolves to {resolved}). \ + FIX: Write to a project or temp directory instead. \ + REF: docs/architecture.md#forbidden-dependencies" )); } @@ -163,9 +165,12 @@ pub fn check_path_with_context( return PathDecision::Deny(crate::worktree_missing_msg(&repo)); } let rel = extract_rel_path(&resolved, &repo, ws_str); + let wt_path = + format!("{}/{}/.worktrees/{}/{}", ws_str, repo, sess.short_id, rel); return PathDecision::Deny(format!( - "REDIRECT: Use worktree path instead: {}/{}/.worktrees/{}/{}", - ws_str, repo, sess.short_id, rel + "WHAT: Write targets main checkout, not the session worktree. \ + FIX: Use the worktree path instead: {wt_path}. \ + REF: docs/architecture.md#key-invariants" )); } } else { @@ -190,7 +195,10 @@ pub fn check_path_with_context( return PathDecision::Deny(crate::worktree_missing_msg(&repo)); } return PathDecision::Deny( - "BLOCKED: No worktree for this session. All repo writes are blocked to prevent editing the main checkout.".into(), + "WHAT: No worktree exists and repo name could not be extracted. \ + FIX: Ensure the write path is inside a known repo directory. \ + REF: docs/architecture.md#key-invariants" + .into(), ); }