refactor(knowledge): extract generic CUE compiler core and projection… - #1
Conversation
… inferred domain discovery
…CLI health checks through it
876b1e6 to
97e6fde
Compare
There was a problem hiding this comment.
Supply Chain Security Review
✅ Approve — 1 finding in 1 file
All 28 dependency updates are published 7+ days ago with no recency concerns. Two vulnerabilities (GHSA-vvgj-x9jq-8cj9, GO-2026-5676) in quic-go are fixed by the upgrade to v0.60.0. The persisting GO-2026-5932 (unmaintained golang.org/x/crypto/openpgp) is a transitive dep not directly used by this project — no exploitable path. New CI workflow uses SHA-pinned actions throughout.
Tag @mendral-app with feedback or questions. View session
|
@mendral-app please provide a detailed code review covering design/architecture, security, patterns, code organization, code quality, modularity, reliability, error handling, documentation, strengths, weaknesses, and features, with clear recommendations for improvement. |
|
@mendral-app perform a deep, evidence-based code review of the entire codebase, not just a summary. Inspect the implementation file by file and identify concrete issues in architecture, design, security, code quality, patterns, maintainability, performance, testing, documentation, and feature completeness. For every finding, include the affected file and code section, explain why it matters, rate its severity, and propose a specific fix or improved implementation. Also highlight strengths, questionable design decisions, hidden risks, technical debt, missing tests, and architectural inconsistencies, then finish with a prioritized remediation plan. |
There was a problem hiding this comment.
Needs attention — 4 issues across 3 files
The architecture is well-designed with clean separation between compiler, runtime, and transport layers. The packageInstance path-traversal fix in evaluator.go is correctly implemented, but sourceProvenance in runtime.go still performs an unguarded path join — this is the only blocking security issue (flagged in five prior rounds). The variable shadowing in projectWithEvaluationInput and the dead import in query.go are minor but worth fixing for clarity.
What this PR does
Extracts a generic CUE knowledge compiler core from the existing diagram evaluator, adds a transport-independent runtime with catalog/describe/get/query/eval/provenance/health operations, exposes these over HTTP routes and CLI subcommands, adds a CI workflow, and wires the catalog into a new frontend Knowledge panel. The design is intentionally agent-safe: the query API never accepts arbitrary CUE expressions, and evaluations require named declarations with schema-validated inputs.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<assessment>
The architecture is well-designed with clean separation between compiler, runtime, and transport layers. The `packageInstance` path-traversal fix in `evaluator.go` is correctly implemented, but `sourceProvenance` in `runtime.go` still performs an unguarded path join — this is the only blocking security issue (flagged in five prior rounds). The variable shadowing in `projectWithEvaluationInput` and the dead import in `query.go` are minor but worth fixing for clarity.
</assessment>
<file name="backend/internal/knowledge/runtime.go">
<issue location="backend/internal/knowledge/runtime.go:478">
`sourceProvenance` joins `project.Package` to root without the `..`-escape check that `packageInstance` in `evaluator.go:629-641` implements. A crafted `Package` value (e.g. `../../etc`) causes `WalkDir` to read arbitrary directories outside the module root. This has been flagged in five previous review rounds and remains unfixed.
</issue>
<issue location="backend/internal/knowledge/runtime.go:407">
Loop variable `name` shadows the function parameter `name` (the evaluation name). While technically correct in Go (the parameter is referenced again after the loop at line 410), this makes the code fragile — a future edit inside the loop using `name` expecting the parameter will silently get the map key.
</issue>
</file>
<file name="backend/internal/knowledge/query.go">
<issue location="backend/internal/knowledge/query.go:23">
Blank-identifier assignment `var _ = evaluation.ErrTimeout` keeps the `evaluation` import alive but serves no purpose — the package is not otherwise used in this file. Remove it or replace with a meaningful re-export.
</issue>
</file>
<file name=".github/workflows/backend-ci.yml">
<issue location=".github/workflows/backend-ci.yml:15">
Adding an explicit `permissions` block limits the GITHUB_TOKEN scope, reducing blast radius if a step is compromised.
</issue>
</file>
Tag @mendral-app with feedback or questions. View session
| packageDir := root | ||
| if project.Package != "" && project.Package != "." { | ||
| packageDir = filepath.Join(root, project.Package) | ||
| } |
There was a problem hiding this comment.
security (P1): sourceProvenance joins project.Package to root without the ..-escape check that packageInstance in evaluator.go:629-641 implements. A crafted Package value (e.g. ../../etc) causes WalkDir to read arbitrary directories outside the module root. This has been flagged in five previous review rounds and remains unfixed.
Suggested change
| packageDir := root | |
| if project.Package != "" && project.Package != "." { | |
| packageDir = filepath.Join(root, project.Package) | |
| } | |
| packageDir := root | |
| if project.Package != "" && project.Package != "." { | |
| if filepath.IsAbs(project.Package) { | |
| return Provenance{}, fmt.Errorf("package must be a relative path") | |
| } | |
| packageDir = filepath.Join(root, project.Package) | |
| rel, err := filepath.Rel(root, packageDir) | |
| if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { | |
| return Provenance{}, fmt.Errorf("package path escapes module root") | |
| } | |
| } |
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/internal/knowledge/runtime.go, line 478:
<issue>
`sourceProvenance` joins `project.Package` to root without the `..`-escape check that `packageInstance` in `evaluator.go:629-641` implements. A crafted `Package` value (e.g. `../../etc`) causes `WalkDir` to read arbitrary directories outside the module root. This has been flagged in five previous review rounds and remains unfixed.
</issue>
| for name, content := range project.Overlay { | ||
| overlay[name] = content | ||
| } |
There was a problem hiding this comment.
maintainability (P3): Loop variable name shadows the function parameter name (the evaluation name). While technically correct in Go (the parameter is referenced again after the loop at line 410), this makes the code fragile — a future edit inside the loop using name expecting the parameter will silently get the map key.
Suggested change
| for name, content := range project.Overlay { | |
| overlay[name] = content | |
| } | |
| for overlayName, content := range project.Overlay { | |
| overlay[overlayName] = content | |
| } |
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/internal/knowledge/runtime.go, line 407:
<issue>
Loop variable `name` shadows the function parameter `name` (the evaluation name). While technically correct in Go (the parameter is referenced again after the loop at line 410), this makes the code fragile — a future edit inside the loop using `name` expecting the parameter will silently get the map key.
</issue>
| return c.engine.EvalQuery(ctx, sourceFrom(request), expression) | ||
| } | ||
|
|
||
| var _ = evaluation.ErrTimeout |
There was a problem hiding this comment.
maintainability (P3): Blank-identifier assignment var _ = evaluation.ErrTimeout keeps the evaluation import alive but serves no purpose — the package is not otherwise used in this file. Remove it or replace with a meaningful re-export.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/internal/knowledge/query.go, line 23:
<issue>
Blank-identifier assignment `var _ = evaluation.ErrTimeout` keeps the `evaluation` import alive but serves no purpose — the package is not otherwise used in this file. Remove it or replace with a meaningful re-export.
</issue>
| test: | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
security (P3): Adding an explicit permissions block limits the GITHUB_TOKEN scope, reducing blast radius if a step is compromised.
Suggested change
| test: | |
| runs-on: ubuntu-latest | |
| test: | |
| runs-on: ubuntu-latest | |
| permissions: | |
| contents: read |
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/backend-ci.yml, line 15:
<issue>
Adding an explicit `permissions` block limits the GITHUB_TOKEN scope, reducing blast radius if a step is compromised.
</issue>
There was a problem hiding this comment.
Supply Chain Security Review
✅ Approve — 3 findings across 2 files
All 28 dependency version bumps are published 7+ days ago with no recency concerns. Two vulnerabilities (GHSA-vvgj-x9jq-8cj9 in quic-go) are fixed by this PR. One persisting low-severity advisory (GO-2026-5932: golang.org/x/crypto/openpgp is unmaintained) remains—this is a transitive dep from go-git and not directly used; no action needed now. GitHub Actions in both new workflows are properly pinned to commit SHAs.
Tag @mendral-app with feedback or questions. View session
| - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 | ||
|
|
||
| - name: install pnpm | ||
| run: npm install -g pnpm@11 |
There was a problem hiding this comment.
maintainability (P3): pnpm is installed with a floating major-version tag (@11). A patch-level pin (e.g. pnpm@11.11.0) prevents silent behavior changes between runs.
Suggested change
| run: npm install -g pnpm@11 | |
| run: npm install -g pnpm@11.11.0 |
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/binaries.yml, line 27:
<issue>
pnpm is installed with a floating major-version tag (`@11`). A patch-level pin (e.g. `pnpm@11.11.0`) prevents silent behavior changes between runs.
</issue>
| jobs: | ||
| test: |
There was a problem hiding this comment.
security (P3): No explicit permissions: block; the workflow inherits the repository default token scope. Adding a least-privilege block limits blast radius if a dependency or step is compromised.
Suggested change
| jobs: | |
| test: | |
| jobs: | |
| test: | |
| runs-on: ubuntu-latest | |
| permissions: | |
| contents: read |
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/backend-ci.yml, line 20:
<issue>
No explicit `permissions:` block; the workflow inherits the repository default token scope. Adding a least-privilege block limits blast radius if a dependency or step is compromised.
</issue>
| jobs: | ||
| build: |
There was a problem hiding this comment.
security (P3): No explicit permissions: block on the build job. Restricting to contents: read follows least-privilege for a build-and-upload workflow.
Suggested change
| jobs: | |
| build: | |
| jobs: | |
| build: | |
| runs-on: ubuntu-latest | |
| permissions: | |
| contents: read |
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/binaries.yml, line 20:
<issue>
No explicit `permissions:` block on the build job. Restricting to `contents: read` follows least-privilege for a build-and-upload workflow.
</issue>
… seams