build: AB7014 from consumer-runtime evidence (option A); gate the generated-JS walk on the compile evidence record (#619 steps b–d) - #634
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
🦋 Changeset detectedLatest commit: 3111475 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
ScriptedAlchemy
left a comment
There was a problem hiding this comment.
Re-audit follow-up: keep the AB7014 improvement; block premature AB6005 scanner removal
Reviewed 5870e90554425c14daf2c9d191fbb4bd5027c8c5 through GitHub source/diff only; no builds were executed for this review.
The option-A dependency policy is correct: bundled does not mean needed by the consumer. The remaining blocker is deleting validation coverage before its replacement exists.
P1 — unobserved dynamic loads still escape compilation evidence
src/build/compile-result.ts still explicitly says expression requests can be left verbatim without a module or external record. This PR removes validate-artifact-modules.ts, the package dist walk, and the residual-import reader without introducing an equivalent compiler rejection/coverage result for that case.
A fixture such as export const load = () => import(process.argv[2]) must not move from a rejected/unverified runtime edge to a successful self-containment verdict simply because the external list is empty. Generated-module happy-path tests do not establish that negative-case coverage.
Before deleting the walks: add the negative fixtures and establish the supported policy with compiler facilities or explicit declarations. Preserve the existing guard where coverage is still missing. This is not a request to revive #602 or grow another JavaScript parser.
P1 — artifact-only validation has no persisted replacement yet
The PR body defers the manifest half of #619 steps 4–5 until after #604, while this patch already removes the validate --artifact JavaScript walk. Main's compileRslibSurfaces() passes only CompileResult.assets to surface finishers; a new process reading a copied artifact does not possess the compilation's external/module report.
Keep artifact-only validation coverage until the required evidence is recorded and read, or explicitly split and document a narrower validator contract. Merely leaving AB6005 in the changeset does not make that check run for a separately loaded artifact. Evidence must be associated with the emitted file inventory/digests and its compiler/policy version; retain missing/outside/unlisted relative-target checks.
Concrete split / merge guidance
- The consumer-runtime AB7014 evidence work can proceed independently, preserving declaration, declared-payload, and install-script semantics with behavioral tests.
- Integrate #604 and the necessary persisted-evidence reader/writer contract.
- Delete each old validator only alongside the replacement for its demonstrated positive and negative cases. Update AGENTS/docs in that same change.
Keep the framework process-dependency record empty while there is no producer; do not add an unused seam or resurrect the deleted checkout launcher.
Required regression matrix: package external via ordinary import and node-commonjs; allowed builtin; valid/missing/outside/unlisted relative output; unobserved dynamic import; declared opaque payload; bundled-only package still AB7014; artifact-only validation after relocation without source. Scanner removal remains pending until this matrix has an explicit disposition.
|
Converting back to draft: the owner audit on #619 (17:11 UTC) reorders PR 3 — the persisted compile evidence record lands first (#638), and scanner deletion is gated on a coverage matrix. This PR will be reworked on top of #638: it keeps the AB7014 option-A re-derivation and deletes only what the matrix covers. |
5870e90 to
6a1b3b8
Compare
…mpile evidence record (#619 steps b–d)
…urces, changeset scope
…ree AB7014 sources everywhere
…ackIgnore/webpackIgnore) import fails AB6005
306edd9 to
2b3f21e
Compare
|
Dispositions for the owner review threads (17:25 review at P1 — unobserved dynamic loads escape compilation evidence. Resolved as the re-review records: the walk is not deleted. Every emitted module is still read; a module the record does not prove (no record, P1 — artifact-only validation has no persisted replacement. Resolved by #638 ( P1 (19:56) — a proven file skips literal-import validation; Split guidance (1–3). Followed in order: #638 is (a); this PR keeps option-A Process-dependency record. Kept empty; no seam added, no launcher revived. Docs now say three Required regression matrix (package external via import and |
…ed, recorded ones pass
…reserved-external wording
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Steps (b) and (d) of #619's reordered PR 3 (owner audit 17:11), rebased onto #638 (step a). The coverage matrix (step c) is posted on #619; this PR deletes exactly what it covers and gates the rest.
AB7014 — option A
prepackjudges an installed dependency used only when a consumer must have it installed:runtimeDependencies(definePrebuilt);.d.tsreferences it — read with TypeScript'spreProcessFile(imports,/// <reference types>,#subpaththrough theimportsmap), not a regex;bin,node_modules/<name>/file, bare--import/--requirepreload) — the same grammar that escalatesAB7015.A package the build inlined is not used; the diagnostic names the
distbundles that inlined it, read fromPackageBuildResult.evidence(#638) rather than a scan.prepackno longer reads packed JavaScript at all:require/createRequire/import.meta.resolveliterals,bin-command strings, inlinenode -eprograms, files traced from install scripts, and the "computed load withholds the check" rule are gone (matrix rows 8–14).pack-dependencies.tsloses ~660 lines.Scanner deletion, gated on the record
rslib.tsassertNoResidualReservedImports— a reserved specifier kept external is apackageExternalIRand fails compile-timeAB6005;guardReservedExternalsandreservedAliasViolationcover the function-form and alias routes (row 7).validate-artifact-modules.ts) stays, keyed onprovenModules. A manifestbundlefile is proven when the artifact'sagent-bundle.compile-evidence.jsonisAB6039-clean andcoverage.rewritableis false; a proven file is lexed, not parsed, and every import it still carries is held to the record: a Node built-in or one of the file's recorded externals passes, a non-literalimport()fails (the form Rslib leaves verbatim), and any other literal request fails too — the compiler neither bundled nor recorded it, which is what anrspackIgnore/webpackIgnoreimport looks like (Rspack leaves it verbatim with no module, external, warning, or dependency object). A matching digest proves the bytes are the compiler's, never that every import was resolved. Every other module — no record, a hatch build,install.mjs, copied scripts — keeps the full acorn parse and import resolution (rows 4–6).ValidateArtifactOptions.bundleSyntaxCheck/bundleSyntaxCheckForare removed; the record decides.import()finding for every module, and the walk over JavaScript the framework did not compile. What still needs a replacement is recorded in the matrix and indocs/entry-conventions.md.The package build applies the same gate to
dist: proven unless atoolshatch ran.Tests
build.test.ts: a compiled script withimport(name)failsbuild()withAB6005 … has a non-literal dynamic import(compiler evidence cannot see it; the walk does);import('left-pad' /* rspackIgnore: true */)and ignored missing/outside/sibling relatives failAB6005 … loads "<request>", which the compiler neither bundled nor recorded as an external; a relocated artifact (source deleted) validates from its record alone and still reports those; hatch build parses in full.package-build.test.ts: the same ignored-import and expression-import fixtures fail thedistwalk and leavedistunpublished;evidence.assets[].packagesattributesnode_modules, linked, and nested-linked dependencies.artifact-validator.test.ts: walk cases restored; record fixtures for no record / covered /rewritable: true/ malformed.prepack.test.ts: inlined-dependencyAB7014names the bundles; declaration, install-script, andruntimeDependenciesacceptance; compile-timeAB6005wins overAB7014when only adistbundle imports a declared dependency.generated-module-evidence.test.ts: the framework's own runtime modules resolve no package (the process-dependency record is empty).Docs / changeset
AGENTS.md("Proof is bytes and processes" → compiler evidence + packed-process tests),docs/diagnostics.md(AB6005,AB7014,AB6039gate note),docs/entry-conventions.md, website en+zh validation, architecture, package-entries, mcp, targets-artifacts. Oneminorchangeset (AB7014evidence changes; internalbundleSyntaxCheckremoval is not release-noted).Deslop
Fable-5.1-high, rebase pass: one
validateCompileEvidence(reads, parses, re-judges, and derivesprovenPaths) instead of two readers emittingAB6039twice;publishedPrefixreused for thedistwalk'sreportedRoot; duplicate imports merged;AB6039message forms aligned with #638.Self-review
Reviewer:
gpt-5.6-sol-medium(change-risk pass over the diff vsorigin/main, 4 findings), all fixed in 9e33a0c:ModuleIR.package(rslib.tsonly recognisednode_modulessegments; Rspack records symlinked workspaces at their real path, soAB7014could not name the bundle that inlined them). Fixed:declaredDependencyRootsnow returns real-root → package name andpackageNameOfResourceconsults it;package-build.test.tsadds a workspace-link fixture assertingevidence.assets[].packagesattributeslinked-dep.provenPaths(the distinguishing test used synthetic evidence). Fixed:build.test.tsbuilds a real artifact, re-writes a compiled bundle with a syntax error and re-signs manifest + record, then asserts the record-proven artifact passes the lexed check and the same artifact withcoverage.rewritable: truefailsAB6005.AB7014evidence sources;pack-inventory.tsconsumes three and the "process-dependency record" is empty. Fixed acrossAGENTS.md,docs/diagnostics.md, website en+zh validation/architecture/targets-artifacts.ValidateArtifactOptions.bundleSyntaxCheckremoval as a public breaking change. Fixed: clause removed.Second pass (
gpt-5.6-sol-medium, 2 findings), fixed in the follow-up commit:packageNameOfResourcetook the first containing root). Fixed: deepest containing root wins;package-build.test.tsaddslinked-nestedinsidelinked-dep's real root and asserts all three packages (fails without the fix).docs/diagnostics.mdAB7014 row,validation.mdx). Fixed; zh already said three.Owner re-review (19:56, at
6a1b3b8): P1 — a proven file skipped literal-import validation, so anrspackIgnore/webpackIgnoreimport escaped. Fixed in2b3f21e(see the dispositions comment and the negative-test matrix on #619).Third pass (
gpt-5.6-sol-medium, 1 finding), fixed in2014721:artifact-validator.test.tsstill expected a proven bundle's bare specifier and dangling sibling to pass — the synthetic test encoded the flaw the owner found. Fixed: those are now expected as unaccountedAB6005; a recordedartifact-relativesibling and a built-in in a proven bundle are shown to pass, and the same sibling without its record entry is shown to fail. Full integration pool: 1127 passed.Fourth pass (
claude-fable-5-1-thinking-high, 3 findings),8cd6d02:docs/diagnostics.mdsaid the pre-manifest pass validates only filesystem entries and JSON. Fixed: it also reads the record against the planned file table (AB6039) and walks the JavaScript modules.docs/entry-conventions.mdoverstated the function-form reserved-external path (guardReservedExternalsintercepts it first; compile-timeAB6005is the backstop for what reaches the module graph). Fixed in both.createRequire(…)('pkg')/import.meta.resolvehas no declaration path, soAB7014fires for it. Dismissed: this is option A as the owner settled it (bundled ≠ used; the closed-world contract says a generated executable bundles everything but Node built-ins, so a resolution-only load of an installed package is outside the contract), the form is listed incoverage.unobserved, matrix row 9 records it, and the owner asked for no unused seam. AruntimeDependenciesfield on entry configs can be added when a fixture needs it.Final pass (
gpt-5.6-sol-medium): the three doc edits verified against the code; one low finding — two test comments inbuild.test.tsstill described the removed residual-import scan — fixed in3111475. No merge risks remaining.