refactor: simplify prebuilt-payload internals (post-#71 follow-up) - #86
Merged
Conversation
Deferred /simplify pass on PR #71's merged diff, applying the reviewer findings that survived aggregation: - Skip the AB4750 payload-freshness mtime walk unless the operator ran `validate` — every dev/inspect/build preparation was paying a blocking full-project stat sweep whose diagnostics were then discarded. - Extract payloadDeclarationEntry/payloadDeclarationSource in discover.ts and consume them from configuredPayloadRoots, discoverPayloads, normalizePayloads, declaredPayloads, and validatePayload, replacing five open-coded copies with three divergent malformed-input policies. - Extract owningPayload and share the innermost-payload ownership rule between prebuiltArtifactPath and validatePrebuiltReference, which had drifted to inside-or-equal semantics in validate.ts. - Make PreparedProject.snapshotSource required so the artifact-service fallback that silently dropped payload roots (recreating the drift bug the closure exists to prevent) is unrepresentable. - Probe prebuilt references against one Set of artifact paths, share the AB4748 emitter, reuse fast-glob stats over per-file stat calls, trust the declared hook-args type instead of silently filtering, and derive MCP-app prebuilt-ness from normalized provenance instead of raw config. - Migrate prebuilt-payload.test.ts onto the shared project fixture helpers used by the sibling suites.
🦋 Changeset detectedLatest commit: b3d2ddf 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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
commit: |
This was referenced Sep 1, 2026
Closed
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
1 participant
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.
Summary
PR #71 merged before its deferred /simplify pass ran (the in-branch cleanup commit did land in the squash; this covers what three post-merge reviewers found beyond it). Scope:
git show 28830bd5. All changes are behavior-preserving for valid configurations; the pinned manifest/path/diagnostic tests are untouched except where noted.Findings → fixes
ProjectServicepreparation — watch rebuilds and workbench request bursts — while its info/warning diagnostics are discarded for every command exceptvalidatevalidateSourcetakes{ payloadFreshness };ProjectServicepassescommand === 'validate'. Defaulttruepreserves the public API contract{source}→ nonempty → resolve → containment) was open-coded five times acrossdiscover/normalize/validatewith three different malformed-input tolerancespayloadDeclarationEntry/payloadDeclarationSourcepair indiscover.ts, consumed everywhere;validatePayloadkeeps its own diagnostics for the malformed shapesprebuiltArtifactPathand a filter/sort invalidatePrebuiltReference— and the validate copy used the file's local inside-or-equal containment, so the two could disagreeowningPayloadexport innormalize.tsPreparedProject.snapshotSourcewas optional, and the artifact-service fallback re-snapshotted without payload roots — silently recreating the always-drifted failure the closure was introduced to fixSet; one parameterized AB4748 emitter;discoverPayloadsreuses fast-globstatsinstead of an extra per-filestat;normalizeHooktrusts the declaredreadonly string[]args type instead of silently filtering (validation already errors via AB4746);normalizeMcpAppsderives prebuilt-ness from normalized provenance instead of re-inspecting raw configprebuilt-payload.test.tsre-implemented the shared project fixturecreateProjectFixture/removeProjectFixtureEdge-case notes (deliberate)
'.') now consistently gets AB4742 instead of passing validation and then silently vanishing during normalization; likewise a prebuilt entry pointing at a payload directory is now consistently AB4744 in both validate and normalize. No test pins the old accidental behavior.normalizePayloadsnow applies the same containment filter as discovery/validation, so a root-escaping declaration can no longer reach the normalized model through a directnormalizeProjectcall.Skipped (owner context needed)
payloadSourcePathscontainment-checks requested-root-resolved paths against the realpath'd root; output roots solve this with component translation). Real bug candidate, needs a decision on supported semantics.args[0]reverse-parse for prebuilt MCP identity with aprebuiltPathfield (mirrors hooks) — perturbs canonical model digests.dist/can flag each other as stale) — semantics change.isSafePayloadNamevscore/paths.isSafePathSegment— the trailing-character restriction may or may not be deliberate.Test plan
pnpm buildpnpm typecheck(clean; pre-existing self-referencing TS2307s resolve after build)pnpm lintpnpm test:unit(full suite green)