Skip to content

fix(pi): build the effort-history context without the SDK-only buildContextEntries - #214

Open
iceteaSA wants to merge 2 commits into
cortexkit:mainfrom
iceteaSA:rv/patch-3
Open

fix(pi): build the effort-history context without the SDK-only buildContextEntries#214
iceteaSA wants to merge 2 commits into
cortexkit:mainfrom
iceteaSA:rv/patch-3

Conversation

@iceteaSA

@iceteaSA iceteaSA commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Summary

oh-my-pi bundles a pi-coding-agent whose SessionManager has no buildContextEntries(), so the turn_start effort-history handler threw and the extension failed to load on that host. The compaction-aware context builder is now ported locally and fed from getEntries() / getLeafId(), which both hosts provide.

The port was checked against the SDK's implementation across eight entry shapes (leaf resolution, compaction with firstKeptEntryId, multiple compactions, branch_summary entries) and matches it, so it replaces the SDK call unconditionally rather than branching per host.

One defect in the first shape of this change was caught in review and fixed here: the call site coalesced an absent getLeafId method into null, and null is the SDK's explicit "no leaf → empty context" — so the hosts this fix targets would have received an empty effort history. resolveSessionLeafId() now keeps the three states distinct: absent method → undefined (fall back to the latest entry), explicit null → empty, string → that leaf.

Credit: original patch by randomvariable (CortexKit Discord, 2026-09-03), who could not open the PR themselves. Reviewed, fixed, tested, and re-authored here with Co-authored-by.

Changes

  • packages/pi/src/effort-history.tsbuildContextEntries(entries, leafId?) (local port, three-state contract documented) and resolveSessionLeafId(sessionManager).
  • packages/pi/src/index.ts — the turn_start handler uses the port with the resolved leaf.
  • packages/pi/src/tests/effort-history.test.ts — nine new cases: helper passthrough, null leaf, undefined leaf, root path, compaction mid-path, no compaction, multi-compaction (latest wins), branch_summary passthrough.

Verification

  • packages/pi: 103 pass / 0 fail (+9) · root typecheck 0 · format/biome clean.
  • Red-first: the absent-method test failed while the helper still coalesced to null; restoring ?? null reddens it again.

Base: upstream/main 360b68e (v1.22.0).


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.


Summary by cubic

Fixes the effort-history context builder in packages/pi so the turn_start handler no longer crashes extension load on hosts like oh-my-pi that bundle their own pi-coding-agent without the SDK-only buildContextEntries() method.

  • Ports the compaction-aware builder locally and feeds it from getEntries() / getLeafId(), which both hosts provide; the SDK call is replaced unconditionally.
  • Keeps an absent getLeafId distinct from an explicit null leaf: absent falls back to the latest entry, null returns an empty context, and a string selects that leaf — coalescing them previously discarded the entire effort history.
  • Adds nine tests covering leaf resolution, firstKeptEntryId compaction, multiple compactions, and branch_summary passthrough.

Written for commit 0317d76. Summary will update on new commits.

Review in cubic

iceteaSA and others added 2 commits September 11, 2026 07:20
oh-my-pi bundles its own pi-coding-agent whose SessionManager lacks
buildContextEntries(), so the turn_start effort-history handler crashed
extension load. Port the compaction-aware context builder locally and
read entries via getEntries()/getLeafId(), which both hosts provide.
Entry shapes (compaction.firstKeptEntryId, thinking_level_change,
custom_message) are identical across hosts.

Co-authored-by: randomvariable <redacted@localhost>
…n the context port

The SDK treats undefined as an unknown leaf and falls back to the latest entry, while null explicitly selects no leaf and returns an empty context. Coalescing an absent getLeafId method into null therefore discarded the entire effort history on hosts without that method.

Co-authored-by: randomvariable <redacted@localhost>

@cubic-dev-ai cubic-dev-ai 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.

1 issue found across 3 files

Confidence score: 3/5

  • In packages/pi/src/effort-history.ts, buildContextEntries treats a stale or unknown leafId like an omitted one and may build history for the wrong branch; distinguish undefined from string IDs and return an empty collection for unknown IDs.
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/effort-history.ts">

<violation number="1" location="packages/pi/src/effort-history.ts:127">
P2: When `leafId` is a stale or unknown string, `buildContextEntries` silently falls back to the latest entry and can build effort history for the wrong branch. Distinguish `undefined` from a string and return an empty context when the string is not present.</violation>
</file>
Architecture diagram
sequenceDiagram
    participant PI as Pi Extension
    participant SM as SessionManager
    participant EH as EffortHistory (Local Port)
    participant DB as Session Data Store
    
    Note over PI,EH: Effort History Context Build (Current State)
    
    PI->>SM: getSessionId()
    SM-->>PI: sessionId
    
    PI->>EH: resolveSessionLeafId(sessionManager)
    EH->>SM: optional getLeafId() method check
    
    alt Host provides getLeafId()
        EH->>SM: getLeafId()
        SM-->>EH: leafId (string or null)
        alt Explicit null leaf
            EH-->>PI: null
        else String leaf
            EH-->>PI: leafId string
        end
    else Host lacks getLeafId() method
        EH-->>PI: undefined
    end
    
    PI->>SM: getEntries()
    SM-->>PI: entries
    
    PI->>EH: buildContextEntries(entries, leafId)
    
    alt Explicit null leaf (empty context)
        EH-->EH: Fast return [] (no history)
    else Non-null leaf or undefined
        EH->>EH: Build entry map by id
        EH->>EH: Resolve leaf (explicit leaf, latest entry, or empty)
        EH->>EH: Walk parent chain to root
        
        alt No compaction on path
            EH-->>PI: Full entry path
        else Compaction(s) on path
            EH->>EH: Find latest compaction entry
            EH->>EH: Rebuild context: compaction + entries from firstKeptEntryId
            EH-->>PI: Compaction-aware context list
        end
    end
    
    PI->>EH: collectPiEffortHistory(contextEntries, branch)
    EH-->>PI: Effort transitions
    PI->>PI: Store per-session history
Loading

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment on lines +127 to +128
if (leafId) leaf = byId.get(leafId)
leaf ??= entries.at(-1)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: When leafId is a stale or unknown string, buildContextEntries silently falls back to the latest entry and can build effort history for the wrong branch. Distinguish undefined from a string and return an empty context when the string is not present.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/pi/src/effort-history.ts, line 127:

<comment>When `leafId` is a stale or unknown string, `buildContextEntries` silently falls back to the latest entry and can build effort history for the wrong branch. Distinguish `undefined` from a string and return an empty context when the string is not present.</comment>

<file context>
@@ -88,3 +88,69 @@ export function collectPiEffortHistory(
+  const byId = new Map(entries.map((entry) => [entry.id, entry]))
+  let leaf: SessionEntryWithParent | undefined
+  if (leafId === null) return []
+  if (leafId) leaf = byId.get(leafId)
+  leaf ??= entries.at(-1)
+  if (!leaf) return []
</file context>
Suggested change
if (leafId) leaf = byId.get(leafId)
leaf ??= entries.at(-1)
if (leafId === undefined) leaf = entries.at(-1)
else leaf = byId.get(leafId)

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