Skip to content

The merge argument allow-list duplicates gh-axi's parser to decide safety, and that model has been wrong twice #259

Description

@HelloWorldSungin

The pattern, not the bug

assert_merge_args_allowed decides whether a merge invocation is safe by modelling how gh-axi parses arguments. That model has now been wrong twice, and each time the error produced a real defect:

  1. It refused a legitimate --sha abc123 because the value was read as an unknown flag. The repair taught the allow-list that value-taking flags consume the next word.
  2. That repair then let -- --subject --auto through: --subject consumed --auto as its value, so nothing was refused, and gh-axi - which finds flags by scanning, not positionally - armed deferred execution anyway. The immediate-execution guarantee the allow-list exists to provide was defeated by argument order.

Both are being fixed. This issue is about the shape that produced them.

The two failures point in opposite directions

This is the part that makes it a design question rather than a bug count. One failure refused something legitimate; the other admitted something forbidden. That is the signature of a model that is wrong, not merely incomplete - a stricter version of the same model would have failed the first case, and a looser version would have failed the second. There is no calibration of this approach that avoids both.

Concretely, the second failure exists because gh-axi 0.1.34 (dist/src/commands/pr.js) takes --auto at line 486 before it takes --subject at line 489, and its flag reader returns undefined rather than erroring on a valueless flag. Our guard assumed positional consumption. That is a version-observed fact about a third-party tool, and the guard's correctness silently depends on it.

Why it recurs

The guard duplicates another program's parser in order to decide safety. That coupling is invisible in the code: the allow-list looks like a self-contained list of permitted strings, and nothing in it says "this is only correct while gh-axi parses the way I assumed". It is also a dated dependency - a change to gh-axi's parsing silently invalidates the guard with no failing test anywhere.

A guard whose correctness depends on an assumption it does not state, about software it does not control, will keep producing this class.

The alternative worth considering

Verify the outcome rather than police the input: rather than proving no forbidden argument can reach the forge, confirm after the call that the merge actually executed immediately and was not queued or deferred. The forge's own state is authority; our model of a CLI's parser is not.

That is a different design with its own costs - it acts and then checks, which is only acceptable where the wrong outcome is detectable and recoverable - and it may well not be right here. It is written down so the option is on the table when someone next repairs this guard.

Deliberately not done now

Not opened inside the merge-path task. Redesigning a guard mid-validation is how a third defect gets introduced, and the two concrete holes are being closed on their own merits first.

Reopen trigger

The next defect traced to the allow-list's model of gh-axi parsing. Two is a pattern; three should force the design question rather than a fourth repair.

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