Skip to content

fix(gateguard): gate force/path git checkout as destructive#2158

Merged
affaan-m merged 2 commits into
affaan-m:mainfrom
bymle:fix/gateguard-checkout-force
Jun 7, 2026
Merged

fix(gateguard): gate force/path git checkout as destructive#2158
affaan-m merged 2 commits into
affaan-m:mainfrom
bymle:fix/gateguard-checkout-force

Conversation

@bymle

@bymle bymle commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Summary

GateGuard challenges destructive commands before they run, but its git checkout handler in scripts/hooks/gateguard-fact-force.js only flagged git checkout -- <path>:

if (command === 'checkout') {
  return rest.includes('--');
}

It missed:

  • git checkout --force / git checkout -f <branch> — force checkout, discards local uncommitted changes
  • git checkout .discards all working-tree changes

So those forms bypassed the gate. Worse, once the once-per-session routine-Bash gate is satisfied by any prior command, git checkout -f main runs with no challenge at all — exactly the destructive scenario GateGuard exists to catch. The semantically-identical git switch -f main is gated, so this is an inconsistent gap, not policy.

Fix

Mirror the existing switch handler (which already covers --force / -f): treat --, ., --force, and short -f-style flags on checkout as destructive.

if (command === 'checkout') {
  return rest.some(t => {
    if (t === '--' || t === '.' || t === '--force') return true;
    if (!t.startsWith('-') || t.startsWith('--')) return false;
    return t.slice(1).includes('f');
  });
}

This preserves the existing -- behavior and leaves branch-switching (git checkout main, git checkout -b new) un-gated.

Verification

Added a test to tests/hooks/gateguard-fact-force.test.js asserting git checkout -f main is denied with a Destructive fact-forcing message (alongside the existing git push --force / git commit --amend cases). Verified it fails before the fix (only the routine gate fires) and passes after. The gateguard suite is green (92/92); full node tests/run-all.js is 2618 passed with 1 pre-existing unrelated failure (scripts/trae-install.test.js, identical on main).


Summary by cubic

Fixes a gap in GateGuard so destructive git checkout forms (--, ., --force, -f) are challenged, preventing unprompted discards of uncommitted changes. Non-destructive branch switching remains unaffected.

  • Bug Fixes
    • Flag git checkout with --, ., --force, or -f as destructive in scripts/hooks/gateguard-fact-force.js.
    • Add test (Test 7b) asserting git checkout -f main is denied as destructive.

Written for commit 82aada8. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • Bug Fixes

    • Broadened detection of destructive git checkout actions to also recognize additional force/discard invocation forms that can overwrite working-tree changes, improving protection against unintended destructive operations.
  • Tests

    • Added new test coverage to confirm these destructive checkout patterns are detected and denied, including checks for denial reasons and gate state handling.

The destructive-command gate's `checkout` handler only flagged
`git checkout -- <path>`. It missed `git checkout --force` / `-f <branch>`
and `git checkout .`, all of which discard uncommitted working-tree changes,
so they bypassed the gate (once the once-per-session routine-Bash gate is
satisfied, they ran with no challenge). The sibling `switch` handler already
covers these force forms; mirror it for `checkout`.
@bymle bymle requested a review from affaan-m as a code owner June 5, 2026 17:50
@coderabbitai

coderabbitai Bot commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: fded5d4c-d707-45d1-9f36-400b26aa159d

📥 Commits

Reviewing files that changed from the base of the PR and between ddb90e4 and 82aada8.

📒 Files selected for processing (1)
  • tests/hooks/gateguard-fact-force.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/hooks/gateguard-fact-force.test.js

📝 Walkthrough

Walkthrough

The PR broadens the gateguard-fact-force hook's destructive detection for git checkout to include --, ., --force, and short flags containing f, and adds a test verifying git checkout -f main is denied with destructive/rollback reasoning.

Changes

Destructive Checkout Detection

Layer / File(s) Summary
Expand destructive checkout detection and test
scripts/hooks/gateguard-fact-force.js, tests/hooks/gateguard-fact-force.test.js
isDestructiveGit logic for git checkout now detects --, ., --force, and short flags containing f as destructive patterns. Added "Test 7b" to assert git checkout -f main produces a denial with Destructive and rollback in the reason.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

  • affaan-m

Poem

🐰 A rabbit peeks at flags in flight,
-f now caught before it bites,
-- and --force too,
Tests hop in to guard the view,
Gate stays steady through the night.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: adding destructive-command detection for force/path variants of git checkout to the GateGuard hook, which is the core focus of the changeset.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
tests/hooks/gateguard-fact-force.test.js (1)

304-318: ⚡ Quick win

Consider adding test coverage for other destructive checkout forms.

Test 7b validates -f, but the implementation also gates git checkout ., git checkout --, and git checkout --force. Adding explicit tests for these patterns would prevent regressions if the logic changes.

🧪 Suggested additional test cases
   // --- Test 7b: gates force/path git checkout as destructive Bash ---
   clearState();
   if (test('denies git checkout -f as destructive Bash', () => {
     const input = {
       tool_name: 'Bash',
       tool_input: { command: 'git checkout -f main' }
     };
     const result = runBashHook(input);
     assert.strictEqual(result.code, 0, 'exit code should be 0');
     const output = parseOutput(result.stdout);
     assert.ok(output, 'should produce JSON output');
     assert.strictEqual(output.hookSpecificOutput.permissionDecision, 'deny');
     assert.ok(output.hookSpecificOutput.permissionDecisionReason.includes('Destructive'));
     assert.ok(output.hookSpecificOutput.permissionDecisionReason.includes('rollback'));
   })) passed++; else failed++;

+  // --- Test 7c: gates git checkout . as destructive Bash ---
+  clearState();
+  if (test('denies git checkout . as destructive Bash', () => {
+    expectDestructiveDeny('git checkout .', 'git checkout .');
+  })) passed++; else failed++;
+
+  // --- Test 7d: gates git checkout -- <path> as destructive Bash ---
+  clearState();
+  if (test('denies git checkout -- path as destructive Bash', () => {
+    expectDestructiveDeny('git checkout -- src/file.js', 'git checkout -- path');
+  })) passed++; else failed++;
+
+  // --- Test 7e: gates git checkout --force as destructive Bash ---
+  clearState();
+  if (test('denies git checkout --force as destructive Bash', () => {
+    expectDestructiveDeny('git checkout --force main', 'git checkout --force');
+  })) passed++; else failed++;
+
+  // --- Test 7f: allows plain git checkout (branch switch) ---
+  clearState();
+  if (test('allows plain git checkout branch switch', () => {
+    expectAllow('git checkout feature-branch', 'git checkout feature-branch');
+  })) passed++; else failed++;

Note: The expectDestructiveDeny and expectAllow helpers are defined later in the file (lines 1179-1204), so these tests would need to be placed after line 1175 or the helper definitions moved earlier.

🤖 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 `@tests/hooks/gateguard-fact-force.test.js` around lines 304 - 318, Add
explicit tests in tests/hooks/gateguard-fact-force.test.js that mirror "Test 7b"
(which uses runBashHook) to cover the other destructive checkout forms: "git
checkout .", "git checkout --", and "git checkout --force" and assert they
produce a deny with a Destructive/rollback reason; use the existing helpers
expectDestructiveDeny (or expectAllow for non-destructive cases) to keep
assertions concise and place these new test cases after the helper definitions
(or move expectDestructiveDeny/expectAllow above these tests) so they can be
referenced, and follow the same test pattern (clearState(), build input with
tool_name 'Bash' and tool_input.command, call runBashHook, parseOutput and
assert permissionDecision === 'deny' and reason includes 'Destructive' and
'rollback').
🤖 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 `@tests/hooks/gateguard-fact-force.test.js`:
- Around line 304-318: Add explicit tests in
tests/hooks/gateguard-fact-force.test.js that mirror "Test 7b" (which uses
runBashHook) to cover the other destructive checkout forms: "git checkout .",
"git checkout --", and "git checkout --force" and assert they produce a deny
with a Destructive/rollback reason; use the existing helpers
expectDestructiveDeny (or expectAllow for non-destructive cases) to keep
assertions concise and place these new test cases after the helper definitions
(or move expectDestructiveDeny/expectAllow above these tests) so they can be
referenced, and follow the same test pattern (clearState(), build input with
tool_name 'Bash' and tool_input.command, call runBashHook, parseOutput and
assert permissionDecision === 'deny' and reason includes 'Destructive' and
'rollback').

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 6a963e37-4d18-48e6-800f-61b59e20cd46

📥 Commits

Reviewing files that changed from the base of the PR and between bc8e12b and ddb90e4.

📒 Files selected for processing (2)
  • scripts/hooks/gateguard-fact-force.js
  • tests/hooks/gateguard-fact-force.test.js

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 2 files

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/hooks/gateguard-fact-force.js">

<violation number="1" location="scripts/hooks/gateguard-fact-force.js:275">
P3: `checkout` short-option parsing may overmatch safe commands: git's stuck-form option syntax (`-bfeature`) causes false-positive destructive gating when the argument contains 'f'</violation>
</file>

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment on lines +275 to +279
return rest.some(t => {
if (t === '--' || t === '.' || t === '--force') return true;
if (!t.startsWith('-') || t.startsWith('--')) return false;
return t.slice(1).includes('f');
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: checkout short-option parsing may overmatch safe commands: git's stuck-form option syntax (-bfeature) causes false-positive destructive gating when the argument contains 'f'

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/hooks/gateguard-fact-force.js, line 275:

<comment>`checkout` short-option parsing may overmatch safe commands: git's stuck-form option syntax (`-bfeature`) causes false-positive destructive gating when the argument contains 'f'</comment>

<file context>
@@ -269,7 +269,14 @@ function isDestructiveGit(tokens) {
+    // `git checkout -- <path>`, `git checkout .`, and the force forms
+    // (`--force` / `-f`) all discard uncommitted working-tree changes,
+    // mirroring the `switch` handler below.
+    return rest.some(t => {
+      if (t === '--' || t === '.' || t === '--force') return true;
+      if (!t.startsWith('-') || t.startsWith('--')) return false;
</file context>
Suggested change
return rest.some(t => {
if (t === '--' || t === '.' || t === '--force') return true;
if (!t.startsWith('-') || t.startsWith('--')) return false;
return t.slice(1).includes('f');
});
return rest.some(t => {
if (t === '--' || t === '.' || t === '--force') return true;
if (!t.startsWith('-') || t.startsWith('--')) return false;
const body = t.slice(1);
// `-bfeature` is a stuck-form argument, not a flag cluster.
// Option-taking short opts like -b/-B/-p/-c/-C consume the rest of the token.
// Only treat `f` as destructive when it appears before the first argument-taking option character, or is a standalone flag.
const argTakers = new Set(['b', 'B', 'c', 'C']);
for (let i = 0; i < body.length; i++) {
if (body[i] === 'f') return true;
if (argTakers.has(body[i])) return false;
}
return false;
});

@affaan-m affaan-m merged commit 0cb8907 into affaan-m:main Jun 7, 2026
3 checks passed
syarfandi pushed a commit to syarfandi/ECC that referenced this pull request Jun 9, 2026
…#2158)

* fix(gateguard): gate force/path git checkout as destructive

The destructive-command gate's `checkout` handler only flagged
`git checkout -- <path>`. It missed `git checkout --force` / `-f <branch>`
and `git checkout .`, all of which discard uncommitted working-tree changes,
so they bypassed the gate (once the once-per-session routine-Bash gate is
satisfied, they ran with no challenge). The sibling `switch` handler already
covers these force forms; mirror it for `checkout`.

* test(gateguard): document Test 7b force-checkout case

---------

Co-authored-by: bymle <229636660+bymle@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants