Skip to content

fix(cli): stop letting a stub's own description freeze a stale CLI reference - #312

Open
thecodedrift wants to merge 1 commit into
mainfrom
fix/nightly-stub-package-reference
Open

fix(cli): stop letting a stub's own description freeze a stale CLI reference#312
thecodedrift wants to merge 1 commit into
mainfrom
fix/nightly-stub-package-reference

Conversation

@thecodedrift

Copy link
Copy Markdown
Member

Summary

npx @taskless/cli-nightly@latest init correctly 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 frontmatter description updated — it keeps saying npx @taskless/cli forever.

Root cause: getEmbeddedSkills()/getEmbeddedCommands() (packages/cli/src/install/install.ts) read name/description straight out of a skill/command's raw source frontmatter. writeSkill/writeCommand then copy that description verbatim into a stub's own frontmatter (StubFrontmatter, packages/cli/src/install/canonical.ts). That copy is never passed through applyCliInvocation — only the canonical .taskless/ copy gets that rewrite (writeCanonicalSkill/writeCanonicalCommand). A description that names the CLI invocation therefore freezes whatever build wrote it first, and stubFrontmatterDrifted only rewrites a stub when name/description actually 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 description fields 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-nightly package is blocked by a deny rule here, I drove the version/channel through the existing test/nightly/ vitest project (vite.config.ts already defines a pinned @taskless/cli-nightly@0.0.0-nightly.test build target for exactly this purpose — see test/nightly/stub-recovery-invocation.test.ts for the established pattern).

  • packages/cli/test/stub-description-invocation.test.ts — source-level guard: asserts no embedded skill/command description names @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 real applyInstallPlan under the nightly build define and reads back the written .claude stub, asserting its frontmatter description names no CLI package, while the canonical .taskless/ file it's compared against correctly does.

Mutation check: reintroduced `npx @taskless/cli agent route` into skills/taskless/SKILL.md's description: 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 — pass
  • NODE_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 runs pnpm cli check) — pass, "No issues found."
  • Manually built the CLI and ran node dist/index.js init --yes in a scratch directory; confirmed the generated .agents/skills/taskless/SKILL.md stub's description no longer contains any CLI invocation. (This only exercises the prod build target locally — the nightly-channel behavior itself is verified through the test/nightly/ project above, not against the real published package.)

What I did not change

packages/cli/src/commands/init.ts is 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

…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.
@thecodedrift

Copy link
Copy Markdown
Member Author

@claude /review

@claude

claude Bot commented Sep 8, 2026

Copy link
Copy Markdown

Claude finished @thecodedrift's task in 4m 15s —— View job


Review complete

Review mode: incremental — read 0 prior review thread(s) before reviewing. .prior-review.json contains no review threads or reviews, only the top-level @claude /review request that triggered this run — this is the first review of this PR.

Summary

The root-cause analysis and fix are correct. getEmbeddedSkills()/getEmbeddedCommands() (packages/cli/src/install/install.ts) do read name/description straight from source frontmatter, writeSkill/writeCommand do copy that description verbatim into StubFrontmatter without ever routing it through applyCliInvocation, and only the canonical .taskless/ copy gets that rewrite (writeCanonicalSkill/writeCanonicalCommand in packages/cli/src/install/canonical.ts). Removing the invocation from the two source description fields (skills/taskless/SKILL.md, commands/tskl/tskl.md) is a sound fix: I confirmed via grep that these were the only two skill/command source files in the repo (skills/**/SKILL.md, commands/tskl/**/*.md currently resolve to exactly these two), and that description has no other consumer in the codebase besides the stub/canonical frontmatter paths — so the fix is completely scoped.

I also verified the self-healing story holds for already-affected installs: stubFrontmatterDrifted (in canonical.ts) compares the on-disk stub's description against the newly-computed source description, and its "description changed → true" branch is already directly unit-tested in packages/cli/test/canonical-store.test.ts. Since this PR changes the source description text, any previously-frozen stub (with the old npx @taskless/cli agent ... text baked in) will diverge from the new source description on the next init/update and get rewritten — so existing broken installs converge without any special-cased migration logic. Good use of the existing general mechanism rather than a bandaid.

The two new test files are well-targeted (source-level guard + a real nightly-build end-to-end check via the existing test/nightly/ vitest project), and I confirmed the nightly/cli vitest project split in vite.config.ts is pre-existing infrastructure this PR correctly reuses rather than introduces.

[New] finding (inline, non-blocking): packages/cli/test/nightly/stub-description-invocation.test.ts:101-109 — the test titled "a second install pinned to a different nightly leaves the stub unchanged" doesn't actually simulate a different pin; it reruns install() twice under the identical fixed nightly define, so it's really an idempotent-reinstall check. Since the underlying stubFrontmatterDrifted behavior is already covered generically elsewhere, this doesn't leave a real coverage gap, but the name overstates what's being tested and, read literally, describes the opposite of the intended behavior (a stub naming a genuinely different/stale pin should be rewritten, not left unchanged). Left as an inline suggestion to rename or replace with a hand-crafted stale-stub fixture.

No correctness, security, or lint/convention issues found in the production code change itself.

Comment on lines +101 to +109
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);
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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:

  1. Since __TASKLESS_CLI__ is a compile-time Vite define fixed for the whole nightly project (see vite.config.ts's single NIGHTLY_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-disk description mimics an older, differently-pinned install, then run install() under this fixed define and assert on the outcome.
  2. 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 stubFrontmatterDrifted doing 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Nightly init leaves .claude/ stub files referencing stale @taskless/cli instead of @taskless/cli-nightly

1 participant