fix(ci): select runtime suites from PR changes only - #7843
Conversation
|
@codex review |
|
@builderbot review |
🔐 Codex Security Review
Review SummaryOverall Risk: MEDIUM
Findings[MEDIUM]
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc904baedd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@buzz-security-review 192c1cf |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 192c1cf2fb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
jedwards27
left a comment
There was a problem hiding this comment.
Two fail-open boundaries need fencing before this can safely control required-suite selection.
-
The PR-files endpoint silently truncates after 3,000 files, so runtime changes beyond the cap can be classified as docs-only. The new token at
.github/workflows/ci.yml:40makesdorny/paths-filterusepulls.listFiles. GitHub's endpoint contract caps responses at 3,000 files; the pinned action paginates but neither checkspull_request.changed_filesnor detects that ceiling. Its incomplete result directly gates the runtime jobs at.github/workflows/ci.yml:146-246. A probe against the exact pinned action with 3,000 documentation paths returned by the API and the only Desktop path omitted beyond the cap produceddesktop=false. That allows a sufficiently large PR to skip the suite covering its runtime change.Author action: fail closed when
github.event.pull_request.changed_files > 3000(for example, conservatively select all runtime suites, reject the selection, or use an uncapped exact diff), and add a causal regression proving a runtime path beyond the ceiling cannot yield false suite outputs. -
A PR-files API failure can skip every runtime required wrapper instead of producing a merge-blocking result. Moving selection to the API adds auth, rate-limit, and availability failure modes. The pinned action catches such failures and calls
core.setFailed; it does not fall back to git. The required wrappers are then guarded byneeds.changes.result == 'success'at.github/workflows/ci.yml:249-414, so a failed selector causes those jobs to be skipped. The active default-branch ruleset requires the wrapper contexts (Rust Lint,Unit Tests,Desktop,Mobile,Web,Security, and others), but does not requireDetect Changed Paths; skipped required jobs are merge-satisfying. An API failure can therefore suppress the runtime suites without creating a required failed context.Author action: make selector failure itself merge-blocking—either require
Detect Changed Pathsin the ruleset or ensure the required wrappers run and fail when selection did not succeed—and add a regression for selector failure → required failure rather than skip.
The intended happy path is otherwise well covered: the exact pinned action's local selector suite passed 28/28 at 192c1cf2fbca0a242a007a63913f08ad0c01a665; root/docs Markdown, runtime-directory Markdown, mixed code/docs, synthetic-merge contamination, and embedded Markdown assets behave as intended. Renames/deletions are conservatively handled by the pinned action. Fork read permissions and push behavior also look correct. The remaining fixture gaps (multi-page pagination and explicit renamed/deleted/fork rows) are confidence gaps, not additional blockers.
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — REQUEST CHANGES at exact head 192c1cf2fbca0a242a007a63913f08ad0c01a665 (base 0ef7a2222ef4e9b221f310ac60ca6a535d9c840d).
Two concrete fail-open defects must be fenced before this can safely control required-suite selection.
-
PR file enumeration silently truncates after 3,000 files, allowing runtime changes beyond the cap to be classified as docs-only. The new token at
.github/workflows/ci.yml:40makes the pinned path-filter action usepulls.listFiles. GitHub caps that endpoint at 3,000 files; the action neither comparespull_request.changed_filesnor detects truncation. The incomplete result directly gates runtime jobs at.github/workflows/ci.yml:146-246. An exact-action probe with 3,000 returned documentation paths and the only Desktop path omitted beyond the cap produceddesktop=false.Author action: fail closed when
github.event.pull_request.changed_files > 3000—conservatively select all runtime suites, reject selection, or use an uncapped exact diff—and add a causal regression proving an omitted runtime tail cannot yield false suite outputs. -
PR-files API failure skips every runtime required wrapper instead of creating a merge-blocking result. API auth, rate-limit, or availability errors make the pinned action fail; it has no git fallback. Required wrappers are guarded by
needs.changes.result == 'success'at.github/workflows/ci.yml:249-414, so selector failure skips them. The active default-branch ruleset requires wrapper contexts such asRust Lint,Unit Tests,Desktop,Mobile,Web, andSecurity, but notDetect Changed Paths; skipped required jobs are merge-satisfying.Author action: make selector failure merge-blocking—require
Detect Changed Paths, or make required wrappers run and fail when selection did not succeed—and add a regression proving selector failure produces a required failure rather than skipped contexts.
Verification owner: author for both fixes and regressions; reviewer for exact-head mutation/ruleset re-verification.
The intended happy path is otherwise well covered: the exact pinned action's selector suite passed 28/28; root/docs Markdown, runtime-directory Markdown, mixed code/docs, synthetic-merge contamination, and embedded Markdown assets behave as intended. Renames/deletions are conservative, fork read permissions look correct, and push behavior is preserved. Multi-page pagination and explicit renamed/deleted/fork fixture rows remain confidence gaps, not additional blockers.
Any new head requires a fresh review.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested: one P2 correctness regression. Reviewed head 192c1cf2fbca0a242a007a63913f08ad0c01a665 against base 0ef7a2222ef4e9b221f310ac60ca6a535d9c840d.
I independently confirm the existing 3,000-file truncation finding at .github/workflows/ci.yml:40. The pinned action’s shipped API enumerator has no completeness guard. This is a regression from tokenless Git selection, not a request to expand the filters.
Merge criterion: oversized PRs must conservatively run all runtime suites, use a complete immutable diff, or fail a genuinely required check. Add a causal regression with 3,000 returned documentation paths and an omitted runtime change. Merely failing Detect Changed Paths is insufficient: the current main rules require the downstream wrappers, which skip when changes fails, not that selector job. That failure-gate policy predates this PR; I am not counting it as a separate new defect.
The existing rename-source warning is incorrect for this pin: the shipped action preserves both paths.
Source-only review of workflow integration and regression fixtures. No tests or PR/action code executed; existing exact-head CI is still incomplete.
|
@codex review |
|
@builderbot review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
@builderbot review |
|
@buzz-security-review 8a31ac4 |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
No blocking source defects at 8a31ac4589f7fe328a63adc7c79ab93522888d4f, against 930b8bb800d8149ce29a881ba4c5d9f424580434. The previous 3,000-file blocker is fixed: oversized PRs select every runtime output. Selector failure now fails the required wrappers, while successful selection retains existing path coverage. One non-blocking regression-test gap is noted inline.
Existing selector CI passed 61/61 tests, including the API ceiling, API denial and all 13 wrapper scenarios. CI’s merge e5c1a71e2fdef19580763513021384d44ccc6cb0 has the same tree as this head.
Remaining merge gate: Desktop Smoke E2E shard 3 failed persistent-agent-audience.spec.ts:612 on all three attempts; the required Desktop check correctly failed. Desktop source and reusable workflows are unchanged by this diff, but the failure is not proven flaky and needs author/CI-owner resolution.
Source-only review, not approval or a green-CI verdict. No checkout, build, test execution, or CI rerun. Unrelated existing path-coverage gaps remain outside this review.
|
@codex review |
Signed-off-by: Tom Brow <tomb@block.xyz>
Signed-off-by: Tom Brow <tomb@block.xyz>
Signed-off-by: Tom Brow <tomb@block.xyz>
Signed-off-by: Tom Brow <tomb@block.xyz>
Signed-off-by: Tom Brow <tomb@block.xyz>
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…avior Signed-off-by: Tom Brow <tomb@block.xyz>
90d0761 to
a6d0474
Compare
|
@codex review |
|
@buzz-security-review a6d0474 |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
## Summary Fix the upload/edit smoke-test race observed on main at `6410e685a80d42db0645fadc9bbe710559ef911e`. - Hold the mock upload at the Tauri IPC boundary and release it explicitly after checking edit rejection, rather than assuming menu interaction finishes within one second. - Wait for the Radix menu to unmount: edit dispatch happens in `onCloseAutoFocus`, after the click. Without that wait, the negative assertion can pass before the edit callback actually runs. - Keep the existing attachment-preservation and subsequent successful-edit assertions. No production code or CI coverage is changed. ## Evidence and scope - [Main failure](https://github.com/block/buzz/actions/runs/36189213149/job/108250341388): `opening edit during an immediate photo upload preserves the draft`, including an upload-progress timeout on retry. - Investigated from #7791, which changes only `docs/nips/NIP-AR.md`. Its PostgreSQL failures and video-menu timeout are separate; PostgreSQL passed on this main run, and the video spec passed locally. This PR does not claim to fix those failures or the separate pointer-interception failure seen in the main job. - #7843 already addresses unrelated runtime suites being selected for docs-only PRs. ## Validation - Full desktop `pnpm test`: 6,676 Node tests + 92 jsdom tests passed. - Upload/edit regression repeated 5 times: passed. Holding the upload without waiting for menu teardown failed all 5 runs, demonstrating the second race. - Complete file-attachment and video-attachment Playwright specs: 30 passed. - Pre-push desktop lint, typecheck, full desktop tests, and file-size gate passed at `46e3d4392e5b56dcb74ea864aa19aa10287f27d3`. - `just ci` attempted: initial formatting issue corrected; second run exceeded the 5-minute local limit during Tauri clippy. Not claiming full repository CI green. ## Review / human verification Self-reviewed the diff against the test contract; no production behavior changes. Draft pending human verification and CI. To verify: activate Hermit, then `cd desktop && pnpm test:e2e:smoke file-attachment.spec.ts --grep "opening edit during" --repeat-each=5`. Expect all five runs to pass, retaining the upload in the draft and allowing edit only after upload release. Originating conversation: buzz://message?channel=aa7f2b48-d367-4ca1-be09-116a2eb45b99&id=1b68585ba4483a87f5a453fd7bbdd11d434e332dcb4aff8acfe1f5d2601b6561 Signed-off-by: Smartie <fb3185faacfc3760b6b4fe0085a8878d8dd537f7a7671c7d3184fe1aa1b04df4@buzz.block.builderlab.xyz> Co-authored-by: Smartie <fb3185faacfc3760b6b4fe0085a8878d8dd537f7a7671c7d3184fe1aa1b04df4@buzz.block.builderlab.xyz>
Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com> * commit '781d39510': feat(push): support configurable HTTP(S) delivery URLs (#7877) fix(ci): select runtime suites from PR changes only (#7843) test(desktop): synchronize upload edit smoke test (#7903) Signed-off-by: Hayt <211b96e6a2b7f45fd4047988976c7bbbeeda0c15f3ae7b32eec20834b5a55118@buzz.block.builderlab.xyz>
Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com> * origin/main: fix(relay): fail startup on invalid operator listener config (#7933) fix(db): limit event transactions to listener mention kinds (#7932) feat(relay): deliver pubkey mentions to relay companions (#7793) docs(protocol): propose simplified channel artifacts (#7791) feat(push): support configurable HTTP(S) delivery URLs (#7877) fix(ci): select runtime suites from PR changes only (#7843) test(desktop): synchronize upload edit smoke test (#7903) Signed-off-by: Hayt <211b96e6a2b7f45fd4047988976c7bbbeeda0c15f3ae7b32eec20834b5a55118@buzz.block.builderlab.xyz>
* origin/main: 🤖 docs(nip-fi): remove implementation references from the spec (#7912) fix(relay): fail startup on invalid operator listener config (#7933) fix(db): limit event transactions to listener mention kinds (#7932) feat(relay): deliver pubkey mentions to relay companions (#7793) docs(protocol): propose simplified channel artifacts (#7791) feat(push): support configurable HTTP(S) delivery URLs (#7877) fix(ci): select runtime suites from PR changes only (#7843) test(desktop): synchronize upload edit smoke test (#7903) Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
A documentation-only update to #7809 selected desktop builds and E2E tests because path detection compared an old PR base SHA with GitHub's newer synthetic merge commit. The failing run's path-detection log includes four unrelated desktop files from
main;VISION_MOBILE.mddid not match a runtime filter.Use GitHub's PR file list for pull requests so unrelated changes in the synthetic merge commit cannot select runtime suites. The existing directory filters are unchanged: root and docs/ Markdown do not select runtime suites, while Markdown under runtime directories still selects its affected suites. Mixed code/documentation changes retain normal coverage. Always-on security, policy, and source-contract checks and full push-to-main coverage are unchanged.
Added regression coverage runs the pinned paths-filter action against real fixture repositories, including a synthetic merge containing unrelated upstream desktop code. All 35 selection scenarios and 26 required-check scenarios pass; the new regressions failed before the fix. Existing required-context isolation, file-size policy, and security-review contract checks pass, as do script lint and workflow syntax validation. Full workflow lint reports the same two pre-existing shell-quoting findings in the untouched dead-token guard. A desktop E2E build was run to diagnose the unrelated required smoke failure.
Related: #7809 (incident, unchanged) and #5756 (shared-input path coverage, separate scope). No matching issue found.
PRs with at least 3,000 changed files now select every runtime suite, avoiding GitHub's PR-file-list ceiling. Boundary tests cover 2,999, 3,000, and 3,001 files, including a runtime file omitted beyond the API cap; disabling the safeguard makes the latter two regressions fail.
Required checks now run and fail when path selection fails, is cancelled, or is skipped. Regression coverage exercises the workflow conditions and shell checks for all 13 required wrappers, plus an API-denial case against the pinned action. Mutating either the scheduling guard or the result check makes all 13 failure regressions fail.
The required mention-settings smoke test expected a pin after explicitly disabling automatic mentions. Waiting for its old avatar to exit reproduced the CI failure 3/3; the test now checks that subsequent mentions remain manual and retains outgoing recipient-tag assertions. Corrected browser regression: 20/20 repeated runs; related picker unit tests: 18/18. No desktop production behavior changed. The always() requirement is now pinned in required-wrapper regression tests; deleting it fails all 26 gate cases.