Skip to content

friction: prevent read-only review worktrees from pushing implementation branches #8321

Description

@ll7

Archetype Metadata

archetype: workflow
evidence_tier: smoke
linked_policy:
  - docs/dev/worktree_lifecycle.md
  - docs/code_review.md
  - AGENTS.md

Goal / Problem

Observed behavior

During the exact-head review of release-tooling PR #8318, a worktree on branch
review/pr-8318-review-20260903 created synthetic merge commit
12347725eaeaa89de03de5bb9e9a9e8a7b5cdad3 and then explicitly pushed
HEAD:fix/issue-8285-erratum-successor-20260902 with force-with-lease. That changed the remote
implementation PR head after its exact-head checks and reviews, despite the worktree's declared
read-only review purpose. The incident was detected by the final remote-head compare-and-swap guard;
#8318 was closed unmerged and replaced by #8320 at the preserved exact commit.

Expected behavior

A read-only review or synthetic-integration worktree must be technically unable to push any remote
ref. Integration checks should use a no-commit merge that is always aborted and should prove the
reviewed remote refs are unchanged before returning success.

Reproduction

# In a worktree intended only for review:
git merge --no-edit origin/main
git push origin HEAD:fix/example-implementation --force-with-lease=<expected-old-sha>

Today the explicit destination refspec bypasses the fact that the review branch has no upstream.

Scope

  • Affected modules: linked-worktree creation/review helpers, push guards, and agent review guidance.
  • Regression: no; this is an observed missing safety boundary.
  • In scope:
    • define a machine-detectable read-only review-worktree mode;
    • reject every git push initiated from that mode, including explicit HEAD:<other-branch> and
      force-with-lease refspecs;
    • provide a canonical synthetic-integration helper using --no-commit --no-ff, guaranteed abort,
      and before/after git ls-remote assertions;
    • add regression coverage for blocked review pushes and allowed implementation-worktree pushes;
    • update review-agent instructions to use the guarded helper.
  • Out of scope:

Added Value Estimation

  • User impact: prevents an apparently read-only review from silently invalidating reviewed PR state.
  • Benchmark impact: protects immutable release and evidence branches from unrelated merge absorption.
  • Maintenance impact: converts a behavioral instruction into an executable, testable boundary.
  • Why now: the failure occurred during the September erratum release and required a replacement PR.

Effort Estimation

  • Rough estimate: 4-8 hours.
  • Best estimate: 6 hours.
  • Unknowns: whether the safest implementation is a per-worktree Git config, guarded wrapper,
    pre-push hook, or a combination; compatibility with existing shared hooks must be tested.

Complexity Estimation

  • Implementation complexity: medium.
  • Dependencies: existing worktree helpers and Git hook installation path.
  • Open questions: how to preserve the guard across fresh shared-venv worktrees without changing
    remote behavior for implementation worktrees.

Risk Assessment

  • Functional risk: a broad guard could block legitimate pushes.
  • Compatibility risk: worktree-local Git configuration differs across installed Git versions.
  • Rollout risk: guidance-only fixes can still be bypassed by explicit refspecs.
  • Mitigation: test both negative review mode and positive implementation mode; fail closed only when
    the worktree is explicitly classified read-only.

Affected Files

  • scripts/dev/create_worktree.sh or a dedicated review-worktree helper — establish protected mode.
  • repository Git-hook/bootstrap owner — reject pushes from protected review worktrees.
  • .agents/skills/implementation-verification/SKILL.md and review workflows — require guarded
    synthetic integration.
  • tests/dev/ — subprocess regression tests with a temporary bare remote.

Definition of Done

  • Root cause and the explicit-refspec bypass are documented.
  • A review-mode worktree cannot push any ref to any configured remote.
  • HEAD:<implementation-branch> and force-with-lease attempts fail before remote mutation.
  • The canonical synthetic merge leaves the worktree clean and the compared remote refs unchanged.
  • An ordinary implementation worktree can still push through the supported path.
  • Focused tests, shell/static checks, and the repository readiness gate pass.

Success Metrics

  • The incident reproduction deterministically fails before the remote ref changes.
  • Before/after remote-ref snapshots are byte-identical in a successful review integration check.
  • Existing implementation push workflows retain their expected behavior.

Validation / Testing

  • Add a temporary bare-remote test reproducing the explicit destination-ref push.
  • Verify protected review mode rejects normal, explicit-refspec, and force-with-lease pushes.
  • Verify the supported implementation path still pushes successfully.
  • Run focused tests/dev/ coverage plus BASE_REF=origin/main scripts/dev/pr_ready_check.sh.

Estimate Discussion

  • Values reflect a bounded workflow/tooling guard plus subprocess Git tests.
  • Confidence: 80%; Git-version and hook-installation behavior may expand the test matrix.
  • The estimate should be revised after identifying the canonical shared hook/config owner.

Project Metadata

  • Priority: high while release branches are active.
  • Effort: 6 hours.
  • Reviewed: no.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workingstate:runningExternal run is currently activetechnical-debttype:workflowWorkflow/tooling semantics task

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions