Bead/mcb o96i 16 hook commit recovery v4 - #206
Open
marlonsc wants to merge 21 commits into
Open
Conversation
added 19 commits
July 28, 2026 13:38
git exports GIT_DIR/GIT_WORK_TREE to hook processes; test fixtures then commit into this repository instead of their temp repos (stray 'Initial commit' commits, mass deletions, README gutted to a test stub). Sanitize the git-hook environment before running the canonical gates. Validated: scripts/lib/tests/test-hooks.sh fixtures pass (primary + linked worktree install and commit gates).
- crossbeam-epoch 0.9.18 -> 0.9.20 (RUSTSEC-2026-0204) - serial_test 3.4 -> 4.0.0: drops scc entirely, removing RUSTSEC-2026-0205 without an msrv bump (4.0.1 requires rustc 1.93.1; workspace validates under 1.92 and CI stable alike) - git2 0.20 -> 0.21 (RUSTSEC-2026-0183/0184); getters now return Result<Option<&str>>, call sites updated (git.rs, submodule.rs) - quick-xml workspace pin 0.39 -> 0.41 for first-party crates - accept RUSTSEC-2026-0194/0195 in the audit ignore SSOT with evidence: vendored opendal pins quick-xml <0.41 and its XML services are disabled in this workspace, so the DoS paths are unreachable until the next opendal bump - box large error variants (CallToolResult, loco_rs::Error) across mcb-server handlers and mcb initializers (result_large_err) - auth: typed AuthError domain error + map_auth_error boundary mapping (replaces loco_rs::Result in mcb-server auth, ported from the v0.4.0 feature line) - mcb_bin: prefer workspace-built binary over PATH (mise shim broke validate in fresh worktrees) - contract snapshots refreshed for serde field ordering Validated: cargo check --workspace --all-targets 0 errors; make check WHAT=lint and WHAT=audit green; contract suite 27/27; nextest mcb-server+mcb-providers+mcb 756/756 on this content.
git exports GIT_DIR/GIT_WORK_TREE to hook processes; the gate's test fixtures then commit into this repository instead of their temp repos (observed as stray 'Initial commit' commits, mass file deletions staged from fixture cleanups, and README gutted to a test stub). Sanitize the git-hook environment before running the canonical gates.
…ommit # Conflicts: # scripts/hooks/pre-commit
…k-commit-recovery-v4
Recoverable checkpoint of in-flight work. Not a delivery; gates deferred. Origin branch: bead/mcb-o96i-16-hook-commit-recovery-v4
Contributor
|
ⓘ 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 available on request
Reviews should be triggered manually for repositories with fewer than 10 stars. Select Trigger review above or comment ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a distributable “workspace command” framework under scripts/ (intended for ~/.ai-hub-managed workspaces), improves Git hook installation to work correctly with Git worktrees, and updates parts of the Rust MCP server/provider code to accommodate dependency upgrades and boxed error handling—along with updating contract test snapshots.
Changes:
- Add a Python-based workspace command registry/dispatcher (
scripts/lib/workspace_command.py) with a default workspace SSOT (scripts/lib/workspace.py) and a thin entrypoint (scripts/dispatch.py). - Rework hook installation and add a “staged” check path (
mcb.sh,dispatch.mk, hooks + hook test) to support worktree-safe hook installation. - Upgrade Rust deps (
git2,quick-xml,serial_test) and adjust related call sites + error plumbing; refresh contract snapshots.
Reviewed changes
Copilot reviewed 57 out of 58 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/lib/workspace.py | Defines default workspace SSOT paths/env for the ai-hub command framework. |
| scripts/lib/workspace_command.py | Implements command discovery/registry + dispatch logic for workspace scripts. |
| scripts/lib/tests/test-hooks.sh | Adds an integration-style fixture test for hook installation across worktrees. |
| scripts/lib/mcb.sh | Adds hook install helpers, staged Rust checks, submodule sync, and tweaks guard behavior. |
| scripts/lib/init.py | Marks scripts/lib as a Python package for dispatcher imports. |
| scripts/hooks/pre-push | Adds a pre-push hook running the full make check WHAT=all gate. |
| scripts/hooks/pre-commit | Updates pre-commit hook to use the new staged gate wiring. |
| scripts/dispatch.py | Adds a thin Python entrypoint for the workspace command framework. |
| makefiles/dispatch.mk | Wires new check WHAT=staged, check WHAT=hooks, and check WHAT=guard cases; adjusts some git/submodule/setup routes. |
| Makefile | Extends WHATS_check to include staged/hooks/guard. |
| crates/mcb/src/initializers/mcp_server.rs | Boxes Loco errors through bootstrap/config resolution and maps back at the boundary. |
| crates/mcb-server/tests/contract/snapshots/contract__vcs_contract_tests__vcs_invalid_args.snap | Snapshot reordering/format updates for VCS invalid-args contract test. |
| crates/mcb-server/tests/contract/snapshots/contract__vcs_contract_tests__vcs_happy_path.snap | Snapshot reordering/format updates for VCS happy-path contract test. |
| crates/mcb-server/tests/contract/snapshots/contract__validate_contract_tests__validate_invalid_args.snap | Snapshot reordering/format updates for validate invalid-args contract test. |
| crates/mcb-server/tests/contract/snapshots/contract__validate_contract_tests__validate_happy_path.snap | Snapshot reordering/format updates for validate happy-path contract test. |
| crates/mcb-server/tests/contract/snapshots/contract__session_contract_tests__session_invalid_args.snap | Snapshot reordering/format updates for session invalid-args contract test. |
| crates/mcb-server/tests/contract/snapshots/contract__session_contract_tests__session_happy_path.snap | Snapshot reordering/format updates for session happy-path contract test. |
| crates/mcb-server/tests/contract/snapshots/contract__search_contract_tests__search_invalid_args.snap | Snapshot reordering/format updates for search invalid-args contract test. |
| crates/mcb-server/tests/contract/snapshots/contract__search_contract_tests__search_happy_path.snap | Snapshot reordering/format updates for search happy-path contract test. |
| crates/mcb-server/tests/contract/snapshots/contract__project_contract_tests__project_invalid_args.snap | Snapshot reordering/format updates for project invalid-args contract test. |
| crates/mcb-server/tests/contract/snapshots/contract__project_contract_tests__project_happy_path.snap | Snapshot reordering/format updates for project happy-path contract test. |
| crates/mcb-server/tests/contract/snapshots/contract__memory_contract_tests__memory_invalid_args.snap | Snapshot reordering/format updates for memory invalid-args contract test. |
| crates/mcb-server/tests/contract/snapshots/contract__memory_contract_tests__memory_happy_path.snap | Snapshot reordering/format updates for memory happy-path contract test. |
| crates/mcb-server/tests/contract/snapshots/contract__memory_contract_tests__memory_get_observation_ids_none.snap | Snapshot reordering/format updates for get_memories (none) contract test. |
| crates/mcb-server/tests/contract/snapshots/contract__memory_contract_tests__memory_get_observation_ids_empty.snap | Snapshot reordering/format updates for get_memories (empty) contract test. |
| crates/mcb-server/tests/contract/snapshots/contract__index_contract_tests__index_invalid_args.snap | Snapshot reordering/format updates for index invalid-args contract test. |
| crates/mcb-server/tests/contract/snapshots/contract__index_contract_tests__index_happy_path.snap | Snapshot reordering/format updates for index happy-path contract test. |
| crates/mcb-server/tests/contract/snapshots/contract__entity_contract_tests__entity_invalid_args.snap | Snapshot reordering/format updates for entity invalid-args contract test. |
| crates/mcb-server/tests/contract/snapshots/contract__entity_contract_tests__entity_happy_path.snap | Snapshot reordering/format updates for entity happy-path contract test. |
| crates/mcb-server/tests/contract/snapshots/contract__agent_contract_tests__agent_store_without_session.snap | Snapshot reordering/format updates for agent store-without-session contract test. |
| crates/mcb-server/tests/contract/snapshots/contract__agent_contract_tests__agent_invalid_args.snap | Snapshot reordering/format updates for agent invalid-args contract test. |
| crates/mcb-server/tests/contract/snapshots/contract__agent_contract_tests__agent_happy_path.snap | Snapshot reordering/format updates for agent happy-path contract test. |
| crates/mcb-server/src/utils/mcp/helpers.rs | Boxes tool-level error returns from MCP helper (require_data_map). |
| crates/mcb-server/src/utils/mcp/fields.rs | Boxes tool-level error returns from MCP JSON field helpers. |
| crates/mcb-server/src/macros/handlers.rs | Updates helper macro docs/behavior for boxed tool errors. |
| crates/mcb-server/src/handlers/vcs/search_branch.rs | Adapts query validation to boxed tool errors. |
| crates/mcb-server/src/handlers/vcs/responses.rs | Adapts repo-path resolution to boxed tool errors. |
| crates/mcb-server/src/handlers/vcs/index_repo.rs | Updates tool-error propagation for boxed tool errors. |
| crates/mcb-server/src/handlers/vcs/compare_branches.rs | Updates tool-error propagation for boxed tool errors. |
| crates/mcb-server/src/handlers/vcs/analyze_impact.rs | Updates tool-error propagation for boxed tool errors. |
| crates/mcb-server/src/handlers/session/update.rs | Updates tool-error propagation for boxed tool errors. |
| crates/mcb-server/src/handlers/session/get.rs | Updates tool-error propagation for boxed tool errors. |
| crates/mcb-server/src/handlers/session/create.rs | Updates tool-error propagation for boxed tool errors. |
| crates/mcb-server/src/handlers/session/common.rs | Updates session-id helpers to boxed tool errors. |
| crates/mcb-server/src/handlers/search.rs | Updates collection resolution errors to boxed tool errors. |
| crates/mcb-server/src/handlers/memory/list_timeline.rs | Updates timeline anchor resolution to boxed tool errors. |
| crates/mcb-server/src/handlers/memory/execution.rs | Updates execution payload validation to boxed tool errors. |
| crates/mcb-server/src/controllers/web/config.rs | Adapts config loading to boxed Loco errors. |
| crates/mcb-server/src/controllers/admin.rs | Adds auth error mapping at controller boundary; adapts config loading to boxed errors. |
| crates/mcb-server/src/controllers/admin_config.rs | Changes admin-config loader to return boxed Loco errors. |
| crates/mcb-server/src/auth.rs | Introduces AuthError and switches auth flows off Loco errors to domain-ish auth errors. |
| crates/mcb-providers/src/vcs/submodule.rs | Adapts to updated git2 submodule URL API returning a Result. |
| crates/mcb-providers/src/vcs/git.rs | Adapts to updated git2 APIs and tightens #[allow] justification style. |
| Cargo.toml | Bumps serial_test, quick-xml, and git2. |
| Cargo.lock | Lockfile refresh for dependency upgrades. |
| AGENTS.md | Replaces embedded universal law with pointers + scope-local instructions block. |
| .mise.toml | Adds mise toolchain configuration (python/native tools/beads). |
| .code-review-graphignore | Adds CRG global ignore policy file. |
Suppressed comments (1)
scripts/lib/workspace_command.py:207
dispatch()currently returns success forWHAT=all/help,OPTIONS/HELP, and dry-run mutations without printing any help/dry-run output. The module already hasrender_verb_help,render_command_help, andrender_dry_run, so these branches should print the corresponding text.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
9
to
11
| echo "→ guard (staged)…"; bash scripts/lib/mcb.sh guard --staged | ||
| echo "→ lint…"; make check WHAT=lint | ||
| echo "→ validate (quick)…"; make check WHAT=validate QUICK=1 | ||
| echo "→ staged check…"; make check WHAT=staged | ||
| echo "✓ pre-commit passed" |
Comment on lines
+67
to
+70
| test "$(grep -c 'check WHAT=lint' "$MAKE_LOG")" -eq 2 | ||
| test "$(grep -c 'check WHAT=validate QUICK=1' "$MAKE_LOG")" -eq 2 | ||
| grep -q "^$PRIMARY|check WHAT=lint$" "$MAKE_LOG" | ||
| grep -q "^$LINKED|check WHAT=lint$" "$MAKE_LOG" |
Comment on lines
+102
to
+106
| # Binary lookup chain: PATH > target/release > target/debug > cargo run | ||
| mcb_bin() { | ||
| command -v mcb 2>/dev/null && return 0 | ||
| [ -x "$MCB_ROOT/target/release/mcb" ] && { echo "$MCB_ROOT/target/release/mcb"; return 0; } | ||
| [ -x "$MCB_ROOT/target/debug/mcb" ] && { echo "$MCB_ROOT/target/debug/mcb"; return 0; } | ||
| [ -x "$MCB_ROOT/target/release/mcb" ] && { echo "$MCB_ROOT/target/release/mcb"; return 0; } | ||
| command -v mcb 2>/dev/null && return 0 |
Comment on lines
+142
to
+163
| if printf '%s\n' "$staged" | grep -qE '^Cargo\.(toml|lock)$'; then | ||
| packages="--workspace" | ||
| else | ||
| while IFS= read -r path; do | ||
| case "$path" in | ||
| crates/*/src/*.rs) | ||
| crate_dir="$(printf '%s\n' "$path" | cut -d/ -f1-2)" | ||
| manifest="$crate_dir/Cargo.toml" | ||
| [ -f "$MCB_ROOT/$manifest" ] || continue | ||
| package="$(sed -n '/^\[package\]/,/^\[/s/^name = "\([^"]*\)"/\1/p' "$MCB_ROOT/$manifest" | head -1)" | ||
| [ -n "$package" ] || mcb_die "$EX_PREREQ" "package name ausente em '$manifest'" | ||
| case " $packages " in *" -p $package "*) ;; *) packages="$packages -p $package" ;; esac | ||
| ;; | ||
| esac | ||
| done <<< "$staged" | ||
| fi | ||
|
|
||
| [ -n "$packages" ] || { mcb_ok "staged check: no Rust package affected"; return 0; } | ||
| mcb_log "staged check: cargo fmt/clippy scope:$packages (deadline ${deadline}s each)" | ||
| timeout --signal=TERM --kill-after=5s "${deadline}s" cargo fmt $packages -- --check | ||
| timeout --signal=TERM --kill-after=5s "${deadline}s" cargo clippy $packages --all-targets -- -D warnings | ||
| mcb_ok "staged check: clean" |
Comment on lines
+192
to
+194
| else | ||
| hits=$(git -C "$MCB_ROOT" diff origin/main...HEAD --unified=0 -- crates 2>/dev/null \ | ||
| | grep -E '^\+[^+].*#\[allow\(' | grep -vE '#\[allow\([^]]+\)\][[:space:]]*//[[:space:]]*[^[:space:]]' || true) |
Comment on lines
+177
to
+190
| def main(argv: Sequence[str] | None = None) -> int: | ||
| args = tuple(sys.argv[1:] if argv is None else argv) | ||
| try: | ||
| ensure_local_python() | ||
| registry = discover() | ||
| if args and args[0] == "--validate": | ||
| return 0 | ||
| if not args or args[0] in {"help", "--help", "-h"}: | ||
| os.environ.get("WHAT", "").strip() | ||
| return 0 | ||
| return dispatch(registry, args[0]) | ||
| except RegistryError: | ||
| return 2 | ||
|
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 107779a85b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| echo "→ guard (staged)…"; bash scripts/lib/mcb.sh guard --staged | ||
| echo "→ lint…"; make check WHAT=lint | ||
| echo "→ validate (quick)…"; make check WHAT=validate QUICK=1 | ||
| echo "→ staged check…"; make check WHAT=staged |
There was a problem hiding this comment.
For commits containing only configuration, scripts, manifests below the workspace root, or other non-src/*.rs files, check WHAT=staged reports that no Rust package is affected and succeeds without running either lint or validation; even detected Rust changes never run architecture validation. This contradicts the repository's required pre-commit sequence and also makes the newly added make check WHAT=hooks test fail because it still verifies both commands. Restore check WHAT=lint and check WHAT=validate QUICK=1, or make the staged gate provide equivalent coverage.
AGENTS.md reference: AGENTS.md:L58-L63
Useful? React with 👍 / 👎.
Comment on lines
104
to
+106
| [ -x "$MCB_ROOT/target/debug/mcb" ] && { echo "$MCB_ROOT/target/debug/mcb"; return 0; } | ||
| [ -x "$MCB_ROOT/target/release/mcb" ] && { echo "$MCB_ROOT/target/release/mcb"; return 0; } | ||
| command -v mcb 2>/dev/null && return 0 |
There was a problem hiding this comment.
When target/debug/mcb remains from an older build, this branch selects it ahead of both a newly built release binary and the configured mcb on PATH. Consequently, make check WHAT=validate can run obsolete validation rules and falsely pass or fail the current tree; this is especially easy to trigger after switching branches or running make build, whose default is release mode. Preserve the documented lookup order so the architecture gate uses the intended validator.
AGENTS.md reference: AGENTS.md:L65-L67
Useful? React with 👍 / 👎.
| set -euo pipefail | ||
| cd "$(git rev-parse --show-toplevel)" | ||
| [ -f scripts/lib/mcb.sh ] || exit 0 | ||
| make check WHAT=all |
There was a problem hiding this comment.
When this runs as an actual pre-push hook, Git exports repository-local variables such as GIT_DIR, GIT_WORK_TREE, and GIT_INDEX_FILE to the hook process. make check WHAT=all inherits them and runs tests such as crates/mcb-server/tests/integration/handlers/vcs_tests.rs:38-50 that invoke Git inside temporary repositories; those nested commands can instead operate on the repository being pushed, causing the gate to fail or stage/commit fixture data in the real worktree. Unset Git's local repository variables before launching the full gate.
Useful? React with 👍 / 👎.
Comment on lines
+193
to
+194
| hits=$(git -C "$MCB_ROOT" diff origin/main...HEAD --unified=0 -- crates 2>/dev/null \ | ||
| | grep -E '^\+[^+].*#\[allow\(' | grep -vE '#\[allow\([^]]+\)\][[:space:]]*//[[:space:]]*[^[:space:]]' || true) |
There was a problem hiding this comment.
Outside staged mode, the suppression check now examines only additions in origin/main...HEAD rather than the full source tree. It therefore misses every unjustified #[allow] already present on the branch, and if the checkout has no origin/main ref the redirected Git error produces an empty pipeline and the guard silently succeeds. Keep the diff-based behavior for guard --staged, but make the advertised full-tree guard inspect src as it did previously.
AGENTS.md reference: AGENTS.md:L61-L63
Useful? React with 👍 / 👎.
Comment on lines
+184
to
+186
| if not args or args[0] in {"help", "--help", "-h"}: | ||
| os.environ.get("WHAT", "").strip() | ||
| return 0 |
There was a problem hiding this comment.
With a valid command registry, every informational path returns success without writing anything: the help branch performs only a discarded environment lookup, WHAT=all/WHAT=help and HELP=Y return immediately, and mutating commands in dry-run mode also return silently. The file defines render_requested_help and render_dry_run, but neither is called anywhere, so users cannot inspect available commands or the mutation they are being asked to approve. Print the appropriate renderer output before returning.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
11 issues found across 58 files
Confidence score: 2/5
scripts/lib/mcb.shcan loop indefinitely inmcb_sync_submodulesunder dash becauseread -dis not supported, leaving populated submodules unable to complete synchronization — replace it with POSIX-compatible input handling.scripts/lib/mcb.shvalidates the worktree instead of the staged snapshot when staged and unstaged edits differ, allowing broken staged code to pass or unrelated edits to cause false failures — make staged checks read exclusively from the index.scripts/lib/tests/test-hooks.shalways fails because its expectedmake checklog does not match the installed pre-commit hook’s actual command — align the assertion with the hook or update the hook behavior.scripts/lib/mcb.shhas additional guard execution gaps: manifest-only changes can skip checks, and passing--workspacetocargo fmterrors out; update package path selection and use rustfmt-supported arguments before relying on these checks.
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="scripts/lib/mcb.sh">
<violation number="1" location="scripts/lib/mcb.sh:81">
P1: When `/bin/sh` is dash, `git submodule foreach` cannot execute `read -d`, so `present` stays empty and `mcb_sync_submodules` loops until killed for every populated submodule. Use POSIX-compatible input handling or explicitly invoke Bash for this probe.</violation>
<violation number="2" location="scripts/lib/mcb.sh:105">
P2: mcb_bin's lookup order now contradicts both its documented chain and the prior behavior: a target/debug build is returned before a release build, and an installed `mcb` on PATH is only considered after both target/ directories. A stale debug artifact in `target/` now shadows the user's installed binary, so `mcb_validate`/`mcb cleanup`-style resolution can run an outdated or slower debug build. Reorder to PATH > release > debug, or update the comment to match the new intent.</violation>
<violation number="3" location="scripts/lib/mcb.sh:143">
P2: `cargo fmt` does not accept `--workspace` — only `--all` is supported (cargo clippy accepts `--workspace`, but rustfmt never adopted it). Passing `--workspace` to `cargo fmt $packages -- --check` here will error out whenever Cargo.toml/Cargo.lock is staged. Also, the staged-path glob `crates/*/src/*.rs` only matches files directly under `src/`, so nested modules like `crates/foo/src/bar/baz.rs` won't trigger the staged fmt/clippy check.</violation>
<violation number="4" location="scripts/lib/mcb.sh:146">
P2: When a staged change is only a crate manifest or non-`src` Rust target, this selector reports no affected package and skips both staged checks. Match all paths under `crates/<package>/`, or explicitly include manifests and every Cargo target.</violation>
<violation number="5" location="scripts/lib/mcb.sh:161">
P1: When staged and unstaged edits differ, these commands validate the worktree rather than the staged snapshot. An unstaged fix can hide broken staged code, while unrelated unstaged code can create a false failure; check an index snapshot or reject unstaged edits before running the gate.</violation>
<violation number="6" location="scripts/lib/mcb.sh:193">
P2: In non-staged mode, `mcb_guard` runs `git diff origin/main...HEAD` with `2>/dev/null` and effectively `|| true` via the trailing grep fallback. If `origin/main` doesn't exist (fresh clones, shallow CI checkouts), the diff fails silently and `hits` stays empty, so the check passes even when new unjustified `#[allow(...)]` directives were added. Fail loud (or fall back to a known ref) instead of swallowing the git error.</violation>
</file>
<file name=".code-review-graphignore">
<violation number="1" location=".code-review-graphignore:2">
P3: This ignore list excludes `build/**` but omits `target/`, which is where this Rust workspace writes all build artifacts (e.g. `target/release/$(BINARY_NAME)` per `makefiles/dispatch.mk`, and `third-party/sea-orm/.../target`). The repo's own `.gitignore` lists `target/` as its first artifact rule, so the code-review graph will still ingest compiled Rust output that this GLOBAL POLICY was meant to skip. Add `target/` (and `**/target/` for nested workspace/third-party builds).</violation>
</file>
<file name="scripts/lib/tests/test-hooks.sh">
<violation number="1" location="scripts/lib/tests/test-hooks.sh:24">
P3: The NON_MCB repo is initialized and given an initial commit but never referenced again, so it is dead test setup. Remove these lines unless a later assertion is intended to exercise a non-MCB worktree.</violation>
<violation number="2" location="scripts/lib/tests/test-hooks.sh:67">
P2: This test always fails. The installed pre-commit hook (copied from scripts/hooks/pre-commit) runs `make check WHAT=staged` and `scripts/lib/mcb.sh guard --staged`, but the test asserts the make log contains `check WHAT=lint` and `check WHAT=validate QUICK=1`. The make stub logs args verbatim, so the log only ever contains `check WHAT=staged`, `grep -c` returns 0, and `test 0 -eq 2` exits non-zero (verified: script exits 1). Update the assertions to match the hook: expect two `check WHAT=staged` entries (one per worktree) and drop the validate/lint checks.</violation>
</file>
<file name="crates/mcb-server/src/controllers/admin_config.rs">
<violation number="1" location="crates/mcb-server/src/controllers/admin_config.rs:14">
P3: The `Box<loco_rs::Error>` returned by `load_admin_config()` (and the corresponding helpers in mcp_server.rs) is redundant: every caller immediately dereferences it with `.map_err(|e| *e)?`. Since all boxes wrap `loco_rs::Error` and the controllers use Loco's `Result<T, loco_rs::Error>`, no `From`/trait-object bound requires boxing. Return `loco_rs::Error` directly, drop the `Box::new(...)` wrappers, and use plain `?` at the call sites.</violation>
</file>
<file name="AGENTS.md">
<violation number="1" location="AGENTS.md:4">
P2: This rewrite replaces the repository's self-contained engineering rules with pointers to files that do not exist anywhere in this repo: `../AGENTS.md` (resolves outside the repository tree) and `~/.agents/UNIVERSAL_CORE.md` (personal, non-versioned configuration). Any agent or contributor working from a standalone checkout of this repository gets an AGENTS.md with no actual rules — the binding core (root-cause-only, fix-forward, no workarounds/suppressions, user-manages-git, blocked-operation protocol) now lives only in files that will be absent for everyone but the author's own machine. The 'Standalone / independent mode' note mitigates nothing because it still does not embed the law; it only tells the user to manually pin an external URL. Consider retaining the essential, self-contained rules inline (or at least the core that must hold for any agent regardless of environment) so the canonical governance survives without the parent/global files.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
|
|
||
| [ -n "$packages" ] || { mcb_ok "staged check: no Rust package affected"; return 0; } | ||
| mcb_log "staged check: cargo fmt/clippy scope:$packages (deadline ${deadline}s each)" | ||
| timeout --signal=TERM --kill-after=5s "${deadline}s" cargo fmt $packages -- --check |
Contributor
There was a problem hiding this comment.
P1: When staged and unstaged edits differ, these commands validate the worktree rather than the staged snapshot. An unstaged fix can hide broken staged code, while unrelated unstaged code can create a false failure; check an index snapshot or reject unstaged edits before running the gate.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/lib/mcb.sh, line 161:
<comment>When staged and unstaged edits differ, these commands validate the worktree rather than the staged snapshot. An unstaged fix can hide broken staged code, while unrelated unstaged code can create a false failure; check an index snapshot or reject unstaged edits before running the gate.</comment>
<file context>
@@ -77,6 +131,38 @@ mcb_validate() { # $1 = "quick" | "full"
+
+ [ -n "$packages" ] || { mcb_ok "staged check: no Rust package affected"; return 0; }
+ mcb_log "staged check: cargo fmt/clippy scope:$packages (deadline ${deadline}s each)"
+ timeout --signal=TERM --kill-after=5s "${deadline}s" cargo fmt $packages -- --check
+ timeout --signal=TERM --kill-after=5s "${deadline}s" cargo clippy $packages --all-targets -- -D warnings
+ mcb_ok "staged check: clean"
</file context>
|
|
||
| while :; do | ||
| materialized="$(git -C "$repo" submodule foreach --quiet --recursive ' | ||
| present="$(git ls-files -z | while IFS= read -r -d "" path; do |
Contributor
There was a problem hiding this comment.
P1: When /bin/sh is dash, git submodule foreach cannot execute read -d, so present stays empty and mcb_sync_submodules loops until killed for every populated submodule. Use POSIX-compatible input handling or explicitly invoke Bash for this probe.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/lib/mcb.sh, line 81:
<comment>When `/bin/sh` is dash, `git submodule foreach` cannot execute `read -d`, so `present` stays empty and `mcb_sync_submodules` loops until killed for every populated submodule. Use POSIX-compatible input handling or explicitly invoke Bash for this probe.</comment>
<file context>
@@ -51,17 +51,71 @@ mcb_retry() { local n="$1" s="$2"; shift 2; local t=1; while ! "$@"; do [ "$t" -
+
+ while :; do
+ materialized="$(git -C "$repo" submodule foreach --quiet --recursive '
+ present="$(git ls-files -z | while IFS= read -r -d "" path; do
+ if [ -e "$path" ] || [ -L "$path" ]; then
+ printf 1
</file context>
Suggested change
| present="$(git ls-files -z | while IFS= read -r -d "" path; do | |
| present="$(git ls-files | while IFS= read -r path; do |
| command -v mcb 2>/dev/null && return 0 | ||
| [ -x "$MCB_ROOT/target/release/mcb" ] && { echo "$MCB_ROOT/target/release/mcb"; return 0; } | ||
| [ -x "$MCB_ROOT/target/debug/mcb" ] && { echo "$MCB_ROOT/target/debug/mcb"; return 0; } | ||
| [ -x "$MCB_ROOT/target/release/mcb" ] && { echo "$MCB_ROOT/target/release/mcb"; return 0; } |
Contributor
There was a problem hiding this comment.
P2: mcb_bin's lookup order now contradicts both its documented chain and the prior behavior: a target/debug build is returned before a release build, and an installed mcb on PATH is only considered after both target/ directories. A stale debug artifact in target/ now shadows the user's installed binary, so mcb_validate/mcb cleanup-style resolution can run an outdated or slower debug build. Reorder to PATH > release > debug, or update the comment to match the new intent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/lib/mcb.sh, line 105:
<comment>mcb_bin's lookup order now contradicts both its documented chain and the prior behavior: a target/debug build is returned before a release build, and an installed `mcb` on PATH is only considered after both target/ directories. A stale debug artifact in `target/` now shadows the user's installed binary, so `mcb_validate`/`mcb cleanup`-style resolution can run an outdated or slower debug build. Reorder to PATH > release > debug, or update the comment to match the new intent.</comment>
<file context>
@@ -51,17 +51,71 @@ mcb_retry() { local n="$1" s="$2"; shift 2; local t=1; while ! "$@"; do [ "$t" -
- command -v mcb 2>/dev/null && return 0
- [ -x "$MCB_ROOT/target/release/mcb" ] && { echo "$MCB_ROOT/target/release/mcb"; return 0; }
[ -x "$MCB_ROOT/target/debug/mcb" ] && { echo "$MCB_ROOT/target/debug/mcb"; return 0; }
+ [ -x "$MCB_ROOT/target/release/mcb" ] && { echo "$MCB_ROOT/target/release/mcb"; return 0; }
+ command -v mcb 2>/dev/null && return 0
echo "cargo run --package mcb --"
</file context>
| @@ -0,0 +1,31 @@ | |||
| # BEGIN AI-HUB CRG GLOBAL POLICY | |||
| build/** | |||
Contributor
There was a problem hiding this comment.
P3: This ignore list excludes build/** but omits target/, which is where this Rust workspace writes all build artifacts (e.g. target/release/$(BINARY_NAME) per makefiles/dispatch.mk, and third-party/sea-orm/.../target). The repo's own .gitignore lists target/ as its first artifact rule, so the code-review graph will still ingest compiled Rust output that this GLOBAL POLICY was meant to skip. Add target/ (and **/target/ for nested workspace/third-party builds).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .code-review-graphignore, line 2:
<comment>This ignore list excludes `build/**` but omits `target/`, which is where this Rust workspace writes all build artifacts (e.g. `target/release/$(BINARY_NAME)` per `makefiles/dispatch.mk`, and `third-party/sea-orm/.../target`). The repo's own `.gitignore` lists `target/` as its first artifact rule, so the code-review graph will still ingest compiled Rust output that this GLOBAL POLICY was meant to skip. Add `target/` (and `**/target/` for nested workspace/third-party builds).</comment>
<file context>
@@ -0,0 +1,31 @@
+# BEGIN AI-HUB CRG GLOBAL POLICY
+build/**
+.worktrees/**
+worktrees/**
</file context>
| git -C "$PRIMARY" commit -q -m initial | ||
| git -C "$PRIMARY" worktree add -q -b linked-test "$LINKED" | ||
|
|
||
| NON_MCB="$FIXTURE/non-mcb" |
Contributor
There was a problem hiding this comment.
P3: The NON_MCB repo is initialized and given an initial commit but never referenced again, so it is dead test setup. Remove these lines unless a later assertion is intended to exercise a non-MCB worktree.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/lib/tests/test-hooks.sh, line 24:
<comment>The NON_MCB repo is initialized and given an initial commit but never referenced again, so it is dead test setup. Remove these lines unless a later assertion is intended to exercise a non-MCB worktree.</comment>
<file context>
@@ -0,0 +1,72 @@
+git -C "$PRIMARY" commit -q -m initial
+git -C "$PRIMARY" worktree add -q -b linked-test "$LINKED"
+
+NON_MCB="$FIXTURE/non-mcb"
+git init -q "$NON_MCB"
+git -C "$NON_MCB" config user.email test@example.com
</file context>
| /// | ||
| /// Fails when config cannot be loaded or serialized to JSON. | ||
| pub fn load_admin_config() -> Result<serde_json::Value> { | ||
| pub fn load_admin_config() -> Result<serde_json::Value, Box<loco_rs::Error>> { |
Contributor
There was a problem hiding this comment.
P3: The Box<loco_rs::Error> returned by load_admin_config() (and the corresponding helpers in mcp_server.rs) is redundant: every caller immediately dereferences it with .map_err(|e| *e)?. Since all boxes wrap loco_rs::Error and the controllers use Loco's Result<T, loco_rs::Error>, no From/trait-object bound requires boxing. Return loco_rs::Error directly, drop the Box::new(...) wrappers, and use plain ? at the call sites.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/mcb-server/src/controllers/admin_config.rs, line 14:
<comment>The `Box<loco_rs::Error>` returned by `load_admin_config()` (and the corresponding helpers in mcp_server.rs) is redundant: every caller immediately dereferences it with `.map_err(|e| *e)?`. Since all boxes wrap `loco_rs::Error` and the controllers use Loco's `Result<T, loco_rs::Error>`, no `From`/trait-object bound requires boxing. Return `loco_rs::Error` directly, drop the `Box::new(...)` wrappers, and use plain `?` at the call sites.</comment>
<file context>
@@ -11,11 +11,11 @@ const DEFAULT_PRO_ADMIN_CONFIG_DIR: &str = "config/pro_admin";
///
/// Fails when config cannot be loaded or serialized to JSON.
-pub fn load_admin_config() -> Result<serde_json::Value> {
+pub fn load_admin_config() -> Result<serde_json::Value, Box<loco_rs::Error>> {
let config_root = std::env::var("MCB_PRO_ADMIN_CONFIG_DIR")
.unwrap_or_else(|_| DEFAULT_PRO_ADMIN_CONFIG_DIR.to_owned());
</file context>
…conflicts) # Conflicts: # .mise.toml # AGENTS.md # Cargo.lock # Makefile # crates/mcb-providers/src/vcs/git.rs # crates/mcb-providers/src/vcs/submodule.rs # crates/mcb/src/initializers/mcp_server.rs # makefiles/dispatch.mk # scripts/hooks/pre-commit # scripts/lib/mcb.sh
The auto-context project_id discovery only parsed git@host:owner/repo and https:// remotes. SSH config alias remotes (github-neptor:owner/repo.git) failed to parse, leaving project_id unresolved and breaking every provenance-gated tool (start_session, entity list plans) with 'project_id is required'. Root-cause fix: parse_ssh_remote now accepts any SCP-style host:owner/repo remote, with regression coverage for canonical, alias, https, and negative forms. Also deduplicates map_auth_error introduced by the develop merge.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Confidence score: 5/5
- In
crates/mcb-server/src/tools/defaults.rs,parse_ssh_remotehas identicalmap_or_elsebranches after strippinggit@, which adds redundant control flow and makes the intended parsing behavior harder to verify; simplify the branch or add focused tests for both prefixed and unprefixed remotes.
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-server/src/tools/defaults.rs">
<violation number="1" location="crates/mcb-server/src/tools/defaults.rs:261">
P3: The rewritten parse_ssh_remote uses `strip_prefix("git@")` with `map_or_else`, but both branches are identical (`url.split_once(':')` vs `s.split_once(':')`, each taking only the part after ':'). Since the "git@" prefix sits before the colon and is always discarded, `strip_prefix` has no effect and the branch is dead logic. Simplify to a single `url.split_once(':')` call.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
Comment on lines
+261
to
+264
| let path = url.strip_prefix("git@").map_or_else( | ||
| || url.split_once(':').map(|(_, p)| p), | ||
| |s| s.split_once(':').map(|(_, p)| p), | ||
| )?; |
Contributor
There was a problem hiding this comment.
P3: The rewritten parse_ssh_remote uses strip_prefix("git@") with map_or_else, but both branches are identical (url.split_once(':') vs s.split_once(':'), each taking only the part after ':'). Since the "git@" prefix sits before the colon and is always discarded, strip_prefix has no effect and the branch is dead logic. Simplify to a single url.split_once(':') call.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/mcb-server/src/tools/defaults.rs, line 261:
<comment>The rewritten parse_ssh_remote uses `strip_prefix("git@")` with `map_or_else`, but both branches are identical (`url.split_once(':')` vs `s.split_once(':')`, each taking only the part after ':'). Since the "git@" prefix sits before the colon and is always discarded, `strip_prefix` has no effect and the branch is dead logic. Simplify to a single `url.split_once(':')` call.</comment>
<file context>
@@ -244,18 +244,24 @@ fn extract_org_and_project_from_git_remote(workspace_root: &str) -> Option<(Stri
- let path = url
- .strip_prefix("git@")
- .and_then(|s| s.split_once(':').map(|(_, p)| p))?;
+ let path = url.strip_prefix("git@").map_or_else(
+ || url.split_once(':').map(|(_, p)| p),
+ |s| s.split_once(':').map(|(_, p)| p),
</file context>
Suggested change
| let path = url.strip_prefix("git@").map_or_else( | |
| || url.split_once(':').map(|(_, p)| p), | |
| |s| s.split_once(':').map(|(_, p)| p), | |
| )?; | |
| let path = url.split_once(':').map(|(_, p)| p)?; |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated 3 comments.
Suppressed comments (2)
scripts/lib/workspace_command.py:186
- The help path does not render any output: the value from
os.environ.get("WHAT")is discarded and the function returns 0, soscripts/dispatch.py help/--helpproduces an empty response even though help-rendering functions exist below.
if not args or args[0] in {"help", "--help", "-h"}:
os.environ.get("WHAT", "").strip()
return 0
scripts/lib/workspace_command.py:189
RegistryErroris swallowed (exit code only) which makesmake/hook failures opaque. Printing the exception message to stderr keeps the failure actionable.
except RegistryError:
return 2
Comment on lines
+198
to
+207
| if what in {"all", "help"}: | ||
| return 0 | ||
| command = registry.command(verb, what) | ||
| if env_enabled("HELP") or env_enabled("OPTIONS"): | ||
| return 0 | ||
| is_dry_run = command.mutates and os.environ.get("APPLY", "N") != "Y" | ||
| validate_invocation(command, require_required=not is_dry_run) | ||
| if is_dry_run: | ||
| return 0 | ||
| return run(command) |
Comment on lines
+86
to
+89
| mkdir -p "$hooks_dir" | ||
| cp "$MCB_ROOT/scripts/hooks/pre-commit" "$MCB_ROOT/scripts/hooks/pre-push" "$hooks_dir/" | ||
| chmod +x "$hooks_dir/pre-commit" "$hooks_dir/pre-push" | ||
| mcb_ok "pre-commit and pre-push hooks installed at $hooks_dir" |
Comment on lines
+16
to
+18
| mkdir -p "$PRIMARY/scripts/hooks" "$PRIMARY/scripts/lib" | ||
| cp "$ROOT/scripts/hooks/pre-commit" "$PRIMARY/scripts/hooks/pre-commit" | ||
| cp "$ROOT/scripts/hooks/pre-push" "$PRIMARY/scripts/hooks/pre-push" |
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
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.
Summary by cubic
Hardened commit/push gates, added a workspace command framework, and cleared audits while adapting to
git20.21. Auto-context now parses SSH-alias remotes (old: onlygit@host:/https://parsed; new: acceptsalias:owner/repo.git), restoringproject_iddiscovery for provenance-gated tools.Hooks and tooling
pre-commit: runs staged guard plus staged-only fmt/clippy; sanitizes leakedGIT_*and works in linked worktrees.pre-push: runsmake check WHAT=alland forwards to Beads when present.scripts/lib/mcb.sh: addsgit-hooks-dir,install-hooks,check-staged, andsync-submodulesto repair empty initialized worktrees.scripts/lib/workspace_command.py) withscripts/dispatch.py;AGENTS.mddocuments canonical verbs and rules.Code and deps
parse_org_and_project_from_remote_urlaccepts SCP-style SSH aliases; tests added.git20.21,quick-xml0.41,serial_test4.0.x; adapt to newgit2getters.AuthErrorat the controller.opendal-relatedquick-xmlDoS advisories (RUSTSEC-2026-0194/0195) as unreachable in this workspace.Migration
make setup WHAT=hooksorscripts/lib/mcb.sh install-hooks.require_*: update to returnResult<_, Box<CallToolResult>>and deref when early-returning.pre-pushormake check WHAT=allfor full gates;pre-commitenforces staged guards only.Written for commit 50dc26d. Summary will update on new commits.