diff --git a/plugins/compound-engineering/skills/ce-code-review/SKILL.md b/plugins/compound-engineering/skills/ce-code-review/SKILL.md index 97edcc9fe..323df8951 100644 --- a/plugins/compound-engineering/skills/ce-code-review/SKILL.md +++ b/plugins/compound-engineering/skills/ce-code-review/SKILL.md @@ -438,8 +438,11 @@ Generate a unique run identifier before dispatching any agents. This ID scopes a ```bash RUN_ID=$(date +%Y%m%d-%H%M%S)-$(head -c4 /dev/urandom | od -An -tx1 | tr -d ' ') mkdir -p "/tmp/compound-engineering/ce-code-review/$RUN_ID" +chmod 700 "/tmp/compound-engineering/ce-code-review/$RUN_ID" ``` +mkdir honors the caller's umask, and this run directory contains per-reviewer findings and evidence, so owner-only permissions are required. + Pass `{run_id}` to every persona sub-agent so they can write their full analysis to `/tmp/compound-engineering/ce-code-review/{run_id}/{reviewer_name}.json`. **Report-only mode:** Skip run-id generation and directory creation. Do not pass `{run_id}` to agents. Agents return compact JSON only with no file write, consistent with report-only's no-write contract. diff --git a/plugins/compound-engineering/skills/ce-code-review/references/subagent-template.md b/plugins/compound-engineering/skills/ce-code-review/references/subagent-template.md index 69aea6072..c6f5df87b 100644 --- a/plugins/compound-engineering/skills/ce-code-review/references/subagent-template.md +++ b/plugins/compound-engineering/skills/ce-code-review/references/subagent-template.md @@ -133,7 +133,7 @@ Rules: - Suppress any finding you cannot honestly anchor at `50` or higher (the actionable floor is `50`; anchors `0` and `25` are suppressed by synthesis anyway, so emitting them only adds noise). If your persona's domain description sets a stricter floor (e.g., anchor `75` minimum), honor it. - Every finding in the full artifact file MUST include at least one evidence item grounded in the actual code. The compact return omits evidence -- the evidence requirement applies to the disk artifact only. - Set `pre_existing` to true ONLY for issues in unchanged code that are unrelated to this diff. If the diff makes the issue newly relevant, it is NOT pre-existing. -- You are operationally read-only. The one permitted exception is writing your full analysis to the `.context/` artifact path when a run ID is provided. You may also use non-mutating inspection commands, including read-oriented `git` / `gh` commands, to gather evidence. Do not edit project files, change branches, commit, push, create PRs, or otherwise mutate the checkout or repository state. +- You are operationally read-only. The one permitted exception is writing your full analysis to the OS temp artifact path `/tmp/compound-engineering/ce-code-review/{run_id}/{reviewer_name}.json` when a run ID is provided. You may also use non-mutating inspection commands, including read-oriented `git` / `gh` commands, to gather evidence. Do not edit project files, change branches, commit, push, create PRs, or otherwise mutate the checkout or repository state. - Set `autofix_class` accurately. The classification governs whether the fixer applies the change automatically (`safe_auto`) or surfaces it for explicit review (`gated_auto` / `manual` / `advisory`). **The wrong-side cost is symmetric:** classifying a contract-change as `safe_auto` produces an unwanted edit; classifying a mechanical fix as `gated_auto` makes the user manually triage findings the fixer could have applied. Bias toward `safe_auto` when the rubric permits it. Use this decision guide: - `safe_auto`: The fix is local and deterministic — the fixer can apply it mechanically. **The test:** you can articulate the fix in one sentence with no "depends on" clauses, AND applying it doesn't change any of {function signature, public-API/response contract, error contract, security posture, permission model}. Examples: extracting a duplicated helper, adding a missing nil/null guard inside an internal function, fixing an off-by-one when the parallel pattern is in scope, adding a missing test for an existing public method, removing dead code, removing an unused import. diff --git a/tests/review-skill-contract.test.ts b/tests/review-skill-contract.test.ts index a338ff4bd..56ac93dab 100644 --- a/tests/review-skill-contract.test.ts +++ b/tests/review-skill-contract.test.ts @@ -609,6 +609,30 @@ describe("ce-code-review contract", () => { } }) + test("protects code-review artifact directories with owner-only permissions", async () => { + const content = await readRepoFile("plugins/compound-engineering/skills/ce-code-review/SKILL.md") + + expect(content).toContain('mkdir -p "/tmp/compound-engineering/ce-code-review/$RUN_ID"') + expect(content).toContain('chmod 700 "/tmp/compound-engineering/ce-code-review/$RUN_ID"') + expect(content).toContain("mkdir honors the caller's umask") + expect(content).toContain("per-reviewer findings and evidence") + expect(content).toContain("owner-only permissions") + }) + + test("subagent template documents OS-temp artifact path", async () => { + const template = await readRepoFile( + "plugins/compound-engineering/skills/ce-code-review/references/subagent-template.md", + ) + + expect(template).toContain( + "/tmp/compound-engineering/ce-code-review/{run_id}/{reviewer_name}.json", + ) + expect(template).toContain( + "The one permitted exception is writing your full analysis to the OS temp artifact path", + ) + expect(template).not.toContain("`.context/` artifact path") + }) + test("leaves data-migration-expert as the unstructured review format", async () => { const content = await readRepoFile( "plugins/compound-engineering/agents/ce-data-migration-expert.agent.md",