Add validate input --server flag for persistent HTTP/REST

[simonbaird]

This adds an ec validate input --server command that starts up a web service that responds on /v1/validate/input with the results the same as if you ran ec validate input --output json with the request body as the input file

Ref: https://redhat.atlassian.net/browse/EC-1882

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds persistent HTTP server mode for ec validate input, with new flags, request handling, lifecycle wiring, acceptance coverage, and updated docs/tests.

Changes

Validate Input Server Mode

Layer / File(s)	Summary
Command surface and docs cmd/root.go, cmd/validate/input.go, cmd/validate/input_test.go, docs/modules/ROOT/pages/ec_validate_input.adoc, Makefile	Adds signal-aware command execution, server-mode flags and validation, server-mode command tests, updated docs/examples, and the lint-fix license check change.
Server runtime and request handling internal/server/server.go, internal/server/handler.go, internal/server/middleware.go, internal/evaluator/conftest_evaluator.go, internal/evaluator/evaluator.go	Defines the HTTP server, routes, middleware, request validation, evaluator execution, readiness handling, error responses, and evaluator concurrency/caching documentation.
Acceptance coverage acceptance/acceptance_test.go, acceptance/cli/server.go, features/validate_input_server.feature	Registers server steps and adds acceptance scenarios for server startup, requests, health checks, invalid input, and response assertions.
Unit tests internal/server/server_test.go	Adds tests for endpoints, request parsing, timeouts, panic recovery, lifecycle shutdown, and server-mode command behavior.

Estimated code review effort: 4 (Complex) | ~60 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: adding a persistent validate input server mode.
Description check	✅ Passed	The description accurately describes the new ec validate input --server HTTP service and endpoint behavior.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[codecov]

Codecov Report

❌ Patch coverage is 89.30041% with 26 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/server/handler.go	82.35%	18 Missing ⚠️
internal/server/server.go	88.40%	8 Missing ⚠️

Flag	Coverage Δ
acceptance	54.16% <80.65%> (+0.54%)	⬆️
generative	16.85% <0.00%> (-0.34%)	⬇️
integration	28.07% <13.99%> (-0.30%)	⬇️
unit	71.55% <62.55%> (-0.19%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
cmd/root.go	100.00% <100.00%> (ø)
cmd/validate/input.go	95.78% <100.00%> (+1.07%)	⬆️
internal/evaluator/conftest_evaluator.go	88.46% <ø> (ø)
internal/server/middleware.go	100.00% <100.00%> (ø)
internal/server/server.go	88.40% <88.40%> (ø)
internal/server/handler.go	82.35% <82.35%> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[coderabbitai]

Actionable comments posted: 7

🧹 Nitpick comments (1)

docs/modules/ROOT/pages/ec_validate_input.adoc (1)

12-22: 🔒 Security & Privacy | 🔵 Trivial

Consider documenting network exposure/security guidance for server mode.

The server binds on all interfaces (:<port>) with no authentication (per internal/server/server.go). Given this is now a persistent network-facing service, docs could note that it should be run behind a firewall/reverse proxy or restricted to trusted networks.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/modules/ROOT/pages/ec_validate_input.adoc` around lines 12 - 22, Update
the server-mode documentation in ec_validate_input.adoc to add a brief security
note that the persistent HTTP server started by --server binds on all interfaces
and has no authentication. Mention that users should run it behind a firewall or
reverse proxy, or restrict it to trusted networks, and place this guidance near
the existing endpoint/server-mode description so it is easy to find.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@cmd/validate/input.go`:
- Around line 146-159: The `--workers` option is currently ignored when
`data.serverMode` is true because `server.New(server.Config{...})` does not use
`Config.Workers`. Update the server path by either plumbing `data.workers`
through `internal/server` and consuming it in the relevant server code, or
remove/hide the flag from server mode so `cmd/validate/input.go` and
`server.Config` do not expose a misleading no-op setting.
- Around line 146-162: Pass a signal-aware context from the root command so
server shutdown can observe cancellation on SIGINT/SIGTERM. Update the root
execution path in cmd/root.go to use a context tied to OS signals instead of
context.Background(), and ensure validate/input.go’s server.New/srv.Start path
continues to consume cmd.Context() so srv.Start can react to ctx.Done() and exit
gracefully.

In `@internal/server/handler.go`:
- Around line 81-84: The evaluation and report build failure paths in handler
logic are leaking raw internal error text to API clients. Keep the detailed
error in server-side logging via the existing log.WithField("error", err) calls
in the handler flow, but change the client-facing responses in the evaluation
and report-build branches to return a generic message instead of fmt.Sprintf
with %v. Update the relevant error handling in the handler method that writes
responses so both failure cases use safe, non-internal text.
- Around line 149-159: The isValidInput helper currently skips validation for
explicit YAML content types, so malformed YAML can slip through and fail later
as a server error. Update isValidInput to validate application/yaml,
application/x-yaml, and text/yaml with utils.IsYamlMap instead of returning
true, and keep the existing JSON parsing behavior for application/json and the
default fallback. Since inputExtension uses the same content-type switch,
consider centralizing the YAML/JSON content-type handling there to avoid
divergence between the two helpers.
- Around line 78-87: Add a timeout around the evaluation flow in
handleValidateInput, since the request context is passed directly into each
evaluator and a slow run can hang the handler. Create a derived context with
context.WithTimeout before the loop that calls e.Evaluate, use that context for
every evaluator invocation, and cancel it when finished. Keep the existing error
handling and logging in the evaluator loop unchanged, but ensure the new
deadline applies to the entire evaluation sequence.

In `@internal/server/server.go`:
- Around line 35-43: The Config.Workers field is currently unused, so request
validation still runs with unbounded concurrency. Update Server to use Workers
as a concurrency limit by adding a semaphore (or equivalent limiter) in the
request path, wiring it through New/Start and enforcing it inside
handleValidateInput before policy evaluation begins. If you do not intend to
bound concurrency, remove Workers from Config and any related setup so the API
stays consistent.
- Around line 83-87: `httpServer` in the server startup setup only sets
`ReadHeaderTimeout`, so add explicit `ReadTimeout`, `WriteTimeout`, and
`IdleTimeout` on the `http.Server` configuration to bound slow client
connections. Update the `http.Server` initialization in the code path that
builds the listener for `handler`, keeping the existing `Addr` and
`ReadHeaderTimeout` values, and choose sensible timeout values consistent with
the service’s expected request/response behavior.

---

Nitpick comments:
In `@docs/modules/ROOT/pages/ec_validate_input.adoc`:
- Around line 12-22: Update the server-mode documentation in
ec_validate_input.adoc to add a brief security note that the persistent HTTP
server started by --server binds on all interfaces and has no authentication.
Mention that users should run it behind a firewall or reverse proxy, or restrict
it to trusted networks, and place this guidance near the existing
endpoint/server-mode description so it is easy to find.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Enterprise

Run ID: 1c1eb87d-d23f-4ada-87fa-e76939c91a9a

📥 Commits

Reviewing files that changed from the base of the PR and between 8bef68c and f00abb9.

⛔ Files ignored due to path filters (1)

• features/__snapshots__/validate_input_server.snap is excluded by !**/*.snap

📒 Files selected for processing (9)

• acceptance/acceptance_test.go
• acceptance/cli/server.go
• cmd/validate/input.go
• docs/modules/ROOT/pages/ec_validate_input.adoc
• features/validate_input_server.feature
• internal/server/handler.go
• internal/server/middleware.go
• internal/server/server.go
• internal/server/server_test.go

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 11:15 AM UTC · Completed 11:21 AM UTC
Commit: 7c8ccca · View workflow run →

[coderabbitai]

🧹 Nitpick comments (1)

cmd/validate/input_test.go (1)

312-325: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Fixed sleep risks flakiness and the assertion is too weak to validate graceful shutdown.

The 100ms sleep before cancel() is a fixed race window; under CI load, PreRunE/server startup may not have progressed enough, making the test pass without actually exercising the server path. Also, when err == nil the test asserts nothing — it can't confirm the server actually started and then shut down cleanly on context cancellation (which is also the scenario flagged in cmd/root.go around the Execute() fatal-error path). Consider synchronizing on an actual readiness signal instead of a sleep, and asserting require.NoError(t, err) for the graceful-shutdown case once ctx-cancellation is confirmed to return nil.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cmd/validate/input_test.go` around lines 312 - 325, The test in
validateCmd.Execute relies on a fixed time.Sleep before canceling, which makes
the server-path check flaky and non-deterministic. Replace that timing-based
pause with a real readiness/synchronization signal from the server startup path
(for example in the PreRunE/server startup flow) so the test only cancels after
the server is actually running. Then strengthen the assertion in the
graceful-shutdown case by requiring a nil error after context cancellation, and
keep the existing flag-validation negative checks only for the error path.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@cmd/validate/input_test.go`:
- Around line 312-325: The test in validateCmd.Execute relies on a fixed
time.Sleep before canceling, which makes the server-path check flaky and
non-deterministic. Replace that timing-based pause with a real
readiness/synchronization signal from the server startup path (for example in
the PreRunE/server startup flow) so the test only cancels after the server is
actually running. Then strengthen the assertion in the graceful-shutdown case by
requiring a nil error after context cancellation, and keep the existing
flag-validation negative checks only for the error path.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Enterprise

Run ID: 44a54a24-52ab-41d0-9626-d0380a433d6d

📥 Commits

Reviewing files that changed from the base of the PR and between f00abb9 and daa0cc7.

📒 Files selected for processing (7)

• acceptance/cli/server.go
• cmd/root.go
• cmd/validate/input.go
• cmd/validate/input_test.go
• internal/server/handler.go
• internal/server/server.go
• internal/server/server_test.go

💤 Files with no reviewable changes (1)

• cmd/validate/input.go

🚧 Files skipped from review as they are similar to previous changes (4)

• acceptance/cli/server.go
• internal/server/handler.go
• internal/server/server_test.go
• internal/server/server.go

[fullsend-ai-review]

Review

Verdict: Approve ✅

This PR adds a well-structured --server mode to ec validate input that starts a persistent HTTP server for policy evaluation. The implementation is clean and follows the project's existing patterns.

Strengths

• Sensible security defaults: Binds to 127.0.0.1 by default; flag help explicitly warns about the lack of authentication and rate limiting.
• Good HTTP server hardening: Appropriate timeouts (ReadHeaderTimeout, ReadTimeout, WriteTimeout, IdleTimeout), request body size limit (10 MB via LimitReader), evaluation timeout (90s), and a panic recovery middleware.
• Error handling discipline: Internal error details are logged server-side via logrus; only generic messages are returned to HTTP clients (no path/engine leaks).
• Correct LimitReader pattern: Reads maxRequestBodySize+1 then checks len(body) > maxRequestBodySize, correctly distinguishing exactly-at-limit from over-limit.
• Graceful lifecycle: Signal handling via signal.NotifyContext in cmd/root.go propagates cancellation to the server, enabling clean shutdown with in-flight request draining.
• Good test coverage: Unit tests cover liveness/readiness probes, valid/invalid input, evaluation errors, timeouts, YAML content type, panic recovery, and server lifecycle. Acceptance tests cover end-to-end scenarios with passing policies, violations, health endpoints, and invalid input.
• Concurrency safety: The conftestEvaluator.Evaluate uses a value receiver (copies the struct), creates all local state fresh per call, policy downloads are cached via sync.OnceValues, prepareDataDirs is read-only, and a new runner is created per invocation — the evaluator is safe for concurrent use from multiple HTTP request goroutines.

Observations (non-blocking)

1. Non-loopback bind address awareness (internal/server/server.go): When --server-address is set to 0.0.0.0, an explicit WARN-level log at startup (e.g., "server is listening on a non-loopback address with no authentication") would make the security posture more visible to operators who may not read flag docs carefully. The current startup log does include the address, but at INFO level without the security callout.

2. YAML body parsed twice (internal/server/handler.go): The request body is deserialized once in isValidInput() for validation, and then again when the evaluator processes the temp file. For a validate-then-process pattern this is standard, and the 10 MB limit plus gopkg.in/yaml.v3 alias expansion limits provide adequate protection. A minor optimization would be to pass the already-parsed structure to the evaluator, but this would require a larger refactor of the EvaluationTarget contract.

3. Concurrency test gap (internal/server/server_test.go): The unit tests exercise the handler synchronously. A concurrent test (e.g., t.Parallel() with multiple goroutines hitting handleValidateInput simultaneously) would strengthen confidence in the thread-safety claim documented on the Evaluator interface.

4. Makefile change scope (Makefile): The addlicense change from . to git ls-files -z | xargs -0 is a good improvement (avoids checking gitignored files) but is orthogonal to the server feature. Consider splitting it into its own commit for clarity.

5. Acceptance test port allocation (acceptance/cli/server.go): The TOCTOU pattern (allocate port via Listen/Close, then reuse) has a small race window where another process could bind the port. This is a well-known testing pattern and unlikely to cause issues in practice, but the server's support for --server-port 0 (which the unit test correctly uses) could also be leveraged in acceptance tests.

---

Labels: PR adds new --server flag and internal/server/ package for persistent HTTP mode

Previous run

Review — Approve

This PR adds a persistent HTTP server mode to ec validate input, enabling policy evaluation over REST via POST /v1/validate/input. The implementation is well-structured, with good separation of concerns across internal/server/, proper signal handling for graceful shutdown, and comprehensive test coverage at both unit and acceptance test levels.

Strengths

• Secure defaults: binds to 127.0.0.1 by default, enforces a 10 MB request body limit, includes a 90-second evaluation timeout, and documents the lack of built-in auth/rate limiting
• Resilient design: recovery middleware catches panics, structured JSON error responses avoid leaking internal paths, and the ready endpoint defers 200 until policy sources are fully loaded
• Correct shutdown: signal.NotifyContext in cmd/root.go propagates SIGINT/SIGTERM through the context, giving the HTTP server a 10-second graceful shutdown window
• Reuse of existing evaluation pipeline: the server reuses output.Output, input.Report, and the evaluator interface, keeping behavior consistent with ec validate input --output json
• Good test coverage: unit tests cover all handler paths (empty body, invalid input, success, violations, timeout, evaluation error, recovery middleware, server lifecycle), plus four BDD acceptance scenarios

Findings

1. Concurrency contract on Evaluator — documented but not stress-tested (low)

The new doc comment on Evaluator states "must be safe for concurrent use," and the server shares evaluator instances across concurrent HTTP requests. Code analysis confirms this is safe: conftestEvaluator.Evaluate has a value receiver (each call gets a copy of the struct), policy downloads are cached via sync.OnceValues, and the conftest runner is created locally per call. However, this is the first code path that actually exercises shared evaluators under concurrency (the non-server worker pool creates separate evaluators per goroutine). Consider adding a Go race-detector test (go test -race) that fires concurrent requests at the handler to prove this empirically.

2. mockEvaluator.target field has no synchronization (low)

In internal/server/server_test.go, the mock stores the evaluation target via m.target = target without synchronization. This is safe in the current sequential tests but would trigger a data race under -race if a concurrent test were added. A minor future-proofing concern.

3. Flag help text says "Mutually exclusive with --file" (low)

The --server flag description says "Mutually exclusive with --file" but the actual PreRunE check covers all input file sources (including positional arguments). The error message correctly says "--server and input files are mutually exclusive." The help text could be updated to say "Mutually exclusive with input files" for precision.

4. Unrelated Makefile change included (low)

The lint/lint-fix targets switch from addlicense ... . to git ls-files -z | xargs -0 addlicense ..., scoping license checks to git-tracked files only. This is a reasonable improvement (avoids checking generated/vendored files) but is orthogonal to the server feature. Consider separating it if this PR needs to be reverted independently.

Architecture Notes

• The choice to load policies once at startup and reuse evaluators is correct for a persistent server — it avoids repeated downloads and OPA compilation. The trade-off (restart to pick up policy changes) is clearly documented.
• Returning HTTP 200 for all completed evaluations (with success: false for violations) follows API best practices — HTTP status codes reflect transport/server errors, not application-level outcomes.
• The input.NewInput(ctx, nil, s.cfg.Policy) call passes nil for paths, which is correct since the server receives inputs per-request rather than at startup.

---

Labels: PR adds a new HTTP server mode to the CLI, touching CLI flags, internal server package, evaluator interface docs, and acceptance tests

Previous run (2)

Review — comment

Scope: New --server mode for ec validate input — adds a persistent HTTP/REST server for policy evaluation with health probes, graceful shutdown, and signal handling.

Summary

This is a well-structured feature addition that introduces server mode to the existing validate input command. The architecture is sound: evaluators are initialized once at startup, policy downloads are cached via sync.OnceValues, and each request creates its own conftest runner and temp file, making concurrent request handling safe. The PR includes thorough unit tests (handler, middleware, lifecycle, timeout), acceptance tests (4 Gherkin scenarios), documentation updates, and proper signal handling in cmd/root.go.

Findings

1. Server binds to all network interfaces by default — medium / security

File: internal/server/server.go (line ~89 in new file)
Code: Addr: fmt.Sprintf(":%d", s.cfg.Port)

The server binds to 0.0.0.0 (all interfaces) by default. The existing TODO acknowledges this (// TODO: add a --server-address flag), but shipping without a --server-address flag means the server is network-accessible from any interface out of the box. Users running ec validate input --server on a machine with a public IP will expose the unauthenticated evaluation endpoint externally.

Remediation: Consider defaulting Addr to 127.0.0.1:%d (localhost-only) and adding --server-address to allow explicit opt-in to network exposure. This follows the principle of least privilege — local-only by default, explicitly bindable to 0.0.0.0 when needed. If deferring this to a follow-up, the documentation and --server help text should warn that the server listens on all interfaces.

2. No authentication or rate limiting on evaluation endpoint — medium / security

File: internal/server/server.go, internal/server/handler.go

The server exposes POST /v1/validate/input with no authentication, authorization, or rate limiting. Each evaluation request triggers policy evaluation with a 90-second timeout, and there is no limit on concurrent evaluations. An adversary (or misconfigured client) could exhaust server resources by sending many concurrent requests.

This is acceptable for an initial implementation, but should be documented as a known limitation. The combination of network-accessible binding (finding #1) and no auth means anyone who can reach the port can consume server resources.

Remediation: For the initial PR, at minimum add a note to the command help text and docs that the server has no built-in authentication and should be deployed behind a reverse proxy or network policy in production. Consider adding --max-concurrent-evaluations as a follow-up to bound resource usage.

3. Concurrent request safety relies on evaluator implementation details — low / correctness

File: internal/server/server.go (lines ~70-80 in new file), internal/server/handler.go

The server comment correctly notes that evaluators are safe to share because policy downloads are cached via sync.OnceValues and each Evaluate call creates its own conftest runner. I verified this: conftestEvaluator.Evaluate (conftest_evaluator.go:414) creates a fresh conftestRunner per call, and the shared fields (workDir, policyDir, dataDir) are read-only after init. The downloadCache uses sync.Map.

However, this safety guarantee is implicit — there's no interface contract or documentation in the Evaluator interface stating that Evaluate must be safe for concurrent use. If a future evaluator implementation is added that mutates shared state in Evaluate, the server would silently break.

Remediation: Consider adding a // Evaluate is safe for concurrent use comment to the Evaluator interface definition, or document this requirement in the interface's godoc.

Positive observations

• Graceful shutdown is properly implemented with signal.NotifyContext in cmd/root.go and a 10-second shutdown timeout in the server.
• Request body validation enforces a 10MB limit, checks for empty bodies, and validates JSON/YAML format before processing.
• Error information leakage prevention — internal error details (file paths, engine errors) are not exposed to API clients.
• Recovery middleware prevents panics from crashing the server.
• Test coverage is strong: unit tests cover all handler paths (empty body, invalid input, success, violations, evaluation error, timeout, YAML input), middleware, and server lifecycle. Acceptance tests cover end-to-end scenarios.
• Proper temp file cleanup via defer os.Remove(tmpPath) prevents disk exhaustion.
• Status logging middleware with latency tracking aids operational monitoring.

Verdict

The implementation is solid and well-tested. The two medium findings are security hardening items that should be tracked for follow-up before production deployment, but don't block the feature from landing. The requires-manual-review label is appropriate given the scope of this change.

---

Note: This PR carries the requires-manual-review label. Automated review covers code-level concerns; human reviewers should additionally assess the operational deployment model and whether the security posture is acceptable for the intended use case.

---

Labels: PR adds new server/HTTP feature with security surface and CLI integration

Previous run (3)

Review — ec validate input --server

This PR adds a persistent HTTP/REST server mode to ec validate input, exposing policy evaluation via POST /v1/validate/input with Kubernetes-style health probes (/live, /ready). The implementation is well-structured: clean separation of concerns across server.go, handler.go, and middleware.go; proper signal handling in cmd/root.go; comprehensive unit and acceptance tests.

Findings

1. Concurrent evaluator safety — shared evaluators across HTTP requests (medium)

File: internal/server/handler.go (line ~97), internal/server/server.go (line ~74)

The server loads evaluators once at startup (input.NewInput) and reuses them across all concurrent HTTP requests:

for _, e := range s.evaluators {
    results, err := e.Evaluate(evalCtx, evaluator.EvaluationTarget{Inputs: []string{tmpPath}})
}

The conftestEvaluator.Evaluate() method re-downloads policy sources into its workDir on every call (s.GetPolicy(ctx, c.workDir, false)), creates a test runner referencing shared directories (c.policyDir, c.dataDir), and performs file I/O against those paths. When multiple HTTP requests invoke Evaluate concurrently on the same evaluator instance, these unsynchronized file system operations can race.

The CLI avoids this because each worker in validateInputCmd.RunE creates its own evaluator via ValidateInput → input.NewInput. The server's architecture differs — evaluators are shared.

Remediation: Either (a) create per-request evaluator instances (matching the CLI's per-worker pattern), (b) serialize evaluation with a mutex/semaphore, or (c) verify and document that GetPolicy is idempotent/safe for concurrent reads after initial download. Option (b) with a bounded semaphore would also provide natural backpressure.

2. Server binds to all interfaces by default (low)

File: internal/server/server.go (line ~87)

Addr: fmt.Sprintf(":%d", s.cfg.Port),

The server binds to 0.0.0.0, making it network-accessible on all interfaces. While this is appropriate for containerized/Kubernetes deployments, there's no --server-address flag to restrict binding to localhost for local development or security-sensitive environments.

The documentation curl example shows http://localhost:8080/..., which may give users the impression the server is localhost-only.

Remediation: Consider adding a --server-address flag (defaulting to 0.0.0.0 or "") so users can bind to 127.0.0.1 when desired. This could be a follow-up.

3. Global logger mutation in server mode (low)

File: internal/server/server.go (lines ~59–63)

if log.GetLevel() < log.InfoLevel {
    log.SetLevel(log.InfoLevel)
}
log.SetFormatter(&log.JSONFormatter{})

This modifies the global logrus logger (level and formatter). Since server mode is a terminal execution path (the server runs until shutdown), this is acceptable in practice. The code comment explains the rationale clearly. Noting it as a design choice rather than an issue — if the server were ever embedded in a larger process, this would need revisiting.

4. Port allocation TOCTOU in acceptance tests (low)

File: acceptance/cli/server.go (lines ~91–96)

The test allocates a free port by binding to :0, closing the listener, then starting the server on that port. Between close and bind, another process could claim the port. This is a standard pattern in test code and is mitigated by waitForReady's retry loop, so it's unlikely to cause flaky tests in practice.

Signal handling change scope

The cmd/root.go change replaces context.Background() with signal.NotifyContext for SIGINT/SIGTERM. This affects all commands, not just validate input --server. This is a positive change — it enables graceful cancellation for any command. However, any command that previously assumed the root context would never be cancelled should be verified. Given that Go's standard library and this codebase generally handle context cancellation correctly, the risk is low.

What's done well

• Request body validation — size limits (maxRequestBodySize), content-type-aware JSON/YAML validation, proper error responses
• Error message hygiene — internal details (file paths, engine errors) are not leaked to API clients
• Recovery middleware — panics in handlers don't crash the server
• Graceful shutdown — proper httpServer.Shutdown with timeout, evaluator cleanup via destroyEvaluators
• Test coverage — unit tests for all handlers/edge cases, acceptance tests covering happy path, violations, health endpoints, and invalid input
• Documentation — command help, asciidoc, and examples all updated consistently
• ReadHeaderTimeout set — avoids Slowloris-style attacks (good security practice for Go HTTP servers)

Verdict

The concurrent evaluator safety issue (finding 1) deserves attention before this sees production traffic. Under concurrent load, the shared evaluators could produce incorrect results or crash due to file system races. The remaining findings are low severity and could be addressed as follow-ups.

Action: comment — the concurrent access concern is worth resolving but does not necessarily block merge if the expected initial usage is single-client or low-concurrency.

---

Labels: PR adds a new HTTP server mode for policy evaluation (new feature), modifies CLI command structure

[simonbaird]

Last change is adding explanatory comments to address the potential concerns.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 3:40 PM UTC · Completed 3:45 PM UTC
Commit: 7c8ccca · View workflow run →

[simonbaird]

I've addressed a few more agentic review suggestions and nitpicks.

[coderabbitai]

🧹 Nitpick comments (1)

cmd/validate/input_test.go (1)

289-316: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value

Fixed 200ms timeout may be flaky in CI.

The test relies on completing policy loading, evaluator init, HTTP bind, and graceful shutdown all within a fixed 200ms window (Line 299). Under CI load this could intermittently fail before the server even finishes starting up, or pass without genuinely exercising a listening server. Consider a more generous timeout or asserting readiness (e.g., poll /ready) before triggering shutdown.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cmd/validate/input_test.go` around lines 289 - 316, The server-mode test in
Test_ValidateInputCmd is using a fixed 200ms context timeout that can race in
CI; make the test wait for actual readiness instead of relying on a short
deadline. Update the context setup and shutdown flow around validateCmd.Execute
so it either uses a more generous timeout or polls the server’s readiness
endpoint before canceling, ensuring the server has bound and started before the
test ends.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@cmd/validate/input_test.go`:
- Around line 289-316: The server-mode test in Test_ValidateInputCmd is using a
fixed 200ms context timeout that can race in CI; make the test wait for actual
readiness instead of relying on a short deadline. Update the context setup and
shutdown flow around validateCmd.Execute so it either uses a more generous
timeout or polls the server’s readiness endpoint before canceling, ensuring the
server has bound and started before the test ends.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Enterprise

Run ID: ad73f828-e941-4b96-8e50-0b83c88e1c96

📥 Commits

Reviewing files that changed from the base of the PR and between 7f68917 and 5973317.

📒 Files selected for processing (6)

• cmd/validate/input.go
• cmd/validate/input_test.go
• docs/modules/ROOT/pages/ec_validate_input.adoc
• internal/evaluator/conftest_evaluator.go
• internal/evaluator/evaluator.go
• internal/server/server.go

✅ Files skipped from review due to trivial changes (3)

• internal/evaluator/evaluator.go
• internal/evaluator/conftest_evaluator.go
• docs/modules/ROOT/pages/ec_validate_input.adoc

🚧 Files skipped from review as they are similar to previous changes (2)

• cmd/validate/input.go
• internal/server/server.go

[simonbaird]

Claude Opus 4.6's response to the remaining nitpick:

The 200ms timeout isn't a race — it's a minimum run time. The server starts in single-digit milliseconds (no-op policy, port 0), then the timeout fires and triggers graceful shutdown. The timeout would need to
be too short for the server to start, not too short for a sleep to fire. If CI is so slow that binding a localhost port takes 200ms, we have bigger problems. Bumping it to e.g. 1s "just in case" would just make the test slower for no real benefit.

[simonbaird]

The e2e fail log includes this, so maybe a flake:

  [TIMEDOUT] A suite timeout occurred
  In [It] at: /tmp/tmp.pg5FZDV85w/e2e-tests/tests/contract/contract.go:213 @ 07/07/26 21:43:16.638

[simonbaird]

/retest

[fullsend-ai-review]

🤖 Review · ❌ Terminated · Started 4:42 PM UTC · Ended 4:49 PM UTC
Commit: 8f05d87 · View workflow run →

[simonbaird]

I rebased, fixed some conflicts, squashed the "add a comment to explain a decision" commits.

[coderabbitai]

🧹 Nitpick comments (3)

Makefile (2)

230-230: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

git ls-files skips untracked files.

Switching from scanning . to git ls-files -z means brand-new files that haven't been git added yet are silently skipped by both the license check and lint-fix. Likely fine for CI (files are committed by then) but can surprise local dev workflows.

Also applies to: 238-238

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Makefile` at line 230, The license check and lint-fix steps now use git
ls-files -z, which skips brand-new untracked files and can hide missing headers
during local development. Update the Makefile targets that invoke addlicense and
lint-fix so they still cover untracked files as well as tracked ones, using the
existing license-check/lint-fix command blocks as the place to adjust the file
discovery logic.

---

230-232: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

Avoid double-running addlicense just to recover the exit code.

Piping to sed drops the exit status, so addlicense is invoked a second time solely to fail the build. This doubles cost for no functional gain; pipefail (if the recipe shell is bash) would let a single invocation preserve both output prefixing and exit status.

♻️ Suggested consolidation (requires bash shell for the recipe)

+SHELL := /bin/bash
+
 lint: tekton-lint go-mod-lint ## Run linter
 # addlicense doesn't give us a nice explanation so we prefix it with one
-	`@git` ls-files -z | xargs -0 go run -modfile tools/go.mod github.com/google/addlicense -c '$(COPY)' -y '' -s -check $(LICENSE_IGNORE) | sed 's/^/Missing license header in: /g'
-# piping to sed above looses the exit code, luckily addlicense is fast so we invoke it for the second time to exit 1 in case of issues
-	`@git` ls-files -z | xargs -0 go run -modfile tools/go.mod github.com/google/addlicense -c '$(COPY)' -y '' -s -check $(LICENSE_IGNORE) >/dev/null 2>&1
+	`@set` -o pipefail; git ls-files -z | xargs -0 go run -modfile tools/go.mod github.com/google/addlicense -c '$(COPY)' -y '' -s -check $(LICENSE_IGNORE) | sed 's/^/Missing license header in: /g'

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Makefile` around lines 230 - 232, The Makefile license-check recipe in the
addlicense target is running addlicense twice just to preserve the exit code
after piping through sed. Consolidate this into a single invocation by using a
shell that supports pipefail (for example, the recipe shell for this target) so
the existing git ls-files | xargs -0 | addlicense | sed flow keeps the prefixed
output and still returns the correct failure status. Update the addlicense
command block and its surrounding shell setup so the target no longer repeats
the check solely for exit-code recovery.

internal/server/server_test.go (1)

293-346: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Flaky test pattern: port reuse race + fixed sleep.

Binding a listener to get a free port then closing it before rebinding (Line 294-297) is a TOCTOU race — another process could grab the port in between, causing intermittent CI failures. Similarly, the fixed time.Sleep(100ms) (Line 330) to wait for server startup is timing-dependent and can flake under load.

Consider polling /live with retries/backoff instead of a fixed sleep, and consider using httptest.Server (which handles port allocation atomically) if the design doesn't strictly require exercising ListenAndServe/Shutdown directly.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/server/server_test.go` around lines 293 - 346, The
TestServerLifecycle test is flaky because it reserves a port by opening and
closing a listener before starting the HTTP server, and it uses a fixed sleep to
wait for readiness. Update TestServerLifecycle to avoid the port reuse TOCTOU by
using atomic test server setup such as httptest.Server when possible, or
otherwise keep the listener open through server startup; then replace the
hardcoded time.Sleep with polling the /live endpoint with retry/backoff until
the server is ready. Keep the shutdown assertions around
httpServer.ListenAndServe, httpServer.Shutdown, and the /live request flow.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@internal/server/server_test.go`:
- Around line 293-346: The TestServerLifecycle test is flaky because it reserves
a port by opening and closing a listener before starting the HTTP server, and it
uses a fixed sleep to wait for readiness. Update TestServerLifecycle to avoid
the port reuse TOCTOU by using atomic test server setup such as httptest.Server
when possible, or otherwise keep the listener open through server startup; then
replace the hardcoded time.Sleep with polling the /live endpoint with
retry/backoff until the server is ready. Keep the shutdown assertions around
httpServer.ListenAndServe, httpServer.Shutdown, and the /live request flow.

In `@Makefile`:
- Line 230: The license check and lint-fix steps now use git ls-files -z, which
skips brand-new untracked files and can hide missing headers during local
development. Update the Makefile targets that invoke addlicense and lint-fix so
they still cover untracked files as well as tracked ones, using the existing
license-check/lint-fix command blocks as the place to adjust the file discovery
logic.
- Around line 230-232: The Makefile license-check recipe in the addlicense
target is running addlicense twice just to preserve the exit code after piping
through sed. Consolidate this into a single invocation by using a shell that
supports pipefail (for example, the recipe shell for this target) so the
existing git ls-files | xargs -0 | addlicense | sed flow keeps the prefixed
output and still returns the correct failure status. Update the addlicense
command block and its surrounding shell setup so the target no longer repeats
the check solely for exit-code recovery.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Enterprise

Run ID: efe71d8a-f745-42a4-acb1-b4fc89c764a9

📥 Commits

Reviewing files that changed from the base of the PR and between 5973317 and 53dcde7.

⛔ Files ignored due to path filters (1)

• features/__snapshots__/validate_input_server.snap is excluded by !**/*.snap

📒 Files selected for processing (14)

• Makefile
• acceptance/acceptance_test.go
• acceptance/cli/server.go
• cmd/root.go
• cmd/validate/input.go
• cmd/validate/input_test.go
• docs/modules/ROOT/pages/ec_validate_input.adoc
• features/validate_input_server.feature
• internal/evaluator/conftest_evaluator.go
• internal/evaluator/evaluator.go
• internal/server/handler.go
• internal/server/middleware.go
• internal/server/server.go
• internal/server/server_test.go

✅ Files skipped from review due to trivial changes (3)

• internal/evaluator/conftest_evaluator.go
• internal/evaluator/evaluator.go
• docs/modules/ROOT/pages/ec_validate_input.adoc

🚧 Files skipped from review as they are similar to previous changes (9)

• features/validate_input_server.feature
• acceptance/acceptance_test.go
• cmd/root.go
• internal/server/middleware.go
• cmd/validate/input_test.go
• acceptance/cli/server.go
• internal/server/server.go
• cmd/validate/input.go
• internal/server/handler.go

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 4:42 PM UTC · Completed 4:49 PM UTC
Commit: 8f05d87 · View workflow run →

[simonbaird]

I'm ignoring the two CodeRabbit nitpicks about the license check change. Made changes for a couple of fullsend's fresh nitpicks.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 5:21 PM UTC · Completed 5:33 PM UTC
Commit: 8f05d87 · View workflow run →