feat: add flow trace replay bundles for flow runs#181
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c589a3b3c7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - `session_bound` | ||
| - `artifact_written` |
There was a problem hiding this comment.
The required event list makes session_bound and artifact_written mandatory for every implementation, but non-ACP flows can complete without any session binding or artifact output (the runtime supports compute/checkpoint-only graphs in src/flows/types.ts). That forces producers to emit synthetic events or violate the spec, which will fragment replay tooling behavior. These event types should be explicitly conditional ("when relevant"), like the ACP/action-specific events below.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks. The implementation PR supersedes this doc-only draft and makes these event types conditional in runtime output; I am leaving this older doc review unresolved because it targets the earlier spec-only commit rather than the current diff.
|
|
||
| - `run_started` | ||
| - `run_completed` | ||
| - `run_failed` |
There was a problem hiding this comment.
This document declares the trace as the source of truth, but the minimum terminal events only cover run_completed and run_failed, with no required representation for timeout termination. Since run state has a distinct timed_out status (FlowRunState.status in src/flows/types.ts), replay consumers cannot reliably reconstruct that outcome from the required trace contract alone. Please require an explicit timeout terminal event (or require terminal payloads to include final status).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks. The implementation PR now emits explicit timeout outcomes in flow state and trace, so this earlier spec-only concern no longer reflects the current branch. I am keeping the historical comment for context but treating it as stale relative to the current head.
|
Final report for the current head Summary:
Validation run locally:
Review status:
CI/CD:
Non-blocking caveats:
This PR looks ready for human review / landing. |
osolmaz
changed the title
|
Final report for the current head Summary:
Validation run locally:
Review status:
CI/CD:
Non-blocking caveats:
This PR looks ready for human review / landing. |
|
Final report for the current head Summary:
Validation run locally:
Review status:
CI/CD:
Non-blocking caveats:
This PR still looks ready for human review / landing. |
1 participant
Summary
manifest.json,flow.json,trace.ndjson, projections, bundled session snapshots/events, and artifact receiptsValidation
pnpm run build:test && node --test dist-test/test/flows-store.test.js dist-test/test/flows.test.jspnpm run checkpnpm run check:docstmpdir=$(mktemp -d /tmp/acpx-trace-smoke-XXXXXX) && printf '{"text":"trace-smoke"}' > "$tmpdir/input.json" && node dist/cli.js --approve-all --cwd "$tmpdir" --format json flow run examples/flows/shell.flow.ts --input-file "$tmpdir/input.json"Notes
FlowRunStoreand adds a regression test