Skip to content

fix(sdk): type-narrow manifest.files in pack staging root helper#95465

Merged
vincentkoc merged 1 commit into
openclaw:mainfrom
wangmiao0668000666:fix/sdk-package-e2e-files-typing
Jun 22, 2026
Merged

fix(sdk): type-narrow manifest.files in pack staging root helper#95465
vincentkoc merged 1 commit into
openclaw:mainfrom
wangmiao0668000666:fix/sdk-package-e2e-files-typing

Conversation

@wangmiao0668000666

Copy link
Copy Markdown
Contributor

What Problem This Solves

check-test-types (tsgo with test/tsconfig/tsconfig.core.test.json) fails on upstream/main with:

packages/sdk/src/package.e2e.test.ts(127,5): error TS2322: Type 'string | URL' is not assignable to type 'string'.
  Type 'URL' is not assignable to type 'string'.

The failure reproduces on Node 24.x + @types/node 24.x (where fs.cp dest is string), but masks cleanly on local @types/node 25.x (where dest is string | URL). It is reproducible on any PR whose base is recent upstream/main and 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 createPackStagingRoot type-narrow manifest.files to string[] before the for-of loop, so path.join and fs.cp get the type they expect.

Why This Change Was Made

PackageManifest declares files only via [key: string]: unknown, so Array.isArray(manifest.files) does not narrow to string[]; the for-of iterator yields unknown. The pre-existing typeof entry !== "string" filter at line 174 is the runtime check, but the TypeScript narrowing does not survive into the path.join call below. The fix is to type the local files variable as string[] 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-types CI gate for any PR rebased onto current upstream/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

$ node scripts/run-tsgo.mjs -p test/tsconfig/tsconfig.core.test.json \
    --incremental --tsBuildInfoFile /tmp/test-main-fix.tsbuildinfo
EXIT=0

CI check-test-types (Node 24.x, @types/node 24.x) is the only environment that exposes the error because of the string-only fs.cp dest signature in that version. After this patch, that signature accepts string cleanly.

Verification

  • node scripts/run-tsgo.mjs -p tsconfig.core.json --incremental — exit 0
  • node scripts/run-tsgo.mjs -p test/tsconfig/tsconfig.core.test.json --incremental — exit 0
  • git diff --check upstream/main...HEAD — passed

No behavior change. Pre-existing typeof entry !== "string" filter still skips non-string entries; only the static type narrows.

@clawsweeper

clawsweeper Bot commented Jun 21, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed June 21, 2026, 12:17 AM ET / 04:17 UTC.

Summary
The PR changes the SDK package e2e pack-staging helper to store manifest.files in a string[] local before copying listed package files.

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 check-test-types passed after the change.

Review metrics: none identified.

Merge readiness
Overall: 🦞 diamond lobster
Proof: 🦞 diamond lobster
Patch quality: 🦞 diamond lobster
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Next step before merge

  • [P2] No repair lane is needed; the remaining action is ordinary maintainer review and merge gating for a clean one-line PR.

Security
Cleared: The diff is confined to a test helper type annotation and does not touch runtime code, dependencies, lockfiles, workflows, permissions, scripts, downloaded artifacts, or secrets.

Review details

Best 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 check-test-types passed after the change.

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 changes

Label justifications:

  • P2: The PR fixes a focused typecheck failure that can block unrelated PR validation, with limited blast radius and no runtime behavior change.
  • rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body includes after-fix terminal output for the relevant tsgo commands, and live PR checks show check-test-types passing on the exact head.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes after-fix terminal output for the relevant tsgo commands, and live PR checks show check-test-types passing on the exact head.
Evidence reviewed

PR surface:

Tests 0. Total 0 across 1 file.

View PR surface stats
Area Files Added Removed Net
Source 0 0 0 0
Tests 1 1 1 0
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 1 1 1 0

What I checked:

  • PR diff: The branch changes only packages/sdk/src/package.e2e.test.ts, replacing the inferred files local with const files: string[] = Array.isArray(manifest.files) ? (manifest.files as string[]) : [];. (packages/sdk/src/package.e2e.test.ts:172, 5bc5f4c10c72)
  • Current main behavior: Current origin/main still reads manifest.files from an unknown index signature and iterates it before passing string-guarded entries into path.join and fs.cp. (packages/sdk/src/package.e2e.test.ts:172, 66f84a9bf108)
  • Caller path: The SDK package e2e test calls createPackStagingRoot for gateway-protocol, gateway-client, and sdk package roots before running npm pack. (packages/sdk/src/package.e2e.test.ts:280, 66f84a9bf108)
  • Manifest shape: All three staged package manifests declare files as a string array containing dist, so the patch preserves the tested package behavior. (packages/sdk/package.json:5, 66f84a9bf108)
  • Dependency contract: Direct inspection of @types/[email protected] shows path.join(...paths: string[]): string and fs.promises.cp(source: string | URL, destination: string | URL, ...), so narrowing entries before the path calls is the relevant type boundary. (node/path.d.ts:64)
  • CI proof: Live PR checks show check-test-types, check-prod-types, and check-lint passing on head 5bc5f4c10c72c517b1a894ccaf5b2370ee56e1f7; some unrelated broad matrix jobs were still pending. (5bc5f4c10c72)

Likely related people:

  • vincentkoc: git blame and git show tie the createPackStagingRoot helper and manifest.files loop to fix(test): repair e2e standalone regressions. (role: introduced current helper behavior; confidence: high; commits: 6d76acc2589f; files: packages/sdk/src/package.e2e.test.ts)
  • steipete: A later package-source documentation commit touched the same helper shape without changing the manifest.files behavior. (role: recent adjacent contributor; confidence: medium; commits: 4df95d3c3fd6; files: packages/sdk/src/package.e2e.test.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P2 Normal backlog priority with limited blast radius. labels Jun 21, 2026
@clawsweeper clawsweeper Bot added rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels Jun 21, 2026
@vincentkoc
vincentkoc merged commit 4db8296 into openclaw:main Jun 22, 2026
199 of 209 checks passed
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Jun 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal backlog priority with limited blast radius. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. size: XS status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants