docs: clarify pre-review validation guidance

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

.github/pull_request_template.md

@@ -6,7 +6,7 @@
 
 ## Change Type
 
-<!-- Check one -->
+<!-- Check all that apply. Refactor-only PRs are for core team or maintainer-requested work. -->
 
 - [ ] Bug fix
 - [ ] New feature
@@ -18,16 +18,19 @@
 
 ## Linked Issue
 
-<!-- Closes #N, or "None" -->
+<!-- Closes #N, Fixes #N, Related #N, or "None". New feature PRs must link an approved issue. -->
 
 ## Validation
 
 <!-- How did you verify this works? -->
 
-- [ ] `cargo fmt`
-- [ ] `cargo clippy --all --benches --tests --examples --all-features`
+- [ ] `cargo fmt --all -- --check`
+- [ ] `cargo clippy --all --benches --tests --examples --all-features -- -D warnings`
+- [ ] `cargo build`
 - [ ] Relevant tests pass: <!-- list specific tests -->
+- [ ] `cargo test --features integration` if database-backed or integration behavior changed
 - [ ] Manual testing: <!-- describe what you tested -->
+- [ ] If a coding agent was used and supports it, `review-pr` or `pr-shepherd --fix` was run before requesting review
 
 ## Security Impact
 
@@ -45,6 +48,10 @@
 
 <!-- How to revert if this causes problems? For Track C changes, this is mandatory. -->
 
+## Review Follow-Through
+
+<!-- Review conversations are author-owned. Summarize any known follow-up or areas where reviewer judgment is still needed. -->
+
 ---
 
-**Review track**: <!-- A (docs/tests/chore) | B (feature/refactor) | C (security/runtime/DB/CI) -->
+**Review track**: <!-- A (docs/tests/chore) | B (feature/maintainer-requested refactor) | C (security/runtime/DB/CI) -->

CONTRIBUTING.md

@@ -10,6 +10,42 @@ cd ironclaw
 
 This installs the Rust toolchain, WASM targets, git hooks, and runs initial checks.
 
+## How to Contribute
+
+- Bug fixes, docs improvements, and focused cleanup tied to a concrete problem are welcome.
+- Search existing issues and PRs before opening a new one to avoid duplicates.
+- Keep changes scoped. One bug, one feature, or one documentation improvement per PR.
+
+### Creating Issues
+
+Open an issue when you are reporting a bug, proposing a feature, or documenting a gap in behavior.
+
+For bug reports, include:
+
+- What you expected to happen
+- What actually happened
+- Clear reproduction steps
+- Relevant logs, screenshots, or error output
+- Environment details when they matter (OS, database backend, feature flags, commit/branch)
+
+For feature requests:
+
+- Open an issue first before writing code
+- Explain the problem being solved, not just the implementation idea
+- Wait for maintainer feedback before investing in a large PR
+
+We require an issue for new features so maintainers can prioritize the work and confirm it fits the roadmap before anyone spends time implementing it.
+
+### Fixing Bugs
+
+- Small, targeted bug-fix PRs are welcome
+- If there is already an issue, link it in your PR
+- If the bug is non-trivial, security-sensitive, or changes behavior across subsystems, open or confirm an issue first so the approach can be aligned before implementation
+
+### Refactor-Only PRs
+
+Refactor-only PRs are not accepted from contributors outside the core team. If a refactor is necessary to land a bug fix or approved feature, keep it minimal and clearly tied to that change.
+
 ## Development Workflow
 
 ```bash
@@ -19,6 +55,45 @@ cargo test                                                   # unit tests
 cargo test --features integration                            # + PostgreSQL tests
 ```
 
+These commands are for day-to-day iteration while you are developing locally. The pre-submission checks below are intentionally stricter and use CI-style flags so you can catch formatting drift and clippy warnings before requesting review.
+
+## Before You Open a PR
+
+Run the local validation checks required before requesting a review. These are stricter than the commands for iterative development:
+
+```bash
+cargo fmt --all -- --check
+cargo clippy --all --benches --tests --examples --all-features -- -D warnings
+cargo build
+cargo test
+```
+
+Also run this when your change touches database-backed or integration behavior:
+
+```bash
+cargo test --features integration
+```
+
+Before asking for review:
+
+- Build and exercise the changed path locally, not just the narrowest unit test
+- Keep the PR focused and avoid mixing unrelated concerns
+- Fill out the PR template with a clear summary, validation notes, and impact assessment
+- If your change affects tracked behavior, update `FEATURE_PARITY.md` in the same branch
+- If onboarding or setup behavior changes, update the relevant setup docs in the same branch
+- If you are using a coding agent and it supports them, run `review-pr` or `pr-shepherd --fix` before opening or updating the PR
+- `codex review --base origin/main` is also encouraged before requesting review
+
+## Review Follow-Through
+
+Review conversations are author-owned.
+
+- Address each review comment with a code change or a clear explanation
+- Resolve conversations you have handled; leave them open only when reviewer judgment is still needed
+- Do not leave review cleanup for maintainers when the follow-through belongs to the author
+
+If a PR is stale for more than 48 hours after review feedback is posted, maintainers may take over the follow-up work and land the changes needed to accomplish the original PR or issue intent.
+
 ## Code Style
 
 - Zero clippy warnings policy
@@ -46,7 +121,7 @@ All PRs follow a risk-based review process:
 | Track | Scope | Requirements |
 |-------|-------|-------------|
 | **A** | Docs, tests, chore, dependency bumps | 1 approval + CI green |
-| **B** | Features, refactors, new tools/channels | 1 approval + CI green + test evidence |
+| **B** | Features, maintainer-requested refactors, new tools/channels | 1 approval + CI green + test evidence |
 | **C** | Security (`src/safety/`, `src/secrets/`), runtime (`src/agent/`, `src/worker/`), database schema, CI workflows | 2 approvals + rollback plan documented |
 
 Select the appropriate track in the PR template based on what your changes touch.

src/tools/builtin/file.rs

@@ -117,13 +117,6 @@ impl Tool for ReadFileTool {
 
         let path = validate_path(path_str, self.base_dir.as_deref())?;
 
-        if super::path_utils::is_sensitive_path(&path) {
-            return Err(ToolError::NotAuthorized(format!(
-                "Access denied: '{}' is a sensitive path. Use the appropriate secrets management tool instead.",
-                path_str
-            )));
-        }
-
         // Check file size
         let metadata = fs::metadata(&path)
             .await
@@ -263,13 +256,6 @@ impl Tool for WriteFileTool {
 
         let path = validate_path(path_str, self.base_dir.as_deref())?;
 
-        if super::path_utils::is_sensitive_path(&path) {
-            return Err(ToolError::NotAuthorized(format!(
-                "Access denied: '{}' is a sensitive path",
-                path_str
-            )));
-        }
-
         // Create parent directories
         if let Some(parent) = path.parent() {
             fs::create_dir_all(parent).await.map_err(|e| {
@@ -378,13 +364,6 @@ impl Tool for ListDirTool {
 
         let path = validate_path(path_str, self.base_dir.as_deref())?;
 
-        if super::path_utils::is_sensitive_path(&path) {
-            return Err(ToolError::NotAuthorized(format!(
-                "Access denied: '{}' is a sensitive directory",
-                path_str
-            )));
-        }
-
         let mut entries = Vec::new();
         list_dir_inner(&path, &path, recursive, max_depth, 0, &mut entries).await?;
 
@@ -468,10 +447,6 @@ async fn list_dir_inner(
         entries.push(display);
 
         if recursive && is_dir && current_depth < max_depth {
-            // Skip sensitive directories during recursive traversal
-            if super::path_utils::is_sensitive_path(&entry_path) {
-                continue;
-            }
             // Skip common non-essential directories
             let name = entry.file_name();
             let name_str = name.to_string_lossy();
@@ -586,13 +561,6 @@ impl Tool for ApplyPatchTool {
 
         let path = validate_path(path_str, self.base_dir.as_deref())?;
 
-        if super::path_utils::is_sensitive_path(&path) {
-            return Err(ToolError::NotAuthorized(format!(
-                "Access denied: '{}' is a sensitive path",
-                path_str
-            )));
-        }
-
         // Read current content
         let content = fs::read_to_string(&path)
             .await

src/tools/builtin/path_utils.rs

@@ -4,119 +4,9 @@
 //! attacks and ensure paths stay within allowed sandboxes.
 
 use std::path::{Path, PathBuf};
-use std::sync::LazyLock;
 
 use crate::tools::tool::ToolError;
 
-/// Paths that contain credentials, secrets, or private keys.
-/// Used by both file tools (exact path check) and shell tool (substring scan).
-/// Keep sorted by category for readability.
-static SENSITIVE_PATH_PATTERNS: LazyLock<Vec<&'static str>> = LazyLock::new(|| {
-    vec![
-        // SSH
-        "/.ssh/",
-        "/id_rsa",
-        "/id_ed25519",
-        "/id_ecdsa",
-        "/id_dsa",
-        "/authorized_keys",
-        "/known_hosts",
-        // GPG
-        "/.gnupg/",
-        // AWS
-        "/.aws/credentials",
-        "/.aws/config",
-        // Kubernetes
-        "/.kube/config",
-        // Cloud providers
-        "/.azure/",
-        "/.gcloud/",
-        "/.config/gcloud/",
-        // Terraform
-        "/.terraform.d/credentials.tfrc.json",
-        // GitHub CLI
-        "/.config/gh/hosts.yml",
-        // Docker
-        "/.docker/config.json",
-        // Vault
-        "/.vault-token",
-        // Shell history
-        "/.bash_history",
-        "/.zsh_history",
-        "/.histfile",
-        // Env files (may contain secrets)
-        "/.env",
-        // Git credentials
-        "/.git-credentials",
-        "/.netrc",
-        "/.pgpass",
-        // IronClaw's own secrets
-        "/.ironclaw/secrets/",
-        // System
-        "/etc/shadow",
-        "/etc/gshadow",
-    ]
-});
-
-/// File extensions that are always sensitive regardless of location.
-static SENSITIVE_EXTENSIONS: LazyLock<Vec<&'static str>> =
-    LazyLock::new(|| vec![".pem", ".key", ".p12", ".pfx", ".jks", ".keystore"]);
-
-/// Suffixes that indicate a file is safe despite matching a sensitive pattern
-/// (e.g., `.env.example`, `.env.sample`).
-static SAFE_SUFFIXES: LazyLock<Vec<&'static str>> =
-    LazyLock::new(|| vec![".example", ".sample", ".template", ".dist", ".bak.example"]);
-
-/// Check if a resolved file path points to a sensitive location.
-/// Used by file tools (read, write, list_dir, apply_patch).
-pub fn is_sensitive_path(path: &Path) -> bool {
-    let path_str = match path.canonicalize() {
-        Ok(p) => p.to_string_lossy().to_string(),
-        Err(_) => path.to_string_lossy().to_string(),
-    };
-
-    // Safe suffixes override sensitive patterns
-    let lower = path_str.to_lowercase();
-    if SAFE_SUFFIXES.iter().any(|s| lower.ends_with(s)) {
-        return false;
-    }
-
-    // Check sensitive path patterns
-    if SENSITIVE_PATH_PATTERNS.iter().any(|p| path_str.contains(p)) {
-        return true;
-    }
-
-    // Check sensitive file extensions
-    if let Some(ext) = path.extension().and_then(|e| e.to_str()) {
-        let dot_ext = format!(".{}", ext.to_lowercase());
-        if SENSITIVE_EXTENSIONS.iter().any(|e| *e == dot_ext) {
-            return true;
-        }
-    }
-
-    false
-}
-
-/// Scan a shell command string for references to sensitive paths.
-/// Returns the first matched pattern, or None if the command is clean.
-/// Used by the shell tool to block `cat ~/.ssh/id_rsa` etc.
-pub fn command_references_sensitive_path(command: &str) -> Option<&'static str> {
-    let normalized = command.to_lowercase();
-
-    for pattern in SENSITIVE_PATH_PATTERNS.iter() {
-        // For path patterns, check case-insensitively
-        if normalized.contains(&pattern.to_lowercase()) {
-            return Some(pattern);
-        }
-    }
-
-    // Check for sensitive extensions in file arguments
-    SENSITIVE_EXTENSIONS
-        .iter()
-        .find(|ext| normalized.contains(*ext))
-        .copied()
-}
-
 /// Normalize a path by resolving `.` and `..` components lexically (no filesystem access).
 ///
 /// This is critical for security: `std::fs::canonicalize` only works on paths that exist,
@@ -346,95 +236,4 @@ mod tests {
         let result = validate_path("a/b/../c.txt", Some(dir.path()));
         assert!(result.is_ok());
     }
-
-    // ── sensitive path tests ──
-
-    #[test]
-    fn test_is_sensitive_path_blocks_ssh() {
-        assert!(is_sensitive_path(Path::new("/home/user/.ssh/id_rsa")));
-        assert!(is_sensitive_path(Path::new(
-            "/home/user/.ssh/authorized_keys"
-        )));
-        assert!(is_sensitive_path(Path::new("/root/.ssh/config")));
-    }
-
-    #[test]
-    fn test_is_sensitive_path_blocks_cloud_credentials() {
-        assert!(is_sensitive_path(Path::new("/home/user/.aws/credentials")));
-        assert!(is_sensitive_path(Path::new("/home/user/.kube/config")));
-        assert!(is_sensitive_path(Path::new("/home/user/.azure/some_token")));
-        assert!(is_sensitive_path(Path::new(
-            "/home/user/.config/gh/hosts.yml"
-        )));
-    }
-
-    #[test]
-    fn test_is_sensitive_path_blocks_system_secrets() {
-        assert!(is_sensitive_path(Path::new("/etc/shadow")));
-        assert!(is_sensitive_path(Path::new("/etc/gshadow")));
-    }
-
-    #[test]
-    fn test_is_sensitive_path_blocks_key_files_by_extension() {
-        assert!(is_sensitive_path(Path::new("/tmp/server.pem")));
-        assert!(is_sensitive_path(Path::new("/app/certs/private.key")));
-        assert!(is_sensitive_path(Path::new("/home/user/keystore.p12")));
-    }
-
-    #[test]
-    fn test_is_sensitive_path_allows_safe_suffixes() {
-        assert!(!is_sensitive_path(Path::new("/app/.env.example")));
-        assert!(!is_sensitive_path(Path::new("/app/.env.sample")));
-        assert!(!is_sensitive_path(Path::new("/app/.env.template")));
-    }
-
-    #[test]
-    fn test_is_sensitive_path_allows_normal_files() {
-        assert!(!is_sensitive_path(Path::new("/app/src/main.rs")));
-        assert!(!is_sensitive_path(Path::new("/home/user/README.md")));
-        assert!(!is_sensitive_path(Path::new("/tmp/output.json")));
-    }
-
-    #[test]
-    fn test_is_sensitive_path_blocks_env_files() {
-        assert!(is_sensitive_path(Path::new("/app/.env")));
-        assert!(is_sensitive_path(Path::new("/app/.env.local")));
-        assert!(is_sensitive_path(Path::new("/app/.env.production")));
-    }
-
-    // ── command scanning tests ──
-
-    #[test]
-    fn test_command_references_sensitive_path_catches_cat_ssh() {
-        assert!(command_references_sensitive_path("cat ~/.ssh/id_rsa").is_some());
-        assert!(
-            command_references_sensitive_path("head -n 5 /home/user/.ssh/authorized_keys")
-                .is_some()
-        );
-    }
-
-    #[test]
-    fn test_command_references_sensitive_path_catches_aws() {
-        assert!(command_references_sensitive_path("cat ~/.aws/credentials").is_some());
-        assert!(command_references_sensitive_path("grep key ~/.aws/config").is_some());
-    }
-
-    #[test]
-    fn test_command_references_sensitive_path_catches_etc_shadow() {
-        assert!(command_references_sensitive_path("cat /etc/shadow").is_some());
-    }
-
-    #[test]
-    fn test_command_references_sensitive_path_catches_key_extensions() {
-        assert!(command_references_sensitive_path("cp server.pem /tmp/").is_some());
-        assert!(command_references_sensitive_path("cat private.key").is_some());
-    }
-
-    #[test]
-    fn test_command_references_sensitive_path_allows_safe_commands() {
-        assert!(command_references_sensitive_path("ls -la").is_none());
-        assert!(command_references_sensitive_path("cargo build").is_none());
-        assert!(command_references_sensitive_path("git status").is_none());
-        assert!(command_references_sensitive_path("cat README.md").is_none());
-    }
 }

src/tools/builtin/shell.rs

@@ -83,12 +83,21 @@ static BLOCKED_COMMANDS: LazyLock<HashSet<&'static str>> = LazyLock::new(|| {
 });
 
 /// Patterns that indicate potentially dangerous commands.
-/// Note: sensitive file paths (/.ssh/, /etc/shadow, etc.) are now handled by
-/// `command_references_sensitive_path` in path_utils.rs for consistency with
-/// file tool protections. This list covers command-level dangers only.
 static DANGEROUS_PATTERNS: LazyLock<Vec<&'static str>> = LazyLock::new(|| {
     vec![
-        "sudo ", "doas ", " | sh", " | bash", " | zsh", "eval ", "$(curl", "$(wget",
+        "sudo ",
+        "doas ",
+        " | sh",
+        " | bash",
+        " | zsh",
+        "eval ",
+        "$(curl",
+        "$(wget",
+        "/etc/passwd",
+        "/etc/shadow",
+        "~/.ssh",
+        ".bash_history",
+        "id_rsa",
     ]
 });
 
@@ -613,11 +622,6 @@ impl ShellTool {
             }
         }
 
-        // Block commands that reference sensitive file paths (shared with file tools)
-        if super::path_utils::command_references_sensitive_path(cmd).is_some() {
-            return Some("Command references sensitive file path");
-        }
-
         None
     }