Skip to content

fix(doctor): detect oversized ZCode hook rule replies - #1343

Merged
SaulMoro merged 2 commits into
Tencent:mainfrom
nateEc:fix/1221-zcode-output-budget
Oct 24, 2026
Merged

SaulMoro merged 2 commits into
Tencent:mainfrom
nateEc:fix/1221-zcode-output-budget

Conversation

@nateEc

@nateEc nateEc commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Doctor could report a registered, enabled ZCode hook as healthy when its project-rules reply alone exceeded ZCode's stdout budget and never reached the model. Measure the actual JSON serialization in UTF-8 bytes against the configured hooks.maxOutputBytes, defaulting to 32768, and provide a concrete repair instead of silently passing.

Type of Change

  • Bug fix
  • Breaking compatibility change (details below)

Test Plan

  • npx tsc --noEmit

  • npm run lint

  • npm run build

  • Added behavioral regression coverage

  • Entire unit suite green (baseline failures documented below)

  • npx vitest run src/__tests__/project-hook-rules.test.ts: 49 passed, 1 Windows-only case skipped. A multibyte rules reply fails the default byte budget, passes a larger configured cap, passes at the exact serialized byte count, and fails one byte below it. The missing diagnostic reproduces on unchanged main.

  • Real built CLI: after npm run build, the new byte-cap case in instruction-targets.test.ts through the E2E config passes. Pull/doctor detects 11,000 multibyte characters exceeding the default cap, then doctor passes after the fixture explicitly increases the user cap.

  • A live ZCode host session is unavailable locally. The config field/default and session snapshot behavior come from official ZCode hook documentation.

Broader regression record: a full combined runtime/test snapshot of these five fixes on main 83f6d1db ran npx vitest run --maxWorkers=8: 12,266 passed, 36 skipped, 4 failed across 502 files. Two failures were Pi normal-file diagnostic labels, subsequently corrected; the final combined rerun of instruction-targets.test.ts and user-rules-files.test.ts passes 373/373, including both regressions. The other two are existing submodule-checkout.test.ts empty legacy Claude/Codex settings cases: a separate clean 83f6d1db worktree reproduces exactly 24 passed / 2 failed on Apple Git 2.50.1 (missing hook.teamai-post-checkout.command). They are not hidden or skipped here.

The final combined snapshot passes type checking, lint and build. Four focused real built-CLI files pass 14 tests on the updated main; the final Pi/ZCode integration rerun passes 2/2 after the diagnostic-label correction. An earlier broader run of those four E2E files passed 56/56.

Local verification used isolated HOME directories and local Git remotes on macOS / Node 22.22.1. Additional live agent, native Windows and GitLab/GitHub provider combinations were not run locally.

Related Issues

Related #1221: addresses the missing ZCode hook-stdout budget diagnostic. This does not claim to solve the separate Hermes plugin-section budget.

Notes for Reviewers

The rules reply is explicitly a lower bound before other session hints. A passing check means the rules alone fit, not that every runtime hint combination fits. TeamAI neither truncates rules nor raises the user's configured cap. The byte count matches the dispatcher's stdout write without an added newline. Bilingual reference and agent troubleshooting docs explain shortening rules or increasing the cap before starting a new ZCode session.

Follow-up verification (2026-10-25)

  • Resolved the troubleshooting conflict while retaining main's archive guidance. Clarified that ZCode truncates oversized stdout and silently drops the unparseable reply.
  • npm run build, npx tsc --noEmit, and npm run lint: passed.
  • project-hook-rules.test.ts: 52 passed, 1 skipped.
  • Built-CLI instruction-targets.test.ts E2E: 43 passed, including the oversized ZCode reply and raised-cap recovery. No live ZCode application was available.
  • Rebased onto a26f74d after fix(hooks): launch OpenCode through the installed Node entry #1336 merged, keeping both troubleshooting sections. Rebase verification: project-hook-rules 52 passed / 1 skipped, all 43 built-CLI instruction-target cases, build, types and lint passed.
  • Regarding the rules-only measurement note: the current doctor fix text already says "The project rules alone need ... UTF-8 bytes of hook stdout, before other session hints" and tells the user to size the cap for the complete reply. Recall and other hints remain explicitly outside this lower-bound check.

Updated head: a86be694a6d43cefb52c9116c0a7f8d3d001f656. Review base: a26f74d5bef51c498cc465b90599faa0130dc666.

@github-actions

Copy link
Copy Markdown

No findings in the specified diff.

The PR description documents sufficient testing: a real built-CLI E2E run verifies failure at the default byte cap and success after increasing it. The unavailable live-host and additional platform/provider coverage is explicitly disclosed and is not blocking.

Review was read-only; I did not run, build, or install PR code.

@SaulMoro SaulMoro left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was generated by AI during triage.

@nateEc Thanks, approving. This covers the ZCode hook-stdout part of the #1221 shared-budget item.

We built this head, its merge-base 83f6d1db and main 54622c3a, and delivered project rules whose session-start reply is 33,107 bytes, as measured from the real hook output. On this head, teamai doctor fails the new check at the 32,768-byte default ("The project rules alone need 33107 UTF-8 bytes of hook stdout …") and passes once the cap is raised to 65,536. Base and main show no check and exit 0 with the same reply. The new unit and e2e cases fail on base and pass here, the doctor and ZCode unit tests pass, and tsc, lint and CI pass.

Suggestions:

  • [P2 non-blocking] The fix text and the EN/zh reference docs say ZCode "rejects" an oversized reply. Going by ZCode 3.16.2-38's bundle (vendor/zcode.cjs), it truncates stdout at the cap and then silently drops the reply it can no longer parse. The outcome is the same, but "truncates and drops it" tells the user why nothing shows up. We read the bundle and did not run a ZCode session.
  • [P3 nit] #1332 put its budget note in diagnostics.md, so this paragraph could live there too. The new test case could join the existing describe block.

CI review: it reported no findings. We agree nothing blocks. The wording point above is the only addition.

@SaulMoro

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Still approved, but this now conflicts with main in skill-data/core/references/troubleshooting.md (entries appended by the PRs merged today). Could you rebase? I will re-check and merge once it is clean.

@nateEc
nateEc force-pushed the fix/1221-zcode-output-budget branch from ad8377d to b8f99c9 Compare October 24, 2026 17:13
@nateEc

nateEc commented Oct 24, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the review follow-up at b8f99c99b4a42990fe18a134ba400c0a391783ff on current main.

  • Resolved the troubleshooting conflict while retaining main's archive guidance. Clarified that ZCode truncates oversized stdout and silently drops the unparseable reply.
  • npm run build, npx tsc --noEmit, and npm run lint: passed.
  • project-hook-rules.test.ts: 52 passed, 1 skipped.
  • Built-CLI instruction-targets.test.ts E2E: 43 passed, including the oversized ZCode reply and raised-cap recovery. No live ZCode application was available.

The PR description contains the updated verification record. Ready for another review.

@github-actions

Copy link
Copy Markdown

No findings in the specified diff.

The PR description documents sufficient testing: a real built-CLI E2E run verifies failure at the default byte cap and recovery after increasing it. Unavailable live-agent and additional platform/provider coverage is explicitly disclosed and is not blocking.

The earlier review reported no findings, so there are none to mark resolved.

Review was read-only; I did not run, build, or install PR code.

@SaulMoro SaulMoro left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was generated by AI during triage.

@nateEc Approving again at b8f99c9. The rebase changes only text. The fix message and both language versions of the docs now say ZCode truncates oversized stdout and drops the reply it cannot parse, which resolves the earlier wording note. Build, tsc, lint and the PR's unit and e2e files pass. On the built CLI the check exits 1 at the default cap and 0 at a 65536 cap, and the base has no check. The new tests fail on the base.

One more thing before it can land: it now conflicts with main in skill-data/core/references/troubleshooting.md, after #1336 added its section at the end of the same file. Keeping both sections resolves it.

  • [P2 non-blocking] The check measures the rules reply only. With recall on, the session-start reply also carries the recall hint: 36,272 bytes against 33,107 for rules alone in the same fixture. Rules a little under the cap would pass doctor while ZCode still drops the full reply. The PR calls it a lower bound; saying so in the check's message would help.
  • [P3 nit] The earlier notes still apply: the budget note could sit with #1332's in docs/guide/diagnostics.md, and the new e2e case is outside any describe. The body dates the follow-up 2026-10-25.

The CI review reports no findings. We agree that nothing blocks. All CI jobs pass.

- Measure the serialized project-rules hook reply in UTF-8 bytes against ZCode hooks.maxOutputBytes, using the documented 32768-byte default.
- Fail doctor with the required byte count and concrete recovery steps while keeping the user cap and full rule content unchanged.
- Describe the rules size as a lower bound before other hints and match the dispatcher output without an appended newline; update bilingual reference and troubleshooting.
- Verify multibyte/default/custom/exact-boundary regressions and a built-CLI pull/doctor scenario, type checking and lint; live ZCode is unavailable locally.
- 明确 ZCode 会截断超限标准输出,并静默丢弃无法解析的回复,与英文说明和诊断提示保持一致。
- 已在当前主分支上重放改动,保留会话归档排查说明。
- 构建、类型检查和 lint 通过;52 个相关单测和 43 个真实 CLI 端到端用例通过。
@nateEc
nateEc force-pushed the fix/1221-zcode-output-budget branch from b8f99c9 to a86be69 Compare October 24, 2026 18:07
@nateEc

nateEc commented Oct 24, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the review follow-up at a86be694a6d43cefb52c9116c0a7f8d3d001f656 on current main.

  • Resolved the troubleshooting conflict while retaining main's archive guidance. Clarified that ZCode truncates oversized stdout and silently drops the unparseable reply.
  • npm run build, npx tsc --noEmit, and npm run lint: passed.
  • project-hook-rules.test.ts: 52 passed, 1 skipped.
  • Built-CLI instruction-targets.test.ts E2E: 43 passed, including the oversized ZCode reply and raised-cap recovery. No live ZCode application was available.
  • Rebased onto a26f74d after fix(hooks): launch OpenCode through the installed Node entry #1336 merged, keeping both troubleshooting sections. Rebase verification: project-hook-rules 52 passed / 1 skipped, all 43 built-CLI instruction-target cases, build, types and lint passed.
  • Regarding the rules-only measurement note: the current doctor fix text already says "The project rules alone need ... UTF-8 bytes of hook stdout, before other session hints" and tells the user to size the cap for the complete reply. Recall and other hints remain explicitly outside this lower-bound check.

The PR description contains the updated verification record. Ready for another review.

@github-actions

Copy link
Copy Markdown

No findings in the specified diff.

The PR description documents sufficient testing: a real built-CLI E2E run verifies failure at the default byte cap and recovery after increasing it. Unavailable live-agent and additional platform/provider coverage is explicitly disclosed and is not blocking.

Earlier reviews reported no findings, so there are none to mark resolved.

Review was read-only; I did not run, build, or install PR code.

@SaulMoro
SaulMoro merged commit 8de97ae into Tencent:main Oct 24, 2026
18 checks passed
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.

3 participants