Add verify and min_severity inputs to action.yml #108
No reviewers
Labels
No labels
Compat/Breaking
Kind/Bug
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
No milestone
No assignees
3 participants
Notifications
Due date
No due date set.
Reference
marvin8/wuming!108
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/issue-107-actionyml-wave1-inputs"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Wave 1 added
WUMING_VERIFY(#102) andWUMING_MIN_SEVERITY(#104) butaction.ymlwas never updated, so repos consuming WuMing as a Forgejo/GitHub Action silently ran both features at their defaults — the settings were unreachable from the action interface.verifyinput (default"true", matching the env default) mapped toWUMING_VERIFYmin_severityinput (default""= no floor) mapped toWUMING_MIN_SEVERITYtests/test_action_inputs.py: asserts everyWUMING_*env var config.py reads has an action.yml input, every mapping resolves to a declared input, no orphan mappings, no unused inputs (fails against pre-fixmain, passes here)action.ymladded to the must-update file list with the same-PR ruleCloses #107
Part of wave 2 (convergence): #79, #85, #84 follow.
tto Add verify and min_severity inputs to action.ymlWuMing
Found 4 issue(s). See inline comments below.
@ -6,3 +6,4 @@- Findings are now fact-checked against the changed file's full content before they are posted — claims the file contradicts are dropped automatically (`WUMING_VERIFY=false` to disable).- Repos can now set a minimum severity for posted findings (`WUMING_MIN_SEVERITY`): anything below the floor — and unrecognised severity values — are dropped before posting; agent prompts now also treat a clean report as the preferred outcome.- Wave-1 settings are reachable from the Forgejo/GitHub Action interface again: `action.yml` now has `verify` and `min_severity` inputs (previously they silently fell back to defaults).docs [LOW]
The word "again" is inaccurate and will mislead readers: per this PR's own description the
verify/min_severitysettings were never reachable from the action interface (action.yml was never updated, so there was nothing to restore). Reword to drop "again" — e.g. "Wave-1 settings are now reachable from the Forgejo/GitHub Action interface:". While here, consider replacing the internal project jargon "Wave-1 settings" with user-facing wording such as "the two review-quality settings from the previous release", matching the phrasing used in the follow-on post at line 37.@ -100,0 +110,4 @@Unset by default — everything posts. Invalid values fail closed with astartup error.required: falsedefault: ""config [MEDIUM]
The default
""is exported asWUMING_MIN_SEVERITY: ${{ inputs.min_severity }}on line 141, so for a default run the env var is present but empty rather than truly unset (contrary to the description on lines 110-111 and the documented 'unset = everything posts' contract). If config.py treats the presence of WUMING_MIN_SEVERITY as 'a value was supplied' and validates it against low|medium|high|critical, the empty string will fail closed with a startup error, breaking every consumer that does not set the input. Fix by either omitting the env mapping when the input is empty, defaulting the input to a sentinel that config.py maps to "no floor", or ensuring config.py treats empty/whitespace as unset.@ -0,0 +16,4 @@_REPO_ROOT = Path(__file__).resolve().parent.parent_ENV_READ_PATTERN = r'os\.environ\.get\(\s*"(WUMING_[A-Z0-9_]+)"'_INPUT_REFERENCE_PATTERN = r"\$\{\{\s*inputs\.([A-Za-z0-9_]+)\s*\}\}"code [MEDIUM]
The input-name regex [A-Za-z0-9_]+ does not allow hyphens, but GitHub Actions input identifiers can contain '-' (along with letters, numbers, and ''). If action.yml ever maps a hyphenated input such as '${{ inputs.my-input }}', re.findall will extract only the prefix before the hyphen, causing test_env_mappings_reference_declared_inputs and test_all_inputs_are_used to fail or pass incorrectly. Widen the capture group to [A-Za-z0-9-]+ so valid hyphenated input names are parsed correctly.
@ -0,0 +64,4 @@env_mappings = _action_env_mappings()config_env_vars = _config_env_vars()for env_var in sorted(config_env_vars - _INTENTIONALLY_UNMAPPED):expect(env_mappings).to_contain(env_var)tests [MEDIUM]
This assertion (like the one in test_env_mappings_are_read_by_config at line 83) only runs inside a loop over a scraped set. If
_config_env_vars()returns an empty set — e.g. config.py is later refactored to read env vars via a helper so_ENV_READ_PATTERNno longer matches — the loop body never executes and the test passes vacuously, silently defeating the exact drift regression it was written to catch. Add a non-emptiness guard before the loop (e.g.expect(config_env_vars).to_contain("WUMING_VERIFY")or assert the set has the expected length) so a broken scrape fails loudly instead of passing.New commits pushed, approval review dismissed automatically according to repository settings
Wave-1 review responses (commit
63257d1)✅ Fixed — input-name regex rejects hyphenated identifiers
tests/test_action_inputs.pyline 19Widened to
[A-Za-z0-9_-]+— valid for both Forgejo and GitHub Actions input names.✅ Fixed — vacuous-pass risk in the scrape loops
tests/test_action_inputs.pyline 67Good catch, and exactly the failure mode this test file exists to prevent. All four tests now open with sentinel expectations (
WUMING_VERIFY/WUMING_BACKENDon the config scrape,WUMING_FORGEJO_TOKENon the env mapping,verifyon the declared inputs), so a broken scrape fails loudly instead of passing vacuously.🔴 Not actioned — empty
WUMING_MIN_SEVERITYfailing closedaction.ymlline 113The conditional premise doesn't hold:
_parse_min_severity()returnsNonefor unset and empty values — the empty string never reaches validation (src/wuming/config.pylines 102–117, docstring: "orNonewhen unset/empty (no floor)").tests/test_config.py::test_min_severity_unsetpins exactly this contract. Present-but-empty is also the standard action.yml pattern every other optional input in this file uses (dry_run,verbose,summary_comment— all default""/"false"and rely on the parser's unset handling), and anenv:block cannot conditionally omit a mapping. So the default run behaves as documented: unset = everything posts.✅ Fixed — "again" in the release-note bullet
Release-Notes.mdline 9Reworded to drop "again" and the internal "Wave-1" jargon: "The review-quality settings are now settable from the Forgejo/GitHub Action interface…"
WuMing
Found 2 issue(s). See inline comments below.
@ -33,6 +34,10 @@ The pass is cautious in both directions: it only judges whether a finding is fac---A small repair for anyone running WuMing as a Forgejo or GitHub Action: the two review-quality settings from the last release — the fact-checking pass and the severity floor — existed only as environment variables, so the action wrapper quietly ignored them and ran with defaults. Both now have proper inputs (`verify` and `min_severity`), so you can set them directly in your workflow file. Anyone already setting the environment variables by hand is unaffected.docs [MEDIUM]
Inconsistency with the rest of the release notes. The version header at line 5 was changed to
wuming 0.12.0, and the two settings' own bullets (lines 7–8,WUMING_VERIFYandWUMING_MIN_SEVERITY) sit under that same 0.12.0 header — so the notes present the fact-checking pass and severity floor as part of this release. Calling them 'the two review-quality settings from the last release' in the new follow-on post contradicts that and will mislead readers about which release introduced them. Either reword to 'from this release' / 'introduced alongside this release', or, if they really shipped in a prior release, keep their bullets under the prior version's header and revert/qualify the version bump. Make the version header and the post agree.@ -0,0 +55,4 @@Returns:Every WUMING_* variable name passed to ``os.environ.get`` in config.py."""config_source = (_REPO_ROOT / "src" / "wuming" / "config.py").read_text()code [MEDIUM]
config_env_vars scrapes env vars from config.py with a regex. This is fragile and undermines the test's purpose of preventing drift: it will miss semantically equivalent reads such as os.getenv('WUMING...') or os.environ['WUMING_...'], and it can produce false positives from comments or string literals. Prefer parsing config.py with the ast module to collect the actual arguments of os.environ.get / os.getenv calls, which also handles single vs double quotes automatically.
Wave-2 review responses (commit
5565c03)✅ Fixed — regex scrape of config.py replaced with ast parsing
tests/test_action_inputs.pyline 67Agreed — the sentinel guards catch a broken scrape loudly, but an extractor that understands the code beats one that needs a sentinel to notice it's broken.
_config_env_vars()now walks config.py's AST and collectsWUMING_*literals fromos.environ.get(...),os.getenv(...), andos.environ[...]reads. Verified it returns the identical 24-variable set the regex found before the rewrite.Separately, the CI failure that motivated this push: the nox
trykesessions install only runtime deps +tryke(uv sync --no-default-groups), sopyyaml— needed to parseaction.yml— was missing there. Thetryke/tryke_betassessions now installpyyamlalongsidetryke.✅ Fixed — "last release" contradicts the 0.12.0 header
Release-Notes.mdline 9Correct — the bullets live under the 0.12.0 header, so "from the last release" was wrong. The follow-on post now reads "the fact-checking pass and the severity floor introduced in this release originally exposed only their environment variables…"
WuMing
Found 2 issue(s). See inline comments below.
@ -0,0 +135,4 @@return env_vars@test("every WUMING_ env var config.py reads has an action.yml input")code [MEDIUM]
The parity tests verify that every env var has an input and vice versa, but they do not check that action.yml default values match the defaults used in config.py (e.g. verify defaulting to 'true' and min_severity defaulting to ''). A mismatch would silently change behavior for Action users, which is the same class of drift this regression test is meant to prevent. Add a test that compares the default in each action.yml input with the literal default passed to the corresponding os.environ.get/os.getenv call in config.py.
@ -0,0 +139,4 @@def test_env_vars_have_inputs() -> None:env_mappings = _action_env_mappings()config_env_vars = _config_env_vars()# Guard against vacuous passes: if the scrape ever breaks (e.g. config.pycode [LOW]
The sentinel comment refers to '_ENV_READ_PATTERN', but the actual scraper uses AST helper functions rather than a regex with that name. Update the comment to match the implementation so it does not confuse future maintainers.
Wave-3 review responses (commit
3eaf1b7)✅ Fixed — input defaults now parity-checked against config.py
tests/test_action_inputs.pyline 193Agreed — defaults are part of the contract. New
test_input_defaults_match_configextracts eachWUMING_*literal default from config.py'sos.environ.getcalls (same ast approach as the scraper) and compares it against the mapped input's declareddefault:. Parser-mediated defaults (the four booleans, plusWUMING_FORGEJO_URL, whose default is deliberately CI-context-dependent: Woodpecker pins the zone URL, Actions auto-detects fromGITHUB_SERVER_URL) are documented in an explicit_DEFAULT_EQUIVALENCEStable; everything else must match literally. The test already earned its keep: it caught that the Forgejo-URL default needed its context dependence written down.✅ Fixed — stale
_ENV_READ_PATTERNreference in the sentinel commenttests/test_action_inputs.pyline 67The comment now describes the real failure mode: "config.py stops reading env vars through the call patterns the scraper understands".
WuMing
Found 3 issue(s). See inline comments below.
@ -0,0 +181,4 @@for node in ast.walk(tree):if not isinstance(node, ast.Call):continueif _dotted_name(node.func) != "os.environ.get" or not node.args:code [MEDIUM]
The defaults scraper only handles
os.environ.get, but_wuming_name_from_calland_config_env_varsalso acceptos.getenv. If config.py reads aWUMING_*variable viaos.getenv(..., default=...), this test will record an empty default instead of the real one and can miss a true mismatch. Change the check to_dotted_name(node.func) in {"os.environ.get", "os.getenv"}and extract the second positional argument for both calls.@ -0,0 +195,4 @@and isinstance(node.args[1], ast.Constant)and isinstance(node.args[1].value, str)):defaults[first_argument.value] = node.args[1].valuecode [MEDIUM]
The scraper only looks at
node.args[1]for the default value. A call likeos.environ.get("WUMING_X", default="...")will be recorded as having an empty default, which can cause false mismatches intest_input_defaults_match_config. Inspectnode.keywordsfor a keyword argument nameddefaultas well.@ -0,0 +240,4 @@mismatches: list[str] = []for env_var, target in _action_env_mappings().items():referenced_inputs = re.findall(_INPUT_REFERENCE_PATTERN, str(target))input_default = str(code [LOW]
If action.yml declares
default:with no value (YAML null) or omitsdefault,yaml.safe_loadreturnsNoneandstr(None)becomes"None". In GitHub Actions an unset default is effectively the empty string, so the comparison should treatNoneas"". Coerce withinput_default = str(input_default or "").Wave-4 review responses (commit
1ce18f6)✅ Fixed — defaults collector now covers
os.getenvtests/test_action_inputs.pyRight — the reader and the defaults extractor now accept the same call set.
✅ Fixed — keyword
default=arguments are extractedtests/test_action_inputs.pyA
_default_argumenthelper now returns the second positional argument or thedefaultkeyword value. Verified against a synthetic module covering all four shapes: positional default, keyword default, no default, and a non-string default — each lands on the right value.✅ Fixed — YAML null defaults coerced to empty string
tests/test_action_inputs.pyNonenow coerces to""before the comparison, matching Actions' treatment of an unset default.WuMing
Found 1 issue(s). See inline comments below.
@ -124,2 +138,4 @@WUMING_VERBOSE: ${{ inputs.verbose }}WUMING_SUMMARY_COMMENT: ${{ inputs.summary_comment }}WUMING_VERIFY: ${{ inputs.verify }}WUMING_MIN_SEVERITY: ${{ inputs.min_severity }}config [MEDIUM]
WUMING_MIN_SEVERITYis always exported, but with an empty string rather than being unset. The description for this input (lines 109-111) promises "Unset by default — everything posts. Invalid values fail closed with a startup error," yet an empty value is present, not absent. If config.py distinguishesos.environ.get("WUMING_MIN_SEVERITY")/Nonefrom""when validating againstlow|medium|high|critical, the shipped default will trip the fail-closed startup error for every consumer of the action. Either make config.py treat an empty (or whitespace-only) value as "no floor" explicitly, or drop thedefault: ""from this input and guard the mapping (e.g. only export the variable when a value was supplied) so the variable is genuinely unset by default. Add a test asserting the default action invocation exits cleanly.Duplicate of a wave-1 finding (already declined)
action.ymlline 113Same substance as the wave-1 finding declined in the consolidated wave-1 reply: the premise that config.py might distinguish
Nonefrom""is false —_parse_min_severity()returnsNonefor both unset and empty (src/wuming/config.pylines 102–117), pinned bytests/test_config.py::test_min_severity_unset. Present-but-empty remains the standard action.yml contract for optional inputs. No further action.