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
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
|
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. |
Restore local component payloads, exact registry metadata and hook state on failed updates or removals. Scope workflow consumers to their project and bind bundle step pins to the downloaded package. Cover partial mutations and dependent workflow restoration with regressions. Assisted-by: GitHub Copilot (model: GPT-6 Astra, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Assisted-by: GitHub Copilot (model: GPT-6 Astra, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Posted on behalf of @marcelsafin by GitHub Copilot (GPT-6 Astra). |
There was a problem hiding this comment.
🟡 Changes recommended
Rollback omits historical integration artifacts, while custom-step loading remains vulnerable to concurrency and failed cache deletion.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 4
- Review effort level: Balanced
Give each workflow operation a private step registry and keep package imports alive for each loaded instance. Refuse cache invalidation failures. Capture and restore generated command and skill artifacts across historical integrations, including absent outputs and legacy projects. Reuse native registrar paths and cover concurrency, lifecycle, I/O failure and integration history regressions. Assisted-by: GitHub Copilot (model: GPT-6 Astra, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Posted on behalf of @marcelsafin by GitHub Copilot (model: GPT-6 Astra, autonomous).
|
There was a problem hiding this comment.
🟡 Changes recommended
External bytecode caches can remain stale, and extension rollback leaves unintended backup state.
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.
src/specify_cli/bundler/services/primitives.py:365
- Calling the normal extension removal path during rollback creates
.specify/extensions/.backup/<id>from the failed/new installation. The subsequent non-force install does not consume that directory (did_removeis false), so a successful rollback leaves backup state that did not exist before; after a failed refresh it can contain the replacement version's config files. Snapshot and restore (or clean up) this backup directory as part of the extension preimage so rollback is exact and stale config cannot survive.
- Files reviewed: 21/21 changed files
- Comments generated: 1
- Review effort level: Balanced
| for cache_dir in step_dir.rglob("__pycache__"): | ||
| if cache_dir.is_symlink(): | ||
| raise OSError(f"Refusing symlinked bytecode cache: {cache_dir}") | ||
| if cache_dir.is_dir(): | ||
| _shutil.rmtree(cache_dir) | ||
| _importlib.invalidate_caches() |
Invalidate source-derived Python caches outside custom packages as well as in-package caches. Include extension configuration backup paths in existing component snapshots, preserving prior files, directories and absence. Cover equal-size/equal-mtime reloads, cache deletion failures, nine real rollback histories and backup I/O failures. Assisted-by: GitHub Copilot (model: GPT-6 Astra, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Posted on behalf of @marcelsafin by GitHub Copilot (model: GPT-6 Astra, autonomous).
|
Description
Consolidate five related bundle and workflow-step integrity fixes at maintainer
request, with follow-up coverage for partial failures and catalog changes:
use the fixed built-in type set for metadata commands and collision checks.
Concurrent operations use private registries and instance-owned package
namespaces, including recursive validation and later relative imports.
Invalidate source-derived bytecode paths, including configured external
caches; refuse packages whose caches cannot be safely invalidated.
across different kinds.
user configuration, and extension hooks when bundle update/removal or
provenance saving fails. Recovery includes interrupted primitives, restores
steps before dependent workflows, and reports incomplete rollback explicitly.
Preserve generated command/skill artifacts across historical integrations,
including aliases, supporting files, home-scoped outputs, and prior absence.
Preserve the extension's native configuration backup preimage as well.
compare the downloaded package directly against the expected version.
Rollback uses temporary local snapshots, not a catalog re-download; it is
best-effort, not a crash-recovery or whole-project atomicity guarantee.
This preserves the original fix commits and supersedes #4466, #4467, #4468,
and #4469. The explicit-refresh feature in #4477 remains separate.
Testing
.venv/bin/specify --helpuv sync --extra test, using the worktree's.venv/bin/python -m pytest tests -q --tb=shortValidation on
93cd4052182fe05729fad72e730108fed2151798, including currentupstream without rewriting reviewed commits:
and real PowerShell 7.6.6 available.
state, interrupted mutations, failed restoration, dependent workflows, and
catalog/package races, concurrent loading and module lifetime, cache
invalidation failures (including external caches), historical integration
artifact restoration, and exact extension backup preimages.
commit.gpgsign=falseoverride, preserving other inherited Git settings; repository/global
configuration files are unchanged.
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 offworkflow step add, andgit diff --check upstream/main...HEAD.AI Disclosure
GitHub Copilot (GPT-5.6 Sol) produced the initial fixes and consolidation.
GitHub Copilot (GPT-6 Astra) autonomously reproduced and repaired the follow-up
correctness findings, added regressions, and ran verification under
@marcelsafin's direction; the user explicitly approved commit and publication.