fix: complete reports integration across extension push, pull, and search#792
Merged
fix: complete reports integration across extension push, pull, and search#792
Conversation
…arch Reports were added as a top-level primitive in #790, but several extension system paths were not updated to handle them consistently with other extension types (models, vaults, drivers, datastores). Bug fixes: - Report bundles were compiled without bundleOptions (denoConfigPath/ packageJsonDir), causing reports using bare specifiers or import maps to fail during `extension push` - Report entry points were excluded from the bare specifier detection that determines whether package.json resolution is needed - `validateSourceCompleteness` in extension pull skipped the reports directory, so missing report dependencies went unwarned - `extension search --content-type reports` was rejected by the CLI's hardcoded content type validation list Consistency fixes: - Manifest validation error message now includes "report" as a valid extension type - Collective validation error messages and docstrings now mention reports - Content metadata debug log now includes report count Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
There was a problem hiding this comment.
Adversarial Review
Critical / High
None.
Medium
None.
Low
extension_pull.ts:596—validateSourceCompletenessuses first dir as boundary: The function usesdirs[0](models dir) as the import resolution boundary regardless of which dir the entry point came from. If a report imports a sibling file via relative path, the boundary check resolves against the models directory, not the reports directory. This is a pre-existing issue not introduced by this PR, and the function'scatchonDeno.statfailure gracefully handles any resulting false positives by reporting them as warnings rather than errors.
Verdict
PASS — This is a clean bug-fix PR. The four logic changes are correct: (1) bundleOptions now passed to report bundling matching all other extension types, (2) report entry points included in bare specifier detection, (3) validateSourceCompleteness now receives the reports directory via the existing rest parameter, and (4) "reports" added to the search content type validation list. All message/docstring updates are consistent. No new logic paths are introduced that could fail.
There was a problem hiding this comment.
Review: Approve ✅
Clean, focused bug fix PR that completes the reports integration across the extension system. All changes are consistent and correct.
What I verified
bundleExtensionfix (extension_push.ts:581): Report bundles now correctly receivebundleOptions(denoConfigPath/packageJsonDir), matching the pattern used by all other extension types (models, vaults, drivers, datastores).- Bare specifier detection (
extension_push.ts:240):reportEntryPointscorrectly added to theallEntryPointsarray so report files with bare specifiers triggerpackageJsonDirresolution. validateSourceCompleteness(extension_pull.ts:948): Uses rest params (...dirs: string[]), so addingabsoluteReportsDiris type-safe and correct.- Content type validation (
extension_search.ts:116): "reports" added tovalidContentTypesarray and help text. - Manifest error message (
extension_manifest.ts:67): Updated to include "report" in the validation message. - Import boundaries: No libswamp boundary violations — all changes are in CLI commands or domain internals, importing correctly.
- DDD alignment: Domain layer (
ExtractedReport,ExtensionContentMetadata,CollectiveMismatch) already modeled reports correctly. This PR fixes the CLI/application layer wiring to use that domain logic consistently. - Test coverage: Manifest test and integration test assertions updated to match new error messages. The existing test suite for collective validator and content extractor already covers reports.
Suggestions (non-blocking)
None — this is a minimal, well-scoped fix with appropriate test updates.
🤖 Generated with Claude Code
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 #790 added reports as a top-level extension primitive, but several extension system paths were not updated to handle them consistently with other extension types. This PR fixes bugs and consistency gaps across push, pull, and search.
Bug fixes
bundleOptions— Reports using bare specifiers or deno.json import maps would fail to compile duringextension pushbecausebundleOptions(denoConfigPath/packageJsonDir) was not passed tobundleExtension(). All other extension types correctly received these options.package.jsonis needed for bundling, report entry points were not checked. If a report was the only file using bare specifiers,packageJsonDirwould never be set, causing a bundling failure.validateSourceCompletenessskipped reports on pull — After pulling an extension, source completeness validation checked models, vaults, drivers, and datastores but not reports. Missing report dependencies would go unwarned.--content-type reportsrejected by CLI — Theextension searchcommand had a hardcoded content type validation list that didn't include"reports", soswamp extension search --content-type reportswould error.Consistency fixes
--content-typeflag lists reportsTest plan
deno run test src/domain/extensions/extension_manifest_test.ts— passes with updated error messagedeno run test src/domain/extensions/extension_collective_validator_test.ts— passesdeno run test src/domain/extensions/extension_content_extractor_test.ts— passesdeno checkon all modified files — clean🤖 Generated with Claude Code