feat: add devkit review command - #160
Merged
Merged
Conversation
## Summary - add `devkit review [--target <path>] [--base <ref>]` for trusted local or already-materialized worktrees - run the configured pre-commit chain against a complete merge-base-to-final snapshot without committing, pushing, fetching, or copying ephemeral changes back - isolate dependencies, reviewer assets, baselines, caches, telemetry, worktrees, and signal-safe cleanup - bind review cache authority to the resolved merge-base and authenticated runtime ## Safety contract - validates Devkit and the effective hook before clean success - preserves target HEAD, index, files, refs, branches, remotes, and worktree registrations - rejects formatter-only ephemeral success and preserves timeout/signal statuses - executes target-controlled hooks and package scripts only after an explicit trusted-target warning ## Validation - `bun run test:run` — full suite - `bun run typecheck` - `bun run lint` (one pre-existing warning in `husky-block.test.mts`) - `bun run lint:structure` - disposable `bun run build`, package-content audit, and `node dist/cli/index.mjs review --help` smoke This is a fresh implementation on current `main` and supersedes the old monolithic #95 approach. Frink and owners-web are unchanged.
Contributor
📝 WalkthroughWalkthroughAdds ChangesTrusted checkout review
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant review-target.sh
participant Git
participant ReviewWorktrees
participant GateChain
participant Telemetry
CLI->>review-target.sh: launch trusted checkout review
review-target.sh->>Git: resolve target, base, and merge-base
review-target.sh->>ReviewWorktrees: materialize captured trees
review-target.sh->>GateChain: execute configured gates
GateChain-->>review-target.sh: return gate result
review-target.sh->>Telemetry: emit review_run_result
review-target.sh-->>CLI: return final exit status
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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.github.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
gate-engine/deterministic/run.mts (1)
169-180: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winMalformed merge-base/runtime fingerprint silently collapses to a shared
unmanagedcache key.Extends
prefixCacheScopereview-mode cache-key generation by adding newDEVKIT_REVIEW_MERGE_BASEandDEVKIT_REVIEW_RUNTIME_FINGERPRINTinputs to thebasestring when they matchOBJECT_ID; otherwise each is replaced withunmanaged. That fallback is correct for lower-level callers that never set these vars, but it also silently applies when a value is present but malformed (wrong length, non-hex, truncated) — in review mode that would collapse the cache key to the sameunmanagedbucket regardless of the actual merge-base/runtime, exactly the cross-base/runtime authority-sharing this change is meant to prevent, with no error surfaced. The test suite only exercises "unset" and "well-formed" cases, not "present-but-invalid."Consider failing fast (throwing) when
DEVKIT_RUN_MODE === 'review'and a supplied fingerprint doesn't matchOBJECT_ID, rather than silently degrading tounmanaged.♻️ Suggested fail-fast for malformed review fingerprints
const reviewMode = process.env.DEVKIT_RUN_MODE === 'review'; const mergeBase = process.env.DEVKIT_REVIEW_MERGE_BASE; const runtime = process.env.DEVKIT_REVIEW_RUNTIME_FINGERPRINT; + if (reviewMode && mergeBase && !OBJECT_ID.test(mergeBase)) { + throw new Error(`devkit review: malformed DEVKIT_REVIEW_MERGE_BASE: ${mergeBase}`); + } + if (reviewMode && runtime && !OBJECT_ID.test(runtime)) { + throw new Error(`devkit review: malformed DEVKIT_REVIEW_RUNTIME_FINGERPRINT: ${runtime}`); + } const reviewBase = mergeBase && OBJECT_ID.test(mergeBase) ? mergeBase : 'unmanaged'; const reviewRuntime = runtime && OBJECT_ID.test(runtime) ? runtime : 'unmanaged';Please confirm whether the upstream review runner (review-target.sh / dependency-runtime.mts) already guarantees these are always well-formed when set, which would make this moot in practice.
🤖 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 `@gate-engine/deterministic/run.mts` around lines 169 - 180, Update the review-mode cache-key generation in prefixCacheScope so present but malformed DEVKIT_REVIEW_MERGE_BASE or DEVKIT_REVIEW_RUNTIME_FINGERPRINT values fail fast by throwing instead of becoming unmanaged. Preserve the unmanaged fallback only when either variable is unset, and keep valid OBJECT_ID values in the generated base key; verify the upstream runner guarantees are reflected in tests if applicable.
🤖 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 `@gate-engine/deterministic/run.mts`:
- Around line 169-180: Update the review-mode cache-key generation in
prefixCacheScope so present but malformed DEVKIT_REVIEW_MERGE_BASE or
DEVKIT_REVIEW_RUNTIME_FINGERPRINT values fail fast by throwing instead of
becoming unmanaged. Preserve the unmanaged fallback only when either variable is
unset, and keep valid OBJECT_ID values in the generated base key; verify the
upstream runner guarantees are reflected in tests if applicable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1d8388d3-d4d2-459f-b45b-67521ce581aa
📒 Files selected for processing (19)
README.mdcli/__tests__/help-cli.test.mtscli/__tests__/review-cache-session.test.mtscli/__tests__/review-private-dependencies.test.mtscli/__tests__/review-run-result.test.mtscli/__tests__/review-setup-manifest.test.mtscli/__tests__/review.test.mtscli/commands/review.mtscli/index.mtscli/lib/ship/review-target.shcli/lib/ship/review/cache/session.mtscli/lib/ship/review/dependency-runtime.mtscli/lib/ship/review/setup-manifest.mtscli/lib/ship/review/telemetry/run-result.mtscli/lib/ship/run-gates-with-capture.shcli/lib/ship/run-packaged-script.mtsdocs/decisions/review-gate-in-chain.mdgate-engine/deterministic/__tests__/run.test.mtsgate-engine/deterministic/run.mts
norvalbv
added a commit
that referenced
this pull request
Jul 22, 2026
norvalbv
added a commit
that referenced
this pull request
Jul 24, 2026
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.
Summary
devkit review [--target <path>] [--base <ref>]for trusted local or already-materialized worktreesSafety contract
Validation
bun run test:run— 1,890 passed, 5 skippedbun run typecheckbun run lint(one pre-existing warning inhusky-block.test.mts)bun run lint:structurebun run build, package-content audit, andnode dist/cli/index.mjs review --helpsmokeThis is a fresh implementation on current
mainand supersedes the old monolithic #95 approach. Frink and owners-web are unchanged.Summary by CodeRabbit
New Features
devkit reviewcommand for reviewing trusted checkouts without modifying the target, fetching updates, or committing changes.--targetand--baseoptions, review summaries, logs, exit statuses, and safe cleanup behavior.devkit init --review.Documentation
Bug Fixes