Added internal package golden path checks - #29877
Open
ErisDS wants to merge 1 commit into
Open
Conversation
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run ghost:test:ci:integration |
✅ Succeeded | 3m 1s | View ↗ |
nx run ghost:test:integration |
✅ Succeeded | 3m 5s | View ↗ |
nx run @tryghost/admin:test:acceptance |
✅ Succeeded | 6m 13s | View ↗ |
nx run ghost:test:e2e |
✅ Succeeded | 2m 39s | View ↗ |
nx run ghost:test:legacy |
✅ Succeeded | 2m 49s | View ↗ |
nx run @tryghost/koenig-lexical:test:acceptance |
✅ Succeeded | 2m 5s | View ↗ |
nx run ghost-monorepo:lint:boundaries |
✅ Succeeded | 22s | View ↗ |
nx run-many -t test:unit -p @tryghost/adapter-b... |
✅ Succeeded | 35s | View ↗ |
Additional runs (8) |
✅ Succeeded | ... | View ↗ |
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗
☁️ Nx Cloud last updated this comment at 2026-08-11 11:26:53 UTC
Contributor
WalkthroughThe change defines Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.agents/skills/migrate-internal-package/SKILL.md (1)
66-66: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winName the required manifest field explicitly.
This step says “with a concise reason” but does not identify
ghostPackage.reason. The validator readspkg.manifest.ghostPackage.reason, so state the exact property to prevent migration work from recording the reason under the wrong field.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.agents/skills/migrate-internal-package/SKILL.md at line 66, Update the migration instruction to explicitly set both ghostPackage.goldenPath to migration and ghostPackage.reason to the concise reason, matching the manifest property read by the validator.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/check-internal-packages.js`:
- Line 248: Update the workspaceNames construction in the internal package
validation flow to include manifest names from every workspace member, including
packages under koenig/, rather than only packages/. Ensure dependencies on
Koenig workspace packages are recognized by the existing workspace:* validation,
and add a fixture covering a package below koenig/.
- Around line 183-203: Validate that exports condition values source, types, and
default are strings before applying the source regex, deriving build paths, or
calling replace in the root export checks. Record malformed values through the
existing aggregated error mechanism and skip dependent validation for invalid
source values, preventing malformed manifests from terminating the checker. Add
fixtures covering non-string values for each condition.
- Around line 281-289: Extend the validation flow around
discoverPackageDirectories() so packages/_template is validated with the same
compliant-package contract as discovered packages, rather than checking only
ghostPackage.goldenPath. Reuse the existing compliant-package validator and
preserve its checks for exports, scripts, dependencies, and configuration; add a
fixture that omits or invalidates one required template field and assert
validation fails.
---
Nitpick comments:
In @.agents/skills/migrate-internal-package/SKILL.md:
- Line 66: Update the migration instruction to explicitly set both
ghostPackage.goldenPath to migration and ghostPackage.reason to the concise
reason, matching the manifest property read by the validator.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b3250b3b-8a3a-4d50-bdb5-55d0060d3a0a
📒 Files selected for processing (17)
.agents/skills/convert-internal-package-to-typescript/SKILL.md.agents/skills/migrate-internal-package/SKILL.md.github/workflows/ci.ymlpackage.jsonpackages/README.mdpackages/_template/package.jsonpackages/adapters/redirects-base/package.jsonpackages/adapters/route-settings-base/package.jsonpackages/admin-api-schema/package.jsonpackages/custom-field-types/package.jsonpackages/i18n/package.jsonpackages/nql-string/package.jsonpackages/parse-email-address/package.jsonpackages/testing/test-data/package.jsonscripts/check-internal-packages.jsscripts/create-package.jsscripts/test/check-internal-packages.test.js
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #29877 +/- ##
=======================================
Coverage 75.42% 75.43%
=======================================
Files 1606 1606
Lines 140676 140676
Branches 17426 17435 +9
=======================================
+ Hits 106108 106117 +9
+ Misses 33520 33511 -9
Partials 1048 1048
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2 tasks
no ref Internal package conventions were documented but could drift silently or leave migrations in a permanent halfway state. This adds explicit lifecycle metadata and focused, dependency-free validation so new packages and migrated packages have an actionable path to compliance.
ErisDS
force-pushed
the
codex/add-internal-package-checks
branch
from
August 11, 2026 11:15
64e1be1 to
0ef2652
Compare
ErisDS
commented
Aug 11, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/check-internal-packages.js (1)
62-85: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winKeep workspace discovery synchronized with
pnpm-workspace.yamlThe hardcoded roots match the current workspace globs. A future workspace glob can bypass the
workspace:*check becauseworkspaceNamesonly includes these hardcoded roots. Derive the roots frompnpm-workspace.yaml, or add a synchronization test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/check-internal-packages.js` around lines 62 - 85, The discoverWorkspacePackageDirectories function hardcodes workspace roots that can diverge from pnpm-workspace.yaml, causing workspace:* packages to be missed. Update this discovery flow to derive workspace roots from the workspace configuration, or add a test that fails whenever the hardcoded roots and pnpm-workspace.yaml globs become inconsistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/check-internal-packages.js`:
- Around line 327-343: In the template manifest handling, narrow the try/catch
around templateManifestPath to only readFile and JSON.parse, then move the
validateCompliantPackage call outside the try block while preserving its
existing arguments and error aggregation. Keep read/parse failures mapped to the
current invalid-JSON message without intercepting validation exceptions.
---
Nitpick comments:
In `@scripts/check-internal-packages.js`:
- Around line 62-85: The discoverWorkspacePackageDirectories function hardcodes
workspace roots that can diverge from pnpm-workspace.yaml, causing workspace:*
packages to be missed. Update this discovery flow to derive workspace roots from
the workspace configuration, or add a test that fails whenever the hardcoded
roots and pnpm-workspace.yaml globs become inconsistent.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0374f917-5a85-4ef6-980c-d4b29e8d409b
📒 Files selected for processing (4)
.agents/skills/migrate-internal-package/SKILL.md.agents/skills/migrate-internal-package/references/legacy-integration.mdscripts/check-internal-packages.jsscripts/test/check-internal-packages.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- .agents/skills/migrate-internal-package/SKILL.md
Comment on lines
+327
to
+343
| const templateManifestPath = path.join(packagesDirectory, '_template', 'package.json'); | ||
| try { | ||
| const templateManifestSource = await readFile(templateManifestPath, 'utf8'); | ||
| const templateManifest = JSON.parse(templateManifestSource | ||
| .replaceAll('{{NAME}}', 'template') | ||
| .replaceAll('{{DESCRIPTION}}', 'Template package') | ||
| .replaceAll('{{DIRECTORY}}', 'packages/template')); | ||
| errors.push(...await validateCompliantPackage({ | ||
| rootDirectory, | ||
| packageDirectory: path.dirname(templateManifestPath), | ||
| packagePath: 'packages/template', | ||
| manifest: templateManifest, | ||
| workspaceNames | ||
| })); | ||
| } catch (error) { | ||
| errors.push(`packages/_template/package.json: invalid JSON (${error.message})`); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Narrow the template try block to the read and parse steps.
The try block wraps validateCompliantPackage. Two consequences follow:
- A missing or unreadable
packages/_template/package.jsonreportsinvalid JSON (ENOENT: ...), which is inaccurate. - If
validateCompliantPackagethrows, its aggregated errors are lost and replaced with a single misleading message.
Move the validation call outside the try block.
🛠️ Proposed fix
const templateManifestPath = path.join(packagesDirectory, '_template', 'package.json');
+ let templateManifest;
try {
const templateManifestSource = await readFile(templateManifestPath, 'utf8');
- const templateManifest = JSON.parse(templateManifestSource
+ templateManifest = JSON.parse(templateManifestSource
.replaceAll('{{NAME}}', 'template')
.replaceAll('{{DESCRIPTION}}', 'Template package')
.replaceAll('{{DIRECTORY}}', 'packages/template'));
+ } catch (error) {
+ errors.push(`packages/_template/package.json: unreadable or invalid JSON (${error.message})`);
+ }
+
+ if (templateManifest) {
errors.push(...await validateCompliantPackage({
rootDirectory,
packageDirectory: path.dirname(templateManifestPath),
packagePath: 'packages/template',
manifest: templateManifest,
workspaceNames
}));
- } catch (error) {
- errors.push(`packages/_template/package.json: invalid JSON (${error.message})`);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const templateManifestPath = path.join(packagesDirectory, '_template', 'package.json'); | |
| try { | |
| const templateManifestSource = await readFile(templateManifestPath, 'utf8'); | |
| const templateManifest = JSON.parse(templateManifestSource | |
| .replaceAll('{{NAME}}', 'template') | |
| .replaceAll('{{DESCRIPTION}}', 'Template package') | |
| .replaceAll('{{DIRECTORY}}', 'packages/template')); | |
| errors.push(...await validateCompliantPackage({ | |
| rootDirectory, | |
| packageDirectory: path.dirname(templateManifestPath), | |
| packagePath: 'packages/template', | |
| manifest: templateManifest, | |
| workspaceNames | |
| })); | |
| } catch (error) { | |
| errors.push(`packages/_template/package.json: invalid JSON (${error.message})`); | |
| } | |
| const templateManifestPath = path.join(packagesDirectory, '_template', 'package.json'); | |
| let templateManifest; | |
| try { | |
| const templateManifestSource = await readFile(templateManifestPath, 'utf8'); | |
| templateManifest = JSON.parse(templateManifestSource | |
| .replaceAll('{{NAME}}', 'template') | |
| .replaceAll('{{DESCRIPTION}}', 'Template package') | |
| .replaceAll('{{DIRECTORY}}', 'packages/template')); | |
| } catch (error) { | |
| errors.push(`packages/_template/package.json: unreadable or invalid JSON (${error.message})`); | |
| } | |
| if (templateManifest) { | |
| errors.push(...await validateCompliantPackage({ | |
| rootDirectory, | |
| packageDirectory: path.dirname(templateManifestPath), | |
| packagePath: 'packages/template', | |
| manifest: templateManifest, | |
| workspaceNames | |
| })); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/check-internal-packages.js` around lines 327 - 343, In the template
manifest handling, narrow the try/catch around templateManifestPath to only
readFile and JSON.parse, then move the validateCompliantPackage call outside the
try block while preserving its existing arguments and error aggregation. Keep
read/parse failures mapped to the current invalid-JSON message without
intercepting validation exceptions.
EvanHahn
removed their request for review
August 11, 2026 16:33
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
compliant,migration, andexemptlifecycle states, with reasons required for temporary or intentional exceptions.Testing
pnpm lint:packagespnpm --filter @internal/scripts lintpnpm --filter @internal/scripts testpnpm lint:docspnpm check— affected lint and unit-test lanes passed; the full run was blocked by unrelated local browser-suite startup failures and an existing Admin archive test timeout