Skip to content

fix(import): accept structured _comment / install_instructions in bundles (#755 part B)#767

Merged
rlagowski merged 2 commits into
developfrom
fix/pipeline-import-version
Jun 8, 2026
Merged

fix(import): accept structured _comment / install_instructions in bundles (#755 part B)#767
rlagowski merged 2 commits into
developfrom
fix/pipeline-import-version

Conversation

@rafeekpro

Copy link
Copy Markdown
Collaborator

What

Part B of #755. Real pipeline bundles (cortex) ship install_instructions and _comment as structured blocks (operator steps + required env vars), not strings — but PipelineImportRequest typed them as str | None, so posting a full bundle 422'd with a string_type error, forcing consumers to strip those keys before import. Type them as Any | None: they're ignored on import regardless of shape.

Tests

test_pipeline_bundle_v2.py (+1): a bundle with a structured install_instructions (steps + required_env) and a dict _comment imports 201. Existing string-form ignore tests still green; ruff/mypy/pytest clean.

Not in this PR — #755 part A (version-bump on name collision)

The main ask (re-import → bump version under the same pipeline_id instead of a duplicate row) is a larger, higher-risk engine change. Findings while scoping it:

  • repo.update_pipeline bumps the version but does not carry backend_profiles onto the new version — cortex relies on those, so a naive bump would drop them. Needs a dedicated import-bump path (or extending update_pipeline).
  • Bundled-agent import re-creates agents every time, so node agent_ids always differ → the "identical definition → no-op" case can't be detected by raw comparison; "always bump on name match" is the reliable behaviour.
  • It mutates which version runs route to, so it needs careful testing.

Recommend a focused follow-up PR for part A.

…dles (#755)

Real pipeline bundles (e.g. cortex) ship install_instructions and _comment as
structured blocks (operator steps + required env), not strings — but the import
schema typed them as str|None, so a full bundle 422'd with a string_type error
(forcing consumers to strip those keys first). Type them as Any|None: they're
ignored on import regardless of shape.

Part B of #755. Part A (version-bump on name collision instead of a duplicate
pipeline row) is a larger engine change — tracked separately.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review council

Findings: no actionable findings

No critical issues found after analysis by the review council.

✅ Council signalled convergence — further analysis would yield hypothetical findings only.


🤖 Reviewed by the Code Review Council (openrouter:deepseek/deepseek-v4-pro) via .github/workflows/gemini-review.yml.

…chema change (#755)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review council

Findings: 🟡 MEDIUM: 1
By agent: Correctness: 1

Council flagged 1 medium. The change relaxes the types for install_instructions and _comment in the pipeline import schema from str | None to Any | None because real bundles ship structured blocks. From a security perspective this is a low-risk expansion: the fields are 'ignored on import', there is no new authentication/authorization bypass, no secret is hardcoded, and the audit trail is unchanged. The TypeScript generated types mirror the change without introducing new client-side risks. The schema change in schemas.py is sound; the new test covers the happy path. The manual TypeScript type change in types.gen.ts may be overwritten if the file is auto-generated, risking type drift. No other correctness issues identified. The diff relaxes _comment and install_instructions from str | None to Any | None on the import schema and corresponding TypeScript types. The fields are not persisted, so there is no data integrity risk. No SQL queries, transactions, ORM usage, schema migrations, or connection lifecycle changes are present. No N+1 or data-integrity concerns exist. No performance concerns in this diff. The change is a purely structural type relaxation to accept Any instead of str for two ignored fields — no loops, queries, renders, cache operations, memory allocations, or async patterns are modified. No frontend changes in this diff — schema adjustments (types.gen.ts) and backend test additions are outside the React/Next.js scope.

Findings

🟡 MEDIUM · likely · Correctnessapps/dashboard/src/lib/api/types.gen.ts:3040

Issue: The TypeScript type definitions for _comment and install_instructions were manually changed from string | null to unknown | null. If this file is auto-generated from an OpenAPI spec (as the name types.gen.ts suggests), manual edits will be overwritten the next time the spec is regenerated. This could lead to silent type mismatches if the generation script is not updated accordingly.

Suggestion: If the file is auto-generated, update the generation script or source (likely the backend schema) to produce the unknown type; regenerate the file and commit the generated output. If it's not generated, remove the .gen suffix or add a comment indicating it's hand-maintained.

Evidence:

The diff changes `_comment?: string | null;` to `_comment?: unknown | null;` and similarly for `install_instructions`. No accompanying change to any generation config or script is present.

🤖 Reviewed by the Code Review Council (openrouter:deepseek/deepseek-v4-pro) via .github/workflows/gemini-review.yml.

@rlagowski rlagowski merged commit 0e45d80 into develop Jun 8, 2026
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants