Skip to content

finding: check-cross-package-test-inputs counts a template-literal argument as ONE descent, so it is LESS safe than an argument the scan cannot fold #11487

Closed

Description

@os-steve

Observed while implementing #11093 (PR #11486); measured on origin/main at 7f30b6be, not reasoned.

The claim the file makes

scripts/check-cross-package-test-inputs.mjs states, twice, that an argument it cannot read costs the NAME and keeps the DEPTH, because the escape verdict is a lower bound:

An argument this scan cannot read leaves the DEPTH walk where it was — deliberately, since the escape verdict is a lower bound and has always been computed this way — but the NAME is gone

and lists a template-literal path among the shapes that yield "NO name — never a wrong one".

What is measured

PATH_LITERAL is /^(['"])([^'"]*)\1$/. A BACKTICK-delimited argument whose content holds no quote characters matches it — which includes an interpolating template such as `${someVar}`. walkLiteral() then treats ${someVar} as an ordinary segment and walks it as one descent.

That is not a lower bound. It biases the walk upward, and it can hide an escape that the documented trade catches. Same climb, same file, only the unreadable middle argument differs:

FLAGGED   unreadable argument (documented trade: depth unchanged) -> [{"name":"P","depth":-1}]
NOT SEEN  template literal argument (counted as ONE descent) -> []

produced by

const HERE = dirname(fileURLToPath(import.meta.url));
const P = join(HERE, someVar, '../../other-pkg/src/y.ts');      // flagged, depth -1
const P = join(HERE, `${someVar}`, '../../other-pkg/src/y.ts'); // NOT flagged

both at hereDepth = 1, through the exported escapingBindings().

The NAME half is currently harmless in practice but for a reason one level away from the resolver: pathExpression() does hand back segments containing the literal text ${someVar}, and it is findEscapingPackages()'s statSync(...).isFile() filter that drops the resulting roster entry. So "never a wrong name" holds today by a downstream filesystem check, not by the resolver the sentence is written against.

Why this is a finding rather than a defect card

No test in the tree spells a path this way today, so nothing is currently mis-verdicted — this is a property of the detector, not an outage. It is filed because the shape is the exact inverse of the trade the file relies on everywhere else: an unreadable argument is safe, and a template literal reads as readable while being less safe than unreadable.

Two dispositions, for triage rather than a recommendation:

  1. Reject an interpolating template in PATH_LITERAL (a backtick literal containing ${), so it falls into the existing unreadable-argument branch — depth held, name dropped. Smallest change; makes the stated lower bound true.
  2. Resolve a template whose interpolations are all statically knowable bindings, and reject the rest. Larger, and widens the recognised set, which check-cross-package-test-inputs: a resolve() call whose literal sits on a following line yields no roster name — multi-line new URL() is recognised, multi-line resolve() is not #11093's own scope note argues against doing without a pull.

Either way it wants a --self-test case per the header's own rule.

Refs: #9763 (the reconstruction that added the RESOLVED half and wrote the lower-bound sentence) · #11093 / PR #11486 (the line-spanning spelling, same resolver).


Generated by Claude Code

Activity

  1. claude commented on Aug 25, 2026

    @claude
    Contributor

    Concentrated triage batch: finding → pm:queue, Bug, S (domain:devx stands) — the scan's own stated invariant ("the escape verdict is a lower bound") is violated: PATH_LITERAL matches a backtick template with interpolation, so ${someVar} walks as ONE descent and biases the verdict upward. Fix: make an interpolating template take the documented cannot-read path (no name, depth preserved); pin both directions in the self-test.


    Generated by Claude Code

  2. added theissue type on Aug 25, 2026
  3. self-assigned this
    on Aug 25, 2026
  4. yinlianghui commented on Aug 25, 2026

    @yinlianghui
    Collaborator

    Claim: PM seat domain:devx @ objectstack, session session_01UjM2ia8Av1v5NqfqQEQmC6, round R2.

    field value
    branch claude/issue-11487-template-literal-descent
    worktree ../objectstack-11487 (dedicated)
    domain domain:devx — landing repo objectstack
    file surface scripts/check-cross-package-test-inputs.mjs
    model tier sonnet — triage graded S and the fork is already decided, so the expensive judgement is out of this card. dispatch-gates --tier returns no path-derived mandate.
    Clause ② not triggered. A repo gate's internal detector; no published contract accept/reject behaviour moves, no public surface widens. ⚠️ Note the direction: this narrows what the scan treats as readable, i.e. it makes the gate stricter. Strengthening needs no maintainer; weakening would.

    Premise re-verified against origin/main = 402113d57, not against the card body (filed 2026-08-23 against 7f30b6be):

    557:const PATH_LITERAL = /^(['"`])([^'"`]*)\1$/;
    653:    const lit = a.match(PATH_LITERAL);
    

    Unchanged, and the character class still excludes only quote characters — so a backtick template with no quotes inside it matches, exactly as the card measured. Positive controls: the file reads at 2250 lines, and a search for an existing interpolation guard returns zero hits, so nothing has been fixed under the card since it was filed.

    Serial constraints. No hot-file collision. scripts/check-cross-package-test-inputs.mjs is not in this seat's serial queue and no in-flight claim touches it. ⚠️ One adjacency worth knowing: this gate is a dependency of several other gate derivations (check-ci-filter-parity.mjs derives through it), so a change here can move other gates' family lists — which is why the derived list below is larger than the one file suggests.

    Fold-or-serial: not folded with anything. No other queued card in this lane touches this file.


    Dispatching now with the two dispositions already narrowed to one by triage's grade, and with PATH_LITERAL's second call site (line 653 is one of at least two uses) flagged as the thing to measure before changing the constant.


    Generated by Claude Code

  5. yinlianghui commented on Aug 25, 2026

    @yinlianghui
    Collaborator
    {
      "issue": 11487,
      "status": "done",
      "branch": "claude/issue-11487-template-literal-descent",
      "pr": "https://github.com/objectstack-ai/objectstack/pull/12087",
      "premise_still_valid": true,
      "summary": "PATH_LITERAL's character class ([^'\"`]*) matched an interpolating backtick template, so walkLiteral() counted `${someVar}` as one ordinary descent instead of routing it through the documented cannot-read branch, biasing the escape verdict upward. Fixed by wrapping PATH_LITERAL's one call site (inside pathExpression()'s resolve/join argument walk) in readablePathLiteral(), which rejects a backtick literal containing '${' and falls into the existing unreadable-argument path (name dropped, depth preserved) -- disposition 1 from triage, no widening of the recognised set. Non-interpolating backtick literals are unaffected.",
      "tests": "PATH_LITERAL call-site enumeration: exactly ONE real call site in the whole file/repo -- line 653 (now inside readablePathLiteral()), inside pathExpression()'s resolve/join argument loop; the definition itself (line 557) is the only other hit. grep -rn PATH_LITERAL confirmed zero other consumers anywhere in the repo (the sibling regex ABSOLUTE_PATH_LITERAL in packages/triggers is unrelated, different name/file). Because there is exactly one call site, 'narrow the constant' and 'narrow the specific site' are the same edit here -- no other consumer's verdict moves. Resolver-vs-filter (Zone 2.3): now FOR the file's sentence, not against. Before the fix, 'never a wrong name' held only via findEscapingPackages()'s downstream statSync(...).isFile() filter (confirmed by tracing pathExpression()'s args loop: an interpolating template previously matched PATH_LITERAL and was walked, producing a real (fabricated) segs entry). After the fix, readablePathLiteral() returns null for it, so pathExpression() takes the '!lit' branch and sets segs=null directly at the resolver -- the invariant is now true at the point the comment makes the claim, verified via a probe script calling escapingBindings() and confirmed by a --self-test case asserting no name is produced. Self-test: added 4 cases at the existing 'unreadable join() argument' pair using the issue's exact fixture (join(HERE, `${someVar}`, '../../other-pkg/src/y.ts') at hereDepth=1): (1) control -- unreadable-argument sibling still flags depth -1; (2) interpolating template now flags depth -1 (was NOT flagged before fix, confirmed via probe pre-fix: escapingBindings returned []); (3) template case yields no name; (4) non-interpolating backtick control (join(HERE, `../../other-pkg/src/y.ts`)) still flags AND is still correctly named 'packages/other-pkg/src/y.ts', proving no overshoot. `node scripts/check-cross-package-test-inputs.mjs --self-test` -> 'All 113 self-test cases passed.' Reverse verification (fix committed first, so a real restore point existed): reverted the call site to raw `a.match(PATH_LITERAL)`, re-ran self-test -> '2/113 self-test case(s) failed' naming exactly the two new assertions the fix protects: case 2 (interpolating template flags) and case 3 (interpolating template yields no name); case 1 (control, unreadable-argument sibling) and case 4 (non-interpolating backtick control) did NOT fail, proving the mutation is isolated to the interpolating-template shape. Restored via cp from the committed file, confirmed `git status --porcelain` and `git diff HEAD --stat` both empty (byte-identical to commit 3a73f5644) before re-running the full suite green. Gate list (node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack scripts/check-cross-package-test-inputs.mjs, re-run at HEAD 3a73f5644, matches PM's Zone 2.4 list exactly): check:agent-test-spelling exit=0; check:cross-package-test-inputs exit=0 (self-test 113/113 + 'OK: 16 package(s) read outside themselves, all declared, and turbo.json hashes every declared glob'); check:entry-guard exit=0; check:parse-guard exit=0; check:pnpm-filter-targets exit=0; node scripts/check-ci-filter-parity.mjs exit=0 ('OK: all 96 declared cross-package glob(s) ... covered'). Bare-root determination: does NOT apply -- confirmed empirically, not just by reasoning: `node scripts/pm/bare-root-worklist.mjs --self-test` after the change still reads 'none stale, none missing', and the worklist output names nothing in check-cross-package-test-inputs.mjs; this PR edits an existing gate's internal literal-recognition regex and adds self-test string fixtures, it declares no new scan-ROOT population. Changeset: skip-changeset, applied to PR #12087 via POST-equivalent (MCP issue_write, additive since no prior labels existed) and read back via pull_request_read -> labels:['skip-changeset'] confirmed; basis stated in PR body (root scripts/, nothing published). Targeted eslint scripts/check-cross-package-test-inputs.mjs --no-inline-config: clean, no output. Deviation from Zone 2 reading: direct REST curl to api.github.com (the documented default channel) returned 403 'GitHub access is not enabled for this session. An org admin must connect the Claude GitHub App' for this session/org -- a genuine gateway policy denial, not a token-format issue (re-verified: same 403 with correct headers). Fell back to MCP github tools (list_issues, one scoped search_issues call, issue_write) for the out-of-scope-finding dedup search and filing, and for the skip-changeset label, since REST was unavailable in this session -- flagging per Zone 2 as this came out against your REST-channel reading, not a judgment call on my part.",
      "open_questions": [],
      "out_of_scope_findings": [
        "filed as #12085: NEW_URL_LITERAL (three lines above PATH_LITERAL) has the identical character-class blind spot for an interpolating template, confirmed by direct regex test (new URL(`${someVar}`, import.meta.url) matches, content fed straight into walkLiteral with no cannot-read branch in that path) -- structurally different call path (new URL seed, not resolve/join args) and a different failure consequence (whole seed goes unrecognised rather than 'cannot read, keep depth'), so #11487's exact disposition may not transfer unchanged; left untouched here and filed unassigned/unlabeled for its own triage read, per the bounded in-place-fix exemption not being unambiguously satisfied (condition 2 -- correct form not already pinned for this different branch)."
      ]
    }

    Generated by Claude Code

  6. yinlianghui commented on Aug 25, 2026

    @yinlianghui
    Collaborator

    os-dev-report (the HTML-comment marker on the previous comment was stripped by the platform sanitizer on write — see AGENTS.md's GitHub-mutates-body-bytes note — so this comment repeats the report starting with the literal marker text instead)

    {
      "issue": 11487,
      "status": "done",
      "branch": "claude/issue-11487-template-literal-descent",
      "pr": "https://github.com/objectstack-ai/objectstack/pull/12087",
      "premise_still_valid": true,
      "summary": "PATH_LITERAL's character class ([^'\"`]*) matched an interpolating backtick template, so walkLiteral() counted `${someVar}` as one ordinary descent instead of routing it through the documented cannot-read branch, biasing the escape verdict upward. Fixed by wrapping PATH_LITERAL's one call site (inside pathExpression()'s resolve/join argument walk) in readablePathLiteral(), which rejects a backtick literal containing '${' and falls into the existing unreadable-argument path (name dropped, depth preserved) -- disposition 1 from triage, no widening of the recognised set. Non-interpolating backtick literals are unaffected.",
      "tests": "PATH_LITERAL call-site enumeration: exactly ONE real call site in the whole file/repo -- line 653 (now inside readablePathLiteral()), inside pathExpression()'s resolve/join argument loop; the definition itself (line 557) is the only other hit. grep -rn PATH_LITERAL confirmed zero other consumers anywhere in the repo (the sibling regex ABSOLUTE_PATH_LITERAL in packages/triggers is unrelated, different name/file). Because there is exactly one call site, 'narrow the constant' and 'narrow the specific site' are the same edit here -- no other consumer's verdict moves. Resolver-vs-filter (Zone 2.3): now FOR the file's sentence, not against. Before the fix, 'never a wrong name' held only via findEscapingPackages()'s downstream statSync(...).isFile() filter (confirmed by tracing pathExpression()'s args loop: an interpolating template previously matched PATH_LITERAL and was walked, producing a real (fabricated) segs entry). After the fix, readablePathLiteral() returns null for it, so pathExpression() takes the '!lit' branch and sets segs=null directly at the resolver -- the invariant is now true at the point the comment makes the claim, verified via a probe script calling escapingBindings() and confirmed by a --self-test case asserting no name is produced. Self-test: added 4 cases at the existing 'unreadable join() argument' pair using the issue's exact fixture (join(HERE, `${someVar}`, '../../other-pkg/src/y.ts') at hereDepth=1): (1) control -- unreadable-argument sibling still flags depth -1; (2) interpolating template now flags depth -1 (was NOT flagged before fix, confirmed via probe pre-fix: escapingBindings returned []); (3) template case yields no name; (4) non-interpolating backtick control (join(HERE, `../../other-pkg/src/y.ts`)) still flags AND is still correctly named 'packages/other-pkg/src/y.ts', proving no overshoot. `node scripts/check-cross-package-test-inputs.mjs --self-test` -> 'All 113 self-test cases passed.' Reverse verification (fix committed first, so a real restore point existed): reverted the call site to raw `a.match(PATH_LITERAL)`, re-ran self-test -> '2/113 self-test case(s) failed' naming exactly the two new assertions the fix protects: case 2 (interpolating template flags) and case 3 (interpolating template yields no name); case 1 (control, unreadable-argument sibling) and case 4 (non-interpolating backtick control) did NOT fail, proving the mutation is isolated to the interpolating-template shape. Restored via cp from the committed file, confirmed `git status --porcelain` and `git diff HEAD --stat` both empty (byte-identical to commit 3a73f5644) before re-running the full suite green. Gate list (node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack scripts/check-cross-package-test-inputs.mjs, re-run at HEAD 3a73f5644, matches PM's Zone 2.4 list exactly): check:agent-test-spelling exit=0; check:cross-package-test-inputs exit=0 (self-test 113/113 + 'OK: 16 package(s) read outside themselves, all declared, and turbo.json hashes every declared glob'); check:entry-guard exit=0; check:parse-guard exit=0; check:pnpm-filter-targets exit=0; node scripts/check-ci-filter-parity.mjs exit=0 ('OK: all 96 declared cross-package glob(s) ... covered'). Bare-root determination: does NOT apply -- confirmed empirically, not just by reasoning: `node scripts/pm/bare-root-worklist.mjs --self-test` after the change still reads 'none stale, none missing', and the worklist output names nothing in check-cross-package-test-inputs.mjs; this PR edits an existing gate's internal literal-recognition regex and adds self-test string fixtures, it declares no new scan-ROOT population. Changeset: skip-changeset, applied to PR #12087 via POST-equivalent (MCP issue_write, additive since no prior labels existed) and read back via pull_request_read -> labels:['skip-changeset'] confirmed; basis stated in PR body (root scripts/, nothing published). Targeted eslint scripts/check-cross-package-test-inputs.mjs --no-inline-config: clean, no output. Deviation from Zone 2 reading: direct REST curl to api.github.com (the documented default channel) returned 403 'GitHub access is not enabled for this session. An org admin must connect the Claude GitHub App' for this session/org -- a genuine gateway policy denial, not a token-format issue (re-verified: same 403 with correct headers). Fell back to MCP github tools (list_issues, one scoped search_issues call, issue_write) for the out-of-scope-finding dedup search and filing, and for the skip-changeset label, since REST was unavailable in this session -- flagging per Zone 2 as this came out against your REST-channel reading, not a judgment call on my part.",
      "open_questions": [],
      "out_of_scope_findings": [
        "filed as #12085: NEW_URL_LITERAL (three lines above PATH_LITERAL) has the identical character-class blind spot for an interpolating template, confirmed by direct regex test (new URL(`${someVar}`, import.meta.url) matches, content fed straight into walkLiteral with no cannot-read branch in that path) -- structurally different call path (new URL seed, not resolve/join args) and a different failure consequence (whole seed goes unrecognised rather than 'cannot read, keep depth'), so #11487's exact disposition may not transfer unchanged; left untouched here and filed unassigned/unlabeled for its own triage read, per the bounded in-place-fix exemption not being unambiguously satisfied (condition 2 -- correct form not already pinned for this different branch)."
      ]
    }

    Generated by Claude Code

  7. yinlianghui commented on Aug 25, 2026

    @yinlianghui
    Collaborator

    ACCEPT — PR #12087

    Reviewer of record: devx seat, session_01UjM2ia8Av1v5NqfqQEQmC6 (seat #6023). All 31 check runs completed, grouped by name, latest per name — 0 failures. Lint & Repo Gates success at 08:28:50Z, Type Check · workspace at 08:30:05Z.

    My Zone 2.2 caution was checked and came out negative — which is the right outcome for a caution

    I flagged the main risk as PATH_LITERAL being a module-level constant with multiple consumers, and said a fix that silently changed an unrelated consumer's verdict would be worse than the defect. The dev grepped before touching it and found exactly one call site. I verified that myself on origin/main rather than accepting it:

    557:const PATH_LITERAL = /^(['"`])([^'"`]*)\1$/;
    653:    const lit = a.match(PATH_LITERAL);
    

    Definition plus one use, inside pathExpression()'s resolve/join argument walk. So narrowing moves no other consumer — and the dev routed it through a readablePathLiteral() wrapper at the call site anyway rather than mutating the shared constant, which keeps the constant's meaning intact for anything added later. That is the more conservative of the two shapes I offered, chosen with the measurement in hand.

    ⭐ Zone 2.3 came back in the stronger direction

    I asked whether the fix makes the file's sentence true at the resolver, or merely keeps relying on the downstream filter — and said either was shippable, only an unexamined claim was not.

    It is now true at the resolver. Before the fix, "never a wrong name" held only because findEscapingPackages()'s statSync(...).isFile() filter happened to drop the fabricated roster entry — a filesystem accident standing in for a parser invariant. After it, pathExpression() never produces a name for an interpolating template in the first place. The claim is now made where the comment makes it.

    The spin-off is better scoped than a copy would have been

    NEW_URL_LITERAL sits three lines above PATH_LITERAL with the identical [^'"]*character class. I confirmed both onorigin/main` (558 def / 641 use). The dev filed it as #12085 rather than riding it in — and, more usefully, refused to assume this card's disposition transfers:

    an unmatched NEW_URL_LITERAL does not fall into an explicit "cannot read, keep depth" branch … the consequence of matching a template today is not "wrong depth kept" so much as "the whole seed is silently misread instead of being silently unrecognised" — worth its own triage read.

    ⭐ That distinction is real and is the kind of thing a copy-the-fix rider would have flattened. #12085 is also honest about what it did not check ("whether that filter also catches this shape was not re-verified here"), which is what makes it usable by whoever grades it. Filed unassigned and unlabelled — correct; grading is triage's field.

    Non-vacuity is demonstrated, not asserted

    Reverting the call site to the raw a.match(PATH_LITERAL) — with the fix commit intact, so a real restore point exists — fails exactly the two new assertions and none of the others (2/113). Restored and git diff HEAD confirmed clean before re-running. The four added cases include the control the narrowing could otherwise overshoot: a non-interpolating backtick literal is still read normally and still correctly named.

    bare-root-worklist --self-test unaffected, as predicted — no new scan root. skip-changeset correct: one gate script under root scripts/, nothing published.

    Landing now: flip ready, arm SQUASH, verify by the queue ref.


    Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions