Repository navigation
fix(action): pass --repo before the check subcommand - #109
Merged
Merged
Conversation
`--repo` is a top-level flag on the forerunner parser, not an argument of the `check` subcommand, so `forerunner check --repo <path>` exited 2 on argparse before any drift check ran. With `fail-on-drift: "true"` the job failed every run and reported it as documentation drift; with `"false"` the trailing `|| true` swallowed exit 2 and the job went green having checked nothing. Add a regression test that extracts the action's invocations and parses them with the real CLI parser, since asserting a non-zero exit alone cannot tell an argparse failure (2) from detected drift (1). Fixes #108
Follow-ups from #108. Only `mcp-server` redefined `--repo` as its own subcommand flag; every other subcommand required the top-level position. That asymmetry is the trap the action's broken invocation grew out of, so share one parent parser and give every subcommand the flag. Both orders now parse; the subcommand's value wins when given on both sides. The action's `|| true` under `fail-on-drift: false` swallowed every non-zero exit, so a check that failed to run reported green. Run the CLI once, capture the code, and downgrade only exit 1 (drift found) to a warning; anything else still fails the job. Cover both with a drifting fixture repo: an end-to-end run asserting exit 1 and the violation text, a table pinning the step's exit-code mapping against a CLI stub, and a CI job running the local composite action ungated (must pass) then gated (must fail) — the pair distinguishes exit 1 from exit 2, which neither step can read on its own. Refs #108
6 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #108.
The bug
action.ymlinvokedforerunner check --repo <path>, but--repois a top-level flag, not an argument of thechecksubcommand. Argparse exited 2 before any drift check ran, so every run of the action failed on argument parsing:fail-on-drift: "true"— job failed 100% of the time, reported as documentation drift. Nothing was checked.fail-on-drift: "false"— the trailing|| trueswallowed exit 2 and the job went green. Also nothing was checked.Changes
action.yml—forerunner --repo "<path>" check, invoked once, branching on the exit code. Only exit 1 (drift found) is downgraded to a::warning::underfail-on-drift: false; every other non-zero code fails the job, so a check that fails to run can never report green again.--repoon every subcommand (cli.py,installer.py) — a sharedparents=parser givesinit,scan,doc,check,doctor,refreshandinstallthe flag. Previously onlymcp-serverredefined it as its own flag, and that asymmetry is plausibly where the action's invocation shape came from. Both orders now parse; the subcommand's value wins when given on both sides.mcp-serverkeeps its own declaration — its help text differs, and a shared parent would conflict.No change to
cmd_check's exit codes. The issue floated making them distinguishable from an argparse failure; they already are —0clean,1drift,2invalid config. Argparse also uses 2, but both 2s mean "failed to run", which is the correct grouping. The distinction that matters (drift vs. everything else) held all along and is now pinned by tests rather than left implicit.Tests
Asserting a non-zero exit would not have caught this — exit 2 is non-zero too. Every assertion here separates "found drift" from "failed to run".
tests/fixtures/drifting-repo/— a config enabling exactly one rule (RI1-missing-cli) and a README that documentsforerunner checkwhile the fixture ships no CLI. Guaranteed one violation. Not a scanned doc path, so this repo's ownforerunner checkstill exits 0.tests/test_action_yml.py— extracts the step's real argv and: parses it with the live CLI parser; asserts--repoprecedes the subcommand; runs it end-to-end against the fixture asserting exit 1 andRI1-missing-clion stderr; and pins the step's shell logic across all six (exit code xfail-on-drift) combinations using aforerunnerstub.tests/test_cli.py— both flag orders parse for every subcommand, later-flag-wins, andmain()'sargs.repo = Nonebackfill.ci.ymlaction-smokejob — runs the local composite action against the fixture, ungated (must pass) then gated (must fail). A composite step exposes only pass/fail, never the exit code, so no single run can tell 1 from 2; the pair can, because exit 2 fails the ungated step too.Mutation-checked both guards: restoring the
|| trueswallow fails the[2-false-2]case, restoring the old flag order fails the ordering test.324 tests pass.
scripts/check_versions.pyclean.Note for consumers
The action installs the released CLI from PyPI, so the smoke job exercises
action.yml's wiring against a realforerunner— which is the bug class here. Consuming repos still on a broken tag can keep calling the CLI directly (forerunner checkfrom the workspace root) until this ships.