feat(cli): refresh starter scaffold and docs#626
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR converts scaffolds, templates, and docs to a feature-first starter flow rooted at Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as rgba(52,160,237,0.5)
participant Templates as rgba(34,197,94,0.5)
participant Agents as rgba(234,88,12,0.5)
participant DB as rgba(124,58,237,0.5)
participant TestRunner as rgba(6,182,212,0.5)
User->>CLI: run `ztd init --starter [--withDogfooding --postgres-image]`
CLI->>Templates: generate feature-first scaffold (`src/features/smoke`, `ztd/ddl`, README, `compose.yaml`)
CLI->>Agents: if `withDogfooding`/starter -> installVisibleAgents(rootDir)
Agents-->>Templates: write AGENTS.md artifacts and .ztd/agents manifest
CLI-->>User: print next-steps (compose.yaml, `npx ztd ztd-config`, `vitest`)
User->>DB: start DB via `docker compose up -d` (compose.yaml)
User->>TestRunner: run `vitest` to verify starter tests
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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. 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: 9
🧹 Nitpick comments (3)
packages/ztd-cli/templates/src/features/smoke/tests/smoke.test.ts (1)
12-12: RenamespecFiletosqlFilefor semantic clarity.The result field is named
specFilebut intentionally holds the SQL file path value fromsmokeSpec.sqlFile. Renaming tosqlFile(orsqlFilePath) would eliminate the naming ambiguity and align with the QuerySpec pattern used throughout the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ztd-cli/templates/src/features/smoke/tests/smoke.test.ts` at line 12, The test asserts result.specFile but the field actually stores the SQL path from smokeSpec.sqlFile and should be renamed for clarity; update the test expectation and any code constructing or reading this object to use result.sqlFile (or result.sqlFilePath) instead of result.specFile, and ensure the QuerySpec pattern is followed by aligning the property name with smokeSpec.sqlFile and updating any references to result.specFile in functions/constructors that create or consume this object (e.g., where smokeSpec, QuerySpec, or the factory that builds result are used).packages/ztd-cli/templates/vitest.config.ts (1)
5-5: Redundant glob pattern.The pattern
src/features/**/*.validation.test.tsis already matched bysrc/features/**/*.test.ts. Vitest will deduplicate, but keeping redundant patterns may cause confusion about the intended test discovery behavior.If both patterns are intentional for documentation purposes (e.g., signaling that
.validation.test.tsis a recognized convention), consider adding a comment. Otherwise, the third pattern can be removed.♻️ Simplified include pattern
- include: ['tests/**/*.test.ts', 'src/features/**/*.test.ts', 'src/features/**/*.validation.test.ts'], + include: ['tests/**/*.test.ts', 'src/features/**/*.test.ts'],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ztd-cli/templates/vitest.config.ts` at line 5, The include array in vitest.config.ts contains a redundant glob: src/features/**/*.validation.test.ts is already matched by src/features/**/*.test.ts; remove the third pattern from the include array (or if you intentionally want to document the .validation.test.ts convention, replace it with a one-line comment near the include array mentioning that .validation.test.ts is a recognized convention) so the include list is not confusing—edit the include array in the vitest.config.ts file (the include symbol) accordingly.packages/ztd-cli/templates/src/AGENTS.md (1)
10-10: Consider retaining the SQL/DDL ownership constraint.The new wording is more general, but the original constraint ("Domain, application, and presentation layers MUST stay free from direct SQL/DDL ownership when those directories exist") provided explicit guardrails that the coding guidelines still enforce. If features now contain local
domain/,application/, andpersistence/subdirectories, you may want to preserve this constraint or adapt it for the feature-local context (e.g., "Feature-local domain and application folders MUST remain independent from SQL/DDL ownership").🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ztd-cli/templates/src/AGENTS.md` at line 10, Restore the explicit SQL/DDL ownership constraint in AGENTS.md by adding a sentence after "Feature-local folders MUST stay close to the code they support" that either reintroduces the original rule verbatim ("Domain, application, and presentation layers MUST stay free from direct SQL/DDL ownership when those directories exist") or adapts it for feature-local folders (e.g., "Feature-local domain and application folders MUST remain independent from SQL/DDL ownership; persistence/DDL must live in dedicated infra/persistence areas"). Ensure the new sentence references the same concepts (domain, application, presentation, persistence/DDL) so readers of AGENTS.md clearly understand who must not own SQL/DDL when feature-local directories are present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/dogfooding/ztd-migration-lifecycle.md`:
- Around line 18-24: Update the migration lifecycle text so the DDL/SQL commands
are copy-paste complete: explicitly show required target flags for `ddl diff`
and `ddl pull` (e.g. include `--url` or a full `--db-*` flag set) and state that
`ZTD_TEST_DATABASE_URL` is the only implicit database owned by ztd-cli — all
other inspection targets must be passed explicitly; adjust the migration
row/prompt to treat `ddl pull` and `ddl diff` as explicit inspection commands
requiring those flags and update any example CLI lines (the ones showing `ddl
diff` / `ddl pull`) to include the corresponding `--url`/`--db-*` options.
In `@docs/guide/sql-first-end-to-end-tutorial.md`:
- Around line 21-29: Update the migration row and step 6 to require explicit
database targets instead of implying use of ZTD_TEST_DATABASE_URL: replace any
suggestion that `npx ztd ddl pull` or `npx ztd ddl diff` can be run implicitly
against the test DB with explicit examples using `--url` or a full `--db-*` flag
set (e.g., `npx ztd ddl diff --url <DATABASE_URL>`), and add a short note that
ZTD_TEST_DATABASE_URL is the only implicit database owned by ztd-cli and all
other inspection/ddl commands (such as `npx ztd ddl diff` / `npx ztd ddl pull`)
must be passed an explicit target via `--url` or `--db-*`.
In `@packages/ztd-cli/src/utils/agents.ts`:
- Around line 73-96: The manifest scopes in INTERNAL_AGENT_TEMPLATES are too
smoke-specific and missing entries (e.g., routing_rules maps src/sql/** to scope
'src-sql' but INTERNAL_AGENT_TEMPLATES never defines 'src-sql', and
src/features/... routes to src-features-smoke-* which leaves non-smoke feature
paths unresolved); update INTERNAL_AGENT_TEMPLATES to include the missing
'src-sql' entry and either (a) add corresponding non-smoke scopes (e.g.,
'src-sql', 'src-features-application', 'src-features-domain',
'src-features-persistence', 'src-features-tests') or (b) change the existing
scope names like 'src-features-smoke-application'/'src-features-smoke-domain'
etc. to generic names that match routing_rules, ensuring every scope referenced
by routing_rules (for example 'src-sql' and the src-features-* variants) is
defined in the template list (look for INTERNAL_AGENT_TEMPLATES and scope
strings such as 'src-features-smoke-application' and 'src-sql').
In `@packages/ztd-cli/templates/AGENTS.md`:
- Line 10: Restore `src/catalog/specs` to the list of protected human-owned
contract directories in the AGENTS.md text: update the sentence that currently
lists "ztd/ddl, src/sql" to include "src/catalog/specs" so it reads `ztd/ddl,
src/catalog/specs, src/sql` and keep the same MUST NOT wording; locate the exact
line containing "Human-owned contract directories" and add `src/catalog/specs`
to the parenthesized list to prevent weakening the guardrail.
In `@packages/ztd-cli/templates/src/features/smoke/application/smoke-workflow.ts`:
- Around line 1-14: The application layer is leaking persistence metadata by
importing smokeSpec and exposing specFile; remove the smokeSpec import and stop
reading smokeSpec.sqlFile inside buildSmokeWorkflow — instead either accept
specFile as an argument to buildSmokeWorkflow or remove specFile from
SmokeWorkflowResult and let the persistence/composition caller attach it. Update
the SmokeWorkflowResult type and the buildSmokeWorkflow signature accordingly
(referencing buildSmokeWorkflow, SmokeWorkflowResult, smokeSpec and
normalizeSmokeOutput) so the module only performs orchestration and
normalization via normalizeSmokeOutput and does not own SQL file details.
In `@packages/ztd-cli/templates/tests/queryspec.example.test.ts`:
- Around line 69-80: The mapping in the validate function is using
Boolean(row.is_active) which can incorrectly coerce strings like "false" or "0"
to true; update the isActive mapping to parse explicit boolean representations
instead: if the value is already a boolean, use it; if it's a string, accept
"true"/"false" (and optionally "1"/"0") and convert accordingly; otherwise throw
a validation error. Implement this logic either inline where isActive is set or
extract to a small helper function (e.g., parseBoolean) and call it from the
validate mapping so invalid or ambiguous values are rejected rather than
silently coerced.
In `@packages/ztd-cli/templates/tests/smoke.test.ts`:
- Around line 17-21: The test currently only asserts the shape of the seam
(typeof client.query) but must exercise SQL execution; update the test that uses
createTestkitClient() to run an actual query via client.query (e.g., a simple
SELECT or an inserted fixture) and assert the returned result rows/DTO shape and
any expected rewrite/mapping/validation effects before calling client.close().
Ensure the assertion verifies execution (not just function shape), checks
returned row structure/fields and expected values produced by the ZTD
rewrite/mapping, and use the existing test function name (smoke: SqlClient seam
is the handoff point after the feature sample) and client methods (client.query,
client.close) so the test continues to cover runtime/rewrite regressions.
In `@packages/ztd-cli/templates/ztd/ddl/demo.sql`:
- Around line 1-10: Add required COMMENT ON statements for the new users table
and its columns so the starter DDL carries the contract metadata; specifically,
after the create table/create index block add COMMENT ON TABLE users IS '<brief
description of the users table>'; and add COMMENT ON COLUMN users.user_id IS
'<pk id description>', COMMENT ON COLUMN users.email IS '<email description>',
COMMENT ON COLUMN users.display_name IS '<display name description>', COMMENT ON
COLUMN users.is_active IS '<active flag description>', and COMMENT ON COLUMN
users.created_at IS '<creation timestamp description>' (use clear, concise prose
for each comment).
In `@README.md`:
- Around line 9-10: The README currently gives conflicting scaffold guidance: it
promotes a feature-first layout under src/features/<feature>/ (and includes
src/features/smoke/) but later asserts SQL belongs in src/sql/ as the single
human-owned location; pick one canonical layout (prefer the new feature-first
layout) and update the README to consistently recommend placing feature code and
SQL inside src/features/<feature>/ (or document that src/sql/ is only an
internal legacy location), replacing or removing the sentence that mandates
src/sql/ and adjusting any examples or references to use src/features/<feature>/
(and mention src/features/smoke/ as the removable teaching feature) so first-run
users see one clear scaffold pattern.
---
Nitpick comments:
In `@packages/ztd-cli/templates/src/AGENTS.md`:
- Line 10: Restore the explicit SQL/DDL ownership constraint in AGENTS.md by
adding a sentence after "Feature-local folders MUST stay close to the code they
support" that either reintroduces the original rule verbatim ("Domain,
application, and presentation layers MUST stay free from direct SQL/DDL
ownership when those directories exist") or adapts it for feature-local folders
(e.g., "Feature-local domain and application folders MUST remain independent
from SQL/DDL ownership; persistence/DDL must live in dedicated infra/persistence
areas"). Ensure the new sentence references the same concepts (domain,
application, presentation, persistence/DDL) so readers of AGENTS.md clearly
understand who must not own SQL/DDL when feature-local directories are present.
In `@packages/ztd-cli/templates/src/features/smoke/tests/smoke.test.ts`:
- Line 12: The test asserts result.specFile but the field actually stores the
SQL path from smokeSpec.sqlFile and should be renamed for clarity; update the
test expectation and any code constructing or reading this object to use
result.sqlFile (or result.sqlFilePath) instead of result.specFile, and ensure
the QuerySpec pattern is followed by aligning the property name with
smokeSpec.sqlFile and updating any references to result.specFile in
functions/constructors that create or consume this object (e.g., where
smokeSpec, QuerySpec, or the factory that builds result are used).
In `@packages/ztd-cli/templates/vitest.config.ts`:
- Line 5: The include array in vitest.config.ts contains a redundant glob:
src/features/**/*.validation.test.ts is already matched by
src/features/**/*.test.ts; remove the third pattern from the include array (or
if you intentionally want to document the .validation.test.ts convention,
replace it with a one-line comment near the include array mentioning that
.validation.test.ts is a recognized convention) so the include list is not
confusing—edit the include array in the vitest.config.ts file (the include
symbol) accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 249813f9-5273-4036-92c0-24c2bd9bf6f7
⛔ Files ignored due to path filters (1)
packages/ztd-cli/tests/__snapshots__/init.command.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (54)
README.mddocs/dogfooding/perf-scale-tuning.mddocs/dogfooding/ztd-application-lifecycle.mddocs/dogfooding/ztd-migration-lifecycle.mddocs/guide/perf-tuning-decision-guide.mddocs/guide/sql-first-end-to-end-tutorial.mdpackages/ztd-cli/README.mdpackages/ztd-cli/src/commands/init.tspackages/ztd-cli/src/utils/agents.tspackages/ztd-cli/src/utils/sqlCatalogDiscovery.tspackages/ztd-cli/templates/AGENTS.mdpackages/ztd-cli/templates/CONTEXT.mdpackages/ztd-cli/templates/CONTEXT.webapi.mdpackages/ztd-cli/templates/PROMPT_DOGFOOD.mdpackages/ztd-cli/templates/PROMPT_DOGFOOD.webapi.mdpackages/ztd-cli/templates/README.mdpackages/ztd-cli/templates/README.webapi.mdpackages/ztd-cli/templates/src/AGENTS.mdpackages/ztd-cli/templates/src/application/AGENTS.mdpackages/ztd-cli/templates/src/domain/AGENTS.mdpackages/ztd-cli/templates/src/features/AGENTS.mdpackages/ztd-cli/templates/src/features/README.mdpackages/ztd-cli/templates/src/features/smoke/AGENTS.mdpackages/ztd-cli/templates/src/features/smoke/README.mdpackages/ztd-cli/templates/src/features/smoke/application/AGENTS.mdpackages/ztd-cli/templates/src/features/smoke/application/README.mdpackages/ztd-cli/templates/src/features/smoke/application/smoke-workflow.tspackages/ztd-cli/templates/src/features/smoke/domain/AGENTS.mdpackages/ztd-cli/templates/src/features/smoke/domain/README.mdpackages/ztd-cli/templates/src/features/smoke/domain/smoke-policy.tspackages/ztd-cli/templates/src/features/smoke/persistence/AGENTS.mdpackages/ztd-cli/templates/src/features/smoke/persistence/README.mdpackages/ztd-cli/templates/src/features/smoke/persistence/smoke.spec.tspackages/ztd-cli/templates/src/features/smoke/persistence/smoke.sqlpackages/ztd-cli/templates/src/features/smoke/tests/AGENTS.mdpackages/ztd-cli/templates/src/features/smoke/tests/README.mdpackages/ztd-cli/templates/src/features/smoke/tests/smoke.test.tspackages/ztd-cli/templates/src/features/smoke/tests/smoke.validation.test.tspackages/ztd-cli/templates/src/infrastructure/AGENTS.mdpackages/ztd-cli/templates/src/infrastructure/db/AGENTS.mdpackages/ztd-cli/templates/src/infrastructure/persistence/AGENTS.mdpackages/ztd-cli/templates/src/infrastructure/persistence/repositories/AGENTS.mdpackages/ztd-cli/templates/src/presentation/AGENTS.mdpackages/ztd-cli/templates/src/presentation/http/AGENTS.mdpackages/ztd-cli/templates/tests/queryspec.example.test.tspackages/ztd-cli/templates/tests/smoke.test.tspackages/ztd-cli/templates/tests/smoke.validation.test.tspackages/ztd-cli/templates/vitest.config.tspackages/ztd-cli/templates/ztd/ddl/demo.sqlpackages/ztd-cli/tests/directoryFinding.docs.test.tspackages/ztd-cli/tests/init.command.test.tspackages/ztd-cli/tests/sqlCatalogDiscovery.unit.test.tspackages/ztd-cli/tests/sqlFirstTutorial.docs.test.tspackages/ztd-cli/tests/utils/agents.test.ts
💤 Files with no reviewable changes (3)
- packages/ztd-cli/templates/PROMPT_DOGFOOD.webapi.md
- packages/ztd-cli/templates/README.webapi.md
- packages/ztd-cli/templates/CONTEXT.webapi.md
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/ztd-cli/src/commands/init.ts (1)
1043-1048: Consider adding error handling forinstallVisibleAgents.The
installVisibleAgents(rootDir)call's return value is only used for a log message check. If the function throws or returns unexpected values, the init flow would continue without visible indication of failure.🛡️ Suggested defensive handling
if (starter) { - const visibleAgentSummaries = installVisibleAgents(rootDir); - if (visibleAgentSummaries.some((summary) => summary.relativePath === 'AGENTS.md')) { - dependencies.log('Visible AGENTS.md files installed for the starter flow.'); + try { + const visibleAgentSummaries = installVisibleAgents(rootDir); + if (visibleAgentSummaries.some((summary) => summary.relativePath === 'AGENTS.md')) { + dependencies.log('Visible AGENTS.md files installed for the starter flow.'); + } + } catch (error) { + dependencies.log(`Warning: Failed to install visible AGENTS.md files: ${extractErrorMessage(error)}`); } }🤖 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 1043 - 1048, Wrap the installVisibleAgents(rootDir) call in a try/catch and validate its return before using it: call installVisibleAgents inside try, assign to visibleAgentSummaries, ensure it's an array (Array.isArray) before calling .some, and on error or invalid return use dependencies.log/dependencies.error to record the failure (include error message and rootDir) and either fall back to an empty array or rethrow/abort the init flow so the failure is visible; update the code around installVisibleAgents, visibleAgentSummaries, and the dependencies.log call accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/ztd-cli/src/commands/init.ts`:
- Around line 1043-1048: Wrap the installVisibleAgents(rootDir) call in a
try/catch and validate its return before using it: call installVisibleAgents
inside try, assign to visibleAgentSummaries, ensure it's an array
(Array.isArray) before calling .some, and on error or invalid return use
dependencies.log/dependencies.error to record the failure (include error message
and rootDir) and either fall back to an empty array or rethrow/abort the init
flow so the failure is visible; update the code around installVisibleAgents,
visibleAgentSummaries, and the dependencies.log call accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 272ed822-62d5-417a-bb1a-ae2d7eb0f0ac
📒 Files selected for processing (3)
.changeset/seven-suits-argue.mdpackages/ztd-cli/src/commands/init.tspackages/ztd-cli/tests/init.command.test.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/seven-suits-argue.md
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/guide/sql-first-end-to-end-tutorial.md (1)
133-136:⚠️ Potential issue | 🟡 MinorShow the explicit
--urlsyntax in the migration loop steps.Step 3 mentions "against an explicit target" but doesn't demonstrate the actual
--urlsyntax. Since this is the copy-paste path for migration work, showing the concrete command helps prevent readers from accidentally using the implicit test database.Suggested wording to match the earlier guidance
2. Run `npx ztd ztd-config` to refresh the ZTD-generated artifacts. -3. Run `npx ztd ddl diff` or `npx ztd ddl pull` against an explicit target when you need a migration plan. -4. Apply the generated SQL outside `ztd-cli`. -5. Re-run `npx vitest run` after the migration lands. +3. If you need to inspect the target first, run `npx ztd ddl pull --url <target-db-url>`. +4. Run `npx ztd ddl diff --url <target-db-url>` when you need a migration plan. +5. Apply the generated SQL outside `ztd-cli`. +6. Re-run `npx vitest run` after the migration lands.Based on learnings: Ensure
ZTD_TEST_DATABASE_URLis the only implicit database owned by ztd-cli; other database targets MUST be passed explicitly via--urlor complete--db-*flag set for inspection work.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/guide/sql-first-end-to-end-tutorial.md` around lines 133 - 136, Update the migration loop steps to show the concrete --url usage: replace or augment step 3 so it demonstrates explicit commands such as running "npx ztd ddl diff --url '<DATABASE_URL>'" and "npx ztd ddl pull --url '<DATABASE_URL>'" (and mention both variants: --url vs the full --db-* flag set), and add a short note reminding readers that ZTD_TEST_DATABASE_URL is the only implicit test DB and all other targets must be passed explicitly via --url or complete --db-* flags; reference the commands npx ztd ddl diff, npx ztd ddl pull, --url, and ZTD_TEST_DATABASE_URL when making the change.
🧹 Nitpick comments (2)
docs/guide/sql-first-end-to-end-tutorial.md (2)
140-150: LGTM! Clear success criteria.The checklist provides good self-assessment questions for readers to verify their understanding of the feature-first workflow.
Optional: Minor style improvement
Consider varying the sentence structure to avoid repetition:
- Where does the next feature live? - Which files should the agent read first? -- Which command verifies the change? -- Which files stay feature-local? +- What command verifies the change? +- What files stay feature-local? - How do I prepare a migration without making `ztd-cli` deploy it for me?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/guide/sql-first-end-to-end-tutorial.md` around lines 140 - 150, Under the "## 7. What good looks like" section the success-checklist is stylistically repetitive (many bullets starting with "Which"); reword the bulleted questions to vary sentence structure and tone while preserving meaning—edit the list entries under the header "## 7. What good looks like" (the five checklist bullets) to mix question formats (e.g., convert some "Which..." questions into imperative or short declarative prompts such as "Locate the next feature" or "Run the command that verifies the change") so the checklist reads smoother and avoids repetitive phrasing.
108-125: LGTM! Repair scenarios are well-documented.The section provides clear CLI examples for DDL, SQL, and DTO repair loops, and line 122 correctly demonstrates the explicit
--url <target-db-url>syntax for migration work.Optional: Minor style improvements
Consider these stylistic refinements:
For SQL repair, keep the SQL assets under the feature folder, keep the query on the starter DDL's `users` table, and pass that folder explicitly as `--sql-root` when you ask `model-gen` to refresh the spec. -For migration work, use an explicit `--url <target-db-url>` with `ddl pull` or `ddl diff` so the target database is never inferred from the starter test database by accident. +For migration work, use an explicit `--url <target-db-url>` with `ddl pull` or `ddl diff` so the target database is never inferred from the starter test database accidentally.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/guide/sql-first-end-to-end-tutorial.md` around lines 108 - 125, No functional changes required—the repair scenarios and CLI examples (e.g., the npx ztd query command, model-gen refresh with --sql-root, and using --url with ddl pull/diff) are correct and can be approved; optionally, apply minor stylistic refinements such as ensuring all CLI examples (npx ztd query uses column users.email --sql-root src/features/users/persistence --specs-dir src/features/users/persistence --any-schema --view detail, model-gen refresh invocation, and ddl pull/diff with --url <target-db-url>) are consistently formatted with inline code/backticks and that the note about vitest passing and separation of tuning concerns remains explicit.
🤖 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/src/commands/init.ts`:
- Around line 345-369: The STARTER_SCHEMA_TEMPLATE produces DDL that diverges
from demo.sql and tests; update the STARTER_SCHEMA_TEMPLATE so the users table
matches the scaffold fixtures: replace the user_name column with display_name,
add an is_active boolean column with the same default/constraint used in
demo.sql, change user_id from "serial8" to "bigint generated by default as
identity" (to match identity semantics), and update the related
comment-on-column texts to reference display_name and is_active as in the tests;
ensure the resulting CREATE TABLE and COMMENT statements in
STARTER_SCHEMA_TEMPLATE mirror the naming, types, and defaults used by demo.sql,
smoke.test.ts, and queryspec.example.test.ts.
---
Duplicate comments:
In `@docs/guide/sql-first-end-to-end-tutorial.md`:
- Around line 133-136: Update the migration loop steps to show the concrete
--url usage: replace or augment step 3 so it demonstrates explicit commands such
as running "npx ztd ddl diff --url '<DATABASE_URL>'" and "npx ztd ddl pull --url
'<DATABASE_URL>'" (and mention both variants: --url vs the full --db-* flag
set), and add a short note reminding readers that ZTD_TEST_DATABASE_URL is the
only implicit test DB and all other targets must be passed explicitly via --url
or complete --db-* flags; reference the commands npx ztd ddl diff, npx ztd ddl
pull, --url, and ZTD_TEST_DATABASE_URL when making the change.
---
Nitpick comments:
In `@docs/guide/sql-first-end-to-end-tutorial.md`:
- Around line 140-150: Under the "## 7. What good looks like" section the
success-checklist is stylistically repetitive (many bullets starting with
"Which"); reword the bulleted questions to vary sentence structure and tone
while preserving meaning—edit the list entries under the header "## 7. What good
looks like" (the five checklist bullets) to mix question formats (e.g., convert
some "Which..." questions into imperative or short declarative prompts such as
"Locate the next feature" or "Run the command that verifies the change") so the
checklist reads smoother and avoids repetitive phrasing.
- Around line 108-125: No functional changes required—the repair scenarios and
CLI examples (e.g., the npx ztd query command, model-gen refresh with
--sql-root, and using --url with ddl pull/diff) are correct and can be approved;
optionally, apply minor stylistic refinements such as ensuring all CLI examples
(npx ztd query uses column users.email --sql-root src/features/users/persistence
--specs-dir src/features/users/persistence --any-schema --view detail, model-gen
refresh invocation, and ddl pull/diff with --url <target-db-url>) are
consistently formatted with inline code/backticks and that the note about vitest
passing and separation of tuning concerns remains explicit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 744d0186-1f59-4fb3-981d-c02ae7ce2c90
📒 Files selected for processing (15)
docs/dogfooding/ztd-migration-lifecycle.mddocs/guide/sql-first-end-to-end-tutorial.mdpackages/ztd-cli/src/commands/init.tspackages/ztd-cli/src/utils/agents.tspackages/ztd-cli/templates/AGENTS.mdpackages/ztd-cli/templates/src/AGENTS.mdpackages/ztd-cli/templates/src/features/smoke/application/smoke-workflow.tspackages/ztd-cli/templates/src/features/smoke/tests/smoke.test.tspackages/ztd-cli/templates/tests/queryspec.example.test.tspackages/ztd-cli/templates/tests/smoke.test.tspackages/ztd-cli/templates/vitest.config.tspackages/ztd-cli/templates/ztd/ddl/demo.sqlpackages/ztd-cli/tests/init.command.test.tspackages/ztd-cli/tests/sqlFirstTutorial.docs.test.tspackages/ztd-cli/tests/utils/agents.test.ts
✅ Files skipped from review due to trivial changes (3)
- packages/ztd-cli/templates/src/features/smoke/tests/smoke.test.ts
- packages/ztd-cli/templates/src/AGENTS.md
- packages/ztd-cli/templates/AGENTS.md
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/ztd-cli/templates/vitest.config.ts
- packages/ztd-cli/templates/src/features/smoke/application/smoke-workflow.ts
- packages/ztd-cli/tests/sqlFirstTutorial.docs.test.ts
- docs/dogfooding/ztd-migration-lifecycle.md
- packages/ztd-cli/tests/utils/agents.test.ts
- packages/ztd-cli/src/utils/agents.ts
There was a problem hiding this comment.
Actionable comments posted: 3
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/init.ts (1)
2309-2414:⚠️ Potential issue | 🟠 Major
--with-sqlclientis a dead flag in the real init path.Line 2435 still exposes the option, and
buildInitDryRunPlan()adds telemetry files for it, butrunInitCommand()never readsoptions.withSqlClient. Today the basesrc/db/sql-client*.tsfiles are emitted regardless, and the extra telemetry files shown in dry-run are never created, so the CLI contract has drifted.Either remove the flag from the public surface, or wire it back into the actual scaffold branches so
--dry-runand the real generator stay aligned.Also applies to: 2435-2435
🤖 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 2309 - 2414, buildInitDryRunPlan exposes and adds telemetry files for the --with-sqlclient flag (options.withSqlClient) but runInitCommand does not honor it, causing dry-run to diverge from the real scaffold; either remove the flag from the public surface or wire it into the real generator: locate buildInitDryRunPlan and the runInitCommand scaffold branches, then either (A) remove options.withSqlClient from the Init options and delete the telemetry file pushes (paths under src/infrastructure/telemetry) so the dry-run no longer shows a non-existent flag, or (B) update runInitCommand to check options.withSqlClient and emit the same telemetry files (types.ts, consoleRepositoryTelemetry.ts, repositoryTelemetry.ts) into the scaffoldLayout when true so dry-run and actual generation are aligned.
🧹 Nitpick comments (1)
docs/guide/sql-first-end-to-end-tutorial.md (1)
121-122: Optional: Simplify repetitive sentence structure.Three consecutive sentences begin with "For" (lines 118, 120, 121-122). While the parallelism is intentional for a tutorial flow, you could rephrase one sentence to improve readability.
♻️ Optional rewording
-For migration work, use an explicit `--url <target-db-url>` with `ddl pull` or `ddl diff` so the target database is never inferred from the starter test database by accident. +When preparing migrations, always pass an explicit `--url <target-db-url>` to `ddl pull` or `ddl diff` so the target database is never inferred from the starter test database.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/guide/sql-first-end-to-end-tutorial.md` around lines 121 - 122, Three consecutive sentences start with "For" — reword the sentence that begins "For migration work, use an explicit `--url <target-db-url>` with `ddl pull` or `ddl diff`..." to avoid repetitive structure; replace it with a phrasing like "When performing migrations, specify an explicit `--url <target-db-url>` with `ddl pull` or `ddl diff` so the target database is never inferred from the starter test database by accident." This keeps the meaning and specific flags intact while breaking the repetitive "For" pattern.
🤖 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/src/commands/init.ts`:
- Around line 574-595: The init flow allows a conflicting combination of the
--starter flag and a non-demo workflow (options.workflow), causing
docs/compose/AGENTS to be generated but starter DDL to be skipped; update the
logic in the Init command (the block that sets workflow, starter, and reads
options?.workflow and overwritePolicy.nonInteractive, and the
prompter.selectChoice handling) so that if starter is true you either force
workflow = 'demo' unconditionally or throw a validation error when
options.workflow is set to anything other than 'demo'; mirror the same
validation in registerInitCommand() so --dry-run and other flag-parsing paths
reject the invalid --starter + non-demo workflow combination consistently.
- Around line 1046-1051: The call to installVisibleAgents() currently discards
its returned FileSummary[] (visibleAgentSummaries), so AGENTS.md files are
logged but not added to the InitResult.files or the human "Created:" output;
capture the returned summaries from installVisibleAgents() wherever it is
invoked in the starter flow (e.g., the block using visibleAgentSummaries and
dependencies.log) and append/merge those FileSummary objects into the
InitResult.files array (and ensure the same fix is applied to the other
invocation around the 2352-2368 region), then include those summaries in the
generated human-readable "Created:" section so the created AGENTS.md entries are
surfaced to users.
- Around line 374-385: The STARTER_README_APPENDIX function currently hardcodes
commands like `npx ztd`/`pnpm exec ztd`, causing drift when the CLI runs with
--local-source-root; update the code so the same computed CLI invocation strings
used in the local-source branch are passed into or derived by
STARTER_README_APPENDIX (e.g., change STARTER_README_APPENDIX to accept
parameters like ztdCmd and pnpmCmd or fetch the existing computed commands used
around the local-source logic) and replace the hard-coded occurrences (`npx
ztd`, `pnpm exec ztd`, `npm run ztd --`) with those parameters so generated
README and CLI summary always reflect the active command setup (refer to
STARTER_README_APPENDIX and the logic that computes the local-source
invocation).
---
Outside diff comments:
In `@packages/ztd-cli/src/commands/init.ts`:
- Around line 2309-2414: buildInitDryRunPlan exposes and adds telemetry files
for the --with-sqlclient flag (options.withSqlClient) but runInitCommand does
not honor it, causing dry-run to diverge from the real scaffold; either remove
the flag from the public surface or wire it into the real generator: locate
buildInitDryRunPlan and the runInitCommand scaffold branches, then either (A)
remove options.withSqlClient from the Init options and delete the telemetry file
pushes (paths under src/infrastructure/telemetry) so the dry-run no longer shows
a non-existent flag, or (B) update runInitCommand to check options.withSqlClient
and emit the same telemetry files (types.ts, consoleRepositoryTelemetry.ts,
repositoryTelemetry.ts) into the scaffoldLayout when true so dry-run and actual
generation are aligned.
---
Nitpick comments:
In `@docs/guide/sql-first-end-to-end-tutorial.md`:
- Around line 121-122: Three consecutive sentences start with "For" — reword the
sentence that begins "For migration work, use an explicit `--url
<target-db-url>` with `ddl pull` or `ddl diff`..." to avoid repetitive
structure; replace it with a phrasing like "When performing migrations, specify
an explicit `--url <target-db-url>` with `ddl pull` or `ddl diff` so the target
database is never inferred from the starter test database by accident." This
keeps the meaning and specific flags intact while breaking the repetitive "For"
pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3452e417-e5a1-4a29-8625-18df23c50ec9
📒 Files selected for processing (5)
README.mddocs/dogfooding/ztd-migration-lifecycle.mddocs/guide/sql-first-end-to-end-tutorial.mdpackages/ztd-cli/src/commands/init.tspackages/ztd-cli/tests/sqlFirstTutorial.docs.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- packages/ztd-cli/tests/sqlFirstTutorial.docs.test.ts
Comment on lines
+374
to
+385
| const STARTER_README_APPENDIX = (postgresImage: string): string => | ||
| [ | ||
| '## Starter Flow', | ||
| '', | ||
| '1. Start Postgres with `docker compose up -d`.', | ||
| `2. The bundled compose file uses \`${postgresImage}\`.`, | ||
| '3. Export `ZTD_TEST_DATABASE_URL=postgres://ztd:ztd@localhost:5432/ztd` before running Vitest.', | ||
| '4. Read `src/features/smoke/` first, then add `src/features/users/` as your first real feature.', | ||
| '5. Run `npx ztd ztd-config` to regenerate DDL-derived test rows and layout metadata.', | ||
| '6. Run `npx ztd model-gen --probe-mode ztd <sql-file> --out <spec-file>` to scaffold a QuerySpec from that SQL file.', | ||
| '7. Run `npm run test` or `npx vitest run` to confirm the smoke slice is green.', | ||
| '' |
There was a problem hiding this comment.
Keep starter instructions compatible with --local-source-root.
Line 2087 returns before the local-source branch, so a starter dogfooding scaffold gets generic npx ztd / pnpm exec ztd guidance instead of the guard-backed npm run ztd -- / pnpm ztd path. The same hard-coded commands are also baked into STARTER_README_APPENDIX(), so the generated README and CLI summary drift from the actual local-source setup.
Suggested direction
+ const starterZtdCommand =
+ scaffoldProfile.dependencyProfile === 'local-source' ? runLocalSourceZtdCommand : ztdCommand;
if (starter) {
const starterNextSteps = [
'Run docker compose up -d to start the bundled Postgres container',
`The bundled compose file uses ${postgresImage}; export ZTD_TEST_DATABASE_URL=postgres://ztd:ztd@localhost:5432/ztd before running Vitest`,
`Run tests (${runScriptCommand('test')} or npx vitest run) to confirm the smoke slice is green`,
'Read src/features/smoke/ first, then copy that layout for src/features/users/ as your first real feature',
- ...generationSteps,
+ ...generationSteps.map((step) => step.replace(ztdCommand, starterZtdCommand)),It would be worth feeding STARTER_README_APPENDIX() from the same computed commands too, so the README cannot drift again.
Also applies to: 2087-2102
🤖 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 374 - 385, The
STARTER_README_APPENDIX function currently hardcodes commands like `npx
ztd`/`pnpm exec ztd`, causing drift when the CLI runs with --local-source-root;
update the code so the same computed CLI invocation strings used in the
local-source branch are passed into or derived by STARTER_README_APPENDIX (e.g.,
change STARTER_README_APPENDIX to accept parameters like ztdCmd and pnpmCmd or
fetch the existing computed commands used around the local-source logic) and
replace the hard-coded occurrences (`npx ztd`, `pnpm exec ztd`, `npm run ztd
--`) with those parameters so generated README and CLI summary always reflect
the active command setup (refer to STARTER_README_APPENDIX and the logic that
computes the local-source invocation).
Comment on lines
+574
to
595
| // Determine workflow: use explicit flag, non-interactive default, starter preset, or prompt. | ||
| let workflow: InitWorkflow; | ||
| let starter = options?.starter === true; | ||
| if (options?.workflow) { | ||
| workflow = options.workflow; | ||
| } else if (starter) { | ||
| workflow = 'demo'; | ||
| } else if (overwritePolicy.nonInteractive) { | ||
| workflow = 'demo'; | ||
| } else { | ||
| const workflowChoice = await prompter.selectChoice( | ||
| 'How do you want to start your database workflow?', | ||
| [ | ||
| 'Starter (recommended): visible AGENTS, compose, smoke tests, and sample DDL', | ||
| 'Pull schema from Postgres (pg_dump)', | ||
| 'Create empty scaffold (I will write DDL)', | ||
| 'Create scaffold with demo DDL (no app code)' | ||
| ] | ||
| ); | ||
| workflow = workflowChoice === 0 ? 'pg_dump' : workflowChoice === 1 ? 'empty' : 'demo'; | ||
| starter = workflowChoice === 0; | ||
| workflow = workflowChoice === 0 ? 'demo' : workflowChoice === 1 ? 'pg_dump' : workflowChoice === 2 ? 'empty' : 'demo'; | ||
| } |
There was a problem hiding this comment.
Reject --starter with non-demo workflows.
Line 577 lets an explicit workflow override the starter preset, so ztd init --starter --workflow empty/pg_dump still emits starter README/compose/AGENTS but skips the starter DDL path. That leaves the generated docs and schema flow disagreeing immediately; either force workflow = 'demo' whenever starter is true or fail fast on any other workflow.
Suggested guard
let workflow: InitWorkflow;
let starter = options?.starter === true;
+ if (starter && options?.workflow && options.workflow !== 'demo') {
+ throw new Error('The starter preset only supports the demo workflow.');
+ }
if (options?.workflow) {
workflow = options.workflow;Mirror the same check in registerInitCommand() so --dry-run rejects the same invalid combination.
Also applies to: 2498-2509
🤖 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 574 - 595, The init flow
allows a conflicting combination of the --starter flag and a non-demo workflow
(options.workflow), causing docs/compose/AGENTS to be generated but starter DDL
to be skipped; update the logic in the Init command (the block that sets
workflow, starter, and reads options?.workflow and
overwritePolicy.nonInteractive, and the prompter.selectChoice handling) so that
if starter is true you either force workflow = 'demo' unconditionally or throw a
validation error when options.workflow is set to anything other than 'demo';
mirror the same validation in registerInitCommand() so --dry-run and other
flag-parsing paths reject the invalid --starter + non-demo workflow combination
consistently.
Comment on lines
+1046
to
+1051
| if (starter) { | ||
| const visibleAgentSummaries = installVisibleAgents(rootDir); | ||
| if (visibleAgentSummaries.some((summary) => summary.relativePath === 'AGENTS.md')) { | ||
| dependencies.log('Visible AGENTS.md files installed for the starter flow.'); | ||
| } | ||
| } |
There was a problem hiding this comment.
Include visible AGENTS.md files in the returned manifest.
Lines 1047-1051 drop the FileSummary[] returned by installVisibleAgents(), so the starter flow can create AGENTS.md, src/features/**/AGENTS.md, etc. without ever surfacing them in InitResult.files. The dry-run plan now advertises those files, so the real result payload is incomplete.
Minimal fix
const summaries: Partial<Record<FileKey, FileSummary>> = {};
+ const extraSummaries: FileSummary[] = [];
@@
if (starter) {
const visibleAgentSummaries = installVisibleAgents(rootDir);
+ extraSummaries.push(...visibleAgentSummaries);
if (visibleAgentSummaries.some((summary) => summary.relativePath === 'AGENTS.md')) {
dependencies.log('Visible AGENTS.md files installed for the starter flow.');
}
}
@@
return {
summary: summaryLines.join('\n'),
- files: Object.values(summaries) as FileSummary[]
+ files: [...Object.values(summaries), ...extraSummaries] as FileSummary[]
};Ideally those summaries should also be rendered in the human-readable “Created:” section, not just returned programmatically.
Also applies to: 2352-2368
🤖 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 1046 - 1051, The call to
installVisibleAgents() currently discards its returned FileSummary[]
(visibleAgentSummaries), so AGENTS.md files are logged but not added to the
InitResult.files or the human "Created:" output; capture the returned summaries
from installVisibleAgents() wherever it is invoked in the starter flow (e.g.,
the block using visibleAgentSummaries and dependencies.log) and append/merge
those FileSummary objects into the InitResult.files array (and ensure the same
fix is applied to the other invocation around the 2352-2368 region), then
include those summaries in the generated human-readable "Created:" section so
the created AGENTS.md entries are surfaced to users.
This was referenced Mar 27, 2026
This was referenced Apr 5, 2026
This was referenced Apr 15, 2026
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
Verification
Summary by CodeRabbit
New Features
Documentation
Improvements
Tests