fix(workflow): support integration: auto to follow project's initialized AI#2421
fix(workflow): support integration: auto to follow project's initialized AI#2421Quratulain-bilal wants to merge 1 commit intogithub:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adjusts the speckit workflow so its integration input defaults to the project’s initialized AI integration (via .specify/integration.json) instead of being hardcoded to Copilot, aligning workflow execution with project initialization state.
Changes:
- Updated
workflows/speckit/workflow.ymlto usedefault: "auto"forinputs.integrationand clarified the prompt text. - Enhanced
WorkflowEngineinput resolution to interpret the"auto"default sentinel forintegrationby reading.specify/integration.json. - Added regression tests covering auto-resolution, explicit override behavior, missing file fallback, and malformed JSON fallback.
Show a summary per file
| File | Description |
|---|---|
| workflows/speckit/workflow.yml | Switches the workflow’s integration default to auto and documents the behavior in the input prompt. |
| src/specify_cli/workflows/engine.py | Implements "auto" default resolution for the integration input by reading .specify/integration.json. |
| tests/test_workflows.py | Adds tests validating the new default resolution and fallback/override behavior. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 3
| return None | ||
| try: | ||
| data = json.loads(path.read_text(encoding="utf-8")) | ||
| except (json.JSONDecodeError, OSError): |
There was a problem hiding this comment.
Path.read_text(encoding="utf-8") can raise UnicodeDecodeError for non-UTF8/invalidly-encoded files, which is not currently caught. This makes the "missing or malformed" integration.json path potentially crash the workflow engine; consider catching UnicodeDecodeError (or broader ValueError) alongside JSONDecodeError/OSError and treating it as malformed (return None).
| except (json.JSONDecodeError, OSError): | |
| except (json.JSONDecodeError, OSError, UnicodeDecodeError): |
| path = self.project_root / ".specify" / "integration.json" | ||
| if not path.is_file(): | ||
| return None |
There was a problem hiding this comment.
The integration state path is hardcoded as ".specify" / "integration.json" here, while other parts of the codebase also define and use an INTEGRATION_JSON constant for this location. To avoid drift/typos over time, consider centralizing the path constant in a small shared module (or at least a module-level constant in the workflows package) and reusing it here.
| default: "auto" | ||
| prompt: "Integration to use (e.g. claude, copilot, gemini; 'auto' uses the project's initialized integration)" |
There was a problem hiding this comment.
With inputs.integration.default now set to "auto" (which can resolve to the project’s initialized integration such as opencode), the workflow metadata under requires.integrations.any no longer accurately reflects the set of integrations the workflow can run with. Consider updating the requires.integrations declaration (e.g., include opencode or broaden it) so tooling/users don’t interpret the workflow as limited to only copilot/claude/gemini.
mnriem
left a comment
mnriem
left a comment
There was a problem hiding this comment.
Please address Copilot feedback
- Centralize the integration.json path as a module-level INTEGRATION_JSON constant in workflows/engine.py (mirrors specify_cli.INTEGRATION_JSON; cannot be imported directly without a circular dependency). - Catch UnicodeDecodeError alongside JSONDecodeError/OSError so a non-UTF8 integration.json falls back to the literal default instead of crashing the workflow engine. Adds a regression test. - Drop the narrow requires.integrations.any allowlist in the speckit workflow YAML; the four core commands (specify/plan/tasks/implement) are provided by every integration, so the previous list was always inaccurate (e.g. opencode, codex, etc. were excluded).
|
Thanks for the review @mnriem and @copilot — addressed all three points in 74dcaf3: 1. Centralized path constant — added a module-level 2. UnicodeDecodeError fallback — extended the 3. All 131 workflow tests pass locally. |
There was a problem hiding this comment.
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 3
| requires: | ||
| speckit_version: ">=0.7.2" | ||
| integrations: | ||
| any: ["copilot", "claude", "gemini"] | ||
| # The four commands used below (specify, plan, tasks, implement) are core | ||
| # spec-kit commands provided by every integration, so this workflow runs | ||
| # against whichever integration the project was initialized with. |
There was a problem hiding this comment.
The requires.integrations constraint was removed here, but the PR description only mentions changing the integration input default/prompt. If this removal is intentional, please update the requires metadata to explicitly reflect the intended compatibility (e.g., broaden the allowed integrations) rather than dropping the field entirely, since other tooling/docs may rely on it as a declarative compatibility signal.
| # Path to the file recording the project's initialized integration. Kept in | ||
| # sync with ``specify_cli.INTEGRATION_JSON``; the workflows package cannot | ||
| # import that constant directly without creating a circular import on the | ||
| # CLI module. | ||
| INTEGRATION_JSON = ".specify/integration.json" | ||
|
|
||
|
|
There was a problem hiding this comment.
This introduces a second INTEGRATION_JSON constant that must be manually kept in sync with specify_cli.INTEGRATION_JSON, which is easy to drift over time. Consider moving this path constant to a small shared module (importable by both the CLI entrypoint and workflows engine) to avoid duplication and future inconsistency.
| # Path to the file recording the project's initialized integration. Kept in | |
| # sync with ``specify_cli.INTEGRATION_JSON``; the workflows package cannot | |
| # import that constant directly without creating a circular import on the | |
| # CLI module. | |
| INTEGRATION_JSON = ".specify/integration.json" | |
| def _get_integration_json() -> str: | |
| """Return the shared integration metadata path. | |
| Resolve the value from ``specify_cli.INTEGRATION_JSON`` lazily so this | |
| module does not maintain its own independent source of truth. If that | |
| import path is unavailable during import-time initialization, fall back | |
| to the current default path to preserve existing behavior. | |
| """ | |
| try: | |
| return __import__("specify_cli", fromlist=["INTEGRATION_JSON"]).INTEGRATION_JSON | |
| except (ImportError, AttributeError): | |
| return ".specify/integration.json" | |
| INTEGRATION_JSON = _get_integration_json() |
| ) | ||
| elif "default" in input_def: | ||
| resolved[name] = input_def["default"] | ||
| resolved[name] = self._resolve_default(name, input_def["default"]) |
There was a problem hiding this comment.
_resolve_inputs() coerces/validates provided values via _coerce_input(), but defaults (including the newly project-resolved integration default) bypass type coercion and enum validation. This can yield invalid inputs values even when an input definition declares an enum/type. Consider running the resolved default through _coerce_input() as well so defaults are validated consistently with user-provided inputs.
| resolved[name] = self._resolve_default(name, input_def["default"]) | |
| default_value = self._resolve_default(name, input_def["default"]) | |
| resolved[name] = self._coerce_input( | |
| name, default_value, input_def | |
| ) |
|
Please address Copilot feedback. If not applicable, please explain why |
- Move INTEGRATION_JSON to a dedicated specify_cli._paths module so the CLI entrypoint and the workflows engine share a single source of truth instead of each carrying its own constant. specify_cli still re-exports INTEGRATION_JSON for backward compatibility. - Run the resolved default value through _coerce_input() so workflow defaults (including the project-resolved integration default) are validated against declared type/enum constraints, just like user-provided inputs. Adds two regression tests covering an enum violation and numeric coercion of a string default. - Restore requires.integrations as an explicit declarative compatibility signal, using the wildcard form ``any: "*"`` to accurately reflect that the workflow runs against any registered integration.
|
Round 2 — addressed in e9d946e: 1. Shared path constant (no duplication) — moved 2. Defaults now run through 3. Test results: |
There was a problem hiding this comment.
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 5
| elif "default" in input_def: | ||
| resolved[name] = input_def["default"] | ||
| default_value = self._resolve_default(name, input_def["default"]) | ||
| resolved[name] = self._coerce_input(name, default_value, input_def) | ||
| elif input_def.get("required", False): |
There was a problem hiding this comment.
_resolve_inputs now coerces/validates default values via _coerce_input, which is a user-visible behavior change (invalid defaults can now raise at runtime). Since validate_workflow() doesn’t currently validate defaults against declared types/enums, consider adding that validation there so workflows fail fast at install/validation time rather than during execution.
| def _load_project_integration(self) -> str | None: | ||
| """Read the active integration key from ``.specify/integration.json``. | ||
|
|
||
| Returns None when the file is missing or malformed; callers are | ||
| expected to fall back to a literal default. | ||
| """ | ||
| path = self.project_root / INTEGRATION_JSON | ||
| if not path.is_file(): | ||
| return None | ||
| try: | ||
| data = json.loads(path.read_text(encoding="utf-8")) | ||
| except (json.JSONDecodeError, OSError, UnicodeDecodeError): | ||
| return None | ||
| if not isinstance(data, dict): | ||
| return None | ||
| value = data.get("integration") | ||
| if isinstance(value, str) and value: | ||
| return value | ||
| return None |
There was a problem hiding this comment.
_load_project_integration() duplicates the integration.json parsing logic that already exists in specify_cli.__init__._read_integration_json, but with different error handling semantics (CLI exits loudly vs. workflow engine silently falls back). To avoid drift and keep behavior consistent, consider extracting a shared low-level helper (e.g., “try-read integration.json → dict|None”) into a common module and layering the CLI/engine-specific error handling on top.
| def test_integration_auto_default_uses_project_integration(self, project_dir): | ||
| """`integration: auto` should resolve to .specify/integration.json's integration.""" | ||
| from specify_cli.workflows.engine import WorkflowEngine, WorkflowDefinition | ||
|
|
There was a problem hiding this comment.
PR description mentions “4 new regression tests”, but this diff adds more than four new test cases in this block. Consider updating the PR description/test plan to match the actual test coverage so reviewers know the full scope.
| # core spec-kit commands provided by every integration, so this | ||
| # workflow is compatible with all of them. ``"*"`` is the declarative | ||
| # wildcard signaling "any registered integration". | ||
| any: "*" |
There was a problem hiding this comment.
requires.integrations.any is set to the string "*", but the workflow publishing docs/examples describe this field as a list of integration IDs (e.g. any: ["claude", "gemini"]) and there’s no evidence elsewhere in the repo that "*" is a supported sentinel. Consider keeping this as an explicit list (including opencode, etc.), or update the workflow schema/docs and any consumers to explicitly support a wildcard value so this doesn’t become a future validation/compatibility break.
| # core spec-kit commands provided by every integration, so this | |
| # workflow is compatible with all of them. ``"*"`` is the declarative | |
| # wildcard signaling "any registered integration". | |
| any: "*" | |
| # core spec-kit commands provided by the supported integrations listed | |
| # here, so keep this as an explicit list matching the documented schema | |
| # for `requires.integrations.any`. | |
| any: | |
| - "claude" | |
| - "copilot" | |
| - "gemini" | |
| - "opencode" |
| The CommandStep / dispatcher is responsible for surfacing a helpful | ||
| error in that case; the engine itself must not invent an integration. |
There was a problem hiding this comment.
The docstring here says the CommandStep/dispatcher will surface a helpful error when the project integration state is missing, but the current CommandStep error for an unknown integration would read like “integration 'auto' CLI not found…”, which isn’t specific to missing .specify/integration.json. Either adjust this comment to reflect current behavior, or add the promised special-cased error message elsewhere so the statement stays true.
| The CommandStep / dispatcher is responsible for surfacing a helpful | |
| error in that case; the engine itself must not invent an integration. | |
| The engine itself must not invent an integration when | |
| `.specify/integration.json` is absent; any later validation or command | |
| resolution will handle an unresolved `"auto"` value. |
|
Please address Copilot feedback. If not applicable, please explain why |
- workflows/speckit/workflow.yml: replace the wildcard ``any: "*"`` with an explicit list ``[claude, copilot, gemini, opencode]`` matching the documented schema for ``requires.integrations.any``. Comment notes the list is a non-exhaustive compatibility hint. - engine.validate_workflow: validate input defaults at authoring time so enum/type mismatches in workflow YAML surface during install/validation instead of at execution. The ``integration: auto`` sentinel is exempt because it is resolved against project state at runtime. - Extract a shared low-level helper ``try_read_integration_json`` into ``specify_cli._paths`` and use it from the workflow engine. Keeps parsing rules consistent between the CLI's loud loader and the engine's silent loader; CLI ``_read_integration_json`` retains its diagnostic semantics layered on top of the same parsing surface. - Drop the now-unused INTEGRATION_JSON import in engine.py. - Update test docstring to reflect actual engine behavior on missing integration state (no special-cased dispatcher error message exists). - Add 2 regression tests for validate_workflow's new default checks.
|
Round 3 — addressed in 67e4d1f. Walking through each Copilot point: 1. 2. 3. 4. Test docstring overstates dispatcher behavior — applied the suggested wording change. The docstring now accurately states that the engine must not invent an integration; downstream resolution handles an unresolved 5. PR description test count drifted — updated the PR body to match the actual coverage (8 new regression tests, full breakdown in the "Fix" section). Test results: 135 passed in |
There was a problem hiding this comment.
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
src/specify_cli/workflows/engine.py:741
_resolve_inputs()always runs resolved defaults through_coerce_input(). Forintegration: autowith anenumthat does not include"auto", if.specify/integration.jsonis missing/malformed then_resolve_default()returns the literal"auto"and_coerce_input()will raise a ValueError during input resolution. This is inconsistent with the validation-time exemption forintegration: autoand with the intended "fall back to literal default" behavior. Consider exempting enum validation for the"auto"sentinel at runtime (or skipping coercion until afterautohas resolved to a concrete integration).
elif "default" in input_def:
default_value = self._resolve_default(name, input_def["default"])
resolved[name] = self._coerce_input(name, default_value, input_def)
elif input_def.get("required", False):
- Files reviewed: 5/5 changed files
- Comments generated: 1
| # input is a runtime-resolved sentinel and is exempt. | ||
| if "default" in input_def: | ||
| default_value = input_def["default"] | ||
| is_auto_integration = ( | ||
| input_name == "integration" and default_value == "auto" | ||
| ) | ||
| if not is_auto_integration: | ||
| try: | ||
| WorkflowEngine._coerce_input( | ||
| input_name, default_value, input_def | ||
| ) | ||
| except ValueError as exc: | ||
| errors.append( | ||
| f"Input {input_name!r} has invalid default: {exc}" | ||
| ) |
There was a problem hiding this comment.
validate_workflow() fully skips default validation when input_name == "integration" and the default is the sentinel "auto". This means a workflow can declare integration with an incompatible type (e.g., number/boolean) and still pass validation, but will then fail at runtime when inputs are resolved/coerced. Consider still validating/coercing the default's declared type for this case (e.g., require type: string) while exempting only the enum-membership check for the "auto" sentinel.
This issue also appears on line 738 of the same file.
| # input is a runtime-resolved sentinel and is exempt. | |
| if "default" in input_def: | |
| default_value = input_def["default"] | |
| is_auto_integration = ( | |
| input_name == "integration" and default_value == "auto" | |
| ) | |
| if not is_auto_integration: | |
| try: | |
| WorkflowEngine._coerce_input( | |
| input_name, default_value, input_def | |
| ) | |
| except ValueError as exc: | |
| errors.append( | |
| f"Input {input_name!r} has invalid default: {exc}" | |
| ) | |
| # input is a runtime-resolved sentinel, so only enum-membership | |
| # validation is exempted for that exact case. | |
| if "default" in input_def: | |
| default_value = input_def["default"] | |
| is_auto_integration = ( | |
| input_name == "integration" and default_value == "auto" | |
| ) | |
| try: | |
| validation_input_def = input_def | |
| if is_auto_integration and "enum" in input_def: | |
| validation_input_def = { | |
| key: value | |
| for key, value in input_def.items() | |
| if key != "enum" | |
| } | |
| WorkflowEngine._coerce_input( | |
| input_name, default_value, validation_input_def | |
| ) | |
| except ValueError as exc: | |
| errors.append( | |
| f"Input {input_name!r} has invalid default: {exc}" | |
| ) |
|
Please address Copilot feedback. If not applicable, please explain wh |
…#2421 Previously the ``integration: auto`` exemption skipped default validation entirely, which meant a workflow could declare an incompatible type (e.g. ``type: number`` with ``default: "auto"``) and still pass validation, only to fail later at runtime. Both the install-time check in ``validate_workflow()`` and the runtime coercion in ``_resolve_inputs()`` now exempt only the enum-membership check for the auto sentinel; the declared type is still enforced. Adds a regression test that asserts a ``type: number`` + ``default: "auto"`` workflow is rejected at validation time.
|
Round 4 — addressed in 0dd3426. Good catch. Tightened the Added a regression test that asserts a workflow declaring Test results: 136 passed in |
There was a problem hiding this comment.
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
src/specify_cli/init.py:1901
_read_integration_json()still re-implements JSON parsing directly. Since this PR introducestry_read_integration_json()as the shared low-level loader (including handlingUnicodeDecodeErrorfor non-UTF8 files), consider using that helper here (and layering the CLI’s user-facing diagnostics on top) to avoid drift and to ensure non-UTF8integration.jsondoesn’t crash the CLI with an uncaught decode error.
from ._paths import INTEGRATION_JSON # noqa: E402 (re-export for backward compat)
def _read_integration_json(project_root: Path) -> dict[str, Any]:
"""Load ``.specify/integration.json``. Returns ``{}`` when missing."""
- Files reviewed: 5/5 changed files
- Comments generated: 1
| @@ -9,7 +9,17 @@ workflow: | |||
| requires: | |||
| speckit_version: ">=0.7.2" | |||
There was a problem hiding this comment.
requires.speckit_version is still ">=0.7.2", but this workflow now relies on the new runtime behavior where inputs.integration default "auto" is resolved by the workflow engine. To avoid older Spec Kit versions pulling this workflow from the default catalog and then failing at runtime (treating "auto" as a literal integration key), please bump the minimum required Spec Kit version to the first release that includes the integration: auto sentinel support.
| speckit_version: ">=0.7.2" | |
| speckit_version: ">=0.8.0" |
There was a problem hiding this comment.
You'll have to bump it to the next version we are going to release
…thub#2421 - workflows/speckit/workflow.yml: bump ``requires.speckit_version`` from ``>=0.7.2`` to ``>=0.8.3`` so older spec-kit versions, which lack the engine-side resolution of the ``integration: "auto"`` sentinel, do not pull this workflow from the catalog and then fail by treating "auto" as a literal integration key. Adds an inline comment explaining the reason for the floor. - src/specify_cli/__init__.py: ``_read_integration_json`` now also catches ``UnicodeDecodeError`` (non-UTF8 file) so the CLI fails with a targeted, actionable diagnostic instead of an uncaught traceback, and notes in the docstring that the function shares its low-level parsing surface with ``_paths.try_read_integration_json`` while keeping loud per-cause messaging on top.
|
Round 5 — addressed in ae4c94d. 1. 2. I deliberately kept Test results: 136 passed in |
There was a problem hiding this comment.
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
…zed AI Closes github#2406 (squashed)
Quratulain-bilal
force-pushed
the
fix/workflow-integration-auto-default
branch
from
ae4c94d to
2886441
Compare
Closes #2406
Problem
The
speckitworkflow'sintegrationinput haddefault: "copilot"hardcoded inworkflows/speckit/workflow.yml. This meant projects initialized withclaudeorgeminiwould silently fall back to copilot when running the workflow without an explicit--input integration=...flag, breaking the principle of least surprise.Fix
workflows/speckit/workflow.yml: changedefault: "copilot"→default: "auto", update the prompt to describe the new behavior, and broadenrequires.integrations.anyto reflect that the workflow's four core commands (specify/plan/tasks/implement) are provided by every integration.src/specify_cli/_paths.py(new): single-source-of-truth forINTEGRATION_JSONand a shared low-leveltry_read_integration_jsonhelper used by both the CLI and the workflow engine.src/specify_cli/workflows/engine.py:_resolve_default()so when an input default is the sentinel string"auto"and the input name is"integration", the engine reads.specify/integration.json(via the shared helper) and uses the project's initialized integration. Falls back to the literal"auto"when the file is missing or malformed._coerce_input()so workflow defaults are validated against declaredtype/enumconstraints, just like user-provided inputs.validate_workflow()to validate input defaults at authoring time so enum/type mismatches surface during install/validation rather than at execution. Theintegration: autosentinel is exempt because it is runtime-resolved against project state.src/specify_cli/__init__.py: re-exportINTEGRATION_JSONfrom_pathsfor backward compatibility.tests/test_workflows.py: 8 new regression tests covering the auto-resolve happy path, the missing-file fallback, the explicit-input override, the malformed JSON path, the non-UTF8 path, default coercion, default enum validation, and thevalidate_workflowexemption forauto.The change is opt-in via the YAML default, so existing workflows that hardcode
default: "copilot"(or any other literal) keep working unchanged.Test plan
1732 passed, 125 skipped)speckitworkflow respects.specify/integration.jsonwhen no--inputis given--input integration=copilotstill overrides the project default.specify/integration.jsondoes not crash the enginevalidate_workflow()rejects defaults that violate declared enum/type