Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis change introduces SSSQL filter scaffolding and refresh CLI commands via a new Changes
Sequence Diagram(s)sequenceDiagram
participant User as User / CLI
participant CLI as ztd-cli<br/>query.ts
participant Builder as SSSQLFilterBuilder
participant Parser as SelectQueryParser
participant Formatter as SqlFormatter
User->>CLI: query sssql scaffold<br/>--sql-file users.sql<br/>--json '{"table.column": value}'
CLI->>CLI: parse & normalize<br/>SssqlScaffoldFilters
CLI->>CLI: read SQL file
CLI->>Builder: scaffold(sqlContent,<br/>filters)
Builder->>Parser: parse(sqlContent)
Parser-->>Builder: SelectQuery
Builder->>Builder: resolve filter targets<br/>in query graph
Builder->>Builder: generate optional<br/>equality branch:<br/>:param is null<br/>or column = :param
Builder->>Builder: append to WHERE clause
Builder-->>CLI: modified SelectQuery
CLI->>Formatter: format(query)
Formatter-->>CLI: formatted SQL
CLI->>User: output to --out/<br/>stdout + JSON envelope
sequenceDiagram
participant User as User / CLI
participant CLI as ztd-cli<br/>query.ts
participant Discoverer as discoverObservedSqlAssetFiles
participant Builder as buildObservedSqlMatchReport
participant Parser as SelectQueryParser
participant Scorer as ScoringEngine
participant Formatter as formatObservedSqlMatchReport
User->>CLI: query match-observed<br/>--sql 'SELECT users...'<br/>--rootDir ./src
CLI->>Discoverer: discover .sql files
Discoverer-->>Builder: [file paths]
CLI->>Builder: buildObservedSqlMatchReport<br/>{observedSql, rootDir}
Builder->>Builder: parse observed SQL
Builder->>Parser: parse(observedSql)
Parser-->>Builder: ObservedSqlQuerySummary
Builder->>Builder: create token sets<br/>(projection/source/<br/>where/order/paging)
Builder->>Builder: scan candidates<br/>from discovered files
loop Each candidate file
Builder->>Parser: parse(candidateSql)
Parser-->>Builder: candidate summaries
Builder->>Scorer: compareTokenSets<br/>(observed vs candidate)
Scorer-->>Builder: section scores +<br/>reasons/differences
Builder->>Builder: rank by<br/>overall score
end
Builder-->>CLI: ObservedSqlMatchReport
CLI->>Formatter: formatObservedSqlMatchReport<br/>(report, 'json'|'text')
Formatter-->>CLI: formatted output
CLI->>User: write to --out/<br/>stdout (JSON/text)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/core/src/transformers/SSSQLFilterBuilder.ts (1)
179-182: Unreachable code:matchwill always be defined at this point.After the checks for
matches.length === 0(which continues) andmatches.length > 1(which throws), the destructuredmatchfrommatchesis guaranteed to exist. Theif (!match)check is unreachable.♻️ Remove unreachable check
- const [match] = matches; - if (!match) { - continue; - } + const match = matches[0]!;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/transformers/SSSQLFilterBuilder.ts` around lines 179 - 182, In SSSQLFilterBuilder.ts where you destructure const [match] = matches after handling matches.length === 0 and matches.length > 1, remove the redundant unreachable guard if (!match) { continue; } so the code simply uses the guaranteed match; ensure no other logic depends on that branch and keep the existing checks for matches.length to preserve error handling.
🤖 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/core/tests/transformers/DynamicQueryBuilder.test.ts`:
- Around line 28-39: The test incorrectly calls builder.buildQuery(...) twice
with identical inputs expecting two different error messages; update the
assertion in DynamicQueryBuilder.test (the case exercising builder.buildQuery)
to assert a single error that includes both keywords by using one
expect(...).toThrow with a regex that matches both "scaffold" and "refresh"
(e.g. /scaffold.*refresh|refresh.*scaffold/i) or assert the full static message
returned by the builder.buildQuery error; keep the reference to
builder.buildQuery and the same input payload.
In `@packages/ztd-cli/src/commands/query.ts`:
- Around line 535-560: The call to new SssqlQueryTransformer().refresh(sql) in
runQuerySssqlRefreshCommand is missing the required filters parameter expected
by SSSQLFilterBuilder.refresh; update runQuerySssqlRefreshCommand to extract
filters from the QuerySssqlRefreshOptions (e.g., const filters = options.filters
?? []) and pass them into SssqlQueryTransformer.refresh(sql, filters), or if the
transformer expects a built filter object, pass
SSSQLFilterBuilder.build(filters) (or equivalent) before calling
SssqlQueryTransformer.refresh; reference the function names
runQuerySssqlRefreshCommand, SssqlQueryTransformer.refresh,
QuerySssqlRefreshOptions and SSSQLFilterBuilder.refresh when making the change.
In `@packages/ztd-cli/tests/cliCommands.test.ts`:
- Around line 330-332: The assertion uses a malformed string 'ud"."status =
:status' — fix the expectation in the test that checks the output stored in
contents (read from outFile) by replacing the incorrect fragment with the
properly quoted identifier, e.g. '"ud"."status" = :status', so the
.not.toContain check on contents correctly verifies the outer query no longer
references ud.status; update the expect(contents).not.toContain(...) call
accordingly.
---
Nitpick comments:
In `@packages/core/src/transformers/SSSQLFilterBuilder.ts`:
- Around line 179-182: In SSSQLFilterBuilder.ts where you destructure const
[match] = matches after handling matches.length === 0 and matches.length > 1,
remove the redundant unreachable guard if (!match) { continue; } so the code
simply uses the guaranteed match; ensure no other logic depends on that branch
and keep the existing checks for matches.length to preserve error handling.
🪄 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: e9db7aa5-32e3-4ef3-a254-da9f576c0872
📒 Files selected for processing (14)
.changeset/soft-tomatoes-explain.mdREADME.mddocs/guide/sql-tool-happy-paths.mdpackages/core/src/index.tspackages/core/src/transformers/DynamicQueryBuilder.tspackages/core/src/transformers/PruneOptionalConditionBranches.tspackages/core/src/transformers/SSSQLFilterBuilder.tspackages/core/tests/transformers/DynamicFilterRoutingDogfooding.test.tspackages/core/tests/transformers/DynamicQueryBuilder.test.tspackages/core/tests/transformers/PruneOptionalConditionBranches.test.tspackages/core/tests/transformers/SSSQLFilterBuilder.test.tspackages/ztd-cli/README.mdpackages/ztd-cli/src/commands/query.tspackages/ztd-cli/tests/cliCommands.test.ts
Comment on lines
+28
to
+39
| it('fails fast when a runtime filter would add a new predicate', () => { | ||
| expect(() => | ||
| builder.buildQuery('SELECT id, name FROM users WHERE active = true', { | ||
| filter: { name: 'Alice' } | ||
| }) | ||
| ).toThrow(/ztd query sssql scaffold/i); | ||
| expect(() => | ||
| builder.buildQuery('SELECT id, name FROM users WHERE active = true', { | ||
| filter: { name: 'Alice' } | ||
| }) | ||
| ).toThrow(/ztd query sssql refresh/i); | ||
| }); |
There was a problem hiding this comment.
Test assertion logic is incorrect: identical calls cannot produce different error messages.
The test calls builder.buildQuery(...) twice with the same input and expects different error message patterns (/ztd query sssql scaffold/i vs /ztd query sssql refresh/i). Based on the context snippet from DynamicQueryBuilder.ts:241-257, the error message is a single static string that contains both "scaffold" and "refresh". Either consolidate into a single assertion that matches the full message, or use a regex that matches both keywords.
💚 Proposed fix
it('fails fast when a runtime filter would add a new predicate', () => {
- expect(() =>
- builder.buildQuery('SELECT id, name FROM users WHERE active = true', {
- filter: { name: 'Alice' }
- })
- ).toThrow(/ztd query sssql scaffold/i);
- expect(() =>
- builder.buildQuery('SELECT id, name FROM users WHERE active = true', {
- filter: { name: 'Alice' }
- })
- ).toThrow(/ztd query sssql refresh/i);
+ expect(() =>
+ builder.buildQuery('SELECT id, name FROM users WHERE active = true', {
+ filter: { name: 'Alice' }
+ })
+ ).toThrow(/ztd query sssql scaffold.*refresh|refresh.*scaffold/i);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('fails fast when a runtime filter would add a new predicate', () => { | |
| expect(() => | |
| builder.buildQuery('SELECT id, name FROM users WHERE active = true', { | |
| filter: { name: 'Alice' } | |
| }) | |
| ).toThrow(/ztd query sssql scaffold/i); | |
| expect(() => | |
| builder.buildQuery('SELECT id, name FROM users WHERE active = true', { | |
| filter: { name: 'Alice' } | |
| }) | |
| ).toThrow(/ztd query sssql refresh/i); | |
| }); | |
| it('fails fast when a runtime filter would add a new predicate', () => { | |
| expect(() => | |
| builder.buildQuery('SELECT id, name FROM users WHERE active = true', { | |
| filter: { name: 'Alice' } | |
| }) | |
| ).toThrow(/ztd query sssql scaffold.*refresh|refresh.*scaffold/i); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/tests/transformers/DynamicQueryBuilder.test.ts` around lines 28
- 39, The test incorrectly calls builder.buildQuery(...) twice with identical
inputs expecting two different error messages; update the assertion in
DynamicQueryBuilder.test (the case exercising builder.buildQuery) to assert a
single error that includes both keywords by using one expect(...).toThrow with a
regex that matches both "scaffold" and "refresh" (e.g.
/scaffold.*refresh|refresh.*scaffold/i) or assert the full static message
returned by the builder.buildQuery error; keep the reference to
builder.buildQuery and the same input payload.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/sql-grep-core/src/observed/match.ts`:
- Around line 252-257: collectQuerySummaries currently calls
splitQueries(sqlText).getNonEmpty() outside any try/catch so a malformed SQL
file can throw and abort the whole scan; wrap the splitQueries(...) call in a
try/catch inside collectQuerySummaries, and on error push an
ObservedSqlMatchWarning (including sqlFileLabel and the caught error message)
into the warnings array and return an empty Array<{ summary:
ObservedSqlQuerySummary; queryText: string }> so the rest of the project
continues to be processed.
- Around line 590-595: The code currently returns different predicate-shape
strings for ParameterExpression and LiteralValue (using literalKind), causing
bound params and concrete literals to be treated as different shapes; update the
branch that handles LiteralValue (the candidate instanceof LiteralValue case) to
normalize concrete literals to the same shape as parameters (e.g., return
'value:param') so both ParameterExpression and LiteralValue produce the same
predicate shape; modify the logic in match.ts where candidate is inspected (and
any use of literalKind for predicate-shape purposes) to ensure LiteralValue maps
to 'value:param' when computing predicate shapes.
In `@packages/ztd-cli/src/commands/query.ts`:
- Around line 510-516: The scaffold command silently no-ops when no filters are
provided; update runQuerySssqlScaffoldCommand to validate inputs after resolving
options (including JSON payload via parseJsonPayload) and the call to
normalizeSssqlFilters, and if the resulting filters object is empty reject/exit
with a clear error message instead of calling new
SSSQLFilterBuilder().scaffold(sql, filters); ensure the check references
normalizeSssqlFilters(...) result (filters) and error out (throw or process.exit
with non-zero) and mention expected --filter/--filters (or --json payload) so
users know how to provide filters.
🪄 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: 507e1fba-4c3c-40c9-a580-c8b82d8a5bb7
📒 Files selected for processing (5)
packages/sql-grep-core/src/index.tspackages/sql-grep-core/src/observed/match.tspackages/sql-grep-core/src/observed/types.tspackages/sql-grep-core/tests/observedSqlMatch.test.tspackages/ztd-cli/src/commands/query.ts
✅ Files skipped from review due to trivial changes (1)
- packages/sql-grep-core/src/observed/types.ts
Comment on lines
+252
to
+257
| function collectQuerySummaries( | ||
| sqlText: string, | ||
| sqlFileLabel: string, | ||
| warnings: ObservedSqlMatchWarning[] | ||
| ): Array<{ summary: ObservedSqlQuerySummary; queryText: string }> { | ||
| const queries = splitQueries(sqlText).getNonEmpty(); |
There was a problem hiding this comment.
Catch splitQueries() failures inside collectQuerySummaries.
splitQueries(sqlText) runs before the per-query try/catch, so one malformed .sql asset can abort the entire project scan before a warning is recorded. That makes match-observed brittle even though the remaining candidates could still be scored.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/sql-grep-core/src/observed/match.ts` around lines 252 - 257,
collectQuerySummaries currently calls splitQueries(sqlText).getNonEmpty()
outside any try/catch so a malformed SQL file can throw and abort the whole
scan; wrap the splitQueries(...) call in a try/catch inside
collectQuerySummaries, and on error push an ObservedSqlMatchWarning (including
sqlFileLabel and the caught error message) into the warnings array and return an
empty Array<{ summary: ObservedSqlQuerySummary; queryText: string }> so the rest
of the project continues to be processed.
There was a problem hiding this comment.
Actionable comments posted: 2
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/repositoryTelemetry.ts (1)
36-43:⚠️ Potential issue | 🟡 MinorUpdate the resolver comment to match the new noop default.
defaultRepositoryTelemetryis now created withcreateNoopRepositoryTelemetry(), but the JSDoc still says callers get a "default console hook." That will send scaffold users looking for logs that never fire.🤖 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 JSDoc above the resolver to reflect that defaultRepositoryTelemetry is a no-op: change the wording that currently claims a "default console hook" to state that the default is a no-op telemetry implementation created by createNoopRepositoryTelemetry(), and clarify that callers who need logging should supply their own telemetry instance to the resolver; reference defaultRepositoryTelemetry and createNoopRepositoryTelemetry in the comment so future readers know the actual default behavior.packages/ztd-cli/templates/src/infrastructure/telemetry/consoleRepositoryTelemetry.ts (1)
50-52:⚠️ Potential issue | 🟠 MajorDon't emit raw
errorMessagefrom the conservative console sink.This breaks the safety guarantee in the module docs: upstream database errors often include literal values and sometimes query fragments in
message. If an application wants full error text, it can opt into that with its own telemetry implementation instead of getting it from the default console serializer.🔒 Proposed safe default
if (event.kind === 'query.execute.error') { payload.errorName = event.errorName; - payload.errorMessage = event.errorMessage; }Based on learnings "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/templates/src/infrastructure/telemetry/consoleRepositoryTelemetry.ts` around lines 50 - 52, The consoleRepositoryTelemetry sink is emitting raw event.errorMessage for 'query.execute.error' which can leak query/text; in consoleRepositoryTelemetry.ts (look for the branch checking event.kind === 'query.execute.error' and the payload.errorMessage assignment) stop assigning raw event.errorMessage to payload.errorMessage — either remove that field entirely or replace it with a safe placeholder/sanitized value (e.g., payload.errorMessage = '[redacted]' or a truncated/hashed token) and keep payload.errorName/errorCode as needed so default console telemetry never emits full error text.
♻️ Duplicate comments (1)
packages/sql-grep-core/src/observed/match.ts (1)
133-135:⚠️ Potential issue | 🟠 MajorGuard
splitQueries()so one bad SQL input doesn't abort the entire report.Lines 135 and 257 call
splitQueries(...).getNonEmpty()before any warning/continue path. If one candidate asset or the observed SQL has an unterminated literal/comment,match-observedcan fail before it records a warning or ranks the remaining candidates. Please catch the split itself and keep scanning, then add a malformed-SQL regression test for this path.Also applies to: 252-257
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sql-grep-core/src/observed/match.ts` around lines 133 - 135, Wrap calls to splitQueries(...).getNonEmpty() in a try/catch at both call sites (the candidateFiles loop where sqlText is read and the observed-SQL parsing path) so a parsing error for one SQL asset doesn’t abort the whole run; on catch, record a warning/diagnostic (using the same warning collection or logger used elsewhere in this module) that identifies the file/asset and the parse error, then continue to the next candidate instead of throwing. Add a regression test that feeds malformed SQL (unterminated literal/comment) to the relevant match function and asserts a warning was recorded and remaining candidates were still processed.
🤖 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/sql-grep-core/src/observed/match.ts`:
- Around line 708-720: The compareTokenSets function treats two empty token sets
as a zero-score match; update compareTokenSets (the function defined as
compareTokenSets(observed: string[], candidate: string[])) to mirror
comparePagingTokens by returning a score of 1 when both observedSet.size and
candidateSet.size are zero, while keeping intersection, missing, and extra as
currently computed; implement a simple early check (if observedSet.size === 0 &&
candidateSet.size === 0) to set score = 1 before computing/returning the result.
In `@packages/ztd-cli/src/commands/init.ts`:
- Around line 1045-1095: The README advertises telemetry files but the telemetry
writeTemplateFile calls (telemetryTypes, telemetryRepository,
telemetryConsoleRepository and infrastructureReadme) are currently only executed
in the starter-only branch, causing docs to reference files that aren't
scaffolded; fix by moving these writeTemplateFile calls (and the summaries
assignments) out of the starter-only conditional so they run for all workflows
(or alternatively remove telemetry references from the shared README template),
and ensure the dry-run behavior remains consistent with the chosen approach;
locate the calls to writeTemplateFile for
telemetryTypes/telemetryRepository/telemetryConsoleRepository and adjust their
surrounding conditional logic in init.ts accordingly.
---
Outside diff comments:
In
`@packages/ztd-cli/templates/src/infrastructure/telemetry/consoleRepositoryTelemetry.ts`:
- Around line 50-52: The consoleRepositoryTelemetry sink is emitting raw
event.errorMessage for 'query.execute.error' which can leak query/text; in
consoleRepositoryTelemetry.ts (look for the branch checking event.kind ===
'query.execute.error' and the payload.errorMessage assignment) stop assigning
raw event.errorMessage to payload.errorMessage — either remove that field
entirely or replace it with a safe placeholder/sanitized value (e.g.,
payload.errorMessage = '[redacted]' or a truncated/hashed token) and keep
payload.errorName/errorCode as needed so default console telemetry never emits
full error text.
In
`@packages/ztd-cli/templates/src/infrastructure/telemetry/repositoryTelemetry.ts`:
- Around line 36-43: Update the JSDoc above the resolver to reflect that
defaultRepositoryTelemetry is a no-op: change the wording that currently claims
a "default console hook" to state that the default is a no-op telemetry
implementation created by createNoopRepositoryTelemetry(), and clarify that
callers who need logging should supply their own telemetry instance to the
resolver; reference defaultRepositoryTelemetry and createNoopRepositoryTelemetry
in the comment so future readers know the actual default behavior.
---
Duplicate comments:
In `@packages/sql-grep-core/src/observed/match.ts`:
- Around line 133-135: Wrap calls to splitQueries(...).getNonEmpty() in a
try/catch at both call sites (the candidateFiles loop where sqlText is read and
the observed-SQL parsing path) so a parsing error for one SQL asset doesn’t
abort the whole run; on catch, record a warning/diagnostic (using the same
warning collection or logger used elsewhere in this module) that identifies the
file/asset and the parse error, then continue to the next candidate instead of
throwing. Add a regression test that feeds malformed SQL (unterminated
literal/comment) to the relevant match function and asserts a warning was
recorded and remaining candidates were still processed.
🪄 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: 16572dd3-cd86-4c14-8858-ff5a69ff91aa
⛔ Files ignored due to path filters (1)
packages/ztd-cli/tests/__snapshots__/describe.cli.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (15)
docs/guide/feature-index.mddocs/guide/observed-sql-matching.mdpackages/core/src/transformers/DynamicQueryBuilder.tspackages/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
✅ Files skipped from review due to trivial changes (4)
- packages/ztd-cli/templates/src/infrastructure/README.md
- packages/sql-grep-core/README.md
- packages/ztd-cli/templates/README.md
- docs/guide/observed-sql-matching.md
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/core/src/transformers/DynamicQueryBuilder.ts
- packages/ztd-cli/src/commands/query.ts
- packages/ztd-cli/tests/cliCommands.test.ts
Comment on lines
+708
to
+720
| function compareTokenSets(observed: string[], candidate: string[]): { score: number; intersection: string[]; missing: string[]; extra: string[] } { | ||
| const observedSet = new Set(observed); | ||
| const candidateSet = new Set(candidate); | ||
| const intersection = [...observedSet].filter((token) => candidateSet.has(token)); | ||
| const missing = [...observedSet].filter((token) => !candidateSet.has(token)); | ||
| const extra = [...candidateSet].filter((token) => !observedSet.has(token)); | ||
| const denominator = Math.max(observedSet.size, candidateSet.size, 1); | ||
| return { | ||
| score: intersection.length / denominator, | ||
| intersection, | ||
| missing, | ||
| extra | ||
| }; |
There was a problem hiding this comment.
Treat empty-vs-empty sections as a full match.
Lines 714-716 return 0 when both token sets are empty. That makes an otherwise exact match with no WHERE and no ORDER BY cap out at 73.0, and it gives the same section score as a candidate that adds extra clauses. compareTokenSets() should mirror comparePagingTokens() here.
🐛 Proposed fix
function compareTokenSets(observed: string[], candidate: string[]): { score: number; intersection: string[]; missing: string[]; extra: string[] } {
const observedSet = new Set(observed);
const candidateSet = new Set(candidate);
const intersection = [...observedSet].filter((token) => candidateSet.has(token));
const missing = [...observedSet].filter((token) => !candidateSet.has(token));
const extra = [...candidateSet].filter((token) => !observedSet.has(token));
+
+ if (observedSet.size === 0 && candidateSet.size === 0) {
+ return {
+ score: 1,
+ intersection,
+ missing,
+ extra
+ };
+ }
+
const denominator = Math.max(observedSet.size, candidateSet.size, 1);
return {
score: intersection.length / denominator,
intersection,
missing,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function compareTokenSets(observed: string[], candidate: string[]): { score: number; intersection: string[]; missing: string[]; extra: string[] } { | |
| const observedSet = new Set(observed); | |
| const candidateSet = new Set(candidate); | |
| const intersection = [...observedSet].filter((token) => candidateSet.has(token)); | |
| const missing = [...observedSet].filter((token) => !candidateSet.has(token)); | |
| const extra = [...candidateSet].filter((token) => !observedSet.has(token)); | |
| const denominator = Math.max(observedSet.size, candidateSet.size, 1); | |
| return { | |
| score: intersection.length / denominator, | |
| intersection, | |
| missing, | |
| extra | |
| }; | |
| function compareTokenSets(observed: string[], candidate: string[]): { score: number; intersection: string[]; missing: string[]; extra: string[] } { | |
| const observedSet = new Set(observed); | |
| const candidateSet = new Set(candidate); | |
| const intersection = [...observedSet].filter((token) => candidateSet.has(token)); | |
| const missing = [...observedSet].filter((token) => !candidateSet.has(token)); | |
| const extra = [...candidateSet].filter((token) => !observedSet.has(token)); | |
| if (observedSet.size === 0 && candidateSet.size === 0) { | |
| return { | |
| score: 1, | |
| intersection, | |
| missing, | |
| extra | |
| }; | |
| } | |
| const denominator = Math.max(observedSet.size, candidateSet.size, 1); | |
| return { | |
| score: intersection.length / denominator, | |
| intersection, | |
| missing, | |
| extra | |
| }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/sql-grep-core/src/observed/match.ts` around lines 708 - 720, The
compareTokenSets function treats two empty token sets as a zero-score match;
update compareTokenSets (the function defined as compareTokenSets(observed:
string[], candidate: string[])) to mirror comparePagingTokens by returning a
score of 1 when both observedSet.size and candidateSet.size are zero, while
keeping intersection, missing, and extra as currently computed; implement a
simple early check (if observedSet.size === 0 && candidateSet.size === 0) to set
score = 1 before computing/returning the result.
Comment on lines
+1045
to
+1095
| const infrastructureReadmeSummary = await writeTemplateFile( | ||
| rootDir, | ||
| absolutePaths.infrastructureReadme, | ||
| relativePath('infrastructureReadme'), | ||
| scaffoldLayout.infrastructureReadmeTemplate, | ||
| dependencies, | ||
| prompter, | ||
| overwritePolicy | ||
| ); | ||
| if (infrastructureReadmeSummary) { | ||
| summaries.infrastructureReadme = infrastructureReadmeSummary; | ||
| } | ||
|
|
||
| const telemetryTypesSummary = await writeTemplateFile( | ||
| rootDir, | ||
| absolutePaths.telemetryTypes, | ||
| relativePath('telemetryTypes'), | ||
| scaffoldLayout.telemetryTypesTemplate, | ||
| dependencies, | ||
| prompter, | ||
| overwritePolicy | ||
| ); | ||
| if (telemetryTypesSummary) { | ||
| summaries.telemetryTypes = telemetryTypesSummary; | ||
| } | ||
|
|
||
| const telemetryRepositorySummary = await writeTemplateFile( | ||
| rootDir, | ||
| absolutePaths.telemetryRepository, | ||
| relativePath('telemetryRepository'), | ||
| scaffoldLayout.telemetryRepositoryTemplate, | ||
| dependencies, | ||
| prompter, | ||
| overwritePolicy | ||
| ); | ||
| if (telemetryRepositorySummary) { | ||
| summaries.telemetryRepository = telemetryRepositorySummary; | ||
| } | ||
|
|
||
| const telemetryConsoleRepositorySummary = await writeTemplateFile( | ||
| rootDir, | ||
| absolutePaths.telemetryConsoleRepository, | ||
| relativePath('telemetryConsoleRepository'), | ||
| scaffoldLayout.telemetryConsoleRepositoryTemplate, | ||
| dependencies, | ||
| prompter, | ||
| overwritePolicy | ||
| ); | ||
| if (telemetryConsoleRepositorySummary) { | ||
| summaries.telemetryConsoleRepository = telemetryConsoleRepositorySummary; | ||
| } |
There was a problem hiding this comment.
Generate the telemetry seam everywhere the shared README advertises it.
README.md is written for every workflow from the shared README_TEMPLATE at Line 809, and those new telemetry strings cannot be coming from STARTER_README_APPENDIX because that appendix never mentions repository telemetry, queryId, paramsShape, or transformations. Keeping these file writes inside the starter-only branch means empty/demo/pg_dump scaffolds now document src/infrastructure/telemetry/* without actually creating it. Please either scaffold these files for all workflows or move the telemetry guidance out of the shared README (and keep --dry-run aligned with whichever behavior you choose).
🤖 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 1045 - 1095, The README
advertises telemetry files but the telemetry writeTemplateFile calls
(telemetryTypes, telemetryRepository, telemetryConsoleRepository and
infrastructureReadme) are currently only executed in the starter-only branch,
causing docs to reference files that aren't scaffolded; fix by moving these
writeTemplateFile calls (and the summaries assignments) out of the starter-only
conditional so they run for all workflows (or alternatively remove telemetry
references from the shared README template), and ensure the dry-run behavior
remains consistent with the chosen approach; locate the calls to
writeTemplateFile for
telemetryTypes/telemetryRepository/telemetryConsoleRepository and adjust their
surrounding conditional logic in init.ts accordingly.
Owner
Author
|
Superseded by #679. |
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
ztd query match-observedto rank observed SELECT SQL against.sqlassets.Verification
pnpm --filter @rawsql-ts/sql-grep-core testpnpm --filter @rawsql-ts/ztd-cli test -- cliCommands.test.tspnpm --filter @rawsql-ts/ztd-cli test -- init.command.test.tspnpm --filter @rawsql-ts/ztd-cli test -- -u describe.cli.test.tspnpm --filter @rawsql-ts/ztd-cli buildSummary by CodeRabbit
Release Notes
New Features
ztd query sssql scaffoldandztd query sssql refreshCLI commands for SQL-first optional filter authoring workflowsztd query match-observedcommand to rank source SQL assets when queryId metadata is unavailableBreaking Changes