Repository navigation
fix: clarify no-workflows audit warnings (#584) - #685
JohnsonLyu wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Careful implementation: step type with a {fail} ceiling, first step of the three controls, symlink escapes handled, action.yml only where zizmor reads it, spec table and TOML updated, good unit coverage. Three things:
- This is the first handler that returns WARN as a concluding, dispositive result, and the corpus invariant
test_warn_outcome_changes_no_existing_control(SC-008 / FR-017: no handler returns WARN) is relaxed to tolerate it. That's a framework decision rather than a bug fix and should be stated in the description; a concluded WARN also stops the control before its manual step, where it used to fall through. @mlieberman85, your call on that. - Needs a rebase, and coordination with #656 on the BR-01.02 grep step.
Re-review after.
mlieberman85
left a comment
There was a problem hiding this comment.
Thanks @JohnsonLyu. On Marco's first point, the framework decision: I'm fine with this being the first step that concludes WARN.
framework-design 3.0 has allowed a concluding WARN under a {fail} ceiling since feature 037: "the handler read the evidence, understood it, and determined it is insufficient to pass". It also says a handler must not return WARN to mean "I am unsure". "No workflow files, nothing to audit" is a determined answer, so it fits. It can't conclude PASS or FAIL, it counts as FAIL for compliance either way, and the message tells the person what to do next. The corpus guard (test_warn_outcome_changes_no_existing_control) was 037's check that adding the outcome changed nothing. Narrowing it to these three controls, this message and fixtures with nothing to audit is the right way to retire that assumption.
I'm also OK with the concluded WARN skipping the manual step on a repository with nothing to audit. The message covers what the manual step would have said.
Before this can merge:
- Please say in the description that this is the first in-tree step to conclude WARN, that it narrows the SC-008 / FR-017 guard, and that the manual step no longer runs on a repository without workflows. Reviewers and the changelog should see the precedent without reading the tests.
- Rebase onto main and coordinate with #656, which adds
fail_exit_codes = [0]to the BR-01.02 grep step. - We need a recorded audit of a real repository without workflows, before and after, since this changes audit results. I can run it after the rebase.
Drafted with AI assistance (Claude), reviewed and filed by me.
Assisted-by: Cursor:Claude-Opus-5.5 Signed-off-by: Johnson Lyu <cl7084@nyu.edu>
Assisted-by: Cursor:Claude-Opus-5.5 Signed-off-by: Johnson Lyu <cl7084@nyu.edu>
a2d848f to
b5937c4
Compare
Assisted-by: Cursor:Claude-Opus-5.5 Signed-off-by: Johnson Lyu <cl7084@nyu.edu>
Marc-cn
left a comment
There was a problem hiding this comment.
Re-reviewed on b5937c4. Rebased on current main; the BR-01.02 grep step keeps #656's fail_exit_codes = [0] with github_workflows_present in front of it. The SC-008 guard is narrowed exactly as far as it needs to be: the three controls, the exact "nothing to audit" message, fixtures with no workflow and no action.yml, with tests for both tolerated and rejected drift. On a repository with no workflows the three controls now read a concluded WARN with the reason, where main gave the #584 unexpected_exit errors. Tests, validate_sync and the corpus gate pass locally.
Still open from Mike's list: the description should say this is the first in-tree step to conclude WARN, that it narrows the SC-008 / FR-017 guard, and that the manual step no longer runs on a repository without workflows — please add that. Mike's before/after audit on a real repository is his to run.
|
Thanks @Marc-cn and @mlieberman85 for the reviews! I've updated the PR description and CHANGELOG to clarify the concluding WARN semantics, the narrowed SC-008 / FR-017 guard, and the manual-step behavior. The rebase and #656 integration are complete, and all CI checks are passing. @mlieberman85, the PR is ready for the before/after audit on a real repository without workflows, as discussed. Thanks! |
Marc-cn
left a comment
There was a problem hiding this comment.
Re-approved on d268738: the only change since b5937c4 is the CHANGELOG, which now states the precedent (first in-tree step to conclude WARN, manual step skipped when there is nothing to audit, SC-008/FR-017 guard narrowed to these three controls). Mike's before/after audit on a real repository is the remaining item, then it can merge.
mlieberman85
left a comment
There was a problem hiding this comment.
Here's the before/after audit I said I'd run, on d268738 merged into main at 155dcc1. It changes the wording as intended and no status.
kelseyhightower/nocode has no workflows and no other CI, so it reaches the new step:
| control | main | this PR |
|---|---|---|
| OSPS-BR-01.01 | WARN, unexpected_exit, by none |
WARN, no error class, by github_workflows_present |
| OSPS-BR-01.02 | WARN, unexpected_exit, by none |
WARN, no error class, by github_workflows_present |
| OSPS-AC-04.02 | WARN, unexpected_exit, by none |
WARN, no error class, by github_workflows_present |
No other control's status, details, concluded_by or error class changes, and the status counts are identical. With zizmor 1.30.1, main's three stderr warnings about exit codes 3 and 2 are gone on this PR.
Repositories that shouldn't change didn't:
sindresorhus/slashhas workflows: no differences.jonschlinkert/is-numberandjonschlinkert/is-oddhave.travis.yml, so these controls are N/A: no differences.mafintosh/pumpandfeross/safe-bufferalso have.travis.yml: no differences in those fields.
The full suite passes on the merged tree (5359 passed, 11 skipped), and so do ruff and validate_sync.
It now conflicts with main in CHANGELOG.md only, since #684 and #670 merged. Could you rebase? The rebase will dismiss the approvals, and we'll re-approve once only the CHANGELOG has changed.
Drafted with AI assistance (Claude), reviewed and filed by me. Measurements quoted were executed and verified.
Summary
Fixes #584.
Improves audit warnings for repositories without GitHub Actions workflow files.
github_workflows_presenthandler to detect missing workflow files before running external tools.OSPS-BR-01.01,OSPS-BR-01.02, andOSPS-AC-04.02to report a descriptive WARN instead of misleadingunexpected_exitwarnings when there is nothing to audit.action.ymlandaction.yamlfiles inOSPS-BR-01.01, ensuring template-injection findings are not suppressed.{fail}ceiling. The SC-008 / FR-017 regression guard is narrowly adjusted to permit this outcome only for the three affected controls and fixtures with no relevant auditable inputs. Since this WARN is conclusive, the manual step no longer runs when there is nothing to audit; the descriptive message provides the operator guidance instead. The result remains non-compliant.Type of Change
Framework Changes Checklist
If this PR modifies the darnit framework (
packages/darnit/):docs/architecture/framework-design.md) if behavior changeduv run python scripts/validate_sync.py --verboseand it passesControl/TOML Changes Checklist
If this PR modifies controls or TOML configuration:
Testing
uv run pytest tests/ -v)uv run ruff check .)Validation results:
uv run ruff check .: passed.uv run python scripts/validate_sync.py --verbose: passed.git diff --check: passed.AI assistance
Cursor with Claude Opus 5.5 assisted with code investigation, implementation, regression tests, documentation updates, and test execution. Changes and results were reviewed before submission.
Additional Notes
unexpected_exitclassification introduced in exec steps label any unexpected exit code error_class=network #562 remains unchanged.OSPS-BR-01.01.OSPS-BR-01.02retainspass_exit_codes = [1],fail_exit_codes = [0], andconcludes = []. The No-workflows repositories: three controls report unexpected_exit instead of saying there is nothing to audit #584 workflow guard remains the first step.