feat: Add support for Pi coding agent - #97
Merged
Conversation
Owner
|
hey @R2D2-cz appreciate the PR - this is great! can you resolve the conflicts and CI failures? will happily merge once that's all green |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
src/core/agents/pi.ts): runs in JSON mode, streams JSONL events, and parses structuredAgentOutputfrom Pi's assistant reply; registers it infactory.tsandconfig.tswith--api-keyblocked as a reserved arg.agent_endunconditionally overwriting alatestAssistantMessagealready set by an earliermessage_endevent.--api-keyrestriction and the requirement to configure the Pi API key outside of gnhf.Risk Assessment
✅ Low: All three rounds of previously identified issues have been addressed; the Pi agent implementation is now correct, well-tested, and bounded in scope.
Testing
npx vitest run src/core/agents/pi.test.tsnpx vitest run src/core/agents/factory.test.ts src/core/config.test.ts src/cli.test.tsnpm run build && npx vitest run --exclude test/e2e.test.ts(all 31 unit test files)Pipeline
Updates from git push no-mistakes
✅ **Rebase** - passed
Round 1 - passed ✅
🔧 **Review** - 3 issues found → auto-fixed (2)
Round 1 - found 3 issues (2 warnings, 1 info)
src/core/agents/pi.ts:376-rememberAssistantMessagein theagent_endhandler (pi.ts:377-383) unconditionally overwriteslatestAssistantMessageeven when it was already set by an earliermessage_endevent. If Pi emitsmessage_endfollowed byagent_end(a plausible sequence whereagent_endsignals overall run completion), the message fromagent_end.messages[]replaces the dedicated per-message event. If that message differs from the one inmessage_end.message(e.g., it is a snapshot with truncated content),JSON.parse(finalText)will fail and trigger a spurious iteration failure. Fix: guard theagent_endbody withif (!latestAssistantMessage), or split therememberAssistantMessagefunction soagent_endonly falls back for both the text and usage paths.src/core/agents/pi.ts:297-anonymousMessageKeyis a single constant string"assistant-anonymous"(pi.ts:297) used as a fallback key for assistant messages that have noresponseId,id, ortimestamp. When multiple such messages arrive, eachusageByMessageKey.set(key, usage)call (pi.ts:306) overwrites the previous entry rather than adding to it, so earlier messages' token counts are silently dropped. The cumulative sum then undercounts. Fix: use a per-invocation counter to generate unique fallback keys (e.g.,"assistant-anon-" + (usageByMessageKey.size)before the set).src/core/agents/pi.ts:390-stopReason === "aborted"(pi.ts:390) is treated identically tostopReason === "error"and surfaces as"pi reported error: ...", which triggers gnhf's backoff streak. A Pi-side abort may indicate quota exhaustion, context overflow, or a user-initiated cancellation — conditions that may not warrant exponential backoff, or that warrant a distinct user-facing message. Worth confirming whether treating Pi'sabortedthe same aserroris intentional, or if it should map to a graceful failure (no backoff increment) or a distinct error message.Round 2 (auto-fix) - found 1 warning
src/core/agents/pi.ts:297- The WeakMap anonymous-key approach assigns a unique slot per object reference. Eachmessage_updateJSONL event produces a fresh parsed object forevent.message, so if Pi ever includesusagedata in streaming update events for an anonymous message (noresponseId/id/timestamp), every update adds a new entry tousageByMessageKeyrather than overwriting the previous one, causing cumulative token counts to inflate N× over the actual value. Messages with a stable key are unaffected (theirsetcall overwrites). Confirm whether Pi's streaming protocol ever carriesusageinmessage_updateevents; if it does, the accumulation guardif (!anonymousKeyMap.has(message))needs to also check whether the currentlatestAssistantMessageobject should be treated as the canonical slot for that message (e.g., promote tolatestAssistantMessageobject identity before callingupdateUsage).Round 3 (auto-fix) - passed ✅
✅ **Test** - passed
Round 1 - passed ✅
npx vitest run src/core/agents/pi.test.tsnpx vitest run src/core/agents/factory.test.ts src/core/config.test.ts src/cli.test.tsnpm run build && npx vitest run --exclude test/e2e.test.ts(all 31 unit test files)🔧 **Document** - 1 issue found → auto-fixed
Round 1 - found 1 warning
README.md:234- Pi is the only agent where--api-key(and--api-key=) is a reserved/blocked flag inisReservedAgentArg(src/core/config.ts:131-132). Users who tryagentArgsOverride.pi: ["--api-key", "..."]will get a config validation error with no explanation. The existing "Flags that gnhf manages itself…are rejected" note frames reserved args as output-shaping or local-server flags, so it doesn't signal that a credential flag is blocked. The Pi Requirements column says "configure a usable provider/model first" but doesn't clarify that the API key must be set via Pi's own config or an environment variable rather than gnhf's agentArgsOverride. A note should be added — either in the Agents table Pi row, or in the agentArgsOverride section — explaining that--api-keyis blocked for Pi and that Pi API key configuration must happen outside gnhf (via Pi's own config or an environment variable).Round 2 (auto-fix) - passed ✅
🔧 **Lint** - 3 issues found → auto-fixed
Round 1 - found 3 warnings
src/core/agents/pi.ts:336- Prettier: line exceeds printWidth (80).rememberAssistantMessagearrow function parameters(message: unknown, streaming = false)must be reformatted to multi-line.src/core/agents/pi.test.ts:189- Prettier: line exceeds printWidth (80). Inline object{ type: "text_delta", contentIndex: 0, delta: "hel" }assigned toassistantMessageEventmust be reformatted to multi-line.src/core/agents/pi.test.ts:194- Prettier: line exceeds printWidth (80). Inline object{ type: "text_delta", contentIndex: 0, delta: "lo" }assigned toassistantMessageEventmust be reformatted to multi-line.Round 2 (auto-fix) - passed ✅
✅ **Push** - passed
Round 1 - passed ✅