fix(pi): derive context entries from the branch both hosts expose - #202
fix(pi): derive context entries from the branch both hosts expose#202tomolom wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new regression tests can silently pass without executing the turn_start handler due to optional chaining, reducing their ability to prevent future regressions.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes Pi/Oh My Pi host API drift by removing the hard dependency on SessionManager.buildContextEntries() (missing in omp 18.x) and deriving the same “context entries” view directly from the branch path exposed by both hosts, while ensuring the turn_start hook cannot disable provider registration via a thrown exception.
Changes:
- Add
deriveContextEntries(branch)to reproduce Pi’s compaction-aware context trimming without calling host-only APIs. - Update the
turn_starthandler to usegetBranch()+ derived context entries and to degrade to “no transitions” on unreadable session shapes. - Extend Pi test coverage for omp-shaped session managers and ensure root
bun run testrunspackages/pitests.
File summaries
| File | Description |
|---|---|
| packages/pi/src/index.ts | Switch turn_start effort-history collection to getBranch() + deriveContextEntries() and guard against hook exceptions. |
| packages/pi/src/effort-history.ts | Introduce deriveContextEntries to replicate compaction trimming from the branch path. |
| packages/pi/src/tests/index.test.ts | Add regression tests for omp-like session manager shape and hook error-degradation behavior. |
| packages/pi/src/tests/effort-history.test.ts | Add direct unit tests for deriveContextEntries and update compaction fixtures. |
| package.json | Extend root test script to include packages/pi tests. |
Review details
Suppressed comments (1)
packages/pi/src/tests/index.test.ts:146
- This assertion also uses optional chaining when calling the
turn_starthandler, which would let the test pass even if the handler was never registered. Capture the handler, assert it exists, then assert its return value.
expect(await events.get('turn_start')?.({ type: 'turn_start' }, ctx)).toBe(
undefined,
)
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| await events.get('turn_start')?.({ type: 'turn_start' }, ctx) | ||
|
|
||
| expect(providers.get('anthropic')).toBeDefined() | ||
| }) |
|
Host-level confirmation against a real So the accessor is genuinely absent on the live host object, |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 5 files
Confidence score: 5/5
- In
packages/pi/src/index.ts, the broad catch aroundgetBranch,deriveContextEntries, andcollectPiEffortHistorycan silently drop data when a host API shape changes, making incompatibilities difficult to diagnose; add logging or narrower error handling.
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/pi/src/index.ts">
<violation number="1" location="packages/pi/src/index.ts:86">
P3: The catch swallows all exceptions from getBranch/deriveContextEntries/collectPiEffortHistory with no log. Issue #200 was exactly this class of silent host incompatibility; a future shape change will now silently drop effort markers with zero diagnostic instead of surfacing. Preserve the degraded no-op but log the error so regressions are detectable.</violation>
</file>
Architecture diagram
sequenceDiagram
participant TurnStart as turn_start handler
participant SM as SessionManager (Pi / OMP)
participant EH as effort-history module
participant MH as effortHistoryBySession store
participant Provider as Anthropic Provider (Fable/Mythos)
Note over TurnStart,Provider: Turn start flow - host compatibility fix
TurnStart->>SM: getSessionId()
alt No session ID
SM-->>TurnStart: null
TurnStart-->>TurnStart: Return early
else Session ID present
SM-->>TurnStart: sessionId
TurnStart->>SM: getBranch()
alt getBranch() succeeds
SM-->>TurnStart: branch entries (leaf-to-root)
TurnStart->>EH: deriveContextEntries(branch)
Note over EH: Applies compaction trim locally<br/>(host buildContextEntries() not<br/>available on OMP 18.x)
EH-->>TurnStart: context entries
TurnStart->>EH: collectPiEffortHistory(contextEntries, branch)
EH-->>TurnStart: effort transitions
TurnStart->>MH: delete(sessionId)
TurnStart->>MH: set(sessionId, transitions)
Note over MH: LRU cap at 128 sessions
TurnStart->>Provider: Request with effort markers
Provider-->>TurnStart: Continue turn
else getBranch() throws
SM-->>TurnStart: Error
TurnStart->>TurnStart: Catch - degrade to empty transitions
TurnStart->>MH: delete(sessionId)
TurnStart->>MH: set(sessionId, [])
TurnStart->>Provider: Request without effort markers (no failure)
Note over TurnStart,Provider: Provider preserved - no per-turn<br/>extension error (issue #200 fix)
end
end
Note over EH: deriveContextEntries() logic
EH->>EH: Scan branch for last compaction entry
alt No compaction entry
EH-->>TurnStart: Return branch slice unchanged
else Compaction found
EH->>EH: Start with compaction entry
EH->>EH: Retain entries from firstKeptEntryId onward
EH->>EH: Append all entries after compaction
EH-->>TurnStart: Derived context entries
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| try { | ||
| const branch = ctx.sessionManager.getBranch() | ||
| transitions = collectPiEffortHistory(deriveContextEntries(branch), branch) | ||
| } catch { |
There was a problem hiding this comment.
P3: The catch swallows all exceptions from getBranch/deriveContextEntries/collectPiEffortHistory with no log. Issue #200 was exactly this class of silent host incompatibility; a future shape change will now silently drop effort markers with zero diagnostic instead of surfacing. Preserve the degraded no-op but log the error so regressions are detectable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/pi/src/index.ts, line 86:
<comment>The catch swallows all exceptions from getBranch/deriveContextEntries/collectPiEffortHistory with no log. Issue #200 was exactly this class of silent host incompatibility; a future shape change will now silently drop effort markers with zero diagnostic instead of surfacing. Preserve the degraded no-op but log the error so regressions are detectable.</comment>
<file context>
@@ -71,10 +74,18 @@ export default function cortexKitPiAnthropicAuth(pi: ExtensionAPI) {
+ try {
+ const branch = ctx.sessionManager.getBranch()
+ transitions = collectPiEffortHistory(deriveContextEntries(branch), branch)
+ } catch {
+ transitions = []
+ }
</file context>
3e3b331 to
38fcaa8
Compare
|
Review feedback addressed in the pushed amend. Optional chaining could pass vacuously (both reviewers) — fixed. Each test now looks the handler up, asserts it exists, and invokes it. Reverting The getBranch-only test only asserted "provider defined" (cubic) — agreed, that was too weak. It is now end-to-end: So the test now fails if the transitions the handler collects never reach the request — not just if the handler throws. Silent catch (cubic) — considered and deliberately kept silent, now documented at the catch site. Two wording corrections also pushed, from re-reading my own evidence:
|
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
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/pi/src/tests/index.test.ts">
<violation number="1" location="packages/pi/src/tests/index.test.ts:186">
P3: The global fetch mock assigns every non-bootstrap response the same canned SSE success and records the last non-bootstrap `init.body` into `requestBody`, without validating method, URL, or response semantics. If the stream flow ever issues an additional non-bootstrap request (e.g. a relay post, quota check, or a retry), `requestBody` will point at the wrong request and the hard-coded `sent.messages[2]`/`output_config` assertions will inspect the wrong payload. Restrict the mock to match the expected `/v1/messages?beta=true` POST URL so only the real messages request is captured.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| }), | ||
| ) | ||
| } | ||
| requestBody = JSON.parse(String(init?.body)) |
There was a problem hiding this comment.
P3: The global fetch mock assigns every non-bootstrap response the same canned SSE success and records the last non-bootstrap init.body into requestBody, without validating method, URL, or response semantics. If the stream flow ever issues an additional non-bootstrap request (e.g. a relay post, quota check, or a retry), requestBody will point at the wrong request and the hard-coded sent.messages[2]/output_config assertions will inspect the wrong payload. Restrict the mock to match the expected /v1/messages?beta=true POST URL so only the real messages request is captured.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/pi/src/tests/index.test.ts, line 186:
<comment>The global fetch mock assigns every non-bootstrap response the same canned SSE success and records the last non-bootstrap `init.body` into `requestBody`, without validating method, URL, or response semantics. If the stream flow ever issues an additional non-bootstrap request (e.g. a relay post, quota check, or a retry), `requestBody` will point at the wrong request and the hard-coded `sent.messages[2]`/`output_config` assertions will inspect the wrong payload. Restrict the mock to match the expected `/v1/messages?beta=true` POST URL so only the real messages request is captured.</comment>
<file context>
@@ -104,34 +133,107 @@ describe('cortexKitPiAnthropicAuth provider registration', () => {
+ }),
+ )
+ }
+ requestBody = JSON.parse(String(init?.body))
+ return new Response(
+ [
</file context>
There was a problem hiding this comment.
Valid, fixed in the pushed amend.
The mock now dispatches explicitly and refuses anything it does not expect:
- bootstrap URL -> bootstrap response
POST https://api.anthropic.com/v1/messages...-> captured, answered with the canned SSE stream- anything else ->
throw new Error("unexpected request: <method> <url>"), so a relay post, quota fetch or retry fails the test instead of silently overwriting the body under assertion
It also asserts requestBody is still undefined before capturing, so a second messages POST cannot replace the first. That assertion runs in the passing run (11 expect calls in this file, up from 9), which is the evidence that exactly one messages request is made on this path.
packages/pi: 100 pass / 0 fail. Reverting src/index.ts + src/effort-history.ts to main still fails the two regression tests with the reported TypeError. Typecheck and biome clean.
`turn_start` called `SessionManager.buildContextEntries()`, which exists on Pi but not on Oh My Pi 18.x — on a live 18.1.14 SessionManager instance the property is `undefined`. The handler threw at its first statement on every turn, so it surfaced a per-turn extension error and collected no Fable/Mythos 5.1 mid-conversation effort markers at all. `getBranch()` is present on both hosts and returns the same root-to-leaf path `buildContextEntries()` walks, so `deriveContextEntries()` applies the compaction trim locally: the compaction entry, the tail retained from `firstKeptEntryId`, then everything appended after it. The handler now also degrades to no transitions instead of throwing. It only annotates requests with effort markers, so an unreadable session shape must cost the session its transitions and nothing else. Also run the pi package's tests in `bun run test`, which covered core and opencode only. Fixes cortexkit#200
38fcaa8 to
5076052
Compare
Fixes #200.
Root cause
turn_startresolved session entries throughctx.sessionManager.buildContextEntries(). That method exists on Pi (@earendil-works/pi-coding-agent0.84.2 —ReadonlySessionManagerincludes it) but not on Oh My Pi 18.x. Probed on a live@oh-my-pi/pi-coding-agent@18.1.14SessionManagerinstance, not just its.d.ts:So the handler threw at its first statement on every turn. Measured consequences:
Extension "…" error: ctx.sessionManager.buildContextEntries is not a function.I did not establish the mechanism by which the reporter also lost the
anthropicprovider: in the runner I inspected, handler errors are contained per handler (#runHandlerWithTimeout→emitError→showExtensionError) and queued provider registrations are flushed at session init, before any turn. Theiromp modelsbefore/after evidence stands on its own; this PR does not claim to explain it, and hardens the handler so it cannot contribute either way.Fix
getBranch()is present on both hosts, and on Pi it returns the same root-to-leaf pathbuildContextEntries()walks (buildContextEntries(entries, leafId, byId)→buildSessionPath(...); both collect leaf-upward and reverse, so the array is root-first — confirmed by the live probe below). The compaction trim it applies is therefore derived locally instead of being requested from the host:deriveContextEntries(branch)ineffort-history.ts— compaction entry, then the tail retained fromfirstKeptEntryId, then everything appended after the compaction; an uncompacted branch passes through.firstKeptEntryIdis present on both hosts'CompactionEntry.turn_startnow takes onegetBranch()call and feeds both arguments ofcollectPiEffortHistory, so the two views can no longer disagree.This is chosen over the optional-call form (
buildContextEntries?.() ?? getEntries()) suggested in the issue:getEntries()returns every entry including abandoned branches, so a session with branches would count assistant messages that are not in context and misplaceafterAssistantMessages. Deriving from the branch keeps one code path with the host semantics intact.The handler additionally degrades to no transitions rather than throwing, per the issue's expected behaviour: this hook only annotates requests with effort markers, so an unreadable session shape should cost the session its transitions and nothing else.
Not changed:
peerDependenciesstill names@earendil-works/*. omp resolves those specifiers through its own legacy shim (legacy-pi-ai-shim.tsre-exports@oh-my-pi/pi-ai), so the package names are not what broke here, and a*-ranged peer would not have caught a removed method anyway. Fixing it at the call site is what makes the plugin work on both hosts.Verification
Live host, driving this branch's built
dist/effort-history.jsagainst a real ompSessionManager:packages/pi/src/tests/index.test.tsdrives the registeredturn_starthandler with an omp-shaped session manager (getSessionId/getBranch/getEntries, nobuildContextEntries). Reverting onlysrc/index.ts+src/effort-history.tsand re-running reproduces the reported error verbatim:TypeError: ctx.sessionManager.buildContextEntries is not a function— 2 fail. With the fix: 5 pass.deriveContextEntriesis covered for the uncompacted branch, the retained tail, an unreachablefirstKeptEntryId, and multiple compactions; the existing compaction effort-history test now asserts the derived array equals the hand-written context entries it already used.bun run typecheckclean;bun test src/testsinpackages/pi: 100 pass / 0 fail;biome checkclean.bun run testat the root ran core and opencode only, so no pi test could gate a release. It now runspackages/pitoo — that is how these regression tests earn their place in CI.