Skip to content

refactor(agents): remove dead hook IPC and derive shared agent defaults - #16089

Merged
brennanb2025 merged 4 commits into
stablyai:mainfrom
trs-80:refactor/agent-registry-tier1
Aug 31, 2026
Merged

brennanb2025 merged 4 commits into
stablyai:mainfrom
trs-80:refactor/agent-registry-tier1

Conversation

@trs-80

@trs-80 trs-80 commented Aug 23, 2026 •

Copy link
Copy Markdown
Contributor

Summary

This PR removes an unused per-agent hook-status IPC surface and consolidates two duplicated agent-registry behaviors without changing the exported desktop configuration shape:

  1. Removes the unused window.api.agentHooks.*Status IPC/preload/web surface. Existing agent launch/status tracking remains intact.
  2. Authors launchCmd and expectedProcess as optional source fields and resolves both from detectCmd once at module initialization.
  3. Derives the mobile agent order, labels, and picker helpers from the shared TUI registry while keeping mobile-only favicon domains local.
  4. Updates the web preload composition contract test to match the intentionally removed namespace.

The branch was rebased onto current main during review. Final diff: 12 files, +93 / -592.

Why

These facts were duplicated across desktop and mobile and could drift when agents were added or changed. The final implementation keeps one shared source for stable agent metadata while preserving existing consumer contracts and leaving broader registry consolidation out of scope.

Compatibility and risk

  • No RPC, remote-wire, persisted-state, route, pairing, or protocol changes.
  • No new dependencies, subprocesses, timers, listeners, polling, or network behavior.
  • Mobile IDs, ordering, labels, disabled-agent semantics, and provider-neutral behavior are preserved.
  • No platform-specific path, shell, shortcut, native-module, SSH, relay, or folder-workspace behavior changed.
  • Registry resolution is a bounded 36-entry module-initialization pass; mobile membership checks use Object.hasOwn.

Verification

  • pnpm typecheck
  • Mobile TypeScript check
  • pnpm lint
  • pnpm build
  • Focused root and mobile unit/integration suites covering the changed registries, preload composition, and hook IPC
  • iOS Metro export: 4,638 modules bundled
  • Same-model senior review loop: clean after fixing the stale web preload composition expectation
  • Opening and closing release-readiness reviews: no proven P0/P1/P2 findings

Electron rendered QA

Validated this exact worktree with isolated user data and Playwright CDP. The real Create Worktree and New Tab agent pickers rendered the expected detected-agent labels/order; Codex was selected and launched successfully. window.api.agentHooks was absent, while agentStatus and launch/foreground tracking remained present. Renderer console had no errors.

Agent picker

Codex selected

Codex launched

New-tab agent picker

Native mobile QA

Native iOS validation was attempted on iPhone 17 Pro through the Orca-managed emulator and this worktree's Metro bundle. It was blocked before the agent picker by an existing home-screen hosts.toSorted render error outside this PR's diff. No web render was substituted for native evidence; the changed catalog remains covered by focused tests and the successful iOS Metro export.

AI disclosure

The original implementation was AI-assisted. The final rebased head was reviewed by multiple independent agents plus a same-model fix/review loop, with only one meaningful regression found and fixed: the stale web preload surface assertion.

@coderabbitai

coderabbitai Bot commented Aug 23, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 682b5a7d-cdeb-48b0-b38d-710c5c9ea669

📥 Commits

Reviewing files that changed from the base of the PR and between bc98655 and feeea7d.

📒 Files selected for processing (12)
  • mobile/src/tasks/mobile-agent-catalog.test.ts
  • mobile/src/tasks/mobile-tui-agents.ts
  • src/main/ipc/agent-hooks.test.ts
  • src/main/ipc/agent-hooks.ts
  • src/preload/api-types.ts
  • src/preload/api/agent-hook-api.ts
  • src/preload/index.ts
  • src/renderer/src/web/preload-api/web-agent-hooks-api.ts
  • src/renderer/src/web/web-preload-api-composition.test.ts
  • src/renderer/src/web/web-preload-api.ts
  • src/shared/tui-agent-config.test.ts
  • src/shared/tui-agent-config.ts
💤 Files with no reviewable changes (7)
  • src/renderer/src/web/web-preload-api-composition.test.ts
  • src/preload/api/agent-hook-api.ts
  • src/preload/index.ts
  • src/renderer/src/web/preload-api/web-agent-hooks-api.ts
  • src/main/ipc/agent-hooks.test.ts
  • src/renderer/src/web/web-preload-api.ts
  • src/main/ipc/agent-hooks.ts
🚧 Files skipped from review as they are similar to previous changes (5)
  • src/preload/api-types.ts
  • src/shared/tui-agent-config.test.ts
  • mobile/src/tasks/mobile-tui-agents.ts
  • mobile/src/tasks/mobile-agent-catalog.test.ts
  • src/shared/tui-agent-config.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

The change centralizes TUI agent configuration defaults and reuses shared ordering, labels, validation, enablement, and selection logic in mobile code. Mobile catalog tests now validate shared configuration and display names. Provider-specific hook status IPC handlers, preload types, preload bindings, and web fallback APIs were removed. Generic agent status handling remains. Configuration tests cover defaults and explicit command overrides.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 5 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ⚠️ Warning The description is detailed and on-topic. It explains the changes, rationale, compatibility impact, verification, visual evidence, mobile limitations, and AI use. However, it omits the required linked… Add a valid issue reference under Linked Issue, such as Fixes #123``. Include or explicitly address the required ELI5, What Changed, Visual Proof, Testing, Review, Agent skill upstream boundary, Notes, and Checklist sections. Confirm the re…
✅ Passed checks (3 passed)
Check name Status Explanation
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.
Title check ✅ Passed The title clearly summarizes the two primary changes: removing unused agent hook IPC and deriving shared agent defaults.
Full details: Description check

Explanation

The description is detailed and on-topic. It explains the changes, rationale, compatibility impact, verification, visual evidence, mobile limitations, and AI use. However, it omits the required linked issue, does not use several required template sections, and does not include the checklist confirmations.

Resolution

Add a valid issue reference under Linked Issue, such as Fixes #123``. Include or explicitly address the required ELI5, What Changed, Visual Proof, Testing, Review, Agent skill upstream boundary, Notes, and Checklist sections. Confirm the required checks and manual testing status.


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.

❤️ Share

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

@brennanb2025

Copy link
Copy Markdown
Contributor

Thanks for this — the audit behind it (recent agent additions touching 27–47 files, a third of those edits re-declaring facts already in TUI_AGENT_CONFIG) matches what we've been running into, and three of the four cleanups here are straightforwardly good.

The ask: drop b3d5bf8e47 and keep the other three.

Why just that one

Main moved under this PR while it was open. src/shared/agent-title-evidence.ts landed in #16148, and it doesn't deduplicate the title chain — it replaces the way title identity is decided. collectAgentTitleEvidence sorts signals into three classes (vendor marker / anchored name / free-text name) and ranks by class, so a shuffle of its registry cannot change the answer. #16157 then put the whole title signal behind hook, process, and launch evidence.

AGENT_TITLE_MATCHERS is the opposite contract. It makes "first matcher wins" explicit and public, and preserves the order faithfully — which is exactly right as a refactor, and exactly the thing being removed. Concretely:

  • It keeps all 8 defects pinned by the characterization suite in test(agent-status): characterize title-derived agent identity before the resolver change #16144. Four are real recorded panes: Switch Claude and Codex off load balance… - grok is a Grok pane that resolves to Codex, because Codex is checked first and the task text mentions it.
  • The table's own test asserts list position (at(-1) must be claude), so ordering becomes a documented invariant future changes have to protect.
  • It leaves two live chains in src/shared with opposite semantics — one order-dependent, one deliberately not.

The duplicate chains do still need to go. They come out as consumers migrate onto the evidence parser (#16237 is the first), so that cleanup arrives without publishing an ordered table in the meantime.

The three to keep

  • 39ea6ad7a0 — dead agentHooks:*Status IPC. Verified: zero renderer/CLI/mobile callers on current main, and the surface had already drifted (kimiStatus existed in main and preload but not in the type or the web stub).
  • d1f7975507 — launchCmd/expectedProcess defaulting to detectCmd. Resolved table verified toEqual the previous one.
  • 18e26e053f — mobile registry derived from src/shared. Verified labels identical to TUI_AGENT_DISPLAY_NAMES across all 36 agents, the shared helpers all exist, and the import is Metro-safe.

Those are real reductions we'd otherwise have to write ourselves, and none of them are affected by the identity work.

Practical note

The branch needs a rebase regardless — git merge-tree now reports conflicts in terminal-title-agent-type.ts and agent-hooks.ts against current main. If it's easier, drop b3d5bf8e47 during that rebase and the remaining three should apply cleanly.

Happy to take the rebase off your hands if you'd prefer — say the word and we'll land the three with credit to you.

@trs-80
trs-80 force-pushed the refactor/agent-registry-tier1 branch from 18e26e0 to 4930851 Compare August 25, 2026 02:18
@coderabbitai

coderabbitai Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

@trs-80

trs-80 commented Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto current main (cc4801320a) with b3d5bf8e47 dropped — agreed that publishing an ordered matcher table has no place once agent-title-evidence owns title identity, and the duplicate chain can come out through the #16237 migration path instead.

The three keepers are now:

  • 604c2c151f — dead agentHooks:*Status IPC. The only conflict of the rebase was here: main's feat(agent-status): add an order-independent title evidence parser #16148-era extraction of the agentStatus:drop* listeners into registerAgentStatusRowTeardownIpcHandlers() crossed with this commit's import removals. Resolved to current main's structure minus the 14 per-agent hook-service imports; no agentHooks: handler survives and the hook services have no other reference in the file.
  • 0be1fc4458 — launchCmd/expectedProcess defaulting to detectCmd (applied clean).
  • 4930851ab6 — mobile registry derived from src/shared (applied clean).

Re-verified on the new base: full pnpm typecheck (node/cli/web/e2e) clean, agent-hooks + tui-agent-config suites pass, and the mobile package typechecks with all 3,681 tests passing.

🤖 Generated with Claude Code

trs-80 and others added 4 commits August 25, 2026 11:29
No renderer, CLI, or mobile caller invoked window.api.agentHooks.*Status; main
already reads install status through MANAGED_AGENT_HOOK_STATUS_READERS. The
14 handlers had also drifted (kimiStatus existed in main/preload but not in
AgentHooksApi or the web stub).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…detectCmd

32 of 36 entries repeated the binary name three times. Entries are now
authored in a source form where both default to detectCmd and resolved once
at module load, so TUI_AGENT_CONFIG keeps its exact shape for consumers
(verified equal to the previous table).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…/shared

The mobile mirror (and its regex-over-desktop-source parity test) predates
mobile importing runtime values from src/shared, which it now does in a dozen
modules. Only the favicon-domain map stays mobile-local because desktop's lives
in the renderer catalog next to bundled ?url imports. The parity test now
imports the real registries and also checks label parity.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@brennanb2025
brennanb2025 force-pushed the refactor/agent-registry-tier1 branch from 4930851 to feeea7d Compare August 25, 2026 19:40
@coderabbitai

coderabbitai Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

@brennanb2025 brennanb2025 changed the title refactor(agents): declare each agent fact once — dead hook IPC, title matcher table, config defaults, mobile derivation refactor(agents): remove dead hook IPC and derive shared agent defaults Aug 25, 2026
@brennanb2025

Copy link
Copy Markdown
Contributor

Review status: ready after required CI

Reviewed final head feeea7d6b09f69f1ea6781c843ee40cbf05fbb3f and pushed the rebased branch to the contributor PR.

Changes made during review

  • Rebased onto current main and resolved the web preload split within the PR's existing scope.
  • Fixed one meaningful regression: the exact web preload composition test still expected the intentionally removed agentHooks namespace (feeea7d6b0).
  • Updated the PR title/body to match the actual 12-file diff and added rendered Electron evidence.

Review result

  • Same-model senior review/fix loop: CLEAN after the test correction.
  • Opening and closing readiness checklists: PASS, with zero proven P0/P1/P2 findings; counsel was correctly skipped because there were no candidates.
  • No scope expansion, dependency/supply-chain change, hot-path regression, new polling/listeners/timers, wire/persistence change, or Windows/Linux/SSH/folder-workspace/provider-specific regression found.

Verification

  • Root and mobile typechecks passed.
  • Lint and build passed.
  • 117 focused root tests and 17 focused mobile tests passed.
  • iOS Metro export passed (4,638 modules).
  • git diff --check passed.
  • Electron Playwright CDP validation passed on this exact worktree: the real Create Worktree and New Tab agent pickers rendered correctly, Codex selected/launched successfully, window.api.agentHooks was absent, and remaining agentStatus plus launch/foreground tracking stayed intact. Screenshots are attached in the PR description.

Remaining gap

Native iOS picker validation was attempted through the Orca-managed emulator, but the app hit an existing hosts.toSorted home-screen render error in mobile/src/home/use-mobile-home-data.ts:131 before reaching the picker. That file is outside this PR's diff, so this is not a PR finding; native QA is reported as blocked rather than replaced with web evidence.

GitHub now reports the rebased PR as mergeable. Required checks have not populated for this pushed SHA yet, so the landing recommendation is: land after required CI passes.

@brennanb2025
brennanb2025 merged commit a651e81 into stablyai:main Aug 31, 2026
49 checks passed
enrick-dev pushed a commit to enrick-dev/orca that referenced this pull request Oct 15, 2026
…ts (stablyai#16089)

* refactor(agent-hooks): drop the unused per-agent hook status IPC surface

No renderer, CLI, or mobile caller invoked window.api.agentHooks.*Status; main
already reads install status through MANAGED_AGENT_HOOK_STATUS_READERS. The
14 handlers had also drifted (kimiStatus existed in main/preload but not in
AgentHooksApi or the web stub).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* refactor(tui-agent-config): default launchCmd and expectedProcess to detectCmd

32 of 36 entries repeated the binary name three times. Entries are now
authored in a source form where both default to detectCmd and resolved once
at module load, so TUI_AGENT_CONFIG keeps its exact shape for consumers
(verified equal to the previous table).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* refactor(mobile): derive the agent order, labels, and picker from src/shared

The mobile mirror (and its regex-over-desktop-source parity test) predates
mobile importing runtime values from src/shared, which it now does in a dozen
modules. Only the favicon-domain map stays mobile-local because desktop's lives
in the renderer catalog next to bundled ?url imports. The parity test now
imports the real registries and also checks label parity.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* test(web): align preload surface after hook IPC removal

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: Brennan Benson <79079362+brennanb2025@users.noreply.github.com>
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