fix(live-config): repair 4 base-rot tests across 4 subsystems - #94
Conversation
Fresh base rot on live-config (e8851f5): 4 unrelated tests red on every PR, not caused by any PR diff. Each an independent subsystem contract that drifted after upstream merges / half-landed features. - prompt_caching: retarget stale test to make_cache_marker() (ttl:1h default) - cli skills: restore PR #76 always-soft-fail (upstream re-added hard raise) - gateway fallback: /btw create site now uses _refresh_fallback_model() - /verbose: add 'log' to cycle+descriptions (feature 39bff67 half-landed) Patch note: ~/.hermes/plans/hermes-patches/fix-live-config-base-rot-4-tests.md
|
Claude finished @exiao's task in 2m 36s —— View job Review — PR #94: live-config base-rot repair (now 8 files)
LGTM. The PR grew past its original 4-test body to also carry the async-safety Verified against source (not just made-green):
Note: sandbox — I could not run pytest, so the 101-passed count is unverified on my end, but each assertion above matches the live code. 0 blocking, 0 minor. |
There was a problem hiding this comment.
Code Review
This pull request updates cli.py to degrade gracefully when requested skills are missing instead of raising a ValueError, allowing the agent to continue starting. It also updates a test assertion in test_prompt_caching.py to use make_cache_marker(). Feedback on the changes points out that using continuing.capitalize() in the warning log will lowercase the rest of the string, which can corrupt the casing of loaded skill names, and suggests a safer capitalization approach.
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.
There was a problem hiding this comment.
LGTM — verified all 4 base-rot repairs against their code paths and test contracts. (1) prompt-cache test retargets to make_cache_marker(), tracking CACHE_TTL as an invariant instead of a stale literal; (2) CLI skills now always soft-fail per the #76 contract; (3) /btw create site uses _refresh_fallback_model() like the other AIAgent create sites (create-time read, no prompt-cache impact); (4) /verbose cycle exposes the already-handled 'log' mode, mode_log i18n key present in all 16 locales. No new core tools, env vars, or role/cache violations. Note: could not run pytest locally (sandbox), so the 101-passed count is unverified on my end.
- test_kanban_worker_q_exit_wiring: FakeCLI._print_exit_summary stub now accepts clear_screen kwarg (real sig gained it via upstream merge) - gateway /compress: wrap bare self._sync_telegram_topic_binding on the loop in asyncio.to_thread (async-safety invariant, test_async_session_db) - cli.py preloaded-skills warn: continuing[0].upper()+continuing[1:] instead of .capitalize() to preserve skill-name casing (gemini review)
|
Fixed in fa6373b — replaced |
CI triage — 3 fixed, 4 files still red from a mis-resolved upstream mergeLanded
Remaining red (9 tests, 4 files) — NOT safe to babysitRoot cause:
Fixing these means porting/relocating the upstream source across |
There was a problem hiding this comment.
LGTM — verified all 6 changes in the final diff against code paths and test contracts. (1) prompt-cache test retargets to make_cache_marker() (invariant over stale literal); (2) CLI skills always soft-fail per the #76 dispatcher contract, gemini casing nit correctly fixed with continuing[0].upper()+continuing[1:]; (3) /btw create site uses _refresh_fallback_model() like all sibling create sites — create-time read, no prompt-cache impact; (4) /verbose cycle exposes the already-handled 'log' mode, mode_log key present in all 16 locales; plus the 2 post-merge fixes (compress asyncio.to_thread offload in an async handler, exit-summary stub signature match). No prompt-cache/role-alternation violations, no new core tool, no new env var. Could not run pytest locally (sandbox), so the 101-passed count is unverified on my end. 0 blocking, 1 minor doc/parity note.
CI 8-slice run surfaced the same class of base rot beyond the 4 named tests, all red on a clean base and none caused by the diff: - anthropic_adapter: 2 more stale cache-marker asserts -> make_cache_marker() - prompt_size + setup_blank_slate: send_file added to file bundle -> 6->7 tools - async session boundary: btw + compress used raw session_store; migrated 7 calls to async_session_store facade + wrapped _sync_telegram_topic_binding in asyncio.to_thread (fixes two async-guard source tests) - compress preview/dry-run/aggressive: wired the gateway handler the feature (ce9aa86) left CLI-only (fixes 5 compress_preview tests) - kanban_worker_q_exit_wiring: FakeCLI._print_exit_summary gained clear_screen - gnu-long-option test: protected-branch policy now flags a plain push to a protected branch; retargeted the no-force negative to a feature branch Patch note updated: fix-live-config-base-rot-4-tests.md
There was a problem hiding this comment.
LGTM — verified all 8 changes against live code paths. (1) cli.py always soft-fails missing skills per #76 dispatcher contract, casing preserved; (2) /btw uses refresh_fallback_model() (create-time, ephemeral agent, no cache impact); (3) /compress async migration correct — both handlers are async def and AsyncSessionStore.getattr offloads to asyncio.to_thread; preview path is read-only; aggressive_unsupported i18n in all 16 locales; (4) /verbose log mode + mode_log key in all 16 locales; (5) prompt-cache tests retarget make_cache_marker() (invariant over stale literal); (6) kanban stub signature matches; (7) prompt_size 6→7 and blank-slate send_file reflect the real file toolset (toolsets.py:195); (8) gnu long-option test switched to a feature branch because git push origin main is now separately flagged by the protected-branch policy (approval.py:780). No new core tool, no HERMES* env var, no prompt-cache/role-alternation violation. 0 blocking. Note: could not run pytest in sandbox, so the 101-passed count is unverified on my end.
Fresh live-config base rot: unrelated tests red on every PR
Every hermes-agent PR targeting
live-config(head e8851f5) inherited a REDtest set — NOT caused by any PR's diff, reproduced on a clean base checkout with
zero PR changes. This blocked the merge-readiness signal for #93 and every other
open live-config PR. Distinct from PR #55's earlier 13-file set; this is a new
set. Each is an independent subsystem contract that drifted after upstream
merges / half-landed features.
The task named 4 tests; CI's full 8-slice run surfaced the same class of rot in
several more files. All are fixed here (code or test, whichever holds the
authoritative contract). Nothing here is a regression from this PR's diff — each
failure reproduces on clean
live-config.Fixes
Prompt caching (tests stale ×3).
apply_anthropic_cache_control/convert_messages_to_anthropicuse the module-default markermake_cache_marker()={"type":"ephemeral","ttl":"1h"}(CACHE_TTL="1h").Three assertions hard-coded the pre-ttl literal. Retargeted to
make_cache_marker()so they track the constant.(
test_prompt_caching,test_anthropic_adapter×2)CLI preloaded skills (code regressed). Fork PR #76 made a missing
force-loaded
--skillsskill ALWAYS a soft-fail (warn + continue), neverexit-1. An upstream merge (018009b) reintroduced a hard
raise ValueErrorfor the all-missing case, conflicting with the contract + its test. Restored
always-soft-fail in
cli.main().Gateway fallback reload (code missed a site). The fallback-reload contract
(NousResearch#60955) requires every AIAgent create site to pass a freshly-read chain via
self._refresh_fallback_model(). The/btwephemeral-agent site still used thefrozen startup snapshot; switched it.
/verbose cycle (feature half-landed). Commit 39bff67 added
logmode,updated the test's expected cycle and all 16 locales, but never added
"log"tothe
cycle/descriptionsin_handle_verbose_command. Added it.send_filetool count (tests stale ×2).send_filewas added to thefiletoolset bundle (cea0241), so the Blank-Slate minimal surface is now 7 tools.
Updated
test_prompt_size.pyandtest_setup_blank_slate.py.Async SessionStore boundary (code missed sites). PR 9d38a23 enforced
"loop-side store calls use the async facade"; the
/btwand/compresshandlers still used raw
self.session_store.*and one bareself._sync_telegram_topic_binding(...)on the loop. Migrated the seven rawcalls to
await self.async_session_store.*and wrapped the topic-binding helperin
await asyncio.to_thread(...). (test_async_session_store,test_async_session_db)/compress --preview/--dry-run/--aggressive (feature half-landed). Commit
ce9aa86 added these flags + shared helpers + a gateway test but only wired the
CLI surface. Wired the gateway
_handle_compress_command: strip flags viaextract_compress_flags, short-circuit withsummarize_compress_previewforpreview, and return
gateway.compress.aggressive_unsupportedfor--aggressive.(5
test_compress_previewcases)_print_exit_summary(clear_screen=...)(test stub stale). The real methodgained a
clear_screenparam (NousResearch#53009); theFakeCLIstub didn't. Added it.(
test_kanban_worker_q_exit_wiring)Protected-branch push policy vs force-detection test (test stale). The
push-to-protected-branch policy (c12ffaf) now flags a plain push to a
protected branch, breaking
test_git_push_no_force_not_flagged(which onlymeans to assert "no force flag → not a force danger"). Retargeted its negative
case to a non-protected branch.
Verification
Every CI-flagged nodeid reproduced RED on a clean
live-configcheckout, thenGREEN after the fixes.
scripts/run_tests.shover all touched files passes. Theonly local reds are 3 macOS-keychain
TestRunOauthSetupTokentests, which passon CI Linux (the keychain reader returns None off-Darwin) and were NOT flagged
by CI.
Patch note:
~/.hermes/plans/hermes-patches/fix-live-config-base-rot-4-tests.mdDo NOT merge — hand-off for Eric.