fix(ci): catch script declaration drift before merge#110248
Conversation
openclaw-barnacle
Bot
added
scripts
clawsweeper
Bot
added
proof: sufficient
|
Codex review: needs maintainer review before merge. Reviewed July 17, 2026, 9:49 PM ET / July 18, 2026, 01:49 UTC. Summary PR surface: Tests +565, Config +2, Other +585. Total +1152 across 25 files. Reproducibility: not applicable. This PR prevents a CI contract-drift class rather than reporting a current user-facing runtime failure; the PR supplies direct before/after verifier evidence. Review metrics: 1 noteworthy metric.
Stored data model Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Maintainer decision needed
Security Review detailsBest possible solution: Adopt one authoritative script-declaration guard after maintainer confirmation that its fail-closed handling is limited to genuinely unverifiable contracts and valid ESM barrel patterns remain accepted. Do we have a high-confidence way to reproduce the issue? Not applicable: this PR prevents a CI contract-drift class rather than reporting a current user-facing runtime failure; the PR supplies direct before/after verifier evidence. Is this the best way to solve the issue? Yes, with maintainer confirmation of the fail-closed boundary: a single verifier wired into existing changed-check and CI guard seams avoids repeating one-off declaration repairs. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 75e4103c5738. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Tests +565, Config +2, Other +585. Total +1152 across 25 files. 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
Review history (1 earlier review cycle)
|
6b31219 to
6277133
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6277133c21
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| function createOpaqueExternalOrigin(importerPath, specifier, kind, name = null) { | ||
| return JSON.stringify(["opaque-external", importerPath, specifier, kind, name]); |
There was a problem hiding this comment.
When two script modules both re-export the same named binding from the same external package and a barrel uses export * from both, ESM still exposes that name (for example, both wrappers re-exporting foo from pkg). Including importerPath in the opaque origin makes those wrappers look like different bindings, so origins.size > 1 reports opaque external star collision and check:script-declarations blocks a valid script/declaration pair. The origin should key named external bindings by the external specifier/kind/name, not the wrapper path.
Useful? React with 👍 / 👎.
6277133 to
42e59df
Compare
clawsweeper
Bot
added
rating: 🐚 platinum hermit
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42e59df60b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else if (!target) { | ||
| if (!isDeclaration) { | ||
| explicitExports.set( | ||
| exportedName, | ||
| new Set([createBindingOrigin(specifier.text, sourceName)]), | ||
| ); |
There was a problem hiding this comment.
When a script has a local named re-export such as export { foo } from "./renamed.mjs" after the target was deleted or moved, resolveReexport returns null but this branch still records foo as a valid runtime export. If the adjacent .d.mts lists the same name, check:script-declarations passes even though the script cannot be linked, so the new guard misses local re-export drift that the unresolved star/declaration paths already fail closed on.
Useful? React with 👍 / 👎.
|
Merged via squash.
|
* fix(ci): verify script declaration contracts * fix(ci): harden declaration export analysis * fix(ci): cover opaque script module exports * test(ci): cover cyclic script declaration barrels
1 participant
What Problem This Solves
Resolves a problem where an exported value could be added to an MJS maintainer script without updating its adjacent declaration sidecar, allowing a broken script type contract to reach
main. This class caused the recent declaration repairs in #109358, #109373, and #109812.Why This Change Was Made
Adds one deterministic value-export contract verifier for paired
scripts/**/*.mjsand.d.mtsfiles, including ESM star-export semantics, TypeScript value/type namespace distinctions, missing sidecars, orphaned runtime sources, and fail-closed handling when external declaration value provenance is unknowable. The same fast command now runs from changed-file checks, the aggregate check preflight, and the blocking CI guards consumed byscripts/pr prepare-run. Existing stale declarations were normalized so the new baseline starts green.AI-assisted. The implementation was reviewed with the repository
autoreviewworkflow until no actionable findings remained.User Impact
No product behavior changes. Maintainers now get a pre-merge failure when script runtime exports and declaration contracts diverge, instead of discovering the break after merge.
Evidence
node scripts/check-script-declarations.mjs— 233 declaration contracts match.node scripts/run-vitest.mjs test/scripts/check-script-declarations.test.ts— 21 passed after rebase onto currentmain.node scripts/check-changed.mjs -- <changed files>— formatting, declaration verification, script/test typechecks, and script lint passed on Blacksmith Testbox: https://github.com/openclaw/openclaw/actions/runs/29625334881