feat(ztd-cli): add telemetry seam and observed SQL matcher - #682
Merged
Conversation
📝 WalkthroughWalkthroughAdds a no-op repository telemetry scaffold and telemetry types to the init starter, wires telemetry template files into Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer / CLI User
participant CLI as ztd CLI (match-observed)
participant Repo as Repository (disk: .sql assets)
participant Matcher as sql-grep-core matcher
participant Out as Stdout / File
Dev->>CLI: run `ztd query match-observed --sql-file` / `--sql`
CLI->>Repo: read project .sql assets (AST)
CLI->>Matcher: provide observed SQL AST + candidate ASTs
Matcher->>Matcher: normalize predicates and operands, compute section scores
Matcher-->>CLI: return ranked matches with scores and reasons
CLI->>Out: print text report or `--format json` output (optionally write `--out`)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
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 docstrings
🧪 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 |
mk3008
force-pushed
the
codex/675-telemetry-observed-sql-clean
branch
from
March 27, 2026 11:36
b6f8f43 to
6a1b5ef
Compare
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 (2)
packages/ztd-cli/templates/src/infrastructure/telemetry/consoleRepositoryTelemetry.ts (1)
34-52:⚠️ Potential issue | 🟠 MajorDo not log raw
errorMessagein the default console sink.Even with SQL text removed,
event.errorMessagecan still carry SQL fragments or parameter values from upstream driver/app errors. Keep the default payload to structured fields such aserrorName, and let applications opt into richer error logging in their own sink.Based on learnings, "Default telemetry behavior MUST stay conservative about query text emission."🛠️ Suggested fix
if (event.kind === 'query.execute.error') { payload.errorName = event.errorName; - payload.errorMessage = event.errorMessage; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ztd-cli/templates/src/infrastructure/telemetry/consoleRepositoryTelemetry.ts` around lines 34 - 52, The default console sink is including event.errorMessage which can leak SQL fragments/params; update the payload construction in consoleRepositoryTelemetry.ts (the code that builds the payload object using payload and event) to stop assigning payload.errorMessage for event.kind === 'query.execute.error' — keep only payload.errorName (and any structured fields like durationMs/rowCount), and ensure no other code in the same sink logs event.errorMessage; if downstream apps need richer error text, they should opt into that in a custom sink.packages/ztd-cli/templates/src/infrastructure/telemetry/repositoryTelemetry.ts (1)
36-43:⚠️ Potential issue | 🟡 MinorUpdate stale JSDoc to match no-op default behavior.
The comment still says “default console hook works,” but the default is now no-op.
📝 Suggested comment fix
/** * Resolve the repository telemetry hook that application code wants to use. * * Repository constructors can accept an optional telemetry dependency and call - * this helper so the default console hook works without extra setup. + * this helper so the default no-op hook works without extra setup. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ztd-cli/templates/src/infrastructure/telemetry/repositoryTelemetry.ts` around lines 36 - 43, Update the stale JSDoc above defaultRepositoryTelemetry to reflect that the default behavior is a no-op (created by createNoopRepositoryTelemetry) rather than a console hook; update the description for the repository telemetry hook and the note about repository constructors accepting an optional telemetry dependency so it states the default is a no-op telemetry implementation that requires no extra setup.
🧹 Nitpick comments (2)
packages/ztd-cli/templates/README.md (1)
70-89: Make adapter snippets directly runnable.The examples use
loggerandtracewithout showing their declarations/imports, which can cause copy/paste friction.♻️ Suggested doc patch
```ts +import { trace } from '@opentelemetry/api'; +const logger = console; + const repositoryTelemetry = { emit(event) { logger.info({ repositoryTelemetry: event }, 'repository telemetry'); } };+import { trace } from '@opentelemetry/api'; + const repositoryTelemetry = { emit(event) { const span = trace.getActiveSpan();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ztd-cli/templates/README.md` around lines 70 - 89, The README snippets use undeclared symbols; make each adapter snippet directly runnable by adding the missing declarations/imports: import trace from '@opentelemetry/api' (or import { trace } ...) and declare a logger (e.g., const logger = console) before the repositoryTelemetry object so repositoryTelemetry.emit, logger.info, and trace.getActiveSpan() resolve when copy/pasted; ensure both code blocks include these lines above their respective repositoryTelemetry definitions.packages/ztd-cli/templates/src/infrastructure/telemetry/types.ts (1)
6-15: TightenRepositoryTelemetryParameterShapeas a discriminated union.Current shape permits contradictory states (e.g.,
kind: 'scalar'witharrayLength: 'many', orkind: 'null'+nullability: 'non-null'). Encoding valid combinations in the type will keep emitted telemetry structurally consistent.♻️ Suggested type refactor
-export interface RepositoryTelemetryParameterShape { - name: string; - kind: RepositoryTelemetryParameterKind; - nullability: RepositoryTelemetryNullability; - arrayLength?: RepositoryTelemetryArrayLength; -} +type RepositoryTelemetryScalarOrUnknownShape = { + name: string; + kind: 'scalar' | 'unknown'; + nullability: 'non-null' | 'mixed'; + arrayLength?: never; +}; + +type RepositoryTelemetryArrayShape = { + name: string; + kind: 'array'; + nullability: 'non-null' | 'mixed'; + arrayLength: RepositoryTelemetryArrayLength; +}; + +type RepositoryTelemetryNullShape = { + name: string; + kind: 'null'; + nullability: 'null'; + arrayLength?: never; +}; + +export type RepositoryTelemetryParameterShape = + | RepositoryTelemetryScalarOrUnknownShape + | RepositoryTelemetryArrayShape + | RepositoryTelemetryNullShape;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ztd-cli/templates/src/infrastructure/telemetry/types.ts` around lines 6 - 15, Replace the loose RepositoryTelemetryParameterShape interface with a discriminated union keyed on RepositoryTelemetryParameterKind so invalid combos are impossible: define separate variants for kind='scalar' (no arrayLength, nullability limited appropriately), kind='array' (requires arrayLength and appropriate nullability), kind='null' (nullability fixed to 'null' and no arrayLength), and kind='unknown' (permissive). Update usages to accept the new union and keep the exported type names RepositoryTelemetryParameterKind, RepositoryTelemetryNullability, RepositoryTelemetryArrayLength unchanged.
🤖 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 2567-2572: The dry-run plan lists multiple AGENTS.md target paths
that runInitCommand() does not actually create in normal starter mode, causing
over-promising; update the dry-run generation logic to only include files that
runInitCommand() will create (e.g., keep the top-level 'AGENTS.md' entry or the
single path produced by runInitCommand()) and remove the extra paths
(path.join('ztd','AGENTS.md'), path.join('ztd','ddl','AGENTS.md'),
path.join('src','AGENTS.md'), path.join('src','features','AGENTS.md'),
path.join('tests','AGENTS.md')) so the plan matches actual outputs, ensuring the
code paths that assemble the dry-run list (where those AGENTS.md entries are
declared) reference the same creation logic used by runInitCommand().
In `@packages/ztd-cli/src/commands/query.ts`:
- Around line 439-451: Remove the duplicate function definition
resolveObservedSqlInput (the one that simply reads --sql or --sql-file) that
appears earlier in the file and causes TS2393; keep the existing, correct
implementation of resolveObservedSqlInput (the version at lines ~669–686) which
performs mutual-exclusion validation between --sql and --sql-file and proper
error handling. Ensure only the single, validated resolveObservedSqlInput
remains in the file so the mutual-exclusion logic is preserved and the duplicate
declaration is eliminated.
In `@packages/ztd-cli/tests/cliCommands.test.ts`:
- Around line 2378-2460: The PR added a duplicate test with the same title
("query match-observed ranks the likely source asset for observed SELECT SQL")
and unnecessarily mutates process.env.ZTD_PROJECT_ROOT even though runCli is
already passed an env override; remove the duplicate test block so only one
variant remains, and delete the manual process.env.ZTD_PROJECT_ROOT save/restore
logic inside that test (leave runCli(..., { ZTD_PROJECT_ROOT: workspace.rootDir
}, workspace.rootDir) as the sole env scoping mechanism) — look for the test
function by its title and references to process.env.ZTD_PROJECT_ROOT and runCli
to make the changes.
---
Outside diff comments:
In
`@packages/ztd-cli/templates/src/infrastructure/telemetry/consoleRepositoryTelemetry.ts`:
- Around line 34-52: The default console sink is including event.errorMessage
which can leak SQL fragments/params; update the payload construction in
consoleRepositoryTelemetry.ts (the code that builds the payload object using
payload and event) to stop assigning payload.errorMessage for event.kind ===
'query.execute.error' — keep only payload.errorName (and any structured fields
like durationMs/rowCount), and ensure no other code in the same sink logs
event.errorMessage; if downstream apps need richer error text, they should opt
into that in a custom sink.
In
`@packages/ztd-cli/templates/src/infrastructure/telemetry/repositoryTelemetry.ts`:
- Around line 36-43: Update the stale JSDoc above defaultRepositoryTelemetry to
reflect that the default behavior is a no-op (created by
createNoopRepositoryTelemetry) rather than a console hook; update the
description for the repository telemetry hook and the note about repository
constructors accepting an optional telemetry dependency so it states the default
is a no-op telemetry implementation that requires no extra setup.
---
Nitpick comments:
In `@packages/ztd-cli/templates/README.md`:
- Around line 70-89: The README snippets use undeclared symbols; make each
adapter snippet directly runnable by adding the missing declarations/imports:
import trace from '@opentelemetry/api' (or import { trace } ...) and declare a
logger (e.g., const logger = console) before the repositoryTelemetry object so
repositoryTelemetry.emit, logger.info, and trace.getActiveSpan() resolve when
copy/pasted; ensure both code blocks include these lines above their respective
repositoryTelemetry definitions.
In `@packages/ztd-cli/templates/src/infrastructure/telemetry/types.ts`:
- Around line 6-15: Replace the loose RepositoryTelemetryParameterShape
interface with a discriminated union keyed on RepositoryTelemetryParameterKind
so invalid combos are impossible: define separate variants for kind='scalar' (no
arrayLength, nullability limited appropriately), kind='array' (requires
arrayLength and appropriate nullability), kind='null' (nullability fixed to
'null' and no arrayLength), and kind='unknown' (permissive). Update usages to
accept the new union and keep the exported type names
RepositoryTelemetryParameterKind, RepositoryTelemetryNullability,
RepositoryTelemetryArrayLength unchanged.
🪄 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: 485200a2-2723-4a7f-8e05-b374e28509d8
⛔ Files ignored due to path filters (1)
packages/ztd-cli/tests/__snapshots__/describe.cli.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (14)
docs/guide/feature-index.mddocs/guide/observed-sql-matching.mdpackages/sql-grep-core/README.mdpackages/sql-grep-core/src/observed/match.tspackages/ztd-cli/src/commands/describe.tspackages/ztd-cli/src/commands/init.tspackages/ztd-cli/src/commands/query.tspackages/ztd-cli/templates/README.mdpackages/ztd-cli/templates/src/infrastructure/README.mdpackages/ztd-cli/templates/src/infrastructure/telemetry/consoleRepositoryTelemetry.tspackages/ztd-cli/templates/src/infrastructure/telemetry/repositoryTelemetry.tspackages/ztd-cli/templates/src/infrastructure/telemetry/types.tspackages/ztd-cli/tests/cliCommands.test.tspackages/ztd-cli/tests/init.command.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/ztd-cli/tests/init.command.test.ts (1)
217-221: Also assert the fields the scaffold must not emit.These lines only check for the new positive markers. A regression that reintroduces
sqlTextor raw parameter-value fields into the generated telemetry files would still pass. Please add a couple of explicitnot.toContain(...)assertions alongside these checks.As per coding guidelines, "Default telemetry behavior MUST stay conservative about query text emission."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ztd-cli/tests/init.command.test.ts` around lines 217 - 221, Add explicit negative assertions to the test: alongside the existing positive checks for repositoryTelemetry.ts, types.ts and consoleRepositoryTelemetry.ts, assert that generated files do NOT contain the raw query text or raw parameter-value fields by adding expect(readNormalizedFile(... 'src/infrastructure/telemetry/repositoryTelemetry.ts')).not.toContain('sqlText'), expect(readNormalizedFile(... 'src/infrastructure/telemetry/consoleRepositoryTelemetry.ts')).not.toContain('sqlText'), and expect(readNormalizedFile(... 'src/infrastructure/telemetry/types.ts')).not.toContain('parameterValues') (or another concrete raw-parameter identifier your generator might produce, e.g., 'paramValues'/'parameterValues'), so the test fails if sqlText or raw parameter-value fields reappear in repositoryTelemetry.ts, consoleRepositoryTelemetry.ts or types.ts.
🤖 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/templates/src/infrastructure/telemetry/types.ts`:
- Around line 6-42: The current telemetry parameter types
(RepositoryTelemetryParameterKind, RepositoryTelemetryNullability,
RepositoryTelemetryArrayLength and the union of
RepositoryTelemetryScalarParameterShape, RepositoryTelemetryArrayParameterShape,
RepositoryTelemetryNullParameterShape, RepositoryTelemetryUnknownParameterShape)
do not match the scaffolded contract: update the shapes so the union includes
fields required by the contract (presence, isNull, kind should allow 'scalar' |
'array' | 'object', explicit empty-string/empty-array flags, booleanValue, and
an optional operator) and remove impossible combinations (e.g., forbid
nullability: 'mixed' for kind: 'scalar'); specifically revise
RepositoryTelemetryScalarParameterShape, RepositoryTelemetryArrayParameterShape,
RepositoryTelemetryNullParameterShape and
RepositoryTelemetryUnknownParameterShape (and adjust
RepositoryTelemetryParameterKind/RepositoryTelemetryNullability types) so each
variant enforces correct presence/isNull semantics and array/empty flags,
ensuring the union statically prevents invalid states and matches the documented
telemetry contract.
---
Nitpick comments:
In `@packages/ztd-cli/tests/init.command.test.ts`:
- Around line 217-221: Add explicit negative assertions to the test: alongside
the existing positive checks for repositoryTelemetry.ts, types.ts and
consoleRepositoryTelemetry.ts, assert that generated files do NOT contain the
raw query text or raw parameter-value fields by adding
expect(readNormalizedFile(...
'src/infrastructure/telemetry/repositoryTelemetry.ts')).not.toContain('sqlText'),
expect(readNormalizedFile(...
'src/infrastructure/telemetry/consoleRepositoryTelemetry.ts')).not.toContain('sqlText'),
and expect(readNormalizedFile(...
'src/infrastructure/telemetry/types.ts')).not.toContain('parameterValues') (or
another concrete raw-parameter identifier your generator might produce, e.g.,
'paramValues'/'parameterValues'), so the test fails if sqlText or raw
parameter-value fields reappear in repositoryTelemetry.ts,
consoleRepositoryTelemetry.ts or types.ts.
🪄 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: 790dd607-c0e1-4597-a5e2-661ac478c05c
📒 Files selected for processing (12)
.changeset/clean-singers-explain.mddocs/guide/feature-index.mddocs/guide/observed-sql-investigation.mddocs/guide/observed-sql-matching.mddocs/guide/repository-telemetry-setup.mddocs/guide/ztd-cli-telemetry-philosophy.mdpackages/ztd-cli/README.mdpackages/ztd-cli/src/commands/init.tspackages/ztd-cli/templates/README.mdpackages/ztd-cli/templates/src/infrastructure/telemetry/types.tspackages/ztd-cli/tests/cliCommands.test.tspackages/ztd-cli/tests/init.command.test.ts
✅ Files skipped from review due to trivial changes (8)
- docs/guide/ztd-cli-telemetry-philosophy.md
- .changeset/clean-singers-explain.md
- packages/ztd-cli/README.md
- packages/ztd-cli/templates/README.md
- docs/guide/feature-index.md
- docs/guide/repository-telemetry-setup.md
- docs/guide/observed-sql-investigation.md
- docs/guide/observed-sql-matching.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/ztd-cli/src/commands/init.ts
- packages/ztd-cli/tests/cliCommands.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/ztd-cli/tests/init.command.test.ts (1)
246-275:localSourceRoot: nullmay violate strict type checking.The parameter type is
localSourceRoot?: string, butnullis passed. While bothnullandundefinedare falsy at runtime, TypeScript withstrictNullCheckstreats them differently. Consider usingundefinedor omitting the property entirely to maintain type correctness.♻️ Suggested fix
const plan = buildInitDryRunPlan(workspace, { appShape: 'default', starter: true, postgresImage: 'postgres:17', withAiGuidance: false, withDogfooding: false, withAppInterface: false, workflow: 'demo', - validator: 'zod', - localSourceRoot: null + validator: 'zod' });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ztd-cli/tests/init.command.test.ts` around lines 246 - 275, The test passes null for localSourceRoot which violates the function parameter type localSourceRoot?: string under strictNullChecks; update the test invocation of buildInitDryRunPlan in this test to either omit localSourceRoot or set it to undefined (e.g., remove the localSourceRoot entry or replace null with undefined) so the call matches the expected optional string signature used by buildInitDryRunPlan.
🤖 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/tests/init.command.test.ts`:
- Around line 246-275: The test passes null for localSourceRoot which violates
the function parameter type localSourceRoot?: string under strictNullChecks;
update the test invocation of buildInitDryRunPlan in this test to either omit
localSourceRoot or set it to undefined (e.g., remove the localSourceRoot entry
or replace null with undefined) so the call matches the expected optional string
signature used by buildInitDryRunPlan.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 540c5e63-c31d-40f3-b232-cd110c3491a0
📒 Files selected for processing (5)
docs/guide/repository-telemetry-setup.mdpackages/ztd-cli/templates/src/infrastructure/telemetry/repositoryTelemetry.tspackages/ztd-cli/templates/src/infrastructure/telemetry/types.tspackages/ztd-cli/tests/init.command.test.tsscripts/verify-published-package-mode.mjs
✅ Files skipped from review due to trivial changes (1)
- docs/guide/repository-telemetry-setup.md
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
queryId,repositoryName,methodName,paramsShape,transformations).ztd query match-observedand an AST-based observed SQL matcher for SELECT-oriented candidate ranking.Customer Value
queryIdwithout exposing SQL text or bind values.Verification
pnpm --filter @rawsql-ts/sql-grep-core test✅pnpm --filter @rawsql-ts/ztd-cli test -- cliCommands.test.ts init.command.test.tsmainto clear the GitHub merge conflict on this PR.Related Issue
Summary by CodeRabbit
New Features
queryIdis missing (supports inline/file input, text or JSON output).Documentation
Improvements
Tests