chore: promote develop to main (edgevec actor + git env fixes) - #207
Conversation
CI runs clippy on Rust 1.97, which enforces clippy::question_mark for this shape; the local toolchain is 1.92 and does not. The lint only became visible now that CI compiles far enough to reach clippy at all — it previously died in the linker. find_crate_deps matched on current.parent() to assign or return None. That is precisely what `?` does, so the behaviour is unchanged. cargo clippy -p mcb-validate --all-targets -- -D warnings: clean.
Uncommitted work-in-progress from the mcb-jdwv lane: EdgeVec actor/client changes for Tokio executor starvation during HNSW insertion, validate run_context simplification, and adjacent test adjustments. Archived the machine-local junk (tool outputs, .cargo/.vscode, data/, context/) that the PR #201 purge had removed but this worktree still carried on disk.
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
Addresses PR #205 review (P1): the WIP purge dropped the GIT_DIR / GIT_WORK_TREE / GIT_INDEX_FILE / GIT_OBJECT_DIRECTORY / GIT_COMMON_DIR env_remove block, so 'git -C <root> ls-files' resolved against the surrounding repository whenever validation ran inside a git operation (pre-commit, rebase, merge). Restores the sanitization and isolates the run_context tests from an inherited git environment.
Addresses PR #205 review (P2): the WIP purge removed the 'pub mod git_helpers_tests;' declaration, leaving git_helpers_tests.rs orphaned on disk and silently stopping the git-isolation coverage for the live mcb_domain::utils::tests::git_helpers module. Re-enables it; the 3 isolation tests pass.
Addresses PR #205 review (P2/P3): the WIP purge replaced run_git (which checked status.success() and surfaced stderr) with bare Command::output()/.status() calls that silently swallowed non-zero git exits and discarded stderr, making setup failures misleading. Captures output and asserts on exit status with stderr in the message in both gap_fixes_e2e and vcs_tests.
Addresses PR #205 review (P3): the dedicated OS actor thread was spawned detached and never joined, so on Tokio teardown while the sender was still held the thread stayed blocked in blocking_recv() instead of being cancelled, risking shutdown hangs/leaks. Stores the JoinHandle and adds a Drop that closes the channel (blocking_recv then returns None and run exits) and joins the thread for deterministic teardown.
# Conflicts: # crates/mcb-validate/src/run_context.rs
fix(edgevec): dedicated OS-thread actor + git env sanitization + review fixes (mcb-jdwv, mcb-88o2)
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
|
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: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 7 files
Confidence score: 4/5
- In
crates/mcb-providers/src/vector_store/edgevec/client.rs, the newDropimplementation joins the actor thread synchronously, so dropping the provider on a Tokio runtime thread can stall shutdown or request teardown while queued messages drain — switch to a non-blocking shutdown path (or bound join time) to avoid runtime blocking. - The test changes in
crates/mcb-server/tests/integration/handlers/vcs_tests.rsandcrates/mcb-server/tests/e2e/gap_fixes_e2e.rsremoverun_gitenvironment sanitization, which can let inheritedGIT_*variables leak into commands and cause host-dependent, flaky failures — restorerun_git(or equivalent full env scrub) for deterministic test behavior. - In
crates/mcb-validate/tests/unit/run_context_tests.rs,git_command()reimplements only part of the shared helper behavior, creating drift frommcb_domain::utils::tests::git_helpersand increasing future inconsistency risk — reuse the centralized helper to keep git test setup aligned across suites.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/mcb-validate/tests/unit/run_context_tests.rs">
<violation number="1" location="crates/mcb-validate/tests/unit/run_context_tests.rs:15">
P3: This change drops the centralized `run_git` helper (still available in `mcb_domain::utils::tests::git_helpers`) and reimplements a partial copy of it as the local `git_command()`, which contradicts the helper module's stated purpose of centralizing git test isolation. The local copy only strips GIT_* env vars; it omits the `GIT_CONFIG_NOSYSTEM=1`, `GIT_CONFIG_GLOBAL=/dev/null` and `core.hooksPath=/dev/null` isolation that `run_git` applies, so the reimplemented path is strictly weaker. Prefer reusing `run_git` (e.g. `run_git(root, &["init"])` and `run_git(root, &["add", "src/lib.rs"])`) so all test suites share the same robust isolation and the duplicate logic is removed.</violation>
</file>
<file name="crates/mcb-server/tests/integration/handlers/vcs_tests.rs">
<violation number="1" location="crates/mcb-server/tests/integration/handlers/vcs_tests.rs:38">
P2: The fixture previously routed through `run_git`, which removed inherited git env vars (GIT_DIR, GIT_WORK_TREE, GIT_INDEX_FILE, GIT_CONFIG, GIT_OBJECT_DIRECTORY, GIT_COMMON_DIR), set GIT_CONFIG_GLOBAL=/dev/null, and added -c core.hooksPath=/dev/null. The deleted comment stated this was required so the fixture is built against the temp dir and not the surrounding repository when the suite runs inside a git hook. The new raw `Command::new("git")` calls drop all of that sanitization, so in a git-hook environment GIT_DIR/GIT_WORK_TREE override the current_dir and these commands operate on the surrounding repo (or a commit hook from global config fails the commit), reintroducing the exact failure the helper was created to prevent. Reuse `run_git(&repo_path, ...)` as before, or clear the same env vars and config on each Command.</violation>
</file>
<file name="crates/mcb-server/tests/e2e/gap_fixes_e2e.rs">
<violation number="1" location="crates/mcb-server/tests/e2e/gap_fixes_e2e.rs:109">
P2: Replacing `run_git` here drops the git environment sanitization that the removed comment explicitly relied on. `run_git` (crates/mcb-domain/src/utils/tests/git_helpers.rs) removes GIT_DIR/GIT_WORK_TREE/GIT_INDEX_FILE/GIT_CONFIG/GIT_OBJECT_DIRECTORY/GIT_COMMON_DIR and disables global/system config and hooksPath. These raw `git add`/`git commit` calls inherit the process environment unchanged, so when the test suite runs inside a git hook (the exact scenario the deleted comment called out), an inherited GIT_DIR/GIT_WORK_TREE can make the `add`/`commit` target the surrounding repository instead of the temp dir, and an inherited `commit.gpgsign`/hooks config can make the commit fail. That contradicts this PR's own "git env fixes" intent. Add `.env_remove(...)` for the GIT_* vars (or keep using `run_git`) on all three commands.</violation>
</file>
<file name="crates/mcb-providers/src/vector_store/edgevec/client.rs">
<violation number="1" location="crates/mcb-providers/src/vector_store/edgevec/client.rs:182">
P2: The new `Drop` calls `handle.join()`, which blocks whatever thread drops the provider until the actor drains all queued messages and exits. The provider is used exclusively through async trait methods on the Tokio runtime, so this join can run on an executor worker thread during runtime shutdown, stalling it. The dedicated actor thread was chosen specifically to keep `blocking_recv` off the runtime, but the blocking now just moves to the drop path. Consider an explicit `shutdown()` that the runtime caller invokes outside the executor (or before the runtime is torn down), and make `Drop` non-blocking for the fallback case.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
| git(&["config", "user.email", "test@example.com"])?; | ||
| git(&["add", "README.md"])?; | ||
| git(&["commit", "-qm", "init"])?; | ||
| let init = Command::new("git") |
There was a problem hiding this comment.
P2: The fixture previously routed through run_git, which removed inherited git env vars (GIT_DIR, GIT_WORK_TREE, GIT_INDEX_FILE, GIT_CONFIG, GIT_OBJECT_DIRECTORY, GIT_COMMON_DIR), set GIT_CONFIG_GLOBAL=/dev/null, and added -c core.hooksPath=/dev/null. The deleted comment stated this was required so the fixture is built against the temp dir and not the surrounding repository when the suite runs inside a git hook. The new raw Command::new("git") calls drop all of that sanitization, so in a git-hook environment GIT_DIR/GIT_WORK_TREE override the current_dir and these commands operate on the surrounding repo (or a commit hook from global config fails the commit), reintroducing the exact failure the helper was created to prevent. Reuse run_git(&repo_path, ...) as before, or clear the same env vars and config on each Command.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/mcb-server/tests/integration/handlers/vcs_tests.rs, line 38:
<comment>The fixture previously routed through `run_git`, which removed inherited git env vars (GIT_DIR, GIT_WORK_TREE, GIT_INDEX_FILE, GIT_CONFIG, GIT_OBJECT_DIRECTORY, GIT_COMMON_DIR), set GIT_CONFIG_GLOBAL=/dev/null, and added -c core.hooksPath=/dev/null. The deleted comment stated this was required so the fixture is built against the temp dir and not the surrounding repository when the suite runs inside a git hook. The new raw `Command::new("git")` calls drop all of that sanitization, so in a git-hook environment GIT_DIR/GIT_WORK_TREE override the current_dir and these commands operate on the surrounding repo (or a commit hook from global config fails the commit), reintroducing the exact failure the helper was created to prevent. Reuse `run_git(&repo_path, ...)` as before, or clear the same env vars and config on each Command.</comment>
<file context>
@@ -36,18 +35,43 @@ fn create_git_repo_fixture() -> Result<(tempfile::TempDir, String), std::io::Err
- git(&["config", "user.email", "test@example.com"])?;
- git(&["add", "README.md"])?;
- git(&["commit", "-qm", "init"])?;
+ let init = Command::new("git")
+ .args(["init", "-q"])
+ .current_dir(&repo_path)
</file context>
| run_git(&repo_path, &["init"])?; | ||
| run_git(&repo_path, &["config", "user.email", "test@example.com"])?; | ||
| run_git(&repo_path, &["config", "user.name", "Test User"])?; | ||
| let init = Command::new("git") |
There was a problem hiding this comment.
P2: Replacing run_git here drops the git environment sanitization that the removed comment explicitly relied on. run_git (crates/mcb-domain/src/utils/tests/git_helpers.rs) removes GIT_DIR/GIT_WORK_TREE/GIT_INDEX_FILE/GIT_CONFIG/GIT_OBJECT_DIRECTORY/GIT_COMMON_DIR and disables global/system config and hooksPath. These raw git add/git commit calls inherit the process environment unchanged, so when the test suite runs inside a git hook (the exact scenario the deleted comment called out), an inherited GIT_DIR/GIT_WORK_TREE can make the add/commit target the surrounding repository instead of the temp dir, and an inherited commit.gpgsign/hooks config can make the commit fail. That contradicts this PR's own "git env fixes" intent. Add .env_remove(...) for the GIT_* vars (or keep using run_git) on all three commands.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/mcb-server/tests/e2e/gap_fixes_e2e.rs, line 109:
<comment>Replacing `run_git` here drops the git environment sanitization that the removed comment explicitly relied on. `run_git` (crates/mcb-domain/src/utils/tests/git_helpers.rs) removes GIT_DIR/GIT_WORK_TREE/GIT_INDEX_FILE/GIT_CONFIG/GIT_OBJECT_DIRECTORY/GIT_COMMON_DIR and disables global/system config and hooksPath. These raw `git add`/`git commit` calls inherit the process environment unchanged, so when the test suite runs inside a git hook (the exact scenario the deleted comment called out), an inherited GIT_DIR/GIT_WORK_TREE can make the `add`/`commit` target the surrounding repository instead of the temp dir, and an inherited `commit.gpgsign`/hooks config can make the commit fail. That contradicts this PR's own "git env fixes" intent. Add `.env_remove(...)` for the GIT_* vars (or keep using `run_git`) on all three commands.</comment>
<file context>
@@ -106,16 +106,42 @@ async fn test_gap2_vcs_list_repositories_discovers_repos() -> TestResult {
- run_git(&repo_path, &["init"])?;
- run_git(&repo_path, &["config", "user.email", "test@example.com"])?;
- run_git(&repo_path, &["config", "user.name", "Test User"])?;
+ let init = Command::new("git")
+ .args(["init"])
+ .current_dir(&repo_path)
</file context>
| if let Some(handle) = self.actor_thread.take() { | ||
| let (closed, _) = mpsc::channel(1); | ||
| drop(std::mem::replace(&mut self.sender, closed)); | ||
| drop(handle.join()); |
There was a problem hiding this comment.
P2: The new Drop calls handle.join(), which blocks whatever thread drops the provider until the actor drains all queued messages and exits. The provider is used exclusively through async trait methods on the Tokio runtime, so this join can run on an executor worker thread during runtime shutdown, stalling it. The dedicated actor thread was chosen specifically to keep blocking_recv off the runtime, but the blocking now just moves to the drop path. Consider an explicit shutdown() that the runtime caller invokes outside the executor (or before the runtime is torn down), and make Drop non-blocking for the fallback case.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/mcb-providers/src/vector_store/edgevec/client.rs, line 182:
<comment>The new `Drop` calls `handle.join()`, which blocks whatever thread drops the provider until the actor drains all queued messages and exits. The provider is used exclusively through async trait methods on the Tokio runtime, so this join can run on an executor worker thread during runtime shutdown, stalling it. The dedicated actor thread was chosen specifically to keep `blocking_recv` off the runtime, but the blocking now just moves to the drop path. Consider an explicit `shutdown()` that the runtime caller invokes outside the executor (or before the runtime is torn down), and make `Drop` non-blocking for the fallback case.</comment>
<file context>
@@ -153,17 +147,39 @@ impl EdgeVecVectorStoreProvider {
+ if let Some(handle) = self.actor_thread.take() {
+ let (closed, _) = mpsc::channel(1);
+ drop(std::mem::replace(&mut self.sender, closed));
+ drop(handle.join());
+ }
+ }
</file context>
| /// `GIT_COMMON_DIR` ahead of `-C`, so a suite running inside a git operation | ||
| /// (pre-commit hook, rebase, merge) would otherwise stage into the surrounding | ||
| /// repository's index instead of the temporary one under test. | ||
| fn git_command() -> Command { |
There was a problem hiding this comment.
P3: This change drops the centralized run_git helper (still available in mcb_domain::utils::tests::git_helpers) and reimplements a partial copy of it as the local git_command(), which contradicts the helper module's stated purpose of centralizing git test isolation. The local copy only strips GIT_* env vars; it omits the GIT_CONFIG_NOSYSTEM=1, GIT_CONFIG_GLOBAL=/dev/null and core.hooksPath=/dev/null isolation that run_git applies, so the reimplemented path is strictly weaker. Prefer reusing run_git (e.g. run_git(root, &["init"]) and run_git(root, &["add", "src/lib.rs"])) so all test suites share the same robust isolation and the duplicate logic is removed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/mcb-validate/tests/unit/run_context_tests.rs, line 15:
<comment>This change drops the centralized `run_git` helper (still available in `mcb_domain::utils::tests::git_helpers`) and reimplements a partial copy of it as the local `git_command()`, which contradicts the helper module's stated purpose of centralizing git test isolation. The local copy only strips GIT_* env vars; it omits the `GIT_CONFIG_NOSYSTEM=1`, `GIT_CONFIG_GLOBAL=/dev/null` and `core.hooksPath=/dev/null` isolation that `run_git` applies, so the reimplemented path is strictly weaker. Prefer reusing `run_git` (e.g. `run_git(root, &["init"])` and `run_git(root, &["add", "src/lib.rs"])`) so all test suites share the same robust isolation and the duplicate logic is removed.</comment>
<file context>
@@ -1,11 +1,31 @@
+/// `GIT_COMMON_DIR` ahead of `-C`, so a suite running inside a git operation
+/// (pre-commit hook, rebase, merge) would otherwise stage into the surrounding
+/// repository's index instead of the temporary one under test.
+fn git_command() -> Command {
+ let mut command = Command::new("git");
+ for key in [
</file context>
2 participants
Promote PR #205 content to main: EdgeVec dedicated OS-thread actor with deterministic teardown, git env sanitization in validation inventory, git_helpers isolation coverage restored, VCS test fixtures assert status+stderr, clippy question_mark fix (mcb-jdwv, mcb-88o2). CI green on develop.
Summary by cubic
Promotes
developtomainwithEdgeVecmoved from a Tokio task to a dedicated OS thread and git environment sanitization restored in validation inventory and tests. This prevents executor starvation during HNSW operations and avoids shutdown hangs, while making git-related test setup failures visible.EdgeVecActor::runis now synchronous and usesblocking_recv;EdgeVecVectorStoreProviderspawns a named OS thread, retains itsJoinHandle, and onDropcloses the channel and joins the thread for deterministic teardown.GIT_*variables; unit tests add agit_commandhelper that removes these vars to ensure enumeration runs against the test repo.gitoutput and assert on exit status with stderr in failure messages to surface fixture setup issues.dependency_parsercollapses a parent walk to use?with no behavior change.Written for commit b9d5c7f. Summary will update on new commits.