fix(approvals): protect persisted security decisions

[9thLevelSoftware]

Protect persisted approval outcomes from agent writes

Follow-up to #17 after an adversarial post-merge review.

Security issue

approval_requests.json contains persisted terminal approval outcomes. The ordinary write_file/patch surfaces previously allowed the agent to replace that file, and terminal redirection/in-place edits did not classify it as a sensitive write target. A forged resolved record with a matching session and argument hash could therefore bypass the intended approval gate.

Fix

• block write_file and patch writes to default, profile-qualified, container, and Windows approval-state paths
• classify shell redirection, tee, cp/mv/install, symlinks, sed -i, and perl/ruby -i writes to that state file as approval-requiring terminal operations
• cover ~/.hermes, $HERMES_HOME, profile-qualified, and resolved absolute active-home forms
• preserve normal internal persistence through the approval module itself

Verification

scripts/run_tests.sh tests/tools/test_approval.py -q -k HermesApprovalStateWriteProtection
9 passed, 0 failed

scripts/run_tests.sh tests/tools/test_file_write_safety.py tests/tools/test_approval_fallback_identity.py tests/tools/test_approval_fallback_execution.py tests/tools/test_request_tool_approval.py -q
77 passed, 0 failed

git diff --check passes.

[gemini-code-assist]

Code Review

This pull request implements write protection for the Hermes approval state file (approval_requests.json) to prevent agents from forging resolved requests and bypassing terminal approval gates. The changes include adding regex patterns to block direct writes, copies, and in-place edits, as well as path checks and corresponding tests. The review feedback highlights two critical security concerns: first, the current patterns do not block the creation of symlinks (e.g., via ln) targeting sensitive paths, which could allow an agent to bypass the write protection; second, path resolution mismatches in containerized environments could bypass the strict string comparison check, requiring a more robust path comparison approach.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7530c744e4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[kilo-code-bot]

Code Review Roast 🔥

Verdict: No Issues Found | Recommendation: Merge

The cp-link bypass is closed. The new pattern at tools/approval.py:682 correctly catches the gap where the existing cp/mv/install rule's _COMMAND_TAIL anchor misses commands like cp ~/.hermes/approval_requests.json /tmp/foo -s (link flag after the sensitive source), and the test loop exercises all four real cp link-creation flags (-s, -l, --symbolic-link, --link). All five previously-open findings — ln symlink destination, container/path canonicalization, profile-qualified paths, dot-segment normalization, description lag — are resolved in this PR's history.

Files Reviewed (2 files changed in increment)

• tools/approval.py - clean (cp-link pattern closes the source-as-sensitive-target bypass)
• tests/tools/test_approval.py - clean (parametrized loop over the four real cp link flags)

---

Ponytail review:

• tools/approval.py:682: shrink: the lookahead \s-[^\s]*[sl][^\s]* matches any short flag containing s or l, so cp -S ~/.hermes/... (backup suffix) and cp -L ~/.hermes/... (dereference) would trip the "link via cp" description even though neither creates a link. Tighten to \s-(?:s|l|sf|lf)\b (or enumerate known link-flag combinations) so only real link-creation flags trigger this rule's label. Not a security issue — the cp/mv/install rule at line 683 still gates the command — just a cosmetic description drift. Optional.

Ponytail net: 0 lines.

---

Final Merge Guidance: Can merge as-is. The cp-link regex nitpick is optional cleanup.

Previous Review Summaries (3 snapshots, latest commit 2ae9a8c)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit 2ae9a8c)

Verdict: No Issues Found | Recommendation: Merge

Oh wait, this incremental commit is actually clean. I had my torch primed for a leftover stash of description rot and symlink-source edge cases, and the diff disarmed every one of them.

📊 Overall: Two surgical regex edits plus three regression tests — like watching a surgeon close one wound, drop the scalpel, and walk out without leaving a suture kit on the tray. The previously-flagged ln description lag finally learned how to read the cp/mv/install neighbor it was always supposed to mimic, and the _HERMES_APPROVAL_STATE_PATH regex's new [^/\s"']+/segment folds both./andprofiles/work/../work/` into the same net without sprouting a profile-specific branch.

Files Reviewed (2 files changed in increment)

• tools/approval.py - clean (ln rule matches sensitive target anywhere; description mirrors cp/mv/install; arbitrary lex-subsegment matcher under ~/.hermes/)
• tests/tools/test_approval.py - clean (symlink-source alias-write + ./ and profiles/work/../work/ regression added)

Previous review (commit 3a7f637)

Verdict: 1 Issue Found | Recommendation: Optional nitpick; safe to merge as-is

Overview

Severity	Count
🚨 critical	0
⚠️ warning	0
💡 suggestion	0
🤏 nitpick	1

Issue Details (click to expand)

File	Line	Roast
tools/approval.py	683	New ln rule description says "Hermes path" but _SENSITIVE_WRITE_TARGET also covers SSH/credential/shell-rc — the same class of lag the cp/mv/install rule was just fixed for, one line above.

---

🏆 Best part: Both previous Kilo nitpicks AND the three external findings (Gemini symlink bypass, Gemini container-path mismatch, Codex profile-qualified path) are all explicitly fixed in this single follow-up commit with surgical diffs — author even left breadcrumb replies on every thread pointing to 3a7f637b. That's the kind of responsiveness you usually only get from a CI bot with low self-esteem.

💀 Worst part: The ln rule at L683 is the cp/mv/install description bug from yesterday, teleported one line down. Same _SENSITIVE_WRITE_TARGET source-of-truth, freshly added "Hermes path" description. Fixing it is a 3-second edit; not fixing it just means the next reviewer gets to discover the same wormhole.

📊 Overall: The hard parts — closing the symlink bypass, making _check_sensitive_path slash-and-profile aware, splitting the test class into 9 idiomatic methods with description assertions — are all done correctly and without growing abstractions. One leftover string-lag; ship it or polish it, your call.

Files Reviewed (4 files)

• tests/tools/test_approval.py - clean (9-method split + description assertion)
• tests/tools/test_file_write_safety.py - clean (container/Windows/profile paths covered)
• tools/approval.py - 1 issue (ln rule description lag)
• tools/file_tools.py - clean (slash + profile suffix matching)

Correctness / Safety Findings

No correctness or safety findings. The ln rule gates symlinks into the existing _SENSITIVE_WRITE_TARGET set (credentials, SSH, shell-rc, Hermes), the file-tool guard now normalizes Windows backslashes and matches both suffix /.hermes/approval_requests.json and /.hermes/profiles/<name>/approval_requests.json, and the regex's optional profiles/[^/\s"']+/segment drops a profile name where one exists. The two-test split mirrorsTestHermesConfigWriteProtectiongranularity and addsassert desc` so future description rot trips the suite.

Ponytail Review

tools/file_tools.py:700-712: shrink — three different match strategies (== exact, endswith suffix, profile-tail length) live inline in _check_sensitive_path. They could fold into one 4-line def _is_approval_state_path(candidate, approval_state) that returns the boolean, leaving the gate function as if _is_approval_state_path(...). Cuts ~6 lines and isolates the match logic for unit testing. Optional — current shape is readable.

Otherwise: the optional profiles/<name>/ regex segment is the minimum that adds profile coverage without bloating _HERMES_APPROVAL_STATE_PATH. The ln rule mirrors the cp|mv|install rule structure on purpose (same _SENSITIVE_WRITE_TARGET group, parallel location) — good consistency.

Ponytail net: 0 lines.

Suggested Minimal Patch

1. tools/approval.py:683 — change description to "link file into sensitive credential/SSH/shell-rc/Hermes path" (mirror the cp/mv/install rule above).

Final Merge Guidance

Can merge. The single nitpick is a description-string consistency issue; the security behavior is correct on current main. Polish the description if already touching the file; otherwise ship it.

Fix these issues in Kilo Cloud

Previous review (commit 7530c74)

Verdict: 2 Issues Found | Recommendation: Approve (optional polish)

Overview

Severity	Count
🚨 critical	0
⚠️ warning	0
💡 suggestion	0
🤏 nitpick	2

Issue Details (click to expand)

File	Line	Roast
tools/approval.py	264 (impacts L682)	Adding _HERMES_APPROVAL_STATE_PATH to _SENSITIVE_WRITE_TARGET quietly obsoleted the cp|mv|install rule's "credential/SSH/shell-rc path" description — now users see a misleading reason string when approval fires on the state file.
tests/tools/test_approval.py	589	TestHermesApprovalStateWriteProtection packs 5 commands into one method while its sibling TestHermesConfigWriteProtection enumerates 20+ idioms across separate methods — coverage is uneven for a security-critical surface.

---

🏆 Best part: The fix is small, surgical, and uses the existing pattern plumbing — a new _HERMES_APPROVAL_STATE_PATH regex plugged into _SENSITIVE_WRITE_TARGET picks up the tee, >, and cp\|mv\|install consumers for free, the file-tools side adds the matching write_file_tool/patch_tool deny via _check_sensitive_path, and the sed/perl/ruby long-flag forms get paired coverage. It is exactly what the previous round of NousResearch#14639 reviews should have asked for, and it's done without growing any abstractions. Both halves (Python tool surface + terminal pattern surface) are gated, so an attacker has no unpaired door to walk through.

💀 Worst part: The cp\|mv\|install description at tools/approval.py:682 is now a relic. A user approving cp forged.json ~/.hermes/approval_requests.json will see "copy/move file into sensitive credential/SSH/shell-rc path" — confusing at best, and it means the test suite will not catch a future description-string rot because no test asserts desc for these cases.

📊 Overall: A focused, well-tested security follow-up that adds ~59 lines and removes a real attack surface. The two findings are polish — leave them if you want to ship, fix them if you're already in the file.

Files Reviewed (4 files)

• tests/tools/test_approval.py - 1 issue (test granularity)
• tests/tools/test_file_write_safety.py - clean
• tools/approval.py - 1 issue (stale description via _SENSITIVE_WRITE_TARGET widening)
• tools/file_tools.py - clean

---

Correctness / Safety Findings

No correctness or safety findings. The new pattern is correctly included in _SENSITIVE_WRITE_TARGET, the sed -i / sed --in-place / perl -i / ruby -i rules, and the file_tools _check_sensitive_path gate. _rewrite_resolved_hermes_home already folds the resolved-absolute active home to ~/.hermes/, so the absolute-path test exercises that path. Path-tail [^\s"']* after _SENSITIVE_WRITE_TARGET correctly handles the case where the destination filename extends past approval_requests.json\b (e.g., a backup sibling stays safe).

Ponytail Review

tools/file_tools.py:692-699: reuse — the inline try/except computing approval_state from get_hermes_home() / "approval_requests.json" mirrors the cached _get_hermes_config_resolved() pattern at tools/file_tools.py:643-659. Extract a sibling _get_hermes_approval_state_resolved() (cache-once, fallback to Path(_expand_tilde("~/.hermes/approval_requests.json"))) so both gates share the resolution strategy. Ponytail net: 0 lines (same code, cleaner reuse).

Otherwise: the new regex _HERMES_APPROVAL_STATE_PATH (three-form alternation ~/ / $HOME/ / $HERMES_HOME/) is the cheapest correct shape — collapsing it would lose form coverage. The approval-module side correctly folds approval state into the existing _SENSITIVE_WRITE_TARGET group instead of adding parallel rules, which is the right call. The file_tools gate is a flat equality check, not an over-engineered helper.

Ponytail net: 0 lines.

Suggested Minimal Patch

1. tools/approval.py:682 — extend the description string to include Hermes paths (see inline suggestion).
2. tests/tools/test_approval.py:589 — split test_terminal_writes_require_approval into per-idiom methods matching TestHermesConfigWriteProtection and add mv/install/ruby -i/sed --in-place/tilde-form tee cases.

Final Merge Guidance

Can merge. Both findings are polish — the security behavior is correct on current main and the existing tests pass. Fix the description and test granularity if you're already touching the file; otherwise ship it.

---

Fix these issues in Kilo Cloud

---

_{Reviewed by minimax-m3 · Input: 53.8K · Output: 14.9K · Cached: 569K}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3a7f637b97

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2ae9a8cfad

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".