fix(rpc): readPackageJson via fs walk so getNetworkInfo.nodeVersion isn't sentinel defaults in production - #865
Conversation
… require()
The require("../../package.json") path used in PR #864 silently failed
in production (the rebuilt dev.node2 returned name: "demos-node" +
version: "0.0.0" — the catch-clause defaults). bun + tsconfig-paths
under ESM has unreliable behaviour around `require()`-resolving JSON,
and the failure mode was a silent catch falling through to the wrong
answer rather than a loud throw.
Walk from `import.meta.url` up to the first directory containing a
readable package.json — same pattern the .git/HEAD walker below already
uses, so both halves of the module now behave identically. Skips
nested workspace manifests by requiring a non-empty version field.
Hard cap at 16 levels so a corrupted fs cannot loop. Defensive
fileURLToPath fallback to cwd preserves the "every failure path
returns null" invariant.
After this fix, dev.node2's getNetworkInfo.nodeVersion surfaces the
real package name + semver + git provenance instead of sentinel
defaults, restoring the diagnostic value the original PR intended.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Review limit reached
More reviews will be available in 32 minutes and 32 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ 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 |
Greptile SummaryThis PR fixes
Confidence Score: 4/5Safe to merge; the core fix is correct and well-bounded, with only minor comment inaccuracies. The upward fs-walk correctly finds the root package.json in both the dev tree and the Docker container layout, and the 16-level hard cap prevents any runaway on a corrupted fs. The two comments are documentation issues only and do not affect runtime behavior. Only src/utilities/nodeVersion.ts changed; no other files need attention. Important Files Changed
Reviews (1): Last reviewed commit: "fix(rpc): readPackageJson via fs walk fr..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| * sentinels). Walking from `import.meta.url` up to the first | ||
| * directory that contains a readable `package.json` mirrors what | ||
| * the git-root walker below already does for `.git/HEAD`, so the | ||
| * two halves of the module behave identically. |
There was a problem hiding this comment.
findRepoRoot still starts from process.cwd() (line 166), while the new readPackageJson starts from import.meta.url. The two walkers therefore have different anchors in the normal (non-fallback) path. The claim that they "behave identically" only holds when import.meta.url throws and the fallback process.cwd() kicks in. No correctness impact for the stated use case (the upward walk from the module dir is strictly better for finding the root package.json), but the comment will confuse future readers.
| // Skip nested package manifests (e.g. node_modules/*) by | ||
| // requiring a non-empty `version` field — those exist on | ||
| // every workspace's package.json, including ours. | ||
| if (typeof pkg.version === "string" && pkg.version.length > 0) { |
There was a problem hiding this comment.
version-presence check actually filters. Packages in node_modules virtually always have a non-empty version field, so the check does not suppress them. What it genuinely skips is private workspace-root manifests that omit version entirely. The upward-walk direction is what keeps node_modules entries out of the picture — they live in child directories, never in parent directories relative to the module file.
| // Skip nested package manifests (e.g. node_modules/*) by | |
| // requiring a non-empty `version` field — those exist on | |
| // every workspace's package.json, including ours. | |
| if (typeof pkg.version === "string" && pkg.version.length > 0) { | |
| // Skip private workspace roots that omit `version` entirely. | |
| // Actual node_modules entries are never encountered because the | |
| // walk moves upward through parent directories, not downward. | |
| if (typeof pkg.version === "string" && pkg.version.length > 0) { |
…from docker-run (#866) Closes the gap left by PR #864/#865: src/utilities/nodeVersion.ts already reads process.env.GIT_COMMIT/GIT_BRANCH/GIT_DIRTY when present, but nothing on the docker path was supplying them. The result was that getNetworkInfo.nodeVersion came back with commit: null on every Docker- booted node, defeating the original "is this host running the binary I think it is?" diagnostic value. Three pieces wire it end-to-end: 1) Dockerfile (runtime stage): ARG GIT_COMMIT/GIT_BRANCH/GIT_DIRTY/ BUILT_AT, then re-export each via ENV so the running node sees them in process.env. Defaults are empty strings — a build without the args (manual `docker build .`) still produces a runnable image, nodeVersion just surfaces null for the missing fields. 2) docker-compose.yml (node service build block): args passes the four values straight through with `:-` defaults, so any caller that exported them (the wrapper below, CI, an operator's shell) gets them baked in; anyone who didn't gets an empty-string ARG and the same null-field fallback. 3) scripts/docker-run: detects the working tree's git status with defensive guards (no-repo, no-git-binary, dirty-vs-clean) and exports the four variables before any `docker compose build`/`up`. `git diff-index --quiet HEAD` exit code → "true"/"false" string the nodeVersion module already parses. BUILT_AT is the UTC ISO-8601 timestamp at invocation. Operator-visible effect: after `./scripts/docker-run --rebuild -d`, curl getNetworkInfo and the response carries the real commit SHA, branch name, dirty flag, and build timestamp the image was made from. That answers "did this host actually pick up the fix I merged?" with a one-liner. Manual repro paths still work: docker compose build --build-arg GIT_COMMIT=$(git rev-parse HEAD) GIT_COMMIT=abc123 docker compose build Co-authored-by: tcsenpai <tcsenpai@discus.sh>
1 participant
Repro
After PR #864 merged + dev.node2 rebuilt,
getNetworkInfo.nodeVersioncame back as:Those are exactly the catch-clause defaults.
require(\"../../package.json\")was silently failing under bun + tsconfig-paths' ESM/CJS interop, and the failure landed on the wrong-answer fallback instead of throwing loudly.Fix
Walk from
import.meta.urlup to the first directory containing a readablepackage.json— same pattern the.git/HEADwalker already uses. Skips nested workspace manifests by requiring a non-emptyversionfield. Hard cap at 16 levels so a corrupted fs cannot loop. DefensivefileURLToPathfallback tocwdpreserves the "every failure path returnsnull" invariant..git/absence in the image is expected and staysnull— that part of the response already does what it should.Test plan
bun --evalsmoke locally returns real values:bun run type-check-ts— no new errorsgetNetworkInfoagain; expect non-sentinelname/version(commit/branch likely still null until.git/is shipped or the GIT_COMMIT env-var override is set)Followup (optional)
If we want
commitpopulated in production too, ship the SHA via a Dockerfile build arg:Then
docker compose build --build-arg GIT_COMMIT=$(git rev-parse HEAD). Out of scope for this PR.