Skip to content

fix: restore live-config post-merge regression coverage - #122

Closed
exiao wants to merge 1 commit into
live-configfrom
fix/restore-live-config-fts-trigger-regression
Closed

fix: restore live-config post-merge regression coverage#122
exiao wants to merge 1 commit into
live-configfrom
fix/restore-live-config-fts-trigger-regression

Conversation

@exiao

@exiao exiao commented Jul 16, 2026

Copy link
Copy Markdown
Owner

Summary

  • delegate gateway /reasoning effort parsing to the shared parser (including max, ultra, false, and disabled)
  • retain v21's partial FTS-trigger repair regression while deleting v20-only FTS/trigram/watchdog tests
  • align catalog and credential-pool tests with their current production seams

Verification

  • PYTHONDONTWRITEBYTECODE=1 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 -- -p no:cacheprovider
  • PYTHONDONTWRITEBYTECODE=1 scripts/run_tests.sh tests/gateway/test_reasoning_command.py tests/test_hermes_state.py -- -p no:cacheprovider

Patch note: ~/.hermes/plans/hermes-patches/restore-live-config-fts-trigger-regression-t6f858a56.md

Patch note: ~/.hermes/plans/hermes-patches/restore-live-config-fts-trigger-regression-t6f858a56.md
@claude

claude Bot commented Jul 16, 2026

Copy link
Copy Markdown

Claude finished @exiao's task in 3m 0s —— View job


Review: PR #122 — restore live-config post-merge regression coverage

  • Read the diff against origin/live-config
  • Review gateway/run.py reasoning-parser delegation
  • Review deleted regression coverage in tests/test_hermes_state.py
  • Review updated test seams (credential pool, catalog)
  • Post review

LGTM. This is a clean post-merge cleanup — every deletion and reseam checks out against current production code.

What I verified:

  • gateway/run.py reasoning delegation — the inline effort branch is correctly replaced by parse_reasoning_effort (hermes_constants.py:829). none/false/disabled{"enabled": False}, valid efforts (incl. max/ultra) → {"enabled": True, "effort": …}, unrecognized → Noneunknown_arg. Behavior is preserved and broadened; the persist-global path still saves the raw effort string, which the same parser reads back on load. No prompt-cache or role-alternation surface touched (agent is evicted via _evict_cached_agent, as before).

  • tests/test_hermes_state.py (-525) — the deleted TestExternalContentFtsMigration and WAL-watchdog tests exercise a v20 design that no longer exists in production: _read_fts_trigram_config, _drop_trigram_schema, the messages_search_v external-content view, and the WAL watchdog are all gone from hermes_state.py (only _try_wal_checkpoint remains). Those tests would fail against current code, so removing them is correct, not a coverage regression. The retained/rewritten test_base_fts_rebuilds_when_one_base_trigger_is_missing matches the live repair path (_fts_trigger_count(cursor) < len(_FTS_TRIGGERS) → rebuild, hermes_state.py:1726) and correctly drops the now-removed _read_fts_trigram_config monkeypatch.

  • Credential-pool & catalog seamshas_available() on the fake _Pool matches the current preferred seam (credential_pool.py:604), and patching models._urlopen_model_catalog_request instead of urllib.request.urlopen targets the exact function used by the catalog fetchers (models.py:930+). Both are more precise, not looser.

Note: I couldn't execute the test suite in this environment (sandbox blocked pytest/run_tests.sh), so the pass/fail confirmation relies on the PR's own verification runs plus static cross-referencing. If you want CI to be the source of truth here, the affected files are self-contained.

0 blocking, 0 minor.
· fix/restore-live-config-fts-trigger-regression

@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 refactors and cleans up several test files. In test_multiplex_credential_isolation.py, a mock has_available method is added to the _Pool class. In test_reasoning_command.py, the reasoning command tests are updated to use a parameterized set of expected outputs. In test_models_secret_scope.py, the monkeypatched target for urlopen is updated to _urlopen_model_catalog_request. Finally, in test_hermes_state.py, the FTS rebuild test is simplified, and the obsolete TestExternalContentFtsMigration class is removed. There are no review comments, so no feedback is provided.

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.

@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 — clean post-merge coverage restoration. Verified: /reasoning delegation to parse_reasoning_effort preserves and broadens behavior; deleted v20 FTS/trigram/watchdog tests target code that no longer exists in hermes_state.py (retained repair test matches the live _fts_trigger_count path); credential-pool has_available() and models._urlopen_model_catalog_request seams match current production. Could not run the suite locally (sandbox), so relying on PR/CI verification. 0 blocking, 0 minor.

@exiao

exiao commented Jul 16, 2026

Copy link
Copy Markdown
Owner Author

Superseded by #124: same contents as this branch plus #120's approval-redaction fix cherry-picked, so the base goes fully green in one merge (this branch alone still fails the approval-hook test that #120 fixes). Recommend merging #124 and closing this.

@exiao

exiao commented Jul 16, 2026

Copy link
Copy Markdown
Owner Author

Superseded by #124, which combined #122 + #120's commit and merged into live-config (5ed8af1). Closing as redundant.

@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