Conversation
nhopeatall
approved these changes
Mar 16, 2026
Collaborator
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
LGTM — Clean, well-structured integration test suite for agentDefinitionsRepository that follows existing patterns in the codebase. All CI checks pass. Tests cover CRUD, upsert semantics, Zod validation round-trips, and the seed helper — comprehensive coverage.
Code Issues
Should Fix
- tests/integration/db/agentDefinitionsRepository.test.ts:32 —
result?.hintstests a non-existent property (the schema field ishint, singular). This assertion will always pass trivially sincehintsis never a property onAgentDefinition. Should beresult?.hintif the intent is to verify the field value, or removed entirely if not testing anything meaningful. (Not caught by TypeScript becausetests/is excluded fromtsconfig.json'sinclude.)
Nitpick
- tests/integration/helpers/seed.ts:340-345 —
seedAgentDefinitiondoes shallow merge (...MINIMAL_AGENT_DEFINITION, ...overrides.definition), meaning partial nested overrides (e.g.,{ identity: { emoji: '⭐' } }without other identity fields) would silently drop required fields and fail at Zod validation. The current tests always provide complete nested objects so this works fine today, but a brief JSDoc note about "overrides replace entire top-level fields, not deep-merge" could save future confusion.
🕵️ claude-code · claude-opus-4-6 · run details
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
agentDefinitionsRepositorycovering all CRUD operations, Zod validation, and upsert semanticsseedAgentDefinition()helper totests/integration/helpers/seed.tswith a minimal validAgentDefinitionagent_definitionstable totruncateAll()intests/integration/helpers/db.tsfor test isolationChanges
New:
tests/integration/db/agentDefinitionsRepository.test.ts28 integration tests covering:
upsertAgentDefinition(create): creates a definition, validates via Zod, rejects invalid definitions (emptytaskPrompt, missing capabilities, duplicate capabilities), defaultsisBuiltintofalse, storesisBuiltin: truewhen explicitly setupsertAgentDefinition(update semantics): sameagentTypeupdates, not duplicates;isBuiltinflag updates; multiple distinct types create separate entriesgetAgentDefinition: returnsnullfor missing, retrieves correctly, returns Zod-parsed typelistAgentDefinitions: empty array, full list, field shape, Zod-parsed entriesdeleteAgentDefinition: removes entry, does not affect others, idempotentseedAgentDefinitionhelper: all override paths testedModified:
tests/integration/helpers/seed.tsMINIMAL_AGENT_DEFINITIONconstant — a validAgentDefinitionfixtureseedAgentDefinition(overrides?)helperModified:
tests/integration/helpers/db.tsagent_definitionsto theTRUNCATE TABLEstatement intruncateAll()Test plan
Card
https://trello.com/c/69b8006de55e14a70861b8d8
🤖 Generated with Claude Code
🕵️ claude-code · claude-sonnet-4-6 · run details