Skip to content

Fix ztd-cli dogfooding guidance and scaffold stability - #620

Merged
mk3008 merged 4 commits into
mainfrom
codex/dogfood-remediation-phase1
Mar 19, 2026
Merged

Fix ztd-cli dogfooding guidance and scaffold stability#620
mk3008 merged 4 commits into
mainfrom
codex/dogfood-remediation-phase1

Conversation

@mk3008

@mk3008 mk3008 commented Mar 19, 2026

Copy link
Copy Markdown
Owner

Summary

  • Hardened ztd-cli scaffold install behavior for consumer and local-source paths.
  • Reworked template samples so QuerySpec and fixture-backed ZTD tests are runnable without the broken published testkit stack.
  • Updated Getting Started With AI guidance in both scaffold READMEs.
  • Increased ztd-cli Vitest timeout so the expensive init command tests complete reliably.

Verification

  • pnpm --filter @rawsql-ts/ztd-cli test -- tests/init.command.test.ts -u

Summary by CodeRabbit

  • New Features

    • Added --skip-install to init with improved install control and next-step messaging.
    • Core package now exposes additional utility modules.
  • Tests

    • Test templates now include fixture-backed test clients; updated smoke and queryspec examples and related assertions.
    • Increased Vitest per-test timeout for CLI tests.
  • Documentation

    • Revised template READMEs with clearer boundaries, project layout, AI guidance, workflow, and failure handling.
  • Chores

    • Replaced workspace dependency links with explicit semver ranges across packages.

@coderabbitai

coderabbitai Bot commented Mar 19, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e4cb3642-fd14-4c92-918f-ac9208a9cd7a

📥 Commits

Reviewing files that changed from the base of the PR and between 55dd560 and dcf641a.

📒 Files selected for processing (1)
  • packages/ztd-cli/templates/tests/queryspec.example.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/ztd-cli/templates/tests/queryspec.example.test.ts

📝 Walkthrough

Walkthrough

Replace workspace dependency links with explicit semver ranges across several packages; re-export two modules from core; add --skip-install and npm workspace detection to ztd-cli init; and replace placeholder testkit-client templates with fixture-backed clients plus updated tests and READMEs.

Changes

Cohort / File(s) Summary
Workspace dependency migration
packages/adapters/adapter-node-pg/package.json, packages/testkit-core/package.json, packages/testkit-postgres/package.json, packages/ztd-cli/package.json
Replace workspace:* / workspace:^ specifiers with explicit caret semver ranges for internal @rawsql-ts/* and rawsql-ts.
Core public API expansion
packages/core/src/index.ts
Add re-exports: ./utils/ParameterHelper, ./transformers/SqlParameterBinder.
CLI init command feature & logic
packages/ztd-cli/src/commands/init.ts
Add skipInstall?: boolean / --skip-install; detect ancestor npm workspace (findAncestorWorkspaceRoot, resolveNpmWorkspaceGuard); pass --workspaces=false for nested installs; make install optional and return install notes instead of throwing; tighten template package regex; update next-step messaging and STACK_DEV_DEPENDENCIES.
Template documentation
packages/ztd-cli/templates/README.md, packages/ztd-cli/templates/README.webapi.md, packages/ztd-cli/templates/src/infrastructure/persistence/README.md
Rewrite and expand README content: clarify boundaries, add "Project layout" and "Getting Started with AI", revise workflow and failure guidance, and clarify schema-change impact checks.
Test support fixtures & client
packages/ztd-cli/templates/tests/support/testkit-client.ts, packages/ztd-cli/templates/tests/support/testkit-client.webapi.ts
Replace placeholder createTestkitClient() with fixture-backed implementation; add exported TestkitClient type, TableFixture interface, and tableFixture() helper; implement constrained SQL parsing, parameter resolution, row filtering/projection, and close() no-op.
Example & smoke tests
packages/ztd-cli/templates/tests/queryspec.example.test.ts, packages/ztd-cli/templates/tests/smoke.test.ts
Convert QuerySpec test to local QuerySpec type + async executor flow; add fixture-backed smoke test using tableFixture; update assertions to reflect fixture-backed behavior.
Init command tests & expectations
packages/ztd-cli/tests/init.command.test.ts, packages/ztd-cli/tests/directoryFinding.docs.test.ts, packages/ztd-cli/tests/sqlDebugDogfooding.cli.test.ts
Update tests to match scaffold content changes (removed @rawsql-ts/sql-contract, fixture-backed guidance, local-source expectations); relax README matching for query-unit guidance; add per-test timeout where set.
Test configuration
packages/ztd-cli/vitest.config.ts
Set testTimeout: 60_000 for ztd-cli vitest configuration.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant CLI
    participant FSys as File System
    participant NPM as NPM Workspace
    participant Installer as Dependency Installer

    User->>CLI: run `ztd init` [--skip-install]
    CLI->>FSys: generate scaffold files
    FSys-->>CLI: scaffold created
    alt skipInstall true
        CLI->>User: show install instruction note
    else
        CLI->>NPM: detect ancestor workspace (resolveNpmWorkspaceGuard)
        NPM-->>CLI: workspace info or null
        alt nested workspace detected
            CLI->>Installer: run `npm install --workspaces=false`
        else
            CLI->>Installer: run `npm install`
        end
        Installer-->>CLI: success or failure
        alt failure
            CLI->>User: "Dependency install failed, but scaffold created" + note
        else
            CLI->>User: include install step in next-steps
        end
    end
    CLI->>User: display final next-steps summary
Loading
sequenceDiagram
    participant Test
    participant Executor as Testkit Executor
    participant Parser as Query Parser
    participant Fixtures as Fixture Store
    participant Projector as Result Projector

    Test->>Executor: createTestkitClient(fixtures)
    Executor-->>Test: TestkitClient (query, close)
    Test->>Executor: client.query(sql, params)
    Executor->>Parser: parse SQL (table, columns, where)
    Parser-->>Executor: parsed info
    Executor->>Fixtures: find fixture rows for table
    Fixtures-->>Executor: fixture rows
    Executor->>Executor: resolve $n params and filter rows
    Executor->>Projector: project requested columns
    Projector-->>Executor: result rows
    Executor-->>Test: return rows
    Test->>Executor: close()
    Executor-->>Test: resolved
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Poem

🐰✨ I nibble semver, tidy the nest,
Scaffolds sprout where workspaces rest.
Fixtures hum and tests take flight,
Skip or install — the CLI’s polite.
Happy hops for exports new and bright.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 9.09% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Fix ztd-cli dogfooding guidance and scaffold stability' directly and clearly describes the main changes in the PR: hardening ztd-cli scaffold behavior, reworking template samples for testkit, updating guidance documentation, and improving test reliability.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/dogfood-remediation-phase1
📝 Coding Plan
  • Generate coding plan for human review comments

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 8

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/templates/tests/queryspec.example.test.ts (1)

48-54: ⚠️ Potential issue | 🟠 Major

Keep the mocked SQL consistent with the declared params.

listActiveUsersSpec declares example: [], and Line 101 calls executor.list(listActiveUsersSpec, []), but Line 87 still returns SQL with $1. That query unit is internally inconsistent and cannot be copied into a real repository without changing either the SQL or the params contract.

Based on learnings The first SQL-backed repository should be mirrored in tests/queryspec.example.test.ts so SQL, QuerySpec, and tests stay aligned.

Also applies to: 83-109

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/ztd-cli/templates/tests/queryspec.example.test.ts` around lines 48 -
54, The mocked SQL returned for listActiveUsersSpec is inconsistent with its
declared params: listActiveUsersSpec sets example: [] and
executor.list(listActiveUsersSpec, []) is called, but the mocked SQL still
contains a $1 parameter; update the test fixture so the SQL, QuerySpec, and
invocation match by either removing the $1 from the mocked SQL (make it a
no-parameter query) or changing listActiveUsersSpec.params.example and the
executor.list call to supply a positional parameter; locate listActiveUsersSpec,
the mocked SQL string (the SQL returned on line ~87), and the executor.list
invocation to make the change so all three (QuerySpec, SQL, and test call) are
consistent.
🧹 Nitpick comments (1)
packages/ztd-cli/templates/tests/support/testkit-client.webapi.ts (1)

3-128: Consider sharing this helper with the default template variant.

Aside from the SqlClient import path, this is the same fixture parser/executor as packages/ztd-cli/templates/tests/support/testkit-client.ts. Pulling the shared logic into one template helper would keep future fixes and supported SQL shapes from drifting between the default and WebAPI scaffolds.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/ztd-cli/templates/tests/support/testkit-client.webapi.ts` around
lines 3 - 128, This file duplicates the fixture parser/executor found in the
default template; extract the shared logic — TestkitClient type, tableFixture,
parseSelectQuery, normalizeIdentifier, resolveTableRows, resolveWhereValue,
projectRows, filterRows and the core createTestkitClient implementation — into a
single shared helper module and have both the WebAPI variant and the default
template import and adapt that helper (keeping only the WebAPI-specific
SqlClient import/adapter in this file). Ensure the WebAPI file delegates to or
re-exports the shared createTestkitClient so future fixes to SQL
parsing/fixtures are applied in one place.
🤖 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/adapters/adapter-node-pg/package.json`:
- Around line 35-39: The package.json dependency entries (e.g., "pg",
"rawsql-ts", "@rawsql-ts/testkit-postgres", "@rawsql-ts/testkit-core",
"@rawsql-ts/shared-binder") were manually changed from workspace:^ to fixed
semver—revert those direct edits in package.json back to the workspace:^ entries
and do not commit manual version bumps; then run pnpm install from the repo root
to regenerate the lockfile so changes to the lock are consistent with the
workspace manifests, or if a version bump is required follow the release
workflow tooling instead of editing package.json directly.

In `@packages/testkit-core/package.json`:
- Line 41: The package version for "rawsql-ts" was edited directly in
package.json causing a lockfile mismatch with pnpm-lock.yaml; either revert the
package.json change back to the workspace specifier for "rawsql-ts" or, if the
version bump is intentional, run pnpm install to regenerate pnpm-lock.yaml so
the lockfile and manifest agree, then commit both the updated package.json and
the regenerated pnpm-lock.yaml (referencing the "rawsql-ts" manifest entry and
pnpm-lock.yaml); do not directly edit package.json without updating the
lockfile.

In `@packages/testkit-postgres/package.json`:
- Around line 35-37: You edited dependency versions directly in
packages/testkit-postgres/package.json for "@rawsql-ts/sql-contract",
"@rawsql-ts/testkit-core", and "rawsql-ts", which requires synchronizing the
lockfile; revert manual version bumps (restore workspace:^ entries if they were
changed) or follow the release workflow, then run pnpm install to regenerate
pnpm-lock.yaml and commit the updated lockfile so the lock matches package.json;
do not manually edit package.json versions outside the release/publish process.

In `@packages/ztd-cli/src/commands/init.ts`:
- Around line 251-252: The early return used when the --skip-install flag is set
(and the install-failure fallback) skips the step that adds template-only
packages computed by ensurePackageJsonFormatting(), so package.json can be left
missing required template deps; update the --skip-install branch and the
install-error fallback to still call ensurePackageJsonFormatting() (or the same
logic that computes and injects missingPackages including template-only packages
like `@rawsql-ts/sql-contract`) and persist the updated package.json before
returning, and ensure the fallback note generation includes the exact "install
-D ..." command string so users can run the correct install command manually.

In `@packages/ztd-cli/templates/README.webapi.md`:
- Around line 73-77: Update the README reference that points to
tests/support/testkit-client.webapi.ts to the actual emitted helper filename
tests/support/testkit-client.ts (and adjust any other mentions of
testkit-client.webapi.ts in the README), so readers are directed to the file the
scaffold actually generates; keep the other example filenames
(tests/queryspec.example.test.ts, tests/smoke.test.ts) unchanged.

In `@packages/ztd-cli/templates/tests/queryspec.example.test.ts`:
- Around line 33-39: The list implementation in createCatalogExecutor currently
passes raw row objects from executor(...) directly into spec.output.validate,
causing DTO fields to be undefined; update the logic in createCatalogExecutor ->
list to first apply spec.output.mapping (if present) to each raw row to produce
the transformed DTO, then run spec.output.validate (if present) on that mapped
object before returning; keep using loader.load(spec.sqlFile) and executor(sql,
params) as-is but replace the rows.map callback to perform mapping then
validation in that order.
- Around line 3-31: Replace the local QuerySpec type and rowMapping shim with
the real exports from `@rawsql-ts/sql-contract`: import QuerySpec and RowMapping
(and any helper rowMapping function) and use those instead of the inline
definitions; ensure the imported QuerySpec uses params.shape that accepts
'positional' | 'named', makes output.mapping optional and typed as
RowMapping<R>, and marks notes as optional, and make rowMapping return a
RowMapping instance rather than a plain object; keep CatalogExecutorOptions but
switch any mapping usage to the imported RowMapping type to stay aligned with
the library API.

In `@packages/ztd-cli/templates/tests/support/testkit-client.ts`:
- Around line 47-63: In resolveWhereValue, ensure positional parameter
references actually exist: after computing index = Number(whereValue.slice(1))
and validating it, check that values is an array and that values.length >=
index; if the slot is missing (index-1 out of bounds) throw a clear Error (e.g.,
`Missing positional parameter for ${whereValue}`) instead of returning
undefined; update resolveWhereValue to perform this bounds check before
returning values[index - 1].

---

Outside diff comments:
In `@packages/ztd-cli/templates/tests/queryspec.example.test.ts`:
- Around line 48-54: The mocked SQL returned for listActiveUsersSpec is
inconsistent with its declared params: listActiveUsersSpec sets example: [] and
executor.list(listActiveUsersSpec, []) is called, but the mocked SQL still
contains a $1 parameter; update the test fixture so the SQL, QuerySpec, and
invocation match by either removing the $1 from the mocked SQL (make it a
no-parameter query) or changing listActiveUsersSpec.params.example and the
executor.list call to supply a positional parameter; locate listActiveUsersSpec,
the mocked SQL string (the SQL returned on line ~87), and the executor.list
invocation to make the change so all three (QuerySpec, SQL, and test call) are
consistent.

---

Nitpick comments:
In `@packages/ztd-cli/templates/tests/support/testkit-client.webapi.ts`:
- Around line 3-128: This file duplicates the fixture parser/executor found in
the default template; extract the shared logic — TestkitClient type,
tableFixture, parseSelectQuery, normalizeIdentifier, resolveTableRows,
resolveWhereValue, projectRows, filterRows and the core createTestkitClient
implementation — into a single shared helper module and have both the WebAPI
variant and the default template import and adapt that helper (keeping only the
WebAPI-specific SqlClient import/adapter in this file). Ensure the WebAPI file
delegates to or re-exports the shared createTestkitClient so future fixes to SQL
parsing/fixtures are applied in one place.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0f4911f6-a3a7-490b-b628-fcbd96872ae7

📥 Commits

Reviewing files that changed from the base of the PR and between ccc7e18 and 2ab5c25.

⛔ Files ignored due to path filters (1)
  • packages/ztd-cli/tests/__snapshots__/init.command.test.ts.snap is excluded by !**/*.snap
📒 Files selected for processing (15)
  • packages/adapters/adapter-node-pg/package.json
  • packages/core/src/index.ts
  • packages/testkit-core/package.json
  • packages/testkit-postgres/package.json
  • packages/ztd-cli/package.json
  • packages/ztd-cli/src/commands/init.ts
  • packages/ztd-cli/templates/README.md
  • packages/ztd-cli/templates/README.webapi.md
  • packages/ztd-cli/templates/src/infrastructure/persistence/README.md
  • packages/ztd-cli/templates/tests/queryspec.example.test.ts
  • packages/ztd-cli/templates/tests/smoke.test.ts
  • packages/ztd-cli/templates/tests/support/testkit-client.ts
  • packages/ztd-cli/templates/tests/support/testkit-client.webapi.ts
  • packages/ztd-cli/tests/init.command.test.ts
  • packages/ztd-cli/vitest.config.ts

Comment on lines +35 to +39
"@rawsql-ts/testkit-postgres": "^0.15.3",
"@rawsql-ts/testkit-core": "^0.16.1",
"@rawsql-ts/shared-binder": "^0.0.1",
"pg": "^8.13.1",
"rawsql-ts": "workspace:^"
"rawsql-ts": "^0.17.0"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Same lockfile synchronization concern as other package manifests.

These workspace:^ → semver version changes follow the same pattern across the PR. Ensure pnpm install regenerates the lockfile after all manifest changes are complete.

As per coding guidelines, direct version edits are prohibited outside of release workflows.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/adapters/adapter-node-pg/package.json` around lines 35 - 39, The
package.json dependency entries (e.g., "pg", "rawsql-ts",
"@rawsql-ts/testkit-postgres", "@rawsql-ts/testkit-core",
"@rawsql-ts/shared-binder") were manually changed from workspace:^ to fixed
semver—revert those direct edits in package.json back to the workspace:^ entries
and do not commit manual version bumps; then run pnpm install from the repo root
to regenerate the lockfile so changes to the lock are consistent with the
workspace manifests, or if a version bump is required follow the release
workflow tooling instead of editing package.json directly.

Comment thread packages/testkit-core/package.json
Comment on lines +35 to +37
"@rawsql-ts/sql-contract": "^0.3.1",
"@rawsql-ts/testkit-core": "^0.16.1",
"rawsql-ts": "^0.17.0"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Direct version edits require lockfile synchronization.

Same pattern as testkit-core/package.json — these workspace:^ → semver changes require the lockfile to be regenerated. Ensure pnpm install is run after these changes to update pnpm-lock.yaml.

As per coding guidelines, editing package versions directly in package.json is prohibited unless this is part of a release/publish workflow.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/testkit-postgres/package.json` around lines 35 - 37, You edited
dependency versions directly in packages/testkit-postgres/package.json for
"@rawsql-ts/sql-contract", "@rawsql-ts/testkit-core", and "rawsql-ts", which
requires synchronizing the lockfile; revert manual version bumps (restore
workspace:^ entries if they were changed) or follow the release workflow, then
run pnpm install to regenerate pnpm-lock.yaml and commit the updated lockfile so
the lock matches package.json; do not manually edit package.json versions
outside the release/publish process.

Comment on lines +251 to 252
'@rawsql-ts/ztd-cli': '^0.20.3',
};

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

--skip-install currently skips declaring template deps, not just installing them.

ensurePackageJsonFormatting() no longer records template-only packages like @rawsql-ts/sql-contract; this function adds them only after it computes missingPackages. The early return on Lines 1526-1529 skips that step entirely, so a scaffold created with --skip-install can tell the user to run a plain install even though package.json is still missing required imports. The same problem shows up on the install-failure path, where the fallback note loses the exact install -D ... command.

Based on learnings Template changes MUST preserve an executable pnpm test path for initialized projects.

Also applies to: 1526-1575

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/ztd-cli/src/commands/init.ts` around lines 251 - 252, The early
return used when the --skip-install flag is set (and the install-failure
fallback) skips the step that adds template-only packages computed by
ensurePackageJsonFormatting(), so package.json can be left missing required
template deps; update the --skip-install branch and the install-error fallback
to still call ensurePackageJsonFormatting() (or the same logic that computes and
injects missingPackages including template-only packages like
`@rawsql-ts/sql-contract`) and persist the updated package.json before returning,
and ensure the fallback note generation includes the exact "install -D ..."
command string so users can run the correct install command manually.

Comment on lines +73 to +77
Good example files to start from:

* `tests/queryspec.example.test.ts` for the `QuerySpec + CatalogExecutor` path
* `tests/smoke.test.ts` for the fixture-backed ZTD rewrite path
* `tests/support/testkit-client.webapi.ts` for the testkit client helper

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Point readers at the emitted helper filename.

The WebAPI scaffold still writes this helper as tests/support/testkit-client.ts; tests/support/testkit-client.webapi.ts only exists in the template source. The current docs will send scaffold users to a file that is not generated.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/ztd-cli/templates/README.webapi.md` around lines 73 - 77, Update the
README reference that points to tests/support/testkit-client.webapi.ts to the
actual emitted helper filename tests/support/testkit-client.ts (and adjust any
other mentions of testkit-client.webapi.ts in the README), so readers are
directed to the file the scaffold actually generates; keep the other example
filenames (tests/queryspec.example.test.ts, tests/smoke.test.ts) unchanged.

Comment on lines +3 to +31
type QuerySpec<TParams extends readonly unknown[], TRow> = {
id: string;
sqlFile: string;
params: {
shape: 'positional';
example: TParams;
};
output: {
mapping: {
name: string;
key: string;
columnMap: Record<string, string>;
};
validate?: (value: unknown) => TRow;
example: TRow;
};
notes: string;
};

type CatalogExecutorOptions = {
loader: {
load(sqlFile: string): Promise<string>;
};
executor: (sql: string, params: readonly unknown[]) => Promise<Record<string, unknown>[]>;
};

function rowMapping(mapping: { name: string; key: string; columnMap: Record<string, string> }) {
return mapping;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
sed -n '35,60p' packages/sql-contract/src/catalog/index.ts
sed -n '426,431p' packages/sql-contract/src/mapper/index.ts

Repository: mk3008/rawsql-ts

Length of output: 831


Replace this local QuerySpec and rowMapping shim with imports from @rawsql-ts/sql-contract.

The template defines a local type that diverges from the actual exported API:

  • mapping should be optional and typed as RowMapping<R>, not a required plain object
  • params.shape should accept both 'positional' | 'named', not just 'positional'
  • notes should be optional, not required
  • rowMapping() should return a RowMapping instance, not the plain object

Since this file serves as the copyable QuerySpec-first sample, users will create code that misaligns with the actual library. Import the real types/helpers instead.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/ztd-cli/templates/tests/queryspec.example.test.ts` around lines 3 -
31, Replace the local QuerySpec type and rowMapping shim with the real exports
from `@rawsql-ts/sql-contract`: import QuerySpec and RowMapping (and any helper
rowMapping function) and use those instead of the inline definitions; ensure the
imported QuerySpec uses params.shape that accepts 'positional' | 'named', makes
output.mapping optional and typed as RowMapping<R>, and marks notes as optional,
and make rowMapping return a RowMapping instance rather than a plain object;
keep CatalogExecutorOptions but switch any mapping usage to the imported
RowMapping type to stay aligned with the library API.

Comment on lines +33 to +39
function createCatalogExecutor({ loader, executor }: CatalogExecutorOptions) {
return {
async list<TParams extends readonly unknown[], TRow>(spec: QuerySpec<TParams, TRow>, params: TParams) {
const sql = await loader.load(spec.sqlFile);
const rows = await executor(sql, params);
return rows.map((value) => (spec.output.validate ? spec.output.validate(value) : (value as TRow)));
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Apply output.mapping before validate().

Line 38 passes the raw { user_id, display_name } row straight into validate(). With the current validator, that yields userId: 'undefined' / displayName: 'undefined', so the sample fails once the scaffold test actually runs.

Suggested fix
 function createCatalogExecutor({ loader, executor }: CatalogExecutorOptions) {
   return {
     async list<TParams extends readonly unknown[], TRow>(spec: QuerySpec<TParams, TRow>, params: TParams) {
       const sql = await loader.load(spec.sqlFile);
       const rows = await executor(sql, params);
-      return rows.map((value) => (spec.output.validate ? spec.output.validate(value) : (value as TRow)));
+      return rows.map((value) => {
+        const mapped = Object.fromEntries(
+          Object.entries(spec.output.mapping.columnMap).map(([field, column]) => [field, value[column]])
+        );
+        return spec.output.validate ? spec.output.validate(mapped) : (mapped as TRow);
+      });
     }
   };
 }
As per coding guidelines `packages/ztd-cli/templates/tests/**/*.test.ts`: Tests MUST verify SQL execution under ZTD rewrite, mapping behavior, validation paths, and DTO shape semantics.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/ztd-cli/templates/tests/queryspec.example.test.ts` around lines 33 -
39, The list implementation in createCatalogExecutor currently passes raw row
objects from executor(...) directly into spec.output.validate, causing DTO
fields to be undefined; update the logic in createCatalogExecutor -> list to
first apply spec.output.mapping (if present) to each raw row to produce the
transformed DTO, then run spec.output.validate (if present) on that mapped
object before returning; keep using loader.load(spec.sqlFile) and executor(sql,
params) as-is but replace the rows.map callback to perform mapping then
validation in that order.

Comment on lines +47 to +63
function resolveWhereValue(whereValue: string | undefined, values?: readonly unknown[] | Record<string, unknown>): unknown {
if (!whereValue) {
return undefined;
}

if (whereValue.startsWith('$')) {
const index = Number(whereValue.slice(1));
if (!Number.isFinite(index) || index < 1) {
throw new Error(`Unsupported parameter reference: ${whereValue}`);
}

if (!Array.isArray(values)) {
throw new Error('Positional parameters are required for fixture-backed queries.');
}

return values[index - 1];
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Throw when a referenced bind slot is missing.

Line 62 returns values[index - 1] without checking that the slot exists. A query like ... where id = $2 with only one value silently becomes undefined and filters to zero rows, which hides a broken contract in the generated tests.

Suggested fix
   if (whereValue.startsWith('$')) {
     const index = Number(whereValue.slice(1));
     if (!Number.isFinite(index) || index < 1) {
       throw new Error(`Unsupported parameter reference: ${whereValue}`);
     }

     if (!Array.isArray(values)) {
       throw new Error('Positional parameters are required for fixture-backed queries.');
     }
+    if (index > values.length) {
+      throw new Error(`Missing value for parameter reference: ${whereValue}`);
+    }

     return values[index - 1];
   }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/ztd-cli/templates/tests/support/testkit-client.ts` around lines 47 -
63, In resolveWhereValue, ensure positional parameter references actually exist:
after computing index = Number(whereValue.slice(1)) and validating it, check
that values is an array and that values.length >= index; if the slot is missing
(index-1 out of bounds) throw a clear Error (e.g., `Missing positional parameter
for ${whereValue}`) instead of returning undefined; update resolveWhereValue to
perform this bounds check before returning values[index - 1].

@mk3008
mk3008 merged commit 1f5539f into main Mar 19, 2026
9 checks passed
@mk3008
mk3008 deleted the codex/dogfood-remediation-phase1 branch March 19, 2026 16:15
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.

1 participant