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]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Enterprise

Run ID: 49e4fdc1-6cdf-4cf5-b103-109c55470c51

📥 Commits

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

📒 Files selected for processing (2)

• internal/evaluator/conftest_evaluator.go
• internal/server/server.go

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

• internal/evaluator/conftest_evaluator.go

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

• internal/server/server.go

---

📝 Walkthrough

Walkthrough

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

Changes

Validate Input Server Mode

Layer / File(s)	Summary
Server runtime internal/server/server.go, internal/server/handler.go, internal/server/middleware.go, internal/evaluator/conftest_evaluator.go	Adds server startup/shutdown, routes, readiness endpoints, request handling, JSON error/report responses, logging middleware, and evaluator teardown.
CLI wiring cmd/root.go, cmd/validate/input.go, cmd/validate/input_test.go, docs/modules/ROOT/pages/ec_validate_input.adoc	Adds signal-aware execution, server-mode flags and branching, CLI validation, server-mode tests, and updated command docs/examples.
Acceptance coverage acceptance/acceptance_test.go, acceptance/cli/server.go, features/validate_input_server.feature	Registers server step definitions and adds end-to-end feature coverage for server startup, requests, and assertions.
Server tests internal/server/server_test.go	Expands unit coverage for endpoints, validation paths, timeout behavior, panic recovery, and server lifecycle.

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 directly matches the changeset by describing the new ec validate input --server web service.
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 88.93617% 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.23%	8 Missing ⚠️

Flag	Coverage Δ
acceptance	53.95% <80.00%> (+0.51%)	⬆️
generative	17.05% <0.00%> (-0.33%)	⬇️
integration	28.31% <11.91%> (-0.36%)	⬇️
unit	71.56% <61.70%> (-0.20%)	⬇️

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.67% <100.00%> (+1.52%)	⬆️
internal/evaluator/conftest_evaluator.go	88.65% <ø> (ø)
internal/server/middleware.go	100.00% <100.00%> (ø)
internal/server/server.go	88.23% <88.23%> (ø)
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 — 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

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 →