Spec: Queryable Artifacts & Conversations V0

[tim-inkeep]

Summary

Simplified spec for queryable artifacts and conversations, replacing the previous full-text-search approach with a dramatically simpler V0.

• List + get endpoints for artifacts and conversations with filters
• "Search" = list returning id, name/title, summary — no FTS indexes, no ranking
• Unified metadata generation module for both artifacts and conversations
• Conversation title + summary auto-generation (artifacts already have this)

What's in this spec

1. Schema Changes

• Add summary column to conversations table (nullable text)
• No changes to ledgerArtifacts (already has name, description, summary)

2. Unified Metadata Generation Module

Extract artifact metadata generation from AgentSession.processArtifact() (~500 lines) into a shared module following the compressor pattern:

BaseMetadataGenerator (abstract)
├── ArtifactMetadataGenerator   — extracted from AgentSession, zero behavior change
└── ConversationMetadataGenerator — NEW: generates title + summary

• Base class handles: model resolution chain, retry with backoff, telemetry, fire-and-forget pattern
• Artifact generator: same prompt, schema, uniqueness check, fallback — just extracted
• Conversation generator: triggered every assistant message, LLM decides whether to update via shouldUpdate boolean

3. API Endpoints

Domain	Endpoint	Status
Run	GET /v1/conversations	Updated: +agentId filter, +summary in response
Run	GET /v1/artifacts	New: list with conversationId, agentId filters
Run	GET /v1/artifacts/{id}	New: get single artifact
Manage	GET /projects/:projectId/conversations	Updated: +agentId filter, +summary
Manage	GET /projects/:projectId/artifacts	New: list with userId, agentId, conversationId filters
Manage	GET /projects/:projectId/artifacts/{id}	New: get single artifact

Authorization: Run API = end-user sees own only (scoped via JWT sub). Manage API = admin sees all, optional userId/agentId/conversationId filters.

4. DAL Changes

• listConversations: add agentId filter
• listLedgerArtifacts: new paginated list with userId/agentId via inner join to conversations
• getLedgerArtifactById: new single artifact retrieval

5. SDK Methods

• client.conversations.list() / .get()
• client.artifacts.list() / .get()

What's NOT in this spec (deferred)

• Full-text search indexes (tsvector, GIN)
• Search ranking (RRF, recency weighting)
• Platform tools for agent self-search
• MCP adapter for search tools
• Semantic/vector search (pgvector)
• Windowed message search within conversations

Phasing

1. Schema + DAL — summary column, agentId filter, artifact list/get DAL
2. Metadata generation module — extract + unify artifact/conversation metadata generation
3. API endpoints — artifact list/get on both domains, conversation list updates
4. SDK — client methods

🤖 Generated with Claude Code

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
agents-api	Ready	Preview, Comment	Apr 2, 2026 5:35pm
agents-docs	Ready	Preview, Comment	Apr 2, 2026 5:35pm
agents-manage-ui	Ready	Preview, Comment	Apr 2, 2026 5:35pm

[changeset-bot]

⚠️ No Changeset found

Latest commit: b213c07

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[pullfrog]

TL;DR — Adds a design spec for queryable artifacts and conversations (V0). This replaces the earlier full-text-search approach with a dramatically simpler model: list + get endpoints with basic filters, a unified metadata generation module, and auto-generated conversation titles and summaries.

Key changes

• Add V0 spec for queryable artifacts and conversations — Defines schema changes (summary column on conversations, new agentId index), a unified BaseMetadataGenerator extraction from AgentSession.processArtifact(), new artifact list/get endpoints on both Run and Manage domains, agentId filtering and summary on conversation list endpoints, a RuntimeClient SDK class, authorization model, and a four-phase implementation plan.

_{Summary ｜ 1 file ｜ 49 commits ｜ base: main ← feature/queryable-artifacts-conversations}

Unified metadata generation module via BaseMetadataGenerator

Before: Artifact metadata generation (~500 lines) lived as a private method processArtifact() tightly coupled inside AgentSession. Conversations had no auto-generated titles or summaries.
After: A shared BaseMetadataGenerator<TResult, TContext> abstract class handles model resolution, retry with backoff, telemetry, and shared prompt helpers (formatConversationHistory, truncateForModel). ArtifactMetadataGenerator extracts the existing logic with zero behavior change, and a new ConversationMetadataGenerator fires on every assistant message — gated by a shouldGenerate() hook that skips the LLM call entirely for conversations with < 3 user messages.

The base class follows the existing compressor pattern (BaseCompressor → specific implementations) and introduces a TContext generic so subclasses define their own context shapes. Prompts are fully owned by subclasses — the base only provides reusable helpers. Conversation metadata generation is fire-and-forget with error logging, never blocking the user response.

How does the LLM decide whether to update conversation metadata?

The schema uses nullable title and summary fields — null means "current value is fine," saving output tokens. The prompt provides the current title and summary alongside the last 10 messages, and instructs the model to return null for fields that still accurately capture the conversation's topic. This replaces the earlier shouldUpdate boolean approach with per-field granularity. The prompt explicitly biases toward stability — only updating when the conversation meaningfully shifts topic.

How is the cost kept low?

Three layers: (1) conversations with < 3 user messages skip the LLM call entirely and use a cheap deterministic fallback that always produces both title and summary, (2) 3+ message conversations use the summarizer model (cheapest available) with a ~500 token prompt biased toward returning null, (3) most LLM calls return null for both fields (no DB write needed).

How are pre-existing conversations handled?

A one-time backfill migration job (pnpm db:backfill-conversation-metadata) queries conversations where title IS NULL OR summary IS NULL, fetches first 3 messages for each, and applies the same deterministic fallback as saveFallback(). No LLM calls — deterministic only. The read-time title fallback is preserved as a safety net for any conversations the backfill hasn't reached yet.

SPEC.md

New artifact endpoints on Run and Manage domains

Before: No API endpoints existed to list or retrieve individual artifacts.
After: GET /v1/artifacts and GET /v1/artifacts/{id} on the Run domain (end-user, scoped via JWT sub); GET /projects/:projectId/artifacts and GET /projects/:projectId/artifacts/{id} on the Manage domain (admin, with optional userId/agentId/conversationId/toolName filters).

Artifact filtering by userId and agentId uses an always-present inner join from ledgerArtifacts.contextId → conversations.id (normalizes Drizzle return types via getTableColumns(ledgerArtifacts)). The toolName filter uses JSONB access (metadata->>'toolName'). The list response exposes description (not summary) since artifact descriptions are typically concise enough to be useful untruncated. getLedgerArtifactById handles the composite PK issue by returning the most recent row by createdAt, with ownership verified in a single query via optional userId join.

SPEC.md

Conversation list updates, SDK client, and phased rollout

Before: Conversation list endpoints had no agentId filter and no summary field. No SDK client existed for querying conversations or artifacts programmatically.
After: Both Run and Manage conversation list endpoints gain an optional agentId query param and return summary (nullable). A new RuntimeClient class in the SDK provides listConversations, getConversation, listArtifacts, and getArtifact methods following the EvaluationClient pattern.

The spec defines a four-phase implementation order: (1) schema + DAL + backfill, (2) metadata generation module extraction, (3) API endpoints, (4) SDK methods. A consistency model section documents the eventual-consistency semantics — every conversation has non-null title and summary after its first completed turn, with brief staleness windows during in-flight generation. Open questions cover composite PK disambiguation (OQ1, deferred to V1), internal message accessibility for agents (OQ2), and within-entity search strategy (OQ3, code mode vs FTS vs ILIKE).

SPEC.md

  ^{｜ View workflow run ｜ Triggered by Pullfrog ｜ Using Claude Opus ｜ 𝕏}

[claude]

PR Review Summary

(6) Total Issues | Risk: Medium

🟠⚠️ Major (4) 🟠⚠️

🟠 1) SPEC.md:347 Run API artifact get-by-id missing user ownership check

Issue: The spec for GET /v1/artifacts/{artifactId} states "Verify artifact's conversation belongs to end-user" but the proposed getLedgerArtifactById DAL function does NOT include a userId parameter or join to conversations. It only takes scopes (tenantId/projectId) and artifactId, leaving the authorization check unspecified.

Why: Without an explicit user ownership check at the DAL level, implementations could accidentally return artifacts from conversations the user does not own. This is a cross-user data exposure risk within the same project. The existing conversation get pattern shows the correct approach: fetch first, then verify ownership.

Fix: Either add userId param to getLedgerArtifactById with a join to conversations (defense-in-depth), OR explicitly document that route handlers MUST verify conversation.userId === endUserId after fetching.

Refs:

• Run routes conversations.ts:329 — existing ownership check pattern

🟠 2) SPEC.md:454 DAL conditional join could leak cross-user data

Issue: The listLedgerArtifacts DAL conditionally joins to conversations only when needsJoin is true (userId or agentId provided). For Run API, userId should ALWAYS come from JWT sub, but the spec doesn't mark it as required in the DAL signature, allowing accidental omission.

Why: If Run API implementation omits userId, artifacts from ALL users in the project would be returned. The DAL signature permits this — making it a latent security footgun.

Fix: Create separate listLedgerArtifactsForUser DAL requiring userId, OR add runtime assertion, OR document the requirement prominently.

Refs:

• Run routes conversations.ts:226-227 — correctly passes userId

🟠 3) SPEC.md:515 Artifact ID retrieval non-deterministic due to composite PK

Issue: getLedgerArtifactById uses .limit(1) without ordering when multiple artifacts share the same id across different taskIds. The "first match" is database-ordering-dependent, making results non-deterministic.

Why: API consumers could receive inconsistent results across calls. The risk section acknowledges this but doesn't specify V0 mitigation.

Fix: Add .orderBy(desc(ledgerArtifacts.createdAt)) to ensure deterministic results (most recent first). Document this behavior in acceptance criteria.

Refs:

• runtime-schema.ts:519 — 4-part composite PK

🟠 4) SPEC.md:521-535 SDK section underspecifies client architecture

Issue: The SDK proposes client.conversations.list() and client.artifacts.list() but: (1) no client object exists in current SDK, (2) artifacts namespace could collide with artifactComponent() builder, (3) unclear if SDK targets Run API or Manage API.

Why: SDK consumers need a clear mental model. The current SDK is builder-focused (agent(), project()), not HTTP-client focused. This introduces a new pattern without clarifying the architecture.

Fix: Name the client explicitly, consider runtimeArtifacts to avoid confusion with artifact component configuration, and specify which API domain(s) the SDK wraps.

Refs:

• agents-sdk/src/index.ts — existing SDK exports

Inline Comments:

• 🟠 Major: SPEC.md:347 Run API get-by-id ownership check
• 🟠 Major: SPEC.md:454 DAL conditional join security
• 🟠 Major: SPEC.md:515 Artifact ID non-deterministic retrieval
• 🟠 Major: SPEC.md:535 SDK architecture unclear

🟡 Minor (2) 🟡

🟡 1) SPEC.md:277 Fire-and-forget creates eventual consistency gap

Issue: The fire-and-forget metadata generation means list endpoints may return stale/null summary immediately after turn completion. The prompt also always generates title/summary even when shouldUpdate: false is expected.

Why: Downstream consumers (SDK, platform tools) may see stale data. Cost implications if most calls return shouldUpdate: false but still generate full output.

Fix: Document the consistency model in API specs. Consider two-phase prompt: decide first, generate if needed.

🟡 2) SPEC.md:332-337 contextId vs conversationId vocabulary mismatch

Issue: API returns conversationId but underlying schema uses contextId. Filter params and response fields use conversationId while internal code uses contextId.

Why: Developers debugging may encounter both terms. Consistent vocabulary reduces cognitive load.

Fix: Add note confirming the API layer maps contextId (database) → conversationId (API) for clarity.

Inline Comments:

• 🟡 Minor: SPEC.md:277 Fire-and-forget consistency gap

💭 Consider (2) 💭

💭 1) SPEC.md:67 Module directory naming

Issue: Spec proposes metadata/ directory but peer modules use more specific naming (compression/, artifacts/, context/).
Fix: Consider metadata-generation/ or document why the generic name is intentional.

💭 2) SPEC.md:384 Path parameter notation inconsistency

Issue: Spec mixes :projectId (Express-style) with {id} (OpenAPI-style).
Fix: Use consistent notation throughout.

🧹 While You're Here (1) 🧹

🧹 1) scope Pagination response shape inconsistency (pre-existing)

Issue: Run domain uses { page, limit, total, pages } but Manage domain uses { page, limit, total, hasMore }. The spec proposes following the Run pattern for new artifact endpoints, but this perpetuates the split.

Why: The canonical ListResponseSchema uses pages, but manage conversations uses hasMore. New endpoints should follow a single pattern.

Fix: Decide which pagination pattern is canonical and align existing endpoints.

Refs:

• ListResponseSchema
• Manage conversations pagination

---

💡 APPROVE WITH SUGGESTIONS

Summary: This is a well-structured spec with clear decisions, phasing, and acceptance criteria. The major findings are all addressable with spec clarifications rather than fundamental design changes. The security concerns (user ownership checks, DAL conditional joins) should be explicitly addressed in the spec to guide implementation. The SDK architecture needs more detail before implementation. Overall, good V0 scope that sets up well for future enhancements.

Discarded (12)

Location	Issue	Reason Discarded
SPEC.md:29-46	Schema migration safety	Assessed as safe — adding nullable column is standard Postgres operation
SPEC.md:308-314	Response schema addition	Backward compatible — adding nullable field is additive
SPEC.md:310	Query param addition	Backward compatible — optional param doesn't break existing clients
SPEC.md:357	Route path conflicts	No conflicts — /artifacts distinct from /artifact-components
SPEC.md:441	DAL function naming	Follows existing pattern correctly
SPEC.md:94	BaseMetadataGenerator vs BaseCompressor pattern	Intentional divergence — different purposes, spec could clarify wording
SPEC.md:186	Generator context includes db client	Acceptable asymmetry between artifact (service) and conversation (DAL) paths
SPEC.md:21	LLM cost on every turn	Risk acknowledged in spec, mitigations mentioned, monitoring via telemetry
SPEC.md:267	Fire-and-forget behavioral change	Not breaking — matches existing artifact pattern, fallback preserved
SPEC.md:527	SDK namespace additions	Additive, no collision with existing exports
SPEC.md:316	Run API missing userId param	Intentional — userId from JWT, not query param
SPEC.md:58	Compressor pattern reference	Accurate reference, no change needed

Reviewers (5)

Reviewer	Returned	Main Findings	Consider	While You're Here	Inline Comments	Pending Recs	Discarded
pr-review-product	5	1	0	0	1	0	3
pr-review-architecture	8	2	1	0	1	0	4
pr-review-security-iam	5	2	0	0	2	0	1
pr-review-breaking-changes	7	0	0	0	0	0	7
pr-review-consistency	8	1	1	1	0	0	5
Total	33	6	2	1	4	0	20

Note: Breaking changes review found no breaking changes — all additions are backward compatible. Consistency review identified pre-existing pagination inconsistency surfaced during this review.

[claude]

🟠 MAJOR: Run API artifact get-by-id missing explicit user ownership check

Issue: The spec for GET /v1/artifacts/{artifactId} states "Verify artifact's conversation belongs to end-user" but the proposed getLedgerArtifactById DAL function (Section 6.3) does NOT include a userId parameter or join to conversations. It only takes scopes (tenantId/projectId) and artifactId.

Why: Without an explicit user ownership check at the DAL level, the implementation could accidentally return artifacts from conversations the user does not own. This is a cross-user data exposure risk within the same project.

Fix: Update the spec to require one of:

1. Add userId param to getLedgerArtifactById and join to conversations table with userId filter (defense-in-depth), OR
2. Explicitly document that the route handler MUST fetch the artifact, then verify its conversation's userId === endUserId, returning 404 if not

Refs:

• Existing pattern in run/routes/conversations.ts — shows the correct approach: fetch first, then check conversation.userId !== endUserId

[claude]

🟠 MAJOR: Artifact ID retrieval non-deterministic due to composite PK

Issue: The spec acknowledges ledgerArtifacts has a 4-part composite PK (tenantId, projectId, id, taskId), and getLedgerArtifactById returns "first match by id + project scope." The proposed implementation uses .limit(1) without ordering, meaning which artifact is returned when multiple exist with the same id but different taskIds is undefined.

Why: API consumers calling GET /v1/artifacts/{artifactId} could receive inconsistent results. The "first match" depends on database query ordering, which is non-deterministic without an explicit ORDER BY.

Fix: Consider:

1. Specify ordering: .orderBy(desc(ledgerArtifacts.createdAt)) to ensure most recent artifact is returned
2. Include taskId in the artifact list response so clients can disambiguate
3. Document in acceptance criteria: "When multiple artifacts share an ID, the most recently created is returned"

Refs:

• ledgerArtifacts schema — shows the 4-part composite PK

[claude]

🟠 MAJOR: SDK section underspecifies client architecture

Issue: The SDK section proposes client.conversations.list() and client.artifacts.list() as "thin HTTP wrappers over the manage API endpoints." However:

1. No client object currently exists in the SDK — it exposes builder functions like agent(), project()
2. The namespace artifacts could collide with the existing artifactComponent() builder (which manages artifact type definitions, not runtime instances)
3. The spec says "manage API" but Run API also has artifact endpoints

Why: SDK consumers need a clear mental model. Introducing client.artifacts when artifactComponent() already exists for artifact schemas creates ambiguity.

Fix: Clarify:

1. Name the client explicitly (e.g., RuntimeDataClient or extend existing patterns)
2. Consider naming client.runtimeArtifacts.list() to avoid confusion with artifact component configuration
3. Document whether SDK targets Run API or Manage API (or both)

[claude]

🟠 MAJOR: Conditional join in DAL could leak cross-user data if userId omitted

Issue: The listLedgerArtifacts DAL conditionally joins to conversations only when needsJoin is true (userId or agentId provided). If neither filter is provided, artifacts from ALL users in the project would be returned. For Run API, userId should ALWAYS be provided from JWT sub, but the spec doesn't mark it as required.

Why: If a Run API implementation accidentally omits the userId parameter, cross-user data leakage occurs. The DAL signature allows this omission.

Fix: Either:

1. Create a separate listLedgerArtifactsForUser DAL function that requires userId and always joins
2. Add explicit documentation that Run API route handlers MUST always pass userId: endUserId
3. Add a runtime assertion in the DAL when called from Run API context

Refs:

• Existing conversation list pattern — correctly passes userId: endUserId

[claude]

🟡 Minor: Fire-and-forget creates eventual consistency gap — consider documenting

Issue: The spec proposes generating conversation metadata using fire-and-forget pattern. While this matches existing artifact metadata behavior, downstream consumers (SDK, platform tools, UI) making list/get calls immediately after a chat response may not see updated metadata.

Why: This is acceptable for display purposes but the consistency model should be documented. Additionally, the prompt always generates title/summary even when shouldUpdate: false is expected — consider a two-phase prompt to reduce cost.

Fix:

1. Add a note in Section 5.1/5.2 that summary fields may be null or stale immediately after turn completion
2. Consider restructuring the prompt to decide first, generate if needed (reduce token output in common case)
3. Track metadata generation costs separately in telemetry to monitor impact

[pullfrog]

Thorough spec — well-structured, phased cleanly, and grounded in real codebase patterns. A few issues surfaced during validation against the code.

High priority: The getLedgerArtifactById design has a real correctness risk due to the 4-part composite PK, and the BaseMetadataGenerator generic signature has an unresolved TContext type. Both are straightforward to fix but should be addressed before implementation begins.

Medium priority: Missing index on conversations.agentId will cause sequential scans on every agentId-filtered query. The ConversationMetadataGenerator prompt always asks the LLM to produce title+summary even when shouldUpdate will be false — wasteful output tokens. And the first-turn behavior needs clarification (no existing title → LLM always gets shouldUpdate: true?).

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using Claude Opus ｜ 𝕏}

[pullfrog]

Missing type parameter: BaseMetadataGenerator<TResult> declares TResult but the generate(context: TContext) method on line 113 and abstract methods on lines 126/129/130 reference TContext, which is never declared as a type parameter on the class. This should be BaseMetadataGenerator<TResult, TContext>, or TContext should be a concrete type defined elsewhere.

This is a spec-level typo but it would block the implementer — worth fixing before handing off.

[pullfrog]

getLedgerArtifactById can return ambiguous results. The ledgerArtifacts PK is (tenantId, projectId, id, taskId). This function filters only on (scopes + id), so it can match multiple rows with different taskId values. The spec acknowledges this in Risk #5 (line 660) but defers disambiguation to V1.

The existing getLedgerArtifacts DAL function has the same issue when filtering by artifactId without taskId — it returns all matches as an array.

Suggested resolution: either (a) return the most recently created artifact with that id (add orderBy(desc(createdAt)).limit(1)) and document this, or (b) require the caller to pass taskId as a disambiguator, which the API endpoint could accept as an optional query param. Option (a) is simpler and likely correct for V0 since task-level duplication is rare in practice.

[pullfrog]

No index on conversations.agentId. The conversations table has no index on agentId (confirmed — only a PK on (tenantId, projectId, id)). Both the listConversations DAL update and the listLedgerArtifacts join will filter on agentId, which means full table scans within the tenant+project scope.

Consider adding an index in the Phase 1 migration:

CREATE INDEX conversations_agent_id_idx ON conversations (tenant_id, project_id, agent_id);

A composite index with tenant+project prefix will serve both the direct filter and the join path efficiently.

[pullfrog]

shouldUpdate wastes output tokens. The LLM always generates title, summary, AND shouldUpdate. When shouldUpdate is false, the title and summary are discarded. This means every turn that doesn't need an update still pays for generating those strings.

Consider a two-phase approach: have the LLM return shouldUpdate first (or as a standalone boolean-only call), then conditionally generate title+summary. Alternatively, flip the schema — return title and summary as nullable, where null means "no update needed." This saves output tokens on the majority of calls where nothing changes.

[pullfrog]

First-turn behavior is underspecified. When existingTitle and existingSummary are both None, the prompt asks the LLM to "set shouldUpdate to false and return them unchanged" if the existing values still capture the conversation. But there are no existing values — None/None clearly doesn't "capture" anything.

The LLM will likely infer the right behavior, but it would be cleaner to add explicit guidance in the prompt: "If there is no current title or summary, always set shouldUpdate to true."

[pullfrog]

Drizzle type mismatch with conditional join. The let baseQuery = db.select().from(ledgerArtifacts) query returns LedgerArtifactSelect[]. After .innerJoin(conversations, ...), the return type changes to { ledger_artifacts: ..., conversations: ... }[]. Reassigning baseQuery with let won't compile in Drizzle because the types are structurally different.

The implementation will need to either: (a) always join and filter to avoid the conditional, or (b) use two separate query builders with a shared where-conditions array, or (c) use db.select({ ...getTableColumns(ledgerArtifacts) }) to normalize the output shape. Worth noting in the spec so the implementer doesn't hit this wall.

[pullfrog]

toolName extraction unclear. ArtifactListItem includes toolName: string | null described as "from metadata". The ledgerArtifacts table has a metadata jsonb column — is toolName a known key in this JSON? Or does it come from some other source?

The existing schema has toolCallId (varchar) on the table itself, but no toolName column. If toolName needs to be extracted from the jsonb metadata field, the spec should document the expected shape of that JSON, or note that a join to the tool definitions table might be needed.

[pullfrog]

Migration scope. The migration section only lists ALTER TABLE conversations ADD COLUMN summary text. Consider also including the conversations_agent_id_idx index (per the comment on line 420) in the same migration, since both are Phase 1 changes and rolling them into one migration is cleaner.

[pullfrog]

recentMessages: FormattedMessage[] // last 10 messages — is 10 the right window? For long conversations this is fine, but for the first 2-3 turns it means the LLM sees the full conversation anyway. Worth noting whether 10 is a hard cap or whether it should be min(messageCount, 10) (which it effectively is, but making it explicit avoids confusion during implementation).

[pullfrog]

SDK wraps manage API — is that intentional for client.conversations.list()? The spec says SDK methods are "thin HTTP wrappers over the manage API endpoints." But client.conversations.list() includes userId as a filter param, which the manage API supports. If the SDK is used by end-users (not just admins), they'd need the run API endpoints instead. Clarify the intended audience — if admin-only, this is fine; if end-user-facing, the SDK should wrap the run API.

[github-actions]

Preview URLs

Use these stable preview aliases for testing this PR:

• UI: https://pr-2961-ui.preview.inkeep.com
• API: https://pr-2961-api.preview.inkeep.com
• API health: https://pr-2961-api.preview.inkeep.com/health

These point to the same Vercel preview deployment as the bot comment, but they stay stable and easier to find.

Raw Vercel deployment URLs

• UI deployment: https://agents-manage-j7vejcoyj.preview.inkeep.com
• API deployment: https://agents-5f4t873w7.preview.inkeep.com

[claude]

PR Review Summary

(0) Total Issues | Risk: Low

This is a delta review scoped to changes since the prior automated review (commit 58dd9ced27f3).

Prior Feedback Status

All major and minor issues from the previous reviews have been addressed in this update:

Prior Issue	Status	Resolution
🟠 Run API get-by-id missing user ownership check	✅ Fixed	Added userId param to getLedgerArtifactById with join to conversations (line 644)
🟠 DAL conditional join security concern	✅ Fixed	Added explicit "Security note" documentation + changed to always-join approach (line 568-572)
🟠 Artifact ID non-deterministic due to composite PK	✅ Fixed	Added .orderBy(desc(ledgerArtifacts.createdAt)) (line 667) + documented in OQ1
🟠 SDK section underspecified	✅ Fixed	Renamed to RuntimeClient, follows EvaluationClient pattern, clear API targeting (line 678+)
🟡 Fire-and-forget consistency gap	✅ Fixed	Added Section 5.4 "Consistency Model" documenting expected behavior (line 528-534)
Missing TContext type parameter	✅ Fixed	Changed to BaseMetadataGenerator<TResult, TContext> (line 105)
Missing index on conversations.agentId	✅ Fixed	Added composite index (line 46-55)
shouldUpdate wastes output tokens	✅ Fixed	Changed schema to nullable title/summary (line 295-305)
First-turn behavior underspecified	✅ Fixed	Added explicit guidance in prompt (line 314)
Drizzle type mismatch with conditional join	✅ Fixed	Changed to always-join with implementation note (line 570-571)
toolName extraction unclear	✅ Fixed	Added comment explaining JSONB access (line 458)
Migration scope incomplete	✅ Fixed	Migration now includes both column and index (line 772)

Key Improvements in Delta

1. Cost optimization: Added shouldGenerate() gate — conversations with < 3 user messages skip the LLM call entirely and use cheap fallback (line 288-292). This addresses the majority of traffic.

2. Token efficiency: Changed from shouldUpdate: boolean + always-generate to nullable fields — LLM returns null when no update needed, saving output tokens (line 295-305).

3. Defense-in-depth security: DAL functions now support user ownership verification in a single query via optional userId param with join (line 644, 653-662).

4. Implementation guidance: Added Drizzle-specific notes about conditional joins and type normalization (line 570-571).

5. Vocabulary clarity: Added explicit mapping between API conversationId and DB contextId (line 572).

Review Assessment

The spec is now well-structured with clear decisions, phasing, acceptance criteria, and explicit risk mitigations. All security concerns have been addressed with defense-in-depth approaches. The implementation guidance is thorough enough to avoid common pitfalls.

---

✅ APPROVE

Summary: Excellent iteration on prior feedback. All major security and correctness concerns have been addressed. The cost optimization via shouldGenerate() gate is a thoughtful addition. The spec is ready for implementation.

Reviewers (1)

Reviewer	Returned	Main Findings	Consider	While You're Here	Inline Comments	Pending Recs	Discarded
orchestrator-delta	12	0	0	0	0	0	0
Total	12	0	0	0	0	0	0

Note: This delta review verified that all prior findings were addressed. No new issues identified.

[claude]

PR Review Summary

(0) Total Issues | Risk: Low

This is a delta review scoped to changes since the prior automated review (commit b9fbc277d620). The delta consists of 1 commit with +110/-56 line changes.

Delta Changes Reviewed

Change	Assessment
D5 reworded — "every conversation always has title and summary" guarantee	✅ Clarification, strengthens the contract
D8 added — Backfill decision locked	✅ Formalizes deferred capability
saveFallback() expanded — now generates both title AND summary	✅ Addresses "no null metadata" guarantee
Section 4.7 (Backfill Strategy) added	✅ Comprehensive, covers graceful degradation
OQ2, OQ3 added — internal messages, retrieval approach	✅ Explicitly captures deferred scope
Deferred table expanded — "Likely approach" column	✅ Useful implementation guidance
Consistency Model rewritten	✅ Much clearer guarantees
Backfill risk mitigation added	✅ Appropriate coverage

Prior Feedback Status

All issues from previous reviews remain addressed. This delta only adds clarifications and improvements — no regressions.

Review Assessment

The delta strengthens the spec without introducing new concerns:

1. "No null metadata" guarantee is now explicit and backed by the expanded saveFallback() implementation that always produces both title and summary.

2. Backfill strategy is sound: paginated, deterministic-only (no LLM cost), and preserves the read-time fallback as a safety net.

3. Open questions are well-scoped — deferring internal message visibility and retrieval approach to V1 is pragmatic for V0.

4. Consistency model now clearly documents the brief staleness window and backfill coverage.

---

✅ APPROVE

Summary: Clean delta that improves spec clarity without introducing new issues. The "every conversation always has metadata" guarantee is now well-specified with deterministic fallback for short conversations and LLM generation for longer ones. Ready for implementation.

Reviewers (1)

Reviewer	Returned	Main Findings	Consider	While You're Here	Inline Comments	Pending Recs	Discarded
orchestrator-delta	8	0	0	0	0	0	0
Total	8	0	0	0	0	0	0

Note: This delta review verified all changes are clarifications/improvements. No new issues identified.