Skip to content

refactor(provider-acp): ACP, ACP Registry and Grok follow the Effect service conventions - #17544

Merged
juliusmarminge merged 1 commit into
mainfrom
t3/audit-acp-family
Oct 9, 2026
Merged

juliusmarminge merged 1 commit into
mainfrom
t3/audit-acp-family

Conversation

@juliusmarminge

@juliusmarminge juliusmarminge commented Oct 9, 2026 •

Copy link
Copy Markdown
Member

Part of the provider-package audit: every module in provider-acp, -acp-registry and -grok should now follow docs/internals/effect-services.md.

Services come from the environment

  • makeAcpClientTerminals is an Effect.fn that yields the spawner. The spawner option is gone.
  • makeAcpAdapterV2 yields the spawner and provides it to the client terminals and to flavor.makeRuntime. The clientTerminals.childProcessSpawner option is gone.
  • terminateWindowsProcessTreeWithTaskkill requires the spawner instead of taking it as an argument.
  • The ACP Registry adapter yields its catalog and treats the runtime coordinator as an optional service. It no longer receives childProcessSpawner, resolver or runtimeCoordinator as options.
  • acpRegistryManagedBinaryDirectories yields FileSystem and Path. The terminal manager provides them.
  • Grok's makeGrokAcpRuntime and makeGrokAcpAdapterFlavor require the spawner.

Imports and errors

  • effect/http and effect/process modules are now subpath namespace imports, in the packages and in the server adapter tests and testkits touched here.
  • Grok's snapshot error no longer copies cause.message into detail.

Left as is, deliberately

  • runtime in the ACP adapter options is a per-session instance built by flavor.makeRuntime, not the service.
  • Three errors still use detail-as-message, and their messages reach users or agents, so changing them changes behavior. Separate PR:
    • AcpMcpOverAcpError
    • AcpRegistryError (one of them carries install stderr)
    • AcpProcessGroupTerminationError

Tests: provider-grok 108 passed, 4 skipped; provider-acp-registry 65 passed; server ACP/Registry/Grok adapter and terminal manager tests 227 passed.

Model: Claude Opus 5.5 via Claude Code in T3 Code.

🤖 Generated with Claude Code


Devin Review

…service conventions

Effect http/process modules are imported as subpath namespaces. Factories
yield ChildProcessSpawner, FileSystem, Path, AcpRegistryCatalog and the
optional AcpRegistryRuntimeCoordinator from the environment instead of taking
them as options: makeAcpClientTerminals, makeAcpAdapterV2 clientTerminals,
terminateWindowsProcessTreeWithTaskkill, makeGrokAcpRuntime,
makeGrokAcpAdapterFlavor, makeAcpRegistryAdapterV2 and
acpRegistryManagedBinaryDirectories. Flavor makeRuntime factories may now
require ChildProcessSpawner, which the ACP adapter provides. The Grok
snapshot error detail no longer copies cause.message.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions github-actions Bot added the vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. label Oct 9, 2026
@juliusmarminge juliusmarminge added the macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews label Oct 9, 2026
@github-actions github-actions Bot added the size:L 100-499 changed lines (additions + deletions). label Oct 9, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — The PR primarily rewires existing ACP, registry, and Grok dependencies to use Effect services without introducing a new workflow or schema. It also modifies an authentication file, so the change requires human review regardless of the otherwise mechanical scope.

You can add or adjust custom eligibility rules. Learn more.

@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

Provider Metric Main baseline This PR Impact PR ceiling
Codex Total thread wire 5.0 KiB 5.0 KiB 0 B (0.0%) 6.8 KiB ✅
Codex Thread snapshot wire 3.8 KiB 3.8 KiB 0 B (0.0%) 4.9 KiB ✅
Codex Live turn WebSocket wire 1.2 KiB 1.2 KiB 0 B (0.0%) 2.0 KiB ✅
Codex Live turn WebSocket decoded 20.9 KiB 20.9 KiB 0 B (0.0%) 29.3 KiB ✅
Codex Live turn messages 2 2 0 (0.0%) 8 ✅
Claude Total thread wire 5.0 KiB 5.0 KiB 0 B (0.0%) 6.8 KiB ✅
Claude Thread snapshot wire 3.8 KiB 3.8 KiB 0 B (0.0%) 4.9 KiB ✅
Claude Live turn WebSocket wire 1.2 KiB 1.2 KiB 0 B (0.0%) 2.0 KiB ✅
Claude Live turn WebSocket decoded 21.2 KiB 21.2 KiB 0 B (0.0%) 29.3 KiB ✅
Claude Live turn messages 2 2 0 (0.0%) 8 ✅

Baseline: 31b04e2 · PR result: 8385f49 · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 108.5 KiB
  • Claude decoded thread snapshot: 108.8 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

@coderabbitai

coderabbitai Bot commented Oct 9, 2026

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Team
  • Run ID: a2d193bf-ee51-45c7-ad42-996eae91007c

📥 Commits

Reviewing files that changed from the base of the PR and between 31b04e2 and 8385f49.


📒 Files selected for processing (28)
  • apps/server/scripts/record-grok-acp-replay-fixture.ts
  • apps/server/src/orchestration-v2/Adapters/AcpAdapterV2.test.ts
  • apps/server/src/orchestration-v2/Adapters/AcpAdapterV2.testkit.ts
  • apps/server/src/orchestration-v2/Adapters/AcpRegistryAdapterV2.test.ts
  • apps/server/src/orchestration-v2/Adapters/AcpRegistryAdapterV2.testkit.ts
  • apps/server/src/orchestration-v2/Adapters/GrokAdapterV2.test.ts
  • apps/server/src/orchestration-v2/Adapters/GrokAdapterV2.testkit.ts
  • apps/server/src/terminal/Manager.ts
  • packages/provider-acp-registry/src/server/AcpRegistrySupport.test.ts
  • packages/provider-acp-registry/src/server/AcpRegistrySupport.ts
  • packages/provider-acp-registry/src/server/adapter.ts
  • packages/provider-acp-registry/src/server/auth.ts
  • packages/provider-acp-registry/src/server/driver.ts
  • packages/provider-acp-registry/src/server/probe.ts
  • packages/provider-acp/src/server/AcpSessionRuntime.ts
  • packages/provider-acp/src/server/adapter.ts
  • packages/provider-acp/src/server/clientTerminals.test.ts
  • packages/provider-acp/src/server/clientTerminals.ts
  • packages/provider-grok/src/server/acpCliProbe.test.ts
  • packages/provider-grok/src/server/acpSupport.ts
  • packages/provider-grok/src/server/adapter.ts
  • packages/provider-grok/src/server/driver.test.ts
  • packages/provider-grok/src/server/driver.ts
  • packages/provider-grok/src/server/skills.test.ts
  • packages/provider-grok/src/server/skills.ts
  • packages/provider-grok/src/server/status.ts
  • packages/provider-grok/src/server/textGeneration.ts
  • packages/provider-grok/src/server/usageLimits.ts

💤 Files with no reviewable changes (2)
  • packages/provider-acp/src/server/clientTerminals.test.ts
  • apps/server/scripts/record-grok-acp-replay-fixture.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.



📝 Walkthrough

Walkthrough

ACP, ACP Registry, and Grok runtime code now obtains process and other dependencies through the Effect environment. Registry adapter dependencies are provided through service layers. Related callers, tests, and imports are updated for these interfaces.

Changes

Effect service dependency updates

Layer / File(s) Summary
ACP process services and terminal lifecycle
packages/provider-acp/src/server/AcpSessionRuntime.ts, packages/provider-acp/src/server/adapter.ts, packages/provider-acp/src/server/clientTerminals.ts, packages/provider-acp/src/server/clientTerminals.test.ts, apps/server/src/orchestration-v2/Adapters/AcpAdapterV2.test.ts, apps/server/src/orchestration-v2/Adapters/AcpAdapterV2.testkit.ts
ACP process termination and client-terminal creation now obtain ChildProcessSpawner through the Effect environment. Adapter setup and tests use service provisioning rather than passing the spawner in options or function arguments.
Registry adapter services and test layers
packages/provider-acp-registry/src/server/adapter.ts, packages/provider-acp-registry/src/server/{auth,driver,probe}.ts, apps/server/src/orchestration-v2/Adapters/AcpRegistryAdapterV2.test.ts, apps/server/src/orchestration-v2/Adapters/AcpRegistryAdapterV2.testkit.ts
The registry adapter reads its catalog and optional runtime coordinator from the Effect environment. Tests provide catalog and coordinator layers, including mocks that fail if resolution occurs in injected-runtime and replay cases.
Registry managed-directory services
packages/provider-acp-registry/src/server/AcpRegistrySupport.ts, packages/provider-acp-registry/src/server/AcpRegistrySupport.test.ts, apps/server/src/terminal/Manager.ts
acpRegistryManagedBinaryDirectories obtains FileSystem and Path from the Effect environment. Its tests and server call site no longer pass those services as input fields.
Grok runtime service wiring
packages/provider-grok/src/server/*, apps/server/scripts/record-grok-acp-replay-fixture.ts, apps/server/src/orchestration-v2/Adapters/GrokAdapterV2.test.ts, apps/server/src/orchestration-v2/Adapters/GrokAdapterV2.testkit.ts
Grok ACP runtime construction now requires ChildProcessSpawner through the Effect environment. Callers and probes no longer pass it as an input. The adapter reuses an ACP request-error predicate, and snapshot-construction errors use a fixed detail message.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Refactor


Merge Risk: ⚪ Minimal · up to 8385f

This refactor moves process and file-system dependencies into the Effect environment and updates callers and tests. No concrete merge-blocking risk was identified.

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check Passed The title is concise, specific, and accurately summarizes the service-convention refactor across ACP, ACP Registry, and Grok.
Description check Passed The description clearly explains the service-injection changes, preserved behavior, deferred error changes, and focused test results. It does not use the template headings or provide an explicit issue…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.


✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR

🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR


  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@juliusmarminge
juliusmarminge merged commit f853d51 into main Oct 9, 2026
33 of 34 checks passed
@juliusmarminge
juliusmarminge deleted the t3/audit-acp-family branch October 9, 2026 16:56
github-actions Bot added a commit to omarcresp/t3code-flake that referenced this pull request Oct 9, 2026
## What's Changed
* fix(web): Local environment switch stays reachable after turning it off by @ScottN-PV in pingdotgg/t3code#17359
* fix(web): keep chat banners inside the lane beside the docked details card by @macodev00 in pingdotgg/t3code#17094
* fix(web): settled and snoozed lines line up with the messages above them by @RakshithBhat03 in pingdotgg/t3code#17191
* fix(web): distinguish project filter from new project by @voltcrash in pingdotgg/t3code#12113
* feat(web): assign a thread details panel shortcut by @maria-rcks in pingdotgg/t3code#16694
* fix(web): chat content keeps pace with sidebar resizing by @flamboh in pingdotgg/t3code#17383
* refactor(provider-core): expose model metadata through a ModelCatalog port by @juliusmarminge in pingdotgg/t3code#17417
* refactor(provider-core): follow the Effect service conventions throughout by @juliusmarminge in pingdotgg/t3code#17427
* refactor(provider-core): latest-version lookups go through a ProviderLatestVersions service by @juliusmarminge in pingdotgg/t3code#17434
* refactor(provider-core): MCP provider sessions live in a McpProviderSessions service by @juliusmarminge in pingdotgg/t3code#17446
* refactor(provider): bring opencode, muse, pi, core and testing in line with Effect conventions by @juliusmarminge in pingdotgg/t3code#17542
* refactor(provider-acp): ACP, ACP Registry and Grok follow the Effect service conventions by @juliusmarminge in pingdotgg/t3code#17544
* refactor(provider-cursor): follow the Effect service conventions by @juliusmarminge in pingdotgg/t3code#17545
* fix(marketing): use app wordmark in header by @voltcrash in pingdotgg/t3code#13240
* fix(web): pr merge actions stay visible while the stack refreshes by @maria-rcks in pingdotgg/t3code#17559

## New Contributors
* @voltcrash made their first contribution in pingdotgg/t3code#12113

**Full Changelog**: pingdotgg/t3code@v0.0.46-nightly.20261009.2873...v0.0.46-nightly.20261009.2886

Upstream release: https://github.com/pingdotgg/t3code/releases/tag/v0.0.46-nightly.20261009.2886
github-actions Bot added a commit to davidvanderklay/t3code-flake that referenced this pull request Oct 9, 2026
## What's Changed
* fix(web): Local environment switch stays reachable after turning it off by @ScottN-PV in pingdotgg/t3code#17359
* fix(web): keep chat banners inside the lane beside the docked details card by @macodev00 in pingdotgg/t3code#17094
* fix(web): settled and snoozed lines line up with the messages above them by @RakshithBhat03 in pingdotgg/t3code#17191
* fix(web): distinguish project filter from new project by @voltcrash in pingdotgg/t3code#12113
* feat(web): assign a thread details panel shortcut by @maria-rcks in pingdotgg/t3code#16694
* fix(web): chat content keeps pace with sidebar resizing by @flamboh in pingdotgg/t3code#17383
* refactor(provider-core): expose model metadata through a ModelCatalog port by @juliusmarminge in pingdotgg/t3code#17417
* refactor(provider-core): follow the Effect service conventions throughout by @juliusmarminge in pingdotgg/t3code#17427
* refactor(provider-core): latest-version lookups go through a ProviderLatestVersions service by @juliusmarminge in pingdotgg/t3code#17434
* refactor(provider-core): MCP provider sessions live in a McpProviderSessions service by @juliusmarminge in pingdotgg/t3code#17446
* refactor(provider): bring opencode, muse, pi, core and testing in line with Effect conventions by @juliusmarminge in pingdotgg/t3code#17542
* refactor(provider-acp): ACP, ACP Registry and Grok follow the Effect service conventions by @juliusmarminge in pingdotgg/t3code#17544
* refactor(provider-cursor): follow the Effect service conventions by @juliusmarminge in pingdotgg/t3code#17545
* fix(marketing): use app wordmark in header by @voltcrash in pingdotgg/t3code#13240
* fix(web): pr merge actions stay visible while the stack refreshes by @maria-rcks in pingdotgg/t3code#17559

## New Contributors
* @voltcrash made their first contribution in pingdotgg/t3code#12113

**Full Changelog**: pingdotgg/t3code@v0.0.46-nightly.20261009.2873...v0.0.46-nightly.20261009.2886

Upstream release: https://github.com/pingdotgg/t3code/releases/tag/v0.0.46-nightly.20261009.2886
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews size:L 100-499 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant