Skip to content

fix(bundler): treat a blank active integration as indeterminate in the FR-019 guard - #3886

Open
jawwad-ali wants to merge 1 commit into
github:mainfrom
jawwad-ali:fix/bundler-blank-integration-guard
Open

fix(bundler): treat a blank active integration as indeterminate in the FR-019 guard#3886
jawwad-ali wants to merge 1 commit into
github:mainfrom
jawwad-ali:fix/bundler-blank-integration-guard

Conversation

@jawwad-ali

Copy link
Copy Markdown
Contributor

Problem

resolve_install_plan in src/specify_cli/bundler/services/resolver.py enforces FR-019 with two guards:

if active_integration and required != active_integration:      # clash        (truthiness)
if active_integration is None and not integration_explicit:    # indeterminate (identity)

An empty string satisfies neither — it is falsy, so the first is skipped; it is not None, so the second is skipped. Execution falls through to effective_integration = required, silently adopting the bundle's pinned integration.

That is precisely what the function's own docstring says it prevents:

…resolution fails instead of silently adopting the bundle's required integration (FR-019 guard).

Reproduction on current main (81bf741)

Same manifest, pinned to copilot:

active=None         -> BundlerError: ... active integration could not be determined
active='' (blank)   -> effective_integration='copilot'   (NO ERROR)   <-- bug
active='claude'     -> BundlerError: ... targets integration 'copilot'

Both a known-different integration and an unknown one halt. Only the blank one silently proceeds — and it proceeds by adopting a different integration than the project has, which is the one outcome FR-019 exists to block.

Fix

Normalise blank to None before the guards, stripping first:

if active_integration is not None:
    active_integration = active_integration.strip() or None

The strip matches the writer side. integration_state.clean_integration_key is the canonical normaliser:

def clean_integration_key(key: Any) -> str | None:
    """Return a stripped integration key, or None for empty/non-string values."""
    if not isinstance(key, str) or not key.strip():
        return None
    return key.strip()

Without stripping, " claude " would be reported as clashing with claude — i.e. with itself. A new test pins that.

No breaking change. None and any non-blank value behave exactly as before. The only inputs whose behaviour changes are blank/whitespace ones, which move from "silently adopt a foreign integration" to the existing could not be determined error — the same path None already takes. --integration with an explicit value still overrides, unchanged.

Verification

  • Fail-before / pass-after: the four new cases fail on unpatched src and pass with the fix. File: 4 failed → 12 passed.
  • Scoped regression over tests/unit: failure set identical to the clean-main baseline captured on 81bf741 (2 pre-existing in scope).
  • uvx ruff@0.15.0 check src tests → clean

Tests sit beside test_pinned_integration_with_indeterminate_active_fails, the existing FR-019 guard test asserting the same could not be determined message.


Written with assistance from Claude Code. Bug found, reproduced, and verified by me on current main.

…-019

`resolve_install_plan`'s two FR-019 guards are a truthiness test and an
`is None` test:

    if active_integration and required != active_integration:   # clash
    if active_integration is None and not integration_explicit: # indeterminate

An empty string satisfies neither, so it falls through to
`effective_integration = required` and the bundle's pinned integration is
silently adopted — the exact outcome the docstring says the guard prevents
("resolution fails instead of silently adopting the bundle's required
integration").

  active=None        -> BundlerError: ... could not be determined
  active='' (blank)  -> effective_integration='copilot'   <-- silent adopt
  active='claude'    -> BundlerError: ... targets integration 'copilot'

Normalise a blank value to None before the guards, and strip first to
match the writer (`integration_state.clean_integration_key`, which returns
`None` for empty/whitespace and strips otherwise) so a padded value is not
reported as clashing with its own unpadded form.

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

Copilot AI 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.

Pull request overview

Fixes FR-019 enforcement by treating blank or whitespace-only active integrations as indeterminate.

Changes:

  • Normalizes active integration values before compatibility checks.
  • Adds coverage for blank, whitespace, and padded integration values.
Show a summary per file
File Description
src/specify_cli/bundler/services/resolver.py Normalizes blank and padded integration IDs.
tests/unit/test_bundler_resolver.py Tests blank rejection and padded-value matching.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Balanced

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.

3 participants