Skip to content

fix(guardrails): block-convention-violation's alias recursion drops git's own globals, over-blocking a mid-merge alias commit #2166

Description

@kyle-sexton

Found by an adversarial verifier while independently checking PR #2152's discharge of #2113. Recorded
as its own item because it is present on origin/main, untouched by that PR, and fixing it there
would have widened a review that already carried two rounds of findings.

The defect

plugins/guardrails/hooks/block-convention-violation.sh:341 and :361 — the alias-recursion argv
rebuild:

check_segment "${w[@]:0:gi+1}" <expansion> "${w[@]:sub_idx+1}"

The slice 0..gi carries everything below the resolved git token, which is where a wrapper's words
live — so env -C inner git … survives the hop. It stops at gi, so git's own globals, which
sit between gi and the subcommand, are dropped. git -C inner qc therefore loses -C inner in the
recursed frame, and the sequencer probe runs in the wrong repository.

Same positional-context family as #2113, one frame deeper: #2113 was about which words effective_dir
may read; this is about which words the recursion forwards.

Measured

Identical on origin/main and on PR #2152's head, so nothing here regressed — it has always been
this way:

origin/main PR #2152 case
0 0 git -C inner commit -F -, MERGE_HEAD in inner → ALLOW (correct — no alias hop)
2 2 git -C inner qc -F - (plain alias), MERGE_HEAD in innershould ALLOW
0 0 git qc -F - (alias), MERGE_HEAD in true repo → ALLOW (correct — no -C)

Row 1 versus row 2 isolates it: the only difference is the alias hop.

Severity: fails CLOSED

This over-blocks. A legitimate mid-merge commit made through a plain git alias, with git's own
-C naming the repository, is content-gated when the guard's own docblock promises a sequencer
exemption ("an in-progress sequencer commit carries a prepared message and is never content-gated").
It is not a bypass — no scenario was found where it lets a violating subject through — so it is a
correctness and usability bug, not a security one.

Suggested direction, not a prescription

The obvious rebuild is "${w[@]:0:sub_idx}" in place of "${w[@]:0:gi+1}", which would forward git's
globals along with the wrapper's words. That is not verified here, and it needs care: the same
splice is what keeps command-line -c/--config/--config-env globals flowing through each alias
hop, and widening it changes what the --config-env shape refusal sees. Whoever takes this should
re-derive it rather than apply the one-liner.

Acceptance

  • git -C <dir> <alias> with a sequencer in progress in <dir> receives the exemption.
  • A control that fails against the current implementation — row 2 above is one, and it is
    base-failing by construction since the behaviour is identical on both current trees.
  • A discriminator proving the exemption stays conditional: the same command with no MERGE_HEAD
    anywhere must still be gated. block-convention-violation.test.sh has already produced three
    fixtures that could not fail (git commit -C HEAD, a trailing -C dec re-triggering the
    reuse-message exemption, and an env -C wrapper row the old every-word scan caught by accident),
    so a lone "now exempt" assertion is not sufficient evidence here.

Related

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

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions