fix(version): walk up to find @automagik/genie package.json (#1464) - #1486
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
Closes #1464 — `genie --version` reported a stale version on dogfood boxes where the binary lived inside a worktree: - worktree package.json: 4.260428.19 - genie --version: 4.260428.16 Root cause (per dog-fooder verdict in state/evidence/1474-d3976b96-20260428T235714Z/1464): The previous resolver tried fixed `..` and `../..` paths from import.meta.dir and accepted the FIRST package.json that existed. For a binary at <repo>/.worktrees/<name>/dist/genie.js, the `../..` candidate resolved to <repo>/package.json (the *parent* repo, often a different version) BEFORE the `..` candidate would have found the worktree's own <repo>/.worktrees/<name>/package.json. Fix: walk UP from the binary's location and return the version of the FIRST package.json whose `name === "@automagik/genie"`. This guarantees we identify our own package no matter how deep the binary lives — the parent-repo package.json is correctly skipped because the worktree's package.json (with matching name) is hit first on the way up. Two-pass strategy: 1. Walk up looking for `name === "@automagik/genie"` (primary) 2. Walk up again accepting any package.json with a `version` field (fallback for tarballs / detached envs where `name` is missing) Bounded by MAX_WALK_DEPTH=10 + filesystem-root stop. Validation: - 6/6 src/lib/version.test.ts pass (new file): VERSION non-empty, matches closest @automagik/genie package.json walking up from this file, source contract assertions for PACKAGE_NAME, MAX_WALK_DEPTH, two-pass order, and root-stop semantics - Empirical: bun -e "import('./src/lib/version.js')..." in this worktree reports 4.260428.19; from the parent repo reports parent's 4.260428.16 - biome clean, tsc clean Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a worktree-aware version resolver that performs an upward directory walk to locate the @automagik/genie package.json, resolving issue #1464. The changes include a new test suite that verifies the resolver's logic. Feedback suggests optimizing the implementation by combining the two-pass directory walk into a single pass to reduce redundant filesystem I/O. Additionally, it is recommended to refactor the tests to focus on behavioral verification using a mocked filesystem rather than asserting against the source code's text, which is currently brittle.
| let current = startDir; | ||
| for (let depth = 0; depth < MAX_WALK_DEPTH; depth++) { | ||
| const candidate = resolve(current, 'package.json'); | ||
| const pkg = readPackageJson(candidate); | ||
| if (pkg?.name === PACKAGE_NAME && pkg.version) { | ||
| return pkg.version; | ||
| } | ||
| const parent = dirname(current); | ||
| if (parent === current) break; // reached filesystem root | ||
| current = parent; | ||
| } | ||
|
|
||
| // Fallback: same walk but accept ANY package.json with a version field. | ||
| // This catches edge cases like running directly from a tarball where | ||
| // `name` may be unset, and preserves the prior resolver's lenience. | ||
| current = startDir; | ||
| for (let depth = 0; depth < MAX_WALK_DEPTH; depth++) { | ||
| const candidate = resolve(current, 'package.json'); | ||
| const pkg = readPackageJson(candidate); | ||
| if (pkg?.version) return pkg.version; | ||
| const parent = dirname(current); | ||
| if (parent === current) break; | ||
| current = parent; | ||
| } | ||
|
|
||
| return FALLBACK_VERSION; |
There was a problem hiding this comment.
The directory walk can be optimized into a single pass. The current implementation performs two separate loops, which results in redundant filesystem operations (reading and parsing the same package.json files twice). By tracking a fallback version during the first walk, you can achieve the same prioritization logic with half the filesystem I/O and less code duplication. Note that using a hardcoded limit like MAX_WALK_DEPTH is acceptable here to prevent excessive I/O.
let fallbackVersion: string | null = null;
let current = startDir;
for (let depth = 0; depth < MAX_WALK_DEPTH; depth++) {
const candidate = resolve(current, 'package.json');
const pkg = readPackageJson(candidate);
if (pkg?.version) {
// Priority 1: The first package.json that matches our name (worktree-aware)
if (pkg.name === PACKAGE_NAME) return pkg.version;
// Priority 2: The closest package.json that has a version (fallback)
fallbackVersion ??= pkg.version;
}
const parent = dirname(current);
if (parent === current) break; // reached filesystem root
current = parent;
}
return fallbackVersion ?? FALLBACK_VERSION;References
- It is acceptable to use hardcoded numeric limits (magic numbers) in non-critical fallback logic, especially when they serve as intentional caps to prevent performance issues like excessive I/O.
| test('source identifies @automagik/genie as the package name to match', () => { | ||
| expect(versionSource).toContain("PACKAGE_NAME = '@automagik/genie'"); | ||
| }); | ||
|
|
||
| test('source bounds the walk depth (no runaway scans)', () => { | ||
| expect(versionSource).toContain('MAX_WALK_DEPTH'); | ||
| }); | ||
|
|
||
| test('source matches by package name FIRST, then falls back to any version', () => { | ||
| // Two-pass walk: name-matched, then any-package-with-version | ||
| const namePassIdx = versionSource.indexOf('pkg?.name === PACKAGE_NAME'); | ||
| const fallbackPassIdx = versionSource.indexOf('Fallback'); | ||
| expect(namePassIdx).toBeGreaterThan(0); | ||
| expect(fallbackPassIdx).toBeGreaterThan(namePassIdx); | ||
| }); | ||
|
|
||
| test('source explicitly stops at filesystem root', () => { | ||
| expect(versionSource).toMatch(/parent === current/); | ||
| }); |
There was a problem hiding this comment.
These tests are highly brittle because they assert against the raw source code of version.ts using string matching and index comparisons. This approach couples the tests to implementation details (like specific variable names, comments, and the two-pass structure), which will cause them to fail during routine refactoring even if the logic remains correct.
Consider replacing these with behavioral tests, for example by using a mocked filesystem to verify that the resolver correctly prioritizes the named package over a generic one at different depths.
d152400 to
c8f3383
Compare
1 participant
Summary
Closes #1464 —
genie --versionreported a stale version on dogfood boxes where the binary lived inside a worktree:package.json: 4.260428.19genie --version: 4.260428.16Root cause (dog-fooder analysis)
Per evidence in
state/evidence/1474-d3976b96-20260428T235714Z/1464:The previous resolver tried fixed
..and../..paths fromimport.meta.dirand accepted the FIRSTpackage.jsonthat existed. For a binary at<repo>/.worktrees/<name>/dist/genie.js:../..candidate →<repo>/package.json(parent repo — wrong)..candidate →<repo>/.worktrees/<name>/package.json(our own — right)The
../..candidate is checked first and wins, returning the parent repo's version.Fix
Walk UP from the binary's location and return the version of the FIRST
package.jsonwhosename === "@automagik/genie". The parent-repopackage.jsonis correctly skipped because the worktree's ownpackage.json(with matching name) is hit first on the way up.Two-pass strategy:
name === '@automagik/genie'(primary, worktree-aware)package.jsonwith aversionfield (fallback for tarballs / detached envs wherenameis missing)Bounded by
MAX_WALK_DEPTH=10+ filesystem-root stop.Empirical verification
Validation
src/lib/version.test.tspass (new file): VERSION non-empty + non-fallback, matches closest@automagik/geniepackage.json, source contract assertions for PACKAGE_NAME / MAX_WALK_DEPTH / two-pass order / root-stopEvidence credit
dog-fooder (the bare
genie/dog-fooderinstance) pinned the smoking gun insrc/lib/version.tswith full file path enumeration. Full audit trail in evidence dir cited above.