Skip to content

Rectify: GitHub Mutation Guard Argv Parsing Immunity (Combined Plan) - #4680

Open
Trecek wants to merge 16 commits into
developfrom
impl-rectify-gh-mutation-guard-argv-20260817-090109
Open

Rectify: GitHub Mutation Guard Argv Parsing Immunity (Combined Plan)#4680
Trecek wants to merge 16 commits into
developfrom
impl-rectify-gh-mutation-guard-argv-20260817-090109

Conversation

@Trecek

@Trecek Trecek commented Aug 17, 2026

Copy link
Copy Markdown
Collaborator

Summary

This plan was developed and reviewed as four parts; they are combined here into one document for convenience, but must still be implemented sequentially in this order — each part depends on state the previous part establishes:

Each part below retains its own Summary, How Tests Missed This, Related Issues Found, Architectural Analysis, Immunity Plan (failing tests → implementation → wiring check), and Verification — every part must independently pass task test-all (this project's configured test_check.command in .autoskillit/config.yaml) before the next begins, per this project's multi-part plan green-gate convention.

Three rounds of adversarial review (a control-flow Foundation Audit, a variable-provenance Interface Mapping pass, and a Registry/completeness Trace) were run against the full draft of this plan; their findings are already incorporated into the text below, not listed separately.

A subsequent /audit-impl pass found two gaps against this plan: REQ-065 (a test-suite parametrize-matrix convention specified in Part D was only partially applied) was fixed directly; REQ-044 (Part C's _gh_args_have_bare_help_flag fix) was implemented deliberately narrower than the plan specified, after tracing that the plan's literal default-arity-flip reintroduces a real mutation-guard bypass the project's own pre-existing regression test already pins — filed as a formal deviation and independently confirmed ACCEPT by the project's own deviation-evaluator agent.

Closes #4655

Implementation Plan

Plan file: /home/talon/projects/generic_automation_mcp/.autoskillit/temp/rectify/rectify_github_mutation_guard_argv_parsing_immunity_2026-08-17_061427_combined.md

🤖 Generated with Claude Code via AutoSkillit

@Trecek Trecek mentioned this pull request Aug 17, 2026
Open
Trecek and others added 16 commits August 19, 2026 08:24
@Trecek @claude
fix(hooks): replace gh api's ad-hoc flag chain with a declarative spe…
af2c2f4 
…c engine

Closes Issue #4655 Defect 1 (rectify Part A).

_analyze_gh_api()'s if/elif chain recognized only --method/-X, --input,
--field/-F, --raw-field/-f, -H/--header/--hostname/--cache, and --paginate.
Any other flag (--jq, --template, -p) fell through to a generic branch that
advanced past the flag token without consuming its value, so the value was
misread as a second route and tripped a false request_cardinality_unresolved
deny.

Replace the ad-hoc chain with a declarative {flag: arity} spec table
(_GH_API_FLAG_SPEC, verified against a live `gh api --help`) consumed by one
shared argv-walking engine (_consume_gh_flag). A flag absent from the table
now fails closed with its own distinguishable reason code
(unrecognized_gh_api_flag) instead of being silently misparsed as a second
route under the same misleading reason code as a genuinely-ambiguous input.

- _FlagArity/_GH_API_FLAG_SPEC/_consume_gh_flag added near _flag_value,
  which they complement rather than replace (curl's extraction is
  unaffected; migrating it is Part C's job).
- _analyze_gh_api's catch-all branch now calls _consume_gh_flag and denies
  unrecognized flags explicitly.
- tests/hooks/test_gh_api_flag_spec_contract.py: live-CLI contract test
  parsing `gh api --help`'s FLAGS section, asserting every value-taking
  flag is present in _GH_API_FLAG_SPEC -- catches future gh CLI additions
  before they silently reintroduce this defect shape.
- New tests close the coded-but-untested form-grammar gaps identified by
  the investigation (--method=, -XPOST, --input=, long-form --raw-field)
  alongside the three newly-recognized flags, extending the gh-issue-edit
  5-form grammar precedent to gh api's value-taking flags.
- docs/safety/hooks.md: document unresolved_mutation's reason-code family,
  including request_cardinality_unresolved (undocumented until now) and
  the new unrecognized_gh_api_flag.
- tests/arch/test_subpackage_isolation.py: bump _command_classification.py's
  REQ-CNST-010-E10 line-limit exemption 2350 -> 2450 for this addition.

Part B (quote-provenance tokenizer, Defect 2) and Part C (extending this
spec-table engine to curl/git/pip/write-guard bypass-direction bugs) follow
in subsequent commits per the combined rectify plan's green-gate sequencing.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@Trecek @claude
fix(hooks): quote-provenance tokenizer closes GraphQL $/[ false-positive
82c734c 
Closes Issue #4655 Defect 2 (rectify Part B).

_DYNAMIC_SHELL_TOKEN_RE (\$|`|\*|\?|\[) runs against already-shlex-dequoted
argv tokens, so a fully single-quoted GraphQL document legitimately using
`$` for variables and `[...]` for list literals (e.g. `-f query='mutation(
$id: ID!) { addLabels(labelIds: [$id]) { ... } }'`) was misclassified as
dynamic_target -- shlex's POSIX dequoting discards quote-type information,
so a bare `$id` substring couldn't be distinguished from text a shell would
already have expanded. A prior investigation explicitly rejected relaxing
the character set itself, since that would also accept a real unquoted
command-substitution fragment.

This closes the actual gap: proving, at the raw-command-text level and
independent of shlex's dequoting, whether a specific argv token sat inside
one unbroken `'...'` run -- which is provably shell-inert regardless of
which characters it contains.

- ArgvToken (text, fully_single_quoted, raw_span) threaded through the
  tokenizer (driving shlex token-by-token via instream.tell() to read each
  token's own source span -- no independent re-implementation of shlex's
  own word-boundary/escaping rules, eliminating the risk of the two
  disagreeing), _partition_output_redirect_indices (a behavior-preserving
  refactor exposing the shared index-selection core _partition_output_
  redirects already used, now also feeding _select_executable_argv_tokens),
  _verb_start_index (same refactor for command_verb_and_args), and all 8
  _is_dynamic_shell_value call sites.
- _flag_value/_consume_gh_flag retyped to return ArgvToken, and (beyond the
  plan's literal "inherit the whole token's flag" instruction for their
  own =/bundled forms) upgraded via the new _argv_token_after_prefix to
  re-derive a value's own quote provenance from its raw span when a known,
  never-quoted prefix (a flag name, or gh's `key=` field syntax) precedes
  it -- proven safe even under adversarial prefix-quoting: a True result
  requires the exact bytes `'<value>'` to appear literally in the raw
  command, which is only possible if that span really was one unbroken
  single-quote run, so misalignment can only ever undershoot (a safe false
  negative), never accept a value that wasn't truly quoted. This was load-
  bearing, not optional polish: without it, the plan's own primary AC2/AC3
  test case (`-f query='...'`, the most common real invocation shape) does
  not resolve, because the `query=` prefix outside the quotes makes the
  whole-token flag false even though the value itself is fully quoted.
- _analyze_gh_segment/_analyze_github_segment keep their existing str-based
  semantic dispatch (`args[:2] == ["pr", "review"]` etc.) untouched, adding
  a parallel argv_args/argv_tokens threaded only to the two leaf calls that
  need it (_analyze_gh_api, _issue_edit_request_count) -- avoids retyping
  the whole classification dispatch chain for a change scoped to gh
  api/curl/issue-edit's dynamic-value provenance.
- Argv-payload segments (parsed `subprocess.run([...])` literals) and the
  hardcoded "/graphql" route both wrap as provably-inert ArgvToken by
  construction (never shell-parsed / not argv-derived at all), not a
  fabricated default -- verified via a wiring-check grep that every
  ArgvToken(...) construction site is either the tokenizer itself, the
  dedicated re-derivation helper, or one of these two documented
  exemptions, per the plan's warning about a fabricated-default workaround
  silently reintroducing the defect.
- git_ops_guard.py's independently-drifted _DYNAMIC_TOKEN_RE (missing `*`
  relative to the shared regex) retired in favor of importing
  _DYNAMIC_SHELL_TOKEN_RE directly.
- docs/safety/hooks.md: document the two-route provenance rule (file-
  content-provable OR full-single-quote-provable) replacing the prior
  undocumented state.
- tests/arch/test_subpackage_isolation.py: bump _command_classification.py's
  REQ-CNST-010-E10 line-limit exemption 2450 -> 2700 for this addition.

Part C (extending this spec-table/consume-engine architecture to curl/git/
pip/write-guard bypass-direction bugs) follows in a subsequent commit per
the combined rectify plan's green-gate sequencing.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@Trecek @claude
fix(hooks): extend spec-table engine to git/curl/pip bypass-direction…
53f89af 
… bugs

Rectify Part C: a parallel investigation searching for recurrences of Issue
tables consulted by an argv-walking loop that doesn't consume an
unrecognized value flag's following token) found at least nine such tables
across the guard suite. Several fail in the opposite, unsafe direction from
subcommand/remote/no-pip-install-detected-at-all), not just a false deny.

Generalizes Part A's gh-api-specific engine into a CLI-agnostic
_consume_argv_flag (ArgvToken-based, for consumers needing quote-provenance)
plus a _consume_str_flag convenience wrapper (for consumers that only need
flag recognition/arity), and fixes each site according to what its own
consumer contract actually requires -- not a uniform drop-in:

- extract_git_subcommand_and_flags: now returns ("<unresolved>", []) on an
  unrecognized global git flag, distinguishable from None. All three
  callers updated for their own correct outcome:
  is_allowed_protected_path_metadata_command (already correct via its
  subcommand-allowlist check), git_ops_guard.py's _classify_git_segment
  (-> ambiguous-deny), and _contains_blocked_git_op (a new third call site
  created by deleting the duplicate _extract_git_subcommand_and_remaining
  -> immediate unconditional deny, bypassing _BLOCKED_GIT_OPS's tuple-match
  loop, which "<unresolved>" could otherwise silently fall through).
- _gh_args_have_bare_help_flag: NOT a blanket default-arity flip (an
  earlier attempt at exactly that broke a pre-existing test involving a
  boolean flag followed by a known value-taking flag followed by a
  gh-CLI help token as that flag's literal value -- a blind advance-2
  default for the unrecognized-but-boolean first flag jumped straight
  past the real, known-value second flag without evaluating it, so the
  help token was reached and wrongly trusted as bare, corrupting the scan
  and wrongly exempting a genuine review-publishing mutation). Scoped
  instead to exactly the ambiguous case: a help-flag token immediately
  preceded by an unrecognized flag is not trusted, everything else
  (including the known value-taking flag's own correct 2-token skip) is
  untouched.
- _find_pip_install (unsafe_install_guard.py): unrecognized global pip flag
  threaded as a new "unresolved-pip-flags" kind through
  _classify_install_invocation -> _iter_install_segments ->
  _is_unsafe_editable_install (unconditional deny), matching the
  pre-existing "unresolved-subprocess" kind's treatment -- not left as a
  bare None a caller could conflate with "not pip install".
- write_guard.py / core/bash_write_targets.py: NOT a fail-closed-sentinel
  fix (both guards' only consumers already treat an empty/None target list
  as allow, so a bolted-on sentinel would silently produce an allow, the
  opposite of intended). Parse-correctness fix instead: extend each
  module's independently-duplicated _GIT_FLAG_WITH_VALUE so the git-flag-
  skip loop reaches the real subcommand instead of stopping at an
  unrecognized flag's value.
- _analyze_curl_segment / git_ops_guard.py's _classify_fetch: mechanical
  rewire onto the generalized engine with a fail-closed default -- new
  _CURL_FLAG_SPEC/_GIT_FETCH_FLAG_SPEC tables built from live --help reads.
- unsafe_install_guard.py registered in FAIL_CLOSED_GUARD_BASENAMES;
  docs/safety/hooks.md and hooks/guards/AGENTS.md's Fail Modes tables and
  "Design principle" sentence updated for both new fail-closed guards.

tests/arch/test_subpackage_isolation.py: bump _command_classification.py's
REQ-CNST-010-E10 line-limit exemption 2700 -> 2950 for this addition.

Part D (test-suite-wide flag-spec coverage enforcement and live-CLI-help
drift detection) follows in a subsequent commit per the combined rectify
plan's green-gate sequencing.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@Trecek @claude
test: enforce CLI flag-spec table coverage, drift detection (rectify …
7dacc6a 
…Part D)

Rectify Part D (final part): fixes the meta-gap that let Parts A-C's bugs
ship untested in the first place -- no convention requiring a CLI-flag spec
table's every entry to actually be exercised by a test, and no mechanism to
catch a future CLI version adding a new flag before it silently
reintroduces this bug class.

- Four generative parametrized tests, one per spec table
  (_GH_API_FLAG_SPEC/_CURL_FLAG_SPEC/_GIT_GLOBAL_FLAG_SPEC/
  _PIP_GLOBAL_FLAG_SPEC), parametrized directly from each table's own keys
  rather than a hand-maintained flag list -- a flag added to a table later
  is automatically covered with no separate test-list edit, eliminating the
  drift risk a copied list would carry.
- tests/arch/test_guard_flag_spec_coverage.py: standing architectural test
  (mirrors test_fail_closed_guard_contract.py's AST-scan-and-batch-report
  shape) asserting every spec table has a live parametrize reference in its
  corresponding test file, not just a test that happens to exist today.
- tests/hooks/test_gh_api_flag_spec_contract.py generalized from Part A's
  gh-specific live-help-diff test into four CLI contract tests sharing one
  diff/assert core, each CLI's own help-line-parsing heuristic passed in
  separately since gh/curl/git/pip's --help formats are structurally
  different (a per-line FLAGS listing, a bracketed usage synopsis with
  nested-bracket optional-value syntax, a sectioned options list). gh/git/
  pip achieve full live-help containment; curl's contract test is scoped
  explicitly to the flags this rectify's investigation named plus curl's
  own already-covered flags (curl's ~250-flag surface is deliberately not
  exhaustively mirrored in _CURL_FLAG_SPEC -- an unrecognized flag now
  fails closed, so full coverage would trade availability for flags outside
  real-world GitHub-mutation-guard usage; the ~194-flag gap is surfaced
  non-fatally in test output rather than silently dropped).
- FLAG_FORM_MATRIX: consolidates five identical
  ids=["space", "equals", "attached-short"] tuples (Parts A/C's flag-form
  tests) into one shared constant.
- tests/AGENTS.md: documents the three-part convention (generative
  parametrized test + architectural coverage-table entry + live-CLI-help
  contract test) as required for any future flag-spec table or
  CLI-argv-consuming guard test.

This is the final part of the combined rectify plan
(rectify_github_mutation_guard_argv_parsing_immunity). Parts A-C supply the
spec tables and quote-provenance tokenizer this part audits and enforces
coverage for.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@Trecek @claude
test: remediate audit-impl REQ-065 GraphQL/flag-form matrix gap
de3f0d8 
Addresses the /audit-impl NO GO findings from
remediation_rectify_gh_mutation_guard_argv_2026-08-17_113537.md.

REQ-065 (fixed): FLAG_FORM_MATRIX was inline in test_command_classification.py
instead of the plan's reusable, shared location, and GRAPHQL_DELIVERY_MATRIX/
GRAPHQL_CONTENT_MATRIX were entirely absent.

- New tests/hooks/_flag_form_matrix.py, mirroring the existing
  tests/infra/_pretty_output_helpers.py private-helper-module convention:
  FLAG_FORM_MATRIX (relocated, unchanged 3-form set), GRAPHQL_DELIVERY_MATRIX,
  GRAPHQL_CONTENT_MATRIX, and shared GraphQL command builders.
- test_command_classification.py: imports the shared matrix instead of
  defining its own copy; adds a genuine 16-cell delivery x content
  parametrized test whose expected outcomes were hand-derived from
  _DYNAMIC_SHELL_TOKEN_RE / _is_dynamic_shell_value and confirmed against
  the live implementation via direct (non-pytest) execution before being
  written as assertions.
- test_github_mutation_guard.py: adds a 4-cell (delivery x event_factory)
  sampling test proving the guard's own decision wiring surfaces the
  classification layer's result, without duplicating the full matrix at
  the integration layer.
- tests/AGENTS.md: updated the FLAG_FORM_MATRIX location pointer and
  documented the new GraphQL matrices.

Also corrects one audit claim: tests/arch/test_guard_flag_spec_coverage.py
does not actually reference FLAG_FORM_MATRIX anywhere (grep-confirmed);
left that file untouched rather than adding a purposeless import to a file
whose actual scope (CLI flag-spec-table coverage) is unrelated to
flag-value-form matrices.

REQ-044 (disagreement filed, not implemented as specified): the plan asked
for _gh_args_have_bare_help_flag's default arity to change from "assume
boolean, advance 1" to "assume value-taking, advance 2" for any unrecognized
flag. Traced through concretely, this reintroduces a real mutation-guard
bypass on a two-hop input the plan's own safety argument does not cover
(an unrecognized boolean flag immediately followed by a real
_GH_KNOWN_VALUE_FLAGS entry whose value is help-flag-shaped) -- exactly the
shape the project's pre-existing regression test for detached help-flag
values already pins. Filed a full writeup at
.autoskillit/temp/audit-impl/accepted_deviations/req_044_narrower_help_flag_fix.md
and ran it through the project's own audit-impl-deviation-evaluator agent,
which independently re-executed both the plan's literal fix and the
implemented fix against the live code and returned ACCEPT.

Verification: pre-commit (ruff format, ruff check, mypy, uv lock --check,
gitleaks) passes clean. All 16+4 new matrix cells independently confirmed
via direct production-function calls outside pytest. `test_check` itself
could not complete a clean run at commit time -- 5/5 attempts failed with
an identical OwnedProcessCleanupError traced to
tests/execution/test_process_channel_b.py's
test_channel_b_timeout_remains_nonterminal, a file this branch does not
touch (last modified by an unrelated PR) whose own docstring documents
WSL2/xdist-load timing sensitivity; the host was at load average ~51-55 on
16 cores with ~22 concurrent autoskillit cook sessions during all 5
attempts. A clean test_check run is still owed before merge.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@Trecek
fix(hooks): strip trailing operator chars from shlex tokenizer spans
2b4ccf4 
Token-by-token span capture via marked[start:instream.tell()].rstrip()
does not strip the operator characters ;, |, & that shlex
shlex(punctuation_chars=";&|") leaves sitting inside the previous
token's source range. A fully-single-quoted token immediately
followed by an unseparated `;` would leak the operator into its
raw_span and fail the fully_single_quoted == "'<token>'" check,
causing legitimate GraphQL mutations to be wrongly denied as
dynamic_target -- directly undermining the AC2/AC3 fix this PR is
built around.

Strip the trailing operator chars (introduced as
_SHELL_OPERATOR_CHARS alongside _SHELL_OPERATORS) so the captured
span matches the comment's claim that it covers "exactly the
characters this lexer itself consumed to produce the token".

Targeted by review-pr finding: src/autoskillit/hooks/_command_classification.py:425 [bugs, critical].
@Trecek
fix(hooks): route gh api -H/--header/--hostname/--cache through spec …
5790ead 
…engine

_analyze_gh_api duplicated the space- and =-form skip logic for
-H/--header/--hostname/--cache even though every one of them is
already enumerated in _GH_API_FLAG_SPEC. The hand-roll duplicated
the spec-table engine for the same four flags, leaving
test_every_gh_api_spec_flag_is_recognized with a coverage illusion
for those entries.

Drop the hand-roll so the spec table is the sole authority for
gh api flag arity; the failing-missing-value path is preserved by
_consume_argv_flag's VALUE handling of those same flags.

Targeted by review-pr finding: src/autoskillit/hooks/_command_classification.py:2052 [arch, warning].
@Trecek
fix: replace stale _consume_gh_flag reference with _consume_argv_flag
2d96b7d 
The rectify renamed _consume_gh_flag to _consume_argv_flag, but four
references escaped the rename: a comment in _command_classification.py
introducing _GH_API_FLAG_SPEC, two adjacent paragraphs in
test_subpackage_isolation.py's _command_classification line-limit
exemption history, and a docstring in
test_command_classification.test_method_provenance_proof_single_quoted_resolves.

Update all four to refer to the real function so future readers can
trace the comment claim to the code it describes.

Targeted by review-pr findings: src/autoskillit/hooks/_command_classification.py:1685 [slop, warning];
tests/arch/test_subpackage_isolation.py:1157 and :1164 [slop, warning];
tests/hooks/test_command_classification.py:2061 [slop, warning].
@Trecek
test(hooks): cover _GIT_FETCH_FLAG_SPEC and harden gh api help-flag test
3639630 
The rectify introduced _GIT_FETCH_FLAG_SPEC (git fetch's own flag spec)
without registering it under the CLI Flag-Spec Coverage Convention
established earlier in this same PR (no entry in
_SPEC_TABLE_TEST_FILES, no generative parametrize test). Add both so
the standing-invariant scanner proves parametrize genuinely iterates
the table, and so a future flag added to the spec extends coverage
automatically.

test_every_gh_api_spec_flag_is_recognized was also vacuous for
-h/--help: _gh_args_have_bare_help_flag short-circuits
analyze_github_mutations before _analyze_gh_api is ever reached for
those two flags, so the assertion passed regardless of whether -h/
--help were actually wired into the spec engine. Restructure the
command for those flags so they are preceded by an unrecognized
`-`-prefixed token, causing the help short-circuit to skip them as
that token's value and forcing the spec engine path the test is
meant to exercise.

Targeted by review-pr findings: src/autoskillit/hooks/guards/git_ops_guard.py:156 [arch, warning];
tests/hooks/test_command_classification.py:2203 [tests, warning].
@Trecek
fix(hooks): drop unreachable cd_argv_args else-branch
cbb6118 
The `else []` arm of the cd_argv_args ternary can never fire: the
surrounding `if _normalize_executable(verb) == "cd"` is only entered
when verb is the literal "cd", and command_verb_and_args (which
returned verb) only returns a non-empty verb when its own
_verb_start_index call returned non-None on the same list. The
defensive `is not None` re-check duplicates that invariant.

Replace the ternary with an assertion followed by unconditional
slicing; the unreachable fallback was noise to every reader and a
hand-rolled alternative to a single-assertion invariant.

Targeted by review-pr finding: src/autoskillit/hooks/_command_classification.py:2721 [overengineering_reachability, warning].
@Trecek
docs(hooks): document _FlagArity semantics
a138718 
Every other spec-table enum, helper, and dataclass in this PR carries a
docstring describing what its members mean; _FlagArity (the enum keying
all 5 CLI flag-spec tables) is the lone exception. Add a 6-line
docstring documenting the BOOLEAN/VALUE distinction and noting that
joined-value forms like -XPOST/--flag=value are handled by the spec
consume engine, so readers do not have to trace _consume_argv_flag to
find out what each member is for.

Targeted by review-pr finding: src/autoskillit/hooks/_command_classification.py:1674 [cohesion, info].
@Trecek
refactor(hooks): reference _PIP_INSTALL_UNRESOLVED instead of bare li…
46ae435 
…teral

Four call sites in unsafe_install_guard (and one in its test file)
hardcoded the literal "unresolved-pip-flags" instead of using the
_PIP_INSTALL_UNRESOLVED module constant. The constant is the source
of truth (per its own header comment: distinguishable from a bare
None for the "ambiguous" case so callers cannot collapse the two);
hard-coding it in the four return statements and one consumer
comparison is duplication that silently desyncs if the sentinel
ever needs to change.

Route all five sites through the constant so the literal lives in
exactly one place.

Targeted by review-pr finding: src/autoskillit/hooks/guards/unsafe_install_guard.py:164 [cohesion, info].
@Trecek
refactor(hooks): derive write_guard._GIT_FLAG_WITH_VALUE from spec table
b94d1b9 
write_guard.py's _GIT_FLAG_WITH_VALUE frozenset was a hand-maintained
mirror of _GIT_GLOBAL_FLAG_SPEC's value-arity members, kept "in sync
manually" per its comment. This PR extends _GIT_GLOBAL_FLAG_SPEC with
two new members; hand-copying them again would let the mirror silently
drift if a future table extension forgets to edit the frozenset.

write_guard.py already imports ten other names from
_command_classification.py via the same sys.path mechanism
git_ops_guard.py uses, so the "kept in sync manually since hook
scripts import via sys.path" justification was inaccurate. Derive
_GIT_FLAG_WITH_VALUE at import time from the spec table directly,
making the frozenset a one-line projection of the source of truth.

Targeted by review-pr finding: src/autoskillit/hooks/guards/write_guard.py:86 [overengineering_abstraction_surface, warning].
@Trecek
test(hooks): add regression test for _contains_blocked_git_op <unreso…
e68de34 
…lved>

The destructive-op preflight in _contains_blocked_git_op correctly
wires the `<unresolved>` fail-closed sentinel at git_ops_guard.py:206,
but until now had no dedicated regression test distinct from
_classify_git_segment's checked-out-ref `<unresolved>` test
(test_unresolved_global_flag_before_subcommand_fails_closed_for_all_owners).
A future refactor that drops the short-circuit and falls through to
the _BLOCKED_GIT_OPS loop (where `<unresolved>` would silently
match nothing and the destructive op would slip through) would pass
all existing tests.

test_denies_push_force_with_unresolved_global_flag runs `git
--attr-source foo push --force`: `--attr-source` is a genuine git
option outside _GIT_GLOBAL_FLAG_SPEC, so extract_git_subcommand_and_flags
returns `<unresolved>`, exercising the short-circuit at line 206. The
guard's destructive-op denial reason (not the checked-out-ref JSON
used by TestCheckedOutRef*) verifies the right preflight handled it.

Targeted by review-pr finding: src/autoskillit/hooks/guards/git_ops_guard.py:206 [defense, warning].
@Trecek
test(hooks): pin AC2/AC3 single-quoted GraphQL + unseparated operator
3e8b7e7 
The post-validator for finding #1 (critical shlex span bug) flagged
that no regression test pins the specific case the strip fix enables:
a fully single-quoted GraphQL document immediately followed by an
unseparated shell operator (`;`, `|`, or `&`). Without it, the
strip's correctness is verified only by the AC2/AC3 single-quoted
test (whitespace-separated) and a future regression that swaps the
strip-set or removes it entirely would slip through the test suite.

parametrize over `;`, `|`, `&` so each punctuation_chars value the
shlex lexer bundles into the previous token's source range has a
test case asserting the legitimate mutation still resolves to
SINGLE_RESOLVED. Mirrors the proven failure mode the strip
addresses.
@Trecek
fix(hooks): restore missing_value detection in spec-engine call sites
16c32c3 
The post-validator for finding #2 (gh api flag engine dedup, commit
6a7d83b) caught a real regression the original fix introduced: by
deleting the hand-rolled `-H/--header/--hostname/--cache` skip logic
the call site lost the explicit missing_required_value return for
those four flags. The spec engine's `_consume_argv_flag` returns
`(None, i+1, True)` for VALUE-arity with no next token, which is
indistinguishable from BOOLEAN — so `gh api -H` (no value) silently
passed as recognized-with-no-value rather than surfacing as
missing_required_value.

The same architectural gap exists in `_analyze_curl_segment` for any
VALUE-arity curl flag not in the special-cased data_flags/value_flags
tuples (`--user-agent`, `--proxy`, `--cacert`, `--connect-timeout`,
...).

Re-check the spec table at the call site: when `_consume_argv_flag`
returns recognized=True with value=None, look up the token in
`_GH_API_FLAG_SPEC` / `_CURL_FLAG_SPEC` and distinguish VALUE-arity
(no next token) from BOOLEAN via a second check, then return
missing_required_value for the VALUE case. Symmetric restoration in
both helpers; tests pin the regression for `-H`/`--header`/
`--hostname`/`--cache` on the gh side and `--user-agent`/`--proxy`/
`--cacert`/`--connect-timeout` on the curl side.
@Trecek
Trecek force-pushed the impl-rectify-gh-mutation-guard-argv-20260817-090109 branch from 8e176c5 to 16c32c3 Compare August 19, 2026 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@Trecek