Skip to content

fix(agent): register ph-explore for shared claude sessions#1781

Merged
tatoalo merged 1 commit intomainfrom
fix/claude-ph-explore-registration
Apr 22, 2026
Merged

fix(agent): register ph-explore for shared claude sessions#1781
tatoalo merged 1 commit intomainfrom
fix/claude-ph-explore-registration

Conversation

@tatoalo
Copy link
Copy Markdown
Contributor

@tatoalo tatoalo commented Apr 21, 2026

Problem

Cloud task runs can use the shared Claude session setup, but ph-explore was only registered in a desktop-specific path. That meant the Explore rewrite could target an agent type that was not available for the current session, causing runs to fail when that subagent path was selected.

Changes

  • moved the ph-explore agent definition into shared Claude session code so it is registered consistently across envs
  • updated session option construction to merge shared default agents with caller-provided agents while still allowing explicit overrides
  • removed the duplicate desktop-only wiring and added regression coverage to ensure rewrite targets stay registered in session options.

@tatoalo tatoalo self-assigned this Apr 21, 2026
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Apr 21, 2026

Prompt To Fix All With AI
This is a comment left during a code review.
Path: packages/agent/src/adapters/claude/session/options.test.ts
Line: 23-36

Comment:
**Prefer parameterised test for rewrite registration**

The test iterates over `SUBAGENT_REWRITES` entries in a for-loop inside a single `it`, which obscures which rewrite entry is failing when the assertion fires. Using `it.each` makes each rewrite a named test case and satisfies the project's preference for parameterised tests.

```suggestion
  it.each(Object.entries(SUBAGENT_REWRITES))(
    'registers rewrite target "%s" → "%s" in options.agents',
    (_source, target) => {
      const options = buildSessionOptions(makeParams());
      const registered = new Set(Object.keys(options.agents ?? {}));
      expect(
        registered.has(target),
        `Rewrite target "${target}" is not registered in options.agents — either register the agent in DEFAULT_CLAUDE_AGENTS or remove the rewrite.`,
      ).toBe(true);
    },
  );
```

**Context Used:** Do not attempt to comment on incorrect alphabetica... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))

How can I resolve this? If you propose a fix, please make it concise.

Reviews (1): Last reviewed commit: "fix(agent): register ph-explore for shar..." | Re-trigger Greptile

Comment thread packages/agent/src/adapters/claude/session/options.test.ts
@tatoalo tatoalo requested a review from a team April 21, 2026 17:28
Comment thread packages/agent/src/adapters/claude/session/agents.ts Outdated
@tatoalo tatoalo force-pushed the fix/claude-ph-explore-registration branch from e9b0427 to a0f6dc4 Compare April 22, 2026 09:14
The Explore -> ph-explore rewrite hook was firing in cloud sessions
where ph-explore wasn't registered, because the agent was wired up only
in the desktop-specific service path. The rewrite then targeted a
non-existent agent type and the run failed.

Register ph-explore in the shared session options, make the rewrite
hook defensive (skip + warn when the target isn't registered) and drop
the duplicate desktop wiring. The agent definition is inlined in
options.ts next to buildAgents so there's no extra module just for one
constant.
@tatoalo tatoalo force-pushed the fix/claude-ph-explore-registration branch from a0f6dc4 to d6f2b93 Compare April 22, 2026 09:20
@tatoalo tatoalo merged commit ce4288f into main Apr 22, 2026
15 checks passed
@tatoalo tatoalo deleted the fix/claude-ph-explore-registration branch April 22, 2026 11:29
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