fix(cli): stop letting a stub's own description freeze a stale CLI reference - #312
fix(cli): stop letting a stub's own description freeze a stale CLI reference#312thecodedrift wants to merge 1 commit into
Conversation
…ference A `.claude`/`.agents` reference stub's frontmatter `description` is copied verbatim from the embedded skill/command source, and unlike the canonical `.taskless/` copy, is never passed through the build-target invocation rewrite. A description that named the CLI invocation therefore froze a stale, unpinned `npx @taskless/cli` reference into every stub forever, even on a nightly install whose canonical file correctly names the pinned `@taskless/cli-nightly@<version>` package. Remove the invocation from the skill and command `description` fields entirely, so there is no second, unrewritten copy left to go stale. The canonical file already carries the correct per-build invocation, and a stub always defers to it. Adds a source-level guard against reintroducing a CLI package reference into any description, plus an end-to-end nightly-build test that installs and reads back the actual stub.
|
@claude /review |
|
Claude finished @thecodedrift's task in 4m 15s —— View job Review completeReview mode: incremental — read 0 prior review thread(s) before reviewing.
SummaryThe root-cause analysis and fix are correct. I also verified the self-healing story holds for already-affected installs: The two new test files are well-targeted (source-level guard + a real nightly-build end-to-end check via the existing [New] finding (inline, non-blocking): No correctness, security, or lint/convention issues found in the production code change itself. |
| it("a second install pinned to a different nightly leaves the stub unchanged", async () => { | ||
| await install(); | ||
| const first = await readFile(skillStubPath(), "utf8"); | ||
|
|
||
| // Re-running install (same build, so same pin) must not rewrite a stub | ||
| // whose description never carried a version to begin with. | ||
| await install(); | ||
| expect(await readFile(skillStubPath(), "utf8")).toBe(first); | ||
| }); |
There was a problem hiding this comment.
[New] Test name doesn't match what the test does. The title says "pinned to a different nightly," but the body (and its own comment) reruns install() with the exact same nightly define twice — it's an idempotency check on identical config, not a cross-pin check:
// Re-running install (same build, so same pin) must not rewrite a stub
// whose description never carried a version to begin with.
await install();Two consequences worth a second look:
- Since
__TASKLESS_CLI__is a compile-time Vite define fixed for the wholenightlyproject (seevite.config.ts's singleNIGHTLY_TEST_INVOCATION), a real "different pin" can't be produced within this test as written — you'd need to hand-write a stub file whose on-diskdescriptionmimics an older, differently-pinned install, then runinstall()under this fixed define and assert on the outcome. - If that scenario were actually constructed, the correct expectation is the opposite of this test's name: a stub whose description still names a stale package reference should get rewritten (that's
stubFrontmatterDrifteddoing its job, and the whole reason this fix removes the invocation from source rather than leaving a second copy to converge on demand). "Unchanged" is only true because the description is now version-free — the two reinstalls are byte-identical inputs, not different pins.
Not a functional bug (the generic drift-on-description-change path is already unit-tested directly in canonical-store.test.ts's stubFrontmatterDrifted suite), but as written this test would not catch a regression where a different pin caused a rewrite (or failed to). Consider either renaming it to describe what it actually checks (idempotent reinstall under the same build), or replacing the body with a hand-crafted stale-stub fixture to test the claim in the title.
Summary
npx @taskless/cli-nightly@latest initcorrectly rewrites the canonical.taskless/skill/command files to name the pinned nightly package, but the.claude/.agents/etc. reference stubs it also writes never get their frontmatterdescriptionupdated — it keeps sayingnpx @taskless/cliforever.Root cause:
getEmbeddedSkills()/getEmbeddedCommands()(packages/cli/src/install/install.ts) readname/descriptionstraight out of a skill/command's raw source frontmatter.writeSkill/writeCommandthen copy thatdescriptionverbatim into a stub's own frontmatter (StubFrontmatter,packages/cli/src/install/canonical.ts). That copy is never passed throughapplyCliInvocation— only the canonical.taskless/copy gets that rewrite (writeCanonicalSkill/writeCanonicalCommand). A description that names the CLI invocation therefore freezes whatever build wrote it first, andstubFrontmatterDriftedonly rewrites a stub whenname/descriptionactually change — which they never will once frozen with a stale string, since the stub is deliberately kept byte-stable across ordinary releases.Fix — Option 1 from the issue (chosen over regenerating the stub)
Removed the CLI invocation from the two source
descriptionfields that had it (skills/taskless/SKILL.md,commands/tskl/tskl.md), so there is no longer a second, unrewritten copy to go stale. The canonical.taskless/file remains the single place the invocation is written, and every stub already just says "read the canonical file" — it never needed to restate the invocation itself.I preferred this over "regenerate the whole stub from
taskless.json's version/channel" because that keeps the duplication and requires the regeneration logic to be correct forever; removing the only place the string could go stale is a smaller, permanent fix rather than an ongoing correctness obligation.Tests
Since the real
@taskless/cli-nightlypackage is blocked by a deny rule here, I drove the version/channel through the existingtest/nightly/vitest project (vite.config.tsalready defines a pinned@taskless/cli-nightly@0.0.0-nightly.testbuild target for exactly this purpose — seetest/nightly/stub-recovery-invocation.test.tsfor the established pattern).packages/cli/test/stub-description-invocation.test.ts— source-level guard: asserts no embedded skill/commanddescriptionnames@taskless/cli(in any form). Runs under the default (prod) project.packages/cli/test/nightly/stub-description-invocation.test.ts— end-to-end: runs a realapplyInstallPlanunder the nightly build define and reads back the written.claudestub, asserting its frontmatterdescriptionnames no CLI package, while the canonical.taskless/file it's compared against correctly does.Mutation check: reintroduced
`npx @taskless/cli agent route`intoskills/taskless/SKILL.md'sdescription:field. Both new test files failed (source-level test and the nightly end-to-end test). Reverted the mutation; both pass again.Verification
NODE_OPTIONS= pnpm typecheck— passNODE_OPTIONS= pnpm --filter @taskless/cli test— 85 files / 1350 tests pass (includes the two new files, and the nightly project run)NODE_OPTIONS= pnpm lint(builds first, then runspnpm cli check) — pass, "No issues found."node dist/index.js init --yesin a scratch directory; confirmed the generated.agents/skills/taskless/SKILL.mdstub'sdescriptionno longer contains any CLI invocation. (This only exercises the prod build target locally — the nightly-channel behavior itself is verified through thetest/nightly/project above, not against the real published package.)What I did not change
packages/cli/src/commands/init.tsis owned by another agent working a separate stdout bug in the same repo; I did not touch it and did not need to.Fixes #298