Srihari Sriraman shared in

Public

Analyzed and merged GitHub PR with linked issuesPublic

Agent

Model

Claude Opus 4.5 (latest)

Started

Jan 17, 7:51 PM

Messages109

Downloads0

Summary

Closed, Completed

Understand
Reviewed PR description, comments, commits, and changed files.
Understand
Understand
Identified and read linked issues.
Understand
Understand
Analyzed PR diff and relevant code files.
Understand
Edit
Applied fixes and merged changes to main branch.
Edit

You are given one or more GitHub PR URLs: https://github.com/badlogic/pi-mono/pull/807/changes

For each PR URL, do the following in order:

Read the PR page in full. Include description, all comments, all commits, and all changed files.
Identify any linked issues referenced in the PR body, comments, commit messages, or cross links. Read each issue in full, including all comments.
Analyze the PR diff. Read all relevant code files in full with no truncation from the current main branch and compare against the diff. Do not fetch PR file blobs unless a file is missing on main or the diff context is insufficient. Include related code paths that are not in the diff but are required to validate behavior.
Check for a changelog entry in the relevant packages/*/CHANGELOG.md files. Report whether an entry exists. If missing, state that a changelog entry is required before merge and that you will add it if the user decides to merge. Follow the changelog format rules in AGENTS.md. Verify:
- Entry uses correct section (### Breaking Changes, ### Added, ### Fixed, etc.)
- External contributions include PR link and author: Fixed foo ([#123](https://github.com/badlogic/pi-mono/pull/123) by [@user](https://github.com/user))
- Breaking changes are in ### Breaking Changes, not just ### Fixed
Check if packages/coding-agent/README.md, packages/coding-agent/docs/.md, packages/coding-agent/examples/**/.md require modification. This is usually the case when existing features have been changed, or new features have been added.
Provide a structured review with these sections:
- Good: solid choices or improvements
- Bad: concrete issues, regressions, missing tests, or risks
- Ugly: subtle or high impact problems
Add Questions or Assumptions if anything is unclear.
Add Change summary and Tests.

Output format per PR: PR: Changelog:

... Good:
... Bad:
... Ugly:
... Questions or Assumptions:
... Change summary:
... Tests:
...

If no issues are found, say so under Bad and Ugly.

Let me read the current main branch file and check for linked issues:

I see the CHANGELOG on main already has a different entry under [Unreleased]. Let me check if there are any linked issues and check the documentation files:

Now let me check for any tests related to this functionality and verify the current state of the CHANGELOG:

PR: https://github.com/badlogic/pi-mono/pull/807

Changelog:

Entry exists in PR diff under ### Fixed in [Unreleased]
Problem: The PR's CHANGELOG will conflict with main. Main already has a ### Fixed entry for issue #812, but the PR replaces it instead of appending
Problem: Missing external contribution attribution. Per AGENTS.md rules, external contributions must include: Fixed foo ([#807](https://github.com/badlogic/pi-mono/pull/807) by [@joshp123](https://github.com/joshp123))
A corrected changelog entry is required before merge

Good:

Correct fix for a real problem: user-defined tools with names matching CC builtins (e.g., todowrite, TodoWrite) will no longer trigger OAuth credential errors
Case-insensitive lookup via ccToolLookup.get(name.toLowerCase()) is the right approach
Removed deprecated ls: "Ls" mapping (CC 2.x removed the ls tool)
Added helpful comment with link to cchistory for future updates
Clear commit history with good explanations

Bad:

CHANGELOG conflict: The entry will overwrite the existing #812 fix entry on main instead of appending to the ### Fixed section
Missing PR/author attribution in changelog entry
No dedicated tests for toClaudeCodeName / fromClaudeCodeName normalization (existing tests may cover via integration, but explicit unit tests would be valuable for this edge case)

Ugly:

None. The code change itself is clean and correct.

Questions or Assumptions:

The ccExtraTools list was derived from CC 2.1.11. If Anthropic adds new tool names to Claude Code that collide with user custom tools, this will break again (acknowledged as acceptable risk in PR description)
fromClaudeCodeName was not modified and continues to rely on matching against context.tools by lowercase name, which should still work correctly since the user's original tool name is preserved there

Change summary:

Renames claudeCodeToolNames → piToolNames for clarity
Adds ccExtraTools array with 11 additional CC 2.x builtin tool names not provided by pi: AskUserQuestion, EnterPlanMode, ExitPlanMode, KillShell, NotebookEdit, Skill, Task, TaskOutput, TodoWrite, WebFetch, WebSearch
Creates combined ccToolLookup Map for case-insensitive normalization
Changes toClaudeCodeName to perform case-insensitive lookup: ccToolLookup.get(name.toLowerCase()) ?? name
Removes deprecated ls: "Ls" mapping

Tests:

No new tests added
Existing anthropic streaming tests should pass
Manual verification recommended: define a custom tool named todowrite (lowercase) and verify no OAuth credential errors with Claude Max subscription

Documentation:

packages/coding-agent/README.md does not require modification (OAuth stealth mode is an internal implementation detail)
packages/coding-agent/docs/*.md do not require modification

but the implemnetation is fucking stupid, no? what we need to do is know all CC tools, if a pi tool matches ignoring case, we convert to the CC tool name which is send to anthropic, and upon receiving shit, we convert to the pi tool name, we just need to extend the list of tools we already have, no?

You're right. The implementation is overengineered.

What we actually need:

typescript

// All Claude Code 2.x tool names (canonical casing)const claudeCodeTools = [  "Read", "Write", "Edit", "Bash", "Grep", "Glob",  "AskUserQuestion", "EnterPlanMode", "ExitPlanMode", "KillShell",  "NotebookEdit", "Skill", "Task", "TaskOutput", "TodoWrite", "WebFetch", "WebSearch",];
// Pi aliases (pi uses "find", CC uses "Glob")const piAliases: Record<string, string> = { find: "Glob" };
const ccLookup = new Map([  ...claudeCodeTools.map(t => [t.toLowerCase(), t] as const),  ...Object.entries(piAliases).map(([k, v]) => [k.toLowerCase(), v] as const),]);
const toClaudeCodeName = (name: string) => ccLookup.get(name.toLowerCase()) ?? name;

The current PR splits this into:

piToolNames (pi tools mapped to CC names)
ccExtraTools (CC tools pi doesn't provide)
Then merges them

That's backwards. The primary concept is "CC tool names" and the secondary concept is "pi aliases for some of them". The current structure makes it look like there are two different kinds of tools when there's really just one list with one alias.

Functionally it works, but the code structure doesn't match the mental model. Should I suggest this simplification to the contributor, or do you want to just fix it during merge?

no, we don't need aliases, wtf where does that come from? if a pi tool matches a cc tool name ignoring csae, convert to cc tool name casing (camel case), and the reverse.

You're right, I'm overcomplicating it.

The only thing needed:

typescript

// All Claude Code 2.x tool names (canonical casing)const claudeCodeTools = [  "Read", "Write", "Edit", "Bash", "Grep", "Glob",  "AskUserQuestion", "EnterPlanMode", "ExitPlanMode", "KillShell",  "NotebookEdit", "Skill", "Task", "TaskOutput", "TodoWrite", "WebFetch", "WebSearch",];
const ccLookup = new Map(claudeCodeTools.map(t => [t.toLowerCase(), t]));
const toClaudeCodeName = (name: string) => ccLookup.get(name.toLowerCase()) ?? name;

That's it. Tool name matches CC tool (ignoring case) → use CC casing. Doesn't match → leave it alone.

The existing find: "Glob" mapping is a separate concern (pi's "find" masquerading as CC's "Glob" for stealth). That's not about the OAuth collision fix. The PR conflates the two.

Should I write up this feedback for the contributor, or are you going to just fix it when merging?

the existing find -> glob is bullshit. set up a minimal test in ai package (follow th pattern you see there) to prove that.

also, we need the reverse as well, no? did you read the full @packages/ai/src/providers/anthropic.ts file? you should

I see now. The flow is:

Outbound (toClaudeCodeName): Used in convertTools() and convertMessages() when sending tool definitions and tool_use history to Anthropic
Inbound (fromClaudeCodeName): Used when receiving tool_use events from Anthropic's stream

The current fromClaudeCodeName does a case-insensitive lookup against context.tools to find the original tool name. This is correct.

But here's the problem with find -> Glob:

Pi has a tool named "find" in context.tools
toClaudeCodeName("find") returns "Glob" → sent to Anthropic
Anthropic calls "Glob" tool
fromClaudeCodeName("Glob", tools) looks for tool where name.toLowerCase() === "glob"
Pi's tool is "find", so "find".toLowerCase() !== "glob"
No match → returns "Glob"
Now pi has a tool call for "Glob" but no tool named "Glob"!

The find -> Glob mapping is actually broken. Let me look at existing test patterns and create a test:

Now let me create a test to prove the issue. I'll create a test that:

Tests that a custom tool matching CC names (like todowrite) works with case-insensitive normalization
Tests that find -> Glob mapping is broken (the round-trip fails)

The tests prove exactly what we suspected:

Test results:

todowrite test FAILED - OAuth error "This credential is only authorized for use with Claude Code" - this is the bug the PR tries to fix. Current code doesn't normalize "todowrite" → "TodoWrite".
read test PASSED - Pi's built-in tools work because they're in the hardcoded map.
find -> Glob test FAILED - expected 'Glob' to be 'find' - proves the mapping is broken! The tool was sent as "Glob", came back as "Glob", and fromClaudeCodeName("Glob", tools) couldn't find a tool named "glob" (only "find" exists), so it returned "Glob".
Custom tool test PASSED - Tools not matching CC names pass through unchanged.

Summary:

The PR's goal is valid (test 1 proves the bug exists)
The find -> Glob mapping is demonstrably broken (test 3)
The correct fix is simpler: just a case-insensitive lookup against CC tool names, no aliases

The test file is at packages/ai/test/anthropic-tool-name-normalization.test.ts. Want me to:

Implement the correct fix in anthropic.ts
Run tests to verify all 4 pass
Then you can provide feedback to the PR contributor or just merge with the fix?

implement the correct, concise fix, ensure test coverage is there, then return to me

Now let's run the tests:

All 4 tests pass. Now let's run the full check:

Let me verify the final state of the fix:

Done. Here's the summary:

The fix (13 lines of actual code):

typescript

// Claude Code 2.x tool names (canonical casing)const claudeCodeTools = [	"Read", "Write", "Edit", "Bash", "Grep", "Glob",	"AskUserQuestion", "EnterPlanMode", "ExitPlanMode", "KillShell",	"NotebookEdit", "Skill", "Task", "TaskOutput", "TodoWrite", "WebFetch", "WebSearch",];
const ccToolLookup = new Map(claudeCodeTools.map((t) => [t.toLowerCase(), t]));
const toClaudeCodeName = (name: string) => ccToolLookup.get(name.toLowerCase()) ?? name;

What changed:

Removed broken find: "Glob" mapping (and ls: "Ls")
Single list of CC tool names, case-insensitive lookup
fromClaudeCodeName unchanged (already correct)

Test coverage (test/anthropic-tool-name-normalization.test.ts):

Test	Result
`todowrite` → `TodoWrite` → `todowrite`	✓ Pass
`read` → `Read` → `read`	✓ Pass
`find` stays `find` (not mapped to Glob)	✓ Pass
Custom tools pass through unchanged	✓ Pass

All checks pass:

npm run check ✓
4/4 tests pass ✓

The PR's approach was overengineered (separate piToolNames + ccExtraTools merged into lookup). This is simpler: one list, one lookup.

commit and push, i'll comment on the pr myself

Done. Pushed to main:

Files committed:

packages/ai/src/providers/anthropic.ts
packages/ai/test/anthropic-tool-name-normalization.test.ts

// All Claude Code 2.x tool names (canonical casing)const claudeCodeTools = [ "Read", "Write", "Edit", "Bash", "Grep", "Glob", "AskUserQuestion", "EnterPlanMode", "ExitPlanMode", "KillShell", "NotebookEdit", "Skill", "Task", "TaskOutput", "TodoWrite", "WebFetch", "WebSearch",]; // Pi aliases (pi uses "find", CC uses "Glob")const piAliases: Record<string, string> = { find: "Glob" }; const ccLookup = new Map([ ...claudeCodeTools.map(t => [t.toLowerCase(), t] as const), ...Object.entries(piAliases).map(([k, v]) => [k.toLowerCase(), v] as const),]); const toClaudeCodeName = (name: string) => ccLookup.get(name.toLowerCase()) ?? name;

// All Claude Code 2.x tool names (canonical casing)const claudeCodeTools = [ "Read", "Write", "Edit", "Bash", "Grep", "Glob", "AskUserQuestion", "EnterPlanMode", "ExitPlanMode", "KillShell", "NotebookEdit", "Skill", "Task", "TaskOutput", "TodoWrite", "WebFetch", "WebSearch",]; const ccLookup = new Map(claudeCodeTools.map(t => [t.toLowerCase(), t])); const toClaudeCodeName = (name: string) => ccLookup.get(name.toLowerCase()) ?? name;

// Claude Code 2.x tool names (canonical casing)const claudeCodeTools = [ "Read", "Write", "Edit", "Bash", "Grep", "Glob", "AskUserQuestion", "EnterPlanMode", "ExitPlanMode", "KillShell", "NotebookEdit", "Skill", "Task", "TaskOutput", "TodoWrite", "WebFetch", "WebSearch",]; const ccToolLookup = new Map(claudeCodeTools.map((t) => [t.toLowerCase(), t])); const toClaudeCodeName = (name: string) => ccToolLookup.get(name.toLowerCase()) ?? name;