From 00df1023642eab128d5d693a5ab2734114c05625 Mon Sep 17 00:00:00 2001 From: Jack McIntyre Date: Mon, 29 Jun 2026 16:32:09 +1000 Subject: [PATCH] feat(01KW5W17): writeRetroProposal tool description matches the schema's accepted proposal types (#404) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The writeRetroProposal tool description (register.ts) and module JSDoc said the proposal schema accepts seven types and listed only the original seven, but RETRO_PROPOSAL_TYPES has grown to thirteen (persona-append, promote-lesson-to-skill, build-story, lesson-consolidation, lesson-retirement, shared-skill-promotion, etc.). The contract was being misread as silently dropping newer types. This exports a WRITE_RETRO_PROPOSAL_DESCRIPTION constant built by interpolating RETRO_PROPOSAL_TYPES (both the count and the enumerated list) so the description can never drift from the schema again, wires register.ts to use it, refreshes the stale JSDoc/comments in write-retro-proposal.ts and retro-proposal.ts, and adds tests asserting the description enumerates exactly RETRO_PROPOSAL_TYPES (AC1) and that every type round-trips through parseRetroProposalFile (AC2). No runtime acceptance behaviour changed — the schema already accepted all thirteen types; this fixes stale documentation. Build green, dist rebuilt, full suite (3861 tests) green. --- plugins/flow/mcp-server/dist/index.js | 18 +- .../mcp-server/src/schemas/retro-proposal.ts | 11 +- .../__tests__/write-retro-proposal.test.ts | 218 +++++++++++++++++- plugins/flow/mcp-server/src/tools/register.ts | 15 +- .../src/tools/write-retro-proposal.ts | 31 ++- 5 files changed, 273 insertions(+), 20 deletions(-) diff --git a/plugins/flow/mcp-server/dist/index.js b/plugins/flow/mcp-server/dist/index.js index 0e612dcc..34f84b3d 100644 --- a/plugins/flow/mcp-server/dist/index.js +++ b/plugins/flow/mcp-server/dist/index.js @@ -39310,6 +39310,21 @@ var SharedSkillPromotionProposalSchema = ProposalBase.extend({ proposed_skill_path: PathInsideRepoSchema, skill_description: external_exports.string().min(1) }).strict(); +var RETRO_PROPOSAL_TYPES = [ + "rule", + "rule-retirement", + "skill-create", + "skill-revise", + "skill-supersede", + "skill-retire", + "team-change", + "persona-append", + "promote-lesson-to-skill", + "build-story", + "lesson-consolidation", + "lesson-retirement", + "shared-skill-promotion" +]; var RetroProposalSchema = external_exports.discriminatedUnion("type", [ RuleProposalSchema, RuleRetirementProposalSchema, @@ -39440,6 +39455,7 @@ function ulid3(seedTime, prng) { } // src/tools/write-retro-proposal.ts +var WRITE_RETRO_PROPOSAL_DESCRIPTION = `Write a single immutable retro-proposal markdown file under /.flow/retro-proposals/.md. The file carries a YAML frontmatter block (validated via RetroProposalFileSchema; source of truth for Epic 6b apply-time re-validation) plus a rendered Markdown body with one H2 per proposal. Refuses collisions with RetroProposalAlreadyExistsError (proposals are immutable). Refuses malformed payloads with MalformedRetroProposalError \u2014 closed discriminated union over ${RETRO_PROPOSAL_TYPES.length} types (${RETRO_PROPOSAL_TYPES.join(", ")}). Story 6.3 (FR58, FR59).`; async function writeRetroProposal(opts) { const { targetRepoRoot, @@ -60173,7 +60189,7 @@ function registerAllTools(server) { }); server.registerTool({ name: "writeRetroProposal", - description: "Write a single immutable retro-proposal markdown file under /.flow/retro-proposals/.md. The file carries a YAML frontmatter block (validated via RetroProposalFileSchema; source of truth for Epic 6b apply-time re-validation) plus a rendered Markdown body with one H2 per proposal. Refuses collisions with RetroProposalAlreadyExistsError (proposals are immutable). Refuses malformed payloads with MalformedRetroProposalError \u2014 closed discriminated union over seven types (rule, rule-retirement, skill-create, skill-revise, skill-supersede, skill-retire, team-change). Story 6.3 (FR58, FR59).", + description: WRITE_RETRO_PROPOSAL_DESCRIPTION, inputSchema: writeRetroProposalInputSchema, handler: async (args) => { try { diff --git a/plugins/flow/mcp-server/src/schemas/retro-proposal.ts b/plugins/flow/mcp-server/src/schemas/retro-proposal.ts index 40d00daf..ba9bebd2 100644 --- a/plugins/flow/mcp-server/src/schemas/retro-proposal.ts +++ b/plugins/flow/mcp-server/src/schemas/retro-proposal.ts @@ -89,7 +89,7 @@ const PathInsideRepoSchema = z ); // --------------------------------------------------------------------------- -// Per-variant schemas (seven discriminator literals) +// Per-variant schemas (one per literal in RETRO_PROPOSAL_TYPES) // --------------------------------------------------------------------------- /** @@ -519,10 +519,13 @@ const SharedSkillPromotionProposalSchema = ProposalBase.extend({ // --------------------------------------------------------------------------- /** - * The closed set of twelve proposal-type literals. Exported as a tuple so + * The closed set of proposal-type literals. Exported as a tuple so * tests can iterate over it and assert the surface has not silently - * grown (the AC2 invariant). Adding a thirteenth variant requires a - * coordinated schema-change story. + * grown (the AC2 invariant). Adding a new variant requires a + * coordinated schema-change story. Consumers that enumerate the accepted + * types (e.g. the `writeRetroProposal` tool description) MUST derive from + * this constant rather than hardcoding a count — it has already grown + * 7 → 11 → 13. * * `build-story` (added Story native:01KV76P2DW42BPBPT4ZQ0FS63Y) is the * engine-safety output: the retro writer emits it in place of a `skill-*` diff --git a/plugins/flow/mcp-server/src/tools/__tests__/write-retro-proposal.test.ts b/plugins/flow/mcp-server/src/tools/__tests__/write-retro-proposal.test.ts index 214881f6..7c0eed9c 100644 --- a/plugins/flow/mcp-server/src/tools/__tests__/write-retro-proposal.test.ts +++ b/plugins/flow/mcp-server/src/tools/__tests__/write-retro-proposal.test.ts @@ -26,12 +26,16 @@ import { MalformedRetroProposalError, RetroProposalAlreadyExistsError, } from "../../errors.js"; -import { parseRetroProposalFile } from "../../schemas/retro-proposal.js"; +import { + parseRetroProposalFile, + RETRO_PROPOSAL_TYPES, +} from "../../schemas/retro-proposal.js"; import { writeRetroProposal, routeDurability, DURABILITY_REASONS, classifySkillChangeTarget, + WRITE_RETRO_PROPOSAL_DESCRIPTION, } from "../write-retro-proposal.js"; // --------------------------------------------------------------------------- @@ -1285,3 +1289,215 @@ describe("renderRetroRecommendationsBlock — integration with summariseRetroPro expect(block).not.toMatch(/\d+\.\s+\[/); }); }); + +// --------------------------------------------------------------------------- +// Tool description ↔ schema parity +// (Story native:01KW5W173M53FMZG7DAWPR121Q — AC1) +// The writeRetroProposal tool description must enumerate exactly the types in +// RETRO_PROPOSAL_TYPES, derived from the constant rather than a hardcoded count +// (the contract has already drifted 7 → 11 → 13). +// --------------------------------------------------------------------------- + +describe("WRITE_RETRO_PROPOSAL_DESCRIPTION — AC1: enumerates exactly RETRO_PROPOSAL_TYPES", () => { + it("lists every accepted proposal type, derived from the constant", () => { + for (const type of RETRO_PROPOSAL_TYPES) { + expect(WRITE_RETRO_PROPOSAL_DESCRIPTION).toContain(type); + } + }); + + it("states the type count derived from the constant, not a hardcoded number", () => { + // The count must equal RETRO_PROPOSAL_TYPES.length, so it can never drift. + expect(WRITE_RETRO_PROPOSAL_DESCRIPTION).toContain( + `${RETRO_PROPOSAL_TYPES.length} types`, + ); + // The stale hardcoded count from the original seven-variant era must be gone. + expect(WRITE_RETRO_PROPOSAL_DESCRIPTION).not.toContain("seven types"); + }); + + it("does not enumerate any type that is not in RETRO_PROPOSAL_TYPES", () => { + // Extract the parenthesised "(a, b, c)" type list that follows "N types". + const match = WRITE_RETRO_PROPOSAL_DESCRIPTION.match( + /\d+ types \(([^)]+)\)/, + ); + expect(match).not.toBeNull(); + const listed = match![1]!.split(",").map((s) => s.trim()); + // The listed set equals RETRO_PROPOSAL_TYPES exactly — order-preserving, + // no extras, no omissions. + expect(listed).toEqual([...RETRO_PROPOSAL_TYPES]); + }); +}); + +// --------------------------------------------------------------------------- +// Every proposal type round-trips through the schema validator +// (Story native:01KW5W173M53FMZG7DAWPR121Q — AC2) +// Each type in RETRO_PROPOSAL_TYPES — including the ones added since the +// original seven — must validate cleanly through parseRetroProposalFile. +// --------------------------------------------------------------------------- + +describe("RETRO_PROPOSAL_TYPES — AC2: every type round-trips through the schema validator", () => { + const ISO_RT = "2026-06-29T12:00:00.000Z"; + const RT_ID = "01HZRETR00000000000000RT01"; + const RT_RULE_ID = "01HZRETR00000000000000RT02"; + + // One minimal valid fixture per accepted proposal type. Keyed by the + // discriminator literal so the test can assert full coverage of + // RETRO_PROPOSAL_TYPES below. + const FIXTURES: Record<(typeof RETRO_PROPOSAL_TYPES)[number], unknown> = { + rule: { + type: "rule", + id: RT_ID, + created_at: ISO_RT, + rationale: "A rule rationale.", + text: "A rule.", + target_failure_class: "some-class", + recommended_promotion_level: "must", + }, + "rule-retirement": { + type: "rule-retirement", + id: RT_ID, + created_at: ISO_RT, + rationale: "Rule never fires.", + target_rule_id: RT_RULE_ID, + fire_count_over_window: 0, + recommended_action: "retire", + }, + "skill-create": { + type: "skill-create", + id: RT_ID, + created_at: ISO_RT, + rationale: "Operators need a wrapper.", + proposed_path: ".flow/skills/do-x.md", + frontmatter_description: "Skill that helps do X.", + body: "# Do X\n\nBody.\n", + }, + "skill-revise": { + type: "skill-revise", + id: RT_ID, + created_at: ISO_RT, + rationale: "Tighten the skill.", + target_skill_path: ".flow/skills/do-x.md", + revised_body: "# Do X (revised)\n\nShorter.\n", + version_bump: "patch", + }, + "skill-supersede": { + type: "skill-supersede", + id: RT_ID, + created_at: ISO_RT, + rationale: "Replace the skill wholesale.", + superseded_skill_path: ".flow/skills/old.md", + replacement: { + proposed_path: ".flow/skills/new.md", + frontmatter_description: "Replacement skill.", + body: "# New\n\nBody.\n", + }, + }, + "skill-retire": { + type: "skill-retire", + id: RT_ID, + created_at: ISO_RT, + rationale: "Skill never fired.", + target_skill_path: ".flow/skills/dead.md", + last_invoked_at: null, + }, + "team-change": { + type: "team-change", + id: RT_ID, + created_at: ISO_RT, + rationale: "Security verdicts keep surfacing.", + action: "hire", + target_role: "security-reviewer", + justification: "12 fires in 10 cycles.", + predicted_impact: { affected_failure_classes: ["security-audit"] }, + }, + "persona-append": { + type: "persona-append", + id: RT_ID, + created_at: ISO_RT, + rationale: "Dev forgot the locked phrase.", + target_role: "generalist-dev", + lesson: "Always emit the locked handoff phrase last.", + }, + "promote-lesson-to-skill": { + type: "promote-lesson-to-skill", + id: RT_ID, + created_at: ISO_RT, + rationale: "Lesson is reusable across roles.", + target_role: "generalist-dev", + lesson_id: RT_RULE_ID, + proposed_skill_path: ".flow/skills/pre-flight.md", + skill_description: "Pre-flight checklist skill.", + skill_body: "# Pre-flight\n\nSteps.\n", + when_to_use: "Run before every story claim.", + }, + "build-story": { + type: "build-story", + id: RT_ID, + created_at: ISO_RT, + rationale: "Engine-targeted skill change needs a build story.", + suggested_title: "Revise core skill via build-and-review", + skill_change_context: + "skill-revise targeting plugins/flow/catalogue/retro-analyst.md", + }, + "lesson-consolidation": { + type: "lesson-consolidation", + id: RT_ID, + created_at: ISO_RT, + rationale: "Two near-duplicate lessons.", + target_role: "generalist-dev", + lesson_a_id: RT_RULE_ID, + lesson_b_id: RT_ID, + lesson_a_text: "Lesson A text.", + lesson_b_text: "Lesson B text.", + merged_lesson: "Merged sharper lesson.", + }, + "lesson-retirement": { + type: "lesson-retirement", + id: RT_ID, + created_at: ISO_RT, + rationale: "These lessons never earned keep.", + target_role: "generalist-dev", + lesson_retirements: [{ id: RT_RULE_ID, reason: "Never fired." }], + }, + "shared-skill-promotion": { + type: "shared-skill-promotion", + id: RT_ID, + created_at: ISO_RT, + rationale: "Two roles share the same lesson.", + sharing_roles: ["generalist-dev", "generalist-reviewer"], + shared_lesson_text: "Shared lesson text.", + representative_lesson_id: RT_RULE_ID, + proposed_skill_path: ".flow/skills/shared.md", + skill_description: "Shared skill.", + }, + }; + + it("has a fixture for every type in RETRO_PROPOSAL_TYPES (no type left untested)", () => { + expect(Object.keys(FIXTURES).sort()).toEqual( + [...RETRO_PROPOSAL_TYPES].sort(), + ); + }); + + it.each([...RETRO_PROPOSAL_TYPES])( + "round-trips a '%s' proposal through parseRetroProposalFile", + (type) => { + const file = parseRetroProposalFile({ + iso_timestamp: ISO_RT, + cycle_window: null, + proposals: [FIXTURES[type]], + }); + expect(file.proposals).toHaveLength(1); + expect(file.proposals[0]!.type).toBe(type); + }, + ); + + it("validates all types together in a single file", () => { + const file = parseRetroProposalFile({ + iso_timestamp: ISO_RT, + cycle_window: null, + proposals: RETRO_PROPOSAL_TYPES.map((t) => FIXTURES[t]), + }); + expect(file.proposals.map((p) => p.type)).toEqual([ + ...RETRO_PROPOSAL_TYPES, + ]); + }); +}); diff --git a/plugins/flow/mcp-server/src/tools/register.ts b/plugins/flow/mcp-server/src/tools/register.ts index ccdd4c46..21cd922f 100644 --- a/plugins/flow/mcp-server/src/tools/register.ts +++ b/plugins/flow/mcp-server/src/tools/register.ts @@ -75,7 +75,10 @@ import { claimStory } from "./claim-story.js"; import { completeStory } from "./complete-story.js"; import { recordStoryRetro } from "./record-story-retro.js"; import { recordReviewerLesson } from "./record-reviewer-lesson.js"; -import { writeRetroProposal } from "./write-retro-proposal.js"; +import { + writeRetroProposal, + WRITE_RETRO_PROPOSAL_DESCRIPTION, +} from "./write-retro-proposal.js"; import { acceptProposal } from "./accept-proposal.js"; import { gatherRetroInputs } from "./gather-retro-inputs.js"; import { listClaimableTodos } from "./list-claimable-todos.js"; @@ -899,15 +902,7 @@ export function registerAllTools(server: AiEngineeringTeamServer): void { // timestamp. FR58, FR59. server.registerTool({ name: "writeRetroProposal", - description: - "Write a single immutable retro-proposal markdown file under " + - "/.flow/retro-proposals/.md. The file carries a YAML " + - "frontmatter block (validated via RetroProposalFileSchema; source of truth for " + - "Epic 6b apply-time re-validation) plus a rendered Markdown body with one H2 per " + - "proposal. Refuses collisions with RetroProposalAlreadyExistsError (proposals are " + - "immutable). Refuses malformed payloads with MalformedRetroProposalError — closed " + - "discriminated union over seven types (rule, rule-retirement, skill-create, " + - "skill-revise, skill-supersede, skill-retire, team-change). Story 6.3 (FR58, FR59).", + description: WRITE_RETRO_PROPOSAL_DESCRIPTION, inputSchema: writeRetroProposalInputSchema, handler: async (args) => { try { diff --git a/plugins/flow/mcp-server/src/tools/write-retro-proposal.ts b/plugins/flow/mcp-server/src/tools/write-retro-proposal.ts index 7465026a..b7560389 100644 --- a/plugins/flow/mcp-server/src/tools/write-retro-proposal.ts +++ b/plugins/flow/mcp-server/src/tools/write-retro-proposal.ts @@ -47,7 +47,11 @@ * frontmatter, not the body. * * FR58 — single proposal markdown file under `/.flow/retro-proposals/.md`. - * FR59 — seven typed proposal variants. + * FR59 — the typed proposal variants enumerated in `RETRO_PROPOSAL_TYPES` + * (schemas/retro-proposal.ts). The contract grew past the original seven + * (it now covers persona-append, lesson-consolidation, shared-skill-promotion, + * and more); the tool description is derived from that constant rather than a + * hardcoded count so it can never drift out of step with the schema again. */ import { promises as fs } from "node:fs"; @@ -57,6 +61,7 @@ import { RetroProposalAlreadyExistsError } from "../errors.js"; import { writeManagedFile } from "../lib/managed-fs.js"; import { parseRetroProposalFile, + RETRO_PROPOSAL_TYPES, type DurabilityRecommendation, type DurabilityRoutingContext, type RetroProposal, @@ -64,6 +69,24 @@ import { } from "../schemas/retro-proposal.js"; import { ulid } from "ulid"; +/** + * The MCP tool description for `writeRetroProposal`, derived from + * `RETRO_PROPOSAL_TYPES` so the enumerated list (and its count) can never drift + * out of step with the schema's accepted variants. The contract has already + * grown 7 → 11 → 13; hardcoding either the list or the number guarantees a stale + * description, so both are interpolated from the constant (Story + * native:01KW5W173M53FMZG7DAWPR121Q — AC1). + */ +export const WRITE_RETRO_PROPOSAL_DESCRIPTION = + "Write a single immutable retro-proposal markdown file under " + + "/.flow/retro-proposals/.md. The file carries a YAML " + + "frontmatter block (validated via RetroProposalFileSchema; source of truth for " + + "Epic 6b apply-time re-validation) plus a rendered Markdown body with one H2 per " + + "proposal. Refuses collisions with RetroProposalAlreadyExistsError (proposals are " + + "immutable). Refuses malformed payloads with MalformedRetroProposalError — closed " + + `discriminated union over ${RETRO_PROPOSAL_TYPES.length} types ` + + `(${RETRO_PROPOSAL_TYPES.join(", ")}). Story 6.3 (FR58, FR59).`; + /** * Options accepted by `writeRetroProposal`. * @@ -143,9 +166,9 @@ export async function writeRetroProposal( // validates `iso_timestamp` (defends against path-traversal in the // filename component) AND each proposal in `proposals` via the // discriminated union — a single Zod pass covers both AC1's "validate - // before path-form" and AC2's "discriminated union over seven - // literals." `parseRetroProposalFile` throws MalformedRetroProposalError - // on failure. + // before path-form" and AC2's "discriminated union over the + // RETRO_PROPOSAL_TYPES literals." `parseRetroProposalFile` throws + // MalformedRetroProposalError on failure. let fileShape: RetroProposalFile = parseRetroProposalFile({ iso_timestamp: isoTimestamp, cycle_window: cycleWindow,