Skip to content

test: repair lifecycle and descriptor cleanup contracts - #9293

Closed
austinywang wants to merge 2 commits into
mainfrom
fix-ci-lifecycle-test-contracts
Closed

austinywang wants to merge 2 commits into
mainfrom
fix-ci-lifecycle-test-contracts

Conversation

@austinywang

@austinywang austinywang commented Jul 31, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Update the Dock runtime-transfer fixture to reflect that clearing an agent status intentionally clears its lifecycle, then re-report .running before asserting lifecycle transfer.
  • Keep the command-runner descriptor test focused on deterministic cleanup contracts: capture descriptors are closed and the Process termination handler is cleared when run() returns.
  • Preserve Lawrence Chen's authorship by cherry-picking the existing focused commits from cmux-tui: render inline Kitty images through libghostty #8811 into a dedicated current-main repair.

No production code changes.

Why

Current main has two independent full-CI failures that are unrelated to the feature branches being tested:

  • AgentNotificationMutationBoundaryTests.swift: the fixture re-added only the visible status after a status clear, then incorrectly expected the intentionally-cleared lifecycle to transfer.
  • CommandRunnerDescriptorLifecycleTests.swift: a weak Foundation.Process assertion depended on asynchronous NSConcreteTask deallocation timing instead of the file-descriptor cleanup contract.

These failures blocked full CI for #9266 after its own focused red/green proof passed.

Evidence

Validation

  • git diff --check origin/main...HEAD
  • ./scripts/lint-pbxproj-test-wiring.sh — passed (642 files)
  • Focused lifecycle suite: run 30617539181 — passed (40 tests; the repaired Dock-owned terminal lifecycle test passed).

View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.


Summary by cubic

Repair failing CI tests by fixing agent lifecycle transfer expectations and making command runner cleanup assertions deterministic. No production code changes.

  • Bug Fixes
    • Dock runtime transfer test: after a status clear (which also clears lifecycle), re-reports .running and asserts lifecycle state before verifying transfer.
    • Command runner test: removes ARC-timing check on a weak Process; now asserts capture FDs are closed and process.terminationHandler is nil after await run() returns.

Written for commit 75b29be. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • Tests
    • Expanded coverage for command execution cleanup, including descriptor closure and termination-handler reset after successful completion.
    • Added validation that clearing agent status also clears its lifecycle state.
    • Verified that restoring agent status and applying a lifecycle update correctly returns the status to a running state.

@cursor

cursor Bot commented Jul 31, 2026

Copy link
Copy Markdown

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

@coderabbitai

coderabbitai Bot commented Jul 31, 2026 •

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The PR updates two lifecycle test suites. It verifies command descriptor cleanup after execution and agent lifecycle-state cleanup and restoration during status mutations.

Changes

Command descriptor lifecycle

Layer / File(s) Summary
Retained execution cleanup assertions
Packages/macOS/CmuxFoundation/Tests/CmuxFoundationTests/Process/CommandRunnerDescriptorLifecycleTests.swift
The test retains CommandExecution, runs it directly, verifies descriptor closure, and checks that the process termination handler is cleared.

Agent notification lifecycle

Layer / File(s) Summary
Agent status lifecycle mutations
cmuxTests/AgentNotificationMutationBoundaryTests.swift
The test verifies lifecycle-state removal when omp status is cleared and running-state restoration after status re-upsert.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related issues

  • manaflow-ai/cmux-dev-artifacts#7167 — The test adds lifecycle-state restoration coverage for Dock-owned terminal agent notifications.
  • manaflow-ai/cmux-dev-artifacts#7126 — The test covers the .running lifecycle-state regression described in the issue.

Possibly related PRs

🚥 Pre-merge checks | ✅ 24 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (24 passed)
Check name Status Explanation
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 The diff changes only two Swift test files and no production paths; the actor-isolation rule explicitly allows test changes.
Cmux Swift Blocking Runtime ✅ Passed The diff changes only two test files. Added lines use no prohibited blocking or timing primitives, and no production Swift code changes exist.
Cmux Browser Automation Off-Main ✅ Passed The PR changes only two lifecycle test fixtures; no browser socket automation, worker routing, WebKit/AppKit access, or browser-state mutation is present.
Cmux Expensive Synchronous Load ✅ Passed The PR changes only two test files; no production Swift paths changed, so the expensive synchronous production-load check is not applicable.
Cmux Cache Substitution Correctness ✅ Passed The diff contains only two Swift test files and makes no production cache, persistence, history, undo, or snapshot substitution.
Cmux No Hacky Sleeps ✅ Passed The diff changes only two Swift test files; the rule covers non-Swift production/runtime code, and no covered sleep or timer was introduced.
Cmux Algorithmic Complexity ✅ Passed The PR changes only two test files. The complexity rules explicitly pass test-only scaffolding, and no production code or scalable runtime algorithm changed.
Cmux Swift Concurrency ✅ Passed The diff changes only two XCTest files. Added async usage awaits existing APIs; it adds no Dispatch, Combine, completion-handler, or fire-and-forget Task pattern.
Cmux Swift @Concurrent ✅ Passed The PR changes only tests. It adds no @concurrent, nonisolated, or actor-isolated declarations; the existing async run call remains outside UI isolation and only changes ownership handling.
Cmux Swift Package Boundaries ✅ Passed The diff changes only two test files; it introduces no production Swift feature logic or app-target code that could violate the package boundary.
Cmux Swiftpm Lockfiles ✅ Passed The PR changes only two Swift test files; it changes no Package.swift, Package.resolved, .gitignore, workflow, or Xcode package-reference files.
Cmux Swift Logging ✅ Passed The PR changes only Swift tests. Added lines contain no print, debugPrint, dump, NSLog, file logging, Logger, or sensitive-data diagnostics.
Cmux User-Facing Error Privacy ✅ Passed The diff changes only two Swift test files; the repository rule explicitly allows developer-only tests, with no production user-facing error changes.
Cmux Full Internationalization ✅ Passed The diff changes only two test files. The internationalization rule explicitly allows tests, and no production or user-facing text or locale data changed.
Cmux Swiftui State Layout ✅ Passed The PR changes only two Swift test files, with no SwiftUI imports or state/layout constructs in added lines; the SwiftUI state-layout rule is not applicable.
Cmux Architecture Rethink ✅ Passed The diff changes only two tests. It adds direct lifecycle assertions and a lifecycle re-report, with no sleeps, polling, locks, observers, side channels, duplicate wiring, or split UI ownership.
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed The PR changes only two test files, adds no auxiliary window code, and scripts/lint_auxiliary_window_close_shortcuts.py passes.
Cmux Source Artifacts ✅ Passed The PR changes only two tracked Swift test source files; no artifact-like paths, binaries, logs, caches, build output, or scratch directories appear in the diff.
Cmux No Test Or Debug Seam In Production Source ✅ Passed The PR changes only two test files; no Swift file under a production Sources/ path changed, so it adds no production test/debug seam.
Cmux No Ambient Global State ✅ Passed The pull request changes only two test files and adds no production Swift code or new ambient global state; the rule applies to production Swift changes.
Title check ✅ Passed The title clearly and concisely identifies the lifecycle and descriptor cleanup test repairs.
Description check ✅ Passed The description provides detailed changes, rationale, evidence, and validation, but omits the template checklist and review-trigger section.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-ci-lifecycle-test-contracts

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 lawrencecchen added the stale-revisit Closed after 30+ days without activity; preserved for possible revisit or reopening. label Sep 23, 2026
@github-project-automation github-project-automation Bot moved this from Todo to Done in cmux backlog Sep 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale-revisit Closed after 30+ days without activity; preserved for possible revisit or reopening.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants