Skip to content

fix(workflow-audit): report commits that only touch .github/audit/ - #450

Closed
dormouse-bot wants to merge 2 commits into
mainfrom
fix/workflow-audit-audit-prompt-attribution
Closed

fix(workflow-audit): report commits that only touch .github/audit/#450
dormouse-bot wants to merge 2 commits into
mainfrom
fix/workflow-audit-audit-prompt-attribution

Conversation

@dormouse-bot

@dormouse-bot dormouse-bot commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator

.github/audit/ sits in workflow-audit.yaml's commit-selection window but not in its attribution window, and the second one decides what gets reported — so a commit touching only the audit prompts is selected, produces no attributable files, and vanishes without even a skipped-line. #449's first commit is exactly that shape. The fix adds the path to both own_changes() arms and to both classifier guards, which is the pairing .config/tend.yaml already has.

What broke

  • Selection includes it: git log --all --since="$SINCE" ... -- .github/workflows/ .config/tend.yaml .github/audit/.
  • own_changes() pathspecs it out — .github/workflows/ .config/tend.yaml in both the single-parent arm and the merge-intersection arm.
  • The report loop does FILES=$(own_changes "$sha") and continues on empty before writing a $SKIPPED line, so the commit is not reported and not listed as skipped either. The comment above the selection window asserts the opposite: "anything landing here is reported on its own content, which is the intent."

It fails the other way for mixed commits. is_tend_regen reads git show --name-only ... -- .github/workflows/, requires every entry to match tend-*.yaml, and requires uvx tend init to leave .github/workflows/ clean — it never looks at .github/audit/. A commit that edits tend-review.yaml and an audit prompt is therefore classified "reproduces from the tend generator" and skipped, with the prompt edit riding along unexamined. is_renovate_pin_bump has the same hole. That is the swallow the .config/tend.yaml comment names as the thing to avoid: "Both classifiers refuse any commit that touches the config, so nothing in the widened window can be swallowed by an arm that doesn't inspect it — that pairing is the invariant, not either half." .github/audit/ was widened into the window with neither half.

Verification

Before/after on a real audit-prompt-only commit

5b7ea046 (the first commit of #449) touches .github/audit/application-security.md, .github/audit/ci-and-secrets.md, and SECURITY.md, and nothing else.

$ git show --name-only --pretty='' 5b7ea046 -- .github/workflows/ .config/tend.yaml
                                                    # old own_changes() → empty → dropped

$ git show --name-only --pretty='' 5b7ea046 -- .github/workflows/ .config/tend.yaml .github/audit/
.github/audit/application-security.md               # new own_changes() → reported
.github/audit/ci-and-secrets.md

$ git log --all --since='2026-08-25' --pretty=format:'%H' \
    -- .github/workflows/ .config/tend.yaml .github/audit/ | grep -c 5b7ea046
1                                                   # selected either way

SECURITY.md is correctly absent from both: it is outside the window by design, and the audit reports the security automation's own inputs, not the spec they check against.

The three pathspec lists (selection, and both own_changes() arms) are now identical, and both classifiers refuse any commit touching .config/tend.yaml or .github/audit/. Comment changes record why each guard exists, and the header block now says a path added to the window must be added in all four places or widening the window is what hides the commit.

Found while reviewing #449; it is a workflow logic change rather than a scope enumeration, so it is separate from that PR. Once this merges, a re-run reports #449's commits on their own content, as intended.

Follow-up commit

4046247e fixes the same enumeration in the audit's two human-facing strings, which the logic fix made wrong: the created issue's first line and the empty-window message each named .github/workflows/ and .config/tend.yaml only. Harmless while audit-prompt-only commits were dropped before reporting — wrong the moment they are reported, since the header would name paths the commit did not touch while the Files list under it shows .github/audit/*.md.

SECURITY.md's "Audit visibility" paragraph has the same two-of-three drift and is left for its own PR: it describes the window, which has held three paths since this workflow was created, so it is independently revertable from this change.

.github/audit/ was added to the commit-selection window but not to
own_changes(), which is what decides whether a selected commit is
reported. A commit touching only the audit prompts entered $COMMITS,
yielded an empty FILES, and was dropped by the empty-FILES continue
without even a $SKIPPED line — the exact opposite of what the comment
above the window asserts.

It failed the other way too: is_tend_regen and is_renovate_pin_bump
both guard .config/tend.yaml but neither looked at .github/audit/, so a
commit editing tend-review.yaml and an audit prompt together was
classified 'reproduces from the tend generator' and skipped with the
prompt edit unexamined.

Add the path to both own_changes() arms and to both classifier guards —
the same pairing .config/tend.yaml already has, which the comments name
as the invariant.
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Aug 26, 2026

Copy link
Copy Markdown

Deploying mouseterm with  Cloudflare Pages  Cloudflare Pages

Latest commit: 4046247
Status: ✅  Deploy successful!
Preview URL: https://7903f5e3.mouseterm.pages.dev
Branch Preview URL: https://fix-workflow-audit-audit-pro.mouseterm.pages.dev

View logs

@dormouse-bot dormouse-bot left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified the premise and the fix. 59b0cedf created this file with .github/audit/ already in the git log selection but absent from both own_changes() arms, so an audit-prompt-only commit is selected into $COMMITS, yields an empty FILES, and hits the continue above is_renovate_pin_bump — no report, no $SKIPPED line. The three pathspec lists are now identical and both classifier guards refuse the path, which closes the mixed-commit swallow too.

Two things the diff didn't reach, both in the audit's human-facing output. Neither line is inside a diff hunk, so no inline suggestion — I'm pushing them as a follow-up commit on this branch since there's no other author to act on them.

The created issue misnames the scope on exactly the commits this PR adds. The body's first line — echo "$COUNT unexplained commit(s) touching \.github/workflows/` or `.config/tend.yaml` since `$SINCE`."— enumerates two of the three window paths. Before this PR that was harmless, because an audit-only commit never reached the report; after it,5b7ea04files an issue whose header says it touched.github/workflows/or.config/tend.yamlwhile the **Files** list under it shows only.github/audit/*.md`. The one artifact a human reads misstates why the commit is in front of them.

Same omission in the empty-window message, echo "No workflow or tend-config changes since $SINCE.". The condition is correct ($COMMITS empty means no change in any of the three paths); only the wording is two-of-three.

Two observations I'm not pushing:

  • SECURITY.md's "Audit visibility" paragraph has the same drift, and it is the prose spec of this workflow: "walks every commit touching .github/workflows/ or .config/tend.yaml" — no .github/audit/, even though a FAIL IF further down ("FAIL IF .github/audit/ is outside workflow-audit.yaml's diff window") asserts the path belongs there. A reader checking that FAIL IF against the paragraph gets a contradiction. It's independently revertable from this change, so it wants its own PR rather than widening this one; #449 touches SECURITY.md but not this paragraph.
  • The invariant this PR adds is comment-enforced, and comments are what just drifted. The header now says a path added to the window "must also be added to own_changes() and refused by both classifiers" — three literal copies of the same list, plus two copies of the window-minus-.github/workflows/ list, kept in sync by prose. WINDOW=(.github/workflows/ .config/tend.yaml .github/audit/) and UNINSPECTED=("${WINDOW[@]:1}") expanded as -- "${WINDOW[@]}" would make the drift unrepresentable and delete most of the comment explaining why it mustn't happen. Against that: explicit pathspecs at each call site are greppable, which has real value in a control whose whole job is being auditable by reading it. Judgment call, and it's yours — I'd take the arrays, but not strongly enough to push it into a security workflow unasked.

…ow text

The issue body and the no-changes message each enumerated two of the
three window paths. Harmless while audit-prompt-only commits were
dropped before reporting; wrong the moment they are reported, since the
header would name paths the commit did not touch.
dormouse-bot added a commit that referenced this pull request Aug 26, 2026
…ependent

The review on this PR flagged two claims that depend on #450 landing first.

"All three paths must be enumerated together wherever the window is
described, here and in the workflow" is false of main: workflow-audit.yaml
describes the window in three more places and each names only two paths.
Scope the claim to the selection pathspec the paragraph is actually the
prose spec of, which names the same three paths today and after #450.

The regen-arm sentence ("must not touch .config/tend.yaml") was the next
stale enumeration and no PR owned it, since #450 changes only the workflow.
State the invariant non-enumeratively instead — both arms inspect only
.github/workflows/, so every other path the window selects has to be
refused outright — which holds regardless of merge order and does not need
re-editing when a path is added.
@nedtwigg

Copy link
Copy Markdown
Member

Superseded by #452, which reached the same diagnosis independently and fixes it the same way — plus one thing you argued that I had gotten wrong.

You are right that SECURITY.md does not belong in that window, and #452 reverses my own addition of it. The reason you give — the audit reports the security automation's own inputs, not the spec they check against — holds up, and there is a threat-model half that makes it sharper: what this job watches is code that executes from a branch nobody reviewed, a workflow on a bot push or a folderOpen task on checkout. A FAIL IF is inert until merged to main, which is admin-gated, so the branch-wide watch adds nothing over PR review — while SECURITY.md changes in nearly every security commit, so including it would report on almost every such PR. A control that fires on routine work is one people learn to skim. .vscode/ stays, because it does execute from a branch.

The one difference in #452's version: WINDOW_NON_WORKFLOW is derived from the window ("${WINDOW[@]:1}") rather than being a second hand-written list. Two literal arrays are consistent right up until someone adds a path to one of them, which is the same drift class as the bug you found, one level up.

Closing as superseded, not as wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants