mirror of
https://github.com/github/spec-kit.git
synced 2026-08-03 06:26:30 +08:00
400ad01f12a9fac9e250bbd59689bc77db79fbc6
109 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Ali jawwad
|
521020bc3a |
fix(workflows): fail a fan-in step whose output is not a mapping (#3887)
execute() did:
output_config = config.get("output") or {}
if not isinstance(output_config, dict):
output_config = {}
so every non-mapping `output` was silently discarded and the step still
returned COMPLETED — every declared aggregation key vanished, and
downstream `{{ steps.<id>.output.<key> }}` resolved to None and
interpolated as an empty string:
output=[] -> completed, error=None
output=False -> completed, error=None
output=0 -> completed, error=None
output='' -> completed, error=None
output=['a'] -> completed, error=None
output='oops' -> completed, error=None
output=5 -> completed, error=None
`validate` already rejects this and its comment names the flaw exactly:
"execute() silently coerces a non-mapping output to {}, so the author's
declared aggregation keys would vanish with no error." The engine does not
auto-validate before execute(), so on an unvalidated run that is what
happened — and `x or {}` masked the falsy shapes before the isinstance
check even ran.
Fail loudly with validate()'s own message, mirroring the `wait_for` guard
in the same method. An explicit `output:` (YAML null) stays valid.
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Marsel Safin
|
14e82353cb |
fix(workflows): refetch non-UTF-8 catalog caches (#3901)
Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> |
||
|
Noor ul ain
|
cf71d00dfe |
fix(workflows): reject a retry gate whose verdict enum forbids the reset value (#3912)
A gate with `on_reject: retry` consumes a bound reject verdict before
pausing by resetting the named input to `""` (documented behaviour, so a
later resume prompts again). Every `resume()` re-resolves the persisted
inputs through `_coerce_input`.
Those two rules collide when the bound input declares an `enum` that does
not list `""`. The reset writes a value the input's own enum forbids, and
the run wedges:
inputs:
spec_verdict:
type: string
enum: [approve, reject]
steps:
- id: review
type: gate
options: [approve, reject]
on_reject: retry
verdict_input: spec_verdict
$ specify workflow run wf --input spec_verdict=reject
Status: paused
$ specify workflow resume <run_id> --input note=b
Error: Input 'spec_verdict' value '' not in allowed values:
['approve', 'reject'].
The workflow validates clean and the first run looks fine, so the failure
only appears at the second resume. It is also unrecoverable in practice:
`_resolve_inputs` re-coerces the whole persisted map, so *any* resume that
supplies an input dies on the stored `""`. Only a resume with no inputs at
all still works -- and that is precisely the call that cannot deliver a new
verdict, which is the one thing the retry cycle exists to allow.
Extend the existing `verdict_input` cross-check (which already confirms the
name is declared) to also require that a retry-bound input's `enum` admits
the reset sentinel, and report it with a fix hint. To do that, thread the
input *definitions* through `_validate_steps` instead of just their names.
Rejected the alternative of popping the key instead of writing `""`: that
lets the input's `default` flow back in on the next resume, so a gate the
user just rejected would silently auto-approve.
Docs: note the `enum` requirement next to the reset behaviour it follows
from.
Adds 4 validation tests for the new guard plus a characterization test that
drives the engine directly to pin the wedge it prevents.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Assisted-by: Claude Opus 5 (1M context)
|
||
|
Manfred Riem
|
a3e183d069 |
feat: support tar archives for installs (#3874)
* feat: support tar archives for installs Add secure .tar.gz and .tgz parity with ZIP installation for extensions, presets, and workflows, including full workflow package preservation. Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bd07c6b3-f1f9-484c-869a-94d8fef970dd * chore: clean rebased archive imports Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bd07c6b3-f1f9-484c-869a-94d8fef970dd * fix: preserve hardened archive install behavior Keep malformed ZIP diagnostics, filesystem-independent manifest selection, and reserved workflow overlays consistent after adding generic archive support. Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bd07c6b3-f1f9-484c-869a-94d8fef970dd * fix: extract staged workflow archives by descriptor Avoid reopening a held staging path on Windows while retaining authoritative-inode archive validation. Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bd07c6b3-f1f9-484c-869a-94d8fef970dd * fix: extract catalog archives from verified bytes Use the already bounded and SHA-verified response bytes directly so Windows file-sharing semantics cannot affect archive detection. Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bd07c6b3-f1f9-484c-869a-94d8fef970dd * fix: address archive install review feedback Preserve forced preset reinstalls, sniff suffixless workflow archives without weakening YAML limits, and restore prior workflow packages before failed-install cleanup. Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bd07c6b3-f1f9-484c-869a-94d8fef970dd --------- Copilot-Session: bd07c6b3-f1f9-484c-869a-94d8fef970dd |
||
|
Marsel Safin
|
515d2810fb |
fix: reject non-object workflow caches (#3860)
* fix: reject non-object workflow caches Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test: cover non-object stale workflow cache Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> |
||
|
Marsel Safin
|
0f3f2aa45e |
fix: escape workflow step metadata (#3863)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> |
||
|
Markus Wondrak
|
675143591d |
feat: bind gate verdict to workflow input via verdict_input (#3725)
* feat: bind gate verdict to workflow input via verdict_input Add an optional `verdict_input` field to gate steps that lets an external system supply a verdict through a declared workflow input instead of an interactive TTY prompt. When the referenced input carries a non-empty string value that matches one of the gate's `options` (case-insensitive), the gate auto-decides, records the matched spelling in `output.choice`, and applies the existing `on_reject` / abort / skip / retry semantics. If the value is present but does not match an option, or is a non-string, the gate fails immediately with a clear error message. When the input is absent, null, or empty, the gate falls back to today's TTY-prompt-or-pause behaviour unchanged. The engine now persists `result.error` alongside each step's status and output so that failed-step error messages survive across runs. The CLI (`workflow run` and `workflow resume`) surfaces these persisted errors after a failed or aborted run. `validate_workflow` cross-references `verdict_input` against the workflow's declared inputs block and reports an error for undeclared names, consistent with the existing `wait_for` id cross-check. Closes discussion: https://github.com/github/spec-kit/discussions/3717 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, autonomous) * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Fix workflow JSON error payloads Include persisted step errors in _workflow_run_payload so workflow run/resume/status --json all surface failure reasons consistently. Add JSON-path tests for failed and successful runs. Assisted-by: GitHub Copilot (model: gpt-5.3-codex, autonomous) Co-authored-by: Copilot <223556219+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): reject verdict inputs in fan-out Fan-out items share workflow inputs and cannot safely consume a bound gate verdict. Reject verdict_input bindings during validation and at runtime while preserving unbound gates. Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: update workflow command handling Assisted-by: GitHub Copilot (model: gpt-5.3-codex, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Markus <markus@example.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> |
||
|
Noor ul ain
|
6712665bba |
fix(workflows): guard the shell step's timeout check against OverflowError (#3865)
PR #3847 hardened the prompt step's `timeout` guard against a huge-int value, but its twin in the shell step — the step the prompt one was mirrored from — still has the hole. `math.isfinite(10**400)` raises `OverflowError: int too large to convert to float`. A 400-digit YAML scalar is an `int` and is not a `bool`, so it clears every clause before `isfinite()` and raises there, escaping `_timeout_error()` as exactly the uncaught crash that helper exists to prevent: steps: - id: qa type: shell run: echo hi timeout: 1000...0 # 400 digits $ specify workflow run wf.yml Traceback (most recent call last): ... File "src/specify_cli/workflows/engine.py", line 361, in _validate_steps step_errors = step_impl.validate(step_config) File "src/specify_cli/workflows/steps/shell/__init__.py", line 127 or not math.isfinite(timeout) OverflowError: int too large to convert to float `workflow_run` calls `engine.validate()` before executing any step, so the OverflowError propagates out of `validate_workflow` and kills the command with a bare traceback that names neither the step nor the field, instead of the "Workflow validation failed" report. `execute()` shares the same helper, so an unvalidated run raises there too — and the engine re-raises anything a step throws, aborting the whole workflow after earlier steps have already run their side effects. The value is genuinely invalid rather than merely unrepresentable in the check: `subprocess.run(timeout=10**400)` raises the same OverflowError. Unlike the prompt step, the shell step checks `isfinite()` *before* `timeout <= 0`, so a negative huge int (`-(10**400)`) crashes as well rather than being caught by the sign check. Wrapped the condition in `try/except OverflowError` and treated the value as invalid, mirroring the prompt step's guard so both steps reject the same values with the same message. Now: Workflow validation failed: - Shell step 'qa': 'timeout' must be a positive number of seconds, got 1000...0. Valid int/float timeouts, non-finite floats, bools, strings and non-positive values are unaffected — the existing clauses are unchanged. Regression tests in `TestShellStep`: `validate()` rejects both signs of the huge int, `validate_workflow()` reports it end to end (pinning the path the CLI actually takes, not just the helper), and `execute()` fails only that step with `subprocess.run` patched to assert it is never reached. Test-the-test: reverting the source change fails all three with `OverflowError` and leaves the rest of `TestShellStep` passing. Assisted-by: Claude Code (model: claude-opus-5, under direct human supervision) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Noor ul ain
|
afbb2c7b65 |
fix(workflows): validate prompt step 'timeout' like the shell step (#3847)
* fix(workflows): validate prompt step 'timeout' like the shell step PR #3768 added a `timeout` to the prompt step and passed it straight into `subprocess.run(timeout=...)`. Neither `validate()` nor `execute()` checks it, so a bad value from a user-authored `workflow.yml` escapes as a raw exception: steps: - id: first type: shell run: echo side-effect - id: ask type: prompt prompt: do it timeout: abc $ specify workflow run wf.yml > [first] shell ... Workflow failed: unsupported operand type(s) for +: 'float' and 'str' The engine re-raises anything a step throws, so this takes down the whole run — after `first` has already run its side effect — with a message that names neither the step nor the field. `timeout: .nan` raises `ValueError: cannot convert float NaN to integer` the same way, and a non-positive `timeout` (`0`, `-5`) makes `subprocess.run` report an immediate TimeoutExpired for a command that never got the time to run. `timeout: true` silently becomes a 1-second limit, since bool is an int subclass. The sibling shell step already rejects exactly these values via a `_timeout_error()` helper shared by `execute()` and `validate()`, so the same workflow failed validation cleanly as a shell step and crashed as a prompt one. Mirrored that helper onto PromptStep: `validate()` reports the contract error, and `execute()` re-checks it so an unvalidated run fails just that step instead of aborting. Now: Workflow validation failed: - Prompt step 'ask': 'timeout' must be a positive number of seconds, got 'abc'. caught before the first step runs. Positive int/float timeouts and an absent `timeout` are unaffected. Regression tests in `TestPromptStep` mirror the shell step's: validate rejects "30"/True/inf/nan/0/-5/list/None, validate accepts 300/5/0.5 and an absent field, and execute fails cleanly with `subprocess.run` patched to assert it is never reached. With the source fix reverted, all 9 rejection tests fail. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Assisted-by: Claude Code (model: claude-opus-5, under direct human supervision) * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * test(workflows): cover the huge-int timeout OverflowError guard The autofix commit wrapped the prompt step's `_timeout_error()` check in `try/except OverflowError` but added no test, so nothing pins the behaviour it introduced. `math.isfinite(10**400)` raises `OverflowError: int too large to convert to float` — the value is an `int`, is `> 0`, and is not a `bool`, so it clears every other clause of the guard and reaches `isfinite()`. Without the `except`, validating ```yaml - id: ask type: prompt prompt: do it timeout: 1000...0 # 400 digits ``` raises that `OverflowError` out of `validate()`/`execute()` — exactly the uncaught-crash failure mode this guard was added to prevent. The same value raises `OverflowError` from `subprocess.run(timeout=...)`. Add `10**400` to both parametrized rejection lists (`validate()` and the `execute()` fails-cleanly loop). Test-the-test: reverting the `try/except` fails both new cases with `OverflowError` and leaves the rest passing. Assisted-by: Claude Opus 5 (model: claude-opus-5, autonomous) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> |
||
|
Ali jawwad
|
6033c6957b |
test(workflows): name the condition-rejection tests for the real boundary (#3808)
`test_validate_rejects_non_string_condition` contradicts its sibling `test_validate_accepts_string_or_bool_condition` in the same class: a bool *is* a non-string, so the two names disagree about the contract the validator actually implements. Rename to `test_validate_rejects_non_string_non_bool_condition` in all three step classes, matching the validator's own message: "'condition' must be a string or boolean, got <type>". Test names only — no behaviour change, and the parametrized values are untouched. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Ali jawwad
|
d99170fb5e |
fix(workflows): dispatch prompt steps via the resolved executable (#3793)
PromptStep._try_dispatch runs `subprocess.run(exec_args, ...)` with an
UNRESOLVED argv[0] -- a bare name like `claude`. On Windows subprocess.run calls
CreateProcess, which does not consult PATHEXT, so an agent CLI installed as a
`.cmd`/`.bat` shim (the usual npm layout) raises FileNotFoundError [WinError 2].
That OSError is swallowed by the method's `except OSError: return None`, and
execute() then reports "CLI not found or not installed" -- even though the
step's own preflight `shutil.which(...)` two lines earlier just found it.
The sibling path does not have this bug: IntegrationBase.dispatch_command (used
by the `command` step) resolves argv[0] through shutil.which first, added in
|
||
|
Ali jawwad
|
751eae727e |
fix(workflows): escape the step-progress line so step ids render (and / stops failing the run) (#3783)
`workflow run` and `workflow resume` both print the step-progress line as
`f" ▸ [{sid}] {label} …"`. Rich parses the bracketed step id as a style tag,
which produces three failures on main:
1. The id is SILENTLY SWALLOWED on every run -- the only identifying content on
the line. `id: greet` prints " ▸ shell …"; "[greet]" is absent.
2. An id that forms a closing tag FAILS THE WHOLE RUN. `validate_workflow`
places no charset restriction on step ids, so `id: "/"` is a valid workflow;
the callback then raises MarkupError, which propagates into execute()'s
handler -> run persisted as `failed` with empty `step_results`, the step
never executed, exit 1 with a Rich internals error.
3. An id that is a real style (`bold`, `red`) is applied as FORMATTING to the
rest of the line.
The unescaped `label` (from `step_config["command"]`) compounds it.
Escape the literal bracket with `\[` and escape both interpolated values, at
both sites. This mirrors the `\[<type>]` step-graph precedent already in this
file (workflow_info). Escaping only the values is NOT sufficient -- the
f-string's own brackets are what Rich consumes.
Verified through the real CLI: ids `greet`/`bold`/`a]b` now render verbatim, and
`id: "/"` goes from a failed run to `Status: completed`.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Ali jawwad
|
52a6514e38 |
fix(workflows): reject falsy non-mapping workflow-catalogs.yml top level (#3707)
* fix(workflows): reject falsy non-mapping workflow-catalogs.yml top level
WorkflowCatalog._load_catalog_config parsed the config with
`yaml.safe_load(...) or {}`, then checked `isinstance(data, dict)`. The
`or {}` coerces a FALSY non-mapping top level (`[]`, `false`, `0`, `''`) to
`{}` *before* the guard runs, so those are silently swallowed as "empty
config" and fall back to the built-in defaults -- while a TRUTHY non-mapping
(`5`, a bare list) correctly raises. Same silent-swallow inconsistency the
bundler catalog reader fixed for its own config.
Drop the `or {}` and branch on `None` (empty document / explicit `null`)
explicitly: `None` stays a valid no-op, every non-mapping (falsy or truthy)
now raises the same actionable error. Correct configs are unaffected.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* docs(workflows): describe the catalog-config fallthrough accurately
The comment said a None return means "no project catalogs, fall back to the
built-in defaults". Both halves were imprecise: _load_catalog_config serves the
project AND user configs, and get_active_catalogs falls through env -> project
-> user -> built-in, so a None from the project layer moves on to the USER
config; the built-in defaults apply only once every layer returned None.
Reword the loader comment and the mirror test docstring. Comments only -- no
behaviour change.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(workflows): close the same falsy-mask gap in 'catalogs' and StepCatalog
Self-review follow-up: the top-level fix left the identical asymmetry live five
lines below, and again in this file's twin loader.
1. WorkflowCatalog._load_catalog_config: the ``catalogs`` shape check sat behind
an emptiness check, so a FALSY non-list (``catalogs: {}``/``''``/``0``/
``false``) was silently swallowed as "no catalogs" while ``catalogs: 5``
raised. Verified before this commit: ``catalogs: {}`` -> None (no error).
Shape now checked first; absent/explicit-null and empty-list stay no-ops
(matching the bundler's reader).
2. StepCatalog._load_catalog_config -- the step-catalog twin, read the same way
-- still had ``yaml.safe_load(...) or {}``, so falsy non-mappings bypassed its
isinstance guard (``[]`` -> None while ``5`` raised). Same two guards applied,
keeping the two loaders in lockstep.
Eight new parametrized cases, all failing before this commit.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(workflows): move StepCatalog guards into TestStepCatalog and add the nested case
Address three review points:
1. The StepCatalog regression tests sat inside TestWorkflowCatalog, so a
targeted `pytest ...::TestStepCatalog` run skipped them entirely. Moved into
that class, where the duplicated twin loader belongs.
2. StepCatalog had no nested-value coverage (only top-level). Added the
parametrized falsy ``catalogs:`` case, plus the absent/null/empty no-op
cases. Verified against upstream/main's catalog.py: 8 fail there, pass here.
3. Dropped the inaccurate parity parenthetical. src/specify_cli/catalogs.py
RAISES for missing/empty ``catalogs`` and coerces a null document to {}, so
it is not the behavior this loader matches -- the comment now just states
what changed (only the misreported shapes) without claiming parity.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
Ali jawwad
|
a9c9905400 |
fix(workflows): reject non-string/non-boolean 'condition' in if/while/do-while steps (#3706)
* fix(workflows): reject non-string 'condition' in if/while/do-while steps
`if_then`, `while_loop`, and `do_while` validate() confirm `condition` is
present but never that it is a string. execute() feeds it to
`evaluate_condition()`, which returns a non-string as-is and takes `bool()`
of it -- so `condition: [1, 2]` (a list authoring mistake) silently resolves
to `True`, branching wrongly / spinning the loop to `max_iterations`, with no
error reported.
Reject a present-but-non-string `condition` at validation, mirroring the
existing prompt/shell/command 'must be a string' guards. `"true"`/`"false"`
and expressions like `"{{ ... }}"` are strings, so they stay valid.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* docs(workflows): describe the evaluate_expression/evaluate_condition split accurately
Address review feedback: the guard comments attributed the non-string
pass-through to evaluate_condition(), which always returns a bool. It is
evaluate_expression() (called by evaluate_condition) that returns a non-string
unchanged; evaluate_condition then applies bool() to that value.
Reword all four sites (if/while/do-while guards + the mirror test comment) to
name the two stages correctly. Comments only -- no behaviour change.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(workflows): keep a literal bool 'condition' valid
Self-review catch: the guard rejected EVERY non-string, which broke an input
that previously worked. An unquoted ``condition: false`` is idiomatic YAML and
resolves exactly today -- evaluate_expression passes the bool through and
evaluate_condition's bool() is a no-op (verified: evaluate_condition(False) is
False, (True) is True). The if/while steps even default ``condition`` to the
bool ``False`` themselves, so bool is the field's natural type, not an
authoring mistake.
Accept (str, bool) and reject only the genuinely silent-coercion types
(list/dict/int/float, e.g. condition: [1, 2] is always True). Message updated
to "must be a string or boolean"; the bad-value tests drop True and gain 1.5,
and each step gains a positive bool case.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
Noor ul ain
|
8bfe6e14d1 |
fix(workflows,extensions): tolerate non-list catalog tags in search/info display (#3770)
`workflow search`, `workflow info`, `extension search` and `extension info`
crashed with `TypeError: 'int' object is not iterable` when a catalog entry
carried a scalar `tags:` value (e.g. `tags: 5`). Catalog payloads are
user-editable YAML/JSON, so this shape reaches the display unvalidated.
Both backends already guard their tag *filter* with
`isinstance(raw_tags, list)` — `WorkflowCatalog.search` and
`ExtensionCatalog.search` skip a non-list `tags` cleanly. Only the display
paths were unguarded: they tested truthiness (`if info.get("tags"):`) and
then iterated. A scalar is truthy but not iterable, so `--tag` filtering
survived while plain `search`/`info` rendering blew up.
Note this is distinct from the non-string *element* handling added in
#3746/#3747: coercing elements with `str(t) for t in ...` does not help when
`tags` is not a sequence at all. The fix is the guard the sibling
integration commands already use — `integrations/_query_commands.py:332,402`
gate on `isinstance(tags, list) and tags`. This aligns workflows and
extensions with that reference pattern, leaving all four tag-join display
sites in these modules consistent.
Regression tests drive the full CLI via CliRunner and cover search + info in
one case per module; both fail before the fix with the exact TypeError.
Assisted-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Pascal THUET
|
118062eac4 |
harden: secure extension and preset archive downloads (#3141)
* harden: secure extension and preset archive downloads Adopt the shared download-security primitives from #3140 across extension and preset catalog, package, direct-URL, and ZIP-install flows: - bound catalog, package, and inline manifest reads; - verify catalog SHA-256 values when present; - replace path-only extraction with bounded traversal/symlink-safe extraction; - validate malformed hosts and ports before opening download URLs; - handle normalized trailing-backslash directory entries consistently. Redirect enforcement and checksum verification remain owned by the shared helpers already on main; this commit wires them into extension and preset behavior. Assisted-by: OpenAI Codex (model: GPT-5, autonomous) * harden: close archive and catalog download edge cases Preflight ZIP central directories before ZipFile allocates them, bound both declared and actual payload sizes, and reject ambiguous or non-portable archive paths before extraction. Keep extension update manifest selection consistent with extraction, reject unsafe catalog-derived output filenames and malformed URL types, and escape untrusted values in download errors. Add regression coverage for parser differentials, collisions, platform-specific filenames, bounded call sites, and failure ordering. Assisted-by: OpenAI Codex (model: GPT-5, autonomous) * harden: address download security review feedback Assisted-by: OpenAI Codex (model: GPT-5, autonomous) * harden: close ZIP preflight review gaps Assisted-by: OpenAI Codex (model: GPT-5, autonomous) * fix: harden extension update preflight and rollback Assisted-by: OpenAI Codex (model: GPT-5, autonomous) * fix: harden extension update rollback Assisted-by: OpenAI Codex (model: GPT-5, autonomous) |
||
|
Noor ul ain
|
962f9f0765 |
fix(workflows): escape remaining untrusted fields in workflow info (#3731)
* fix(workflows): escape remaining untrusted fields in `workflow info` Follow-up to #3690, which escaped only the step-graph brackets. Every other metadata field `workflow info` prints is untrusted content (workflow.yml or catalog JSON), and console.print has Rich markup enabled, so an unescaped `[...]` in any of them is parsed as a style tag and silently swallowed: - definition path: name, version, author, description, integration, and each input's name/type - catalog path: name, version, description, tags, and the "not found" workflow id A description of `Does [stuff] nicely` rendered as `Does nicely`; an integration of `claude [code]` rendered as `claude `. Route every field through _escape_markup, matching the sibling `workflow list` / catalog `search` commands, so bracketed text renders literally. Add two regression tests covering the definition and catalog paths; both fail on the pre-fix source (fields with brackets come back truncated). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test: cover version + not-found-id escapes in workflow info Addresses Copilot review feedback on the workflow-info markup-escape tests: - The definition-path and catalog-path regression tests left `version` bracket-free and never asserted it, so the version escapes could be removed without failing. Use bracketed version values and assert they survive verbatim. - The newly escaped not-found identifier is a separate output path that no test reached. Add a case where local load raises FileNotFoundError and catalog lookup returns None, invoke `workflow info` with a bracketed ID, and assert the literal ID is preserved in the error. Verified each new assertion fails when its source escape is removed (test-the-test). 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
|
99dc915ae3 |
fix: escape Rich markup in catalog list output (#3738)
The `catalog list` subcommands for workflows, workflow steps, presets, and integrations printed user-editable catalog fields (name/url/ description from the `*-catalogs.yml` files) through `console.print` with Rich markup enabled. Any bracketed content such as a description `Does [stuff] nicely` was parsed as a style tag and silently swallowed, and a malformed tag could raise while rendering. Route each untrusted field through the module's already-imported `escape` helper, matching the pattern already used by `extension catalog list`. Adds regression tests for all four commands that inject bracketed name/url/description and assert the brackets survive verbatim in the output. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Ali jawwad
|
103ad73775 |
fix(workflows): guard non-mapping 'workflow:' block in WorkflowDefinition (#3694)
* fix(workflows): guard non-mapping 'workflow:' block in WorkflowDefinition
A present-but-non-mapping top-level `workflow:` block (bare `workflow:` ->
YAML null, or `workflow: <str>` / `workflow: [..]`) crashed
WorkflowDefinition.__init__ with AttributeError: the `{}` default of
`data.get("workflow", {})` only applies when the key is ABSENT, so a non-dict
value reached `workflow.get("id", ...)`. This fires inside from_yaml/
from_string — before validate_workflow can report the malformed shape — and
in the CLI escapes as a raw traceback (load_workflow is wrapped to catch only
FileNotFoundError/ValueError).
Normalize the local `workflow` to {} when it is not a mapping (self.data keeps
the raw value so validate_workflow still reports it), mirroring the adjacent
default_options guard.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(workflows): assert self.data preserves the raw non-mapping workflow value
Address review: the previous assertion only proved the key stayed present; it
would pass even if construction replaced the malformed value with {}. Assert
definition.data["workflow"] equals the original parsed value and is still a
non-mapping, proving the guard normalizes only the local variable.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
Ali jawwad
|
ea6843c1fe |
fix(workflows): guard non-mapping 'inputs:' block in engine._resolve_inputs (#3696)
execute()/resume() run UNVALIDATED definitions (load_workflow does not
validate). WorkflowDefinition stores `inputs` raw, so a non-mapping
`inputs:` block (bare `inputs:` -> None, or `inputs: []`) crashed
_resolve_inputs at `for name, input_def in definition.inputs.items()` with
AttributeError, aborting the whole run.
Return {} when inputs is not a mapping, mirroring validate_workflow's own
`isinstance(definition.inputs, dict)` check. Protects both call sites
(execute and resume); normal dict resolution 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
|
0b6bf865c1 |
fix(workflows): escape step-graph brackets in workflow info so the type shows (#3690)
`workflow info` rendered each step as `→ <id> [<type>]`, but console.print has Rich markup enabled, so `[<type>]` was parsed as a style tag named after the step type (command/gate/prompt/…) and silently swallowed — every step printed as `→ <id> ` with the type gone. Escape the literal bracket with `\[` (and escape id/type via _escape_markup, as the sibling workflow_list does), so Rich renders `[<type>]` literally. Mirrors the in-file `\[disabled]` precedent. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Ali jawwad
|
043c4ec572 |
fix(workflows): filter parser rejects trailing tokens (fullmatch, not match) (#3689)
_apply_filter parsed a name(arg) filter with an UNANCHORED regex (re.match(r"(\w+)\((.+)\)")), so any tokens after the closing paren were silently discarded. Because _evaluate_simple_expression splits the top-level pipe before comparison/boolean operators, `count | default(0) > 5` was split into value `count` and filter segment `default(0) > 5`; the segment matched as `default(0)` and `> 5` vanished — the filter's value was returned as the whole expression, giving a silently wrong result. Use re.fullmatch so a mis-wired segment falls through to the existing "unsupported form" ValueError, mirroring the from_json branch's strict trailing-token handling. The greedy `.+` still matches legitimate forms (literal `)` / `|` inside quoted args), so registered/chained/quoted-pipe filters are unaffected. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Ali jawwad
|
37041087dd |
docs(workflows): gate step docstring lists the 'retry' on_reject behaviour (#3656)
The GateStep docstring said on_reject "controls abort / skip behaviour", omitting the third value. validate() accepts 'abort', 'skip', or 'retry', and execute() has a dedicated retry branch (returns PAUSED so the next resume re-runs the gate) distinct from abort (FAILED) and skip (COMPLETED). Add 'retry' to the docstring so it matches the same file's validate() and execute() authority. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Pascal THUET
|
3a7a8758f7 |
fix: harden bounded reads and redirect validation (#3671)
Assisted-by: OpenAI Codex (model: GPT-5, autonomous) |
||
|
Ali jawwad
|
3356161d88 |
docs(workflows): init step docstring lists the 'py' script type (#3655)
The InitStep `script` field docstring claimed only 'sh' or 'ps', but the
step's own VALID_SCRIPT_TYPES = tuple(SCRIPT_TYPE_CHOICES.keys()) is
('sh', 'ps', 'py') and validate() accepts all three (its error message is
built from VALID_SCRIPT_TYPES). Update the docstring to list 'py' too, so
it no longer contradicts the same class's validate() authority.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
Pascal THUET
|
5601830ba3 |
harden: bound HTTP reads and enforce strict redirects (#3140)
* harden: bound HTTP reads and enforce strict redirects Add a shared _download_security module (read_response_limited, is_https_or_localhost_http, size constants) and route the GitHub release and Azure DevOps token network reads through bounded reads so an oversized response can't exhaust memory. Add a strict_redirects mode to authentication.open_url: the redirect handler now rejects any redirect whose target isn't HTTPS (or HTTP to localhost), composing with the existing per-hop redirect_validator and auth-stripping. The Azure DevOps token POST is routed through that handler so a 307/308 cannot forward the client_secret body to a non-HTTPS host. Assisted-by: Codex (model: GPT-5, autonomous) * test: align HTTP fakes with bounded reads Assisted-by: Codex (model: GPT-5, autonomous) * fix: tolerate invalid token response encoding Assisted-by: Codex (model: GPT-5, autonomous) * test: align GHES fakes with bounded reads Assisted-by: Codex (model: GPT-5, autonomous) * test: reuse shared upgrade HTTP response helper Assisted-by: Codex (model: GPT-5, autonomous) * fix: include rejected redirect target in error Assisted-by: Codex (model: GPT-5, autonomous) * fix: enforce strict redirects by default Assisted-by: Codex (model: GPT-5, autonomous) * fix: close redirect credential and SSRF gaps Assisted-by: Codex (model: GPT-5, autonomous) |
||
|
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> |