Skip to content

Keep sidebar metadata cap strictly priority-ordered (#5845 follow-up) - #5879

Closed
austinywang wants to merge 12 commits into
mainfrom
fix-5845-metadata-cap-priority
Closed

austinywang wants to merge 12 commits into
mainfrom
fix-5845-metadata-cap-priority

Conversation

@austinywang

@austinywang austinywang commented Jun 11, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Follow-up to #5845 / #5855. The sidebar status/metadata cap added a "just-inserted grace tier" so a brand-new status entry survives its own synchronous trim long enough for the set_status --pid PID-coupling handoff to mark it live. That tier was also applied to the metadata trim for symmetry — but metadata blocks have no such follow-up coupling, so the tier is wrong there: at the 200-block cap, a newer low-priority block displaces an existing high-priority one, contradicting priority as the retention signal defined by sidebarMetadataBlocksInDisplayOrder() and the public metadata API.

(Caught by the $autoreview codex gate after #5855 had already merged.)

Fix

Drop the grace tier from trimSidebarMetadataBlocksIfNeeded(); metadata now evicts strictly by priority then timestamp (storage-key keyed). Status entries keep their grace tier, which they genuinely need.

Tests

Two-commit red/green (cmuxTests/WorkspaceSidebarObservationTests.swift):

  • Commit 1 (red): testMetadataCapRetainsHighPriorityOverNewerLowPriorityFlood — one high-priority block plus a 2×cap newer low-priority flood; asserts the high-priority block survives. Fails against the grace-tier behavior on main.
  • Commit 2 (green): the trim fix.

Uses only pre-existing symbols + literals.

🤖 Generated with Claude Code


View with Codesmith Autofix with Codesmith
Need help on this PR? Tag /codesmith with what you need. Autofix is disabled.


Summary by cubic

Keep sidebar metadata eviction strictly priority-ordered and cap both status and metadata at 200 entries. Status trim is tiered, applies grace only to set_status --pid, preserves reserved cmux keys and live-agent statuses, and clears agent runtime on eviction; detached adoption avoids orphan PIDs (including dotted keys).

  • Bug Fixes

    • Metadata: enforce 200-cap on write in WorkspaceSidebarMetadataModel; evict by priority > timestamp > key; no just-inserted tier.
    • Status: synchronous trim on every write with tiers — reserved cmux keys (remote.error, remote.port_conflicts), live agent-backed (PID or lifecycle), just-inserted PID-handoff keys, then priority/timestamp — so new agent statuses survive their own trim and plain telemetry can’t displace them.
    • Runtime/adoption: added recordAgentPIDForSurvivingStatusKey; cap evictions purge PID/ownership/lifecycle state and refresh ports; detached adoption records PID/ownership only if the adopted status survives and skips creating dotted-key orphans.
  • Refactors

    • Refreshed Swift file length budget.

Written for commit b871b6f. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • Bug Fixes
    • Fixed metadata block retention in the sidebar to properly prioritize high-priority entries over newly added low-priority blocks when capacity limits are reached.

austinywang and others added 2 commits June 11, 2026 04:14
…ity flood

The #5845 status cap gave metadata blocks a just-inserted grace tier intended
only for status entries' set_status --pid handoff. On metadata that tier lets a
newer low-priority block displace an existing high-priority one at the cap,
contradicting priority as the retention signal in
sidebarMetadataBlocksInDisplayOrder(). This test fails against that behavior.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…5845)

Metadata blocks have no follow-up PID/lifecycle coupling that must survive its
own trim (the reason status entries have a just-inserted grace tier), so eviction
must rank strictly by priority then timestamp. Drop the grace tier from the
metadata trim so a newer low-priority block can no longer displace an existing
high-priority one. Follow-up to the #5845 cap (PR #5855), which applied the tier
to metadata for symmetry.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@vercel

vercel Bot commented Jun 11, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
cmux Ready Ready Preview, Comment Jun 20, 2026 1:01am
cmux-staging Building Building Preview, Comment Jun 20, 2026 1:01am

@coderabbitai

coderabbitai Bot commented Jun 11, 2026 •

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The PR refines metadata block trimming by removing a grace-tier mechanism that retained newly inserted entries, instead relying solely on priority and timestamp ordering. The metadataBlocks update no longer passes previous keys to the trim method, simplifying the retention logic. A new test validates that high-priority entries are preserved when lower-priority entries flood the workspace.

Changes

Metadata Block Trimming Logic Refinement

Layer / File(s) Summary
Trimming logic and didSet update
Sources/Workspace.swift
trimSidebarMetadataBlocksIfNeeded removes "just-inserted grace tier" logic and now trims by priority and timestamp ordering alone; metadataBlocks didSet calls it without passing previousKeys.
High-priority retention test
cmuxTests/WorkspaceSidebarObservationTests.swift
New test testMetadataCapRetainsHighPriorityOverNewerLowPriorityFlood verifies high-priority blocks survive eviction when flooded by newer low-priority entries within the cap limit.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

  • manaflow-ai/cmux#5855: The main PR's change to Workspace.metadataBlocks trimming logic (removing the previous "just-inserted grace tier" and updating retention behavior) and its new flood-retention regression test directly overlap with the retrieved PR's metadata cap + trimming/ordering behavior in Sources/Workspace.swift and cmuxTests/WorkspaceSidebarObservationTests.swift.

Poem

🐰 A trim so clean, with grace removed,
Priority reigns where chaos brewed,
Old blocks of worth survive the flood,
Priority ordering cuts through mud! 🌾


Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (1 error, 1 warning)

Check name Status Explanation Resolution
Cmux Source Artifacts ❌ Error PR adds local tool artifacts: .claude/ (25+ files including process locks, commands, skills), .agents/, .cursor/, and .coderabbit.* files that should be in .gitignore, violating source-control-arti... Remove .claude/, .agents/, .cursor/ directories and .coderabbit.* files from commit, or add precise .gitignore patterns (e.g., .claude/, .agents/, .cursor/*, .coderabbit.) and don't commit them.
Docstring Coverage ⚠️ Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (19 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main change: removing the grace tier from sidebar metadata trimming to maintain strict priority ordering, with explicit reference to the follow-up nature (#5845 follow-up).
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 Changes occur within @MainActor final class Workspace; modify only private method logic for trimming a value-type dictionary, with no new shared mutable Sendable types or background context access...
Cmux Swift Blocking Runtime ✅ Passed PR changes only sorting/filtering logic without introducing blocking primitives. Production changes in Sources/Workspace.swift remove "just-inserted grace tier" logic via pure priority/timestamp so...
Cmux Expensive Synchronous Load ✅ Passed No expensive synchronous loaders added. Changes trim metadata blocks via lightweight in-memory sorting/filtering on @MainActor's didSet, with no disk I/O, syscalls, or RestorableAgentSessionIndex.l...
Cmux Cache Substitution Correctness ✅ Passed The PR changes trimming logic for in-memory metadataBlocks dictionary, not persistence/cache paths. metadataBlocks is not included in snapshots, not persisted, and not restored; the change does not...
Cmux No Hacky Sleeps ✅ Passed Custom check scope excludes Swift code; rule file explicitly states "Swift timing and blocking primitives are covered by swift-blocking-runtime.md". All PR changes are Swift files (Sources/Workspac...
Cmux Algorithmic Complexity ✅ Passed The metadata trim function operates on a bounded 200-item collection with O(n log n) complexity and no nested iteration patterns. All changes simplify the algorithm by removing grace-tier logic wit...
Cmux Swift Concurrency ✅ Passed No legacy async patterns introduced or expanded; PR removes grace-tier logic complexity and adds only an XCTest-scoped test with simple property assignments and XCTest assertions.
Cmux Swift @Concurrent ✅ Passed Changes are synchronous @MainActor-bound operations on dictionary sorting/filtering with no async work, @concurrent annotations, or actor isolation violations.
Cmux Swift File And Package Boundaries ✅ Passed Small focused bug fix: Workspace.swift +8/-9 (net -1), only existing oversized file touched incidentally. Test code addition in cmuxTests is allowed. Preserves clear extraction path and coherent re...
Cmux Swift Logging ✅ Passed No Swift logging violations found in the diff. The modified trimSidebarMetadataBlocksIfNeeded() function and new test contain no print, debugPrint, dump, NSLog, file I/O, or secret exposure.
Cmux User-Facing Error Privacy ✅ Passed PR contains only developer-facing changes: internal implementation logic in production code (metadata trim ordering) and developer doc comments, plus test code. No user-facing error messages, alert...
Cmux Full Internationalization ✅ Passed Changes are limited to a private function's implementation and doc comment (developer-only), plus test code, with no user-facing text or localization-requiring changes.
Cmux Swiftui State Layout ✅ Passed PR only touches existing legacy Workspace ObservableObject state via didSet lifecycle callback to remove grace tier from metadata trim logic. No new @Published/@observable properties, no GeometryRe...
Cmux Architecture Rethink ✅ Passed Small correctness fix removing grace-tier logic from metadata trim; no timing patterns, observers, locks, or split lifecycle added; clear invariant (priority > timestamp) and single owner (Workspac...
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed PR does not add or materially change any NSWindow, NSPanel, NSWindowController, SwiftUI Window, or WindowGroup code; changes are limited to metadata block trimming logic and test-only fixtures.
Description check ✅ Passed PR description covers the core fix and testing approach clearly, but omits several template sections including manual testing verification, demo video, and review trigger commands.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-5845-metadata-cap-priority

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 and usage tips.

@greptile-apps

greptile-apps Bot commented Jun 11, 2026 •

Copy link
Copy Markdown
Contributor

Greptile Summary

This follow-up to #5845 fixes the sidebar metadata cap so it evicts entries strictly by priority then timestamp, removing the "just-inserted grace tier" that was mistakenly carried over from the status trim. Status entries keep the grace tier (needed for set_status --pid PID-coupling handoff); metadata blocks have no such coupling and now evict purely by priority.

  • Bug fix: cappedMetadataBlocksForDisplay in WorkspaceSidebarMetadataModel drops the grace tier entirely; trimSidebarStatusEntriesIfNeeded is moved to Workspace (where agent runtime context lives) and retains a four-tier sort: reserved cmux keys → live agent-backed → just-inserted PID-handoff → priority/timestamp.
  • Eviction cleanup: purgeAgentRuntimeState tears down coupled PID/ownership/lifecycle state for any status key evicted by the cap, preventing those runtime maps from growing unbounded alongside statusEntries.
  • Adoption hardening: adoptDetachedAgentRuntimeState now skips recording PIDs (and dotted-key PIDs) for adopted status entries that self-evict at the destination, preventing orphan PID map entries.

Confidence Score: 5/5

Safe to merge — the fix is narrowly scoped, the trim logic is well-reasoned, and all changed paths are exercised by new regression tests that were red before this commit.

The metadata cap now correctly evicts by priority then timestamp with no grace tier, matching the documented retention contract. The status cap retains its grace tier only for PID-handoff writes, backed by a guard-before-record wrapper that prevents orphan PID entries. Evicted status keys now clean up their coupled runtime state. The adoption path is hardened against self-evicting adopted statuses creating orphan PIDs, including the dotted-key variant. The test suite covers all four tiers, lifecycle cleanup, and adoption edge cases.

No files require special attention — the logic is localized and the tests are comprehensive.

Important Files Changed

Filename Overview
Packages/macOS/CmuxSidebar/Sources/CmuxSidebar/WorkspaceModel/WorkspaceSidebarMetadataModel.swift Core fix: drops the just-inserted grace tier from metadata trim and replaces it with a pure priority → timestamp → key sort; adds public maxMetadataBlocks constant.
Sources/Workspace.swift Moves status-entry trim to Workspace level (needs agentPIDs/lifecycle context); adds sidebarStatusPIDHandoffGraceKeys, setSidebarStatusEntry, replaceSidebarStatusEntries, and a four-tier trimSidebarStatusEntriesIfNeeded; exposes maxSidebarStatusEntries/maxSidebarMetadataBlocks constants.
Sources/Workspace+PanelLifecycle.swift Adds recordAgentPIDForSurvivingStatusKey (guard-before-record), statusKeysWithCoupledAgentRuntime, purgeAgentRuntimeState; refactors adoptDetachedAgentRuntimeState to merge via replaceSidebarStatusEntries and skip PIDs for self-evicted adopted statuses, including dotted-key orphan protection.
cmuxTests/WorkspaceSidebarObservationTests.swift Migrated from XCTestCase to Swift Testing; adds 11 new focused regression tests covering bounded metadata, status caps, live-agent retention, grace-tier semantics, lifecycle cleanup, and detached-adoption PID orphan prevention.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["statusEntries[key] = entry\n(Workspace computed setter)"] --> B["Capture previousKeys\n& pidHandoffGraceKeys\nClear graceKeys set"]
    B --> C["sidebarMetadata.statusEntries = newValue"]
    C --> D{"count > maxSidebarStatusEntries?"}
    D -- No --> Z["Done"]
    D -- Yes --> E["Build liveAgentStatusKeys\nfrom agentPIDs + lifecycle"]
    E --> F["Compute justInsertedPIDHandoffKeys\n= newKeys ∩ graceKeys"]
    F --> G["Sort all entries by tier:\n1. Reserved cmux keys\n2. Live agent-backed\n3. Just-inserted PID-handoff\n4. priority desc → timestamp desc → key asc"]
    G --> H["prefix(200) → kept set\nevictedKeys = all - kept"]
    H --> I{"evictedKeys empty?"}
    I -- Yes --> Z
    I -- No --> J["purgeAgentRuntimeState\n(clear PIDs, ownership, lifecycle\nfor evicted keys)"]
    J --> K["sidebarMetadata.statusEntries\n= statusEntries.filter kept\n(direct assign, no re-trim)"]
    K --> Z

    M["metadataBlocks[key] = block\n(WorkspaceSidebarMetadataModel didSet)"] --> N{"count > maxMetadataBlocks?"}
    N -- No --> P["metadataBlocksSubject.send"]
    N -- Yes --> O["cappedMetadataBlocksForDisplay:\nsort by priority desc → timestamp desc → key asc\nprefix(200)"]
    O --> Q["self.metadataBlocks = capped\n(re-triggers didSet once)"]
    Q --> P
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A["statusEntries[key] = entry\n(Workspace computed setter)"] --> B["Capture previousKeys\n& pidHandoffGraceKeys\nClear graceKeys set"]
    B --> C["sidebarMetadata.statusEntries = newValue"]
    C --> D{"count > maxSidebarStatusEntries?"}
    D -- No --> Z["Done"]
    D -- Yes --> E["Build liveAgentStatusKeys\nfrom agentPIDs + lifecycle"]
    E --> F["Compute justInsertedPIDHandoffKeys\n= newKeys ∩ graceKeys"]
    F --> G["Sort all entries by tier:\n1. Reserved cmux keys\n2. Live agent-backed\n3. Just-inserted PID-handoff\n4. priority desc → timestamp desc → key asc"]
    G --> H["prefix(200) → kept set\nevictedKeys = all - kept"]
    H --> I{"evictedKeys empty?"}
    I -- Yes --> Z
    I -- No --> J["purgeAgentRuntimeState\n(clear PIDs, ownership, lifecycle\nfor evicted keys)"]
    J --> K["sidebarMetadata.statusEntries\n= statusEntries.filter kept\n(direct assign, no re-trim)"]
    K --> Z

    M["metadataBlocks[key] = block\n(WorkspaceSidebarMetadataModel didSet)"] --> N{"count > maxMetadataBlocks?"}
    N -- No --> P["metadataBlocksSubject.send"]
    N -- Yes --> O["cappedMetadataBlocksForDisplay:\nsort by priority desc → timestamp desc → key asc\nprefix(200)"]
    O --> Q["self.metadataBlocks = capped\n(re-triggers didSet once)"]
    Q --> P
Loading

Reviews (10): Last reviewed commit: "merge: resolve conflicts with main" | Re-trigger Greptile

Comment thread cmuxTests/WorkspaceSidebarObservationTests.swift

@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 `@Sources/Workspace.swift`:
- Around line 14542-14543: Update the doc comment that currently reads "priority
is the sole retention signal" to accurately reflect the implementation: change
it to state that "priority is the primary retention signal, with timestamp as
the secondary criterion and dictionary key as the tiebreaker" so it matches the
sorting/eviction logic used in the grace-tier implementation (the code that
sorts by priority first, then timestamp, then dictionary key).
🪄 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

Run ID: 8c9a2ef7-f1c0-42f7-a8e0-3d6d638b29f2

📥 Commits

Reviewing files that changed from the base of the PR and between 48dd82b and 72b2861.

📒 Files selected for processing (2)
  • Sources/Workspace.swift
  • cmuxTests/WorkspaceSidebarObservationTests.swift

Comment thread Sources/Workspace.swift Outdated
@austinywang

Copy link
Copy Markdown
Contributor Author

Closing — not pursuing this approach anymore.

This branch was successfully deployed

1 active deployment
Preview – cmux — b871b6f4 Deployed Jun 20, 2026 by vercel[bot]
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