feat: validate flow definitions and require defineFlow#219
Conversation
osolmaz
changed the title
osolmaz
changed the title
|
Final report for Implemented the follow-up flow authoring cleanup on top of the Key points:
Validation run on this branch:
Review / comments:
CI:
This PR is ready to merge from my side, but I have not merged it. |
osolmaz
changed the title
![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: 62de479334
ℹ️ 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".
| if (isDefinedFlow(candidate)) { | ||
| return candidate; |
There was a problem hiding this comment.
Switching findFlowDefinition to require the internal defineFlow brand means .cjs flows can no longer use the previously valid plain-object export path, but those modules still hit prepareFlowModuleImport rewriting of "acpx/flows" to a file:// URL. In CommonJS that produces require("file:///..."), which Node resolves as MODULE_NOT_FOUND, so a CJS flow cannot satisfy the new requirement and flow run fails. Please keep the new brand gate, but avoid URL rewriting for CJS/CTS (or rewrite to a filesystem path) so CommonJS flows remain loadable.
Useful? React with 👍 / 👎.
|
Landed via temp rebase onto main.\n\n- Gate: \
Checking formatting... All matched files use the correct format.
markdownlint-cli2 v0.22.0 (markdownlint v0.40.0)
Checking formatting... All matched files use the correct format.
Found 0 warnings and 0 errors.
ℹ tsdown v0.21.7 powered by rolldown v1.0.0-rc.12
vite v8.0.3 building client environment for production... ✓ built in 412ms
✔ resolveAgentCommand maps known agents to commands (0.497375ms) |
|
Landed via temp rebase onto
Thanks @osolmaz! |
|
Landed via temp rebase onto main.\n\n- Gate: \
Checking formatting... All matched files use the correct format.
markdownlint-cli2 v0.22.0 (markdownlint v0.40.0)
Checking formatting... All matched files use the correct format.
Found 0 warnings and 0 errors.
ℹ tsdown v0.21.7 powered by rolldown v1.0.0-rc.12
vite v8.0.3 building client environment for production... ✓ built in 779ms
✔ resolveAgentCommand maps known agents to commands (0.714125ms) |
1 participant
Summary
This PR tightens flow definition validation and standardizes flow authoring around the public
acpx/flowssurface.It does five things:
zod-backed shape validation for flow definitions and node helpers while keeping the existing public flow shape (name/startAt/nodes/edges)defineFlow(...)fromacpx/flowsinstead of arbitrary plain-object defaultssrc/flows...imports toacpx/flowspr-triageexample to the samedefineFlow(...)authoring pattern as the other flow examplesFlowRunnervalidation structural so programmatic callers can still pass copied/composed flow objectsWhy
The goal is to make one authoring path explicit for flow files without turning flows into a new DSL or changing the graph model.
The intended split is:
acpx/flowsand exportingdefineFlow(...)FlowDefinitionobjects for programmaticFlowRunneruseThat preserves the existing graph surface while making the file-based authoring story consistent and easier to document.
Validation
pnpm run check:docs✅pnpm run build:test && node --test --test-name-pattern='defineFlow allows staged assembly before full graph validation|validateFlowDefinition accepts structural copies of defined flows|repo flow modules use defineFlow\(\.\.\.\) and import from "acpx/flows"|integration: flow run requires defineFlow before permission gating|integration: flow run resolves "acpx/flows" imports for external flow files|integration: flow run supports staged defineFlow assembly in external modules' dist-test/test/flows.test.js dist-test/test/integration.test.js✅pnpm run test✅ (478passed,0failed)pnpm run check❌ local-only coverage gate on Node 24:82.26%line coverage vs configured83%; format, typecheck, lint, build, viewer, and all tests passedGitHub CI on the PR head is the source of truth for merge readiness.