feat(agent): tool-call repair observability (port of #62640) - #77941
feat(agent): tool-call repair observability (port of #62640)#77941swissly wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds an operator-only observability layer for Hermes’ tool-call argument repair pipeline, introducing a bounded, thread-safe in-process collector and wiring it into existing repair/coercion paths plus a new CLI surface for inspection.
Changes:
- Added
agent/tool_repair_stats.pyimplementingRepairPattern, a thread-safeToolRepairStatscollector, and arecord_repair()convenience entry point. - Instrumented existing repair/coercion paths to emit repair events, and added a
hermes repair-statsCLI command to print a summary. - Added a new test suite validating counting, overflow behavior, thread safety, and summary/reset behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
agent/tool_repair_stats.py |
New repair stats collector + recording/summary APIs. |
agent/message_sanitization.py |
Emits stats events when JSON repair passes fire. |
agent/agent_runtime_helpers.py |
Emits stats events when history tool-call args are sanitized to {}. |
model_tools.py |
Emits stats events when schema coercion wraps bare values. |
hermes_cli/main.py |
Adds repair-stats operator CLI command to display the summary. |
tests/test_tool_repair_stats.py |
New unit tests covering stats recording/querying/overflow/thread-safety. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try: | ||
| from agent.tool_repair_stats import record_repair as _record_repair | ||
| from agent.tool_repair_stats import RepairPattern as _RP | ||
| except ImportError: | ||
| _record_repair = None # type: ignore[assignment] | ||
| _RP = None # type: ignore[assignment] |
| try: | ||
| from agent.tool_repair_stats import record_repair as _record_repair | ||
| except ImportError: | ||
| _record_repair = None # type: ignore[assignment] |
| def record_repair( | ||
| pattern: Any, | ||
| tool_name: str = "?", | ||
| model_name: str = "unknown", | ||
| success: bool = True, | ||
| detail: str = "", | ||
| ) -> None: | ||
| """Record a repair event. Silently no-ops on any failure.""" | ||
| try: | ||
| get_stats().record(pattern, tool_name, model_name, success, detail) | ||
| except Exception: | ||
| pass |
alt-glitch
added
type/feature
|
Addressed all 3 Copilot findings in commit 1. ImportError guard — 2. ImportError guard — 3. Per-model breakdown never fed — Fixed using the same pattern the codebase already uses for per-turn model state (
Note: Also fixed: PR body — Verification: 34 passed ( |
|
Ping @kshitijk4poor — this P3 feature (tool-call repair observability, port of #62640) has been ready for review since Aug 4:
No rush — just wanted to make sure it's on your radar. Related functional-repair PR #77395 is still open and complementary. |
|
Ping @teknium1 — this P3 feature (tool-call repair observability, port of #62640) is clean and ready to merge:
No rush — just surfacing it since it's been review-ready since Aug 4 and the window on the related functional-repair PR #77395 is still open. Happy to rebase or adjust anything. |
…62640) Ports the tool-call repair observability layer onto current main as a fresh, scoped PR. Supersedes NousResearch#62640 (5434 commits stale, never merged). - agent/tool_repair_stats.py: thread-safe ring-buffer singleton, per-model and per-pattern counts, 21 tests. Final review version (no dead set_current_model). - Instrumentation in the 3 repair paths: message_sanitization (_stat), agent_runtime_helpers (truncated_args), model_tools (bare-string/object wrap). Lazy-imported + defensive no-op so the module can be absent. - Operator output surface: new 'hermes repair-stats' CLI command wires summary() to a real call site (fixes Teknium finding NousResearch#4 — summary was dead code in the original PR). - 21 new tests + existing sanitize/coerce regression pass. Steps: Step 0 overlap check done — NousResearch#77395 (LIFO close) already merged upstream (functional part), NousResearch#34132/NousResearch#68612 are the repair logic itself not observability. This is the only stats/observability PR.
… (Copilot NousResearch#77941) - message_sanitization / agent_runtime_helpers: separate ImportError (expected, module absent) from Exception (module broken) in the stats import guard; broken module degrades to no-op with WARNING instead of killing repair - tool_repair_stats: add _current_model ContextVar + set_current_model() so record_repair attributes events to the live model without call-site plumbing (set_current_model now has a real caller + consumer, no longer dead code) - turn_context: bind set_current_model per turn next to set_runtime_main (same pattern, strictly best-effort) - tests: 4 new tests for contextvar attribution, unknown fallback, explicit model override, thread isolation
3266ba5 to
5ac7406
Compare
What does this PR do?
Adds tool-call repair observability — a bounded, thread-safe collector that records when the repair pipeline fixes malformed tool-call arguments, with per-pattern and per-model breakdowns. Operator-only: no new model tools, no prompt impact, no model-layer changes.
Port / supersede: This is a clean re-port of #62640 onto current
main(that branch is 5434 commits stale and was never merged). Same feature, final review-state, plus review-round-2 improvements (Copilot #77941): hardened import guards (ImportErrorvsExceptionseparation) andset_current_modelreintroduced — this time with a real per-turn caller (agent.turn_context, same binding point asset_runtime_main) and a real consumer (record_repaircontextvar fallback), so the per-model breakdown actually works. In #62640 it was dead code (Teknium finding #2); now it has a caller.What it fixes / adds
agent/tool_repair_stats.py(new):RepairPatternenum (24 patterns), thread-safeToolRepairStatssingleton with a 10,000-event ring buffer, per-model + per-pattern counts,summary(),reset(). Per-turn model context viaset_current_model()(ContextVar, bound fromagent.turn_context). 25 tests.message_sanitization.py::_repair_tool_call_arguments→ empty_args, none_literal, control_char_escape, malformed_json_repair, unrepairableagent_runtime_helpers.py::sanitize_tool_call_arguments→ truncated_argsmodel_tools.py::coerce_tool_args→ bare_string_wrap, bare_object_wraphermes repair-statsCLI command wiressummary()to a real call site. In the original feat(agent): add tool-call repair observability #62640,summary()had no production caller — Teknium's review finding Fix terminal interactivity #4 (dead code). This port fixes it.Why observability matters
Hermes' repair layer silently fixes malformed tool-call JSON before it reaches the API. Without stats there is no visibility into which models fail, how often, and on which patterns — the data needed for model routing / provider decisions. This collects that signal in-process with zero model-layer cost.
Scope & overlap
Tests
tests/test_tool_repair_stats.py(ring-buffer trim accounting, per-model counts, summary, reset, pattern normalization, model-context attribution + thread isolation).tests/test_sanitize_tool_error.py(existing) still passes — the instrumented functions behave identically without the stats module.tool_repair_stats.pychanges nothing.