fix(agents-core): remove scheduled triggers tables from manage db#2929
fix(agents-core): remove scheduled triggers tables from manage db#2929miles-kt-inkeep merged 4 commits intomainfrom
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
TL;DR — Removes the deprecated Key changes
Summary | 6 files | 4 commits | base: Deprecated manage-side scheduled trigger tables removed
The tables were migrated to the runtime database in a prior change. This cleans up the dead schema from the manage side so there is a single source of truth.
|
There was a problem hiding this comment.
Clean removal of the deprecated scheduled_triggers and scheduled_workflows tables from the manage schema (Doltgres). The migration drops tables in the correct FK order, the snapshot omits the removed tables, no stale references remain in the codebase, and the agentRelations closure is well-formed. The two one-off scripts are well-structured with dry-run defaults and proper Dolt branch iteration. No actionable issues found.
Claude Opus | 𝕏
|
@claude add a changeset |
|
Claude finished @miles-kt-inkeep's task in 56s —— View job Adding changeset
Added changeset |
aa7cafe to
3db7b39
Compare
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
This PR cleanly removes the deprecated scheduled_triggers and scheduled_workflows tables from the manage database (Doltgres). The schema changes are correct, the migration drops tables in the proper order (CASCADE handles FK dependencies), and the one-time cleanup scripts are well-structured with dry-run defaults and proper resource cleanup.
💭 Consider (5) 💭
These are valid observations but acceptable tradeoffs for one-time migration scripts. The developer can reasonably choose not to address them.
💭 1) 0016_complex_klaw.sql:1-2 Drop order could match script pattern
Issue: Migration drops scheduled_triggers (parent) before scheduled_workflows (child), while the companion script correctly drops child first.
Why: CASCADE makes this work, but consistency with drop-scheduled-trigger-tables.ts (lines 95-96) would be cleaner.
Fix: Optionally reorder to drop scheduled_workflows first.
Refs: drop-scheduled-trigger-tables.ts:95-96
💭 2) 0016_complex_klaw.sql:2 Missing trailing newline
Issue: SQL file ends without a newline character.
Why: POSIX convention; some tools expect files to end with newline.
Fix: Add trailing newline.
💭 3) delete-deprecated-scheduled-triggers.ts:88-90 Silent skip on zero-count branches
Issue: Branches with the table but 0 rows are silently skipped without log output.
Why: Operators can't distinguish processed-empty branches from errored branches.
Fix: Add: console.log(\Branch "${branchName}": 0 triggers (no action needed)`);`
Refs: delete-deprecated-scheduled-triggers.ts:88-90
💭 4) scripts/*.ts Empty catch blocks swallow checkout-to-main errors
Issue: Both scripts have empty catch blocks (lines 108-109 and 107-108) when checking out back to main.
Why: If this fails repeatedly, operators get no warning. Low risk since pool is destroyed at script end.
Fix: Log a warning: console.warn(\Warning: failed to checkout main...`, err);`
Refs: delete script:108 · drop script:107
💭 5) scripts/*.ts String interpolation for DOLT_CHECKOUT
Issue: Branch names are interpolated directly: SELECT DOLT_CHECKOUT('${branchName}').
Why: Defense-in-depth: parameterized queries are safer. However, branch names come from dolt_branches system table and are validated via BranchNameSchema at creation (^[a-zA-Z0-9\-_./]+$), making practical risk very low.
Fix: Optionally use parameterized query: connection.query('SELECT DOLT_CHECKOUT($1)', [branchName])
Refs: BranchNameSchema validation
💡 APPROVE WITH SUGGESTIONS
Summary: Clean schema cleanup with well-documented one-time scripts. The tables were successfully migrated to the runtime database in a prior change, and this PR properly removes the dead schema from the manage side. The two-phase approach (delete data, then drop tables across branches) correctly handles Doltgres FK CASCADE limitations during merges. Ship it! 🚀
Discarded (2)
| Location | Issue | Reason Discarded |
|---|---|---|
scripts/*.ts:96-102, 94-101 |
Partial failure state if DELETE/DROP succeeds but DOLT_COMMIT fails | Acceptable for one-time scripts — operator can check Dolt status manually and re-run. Script exits on error with clear message. |
scripts/*.ts |
SQL injection in commit message interpolation | Commit messages are hardcoded strings, not user input. |
Reviewers (4)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-breaking-changes |
2 | 0 | 2 | 0 | 0 | 0 | 0 |
pr-review-errors |
7 | 0 | 3 | 0 | 0 | 0 | 2 |
pr-review-appsec |
1 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 10 | 0 | 5 | 0 | 0 | 0 | 2 |
Note: appsec finding merged with errors findings (SQL injection). Two partial-failure findings discarded as acceptable for one-time scripts.
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
This is a delta review of commits aec413cf6 and b1e485096 since the prior review.
Delta Changes
The migration file 0016_complex_klaw.sql was updated to:
- Remove
CASCADE— Unnecessary since child table is dropped first - Reorder drops —
scheduled_workflows(child) beforescheduled_triggers(parent) — correct FK order - Fix statement-breakpoint — Moved to correct position between statements
- Add trailing newline — POSIX compliance
Prior Feedback Addressed
| Item | Status |
|---|---|
| 💭 Drop order could match script pattern | ✅ Fixed |
| 💭 Missing trailing newline | ✅ Fixed |
| 💭 Silent skip on zero-count branches | Unchanged (acceptable for one-time script) |
| 💭 Empty catch blocks | Unchanged (acceptable for one-time script) |
| 💭 String interpolation for DOLT_CHECKOUT | Unchanged (branch names validated via system table) |
The remaining 3 Consider items from the prior review are in the one-time scripts (not the migration file), and remain acceptable tradeoffs for single-use migration tooling.
✅ APPROVE
Summary: Clean fix that addresses prior feedback on the migration file. The DROP order now correctly matches the companion script (child before parent), CASCADE was removed (unnecessary), and the file now ends with a newline. The one-time cleanup scripts remain unchanged and are acceptable as-is. Ship it! 🚀
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| Total | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: No sub-reviewers dispatched — delta was a 4-line fix directly addressing prior feedback. Manual review confirmed changes are correct and minimal.
|
This PR removes internal database tables ( For adding a changeset, please use |
- Fix nested backtick rendering in constraint table (lines 38, 41) - Clarify version pinning: note docker-compose.dbs.yml uses :latest - Replace fictional status column example with real credentialScope from manage-schema.ts - Add hyperlink to PR #2929 precedent - Remove specific beta version number to reduce staleness Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

No description provided.