fix(guardrails): loop-detect read-only shell commands in terminal

[rodrigogs]

Problem

A single terminal call repeated 517 times in one turn, ~46 minutes, each returning a byte-identical [] with exit_code: 0. Nothing stopped it until max_iterations.

terminal {"command":"cd ~/repo && gh api repos/o/r/pulls/N/reviews 2>&1","timeout":15}
  -> {"output": "[]", "exit_code": 0, "error": null}   x517

Both loop detectors in ToolCallGuardrailController missed it:

• No-progress detector only considers tools in IDEMPOTENT_TOOL_NAMES. terminal is in MUTATING_TOOL_NAMES, so _is_idempotent() returns False and repeated identical results were never tracked — even though the command being repeated was a pure read.
• Failure detector only counts non-zero exits (classify_tool_failure). The command succeeded every time. Logically stuck, technically fine.

Same session also showed 89x, 69x, 38x, 36x and 26x repeats of other terminal commands.

Fix

Classify the command, not just the tool name.

shell_command_is_read_only() splits on &&/||/;/|/newline and requires every segment to be a known read. Allowlist-only — the default answer is "this writes":

• unknown command, subcommand, or flag → write
• $(...) / backticks → write (can hide anything)
• > out / >> out → write; 2>&1 and 2>/dev/null do not disqualify
• write flags are scoped per command, because a global list conflates meanings: -f is --field to gh but --file to grep; -x is --method to gh but --exclude-type to df
• _FLAG_DECIDED_SUBCOMMANDS handles cases where flags decide: git config --get reads, git config k v writes; git branch lists, git branch name creates

A miss therefore costs a later block (the mutating threshold still applies), never a wrongly-blocked write.

Mutating calls now get the same detector at a looser ceiling (mutating_no_progress_block_after, default 12): a write repeating identical args and output is also making no progress, just with a weaker signal since it may be legitimately polling something external.

Verification

• Classifier checked against 79 commands (readers, writers, flag edge cases, injection attempts): 0 mismatches.
• Replaying the affected session's 828 real tool calls through the patched controller blocks at the 11th identical call instead of admitting all 517.
• False-positive check on legitimate patterns: a sleep-then-poll CI wait with stable output survives 20 polls; 30 distinct reads and a rebuild loop whose output changes are never blocked.
• pytest -k "guardrail or no_progress or loop_cap or tool_executor or turn_finalizer or turn_context" → 97 passed, 4 skipped. ruff clean.

Note on thresholds

The shipped hard_stop_after.idempotent_no_progress of 5 cut off a legitimate CI wait too early in testing. Operators running this may want 10. hard_stop_enabled still defaults to false, so nothing changes for existing users until they opt in.

[rodrigogs]

Related: #79840 fixes the fallback-chain defect that put the loop-prone free model in charge in the first place. The two are independent — this PR stops the loop, that one stops the bad provider selection — but they were found in the same incident.

[rodrigogs]

Hi — heads-up that CI has never actually run on this PR: every workflow run (CI + Docker Build) since it was opened ends in action_required with zero jobs, and the run page says 'This workflow is awaiting approval from a maintainer'. The branch is mergeable with no conflicts and doesn't touch any workflow files. Could a maintainer approve the workflow runs (or enable CI for outside collaborators)? Happy to address anything the checks find.

[rodrigogs]

Rebased onto current upstream/main (0a171fffe), plus one follow-up commit. New head bbdb26ad04a4.

Rebase

Conflict-free, and the change is unaltered — the diff against the new base is byte-for-byte the same size
as before (317 lines in agent/tool_guardrails.py, 91 in the test module). Upstream has changed
agent/tool_guardrails.py a lot in the meantime — per-turn runaway loop caps, observe_call /
IdenticalCallObservation stall guards, result-reference stubbing, UTF-16 surrogate-safe hashing — but none
of it touched the three regions this PR edits, so both commits replayed on context alone.

Verification on the rebase: tests/agent/test_tool_guardrails.py → 12 passed; neighbouring modules
(test_stall_guards.py, test_turn_context.py, test_tool_call_guardrail_runtime.py) → 100 passed.

Follow-up: the two new knobs are now real config keys

bbdb26ad04a4 closes a gap in this PR's own feature. It introduced
warn_after.mutating_no_progress and hard_stop_after.mutating_no_progress, consumed by
ToolCallGuardrailConfig, but never registered them in hermes_cli/config_defaults.py. Since
_validate_config_key validates dotted keys by walking DEFAULT_CONFIG,
hermes config set tool_loop_guardrails.warn_after.mutating_no_progress 7 saved the value and then told the
operator "'…' is not a recognized config key — it was saved anyway, but Hermes may not read it", with a
Did you mean: pointing at the idempotent_no_progress sibling. The knobs worked; the CLI said they
probably did not, and nothing documented them.

• hermes_cli/config_defaults.py:727,733 — both keys registered next to their idempotent_no_progress
  siblings, with the dataclass defaults (4 and 12) and a short note on why the mutating ceiling is looser.
• website/docs/user-guide/configuration.md:1729,1734 and the prose at :1742 — documented, including that
  for terminal the applicable threshold pair is decided by the command, not the tool name.
• cli-config.yaml.example:514,519 — same two entries in the shipped template, which already enumerated
  every other warn_after / hard_stop_after sibling.
• Regressions written before the fix and watched fail:
  test_mutating_no_progress_thresholds_are_registered_in_shipped_defaults (also asserts
  from_mapping(defaults) == ToolCallGuardrailConfig(), so the shipped defaults can never drift from the
  dataclass) and the TestValidateConfigKey::test_known_keys_pass parametrisation extended to all four
  dotted paths.

No _config_version bump: the addition is purely additive and load_config deep-merges DEFAULT_CONFIG,
so existing config.yaml files pick the keys up on the next read. Guardrail behaviour and thresholds are
untouched, and the PR's two original commits were neither amended nor reordered.

tests/agent/test_tool_guardrails.py → 14 passed; TestValidateConfigKey → 19 passed; config suites →
101 passed. End-to-end outside pytest, in an isolated HERMES_HOME: config set on both keys writes ints
with no unknown-key notice, ToolCallGuardrailConfig.from_mapping reads them back, and setting 2 / 3 makes a
write-classified terminal call warn at repeat 2 and return action=block /
code=repeated_identical_result_block at repeat 3 instead of the built-in 4 / 12.

One thing left for you to rule on

A looping terminal call now receives main's stall-guard notice (3rd consecutive identical call) and this
PR's no-progress warning — two overlapping guidance strings appended to the same tool result. Functionally
fine, and I did not touch it because deduplicating the messaging is a wording decision that belongs to
whoever owns the guardrail voice. Say the word and I will fold them.

This repository does not run CI on pull requests from forks, so the checks tab stays empty and
protect-main's required All required checks pass context never reports — which is why this PR shows
mergeable: true with mergeStateStatus: BLOCKED.

[rodrigogs]

Force-pushed a metadata-only fix so the contributor attribution check can pass.

.github/workflows/contributor-check.yml exits 1 on any commit-author email that has no file under
contributors/emails/, and ci.yaml calls it, so it feeds the required All required checks pass context.
This branch carried a placeholder author identity from a misconfigured local git config, which the check
would have rejected the moment a maintainer approved the workflow runs.

Every commit's author is now Rodrigo Gomes <2362425+rodrigogs@users.noreply.github.com> — the account's
GitHub noreply address, which the check auto-resolves via its +…@users.noreply.github.com rule, so no
mapping file is needed. Author dates are preserved, and the tree is byte-identical: git diff <old-head> <new-head> is empty, so nothing about the change under review moved and the verification I posted earlier
still stands.