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 6 commits into
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
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.
| ) | ||
| 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
|
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
|
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
…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.
|
happy to address any further feedback whenever you have a chance to review. CI is green and all review threads are resolved. |
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: 4/4 changed files
- Comments generated: 1
|
Please addres Copilot feedback. If not applicable, please explain why |
Address Copilot feedback: UnicodeDecodeError can be raised by both read_text() and json.loads(), so combining the handlers ensures both cases produce a consistent, clear error message.
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: 4/4 changed files
- Comments generated: 2
|
Please address Copilot feedback |
… in 'integration: auto' Three Copilot follow-ups on PR github#2421: 1. engine.py:799 — `_load_project_integration` was bypassing the same schema guard `_read_integration_json` enforces. It now reads the schema field directly, returns None on a future schema (so the workflow falls back to the literal 'auto' default rather than guessing), and routes through `normalize_integration_state` / `default_integration_key` so modern installs that record `default_integration` / `installed_integrations` (without the legacy top-level `integration` field) resolve correctly. 2. test_workflows.py — added two regression cases: - `integration: auto` resolves a modern normalized state file - `integration: auto` falls back when the state file declares a newer `integration_state_schema` than this CLI supports 3. test_cli.py — added a CLI-level regression for the `UnicodeDecodeError` branch in `_read_integration_json` to match the existing malformed-JSON coverage.
| # 0.8.3 is the first release with engine-side resolution of the | ||
| # ``integration: "auto"`` default. Older versions would treat "auto" | ||
| # as a literal integration key and fail at dispatch. |
| # The four commands below (specify, plan, tasks, implement) are core | ||
| # spec-kit commands provided by every integration. The list here is an | ||
| # explicit non-exhaustive compatibility hint following the documented | ||
| # ``any: [...]`` schema; the workflow is intended to run against any | ||
| # integration the project was initialized with, including ones not | ||
| # listed below. |
| # Validate the default eagerly so authoring mistakes (e.g. a | ||
| # default not in the declared enum, or a non-numeric default for | ||
| # a number input) surface at install/validation time instead of | ||
| # at workflow-execution time. ``"auto"`` for the integration | ||
| # input is a runtime-resolved sentinel, so only the | ||
| # enum-membership check is exempted for that exact case — the | ||
| # declared type is still enforced (e.g. ``type: number`` paired | ||
| # with ``default: "auto"`` is still rejected). | ||
| if "default" in input_def: | ||
| default_value = input_def["default"] | ||
| is_auto_integration = ( | ||
| input_name == "integration" and default_value == "auto" | ||
| ) | ||
| validation_input_def: dict[str, Any] = 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" | ||
| } | ||
| try: | ||
| WorkflowEngine._coerce_input( | ||
| input_name, default_value, validation_input_def | ||
| ) |
| def _load_project_integration(self) -> str | None: | ||
| """Read the default integration key from ``.specify/integration.json``. | ||
|
|
||
| Honors the same schema guard as ``_read_integration_json`` (rejects | ||
| files whose ``integration_state_schema`` is newer than this CLI | ||
| supports) and reads the canonical normalized state, so modern | ||
| installs that record ``default_integration`` / ``installed_integrations`` | ||
| resolve correctly under ``integration: auto``. Returns None when the | ||
| file is missing, malformed, or written by a newer CLI; 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 |
mnriem
left a comment
mnriem
left a comment
There was a problem hiding this comment.
Please address Copilot feedback
Address Copilot review on PR github#2421: Both `_read_integration_json` (CLI) and `_load_project_integration` (workflow engine) were parsing `.specify/integration.json` independently, duplicating the schema guard and risking drift between the two readers. Extract the parse + schema validation into a single low-level helper `try_read_integration_json` in `integration_state.py` that returns either the normalized state or a structured `IntegrationReadError`. Both callers now delegate to this helper: - CLI keeps its loud-fail UX: each error kind ("decode", "os", "not_object", "schema_too_new") is translated into the existing console message + typer.Exit(1). - Engine keeps its silent fallback: any error simply returns None so `integration: auto` falls back to the workflow's literal default. This eliminates the divergence Copilot flagged without changing observable behavior for either caller.
| # 0.8.3 is the first release with engine-side resolution of the | ||
| # ``integration: "auto"`` default. Older versions would treat "auto" | ||
| # as a literal integration key and fail at dispatch. |
| # Validate the default eagerly so authoring mistakes (e.g. a | ||
| # default not in the declared enum, or a non-numeric default for | ||
| # a number input) surface at install/validation time instead of | ||
| # at workflow-execution time. ``"auto"`` for the integration | ||
| # input is a runtime-resolved sentinel, so only the | ||
| # enum-membership check is exempted for that exact case — the | ||
| # declared type is still enforced (e.g. ``type: number`` paired | ||
| # with ``default: "auto"`` is still rejected). | ||
| if "default" in input_def: | ||
| default_value = input_def["default"] | ||
| is_auto_integration = ( | ||
| input_name == "integration" and default_value == "auto" | ||
| ) | ||
| validation_input_def: dict[str, Any] = 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" | ||
| } | ||
| try: | ||
| WorkflowEngine._coerce_input( | ||
| input_name, default_value, validation_input_def | ||
| ) |
| def try_read_integration_json( | ||
| project_root: Path, | ||
| ) -> tuple[dict[str, Any] | None, IntegrationReadError | None]: | ||
| """Parse ``.specify/integration.json`` without raising. | ||
|
|
||
| Returns ``(normalized_state, None)`` on success, ``(None, None)`` when the | ||
| file does not exist, or ``(None, error)`` for any parse / validation | ||
| failure. This is the single low-level reader; both the CLI's loud | ||
| ``_read_integration_json`` and the workflow engine's silent | ||
| ``_load_project_integration`` consume it so the schema guard and parse | ||
| logic cannot drift between them. | ||
| """ | ||
| path = project_root / INTEGRATION_JSON | ||
| if not path.is_file(): | ||
| return None, None |
There was a problem hiding this comment.
you're right that the description was stale. The single source of truth for INTEGRATION_JSON and
try_read_integration_json is src/specify_cli/integration_state.py (which has been the canonical home for
integration-state helpers since write_integration_json, normalize_integration_state, etc. were extracted earlier).
There is no _paths.py; I've updated the PR description to reflect that. Thanks!
Quratulain-bilal
changed the title
| if not path.is_file(): | ||
| return None, None |
|
Please address Copilot feedback |
Summary
Adds support for
integration: autoin workflow YAML so workflows dispatch to the AI agent the project was actuallyinitialized with, instead of a value baked into the workflow file.
Problem
Workflow YAML steps currently bake the integration into the file (
integration: claude). When a user initializes aproject with a different agent (e.g., Copilot), they have to manually edit every workflow file. There was no way to say
"use whatever the project was initialized with."
Solution
In
workflows/engine.py::_resolve_default, when a step'sintegrationinput is set to the sentinel"auto", theengine now reads the default integration key from
.specify/integration.jsonand substitutes it. If the state file ismissing, malformed, or written by a newer CLI, the resolver returns
Noneand the workflow falls back to whateverliteral value the YAML specifies.
workflows/speckit/workflow.ymlis updated to useintegration: autoso the bundled SDD workflow works out-of-the-boxwith any initialized agent.
Shared low-level reader
To avoid drift between the CLI's loud-fail
_read_integration_jsonand the engine's silent fallback_load_project_integration, both readers now delegate to a single low-level helper:integration_state.try_read_integration_json(project_root)— pure parse + schema-guard, returns either normalizedstate or a structured
IntegrationReadError(no side effects, no console output)_read_integration_json) — translates each error kind (decode,os,not_object,schema_too_new) intoits existing console message +
typer.Exit(1)_load_project_integration) — treats any error as a silent fallback to the workflow's literal defaultINTEGRATION_JSON,INTEGRATION_STATE_SCHEMA, the new helper, and all related state helpers live together insrc/specify_cli/integration_state.py— the canonical home for integration state.Tests
tests/test_workflows.py— 8 new tests coveringintegration: autoresolution across:default_integration+installed_integrations)integration.jsontests/integrations/test_cli.py— CLI-level test forUnicodeDecodeErroron malformedintegration.jsonFull test suite: 2782 passed, 136 skipped, 0 failures.
Files changed
src/specify_cli/integration_state.py—try_read_integration_json+IntegrationReadErrorsrc/specify_cli/__init__.py—_read_integration_jsondelegates to shared helpersrc/specify_cli/workflows/engine.py—_resolve_default+_load_project_integrationhonorintegration: autoworkflows/speckit/workflow.yml— opt intointegration: autotests/test_workflows.py,tests/integrations/test_cli.py— coverage