refactor(agent-profiles): add getLlmistGadgets() to AgentProfile for unified backend support#424
Conversation
…unified backend support
|
🔍 Reviewing PR... |
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Two gadget-set mismatches that would break review and respond-to-review agents on the llmist backend.
Code Issues
Blocking
-
src/backends/agent-profiles.ts:170-188 —
buildReviewLlmistGadgets()is missing theCreatePRReviewgadget. The review agent's primary purpose is submitting PR reviews viaCreatePRReview. The existingreview.tsagent includes it (line 160), and it's exported fromgadgets/github/index.ts, but it's neither imported nor used in the new builder. A review agent on the llmist backend would be unable to submit its review — the core action of the agent. -
src/backends/agent-profiles.ts line 710 (PROFILE_REGISTRY) —
respond-to-reviewmaps toreviewProfile, which usesbuildReviewLlmistGadgets(true)— a read-only gadget set with no file editing tools. However, the existingrespond-to-reviewagent (respond-to-review.ts:23) usescreatePRAgentGadgets({ includeReviewComments: true })which includesFileSearchAndReplace,WriteFile, andAstGrep, becauserespond-to-reviewneeds to make code changes in response to review feedback. This should usebuildPRAgentLlmistGadgets(true)instead, matching the existing behavior.
Should Fix
- tests/unit/backends/agent-profiles.test.ts:287-341 — All 6 new tests only check
gadgets.length > 0. They don't verify that critical gadgets are present (e.g.,CreatePRReviewfor review, file editing gadgets forrespond-to-review) or that capability-gated gadgets are correctly included/excluded (e.g.,CreatePRonly for implementation). Both blocking issues above would have been caught with more specific assertions. Consider testing for the presence/absence of specific gadget types per profile.
| new Tmux(), | ||
| new Sleep(), | ||
| new TodoUpsert(), | ||
| new TodoUpdateStatus(), |
There was a problem hiding this comment.
BLOCKING: CreatePRReview is missing from this builder. The review agent (see review.ts:149-163) needs CreatePRReview to submit its review — it's the core action. It's exported from gadgets/github/index.ts but not imported here.
Fix: add import { CreatePRReview } from '../gadgets/github/index.js'; (or add it to the existing import block) and include new CreatePRReview() in this gadget array. Also consider whether PostPRComment belongs here — the existing review agent doesn't include it, since it posts comments via CreatePRReview.
There was a problem hiding this comment.
Fixed. Added CreatePRReview to the import block and to buildReviewLlmistGadgets(), replacing PostPRComment to match the authoritative gadget list in review.ts. The review agent now has exactly: ListDirectory, ReadFile, Tmux, Sleep, TodoUpsert/UpdateStatus/Delete, GetPRDetails, GetPRDiff, GetPRChecks, CreatePRReview, UpdatePRComment, Finish — matching the real review agent.
| const gadgets = profile.getLlmistGadgets('review'); | ||
| expect(gadgets).toBeDefined(); | ||
| expect(gadgets.length).toBeGreaterThan(0); | ||
| }); |
There was a problem hiding this comment.
SHOULD_FIX: This test would pass even though CreatePRReview is missing from the review gadgets — it only checks length > 0. Consider verifying specific gadget presence, e.g.:
const gadgetNames = gadgets.map(g => g.constructor.name);
expect(gadgetNames).toContain("CreatePRReview");
expect(gadgetNames).not.toContain("FileSearchAndReplace"); // review is read-onlySimilar assertions for other profiles would catch capability mismatches early.
There was a problem hiding this comment.
Fixed. All tests now verify specific gadget presence/absence using constructor.name assertions. For example, the review test verifies CreatePRReview is present and FileSearchAndReplace/PostPRComment are absent. Each profile test checks capability-gated gadgets are correctly included or excluded. Both blocking bugs (missing CreatePRReview, wrong respond-to-review profile) would now be caught by these tests.
|
👀 Checking this out... |
|
🔍 Reviewing PR... |
aaight
left a comment
There was a problem hiding this comment.
Review of PR #424: getLlmistGadgets() addition to AgentProfile
Great direction overall — moving gadget construction into AgentProfile as a single source of truth is the right architectural call. The structure of the three builder helpers is clean and the capability-gating logic is sound. However, there's one blocking bug that will cause the review agent to silently fail when run via the llmist backend.
🚨 BLOCKING: CreatePRReview is missing from buildReviewLlmistGadgets()
File: src/backends/agent-profiles.ts, around line 188
Comparing the new buildReviewLlmistGadgets() to the authoritative source in src/agents/review.ts (lines 149-163):
// review.ts — the real review agent gadget list
getGadgets: () => [
new ListDirectory(),
new ReadFile(),
new Tmux(),
new Sleep(),
new TodoUpsert(),
new TodoUpdateStatus(),
new TodoDelete(),
new GetPRDetails(),
new GetPRDiff(),
new GetPRChecks(),
new CreatePRReview(), // ← THE CORE ACTION GADGET
new UpdatePRComment(),
new Finish(),
],buildReviewLlmistGadgets() omits CreatePRReview entirely and instead includes PostPRComment (which the real review agent does NOT have). The result: when the llmist backend uses getLlmistGadgets() for the review agent, the agent will have no way to submit a review, which is its sole purpose.
Fix required:
- Add
CreatePRReviewto the import block:import { ..., CreatePRReview, ... } from '../gadgets/github/index.js'; - Replace
new PostPRComment()withnew CreatePRReview()inbuildReviewLlmistGadgets()(or add it alongside and reconsider whetherPostPRCommentbelongs)
⚠️ Test Coverage Too Weak to Catch This Bug
File: tests/unit/backends/agent-profiles.test.ts, around line 307
The new tests only verify gadgets.length > 0, which means the missing CreatePRReview bug above goes completely undetected. A stronger assertion would have caught this:
it('returns non-empty gadget array for review', () => {
const profile = getAgentProfile('review');
const gadgets = profile.getLlmistGadgets('review');
const names = gadgets.map((g) => (g as object).constructor.name);
expect(names).toContain('CreatePRReview'); // core action
expect(names).toContain('GetPRDiff'); // needs to read diff
expect(names).not.toContain('FileSearchAndReplace'); // read-only agent
expect(names).not.toContain('CreatePR'); // no branch creation
});Similar constructor-name assertions for the other profiles (e.g., verifying FileSearchAndReplace is present for implementation but absent for planning) would make these tests much more useful as a safety net during future refactors.
Minor Observations (no change required)
buildPRAgentLlmistGadgets is missing VerifyChanges
src/agents/base.ts includes VerifyChanges for agents with canEditFiles. buildPRAgentLlmistGadgets includes all other editing gadgets (FileSearchAndReplace, FileMultiEdit, WriteFile) but skips VerifyChanges. This may be intentional for PR-branch agents, but worth confirming.
_agentType unused parameter with explicit false
getLlmistGadgets: (_agentType) => buildPRAgentLlmistGadgets(false),The false is redundant since the default is false — calling buildPRAgentLlmistGadgets() would be slightly cleaner. Very minor.
Summary
The blocking issue is the missing CreatePRReview in buildReviewLlmistGadgets(). Please fix that and strengthen the review-agent test to verify the gadget is present. Everything else is solid.
|
@aaight address code review concerns |
Request AddressedWhat was askedAddress code review concerns raised by @nhopeatall and @aaight on the Changes Made
Verification
|
- Add missing CreatePRReview to buildReviewLlmistGadgets() (blocking bug: review agent couldn't submit reviews via llmist backend) - Replace PostPRComment with CreatePRReview to match review.ts gadget list - Create dedicated respondToReviewProfile with buildPRAgentLlmistGadgets(true) instead of reusing read-only reviewProfile (blocking bug: respond-to-review agent had no file editing gadgets) - Add VerifyChanges to buildPRAgentLlmistGadgets() matching base.ts pattern - Remove redundant explicit false in respondToCIProfile - Strengthen tests: verify specific gadget presence/absence per profile using constructor.name assertions instead of just checking array length Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Addressed all code review concerns from @nhopeatall and @aaight: Blocking fixes:
Should-fix improvements: All 2195 tests pass, TypeScript and lint are clean. |
|
🔍 Reviewing PR... |
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
LGTM — clean refactoring step that adds getLlmistGadgets() to AgentProfile, moving toward a single source of truth for agent gadget configuration across backends.
Key Observations
Bugfix embedded in refactoring (good): The respond-to-review profile was previously aliased to reviewProfile (read-only, review-focused). This PR correctly creates a dedicated respondToReviewProfile with file editing capabilities, matching what the actual respond-to-review agent does (it addresses review feedback by making code changes). This was a real bug in the old code.
Gadget parity verified:
buildWorkItemLlmistGadgetsmatchesgetBaseAgentGadgetsfromagents/base.ts✓buildReviewLlmistGadgetsmatchesreview.tsgetGadgets()✓buildPRAgentLlmistGadgetsextendscreatePRAgentGadgetsfromagents/shared/gadgets.tswith three additional gadgets (FileMultiEdit,VerifyChanges,GetPRChecks) — reasonable enhancements that bring PR agents in line with the base agent gadget set for file-editing agents
Minor note (not blocking): The respond-to-review entry in capabilities.ts has canEditFiles: false, isReadOnly: true, which contradicts the actual agent behavior (and this PR's correct gadget assignment). This is a pre-existing inconsistency — the capabilities aren't consulted by buildPRAgentLlmistGadgets so it doesn't cause a bug, but it could confuse future readers. Worth a follow-up cleanup.
Tests are thorough and all pass. CI is green. Clean implementation of the refactoring plan's Steps 1-2.
Summary
This PR implements Step 1 (Audit) and Step 2 (partial) of the dual agent execution path unification refactoring.
Card: https://trello.com/c/699625975a8f4cdd89b359f0
What was implemented
getLlmistGadgets()toAgentProfileinterface — each profile now knows how to construct the llmist gadget instances appropriate for its agent type, makingAgentProfilethe single source of truth for agent behavior across both backendsagent-profiles.ts:buildWorkItemLlmistGadgets()— for briefing, planning, implementation, respond-to-planning-comment, debug agentsbuildReviewLlmistGadgets()— for review and respond-to-review agents (read-only, PR-focused)buildPRAgentLlmistGadgets()— for respond-to-ci and respond-to-pr-comment agents (file editing + PR tools, no CreatePR)getLlmistGadgets()with correct capabilitiesagent-profiles.test.ts(6 new test cases coveringgetLlmistGadgets())Key decisions
getLlmistGadgets(agentType)takes the agent type string to correctly resolve capabilities (e.g.,implementationvsplanningboth usebuildWorkItemLlmistGadgetsbut with different capabilities)buildReviewLlmistGadgets(true)to include review comment tools, aligning with the existingcreatePRAgentGadgets({ includeReviewComments: true })pattern fromagents/shared/gadgets.tsgetBaseAgentGadgets()fromagents/base.ts, consolidating the logic into profilesWhat comes next (follow-up PRs)
LlmistBackend.execute()to use the pre-builtAgentBackendInputand gadgets fromgetLlmistGadgets()instead of delegating to old executor functionsregistry.tsto route llmist throughexecuteWithBackend(removing the dummy input hack)Testing
🤖 Generated with Claude Code