test: add CLI testscript suite with 25 black-box tests

[adnaan]

Summary

• Extract run() int from main() to enable testscript.Main for in-process CLI testing
• Add 25 .txtar test scripts covering all CLI commands: version, help, new, blocks, fix, validate, serve, build, and cli CRUD operations
• Add github.com/rogpeppe/go-internal as a direct dependency for the testscript framework

Test plan

• All 25 testscript tests pass (go test ./cmd/tinkerdown/ -run TestCLIScripts -v)
• Internal package tests still pass (go test ./internal/... -short)
• CI passes

Closes #64

🤖 Generated with Claude Code

[claude]

PR Review: test: add CLI testscript suite with 25 black-box tests

Overall this is a clean, well-scoped PR. The run() int refactor is minimal and correct, and using testscript for black-box CLI testing is a solid approach. A few things worth addressing:

---

Potential Bugs

serve_errors.txtar — port test may never reach port validation

! exec tinkerdown serve --port abc
stderr 'invalid port'

No directory argument is supplied here. If serve validates the directory before parsing --port, the error will be directory does not exist (or similar), not invalid port, causing the test to pass for the wrong reason — or fail unexpectedly. Worth adding a valid/existing path before --port abc.

---

cli_sqlite_crud.txtar — assumes auto-increment starts at 1

exec tinkerdown cli . update tasks --id=1 --done=true
stdout 'Item 1 updated successfully.'

Testscript uses a fresh tmpdir per run so this is safe today, but if the application ever pre-seeds the table (migrations, fixtures) the hardcoded id=1 will break. A more robust approach would be to capture the ID from the add output, if the command prints it.

---

Misleading Comment

fix_dry_run.txtar

# The embedded test.md has no trailing newline, which fix should detect.

The embedded test.md actually ends with several blank lines, not a missing trailing newline. The comment should describe what the fix being detected actually is (excess blank lines), or the fixture should be updated to match the comment.

---

Minor Style Issues

no_args.txtar — usage goes to stdout, not stderr

! exec tinkerdown
stdout 'Usage:'

Convention is to print usage/error messages to stderr on failure. Since this is an existing behaviour exposed by the test rather than introduced by it, it's fine to leave as-is — but worth a follow-up to align with the convention in main.go.

help.txtar — unknown command check uses stdout

! exec tinkerdown nosuchcommand
stdout 'Unknown command: nosuchcommand'

Same as above — the Unknown command: message is printed with fmt.Printf (stdout). Not a bug in this PR but flagging it for visibility.

---

What's Working Well

• The TestMain / testscript.Main wiring in cli_test.go is idiomatic and correct.
• Coverage across all commands (version, help, new, blocks, fix, validate, serve, build, cli CRUD) is comprehensive.
• Error-path tests are thorough — readonly, missing fields, missing ID, unknown action, bad path.
• cli_csv_source.txtar and cli_json_source.txtar are a nice touch to verify non-SQLite sources work end-to-end.

The serve_errors.txtar port test is the only item I'd want verified before merging, since it may silently be testing the wrong error path.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR adds a black-box CLI test suite using github.com/rogpeppe/go-internal/testscript, enabling repeatable end-to-end validation of tinkerdown commands via .txtar scripts. It also refactors the CLI entrypoint to expose a run() int function so the CLI can be executed under testscript.Main without building an external binary.

Changes:

• Extracted run() int from main() and switched the CLI to return exit codes instead of calling os.Exit directly in command paths.
• Added a testscript-based runner (TestMain + TestCLIScripts) and a suite of 25 black-box CLI scripts.
• Added github.com/rogpeppe/go-internal as a direct dependency to support testscript.

Reviewed changes

Copilot reviewed 28 out of 28 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
go.mod	Adds github.com/rogpeppe/go-internal dependency for testscript.
cmd/tinkerdown/main.go	Refactors CLI entrypoint to run() int to support in-process testscript execution.
cmd/tinkerdown/cli_test.go	Adds testscript.Main registration and runs the script suite under go test.
cmd/tinkerdown/testdata/scripts/version.txtar	Script coverage for tinkerdown version.
cmd/tinkerdown/testdata/scripts/no_args.txtar	Script coverage for no-args usage output and non-zero exit.
cmd/tinkerdown/testdata/scripts/help.txtar	Script coverage for help flags/subcommand and unknown command behavior.
cmd/tinkerdown/testdata/scripts/new_basic.txtar	Script coverage for new default template behavior and output.
cmd/tinkerdown/testdata/scripts/new_template.txtar	Script coverage for new template flag variants (--template, -t, with/without =).
cmd/tinkerdown/testdata/scripts/new_errors.txtar	Script coverage for new error cases (missing name, spaces, unknown template, dir exists).
cmd/tinkerdown/testdata/scripts/validate_basic.txtar	Script coverage for validate success case on simple markdown.
cmd/tinkerdown/testdata/scripts/fix_errors.txtar	Script coverage for fix error on missing directory.
cmd/tinkerdown/testdata/scripts/fix_dry_run.txtar	Script coverage for fix --dry-run output behavior.
cmd/tinkerdown/testdata/scripts/fix_apply.txtar	Script coverage for fix applying changes and printing summary output.
cmd/tinkerdown/testdata/scripts/blocks.txtar	Script coverage for blocks basic output.
cmd/tinkerdown/testdata/scripts/blocks_verbose.txtar	Script coverage for blocks --verbose output.
cmd/tinkerdown/testdata/scripts/blocks_errors.txtar	Script coverage for blocks error on missing directory.
cmd/tinkerdown/testdata/scripts/serve_errors.txtar	Script coverage for serve error cases (missing dir, invalid port).
cmd/tinkerdown/testdata/scripts/build_errors.txtar	Script coverage for build error cases (missing input, missing path).
cmd/tinkerdown/testdata/scripts/cli_usage.txtar	Script coverage for cli usage errors with too few args.
cmd/tinkerdown/testdata/scripts/cli_unknown_action.txtar	Script coverage for cli unknown action error.
cmd/tinkerdown/testdata/scripts/cli_errors.txtar	Script coverage for cli error cases (missing path, empty config).
cmd/tinkerdown/testdata/scripts/cli_source_not_found.txtar	Script coverage for cli source-not-found behavior.
cmd/tinkerdown/testdata/scripts/cli_requires_id.txtar	Script coverage for cli update/delete requiring --id.
cmd/tinkerdown/testdata/scripts/cli_requires_fields.txtar	Script coverage for cli add requiring at least one field.
cmd/tinkerdown/testdata/scripts/cli_readonly.txtar	Script coverage for rejecting writes to read-only sources.
cmd/tinkerdown/testdata/scripts/cli_json_source.txtar	Script coverage for listing items from a JSON source.
cmd/tinkerdown/testdata/scripts/cli_csv_source.txtar	Script coverage for listing items from a CSV source.
cmd/tinkerdown/testdata/scripts/cli_sqlite_crud.txtar	Script coverage for SQLite-backed CRUD lifecycle via cli.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[claude]

Code Review

Overall this is a well-structured addition of a black-box testscript suite. The run() int extraction is the right pattern for testscript.Main. A few observations:

Correctness

no_args.txtar checks stdout instead of stderr

! exec tinkerdown
stdout 'Usage:'

printUsage() likely writes to stdout, but usage/error output is often directed to stderr. If the implementation ever moves usage to stderr (common practice), this test will silently break. Consider whether stderr 'Usage:' is more appropriate, or at minimum add a stderr assertion alongside it.

help.txtar — unknown command stdout vs stderr

! exec tinkerdown nosuchcommand
stdout 'Unknown command: nosuchcommand'

The diff shows main.go uses fmt.Printf (stdout) for the unknown command message. This is inconsistent with how errors are handled elsewhere (fmt.Fprintf(os.Stderr, ...)). The test locks in the current (arguably incorrect) behavior. Worth fixing in the implementation and updating the test to use stderr.

cli_sqlite_crud.txtar assumes auto-increment starts at 1

exec tinkerdown cli . update tasks --id=1 --done=true

This is fine for isolated testscript runs (each gets a fresh working directory), but the assumption is fragile if the SQLite table schema or initialization ever changes. A more robust approach would capture the ID from the add output — though that requires testscript env-var tricks (stdout capture isn't natively supported without custom commands).

go.sum not in the diff

go.mod adds github.com/rogpeppe/go-internal v1.14.1 but go.sum changes are not visible. Ensure go.sum was updated and committed; otherwise CI will fail with a checksum mismatch.

Test Coverage Gaps

• build success path — build_errors.txtar only tests failure cases. There's no happy-path test for a successful build run on a valid input file.
• serve success path — similarly, only error cases are tested. A smoke test that starts serve and checks it binds (with a timeout/background exec) would be valuable, though this is admittedly harder with testscript.
• validate with invalid content — only a valid-file happy path is tested. An invalid file (e.g., broken frontmatter or bad block syntax) would round out coverage.

Minor

• The TestMain / TestCLIScripts split is correct and idiomatic for testscript.
• .txtar files are well-commented and focused — easy to read and maintain.
• Using testscript for black-box CLI testing is the right tool for this job and closes 5.6 CLI Testscript Suite #64 cleanly.

Summary

The core refactor (run() int) is correct and minimal. The test suite covers the main happy-paths and error cases well. Addressing the stdout/stderr inconsistency for error output, verifying go.sum is committed, and adding a couple of missing success-path tests would make this solid.

[claude]

PR Review: CLI Testscript Suite. Overall this is solid and well-structured. Positives: run() int extraction is idiomatic Go for testable CLIs; printUsage(w io.Writer) correctly routes errors to stderr and help to stdout; testscript is an excellent black-box testing choice; good happy-path and error-path coverage across all commands. Issues: (1) go.sum is not in the diff - if rogpeppe/go-internal was not already an indirect dep, CI will fail; run go mod tidy to verify. (2) cli_sqlite_crud.txtar hardcodes --id=1 for update/delete, which is safe given testscript temp dir isolation but would break with UUID or non-sequential IDs. (3) cli_sqlite_crud.txtar supplies no .db file - this implicitly requires the implementation to auto-create the DB and table on first use; a comment explaining that contract would help future readers. (4) fix_dry_run.txtar only checks stdout, not that the file is actually unchanged - the defining property of dry-run. A cmp assertion against the original content would close that gap. (5) blocks_verbose.txtar asserts on exact strings like Type: server and Language: go, locking in the verbose output format as a public contract - worth being deliberate about. Minor: cli_requires_fields and cli_requires_id test pre-DB validation which is fast and reliable; the 4-variant flag test in new_template.txtar is thorough; serve_errors.txtar correctly avoids testing the blocking server path. Summary: run()/printUsage(w) improvements are unambiguously good. Three action items before merge: confirm go.sum is current, add file-unchanged assertion to fix_dry_run.txtar, add a comment about auto-create DB in cli_sqlite_crud.txtar.