Repository navigation
[Tests] Cover the listing name containment guard - #8941
Draft
github-actions[bot] wants to merge 1 commit into
Draft
github-actions[bot] wants to merge 1 commit into
github-actions[bot] wants to merge 1 commit into
Conversation
Co-Authored-By: Claude Opus 4.8 <[email protected]>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
WHY are these changes introduced?
validateListingNameinpackages/theme/src/cli/utilities/theme-file-path.tsguards the user-supplied--listingflag ontheme dev,theme pushandtheme share, and has no co-located test. It was added by #8834 ("Keep theme files in the selected directory") to stop a listing name from escaping the theme directory, so its rejection branches are the containment contract rather than incidental validation.Of the five rejection branches, only one is exercised today, and only indirectly:
theme-listing.test.ts:205passes'../outside'throughensureListingExists, which happens to hit the..-separator branch. Nothing asserts that an empty name, a bare., a bare.., a Windows\separator, or a drive-letter prefix is rejected, and nothing pins down which names stay accepted. A refactor of that predicate could drop the Windows separator or the drive-letter check and every test in the repository would still pass.This is not a flaky-test fix. Reviewing
tests-main.ymlruns created between 2026-10-12T00:26Z and 2026-10-19T00:26Z UTC (36 runs acrossmainandstable/4.9, all first attempts, no reruns) found four failures, all onwindows-latest, all timeouts, and all reducing to two signatures: the PowerShell record test inapp-security-commands.test.ts(runs 38773610537, 38766274961, 38709303616, 38383005193) and the 32-hex secret test insecret-safety.test.ts(run 38766274961). Both already have open fixes, #8832 and #8928 respectively, so there is no actionable flake left in the window.WHAT is this pull request doing?
Adds
packages/theme/src/cli/utilities/theme-file-path.test.tswith two table-driven cases overvalidateListingName: nine names it must reject, and four it must accept.The accepted names are the ones that make the predicate's boundary explicit —
..periodsand...start with two periods but are ordinary directory names, so the function compares against.and..exactly rather than matching a prefix. Without those cases a stricter rewrite that rejected anything starting with..would look correct.The function is pure and synchronous, so the test needs no temporary directory and no filesystem access. The names are passed directly rather than driven through
ensureListingExists, which keeps each case a statement about the guard instead of about the caller, and avoids the realgit-and-scan setup that makes the existing listing tests slow. No production code is changed.Validation: the new file passes locally on Linux Node 26.1.0 (13 passed), five times under
--sequence.shuffle, and alongside all 46 test files inpackages/theme/src/cli/utilities(621 passed).nx type-check themeand ESLint on the new file both pass. I confirmed the tests earn their keep by temporarily neutralizing the predicate: all nine rejection cases fail and the four acceptance cases still pass, then restored it.Remaining uncertainty: the drive-letter and backslash cases describe Windows-shaped input but the predicate is plain string matching with no platform branch, so they assert the same thing on every OS — I verified them on Linux only, which is sufficient for this function but does not exercise real Windows path handling.
How to manually test your changes?
Both should fail with
Unsafe listing name.Checklist
patchfor bug fixes ·minorfor new features ·majorfor breaking changes) and added a changeset withpnpm changeset add