Skip to content

feat(agent): wire up hog (pi) as a selectable runtime adapter#3264

Open
jonathanlab wants to merge 3 commits into
mainfrom
hog-adapter-integration
Open

feat(agent): wire up hog (pi) as a selectable runtime adapter#3264
jonathanlab wants to merge 3 commits into
mainfrom
hog-adapter-integration

Conversation

@jonathanlab

Copy link
Copy Markdown
Contributor

Problem

Changes

How did you test this?

Automatic notifications

  • Publish to changelog?
  • Alert Sales and Marketing teams?

@trunk-io

trunk-io Bot commented Jul 8, 2026

Copy link
Copy Markdown

Merging to main in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here

@jonathanlab jonathanlab marked this pull request as ready for review July 8, 2026 12:07
@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown

React Doctor found no issues in the changed files. 🎉

Reviewed by React Doctor for commit 3e1aae5.

Comment thread packages/harness/src/acp.ts Fixed
Comment thread packages/harness/src/extensions/posthog-provider/models.ts Fixed
@greptile-apps

greptile-apps Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Comments Outside Diff (1)

  1. packages/harness/src/acp.ts, line 1034-1047 (link)

    P1 "error" mapped to "refusal" conflates error and refusal

    "error" as Pi's stop reason indicates an internal failure (gateway 5xx, malformed LLM response, tool crash), not a model content policy refusal. Mapping it to "refusal" surfaces to the client as "model declined to answer," hiding the real cause and making it appear as a deliberate refusal when the agent simply crashed. Any stop reason not explicitly covered by the ACP spec is probably safer mapped to "end_turn" (or a dedicated error state if the protocol supports one) so clients don't misclassify infrastructure failures as safety blocks.

Reviews (1): Last reviewed commit: "feat(agent): wire up hog (pi) as a selec..." | Re-trigger Greptile

Comment on lines +2261 to +2264
// The gateway returns models in an arbitrary order. Sort Claude and hog
// models oldest-to-newest so the picker is deterministic and the newest
// model lands at the end of the list, closest to the trigger.
if (adapter !== "codex") {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 getClaudeModelRecency sort applied to mixed Claude + OpenAI models in the hog adapter

getClaudeModelRecency uses Claude model naming conventions. For OpenAI model IDs (gpt-*, o*), it almost certainly returns the same default value, so all OpenAI models will cluster together in an undefined order. Sorting the hog adapter's mixed catalogue is fine, but a sort key that is meaningful for all model families would be clearer intent.

Suggested change
// The gateway returns models in an arbitrary order. Sort Claude and hog
// models oldest-to-newest so the picker is deterministic and the newest
// model lands at the end of the list, closest to the trigger.
if (adapter !== "codex") {
// The gateway returns models in an arbitrary order. Sort Claude models
// oldest-to-newest so the picker is deterministic; OpenAI models returned
// by the hog adapter have no comparable recency function and will cluster
// at the same sort key (keeping their gateway order relative to each other).
if (adapter !== "codex") {

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed: updated the comment to accurately describe the behavior — OpenAI models clustering at the same sort key (no comparable recency function) is expected, not a bug, so this is a clarity fix rather than a logic change.

Comment on lines 43 to 46
function getOtherAdapter(adapter: AgentAdapter): AgentAdapter {
return adapter === "claude" ? "codex" : "claude";
const index = ADAPTER_CYCLE.indexOf(adapter);
return ADAPTER_CYCLE[(index + 1) % ADAPTER_CYCLE.length];
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 getOtherAdapter name no longer describes the function's behaviour

After this change the function cycles through a three-element ring rather than returning a binary "other." Calling it getNextAdapter (or getNextAdapterInCycle) would match what the code actually does and avoid confusion for future readers.

Suggested change
function getOtherAdapter(adapter: AgentAdapter): AgentAdapter {
return adapter === "claude" ? "codex" : "claude";
const index = ADAPTER_CYCLE.indexOf(adapter);
return ADAPTER_CYCLE[(index + 1) % ADAPTER_CYCLE.length];
}
function getNextAdapter(adapter: AgentAdapter): AgentAdapter {
const index = ADAPTER_CYCLE.indexOf(adapter);
return ADAPTER_CYCLE[(index + 1) % ADAPTER_CYCLE.length];
}

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed: renamed to getNextAdapter, and the local variable/prop usages (otherAdapter -> nextAdapter) for consistency.

- packages/harness/src/acp.ts: stop mapping pi's internal-error stop reason
  to ACP's "refusal" (which implies a deliberate content-policy decline);
  map to "end_turn" instead since ACP has no dedicated failure state
- packages/workspace-server: clarify the hog-adapter model sort comment
  (OpenAI models have no comparable recency function and cluster together,
  which is expected, not a bug)
- packages/ui: rename getOtherAdapter -> getNextAdapter now that it cycles
  through a three-element ring instead of returning a binary other
@jonathanlab

Copy link
Copy Markdown
Contributor Author

Addressed the greptile P1 finding: piStopReasonToAcp in packages/harness/src/acp.ts no longer maps pi's "error" stop reason to ACP's "refusal" — that conflated an internal failure (gateway 5xx, malformed response, tool crash) with a deliberate model content-policy decline. Since ACP has no dedicated failure stop reason, it now maps to "end_turn", the neutral choice among what the protocol actually offers.

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.

2 participants