Skip to content

fix: reconcile live-config post-merge regressions - #121

Closed
exiao wants to merge 2 commits into
live-configfrom
fix/restore-live-config-regression-t2501c013
Closed

fix: reconcile live-config post-merge regressions#121
exiao wants to merge 2 commits into
live-configfrom
fix/restore-live-config-regression-t2501c013

Conversation

@exiao

@exiao exiao commented Jul 16, 2026

Copy link
Copy Markdown
Owner

Summary

  • Restore max and ultra in the concrete gateway /reasoning handler that shadows the newer mixin implementation.
  • Update catalog and credential-pool tests to exercise upstream’s secure request wrapper and availability API.
  • Retire the obsolete v20 external-content FTS, trigram gate, and WAL watchdog test class while retaining a v21 base-FTS trigger-repair regression.

Verification

  • scripts/run_tests.sh tests/gateway/test_reasoning_command.py tests/gateway/test_multiplex_credential_isolation.py tests/test_hermes_state.py tests/hermes_cli/test_models_secret_scope.py — 431 passed.
  • uv tool run ruff check gateway/run.py tests/gateway/test_multiplex_credential_isolation.py tests/hermes_cli/test_models_secret_scope.py tests/test_hermes_state.py — passed.
  • git diff --check — passed.

Notes

Patch note: ~/.hermes/plans/hermes-patches/restore-live-config-regressions-t2501c013.md
@claude

claude Bot commented Jul 16, 2026

Copy link
Copy Markdown

Claude finished @exiao's task in 2m 18s —— View job


PR Review: reconcile live-config post-merge regressions

  • Gather diff against origin/live-config
  • Review gateway/run.py reasoning handler change
  • Review tools/approval.py deletion
  • Review test changes (credential pool, models secret scope, hermes state)
  • Post review

LGTM. All four reconciliations match current production; verified each against the source it re-aligns to.

  • gateway/run.py:14347 — restoring max/ultra matches the canonical VALID_REASONING_EFFORTS ("minimal","low","medium","high","xhigh","max","ultra"). Without them the concrete handler silently rejected two valid levels the mixin accepts.
  • tools/approval.py — the removed early _redact_for_approval(...) block was dead: display_command/display_code/display_description are unconditionally re-assigned by the later redact_sensitive_text(...) block before any use, and nothing reads them in between. Display-time redaction is preserved.
  • test_models_secret_scope.py — production fetches catalogs through _urlopen_model_catalog_request (→ open_credentialed_url), so the old urllib.request.urlopen patch no longer intercepted the call. Retargeting the mock to the wrapper is correct.
  • test_multiplex_credential_isolation.py — the listing path calls pool.has_available(); adding it to the _Pool mock matches the real Pool.has_available (credential_pool.py:604).
  • test_hermes_state.pyTestExternalContentFtsMigration covered the external-content FTS mode, the _read_fts_trigram_config gate, and the WAL watchdog — all gone from production (schema is now 21, external-content switched to inline, _read_fts_trigram_config no longer exists, so the old test's monkeypatch.setattr(SessionDB, "_read_fts_trigram_config", ...) patched a phantom). The retained/rewritten test_base_fts_rebuilds_when_base_trigger_missing still exercises real behavior: drop a base trigger → reopen repairs it and reindexes the gap, consistent with the current triggers_need_repair = _fts_trigger_count < len(_FTS_TRIGGERS) logic. No live coverage lost.

No prompt-cache, role-alternation, footprint, env-var, or profile-safety concerns — this is test/handler reconciliation only.

Note: I could not execute the test suite in this environment (command approval unavailable), so I relied on static verification plus the PR's reported 431 passed.

0 blocking, 0 minor.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — verified all four changes reconcile tests/handler against current production. gateway/run.py restores max/ultra to match the canonical effort set (hermes_constants.py, chat_completions.py, slash_commands.py). Test mocks now match production (has_available in model_switch.py:1418-1419; _urlopen_model_catalog_request wrapper in models.py:34). Retired TestExternalContentFtsMigration is genuinely obsolete (v20 external-content FTS / trigram gate / WAL watchdog no longer exist at schema 21); retained v21 base-FTS trigger-repair regression still exercises real behavior. 0 blocking, 0 minor.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request modifies several test suites. In test_multiplex_credential_isolation.py, a mock has_available method is added to the _Pool class. In test_models_secret_scope.py, the monkeypatching of urllib.request.urlopen is updated to target models._urlopen_model_catalog_request. In test_hermes_state.py, the FTS rebuild test is simplified, and the TestExternalContentFtsMigration test class is removed entirely. There are no review comments, so we have no feedback to provide.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

check_execute_code_guard redacted three display_* copies with
_redact_for_approval (force=False) at the top of the approval path, before
the smart-approval observer's two force=True calls. That made the redaction
sequence [False,False,False,True,True] instead of the expected [True,True],
failing test_smart_observer_redaction_is_forced_when_config_disables_redaction
and blocking every open PR into live-config.

The eager block was dead: all three display_* names are unconditionally
reassigned lower down (past the early-return gates) before first use. Remove
it so the execute-code path matches check_all_command_guards, which already
redacts for display only after the smart branch. Display/redaction behavior
is unchanged.

Patch note: ~/.hermes/plans/hermes-patches/execute-code-guard-eager-redact.md

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — verified all four reconciliations against current production: gateway/run.py restores max/ultra to match VALID_REASONING_EFFORTS; the removed approval.py redaction block was dead (overwritten before use); test mocks now target the real _urlopen_model_catalog_request wrapper and pool.has_available; and the retired TestExternalContentFtsMigration is genuinely obsolete (external-content/trigram-gate/WAL-watchdog gone at schema 21) while the retained base-FTS trigger-repair test still exercises real behavior. 0 blocking, 0 minor.

@exiao

exiao commented Jul 16, 2026

Copy link
Copy Markdown
Owner Author

Closing as duplicate of #122. Both fix the same live-config post-merge regression (SessionDB/FTS, credential-pool, hermes_state); #122 is the superset — it also carries the /reasoning false|disabled parser fix + regression that this PR lacks. #121's only unique file (tools/approval.py eager-redaction) is already shipping standalone as #120, so nothing is lost. Branch left intact. Root cause of the duplicate: a kanban double-dispatch race (see infra-ops card) made the original run look like a no-op, prompting a redundant re-cut.

@exiao exiao closed this Jul 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant