feat(agents-core): add missing refs to runtime tables#2688
feat(agents-core): add missing refs to runtime tables#2688miles-kt-inkeep merged 7 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 1cd775e 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid, well-scoped change. The schema additions, cascade-delete logic, and ref propagation are all consistent with existing patterns. One substantive concern about silent ref resolution failure in the workflow step, and a couple of non-blocking observations below.
| @@ -287,6 +290,7 @@ export async function createInvocationIdempotentStep(params: { | |||
| projectId: params.projectId, | |||
| agentId: params.agentId, | |||
| scheduledTriggerId: params.scheduledTriggerId, | |||
| ref: resolvedRef ?? undefined, | |||
There was a problem hiding this comment.
The removed code in scheduledTriggers.ts threw an error when resolveRef returned null (Failed to resolve ref for project). Here the null is silently coerced to undefined and stored as null in the DB. The sibling step checkTriggerEnabledStep (line ~172) handles null ref by returning shouldContinue: false — consider adding a similar guard or at minimum logging a warning so failed ref resolution doesn't go unnoticed in scheduled workflows.
| ALTER TABLE "dataset_run" ADD COLUMN "ref" jsonb;--> statement-breakpoint | ||
| ALTER TABLE "evaluation_run" ADD COLUMN "ref" jsonb;--> statement-breakpoint | ||
| ALTER TABLE "scheduled_trigger_invocations" ADD COLUMN "ref" jsonb;--> statement-breakpoint | ||
| ALTER TABLE "trigger_invocations" ADD COLUMN "ref" jsonb; No newline at end of file |
There was a problem hiding this comment.
Nit: missing trailing newline.
| sql`${triggerInvocations.ref}->>'name' = ${fullBranchName}` | ||
| ) |
There was a problem hiding this comment.
Pre-existing gap (non-blocking): all cascade-delete functions filter on ref->>'name' but no table has an index on the ref jsonb column or an expression index on (ref->>'name'). As table sizes grow, these DELETEs will scan the full tenant+project partition. Worth a follow-up to add expression indexes.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(2) Total Issues | Risk: Medium
🟠⚠️ Major (1) 🟠⚠️
🟠 1) cascadeDelete.test.ts Missing test coverage for new cascade-delete tables
files:
packages/agents-core/src/__tests__/data-access/cascadeDelete.test.ts
Issue: The cascade-delete tests do not cover the 4 newly added tables (triggerInvocations, scheduledTriggerInvocations, datasetRun, evaluationRun). The test file imports and cleans up existing tables but does not include the 4 new tables. The cascadeDeleteByBranch, cascadeDeleteByProject, and cascadeDeleteByAgent tests only verify deletion counts for pre-existing tables, not the new triggerInvocationsDeleted, scheduledTriggerInvocationsDeleted, datasetRunsDeleted, or evaluationRunsDeleted fields.
Why: Without test coverage for the new cascade delete behavior, regressions in branch-based deletion of these tables would go undetected. A bug in the SQL query (e.g., wrong column reference in the JSON path like ref->>'name') could leave orphaned records when branches are deleted, causing data inconsistency. The cascade-delete logic is critical infrastructure that affects data integrity across the evaluation and trigger subsystems.
Fix: Add test coverage for the new tables in the existing test describe blocks:
// 1. Add imports for the new tables
import { triggerInvocations, scheduledTriggerInvocations, datasetRun, evaluationRun } from '../../db/runtime/runtime-schema';
// 2. Add cleanup in beforeEach
await db.delete(triggerInvocations);
await db.delete(scheduledTriggerInvocations);
await db.delete(datasetRun);
await db.delete(evaluationRun);
// 3. Create test data with branch refs
await db.insert(triggerInvocations).values({
tenantId, projectId, agentId, id: generateId(),
triggerId: 'trigger-1', ref: branch1Ref,
status: 'pending', requestPayload: {},
});
await db.insert(datasetRun).values({
tenantId, projectId, id: generateId(),
datasetId: 'dataset-1', ref: branch1Ref,
});
// ... similar for scheduledTriggerInvocations and evaluationRun
// 4. Add assertions
expect(result.triggerInvocationsDeleted).toBe(1);
expect(result.scheduledTriggerInvocationsDeleted).toBe(1);
expect(result.datasetRunsDeleted).toBe(1);
expect(result.evaluationRunsDeleted).toBe(1);Refs:
- cascadeDelete.test.ts — existing test file
- cascade-delete.ts:96-145 — new deletion logic that needs test coverage
🟡 Minor (1) 🟡
Inline Comments:
- 🟡 Minor:
scheduledTriggerSteps.ts:282-283Inconsistent null handling for ref resolution vs checkTriggerEnabledStep
💭 Consider (1) 💭
Inline Comments:
- 💭 Consider:
cascade-delete.ts:387-414Add clarifying comment for scope asymmetry between agent-scoped triggers and project-scoped evals
🚫 REQUEST CHANGES
Summary: This PR correctly adds ref columns to 4 runtime tables following established patterns. The schema changes, migration, and cascade-delete logic are well-implemented. However, the cascade-delete tests need to be updated to cover the new tables before merging. The test coverage gap is the primary blocker. The inline comments about null handling inconsistency and documentation are minor improvements to consider.
Discarded (4)
| Location | Issue | Reason Discarded |
|---|---|---|
0023_bumpy_vampiro.sql |
Migration safety | INFO-level only - migration is safe, additive, and follows best practices |
cascade-delete.ts:446-458 |
datasetRuns/evaluationRuns hardcoded to 0 in cascadeDeleteByAgent | Intentional design - these tables are project-scoped, not agent-scoped per schema |
runtime-schema.ts |
Type safety of nullable JSONB columns | Type design is correct and consistent with existing patterns |
0023_bumpy_vampiro.sql:4 |
Missing trailing newline | Consistent with drizzle-kit generation pattern in this codebase |
Reviewers (6)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-tests |
4 | 1 | 0 | 0 | 0 | 0 | 0 |
pr-review-precision |
2 | 0 | 0 | 0 | 2 | 0 | 0 |
pr-review-standards |
1 | 0 | 0 | 0 | 0 | 0 | 1 |
pr-review-consistency |
5 | 0 | 0 | 0 | 0 | 0 | 5 |
pr-review-breaking-changes |
1 | 0 | 0 | 0 | 0 | 0 | 1 |
pr-review-types |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 13 | 1 | 0 | 0 | 2 | 0 | 7 |
Note: pr-review-types found no issues - nullable JSONB typing is correct and consistent.
| const ref = getProjectScopedRef(params.tenantId, params.projectId, 'main'); | ||
| const resolvedRef = await resolveRef(manageDbClient)(ref); |
There was a problem hiding this comment.
🟡 Minor: Inconsistent null handling for ref resolution
Issue: When resolveRef returns null, this code silently continues with undefined, creating a record without branch tracking. However, checkTriggerEnabledStep (line 174) treats the same scenario as a hard failure: 'Failed to resolve ref for project, treating trigger as deleted'.
Why: Invocations created without refs cannot be cleaned up by the branch-scoped cascade delete logic that this PR enables. This creates potential orphan records.
Fix: Consider either:
- Failing early when
resolvedRefis null (consistent withcheckTriggerEnabledStep) - Adding a comment documenting that invocations without refs will require manual cleanup
Refs:
- checkTriggerEnabledStep:174 — treats null ref as fatal
| .returning(); | ||
| tasksDeleted = tasksResult.length; | ||
|
|
||
| // Delete trigger invocations for this agent on this branch | ||
| const triggerInvocationsResult = await db | ||
| .delete(triggerInvocations) | ||
| .where( | ||
| and( | ||
| eq(triggerInvocations.tenantId, scopes.tenantId), | ||
| eq(triggerInvocations.projectId, scopes.projectId), | ||
| eq(triggerInvocations.agentId, scopes.agentId), | ||
| sql`${triggerInvocations.ref}->>'name' = ${fullBranchName}` | ||
| ) | ||
| ) | ||
| .returning(); | ||
|
|
||
| // Delete scheduled trigger invocations for this agent on this branch | ||
| const scheduledTriggerInvocationsResult = await db | ||
| .delete(scheduledTriggerInvocations) | ||
| .where( | ||
| and( | ||
| eq(scheduledTriggerInvocations.tenantId, scopes.tenantId), | ||
| eq(scheduledTriggerInvocations.projectId, scopes.projectId), | ||
| eq(scheduledTriggerInvocations.agentId, scopes.agentId), | ||
| sql`${scheduledTriggerInvocations.ref}->>'name' = ${fullBranchName}` | ||
| ) | ||
| ) | ||
| .returning(); |
There was a problem hiding this comment.
💭 Consider: Add clarifying comment for scope asymmetry
Issue: The function deletes triggerInvocations and scheduledTriggerInvocations by agentId, but returns hardcoded 0 for datasetRunsDeleted and evaluationRunsDeleted. This is correct (those tables are project-scoped, not agent-scoped), but the asymmetry may confuse future maintainers.
Why: The behavior is intentional since datasetRun and evaluationRun are projectScoped in the schema, while trigger invocations are agentScoped. A comment would make this explicit.
Fix: Consider adding a brief comment:
// Delete trigger invocations for this agent on this branch
// Note: datasetRun and evaluationRun are project-scoped (not agent-scoped),
// so they're handled by cascadeDeleteByProject, not hereRefs:
- runtime-schema.ts:524-535 — datasetRun is projectScoped
- runtime-schema.ts:587-597 — evaluationRun is projectScoped
There was a problem hiding this comment.
PR Review Summary
(1) Total Issues | Risk: Low
🟡 Minor (1) 🟡
Inline Comments:
- 🟡 Minor:
scheduledTriggerSteps.ts:293Inconsistent null handling for ref resolution vs checkTriggerEnabledStep
💭 Consider (1) 💭
💭 1) cascadeDelete.test.ts Add explicit zero-count assertions in cascadeDeleteBySubAgent tests
Issue: The cascadeDeleteBySubAgent tests don't assert the expected zero counts for triggerInvocationsDeleted, scheduledTriggerInvocationsDeleted, datasetRunsDeleted, and evaluationRunsDeleted.
Why: While the implementation correctly returns 0 (since sub-agents don't own triggers or evaluation runs), explicitly asserting these values documents the intentional behavior and protects against accidental future changes.
Fix: After expect(result.tasksDeleted).toBe(1), add:
expect(result.triggerInvocationsDeleted).toBe(0);
expect(result.scheduledTriggerInvocationsDeleted).toBe(0);
expect(result.datasetRunsDeleted).toBe(0);
expect(result.evaluationRunsDeleted).toBe(0);Refs:
- cascadeDelete.test.ts:743 — existing test that could include this
🕐 Pending Recommendations (1)
- 🟡
scheduledTriggerSteps.ts:293Silent ref resolution (pullfrog) — invocations created without refs won't be cleaned up by branch cascade delete
💡 APPROVE WITH SUGGESTIONS
Summary: The test coverage gap identified in the prior review has been thoroughly addressed. The new tests comprehensively cover cascade-delete for all 4 new runtime tables across branch, project, and agent scopes. The implementation is well-structured and follows existing patterns.
The remaining minor issue about null ref handling is a pre-existing pattern inconsistency that surfaced during this PR — it's worth addressing but not a blocker. Nice work! 🎉
Discarded (2)
| Location | Issue | Reason Discarded |
|---|---|---|
cascadeDelete.test.ts |
No test for entities with ref: null |
Edge case outside PR scope — existing records predate this feature, and null refs are correctly excluded from branch-based deletion |
0023_bumpy_vampiro.sql:4 |
Missing trailing newline | Consistent with drizzle-kit generation pattern in this codebase |
Reviewers (5)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-tests |
2 | 0 | 1 | 0 | 0 | 0 | 1 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-breaking-changes |
1 | 0 | 0 | 0 | 0 | 0 | 1 |
pr-review-consistency |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-precision |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Orchestrator | - | 0 | 0 | 0 | 1 | 1 | 0 |
| Total | 3 | 0 | 1 | 0 | 1 | 1 | 2 |
Note: The inline comment addresses an issue identified by pullfrog in a prior review that remains unresolved in this delta.
| projectId: params.projectId, | ||
| agentId: params.agentId, | ||
| scheduledTriggerId: params.scheduledTriggerId, | ||
| ref: resolvedRef ?? undefined, |
There was a problem hiding this comment.
🟡 Minor: Inconsistent null handling for ref resolution
Issue: When resolvedRef is null, this code silently continues with undefined, creating a record without branch tracking. However, checkTriggerEnabledStep (line 174) treats the same scenario as a hard failure: 'Failed to resolve ref for project, treating trigger as deleted'.
Why: Invocations created without refs cannot be cleaned up by the branch-scoped cascade delete logic that this PR enables. This creates potential orphan records.
Fix: Consider either:
- Failing early when
resolvedRefis null (consistent withcheckTriggerEnabledStep) - Adding a warning log when creating invocations without refs
if (!resolvedRef) {
logger.warn(
{ tenantId: params.tenantId, projectId: params.projectId },
'Creating invocation without ref - will not be cleaned up by branch cascade delete'
);
}Refs:
- checkTriggerEnabledStep:174 — treats null ref as fatal
There was a problem hiding this comment.
PR Review Summary
(1) Total Issues | Risk: Low
🟠⚠️ Major (1) 🟠⚠️
Inline Comments:
- 🟠 Major:
scheduledTriggerSteps.ts:177Truncated log message introduced by copy-paste error
🕐 Pending Recommendations (2)
- 💭
cascadeDelete.test.ts(prior review) — Consider adding explicit zero-count assertions incascadeDeleteBySubAgenttests - 🟡
scheduledTriggerSteps.ts:293(pullfrog) — Silent ref resolution creates invocations that won't be cleaned up by branch cascade delete
🚫 REQUEST CHANGES
Summary: The delta addresses the prior feedback about silent ref resolution by adding a warning log — good addition! However, a copy-paste error introduced a truncated log message at line 177 that needs a quick fix. The inline comment has a 1-click suggestion to restore the original complete message.
Discarded (0)
No discarded items.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| Orchestrator | 1 | 0 | 0 | 0 | 1 | 2 | 0 |
| Total | 1 | 0 | 0 | 0 | 1 | 2 | 0 |
Note: Delta review — sub-agents not dispatched due to small scope (1 file, ~8 lines).
agents-api/src/domains/run/workflow/steps/scheduledTriggerSteps.ts
Outdated
Show resolved
Hide resolved
|
@claude add a changeset |
|
Claude finished @miles-kt-inkeep's task in 37s —— View job Adding changeset
Added changeset |
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
✅ Delta Changes Verified
The delta since the last review contains:
- Truncated log message fix — Line 177 now correctly reads
'Failed to resolve ref for project, treating trigger as deleted'instead of the truncated'Failed to resolve ref for project, run will not be'. ✅ - Changeset added — Proper versioning with patch bumps for
@inkeep/agents-apiand@inkeep/agents-core. ✅
🕐 Pending Recommendations (1)
- 💭
cascadeDelete.test.ts(prior review) — Consider adding explicit zero-count assertions incascadeDeleteBySubAgenttests fortriggerInvocationsDeleted,scheduledTriggerInvocationsDeleted,datasetRunsDeleted, andevaluationRunsDeleted
✅ APPROVE
Summary: The blocking issue from the previous review (truncated log message) has been fixed. The PR correctly adds ref columns to 4 runtime tables for branch tracking support, with comprehensive cascade-delete logic and thorough test coverage. Ship it! 🚀
Discarded (0)
No discarded items.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| Orchestrator | 0 | 0 | 0 | 0 | 0 | 1 | 0 |
| Total | 0 | 0 | 0 | 0 | 0 | 1 | 0 |
Note: Delta review — sub-agents not dispatched due to minimal scope (1 line change + changeset).
…s.ts Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
Co-authored-by: miles-kt-inkeep <miles-kt-inkeep@users.noreply.github.com>
miles-kt-inkeep
force-pushed
the
feat/add-runtime-refs
branch
from
8d1515c to
1cd775e
Compare
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
✅ Delta Changes Verified
The delta since the last approved review (929a5da72) contains 2 commits:
-
babe70f2btest ci trigger — CI test commit, no functional changes. -
1cd775e7enarrow ref schema — Addresses @amikofalvy's concern about broad types:- Before: Drizzle's
createSelectSchemainferred overly broadanyOftypes for JSONBrefcolumns (string | number | boolean | object | array | null) - After: Explicitly typed as
ResolvedRefSchema.nullable().optional()across 7 select schemas - OpenAPI: Correctly narrowed from broad
anyOfto$ref: "#/components/schemas/ResolvedRef"withallOf+nullable: true
- Before: Drizzle's
Schemas updated:
TriggerInvocationSelectSchemaScheduledTriggerInvocationSelectSchemaTaskSelectSchemaConversationSelectSchemaContextCacheSelectSchemaDatasetRunSelectSchemaEvaluationRunSelectSchema
🕐 Pending Recommendations (1)
- 💭
cascadeDelete.test.ts(prior review) — Consider adding explicit zero-count assertions incascadeDeleteBySubAgenttests fortriggerInvocationsDeleted,scheduledTriggerInvocationsDeleted,datasetRunsDeleted, andevaluationRunsDeleted
✅ APPROVE
Summary: The type narrowing fix correctly addresses the human reviewer's concern about overly broad types in the OpenAPI schema. The ref fields now properly reference the structured ResolvedRef schema ({type, name, hash}) instead of accepting arbitrary JSON values. This is a good improvement that strengthens the API contract. Ship it! 🚀
Discarded (0)
No discarded items.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| Orchestrator | 0 | 0 | 0 | 0 | 0 | 1 | 0 |
| Total | 0 | 0 | 0 | 0 | 0 | 1 | 0 |
Note: Delta review — sub-agents not dispatched due to minimal scope (type narrowing only, 2 files changed).
|
TL;DR — Adds a nullable Key changes
Summary ｜ 18 files ｜ 7 commits ｜ base:
|
Ito Test Report ❌16 test cases ran. 14 passed, 2 failed. ✅ Most routing, authorization, replay-protection, and XSS-safety checks behaved as expected. 🔍 Code-first verification confirms two real product defects: duplicate manual dispatch is possible on rapid Run Now actions, and branch deletion can fail with an internal error because branch removal uses non-force Dolt deletion. ✅ Passed (14)
❌ Failed (2)
Branch deletion cascades newly added runtime tables – Failed
Rapid double-click Run now should not create duplicate invocations – Failed
},
📋 View Recording |
2 participants
Feature:
Future Work: