Repository navigation
Restore #918's CI guards, minus the parts that turned out to be wrong - #937
Conversation
Measured, not assumed. The GraphQL quota for the repo owner keeps hitting zero (`sweep-review-threads` died on it again today), so I inventoried where the calls come from rather than guessing: - Scheduled workflows are NOT the driver: 11 crons totalling ~3 runs/day. - 24 workflows fire on push / pull_request / issue_comment. Of those, 19 had no concurrency guard at all, so every push to an open PR started a fresh full set while the previous set kept running to completion. Nobody is waiting on the older run's verdict; it burns runners and API calls to report on a commit that has already been replaced. This adds a guard to the nine read-only check workflows — the ones that verified as zero write operations, no commits, no deploys, no PR/issue mutations, so a cancelled run cannot leave partial state behind: check-dotfolder-anchors check-notebooks-paired check-portable-paths cross-platform-smoke laf-usb-manifest-policy large-file-policy validate-daily-notes validate-agent-content secret-pattern-policy Two deliberate choices in the group key: - `github.event_name` is included so the push-event run and the pull_request-event run of one branch land in SEPARATE groups. Collapsing them would dedupe the double-fire, but at the cost of letting a push run cancel a required PR check — and a cancelled required check reads as a blocked PR. Not worth it. - merge_group is exempted from cancellation. Cancelling a merge-queue check breaks the queue. No coverage is lost: the final state of a branch is still checked in full. Only verdicts on superseded commits are dropped. Verified: all 49 workflow files still parse as YAML; the nine resolve the expected cancel-in-progress expression; and PR #913's diff still applies cleanly on top of this branch, so the two are independent. Left alone, deliberately, because they change behavior rather than just scheduling — Logan's call, not mine: - secret-pattern-policy still carries `push: branches: ['**']` where the other seven use ['main']. That is the duplicate run AND the whole-tree scan that false-reds every new branch. Narrowing it narrows a security control, so it is not something to do unasked. - review-feedback-loop fires on every issue_comment, including each bot's (CodeRabbit, Sourcery, Codacy), and its sweep queries reviewThreads(first:100){comments(first:100)} — up to 10,000 nodes per call. Gating on non-bot commenters would cut most firings, but I have not verified what verify-agent-claims expects to react to. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBV6TkrwsZhcwkh1b6NUHs
Two changes to the machinery that has been failing all day, both measured.
1. secret-pattern-policy: the new-branch fallback scanned the whole repo.
The `push: branches: ['**']` trigger STAYS. Its own comment records that it
was broadened on 2026-07-02 because a real leak arrived by direct push to a
working branch, which a main-only trigger never sees. Narrowing it would
reopen the hole a documented incident closed. (An earlier version of this
branch's PR description recommended exactly that; it was wrong and has been
withdrawn.)
The defect is one level down. On a new branch (`before == 000…0`) the job
ran `git ls-tree -r` over everything:
whole tree 75,420 files
files the branch introduces 16 files
It re-reported the same ~55 pre-existing findings (minified plugin bundles,
clipped articles with embedded map keys) on #910, #913 and #918 in one day,
which is how a gate that caught a genuine leak gets trained into noise.
Now it diffs from the parent of the oldest commit reachable from this branch
and from no other ref — every file the branch's own commits touch. A secret
pushed straight to a working branch is by definition in such a file, so the
leak this trigger exists for is still caught. If that base cannot be
determined (root commit, orphan branch, refs not fetched) it FAILS CLOSED to
the whole-tree scan, so the worst case is exactly today's behaviour.
2. review-feedback-loop: `sweep-review-threads` had no concurrency guard.
This is the repository's largest GraphQL consumer. It fires on `synchronize`
— every push to every open PR — and each run issues
`reviewThreads(first: 100) { comments(first: 100) }`, which GitHub prices at
roughly 100 points against a 5,000/hour budget. That is ~50 sweeps an hour.
A few pushes across a few branches exhaust it, and then every PR in the repo
reports "API rate limit already exceeded for user ID 136375980" — which is
what has been happening.
Cancelling a superseded sweep is safe because sync-pr is a *sync*: it drives
labels and thread state toward what the PR currently looks like, so a run cut
short is corrected by the next one, which reads current state rather than
resuming. Only the last run's verdict matters. Guard is job-level, so the two
issue_comment jobs are untouched.
Not done, deliberately: shrinking the query's page sizes would cut the cost
further, but consumers read comment bodies out of those threads and I could
not establish a smaller page that never drops one. Silently missing a comment
is the failure class this commit is cleaning up, not one to introduce.
Corrected from my own earlier claim on #918: bot comments are NOT the main
drain. `acknowledge-apply` and `verify-claim` filter on the comment body before
the expensive fetch, so a CodeRabbit or Sourcery comment costs `ensure_labels()`
and exits. The pushes are the cost, and today most of them were mine.
Verified: the scoping logic run verbatim against this branch yields base
a83c1d6 and 16 files; the empty-base path falls through to the whole-tree scan;
the scoped file set passes check_secret_patterns.py; all 49 workflows parse; the
rewritten shell block passes `bash -n`.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EBV6TkrwsZhcwkh1b6NUHs
The two commits below were merged into logan/obsidian as #918 on 2026-08-04T05:27:58Z (merge commit 913a84c) and discarded ten minutes later when 3591a4b was committed on main's tip and pushed to logan/obsidian, resetting that branch. Neither commit is an ancestor of main or of logan/obsidian today; both survive on origin/claude/ci-concurrency-qzt7le. Cherry-picked, not rewritten, so authorship and content are unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBV6TkrwsZhcwkh1b6NUHs
|
Warning Review limit reached
Next review available in: 9 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughChangesWorkflow execution controls
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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. Comment |
Reviewer's GuideRestores the previously merged CI concurrency safeguards and scoped secret scanning behavior by cherry-picking #918’s commits back onto main, adding standardized concurrency groups to nine workflows, a job-level concurrency guard to the review sweep, and tightening the new-branch secret scan to only files introduced by the branch with a fail-closed fallback. Sequence diagram for review-feedback-loop sweep job concurrency guardsequenceDiagram
actor Developer
participant GitHub
participant review_feedback_loop_workflow
participant GitHubGraphQLAPI
Developer->>GitHub: Push to PR (synchronize)
GitHub->>review_feedback_loop_workflow: Trigger sweep-review-threads job
review_feedback_loop_workflow->>review_feedback_loop_workflow: Apply concurrency group review-feedback-loop-sweep-${{ github.event.pull_request.number }}
alt Existing sweep job running
review_feedback_loop_workflow->>review_feedback_loop_workflow: cancel-in-progress: true (cancel previous sweep)
else No existing sweep
review_feedback_loop_workflow->>review_feedback_loop_workflow: Start new sweep-review-threads run
end
review_feedback_loop_workflow->>GitHubGraphQLAPI: Query reviewThreads(first: 100) { comments(first: 100) }
GitHubGraphQLAPI-->>review_feedback_loop_workflow: Return review threads and comments (rate limit ~100 points per run)
review_feedback_loop_workflow-->>GitHub: Update PR review thread state based on latest run
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
loganfinney27
added
risk/low
|
Three red checks. None is caused by this diff, but one of them is a genuine ordering dependency: #936 has to merge first.
That is the old step — this branch is cut from I'm deliberately not fixing it here. Copying #936's change into this branch would duplicate a fix already under review and create a conflict between the two PRs on the same file. Merge #936, then this goes green — either on its own next run or after I merge
Green here: all six One correction to something I said on #936, before it gets misread: Incidentally the checkout log confirms the scan-scope figure in the description: Generated by Claude Code Generated by Claude Code |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Coverage variation | ✅ -0.02% coverage variation (-1.00%) |
| Diff coverage | ✅ ∅ diff coverage |
Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (2de9687) 13529 11333 83.77% Head commit (249360c) 13529 (+0) 11330 (-3) 83.75% (-0.02%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>
Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#937) 0 0 ∅ (not applicable) Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
Insufficient balance to process this code review. Please add funds or upgrade your plan in billing. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new-branch scoping logic in
secret-pattern-policy.ymlassumes anoriginremote and a complete set of local/remote refs; consider explicitly handling forks or atypical remote names so theothersset andbaseselection are robust in more CI environments. - The concurrency block (group key and
cancel-in-progresscondition) is duplicated across nine workflows; consider extracting this into a shared reusable workflow or clearly centralizing the pattern to reduce future drift if you need to adjust the grouping rules.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new-branch scoping logic in `secret-pattern-policy.yml` assumes an `origin` remote and a complete set of local/remote refs; consider explicitly handling forks or atypical remote names so the `others` set and `base` selection are robust in more CI environments.
- The concurrency block (group key and `cancel-in-progress` condition) is duplicated across nine workflows; consider extracting this into a shared reusable workflow or clearly centralizing the pattern to reduce future drift if you need to adjust the grouping rules.
## Individual Comments
### Comment 1
<location path=".github/workflows/review-feedback-loop.yml" line_range="75-59" />
<code_context>
+# blocks the PR.
+#
+# merge_group is never cancelled — cancelling a merge-queue check breaks the queue.
+concurrency:
+ group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.head_ref || github.ref }}
+ cancel-in-progress: ${{ github.event_name != 'merge_group' }}
</code_context>
<issue_to_address>
**issue (bug_risk):** This job defines `concurrency` twice, which will result in an invalid/ambiguous workflow configuration.
GitHub Actions supports only one `concurrency` block per job. Defining both the existing `concurrency` (with the `sweep-review-threads-${{ github.event_name }}-...` group) and this new one (with `review-feedback-loop-sweep-...`) under `sweep-review-threads` produces duplicate YAML keys, which may cause the workflow to fail or ignore one definition. Please consolidate to a single `concurrency` block that reflects the desired cancellation behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
loganfinney27
added
the
review/threads-open
There was a problem hiding this comment.
Pull request overview
Restores CI concurrency controls and scoped secret scanning intended to reduce redundant workflow runs and rate-limit pressure.
Changes:
- Adds concurrency guards to nine validation workflows.
- Adds job-level concurrency to the review sweep.
- Scopes new-branch secret scans to branch-introduced files.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/validate-daily-notes.yml |
Adds concurrency control. |
.github/workflows/validate-agent-content.yml |
Adds concurrency control. |
.github/workflows/secret-pattern-policy.yml |
Adds concurrency and scoped scanning. |
.github/workflows/review-feedback-loop.yml |
Adds review-sweep concurrency. |
.github/workflows/large-file-policy.yml |
Adds concurrency control. |
.github/workflows/laf-usb-manifest-policy.yml |
Adds concurrency control. |
.github/workflows/cross-platform-smoke.yml |
Adds concurrency control. |
.github/workflows/check-portable-paths.yml |
Adds concurrency control. |
.github/workflows/check-notebooks-paired.yml |
Adds concurrency control. |
.github/workflows/check-dotfolder-anchors.yml |
Adds concurrency control. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Cherry-picking #918 added a second `concurrency:` key to the sweep-review-threads job. That job already had one on main. Two problems, both real: - Duplicate mapping keys in a job are invalid for GitHub Actions. - Under last-key-wins parsing, #918's block silently REPLACED main's. Main's key namespaces by `github.event_name`; #918's does not. This workflow also fires on `issue_comment`, where `issue.number` is the same number as `pull_request.number` — so without the namespace a comment event can contend with, and cancel, a live sweep for that PR. The duplicate reintroduced exactly the collision main's comment warns about. Main's guard is kept unchanged. #918's is dropped: that part of #918 was independently re-implemented on main, and better. Found by Sourcery on #937. It got past my own check because yaml.safe_load accepts duplicate keys silently and keeps the last. Replaced with a loader that raises on duplicates; all 52 workflows scanned, 0 failures. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBV6TkrwsZhcwkh1b6NUHs
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/review-feedback-loop.yml:
- Around line 75-77: Remove the later duplicate concurrency block from the
review-feedback-loop job, keeping the existing earlier concurrency definition
and its behavior unchanged.
In @.github/workflows/secret-pattern-policy.yml:
- Around line 71-72: Update the `others` ref collection in the secret-pattern
policy workflow to include `refs/tags` alongside `refs/heads` and
`refs/remotes/origin`, so `git rev-list` recognizes commits reachable only
through tags as existing history while preserving the current exclusions for the
local and remote branch refs.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e2e9d4a9-5dca-49f7-b8e5-34406c0d5799
📒 Files selected for processing (10)
.github/workflows/check-dotfolder-anchors.yml.github/workflows/check-notebooks-paired.yml.github/workflows/check-portable-paths.yml.github/workflows/cross-platform-smoke.yml.github/workflows/laf-usb-manifest-policy.yml.github/workflows/large-file-policy.yml.github/workflows/review-feedback-loop.yml.github/workflows/secret-pattern-policy.yml.github/workflows/validate-agent-content.yml.github/workflows/validate-daily-notes.yml
loganfinney27
added
the
review/suggestions-ready
…ing history Two fixes to the same file, both from review on #937. 1. push.branches back to ['**']. Restoring #918's scoping without this was restoring a fix for a code path that cannot run. `main` has push.branches: [main], so a push to a working branch never starts this workflow at all — the new-branch case the scoping handles is unreachable. The broadening is a third casualty of the same 05:37 rewind: `git log -S` finds ['**'] nowhere in main's history for this file. It was broadened on logan/obsidian on 2026-07-02 because a real leak arrived by direct push to a working branch, which a main-only trigger never scans. The trigger and the scoping are one design — the trigger is what provides the coverage, the scoping is what keeps that coverage from drowning in ~55 pre-existing findings on every new branch. The PR body claimed this trigger was already intact. It was not. That claim was copied from #918's description instead of read off the file. 2. refs/tags added to the `others` set. If a branch starts from a commit reachable only through a tag, omitting tags makes `rev-list --not others` treat that pre-existing history as branch-unique and widen the scan to unrelated files — the exact noise the scoping exists to prevent. Over-scan, not under-scan, so no leak was missable; it just defeats the purpose. Verified: 52 workflows scanned for duplicate keys, 0 failures; bash -n clean; scoping logic re-run verbatim with tags -> base 02b3af1, 9 files against a 38,480-file tree, check_secret_patterns.py OK. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBV6TkrwsZhcwkh1b6NUHs
|
Insufficient balance to process this code review. Please add funds or upgrade your plan in billing. |
loganfinney27
removed
the
review/suggestions-ready
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Suppressed comments (13)
.github/workflows/validate-agent-content.yml:20
- This workflow scans only the files in each push event’s
before..afterrange. If a later push arrives while the prior run is active, cancellation can discard validation of files changed only by the prior push, even though those files remain on the branch. Use a SHA-specific group for push events (as below), or change the validator to rescan cumulative/full branch state.
group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.head_ref || github.ref }}
cancel-in-progress: ${{ github.event_name != 'merge_group' }}
.github/workflows/validate-daily-notes.yml:23
- The push path below validates only the current event’s
before..afterdelta, so canceling an earlier run can permanently skip a violation. For example, push A can add a bad placeholder, then unrelated push B can cancel A while B scans only its own files. Make push groups SHA-specific (as below), or make every replacement run validate cumulative/full state before retaining branch-level cancellation.
group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.head_ref || github.ref }}
cancel-in-progress: ${{ github.event_name != 'merge_group' }}
.github/workflows/secret-pattern-policy.yml:85
- With
set -eo pipefail, anygit rev-listfailure exits the step at this assignment instead of reaching the advertised whole-tree fallback. Convert failure to an empty result so valid error cases (for example, a tag ref that does not resolve to a commit) actually fail closed.
first=$(git rev-list "$after" --not $others 2>/dev/null | tail -1)
.github/workflows/large-file-policy.yml:22
- The push branch validates only each event’s
before..afterfile list, so canceling an earlier run can skip a large file that remains in the tree when a later unrelated push replaces that run. Make push groups SHA-specific (as below), or change replacement runs to validate cumulative/full state.
group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.head_ref || github.ref }}
cancel-in-progress: ${{ github.event_name != 'merge_group' }}
.github/workflows/check-portable-paths.yml:21
- Both push checks below consume only the current event’s
before..afterpaths. A subsequent push can therefore cancel validation of a violation introduced by the prior push and then scan only unrelated files. Make push groups SHA-specific (as below), or make replacement runs validate cumulative/full state.
group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.head_ref || github.ref }}
cancel-in-progress: ${{ github.event_name != 'merge_group' }}
.github/workflows/laf-usb-manifest-policy.yml:28
github.head_refcontains only the source branch name, not the source repository or PR number. Two fork PRs namedmainorpatch-1(or one head branch targeting two bases) therefore share a group and can cancel each other’s required check.github.refis stable per PR (refs/pull/<number>/merge) and avoids that collision.
group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.head_ref || github.ref }}
.github/workflows/cross-platform-smoke.yml:34
github.head_refis only the unqualified source branch name, so unrelated PRs from forks with the same common branch name enter one concurrency group and cancel each other. Usegithub.ref, whose pull-request value includes the PR number, while preserving deduplication across updates to the same PR.
group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.head_ref || github.ref }}
.github/workflows/check-notebooks-paired.yml:35
- Using the unqualified
github.head_refmakes distinct PRs collide whenever their source branches have the same name, which can leave one required notebook check canceled.github.refis PR-specific and remains stable across synchronize events.
group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.head_ref || github.ref }}
.github/workflows/check-dotfolder-anchors.yml:20
github.head_refomits the fork/repository and PR number. Distinct PRs with identically named source branches can therefore cancel one another’s required check. Use the PR-specificgithub.refinstead; it still groups successive runs of the same PR.
group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.head_ref || github.ref }}
.github/workflows/validate-daily-notes.yml:22
- For pull-request runs,
github.head_refis only the unqualified branch name. Distinct fork PRs with the same source branch name can therefore cancel each other’s required check; use the PR-specificgithub.ref(as in the other suggested replacement on this block).
group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.head_ref || github.ref }}
.github/workflows/secret-pattern-policy.yml:26
- For pull-request events,
github.head_refomits the source repository and PR number, so unrelated fork PRs with a common branch name can cancel one another. Use the PR-specificgithub.ref; the other suggested replacement on this block combines that with SHA-specific push groups.
group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.head_ref || github.ref }}
.github/workflows/large-file-policy.yml:21
github.head_refis not unique to a pull request: fork PRs commonly reuse names such asmainorpatch-1, causing unrelated required checks to cancel each other. Use the PR-specificgithub.ref; the other suggested replacement on this block also keeps push deltas isolated by SHA.
group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.head_ref || github.ref }}
.github/workflows/check-portable-paths.yml:20
- For pull requests,
github.head_refcontains only the source branch name, so different fork PRs with the same branch name share this group and can cancel one another. Usegithub.ref, which includes the PR number; the other suggested replacement on this block combines that with safe push grouping.
group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.head_ref || github.ref }}
#918 justified branch-level cancellation on the grounds that these checks are read-only, so a cancelled run cannot leave partial state. That is true and it is not the relevant property. Read-only is not the same as cumulative. Five of the nine guarded workflows scan only the current push's before..after delta: check-portable-paths large-file-policy secret-pattern-policy validate-agent-content validate-daily-notes For those, cancelling a push run loses that delta's coverage permanently. Push A adds a violation; push B touches an unrelated file and cancels A; B scans only its own files, and A's remain on the branch never having been examined. Nothing rescans them, because the next push's `before` is A's `after`. On secret-pattern-policy that is a hole in the check whose entire purpose is catching a secret pushed straight to a working branch — the same leak the ['**'] trigger was broadened for. Restoring that trigger while adding branch-level cancellation would have handed back the coverage with one hand and removed it with the other. Push groups on those five are now SHA-specific, so every push run is alone in its group and always completes. pull_request runs scan base..head, which is cumulative, so superseding one loses nothing — they still dedupe. The other four (check-dotfolder-anchors, check-notebooks-paired, cross-platform-smoke, laf-usb-manifest-policy) do not read github.event.before and are left exactly as #918 had them. Found by Copilot on #937. Measured before applying: grep for github.event.before across all nine, 5 hits / 4 clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBV6TkrwsZhcwkh1b6NUHs
Reverts the refs/tags addition from 088a676. Two reviewers disagreed and the security-relevant one is right. Everything in the `others` set is asserted to be already-scanned history. This workflow triggers on push.branches with no `tags:` filter, so a tag push never runs it — tagged commits have never been scanned by anything. Subtracting them asserts a baseline that does not exist. If a tag points at secret-bearing commit S and a branch adds harmless commit U on top, `rev-list --not <tags>` returns only U, `base` becomes S, and the S..U scan steps over the secret. CodeRabbit asked for tags to be included to avoid an over-scan when a branch starts from a tag-only-reachable commit. That cost is real but it is noise. Copilot's cost is a miss. For a secret scanner those are not comparable, so this fails toward noise. I applied the first suggestion without asking what the subtraction was asserting. The reasoning is now in the file so the next person does not re-add it for the same plausible reason. Zero tags exist in this repo today, so nothing changes in practice — the code is wrong regardless of whether it is currently reachable. Verified: 52 workflows 0 duplicate-key/parse failures; bash -n clean; scoping run verbatim -> base 02b3af1, 20 files of 38,480, guard OK. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBV6TkrwsZhcwkh1b6NUHs
|
Insufficient balance to process this code review. Please add funds or upgrade your plan in billing. |
|
Copilot is right, and this reverts a change I made two commits ago on CodeRabbit's advice. Fixed in The two reviewers disagree, and only one of them is safe to follow. Everything subtracted in the
CodeRabbit's concern was genuine, just not comparable in magnitude. Over-scanning a secret check produces findings someone has to dismiss; under-scanning produces a leak nobody sees. It now fails toward noise, with the reasoning in the file so the next person doesn't re-add tags for the same plausible-sounding reason. Zero tags exist in this repo today, so nothing changes in practice. The code was wrong regardless of whether it was currently reachable. What I should have done differently: applied the first suggestion only after asking what the subtraction was claiming. "Add Verified after the revert: 52 workflows, 0 duplicate-key or parse failures; Generated by Claude Code Generated by Claude Code |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Coverage variation | ✅ -0.02% coverage variation (-1.00%) |
| Diff coverage | ✅ ∅ diff coverage |
Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (9f82992) 13529 11333 83.77% Head commit (d3d84bd) 13529 (+0) 11330 (-3) 83.75% (-0.02%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>
Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#937) 0 0 ∅ (not applicable) Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Suppressed comments (6)
.github/workflows/secret-pattern-policy.yml:83
- The PR description says
refs/tagsis included in the ref set, while this code now deliberately excludes tags for safety. Update the PR description so reviewers are not being asked to approve the opposite behavior.
# refs/tags is deliberately NOT in this list, and the reason is worth
# keeping because it looks like an oversight.
.github/workflows/secret-pattern-policy.yml:17
- This summary contradicts the guard below: the SHA suffix deliberately prevents one push from canceling another, and this job checks only each push's
before..afterdelta rather than the full branch. Keep the accurate delta-safety explanation and describe cancellation as applying only to cumulative pull-request runs.
This issue also appears on line 82 of the same file.
# Supersede, don't stack: a second push to the same branch cancels the still-running
# check for the previous commit, which nobody is waiting on. The final state of the
# branch is still checked in full, so no coverage is lost.
.github/workflows/large-file-policy.yml:12
- This summary contradicts the guard below: the SHA suffix deliberately prevents one push from canceling another, and this job checks only each push's
before..afterdelta rather than the full branch. Keep the accurate delta-safety explanation and describe cancellation as applying only to cumulative pull-request runs.
# Supersede, don't stack: a second push to the same branch cancels the still-running
# check for the previous commit, which nobody is waiting on. The final state of the
# branch is still checked in full, so no coverage is lost.
.github/workflows/check-portable-paths.yml:11
- This summary contradicts the guard below: the SHA suffix deliberately prevents one push from canceling another, and this job checks only each push's
before..afterdelta rather than the full branch. Keep the accurate delta-safety explanation and describe cancellation as applying only to cumulative pull-request runs.
# Supersede, don't stack: a second push to the same branch cancels the still-running
# check for the previous commit, which nobody is waiting on. The final state of the
# branch is still checked in full, so no coverage is lost.
.github/workflows/validate-daily-notes.yml:20
- This summary says later pushes cancel earlier ones and the branch is checked in full, but the SHA-specific key intentionally retains every push because this job scans only that push's delta. It also mentions
merge_group, which this workflow does not trigger. Align the header with the accurate explanation below.
# Supersede, don't stack: a second push to the same branch cancels the still-running
# check for the previous commit, which nobody is waiting on. The final state of the
# branch is still checked in full, so no coverage is lost.
.github/workflows/check-notebooks-paired.yml:33
- This workflow has only
pull_requestandworkflow_dispatchtriggers, so the added explanation about push-event separation and merge-queue cancellation describes events that can never occur here. Document the actual grouping: cumulative PR runs and full-state manual runs are independently superseded by ref.
# Supersede, don't stack: a second push to the same branch cancels the still-running
# check for the previous commit, which nobody is waiting on. The final state of the
# branch is still checked in full, so no coverage is lost.
…ng code Two problems, both from Copilot on #937. 1. Subtracting every ref asserted a baseline that does not exist. The old logic found "commits unique to this branch" with `rev-list --not <all branches>`. Everything subtracted is an assertion that the history has already been scanned. It has not: until this PR broadens the trigger, only the default branch was scanned on push, so a sibling branch ref may never have been scanned at all — and a ref survives a scan that FAILED. If such a ref points at secret-bearing commit S and this branch adds U on top, subtracting it sets base to S and the S..U scan steps over the secret. The leak that broadened this trigger on 2026-07-02 was a direct push to a working branch, which is exactly that shape. This is the same argument I accepted one commit ago for refs/tags. It applies to branch refs for the same reason, and I did not carry it through. Now: merge-base against the default branch — the only baseline here that is genuinely scanned (on every push to it and on every pull_request). Falls back to the whole tree when the default branch cannot be resolved. Cost: a branch cut from another unmerged branch rescans that branch's files. Over-scan, not a miss. That is the correct direction to fail, and it drops the ref enumeration, self/tag exclusions and --topo-order reasoning entirely — about 25 lines of logic replaced by 6. 2. Headers claiming behaviour the code no longer has. The #918 header says "a second push to the same branch cancels the still-running check ... no coverage is lost". On the four SHA-keyed workflows that is now the opposite of true, and the comment beneath `concurrency:` already states the real rule — so the outer header is deleted rather than reworded. check-notebooks-paired carried a header about push separation and merge-queue cancellation while triggering only on pull_request and workflow_dispatch. laf-usb-manifest-policy promised merge_group handling it has no trigger for. Both rewritten to their actual triggers. Verified: 52 workflows, 0 parse/duplicate-key failures, 131 run blocks bash -n clean; merge-base logic simulated on this branch -> base 2de9687, 9 files of 38,480, guard OK; unresolvable default branch -> whole-tree fallback; automated check that no header claims an event its workflow does not declare, 8/8 OK. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBV6TkrwsZhcwkh1b6NUHs
|
Insufficient balance to process this code review. Please add funds or upgrade your plan in billing. |
|
Both correct again. Fixed in 1. I accepted the tag argument and failed to carry it through to branches. One commit ago I removed I should have re-examined the whole Replaced with merge-base against the default branch — the only baseline here that genuinely is scanned (every push to it, every PR): base=""
for ref in "origin/${DEFAULT_BRANCH}" "${DEFAULT_BRANCH}"; do
if git rev-parse -q --verify "$ref" >/dev/null 2>&1; then
base=$(git merge-base "$ref" "$after" 2>/dev/null) && break
base=""
fi
doneCost: a branch cut from another unmerged branch rescans that branch's files. Over-scan, not a miss — the correct direction. It also deletes the ref enumeration, the self/tag exclusions and the 2. Comments now contradicted the code — including comments I wrote. #918's header says "a second push to the same branch cancels the still-running check … no coverage is lost." On the four SHA-keyed workflows that is now exactly backwards. The comment under
Added an automated check so this class doesn't recur silently — it asserts no header claims an event its workflow doesn't declare, and no header claims push-cancellation while SHA-keyed. 8/8 OK. Verification: 52 workflows, 0 parse/duplicate-key failures, 131 run blocks The PR description's Generated by Claude Code Generated by Claude Code |
loganfinney27
removed
the
review/threads-open
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (1)
.github/workflows/secret-pattern-policy.yml:69
- The PR body still says the new-branch baseline is derived with
rev-listagainst all refs (including tags) and calls out--topo-order, but this implementation now uses only the merge-base with the default branch. Those algorithms have different coverage and re-scan behavior. Update the PR description and verification claims to describe the merge-base implementation so reviewers are evaluating the security boundary actually being merged.
# Diff from the merge-base with the default branch. That is the ONLY
# baseline in this repo that has actually been scanned: the default
# branch is scanned on every push to it and on every pull_request.
|
If you are watching or subscribed to this PR, please respond to this comment. |
|
Watching. Session State at
The last one collapsed ~25 lines into 6 and made three earlier findings stop existing rather than be patched. Red: Two things I'd flag for you rather than bury:
Generated by Claude Code Generated by Claude Code |
|
Insufficient balance to process this code review. Please add funds or upgrade your plan in billing. |
Pull Request Summary by devActivityMetrics
Achievements
|
AGENT PR TEMPLATE
Agent: Claude Code (
agent:claude-code)Date: 2026-08-08
Branch:
claude/poka-yoke-qzt7le→mainWhat this is
#918 was genuinely merged into
logan/obsidian(913a84c76), then discarded ten minutes later when3591a4bbwas committed on main's tip and pushed tologan/obsidian, resetting it.This restores it — minus four pieces review showed were wrong — plus the trigger it depends on.
Changes
1. Eight concurrency guards, in two shapes, because the shape must match what the check scans:
check-dotfolder-anchors,check-notebooks-paired,cross-platform-smoke,laf-usb-manifest-policygithub.event.before; superseding loses nothingcheck-portable-paths,large-file-policy,secret-pattern-policy,validate-daily-notesbefore..afterdelta2.
secret-pattern-policynew-branch scan baselined onmerge-basewith the default branch — 9 files vs 38,480 — failing closed to a whole-tree scan when that branch can't be resolved.3.
push.branchesrestored to['**']. Predates #918; broadened 2026-07-02 after a real leak arrived by direct push to a working branch.git log -Sfinds it nowhere in main's history — a third casualty of the same rewind. Without it item 2 is a fix for unreachable code.Four things from #918 deliberately NOT restored
The sweep-job guard.
mainalready has a better one (namespaced bygithub.event_name, which matters because this workflow also fires onissue_commentwhereissue.numberequalspull_request.number). My cherry-pick added a duplicateconcurrency:key that silently replaced the good guard under last-key-wins.Branch-level cancellation on delta scans. #918 argued the guards were safe because the checks are read-only. Read-only is not cumulative — cancel a delta scan and nothing ever examines that delta, because the next push's
beforeis the cancelled run'safter.Any guard on
validate-agent-content. It fires onpushonly and scans a delta: branch grouping loses coverage, SHA grouping is inert. Genuinely unguardable as written.The whole
rev-list --not <every ref>approach. Subtracting a ref asserts that history is already scanned. Sibling branch refs are not — until this PR only the default branch was scanned on push, and a ref survives a scan that failed. A stale ref at secret-bearingSplus a new commitUsetsbasetoSand steps over the secret. Replaced by merge-base, which is the one genuinely-scanned baseline. ~25 lines became 6, and therefs/tagsand--topo-orderfindings stopped existing rather than being patched.Verification
bash -n, 131 run blocks2de9687dd, 9 files of 38,480check_secret_patterns.pyon the scoped setsecret-pattern guard: OKreview-feedback-loop.ymlvsmainOn the verification that failed me: my original check was
yaml.safe_load, which accepts duplicate mapping keys and silently keeps the last. It reported "0 parse failures" while a duplicateconcurrency:sat there. Replaced with a loader that raises.actionlintcatches it directly and isn't installed here — its own gap.A comment-accuracy check now runs too, after review found headers asserting the opposite of their code: it fails if a header claims an event the workflow doesn't declare, or claims push-cancellation while SHA-keyed.
Interaction with #936
pairedis red here because #936 hasn't merged — re-verified against currentmain'suv.lock: 20 packages, pytest present, jupytext still absent, and the failure is the sameStopIterationat the lockfile lookup. Merge #936 and this goes green.Blockers
paired.Checklist:
Risk Level:
The one behavioural change worth your eyes:
secret-pattern-policygoes from[main]to every branch. That is the point of the 2026-07-02 broadening, and the scoping is what makes it affordable rather than ~55 repeat findings per branch.Not addressed
github.head_refis not fork-qualified, so two fork PRs with the same branch name share a concurrency group. Sound reasoning; declined here only because the idiom is shared with the ten guards already onmain, so fixing it means editing workflows this PR doesn't touch.Labels to apply:
agent:claude-codehttps://claude.ai/code/session_01EBV6TkrwsZhcwkh1b6NUHs