fix(tools): enforce the autonomy gate before memory_forget reads the store

[sulthannauval]

Replaces #433 — same commit, same diff. That PR was closed automatically when its branch was renamed from fix/085-memory-forget-gate-ordering to this one; no review had happened on it.

Summary

• Problem: MemoryForgetTool::execute resolved its selector first and consulted enforce_tool_operation second. The key selector needs no lookup, so the ordering was invisible there — but contains resolves via resolve_unique_entry, which reads the whole store and, on a phrase matching more than one entry, returns an error naming the candidates.
• Why it matters: a refused caller was answered out of memory contents instead of being told it was refused. Nothing was deleted — the gate did stop the mutation, just late — but a blocked caller was told "be more specific", an instruction to retry a call it can never complete; a full list() ran on every call the policy had already decided to refuse; and the rate-limit path was worse than the read-only one, because enforce_tool_operation is what records the action, so that work happened outside anything the limiter accounts for.
• What changed: argument-shape validation stays first (a malformed call is the model's mistake whether or not the caller may act), the gate runs next, and contains resolves only for a permitted call.
• What did NOT change (scope boundary): what resolve_unique_entry reveals on an ambiguous match is untouched — naming the candidates is what makes the error actionable for a permitted caller, and forget_by_ambiguous_contains_is_rejected pins it. No new ToolOperation granularity so a resolve counts as a Read: YAGNI, no caller needs that distinction today.

Measured against the previous behaviour:

ReadOnly + {"contains": "deploy"}
-> 'deploy' matches 2 memories (b, a); be more specific or address one by key

memory_store was checked for the same shape and does not have it — it already enforces before resolving its replaces selector. The plan called for fixing both if both were affected; only one was.

Honest framing of severity: this is not a privilege escalation. ReadOnly permits ToolOperation::Read, so memory_recall is available to the same caller and returns memory content directly — the error message reveals nothing the caller could not already ask for. What was wrong is the ordering, the misleading refusal message, and the uncounted work.

Label Snapshot (required)

• Risk label (risk: low|medium|high): risk: low
• Size label: auto-managed
• Scope labels: tool, security, memory
• Module labels: tool: memory_forget
• Contributor tier label: auto-managed
• If any auto-label is incorrect, note requested correction: —

Change Metadata

• Change type: bug
• Primary scope: security

Linked Issue

• Closes #
• Related #
• Depends on # — none; independent of chore(deps): bump the rust-all group across 1 directory with 23 updates #82/chore(deps): bump which from 7.0.3 to 8.0.3 #83/feat(gateway): console model catalog + secrets + best-effort refresh (0.6.70-alpha) #84 in this series
• Supersedes # — none

Validation Evidence (required)

cargo fmt --all -- --check                                              # clean
cargo +1.92.0 clippy --locked --all-targets -- -D clippy::correctness    # clean
BASE_SHA=<main> ./scripts/ci/rust_strict_delta_gate.sh                  # no blocking issues on changed lines
cargo test --lib -- memory_forget                                        # 13 passed, 0 failed (9 existing unchanged)

Clippy is run under 1.92.0 because that is what .github/workflows/ci-run.yml pins; the local default toolchain is 1.97 and reports a large pre-existing diagnostic set that CI does not gate on.

• Evidence provided: four new tests. The pre-fix behaviour was captured directly by a probe (output quoted above) before any code changed. The discriminating cases use an ambiguous phrase, because with a unique match the old ordering also produced the read-only message — a unique-match test alone would not distinguish fixed from unfixed:

  • forget_by_ambiguous_contains_reports_the_gate_not_the_ambiguity — asserts the error says read-only mode and does not say be more specific
  • forget_by_contains_blocked_when_rate_limited — ambiguous phrase, asserts Rate limit exceeded
  • forget_by_contains_blocked_in_readonly_mode — unique match; plain coverage, does not discriminate
  • a_refused_forget_does_not_read_memory — a counting Memory proving a refused call performs zero reads, with a permitted control so the counter is not vacuous

  All four existing gate tests (:224, :245, :263, :165) pass unchanged — the plan required that a reordering not need them edited.

• If any command is intentionally skipped: full-workspace cargo test was not run (disk-bound on this machine); memory_forget was run instead.

Security Impact (required)

• New permissions/capabilities? No — this narrows what a refused call does, it does not widen anything.
• New external network calls? No
• Secrets/tokens handling changed? No
• File system access scope changed? No

Describe risk and mitigation: a ReadOnly or rate-limited caller previously received a memory-derived error naming matching keys. As noted above this is not an escalation (memory_recall is permitted to the same caller), but a refused call now returns the refusal and reads nothing.

Privacy and Data Hygiene (required)

• Data-hygiene status: pass
• Redaction/anonymization notes: none needed — fixtures use the deploy runbook / the deploy schedule.
• Neutral wording confirmation: confirmed.

Compatibility / Migration

• Backward compatible? Yes
• Config/env changes? No
• Migration needed? No

One behaviour change visible to a model: memory_forget with contains under a refusing policy now returns the policy error rather than the ambiguity error. That is the fix.

Human Verification (required)

• Verified scenarios: ReadOnly + ambiguous contains → refusal, both entries survive; ReadOnly + unique contains → refusal, entry survives; rate-limited + ambiguous contains → Rate limit exceeded; a refused call reads the store zero times, while a permitted one reads it at least once.
• Edge cases checked: neither selector still returns a hard anyhow::Err (not a ToolResult) so the "model called this wrong" vs "call refused" distinction is preserved — forget_missing_key asserts it and is unchanged; both selectors together still returns "not both" before the gate.
• What was not verified: no live agent run against a real provider with a ReadOnly profile. All evidence is unit-level against the tool.

Side Effects / Blast Radius (required)

• Affected subsystems: tools/memory_forget only. One function body reordered.
• Potential unintended effects: none identified — the gate takes a fixed ToolOperation::Act and a literal tool name, so it depends on nothing the selector match produces. That is what makes this a pure reordering.
• Guardrails: a_refused_forget_does_not_read_memory fails if a future edit moves resolution back above the gate.

Agent Collaboration Notes (recommended)

• Agent tools used: Claude Code.
• Workflow/plan summary: plans/085-memory-forget-gate-runs-after-reading-memory.md.
• Verification focus: the plan's suggested unique-match test was found not to discriminate (it passes against the unfixed code too), so the discriminating cases were switched to ambiguous phrases and a counting-mock test was added to assert the "no work on a refused call" property directly. The unique-match test is kept as coverage and labelled as such.
• Confirmation: naming + architecture boundaries followed (AGENTS.md + CONTRIBUTING.md) — yes.

Rollback Plan (required)

• Fast rollback: git revert ab7647c. Single commit, one function body.
• Feature flags: none.
• Observable failure symptoms: memory_forget returning "not both" or "Missing 'key' or 'contains'" where a policy refusal was expected, or an agent looping on memory_forget retries.

Risks and Mitigations

• Risk: a local enum Selector was introduced inside execute to split validation from resolution without a match arm that cannot be reached.

  • Mitigation: the alternative shapes both needed an unreachable!() or a duplicated error message. §7.3 asks for no panics in the runtime path, and §3.1 asks for explicit match branches and typed values; a three-line local enum satisfies both. It is private to the function.

• Risk: the refusal message a model sees on the contains path changes, so an agent trained against the old string would see different text.

  • Mitigation: that is the defect being fixed — the old string instructed a retry that could never succeed.

🤖 Generated with Claude Code

[github-actions]

PR intake checks found warnings (non-blocking)

Fast safe checks found advisory issues. CI lint/test/build gates still enforce merge quality.

• Incomplete required PR template fields: summary problem, summary why it matters, summary what changed, validation commands, security risk/mitigation, privacy status, rollback plan

Action items:

1. Complete required PR template sections/fields.
2. Remove tabs, trailing whitespace, and merge conflict markers from added lines.
3. Re-run local checks before pushing:
   • ./scripts/ci/rust_quality_gate.sh
   • ./scripts/ci/rust_strict_delta_gate.sh
   • ./scripts/ci/docs_quality_gate.sh

Run logs: https://github.com/RantAI-dev/RantAIClaw/actions/runs/31260474038

Detected blocking line issues (sample):

• none

Detected advisory line issues (sample):

• none