From 78b991b6ab9d36aeb6b16c8da52bfe64fefe0689 Mon Sep 17 00:00:00 2001 From: ak2k <19240940+ak2k@users.noreply.github.com> Date: Mon, 8 Jun 2026 16:46:55 -0400 Subject: [PATCH] fix(converter): render skill-rules.json from the staged dist, not in-tree dist/ write_hooks(dist) received the staging directory but called render_hook_rules() without forwarding it, so the hook rules were derived from the committed dist/ manifest (the previous release) instead of the build in progress. Harmless when the staged and committed manifests match, but a cross-release bump emits skill-rules.json keyed to the old persona set, and validate.py rejects the orphaned personas. Surfaced converting upstream v3.11.2, which drops 7 agents relative to v3.8.4 (kieran python/rails/typescript, dhh-rails, schema-drift, data-migration-expert; data-migrations-reviewer -> data-migration-reviewer). - Forward `dist` through render_hook_rules so rules track the built manifest. - Add a regression test asserting write_hooks reads the manifest from the dir it is given. - Prune the 7 removed/renamed personas from persona-keywords.yaml and add Pass-B keywords for the new ce-data-migration-reviewer. - Correct the validate.py failure message (referenced a non-existent DEFAULT_HOOK_RULES; the real causes are stale overrides or an unthreaded dist). --- converter/generate_wrappers.py | 2 +- converter/overrides/persona-keywords.yaml | 132 +++------------------- converter/validate.py | 8 +- tests/test_converter.py | 47 ++++++++ 4 files changed, 66 insertions(+), 123 deletions(-) diff --git a/converter/generate_wrappers.py b/converter/generate_wrappers.py index 645fe31..694829c 100644 --- a/converter/generate_wrappers.py +++ b/converter/generate_wrappers.py @@ -933,7 +933,7 @@ def write_hooks(dist: Path) -> list[Path]: script_path = hooks_dir / "auto_suggest.py" config_path.write_text(render_hook_config(), encoding="utf-8") - rules_path.write_text(render_hook_rules(), encoding="utf-8") + rules_path.write_text(render_hook_rules(dist), encoding="utf-8") if not _HOOK_SCRIPT_SOURCE_PATH.is_file(): raise FileNotFoundError( f"missing hook script source at {_HOOK_SCRIPT_SOURCE_PATH} — " diff --git a/converter/overrides/persona-keywords.yaml b/converter/overrides/persona-keywords.yaml index 09434b4..c2a1e55 100644 --- a/converter/overrides/persona-keywords.yaml +++ b/converter/overrides/persona-keywords.yaml @@ -210,41 +210,24 @@ ce-data-integrity-guardian: phrasing: The prompt looks database or migration-flavored. Consider `/ce-lite:ce-data-integrity-guardian` for migration safety, constraint validation, transaction boundaries, and data-privacy compliance. -ce-data-migration-expert: - _rationale: These terms are tightly scoped to database migration operations—backfills, - DDL changes, enum remapping, dual-write patterns—where silent data corruption - is the primary risk, making a specialist review distinctly valuable. - keywords: - - backfill - - data migration - - column rename - - enum conversion - - dual-write - - id mapping - - ALTER TABLE - - idempotent - - schema change - phrasing: Migration-flavored prompt detected. Consider `/ce-lite:ce-data-migration-expert` - for validating backfills, ID mappings, enum conversions, and schema changes against - production data reality. -ce-data-migrations-reviewer: - _rationale: These terms strongly signal database migration work where deployment-window - hazards — swapped enum mappings, missing backfills, unsafe column drops, and broken - dual-write transitions — require a specialist rather than a generic code reviewer. +ce-data-migration-reviewer: + _rationale: Migration diffs carry silent data-loss and deploy-window risk that general + review misses; the specialist checks schema drift, mapping correctness, backfill + completeness, dual-write hygiene, and rollback viability. keywords: + - schema drift + - deploy-window - backfill - - migration + - schema.rb + - db/migrate - dual-write - NOT NULL - enum mapping - - column drop - - schema change - - concurrent index - - data backfill - - rollback plan - phrasing: Schema-change / migration flavor. Consider `/ce-lite:ce-data-migrations-reviewer` - for a specialist review of rollback safety, dual-write windows, NOT NULL backfills, - and swapped enum mappings. + - structure.sql + - missing backfill + phrasing: Migration-flavored prompt detected. Consider `/ce-lite:ce-data-migration-reviewer` + for schema-drift detection, backfill safety, deploy-window breaks, enum/mapping + correctness, and rollback-plan review. ce-deployment-verification-agent: _rationale: Keywords anchor on the concrete artifacts this persona uniquely produces—backfill operations, rollback plans, Go/No-Go gates, data invariants, and SQL verification @@ -315,25 +298,6 @@ ce-design-lens-reviewer: phrasing: Design-planning doc detected. Consider `/ce-lite:ce-design-lens-reviewer` for dimensional review of IA, interaction states, user flows, and AI-slop risk before implementation begins. -ce-dhh-rails-reviewer: - _rationale: Keywords target Rails-specific architectural debates—service objects, - repository patterns, anemic models, JWT vs sessions, GraphQL ceremony, Hotwire - alternatives—that signal code drifting from Rails conventions and warrant the - DHH opinionated review persona. - keywords: - - service object - - repository pattern - - dependency injection - - anemic model - - GraphQL - - presenter - - service layer - - JWT - - Hotwire - - majestic monolith - phrasing: The prompt looks Rails-architecture-flavored. Consider `/ce-lite:ce-dhh-rails-reviewer` - for a DHH-perspective review that catches service objects, repository layers, - JWT-over-session, and other un-Rails abstractions. ce-feasibility-reviewer: _rationale: Keywords target planning-document vocabulary (brownfield, migration risk, dependency gap, rollback strategy) that is distinctive to feasibility review @@ -440,57 +404,6 @@ ce-julik-frontend-races-reviewer: phrasing: Async/lifecycle-flavored prompt. Consider `/ce-lite:ce-julik-frontend-races-reviewer` for race conditions, stale timers, and lifecycle cleanup in Turbo/Stimulus/async UI code. -ce-kieran-python-reviewer: - _rationale: Keywords target exact Python idioms, type-system constructs, and anti-patterns - Kieran hunts (missing annotations, exception swallowing, un-Pythonic structure), - minimizing false fires from generic prompts. - keywords: - - pythonic - - type hint - - type annotation - - TypedDict - - dataclass - - pydantic - - context manager - - bare except - - except pass - phrasing: 'Prompt looks Python-quality-flavored. Consider `/ce-lite:ce-kieran-python-reviewer` - for strict Pythonic review: type hints, structure, and exception-handling quality.' -ce-kieran-rails-reviewer: - _rationale: Keywords target Rails-exclusive vocabulary (Turbo, Hotwire, before_action, - strong parameters, ActiveRecord callbacks) that signal the user is working on - a Rails diff matching Kieran's specialist review scope. - keywords: - - turbo stream - - Hotwire - - service object - - controller action - - ActiveRecord - - before_action - - strong parameters - - rails callback - - stimulus controller - phrasing: Rails-flavored diff detected. Consider `/ce-lite:ce-kieran-rails-reviewer` - for a strict Rails review covering controller clarity, Turbo/Hotwire patterns, - service object extraction, and regression hunting. -ce-kieran-typescript-reviewer: - _rationale: Keywords target TypeScript-specific constructs (suppression directives, - cast patterns, union narrowing) that signal the user needs Kieran's high-bar type-safety - and structural-clarity review rather than a generic code reviewer. - keywords: - - TypeScript - - type safety - - as any - - '@ts-ignore' - - discriminated union - - type assertion - - nullable - - type narrowing - - ts-expect-error - - unknown as - phrasing: 'TypeScript-flavored prompt detected. Consider `/ce-lite:ce-kieran-typescript-reviewer` - for strict type-safety review: `any` usage, unsafe casts, nullable flows, and - discriminated-union exhaustiveness.' ce-learnings-researcher: _rationale: This persona specializes in searching docs/solutions/ via frontmatter metadata for past learnings, conventions, and prior decisions—preventing the team @@ -675,25 +588,6 @@ ce-repo-research-analyst: phrasing: 'Prompt looks codebase-research-flavored. Consider `/ce-lite:ce-repo-research-analyst` for systematic repo research: structure, conventions, patterns, templates, and onboarding insights.' -ce-schema-drift-detector: - _rationale: These keywords target Rails-specific ORM vocabulary and migration workflow - (schema.rb, db:migrate, migration DSL methods like add_column/create_table) that - uniquely identify schema drift scenarios rather than generic database or code - review tasks. - keywords: - - schema.rb - - schema drift - - db:migrate - - ActiveRecord - - add_column - - create_table - - schema version - - unrelated migration - - db/migrate - - migration timestamp - phrasing: Rails schema-drift signals detected. Consider `/ce-lite:ce-schema-drift-detector` - for detecting unrelated schema.rb changes by cross-referencing your PR's migrations - against schema.rb. ce-scope-guardian-reviewer: _rationale: Keywords target vocabulary exclusive to scope-and-complexity review (YAGNI, gold-plating, scope creep, premature abstraction) that strongly signals diff --git a/converter/validate.py b/converter/validate.py index 2dfa011..05b2cbc 100644 --- a/converter/validate.py +++ b/converter/validate.py @@ -148,9 +148,11 @@ def check_hooks_dir(dist: Path) -> None: if bad: fail( f"dist/hooks/skill-rules.json references personas not in the " - f"manifest: {bad}. Either upstream renamed/removed them, or " - f"the rules file was hand-edited without updating " - f"DEFAULT_HOOK_RULES in generate_wrappers.py." + f"manifest: {bad}. Either upstream renamed/removed them (prune " + f"the stale entries from converter/overrides/persona-keywords.yaml) " + f"or skill-rules.json was rendered against a different manifest " + f"than this dist's (generate_wrappers.write_hooks must pass its " + f"`dist` through to render_hook_rules)." ) diff --git a/tests/test_converter.py b/tests/test_converter.py index 58c55cd..4e30fd3 100644 --- a/tests/test_converter.py +++ b/tests/test_converter.py @@ -676,6 +676,53 @@ def test_render_hook_rules_covers_all_personas(): ) +def test_write_hooks_derives_rules_from_passed_dist_not_in_tree(tmp_path: Path): + """skill-rules.json must be built from the manifest of the dist being + written, not the in-tree dist/. + + Regression: the publish-dist workflow builds into a staging dir while the + committed dist/ still holds the *previous* release's manifest. When + write_hooks ignored its `dist` arg and let render_hook_rules fall back to + the in-tree default, a cross-release bump emitted skill-rules.json keyed + to the old persona set — orphan personas that validate.py then rejected. + """ + from generate_wrappers import write_hooks + + sentinel = "ce-staging-only-sentinel-reviewer" + staged = tmp_path / "dist" + (staged / "references" / "agent-prompts").mkdir(parents=True) + (staged / "references" / "agent-prompts" / "manifest.json").write_text( + _json.dumps( + { + "schema_version": 1, + "upstream_tag": "test", + "agent_count": 1, + "agents": [ + { + "name": sentinel, + "description": "Use when reviewing staging fixtures.", + "model": "inherit", + "tools": None, + "prompt_path": f"references/agent-prompts/{sentinel}.md", + "upstream_source": f"agents/{sentinel}.md", + } + ], + } + ), + encoding="utf-8", + ) + + write_hooks(staged) + + rules = _json.loads( + (staged / "hooks" / "skill-rules.json").read_text(encoding="utf-8") + )["rules"] + rule_personas = {r["persona"] for r in rules} + assert rule_personas == {sentinel}, ( + f"skill-rules.json should reflect the staged manifest only; got {rule_personas}" + ) + + # -------- hook script live runtime tests (Phase B.7) -------- # # render_hook_script tests above prove we generate the right SOURCE.