fix(goals): preserve created_at on status transitions and edits#226
Open
khustup2 wants to merge 3 commits into
Open
fix(goals): preserve created_at on status transitions and edits#226khustup2 wants to merge 3 commits into
khustup2 wants to merge 3 commits into
Conversation
Every goal/KPI write went through an UPDATE that set created_at = now, clobbering the original creation timestamp. Because the listing and VFS bootstrap order by created_at DESC (and KPIs by created_at ASC), any status move or content edit silently reset the timestamp and bumped the row to the top — reshuffling the list on every transition. Add a dedicated updated_at column (auto-healed onto existing tables) and record edit time there, leaving created_at untouched. INSERT paths now populate both. Covers all write sites: VFS upsertGoalRow/upsertKpiRow, the goal/kpi CLI, and the openclaw write-side tools. Adds a regression test asserting a status transition writes updated_at and never touches created_at.
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR adds ChangesTimestamp Lifecycle Management
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Contributor
Coverage ReportScope: files changed in this PR. Enforced threshold: 90% per metric (per file via
File Coverage — 3 files changed
Generated for commit b48cb0e. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/commands/goal.ts (1)
188-193:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHeal the goals/KPIs schema before these update-only commands.
goal progress/doneandkpi bumpnow writeupdated_at, but unlike the add paths they never callensureGoalsTable()/ensureKpisTable(). On an upgraded workspace with preexisting tables, these UPDATEs can fail withupdated_at does not existuntil some other code path happens to run schema healing first.Suggested fix
async function goalProgress(goalId: string, status: string): Promise<void> { if (!VALID_STATUS.has(status)) { process.stderr.write(`invalid status: ${status} (expected opened|in_progress|closed)\n`); process.exit(1); } const cfg = loadConfig(); if (!cfg) { process.stderr.write("not logged in\n"); process.exit(1); } - const { query } = loadApiOrDie(cfg.goalsTableName); + const { api, query } = loadApiOrDie(cfg.goalsTableName); + await api.ensureGoalsTable(cfg.goalsTableName); const safe = sqlIdent(cfg.goalsTableName); const ts = new Date().toISOString(); await query( `UPDATE "${safe}" SET status = '${sqlStr(status)}', updated_at = '${sqlStr(ts)}' WHERE goal_id = '${sqlStr(goalId)}'` ); process.stdout.write(`${goalId} -> ${status}\n`); } @@ async function kpiBump(goalId: string, kpiId: string, deltaStr: string): Promise<void> { @@ const cfg = loadConfig(); if (!cfg) { process.stderr.write("not logged in\n"); process.exit(1); } - const { query } = loadApiOrDie(cfg.kpisTableName); + const { api, query } = loadApiOrDie(cfg.kpisTableName); + await api.ensureKpisTable(cfg.kpisTableName); const safe = sqlIdent(cfg.kpisTableName);Also applies to: 267-290
🤖 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/commands/goal.ts` around lines 188 - 193, Before running UPDATEs ensure the goals schema is healed by calling ensureGoalsTable() (or the equivalent schema-ensure helper) prior to loadApiOrDie/query usage; specifically, in the handlers that perform updates (the block using loadApiOrDie, sqlIdent/sqlStr, query that updates status/updated_at and the other block around lines 267-290), invoke ensureGoalsTable() and await its completion so the updated_at column exists on upgraded workspaces before executing the UPDATE statements.src/shell/deeplake-fs.ts (1)
489-517:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep
created_atandupdated_atindependent on structured INSERTs.Both insert branches compute one
tsfromlastUpdateDate ?? creationDateand write it into both columns. If a caller inserts a goal/KPI throughwriteFileWithMeta()with distinct timestamps,created_atis silently replaced bylastUpdateDate.Suggested fix
private async upsertGoalRow(r: PendingRow): Promise<void> { @@ - const ts = r.lastUpdateDate ?? r.creationDate ?? new Date().toISOString(); + const now = new Date().toISOString(); + const createdAt = r.creationDate ?? now; + const updatedAt = r.lastUpdateDate ?? createdAt; @@ - `updated_at = '${esc(ts)}' ` + + `updated_at = '${esc(updatedAt)}' ` + `WHERE goal_id = '${esc(parts.goal_id)}'` ); } else { const id = randomUUID(); await this.client.query( `INSERT INTO "${safe}" (id, goal_id, owner, status, content, version, created_at, updated_at, agent, plugin_version) VALUES (` + @@ - `'${esc(ts)}', ` + - `'${esc(ts)}', ` + + `'${esc(createdAt)}', ` + + `'${esc(updatedAt)}', ` + `'manual', ` + `''` + `)` ); } @@ private async upsertKpiRow(r: PendingRow): Promise<void> { @@ - const ts = r.lastUpdateDate ?? r.creationDate ?? new Date().toISOString(); + const now = new Date().toISOString(); + const createdAt = r.creationDate ?? now; + const updatedAt = r.lastUpdateDate ?? createdAt; @@ - `updated_at = '${esc(ts)}' ` + + `updated_at = '${esc(updatedAt)}' ` + `WHERE goal_id = '${esc(parts.goal_id)}' AND kpi_id = '${esc(parts.kpi_id)}'` ); } else { const id = randomUUID(); await this.client.query( `INSERT INTO "${safe}" (id, goal_id, kpi_id, content, version, created_at, updated_at, agent, plugin_version) VALUES (` + @@ - `'${esc(ts)}', ` + - `'${esc(ts)}', ` + + `'${esc(createdAt)}', ` + + `'${esc(updatedAt)}', ` + `'manual', ` + `''` + `)` ); }Also applies to: 536-561
🤖 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.
Outside diff comments:
In `@src/commands/goal.ts`:
- Around line 188-193: Before running UPDATEs ensure the goals schema is healed
by calling ensureGoalsTable() (or the equivalent schema-ensure helper) prior to
loadApiOrDie/query usage; specifically, in the handlers that perform updates
(the block using loadApiOrDie, sqlIdent/sqlStr, query that updates
status/updated_at and the other block around lines 267-290), invoke
ensureGoalsTable() and await its completion so the updated_at column exists on
upgraded workspaces before executing the UPDATE statements.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2be3b40d-2dbb-49f7-a9b0-bbd1d78d38ba
📒 Files selected for processing (7)
openclaw/src/index.tssrc/commands/goal.tssrc/deeplake-schema.tssrc/shell/deeplake-fs.tstests/claude-code/cli-goal.test.tstests/claude-code/deeplake-fs.test.tstests/openclaw/hivemind-tools.test.ts
goal progress/done and kpi bump issued an UPDATE writing updated_at but, unlike the add paths, never called ensureGoalsTable/ensureKpisTable first. On an upgraded workspace whose table predates updated_at, those commands could fail with 'column updated_at does not exist' until some other path healed the schema. Heal before the UPDATE, matching the add paths.
…INSERT upsertGoalRow/upsertKpiRow computed a single ts (lastUpdateDate ?? creationDate) and wrote it into both created_at and updated_at on INSERT. A caller passing distinct timestamps via writeFileWithMeta would have its created_at silently overwritten by lastUpdateDate. Compute createdAt (creationDate ?? now) and updatedAt (lastUpdateDate ?? createdAt) separately so a fresh row preserves its real creation time. Addresses CodeRabbit finding on PR #226.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Every goal/KPI write path issued an
UPDATE ... SET created_at = <now>, clobbering the original creation timestamp on every status move or content edit. Because the listing (hivemind goal list), the VFS bootstrap, and the SessionStart banner all order bycreated_at DESC(and KPIs bycreated_at ASC), moving a goalopened → closedsilently reset its timestamp and bumped it to the top of the list — reshuffling order on every transition and destroying the real creation time.This affected all write sites, not just VFS
mv:src/shell/deeplake-fs.ts—upsertGoalRow/upsertKpiRow(VFS file writes + goal-statusmv)src/commands/goal.ts—goal progress/done+kpi bumpopenclaw/src/index.ts—hivemind_goal_add/hivemind_kpi_addFix
updated_atcolumn toGOALS_COLUMNS/KPIS_COLUMNS. It's auto-healed onto existing tables viahealSchema(ALTER ... ADD COLUMN ... DEFAULT '').updated_at = <now>and leavecreated_atuntouched.The moving logic itself was otherwise correct — single-row UPDATE-or-INSERT keyed by
goal_id, cleanremoveFromTreeof the source path, no phantom left behind.Tests
tests/claude-code/deeplake-fs.test.ts: a status transition writesupdated_atand never touchescreated_at.cli-goal.test.ts/hivemind-tools.test.ts.Verification
Rebuilt bundles and smoke-tested the live CLI against the production goals table:
goal add → progress in_progress → doneconfirmedhealSchemasuccessfully addsupdated_atto the existing table and transitions work. (A briefopenedread immediately after the writes was Deeplake's documented read-after-write eventual-consistency lag — settled toclosedwithin ~3s.)Note
versionstill stays at1(the single-row UPDATE-or-INSERT model never bumps it) — orthogonal to this fix; left as-is.Summary by CodeRabbit