-
Notifications
You must be signed in to change notification settings - Fork 427
feat(agent-hooks): improve amp,claude state detection #1897
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| // @i-know-the-amp-plugin-api-is-wip-and-very-experimental-right-now | ||
| /* global fetch, process */ | ||
|
|
||
| export default function EmdashAmpNotifications(amp) { | ||
| const port = process.env.EMDASH_HOOK_PORT; | ||
| const token = process.env.EMDASH_HOOK_TOKEN; | ||
| const ptyId = process.env.EMDASH_PTY_ID; | ||
| if (!port || !token || !ptyId) return; | ||
|
|
||
| const post = async (eventType, body) => { | ||
| try { | ||
| await fetch(`http://127.0.0.1:${port}/hook`, { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| 'X-Emdash-Token': token, | ||
| 'X-Emdash-Pty-Id': ptyId, | ||
| 'X-Emdash-Event-Type': eventType, | ||
| }, | ||
| body: JSON.stringify(body ?? {}), | ||
| }); | ||
| } catch { | ||
| // Hook delivery is best-effort and must never interrupt Amp. | ||
| } | ||
| }; | ||
|
|
||
| amp.on('agent.start', async (event) => { | ||
| await post('working', { | ||
| message: typeof event.message === 'string' ? event.message : undefined, | ||
| }); | ||
| return {}; | ||
| }); | ||
|
|
||
| amp.on('agent.end', async (event) => { | ||
| await post(event.status === 'error' ? 'error' : 'stop', { | ||
| message: typeof event.message === 'string' ? event.message : undefined, | ||
| }); | ||
| }); | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -5,6 +5,10 @@ import { db } from '@main/db/client'; | |||||||
| import { conversations } from '@main/db/schema'; | ||||||||
| import type { RawHookRequest } from './hook-server'; | ||||||||
|
|
||||||||
| function readString(value: unknown): string | undefined { | ||||||||
| return typeof value === 'string' ? value : undefined; | ||||||||
| } | ||||||||
|
|
||||||||
| function normalizePayload( | ||||||||
| providerId: string, | ||||||||
| body: Record<string, unknown> | ||||||||
|
|
@@ -15,8 +19,8 @@ function normalizePayload( | |||||||
| lastAssistantMessage: (body.last_assistant_message ?? body.lastAssistantMessage) as | ||||||||
| | string | ||||||||
| | undefined, | ||||||||
| title: body.title as string | undefined, | ||||||||
| message: body.message as string | undefined, | ||||||||
| title: readString(body.title), | ||||||||
| message: readString(body.message) ?? readString(body.error_type), | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/main/core/agent-hooks/event-enricher.ts
Line: 23
Comment:
Using `error_type` as a fallback for `message` may surface a raw technical identifier (e.g. `"network_error"`, `"timeout"`) as user-facing text in places that render `payload.message`. If `error_type` is an enum value rather than a human-readable string, consider only falling back to it in a dedicated field (e.g. `errorType`) rather than aliasing it onto `message`.
```suggestion
message: readString(body.message),
errorType: readString(body.error_type),
```
How can I resolve this? If you propose a fix, please make it concise. |
||||||||
| }; | ||||||||
|
|
||||||||
| if (!payload.notificationType && providerId === 'codex' && body.type === 'agent-turn-complete') { | ||||||||
|
|
||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -66,6 +66,8 @@ export type AgentProviderDefinition = { | |
| invertInDark?: boolean; | ||
| terminalOnly?: boolean; | ||
| supportsHooks?: boolean; | ||
| /** Extra env vars injected when this provider is launched. */ | ||
| runtimeEnv?: Record<string, string>; | ||
| }; | ||
|
|
||
| export const AGENT_PROVIDERS: AgentProviderDefinition[] = [ | ||
|
|
@@ -206,6 +208,8 @@ export const AGENT_PROVIDERS: AgentProviderDefinition[] = [ | |
| icon: 'ampcode.png', | ||
| alt: 'Amp CLI', | ||
| terminalOnly: true, | ||
| supportsHooks: true, | ||
| runtimeEnv: { PLUGINS: 'all' }, | ||
|
Comment on lines
+211
to
+212
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/shared/agent-provider-registry.ts
Line: 211-212
Comment:
**Broad `PLUGINS` env var may load unintended plugins**
`PLUGINS: 'all'` is injected into every Amp session. If Amp's plugin system interprets this as "load all discovered plugins" (beyond just those in `.amp/plugins/`), users who have other Amp plugins installed could see them activated unexpectedly when running Amp through Emdash. Since the plugin API is explicitly marked as WIP and experimental, it's worth confirming that `all` is scoped only to the project-local `.amp/plugins/` directory before shipping.
How can I resolve this? If you propose a fix, please make it concise. |
||
| }, | ||
| { | ||
| id: 'opencode', | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agent.endhandler does not return{}, unlikeagent.start. If Amp's plugin API uses the return value for event flow control (as theagent.startpattern suggests), omitting it here could causeagent.endto behave differently or be ignored in future Amp versions. The test suite only asserts thatreturn {};exists somewhere in the plugin string, not that it is present in this specific handler.Prompt To Fix With AI