chore: remove pnpm-lock.yaml to verify unlocked deps#5859
Conversation
Remove the lockfile so CI resolves latest dependency versions. This verifies the project works with unlocked dependencies. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Note Gemini is unable to generate a review for this pull request due to the file types involved not being currently supported. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCI workflows loosen pnpm lockfile enforcement and remove pnpm cache flags; a devDependency ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Deploying egg with
|
| Latest commit: |
c4ae9ea
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a0f850b1.egg-cci.pages.dev |
| Branch Preview URL: | https://chore-unlock-deps.egg-cci.pages.dev |
There was a problem hiding this comment.
Pull request overview
Removes the pnpm lockfile and adjusts CI installs to allow resolving the latest dependency versions, to establish a baseline for isolating dependency-upgrade issues.
Changes:
- Switch GitHub Actions installs from
pnpm install --frozen-lockfiletopnpm install --no-frozen-lockfilein CI. - Switch GitHub Actions installs from
pnpm install --frozen-lockfiletopnpm install --no-frozen-lockfilein E2E CI.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| .github/workflows/e2e-test.yml | Updates pnpm install command to not require a lockfile. |
| .github/workflows/ci.yml | Updates pnpm install command across CI jobs to not require a lockfile. |
Deploying egg-v3 with
|
| Latest commit: |
c4ae9ea
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://6e09a3ac.egg-v3.pages.dev |
| Branch Preview URL: | https://chore-unlock-deps.egg-v3.pages.dev |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1. tegg/plugin/tegg/src/types.ts: export interface Application in declare module block (oxlint new version flags unused interfaces) 2. tegg/core/mcp-client/src/HeaderUtil.ts: use globalThis.Headers instead of urllib's re-export to avoid HeadersInit type mismatch with newer undici types 3. package.json: add unplugin-unused to root devDependencies (tsdown peer dependency that was only resolved via lockfile) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #5859 +/- ##
==========================================
+ Coverage 85.28% 85.88% +0.59%
==========================================
Files 666 666
Lines 13247 18841 +5594
Branches 1538 3636 +2098
==========================================
+ Hits 11298 16181 +4883
- Misses 1818 2297 +479
- Partials 131 363 +232 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Remove pnpm dedupe --check step (no lockfile to check against) - Update ajv-decorator and tegg snapshots for @sinclair/typebox new exports (EvaluateUnionFast, Match, TakeLeft, IsTemplateLiteralFinite, IsTemplateLiteralPattern, TemplateLiteralDecodeUnsafe, _Function_, _Object_) - Add pnpm-lock.yaml to gitignore Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 9 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
tegg/core/mcp-client/src/HeaderUtil.ts:12
mergeHeaders()buildsresasRecord<string, string | null>and then asserts it toRecord<string, string>on return. It would be better to avoid the unsafe cast by only assigning non-null header values (or by initializingresasRecord<string, string>and narrowing theget()result), so the emitted types match runtime expectations forHeadersInit.
// Use global Headers (Node.js 22+) to avoid type mismatch with undici's HeadersInit
const headers = new globalThis.Headers(headersInit);
for (const key of headers.keys()) {
res[key] = headers.get(key);
}
}
return res as Record<string, string>;
}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gitignore:
- Line 120: The .gitignore has a malformed combined entry
"ecosystem-ci/examplespnpm-lock.yaml" so split it into two separate ignore lines
("ecosystem-ci/examples" and "pnpm-lock.yaml") and remove any negation that
re-enables the lockfile (e.g., a "!pnpm-lock.yaml" entry) so the pnpm-lock.yaml
is actually ignored; update the .gitignore by adding the two distinct lines and
deleting the un-ignore entry.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 09df5447-8be5-4340-a2d5-edc58c35057c
⛔ Files ignored due to path filters (2)
tegg/core/ajv-decorator/test/__snapshots__/index.test.ts.snapis excluded by!**/*.snaptegg/core/tegg/test/__snapshots__/ajv.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
.github/workflows/ci.yml.gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci.yml
CI resolves a newer @sinclair/typebox that exports these additional functions in ExtendsResult. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The snapshot tests captured ALL @sinclair/typebox exports, which break on every minor version bump. Replace with targeted assertions that only check egg's own exports (TransformEnum, AjvInvalidParamError, Ajv) plus a basic typebox re-export check (Type). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| - name: Run lint | ||
| run: pnpm run lint | ||
|
|
||
| - name: Check dedupe | ||
| run: pnpm dedupe --check | ||
|
|
||
| - name: Run typecheck | ||
| run: pnpm run typecheck |
There was a problem hiding this comment.
The PR description lists only lockfile removal and switching CI installs to --no-frozen-lockfile, but this workflow change also removes the pnpm dedupe --check step. If this is intentional, please update the PR description; otherwise consider re-adding the dedupe check to keep the existing CI guardrail.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tegg/core/ajv-decorator/test/index.test.ts`:
- Around line 8-10: The test is asserting a runtime export for the
TypeScript-only interface Ajv (exports.Ajv) which is erased at compile time;
remove the runtime assertion for Ajv in index.test.ts and, if you want
compile-time validation that Ajv is exported, replace it with a type-only check
by importing Ajv with "import type { Ajv }" and assigning a dummy typed variable
(e.g., const _typeCheck: Ajv = {} as any) to ensure the type is exported without
runtime assertions.
In `@tegg/core/tegg/test/ajv.test.ts`:
- Around line 8-10: The test is asserting a TypeScript interface at runtime
(exports.Ajv) which is erased and causes CI failures; remove the runtime
assertion for exports.Ajv from tegg/core/tegg/test/ajv.test.ts and, if you need
compile-time verification that the Ajv type is exported, add a type-only check
using an import type (e.g., import type { Ajv } from '...') and a dummy
assignment like const _typeCheck: Ajv = {} as any in the test file so the type
system verifies the re-export without runtime assertions.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c927abc9-9a75-4e63-b379-55298d55c9a8
⛔ Files ignored due to path filters (2)
tegg/core/ajv-decorator/test/__snapshots__/index.test.ts.snapis excluded by!**/*.snaptegg/core/tegg/test/__snapshots__/ajv.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
tegg/core/ajv-decorator/test/index.test.tstegg/core/tegg/test/ajv.test.ts
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ule id format The decorator stack-trace skip logic only matched filesystem paths (/@oxc-project/runtime/) but newer vite/rolldown use virtual module ids like \x00@oxc-project+runtime@0.122.0/helpers/decorate.js. Match the package@version segment too. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Vitest 4 forbids describe() inside it(). Lift the nested describe blocks to the top level: - Move appService initialization to outer beforeAll - Use beforeEach in raw query describe (outer afterEach truncates) - Skip multi db tests (require apple/banana tables not in prepare.js) These nested tests were previously unreachable. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The oxc parser now reports parse errors as: [PARSE_ERROR] Error: Expected \`;\` but found \`Identifier\` instead of the older 'failed: Expected ...' format. Drop the literal 'failed: ' prefix from the regex so both formats match. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
The resolve aliases pointed to non-existent root index.ts files (e.g. ../core-decorator/index.ts) instead of the actual entry points at src/index.ts. Under pnpm with proper symlinks the aliases were never used (pnpm symlinks took priority), but they fail when the nested node_modules tree changes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…gnore Address Copilot/coderabbit review comments: - release.yml: switch to --no-frozen-lockfile and drop cache: pnpm - .gitignore: split malformed 'ecosystem-ci/examplespnpm-lock.yaml' into two entries and remove the !pnpm-lock.yaml un-ignore so the lockfile is actually ignored Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 16 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
tegg/core/mcp-client/src/HeaderUtil.ts:12
mergeHeadersbuilds aRecord<string, string | null>(becauseHeaders.get()returnsstring | null), but then returns it asHeadersInitviaas Record<string, string>. This hides a potentialnullvalue and can produce a return object that doesn't conform toHeadersInit. Consider usingheaders.forEach((value, key) => { ... })(value is always astring) or skipping keys whereget()returnsnull, and then return the object without an unsafe cast.
// Use global Headers (Node.js 22+) to avoid type mismatch with undici's HeadersInit
const headers = new globalThis.Headers(headersInit);
for (const key of headers.keys()) {
res[key] = headers.get(key);
}
}
return res as Record<string, string>;
}
| // TODO: apple/banana databases need tables created in prepare.js | ||
| // These tests were previously unreachable (nested inside an it() block) | ||
| describe.skip('multi db', () => { | ||
| it('should work for multi database', async () => { |
There was a problem hiding this comment.
describe.skip('multi db', ...) permanently disables the multi-database behavior checks. Since these tests are now reachable (no longer nested under an it()), it would be better to either (a) fix the fixture setup so they can run, or (b) gate them on an explicit env/flag (or use describe.todo) so CI still has an actionable signal rather than silently skipping coverage.
| assert.equal(appleClient.options.database, 'apple'); | ||
| assert.equal(bananaClient.options.database, 'banana'); | ||
| assert.equal(bananaClient.options.database, 'banana'); |
There was a problem hiding this comment.
In the "should work for multi database" test, the assertions are duplicated (same property checked twice for both appleClient and bananaClient). This reduces the test's effectiveness and looks like a copy/paste mistake; consider removing the duplicates or asserting a second distinct property/behavior.
| assert.equal(appleClient.options.database, 'apple'); | |
| assert.equal(bananaClient.options.database, 'banana'); | |
| assert.equal(bananaClient.options.database, 'banana'); | |
| assert.equal(bananaClient.options.database, 'banana'); | |
| assert.notEqual(appleClient, bananaClient); |
| declare module 'egg' { | ||
| export interface EggModule {} | ||
|
|
||
| interface Application { | ||
| export interface Application { | ||
| eggPrototypeCreatorFactory: typeof EggPrototypeCreatorFactory; | ||
| eggPrototypeFactory: EggPrototypeFactory; |
There was a problem hiding this comment.
This module augmentation exports Application, but other tegg plugins augment egg using non-exported interfaces (e.g. tegg/plugin/controller/src/types.ts:7-12 uses interface Application { ... }). Mixing exported vs non-exported augmentation interfaces can be confusing and may not be necessary. Consider aligning with the existing pattern (remove export and address the linter warning with a targeted suppression), or apply the same approach consistently across augmentation interfaces in this file (including Context).
|
|
||
| - name: Install dependencies | ||
| run: pnpm install --frozen-lockfile | ||
| run: pnpm install --no-frozen-lockfile |
There was a problem hiding this comment.
Using pnpm install --no-frozen-lockfile in the release workflow makes the release build non-reproducible (dependency graph can change between runs without any git changes), which increases the risk of publishing broken or unverifiable artifacts. Consider keeping releases deterministic (e.g., commit a lockfile and use --frozen-lockfile at least for release.yml), while running the unlocked-deps experiment in a separate CI workflow.
| run: pnpm install --no-frozen-lockfile | |
| run: pnpm install --frozen-lockfile |
Take next branch's versions of files that already received fixes in the chore-unlock-deps PR (#5859): - StackUtil.ts (oxc virtual module id format) - HeaderUtil.ts (globalThis.Headers) - Loader.test.ts (oxc parser error format) - ORM tests (Vitest 4 nesting) - ajv export tests (targeted assertions) - vitest config (alias src/index.ts paths) - types.ts (export Application interface) Keep HEAD versions of CI workflow files (ci.yml, e2e-test.yml) which contain the ut migration changes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
Remove
pnpm-lock.yamland use--no-frozen-lockfileso CI resolves the latest dependency versions on every run. Then fix all the breakages this surfaces.This is intentionally a baseline test for unlocked dependencies — it isolates dependency-upgrade issues from any CI tooling migration.
CI configuration
pnpm-lock.yamlpnpm install --frozen-lockfilewithpnpm install --no-frozen-lockfileinci.ymlande2e-test.ymlcache: 'pnpm'fromsetup-node(requires a lockfile)pnpm dedupe --checkstep (no lockfile to check against)pnpm-lock.yamlto.gitignorePre-existing bugs (latent, exposed by upgrades)
These were always wrong but happened to work under specific resolved versions. They are real bugs and should be fixed regardless of locking strategy.
tegg/core/vitest/vitest.config.ts— Resolve aliases pointed to non-existent root files (../core-decorator/index.ts,../common-util/index.ts,../types/index.ts). The actual entry points live atsrc/index.ts. Under pnpm symlinks the aliases were never reached, but they fail as soon as the resolver hits them. Fixed all four to point atsrc/....tegg/plugin/orm/test/index.test.ts— Twodescribe()blocks were nested inside anit(), so they were silently never executed as test suites. Vitest 4 (already in catalog) actually enforces this and throws. Restructured:appServiceinitialization to outerbeforeAllraw queryandmulti dbdescribes to top levelbeforeEachinraw query(the outerafterEachtruncates tables between tests)multi db(the apple/banana databases are empty inprepare.js— separate fix needed)tegg/plugin/tegg/src/types.ts—interface Applicationinsidedeclare module 'egg'was missingexport. Newer oxlint versions correctly flag it as unused. Addedexport.Adaptations to upgraded dependencies
These are not bugs in the original code; they adapt to newer versions of upstream packages.
tegg/core/common-util/src/StackUtil.ts— The decorator file-path detection skipped frames whose path contained/@oxc-project/runtime/. Newer vite/rolldown produce virtual module ids in the form\x00@oxc-project+runtime@0.122.0/helpers/decorate.js. Added a third match (@oxc-project+runtime@) so the helper frame is skipped in both formats.tegg/core/mcp-client/src/HeaderUtil.ts—new (urllib.Headers)(headersInit)failed to type-check because newerundicitypes are no longer assignable from the globalHeadersInit. UseglobalThis.Headers(Node.js 22+ has it natively) and drop theurllibimport.tegg/core/loader/test/Loader.test.ts— Newer@oxc-parserreports parse errors as[PARSE_ERROR] Error: Expected \;` but found `Identifier`instead of the olderfailed: Expected …format. Loosened the regex by dropping thefailed: ` literal.tegg/core/ajv-decorator/test/index.test.tsandtegg/core/tegg/test/ajv.test.ts— These tests usedtoMatchSnapshot()to capture all exports of@sinclair/typebox, which churns on every minor version. Replaced with targeted assertions that only verify egg's own exports (TransformEnum,AjvInvalidParamError) plus a basic typebox re-export check (Type). Deleted the corresponding.snapfiles.package.json— Addedunplugin-unusedto rootdevDependencies. It is atsdownpeerDependencythat was only resolved transitively via the lockfile.Test plan