Illia Polosukhin c725366e70 docs(rules): add review-driven guidance for Claude Code (#2714) ...

* docs(rules): add review-driven guidance for Claude Code

Synthesizes recurring patterns from ~30 merged PRs, 147 bot review
comments (Copilot/Gemini), human reviews, and ~50 issues filed in the
past 2 weeks. Each rule cites the motivating PR/issue numbers.

New files:
- error-handling.md — silent-failure taxonomy (unwrap_or_default, .ok()?,
  poisoned caches), persist-then-reload atomicity, channel-edge error
  mapping. (#2526, #2633, #2653, #2673, #2546, #2407, #2408)
- agent-evidence.md — side-effect claims must cite tool evidence,
  empty-fast outputs are errors, external-effect tools must read back,
  setup UI round-trip. (#2544, #2580, #2582, #2541, #2545, #2411, #2543,
  #2586)
- lifecycle.md — discovery vs. activation, terminal auth rejection,
  list_installed vs. list_active, deactivation unwinds, snapshot
  rehydrate must re-validate. (#2556, #2557, #2558, #2564, #2419,
  PR #2617, PR #2631)

Extended:
- types.md — from_trusted boundary rule, validated-newtype template with
  shared validate(&str), serde(try_from) required for validated types,
  wire-stable enums (no Debug; serde alias for migrations), canonical
  wire-contract field naming. (PR #2685, #2681, #2687, #2678, #2669,
  #2665, #2683, #2702)
- safety-and-sandbox.md — every new ingress scans pre-transform/pre-
  injection, bounded resources (interners/streams/fan-out caps), cache
  keys must be complete. (#2491, #2676, #2470, #2633, #2673, #2710,
  PR #2702)
- review-discipline.md — PR scope discipline, guardrail scripts are
  code (regression tests, grouped-import parsing, CI has_code inclusion),
  absolute-path ban in committed docs, stale comments after refactors.
  (PR #2668, #2628, #2680, #2687, #2647, #2689, #2701)

All new files carry paths: frontmatter so they auto-load only on
matching files.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* refactor(rules): split agent-evidence into prompt + code rule

agent-evidence.md mixed two concerns: runtime agent instruction (what
the LLM should do when concluding a turn) and code-enforcement rules
(what the dispatcher, engine, and tools must implement). Rules under
.claude/rules/ only guide Claude Code when editing the repo — the
runtime agent never reads them.

Splits the two:

- crates/ironclaw_engine/prompts/codeact_postamble.md — new section
  "Evidence before claiming side effects". Sits next to the existing
  "FINAL() answer quality" guidance; loaded via include_str! in
  executor/prompt.rs (no Rust change needed).
- .claude/rules/tool-evidence.md — renamed from agent-evidence.md,
  keeps only the code invariants (engine v2 side-effect gate,
  empty-fast ToolError::EmptyResult, external-effect tools must read
  back, setup UI round-trip).

Prompt tests pass unchanged; the postamble addition is pure text.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* prompt: tighten evidence rule to FINAL() claims only, not tool use

Live-test validation of the "Evidence before claiming side effects"
section (added in the prior commit) showed it inhibited legitimate
tool use. With the original wording, `zizmor_scan_v2` live-recording
timed out at 302s with zero responses; reverting the postamble
restored healthy behavior (88s run, 8 shell calls including
`cargo install zizmor` and full workflow analysis).

The original phrasing conflated two things: what the agent should
claim and what tools it should call. The rule is only about the
claim. Re-tunes the section to:

- Open with an explicit "this does not restrict tool calls" scope.
- Drop the "<1ms = failure" heuristic (too broad — normal tools like
  `tool_info(schema)` are legitimately fast).
- Drop the full enumeration of forbidden side-effect verbs; keep the
  rule narrower and clearer.
- Shorten the code example (remove redundant early-return).

Re-tuned run: agent is active (shell calls, real reasoning), live
recording completes in ~9s. The remaining test failure is a
pre-existing assertion bug (exact `t == "shell"` match against tool
strings that now carry arguments like `"shell(cmd)"`) — reproduces
with the old postamble too.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* test(live): fix tool-name assertions + re-record zizmor traces

The two `zizmor_scan*` live tests had four broken tool-name assertions
that silently failed to match: `tools.iter().any(|t| t == "shell")`
against a tool list that now contains `"shell(cmd)"` strings (tool
events carry args via `format_action_display_name` in
`src/bridge/router.rs`). Two of the four were negative assertions
checking for the absence of `tool_install` recovery loops — those
silently passed even when a recovery loop actually ran. `sandbox_live_e2e.rs:203`
already used the correct `t == "shell" || t.starts_with("shell(")`
pattern; applied it consistently to all four sites.

Verified live:

- `IRONCLAW_LIVE_TEST=1 cargo test --test e2e_live -- zizmor_scan --ignored --test-threads=1`
  → 2 passed, 0 failed, 51.78s. Agent installs and runs zizmor
  end-to-end, producing real findings (exit code 14, dangerous
  triggers, excessive permissions, etc.).

Traces re-recorded with the tuned postamble (commit 50d85175) and
scrubbed: replaced `/home/illia/.cargo/bin/zizmor` with
`/home/user/.cargo/bin/zizmor` per the developer-local-path ban in
`.claude/rules/review-discipline.md`. No credentials, PII, or
high-entropy secrets in either trace (only git SHAs from zizmor's
workflow analysis output).

Replay still passes: `cargo test --test e2e_live -- zizmor_scan --ignored`
→ 2/2 ok.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* test(replay): update zizmor_scan_v2 insta snapshot

The engine v2 replay-snapshot gate (`engine_v2_tests::snapshot_zizmor_scan_v2`)
failed against the re-recorded trace from 1691efec because the old
snapshot encoded a broken run:

- final_state: Failed
- Missing Assistant message role
- 3 issues: thread_failure (error), no_response (warning), llm_error (error)
- 6 tool calls that never produced a final answer

The new trace completes cleanly:

- final_state: Done
- System / User / Assistant roles present
- 1 issue: mixed_mode (info)
- 3 shell tool calls + successful `FINAL()` with real findings

The snapshot was pinning a regression. Regenerated with
`INSTA_UPDATE=always cargo test --test e2e_engine_v2 -- snapshot_zizmor_scan_v2`;
passes on replay.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* docs(rules): address PR #2714 review feedback

- review-discipline: reword "Doc Absolute Paths" as a review convention
  (pre-commit only scans .rs; the rule misleadingly claimed enforcement).
- safety-and-sandbox: broaden `paths:` frontmatter to include the actual
  ingress owners (`bridge`, `channels`, `workspace`, `agent`, engine
  crate) so the rule auto-loads where it applies.
- tool-evidence: mark the side-effect gate, empty-fast rule, and
  `unverified` flag as target/aspirational invariants — neither
  `ToolError::EmptyResult`, an `unverified` field on `ToolOutput`, nor a
  byte-count field on `ActionRecord` exist today. Point at concrete
  interim conventions (`ToolError::ExecutionFailed`, `unverified: true`
  in the JSON result body).
- types: scope "Validated newtypes must gate Deserialize" to *new*
  types, document the `CredentialName`/`ExtensionName` exception (they
  intentionally use `#[serde(transparent)]` + derived `Deserialize`
  under the `serde_does_not_revalidate` test). Clarify the
  `from_trusted` trust boundary (trusted = typed upstream, untrusted =
  raw JSON field even if the field *name* is "registry entry").
  Switch `new` template to `impl Into<String>` to avoid an unnecessary
  clone when an owned `String` is passed.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* docs(rules): simplify types.md + split doc-hygiene; address review round 2

- types: collapse two templates into one canonical validated-newtype
  shape. New types use `#[serde(try_from = "String")]` with a shared
  `validate(&str)` helper — no more dual "transparent for some /
  try_from for others" guidance. `CredentialName`/`ExtensionName` are
  documented as the sole legacy exception (locked in by the
  `serde_does_not_revalidate` test); new code must not copy their
  `transparent` + `from_trusted` pattern. Removes the long "Using
  `from_trusted` safely" section and the separate "Validated newtypes
  must gate Deserialize" subsection that contradicted the Don'ts list.
- doc-hygiene: new tiny rule file scoped to `**/*.md`, `**/*.py`,
  `docs/**` that carries the "no developer-local absolute paths in
  committed docs" convention. Removed from review-discipline.md where
  its `src/**/*.rs` scope meant the rule never loaded on the files it
  governed.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* test(live): match hyphenated tool-install in attempted_relevant_tool

The engine records `action_name` as the raw string the LLM emitted
(`crates/ironclaw_engine/src/executor/structured.rs:381`), and the
registry's lookup canonicalization only affects dispatch — not the
name that reaches `StatusUpdate::ToolStarted`. The two other predicates
in this file (`bad_recovery` at :420, `phase_b_recovery` at :531)
already defend against both forms; this one should too, for
consistency. Addresses PR #2714 review.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

2026-04-21 00:03:29 +09:00

..

commands

…

rules

…

skills

…