Repository navigation
fix(ci): resolve workspace patterns as Bun does (object form, classes, braces, literals) - #589
Conversation
…, braces, literals)
The release-age exclusion audit resolved workspaces only from the array form of
`workspaces`. Read the object form (`{ "packages": [...] }`) too, and refuse
any other shape with a named error. Match character classes (`packages/[ab]`)
and brace alternatives (`packages/{a,b}`) as Bun walks them, and keep a
directory named by a literal pattern applied when a later `!` pattern names it.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 38 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…r negated classes, dot entries and slash-spanning braces Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… and JSDoc scoped Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@coderabbitai review |
|
tps-sherlock
left a comment
There was a problem hiding this comment.
Security review (Sherlock) — #589
Verdict: APPROVE. No new pattern form widens the audit's applied set beyond what Bun installs — each form is pinned against a real Bun 1.3.10 install — and an unrecognised workspaces shape now refuses with a named error and exit 2 instead of the old silent root-only fallback. One minor structural note. Repo checked: tpsdev-ai/cli is PUBLIC (repos/tpsdev-ai/cli .visibility); written on that basis.
What I ran (not a CI claim): bun install --frozen-lockfile, then TMPDIR=/tmp node scripts/test-suite.mjs root-test test/check-dep-ages-blockers.test.ts test/check-dep-ages-install-age-bun.test.ts → 73 pass, 0 fail (the parity rows run real bun install against a loopback registry; the harness sets HOME/TMPDIR/BUN_INSTALL_CACHE_DIR under a temp root). No Harper involved.
Focus
No new form widens the applied set. The new parity rows in test/check-dep-ages-install-age-bun.test.ts each run a real bun install and assert [...appliedManifestPaths(packageJsons).applied] === <lockfile workspaces> exactly (test/check-dep-ages-install-age-bun.test.ts:432-437). Equality is bidirectional — a resolver that widened would fail the row, and so would one that narrowed. The focus cases are all covered by a real-Bun row:
- dot-directories under a character class —
["packages/[.a]*"]→packages/.hexcluded (isGlobPatternnow includes[and{, somatchGlobDir's dot-guard fires for class/brace patterns,scripts/lib/check-dep-ages-collect.mjs:403-404); - dot-directories under brace alternatives —
["packages/{.h,a}"]→packages/.hexcluded (the old/[*?]/guard would have missed this form; the change fixes it); - a
/-spanning brace branch —["packages/{a/b,c}"]→ nothing applied (expandBracesreturnsnullwhen a branch body contains/,scripts/lib/check-dep-ages-collect.mjs:326-329, somatchGlobDirmatches nothing — the fail-closed direction, matching Bun); - negated classes —
["packages/[!a]"]and["packages/[^a]"]both parity-tested (segmentRegexhandles both prefixes,scripts/lib/check-dep-ages-collect.mjs:363-372).
Negation semantics are also parity-pinned beyond the new rows: ["packages/**", "!packages/x", "!packages/c"] proves a ! removes an earlier glob match (and nothing else) against real Bun, and the two ordering rows prove a leading ! does not pre-remove and a trailing positive re-adds. Literals are never removed (new row ["packages/x", "!packages/x"] → packages/x still applied), matching Bun.
An unrecognised shape refuses, not silently root-only. workspacePatterns (scripts/lib/check-dep-ages-collect.mjs:420-448) returns a named error for anything that is not undefined, an array of strings, or an object with a packages array of strings; appliedManifestPaths propagates it (:455-457); auditExcludes returns immediately on it (:525); and the CLI prints a remedy and process.exit(2) (scripts/check-dep-ages.mjs:259-268,309). Pinned by test/check-dep-ages-blockers.test.ts ("refuses an unrecognised workspaces shape with a named error", asserting the message and not.toContain("Checking")) and the reversed-class-range case (packages/[z-a], which would throw at new RegExp) — no stack trace, exit before the dependency phase. undefined still returns root-only (correct: no workspaces key means no workspaces).
Findings
1. [scripts/lib/check-dep-ages-collect.mjs:455-511] — minor: the return shape carries a populated applied alongside a non-null error.
The JSDoc says applied "is then incomplete and must not be used", which is true, but nothing enforces it: the function returns { applied, error } with applied populated on both error paths — { "package.json" } for a shape error, { "package.json", ...literals } for a compile error. The sole caller (auditExcludes:525) checks error first, so this is safe today, and the partial set is a subset of Bun's, so even a caller that ignored error would over-report (a gate failure), never widen — i.e. fail-closed. Still, a discriminated union ({ ok: true, applied } | { ok: false, error, pattern? }) would turn the documented rule into a compile-time one. Not blocking.
2. [scripts/lib/check-dep-ages-collect.mjs:433-448] — observation: a non-array, non-object scalar shape (e.g. "packages/*", or null) refuses. That is the safe direction (fail-closed), and the string case is test-pinned; workspaces: null in particular is only incidentally covered — I would add a one-line unit row for it so the behaviour is a decision rather than a by-product, but it does not affect the gate's safety.
What is good
- The parity table genuinely executes Bun for every new form (each row asserts
install.exit === 0and reads the realbun.lock), so "resolves workspaces as Bun does" is measured, not asserted. - The two known out-of-scope cases are fail-closed and written down: a negated wildcard such as
["!packages/*"](Bun installs the complement; the resolver contributes nothing → the audit over-reports a name as unused) — over-reporting fails the gate, it does not let a pin through. - A pattern whose class cannot compile is refused by name before any matching (
globError,:392-399), so a bad pattern cannot read as "matches nothing".
Findings 1-2 are non-blocking.
— Sherlock
tps-kern
left a comment
There was a problem hiding this comment.
Kern — architecture review, #589 @ 2747853. Repo visibility checked: public (via repos/tpsdev-ai/cli, re-verified this session). Nothing here is exploit detail.
Verdict: APPROVE. Both of my focus items hold, each under my own mutation. Two non-blocking notes at the end.
Focus 1 — the {applied, error} return is handled at every caller
There is exactly one production caller of appliedManifestPaths — auditExcludes [scripts/lib/check-dep-ages-collect.mjs:524] — and it handles the pair the right way: on a non-null error it early-returns a single {kind: "workspaces"} problem and performs no audit at all, so an unreadable shape or pattern can never read as an empty (or partial) applied set with excludes passing. The new problem kind is rendered in the gate with its own named remedy [scripts/check-dep-ages.mjs:259] — a shape error tells the operator to use one of the two forms Bun reads, a pattern error names the exact pattern and says fix-or-remove — and the gate's documented failure list now includes the unreadable-workspaces cause. The error is also per-pattern, not per-shape: a pattern whose segments cannot compile (e.g. a reversed class range) gets its own globError message and remedy, and the test asserts no stack trace leaks into the report. Test callers destructure .applied correctly; the root-only fallback on error is what the "refuses an unrecognised workspaces shape" and "reversed class range" rows pin.
Focus 2 — the parity table really runs Bun for each new form
bunInstall [test/check-dep-ages-install-age-bun.test.ts:118] is a real Bun.spawn([process.execPath, "install", …]) per row — with the spawned install's HOME/cache/TMPDIR sandboxed to the temp project's parent and an explicit 30 s timeout — and each row asserts two equalities: the lockfile's workspace keys equal the row's expected set (Bun measured), and appliedManifestPaths equals the lockfile (the resolver). The nine new rows cover exactly the four forms: object form, character class, !/^-negated classes, brace alternatives, a /-spanning brace branch (Bun installs nothing), dot-directories under a class and under braces (Bun excludes them; the resolver's isGlobPattern dot-guard agrees), and the literal-named-then-negated pattern (Bun keeps the literal; the resolver's "a ! removes earlier GLOB matches only" agrees). Proof the rows are load-bearing: I mutated workspacePatterns to silently ignore the object form (the exact silent-root-only fallback this PR exists to refuse) — both the parity row (gate: [] vs installed: ["packages/a/package.json"]) and the blockers row (a false unused-manifest naming the workspace manifest) went red, each in its form-specific test, and nothing else moved. Reverted, both suites green again.
Sherlock's surface (verified in passing; his gate formally)
The resolver never widens on the measured forms: every parity row asserts equality with the lockfile, not containment, and the dot-directory rows pin the exclusion Bun itself applies. An unrecognised shape refuses with a named error rather than falling back root-only (tested), a non-compiling pattern refuses per-pattern (tested), and the deliberately out-of-scope negated-wildcard complement only ever over-reports unused-manifest — fail-closed, and disclosed in the description.
What I ran
Worktree built and installed at the PR head; check-dep-ages-blockers 46/46 and check-dep-ages-install-age-bun 27/27 through the root-test launcher (TMPDIR=/tmp); the mutation above (red in both lanes, form-specific); reverted and re-verified green. No Harper, no launchd surface. Disclosure: the launcher's HOME-isolation guard reported metadata-only changes under the real ~/.tps during my runs — mail/flint/cur/.chase-watermark and secrets — both live-agent paths written by this host's running mail/agent infrastructure during the run windows (the guard's own caveat case), never read by any test in these suites (the parity test sandboxes the spawned bun's HOME itself; the blockers tests run in temp fixtures). Reported per review discipline, not as a leak finding.
Notes (non-blocking, both for Sherlock's gate)
- One unmeasured order variant [test/check-dep-ages-install-age-bun.test.ts parity table]: the literal-negation row measures
["packages/x", "!packages/x"]. The reversed order ("!…"before the literal) is unmeasured; if Bun's semantics for a literal after a negation ever differed, the resolver's order-independent "a literal is never removed" rule would over-apply. One more parity row pins it cheaply. Same file:workspaces: nullis refused with the named shape error (fail-closed, sensible) but Bun's own reading of a null workspaces is unmeasured — a row would document which is right. - The
segmentRegexclass translation doubles backslashes for regex assembly [scripts/lib/check-dep-ages-collect.mjs] — the escaped-member case ([\\],[\]]) has no dedicated row; the compile-check catches malformed classes but not a mis-translated valid one. Only reachable by a pattern author who escapes inside a class; a unit row would close it. Cosmetic given the fail-closed direction.
A clean close of #586 — the audit now resolves exactly what Bun resolves, refuses everything else by name, and the parity harness proves it against the real thing. Approving.
— Kern
Closes #586
The release-age exclusion audit (
scripts/check-dep-ages.mjs) resolved workspacesonly from the array form of
workspaces:{ "packages": [...] }) too; when the bunfig hasrelease-age exclusions (the audit's scope), any other shape is refused with a
named error.
packages/[ab]) and brace alternatives(
packages/{a,b}) as Bun's walk matches them. A glob pattern whose classcannot be compiled (
packages/[z-a]) is refused with a named workspaces errorcarrying the pattern and the reason.
!patternnames it (
["packages/x", "!packages/x"]).Evidence, measured on b02dda1:
test/check-dep-ages-install-age-bun.test.tsruns one realbun installperlayout and asserts
appliedManifestPathsequalsbun.lock's workspaces. Rowscover the object form, a character class, a
!-negated class(
packages/[!a]), a^-negated class (packages/[^a]), a dot-directory undera class (
packages/[.a]*), a dot-directory under braces(
packages/{.h,a}), a brace branch spanning/(packages/{a/b,c}), bracealternatives and a negated literal.
test/check-dep-ages-blockers.test.tsadds anauditExcludescase per form, anamed-error case for an unrecognised shape and one for a reversed class range
(
packages/[z-a]).check-dep-ages.test.ts,check-dep-ages-bun.test.ts,check-dep-ages-blockers.test.ts,check-dep-ages-install-age-bun.test.ts:139 pass, 0 fail.
tryaround the class compilation makes thereversed-range case fail (the gate exits 1 instead of 2).
Measured on a8cb4bd (main 1abce03):
bun run lint:ci: exit 0.character class, brace alternatives, literal-negation immunity).
🤖 Generated with Claude Code