Skip to content

fix(client): keep Grok tool names across ACP tool_call_update patches - #2387

Closed
L42y wants to merge 1 commit into
first-tree-ai:mainfrom
L42y:jeremy/grok-unknown-tool-merge
Closed

fix(client): keep Grok tool names across ACP tool_call_update patches#2387
L42y wants to merge 1 commit into
first-tree-ai:mainfrom
L42y:jeremy/grok-unknown-tool-merge

Conversation

@L42y

@L42y L42y commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Grok ACP tool_call_update is a patch: later completed/failed updates often send only status + rawOutput and omit title / _meta["x.ai/tool"].
  • The Grok adapter treated each payload as a full snapshot and labeled the missing name grok:unknown. The Working lane then kept the latest row per toolUseId, so a nameless completion overwrote a good start name.
  • Merge last-known GrokToolInfo by toolCallId before emitting session events. Keep grok:unknown only when that id has never had a name or title.

Validation

  • biome check on the four changed Grok files
  • pnpm check (repo-wide)
  • pnpm typecheck (repo-wide)
  • pnpm --filter @first-tree/client exec vitest run src/providers/grok/__tests__/events.test.ts src/providers/grok/__tests__/handler.test.ts (91 passed)
  • pnpm test (repo-wide)

Change Surface

  • apps/cli public CLI or help output
  • tree onboarding / binding / inspection behavior
  • shipped or planned skill topology
  • docs or contributor-facing repository metadata
  • CI / packaging / release plumbing

Notes

  • package or install behavior changes: none
  • docs or tests updated to match: Grok events + handler coverage for title-less completed updates, including preserved shell args/refs
  • follow-up work: optional Working-lane TOOL_VERBS mapping for Grok names (run_terminal_cmd / run_terminal_command) so shells render as run <command> instead of use <title>

ACP completions often omit title and x.ai/tool meta. Merge by toolCallId
so the Working lane does not overwrite a real name with grok:unknown.

@yuezengwu yuezengwu 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.

Reviewed exact head 785dc09b099777d8b54908e0f5712125b9feeb25.

The Grok ACP patch itself looks sound. Independent exact-head validation passed: git diff --check, pnpm check, build (5/5), typecheck (9/9), the 91 focused Grok tests, and the full client suite (2,898 passed, 7 skipped). All substantive CI jobs are also green.

Blocking issue: Check Branch Name fails because jeremy/grok-unknown-tool-merge does not match the repository's required prefixes (feat/, fix/, refactor/, test/, docs/, chore/, or merge/). Please move this change to a compliant branch/PR (for example, fix/grok-unknown-tool-merge) so the required check can pass, then request a fresh exact-head review.

@L42y

L42y commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by #2388 — same exact head 785dc09b099777d8b54908e0f5712125b9feeb25 on the required fix/ prefix so Check Branch Name can pass.

@L42y L42y closed this Sep 4, 2026
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