Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions .claude/rules/doc-hygiene.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
---
paths:
- "**/*.md"
- "**/*.py"
- "docs/**"
---
# Doc Hygiene

## Absolute Paths

Committed `.md` and `.py` files (outside `tests/` and `scripts/`)
must not contain developer-local absolute paths (`/home/<user>/`,
`/Users/<user>/`, `/tmp/`). This is a review convention, not
pre-commit-enforced — grep before merging a docs-touching PR.
Reference: PR #2689.
51 changes: 51 additions & 0 deletions .claude/rules/error-handling.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
---
paths:
- "src/**/*.rs"
- "crates/**/*.rs"
---
# Error Handling

Existing rules forbid `.unwrap()` / `.expect()` in production. The footguns below are equally dangerous and equally banned on DB, IO, workspace, and settings reads.

## Silent-Failure Anti-Patterns

- `.unwrap_or_default()` on a `Result` — collapses errors into empty state. Masks DB outages, migration failures, schema drift. (#2526 `list_projects`, #2653 `.env` scan.)
- `.ok()?` on `Result` — drops the error entirely.
- `let Ok(x) = ... else { return None }` / `else { return }` — same shape, structured.
- `if let Err(e) = ... { warn!(...) }` followed by caching / inserting / continuing — poisons downstream state with a half-initialized value and hides the failure forever. (#2633 `seed_if_empty` cache.)

**Required pattern — fail loud by default:**

```rust
let projects = store.list_projects(&owner_id).await?;
```

**When fallback is genuinely acceptable** — must be justified inline and name the operation:

```rust
let rows = store.list_agent_jobs().await.unwrap_or_default(); // silent-ok: dashboard refresh, next poll retries
```

Review flag: added lines containing `unwrap_or_default()`, `.ok()?`, or `else { return` / `else { return None }` on a DB/IO/workspace call must carry a `// silent-ok: <reason>` comment or be rejected.

## Persist-Then-Reload Atomicity

A write that triggers a runtime rebuild (provider chain reload, settings reload, credential reinjection) is multi-step. The DB row may commit while the rebuild fails — do NOT leave split-brain state.

Two acceptable patterns:

- **Pre-validate** — attempt the rebuild on the new value *without persisting*; only persist on success.
- **Snapshot + rollback** — snapshot the old value, write, attempt rebuild; on rebuild failure, restore the snapshot and return the error.

Reference: PR #2673 `reload_llm_after_settings_change`.

## Error Boundaries at the Channel Edge

No internal identifier, traceback, or transport error may cross a channel boundary to the user. Map at the source:

- `LlmError::BadGateway` / raw HTTP 5xx → "provider temporarily unavailable"
- `LlmError::ContextOverflow` / HTTP 413 → "message too large — summarizing" (every direct-HTTP provider must detect 413)
- Filesystem / workspace errors → "can't access your workspace file" (never expose paths)
- Orchestrator worker tracebacks → "internal task failed" + opaque job id for correlation

Forbidden in user-facing output: raw 5xx codes, Python tracebacks, absolute paths (`/workspace/...`, `/home/...`), internal file names (`.system/`, `AGENTS.md`, `HEARTBEAT.md`, `BOOTSTRAP.md`), wire-format prefixes (`message{content:`, literal `\n`). References: #2546, #2407, #2408, #2489, #2584.
29 changes: 29 additions & 0 deletions .claude/rules/lifecycle.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
---
paths:
- "src/channels/**"
- "src/tools/wasm/**"
- "src/tools/mcp/**"
- "src/bridge/**"
---
# Discovery vs. Activation

**Installed is not active.** These are distinct states with distinct triggers:

- **Discovery** — boot-time scan that enumerates what the user has installed (WASM channels, MCP servers, extensions). Produces a manifest. Side-effect-free.
- **Activation** — explicit state transition that brings an installed thing into a running state (channel opened, WS connected, hooks registered, credentials bound).

A bug shape has recurred 5× on the WASM channel surface (#2556, #2557, #2558, #2564, #2419): activation-level behavior bound to the discovery scan.

## Rules

1. **Registration of runtime effects happens at activation, not discovery.** Hook registration, websocket spawn, poll task spawn, reconnect loops, long-lived state — none of these may run from `discover()`, `list_installed()`, or boot-time iteration of the manifest.

2. **Auth rejection is terminal until credentials change.** A WASM/MCP channel that fails auth on connect MUST NOT retry in a reconnect loop. It transitions to `AuthFailed` and stays there until a credential-change event (new OAuth token, new bot token) triggers re-activation.

3. **`list_installed` vs. `list_active` are separate queries.** Status surfaces and dispatch paths must use the query that matches their intent. A dashboard saying "N channels" must specify which.

4. **Deactivation unwinds everything activation set up.** When a user disables or uninstalls an extension, every runtime effect must be reversed: hooks removed, WS closed, poll task cancelled, in-flight reconnect aborted, snapshot state dropped. No orphaned tasks.

5. **Discovery is idempotent and side-effect-free.** Repeated discovery scans produce the same manifest and must not start tasks, open connections, or touch the network.

6. **Snapshot rehydrate must re-validate.** When restoring cached state across a restart (pending auth prompts, leases, gate state), re-run the type's `::new()` constructor AND check domain invariants (not-revoked, not-expired, `thread_id` matches). Stale snapshots cause "ghost" leases and replayed auth. References: PR #2617 `restore_selected_auth_prompt`, PR #2631 paused-lease rehydrate.
21 changes: 21 additions & 0 deletions .claude/rules/review-discipline.md
Original file line number Diff line number Diff line change
Expand Up @@ -46,3 +46,24 @@ cargo check --all-features # all features
- `grep -rn 'super::' <files>` -- prefer `crate::` for cross-module imports (`super::` OK in tests/intra-module)
- If you fixed a pattern bug, `grep` for other instances across `src/`
- Run `scripts/pre-commit-safety.sh` to catch UTF-8, case-sensitivity, hardcoded /tmp, and logging issues

## PR Scope Discipline

A PR's title and body must match its diff.

- If the title describes one change ("fix auth cancel") but the diff spans multiple layers (provider → bridge → orchestrator → Python), retitle, split, or explicitly call out the scope expansion in the body. Reference: zmanian's review on #2668 (+590/-72 under a title advertising ~10 lines).
- **Move-only refactors** must state "no behavior change" in the body and file a follow-up issue for every pre-existing correctness/perf concern surfaced during the move. Don't silently fix things mid-move — it's unreviewable. Pattern across #2628, #2680, #2687.
- After a refactor that relocates or renames code, grep for `.md` and `CLAUDE.md` references to the moved paths and update them in the same PR. `web/CLAUDE.md` pointing at `server.rs` after its contents moved (#2687) is a review fail.

## Guardrail Scripts Are Code

Lint/boundary/safety scripts under `scripts/` are enforcement infrastructure. They must:

- **Have regression tests** exercising every documented exemption (e.g. `dispatch-exempt`, `silent-ok`, `#[cfg(test)]` skip).
- **Be included in the CI `has_code` / diff-filter** that gates required checks — a guardrail that isn't run on changes to itself can be weakened without anyone noticing. Reference: PR #2647.
- **Parse grouped / multiline Rust syntax** when inspecting imports. Line-based regex misses `use crate::channels::web::{handlers::auth::...}` and shim re-exports.
- **Actually enforce their documented skips** — if the exemption says "skips `#[cfg(test)]` blocks", the scanner must track brace nesting, not match a regex on the first line.

## Stale Comments After Refactors

Doc strings and inline comments are part of the contract. A comment that says "strips trailing punctuation + whitespace" while the code only strips periods (#2701 `src/bridge/router.rs`) is a bug report waiting to happen. When you change behavior in a function, re-read its docstring and adjacent comments — update or delete them in the same change.
34 changes: 34 additions & 0 deletions .claude/rules/safety-and-sandbox.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,11 @@ paths:
- "src/sandbox/**"
- "src/secrets/**"
- "src/tools/wasm/**"
- "src/bridge/**"
- "src/channels/**"
- "src/workspace/**"
- "src/agent/**"
- "crates/ironclaw_engine/**"
---
# Safety Layer & Sandbox Rules

Expand Down Expand Up @@ -32,3 +37,32 @@ The shell tool scrubs sensitive env vars before executing commands. The sanitize
## Zero-Exposure Credential Model

Secrets are stored encrypted on the host and injected into HTTP requests by the proxy at transit time. Container processes never see raw credential values.

## Every New Ingress Scans Before Storage or LLM

Mirror of CLAUDE.md's "Everything Goes Through Tools" rule: every new surface that accepts external data into the system — user messages, webhook payloads, memory writes, URL fetches, file ingestion — must run the matching safety scan on the **pre-transform, pre-injection** payload before the data reaches the LLM or the database.

Recurring bug shape: a new code path is added, and the safety scan is skipped, applied post-injection (too late), or applied to the wrong stage. References: #2491 Engine v2 inbound, #2676 WASM URL post-injection, #2470 memory write layer.

Rules:

- **Inbound user text** → `safety_layer.scan_inbound_for_secrets()` before engine dispatch.
- **Tool output** (existing) → sanitize + leak detector before LLM, wrapped in `<tool_output>` XML.
- **LLM response** → leak detector before user delivery.
- **Memory / workspace writes** → injection scan on the pre-storage value. Never on the transformed/rendered value.
- **URL fetches** → leak-pattern scan on the resolved URL **before** credential injection; not on the post-injection URL.

A newly added ingress handler (HTTP route, webhook receiver, `Channel::send_message` impl) that reaches an LLM call or DB write without calling a `safety_layer.*` function on the payload is a review-blocker.

## Bounded Resources

User-controlled inputs must not grow unbounded. Apply caps at the boundary:

- **Interners, caches, accumulators** — hard size limit (entries + total bytes), eviction policy documented. PR #2673 model-name interner: 256-byte value cap, 1024-entry cap.
- **File reads in HTTP handlers** — stream with `tokio::fs::File::open` + `ReaderStream`; never `tokio::fs::read`, which buffers the whole file. Reference: #2633 item 3.
- **Fan-out scans (portfolio addresses, batch tool calls)** — position cap + O(n) algorithm required, not O(n²). Tool-specific fuel limits, not global raises. Reference: #2710 portfolio tool.
- **Tokio task fan-out** — in-flight dedup or bounded semaphore on spawns driven by user input (PR #2702).

## Cache Keys Must Be Complete

A cache whose stored value depends on input X must include a stable representation of X in its key. `WorkspacePool` keyed on `user_id` but applying token-specific `workspace_read_scopes` before caching (#2633 item 1) froze the first token's scopes for every subsequent request — canonical example. Rule: if `get_or_create(a, b)` inserts using only `a` but `b` affects the stored value, that is a bug.
37 changes: 37 additions & 0 deletions .claude/rules/tool-evidence.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
---
paths:
- "src/agent/**"
- "src/tools/**"
- "src/channels/web/**"
- "crates/ironclaw_engine/**"
---
# Tool Evidence and Side-Effect Verification

The most dangerous user-visible bug class is **claim/evidence drift**: the agent narrates "message sent" / "file attached" / "tool installed" with no corresponding side effect. The agent-facing half of this rule lives in `crates/ironclaw_engine/prompts/codeact_postamble.md` ("Claims in FINAL() need tool evidence"). This file documents the *target* code invariants that make the rule enforceable at the tool layer. Several of these are aspirational — where that's the case, it's called out inline so new contributors don't assume an enforcement mechanism exists.

## Engine v2 Side-Effect Gate (target invariant)

Engine v2 should classify user turns for side-effect intent (send / save / install / schedule / post / write / delete) and a model-final turn that lacks at least one successful tool call matching the intent should be rejected before it reaches the user — surfacing "action not performed" instead of the agent's narration.

**Current state:** only a soft tool-intent *nudge* exists in `crates/ironclaw_engine/src/executor/loop_engine.rs`; there is no hard rejection gate. Adding one belongs on the engine roadmap. Until it lands, the prompt-side guidance in `codeact_postamble.md` is the primary defence. Reference: #2544, #2580, #2582, #2541, #2447.

## Empty-Fast Outputs Are Errors (tool-author convention)

A tool that completes in `< 1 ms` **and** returns empty content is almost always a silent failure. Tool authors must treat this shape as an error at the tool implementation: return a descriptive `ToolError::ExecutionFailed("empty result from <service>: …")` (or the closest matching variant in `src/tools/tool.rs`) rather than a successful empty `ToolOutput`.

**Current state:** the dispatcher does not today enforce a generic "fast + empty = error" rule, and `ToolOutput` / `ActionRecord` do not carry a dedicated byte-count field — timing is captured on `ToolOutput.duration` and content size is implicit in the serialized `result`. A future enforcement path (dedicated `ToolError::EmptyResult` variant, explicit byte-count on `ActionRecord`, UI suppression of the success checkmark on zero-byte output) is desirable; when adding those, update this rule to cite the concrete APIs. Reference: #2545.

## External-Effect Tools Must Read Back

A tool whose side effect is visible only to an external system (Telegram send, Slack post, file write, extension install, OAuth completion) MUST read back the effect before returning success:

- `telegram_send` → capture and return `message_id` from the API response; error if the response lacks one.
- `file_write` → re-stat and return the actual byte count; error on mismatch.
- `extension_install` → call `extensions_list` and assert the new extension is present and active.
- OAuth completion → perform a minimal authenticated read against the provider before declaring success.

A tool without a read-back path is claim-only. There is no canonical `unverified` field on `ToolOutput` today — when you write a claim-only tool, include an `unverified: true` key in the JSON `result` body and a clear hedge in the text output ("submitted; delivery not confirmed") so downstream layers and the user can see it. If/when a first-class field lands on `ToolOutput`, migrate to it. References: #2411 Telegram token Save, #2543 Linear MCP OAuth, #2586 Slack Install.

## Setup UI Round-Trip

Save / Install / Connect buttons in the setup UI must issue a read-back verification immediately after the write succeeds and render the read-back value (or explicit error) to the user — not a local optimistic checkmark. Install actions dispatch through `ToolDispatcher::dispatch` and surface the resulting `ActionRecord`. A UI success state with no corresponding backend read-back is the same bug class as agent claim drift. References: #2411, #2534, #2543, #2586.
Loading
Loading