Repository navigation
Fix apps/api test hang: bump @adonisjs/ace to 14.1.1 (Node 24.20 loader regression) - #199
Conversation
Root-caused the deterministic "Invalid command exported... Invalid URL" failure from the previous fix by bisecting Node versions locally (same OS, only the Node binary changed): clean on 24.18.1, reproduces 100% on 24.20.0, including the hang. Node 24.20.0's loader changes (nodejs/node#63917 "enforce path normalization before lookup", alongside #62239's package-maps work) break the .ts-to-.js specifier rewrite @adonisjs/ace's FsLoader relies on to dynamically import apps/api/commands/*.ts files. Pin every CI job and local dev (.nvmrc) to 24.18.1 until ace or Node fixes this — that's the actual fix. Production isn't affected regardless of patch version, since it runs the pre-compiled build/ output, not source .ts files needing this loader trick. Also narrows the try/catch added in the previous commit (per review feedback) to only swallow this specific known error and rethrow anything else — it was catching every boot() failure, which would have hidden a real regression in apps/api/commands/*.ts just as easily as this Node-version issue. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (6 files)
Fix these issues in Kilo Cloud Previous Review Summary (commit de26dc2)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit de26dc2)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (4 files)
Reviewed by deepseek-v4-pro-0813 · Input: 42K · Output: 27K · Cached: 538.8K Review guidance: REVIEW.md from base branch |
Found the upstream report: adonisjs/ace#169 ("Custom commands fail to load on Node.js 24.20.0 with 'Invalid URL'"), already fixed in ace 14.1.1 (adonisjs/ace#170, merged the same week). @adonisjs/core@7.4.0 depends on ^14.1.0, which still resolves to the broken 14.1.0, so pnpm-workspace.yaml now overrides @adonisjs/ace to 14.1.1 project-wide — the same mechanism already used there for @poppinss/utils, with the same rationale documented inline. This is the real fix, not the Node-version pin from the previous commit — verified locally by running apps/api's suite on Node 24.20.0 with the override in place: 531/531 passing, no warm-up warning. Reverts that pin (.github/actions/setup/action.yml, native-build.yml, .nvmrc) back to floating on Node 24, and updates the bootstrap.ts comment to point at the dependency fix instead. The narrowed try/catch from the previous commit stays as defense-in-depth, but should no longer ever trigger. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
brianramseyau
changed the title
| dependencies: | ||
| '@eslint-community/regexpp': 4.12.2 | ||
| '@typescript-eslint/parser': 8.67.0(eslint@10.8.1(jiti@2.7.0))(typescript@6.0.3) | ||
| '@typescript-eslint/parser': 8.67.0(eslint@9.39.5(jiti@2.7.0))(typescript@5.9.3) |
There was a problem hiding this comment.
SUGGESTION: Unrelated @typescript-eslint peer-dependency churn introduced by the lockfile re-resolve
Adding the @adonisjs/ace override forced a full lockfile re-resolve that also re-resolved this plugin's @typescript-eslint/parser peer from eslint@10.8.1/typescript@6.0.3 to eslint@9.39.5/typescript@5.9.3 — while the plugin itself (and its remaining deps, e.g. type-utils, eslint: 10.8.1, typescript: 6.0.3 a few lines below) is still peer-resolved against eslint@10.8.1/typescript@6.0.3. The parser now links a different TypeScript/eslint than the plugin that consumes it, which is unrelated to the ace bump and can surface as a "TypeScript version used by @typescript-eslint/typescript-estree is not compatible" mismatch in type-aware linting. It also suggests the lockfile may have been regenerated under a pnpm version other than the packageManager pin (pnpm@10.33.0), which risks --frozen-lockfile drift in CI. Consider regenerating the lockfile cleanly under pnpm@10.33.0 and confirming the parser peer resolves back to eslint@10.8.1/typescript@6.0.3.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
* Pin Node to 24.20.0 everywhere Was floating on the 24.x major (>=24 engine range, unpinned Dockerfile/CI tags), which let the Node 24.20 ace loader regression (#199) reach prod undetected until it broke tests. Pin package.json engines, .nvmrc, docker/Dockerfile, docker/dev.Dockerfile, and both CI node-version fields to the same exact version. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Loosen engines.node to a floor instead of an exact pin Exact-pinning engines.node warns (and hard-fails under apps/web/.npmrc's engine-strict=true) on every future patch release. The actual reproducible pin lives in .nvmrc/Docker/CI already; engines should just express the minimum supported version. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
1 participant
Summary
Root-causes and fixes the "Invalid command exported... Invalid URL" hang that appeared after #198's warm-up-boot fix, per Kilo's review feedback that swallowing it broadly masked a real, deterministic issue rather than a flaky one.
Root cause: Node 24.20.0 shipped loader changes (nodejs/node#63917 "enforce path normalization before lookup", alongside #62239's package-maps work) that break
@adonisjs/ace's command-metadata validator — already reported and fixed upstream as adonisjs/ace#169 / #170, released in ace14.1.1.@adonisjs/core@7.4.0depends on^14.1.0, which still resolves to the broken14.1.0by default.The fix:
@adonisjs/ace: 14.1.1topnpm-workspace.yaml's existingoverridesblock (same mechanism/pattern already used there for@poppinss/utils, with the reasoning documented inline).apps/api's suite is clean on 24.18.1, reproduces the exact CI error and the hang 100% of the time on 24.20.0 — and with theaceoverride in place, 531/531 pass cleanly on 24.20.0 too.try/catchadded in Split monolithic CI job into parallel jobs #198 (per Kilo's specific feedback) to only swallow this exact known error message and rethrow anything else, so a real regression inapps/api/commands/*.tscan't hide behind it. With the override in place it shouldn't ever trigger — it's defense-in-depth, not the fix..tsfiles, so the loader trick this bug lives in never runs there.Test plan
apps/apitests pass clean on 24.18.1@adonisjs/aceoverride — confirms the real fixtest / Test (api)completes fast (no hang)🤖 Generated with Claude Code