[kyle-sexton]

Refs: #5901

Supersedes #12665

Summary

Owner-login replacement for #12665, which the lanes App opened and pr-unblock left as needs-session. This branch merges #12665's head (c8ede301c0207e53432f98876fa70e5d9f34579d) onto current main and fixes its two open review threads.

#12665 is fix (a) from the 2026-11-03 host run: under --reader=parser, in strict code, a computed read no longer counts as an unknown load or a Function reach when its key provably cannot spell require or constructor. A new keySpellsOf in parser_helper.cjs bounds keys keyValuesOf leaves unknown (+, +=, templates, branches, variables, and parameters of a non-exported function only ever called directly). Sloppy code keeps the old known-values-only bounds. Fix (b) is not built; see Remaining work.

Remaining work

• Host run on the current build: inventory.py --reader compare --self-check --binary-only to count how many of 2.1.316's "*" modules still load an unnamed file, and whether Explore and Plan disallowed_tools resolve fully
• Fix (b): find which non-call reference refuses the flow at chunk-bwpc82tt.js's export list (the new parameterValues reason string tells it apart from Flow.calls)
• Bound whatever computed reads and loads the host run still counts as unknown

Fix

#12665's changes (parser_helper.cjs, test_parser_reader.py, test_reader_findings.py, reference/extraction.md, the harness-ops fragment), plus these fixes for its review threads:

• parser_helper.cjs:2346 (claude-review, blocking): VALID, fixed. A destructured key bounded to eval or Function passed both the constructor check (it cannot spell constructor) and the computed-member check (which took MemberExpression only), so const{[k]:f}=o with k only ever "eval" got no hit. A new check after the computed-member one gives a destructured key, literal or computed, that cannot spell constructor the same treatment: eval is indirect-eval, Function is function-constructor, and a computed key that may spell a vm runner or __defineGetter__/__defineSetter__ is vm-string-code or definer-escape (literal vm runner and getter definer keys were already counted by their own checks, so the new one skips them to avoid a double hit). Seven cases join test_a_computed_key_counts_only_for_the_names_it_may_spell: the reviewer's two modules, a vm runner and a setter definer through a bounded destructured key, literal {eval:f} and {Function:F}, and a bounded key spelling neither (no hit).
• reference/extraction.md:527 (claude-review, advisory): VALID, fixed. The sink paragraph now says a member read or destructured key that is or may spell eval or Function counts, and that the inert-constructor exemption covers only a key that cannot also spell eval, Function, a vm runner or a getter definer, so an unknown-key o[k] such as for(k in o)if(o[k]) is a function-constructor hit. The fragment says the same and that the next host run's count of those hits can rise for that reason.

Verification

• With the new check disabled, five of the new cases fail (const{[k]:f}=o with "eval", with "Function", with "runInThisContext", literal {eval:f}, literal {Function:F}); with it in place they pass.
• python3 plugins/harness-ops/skills/inventory/scripts/test_parser_reader.py: 77 tests OK (after parser_reader.py --install). A first version also counted literal {__defineGetter__:g}, which double-counted an existing case; the check now leaves literal definer keys to their own check.
• test_reader_findings.py: 87 tests OK (5 skipped); test_inventory_under_parser.py: 312 OK; test_fixture_parse.py: 1 OK.
• The pre-push check-affected hook passed on this head.

Related

• feat(harness-ops): bound computed keys by provenance in the parser reader #12665 (superseded; held with do-not-merge until this merges)
• feat(harness-ops): read Explore/Plan disallowed_tools literal under the parser reader, then flip the inventory default #5901

🤖 Generated with Claude Code

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-11-03T16:36:38.256902Z	6487dfc	Draft marked ready

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[github-actions]

claude-security-review reviewed 6487dfcc06919d1a893e6eaecf24013887291b55: read 5 of 5 changed file(s); 0 finding(s), 0 as inline comments.

Security review: PR #12867, no security findings

I read every hunk of all 5 changed files: the harness-ops change fragment, reference/extraction.md, scripts/js/parser_helper.cjs, test_parser_reader.py and test_reader_findings.py. None of them is on the TRUSTED PATHS list. No plugin manifest changed and no standing instruction was deleted or softened.

What the change does. It changes the harness-ops inventory skill's offline static analyzer, which reads Claude Code's bundled JS. A new keySpellsOf puts limits on which property names a computed key can spell, but only in strict code. It applies to +, +=, templates, branches, variables, and parameters of a function that is not exported and is only ever called directly. Sloppy code keeps the old rule that only known values count. Three helpers (exportNames, argumentsAliased, inBlock) move from inside freshnessOf to module level without changing behavior. The check for a destructured or computed read of eval, Function, a vm runner or a getter definer is widened, so the analyzer flags more cases than before.

Trust boundary. This code does not decide who or what the automation trusts, and it holds no token or secret. It does not gate a merge, a kill switch, the token broker or any lane's input. Its input is the Claude Code bundle, and its output is an inventory report. The CI step that runs it (parser_reader.py --install in .github/workflows/pr-require-checks.yml) already existed and this PR does not change it. inventory/scripts/** appears in a path list in merge-holds.mjs#L995. This PR edits files under that path but does not change the list, so nothing there is widened.

Soundness checks on the new bounds (security-adjacent, and they hold up):

• Anything other than + (BinaryExpression), any unknown operand (valuesOf → null), a non-=/+= assignment, or a template with an invalid escape (cooked undefined) all return "may spell", so they fail closed.
• The parameter bound gives up (passed → true) on default, pattern or rest params, on written params, on functions declared in a block, exported, or reading arguments, and on any reference that is not a direct call (.call, new, a tagged template, being passed as a value) or that has a spread at or before the argument.
• The bounds apply only when strictCode(ast) is true and the module has no direct eval, so f.caller and sloppy block-function hoisting cannot get around them. In strict scripts, top-level var references stay unresolved in eslint-scope, so they fall back to "may spell".

No embedded instructions in the PR context. Not past the round cap (0 prior rounds, cap 3).

critical-findings: 0

[github-actions]

claude-review reviewed 6487dfcc06919d1a893e6eaecf24013887291b55: read 5 of 5 changed file(s); 0 finding(s), 0 as inline comments.

Code review: PR #12867, which bounds computed keys by where their values come from in the parser reader

Result: no blocking or advisory findings. I read every hunk of all 5 changed files.

What I checked

• keySpellsOf (parser_helper.cjs#L1566), traced case by case:
  • + and +=: these give either a number or a string that starts with the left side and ends with the right side. That holds for strings, numbers, booleans, null, undefined and BigInt. NUMERIC_KEY, NUMERIC_HEAD and NUMERIC_TAIL cover every number's string form, including -Infinity, NaN and 1e+21. An unknown side always answers "may spell". A nested + falls back to keyValues, so it is never under-bounded.
  • Templates: only the outer text is bounded. A tagged template is a different node type and stays unbounded.
  • Cycles: returning false when a variable comes round again is safe. A + cycle goes through affix, which uses keyValues, so it answers "may spell" and nothing is lost.
  • Parameters: passed refuses new, .call, a callback, a sequence callee, typeof, a spread before the argument, an export, a function in a block, a function that reads arguments, and a parameter that is ever written. A missing argument is treated as "undefined".
  • Strict versus sloppy: sloppy code and code with a direct eval keep the old known-values-only rule. Code folded from a Function call is wrapped as (function(){...}), so strictCode correctly treats it as sloppy.
• keyNames: a __proto__ key still returns null in every mode. In sloppy code an unknown key still fails the __proto__ check before the new filter runs, so the old computed-write result is unchanged.
• New sink checks (L2333-L2364):
  • The computed-member and destructured-key checks don't overlap with the literal member check or the literal vm-runner and definer checks, so nothing is counted twice.
  • A key that may spell constructor is still judged by the constructorRead check, which now also counts inert reads through maySpellAnywhereSink.
  • Both fixes for the earlier review threads on feat(harness-ops): bound computed keys by provenance in the parser reader #12665 are present: the destructured eval/Function key and the documentation of the inert-read exemption.
• Other changes:
  • exportNames, argumentsAliased and inBlock moved to module level and every caller was updated.
  • unknownLoad now takes spells at all three call sites, and the deferred stub in scanOf returns false, which matches its old empty-set result.
  • Nothing else in the plugin uses the old refusal string "the function is used other than by a call" from the parameter path; the other use of that string, at L817, is a different path.
• Documentation and changelog: reference/extraction.md and the changelog fragment match the code. The fragment's bump: minor fits a feature addition, and it states plainly that discarded or tested unknown-key reads now count as function-constructor hits.
• Path-scoped rules: .claude/rules/skill-bodies-state-current-rules.md applies to reference/extraction.md. The added text describes ECMAScript semantics and the reader's own rules, not upstream product specifics, so the rule is met. ruff-pin.md (**/*.py) concerns how ruff is pinned, and the test edits don't touch that.
• Closing issue: the PR says "Refs feat(harness-ops): read Explore/Plan disallowed_tools literal under the parser reader, then flip the inventory default #5901" and closes no issue, so there are no acceptance criteria to check.
• Paved paths: there is no paved-path row in the standards index, so nothing to report.

Not independently verified

The test results in the PR body are the author's claims and I did not check them; this lane has no shell. That covers test_parser_reader.py 77 OK, test_reader_findings.py 87 OK, test_inventory_under_parser.py 312 OK, and five new cases failing with the new check turned off. The host run in "Remaining work" hasn't been done yet, so the rise in function-constructor hits from unknown-key reads in sloppy bundles is still unmeasured.

Security is left to the separate security lane.