fix(providers): 拒绝站在 A 仓验证 B 仓,并让 CODE_INTEL_REPO_ROOT 真的生效 - #82
Conversation
|
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 change adds checkout-aware manifest validation, makes integration manifest selection explicit, supports repository-root overrides in PowerShell forwarders, and updates graph conformance provenance hashes. ChangesManifest resolution and entrypoint selection
Graph provenance refresh
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant code_intel_cli
participant current_checkout
participant orchestration_manifest
code_intel_cli->>current_checkout: discover local manifest
current_checkout-->>code_intel_cli: return manifest path
code_intel_cli->>orchestration_manifest: validate checkout identity
orchestration_manifest-->>code_intel_cli: accept matching checkout or reject foreign checkout
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 |
…ther `orchestration_manifest()` resolves CODE_INTEL_HOME before the built-from and cwd candidates. On a machine where that variable names a primary checkout — the normal setup — every provider/route validation run from a git worktree read the *primary* checkout's manifest while appearing to validate the worktree. The dangerous direction is not the false failure this produces when the two disagree; it is that a worktree with a genuinely broken registry reports green whenever the primary checkout happens to be consistent. Resolution order is unchanged. When the resolved manifest and the manifest belonging to the current directory are different files, the call now fails with both paths named and the two ways to disambiguate. CODE_INTEL_INTEGRATIONS_MANIFEST is exempt: it is an explicit statement of which file the operator means, so there is nothing to disambiguate. Registry unit tests no longer read the ambient variable at all — they resolve the checkout they were compiled from, which is what they were always asserting about. graph_adapter's spawned-binary validation names its manifest explicitly for the same reason. Also makes CODE_INTEL_REPO_ROOT real. install-code-intel-pipeline.ps1 tells the operator to set it when a forwarder cannot find its repo, but the generated forwarder hardcoded the install-time path and never read the variable, so the advice was a dead end. Verified both directions: pointing it at a missing root produces the documented error, unsetting it restores the shim. Refs #78
The first cut of this guard treated any orchestration/integrations.json
under the current directory as a rival checkout. That broke a guarantee
test-integration-orchestration.ps1:243 has always asserted: standing in
an unrelated directory that merely happens to contain a file at that
path must not shadow an explicitly named CODE_INTEL_HOME. CI caught it —
the fixture writes {"policy":{"name":"unrelated"},"integrations":[]} and
expects resolution to succeed.
Both cases look identical from the path alone, so the discriminator is
`is_safe_cwd_manifest`, which the last-resort cwd branch already trusts
for exactly this question. An unrelated file is not a rival: the
operator named a checkout and nothing here contests it. A real
code-intel registry is a rival, and that is the worktree case the guard
exists for.
Verified both directions against a rebuilt binary: the CI fixture
scenario (cwd = unrelated manifest, CODE_INTEL_HOME = checkout) now
returns ok with zero errors, while cwd = this worktree with
CODE_INTEL_HOME = the primary checkout still refuses. The PowerShell
suite passes when CODE_INTEL_HOME names the checkout under test, which
is what CI does.
Refs #78
1a1d46b to
92b9323
Compare
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/code-intel-cli/src/providers.rs (2)
1254-1267: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared ancestor-walk-for-safe-manifest logic.
cwd_manifest_path()(Lines 1254-1267) and the cwd fallback loop insideresolve_orchestration_manifest()(Lines 1355-1363) both walkcwd.ancestors()and test each candidate withis_safe_cwd_manifest. Keeping this walk in two places risks the two copies drifting apart if one is updated (for example, to change the ancestor-search termination rule) and the other is not — which would reintroduce the exact detection gap this PR fixes.Extract a single helper, for example
fn find_safe_cwd_manifest() -> Option<PathBuf>, and call it from bothcwd_manifest_path()and the fallback branch inresolve_orchestration_manifest().Also applies to: 1355-1363
🤖 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/src/providers.rs` around lines 1254 - 1267, Extract the shared cwd ancestor traversal and safe-manifest filtering into a helper such as find_safe_cwd_manifest(), preserving the current candidate construction and is_safe_cwd_manifest checks. Update cwd_manifest_path() and the fallback branch in resolve_orchestration_manifest() to call this helper so both paths use identical search behavior.
1242-1249: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDecouple the "explicit" bypass decision from the actual manifest-resolution source.
orchestration_manifest()computesexplicitby independently callingenv::var("CODE_INTEL_INTEGRATIONS_MANIFEST").is_ok(), at Line 1243. In non-test builds this matches the same check insideresolve_orchestration_manifest(). But in#[cfg(test)]builds,test_manifest_override()(Line 1304) runs before theCODE_INTEL_INTEGRATIONS_MANIFESTcheck and returns unconditionally if it finds a manifest. If a unit test setsCODE_INTEL_INTEGRATIONS_MANIFESTwhiletest_manifest_override()also succeeds,orchestration_manifest()will treat the call as "explicit" and skipreject_foreign_checkout, even though the manifest actually used came from the built-checkout override, not from the env var the caller set. This decouples the bypass decision from what was actually resolved, which is exactly the kind of silent-false-PASS this PR is designed to eliminate.Reorder so the explicit env var always takes priority over the test override, keeping the two checks in sync:
🐛 Proposed fix to restore priority of the explicit override
fn resolve_orchestration_manifest() -> std::result::Result<(PathBuf, PathBuf), String> { - #[cfg(test)] - if let Some(path) = test_manifest_override() { - return manifest_candidate(path).ok_or_else(|| { - "built-from checkout has no readable integrations manifest".to_string() - }); - } - if let Ok(explicit) = env::var("CODE_INTEL_INTEGRATIONS_MANIFEST") { let path = PathBuf::from(explicit); let path = if path.is_absolute() { path } else { env::current_dir() .map_err(|error| format!("cannot resolve current directory: {error}"))? .join(path) }; return manifest_candidate(path).ok_or_else(|| { "CODE_INTEL_INTEGRATIONS_MANIFEST does not identify a readable integrations manifest" .to_string() }); } + + #[cfg(test)] + if let Some(path) = test_manifest_override() { + return manifest_candidate(path).ok_or_else(|| { + "built-from checkout has no readable integrations manifest".to_string() + }); + }This still guards unit tests against ambient
CODE_INTEL_HOME, which is the case the surrounding comment (Lines 1289-1292) calls out, while keeping theexplicitbypass inorchestration_manifest()truthful.Also applies to: 1302-1309
🤖 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/src/providers.rs` around lines 1242 - 1249, Update resolve_orchestration_manifest and its test-only test_manifest_override path so CODE_INTEL_INTEGRATIONS_MANIFEST is checked before any test override and always takes precedence. Ensure orchestration_manifest’s explicit decision remains aligned with the manifest source actually returned, while preserving the test override behavior when the explicit environment variable is absent.
🤖 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/src/providers.rs`:
- Around line 1254-1267: Extract the shared cwd ancestor traversal and
safe-manifest filtering into a helper such as find_safe_cwd_manifest(),
preserving the current candidate construction and is_safe_cwd_manifest checks.
Update cwd_manifest_path() and the fallback branch in
resolve_orchestration_manifest() to call this helper so both paths use identical
search behavior.
- Around line 1242-1249: Update resolve_orchestration_manifest and its test-only
test_manifest_override path so CODE_INTEL_INTEGRATIONS_MANIFEST is checked
before any test override and always takes precedence. Ensure
orchestration_manifest’s explicit decision remains aligned with the manifest
source actually returned, while preserving the test override behavior when the
explicit environment variable is absent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3a2f425d-89d3-487a-8bf0-d475e3e18c4d
📒 Files selected for processing (4)
crates/code-intel-cli/src/providers.rscrates/code-intel-cli/tests/graph_adapter.rslegacy/install-code-intel-pipeline.ps1orchestration/internalization/graph.json
1 participant
叠在 #81 上。
问题
orchestration_manifest()的解析顺序把CODE_INTEL_HOME排在「编译自哪棵树」和 cwd 之前。本机那个变量指向主 checkout——这是常规配置——于是在 worktree 里跑的每一次 provider/route 校验,读的都是主 checkout 的 manifest,却看起来在验 worktree。危险的方向不是两边不一致时的假失败,而是:worktree 的注册表真坏了,只要主 checkout 恰好自洽,结果照样绿。
改法
解析顺序不动。当解析出的 manifest 与「当前目录所属的 manifest」是两个不同文件时,直接失败并同时报出两条路径和两种消歧方式:
CODE_INTEL_INTEGRATIONS_MANIFEST豁免——它是操作者对「就用这一个文件」的明确声明,不存在歧义。注册表单元测试不再读环境变量,改为解析自己编译时所在的那棵树(它们本来断言的就是这个)。
graph_adapter里那次 spawn 二进制的校验同理,显式指定自己的 manifest。顺带:CODE_INTEL_REPO_ROOT 从来没生效过
install-code-intel-pipeline.ps1:444在 forwarder 找不到仓时提示「set CODE_INTEL_REPO_ROOT to override」,但生成的 forwarder 把安装时路径写死,从不读那个变量。全仓搜索,这个名字只出现在这句提示和三个option_env!测试辅助里——提示是死路一条。现在真读了。双向验证:指向不存在的根会给出那句文档化的错误,取消设置后 shim 恢复正常。
这条不是凭空想的:本机自扫的
doctor节点一直是红的,查下来是%LOCALAPPDATA%\code-intel\bin\sentrux-shim.ps1里写死的路径指向...\archive,而 #77 把archive/改名成了legacy/。forwarder 是改名前装的,提示让人设的那个变量又是死的,于是只能重装。验证
cargo test全绿test-atomic-capability-contract.ps1通过test-retirement-packets.ps18 packets + 2 audits 通过No degradation detectedorchestration/internalization/graph.json的 conformance digest(改了tests/graph_adapter.rs)Refs #78