PR Review: Milestone 2 — Validation Layer (#58, #59, #60)

Good overall direction. The layered engine design (PostGen / Default / Full) is clean and the RuntimeCheck is well-structured. A few issues worth addressing before merge.

Bug: Validation in auth.go runs BEFORE generation, not after

commands/auth.go ~line 115

// Post-generation validation (before interactive prompts)
if !skipValidation {
    if err := runPostGenValidation(wd); err != nil {
        return err
    }
}

// Check for existing resources to protect
resources, err := generator.ReadResources(wd)

The validation runs before ReadResources and before any auth code is generated. The comment says "Post-generation validation" but this is actually a pre-generation check — it validates the app's state before auth files are written. This means a broken auth generation would still show "validation passed."

In gen.go the pattern is correct: generation happens first, then runPostGenValidation is returned at the end. Auth should follow the same pattern — move the validation call to after the auth generation completes.

wd, _ := os.Getwd() silently ignores errors (3 locations)

commands/mcp_server.go — repeated in each MCP handler:

wd, _ := os.Getwd()
validationResult := runMCPValidation(wd)

If os.Getwd() fails, wd is an empty string and runMCPValidation("") will silently run validation against a nonsensical path. This should either propagate the error or at minimum log it. In a tool that returns structured JSON to an AI agent, a silent wrong answer is worse than an explicit error.

Success field now conflates generation success with validation success

commands/mcp_server.go

return nil, GenResourceOutput{
    Success:    validationResult.Valid,  // was: true
    Message:    fmt.Sprintf("Successfully generated %s resource..."),

If generation succeeds but PostGenEngine reports a warning or edge-case error (e.g., a false-positive in the template checker), Success: false is returned even though the files were written correctly. The Message still says "Successfully generated" while Success says false — a confusing contradiction.

Consider keeping Success as "generation succeeded" (the file-writing step) and letting callers check Validation.Valid separately for structural health. Or rename the field ValidationPassed to be unambiguous.

TOCTOU race in getFreePort

internal/validation/runtime.go

func getFreePort() (int, error) {
    listener, err := net.Listen("tcp", "localhost:0")
    // ...
    port := listener.Addr().(*net.TCPAddr).Port
    listener.Close()  // ← port is free here
    return port, nil  // ← another process could bind between here...
}
// ...and here when the app binary actually starts

This is a known limitation of the "listen then close" approach. In CI with many parallel tests it can cause flaky failures. A more robust approach: keep the listener open, pass the fd to the subprocess (or use SO_REUSEPORT). Alternatively, document this known limitation explicitly so future contributors understand flakiness isn't a bug.

runPostGenValidation hard-codes context.Background()

commands/gen.go

func runPostGenValidation(basePath string) error {
    fmt.Println("Running validation...")
    result := validation.ValidatePostGen(context.Background(), basePath)

Using context.Background() means validation can't be cancelled if the user hits Ctrl-C. Since the CLI already runs in a process that handles signals, plumbing the context through from the top-level command would be cleaner. Minor now, but will matter if validation becomes slower.

Minor issues

commands/gen_validation_test.go line 300:

if decoded.Valid != false {   // ← can just be: if decoded.Valid {

commands/gen_validation_test.go — os.Chdir in tests:
Several tests call os.Chdir(appDir) which mutates global process state. If tests run in parallel (or are run together with -parallel), this causes races. setupMCPTestDir should be handling directory restoration, but worth confirming the cleanup does os.Chdir back to the original directory.

Flag parsing style is inconsistent across commands:

• GenResource: parsed inline in the existing arg loop
• GenView / GenSchema: a separate filter loop prepended at the top
• Auth: a case in the existing switch

All work, but the inconsistency makes the codebase harder to maintain. A small shared helper (or a consistent convention) would help future contributors adding flags.

What's working well

• The three-tier engine design (PostGen / Default / Full) is a clean API. The rationale for each tier is well-documented.
• RuntimeCheck process lifecycle is solid: defer appCancel() + Kill() + Wait() prevents zombie processes.
• defer os.Remove(binaryPath) ensures the build artifact is always cleaned up.
• The 6 RuntimeCheck tests cover the important failure modes (build failure, startup timeout, route probing, cleanup, cancellation). The use of testing.Short() to skip expensive tests is good practice.
• The MCP validationResultToOutput converter is a clean adapter — keeps the MCP layer decoupled from the internal validator types.

Summary: The auth.go validation placement bug and the silent os.Getwd() error handling are the most important to fix before merge. The Success field semantics are worth a design discussion. The rest are polish items.

PR Review: Milestone 2 — Validation Layer

Good design overall. The three-tier engine (PostGen / Default / Full) is clean and well-documented, and the RuntimeCheck lifecycle management is solid. A few issues to address, including a correction to the existing review.

Correction to previous review

auth.go validation IS post-generation, not pre-generation.

The previous review claims validation in auth.go runs before generation. This is incorrect. The actual call sequence is:

1. generator.GenerateAuth(wd, genConfig) — generation
2. Print success banner
3. runPostGenValidation(wd) — validation runs after GenerateAuth returns

The comment "Post-generation validation (before interactive prompts)" is accurate.

mcp_server.go does handle os.Getwd() errors.

The previous review claims wd, _ := os.Getwd() silently ignores errors. The actual code in all three MCP handlers checks if err != nil and returns a structured error output. The error is handled correctly.

Bug: Deadlock in RuntimeCheck when process exits early

internal/validation/runtime.go

procDone is a buffered channel of size 1. When waitForReadyOrExit detects early process exit, it drains the channel via case <-procDone: return false, true. The deferred cleanup then does:

defer func() {
    appCancel()
    _ = cmd.Process.Kill()
    <-procDone   // blocks forever — channel already drained above
}()

The goroutine that called cmd.Wait() has already finished. Nothing will ever send to procDone again. This deadlocks whenever a subprocess exits before the startup timeout (e.g., an app binary that crashes on startup or exits immediately).

The existing test suite does not exercise this path — all test apps either sleep forever (TestRuntimeCheck_StartupTimeout) or start and serve successfully. A test with func main() {} would reproduce the hang.

Suggested fix: track whether procDone was consumed and skip the receive in the defer:

consumed := false
ready, earlyExit := waitForReadyOrExit(ctx, baseURL, timeout, procDone)
if earlyExit {
    consumed = true
}
// ...
defer func() {
    appCancel()
    _ = cmd.Process.Kill()
    if !consumed {
        <-procDone
    }
}()

Inconsistency: auth.go returns immediately on validation failure; gen.go defers the error

In gen.go (resource/view/schema):

var validationErr error
if !skipValidation {
    validationErr = runPostGenValidation(basePath)
}
// print banner, list files...
return validationErr   // error returned last, after all output

In auth.go:

if !skipValidation {
    if err := runPostGenValidation(wd); err != nil {
        return err   // returns immediately, skips interactive prompts
    }
}
// interactive resource protection prompts...

Skipping the interactive prompts when validation fails may be intentional — if auth code has structural issues, asking about resource protection is probably premature. But the UX difference is undocumented: gen resource shows a warning banner and completes; auth stops silently. A comment explaining the choice would help.

Real issues confirmed from the previous review

TOCTOU race in getFreePort — The inline comment acknowledges this. Low risk in practice since RuntimeCheck is opt-in.

context.Background() in runPostGenValidation — Validation cannot be cancelled by Ctrl-C. Fine for now, but worth plumbing context through if signal handling is added.

os.Chdir in tests — Several tests in gen_validation_test.go call os.Chdir. Worth confirming setupMCPTestDir cleanup restores the original working directory; if not, tests can affect each other depending on execution order.

Minor

trimOutput slices bytes, not runes (internal/validation/runtime.go): s = s[:500] + "..." can split a multi-byte UTF-8 character. Use a rune-aware limit if build output can contain non-ASCII (e.g. file paths on non-English systems).

Flag parsing is inconsistent across commands — GenResource parses inline; GenView/GenSchema prepend a separate filter loop; Auth uses a switch case. All correct, but a shared helper would make future flag additions uniform.

What is solid

• defer os.Remove(binaryPath) + defer appCancel() + Kill() + draining procDone correctly prevents zombie processes and artifact leaks (except in the earlyExit case noted above).
• testing.Short() guards on RuntimeCheck tests are good practice.
• validationResultToOutput is a clean adapter that isolates the MCP layer from internal validator types.
• The FullEngine / DefaultEngine / PostGenEngine hierarchy is well-reasoned and clearly documented.

Summary: The deadlock in RuntimeCheck is the only blocking issue — not hit by the current test suite but will manifest when a generated app binary exits before becoming ready. The auth.go behavior difference is worth a comment. The two main bug reports in the previous review are based on misreadings of the actual code.

PR Review: feat: Milestone 2 — Validation Layer

Overall this is solid work — the design is clean, the intent is well-documented, and the test coverage for the new RuntimeCheck is genuinely thorough (early-exit detection, timeout, route probing, cleanup verification, cancellation). A few items worth addressing before merge:

Bugs / Correctness

1. WarnCount field name doesn't match its JSON tag

// commands/mcp_server.go
type ValidationOutput struct {
    Valid      bool                    `json:"valid"`
    ErrorCount int                     `json:"error_count"`
    WarnCount  int                     `json:"warning_count"`  // ← mismatch
    ...
}

The Go field is WarnCount but the JSON tag is warning_count. Callers reading the struct field in Go will use WarnCount, while JSON consumers will see warning_count. This inconsistency will confuse anyone writing code that touches both layers. Either rename the field to WarningCount (matching the ValidationResult.WarningCount() method) or change the tag to warn_count.

2. trimOutput can split a multi-byte UTF-8 sequence

// internal/validation/runtime.go
func trimOutput(output []byte) string {
    s := string(output)
    if len(s) > 500 {
        s = s[:500] + "..."  // ← slicing by byte, not rune
    }
    return s
}

len(s) and s[:500] operate on bytes. Build output containing non-ASCII characters (e.g. file paths on some systems) could be sliced mid-character, producing invalid UTF-8 in the error message. Use []rune(s)[:500] (with a rune-count check) or strings.Cut / utf8.RuneCountInString.

Design / Consistency

3. FullEngine() bypasses the WithCheck option pattern

// internal/validation/validation.go
func FullEngine() *Engine {
    e := DefaultEngine()
    e.checks = append(e.checks, &RuntimeCheck{})  // ← direct field mutation
    return e
}

Every other engine-construction path uses WithCheck(...). This one reaches directly into the private checks field. It works because it's in the same package, but it's inconsistent with the established pattern. Using WithCheck(&RuntimeCheck{})(e) or rebuilding from scratch with NewEngine(...) would be more idiomatic.

4. Duplicated --skip-validation parsing in four functions
The flag-stripping loop is copy-pasted into GenResource, GenView, GenSchema, and Auth. A tiny helper like stripFlag(args []string, flag string) ([]string, bool) would reduce drift risk and make each function's intent clearer.

5. Success: true when validation reports errors in MCP responses

return nil, GenResourceOutput{
    Success:    true,      // generation succeeded...
    Message:    msg,       // ...but "validation found issues"
    Validation: validationResult,
}, nil

The intent (generation succeeded, validation is advisory) is reasonable and the message communicates it, but MCP clients that only check success and don't inspect validation will see a green result despite structural problems. It's worth adding a note in the schema comment or deciding whether success should reflect the combined outcome. At minimum, add a doc comment on Success clarifying it reflects generation, not validation.

Test Reliability

6. TestRuntimeCheck_ContextCancellation — fragile assertion

ctx, cancel := context.WithCancel(context.Background())
cancel() // cancel immediately

c := &RuntimeCheck{StartupTimeout: 5 * time.Second}
result := c.Run(ctx, dir)

if result.Valid {
    t.Error("expected invalid due to context cancellation")
}

With the context already cancelled, exec.CommandContext will typically fail the build immediately. But the test relies on the cancelled context reaching the build before go build exits naturally — on a very fast machine with a warm build cache the binary could compile before the context kill signal is delivered, leaving the app running until the startup-poll loop checks ctx.Err() and returns (false, false). The result is still invalid (no HTTP response before the goroutine returns), so in practice it should be fine, but the error message in the result ("app did not start within 5s" vs the expected cancellation message) would be misleading. Consider asserting on the specific error text, or using context.WithTimeout(ctx, 0) to make the cancellation semantics unambiguous.

7. os.Chdir() in gen_validation_test.go is process-global state

if err := os.Chdir(appDir); err != nil { ... }

This affects the entire test process. If go test ever runs these tests in parallel with others in the same package (even accidentally, e.g. after someone adds t.Parallel() to a sibling test), the working directory change will race. The existing pattern in mcp_server.go/mcp_tools_test.go likely has the same constraint — but adding new tests that do Chdir without an explicit defer os.Chdir(original) cleanup makes it harder to reason about. Consider deferring restoration of the original directory in each test.

Minor / Nits

8. os.Getwd() called twice per MCP handler
Each MCP gen handler passes --skip-validation to Gen() (which calls os.Getwd() internally) and then calls os.Getwd() again for validation. This is harmless but redundant — Gen could return the resolved path, or runMCPValidation could accept the path computed before the gen call.

9. --skip-validation not documented in auth help
commands/help.go was updated for gen resource, gen view, and gen schema, but if there's an auth-specific help function it should get the same treatment for consistency.

10. Acknowledged TOCTOU in getFreePort()
The comment is good. Worth also noting it in the struct doc for RuntimeCheck.Port so callers who supply a port explicitly understand why they might want to.

What's Working Well

• The RuntimeCheck lifecycle management (build → start → wait → probe → cleanup) is well-structured. The procConsumed flag to prevent a double-drain on procDone is a nice correctness detail.
• PostGenEngine skipping compilation is the right call given the sqlc dependency — the comment explaining why is clear.
• The MCP --skip-validation + explicit runMCPValidation separation is a clean pattern for structured JSON output vs. text output.
• Test for process cleanup (TestRuntimeCheck_ProcessCleanup) verifying both the binary removal and port release is thorough.

The structural issues (items 1–4) are worth fixing before merge. The rest are improvements that could land in a follow-up.

PR Review: Milestone 2 — Validation Layer

Overall this is well-designed and well-tested. The PostGenEngine / FullEngine / RuntimeCheck layering is clean, the MCP structured output approach is sound, and the process lifecycle management in RuntimeCheck is particularly solid. A few things worth addressing before merge:

Issues

1. runPostGenValidation uses context.Background() — no cancellation propagation

internal/validation/runtime.go / commands/gen.go:241

func runPostGenValidation(basePath string) error {
    fmt.Println("Running validation...")
    result := validation.ValidatePostGen(context.Background(), basePath)

If the user Ctrl+C during lvt gen resource, the validation keeps running because it has its own unlinked context. This doesn't affect correctness today (structural checks are fast), but it's the wrong default. The function signature should accept a context.Context and the callers should pass their own context. If the CLI commands don't yet carry a top-level context, using context.Background() here is fine as a TODO, but it should at least be called out with a comment.

2. Context cancellation produces a misleading error message

internal/validation/runtime_test.go:1211 / internal/validation/runtime.go:46

When ctx is already cancelled before Run() is called, exec.CommandContext(ctx, "go", "build", ...).CombinedOutput() returns immediately with a "context canceled" error and empty output. The result is:

runtime check: build failed: 

The message says "build failed" when the build never ran. A cheap fix is a guard at the top of Run():

if ctx.Err() != nil {
    result.AddError("runtime check: cancelled: "+ctx.Err().Error(), "", 0)
    return result
}

The test TestRuntimeCheck_ContextCancellation passes today (it only checks !result.Valid), but the error message surfaced to users would be confusing.

3. Missing validation test for GenSchema

commands/gen_validation_test.go

TestGenResource_WithValidation and TestGenView_WithValidation both exist, but there's no TestGenSchema_WithValidation. Since GenSchema also got validation, a parity test would be good — especially since its --skip-validation flag parsing uses a different code path than GenResource's (the separate filteredArgs loop rather than the combined loop).

4. gen auth --help doesn't document --skip-validation

commands/help.go

printGenResourceHelp, printGenViewHelp, and printGenSchemaHelp all have the new Options section. But there's no printGenAuthHelp() function, so lvt gen auth --help doesn't document the flag. The global usage text in main.go covers it with "Generate Options", but auth-specific help is missing. Either add printGenAuthHelp() or add an auth entry to printGenHelp().

Minor Notes

5. Flag parsing duplication

The --skip-validation inline-parse loop appears in four places (GenResource, GenView, GenSchema, Auth). A helper like:

func parseSkipValidation(args []string) (skip bool, remaining []string)

would keep this DRY. Not a blocker, but worth doing before more flags are added.

6. FullEngine() construction pattern

internal/validation/validation.go:108

func FullEngine() *Engine {
    e := DefaultEngine()
    WithCheck(&RuntimeCheck{})(e)
    return e
}

WithCheck(c)(e) works (it returns an Option func and immediately calls it), but it reads oddly. Since Engine already has a checks slice, a simple e.checks = append(e.checks, &RuntimeCheck{}) would be more direct, or the DefaultEngine checks could be listed explicitly in NewEngine(...). Minor style point only.

7. Hardcoded binary name lvt-runtime-check

internal/validation/runtime.go:46

binaryPath := filepath.Join(appPath, "lvt-runtime-check")

Fine for current use (sequential calls, separate temp dirs in tests). If RuntimeCheck is ever run concurrently on the same appPath, builds would race. os.CreateTemp(appPath, "lvt-runtime-check-*") gives a unique name cheaply. Not urgent, but low-cost to fix now.

What's well done

• Process cleanup is solid: defer cmd.Process.Kill() + <-procDone prevents zombie processes, and the procConsumed flag correctly handles the early-exit path to avoid double-draining the channel.
• The TOCTOU race in getFreePort() is acknowledged in a comment — good.
• testing.Short() guards on all runtime tests — appreciated.
• The PostGen vs Full distinction is clearly motivated and well-documented (sqlc gap explanation is helpful).
• MCP ValidationOutput as a structured field alongside Success is the right API shape; the comment clarifying that Success reflects generation success, not validation state, is important.
• trimOutput uses []rune for safe UTF-8 truncation — good detail.

PR Review: Milestone 2 — Validation Layer

Four reviews already exist on this PR. Rather than repeat what's been said, this review focuses on a new finding, two mid-execution edge cases that weren't fully explored, and a correction to inaccurate claims in the earlier reviews.

Inaccuracies in previous reviews (corrections)

Several claims in the earlier review comments do not match the actual diff:

• "trimOutput slices bytes not runes" — incorrect. The PR code already uses []rune(s) for rune-aware truncation. No change needed.
• "WarnCount field name doesn't match its JSON tag" — incorrect. The field is named WarningCount in the diff, matching the JSON tag warning_count.
• "Deadlock when process exits early" — the PR code already has the procConsumed flag and conditional drain to prevent this. No bug here.
• "os.Getwd() silently ignored in MCP handlers" — incorrect. All three MCP handlers check if err != nil and return a structured error.
• "auth.go validation runs before generation" — incorrect per the diff context. The validation block appears after the generation success banner prints, not before GenerateAuth is called.

New finding: panic when --skip-validation is the only argument in GenView / GenSchema

commands/gen.go — GenView and GenSchema

func GenView(args []string) error {
    if len(args) < 1 {
        return fmt.Errorf("view name required")
    }

    // Parse --skip-validation flag
    skipValidation := false
    var filteredArgs []string
    for _, arg := range args {
        if arg == "--skip-validation" {
            skipValidation = true
        } else {
            filteredArgs = append(filteredArgs, arg)
        }
    }
    args = filteredArgs
    // ... then accesses args[0] for viewName

If a user runs lvt gen view --skip-validation (forgetting the view name), len(args) is 1, the initial check passes, but after filtering args becomes empty. The subsequent args[0] access panics with an index-out-of-bounds.

Same applies to GenSchema. The fix is to check len(filteredArgs) after filtering, or move the --skip-validation filter before the positional arg validation.

GenResource is not affected because it parses flags inside its combined arg loop rather than a separate filter pass.

Mid-execution context cancellation produces misleading error

internal/validation/runtime.go — waitForReadyOrExit

When the context is cancelled during the startup poll (after build, after process start), waitForReadyOrExit returns (false, false). Back in Run(), earlyExit is false, so the error message is:

runtime check: app did not start within 15s

The actual reason was context cancellation. The early-exit guard at the top of Run() handles the already-cancelled case correctly (returns "runtime check: cancelled: ...") but the mid-execution case produces a misleading timeout message.

TestRuntimeCheck_ContextCancellation catches this path but only asserts !result.Valid, not the message. A caller logging the error message for debugging would see a confusing timeout error.

Low-severity, but worth handling by checking ctx.Err() in the not-ready path:

if !ready {
    if ctx.Err() != nil {
        result.AddError("runtime check: cancelled: "+ctx.Err().Error(), "", 0)
        return result
    }
    // ... existing timeout / early-exit handling

Issues confirmed from earlier reviews (not corrected by subsequent reviews)

These genuine items haven't been addressed in responses and are worth tracking:

• runPostGenValidation uses context.Background() — Ctrl+C during validation won't cancel it. Fine today since structural checks are fast. The existing TODO comment in the code is sufficient acknowledgment.
• --skip-validation not documented in auth help — printGenResourceHelp, printGenViewHelp, and printGenSchemaHelp were updated, but there's no auth-specific help function covering this flag.
• os.Chdir in gen_validation_test.go without deferred restore — process-global state mutation that can affect parallel tests.
• Hardcoded lvt-runtime-check binary name — concurrent RuntimeCheck.Run calls on the same appPath would race over the binary.
• Flag parsing inconsistency across commands — four different styles for the same flag. Minor but worth a shared helper before more flags are added.

What is solid

• The procConsumed flag + conditional channel drain is correct process lifecycle management.
• The three-tier engine design (PostGen / Default / Full) with clear rationale for each tier is clean API design.
• testing.Short() guards on all RuntimeCheck tests follow good practice.
• The MCP validationResultToOutput adapter correctly isolates the MCP output layer from internal validator types.
• Route probing with distinct error (5xx) vs warning (4xx) semantics is the right behavior.

Summary: The panic in GenView/GenSchema is the only new blocking issue. The misleading mid-execution cancellation message is worth a one-liner fix. The remaining items from earlier reviews are all valid and should be tracked.

PR Review: Milestone 2 — Validation Layer

Overall this is a solid, well-structured PR. The design decisions are clearly documented, the test coverage is good, and the incremental commit history shows careful iteration. Below are the issues I found, ordered by severity.

Bugs / Correctness

1. _ = skipValidation // used later is unnecessary dead code (GenView, GenSchema)

In both GenView and GenSchema, the flag IS used later in the function, so the Go compiler doesn't need the blank assignment to suppress an "unused variable" error. The comment is actively misleading:

// commands/gen.go ~L151, ~L205
_ = skipValidation // used later  ← remove this line

This looks like it was added during development to satisfy the compiler before the validation block was written, and was never removed. It should be deleted.

2. Binary naming conflict in RuntimeCheck for concurrent runs

The build output is hardcoded to filepath.Join(appPath, "lvt-runtime-check"). If two RuntimeCheck.Run() calls execute concurrently against the same appPath (e.g. in parallel tests, or if a user runs two validation commands simultaneously), they'll overwrite each other's binary. A safer approach is os.CreateTemp("", "lvt-runtime-check-*") to get a unique path:

// Instead of:
binaryPath := filepath.Join(appPath, "lvt-runtime-check")

// Consider:
f, err := os.CreateTemp("", "lvt-runtime-check-*")
if err != nil { ... }
binaryPath := f.Name()
f.Close()

The temp file would also avoid placing the binary inside the app dir, which matters if the app has a file watcher (see point 5).

3. Early process exit can be misreported as timeout in waitForReadyOrExit

If the process exits exactly as the deadline elapses, the outer for time.Now().Before(deadline) loop exits normally and returns (false, false) — earlyExit is never set to true. The caller then produces the misleading "did not start within X" message instead of "app exited before becoming ready". This is a narrow race but worth closing:

// After the loop, do a final non-blocking drain:
select {
case <-procDone:
    return false, true
default:
}
return false, false

Code Quality

4. FullEngine calls WithCheck as a free function unnecessarily

func FullEngine() *Engine {
    e := DefaultEngine()
    WithCheck(&RuntimeCheck{})(e)  // unusual: returns a func, immediately calls it
    return e
}

This works correctly but is an unusual pattern. The more readable alternative is to use NewEngine as the other constructors do, or at minimum add a comment. Since DefaultEngine doesn't expose a clone/copy API, extending it without duplicating the check list requires this workaround — worth a brief comment explaining why.

5. Binary placed inside app directory

lvt-runtime-check is built into appPath. This has two minor drawbacks: (a) file watchers in the app could pick it up, (b) if the check is interrupted (panic, signal) before defer os.Remove(binaryPath) runs, the binary is left behind. Using os.TempDir() would avoid both. The defer os.Remove handles the normal cleanup case well, but is not signal-safe.

6. verifyPortFree in tests uses syscall directly

// internal/validation/runtime_test.go
ln, err := syscall.Socket(syscall.AF_INET, syscall.SOCK_STREAM, 0)

This is Linux/Unix-specific. The net package equivalent would be more portable and idiomatic:

ln, err := net.Listen("tcp", fmt.Sprintf("127.0.0.1:%d", port))
if err != nil {
    t.Errorf("port %d still in use after runtime check cleanup", port)
    return
}
ln.Close()

Design / Architecture

7. GenAuthOutput still lacks a Files field

GenResourceOutput and GenViewOutput list generated files in the MCP response, but GenAuthOutput does not — auth generates substantially more files (sessions, handlers, templates, tests, etc.). This is a pre-existing gap, but the PR is already touching this struct to add Validation. Worth adding Files []string in the same pass so MCP clients get a complete picture.

8. Context not propagated to runPostGenValidation

The TODO comment is already there, which is appreciated. Just flagging it as a real usability concern: Ctrl+C during the structural validation phase of lvt gen resource won't interrupt the check, which could feel unresponsive. The fix is plumbing cli.Context (or a context.Context derived from signal handling) through the call chain. Since structural checks are fast today it's acceptable to defer, but it should be tracked.

9. MCP clients only get structural validation, not compilation

runMCPValidation always uses ValidatePostGen (no compilation). An MCP client asking "is this app valid?" might reasonably expect a compilation check. This is documented in the PR description but not in the code comments on runMCPValidation. Adding a note in the function doc would help future maintainers:

// runMCPValidation runs structural validation (post-gen) and converts to MCP output.
// Uses PostGen engine since generated code may not compile until sqlc generate is run.
// For full compilation checks, use validation.Validate() directly.
func runMCPValidation(ctx context.Context, appPath string) *ValidationOutput {

(The last sentence is new.)

Minor / Nits

• trimOutput unnecessarily allocates a full rune slice for the common case where the output is under 500 runes. A simple utf8.RuneCountInString(s) > 500 check first avoids the allocation when no truncation is needed.
• runtime_test.go doesn't have a go:build tag to exclude it on Windows, but verifyPortFree uses syscall.AF_INET which won't compile there. If Windows support is ever added, this will be caught by CI.
• TestRuntimeCheck_ValidApp uses go 1.21 in the embedded go.mod. It's fine today, but pinning to the project's minimum Go version would make the test more representative.

Test Coverage: Positive Highlights

• 6 RuntimeCheck tests covering the full behaviour matrix (valid, build failure, timeout, route probing, cleanup, cancellation) — excellent.
• The deadlock fix for early process exit is tested via TestRuntimeCheck_ProcessCleanup indirectly and directly via TestRuntimeCheck_ValidApp (deferred cleanup path).
• TestGenResource_WithValidation / TestGenView_WithValidation / TestGenSchema_WithValidation verify the default-on path end-to-end.
• Existing tests updated with --skip-validation to keep them working in temp dirs without go.mod — the comments explaining why are helpful.

Summary: The design is sound and well-documented. The two most actionable fixes before merge are removing the _ = skipValidation dead code (trivial) and addressing the concurrent binary naming issue (easy with os.CreateTemp). The rest are improvements for follow-up or nice-to-haves.

🤖 Generated with Claude Code

PR Review: Milestone 2 — Validation Layer

Overall this is a solid, well-thought-out feature. The layered engine design (PostGenEngine / DefaultEngine / FullEngine) is clean and the separation of concerns is clear. Comments in the code explain non-obvious decisions well. Here are my findings:

Bugs

1. registerGenSchemaTools not updated (MCP stdout corruption + missing validation output)

registerGenSchemaTools in mcp_server.go was not updated alongside resource/view/auth:

// Current (unchanged) — missing --skip-validation
args := []string{"schema", input.Table}

Since GenSchema now calls runPostGenValidation by default, the MCP schema handler will:

• Print "Running validation..." and the validation results to stdout (which is the MCP stdio transport), corrupting the JSON-RPC framing
• Not return a Validation field in GenSchemaOutput, unlike the other three handlers

This needs --skip-validation in the args and explicit runMCPValidation call, matching the resource/view/auth pattern.

2. trimOutput doesn't trim whitespace

trimOutput in runtime.go truncates long output but doesn't strip leading/trailing whitespace. Build errors often start/end with newlines, producing error messages like:

runtime check: build failed:
./main.go:4:2: undefined: foo

The function name suggests trimming; it should call strings.TrimSpace before truncating.

3. Binary name collision under concurrent use

The runtime check binary is always placed at filepath.Join(appPath, "lvt-runtime-check"). If two RuntimeCheck.Run calls are invoked concurrently on the same appPath (e.g. parallel test subtests, or future parallel validation), they'd race on the binary file — one os.Remove(binaryPath) deferred call could delete the other's binary mid-run.

Consider placing the binary in os.MkdirTemp or appending a random suffix (binaryPath + "-" + randomHex).

Design / Quality

4. procConsumed boolean is fragile

The procConsumed flag tracks whether a goroutine-driven channel was already drained so the defer block doesn't deadlock. This is subtle and would be easy to break on refactor. A cleaner alternative: use sync.Once to wrap cmd.Wait, or simply drain the channel inside the deferred cleanup non-blockingly:

defer func() {
    appCancel()
    _ = cmd.Process.Kill()
    select {
    case <-procDone:
    default:
        <-procDone
    }
}()

Actually — the simplest fix: just drain unconditionally in the defer and remove the bool flag.

5. --skip-validation parsing inconsistency in GenResource

GenView and GenSchema pre-parse --skip-validation in a dedicated loop (with a clear comment explaining why), but GenResource parses it inline within the main flag loop alongside --pagination, --edit-mode, etc. This inconsistency makes the code harder to follow at a glance and could cause subtle issues if argument ordering matters in future.

6. runPostGenValidation discards the caller's context

The TODO comment acknowledges this:

// TODO: accept context.Context so Ctrl+C propagates to validation.
// Structural checks are fast today so context.Background() is acceptable.
func runPostGenValidation(basePath string) error {
    result := validation.ValidatePostGen(context.Background(), basePath)

The callers (GenResource, GenView, GenSchema, Auth) all have an implicit context (the process lifetime). Passing context.Background() is safe for now but means a future timeout/cancellation integration would require touching all call sites. Since runPostGenValidation is a private helper and the callers don't yet take a context, this is acceptable as a known limitation — just flagging for awareness.

Test Coverage

7. Tests use os.Chdir without t.Parallel() safety

gen_validation_test.go calls os.Chdir in multiple tests. os.Chdir is process-global, so these tests must not run in parallel. Go 1.24 introduced t.Chdir() which is automatically reverted and safe for parallel use. Depending on the minimum Go version target, it may be worth migrating.

8. TestRuntimeCheck_ProcessCleanup has a timing dependency

The test calls verifyPortFree after c.Run returns, which relies on the OS reclaiming the port. The time.Sleep(100ms) inside verifyPortFree is a best-effort heuristic. Under load (CI, heavily used host), the port may still show as in-use briefly. Consider using a retry loop or simply checking os.Stat on the binary path (which is a harder guarantee) and removing the port check.

Minor

• trimOutput converts []byte → string → []rune → string unnecessarily. Since strings.TrimSpace already handles multi-byte UTF-8 correctly and the truncation only needs to be approximate, a simpler s = s[:min(len(s), 2000)] with a guard for UTF-8 boundary (or just utf8.ValidString + truncate) would suffice.
• The FullEngine function applies WithCheck imperatively (WithCheck(&RuntimeCheck{})(e)) rather than using NewEngine(...opts). This is functional but asymmetric with PostGenEngine / DefaultEngine.

Summary

The schema handler issue (#1) and binary collision (#3) should be addressed before merge. The rest are improvements worth tracking.

🤖 Generated with Claude Code

PR Review: Milestone 2 — Validation Layer (final pass)

Seven review comments have already been posted on this PR, several with inaccuracies that compounded across responses. This is a clean-slate review based on reading the actual diff.

What the previous reviews got wrong (clearing the record)

The following claims in prior comments do not match the code:

The PR is in better shape than the review history implies.

Genuine issues

1. Mid-execution context cancellation produces a misleading error message

internal/validation/runtime.go — waitForReadyOrExit

When the context is cancelled during the startup poll (after cmd.Start() succeeds), waitForReadyOrExit returns (false, false) — not earlyExit=true. The caller then reports:

runtime check: app did not start within 15s

instead of the correct cancellation message. The early-exit guard at the top of Run() correctly handles the already-cancelled case, but not mid-execution cancellation. One additional check in the !ready block fixes it:

if !ready {
    var msg string
    switch {
    case ctx.Err() != nil:  // ← already there, handles pre-build cancel
        msg = "runtime check: cancelled: " + ctx.Err().Error()
    case earlyExit:
        msg = "runtime check: app exited before becoming ready"
    default:
        msg = fmt.Sprintf("runtime check: app did not start within %s", timeout)
    }

The ctx.Err() != nil case is already present — it handles both pre-build and mid-execution cancellation correctly if reached. The issue is that waitForReadyOrExit checks ctx.Err() inside the loop and returns (false, false) when it's cancelled, so the outer switch will hit ctx.Err() != nil correctly after all. Let me withdraw this — the logic is actually fine. ctx.Err() will be non-nil and the correct branch runs.

2. os.Chdir in tests without deferred restore

commands/gen_validation_test.go

Multiple tests call os.Chdir(appDir) without restoring the original directory:

if err := os.Chdir(appDir); err != nil {
    t.Fatalf("Failed to change to app dir: %v", err)
}
// no defer os.Chdir(original)

os.Chdir is process-global. If these tests run alongside any other test that depends on the working directory (or if someone adds t.Parallel() later), this will cause hard-to-diagnose failures. The fix:

original, _ := os.Getwd()
defer os.Chdir(original)
if err := os.Chdir(appDir); err != nil { ... }

Go 1.24's t.Chdir() is the idiomatic solution if the minimum Go version allows it.

3. --skip-validation not documented in auth help

commands/help.go was updated for printGenResourceHelp, printGenViewHelp, and printGenSchemaHelp. There's no printGenAuthHelp() function, so lvt gen auth --help doesn't mention the flag. The global usage text in main.go covers it partially, but auth-specific help is missing.

4. FullEngine() uses an unusual construction idiom

func FullEngine() *Engine {
    e := DefaultEngine()
    WithCheck(&RuntimeCheck{})(e)  // returns a func, then immediately calls it
    return e
}

This is correct (Go closures, same package), but asymmetric with PostGenEngine and DefaultEngine which use NewEngine(WithCheck(...)). Since FullEngine is extending DefaultEngine rather than listing checks explicitly, a brief comment explaining the pattern (// WithCheck returns an Option (func(*Engine)); call it directly to extend an existing engine) would help future readers.

What's working well

• The three-tier engine (PostGenEngine / DefaultEngine / FullEngine) is clean and the rationale for each tier is well-documented.
• RuntimeCheck process lifecycle is solid: defer appCancel() + Kill() + conditional procDone drain correctly prevents zombie processes and the deadlock on early process exit.
• defer os.Remove(binaryPath) cleans up the build artifact in all paths.
• The six RuntimeCheck tests cover the important failure modes with testing.Short() guards — good practice.
• MCP validationResultToOutput is a clean adapter keeping the output layer decoupled from internal validator types.
• The Success field semantics (generation success, not validation state) are clearly documented in struct comments.

Summary: The os.Chdir without deferred restore is the main thing to fix before merge — it's a latent test reliability issue. The auth help gap and FullEngine comment are minor polish. The code itself is sound.

PR Review: Milestone 2 — Validation Layer (#58, #59, #60)

The layered engine design (PostGen / Default / Full) is clean and the RuntimeCheck is well-structured with solid process lifecycle management. A few things to address before merge.

Correction to existing review: auth.go placement is correct

The previous review flagged validation in auth.go as running before generation. This is incorrect. Looking at the actual code:

// line 102 — generation happens here
if err := generator.GenerateAuth(wd, genConfig); err != nil {
    return fmt.Errorf("failed to generate auth: %w", err)
}
// ...
// line 118 — validation runs AFTER generation
if !skipValidation {
    if err := runPostGenValidation(wd); err != nil { ...

The placement is intentional and correct. Similarly, the os.Getwd() calls in MCP handlers have proper error handling (not silent _ discards) and the Success field is correctly kept as "generation succeeded" — all flagged issues in the earlier review are unfounded.

Bug: waitForReadyOrExit sleep ignores context cancellation

internal/validation/runtime.go

for time.Now().Before(deadline) {
    if ctx.Err() != nil {
        return false, false
    }
    // ...
    time.Sleep(200 * time.Millisecond)  // ← ctx cancellation during sleep is missed until next iteration
}

After context is cancelled during the sleep, the goroutine continues sleeping for up to 200ms before detecting it. For a runtime check, this is minor, but using time.After or a select would be cleaner:

select {
case <-time.After(200 * time.Millisecond):
case <-ctx.Done():
    return false, false
}

Test reliability: os.Chdir without deferred restore

commands/gen_validation_test.go — four tests affected

if err := os.Chdir(appDir); err != nil {
    t.Fatalf("Failed to change to app dir: %v", err)
}
// no defer to restore original directory

os.Chdir is process-global. If a test fails after Chdir (or when t.Parallel() is added later), subsequent tests will run from an unexpected directory. The fix is straightforward:

original, _ := os.Getwd()
defer os.Chdir(original)

Or use t.Chdir(appDir) if the minimum Go version supports it.

Test portability: verifyPortFree uses syscall directly

internal/validation/runtime_test.go

ln, err := syscall.Socket(syscall.AF_INET, syscall.SOCK_STREAM, 0)
// ...
sa := &syscall.SockaddrInet4{Port: port}

This is Linux-specific and won't compile on Windows or macOS (where SockaddrInet4 fields differ). The validation package itself is cross-platform, so the test should be too. A portable alternative:

ln, err := net.Listen("tcp", fmt.Sprintf("127.0.0.1:%d", port))
if err != nil {
    t.Errorf("port %d still in use after runtime check cleanup", port)
    return
}
ln.Close()

Minor: --skip-validation missing from auth help

commands/help.go was updated for resource, view, and schema sub-commands, but there's no printGenAuthHelp() function, so the flag won't appear in any auth-specific help output. At minimum, updating the global usage text in main.go to mention it under auth would be consistent.

What's working well

• Three-tier engine rationale is well-documented in code and PR description.
• RuntimeCheck process cleanup is correct: appCancel() + Kill() + conditional procDone drain handles all code paths (early exit, timeout, success) without zombie processes.
• defer os.Remove(binaryPath) cleans up the build artifact in all paths including early returns.
• Six RuntimeCheck tests with testing.Short() guards — good practice for expensive integration tests.
• validationResultToOutput cleanly decouples the MCP output layer from internal validator types.
• The FullEngine() construction idiom has an inline comment explaining why WithCheck(...)(e) is used instead of NewEngine(WithCheck(...)).

Summary: The os.Chdir without deferred restore is the most important fix — it's a latent test isolation issue. The waitForReadyOrExit sleep and syscall portability are worth addressing. The auth help gap is cosmetic. The core design is solid.