Skip to content

Add non-empty-diff check - #18

Merged
mark-brannan merged 4 commits into
mainfrom
add-nonempty-diff-check
Sep 15, 2026
Merged

mark-brannan merged 4 commits into
mainfrom
add-nonempty-diff-check

Conversation

@mark-brannan

@mark-brannan mark-brannan commented Sep 13, 2026 •

Copy link
Copy Markdown
Owner

Runs the shared reusable nonempty-diff workflow (mark-brannan/.github#25) as a job in the ci-gate needs, so the gate already requires it and no ruleset entry is needed. It refuses to call a PR green when its tree is identical to its base at the merge base (see mark-brannan/dotfiles#196/#198: an empty PR merged and was closed as done, having landed nothing).

Merge mark-brannan/.github#25 first; until then the nonempty job cannot resolve the reusable workflow and this PR is red.

🤖 Generated with Claude Code

@coderabbitai

coderabbitai Bot commented Sep 13, 2026 •

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 26737c4d-f97f-41b9-b873-77c090e28eba


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@claude

claude Bot commented Sep 13, 2026 •

Copy link
Copy Markdown

Claude finished @mark-brannan's task in 2m 24s —— View job


Review complete

  • Read CLAUDE.md / AGENTS.md — neither exists in this repo, nothing to reconcile against
  • Read the diff (.github/workflows/nonempty-diff.yml, +7/-0)
  • Checked CI status for this PR
  • Ran npm ci, npm test
  • Posted inline finding

What I ran:

  • npm ci — clean install, 0 vulnerabilities.
  • npm test (node --test) — 13/13 pass.
  • npm run build:web — blocked by sandbox approval, not executed (unrelated to this diff anyway; the change is workflow-only).
  • Checked this PR's live CI status via the CI status tool, and pulled job details for the new check's run.

Finding (inline on line 7): The nonempty-diff check added by this PR is itself failing on this exact PR (run 34788138520, conclusion failure, zero jobs reported) — consistent with the reusable workflow call to mark-brannan/.github/.github/workflows/nonempty-diff.yml@main failing to resolve, rather than the empty-diff logic ever executing. The PR body frames "shown green here" as the bar before it's considered landed; right now it isn't green. Worth confirming mark-brannan/.github#25 is actually merged to main and declares on: workflow_call before calling this done.

Minor, non-blocking note in the same comment: the call is pinned to the floating @main ref rather than a tag/SHA — low risk since it's a same-owner repo, but relevant if this becomes an enforced ruleset gate later.

The workflow YAML itself is syntactically sound and the intent (refuse to call an empty-diff PR green) is sound; the only issue found is that it isn't currently working end-to-end.

Comment thread .github/workflows/nonempty-diff.yml Outdated
Comment thread .github/workflows/nonempty-diff.yml Fixed
@claude

claude Bot commented Sep 14, 2026 •

Copy link
Copy Markdown

Claude finished @mark-brannan's task in 1m 57s —— View job


Review complete

  • Read CLAUDE.md / AGENTS.md — neither exists in this repo, nothing to reconcile against
  • Read the current diff (git diff origin/main...HEAD, .github/workflows/test.yml +7/-1)
  • Checked live CI status for this PR
  • Ran npm ci, npm test
  • Posted inline finding

What changed since the last review: this PR was synchronized — the approach moved from a standalone nonempty-diff.yml workflow to a nonempty job folded directly into test.yml, listed in ci-gate's needs: [test, nonempty] (test.yml:29-30, :41).

What I ran:

  • npm ci — clean install, 0 vulnerabilities.
  • npm test (node --test) — 13/13 pass.
  • npm run build:web — not run; irrelevant to this diff (workflow-only change), consistent with what the prior review on this PR did.
  • Checked this PR's live CI via the CI status tool and pulled job details for the test.yml run on the current commit.

Finding (inline on test.yml:30, blocking): The test.yml run for this exact commit (34876743478) is failure with zero jobs reported — the same symptom the prior review flagged for the old standalone nonempty-diff.yml (the reusable-workflow call not resolving, rather than the diff-emptiness logic ever executing). What's changed: since nonempty now lives inside test.yml rather than its own file, a reusable-workflow resolution failure appears to fail the whole calling workflow file before any jobs are created — which would mean this one bad reference can also block the test matrix and ci-gate jobs in the same run, not just the empty-diff check. That's a larger blast radius than the version reviewed before. I couldn't reach mark-brannan/.github from this sandbox (gh api/network calls require approval unavailable here) to directly confirm whether mark-brannan/.github#25 is merged to main and declares on: workflow_call — worth confirming before treating this as landed.

Resolved from the prior review: the earlier CodeQL finding ("workflow does not contain permissions") no longer applies — the nonempty job now inherits the permissions: contents: read already declared at the top of test.yml (line 8-9), rather than needing its own block.

Unchanged, minor: the reference is still pinned to the floating @main ref rather than a tag/SHA. Low risk given it's a same-owner repo, but worth a thought if this becomes an enforced ruleset gate.

The intent (refuse to call an empty-diff PR green) is sound and the YAML is syntactically valid; the blocking issue is that it still isn't working end-to-end on this commit, and the refactor may have widened what a resolution failure takes down with it.

Comment thread .github/workflows/test.yml
@mark-brannan
mark-brannan enabled auto-merge (squash) September 14, 2026 18:42
@claude

claude Bot commented Sep 14, 2026 •

Copy link
Copy Markdown

Claude finished @mark-brannan's task in 2m 7s —— View job


Review complete

  • Read CLAUDE.md / AGENTS.md — neither exists in this repo
  • Read the current diff (git diff origin/main...HEAD, .github/workflows/test.yml)
  • Checked live CI status for this commit (1684351)
  • Ran npm ci, npm test
  • Posted inline findings — none needed, no blocking issues found

What changed since the last review: the uses: reference for the nonempty job is now pinned to a full commit SHA (mark-brannan/.github/.github/workflows/nonempty-diff.yml@7b15aea59f53dcb5762a091f585b3d83d6dad545) instead of the floating @main ref — this addresses the minor pinning note from the prior review.

What I ran:

  • npm ci — clean install, 0 vulnerabilities.
  • npm test (node --test) — 13/13 pass.
  • No build script exists (build:web is unrelated to this workflow-only diff, consistent with prior reviews).

CI verification (the key open question from the two prior reviews): the test workflow run for this exact commit (34883851745) is now completed / success, with all five jobs green: test (20/22/24), nonempty / nonempty, and ci-gate / gate. This resolves the blocking finding from both prior reviews — the reusable workflow reference now resolves correctly, mark-brannan/.github#25 is evidently merged and reachable at that SHA, and the earlier concern that a bad reusable-workflow reference could take down the whole test.yml file (not just the nonempty job) is moot since resolution now succeeds.

I could not independently fetch mark-brannan/.github's commit/contents from this sandbox (outbound gh api calls to that repo require an approval this session doesn't have), but the live job execution succeeding end-to-end is stronger evidence than reading the source would be.

No blocking or non-blocking issues found. The intent (refuse to call an empty-diff PR green, folded into ci-gate's needs with no ruleset change required) is sound, the YAML is valid, and it now demonstrably works.

@claude

claude Bot commented Sep 15, 2026 •

Copy link
Copy Markdown

Claude finished @mergify[bot]'s task in 1m 17s —— View job


Review complete

  • Read CLAUDE.md / AGENTS.md — neither exists in this repo
  • Read current diff (git diff origin/main...HEAD, .github/workflows/test.yml, +7/-1)
  • Checked live CI status for current HEAD
  • Ran npm ci, npm test
  • Posted findings

Diff: adds a nonempty job (calls mark-brannan/.github/.github/workflows/nonempty-diff.yml@main) to test.yml, and adds it to ci-gate's needs: [test, nonempty].

What I ran:

  • npm ci — clean install, 0 vulnerabilities.
  • npm test (node --test) — 13/13 pass.
  • No build script exists; build:web is unrelated to this workflow-only change.

CI verification: live test workflow run for current HEAD (6e3fedf, run 34987305540) is completed/success across all five jobs — test (20/22/24), nonempty / nonempty, ci-gate / gate. The reusable-workflow reference that failed to resolve in the two earlier reviews on this PR now resolves and runs end-to-end.

On the prior open items:

  • Reusable-workflow resolution failure — resolved, confirmed live rather than hand-traced.
  • Missing permissions on the nonempty job (CodeQL) — inherits permissions: contents: read from the top of test.yml; no separate block needed.
  • Floating @main ref (my earlier minor note) — briefly pinned to a SHA as a workaround while mark-brannan/.github#25 was unmerged (5344f9f), then deliberately reverted to @main once merged (49c03bd), citing a repo convention that same-owner reusable workflows are called at the tip of main. That's a stated policy decision, not an oversight — no longer a finding.

No blocking or non-blocking issues found. The mechanism (refuse to call an empty-diff PR green, wired into ci-gate's needs, no ruleset change required) is sound, the YAML is valid, and it's demonstrably working on this commit.

Note: couldn't post via gh pr comment as the task's custom instructions specify — public-issue-guard blocked it because this session's continuity state repo (claude_prompts_scratch) isn't attached, so the private-terms denylist it checks against is unreadable. Posting the full review here instead rather than bypassing that guard.

mark-brannan and others added 4 commits September 15, 2026 10:05
Co-Authored-By: Solace (Mark) <[email protected]>
One fewer workflow file, and no per-repo ruleset entry: the gate already
requires every job in its needs.

Co-Authored-By: Claude Fable 5.1 <[email protected]>
Pins mark-brannan/.github/.github/workflows/nonempty-diff.yml to
7b15aea59f53dcb5762a091f585b3d83d6dad545 (mark-brannan/.github#25's merged
commit) instead of the mutable @main ref.

Co-Authored-By: Claude Sonnet 5 <[email protected]>
The SHA pin was a workaround for mark-brannan/.github#25 being unmerged; it
merged at 2026-09-14T18:10:34Z, so the workaround's reason is gone. Our own
reusable workflows are called at the tip of main so one copy stays the one
copy to fix -- rule added in mark-brannan/dotfiles#225.

Co-Authored-By: Claude Opus 5 <[email protected]>
@mark-brannan
mark-brannan force-pushed the add-nonempty-diff-check branch from 6e3fedf to 126bcf5 Compare September 15, 2026 17:05
@mark-brannan
mark-brannan merged commit c447b67 into main Sep 15, 2026
11 of 12 checks passed
@mark-brannan
mark-brannan deleted the add-nonempty-diff-check branch September 15, 2026 17:05
@mergify mergify Bot removed the awaiting-human label Sep 15, 2026
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