[rig-claude] Improve Claude dynamic-workflow compatibility for rig - #497
Merged
Merged
Conversation
…ort pattern Add a test in the rig/globals describe block that validates pipeline() and parallel() from rig/globals delegate to the active workflow context — the same as call() does. This is the missing coverage for the incremental flat-port migration pattern documented in 340-flat-workflow-port.md and claude-workflow-conversion.md. The flat-port style (import call/pipeline/parallel from "rig/globals" at the module top level) is the most common first step when porting a Claude dynamic workflow to rig. The test shows the complete trio working together, mirroring the Claude dynamic workflow globals (agent/pipeline/parallel). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Author
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
Author
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — one suggestion on test completeness.
📋 Key Themes & Highlights
Key Themes
- Out-of-context error coverage gap: The
call()ambient has a dedicated "throws outside a workflow run" test, butpipeline()andparallel()don't get the same treatment. Completing the trio would ensure regressions on those error paths are caught.
Positive Highlights
- ✅ Clean Arrange/Act/Assert structure — the test is easy to read and understand
- ✅ Mirrors the documented flat-port migration pattern faithfully
- ✅ Covers real return-value semantics (10, 20, 30 / 40, 50) rather than just checking no-throw
- ✅ PR description is thorough and links the change to the documented migration guide
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 25.1 AIC · ⌖ 5.8 AIC · ⊞ 6.3K
Comment /matt to run again
| pipelineResult: [10, 20, 30], | ||
| parallelResult: [40, 50], | ||
| }); | ||
| }); |
Contributor
Author
There was a problem hiding this comment.
[/tdd] Missing out-of-context throw tests for pipeline() and parallel() — the existing call() out-of-context test (line 467) establishes a pattern not mirrored here. Without equivalent guard tests, a regression that silently swallows the out-of-context error for ambientPipeline/ambientParallel would go undetected.
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
1 participant
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.
Compatibility gap addressed
The
rig/globalstest section coveredcall(),call.text(), andcall.workflow()but had no coverage forpipeline()andparallel()from"rig/globals". This left the flat-port migration pattern — the most common first step when porting a Claude dynamic workflow to rig — untested.A flat Claude dynamic workflow uses injected globals (
agent,pipeline,parallel) at the module top level. The rig equivalent imports all three from"rig/globals":Without a test covering the complete trio, there was no signal if the ambient
pipeline/paralleldelegation ever broke.Why this improves transfer from Claude dynamic workflows to rig
When someone reads
340-flat-workflow-port.mdorclaude-workflow-conversion.mdand learns the"rig/globals"migration path, they can now see the behavior is validated end-to-end —call,pipeline, andparallelall route through the active workflow context, matching the Claude dynamic workflow globals trio.Files changed
src/workflow.test.ts— added one test in therig/globalsdescribe block:"pipeline() and parallel() from rig/globals run inside the active context — flat Claude workflow port pattern". Also updated the import to includeambientPipelineandambientParallel.Validation
The two pre-existing failures in
launcher-default-engine.test.tsare unrelated to this change.Remaining intentional differences
None introduced. All existing intentional differences documented in
claude-workflow-conversion.md(failure holes, pipeline stage signature, budget units, no sandbox, no resume journal) are unchanged.