Skip to content

Approval gate does not inspect gh pr create --fill #355

Description

@twistedmelonman

gh pr create -f/--fill produces a body that _gh_wrapper_approval_gate never
inspects. Found immediately after #354 merged, while confirming the gate against
the way the flag is actually used day to day.

What happens now

--fill builds the title and body from commit messages. It is not -F, and the
gate's arg walk does not look for it, so a --fill invocation carries no flag
the gate recognises, falls through the "no body flag at all" early return, and
passes ungated.

-f and -F are distinguished correctly -- bash case is case-sensitive, and a
-F attached-value form (-Fpath) still matches the file branch. Verified, not
assumed.

Why this is not urgent

The body --fill produces comes from commit messages, and every commit message
already passed the gate on its way into the repo. The text has been read. So the
current behaviour is right in practice.

It is right by accident, though. Nothing in the code says --fill was considered,
and a reader has to reconstruct the argument to know the path is safe. That is the
same shape as the defects in the false-OK pattern: correct output, reasoning that
was never written down, no test pinning it.

Decided: treat --fill as pre-approved (option 1 of 2)

Do not gate it. Record why, and pin it.

  1. Name -f, --fill, --fill-first and --fill-verbose in the gate's comment
    as deliberately ungated, with the reason: their body is derived from commit
    text the gate already approved.
  2. Add test cases asserting each passes, so a later refactor that starts gating
    them fails loudly instead of silently adding friction.
  3. Note the one soft spot: on a multi-commit branch, --fill concatenates
    messages into a body no one approved as that combined text. Every sentence in
    it was approved individually. Accepted as covered-by-construction rather than
    closed.

The alternative -- reconstructing the body and checking it -- was rejected. It
would block gh pr create -f on any multi-commit branch until the concatenation
was approved, which is a round-trip to defend against text that was already read.
--fill is the normal way this repo's PRs get made, so the cost lands on the
common path.

Not included

--fill-first and --fill-verbose take their body from commit text too and are
covered by the same reasoning. --web opens a browser and is out of scope for a
shell-side gate.

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

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions