Skip to content

fix(daemon): preserve ACP startup diagnostics - #6267

Open
Siri-Ray wants to merge 3 commits into
mainfrom
agent/fix-acp-startup-diagnostics
Open

fix(daemon): preserve ACP startup diagnostics#6267
Siri-Ray wants to merge 3 commits into
mainfrom
agent/fix-acp-startup-diagnostics

Conversation

@Siri-Ray

Copy link
Copy Markdown
Contributor

Fixes #6266

Why

The 0.16.1 reliability investigation found Hermes and Kimi startup failures collapsing into the same generic ACP session exited before completion error. The ACP bridge retained stderr only for an AMR-specific retry classifier, so when a child exited before producing a JSON-RPC error, the provider/configuration reason was discarded.

This change preserves actionable startup evidence while bounding it, stripping terminal control codes, and applying the repository's secret/PII redactor before the diagnostic leaves the process boundary.

What users will see

The primary error message stays concise. Support traces and structured error details now identify the failed ACP phase (initialize, session/new, model selection, or prompt), exit code/signal, and a redacted stderr tail when available.

Surface area

  • UI — new page / dialog / panel / menu item / setting / empty state in apps/web or apps/desktop (including Electron menu bar)
  • Keyboard shortcut — new or changed
  • CLI / env var — new od subcommand or flag, new tools-dev / tools-pack flag, or new OD_* env var
  • API / contract — new /api/* endpoint, new SSE event, or changed shape in packages/contracts
  • Extension point — new entry under skills/, design-systems/, design-templates/, or craft/, or change to the skills protocol
  • i18n keys — added new translation keys (see TRANSLATIONS.md for the locale workflow)
  • New top-level dependency — adding any new entry to the root package.json (dependencies or devDependencies); workspace-package package.json files are out of scope. Include a paragraph on what we get vs. what bytes we ship (see CONTRIBUTING.md → Code style)
  • Default behavior change — changes what existing users experience without opting in (default model, default setting, file/SQLite schema, auto-network on startup, auto-install)
  • None — internal refactor, docs, tests, or translation update only

Screenshots

Not applicable; this is an agent-runtime diagnostics change with no UI changes.

Bug fix verification

  • Test path: apps/daemon/tests/acp.test.ts (preserves redacted stderr diagnostics for startup exits; classifies exits after initialize as session setup failures)
  • Both tests failed on main because the error contained neither structured details nor stderr, then passed on this branch.

Validation

  • corepack pnpm --filter @open-design/daemon exec vitest run tests/acp.test.ts (73 passed)
  • corepack pnpm --filter @open-design/daemon exec vitest run tests/amr-acp-integration.test.ts (40 passed)
  • corepack pnpm guard
  • corepack pnpm typecheck

@lefarcen
lefarcen requested a review from nettee July 30, 2026 04:32
@lefarcen lefarcen added size/S PR changes 20-100 lines risk/high High risk: apps/desktop, daemon, auth, migration, workflows, package deps type/bugfix Bug fix needs-validation Runtime change detected; needs human or /explore agent validation. labels Jul 30, 2026
@lefarcen

Copy link
Copy Markdown
Contributor

Queued for QA validation — this PR changes runtime diagnostics that users and support flows rely on, so we want a manual QA pass before merge. Nothing needed from you right now; we will follow up here once it is validated.

@nettee nettee left a comment

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.

@Siri-Ray I reviewed the ACP startup diagnostics change in apps/daemon/src/agent-protocol/acp/session.ts and the new coverage in apps/daemon/tests/acp.test.ts. The phase tracking, bounded stderr capture, ANSI/control-character cleanup, and shared secret redaction all line up with the intended failure-path behavior, and I re-ran corepack pnpm --filter @open-design/daemon exec vitest run tests/acp.test.ts locally with 73 passing tests. Nice tightening of a high-risk runtime path while keeping the user-facing error concise.

🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.

@lefarcen
lefarcen requested a review from ivy-ting July 30, 2026 04:42
Preserve both the startup-diagnostics regression coverage from the PR and the opted-in ACP turn-end completion coverage from main.

Generated-By: looper 0.11.1 (runner=fixer, agent=codex)
@lefarcen
lefarcen requested a review from nettee July 30, 2026 06:58
@lefarcen lefarcen added the backport release/v0.20.0 Backport this fix to release/v0.20.0 label Aug 19, 2026

@AmyShang-alt AmyShang-alt left a comment

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.

@Siri-Ray

Thanks for the contribution. QA found a blocking secret-redaction edge case in the ACP child-exit diagnostic. Please address the inline finding before validation.

Comment thread apps/daemon/src/agent-protocol/acp/session.ts Outdated
@AmyShang-alt AmyShang-alt removed the backport release/v0.20.0 Backport this fix to release/v0.20.0 label Aug 19, 2026
Generated-By: looper 0.11.8 (runner=fixer, agent=codex)
@Siri-Ray

Copy link
Copy Markdown
Contributor Author

Looper fixer round completef43a8e4

  • ✅ Review comment on apps/daemon/src/agent-protocol/acp/session.ts (@AmyShang-alt) — thread
    • Reordered ACP stderr handling in apps/daemon/src/agent-protocol/acp/session.ts so ANSI/control normalization runs before redactSecrets, and added an apps/daemon/tests/acp.test.ts regression for an sk-test credential split by ANSI sequences.

🔁 Powered by Looper · runner=fixer · agent=codex · An autonomous AI dev team for your GitHub repos.

@lefarcen
lefarcen requested a review from AmyShang-alt August 19, 2026 03:32
@ivy-ting

Copy link
Copy Markdown
Contributor

@Siri-Ray

Thanks for the contribution. I revalidated the current head after the ANSI-split credential fix. The focused ACP suite passed 75/75, AMR ACP integration passed 40/40, daemon type checks passed, and current-head CI is green. Child-exit diagnostics now normalize terminal control sequences before redaction, keep stderr bounded, and preserve phase and exit metadata without exposing the tested credential.

Conclusion: Accepted.

@ivy-ting ivy-ting added validated Runtime change validated (via /explore Pass or manual QA). and removed needs-validation Runtime change detected; needs human or /explore agent validation. labels Aug 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

risk/high High risk: apps/desktop, daemon, auth, migration, workflows, package deps size/S PR changes 20-100 lines type/bugfix Bug fix validated Runtime change validated (via /explore Pass or manual QA).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(daemon): preserve ACP startup exit diagnostics

5 participants