ref(cli): move the recipe glob and renderer into src/prompts - #70
ref(cli): move the recipe glob and renderer into src/prompts#70thecodedrift wants to merge 11 commits into
Conversation
fbe48f9 to
1f60eb2
Compare
A spec-only proposal is its own tip until its implementation is stacked on top, so the gate demanded it archive a change nobody had built yet. #70, #71, and #72 have failed this check on every run since July 28 for exactly that reason — weeks of red on PRs that were never merge-eligible, which is how a team learns to stop reading red. A draft cannot merge, so skipping it costs nothing: no unarchived change can reach `main` either way. `ready_for_review` is added to the trigger types because the default set (opened/synchronize/reopened) does not include it — without that, a draft could be marked ready and merged on a green that was never re-evaluated. That event is what preserves the guarantee. The tip rule is unchanged for PRs that are ready for review.
…rompts Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LdEhGzeQfSGJM3nKKNj7Bp
Prompts become render functions returning fully-rendered text rather than raw placeholder-bearing strings — every %(KEY)s resolves inside the package, and rendering carries applyCliInvocation so the export cannot diverge from `taskless help` under non-prod builds. Confirms @taskless/cli/prompts as the published subpath, and adds a completeness check requirement so topic membership stays an explicit list that a new or deleted recipe file cannot silently change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CDv57zHq7abms3RReSQw6q
The cloud consumer established that only `static` is reachable server-side: `route` decides a pre-service authoring destination, `remote` states the service owns rule-type selection, and `detect` / `existing` / `rule-meta` are local-only. TOPICS ships minimal and the rest stay internal, since an exported name is a promise held for a major. Recorded as D6, correcting an earlier six-topic assumption this design had read off the consumer's draft rather than the recipes. Adds PromptOptions.header (default true) so a consumer can drop the version-bearing header line, which would otherwise sit in an LLM prompt-cache key and be invalidated by every CLI publish. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CDv57zHq7abms3RReSQw6q
The consumer retracted its prerelease request, preferring to wait for a stable version over pinning a moving one against an API that isn't semver-stable yet. Nothing downstream now waits on this change, so it releases at whatever cadence suits this repo. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CDv57zHq7abms3RReSQw6q
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CDv57zHq7abms3RReSQw6q
Two units, each independently safe: a pure refactor that must leave help output byte-identical, then the new export surface nothing depends on yet. Splitting them keeps the risky part — touching a shipped command — reviewed apart from the new public API. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CDv57zHq7abms3RReSQw6q
Every one of these PRs carried skip-changeset while it was spec-only, which becomes wrong the moment implementation lands. Stating the impact in the proposal means the tip PR needs a changeset written, not a label kept. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CDv57zHq7abms3RReSQw6q
1f60eb2 to
1efa79b
Compare
`commands/help.ts` owned the `import.meta.glob` over `help/*.txt`, the canonical/anonymous map build, the `TOPIC_INPUT_SCHEMAS` table, and `renderRecipe`. That put the recipe source and its interpolation behind a citty command, so nothing else could reach the text without duplicating it. Move all of it to `src/prompts/recipes.ts` and have `help` and `onboard` call `getRecipe`. The module carries no CLI runtime: embedded text, `sprintf-js`, `applyCliInvocation`, and the two leaf Zod input schemas. Pure refactor. `help` output is byte-identical, verified across 38 captures covering all 18 canonical topics, their `--anonymous` variants, the topic index, and the unknown-topic error path, with a zero-byte diff before and after. Unit 1 of 2 for export-knowledge-prompts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Refactors the CLI’s embedded help “recipes” so the glob, lookup maps, and rendering logic live in a shared src/prompts/recipes.ts module, which is then consumed by both help and onboard. This is unit 1 of the stacked change to later export prompts via @taskless/cli/prompts, and is intended to preserve byte-identical help output while de-coupling prompt rendering from the help command implementation.
Changes:
- Introduces
packages/cli/src/prompts/recipes.tsto own recipe embedding (import.meta.glob), canonical/anonymous maps, and rendering. - Updates
packages/cli/src/commands/help.tsto use the sharedgetRecipeimplementation (removing its local glob/renderer). - Updates
packages/cli/src/commands/onboard.tsto importgetRecipefrom the shared prompts module.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/cli/src/prompts/recipes.ts | New shared module that embeds and renders help recipe text and exposes getRecipe. |
| packages/cli/src/commands/help.ts | Removes local recipe embedding/rendering and delegates to getRecipe. |
| packages/cli/src/commands/onboard.ts | Switches recipe lookup import to the shared prompts module. |
| openspec/changes/export-knowledge-prompts/.openspec.yaml | Adds OpenSpec change metadata for the prompts-export effort. |
| openspec/changes/export-knowledge-prompts/tasks.md | Adds task checklist for the change (currently has path/status inconsistencies vs this PR). |
| openspec/changes/export-knowledge-prompts/specs/cli-knowledge-prompts/spec.md | Adds spec requirements for the prompts export and behavior. |
| openspec/changes/export-knowledge-prompts/proposal.md | Adds proposal describing the prompts export change (currently references outdated module path). |
| openspec/changes/export-knowledge-prompts/design.md | Adds design decisions for the prompts export (currently references outdated module path). |
Suppressed comments (1)
openspec/changes/export-knowledge-prompts/design.md:24
- Design doc D2 references
src/prompts/index.ts, but the implementation module issrc/prompts/recipes.tsin this PR. Keeping this consistent helps validate the “no CLI runtime deps” constraint against the correct file.
`src/prompts/index.ts` imports nothing from the CLI runtime — no `citty`/command tree, no telemetry, no filesystem or network. It may import `sprintf-js` and the two Zod input schemas (`schemas/rules-create`, `schemas/rules-improve`), which are leaf modules whose only dependency is `zod`; both are already dependencies of the intended consumer. The subpath export `@taskless/cli/prompts` maps to a dedicated `dist/prompts.js` so importing it never loads `dist/index.js`.
* origin/main: chore: grow the changeset with the committed sg config docs(openspec): make cli-check internally consistent; correct the generateSgConfig note test(cli): isolate the missing-binary test from the host environment docs(openspec): archive partition-rules-by-engine ref(cli): declare COMMITTED_SG_CONFIG beside the layout it derives from docs(cli): describe the engine-partitioned layout in help and scaffolding feat(cli): run ast-grep against the committed sg config chore: grow the changeset with the runtime discovery path docs(openspec): correct the group-1 note, which no longer describes reality ref(cli): retire layout.ts now that ENGINE_LAYOUTS supersedes it feat(cli): discover runtime rules under the runtime engine directory
|
@claude /review |
|
Claude finished @thecodedrift's task in 4m 54s —— View job Review: unit 1 of the knowledge-prompt exportTasks
SummaryThis is a clean, well-contained refactor. I traced every deleted line in
Two things worth a look (posted inline)
Neither is a correctness bug — the reachable Also noted (already flagged by the other automated review, so not re-posted inline)The No security, correctness, or test-adequacy issues found in the |
| export interface RecipeOptions { | ||
| /** | ||
| * Select the `.anonymous` variant of the topic, falling back to the | ||
| * canonical recipe when the topic has no variant. | ||
| * | ||
| * @default false | ||
| */ | ||
| anonymous?: boolean; | ||
| /** | ||
| * Value substituted for the `%(PACKAGE_MANAGER_DLX)s` placeholder. The | ||
| * default is an agent-fill marker, which is the right answer whenever | ||
| * the caller does not know the consuming repo's package manager. | ||
| * | ||
| * @default "<package-manager-dlx>" | ||
| */ | ||
| packageManagerDlx?: string; | ||
| /** | ||
| * Include the `# Topic: <name> (CLI v<version> / topic vN)` first line. | ||
| * Suppressing it drops the CLI version from the text, which matters to | ||
| * an LLM consumer whose prompt-cache key would otherwise churn on every | ||
| * CLI publish. | ||
| * | ||
| * @default true | ||
| */ | ||
| header?: boolean; | ||
| } | ||
|
|
||
| /** | ||
| * Render a recipe by interpolating sprintf-js named arguments. The recipe | ||
| * source uses `%(KEY)s` placeholders; the variable table built here resolves | ||
| * each known placeholder to its rendered string. Recipes that contain a | ||
| * literal `%` character must escape it as `%%` per sprintf-js conventions. | ||
| * | ||
| * Two flavors of substitution coexist in the variables table: | ||
| * - System-resolved values (e.g. `CLI_VERSION`) — rendered to a real value. | ||
| * - Agent-fill markers (e.g. `PACKAGE_MANAGER_DLX`) — rendered as | ||
| * `<lower-kebab-name>` so the consuming agent knows to substitute. | ||
| */ | ||
| function renderRecipe( | ||
| content: string, | ||
| topic: string, | ||
| options: RecipeOptions = {} | ||
| ): string { | ||
| const variables: Record<string, string> = { | ||
| CLI_VERSION: __VERSION__, | ||
| PACKAGE_MANAGER_DLX: | ||
| options.packageManagerDlx ?? PACKAGE_MANAGER_DLX_MARKER, | ||
| }; | ||
| if (content.includes("%(INPUT_SCHEMA)s")) { | ||
| const schema = TOPIC_INPUT_SCHEMAS[topic]; | ||
| variables.INPUT_SCHEMA = schema | ||
| ? JSON.stringify(z.toJSONSchema(schema), null, 2) | ||
| : "(no input schema for this topic)"; | ||
| } | ||
| const rendered = sprintf(applyCliInvocation(content), variables); | ||
| return options.header === false ? stripHeader(rendered) : rendered; |
There was a problem hiding this comment.
Scope: new, untested behavior inside a "pure refactor" unit.
RecipeOptions.header / packageManagerDlx and the stripHeader() branch (line 124) are not moved code — they're new functionality that didn't exist in help.ts before this PR. Neither is reachable from any caller today: help.ts only ever passes { anonymous } to getRecipe, so options.header === false and a caller-supplied packageManagerDlx are dead paths with zero test coverage in this PR.
The 38-capture byte-identical parity evidence in the PR description only exercises the reachable path (the existing help behavior), so it doesn't actually validate stripHeader()'s correctness — e.g. behavior on a recipe with no blank line after the header, no header at all, or a # Topic:-looking string inside a fenced example later in the body. That logic will become load-bearing the moment unit 2 (#87) wires it up, with no verification here.
This also cuts against the PR's own stated rationale for the two-unit split (proposal.md: "the risky part … is reviewed apart from the new API surface") — this is new API surface landing inside the unit that's supposed to be risk-free.
Suggest either: (a) add unit tests for stripHeader/packageManagerDlx now since the code ships now, or (b) defer this plumbing to unit 2 (#87) alongside its own tests, keeping unit 1 a true no-op move.
| const { helpMap, anonymousMap } = buildHelpMaps(); | ||
|
|
||
| /** The canonical `<topic>.txt` recipe names present in the build. */ | ||
| export function canonicalRecipeTopics(): string[] { |
There was a problem hiding this comment.
canonicalRecipeTopics() is exported but has no callers and no tests anywhere in the repo (verified via repo-wide grep). It reads like groundwork for unit 2's completeness check (tasks.md 3.4: TOPICS ∪ INTERNAL_TOPICS vs. on-disk topics), but as written it's dead code shipped in the unit-1 "pure refactor" PR. Consider dropping it from this PR and adding it in #87 when it has a consumer, or at minimum add a test that exercises it if it's meant to ship now.
…edge-prompts * origin/main: fix(cli): size the vitest timeout to what the suite actually does
The proposal, design D1/D2, and task 1.1 all named `src/prompts/index.ts` as the module holding the embed and the renderer. What landed splits in two: `recipes.ts` owns the embed and the render path, `index.ts` is the public entry stacked on top of it. Tasks 1.3 and 1.4 were also checked off here, but the typed API and the INTERNAL_TOPICS classification are unit 2's work in `index.ts` — nothing in this PR implements them. Uncheck them so the tracking matches the diff. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Stack (root → tip):
Carries unit 1 of the knowledge-prompt export: a pure refactor that moves the recipe source and its renderer out from behind the
helpcommand.commands/help.tsowned theimport.meta.globoverhelp/*.txt, the canonical/anonymous map build, theTOPIC_INPUT_SCHEMAStable, andrenderRecipe. All of it now lives insrc/prompts/recipes.ts, whichhelpandonboardconsume viagetRecipe. The module carries no CLI runtime: embedded text,sprintf-js,applyCliInvocation, and the two leaf Zod input schemas.Parity evidence.
helpoutput is byte-identical. It was captured across all 18 canonical topics plus their--anonymousvariants, the topic index, and the unknown-topic error path, 38 captures in total, with a zero-byte diff before and after.Nothing observable changes, so this ships no release note and keeps the
skip-changesetlabel.Where the export lives
The public surface is unit 2, #87: the
./promptssubpath export, the Vite entry, theTOPICS/PromptOptionsAPI, and the completeness check. That PR carries the changeset and archives the change.stack: openspec-archivedskips this PR, because #87 targets this branch and so this is no longer the tip of its stack. The change is archived exactly once, on #87.Where this sits
@taskless/cli/promptsexport (unit 2)vale/directory it scaffolds) and #72 (needs a binary to resolve)This change is delivered as a two-PR stack, merging forward: unit 1 is independently safe in production because it changes no observable behavior, and unit 2 only adds a new export. Each lands on
mainin turn.#87 and #71 are coupled by exactly one line: whichever lands second adds the engine-selection topic to
TOPICS. Ordering between them doesn't matter.Downstream, the generator's decision router (TSKL-279) needs a published release containing #87 and #71. It consumes a normal release, with no prerelease and no path dependency, so it waits without blocking anything here.
Refs OSS-20