fix(git-safe): close checkout/switch force+detach smuggling and pull last-wins gap by devin-ai-integration[bot] · Pull Request #223 · rmems/writ

devin-ai-integration · 2026-10-09T03:00:36Z

Summary

SafeGitCommand spelled its force/detach checks exactly, so combined short-option clusters smuggled the same flags straight through — reopening the WIP-destruction paths the checks exist to block:

git checkout -fb / checkout -tf — discards uncommitted work, identical to the rejected bare -f/--force.
git switch -fd / switch -td — discards worktree changes and detaches HEAD, bypassing both the force rejection and the --detach rejection in one token. switch admits -d as detach (is_detach_flag), so a cluster starting with -d was caught, but -fd/-td were not.
git switch --discard-changes — the long spelling of switch -f, unhandled.
git pull --rebase --rebase=false — pull_uses_safe_history_strategy returned on the first --rebase it saw, but git applies the last rebase option, so this spelled a merge pull that slipped past the rebase/ff-only requirement.

The push-scoped cluster scan (is_combined_short_force_cluster) only ran for subcommand == "push", so none of this was visible on the local-side commands where it matters most — including through admit_git_invocation and the writ supervisor path (reject_mismatched_checkout sees checkout -fb <expected> as a matching target).

Fix

New smuggled_cluster_flag(subcommand, arg, flag) scans cluster letters only until the first value-taking option (checkout -b/-B, switch -c/-C) — so switch -cd <name> still parses d as -c's attached value, not --detach.
is_bare_force_flag now also rejects --discard-changes[=...].
pull_uses_safe_history_strategy evaluates rebase options last-wins like git; --ff-only still qualifies anywhere.

Six new tests cover the cluster spellings, --discard-changes, the -cd value-parsing boundary, and both last-wins pull orderings.

Validation

cargo test -p writ-core git_safe: 136 passed (includes 6 new)
cargo test --workspace: 622 passed
cargo fmt --check: clean
Baseline note: cargo clippy --workspace --all-targets -- -D warnings fails on a pre-existing needless_return at paths.rs:25 on a clean tree (unrelated to this diff; my files are clippy-clean).

Link to Devin session: https://app.devin.ai/sessions/ca3edfc32e1f44ada3bbab1345c3f358
Open in Devin Desktop: https://app.devin.ai/desktop/session/ca3edfc32e1f44ada3bbab1345c3f358?variant=devin
Requested by: @rmems

Summary by cubic

Fixes SafeGitCommand so checkout/switch can no longer smuggle force/detach flags via combined short clusters (-fb, -tf, -fd, -td), and git pull now respects git's last-wins rebase option ordering. Option scanning stops at --, so a pathspec like checkout -- -fd is never read as a flag cluster, and falsy --discard-changes= values disable discarding instead of carrying -f semantics.

Resolves create-branch values inside attached clusters (switch -cd → branch d, checkout -bfoo → foo) so the supervisor's expected-branch pin compares the branch HEAD actually lands on, covering -b/-B and -c/-C in attached and =-prefixed forms. Attached values are kept verbatim: git creates =name for -c=name, so the pin compares the literal name.
Rejects --discard-changes as the long spelling of switch -f, except =false/0/no/off.
Evaluates --rebase/--no-rebase/--rebase= in order so --rebase --rebase=false is correctly rejected as a merge pull.
Moved the detach and cluster checks into reject_checkout_switch_escape; no behavior change.
New tests cover the cluster spellings, verbatim create values, --discard-changes booleans, the -- pathspec boundary, and both pull orderings.

^{Written for commit 2bc724f. Summary will update on new commits.}

…last-wins gap Combined short-option clusters admitted force and detach spellings that the standalone-flag checks were written to reject: - git checkout -fb / -tf discarded uncommitted work (same as blocked -f) - git switch -fd / -td discarded work and detached HEAD, bypassing the --detach rejection and the assigned-branch invariant - git switch --discard-changes is the long spelling of switch -f - pull --rebase --rebase=false was admitted as a rebase pull though git applies the last option and performs a merge Scan clusters up to the first value-taking letter (checkout -b/-B, switch -c/-C) so switch -cd keeps parsing d as -c's attached value. Evaluate pull's rebase flags last-wins like git does. Co-Authored-By: Raul Cardenas Montoya <montoyaraul34@gmail.com>

devin-ai-integration · 2026-10-09T03:00:40Z

I'll fix CI failures and address comments from users with write access that start with 'Devin'.

Disable automatic comment, CI, and merge conflict monitoring

Original prompt from Raul Cardenas Montoya

!!bug_catcher !!_repo_maintenance !!triage !!plan
Find me where as is the techincal debt at? I am willing to delete a large amount of strict code, main focus is to solve collabartion among different harness and subagents on cloud and locally. idea is that so agents aren't ovelapping each other work, can keep human with what is going on and report back to the main agent. Collabaration to increase creativity and diveristy agent ideas. right now we are pull planning to create issues. Tell me if should keep github/linear twin or some linear or linear only?

@playbook:playbook-973b97a113a74767801198db48b21a21

codeant-ai · 2026-10-09T03:00:42Z

Skipping PR review because a bot author is detected.

If you want to trigger CodeAnt AI, comment @codeant-ai review to trigger a manual review.

coderabbitai · 2026-10-09T03:00:49Z

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository: rmems/writ/.coderabbit.yaml
Review profile: CHILL
Plan: Advanced
Run ID: 2b79f640-a3f0-42fb-98a8-ee5e9160c81f

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.}

codacy-production · 2026-10-09T03:01:45Z

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 10 duplication

Metric Results

Duplication 10

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer}
_{TIP This summary will be updated as you push new changes.}

codecov · 2026-10-09T03:03:50Z

Codecov Report

❌ Patch coverage is 94.28571% with 6 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
crates/writ-core/src/git_safe/mod.rs	94.28%	6 Missing ⚠️

📢 Thoughts on this report? Let us know!

CodeScene delta review flagged the added checks: SafeGitCommand::new grew 24->28 cyclomatic complexity and smuggled_cluster_flag carried a compound guard conditional. Move the detach + cluster-force + cluster-detach rejection into reject_checkout_switch_escape and split the new conditionals into single-branch guards. No behavior change; the six regression tests still pass. Co-Authored-By: Raul Cardenas Montoya <montoyaraul34@gmail.com>

devin-ai-integration · 2026-10-09T03:05:00Z

Complex Method increase addressed: the three checks (detach, cluster f, cluster d) now live in reject_checkout_switch_escape, called once from new. SafeGitCommand::new is back under its pre-PR complexity.

Devin: writ agent: fixed in 5f91f15

devin-ai-integration · 2026-10-09T03:05:05Z

Complex Conditional fixed: the compound !starts_with('-') || starts_with("--") || len <= 2 guard is now three single-branch early returns.

Devin: writ agent: fixed in 5f91f15

Codecov flagged the -b/-B value-taking arm and the non-alphabetic early exit in smuggled_cluster_flag as the only uncovered lines. Co-Authored-By: Raul Cardenas Montoya <montoyaraul34@gmail.com>

kilo-code-bot · 2026-10-09T03:37:03Z

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Files Reviewed (2 files)

crates/writ-core/src/git_safe/mod.rs - incremental change verified
crates/writ-core/src/git_safe/tests.rs - incremental change verified

Previous Review Summaries (4 snapshots, latest commit 27e4d43)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit `27e4d43`)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (2 files)

crates/writ-core/src/git_safe/mod.rs - doc comment clarification
crates/writ-core/src/git_safe/tests.rs - new parametrized test

Previous review (commit `d451c0f`)

Status: 3 Suggestions | Recommendation: Merge (suggestions are optional improvements)

Overview

Severity	Count
CRITICAL	0
WARNING	0
SUGGESTION	3

Issue Details (click to expand)

SUGGESTION

File	Line	Issue
`crates/writ-core/src/git_safe/mod.rs`	623	Comment could clarify both attached and separate value forms
`crates/writ-core/src/git_safe/tests.rs`	289	Add tests for force-create variants (`-B`, `-C`)
`crates/writ-core/src/git_safe/tests.rs`	289	Add tests for attached equals form (`-b=foo`, `-c=foo`)

Files Reviewed (2 files)

crates/writ-core/src/git_safe/mod.rs - 1 suggestion
crates/writ-core/src/git_safe/tests.rs - 2 suggestions

Note: Two previous review findings (smuggled cluster -- handling and --discard-changes=false false positive) are already fixed in the current code.

Previous review (commit `f74effc`)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (2 files)

crates/writ-core/src/git_safe/mod.rs
crates/writ-core/src/git_safe/tests.rs

Previous review (commit `17937bc`)

Status: 2 Issues Found | Recommendation: Low-risk suggestions, safe to merge

Overview

Severity	Count
CRITICAL	0
WARNING	0
SUGGESTION	2

Issue Details (click to expand)

SUGGESTION

File	Line	Issue
`crates/writ-core/src/git_safe/mod.rs`	709	`smuggled_cluster_flag` does not skip args after `--` (end-of-options), causing false positives on file paths like `git checkout -- -fd`
`crates/writ-core/src/git_safe/mod.rs`	1023	`--discard-changes=false` (negated form, a no-op) is rejected by `starts_with("--discard-changes=")`

Files Reviewed (2 files)

crates/writ-core/src/git_safe/mod.rs - 2 SUGGESTIONs
crates/writ-core/src/git_safe/tests.rs

The PR closes all four smuggling vectors described in the PR:

checkout -fb/-tf — caught by new smuggled_cluster_flag(subcommand, arg, 'f') in reject_checkout_switch_escape, which scans short-option cluster letters up to the first value-taking option (-b/-B for checkout).
switch -fd/-td — -fd caught by the 'f' cluster scan; -td caught by the 'd' cluster scan (switch-only). Both correctly separated from the --detach/-d exact checks in is_detach_flag.
switch --discard-changes — added to is_bare_force_flag, rejecting it globally (safe since no other subcommand uses this option, verified against git 2.55.0 binary).
pull --rebase --rebase=false — pull_uses_safe_history_strategy rewritten to last-wins semantics (single loop with rebase accumulator), matching git's behavior where the last rebase option wins.

The refactoring in commit 2 cleanly extracts all checkout/switch escape checks into reject_checkout_switch_escape, reducing SafeGitCommand::new cyclomatic complexity back below the CodeScene threshold. Six new tests plus one coverage test cover all cluster spellings, --discard-changes, value-taking boundaries (-cd/-bf), and both pull orderings.

All entry points (main.rs, supervisor_cmd.rs, hook.rs, restricted.rs) route through SafeGitCommand::new, so the fix covers CLI, supervisor, hook, and restricted paths.

Verified against git 2.55.0: --discard (without -changes) is NOT a valid option in git-switch or git-checkout (confirmed by binary string search), so the --discard-changes check is sufficient.

_{Reviewed by free · Input: 684.3K · Output: 49.2K · Cached: 1.2M}

_{Review guidance: REVIEW.md from base branch main}

Kilo review nits: args after the end-of-options marker are pathspecs, so checkout -- -fd must not be read as a force cluster; and the boolean --discard-changes=<bool> takes explicit values where =false/0/no/off disable it rather than carry -f semantics. Co-Authored-By: Raul Cardenas Montoya <montoyaraul34@gmail.com>

devin-ai-integration · 2026-10-09T03:39:25Z

Fixed: all three scans in reject_checkout_switch_escape now stop at --, so git checkout -- -fd (a pathspec) is no longer read as a force cluster. Covered by checkout_pathspec_after_end_of_options_is_not_scanned.

Devin: writ agent: fixed in f74effc

devin-ai-integration · 2026-10-09T03:39:30Z

Fixed: --discard-changes=<v> now only rejects truthy values — =false/=0/=no/=off pass through as the no-op negations they are. =true/=yes/bare still rejected. Covered by switch_discard_changes_falsy_values_are_noops.

Devin: writ agent: fixed in f74effc

devin-ai-integration

✅ Devin resolved all 2 findings on `27e4d43`

Fixed by Devin (2)

Attached branch names bypass branch pinning
Equals-prefixed branch escapes branch pin

View all findings in Devin Review

…ters checkout_or_switch_target skipped `-cd`-style clusters as pure flags, so `switch -cd <point>` read the start point as the target branch while git actually creates and lands on the attached name. The supervisor's --expected-branch pin compared the wrong name and admitted the escape. cluster_create_value now resolves the value at the first value-taking letter (-b/-B on checkout, -c/-C on switch): attached remainder, optional '=' prefix, or the next argv token when the letter is last. The same letter table is shared with smuggled_cluster_flag. Found by Devin Review on PR #223. Co-Authored-By: Raul Cardenas Montoya <montoyaraul34@gmail.com>

Per Kilo review on #223: parametrize the create-cluster test over -b/-B (checkout) and -c/-C (switch) in both attached and =-prefixed spellings, and clarify the cluster_create_value doc comment. Co-Authored-By: Raul Cardenas Montoya <montoyaraul34@gmail.com>

devin-ai-integration · 2026-10-09T04:14:53Z

Resolved — coverage added in 27e4d43.

devin-ai-integration · 2026-10-09T04:14:56Z

Resolved — doc clarified in 27e4d43.

devin-ai-integration · 2026-10-09T04:15:03Z

Resolved — force-create variants covered in 27e4d43.

git does NOT strip '=' from short-option values: 'switch -c=name' creates branch '=name' (verified on git 2.x). Stripping it made the branch pin compare 'name' while HEAD landed on '=name' — a pin escape for any expected branch the attacker spells '-c=<expected>'. Found by Devin Review on PR #223. Co-Authored-By: Raul Cardenas Montoya <montoyaraul34@gmail.com>

codescene-access

Code Health Improved (1 files improve in Code Health)

Gates Passed
6 Quality Gates Passed

See analysis details in CodeScene

View Improvements

File	Code Health Impact	Categories Improved
mod.rs	4.50 → 4.50	Low Cohesion, Complex Method, Large Method, Overall Code Complexity, String Heavy Function Arguments

Quality Gate Profile: Pay Down Tech Debt
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.

devin-ai-integration · 2026-10-09T04:57:00Z

Resolved — fixed in 2bc724f (verbatim attached values).

devin-ai-integration · 2026-10-09T04:57:01Z

Resolved — fixed in d451c0f (cluster-aware target parsing).

devin-ai-integration · 2026-10-09T06:29:06Z

Devin is currently unreachable - the session may have died.

View session

github-project-automation Bot added this to Autonomous Software Engineering Oct 9, 2026

devin-ai-integration Bot assigned rmems Oct 9, 2026

devin-ai-integration Bot added bug Something isn't working safety labels Oct 9, 2026