fix(bisect): address code review feedback for security and logic issues#333
fix(bisect): address code review feedback for security and logic issues#333marshawcoco wants to merge 12 commits intoweb3infra-foundation:mainfrom
Conversation
- Preserve bisect state after culprit detection until explicit reset - Add `completed` flag to BisectState to distinguish active vs completed sessions - `is_in_progress()` returns false after completion, but state remains for reset - Ensures HEAD moves to the reported bad commit on convergence - Restore original branch (not detached HEAD) on bisect reset - Use `orig_head_name` to restore to branch when available - Add `restore_to_branch()` function to properly restore branch context - Block bisect in bare repositories (P0 security fix) - Bare repos have no working tree, bisect checkout would be destructive - Add early check in handle_start with helpful error message - Reject empty bisect candidate sets with explicit error - Return error instead of Ok(None) for invalid good/bad ranges - Prevents silent incorrect completion for contradictory inputs - Update README to mark bisect as supported command - Add bisect to command list - Remove from "Pending Git commands" section Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ists - Use has_state() instead of is_in_progress() in handle_start - Prevents overwriting preserved orig_head from completed session - Users must run reset before starting new bisect - Require clean working tree before starting bisect - Check both staged and unstaged changes - Prevents data loss from deleting untracked files during checkout Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![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: fe659a5386
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…check - Add ALTER TABLE migration for completed column - Older bisect_state tables don't have the completed column - Check pragma_table_info and add column if missing - Prevents "no such column: completed" error after upgrade - Use IncludeIgnored policy when checking working tree - changes_to_be_staged() omits ignored files by default - Bisect checkout clears entire worktree including ignored files - Prevents silent deletion of .env, cache dirs, etc. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@claude Review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d323801f2f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR refines the bisect command implementation to be safer and more correct by persisting session state through completion, improving reset behavior, and adding guardrails to prevent destructive operations.
Changes:
- Add a
completedflag to persisted bisect state and keep state after culprit detection until explicitbisect reset. - Improve
bisect resetto restore back to the original branch context when available (avoid detached HEAD). - Add safety checks to block bisect in bare repos and require a clean working tree at
bisect start; update README command list accordingly.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/command/bisect.rs | Adds completed state/migration, bare-repo + clean-worktree guardrails, convergence behavior updates, and branch-aware reset restore. |
| README.md | Documents bisect as a supported command and removes it from the “not implemented” list. |
Comments suppressed due to low confidence (1)
src/command/bisect.rs:381
- The PR introduces new behaviors (bare-repo blocking, dirty worktree rejection, and completed-session persistence) but the existing bisect tests don’t cover these paths. Add/adjust L1 tests to assert: (1)
bisect startfails whencore.bareis true, (2)bisect startfails with uncommitted/untracked changes present, and (3) after convergenceis_in_progress()==falsebut state still exists forresetandbad/good/skipare rejected while completed.
// Bare repositories have no working tree - bisect requires checkout operations
if is_bare_repository().await {
return Err(CliError::fatal(
"bisect cannot be run in a bare repository",
)
.with_hint("bisect requires a working tree to check out commits for testing"));
}
// Require a clean working tree to prevent data loss
// Bisect checkout removes and restores files, which would delete untracked content
// Use IncludeIgnored policy to catch ignored files (.env, cache dirs) that would also be deleted
let staged = changes_to_be_committed_safe()
.await
.map_err(|e| CliError::fatal(format!("Failed to check staged changes: {e}")))?;
let unstaged = changes_to_be_staged_with_policy(IgnorePolicy::IncludeIgnored)
.map_err(|e| CliError::fatal(format!("Failed to check unstaged changes: {e}")))?;
if !staged.is_empty() || !unstaged.is_empty() {
return Err(CliError::fatal(
"working tree contains uncommitted changes",
)
.with_hint("commit or stash your changes before running bisect to prevent data loss"));
}
// Check if there's any existing bisect state (active or completed)
// Must use has_state to prevent overwriting preserved orig_head from a completed session
if BisectState::has_state()
.await
.map_err(CliError::fatal)?
{
return Err(CliError::fatal(
"bisect is already in progress, use 'bisect reset' to end it first",
));
}
- Parse bare repo config as full Git boolean (true/yes/on/1) - Previous code only recognized "true" literal - Now handles all Git boolean values correctly - Use CREATE TABLE IF NOT EXISTS for idempotency - Prevents race condition with concurrent bisect commands - Validate bounds before saving bisect state - Prevents orphaned state when bad/good have invalid relationship - Returns error without saving if no commits to test - Block bad/good/skip on completed sessions - After finding culprit, these commands now error - User must run 'bisect reset' to clear completed state - Verify HEAD update succeeded in restore_to_branch - Check actual HEAD value after update_with_conn - Return error if update failed silently - Update hint to warn about per-step workdir clearing - Inform users that each step resets workdir - Warn about deletion of build artifacts and untracked files Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2427304c07
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3ac62bf43
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
When all remaining candidates are skipped, find_next_bisect_point should return Ok(None) so handle_skip can report "all commits have been skipped" and persist the updated state. Previously it returned an error, breaking the skip completion path. Now distinguishes between: - All candidates skipped (skipped commits exist, would have candidates without skip filter) -> return Ok(None) - Invalid bounds (no candidates even without skip filter) -> return error Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ommit - is_bare_repository() now returns Result<bool, CliError> - Fail on unreadable config instead of defaulting to false - Fail on invalid core.bare values instead of silently treating as non-bare - This prevents bisect from proceeding in potentially bare repos - handle_reset now uses the branch's current commit - Branch may have moved during bisect session - Resolve branch commit at reset time instead of using saved orig_head - If branch was deleted, fall back to orig_head as detached HEAD Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Document basic bisect workflow - List all bisect subcommands - Highlight safety features (clean worktree, bare repo protection, etc.) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@codex review |
|
@claude review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91bd1ba0ec
ℹ️ 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".
…or messages - Introduce BisectNext enum (Next/Converged/AllSkipped) to properly distinguish between finding the culprit and exhausting all candidates via skips - Fix misleading error message to mention completed sessions - Handle concurrent schema migration by catching "duplicate column name" error - Fix unused imports in reset_test.rs and tag_test.rs Addresses Copilot review feedback on PR branch. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Move the "Marked X as good/bad" output to after successful validation and save to ensure command output matches actual state transitions. Previously, the message was printed before find_next_bisect_point could return an error for invalid bounds, misleading users and scripts that parse stdout into thinking the mark succeeded when it was discarded. Addresses chatgpt-codex-connector review feedback. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add #[cfg(unix)] conditional imports for: - StableErrorCode in reset_test.rs (used in IO failure test) - path in tag_test.rs (used in tag store failure test) These imports are only needed on Unix where permission-based IO tests run. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Preserve bisect state after culprit detection until explicit reset
completedflag to BisectState to distinguish active vs completed sessionsis_in_progress()returns false after completion, but state remains for resetRestore original branch (not detached HEAD) on bisect reset
orig_head_nameto restore to branch when availablerestore_to_branch()function to properly restore branch contextBlock bisect in bare repositories (P0 security fix)
Reject empty bisect candidate sets with explicit error
Update README to mark bisect as supported command