360 lines
42 KiB
Markdown
360 lines
42 KiB
Markdown
# Project audit 1
|
||
|
||
Date: 2026-10-12
|
||
Revision reviewed: `bfe98528265b45be7bfdcdf025710981bae06a7b`
|
||
Perspectives: software engineering, security, maintenance, and databases.
|
||
|
||
## Repair progress
|
||
|
||
Implementation batches (2026-10-12):
|
||
|
||
- [x] **A01, PostgreSQL failure propagation:** stop on the first SQL failure by default; explicit continuation still returns a structured `ExecutionFailure`. Reports include skipped entries, and requested report write/close failures are errors. Live tests cover a middle-statement failure in both modes and verify dependent jobs do not start.
|
||
- [x] **A02, implicit replacement removed:** both full-DDL and diff type changes propagate conversion errors without renaming/replacing the column. Live populated-data tests verify original values, column count/type, unique-index identity, and constraint enforcement survive incompatible conversions.
|
||
- [x] **Related defect exposed by A01:** full-DDL column additions now use native `ADD COLUMN IF NOT EXISTS`, fixing duplicate-column errors for flattened output that previously appeared successful.
|
||
- [x] **A01, shared execution results:** PostgreSQL, SQLite, SQL Server, and raw script execution now use `writers.ExecutionReport` / `ExecutionFailure`. Stop/continue modes always fail on SQL errors; counters/results survive early failure and aggregate across script schemas. Reports distinguish applied, failed, skipped, rolled-back, and uncertain operations. Generated DDL supports optional atomic execution; raw scripts report script-managed outcomes without promising statement-level commit/rollback. Requested report failures are errors in every executor. Live PostgreSQL, SQL Server, SQLite, and dependent-job regressions pass.
|
||
- [x] **A02, conversion expressions and preflight:** column metadata `pgsql_using` configures an explicit SQL conversion expression; `pgsql_preflight` adds a row predicate. Full-DDL and diff plans evaluate every existing value before schema mutations, and live preflight failure stops even with continuation enabled. Tests cover later invalid rows, preservation of a constraint scheduled for removal, custom conversions, predicate rejection, invalid configuration, and block-delimiter collisions. Standalone schema generation also includes preflights.
|
||
|
||
- [x] **A03, contained template output:** multi-file output has an explicit directory root, portable relative filename validation, and preflight collision/symlink checks. Rendering/validation complete before writing; exclusive per-file staging and root-scoped rename prevent generated filenames from escaping the root. Multi-file atomic publication remains A16 work.
|
||
- [x] **A04, contained asset reads:** embed directives and manifest execution use shared root-scoped reads, symlink checks, regular-file validation, and a bounded 64 MiB default. Manifest paths are rechecked at execution. Imported source-path metadata cannot authorize file reads without a trusted configured asset root. Library callers can explicitly configure a root/size limit; directives cannot opt out of containment.
|
||
- [x] **A05, PostgreSQL quoting:** schema/full-DDL/diff paths and embedded templates always quote model identifiers, quote literal/catalog/regclass values, sanitize names in generated line comments, and choose collision-safe dollar delimiters for procedural and audit function bodies. NUL is rejected before output; migration comparisons preserve identifier case. Raw SQL expressions remain an explicit executable-input boundary.
|
||
- [x] **Requested lowercase option:** `--lowercase` on convert/batch/merge/split/templ/job run, job `options.lowercase_identifiers`, and PostgreSQL `WriterOptions.LowercaseIdentifiers` normalize names and structured references on a copy, rejecting collisions. SQL expressions, defaults, comments, enum values, and data keep their case; callers must adjust references inside raw SQL themselves.
|
||
- [x] **A08, SQL-aware substitution/execution:** embeds use one-pass literal substitution, preserving dollar characters and preventing replacement rescanning. Named parameters and embed directives respect SQL strings, quoted identifiers, nested comments, casts, and dollar bodies; duplicate/unused embed variables fail. MySQL executes structured generated statements without splitting multiline literals or stripping their contents.
|
||
|
||
- [x] **A06, nullable MySQL catalogs:** constraint references, functional-index columns/expressions, index prefix/direction fields, column dimensions/collations/generation metadata, and optional table auto-increment counters use nullable destinations. Queries qualify joined columns. Live primary/unique/composite/functional-key reads pass, including unsigned auto-increment values beyond signed 64-bit range.
|
||
- [x] **A07, scoped MySQL fidelity and diagnostics:** native types/default spellings and typed dimensions/index parts survive MySQL → model → JSON/YAML → DDL. Output emits foreign keys/actions after all tables, checks/enforcement, unique/ordinary/functional/prefix/descending/invisible/fulltext indexes, generated columns, comments/collations/charsets, ON UPDATE, invisible columns, and auto-increment counters. Known unsupported objects/attributes produce warnings or `--strict-conversion` / job `options.strict_conversion` / library `StrictConversion` failures before output or connection. See [the scoped capability matrix](MYSQL_FIDELITY.md); this covers new-table DDL on MySQL 8.4 / InnoDB, not all engines, MariaDB versions, other format mappings, or migrations.
|
||
|
||
A06–A07 verification (2026-10-12): live MySQL 8.4.11 tests compare source/target `SHOW CREATE TABLE` definitions (normalizing temporary target suffixes and ordinary-index display order), re-read the target, and verify FK/check enforcement and binary/bit/multiline/backslash/default values. The suite covers composite/functional unique keys, reverse table dependency order, generated/check/index expressions containing SQL literals, precision, unsigned ranges, and multiple FKs to the same target. It exposed and fixed truncated binary defaults in `information_schema` by extracting native defaults from SQL-aware `SHOW CREATE TABLE` definitions. Native SHOW CREATE also avoids stale auto-increment statistics, and escaped catalog expressions are decoded before rendering. Unsupported views/triggers/partitioned tables produce warnings and strict failures; strict failure preserves existing files and happens before opening an output connection. CLI dry-run and per-job strict options are tested. The MySQL container test runner now selects reader/writer packages by default. `go test -mod=vendor -p 1 ./...` passed with `RELSPEC_TEST_MYSQL_CONN` configured; `go vet -mod=vendor ./...`, CLI strict/dry-run checks, shell syntax, and `git diff --check` passed. The MySQL container runner also passed using its schema-scoped account; its disposable server enables trigger-fixture creation after final-server readiness. PostgreSQL and SQL Server live variables were unset for this batch. Checks used `go1.27.1-X:nodwarf5 linux/amd64`; declared-minimum-toolchain coverage remains A13 work. Disposable MySQL instances were removed after verification. A08 now also has live MySQL coverage for embedded statement delimiters/default contents.
|
||
|
||
A05–A08 verification: unit regressions and live PostgreSQL 18 checks cover unusual names, comments/defaults, rerunnable file/statement output, diff migrations, sequences, primary-key replacement, audit functions/triggers, and both standard-string modes. MySQL statement preservation is unit-tested; a live MySQL instance was not used for A08. `go test -mod=vendor -p 1 ./...` passed with `RELSPEC_TEST_PG_CONN` set to the disposable instance; `go vet -mod=vendor ./...`, executable CLI help checks, and `git diff --check` passed. SQL Server tests were not configured for this batch. The disposable instance was removed after verification.
|
||
|
||
Additional observation: a parallel live full-suite run hit a PostgreSQL reader catalog race (`could not open relation with OID`) while another package's test dropped schema objects. The writer safely fell back to full DDL and its preflight still stopped all mutations, but the existing test assumed no successful read-only preflights before failure. Running live packages serially passed. Reader resilience to concurrent catalog changes remains outside A05/A08; A10 specifically concerns cancellation/timeouts.
|
||
|
||
A03–A04 verification: `go test -mod=vendor ./...`, targeted regression tests, `go vet -mod=vendor ./...`, and `git diff --check` passed. Database-dependent tests were not configured for this filesystem-only batch. Windows test binaries were cross-compiled for the template and asset packages; Windows runtime filesystem tests remain unperformed.
|
||
|
||
Verification: `go test -mod=vendor ./...` passed with `RELSPEC_TEST_PG_CONN` and `RELSPEC_TEST_MSSQL_CONN` pointing to disposable PostgreSQL 18 and SQL Server 2022 Developer instances; SQLite regressions use temporary database files. A minimal public table was seeded for the existing PostgreSQL `ReadTable` test, which requires a nonempty default schema. `go vet -mod=vendor ./...` and `git diff --check` passed. Executable CLI checks verified `scripts execute --ignore-errors`, `--continue-on-error`, and `--coe` all attempt a later script after failure and exit 1. Raw script tests intentionally make no statement-level rollback claim. A09 remains open for concurrency, history, and migration recovery; sequence/external side effects are outside transaction rollback guarantees. Original audit evidence is retained for historical context.
|
||
|
||
## Assessment
|
||
|
||
RelSpec has a useful architecture, broad format support, and substantial tests. Its most serious weaknesses are in the boundary between generating a schema and safely applying that schema to a real database. A successful process exit does not reliably mean a successful migration. Some conversions silently discard database semantics, and several file/SQL generation paths do not preserve the safety guarantees implemented elsewhere.
|
||
|
||
**Recommendation: fix the high-priority findings before relying on unattended production database writes or processing externally supplied specifications.** Continue using generated output with review and disposable database validation while these issues are addressed. This is a repository audit, not a certification of production readiness.
|
||
|
||
### Existing strengths
|
||
|
||
- Readers, writers, the intermediate model, diffing, inspection, and transformations are separated into packages.
|
||
- There are many unit and regression tests, including live PostgreSQL writer tests and SQLite tests.
|
||
- Job files have semantic validation, dependency-cycle detection, environment-based connection references, path containment checks, and symlink checks.
|
||
- Single-file job conversions already use temporary output followed by rename.
|
||
- The asset manifest executor binds file bytes as database parameters.
|
||
- The repository includes a private vulnerability reporting policy, vendored dependencies, and release-time vet/static analysis/vulnerability-check steps.
|
||
|
||
These controls should be extended and made consistent, rather than replaced wholesale.
|
||
|
||
## Scope and verification
|
||
|
||
Reviewed the CLI, job handling, database readers/writers, PostgreSQL templates and migration generation, asset embedding, generic templates, connection builder, updater, build/release workflows, documentation, and the VS Code extension. Review depth was greatest for database mutation and input/output trust boundaries; every parser and format combination was not exhaustively audited.
|
||
|
||
Checks performed:
|
||
|
||
| Check | Result |
|
||
| --- | --- |
|
||
| `GOCACHE=/tmp/relspec-audit-gocache go test -mod=vendor ./...` | Initial sandbox run failed because HTTP tests could not open loopback sockets. Approved rerun outside the sandbox passed. |
|
||
| `GOCACHE=/tmp/relspec-audit-gocache go vet -mod=vendor ./...` | Passed. |
|
||
| `make fmt-check GOLANGCI_LINT=false` | Incorrectly passed, confirming that a failed formatter can be hidden. |
|
||
| Temporary Go reproduction using exported package APIs | Confirmed template path escape, embed path escape, embed dollar corruption, invalid PostgreSQL identifier/literal rendering, omitted MySQL FK/index output, and placeholder matching inside SQL strings. |
|
||
|
||
The local compiler reported `go1.27.1-X:nodwarf5 linux/amd64`; `go.mod` requires `1.26.9`. This was not a verification under the declared minimum toolchain. No live PostgreSQL, MySQL, or SQL Server connection was configured for the audit. Tests requiring those connections skipped; a passing package does not demonstrate live database correctness. Race testing, fuzz campaigns, benchmarks, `staticcheck`, and a fresh `govulncheck` scan were not performed. No claims about specific dependency CVEs are made.
|
||
|
||
Reproduction files and logs were placed in `/tmp`; the only intended repository change is this document. No real credentials or user data were used in reproductions. Database injection and migration effects were assessed from code/generated SQL, not executed against a database.
|
||
|
||
### Priority and evidence notation
|
||
|
||
- **P1 / High:** address before unattended database use or handling less-trusted inputs; credible data-integrity, confidentiality, or integrity impact.
|
||
- **P2 / Medium:** schedule in the next maintenance cycle; operational reliability, reproducibility, or defense-in-depth issue.
|
||
- **P3 / Low:** documentation or developer-experience issue.
|
||
- **Reproduced:** a local executable check demonstrated the stated behavior.
|
||
- **Code-confirmed:** the implementation establishes the behavior, but the complete external-system scenario was not executed.
|
||
|
||
## Findings
|
||
|
||
### A01 — PostgreSQL writes report success after statement failures
|
||
|
||
**Repair status: addressed.** The shared executor now enforces stop/continue behavior and nonzero failure across PostgreSQL, SQLite, SQL Server, and raw scripts, including requested report errors. Reports include individual outcomes, skipped operations, confirmed rollbacks, and uncertain finalization. Atomic generated-DDL runs are optional; raw scripts explicitly report script-managed execution. A09 remains a separate migration-management/concurrency contract.
|
||
|
||
**P1 / High · Engineering, database · Code-confirmed**
|
||
|
||
Evidence: `pkg/writers/pgsql/writer.go:2200–2322`, especially `executeStatements` and `finishReport`; `pkg/writers/pgsql/live_execute_test.go:144` explicitly tests continuing after failure.
|
||
|
||
Every failed statement is recorded and execution continues. `finishReport` prints an error count but returns `nil`. Failure to write the requested report is also only a warning. This behavior is unconditional in this execution path. A deployment or dependent job can proceed after an incomplete schema change because the writer returned success.
|
||
|
||
**Fix:** return a structured error when any statement fails; stop on the first error by default. A continue-on-error option should control continuation, separately from the final exit status. Aggregate results consistently across PostgreSQL, SQLite, SQL Server, and script execution. Treat failure to produce an explicitly requested execution report as an error.
|
||
|
||
**Acceptance:** force a middle statement to fail; the CLI exits nonzero, dependent jobs do not run, and the report distinguishes applied, failed, skipped, and rolled-back operations. Test both stop and continue modes.
|
||
|
||
### A02 — Failed type conversion silently substitutes an empty column
|
||
|
||
**Repair status: addressed.** The shared type-change template propagates conversion errors. Configurable `pgsql_using` expressions and `pgsql_preflight` predicates are supported, with automatic whole-column cast evaluation before schema mutations. Full-DDL and diff live tests verify preservation of populated columns and unique dependencies, custom conversions, and rejected predicates. Preflight is advisory under concurrent writes; atomic generated-DDL execution is available for transactionally supported changes, and separately reviewed rename/replacement remains required.
|
||
|
||
**P1 / High · Database · Code-confirmed**
|
||
|
||
Evidence: `pkg/writers/pgsql/templates/alter_column_type_with_check.tmpl:18–27`; `pkg/writers/pgsql/migration_writer.go:491`; `pkg/writers/pgsql/writer.go:514`.
|
||
|
||
The type-change block catches `WHEN OTHERS`, renames the existing column, and adds a fresh column with the original name. For example, converting populated text values containing non-numeric data to an integer can fail the cast and activate this fallback. The application then sees an empty replacement column. Existing indexes, constraints, and dependencies may remain attached to the renamed column. Original values are not necessarily deleted, but the application's expected column no longer contains them.
|
||
|
||
The broad exception handler also treats unrelated failures as permission to attempt this schema rewrite.
|
||
|
||
**Fix:** fail on conversion errors. Require a separately reviewed migration for any rename-and-replace operation, including backfill, validation, dependency handling, and recovery. Support an explicit conversion expression and preflight checks where appropriate.
|
||
|
||
**Acceptance:** migrating populated incompatible data leaves the original column, values, and dependencies intact and returns an actionable failure. No rename/replacement occurs implicitly.
|
||
|
||
### A03 — Generated template filenames escape the output directory
|
||
|
||
**Repair status: addressed.** Multi-file roots, generated paths, collisions, symlinks, and root-scoped writes are covered by regression tests. A nonexistent output path now consistently denotes the directory itself rather than its parent.
|
||
|
||
**P1 / High · Security · Reproduced**
|
||
|
||
Evidence: `pkg/writers/template/writer.go:249–308`.
|
||
|
||
The default filename pattern uses `{{.Name}}.txt`. `generateFilename` joins the rendered name with the destination without checking containment; `writeOutput` creates directories and writes the resulting path. A schema named `../escaped` wrote `escaped.txt` beside the selected output directory in the reproduction.
|
||
|
||
An imported model can therefore cause writes outside the intended destination, including overwriting files writable by the current user. Job-level checks validate the declared output path, not every subsequently generated filename. Exploitation requires the user to process the supplied model in a multi-file template mode.
|
||
|
||
**Fix:** validate every rendered filename against a clearly defined output root; reject traversal, unsafe absolute paths, and symlink escapes. Detect filename collisions before writing. Prefer root-scoped filesystem operations so validation and opening cannot be separated by a symlink replacement.
|
||
|
||
**Acceptance:** malicious names, filename patterns, symlink parents, duplicate names, and platform-specific paths cannot write outside the root or silently overwrite another generated artifact.
|
||
|
||
### A04 — SQL embed directives can read outside the script directory
|
||
|
||
**Repair status: addressed.** Shared bounded root-scoped reads protect directives and manifest assets, including revalidation after discovery. Model source-path metadata alone no longer grants local file access.
|
||
|
||
**P1 / High · Security · Reproduced**
|
||
|
||
Evidence: `pkg/assetloader/embed.go:73–74`; compare the containment checks in `pkg/assetloader/loader.go` and `pkg/jobs/jobs.go`.
|
||
|
||
Embed paths are cleaned and joined to the script directory, then passed directly to `os.ReadFile`. A directive using `path=../synthetic-secret.txt` successfully embedded a sibling file. There is no equivalent root or symlink policy for this path. Script metadata also supplies the source path used for resolution.
|
||
|
||
A supplied script can cause local readable files to be copied into generated SQL or sent to the destination database. Permission to execute SQL against a database should not implicitly grant unrestricted local-file access. This is a local input trust-boundary issue, not an unauthenticated network endpoint.
|
||
|
||
**Fix:** define an explicit asset root, contain all embed reads within it, check symlink resolution, limit file size, and document any opt-in allowance for external files. Reuse the policy across manifest assets and embed directives.
|
||
|
||
**Acceptance:** traversal and symlink escape fail before reading; valid in-root text and binary assets still work; diagnostics do not print file contents.
|
||
|
||
### A05 — PostgreSQL SQL quoting is incomplete across syntactic contexts
|
||
|
||
**P1 / High · Security, database · Reproduced rendering defects**
|
||
|
||
Evidence: `pkg/writers/pgsql/template_functions.go:134`; `pkg/writers/pgsql/templates/add_column_with_check.tmpl:5–7`; `create_foreign_key_with_check.tmpl`; `alter_column_type_with_check.tmpl`.
|
||
|
||
Two separate defects were confirmed:
|
||
|
||
1. `quoteIdent` does not actually check reserved keywords, despite its comment. The template executor generated `CREATE TABLE IF NOT EXISTS public.order (select text);` for legal names requiring quoting.
|
||
2. Catalog lookups insert names directly into single-quoted SQL strings. A table named `O'Reilly` generated `table_name = 'O'Reilly'`. Quoting a name for an identifier elsewhere does not escape it as a string literal here.
|
||
|
||
Fixed `DO $$ ... $$` delimiters create another context that must be protected from embedded delimiter text. Besides breaking valid database names, insufficient context-aware quoting is an injection risk when less-trusted metadata is rendered and executed using privileged database credentials. A complete injection exploit was not executed during this audit. Raw SQL expressions are a separate, intentional executable-input boundary and should be documented as such.
|
||
|
||
**Fix:** centralize identifier, literal, qualified-name, and block-delimiter handling. Always quote identifiers where practical; parameterize values where supported. Generate collision-safe dollar delimiters or avoid interpolating arbitrary content into fixed blocks. Apply this across all templates, not only the examples above.
|
||
|
||
**Acceptance:** generated SQL executes correctly for reserved words, apostrophes, double quotes, Unicode, newlines, and dollar delimiters, and cannot introduce additional SQL statements through a name.
|
||
|
||
PostgreSQL documents the distinct identifier, string, and dollar-quote rules in [Lexical Structure](https://www.postgresql.org/docs/current/sql-syntax-lexical.html).
|
||
|
||
### A06 — MySQL constraint reading scans nullable metadata into strings
|
||
|
||
**Repair status: addressed.** Nullable metadata scans and qualified queries are covered by live MySQL 8.4 round trips with primary, unique, composite FK, and functional-index definitions.
|
||
|
||
**P1 / High · Database · Code-confirmed; live MySQL not exercised**
|
||
|
||
Evidence: `pkg/readers/mysql/reader.go:181–220`, specifically the `rs`, `rt`, and `rc` string variables passed to `rows.Scan`.
|
||
|
||
The constraint query reads `REFERENCED_TABLE_SCHEMA`, `REFERENCED_TABLE_NAME`, and `REFERENCED_COLUMN_NAME` for primary and unique constraints as well as foreign keys. Non-foreign-key rows have no referenced object and can contain SQL NULL. Scanning these into Go strings fails, aborting inspection of ordinary tables with a primary key.
|
||
|
||
**Fix:** use nullable destinations such as `sql.NullString`, or explicit `COALESCE` where an empty string has the intended meaning. Review other nullable metadata, including expression-index column names, in the same reader.
|
||
|
||
**Acceptance:** live MySQL tests read tables with primary, unique, composite foreign-key, and expression-index definitions without scan failures and preserve their metadata. The current reader tests only cover type mapping and a missing connection string.
|
||
|
||
Reference: [MySQL KEY_COLUMN_USAGE documentation](https://dev.mysql.com/doc/refman/8.0/en/information-schema-key-column-usage-table.html) and [MySQL's documented non-FK NULL behavior](https://bugs.mysql.com/bug.php?id=9587).
|
||
|
||
### A07 — MySQL conversion silently loses constraints, indexes, and type detail
|
||
|
||
**Repair status: addressed within the [documented MySQL table-schema contract](MYSQL_FIDELITY.md).** Supported features survive live round trips through JSON/YAML; known unsupported modeled/discovered features produce diagnostics or strict conversion failures. MySQL output remains creation DDL for existing databases, with engine/version/SQL-mode limits stated in the capability matrix.
|
||
|
||
**P1 / High · Database · Reproduced omissions; additional losses code-confirmed**
|
||
|
||
Evidence: `pkg/writers/mysql/writer.go:96–163`; `pkg/readers/mysql/reader.go:153–178`; `pkg/mariadb/datatypes.go`.
|
||
|
||
The writer emits columns, primary keys, and unique constraints, but does not emit foreign keys, checks, or ordinary indexes from the model. A reproduction containing both a foreign key and an index returned success while emitting neither. The reader maps `COLUMN_TYPE` to a canonical base type without populating the separate length/precision/scale fields; its tests explicitly map `bigint unsigned` to `int64`.
|
||
|
||
Consequences include lost referential integrity, missing performance-critical indexes, changed ranges, and changed string/decimal definitions. Advertising a format as supported does not communicate these losses.
|
||
|
||
**Fix:** preserve and emit supported semantics; return explicit diagnostics for every unsupported object or attribute. Add a strict conversion mode that refuses lossy output and a per-format capability matrix. Preserve unsignedness, dimensions, generated expressions, and FK actions through the intermediate representation.
|
||
|
||
**Acceptance:** inspect → model → DDL → disposable database → inspect tests compare semantic equivalence for representative MySQL schemas. Unsupported features produce visible, testable failures or warnings, never silent success.
|
||
|
||
### A08 — Embed substitution corrupts dollar characters; placeholder parsing ignores SQL syntax
|
||
|
||
**P2 / Medium · Engineering, database · Reproduced**
|
||
|
||
Evidence: `pkg/assetloader/embed.go:159–160`; `pkg/assetloader/executor.go:32–87`.
|
||
|
||
`replaceEmbedPlaceholder` passes file content into the replacement argument of `regexp.ReplaceAllString`. Dollar characters are interpreted as capture references. Embedding `price $1 ${2} $HOME $$` produced `price ; $` in the local reproduction, rather than preserving the input.
|
||
|
||
Separately, `BuildQuery` matches placeholders inside quoted strings and comments. `SELECT ':literal', :bytes` fails with an unknown-placeholder error even though `:literal` is SQL string content. Similar text-based processing makes the MySQL executor's `strings.Split(..., ";\n")` fragile for multiline defaults containing that sequence.
|
||
|
||
**Fix:** use literal replacement callbacks or explicit span assembly for embed content. Parse SQL lexical contexts for named placeholders. Execute a structured list of generated statements instead of splitting SQL text on delimiters.
|
||
|
||
**Acceptance:** dollar text, repeated placeholders, quoted strings, comments, casts, dollar-quoted bodies, and embedded statement delimiters preserve exact intended semantics.
|
||
|
||
### A09 — Migration execution lacks a coherent atomicity and concurrency contract
|
||
|
||
**P1 / High · Database, engineering · Code-confirmed**
|
||
|
||
Evidence: `pkg/writers/pgsql/writer.go:2200–2300`; `pkg/writers/sqlexec/writer.go:107–222`; `pkg/writers/sqlite/writer.go:118–164`; `pkg/writers/mysql/writer.go:165–188`.
|
||
|
||
Executors issue independent statements/scripts without a transaction spanning the operation, migration-wide lock, or applied-script ledger in these paths. A failure can leave earlier work committed. Re-running script execution repeats supplied scripts; concurrent invocations can interleave. A script may contain its own transaction, but that does not establish a consistent executor-level guarantee.
|
||
|
||
The necessary behavior differs by database and statement type: wrapping all MySQL DDL in a transaction would not provide the same rollback guarantee as transactional PostgreSQL DDL.
|
||
|
||
**Fix:** define explicit execution modes with transaction boundaries, concurrency control, retry/recovery semantics, and accurate partial-application reports. If `scripts execute` remains a raw runner, document that clearly. If it is intended as a migration manager, add migration identity, checksums, applied history, and duplicate/concurrent-run protection.
|
||
|
||
**Acceptance:** failure injection and simultaneous-run tests establish predictable behavior; supported transactional runs roll back; nontransactional runs report exactly what committed and how to resume safely.
|
||
|
||
### A10 — Database operations cannot consistently inherit cancellation or deadlines
|
||
|
||
**P2 / Medium · Engineering, operations · Code-confirmed**
|
||
|
||
Evidence: `pkg/readers/reader.go`; `pkg/writers/writer.go`; `pkg/writers/pgsql/writer.go:2205`; `pkg/writers/sqlexec/writer.go`; `pkg/readers/mysql/reader.go:25`; `pkg/writers/mysql/writer.go:184`.
|
||
|
||
Reader/writer interfaces do not accept a caller context, and several implementations start with `context.Background()`. This disconnects CLI/job cancellation from connection establishment, catalog reads, and SQL execution. A blocked statement or lock wait has no consistent application-level deadline.
|
||
|
||
**Fix:** carry caller contexts through reader/writer operations; configure connection, statement, and lock-wait limits separately. Use a signal-aware CLI root context and preserve cancellation errors in job results.
|
||
|
||
**Acceptance:** a blocked database query terminates promptly on cancellation or timeout, cleanup is bounded, and the job exits nonzero without starting dependents.
|
||
|
||
### A11 — Windows updater executes downloads without artifact authentication
|
||
|
||
**P2 / Medium · Security, supply chain · Code-confirmed**
|
||
|
||
Evidence: `cmd/relspec/update.go:115–152`; `.gitea/workflows/release.yml`.
|
||
|
||
After confirmation or `--yes`, the updater downloads the asset URL from release metadata and starts the resulting executable. There is no signature verification, trusted-publisher check, content-size limit, or asset-origin/redirect restriction in this path. The default release API uses HTTPS, which is a useful transport control, but it is not independent artifact authentication. Compromised release metadata or publication infrastructure could substitute an installer.
|
||
|
||
**Fix:** sign installers or release manifests and verify against an independently trusted identity before launch. Require HTTPS throughout redirects, apply a deliberate origin policy, cap downloads, and clean up failed downloads. A checksum fetched from the same compromised endpoint alone is insufficient authentication.
|
||
|
||
**Acceptance:** tampered, unsigned, wrong-publisher, oversized, and disallowed-origin assets are rejected before execution.
|
||
|
||
### A12 — Connection and development-service defaults permit avoidable exposure
|
||
|
||
**P2 / Medium · Security · Code-confirmed**
|
||
|
||
Evidence: `pkg/ui/connstring.go:42–49`; `docker-compose.yml`; `Makefile`'s `docker-up` target.
|
||
|
||
The UI starts PostgreSQL and SQL Server connection fields with encryption disabled; changing the host does not change that setting. Compose publishes database ports without a loopback host restriction and supplies fixed development passwords. These are development credentials, not evidence of leaked production secrets, but the services can become reachable on host network interfaces.
|
||
|
||
**Fix:** default remote connections to authenticated encrypted transport and make local exceptions explicit. Bind development database ports to `127.0.0.1` by default; document how to deliberately expose a test service. Prefer a limited inspection role over administrative defaults.
|
||
|
||
**Acceptance:** changing a connection to a remote host does not silently retain an insecure default; standard local test startup only exposes loopback ports.
|
||
|
||
### A13 — CI does not consistently test the declared toolchain or live writer paths
|
||
|
||
**P1 / High · Maintenance, database assurance · Code-confirmed configuration defects**
|
||
|
||
Evidence: `.github/workflows/ci.yml:15,58,79`; `.github/workflows/integration-tests.yml:59–76`; `.gitea/workflows/release.yml:48–49`; `Makefile:92–108`; `tests/dbtest/dbs/mysql.sh`.
|
||
|
||
- GitHub CI specifies Go 1.24/1.25 while `go.mod` requires 1.26.9. Automatic toolchain switching can make that matrix misleading; with switching disabled those jobs cannot build the module as declared.
|
||
- The GitHub integration job runs directly on the host but connects to `postgres`, the service-container name, rather than the published loopback port. GitHub documents `localhost` for this job layout. This networking conclusion applies to GitHub-hosted execution; the Gitea runner configuration was not inspected.
|
||
- `make test-integration` runs the PostgreSQL reader and `tests/integration`, but omits live tests under `pkg/writers/pgsql` and `pkg/writers/sqlexec`. Release `go test ./...` has no configured database service/connection, so those tests can skip there too.
|
||
- MySQL has container scaffolding but no Go tests consuming `RELSPEC_TEST_MYSQL_CONN`; this leaves A06 undetected.
|
||
- `make test-unit` excludes the entire PostgreSQL reader package, including `queries_pure_test.go`.
|
||
|
||
**Fix:** use `go-version-file: go.mod`, explicitly test supported toolchains, correct GitHub service networking, and run all live readers and writers against disposable databases in required CI jobs. Make unexpected integration-test skips fail the integration job. Verify which workflows the actual Gitea deployment discovers; CI badges alone do not establish execution.
|
||
|
||
**Acceptance:** CI artifacts show real reader/writer execution, not just green packages with skipped live tests, and demonstrate MySQL constraint inspection and failing-migration behavior.
|
||
|
||
Reference: [GitHub PostgreSQL service-container networking](https://docs.github.com/en/actions/tutorials/use-containerized-services/create-postgresql-service-containers).
|
||
|
||
### A14 — Formatting gate ignores formatter failure
|
||
|
||
**P2 / Medium · Maintenance · Reproduced**
|
||
|
||
Evidence: `Makefile:62–66`.
|
||
|
||
The recipe captures formatter stdout and checks whether it is empty, without preserving the command's failure status. `make fmt-check GOLANGCI_LINT=false` prints `Checking formatting...` and exits successfully. A missing/broken formatter or dependency/toolchain failure can therefore pass this gate without checking formatting.
|
||
|
||
**Fix:** explicitly fail when the formatter command fails before inspecting its diff output. Pin the formatter version so the gate is reproducible.
|
||
|
||
**Acceptance:** a formatter that exits nonzero with empty stdout fails the target; clean formatting passes; a real diff fails.
|
||
|
||
### A15 — Release artifacts can disagree with the requested release tag
|
||
|
||
**P2 / Medium · Maintenance, supply chain · Code-confirmed**
|
||
|
||
Evidence: `.gitea/workflows/release.yml:55–94,241–269,297–314`; upload steps around lines 113–130, 278–289, and 360–371; `Makefile:84,143,256,273`.
|
||
|
||
For manual dispatch with a tag input, checkout does not explicitly select that tag. Binaries and packages are built from the dispatched checkout, while the source archives use the supplied tag and version strings claim that version. These can represent different commits. Uploads use `curl -s` without failing on HTTP error status, so publication failures can also look successful.
|
||
|
||
Additional reproducibility gaps: ordinary `make build` runs `go mod tidy` through `deps`; analysis tools use `@latest`; `release-version` lacks test/build prerequisites; `rerelease` force-moves an existing release tag.
|
||
|
||
**Fix:** resolve and validate the release tag once, check out its commit in every artifact job, and record that commit in a manifest. Fail on HTTP errors and verify the uploaded inventory. Keep normal builds read-only with respect to dependency manifests, pin build tools, and publish immutable versions with consistent quality gates.
|
||
|
||
**Acceptance:** dispatching a historical tag from a newer branch produces binaries and source from the same historical commit; failed uploads fail the workflow; normal builds leave the working tree unchanged.
|
||
|
||
### A16 — Output replacement guarantees differ across commands and writers
|
||
|
||
**P2 / Medium · Engineering, maintenance · Code-confirmed**
|
||
|
||
Evidence: `cmd/relspec/job.go:859–886`; `pkg/writers/template/writer.go:288–308`; `pkg/writers/mysql/writer.go:54–68`; `pkg/writers/json/writer.go:56–58`.
|
||
|
||
Job single-file conversion has an atomic-write wrapper, but direct writers and template output write or truncate final paths in place. Some writer cleanup functions ignore close errors. A disk error, failed generation, or interrupted multi-file run can replace a previously valid artifact with incomplete output or leave a mixed generation.
|
||
|
||
**Fix:** centralize output staging and finalization for direct CLI use and jobs. Handle close/flush errors, use exclusive unpredictable temporary files, preflight filename collisions, and define a staging/manifest strategy for multi-file generations. Preserve existing files when generation fails.
|
||
|
||
**Acceptance:** injected generation/write/finalization failures leave the previous valid output intact and return an error; no command claims success with a partial replacement.
|
||
|
||
## Perspective-specific maintenance needs
|
||
|
||
These are follow-up investments and scope gaps, not additional proven vulnerabilities.
|
||
|
||
### Software engineering
|
||
|
||
- **A common capability registry:** format construction is repeated across `convert.go`, `diff.go`, `merge.go`, `inspect.go`, and `edit.go`. Job format validation is another list. Centralize reader/writer creation and capabilities so adding a format does not require synchronized switches throughout the CLI.
|
||
- **Typed execution options and results:** connection settings, execution counters, and flags are carried through string-keyed metadata maps. Introduce typed configuration/result structures for live execution while retaining metadata for extensions.
|
||
- **Model validation before generation:** JSON/YAML readers primarily unmarshal and return models. Establish a shared normalization/validation boundary for nil entries, duplicate objects, unresolved references, incompatible attributes, and absent required fields. The inspector is useful but should not be the only optional path to structural safety.
|
||
- **Smaller database writer components:** `pkg/writers/pgsql/writer.go` is 2,643 lines and combines generation, execution, reporting, type/index policy, and statement parsing. Separate these responsibilities to make security and migration changes easier to review.
|
||
- **Adversarial parser tests:** no `Fuzz...` tests were found in `pkg` or `cmd`. Add fuzz targets for DBML/directives, ORM readers, embed handling, SQL lexical processing, and invalid intermediate models. Add large-schema benchmarks before introducing concurrency or caching.
|
||
|
||
### Security
|
||
|
||
- Document the trust model: SQL scripts, expressions/defaults, asset calls, schema metadata, templates, and release assets have different execution and local-file capabilities.
|
||
- Test redaction across every CLI/library logging path, not just the job logger and masked UI preview. Cover URL-encoded passwords, DSN parse errors, and database error messages without printing real credentials in tests.
|
||
- Run pinned vulnerability checks in required change-validation workflows and record the tool/database versions. Produce release checksums and an SBOM; signing and verification are covered by A11.
|
||
- Use scoped database roles: inspection should not require DDL rights, and schema application should not normally use a server administrator account.
|
||
|
||
### Maintenance
|
||
|
||
- Update contradictory documentation. README advertises Go 1.24.0; CONTRIBUTING says 1.25+; `go.mod` requires 1.26.9. TODO says MySQL, watch mode, type mappings, Windows installer, and installer CI are missing even though implementations exist. `tests/dbtest/README.md` still says there is no MySQL driver/reader.
|
||
- Document actual feature completeness, not just format names. Distinguish implemented, partial, experimental, and unsupported behavior with links to tests.
|
||
- The VS Code extension's `renderTemplate` is a placeholder and syntax validation mainly counts delimiters per line (`vscode-extension/relspec-template-editor/src/extension.ts`). Label it experimental or connect it to the actual Go parser/renderer. Reuse and dispose a single diagnostics collection rather than creating one on each validation.
|
||
- Align GitHub and Gitea workflow ownership, tool versions, required checks, release policies, and developer setup. Add Windows execution tests for filesystem behavior and installer/updater code, rather than relying solely on cross-compilation.
|
||
|
||
### Database specialization
|
||
|
||
- Publish an object/attribute fidelity matrix covering every supported source/destination, including views, sequences, enums, generated/identity columns, FK actions/deferral, expression/partial indexes, precision/scale, unsigned types, and permissions.
|
||
- PostgreSQL generation needs explicit fidelity review beyond tables: the model includes views, enums, permissions, and standalone sequences, but the main PostgreSQL writer does not provide corresponding complete emission paths. Its sequence generation focuses on table primary keys; scanning views to infer extensions does not emit the views. Unsupported objects need warnings or rejection.
|
||
- Establish migration safety policy: destructive-change approval in the product, reviewed plans, populated-data preflight, lock budgets, backfills, recovery, and post-apply verification. A dry run that only validates options is not proof that a migration can execute safely.
|
||
- Test semantic convergence: after applying a model, re-inspect and diff; a second application should generate no unintended change. Include populated tables, cross-schema dependencies, generated columns, reserved names, and failure recovery.
|
||
- Keep database-specific transaction and DDL behavior explicit. Do not promise uniform rollback or schema support across PostgreSQL, SQLite, SQL Server, and MySQL.
|
||
|
||
## Prioritized repair plan
|
||
|
||
| Order | Work | Completion evidence |
|
||
| --- | --- | --- |
|
||
| 1 | Fix misleading success and unsafe type fallback: A01–A02 | Failure-injection tests prove nonzero exit and preservation of existing data/schema. |
|
||
| 2 | Close file and SQL rendering boundaries: A03–A05, A08 | Adversarial filename, embed, identifier, literal, and delimiter cases pass. |
|
||
| 3 | Repair MySQL reading and fidelity: A06–A07 | Live round trips preserve constraints, indexes, and type dimensions; unsupported features fail explicitly. |
|
||
| 4 | Restore trustworthy CI coverage: A13–A14 | All relevant live writers run, toolchain matches policy, and tool failures fail gates. |
|
||
| 5 | Define execution safety: A09–A10 | Documented transaction/concurrency modes, cancellation, recovery, and precise results. |
|
||
| 6 | Harden distribution and output: A11–A12, A15–A16 | Verified installer artifacts, consistent release commit, safer defaults, and staged file replacement. |
|
||
| 7 | Reduce maintenance debt | Capability registry, typed options, accurate docs, supported-feature matrix, fuzzing, and measured performance baseline. |
|
||
|
||
Avoid expanding the format list or adding a web/API layer until the existing execution and fidelity contracts are dependable. The current code already offers enough functionality that correctness, safety, and maintainability will yield more value than additional breadth.
|