fix: correct format command detection and questions JSON errors by ZuyiZhou · Pull Request #714 · EverMind-AI/Raven

ZuyiZhou · 2026-09-23T08:56:15Z

Summary

Fix two misleading tool refusals:

Recognize the format executable through shell token parsing. URL parameters such as &format=json and quoted argument text no longer trigger a hard denial, while format c: remains denied with a disk-formatting reason. The executable is matched as format, format.exe or format.com (case-insensitive), so the Windows .com spelling the old regex caught is still refused.
Report malformed questions JSON with the parser message, line, column, and character offset. Cover both direct ask_user execution and registry normalization, while keeping display previews tolerant of malformed input.

Type

Verification

Rebased: head f5a7841 on origin/main 8ee5ae0, without conflicts. git range-diff shows every commit replayed unchanged. The second commit adds format.com to the hard-deny set; its two new cases fail with the source change reverted. The branch's own test files rerun at this head: 103 passed. Numbers elsewhere in this description are from earlier heads.

uv run pytest tests/test_shell_policy_reasons.py tests/test_ask_user_tool.py -q - 101 passed.
uv run pytest tests/test_permissions_gate.py tests/test_shell_approval.py tests/test_shell_comments.py tests/test_shell_file_removals.py tests/test_tool_registry_execute.py -q - 570 passed.
uv run ruff check raven/agent/tools/ask_user.py raven/permissions/builtin.py raven/permissions/shell_policy.py tests/test_ask_user_tool.py tests/test_shell_policy_reasons.py - passed.
uv run ruff format --check raven/agent/tools/ask_user.py raven/permissions/builtin.py raven/permissions/shell_policy.py tests/test_ask_user_tool.py tests/test_shell_policy_reasons.py - all five files formatted.
git diff --check - passed.
Relevant tests pass locally
Relevant lint / type checks pass locally
User-facing docs or screenshots are updated when needed

No documentation or screenshot changes are needed for these validation fixes. The lint checkbox reflects Ruff checks; no separate type check was run.

Risk

Security impact considered
Backward compatibility considered
Rollback path is clear for risky changes

Real format commands remain unconditionally denied, including compound commands and recognized wrappers. Matching also covers format.exe and case variants. Operator-supplied deny patterns retain their existing semantics. Valid questions and empty-question errors retain their behavior; malformed JSON now returns actionable diagnostics before asking the user. Revert this change to restore the previous behavior.

Related Issues

N/A

gloryfromca

Blocking: the replacement format matcher lets format.com bypass the hard deny.

I reviewed the full diff and surrounding call paths, including ToolRegistry error containment, shell tokenization and wrapper handling, the permission gate's reason mapping, and the relevant history. I also checked the repository rules in AGENTS.md, CLAUDE.md, and CONTEXT-MAP.md, backward compatibility, and whether the tests were weakened; the changed tests strengthen the expected error and safety behavior.

Verification: uv run pytest tests/test_shell_policy*.py tests/test_ask_user_tool.py -x passed all 101 tests. The missing format.com case is not covered and reproduces as PolicyOutcome(decision=ALLOW, reason_code='').

gloryfromca

Blocking: the existing format.com safety regression must be fixed.

This revision is a rebase with an equivalent PR patch (git range-diff reports the old and new commits as unchanged), so the existing inline finding still applies and remains open; I did not duplicate it. On this head, both format.com c: and FORMAT.COM C: still classify as ALLOW, while format and format.exe are denied.

I rechecked the full PR diff, the shell-policy and permission-gate call path, the ask-user registry/direct paths, relevant history and backward compatibility, repository rules and architecture terms, and the test changes for weakening; I found no additional issues. Verification: uv run pytest tests/test_ask_user_tool.py tests/test_shell_policy_reasons.py -x passed all 101 tests.

…al groups (#735) ## Summary Two tests from #712 fail on `main` and on every PR rebased onto it (#703, #714, #719 and #723 all fail the same `unit` shard). They fail for two unrelated reasons, one of which is in the production code. - **`_stop_child` spent its SIGTERM round on a group it had already SIGKILLed** (`raven/cli/serve_commands.py`). When the gateway ignored SIGTERM past `_CHILD_STOP_S`, the whole group was SIGKILLed, and then the code still sent the group SIGTERM and polled `_group_gone` against the original deadline, which by then had passed. A member the gateway orphaned stays in the group as a zombie until init reaps it, so that first probe found the group present, gave up at once, printed "the gateway's own processes ignored SIGTERM; killing them" and sent a second SIGKILL. On a CI runner the reap is slow enough to lose that race every time, which is what `test_a_stubborn_gateway_is_killed_with_its_group_in_one_step` saw. A group killed here now gets the `_KILL_WAIT_S` wait and returns; the SIGTERM round remains for a gateway that stopped on its own or was already gone. - **`TestStopping` could signal a real process group** (`tests/test_cli_serve_commands.py`). Those tests use made-up pids 111 and 222 and fake `os.kill`, but `_force_kill` still called the real `os.getpgid` and `os.killpg`. On the runner that failed `main`, one of those pids was a live process leading its own group, so `killpg` raised `PermissionError`, `_stop_resident` reported it as a warning and skipped that process's kill wait: `45.05 >= 2 * 25.0` in `test_both_processes_it_names_waited_the_time_it_reports`. Had the pid belonged to the test's own user, the test would have SIGKILLed that group. An autouse fixture on the class now fakes `getpgid` as "no such process" and fails the test on any `killpg` the test did not arrange; the tests that exercise groups already install their own fakes over it. ## Type - [x] Fix - [ ] Feature - [ ] Docs - [ ] CI / tooling - [ ] Refactor - [ ] Other ## Verification Head f221828, on origin/main afdc04f. ``` COLORTERM=truecolor uv run --frozen --python 3.12 --all-extras pytest -q -p no:cacheprovider tests/test_cli_serve_commands.py 122 passed COLORTERM=truecolor uv run --frozen --python 3.12 --all-extras pytest -q -p no:cacheprovider 25901 passed, 109 skipped, 1 failed: tests/test_rpc_files.py::test_a_host_without_libreoffice_says_so, which fails on origin/main on this machine too (LibreOffice is installed here; CI has none) uv run --frozen --python 3.12 --extra dev ruff check raven tests / ruff format --check make lint-imports lint-deps lint-types / git diff --check origin/main...HEAD PYTHONPATH=. uv run --frozen --python 3.12 --extra dev python scripts/check_source_language.py origin/main...HEAD make check-commits / COMMIT_RANGE=origin/main...HEAD make check-large-files all clean ``` The zombie cannot be produced on macOS (launchd reaps an orphan at once), so the new test `test_a_killed_group_still_being_reaped_is_waited_out_not_reported` stands in for it: a real gateway that ignores SIGTERM, with the first group probe after the SIGKILL reporting the group present. With `raven/cli/serve_commands.py` restored to origin/main it fails (the "own processes" line is printed); with this change it passes. The fixture's case was reproduced by making the fake pid 111 behave as a foreign group leader (`getpgid` returns the pid, `killpg` raises `PermissionError`): `test_both_processes_it_names_waited_the_time_it_reports` then fails with `assert 45.1 >= 50.0`, the shape of the `main` failure. With the fixture as committed it passes. - [x] Relevant tests pass locally - [x] Relevant lint / type checks pass locally - [ ] User-facing docs or screenshots are updated when needed No user-facing docs: the stop's interface and budgets are unchanged. ## Risk User-visible: a stopping supervisor whose gateway had to be killed no longer prints the second "own processes ignored SIGTERM" line or sends a second SIGKILL; it waits up to `_KILL_WAIT_S` (5s) for the killed group to be reaped, where it used to wait up to the same 5s after the second SIGKILL. The stop's total budget is unchanged. The test fixture changes no production behaviour. Rollback: revert the squash commit. - [x] Security impact considered - [x] Backward compatibility considered - [x] Rollback path is clear for risky changes ## Related Issues N/A Co-authored-by: Claude (claude-opus-5-5) <noreply@anthropic.com>

gloryfromca

No blockers; this can merge as far as I am concerned.

The format.com fix closes the prior safety regression without restoring the URL and quoted-text false positives, and the regression cases cover both ordinary and case-insensitive invocation. I reviewed the full PR diff and new delta, shell-policy and permission-gate callers, ask-user registry/direct paths, relevant history and backward compatibility, repository rules in AGENTS.md/CLAUDE.md/CONTEXT*.md, architecture boundaries, and the test changes for weakening; I found no additional issues.

Verification: uv run pytest tests/test_ask_user_tool.py tests/test_shell_policy_reasons.py -x passed all 103 tests. Direct classification checks also confirmed the two format.com commands are hard-denied and the negative controls remain allowed. The original thread has a closing reply and is resolved.

gloryfromca

No blockers; this can merge as far as I am concerned.

This head is a base refresh: git range-diff reports both PR commits as patch-equivalent to the previously reviewed clean revision. I rechecked the full diff, affected callers and history, backward compatibility, repository rules and architecture constraints, and test strength; no new issue was introduced, and the settled safety thread remains resolved.

Verification: uv run pytest tests/test_ask_user_tool.py tests/test_shell_policy_reasons.py -x passed all 103 tests.

Match format executables through shell token parsing to avoid rejecting URL parameters and quoted text. Report malformed questions JSON with its parse error and location in direct and registry calls. Co-authored-by: OpenAI Codex <noreply@openai.com>

The token matcher this branch introduced covers `format` and `format.exe` but not `format.com`, the same Windows formatter under its .com name. `format.com c:` classified as ALLOW, so the disk-format guard was bypassable through the alias -- and the pattern it replaced matched that spelling. `BuiltinRulings().ruling()` returned None for it too, so no refusal was reported either. Both names now hard-deny with reason `disk_format`, and the regression cases cover `format.com` and `FORMAT.COM`. Verified by reverting the source change alone: the two new cases fail without it and pass with it, while the URL and quoted-text cases the branch added stay allowed. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>

gloryfromca

No blockers; this can merge as far as I am concerned.

git range-diff shows both PR commits remain patch-equivalent after this base refresh. I rechecked the complete diff and affected callers against the updated base, including repository rules, architecture constraints, backward compatibility, history, and test strength; nothing changes the prior clean assessment, and the settled thread remains resolved.

Verification: uv run pytest tests/test_ask_user_tool.py tests/test_shell_policy_reasons.py -x passed all 103 tests.

ZuyiZhou requested a review from LivXue as a code owner September 23, 2026 08:56