fix(runtime): remediate drift gate blockers - #27783
solovision24 wants to merge 1 commit into
Conversation
be012a1 to
71c047d
Compare
|
Dev rebase/verification update for Kanban t_499a9596.
Merge attempts from |
|
Thanks for the focused remediation work here. The overall direction looks salvageable, but I found one concrete regression in the PR diff. Problems
Suggested changes
This is an automated hermes-sweeper review. |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused runtime-gate remediation. The inline-shell process-group cleanup addresses a behavior still present on current main (agent/skill_preprocessing.py:73-81), but the Popen conversion needs to preserve two current-main invariants.
Problems
agent/skill_preprocessing.py:87in PR head71c047d8b20f1303f17c2369cad2e8c95da1b655callssubprocess.Popenwithoutstdin=subprocess.DEVNULL.scripts/check_subprocess_stdin.py:9-11requires this foragent/subprocesses, and current main already protects this call atagent/skill_preprocessing.py:80(commitd1f23bb2d).- The new kwargs also omit current main's Windows
creationflags=windows_hide_flags()handling atagent/skill_preprocessing.py:71,81.tests/test_windows_subprocess_no_window_flags.py:286-301covers this behavior and currently mockssubprocess.run, so it must be updated for the Popen path.
Suggested changes
- Add
stdin=subprocess.DEVNULLand preserve the existing Windows creation flags inpopen_kwargs, alongside the POSIX process-group setup. - Update the Windows regression test to mock
Popenand assert the creation flags; include the subprocess-stdin guard in verification.
This is an automated hermes-sweeper review.
| timeout=max(1, int(timeout)), | ||
| check=False, | ||
| ) | ||
| proc = subprocess.Popen(["bash", "-c", command], **popen_kwargs) |
There was a problem hiding this comment.
Please preserve current main's subprocess invariants here: add stdin=subprocess.DEVNULL (required by scripts/check_subprocess_stdin.py for agent/ calls) and carry forward the existing Windows creationflags=windows_hide_flags() behavior. The current Windows regression test mocks subprocess.run, so update it to exercise this Popen path too.
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Summary
Two PRs address or reference this issue complex. #13525 implemented the ACP approval-routing and per-thread callback fix, while #27783 updates its regression coverage and fixes separate inline-shell timeout and Kanban test-hermeticity drift blockers.
Related pull requests
- #13525 [merged]
related— (+236/-20) — merged reference implementation: wires ACP sessions into the interactive approval path and replaces process-global approval and sudo callbacks with thread-local storage, directly addressing both the unused callback and concurrent-session collision causes; it remains relevant as the implementation whose regression test #27783 adjusts. - #27783
related— (+77/-17) — keep open pending fixes: the diff correctly replaces the obsolete hardline ACP fixture, preserves explicit hardline blocking coverage, adds process-group cleanup for timed-out inline shell commands, and makes Kanban liveness tests more hermetic. Consistent with the keep_open review on #27783, its Popen conversion currently drops stdin=subprocess.DEVNULL and current-main Windows creation flags, so those invariants and their guard tests must be restored before merge.
Suggested consolidation
Keep #13525 as the merged reference implementation and keep #27783 open for consolidation of the remaining drift-gate remediations; merge #27783 only after restoring stdin=subprocess.DEVNULL, preserving Windows creation flags, updating the affected Popen regression coverage, and rerunning the subprocess-stdin, Windows, ACP, inline-shell, and Kanban gates. The PRs are complementary rather than duplicates, so neither should be closed as a duplicate.
Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 21 kB of PR diffs, 4 kB of issue/PR text, 5 kB of discussion (5 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.
Remediates the three Hermes origin/main drift gate blockers found during SoLoVision live fast-forward review.\n\nChanges:\n- Use a non-hardline dangerous command in the ACP approval isolation test and assert hardline commands still bypass callbacks and block unconditionally.\n- Make inline skill shell timeout handling kill the process group and return the timeout marker without dropping the rest of the skill message.\n- Make Kanban PID/liveness tests hermetic under the live-system guard, with a narrow bypass only for the real child zombie signal test.\n\nVerification:\n- python -m pytest tests/acp/test_approval_isolation.py::TestAcpExecAskGate::test_interactive_env_var_routes_to_callback tests/agent/test_skill_commands.py::TestInlineShellExpansion::test_inline_shell_timeout_does_not_break_message tests/hermes_cli/test_kanban_core_functionality.py::test_pid_alive_helper tests/hermes_cli/test_kanban_core_functionality.py::test_pid_alive_detects_zombie tests/hermes_cli/test_kanban_core_functionality.py::test_detect_crashed_workers_increments_counter -q -o 'addopts=' → 5 passed in 2.90s\n- python -m pytest tests/acp/test_approval_isolation.py tests/agent/test_skill_commands.py tests/hermes_cli/test_kanban_core_functionality.py -q -o 'addopts=' → 201 passed in 6.09s