Skip to content

fix(scripts): check-org-identifier takes the shared string-aware comment mask - #9496

Merged
os-steve merged 1 commit into
mainfrom
claude/issue-9444-org-identifier-comment-mask
Aug 18, 2026
Merged

fix(scripts): check-org-identifier takes the shared string-aware comment mask#9496
os-steve merged 1 commit into
mainfrom
claude/issue-9444-org-identifier-comment-mask

Conversation

@claude

@claudeclaudeBot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Fixes#9444

The seventh gate in the family PR #9445 fixed. scripts/check-org-identifier.mjs now
takes maskComments from the shared scripts/js-comment-mask.mjs instead of answering
"comment or code?" itself. That shared module is not touched by this PR.

The defect, and the one the card did not name

The card names the truncation:

constcode=line.replace(/\/\/.*$/,'');

It cuts at the first doubled slash on the line, whatever that slash is, so a URL or
any slash-bearing string literal deletes the rest of its own line — including the very
session.tenantId read the gate exists to catch. Silent under-reporting: the gate prints
OK over a line it truncated.

Reading the whole script found the mirror defect, which the card does not mention. The
guard above the truncation is trimmed.startsWith('*' | '//' | '/*'), so only a line whose
first non-space characters open a comment counted as one. An interior line of a block
comment (opener on one line, prose with no leading star on the next) and a trailing
/* … */ on a code line both read as live code — the gate would manufacture a finding
out of prose. So this gate belongs to both families in #9445's table at once: it blinds
and it fabricates. One mask closes both, and both are pinned in --self-test.

Those two are the whole of the comment handling in this script; there is no third
hand-rolled parser in it.

Reachability: LATENT, and structurally so

Measured on af2a989be, over this gate's own corpus (2051 author-facing source files),
this gate's old strip diffed against the shared masker:

measurementresult
files where the two projections disagree on text271
files where the gate's verdict changes0
corpus lines naming session.tenantId10 — every one inside a comment; old and new agree on all ten
executable session.tenantId reads in the corpus0

So: LATENT, not live. And it is worth being precise about why, because for this gate
"latent" is not luck. This is a hard-fail zero-occurrence guard — a corpus that holds
the hazard is a corpus in which the gate is already red. The intersection the card asks
for (doubled-slash-in-a-string × a real read, on one line) can only ever be empty while
the gate is green, so an empty intersection is not evidence about the mechanism. The
measurement that carries information is the near miss, and that half is everywhere:

  • 665 lines across 181 files carry a doubled slash inside a string, template or regex
    literal.

The day one of those lines also carries the removed read, the old gate goes quiet. That is
the same conclusion #9445 reached for its two latent gates, arrived at from the other side.

Projection: maskComments, measured against the alternative

This gate reports a file and a line, so it takes the blanking projection — the masked text
stays byte-aligned with the source and line i is still line i. The card flags #9367's
51x cliff (6.4s → 5m27s), which came from dragging a lazy[\s\S]*? across the
whitespace blanking leaves behind. This gate's matcher is a short anchored pattern run per
line, so it is not exposed to that. Measured anyway, same corpus, best of 3, scan + match
only:

projectiontimeoffenders
old per-line .replace195 ms0
maskComments (blank)2088 ms0
stripComments (delete)1275 ms0

No cliff — the ~2s is the scanner's own linear cost over 29 MB, and both projections return
the same verdict here. Whole-gate wall clock goes 0.41s → 2.39s; flagging that plainly
since a gate quietly getting slower is a cost its own green never shows.

The verification that matters

A green run over a corpus with zero occurrences proves nothing at all here, so the gate was
driven end to end against a planted fixture (tracked, so git ls-files picks it up),
once with the original script from origin/main and once with this one:

planted fixtureOLDNEWdirection proven
const docs = 'https://objectstack.ai/docs/hooks'; return ctx.session.tenantId;exit 0 — "OK … no removed alias"exit 1 — quotes the lineblinds
a block-comment interior line + a trailing /* … */, both naming the aliasexit 1 — 2 occurrencesexit 0fabricates

Note the second row runs the reverse-verification backwards from the usual template:
ablating the fix turns the gate red, not green, because on that shape the defect
invents findings rather than dropping them. Both fixtures were removed from the index and
the tree afterwards.

The shapes themselves are pinned in a new --self-test (14 cases), the way every other
gate in this tree pins its matcher, and check:org-identifier now runs it first — the
convention already used by check:error-code-casing, check:nul-bytes and ~60 others. An
unwired self-test is the gap lint.yml already calls out for dispatch-gates. One drafted
case was discarded because the reference disagreed with it: a bare * … line with no
opener above it is not a comment in JavaScript, only in the old heuristic, so the case was
rewritten as a real docblock.

Two deliberate non-changes, both of which a naive conversion gets wrong:

  • The os-allow-tenant-id waiver keeps being read from the raw line. It lives in a
    comment, and the mask blanks comments — testing the masked line for it would silently
    revoke every waiver in the tree. (There are none in the corpus today; that is exactly why
    a green run would not have caught it.)
  • String contents stay in scope. The masker leaves literals intact, so an authoring sample
    that spells the removed alias inside a template is still a finding. That was already the
    semantics whenever no doubled slash preceded it; it is now consistent, and the corpus has
    zero such lines (the 0-verdict-change row above).

CI

check:org-identifieris a CI gate — .github/workflows/lint.yml, step "Org-identifier
authoring guard". Unlike check:platform-checklist in #9445, this one's green is CI's, not
only mine.

Gates run locally, on ed6f91066

Families re-derived from the actual changed paths with
node scripts/pm/dispatch-gates.mjs scripts/check-org-identifier.mjs package.json — it
places check:org-identifier and nothing else.

  • node scripts/check-org-identifier.mjs --self-test — 14 cases pass
  • node scripts/check-org-identifier.mjs — OK, 2051 files, exit 0
  • node scripts/check-nul-bytes.mjs --self-test && node scripts/check-nul-bytes.mjs — OK,
    6130 files, no raw control bytes
  • node scripts/js-comment-mask.mjs --self-test — 15 cases pass (unchanged module, run to
    show the shared instrument is still green under its new consumer)

No changeset: root scripts/ plus one root package.json script line, nothing published.

The red check on this PR is #9350, not this diff

Temporal Conformance (live PG + MySQL) is red on this PR. It is the already-open flake
card #9350 — same file, same describe block:

 FAIL src/sql-driver-datetime-mysql-storage.test.ts > os migrate plan lists the MySQL widening (#3954) > applies exactly what it planned, and then finds nothing left
Error: Test timed out in 5000ms.
❯ src/sql-driver-datetime-mysql-storage.test.ts:277:3
Test Files 1 failed | 104 passed (105)
Tests 1 failed | 2283 passed (2284)

It is a timeout, not an assertion mismatch: that case does initObjects
previewDeferredSchemaWorkflushDeferredSchemaDdl (an ALTER … MODIFY full table
rebuild) → initObjects → re-plan, all on vitest's default 5000ms because the file sets no
explicit timeout. The two files in that package that do set one use 40s and 60s. Full
evidence, including why this occurrence discriminates between #9350's two hypotheses, is
posted on that card; this is the first PR-side occurrence of a pattern recorded there as
queue-only.

Why it cannot be this diff: the failing step runs pnpm --filter @objectstack/driver-sql test
(vitest inside packages/drivers/driver-sql). This PR adds no code under packages/;
scripts/check-org-identifier.mjs is imported by nothing outside itself, and the changed
root package.json line is the value of check:org-identifier, which that step never
invokes. Turbo's global hash does include the root package.json, so this job likely
rebuilt cold instead of restoring cache — but that is step 11, which passed, and step 12
does not invoke turbo at all. driver-sql's vitest config aliases @objectstack/spec and
@objectstack/core to their src trees, so the test step transforms TypeScript from
source and its workload is identical on a cache hit or a miss.

The one line outside the claimed file surface is that package.json entry, which runs the
new --self-test. It is kept deliberately — a self-test nothing runs is the gap lint.yml
already names for dispatch-gates — and reverting it is a one-line change if the surface
fence should win instead.

Generated by Claude Code


Generated by Claude Code

…ent mask
The gate decided "comment or code?" per line, with a `trimmed.startsWith`
triple and `line.replace(/\/\/.*$/, '')`. That truncates at the FIRST
doubled slash on the line whatever it is, so a URL or any slash-bearing
string literal deleted the rest of its own line -- including the very
`session.tenantId` read the gate exists to catch. Silent under-reporting:
it printed OK over a line it had truncated.
It was wrong in the mirror direction too, which the card did not name:
only a line STARTING with `*`, `//` or `/*` counted as a comment, so an
interior line of a block comment and a trailing `/* … */` on a code line
both read as live code and would have been reported as findings.
Both directions go away with `maskComments` from scripts/js-comment-mask.mjs
(#9367), which is string-, template- and regex-aware and blanks in place, so
the reported line number stays true. The waiver marker keeps being read from
the RAW line: it lives in a comment, and testing the masked line for it would
silently revoke every waiver in the tree.
Measured on 2051 author-facing files: the two projections disagree on the
text of 271 files, the gate's verdict on 0 -- all 10 corpus lines naming
the alias are comments, so the defect is LATENT. Structurally so: this is a
zero-occurrence hard-fail guard, so a corpus holding the hazard is a corpus
where the gate is already red. The near-miss half is everywhere -- 665 lines
in 181 files carry a doubled slash inside a literal.
The 14 shapes are pinned in a new `--self-test`, wired into
`check:org-identifier` the way every other gate in this tree wires its own.
Fixes#9444
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XqDQYVU5smx29ts9pAErja
@claudeclaudeBot added the skip-changeset PR has no user-facing published change; bypasses the changeset gate label Aug 18, 2026
@github-actionsgithub-actionsBot added size/m dependencies Pull requests that update a dependency file labels Aug 18, 2026
@os-steve
os-steve marked this pull request as ready for review August 18, 2026 05:58
@os-steve
os-steve added this pull request to the merge queueAug 18, 2026
Merged via the queue into main with commit 502a028Aug 18, 2026
44 of 45 checks passed
@os-steve
os-steve deleted the claude/issue-9444-org-identifier-comment-mask branch August 18, 2026 06:19
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependenciesPull requests that update a dependency filesize/mskip-changesetPR has no user-facing published change; bypasses the changeset gate

Projects

None yet

2 participants

@os-steve@claude