feat(agents): join from template generation - #323
Conversation
|
Claude finished @neekolas's task —— View job Code Review Complete ✅I've completed a thorough review of this PR. Overall, this is excellent work — the implementation is clean, well-tested, and follows established patterns. The design doc is comprehensive and the code matches the spec precisely. SummaryThis PR successfully adds Strengths 🎯Architecture & Design
Security
Testing
Code Quality
Issues & Recommendations🟡 Minor Issues1. Missing JSDoc for Test Seam (join.ts:42-46)The exported Recommendation: /**
* Test seam: overrides generation lookup. Pass null to restore default Prisma lookup.
* Mirrors __setTemplateFinderForTests pattern. Not used in production.
*/
export function __setGenerationFinderForTests(
finder: GenerationFinder | null,
): void {
_generationFinder = finder ?? defaultGenerationFinder;
}2. Design Doc Typo (line 363)The verification command references incorrect test filename: pnpm test -- tests/agent-templates-detail.test.ts # adjust to actual filenameShould be: 3. Markdown Linting (design doc line 140)The flow diagram code block is missing a language identifier, which violates markdownlint MD040. Fix: Change 🔵 Suggestions for Future Consideration1. Generation Status Edge CasesThe implementation correctly ignores
Current behavior is correct (fail-open at backend, workflow handles cleanup), but explicit documentation in code comments would help future maintainers. 2. Telemetry Test StabilityThe telemetry test fix (using
3. Error Message ConsistencyThe Not blocking — current wording is acceptable and matches Detailed Code Analysissrc/api/v2/agents/handlers/join.tsLines 20-23: Type definitions are clean type GenerationRow = Awaited<ReturnType<typeof prisma.agentTemplateGeneration.findUnique>>;
type GenerationFinder = (id: string) => Promise<GenerationRow>;✅ Good use of Lines 97, 110-114: Schema changes are correct generationId: z.string().uuid().optional(),
// ...
.refine(
(b) => !(b.templateId !== undefined && b.generationId !== undefined),
{ message: "Provide at most one of templateId or generationId", path: ["generationId"] }
)✅ UUID validation, mutual exclusivity properly enforced Lines 565-607: Generation resolution logic
Lines 664-666: Dispatch body construction if (generationId !== undefined) {
dispatchBody.generationId = generationId;
}✅ Only adds field when present, keeps dispatch minimal Lines 175: Schema extension generationId: z.string().uuid().optional(),✅ Added to tests/agents-join.test.tsLines 83-108: Lines 1009-1187: Generation test coverage is excellent
Lines 351-368, 370-382: Input validation tests tests/agent-templates.detail.test.tsLines 235-271: Draft template API key access test test("returns a draft template to an agent API key caller", async () => {
// ... creates draft owned by API_KEY_DRAFT_OWNER_ID
const reader = await readDetail({ path: `/api/v2/agent-templates/${draft.id}` });
expect(reader.response.status).toBe(404); // Hidden from regular auth
const response = await fetch(`${baseURL}/api/v2/agent-templates/${draft.id}`,
{ headers: agentKeyHeaders() }
);
expect(response.status).toBe(200); // Visible to API key
expect(body.status).toBe("draft");
});✅ Excellent addition — locks in the downstream resolution behavior tests/telemetry-metrics.test.tsLines 33, 228-234: Async test fix const BATCH_ID = "77777777-7777-4777-8777-777777777777"; // Changed from previous value
// ...
test("forward failure → 502 and batch NOT recorded", async () => {
vi.mocked(forwardMetrics).mockResolvedValue(false);
const res = await post(makeApp()).send(makeBody());
expect(res.status).toBe(502);
await vi.waitFor(async () => {
expect(await prisma.telemetryBatch.findUnique({
where: { batchId: BATCH_ID }
})).toBeNull();
});
});✅ Proper async handling with Security Review ✅Ownership Validation
Input Validation
Authorization
Performance Considerations ✅
Test Coverage Assessment ✅New Functionality Coverage
Regression Coverage
Edge Cases Covered
Coverage assessment: Excellent — all acceptance criteria and edge cases tested Verdict✅ Approve with Minor SuggestionsThis is production-ready code. The three minor issues identified (missing JSDoc, design doc typo, markdown linting) are non-blocking and can be addressed in a follow-up if desired. Why This Is Good Work
Recommended Next Steps
Questions for Consideration
Great work on this feature! The implementation quality is high and the testing is thorough. 🎉 |
Walkthrough
ChangesgenerationId join support and draft template visibility
Telemetry test async fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 markdownlint-cli2 (0.22.1)docs/plans/2026-06-22-generation-id-join-design.mdmarkdownlint-cli2 v0.22.1 (markdownlint v0.40.0) Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ApprovabilityVerdict: Needs human review New feature adding No code changes detected at You can customize Macroscope's approvability policy. Learn more. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/api/v2/agents/handlers/join.ts (1)
42-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd JSDoc for the new exported test seam.
__setGenerationFinderForTestsis exported but undocumented; adding a short JSDoc block keeps exported API intent clear.As per coding guidelines: “Add JSDoc comments for public APIs.”
📝 Suggested fix
+/** + * Overrides generation lookup in tests. Pass `null` to restore default Prisma lookup. + */ export function __setGenerationFinderForTests( finder: GenerationFinder | null, ): void { _generationFinder = finder ?? defaultGenerationFinder; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/v2/agents/handlers/join.ts` around lines 42 - 47, The exported function __setGenerationFinderForTests lacks JSDoc documentation which is required for all public APIs per the coding guidelines. Add a JSDoc comment block directly above the function definition that briefly describes its purpose as a test seam for setting the GenerationFinder instance, including parameter and return type documentation.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/plans/2026-06-22-generation-id-join-design.md`:
- Line 363: The test command in the verification steps at line 363 references an
incorrect test file path `tests/agent-templates-detail.test.ts`. Update the
command to use the correct filename `tests/agent-templates.detail.test.ts` (note
the period before 'detail' instead of hyphen). Change the pnpm test command to
reference the actual test file path that exists in this PR.
- Line 140: The fenced code block in the flow diagram documentation (starting
with "POST /v2/agents/join") is missing a language identifier after the opening
triple backticks, which violates markdownlint rule MD040. Add "text" as the
language identifier to the opening fence (change ``` to ```text) to make the
code block compliant with markdown linting standards.
---
Nitpick comments:
In `@src/api/v2/agents/handlers/join.ts`:
- Around line 42-47: The exported function __setGenerationFinderForTests lacks
JSDoc documentation which is required for all public APIs per the coding
guidelines. Add a JSDoc comment block directly above the function definition
that briefly describes its purpose as a test seam for setting the
GenerationFinder instance, including parameter and return type documentation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 235a1946-e79c-41b8-8764-8abbd4a3eaa7
📒 Files selected for processing (5)
docs/plans/2026-06-22-generation-id-join-design.mdsrc/api/v2/agents/handlers/join.tstests/agent-templates.detail.test.tstests/agents-join.test.tstests/telemetry-metrics.test.ts
| resolves **no** template, leaving `resolvedTemplate = null` and forwarding | ||
| `generationId` in the dispatch body. | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language identifier to the fenced flow diagram block.
Line 140 opens a fenced code block without a language tag, which triggers markdownlint MD040.
📝 Suggested fix
-```
+```text
POST /v2/agents/join { conversationId, generationId, options:{onboarding:"agent-builder"} }
│
├─ bodySchema: at-most-one(templateId, generationId); exactly-one(slug, conversationId)
@@
→ detail.ts serves it (draft OK for isApiKeyListener) ← this spec only CONFIRMS this works
-```
+```📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 140-140: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/plans/2026-06-22-generation-id-join-design.md` at line 140, The fenced
code block in the flow diagram documentation (starting with "POST
/v2/agents/join") is missing a language identifier after the opening triple
backticks, which violates markdownlint rule MD040. Add "text" as the language
identifier to the opening fence (change ``` to ```text) to make the code block
compliant with markdown linting standards.
Source: Linters/SAST tools
| pnpm test -- tests/agents-join.test.ts | ||
|
|
||
| # Detail-handler regression (draft + API-key): | ||
| pnpm test -- tests/agent-templates-detail.test.ts # adjust to actual filename |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the test command filename in verification steps.
Line 363 references tests/agent-templates-detail.test.ts, but the test file path in this PR is tests/agent-templates.detail.test.ts.
📝 Suggested fix
-pnpm test -- tests/agent-templates-detail.test.ts # adjust to actual filename
+pnpm test -- tests/agent-templates.detail.test.ts📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pnpm test -- tests/agent-templates-detail.test.ts # adjust to actual filename | |
| pnpm test -- tests/agent-templates.detail.test.ts |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/plans/2026-06-22-generation-id-join-design.md` at line 363, The test
command in the verification steps at line 363 references an incorrect test file
path `tests/agent-templates-detail.test.ts`. Update the command to use the
correct filename `tests/agent-templates.detail.test.ts` (note the period before
'detail' instead of hyphen). Change the pnpm test command to reference the
actual test file path that exists in this PR.
Co-authored-by: Claude <noreply@anthropic.com>
acc360d to
6c10495
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/api/v2/agents/handlers/join.ts (1)
20-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the new generation-finder seam with the repo’s TypeScript function-signature conventions.
The newly added finder uses a positional parameter and explicit return type on the exported setter. Please switch this seam to object-argument style and inferred returns for consistency with
srcrules.♻️ Suggested refactor
-type GenerationFinder = (id: string) => Promise<GenerationRow>; +type GenerationFinder = (args: { id: string }) => Promise<GenerationRow>; -const defaultGenerationFinder: GenerationFinder = (id) => - prisma.agentTemplateGeneration.findUnique({ where: { id } }); +const defaultGenerationFinder: GenerationFinder = ({ id }) => + prisma.agentTemplateGeneration.findUnique({ where: { id } }); -export function __setGenerationFinderForTests( - finder: GenerationFinder | null, -): void { +export function __setGenerationFinderForTests( + finder: GenerationFinder | null, +) { _generationFinder = finder ?? defaultGenerationFinder; } - generation = await _generationFinder(generationId); + generation = await _generationFinder({ id: generationId });As per coding guidelines,
src/**/*.{ts,tsx}should use object parameter syntax and avoid explicit function return types when inference is sufficient.Also applies to: 27-28, 42-46, 568-568
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/v2/agents/handlers/join.ts` around lines 20 - 24, The GenerationFinder type definition uses positional parameter syntax and explicit return type which conflicts with the repository's TypeScript conventions. Refactor the GenerationFinder type (and any other similar function signatures referenced in lines 27-28, 42-46, and 568) to use object-argument style (passing arguments as an object with named properties instead of positional parameters) and remove the explicit Promise return type to allow TypeScript to infer it automatically. This ensures consistency with the codebase's typing conventions for src/**/*.{ts,tsx} files.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/api/v2/agents/handlers/join.ts`:
- Around line 20-24: The GenerationFinder type definition uses positional
parameter syntax and explicit return type which conflicts with the repository's
TypeScript conventions. Refactor the GenerationFinder type (and any other
similar function signatures referenced in lines 27-28, 42-46, and 568) to use
object-argument style (passing arguments as an object with named properties
instead of positional parameters) and remove the explicit Promise return type to
allow TypeScript to infer it automatically. This ensures consistency with the
codebase's typing conventions for src/**/*.{ts,tsx} files.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d12653b7-1b7d-4585-9a38-912e9fc71bbd
📒 Files selected for processing (5)
docs/plans/2026-06-22-generation-id-join-design.mdsrc/api/v2/agents/handlers/join.tstests/agent-templates.detail.test.tstests/agents-join.test.tstests/telemetry-metrics.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/agents-join.test.ts
1 participant
Summary
This PR lets
POST /api/v2/agents/joinstart an assistant from an in-progress agent template generation. Callers can now pass agenerationIdinstead of waiting for generation to complete and then passingtemplateId.The join handler validates that the generation exists and belongs to the joining account, then dispatches convos-assistants with
template: null, the original callerownerAccountId, and the forwardedgenerationId. ExistingtemplateIdjoins and bare joins are left unchanged.What changed
generationIdsupport to the agents join request schema.templateIdandgenerationIdas mutually exclusive inputs.generationIdto the strict assistant dispatch envelope so generation-based joins do not fail dispatch validation.404 GENERATION_NOT_FOUND403 GENERATION_FORBIDDEN500 GENERATION_LOOKUP_FAILEDstatusortemplateId, so pending generations can be used to start provisioning.templateId + options.onboarding=agent-builderrejection while allowinggenerationId + options.onboarding=agent-builder.docs/plans/2026-06-22-generation-id-join-design.md.Verification
DATABASE_URL='postgres://postgres:convos@localhost:5432/postgres?sslmode=disable' pnpm test tests/agents-join.test.ts tests/agent-templates.detail.test.ts tests/invites.test.ts tests/notifications-webhook-account-guard.test.tsDATABASE_URL='postgres://postgres:convos@localhost:5432/postgres?sslmode=disable' pnpm test tests/telemetry-metrics.test.ts tests/telemetry-dedup.test.tsDATABASE_URL='postgres://postgres:convos@localhost:5432/postgres?sslmode=disable' pnpm run test:localpnpm run checktsc --noEmitprettier --check .eslint .git diff --checkNotes
This PR only covers the convos-backend side of generation-based joins. The convos-assistants workflow that polls generation progress and resolves the final template remains out of scope for this branch.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Add
generationIdsupport to POST /v2/agents/join as an alternative totemplateIdgenerationId(UUID) in the request body, mutually exclusive withtemplateId.generationIdis provided, the handler looks up the generation, enforces ownership (returns 404/403/500 on failure), and forwardsgenerationIdwithtemplate: nullto the assistants service dispatch.__setGenerationFinderForTestsseam mirroring the existing template finder override, with full test coverage in agents-join.test.ts.templateIdandgenerationIdnow return 400INVALID_REQUEST.Macroscope summarized 6c10495.
Summary by CodeRabbit