Repository navigation
fix(doctor): check the complete Hermes SOUL read budget - #1331
Conversation
The PR description provides sufficient testing documentation, including build verification and a representative real-CLI run covering overflow, unchanged contents, and recovery after shortening. No missing-testing finding. I did not run, build, or install anything. |
There was a problem hiding this comment.
This was generated by AI during triage.
@nateEc Thanks, approving. This covers the Hermes SOUL.md part of the #1221 shared-budget item.
We built this head and its merge-base and ran teamai doctor --json against a fixture Hermes home. With an explicit context_file_max_chars, a SOUL.md one character over fails the new check, and one at the limit passes. Base has no such check. The new unit and e2e tests fail on base and pass here, and the existing doctor and Hermes tests pass. tsc and lint pass.
Suggestions:
[P2 non-blocking]Without an explicit cap, the check isinformational, but that flag only affects pull's summary (src/pull.ts).teamai doctorstill exits 1 with "Some checks failed" (allPassed = results.every(r => r.ok)insrc/doctor.ts). Going by Hermes's source, the default cap ismax(20000, min(context_window × 4 × 0.06, 500000)), so with a 128k-context model a 25,000-character SOUL.md is read whole, yet doctor fails. The existinginformationalchecks behave the same, so this follows the repo. Passing the check when no cap is set, or saying in the fix text that doctor counts this as a failure, would avoid a red doctor for a file Hermes reads whole.[P2 non-blocking]A malformedconfig.yamlfails the check as "budget unknown". Going by Hermes's source, its loader falls back to the last good config or the defaults, so nothing is truncated. We did not run Hermes.[P3 nit].trim()is not Python's.strip(): Python also strips U+0085 and U+001C to U+001F. It only matters for a file at the exact boundary.[P3 nit]The unmanaged-file test has an unusedawait budget('').
#1332 adds a sibling check and touches the same paragraphs in docs/guide/diagnostics.md, so whichever lands second needs a rebase there.
CI review: its one finding ([P3 nit] .trim() vs Python .strip()) is the third suggestion above, and we agree. It also accepted the testing record, which reproduces here. It did not raise the two [P2 non-blocking] points.
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
@nateEc The runtime review remains positive, but main moved when #1332 merged. This review supersedes the approval until the documentation conflicts are resolved.
- [P1 blocking] This head no longer merges cleanly with main. A merge against main
12b80e7fconflicts indocs/guide/diagnostics.mdanddocs/guide/zh-CN/diagnostics.md. Please merge or rebase current main, preserving both this PR's Hermes guidance and #1332's OpenClaw guidance. The production doctor changes merge cleanly; full content re-review is deferred to the updated head.
CI review reported one [P3 nit] about JavaScript .trim() differing from Python .strip(), and accepted the representative real-CLI testing record. We agree that the whitespace point is non-blocking and the record is sufficient. Existing head CI passed; the new blocker is the merge conflict caused by main advancing, not a failing CI job.
@nateEc the conflict with |
c07733d to
e7a53ed
Compare
|
Addressed the review follow-up at
The PR description contains the updated verification record. Ready for another review. |
|
No new findings in the scoped PR diff.
I only read the diff and trusted base review rules; I did not run, build, or install anything from the PR. |
- Measure member-authored text and every managed block together, using Python Unicode code points after trimming whitespace as Hermes does. - Honor an explicit context_file_max_chars cap and report malformed YAML; treat the model-dependent 20,000-character default floor as an informational warning. - Keep instructions intact and explain how to shorten the complete file or review the configured cap, with matching English and Chinese diagnostic guidance. - Verify 276 focused tests, TypeScript, lint and build; the real doctor CLI rejects an oversized fixture, preserves its contents and passes the budget check after shortening.
- 对齐 Python str.strip 的 Unicode 空白集合,正确剥离 C0 分隔符和 NEL,同时保留 BOM。 - 添加精确上限用例,修复前复现失败,修复后通过;保留主分支 OpenClaw 诊断说明和定位接口。 - 22 个相关单测和真实 CLI 预算诊断用例通过,构建、类型检查和 lint 通过。
e7a53ed to
290662f
Compare
|
Addressed the review follow-up at
The PR description contains the updated verification record. Ready for another review. |
|
No new findings in the scoped PR diff.
I only read the scoped diff and trusted base review rules; I did not run, build, or install anything from the PR. |
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
@nateEc Thanks, approving at 290662f. The latest rebase keeps #1336's OpenCode paragraph next to this PR's Hermes paragraph in both diagnostics guides, so the conflict is resolved. This is also the full content review I deferred last time. The whitespace handling now matches Python's strip(): the regex covers exactly the 29 code points isspace() accepts. Build, tsc, lint, the unit files (568 tests), the Hermes e2e and the doctor e2e pass, also with a symlinked TMPDIR.
- [P2 non-blocking] Hermes reads
SOUL.mdwithutf-8-sig, so it never sees a BOM, but this check counts it. A file with a BOM exactly at the cap is reported as over: the built CLI counts 20,001 where Hermes loads 20,000. Thekeeps BOMtest pins that behavior. - [P3 nit] The trailing-whitespace regex is quadratic. 50k trailing spaces take 8.9 s and 100k take 47.6 s, which is unlikely in a real
SOUL.md. The lint directive iseslint-disablewheresrcusesoxlint-disable. The unmanaged-file test still has an unusedawait budget(''). - The earlier notes stand as accepted: a default-cap overrun makes
doctorexit 1, per the existing policy, and malformed YAML reports an unknown budget.
The CI review reports no findings. We agree that nothing blocks. CI at this head is still running.
Summary
A current managed rules block is insufficient proof of delivery when member-authored text makes the complete
SOUL.mdexceed Hermes' context-file cap. Doctor now measures the whole managed file using Python Unicode code points after trimming, and readscontext_file_max_charsfrom Hermes' resolvedconfig.yaml.Type of Change
Test Plan
npx tsc --noEmitpassesnpm run lintpassesnpx vitest runpasses completely (two unchanged local baseline failures; see below)Added/updated tests for the change
RED: an authored prefix plus a small managed block had no read-budget diagnostic.
276 focused tests pass, covering exact boundaries, astral Unicode, missing caps, malformed YAML and unmanaged files.
TypeScript, lint and build pass.
Real CLI:
doctor-hermes-budget.test.tsrunsdoctor --jsonon an oversized file, verifies the budget failure and unchanged contents, then shortens the file and verifies that check passes.Real-CLI command:
npx vitest run --config vitest.e2e.config.ts src/__tests__/e2e/doctor-hermes-budget.test.ts.Repository-wide regression: the five-patch combined run on
origin/main6c57c3b7completed with 12,268 passed, 36 skipped, 2 failed across 503 files. Both failures are the existingsubmodule-checkoutcases for preserving empty old Claude/Codex hook files (git config --get hook.teamai-post-checkout.command). An independent pristine checkout of the same base reproduces exactly those 2 failures (24 other submodule tests pass). No new full-suite failure was introduced. The final ZCode exclusion follow-up is additionally covered by 267 uninstall/lifecycle tests and a post-uninstall pull through the real CLI.The final combined build, TypeScript and lint checks also pass. A final real-CLI run with all five patches passes 6 critical cases across all 5 affected E2E files, including the post-uninstall pull.
Related Issues
Related to #1221: Hermes standing-instruction truncation.
Notes for Reviewers
An explicit configured overflow fails the check. Without an explicit cap, Hermes derives a model-dependent limit with a 20,000-character floor, so an overflow of that floor is an informational warning rather than a claim of definite truncation. Malformed YAML reports an unknown budget. The fix hint and bilingual guide explain shortening the file or reviewing the cap; no content is truncated automatically.
Source: Hermes SOUL guide, prompt assembly, and the prompt-builder implementation referenced by #1221. This tests the CLI diagnostic, not a live Hermes/model session.
Local runtime tests use the Git provider and isolated filesystem fixtures; additional providers and live agent applications are deferred to CI/manual verification.
Follow-up verification (2026-10-25)
str.stripboundary mismatch: strip C0 separators and NEL, preserve BOM. The new boundary regression failed before the change and passed afterwards.doctor-hermes-budget.test.ts: 1 passed. Build, type checking and lint passed.Updated head:
290662f7f2ff5d36c8542e2fb8dbb3ff126f896a. Review base:a26f74d5bef51c498cc465b90599faa0130dc666.