Skip to content

Fix integration review follow-ups - #8948

Merged
lawrencecchen merged 4 commits into
task-integrate-web-determinism-repairsfrom
fix-integration-review-followups
Jul 26, 2026
Merged

lawrencecchen merged 4 commits into
task-integrate-web-determinism-repairsfrom
fix-integration-review-followups

Conversation

@lawrencecchen

@lawrencecchen lawrencecchen commented Jul 26, 2026 •

Copy link
Copy Markdown
Contributor

Repairs the exact-head Bun review finding on #8941.

  • Rejects Bun versions before 1.3.14, where --isolate still leaks process-global mocks.
  • Pins the web required check to Bun 1.3.14.

The first commit is test-only and fails on the parent head. The second commit makes it pass.

Validation:

  • bash tests/test_ci_self_hosted_guard.sh
  • Bun 1.2.14 runner rejection
  • Bun 1.3.14 web suite: 890 pass, 122 skip, 0 fail
  • bun run typecheck

@coderabbitai

coderabbitai Bot commented Jul 26, 2026 •

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@lawrencecchen, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 10 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 595d29db-de3d-47da-a0ee-f133c238abd8

📥 Commits

Reviewing files that changed from the base of the PR and between 2f4eb5a and 7a26db9.

📒 Files selected for processing (3)
  • .github/workflows/ci.yml
  • tests/test_ci_self_hosted_guard.sh
  • web/scripts/run-tests.sh
📝 Walkthrough

Walkthrough

The web test runner now requires Bun 1.3.14 or newer, with CI and self-hosted guard coverage. Notification history persistence no longer awaits the persistence call in schedulePersistence().

Changes

Bun version enforcement

Layer / File(s) Summary
Minimum Bun version guard
web/scripts/run-tests.sh
Validates bun --version against 1.3.14 and rejects older versions before running tests.
CI pin and regression coverage
tests/test_ci_self_hosted_guard.sh, .github/workflows/ci.yml
Adds configurable Bun version mocking, rejects version 1.2.14 in regression coverage, and pins the typecheck job to Bun 1.3.14.

Notification persistence scheduling

Layer / File(s) Summary
Persistence invocation change
Sources/NotificationFeedHistoryStore.swift
Removes await from the persistence call within schedulePersistence().

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant run-tests.sh
  participant Bun CLI
  participant Bun test suite
  run-tests.sh->>Bun CLI: Request bun --version
  Bun CLI-->>run-tests.sh: Return installed version
  run-tests.sh->>run-tests.sh: Compare version with 1.3.14
  run-tests.sh->>Bun test suite: Run bun test --isolate when supported
Loading

Possibly related PRs

Suggested reviewers: azooz2003-bit

🚥 Pre-merge checks | ✅ 24 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description has summary and testing, but it omits required template sections like Review Trigger and Checklist. Add the missing template sections, especially Review Trigger and Checklist, and include a Demo Video link if this change affects behavior.
✅ Passed checks (24 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Cmux Swift Actor Isolation ✅ Passed Store remains explicitly @MainActor, persistence stays actor-isolated, and the PR adds no new shared-mutable or background-access isolation debt.
Cmux Swift Blocking Runtime ✅ Passed No new semaphores, sleeps, sync waits, or locks appear in the Swift change; the persistence path stays actor-owned and async.
Cmux Browser Automation Off-Main ✅ Passed Diff only updates CI Bun pinning and web test version gating; no browser-automation routing or main-actor browser commands were changed.
Cmux Expensive Synchronous Load ✅ Passed PR diff only changes tests and workflow/toolchain; no production Swift loader was added or moved, and the existing nil-guarded load call is untouched.
Cmux Cache Substitution Correctness ✅ Passed The only production change is a no-op await removal; no authoritative read was replaced by a stale or cached value in a persistence/history path.
Cmux No Hacky Sleeps ✅ Passed No fixed sleeps or polling were added to the changed runtime scripts; workflow sleeps are out of scope, and the new test timeout is bounded test scaffolding.
Cmux Algorithmic Complexity ✅ Passed PASS: The production diff only adds Bun version gating and removes an await; no scalable collection gets a new nested scan or repeated rescan, and the file-scan lives in a test harness.
Cmux Swift Concurrency ✅ Passed The Swift change only tweaks an existing stored Task-backed persistence loop; it doesn’t add Dispatch queues, Combine, completion handlers, or unmanaged fire-and-forget lifecycle.
Cmux Swift @Concurrent ✅ Passed No Swift files are changed in the PR diff (HEAD~2..HEAD), so the @concurrent/nonisolated-async rule is not applicable.
Cmux Swift Package Boundaries ✅ Passed The Swift change is a one-line await removal inside existing app-target store code; it doesn't introduce or expand reusable domain logic beyond a package boundary.
Cmux Swiftpm Lockfiles ✅ Passed PR changes touch Bun workflow/web scripts and Swift code only; no Package.swift, Package.resolved, .gitignore, or Xcode project lockfile changes are present.
Cmux Swift Logging ✅ Passed The production Swift diff only removes an unnecessary await; it adds no print/debugPrint/dump/NSLog/Logger usage or sensitive logging.
Cmux User-Facing Error Privacy ✅ Passed PASS: The new Bun error/output is confined to developer/test scripts and CI; no production user-facing error text or sensitive details were added.
Cmux Full Internationalization ✅ Passed Only CI/test script changes were introduced; no user-facing Swift/UI/web copy or locale catalogs were added or altered.
Cmux Swiftui State Layout ✅ Passed The PR’s SwiftUI file change in ContentView is only an initializer default-arg tweak; no new ObservableObject/@Published/GeometryReader/lazy-row store refs or render-time state writes appear in the...
Cmux Architecture Rethink ✅ Passed PASS: the inspected PR delta has no Swift file changes; the only Swift persistence code remains unchanged and still keeps the actor as source of truth.
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed PASS: The PR only changes ContentView/ClosedWorkspaceHistory signatures and NotificationFeedHistoryStore persistence; no standalone window, cmux.* identifier, or cmuxAuxiliaryWindowIdentifiers chan...
Cmux Source Artifacts ✅ Passed All changed paths are intentional source/config/test/fixture files; no logs, caches, temp dirs, build output, or artifact paths were added.
Cmux No Test Or Debug Seam In Production Source ✅ Passed Touched Sources files only change DI/concurrency details; no new #if DEBUG, test-only member, or visibility-widening seam appears in the diff.
Cmux No Ambient Global State ✅ Passed The Swift changes only adjust instance methods, injected defaults, and a static constant; no new top-level func, mutable global, or new singleton was added.
Title check ✅ Passed The title is concise and accurately reflects the main theme of the changes.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-integration-review-followups

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.

@lawrencecchen

Copy link
Copy Markdown
Contributor Author

@codex review

@lawrencecchen

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 26, 2026 •

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@greptile-apps

greptile-apps Bot commented Jul 26, 2026 •

Copy link
Copy Markdown
Contributor

Greptile Summary

Pins the required web test job to Bun 1.3.14 and enforces that minimum version in the shared test runner.

  • Adds deterministic rejection coverage for Bun 1.2.14 and 1.3.13.
  • Keeps the workflow version aligned with the runner’s isolation boundary.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
.github/workflows/ci.yml Pins the web test job to Bun 1.3.14, matching the runner’s new minimum.
tests/test_ci_self_hosted_guard.sh Extends the test-runner fixture and verifies rejection of Bun versions below 1.3.14.
web/scripts/run-tests.sh Adds an early semantic-version check that prevents the suite from running on Bun releases with process-global mock leakage.

Reviews (4): Last reviewed commit: "test: rely on executed Bun version guard" | Re-trigger Greptile

break
}
await persistence.persist(snapshot)
persistence.persist(snapshot)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Preserve the explicit actor hop

persist is an actor-isolated method called from the main-actor task, so removing await leaves an implicit cross-actor hop that emits a concurrency warning under the current Swift language mode and becomes a compile error under Swift 6. Keep the explicit await on this call.

File Used: .github/review-bot-rules/swift-actor-isolation.md (source)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2f4eb5a5e3

ℹ️ 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".

Comment thread tests/test_ci_self_hosted_guard.sh
@lawrencecchen
lawrencecchen force-pushed the fix-integration-review-followups branch from 2f4eb5a to 7afcfd0 Compare July 26, 2026 02:46
@lawrencecchen

Copy link
Copy Markdown
Contributor Author

@greptileai review\n@codex review

@lawrencecchen

Copy link
Copy Markdown
Contributor Author

@codex review exact head 7afcfd0

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 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 `@tests/test_ci_self_hosted_guard.sh`:
- Around line 890-909: Extend the shared web test runner rejection checks to
also execute with CMUX_WEB_TEST_RUNNER_BUN_VERSION set to 1.3.13 and fail if it
is accepted. Keep the existing 1.2.14 rejection and 1.3.14 web-typecheck pin
assertion, ensuring the suite verifies the patch-level lower boundary.
🪄 Autofix (Beta)

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: ASSERTIVE

Plan: Pro Plus

Run ID: b7b31094-ba83-47bb-a55f-9f829d7e2891

📥 Commits

Reviewing files that changed from the base of the PR and between b4a1ab2 and 2f4eb5a.

📒 Files selected for processing (4)
  • .github/workflows/ci.yml
  • Sources/NotificationFeedHistoryStore.swift
  • tests/test_ci_self_hosted_guard.sh
  • web/scripts/run-tests.sh

Comment thread tests/test_ci_self_hosted_guard.sh Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7afcfd0bd5

ℹ️ 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".

Comment thread tests/test_ci_self_hosted_guard.sh Outdated
@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Exact head is now 0e9a1794d4e3783d1f6da99e9e28bfeb36b2a803. Added the requested patch-boundary guard for Bun 1.3.13; the full workflow guard passes.

@codex review
@greptileai review
@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 26, 2026 •

Copy link
Copy Markdown

@lawrencecchen: Reviewing the updated exact head, including the Bun 1.3.13 patch-boundary guard.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Exact head is 7a26db9e779edbacbbf4f95b034bdbd05aeb685f. The metadata-only workflow assertion is removed; the executed runner fixture enforces Bun 1.2.14 and 1.3.13 rejection, and the required web job runs the same guarded entrypoint. All exact-head threads are resolved.

@codex review
@greptileai review
@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 26, 2026 •

Copy link
Copy Markdown

@lawrencecchen: Reviewing exact head 7a26db9e779edbacbbf4f95b034bdbd05aeb685f, including the executed Bun-version rejection fixture and guarded required web entrypoint.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@lawrencecchen
lawrencecchen merged commit 20ee080 into task-integrate-web-determinism-repairs Jul 26, 2026
14 checks passed

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7a26db9e77

ℹ️ 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".

Comment on lines +899 to +903
if PATH="$fixture_dir/bin:/usr/bin:/bin" \
CMUX_WEB_TEST_RUNNER_ARGS_LOG="$args_log" \
CMUX_WEB_TEST_RUNNER_BUN_VERSION="1.3.13" \
/bin/bash "$fixture_runner" >/dev/null 2>&1; then
echo "FAIL: shared web test runner must enforce the patch-level Bun isolation boundary"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Put the patch-boundary test before the fix

The 1.3.13 regression case first appears in commit 0e9a1794, after commit 7afcfd0b has already implemented the minimum-version guard, so the series never demonstrates that this precise patch-level test fails without the fix; the earlier 1.2.14 case could still pass with an incorrect major/minor-only guard. Move this boundary assertion into the test-only commit before the implementation so CI records the required red-to-green transition.

AGENTS.md reference: AGENTS.md:L127-L134

Useful? React with 👍 / 👎.

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.

1 participant