Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the session-scoped assignment design and the focused tests. I found one cross-platform correctness issue.
Problems
agent/context_experiments.py:67-75— themsvcrtbranch opens an emptya+lock file and, onOSError, logs then executes the state read-modify-write cycle without a lock. This breaks the stated round-robin serialization guarantee for concurrent Windows processes. Existing lock implementations document and handle this requirement by seeding one byte, openingr+, and seeking to offset zero beforemsvcrt.locking()(hermes_cli/auth.py:1024-1037;tools/skill_usage.py:99-108).
Suggested changes
- Seed and seek the Windows lock file using the established helper pattern, and do not perform the assignment update when locking cannot be acquired.
- Add coverage for the
msvcrtpath and lock failure/concurrent assignment behavior; the new tests currently exercise only serial direct builder calls (tests/agent/test_context_experiments.py:43-78).
Automated hermes-sweeper review.
| / f"{_safe_state_name(experiment_name)}.json" | ||
| ) | ||
|
|
||
|
|
There was a problem hiding this comment.
On Windows, msvcrt.locking() requires a non-empty lock file and a file pointer at offset zero; this a+ open leaves the new lock file empty, and the exception path below then continues the round-robin read-modify-write cycle unlocked. Please follow the seeded-byte + r+ + seek(0) pattern in hermes_cli/auth.py:1024-1037, and avoid assigning when the process lock cannot be acquired.
39467f8 to
e5eb2a6
Compare
|
Rechecked the current head The current implementation and regression coverage are present; no additional code change was needed in this recheck. Verification:
Please re-review the current head. |
Summary
context_experimentsconfig block for A/B testing prompt context arms before system-prompt assemblyTests
scripts/run_tests.sh tests/agent/test_context_experiments.py tests/agent/test_system_prompt.py tests/agent/test_prompt_builder.py tests/hermes_cli/test_prompt_size.py tests/hermes_cli/test_prompt_compose_command.py -q.venv/bin/python -m ruff check agent/context_experiments.py agent/prompt_builder.py agent/system_prompt.py tests/agent/test_context_experiments.py tests/agent/test_system_prompt.py.venv/bin/ty check agent/context_experiments.py tests/agent/test_context_experiments.py tests/agent/test_system_prompt.pyNotes
scripts/run_tests.shattempt initially failed because the newly created local.venvlacked pytest; after syncing the dev extra, the targeted canonical suite above passed.