[ajitpratap0]

Summary

W0-02 of the perf plan, plus critique item 15 (positions, Normalize/Fingerprint and LSP coverage). The behaviour goldens freeze observable output over the whole internal/benchcorpus corpus (W0-01: 2,999 statements in 254 files: every golden except lsp has 254 rows, the 256 MANIFEST entries minus portable_queries.txt and known_failures.txt). Wave-1+ perf PRs can then prove "no behaviour change" (AC-STD 2) or list each intended diff.

Golden (testdata/goldens/)	Test	Rows x columns	Pinned per statement
tokens	pkg/sql/tokenizer TestGoldenTokens	file x 11 dialects (incl. bigquery, redshift)	every token of Tokenize([]byte): type, value, span, quote, Long, Word (nil-ness included); every captured comment (tkz.Comments). TokenizeString, tokscratch.String (the path gosqlx.Parse, ParseWithDialect and Validate use) and tokscratch.Bytes also run; a string: / scratch: / scratch_bytes: section is appended to the cell only where that path differs from the public dump (none today)
parse	pkg/sql/parser TestGoldenParse	file x (11 dialects + default)	ParseWithDialect → SQL(), or error code + location + hint + message; every models.Location in the AST (node Pos, StatementStarts/Ends, comment spans), from a reflective outline walk; ValidateWithDialect verdict. default column: the dialect-free entry points, which run separate code: gosqlx.Parse (same SQL()/positions, errors with its ErrTokenize/ErrSyntax wrapping), the parser.Validate fast path and gosqlx.Validate
format	pkg/formatter TestGoldenFormat	file x 6	FormatWithDialect (default and upper/indent-4/semicolon), FormatASTChecked compact / readable / lower / compact-no-dialect
fingerprint	pkg/fingerprint TestGoldenFingerprint	file x 2	Normalize, Fingerprint values
lint	pkg/linter TestGoldenLint	file x 2	every violation (rule, name, severity, location, message, suggestion, autofix, line) of LintWithDialect (own dialect) and Lint
security	pkg/sql/security TestGoldenSecurity	file x 2	ScanSQL and Scan (AST) findings and counts
transpile	pkg/transpiler TestGoldenTranspile	file x 9 targets	Transpile from the corpus dialect to every dialect of the 9x9 rule matrix, identity included (the corpora cover all 9 sources, so all 81 pairs run)
lsp	pkg/lsp TestGoldenLSP	doc@dialect x 3	published diagnostics, semantic tokens decoded with the advertised legend, document symbols. Documents: ~30 KB realworld script per dialect (under its own dialect and under generic), the generic corpora (under generic, bigquery, redshift), the feature samples, and a multi-error doc with non-ASCII text, tabs and CRLF

Compact format. Each line is <corpus file> <statements> <64-bit hash per column>. A cell hash covers the full output of every statement in the file, in order. = means "same hash as the column to the left", so dialects that agree collapse and the ones that differ stand out. All eight files total 173 KB.

Readable diff mode.

• A mismatch lists the changed cells (row, column, old hash → new hash).
• task goldens:diff BASE=origin/main (scripts/goldens-diff.sh) dumps the base revision's outputs from a temporary worktree (-goldens.dump). It then runs the working tree against that dump (-goldens.base) and prints, for each changed cell, the first differing statements: the input SQL and a line diff of the output (examples below).

Flags and tasks:

• -goldens.update regenerates the goldens (task goldens:update). The flag is namespaced like -goldens.dump/-goldens.base so it cannot clash with a package's own -update flag.
• task goldens runs them.
• Before comparing, the goldens check that the corpus still matches MANIFEST.sha256, so a corpus edit cannot pass for a behaviour change.

Robustness:

• Statements are processed in parallel on GOMAXPROCS workers with index-keyed results, so output does not depend on scheduling. This also exercises the pools under -race.
• A panic is recorded as a PANIC: <value> output, without a stack, which would be nondeterministic. Every library call runs under goldens.Safe, including the formatter golden's ParseWithDialect and the LSP golden's NewServer.

Also added (critique 15): pkg/sql/ast/span_invariant_test.go fails if any non-test code can write the unsynchronised global spanInfo map, which would be a race under concurrent use: any reference to SetSpan (call or method value), with the import resolved by path so aliased and dot imports count, and inside package ast any spanInfo write (index assignment, ++/--, reassignment, delete/clear) outside SetSpan. TestSpanWritesDetector checks the detector on 14 synthetic sources.

internal/goldens holds the shared infrastructure (Golden, Render/Compare/Report, Dump, the token/position/error renderers, Corpus, Parallel, Safe) and has its own unit tests.

Test Plan

Gates (AC-STD), all on this branch at 29b95cc (origin/main b420e2d merged in, which includes #907 ALTER TABLE commas and #912):

• task check (fmt, vet, golangci-lint with 0 issues, test:race): pass.
• task test:cbinding: pass.
• go test -count=1 ./pkg/compatibility/...: pass.
• go run honnef.co/go/tools/cmd/staticcheck@latest ./internal/goldens ./pkg/sql/ast ./pkg/sql/tokenizer ./pkg/sql/parser ./pkg/formatter ./pkg/fingerprint ./pkg/linter ./pkg/sql/security ./pkg/transpiler ./pkg/lsp: clean.

Review follow-up (29b95cc):

• task goldens:update then task goldens: pass. Changed cells: every tokens cell (comments are now rendered; no scratch/string section appears, so all paths agree today) and the new parse [default] column. Every existing parse dialect cell is byte-identical to the previous commit (checked by stripping the last column). The other six files change only in the header line (flag rename).
• Goldens still pass on current origin/main: the branch now has b420e2d merged in (it includes 4def1cb, fix(ast): separate ALTER TABLE actions with commas in SQL() [W5-02b] #907 ALTER TABLE commas), with no dialect-column changes.
• GOMAXPROCS=1 go test -count=2 -run '^TestGolden' (tokenizer, parser, formatter, lsp): pass. go test -race -run '^TestGolden|TestSpan' (tokenizer, parser, formatter, lsp, ast, internal/goldens): pass.
• New mutations, reverted afterwards: gosqlx.Parse error wrap "%w: %w" → "%w; %w" fails parse in 90 [default] cells (no dialect cell). The scratch path setting Token.Word = nil on non-keyword words (SQL() unchanged) fails tokens in 2794 cells.

Stability:

• go test -count=3 -run '^TestGolden' <8 golden pkgs>: pass (3 runs).
• GOMAXPROCS=1 go test -count=1 -run '^TestGolden' ...: pass. Output does not depend on parallelism.
• go test -race -count=1 -run '^TestGolden' ...: pass. Race timings on an M4 (10 cores, load ~13): tokens 3.8s, parse 6.1s, format 1.2s, fingerprint 0.8s, lint 1.4s, security 1.1s, transpile 3.4s, lsp 1.0s. Non-race, each is under 1s.

Mutation sensitivity. One mutation was injected into library code per golden. The matching golden was run against a clean base dump (-goldens.base), and the mutation was reverted afterwards. Every golden failed:

Golden	Injected mutation	Result
tokens	double-quoted identifier fast path sets Token.Quote: 0 (value unchanged)	FAIL, 280 cells
parse	ClickHouse ARRAY JOIN Pos column + 1 (SQL() unchanged)	FAIL, 1 cell
format	comma-join FROM list rendered as " , "	FAIL, 237 cells
fingerprint	digest %x → %X (Normalize unchanged)	FAIL, 237 cells (fingerprint column only)
lint	L016 suggestion text edited	FAIL, 173 cells
security	AST time-based finding severity HIGH → MEDIUM	FAIL, 2 cells
transpile	PostgreSQL→SQLite stops rewriting BIGSERIAL	FAIL, 1 cell ([sqlite] of one file)
lsp	semantic tokens drop readonly on TRUE/FALSE/NULL	FAIL, 23 cells

Diff-mode output for the position-only mutation:

--- FAIL: TestGoldenParse (0.21s)
    parse_golden_test.go:35: golden "parse" changed: 1 cell(s) differ
          realworld/clickhouse/05_dialect_specific.sql [clickhouse]: a5b07db7fa151169 -> 4076ddf6f257d3ac
        === realworld/clickhouse/05_dialect_specific.sql [clickhouse]
        --- statement #7
          input: SELECT id, tag FROM t ARRAY JOIN tags AS tag
                Elements[0]
                  Expr(Identifier)
                    Pos 2:34
          -     Pos 2:23
          +     Pos 2:24

and for the transpile mutation:

        === realworld/postgresql/03_ddl_migrations.sql [sqlite]
        --- statement #0
          input: -- Schema migrations as generated by Rails, ... CREATE TABLE users (id BIGSERIAL PRIMARY KEY, ...
          - CREATE TABLE users (id INTEGER PRIMARY KEY, email VARCHAR(255) NOT NULL UNIQUE, ...
          + CREATE TABLE users (id BIGSERIAL PRIMARY KEY, email VARCHAR(255) NOT NULL UNIQUE, ...

scripts/goldens-diff.sh HEAD was also run end to end with the transpile mutation in the working tree. It printed the same statement diff and removed its temporary worktree.

The SetSpan invariant test also fails when ast.SetSpan(clause, ...) is added to parseArrayJoin.

internal/goldens unit tests cover:

• order-independent rendering under concurrent Add
• Compare reporting exactly the mutated cell, including cells rendered as =
• row and column changes
• the base-dump diff report
• the dump round trip, including adversarial content
• LineDiff
• token, position and error renderers
• Safe and Parallel

Breaking Changes

None. The PR adds tests, test data, a script and three Taskfile tasks; no library code changes. CHANGELOG is left for the consolidated W0-12 entry. Golden outputs reflect origin/main at b420e2d (merged into the branch), which includes the W0-00a lambda/U& fixes and #907.

🤖 Generated with Claude Code

https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment

Project	Deployment	Actions	Updated
gosqlx-website	Ignored	Preview	Nov 1, 2026 8:22am UTC

[github-actions]

Claude encountered an error after 0s —— View job

---

I'll analyze this and get back to you.

[github-actions]

Claude finished @ajitpratap0's task in 29s —— View job

---

Review of PR #915: behaviour goldens [W0-02]

I read internal/goldens/dump.go, scripts/goldens-diff.sh and pkg/sql/ast/span_invariant_test.go in full. I did not read the other golden test files or the corpus plumbing, and I did not run the tests. The PR's own test plan reports them passing, and I haven't checked that.

Overall: the design is sound. One hash per file and column keeps the committed data to about 168 KB. The MANIFEST.sha256 check stops a corpus edit from passing as a behaviour change. Index-keyed parallel results make the output independent of scheduling. The mutation table is good evidence that each golden catches the change it targets.

Observations

1. scripts/goldens-diff.sh:30-31 hides base-run failures.

   • The base run ends in || true with output sent to /dev/null. If the base revision builds only partly, or one package panics before dumping, a partial dump directory still passes the [ -d "$tmp/dump" ] check.
   • The comparison then reports cells as changed or missing without saying why.
   • Suggestion: keep the base run's log in $tmp and print its tail when the comparison fails. Alternatively, check that a dump file exists for each of the 8 goldens.
   • Related: origin/main has no goldens until this PR merges, so the script exits 2 until then. That is handled, but worth a line in the README.

2. internal/goldens/dump.go:70 derives modulePrefix by slicing a package path at a fixed length.

   • reflect.TypeOf(ast.AST{}).PkgPath()[:len("github.com/ajitpratap0/GoSQLX")] breaks silently if the module path or a fork's path changes. The slice may panic or give a wrong prefix, and inModule would then skip every type and render empty position output.
   • Suggestion: use strings.TrimSuffix(pkgPath, "/pkg/sql/ast"). Also add a unit test asserting Positions returns non-empty output for a simple SELECT, so an empty walk cannot pass.

3. mayHold caching (dump.go:201-235) is correct but subtle.

   • A false found mid-recursion is not cached unless the call is the outermost one, which is right.
   • This relies on len(visiting) == 0, so a short comment explaining why would help the next reader.
   • The cache is shared through sync.Map and the visiting map is per call, so it is race-safe.

4. span_invariant_test.go:72 identifies package ast by directory name.

   • inAST is Name == "ast" plus a parent directory named ast. Any other module package at */ast with package ast would get the bare-identifier checks too, which could give false positives on an identifier named SetSpan.
   • Today it only matches pkg/sql/ast, so this is theoretical. Comparing the directory path against pkg/sql/ast relative to the root would be stricter.
   • The detector cases are good, and they stop the invariant from passing by missing a write.

5. Positions assumes position data lives only in models.Location fields.

   • A node that stores a position as a bare int offset, or in the global spanInfo, is outside this walk.
   • The SetSpan test covers spanInfo. If the AST later gains another position representation, the parse golden's claim that every position is pinned will quietly stop being true. The README could say so.

6. Cost under -race. Parse takes about 6 s and tokens about 3.8 s on a 10-core machine, per the PR description. Hosted CI runners with fewer cores will be slower. Check that the package-level test timeouts (the -timeout 60s in CLAUDE.md) still hold in CI for pkg/sql/parser.

7. Housekeeping.

   • CLAUDE.md says the CHANGELOG should be updated for new features. The PR defers this to W0-12, which is reasonable for test-only changes.
   • Please confirm task test:race includes internal/goldens, since it has its own unit tests.

Security

Nothing of concern. The change adds tests, test data and a script. The script uses mktemp -d, quotes its variables, and removes its worktree through a trap. It creates a git worktree from a user-supplied ref, which is acceptable for a developer tool.

Verdict

I found no blocking issues. Items 1 and 2 are the ones I'd fix before merging, and both are small. The rest are optional.

[github-actions]

Claude finished @ajitpratap0's task in 14s —— View job

---

Review of PR #915: behaviour goldens [W0-02]

I re-read scripts/goldens-diff.sh and the modulePrefix use in internal/goldens/dump.go. I did not run the tests. The earlier review's findings still apply to the current code.

Overall: the design is sound and I found nothing blocking. It adds only tests, test data, a script and Taskfile tasks, so there is no library risk. The MANIFEST.sha256 check, index-keyed parallel results and per-golden mutation evidence are strong.

Worth fixing before merge (both small)

1. scripts/goldens-diff.sh:30-31 discards the base run's output and exit status.

   • A partial dump, for example one package panicking or failing to build, still passes the [ -d "$tmp/dump" ] check.
   • The comparison then reports confusing changed or missing cells.
   • Fix: write the base run's log to $tmp/base.log and print its tail if the comparison fails. Alternatively, require a dump file for each of the 8 goldens.

2. internal/goldens/dump.go:70 slices the package path at a fixed length.

   • PkgPath()[:len("github.com/ajitpratap0/GoSQLX")] assumes that module path. A fork or renamed module can panic or give a wrong prefix.
   • A wrong prefix makes inModule (line 189) skip every type, so the position walk returns empty output and the parse golden pins nothing.
   • Fix: strings.TrimSuffix(reflect.TypeOf(ast.AST{}).PkgPath(), "/pkg/sql/ast"). Add a unit test that Positions returns non-empty output for a simple SELECT.

Optional

• Add a comment on mayHold explaining why a false is cached only when len(visiting) == 0.
• span_invariant_test.go finds package ast by name and parent directory name. Compare against pkg/sql/ast relative to the module root instead.
• Say in the README that the parse golden covers only models.Location fields. Any other position representation is not covered.
• Check that -race runtimes (parse about 6 s, tokens about 3.8 s on 10 cores) fit CI timeouts on smaller runners.
• Confirm that task test:race includes internal/goldens.
• Note in the README that goldens:diff exits 2 against a base that predates the goldens.

Security

Nothing of concern. The script uses mktemp -d, quotes its variables and removes the worktree in a trap.