Repository navigation
Conversation
|
@comp615 is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds end-to-end Amp session support: agent modeling/presentation, hook-store indexing, optional title capture/persistence in CLI hook flows, transcript parsing alignment, localization and icon asset, tests, and Xcode wiring. ChangesAmp session support across CLI, index, and UI
Sequence Diagram(s)sequenceDiagram
participant AmpExtension
participant CmuxCLI
participant SessionIndexStore
participant HookStoreFile
AmpExtension->>CmuxCLI: emit prompt-submit / session-start (may include title)
CmuxCLI->>SessionIndexStore: parse hook input and update record (title optional)
SessionIndexStore->>HookStoreFile: read amp-hook-sessions.json
SessionIndexStore->>HookStoreFile: derive modified, normalize cwd
CmuxCLI->>SessionIndexStore: loadAmpEntries (search/pagination)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (17 passed)
✨ Finishing Touches🧪 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 |
Greptile SummaryAdds Amp (Sourcegraph Amp CLI) as a first-class agent in the Vault sidebar, reading sessions from cmux's own hook store (
Confidence Score: 5/5Safe to merge — all three bugs flagged in earlier rounds are confirmed fixed, and the new title pipeline is well-guarded against late async races. The epoch-based cancellation, debounce, subscription cleanup, per-record resilient JSON decoding, and explicit nil-guards on title persistence each close a specific failure mode. The normalizedTitleUpdate Swift side mirrors the TypeScript truncation limits. Localization is complete for both supported locales. Tests cover store parsing, cwd filtering, untrusted launch command rejection, title preference, type-drifted record skipping, and end-to-end CLI title persistence. No files require special attention. CLI/CMUXCLI+AmpExtension.swift at 824 lines just crosses the 800-line budget threshold but is explicitly tracked and updated in the budget TSV. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Amp as Amp Process
participant Plugin as Amp Plugin (TS)
participant CLI as cmux CLI
participant Store as amp-hook-sessions.json
participant Vault as Session Index (Swift)
Amp->>Plugin: session.start event
Plugin->>Plugin: watchThreadTitle(sessionId)
Plugin->>CLI: "hooks amp session-start {session_id, title?}"
CLI->>Store: upsert(agentLifecycle: .unknown, title?)
Amp->>Plugin: agent.start event
Plugin->>CLI: "hooks amp prompt-submit {session_id, title?}"
CLI->>Store: upsert(agentLifecycle: .running, title?)
Plugin-->>Plugin: resolveSessionTitleBestEffort() [async, epoch-guarded]
Note over Plugin: thread.title observable fires
Plugin->>Plugin: debounce 250ms
Plugin->>CLI: "hooks amp title-update {session_id, title}"
CLI->>Store: upsert(title only, no lifecycle change)
Amp->>Plugin: agent.end event
Plugin->>Plugin: "turnActive=false, turnEpoch++, sessionStartEpoch++"
Plugin->>CLI: "hooks amp stop {session_id}"
Plugin-->>Plugin: resolveSessionTitleBestEffort() [final, if no observed title]
Plugin->>CLI: "hooks amp title-update {session_id, title} [if new]"
Plugin->>Plugin: cleanupThreadTitle(sessionId)
Note over Vault: User opens Vault sidebar
Vault->>Store: loadAmpEntries(needle, cwdFilter, offset, limit)
Store-->>Vault: [AmpIndexedSession]
Vault->>Vault: sort by updatedAt desc, sessionId asc
Vault-->>Vault: render SessionEntry list
%%{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"}}}%%
sequenceDiagram
participant Amp as Amp Process
participant Plugin as Amp Plugin (TS)
participant CLI as cmux CLI
participant Store as amp-hook-sessions.json
participant Vault as Session Index (Swift)
Amp->>Plugin: session.start event
Plugin->>Plugin: watchThreadTitle(sessionId)
Plugin->>CLI: "hooks amp session-start {session_id, title?}"
CLI->>Store: upsert(agentLifecycle: .unknown, title?)
Amp->>Plugin: agent.start event
Plugin->>CLI: "hooks amp prompt-submit {session_id, title?}"
CLI->>Store: upsert(agentLifecycle: .running, title?)
Plugin-->>Plugin: resolveSessionTitleBestEffort() [async, epoch-guarded]
Note over Plugin: thread.title observable fires
Plugin->>Plugin: debounce 250ms
Plugin->>CLI: "hooks amp title-update {session_id, title}"
CLI->>Store: upsert(title only, no lifecycle change)
Amp->>Plugin: agent.end event
Plugin->>Plugin: "turnActive=false, turnEpoch++, sessionStartEpoch++"
Plugin->>CLI: "hooks amp stop {session_id}"
Plugin-->>Plugin: resolveSessionTitleBestEffort() [final, if no observed title]
Plugin->>CLI: "hooks amp title-update {session_id, title} [if new]"
Plugin->>Plugin: cleanupThreadTitle(sessionId)
Note over Vault: User opens Vault sidebar
Vault->>Store: loadAmpEntries(needle, cwdFilter, offset, limit)
Store-->>Vault: [AmpIndexedSession]
Vault->>Vault: sort by updatedAt desc, sessionId asc
Vault-->>Vault: render SessionEntry list
Reviews (16): Last reviewed commit: "fix: guard Amp title lookup timeouts" | Re-trigger Greptile |
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 `@Sources/AmpSessionIndex.swift`:
- Around line 89-90: Sorting only by modified in indexed.sort { $0.modified >
$1.modified } is unstable when timestamps tie; update the comparator to add a
deterministic secondary key (e.g., compare sessionId) so equal modified values
are ordered consistently (for example: if $0.modified == $1.modified then
compare $0.sessionId and $1.sessionId). Ensure the comparator preserves the
intended descending modified order and uses a stable tie-breaker like sessionId
to prevent pagination (offset/limit) skips/duplicates.
In `@Sources/SessionIndexView.swift`:
- Around line 1499-1500: The .amp case is currently returning nil in the
parser/inference branch, which causes Amp lines to be rejected
(shouldParseRawLine == false, parseLine == nil, inferredRole == nil) and empty
previews; fix by either (A) removing .amp from the case that returns nil so .amp
falls through to the generic parser/inference path used by
loadSynchronously(...) (ensuring shouldParseRawLine, parseLine and inferredRole
are populated), or (B) add an Amp-specific loading path in load(entry:) before
the hermesAgent routing so .amp is handled by its own loader; update the
switch/case handling and load(entry:) routing accordingly (referencing case
.amp, case .hermesAgent, load(entry:), loadSynchronously(...),
shouldParseRawLine, parseLine, inferredRole).
🪄 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: 8c112ee1-1311-4a64-85db-491b44416433
⛔ Files ignored due to path filters (1)
Assets.xcassets/AgentIcons/Amp.imageset/Amp.svgis excluded by!**/*.svg
📒 Files selected for processing (13)
Assets.xcassets/AgentIcons/Amp.imageset/Contents.jsonCLI/CMUXCLI+AmpExtension.swiftCLI/cmux.swiftResources/Localizable.xcstringsSources/AmpSessionIndex.swiftSources/SessionAgentPresentation.swiftSources/SessionIndexModels.swiftSources/SessionIndexStore.swiftSources/SessionIndexView.swiftcmux.xcodeproj/project.pbxprojcmuxTests/AmpSessionIndexTests.swiftcmuxTests/CLIGenericHookPersistenceTests.swiftcmuxTests/SessionIndexViewTests.swift
- Guard turnActive after title resolution in agent.start so a late prompt-submit can't revive a finished session as running (greptile P1) - Stable secondary sort by sessionId for equal modified timestamps to keep Vault pagination deterministic (coderabbit) - Localize the Amp store-read error via xcstrings (en/ja) Amp-Thread-ID: https://ampcode.com/threads/T-019e93a5-4b13-747c-9098-5f3117736ab0 Co-authored-by: Amp <amp@ampcode.com>
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
Status, support, documentation and community
|
Amp threads now live server-side, so the legacy ~/.local/share/amp/threads JSON store is abandoned. Read Amp sessions from cmux's own hook store (~/.cmuxterm/amp-hook-sessions.json, written by the bundled Amp plugin) so they appear in the Vault alongside Claude, Codex, Grok, etc. - New Sources/AmpSessionIndex.swift scanner + AmpSessionIndexTests - SessionAgent.amp + AgentSpecifics.amp (resume: amp threads continue <id>) - Store dispatch, presentation, icon asset, localized strings (en/ja) - Titles are synthesized from the cwd since Amp exposes no local title Amp-Thread-ID: https://ampcode.com/threads/T-019e93a5-4b13-747c-9098-5f3117736ab0 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019e93a5-4b13-747c-9098-5f3117736ab0 Co-authored-by: Amp <amp@ampcode.com>
Prefer the real thread title via the Amp plugin API (ctx.thread.title), subscribing so async title generation reaches cmux even for single-turn threads. Falls back to the first user message on older Amp versions whose plugin API predates thread.title. Amp-Thread-ID: https://ampcode.com/threads/T-019e93a5-4b13-747c-9098-5f3117736ab0 Co-authored-by: Amp <amp@ampcode.com>
- Don't revive an idle session: gate the title-change subscription on turnActive so a title that finalizes after the turn ends can't re-fire the session-start hook (which marks the session running). - Normalize Amp cwd with standardizingPath to match SessionIndexStore.normalizedDirectory, so entries bucket/scope-filter consistently with other agents. - Drop the absolute store path from the user-facing read error (filename only), matching the OpenCode/Codex error style. - Tests: assert shell-quoting with a whitespace-containing id instead of echoing SessionEntry.shellQuote; fail fast on unexpected mock RPC methods. Amp-Thread-ID: https://ampcode.com/threads/T-019e93a5-4b13-747c-9098-5f3117736ab0 Co-authored-by: Amp <amp@ampcode.com>
The Vault renders SessionIndex errors in-UI, so per .github/review-bot-rules/user-facing-errors.md the message must not carry raw decode details. Drop error.localizedDescription; keep the filename (no absolute path). Amp-Thread-ID: https://ampcode.com/threads/T-019e93a5-4b13-747c-9098-5f3117736ab0 Co-authored-by: Amp <amp@ampcode.com>
- Guard turnActive after title resolution in agent.start so a late prompt-submit can't revive a finished session as running (greptile P1) - Stable secondary sort by sessionId for equal modified timestamps to keep Vault pagination deterministic (coderabbit) - Localize the Amp store-read error via xcstrings (en/ja) Amp-Thread-ID: https://ampcode.com/threads/T-019e93a5-4b13-747c-9098-5f3117736ab0 Co-authored-by: Amp <amp@ampcode.com>
User-facing copy shouldn't leak the internal hook-store filename (.github/review-bot-rules/user-facing-errors.md). State the failure in product terms instead. Amp-Thread-ID: https://ampcode.com/threads/T-019e93a5-4b13-747c-9098-5f3117736ab0 Co-authored-by: Amp <amp@ampcode.com>
df06bbf to
8f34725
Compare
String(localized:defaultValue:) returns the catalog value as-is, so the %@ placeholder was never substituted outside unit tests (where the defaultValue's Swift interpolation masked it). Wrap with String(format:) like other %@ catalog callers. Amp-Thread-ID: https://ampcode.com/threads/T-019e93a5-4b13-747c-9098-5f3117736ab0 Co-authored-by: Amp <amp@ampcode.com>
|
@coderabbitai full review All review threads (Greptile P1s + CodeRabbit findings) are resolved as of Localization scope — en/ja is the documented standard, not all 20 catalog locales. The repo's own agent guidance in
The other ~18 locales present in the catalog are partial legacy backfills (e.g. So the 4 new Amp keys are fully localized per the project standard. Could you please treat the "all catalog locales" requirement as satisfied by en/ja here, consistent with AGENTS.md? 🤖 (comment drafted by an AI assistant) |
|
The PR description image shows the Vault sidebar ("By agent" view) with three collapsed sections — Claude Code, Codex, and Amp — each displaying session titles and recency timestamps. The Amp section lists five entries ("Successful test thread", "Debug AMP titles in Vault UI", "Can you set a test title for this?", "Amp session in cmux-fork", "Amp session in android-register") with red/orange Amp icons, confirming end-to-end title resolution and display is working correctly.
The 4 new Amp keys ( Triggering a full review now with that localization scope in mind. Please treat the new 🧠 Learnings used✅ Action performedFull review finished. |
There was a problem hiding this comment.
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 `@cmuxTests/AmpSessionIndexTests.swift`:
- Around line 1-8: Replace XCTest usage in AmpSessionIndexTests.swift with the
project-standard Swift Testing primitives: change "import XCTest" to "import
SwiftTesting" (or the repo's Swift Testing module), convert the test container
from XCTestCase (AmpSessionIndexTests) to the Swift Testing form (use the
TestCase/struct pattern the codebase uses), and update all assertions and
helpers—replace XCTAssertEqual(...) with `#expect`(... == ...), XCTUnwrap(...)
with try `#require`(...), XCTAssertTrue(...) with `#expect`(...), and rename test
methods to drop the "test" prefix so they follow Swift Testing naming
conventions; update any references to XCTest-specific APIs accordingly.
🪄 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: ed4b9423-ec5e-4d74-ac6d-87db55f934ab
⛔ Files ignored due to path filters (1)
Assets.xcassets/AgentIcons/Amp.imageset/Amp.svgis excluded by!**/*.svg
📒 Files selected for processing (13)
Assets.xcassets/AgentIcons/Amp.imageset/Contents.jsonCLI/CMUXCLI+AmpExtension.swiftCLI/cmux.swiftResources/Localizable.xcstringsSources/AmpSessionIndex.swiftSources/SessionAgentPresentation.swiftSources/SessionIndexModels.swiftSources/SessionIndexStore.swiftSources/SessionIndexView.swiftcmux.xcodeproj/project.pbxprojcmuxTests/AmpSessionIndexTests.swiftcmuxTests/CLIGenericHookPersistenceTests.swiftcmuxTests/SessionIndexViewTests.swift
New unit tests must use Swift Testing per AGENTS.md; XCTest is reserved for UI tests and socket-harness suites. Migrate XCTestCase -> @suite struct, XCTAssert* -> #expect, XCTUnwrap -> try #require. Amp-Thread-ID: https://ampcode.com/threads/T-019e93a5-4b13-747c-9098-5f3117736ab0 Co-authored-by: Amp <amp@ampcode.com>
|
@coderabbitai full review All review threads are resolved as of 🤖 (comment drafted by an AI assistant) |
|
The PR description image shows the Vault sidebar in "By agent" mode with three sections: Claude Code (5 identical session entries, 4h ago), Codex (5 varied entries), and Amp (5 titled entries: "Successful test thread", "Debug AMP titles in Vault UI", "Can you set a test title for this?", "Amp session in cmux-fork", "Amp session in android-register" with recency timestamps from 2s–1h ago and the orange Amp icons). This confirms title resolution and the Vault UI rendering are working end-to-end.
🧠 Learnings used✅ Action performedFull review finished. |
There was a problem hiding this comment.
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/AmpSessionIndex.swift`:
- Around line 10-12: The current decoding of the `sessions` dictionary is
brittle: replace the single-map decode of `[String: AmpHookSessionRecord]` with
a loss-tolerant, per-key decode that iterates the `sessions` keyed container and
attempts to decode each `AmpHookSessionRecord` individually (using
`decode`/`decodeIfPresent` inside a do/catch) so that decoding failures for a
single record are caught and skipped rather than failing the entire map; apply
the same per-entry lossy decoding approach wherever `sessions` is decoded (e.g.,
in the `loadAmpEntries`/decoding logic and the other occurrence around lines
31-37), preserving valid entries in the resulting `[String:
AmpHookSessionRecord]` and ignoring malformed ones.
🪄 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: 2468b3c0-7f48-4524-9748-7f925dd707e1
⛔ Files ignored due to path filters (1)
Assets.xcassets/AgentIcons/Amp.imageset/Amp.svgis excluded by!**/*.svg
📒 Files selected for processing (13)
Assets.xcassets/AgentIcons/Amp.imageset/Contents.jsonCLI/CMUXCLI+AmpExtension.swiftCLI/cmux.swiftResources/Localizable.xcstringsSources/AmpSessionIndex.swiftSources/SessionAgentPresentation.swiftSources/SessionIndexModels.swiftSources/SessionIndexStore.swiftSources/SessionIndexView.swiftcmux.xcodeproj/project.pbxprojcmuxTests/AmpSessionIndexTests.swiftcmuxTests/CLIGenericHookPersistenceTests.swiftcmuxTests/SessionIndexViewTests.swift
The store is persisted cross-version state; a single type-drifted record previously failed the whole map decode and blanked the Amp listing. Decode entry-by-entry and skip bad records, fulfilling the tolerance the struct's doc comment already promises. Add a regression test. Amp-Thread-ID: https://ampcode.com/threads/T-019e93a5-4b13-747c-9098-5f3117736ab0 Co-authored-by: Amp <amp@ampcode.com>
|
@coderabbitai full review All review threads are resolved as of 🤖 (comment drafted by an AI assistant) |
|
The PR description image shows the Vault sidebar in "By agent" view with Claude Code, Codex, and Amp sections — the Amp section displaying five titled entries ("Successful test thread", "Debug AMP titles in Vault UI", "Can you set a test title for this?", "Amp session in cmux-fork", "Amp session in android-register") with orange Amp icons and recency timestamps, confirming end-to-end title resolution and UI rendering are working.
Triggering a full review against ✏️ Learnings added
🧠 Learnings used✅ Action performedFull review finished. |
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary
Adds Amp (Sourcegraph Amp CLI) to the Vault (Session Index) sidebar, so Amp threads can be browsed, searched, and resumed alongside Claude Code, Codex, Grok, OpenCode, Rovo Dev, and Hermes. Amp was already a restorable agent (auto-relaunch on app start); this brings it to parity in the past-sessions UI.
Demo image illustrates the progression from fallback -> first message -> title observing
Why the hook store (not Amp's thread files)
Amp threads now live server-side. The legacy
~/.local/share/amp/threads/*.jsonstore is abandoned (stale for over a month locally), so this reads Amp sessions from cmux's own hook store~/.cmuxterm/amp-hook-sessions.json— written by the bundled Amp plugin viacmux hooks amp. That store is current, cmux-owned, and already powers the restore flow.Titles
The hook store carries no conversation title on its own, so the bundled Amp plugin supplies one. It reads the real thread title from the Amp plugin API (
ctx.thread.title) and subscribes to it, so a title that Amp generates asynchronously reaches cmux even for single-turn threads. It falls back to the first user message on older Amp versions whose plugin API predatesthread.title, and finally to a cwd-derived label (Amp session in <dir>). The CLI persists whatevertitleit receives and the Vault prefers it — where the title comes from is the plugin's concern. This avoids parsing Amp's unstable debug logs.Changes
Sources/AmpSessionIndex.swift— read-only scanner for the Amp hook store (sort, needle + cwd filtering, pagination), preferring the storedtitleand synthesizing a cwd label otherwiseSessionAgent.amp+AgentSpecifics.ampwith resumeamp threads continue <id>CLI/CMUXCLI+AmpExtension.swift— plugin resolves the thread title viactx.thread.title(get + subscribe) with first-message/cwd fallback, and prefers the app-bundledcmuxfor hooksCLI/cmux.swift— persists a generictitlefield on the hook session record without clobbering an existing onecmuxTests/AmpSessionIndexTests.swift) and end-to-endtitlepersistence (cmuxTests/CLIGenericHookPersistenceTests.swift)Verification
main;cmuxapp target andcmux-unittest target both compile (build-for-testing→TEST BUILD SUCCEEDED).Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Adds Amp to the Vault (Session Index) so you can browse, search, preview, and resume Amp threads alongside other agents. Sessions are read from
~/.cmuxterm/amp-hook-sessions.json, and titles update live via the plugin when available.New Features
ctx.thread.title(get + subscribe) with fallback to the first user message, then a CWD label. CLI persiststitle; the plugin prefers the app-bundledcmux(fallback to PATH).Bug Fixes
title-updatehook; updates are debounced, length-capped, timeout-guarded (stale results ignored), epoch- and turn-gated, sent monotonically, never replace with empty values; observers cleaned up on stop/exit; pending titles flush at turn end.sessionId; Amp CWDs normalized; per-record decode skips bad entries; store-read errors are sanitized and localized; fixed directory-title placeholder rendering; reject integer keys in the hook store.Written for commit 66f1425. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Tests