fix(pi): resolve native embedding routing before registration - #462
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 2 files
You’re at about 94% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Thanks @Qiiks — the routing-boundary analysis is correct. I reproduced master registering We are merging this with two small fixes on top, which the two bot reviews also flagged: a registry-level shadow-only unregister called from both harnesses when routing returns no shadow (a pre-existing OpenCode gap that this PR exposes on Pi), and memoizing deterministic no-probe routing outcomes while keeping the retry for an actually attempted unavailable Synapse lane. One consequence worth knowing for anyone who had a raw Pi Synapse registration: the pending identity is no longer reused, so the discovered or fallback identity becomes the active vector space on upgrade. Ships in v0.42.6. |
There was a problem hiding this comment.
2 issues found across 4 files (changes from recent commits).
You’re at about 94% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/plugin/src/features/magic-context/project-embedding-registry.ts">
<violation number="1" location="packages/plugin/src/features/magic-context/project-embedding-registry.ts:2249">
P2: If unregister runs while a shadow batch is in flight, the worker can still write vectors for the retired shadow model after this disposal. Add cancellation or a registration-generation check before committing the batch, and suppress stale worker state updates.</violation>
</file>
<file name="packages/pi-plugin/src/embedding-bootstrap.test.ts">
<violation number="1" location="packages/pi-plugin/src/embedding-bootstrap.test.ts:147">
P3: The `expect(disposed).toBe(true)` assertion depends on a synchronous timing detail: `unregisterProjectShadowEmbedding` → `disposeProvider` calls `void provider.dispose()` without awaiting, and the test provider's async `dispose` body sets `disposed = true` before its first (nonexistent) await. This passes only because the async body runs synchronously. If `disposeProvider` ever starts deferring disposal or the provider discards returns control before setting the flag, the test would silently pass/fail based on timing rather than actual retirement. Since the disposal is fire-and-forget in the production code, assert on the deterministic observable (shadow cohort null, primary intact) and avoid relying on the fire-and-forget side effect timing for the disposal check.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| shadowBackfillStopReasons.delete(key); | ||
| shadowBackfillLastWriteOutcomes.delete(key); | ||
| } | ||
| disposeProvider(shadow?.provider ?? null); |
There was a problem hiding this comment.
P2: If unregister runs while a shadow batch is in flight, the worker can still write vectors for the retired shadow model after this disposal. Add cancellation or a registration-generation check before committing the batch, and suppress stale worker state updates.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/plugin/src/features/magic-context/project-embedding-registry.ts, line 2249:
<comment>If unregister runs while a shadow batch is in flight, the worker can still write vectors for the retired shadow model after this disposal. Add cancellation or a registration-generation check before committing the batch, and suppress stale worker state updates.</comment>
<file context>
@@ -2232,6 +2232,23 @@ export function registerProjectInObservationMode(
+ shadowBackfillStopReasons.delete(key);
+ shadowBackfillLastWriteOutcomes.delete(key);
+ }
+ disposeProvider(shadow?.provider ?? null);
+}
+
</file context>
| await ensureProjectRegisteredFromPiDirectory(directory, db); | ||
| expect(getShadowEmbeddingMeasurementCohort(identity)).toBeNull(); | ||
| expect(getProjectEmbeddingSnapshot(identity)?.provider).toBe("off"); | ||
| expect(disposed).toBe(true); |
There was a problem hiding this comment.
P3: The expect(disposed).toBe(true) assertion depends on a synchronous timing detail: unregisterProjectShadowEmbedding → disposeProvider calls void provider.dispose() without awaiting, and the test provider's async dispose body sets disposed = true before its first (nonexistent) await. This passes only because the async body runs synchronously. If disposeProvider ever starts deferring disposal or the provider discards returns control before setting the flag, the test would silently pass/fail based on timing rather than actual retirement. Since the disposal is fire-and-forget in the production code, assert on the deterministic observable (shadow cohort null, primary intact) and avoid relying on the fire-and-forget side effect timing for the disposal check.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/pi-plugin/src/embedding-bootstrap.test.ts, line 147:
<comment>The `expect(disposed).toBe(true)` assertion depends on a synchronous timing detail: `unregisterProjectShadowEmbedding` → `disposeProvider` calls `void provider.dispose()` without awaiting, and the test provider's async `dispose` body sets `disposed = true` before its first (nonexistent) await. This passes only because the async body runs synchronously. If `disposeProvider` ever starts deferring disposal or the provider discards returns control before setting the flag, the test would silently pass/fail based on timing rather than actual retirement. Since the disposal is fire-and-forget in the production code, assert on the deterministic observable (shadow cohort null, primary intact) and avoid relying on the fire-and-forget side effect timing for the disposal check.</comment>
<file context>
@@ -92,4 +100,104 @@ describe("ensureProjectRegisteredFromPiDirectory", () => {
+ await ensureProjectRegisteredFromPiDirectory(directory, db);
+ expect(getShadowEmbeddingMeasurementCohort(identity)).toBeNull();
+ expect(getProjectEmbeddingSnapshot(identity)?.provider).toBe("off");
+ expect(disposed).toBe(true);
+ } finally {
+ _resetProjectEmbeddingRegistryForTests();
</file context>
…ion; shadow-only unregister on both harnesses; no-probe memoization Co-authored-by: Alfonso <alfonso-magic-context@users.noreply.github.com>
Summary
Pi's
ensureProjectRegisteredFromPiDirectorypassed the rawconfig.embeddingblock straight toregisterProjectEmbedding. The OpenCode plugin resolves the same block throughresolveEmbeddingRoutingat the identical boundary (packages/plugin/src/plugin/embedding-bootstrap.ts:30-46); Pi did not, so:provider:"synapse"with an unreachable daemon registered a Synapse identity backed by an empty connection file — a fallback vector labelled Synapse.shadow_embeddingwas ignored entirely.Change
resolveEmbeddingRoutingat the pre-registration boundary and registerrouting.primary/ armrouting.shadow.registers the fallback identity when native discovery is unavailablefails pre-fix (registry.provider === "synapse"with an unreachable daemon) and passes post-fix. Noteembedding.provider/subc/shadow_embeddingare user-tier only (stripUnsafeProjectConfigFields), so the test arms the lane from the user config and pinsXDG_CONFIG_HOME, not justHOME.Verification
cd packages/pi-plugin && bun test src/embedding-bootstrap.test.ts→ 2 pass / 0 fail post-fix; pre-fix the new test fails withExpected: not "synapse".Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Fixes Pi's embedding registration so it resolves the embedding config through
resolveEmbeddingRoutingbefore registering, matching how the OpenCode plugin handles the same boundary.Bug Fixes
shadow_embeddingis armed when present and retired (and disposed) when routing no longer resolves a shadow.Written for commit c972f50. Summary will update on new commits.
The PR appears safe to merge, with no outstanding finding that meets the threshold for a new review comment.
Summary
Fixes Pi embedding bootstrap behavior by resolving configured embedding routes before registration.
Diagram
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Load trusted Pi configuration] --> B[Resolve embedding routing] B --> C[Register resolved primary lane] C --> D{Shadow lane resolved?} D -->|Yes| E[Register shadow lane] D -->|No| F[Unregister prior shadow lane] E --> G{Recoverable discovery failure?} F --> G G -->|Yes| H[Clear fingerprint so next call retries] G -->|No| I[Memoize configuration fingerprint]Reviews (2) · Last reviewed commit: "fix(pi): retire stale shadow routing and..."