Issues foundation hardening: dependency graph, vocabulary, structure#828
Merged
Conversation
- Rename category `type Group` → `CategoryGroup` (collided with Issue.Group = API group). - Export `issues.SeverityRank`; delete the byte-identical `composeSeverityRank` (server) + `mcpComposeSeverityRank` (mcp) clones, route both at it. - `subject.IssueID(scope,key,category)` → generic `subject.StableID(scope,key,discriminator)` — subject produces subject identity; issue-id (subject+category) is composed by the issues layer. Byte layout unchanged (no re-key). - Drop the unused `podMeta metav1.Object` param from `subject.ResolveSubject` (made the API look Pod-coupled; it wasn't used). - Document `PodOwnerResolver`'s RS→Deployment hash-strip as a label-free APPROXIMATION (wrong for Argo Rollout / CRD-owned ReplicaSets) — use the cache-aware resolver when a cache is available.
The domain model (Issue/Severity/Source/Category/Ref) no longer imports internal/filter; the CEL alias + Filters live in options.go. HTTP/MCP own CEL compilation; the data types stay a pure leaf.
The `Problem` struct was a near-synonym of `Issue` (the classified/grouped public type), forcing readers to learn an arbitrary distinction between two layers of one pipeline. Renamed the transport-neutral detector-output type to `Detection` (it IS a raw detection a detector emits), consumed by the issues classifier + dashboard + MCP. Semantic rename via gopls — the `Problem` FIELD in NodeProblem/HPAProblem/ CronJobProblem helper structs and the Detect*Problems function names are correctly untouched; no wire/JSON change. Resolves the Problem/Issue vocabulary collision.
Audit should not be the identity foundation. Moved ResourceKey + GroupForBuiltinKind into a dependency-free leaf pkg/resourceid; pkg/audit now re-exports them (existing bp.ResourceKey callers unchanged), and pkg/subject + internal/issues depend on resourceid directly instead of pkg/audit.
…n logic Package inventory (pkg/packages) shouldn't own generic condition-state semantics for issues/GitOps/CRDs. Moved the transient-reason set + the genuine-failure carve-out (ArtifactFailed/ChartNotReady) into a dependency-free leaf pkg/conditions, exposing IsTransientConditionReason (health display) and IsInProgressForIssues (transient MINUS genuine failures, for issue detectors). - pkg/packages re-exports IsTransientConditionReason (existing callers unchanged). - internal/issues drops its local genuineFailureReason map + pkg/packages dep, uses conditions.IsInProgressForIssues. - internal/k8s Flux detector's hand-rolled isGitOpsInProgressReason replaced by conditions.IsInProgressForIssues — the Flux detector and the CRD noise-floor now share ONE in-progress predicate instead of two lists that could drift (A2).
…seCondition) Collapsed three copies of "walk status.conditions, find a False entry" — the verbatim closure in DetectCAPIProblems, the narrow readyFalseCondition in the Flux detector, and internal/issues.FindFalseCondition — into a single conditions.FindFalseCondition (v1beta2-fallback aware, condTypes-parameterized). internal/issues/conditions.go deleted; the CAPI + Flux detectors + the generic CRD fallback all route through the one reader. Added pkg/conditions tests.
problems.go was the v0 all-in-one "problems" feature; in the issues architecture its real role is the DETECTOR LAYER — it reads the cache and emits []Detection (raw findings) that internal/issues classifies + groups, and that the dashboard + MCP also consume. The name and 1024-line god-file structure hid that. - Renamed problems.go → detection.go (matches the Detection type; P1 follow-through). - Split by detector to match the clean single-purpose files in internal/issues: detection.go (Detection type + the core DetectProblems workload/pod/service/etc detector + shared helpers), capi.go (DetectCAPIProblems), gitops.go (DetectGitOpsProblems + Argo/Flux helpers). Siblings health.go/missing_refs.go/ scheduling.go were already separate detectors. - Added a layer-framing doc on Detection: detectors → classify/group (issues) → render; this is the bottom tier, not a parallel surface to issues.
Mirrors the detector-layer split: one 657-line file → cohesive single-purpose files matching the rest of the package. - compose.go: Provider/ComposeStats + Compose/ComposeWithStats (the pipeline). - source_conditions.go: the generic CRD .status.conditions source + curated-kind exclusion + transient suppression. - normalize.go: Detection→Issue (fromProblem/classifyIssue/resolveGroup). - dedupe.go: evidence-level dedup (scheduling-over-problem, workload-over-child). - filters.go: severity/kind + cluster-scoped-access filters + SeverityRank. No logic change; pure reorganization to cut the high-conflict god-file.
…avigate The Audit and Issues render sites carried verbatim-identical inline onNavigateToResource callbacks (kindToPlural → setSelectedResource → rebuild searchParams → navigate to /resources/<plural>). Extracted one navigateToResourceList useCallback; both pass it. (Topology/Workload/Helm navigate sites differ and are left as-is.)
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c2cd097. Configure here.
| continue | ||
| } | ||
| _, reason, msg, since, ok := conditions.FindFalseCondition(obj, "Ready") | ||
| if !ok || conditions.IsInProgressForIssues(reason) { |
There was a problem hiding this comment.
Flux detector silently suppresses three new failure reasons
Medium Severity
detectFluxProblems replaced its old narrow isGitOpsInProgressReason (7 reasons) with conditions.IsInProgressForIssues (10 reasons), silently adding Creating, Issuing, and Waiting as suppressed in-progress reasons. Flux objects with these condition reasons were previously surfaced as issues and will now be dropped. The comment still says "NARROW in-progress set" but the set is wider than before. In a PR that claims "no behavior change," this widens the suppression window for the Flux detector.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit c2cd097. Configure here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pure-refactor pass on the Issues stack from the architecture review — no behavior change (the precision/recall + public-contract work lives in #811). Tightens the seams the reviewer flagged so the layer is a clean foundation for the Applications / Packages surfaces to build on. Stacked on #811 (
feat/issues-ui); retarget tomainonce that merges.Dependency graph
pkg/resourceid(new, leaf):ResourceKey+GroupForBuiltinKindmoved out ofpkg/audit.subject/issuesno longer depend onauditfor identity;auditre-exports (callers unchanged). Audit is no longer the identity foundation.pkg/conditions(new, leaf): the transient-reason vocabulary + the genuine-failure carve-out moved out ofpkg/packages.issues/k8sdepend on the neutral leaf, not on package-inventory; the two transient narrowings (Flux detector + CRD noise-floor) collapse to one sharedIsInProgressForIssues.types.godepured —Filters/CEL moved tooptions.go; the domain model (Issue/Severity/Source/Category/Ref) is now a pure leaf with no CEL import.IssueID→ genericsubject.StableID(subject produces subject identity; issue-id = subject+category is composed by the issues layer).ResolveSubjectlost its unusedpodMetaparam.Vocabulary + dedup
Problemtype →Detection(gopls semantic rename) — kills theProblem/Issuesynonym; helper structs +Detect*funcs correctly untouched.Group→CategoryGroup(collided withIssue.Group= API group).issues.SeverityRank— deleted two byte-identical clones (server + MCP).conditions.FindFalseCondition— collapsed three copies (CAPI closure, FluxreadyFalseCondition, the issues reader).Structure (god-file splits, matching the clean
issues/files)problems.go→detection.go+ split by detector intocapi.go/gitops.go, with a layer-framing doc: this is the detector layer (emits rawDetections);internal/issuesis the layer above (classify/group); dashboard + MCP also consume it — the generalized successor to the v0 "problems" feature, not a parallel surface.issues.go(657 lines) split →compose.go/source_conditions.go/normalize.go/dedupe.go/filters.go.navigateToResourceListextracted inApp.tsx(Audit + Issues had a verbatim-identical navigate callback).Not included (deliberate)
Detectionconstructor for the Age/Duration quartet): skipped — the 24 sites have genuine variation (separate age/duration, no-age HPA/CronJob, age-only Node, +crash/owner pods); a single constructor is a leaky abstraction and the clean subsets already have one.Affected→ kind-keyed map, a wire change): deferred as judgment-calls, not clear wins.go build ./...,make test, andmake tscall green.Note
Medium Risk
Large file moves and renames across issues/k8s/pkg with stated no-behavior-change intent; mistaken drift in compose/dedupe or condition suppression would affect live issue lists and IDs.
Overview
Foundation refactor for the Issues pipeline: split the monolithic
issues.go/problems.gointo focused files (compose,dedupe,filters,normalize,source_conditions;detection+capi/gitops), and move compose options/CEL types tooptions.goso domain types stay a leaf.Vocabulary and shared libs:
k8s.Problem→Detection; category rollupGroup→CategoryGroup(vsIssue.GroupAPI group);subject.IssueID→subject.StableIDwith identity keys from newpkg/resourceid(ResourceKey,GroupForBuiltinKind, audit re-exports); transient/false-condition logic centralized inpkg/conditions(FindFalseCondition,IsInProgressForIssues) replacing duplicated readers andinternal/issues/conditions.go; GitOps Flux path uses sharedFindFalseConditioninstead of localreadyFalseCondition.Dedup: exported
issues.SeverityRankreplaces identical server/MCP helpers;groupinguses it for sort/representative choice.Small product fix:
/.well-known/oauth-protected-resourceandoauth-authorization-serverreturn 404 so MCP clients don’t treat SPA HTML as OAuth metadata.Reviewed by Cursor Bugbot for commit c2cd097. Bugbot is set up for automated code reviews on this repo. Configure here.