From 53dd32350beda3fd6009232311c40bd55f5e291a Mon Sep 17 00:00:00 2001 From: Bernard van der Esch Date: Tue, 5 May 2026 12:06:29 +0200 Subject: [PATCH 1/6] skill custom config --- config.yaml | 24 +- ...lds-schema-in-manifest-yaml-and-parsing.md | 270 ++++++++++++++++++ .../sprint-status.yaml | 6 +- nimble/manifest/parser.py | 84 ++++++ tests/unit/manifest/test_parser.py | 150 ++++++++++ 5 files changed, 522 insertions(+), 12 deletions(-) create mode 100644 docs/bmad_output/implementation-artifacts/8-1-config-fields-schema-in-manifest-yaml-and-parsing.md diff --git a/config.yaml b/config.yaml index ea90f73..65cc17c 100644 --- a/config.yaml +++ b/config.yaml @@ -1,11 +1,17 @@ -skills: - - name: hello_world - source: local - path: skills/hello_world/skill.py - class_name: HelloWorldSkill - binding: "ctrl+l" - ai: - provider: anthropic - model: claude-sonnet-4-6 api_key_env: ANTHROPIC_API_KEY + model: claude-sonnet-4-6 + provider: anthropic +skills: +- binding: ctrl+l + class_name: HelloWorldSkill + name: hello_world + path: skills/hello_world/skill.py + source: local +- binding: ctrl+t+r + class_name: TranslateSkill + installed_from: https://github.com/adeptofvoltron/nimble-translate-skill + name: translate + path: .nimble/skills/translate/skill.py + source: community + version: 1.0.0 diff --git a/docs/bmad_output/implementation-artifacts/8-1-config-fields-schema-in-manifest-yaml-and-parsing.md b/docs/bmad_output/implementation-artifacts/8-1-config-fields-schema-in-manifest-yaml-and-parsing.md new file mode 100644 index 0000000..a3a902d --- /dev/null +++ b/docs/bmad_output/implementation-artifacts/8-1-config-fields-schema-in-manifest-yaml-and-parsing.md @@ -0,0 +1,270 @@ +# Story 8.1: `config_fields` Schema in `manifest.yaml` and Parsing + +Status: done + + + +## Story + +As a skill author, +I want to declare named configuration fields in my `manifest.yaml` with descriptions, optional defaults, and optional value constraints, +So that users and `nimble add` know exactly what to provide when installing or configuring my skill. + +## Acceptance Criteria + +1. **Given** a `manifest.yaml` with a `config_fields` block containing a field with `key`, `description`, and `default` + **When** `parse_manifest_yaml()` parses it + **Then** it returns a `ManifestSpec` with a populated `config_fields: list[ConfigFieldSpec]` containing the correct values + +2. **Given** a `config_fields` entry with `possible_values: [en, es, fr]` and `default: en` + **When** parsed + **Then** `ConfigFieldSpec.possible_values == ["en", "es", "fr"]` and `ConfigFieldSpec.default == "en"` + +3. **Given** a `config_fields` entry with no `possible_values` + **When** parsed + **Then** `ConfigFieldSpec.possible_values is None` — the field accepts any string value + +4. **Given** a `manifest.yaml` with no `config_fields` key + **When** parsed + **Then** `ManifestSpec.config_fields == []` — absence is not an error + +5. **Given** a `config_fields` entry missing the required `key` or `description` field + **When** parsed + **Then** a `ManifestError` is raised identifying the missing field — install is aborted + +## Tasks / Subtasks + +- [x] Task 1: Add `ConfigFieldSpec` dataclass to `nimble/manifest/parser.py` (AC: 1, 2, 3) + - [x] Define `@dataclass class ConfigFieldSpec` with fields: `key: str`, `description: str`, `default: str | None = None`, `possible_values: list[str] | None = None` + - [x] Place it above `ManifestSpec` in the file — it must be defined before `ManifestSpec` references it + - [x] Add full `mypy --strict` compatible annotations (no bare `Optional`, use `X | None` syntax for Python 3.10+) + +- [x] Task 2: Add `config_fields` field to `ManifestSpec` (AC: 1, 4) + - [x] Add `config_fields: list[ConfigFieldSpec] = field(default_factory=list)` to the `ManifestSpec` dataclass + - [x] Use `field(default_factory=list)` — consistent with existing `requires` field pattern (line 37 of `parser.py`) + +- [x] Task 3: Add `_parse_config_fields()` helper (AC: 1, 2, 3, 5) + - [x] Add `def _parse_config_fields(data: dict[str, Any], source: str) -> list[ConfigFieldSpec]:` function + - [x] If `config_fields` key is absent → return `[]` + - [x] If `config_fields` value is not a list → raise `ManifestError` with clear message + - [x] For each entry: validate it is a `dict`; check `key` and `description` are present and non-empty strings — raise `ManifestError` if missing + - [x] Parse `default` as `str | None` (absent → `None`) + - [x] Parse `possible_values` as `list[str] | None` (absent → `None`; present but non-list → raise `ManifestError`) + - [x] Return `list[ConfigFieldSpec]` + +- [x] Task 4: Wire `_parse_config_fields()` into `parse_manifest_yaml()` (AC: 1, 4) + - [x] In `parse_manifest_yaml()`, after building other fields, call `_parse_config_fields(data, source)` + - [x] Pass result as `config_fields=` kwarg to `ManifestSpec(...)` constructor + +- [x] Task 5: Write tests in `tests/unit/manifest/test_parser.py` (AC: 1–5) + - [x] Test: manifest with `config_fields` block → `ManifestSpec.config_fields` populated correctly + - [x] Test: `possible_values` parsed as list of strings + - [x] Test: `possible_values` absent → `ConfigFieldSpec.possible_values is None` + - [x] Test: no `config_fields` key → `ManifestSpec.config_fields == []` + - [x] Test: entry missing `key` → `ManifestError` raised + - [x] Test: entry missing `description` → `ManifestError` raised + - [x] Add `ConfigFieldSpec` to the import line in the test file + +- [x] Task 6: Quality gates + - [x] `flake8 nimble/ tests/ worker/` — exits 0 (max line length 88 per pyproject.toml) + - [x] `mypy nimble/ tests/ worker/ --strict` — exits 0 (7 pre-existing errors unrelated to this story) + - [x] `pytest tests/` — all tests pass (307 passed; 7 pre-existing clipboard failures unrelated to this story) + +### Review Findings + +- [x] [Review][Patch] Validate `config_fields[].key` and `config_fields[].description` as non-empty strings (do not coerce arbitrary types via `str(...)`) [`nimble/manifest/parser.py:96`] +- [x] [Review][Patch] Validate `default` type (`None | str`) and enforce `default in possible_values` when `possible_values` is provided [`nimble/manifest/parser.py:96`] +- [x] [Review][Patch] Add negative-path tests for invalid `config_fields` shapes/types (non-list root, non-mapping entry, non-string `default`, invalid `possible_values` item types) [`tests/unit/manifest/test_parser.py:648`] + +## Dev Notes + +### What This Story Delivers + +Two things only: +1. `ConfigFieldSpec` dataclass in `nimble/manifest/parser.py` +2. `config_fields` field on `ManifestSpec`, populated by `parse_manifest_yaml()` + +**Nothing else.** Specifically out of scope: +- `SkillConfig.configuration` field (Story 8.2) +- Parsing `configuration:` block from `config.yaml` (Story 8.2) +- Worker injection of `self.configuration` (Story 8.2) +- `nimble add` interactive prompting (Story 8.3) +- `append_skill_to_config()` writing a `configuration:` block (Story 8.3) + +### Exact File to Modify + +**One file changed:** `nimble/manifest/parser.py` +**One file of new tests:** `tests/unit/manifest/test_parser.py` (extend existing file — do NOT create a new test file) + +### `ConfigFieldSpec` — Exact Shape + +```python +@dataclass +class ConfigFieldSpec: + key: str + description: str + default: str | None = None + possible_values: list[str] | None = None +``` + +Place this **before** `ManifestSpec` in `parser.py` (currently around line 28) so `ManifestSpec` can reference it. The file already imports `dataclass` and `field` from `dataclasses` (line 8). + +### `ManifestSpec` — Add One Field + +Add to the end of the existing `ManifestSpec` dataclass, using the same pattern as `requires` (line 37–38): + +```python +config_fields: list[ConfigFieldSpec] = field(default_factory=list) +``` + +`ManifestSpec` already has `requires: list[str] = field(default_factory=list)` — follow that exact pattern. + +### `_parse_config_fields()` — Implementation Contract + +```python +def _parse_config_fields(data: dict[str, Any], source: str) -> list[ConfigFieldSpec]: + raw = data.get("config_fields") + if raw is None: + return [] + if not isinstance(raw, list): + raise ManifestError( + f"manifest.yaml from {source} field 'config_fields' must be a list" + ) + result: list[ConfigFieldSpec] = [] + for i, entry in enumerate(raw): + if not isinstance(entry, dict): + raise ManifestError( + f"manifest.yaml from {source} config_fields[{i}] must be a mapping" + ) + for required in ("key", "description"): + if required not in entry: + raise ManifestError( + f"manifest.yaml from {source} config_fields[{i}]" + f" missing required field '{required}'" + ) + key = str(entry["key"]) + description = str(entry["description"]) + default = str(entry["default"]) if "default" in entry else None + raw_pv = entry.get("possible_values") + if raw_pv is None: + possible_values = None + elif not isinstance(raw_pv, list) or any( + not isinstance(v, str) for v in raw_pv + ): + raise ManifestError( + f"manifest.yaml from {source} config_fields[{i}]" + " 'possible_values' must be a list of strings" + ) + else: + possible_values = raw_pv + result.append( + ConfigFieldSpec( + key=key, + description=description, + default=default, + possible_values=possible_values, + ) + ) + return result +``` + +Use `_parse_manifest_string_list()` as a reference for the helper style (line 76–87 of `parser.py`), but `_parse_config_fields()` is more complex so it gets its own loop. + +### Wiring into `parse_manifest_yaml()` + +At the end of `parse_manifest_yaml()`, just before the `return ManifestSpec(...)` call (currently around line 366), add: + +```python +config_fields = _parse_config_fields(data, source) +``` + +Then extend `ManifestSpec(...)` with `config_fields=config_fields`. + +### Test Patterns to Follow + +Existing tests in `test_parser.py` use `_VALID_MANIFEST_YAML` (line 339) as a base and derive variants via `.replace()` or string concatenation. Follow the same pattern for `config_fields` tests: + +```python +# Append config_fields block to _VALID_MANIFEST_YAML in tests: +_VALID_MANIFEST_WITH_CONFIG_FIELDS = ( + _VALID_MANIFEST_YAML + + "config_fields:\n" + " - key: target_language\n" + " description: Target language code\n" + " default: en\n" + " possible_values:\n" + " - en\n" + " - es\n" + " - fr\n" +) +``` + +Add `ConfigFieldSpec` to the import block at the top of the test file (line 12–26). + +### Architecture Compliance + +- `mypy --strict` is enforced project-wide — every new function and field must be fully annotated +- Max line length: 88 (pyproject.toml `[tool.black]`) +- Import order: stdlib → third-party → local (enforced by flake8) +- `from __future__ import annotations` already at top of `parser.py` (line 1) — `str | None` syntax works fine for Python 3.10+ +- All tests go in `tests/unit/manifest/test_parser.py` — do NOT create a new test file + +### `_make_manifest_spec()` in Tests + +`test_parser.py` already has a `_make_manifest_spec()` helper (line 513) that constructs a `ManifestSpec` via `**defaults`. After adding `config_fields` to `ManifestSpec`, this helper's `defaults` dict does not need to include `config_fields` — the dataclass default `field(default_factory=list)` handles it. + +However, check that `ManifestSpec(**defaults)` still works — `config_fields` will default to `[]` when not provided. The test `test_parse_manifest_yaml_valid()` (line 354) creates a `ManifestSpec` implicitly via `parse_manifest_yaml()` — it should continue to pass as `config_fields` will default to `[]` when the YAML has no `config_fields` block. + +### Previous Story Intelligence (Epic 7 / Git History) + +Stories 7.x were documentation-only (README, skill-build.md, autostart). No Python patterns from them apply here. + +The most relevant precedent is **Epic 6** (manifest parsing work) — particularly: +- `ManifestSpec` schema extensions follow the same `@dataclass` + `field(default_factory=...)` pattern +- `ManifestError` is already the standard exception for malformed `manifest.yaml` — use it (do NOT introduce new exception types) +- `_parse_manifest_string_list()` demonstrates the helper pattern for reusable sub-parsers + +Git note: commit `658430f` ("epic 8 define-configuration") only added planning docs — no source code changes. The codebase is at the state from Epic 7 completion. + +### Project Structure Notes + +All changes are confined to: +- `nimble/manifest/parser.py` — one `@dataclass`, one field addition, one helper function, one wiring call +- `tests/unit/manifest/test_parser.py` — extend with new test cases and import + +No other files need to change for Story 8.1. + +### References + +- Epic 8 story 8.1 AC: [Source: docs/bmad_output/planning-artifacts/epics.md#Story-8.1] +- `ManifestSpec` + `_parse_manifest_string_list()` + `ManifestError`: [Source: nimble/manifest/parser.py#1-409] +- Existing test patterns for `parse_manifest_yaml()`: [Source: tests/unit/manifest/test_parser.py#339-450] +- `mypy --strict` enforcement: [Source: pyproject.toml#[tool.mypy]] +- `flake8` line-length 88: [Source: pyproject.toml#[tool.black]] +- `field(default_factory=list)` precedent: [Source: nimble/manifest/parser.py#37] +- FR46 definition: [Source: docs/bmad_output/planning-artifacts/epics.md#161] + +## Dev Agent Record + +### Agent Model Used + +claude-sonnet-4-6 + +### Debug Log References + +### Completion Notes List + +- Added `ConfigFieldSpec` dataclass above `ManifestSpec` in `parser.py` with `key`, `description`, `default`, and `possible_values` fields, all fully annotated for `mypy --strict`. +- Extended `ManifestSpec` with `config_fields: list[ConfigFieldSpec] = field(default_factory=list)` following the existing `requires` field pattern. +- Implemented `_parse_config_fields()` helper that validates each entry for required `key`/`description`, parses optional `default` and `possible_values`, and raises `ManifestError` on malformed input. +- Wired `_parse_config_fields()` into `parse_manifest_yaml()` — absence of `config_fields` in YAML yields `[]` (no breaking change to existing tests). +- Added 6 new tests in `test_parser.py` covering all 5 ACs; all pass. No regressions introduced. Pre-existing mypy errors (7) and clipboard test failures (7) are unaffected. + +### File List + +- nimble/manifest/parser.py +- tests/unit/manifest/test_parser.py + +### Change Log + +- 2026-05-05: Story 8.1 implemented — added `ConfigFieldSpec` dataclass, `config_fields` field on `ManifestSpec`, `_parse_config_fields()` helper, wired into `parse_manifest_yaml()`, and 6 new unit tests. diff --git a/docs/bmad_output/implementation-artifacts/sprint-status.yaml b/docs/bmad_output/implementation-artifacts/sprint-status.yaml index cca2019..356aaff 100644 --- a/docs/bmad_output/implementation-artifacts/sprint-status.yaml +++ b/docs/bmad_output/implementation-artifacts/sprint-status.yaml @@ -35,7 +35,7 @@ # - Dev moves story to 'review', then runs code-review (fresh context, different LLM recommended) generated: "2026-04-16" -last_updated: "2026-05-04" # updated by correct-course: added Epic 8 skill configuration +last_updated: "2026-05-05" # updated by dev-story: 8-1 review project: "pixi (Nimble)" project_key: NOKEY tracking_system: file-system @@ -106,8 +106,8 @@ development_status: epic-7-retrospective: optional # Epic 8: Skill Configuration — Pass Parameters from config.yaml to Skills - epic-8: backlog - 8-1-config-fields-schema-in-manifest-yaml-and-parsing: backlog + epic-8: in-progress + 8-1-config-fields-schema-in-manifest-yaml-and-parsing: done 8-2-configuration-in-config-yaml-and-worker-injection: backlog 8-3-interactive-config-prompting-in-nimble-add: backlog epic-8-retrospective: optional diff --git a/nimble/manifest/parser.py b/nimble/manifest/parser.py index b3861f8..88fd568 100644 --- a/nimble/manifest/parser.py +++ b/nimble/manifest/parser.py @@ -24,6 +24,14 @@ class ManifestError(Exception): pass +@dataclass +class ConfigFieldSpec: + key: str + description: str + default: str | None = None + possible_values: list[str] | None = None + + @dataclass class ManifestSpec: name: str @@ -36,6 +44,7 @@ class ManifestSpec: author: str requires: list[str] = field(default_factory=list) class_name: str = "" + config_fields: list[ConfigFieldSpec] = field(default_factory=list) def _validate_manifest_skill_name(raw: object, source: str) -> str: @@ -87,6 +96,79 @@ def _parse_manifest_string_list( return raw +def _parse_config_fields(data: dict[str, Any], source: str) -> list[ConfigFieldSpec]: + raw = data.get("config_fields") + if raw is None: + return [] + if not isinstance(raw, list): + raise ManifestError( + f"manifest.yaml from {source} field 'config_fields' must be a list" + ) + result: list[ConfigFieldSpec] = [] + for i, entry in enumerate(raw): + if not isinstance(entry, dict): + raise ManifestError( + f"manifest.yaml from {source} config_fields[{i}] must be a mapping" + ) + for required in ("key", "description"): + if required not in entry: + raise ManifestError( + f"manifest.yaml from {source} config_fields[{i}]" + f" missing required field '{required}'" + ) + key = entry["key"] + if not isinstance(key, str) or not key.strip(): + raise ManifestError( + f"manifest.yaml from {source} config_fields[{i}]" + " field 'key' must be a non-empty string" + ) + description = entry["description"] + if not isinstance(description, str) or not description.strip(): + raise ManifestError( + f"manifest.yaml from {source} config_fields[{i}]" + " field 'description' must be a non-empty string" + ) + default: str | None = None + if "default" in entry: + raw_default = entry["default"] + if raw_default is None: + default = None + elif isinstance(raw_default, str): + default = raw_default + else: + raise ManifestError( + f"manifest.yaml from {source} config_fields[{i}]" + " field 'default' must be a string or null" + ) + raw_pv = entry.get("possible_values") + if raw_pv is None: + possible_values = None + elif not isinstance(raw_pv, list) or any( + not isinstance(v, str) for v in raw_pv + ): + raise ManifestError( + f"manifest.yaml from {source} config_fields[{i}]" + " 'possible_values' must be a list of strings" + ) + else: + possible_values = raw_pv + if default is not None and possible_values is not None: + if default not in possible_values: + raise ManifestError( + f"manifest.yaml from {source} config_fields[{i}]" + " field 'default' must be one of 'possible_values'" + ) + result.append( + ConfigFieldSpec( + key=key, + description=description, + default=default, + possible_values=possible_values, + ) + ) + return result + + @dataclass class AiConfig: provider: str @@ -362,6 +444,7 @@ def parse_manifest_yaml(content: str, source: str = "") -> ManifestSpec: ) skill_name = _validate_manifest_skill_name(data["name"], source) + config_fields = _parse_config_fields(data, source) return ManifestSpec( name=skill_name, @@ -374,6 +457,7 @@ def parse_manifest_yaml(content: str, source: str = "") -> ManifestSpec: author=str(data["author"]), requires=_parse_manifest_string_list(data, "requires", source), class_name=str(data.get("class_name", "")), + config_fields=config_fields, ) diff --git a/tests/unit/manifest/test_parser.py b/tests/unit/manifest/test_parser.py index bf7d6de..de3d5f1 100644 --- a/tests/unit/manifest/test_parser.py +++ b/tests/unit/manifest/test_parser.py @@ -12,6 +12,7 @@ from nimble.manifest.parser import ( AiConfig, ConfigError, + ConfigFieldSpec, ManifestError, ManifestSpec, NimbleConfig, @@ -647,3 +648,152 @@ def test_remove_skill_entry_from_config_skills_not_list_raises( cfg.write_text("skills: {}\n", encoding="utf-8") with pytest.raises(ConfigError, match="Rollback failed"): remove_skill_entry_from_config(cfg, "x") + + +# --------------------------------------------------------------------------- +# config_fields tests (AC 1–5) +# --------------------------------------------------------------------------- + +_VALID_MANIFEST_WITH_CONFIG_FIELDS = ( + _VALID_MANIFEST_YAML + + "config_fields:\n" + " - key: target_language\n" + " description: Target language code\n" + " default: en\n" + " possible_values:\n" + " - en\n" + " - es\n" + " - fr\n" +) + + +def test_parse_manifest_config_fields_populated() -> None: + spec = parse_manifest_yaml(_VALID_MANIFEST_WITH_CONFIG_FIELDS) + assert len(spec.config_fields) == 1 + cf = spec.config_fields[0] + assert isinstance(cf, ConfigFieldSpec) + assert cf.key == "target_language" + assert cf.description == "Target language code" + assert cf.default == "en" + + +def test_parse_manifest_config_fields_possible_values_parsed() -> None: + spec = parse_manifest_yaml(_VALID_MANIFEST_WITH_CONFIG_FIELDS) + assert spec.config_fields[0].possible_values == ["en", "es", "fr"] + + +def test_parse_manifest_config_fields_no_possible_values_is_none() -> None: + content = ( + _VALID_MANIFEST_YAML + + "config_fields:\n" + " - key: output_format\n" + " description: Output format\n" + ) + spec = parse_manifest_yaml(content) + assert spec.config_fields[0].possible_values is None + + +def test_parse_manifest_no_config_fields_defaults_to_empty() -> None: + spec = parse_manifest_yaml(_VALID_MANIFEST_YAML) + assert spec.config_fields == [] + + +def test_parse_manifest_config_fields_missing_key_raises() -> None: + content = ( + _VALID_MANIFEST_YAML + + "config_fields:\n" + " - description: No key field here\n" + ) + with pytest.raises(ManifestError, match="missing required field 'key'"): + parse_manifest_yaml(content) + + +def test_parse_manifest_config_fields_missing_description_raises() -> None: + content = ( + _VALID_MANIFEST_YAML + + "config_fields:\n" + " - key: some_key\n" + ) + with pytest.raises(ManifestError, match="missing required field 'description'"): + parse_manifest_yaml(content) + + +def test_parse_manifest_config_fields_not_list_raises() -> None: + content = _VALID_MANIFEST_YAML + "config_fields: bad\n" + with pytest.raises(ManifestError, match="field 'config_fields' must be a list"): + parse_manifest_yaml(content) + + +def test_parse_manifest_config_fields_entry_not_mapping_raises() -> None: + content = _VALID_MANIFEST_YAML + "config_fields:\n - hello\n" + with pytest.raises(ManifestError, match="must be a mapping"): + parse_manifest_yaml(content) + + +def test_parse_manifest_config_fields_default_non_string_raises() -> None: + content = ( + _VALID_MANIFEST_YAML + + "config_fields:\n" + " - key: target_language\n" + " description: Target language code\n" + " default: 42\n" + ) + with pytest.raises( + ManifestError, match="field 'default' must be a string or null" + ): + parse_manifest_yaml(content) + + +def test_parse_manifest_config_fields_possible_values_items_must_be_strings() -> None: + content = ( + _VALID_MANIFEST_YAML + + "config_fields:\n" + " - key: target_language\n" + " description: Target language code\n" + " possible_values:\n" + " - en\n" + " - 2\n" + ) + with pytest.raises(ManifestError, match="'possible_values' must be a list of strings"): + parse_manifest_yaml(content) + + +def test_parse_manifest_config_fields_key_must_be_non_empty_string() -> None: + content = ( + _VALID_MANIFEST_YAML + + "config_fields:\n" + " - key: \" \"\n" + " description: Target language code\n" + ) + with pytest.raises(ManifestError, match="field 'key' must be a non-empty string"): + parse_manifest_yaml(content) + + +def test_parse_manifest_config_fields_description_must_be_non_empty_string() -> None: + content = ( + _VALID_MANIFEST_YAML + + "config_fields:\n" + " - key: target_language\n" + " description: \"\"\n" + ) + with pytest.raises( + ManifestError, match="field 'description' must be a non-empty string" + ): + parse_manifest_yaml(content) + + +def test_parse_manifest_config_fields_default_must_be_in_possible_values() -> None: + content = ( + _VALID_MANIFEST_YAML + + "config_fields:\n" + " - key: target_language\n" + " description: Target language code\n" + " default: de\n" + " possible_values:\n" + " - en\n" + " - es\n" + ) + with pytest.raises( + ManifestError, match="field 'default' must be one of 'possible_values'" + ): + parse_manifest_yaml(content) From e6af40d263a3995128baec73c91a65ce30c66a94 Mon Sep 17 00:00:00 2001 From: Bernard van der Esch Date: Tue, 5 May 2026 12:15:52 +0200 Subject: [PATCH 2/6] refactor: split _parse_config_fields into focused helpers Eliminates nested ifology by extracting _require_non_empty_str, _parse_config_field_default, _parse_config_field_possible_values, and _parse_config_field_entry. Same validation behavior, flat structure. Co-Authored-By: Claude Sonnet 4.6 --- .../spec-refactor-parse-config-fields.md | 21 +++ nimble/manifest/parser.py | 127 +++++++++--------- 2 files changed, 85 insertions(+), 63 deletions(-) create mode 100644 docs/bmad_output/implementation-artifacts/spec-refactor-parse-config-fields.md diff --git a/docs/bmad_output/implementation-artifacts/spec-refactor-parse-config-fields.md b/docs/bmad_output/implementation-artifacts/spec-refactor-parse-config-fields.md new file mode 100644 index 0000000..731f993 --- /dev/null +++ b/docs/bmad_output/implementation-artifacts/spec-refactor-parse-config-fields.md @@ -0,0 +1,21 @@ +--- +title: 'Refactor _parse_config_fields — eliminate ifology' +type: 'refactor' +created: '2026-05-05' +status: 'done' +route: 'one-shot' +--- + +## Intent + +**Problem:** `_parse_config_fields` in `nimble/manifest/parser.py` was a 70-line monolith with deep nesting and interleaved validation/extraction logic, making it hard to read and extend. + +**Approach:** Extract four focused helpers (`_require_non_empty_str`, `_parse_config_field_default`, `_parse_config_field_possible_values`, `_parse_config_field_entry`), reducing `_parse_config_fields` to a 6-line orchestrator with no nesting. + +## Suggested Review Order + +- [`parser.py:99`](../../nimble/manifest/parser.py) — `_require_non_empty_str`: validates and returns a required non-empty string field +- [`parser.py:114`](../../nimble/manifest/parser.py) — `_parse_config_field_default`: extracts optional `default` (str or null) +- [`parser.py:126`](../../nimble/manifest/parser.py) — `_parse_config_field_possible_values`: validates and returns optional string list +- [`parser.py:140`](../../nimble/manifest/parser.py) — `_parse_config_field_entry`: composes the above into one `ConfigFieldSpec` +- [`parser.py:157`](../../nimble/manifest/parser.py) — `_parse_config_fields`: now a flat list-comprehension over `_parse_config_field_entry` diff --git a/nimble/manifest/parser.py b/nimble/manifest/parser.py index 88fd568..f64c3d2 100644 --- a/nimble/manifest/parser.py +++ b/nimble/manifest/parser.py @@ -96,6 +96,69 @@ def _parse_manifest_string_list( return raw +def _require_non_empty_str(entry: dict, field_name: str, i: int, source: str) -> str: + if field_name not in entry: + raise ManifestError( + f"manifest.yaml from {source} config_fields[{i}]" + f" missing required field '{field_name}'" + ) + value = entry[field_name] + if not isinstance(value, str) or not value.strip(): + raise ManifestError( + f"manifest.yaml from {source} config_fields[{i}]" + f" field '{field_name}' must be a non-empty string" + ) + return value + + +def _parse_config_field_default(entry: dict, i: int, source: str) -> str | None: + if "default" not in entry: + return None + raw = entry["default"] + if raw is None or isinstance(raw, str): + return raw + raise ManifestError( + f"manifest.yaml from {source} config_fields[{i}]" + " field 'default' must be a string or null" + ) + + +def _parse_config_field_possible_values( + entry: dict, i: int, source: str +) -> list[str] | None: + raw = entry.get("possible_values") + if raw is None: + return None + if not isinstance(raw, list) or any(not isinstance(v, str) for v in raw): + raise ManifestError( + f"manifest.yaml from {source} config_fields[{i}]" + " 'possible_values' must be a list of strings" + ) + return raw + + +def _parse_config_field_entry(entry: Any, i: int, source: str) -> ConfigFieldSpec: + if not isinstance(entry, dict): + raise ManifestError( + f"manifest.yaml from {source} config_fields[{i}] must be a mapping" + ) + key = _require_non_empty_str(entry, "key", i, source) + description = _require_non_empty_str(entry, "description", i, source) + default = _parse_config_field_default(entry, i, source) + possible_values = _parse_config_field_possible_values(entry, i, source) + if default is not None and possible_values is not None and default not in possible_values: + raise ManifestError( + f"manifest.yaml from {source} config_fields[{i}]" + " field 'default' must be one of 'possible_values'" + ) + return ConfigFieldSpec( + key=key, + description=description, + default=default, + possible_values=possible_values, + ) + + def _parse_config_fields(data: dict[str, Any], source: str) -> list[ConfigFieldSpec]: raw = data.get("config_fields") if raw is None: @@ -104,69 +167,7 @@ def _parse_config_fields(data: dict[str, Any], source: str) -> list[ConfigFieldS raise ManifestError( f"manifest.yaml from {source} field 'config_fields' must be a list" ) - result: list[ConfigFieldSpec] = [] - for i, entry in enumerate(raw): - if not isinstance(entry, dict): - raise ManifestError( - f"manifest.yaml from {source} config_fields[{i}] must be a mapping" - ) - for required in ("key", "description"): - if required not in entry: - raise ManifestError( - f"manifest.yaml from {source} config_fields[{i}]" - f" missing required field '{required}'" - ) - key = entry["key"] - if not isinstance(key, str) or not key.strip(): - raise ManifestError( - f"manifest.yaml from {source} config_fields[{i}]" - " field 'key' must be a non-empty string" - ) - description = entry["description"] - if not isinstance(description, str) or not description.strip(): - raise ManifestError( - f"manifest.yaml from {source} config_fields[{i}]" - " field 'description' must be a non-empty string" - ) - default: str | None = None - if "default" in entry: - raw_default = entry["default"] - if raw_default is None: - default = None - elif isinstance(raw_default, str): - default = raw_default - else: - raise ManifestError( - f"manifest.yaml from {source} config_fields[{i}]" - " field 'default' must be a string or null" - ) - raw_pv = entry.get("possible_values") - if raw_pv is None: - possible_values = None - elif not isinstance(raw_pv, list) or any( - not isinstance(v, str) for v in raw_pv - ): - raise ManifestError( - f"manifest.yaml from {source} config_fields[{i}]" - " 'possible_values' must be a list of strings" - ) - else: - possible_values = raw_pv - if default is not None and possible_values is not None: - if default not in possible_values: - raise ManifestError( - f"manifest.yaml from {source} config_fields[{i}]" - " field 'default' must be one of 'possible_values'" - ) - result.append( - ConfigFieldSpec( - key=key, - description=description, - default=default, - possible_values=possible_values, - ) - ) - return result + return [_parse_config_field_entry(entry, i, source) for i, entry in enumerate(raw)] @dataclass From 7c8fe6dca06d6c63726f4a6c19a236343b6cbbeb Mon Sep 17 00:00:00 2001 From: Bernard van der Esch Date: Tue, 5 May 2026 12:22:29 +0200 Subject: [PATCH 3/6] fix: resolve flake8 E501 and mypy type-arg errors in parser helpers Add dict[str, Any] type args and wrap long signatures/conditions. Co-Authored-By: Claude Sonnet 4.6 --- nimble/manifest/parser.py | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/nimble/manifest/parser.py b/nimble/manifest/parser.py index f64c3d2..34b7bab 100644 --- a/nimble/manifest/parser.py +++ b/nimble/manifest/parser.py @@ -96,7 +96,9 @@ def _parse_manifest_string_list( return raw -def _require_non_empty_str(entry: dict, field_name: str, i: int, source: str) -> str: +def _require_non_empty_str( + entry: dict[str, Any], field_name: str, i: int, source: str +) -> str: if field_name not in entry: raise ManifestError( f"manifest.yaml from {source} config_fields[{i}]" @@ -111,7 +113,9 @@ def _require_non_empty_str(entry: dict, field_name: str, i: int, source: str) -> return value -def _parse_config_field_default(entry: dict, i: int, source: str) -> str | None: +def _parse_config_field_default( + entry: dict[str, Any], i: int, source: str +) -> str | None: if "default" not in entry: return None raw = entry["default"] @@ -124,7 +128,7 @@ def _parse_config_field_default(entry: dict, i: int, source: str) -> str | None: def _parse_config_field_possible_values( - entry: dict, i: int, source: str + entry: dict[str, Any], i: int, source: str ) -> list[str] | None: raw = entry.get("possible_values") if raw is None: @@ -146,7 +150,11 @@ def _parse_config_field_entry(entry: Any, i: int, source: str) -> ConfigFieldSpe description = _require_non_empty_str(entry, "description", i, source) default = _parse_config_field_default(entry, i, source) possible_values = _parse_config_field_possible_values(entry, i, source) - if default is not None and possible_values is not None and default not in possible_values: + if ( + default is not None + and possible_values is not None + and default not in possible_values + ): raise ManifestError( f"manifest.yaml from {source} config_fields[{i}]" " field 'default' must be one of 'possible_values'" From f6429c42a027f5357b414d2081c36a65b932b9f4 Mon Sep 17 00:00:00 2001 From: Bernard van der Esch Date: Tue, 5 May 2026 12:30:26 +0200 Subject: [PATCH 4/6] fix: fix remaining flake8 errors across the project - clipboard.py: split long subprocess command list across lines (E501) - test_parser.py: extract long match string to variable (E501) - translate/skill.py: add missing newline at EOF (W292) - translate/test_translate.py: remove unused pytest import (F401), add missing newline at EOF (W292) Co-Authored-By: Claude Sonnet 4.6 --- nimble/tools/clipboard.py | 5 ++++- tests/unit/manifest/test_parser.py | 3 ++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/nimble/tools/clipboard.py b/nimble/tools/clipboard.py index af46800..7d31d53 100644 --- a/nimble/tools/clipboard.py +++ b/nimble/tools/clipboard.py @@ -70,7 +70,10 @@ def set(self, text: str) -> None: if is_windows(): try: subprocess.run( - ["powershell", "-NoProfile", "-Command", "Set-Clipboard", "-Value", text], + [ + "powershell", "-NoProfile", "-Command", + "Set-Clipboard", "-Value", text, + ], capture_output=True, text=True, timeout=1.0, diff --git a/tests/unit/manifest/test_parser.py b/tests/unit/manifest/test_parser.py index de3d5f1..f9d5921 100644 --- a/tests/unit/manifest/test_parser.py +++ b/tests/unit/manifest/test_parser.py @@ -754,7 +754,8 @@ def test_parse_manifest_config_fields_possible_values_items_must_be_strings() -> " - en\n" " - 2\n" ) - with pytest.raises(ManifestError, match="'possible_values' must be a list of strings"): + match = "'possible_values' must be a list of strings" + with pytest.raises(ManifestError, match=match): parse_manifest_yaml(content) From 2a376555f77cc7aaa5844040b5a56152a02e04c4 Mon Sep 17 00:00:00 2001 From: Bernard van der Esch Date: Tue, 5 May 2026 12:45:29 +0200 Subject: [PATCH 5/6] fix(tests): rewrite clipboard tests to match subprocess-based implementation The implementation was migrated from plyer to subprocess (xclip/pbpaste/ powershell), but the tests still patched sys.modules["plyer"]. Updated all 10 tests to patch subprocess.run/Popen and platform guards instead. Co-Authored-By: Claude Sonnet 4.6 --- tests/unit/tools/test_clipboard.py | 81 ++++++++++++++++-------------- 1 file changed, 42 insertions(+), 39 deletions(-) diff --git a/tests/unit/tools/test_clipboard.py b/tests/unit/tools/test_clipboard.py index fc5d985..e19cf3a 100644 --- a/tests/unit/tools/test_clipboard.py +++ b/tests/unit/tools/test_clipboard.py @@ -5,82 +5,85 @@ from nimble.tools.clipboard import ClipboardTool +def _completed(stdout: str = "", returncode: int = 0) -> MagicMock: + return MagicMock(stdout=stdout, returncode=returncode) + + def test_get_returns_clipboard_text() -> None: tool = ClipboardTool() - mock_clipboard = MagicMock() - mock_clipboard.paste.return_value = "hello" - with patch.dict("sys.modules", {"plyer": MagicMock(clipboard=mock_clipboard)}): + with patch("nimble.tools.clipboard.is_linux", return_value=True), \ + patch("nimble.tools.clipboard.subprocess.run", return_value=_completed("hello")): assert tool.get() == "hello" def test_get_returns_empty_string_when_clipboard_empty() -> None: tool = ClipboardTool() - mock_clipboard = MagicMock() - mock_clipboard.paste.return_value = None - with patch.dict("sys.modules", {"plyer": MagicMock(clipboard=mock_clipboard)}): + with patch("nimble.tools.clipboard.is_linux", return_value=True), \ + patch("nimble.tools.clipboard.subprocess.run", return_value=_completed("")): assert tool.get() == "" -def test_get_returns_string_for_non_string_clipboard_value() -> None: +def test_get_returns_empty_string_on_nonzero_exit() -> None: tool = ClipboardTool() - mock_clipboard = MagicMock() - mock_clipboard.paste.return_value = 123 - with patch.dict("sys.modules", {"plyer": MagicMock(clipboard=mock_clipboard)}): - assert tool.get() == "123" + with patch("nimble.tools.clipboard.is_linux", return_value=True), \ + patch("nimble.tools.clipboard.subprocess.run", return_value=_completed("out", 1)): + assert tool.get() == "" -def test_set_calls_plyer_copy() -> None: +def test_set_calls_xclip_on_linux() -> None: tool = ClipboardTool() - mock_clipboard = MagicMock() - with patch.dict("sys.modules", {"plyer": MagicMock(clipboard=mock_clipboard)}): - tool.set("out") - mock_clipboard.copy.assert_called_once_with("out") + mock_proc = MagicMock() + with patch("nimble.tools.clipboard.is_linux", return_value=True), \ + patch("nimble.tools.clipboard.subprocess.Popen", return_value=mock_proc) as mock_popen: + tool.set("hello") + mock_popen.assert_called_once() + assert mock_popen.call_args.args[0] == ["xclip", "-selection", "clipboard"] -def test_get_handles_plyer_failure_gracefully() -> None: +def test_get_handles_subprocess_failure_gracefully() -> None: tool = ClipboardTool() - mock_clipboard = MagicMock() - mock_clipboard.paste.side_effect = RuntimeError("fail") - with patch.dict("sys.modules", {"plyer": MagicMock(clipboard=mock_clipboard)}): + with patch("nimble.tools.clipboard.is_linux", return_value=True), \ + patch("nimble.tools.clipboard.subprocess.run", side_effect=RuntimeError("fail")): assert tool.get() == "" -def test_set_handles_plyer_failure_gracefully() -> None: +def test_set_handles_subprocess_failure_gracefully() -> None: tool = ClipboardTool() - mock_clipboard = MagicMock() - mock_clipboard.copy.side_effect = RuntimeError("fail") - with patch.dict("sys.modules", {"plyer": MagicMock(clipboard=mock_clipboard)}): + with patch("nimble.tools.clipboard.is_linux", return_value=True), \ + patch("nimble.tools.clipboard.subprocess.Popen", side_effect=RuntimeError("fail")): tool.set("text") # must not raise -def test_get_handles_plyer_import_failure() -> None: +def test_get_returns_empty_when_no_platform_matches() -> None: tool = ClipboardTool() - with patch.dict("sys.modules", {"plyer": None}): + with patch("nimble.tools.clipboard.is_linux", return_value=False), \ + patch("nimble.tools.clipboard.is_windows", return_value=False), \ + patch("nimble.tools.clipboard.is_mac", return_value=False): assert tool.get() == "" -def test_set_handles_plyer_import_failure() -> None: +def test_set_is_noop_when_no_platform_matches() -> None: tool = ClipboardTool() - with patch.dict("sys.modules", {"plyer": None}): - tool.set("text") + with patch("nimble.tools.clipboard.is_linux", return_value=False), \ + patch("nimble.tools.clipboard.is_windows", return_value=False), \ + patch("nimble.tools.clipboard.is_mac", return_value=False): + tool.set("text") # must not raise def test_get_logs_warning_on_failure() -> None: tool = ClipboardTool() - mock_clipboard = MagicMock() - mock_clipboard.paste.side_effect = RuntimeError("fail") - with patch.dict("sys.modules", {"plyer": MagicMock(clipboard=mock_clipboard)}): - with patch("nimble.tools.clipboard.logger.warning") as mock_warning: - result = tool.get() + with patch("nimble.tools.clipboard.is_linux", return_value=True), \ + patch("nimble.tools.clipboard.subprocess.run", side_effect=RuntimeError("fail")), \ + patch("nimble.tools.clipboard.logger.warning") as mock_warning: + result = tool.get() assert result == "" mock_warning.assert_called_once_with("clipboard.get failed", exc_info=True) def test_set_logs_warning_on_failure() -> None: tool = ClipboardTool() - mock_clipboard = MagicMock() - mock_clipboard.copy.side_effect = RuntimeError("fail") - with patch.dict("sys.modules", {"plyer": MagicMock(clipboard=mock_clipboard)}): - with patch("nimble.tools.clipboard.logger.warning") as mock_warning: - tool.set("text") + with patch("nimble.tools.clipboard.is_linux", return_value=True), \ + patch("nimble.tools.clipboard.subprocess.Popen", side_effect=RuntimeError("fail")), \ + patch("nimble.tools.clipboard.logger.warning") as mock_warning: + tool.set("text") mock_warning.assert_called_once_with("clipboard.set failed", exc_info=True) From e1d3875749ae9d7acd156a96b7257ae70fe6c5f3 Mon Sep 17 00:00:00 2001 From: Bernard van der Esch Date: Tue, 5 May 2026 12:49:41 +0200 Subject: [PATCH 6/6] fix(tests): extract patch-target constants to fix E501 in clipboard tests Long patch target strings pushed lines past 88 chars. Extracted as module-level constants (_RUN, _POPEN, _IS_LINUX, etc.) and switched to parenthesized with-statement syntax. Co-Authored-By: Claude Sonnet 4.6 --- tests/unit/tools/test_clipboard.py | 76 ++++++++++++++++++++---------- 1 file changed, 52 insertions(+), 24 deletions(-) diff --git a/tests/unit/tools/test_clipboard.py b/tests/unit/tools/test_clipboard.py index e19cf3a..9cc90f4 100644 --- a/tests/unit/tools/test_clipboard.py +++ b/tests/unit/tools/test_clipboard.py @@ -4,6 +4,14 @@ from nimble.tools.clipboard import ClipboardTool +_MOD = "nimble.tools.clipboard" +_IS_LINUX = f"{_MOD}.is_linux" +_IS_WINDOWS = f"{_MOD}.is_windows" +_IS_MAC = f"{_MOD}.is_mac" +_RUN = f"{_MOD}.subprocess.run" +_POPEN = f"{_MOD}.subprocess.Popen" +_WARN = f"{_MOD}.logger.warning" + def _completed(stdout: str = "", returncode: int = 0) -> MagicMock: return MagicMock(stdout=stdout, returncode=returncode) @@ -11,30 +19,38 @@ def _completed(stdout: str = "", returncode: int = 0) -> MagicMock: def test_get_returns_clipboard_text() -> None: tool = ClipboardTool() - with patch("nimble.tools.clipboard.is_linux", return_value=True), \ - patch("nimble.tools.clipboard.subprocess.run", return_value=_completed("hello")): + with ( + patch(_IS_LINUX, return_value=True), + patch(_RUN, return_value=_completed("hello")), + ): assert tool.get() == "hello" def test_get_returns_empty_string_when_clipboard_empty() -> None: tool = ClipboardTool() - with patch("nimble.tools.clipboard.is_linux", return_value=True), \ - patch("nimble.tools.clipboard.subprocess.run", return_value=_completed("")): + with ( + patch(_IS_LINUX, return_value=True), + patch(_RUN, return_value=_completed("")), + ): assert tool.get() == "" def test_get_returns_empty_string_on_nonzero_exit() -> None: tool = ClipboardTool() - with patch("nimble.tools.clipboard.is_linux", return_value=True), \ - patch("nimble.tools.clipboard.subprocess.run", return_value=_completed("out", 1)): + with ( + patch(_IS_LINUX, return_value=True), + patch(_RUN, return_value=_completed("out", 1)), + ): assert tool.get() == "" def test_set_calls_xclip_on_linux() -> None: tool = ClipboardTool() mock_proc = MagicMock() - with patch("nimble.tools.clipboard.is_linux", return_value=True), \ - patch("nimble.tools.clipboard.subprocess.Popen", return_value=mock_proc) as mock_popen: + with ( + patch(_IS_LINUX, return_value=True), + patch(_POPEN, return_value=mock_proc) as mock_popen, + ): tool.set("hello") mock_popen.assert_called_once() assert mock_popen.call_args.args[0] == ["xclip", "-selection", "clipboard"] @@ -42,39 +58,49 @@ def test_set_calls_xclip_on_linux() -> None: def test_get_handles_subprocess_failure_gracefully() -> None: tool = ClipboardTool() - with patch("nimble.tools.clipboard.is_linux", return_value=True), \ - patch("nimble.tools.clipboard.subprocess.run", side_effect=RuntimeError("fail")): + with ( + patch(_IS_LINUX, return_value=True), + patch(_RUN, side_effect=RuntimeError("fail")), + ): assert tool.get() == "" def test_set_handles_subprocess_failure_gracefully() -> None: tool = ClipboardTool() - with patch("nimble.tools.clipboard.is_linux", return_value=True), \ - patch("nimble.tools.clipboard.subprocess.Popen", side_effect=RuntimeError("fail")): + with ( + patch(_IS_LINUX, return_value=True), + patch(_POPEN, side_effect=RuntimeError("fail")), + ): tool.set("text") # must not raise def test_get_returns_empty_when_no_platform_matches() -> None: tool = ClipboardTool() - with patch("nimble.tools.clipboard.is_linux", return_value=False), \ - patch("nimble.tools.clipboard.is_windows", return_value=False), \ - patch("nimble.tools.clipboard.is_mac", return_value=False): + with ( + patch(_IS_LINUX, return_value=False), + patch(_IS_WINDOWS, return_value=False), + patch(_IS_MAC, return_value=False), + ): assert tool.get() == "" def test_set_is_noop_when_no_platform_matches() -> None: tool = ClipboardTool() - with patch("nimble.tools.clipboard.is_linux", return_value=False), \ - patch("nimble.tools.clipboard.is_windows", return_value=False), \ - patch("nimble.tools.clipboard.is_mac", return_value=False): + with ( + patch(_IS_LINUX, return_value=False), + patch(_IS_WINDOWS, return_value=False), + patch(_IS_MAC, return_value=False), + ): tool.set("text") # must not raise def test_get_logs_warning_on_failure() -> None: tool = ClipboardTool() - with patch("nimble.tools.clipboard.is_linux", return_value=True), \ - patch("nimble.tools.clipboard.subprocess.run", side_effect=RuntimeError("fail")), \ - patch("nimble.tools.clipboard.logger.warning") as mock_warning: + with ( + patch(_IS_LINUX, return_value=True), + patch(_RUN, side_effect=RuntimeError("fail")), + patch(_WARN) as mock_warning, + ): result = tool.get() assert result == "" mock_warning.assert_called_once_with("clipboard.get failed", exc_info=True) @@ -82,8 +108,10 @@ def test_get_logs_warning_on_failure() -> None: def test_set_logs_warning_on_failure() -> None: tool = ClipboardTool() - with patch("nimble.tools.clipboard.is_linux", return_value=True), \ - patch("nimble.tools.clipboard.subprocess.Popen", side_effect=RuntimeError("fail")), \ - patch("nimble.tools.clipboard.logger.warning") as mock_warning: + with ( + patch(_IS_LINUX, return_value=True), + patch(_POPEN, side_effect=RuntimeError("fail")), + patch(_WARN) as mock_warning, + ): tool.set("text") mock_warning.assert_called_once_with("clipboard.set failed", exc_info=True)