From 9a98075f2f38d25beaabcdaf5e81d47d54c07c23 Mon Sep 17 00:00:00 2001 From: Sukanth Gunda Date: Thu, 5 Nov 2026 01:52:48 -0600 Subject: [PATCH 1/3] feat(workflows): add per-step reasoning effort and context size settings Workflow command and prompt steps can set reasoning_effort and context_size, and workflow.reasoning_effort and workflow.context_size set defaults for both. Values resolve like model: the step value, then the workflow default, then unset. An expression that resolves to an empty string or null unsets the setting for that step. Values must be strings that match a host-owned, flag-safe pattern. Malformed values are summarized rather than echoed. An integration receives a setting only if it declares the setting on the class that implements the receiving method (build_exec_args, or dispatch_command when overridden) and translates it. Otherwise the step fails before the CLI runs, with an error that names the setting and its source. The base and format-base builders raise rather than drop a setting. With both settings unset, argv and integration keyword arguments are unchanged. Step outputs record both values. Part of #4916. Assisted-by: GitHub Copilot (model: Claude Opus 5.5, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- design/integration.md | 72 +- docs/integrations/cli.md | 7 + docs/workflow-steps/api.md | 2 + docs/workflow-steps/types.md | 93 +- docs/workflows/api.md | 3 +- src/specify_cli/integrations/base.py | 302 ++++ src/specify_cli/workflows/_execution.py | 2 + src/specify_cli/workflows/base.py | 6 + src/specify_cli/workflows/engine.py | 18 + .../workflows/step/_dispatch_settings.py | 129 ++ .../workflows/step/command/__init__.py | 42 +- .../workflows/step/prompt/__init__.py | 66 +- tests/integrations/test_base.py | 623 ++++++++ .../integrations/test_installed_adapters.py | 223 +++ tests/workflows/test_dispatch_settings.py | 1366 +++++++++++++++++ 15 files changed, 2935 insertions(+), 19 deletions(-) create mode 100644 src/specify_cli/workflows/step/_dispatch_settings.py create mode 100644 tests/workflows/test_dispatch_settings.py diff --git a/design/integration.md b/design/integration.md index e5d312827f..8076ffeba2 100644 --- a/design/integration.md +++ b/design/integration.md @@ -41,13 +41,74 @@ Hyphenated keys use underscores in package names. Its class declares: Import and `_register()` the class in `src/specify_cli/integrations/__init__.py` (both lists alphabetically). This registry is the source of built-in Python integration behavior. If the agent supports non-interactive workflows, implement -`build_exec_args()` with the full base signature; `options()` declares -install-time `--integration-options`, not per-workflow runtime options. +`build_exec_args()` with the full base signature. The `reasoning_effort` and +`context_size` keywords are required only for the settings it translates; accept +an untranslated one only to reject it through `check_dispatch_settings()` (see +[Per-step dispatch settings](#per-step-dispatch-settings)). `options()` +declares install-time `--integration-options`, not per-workflow runtime options. Agent-specific native events can be declared on the integration. Set `multi_install_safe = True` only when the integration emits no files or uses a static, non-overlapping agent root and command directory; shared dynamic paths are not safe by default. +### Per-step dispatch settings + +Workflow `command` and `prompt` steps can request the settings in +`DISPATCH_SETTINGS` (`reasoning_effort`, then `context_size`) for one CLI +invocation. Values are opaque strings whose meaning belongs to the agent's CLI. +Prompt steps pass requested settings to `build_exec_args()`. Command steps pass +them to `dispatch_command()`, whose base implementation forwards only the +requested keywords to `build_exec_args()`. The *receiving method* is therefore +`build_exec_args()`, or `dispatch_command()` for a command step when the +integration overrides it. + +Built-in integrations and external adapters opt in per setting with these +`specify_cli.integrations.base` APIs: + +- `supported_dispatch_settings`: a `frozenset` of the settings the integration + translates. Declare it on the class that implements the receiving method. + `effective_dispatch_settings()` reads the declaration from that class only: + a subclass that inherits the method inherits support, but a subclass that + overrides it must declare support again. A declaration on any other class, + or one that is not a set or frozenset, declares nothing. +- `reasoning_effort` and `context_size` keywords: the receiving method accepts + one, defaulting to `None`, for each declared setting. Hosts pass a keyword + only when a step sets that setting, so calls without settings are unchanged. + Treat any value other than `None` as requested; hosts never pass `""`. + Translate the value into CLI arguments placed after those from + `SPECKIT_INTEGRATION__EXTRA_ARGS`, and do not pass it on: + `IntegrationBase.build_exec_args()` and the format-base builders translate + neither setting and raise `DispatchSettingsError` rather than drop a value. +- `validate_dispatch_setting(name, value)`: an optional per-setting hook that + the gate calls before anything runs. The default accepts every value; raise + `ValueError` to reject a value the CLI would ignore or misread, and the host + attributes the failure to that setting. If the receiving method itself must + reject a setting, raise `DispatchSettingsError` with `setting`; without it, + the step error lists every requested setting with its source. +- `dispatch_setting_hint(setting)`: optional remediation text, such as another + step field to use instead, which the host appends to its error for an + unsupported setting. The default returns `None`. + +The host gate, `check_dispatch_settings(integration, settings, *, via)`, cannot +be overridden by integrations. Steps run it before executable detection, with +*via* set to `"dispatch_command"` for command steps and `"build_exec_args"` for +prompt steps; the base `dispatch_command()` runs it with `"build_exec_args"`. +For each requested setting, in `DISPATCH_SETTINGS` order, the gate requires a +string of 1 to 64 letters, digits, `.`, `_`, or `-` that starts with a letter +or digit, support under the ownership rule above, and a receiving method that +accepts the keyword or `**kwargs`. It then calls `validate_dispatch_setting()`. +Each failure raises `DispatchSettingsError`, a `ValueError` whose `setting` +attribute names the setting, so the step can report where its value came from. +An unsupported-setting error lists the integrations that support the setting, +and a malformed value is summarized rather than echoed. A translating method +can also run the gate itself, with *via* naming that method, so that direct +calls are checked too. + +The host cannot observe what the receiving method does with a declared +setting. An integration that declares support and then ignores the value, for +example by absorbing it in `**kwargs`, drops it silently: a declaration is a +promise to translate the setting. + ## Output flavors Choose the smallest base class that matches the agent's native format. The @@ -182,6 +243,13 @@ output_json=True, integration_args=None, integration_options=None, project_root=None)` as appropriate. Every explicitly declared host keyword parameter must have a default; accepting omitted keywords through `**kwargs` is also supported. Validation checks both prompt-only and complete host calls. +This required signature does not change for +[per-step dispatch settings](#per-step-dispatch-settings), and installation +does not check dispatch-setting declarations. An adapter that does not declare +support never receives `reasoning_effort` or `context_size`: a step that +requests either fails before the adapter's CLI runs. Adapters opt in like +built-in integrations, and the host gate checks the declaration and the +receiving method's signature when a step requests a setting. An adapter using only the host API and standard library needs no pip installation or source-registry edit. Import-side-effect registration is rejected. diff --git a/docs/integrations/cli.md b/docs/integrations/cli.md index ed63c12fc1..d644b4ba02 100644 --- a/docs/integrations/cli.md +++ b/docs/integrations/cli.md @@ -486,6 +486,13 @@ Docker Agent currently exposes structured agent/safety configuration. Headless approval flags, model forwarding, and structured-output support are also adapter-specific, not universal guarantees. +The [`reasoning_effort` and `context_size`](../workflow-steps/types.md#reasoning-effort-and-context-size) +settings of `command` and `prompt` steps instead follow one shared contract. +Spec Kit checks each value and passes it only to an integration that declares +and translates that setting; with any other integration, the step fails before +the CLI runs. Adapter authors opt in as described in the +[integration design](https://github.com/github/spec-kit/blob/main/design/integration.md#per-step-dispatch-settings). + ## FAQ ### Can I install multiple integrations in the same project? diff --git a/docs/workflow-steps/api.md b/docs/workflow-steps/api.md index 0c9629d299..7269ab091f 100644 --- a/docs/workflow-steps/api.md +++ b/docs/workflow-steps/api.md @@ -54,6 +54,8 @@ invalid configuration in `execute()` too. | `run_id` | `str \| None` | Current run ID | | `is_resume` | `bool` | Whether this execution is a resume attempt | | `workflow_dir` | `str \| None` | Source directory of the current workflow definition | +| `default_reasoning_effort` | `str \| None` | Workflow `reasoning_effort` default as written; can be an unevaluated expression | +| `default_context_size` | `str \| None` | Workflow `context_size` default as written; can be an unevaluated expression | Mappings default to empty dictionaries, boolean flags to false, and optional paths/identifiers to `None`. Paths are strings, not `Path` objects. Use diff --git a/docs/workflow-steps/types.md b/docs/workflow-steps/types.md index dd5bbbac5d..cf857d07c6 100644 --- a/docs/workflow-steps/types.md +++ b/docs/workflow-steps/types.md @@ -35,7 +35,9 @@ Invoke an installed Spec Kit command by name via the integration CLI: ``` For positional arguments and named runtime options, see -[Per-step integration configuration](#per-step-integration-configuration). +[Per-step integration configuration](#per-step-integration-configuration). For +`reasoning_effort` and `context_size`, see +[Reasoning effort and context size](#reasoning-effort-and-context-size). ## Prompt Steps @@ -195,6 +197,88 @@ or paused command is resumed, the complete dispatch configuration is re-resolved from the current inputs, so values supplied with `workflow resume --input` take effect consistently. A resume without updated inputs reproduces the same values. +A command step's `options` mapping, merged over `workflow.options`, is recorded +as `steps..output.options` but is not passed to the integration CLI. To +change the invocation, use `integration_args`, `integration_options`, `model`, +or [`reasoning_effort` and `context_size`](#reasoning-effort-and-context-size). + +## Reasoning Effort and Context Size + +`command` and `prompt` steps can set `reasoning_effort` and `context_size` for +the integration CLI they invoke. `workflow.reasoning_effort` and +`workflow.context_size` set defaults for both step types: + +```yaml +workflow: + id: plan-and-review + name: Plan and review + reasoning_effort: "medium" # Default for command and prompt steps + +inputs: + review_effort: + type: string + default: "" # Empty: the review step sends no effort + +steps: + - id: plan + command: speckit.plan + reasoning_effort: "high" # Overrides the workflow default + context_size: "200000" # Quoted: values are strings + + - id: review + type: prompt + prompt: "Review the plan for gaps" + reasoning_effort: "{{ inputs.review_effort }}" +``` + +**Resolution.** Each setting resolves like `model`: the step's value, then the +workflow default, then unset. The step value and the default can both be +expressions. A step value that is missing, `null`, or `""` inherits the +default. An expression that resolves to `""` or null unsets the setting for +that step, without falling back to the default; this is how a step opts out of +a default. In the example, `review` sends no reasoning effort unless the run +supplies `--input review_effort=`. + +**Values.** Values are strings. Quote numbers and words that YAML reads as +booleans, such as `on`, `off`, `yes`, and `no`: write `context_size: "200000"`, +not `context_size: 200000`. An expression must resolve to a string too, so +read a number from an input declared with `type: string`, not `type: number`. +A value must be 1 to 64 letters, digits, `.`, `_`, or `-`, starting with a +letter or digit. Literal values are checked when the workflow is validated, and +expression results when the step runs. Spec Kit passes the value to the +integration unchanged. Its meaning and accepted values are defined by the +integration's CLI, and an integration can reject a value before the CLI runs. + +**Support.** An integration translates a setting only if it declares support +for it. Otherwise the step fails before the CLI runs, with an error that names +the setting, its source (the step or the workflow default), and any +integrations that support it. Workflow defaults are not exempt: a default fails +each inheriting step whose integration does not support it. No built-in +integration supports these settings yet; see +[Workflow CLI dispatch](../integrations/cli.md#workflow-cli-dispatch) for how +integrations opt in. + +**CLI rejections.** When the CLI rejects a value and exits with a non-zero code, +the step fails like any other CLI failure. The step error reports the exit +code, and the CLI's message appears on the terminal, or in `output.stderr` with +[`capture: true`](#capturing-command-and-prompt-output). + +**Recording and resume.** The resolved values are recorded in +`steps..output.reasoning_effort` and `steps..output.context_size`, which +are `null` when unset. When `specify workflow resume` re-executes a step, it +resolves both again from the current inputs, so values supplied with `--input` +take effect. + +**Extra arguments.** An integration that supports these settings adds their +flags after any arguments from `SPECKIT_INTEGRATION__EXTRA_ARGS`. If both +set the same CLI flag, the CLI decides which value wins, so set each flag in +only one place. + +**Compatibility.** Spec Kit versions without this feature ignore these fields, +including the workflow defaults, and run steps without them. +`requires.speckit_version` does not prevent this: it is advisory and is not +enforced. + ## Shell step `type: shell` executes `run` through the platform shell, retaining support for @@ -628,9 +712,10 @@ preserved across workflow boundaries. ## Defaults and Errors at the Call Boundary -Parent `integration`, `model`, and `options` defaults apply only to steps in -the parent workflow. A child uses its own defaults or automatic resolution. To -make a value common to both, declare it as a child input and map it explicitly. +Parent `integration`, `model`, `options`, `reasoning_effort`, and +`context_size` defaults apply only to steps in the parent workflow. A child +uses its own defaults or automatic resolution. To make a value common to both, +declare it as a child input and map it explicitly. The call result intentionally has an empty `input` field and does not expose the child's private inputs, step records, or logs. diff --git a/docs/workflows/api.md b/docs/workflows/api.md index 7bdedbe1d7..58097cdf9c 100644 --- a/docs/workflows/api.md +++ b/docs/workflows/api.md @@ -13,7 +13,8 @@ This reference describes workflow YAML and execution/state semantics. The CLI's | `workflow.version` | String in `X.Y.Z` form; omitted defaults to `0.0.0` | | `workflow.author`, `workflow.description` | Descriptive metadata | | `workflow.integration`, `workflow.model` | Optional string/null dispatch defaults | -| `workflow.options` | Optional mapping/null of dispatch options | +| `workflow.reasoning_effort`, `workflow.context_size` | Optional string/null dispatch defaults for `command` and `prompt` steps; literals must follow the [value rules](../workflow-steps/types.md#reasoning-effort-and-context-size) | +| `workflow.options` | Optional mapping/null of default command-step `options`; recorded in step output, not passed to the integration CLI | | `requires` | Optional mapping accepting `speckit_version` and `integrations` only | | `inputs` | Mapping of input definitions | | `outputs` | Mapping of declared workflow-call outputs | diff --git a/src/specify_cli/integrations/base.py b/src/specify_cli/integrations/base.py index 09069ffffc..5b1a9bf192 100644 --- a/src/specify_cli/integrations/base.py +++ b/src/specify_cli/integrations/base.py @@ -80,6 +80,224 @@ def resolve_registrar_config(integration: IntegrationBase) -> dict[str, Any]: return config +# --------------------------------------------------------------------------- +# Per-step dispatch settings +# --------------------------------------------------------------------------- + +#: Settings a workflow ``command``/``prompt`` step can request for one CLI +#: invocation, in the order they are checked. Values are opaque strings that an +#: integration translates into its CLI's own arguments; see +#: :func:`check_dispatch_settings`. +DISPATCH_SETTINGS: tuple[str, ...] = ("reasoning_effort", "context_size") + +_DISPATCH_PATHS = ("dispatch_command", "build_exec_args") + +# One argv-safe token: no leading "-", whitespace, control characters or shell +# metacharacters (relevant to Windows ``.cmd`` shims), without restricting any +# integration's vocabulary. +_DISPATCH_SETTING_VALUE = re.compile(r"[A-Za-z0-9][A-Za-z0-9._-]{0,63}") +_DISPATCH_SETTING_PREVIEW = 24 + + +class DispatchSettingsError(ValueError): + """A per-step dispatch setting is malformed or unsupported. + + ``setting`` names the offending setting when known, so a workflow step can + report where its value came from. + """ + + def __init__(self, message: str, *, setting: str | None = None) -> None: + super().__init__(message) + self.setting = setting + + +def requested_dispatch_settings( + *, reasoning_effort: str | None = None, context_size: str | None = None +) -> dict[str, Any]: + """Return the settings that were passed (``is not None``) in check order.""" + values = {"reasoning_effort": reasoning_effort, "context_size": context_size} + return { + name: values[name] for name in DISPATCH_SETTINGS if values[name] is not None + } + + +def _summarize_untrusted_value(value: str) -> str: + preview = value[:_DISPATCH_SETTING_PREVIEW] + ellipsis = "…" if len(value) > _DISPATCH_SETTING_PREVIEW else "" + # repr() escapes control characters, so the summary is always one safe line. + return f"a string of length {len(value)} ({preview!r}{ellipsis})" + + +def dispatch_setting_value_error( + name: str, value: object, *, from_expression: bool = False +) -> str | None: + """Return why *value* is not a valid *name* setting, or ``None`` if it is. + + *from_expression* marks a value that a workflow expression produced, so a + non-string gets advice about the expression instead of YAML quoting. A + malformed value can come from untrusted expression results (for example + captured agent output), so it is summarized rather than echoed. + """ + if not isinstance(value, str): + type_name = type(value).__name__ + if from_expression: + return ( + f"'{name}' must be a string, but its expression resolved to " + f"{type_name}; make it resolve to a string, for example by " + "reading an input declared with type: string." + ) + return ( + f"'{name}' must be a string, got {type_name}; quote YAML values " + "such as 200000, on, or off." + ) + if _DISPATCH_SETTING_VALUE.fullmatch(value) is None: + return ( + f"'{name}' must be 1-64 letters, digits, '.', '_', or '-' starting " + f"with a letter or digit, got {_summarize_untrusted_value(value)}." + ) + return None + + +def _defining_class(cls: type, attribute: str) -> type | None: + for klass in cls.__mro__: + if attribute in vars(klass): + return klass + return None + + +def _receiving_method(integration: IntegrationBase, via: str) -> str: + if via not in _DISPATCH_PATHS: + raise ValueError(f"Unknown dispatch path {via!r}.") + if ( + via == "dispatch_command" + and _defining_class(type(integration), "dispatch_command") + is not IntegrationBase + ): + return "dispatch_command" + return "build_exec_args" + + +def effective_dispatch_settings( + integration: IntegrationBase, *, via: str = "build_exec_args" +) -> frozenset[str]: + """Return the dispatch settings *integration* translates on path *via*. + + Support counts only when declared on the class that implements the method + receiving the settings: ``dispatch_command`` when *via* is + ``"dispatch_command"`` and the integration overrides it, otherwise + ``build_exec_args``. A subclass that replaces that method must declare + ``supported_dispatch_settings`` itself, so an inherited declaration never + vouches for code that does not translate the settings. A declaration that + is not a set or frozenset declares nothing. + """ + owner = _defining_class(type(integration), _receiving_method(integration, via)) + if owner is None: + return frozenset() + declared = vars(owner).get("supported_dispatch_settings") + if not isinstance(declared, (set, frozenset)): + return frozenset() + return frozenset(name for name in DISPATCH_SETTINGS if name in declared) + + +def _accepts_keyword(function: Any, name: str) -> bool: + import inspect + + try: + parameters = inspect.signature(function).parameters + except (TypeError, ValueError): + return False + if any( + parameter.kind is inspect.Parameter.VAR_KEYWORD + for parameter in parameters.values() + ): + return True + parameter = parameters.get(name) + return parameter is not None and parameter.kind in ( + inspect.Parameter.POSITIONAL_OR_KEYWORD, + inspect.Parameter.KEYWORD_ONLY, + ) + + +def _dispatch_setting_supporters(name: str, via: str) -> list[str]: + from . import INTEGRATION_REGISTRY + from .installer import dispatch_registry + + registry = dispatch_registry.get() + if registry is None: + registry = INTEGRATION_REGISTRY + return sorted( + key + for key, candidate in registry.items() + if name in effective_dispatch_settings(candidate, via=via) + ) + + +def _unsupported_dispatch_setting_message( + integration: IntegrationBase, name: str, via: str +) -> str: + message = f"Integration {integration.key!r} does not support per-step '{name}'." + hint = integration.dispatch_setting_hint(name) + if isinstance(hint, str) and hint.strip(): + message = f"{message} {hint.strip()}" + supporters = _dispatch_setting_supporters(name, via) + if supporters: + message = ( + f"{message} Integrations that support it: {', '.join(supporters)}." + ) + else: + message = f"{message} No available integration supports it." + return ( + f"{message} Remove the setting from this step (and from the workflow " + "defaults it inherits), or dispatch the step to a supporting integration." + ) + + +def check_dispatch_settings( + integration: IntegrationBase, settings: Mapping[str, Any], *, via: str +) -> None: + """Gate requested per-step dispatch settings before anything is executed. + + Every dispatch path calls this host-owned check when at least one setting + is requested; integrations add value rules through + :meth:`IntegrationBase.validate_dispatch_setting` but cannot bypass it. + *via* names the integration method the settings are dispatched through + (``"dispatch_command"`` or ``"build_exec_args"``). Settings are checked in + ``DISPATCH_SETTINGS`` order, and every failure raises + :class:`DispatchSettingsError` naming the setting. + """ + receiver = _receiving_method(integration, via) + unknown = sorted(set(settings) - set(DISPATCH_SETTINGS)) + if unknown: + raise ValueError(f"Unknown dispatch settings: {', '.join(unknown)}.") + supported = effective_dispatch_settings(integration, via=via) + for name in DISPATCH_SETTINGS: + value = settings.get(name) + if value is None: + continue + problem = dispatch_setting_value_error(name, value) + if problem is not None: + raise DispatchSettingsError(problem, setting=name) + if name not in supported: + raise DispatchSettingsError( + _unsupported_dispatch_setting_message(integration, name, via), + setting=name, + ) + if not _accepts_keyword(getattr(integration, receiver), name): + raise DispatchSettingsError( + f"Integration {integration.key!r} declares per-step '{name}' " + f"support, but its {receiver}() does not accept '{name}'.", + setting=name, + ) + try: + integration.validate_dispatch_setting(name, value) + except DispatchSettingsError as exc: + if exc.setting is None: + exc.setting = name + raise + except ValueError as exc: + raise DispatchSettingsError(str(exc), setting=name) from exc + + # --------------------------------------------------------------------------- # IntegrationOption # --------------------------------------------------------------------------- @@ -151,6 +369,15 @@ class IntegrationBase(ABC): and command directory. Registry tests enforce these invariants. """ + supported_dispatch_settings: frozenset[str] = frozenset() + """Per-step dispatch settings (``DISPATCH_SETTINGS``) this integration translates. + + Declare support on the class that implements the method receiving the + settings (``build_exec_args``, or ``dispatch_command`` when overridden), and + translate every declared setting there; inherited declarations only cover + inherited implementations (see :func:`effective_dispatch_settings`). + """ + legacy_flat_command_dir: str | None = None """Previous flat command directory retired after skill replacements exist.""" @@ -277,6 +504,8 @@ def build_exec_args( integration_args: Sequence[str] | None = None, integration_options: Mapping[str, Any] | None = None, project_root: Path | None = None, + reasoning_effort: str | None = None, + context_size: str | None = None, ) -> list[str] | None: """Build CLI arguments for non-interactive execution. @@ -285,10 +514,55 @@ def build_exec_args( if the integration does not support CLI dispatch. Subclasses for CLI-based integrations should override this. + *reasoning_effort* and *context_size* are passed only when a workflow + step requests them and only to integrations that declare them in + ``supported_dispatch_settings``; this implementation translates + neither and rejects both. """ self.validate_runtime_config(integration_args, integration_options) + self._reject_untranslated_dispatch_settings( + reasoning_effort=reasoning_effort, context_size=context_size + ) return None + def validate_dispatch_setting(self, name: str, value: str) -> None: + """Validate one requested dispatch setting value for this integration. + + Called by :func:`check_dispatch_settings` for each supported setting + after the host's own checks. Values are opaque by default; override to + reject values the CLI would ignore or misread, raising ``ValueError``. + """ + + def dispatch_setting_hint(self, setting: str) -> str | None: + """Return remediation text for an unsupported *setting*, or ``None``.""" + return None + + def _reject_untranslated_dispatch_settings( + self, + *, + reasoning_effort: str | None = None, + context_size: str | None = None, + ) -> None: + """Fail when dispatch settings reach a builder that does not translate them. + + Builders that do translate settings consume them before delegating to + ``super().build_exec_args()``; reaching a generic builder therefore + means the settings would be silently dropped. + """ + requested = requested_dispatch_settings( + reasoning_effort=reasoning_effort, context_size=context_size + ) + if not requested: + return + names = ", ".join(f"'{name}'" for name in requested) + raise DispatchSettingsError( + f"Integration {self.key!r} does not translate per-step {names}. " + "Declare supported_dispatch_settings on the class that implements " + "build_exec_args() and consume the settings there instead of " + "passing them to a base implementation.", + setting=next(iter(requested)), + ) + def validate_runtime_config( self, integration_args: Sequence[str] | None = None, @@ -434,6 +708,8 @@ def dispatch_command( integration_options: Mapping[str, Any] | None = None, output_json: bool | None = None, capture_max_bytes: int | None = None, + reasoning_effort: str | None = None, + context_size: str | None = None, ) -> dict[str, Any]: """Dispatch a Spec Kit command through this integration's CLI. @@ -465,6 +741,11 @@ def dispatch_command( ``None`` (the default) keeps the unbounded capture, which raises ``subprocess.TimeoutExpired``. + *reasoning_effort* and *context_size* are per-step dispatch + settings. When either is passed, :func:`check_dispatch_settings` + validates it first and only the passed settings are forwarded to + ``build_exec_args()``, so builders that predate them keep working. + Returns a dict with ``exit_code``, ``stdout``, and ``stderr``. Raises ``NotImplementedError`` if the integration does not support CLI dispatch. @@ -475,6 +756,11 @@ def dispatch_command( raise ValueError("capture_max_bytes requires stream=False.") self.validate_runtime_config(integration_args, integration_options) + dispatch_settings = requested_dispatch_settings( + reasoning_effort=reasoning_effort, context_size=context_size + ) + if dispatch_settings: + check_dispatch_settings(self, dispatch_settings, via="build_exec_args") prompt = self._build_dispatch_prompt(command_name, args, project_root) # When streaming to the terminal, request text output so the # user sees readable output instead of raw JSONL events. @@ -487,6 +773,7 @@ def dispatch_command( integration_args=integration_args, integration_options=integration_options, project_root=project_root, + **dispatch_settings, ) if exec_args is None: @@ -1190,8 +1477,13 @@ def build_exec_args( integration_args: Sequence[str] | None = None, integration_options: Mapping[str, Any] | None = None, project_root: Path | None = None, + reasoning_effort: str | None = None, + context_size: str | None = None, ) -> list[str] | None: self.validate_runtime_config(integration_args, integration_options) + self._reject_untranslated_dispatch_settings( + reasoning_effort=reasoning_effort, context_size=context_size + ) if not self.config or not self.config.get("requires_cli"): return None args = [self._resolve_executable(), "-p", prompt] @@ -1286,8 +1578,13 @@ def build_exec_args( integration_args: Sequence[str] | None = None, integration_options: Mapping[str, Any] | None = None, project_root: Path | None = None, + reasoning_effort: str | None = None, + context_size: str | None = None, ) -> list[str] | None: self.validate_runtime_config(integration_args, integration_options) + self._reject_untranslated_dispatch_settings( + reasoning_effort=reasoning_effort, context_size=context_size + ) if not self.config or not self.config.get("requires_cli"): return None args = [self._resolve_executable(), "-p", prompt] @@ -1761,8 +2058,13 @@ def build_exec_args( integration_args: Sequence[str] | None = None, integration_options: Mapping[str, Any] | None = None, project_root: Path | None = None, + reasoning_effort: str | None = None, + context_size: str | None = None, ) -> list[str] | None: self.validate_runtime_config(integration_args, integration_options) + self._reject_untranslated_dispatch_settings( + reasoning_effort=reasoning_effort, context_size=context_size + ) if not self.config or not self.config.get("requires_cli"): return None args = [self._resolve_executable(), "-p", prompt] diff --git a/src/specify_cli/workflows/_execution.py b/src/specify_cli/workflows/_execution.py index 82324a9833..7cd2c1b51d 100644 --- a/src/specify_cli/workflows/_execution.py +++ b/src/specify_cli/workflows/_execution.py @@ -382,6 +382,8 @@ def child_context_for_call(parent, binding, definition): default_integration=definition.default_integration, default_model=definition.default_model, default_options=definition.default_options, + default_reasoning_effort=definition.default_reasoning_effort, + default_context_size=definition.default_context_size, ) diff --git a/src/specify_cli/workflows/base.py b/src/specify_cli/workflows/base.py index 59d8319d4d..dcf4dcf8a4 100644 --- a/src/specify_cli/workflows/base.py +++ b/src/specify_cli/workflows/base.py @@ -83,6 +83,12 @@ class StepContext: #: Source directory of the workflow definition file. workflow_dir: str | None = None + #: Workflow-level default reasoning effort. + default_reasoning_effort: str | None = None + + #: Workflow-level default context size. + default_context_size: str | None = None + @dataclass class StepResult: diff --git a/src/specify_cli/workflows/engine.py b/src/specify_cli/workflows/engine.py index 7cd85c091a..d292aad6d9 100644 --- a/src/specify_cli/workflows/engine.py +++ b/src/specify_cli/workflows/engine.py @@ -27,6 +27,7 @@ default_integration_key, try_read_integration_json, ) +from ..integrations.base import DISPATCH_SETTINGS, dispatch_setting_value_error from .base import RunStatus, StepContext @@ -93,6 +94,8 @@ def __init__(self, data: dict[str, Any], source_path: Path | None = None) -> Non # and retains its existing runtime representation as an empty mapping. self.default_integration: Any = workflow.get("integration") self.default_model: Any = workflow.get("model") + self.default_reasoning_effort: Any = workflow.get("reasoning_effort") + self.default_context_size: Any = workflow.get("context_size") raw_default_options = workflow.get("options") self.default_options: Any = ( {} if raw_default_options is None else raw_default_options @@ -210,6 +213,17 @@ def _dispatch_default_errors(definition: WorkflowDefinition) -> list[str]: f"({definition.default_options!r})." ) + # ``null`` and ``""`` leave a dispatch setting unset, and the step checks an + # expression once it is resolved. A malformed value is summarized rather + # than echoed. + for name in DISPATCH_SETTINGS: + value = getattr(definition, f"default_{name}") + if value is None or value == "" or (isinstance(value, str) and "{{" in value): + continue + problem = dispatch_setting_value_error(f"workflow.{name}", value) + if problem is not None: + errors.append(problem) + return errors @@ -1199,6 +1213,8 @@ def execute( project_root=str(self.project_root), run_id=state.run_id, workflow_dir=workflow_dir, + default_reasoning_effort=definition.default_reasoning_effort, + default_context_size=definition.default_context_size, ) # Execute steps @@ -1298,6 +1314,8 @@ def resume( run_id=state.run_id, is_resume=True, workflow_dir=state.workflow_dir, + default_reasoning_effort=definition.default_reasoning_effort, + default_context_size=definition.default_context_size, ) from . import STEP_REGISTRY diff --git a/src/specify_cli/workflows/step/_dispatch_settings.py b/src/specify_cli/workflows/step/_dispatch_settings.py new file mode 100644 index 0000000000..3faf114976 --- /dev/null +++ b/src/specify_cli/workflows/step/_dispatch_settings.py @@ -0,0 +1,129 @@ +"""Shared per-step dispatch settings for the ``command`` and ``prompt`` steps. + +``reasoning_effort`` and ``context_size`` resolve like ``model``: the step's +value, else the workflow default, evaluated as an expression. A result of +``None`` or ``""`` leaves the setting unset, so a step can opt out of an +inherited default. Set values are passed to the integration, which translates +them into its CLI's own arguments or rejects them; see +:func:`specify_cli.integrations.base.check_dispatch_settings`. This module +holds the parts of that contract the two steps must agree on: validation, +resolution, and naming where a rejected value came from. +""" + +from __future__ import annotations + +from collections.abc import Mapping +from dataclasses import dataclass +from typing import Any + +from specify_cli.integrations.base import ( + DISPATCH_SETTINGS, + DispatchSettingsError, + dispatch_setting_value_error, +) +from specify_cli.workflows.base import StepContext +from specify_cli.workflows.expressions import evaluate_expression + + +@dataclass(frozen=True) +class ResolvedDispatchSettings: + """Dispatch settings resolved for one step execution.""" + + #: Set values only, in ``DISPATCH_SETTINGS`` order. + settings: dict[str, str] + + #: Where each set value came from: ``"step"`` or ``"workflow."``. + sources: dict[str, str] + + #: Every setting's value, or ``None`` when unset, for the step output. + outputs: dict[str, str | None] + + +def _is_unset(value: Any) -> bool: + return value is None or value == "" + + +def dispatch_setting_errors(config: dict[str, Any], label: str) -> list[str]: + """Return validation errors for the step's literal dispatch settings. + + ``None`` and ``""`` inherit the workflow default, and an expression is + checked once it is resolved. *label* names the step kind (``"Command"``). + """ + errors: list[str] = [] + for name in DISPATCH_SETTINGS: + value = config.get(name) + if _is_unset(value) or (isinstance(value, str) and "{{" in value): + continue + problem = dispatch_setting_value_error(name, value) + if problem is not None: + errors.append(f"{label} step {config.get('id', '?')!r}: {problem}") + return errors + + +def resolve_dispatch_settings( + config: dict[str, Any], context: StepContext, label: str +) -> ResolvedDispatchSettings | str: + """Resolve the step's dispatch settings, or return an error message. + + A missing, ``None``, or ``""`` step value inherits the workflow default, + and a value containing ``{{`` is evaluated. A result of ``None`` or ``""`` + leaves the setting unset without a second fallback; any other result must + be a valid setting value. An error names the value's source. + """ + settings: dict[str, str] = {} + sources: dict[str, str] = {} + for name in DISPATCH_SETTINGS: + value = config.get(name) + source = "step" + if _is_unset(value): + value = getattr(context, f"default_{name}", None) + source = f"workflow.{name}" + from_expression = isinstance(value, str) and "{{" in value + if from_expression: + value = evaluate_expression(value, context) + if _is_unset(value): + continue + problem = dispatch_setting_value_error( + name, value, from_expression=from_expression + ) + if problem is not None: + return ( + f"{label} step {config.get('id', '?')!r}: {problem} " + f"{dispatch_settings_attribution(name, {name: source})}" + ) + settings[name] = value + sources[name] = source + return ResolvedDispatchSettings( + settings=settings, + sources=sources, + outputs={name: settings.get(name) for name in DISPATCH_SETTINGS}, + ) + + +def dispatch_settings_attribution( + setting: str | None, sources: Mapping[str, str] +) -> str: + """Return where a rejected dispatch setting came from, for an error message. + + Names the source of *setting* when it was set. Otherwise, for example for + an integration's own error that names no setting, lists every set setting + with its source. Returns ``""`` when no setting was set. + """ + if setting in sources: + return f"(set by {sources[setting]})" + listed = "; ".join( + f"{name} from {sources[name]}" for name in DISPATCH_SETTINGS if name in sources + ) + return f"(settings: {listed})" if listed else "" + + +def dispatch_settings_failure( + label: str, + config: dict[str, Any], + error: DispatchSettingsError, + sources: Mapping[str, str], +) -> str: + """Return the step error for a dispatch setting the integration rejected.""" + message = f"{label} step {config.get('id', '?')!r}: {error}" + attribution = dispatch_settings_attribution(error.setting, sources) + return f"{message} {attribution}" if attribution else message diff --git a/src/specify_cli/workflows/step/command/__init__.py b/src/specify_cli/workflows/step/command/__init__.py index 10a464850f..ba9e35f377 100644 --- a/src/specify_cli/workflows/step/command/__init__.py +++ b/src/specify_cli/workflows/step/command/__init__.py @@ -7,6 +7,11 @@ from pathlib import Path from typing import Any +from specify_cli.integrations.base import ( + DispatchSettingsError, + check_dispatch_settings, + requested_dispatch_settings, +) from specify_cli.integrations.installer import project_dispatch from specify_cli.workflows.base import StepBase, StepContext, StepResult, StepStatus from specify_cli.workflows.expressions import evaluate_expression @@ -17,6 +22,11 @@ record_capture, timeout_error, ) +from specify_cli.workflows.step._dispatch_settings import ( + dispatch_setting_errors, + dispatch_settings_failure, + resolve_dispatch_settings, +) _DEFAULT_CAPTURE_TIMEOUT = 600 @@ -127,6 +137,10 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult: ), ) + dispatch = resolve_dispatch_settings(config, context, "Command") + if isinstance(dispatch, str): + return StepResult(status=StepStatus.FAILED, error=dispatch) + # Merge options (workflow defaults ← step overrides) options = dict(context.default_options) step_options = config.get("options", {}) @@ -176,6 +190,7 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult: "command": command, "integration": integration, "model": model, + **dispatch.outputs, "options": options, "input": resolved_input, "integration_args": integration_args, @@ -196,6 +211,17 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult: integration_options, capture_max_bytes=capture_max_bytes, timeout=timeout, + **dispatch.settings, + ) + except DispatchSettingsError as exc: + output["exit_code"] = 1 + output["dispatched"] = False + return StepResult( + status=StepStatus.FAILED, + output=output, + error=dispatch_settings_failure( + "Command", config, exc, dispatch.sources + ), ) except ValueError as exc: output["exit_code"] = 1 @@ -262,6 +288,8 @@ def _try_dispatch( *, capture_max_bytes: int | None = None, timeout: float = _DEFAULT_CAPTURE_TIMEOUT, + reasoning_effort: str | None = None, + context_size: str | None = None, ) -> dict[str, Any] | None: """Invoke *command* by name through the integration CLI. @@ -273,11 +301,16 @@ def _try_dispatch( With *capture_max_bytes* set, the CLI's textual output is captured (bounded by that limit and by *timeout*) instead of streamed. + *reasoning_effort* and *context_size* are checked by + ``check_dispatch_settings`` before executable detection and forwarded + to ``dispatch_command`` only when set. + Returns the dispatch result dict, or ``None`` if dispatch is not possible (integration not found, CLI not installed, or dispatch not supported). Raises ``ValueError`` if capture is requested from an integration whose ``dispatch_command`` predates - the capture keywords. + the capture keywords, and ``DispatchSettingsError`` if the + integration rejects a dispatch setting. """ if not integration_key or not isinstance(integration_key, str): # A non-string integration (a list/dict/expression that resolved to @@ -296,6 +329,11 @@ def _try_dispatch( return None impl.validate_runtime_config(integration_args, integration_options) + dispatch_settings = requested_dispatch_settings( + reasoning_effort=reasoning_effort, context_size=context_size + ) + if dispatch_settings: + check_dispatch_settings(impl, dispatch_settings, via="dispatch_command") project_root = Path(context.project_root) if context.project_root else None @@ -345,6 +383,7 @@ def _try_dispatch( integration_args=integration_args, integration_options=integration_options, **capture_kwargs, + **dispatch_settings, ) except (NotImplementedError, OSError): return None @@ -466,6 +505,7 @@ def validate(self, config: dict[str, Any]) -> list[str]: f"Command step {config.get('id', '?')!r}: 'model' must be a " f"string, got {type(model).__name__}." ) + errors.extend(dispatch_setting_errors(config, "Command")) errors.extend(capture_errors(config, "Command")) # 'timeout' is only applied to a captured step, so a streaming step's # value is left unchecked as it always was. diff --git a/src/specify_cli/workflows/step/prompt/__init__.py b/src/specify_cli/workflows/step/prompt/__init__.py index 0177260c67..7a21c3ced8 100644 --- a/src/specify_cli/workflows/step/prompt/__init__.py +++ b/src/specify_cli/workflows/step/prompt/__init__.py @@ -8,6 +8,11 @@ from typing import Any from specify_cli.integrations._capture import run_captured +from specify_cli.integrations.base import ( + DispatchSettingsError, + check_dispatch_settings, + requested_dispatch_settings, +) from specify_cli.integrations.installer import project_dispatch from specify_cli.workflows.base import StepBase, StepContext, StepResult, StepStatus from specify_cli.workflows.expressions import evaluate_expression @@ -17,6 +22,11 @@ capture_limit, record_capture, ) +from specify_cli.workflows.step._dispatch_settings import ( + dispatch_setting_errors, + dispatch_settings_failure, + resolve_dispatch_settings, +) class PromptStep(StepBase): @@ -98,6 +108,10 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult: ), ) + dispatch = resolve_dispatch_settings(config, context, "Prompt") + if isinstance(dispatch, str): + return StepResult(status=StepStatus.FAILED, error=dispatch) + # An invalid timeout reaches subprocess.run() and raises a raw # TypeError ("unsupported operand type(s) for +: 'float' and 'str'") # or ValueError, which the engine re-raises — taking down the whole @@ -116,23 +130,37 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult: ) capture_max_bytes = capture_limit(config) - # Attempt CLI dispatch - timeout = config.get("timeout", 300) - dispatch_result = self._try_dispatch( - prompt, - integration, - model, - context, - timeout=timeout, - capture_max_bytes=capture_max_bytes, - ) - output: dict[str, Any] = { "prompt": prompt, "integration": integration, "model": model, + **dispatch.outputs, } + # Attempt CLI dispatch. A rejected dispatch setting fails the step; + # any other error propagates. + timeout = config.get("timeout", 300) + try: + dispatch_result = self._try_dispatch( + prompt, + integration, + model, + context, + timeout=timeout, + capture_max_bytes=capture_max_bytes, + **dispatch.settings, + ) + except DispatchSettingsError as exc: + output["exit_code"] = 1 + output["dispatched"] = False + return StepResult( + status=StepStatus.FAILED, + output=output, + error=dispatch_settings_failure( + "Prompt", config, exc, dispatch.sources + ), + ) + if dispatch_result is not None: output["exit_code"] = dispatch_result["exit_code"] output["stdout"] = dispatch_result["stdout"] @@ -220,11 +248,19 @@ def _try_dispatch( context: StepContext, timeout: int = 300, capture_max_bytes: int | None = None, + *, + reasoning_effort: str | None = None, + context_size: str | None = None, ) -> dict[str, Any] | None: """Dispatch *prompt* directly through the integration CLI. With *capture_max_bytes* set, stdout and stderr are captured (bounded by that limit) and returned instead of streamed to the terminal. + + *reasoning_effort* and *context_size* are checked by + ``check_dispatch_settings`` before executable detection and forwarded + to ``build_exec_args`` only when set; a rejected setting raises + ``DispatchSettingsError``. """ if not integration_key or not isinstance(integration_key, str) or not prompt: # A non-string integration would raise TypeError: unhashable type @@ -241,6 +277,12 @@ def _try_dispatch( if impl is None: return None + dispatch_settings = requested_dispatch_settings( + reasoning_effort=reasoning_effort, context_size=context_size + ) + if dispatch_settings: + check_dispatch_settings(impl, dispatch_settings, via="build_exec_args") + project_root = ( Path(context.project_root) if context.project_root else Path.cwd() ) @@ -250,6 +292,7 @@ def _try_dispatch( model=model, output_json=False, project_root=project_root, + **dispatch_settings, ) # Check if the CLI tool is actually installed. @@ -352,6 +395,7 @@ def validate(self, config: dict[str, Any]) -> list[str]: f"Prompt step {config.get('id', '?')!r}: 'model' must be a " f"string, got {type(model).__name__}." ) + errors.extend(dispatch_setting_errors(config, "Prompt")) timeout_error = self._timeout_error(config) if timeout_error is not None: errors.append(timeout_error) diff --git a/tests/integrations/test_base.py b/tests/integrations/test_base.py index 9344cc04a4..0a30f1cce6 100644 --- a/tests/integrations/test_base.py +++ b/tests/integrations/test_base.py @@ -8,10 +8,17 @@ import pytest from specify_cli.integrations.base import ( + DISPATCH_SETTINGS, + DispatchSettingsError, IntegrationBase, IntegrationOption, MarkdownIntegration, SkillsIntegration, + TomlIntegration, + check_dispatch_settings, + dispatch_setting_value_error, + effective_dispatch_settings, + requested_dispatch_settings, ) from specify_cli.integrations.manifest import IntegrationManifest from .conftest import StubIntegration @@ -773,3 +780,619 @@ def test_marks_py_and_sh_executable(self, monkeypatch, tmp_path): assert sh_file.stat().st_mode & 0o111 # Negative: a non-script file is not made executable. assert not (txt_file.stat().st_mode & 0o111) + + +# -- Per-step dispatch settings contract --------------------------------------- + +_CLI_CONFIG = {**StubIntegration.config, "requires_cli": True} + + +class _TranslatingIntegration(StubIntegration): + """Declares both settings and translates them in its own builder.""" + + key = "translating-stub" + config = _CLI_CONFIG + supported_dispatch_settings = frozenset(DISPATCH_SETTINGS) + + def build_exec_args( + self, + prompt, + *, + model=None, + output_json=True, + integration_args=None, + integration_options=None, + project_root=None, + reasoning_effort=None, + context_size=None, + ): + args = super().build_exec_args( + prompt, + model=model, + output_json=output_json, + integration_args=integration_args, + integration_options=integration_options, + project_root=project_root, + ) + if reasoning_effort is not None: + args.extend(["--effort", reasoning_effort]) + if context_size is not None: + args.extend(["--context", context_size]) + return args + + +class _InheritingIntegration(_TranslatingIntegration): + key = "inheriting-stub" + + +class _LegacyKwargsOverride(_TranslatingIntegration): + """Replaces the capable builder without re-declaring support.""" + + key = "legacy-kwargs-stub" + + def build_exec_args(self, prompt, **kwargs): + return ["legacy-kwargs-stub", "-p", prompt] + + +class _DeclaringFormatBase(StubIntegration): + """Declares support but inherits a generic format-base builder.""" + + key = "declaring-format-stub" + config = _CLI_CONFIG + supported_dispatch_settings = frozenset({"reasoning_effort"}) + + +class _StringDeclaration(StubIntegration): + key = "string-declaration-stub" + config = _CLI_CONFIG + supported_dispatch_settings = "reasoning_effort" + + def build_exec_args(self, prompt, **kwargs): + return ["string-declaration-stub", "-p", prompt] + + +class _DeclaresWithoutKeyword(StubIntegration): + key = "no-keyword-stub" + config = _CLI_CONFIG + supported_dispatch_settings = frozenset({"reasoning_effort"}) + + def build_exec_args( + self, + prompt, + *, + model=None, + output_json=True, + integration_args=None, + integration_options=None, + project_root=None, + ): + return ["no-keyword-stub", "-p", prompt] + + +class _ForwardsToSuper(StubIntegration): + """Declares support but hands the settings to the generic builder.""" + + key = "forwarding-stub" + config = _CLI_CONFIG + supported_dispatch_settings = frozenset({"reasoning_effort"}) + + def build_exec_args(self, prompt, **kwargs): + return super().build_exec_args(prompt, **kwargs) + + +class _CommandOwner(StubIntegration): + """Translates settings only in an overridden dispatch_command.""" + + key = "command-owner-stub" + supported_dispatch_settings = frozenset({"reasoning_effort"}) + + def dispatch_command(self, command_name, args="", *, reasoning_effort=None, **kwargs): + return {"exit_code": 0, "stdout": reasoning_effort or "", "stderr": ""} + + +class _ValidatingIntegration(_TranslatingIntegration): + key = "validating-stub" + + def validate_dispatch_setting(self, name, value): + if value == "rejected": + raise ValueError(f"{name} value is not accepted by this CLI") + if value == "unlabeled": + raise DispatchSettingsError("unlabeled failure") + + +class _HintingIntegration(StubIntegration): + key = "hinting-stub" + + def dispatch_setting_hint(self, setting): + if setting == "context_size": + return "Choose a long-context model instead." + return None + + +class _LegacyStrictBuilder(StubIntegration): + """A CLI builder whose signature predates dispatch settings.""" + + key = "legacy-strict-stub" + config = _CLI_CONFIG + + def __init__(self): + self.calls = [] + + def build_exec_args( + self, + prompt, + *, + model=None, + output_json=True, + integration_args=None, + integration_options=None, + project_root=None, + ): + self.calls.append( + { + "model": model, + "output_json": output_json, + "integration_args": integration_args, + "integration_options": integration_options, + "project_root": project_root, + } + ) + return ["legacy-strict-stub", "-p", prompt] + + +class _PlainBase(IntegrationBase): + key = "plain-stub" + config = _CLI_CONFIG + + +class _TomlStub(TomlIntegration): + key = "toml-stub" + config = _CLI_CONFIG + + +class _SkillsStub(SkillsIntegration): + key = "skills-stub" + config = _CLI_CONFIG + + +@pytest.fixture +def scoped_registry(): + """Pin the registry used to list supporting integrations.""" + from specify_cli.integrations.installer import dispatch_registry + + tokens = [] + + def pin(*integrations): + tokens.append( + dispatch_registry.set({item.key: item for item in integrations}) + ) + + yield pin + for token in reversed(tokens): + dispatch_registry.reset(token) + + +def _fake_run(calls): + def run(args, **kwargs): + calls.append(list(args)) + return SimpleNamespace(returncode=0, stdout="", stderr="") + + return run + + +class TestDispatchSettingsValues: + def test_settings_are_ordered_and_named(self): + assert DISPATCH_SETTINGS == ("reasoning_effort", "context_size") + + def test_requested_settings_use_is_not_none_semantics(self): + assert requested_dispatch_settings() == {} + assert requested_dispatch_settings( + context_size="long_context", reasoning_effort="high" + ) == {"reasoning_effort": "high", "context_size": "long_context"} + assert list( + requested_dispatch_settings(context_size="a", reasoning_effort="b") + ) == ["reasoning_effort", "context_size"] + assert requested_dispatch_settings(reasoning_effort="") == { + "reasoning_effort": "" + } + + @pytest.mark.parametrize( + "value", ["low", "xhigh", "long_context", "200000", "a.b-c_d", "A" * 64] + ) + def test_accepts_argv_safe_tokens(self, value): + assert dispatch_setting_value_error("reasoning_effort", value) is None + + @pytest.mark.parametrize( + "value", + [ + "", + "-x", + "--yolo", + "x y", + "high\n", + "high\r", + "a" * 65, + "a&b", + "a|b", + "a;b", + "ab", + "a^b", + "a\x00b", + ".hidden", + "_x", + ], + ) + def test_rejects_unsafe_tokens(self, value): + message = dispatch_setting_value_error("reasoning_effort", value) + assert message is not None + assert "'reasoning_effort' must be 1-64 letters" in message + + @pytest.mark.parametrize("value", [200000, True, None, 1.5, ["high"]]) + def test_non_strings_get_a_quoting_hint(self, value): + message = dispatch_setting_value_error("context_size", value) + assert message is not None + assert f"must be a string, got {type(value).__name__}" in message + assert "quote YAML values" in message + + @pytest.mark.parametrize("value", [200000, True, 1.5, ["high"]]) + def test_non_string_expression_results_get_expression_advice(self, value): + message = dispatch_setting_value_error( + "context_size", value, from_expression=True + ) + assert message == ( + "'context_size' must be a string, but its expression resolved to " + f"{type(value).__name__}; make it resolve to a string, for example " + "by reading an input declared with type: string." + ) + + def test_expression_flag_changes_only_the_non_string_advice(self): + assert ( + dispatch_setting_value_error("context_size", "200000", from_expression=True) + is None + ) + assert dispatch_setting_value_error( + "reasoning_effort", "-rf", from_expression=True + ) == dispatch_setting_value_error("reasoning_effort", "-rf") + + def test_malformed_values_are_summarized_not_echoed(self): + value = "\x1b[31m" + "x" * (1024 * 1024) + "\nsecret" + message = dispatch_setting_value_error("reasoning_effort", value) + assert message is not None + assert len(message) < 300 + assert "\x1b" not in message + assert "\n" not in message + assert "secret" not in message + assert f"length {len(value)}" in message + + +class TestEffectiveDispatchSettings: + @pytest.mark.parametrize("via", ["dispatch_command", "build_exec_args"]) + def test_declaring_builder_owns_its_settings(self, via): + assert effective_dispatch_settings( + _TranslatingIntegration(), via=via + ) == frozenset(DISPATCH_SETTINGS) + + @pytest.mark.parametrize("via", ["dispatch_command", "build_exec_args"]) + def test_subclass_without_override_inherits_support(self, via): + assert effective_dispatch_settings( + _InheritingIntegration(), via=via + ) == frozenset(DISPATCH_SETTINGS) + + @pytest.mark.parametrize( + "integration", + [ + _LegacyKwargsOverride(), + _DeclaringFormatBase(), + _StringDeclaration(), + StubIntegration(), + ], + ids=["legacy-kwargs-override", "format-base", "string", "undeclared"], + ) + @pytest.mark.parametrize("via", ["dispatch_command", "build_exec_args"]) + def test_untranslated_declarations_count_as_unsupported(self, integration, via): + assert effective_dispatch_settings(integration, via=via) == frozenset() + + def test_dispatch_command_owner_supports_only_the_command_path(self): + integration = _CommandOwner() + + assert effective_dispatch_settings( + integration, via="dispatch_command" + ) == frozenset({"reasoning_effort"}) + assert effective_dispatch_settings( + integration, via="build_exec_args" + ) == frozenset() + + def test_unknown_dispatch_path_is_a_programming_error(self): + with pytest.raises(ValueError, match="Unknown dispatch path"): + effective_dispatch_settings(_TranslatingIntegration(), via="prompt") + + def test_builtin_declarations_are_owned_by_their_receivers(self): + from specify_cli.integrations import INTEGRATION_REGISTRY + + for key, integration in INTEGRATION_REGISTRY.items(): + for via in ("dispatch_command", "build_exec_args"): + supported = effective_dispatch_settings(integration, via=via) + assert supported <= set(DISPATCH_SETTINGS), key + receiver = ( + "dispatch_command" + if via == "dispatch_command" + and "dispatch_command" in vars(type(integration)) + else "build_exec_args" + ) + parameters = inspect.signature( + getattr(integration, receiver) + ).parameters + for name in supported: + assert name in parameters, (key, via, name) + + +class TestCheckDispatchSettings: + def test_unsupported_setting_lists_no_supporters(self, scoped_registry): + integration = _LegacyStrictBuilder() + scoped_registry(integration) + + with pytest.raises(DispatchSettingsError) as excinfo: + check_dispatch_settings( + integration, {"reasoning_effort": "high"}, via="dispatch_command" + ) + + assert excinfo.value.setting == "reasoning_effort" + message = str(excinfo.value) + assert "'legacy-strict-stub' does not support per-step 'reasoning_effort'" in message + assert "No available integration supports it." in message + assert "Remove the setting from this step" in message + + def test_unsupported_setting_lists_pinned_supporters(self, scoped_registry): + scoped_registry( + _LegacyStrictBuilder(), _TranslatingIntegration(), _CommandOwner() + ) + + with pytest.raises(DispatchSettingsError) as excinfo: + check_dispatch_settings( + _LegacyStrictBuilder(), + {"reasoning_effort": "high"}, + via="build_exec_args", + ) + + assert "Integrations that support it: translating-stub." in str(excinfo.value) + + def test_hint_is_appended_to_unsupported_error(self, scoped_registry): + scoped_registry() + + with pytest.raises(DispatchSettingsError, match="Choose a long-context model"): + check_dispatch_settings( + _HintingIntegration(), + {"context_size": "long_context"}, + via="dispatch_command", + ) + + def test_settings_are_checked_in_declared_order(self, scoped_registry): + scoped_registry() + + with pytest.raises(DispatchSettingsError) as excinfo: + check_dispatch_settings( + StubIntegration(), + {"context_size": "x y", "reasoning_effort": "x y"}, + via="build_exec_args", + ) + + assert excinfo.value.setting == "reasoning_effort" + + def test_value_errors_are_raised_before_support_errors(self): + with pytest.raises(DispatchSettingsError, match="must be 1-64") as excinfo: + check_dispatch_settings( + StubIntegration(), {"context_size": "-rf"}, via="build_exec_args" + ) + + assert excinfo.value.setting == "context_size" + + @pytest.mark.parametrize( + "integration", + [_LegacyKwargsOverride(), _DeclaringFormatBase(), _StringDeclaration()], + ids=["legacy-kwargs-override", "format-base", "string"], + ) + def test_untranslated_declarations_fail_instead_of_dropping( + self, scoped_registry, integration + ): + scoped_registry() + + with pytest.raises(DispatchSettingsError, match="does not support") as excinfo: + check_dispatch_settings( + integration, {"reasoning_effort": "high"}, via="dispatch_command" + ) + + assert excinfo.value.setting == "reasoning_effort" + + def test_declared_receiver_must_accept_the_keyword(self): + with pytest.raises(DispatchSettingsError, match="does not accept") as excinfo: + check_dispatch_settings( + _DeclaresWithoutKeyword(), + {"reasoning_effort": "high"}, + via="dispatch_command", + ) + + assert excinfo.value.setting == "reasoning_effort" + + def test_integration_value_errors_are_attributed_to_their_setting(self): + with pytest.raises(DispatchSettingsError) as excinfo: + check_dispatch_settings( + _ValidatingIntegration(), + {"reasoning_effort": "high", "context_size": "rejected"}, + via="build_exec_args", + ) + + assert excinfo.value.setting == "context_size" + assert "context_size value is not accepted" in str(excinfo.value) + assert isinstance(excinfo.value.__cause__, ValueError) + + def test_unlabeled_integration_errors_are_labeled(self): + with pytest.raises(DispatchSettingsError, match="unlabeled failure") as excinfo: + check_dispatch_settings( + _ValidatingIntegration(), + {"reasoning_effort": "unlabeled"}, + via="build_exec_args", + ) + + assert excinfo.value.setting == "reasoning_effort" + + def test_supported_settings_pass(self): + check_dispatch_settings( + _TranslatingIntegration(), + {"reasoning_effort": "high", "context_size": "long_context"}, + via="build_exec_args", + ) + check_dispatch_settings( + _CommandOwner(), {"reasoning_effort": "high"}, via="dispatch_command" + ) + + def test_none_values_are_skipped(self): + check_dispatch_settings( + StubIntegration(), + {"reasoning_effort": None, "context_size": None}, + via="build_exec_args", + ) + + def test_unknown_setting_names_are_programming_errors(self): + with pytest.raises(ValueError, match="Unknown dispatch settings"): + check_dispatch_settings( + _TranslatingIntegration(), {"temperature": "1"}, via="build_exec_args" + ) + + def test_builtins_reject_settings_they_do_not_declare(self, scoped_registry): + from specify_cli.integrations import INTEGRATION_REGISTRY + + scoped_registry() + for key, integration in INTEGRATION_REGISTRY.items(): + for via in ("dispatch_command", "build_exec_args"): + supported = effective_dispatch_settings(integration, via=via) + for name in DISPATCH_SETTINGS: + if name in supported: + continue + with pytest.raises(DispatchSettingsError) as excinfo: + check_dispatch_settings( + integration, {name: "high"}, via=via + ) + assert excinfo.value.setting == name, (key, via) + + +class TestDispatchSettingsBuilders: + @pytest.mark.parametrize( + "integration", + [_PlainBase(), StubIntegration(), _TomlStub(), _SkillsStub()], + ids=["base", "markdown", "toml", "skills"], + ) + @pytest.mark.parametrize( + ("settings", "expected"), + [ + ({"reasoning_effort": "high"}, "reasoning_effort"), + ({"context_size": "long_context"}, "context_size"), + ( + {"reasoning_effort": "high", "context_size": "long_context"}, + "reasoning_effort", + ), + ], + ) + def test_generic_builders_reject_untranslated_settings( + self, integration, settings, expected + ): + with pytest.raises(DispatchSettingsError, match="does not translate") as excinfo: + integration.build_exec_args("prompt", **settings) + + assert excinfo.value.setting == expected + + @pytest.mark.parametrize( + "integration", + [_PlainBase(), StubIntegration(), _TomlStub(), _SkillsStub()], + ids=["base", "markdown", "toml", "skills"], + ) + def test_generic_builders_are_unchanged_without_settings(self, integration): + assert integration.build_exec_args( + "prompt", reasoning_effort=None, context_size=None + ) == integration.build_exec_args("prompt") + + def test_forwarding_declared_settings_to_super_fails_loudly(self): + with pytest.raises(DispatchSettingsError, match="does not translate"): + _ForwardsToSuper().build_exec_args("prompt", reasoning_effort="high") + + def test_command_check_probe_is_unchanged(self, tmp_path): + from specify_cli.integrations import INTEGRATION_REGISTRY + + for key, integration in INTEGRATION_REGISTRY.items(): + try: + integration.build_exec_args("test", project_root=tmp_path) + except DispatchSettingsError: + pytest.fail(f"{key}: settings-free probe hit the dispatch-settings guard") + except ValueError: + # Pre-existing integration requirements (e.g. Docker Agent's + # agent reference) are unrelated to dispatch settings. + pass + + +class TestDispatchCommandSettings: + def test_unset_settings_reach_legacy_builders_unchanged( + self, monkeypatch, tmp_path + ): + calls = [] + monkeypatch.setattr("subprocess.run", _fake_run(calls)) + integration = _LegacyStrictBuilder() + + result = integration.dispatch_command( + "speckit.plan", "add auth", project_root=tmp_path + ) + + assert result["exit_code"] == 0 + assert integration.calls == [ + { + "model": None, + "output_json": False, + "integration_args": None, + "integration_options": None, + "project_root": tmp_path, + } + ] + assert calls[0][1:] == ["-p", "/speckit.plan add auth"] + + def test_only_requested_settings_are_forwarded(self, monkeypatch, tmp_path): + calls = [] + monkeypatch.setattr("subprocess.run", _fake_run(calls)) + + _TranslatingIntegration().dispatch_command( + "speckit.plan", project_root=tmp_path, reasoning_effort="high" + ) + + assert calls[0][-2:] == ["--effort", "high"] + assert "--context" not in calls[0] + + def test_unsupported_settings_fail_before_building_or_running( + self, monkeypatch, scoped_registry, tmp_path + ): + calls = [] + monkeypatch.setattr("subprocess.run", _fake_run(calls)) + integration = _LegacyStrictBuilder() + scoped_registry(integration) + + with pytest.raises(DispatchSettingsError, match="does not support"): + integration.dispatch_command( + "speckit.plan", project_root=tmp_path, context_size="long_context" + ) + + assert integration.calls == [] + assert calls == [] + + def test_forwarding_to_super_fails_before_running(self, monkeypatch, tmp_path): + calls = [] + monkeypatch.setattr("subprocess.run", _fake_run(calls)) + + with pytest.raises(DispatchSettingsError, match="does not translate"): + _ForwardsToSuper().dispatch_command( + "speckit.plan", project_root=tmp_path, reasoning_effort="high" + ) + + assert calls == [] + + def test_empty_string_is_validated_not_ignored(self, tmp_path): + with pytest.raises(DispatchSettingsError, match="must be 1-64"): + _TranslatingIntegration().dispatch_command( + "speckit.plan", project_root=tmp_path, reasoning_effort="" + ) diff --git a/tests/specify_cli/integrations/test_installed_adapters.py b/tests/specify_cli/integrations/test_installed_adapters.py index 12484c6f04..4c8f131bd3 100644 --- a/tests/specify_cli/integrations/test_installed_adapters.py +++ b/tests/specify_cli/integrations/test_installed_adapters.py @@ -4755,3 +4755,226 @@ def test_extension_info_lists_registered_names_without_loading_adapter( assert f"• {listed}: " in info.output, info.output assert f"• {not_listed}: " not in info.output, info.output assert KEY not in INTEGRATION_REGISTRY + + +DISPATCH_SETTINGS_BODY = ''' + supported_dispatch_settings = frozenset({"reasoning_effort", "context_size"}) + + def build_exec_args(self, prompt, *, model=None, output_json=True, + integration_args=None, integration_options=None, + project_root=None, reasoning_effort=None, context_size=None): + import sys + args = [sys.executable, "-c", SCRIPT, prompt] + if reasoning_effort is not None: + args += ["--effort", reasoning_effort] + if context_size is not None: + args += ["--context", context_size] + return args +''' +LEGACY_DISPATCH_BODY = ''' + def build_exec_args(self, prompt, *, model=None, output_json=True, + integration_args=None, integration_options=None, + project_root=None): + import sys + return [sys.executable, "-c", SCRIPT, prompt] +''' +# Inherits the declaration of a capable adapter in ``capable.py`` but replaces +# the builder that translates the settings with one that absorbs them. It +# imports the module, not the class, so the package exports one adapter class. +LEGACY_OVERRIDE_OF_CAPABLE_ADAPTER = '''from . import capable + + +class SampleIntegration(capable.SampleIntegration): + def build_exec_args(self, prompt, **kwargs): + import sys + return [sys.executable, "-c", capable.SCRIPT, prompt] +''' + + +def agent_cli(tmp_path): + """Return a stand-in agent CLI script with its argument log and fail flag. + + The script appends the arguments it receives to ``log`` as one JSON line, + then exits 1 while ``fail`` exists and 0 otherwise. + """ + log = tmp_path / "agent-cli-calls.jsonl" + fail = tmp_path / "agent-cli-fails" + script = ( + "import json, os, sys\n" + f"with open({str(log)!r}, 'a', encoding='utf-8') as stream:\n" + " stream.write(json.dumps(sys.argv[1:]) + '\\n')\n" + f"sys.exit(1 if os.path.exists({str(fail)!r}) else 0)\n" + ) + return SimpleNamespace(script=script, log=log, fail=fail) + + +def agent_cli_implementation(cli, body): + """Return adapter source whose ``SCRIPT`` is the stand-in CLI's script.""" + return implementation(body=body) + f"\nSCRIPT = {cli.script!r}\n" + + +def dispatched(cli): + """Return the argument lists the stand-in agent CLI received, in order.""" + if not cli.log.exists(): + return [] + return [json.loads(line) for line in cli.log.read_text(encoding="utf-8").splitlines()] + + +def write_dispatch_workflow(project, steps, *, inputs=None, **defaults): + data = { + "schema_version": "1.0", + "workflow": { + "id": "sample-workflow", "name": "Sample Workflow", "version": "1.0.0", + "integration": KEY, **defaults, + }, + "steps": steps, + } + if inputs is not None: + data["inputs"] = inputs + path = project / "sample-dispatch-workflow.yml" + path.write_text(yaml.safe_dump(data)) + return path + + +def dispatch_outputs(project, run_id): + """Return each recorded step's ``(reasoning_effort, context_size)`` output.""" + from specify_cli.workflows.engine import RunState + + return { + step_id: (result["output"]["reasoning_effort"], result["output"]["context_size"]) + for step_id, result in RunState.load(run_id, project).step_results.items() + } + + +def run_fresh_process(project, arguments): + executable = Path(sys.executable).parent / ("specify.exe" if os.name == "nt" else "specify") + return subprocess.run( + [str(executable), *arguments], cwd=project, capture_output=True, + text=True, encoding="utf-8", timeout=120, check=False, + ) + + +def test_declared_dispatch_settings_reach_installed_adapter_on_run_and_resume(tmp_path, server): + cli = agent_cli(tmp_path) + publish(server, code=agent_cli_implementation(cli, DISPATCH_SETTINGS_BODY)) + project = catalog_project(tmp_path, server) + install(project) + workflow = str(write_dispatch_workflow(project, [ + {"id": "plan", "type": "command", "command": "speckit.plan", + "reasoning_effort": "{{ inputs.effort }}"}, + {"id": "review", "type": "prompt", "prompt": "Review the plan", "context_size": "200000"}, + ], inputs={"effort": {"type": "string", "default": "high"}})) + + completed = run_fresh_process(project, ["workflow", "run", workflow, "--json"]) + assert completed.returncode == 0, completed.stdout + completed.stderr + report = json.loads(completed.stdout) + assert report["status"] == "completed" + assert dispatched(cli) == [ + ["/speckit-plan", "--effort", "high"], + ["Review the plan", "--context", "200000"], + ] + assert dispatch_outputs(project, report["run_id"]) == { + "plan": ("high", None), "review": (None, "200000"), + } + + cli.fail.touch() + failed = run_fresh_process(project, ["workflow", "run", workflow, "--json"]) + assert failed.returncode == 1, failed.stdout + failed.stderr + failed_run = json.loads(failed.stdout) + assert (failed_run["status"], failed_run["current_step_id"]) == ("failed", "plan") + assert dispatched(cli)[2:] == [["/speckit-plan", "--effort", "high"]] + + cli.fail.unlink() + resumed = run_fresh_process( + project, ["workflow", "resume", failed_run["run_id"], "--input", "effort=low", "--json"], + ) + assert resumed.returncode == 0, resumed.stdout + resumed.stderr + assert json.loads(resumed.stdout)["status"] == "completed" + assert dispatched(cli)[3:] == [ + ["/speckit-plan", "--effort", "low"], + ["Review the plan", "--context", "200000"], + ] + assert dispatch_outputs(project, failed_run["run_id"]) == { + "plan": ("low", None), "review": (None, "200000"), + } + + +@pytest.mark.parametrize("continue_on_error", [False, True]) +@pytest.mark.parametrize("kind", ["command", "prompt"]) +@pytest.mark.parametrize("adapter", ["undeclared", "inherited-declaration"]) +def test_legacy_installed_adapter_fails_only_steps_requesting_dispatch_settings( + tmp_path, server, adapter, kind, continue_on_error, +): + """A legacy adapter keeps loading and running steps; a step that requests a + setting its builder does not translate fails before the CLI runs.""" + from specify_cli.workflows.engine import RunState + + cli = agent_cli(tmp_path) + if adapter == "undeclared": + publish(server, code=agent_cli_implementation(cli, LEGACY_DISPATCH_BODY)) + else: + publish(server, code=LEGACY_OVERRIDE_OF_CAPABLE_ADAPTER, members={ + "capable.py": agent_cli_implementation(cli, DISPATCH_SETTINGS_BODY).encode(), + }) + project = catalog_project(tmp_path, server) + install(project) + if adapter == "inherited-declaration": + inherited = INTEGRATION_REGISTRY[KEY].supported_dispatch_settings + assert inherited == {"reasoning_effort", "context_size"} + requesting = {"id": "effort", "type": kind, "reasoning_effort": "high"} + if kind == "command": + requesting["command"] = "speckit.tasks" + else: + requesting["prompt"] = "Summarize the plan" + if continue_on_error: + requesting["continue_on_error"] = True + workflow = write_dispatch_workflow(project, [ + {"id": "plan", "type": "command", "command": "speckit.plan"}, + requesting, + {"id": "review", "type": "prompt", "prompt": "Review the plan"}, + ]) + + result = run_fresh_process(project, ["workflow", "run", str(workflow), "--json"]) + + assert result.returncode == (0 if continue_on_error else 1), result.stdout + result.stderr + assert "Traceback" not in result.stderr + report = json.loads(result.stdout) + assert report["status"] == ("completed" if continue_on_error else "failed") + assert dispatched(cli) == ( + [["/speckit-plan"], ["Review the plan"]] if continue_on_error else [["/speckit-plan"]] + ) + results = RunState.load(report["run_id"], project).step_results + assert results["plan"]["status"] == "completed" + assert results["effort"]["status"] == "failed" + assert results["effort"]["output"]["dispatched"] is False + error = results["effort"]["error"] + assert error.startswith( + f"{kind.capitalize()} step 'effort': Integration {KEY!r} does not support " + "per-step 'reasoning_effort'." + ), error + assert error.endswith(" (set by step)"), error + assert report.get("error") == (None if continue_on_error else error) + + +@pytest.mark.parametrize( + "unset", [{}, {"reasoning_effort": None, "context_size": ""}], ids=["absent", "null-or-empty"], +) +def test_installed_adapter_receives_no_dispatch_settings_when_unset(tmp_path, server, unset): + cli = agent_cli(tmp_path) + publish(server, code=agent_cli_implementation(cli, DISPATCH_SETTINGS_BODY)) + project = catalog_project(tmp_path, server) + install(project) + workflow = write_dispatch_workflow(project, [ + {"id": "plan", "type": "command", "command": "speckit.plan", **unset}, + {"id": "review", "type": "prompt", "prompt": "Review the plan", **unset}, + ], **unset) + + result = run_fresh_process(project, ["workflow", "run", str(workflow), "--json"]) + + assert result.returncode == 0, result.stdout + result.stderr + report = json.loads(result.stdout) + assert report["status"] == "completed" + assert dispatched(cli) == [["/speckit-plan"], ["Review the plan"]] + assert dispatch_outputs(project, report["run_id"]) == { + "plan": (None, None), "review": (None, None), + } diff --git a/tests/workflows/test_dispatch_settings.py b/tests/workflows/test_dispatch_settings.py new file mode 100644 index 0000000000..c5b9c6b616 --- /dev/null +++ b/tests/workflows/test_dispatch_settings.py @@ -0,0 +1,1366 @@ +"""Per-step ``reasoning_effort`` and ``context_size`` on the dispatching steps. + +The integrations here are fakes registered for one test, or built-ins that +translate neither setting (Gemini, Docker Agent). Executable lookup and the +CLI process are faked, so no agent CLI runs. +""" + +from __future__ import annotations + +import dataclasses +from types import SimpleNamespace + +import pytest +import yaml + +from specify_cli.integrations import INTEGRATION_REGISTRY +from specify_cli.integrations._capture import CapturedProcess +from specify_cli.integrations.base import ( + DISPATCH_SETTINGS, + DispatchSettingsError, + IntegrationBase, + dispatch_setting_value_error, +) +from specify_cli.integrations.gemini import GeminiIntegration +from specify_cli.workflows import STEP_REGISTRY +from specify_cli.workflows.base import ( + RunStatus, + StepBase, + StepContext, + StepResult, + StepStatus, +) +from specify_cli.workflows.engine import ( + WorkflowDefinition, + WorkflowEngine, + validate_workflow, +) +from specify_cli.workflows.step._dispatch_settings import ( + dispatch_settings_attribution, + resolve_dispatch_settings, +) +from specify_cli.workflows.step.command import CommandStep +from specify_cli.workflows.step.prompt import PromptStep + +STEPS = { + "command": (CommandStep, {"command": "speckit.plan"}), + "prompt": (PromptStep, {"type": "prompt", "prompt": "Summarise the plan"}), +} +LABELS = {"command": "Command", "prompt": "Prompt"} +PROMPTS = {"command": "/speckit.plan", "prompt": "Summarise the plan"} +KINDS = pytest.mark.parametrize("kind", list(STEPS)) + +UNSUPPORTED = ( + "Integration {key!r} does not support per-step '{name}'. {supporters} " + "Remove the setting from this step (and from the workflow defaults it " + "inherits), or dispatch the step to a supporting integration." +) +NO_SUPPORTERS = "No available integration supports it." +UNSAFE_VALUES = pytest.mark.parametrize( + "value", + [ + pytest.param("-high", id="leading-dash"), + pytest.param("high effort", id="space"), + pytest.param("high\n", id="trailing-newline"), + pytest.param("a" * 65, id="65-chars"), + pytest.param("a&b", id="ampersand"), + pytest.param("a|b", id="pipe"), + pytest.param("a;b", id="semicolon"), + pytest.param("ab", id="greater-than"), + pytest.param("a^b", id="caret"), + ], +) +# Untrusted text, such as captured agent output, that reaches a setting. +HUGE = "\x1b[31m" + "x" * (1024 * 1024) + "\nSECRET" + + +# -- Fake integrations ---------------------------------------------------- + + +class _Translating(IntegrationBase): + """Declares both settings and translates them in its own builder.""" + + key = "fake-translating" + supported_dispatch_settings = frozenset(DISPATCH_SETTINGS) + + def __init__(self) -> None: + self.calls: list[dict] = [] + + def build_exec_args(self, prompt, **kwargs): + self.calls.append(kwargs) + args = [self.key, "-p", prompt] + self._apply_extra_args_env_var(args) + for flag, name in ( + ("--effort", "reasoning_effort"), + ("--context", "context_size"), + ): + if name in kwargs: + args.extend([flag, kwargs[name]]) + return args + + +class _Inheriting(_Translating): + """Inherits the declaration together with the builder it covers.""" + + key = "fake-inheriting" + + +class _EffortOnly(IntegrationBase): + """Translates reasoning effort but not context size.""" + + key = "fake-effort-only" + supported_dispatch_settings = frozenset({"reasoning_effort"}) + + def build_exec_args(self, prompt, *, reasoning_effort=None, **kwargs): + args = [self.key, "-p", prompt] + if reasoning_effort is not None: + args.extend(["--effort", reasoning_effort]) + return args + + +class _CommandOwner(IntegrationBase): + """Translates reasoning effort in its own ``dispatch_command`` only.""" + + key = "fake-command-owner" + supported_dispatch_settings = frozenset({"reasoning_effort"}) + + def __init__(self) -> None: + self.calls: list[dict] = [] + + def dispatch_command(self, command_name, args="", **kwargs): + self.calls.append(kwargs) + return {"exit_code": 0, "stdout": "", "stderr": ""} + + +class _Validating(_Translating): + """Rejects the value ``rejected`` with a plain ``ValueError``.""" + + key = "fake-validating" + + def validate_dispatch_setting(self, name, value): + if value == "rejected": + raise ValueError(f"the CLI does not accept this {name}") + + +class _Unlabeled(_Translating): + """Raises its own ``DispatchSettingsError`` that names no setting.""" + + key = "fake-unlabeled" + supported_dispatch_settings = frozenset(DISPATCH_SETTINGS) + + def build_exec_args(self, prompt, **kwargs): + if any(name in kwargs for name in DISPATCH_SETTINGS): + raise DispatchSettingsError("the CLI rejected these settings") + return super().build_exec_args(prompt, **kwargs) + + +class _LegacyKwargsOverride(_Translating): + """Replaces the capable builder without declaring support itself.""" + + key = "fake-legacy-kwargs" + + def build_exec_args(self, prompt, **kwargs): + return [self.key, "-p", prompt] + + +class _StringDeclaration(IntegrationBase): + """Declares support with a string, which is not a set of settings.""" + + key = "fake-string-declaration" + supported_dispatch_settings = "reasoning_effort" + + def build_exec_args(self, prompt, **kwargs): + return [self.key, "-p", prompt] + + +def _forward_to_toml_builder(self, prompt, **kwargs): + """A Gemini builder that hands every keyword to ``TomlIntegration``.""" + return super(GeminiIntegration, self).build_exec_args(prompt, **kwargs) + + +class _StrictBuilder(IntegrationBase): + """A builder whose signature predates the dispatch settings.""" + + key = "fake-strict-builder" + + def __init__(self) -> None: + self.calls: list[str] = [] + + def build_exec_args( + self, + prompt, + *, + model=None, + output_json=True, + integration_args=None, + integration_options=None, + project_root=None, + ): + self.calls.append(prompt) + return [self.key, "-p", prompt] + + +class _StrictCommand(IntegrationBase): + """A ``dispatch_command`` whose signature predates the dispatch settings.""" + + key = "fake-strict-command" + + def __init__(self) -> None: + self.calls: list[str] = [] + + def dispatch_command( + self, + command_name, + args="", + *, + project_root=None, + model=None, + timeout=600, + stream=True, + integration_args=None, + integration_options=None, + output_json=None, + capture_max_bytes=None, + ): + self.calls.append(command_name) + return {"exit_code": 0, "stdout": "", "stderr": ""} + + +class _After(StepBase): + """Records that the run went on past the step under test.""" + + type_key = "dispatch-settings-after" + + def execute(self, config, context): + return StepResult(output={"ran": True}) + + +# -- Fixtures and helpers ------------------------------------------------- + + +@pytest.fixture +def register(monkeypatch): + """Register a fake integration for one test.""" + + def add(integration): + monkeypatch.setitem(INTEGRATION_REGISTRY, integration.key, integration) + return integration + + return add + + +@pytest.fixture +def cli(monkeypatch): + """Find every CLI and fake running it, recording each argv.""" + state = SimpleNamespace(runs=[], returncode=0, stderr="") + + def run(args, **kwargs): + state.runs.append(list(args)) + return SimpleNamespace(returncode=state.returncode, stdout="", stderr="") + + def run_captured(args, **kwargs): + state.runs.append(list(args)) + return CapturedProcess( + exit_code=state.returncode, stdout="", stderr=state.stderr + ) + + monkeypatch.setattr("shutil.which", lambda name: name) + monkeypatch.setattr("subprocess.run", run) + for module in ( + "specify_cli.integrations.base", + "specify_cli.workflows.step.prompt", + ): + monkeypatch.setattr(f"{module}.run_captured", run_captured) + return state + + +@pytest.fixture +def no_cli(monkeypatch): + """Find no CLI, so reaching executable detection means 'not installed'.""" + monkeypatch.setattr("shutil.which", lambda name: None) + + +@pytest.fixture +def without_builtin_supporters(monkeypatch): + """Withdraw built-in declarations so supporter lists name only fakes.""" + for integration in list(INTEGRATION_REGISTRY.values()): + for klass in type(integration).__mro__: + if vars(klass).get("supported_dispatch_settings"): + monkeypatch.setattr(klass, "supported_dispatch_settings", frozenset()) + + +@pytest.fixture +def after_step(monkeypatch): + monkeypatch.setitem(STEP_REGISTRY, _After.type_key, _After()) + return {"id": "after", "type": _After.type_key} + + +def _config(kind: str, **fields) -> dict: + return {"id": "ask", **STEPS[kind][1], **fields} + + +def _execute(kind, project_dir, integration, *, inputs=None, defaults=None, **fields): + defaults = defaults or {} + context = StepContext( + inputs=inputs or {}, + default_integration=integration, + project_root=str(project_dir), + default_reasoning_effort=defaults.get("reasoning_effort"), + default_context_size=defaults.get("context_size"), + ) + return STEPS[kind][0]().execute(_config(kind, **fields), context) + + +def _received(integration) -> dict: + """Return the dispatch settings *integration* last dispatched with.""" + call = integration.calls[-1] + return {name: call[name] for name in DISPATCH_SETTINGS if name in call} + + +def _outputs(result) -> dict: + return {name: result.output[name] for name in DISPATCH_SETTINGS} + + +def _assert_summarized(message: str) -> None: + """A malformed value is summarized, never echoed into the message.""" + assert len(message) < 400 + assert "\x1b" not in message + assert "\n" not in message + assert "SECRET" not in message + + +def _workflow(steps, *, defaults=None, inputs=None, workflow_id="wf"): + data = { + "schema_version": "1.0", + "workflow": { + "id": workflow_id, + "name": workflow_id, + "version": "1.0.0", + **(defaults or {}), + }, + "steps": steps, + } + if inputs: + data["inputs"] = inputs + return WorkflowDefinition(data) + + +def _install(project_dir, definition: WorkflowDefinition) -> None: + from specify_cli.workflows.catalog import WorkflowRegistry + + directory = project_dir / ".specify" / "workflows" / definition.id + directory.mkdir(parents=True, exist_ok=True) + (directory / "workflow.yml").write_text( + yaml.safe_dump(definition.data), encoding="utf-8" + ) + WorkflowRegistry(project_dir).add( + definition.id, {"version": "1.0.0", "enabled": True} + ) + + +# -- Resolution ----------------------------------------------------------- + + +@KINDS +class TestResolution: + @pytest.mark.parametrize( + ("fields", "defaults", "expected"), + [ + ( + {"reasoning_effort": "high"}, + {"reasoning_effort": "low"}, + {"reasoning_effort": "high"}, + ), + ({}, {"reasoning_effort": "low"}, {"reasoning_effort": "low"}), + ( + {"reasoning_effort": None}, + {"reasoning_effort": "low"}, + {"reasoning_effort": "low"}, + ), + ( + {"reasoning_effort": ""}, + {"reasoning_effort": "low"}, + {"reasoning_effort": "low"}, + ), + ({"reasoning_effort": None, "context_size": ""}, {}, {}), + ( + {"context_size": "200000"}, + {"reasoning_effort": "low", "context_size": "long_context"}, + {"reasoning_effort": "low", "context_size": "200000"}, + ), + ], + ids=[ + "step-wins", + "workflow-default", + "null-inherits", + "empty-inherits", + "unset", + "mixed-sources", + ], + ) + def test_step_value_then_workflow_default_then_unset( + self, kind, fields, defaults, expected, project_dir, register, cli + ): + fake = register(_Translating()) + result = _execute(kind, project_dir, fake.key, defaults=defaults, **fields) + + assert result.status is StepStatus.COMPLETED, result.error + assert _received(fake) == expected + assert _outputs(result) == { + name: expected.get(name) for name in DISPATCH_SETTINGS + } + + @pytest.mark.parametrize( + ("fields", "defaults", "expected"), + [ + ({"reasoning_effort": "{{ inputs.effort }}"}, {}, "medium"), + ({}, {"reasoning_effort": "{{ inputs.effort }}"}, "medium"), + ({"reasoning_effort": "x-{{ inputs.effort }}"}, {}, "x-medium"), + ], + ids=["step", "workflow-default", "interpolated"], + ) + def test_expressions_are_evaluated( + self, kind, fields, defaults, expected, project_dir, register, cli + ): + fake = register(_Translating()) + result = _execute( + kind, + project_dir, + fake.key, + inputs={"effort": "medium"}, + defaults=defaults, + **fields, + ) + + assert result.status is StepStatus.COMPLETED, result.error + assert _received(fake) == {"reasoning_effort": expected} + assert result.output["reasoning_effort"] == expected + + @pytest.mark.parametrize("effort", ["", None]) + @pytest.mark.parametrize( + ("fields", "defaults"), + [ + ({"reasoning_effort": "{{ inputs.effort }}"}, {"reasoning_effort": "low"}), + ({}, {"reasoning_effort": "{{ inputs.effort }}"}), + ], + ids=["step-opts-out-of-default", "default-resolves-empty"], + ) + def test_resolved_empty_value_is_unset_without_a_second_fallback( + self, kind, effort, fields, defaults, project_dir, register, cli + ): + fake = register(_Translating()) + result = _execute( + kind, + project_dir, + fake.key, + inputs={"effort": effort}, + defaults=defaults, + **fields, + ) + + assert result.status is StepStatus.COMPLETED, result.error + assert _received(fake) == {} + assert result.output["reasoning_effort"] is None + + +def test_resolution_records_where_each_set_value_came_from(): + context = StepContext( + default_reasoning_effort="low", default_context_size="long_context" + ) + resolved = resolve_dispatch_settings( + {"id": "ask", "context_size": "200000"}, context, "Command" + ) + + assert resolved.settings == {"reasoning_effort": "low", "context_size": "200000"} + assert list(resolved.settings) == list(DISPATCH_SETTINGS) + assert resolved.sources == { + "reasoning_effort": "workflow.reasoning_effort", + "context_size": "step", + } + assert resolved.outputs == resolved.settings + + unset = resolve_dispatch_settings({"id": "ask"}, StepContext(), "Command") + assert unset.settings == {} + assert unset.sources == {} + assert unset.outputs == dict.fromkeys(DISPATCH_SETTINGS) + + +@pytest.mark.parametrize( + ("setting", "sources", "expected"), + [ + ( + "context_size", + {"reasoning_effort": "step", "context_size": "workflow.context_size"}, + "(set by workflow.context_size)", + ), + ( + None, + {"context_size": "step", "reasoning_effort": "workflow.reasoning_effort"}, + "(settings: reasoning_effort from workflow.reasoning_effort; " + "context_size from step)", + ), + ( + "context_size", + {"reasoning_effort": "step"}, + "(settings: reasoning_effort from step)", + ), + (None, {}, ""), + ("reasoning_effort", {}, ""), + ], + ids=[ + "named", + "unnamed-lists-all", + "unset-setting-lists-all", + "none-set", + "named-none-set", + ], +) +def test_attribution_never_indexes_an_unknown_setting(setting, sources, expected): + assert dispatch_settings_attribution(setting, sources) == expected + + +# -- Validation ----------------------------------------------------------- + + +@KINDS +class TestValidation: + @pytest.mark.parametrize("name", DISPATCH_SETTINGS) + @pytest.mark.parametrize( + "value", [None, "", "high", "200000", "long_context", "{{ inputs.effort }}"] + ) + def test_valid_values_pass(self, kind, name, value): + assert STEPS[kind][0]().validate(_config(kind, **{name: value})) == [] + + @pytest.mark.parametrize("name", DISPATCH_SETTINGS) + @pytest.mark.parametrize("value", [200000, True, False, 1.5, ["high"]]) + def test_non_strings_are_rejected_with_a_quoting_hint(self, kind, name, value): + errors = STEPS[kind][0]().validate(_config(kind, **{name: value})) + + assert errors == [ + f"{LABELS[kind]} step 'ask': {dispatch_setting_value_error(name, value)}" + ] + assert f"'{name}' must be a string, got {type(value).__name__}" in errors[0] + assert "quote YAML values" in errors[0] + + @pytest.mark.parametrize("name", DISPATCH_SETTINGS) + @UNSAFE_VALUES + def test_unsafe_literals_are_rejected(self, kind, name, value): + errors = STEPS[kind][0]().validate(_config(kind, **{name: value})) + + assert len(errors) == 1 + assert errors[0].startswith( + f"{LABELS[kind]} step 'ask': '{name}' must be 1-64 letters" + ) + _assert_summarized(errors[0]) + + def test_oversized_literal_is_summarized(self, kind): + (error,) = STEPS[kind][0]().validate(_config(kind, reasoning_effort=HUGE)) + + _assert_summarized(error) + + +@KINDS +class TestResolvedValueValidation: + @pytest.mark.parametrize( + ("fields", "defaults", "inputs", "value", "source", "from_expression"), + [ + ( + {"reasoning_effort": "{{ inputs.value }}"}, + {}, + {"value": 5}, + 5, + "step", + True, + ), + ( + {"reasoning_effort": "{{ inputs.value }}"}, + {}, + {"value": True}, + True, + "step", + True, + ), + ( + {}, + {"reasoning_effort": "{{ inputs.value }}"}, + {"value": ["high"]}, + ["high"], + "workflow.reasoning_effort", + True, + ), + ({"reasoning_effort": 200000}, {}, {}, 200000, "step", False), + ( + {}, + {"reasoning_effort": False}, + {}, + False, + "workflow.reasoning_effort", + False, + ), + ], + ids=[ + "step-expression-int", + "step-expression-bool", + "default-expression-list", + "unvalidated-step-literal", + "unvalidated-default-literal", + ], + ) + def test_non_string_results_fail_the_step( + self, + kind, + fields, + defaults, + inputs, + value, + source, + from_expression, + project_dir, + register, + cli, + ): + fake = register(_Translating()) + result = _execute( + kind, project_dir, fake.key, inputs=inputs, defaults=defaults, **fields + ) + + type_name = type(value).__name__ + advice = ( + f"but its expression resolved to {type_name}; make it resolve to a " + "string, for example by reading an input declared with type: string." + if from_expression + else f"got {type_name}; quote YAML values such as 200000, on, or off." + ) + assert result.status is StepStatus.FAILED + assert result.error == ( + f"{LABELS[kind]} step 'ask': 'reasoning_effort' must be a string, " + f"{advice} (set by {source})" + ) + assert fake.calls == [] + assert cli.runs == [] + + @UNSAFE_VALUES + def test_unsafe_results_fail_the_step( + self, kind, value, project_dir, register, cli + ): + fake = register(_Translating()) + result = _execute( + kind, + project_dir, + fake.key, + inputs={"value": value}, + context_size="{{ inputs.value }}", + ) + + assert result.status is StepStatus.FAILED + assert result.error.startswith( + f"{LABELS[kind]} step 'ask': 'context_size' must be 1-64 letters" + ) + assert result.error.endswith(" (set by step)") + _assert_summarized(result.error) + assert cli.runs == [] + + def test_oversized_untrusted_result_is_summarized( + self, kind, project_dir, register, cli + ): + fake = register(_Translating()) + result = _execute( + kind, + project_dir, + fake.key, + inputs={"captured": HUGE}, + defaults={"reasoning_effort": "{{ inputs.captured }}"}, + ) + + assert result.status is StepStatus.FAILED + assert result.error.endswith(" (set by workflow.reasoning_effort)") + _assert_summarized(result.error) + assert cli.runs == [] + + +# -- Unsupported integrations --------------------------------------------- + + +@KINDS +class TestUnsupportedIntegrations: + @pytest.mark.parametrize("integration", ["gemini", "docker-agent"]) + @pytest.mark.parametrize("name", DISPATCH_SETTINGS) + def test_fails_before_executable_detection( + self, kind, integration, name, project_dir, no_cli, without_builtin_supporters + ): + result = _execute(kind, project_dir, integration, **{name: "high"}) + + assert result.status is StepStatus.FAILED + assert result.error == ( + f"{LABELS[kind]} step 'ask': " + + UNSUPPORTED.format(key=integration, name=name, supporters=NO_SUPPORTERS) + + " (set by step)" + ) + assert result.output["exit_code"] == 1 + assert result.output["dispatched"] is False + assert result.output[name] == "high" + + def test_names_the_source_of_the_first_rejected_setting( + self, kind, project_dir, no_cli + ): + result = _execute( + kind, + project_dir, + "gemini", + defaults={"reasoning_effort": "high"}, + context_size="long_context", + ) + + assert result.status is StepStatus.FAILED + assert "does not support per-step 'reasoning_effort'" in result.error + assert result.error.endswith(" (set by workflow.reasoning_effort)") + + def test_names_the_source_when_only_one_setting_is_unsupported( + self, kind, project_dir, register, no_cli + ): + fake = register(_EffortOnly()) + result = _execute( + kind, + project_dir, + fake.key, + defaults={"context_size": "long_context"}, + reasoning_effort="high", + ) + + assert result.status is StepStatus.FAILED + assert result.error.startswith( + f"{LABELS[kind]} step 'ask': Integration 'fake-effort-only' does not " + "support per-step 'context_size'." + ) + assert result.error.endswith(" (set by workflow.context_size)") + assert _outputs(result) == { + "reasoning_effort": "high", + "context_size": "long_context", + } + + def test_lists_the_integrations_that_support_it_on_this_path( + self, kind, project_dir, register, no_cli, without_builtin_supporters + ): + register(_Translating()) + register(_CommandOwner()) + result = _execute(kind, project_dir, "gemini", reasoning_effort="high") + + # Only the command step dispatches through dispatch_command. + supporters = { + "command": "fake-command-owner, fake-translating", + "prompt": "fake-translating", + }[kind] + assert result.error == ( + f"{LABELS[kind]} step 'ask': " + + UNSUPPORTED.format( + key="gemini", + name="reasoning_effort", + supporters=f"Integrations that support it: {supporters}.", + ) + + " (set by step)" + ) + + +# -- Supported integrations ----------------------------------------------- + + +@KINDS +class TestSupportedIntegrations: + @pytest.mark.parametrize( + "fake_type", [_Translating, _Inheriting], ids=["declaring", "inheriting"] + ) + @pytest.mark.parametrize( + "settings", + [ + {"reasoning_effort": "high"}, + {"context_size": "200000"}, + {"reasoning_effort": "xhigh", "context_size": "long_context"}, + ], + ids=["effort", "context", "both"], + ) + def test_receives_exactly_the_set_keywords( + self, kind, fake_type, settings, project_dir, register, cli + ): + fake = register(fake_type()) + result = _execute(kind, project_dir, fake.key, **settings) + + assert result.status is StepStatus.COMPLETED, result.error + assert _received(fake) == settings + flags = [] + if "reasoning_effort" in settings: + flags += ["--effort", settings["reasoning_effort"]] + if "context_size" in settings: + flags += ["--context", settings["context_size"]] + assert cli.runs == [[fake.key, "-p", PROMPTS[kind], *flags]] + + @pytest.mark.parametrize("capture", [False, True], ids=["streamed", "captured"]) + def test_settings_compose_with_capture( + self, kind, capture, project_dir, register, cli + ): + fake = register(_Translating()) + result = _execute( + kind, project_dir, fake.key, reasoning_effort="low", capture=capture + ) + + assert result.status is StepStatus.COMPLETED, result.error + assert _received(fake) == {"reasoning_effort": "low"} + assert cli.runs[-1][-2:] == ["--effort", "low"] + + +def test_command_step_dispatches_through_an_owning_dispatch_command( + project_dir, register, cli +): + fake = register(_CommandOwner()) + result = _execute("command", project_dir, fake.key, reasoning_effort="high") + + assert result.status is StepStatus.COMPLETED, result.error + assert _received(fake) == {"reasoning_effort": "high"} + + +def test_prompt_step_rejects_support_declared_only_on_dispatch_command( + project_dir, register, cli +): + """A prompt is built by ``build_exec_args``, which this fake does not own.""" + fake = register(_CommandOwner()) + result = _execute("prompt", project_dir, fake.key, reasoning_effort="high") + + assert result.status is StepStatus.FAILED + assert result.error.startswith( + "Prompt step 'ask': Integration 'fake-command-owner' does not support " + "per-step 'reasoning_effort'." + ) + assert fake.calls == [] + assert cli.runs == [] + + +# -- Declarations that are not backed by a translating builder ------------ + + +@pytest.fixture( + params=[ + "format-base-declaration", + "legacy-kwargs-override", + "string-declaration", + "forwards-to-super", + ] +) +def untranslated(request, monkeypatch, register): + """Return ``(key, problem)`` for an integration that would drop a setting.""" + for suffix in ("EXTRA_ARGS", "EXECUTABLE"): + monkeypatch.delenv(f"SPECKIT_INTEGRATION_GEMINI_{suffix}", raising=False) + unsupported = "does not support per-step 'reasoning_effort'" + if request.param in ("format-base-declaration", "forwards-to-super"): + # Gemini declaring support while TomlIntegration builds its argv. + monkeypatch.setattr( + GeminiIntegration, + "supported_dispatch_settings", + frozenset(DISPATCH_SETTINGS), + ) + if request.param == "format-base-declaration": + return "gemini", unsupported + if request.param == "forwards-to-super": + monkeypatch.setattr( + GeminiIntegration, "build_exec_args", _forward_to_toml_builder + ) + return "gemini", "does not translate per-step 'reasoning_effort'" + fake_type = { + "legacy-kwargs-override": _LegacyKwargsOverride, + "string-declaration": _StringDeclaration, + }[request.param] + return register(fake_type()).key, unsupported + + +@KINDS +def test_untranslated_declarations_never_dispatch(kind, untranslated, project_dir, cli): + key, problem = untranslated + assert _execute(kind, project_dir, key).status is StepStatus.COMPLETED + cli.runs.clear() + + result = _execute(kind, project_dir, key, reasoning_effort="high") + + assert result.status is StepStatus.FAILED + assert result.error.startswith( + f"{LABELS[kind]} step 'ask': Integration {key!r} {problem}." + ) + assert result.error.endswith(" (set by step)") + assert result.output["dispatched"] is False + assert cli.runs == [] + + +# -- Attribution of integration-raised errors ----------------------------- + + +@KINDS +class TestAttribution: + @pytest.mark.parametrize( + ("fields", "defaults", "message"), + [ + ( + {"reasoning_effort": "high"}, + {"context_size": "rejected"}, + "the CLI does not accept this context_size " + "(set by workflow.context_size)", + ), + ( + {"reasoning_effort": "rejected"}, + {"context_size": "long_context"}, + "the CLI does not accept this reasoning_effort (set by step)", + ), + ], + ids=["default-rejected", "step-rejected"], + ) + def test_validator_value_error_names_the_exact_source( + self, kind, fields, defaults, message, project_dir, register, cli + ): + fake = register(_Validating()) + result = _execute(kind, project_dir, fake.key, defaults=defaults, **fields) + + assert result.status is StepStatus.FAILED + assert result.error == f"{LABELS[kind]} step 'ask': {message}" + assert result.output["exit_code"] == 1 + assert result.output["dispatched"] is False + assert cli.runs == [] + + def test_unlabeled_integration_error_lists_every_source( + self, kind, project_dir, register, cli + ): + fake = register(_Unlabeled()) + result = _execute( + kind, + project_dir, + fake.key, + defaults={"context_size": "long_context"}, + reasoning_effort="high", + ) + + assert result.status is StepStatus.FAILED + assert result.error == ( + f"{LABELS[kind]} step 'ask': the CLI rejected these settings " + "(settings: reasoning_effort from step; " + "context_size from workflow.context_size)" + ) + assert result.output["dispatched"] is False + assert cli.runs == [] + + @pytest.mark.parametrize("continue_on_error", [True, False]) + @pytest.mark.parametrize( + ("fake_type", "defaults", "attribution"), + [ + ( + _Validating, + {"context_size": "rejected"}, + "(set by workflow.context_size)", + ), + ( + _Unlabeled, + {"context_size": "long_context"}, + "(settings: reasoning_effort from step; " + "context_size from workflow.context_size)", + ), + ], + ids=["validator", "unlabeled"], + ) + def test_rejection_in_a_run_fails_only_the_step( + self, + kind, + continue_on_error, + fake_type, + defaults, + attribution, + project_dir, + register, + cli, + after_step, + ): + fake = register(fake_type()) + steps = [ + _config( + kind, + integration=fake.key, + reasoning_effort="high", + continue_on_error=continue_on_error, + ), + after_step, + ] + state = WorkflowEngine(project_dir).execute(_workflow(steps, defaults=defaults)) + + assert state.step_results["ask"]["status"] == "failed" + assert state.step_results["ask"]["error"].endswith(f" {attribution}") + if continue_on_error: + assert state.status is RunStatus.COMPLETED + assert state.step_results["after"]["output"] == {"ran": True} + else: + assert state.status is RunStatus.FAILED + assert "after" not in state.step_results + assert cli.runs == [] + + +# -- Unset invariant ------------------------------------------------------ + + +@pytest.mark.parametrize( + ("kind", "fake_type"), + [ + ("command", _StrictCommand), + ("command", _StrictBuilder), + ("prompt", _StrictBuilder), + ], + ids=["command-dispatch-command", "command-base-dispatch-command", "prompt-builder"], +) +@pytest.mark.parametrize( + ("fields", "defaults"), + [ + ({}, {}), + ({"reasoning_effort": None, "context_size": ""}, {"reasoning_effort": ""}), + ], + ids=["absent", "null-and-empty"], +) +def test_unset_settings_pass_no_new_keywords( + kind, fake_type, fields, defaults, project_dir, register, cli +): + """A strict signature would raise TypeError on any added keyword.""" + fake = register(fake_type()) + result = _execute(kind, project_dir, fake.key, defaults=defaults, **fields) + + assert result.status is StepStatus.COMPLETED, result.error + assert fake.calls + assert _outputs(result) == dict.fromkeys(DISPATCH_SETTINGS) + + +@KINDS +def test_output_keys_are_null_when_dispatch_is_impossible( + kind, project_dir, register, no_cli +): + fake = register(_Translating()) + result = _execute(kind, project_dir, fake.key) + + assert result.status is StepStatus.FAILED + assert "CLI not found" in result.error + assert _outputs(result) == dict.fromkeys(DISPATCH_SETTINGS) + + +@pytest.mark.parametrize( + "fields", [{}, {"reasoning_effort": "high"}], ids=["unset", "set"] +) +def test_prompt_step_still_raises_a_malformed_extra_args_error( + fields, project_dir, monkeypatch, register, cli +): + """Only a rejected dispatch setting is turned into a failed prompt step.""" + fake = register(_Translating()) + monkeypatch.setenv("SPECKIT_INTEGRATION_FAKE_TRANSLATING_EXTRA_ARGS", '"open') + + with pytest.raises(ValueError, match="EXTRA_ARGS is not parseable") as raised: + _execute("prompt", project_dir, fake.key, **fields) + + assert not isinstance(raised.value, DispatchSettingsError) + assert cli.runs == [] + + +# -- Native CLI rejection ------------------------------------------------- + + +@KINDS +class TestNativeRejection: + def test_captured_stderr_stays_in_the_output( + self, kind, project_dir, register, cli + ): + fake = register(_Translating()) + cli.returncode = 2 + cli.stderr = "error: unknown reasoning effort 'turbo'" + result = _execute( + kind, project_dir, fake.key, reasoning_effort="turbo", capture=True + ) + + assert result.status is StepStatus.FAILED + assert result.error == f"{LABELS[kind]} exited with code 2" + assert result.output["stderr"] == cli.stderr + assert result.output["reasoning_effort"] == "turbo" + + def test_streamed_rejection_reports_a_generic_error( + self, kind, project_dir, register, cli + ): + fake = register(_Translating()) + cli.returncode = 2 + result = _execute(kind, project_dir, fake.key, reasoning_effort="turbo") + + assert result.status is StepStatus.FAILED + assert result.error == f"{LABELS[kind]} exited with code 2" + assert result.output["stderr"] == "" + + @pytest.mark.parametrize("continue_on_error", [True, False]) + @pytest.mark.parametrize("capture", [True, False], ids=["captured", "streamed"]) + def test_rejection_in_a_run_keeps_stderr_out_of_the_error_and_log( + self, kind, capture, continue_on_error, project_dir, register, cli, after_step + ): + fake = register(_Translating()) + cli.returncode = 2 + cli.stderr = "error: unknown reasoning effort 'turbo'" + steps = [ + _config( + kind, + integration=fake.key, + reasoning_effort="turbo", + capture=capture, + continue_on_error=continue_on_error, + ), + after_step, + ] + state = WorkflowEngine(project_dir).execute(_workflow(steps)) + + ask = state.step_results["ask"] + assert ask["error"] == f"{LABELS[kind]} exited with code 2" + assert ask["output"]["stderr"] == (cli.stderr if capture else "") + log = ( + project_dir / ".specify" / "workflows" / "runs" / state.run_id / "log.jsonl" + ).read_text(encoding="utf-8") + assert "unknown reasoning effort" not in log + assert state.status is ( + RunStatus.COMPLETED if continue_on_error else RunStatus.FAILED + ) + assert ("after" in state.step_results) is continue_on_error + + +# -- Workflow defaults and engine wiring ---------------------------------- + +_PROMPT = {"id": "ask", "type": "prompt", "prompt": "hi"} + + +class TestWorkflowDefaults: + @pytest.mark.parametrize("name", DISPATCH_SETTINGS) + @pytest.mark.parametrize( + "value", [None, "", "high", "200000", "{{ inputs.effort }}"] + ) + def test_valid_defaults_pass(self, name, value): + definition = _workflow([_PROMPT], defaults={name: value}) + + assert validate_workflow(definition) == [] + assert getattr(definition, f"default_{name}") == value + + @pytest.mark.parametrize("name", DISPATCH_SETTINGS) + @pytest.mark.parametrize("value", [200000, True, ["token=SECRET"]]) + def test_non_string_defaults_are_rejected_without_echoing_them( + self, name, value + ): + errors = validate_workflow(_workflow([_PROMPT], defaults={name: value})) + + assert errors == [dispatch_setting_value_error(f"workflow.{name}", value)] + assert errors[0].startswith( + f"'workflow.{name}' must be a string, got {type(value).__name__}" + ) + assert "quote YAML values" in errors[0] + assert "SECRET" not in errors[0] + + @pytest.mark.parametrize("name", DISPATCH_SETTINGS) + @UNSAFE_VALUES + def test_unsafe_default_literals_are_rejected(self, name, value): + errors = validate_workflow(_workflow([_PROMPT], defaults={name: value})) + + assert len(errors) == 1 + assert errors[0].startswith(f"'workflow.{name}' must be 1-64 letters") + _assert_summarized(errors[0]) + + def test_oversized_default_is_summarized(self): + (error,) = validate_workflow( + _workflow([_PROMPT], defaults={"context_size": HUGE}) + ) + + _assert_summarized(error) + + def test_run_rejects_an_invalid_default_before_any_step( + self, project_dir, register, cli + ): + fake = register(_Translating()) + definition = _workflow( + [{**_PROMPT, "integration": fake.key}], + defaults={"reasoning_effort": "high effort"}, + ) + + with pytest.raises(ValueError, match=r"'workflow\.reasoning_effort' must be"): + WorkflowEngine(project_dir).execute(definition) + assert fake.calls == [] + + +@KINDS +class TestEngineWiring: + DEFAULTS = {"reasoning_effort": "high", "context_size": "long_context"} + + def test_run_passes_workflow_defaults_and_records_them( + self, kind, project_dir, register, cli + ): + fake = register(_Translating()) + definition = _workflow( + [_config(kind, integration=fake.key)], defaults=self.DEFAULTS + ) + state = WorkflowEngine(project_dir).execute(definition) + + assert state.status is RunStatus.COMPLETED, state.error + assert _received(fake) == self.DEFAULTS + assert state.step_results["ask"]["output"]["reasoning_effort"] == "high" + assert state.step_results["ask"]["output"]["context_size"] == "long_context" + + def test_unset_output_keys_are_null_and_not_recorded_at_top_level( + self, kind, project_dir, register, cli + ): + fake = register(_Translating()) + state = WorkflowEngine(project_dir).execute( + _workflow([_config(kind, integration=fake.key)]) + ) + + assert state.status is RunStatus.COMPLETED, state.error + result = state.step_results["ask"] + assert {name: result["output"][name] for name in DISPATCH_SETTINGS} == ( + dict.fromkeys(DISPATCH_SETTINGS) + ) + assert not set(DISPATCH_SETTINGS) & set(result) + assert _received(fake) == {} + + def test_resume_passes_workflow_defaults(self, kind, project_dir, register, cli): + fake = register(_Translating()) + engine = WorkflowEngine(project_dir) + cli.returncode = 1 + state = engine.execute( + _workflow([_config(kind, integration=fake.key)], defaults=self.DEFAULTS) + ) + assert state.status is RunStatus.FAILED + fake.calls.clear() + + cli.returncode = 0 + resumed = engine.resume(state.run_id) + + assert resumed.status is RunStatus.COMPLETED, resumed.error + assert _received(fake) == self.DEFAULTS + assert len(cli.runs) == 2 + + def test_resume_re_resolves_settings_from_new_inputs( + self, kind, project_dir, register, cli + ): + fake = register(_Translating()) + engine = WorkflowEngine(project_dir) + step = _config( + kind, integration=fake.key, reasoning_effort="{{ inputs.effort }}" + ) + definition = _workflow( + [step], inputs={"effort": {"type": "string", "default": "low"}} + ) + cli.returncode = 1 + state = engine.execute(definition) + assert state.status is RunStatus.FAILED + assert _received(fake) == {"reasoning_effort": "low"} + + cli.returncode = 0 + resumed = engine.resume(state.run_id, {"effort": "high"}) + + assert resumed.status is RunStatus.COMPLETED, resumed.error + assert _received(fake) == {"reasoning_effort": "high"} + assert resumed.step_results["ask"]["output"]["reasoning_effort"] == "high" + + def test_number_input_expression_fails_with_expression_advice( + self, kind, project_dir, register, cli + ): + fake = register(_Translating()) + step = _config(kind, integration=fake.key, context_size="{{ inputs.window }}") + definition = _workflow( + [step], inputs={"window": {"type": "number", "default": 200000}} + ) + + state = WorkflowEngine(project_dir).execute(definition) + + assert state.status is RunStatus.FAILED + assert state.step_results["ask"]["error"] == ( + f"{LABELS[kind]} step 'ask': 'context_size' must be a string, but its " + "expression resolved to int; make it resolve to a string, for example " + "by reading an input declared with type: string. (set by step)" + ) + assert fake.calls == [] + assert cli.runs == [] + + def test_string_input_expression_passes_a_number_through( + self, kind, project_dir, register, cli + ): + fake = register(_Translating()) + step = _config(kind, integration=fake.key, context_size="{{ inputs.window }}") + definition = _workflow( + [step], inputs={"window": {"type": "string", "default": "200000"}} + ) + + state = WorkflowEngine(project_dir).execute(definition) + + assert state.status is RunStatus.COMPLETED, state.error + assert _received(fake) == {"context_size": "200000"} + + @pytest.mark.parametrize( + ("child_defaults", "expected"), + [({"reasoning_effort": "low"}, {"reasoning_effort": "low"}), ({}, {})], + ids=["own-defaults", "no-defaults"], + ) + def test_called_workflow_uses_only_its_own_defaults( + self, kind, child_defaults, expected, project_dir, register, cli + ): + fake = register(_Translating()) + _install( + project_dir, + _workflow( + [_config(kind, integration=fake.key)], + defaults=child_defaults, + workflow_id="child", + ), + ) + parent = _workflow( + [{"id": "call", "type": "workflow", "workflow": "child"}], + defaults=self.DEFAULTS, + ) + state = WorkflowEngine(project_dir).execute(parent) + + assert state.status is RunStatus.COMPLETED, state.error + assert len(cli.runs) == 1 + assert _received(fake) == expected + + def test_fan_out_items_inherit_workflow_defaults( + self, kind, project_dir, register, cli + ): + fake = register(_Translating()) + definition = _workflow( + [ + { + "id": "fan", + "type": "fan-out", + "items": [1, 2], + "step": _config(kind, integration=fake.key), + } + ], + defaults=self.DEFAULTS, + ) + state = WorkflowEngine(project_dir).execute(definition) + + assert state.status is RunStatus.COMPLETED, state.error + assert [run[-4:] for run in cli.runs] == [ + ["--effort", "high", "--context", "long_context"] + ] * 2 + + +def test_step_context_positional_construction_is_unchanged(): + context = StepContext( + {"spec": "x"}, + {}, + None, + False, + {}, + "claude", + "sonnet", + {"max_tokens": 1}, + "/project", + "run-1", + True, + "/workflows/wf", + ) + + assert context.project_root == "/project" + assert context.run_id == "run-1" + assert context.is_resume is True + assert context.workflow_dir == "/workflows/wf" + assert context.default_reasoning_effort is None + assert context.default_context_size is None + names = [field.name for field in dataclasses.fields(StepContext)] + assert names[8] == "project_root" + assert names[-2:] == ["default_reasoning_effort", "default_context_size"] From 964adcbdd56f96049b7a253b13df8202c631ad18 Mon Sep 17 00:00:00 2001 From: Sukanth Gunda Date: Thu, 5 Nov 2026 02:54:30 -0600 Subject: [PATCH 2/3] fix(workflows): keep rejected dispatch setting values out of errors A reasoning_effort or context_size value can come from an expression over captured agent output. When such a value failed the host's format check, the error quoted a 24-character repr prefix of it. Step errors are printed and written to log.jsonl as step_failed events, so captured text could reach both, contrary to the documented guarantee that captured text is neither printed nor written to log.jsonl. The error now reports only the value's type and length. The design doc and the validate_dispatch_setting() docstring tell adapter authors to keep the value out of their own errors for the same reason. Part of #4916. Assisted-by: GitHub Copilot (model: Claude Opus 5.5, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- design/integration.md | 10 ++-- docs/workflow-steps/types.md | 3 +- src/specify_cli/integrations/base.py | 15 ++---- src/specify_cli/workflows/engine.py | 4 +- tests/integrations/test_base.py | 32 ++++++++---- tests/workflows/test_dispatch_settings.py | 64 +++++++++++++++++++---- 6 files changed, 92 insertions(+), 36 deletions(-) diff --git a/design/integration.md b/design/integration.md index 8076ffeba2..ad2c41461b 100644 --- a/design/integration.md +++ b/design/integration.md @@ -84,7 +84,9 @@ Built-in integrations and external adapters opt in per setting with these `ValueError` to reject a value the CLI would ignore or misread, and the host attributes the failure to that setting. If the receiving method itself must reject a setting, raise `DispatchSettingsError` with `setting`; without it, - the step error lists every requested setting with its source. + the step error lists every requested setting with its source. Keep the value + out of either error's message: it can come from captured output, which Spec + Kit does not print to the terminal or write to `log.jsonl`. - `dispatch_setting_hint(setting)`: optional remediation text, such as another step field to use instead, which the host appends to its error for an unsupported setting. The default returns `None`. @@ -100,9 +102,9 @@ accepts the keyword or `**kwargs`. It then calls `validate_dispatch_setting()`. Each failure raises `DispatchSettingsError`, a `ValueError` whose `setting` attribute names the setting, so the step can report where its value came from. An unsupported-setting error lists the integrations that support the setting, -and a malformed value is summarized rather than echoed. A translating method -can also run the gate itself, with *via* naming that method, so that direct -calls are checked too. +and an error about a malformed value reports only its type and length, never +the value. A translating method can also run the gate itself, with *via* +naming that method, so that direct calls are checked too. The host cannot observe what the receiving method does with a declared setting. An integration that declares support and then ignores the value, for diff --git a/docs/workflow-steps/types.md b/docs/workflow-steps/types.md index cf857d07c6..9a22906ce0 100644 --- a/docs/workflow-steps/types.md +++ b/docs/workflow-steps/types.md @@ -245,7 +245,8 @@ not `context_size: 200000`. An expression must resolve to a string too, so read a number from an input declared with `type: string`, not `type: number`. A value must be 1 to 64 letters, digits, `.`, `_`, or `-`, starting with a letter or digit. Literal values are checked when the workflow is validated, and -expression results when the step runs. Spec Kit passes the value to the +expression results when the step runs. An error about a malformed value reports +only its type and length, not its text. Spec Kit passes the value to the integration unchanged. Its meaning and accepted values are defined by the integration's CLI, and an integration can reject a value before the CLI runs. diff --git a/src/specify_cli/integrations/base.py b/src/specify_cli/integrations/base.py index 5b1a9bf192..6bbc59e2da 100644 --- a/src/specify_cli/integrations/base.py +++ b/src/specify_cli/integrations/base.py @@ -96,7 +96,6 @@ def resolve_registrar_config(integration: IntegrationBase) -> dict[str, Any]: # metacharacters (relevant to Windows ``.cmd`` shims), without restricting any # integration's vocabulary. _DISPATCH_SETTING_VALUE = re.compile(r"[A-Za-z0-9][A-Za-z0-9._-]{0,63}") -_DISPATCH_SETTING_PREVIEW = 24 class DispatchSettingsError(ValueError): @@ -121,13 +120,6 @@ def requested_dispatch_settings( } -def _summarize_untrusted_value(value: str) -> str: - preview = value[:_DISPATCH_SETTING_PREVIEW] - ellipsis = "…" if len(value) > _DISPATCH_SETTING_PREVIEW else "" - # repr() escapes control characters, so the summary is always one safe line. - return f"a string of length {len(value)} ({preview!r}{ellipsis})" - - def dispatch_setting_value_error( name: str, value: object, *, from_expression: bool = False ) -> str | None: @@ -136,7 +128,8 @@ def dispatch_setting_value_error( *from_expression* marks a value that a workflow expression produced, so a non-string gets advice about the expression instead of YAML quoting. A malformed value can come from untrusted expression results (for example - captured agent output), so it is summarized rather than echoed. + captured agent output, which Spec Kit does not print or log), so the + message reports only the value's type and length, never its text. """ if not isinstance(value, str): type_name = type(value).__name__ @@ -153,7 +146,7 @@ def dispatch_setting_value_error( if _DISPATCH_SETTING_VALUE.fullmatch(value) is None: return ( f"'{name}' must be 1-64 letters, digits, '.', '_', or '-' starting " - f"with a letter or digit, got {_summarize_untrusted_value(value)}." + f"with a letter or digit, got a string of length {len(value)}." ) return None @@ -531,6 +524,8 @@ def validate_dispatch_setting(self, name: str, value: str) -> None: Called by :func:`check_dispatch_settings` for each supported setting after the host's own checks. Values are opaque by default; override to reject values the CLI would ignore or misread, raising ``ValueError``. + Keep *value* out of the message: it can come from captured agent + output, which Spec Kit does not print or log. """ def dispatch_setting_hint(self, setting: str) -> str | None: diff --git a/src/specify_cli/workflows/engine.py b/src/specify_cli/workflows/engine.py index d292aad6d9..14aaf80a69 100644 --- a/src/specify_cli/workflows/engine.py +++ b/src/specify_cli/workflows/engine.py @@ -214,8 +214,8 @@ def _dispatch_default_errors(definition: WorkflowDefinition) -> list[str]: ) # ``null`` and ``""`` leave a dispatch setting unset, and the step checks an - # expression once it is resolved. A malformed value is summarized rather - # than echoed. + # expression once it is resolved. An error reports only a malformed value's + # type and length. for name in DISPATCH_SETTINGS: value = getattr(definition, f"default_{name}") if value is None or value == "" or (isinstance(value, str) and "{{" in value): diff --git a/tests/integrations/test_base.py b/tests/integrations/test_base.py index 0a30f1cce6..f901536248 100644 --- a/tests/integrations/test_base.py +++ b/tests/integrations/test_base.py @@ -1055,15 +1055,29 @@ def test_expression_flag_changes_only_the_non_string_advice(self): "reasoning_effort", "-rf", from_expression=True ) == dispatch_setting_value_error("reasoning_effort", "-rf") - def test_malformed_values_are_summarized_not_echoed(self): - value = "\x1b[31m" + "x" * (1024 * 1024) + "\nsecret" - message = dispatch_setting_value_error("reasoning_effort", value) - assert message is not None - assert len(message) < 300 - assert "\x1b" not in message - assert "\n" not in message - assert "secret" not in message - assert f"length {len(value)}" in message + @pytest.mark.parametrize( + "value", + [ + pytest.param("SECRET-api-key-0123456789 tail", id="leading-secret"), + pytest.param("SECRET!", id="short-secret"), + pytest.param("-SECRET", id="leading-dash"), + pytest.param("\x1b[31m" + "x" * (1024 * 1024) + "\nSECRET", id="huge"), + ], + ) + @pytest.mark.parametrize("from_expression", [False, True]) + def test_malformed_values_report_only_type_and_length( + self, value, from_expression + ): + """A value can be captured output, which Spec Kit does not print or log.""" + message = dispatch_setting_value_error( + "reasoning_effort", value, from_expression=from_expression + ) + + assert message == ( + "'reasoning_effort' must be 1-64 letters, digits, '.', '_', or '-' " + f"starting with a letter or digit, got a string of length {len(value)}." + ) + assert "SECRET" not in message class TestEffectiveDispatchSettings: diff --git a/tests/workflows/test_dispatch_settings.py b/tests/workflows/test_dispatch_settings.py index c5b9c6b616..272bc51e44 100644 --- a/tests/workflows/test_dispatch_settings.py +++ b/tests/workflows/test_dispatch_settings.py @@ -8,6 +8,7 @@ from __future__ import annotations import dataclasses +import json from types import SimpleNamespace import pytest @@ -253,7 +254,7 @@ def add(integration): @pytest.fixture def cli(monkeypatch): """Find every CLI and fake running it, recording each argv.""" - state = SimpleNamespace(runs=[], returncode=0, stderr="") + state = SimpleNamespace(runs=[], returncode=0, stdout="", stderr="") def run(args, **kwargs): state.runs.append(list(args)) @@ -262,7 +263,7 @@ def run(args, **kwargs): def run_captured(args, **kwargs): state.runs.append(list(args)) return CapturedProcess( - exit_code=state.returncode, stdout="", stderr=state.stderr + exit_code=state.returncode, stdout=state.stdout, stderr=state.stderr ) monkeypatch.setattr("shutil.which", lambda name: name) @@ -322,8 +323,10 @@ def _outputs(result) -> dict: return {name: result.output[name] for name in DISPATCH_SETTINGS} -def _assert_summarized(message: str) -> None: - """A malformed value is summarized, never echoed into the message.""" +def _assert_summarized(message: str, value: str) -> None: + """A malformed value is reported by its length, never echoed into the message.""" + assert f"got a string of length {len(value)}." in message + assert value not in message assert len(message) < 400 assert "\x1b" not in message assert "\n" not in message @@ -552,12 +555,12 @@ def test_unsafe_literals_are_rejected(self, kind, name, value): assert errors[0].startswith( f"{LABELS[kind]} step 'ask': '{name}' must be 1-64 letters" ) - _assert_summarized(errors[0]) + _assert_summarized(errors[0], value) def test_oversized_literal_is_summarized(self, kind): (error,) = STEPS[kind][0]().validate(_config(kind, reasoning_effort=HUGE)) - _assert_summarized(error) + _assert_summarized(error, HUGE) @KINDS @@ -658,7 +661,7 @@ def test_unsafe_results_fail_the_step( f"{LABELS[kind]} step 'ask': 'context_size' must be 1-64 letters" ) assert result.error.endswith(" (set by step)") - _assert_summarized(result.error) + _assert_summarized(result.error, value) assert cli.runs == [] def test_oversized_untrusted_result_is_summarized( @@ -675,9 +678,50 @@ def test_oversized_untrusted_result_is_summarized( assert result.status is StepStatus.FAILED assert result.error.endswith(" (set by workflow.reasoning_effort)") - _assert_summarized(result.error) + _assert_summarized(result.error, HUGE) assert cli.runs == [] + @pytest.mark.parametrize("continue_on_error", [True, False]) + def test_rejected_captured_output_stays_out_of_the_error_and_log( + self, kind, continue_on_error, project_dir, register, cli, after_step + ): + """Spec Kit does not print or log captured text, even as a rejected value.""" + fake = register(_Translating()) + cli.stdout = "SECRET-api-key-0123456789 tail" + steps = [ + _config(kind, id="read", integration=fake.key, capture=True), + _config( + kind, + integration=fake.key, + reasoning_effort="{{ steps.read.output.stdout }}", + continue_on_error=continue_on_error, + ), + after_step, + ] + state = WorkflowEngine(project_dir).execute(_workflow(steps)) + + assert state.step_results["read"]["output"]["stdout"] == cli.stdout + error = state.step_results["ask"]["error"] + assert error == ( + f"{LABELS[kind]} step 'ask': 'reasoning_effort' must be 1-64 letters, " + "digits, '.', '_', or '-' starting with a letter or digit, got a " + f"string of length {len(cli.stdout)}. (set by step)" + ) + log = ( + project_dir / ".specify" / "workflows" / "runs" / state.run_id / "log.jsonl" + ).read_text(encoding="utf-8") + failure = "step_continue_on_error" if continue_on_error else "step_failed" + events = [json.loads(line) for line in log.splitlines()] + assert [event["error"] for event in events if event["event"] == failure] == [ + error + ] + assert "SECRET" not in log + assert len(cli.runs) == 1 + assert state.status is ( + RunStatus.COMPLETED if continue_on_error else RunStatus.FAILED + ) + assert ("after" in state.step_results) is continue_on_error + # -- Unsupported integrations --------------------------------------------- @@ -1158,14 +1202,14 @@ def test_unsafe_default_literals_are_rejected(self, name, value): assert len(errors) == 1 assert errors[0].startswith(f"'workflow.{name}' must be 1-64 letters") - _assert_summarized(errors[0]) + _assert_summarized(errors[0], value) def test_oversized_default_is_summarized(self): (error,) = validate_workflow( _workflow([_PROMPT], defaults={"context_size": HUGE}) ) - _assert_summarized(error) + _assert_summarized(error, HUGE) def test_run_rejects_an_invalid_default_before_any_step( self, project_dir, register, cli From 553667f36d33d73510796d697a882e4bbed751ce Mon Sep 17 00:00:00 2001 From: Sukanth Gunda Date: Fri, 6 Nov 2026 13:11:31 -0600 Subject: [PATCH 3/3] docs(integrations): document per-step dispatch settings in the API reference #5296 made docs/integrations/api.md the public reference for the integration adapter contract and replaced the docs' links to design/integration.md. Add the opt-in contract for per-step reasoning_effort and context_size there, condensed from the design document, and point the CLI reference's opt-in link at it. The documented build_exec_args() signature is unchanged: an adapter that does not opt in never receives either keyword. Part of #4916. Assisted-by: GitHub Copilot (model: Claude Opus 5.5, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- docs/integrations/api.md | 46 ++++++++++++++++++++++++++++++++++++++++ docs/integrations/cli.md | 2 +- 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/docs/integrations/api.md b/docs/integrations/api.md index c06fa1cb04..4932c8cc7a 100644 --- a/docs/integrations/api.md +++ b/docs/integrations/api.md @@ -244,6 +244,52 @@ CLI adapter declares none, tool checking probes its runtime executable. Version detection is agent-specific; declaring a version constraint does not supply a universal `--version` invocation. +### Per-step dispatch settings + +Workflow `command` and `prompt` steps can request +[`reasoning_effort` and `context_size`](../workflow-steps/types.md#reasoning-effort-and-context-size) +for one CLI invocation. `DISPATCH_SETTINGS` in `specify_cli.integrations.base` +lists them in the order they are checked. Values are opaque strings whose +meaning belongs to the agent's CLI. + +The `build_exec_args()` signature above does not change, and an adapter that +does not opt in never receives either keyword: a step that requests one fails +before the CLI runs. Prompt steps pass requested settings to +`build_exec_args()`. Command steps pass them to `dispatch_command()`, whose +base implementation forwards them to `build_exec_args()`. The *receiving +method* is therefore `build_exec_args()`, or `dispatch_command()` for a command +step when the integration overrides it. To opt in: + +- Declare `supported_dispatch_settings`, a `frozenset` of the settings the + integration translates, on the class that defines the receiving method. A + subclass that overrides that method must declare support again. A + declaration on any other class, or one that is not a set or frozenset, + declares nothing. +- Accept a `reasoning_effort=None` or `context_size=None` keyword on the + receiving method for each declared setting. The host passes a keyword only + when a step sets that setting, and never passes `""`. Translate the value + into CLI arguments placed after those from + `SPECKIT_INTEGRATION__EXTRA_ARGS`, and do not pass it on: + `IntegrationBase.build_exec_args()` and the format-base builders translate + neither setting and raise `DispatchSettingsError` rather than drop a value. +- Optionally, override `validate_dispatch_setting(name, value)` to raise + `ValueError` for a value the CLI would ignore or misread, and + `dispatch_setting_hint(setting)` to return remediation text that the host + appends to its error for an unsupported setting. + +Before the CLI runs, the host gate `check_dispatch_settings()` checks each +requested setting in `DISPATCH_SETTINGS` order. The value must be 1 to 64 +letters, digits, `.`, `_`, or `-`, starting with a letter or digit; the +integration must declare the setting as above; and the receiving method must +accept the keyword or `**kwargs`. The gate then calls +`validate_dispatch_setting()`. Each failure raises `DispatchSettingsError`, a +`ValueError` whose `setting` attribute names the setting; if the receiving +method itself must reject a value, raise it with `setting`. Keep the value out +of every error message: it can come from captured output, which Spec Kit does +not print to the terminal or write to `log.jsonl`. The host cannot detect a +declared setting that the receiving method accepts and then ignores, for +example through `**kwargs`, so a declaration is a promise to translate it. + ## Setup and file ownership Format bases implement setup and teardown. Custom layouts may override: diff --git a/docs/integrations/cli.md b/docs/integrations/cli.md index d644b4ba02..d67252af5d 100644 --- a/docs/integrations/cli.md +++ b/docs/integrations/cli.md @@ -491,7 +491,7 @@ settings of `command` and `prompt` steps instead follow one shared contract. Spec Kit checks each value and passes it only to an integration that declares and translates that setting; with any other integration, the step fails before the CLI runs. Adapter authors opt in as described in the -[integration design](https://github.com/github/spec-kit/blob/main/design/integration.md#per-step-dispatch-settings). +[integration API reference](api.md#per-step-dispatch-settings). ## FAQ