feat: summarize verbose workflow JSON nodes - #2414
Conversation
Verbose workflow JSON returned raw event streams, forcing consumers to duplicate the CLI's lifecycle fold and leaving tied event ordering nondeterministic. Changes: - Reuse the shared node-summary fold for verbose JSON output - Add startedAt metadata and an explicit --events escape hatch - Stabilize workflow event ordering and document/test the contract Fixes #2359
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change adds durable ordering to workflow events and uses that ordering in event queries. Verbose JSON workflow commands now return derived node summaries by default, with ChangesWorkflow event ordering and CLI output
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant workflowStatusCommand
participant workflowGetCommand
participant workflowEvents
participant buildNodeSummaries
CLI->>workflowStatusCommand: pass verbose JSON and --events options
CLI->>workflowGetCommand: pass verbose JSON and --events options
workflowStatusCommand->>workflowEvents: fetch workflow events
workflowGetCommand->>workflowEvents: fetch workflow events
workflowStatusCommand->>buildNodeSummaries: derive nodes when raw events are not requested
workflowGetCommand->>buildNodeSummaries: derive nodes when raw events are not requested
buildNodeSummaries-->>workflowStatusCommand: return node summaries
buildNodeSummaries-->>workflowGetCommand: return node summaries
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Consolidated Review: PR #2359Date: 2026-08-03T14:24:00+03:00 Executive SummaryThe available code-review artifact finds the PR well structured: it reuses one node-summary fold for human-readable and JSON output, preserves raw events behind Overall Verdict: REQUEST_CHANGES Auto-fix Candidates: 1 HIGH issue can be auto-fixed; the HIGH issue needs a schema/data-access design decision before implementation.
Statistics
CRITICAL Issues (Must Fix)None identified in the available artifacts. HIGH Issues (Should Fix)Issue 1: UUID tie-breaker does not preserve event chronologySource Agent: code-review Problem:
Recommended Fix: // Persist an event-order key when inserting the event, allocating it atomically
// with the insert; use it for every ascending lifecycle read.
await query(
`INSERT INTO remote_agent_workflow_events
(id, workflow_run_id, event_order, event_type, step_index, step_name, data)
VALUES ($1, $2, $3, $4, $5, $6, $7)`,
[id, data.workflow_run_id, eventOrder, data.event_type /* ... */]
);
// ORDER BY created_at ASC, event_order ASCAdd a real-SQLite regression that creates lifecycle events through Why High: Chronological lifecycle ordering is central to the new compact node-status surface. A random UUID does not preserve insertion order, particularly for short nodes or concurrent writers. MEDIUM Issues (Options for User)Issue 1: A retried node remains terminal after its next start eventSource Agent: code-review Problem: On a subsequent Options:
Recommendation: Fix now. It is a localized change that matches the compact one-current-state contract. case 'node_started': {
startTimes.set(nodeId, new Date(event.created_at).getTime());
summaries.set(nodeId, {
nodeId,
state: 'running',
startedAt: event.created_at,
});
break;
}Add a test for LOW Issues (For Consideration)None identified in the available artifacts. Positive Observations
Suggested Follow-up Issues
Next Steps
Agent Artifacts
Metadata
|
Fixed: - preserve lifecycle event chronology when timestamps tie - reset retried node summaries to running state Tests added: - SQLite lifecycle ordering regression - retry node-summary regression Skipped: - none
⚡ Self-Fix Report (Aggressive)Status: COMPLETE
Fixes Applied (2 total)
View all fixes
Tests Added
Skipped (0)(none — all findings addressed) Suggested Follow-up Issues(none) Validation✅ Type check | ✅ Lint | ✅ Tests (all package suites passed) Self-fix by Archon · aggressive mode · fixes pushed to |
|
Verified the migration against real Postgres 18 — one claim in it is wrong. ```sql Reproduced on a clean postgres:18 container: ``` id | note | event_order Postgres backfills the identity column. The comment is correct for SQLite — plain `ADD COLUMN ... INTEGER` leaves NULL — and wrong for Postgres. So the two databases diverge on pre-existing data: SQLite rows take the legacy ID fallback, Postgres rows never do because they already have values. The comment asserts they behave identically. Worth either correcting the comment and confirming the read path is fine with backfilled values, or making the Postgres column plain `INTEGER` so both start NULL and the fallback is genuinely uniform. Second concern — not verified, worth measuring`ADD COLUMN ... GENERATED BY DEFAULT AS IDENTITY` forces a full table rewrite under `ACCESS EXCLUSIVE` in Postgres. `remote_agent_workflow_events` is the largest table in the schema — 13,163 rows on this install, unbounded on a busy one — and Archon auto-applies the schema on startup. That is a blocking migration at boot. I have not measured it; a plain `INTEGER` column would avoid the rewrite entirely and also fix the divergence above. What is right hereThe contract implementation matches #2365 exactly: `nodes` replaces `events`, `--events` restores raw rows, `startedAt` is present, first-appearance order is documented and tested, and `buildNodeSummaries` is shared so text and JSON cannot drift. The schema addition is also well-motivated rather than scope creep — SQLite timestamps have one-second precision, so `created_at` genuinely cannot order events within a second, and the ordering contract depends on it. Both schemas were updated and the bundle regenerated, which is the hand-maintained parity CLAUDE.md requires and is easy to miss. |
`ADD COLUMN ... GENERATED BY DEFAULT AS IDENTITY` rewrites the entire table under ACCESS EXCLUSIVE. Verified on postgres:18 — relfilenode changes on the identity form and does not on a plain column. remote_agent_workflow_events is the largest table in the schema and the schema auto-applies on startup, so this was a boot-time stall proportional to event history, with the table locked. Now a plain BIGINT column plus a sequence DEFAULT: ADD COLUMN with no default is metadata-only, and SET DEFAULT afterwards applies to future inserts only. It also makes the accompanying comment true. The identity form back-filled existing Postgres rows (1, 2, 3...) while SQLite left them NULL, so the two databases disagreed about which rows take the COALESCE(event_order, 0) fallback. Both now leave existing rows NULL. Verified against postgres:18: fresh apply clean, re-apply idempotent, and event_order auto-assigns 1, 2 on insert.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core/src/db/adapters/sqlite.ts (1)
438-459: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winScope the order-assignment trigger's MAX() query to the workflow run.
The trigger computes
MAX(event_order)across the entireremote_agent_workflow_eventstable on every insert, not scoped toNEW.workflow_run_id. This forces a scan whose cost grows with the total number of events ever recorded, on every single event write. The existing partial index is on(workflow_run_id, event_order), so scoping the subquery byworkflow_run_idlets SQLite use that index as a bounded range scan instead.♻️ Proposed fix to scope the trigger by workflow_run_id
this.db.run( `CREATE TRIGGER IF NOT EXISTS remote_agent_workflow_events_assign_order AFTER INSERT ON remote_agent_workflow_events WHEN NEW.event_order IS NULL BEGIN UPDATE remote_agent_workflow_events SET event_order = ( SELECT COALESCE(MAX(event_order), 0) + 1 FROM remote_agent_workflow_events + WHERE workflow_run_id = NEW.workflow_run_id ) WHERE rowid = NEW.rowid; END` );Apply the same change to the duplicate trigger definition in
createSchema()(Lines 751-761) so both paths stay consistent.🤖 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 `@packages/core/src/db/adapters/sqlite.ts` around lines 438 - 459, Update both remote_agent_workflow_events_assign_order trigger definitions in the migration block and createSchema() so the MAX(event_order) subquery filters rows to NEW.workflow_run_id. Preserve the existing COALESCE and increment behavior while ensuring both trigger paths use the workflow_run_id-scoped lookup.
🤖 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.
Inline comments:
In `@migrations/000_combined.sql`:
- Around line 524-532: Replace the identity-based event_order addition in the
remote_agent_workflow_events migration with a nullable INTEGER column for both
database dialects. Add a non-blocking, batched backfill that assigns unique
per-workflow event_order values to existing rows, then retain the partial unique
index and update the migration comment to reflect that existing rows are
populated rather than left NULL.
---
Nitpick comments:
In `@packages/core/src/db/adapters/sqlite.ts`:
- Around line 438-459: Update both remote_agent_workflow_events_assign_order
trigger definitions in the migration block and createSchema() so the
MAX(event_order) subquery filters rows to NEW.workflow_run_id. Preserve the
existing COALESCE and increment behavior while ensuring both trigger paths use
the workflow_run_id-scoped lookup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d07cd363-d694-4ddc-a774-79e34ac9ad76
⛔ Files ignored due to path filters (1)
packages/core/src/db/bundled-schema.generated.tsis excluded by!**/*.generated.*
📒 Files selected for processing (10)
migrations/000_combined.sqlpackages/cli/src/cli.tspackages/cli/src/commands/workflow.test.tspackages/cli/src/commands/workflow.tspackages/core/src/db/adapters/sqlite.tspackages/core/src/db/workflow-events.since.integration.test.tspackages/core/src/db/workflow-events.test.tspackages/core/src/db/workflow-events.tspackages/core/src/schemas/workflow-event.tspackages/docs-web/src/content/docs/reference/cli.md
| -- Lifecycle ordering (#2359 follow-up): timestamps can tie, especially on | ||
| -- SQLite, so a database-assigned order preserves the event stream chronology. | ||
| -- Existing rows remain NULL and retain the legacy ID fallback in read queries. | ||
| ALTER TABLE remote_agent_workflow_events | ||
| ADD COLUMN IF NOT EXISTS event_order INTEGER GENERATED BY DEFAULT AS IDENTITY; | ||
| CREATE UNIQUE INDEX IF NOT EXISTS idx_workflow_events_run_order | ||
| ON remote_agent_workflow_events(workflow_run_id, event_order) | ||
| WHERE event_order IS NOT NULL; | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does PostgreSQL ALTER TABLE ADD COLUMN GENERATED BY DEFAULT AS IDENTITY require an ACCESS EXCLUSIVE lock and rewrite the table on versions 11+?
💡 Result:
Yes, the ALTER TABLE ... ADD COLUMN ... GENERATED BY DEFAULT AS IDENTITY command requires an ACCESS EXCLUSIVE lock in PostgreSQL versions 11 and later [1][2]. While many ADD COLUMN operations in modern PostgreSQL (version 11+) are "instant" because they do not require a full table rewrite [2][3], the addition of an identity column is an exception [4]. Specifically, adding an identity column triggers a table rewrite [4]. This occurs because the operation must initialize the underlying implicit sequence and ensure the new column is correctly populated and constrained across all existing rows [5][4]. Because it performs a full table rewrite, this operation is generally not instant on large tables and will block all access (including reads) to the table for the duration of the process due to the ACCESS EXCLUSIVE lock [1][2].
Citations:
- 1: https://dev.to/mickelsamuel/which-alter-table-operations-lock-your-postgresql-table-1082
- 2: https://postgres.ai/docs/postgres-howtos/schema-design/ddl-operations/how-to-add-a-column
- 3: https://www.postgresql.org/docs/17/sql-altertable.html
- 4: https://www.postgresql.org/message-id/00e6eb5f5c793b8ef722252c7a519c9a%40oss.nttdata.com
- 5: https://www.postgresql.org/docs/18/ddl-identity-columns.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== migrations files =="
git ls-files 'migrations/**/*.sql' 'migrations/*.sql' | sed -n '1,80p'
echo
echo "== relevant migration sections =="
if [ -f migrations/000_combined.sql ]; then
sed -n '240,310p' migrations/000_combined.sql
echo "---- lines 505-540 ----"
sed -n '505,540p' migrations/000_combined.sql
fi
echo
echo "== db adapter references to event_order / SQLite identity-like handling =="
rg -n "event_order|GENERATED BY DEFAULT AS IDENTITY|remote_agent_workflow_events|workflow_events" . --glob '!node_modules' --glob '!dist' --glob '!build' | sed -n '1,200p'
echo
echo "== lock/rewrite docs from local installed postgres docs if available =="
if command -v psql >/dev/null 2>&1; then
psql -U postgres -h localhost -c "SELECT version();" 2>/dev/null || true
fi
if command -v pg_config >/dev/null 2>&1; then
pg_config --docdir || true
fi
python3 - <<'PY'
from pathlib import Path
for root in [Path('/usr/share/doc/postgresql*')]:
pass
PYRepository: coleam00/Archon
Length of output: 50371
🌐 Web query:
PostgreSQL 18 identity columns table rewrite when alter table add column GENERATED BY DEFAULT AS IDENTITY existing rows NULL documentation
💡 Result:
In PostgreSQL 18, adding a column defined with GENERATED BY DEFAULT AS IDENTITY to an existing table will cause the entire table and all its indexes to be rewritten [1][2]. According to the official PostgreSQL 18 documentation, adding a column with a volatile default (such as clock_timestamp), a stored generated column, an identity column, or a column with a domain data type that has constraints requires a complete table rewrite [1][2]. This is distinct from adding a column with a simple, non-volatile constant default value, which is a metadata-only operation that does not require a rewrite [1][2]. Because this operation involves a full table rewrite, it can be resource-intensive and time-consuming on large tables, as it requires the creation of a new table file and the copying of all existing data. Even though the identity column will effectively be NULL for existing rows until they are updated or rewritten, the structural change associated with identity columns currently forces this physical rewrite of the table [1][2].
Citations:
- 1: https://www.postgresql.org/docs/18/sql-altertable.html
- 2: https://www.postgresql.org/docs/current/sql-altertable.html
Use a nullable order column or batched backfill instead of adding an identity column.
PostgreSQL identity columns are non-null, and they can only be added with an ACCESS EXCLUSIVE rewrite. Update the migration comment at migrations/000_combined.sql:526 because existing rows do not stay NULL under PostgreSQL. Migrate both dialects to a nullable event_order with non-blocking assignment/backfill instead of GENERATED BY DEFAULT AS IDENTITY so existing tables do not block reads/writes during startup.
🧰 Tools
🪛 Squawk (2.61.0)
[warning] 528-528: Adding a generated column requires a table rewrite with an ACCESS EXCLUSIVE lock. In Postgres versions 11+, non-VOLATILE DEFAULTs can be added without a rewrite. Add the column as nullable, backfill existing rows, and add a trigger to update the column on write instead.
(adding-field-with-default)
🤖 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 `@migrations/000_combined.sql` around lines 524 - 532, Replace the
identity-based event_order addition in the remote_agent_workflow_events
migration with a nullable INTEGER column for both database dialects. Add a
non-blocking, batched backfill that assigns unique per-workflow event_order
values to existing rows, then retain the partial unique index and update the
migration comment to reflect that existing rows are populated rather than left
NULL.
Source: Linters/SAST tools
|
Fixed in `27627b3f`, and correcting my earlier comment — I overstated one part of it. What I got wrongI said the Postgres/SQLite divergence over back-filled rows was a functional problem. It is not. The read path is: ```sql `created_at` dominates, so `event_order` only ever breaks same-second ties. Whether legacy Postgres rows hold 1, 2, 3 or NULL does not change the result. That finding was cosmetic. What actually mattered
```
Now a plain Verified against a real postgres:18:
Also worth recordingWhile checking whether this column was necessary at all, I confirmed it is: The rest of the PR — the #2365 contract, the shared |
1 participant
Summary
startedAt, supports raw events through--events, and uses a deterministic event-order tie-breaker.UX Journey
Before
After
Architecture Diagram
Before
After
Connection inventory (list every module-to-module edge, mark changes):
--eventsto verbose JSON selection.Label Snapshot
risk: lowsize: Mcli,core,docs,testscli:workflow,core:workflow-eventsChange Metadata
featuremultiLinked Issue
Validation Evidence (required)
Commands and result summary:
bun run type-check bun run lint bun run format:check bun run test bun run build bun run validateAll commands passed. Focused CLI contract tests (202), core workflow-event unit tests (20), and SQLite ordering integration tests (5) also passed.
912aae447fd074736cd05b18682770d9record successful checks.Security Impact (required)
No)No)No)No)Yes, describe risk and mitigation: not applicable.Compatibility / Migration
Yes)No)No)Human Verification (required)
What was personally validated beyond CI:
statusandgetJSON expose ordered node summaries;--eventsreturns raw lifecycle rows; text and JSON share the same fold.Side Effects / Blast Radius (required)
status/get, workflow-event reads, CLI reference documentation.eventsin default verbose JSON must add--events.Rollback Plan (required)
da66b19c.--eventsremains a diagnostics-only opt-in.nodes, node ordering is unstable, or raw diagnostic events are unavailable through--events.Risks and Mitigations
--events.Fixes #2359
Summary by CodeRabbit
New Features
--eventsto verbose workflow status and get commands for viewing raw event records.Bug Fixes
Documentation