Add runtime artifact list and get-by-ID API endpoints#2938
Add runtime artifact list and get-by-ID API endpoints#2938tim-inkeep wants to merge 5 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 7a86f16 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
TL;DR — Adds list and get-by-ID endpoints for runtime artifacts on both the admin manage API ( Key changes
Summary ｜ 10 files ｜ 2 commits ｜ base: Data access layer for artifact queries
The Admin artifact endpoints on the manage domain
Both routes use
End-user artifact endpoints on the run domain
The list endpoint automatically scopes by
|
|
TL;DR — Adds list and get-by-ID endpoints for runtime artifacts in both the manage API (admin-facing, project-scoped) and the run API (end-user-facing, JWT-scoped). This mirrors the existing conversations API pattern, backed by two new DAL functions in Key changes
Summary ｜ 14 files ｜ 5 commits ｜ base: Manage-domain artifact listing and detail
The list response uses
End-user artifact routes with JWT scoping
The list endpoint uses
New DAL functions for artifact queries
The
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overall well-structured PR that mirrors the existing conversations API pattern. The DAL layer, middleware registration, auth, and tests are solid. Two medium-severity issues around the run route's manual field-picking (fragile to schema evolution) and a missing conversationId ownership check on the list endpoint that could leak artifact existence via count.
Claude Opus ｜ 𝕏
| data: result.artifacts.map((artifact) => ({ | ||
| id: artifact.id, | ||
| tenantId: artifact.tenantId, | ||
| projectId: artifact.projectId, | ||
| taskId: artifact.taskId, | ||
| toolCallId: artifact.toolCallId, | ||
| contextId: artifact.contextId, | ||
| type: artifact.type, | ||
| name: artifact.name, | ||
| description: artifact.description, | ||
| summary: artifact.summary, | ||
| mime: artifact.mime, | ||
| visibility: artifact.visibility, | ||
| createdAt: toISODateString(artifact.createdAt), | ||
| updatedAt: toISODateString(artifact.updatedAt), | ||
| })), |
There was a problem hiding this comment.
This manual field-picking in the list handler is fragile — if a new column is added to LedgerArtifactApiSelectSchema (and the list item schema), this mapping silently drops it. The ArtifactListItemSchema already defines the shape via .omit() on the API select schema, so you could destructure or spread-and-strip instead.
For example, you could spread each artifact and delete the excluded keys, or use a helper. This is the same concern the route-handler-authoring skill flags for request bodies, but it applies equally to response mapping.
| data: result.artifacts.map((artifact) => ({ | |
| id: artifact.id, | |
| tenantId: artifact.tenantId, | |
| projectId: artifact.projectId, | |
| taskId: artifact.taskId, | |
| toolCallId: artifact.toolCallId, | |
| contextId: artifact.contextId, | |
| type: artifact.type, | |
| name: artifact.name, | |
| description: artifact.description, | |
| summary: artifact.summary, | |
| mime: artifact.mime, | |
| visibility: artifact.visibility, | |
| createdAt: toISODateString(artifact.createdAt), | |
| updatedAt: toISODateString(artifact.updatedAt), | |
| })), | |
| data: result.artifacts.map(({ tenantId, projectId, parts, metadata, allowedAgents, derivedFrom, ...rest }) => ({ | |
| ...rest, | |
| createdAt: toISODateString(rest.createdAt), | |
| updatedAt: toISODateString(rest.updatedAt), | |
| })), |
| const { page = 1, limit = 20, conversationId } = c.req.valid('query'); | ||
|
|
||
| const result = await listLedgerArtifacts(runDbClient)({ | ||
| scopes: { tenantId, projectId }, | ||
| userId: endUserId, | ||
| conversationId, | ||
| pagination: { page, limit }, | ||
| }); |
There was a problem hiding this comment.
When conversationId is provided, the list query filters by contextId at the DAL level but there is no ownership check verifying that the conversation belongs to endUserId. An attacker who guesses another user's conversationId won't see the artifacts (the userId filter handles that), but the total count in the response could leak whether artifacts exist for that conversation.
Consider validating conversation ownership upfront when conversationId is supplied — same pattern used in the get-by-ID handler below (lines 171-181).
| const { page = 1, limit = 20, conversationId } = c.req.valid('query'); | |
| const result = await listLedgerArtifacts(runDbClient)({ | |
| scopes: { tenantId, projectId }, | |
| userId: endUserId, | |
| conversationId, | |
| pagination: { page, limit }, | |
| }); | |
| const { page = 1, limit = 20, conversationId } = c.req.valid('query'); | |
| if (conversationId) { | |
| const conversation = await getConversation(runDbClient)({ | |
| scopes: { tenantId, projectId }, | |
| conversationId, | |
| }); | |
| if (!conversation || conversation.userId !== endUserId) { | |
| return c.json({ data: [], pagination: { page, limit, total: 0, pages: 0 } }); | |
| } | |
| } | |
| const result = await listLedgerArtifacts(runDbClient)({ | |
| scopes: { tenantId, projectId }, | |
| userId: endUserId, | |
| conversationId, | |
| pagination: { page, limit }, | |
| }); |
| artifacts: result.artifacts.map((artifact) => ({ | ||
| id: artifact.id, | ||
| taskId: artifact.taskId, | ||
| toolCallId: artifact.toolCallId, | ||
| contextId: artifact.contextId, | ||
| type: artifact.type, | ||
| name: artifact.name, | ||
| description: artifact.description, | ||
| summary: artifact.summary, | ||
| mime: artifact.mime as string[] | null, | ||
| visibility: artifact.visibility, | ||
| createdAt: toISODateString(artifact.createdAt), | ||
| updatedAt: toISODateString(artifact.updatedAt), | ||
| })), |
There was a problem hiding this comment.
Same manual field-picking concern as the run route. Consider destructuring out the excluded fields and spreading the rest to stay in sync with schema changes.
| artifacts: result.artifacts.map((artifact) => ({ | |
| id: artifact.id, | |
| taskId: artifact.taskId, | |
| toolCallId: artifact.toolCallId, | |
| contextId: artifact.contextId, | |
| type: artifact.type, | |
| name: artifact.name, | |
| description: artifact.description, | |
| summary: artifact.summary, | |
| mime: artifact.mime as string[] | null, | |
| visibility: artifact.visibility, | |
| createdAt: toISODateString(artifact.createdAt), | |
| updatedAt: toISODateString(artifact.updatedAt), | |
| })), | |
| artifacts: result.artifacts.map(({ tenantId, projectId, parts, metadata, allowedAgents, derivedFrom, ...rest }) => ({ | |
| ...rest, | |
| mime: rest.mime as string[] | null, | |
| createdAt: toISODateString(rest.createdAt), | |
| updatedAt: toISODateString(rest.updatedAt), | |
| })), |
| const ManageArtifactListItemSchema = z.object({ | ||
| id: z.string(), | ||
| taskId: z.string(), | ||
| toolCallId: z.string().nullable(), | ||
| contextId: z.string(), | ||
| type: z.string(), | ||
| name: z.string().nullable(), | ||
| description: z.string().nullable(), | ||
| summary: z.string().nullable(), | ||
| mime: z.array(z.string()).nullable(), | ||
| visibility: z.string().nullable(), | ||
| createdAt: z.string(), | ||
| updatedAt: z.string(), | ||
| }); |
There was a problem hiding this comment.
This schema is manually defined inline rather than derived from LedgerArtifactApiSelectSchema.omit(...) like the run route does. If ledgerArtifacts schema evolves (e.g. new nullable column), this won't pick it up. Consider deriving from the source schema for consistency:
const ManageArtifactListItemSchema = LedgerArtifactApiSelectSchema.omit({
parts: true,
metadata: true,
allowedAgents: true,
derivedFrom: true,
});![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(3) Total Issues | Risk: Low
🟡 Minor (2) 🟡
🟡 1) runtimeArtifacts.ts:40-52 Response shape follows pre-existing Manage domain pattern
Issue: The Manage API uses a nested response shape { data: { artifacts: [...], pagination: {...} }} with hasMore pagination, while the Run API uses the canonical flat shape { data: [...], pagination: { pages } } from ListResponseSchema.
Why: This creates inconsistent API contracts between Run and Manage domains for the same resource type. However, this pattern matches the existing manage/conversations.ts route (lines 109-121), so the new code is following an established (though non-canonical) pattern in the Manage domain.
Fix: Two options:
- Keep as-is - maintains consistency with existing Manage domain patterns
- Migrate to canonical - update both
runtimeArtifactsandconversationsroutes to useListResponseSchemawithPaginationSchema(includespagesfield instead ofhasMore)
Refs:
- Canonical ListResponseSchema
- Pre-existing pattern in manage/conversations.ts
- Run API using canonical pattern
Inline Comments:
- 🟡 Minor:
runtimeArtifacts.ts:128Path parameter naming inconsistency ({id}vs{artifactId})
💭 Consider (1) 💭
Inline Comments:
- 💭 Consider:
runtimeArtifacts.ts:25-38Schema derivation pattern — derive from base schema like Run API does
✅ APPROVE
Summary: This PR adds well-implemented artifact list and get-by-ID endpoints following established patterns in the codebase. Security review found proper tenant/user isolation with projectScopedWhere and conversation ownership checks. The minor consistency items noted follow existing Manage domain patterns — the author may choose to address them now or defer to a future consistency migration. Test coverage is comprehensive with proper security tests for cross-user access.
Discarded (5)
| Location | Issue | Reason Discarded |
|---|---|---|
| Tests | No test for combined conversationId + userId filters | Low confidence — edge case scenario, current AND logic handles this correctly |
| Tests | No test for artifact retrieval order | Low confidence — ordering is standard DB behavior, low risk of regression |
| Tests | No test for null userId in conversation | Medium confidence but current code correctly handles via !== endUserId check |
| Tests | No test for page beyond results | Low confidence — standard DB pagination handles gracefully |
manage/conversations.ts |
Pre-existing pattern inconsistency | Pre-existing issue not introduced by this PR |
Reviewers (4)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-consistency |
5 | 1 | 1 | 0 | 2 | 0 | 1 |
pr-review-tests |
4 | 0 | 0 | 0 | 0 | 0 | 4 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-security-iam |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 9 | 1 | 1 | 0 | 2 | 0 | 5 |
| createProtectedRoute({ | ||
| method: 'get', | ||
| path: '/{id}', | ||
| summary: 'Get Runtime Artifact', |
There was a problem hiding this comment.
🟡 Minor: Path parameter naming inconsistency
Issue: This route uses /{id} while the Run API equivalent uses /{artifactId}.
Why: Consistent parameter naming across domains makes the API easier to understand and SDK generation more predictable. The Run API at line 128 uses /{artifactId}.
Fix: Consider renaming to /{artifactId} and updating the params schema and handler to use artifactId instead of id:
path: '/{artifactId}',
// ...
request: {
params: TenantProjectParamsSchema.extend({ artifactId: z.string() }),
},
// in handler:
const { tenantId, projectId, artifactId } = c.req.valid('param');Refs:
| const ManageArtifactListItemSchema = z.object({ | ||
| id: z.string(), | ||
| taskId: z.string(), | ||
| toolCallId: z.string().nullable(), | ||
| contextId: z.string(), | ||
| type: z.string(), | ||
| name: z.string().nullable(), | ||
| description: z.string().nullable(), | ||
| summary: z.string().nullable(), | ||
| mime: z.array(z.string()).nullable(), | ||
| visibility: z.string().nullable(), | ||
| createdAt: z.string(), | ||
| updatedAt: z.string(), | ||
| }); |
There was a problem hiding this comment.
💭 Consider: Schema derivation pattern
Issue: This schema is manually defined with each field explicitly listed.
Why: The Run API derives ArtifactListItemSchema from LedgerArtifactApiSelectSchema.omit({...}). Deriving from the base schema reduces field drift risk if the base schema changes and ensures type consistency across domains.
Fix: Consider deriving from the base schema:
const ManageArtifactListItemSchema = LedgerArtifactApiSelectSchema.omit({
parts: true,
metadata: true,
allowedAgents: true,
derivedFrom: true,
tenantId: true,
projectId: true,
});Refs:
There was a problem hiding this comment.
PR Review Summary
(1) Total Issues | Risk: Medium
Scope: Delta review — 1 commit since prior automated review (
6d6a34369d21..e0e8b00cd534)
✅ Prior Feedback Addressed
The commit e0e8b00cd successfully addresses all feedback from the prior review:
- Schema derivation —
ManageArtifactListItemSchemanow derives fromLedgerArtifactApiSelectSchema.omit({...})instead of manually defining each field ✅ - Response mapping — Both routes now use destructuring with rest spread pattern (
{ parts, metadata, ...rest }) instead of manual field picking ✅ - Path parameter consistency — Changed from
/{id}to/{artifactId}to match the Run API pattern ✅ - conversationId ownership check — Added upfront ownership validation when
conversationIdis provided on the list endpoint (lines 89-97 ofrun/routes/artifacts.ts) ✅
🟠⚠️ Major (1) 🟠⚠️
Inline Comments:
- 🟠 Major:
PromptConfig.ts:12-23Unrelated change inlines templates as string constants
💭 Consider (1) 💭
💭 1) PromptConfig.ts Commit scope clarity
Issue: The commit message "updated according to review" implies only artifact API changes, but includes a significant unrelated refactoring of PromptConfig.ts that changes template imports to inline strings.
Why: Mixed commits make it harder to review, revert, or bisect issues. The PromptConfig change should ideally be a separate commit with its own rationale.
Fix: Consider splitting this into separate commits in future PRs, or at minimum updating the commit message to reflect the full scope of changes.
🚫 REQUEST CHANGES
Summary: The artifact API changes are well-implemented and address all prior review feedback. However, the delta includes an unrelated change that inlines template file contents as string constants in PromptConfig.ts while leaving the original template files in place. This creates code duplication and confusion about the source of truth. Please either revert the PromptConfig change (keeping template file imports) or delete the now-unused template files if this change is intentional — and clarify the rationale.
Discarded (0)
No findings were discarded.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator |
2 | 0 | 1 | 0 | 1 | 0 | 0 |
| Total | 2 | 0 | 1 | 0 | 1 | 0 | 0 |
Note: Delta-scoped review. No sub-agents dispatched — prior review coverage was comprehensive and delta changes were straightforward to validate directly.
| const dataComponentTemplate = | ||
| '<data-component>\n <name>{{COMPONENT_NAME}}</name>\n <description>{{COMPONENT_DESCRIPTION}}</description>\n <props>\n <schema>\n {{COMPONENT_PROPS_SCHEMA}}\n </schema>\n </props>\n</data-component> '; | ||
| const dataComponentsTemplate = | ||
| '<data_components_section description="These are the data components available for you to use in generating responses. Each component represents a single structured piece of information. You can create multiple instances of the same component type when needed.\n\n***MANDATORY JSON RESPONSE FORMAT - ABSOLUTELY CRITICAL***:\n- WHEN DATA COMPONENTS ARE AVAILABLE, YOU MUST RESPOND IN JSON FORMAT ONLY\n- DO NOT respond with plain text when data components are defined\n- YOUR RESPONSE MUST BE STRUCTURED JSON WITH dataComponents ARRAY\n- THIS IS NON-NEGOTIABLE - JSON FORMAT IS REQUIRED\n\nCRITICAL JSON FORMATTING RULES - MUST FOLLOW EXACTLY:\n1. Each data component must include id, name, and props fields\n2. The id and name should match the exact component definition\n3. The props field contains the actual component data using exact property names from the schema\n4. NEVER omit the id and name fields\n\nCORRECT: [{\\"id\\": \\"component1\\", \\"name\\": \\"Component1\\", \\"props\\": {\\"field1\\": \\"value1\\", \\"field2\\": \\"value2\\"}}, {\\"id\\": \\"component2\\", \\"name\\": \\"Component2\\", \\"props\\": {\\"field3\\": \\"value3\\"}}]\nWRONG: [{\\"field1\\": \\"value1\\", \\"field2\\": \\"value2\\"}, {\\"field3\\": \\"value3\\"}]\n\nAVAILABLE DATA COMPONENTS: {{DATA_COMPONENTS_LIST}}">\n\n{{DATA_COMPONENTS_XML}}\n\n</data_components_section>'; |
There was a problem hiding this comment.
🟠 MAJOR: Unrelated change inlines templates as string constants
Issue: This commit ("updated according to review") includes a significant change that replaces template file imports with inline string constants. The template files (templates/v1/prompt/system-prompt.xml, etc.) still exist in the repository but are now unused, creating code duplication and drift risk.
Why:
- The commit message suggests this is only addressing review feedback for the artifact API, but this change is unrelated to artifacts
- Template files still exist, creating confusion about the source of truth
- Inline strings are harder to maintain and review than separate template files
- This appears to circumvent Vite's
?rawimport syntax, possibly due to a build or test issue that should be addressed at the root cause
Fix: Either:
- Revert this change — keep using the template file imports and address any build/test issues separately
- If intentional — delete the now-unused template files and add a changeset explaining the architectural decision to inline templates
Refs:
|
Deployment failed with the following error: |
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Scope: Delta review — 1 commit since prior automated review (
e0e8b00cd534..810e192dc636)
✅ Prior Blocking Issue Resolved
The commit 810e192dc ("removed bad file change") successfully reverts the PromptConfig.ts change that was flagged as a MAJOR issue in the prior review:
| Issue | Resolution |
|---|---|
| PromptConfig.ts inlined templates as string constants, creating code duplication and confusion about source of truth | ✅ Reverted to using template file imports via ?raw syntax |
Before (problematic):
const systemPromptTemplate = '<system_message>...'; // 6 inline string constantsAfter (fixed):
import systemPromptTemplate from '../../../../../../templates/v1/prompt/system-prompt.xml?raw';✅ Artifact API Implementation
The core feature of this PR (artifact list and get-by-ID endpoints) was already reviewed and approved in prior reviews:
- DAL functions
listLedgerArtifactsandgetLedgerArtifact✅ - Manage domain routes with
requireProjectPermission('view')✅ - Run domain routes with JWT scoping and conversation ownership checks ✅
- Comprehensive test coverage (264 lines manage, 384 lines run) ✅
- Schema derivation pattern using
LedgerArtifactApiSelectSchema.omit({...})✅ - Response mapping with destructuring rest spread ✅
- Path parameter consistency (
/{artifactId}) ✅ - conversationId ownership validation on list endpoint ✅
✅ APPROVE
Summary: The blocking issue from the prior review has been resolved. The PromptConfig.ts file now correctly imports templates from files rather than inlining them as string constants. The artifact API implementation is well-structured with proper tenant/user isolation, comprehensive test coverage, and follows established patterns in the codebase. Ship it! 🚀
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator |
1 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 1 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: Delta-scoped review. The change was straightforward — a clean revert of the problematic PromptConfig.ts change. No sub-agents dispatched as prior review coverage was comprehensive.
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Scope: Delta review — 2 commits since prior automated review (
810e192dc636..7a86f168935b)
✅ Delta Contains No New Issues
The delta since the last review consists of:
| Commit | Description |
|---|---|
7a86f1689 |
Merge branch 'main' into feature/artifact-api |
68a55f5ce |
Fix false positive 'Needs Login' for connected MCP servers (#2931) |
Files changed in delta:
.changeset/urban-amaranth-lark.md— changeset for MCP tools fixagents-api/src/__tests__/manage/routes/crud/tools.test.ts— test updatespackages/agents-core/src/__tests__/data-access/dbResultToMcpTool.test.ts— new testspackages/agents-core/src/data-access/manage/tools.ts— MCP tools status fix
These changes are entirely from the merge bringing in PR #2931 — an unrelated bug fix for MCP server status detection. None of these changes affect the artifact API implementation.
✅ Artifact API Implementation (Previously Approved)
The core feature of this PR was comprehensively reviewed and approved in the prior review at 810e192dc:
- ✅ DAL functions
listLedgerArtifactsandgetLedgerArtifact - ✅ Manage domain routes with
requireProjectPermission('view') - ✅ Run domain routes with JWT scoping and conversation ownership checks
- ✅ Comprehensive test coverage (264 lines manage, 384 lines run)
- ✅ Schema derivation pattern using
LedgerArtifactApiSelectSchema.omit({...}) - ✅ Response mapping with destructuring rest spread
- ✅ Path parameter consistency (
/{artifactId}) - ✅ conversationId ownership validation on list endpoint
- ✅ PromptConfig.ts reverted to template file imports
✅ APPROVE
Summary: The delta contains only a merge from main bringing in an unrelated MCP tools fix (PR #2931). No changes to the artifact API feature. The prior review already approved the artifact API implementation with proper tenant/user isolation, comprehensive test coverage, and adherence to established patterns. Ship it! 🚀
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: Delta-scoped review. No sub-agents dispatched — the delta contains only merged changes from main that are unrelated to this PR's feature.
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Scope: Re-review at same commit — no changes since prior automated review (
7a86f168935b)
✅ Prior Approval Confirmed
This review was triggered on ready_for_review but the PR is at the same commit as the prior automated review which already approved this PR. Re-verification confirms:
| Area | Status |
|---|---|
DAL functions (listLedgerArtifacts, getLedgerArtifact) |
✅ Proper projectScopedWhere scoping |
| Manage domain routes | ✅ requireProjectPermission('view') enforced |
| Run domain routes | ✅ JWT scoping + conversation ownership checks |
| Schema derivation | ✅ LedgerArtifactApiSelectSchema.omit({...}) pattern |
| Response mapping | ✅ Destructuring rest spread pattern |
| Path parameters | ✅ Consistent /{artifactId} naming |
| conversationId filter | ✅ Ownership validation on list endpoint |
| Test coverage | ✅ 264 lines manage + 384 lines run |
| Changeset | ✅ Present for agents-core and agents-api |
✅ APPROVE
Summary: Well-implemented artifact API endpoints mirroring the existing conversations API pattern. Proper tenant/user isolation, comprehensive test coverage, and adherence to established codebase patterns. No new commits since prior approval — ready to merge.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: Re-review at same commit. Prior comprehensive review already approved this PR. Verified implementation remains solid.
There was a problem hiding this comment.
Solid addition that mirrors the existing conversations API pattern well. Authorization on the run endpoints is correctly implemented. Three issues worth addressing: a docs path mismatch that will break API reference linking, an inconsistency between manage and run response envelope shapes worth making intentional, and a redundant userId subquery when conversationId is already verified.
Claude Opus ｜ 𝕏
|
|
||
| {/* This file was generated by Fumadocs. Do not edit this file directly. Any changes should be made by running the generation command again. */} | ||
|
|
||
| <APIPage document={"index"} webhooks={[]} operations={[{"path":"/manage/tenants/{tenantId}/projects/{projectId}/artifacts","method":"get"},{"path":"/manage/tenants/{tenantId}/projects/{projectId}/artifacts/{id}","method":"get"},{"path":"/run/v1/artifacts","method":"get"},{"path":"/run/v1/artifacts/{artifactId}","method":"get"}]} showTitle={true} /> No newline at end of file |
There was a problem hiding this comment.
The manage detail route path references artifacts/{id} but the actual OpenAPI spec (and route definition at runtimeArtifacts.ts:119) uses artifacts/{artifactId}. This mismatch means the Fumadocs APIPage component won't find the operation and the "Get Runtime Artifact" section will be empty or broken.
If this file is auto-generated, re-run the generation script. If it was manually created, fix the path to artifacts/{artifactId}.
| userId: endUserId, | ||
| conversationId, | ||
| pagination: { page, limit }, | ||
| }); |
There was a problem hiding this comment.
When conversationId is provided, the handler already verifies ownership at lines 89–97 and short-circuits with an empty response if it doesn't belong to the user. Passing both userId and conversationId to the DAL means the query still runs a userId-based subquery against the conversations table — an extra join that is guaranteed to match the single already-verified conversation. Consider omitting userId from the DAL call when conversationId is present and already verified:
const result = await listLedgerArtifacts(runDbClient)({
scopes: { tenantId, projectId },
...(conversationId ? { conversationId } : { userId: endUserId }),
pagination: { page, limit },
});Not a correctness issue, but it simplifies the query and avoids the subquery.
| derivedFrom: true, | ||
| }); | ||
|
|
||
| const ManageArtifactListResponseSchema = z |
There was a problem hiding this comment.
The manage list endpoint wraps the response as { data: { artifacts: [...], pagination: {...} } } with hasMore in pagination, while the run list endpoint uses the ListResponseSchema pattern: { data: [...], pagination: { page, limit, total, pages } }. Both shapes are valid and match their respective conversations counterparts (manage conversations uses data.conversations, run conversations uses data[]), so this is intentional. Just flagging it explicitly — the two "Artifacts" tag operations will show different response shapes in the OpenAPI docs.
| conversationId, | ||
| }); | ||
| if (!conversation || conversation.userId !== endUserId) { | ||
| return c.json({ data: [], pagination: { page, limit, total: 0, pages: 0 } }); |
There was a problem hiding this comment.
Minor: when the conversation doesn't belong to the user, this returns a 200 with empty data. The analogous check in the detail endpoint (line 175) throws a 404. The list behavior is reasonable for filtering semantics, but worth noting the asymmetry is intentional — "list with bad filter = empty" vs. "get with bad ID = not found".
| ); | ||
| } | ||
|
|
||
| const whereClause = and(...whereConditions); |
There was a problem hiding this comment.
and(...whereConditions) is called when whereConditions might contain only the base project-scope condition (no optional filters). Drizzle's and() with a single argument works correctly, but if whereConditions were somehow empty (it can't be today since projectScopedWhere is always pushed), the spread would produce and() with zero args which returns undefined. The existing code is safe, just noting for defensiveness.
| const endUserId = requireEndUserId(executionContext); | ||
| const { artifactId } = c.req.valid('param'); | ||
|
|
||
| const artifact = await getLedgerArtifact(runDbClient)({ |
There was a problem hiding this comment.
In the GET /{artifactId} handler, getLedgerArtifact (line 158) fetches the artifact scoped only to { tenantId, projectId } — not by userId. Authorization is deferred to
the conversation ownership check 12 lines later.
This creates a two-step authorization pattern instead of a single atomic check:
- Fetch artifact (no user constraint)
- Fetch conversation, then verify ownership
If getLedgerArtifact ever returned an artifact where contextId is not a valid conversation ID (e.g., a direct artifact not tied to a conversation), getConversation would
return null, the !conversation branch would throw not_found — which is safe. But the pattern means correctness depends on the conversation check never being bypassed or
refactored away.
Recommendation: The current code is safe given the existing control flow, but it would be more robust to pass userId into getLedgerArtifact so the DB query itself
enforces ownership — consistent with how the list route works. That eliminates the two-step dependency and makes the single-artifact route's authorization
self-contained.
2 participants
Summary
/run/v1/artifacts) and admin (/manage/.../artifacts) API endpoints for listing and retrieving runtime artifacts, mirroring the existing conversations API patternlistLedgerArtifactsandgetLedgerArtifactDAL functions with pagination,userIdfiltering (via conversations subquery), andconversationIdfiltering/run/v1/artifactsas a lightweight run route that skips project config middleware, matching conversations behavior