Fix/dry run#932
Draft
thomashebrard wants to merge 4 commits into
Draft
Conversation
D-plan-for-codex.md is the decided 8-point plan that drove the refactor (deletion of dry_run.py / dry_run_pipeline.py / dry_run_with_graph.py / dry_pipe_router.py, PipelexRunner DRY-routes-local, validator migration, validate_bundle dry-on-load restoration). A/B/C cover the upstream taxonomy, load-profile audit, and synthesis the plan was built on. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Greptile SummaryThis PR routes dry-run validation through the main runner path. The main changes are:
Confidence Score: 2/5This should not merge until the dry-run routing and validation regressions are fixed.
Focus on Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+89
to
+90
| if self.pipe_run_mode is PipeRunMode.DRY: | ||
| return PipeRun(pipe_router=PipeRouter(observer=ObserverNoOp())) |
There was a problem hiding this comment.
This branch builds a fresh local router with ObserverNoOp, so every dry run executed through PipelexRunner skips the observer configured during Pipelex.setup(). The normal direct path wires the router with the MultiObserver, which includes telemetry and any custom observer hooks. A hosted process that records validation, audit, or metrics events through observers will see live runs emit those hooks, while dry runs silently bypass them.
Prompt To Fix With AI
This is a comment left during a code review.
Path: pipelex/pipeline/runner.py
Line: 89-90
Comment:
**Dry runs skip observers**
This branch builds a fresh local router with `ObserverNoOp`, so every dry run executed through `PipelexRunner` skips the observer configured during `Pipelex.setup()`. The normal direct path wires the router with the `MultiObserver`, which includes telemetry and any custom observer hooks. A hosted process that records validation, audit, or metrics events through observers will see live runs emit those hooks, while dry runs silently bypass them.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+228
to
231
| # Only teardown library if it was successfully created and the runner owns it | ||
| if library_id_resolved is not None and not self.keep_library_loaded: | ||
| get_library_manager().teardown(library_id=library_id_resolved) | ||
| teardown_current_library() |
There was a problem hiding this comment.
When keep_library_loaded=True, this cleanup skips both tearing down the library and clearing the current-library context. pipeline_run_setup() has already called set_current_library(library_id), so helpers such as validate_bundle() leave the async context pointing at the temporary validation library after they return. Later code that calls get_current_library() can then read the just-validated bundle's library instead of the caller's previous library, causing cross-call contamination.
Prompt To Fix With AI
This is a comment left during a code review.
Path: pipelex/pipeline/runner.py
Line: 228-231
Comment:
**Library context leaks**
When `keep_library_loaded=True`, this cleanup skips both tearing down the library and clearing the current-library context. `pipeline_run_setup()` has already called `set_current_library(library_id)`, so helpers such as `validate_bundle()` leave the async context pointing at the temporary validation library after they return. Later code that calls `get_current_library()` can then read the just-validated bundle's library instead of the caller's previous library, causing cross-call contamination.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+117
to
+119
| result = await validate_bundle(mthds_file_path=bundle_path, library_dirs=library_dirs) | ||
| if not any(pipe_code in {the_pipe.pipe_ref, the_pipe.code} for the_pipe in result.pipes): | ||
| get_required_pipe(pipe_code=pipe_code) |
There was a problem hiding this comment.
validate_pipe_in_bundle() now calls validate_bundle(), and this PR changed validate_bundle() to dry-run every pipe in the bundle. That means a request to validate good_pipe inside a bundle fails if any unrelated pipe in the same bundle is broken, even though the command contract is to validate only the selected pipe. This breaks partial validation workflows for bundles that contain experimental or currently broken pipes.
Prompt To Fix With AI
This is a comment left during a code review.
Path: pipelex/builder/operations/validate_ops.py
Line: 117-119
Comment:
**Single-pipe validation widens**
`validate_pipe_in_bundle()` now calls `validate_bundle()`, and this PR changed `validate_bundle()` to dry-run every pipe in the bundle. That means a request to validate `good_pipe` inside a bundle fails if any unrelated pipe in the same bundle is broken, even though the command contract is to validate only the selected pipe. This breaks partial validation workflows for bundles that contain experimental or currently broken pipes.
How can I resolve this? If you propose a fix, please make it concise.![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 24 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".notes/dry-run-refactor/A-taxonomy.md">
<violation number="1" location=".notes/dry-run-refactor/A-taxonomy.md:56">
P2: The document claims `DryPipeRouter` exists at `pipelex/pipe_run/dry_pipe_router.py:9` and describes it as dead code, but the file doesn't exist in the codebase. Update the document to reflect the current state: either remove the `DryPipeRouter` references or update them to note that the file/class has already been removed.</violation>
</file>
<file name="pipelex/cli/agent_cli/commands/validate/_validate_core.py">
<violation number="1" location="pipelex/cli/agent_cli/commands/validate/_validate_core.py:102">
P2: When the pipe is missing from the bundle, the fallback `get_required_pipe` can still resolve a library pipe and incorrectly return success. Raise immediately on failed membership instead of doing a global lookup.</violation>
</file>
<file name="pipelex/builder/operations/validate_ops.py">
<violation number="1" location="pipelex/builder/operations/validate_ops.py:117">
P1: This call widens single-pipe validation into full-bundle dry-run, so validating one pipe can fail due to unrelated broken pipes in the same bundle.</violation>
<violation number="2" location="pipelex/builder/operations/validate_ops.py:119">
P2: `validate_pipe_in_bundle` can incorrectly succeed for a pipe that is not in the target bundle because the fallback lookup searches the whole loaded library.</violation>
</file>
<file name="pipelex/pipeline/runner.py">
<violation number="1" location="pipelex/pipeline/runner.py:172">
P1: PipeRun selection uses constructor mode instead of the resolved per-job run mode, so env/default-resolved DRY runs can still be dispatched through `get_pipe_run()`.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| keep_library_loaded=self.keep_library_loaded, | ||
| ) | ||
| effective_pipe_run = self._pipe_run or get_pipe_run() | ||
| effective_pipe_run = self._resolve_pipe_run() |
There was a problem hiding this comment.
P1: PipeRun selection uses constructor mode instead of the resolved per-job run mode, so env/default-resolved DRY runs can still be dispatched through get_pipe_run().
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pipelex/pipeline/runner.py, line 172:
<comment>PipeRun selection uses constructor mode instead of the resolved per-job run mode, so env/default-resolved DRY runs can still be dispatched through `get_pipe_run()`.</comment>
<file context>
@@ -148,8 +167,9 @@ async def execute_pipeline(
+ keep_library_loaded=self.keep_library_loaded,
)
- effective_pipe_run = self._pipe_run or get_pipe_run()
+ effective_pipe_run = self._resolve_pipe_run()
pipe_output = await effective_pipe_run.run(pipe_job, delivery_assignment=delivery_assignment)
except PipeRouterError as exc:
</file context>
Suggested change
| effective_pipe_run = self._resolve_pipe_run() | |
| effective_pipe_run = self._pipe_run | |
| if effective_pipe_run is None: | |
| if pipe_job.pipe_run_params.run_mode is PipeRunMode.DRY: | |
| effective_pipe_run = PipeRun(pipe_router=PipeRouter(observer=ObserverNoOp())) | |
| else: | |
| effective_pipe_run = get_pipe_run() |
| # Now get the specific pipe and dry-run only that one | ||
| the_pipe = get_required_pipe(pipe_code=pipe_code) | ||
| dry_run_output = await dry_run_pipe(the_pipe, raise_on_failure=True) | ||
| result = await validate_bundle(mthds_file_path=bundle_path, library_dirs=library_dirs) |
There was a problem hiding this comment.
P1: This call widens single-pipe validation into full-bundle dry-run, so validating one pipe can fail due to unrelated broken pipes in the same bundle.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pipelex/builder/operations/validate_ops.py, line 117:
<comment>This call widens single-pipe validation into full-bundle dry-run, so validating one pipe can fail due to unrelated broken pipes in the same bundle.</comment>
<file context>
@@ -163,30 +111,16 @@ async def validate_pipe_in_bundle(
- # Now get the specific pipe and dry-run only that one
- the_pipe = get_required_pipe(pipe_code=pipe_code)
- dry_run_output = await dry_run_pipe(the_pipe, raise_on_failure=True)
+ result = await validate_bundle(mthds_file_path=bundle_path, library_dirs=library_dirs)
+ if not any(pipe_code in {the_pipe.pipe_ref, the_pipe.code} for the_pipe in result.pipes):
+ get_required_pipe(pipe_code=pipe_code)
</file context>
|
|
||
| --- | ||
|
|
||
| ### DryPipeRouter (`pipelex/pipe_run/dry_pipe_router.py:9`) |
There was a problem hiding this comment.
P2: The document claims DryPipeRouter exists at pipelex/pipe_run/dry_pipe_router.py:9 and describes it as dead code, but the file doesn't exist in the codebase. Update the document to reflect the current state: either remove the DryPipeRouter references or update them to note that the file/class has already been removed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .notes/dry-run-refactor/A-taxonomy.md, line 56:
<comment>The document claims `DryPipeRouter` exists at `pipelex/pipe_run/dry_pipe_router.py:9` and describes it as dead code, but the file doesn't exist in the codebase. Update the document to reflect the current state: either remove the `DryPipeRouter` references or update them to note that the file/class has already been removed.</comment>
<file context>
@@ -0,0 +1,350 @@
+
+---
+
+### DryPipeRouter (`pipelex/pipe_run/dry_pipe_router.py:9`)
+**What it does:** Dead code; calls `pipe.dry_run_pipe()` at line 18. No production usage.
+
</file context>
| result = await validate_bundle(mthds_file_path=bundle_path, library_dirs=library_dirs) | ||
| if not any(pipe_code in {the_pipe.pipe_ref, the_pipe.code} for the_pipe in result.pipes): | ||
| # Use the runtime lookup to surface the same error path as before | ||
| get_required_pipe(pipe_code=pipe_code) |
There was a problem hiding this comment.
P2: When the pipe is missing from the bundle, the fallback get_required_pipe can still resolve a library pipe and incorrectly return success. Raise immediately on failed membership instead of doing a global lookup.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pipelex/cli/agent_cli/commands/validate/_validate_core.py, line 102:
<comment>When the pipe is missing from the bundle, the fallback `get_required_pipe` can still resolve a library pipe and incorrectly return success. Raise immediately on failed membership instead of doing a global lookup.</comment>
<file context>
@@ -127,27 +93,13 @@ async def validate_pipe_in_bundle_core(
+ result = await validate_bundle(mthds_file_path=bundle_path, library_dirs=library_dirs)
+ if not any(pipe_code in {the_pipe.pipe_ref, the_pipe.code} for the_pipe in result.pipes):
+ # Use the runtime lookup to surface the same error path as before
+ get_required_pipe(pipe_code=pipe_code)
return {
</file context>
| dry_run_output = await dry_run_pipe(the_pipe, raise_on_failure=True) | ||
| result = await validate_bundle(mthds_file_path=bundle_path, library_dirs=library_dirs) | ||
| if not any(pipe_code in {the_pipe.pipe_ref, the_pipe.code} for the_pipe in result.pipes): | ||
| get_required_pipe(pipe_code=pipe_code) |
There was a problem hiding this comment.
P2: validate_pipe_in_bundle can incorrectly succeed for a pipe that is not in the target bundle because the fallback lookup searches the whole loaded library.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pipelex/builder/operations/validate_ops.py, line 119:
<comment>`validate_pipe_in_bundle` can incorrectly succeed for a pipe that is not in the target bundle because the fallback lookup searches the whole loaded library.</comment>
<file context>
@@ -163,30 +111,16 @@ async def validate_pipe_in_bundle(
- dry_run_output = await dry_run_pipe(the_pipe, raise_on_failure=True)
+ result = await validate_bundle(mthds_file_path=bundle_path, library_dirs=library_dirs)
+ if not any(pipe_code in {the_pipe.pipe_ref, the_pipe.code} for the_pipe in result.pipes):
+ get_required_pipe(pipe_code=pipe_code)
return {
</file context>
Suggested change
| get_required_pipe(pipe_code=pipe_code) | |
| raise ValueError(f"Pipe '{pipe_code}' is not part of bundle '{bundle_path}'.") |
…ion + validation)
Completes the .notes/dry-run-refactor/ bundle so a reader landing on the PR
has end-to-end context without needing the original session.
- README.md index, read order, TL;DR
- E-parity-gate.md API/worker library preload parity verification that
gated "DRY -> always local" routing
- F-implementation.md file-by-file map of the diff, the keep_library_loaded
flag added beyond the plan, behavioral changes worth
flagging in review (DryRunStatus.SKIPPED gone,
ValidateBundleResult schema change, etc.)
- G-validation.md commands run, results, known flakes, pre-merge checklist
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
Summary by cubic
Routes DRY mode through the standard runner/orchestrator and deletes legacy dry-run paths. Restores dry-on-load validation in
validate_bundle, updates CLI, builder, and graph rendering to the new API, and adds akeep_library_loadedoption. Developer notes documenting the refactor were added under.notes/dry-run-refactor/*.Refactors
pipelex/pipe_run/{dry_run.py,dry_run_pipeline.py,dry_run_with_graph.py,dry_pipe_router.py}; DRY now usesPipelexRunner+PipeRunMode.DRY.dry_run_loaded_pipes_or_raiseinvalidate_bundle; migrate CLI and builder to use it; restore dry-on-load with a new integration test.graph_renderingto dry-run bundles viaPipelexRunnerand emitGraphSpec.PipelexRunnertoPipeRun/PipeRouterfor consistent LIVE/DRY execution; DRY runs locally for graphing/validation.WorkingMemoryFactorywithconvert_input_specs_to_typed; replace oldconvert_to_working_memory_formatcalls.keep_library_loadedtopipeline_run_setup/runner to control library teardown..notes/dry-run-refactor/*.Migration
dry_run_pipe,dry_run_pipes,dry_run_pipeline,dry_run_with_graphwithdry_run_loaded_pipes_or_raiseor run viaPipelexRunner+PipeRunMode.DRY.DryPipeRouterandDryRunStatusreferences.allowed_to_fail_pipesfrompipelex.tomlandDryRunConfig.WorkingMemoryFactory.convert_input_specs_to_typed.Written for commit 94938d5. Summary will update on new commits. Review in cubic