feat(approvals): persist unresolved requests

[9thLevelSoftware]

Persist unresolved approval requests across restart

Part of the 2026-07-13 Hermes capability review.

What this PR does

Persists the existing request-ID-based fallback approval state machine to approval_requests.json under the active profile. Pending and resolved requests reload lazily after restart, retain scope/expiry/outcome, and remain bound to their existing argument hash.

Safety

• File mode is 0600.
• Raw tool arguments, shell commands, and code are not persisted.
• Only the existing safe identity, expiry, resolution, and display metadata are stored.
• Existing approval gates and defaults are unchanged; this adds no new human prompt.

Implementation

• Reuses the current _pending / _pending_by_session state machine.
• Reuses utils.atomic_json_write; no database or dependency added.
• Persists before returning from submit/resolve/consume paths.
• Keeps profile state isolated by the active Hermes home.

Verification

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

Baseline note: the broader tests/tools/test_approval.py currently has one unrelated failure on origin/main (test_nonrecursive_verification_artifact_cleanup_is_not_dangerous); this branch does not touch that path.

Out of scope

• No persistence of session YOLO state.
• No persistence of live callback/threading events; only restart-safe fallback requests.
• No new approval mode or policy.

[gemini-code-assist]

Code Review

This pull request introduces persistence for unresolved fallback approvals by saving them to a JSON file (approval_requests.json) and loading them upon initialization. Key changes include adding _load_pending_locked and _persist_pending_locked helper functions, updating pruning and resolution logic to trigger persistence, and updating the test suite to verify that resolved requests survive process restarts. The review feedback highlights three main improvement opportunities: validating that the loaded requests field is indeed a list to prevent potential TypeError crashes, preventing duplicate request_id entries when rebuilding the session index, and replacing unnecessary copy.deepcopy calls with a shallow copy to reduce performance overhead.

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.

[kilo-code-bot]

Code Review Roast 🔥

Verdict: No Issues Found | Recommendation: Merge

Well. I warmed up the flamethrower, laid out the previous review's list of grievances — the corrupted-JSON crash, the duplicate-ID ghost entries, the pointless per-field deepcopy, the clear_session that clung to persisted requests like an ex who won't move out, and the missing pending-survival test — and every single one got quietly handled in 9eb7073b2. Rude. I had material.

What got fixed

File	Line	Was	Now
tools/approval.py	1574-1576	for request in payload.get("requests", []) blew up on a non-list	isinstance(requests, list) guard — malformed JSON loads as empty
tools/approval.py	1582	duplicate IDs double-appended to the session index	request_id in _pending dedup on load
tools/approval.py	1601	copy.deepcopy(value) per safe field	shallow projection — atomic_json_write re-serializes anyway
tools/approval.py	2027-2032	clear_session never touched durable state	_prune_pending_locked() loads first, _persist_pending_locked() writes after — cleared sessions actually stay cleared
tests/tools/test_approval_fallback_identity.py	139+	pending-survival shipped on trust	test_pending_request_survives_process_restart, test_clear_session_removes_persisted_requests, test_malformed_requests_collection_loads_as_empty

---

🏆 Best part: clear_session now calls _prune_pending_locked() (which loads durable state) before popping the in-memory index and persists after — so a session cleared in one process is genuinely gone on the next boot, not resurrected from disk. The three new tests each simulate a restart by resetting _pending_loaded_home, and one of them feeds {"requests": 7} straight at the loader to prove the guard holds. That's testing the actual failure shape, not the mock. I'm almost disappointed.

💀 Worst part: The lazy from hermes_constants import get_hermes_home inside _pending_path() still runs on every call — but it's a nitpick, it's not new to this diff, and profile-timing makes a top-level import genuinely debatable. Not worth the flame.

📊 Overall: Like a to-do list where someone actually crossed everything off before the standup — suspicious, but I'll take it.

Correctness / Safety Findings

No correctness or safety findings. Every issue from the previous pass (list validation, duplicate-ID dedup, dropped deepcopy, durable clear_session, pending-survival coverage) is resolved on current HEAD.

Ponytail Review

Ponytail: Lean already. Ship. The diff removed the deepcopy overhead and expressed the fixes as minimal guards — no new abstractions, no speculative config, no one-implementation interfaces.

Ponytail net: 0 lines.

Suggested Minimal Patch

No patch needed.

Final Merge Guidance

Can merge as-is.

Files Reviewed (2 files)

• tools/approval.py - 0 issues (4 prior findings resolved)
• tests/tools/test_approval_fallback_identity.py - 0 issues (3 new restart/malformed tests added)

Previous Review Summary (commit 79e9234)

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

Previous review (commit 79e9234)

Verdict: 1 Issue Found | Recommendation: Address test gap before merge

Overview

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

Issue Details (click to expand)

File	Line	Roast
tests/tools/test_approval_fallback_identity.py	118	The new test exercises "resolved survives restart" but the PR headline promises both pending and resolved survive restart — the pending half is shipping on trust, not coverage.

---

🏆 Best part: The _load_pending_locked guard using str(path.parent) for the once-per-home cache is genuinely tasteful — it sidesteps the classic "did I already load for this profile" trap without dragging in a state machine. And the _pending_loaded_home reset pattern in _clear_state is a clean way to simulate process restart in tests without spawning a subprocess.

💀 Worst part: The safe_fields allowlist at tools/approval.py:1590 hardcodes 19 field names as a Python set, but submit_pending at tools/approval.py:1870-1885 mutates the request via request.update({...}) — any new field added to submit's update dict without a matching entry in safe_fields will silently disappear from disk on the next persist. That's a foot-gun shaped exactly like the bug it's trying to prevent.

📊 Overall: Like a chef who seasons the steak but forgets to salt the fries — the main course is solid, the garnish has a gap.

Correctness / Safety Findings

No correctness or safety findings beyond the three already raised by gemini-code-assist (requests must be validated as a list before iteration; duplicate request_id dedup on load; deepcopy per-field is unnecessary overhead in _persist_pending_locked). All three are valid and worth applying before merge.

Ponytail Review

• tests/tools/test_approval_fallback_identity.py:118: test-shrink — the test does extra work asserting "rm -rf" not in persisted (good PII-safety check) but skips the actual headline behavior. Collapse to one behavior assertion and add a sibling test for pending survival; the PII assertion can stay as a one-liner in the same test.
• tools/approval.py:1597-1601: native — copy.deepcopy(value) per safe field is replaced by a shallow {key: value for key, value in request.items() if key in safe_fields}; values are primitives or pattern_keys (a list of strings) and atomic_json_write will re-serialize immediately.
• tools/approval.py:1574: native — payload.get("requests", []) should be hoisted into a local requests variable with an isinstance(requests, list) guard, replacing the unbounded for request in payload.get("requests", []): form. Fixes a real crash on corrupted JSON.
• tools/approval.py:1581: delete — the _pending[request_id] = request plus unconditional _pending_by_session.setdefault(session_key, []).append(request_id) should become if request_id not in _pending: wrapping both; without it, duplicate IDs in the file duplicate the session-index entry and FIFO resolution starts resolving ghosts.
• tools/approval.py:1553-1556: native — _pending_path does a lazy from hermes_constants import get_hermes_home on every call. Move the import to the top of the file (the module already imports from hermes_constants indirectly elsewhere); saves the per-call import lookup and is the standard pattern.

Ponytail net: -8 lines.

Suggested Minimal Patch

1. Apply the three gemini suggestions (list-check, dedup, drop deepcopy).
2. Add a sibling test test_pending_request_survives_process_restart mirroring the resolved variant but asserting restored["status"] == "pending" instead of "resolved".
3. (Optional) Hoist the from hermes_constants import get_hermes_home import out of _pending_path.

Final Merge Guidance

Can merge after the pending-survival test is added and the three gemini suggestions are applied. The persistence implementation is sound; the gaps are coverage and defensive validation, not correctness.

---

Fix these issues in Kilo Cloud

---

_{Reviewed by minimax-m3 · Input: 67.4K · Output: 6.1K · Cached: 625.7K}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 79e9234a9b

ℹ️ 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".