@qwen-code /triage

Thanks for the PR!

Template looks good ✓

On direction: this is a clean correctness fix — stopHookBlockingCap is a count-like safety valve, and silently flooring fractional values (e.g., 1.5 → 1) can make the loop protection fire earlier than the user expects. Directly addresses issue #5664. Well within scope.

On approach: minimal and focused — 5 files, +39/-7, no scope creep. The two-layer defense (JSON schema rejects fractional values in editors, env parser rejects fractional strings at runtime, normalizeStopHookBlockingCap floors as a last-resort safety net) is a sensible design. One minor observation: the PR description says "Fractional direct settings values fall back to DEFAULT_STOP_HOOK_BLOCK_CAP" but the code actually preserves the flooring behavior for programmatic config values (resolveStopHookBlockingCap(3.7) → 3). The test 'preserves legacy fractional config values' documents this intentionally. The description is slightly misleading but the code is correct — the schema prevents fractional values from reaching the code path in practice.

Moving on to code review. 🔍

— Qwen Code · qwen3.7-max

Code Review

Independent proposal (before reading the diff): I'd add a Number.isInteger() check on env var parsing to reject fractional strings, set the JSON schema type to "integer" with minimum: 1, and keep normalizeStopHookBlockingCap as-is for backward compatibility with programmatic callers.

Comparison: the PR matches this approach exactly. parseStopHookBlockingCapEnv() adds the integer gate for env vars, the jsonSchemaOverride sets type: 'integer', minimum: 1, and normalizeStopHookBlockingCap is untouched as a safety net. Clean implementation.

No critical blockers found. The diff is minimal and every change serves the stated goal. jsonSchemaOverride follows an established pattern (7 other usages in the file). The generated settings.schema.json correctly reflects the override.

Test Results

Before (main branch — stopHookCap.test.ts)

 RUN  v3.2.4 /home/runner/work/qwen-code/qwen-code/packages/core
      Coverage enabled with v8

 ✓ src/hooks/stopHookCap.test.ts (7 tests) 4ms

 Test Files  1 passed (1)
      Tests  7 passed (7)
   Start at  22:49:53
   Duration  4.11s (transform 45ms, setup 24ms, collect 22ms, tests 4ms, environment 0ms, prepare 83ms)

After (this PR — stopHookCap.test.ts)

 RUN  v3.2.4 /home/runner/work/qwen-code/qwen-code/.qwen/worktrees/triage/packages/core
      Coverage enabled with v8

 ✓ src/hooks/stopHookCap.test.ts (9 tests) 4ms

 Test Files  1 passed (1)
      Tests  9 passed (9)
   Start at  22:50:57
   Duration  4.04s (transform 54ms, setup 30ms, collect 21ms, tests 4ms, environment 0ms, prepare 87ms)

After (this PR — settingsSchema.test.ts)

 RUN  v3.2.4 /home/runner/work/qwen-code/qwen-code/.qwen/worktrees/triage/packages/cli
      Coverage enabled with v8

 ✓ src/config/settingsSchema.test.ts (27 tests) 18ms

 Test Files  1 passed (1)
      Tests  27 passed (27)
   Start at  22:52:12
   Duration  10.57s (transform 2.75s, setup 118ms, collect 4.31s, tests 18ms, environment 423ms, prepare 86ms)

All tests pass. 2 new tests added: rejects fractional env overrides and preserves legacy fractional config values.

— Qwen Code · qwen3.7-max

Going back to my independent proposal — the PR matches it almost exactly. The approach is sound: reject fractional env vars at the parsing layer, reject fractional config values at the schema layer, keep Math.floor as a defense-in-depth safety net for programmatic callers.

The diff is minimal and every line earns its place. No drive-by refactors, no scope creep. Two well-targeted new tests. The only thing I'd flag is the PR description's claim that "fractional direct settings values fall back to DEFAULT_STOP_HOOK_BLOCK_CAP" — the code preserves flooring for programmatic config, which the test correctly documents. The description is slightly imprecise but doesn't affect the code quality.

This is a straightforward correctness fix that ships what it promises. Tests confirm it works. LGTM. ✅

— Qwen Code · qwen3.7-max

✅ Real-binary verification report (local, tmux)

This PR makes the stop-hook blocking cap reject non-integers (falling back to the default) instead of silently flooring them, and declares the setting as integer / minimum: 1 in the JSON schema. I built the real qwen binary from PR head in an isolated worktree, drove it in tmux, and added function-level and mutation checks.

Verdict: correct, well-tested (non-vacuous tests), and consistent across the runtime guard + JSON schema + VSCode schema — safe to merge. One behavioral nuance is noted at the end.

1. Real-binary e2e (tmux) — the cap flows end-to-end

Setup: an always-block Stop hook (echo '{"decision":"block",...}') drives the runaway-loop safety valve; QWEN_CODE_STOP_HOOK_BLOCK_CAP=1.9 (a fractional override); identical prompt on both builds.

The hook logged each invocation to a counter file (1 vs 8, matching the warning). So a fractional cap is no longer silently floored to a small number — it is rejected and falls back to the safe default 8.

2. Function-level A/B (normalizeStopHookBlockingCap / resolveStopHookBlockingCap)

Integers behave identically; every non-integer now returns the default 8 instead of being floored. NaN / Infinity / strings already returned the default on both sides.

3. Mutation test — the added tests genuinely guard the change

• stopHookCap.test.ts: reverting the source to base → 2 new assertions fail (expected 3 to be 8, expected 1 to be 8).
• settingsSchema.test.ts: removing the new jsonSchemaOverride block → fails (expected undefined to deeply equal { type: 'integer', minimum: 1, default: 8 }).

4. Schema consistency

settingsSchema.ts (jsonSchemaOverride) and the VSCode settings.schema.json mirror both move stopHookBlockingCap from type: "number" (no lower bound) to type: "integer", minimum: 1, default: 8. Consistent.

Behavioral nuance (not a blocker)

The effective cap for fractional inputs can move in either direction: 5.9 was floored to 5 → now 8; 2.9 was 2 → now 8. A fractional iteration count is user error anyway, and falling back to the default 8 is the right behavior — just flagging that the resolved value can go up or down versus the old flooring, for anyone who set a fractional cap on purpose.

Environment

Real qwen built from PR head (0b41eda) via npm ci && npm run build; tmux 3.6a, isolated HOME with an always-block Stop hook; macOS, Node v22. BEFORE built by reverting only stopHookCap.ts to base and rebuilding core (cli resolves @qwen-code/qwen-code-core from core/dist at runtime).

✅ Local real-build verification (tmux A/B)

Maintainer verification before merge. I built the actual qwen binary at the PR head (0b41eda) and ran an A/B against a pre-fix build (the PR's parent 01d28a1, only stopHookCap.ts reverted + core rebuilt). Environment: macOS arm64, Node v22.22.2.

Verdict: LGTM. The fix changes user-visible runtime behavior exactly as intended (fractional caps now fall back to the documented default instead of being silently floored), with zero regression on every integer / out-of-range / NaN / clamp path.

1. Real-TUI A/B — the behavior issue #5664 describes

Harness: a user-scope Stop hook that always returns {"decision":"block"} (and appends one line to a counter file per firing), driven in tmux with QWEN_CODE_STOP_HOOK_BLOCK_CAP=1.5. The effective cap is observable two ways: the number printed in the override message, and the number of hook firings before the loop is overridden.

Before (pre-fix): 1.5 → Math.floor → cap 1 → loop protection fires after a single block:

✦ 你好！有什么我可以帮你的吗？
 ⎿ Stop says:
    Stop hook blocked continuation 1 consecutive time; overriding and ending the turn.

counter file = 1 firing

After (this PR): 1.5 is not an integer → falls back to DEFAULT_STOP_HOOK_BLOCK_CAP = 8 → the hook blocks 8 times before override:

✦ ok
● Ran 1 stop hooks
    ⎿  Stop hook error: Reply with only the word: ok
   … (×8 block cycles) …
✦ ok
 ⎿ Stop says:
    Stop hook blocked continuation 8 consecutive times; overriding and ending the turn.

counter file = 8 firings

(The Stop hook error: … line is just my test hook's own block-reason being echoed — not a real error.) This is precisely the surprise from #5664: a configured 1.5 made loop protection trip after 1 block instead of the intended 8. Note the message text itself carries the effective cap (1 time vs 8 times), so the wrong value was directly user-visible.

2. Built-function A/B (normalizeStopHookBlockingCap / resolveStopHookBlockingCap, imported from the built core/dist)

Only fractional inputs change; every existing valid/invalid/clamp path is byte-for-byte identical → the change is correctly scoped.

3. Mutation tests (proving the new tests aren't vacuous)

Reverted each source file to base while keeping the PR's tests, ran the suite, expected failures:

• packages/core — stopHookCap.test.ts: 2 tests fail against base —
  normalizes invalid values to the default cap → expected 3 to be 8, and
  rejects fractional environment overrides → expected 1 to be 8. Both pass on the PR. ✅
• packages/cli — settingsSchema.test.ts: should define stopHookBlockingCap schema override as a positive integer fails against base → expected undefined to deeply equal { type: 'integer', minimum: 1, default: 8 }. Passes on the PR. ✅

4. Schema generation is reproducible

npm run generate:settings-schema on the PR leaves git diff empty (committed settings.schema.json matches a fresh generation). Reverting settingsSchema.ts to base regenerates "type": "number" (no minimum), confirming the committed integer / minimum: 1 / default: 8 block is genuinely produced by the source change.

✦ 你好！有什么我可以帮你的吗？
 ⎿ Stop says:
    Stop hook blocked continuation 1 consecutive time; overriding and ending the turn.

✦ ok
● Ran 1 stop hooks
    ⎿  Stop hook error: Reply with only the word: ok
   … （共 8 个阻塞循环）…
✦ ok
 ⎿ Stop says:
    Stop hook blocked continuation 8 consecutive times; overriding and ending the turn.

✅ Local verification report (maintainer)

Built and tested this PR locally to confirm the fix before merge.

Environment

• macOS (Darwin 25.5), Node v22.22.2
• PR head 0b41eda6d — MERGEABLE. stopHookCap.ts has no drift; settingsSchema.ts drifted on main but auto-merges (different region).
• Method: isolated git worktree at the PR head with a clean npm ci + core/acp-bridge build, then verified each claim independently.

Results

Mutation test (the key evidence).

• Revert stopHookCap.ts to main → 2 tests fail: normalizes invalid values to the default cap (now includes 3.7 / 100.9 → default) and rejects fractional environment overrides (QWEN_…=1.5 → default). The old code floored these (3.7 → 3) instead of rejecting them.
• Revert settingsSchema.ts to the PR base → exactly 1 test fails: should define stopHookBlockingCap schema override as a positive integer (26 others pass).

Correctness review

• The core change is Number.isFinite → Number.isInteger (and dropping Math.floor): a fractional cap like 1.5 / 3.7 / 100.9 now falls back to DEFAULT_STOP_HOOK_BLOCK_CAP (8) instead of being silently floored, while valid integers are still clamped to [1, MAX=100]. This resolves bug(core): stop hook blocking cap accepts fractional values #5664. The schema side adds jsonSchemaOverride: { type: 'integer', minimum: 1, default: 8 }, the regenerated settings.schema.json (number → integer), and a schema test — all verified in sync with the generator.

Minor / non-blocking observation. resolveStopHookBlockingCap parses the env override with Number(env) (not the integer-syntax regex that #5652 used). So a giant fractional string like 9007199254740990.5 float-rounds to a safe integer, passes Number.isInteger, and is accepted — but it's then clamped to MAX=100, so the outcome is bounded and harmless, and the targeted fractional cases (1.5, 3.7) are correctly rejected. Given the MAX clamp this is effectively moot; mentioning only for parity with #5652.

Verdict: LGTM. Correct, minimal, mutation-verified, schema in sync; merges cleanly, type/lint/format clean. Safe to merge from my side.

✅ Local verification — fix(core): require integer stop hook cap

Verified on an isolated worktree at a123c8201 with a real npm ci + build, driving the compiled artifact (packages/core/dist/src/hooks/stopHookCap.js) under tmux — not just the TS source.

What the fix does: QWEN_CODE_STOP_HOOK_BLOCK_CAP now requires an integer — a fractional override (e.g. 1.5) falls back to the default 8 instead of being silently floored to 1. Config-file values keep their legacy flooring (back-compat), and the settings JSON schema now enforces integer, minimum 1.

Tests

• packages/core stopHookCap.test.ts — 9/9 ✅
• packages/cli settingsSchema.test.ts — 27/27 ✅
• tsc --noEmit (core) — 0 errors; eslint (changed files) — 0 problems ✅

Real compiled artifact under tmux (15/15 cases)

A/B proof (on the real build, under tmux)

Every other case is byte-identical between old and new — the change is surgical. Reverting the env guard also flips stopHookCap.test.ts > rejects fractional environment overrides from green to AssertionError: expected 1 to be 8, confirming the test actually guards the behavior.

Notes (merge reference, not blockers)

• The env path is now strict-integer, while the config path still floors fractionals (3.7 → 3) for backward compatibility — intentional and covered by preserves legacy fractional config values. So the JSON-schema integer constraint is advisory (editor-side) for config values, whereas the env override is enforced at runtime. Reasonable; just worth being aware of.
• Consumption sites (core/client.ts Stop loop at ~2362, agent.ts SubagentStop loop at ~1329) read the resolved cap via config.getStopHookBlockingCap() and are unchanged by this PR.

Verdict: ✅ Safe to merge. Behavior is correct on the compiled build, fully tested, back-compatible, and the A/B confirms the fix removes the silent-floor footgun (a typo like 1.5 no longer collapses the safety valve to a single iteration).

@qwen-code /triage