feat(sdk,ci): 'rocketride diff' — semantic .pipe diff (layout-noise-free) + PR-comment Action#1607
Conversation
…rocketride-org#1606) New pipediff package: a structural diff that surfaces what actually changed in a pipeline and hides the canvas coordinate churn that dominates a raw JSON diff. - engine.py: strict-JSON load + shape validation; match components by id; report added/removed nodes, provider changes, and a deep config diff rendered as dotted field paths (config.parameters.model: gpt-4 -> gpt-4o) over nested dicts and lists; reconstruct edges from each component's input[] (data lanes) AND control[] (agent orchestration wiring — present in most bundled examples) as a (from, lane, to) set diff. - The ui block (position/measured), top-level viewport, and untouched- node layout are ignored by default and collapse to a single 'layout changed' flag; include_layout surfaces them. A pure canvas move is never mistaken for a semantic change. - gitref.py: resolve a pipe from a git ref via 'git show' (arg-list, never a shell string; None when the path is absent in the ref = all-added; PipeDiffError on git/parse failure). - model.py: frozen FieldChange/EdgeChange + NodeChange/PipeDiff with a has_semantic_changes property. 49 unit tests (engine + gitref), including a real tmp-repo git integration test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
rocketride diff <old.pipe> <new.pipe> | --git <ref> <file.pipe> [--include-layout] [--json] [--markdown] [--exit-zero] Semantic pipeline diff for terminal and CI review. Unlike the other subcommands it takes no connection args and never touches the engine or network — it is pure local file/JSON/git work. Human output groups Nodes/Edges/Config with +/-/~ markers (NO_COLOR and non-tty aware); --json emits a single change document; --markdown emits PR-comment-safe output (untrusted .pipe values are escaped so a config value can't break out of a comment). Exit codes: 0 no semantic changes, 1 changes present, 2 usage/parse/git error; --exit-zero forces 0 for non-gating use. 30 tests (reporters + CLI) driving the full command against parsed fixtures — no server required. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ticky PR comment On PRs touching .pipe files, renders each changed file's semantic diff (vs the PR base) and posts/updates one sticky comment — turning 'review pipelines like code' into visible review UX. Installs the rocketride CLI (fails fast if the release lacks 'diff'; cli-version to pin), fetches the PR base commit so it works with actions/checkout default fetch-depth 1, and runs 'rocketride diff --git <base> <file> --markdown'. Third-party actions pinned by commit SHA; GITHUB_TOKEN never echoed; needs pull-requests: write (documented). No existing workflow files touched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Documents the diff subcommand (flags, --git mode, three output modes, exit codes) and adds examples/pipe-diff-example.md showing a raw JSON diff full of x/y churn next to the semantic output for the same change. Ties validate + eval + diff into the 'pipelines reviewed like code in CI' story. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
github-actions
Bot
added
docs
🤖 Internal: Discord sync markerAuto-managed by the Discord notification workflow. Stores the linked Discord message ID and forum thread ID. Do not edit or delete. |
nihalnihalani
left a comment
nihalnihalani
left a comment
There was a problem hiding this comment.
Self-review from the author: inline notes on the four decisions most worth a reviewer's attention. Full evidence (79 unit tests, the raw-vs-semantic money demo, git-mode, markdown-injection audit) is in the PR description.
| return indexed | ||
|
|
||
|
|
||
| def _extract_edges(pipe: dict) -> set[tuple[Any, Any, Any]]: |
There was a problem hiding this comment.
The one deliberate extension beyond the issue spec: _extract_edges reads both input[] (data lanes) AND control[] (agent-orchestration wiring: agent→llm/tool/memory). I checked the bundled corpus — control[] appears in 7 of 10 example .pipe files and is the only channel agent workflows use to express that wiring, so an input-only differ would be blind to agent reconfiguration (the exact kind of change reviewers most need to see). input[] stays canonical for data lanes; both are normalized to the same (from, lane, to) triple set. Trivially revertible to input-only by dropping the control loop if you'd prefer to keep scope tighter — flagging it explicitly rather than burying it.
| return _diff_value(old if old is not None else {}, new if new is not None else {}, 'config') | ||
|
|
||
|
|
||
| def diff_pipes(old: dict, new: dict, *, include_layout: bool = False) -> PipeDiff: |
There was a problem hiding this comment.
The whole value proposition lives in what this ignores. ui (position/measured), top-level viewport, and the layout of untouched nodes are excluded by default and collapsed to a single layout_changed flag — so a pure canvas nudge yields has_semantic_changes == False even though the JSON changed. --include-layout opts the ui.* field churn back in for the rare case someone wants it. There's a dedicated test asserting a position-only change registers layout_changed=True but has_semantic_changes=False; the money demo in the PR shows 11 nudged positions collapsing to one line. A provider change on a matched id is reported as its own NodeChange kind (not add+remove), and separately from any config change on the same node.
| # ========================================================================= | ||
|
|
||
|
|
||
| def _md_code(text: str) -> str: |
There was a problem hiding this comment.
_md_code is the security choke point and was a devil's-advocate catch. Because --markdown output goes into a PR comment, untrusted .pipe content (a config value, a node id) reaches rendered markdown — a value containing backticks, |, or </details> could otherwise break out of a table cell or the comment itself. Every value rendered in markdown routes through here: pipe-escaped and wrapped in a backtick code span with a double-backtick delimiter so embedded backticks can't close it early. A test feeds exactly those payloads and asserts the output is inert. Human/JSON modes don't need this (no markup), but centralizing it means no reporter can forget it.
| PipeDiffError: If ``git`` is not found on ``PATH`` or the call times out. | ||
| """ | ||
| try: | ||
| return subprocess.run( |
There was a problem hiding this comment.
--git mode shells to git show but never builds a shell string — args are passed as a list (subprocess.run([...], shell=False)), so a ref or path can't inject. Three outcomes handled distinctly: file present → parse and diff; path absent in the ref → return None, which the engine treats as 'everything added' (correct for a newly-added pipe in a PR); git/parse failure → PipeDiffError → CLI exit 2 with a clear stderr message. The path is realpath'd before the relpath-against-repo-root computation so symlinked repo roots (macOS /var vs /private/var) resolve consistently — exercised by the real tmp-repo integration test.
📝 WalkthroughWalkthroughChangesPipe diff engine and renderers
CLI and CI integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant PullRequest
participant PipeDiffAction
participant Git
participant RocketRideCLI
participant PipeDiffEngine
participant GitHubComment
PullRequest->>PipeDiffAction: trigger workflow
PipeDiffAction->>Git: resolve changed files and base revisions
PipeDiffAction->>RocketRideCLI: run local semantic diff
RocketRideCLI->>PipeDiffEngine: compare pipeline files
PipeDiffEngine-->>RocketRideCLI: return structured diff
RocketRideCLI-->>PipeDiffAction: return Markdown and exit code
PipeDiffAction->>GitHubComment: create or update sticky comment
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/docs/content-static/cli.mdxOops! Something went wrong! :( ESLint: 9.39.4 TypeError: expand is not a function 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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/client-python/src/rocketride/cli/main.py (1)
573-581: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate
run()’s exit-code documentation fordiff.The docstring still says 0 means success and 1 means error, while this branch returns 1 for detected changes and 2 for errors. Document the command-specific contract so generated SDK reference documentation is accurate.
Based on learnings, public Python SDK reference documentation is generated from source docstrings.
Also applies to: 593-598
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/client-python/src/rocketride/cli/main.py` around lines 573 - 581, Update the run() docstring’s exit-code documentation to describe the diff command’s contract: return 0 when no changes are detected, 1 when changes are detected, and 2 for errors. Preserve accurate documentation for other commands and ensure the generated SDK reference reflects these command-specific outcomes.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/actions/pipe-diff/action.yml:
- Around line 308-315: Update the existing comment lookup in the sticky-comment
flow to require both the hidden marker and the expected GitHub Actions bot
author before selecting a comment. Preserve the current pagination and update
behavior, but ensure user-authored comments containing the marker are never
modified.
- Around line 225-258: Update the changed-file reporting and error handling
around the loop over changed_files so untrusted filenames and err_msg cannot
inject Markdown or workflow commands. Encode filenames safely for Markdown code
spans, escape or use a collision-safe fenced block for error text, and safely
encode the file and message values passed to the ::error annotation while
preserving the existing semantic-diff and failure output.
- Around line 62-66: Make the non-commenting path publish the captured diff by
appending body_file to $GITHUB_STEP_SUMMARY, including early-success paths in
the action flow around the relevant summary handling. Update the comment
description in .github/actions/pipe-diff/action.yml lines 62-66 only to promise
a job summary if this behavior is implemented, and align the corresponding
non-comment behavior documentation in .github/actions/pipe-diff/README.md lines
60-62 with the actual publication channel.
- Around line 241-245: Update the semantic-result handling around the case “0”
branch and its related status logic to track deletion state separately from
layout-only changes. Treat deletion-only diffs as semantic changes, including
setting has-semantic-changes=true and avoiding the “No semantic changes (layout
only)” message, while preserving the existing formatting-only and genuinely
identical-content behavior.
- Around line 23-34: Limit the network-free claim in the action metadata and
description to the local CLI semantic comparison, removing any implication that
the complete action avoids network access; update
.github/actions/pipe-diff/action.yml lines 23-34 accordingly. Document in
.github/actions/pipe-diff/README.md lines 12-15 that the action installs from
PyPI, fetches from Git, and may use GitHub’s comment API, while the comparison
itself remains local.
In `@examples/pipe-diff-example.md`:
- Around line 159-188: Remove the “## Pipeline changes in rag.pipe” heading from
the bare --markdown CLI output example, since the command output begins with
“**Pipeline diff:**”. If the heading is needed for surrounding documentation,
clearly mark it as wrapper-added content rather than CLI output.
In `@packages/client-python/src/rocketride/pipediff/__init__.py`:
- Around line 59-67: Remove the opportunistic try/except around the reporter
imports in the pipediff package initializer. Import render_human, render_json,
and render_markdown directly from the sibling reporters module, and retain their
entries in __all__ so any packaging or transitive import failure surfaces
immediately.
In `@packages/client-python/src/rocketride/pipediff/engine.py`:
- Around line 103-112: Normalize invalid UTF-8 handling in both input paths:
update the local-file read flow around engine.py lines 103-112 to catch
UnicodeError and raise a contextual PipeDiffError, and update the text-mode Git
output handling in gitref.py lines 149-155 similarly. Add regression coverage
for malformed UTF-8 in local files and git-ref inputs, preserving the documented
exit code 2.
- Around line 135-143: Update the validation logic around component wire
processing before _extract_edges to require every input/control collection to be
a list and validate each wire’s required scalar fields, including lane and from,
with appropriate types and non-empty values. Reject malformed or unhashable
values using PipeDiffError before edge extraction, while preserving the existing
component validation behavior.
- Around line 233-239: Update the include_layout handling in the diff engine so
top-level viewport changes are represented as semantic NodeChange entries
alongside component ui changes. Ensure viewport-only edits set the semantic
change state and produce a nonzero diff result when include_layout is enabled,
while preserving current behavior when it is disabled. Add a regression test
covering a viewport-only change.
In `@packages/client-python/src/rocketride/pipediff/reporters.py`:
- Around line 292-294: Update the human and Markdown rendering paths in the
relevant reporter functions, including the version-change block and the title,
ID, lane, provider, path, and string-value outputs. Route Markdown title and
version values through _md_code, and introduce or reuse a terminal-safe
formatter that visibly escapes CR/LF and control characters before every
human-rendered value is emitted. Add payload tests covering malicious titles,
versions, and \x1b sequences.
In `@packages/client-python/tests/test_pipediff_reporters.py`:
- Around line 23-30: Update the tests in test_pipediff_reporters.py to import
and use the four production diff model classes from rocketride.pipediff.model
instead of defining mirrored lightweight dataclasses. Replace the local fixture
types and construct test data with the real public model API while preserving
the existing reporter coverage and assertions.
---
Outside diff comments:
In `@packages/client-python/src/rocketride/cli/main.py`:
- Around line 573-581: Update the run() docstring’s exit-code documentation to
describe the diff command’s contract: return 0 when no changes are detected, 1
when changes are detected, and 2 for errors. Preserve accurate documentation for
other commands and ensure the generated SDK reference reflects these
command-specific outcomes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 27137df3-2ed6-4150-8eed-c18900d4adfb
📒 Files selected for processing (17)
.github/actions/pipe-diff/README.md.github/actions/pipe-diff/action.ymldocs/README-python-client.mdexamples/pipe-diff-example.mdpackages/client-python/src/rocketride/cli/commands/__init__.pypackages/client-python/src/rocketride/cli/commands/diff.pypackages/client-python/src/rocketride/cli/main.pypackages/client-python/src/rocketride/pipediff/__init__.pypackages/client-python/src/rocketride/pipediff/engine.pypackages/client-python/src/rocketride/pipediff/gitref.pypackages/client-python/src/rocketride/pipediff/model.pypackages/client-python/src/rocketride/pipediff/reporters.pypackages/client-python/tests/test_diff_cli.pypackages/client-python/tests/test_pipediff_engine.pypackages/client-python/tests/test_pipediff_gitref.pypackages/client-python/tests/test_pipediff_reporters.pypackages/docs/content-static/cli.mdx
| # Composite action that posts a semantic diff of changed .pipe pipeline files as | ||
| # a sticky pull-request comment. It wraps the local `rocketride diff` subcommand | ||
| # (Python SDK/CLI), which compares parsed .pipe JSON entirely on the runner and | ||
| # never contacts the RocketRide engine or the network. Raw JSON diffs of .pipe | ||
| # files are dominated by canvas coordinate churn (the per-node "ui" block and the | ||
| # top-level "viewport"); this surfaces what actually changed — nodes, edges, and | ||
| # config — so reviewers see signal instead of layout noise. | ||
| name: RocketRide Pipe Diff | ||
| description: >- | ||
| Comment a semantic diff of changed .pipe pipeline files on a pull request, | ||
| hiding canvas-layout noise. Runs entirely on the runner; never contacts the | ||
| RocketRide engine or the network. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Limit the network-free guarantee to the CLI computation. The action itself installs from PyPI, fetches from Git, and calls GitHub APIs.
.github/actions/pipe-diff/action.yml#L23-L34: describe the semantic comparison as local, without claiming that the complete action avoids network access..github/actions/pipe-diff/README.md#L12-L15: document the action’s installation, fetch, and optional comment API traffic.
📍 Affects 2 files
.github/actions/pipe-diff/action.yml#L23-L34(this comment).github/actions/pipe-diff/README.md#L12-L15
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/actions/pipe-diff/action.yml around lines 23 - 34, Limit the
network-free claim in the action metadata and description to the local CLI
semantic comparison, removing any implication that the complete action avoids
network access; update .github/actions/pipe-diff/action.yml lines 23-34
accordingly. Document in .github/actions/pipe-diff/README.md lines 12-15 that
the action installs from PyPI, fetches from Git, and may use GitHub’s comment
API, while the comparison itself remains local.
| comment: | ||
| description: >- | ||
| When "true" (default), post or update ONE sticky PR comment with the diff. | ||
| Set to "false" to compute the diff and write a job summary without | ||
| commenting (requires no pull-requests:write permission). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the generated diff visible when commenting is disabled. The implementation captures the report but currently publishes neither a job summary nor report content in the log.
.github/actions/pipe-diff/action.yml#L62-L66: retain the job-summary promise only if implemented..github/actions/pipe-diff/action.yml#L277-L286: appendbody_fileto$GITHUB_STEP_SUMMARY, including early-success paths..github/actions/pipe-diff/README.md#L60-L62: align the documented non-comment behavior with the implemented publication channel.
📍 Affects 2 files
.github/actions/pipe-diff/action.yml#L62-L66(this comment).github/actions/pipe-diff/action.yml#L277-L286.github/actions/pipe-diff/README.md#L60-L62
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/actions/pipe-diff/action.yml around lines 62 - 66, Make the
non-commenting path publish the captured diff by appending body_file to
$GITHUB_STEP_SUMMARY, including early-success paths in the action flow around
the relevant summary handling. Update the comment description in
.github/actions/pipe-diff/action.yml lines 62-66 only to promise a job summary
if this behavior is implemented, and align the corresponding non-comment
behavior documentation in .github/actions/pipe-diff/README.md lines 60-62 with
the actual publication channel.
| for f in ${changed_files[@]+"${changed_files[@]}"}; do | ||
| printf '### `%s`\n\n' "${f}" >> "${sections_file}" | ||
|
|
||
| # Build the command as an always-non-empty array so "${cmd[@]}" is | ||
| # safe under `set -u` on every bash version (an empty-array expansion | ||
| # errors on bash < 4.4). | ||
| cmd=(rocketride diff --git "${BASE_SHA}" "${f}" --markdown) | ||
| if [ -n "${include_layout_flag}" ]; then | ||
| cmd+=("${include_layout_flag}") | ||
| fi | ||
|
|
||
| set +e | ||
| out="$("${cmd[@]}" 2>"${err_file}")" | ||
| rc=$? | ||
| set -e | ||
|
|
||
| case "${rc}" in | ||
| 0) | ||
| # Exit 0 = no semantic changes. Any textual change the git diff | ||
| # picked up was pure layout/formatting noise. | ||
| printf 'No semantic changes (layout only).\n\n' >> "${sections_file}" | ||
| ;; | ||
| 1) | ||
| any_semantic=true | ||
| printf '%s\n\n' "${out}" >> "${sections_file}" | ||
| ;; | ||
| *) | ||
| any_error=true | ||
| err_msg="$(cat "${err_file}")" | ||
| echo "::error file=${f}::rocketride diff failed for ${f}: ${err_msg}" | ||
| { | ||
| printf 'Could not diff this file (exit %s):\n\n' "${rc}" | ||
| printf '```\n%s\n```\n\n' "${err_msg}" | ||
| } >> "${sections_file}" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Escape action-generated Markdown and workflow-command values.
Git paths can contain backticks or newlines, and error text can close the triple-backtick fence. A PR can therefore inject headings, mentions, or misleading content into the sticky comment and malformed annotations. Encode filenames as safe Markdown code spans and use a collision-safe fence/escaping routine for errors.
Also applies to: 263-265
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/actions/pipe-diff/action.yml around lines 225 - 258, Update the
changed-file reporting and error handling around the loop over changed_files so
untrusted filenames and err_msg cannot inject Markdown or workflow commands.
Encode filenames safely for Markdown code spans, escape or use a collision-safe
fenced block for error text, and safely encode the file and message values
passed to the ::error annotation while preserving the existing semantic-diff and
failure output.
| case "${rc}" in | ||
| 0) | ||
| # Exit 0 = no semantic changes. Any textual change the git diff | ||
| # picked up was pure layout/formatting noise. | ||
| printf 'No semantic changes (layout only).\n\n' >> "${sections_file}" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not classify every non-successful semantic result as “layout only.”
Exit 0 can represent formatting-only or genuinely identical content, while a deleted pipeline is a semantic removal. Currently a deletion-only PR reports No semantic changes (layout only) and has-semantic-changes=false. Track deletion/layout state separately and mark deletions as semantic changes.
Also applies to: 263-275
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/actions/pipe-diff/action.yml around lines 241 - 245, Update the
semantic-result handling around the case “0” branch and its related status logic
to track deletion state separately from layout-only changes. Treat deletion-only
diffs as semantic changes, including setting has-semantic-changes=true and
avoiding the “No semantic changes (layout only)” message, while preserving the
existing formatting-only and genuinely identical-content behavior.
| // Find our one sticky comment by its hidden marker. | ||
| const comments = await github.paginate(github.rest.issues.listComments, { | ||
| owner, repo, issue_number, per_page: 100, | ||
| }); | ||
| const existing = comments.find((c) => c.body && c.body.includes(marker)); | ||
|
|
||
| if (existing) { | ||
| await github.rest.issues.updateComment({ owner, repo, comment_id: existing.id, body }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Only update comments created by this action.
Searching solely for the public marker can select and overwrite a user-authored comment containing that marker. Restrict matches to the expected GitHub Actions bot author as well as the marker.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/actions/pipe-diff/action.yml around lines 308 - 315, Update the
existing comment lookup in the sticky-comment flow to require both the hidden
marker and the expected GitHub Actions bot author before selecting a comment.
Preserve the current pagination and update behavior, but ensure user-authored
comments containing the marker are never modified.
| with open(source, encoding='utf-8') as handle: | ||
| text = handle.read() | ||
| except FileNotFoundError as exc: | ||
| raise PipeDiffError(f'Pipe file not found: {source}') from exc | ||
| except OSError as exc: | ||
| raise PipeDiffError(f'Could not read pipe file {source}: {exc}') from exc | ||
| try: | ||
| obj = json.loads(text) | ||
| except json.JSONDecodeError as exc: | ||
| raise PipeDiffError(f'Invalid JSON in {source}: {exc}') from exc |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Normalize UTF-8 decoding failures across both input paths.
Malformed encoding currently bypasses PipeDiffError, so unreadable inputs can produce a traceback rather than the documented exit code 2.
packages/client-python/src/rocketride/pipediff/engine.py#L103-L112: catchUnicodeErrorwhile reading local files and raise contextualPipeDiffError.packages/client-python/src/rocketride/pipediff/gitref.py#L149-L155: catchUnicodeErrorfrom text-mode Git output and raise contextualPipeDiffError.
Add regressions for invalid UTF-8 in both local and git-ref inputs.
📍 Affects 2 files
packages/client-python/src/rocketride/pipediff/engine.py#L103-L112(this comment)packages/client-python/src/rocketride/pipediff/gitref.py#L149-L155
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/client-python/src/rocketride/pipediff/engine.py` around lines 103 -
112, Normalize invalid UTF-8 handling in both input paths: update the local-file
read flow around engine.py lines 103-112 to catch UnicodeError and raise a
contextual PipeDiffError, and update the text-mode Git output handling in
gitref.py lines 149-155 similarly. Add regression coverage for malformed UTF-8
in local files and git-ref inputs, preserving the documented exit code 2.
| components = obj.get('components') | ||
| if not isinstance(components, list): | ||
| raise PipeDiffError(f"{source}: pipe is missing a 'components' list") | ||
| for index, component in enumerate(components): | ||
| if not isinstance(component, dict): | ||
| raise PipeDiffError(f'{source}: component at index {index} is not an object') | ||
| component_id = component.get('id') | ||
| if not isinstance(component_id, str) or not component_id: | ||
| raise PipeDiffError(f"{source}: component at index {index} is missing a string 'id'") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate wire collections before _extract_edges consumes them.
Validation accepts arbitrary input/control values and wire fields. A numeric collection or unhashable lane/from value causes TypeError; malformed dictionaries can instead create bogus None edges. Validate each collection as a list and each wire’s required scalar fields before diffing.
Also applies to: 313-318
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/client-python/src/rocketride/pipediff/engine.py` around lines 135 -
143, Update the validation logic around component wire processing before
_extract_edges to require every input/control collection to be a list and
validate each wire’s required scalar fields, including lane and from, with
appropriate types and non-empty values. Reject malformed or unhashable values
using PipeDiffError before edge extraction, while preserving the existing
component validation behavior.
| field_changes = deep_diff_config(old_component.get('config'), new_component.get('config')) | ||
| if include_layout: | ||
| field_changes = field_changes + _diff_value( | ||
| old_component.get('ui') or {}, new_component.get('ui') or {}, 'ui' | ||
| ) | ||
| if field_changes: | ||
| node_changes.append(NodeChange(id=component_id, kind='config', field_changes=field_changes)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Make --include-layout include top-level viewport changes.
Only component ui changes are promoted into node_changes. Because layout_changed never gates semantic changes, a viewport-only edit still exits 0 with include_layout=True, contradicting the CLI contract. Represent viewport field changes—or otherwise make them semantic when opted in—and add a viewport-only regression test.
Also applies to: 255-262, 348-351
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/client-python/src/rocketride/pipediff/engine.py` around lines 233 -
239, Update the include_layout handling in the diff engine so top-level viewport
changes are represented as semantic NodeChange entries alongside component ui
changes. Ensure viewport-only edits set the semantic change state and produce a
nonzero diff result when include_layout is enabled, while preserving current
behavior when it is disabled. Add a regression test covering a viewport-only
change.
| version_change = org['version_change'] | ||
| if version_change: | ||
| parts.append(f'version {_fmt_value(version_change[0])} → {_fmt_value(version_change[1])}') |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Encode untrusted values at every output sink.
A .pipe version can break out of the Markdown summary, and caller-controlled title is emitted as raw Markdown. Human output also writes IDs, lanes, providers, paths, and string values verbatim, allowing forged lines or ANSI terminal-control injection.
Route Markdown title/version values through _md_code, and apply a terminal-safe formatter that visibly escapes CR/LF and control characters to every human-rendered value. Add payload tests for title, version, and \x1b.
Also applies to: 309-322, 359-377, 385-394, 554-558
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/client-python/src/rocketride/pipediff/reporters.py` around lines 292
- 294, Update the human and Markdown rendering paths in the relevant reporter
functions, including the version-change block and the title, ID, lane, provider,
path, and string-value outputs. Route Markdown title and version values through
_md_code, and introduce or reuse a terminal-safe formatter that visibly escapes
CR/LF and control characters before every human-rendered value is emitted. Add
payload tests covering malicious titles, versions, and \x1b sequences.
| """ | ||
| Tests for rocketride.pipediff.reporters. | ||
|
|
||
| The reporters are read-only over the diff model and are duck-typed, so these | ||
| tests build fixtures from lightweight dataclasses that mirror the pinned shapes | ||
| of ``rocketride.pipediff.model`` (owner: Implementer A). The real model | ||
| dataclasses are structurally identical; exercising the reporters against these | ||
| mirrors runs the true render code without depending on the engine being present. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Use the production diff model instead of mirrored dataclasses.
These copies can drift from rocketride.pipediff.model while tests remain green. Import the real four model classes, which also exercises the package’s newly declared public API.
Proposed refactor
import json
import unittest
-from dataclasses import dataclass, field
-from typing import Any, List, Optional, Tuple
+from rocketride.pipediff import EdgeChange, FieldChange, NodeChange, PipeDiff
from rocketride.pipediff.reporters import render_human, render_json, render_markdown
-
-# Remove the local FieldChange, NodeChange, EdgeChange, and PipeDiff mirrors.Also applies to: 40-96
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/client-python/tests/test_pipediff_reporters.py` around lines 23 -
30, Update the tests in test_pipediff_reporters.py to import and use the four
production diff model classes from rocketride.pipediff.model instead of defining
mirrored lightweight dataclasses. Replace the local fixture types and construct
test data with the real public model API while preserving the existing reporter
coverage and assertions.
1 participant
#Hire-Me-RocketRide
Contribution Type
Feature —
rocketride diff: a semantic diff for.pipefiles (Python CLI + a PR-comment GitHub Action). Makes pipeline changes actually reviewable in git/CI.Closes #1606
Summary
RocketRide's wedge is that pipelines are git-versioned JSON reviewed like code — but a reviewer looking at a
.pipechange today sees raw JSON, dominated by canvas coordinate churn. Every node carries auiblock (position,measured) that changes whenever anyone nudges the canvas, so a one-line model swap can surface as 70 lines ofx/ydeltas. This is the exact pain even the market leader hasn't solved (n8n's noisy git-sync diffs are a standing Enterprise complaint).Same change, two views — a diff that swaps one model, adds a node, rewires an edge, and nudges every canvas position:
diff -u(today)rocketride diff(this PR)Edges
Config
llm_openai_1
+ config.parameters.model = gpt-4o
Layout: changed (ui/viewport)
This completes a coherent CI story with the other two subcommands proposed in this batch:
validateproves a pipe is well-formed,evalproves it behaves, anddiffmakes the change itself reviewable — structure, behavior, review, all in git.id; report added/removed nodes,providerchanges, and a deepconfigdiff rendered as dotted field paths (config.parameters.model: gpt-4 → gpt-4o) over nested dicts and lists; reconstruct edges from each component'sinput[](data lanes) andcontrol[](agent-orchestration wiring — present in most bundled examples) as a(from, lane, to)set diff.uiblock, top-levelviewport, and untouched-node layout collapse to a singlelayout changedflag;--include-layoutopts the fullx/ychurn back in. A pure canvas move is never mistaken for a semantic change.--git <ref> <file.pipe>to diff a working-tree pipe against a git ref (HEAD, a branch, the PR base) viagit show— no engine, no network, no server (a deliberate difference from every other subcommand, stated in--help).NO_COLOR/non-tty aware),--json(single change document),--markdown(PR-comment-safe). Exit codes mirror the batch:0no changes ·1changes ·2usage/parse/git error;--exit-zerofor non-gating use..github/actions/pipe-diff): on PRs touching.pipefiles, renders each changed file's semantic diff vs the PR base and posts/updates one sticky comment — "real diffs, real review" made visible. Pure Python, no engine.Validation
Unit (no server): 79/79.
providerchange reported separately fromconfig; nested-dict added/removed/changed keys; list element + list-length changes with correct index paths; edge add/remove/rewire;control[]edge diff; version change; layout-only change →has_semantic_changesFalse butlayout_changedTrue; malformed/non-object/missing-components/missing-id→PipeDiffError.--jsonpurity (document only on stdout; errors to stderr),--exit-zero,--include-layouttoggling.ruff check+ruff format --checkclean on all 10 files;gitleaksclean;action.ymlYAML-parses and all bash blocks passbash -n.Security — markdown injection (devil's-advocate catch, fixed): untrusted
.pipecontent reaches a PR comment, so a config value containing backticks /|/</details>could have broken out of the rendered comment. Fixed at the single choke point (_md_code) — values are pipe-escaped and wrapped in a backtick code span with a double-backtick delimiter; a dedicated test confirms the payload renders inert.Live demo (real engine not required — it's client-side): ran the pip-installed CLI against real
examples/*.pipederivatives — the money demo above reproduces exactly (11 position nudges collapse to one line by default;--include-layoutexpands them), identical copy → exit 0, git mode against a throwaway repo shows the working-tree change and exits 2 cleanly on a bad ref.Notes for reviewers
input[]andcontrol[].control[]is how agent workflows wire agent→llm/tool/memory and it appears in 7/10 bundled examples; diffing onlyinput[]would make the diff blind to agent reconfiguration.input[]stays canonical for data lanes; documented in the engine docstrings, trivially revertible if you'd prefer input-only.versionchanges always count as semantic (their own line);ui/viewportare pure layout, hidden unless--include-layout.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
diffcommand for comparing.pipepipeline files and Git revisions.Documentation