feat(workflow): add observability storage contracts - #151
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThe PR adds persisted workflow phases, token usage, provider sessions, and per-call activity. It introduces database migration 9, extends ChangesWorkflow observability
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant WorkflowLaunch
participant WorkflowStore
participant workflow_runs
participant workflow_agent_calls
participant workflow_agent_activity
WorkflowLaunch->>WorkflowStore: createRun with phase metadata
WorkflowStore->>workflow_runs: store phases_json
WorkflowStore->>workflow_agent_calls: update session and token usage
WorkflowStore->>workflow_agent_activity: append and list call activity
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
Waishnav
changed the title
Greptile SummaryThis PR adds persisted workflow phases, token usage, provider-session attachment, and per-call activity records, plus a fixture generator for exercising workflow TUI states. The persistence layer is implemented and tested directly, but the production provider execution path does not yet populate the newly introduced observability data.
Confidence Score: 4/5The PR should not merge until real provider executions populate the newly added session, usage, and activity observability records. The database and store support observability data, but the production agent execution path never calls the new persistence APIs, leaving the feature empty for actual workflows. Files Needing Attention: src/workflow-store.ts and the production provider execution path in src/workflow-api.ts
|
| Filename | Overview |
|---|---|
| src/workflow-store.ts | Adds observability persistence and mapping APIs, but production execution does not invoke them. |
| src/db/migrations.ts | Adds the workflow observability columns, activity table, and supporting index. |
| src/db/schema.ts | Models persisted phases, token usage, and agent activity in the Drizzle schema. |
| src/workflow-launch.ts | Persists validated workflow phase metadata when launching a run. |
| scripts/workflow-tui-fixture.ts | Seeds multiple workflow lifecycle states for manual TUI testing. |
| src/workflow-types.ts | Introduces public types for workflow token usage and agent activity records. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Workflow launch] --> B[Persist declared phases]
B --> C[Production agent execution]
C --> D[Start and complete agent call]
C -. missing integration .-> E[Attach provider session]
C -. missing integration .-> F[Persist token usage]
C -. missing integration .-> G[Append agent activity]
E --> H[(Workflow database)]
F --> H
G --> H
Reviews (1): Last reviewed commit: "test(db): expect workflow observability ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
scripts/workflow-tui-fixture.ts (2)
40-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider printing a usage error instead of throwing for an unknown
--statevalue.Line 45 calls
fail, which throws. Node prints a stack trace for user input errors. A stack trace hides the usage message. Print the message tostderrand set a non-zero exit code instead.♻️ Proposed change
function fail(message: string): never { - throw new Error(message); + console.error(message); + process.exit(1); }🤖 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 `@scripts/workflow-tui-fixture.ts` around lines 40 - 49, Replace the throwing fail call in the selectedFixtures state-validation branch with a user-facing usage error: print the unknown-state message to stderr and set a non-zero process exit code, ensuring invalid --state input exits without emitting a stack trace.
231-231: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBuild the fixture worktree path with
tmpdir()andjoin.Line 231 hardcodes
/tmp. The rest of the script usestmpdir()andjoinfromnode:path. The literal is not valid on Windows, and it is inconsistent with the file's own convention.♻️ Proposed change
- worktreePath: worktree ? `/tmp/devspace-fixture-worktree-${callIndex}` : undefined, + worktreePath: worktree + ? join(tmpdir(), `devspace-fixture-worktree-${callIndex}`) + : undefined,🤖 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 `@scripts/workflow-tui-fixture.ts` at line 231, Update the worktreePath construction in the fixture setup to use the existing tmpdir() and join utilities instead of the hardcoded /tmp prefix, while preserving the callIndex-based directory name and undefined behavior when no worktree is requested.src/workflow-store.ts (1)
245-257: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winValidate observability input before persistence.
Line 245 serializes
input.phaseswithout parsing it. Invalid runtime input then causesrowToRunto throw when it reads the run.Lines 1009-1116 write
usage.state, activitykind, and activitystatuswithout runtime validation.rowToAgentCallmaps every unsupported usage state to"partial".rowToAgentActivitythrows only when a later read occurs.Parse phase metadata before serialization. Reject unsupported enum values before each update or insert.
Proposed validation
createRun(input: CreateWorkflowRunInput): WorkflowRunRecord { const now = isoNow(); const argsJson = input.argsJson ?? "null"; - const phasesJson = JSON.stringify(input.phases ?? []); + const phases = z.array(workflowPhaseMetaSchema).parse(input.phases ?? []); + const phasesJson = JSON.stringify(phases); assertArgsSize(argsJson); ... - phases: input.phases ?? [], + phases,updateAgentUsage(...) { + if (usage.state !== "partial" && usage.state !== "final") { + throw new Error("Unknown workflow token usage state"); + } for (const value of [...]) {appendAgentActivity(input: AppendWorkflowAgentActivityInput) { + if (!["tool", "command", "file", "status"].includes(input.kind)) { + throw new Error(`Unknown workflow agent activity kind: ${input.kind}`); + } + if (!["running", "completed", "failed"].includes(input.status)) { + throw new Error(`Unknown workflow agent activity status: ${input.status}`); + }As per coding guidelines, “Represent important behavior through schemas, types, checks, or explicit tool results rather than hidden prompt conventions.”
Also applies to: 1009-1116, 1218-1218, 1264-1274
🤖 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/workflow-store.ts` around lines 245 - 257, Validate observability inputs before persistence: parse and validate input.phases before JSON.stringify in the workflow-run insert path, and reject unsupported usage.state, activity.kind, and activity.status values in the update/insert paths around the relevant usage and activity persistence methods. Reuse the existing schemas or enum validators so rowToRun, rowToAgentCall, and rowToAgentActivity receive only supported values, while preserving valid inputs and rejecting invalid runtime data before database writes.Source: Coding guidelines
🤖 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 `@scripts/workflow-tui-fixture.ts`:
- Around line 86-91: Update the fixture setup around the run configuration
containing resumedFromRunId so the replayed case first seeds a real prior run,
then references that created run’s actual ID instead of the hardcoded
wfr_previous_fixture placeholder. Preserve the existing inline behavior for
non-replayed fixtures and ensure the seeded parent is available before creating
the replayed run.
- Line 95: Update the fixture run setup around store.claimRun so TUI fixtures do
not persist the short-lived fixture CLI process.pid. Use a fixed sentinel PID
for these fixture runs, or ensure they are exclusively managed by a known
process-controlled reaper path, preserving the intended running state for
running, phased-running, replayed, and call-failed fixtures.
In `@src/workflow-launch.test.ts`:
- Line 17: Extend the tests in workflow-launch.test.ts beyond the existing
spawn:false persistence case by adding an integration scenario that invokes
launchWorkflowRun with spawn:true through the published package entry point,
covering the actual workflow __worker startup and restart behavior for a CLI or
MCP-host path. Ensure the test exercises the packaged npx devspace/MCP workflow
rather than only internal persistence.
---
Nitpick comments:
In `@scripts/workflow-tui-fixture.ts`:
- Around line 40-49: Replace the throwing fail call in the selectedFixtures
state-validation branch with a user-facing usage error: print the unknown-state
message to stderr and set a non-zero process exit code, ensuring invalid --state
input exits without emitting a stack trace.
- Line 231: Update the worktreePath construction in the fixture setup to use the
existing tmpdir() and join utilities instead of the hardcoded /tmp prefix, while
preserving the callIndex-based directory name and undefined behavior when no
worktree is requested.
In `@src/workflow-store.ts`:
- Around line 245-257: Validate observability inputs before persistence: parse
and validate input.phases before JSON.stringify in the workflow-run insert path,
and reject unsupported usage.state, activity.kind, and activity.status values in
the update/insert paths around the relevant usage and activity persistence
methods. Reuse the existing schemas or enum validators so rowToRun,
rowToAgentCall, and rowToAgentActivity receive only supported values, while
preserving valid inputs and rejecting invalid runtime data before database
writes.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2c20d1aa-52b3-4c4c-b1e8-efd3a9e850c0
📒 Files selected for processing (11)
package.jsonscripts/workflow-tui-fixture.tssrc/db/migrations.tssrc/db/schema.tssrc/oauth-store.test.tssrc/workflow-contracts.tssrc/workflow-launch.test.tssrc/workflow-launch.tssrc/workflow-store.test.tssrc/workflow-store.tssrc/workflow-types.ts
| source: name === "replayed" ? "resume" : "inline", | ||
| scriptPath: join(stateDir, "fixtures", `${name}.js`), | ||
| scriptHash, | ||
| workspaceRoot: workspace, | ||
| resumedFromRunId: name === "replayed" ? "wfr_previous_fixture" : undefined, | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether resumedFromRunId and replayedFromRunId are dereferenced anywhere.
set -euo pipefail
rg -n -C6 --type=ts 'resumedFromRunId|replayedFromRunId|replayedFromCallIndex' srcRepository: Waishnav/devspace
Length of output: 27201
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
fd -a 'workflow-tui-fixture|workflow-worker|workflow-view|workflow-store|workflow-cli|workflow-output' . | sed 's#^\./##'
echo
echo "== scripts/workflow-tui-fixture.ts relevant sections =="
wc -l scripts/workflow-tui-fixture.ts
sed -n '1,130p' scripts/workflow-tui-fixture.ts
sed -n '260,310p' scripts/workflow-tui-fixture.ts
echo
echo "== workflow-worker.ts replay handling =="
wc -l src/workflow-worker.ts
sed -n '55,90p' src/workflow-worker.ts
echo
echo "== all references for resumedFromRunId / replayedFromRunId =="
rg -n -C3 --type=ts '(resumedFromRunId|replayedFromRunId|createWorkflowReplay|listAgentCalls|getAgentRun|getRun|workflowAgentCalls)' src scripts | sed -n '1,220p'Repository: Waishnav/devspace
Length of output: 20088
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== createWorkflowReplay implementation =="
wc -l src/workflow-replay.ts
sed -n '1,120p' src/workflow-replay.ts
echo
echo "== listAgentCalls implementation =="
rg -n -C8 --type=ts 'listAgentCalls|getAgentCalls|agent_calls|workflow_agent_calls' src/workflow-store.ts
echo
echo "== createWorkflowReplay usages =="
rg -n -C5 --type=ts 'createWorkflowReplay' src scripts
echo
echo "== workflow-summary excerpt for resume path =="
sed -n '60,120p' src/workflow-summary.tsRepository: Waishnav/devspace
Length of output: 16661
Seed a real replay parent for the fixture.
replayedFromRunId: "wfr_previous_fixture" is only display and provenance metadata. The fixture should seed a real prior run and reference that id so it does not represent a missing run.
🤖 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 `@scripts/workflow-tui-fixture.ts` around lines 86 - 91, Update the fixture
setup around the run configuration containing resumedFromRunId so the replayed
case first seeds a real prior run, then references that created run’s actual ID
instead of the hardcoded wfr_previous_fixture placeholder. Preserve the existing
inline behavior for non-replayed fixtures and ensure the seeded parent is
available before creating the replayed run.
|
|
||
| if (name === "starting") return { name, stateDir, run }; | ||
|
|
||
| store.claimRun(run.id, process.pid); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find code that treats a persisted workflow run pid as a liveness or staleness signal.
set -euo pipefail
rg -n -C6 --type=ts '\bpid\b' src | rg -n -C6 'kill|process\.kill|heartbeat|stale|reap|alive|orphan' || echo 'no liveness usage found'Repository: Waishnav/devspace
Length of output: 16128
Do not claim running TUI fixtures with the fixture CLI pid.
claimRun(run.id, process.pid) persists the short-lived fixture CLI pid for the running, phased-running, replayed, and call-failed fixtures. WorkflowStore.reapStale() treats a stale heartbeat with no live pid as run_failed, so these fixtures can change state before use. Use a fixed sentinel pid for fixture runs, or confirm fixtures live only under a known process-controlled reaper/staleness path.
🤖 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 `@scripts/workflow-tui-fixture.ts` at line 95, Update the fixture run setup
around store.claimRun so TUI fixtures do not persist the short-lived fixture CLI
process.pid. Use a fixed sentinel PID for these fixture runs, or ensure they are
exclusively managed by a known process-controlled reaper path, preserving the
intended running state for running, phased-running, replayed, and call-failed
fixtures.
Source: Coding guidelines
| source: { | ||
| kind: "inline", | ||
| script: `export const meta = { name: 'launch-demo', description: 'd' }\nreturn 1\n`, | ||
| script: `export const meta = { name: 'launch-demo', description: 'd', phases: [{ title: 'Plan' }, { title: 'Build', detail: 'Implement it' }] }\nreturn 1\n`, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -a -t f '^(package\.json|workflow-launch\.ts|workflow-launch\.test\.ts|workflow-tui-fixture\.ts)$' .
ast-grep outline src/workflow-launch.ts --items all --match 'launchWorkflowRun|spawnWorkflowWorker'
rg -n -C 3 --glob 'package.json' '"bin"|"scripts"|workflow' .
rg -n -C 3 --type ts '\blaunchWorkflowRun\s*\(|\bspawnWorkflowWorker\s*\(' src scriptsRepository: Waishnav/devspace
Length of output: 6051
🏁 Script executed:
#!/bin/bash
.set -euo pipefail
echo '--- package.json ---'
cat -n package.json | sed -n '1,220p'
echo '--- src/workflow-launch.ts relevant lines ---'
cat -n src/workflow-launch.ts | sed -n '1,180p'
echo '--- src/workflow-launch.test.ts relevant lines ---'
cat -n src/workflow-launch.test.ts | sed -n '1,160p'
echo '--- src/workflow-cli.ts workflow launch call area ---'
cat -n src/workflow-cli.ts | sed -n '130,200p'
echo '--- src/workflow-worker.ts spawn implementation ---'
cat -n src/workflow-worker.ts | sed -n '150,210p'
echo '--- references to workflow worker commands and publish files ---'
rg -n --glob '!dist/**' --glob '!node_modules/**' '"workflow"|"__worker"|workflow-worker|package\.json|files' package.json src dist 2>/dev/null | head -n 200Repository: Waishnav/devspace
Length of output: 41436
Add a real workflow launch coverage path.
src/workflow-launch.test.ts uses spawn: false, so it only covers persistence through launchWorkflowRun. It does not cover the actual workflow __worker startup path, restart behavior, or packaged npx devspace/MCP-host workflows. Add an integration test that resolves with spawn: true for a CLI/MCP path, and run the covered path behind the published package entry point.
🤖 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/workflow-launch.test.ts` at line 17, Extend the tests in
workflow-launch.test.ts beyond the existing spawn:false persistence case by
adding an integration scenario that invokes launchWorkflowRun with spawn:true
through the published package entry point, covering the actual workflow __worker
startup and restart behavior for a CLI or MCP-host path. Ensure the test
exercises the packaged npx devspace/MCP workflow rather than only internal
persistence.
Source: Coding guidelines
1 participant
This layer adds the durable primitives used by workflow observability: declared phase metadata, provider session ids, partial and final token snapshots, and bounded normalized agent activity. The v9 SQLite migration remains backward compatible, replayed calls remain distinguishable, and focused storage and launch tests cover the contracts.\n\nThis is the first layer above #144.
Summary by CodeRabbit