mirror of
https://github.com/github/spec-kit.git
synced 2026-08-03 06:26:30 +08:00
bd90f766fb057ff332cb4a88e76acc768ada7c74
83 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Ali jawwad
|
0add7131c9 |
fix(workflows): workflow add detects local YAML files case-insensitively (#3633)
`workflow add` gated the local-file branches on a case-SENSITIVE
`.suffix in (".yml", ".yaml")` (the `--dev` branch and the plain
local-path branch), while every other YAML-file detector in the CLI
normalizes case: `workflow run` uses `source_path.suffix.lower()` and
`WorkflowEngine.load_workflow` uses `path.suffix.lower()`.
The result was an add/run inconsistency: `specify workflow run Sample.YAML`
loads the file, but `specify workflow add Sample.YAML` does not recognize
it as a local workflow — the `--dev` branch rejects it with "--dev source
must be a workflow YAML file ..." and the plain path falls through to a
catalog lookup that fails with "not found in catalog".
Add `.lower()` to both suffix reads so `workflow add` matches its siblings.
The lowercase happy path is unchanged.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
Ali jawwad
|
cef00a1cb3 |
fix(workflows): list-literal expression ignores trailing/empty commas (#3631)
A workflow list-literal expression with a trailing (or leading/double) comma —
'{{ [1, 2,] }}' — evaluated to [1, 2, None]: _split_top_level_commas returns a
trailing empty segment, which _evaluate_simple_expression resolves as an empty
dot-path to None. That silently widens membership tests and renders a stray
None in joins. Python and Jinja2 both tolerate trailing commas.
Drop whitespace-empty segments from the comprehension. An intentional
empty-string element ('') survives because its segment strips to "''" (truthy),
so ['', 'a'] is preserved. Completes the quoted-comma handling from #3134.
Test: [1, 2,] and [1,, 2] -> [1, 2]; ['', 'a'] -> ['', 'a'] (fails before:
trailing None).
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
Ali jawwad
|
03f9013a7b |
fix(workflows): StepRegistry.add tolerates a corrupted non-dict existing entry (#3630)
StepRegistry.add read existing = self.data['steps'].get(step_id, {}) then called
existing.get('installed_at', ...). A corrupted-but-parseable registry holding a
non-dict entry (e.g. {'steps': {'foo': 'corrupted'}}) — which _load() accepts,
since it validates only the top-level dict and that 'steps' is a dict — made
add() raise AttributeError. WorkflowRegistry.add was hardened for exactly this
(#3419); mirror its isinstance guard so a non-dict existing entry is treated as
absent.
Test copies the WorkflowRegistry sibling test for StepRegistry (fails before:
AttributeError on existing.get()).
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
Ali jawwad
|
aee9df00d4 |
fix(workflows): command/prompt steps fail cleanly on a non-string integration (#3626)
_try_dispatch guarded only 'if not integration_key', then called
get_integration(integration_key). A non-string integration (a list/dict, or an
expression like integration: "{{ steps.pick.output.agents }}" that resolves to a
list) reached the registry dict lookup and raised 'TypeError: unhashable type:
list', aborting the entire workflow run. Widen the guard to also require a str,
so a non-string integration is treated as not-dispatchable and execute() falls
through to its existing FAILED StepResult (unconfigured integration=None still
returns None as before). Applied to both command and prompt steps.
Tests: a list integration now yields a FAILED result (fail before: TypeError).
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
Ali jawwad
|
9ef477167d |
fix(workflows): gate prompt uses isdecimal() so a superscript digit doesn't crash (#3624)
The interactive gate prompt guarded numeric choices with raw.isdigit(), but
str.isdigit() returns True for characters int() rejects — superscripts/subscripts
like '²'. So typing '²' passed the guard and int('²') raised an uncaught
ValueError, crashing the prompt loop. Use raw.isdecimal(), which is exactly the
decimal-digit set int() accepts (Numeric_Type=Decimal), so such input is treated
as an invalid choice and re-prompted. No behavior change for valid input.
Test: input '²' then '1' returns the first option (fails before: ValueError).
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
Quratulain-bilal
|
30e99ec083 |
fix(workflows): validate every redirect hop when fetching workflow/step catalogs (#3637)
WorkflowCatalog._fetch_single_catalog and StepCatalog._fetch_single_catalog opened the catalog URL with open_url(entry.url, timeout=30) and validated only the final resp.geturl(). open_url follows redirects, so an https:// catalog entry that 30x-redirects through a non-HTTPS host mid-chain could let a network attacker rewrite the next hop and slip a payload past the terminal-URL-only check. The payload then drives step/workflow catalog data. Pass a redirect_validator that runs the existing HTTPS/hostname check before every redirect hop, keeping the final geturl() check as a defense-in-depth backstop. This brings both workflow catalog loaders to parity with the presets (#3523) and extensions (#3524) catalog fetchers. Tests: add per-hop redirect-validation tests for both WorkflowCatalog and StepCatalog (a non-HTTPS intermediate hop is rejected); both fail before the fix ("NoneType object is not callable" — no validator passed). Update the two existing malformed-redirect tests whose open_url stub lacked the redirect_validator kwarg. |
||
|
Noor ul ain
|
48686521ff |
fix(workflows): reject a non-string 'integration'/'model' in command & prompt steps (#3597)
* fix(workflows): reject a non-string 'integration'/'model' in command & prompt steps
A non-string `integration` on a command or prompt step is passed to
`get_integration()`, which uses it as a dict key: an unhashable list/dict
raises a raw `TypeError` there — and because neither `validate()` nor
`validate_workflow` checked the type, this crashes even a *validated* run,
not just an unvalidated one. A non-string `model` likewise reaches
`build_exec_args()` and is fed into the CLI argv.
Guard both fields in `validate()` (reject a literal non-string, mirroring the
existing 'command'/'prompt'/'input'/'options' checks) and in `execute()`
(fail the step cleanly rather than take down the whole run, mirroring the
'input'/'options' guards). An explicit YAML-null (inherit the workflow
default) and a "{{ ... }}" expression both stay valid.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(workflows): route falsey non-string integration/model to the type guard
Address Copilot review: `config.get("integration") or context.default_integration`
(and the model equivalent) coerced a *falsey* non-string ([], {}, 0, False) into
the workflow default before the type guard ran. On an unvalidated execute() such a
step was silently accepted and — with a configured default — could dispatch using
the wrong integration/model instead of failing with the contract error.
Fall back to the workflow default only for genuinely-unset values (missing /
YAML-null / empty string) so every non-string reaches the guard. Add parametrized
falsey execute() cases ([], {}, 0, False) to both TestCommandStep and
TestPromptStep; with the fix stashed all 8 fail (swallowed into the default).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
Noor ul ain
|
d7699c39f2 |
fix(workflows): reject non-list input 'enum' instead of crashing (#3601)
A workflow input whose `enum` is a scalar or string (e.g. `enum: 5`,
`enum: "abc"`) previously slipped past `validate_workflow` and crashed
at run time. The `value not in enum_values` membership test in
`_coerce_input` raises a raw `TypeError` ("argument of type 'int' is
not iterable") for a scalar, and a bare string turns enum membership
into a silent substring test. The `TypeError` also escapes
`validate_workflow`'s `except ValueError`, breaking its documented
"return a list of errors, never raise" contract.
This is the same unvalidated-`execute()` crash class as the fan-in
`wait_for` (#3482) and fan-out step-template (#3537) fixes: `validate()`
should reject the value, but the value can still reach the engine via
`execute()`, which accepts unvalidated definitions.
Fix:
- `_coerce_input` requires a list `enum` (or `None`), raising a clean
ValueError for any other shape — so both `validate_workflow` and
runtime `_resolve_inputs` fail fast with a clear message.
- `validate_workflow` checks `enum` shape directly (not only via the
default-coercion path, which is reached only when a `default` exists),
and strips a malformed `enum` before coercing the default so the
wrong-typed-default error is not duplicated as an enum-shape error.
- The `integration: auto` sentinel only strips a *list* `enum`; a
non-list `enum` stays in the definition so it is rejected rather than
silently exempted by the `auto` membership skip.
Tests cover all three layers: `_coerce_input` directly, authoring-time
`validate_workflow` (with no default present), and runtime
`_resolve_inputs`, plus the `integration: auto` interaction.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
Noor ul ain
|
70c547cfab |
fix(workflows): reject a non-string 'command' in command-step (#3596)
`CommandStep.validate` only checked that a `command` field is *present*,
never its type. On an unvalidated run (the engine does not auto-validate
before `execute`) a non-string `command` — null, a list, an int — was
passed straight through `_try_dispatch` to the integration's
`build_command_invocation`, which does `command_name.startswith("speckit.")`
and crashes the whole workflow with a raw `AttributeError` once a
resolvable integration with an installed CLI is found.
Guard both paths, mirroring the sibling steps:
- `validate()` rejects a non-string `command` (like prompt-step `prompt`
#3582 and shell-step `run`).
- `execute()` fails the step cleanly with the same contract error before
dispatch (like the existing `input`/`options` guards in this file), so
an unvalidated run FAILs the step instead of crashing the run.
An expression like `{{ inputs.cmd }}` is still a string, so it stays valid.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
Noor ul ain
|
2f9e45514c |
fix(workflows): fail gate step loudly on a malformed 'options' (#3595)
`GateStep.validate` rejects a non-list (or empty) `options` and requires every option to be a string, but the engine does not auto-validate before `execute`. On an unvalidated run a scalar/dict/None `options` reached `_prompt` and crashed the whole workflow with a raw `TypeError` (`enumerate`/`len` on a non-iterable) or `KeyError` (indexing a dict); an empty list spun `_prompt`'s input loop forever; a non-string option crashed the reject check at `choice.lower()` with `AttributeError`. Guard `execute` to FAIL the step cleanly instead, before the non-TTY PAUSE short-circuit so the error surfaces in CI too rather than pausing and only crashing later on interactive resume. Mirrors the switch 'cases' and command 'input' unvalidated-execute guards. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Markus Wondrak
|
d6fa0460ed |
feat(workflows): WorkflowResolver standalone (PR 1) (#3557)
* feat(workflows): add standalone WorkflowResolver and overlay subsystem Implement PR 1 of the workflow-overlays plan: a concrete, standalone WorkflowResolver for downstream workflow extensibility without touching the Preset subsystem. - Add overlay manifest schema (Overlay, OverlayEdit, validate_overlay_yaml) - Add pure-function merge engine (find_step, apply_edit, merge_steps, validate_edits) with recursive anchor search and higher-wins semantics - Add StepListComposer and tiered layer sources (project, installed, base) - Add WorkflowResolver facade with inline HIGHER_WINS priority sorting - Add CLI verbs: workflow overlay add/set-priority/enable/disable/remove/list and workflow resolve <id> - Wire WorkflowEngine.load_workflow through WorkflowResolver - Extend workflow add to copy optional overlays/ subdirectory from local workflow directories - Add comprehensive unit, integration, and security tests Refs: discussion #3473 (https://github.com/github/spec-kit/discussions/3473) Assisted-by: Kimi (model: opencode-go/kimi-k2.7-code, autonomous) * fix(workflows): reject symlinked overlay directories in layer sources Address PR #3557 review comments r3594064534 and r3594064563: - ProjectOverlaySource.collect now rejects symlinked per-workflow overlay directories (.specify/workflows/overlays/<id>) before iterating - InstalledOverlaySource.collect now rejects symlinked installed overlay directories (.specify/workflows/<id>/overlays) before iterating - workflow_overlay_list catches ValueError from resolver and exits with code 1 instead of crashing on unhandled exceptions - Added .specify/workflows/overlays to _reject_unsafe_workflow_storage chokepoint for defense-in-depth These guards prevent symlinked overlay directories from redirecting auto-loaded overlay YAML to attacker-controlled content outside the project, which could inject executable shell steps into trusted workflows. Refs: PR #3557 review comments r3594064534, r3594064563 Assisted-by: opencode-go/qwen3.7-max (autonomous) * fix(workflows): address Copilot review findings in merge engine - Apply inserts before winning replace to prevent anchor-not-found errors when replace changes step ID (r3594064604) - Track attribution recursively for nested steps in composite inserts/replaces so workflow resolve attributes all child steps correctly (r3594064638) - Add regression tests for both fixes Refs: PR #3557 review discussion Assisted-by: GitHub Copilot (model: qwen3.7-plus, autonomous) * refactor(workflows): simplify overlay architecture to 2-tier Remove installed overlays tier to enforce clean separation of concerns: - workflow add installs workflows only (no overlay copying) - workflow overlay add installs overlays only (project-local) Changes: - Remove InstalledOverlaySource class and all references - Remove overlay-copying logic from _validate_and_install_local() - Update WorkflowResolver to 2-tier: project overlays + base workflow - Fix --priority override timing: apply before validation, not after - Remove tests for installed overlays (no longer applicable) Rationale: If upstream controls both base workflow and shipped overlays, and both get overwritten on bundle update, there's no reason to ship overlays separately. Overlays only make sense when someone other than the base author adds them. Resolves all three review findings from PR #3557: - r3594064677: workflow add no longer copies overlays from all call sites - r3594064705: --priority override now applied before validation - r3594064726: no stale installed overlays (tier removed entirely) Assisted-by: Claude (model: claude-opus-4-7, autonomous) * fix(workflows): harden overlay symlink handling Assisted-by: GitHub Copilot (model: GPT-5.4, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * docs(workflows): remove stale installed-overlay references from workflows.md The 2-tier refactor ( |
||
|
Ali jawwad
|
57cc518d63 |
fix(workflows): reject bool / .inf catalog priority in workflow & step catalog loaders (#3526)
* fix(workflows): reject bool/.inf catalog priority in workflow & step catalog loaders
The WorkflowRegistry and StepRegistry catalog-config loaders coerced priority
with int() inside except (TypeError, ValueError), missing two guards the base
CatalogStackBase loader already has:
- bool is an int subclass, so 'priority: true' was silently coerced to 1;
- int(float('inf')) raises OverflowError (not caught), so 'priority: .inf'
crashed with an uncaught traceback.
Add the explicit bool check and OverflowError to both loaders, and add
OverflowError to the two _coerce_priority helpers used by 'catalog add' (they
return 0 on an uncoercible existing priority instead of crashing).
Parametrized tests on both TestWorkflowCatalog and TestStepCatalog reject
priority true/false/.inf (fail before: bool coerced to 1 / inf OverflowError).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(workflows): cover add_catalog() OverflowError fallback for existing priority: .inf
The workflow/step catalog priority guards added OverflowError to _coerce_priority
(the 'catalog add' fallback), but the tests only exercised get_active_catalogs().
Add tests that prewrite an existing 'priority: .inf' entry and call add_catalog()
for both WorkflowCatalog and StepCatalog, asserting the command succeeds and the
new entry gets a valid priority (inf coerced to 0, +1). Fails before: int(inf)
OverflowError crashed add_catalog.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
Noor ul ain
|
3d2901eb75 |
fix(workflows): fail fan-in loudly on a non-string wait_for entry (#3579)
`FanInStep.execute` already guards a non-list `wait_for` (#3482), and the engine's load-time validation rejects non-string entries. But the engine does not auto-validate step config, so on an unvalidated run `execute` iterated the list's *elements* raw: - An unhashable entry (a list/dict from a YAML indentation slip like `wait_for: [[a, b]]`) crashed the whole run at `context.steps.get(entry, ...)` with a raw `TypeError: cannot use 'list' as a dict key`. - A hashable-but-non-string entry (`wait_for: [123]`) silently joined an empty `{}` and still reported COMPLETED — the exact "silent empty result + COMPLETED" wiring bug the whole-list guard and the engine's fan-in validation both exist to prevent. Extend the execute() guard to reject any non-string entry with the engine's "entries must be step-id strings" phrasing, mirroring the sibling non-list guard right above it. Adds regression coverage for unhashable and hashable-non-string entries. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Noor ul ain
|
c1e5cfa0aa |
fix(workflows): fail fan-out loudly on a truthy non-mapping step template (#3537)
* fix(workflows): fail fan-out loudly on a truthy non-mapping step template
A fan-out step whose `step:` is a truthy scalar or list (an authoring mistake) passed execute and reached the engine, which calls template.get("id", ...) in _run_fan_out — raising AttributeError and taking down the whole run. validate already rejects a non-mapping step, but the engine does not auto-validate, so an unvalidated run crashed.
Guard execute to FAIL the step (with a clear error and normalized empty output) instead, mirroring the existing non-list items guard and the switch non-dict cases guard. Add the matching test_execute_non_dict_step_fails_loudly covering the execute-path guard (validate was already covered).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(workflows): reject explicit fan-out `step: null` in validate()
The runtime guard in execute() rejects a truthy non-mapping step, but
`config.get("step", {})` only substitutes the `{}` default for an *absent*
key — an explicit `step: null` reaches the guard as None and FAILS the step.
validate() previously exempted None (`step is not None and ...`), so such a
workflow passed validation and then failed during execution.
Align validate() with the runtime guard: a present-but-non-mapping `step`
(including `None`) is an authoring mistake and is now rejected up front.
Extend the validate and execute regression cases to cover None.
Addresses Copilot review feedback on #3537.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
Noor ul ain
|
b139bd0393 |
fix(workflows): reject a non-string prompt in prompt-step validate() (#3582)
`PromptStep.execute` str()-coerces `config['prompt']` and dispatches the
result to the integration CLI as the model's instructions. But its `validate`
only checked that `prompt` was *present*, not that it was a string — the exact
parity gap the sibling `ShellStep` closes for `run`.
So a YAML authoring slip like `prompt: [review, this]` or `prompt:` (null)
passed validation, then `execute` sent the Python repr (`"['review', 'this']"`,
`"None"`) to the LLM verbatim — silently wrong instructions with no error and a
COMPLETED status. The engine does not auto-validate step config
(`load_workflow` explicitly defers validation), so validation is the only place
this surfaces before dispatch.
Extend `validate` to reject any non-string `prompt` with the shell-step's
phrasing ("'prompt' must be a string, got <type>"), mirroring the shell `run`
and command `input`/`options` type checks. A `{{ ... }}` expression is still a
str, so it stays valid. Adds regression coverage for non-string prompts
(null/list/int/dict) and confirms an expression prompt still validates.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
Ali jawwad
|
f75f5f836b |
fix(workflows): route 'workflow status --json' errors to stderr (#3520)
* fix(workflows): route 'workflow status --json' errors to stderr The workflow_status run_id error paths (FileNotFoundError -> 'Run not found', ValueError -> invalid run) used the stdout console and fired before the json_output branch, so 'specify workflow status <bad-id> --json' wrote a Rich-rendered error to stdout and corrupted the JSON stream a consumer would json.loads(). Route both through _error_console(json_output) so they go to stderr under --json, matching the sibling 'workflow run'/'workflow resume' commands (which use the identical RunState.load try/except) and the documented stdout-purity contract. Test asserts the not-found error appears on stderr and stdout stays empty under --json (fails before: the error was on stdout). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(workflows): cover the ValueError handler in workflow status --json purity The stderr-routing fix reroutes both the FileNotFoundError and ValueError run_id handlers, but the test only exercised FileNotFoundError — a regression of the ValueError path back to stdout would have gone uncaught. Add a ValueError case (RunState.load raising) asserting the same stderr-only / empty-stdout behavior. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Manfred Riem
|
208d38695f |
feat(extensions): add assess idea assessment pipeline extension (#3568)
* feat(extensions): add assess idea assessment pipeline extension Add a role-neutral, opt-in "Idea Assessment Pipeline" extension (id: assess) covering the discovery work that happens BEFORE spec-driven development. It provides a five-stage funnel: intake, research, define, shape, decide, each writing one artifact under .specify/assessments/<slug>/. A go verdict hands off to /speckit.specify; killing an idea is a first-class success outcome. Registration: - extensions/catalog.json: bundled core opt-in entry (before bug) - pyproject.toml: force-include maps into core_pack so it ships in the installed wheel (verified via wheel build) Also normalizes a Rich-wrapped substring assertion in test_workflows.py so the suite passes at CI's 80-column non-TTY width. Assisted-by: GitHub Copilot (model: Claude Opus 4.8, supervised) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1f25cf9d-b7eb-4b2b-b811-3e91d8db8f6a * fix(extensions): address PR review on assess extension Resolve review feedback on github/spec-kit#3568: - catalog.json: bump top-level updated_at to this revision (2026-07-17) - extension.yml + catalog.json: shorten the assess description to under the documented 200-char manifest limit (kept aligned across both) - extension.yml: make the before_specify hook prompt condition-neutral (it fires on every /speckit.specify, so it must not claim "no assessment found") - intake.md: fix slug normalization to explicitly allow lowercase letters a-z (the old rule permitted only digits and '-', contradicting the offline-mode example) - intake.md + research.md: require a sanitized source URL (strip userinfo and credential/signature query params) instead of persisting a verbatim URL that could leak secrets into project artifacts - decide.md: remove the "trivially small" exception so a go always requires a shaped concept, making verdict behavior deterministic and consistent with the guardrails and README Assisted-by: GitHub Copilot (model: Claude Opus 4.8, supervised) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1f25cf9d-b7eb-4b2b-b811-3e91d8db8f6a * refactor(extensions): remove before_specify hook from assess Assess is a separate business process from spec-driven development, so it should not inject itself into the /speckit.specify lifecycle. The hook fired on every /speckit.specify invocation (it had no condition), nagging even when an assessment already existed and the user was deliberately proceeding. Unlike git's before_specify (a mechanical prerequisite: create a feature branch) or agent-context's after_* hooks (reacting to spec output), assess is an upstream, optional, human-judgment process. The coupling that belongs here already runs forward and by choice: a `go` verdict from /speckit.assess.decide hands off to /speckit.specify. The backward hook was the redundant, intrusive direction. - extension.yml: drop the hooks block (commands-only manifest) - README.md: replace the Hooks section with a Handoff section - test: replace the hook assertion with test_declares_no_hooks to lock in the standalone-pipeline design Assisted-by: GitHub Copilot (model: Claude Opus 4.8, supervised) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1f25cf9d-b7eb-4b2b-b811-3e91d8db8f6a * fix(extensions): harden assess slug handling and clarify verdict logic Address the second review round on github/spec-kit#3568: - Slug path traversal: intake and all four downstream commands (research, define, shape, decide) now normalize an explicit or user-supplied slug to the [a-z0-9-] alphabet (dropping '.', '/', '\\') and reject an empty normalized result before constructing ASSESS_DIR. This guarantees a slug like `../..` cannot escape .specify/assessments/. - Metadata accuracy: the extension.yml and catalog.json descriptions no longer imply a "build/kill" call is handed to /speckit.specify — only a `go` hands off; a `kill` closes the assessment. - Verdict determinism (decide): a `go` now explicitly requires evidence strength `adequate`+ (never weak/unknown), resolving the conflict with the thin-evidence guardrail. - Risk polarity (decide): renamed the "Risk" criterion to "Risk posture" with positive polarity (strong = risks understood and mitigated) so it composes with the other scores that feed the verdict. - README: aligned the go-threshold guardrail with the evidence rule and documented the slug-normalization safety property. The PR description was also updated to drop the stale before_specify hook claim (the hook was removed in the previous commit). Assisted-by: GitHub Copilot (model: Claude Opus 4.8, supervised) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1f25cf9d-b7eb-4b2b-b811-3e91d8db8f6a * fix(extensions): add symlink/realpath containment and pin research host allowlist Address the third review round on github/spec-kit#3568: - Path safety (intake, research, define, shape, decide): slug normalization blocks lexical `..` but not symlinked path components. Each command now, before any mkdir/read/write, resolves the real path of .specify/assessments/<slug>/ and every artifact, refuses to follow a symlinked .specify / assessments / slug dir / artifact, and verifies the resolved path stays inside the project root. This blocks a cloned or crafted project from redirecting reads/writes outside the repository. Each stage enforces this independently since research/define/decide can run without intake. - research URL policy: replaced the open-ended "and comparable well-known hosts" no-prompt branch with intake's exact enumerated allowlist, so an agent cannot classify an attacker-controlled host as "comparable" and fetch it without confirmation. - README: guardrail now documents symlink/realpath containment. Assisted-by: GitHub Copilot (model: Claude Opus 4.8, supervised) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1f25cf9d-b7eb-4b2b-b811-3e91d8db8f6a * fix(extensions): redact secrets in captured idea and stop on explicit-slug collision Address the fourth review round on github/spec-kit#3568 (intake): - Secret leak in the captured idea: quoting the original "verbatim" contradicted the URL sanitization rule when the idea itself contained a credential-bearing URL. Capture now redacts secrets (sanitize URLs; strip tokens, passwords, keys, cookies) inside the quoted text as well as the Source field, and the section heading is "Idea (as captured)" rather than "verbatim". - Explicit-slug collision: in automated mode an existing intake.md caused a silent switch to a new slug, contradicting the no-suffix guarantee for user-provided slugs. Now: user-provided slug collision -> stop and report; only a self-generated slug (already disambiguated at resolution) is re-slugged. Assisted-by: GitHub Copilot (model: Claude Opus 4.8, supervised) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1f25cf9d-b7eb-4b2b-b811-3e91d8db8f6a * fix(extensions): reject IPv6 private ranges and DNS-rebinding in URL policy Address the remaining open comment from review 4722852090 on github/spec-kit#3568 (the other six comments in that round were already resolved by the slug-validation and host-allowlist fixes in |
||
|
Noor ul ain
|
459f483f57 |
fix(workflows): fail if/switch steps on non-list branch instead of crashing (#3515)
* fix(workflows): fail if/switch steps on non-list branch instead of crashing `IfThenStep.validate()` and `SwitchStep.validate()` already reject a non-list branch (`then`/`else`, and `case`/`default`), but the engine's `execute()` path does not auto-validate (see `WorkflowEngine.load_workflow`, whose docstring notes the definition is "not yet validated"). On an unvalidated run, the selected branch is fed straight into `next_steps`, which `_execute_steps` iterates as step mappings. A non-list branch — a single mapping or scalar authoring mistake — was iterated element-wise (a dict yields its string keys, a str its characters) and raised `AttributeError` on `.get()`, taking down the whole run; the engine invokes `step_impl.execute()` with no surrounding try/except. Guard both `execute` paths to return a FAILED StepResult naming the type error instead, mirroring the switch non-mapping `cases` and fan-out non-list `items` handling. The switch guard is factored into a shared `_non_list_branch_failure` helper covering both `case` and `default` branches. A missing `else`/`default` still defaults to an empty list (COMPLETED), unchanged; the guard fires only on an explicit non-list value. The condition/expression is still evaluated first, so its result is surfaced in the step output for downstream context. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * test(workflows): cover switch non-list branch execute paths Copilot flagged the new switch branch guards as untested: coverage stopped at a non-mapping `cases` container. Add SwitchStep.execute tests for a matched case with a non-list body and a non-list default (dict/str/int), asserting FAILED, the branch-specific error, empty next_steps, and preserved expression_value. Also add explicit `default: null` / `else: null` normalization tests so the validator-approved empty-branch contract cannot regress. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> |
||
|
Noor ul ain
|
c1722a425e |
fix(workflows): raise a clear error, not a cryptic crash, on non-string filter args (#3522)
The `map`, `join`, and `contains` expression filters assumed their
argument was a string. A non-string argument — an authoring mistake such
as `| map(5)`, `| join(5)`, or `| contains(5)` — reached an operation
that only strings support and raised a cryptic exception that escaped the
evaluator entirely:
* `map(5)` -> `attr.split(".")` -> AttributeError
* `join(5)` -> `separator.join(...)` -> AttributeError
* `contains(5)` on a string value -> `x in str` -> TypeError
The engine wraps neither expression evaluation nor `step_impl.execute()`
in a try/except, so each of these took down the whole run with a message
that names none of the real problem.
Validate the argument type up front and raise a `ValueError` naming the
filter and the offending type instead, mirroring the strict argument
handling already in `from_json`. `contains` guards only the string-value
branch: for a list value, membership of any element type is legitimate
(`5 in [1, 2, 5]`), so that branch is intentionally left unguarded.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
Roland Huss
|
6688b447b7 |
feat(workflows): expose workflow source directory to steps (#3469)
* feat(workflows): expose workflow source directory to steps (#3467) Propagate WorkflowDefinition.source_path to steps via {{ context.workflow_dir }} in template expressions and SPECKIT_WORKFLOW_DIR env var for shell steps. The original source directory is persisted in state.json so resume restores the correct value instead of the run-directory copy path. Closes #3467 Assisted-By: 🤖 Claude Code * fix: apply bot review suggestions (#2) Applied fixes from bot review comments: - Comment #3563319058: prevent stale SPECKIT_WORKFLOW_DIR leak from parent env - Comment #3563319094: use cross-platform Python one-liner instead of printenv - Comment #3563319103: add monkeypatch.delenv for deterministic env var test - Comment #3563319116: same env leak fix as #3563319058 Assisted-By: 🤖 Claude Code * fix: use YAML single-quotes and forward-slash paths for Windows CI (#2) sys.executable on Windows returns backslash paths (D:\a\...) which YAML double-quoted strings interpret as escape sequences. Switch to single-quoted YAML strings and normalize paths with replace("\\", "/"). Assisted-By: 🤖 Claude Code * fix: resolve workflow_dir to absolute path and add installed-by-ID test (#3469) Applied fixes from bot review comments: - Comment #3563382853: resolve source_path before taking parent to ensure absolute paths - Comment #3563382864: add test for installed-by-ID workflow_dir semantics Assisted-By: 🤖 Claude Code * docs: document context.workflow_dir and SPECKIT_WORKFLOW_DIR Add reference documentation for the new workflow_dir runtime value in both workflows/README.md and docs/reference/workflows.md so workflow authors can discover the feature and its semantics. Assisted-By: 🤖 Claude Code * fix: clarify installed workflow_dir is an absolute path (#3469) The documentation for context.workflow_dir described the installed-by-ID case as ".specify/workflows/<id>/" which appears relative, contradicting the "resolved absolute path" semantics. Clarified that it is the absolute path to the installation directory. Assisted-By: 🤖 Claude Code * fix: apply bot review suggestions (#3469) Applied fixes from bot review comments: - Comment #3580005128: Quote sys.executable in shell step env var test - Comment #3580005174: Quote sys.executable in no-env-var test Assisted-By: 🤖 Claude Code * fix: apply bot review suggestions (#3469) Applied fixes from bot review comments: - Comment #3587146944: Quote interpolated workflow_dir path in example Assisted-By: 🤖 Claude Code |
||
|
Ali jawwad
|
fb076a38b8 |
fix(workflows): fan-out max_concurrency .inf falls back to sequential, not crash (#3521)
_run_fan_out coerces max_concurrency with int() inside except (TypeError,
ValueError). int(float('inf')) raises OverflowError, which is not in that tuple,
so a YAML 'max_concurrency: .inf' crashed the whole run with an uncaught
OverflowError instead of the documented 'cannot be coerced -> sequential'
fallback. Add OverflowError to the except tuple (nan already coerced via
ValueError).
Extends the existing invalid-value parametrization with float('inf')/nan (fails
before on inf: OverflowError; passes after: sequential, all items in order).
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
Noor ul ain
|
77ebd5fcea |
fix(workflows): fail while/do-while steps on non-list steps instead of crashing (#3519)
`WhileStep.validate()` and `DoWhileStep.validate()` already reject a non-list `steps` body, but the engine's `execute()` path does not auto-validate (see `WorkflowEngine.load_workflow`, whose docstring notes the definition is "not yet validated"). On an unvalidated run the body is returned as `next_steps`, and the engine feeds it straight into `_execute_steps`, which iterates it as step mappings. A non-list `steps` — a single mapping or scalar authoring mistake — was iterated element-wise (a dict yields its string keys, a str its characters) and raised `AttributeError` on `.get()`, taking down the whole run; the engine invokes `step_impl.execute()` with no surrounding try/except. Guard both `execute` paths to return a FAILED StepResult naming the type error instead, mirroring the if/switch non-list-branch and fan-out non-list `items` handling. The do-while body always dispatches on the first call, so its guard is unconditional; the while body only dispatches when the condition is truthy, so its guard fires only then — a false condition leaves a non-list `steps` benign and the step completes, unchanged. The condition/expression is still evaluated first, so its result is surfaced in the step output for downstream context. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Noor ul ain
|
e742b8010a |
fix(workflows): raise catalog error, not raw ValueError, on a malformed catalog URL (#3484)
* fix(workflows): raise catalog error, not raw ValueError, on a malformed catalog URL The four catalog URL validators in `workflows/catalog.py` (`WorkflowCatalog`/`StepCatalog` `_validate_catalog_url`, and the nested fetch-path validators) accessed `urlparse(url).hostname` unguarded. A malformed authority — e.g. an unterminated IPv6 bracket `https://[::1` or a bracketed non-IP host `https://[not-an-ip]` — makes urlparse / hostname raise `ValueError`. Each validator's contract is to raise a domain error (`WorkflowValidationError` / `StepValidationError` / `WorkflowCatalogError` / `StepCatalogError`), and the command handlers catch only those. So `specify workflow catalog add "https://[::1"` surfaced an uncaught `ValueError` traceback instead of the clean `Error: Catalog URL is malformed` + exit 1 that a bad URL should give. The fetch-path validators also run on the post-redirect `resp.geturl()`, so a hostile redirect target could crash the fetch the same way. Guard each `urlparse`/`.hostname` access with `try/except ValueError -> domain error`, mirroring the fixes already applied to `specify_cli.catalogs` (#3435) and the bundler adapters (#3433). Also read `hostname` once and reuse it for the host check, matching those siblings. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(workflows): cover post-redirect malformed-URL guard (#3484 review) Copilot review asked for regression tests on the fetch-path validators that re-check resp.geturl() after redirects — the branch that turns a malformed redirect target into a domain error instead of a raw ValueError. - test_fetch_malformed_redirect_target_raises_catalog_error on both TestWorkflowCatalog and TestStepCatalog: stub open_url with a response whose geturl() is malformed (https://[::1 / https://[not-an-ip]/x) while entry.url is valid, so validation only trips on the redirect target, and assert _fetch_single_catalog raises WorkflowCatalogError / StepCatalogError with a "malformed" message (force_refresh + fresh project_dir so no cache masks it). - Test-the-test: both fail on pre-fix source (raw ValueError re-wrapped as "...Invalid IPv6 URL", no "malformed" match) and pass with the guard. Also merges latest upstream/main into the branch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Noor ul ain
|
73093954e2 |
fix(workflows): evaluate 'in'/'not in' safely on a non-iterable right operand (#3447) (#3468)
* fix(workflows): evaluate 'in'/'not in' safely on a non-iterable right operand (#3447) The `in` / `not in` operators in `_evaluate_simple_expression` only guarded `right is not None`, but `left in right` also raises `TypeError` for any other non-iterable right operand (int, bool, float). So a workflow condition like `{{ inputs.tag in inputs.count }}` where `count` is a number leaked a raw `TypeError: argument of type 'int' is not iterable` and crashed the whole run, instead of evaluating like the None case beside it. This was asymmetric with `_safe_compare`, which already swallows `TypeError` and returns False for the ordering operators. Add a `_safe_contains` helper (mirroring `_safe_compare`) that treats both a None and a non-container right operand as "nothing is contained": `in` -> False, `not in` -> True. Add a regression test covering int/bool/float/None right operands and asserting genuine containment against iterables still works. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * fix(workflows): address review feedback on #3468 #3447 was fixed independently by #3448 (merged first), which added the same _safe_membership helper this branch introduced. Per Copilot review: - Revert the redundant _safe_contains rename in expressions.py so the file matches main; the working membership guard already lives there. - Drop the duplicate test_in_operator_non_iterable_right_operand test and fold its only new coverage (not in against float/bool/None right operands, which the base test only checked for the int case) into the existing test_membership_against_non_iterable_is_false_not_error. Also merges latest upstream/main into the branch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> |
||
|
Marsel Safin
|
d7b6626218 |
feat(workflows): align workflow CLI with extension command surface (#3419)
* feat(workflows): align workflow CLI with extension command surface Adds the missing workflow commands and flags so the workflow CLI matches the extension/preset pattern: add --dev and --from, search --author, update, enable and disable. Disabled workflows are blocked from running and marked in list output. Fixes #2342 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(workflows): preserve disabled state on update, guard corrupted registry entries Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(workflows): guard list against corrupted registry entries, re-raise typer.Exit in catalog install workflow list now skips non-dict registry entries with a warning instead of crashing, matching update/enable/disable. The broad except in _install_workflow_from_catalog no longer swallows typer.Exit, so precise errors like the non-HTTPS redirect message are not duplicated. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(workflows): escape rich markup in id-mismatch errors and validate --from source early The two id-mismatch error paths interpolated repr() into Rich markup, so a stray bracket in a user typo could be parsed as markup. Route both through rich.markup.escape. `workflow add <source> --from <url>` also validated the source only after downloading. Validate it up front so a URL/path/typo fails without a network fetch. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(workflows): escape rich markup in list output and catalog install errors, isolate update failures workflow list now escapes id/name/version/description before printing, matching how extensions render user-editable fields. The catalog install helper computes safe_wf_id once and uses it for every early error path plus the final failure message. workflow update wraps _safe_workflow_id_dir and the backup read inside the try/except typer.Exit block so an unsafe id in a corrupted registry fails that one workflow and the rest continue. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(workflows): escape rich markup in --from download exception message Matches how the catalog install path escapes exception strings. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(workflows): catch OSError in per-workflow update loop and make restore best-effort Transient FS errors (perms, disk full) from backup read or write no longer abort the whole update run. The restore is wrapped in its own try/except so a failed write only warns, and the offending workflow is reported via 'Failed to update' like other per-workflow failures. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(workflows): escape rich markup in search output workflow search now escapes catalog-derived name/id/version/description/ tags before printing, matching extension search and workflow list. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: escape workflow validation errors before Rich output Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(workflows): escape remaining unescaped Rich markup paths Covers the last few review threads not yet addressed: - Escape yaml.YAMLError text in the local workflow add install path (matches the already-escaped download/catalog paths). - Escape the non---dev local directory fallback's "No workflow.yml found in <path>" message (the --dev branch already escaped it). - Escape the redirected final_url in the --from non-HTTPS redirect error (IPv6 literals like http://[::1]/... are legal and contain brackets). - Escape the "Downloaded workflow is invalid" exception message in _install_workflow_from_catalog, matching the sibling catalog-install exception handler a few lines above it. Adds regression tests for each in TestWorkflowCliAlignment, following the existing escaping-test pattern in this class. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(workflows): escape workflow name/id in install success messages Workflow names and ids come from user-controlled YAML or external catalog data; printing them unescaped lets bracket characters be interpreted as Rich tags. Escape them in the add/catalog-install success messages and the remaining catalog error paths, matching the rest of the output hardening. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(workflows): fail cleanly on unparseable catalog install URLs urlparse raises ValueError on e.g. an unbalanced IPv6 literal before the invalid-URL branch is reached; on workflow update that also bypassed the per-workflow handler and aborted the whole command. Convert the parse failure into a clean error so add fails cleanly and update skips just the affected workflow. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(workflows): reject catalog updates whose downloaded version mismatches The update path never verified the downloaded workflow carries the catalog version that triggered the update, so a stale or misconfigured URL could report success while leaving the old version installed or downgrading it. Pass the expected version into the install helper and fail the update when the downloaded definition does not match. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(workflows): validate workflow ID in run command and document new CLI flags Path-equivalent spellings like "align-wf/" previously bypassed the registry disabled check because the engine normalizes the path while the registry matches the raw string. workflow run now validates non-file sources against the workflow ID pattern before lookup. Also updates docs/reference/workflows.md with --dev/--from install options, update/enable/disable commands, and the search --author flag. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(workflows): enforce disabled state for direct paths to installed workflows Running the installed copy's YAML directly (specify workflow run .specify/workflows/align-wf/workflow.yml) skipped the registry check. File sources resolving inside .specify/workflows/<id>/ now map back to the workflow ID and refuse to run while disabled. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(workflows): reject explicit empty --from URL instead of catalog fallback 'workflow add foo --from ""' fell through 'from_url or ...' to a catalog install. Distinguish None from empty string so explicit values stay on the URL-validation path and fail closed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(workflows): registry rollback on save failure, consistent disabled check, honest update summary - WorkflowRegistry.add now rolls back its in-memory mutation when save() raises, so a later successful save cannot persist metadata for a failed update alongside the restored YAML backup. - workflow run uses the same truthiness check for 'enabled' as list and disable, so malformed values like 0 or null refuse to run. - workflow update reports 'No workflows were eligible for update' when every target was skipped instead of claiming all are up to date. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(workflows): guard non-string catalog URL and keep enable/disable rollback intact - A truthy non-string catalog url (e.g. 123) reached urlparse and raised AttributeError, escaping the clean error path; validate it is a string. - enable/disable mutated the live registry entry before add(), so add's rollback snapshot captured the already-toggled object; pass a fresh mapping instead. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(workflows): tolerate non-dict registry entries in add and clarify test docstrings A corrupted-but-parseable registry entry (e.g. a string value) crashed WorkflowRegistry.add with AttributeError on existing.get. Guard the non-dict case while still restoring the original raw value on rollback. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(workflows): atomic registry save and accurate mixed-target update summary - save() wrote the registry with open('w'), so a failed dump truncated the file and the next load reset every entry. Write to a sibling temp file and os.replace into place. - workflow update no longer claims all workflows are up to date when some targets were skipped; it reports checked-only status with a skipped count. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(workflows): exclusive temp file for registry save and cwd-independent disabled guard - save() now uses tempfile.mkstemp in the workflows dir (matching the engine's atomic writer), so a pre-created symlink at a predictable .tmp path cannot redirect the write and concurrent processes cannot collide. - The direct-path disabled guard derives the owning project from the resolved file path instead of the caller's cwd, so running an installed workflow's YAML from outside the project still refuses when disabled. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(workflows): symlink guards and shape validation in workflow registry, dev-dir file check - WorkflowRegistry now mirrors StepRegistry: _load refuses symlinked parents/registry file and normalizes a non-dict workflows field; save() rejects symlinked paths before writing. - workflow add --dev requires workflow.yml to be a regular file so a directory named workflow.yml gets the documented CLI error instead of an uncaught IsADirectoryError. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(workflows): validate download redirects before following them All three workflow download sites (add --from, catalog install, step install) passed no redirect_validator to open_url, so an HTTPS URL redirecting to cleartext HTTP issued the insecure request before the post-hoc geturl() check reported it. Shared validator now rejects non-HTTPS redirects (loopback HTTP allowed) pre-follow, matching the preset download path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test(workflows): accept redirect_validator kwarg in step-add open_url fakes Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(workflows): guard directory-shaped workflow.yml and unreadable registry - workflow add's plain local-path fallback (no --dev) checked wf_file.exists() before installing, so a directory literally named workflow.yml passed the guard and _validate_and_install_local() leaked an uncaught IsADirectoryError instead of the documented CLI error. Use is_file(), matching the --dev branch's existing guard. - WorkflowRegistry._load() treated any OSError while reading an existing registry the same as corrupted JSON, resetting to an empty in-memory registry. A later save() would then silently persist that empty state via os.replace, discarding every previously installed workflow entry. Track a _load_error flag on OSError-during-read and have save() refuse to write when it is set, so a transient I/O failure can no longer overwrite intact data on disk. - docs/reference/workflows.md: document `--from <url>` with its value placeholder, matching extensions.md and presets.md. Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(workflow): rollback registry.remove() and guard OSError at CLI boundaries Critical: WorkflowRegistry.remove() deleted the in-memory entry then called save() with no rollback, unlike add(). Combined with workflow_remove deleting the workflow directory before calling registry.remove(), a save failure permanently destroyed the workflow's files, left the on-disk registry still claiming it installed, and surfaced a raw unhandled OSError with no CLI message. - WorkflowRegistry.remove() now rolls back the in-memory entry on a save() OSError, mirroring add()'s existing rollback pattern. - workflow_remove persists the registry removal (registry.remove(), wrapped in try/except OSError -> clean escaped message) before deleting any files, so a save failure never touches the workflow directory. Important sibling paths: workflow add (local/--dev/--from and catalog), enable, and disable all called registry.add() without catching its deliberate OSError, so a save failure surfaced either an orphaned install directory (fresh local/catalog installs) or a raw/unhandled exception with no clean CLI output. - _validate_and_install_local (backs local/--dev/--from) now removes the freshly created directory on a fresh install, or restores the prior workflow.yml bytes on a reinstall-over-existing-local install, before raising a clean escaped error. - _install_workflow_from_catalog wraps the final registry.add() using the function's own established convention (rmtree the just-downloaded workflow_dir, then a clean escaped error) -- workflow_update's existing backup/restore around this function is unaffected. - workflow_enable/workflow_disable catch registry.add()'s OSError and print a clean escaped message instead of leaking the exception. Added failing-first tests proving each behavior (registry-unit rollback test, CLI-level remove/add/enable/disable save-failure tests parametrized where they share one root cause), all confirmed red before the fix and green after. Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(workflow): preserve prior catalog install on reinstall registry-save failure _install_workflow_from_catalog's final registry.add() failure handler unconditionally rmtree'd workflow_dir. That's safe for a brand-new install, but plain `workflow add <catalog-id>` also allows re-adding an already-installed workflow, downloading the new version over the existing directory first. If registry.add() then failed to save, the unconditional rmtree deleted the prior working install while the registry (after its own rollback) still reported it installed -- data loss with no way back. workflow_update already avoids this via an outer backup/restore around this function, but plain add has no such caller. Fix mirrors _validate_and_install_local's existed-before/backup-aware handling: capture whether workflow_dir existed and back up its workflow.yml bytes before any download write, then on a registry.add() OSError, restore those bytes for a reinstall or rmtree only a brand-new directory. Only one file (workflow.yml) is ever written by this path, so no further per-file bookkeeping is needed. Added a failing-first regression: install a catalog workflow, re-add it with a simulated registry save OSError, and assert a clean error, the original workflow.yml restored byte-for-byte, and the registry still reporting the original version installed. Confirmed red (prior file deleted) before the fix, green after. Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(workflow): centralize catalog-install cleanup across all failure branches _install_workflow_from_catalog is new in this PR and has seven failure branches after the mkdir/download step, each independently rmtree'ing workflow_dir: redirect-to-non-HTTPS rejection, a generic download exception, invalid downloaded YAML, a validate_workflow failure, a workflow-id/catalog-key mismatch, a version mismatch, and (fixed in the prior commit) a registry.add() OSError. Only the last one had been special-cased to spare a prior working install on reinstall; the other six still unconditionally deleted the whole directory, so re-adding an already-installed catalog workflow and hitting any of those six earlier failures destroyed the working install even though nothing about it had actually changed. Replaced all seven ad hoc rmtree call sites with a single local _cleanup_failed_install() helper that closes over the existed_before / prior_workflow_bytes captured once at the top of the function: restore the prior workflow.yml for a reinstall, or rmtree only a directory that this attempt itself created. Every failure branch now calls this one helper, so the fix is structural rather than duplicated, and every existing error message/exit code is unchanged -- only the cleanup performed before each message is different. Added a parametrized regression test covering the four early-failure trigger points reachable from plain workflow add (redirect rejection, download exception, invalid YAML, ID mismatch): each installs a catalog workflow, re-adds it while forcing that specific failure, and asserts a clean error plus the original workflow.yml surviving byte-for-byte. Confirmed red against the unfixed code (all four raised FileNotFoundError reading the deleted file) before applying the helper, green after. Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(workflow): restore registry entry verbatim on post-removal rmtree failure workflow_remove now persists registry.remove() before deleting any files (fixed previously), but if the registry write succeeds and the subsequent shutil.rmtree(workflow_dir) then fails, the registry was left claiming the workflow uninstalled while its directory remained on disk -- an orphaned install with no path back to a clean state. workflow_step_remove already handles this exact sequencing by capturing the registry entry before removal and restoring it directly into registry.data plus save() (bypassing add(), which would stamp a new updated_at) if the directory removal fails afterwards. Applied the same pattern to workflow_remove: capture registry_metadata via registry.get() before registry.remove(), and on an rmtree OSError, write it straight back into registry.data["workflows"][workflow_id] and save(), matching workflow_step_remove's restore-failure handling (a yellow warning, not a hard failure, since the primary error is already about to be reported). Existing error message and exit behavior for the rmtree failure are unchanged. Added a failing-first regression: install a workflow, monkeypatch shutil.rmtree to raise OSError, and assert a clean existing error message, the directory remaining (rmtree never actually deleted anything), and the registry entry restored byte-for-byte identical (including installed_at/updated_at) -- proving the fix bypasses add() and doesn't re-stamp timestamps. Confirmed red (registry entry stayed None) before the fix, green after. Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix 4 current Copilot review findings on workflow run/registry/install 1. workflow run ownership check followed symlinks via Path.resolve() before mapping a direct YAML path back to its installed workflow ID. A symlinked .specify/workflows/<id>/workflow.yml resolved outside the tree, missed the ownership match entirely, and let the disabled-workflow guard be silently skipped while engine.load_workflow still followed the symlink. Now maps ownership from a lexically-normalized path (os.path. normpath, no symlink following) and explicitly refuses to run if the installed <id> directory or workflow.yml leaf is itself a symlink. Direct external workflow paths that don't match .specify/workflows/... are unaffected. 2. WorkflowRegistry._load() caught a read OSError and silently fell back to an empty in-memory registry, only blocking a later save(). Callers that only query is_installed()/get()/list() before writing a file (e.g. commands/init.py's bundled speckit install, which overwrites workflow.yml once is_installed() reports false) could act on that false-empty state and destroy real data before ever reaching save(). _load() now raises OSError immediately so an unreadable registry fails closed at construction, before any query or side effect is possible. Added _open_workflow_registry() to give every CLI command a consistent clean-error boundary around registry construction. 3. _validate_and_install_local's mkdir/copy2 ran before the try/except that protected registry.add(); a copy2 failure (e.g. a truncating partial write on a reinstall) was not caught at all, so the existing backup-restore cleanup never ran and the prior working workflow.yml was corrupted with a raw traceback surfaced to the user. mkdir/copy2 now run inside the same rollback-protected section as registry.add(), sharing one _cleanup_failed_install() helper. 4. workflow update's skip message claimed any non-catalog source was installed "from a local path or URL", which is wrong for the bundled speckit workflow (source: "bundled"). Message is now source-neutral. Verified all 4 threads are current (not outdated) via GraphQL review thread query on PR #3419, HEAD |
||
|
Ali jawwad
|
52c1acf8ba |
fix(workflows): validate command step input/options are mappings (#3262)
* fix(workflows): validate command step input/options are mappings CommandStep.validate() only checked for 'command'; execute() then does input.items() and options.update(step_options). A non-mapping input:/options: (e.g. a YAML list or scalar) raised AttributeError at run time, bypassing the per-step FAILED/continue-on-error contract -- unlike the sibling steps (switch 'cases', fan-out 'step') which type-check their config fields in validate(). Add the same checks, plus a defense-in-depth coercion in execute() since the engine does not auto-validate before running a step. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs: fix code-comment typo in CommandStep.validate The explanatory comment said options.update(options) but execute() does options.update(step_options). Comment-only change; no behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(workflows): command step FAILS on malformed input/options instead of coercing execute() previously coerced a non-mapping 'input' to {} and silently ignored a non-mapping 'options', then dispatched the command anyway. For a workflow that skipped validation (the engine does not auto-validate before execute()), that let an explicitly malformed step run with empty args and report COMPLETED — masking the config error and defeating the per-step FAILED / continue_on_error semantics this change is meant to provide. Both now return a FAILED StepResult with the same contract error validate() reports (never crashing on .items()/.update()). Valid mapping configs are unaffected. Strengthened the execute() test to assert FAILED + the exact 'must be a mapping' error for input and options (fails before: the result carried the downstream dispatch error, not the shape error). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Noor ul ain
|
a965413a24 |
fix(workflows): fail fan-in step on non-list wait_for instead of crashing (#3482)
`FanInStep.validate()` and the engine's fan-in checks both reject a
non-list `wait_for`, but the engine's `execute()` path does not
auto-validate (see `WorkflowEngine.load_workflow`, whose docstring notes
the definition is "not yet validated"). On an unvalidated run, `execute`
iterated the raw value with `for step_id in wait_for`, with two bad
outcomes:
* a scalar (`wait_for: 5`, `wait_for: null`) raised `TypeError` and
took down the whole run — the engine invokes `step_impl.execute()`
with no surrounding try/except; and
* a string (`wait_for: stepA`) silently iterated its characters and
returned a join of empty results with a COMPLETED status — the exact
"silent empty result + COMPLETED" wiring bug the engine's own fan-in
validation comment warns against.
Guard `execute` to return a FAILED StepResult naming the type error
instead, mirroring the fan-out step's non-list `items` handling. A
missing `wait_for` key still defaults to an empty list (COMPLETED),
unchanged; the guard fires only on an explicit non-list value.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
Noor ul ain
|
e590cd8007 |
fix(workflows): fail switch step on non-mapping cases instead of crashing (#3481)
`SwitchStep.validate()` already rejects a non-mapping `cases`, but the engine's `execute()` path does not auto-validate (see `WorkflowEngine.load_workflow`, whose docstring notes the definition is "not yet validated"). On an unvalidated run, `execute` called `cases.items()` on the raw value, so a list or scalar `cases` authoring mistake raised `AttributeError` and took down the whole run — the engine invokes `step_impl.execute()` with no surrounding try/except. Guard `execute` to return a FAILED StepResult naming the type error instead, mirroring the fan-out step's non-list `items` handling. The expression is still evaluated first, so its value is surfaced in the step output for downstream context. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Noor ul ain
|
32952c94f4 |
feat(workflows): make shell step timeout configurable (#3327) (#3328)
* feat(workflows): make shell step timeout configurable (#3327)
The `shell` step hardcoded a 300s subprocess timeout, so any command
that legitimately runs longer than five minutes (a full build, a linter
aggregator, an integration-test target) was killed with TimeoutExpired
and failed the whole run, with no YAML knob to raise the limit.
Add an optional `timeout` field (seconds) that defaults to 300 for
backward compatibility and is threaded through to `subprocess.run`. The
timeout failure message now reports the configured value instead of a
hardcoded 300. `validate` rejects a `timeout` that is not a positive
number (bool is rejected explicitly, since it is an int subclass but a
config error rather than a duration).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Potential fix for pull request finding
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* test(workflows): cover non-finite timeout rejection in shell step
The isfinite guard added in
|
||
|
Quratulain-bilal
|
86d769b47c |
fix(workflows): don't crash on membership test against a non-iterable (#3448)
* fix(workflows): don't crash on membership test against a non-iterable
the `in` / `not in` operators in _evaluate_simple_expression only guarded
`right is not None`, so `left in right` still raised a raw TypeError when the
right operand was any other non-iterable (int, bool, float). a condition like
`{{ inputs.tag in inputs.count }}` where count is a number crashed the whole
workflow run instead of evaluating.
nothing is contained in a non-iterable, so treat membership as False (`not in`
as True) via a new _safe_membership helper that swallows TypeError. this
generalizes the old None guard and mirrors _safe_compare, which already
catches TypeError for the ordering operators.
added a regression test; confirmed it fails on the pre-fix code (raw
TypeError) and that genuine list/substring membership still works.
* address review: float membership case + broaden _safe_membership docstring
- add a float right-operand assertion so the test matches its comment (was
claiming float coverage while only exercising int/bool/None).
- reword the _safe_membership docstring to describe TypeError generally
(non-iterable right is the common case, but also e.g. an unhashable left
against a set) rather than implying only the right operand matters.
|
||
|
Ali jawwad
|
55c66125f0 |
fix(workflows): if-step validate accepts falsy non-list else (#3264)
* fix(workflows): if-step validate accepts falsy non-list else
IfThenStep.validate() guarded the 'else' branch with
'if else_branch and not isinstance(else_branch, list)'. The leading
truthiness check short-circuits for falsy non-list values (False, 0,
'', {}), so a malformed else-branch passes validation and is then
silently skipped at runtime. The sibling 'then' branch is validated
strictly; 'else' now matches by switching to an 'is not None' guard.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* test(workflows): cover explicit else:None and missing-else separately
Per Copilot feedback: the parametrized valid-else test omitted the
'else' key when the value was None, so it covered only the missing-else
case, not an explicit 'else: None'. Set 'else' explicitly (including
None) in the parametrized test and add a dedicated missing-else test, so
both accepted shapes are pinned.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
|
||
|
Ali jawwad
|
9d96c62901 |
fix(workflows): engine loop cap ignores bool max_iterations (#3270)
The while/do-while loop cap guard 'not isinstance(max_iters, int) or max_iters < 1' does not fall back to the default for a boolean max_iterations: isinstance(True, int) is True and True < 1 is False. The loop then runs range(max_iters - 1) == range(True - 1) == range(0), capping at a single iteration instead of the default 10. Exclude bools, mirroring the merged while/do-while validators (#3237) and this function's own continue_on_error bool handling. execute() does not auto-validate, so this engine guard is the only defence. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Ali jawwad
|
5c90a0547e |
fix(workflows): harden catalog.py against mis-shaped registry & non-string fields (#3375)
* fix(workflows): harden catalog.py against mis-shaped registry & non-string fields Two robustness gaps where WorkflowRegistry/WorkflowCatalog diverged from their StepRegistry/StepCatalog siblings, which already guard both: - WorkflowRegistry._load returned json.load() verbatim, so a JSON-valid but mis-shaped registry (a list root, or a dict lacking a 'workflows' mapping) made is_installed/get/list/remove/add crash with TypeError/KeyError. Mirror StepRegistry._load: validate the shape and reset to default, and widen the except tuple to OSError/UnicodeError. - WorkflowCatalog.search joined name/description/id without coercion, so a null or non-string field raised TypeError. Coerce with str(... or '') exactly as StepCatalog.search does. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(workflows): tighten mis-shaped-registry assertions Per review: WorkflowRegistry.list() always returns a dict, so assert '== {}' directly (the previous '== {} or == []' called list() twice and admitted a shape it never returns), and reference WorkflowRegistry.SCHEMA_VERSION instead of hard-coding '1.0'. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> |
||
|
Marsel Safin
|
34514fb20a |
fix(workflows): report validation errors instead of crashing on non-string workflow.yml scalars (#3421)
* fix(workflows): validate scalar types before string operations in workflow validation YAML parses unquoted scalars like version: 1.0 and id: 123 as float/int, which crashed validate_workflow and workflow add with raw tracebacks. Type-check id, name, version and step ids before regex and string operations so these surface as validation errors. Accept an unquoted schema_version: 1.0 instead of printing a self-identical rejection message. Fixes #3420 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(workflows): treat falsey non-strings as type errors, not missing fields Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(workflows): only accept schema_version 1.0 so the error message is accurate The check also accepted "1" while the error said Expected '1.0'. Unquoted YAML 1.0 still works via str(); plain 1 is now rejected with the message that matches. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> |
||
|
Marsel Safin
|
eedf73f714 |
feat(workflows): make shell step timeout configurable (#3404)
* feat(workflows): make shell step timeout configurable The shell step hardcoded a 300s subprocess timeout, killing any legitimate long-running QA command. Read an optional timeout field (seconds, positive integer, default 300) and validate it. Fixes #3327 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * style: multi-line timeout validation, assert status in default test Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Guard against unvalidated timeout in ShellStep.execute() The engine does not auto-validate step config, so a string or null timeout would reach subprocess.run() and crash the run with a TypeError. Fall back to the 300s default for malformed values, mirroring how the engine treats unvalidated continue_on_error. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> |
||
|
Noor ul ain
|
a4d94309e0 |
fix(workflows): apply chained expression filters left-to-right (#3339)
* fix(workflows): apply chained expression filters left-to-right
The pipe-filter parser in `_evaluate_simple_expression` split the
expression only at the *first* top-level `|` and treated the whole
remainder as a single filter. So a filter chain like
`{{ inputs.rows | map('name') | join(', ') }}` handed
`map('name') | join(', ')` to one filter, where the `(\w+)\((.+)\)`
regex mangled it and raised `ValueError`.
This broke the canonical use of `map`: it returns a list, and `join`
is the only filter that renders a list to a string, so the two are
meant to be chained. Chaining was impossible for every registered
filter.
Split the pipe segments at the top level (quote/bracket aware, so a
literal `|` inside a quoted argument like `join(' | ')` is preserved)
and fold each filter over the running value. The single-filter logic
is extracted verbatim into `_apply_filter`, so all existing strict
handling (`from_json` arity, unsupported-form vs unknown-filter
messages) is unchanged and now applies to every link in the chain.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Potential fix for pull request finding
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
||
|
Marsel Safin
|
882e1e90d0 |
fix(cli): exit cleanly on malformed IPv6 URLs in extension/preset/workflow add (#3369)
* fix(cli): exit cleanly on malformed IPv6 URLs in extension/preset/workflow add extension add --from, preset add --from, and workflow add <url> parsed the user-supplied URL with a bare urlparse before their HTTPS/host validation, so an unclosed IPv6 bracket escaped as a raw ValueError traceback. Wrap each parse and emit the surrounding validation's clean error style + typer.Exit(1) instead. Fixes #3368 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(cli): convert malformed redirect URLs to URLError in shared redirect handler Parse the redirect target once in _StripAuthOnRedirect.redirect_request before the validator and stdlib handler run, converting ValueError into URLError which every download path already catches. Also escape from_url in the preset install message so IPv6 brackets don't break Rich markup. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> |
||
|
Ali jawwad
|
220e6fcc4e |
fix(workflows): fan-in validate() rejects non-mapping output (#3349)
FanInStep.validate() only checked wait_for, so a non-mapping 'output'
(a list or scalar) validated clean; execute() then silently coerces it
to {}, so the author's declared aggregation keys vanish with COMPLETED
status and no diagnostic. Reject a non-mapping output at validation,
mirroring the command-step (#3262) non-mapping fix. execute()'s
defensive coercion is left in place for unvalidated callers.
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
|
||
|
Ali jawwad
|
fb796c2a39 |
fix(workflows): shell step validate() rejects non-string run (#3348)
ShellStep.validate() only checked that 'run' was present, so run: (null) or a GitHub-Actions-style list validated clean; execute() then str()-coerces the value and invokes it under shell=True, literally running 'None' or "['echo', 'hi']" as a command. Add a type check after the presence check, mirroring the command-step (#3262) and gate options validation. Expression strings ('{{ ... }}') are strings, so they stay valid. Co-authored-by: Claude Fable 5 <noreply@anthropic.com> |
||
|
Noor ul ain
|
5217206fdf |
fix(workflows): match gate reject option case-insensitively (#3335)
`validate` accepts a reject option case-insensitively
(`o.lower() in {"reject", "abort"}`), so a gate authored as
`options: [Approve, Reject]` passes validation. But `execute`
compared the echoed choice case-sensitively, so picking `Reject`
fell through to the approval path and silently ran downstream
steps instead of aborting.
Lower-case `choice` before the reject comparison so the runtime
agrees with the validation that let the option through.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
Quratulain-bilal
|
44c112c807 |
fix(workflows): compare non-numeric strings lexicographically instead of returning False (#3323)
_safe_compare coerced both operands to int/float unconditionally for <, >, <=,
>=. any non-numeric string (an iso date, a version tag, a name) failed that
coercion and the whole comparison silently returned False -- so
`{{ inputs.d < '2026-02-01' }}` was False even when the date was earlier.
only coerce when both operands look numeric; otherwise compare the original
values, so two strings order lexicographically the way python does and two
numeric strings still compare as numbers ("10" > "9"). a number vs a
non-numeric string stays incomparable and yields False.
add a regression test covering dates, plain strings, numeric strings, and the
number-vs-string case.
|
||
|
Quratulain-bilal
|
3b4e7f3cb6 |
fix(workflows): quote-aware interpolation so a literal }} in a filter arg doesn't break multi-expression templates (#3307)
* fix(workflows): quote-aware interpolation so a literal }} in a filter arg doesn't break multi-expression templates
#3208/#3228 hardened the single-expression fast path (_is_single_expression)
so a literal {{ or }} inside a string argument like `| default('}}')` stays on
the typed path. the multi-expression interpolation path was left on the old
_EXPR_PATTERN regex, whose non-greedy `(.+?)}}` body stops at the first }}
regardless of quoting. so a multi-expression template with a literal }} in any
block captured a truncated body, hit the filter parser malformed, and raised
ValueError.
e.g. `{{ inputs.name }}: {{ inputs.missing | default('}}') }}` raised instead
of interpolating.
replace _EXPR_PATTERN.sub with _interpolate_expressions, which scans each block
for a }} outside string literals - the same quote handling _is_single_expression
already uses. plain-value passthrough (a literal }} in a resolved value, not an
expression) is unchanged.
add regression tests for a literal }} in the second block and in the first
block, plus a literal {{ guard.
* fix(workflows): surface malformed templates in interpolation instead of emitting verbatim
address copilot review on #3307: when the quote-aware scan finds no block-closing
`}}` (e.g. an unbalanced quote in a filter arg swallowed the delimiter), fall back
to the first raw `}}` in the tail and evaluate it, so the parser raises ValueError
just as the old _EXPR_PATTERN.sub path did. only when there is no `}}` at all is
the tail left verbatim (a genuinely unterminated `{{`, which the regex also could
not match). keeps a typo failing loudly rather than being silently hidden.
add a regression test for an unbalanced quote in a multi-expression template.
|
||
|
Noor ul ain
|
5b682b2cb3 |
fix: interpolate multi-expression templates instead of returning None (#3208) (#3228)
* fix: interpolate multi-expression templates instead of returning None (#3208) `evaluate_expression` returned None for templates containing two or more `{{ }}` blocks with no surrounding literal text, e.g. `"{{ context.run_id }} {{ inputs.issue }}"`. The single-expression fast path used `_EXPR_PATTERN.fullmatch()`, but `fullmatch` defeats the pattern's non-greedy `(.+?)` body: for two adjacent expressions it still matches, capturing everything between the first `{{` and the last `}}` (`"context.run_id }} {{ inputs.issue"`) as the body. That garbage failed dot-path resolution and returned None directly, bypassing the `sub()` interpolation path that would have resolved each expression. Downstream this surfaced as the literal string "None" reaching commands. Guard the fast path on `stripped.count("{{") == 1` so only genuine single-expression templates take the typed return; multi-expression templates fall through to `sub()` and interpolate correctly. Add regression tests for two expressions separated by a space and for adjacent expressions with no separator. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * fix(expressions): use match-span guard so single expressions with literal {{ keep their type The previous `stripped.count("{{") == 1` guard misclassified a genuine single expression whose string argument contains a literal `{{` (e.g. `{{ inputs.text | contains('{{') }}`) as multi-expression, routing it through `sub()` interpolation and coercing the typed (bool/int/list) return value to a string -- breaking the type-preservation the docstring promises (Copilot review on #3228). Anchor a single match at the start and require it to consume the whole stripped string instead. The non-greedy body stops at the first `}}`, so a two-block template fails the span check (falls through to interpolation, fixing #3208) while a lone expression -- including one with a `{{` inside a string literal -- matches to the end and keeps its typed value. Add a regression test for the literal-brace single-expression case. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * fix(expressions): detect single expression with quote-aware scan The match-span guard using the non-greedy _EXPR_PATTERN stopped at the first `}}`, so a lone expression whose string argument contains a literal `}}` (e.g. `{{ inputs.text | contains('}}') }}`) was misclassified as multi-expression and mis-parsed by the interpolation path, raising ValueError and turning CI red (Copilot review on #3228). Replace the span check with `_is_single_expression`, which scans the `{{ ... }}` body for a block-closing `}}` outside string literals (mirrors the quote handling already in `_split_top_level_commas`). A genuine two-block template closes early and falls through to interpolation (fixing #3208); a lone expression with a literal `{{` or `}}` inside a string argument keeps its typed return value. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> |
||
|
darion-yaphet
|
810d6fcfe1 |
refactor: move workflow command handlers to workflows/_commands.py (PR-8/8) (#3159)
* refactor: move workflow command handlers to workflows/_commands.py (PR-8/8) Final PR of the __init__.py split. Moves the workflow command group out of __init__.py into the existing workflows/ package, completing the domain-dir layout established in PR-5 (integrations), PR-6 (presets) and PR-7 (extensions). - New workflows/_commands.py holds the four Typer apps (workflow / catalog / step / step-catalog), all 25 command handlers, the six workflow-only helpers (_parse_input_values, _workflow_run_payload, _emit_workflow_json, _stdout_to_stderr_when, _validate_step_id_or_exit, _resolve_steps_base_dir_or_exit), and a register(app) entry point. - workflows is already a package, so no rename is needed; intra-package imports change from `.workflows.x` to `.x`. The only root-helper dep (_require_specify_project) is reached through a call-time shim so test monkeypatching of specify_cli._require_specify_project keeps working. - __init__.py drops ~1445 lines (2066 -> 621); the workflow group is re-attached via register(app). Dead `contextlib` import removed. - tests/test_workflows.py: import the now-relocated _stdout_to_stderr_when helper from its new home (workflows._commands) instead of the package root. No behavior change. Full suite green (3847 passed), ruff clean. * Prevent workflow state writes through symlinked storage Workflow commands persist run state under .specify/workflows/runs, so the command-local project shim now rejects symlinked workflow storage before any workflow command proceeds. The standalone YAML path uses the same guard because it intentionally bypasses the normal project requirement while still creating workflow state under the current directory. Constraint: Local YAML workflow runs do not require an existing .specify project directory but still create .specify/workflows/runs state Rejected: Guard only .specify in the file-source path | .specify/workflows and runs can independently redirect writes Confidence: high Scope-risk: narrow Directive: Keep workflow storage symlink checks centralized before constructing WorkflowEngine Tested: .venv/bin/python -m pytest tests/test_workflow_run_without_project.py tests/test_workflows.py::TestWorkflowAddSymlinkGuard -v Tested: .venv/bin/python -m py_compile src/specify_cli/workflows/_commands.py tests/test_workflow_run_without_project.py tests/test_workflows.py Not-tested: Ruff lint; ruff is not installed in the repo virtualenv Assisted-by: OpenAI Codex (model: GPT-5, autonomous) * fix(workflows): pass github_hosts allowlist to GHES release asset resolver workflow add resolved GitHub release download URLs without forwarding the github_provider_hosts() allowlist, so resolve_github_release_asset_api_url never treated any host as GHES. This regressed GitHub Enterprise Server release asset resolution and diverged from presets/extensions, which already pass github_hosts. Forward github_provider_hosts() at both the direct-URL and catalog install call sites. The allowlist remains the anti-SSRF gate. * fix(workflows): reject symlinked/traversal <id> dir on workflow install Local/URL and catalog installs wrote to .specify/workflows/<id>/workflow.yml without guarding the <id> segment. A pre-planted symlink at <id> or <id>/workflow.yml let mkdir+copy/download follow it and write outside the project root; a non-directory <id> made mkdir raise unhandled. Add _safe_workflow_id_dir() to reject path traversal, symlinked or non-directory <id>, and a symlinked workflow.yml leaf before any write. Fold the catalog branch's existing traversal check into the helper. * fix(workflows): harden _safe_workflow_id_dir output and leaf checks - Reorder symlink/non-directory check before resolve() so a symlinked <id> reports as symlinked instead of misleading "Invalid workflow ID" - Reject a pre-existing <id>/workflow.yml that is not a file, avoiding an unhandled IsADirectoryError on later write/copy2 - Escape workflow_id in Rich output to prevent markup injection; escape the repr (not the raw id) so repr-added backslashes cannot re-expose brackets, matching extensions/_commands.py hardening - Add tests for workflow.yml-as-directory and markup-escaped invalid id * Avoid stale lint failures from config helper imports Move PyYAML loading into the helpers that read and write agent-context configuration, and replace the broad Any annotation with object. The runtime behavior stays the same while the module no longer exposes top-level imports that can be flagged as unused when CI analyzes a narrower code shape. * Prevent workflow commands from targeting reserved storage Workflow install and removal paths are derived from workflow IDs before any catalog download, local copy, or directory deletion. Validate that IDs are single workflow-id path segments and reject names reserved for workflow runtime storage so commands cannot target .specify/workflows/runs or .specify/workflows/steps. |
||
|
Ali jawwad
|
3571ba72d8 |
fix(workflows): reject bool max_iterations in while/do-while validation (#3237)
* fix(workflows): reject bool max_iterations in while/do-while validation while/do-while validate() checked 'not isinstance(max_iter, int) or max_iter < 1'. Since bool is a subclass of int, isinstance(True, int) is True and True < 1 is False, so 'max_iterations: true' passed validation and then ran as a single iteration (range(True) == range(1)) instead of being reported as a type error. Reject bools explicitly, matching the fail-fast-on-bool handling already used for number inputs and gate options. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test: assert empty error list for the valid do-while max_iterations case Address Copilot review: the accepted-config assertion only checked that no error mentioned 'max_iterations', which could let an unrelated validation error pass unnoticed. For a known-good config, assert the entire error list is empty (consistent with the other validate tests in this file). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Ben Buttigieg
|
c47dd2b812 |
chore: retire Windsurf integration — absorbed into Cognition Devin (#3168) (#3213)
* chore: retire windsurf integration — absorbed into Cognition Devin (#3168) windsurf.com now permanently redirects to devin.ai/desktop following acquisition. Remove subpackage, registry/catalog entries, docs, and tests; re-point sample-agent test fixtures to Kilo Code. Assisted-by: GitHub Copilot (model: Claude Opus 4.8, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * docs: remove stale Windsurf support references Assisted-by: GitHub Copilot (model: gpt-5.3-codex, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * docs: fix Kilo Code command path in upgrade guide Assisted-by: GitHub Copilot (model: gpt-5.3-codex, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * chore: align integration lists after rebase Assisted-by: GitHub Copilot (model: gpt-5.3-codex, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * docs: align kilocode example with runtime behavior Assisted-by: GitHub Copilot (model: gpt-5.3-codex, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> |
||
|
Huy Do
|
20f430686c |
feat(workflows): honor max_concurrency in fan-out via a bounded thread pool (#3224)
* feat(workflows): honor max_concurrency in fan-out via a bounded thread pool * feat(workflows): address review — sliding-window fan-out, locked output, faithful halt Address the reviewer feedback on the bounded fan-out concurrency: - Sliding submission window: keep at most `workers` items in flight and stop launching new items once the run is halting, instead of submitting all items up front (which let the pool keep starting queued work after a halt). - Faithful halt prefix: attribute a halt to the specific item whose own recorded result halted the run (replaying the sequential break condition, honoring continue_on_error/aborted), not the shared run status a later concurrent item may have flipped. The returned prefix now includes the actual halting item, matching the sequential path. An item that fails before recording a result (e.g. an unknown step type) is attributed too, since every item runs the same template. - Lock the parent fan-out output mutation: route the post-fan-out step_results[...]['output'] update through a new RunState.set_step_output() under the run lock, so it cannot race a concurrent save(). - Docstring: describe int() coercion accurately (numeric strings / floats are honored; only non-coercible or <= 1 runs sequentially). Tests: add concurrent halt-includes-halting-item, continue_on_error-does-not- truncate, and unknown-template-type-matches-sequential coverage; make the timing test use a monotonic clock with a looser threshold to avoid CI flakiness. * feat(workflows): address second review pass — concurrency hardening - append_log: serialize the log_entries append + log.jsonl write under a dedicated RunState._log_lock so concurrent fan-out workers can't interleave or corrupt log lines (kept separate from the state lock; never nested). - _run_fan_out.run_item: read the item output back through the item_ctx it executed against rather than the outer context closure — clearer and robust if StepContext ever stops sharing the steps dict by reference. - StepBase: document the thread-safety contract — STEP_REGISTRY holds one shared instance per type, so concurrent fan-out invokes execute() on the same object; implementations must be stateless/thread-safe (the built-ins already are). - test_concurrency_is_real: prove parallelism deterministically with a threading.Barrier (sequential execution can't clear it) instead of a wall-clock timing assertion. * feat(workflows): address review — stamp updated_at under lock, clarify cancel semantics - RunState.save(): move the updated_at timestamp assignment inside the run lock so the timestamp matches the snapshot the thread serializes and concurrent savers don't race on it. - _run_fan_out docstring: clarify that on a halt only not-yet-started items are cancelled; items already running finish but their outputs are ignored (Future.cancel() can't stop running work, and the pool joins on exit). * feat(workflows): serialize on_step_start callback under a lock The concurrent fan-out path invokes _execute_steps from worker threads, which calls the engine's on_step_start callback (the CLI sets it to a console.print lambda). Concurrent invocation could interleave/garble progress output. Guard the call with a WorkflowEngine._callback_lock so callbacks are serialized; the lock is uncontended for sequential runs. * feat(workflows): re-raise worker exceptions in-place to preserve traceback In _run_fan_out's concurrent path, a worker exception was stashed in first_exc and re-raised after the loop. Re-raise it from within the except block with a bare `raise` (after cancelling outstanding futures) so the original traceback is preserved, and drop the now-unneeded first_exc variable. The ThreadPoolExecutor __exit__ still joins any already-running workers before the exception escapes. * feat(workflows): lock final fan-out status, drop redundant output write, bound workers Address third review pass: - Remove the unlocked `context.steps[step_id]["output"] = …` writes in the fan-out parent update. context.steps[step_id] is the same dict object that set_step_output() updates under the run lock, so the direct (unsynchronized) mutation was redundant. - Preserve sequential halt semantics under concurrency: a later in-flight item could overwrite state.status after the halting item was identified. _run_fan_out now derives the halting item's run status (item_halt_status, replacing the bool item_halted) and restores it after the pool joins, so the final status is the first halting item's outcome. - Bound the pool: workers = min(max_concurrency, len(items)) and early-return for empty items, so a user-controlled max_concurrency can't over-allocate threads. Add coverage that an earlier PAUSED item's status wins over a later concurrent FAILED item. * feat(workflows): avoid unlocked context.steps writes when it aliases step_results On a resume run, StepContext is built with steps=state.step_results, so the two direct `context.steps[...] = ...` writes mutated the shared dict outside the run lock and could race save(). Route both through a new _record_result helper that mirrors into context.steps only when it is a distinct object (a fresh run) and otherwise relies solely on record_step_result's locked write. |
||
|
Ali jawwad
|
876dca8659 |
fix(workflows): gate validate() must not crash on non-string options (#3233)
GateStep.validate() reports non-string options as an error, but then -- when on_reject is 'abort'/'retry' -- still runs the reject-choice check 'any(o.lower() in ... for o in options)'. For a non-string option (e.g. options: [123]) o.lower() raised AttributeError, which escaped validate() and broke validate_workflow's documented 'return a list of errors, never raise' contract. Guard the check so it only runs when every option is a string (the non-string case is already reported above). Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Ali jawwad
|
9ece347a77 |
fix(workflows): make pipe-filter detection quote-aware in expressions (#3232)
_evaluate_simple_expression used 'if "|" in expr' / expr.split("|", 1) to detect a filter pipe, so a literal '|' inside a quoted operand (e.g. inputs.x == 'a|b') was mistaken for a filter separator and raised a spurious ValueError ('unknown filter') instead of comparing the string. Use the existing quote/bracket-aware _find_top_level helper (added for the operator-splitting fix) so only a top-level pipe is treated as a filter separator.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
Huy Do
|
3036fe6954 |
fix(workflows): reject a fan-in wait_for that names an unknown step at validation (#3225)
* fix(workflows): reject a fan-in wait_for that names an unknown step at validation * fix(workflows): reject fan-in wait_for self-reference and non-string entries Address review feedback on the fan-in wait_for validator: - A fan-in's own id is added to seen_ids before the wait_for check, so `wait_for: [<self>]` passed validation while producing a silent empty join at runtime. Reject self-references explicitly. - Non-string entries (e.g. YAML `wait_for: [123]`) were skipped by the isinstance(str) guard and validated even though they can never match a real step id. Flag them as wiring errors. Add coverage for both cases. |