Add Hermes and Pi agent provider support#2748
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
vouch:unvouched
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| generateBranchName, | ||
| generateThreadTitle, | ||
| } satisfies TextGenerationShape; | ||
| }); |
There was a problem hiding this comment.
Near-identical TextGeneration files duplicate hundreds of lines
Low Severity
HermesTextGeneration.ts and PiTextGeneration.ts are functionally identical — same structure, same logic, same 256-line length — differing only in provider name strings and ACP runtime factory. This full-file duplication also extends to the adapter pair (HermesAdapter.ts/PiAdapter.ts) and the driver pair. A shared, parameterized makeAcpTextGeneration factory accepting the provider name and ACP runtime builder would eliminate hundreds of duplicated lines and ensure bug fixes apply to both providers.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 211f230. Configure here.
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| function scheduleRestart() { | ||
| clearTimeout(restartTimer); | ||
| restartTimer = setTimeout(() => { | ||
| restartTimer = undefined; | ||
| console.log("Restarting server..."); | ||
|
|
||
| const previous = child; | ||
| previous.once("exit", () => { | ||
| if (!stopping) { | ||
| start(); | ||
| } | ||
| }); | ||
| previous.kill("SIGTERM"); | ||
| }, 100); |
There was a problem hiding this comment.
🟢 Low scripts/dev-watch.mjs:58
If the child process exits on its own during the 100ms debounce window, the previous.once("exit", ...) listener in scheduleRestart is registered after the 'exit' event already fired. Since ChildProcess emits 'exit' only once, the listener never runs and start() is never called — the server stays dead. The parent process also won't exit because the exit handler at line 24 returns early when restartTimer is truthy. Consider checking previous.killed or previous.exitCode before registering the listener, and ensure the restart proceeds immediately if the process already exited.
const previous = child;
+ if (previous.exitCode !== null || previous.killed) {
+ if (!stopping) {
+ start();
+ }
+ } else {
previous.once("exit", () => {
if (!stopping) {
start();
}
});
previous.kill("SIGTERM");
+ }🤖 Copy this AI Prompt to have your agent fix this:
In file apps/server/scripts/dev-watch.mjs around lines 58-71:
If the child process exits on its own during the 100ms debounce window, the `previous.once("exit", ...)` listener in `scheduleRestart` is registered after the `'exit'` event already fired. Since `ChildProcess` emits `'exit'` only once, the listener never runs and `start()` is never called — the server stays dead. The parent process also won't exit because the exit handler at line 24 returns early when `restartTimer` is truthy. Consider checking `previous.killed` or `previous.exitCode` before registering the listener, and ensure the restart proceeds immediately if the process already exited.
Evidence trail:
apps/server/scripts/dev-watch.mjs lines 24-27 (exit handler returns early when `restartTimer` is truthy), lines 58-71 (`scheduleRestart` registers the `once('exit')` listener only after the debounce timeout, which may be after the child already exited). Node.js ChildProcess 'exit' event is emitted once and not replayed for late listeners. `child.kill()` on a dead process is a no-op. Verified at commit REVIEWED_COMMIT.
ApprovabilityVerdict: Needs human review Diff is too large for automated approval analysis. A human reviewer should evaluate this PR. You can customize Macroscope's approvability policy. Learn more. |
|
Release-readiness follow-up pushed in joeynyc/t3code@d439ab14. Validated locally:
Addressed review feedback:
Remaining external check: Vercel marketing deploy still fails with Authorization required to deploy, which appears to be a Vercel/GitHub permission gate rather than a code failure. Windows installer was built on macOS but not executed on a Windows host in this environment. |
| streamEvents, | ||
| } satisfies PiAdapterShape; | ||
| }); | ||
| } |
There was a problem hiding this comment.
Massive code duplication across Hermes and Pi implementations
Low Severity
PiAdapter.ts (~836 lines), PiDriver.ts (~175 lines), and PiTextGeneration.ts (~256 lines) are near-identical copies of their Hermes counterparts, differing only in provider name strings, the ACP runtime factory import, and one extra sanitizePiAssistantTextDelta call in the Pi adapter's ContentDelta handler. Over 1,200 lines of logic are duplicated verbatim between the two providers. A shared parameterized factory (taking provider name, ACP runtime builder, and optional text sanitizer) would eliminate this redundancy and reduce risk of inconsistent future bug fixes.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit d439ab1. Configure here.
|
Polish follow-up pushed in joeynyc/t3code@8eff67b2. Changes:
Validation:
Note: I verified the provider settings polish in the paired in-app browser. A separate headless screenshot pass hit the app's pairing-token gate, so I did not replace the existing README chat screenshots in this commit. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 4 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8eff67b. Configure here.
| return Effect.void; | ||
| } | ||
| return Ref.update(outputRef, (current) => current + content.text); | ||
| }); |
There was a problem hiding this comment.
Pi update notice not sanitized in text generation
Medium Severity
The sanitizePiAssistantTextDelta function strips Pi CLI update notices from chat output in PiAdapter.ts, but the same sanitization is missing from PiTextGeneration.ts. The handleSessionUpdate callback in text generation accumulates raw agent_message_chunk text without filtering, so if a Pi CLI update notice appears during commit message, PR content, branch name, or thread title generation, it will corrupt the accumulated output and likely break extractJsonObject JSON parsing or produce garbage results.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 8eff67b. Configure here.
| } | ||
|
|
||
| const changedPath = resolve(srcDir, filename); | ||
| if (!changedPath.startsWith(`${srcDir}/`)) { |
There was a problem hiding this comment.
Dev watch path check fails on Windows
Low Severity
The shouldRestart function checks changedPath.startsWith(${srcDir}/) using a forward slash, but on Windows resolve() produces backslash-separated paths. This causes the guard to always return false on Windows, meaning file changes will never trigger a server restart during development.
Reviewed by Cursor Bugbot for commit 8eff67b. Configure here.
1 participant
Summary
Verification
Note
Medium Risk
Adds two new provider drivers that spawn local CLIs via ACP and changes provider/model selection gating logic in the web UI, which could affect session startup and model-picker behavior. Risk is moderated by targeted unit/browser tests and largely additive integration paths.
Overview
Adds Hermes and Pi as first-class providers, including new server
ProviderDrivers plus ACP adapters that manage sessions/turns, permissions, attachments, and runtime event streaming.Implements provider health/status layers for both (binary discovery, version/auth probing, default-model detection, suggested-path hints) and background snapshot enrichment for update advisories; Pi also hardens ACP spawning by injecting
PI_ACP_PI_COMMANDand fixing PATH handling, and filters Pi CLI “new version” notices from assistant deltas.Extends text-generation to use Hermes/Pi ACP runtimes for commit/PR/branch/thread-title generation, registers both drivers in the built-in driver list, and updates the web UI/model picker to only allow selection of selectable instances (
enabled+ available +ready+ has models) while adding provider-specific chat surfaces (chat-surface-hermes/chat-surface-pi) and improved empty/setup messaging; README/docs are updated with setup guidance and screenshots.Reviewed by Cursor Bugbot for commit 8eff67b. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add Hermes and Pi as built-in agent provider types with ACP-backed text generation
HermesDriverandPiDriveras built-in provider drivers, each backed by a new ACP runtime (makeHermesAcpRuntime/makePiAcpRuntime) that spawns the respective CLI tool and streams chunked responses with a 180s timeout.HermesSettingsandPiSettingsschemas to server and client settings, including binary path fields, custom models, and patch schemas for partial updates.parseHermesConfigModelDefaults,parsePiConfigModelDefaults) to detect the active model from each provider's YAML/JSON config on disk.isSelectableProviderInstance(enabled + available + ready + has models), so Hermes/Pi instances that are installed but unauthenticated appear disabled rather than selectable.chat-surface-hermes,chat-surface-pi).scripts/dev-watch.mjs) replacingnode --watchwith a debounced, SIGTERM-graceful file watcher.resolveSelectableProviderInstancenow requiresstatus === 'ready'and at least one model — previously enabled+available was sufficient, which could silently change the auto-selected provider for existing users.Macroscope summarized 8eff67b.