Insights v2: add single-binary offline analysis - #15
Conversation
Signed-off-by: Zbynek Roubalik <zroubalik@gmail.com>
Signed-off-by: Zbynek Roubalik <zroubalik@gmail.com>
Signed-off-by: Zbynek Roubalik <zroubalik@gmail.com>
There was a problem hiding this comment.
🟢 Approval recommended
The implementation matches the stated offline invocation requirements and is covered by targeted tests, with only minor follow-up suggestions noted in review comments.
Pull request overview
Adds an offline “Insights v2” entry point to the public CLI by invoking a separately installed local analyzer binary and validating the request/response protocol so the CLI can produce machine-readable recommendations without importing private engine code or contacting Kedify SaaS.
Changes:
- Introduces
kedify analyze recommendations <snapshot-request>and wires it into the root CLI command tree. - Implements local analyzer discovery (explicit path → sibling binary →
PATH), bounded I/O, exit-code propagation, and protocol/schema/version validation. - Documents the offline request envelope and usage in the README, and adds focused unit tests for discovery/validation/I-O/limits.
File summaries
| File | Description |
|---|---|
| README.md | Documents offline analyzer invocation, discovery order, protocol/schema requirements, and size limits. |
| internal/cli/run.go | Registers the new analyze command group and the analyze recommendations subcommand. |
| internal/cli/analyze/recommendations.go | Implements snapshot ingestion, analyzer discovery/execution, response validation, and stdout/stderr behavior. |
| internal/cli/analyze/recommendations_test.go | Adds tests covering stdin/file input, discovery order, protocol validation, size limits, and exit-code propagation. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Signed-off-by: Zbynek Roubalik <zroubalik@gmail.com>
Signed-off-by: Zbynek Roubalik <zroubalik@gmail.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Handle analyzer exits where ExitCode() == -1 before approval.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
internal/cli/analyze/recommendations.go:78
- Handle the
ExitCode() == -1case before wrapping this error. Go returns-1when the analyzer was terminated by a signal or otherwise has no exit code; passing that throughCommandResultErrormakes the top-level CLI return an invalid, non-portable status and suppresses the useful termination error. Map only nonnegative child codes and return a wrapped error otherwise.
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Signed-off-by: Zbynek Roubalik <zroubalik@gmail.com>
There was a problem hiding this comment.
🟡 Changes recommended
Analyzer signal termination and oversized-error handling do not correctly preserve exit behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
Oversized analyzer output can hang execution and incorrectly override nonzero analyzer exit codes.
Review details
Suppressed comments (2)
internal/cli/analyze/recommendations.go:75
- The response cap only limits retained memory: once the buffer is full,
Writestill reports every byte as successfully consumed, socommand.Run()keeps draining stdout and waits for the analyzer to exit. An analyzer that continuously emits output can therefore hang the CLI indefinitely despite the documented 64 MiB limit. Abort/kill and reap the subprocess as soon as the first byte beyond the limit is observed.
response := cappedBuffer{limit: maxAnalyzerResponseBytes}
command.Stdout = &response
runErr := command.Run()
if response.exceeded {
internal/cli/analyze/recommendations.go:76
- Handle a nonzero analyzer exit before checking
response.exceeded. As written, an analyzer that writes more than 64 MiB and exits with (for example) code 2 is converted into a generic CLI error/exit 1, contradicting the promised exit-code preservation. The oversized-output check should apply after successful execution; a combined oversized-output/nonzero-exit regression test would cover this case.
if response.exceeded {
return fmt.Errorf("analyzer response exceeds %d-byte limit", maxAnalyzerResponseBytes)
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Signed-off-by: Zbynek Roubalik <zroubalik@gmail.com>
Signed-off-by: Zbynek Roubalik <zroubalik@gmail.com>
Signed-off-by: Zbynek Roubalik <zroubalik@gmail.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The recommender must be updated to clamp confidence to 95 for intervals above 168 hours.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
go.mod:13
- The pinned engine does not cap confidence at its
maximumConfidencevalue:recommendationConfidencecomputesmax(20, hours*95/168), so a valid snapshot withobservedIntervalHours: 336returns confidence190. Since this command accepts any positive interval and exposes the result directly, pin an engine revision that clamps confidence to 95 (and covers intervals above 168 hours) before release.
- Files reviewed: 6/7 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Signed-off-by: Zbynek Roubalik <zroubalik@gmail.com>
There was a problem hiding this comment.
🔵 Needs a closer look
JSON output can silently succeed when only partially written; the success-path test also needs meaningful analysis data.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
internal/cli/analyze/recommendations.go:45
- Structured command results elsewhere use
ctx.WriteOutput, whose implementation detectsn < len(data)(internal/output/output.go:43-49).json.Encoder.Encodedoes not detect a writer that short-writes without returning an error, so this command can exit successfully after emitting truncated JSON. Route this result throughctx.WriteOutput(..., "json")and initialize that dependency in these command tests.
internal/cli/analyze/recommendations_test.go:44
- The only successful analysis fixture has no containers and this assertion requires an empty result, so the test never exercises the command's core promise of passing observations through to evidence, data-quality, and recommendation output. Use a non-empty normalized snapshot and assert at least one calculated result and its key fields; keep a separate empty-input case if that behavior also matters.
if result.Results == nil || len(result.Results) != 0 {
t.Fatalf("results = %#v, want an empty array", result.Results)
- Files reviewed: 6/7 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Why
Users should run offline recommendation analysis with only the public Kedify CLI. The CLI and Dashboard API must use the same calculation implementation.
What
kedify analyze recommendationsfor a normalized snapshot file or stdin.github.com/kedify/recommender/analysispackage and call it in-process.There is no analyzer subprocess, executable discovery, runtime download, or private Go dependency. Prometheus and Kubernetes source adapters remain tracked by kedify/agent#618 and #14.
Verification
go test -count=1 -race ./...go vet ./...proxy.golang.orgwith private-module settings disabledDependency
The licensed public module is released as v0.1.0 and pinned here. Dashboard API #524 pins the same version.