From 4af2197c5970f576b66a95f77233d9902efe77d8 Mon Sep 17 00:00:00 2001 From: Jeroen Schweitzer Date: Wed, 18 Feb 2026 00:23:21 +0100 Subject: [PATCH] =?UTF-8?q?fix(ci):=20address=20PR=20review=20=E2=80=94=20?= =?UTF-8?q?filter=20comments,=20trim=20whitespace,=20warn=20on=20missing?= =?UTF-8?q?=20scripts?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address all review comments from Hoshe and Tyre on PR #27: - Remove 2>/dev/null from pre-pr-fixtures (critical: swallowed errors) - Remove dead _file_type function - Check 4: error on districts with no locations declared - Check 5: print advisory message when skipping - Check 8: cross-file line ID uniqueness (not just per-file) - Check 9: document D-034 asymmetric relationships in docstring - Document regex fallback rationale in _scan_knowledge - Add D-035 decision trace to schema descriptions - Use concrete protocol version in DEVOPS.md example - Amend D-035 with focused (9th mood) and greeting (14th situation) Co-Authored-By: Claude Opus 4.6 --- Makefile | 2 +- content/_schema/dialogue-pool.schema.json | 4 +- decisions/content.md | 1 + docs/DEVOPS.md | 2 +- tooling/validate-content | 50 ++++++++++++++--------- 5 files changed, 35 insertions(+), 24 deletions(-) diff --git a/Makefile b/Makefile index 6658e2e5f..781235427 100644 --- a/Makefile +++ b/Makefile @@ -149,7 +149,7 @@ pre-pr-validate: validate-content check-fact-ids pre-pr-fixtures: @echo "Checking fixture staleness..." - @cd server && cargo test --test gen_fixtures -- --ignored 2>/dev/null + @cd server && cargo test --test gen_fixtures -- --ignored @if git diff --quiet client/tests/fixtures/; then \ echo "--- Fixtures: UP TO DATE ---"; \ else \ diff --git a/content/_schema/dialogue-pool.schema.json b/content/_schema/dialogue-pool.schema.json index f34e466f2..0327ded73 100644 --- a/content/_schema/dialogue-pool.schema.json +++ b/content/_schema/dialogue-pool.schema.json @@ -71,7 +71,7 @@ }, "minItems": 1, "uniqueItems": true, - "description": "Situation contexts when this line can fire (D-035: list)" + "description": "Situation contexts when this line can fire (D-035: list). greeting added Sprint 8 for PC dialogue pools" }, "topic": { "type": "array", @@ -96,7 +96,7 @@ ] }, "uniqueItems": true, - "description": "Mood tags for selection weighting (D-035: list)" + "description": "Mood tags for selection weighting (D-035: list, v0.1 8 moods + focused added Sprint 8)" }, "tags": { "type": "array", diff --git a/decisions/content.md b/decisions/content.md index 522e57817..9b7d33561 100644 --- a/decisions/content.md +++ b/decisions/content.md @@ -82,6 +82,7 @@ How narrative, NPCs, and world content are created: content tiers, NPC generatio - **Cross-reference:** Dialogue architecture ([D-028](#d-028-dialogue-architecture--tagged-line-pools-with-four-relational-layers)) - **Raised by:** Gestalt (schema design, Round 1/2), Mellanie (authoring validation, Round 1/2). Converged across both agents in Round 2. - **Dissent:** None. Minor consolidations: Mellanie's 8 moods mapped to Gestalt's 8 (different names, same concepts). Mellanie's `crime` topic deliberately excluded (NPCs think of it as `cargo` or `money`). +- **Amendment (Sprint 8):** `focused` added as 9th mood (used in Kael dialogue at The Terminal and maintenance corridors). `greeting` added as 14th situation (used in PC dialogue pools for initial contact lines). Schema updated to match. ### D-036: Sova Transit District / Krenn System as v0.1 setting - **Date:** 2026-02-11 diff --git a/docs/DEVOPS.md b/docs/DEVOPS.md index 550fc97c9..9a400fdb0 100644 --- a/docs/DEVOPS.md +++ b/docs/DEVOPS.md @@ -112,7 +112,7 @@ If `pre-pr-fixtures` fails, your protocol changes require fixture regeneration: ```bash make fixtures git add client/tests/fixtures/ -git commit -m "chore(fixtures): regenerate for protocol vN" +git commit -m "chore(fixtures): regenerate for protocol v8" ``` The fixture staleness check is a **blocker** (exit 1) — stale fixtures cause false positive client tests. diff --git a/tooling/validate-content b/tooling/validate-content index 6cd62aac4..830ffd466 100755 --- a/tooling/validate-content +++ b/tooling/validate-content @@ -82,17 +82,6 @@ def _load_yaml(path: Path) -> dict | list | None: return None -def _file_type(path: Path) -> str | None: - """Determine content type from path (npc, dialogue, monologue, district, triangle).""" - rel = path.relative_to(CONTENT_DIR) - for part in reversed(rel.parts[:-1]): - if part in ("npcs", "dialogue", "monologue", "triangles", "locations"): - return part - if path.name == "district.yaml": - return "district" - return None - - def _district_dir(path: Path) -> Path | None: """Find the district directory containing this file.""" parts = path.resolve().parts @@ -162,7 +151,8 @@ class ContentIndex: continue data = _load_yaml(path) if not data or not isinstance(data, dict): - # Try line-by-line grep for fact_id definitions + # Stub catalogs (comment-only or flat key-value) don't parse + # as dicts. Fall back to regex extraction of fact_id lines. try: with open(path) as f: for line in f: @@ -289,7 +279,12 @@ class ContentIndex: continue district_key = str(district_dir) district_locs = self.locations.get(district_key, set()) - if district_locs and location not in district_locs: + if not district_locs: + print(f'XREF ERROR: dialogue location "{location}" references district with no locations declared') + print(f" In: {_rel(path)}") + print(f" District: {_rel(district_dir / 'district.yaml')}") + errors += 1 + elif location not in district_locs: print(f'XREF ERROR: dialogue location "{location}" not in district') print(f" In: {_rel(path)}") print(f" District locations: {', '.join(sorted(district_locs))}") @@ -299,7 +294,7 @@ class ContentIndex: def _check_5_fact_ids(self) -> int: """Check that all referenced fact_ids resolve to knowledge catalogs.""" if not self.fact_ids: - # Advisory mode: catalogs not populated yet + print(" Check 5 (fact_ids): SKIPPED — no canonical fact_ids in knowledge catalogs yet") return 0 errors = 0 @@ -414,31 +409,46 @@ class ContentIndex: return warnings def _check_8_dialogue_line_ids(self) -> int: - """Check that dialogue line IDs are unique within each file.""" + """Check that dialogue line IDs are unique across all dialogue files.""" errors = 0 + # Cross-file uniqueness: same line ID in two files is an error + global_seen: dict[str, Path] = {} for path, data in self.dialogue_files: lines = data.get("lines", []) if not isinstance(lines, list): continue - seen: dict[str, int] = {} + local_seen: dict[str, int] = {} for i, line in enumerate(lines, 1): if not isinstance(line, dict): continue line_id = line.get("id") if not line_id: continue - if line_id in seen: + # Per-file duplicate + if line_id in local_seen: print(f'XREF ERROR: duplicate dialogue line id "{line_id}"') print(f" In: {_rel(path)}") - print(f" First: line {seen[line_id]}") + print(f" First: line {local_seen[line_id]}") print(f" Duplicate: line {i}") errors += 1 else: - seen[line_id] = i + local_seen[line_id] = i + # Cross-file duplicate + if line_id in global_seen and global_seen[line_id] != path: + print(f'XREF ERROR: dialogue line id "{line_id}" used in multiple files') + print(f" First: {_rel(global_seen[line_id])}") + print(f" Also in: {_rel(path)}") + errors += 1 + elif line_id not in global_seen: + global_seen[line_id] = path return errors def _check_9_bidirectional_relationships(self) -> int: - """Check that NPC relationships have reciprocal entries.""" + """Check that NPC relationships have reciprocal entries. + + WARNING severity: asymmetric relationships are valid by design (D-034 + allows one-directional awareness). This check flags them for review, + not as errors.""" warnings = 0 # Build map: canonical_id -> set of relationship targets npc_rels: dict[str, set[str]] = {}