Contribution proposal: unit tests for crates/git, Vitest infrastructure, and Claude executor ToolResult support #3084
fatihkutlar
started this conversation in
Ideas
Replies: 0 comments
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Uh oh!
There was an error while loading. Please reload this page.
Hi team,
I've been exploring the codebase and identified three areas where I think I can add meaningful value. I'd like to get your thoughts before opening any PRs — happy to adjust scope or approach based on your feedback.
I'm planning to open three separate PRs, one per topic.
1. Unit tests for
crates/gitcrates/git/src/lib.rsis ~1,970 lines with no#[cfg(test)]blocks. It covers commit, diff, worktree creation/removal, merge, and branch operations — all untested.I'd like to add a
#[cfg(test)]module usingtempfile::TempDirandgit2(both already available as dev-dependencies) following the same pattern as the existing test incrates/worktree-manager/src/worktree_manager.rs. The starting set would covercommit(),is_worktree_clean(),get_diffs(),get_current_branch(),is_branch_name_valid(), andget_head_info(). No production code would be touched.Is there a specific subset of
GitServicemethods the team considers highest priority to cover first?2. Vitest infrastructure + first tests in
packages/web-coreThere are currently no test files anywhere in the frontend packages and no test framework installed.
CONTRIBUTORS.mdmentions Vitest as the expected tool. There's also an orphaned fixture atsrc/test/fixtures/normalized_entries.jsonwith nothing using it.I'd like to add Vitest +
@testing-library/reacttopackages/web-core, avitest.config.tsthat reuses the existing path aliases fromtsconfig.json, and write the first two test suites:utils.test.ts— covering thecn()utility (including documenting the currentclsx-only behaviour whiletwMergeis disabled)useTodos.test.ts— covering the timestamp comparison,hasMeaningfulTodosfiltering, andin_progress/in-progressdetection logic in theuseTodoshookThe orphaned fixture would be used in the
useTodossuite.3. ToolResult support in the Claude executor
There are two places in
crates/executors/src/executors/claude.rswhereToolResultblocks are silently dropped with a TODO comment (lines ~1020 and ~1575). These are the outputs the agent receives back from its own tool calls — bash results, file contents, test output — and they're currently invisible in the conversation log.I'd like to:
ActionType::ToolResult { tool_use_id: String, is_error: bool }variant tocrates/executors/src/logs/mod.rsNormalizedEntryusing the existingnormalize_claude_tool_result_value()helper already in the filegenerate_concise_content()arm for the new variantclaude.rswith cases covering both theClaudeContentItem::ToolResultandClaudeJson::ToolResultpathsI'd keep this scoped to the Claude executor only — no changes to
droid,opencode, or other executors. Frontend rendering would fall through to the existingOtherfallback until a follow-up UI PR is warranted. I'd also runpnpm run generate-types:checkbefore submitting since the new variant derivesTS.Is the TODO comment on these sites still an accurate reflection of the blocker ("when the type system supports it"), or has something changed since it was written?
Thanks for reading. Let me know which of these are aligned with where the project is headed, and I'll start with whichever is most useful.
All reactions