loader: strict /^\d+$/ parse for MILTON_ATTN_MIN_TOKENS — Refs #62 - #63
Conversation
tps-flint
left a comment
There was a problem hiding this comment.
PLAN ACCEPTED on head 28e8571 — undraft and request tps-kern / tps-sherlock; their briefs are already sent. Do not wait on me further.
This is #62 as filed: String(raw).trim() then /^\d+$/, anything else takes the existing warn-once fallback to 32, the 1..8192 clamp and every documented input unchanged, and 0x10 / 3.9 moved from the silently-coerced list to the invalid list with the warning asserted. JS and tests only, no blob, so no pair. Merge follows K&S on this head.
tps-kern
left a comment
There was a problem hiding this comment.
APPROVED. All three disprove points on 28e8571:
- DOCUMENTED INPUTS — the test rows (harness/test/attn-min-tokens.test.js):
- "unset / empty use default 32 and do not warn" (line 26-33):
__UNSET__and__EMPTY__→ 32, zero warnings. - "in-range integers apply and do not warn" (line 36-45):
1,32,64,8192(= ATTN_MIN_TOKENS_MAX) → applied verbatim, zero warnings.3.9correctly REMOVED from this row. - "non-numeric / out-of-range fall back to 32 and warn once" (line 47-70):
abc,0,-3,NaN,Infinity,-Infinity,99999(all pre-existing) +0x10and3.9(moved in) → 32 + exactly one warn naming both the raw value and the applied 32. Each case is asserted twice (first/second resolve calls) with warnings.length === 1 — the warn-once is pinned per invalid input.
-
ACCEPTANCE MATRIX — I ran the full old-vs-new comparison. CHANGED (now rejected with warn):
+32(was silently applied as 32),3.9(was silently applied as 3 — Math.floor),0x10(was silently applied as 16 — the real bug Sherlock flagged: Number() parses hex),1e2(was silently applied as 100),32.0(was applied as 32). UNCHANGED:032/000032/08192→ applied (leading zeros pass /^\d+$/ and Number gives the right int — same as before),32\n/32/\t32→ 32 applied (String(raw).trim() at src/attn-min-tokens.js:26 handles whitespace/newlines; note the empty-string early return at line 23 checks the RAW value, sofalls through to the trimmed path and is rejected — slightly different from""which returns 32 silently, butwas also a warn-case before via !Number.isFinite),00/0→ fallback 32 + warn (Number("00")=0 < 1),8193→ fallback + warn, unicode digits: ARABIC-INDIC ٣٢ and FULLWIDTH 32 were ALWAYS rejected on the old path too (Number() returns NaN for non-ASCII digits — verified) so no regression; JS\dis ASCII-only both with and without the u flag (verified: /^\d+$/u.test("٣") === false). Every behavioral delta is in the REJECT direction — nothing new is accepted, and the previously-silent hex/exponent/plus-sign/decimal parses were operator-invisible surprise values. Strictly a tightening. -
WARN-ONCE — single site, unchanged. warnAttnClamp (src/attn-min-tokens.js:16-20) is the only warning source; it is called from exactly two places (line 30 regex-fail, line 35 range-fail), mutually exclusive branches, both behind the same module-level clampWarned guard (line 13, 17-18). grep confirms no other console.warn naming MILTON_ATTN_MIN_TOKENS anywhere in src/. Across-call warn-once semantics identical to the #51 MILTON_THREADS shape (second call with a different invalid value also stays silent — pre-existing, unchanged by this PR).
CI run 33834092009: all lanes green; local run of the test file 3/3. No blob, no kernel, no report-shape change — the JS resolver is the only consumer of the parse and everything downstream (applyAttnMinTokens, report field, compare tie) sees the same resolved value.
tps-sherlock
left a comment
There was a problem hiding this comment.
SECURITY review — APPROVE (all three checks hold).
1. Regex anchored + linear, String(raw) cannot throw — HOLDS. The parse is String(raw).trim() then /^\d+$/.test(trimmed) (src/attn-min-tokens.js:27-28). String(raw) is total — it never throws on any env value (undefined/empty are already returned early at :24-26; any other value, including a Symbol or object, stringifies). The regex /^\d+$/ is anchored both ends and has no alternation, no nested quantifier, no backreference — it is a single linear scan, no catastrophic-backtracking / ReDoS surface regardless of input length. Number(trimmed) is only reached after the regex has already guaranteed the string is all ASCII digits, so it cannot produce NaN/Infinity/hex/float.
2. Warn text carries no raw value into a persisted report — HOLDS. warnAttnClamp (attn-min-tokens.js:16-21) writes only to console.warn (stderr). lastThreadReport.attnMinTokens is still set from applyAttnMinTokens → api.attnMinTokens() >>> 0 (the resolved integer), never the raw env string. The #55 discipline is preserved.
3. Nothing else changed — HOLDS. The diff is exactly the 4 lines in resolveAttnMinTokens (the Number(raw)/`!Number.isFinite/\Math.floor→String(raw).trim()/\/^\d+$//\Number(trimmed)swap) plus the test updates that move"3.9"and"0x10"` into the warn-once fallback set. No other file, no blob, no kernel.
Net: the strict parse closes the #61 note — hex and float now fall back to 32 with a warn, the regex is ReDoS-safe, and the report still carries only the resolved integer. Approving.
Summary
Tighten
MILTON_ATTN_MIN_TOKENSso only a trimmed decimal integer (/^\d+$/) is accepted. Hex (0x10) and floats (3.9) no longer coerce silently viaNumber()into an in-range gate.Invalid values still fall back to 32 on the existing warn-once path. In-range decimal integers still clamp to 1..8192. Unset / empty still default to 32 with no warning.
No wasm blob change. No remapped build-of-record. No crossover / kernel / docs work.
Refs #62. Refs #59 #61.
Changes
src/attn-min-tokens.js: parse isString(raw).trim()then/^\d+$/; non-match → 32 + warn-once; valid ints still clamp 1..8192.harness/test/attn-min-tokens.test.js:0x10and3.9assert effective gate 32 + warn;3.9removed from the in-range (no-warn) list.Base
Pinned to
77f9ef54d54e5765b3101641ac4a1883bc51db45(squash of #61). Not rebased onto a movingmaintip.HEAD
28e8571df3c7036bf52086595479803ff438d9bd. Diff vs pin: 2 files, +7/−7. Nowasm/churn.How verified
Local Node v22 + CI run
33834092009SUCCESS (8/8), HEAD28e8571df3c7036bf52086595479803ff438d9bd, base77f9ef54d54e5765b3101641ac4a1883bc51db45.Documented paths (isolated process):
Chip cases (isolated process):
Already-invalid strings that already warned (
abc,0,-3,NaN,Infinity,-Infinity,99999) still warn once and apply 32.CI
338340920098/8 SUCCESS includingci / wasm(byte-compare, no blob commit) andci / wasm-compare.No epsilon / goldens /
expected.json/ wasm blob change.Assignee
issue_writeassign #62 → heskew returned 403 (Resource not accessible by personal access token). Issue already lists assigneeheskew. Same as #61.Stay draft. Off merge. Cos sends Reviewer later. Do not request tps-flint / tps-kern / tps-sherlock.
HEAD green — parent can ping Cos for Reviewer.