Skip to content

fix(cli): support custom ACP history synchronization - #335

Open
UniversePeak wants to merge 2 commits into
LodyAI:mainfrom
UniversePeak:fix/custom-acp-history-sync
Open

fix(cli): support custom ACP history synchronization#335
UniversePeak wants to merge 2 commits into
LodyAI:mainfrom
UniversePeak:fix/custom-acp-history-sync

Conversation

@UniversePeak

Copy link
Copy Markdown

Related issue

Closes #187

Problem / pressure

The local-project history UI lists every agent configuration, including custom ACP agents, but the history sync launch path only received cliType and agentType. Builtin and registry agents can resolve their executables from static definitions; a custom agent's executable is stored in its machine-scoped customAcp configuration. As a result, every custom history sync failed before spawning the ACP process with Custom ACP <agentType> has no launch command configured.

Summary

  • Allow the local-project control guard to accept custom ACP history providers.
  • Resolve the daemon-authoritative custom agent configuration for the requested machine and provider before history work starts.
  • Thread the resolved customAcp launch spec through the shared history catalog client so session listing and replay use the same command and arguments as normal custom ACP sessions.
  • Apply the resolved launch to sync, selected-session import, and conflict-resolution replay paths without widening the control-plane provider wire contract.
  • Add regression coverage for custom launch resolution and custom history control requests.

Before / after

Before After
Custom ACP providers appeared in local-project history settings but were rejected by the remote control guard or reached launch resolution without a command. Custom providers pass the control boundary, and the owning daemon resolves their current machine-scoped customAcp command before spawning history ACP processes.
History listing/import/conflict resolution could never start a custom ACP process. All history entry points share the resolved custom launch while builtin and registry behavior remains unchanged.

Test plan

  • corepack pnpm --filter acp-extension-core build — passed, required local ACP package entry was built.
  • corepack pnpm --filter acp-extension-claude build — passed, required local workspace entry was built.
  • corepack pnpm --filter lody typecheck — passed.
  • corepack pnpm --filter @lody/shared typecheck — passed.
  • corepack pnpm --filter lody exec vitest run tests/history-session-catalog-client.test.ts -t 'custom ACP' — passed, 1 test.
  • corepack pnpm --filter @lody/shared exec vitest run tests/local-project-control.test.ts — passed, 29 tests.
  • corepack pnpm --filter lody exec vitest run tests/local-project-history-sync-service.test.ts — passed, 30 tests.
  • Focused type-aware oxlint on changed CLI/shared files — 0 errors.
  • Prettier checks on all changed files and git diff --check — passed.
  • Full @lody/shared suite — 1,012 passed, 3 unrelated baseline failures (one local CLI host-lease assertion and two source-guard tests timed out); no changed history-control test failed.

Context handoff

Instructions for reviewing agents

  • Review focus: Check LocalProjectHistorySyncService.resolveLaunchProvider, the three history operation paths, and history-session-catalog-client.ts to ensure only daemon-authoritative machine config supplies the custom command.
  • Decisions to challenge: Confirm that resolving by machine plus cliType:agentType is the right identity boundary and that keeping customAcp out of the control-plane wire schema avoids trusting caller-supplied executable paths.
  • Plausible failures / evidence gaps: The full shared suite retains three unrelated baseline failures; live spawning of a user-provided ACP server was not run, while the launch resolver regression verifies exact command and argv propagation.

Authoring context

  • User goal / directives: Make local-project history synchronization work for custom ACP agents using the command already stored in their agent configuration.
  • Constraints / non-goals: Keep builtin and registry launches unchanged, do not accept executable paths from the RPC caller, do not duplicate the custom command in session metadata, and do not broaden the change beyond local-project history operations.
  • Risk-bearing decisions: The daemon reads the merged config for the target machine and provider, then passes that trusted launch spec through list/replay; a missing config or command retains the existing clear failure instead of falling back.
  • Destructive or irreversible behavior: None; this changes process-launch selection and adds tests only, with no persistence migration or user-data rewrite.
  • Deliberately not done or tested: No live custom ACP server was launched and no full desktop build was run; focused type, lint, history-service, control-schema, and launch-resolution checks cover the changed paths.
  • Unknowns / confidence: High confidence in the control and launch plumbing; the remaining uncertainty is compatibility with varied third-party custom ACP servers, which CI and maintainer review can exercise.

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f5d56fe91e

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

candidate.cliType === this.provider.cliType &&
candidate.agentType === this.provider.agentType
);
const resolved = { ...this.provider, customAcp: config?.customAcp };

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Propagate the custom agent environment into history spawns

When a custom ACP config relies on config.env for API keys, base URLs, or PATH, this resolver retains only customAcp; the history client consequently builds the child environment from the daemon process and login shell rather than the selected machine config. Normal sessions launch with the config environment, so history listing, import, and conflict replay can now start the correct executable under the wrong credentials or endpoint and fail for otherwise working custom agents. Include the authoritative config environment in the resolved history launch and merge it before spawning.

Useful? React with 👍 / 👎.

@UniversePeak

Copy link
Copy Markdown
Author

Addressed the P1 review in commit 87f89b4.

History sync now resolves the selected machine's authoritative AgentConfigMeta and propagates its env alongside customAcp for session listing, import, and conflict replay. The caller cannot inject an executable path; the launch still comes from the resolved machine configuration. Added a regression test covering config environment precedence for API key, base URL, and PATH. Targeted history-sync tests, CLI/shared typechecks, changed-file oxlint, Prettier, and git diff --check pass; the remaining full-suite failures are pre-existing Windows/path and environment/submodule issues.

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 87f89b469c

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

return (
isObjectRecord(value) &&
(value.cliType === 'builtin' || value.cliType === 'registry') &&
(value.cliType === 'builtin' || value.cliType === 'registry' || value.cliType === 'custom') &&

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Allow custom providers through the desktop IPC guard

In the OSS Electron flow, the renderer routes local-project history operations through LocalProjectsIpc.syncHistory/importHistory/resolveHistoryConflict, but apps/electron/src/main/ipc/services/local-projects-ipc.ts:59-67 still accepts only builtin and registry. Consequently, selecting a newly exposed custom provider returns Invalid history provider before the request reaches this updated validator or the CLI, so custom ACP history synchronization remains unusable from the desktop. Update the Electron guard alongside this accepted provider set.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] History sync from a custom ACP agent fails: no launch command configured

1 participant