fix: harden bundle and workflow step integrity - #4470
Conversation
Assisted-by: GitHub Copilot (model: gpt-5.6-sol, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Approval recommended
The reviewed changes have appropriate regression coverage and no unresolved issues.
Pull request overview
Enforces bundle-declared version pins when installing workflow steps.
Changes:
- Validates requested step versions against catalog metadata.
- Adds regression coverage ensuring mismatches prevent installation.
File summaries
| File | Description |
|---|---|
tests/unit/test_bundler_primitives.py |
Tests rejection of mismatched step versions. |
src/specify_cli/bundler/services/primitives.py |
Adds step version-pin validation before installation. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
This is excellent work — you've filed a focused, well-tested set of bundle-integrity fixes (#4466–#4470: component scoping, duplicate rejection, step-version mismatch, record-save rollback, and version-pin enforcement), all green with regression coverage and clear disclosure. Genuinely high-value stuff. One process note: that's 5 open PRs, past the 3-open-PR guidance in CONTRIBUTING, and since they're all the same theme touching the bundler, they'd be much faster to review as a single consolidated PR (or a smaller stack) rather than five separate ones. Could you group the related ones? I want to land these — consolidating just helps them move quicker. Marking this one |
Assisted-by: GitHub Copilot (model: gpt-5.6-sol, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Assisted-by: GitHub Copilot (model: gpt-5.6-sol, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Assisted-by: GitHub Copilot (model: gpt-5.6-sol, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Assisted-by: GitHub Copilot (model: gpt-5.6-sol, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Assisted-by: GitHub Copilot (model: gpt-5.6-sol, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Assisted-by: GitHub Copilot (model: gpt-5.6-sol, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Assisted-by: GitHub Copilot (model: gpt-5.6-sol, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Assisted-by: GitHub Copilot (model: gpt-5.6-sol, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Assisted-by: GitHub Copilot (model: gpt-5.6-sol, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Consolidated all five fixes here on |
There was a problem hiding this comment.
🟡 Changes recommended
Missing package versions and catalog lookup failures can still bypass the new version-integrity checks.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 2
- Review effort level: Balanced
Assisted-by: GitHub Copilot (model: gpt-5.6-sol, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Assisted-by: GitHub Copilot (model: gpt-5.6-sol, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Addressed both current-head review findings in |
There was a problem hiding this comment.
🟡 Changes recommended
Provenance-save failures during bundle updates can leave refreshed or removed components inconsistent with the retained record.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
tests/unit/test_bundler_primitives.py:123
- The new step-pin tests cover only rejection paths (mismatch, absent metadata, and lookup failure). Add a matching catalog-version case that asserts
workflow_step_addis invoked, so a regression that rejects every valid pinned step cannot satisfy this test group.
- Files reviewed: 11/11 changed files
- Comments generated: 2
- Review effort level: Balanced
Assisted-by: GitHub Copilot (model: gpt-5.6-sol, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Assisted-by: GitHub Copilot (model: gpt-5.6-sol, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@marcelsafin — GitHub Copilot (GPT-5.6 Sol) addressed the latest review round
Validation on this exact SHA: 1,099 affected-file tests passed; full suite |
There was a problem hiding this comment.
🟡 Changes recommended
Direct registry consumers can still leak project A’s custom step types into project B without rescanning.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 1
- Review effort level: Balanced
| # Custom steps are project-scoped even though the registry and Python module | ||
| # cache are process-global. Clear the previous project's classes and package | ||
| # modules before every scan so removed or updated code cannot remain active. | ||
| for _type_key in tuple(STEP_REGISTRY): | ||
| if _type_key not in BUILTIN_STEP_TYPES: |
There was a problem hiding this comment.
🟡 Changes recommended
Rollback can misreport restoration, removal rollback remains incomplete, and project/version isolation has unresolved gaps.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
src/specify_cli/workflows/init.py:105
- Clearing stale entries only when
load_custom_stepsis called leaves other step commands project-unsafe in a long-lived process. After scanning project A,workflow_step_addin project B still checks the stale global registry and can reject A's custom ID as a built-in;workflow_step_list/infocan similarly report it as built-in. Update built-in-only consumers to useBUILTIN_STEP_TYPES(and ensure project-dependent consumers load the current root).
# Custom steps are project-scoped even though the registry and Python module
# cache are process-global. Clear the previous project's classes and package
# modules before every scan so removed or updated code cannot remain active.
for _type_key in tuple(STEP_REGISTRY):
if _type_key not in BUILTIN_STEP_TYPES:
- Files reviewed: 13/13 changed files
- Comments generated: 3
- Review effort level: Balanced
| Atomicity is scoped, not global: completed component mutations are reversed | ||
| on failure, and the provenance record is written solely on full success. | ||
| Rollback is best-effort because primitive restoration can itself fail. |
| metadata = metadata or {} | ||
| actual_version = version.strip() if isinstance(version, str) else None | ||
| source = metadata.get("source") | ||
| priority = metadata.get("priority") | ||
| return ComponentRef( |
| f"is disabled; re-run without --offline or install it first with " | ||
| f"'specify workflow step add {component.id}'." | ||
| ) | ||
| self._assert_pinned_version(component) |
|
Thanks @marcelsafin — solid direction and good to see CI green across the matrix. Before I do a full review of the rollback/registry logic, there are four open correctness threads that go to the heart of what this PR hardens; CI passes because these are partial-failure/TOCTOU paths the happy-path tests don't exercise:
Once those are closed, re-request a review and I'll take the full pass. Heads-up: this overlaps #4477 on the rollback path, so whichever lands first, the other will need a rebase to the non-overlapping hunks. |
Description
Consolidate five related bundle and workflow-step integrity fixes at maintainer
request:
project, including same-path reloads that could otherwise reuse stale
bytecode.
preserving valid identical IDs across different kinds.
catalog metadata.
provenance fails, using actual installed metadata rather than bundle-record
pins and reporting incomplete restoration explicitly.
Each behavior has focused regression coverage and remains in a separate commit.
This PR supersedes #4466, #4467, #4468, and #4469.
Testing
uv run specify --helpuv sync && uv run pytestValidation on the consolidated current head:
pwsh, which is unavailablelocally and fails identically on unchanged upstream.
uvx ruff@0.15.0 check src testsuvx --from pip-audit==2.10.0 pip-audit --disable-pip --require-hashes -r .github/security-audit-requirements.txt --progress-spinner offgit diff --check upstream/main...HEADupstream/main.AI Disclosure
GitHub Copilot (GPT-5.6 Sol) autonomously reproduced the bugs, wrote the
regression tests and implementations, consolidated the reviewed commits, and
ran verification under @marcelsafin's direction and review.