fix(doctor): probe ast-grep so bootstrap stops reporting ok on a machine where edit.ast-grep-plan cannot run - #143
Conversation
…t-grep-plan
`doctor bootstrap --json` reported a `tools` array of exactly six entries —
rg, git, python, repowise, repomix, sentrux — because the probe list is
hardcoded in doctor_bootstrap/mod.rs. ast-grep was not in it.
Meanwhile `orchestration/integrations.json` ships `edit.ast-grep-plan` as a
capability with `runtimeAdapter: edit.ast-grep-plan.compat`, and that adapter
resolves `ast-grep` through `tool_path` before shelling out to
`ast-grep --version`. On a machine without ast-grep the doctor answered
`ok: true, missing: []` and the truth only arrived at capability-exec time as
`AdapterError::Unavailable("start ast-grep: ...")`. CI treats the tool as a
hard dependency — both jobs install a sha256-pinned 0.42.3 and throw on a
version mismatch — while the doctor treated it as nonexistent.
The probe is optional (`required: false`), matching what
`orchestration/toolchain-versions.v1.json` already declares for ast-grep and
matching repomix, so no machine that was green turns red. The fix is that the
observation now reports the tool at all: `found` plus a resolved `source`,
and a `MISSING ast-grep` line in the human rendering when it is absent.
Tests: the existing tool-name-order assertion in mod.rs gains "ast-grep", and
a new integration test reads the shipped manifest, confirms
`edit.ast-grep-plan` is still a runtime capability, and only then requires the
probe list to cover it — so retiring the capability relaxes the test instead
of leaving a restated constant behind. Both fail with the probe line removed.
`capabilityDeclaration.implementation.toolchainDigests` for the doctor
capability is re-pinned to mod.rs's new sha256 (verified by hand; #133).
Refs #F4-doctor-blind-to-ast-grep
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
🔒 Repowise is not analyzing this repository The PR bot is free on public repositories. This one is private, which needs a Pro plan. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe doctor bootstrap now probes ChangesDoctor bootstrap observation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
Code Intel change risk
Top signals
revspec: |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/code-intel-cli/tests/doctor_bootstrap_cli.rs (1)
251-304: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCover the complete manifest-to-observation contract.
The test only checks that
ast-grepexists, is optional in the output, and has a booleanfoundvalue. It does not compare against the capability'srequiredfield inorchestration/integrations.json, verify the emittedsource, or exercise a controlled missing-tool case. A regression that dropssourceor always reportsfound: falsecan pass.Capture the manifest entry, assert its
requiredvalue, compare it withprobed["required"], and assert thatsourcematchesfound. Also assert that an absent optional tool is not added tomissing.Suggested assertions
- let ships_ast_grep_plan = manifest["integrations"] + let capability = manifest["integrations"] .as_array() .expect("integrations array") .iter() - .any(|entry| entry["id"] == "edit.ast-grep-plan" && entry["runtimeAdapter"].is_string()); - assert!( - ships_ast_grep_plan, - "edit.ast-grep-plan is no longer a shipped runtime capability; drop this test rather than weakening it" - ); + .find(|entry| entry["id"] == "edit.ast-grep-plan") + .expect("edit.ast-grep-plan capability"); + assert!(capability["runtimeAdapter"].is_string()); + assert_eq!(capability["required"], json!(false)); - assert_eq!(probed["required"], json!(false)); - assert!(probed["found"].is_boolean()); + assert_eq!(probed["required"], capability["required"]); + let found = probed["found"].as_bool().expect("found boolean"); + let source = probed["source"].as_str().expect("source string"); + assert_eq!(found, !source.is_empty()); + assert!(!observation["missing"] + .as_array() + .expect("missing array") + .iter() + .any(|value| value == "ast-grep"));🤖 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 `@crates/code-intel-cli/tests/doctor_bootstrap_cli.rs` around lines 251 - 304, Strengthen the test the_probe_list_covers_the_external_tool_a_shipped_capability_shells_out_to by retaining the matching edit.ast-grep-plan manifest entry, reading its required value, and asserting it matches probed["required"]. Verify the probe emits a source consistent with probed["found"], and in the controlled missing-tool scenario assert that ast-grep is not included in the observation's missing list because it is optional.
🤖 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 `@crates/code-intel-cli/tests/doctor_bootstrap_cli.rs`:
- Around line 251-304: Strengthen the test
the_probe_list_covers_the_external_tool_a_shipped_capability_shells_out_to by
retaining the matching edit.ast-grep-plan manifest entry, reading its required
value, and asserting it matches probed["required"]. Verify the probe emits a
source consistent with probed["found"], and in the controlled missing-tool
scenario assert that ast-grep is not included in the observation's missing list
because it is optional.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 33cc104c-e15c-4165-809c-1fe24bd83ffb
📒 Files selected for processing (3)
crates/code-intel-cli/src/doctor_bootstrap/mod.rscrates/code-intel-cli/tests/doctor_bootstrap_cli.rsorchestration/integrations.json
The finding
code-intel doctor bootstrap --repo-path . --no-require-repowise --require-provider-conformance --jsonreturned"ok": true,"missing": [], and atoolsarray of exactly six entries:rg,git,python,repowise,repomix,sentrux. The probe list is hardcoded incrates/code-intel-cli/src/doctor_bootstrap/mod.rsand contained noast-grep.That is a blind spot with three independent confirmations in this tree:
orchestration/integrations.jsonshipsedit.ast-grep-planas a capability withruntimeAdapter: edit.ast-grep-plan.compat.crates/code-intel-cli/src/structured_edit.rsresolvestool_path::resolve("ast-grep")and shells out toast-grep --version; a machine without it fails at capability-exec time withAdapterError::Unavailable("start ast-grep: ...")..github/workflows/ci.ymlinstalls a sha256-pinned ast-grep 0.42.3 in both jobs and throws on a version mismatch, andorchestration/toolchain-versions.v1.jsoncarries anast-grepentry with acommandprobe.So CI treated ast-grep as a hard dependency while the doctor treated it as nonexistent.
The fix
One probe added to the
toolsvector,required: false.Optional is the honest setting, not a softening:
orchestration/toolchain-versions.v1.jsonalready declaresast-grepwith"required": false, andintegrations.jsondeclaresedit.ast-grep-planwith"required": false. Making it required would have flipped the doctor red on every installed machine without ast-grep — a gate change, not a bug fix. What changes is that the observation now reports the tool:foundplus a resolvedsourcepath, and aMISSING ast-grepline in the human rendering when it is absent, instead of silently omitting it.No existing green turns red, and no gate was weakened, waived, or relabelled.
Tests
Two, both of which fail if the one-line probe is reverted:
doctor_bootstrap::tests::observation_carries_the_v1_contract_and_every_retired_check— the existing tool-name-order assertion gains"ast-grep".the_probe_list_covers_the_external_tool_a_shipped_capability_shells_out_to(new,tests/doctor_bootstrap_cli.rs) — reads the shippedorchestration/integrations.json, confirmsedit.ast-grep-planis still a runtime capability, and only then requires the probe list to cover it. The manifest read is the point: retiring the capability relaxes the test instead of leaving a restated constant behind.Revert proof. With
probe::probe_tool("ast-grep", false, prefix)removed and nothing else changed:Restored, both pass.
Digest re-pin
capabilityDeclaration.implementation.toolchainDigestsfor thedoctorcapability pinsdoctor_bootstrap/mod.rs. Re-pinned by hand (b4b6fc64… -> ef26c267…) and verified with sha256, per the knownrepin --writeblind spot (#133).registry_toolchain_digests_bind_the_adapter_and_dispatch_sourcescaught the stale pin and now passes.Verification
cargo test --workspace --locked— 2959 passed, 51 suites, fully green.cargo fmt -p code-intel -- --check— clean.run execute --repo . --manifest orchestration/integrations.json --doctor-require-repowise false) — exit 0,"failures": {"domain": [], "process": []}, all 8 nodesverdict: pass.mod.rssits at 25 recognised functions, and the rule isfunctions > 25 && loc > 400. Adding any newfnto that file — including a test fn — would have made it a god file and tripped the ratchet. That is why the new test lives intests/doctor_bootstrap_cli.rsand the in-file change is a comment plus one expression. Post-change:mod.rsloc 779 / 25 fns,doctor_bootstrap_cli.rsloc 312 / 11 fns — neither trips it.Post-fix output
{"found": true, "name": "ast-grep", "required": false, "source": "...\\Scripts\\ast-grep.exe"}Seven entries now, and the doctor answers for a tool a shipped capability cannot run without.
Refs the dogfooding finding
F4-doctor-blind-to-ast-grep. Do not merge without review.