test: read the credentials isolation imports from the esbuild graph #45

jercik · 2026-10-05T05:59:00Z

jercik commented

2026-10-05 05:59:00 +00:00

Follow-up to #44 (summary-only claim 01M458MP2PD8H6ZQM3R1NWVR34): the credentials isolation tests now read imports from esbuild's metafile instead of a regex, so wrapped, bare and dynamic imports all count, and the importer test fails unless it sees src/main.ts's import. It targets main with no merge-order constraint, but whichever of this PR and #44 merges second conflicts in src/credentials.test.ts: #44 still calls readFileSync, which this PR drops from the node:fs import, and #46 adds a second esbuild import.

🤖 Generated with Claude Code

Follow-up to #44 (summary-only claim 01M458MP2PD8H6ZQM3R1NWVR34): the credentials isolation tests now read imports from esbuild's metafile instead of a regex, so wrapped, bare and dynamic imports all count, and the importer test fails unless it sees `src/main.ts`'s import. It targets `main` with no merge-order constraint, but whichever of this PR and #44 merges second conflicts in `src/credentials.test.ts`: #44 still calls `readFileSync`, which this PR drops from the `node:fs` import, and #46 adds a second `esbuild` import. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

jercik added 1 commit

2026-10-05 05:59:00 +00:00

test: count wrapped imports in the credentials isolation tests

commit-msg / commitlint (pull_request) Successful in 27s

Details

Checks / quality-checks (pull_request) Successful in 49s

Details

Review / Review (pull_request_target) Successful in 3m21s

Details

9b7961a1c8

The import scan stopped at the first newline after `import`, so an import
the formatter wraps over several lines yielded no specifier and both
isolation tests stayed green. The gap before `from` now crosses lines but
not statements, and the builtins test fails when it finds no import at
all instead of passing with no assertions.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

jercik referenced this pull request

2026-10-05 06:00:48 +00:00

test: fail when a second module reads the capability token #44

forgejo-actions commented

2026-10-05 06:02:20 +00:00

Review 01M45PPWRHFZ0WAZD6ZMSKFZ15 — head 875c6a16fa058763fcce5b5b2593022965cb668e

Review — j4k-oss/review-wrapper @ `bbcbbdc9f6`

Scope: diff against base tree 6fc1cae5b0f0
Status: dispatched — coverage complete (5/5 slots terminal)
Facts: current review-wide projection

Computed under:

{
  "abandonment": "abandonment-v1",
  "anchor_recipe": 1,
  "batch_policy": "batch-v1",
  "coverage": "coverage-v3",
  "dispatch_policy": "dispatch-v2",
  "grounder_version": 1,
  "grounding_read_rule": "grounding-read-v1",
  "promotion_policy": "promotion-v1",
  "report": "report-v4",
  "tally": "tally-v1",
  "triage_settle": "triage-settle-v2"
}

Findings (0)

No findings survived.

Reviewed:

general-bug (whole/default): no-claims
writing-quality (whole/default): no-claims
test-trimming (whole/default): no-claims
restated-sets (whole/default): no-claims
project-docs (whole/default): no-claims

Other claims

grounding-pending (0)
ungrounded (0)
rejected (0)
duplicate-of (0)
unadjudicated (0)

Coverage

Coverage pass: 01M45PQ347RR4GCGJ42YKJZSB9
Accounting: complete
Slot health: healthy

lens	part	arm	unit status	runs	loss
general-bug	whole	default	no-claims	1	no
writing-quality	whole	default	no-claims	1	no
test-trimming	whole	default	no-claims	1	no
restated-sets	whole	default	no-claims	1	no
project-docs	whole	default	no-claims	1	no

**Review** `01M45PPWRHFZ0WAZD6ZMSKFZ15` — head `875c6a16fa058763fcce5b5b2593022965cb668e` # Review — j4k-oss/review-wrapper @ bbcbbdc9f6bd Scope: diff against base tree `6fc1cae5b0f0` Status: dispatched — coverage complete (5/5 slots terminal) Facts: current review-wide projection Computed under: ```json { "abandonment": "abandonment-v1", "anchor_recipe": 1, "batch_policy": "batch-v1", "coverage": "coverage-v3", "dispatch_policy": "dispatch-v2", "grounder_version": 1, "grounding_read_rule": "grounding-read-v1", "promotion_policy": "promotion-v1", "report": "report-v4", "tally": "tally-v1", "triage_settle": "triage-settle-v2" } ``` ## Findings (0) No findings survived. Reviewed: - general-bug (whole/default): no-claims - writing-quality (whole/default): no-claims - test-trimming (whole/default): no-claims - restated-sets (whole/default): no-claims - project-docs (whole/default): no-claims ## Other claims - grounding-pending (0) - ungrounded (0) - rejected (0) - duplicate-of (0) - unadjudicated (0) ## Coverage Coverage pass: 01M45PQ347RR4GCGJ42YKJZSB9 Accounting: complete Slot health: healthy | lens | part | arm | unit status | runs | loss | | --- | --- | --- | --- | --- | --- | | general-bug | whole | default | no-claims | 1 | no | | writing-quality | whole | default | no-claims | 1 | no | | test-trimming | whole | default | no-claims | 1 | no | | restated-sets | whole | default | no-claims | 1 | no | | project-docs | whole | default | no-claims | 1 | no |

🚀 1

forgejo-actions reviewed

2026-10-05 06:02:21 +00:00

src/credentials.test.ts Outdated

					
				@ -5,2 +5,3 @@

				const IMPORT_SPECIFIER = /(?:^|\n)\s*(?:import|export)[^\n]*?from\s+"(?<specifier>[^"]+)"/gu;

				// The gap before `from` crosses lines but not statements, so a wrapped import counts.

				const IMPORT_SPECIFIER = /(?:^|\n)\s*(?:import|export)\b[^;]*?\bfrom\s+"(?<specifier>[^"]+)"/gu;

low — "imports only node: builtins" cannot see side-effect or dynamic imports, because IMPORT_SPECIFIER requires a from clause

The isolation test misses two ways credentials.ts could import a wrapper module: a side-effect import (import "./review/api.ts";) or a dynamic import (await import("./review/api.ts")). The pattern requires \bfrom\s+"...", so importSpecifiers never returns either specifier. Each loads wrapper code into the token module, and the test stays green. The source comment in src/credentials.ts says the module "must import nothing from the rest of the wrapper (enforced by src/credentials.test.ts)", so this test is the stated enforcement.

The test reads const specifiers = importSpecifiers(readFileSync("src/credentials.ts", "utf8")); expect(specifiers).not.toStrictEqual([]); for (const specifier of specifiers) { expect(specifier).toMatch(/^node:/u); }. Today credentials.ts has one import, import process from "node:process";, which is why the new non-empty guard passes.

Fix: widen the pattern to capture bare and dynamic forms too. For example, add the alternatives (?:^|\n)\s*import\s*"(?<bare>[^"]+)" and \bimport\s*\(\s*"(?<dyn>[^"]+)", and map whichever group matched. Keep the existing from branch so wrapped static imports still count. The importer-isolation test shares this helper, so the fix also covers modules that would load credentials.ts through those forms.

I ran the subject's regex in node on these inputs. import process from "node:process";\nimport "./main.ts";\n gave ['node:process']. import process from "node:process";\nconst m = await import("./main.ts");\n gave ['node:process']. In both, every specifier matches ^node:, so the test would pass. I did not run vitest because there is no node_modules in the sandbox. I checked oxlint.config.ts and knip.json at the top level only and found no rule that restricts which modules credentials.ts may import. A lint rule that already forbids non-node imports in src/credentials.ts would reduce this claim to a documentation gap.

lens test-trimming · arm default · tally 1 valid / 0 invalid / 0 uncertain
claim 01M45ADFW9C9DGREXVVBFP565W of review 01M45A9VH96K648J2TW06EQQ8Q

**low** — "imports only node: builtins" cannot see side-effect or dynamic imports, because IMPORT_SPECIFIER requires a `from` clause > The isolation test misses two ways credentials.ts could import a wrapper module: a side-effect import (`import "./review/api.ts";`) or a dynamic import (`await import("./review/api.ts")`). The pattern requires `\bfrom\s+"..."`, so `importSpecifiers` never returns either specifier. Each loads wrapper code into the token module, and the test stays green. The source comment in src/credentials.ts says the module "must import nothing from the rest of the wrapper (enforced by src/credentials.test.ts)", so this test is the stated enforcement. > > The test reads `const specifiers = importSpecifiers(readFileSync("src/credentials.ts", "utf8")); expect(specifiers).not.toStrictEqual([]); for (const specifier of specifiers) { expect(specifier).toMatch(/^node:/u); }`. Today credentials.ts has one import, `import process from "node:process";`, which is why the new non-empty guard passes. > > Fix: widen the pattern to capture bare and dynamic forms too. For example, add the alternatives <code>(?:^|\n)\s*import\s*"(?<bare>[^"]+)"</code> and <code>\bimport\s*\(\s*"(?<dyn>[^"]+)"</code>, and map whichever group matched. Keep the existing `from` branch so wrapped static imports still count. The importer-isolation test shares this helper, so the fix also covers modules that would load credentials.ts through those forms. > > I ran the subject's regex in node on these inputs. `import process from "node:process";\nimport "./main.ts";\n` gave `['node:process']`. `import process from "node:process";\nconst m = await import("./main.ts");\n` gave `['node:process']`. In both, every specifier matches `^node:`, so the test would pass. I did not run vitest because there is no node_modules in the sandbox. I checked oxlint.config.ts and knip.json at the top level only and found no rule that restricts which modules credentials.ts may import. A lint rule that already forbids non-node imports in src/credentials.ts would reduce this claim to a documentation gap. lens `test-trimming` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain claim `01M45ADFW9C9DGREXVVBFP565W` of review `01M45A9VH96K648J2TW06EQQ8Q`

👍 1

Fixed in 07baca3. The tests no longer scan source with a regex. They read the import graph from esbuild's metafile, which records every way one module loads another: wrapped and bare static imports, re-exports, and dynamic imports of a literal path. I checked it by adding import "./review/api.ts"; and then await import("./review/api.ts") to src/credentials.ts: "imports only node: builtins" failed both times and passed again once I reverted them.

Fixed in 07baca3. The tests no longer scan source with a regex. They read the import graph from esbuild's metafile, which records every way one module loads another: wrapped and bare static imports, re-exports, and dynamic imports of a literal path. I checked it by adding `import "./review/api.ts";` and then `await import("./review/api.ts")` to `src/credentials.ts`: "imports only node: builtins" failed both times and passed again once I reverted them.

jercik marked this conversation as resolved

jercik added 1 commit

2026-10-05 06:22:11 +00:00

test: read credentials imports from esbuild's import graph

commit-msg / commitlint (pull_request) Successful in 23s

Details

Checks / quality-checks (pull_request) Successful in 49s

Details

Review / Review (pull_request_target) Successful in 1m50s

Details

07baca3a27

The regex scan required a `from` clause, so a side-effect import or a
dynamic import in credentials.ts passed the builtins test. The importer
test also passed when the scan saw no import of credentials.ts at all.
esbuild's metafile lists every import a module loads, in any of those
forms, and the importer test now expects exactly src/main.ts, so a scan
that stops seeing that import fails.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

jercik commented

2026-10-05 06:24:22 +00:00

Replying to comment #126357

Round 1, review 01M45A9VH96K648J2TW06EQQ8Q of 9b7961a, all five slots delivered. Both findings are fixed in 07baca3, which reads imports from esbuild's metafile instead of a regex:

01M45AD4SYSDWA3FZ4A93303E8 (summary only): the importer test now asserts toStrictEqual(["src/main.ts"]), so main.ts's import is a positive control. Removing that import from src/main.ts makes the test fail.
01M45ADFW9C9DGREXVVBFP565W: answered in thread #126358.

> Replying to comment #126357 Round 1, review `01M45A9VH96K648J2TW06EQQ8Q` of `9b7961a`, all five slots delivered. Both findings are fixed in 07baca3, which reads imports from esbuild's metafile instead of a regex: - `01M45AD4SYSDWA3FZ4A93303E8` (summary only): the importer test now asserts `toStrictEqual(["src/main.ts"])`, so main.ts's import is a positive control. Removing that import from `src/main.ts` makes the test fail. - `01M45ADFW9C9DGREXVVBFP565W`: answered in thread #126358.

jercik changed title from ~~test: count wrapped imports in the credentials isolation tests~~ to test: read the credentials isolation imports from the esbuild graph

2026-10-05 06:24:39 +00:00

forgejo-actions reviewed

2026-10-05 06:26:29 +00:00

src/credentials.test.ts

					
				@ -172,4 +191,4 @@

				    }

				  });

				  it("is imported by no wrapper module other than src/main.ts", () => {

low — Test name "is imported by no wrapper module other than src/main.ts" no longer matches its assertion, which now also requires src/main.ts to import credentials.ts

The test now fails in a case its name says is allowed: no module imports src/credentials.ts at all. That happens if src/main.ts drops its import, for example when token acquisition moves behind the OIDC exchange that the src/credentials.ts header comment anticipates. The failure line shows only the name, and the name says the test guards against extra importers. A developer reading it can decide the test is wrong and loosen the assertion, when the test is actually reporting a missing importer.

The diff replaced the body. The old body filtered out src/main.ts and asserted the rest was [], which matched the name. The new body asserts expect(importers).toStrictEqual(["src/main.ts"]);, which requires exactly one importer and requires it to be src/main.ts. The name was left as it was.

Correction: rename the test so it states both halves of the assertion: it("is imported by src/main.ts and no other wrapper module", ...). This keeps the boundary the old name stated and adds the requirement the new assertion enforces. The sibling test keeps the name "imports only node: builtins". Its new expect(specifiers).not.toStrictEqual([]) only stops the test from passing vacuously, so that name still describes it.

What I examined: the diff and the whole of src/credentials.test.ts, the import of ./credentials.ts in src/main.ts (the only non-test importer under src, found by grep), and the header comment of src/credentials.ts. I did not run vitest, because node_modules is absent. The claim rests on reading the assertion against the name.

lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain
claim 01M45BRYXYXD3CFKXYP142GWYD of review 01M45BMA1MQ417JKE8C6S91G0S

**low** — Test name "is imported by no wrapper module other than src/main.ts" no longer matches its assertion, which now also requires src/main.ts to import credentials.ts > The test now fails in a case its name says is allowed: no module imports `src/credentials.ts` at all. That happens if `src/main.ts` drops its import, for example when token acquisition moves behind the OIDC exchange that the `src/credentials.ts` header comment anticipates. The failure line shows only the name, and the name says the test guards against extra importers. A developer reading it can decide the test is wrong and loosen the assertion, when the test is actually reporting a missing importer. > > The diff replaced the body. The old body filtered out `src/main.ts` and asserted the rest was `[]`, which matched the name. The new body asserts `expect(importers).toStrictEqual(["src/main.ts"]);`, which requires exactly one importer and requires it to be `src/main.ts`. The name was left as it was. > > Correction: rename the test so it states both halves of the assertion: `it("is imported by src/main.ts and no other wrapper module", ...)`. This keeps the boundary the old name stated and adds the requirement the new assertion enforces. The sibling test keeps the name "imports only node: builtins". Its new `expect(specifiers).not.toStrictEqual([])` only stops the test from passing vacuously, so that name still describes it. > > What I examined: the diff and the whole of `src/credentials.test.ts`, the import of `./credentials.ts` in `src/main.ts` (the only non-test importer under `src`, found by grep), and the header comment of `src/credentials.ts`. I did not run vitest, because `node_modules` is absent. The claim rests on reading the assertion against the name. lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain claim `01M45BRYXYXD3CFKXYP142GWYD` of review `01M45BMA1MQ417JKE8C6S91G0S`

🚀 1

jercik marked this conversation as resolved

jercik referenced this pull request

2026-10-05 06:27:17 +00:00

test: find capability-token reads in esbuild output, not by tokenizing #46

jercik referenced this pull request

2026-10-05 06:27:30 +00:00

test: find capability-token reads in esbuild output, not by tokenizing #46

jercik referenced this pull request

2026-10-05 06:41:37 +00:00

test: name both halves of the credentials importer assertion #47

jercik commented

2026-10-05 06:41:49 +00:00

Replying to review comment #126678

Tracked in #47, which is stacked on this PR. It renames the test to "is imported by src/main.ts and no other wrapper module", as you suggested. This is round 2 and the test still checks the right thing under its old name, so the rename goes in #47 instead of here.

> Replying to review comment #126678 Tracked in https://code.j4k.dev/j4k-oss/review-wrapper/pulls/47, which is stacked on this PR. It renames the test to "is imported by src/main.ts and no other wrapper module", as you suggested. This is round 2 and the test still checks the right thing under its old name, so the rename goes in #47 instead of here.

jercik commented

2026-10-05 06:41:52 +00:00

Replying to comment #126357

Round 2, review 01M45BMA1MQ417JKE8C6S91G0S of 07baca3, all five slots delivered. Its one finding, 01M45BRYXYXD3CFKXYP142GWYD (thread #126678, the test name that no longer matches its assertion), is tracked in #47, stacked on this PR.

> Replying to comment #126357 Round 2, review `01M45BMA1MQ417JKE8C6S91G0S` of `07baca3`, all five slots delivered. Its one finding, `01M45BRYXYXD3CFKXYP142GWYD` (thread #126678, the test name that no longer matches its assertion), is tracked in https://code.j4k.dev/j4k-oss/review-wrapper/pulls/47, stacked on this PR.

jercik force-pushed test/wrapped-import-scan from 07baca3a27

commit-msg / commitlint (pull_request) Successful in 23s

Details

Checks / quality-checks (pull_request) Successful in 49s

Details

Review / Review (pull_request_target) Successful in 1m50s

Details

to 875c6a16fa

commit-msg / commitlint (pull_request) Successful in 22s

Details

Checks / quality-checks (pull_request) Successful in 46s

Details

Review / Review (pull_request_target) Successful in 3m11s

Details

2026-10-05 09:35:50 +00:00

Compare

jercik merged commit 65c314dcfa into main

2026-10-05 09:36:52 +00:00

jercik referenced this pull request from a commit

2026-10-05 09:36:53 +00:00

test: read the credentials isolation imports from the esbuild graph (#45)

Sign in to join this conversation.

No reviewers

No labels

No milestone

No assignees

2 participants

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

j4k-oss/review-wrapper!45

Rows
Columns