[ljchang]

Phase P1a of docs/MAIL-RULES-DESIGN.md §11: the mail-rules store, as a mecha-core library (mecha-core/src/mail_rules/). There is no matching, no mail I/O and no CLI yet. Those are P1b (engine) and P1c (surface).

What it is

• rule.toml loader. Name validation, header matchers, match.describe parsed but not evaluated (P4), and priority defaulting to 100. Action groups follow §5's table; outbound groups parse but cannot leave shadow in P1. Action identity is verb + digest, and a duplicated action is refused. Every named file is proved contained in rules/<name>/, with symlinks out refused. draft_with uses the skill store's own name check, which is why skill::validate_name becomes pub(crate).
• approvals.jsonl and mode resolution. One row per (rule, group). Authority is replayed against the preceding state: a raise, a write of off, and a same-mode row rebinding a digest all need a positive Actor::Owner. A row whose actor, mode or group cannot be read holds the rule rather than being skipped. Lapses are written as per-group rows, min(current, propose), and cover historical groups. An already-off group is excluded. A run's off is clamped to shadow. Lowering never depends on loading the rule: the owner can always turn a rule off, and an observed load failure steps it down durably. Raises require a successful load and a matching shown digest.
• ledger.jsonl and key closing. Every §4.4/§8 field is written from day one: verdict_input, the match digest's components even when empty, closes, and the settlement→pending link. Closing is the settlement lookup first, then the union of three signals, and it is decided by the presence of a closing row, not the latest one. Rows missing key fields fail closed. An unreadable verb still closes its key by digest and scope.
• Deferred queue, aliases and limits. The queue is rewritten, not appended; entries expire after seven days as expired, not failed. Settlement preserves work enqueued concurrently. A complete-but-unterminated final row is terminated before the next append.
• The installation identity. Install reserves a name durably in shadow before the directory becomes visible, and refuses a previously used name.

How it was checked. Implemented by Codex (gpt-6-astra, high) from a written brief. Two fresh, read-only Codex runs then tried to refute it:

• The first found 5 major and 2 medium fail-open paths: unreadable actors keeping authority, a malformed group resurrecting auto, remove-then-restore re-arming auto, reinstall inheriting approvals, ledger rows with missing keys hiding closures, off-valued holds, and an unterminated row poisoning the next append.
• The second confirmed those were closed and found 3 medium issues: an unreadable verb hiding a closure, an unreadable rule that could not be turned off, and concurrent enqueue stranding finished work.
• Every fix landed with a regression test that failed on the code before it. The vacuous digest test was proved vacuous by disabling the code under test.

Tests: 52 store tests; workspace clippy -D warnings --all-targets --all-features clean. Fixtures are made up.

[claude]

A raise above a permanent built-in ceiling is recorded rather than refused, and ModeDecision says nothing about it.

This ensure! refuses a raise past the outbound ceiling, but it is keyed on supported() rather than on the ceiling that built_in_holds actually computed — and ActionGroup::Report is supported(). So change_mode("notices", Report, Auto, Some(Actor::Owner), …) succeeds: gate_mode(Propose, Auto, Owner) returns target: Auto, clamped: false, the raise branch passes all three checks, and approvals.jsonl gets an owner-signed to_mode: "auto" row for the spam group.

resolve_mode still clamps effective to Propose (all_action_groups_load_and_outbound_cannot_leave_shadow:153 asserts exactly that), so nothing acts in auto today. Two things follow anyway:

1. change_mode returns ModeDecision, which has no holds field and here reports clamped: false. P1c's mecha mail rule mode <name> report auto has nothing to print but "set to auto" — but §6.1 requires the CLI to say when it did not do what was asked, and §11 calls spam "the one verb that would reach the provider's filter and has no restore, never runs in auto".
2. §3 moved the mode out of rule.toml "precisely so nothing could self-grant auto". Storing an owner-signed auto for the one unrecoverable verb makes the durable record disagree with the invariant, and the only thing standing between it and a live auto spam action is that built_in_holds keeps returning the ceiling.

The sibling case is handled by refusing, so the asymmetry looks unintended. The general fix is to compare the target against the hold ceilings resolve_mode already computes (mode_inputs throws resolved.holds away) rather than against supported(), and to surface the ceiling on ModeDecision so the CLI can name it:

            let ceiling = resolve_mode(&rule, group, &rows, holds)
                .holds
                .iter()
                .fold(Mode::Auto, |ceiling, hold| ceiling.lower(hold.ceiling));
            ensure!(
                decision.target.rank() <= ceiling.rank(),
                "`{group:?}` is held at {ceiling:?} and cannot be raised further"
            );

[claude]

FailureKind::Unknown is the default and does not count toward the failure brake — the one place in this file where unknown fails open.

§6.4 names exactly two exclusions from the consecutive-failure brake: Environment and Interrupted. Unknown is neither, but is_action_failure requires a positive Action, so an unclassifiable failure is silently excluded. Two ways that bites:

• header() (store.rs:180) hard-codes FailureKind::Unknown and settled() clones it through, so a P1b caller that settles Outcome::Failed without remembering to set failure_kind gets a failure that no brake can see. The compiler cannot find that omission — this is the "a closed enum written to an append-only store is a wire format" shape, where the degrade direction matters.
• failure_kind is read with wire, so a row whose kind this build cannot parse degrades to Unknown and drops out of the streak. Every other unreadable field in this PR degrades toward more restriction (Mode::Unknown → held at shadow, ActionVerb::Unknown → closes the key, Outcome::Unknown → closes); this one degrades toward less braking.

Inverting it keeps the rule the doc states and makes a forgotten field fail closed:

    /// §6.4 counts a failure unless it is positively an environment failure
    /// or an interruption: an unreadable kind is the rule's failure, not the
    /// harness's.
    pub fn is_action_failure(&self) -> bool {
        self.outcome == Outcome::Failed
            && !matches!(
                self.failure_kind,
                FailureKind::Environment | FailureKind::Interrupted
            )
    }

If that is deliberate instead — because interrupted() is the only producer today and it does stamp its kind — then the default on header() is the thing to change, so Failed can never be written without a stated kind. Either way it wants a test; nothing currently exercises is_action_failure.

[claude]

Review — mail rules P1a

Read CLAUDE.md, docs/MAIL-RULES-DESIGN.md §3–§6.4, §9 and §11, then the four modules and the test file. The change is sound, and unusually so for 3.9k lines: the security-relevant invariants this repo cares about are all either not in scope or structurally honoured.

Verified locally (not taken from the description): cargo test -p mecha-core --lib mail_rules → 52 passed, cargo clippy -p mecha-core --all-targets --all-features -- -D warnings → clean.

What I checked against the invariants

• Path jail. No ToolCtx involvement — correct, there is no tool here (P7). Every name instead goes through persona::validate_name / skill::validate_name, and every path through contained(), which canonicalizes and proves containment. get() additionally refuses a symlinked rule directory via symlink_metadata before canonicalizing, directory_inputs::walk re-proves containment per entry with a cycle guard, and RuleStore::open catches rules/ being a symlink out because try_exists follows the link and contained then refuses it. store_files_and_skill_files_refuse_outside_symlinks and named_files_cannot_escape_by_parent_absolute_path_or_symlink cover the negatives.
• Taint / network / .from_outside() / dispatch order. Not touched; nothing here reaches the network or the agent loop.
• No abstraction leak. agent.rs untouched; HoldProvider keeps the holds that P1b/P4 will add out of the resolver.
• Config reachability. No Config/ConfigLayer field — the caps live in limits.toml in the rule store, which is what §6.2 explicitly requires to dodge that trap. mail_rules is a pub mod in lib.rs and nothing else.
• Fail-closed. read_rows refuses a partial history rather than dropping a row (the right call for a store where a lost row re-authorizes an action); an unreadable actor, group or mode holds the rule instead of revealing the permission underneath it; valid_transition validates against the preceding validated state, never the row's claimed from_mode; message_closed/thread_closed return true on any incomplete key; Outcome::Unknown and ActionVerb::Unknown both close. GateStatus::of_origins is right because ModelUntrusted is Origin's #[default]/#[serde(other)], so a new variant lands on failing.
• Digest scope matches §6.2/§4.2. Approval digest = directory + skill directories + aliases.toml/limits.toml only when uses_limits() (outbound ∪ Board) ✓. Match digest = matcher (incl. describe) + priority + origin_gate only where once_per_thread(), which is exactly §9's "no outbound group — and not the Board group either" ✓, and the top-level description label is excluded ✓. Lapse rows record effective.lower(Propose) per group, skip already-off groups, and are appended to approvals.jsonl so a revert is inert — §6.2's whole argument.
• brake_targets matches §6.4's thresholds (≥10 and >20% on both rate brakes, ≥3 consecutive, Shadow overwriting Propose because the floor check comes second) and correctly limits the zero-tolerance send brake to is_outbound(), since §6.4 extends only the caps to Board.

Tests

These are the real thing, not scripted-provider theatre: concurrent_queue_rewrites_keep_both_entries_and_concurrent_claims_have_one_winner spawns threads behind a Barrier and asserts exactly one claim wins; historical_row + the unreadable_* tests write adversarial JSONL straight to disk rather than asserting about the constructors; action_identity_ignores_position_and_key_order_but_not_own_parameters pins the digest's stability to behaviour, which is also what would catch toml's preserve_order arriving through feature unification and silently invalidating every stored approval. broken_definition_lowers driving five operations through one body is a good shape for the "an unreadable rule can still be turned off" family.

Findings

Two inline, both medium, both in the direction of a safety mechanism rather than a crash:

1. store.rs:445 — a raise past a permanent built-in ceiling is recorded rather than refused, because the guard keys on supported() instead of on the ceiling built_in_holds computed. An owner can write to_mode: "auto" for the spam group; resolve_mode still clamps it, but ModeDecision comes back clamped: false so the CLI has nothing to say, and the durable record disagrees with §11's "never runs in auto".
2. ledger.rs:230 — is_action_failure requires a positive FailureKind::Action, so Unknown (the header() default, and what wire degrades an unreadable kind to) drops out of the consecutive-failure brake. It is the one unknown in this PR that fails open; §6.4 names only Environment and Interrupted as exclusions.

Minor — yours to weigh at merge, not requests

• list() (store.rs:202) fails whole if rules/ holds any entry get() cannot load. .-prefixed install siblings are skipped, so .DS_Store is fine, but a stray README or a rule whose draft_with skill was deleted makes every rule unenumerable. Fail-closed, and probably what you want for an engine, but P1c's rule list will show the owner an error instead of the other nine rules and the one that is broken.
• enqueue() (store.rs:788) merges into the existing entry for (account, thread_id) and discards the caller's entry.id, so a caller holding that id cannot settle_deferred with it and has to re-read deferred() to find the surviving one. Returning the effective id would remove the trap.
• claim_actions is all-or-nothing: one already-closed sibling returns false for the whole occasion. Reasonable if P1b filters closed actions first; worth stating in the doc comment, which currently reads as if siblings are independent.
• LedgerRow::header indexes rule.actions[action] after bounds-checking definition.actions. Equal lengths are guaranteed by the loader's zip, but LoadedRule's fields are pub, so a hand-built one panics instead of erroring.
• docs/ARCHITECTURE.md gets its Mail rules section "when P1 lands" per §11, and P1a is not P1 — but the invariants most likely to be re-derived wrong by the next lane (the approval-replay rules, effective = recorded ∩ holds, pending-suppression preceding every closure signal) are landing now and currently live only in module docs.

[claude]

claim_actions is all-or-nothing: one closed sibling key silently blocks the retry of a sibling that Outcome::closes deliberately left open.

Outcome::closes returns false for Failed on Mailbox/PrivateRecord precisely so those attempts can be retried, and interrupted() exists to write that row. But this loop returns Ok(false) for the whole occasion as soon as any presented row's key is closed:

1. Rule is [[action]] do='archive' + [[action]] do='tag' (both once_per_thread() == false, so only message_closed applies).
2. claim_actions([archive_pending, tag_pending]) → true; both rows land.
3. tag settles Done (closes = true); archive settles Failed with group Mailbox (closes = false).
4. Next pass re-presents the occasion. archive's key is correctly open, but tag's surviving settlement row is closing() → message_closed is true → return Ok(false).
5. The archive is never retried, and nothing is written to say so.

The caller can't pre-filter its way out of this: the key check is exactly what this function owns under the lock, and the doc comment's own reasoning ("sibling actions share the pre-claim thread check, so one task does not suppress another action in the same occasion") implies the engine presents the whole occasion rather than one action per call — a per-action call would see the previous sibling's thread-closing row.

Returning per-row claim results, or skipping closed rows and claiming the rest, would keep the atomic check while preserving the retryability Outcome::closes is written to express. If the intent is that P1b filters first, the contract is worth stating here, since "already handled" currently means "something in this occasion was handled".

[claude]

Review: Mail rules P1a — the rule store

I read CLAUDE.md, docs/MAIL-RULES-DESIGN.md §§3–6.4 and §9, then the four new modules and the 58 tests. The change is sound. One medium finding is inline; everything else below is an observation for merge-time weighing, not a request.

What I checked and found correct

• Path containment. No fs::* on an unvalidated name. contained() canonicalizes and proves starts_with for the rule directory, rule.toml, every named file, every unnamed file reached by directory_inputs, the resolved draft_with skill directory, and the store's own flat files. get() additionally refuses a symlinked rule directory via symlink_metadata before canonicalizing — a canonicalize-only check would have accepted a rules/alias link. Names pass persona::validate_name / skill::validate_name, so no separator or dot can be spelled. named_files_cannot_escape_by_parent_absolute_path_or_symlink and rule_toml_rule_directory_and_unnamed_files_are_contained_too both exercise the negative, and the in-directory alias.md -> inside.md case proves the positive is not vacuous.
• No abstraction leak, no loop or taint surface. Pure library: no agent.rs edit, no provider name, no tool, no Capabilities, no Conversation. Provenance is taken as a parameter (GateStatus, HoldProvider) rather than inferred, and of_origins returns Unknown for an empty slice and Failing for any ModelUntrusted — the right direction given Origin's untrusted default.
• Config reachability. No Config field, so no ConfigLayer trap. Caps living in the rule store rather than layered config is what §6.2 asks for, and it also keeps a cloned repo from carrying a cap into a trusted session.
• Fail-closed throughout, and measured. read_rows refuses a partial history rather than returning a short list; append re-parses before extending, so a torn final row refuses the append instead of being buried under it; ApprovalRow's hand-written Deserialize derives actor_readable from the wire string and never trusts that bit from disk, so an unreadable actor cannot authenticate even its claimed transition; valid_transition validates against the preceding validated state, so a bad row cannot reveal an older permission underneath it; message_closed and thread_closed return true on any row with an incomplete key; Outcome::Unknown and an unreadable verb both close. Each has a test, and append_terminates_complete_final_rows_and_refuses_torn_rows, unreadable_approval_group_holds_every_group_instead_of_resurrecting_auto and a_nonowner_cannot_rebind_auto_to_new_inputs_without_a_raise_row would all fail on the obvious weaker implementation.
• Closed enums written to an append-only store are treated as wire formats. serde(other) plus the wire deserializer means a non-string value degrades to the default rather than failing the row, and every default is the conservative variant.
• Spec conformance where I could check it mechanically: uses_limits() equals outbound plus Board, matching §6.2's "any rule with an outbound action or a task"; lapse rows record the lower of the group's effective mode and propose, and skip already-off groups entirely (§6.2); brake_targets' three rate thresholds match §6.4 exactly; once_per_thread() equals outbound plus Board plus Unknown, matching §9's gate scope; match_digest covers matcher, priority and the applicable gate, writing every component explicitly and empty when absent (§4.2).
• action_identity_ignores_position_and_key_order_but_not_own_parameters is load-bearing beyond its name. The action digest is serde_json::to_vec over a toml::Value table, in a workspace where serde_json has preserve_order on, so this test is the only thing pinning key-order independence — worth keeping if the toml major version ever moves.
• cargo test -p mecha-core --lib mail_rules passes 58 tests; cargo clippy -p mecha-core --lib --all-features is clean.

Observations (minor, not requests)

• Store directories are created with default permissions while their contents are 0600. RuleStore::open's create_dir_all, its create_dir(&rules), and install's create_dir(&temp) all land at 0755 under the usual umask. Contents are safe — write_new and append both set 0600 — so this leaks only names. But rule names and template filenames are themselves mail-shaped, and frontdoor.rs asserts 0700 on its own requests directory, so the convention already exists.
• brake_targets silently drops a Board entry in wrong_automatic_sends. The group.is_outbound() guard is redundant with set membership, so its only effect is that a caller reporting a wrong automatic task gets neither a brake nor an error. §6.4's zero-tolerance bullet says "any automatic send", and its last bullet puts Board on the send side of the risk line — though only for caps, so this may well be the intended reading. If it is, the guard reads as a filter rather than a decision.
• No retention story for ledger.jsonl, in the code or in the design doc. append re-reads and re-parses the whole file under the lock on every row, so cost is linear per action and quadratic over the store's life, with one row per action per message and no rotation. Not a P1a problem; worth a line in the design doc before P1b starts writing rows at mail volume.

Provenance of this pass

Everything I could check against the tree held up, and the regression tests do fail on the behaviour they describe — I re-derived valid_transition's digest-rebinding authority and install's name-reservation ordering rather than taking the PR summary's word for them. Stating the obvious per CLAUDE.md: this is a session-level review, not the repo's. claude-code-review.yml's pass on this branch is the one that counts, and merging is yours.

[claude]

The store root and rules/ are created with bare fs::create_dir_all / fs::create_dir, so they land at 0o777 & ~umask — 0o755 under the usual umask. Same at line 268 (fs::create_dir(&temp)), which is renamed into place, so every published rules/<name>/ directory is group/world-readable too.

Every other ~/.mecha store goes through crate::create_private_dir instead — outbox.rs:901, trigger.rs:762, learning.rs:1431/1435, persona.rs:1437 — and work.rs's test states the convention outright ("owner-only, like every ~/.mecha leaf"). The files here are correctly 0o600 (write_new, and the .mode(0o600) on each append), so this leaks the directory listing rather than the ledger contents: on a shared machine any local account can enumerate the owner's rule names and watch rules appear and disappear.

Suggest crate::create_private_dir(&root)? / (&rules)? here and in install, with an assertion on the mode of the published rule directory so this cannot regress.

Suggested change

	fs::create_dir_all(&root)?;
	let root = root.canonicalize()?;
	let rules = root.join("rules");
	if !rules.try_exists()? {
	fs::create_dir(&rules)?;
	}
	pub fn open(root: impl Into<PathBuf>) -> Result<Self> {
	let root = root.into();
	crate::create_private_dir(&root)?;
	let root = root.canonicalize()?;
	let rules = root.join("rules");
	if !rules.try_exists()? {
	crate::create_private_dir(&rules)?;
	}

[claude]

enqueue admits a snapshot whose outcome is Pending, and expire_deferred then cannot leave the key open the way §4.4 requires ("would have, skipped and expired leave it open", design line 848).

The two deferred flows are both permitted by the code but only one is handled:

• Snapshots are not ledger rows (what DeferredEntry's doc comment on ledger.rs:320 says, and what the tests use — deferred_rewrites_removes_settled_and_expires_as_expired_not_failure queues an Outcome::WouldHave row). Expiry works: the expired: row has closes = false, and message_closed stays false.
• Snapshots are the claimed Pending rows. settle_deferred → append_ledger_locked validates row.settlement against a pending row in the ledger, which only means anything if claim_actions already appended it, so this flow is anticipated. But expire_deferred writes a new row with id = "expired:…" and settlement = None. settles() requires self.settlement.as_deref() == Some(&pending.id), so the original pending row is never suppressed by surviving(), and Outcome::Pending.closes() is true (ledger.rs:50) — so the key reads closed forever and the action can never be retried, even though its row says expired.

Concretely: claim an archive action (pending row X in the ledger), enqueue that same row as the deferred snapshot, let seven days pass, expire_deferred → ledger holds X (Pending, unsettled) plus expired:<entry>:X; message_closed for that key is true, so the archive is silently dropped rather than retried.

Either enforce the contract — add r.outcome != Outcome::Pending to the ensure! below, so a caller cannot queue an execution row — or have expire_deferred use action.settled(Outcome::Expired, …) when the snapshot is pending. The comment-only contract is the gap; nothing currently fails if P1b picks the second reading.

[claude]

Review: mail rules P1a (the store)

I read CLAUDE.md, docs/MAIL-RULES-DESIGN.md §§3–6 and §11, then the four new modules and the test file. cargo test -p mecha-core --lib mail_rules → 60 passed; cargo clippy -p mecha-core --all-targets → clean.

The change is sound where it matters most. The invariants I went looking for are all actually enforced rather than asserted about:

• Path containment. contained() canonicalises and proves starts_with, and every file that enters a rule's identity goes through it — rule.toml, each template/note, the rule directory itself (plus an explicit symlink_metadata refusal on the directory), the skill directory, and path() for the store's own JSONL files. directory_inputs walks with a cycle guard and bails on anything that is not a regular file or directory. install additionally constrains written file names to <persona::validate_name>.md, so there are no subdirectory or traversal names to resolve. The store sits in ~/.mecha/, out of ToolCtx::resolve's reach, which is exactly why this matters — and §3's requirement is met.
• Fail-closed, consistently. Origin::default() is ModelUntrusted, so GateStatus::of_origins cannot read unknown provenance as clean. An approval row whose actor is not one of the three known strings sets actor_readable = false and holds the group rather than being skipped (approvals.rs:98), and actor_readable is #[serde(skip)] so the bit is never trusted from disk. valid_transition replays against the validated preceding state, not the row's claimed from_mode. Outcome::Unknown.closes() is true and settles() refuses an Unknown verb or group, so an unreadable row suppresses a re-attempt instead of licensing a duplicate one. read_rows refuses a partial history rather than returning the rows it could parse.
• The closed-enum-as-wire-format rule is honoured: Mode, ActionGroup, ActionVerb, Outcome, FailureKind, VerdictInput and ApprovalReason each carry #[serde(other)] + #[default], and FieldType — which only ever appears in owner-written rule.toml, not in an append-only store — correctly does not.
• Both actor gates ship, as §11 requires: a raise needs a positive Actor::Owner and a shown digest equal to the current approval_digest and a built_in_holds re-check, while a lowering needs neither a loadable rule nor a fresh review, and off from a non-owner clamps to shadow. Permanent ceilings (Report → propose, unsupported/description groups → shadow) are re-applied at write time so an ungrantable permission is never recorded as granted — that distinction is easy to miss and it is here.
• Flow-control shapes are right. No nested lock() anywhere (flock is per-open-file-description, so a nested acquire in-process would deadlock); writer-side rewrite is temp-sibling-and-rename with fsync-file → rename → fsync-dir; install makes the shadow approval rows durable before the directory becomes visible, so a crash consumes a name rather than inheriting authority.
• The tests are not scripted-provider theatre. They drive the real store on a real temp directory, and the negatives are non-vacuous — named_files_cannot_escape_by_parent_absolute_path_or_symlink, removing_a_group_then_restoring_it_cannot_restore_auto, a_nonowner_cannot_rebind_auto_to_new_inputs_without_a_raise_row, corrupt_history_stays_held_until_a_valid_owner_transition_repairs_it and broken_definition_lowers would each pass only on the fixed code. brake_targets matches §6.4's four brakes and their thresholds, and rule_cap() = 20 / global_cap() = 50 match §6.4's "twenty matching messages" and "the fifty-first".

Nothing in here touches agent.rs, the dispatch order, Capabilities, taint, Config/ConfigLayer, or any tool — so categories 2–4 of the review brief have no surface in this PR, which matches "no matching, no mail I/O and no CLI yet".

Two findings, both posted inline

1. Store directories are world-readable (store.rs:46, :50, :268) — bare fs::create_dir* instead of crate::create_private_dir, unlike every other ~/.mecha store. Files are correctly 0o600, so this leaks the rule-name listing rather than the ledger.
2. expire_deferred cannot reopen the key when a deferred snapshot is a claimed Pending ledger row (store.rs:821) — the "snapshots are not execution rows" contract lives only in a doc comment, while settle_deferred's settlement validation implies the other flow is anticipated. One ensure! closes it.

Minor, for the owner to weigh at merge

• CLAUDE.md's module map has no mail_rules line, and docs/ARCHITECTURE.md has no Mail rules section. §11 defers the ARCHITECTURE section to "when P1 lands", which P1a isn't — but the module-map line is the cheap half, and it is what tells the next session this store exists before it reaches for ~/.mecha/mail-rules/.
• store.rs:126 zips two vectors that must be the same length: definition.actions.iter().zip(tables). They cannot currently diverge (both come from one toml::from_str of the same text), but zip truncates silently, and LedgerRow::header then indexes rule.actions[action] (ledger.rs:170) after checking definition.actions.get(action) — a divergence becomes a panic rather than an error. An ensure!(definition.actions.len() == tables.len(), …) before the loop, or .get(action) on both, makes it an error instead.
• One unparseable line in approvals.jsonl blocks change_mode entirely (store.rs:418 propagates self.approvals()?), so the owner cannot write off. That is the fail-closed direction, and read_rows's refusal of a partial history is deliberate — but it is a different class from the semantic cases this PR carefully made recoverable ("an unreadable rule that could not be turned off"), and worth a sentence in the P1 doctor work so the remedy is discoverable.
• list() returns Err if any single rule fails to load, so one malformed rule.toml stops every rule from being evaluated. Fail-closed, and plausibly P1b's call, but §4.2's "omitted rules show held: list cap" language suggests per-rule degradation was the intent.
• load_directory calls self.aliases()? / self.limits()? unconditionally, so a corrupt limits.toml fails the load of a mailbox-only rule that does not use limits. Safe direction; noting it only because the digest deliberately excludes those files for such rules, so the two treatments disagree.

[claude]

Minor: the bounds check at line 160-163 is on rule.definition.actions, but the index is then used on the other vector, rule.actions. They're parallel only because load_directory builds them together (store.rs:133), and that zip silently truncates if tables is ever shorter than definition.actions. LoadedRule's fields are all pub, so a P1b caller that constructs one by hand gets a panic instead of an error here.

Not reachable today — both vectors come from the same action array of the same TOML — but a .get() closes it for free:

Suggested change

	action: rule.actions[action].clone(),
	action: rule
	.actions
	.get(action)
	.ok_or_else(|| anyhow::anyhow!("no such action identity"))?
	.clone(),

[claude]

Review: sound. No major or medium finding.

I checked this against the invariants in CLAUDE.md and against docs/MAIL-RULES-DESIGN.md §3/§4.4/§6.1/§6.2/§6.4/§8/§11, then ran the suite and clippy locally: cargo test -p mecha-core --lib mail_rules → 63 passed, and cargo clippy -p mecha-core --all-targets --all-features → clean.

What I verified rather than took on trust

Security invariants. No tool, no network, no agent.rs touch, no Config field — so the path jail, taint, .from_outside(), dispatch order and the Config/ConfigLayer trap are all out of scope by construction. The store's own containment stands in for ToolCtx::resolve as §3 requires: every fs::* on a name out of rule.toml goes through contained() in store.rs, which canonicalizes first and uses Path::starts_with (component-wise, not a string prefix), so ../, an absolute path and a symlink out all fail the load rather than reading elsewhere in ~/.mecha/. directory_inputs' walk re-checks containment per entry and refuses symlink cycles. Checking symlink_metadata(rules) before create_private_dir in open() is the right order — opening a store must not chmod a directory outside its root — and opening_a_store_does_not_chmod_an_escaping_rules_symlink measures it.

Fail-closed, the shape CLAUDE.md names as the silently-degrading guard. Every direction I probed lands closed: Mode/Outcome/ActionGroup/VerdictInput/FailureKind default to the safe variant with serde(other); GateStatus::of_origins inherits imagelib::Origin's ModelUntrusted default, and empty is Unknown, never clean; read_rows refuses a partial history rather than returning a short list; message_closed/thread_closed return closed when any row's key is unreadable; closing() is a union of three signals so closes: false can only ever add nothing (§8); an unreadable settlement row still settles and still closes; Outcome::Unknown closes; is_action_failure counts an unknown kind. valid_transition replaying against the preceding validated state — not the row's claimed from_mode — is the right call: a bad row cannot reveal an older permission underneath it.

Both actor gates (§6.1). I traced the matrix by hand and it holds: a non-owner raise errors; a non-owner off clamps to shadow and records Actor::Unknown, so the page cannot attribute the pause to the owner; an already-off group takes no row on any non-owner step-down, lapses and brakes included; Mode::lower preserves an existing Off through the ceiling clamp without ever manufacturing one; actor_readable is recomputed from the wire string on every read and skipped on write, so it is never trusted from disk. The "owner" | "owner-approved" | "unknown" match in ApprovalRow's Deserialize lines up with Actor's rename_all = "kebab-case". gate_mode's early return for current == Off correctly still falls through to the raise ensure! when a non-owner asks for propose on an off group.

Digests and keys. The match digest covers exactly §4.2's narrow set and the approval digest everything that decides a send — including aliases.toml/limits.toml only when uses_limits(), and the named skill's whole directory. Action identity is verb plus a hash of the action's own table, never its index, and it is key-order-stable: toml's Map is a BTreeMap (preserve_order is enabled on serde_json but not on toml, and to_vec here serializes the toml::Value directly), which is what makes action_identity_ignores_position_and_key_order true rather than incidental. Caps are 20/50 per §5.4, and priority defaults to 100 with ties broken by name per §4.3.

Locking and crash windows. Every append_approval_locked / append_ledger_locked / rewrite caller holds the flock, and I checked for nesting — there is none, which matters because flock on a second fd blocks the same process. install validates and loads before consuming the name, and the approval rows land before the directory becomes visible, so a crash spends a name without ever inheriting old authority. settle_deferred's index-tracking retain preserves snapshots enqueued concurrently, and the expired:<entry>:<action> id makes expiry idempotent across a retry.

Tests. The coverage is unusually load-bearing for a store this size, and the ones I expected to be vacuous are not: digest_inputs_... exercises match_digest's once_per_thread gate branch in both directions (Board changes with GateStatus, Mailbox does not), so inverting or dropping that condition would fail; concurrent_queue_rewrites_... uses real threads behind a Barrier and asserts exactly one claim winner, which is a genuine lock test rather than an assertion about an argv.

Minor — for you at merge time, not blocking

1. One inline comment on ledger.rs:170: the bounds check is on definition.actions but the index is then used on rule.actions. Not reachable today; a latent panic once P1b constructs a LoadedRule by hand, since the fields are pub.
2. brake_targets gates the wrong-send brake on group.is_outbound(), so a Board entry in wrong_automatic_sends is silently dropped. §6.4 puts an automatic task "on the send side of the risk line" for caps but names only "any automatic send" for this brake, so this may well be intended — worth a word either way, because the set's type currently admits a caller mistake that never fires.
3. approval_state's unknown_rule branch is unreachable through the store: approvals() already refuses a row with a blank rule. Harmless defence in depth — flagging it only so a later reader doesn't take it for a live path.

One thing worth recording in docs rather than code

§3 states the reading as "the effective mode is the latest row for the rule and action group", and §8 as "an unknown mode reads as shadow". What approval_state actually implements is stricter and better: a full replay in which one unreadable or unauthorized row invalidates its tail and holds the group at shadow until a positively identified owner row repairs it. The PR description says so, but the design doc's two sentences are now not the whole truth. §11 already schedules ARCHITECTURE.md §Mail rules, CLAUDE.md's module-map line and backlog.rs's "five stores" for when P1 lands — none of them due at P1a — so this is only a note that the replay rule, not the latest-row rule, is the invariant that section should carry.

Merging is yours.

🤖 Generated with Claude Code

[ljchang]

Merging after a review pass with no major or medium finding. For P1b, the first caller to construct a LoadedRule by hand: make ledger.rs's action lookup index the vector it bounds-checks, or make LoadedRule's parallel vectors one vector of pairs, so a hand-built rule errors rather than panics. For P1c's mecha doctor: one unparseable line in approvals.jsonl blocks change_mode, owner off included. That is fail-closed and deliberate, but the doctor should report it and say how to remove the line.

[claude]

Not a live bug — a question to settle before P4 builds on it. This writes resolved.effective into a durable approvals.jsonl row, and resolve_mode folds every HoldProvider ceiling into effective. So whatever hold happens to be in force at lapse time becomes the recorded mode. lower_modes does the same at store.rs:692 (current there is also the hold-clamped effective).

With P1a's own providers nothing misbehaves: built_in_holds only returns a Shadow ceiling for conditions that are permanent for the build (!group.supported(), is_description()), and P1Holds only returns Propose — which is where a lapse lands anyway. The divergence appears when §5.4's holds land in P4. Two of them — "an unknown own address" and "a provider whose authentication headers cannot be read reliably" — are environment conditions that come and go. A provider outage overlapping an owner's edit would then record shadow rather than propose, and §4.4 makes that durable rather than cosmetic: a lapse to propose does not move the activation cutoff, but a step down to shadow ends the stretch, so the mail that arrived during the outage is orphaned and the owner pays a raise per group. That is the precise cost §6.4 argues against for the consecutive-failure brake ("recovering from someone else's downtime would cost an owner raise per group").

§6.2's wording does say "the lower of that group's current effective mode and propose", so the code is defensible as written — but §3 says the opposite about the same inputs ("A hold leaves the recorded modes intact; it stops evaluation, not the record of what permission the owner set"). The design disagrees with itself here, and this is the line where the disagreement gets resolved. Worth either using resolved.recorded with built_in_holds only (permanent ceilings, not computed ones), or adding a sentence to §6.2 saying computed holds are deliberately baked in and why the activation-stretch cost is accepted.

[claude]

Review: sound. No major or medium finding.

I read CLAUDE.md, MAIL-RULES-DESIGN.md §3–§6 and §8/§11, then the four new modules against them. Checks I ran locally: cargo test -p mecha-core --lib mail_rules → 63 passed, and cargo clippy -p mecha-core --all-targets --all-features → clean.

Where I pushed hardest, and what held

Containment. This store sits in ~/.mecha/, outside ToolCtx::resolve's reach, so §3's own containment rule is the substitute and contained() is the whole of it. It canonicalizes and uses component-wise starts_with against an already-canonical root at every call site (open canonicalizes first; get canonicalizes rules/ before joining; load_directory and directory_inputs inherit a canonical root). fs::read only ever happens on the resolved path. get refuses a symlinked rule directory outright; directory_inputs carries an ancestor set for symlink cycles; path() turns a dangling store-file symlink into an error rather than a NotFound, which is the right direction. install's file names must end .md with a persona::validate_name stem, so no separator or .. reaches a join. I could not find a fs::* on an unproved path.

Fail-closed. The seven paths the second refutation run closed all read as closed to me, each for a reason I could trace: actor_readable is computed at deserialize time and #[serde(skip)] so it can never be asserted from disk; valid_transition validates against the preceding validated state rather than the row's claimed from_mode, so a bad row cannot reveal an older permission underneath it; an Unknown-group row applies to every group and is invalid in all of them, which is what stops a malformed group resurrecting auto; held is cleared only by a later positively-identified owner; read_rows refuses a partial history rather than losing a row that could re-authorize an action. GateStatus::of_origins keys off Origin::ModelUntrusted, which is Origin's own #[default] + #[serde(other)], so an unreadable provenance reading classifies failing — the right direction, and it matches imagelib's stated default.

Both actor gates (§6.1). Actor::OwnerApproved is correctly not owner: it cannot raise and cannot write off. A non-owner off clamps to shadow in gate_mode and is independently refused in valid_transition — defense in depth across the read and write paths. A raise requires loaded? to succeed plus approval_digest == shown, and built_in_holds is re-checked so an unsupported permission is never recorded as granted. The one case I specifically tried to break — owner requests auto on a rule whose files have become unreadable — returns the load error rather than writing the row, because the loaded? sits inside the target.rank() > current.rank() branch.

The ledger's wire format. Matches §8's union precisely: settlement lookup first (surviving), then closes ∨ the reader's own §4.4 table ∨ unreadability, so closes: false can only ever add nothing. Outcome::closes maps §4.4's table exactly, with Unknown => true and Failed open only for Mailbox/Private record. thread_closed filters on once_per_thread(), which is why a Mailbox done correctly does not close a thread while a Board done does. settles() refuses to let an ambiguous verb or group discard a pending row's closure evidence. The closes loosening on a settlement row is right — §8 says the settlement row's own closes decides once it exists.

Items 1–4 of the review brief are mostly not in scope here: no tool, no network call, no agent-loop or provider contact, and no Config/ConfigLayer field (the only edit outside mail_rules/ is skill::validate_name → pub(crate), which is the right visibility for a core-internal caller and keeps §6.3's "the skill store's own name check" honest rather than re-deriving it).

Tests. These are measuring, not asserting. The digest tests separate what lapses approval from what resets matching history, which is the §6.2/§4.2 distinction that is easy to collapse. unknown_failure_kinds_count_toward_the_consecutive_failure_brake and a_settled_failure_without_an_explicit_kind_counts_toward_the_brake test the unknown direction, which is the one that fails open. opening_a_store_does_not_chmod_an_escaping_rules_symlink asserts the permissions of the escaped directory afterwards rather than just the error — a negative that isn't vacuous. brakes_have_strict_rate_thresholds_and_failure_floor pins > 0.2 rather than >= 0.2.

Minor, for you to weigh at merge time (not requests)

• One inline comment on lapse_locked writing the hold-clamped effective into a durable row. Not reachable with P1a's own hold providers; it is a §3-vs-§6.2 disagreement in the design that this line resolves, and worth resolving on purpose before §5.4's transient holds arrive in P4.
• store.rs:133 — definition.actions.iter().zip(tables) truncates to the shorter side. Both come from the same text, and I could not construct a TOML where they diverge (a non-array action fails the typed parse, a missing one fails .context("missing actions")), so this is unreachable today. But if it ever did diverge, LedgerRow::header would index rule.actions[action] at ledger.rs:170 after definition.actions.get(action) succeeded — a panic in a library the engine calls per message. An ensure! on equal lengths makes the invariant local instead of inferred.
• No docs/ARCHITECTURE.md §Mail rules section and no CLAUDE.md module-map line for mail_rules/. §11 defers the section to "when P1 lands" and P1a is not all of P1, so deferring is defensible — but ~2,100 lines of new subsystem are in the tree now, and the module map is the first thing a fresh session reads. A one-line entry would be cheap insurance against the next lane not knowing this exists.

Merging is yours. Nothing here blocks it.

[ljchang]

Merged with the branch behind main on GitHub's view. The gate's intent (test the tree being merged) was met locally: branch + main at fec8d55 was merged in a scratch worktree, and workspace clippy -D warnings --all-targets --all-features plus cargo test -p mecha-core (4077 passed) ran on it. main was still at that commit when this merged. main moved 9 commits during a single CI cycle, which is why the server-side update was not used.