fix(sdk): type-narrow manifest.files in pack staging root helper#95465
Conversation
|
Codex review: needs maintainer review before merge. Reviewed June 21, 2026, 12:17 AM ET / 04:17 UTC. Summary PR surface: Tests 0. Total 0 across 1 file. Reproducibility: no. high-confidence executed current-main repro was run in this read-only review. Source inspection plus dependency types show the relevant type boundary, and PR-head Review metrics: none identified. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Next step before merge
Security Review detailsBest possible solution: Land the narrow test-helper type fix after normal maintainer review, preserving runtime behavior and relying on CI for the final typecheck gate. Do we have a high-confidence way to reproduce the issue? No high-confidence executed current-main repro was run in this read-only review. Source inspection plus dependency types show the relevant type boundary, and PR-head Is this the best way to solve the issue? Yes. Changing only the SDK test helper local is the narrowest maintainable fix for this typecheck failure, and package behavior remains unchanged. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 66f84a9bf108. Label changesLabel justifications:
Evidence reviewedPR surface: Tests 0. Total 0 across 1 file. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
What Problem This Solves
check-test-types(tsgo withtest/tsconfig/tsconfig.core.test.json) fails onupstream/mainwith:The failure reproduces on Node 24.x + @types/node 24.x (where
fs.cpdest isstring), but masks cleanly on local @types/node 25.x (where dest isstring | URL). It is reproducible on any PR whose base is recentupstream/mainand it is unrelated to the PR's actual changes — the lint gate that blocks unrelated merges.This PR is the minimal one-line fix to make
createPackStagingRoottype-narrowmanifest.filestostring[]before the for-of loop, sopath.joinandfs.cpget the type they expect.Why This Change Was Made
PackageManifestdeclaresfilesonly via[key: string]: unknown, soArray.isArray(manifest.files)does not narrow tostring[]; the for-of iterator yieldsunknown. The pre-existingtypeof entry !== "string"filter at line 174 is the runtime check, but the TypeScript narrowing does not survive into thepath.joincall below. The fix is to type the localfilesvariable asstring[]so the narrowed type is preserved across the loop, with no behavior change.The assertion is sound: the same
typeof entry !== "string"guard still runs on every iteration, so non-string entries are still skipped at runtime.User Impact
No behavior change. Fixes a false-positive TS error that blocks the
check-test-typesCI gate for any PR rebased onto currentupstream/main. After this lands, the test-types gate passes again on main and the unrelated PRs that trip on it (including #95352) stop carrying the noise.Evidence
CI
check-test-types(Node 24.x, @types/node 24.x) is the only environment that exposes the error because of thestring-onlyfs.cpdest signature in that version. After this patch, that signature acceptsstringcleanly.Verification
node scripts/run-tsgo.mjs -p tsconfig.core.json --incremental— exit 0node scripts/run-tsgo.mjs -p test/tsconfig/tsconfig.core.test.json --incremental— exit 0git diff --check upstream/main...HEAD— passedNo behavior change. Pre-existing
typeof entry !== "string"filter still skips non-string entries; only the static type narrows.