mirror of
https://github.com/github/spec-kit.git
synced 2026-08-03 06:26:30 +08:00
9a30db484b0876cb7e5a391cf735d59bd968e985
426 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Ali jawwad
|
41c5dfc3a1 |
fix(auth): Azure DevOps az-CLI token acquisition returns None on undecodable output (#3527)
_acquire_via_az_cli runs 'az account get-access-token' with text=True, so subprocess.run decodes stdout with the locale encoding and raises UnicodeDecodeError (a ValueError sibling, NOT a JSONDecodeError) when the output can't be decoded. That escaped the except (OSError, TimeoutExpired, JSONDecodeError, KeyError) tuple and crashed a helper whose contract is to return str | None. Add UnicodeDecodeError to the tuple. Test patches subprocess.run to raise UnicodeDecodeError and asserts resolve_token returns None (fails before: the error propagated). Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Andrew Chen
|
0d780162f9 |
fix(bundle): surface a clean BundlerError on a malformed bundle download URL (#3586)
`_download_manifest` and its `_require_https` helper parsed the catalog entry's `download_url` with an unguarded `urlparse(url)`. A malformed authority — e.g. an unclosed IPv6 bracket like `https://[::1` — makes `urlparse` (or `.hostname` on older Pythons) raise a raw `ValueError`. The three `bundle` CLI commands (`info`, `install`, `update`) only catch `BundlerError`, so that `ValueError` escaped as an uncaught traceback. Wrap both parse sites in the same `try/except ValueError -> BundlerError` guard already used by the sibling `_validate_remote_url` (and established by the merged catalog-URL fix #3576), so a bad `download_url` reports a clean, actionable error in every mode. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Andrew Chen
|
0a60e53e06 |
fix(presets): raise PresetValidationError, not raw ValueError, on malformed catalog URL (#3576)
`PresetCatalog._validate_catalog_url` called `urlparse(url).hostname` without guarding it. For a malformed authority such as an unterminated IPv6 bracket (`https://[::1`), `urlparse(...).hostname` raises `ValueError: Invalid IPv6 URL`, which escapes the method. Its docstring promises `PresetValidationError`, and its callers (`preset catalog add`, `preset catalog list` reading the `SPECKIT_PRESET_CATALOG_URL` env var / `.specify/preset-catalogs.yml`) only catch `PresetValidationError` -- so a malformed URL crashes the CLI with a traceback instead of a clean error message. The shared `CatalogStackBase` (#3435), `workflows` (#3484), `bundler` (#3433) and `IntegrationCatalog` copies already wrap this in `try/except ValueError`; the preset validator was the remaining un-updated twin. Mirror the shared implementation: wrap `urlparse` + `.hostname`, re-raise as `PresetValidationError("Catalog URL is malformed: ...")`, and read the local `hostname` in the host check. Add a regression test mirroring `IntegrationCatalog`'s `test_malformed_url_rejected_cleanly`; it is red before the fix (raw `ValueError`) and green after. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
WOLIKIMCHENG
|
4fed84a08d |
fix(extensions): resolve __SPECKIT_COMMAND tokens in auto-registered skills (#3544)
* fix(extensions): resolve command ref tokens in extension skills * fix(extensions): render skill command refs by invocation style Resolve extension skill command-reference tokens with the active skill invocation style so Codex and ZCode use $speckit-* while slash-style agents keep their native forms. Preserve literal command-looking text. * fix(extensions): resolve slash skill command refs from init options --------- Co-authored-by: root <kinsonnee@gmail.com> |
||
|
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> |
||
|
Nate Chadwick
|
fd101d531e |
feat(integrations): add Grok Build skills-based integration (#3535)
* feat(integrations): add Grok Build skills-based integration Add first-class support for xAI Grok Build via SkillsIntegration, installing speckit skills under .grok/skills and wiring init/invocation/catalog surfaces. Assisted-by: Grok Build (model: grok-4, supervised) * test+docs: address Copilot review on Grok multi-install and next steps Assert init next-steps guidance for Grok (.grok/skills, /speckit-*) and clarify that multi-install safety is path/manifest isolation, not agent-context defaults such as shared AGENTS.md. * fix(integrations): Grok headless --always-approve and isolation paths Document Grok multi-install isolation as .grok/skills and .grok/rules. Override build_exec_args to pass --always-approve so non-interactive dispatch is not blocked at tool permission gates. * docs(integrations): list only managed .grok/skills for Grok isolation Multi-install isolation documents Spec Kit-managed paths; Grok only writes .grok/skills, so drop the read-only .grok/rules entry. * fix(integrations): always-slash Grok hooks and refresh catalog date Move grok to ALWAYS_SLASH_AGENTS so hooks never emit /speckit.plan when ai_skills is missing/false. Update slash-format tests, persist ai_skills on init, and bump catalog updated_at for the Grok entry. --------- Co-authored-by: Nate Chadwick <1232206+natechadwick@users.noreply.github.com> Co-authored-by: test <test@example.com> |
||
|
Manfred Riem
|
2fb18c73cb |
fix(integration): preserve ai_skills on use for skills-mode Copilot (#3550) (#3551)
`specify integration use copilot` against a Copilot install configured with `--integration-options "--skills"` dropped `"ai_skills": true` from init-options.json and regenerated extension commands in the legacy `.agent.md`/`.prompt.md` layout, contradicting `integration.json`'s stored `parsed_options.skills: true`. `_update_init_options_for_integration` only inspected `SkillsIntegration` / the instance `_skills_mode` flag. On the `use` path no `setup()` runs, so the freshly-resolved Copilot instance has `_skills_mode == False` and the stored skills intent in `parsed_options` was ignored. Thread the resolved `parsed_options` through and treat `parsed_options["skills"]` as skills mode. Adds a regression test that resets the registry singleton's `_skills_mode` to simulate a fresh process (in-process singleton reuse otherwise masks the bug). Assisted-by: GitHub Copilot (model: Claude Opus 4.8, autonomous) Copilot-Session: 06fb6ae9-f444-4dfd-ab3f-d0669c5d0604 Co-authored-by: Copilot App <223556219+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>
|
||
|
Ben Buttigieg
|
353851e966 |
fix(presets): seed constitution from preset constitution-template (#3272) (#3276)
* fix(presets): seed constitution from preset constitution-template (#3272) The constitution is the only template materialized to a live file (.specify/memory/constitution.md) rather than resolved on demand, yet ensure_constitution_from_template hardcoded a copy from the core template and ignored PresetResolver. Combined with init seeding the constitution before preset installation, a preset's constitution-template (e.g. strategy: replace with a ratified constitution) could never go live. Changes: - ensure_constitution_from_template now resolves constitution-template through PresetResolver, so a preset/override/extension wins and core is the fallback. - init seeds the constitution after preset installation so init --preset uses the resolved stack. - install_from_directory re-seeds memory/constitution.md from the resolved preset template, guarded to only act when the memory file is missing or still contains generic placeholder tokens — authored constitutions are never overwritten. Covers preset add and install_from_zip. - Tests for preset seeding, placeholder re-seed, authored-constitution preservation, override resolution, and resolver-aware init seeding. Assisted-by: GitHub Copilot (model: Claude Opus 4.8, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * fix(presets): compose constitution-template when seeding memory Take on review feedback from Copilot and gglachant: - constitution seeding previously copied the top layer file path verbatim even when the winning layer used a composing strategy (prepend/append/wrap), which could leave {CORE_TEMPLATE} unresolved. - both seeding paths now inspect resolver layers and only copy verbatim for replace; non-replace strategies materialize composed content via PresetResolver.resolve_content(). - add regression tests for wrap strategy composition in both PresetManager seeding and ensure_constitution_from_template. - add a drift-guard test pinning _CONSTITUTION_PLACEHOLDER_TOKENS to the placeholders in templates/constitution-template.md. Assisted-by: GitHub Copilot (model: GPT-5.3-Codex, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * refactor(presets): unify constitution template materialization Address latest Copilot feedback on the constitution seeding path: - moved resolver/layer I/O behind the existing-memory fast path in init - corrected tracker output for composed materialization - deduplicated materialization logic shared by init and preset install seeding into presets._materialize_constitution_template() Behavior is unchanged for replace strategies (copy verbatim) and remains composed for prepend/append/wrap via resolve_content(). Assisted-by: GitHub Copilot (model: GPT-5.3-Codex, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * fix(init): restore shutil import The constitution materialization refactor removed the module import, but init still uses shutil.rmtree when cleaning up a failed new-project initialization. Restore the import so the required ruff check passes. Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * fix(presets): harden constitution materialization Address the outstanding review batch for preset constitution seeding: - use checked atomic writes and reject symlinked memory paths - replace placeholder heuristics with hash/source provenance - rematerialize unchanged generated constitutions by resolver priority - preserve authored or edited constitutions, including placeholder mentions - warn non-fatally when post-install materialization cannot complete - retain exact core-template comparison for legacy projects without provenance Add focused provenance, priority, symlink, and failure-path coverage, and update integration inventories for the generated provenance sidecar. Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1b2c095d-b45c-4d52-8d56-bd6121d96ab6 * fix(presets): reconcile constitution after removal When the removed preset supplied constitution-template, rematerialize the winning remaining resolver layer only if provenance proves the live file is still generated and unchanged. Preserve edited constitutions and report post-removal reconciliation failures as non-fatal warnings. Add coverage for restoring the core layer, falling back from a removed higher-priority preset, and preserving edited generated content. Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1b2c095d-b45c-4d52-8d56-bd6121d96ab6 * fix(presets): tighten legacy constitution provenance Trust only the immutable bundled/source constitution template when migrating legacy projects without provenance. Do not infer core provenance from mutable project templates or preset source labels, including IDs beginning with core. Also detect convention-based constitution-template files before preset removal so unchanged generated constitutions reconcile to the next resolver layer. Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1b2c095d-b45c-4d52-8d56-bd6121d96ab6 * fix(presets): preserve files with invalid provenance Use immutable-core legacy migration only when the provenance sidecar is absent. If a sidecar is malformed or its hash does not match the live constitution, treat the file as edited and preserve it. Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1b2c095d-b45c-4d52-8d56-bd6121d96ab6 * fix(presets): reconcile constitution on stack changes Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1b2c095d-b45c-4d52-8d56-bd6121d96ab6 * fix(presets): guard constitution reconciliation edges Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1b2c095d-b45c-4d52-8d56-bd6121d96ab6 --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.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> |
||
|
Vincent Lee
|
d83b8d1188 |
fix: add trailing newline to init-options.json output (#3509)
`save_init_options()` omitted a final newline, causing `end-of-file-fixer` from .pre-commit-config.yaml (#3430) to flag a diff on every `specify integration upgrade` run. Append `\n` to the `json.dumps()` output to match POSIX expectations and align with `integration_state.py` which already includes the trailing newline. Ref: https://github.com/github/spec-kit/pull/3430 |
||
|
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 |
||
|
thejesh23
|
2537be8144 |
fix(extensions): stop env-var config leaking across prefix-colliding extension IDs (#3497)
* fix(extensions): stop env-var config leaking across prefix-colliding IDs (#3494) Because ``_`` doubles as both the separator between an extension ID and its config path AND the substitute for ``-`` inside an extension ID, an env var like ``SPECKIT_GIT_HOOKS_URL`` starts with *both* the ``SPECKIT_GIT_`` prefix of the ``git`` extension and the ``SPECKIT_GIT_HOOKS_`` prefix of a co-installed ``git-hooks`` extension. ``ConfigManager._get_env_config`` matched only on the shorter prefix, so the same env var silently surfaced inside both extensions' configs (as ``{'hooks': {'url': ...}}`` for ``git`` and ``{'url': ...}`` for ``git-hooks``). Impact: config intended for one extension leaked into another and, worse, could flip ``config.<field> is set`` hook conditions on the wrong extension. Route the env var to the extension whose normalized ID is the longest match — the more specific one. When another installed sibling's normalized ID + ``_`` claims the remainder, skip the var here. The sibling scan reads ``.specify/extensions/`` directly and degrades to a no-op if the dir is missing (fresh project / ad-hoc harness), so the pre-fix single-extension behaviour is unchanged when there is no collision. Distinct from #3350 (intra-extension prefix collision between two keys of the same extension) — this fixes the cross-extension case. Fixes #3494 * fix(extensions): source sibling scan from registry, not directory Address Copilot review on #3497: ``ExtensionManager.remove(..., keep_config=True)`` preserves the extension directory but drops the registry entry, so the previous directory-scan approach would treat a config-only leftover as an installed sibling and silently discard ``SPECKIT_<sibling>_*`` env vars into no owner. Sourced the sibling list from ``ExtensionRegistry.keys()`` — the registry is the source of truth for "installed" — and kept the same graceful ``[]`` fallback so the fresh-project / ad-hoc harness path is unaffected. Updated the ``TestConfigManagerCrossExtensionEnvLeak`` ``_install`` helper to register its fake installations and added ``test_config_only_leftover_not_treated_as_sibling`` to lock in the new behaviour for the ``keep_config=True`` scenario. Full suite: 3978 passed, 110 skipped. * fix(extensions): swallow non-UTF-8 registry in sibling scan Address Copilot follow-up on #3497: ``ExtensionRegistry._load()`` catches ``JSONDecodeError`` / ``FileNotFoundError`` but not decode failures — a registry file with invalid text encoding would surface a ``UnicodeDecodeError`` out of ``_sibling_extension_ids`` and break every config read instead of degrading to the documented pre-fix behaviour. Extend the fallback in ``_sibling_extension_ids`` to also catch ``UnicodeError`` and add ``test_non_utf8_registry_does_not_crash`` as a regression pin (kept ``_load()`` itself out of scope — that broader hardening belongs in a separate PR since it affects all readers). Full suite: 3979 passed, 110 skipped. |
||
|
Marsel Safin
|
6ab0c1dac1 |
fix(integrations): escape control characters in goose recipe YAML renderer (#3384)
* fix(integrations): escape control characters in goose recipe YAML renderer YAML forbids C0 control characters (except tab and newline) and DEL in every scalar form, and a bare CR acts as a line break inside a block scalar. _render_yaml wrote the body verbatim into a |2 literal block scalar, so such bodies produced recipes the YAML parser rejects. Detect block-scalar-unsafe characters and fall back to an escaped double-quoted scalar via yaml.safe_dump, mirroring the TOML renderer's fallback strategy from #3341. Fixes #3382 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(integrations): use sys.maxsize instead of float inf for yaml width Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(integrations): extend block-scalar guard to C1 controls and Unicode line breaks YAML's printable set excludes C1 controls (U+0080-U+009F except NEL), and YAML 1.1 treats NEL/LS/PS as line breaks inside a literal block scalar, so bodies carrying any of these still produced unparseable recipes. Widen the fallback guard to the full class and cover it in the regression loop. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(integrations): also treat surrogates and U+FFFE/U+FFFF as block-scalar unsafe YAML's printable set also excludes lone UTF-16 surrogates and the non-characters U+FFFE/U+FFFF; bodies carrying them still hit the literal block path and produced unparseable recipes. Extend the guard and the regression loop. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * docs(integrations): clarify YAML prompt serialization Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> |
||
|
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> |
||
|
Ali jawwad
|
fc1a3fd76c |
fix(presets): resolve() honors manifest-declared file: for installed presets (#3351)
* fix(presets): resolve() honors manifest-declared file: for installed presets PresetResolver.resolve()'s tier-2 (installed presets) loop was convention-only: it looked for templates/<name>.md and <name>.md, ignoring a preset manifest that declares the template with an explicit, non-convention file: path. So resolve() returned the core template (and resolve_with_source() misattributed source='core') while collect_all_layers()/resolve_content() correctly used the preset's declared file — a divergence inside the same class. It could also return a stray convention-path file the manifest deliberately points away from. Mirror collect_all_layers()'s manifest-first logic: use the declared file: when present (skip convention fallback if it's missing, to avoid masking typos), and fall back to the convention walk only when the manifest is absent or doesn't list the template. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(presets): clarify the empty/falsey manifest-file branch comment Per review: 'file' is a required key for every template entry (PresetManifest._validate()), so the manifest-found branch is reached for an empty/falsey/non-usable 'file' value, not a truly absent one. Reword the comment to say so. Comment-only. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(presets): resolve() returns only real files; test missing-file skip Per review: - Use is_file() (not exists()) when honoring a manifest-declared file: so a manifest pointing at a directory is treated as missing rather than returned to a caller that will read_text() it. Applied in both resolve() and collect_all_layers() so the two stay consistent. - Add a regression test for the skip-convention-fallback-when-declared-file- missing behavior: manifest declares a missing custom/spec.md while the pack has a convention templates/spec-template.md; resolve() must skip the pack and fall through to core, not pick up the stray convention file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(presets): resolve()/collect_all_layers() require a regular file for manifest file: A manifest-declared file: path is honored via exists(), which also accepts a directory. If a preset points file: at a directory, resolve() returned it and downstream read_text() crashes. Use is_file() in both resolve() and collect_all_layers() so a non-file (directory) is treated as missing and the convention fallback is skipped (pack yields to core), matching the existing missing-file behavior. Adds a directory-at-file: test (fails on exists(), passes on is_file()) that also asserts collect_all_layers() never returns the directory as a layer. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(presets): extract shared _manifest_declared_template for resolve()/collect_all_layers() Both methods reimplemented the manifest-entry lookup + authoritative-fallback rules independently — the exact duplication that let them diverge and caused the bug this PR fixes. Extract a single _manifest_declared_template(pack_dir, name, type) -> (entry, candidate) helper (candidate is the declared file only when it is_file(); a declared-but-unusable file returns (entry, None) so callers skip the convention fallback). resolve() and collect_all_layers() now both call it, so their manifest-first resolution cannot silently diverge again. Pure refactor, behavior-preserving: full test_presets.py (331) still passes, including the directory-at-file:, missing-file, and manifest-file-wins cases. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> |
||
|
Ali jawwad
|
993083405e |
fix(init): don't block on confirmation for 'init --here' without a TTY (#3236)
* fix(init): don't block on confirmation for 'init --here' without a TTY When 'specify init --here' targets a non-empty directory without --force, it called typer.confirm() unconditionally. In a non-interactive session (no TTY -- CI, piped, agent) there is no input, so the prompt reads EOF and aborts unhelpfully (or blocks), with no actionable message. The named-project path already fails fast and points to --force; --here was the inconsistent outlier. Guard the confirmation with the existing _stdin_is_interactive() helper: when non-interactive, print a clear 'directory not empty; re-run with --force' error and exit 1 instead of prompting. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(init): honor piped confirmation for 'init --here'; only fail-fast on empty stdin The first version of this fix short-circuited on '_stdin_is_interactive()' (isatty) before typer.confirm, which broke 'init --here' when confirmation is piped (e.g. 'echo y | specify init --here' / CliRunner input='y\n') -- a non-TTY pipe with valid input was wrongly rejected, regressing test_init_here_without_force_preserves_shared_infra. Instead, call typer.confirm normally (piped 'y'/'n' is honored) and catch the Abort/EOFError it raises only when stdin is empty, converting that to the actionable '--force' guidance. This keeps the UX win for the no-input case without rejecting piped input. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(init): distinguish interactive cancel from no-input; defer merge warning Address Copilot review on the --here non-empty path: (1) treat typer.Abort during an interactive confirm (e.g. Ctrl+C) as a normal cancellation (exit 0), and only emit the '--force' guidance + exit 1 when there is no TTY (empty stdin / EOF) -- no longer conflating the two; (2) move the 'will be merged / may overwrite' warning so it only shows when actually proceeding (force) or folded into the confirmation prompt, not on the fail-fast path where nothing is merged. Piped confirmation (e.g. 'echo y | specify init --here') is still honored, which is why the prompt is attempted rather than refused outright when non-interactive -- the existing test_init_here_without_force_preserves_shared_infra pipes 'y' and must succeed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(init): fail fast on non-interactive --here instead of prompting Per Copilot review: do not call typer.confirm when stdin is not a TTY -- an open-but-idle non-TTY stdin (CI/agent) could block on the prompt. When the directory is non-empty and --force is not given, fail fast with '--force' guidance unless an interactive terminal is present. Interactive confirm still offers the merge-but-preserve path (distinct from --force, which overwrites); a Ctrl+C there is treated as a normal cancellation (exit 0). The merge/overwrite warning is only printed when actually proceeding, not on the fail-fast path. Updated the preserve-merge E2E test to simulate an interactive terminal so it exercises the confirm path (non-interactive sessions now require --force). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(init): honor piped y/n for 'init --here', error only on no-input Per maintainer review: restore the second-revision shape. Calling typer.confirm normally keeps 'echo y | specify init --here' reaching the non-destructive preserve-merge path (and piped 'n' cancels with exit 0). Only when no confirmation input is available at all (closed/empty stdin -> typer.Abort/EOFError) is it converted into the actionable error that points at --force. This drops the _stdin_is_interactive fail-fast that broke the common piped-confirm idiom and made preserve-merge interactive-only. The preserve test no longer needs to monkeypatch _stdin_is_interactive - it passes on the real contract. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(init): preserve interactive-cancel semantics; fold merge risk into the prompt Two review-driven refinements to the 'init --here' non-empty confirm, keeping the maintainer-endorsed control flow (piped y/n honored; non-interactive EOF → actionable --force error): 1. typer.confirm raises typer.Abort for BOTH an interactive Ctrl+C and an EOF on closed/empty stdin. Catching it unconditionally reported 'no confirmation input available, use --force' and exited 1 even when the user cancelled at a real TTY. Branch on _stdin_is_interactive(): a TTY cancel is a normal exit 0 ('Operation cancelled'); only non-interactive EOF becomes the --force error. 2. Fold the merge-risk warning into the confirmation question instead of printing it unconditionally beforehand, so the EOF/no-input path (which exits without changing anything) no longer prints a misleading 'will be merged' line first. Adds test_init_here_interactive_cancel_exits_zero (fails before: exit 1 with --force; passes after: exit 0, 'cancelled', pre-existing file untouched). The non-interactive EOF and piped-y preserve-merge tests are unchanged and still pass. 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
|
c05a626cbc |
fix(integrations): exit cleanly on unbalanced quote in --integration-options (#3457) (#3466)
* fix(integrations): exit cleanly on unbalanced quote in --integration-options (#3457) `_parse_integration_options` called `shlex.split(raw_options)` unguarded, so an unbalanced quote in the flag value (e.g. `--integration-options='--commands-dir "foo'`) made shlex raise `ValueError: No closing quotation` and a raw traceback escaped — unlike every other bad-input path in this function (unknown option, missing value, unexpected value), which print a message and exit 1. Reachable from `specify init --integration-options=...` and every `specify integration install/switch/upgrade/migrate --integration-options=...`. Wrap the split in a try/except ValueError that prints a one-line error and raises `typer.Exit(1)`, matching the existing loud-fail UX. Add a test asserting the unbalanced-quote input raises `typer.Exit` with exit code 1. 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> |
||
|
Noor ul ain
|
0acb5c6461 |
fix(integrations): declare kiro-cli multi-install safe (#3471) (#3485)
kiro-cli confines all of its managed files to an isolated agent root
(`.kiro/`, with commands in `.kiro/prompts`) that no other integration
writes to, so it meets every documented criterion for multi-install
safety — but `KiroCliIntegration` never set `multi_install_safe = True`.
As a result, co-installing kiro-cli alongside any other integration left
`specify integration status` permanently in ERROR:
error unsafe-multi-install: Installed integrations are not all
declared multi-install safe: kiro-cli
`--force` bypasses the install-time gate but does not clear the status
error, and there is no flag or config to acknowledge it, so the error is
permanent while both integrations remain installed.
Set `multi_install_safe = True`. The registry's parametrized
multi-install-safe contract tests (static isolated root, distinct agent
roots / command dirs, disjoint manifests) now cover kiro-cli
automatically, and a focused regression test pins the declaration so a
future edit cannot silently drop it and reintroduce the error.
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> |
||
|
NgoQuocViet2001
|
6664cf813c |
fix: mark Kiro integration as multi-install safe (#3472)
Assisted-by: Codex (model: GPT-5, autonomous) |
||
|
Marsel Safin
|
3b7d95a408 |
fix: rewrite extension-relative subdir paths in generated command bodies (#3444)
* fix: rewrite extension-relative subdir paths in generated command bodies Extension command bodies reference bundled files relative to the extension root (agents/, knowledge-base/, templates/, ...). Generated SKILL.md and command files emitted those paths verbatim, so agents resolved them against the workspace root where they do not exist. Add CommandRegistrar.rewrite_extension_paths, which rewrites references to subdirectories that actually exist in the installed extension to .specify/extensions/<id>/..., and call it once in register_commands so every output format and alias gets the fix. commands/, specs/ and dot-directories are never rewritten. Fixes #2101 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: only rewrite relative extension path references Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: use callable re.sub replacement for extension subdir rewrite subdir and extension_id come from filesystem directory names and were interpolated into a re.sub string replacement template. A directory name containing a backslash (e.g. assets\q) would raise re.error: bad escape, aborting command registration even when the body didn't reference it. Use a callable replacement so these values are treated literally. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: make subdir rewrite regression test cross-platform Renamed the test's subdir fixture from "assets\\q" to "assets[q]": on Windows, backslash is a path separator, so mkdir would create nested "assets/q" dirs instead of one literally-named directory, and iterdir() would only discover "assets", never exercising the rewrite. extension_id keeps a real backslash/"\\1" since it isn't used to create a directory, still verifying the callable replacement handles it literally. Added a sanity assertion for this assumption. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: apply extension subdir path rewrite in skills-mode renderer register_commands() rewrote extension-relative subdir references (agents/, knowledge-base/, etc.) via rewrite_extension_paths(), but _register_extension_skills() - the separate renderer used for active non-native skills agents (e.g. Claude with ai_skills: true) - never called it. Generated SKILL.md files left agents/... and knowledge-base/... unresolved, and mapped the extension's own templates/ through the generic project-level rewrite instead of its installed .specify/extensions/<id>/templates/ location. Reuse the existing rewrite_extension_paths() helper in _register_extension_skills() at the same point register_commands() applies it (before resolve_skill_placeholders' generic rewrite), and add a skills-mode regression test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: apply extension subdir path rewrite on preset restore/reconcile paths _unregister_skills() restored extension-backed SKILL.md content via resolve_skill_placeholders() without first calling rewrite_extension_paths(), so removing a preset override that shadowed an extension command restored the bare, unresolvable agents/... and knowledge-base/... references. Carried extension_id/extension_dir through _build_extension_skill_restore_index() and applied the same rewrite used at initial registration before restoring. Found the identical gap in _reconcile_composed_commands()'s non-skill agent path: when a removed preset's command reverts to an extension winner, register_commands_for_non_skill_agents() was called without extension_id, so the rewrite never ran for plain command-file agents either. Passed extension_id through there too. Added regression tests for both restore paths (skills-mode and non-skill-agent command files) in tests/test_presets.py. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: apply extension subdir path rewrite when composing over extension base PresetResolver.resolve_content() read the effective base layer's raw content directly via path.read_text() before composing append/prepend/ wrap overlays on top of it, and its outright-replace shortcut did the same. When that base layer was extension-provided, neither read path applied rewrite_extension_paths(), so composing a preset over an extension command (or an extension winning outright through resolve_content) left bare, unresolvable agents/... and knowledge-base/... references in the composed output. All three call sites (PresetManager._register_commands()'s composed path, _reconcile_composed_commands()'s composed path, and skills-mode reading the .composed file written by either) consume resolve_content's return value, so fixing the read at its source covers command output, skill output, and both initial-install and reconcile flows without threading extension identity through each caller. Tagged extension layers in collect_all_layers() with extension_id/ extension_dir, and added a _read_layer_content() helper in resolve_content() that applies rewrite_extension_paths() whenever a layer carries that extension identity — used at both raw-read sites (outright-replace shortcut and composition base). Composing (append/prepend/wrap) layers are never extension-provided (extensions are always inserted with strategy "replace"), so no other read site needs the rewrite. Added regression tests: a parametrized resolve_content() test covering append/prepend/wrap composing over an extension base, a skills-mode test asserting the composed SKILL.md resolves the extension's subdir references, and a non-skill-agent (Gemini) install-time test matching the reported live repro. Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.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
|
903d707d21 |
fix(extensions): set-priority repairs corrupted boolean priority (#3268)
The set-priority skip guard 'isinstance(raw_priority, int) and raw_priority == priority' treats a stored boolean as a match because isinstance(True, int) is True and True == 1 (False == 0). So a corrupted boolean priority short-circuits to 'already has priority N' and is never rewritten to a real int — contradicting the adjacent comment that promises corrupted values get repaired. Exclude bools explicitly, mirroring normalize_priority's own bool guard. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Ali jawwad
|
f6f3540409 |
fix(presets): set-priority repairs corrupted boolean priority (#3269)
Same bool-is-int trap as the extension set-priority command: the skip guard 'isinstance(raw_priority, int) and raw_priority == priority' treats a stored boolean as a match (isinstance(True, int) is True, True == 1), so a corrupted boolean priority reports 'already has priority N' and is never rewritten to a real int — contradicting the adjacent comment. Exclude bools explicitly, mirroring normalize_priority's bool guard. 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> |
||
|
Ali jawwad
|
ba1f13a8b1 |
fix(bundle): reject file:// / local download_url — catalog URLs are HTTPS-only (#3344)
* fix(bundle): resolve file:// download_url via the file-URL helper _download_manifest built the local path from raw parsed.path, which keeps the leading slash of file:///C:/x (yielding a \C:\x path that never exists on Windows) and skips percent-decoding (my%20bundles stays encoded on every OS) — so a catalog entry whose download_url is the canonical URI Python itself produces via Path.as_uri() always fails with 'Bundle manifest not found'. Route the file scheme through the existing bundler.services.adapters._file_url_to_path helper, which already handles drive letters, UNC hosts, and percent-decoding for catalog file:// URLs (make_catalog_fetcher). The bare-path branch is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(bundle): reject file:// / local download_url; catalog URLs are HTTPS-only Per maintainer review (route B): file:// in a catalog download_url was never intended — catalog URLs are HTTPS-only (http for localhost) across the extensions/presets/workflows catalog systems, and disk installs go through the positional path (specify bundle install <path>), handled by _local_manifest_source before catalog resolution. Remove the file:///bare-path branch from _download_manifest and route everything through _download_remote_manifest (HTTPS-only via _require_https), with an actionable error pointing at the positional install. Invert the file:// tests to assert rejection (+ a positional-path resolution test), and migrate the three bundle-info contract tests off local download_urls onto an HTTPS-only entry with a mocked manifest fetch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(bundle): validate HTTPS before the offline gate in _download_manifest Per review: for a non-local download_url the offline check ran before any URL validation, so an invalid/non-HTTPS scheme surfaced a misleading 'Network access disabled' error under --offline when the real problem is the URL would be rejected even online. Call _require_https before the offline gate so the correct error is reported in every mode. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(bundle): reword non-HTTPS download_url error to not mislabel scheme-less URLs A scheme-less download_url (urlparse scheme == "") can be a bare filesystem path OR a missing-scheme value like 'example.com/foo.zip', not necessarily file://. Reword the reject error to state the real HTTPS-only constraint and enumerate what is rejected (file://, local path, scheme-less), instead of labeling every case 'local/file://'. Behavior unchanged; the 'bundle install' actionable hint is preserved, so the existing reject-path tests still pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> |
||
|
Ali jawwad
|
e59da78677 |
fix(extensions): handle prefix-colliding env vars in _get_env_config (#3350)
* fix(extensions): handle prefix-colliding env vars in _get_env_config
_get_env_config built the nested dict with 'if part not in current:
current[part] = {}' and an unconditional leaf assignment. Two env vars
that collide on a prefix — e.g. SPECKIT_X_CONNECTION and
SPECKIT_X_CONNECTION_URL — then either crash (scalar processed first:
the walk indexes into a str -> TypeError 'str object does not support
item assignment') or silently clobber the nested dict (scalar processed
last). Via should_execute_hook's blanket except, the crash silently
disables every config-based hook for the extension. Guard the walk and
the leaf assignment with isinstance checks so a colliding scalar yields
to the nested dict; result is order-independent
({'connection': {'url': ...}} either way), matching _merge_configs'
dict-preserving semantics.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* test(extensions): assert via public should_execute_hook, not private helper
Per review: the colliding-env hook test described should_execute_hook
swallowing the TypeError, but asserted on the private _evaluate_condition.
Assert on the public should_execute_hook instead — it returns False
(silently disabled) before the fix and True after, matching the
real-world failure mode and not coupling to a private helper.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(extensions): ignore malformed env var names in _get_env_config
Per review: a name like SPECKIT_<EXT>_ (no key) or with consecutive
underscores produced empty path components, creating surprising entries
under an empty key (env_config[''] = ...). Filter out empty parts and
skip the variable entirely when nothing remains.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
|
||
|
Quratulain-bilal
|
74d03a2814 |
fix(auth): return no matches, not raw ValueError, for a malformed URL (#3437)
find_entries_for_url did (urlparse(url).hostname or "").lower() unguarded. a malformed authority (e.g. an unterminated ipv6 bracket "https://[::1") makes urlparse/hostname raise ValueError, so instead of the empty list the function already returns for a host-less url, a raw ValueError leaked out of the shared http client (build_request / open_url call this before any url validation). no auth entry can match such a url, so treat it like the host-less case and return no matches. added a regression test over an unterminated bracket and a bracketed non-ip host; confirmed it fails on the pre-fix code. |
||
|
Quratulain-bilal
|
14fa3ada08 |
fix(catalogs): raise catalog error, not raw ValueError, on a malformed URL (#3435)
CatalogStackBase._validate_catalog_url read parsed.hostname, which raises ValueError on a malformed authority (e.g. an unclosed ipv6 bracket "https://[::1"). every other reject path raises the class catalog error, so the raw ValueError leaked to the caller. wrap the parse + hostname access and convert ValueError to the normal error via cls._error. twin of the bundler fix in adapters._validate_remote_url. |
||
|
Quratulain-bilal
|
1736f0746b |
fix(bundler): raise BundlerError, not raw ValueError, on a malformed catalog URL (#3433)
adapters._validate_remote_url reads parsed.hostname, which raises ValueError on a malformed authority (e.g. an unclosed ipv6 bracket https://[::1). the function's contract is to raise BundlerError for any bad url - every other reject path does - so the raw ValueError leaked to the caller and crashed the fetch instead of failing cleanly. wrap the parse and convert to BundlerError. bundler sibling of #3369, which fixed the cli extension/preset/workflow add paths but not this validator. added a regression test that fails pre-fix. |
||
|
Vincent Lee
|
c8ce488073 |
chore: add pre-commit config and fix trailing whitespace/end-of-file (#3430)
issues |
||
|
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> |
||
|
Marsel Safin
|
55da30c66d |
feat(templates): add py: lines to command templates' scripts frontmatter (#3403)
* feat(templates): add py: lines to command templates' scripts frontmatter Every templates/commands/*.md with a scripts: block now declares a py: variant so --script py renders a Python invocation via the existing interpreter-prefixing in process_template. Fixes #3283 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: install scripts/python for the py script type install_shared_infra mapped every non-sh script type to powershell, so --script py rendered invocations pointing at files that were never installed. Map py to the python variant dir and skip __pycache__ artifacts during the copy. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test: read templates with explicit utf-8 encoding Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: keep py: lines standalone, enforce script existence in tests Drop the plan/tasks py: lines that referenced scripts shipping in the core port (#3280); they move to that PR. Tests now assert every py: line points at a script the repo ships, so a dangling reference can never merge green. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> |
||
|
Marsel Safin
|
15ac745e8d |
fix(integrations): skip Windows Store python3 alias stub in resolve_python_interpreter (#3385)
* fix(integrations): skip Windows Store python3 alias stub in resolve_python_interpreter
On stock Windows, python3 on PATH is the Microsoft Store App Execution
Alias stub: it exists but only prints an installer hint and exits
non-zero, so generated {SCRIPT} invocations for the py script type were
broken. Verify the found interpreter actually runs before accepting it,
on Windows only, mirroring the parse-success-not-availability approach
of #3312/#3320 for the sh scripts. POSIX keeps the plain existence
check. sys.executable remains the fallback and is always live.
Fixes #3383
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* fix(integrations): probe interpreter isolated and without site, discard I/O
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* test: pin POSIX platform in PATH-resolution tests
The tests fake shutil.which with POSIX paths; on Windows CI the real
sys.platform made the stub probe run against those fake paths and
fall through to sys.executable.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
---------
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
||
|
Marsel Safin
|
8e2e2d2f25 |
fix(integrations): escape control characters in SKILL.md frontmatter (#3399)
yaml.safe_dump with default_style='"' replaces the hand-rolled quote helpers in base.py and hermes, so newlines and control characters in template descriptions round-trip instead of producing unparseable YAML. Fixes #3391 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>
|
||
|
Ali jawwad
|
892dd656f2 |
fix(shared-infra): refresh_shared_templates preserves recovered user files (#3378)
* fix(shared-infra): refresh_shared_templates preserves recovered user files refresh_shared_templates skipped a shared template only when it was untracked or modified, ignoring the manifest's is_recovered marker that install_shared_infra already honors. So a pre-existing user template (adopted via record_existing(recovered=True), hence tracked and hash-unmodified) was silently overwritten with bundled content on refresh — the exact data-loss class that #2918 fixed for install_shared_infra. Add the is_recovered check to the skip predicate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(shared-infra): include recovered files in refresh skip warning The skip predicate now also skips recovered (pre-existing user) files, so the warning saying only 'modified or untracked' could mislead a user into thinking they edited a file that was simply recorded as recovered. Reword to 'modified, untracked, or preserved (recovered)'. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> |
||
|
Ali jawwad
|
54ed736479 |
fix(agents): resolve skill placeholders in Goose (yaml) command output (#3374)
* fix(agents): resolve skill placeholders in Goose (yaml) command output
CommandRegistrar.register_commands resolves {SCRIPT}/__AGENT__ and the
$ARGUMENTS placeholder in the markdown and toml branches, but the yaml
branch (Goose recipes) called render_yaml_command directly, skipping
both. So extension/preset command bodies installed for Goose kept literal
{SCRIPT}, __AGENT__, and repo-relative script paths in the generated
.goose/recipes/*.yaml prompt. Mirror the markdown/toml branches: run
resolve_skill_placeholders + _convert_argument_placeholder on the body
before render_yaml_command.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* test(goose): assert positive placeholder replacements in recipe prompt
Per review: parse the generated recipe with yaml.safe_load and assert the
prompt contains the resolved values (.specify/scripts/, 'agent goose',
{{args}}), not merely that the literal tokens are absent — a wrong output
that happens to omit the exact strings would otherwise pass.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
|
||
|
Ali jawwad
|
7b1065d857 |
fix(bundler): enforce version pin on bundled preset/extension installs (#3377)
* fix(bundler): enforce version pin on bundled preset/extension installs The bundled install branch of _PresetKindManager/_ExtensionKindManager called install_from_directory and returned before _assert_pinned_version, so a bundle manifest pinning e.g. 2.0.0 would silently install the bundled asset's own version (1.0.0) — the pin was only enforced on the catalog path. _WorkflowKindManager already enforces it unconditionally. Read the bundled asset's declared version from its manifest (best-effort; None => cannot enforce, matching the catalog 'advertises no version' escape hatch) and call _assert_pinned_version before install, in both bundled branches. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(bundler): address review on bundled version-pin check - Make _assert_pinned_version's error source-agnostic ('resolved version' / 'the source') so bundled preset.yml/extension.yml mismatches read correctly, not just catalog ones. - Type-guard _bundled_manifest_version: only a non-empty string version is usable; missing/non-string/whitespace -> None ('cannot enforce'). - Add bundled-preset success-path test (matching pin + version=None both proceed to install_from_directory), mirroring the extension test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> |
||
|
Quratulain-bilal
|
94c7ec288f |
fix(toml): escape control characters so generated command files parse (#3341)
* fix(toml): escape control characters so generated command files parse both toml renderers (TomlIntegration._render_toml_string for gemini/tabnine and CommandRegistrar.render_toml_command for extension/preset commands) wrote control characters raw into a multiline or basic string. toml forbids literal control chars (U+0000-U+001F except tab/newline, and U+007F) in every string form, and a bare CR that is not part of a CRLF pair, so a description or body containing one produced a .toml file that fails to parse. route any value with such a character to a fully-escaped basic string that emits the leftover control chars as \uXXXX. added regression tests that round-trip through tomllib. * refactor(toml): centralize control-char escaping in one shared helper the control-char detection and basic-string escaping added for both toml renderers were copy-pasted into agents.py and integrations/base.py. move the two functions into specify_cli/_toml_string.py and have both renderers delegate to it, so the escaping rules can't drift apart later. no behavior change; both renderers now reference the same implementation. |
||
|
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> |