Prepare the analysis engine for public use - #4
Conversation
Signed-off-by: Zbynek Roubalik <zroubalik@gmail.com>
There was a problem hiding this comment.
🟡 Changes recommended
There are a couple of concrete contract/safety issues to address (clarifying the pre-aggregated CPU aggregatedUsage requirement in the analyzer docs and bounding stdin request size to avoid unbounded resource consumption).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR packages the existing analysis engine as a standalone, offline-capable kedify-analyzer executable with a strict stdin/stdout JSON protocol, plus CI/release automation and documentation to distribute it separately from the public CLI.
Changes:
- Added
cmd/kedify-analyzerbinary that reads a single versioned JSON request from stdin and writes a deterministic JSON response to stdout (with stable exit codes). - Added executable-level tests that build/run the analyzer and validate both happy-path determinism and invalid-input handling against existing engine fixtures.
- Added GoReleaser config and GitHub Actions workflows to build snapshot archives on PRs and publish release artifacts on version tags; documented local invocation and distribution/discovery.
File summaries
| File | Description |
|---|---|
| README.md | Documents the local analyzer protocol, exit codes, discovery order, and air-gapped usage. |
| cmd/kedify-analyzer/main.go | Implements the analyzer stdin→analysis.Analyze→stdout JSON bridge with strict decoding and stable exit codes. |
| cmd/kedify-analyzer/main_test.go | Builds/executes the analyzer and asserts determinism + invalid-request behavior using existing engine fixtures. |
| .goreleaser.yaml | Defines cross-platform builds, archives, checksums, and snapshot versioning for kedify-analyzer. |
| .gitignore | Ignores GoReleaser dist/ output. |
| .github/workflows/release.yaml | Publishes tagged releases via GoReleaser. |
| .github/workflows/pr-check.yaml | Validates snapshot packaging via GoReleaser on PRs. |
Review details
- Files reviewed: 6/7 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>
Signed-off-by: Zbynek Roubalik <zroubalik@gmail.com>
Signed-off-by: Zbynek Roubalik <zroubalik@gmail.com>
There was a problem hiding this comment.
🟡 Changes recommended
The PR snapshot packaging workflow is likely to be unreliable due to a shallow checkout (and a couple of security/least-privilege hardening tweaks should be addressed).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
cmd/kedify-analyzer/main.go:43
runreads from stdin without any size limit. A very large request can cause excessive memory/CPU usage (especially via huge arrays/strings) and makes the analyzer easier to DoS in automation. Consider placing a reasonable upper bound on request size before decoding.
func run(stdin io.Reader, stdout, stderr io.Writer) int {
decoder := json.NewDecoder(stdin)
decoder.DisallowUnknownFields()
- Files reviewed: 7/8 changed files
- Comments generated: 2
- Review effort level: Lite
Signed-off-by: Zbynek Roubalik <zroubalik@gmail.com>
Signed-off-by: Zbynek Roubalik <zroubalik@gmail.com>
There was a problem hiding this comment.
🟡 Changes recommended
The analyzer’s exit-code contract for stdin read failures is inconsistent (and should be treated as an internal I/O failure) and the README/tests should be aligned to prevent regressions.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 7/8 changed files
- Comments generated: 3
- Review effort level: Lite
Signed-off-by: Zbynek Roubalik <zroubalik@gmail.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The analyzer’s exit-code contract is internally inconsistent (stdin read errors are treated as “invalid request”), and the docs/implementation should align before consumers rely on the stable protocol.
Review details
Suppressed comments (2)
cmd/kedify-analyzer/main.go:48
io.ReadAllfailures are environmental/IO problems rather than a request validation failure, but they currently return exit code 2. That makes it hard for callers to distinguish “bad input” from “couldn’t read stdin” and doesn’t align with the README’s exit-code meanings.
if err != nil {
writeDiagnostic(stderr, "unable to read request: %v", err)
return exitInternal
}
README.md:68
- If stdin read errors return exit code 1 (internal/IO) rather than 2 (invalid request), this table should reflect that so consumers can handle IO failures vs validation failures consistently.
| `1` | The analyzer could not read its request or write its response. |
- Files reviewed: 7/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
Minor but concrete documentation/packaging hygiene issues (Windows build snippet and ignoring locally-built binaries) should be corrected to prevent confusion and accidental commits.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
README.md:38
- The build/run snippet is not Windows-friendly:
go build -o kedify-analyzer ...will not produce akedify-analyzer.exefile name, and the next line shows invokingkedify-analyzerwithout the.exesuffix. This contradicts the later Windows note and can cause copy/paste failures on Windows.
- Files reviewed: 7/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The documented Unix invocation cannot run the newly built local executable unless the current directory is already on PATH.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 7/8 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🟢 Approval recommended
The implementation, tests, packaging configuration, workflows, and documentation consistently satisfy the stated analyzer contract.
Review details
- Files reviewed: 7/8 changed files
- Comments generated: 0 new
- Review effort level: Balanced
9ae0dd9 to
93480c8
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
The documented local invocation fails when the current directory is not on PATH.
Review details
Suppressed comments (1)
README.md:38
- The preceding build writes the executable into the current directory, but this command searches
PATH; on standard Linux/macOS setups where.is not onPATH, the documented invocation fails with “command not found.” Invoke the freshly built file via./kedify-analyzer.
kedify-analyzer < request.json > response.json
- Files reviewed: 7/8 changed files
- Comments generated: 0 new
- Review effort level: Balanced
902d5e0 to
f784548
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The analyzer and packaging described by the PR are absent, and the backup workflow uses mutable third-party action tags.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
.github/workflows/repo-backup-template.yml:42
- This third-party action is also selected through a mutable tag and runs in the same job that handles the GCP credential. Pin the action to a reviewed full commit SHA so an upstream tag move cannot alter the backup job unexpectedly.
uses: google-github-actions/setup-gcloud@v1
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Balanced
f784548 to
9a35c8b
Compare
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
Signed-off-by: Zbynek Roubalik <zroubalik@gmail.com>
9a35c8b to
08efeb1
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The PR workflow uses mutable action tags with write permissions, and the backup template misdocuments its secret interface.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (5)
.github/workflows/pr-check.yaml:25
- This action is referenced through a mutable major-version tag while the job has write permissions. Pin the reviewed commit SHA so a moved tag cannot change the code executed by PR checks.
uses: actions/setup-go@v6
.github/workflows/pr-check.yaml:45
- This third-party action is selected by a mutable tag, so retagging could inject unreviewed code into a job with write permissions. Pin the exact reviewed commit, as the backup workflow already does for external actions.
uses: golangci/golangci-lint-action@v9.2.1
.github/workflows/pr-check.yaml:65
- This scanner is a third-party action referenced by a mutable release tag. Because the job grants write permissions, pin the reviewed commit SHA to prevent tag movement from changing executed code.
uses: securego/gosec@v2.24.7
.github/workflows/pr-check.yaml:86
- The SARIF uploader is also referenced through a mutable major tag and receives
security-events: write. Pin its reviewed commit SHA so the privileged action implementation cannot change through retagging.
uses: github/codeql-action/upload-sarif@v4
.github/workflows/pr-check.yaml:51
- The floating
v1reference can change without review and would execute third-party code with this workflow's write-scoped token. Pin the action to the reviewed commit SHA.
uses: dominikh/staticcheck-action@v1
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Balanced
Signed-off-by: Zbynek Roubalik <zroubalik@gmail.com>
There was a problem hiding this comment.
🟡 Changes recommended
The gosec action transitively uses a mutable container tag, weakening the workflow’s supply-chain pinning.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
Why
The public Kedify CLI should use the same recommendation calculation as Dashboard API while remaining a single binary. A standalone analyzer process is unnecessary now that the engine module is public.
What
Outcome
Dashboard API and the CLI can pin and import one public calculation package. The CLI runs offline analysis without a second executable, private build credentials, or runtime download.
Verification
go test -count=1 -race ./...go vet ./...