fix(cli): correct verify-schema docstrings to the current command form - #318
Conversation
`packages/cli/src/schemas/index.ts` published `verifyOutputSchema` and `valeVerifyOutputSchema` under docstrings naming `taskless rule verify <id> --json`, a command form that no longer exists. `taskless rule` has four subcommands (create/improve/meta/delete); `verify` is a top-level, path-addressed command, precisely because an id can name a rule under two engines and a path cannot (see `resolve-path.ts`). Digging further: `rule verify <id>` was removed in c4dc03d (Aug 13) when addressing moved from id to path. `@taskless/cli/schemas` was created a month later (189afa5, Sep 4) to publish the real `verify --json` envelope for the Cloud eval team, and picked up these two schemas from `rules-verify.ts` without noticing the command they described was gone. So the docstrings aren't merely misnamed — the shape they describe is not emitted by any current command. `verify`/`test --json` print `verifyTestOutputSchema`'s flattened envelope; `verifyOutputSchema`/ `valeVerifyOutputSchema` describe the pre-flattening layer detail that `verifyRule()`/`verifyValeRule()` compute internally and the removed command used to print verbatim. Docstrings corrected to say this. Also corrected a stale `rule verify --schema` comment on `astGrepRuleSchema` (`--schema` was removed CLI-wide in May). Tests: added coverage in `test/schemas-export.test.ts` that reconstructs the exact envelope the removed command built (`{ engine: "sg", ...verifyRule() }`, and the mapped `passed` -> `success` shape for Vale) against the REAL return values of `verifyRule()`/`verifyValeRule()`, and parses it with the published schema. Since no command emits this shape any more, spawning the built CLI (as the existing `verifyTestOutputSchema` test does) isn't possible here; this is the closest available pin, and it fails if either function's return shape drifts from what's published. Mutation-checked: renaming `verifyOutputSchema`'s `ruleId` to `id`, and reverting `valeVerifyOutputSchema`'s `success` mapping back to `passed`, each independently failed the new tests; both were reverted and the suite passes again. Two adjacent questions investigated per issue #283, documented in the PR body rather than acted on: whether `getSchemaPayload`/`schemaOutputSchema` are safe to remove (not fully — `getSchemaPayload` is still used by `test/verify.test.ts`, outside this PR's file scope), and whether the `rule create`/`improve` vs `verify` publication asymmetry is deliberate (it tracked one consumer's specific ask, not a general policy). Fixes #283
|
@claude /review |
|
Claude finished @thecodedrift's task in 2m 45s —— View job Review completeReview mode: incremental — read 0 prior review thread(s) before reviewing.
The core fix is accurate: I independently confirmed Two findings, both about the PR's own stated goal — eliminating stale references to the removed 1. This is the exact stale command form ( 2. // --- Schema mode output (--schema --json) ---Same file this PR already touches — two sections below, the PR correctly rewrote Nothing else stood out: the docstring rewrites in |
Stack (root → tip):
Summary
packages/cli/src/schemas/index.tspublishedverifyOutputSchemaandvaleVerifyOutputSchemaunder docstrings namingtaskless rule verify <id> --json. That command form does not exist:taskless rulehas four subcommands (create/improve/meta/delete), andverifyis a top-level, path-addressed command — deliberately so, since an id can name a rule under two engines and a path cannot (resolve-path.ts:25).Digging past the surface-level fix, the real history is stranger than "wrong command name":
rule verify <id>was removed entirely inc4dc03d(Aug 13) when rule addressing moved from id to path.@taskless/cli/schemas(the entry these docstrings live in) was created a month later, in189afa5(Sep 4), specifically to publish the realverify --jsonenvelope for the Cloud eval team (issue #263). That commit re-exportedverifyOutputSchema/valeVerifyOutputSchemafromrules-verify.tsalongside the correctverifyTestOutputSchema, without checking whether the command they described was still current. It wasn't.So the docstrings weren't just misnamed — the shape they describe is not emitted by any current CLI command.
taskless verify --json/taskless test --jsonprintverifyTestOutputSchema's flattened{ ok, rules: [{ errors, violations, ... }] }envelope.verifyOutputSchema/valeVerifyOutputSchemadescribe the pre-flattening per-layer detail (schema/requirements/testsfor ast-grep;fixtures/missingFailures/unexpectedFindingsfor Vale) thatverifyRule()/verifyValeRule()compute internally, and that the removedrule verify <id>command used to print verbatim as{ engine: "sg", ...verifyRule(...) }(confirmed by reading that command's source before its removal).Docstrings on both exports (and the module-adjacent comment block in
rules-verify.ts) are corrected to say this accurately, rather than naming a command form — old or new — that doesn't correspond to real output. Also fixed a second stale reference:ast-grep-rule.tscredited the embedded JSON Schema torule verify --schema, a flag removed CLI-wide back in May.Two open questions from the issue, investigated and answered
1. Are
getSchemaPayload/schemaOutputSchemaorphaned?schemaOutputSchema(inschemas/rules-verify.ts, not published): confirmed genuinely unused. Nothing insrcortestimports it, and the package'sexportsmap inpackage.jsonrestricts subpath resolution to declared entries only (.,./prompts,./layout,./schemas,./node/runtimes,./reference.json), so no external consumer can reach it either. I'm leaving it in place rather than removing it in this PR — it's adjacent cleanup, not what the issue asks for, and I'd rather flag it than fold in an unrelated deletion.getSchemaPayload(insrc/rules/verify.ts, outside this PR's file scope — owned by another agent's PR territory): the--schemaCLI flag that once called it was removed in May (commit7813546), whose message says it was kept "since the recipe will reuse the payload content." I checked: nothing insrc/helporsrc/promptsreferencesastGrepSchema/tasklessRequirements, so that reuse hasn't happened four months later. However it's still directly imported and exercised bytest/verify.test.ts, which is outside my assigned files. I did not establish it's safe to remove, so per the guardrail in.conventions/STYLEGUIDE-CODE.mdI'm leaving it and flagging it here instead.2. Is the
rule create/improvevsverify/testpublication asymmetry deliberate?Not deliberate in the sense of a documented policy — it tracked one consumer's specific ask.
189afa5's own commit message says the entry exists because "the Cloud eval team is doing exactly that today against theverifyenvelope" (#263). Nothing in that commit or since asked to publishcreate/improve's shapes. I'm not expanding this PR to publish new export paths — that's a surface change with its own tradeoffs (what's the compatibility commitment, doesnoticesstay optional forever, etc.) and deserves its own proposal, not a rider on a docstring fix.Tests
test/schemas-export.test.tsonly name-checked the export list and exercisedverifyTestOutputSchemaagainst real CLI output — nothing exercisedverifyOutputSchema/valeVerifyOutputSchema, which is exactly why the command-form mismatch went unnoticed for a month.Given the finding above (no command emits this shape any more), I can't pin it the way
verifyTestOutputSchemais pinned — spawning the built CLI and parsing stdout. The closest real pin: reconstruct the exact envelope the removed command built ({ engine: "sg", ...verifyRule(...) }, and thepassed→successmapping for Vale) from the real return values ofverifyRule()/verifyValeRule()(imported directly from source, since they're intentionally not part of the public surface — see the module docstring's "What this is NOT" section), and parse it with the built schema. This fails if either function's return shape ever drifts from what's published, which is the property that matters.Mutation-checked both new assertions:
verifyOutputSchema'sruleIdfield toid→ the new sg test failed with a real Zod error (invalid_typeat path["id"]). Reverted → passes again.valeVerifyOutputSchema'ssuccessmapping back to the internalpassedfield name → the new vale test failed (invalid_typeat path["success"]). Reverted → passes again.Full suite:
pnpm typecheck(clean),pnpm test(85 files / 1372 tests, all pass, including the vale-gated test since a vale binary is present in this environment),pnpm lint(clean, including thetaskless checkhouse-style pass over the rebuilt CLI: "No issues found").Changeset
patch. No runtime behavior changes — the schemas themselves are byte-identical, only docstrings and one internal comment changed, plus tests. It's apatchrather than no-release-note because these docstrings ship inside@taskless/cli/schemas, the artifact this repo's own convention says a consumer reads via editor tooltips or generated docs; a consumer following the old docstring would be pointed at a command that doesn't exist.Scope note
I stayed within my assigned files (
packages/cli/src/schemas/andpackages/cli/test/schemas-export.test.ts) and did not touchpackages/cli/src/commands/rules.ts,test/error-envelope.test.ts, ortest/verify-test-commands.test.ts, which other agents own.Fixes #283