fix(doctor): note when a store checkout is behind its upstream ref#1287
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds Git checkout drift detection to store health. Doctor computes ahead/behind counts, passes them into relationship health, and reports an info diagnostic when the checkout is behind. Documentation and tests cover the new output and scenarios. ChangesStore Checkout Drift Reporting
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant DoctorCommand
participant GitModule
participant RelationshipHealth
User->>DoctorCommand: run openspec doctor
DoctorCommand->>GitModule: gitTrackingDrift(storeRoot)
GitModule-->>DoctorCommand: drift or null
DoctorCommand->>RelationshipHealth: inspectRelationships(storeFacts with drift)
RelationshipHealth->>RelationshipHealth: evaluate behind count
RelationshipHealth-->>DoctorCommand: store health and diagnostic
DoctorCommand-->>User: JSON or human output
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
test/core/relationship-health.test.ts (1)
100-149: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider splitting scenarios into separate
itblocks.This single test covers behind, singular-behind, diverged, ahead-only, in-sync, and no-drift cases. A failure partway through aborts later assertions in the same run and the failure message won't clearly identify which scenario broke. Splitting into named
it.eachor individualitblocks would improve failure isolation and readability, though the current coverage itself is correct.🤖 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 `@test/core/relationship-health.test.ts` around lines 100 - 149, Split the broad scenario coverage in inspectRelationships tests into separate named cases so failures are isolated and easier to read. Keep the existing assertions for behind, singular-behind, diverged, ahead-only, in-sync, and no-drift, but move them into individual it blocks or an it.each table in relationship-health.test.ts. Use the inspectRelationships and baseInput helpers to preserve the same coverage while making each scenario’s intent explicit.test/commands/doctor.test.ts (1)
215-310: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared git-bootstrap helper to reduce duplication.
The
git init/add/commit/capture-headboilerplate (Lines 216-226, 254-264, 293-299) is repeated near-verbatim across all three new tests. Consider a shared helper (e.g.createGitStoreWithBase(storeRoot, tempDir)) that returns{ git, head }to keep the scenario-specific logic (upstream setup, divergence) as the only differentiator per test.♻️ Sketch of a shared helper
function createGitStore(storeRoot: string, tempDir: string) { const gitEnv = { ...process.env, ...isolatedGitEnv(tempDir) }; const git = (args: string[]) => execFileSync('git', args, { cwd: storeRoot, env: gitEnv, stdio: 'ignore' }); git(['init']); git(['add', '-A']); git(['commit', '-m', 'shared base']); const head = execFileSync('git', ['branch', '--show-current'], { cwd: storeRoot }) .toString() .trim(); return { git, head }; }🤖 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 `@test/commands/doctor.test.ts` around lines 215 - 310, The three doctor tests repeat the same git bootstrap setup, so extract that shared setup into a helper to reduce duplication. Move the repeated git init/add/commit and current-branch capture logic into a reusable helper near the test block, returning the shared `git` command wrapper and `head` value, then update the `notes an upstream-behind store checkout as info drift`, `reports diverged drift when the checkout is both ahead and behind`, and `reports no drift for a store checkout with no upstream tracking branch` tests to use it while keeping only the scenario-specific upstream/divergence steps inline.src/commands/doctor.ts (1)
62-64: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winParallelize the two independent git probes.
gitOriginUrlandgitTrackingDriftare independent async git subprocess calls but are awaited sequentially, adding avoidable latency todoctor.♻️ Proposed refactor
const isRepo = await isGitRepositoryAtRoot(root.path); - const originUrl = isRepo ? await gitOriginUrl(root.path) : null; - const drift = isRepo ? await gitTrackingDrift(root.path) : null; + const [originUrl, drift] = isRepo + ? await Promise.all([gitOriginUrl(root.path), gitTrackingDrift(root.path)]) + : [null, null];🤖 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 `@src/commands/doctor.ts` around lines 62 - 64, In doctor() the independent git subprocess probes gitOriginUrl and gitTrackingDrift are awaited one after the other, which adds unnecessary latency. Refactor the isRepo branch to start both calls together and await them in parallel, keeping the existing isGitRepositoryAtRoot check and preserving the current null behavior when the root is not a git repository.
🤖 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.
Nitpick comments:
In `@src/commands/doctor.ts`:
- Around line 62-64: In doctor() the independent git subprocess probes
gitOriginUrl and gitTrackingDrift are awaited one after the other, which adds
unnecessary latency. Refactor the isRepo branch to start both calls together and
await them in parallel, keeping the existing isGitRepositoryAtRoot check and
preserving the current null behavior when the root is not a git repository.
In `@test/commands/doctor.test.ts`:
- Around line 215-310: The three doctor tests repeat the same git bootstrap
setup, so extract that shared setup into a helper to reduce duplication. Move
the repeated git init/add/commit and current-branch capture logic into a
reusable helper near the test block, returning the shared `git` command wrapper
and `head` value, then update the `notes an upstream-behind store checkout as
info drift`, `reports diverged drift when the checkout is both ahead and
behind`, and `reports no drift for a store checkout with no upstream tracking
branch` tests to use it while keeping only the scenario-specific
upstream/divergence steps inline.
In `@test/core/relationship-health.test.ts`:
- Around line 100-149: Split the broad scenario coverage in inspectRelationships
tests into separate named cases so failures are isolated and easier to read.
Keep the existing assertions for behind, singular-behind, diverged, ahead-only,
in-sync, and no-drift, but move them into individual it blocks or an it.each
table in relationship-health.test.ts. Use the inspectRelationships and baseInput
helpers to preserve the same coverage while making each scenario’s intent
explicit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9de3b19e-f46d-451c-bce6-749ad6dd7ba8
📒 Files selected for processing (7)
docs/agent-contract.mddocs/cli.mdsrc/commands/doctor.tssrc/core/relationship-health.tssrc/core/store/git.tstest/commands/doctor.test.tstest/core/relationship-health.test.ts
|
Took two of the three — git probes now run in parallel, and the drift tests share an Left the test-split one on purpose: keeping it a single |
5392646 to
ce48875
Compare
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
The implementation looks sound at ce48875: the current merge ref builds and the doctor/relationship-health tests pass locally. I cannot approve yet because this head has no project CI runs at all, only CodeRabbit; please rebase or push an update to trigger the required Linux, macOS, Windows, and lint gates, then ping me.
Stores have no commit pin, so teammates on different commits of the same store silently resolve different specs. Add a read-only info diagnostic (store_checkout_drift) reporting ahead/behind counts against the local upstream ref, and surface them in doctor --json. Fires only when behind (ahead-only is normal — OpenSpec never pushes stores) and never fetches, so it flags already-known drift, not a live cross-machine check. Refs Fission-AI#1273
Run the independent gitOriginUrl / gitTrackingDrift probes concurrently, and extract the repeated git-bootstrap in the drift tests into a shared initGitStore() helper. No behavior change.
ce48875 to
020ebc3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/commands/doctor.test.ts`:
- Around line 48-63: Update the branch-name execFileSync call in initGitStore to
pass the existing isolated gitEnv alongside cwd, matching the environment used
by the git helper commands.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e6f1fbb8-e731-48b8-831e-3fb515b4e1fc
📒 Files selected for processing (7)
docs/agent-contract.mddocs/cli.mdsrc/commands/doctor.tssrc/core/relationship-health.tssrc/core/store/git.tstest/commands/doctor.test.tstest/core/relationship-health.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- test/core/relationship-health.test.ts
- docs/cli.md
- src/core/relationship-health.ts
- src/commands/doctor.ts
|
@alfred-openspec Rebased onto main@9acddcd to trigger the required CI gates — no content changes, same two commits (now at 020ebc3), plus one follow-up from CodeRabbit passing the isolated git env in a test helper (2701a5c). All gates are green on the new head: Linux/macOS/Windows tests, lint & type check. Doctor/relationship-health suites, tsc, lint, and build also pass locally. Ready for another look. |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Verified at 2701a5c after the rebase and isolated-Git test fix: the merge ref builds, 20 focused doctor and relationship-health tests pass locally, and the full Linux, macOS, Windows, lint, and release-tracking gates are green.
3 participants
Why
Stores have no commit pin, so two teammates on different commits of the
same store silently resolve different specs (#1273).
openspec doctoralready reports store health, but only checks recorded-remote vs origin —
it never looks at commit drift, so nothing surfaces the mismatch.
What
Adds a read-only
infodiagnostic,store_checkout_drift, toopenspec doctor. For a git-backed store checkout it reports how far HEADis ahead/behind its upstream tracking ref:
never pushes them), so it stays quiet.
doctor --jsonasstore.drift.It mirrors the existing
store_remote_divergenceinfo diagnostic inseverity and shape, and leaves the exit code unchanged.
Scope
Not a commit pin, and it doesn't touch the "OpenSpec never
clones/pulls/pushes" contract. The comparison is against the local
upstream ref (
@{upstream}), so it flags drift already known locally — itcan't see a remote that advanced before you fetched. A true reproducible
pin (an expected store commit per code repo) is a separate design question;
happy to open a proposal for that rather than fold it in here.
agent-contract.md(§4.9 shape, §6 catalog) andcli.mdare updated to match.Tests
relationship-health.test.ts: behind / diverged / ahead-only-quiet /in-sync / no-drift composition.
doctor.test.ts: real-git fixtures (isolated viaisolatedGitEnv) forbehind, diverged, and no-upstream — asserting JSON and human output.
No changeset yet — happy to add one if you'd like.
Refs #1273
Summary by CodeRabbit
openspec doctor, including optional JSONstore.drift(ahead/behind) when an upstream tracking branch is available.store_checkout_drift) when the store checkout is behind or diverged.doctor --jsonand relationship-health documentation to describe the new drift output.