Make guard optional and infer zero-markings in defineWorkflow#7
Open
joshuaisaact wants to merge 2 commits intomainfrom
Open
Make guard optional and infer zero-markings in defineWorkflow#7joshuaisaact wants to merge 2 commits intomainfrom
joshuaisaact wants to merge 2 commits intomainfrom
Conversation
- `guard` on WorkflowTransition is now optional (no need to write `guard: null`) - `initialMarking` accepts Partial<Marking<Place>>; unspecified places default to 0 - Updated all workflow definitions and tests to use the shorter form
📝 WalkthroughWalkthroughThis PR refactors workflow transition definitions by making the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/engine/test/definition-store.test.ts (1)
93-109:⚠️ Potential issue | 🟠 MajorThe omitted-guard serializer contract still isn't enforced here.
This case now uses a transition with no
guard, but it only checkstimeout.packages/engine/src/persistence/definition-store.tsstill builds serialized transitions withguard: t.guard ?? null, so aWorkflowDefinition -> serializeDefinition()path can keep emittingguard: nulleven though the new contract is “guard omitted when absent”. Please assert the missing field here and update the serializer accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/engine/test/definition-store.test.ts` around lines 93 - 109, The test currently only asserts timeout but not that absent guards are omitted; update the test to also expect no guard field (e.g., expect(serialized.transitions[0]!.guard).toBeUndefined() or that transitions[1].guard is undefined) and change the serializer in packages/engine/src/persistence/definition-store.ts (the serializeDefinition / transition serialization logic that currently does `guard: t.guard ?? null`) to omit the guard property when t.guard is undefined (only include guard when t.guard is defined) so serializeDefinition no longer emits `guard: null`.
🧹 Nitpick comments (2)
packages/engine/src/workflow.ts (1)
83-87: Please add a direct regression test for the zero-fill branch.This loop is the new behavior in the PR, but none of the updated engine tests explicitly assert that omitted places become
0innet.initialMarkingor after a serialize/define round-trip. A focused test would make this contract much harder to regress.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/engine/src/workflow.ts` around lines 83 - 87, Add a regression unit test that covers the new zero-fill branch: create a workflow/net definition that omits one or more places from initialMarking, call the API that produces/returns the constructed net (so the code path that runs the fullMarking loop executes), and assert that net.initialMarking contains explicit 0 entries for those omitted places; then perform the serialize/define (or define/serialize) round-trip and assert again that net.initialMarking still contains zeros for the previously-omitted places. Reference the fullMarking creation code path and assert on net.initialMarking before and after the serialize/define round-trip to prevent regressions.workflows/simple-agent/definition.ts (1)
36-36: Consider removing trailing blank lines in transition objects.The blank lines at lines 36, 48, 60, 72, and 84 appear to be artifacts from removing
guard: null. While harmless, removing them would tighten up the formatting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workflows/simple-agent/definition.ts` at line 36, Remove the stray trailing blank lines left inside the Transition object literals in the transitions array of the agent definition (they were left behind after removing guard: null); open each Transition object in the transitions array (Transition objects) and delete the blank line after the last property so the object closes immediately, then re-run your formatter to ensure consistent spacing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/server/test/http.test.ts`:
- Around line 358-362: The negative test fixture's request body omitted the node
"type" causing schema validation to fail earlier; restore type: "automatic" in
the JSON body for the test (the object with name: "bad", places: ["a"],
transitions: [{ name: "t", inputs: ["a"], outputs: ["unknown"] }],
initialMarking: { a: 1 }) so the request reaches the "unknown output place"
validation; update that body object in the test in
packages/server/test/http.test.ts accordingly.
---
Outside diff comments:
In `@packages/engine/test/definition-store.test.ts`:
- Around line 93-109: The test currently only asserts timeout but not that
absent guards are omitted; update the test to also expect no guard field (e.g.,
expect(serialized.transitions[0]!.guard).toBeUndefined() or that
transitions[1].guard is undefined) and change the serializer in
packages/engine/src/persistence/definition-store.ts (the serializeDefinition /
transition serialization logic that currently does `guard: t.guard ?? null`) to
omit the guard property when t.guard is undefined (only include guard when
t.guard is defined) so serializeDefinition no longer emits `guard: null`.
---
Nitpick comments:
In `@packages/engine/src/workflow.ts`:
- Around line 83-87: Add a regression unit test that covers the new zero-fill
branch: create a workflow/net definition that omits one or more places from
initialMarking, call the API that produces/returns the constructed net (so the
code path that runs the fullMarking loop executes), and assert that
net.initialMarking contains explicit 0 entries for those omitted places; then
perform the serialize/define (or define/serialize) round-trip and assert again
that net.initialMarking still contains zeros for the previously-omitted places.
Reference the fullMarking creation code path and assert on net.initialMarking
before and after the serialize/define round-trip to prevent regressions.
In `@workflows/simple-agent/definition.ts`:
- Line 36: Remove the stray trailing blank lines left inside the Transition
object literals in the transitions array of the agent definition (they were left
behind after removing guard: null); open each Transition object in the
transitions array (Transition objects) and delete the blank line after the last
property so the object closes immediately, then re-run your formatter to ensure
consistent spacing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ed110d9d-015b-4d65-b7f4-77298caef1a5
📒 Files selected for processing (26)
apps/viewer/src/editor/useEditorState.tsapps/viewer/src/hooks/useAnalysis.tscomparisons/openclaw-safety/src/scenarios/budget-escalation.tscomparisons/openclaw-safety/src/scenarios/message-gating.tscomparisons/openclaw-safety/src/scenarios/sandbox-isolation.tscomparisons/openclaw-safety/src/scenarios/tool-approval.tspackages/cli/test/analyse.test.tspackages/engine/src/persistence/definition-store.tspackages/engine/src/types.tspackages/engine/src/workflow.tspackages/engine/test/definition-store.test.tspackages/engine/test/engine.test.tspackages/engine/test/executor.test.tspackages/engine/test/nodes.test.tspackages/engine/test/scheduler.test.tspackages/engine/test/sqlite-adapter.test.tspackages/engine/test/timeout.test.tspackages/engine/test/types.test.tspackages/server/test/http.test.tspackages/server/test/runtime.test.tsworkflows/agent-benchmark/definition.tsworkflows/coffee/definition.tsworkflows/contract-approval/definition.tsworkflows/github-lookup/definition.tsworkflows/order-checkout/definition.tsworkflows/simple-agent/definition.ts
💤 Files with no reviewable changes (9)
- packages/engine/test/engine.test.ts
- apps/viewer/src/hooks/useAnalysis.ts
- packages/engine/test/scheduler.test.ts
- comparisons/openclaw-safety/src/scenarios/tool-approval.ts
- packages/engine/test/nodes.test.ts
- comparisons/openclaw-safety/src/scenarios/budget-escalation.ts
- comparisons/openclaw-safety/src/scenarios/message-gating.ts
- comparisons/openclaw-safety/src/scenarios/sandbox-isolation.ts
- packages/engine/test/timeout.test.ts
Comment on lines
358
to
362
| body: JSON.stringify({ | ||
| name: "bad", | ||
| places: ["a"], | ||
| transitions: [{ name: "t", inputs: ["a"], outputs: ["unknown"], guard: null }], | ||
| transitions: [{ name: "t", inputs: ["a"], outputs: ["unknown"] }], | ||
| initialMarking: { a: 1 }, |
There was a problem hiding this comment.
Keep this negative test focused on the output-place validation.
This fixture now drops type, so the request can start failing for a different schema error before it reaches the "unknown output place" branch. Re-adding type: "automatic" keeps the test pinned to the behavior it is naming.
Suggested change
body: JSON.stringify({
name: "bad",
places: ["a"],
- transitions: [{ name: "t", inputs: ["a"], outputs: ["unknown"] }],
+ transitions: [{ name: "t", type: "automatic", inputs: ["a"], outputs: ["unknown"] }],
initialMarking: { a: 1 },
initialContext: {},
terminalPlaces: [],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| body: JSON.stringify({ | |
| name: "bad", | |
| places: ["a"], | |
| transitions: [{ name: "t", inputs: ["a"], outputs: ["unknown"], guard: null }], | |
| transitions: [{ name: "t", inputs: ["a"], outputs: ["unknown"] }], | |
| initialMarking: { a: 1 }, | |
| body: JSON.stringify({ | |
| name: "bad", | |
| places: ["a"], | |
| transitions: [{ name: "t", type: "automatic", inputs: ["a"], outputs: ["unknown"] }], | |
| initialMarking: { a: 1 }, | |
| initialContext: {}, | |
| terminalPlaces: [], |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/server/test/http.test.ts` around lines 358 - 362, The negative test
fixture's request body omitted the node "type" causing schema validation to fail
earlier; restore type: "automatic" in the JSON body for the test (the object
with name: "bad", places: ["a"], transitions: [{ name: "t", inputs: ["a"],
outputs: ["unknown"] }], initialMarking: { a: 1 }) so the request reaches the
"unknown output place" validation; update that body object in the test in
packages/server/test/http.test.ts accordingly.
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
2 participants
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.
guardon WorkflowTransition is now optional (no need to writeguard: null)initialMarkingaccepts Partial<Marking>; unspecified places default to 0Summary by CodeRabbit
guardproperty optional on workflow transitions for cleaner definitions.initialMarkingto require only non-zero place token values, automatically defaulting unspecified places to zero.