fix(amp): correct merged connector follow-up - #483
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe Amp plugin now installs as a staged directory, preloads Context Bundles per session, consumes them once per thread, coordinates manual and automatic captures, improves message signatures, and updates integration tests and coverage settings. ChangesAmp plugin lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Amp
participant BootstrapManager
participant ContextBundleCLI
Amp->>BootstrapManager: session.start(threadId)
BootstrapManager->>ContextBundleCLI: preload Context Bundle
ContextBundleCLI-->>BootstrapManager: bundle result
Amp->>BootstrapManager: agent.start(threadId)
BootstrapManager-->>Amp: consume bootstrap message once
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
nowledge-mem-amp-plugin/tests/bootstrap.test.ts (1)
37-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the CLI argv and cover
consumewithoutpreload.The suite verifies call counts but never the arguments passed to
nmem. A regression in the["context", "--source-app", sourceApp]argv would pass. Add that assertion.Also add a case where
consumeruns for a thread that never receivedpreload. That path currently marks the thread consumed and returns{}, which is the behavior discussed in thesrc/bootstrap.tscomment onconsume.🧪 Proposed test additions
it("does not refetch the same thread and manages sessions independently", async () => { const nmem = vi.fn(fakeNmem('{"ok":true}')) const manager = new BootstrapManager({ nmem }, { sourceApp: "amp", enabled: true }) manager.preload("T-1") manager.preload("T-1") manager.preload("T-2") await manager.consume("T-1") await manager.consume("T-2") expect(nmem).toHaveBeenCalledTimes(2) + expect(nmem).toHaveBeenCalledWith(["context", "--source-app", "amp"]) }) + + it("returns an empty result when the session was never preloaded", async () => { + const nmem = vi.fn(fakeNmem('{"ok":true}')) + const manager = new BootstrapManager({ nmem }, { sourceApp: "amp", enabled: true }) + expect(await manager.consume("T-9")).toEqual({}) + expect(nmem).not.toHaveBeenCalled() + })🤖 Prompt for 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. In `@nowledge-mem-amp-plugin/tests/bootstrap.test.ts` around lines 37 - 78, Update the BootstrapManager tests to assert that nmem is called with ["context", "--source-app", "amp"] in the preload/consume flow. Add a test covering consume("T-1") without a prior preload, asserting it returns {} and preserves the expected consumed-thread behavior described by consume.nowledge-mem-amp-plugin/src/bootstrap.ts (1)
173-190: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a disposed guard so
preloadcannot start work after disposal.
disposeclears the map but does not block later calls. Ifsession.startis delivered afteramp.onDisposeruns,preloadrecreates the session state and spawns a newnmemCLI process. The PR objectives state that the sync path prevents post-dispose rescheduling, so this manager should match that guarantee.♻️ Proposed refactor
+ private disposed = false + public preload(threadId: string): void { - if (!this.options.enabled) return + if (!this.options.enabled || this.disposed) returnpublic dispose(): void { + this.disposed = true this.sessions.clear() }🤖 Prompt for 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. In `@nowledge-mem-amp-plugin/src/bootstrap.ts` around lines 173 - 190, Update the session manager’s dispose/preload flow to track disposal explicitly and make preload return without creating state or starting work after dispose() has run. Ensure dispose() marks the manager disposed before clearing sessions, and have preload check that guard while preserving normal behavior before disposal.nowledge-mem-amp-plugin/tests/index.test.ts (1)
129-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the disposal message, not any log call.
ampKnowledgeMemalso logs the connector-loaded message during registration.amp.logger.logis therefore already called before the disposer runs, so this assertion passes even if the disposal log is deleted. Match the disposal text instead.🧪 Proposed change
- expect(amp.logger.log).toHaveBeenCalledWith(expect.stringContaining("disposed")) + expect(amp.logger.log).toHaveBeenCalledWith(expect.stringContaining("connector disposed"))Replace line 129 with the assertion above.
🤖 Prompt for 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. In `@nowledge-mem-amp-plugin/tests/index.test.ts` at line 129, Update the disposer test for ampKnowledgeMem to assert that amp.logger.log was called with the specific disposal message, rather than merely checking that logging occurred. Preserve the existing registration assertions and match the exact text emitted by the disposer.
🤖 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 `@nowledge-mem-amp-plugin/scripts/install.sh`:
- Around line 52-64: Update install_staged so the existing SKILL_DEST is moved
to BACKUP_SKILL only when STAGED_SKILL exists and will replace it. Preserve the
current skill untouched when no staged skill is present, while retaining backup
cleanup and restore behavior for actual skill replacements.
- Around line 25-36: Update the staging setup around STAGING_DIR so it is
created under PLUGINS_DIR rather than the system temporary directory, ensuring
plugin moves remain same-filesystem renames. Because SKILL_DEST is under
SKILLS_DIR, create and track a separate staging directory under SKILLS_DIR when
those directories may use different filesystems, and ensure cleanup removes both
staging directories.
In `@nowledge-mem-amp-plugin/src/bootstrap.ts`:
- Around line 155-165: Update consume so state.consumed is set only after
state.preloadedMessage is confirmed present and immediately before returning the
injected message. Leave the flag unchanged when no message exists, allowing a
later preload for the same thread to be consumed, and release the cached bundle
text from the thread state after returning it.
In `@nowledge-mem-amp-plugin/src/sync.ts`:
- Around line 157-164: Recheck the disposed state after awaiting the existing
state.inFlight capture in the manual-save flow, and return the disposed result
immediately when teardown occurred during the wait. Only clear the in-flight
state and call captureThread with force: true when the instance remains active.
- Around line 157-163: Update the in-flight cleanup loop in syncNow to set
state.inFlight to undefined only when it still references the captured previous
promise, then re-evaluate the loop against any newer guard. Add coverage for
three concurrent syncNow calls to verify captures remain serialized without
clearing a newer in-flight state.
---
Nitpick comments:
In `@nowledge-mem-amp-plugin/src/bootstrap.ts`:
- Around line 173-190: Update the session manager’s dispose/preload flow to
track disposal explicitly and make preload return without creating state or
starting work after dispose() has run. Ensure dispose() marks the manager
disposed before clearing sessions, and have preload check that guard while
preserving normal behavior before disposal.
In `@nowledge-mem-amp-plugin/tests/bootstrap.test.ts`:
- Around line 37-78: Update the BootstrapManager tests to assert that nmem is
called with ["context", "--source-app", "amp"] in the preload/consume flow. Add
a test covering consume("T-1") without a prior preload, asserting it returns {}
and preserves the expected consumed-thread behavior described by consume.
In `@nowledge-mem-amp-plugin/tests/index.test.ts`:
- Line 129: Update the disposer test for ampKnowledgeMem to assert that
amp.logger.log was called with the specific disposal message, rather than merely
checking that logging occurred. Preserve the existing registration assertions
and match the exact text emitted by the disposer.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1926f20c-d1b0-4583-baad-2fbc765f1ad9
📒 Files selected for processing (12)
nowledge-mem-amp-plugin/scripts/install.shnowledge-mem-amp-plugin/src/bootstrap.tsnowledge-mem-amp-plugin/src/index.tsnowledge-mem-amp-plugin/src/messages.tsnowledge-mem-amp-plugin/src/sync.tsnowledge-mem-amp-plugin/tests/bootstrap.test.tsnowledge-mem-amp-plugin/tests/e2e.test.tsnowledge-mem-amp-plugin/tests/index.test.tsnowledge-mem-amp-plugin/tests/messages.test.tsnowledge-mem-amp-plugin/tests/sync.test.tsnowledge-mem-amp-plugin/vitest.config.tstests/plugin_e2e/test_key_plugins_e2e.py
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 `@nowledge-mem-amp-plugin/src/sync.ts`:
- Around line 160-172: Add an early this.disposed check at the start of the then
callback in the run chain, before waiting on priorManual, returning
skipped("disposed", this.stableThreadId(threadId)) to prevent a forced capture
after teardown. Extend the manual-capture tests to queue two captures, dispose
while the first is pending, resolve it, and assert the second returns disposed
with only one write.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9b9f0012-af83-4b3d-9a64-e58dc6265017
📒 Files selected for processing (4)
nowledge-mem-amp-plugin/scripts/install.shnowledge-mem-amp-plugin/src/bootstrap.tsnowledge-mem-amp-plugin/src/sync.tsnowledge-mem-amp-plugin/tests/sync.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- nowledge-mem-amp-plugin/scripts/install.sh
|
Applied the latest review fixes and pushed commit |
|
Thanks @IceCodeNew, this follow-up is very helpful. I did one small maintainer-side pass to keep the Amp installer fail-closed when the skill payload is missing, and re-ran the Amp unit/type/static checks plus the GitHub checks. Everything is green now, merging this in. |
2 participants
Follow-up to merged PR #480. Fixes the Codex registry/test version contract, makes Amp installation recoverable, disposes bootstrap state, strengthens transcript fingerprints, serializes manual and automatic capture, prevents post-dispose rescheduling, and aligns static tests with the session.start bootstrap. Focused Amp tests: 187 passed, 100% coverage; Codex validator passes. The unrelated integrations/README changes from PR #482 are intentionally not included.
Summary by CodeRabbit
New Features
Bug Fixes