fix(ztd-cli): wire traditional QuerySpec runner#793
Merged
mk3008 merged 3 commits intoApr 29, 2026
Merged
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughThis PR implements the "traditional" test execution mode for ztd-cli's query boundary tests, enabling physical database schema setup and fixture seeding as an alternative to CTE-based fixture rewriting. The traditional runner accepts case inputs matching ZTD shape, optionally supports post-state assertions via Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Vitest Test
participant Harness as runQuerySpecTraditionalCases
participant Verifier as verifyQuerySpecTraditionalCase
participant Executor as PhysicalQuerySpecExecutorClient
participant DB as Postgres DB
Test->>Harness: call(cases, executor)
Harness->>Verifier: for each case
Verifier->>DB: CREATE TEMP SCHEMA
Verifier->>DB: LOAD starter DDL
Verifier->>DB: INSERT fixture rows
Verifier->>Executor: initialize with temp schema
Executor->>DB: EXECUTE query (rewritten to temp schema)
Executor->>DB: RECORD trace entries
alt case.afterDb provided
Executor->>DB: assertAfterDb (table/row matching)
end
Verifier->>DB: DROP TEMP SCHEMA
Verifier->>Harness: return Evidence (mode: 'traditional', physicalSetupUsed: true)
Harness->>Test: []QuerySpecExecutionEvidence
Test->>Test: assert mode === 'traditional' && physicalSetupUsed === true
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 27 minutes and 16 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ztd-cli/src/commands/featureTests.ts (1)
1-1:⚠️ Potential issue | 🟠 MajorPipeline failure: Missing "## CLI Surface Migration" section.
The PR check failed with:
Missing "## CLI Surface Migration" section for a CLI-facing change.This is a required merge gate per the pipeline output. The PR modifies--test-kind traditionalbehavior from placeholder to active execution, which qualifies as a CLI surface change requiring documentation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ztd-cli/src/commands/featureTests.ts` at line 1, The PR is missing the required "## CLI Surface Migration" section for a CLI-facing change; add that section to the PR description (or changelog) documenting the behavioral change to --test-kind traditional introduced by edits in packages/ztd-cli/src/commands/featureTests.ts, explain the new active execution behavior (previously a placeholder), list migration steps or opt-in/opt-out flags, show example CLI invocations that demonstrate the new default and any flags affected (e.g., --test-kind traditional), and call out any backward-compatibility considerations so the pipeline gate recognizes the change as documented.
🧹 Nitpick comments (4)
packages/ztd-cli/templates/tests/support/ztd/verifier.ts (1)
269-334: Verify resource cleanup on partial setup failure.In
createPhysicalQuerySpecExecutor, if an error occurs during DDL application or fixture seeding (lines 280-283), the catch block drops the schema, releases client, and ends pool. However, ifdropPhysicalSchemaitself fails,client.release()may not execute.Consider wrapping cleanup in try-finally:
🔒 Proposed hardening for cleanup
} catch (error) { - await dropPhysicalSchema(client, schemaName); - client.release(); - await pool.end(); + try { + await dropPhysicalSchema(client, schemaName); + } finally { + client.release(); + await pool.end(); + } throw error; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ztd-cli/templates/tests/support/ztd/verifier.ts` around lines 269 - 334, The catch block in createPhysicalQuerySpecExecutor can fail to release resources if dropPhysicalSchema throws; update the catch to ensure client.release() and pool.end() always run by wrapping the dropPhysicalSchema call in its own try/catch or using try { await dropPhysicalSchema(...); } finally { client.release(); await pool.end(); } then rethrow the original error; reference the catch inside createPhysicalQuerySpecExecutor and the cleanup helpers dropPhysicalSchema, client.release, and pool.end to locate where to apply this change.packages/ztd-cli/templates/README.md (1)
67-67: Consider breaking the AI prompt paragraph for readability.Line 67 packs substantial information (scaffold command, test-kind options, evidence expectations, entrypoint naming) into a single dense paragraph. While AI prompts benefit from completeness, this could be restructured with line breaks or bullet points to improve human scannability when reviewing or modifying the prompt.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ztd-cli/templates/README.md` at line 67, The README paragraph around the "ztd feature tests scaffold" command is overly dense; split it into clearer lines or bullets so each concept is scannable — separate the scaffold command invocation (`ztd feature tests scaffold --feature <feature-name>`), what files are generated or kept (`src/features/.../tests/generated/TEST_PLAN.md`, `analysis.json`, `generated/*`, thin entrypoint `*.boundary.ztd.test.ts`, persistent `cases/`), the `--test-kind traditional` guidance and when to use it, where to get fixture metadata (`.ztd/generated/ztd-fixture-manifest.generated.ts`) and what `beforeDb` returns, the evidence fields (`mode`, `rewriteApplied`, `physicalSetupUsed`) and expected values for ZTD vs traditional runs, and the optional SQL trace env vars (`ZTD_SQL_TRACE`, `ZTD_SQL_TRACE_DIR`) plus how to run the final entrypoint (`npx vitest run`); reorganize into short bullets or separate sentences so each symbol/path/option from the paragraph is its own readable item.packages/ztd-cli/tests/featureTestsScaffold.unit.test.ts (1)
432-454:afterDbの生成ケース実体にも直接アサートを追加するとさらに堅くなります。型・TEST_PLAN 文言は検証されていますが、
cases/basic.traditional.case.ts側の実体にafterDbが入ることを直接固定しておくと、テンプレート退行をより早く検知できます。Diff suggestion
const traditionalTypes = readFileSync( path.join(featureDir, 'queries', 'insert-users', 'tests', 'boundary-traditional-types.ts'), 'utf8' ); expect(traditionalTypes).toContain("import type { QuerySpecTraditionalCase } from '../../../../../../tests/support/ztd/case-types.js';"); expect(traditionalTypes).toContain('export type InsertUsersQueryBoundaryTraditionalCase = QuerySpecTraditionalCase<'); + + const traditionalCaseFile = readFileSync( + path.join(featureDir, 'queries', 'insert-users', 'tests', 'cases', 'basic.traditional.case.ts'), + 'utf8' + ); + expect(traditionalCaseFile).toContain('afterDb');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ztd-cli/tests/featureTestsScaffold.unit.test.ts` around lines 432 - 454, Add an assertion that the generated case file (cases/basic.traditional.case.ts) actually contains the afterDb callback to catch template regressions: read the file at path.join(featureDir, 'queries', 'insert-users', 'tests', 'cases', 'basic.traditional.case.ts') and expect its contents toContain a clear afterDb marker (e.g. "afterDb" or the exact snippet used by the template), similar to the existing checks for boundary-traditional-types.ts, analysis.traditional.json and TEST_PLAN.traditional.md, so the test fails if the template stops emitting afterDb.packages/ztd-cli/src/perf/sandbox.ts (1)
204-210: String matching onSqlParsererror messages is fragile, but requires upstream changes to improve.Line 209 uses
message.includes('[SqlParser] No SQL statements found in input.')to distinguish this specific error from other parse failures. While the concern about message-based matching is valid,SqlParser.parse()currently exposes only plainErrorobjects without stable error codes or custom error classes. No error type or code property is available for type-safe discrimination.To address this properly would require either:
- Contributing to
SqlParserto expose error codes or custom error classes (e.g.,class NoStatementsError extends Error { code = 'NO_SQL_STATEMENTS' })- Adding a validation method to
SqlParserlikehasStatements(sql: string): booleanFor now, consider adding a comment above the string check explaining why it's necessary as a workaround, and document the fragility. If this pattern becomes common across the codebase, it would be worth advocating for stable error handling in
SqlParser.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ztd-cli/src/perf/sandbox.ts` around lines 204 - 210, The current string-match on SqlParser.parse error messages in parseOptionalDdlStatement is fragile; add a short explanatory comment immediately above the check that documents why message.includes('[SqlParser] No SQL statements found in input.') is required as a temporary workaround (SqlParser only throws plain Error and has no error codes/classes or a hasStatements helper), note the fragility and recommend upstream fixes (error codes or custom error classes/have a hasStatements method) for future refactors, and leave the existing conditional behavior unchanged until SqlParser is improved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ztd-cli/README.md`:
- Around line 192-194: The PR is blocked by the check-pr-readiness required
check because CLI-facing changes (e.g., the new behavior described for `ztd
feature tests scaffold --test-kind traditional` in README.md) lack the mandated
PR checklist items; update the PR description to add the missing "## CLI Surface
Migration" and "## Scaffold Contract Proof" sections and mark the required
checkboxes (or otherwise satisfy the `check-pr-readiness` criteria) so the
pipeline passes and the PR can be merged.
---
Outside diff comments:
In `@packages/ztd-cli/src/commands/featureTests.ts`:
- Line 1: The PR is missing the required "## CLI Surface Migration" section for
a CLI-facing change; add that section to the PR description (or changelog)
documenting the behavioral change to --test-kind traditional introduced by edits
in packages/ztd-cli/src/commands/featureTests.ts, explain the new active
execution behavior (previously a placeholder), list migration steps or
opt-in/opt-out flags, show example CLI invocations that demonstrate the new
default and any flags affected (e.g., --test-kind traditional), and call out any
backward-compatibility considerations so the pipeline gate recognizes the change
as documented.
---
Nitpick comments:
In `@packages/ztd-cli/src/perf/sandbox.ts`:
- Around line 204-210: The current string-match on SqlParser.parse error
messages in parseOptionalDdlStatement is fragile; add a short explanatory
comment immediately above the check that documents why
message.includes('[SqlParser] No SQL statements found in input.') is required as
a temporary workaround (SqlParser only throws plain Error and has no error
codes/classes or a hasStatements helper), note the fragility and recommend
upstream fixes (error codes or custom error classes/have a hasStatements method)
for future refactors, and leave the existing conditional behavior unchanged
until SqlParser is improved.
In `@packages/ztd-cli/templates/README.md`:
- Line 67: The README paragraph around the "ztd feature tests scaffold" command
is overly dense; split it into clearer lines or bullets so each concept is
scannable — separate the scaffold command invocation (`ztd feature tests
scaffold --feature <feature-name>`), what files are generated or kept
(`src/features/.../tests/generated/TEST_PLAN.md`, `analysis.json`,
`generated/*`, thin entrypoint `*.boundary.ztd.test.ts`, persistent `cases/`),
the `--test-kind traditional` guidance and when to use it, where to get fixture
metadata (`.ztd/generated/ztd-fixture-manifest.generated.ts`) and what
`beforeDb` returns, the evidence fields (`mode`, `rewriteApplied`,
`physicalSetupUsed`) and expected values for ZTD vs traditional runs, and the
optional SQL trace env vars (`ZTD_SQL_TRACE`, `ZTD_SQL_TRACE_DIR`) plus how to
run the final entrypoint (`npx vitest run`); reorganize into short bullets or
separate sentences so each symbol/path/option from the paragraph is its own
readable item.
In `@packages/ztd-cli/templates/tests/support/ztd/verifier.ts`:
- Around line 269-334: The catch block in createPhysicalQuerySpecExecutor can
fail to release resources if dropPhysicalSchema throws; update the catch to
ensure client.release() and pool.end() always run by wrapping the
dropPhysicalSchema call in its own try/catch or using try { await
dropPhysicalSchema(...); } finally { client.release(); await pool.end(); } then
rethrow the original error; reference the catch inside
createPhysicalQuerySpecExecutor and the cleanup helpers dropPhysicalSchema,
client.release, and pool.end to locate where to apply this change.
In `@packages/ztd-cli/tests/featureTestsScaffold.unit.test.ts`:
- Around line 432-454: Add an assertion that the generated case file
(cases/basic.traditional.case.ts) actually contains the afterDb callback to
catch template regressions: read the file at path.join(featureDir, 'queries',
'insert-users', 'tests', 'cases', 'basic.traditional.case.ts') and expect its
contents toContain a clear afterDb marker (e.g. "afterDb" or the exact snippet
used by the template), similar to the existing checks for
boundary-traditional-types.ts, analysis.traditional.json and
TEST_PLAN.traditional.md, so the test fails if the template stops emitting
afterDb.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bceed848-5752-45c8-a471-1f0cb1a83665
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (20)
.changeset/traditional-runner-mode.md.codex/agents/reporting.md.codex/agents/review.md.codex/agents/verification.mdAGENTS.mdpackages/testkit-core/tests/SelectFixtureRewriter.test.tspackages/ztd-cli/README.mdpackages/ztd-cli/package.jsonpackages/ztd-cli/src/commands/featureTests.tspackages/ztd-cli/src/commands/testEvidence.tspackages/ztd-cli/src/perf/sandbox.tspackages/ztd-cli/templates/README.mdpackages/ztd-cli/templates/tests/support/ztd/README.mdpackages/ztd-cli/templates/tests/support/ztd/case-types.tspackages/ztd-cli/templates/tests/support/ztd/harness.tspackages/ztd-cli/templates/tests/support/ztd/verifier.tspackages/ztd-cli/tests/featureTestsScaffold.unit.test.tspackages/ztd-cli/tests/perfSandbox.unit.test.tspackages/ztd-cli/tests/testEvidence.unit.test.tspackages/ztd-cli/tests/ztdVerifier.unit.test.ts
Comment on lines
+192
to
+194
| `ztd feature tests scaffold --test-kind traditional` creates an active `.boundary.traditional.test.ts` entrypoint that calls the shared mode runner. | ||
| Traditional cases keep the same `beforeDb`, `input`, and `output` shape and can add optional `afterDb` assertions. | ||
| The traditional runner physically prepares DDL and fixture rows, then reports `mode=traditional` and `physicalSetupUsed=true`. |
There was a problem hiding this comment.
PR merge gate が未解消です（check-pr-readiness の required failure）。
Line 192 以降の CLI-facing 変更に対して、パイプライン上で ## CLI Surface Migration / ## Scaffold Contract Proof の欠落とチェックボックス未選択が報告されています。コード変更とは別に、PR本文の必須項目を満たさない限りマージ準備完了にはできません。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ztd-cli/README.md` around lines 192 - 194, The PR is blocked by the
check-pr-readiness required check because CLI-facing changes (e.g., the new
behavior described for `ztd feature tests scaffold --test-kind traditional` in
README.md) lack the mandated PR checklist items; update the PR description to
add the missing "## CLI Surface Migration" and "## Scaffold Contract Proof"
sections and mark the required checkboxes (or otherwise satisfy the
`check-pr-readiness` criteria) so the pipeline passes and the PR can be merged.
mk3008
deleted the
789-wire-ztd-cli-traditional-test-lane-to-a-supported-mode-switching-runner
branch
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
--test-kind traditionalQuerySpec lane to a supported physical runner instead of leaving it as placeholder-only scaffold.Customer Value
Users can generate and run traditional QuerySpec tests as a real supported mode-switching lane, alongside the existing ZTD rewritten lane. The scaffold now makes the traditional path reviewable and executable instead of requiring users to invent local harness glue.
Acceptance Criteria
ztd feature tests scaffold --test-kind traditionalgenerates an active traditional entrypoint.mode: "ztd"andmode: "traditional"execution.afterDbsupport.dotenvis fixed by adding the dependency where ztd-cli tests import starter template support.Additional Hardening
AGENTS.mdand the local review, verification, and reporting agents so required-check failures cannot be dismissed as unrelated solely because the stack trace points outside touched files.pnpm testfailures discovered by the pre-commit hook:afterEachexplicitly instead of relying on Vitest globals.Verification
pnpm --filter @rawsql-ts/ztd-cli test -- featureTestsScaffold.unit.test.ts ztdVerifier.unit.test.ts init.command.test.ts directoryFinding.docs.test.tspnpm --filter @rawsql-ts/ztd-cli buildpnpm --filter @rawsql-ts/ztd-cli test -- repoGuidance.unit.test.tspnpm --filter @rawsql-ts/ztd-cli testpnpm test -- packages/testkit-core/tests/SelectFixtureRewriter.test.ts packages/ztd-cli/tests/commandTelemetry.unit.test.ts packages/ztd-cli/tests/testEvidence.unit.test.ts packages/ztd-cli/tests/perfSandbox.unit.test.tspnpm --filter @rawsql-ts/ztd-cli test -- commandTelemetry.unit.test.ts perfSandbox.unit.test.tspnpm test -- packages/ztd-cli/tests/testEvidence.unit.test.tspnpm --filter @rawsql-ts/ztd-cli test -- testEvidence.unit.test.tseb89983cpnpm typecheckpnpm test(350 passed,3116 passed | 8 skipped)pnpm buildpnpm lintNote: package-level ztd-cli full test output still reports DB-dependent CLI tests skipped when
pg_dumpis not onPATH; the command exits successfully under the repository's existing skip behavior.Merge Readiness
Tracking issue: not needed; no baseline exception requested.
Scoped checks run: not needed; no baseline exception requested.
Why full baseline is not required: full baseline exception path not used for this PR.
Self Review
Self-review workflow: repo-local self-review skill and global self-review skill two-cycle passes completed before updating this PR.
Self-review result: no unresolved blockers; review nits addressed or intentionally scoped with CI-backed guardrails.
CLI Surface Migration
No-migration rationale: not selected for this PR.
Upgrade note:
ztd feature tests scaffold --test-kind traditionalnow emits an active traditional QuerySpec test entrypoint backed by isolated physical database setup.Deprecation/removal plan or issue: no deprecation or removal; this completes the existing traditional mode path requested in #789.
Docs/help/examples updated: ztd-cli README and template README files now describe traditional mode, physical setup, evidence, and optional
afterDbassertions.Release/changeset wording:
.changeset/traditional-runner-mode.mdrecords a patch release for the traditional QuerySpec runner wiring.Scaffold Contract Proof
No-proof rationale: not selected for this PR.
Non-edit assertion:
featureTestsScaffold.unit.test.tskeeps the scaffold assertions focused on generated query-local files and does not require parent boundary edits for the traditional lane.Fail-fast input-contract proof: existing scaffold input-contract coverage remains in
featureTestsScaffold.unit.test.ts, including invalid feature/query layout cases; this PR extends the generated mode contract without weakening those checks.Generated-output viability proof:
featureTestsScaffold.unit.test.tsasserts active traditional entrypoint imports, generated traditional case types,modeevidence, andphysicalSetupUsedevidence;ztdVerifier.unit.test.tsexercises the physical traditional runner and optionalafterDb.Open Questions
None.
Merge Blockers
None known.