Repository navigation
fix(gateway): authoritative restart-initiator breadcrumb for F2 breaker - #80
Merged
Merged
Conversation
The F2 self-completing-restart-loop breaker (#70/#72) detects which session initiated a restart via C1, a command-string heuristic that matches the literal `safe-restart.py` in a terminal tool call. That silently false-NEGATIVES if the skill is invoked via an alias/wrapper/rename — the breaker quietly stops arming for that path, discovered only during the cascade it exists to prevent. Replace the heuristic's authority with a self-declared breadcrumb: safe-restart.py (which runs synchronously in the initiating turn and knows its own session key) drops a per-session, per-boot file in HERMES_HOME; the clean-turn gate consumes it (read+unlink) independent of how the command was spelled. C1/F1 retained as defense-in-depth fallback. - boot_id (pid:psutil.create_time) gates cross-boot crumbs → kills the false-trip vector where an interrupted initiator's crumb marks the next real-work turn. Gateway is the SOLE boot_id producer (writes it to gateway_state.json); the script copies the string verbatim — no parser drift (macOS /proc returns None, so the prior start_time field was pid-only; psutil is cross-platform). - Per-session files → no shared-list RMW race / lost-update. - Always-consume (no `flag or consume()` short-circuit) so a C1-flagged turn can't leak its crumb. Startup sweep + finally-consume bound any leak. - 16 new tests (RED-proven), incl. a real-subprocess round-trip proving detection works when the gateway never sees the command line. Spec: 2026-06-22_f2-initiator-detection-authoritative-breadcrumb.md (2 Opus passes + Phase-0 live probe). Parent: #72.
…fail-safe Final diff review (2 Opus reviewers, both BLOCK) found the feature was inert after restart #1 and the tests masked it. Fixes: - BLOCKER-1: write_runtime_status now refreshes boot_id on EVERY write (status.py). gateway_state.json survives reboots, so the prior boot's boot_id persisted while pid updated → the gate's boot check mismatched every breadcrumb → feature silently inert. One-line producer fix restores "single producer writes the current boot's id." - BLOCKER-2: round-trip tests now drive the REAL write_runtime_status producer with a stale prior-boot file (stale_first=True), not a hand-seeded state file that masked the bug. New test_real_producer_refreshes_boot_id_across_restart is RED against the pre-fix producer. - MAJOR-1: consume side rejects a degraded pid-only boot_id ("1234:") on either side — can't prove same-boot, fall back to C1/F1 (pid reuse defense). - MINOR-1: honest comment on the I-8 hash-back (integrity vs the 0700-dir authZ). - MINOR-2: --no-spawn no longer plants a breadcrumb unless --write-breadcrumb is also passed → a diagnostic run under a live gateway can't inject a mark. All 42 cascade tests + 135 blast-radius green; BLOCKER-1 + MAJOR-1 RED-proven.
…metry, CI-safe contract Second diff review (2 Opus reviewers, both APPROVE-WITH-CHANGES; all 5 prior BLOCK/findings verified CLOSED). Non-blocking pre-merge items addressed: - _compute_boot_id logs a WARNING when it falls back to pid-only (psutil failure) so the otherwise-silent fail-closed-inert state is observable (both reviewers flagged HIGH). - Sweep applies the same degraded-boot_id short-circuit as the consume side (symmetry): a degraded current boot reaps all crumbs instead of trusting a same-pid match. + test_sweep_degraded_boot_reaps_all (RED-proven). - CI-safe cross-process contract test (test_cross_process_breadcrumb_contract_ci_safe): the real-script round-trip tests pytest.skip off-host, so the anti-masking proof could vacuum on CI. This replicates the script's breadcrumb-write contract inline (real producer + real gate, no skill-script dependency) so the gate side of the cross-process contract runs on every host. - safe-restart.py: unique tmp suffix (pid+uuid) for the breadcrumb atomic write (no same-session concurrent-restart race); dead None branch removed from --print-home. 44 cascade + 137 blast-radius + 25 skill green; sweep symmetry RED-proven.
Kyzcreig
enabled auto-merge (squash)
June 22, 2026 05:44
🔎 Lint report:
|
| Rule | Count |
|---|---|
unresolved-attribute |
2 |
unresolved-import |
1 |
First entries
run_agent.py:2940: [unresolved-attribute] unresolved-attribute: Object of type `Self@get_credits_spent_micros` has no attribute `_credits_session_start_micros`
gateway/status.py:226: [unresolved-import] unresolved-import: Cannot resolve imported module `psutil`
tests/run_agent/test_credits_notices_toggle.py:76: [unresolved-attribute] unresolved-attribute: Unresolved attribute `_credits_session_start_micros` on type `AIAgent`
✅ Fixed issues (1):
| Rule | Count |
|---|---|
invalid-assignment |
1 |
First entries
tests/run_agent/test_credits_notices_toggle.py:76: [invalid-assignment] invalid-assignment: Object of type `None` is not assignable to attribute `_credits_session_start_micros` of type `int`
Unchanged: 5899 pre-existing issues carried over.
Diagnostics are surfaced as warnings — this check never fails the build.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+688
to
+691
|
|
||
|
|
||
| def test_current_boot_breadcrumb_survives_sweep(tmp_path, monkeypatch): | ||
| """Pass-2 RC-4: a fresh current-boot crumb (an in-flight initiator) must |
There was a problem hiding this comment.
Four subprocess round-trip tests embed /Users/alexgierczyk/.hermes/... — a path unique to one developer's filesystem. Every other machine and CI silently skips these tests, which means the key cross-process contract guarantee (that the real script's breadcrumb write shape matches what the gate expects) is only ever exercised locally. The test_cross_process_breadcrumb_contract_ci_safe test covers the gate side in CI, but cannot catch drift in the real script. Consider resolving the script path via an environment variable (e.g. os.environ.get("HERMES_SAFE_RESTART_SCRIPT")) with a fallback to HERMES_HOME / "skills-shared/..." so the tests run wherever the skill is installed.
Comment on lines
+611
to
+631
| runner._apply_post_turn_resume_gate(sk) | ||
| counts = runner._load_restart_failure_counts().get(sk, {}) | ||
| assert len(counts.get("replay_marks", [])) <= 1 | ||
|
|
||
|
|
||
| def test_breadcrumb_and_c1_flag_same_turn_records_one_mark(tmp_path, monkeypatch): | ||
| """I-1: with BOTH the C1/F1 flag and a breadcrumb present for one turn, the | ||
| gate records exactly ONE mark (not two → would trip the breaker early).""" | ||
| monkeypatch.setenv("HERMES_RESTART_LOOP_THRESHOLD", "3") | ||
| runner, _adapter = _runner(tmp_path, monkeypatch) | ||
| sk = _entry(runner, "both").session_key | ||
| runner._resumed_this_boot.add(sk) | ||
| runner._session_initiated_restart[sk] = True | ||
| _write_breadcrumb(runner, sk) | ||
| runner._apply_post_turn_resume_gate(sk) | ||
| counts = runner._load_restart_failure_counts().get(sk, {}) | ||
| assert len(counts.get("replay_marks", [])) == 1 | ||
|
|
||
|
|
||
| def test_c1_flag_true_still_consumes_breadcrumb(tmp_path, monkeypatch): | ||
| """Pass-2 B-3: a C1/F1-flagged turn must STILL unlink the breadcrumb (no |
There was a problem hiding this comment.
Both test_single_gate_call_site and test_finally_consume_present_in_handler use inspect.getsource + string matching against the live source text. test_single_gate_call_site counts the literal self._apply_post_turn_resume_gate(session_key) — a rename of the local variable from session_key to any other name would make the count zero and the test would incorrectly report a gate-call regression. test_finally_consume_present_in_handler checks for _consume_restart_initiated_breadcrumb(_sk_cleanup) verbatim, so renaming _sk_cleanup silently breaks the structural guarantee the test is meant to enforce without the test failing. These tests are also susceptible to false positives from matching inside comments or docstrings.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
+605
to
+615
| def _restart_initiated_filename(session_key: str) -> str: | ||
| """Per-session breadcrumb filename = sha8 of the session key. | ||
|
|
||
| Per-session files mean the writer (script) and the consumer (gateway) never | ||
| touch the same inode → no read-modify-write race / lost-update (a shared | ||
| list would have one). The full key is also stored INSIDE the file and must | ||
| hash back to this name (anti-forgery, I-8). | ||
| """ | ||
| import hashlib | ||
|
|
||
| return hashlib.sha256((session_key or "").encode("utf-8")).hexdigest()[:8] |
There was a problem hiding this comment.
import hashlib is deferred inside the function body, which is non-idiomatic Python. Since hashlib is part of the standard library and already available at module import time, moving it to the top-level import block avoids the per-call attribute lookup on sys.modules and keeps imports in their conventional location.
Suggested change
| def _restart_initiated_filename(session_key: str) -> str: | |
| """Per-session breadcrumb filename = sha8 of the session key. | |
| Per-session files mean the writer (script) and the consumer (gateway) never | |
| touch the same inode → no read-modify-write race / lost-update (a shared | |
| list would have one). The full key is also stored INSIDE the file and must | |
| hash back to this name (anti-forgery, I-8). | |
| """ | |
| import hashlib | |
| return hashlib.sha256((session_key or "").encode("utf-8")).hexdigest()[:8] | |
| def _restart_initiated_filename(session_key: str) -> str: | |
| """Per-session breadcrumb filename = sha8 of the session key. | |
| Per-session files mean the writer (script) and the consumer (gateway) never | |
| touch the same inode → no read-modify-write race / lost-update (a shared | |
| list would have one). The full key is also stored INSIDE the file and must | |
| hash back to this name (anti-forgery, I-8). | |
| """ | |
| return hashlib.sha256((session_key or "").encode("utf-8")).hexdigest()[:8] |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Kyzcreig
added a commit
that referenced
this pull request
Jun 22, 2026
… gate, watcher guardrail) (#86) * feat(gateway): F2 breadcrumb backlog cleanup — config family, contract gate, no-regression guard Closes the three residuals #80 logged (spec 2026-06-22_f2-breadcrumb-backlog-cleanup): - Item 3 (config consistency): surface the whole restart_* family (restart_loop_threshold/window_secs/initiated_ttl_secs) in the config defaults dict with doc comments, and add the missing restart_initiated_ttl_secs config→env bridge. Extracted the inline agent-config bridge into _bridge_agent_config_to_env + _AGENT_CONFIG_ENV_BRIDGE (single-sourced, unit-testable) — config UNCONDITIONALLY wins over a pre-set env var, matching the shipped siblings (PR NousResearch#18413). - Item 2 (contract drift): a # F2 BREADCRUMB CONTRACT block as the single documented source of truth + an always-on frozen-literal conformance test over a key vector (the skill has a mirror test in hermes-home so drift in EITHER repo reddens its own CI — no cross-repo skip-no-op). - I-1 no-regression: source-guard test asserting the shipped #80 detection bodies are unchanged; this PR adds config/tests/docs only — zero behavior change to the breadcrumb path. Tests: 53 cascade (9 new) + 246 blast-radius green; bridge, config-wins, conformance (dirname + hash skew), and source-guard all RED-proven. Skill side (watcher guardrail + mirror conformance) lands in hermes-home. Spec reviewed: 2x Opus APPROVE-WITH-CHANGES (precedence corrected from ground-truth, conformance gate de-no-op'd). * test: address Greptile review — drop duplicate imports, assert bridge-map membership --------- Co-authored-by: Kyzcreig <9063726+Kyzcreig@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Make the F2 self-completing-restart-loop breaker's restart-initiator detection authoritative, closing the documented BACKLOG false-negative from #72.
The breaker (#70/#72) must know which session initiated a restart. Today that's C1 — a command-string heuristic matching the literal
safe-restart.pyin aterminaltool call. It silently false-negatives if the skill is invoked via an alias/wrapper/rename: the breaker quietly stops arming for that path, discovered only during the cascade it exists to prevent.This replaces the heuristic's authority with a self-declared breadcrumb:
safe-restart.py(which runs synchronously in the initiating turn and knows its own session key) drops a per-session, per-boot file inHERMES_HOME; the clean-turn gate consumes it (read+unlink) independent of how the command was spelled. C1/F1 are retained as defense-in-depth fallback.Design (spec: 2 Opus passes + Phase-0 live probe + 2 diff-review passes)
pid:psutil.create_time) gates cross-boot crumbs → kills the false-trip vector where an interrupted initiator's crumb marks the next real-work turn. The gateway is the sole boot_id producer (writes it togateway_state.json); the script copies the string verbatim — no parser drift. (Phase-0 probe found the macOS/procpath returnsNone; psutil is cross-platform.)flag or consume()short-circuit) so a C1-flagged turn can't leak its crumb. Startup sweep + afinally-block defensive consume bound any leak.Testing
Review trail
Spec: 2 Opus passes (APPROVE-WITH-CHANGES ×4) + a Phase-0 live probe that falsified and corrected the boot_id mechanism. Diff review: pass-1 BLOCK (caught a feature-inert
boot_id-never-refreshed bug + masking tests) → fixed + RED-proven → pass-2 APPROVE-WITH-CHANGES (all 5 prior findings verified closed) → folded.Script side (safe-restart.py / watcher.py) lands in
ANG-Ventures/hermes-home. Parent: #72.🤖 Generated with Apollo (Hermes agent)