fix: stabilization fixes and architectural enhancements#14957
fix: stabilization fixes and architectural enhancements#14957aniruddhaadak80 wants to merge 7 commits into
Conversation
…and symlink privilege errors
austinpickett
left a comment
There was a problem hiding this comment.
Code Review -- PR #14957
Verdict: Request Changes -- one critical issue must be resolved before merge; several items warrant follow-up.
Critical
scratch/test_proxy.py committed to production
A bare debug/exploration script with loose top-level print() calls -- zero pytest/unittest structure -- has been committed. It tests built-in Python stdlib behavior (urllib.request.proxy_bypass_environment) that has no connection to any actual change in this PR. This file has no home in the production tree. Must be removed before merge. If the intent is to codify this behaviour, move it to tests/tools/test_proxy_bypass.py with proper pytest test functions.
Warnings
1. Dead code in test_curses_color_compat.py
The file already has a module-level pytest.skip(allow_module_level=True) guard that fires before any of the new code runs. The newly added pytestmark = pytest.mark.skipif(...) and the entire try/except ImportError: class DummyCurses block are unreachable dead code. Clean this up.
2. run_agent.py verifier methods not wired up in this diff
_record_file_mutation_result(), _file_mutation_verifier_enabled(), and _format_file_mutation_failure_footer() are added/restored to AIAgent but no call sites in run_agent.py are modified. If these existed before and were accidentally dropped, the PR should note when. If they are new, they need at least one call site or integration test.
3. approval.py: ~/.hermes/ replacement hardcoded for Windows paths
In _rewrite_resolved_hermes_home, the substitution string is always "~/.hermes/" (POSIX home-relative) even for the new Windows path branch. On native win32, this is not meaningful -- consider making the replacement Windows-aware.
4. api_server.py mutates os.environ at server startup
os.environ["IMAGE_SERVE_BASE_URL"] is set as a process-global side effect after bind. If a test suite starts multiple APIServerAdapter instances, the first one silently wins. Consider an instance attribute and explicit injection into the image tool.
Suggestions
- hermes_cli/config.py: TINKER_API_KEY / WANDB_API_KEY are RL-training keys unrelated to stabilisation scope. Consider a dedicated RL-tools PR.
- delegate_tool.py per-task api_key: confirm it is masked in any debug logging emitted by _build_child_agent.
- image_gen.serve_base_url and _maybe_rewrite_image_url are well-structured and defensively guarded.
Looks Good
- cron session_name: threads cleanly through create_job -> _run_job_impl -> cronjob() with schema exposure.
- Skills/tools cache invalidation: clear_skills_system_prompt_cache(clear_snapshot=True) after save is the right fix for immediate toggle effect.
- Windows test suite skips in test_search_hidden_dirs.py, test_file_operations.py, test_local_shell_init.py, test_windows_native_support.py are targeted and correct.
- test_background_review_summary.py import path update follows the refactor correctly.
Reviewed by Hermes Agent (automated)
Code Review SummaryPR #14957 -- fix: stabilization fixes and architectural enhancements Criticalscratch/test_proxy.py must not be committed to production. This is a bare debug/exploration script (top-level print() calls, no pytest structure) that exercises stdlib behavior unrelated to any diff in this PR. Remove or move to Warnings
Suggestions
Looks Good
Reviewed by Hermes Agent (automated) |
austinpickett
left a comment
There was a problem hiding this comment.
All three blockers from my last review are resolved:
scratch/test_proxy.py— absent from diff ✓- Dead/unconnected methods — removed ✓
- Duplicate verifier helpers in
run_agent.py— cleaned per commit message ✓
The remaining changes are genuine fixes:
- Chinese think-tag stripping (
思考/反思/推理/推敲) in bothstrip_think_blocksand the CLI stream filter — real bug for Chinese-locale models leaking thought blocks - Windows path fixes in
subdirectory_hints.py—RuntimeErrorcatch,posix=os.name!='nt'onshlex.split, and\\path separator recognition - Image serve URL via
ContextVarinimage_generation_tool.py+api_server.py— correct thread-safe fix for remote API clients that can't access local FS - Cache invalidation after
hermes toolsskill/tool disable — happens outside conversations, correct behavior --yeson headlessskills install— prevents interactive prompt in web server contextsession_namefor cron jobs — small, well-contained, allows named sessions to resume history across runs- Per-task endpoint creds in
delegate_task— useful, degrades cleanly to parent creds
One minor nit: the api_mode heuristic in delegate_tool.py checks hardcoded URL substrings (api.anthropic.com, api.kimi.com/coding, chatgpt.com/backend-api/codex) — will silently miss future endpoints, but the fallback to parent api_mode is safe. Consider a config-driven approach as a follow-up.
|
The branch has a merge conflict against current git fetch origin
git rebase origin/main
# resolve any conflicts, then:
git push --force-with-leaseEverything else is approved and looking good — just needs the rebase. |
…and stabilize Windows test suite
017e5a1 to
70a9177
Compare
Re-review (2026-06-11, new commit
|
…r tilde test on Windows
… in skill commands
# Conflicts: # pyproject.toml # scripts/run_tests_parallel.py # tests/conftest.py
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 54 out of 54 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (12)
tools/image_generation_tool.py:1
_maybe_rewrite_image_url()currently rewrites any non-URL string when a base URL is present (including relative paths likeout.pngorimages/foo.png). This can produce incorrect URLs and hide real relative-path semantics. Fix by first confirming the value is an absolute local filesystem path (e.g.,os.path.isabs(...)for POSIX, drive-letter/UNC detection for Windows, and optionally~-prefixed paths afterexpanduser()), and only then rewrite; otherwise return the original string.
tools/image_generation_tool.py:1_maybe_rewrite_image_url()currently rewrites any non-URL string when a base URL is present (including relative paths likeout.pngorimages/foo.png). This can produce incorrect URLs and hide real relative-path semantics. Fix by first confirming the value is an absolute local filesystem path (e.g.,os.path.isabs(...)for POSIX, drive-letter/UNC detection for Windows, and optionally~-prefixed paths afterexpanduser()), and only then rewrite; otherwise return the original string.
tools/image_generation_tool.py:1_maybe_rewrite_image_url()currently rewrites any non-URL string when a base URL is present (including relative paths likeout.pngorimages/foo.png). This can produce incorrect URLs and hide real relative-path semantics. Fix by first confirming the value is an absolute local filesystem path (e.g.,os.path.isabs(...)for POSIX, drive-letter/UNC detection for Windows, and optionally~-prefixed paths afterexpanduser()), and only then rewrite; otherwise return the original string.
tools/cronjob_tools.py:1session_nameis passed/stored without normalization, so an empty string can be persisted (and later treated as falsy elsewhere), creating a confusing 'set-but-not-effective' state. Consider normalizingsession_namesimilarly to other optional string fields (strip whitespace; treat empty asNone) on both create and update paths, and document how to clear the field (e.g.,\"\"clears vsnullleaves unchanged).
tools/cronjob_tools.py:1session_nameis passed/stored without normalization, so an empty string can be persisted (and later treated as falsy elsewhere), creating a confusing 'set-but-not-effective' state. Consider normalizingsession_namesimilarly to other optional string fields (strip whitespace; treat empty asNone) on both create and update paths, and document how to clear the field (e.g.,\"\"clears vsnullleaves unchanged).
tools/cronjob_tools.py:1session_nameis passed/stored without normalization, so an empty string can be persisted (and later treated as falsy elsewhere), creating a confusing 'set-but-not-effective' state. Consider normalizingsession_namesimilarly to other optional string fields (strip whitespace; treat empty asNone) on both create and update paths, and document how to clear the field (e.g.,\"\"clears vsnullleaves unchanged).
tools/cronjob_tools.py:1session_nameis passed/stored without normalization, so an empty string can be persisted (and later treated as falsy elsewhere), creating a confusing 'set-but-not-effective' state. Consider normalizingsession_namesimilarly to other optional string fields (strip whitespace; treat empty asNone) on both create and update paths, and document how to clear the field (e.g.,\"\"clears vsnullleaves unchanged).
tools/cronjob_tools.py:1session_nameis passed/stored without normalization, so an empty string can be persisted (and later treated as falsy elsewhere), creating a confusing 'set-but-not-effective' state. Consider normalizingsession_namesimilarly to other optional string fields (strip whitespace; treat empty asNone) on both create and update paths, and document how to clear the field (e.g.,\"\"clears vsnullleaves unchanged).
tools/skills_tool.py:1- Path normalization is repeated in multiple comprehensions via
.replace(\"\\\\\", \"/\"). For clarity and consistency, consider usingPath(...).as_posix()(orPurePosixPath(relative_path)if you need a pure conversion) in one place, or a small helper to normalize relative paths to'/'separators.
tools/skills_tool.py:1 - Path normalization is repeated in multiple comprehensions via
.replace(\"\\\\\", \"/\"). For clarity and consistency, consider usingPath(...).as_posix()(orPurePosixPath(relative_path)if you need a pure conversion) in one place, or a small helper to normalize relative paths to'/'separators.
tools/skills_tool.py:1 - Path normalization is repeated in multiple comprehensions via
.replace(\"\\\\\", \"/\"). For clarity and consistency, consider usingPath(...).as_posix()(orPurePosixPath(relative_path)if you need a pure conversion) in one place, or a small helper to normalize relative paths to'/'separators.
tools/skills_tool.py:1 - Path normalization is repeated in multiple comprehensions via
.replace(\"\\\\\", \"/\"). For clarity and consistency, consider usingPath(...).as_posix()(orPurePosixPath(relative_path)if you need a pure conversion) in one place, or a small helper to normalize relative paths to'/'separators.
| _DUMMY_HASH_VAL: str | None = None | ||
|
|
||
|
|
||
| def _get_dummy_hash() -> str: | ||
| global _DUMMY_HASH_VAL | ||
| if _DUMMY_HASH_VAL is None: | ||
| _DUMMY_HASH_VAL = hash_password("dummy-password-for-constant-time-verify") | ||
| return _DUMMY_HASH_VAL |
| username.encode("utf-8"), self._username.encode("utf-8") | ||
| ) | ||
| target_hash = self._password_hash if username_ok else _DUMMY_HASH | ||
| target_hash = self._password_hash if username_ok else _get_dummy_hash() |
| _OPEN_THINK_TAGS = ( | ||
| "<REASONING_SCRATCHPAD>", "<think>", "<reasoning>", | ||
| "<THINKING>", "<thinking>", "<thought>", | ||
| "<reasoning_scratchpad>", "<think>", "<reasoning>", | ||
| "<thinking>", "<thought>", " 思考", " 反思", " 推理", " 推敲", | ||
| ) | ||
| _CLOSE_THINK_TAGS = ( | ||
| "</REASONING_SCRATCHPAD>", "</think>", "</reasoning>", | ||
| "</THINKING>", "</thinking>", "</thought>", | ||
| "</reasoning_scratchpad>", "</think>", "</reasoning>", | ||
| "</thinking>", "</thought>", " 思考", " 反思", " 推理", " 推敲", | ||
| ) |
Summary of Changes
Details
This PR addresses several critical stability issues and architectural enhancements to improve the Hermes Agent's reliability across platforms and its overall user experience.
Related Issues
Fixes #14712, #12176
Type of Change