Skip to content

fix(tui): make Full Access truly full access for publish-like shell#4596

Merged
Hmbown merged 1 commit into
mainfrom
agent/091-push-gate
Jul 20, 2026
Merged

fix(tui): make Full Access truly full access for publish-like shell#4596
Hmbown merged 1 commit into
mainfrom
agent/091-push-gate

Conversation

@Hmbown

@Hmbown Hmbown commented Jul 20, 2026

Copy link
Copy Markdown
Owner

Fixes #4595.

What changed

Two defects made routine development prompt under the Full Access posture:

  1. Every git push was classified publish-like. Classification is now ref-aware and fail-closed: force/delete/tags/mirror/all/prune forms, +refspecs, wildcard refspecs, empty-src deletes, pushes to main/master/HEAD, release-lane refs, tag-like refs (v<digit>, refs/tags), unrecognised flags, and ambiguous upstream forms (bare git push, git push origin) all remain publish-like; only an explicit non-force push whose every refspec destination is a plain unprotected branch is routine shell.

  2. The publish floor held in every posture, including Bypass. Full Access means the user granted publish authority, so the floor now applies only to the Ask/Auto-Review postures. The catastrophic-destroyer floor (device writes, system-path rm -rf) still applies in every posture — it guards against model error, not user intent.

Evidence

  • 37 auto_review tests including new matrices: feature-branch pushes not held, 20 protected/ambiguous forms held, Full Access skips the publish floor.
  • The yolo_mode publish engine test now locks the new contract end to end (no ApprovalRequired under Bypass); the background-destructive engine test keeps proving destroyers still hold.
  • Full locked TUI suite: 7,612 passed / 0 failed / 3 ignored. Strict all-target/all-feature locked clippy clean.

Fixes #4595.

Two defects made routine development prompt under the Full Access
posture:

1. Every git push was classified publish-like. shell_tokens_are_publish_like
   treated the command-safety canonical class 'git push' as a publish
   regardless of target, so a feature-branch push tripped the durable-review
   floor. Classification is now ref-aware and fail-closed: force/delete/
   tags/mirror/all/prune forms, +refspecs, wildcard refspecs, empty-src
   deletes, pushes to main/master/HEAD, release-lane refs, tag-like refs
   (v<digit>, refs/tags), unrecognised flags, and ambiguous upstream forms
   (bare 'git push', 'git push origin') all remain publish-like; only an
   explicit non-force push whose every refspec destination is a plain
   unprotected branch is routine shell.

2. The publish floor held in every posture, including Bypass. Full Access
   means the user granted publish authority, so the floor now applies only
   to the Ask/Auto-Review postures. The catastrophic-destroyer floor
   (device writes, system-path rm -rf) still applies in every posture —
   it guards against model error, not user intent.

The yolo_mode publish engine test now locks the new contract end to end:
no ApprovalRequired for a publish-like shell under Bypass, while the
background-destructive test keeps proving destroyers still hold.

Verified locally: 37 auto_review tests (including new feature-branch,
protected/ambiguous, and Full Access floor matrices), 502
engine/approval/command_safety tests, strict all-target/all-feature
locked clippy, fmt.

Signed-off-by: Hunter B <hmbown@gmail.com>
Copilot AI review requested due to automatic review settings July 20, 2026 02:18
@cursor

cursor Bot commented Jul 20, 2026

Copy link
Copy Markdown

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Claude finished @Hmbown's task in 4m 36s —— View job


Claude finished reviewing @Hmbown's PR

  • Gather context (diff, changed files, CLAUDE.md/AGENTS.md guidance)
  • Understand the request (code review per custom instructions)
  • Review auto_review.rs classification logic (ref-aware push classification)
  • Review auto_review.rs publish-floor posture gating
  • Review test coverage in engine/tests.rs and auto_review.rs
  • Check provider/model/route safety invariant (not applicable to this PR)
  • Post final review summary

Summary

Solid, well-scoped fix. The safety_floor gating on ApprovalMode::Bypass (auto_review.rs:350) is correct and minimal — it only touches the Publish arm, leaves the destroyer floor (auto_review.rs:356-362) untouched, and matches the PR's stated intent ("Full Access means the user granted publish authority"). The git push ref-parsing (git_push_tokens_are_publish_like / git_push_refspec_is_protected, auto_review.rs:592-676) is careful about flags, force/delete/tag/mirror/all forms, +refspecs, wildcards, and ambiguous-upstream shapes, and the 37-test matrix (feature branches routine, 20 protected/ambiguous forms held, Bypass skips the floor) backs it up well. I traced each test case against the implementation by hand and they line up.

Finding: unresolved shell expansion in a refspec destination defaults to "not protected"

crates/tui/src/tui/auto_review.rs:643-676 (git_push_refspec_is_protected)

The function is a denylist: it returns false (routine, floor skipped) unless the destination textually matches a known-unsafe pattern (main/master/head, release*, v<digit>, refs/*, etc.). But the code's own doc comment states the opposite design goal:

"Fail closed: any flag, shape, or ref we do not positively recognise keeps the durable-review hold." (auto_review.rs:589-591)

A destination containing an unresolved shell variable or command substitution — e.g. git push origin $TARGET_BRANCH or git push origin `git symbolic-ref --short HEAD` — doesn't match any of the unsafe patterns textually (the token is literally $TARGET_BRANCH), so git_push_refspec_is_protected returns false and the push is classified as routine shell. If $TARGET_BRANCH resolves to main at execution time (plausible in a scripted agent workflow, or if the env var was set earlier in the same session), the push reaches main while skipping the durable-review floor in the Ask/Auto-Review postures — exactly the failure mode this PR is designed to close for the "model error, not user intent" case described in the PR body.

Suggest treating a destination containing shell-expansion characters ($, `, (, ), {, }) as protected (return true) rather than falling through to the pattern checks, consistent with the stated fail-closed contract. This is a narrow, low-likelihood edge case (not a regression from prior behavior — the old code held every git push, so this specific gap is new surface introduced by making the classification ref-aware), but worth closing given the safety-floor purpose.

Fix this → — patches git_push_refspec_is_protected and adds a regression test.

Other notes

  • Reuse: git_push_tokens_are_publish_like correctly reuses the existing git_subcommand_index / shell_token_eq / shell_token_trim helpers rather than reimplementing subcommand lookup — no duplication concerns.
  • Tests: coverage is thorough for the stated contract; no gaps beyond the expansion case above. The renamed yolo_mode_executes_publish_like_shell_without_prompt test in engine/tests.rs correctly inverts the old assertion (panics on ApprovalRequired instead of expecting/denying it) and keeps the background-destructive test as a separate, untouched guard.
  • Provider/model/route safety (EPIC v0.8.65 EPIC: Separate provider facts, model facts, offerings, and route resolution #2608 invariant): not applicable — this PR only touches shell/git-push classification and approval posture, no model or provider selection logic.
  • I was not able to run cargo check/cargo test/clippy in this sandbox (bash network/tool access is restricted here), so I could not independently reproduce the "7,612 passed" run mentioned in the PR description — my review is based on manual tracing of the diff against the test matrix, which lined up correctly in every case I checked.

@Hmbown
Hmbown merged commit 7ec5c7e into main Jul 20, 2026
23 checks passed
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.

Full Access prompts for feature-branch git push: publish floor classifies every push as publish-like

2 participants