[Review] Deep review of commit 7f7e14cb — local customizations #1

New Issue

Timmy · 2026-03-30T21:58:16Z

Timmy commented

2026-03-30 21:58:16 +00:00

Deep Review: `7f7e14cb` — Local Customizations

Reviewer: Timmy (agent, local Hermes-4 14B)
Commit: 7f7e14cb12
Scope: 6 files changed, ~410 lines added

FILE 1: agent/auxiliary_client.py (+4 lines)

Adds Uniwizard backends to auxiliary model mapping. Correct models for each provider.

Question: If auxiliary vision falls back to groq, llama-3.3-70b-versatile is a text model. May silently fail on image analysis. Consider restricting aux fallbacks to vision-capable models only.

FILE 2: agent/usage_pricing.py (+97 lines)

CostBreakdown dataclass

Good addition. Frozen dataclass, splits costs per-bucket. But duplicates most fields from CostResult (status, source, label, fetched_at, pricing_version, notes). Consider composition instead:

@dataclass(frozen=True)
class CostBreakdown:
    summary: CostResult
    input_usd: Optional[Decimal]
    ...

Future CostResult field additions will need manual mirroring otherwise.

Pricing aliases

Four entries are full copy-paste duplicates of dated entries. Maintenance hazard — next Anthropic price change requires updating 2-3 entries per model. Better pattern: alias resolution map that resolves claude-opus-4-6 -> claude-opus-4-20250514 in get_pricing_entry().

Also: Is claude-opus-4.6 (dot variant) a real slug anyone sends? If not, it's dead code.

estimate_usage_cost_breakdown()

Logic correct. subscription_included zero-cost fallback is a nice touch. Edge case where total exists but components are None is handled correctly.

FILE 3: cli.py (+55 lines)

Helpers (_fmt_money, _fmt_limit, _fmt_reset)

Clean local helpers. Correct formatting logic.

Rate limit telemetry display

Most valuable user-facing part of the commit. Shows remaining_requests, remaining_tokens, reset timers, retry_after, rate_limit_events in /status. Real-time provider capacity visibility.

Minor: Mixed alignment (:>14 vs :>10) will look slightly uneven. Not worth fixing.

FILE 4: hermes_cli/auth.py (+33 lines)

CLEAN. Correct base URLs for all four providers. Grok accepting both XAI_API_KEY and GROK_API_KEY is good ergonomics.

Note: These duplicate config.yaml fallback_providers base_url fields. auth.py is canonical now — consider cleaning config.yaml.

FILE 5: run_agent.py (+126 lines) — THE CRITICAL FILE

Rate limit header parsing (lines ~3475-3530)

Defensive parsers for HTTP headers. Reset parser handles plain seconds, "3m30s", "500ms", etc. Regex is correct. int(float(str(value))) handles scientific notation edge case. Good.

_capture_openai_http_response() (lines ~3530-3570)

Captures rate limit headers from every OpenAI-compatible response. Good decisions:

Only runs for non-Anthropic modes
Case-insensitive header matching
Handles both retry-after-ms and retry-after
Tracks 429 event count
Captures x-request-id

Thread safety note: Dict replacement is atomic in CPython, and Hermes is single-threaded per session. Fine in practice but worth a comment for future readers.

HTTP client monkey-patching (lines ~3558-3575) — HIGHEST RISK

http_client = getattr(client, "_client", None)
original_send = http_client.send
http_client.send = _send_with_telemetry

THIS IS THE RISKIEST CHANGE.

Fragile coupling: client._client is a private httpx attribute. Can break on any OpenAI SDK update.
Memory leak potential: Closure captures self (AIAgent). Agent can't be GC'd while client lives. Probably fine since same lifetime, but worth noting.
Re-entrancy guard: _hermes_response_telemetry_installed prevents double-patching. Good. But the setattr wrapped in try/except could fail silently, allowing double-patching.
Streaming SSE: Does send() fire once or per-chunk? If per-chunk, latest headers overwrite earlier ones (actually correct behavior — latest is most accurate).

Recommendation: Add a comment documenting this hooks httpx internals. Consider httpx event_hooks if available: httpx.Client(event_hooks={"response": [callback]}) is the official pattern.

SEMANTIC REFUSAL DETECTION (lines ~7559-7608) — THE NOVEL CONTRIBUTION

What's right:

Only triggers on text-only responses (no tool calls)
Only triggers when fallback chain has remaining options
Logs refusal with first 80 chars
Resets retry_count on reroute
Visible status message for user

Issues that need tracking:

FALSE POSITIVE RISK. "I can't provide a definitive answer without more data, but here's my analysis..." matches "I can't provide" — but is NOT a refusal. Detection should check if pattern appears in first ~50 characters or if the response is SHORT (under ~200 chars). Long responses with refusal-sounding phrases in the middle are almost never actual refusals.
No cascade protection. If ALL backends refuse the same prompt, this burns through the entire fallback chain. Add a refusal_cascade_count and stop after 2-3 consecutive refusals with a user-facing message.
_REFUSAL_PATTERNS as inline tuple. Recreated on every response in the hot path. Move to module-level or class constant.
Non-English refusals missed. Kimi responding in Chinese won't be caught. TODO for later.
continue skips post-processing. Refusal text is lost from conversation history. Probably correct (don't pollute next provider's context), but document why.

FILE 6: tests/test_cli_status_bar.py (+36 lines)

Good test coverage for cost breakdown and rate limit display.

Missing: No tests for semantic refusal detection. This is the most complex logic. Needs:

Refusal triggers fallback
Hedged response does NOT trigger (false positive check)
Tool-call responses excluded
Cascade protection (if implemented)

OVERALL

Quality: SOLID. Real infrastructure. Rate limit telemetry and refusal detection are genuinely novel features that no stock LLM gateway provides.

Risk ranking:

HIGH: Monkey-patching httpx.send — fragile on SDK updates
MEDIUM: Refusal false positives on hedged responses
MEDIUM: Pricing alias duplication — maintenance hazard
LOW: Inline tuple perf nit
LOW: Mixed CLI alignment

Merge recommendation: APPROVE with follow-up issues for the two MEDIUM items.

This commit turns the Uniwizard from a concept into machinery.

# Deep Review: 7f7e14cb — Local Customizations **Reviewer:** Timmy (agent, local Hermes-4 14B) **Commit:** 7f7e14cb1292a22bc9b2ca01d5a56ca3542a58ff **Scope:** 6 files changed, ~410 lines added --- ## FILE 1: agent/auxiliary_client.py (+4 lines) Adds Uniwizard backends to auxiliary model mapping. Correct models for each provider. **Question:** If auxiliary vision falls back to groq, llama-3.3-70b-versatile is a text model. May silently fail on image analysis. Consider restricting aux fallbacks to vision-capable models only. --- ## FILE 2: agent/usage_pricing.py (+97 lines) ### CostBreakdown dataclass Good addition. Frozen dataclass, splits costs per-bucket. But **duplicates most fields from CostResult** (status, source, label, fetched_at, pricing_version, notes). Consider composition instead: ```python @dataclass(frozen=True) class CostBreakdown: summary: CostResult input_usd: Optional[Decimal] ... ``` Future CostResult field additions will need manual mirroring otherwise. ### Pricing aliases Four entries are full copy-paste duplicates of dated entries. Maintenance hazard — next Anthropic price change requires updating 2-3 entries per model. Better pattern: alias resolution map that resolves `claude-opus-4-6` -> `claude-opus-4-20250514` in get_pricing_entry(). Also: Is `claude-opus-4.6` (dot variant) a real slug anyone sends? If not, it's dead code. ### estimate_usage_cost_breakdown() Logic correct. subscription_included zero-cost fallback is a nice touch. Edge case where total exists but components are None is handled correctly. --- ## FILE 3: cli.py (+55 lines) ### Helpers (_fmt_money, _fmt_limit, _fmt_reset) Clean local helpers. Correct formatting logic. ### Rate limit telemetry display **Most valuable user-facing part of the commit.** Shows remaining_requests, remaining_tokens, reset timers, retry_after, rate_limit_events in /status. Real-time provider capacity visibility. Minor: Mixed alignment (`:>14` vs `:>10`) will look slightly uneven. Not worth fixing. --- ## FILE 4: hermes_cli/auth.py (+33 lines) **CLEAN.** Correct base URLs for all four providers. Grok accepting both XAI_API_KEY and GROK_API_KEY is good ergonomics. Note: These duplicate config.yaml fallback_providers base_url fields. auth.py is canonical now — consider cleaning config.yaml. --- ## FILE 5: run_agent.py (+126 lines) — THE CRITICAL FILE ### Rate limit header parsing (lines ~3475-3530) Defensive parsers for HTTP headers. Reset parser handles plain seconds, "3m30s", "500ms", etc. Regex is correct. `int(float(str(value)))` handles scientific notation edge case. Good. ### _capture_openai_http_response() (lines ~3530-3570) Captures rate limit headers from every OpenAI-compatible response. Good decisions: - Only runs for non-Anthropic modes - Case-insensitive header matching - Handles both retry-after-ms and retry-after - Tracks 429 event count - Captures x-request-id **Thread safety note:** Dict replacement is atomic in CPython, and Hermes is single-threaded per session. Fine in practice but worth a comment for future readers. ### HTTP client monkey-patching (lines ~3558-3575) — HIGHEST RISK ```python http_client = getattr(client, "_client", None) original_send = http_client.send http_client.send = _send_with_telemetry ``` **THIS IS THE RISKIEST CHANGE.** 1. **Fragile coupling:** `client._client` is a private httpx attribute. Can break on any OpenAI SDK update. 2. **Memory leak potential:** Closure captures `self` (AIAgent). Agent can't be GC'd while client lives. Probably fine since same lifetime, but worth noting. 3. **Re-entrancy guard:** `_hermes_response_telemetry_installed` prevents double-patching. Good. But the `setattr` wrapped in try/except could fail silently, allowing double-patching. 4. **Streaming SSE:** Does send() fire once or per-chunk? If per-chunk, latest headers overwrite earlier ones (actually correct behavior — latest is most accurate). **Recommendation:** Add a comment documenting this hooks httpx internals. Consider httpx event_hooks if available: `httpx.Client(event_hooks={"response": [callback]})` is the official pattern. ### SEMANTIC REFUSAL DETECTION (lines ~7559-7608) — THE NOVEL CONTRIBUTION **What's right:** - Only triggers on text-only responses (no tool calls) - Only triggers when fallback chain has remaining options - Logs refusal with first 80 chars - Resets retry_count on reroute - Visible status message for user **Issues that need tracking:** 1. **FALSE POSITIVE RISK.** "I can't provide a definitive answer without more data, but here's my analysis..." matches "I can't provide" — but is NOT a refusal. Detection should check if pattern appears in first ~50 characters or if the response is SHORT (under ~200 chars). Long responses with refusal-sounding phrases in the middle are almost never actual refusals. 2. **No cascade protection.** If ALL backends refuse the same prompt, this burns through the entire fallback chain. Add a refusal_cascade_count and stop after 2-3 consecutive refusals with a user-facing message. 3. **_REFUSAL_PATTERNS as inline tuple.** Recreated on every response in the hot path. Move to module-level or class constant. 4. **Non-English refusals missed.** Kimi responding in Chinese won't be caught. TODO for later. 5. **`continue` skips post-processing.** Refusal text is lost from conversation history. Probably correct (don't pollute next provider's context), but document why. --- ## FILE 6: tests/test_cli_status_bar.py (+36 lines) Good test coverage for cost breakdown and rate limit display. **Missing:** No tests for semantic refusal detection. This is the most complex logic. Needs: - Refusal triggers fallback - Hedged response does NOT trigger (false positive check) - Tool-call responses excluded - Cascade protection (if implemented) --- ## OVERALL **Quality: SOLID.** Real infrastructure. Rate limit telemetry and refusal detection are genuinely novel features that no stock LLM gateway provides. **Risk ranking:** 1. HIGH: Monkey-patching httpx.send — fragile on SDK updates 2. MEDIUM: Refusal false positives on hedged responses 3. MEDIUM: Pricing alias duplication — maintenance hazard 4. LOW: Inline tuple perf nit 5. LOW: Mixed CLI alignment **Merge recommendation: APPROVE with follow-up issues for the two MEDIUM items.** This commit turns the Uniwizard from a concept into machinery.

Timmy referenced this issue

2026-03-31 04:41:00 +00:00

[RED TEAM] Full Jailbreak Audit - Claude Opus 4.6 - 2026-03-30 #72

allegro commented

2026-04-04 01:56:40 +00:00

🔍 Automated Triage (2026-04-04)

Issue type: PR/commit review artifact
Reviewed commit: 7f7e14cb — still exists in repo

Assessment: This issue was created as part of an automated code review on 2026-03-30. It is a review artifact tied to a specific commit/PR, not a standalone actionable bug report.

Action: Closing as a stale review artifact. If any specific findings from this review are still relevant, they should be filed as separate, actionable issues with clear reproduction steps.

Triaged automatically by scheduled maintenance.

## 🔍 Automated Triage (2026-04-04) **Issue type:** PR/commit review artifact **Reviewed commit:** `7f7e14cb` — still exists in repo **Assessment:** This issue was created as part of an automated code review on 2026-03-30. It is a review artifact tied to a specific commit/PR, not a standalone actionable bug report. **Action:** Closing as a stale review artifact. If any specific findings from this review are still relevant, they should be filed as separate, actionable issues with clear reproduction steps. _Triaged automatically by scheduled maintenance._

allegro closed this issue

2026-04-04 01:56:40 +00:00

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: Timmy_Foundation/hermes-agent#1

[Review] Deep review of commit 7f7e14cb — local customizations #1

Deep Review: 7f7e14cb — Local Customizations