fix(doctor): note when a store checkout is behind its upstream ref#1287
Open
mc856 wants to merge 2 commits into
Open
fix(doctor): note when a store checkout is behind its upstream ref#1287mc856 wants to merge 2 commits into
mc856 wants to merge 2 commits into
Conversation
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
Contributor
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds git checkout drift detection (ahead/behind upstream tracking counts) to the store health system. A new 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: isGitRepositoryAtRoot(storeRoot)
GitModule-->>DoctorCommand: isRepo
DoctorCommand->>GitModule: gitTrackingDrift(storeRoot)
GitModule-->>DoctorCommand: drift or null
DoctorCommand->>RelationshipHealth: inspectRelationships(storeFacts with drift)
RelationshipHealth->>RelationshipHealth: check drift.behind > 0
RelationshipHealth-->>DoctorCommand: store health with status/drift
DoctorCommand-->>User: JSON/human output
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
Contributor
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
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.
Author
|
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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
New Features
doctor, including optional JSONstore.driftwith ahead/behind counts when an upstream tracking branch exists.store_checkout_driftwhen the checkout is behind or diverged.Bug Fixes
Documentation
doctor --jsonand relationship-health documentation to reflect the new drift details.Tests