Skip to content

Add read tool and DeepSeek reviewer - #600

Open
jahooma wants to merge 1 commit into
mainfrom
jahooma/research-edit-tool
Open

Add read tool and DeepSeek reviewer#600
jahooma wants to merge 1 commit into
mainfrom
jahooma/research-edit-tool

Conversation

@jahooma

@jahooma jahooma commented May 6, 2026

Copy link
Copy Markdown
Contributor

Adds a new single-file read tool with path, offset, and limit support across common schemas, agent-runtime handling, and the SDK, with tests for truncation, limits, and path handling.
Updates base2/base-deep/general prompts and tool lists to use read, preserves read paths during context pruning, and removes base2 read_files references.
Adds code-reviewer-deepseek for DeepSeek V4 Pro free-mode sessions while keeping code-reviewer-lite for other free models.
Validated with targeted Bun tests and typechecks for agents, common, sdk, and packages/agent-runtime.

@codebuff-team

Copy link
Copy Markdown
Contributor

Nice engineering on both pieces individually:

  • sdk/src/tools/read.ts's truncation logic (2000-line/50KB caps, offset/limit continuation messages, 10MB hard reject) is carefully tested in sdk/src/__tests__/read.test.ts, including edge cases like offset beyond EOF and absolute-vs-relative paths.
  • The DeepSeek reviewer routing in base2.ts is minimal and covered by agents/__tests__/base2.test.ts and the free-agents.test.ts allowlist check.

Two things would need attention before porting:

  1. Two unrelated features in one PR. The read tool and the code-reviewer-deepseek addition touch completely different concerns (tool infra vs. model routing) and would be easier to review and port as separate PRs.
  2. Dropping read_files from base2/base-deep is a real behavior change that isn't explained. agents/base2/base2.ts and agents/base2/base-deep.ts replace 'read_files' outright with 'read' in toolNames, so the default/deep agents lose the ability to batch-read multiple files in a single tool call — they now must issue one read call per file. Meanwhile general-agent.ts keeps both tools side by side. That inconsistency looks unintentional, or at least undocumented; if the intent was to add offset/limit support for large files, read_files could have kept the batch case and read added alongside it (as done in general-agent), rather than swapped wholesale in the primary agents.
  3. Minor: resolveProjectPath in sdk/src/tools/read.ts uses filePath.startsWith(projectRoot) as a substring check (not boundary-aware), which would misclassify a sibling directory like /projectevil/... as "inside" before falling through to the later startsWith(projectRoot + path.sep) check that catches it. It works today but is fragile — worth simplifying directly with path.relative + a single boundary check.

Would suggest splitting into two PRs and adding a rationale (or a fallback) for the read_files removal in base2/base-deep.

@codebuff-team codebuff-team added bot:triaged Classified by the community triage bot pr:needs-work Right idea, not mergeable as written labels Aug 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:triaged Classified by the community triage bot pr:needs-work Right idea, not mergeable as written

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants