Skip to content

refactor(pipeline): add PipelineError::reconstruct to de-duplicate outcome rematch - #416

Merged
nh13 merged 1 commit into
feat-runallfrom
feat-runall-22-pipeline-error-reconstruct
Jun 16, 2026
Merged

nh13 merged 1 commit into
feat-runallfrom
feat-runall-22-pipeline-error-reconstruct

Conversation

@nh13

@nh13 nh13 commented Jun 16, 2026

Copy link
Copy Markdown
Member

Summary

Pipeline::run (core/builder.rs) and the fused single-thread driver (core/runtime/fused.rs) each open-coded the same per-variant match to turn the borrowed signal.outcome(): Option<&PipelineError> into the owned error they return — necessary because PipelineError is not Clone (its Io variant holds a non-Clone io::Error). Extract that into PipelineError::reconstruct(&self) -> PipelineError; both call sites collapse to Some(err) => Err(err.reconstruct()).

Behavior preservation

Identical — the Io source is still rebuilt from kind() + display string, all other variants copied. A future error variant now needs handling in one place instead of two. Fused-pipeline and pipeline error-path tests pass unchanged.

Context

Third of the post-#330 subsystem-simplification cleanups (discovery bundle "pipeline misc"). Dual-reviewed before push (CLI: no findings; skill: 0 actionable).

@nh13
nh13 temporarily deployed to github-actions June 16, 2026 03:25 — with GitHub Actions Inactive
@codecov

codecov Bot commented Jun 16, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.00000% with 1 line in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (feat-runall@d1aa593). Learn more about missing BASE report.

Files with missing lines Patch % Lines
src/lib/pipeline/core/signal.rs 95.65% 1 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff               @@
##             feat-runall     #416   +/-   ##
==============================================
  Coverage               ?   93.31%           
==============================================
  Files                  ?      138           
  Lines                  ?    54928           
  Branches               ?        0           
==============================================
  Hits                   ?    51254           
  Misses                 ?     3674           
  Partials               ?        0           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…tcome rematch

`Pipeline::run` (core/builder.rs) and the fused single-thread driver
(core/runtime/fused.rs) each open-coded the same per-variant match to turn the
borrowed `signal.outcome(): Option<&PipelineError>` into the owned error they
return — necessary because `PipelineError` is not `Clone` (its `Io` variant
holds a non-`Clone` `io::Error`). Extract that into
`PipelineError::reconstruct(&self) -> PipelineError`; both call sites collapse
to `Some(err) => Err(err.reconstruct())`.

Behavior-identical (the `Io` source is still rebuilt from `kind()` + display
string), and a future error variant now only needs handling in one place
instead of two. Fused-pipeline and pipeline error-path tests pass unchanged.
@nh13
nh13 force-pushed the feat-runall-22-pipeline-error-reconstruct branch from 4a6fa4b to 87ed834 Compare June 16, 2026 03:36
@nh13
nh13 temporarily deployed to github-actions June 16, 2026 03:36 — with GitHub Actions Inactive
@nh13

nh13 commented Jun 16, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 16, 2026 •

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai

coderabbitai Bot commented Jun 16, 2026 •

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@nh13, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 36 minutes and 15 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 1d296e52-aeea-44e2-ab7d-a7d0865841fe

📥 Commits

Reviewing files that changed from the base of the PR and between d1aa593 and 87ed834.

📒 Files selected for processing (3)
  • src/lib/pipeline/core/builder.rs
  • src/lib/pipeline/core/runtime/fused.rs
  • src/lib/pipeline/core/signal.rs
📝 Walkthrough

Walkthrough

Adds PipelineError::reconstruct(&self) -> PipelineError to signal.rs, which rebuilds an owned PipelineError from a borrowed one—recreating the Io variant via io::Error::new(kind(), display_string) and copying other variants directly. Two call sites—Pipeline::run in builder.rs and run_fused_single_thread in fused.rs—drop their explicit per-variant match arms and the local PipelineError import in favor of a single err.reconstruct() call. A new unit test verifies round-trip fidelity for all four variants.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed Title accurately describes the main refactoring: extracting PipelineError::reconstruct to eliminate duplicated outcome matching logic across two call sites.
Description check ✅ Passed Description clearly explains the refactoring's purpose, implementation approach, and behavior preservation; directly relates to the code changes.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat-runall-22-pipeline-error-reconstruct

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.


Comment @coderabbitai help to get the list of available commands and usage tips.

@nh13

nh13 commented Jun 16, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Jun 16, 2026 •

Copy link
Copy Markdown
✅ Action performed

Full review finished.

@nh13
nh13 merged commit 049471a into feat-runall Jun 16, 2026
10 checks passed
@nh13
nh13 deleted the feat-runall-22-pipeline-error-reconstruct branch June 16, 2026 14:47
nh13 added a commit that referenced this pull request Jun 18, 2026
…tcome rematch (#416)

`Pipeline::run` (core/builder.rs) and the fused single-thread driver
(core/runtime/fused.rs) each open-coded the same per-variant match to turn the
borrowed `signal.outcome(): Option<&PipelineError>` into the owned error they
return — necessary because `PipelineError` is not `Clone` (its `Io` variant
holds a non-`Clone` `io::Error`). Extract that into
`PipelineError::reconstruct(&self) -> PipelineError`; both call sites collapse
to `Some(err) => Err(err.reconstruct())`.

Behavior-identical (the `Io` source is still rebuilt from `kind()` + display
string), and a future error variant now only needs handling in one place
instead of two. Fused-pipeline and pipeline error-path tests pass unchanged.

This branch was previously deployed

1 inactive deployment
github-actions — 87ed8349 Deployed Jun 16, 2026 by nh13 via coverage #1644
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant