Repository navigation
Show workload logs, alert rules, Helm releases and permissions in Findings - #1681
Merged
Merged
Conversation
PR Summary by QodoSurface logs, alerts, Helm, and permissions in Findings
AI Description
Diagram
High-Level Assessment
Files changed (8)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
Author
|
bugbot run |
Contributor
Author
|
Review round (Cursor Bugbot, head 602c190):
|
nadaverell
force-pushed
the
feat/findings-adapters
branch
from
September 8, 2026 14:52
7a99859 to
03efaa2
Compare
…dings evidence Findings dropped four results the agent already fetches. Each now becomes Radar-authored evidence from the producer's documented payload: - get_workload_logs reuses the logs kind, one observation per pod/container from one source, so a multi-container read stays citable as one result. logsError is a limitation, a zero-pod read is a receipt scoped to the workload, narrowHint is a coverage note. - get_prometheus_rules adds the alerts kind. Relevance comes from each instance's labels (target namespace plus a workload label or a controller-shaped pod name); rule-level labels only reach producer-related. A receipt is emitted only for a complete, evaluated firing/pending query that named nothing about the target, worded with the actual filters. Failing evaluation health is a limitation row. - get_helm_release adds the helm kind, adverse when the status is not deployed, a health issue is set, or the last operation failed. - get_subject_permissions adds the permissions kind: access checks render the verdict, subject reads render bindings and effective-rule counts. Target relevance requires the same turn's resource observation to name the ServiceAccount; no blast-radius inference from a payload without per-binding rules. Collapsed events cards lead with the newest warning's reason and message with counts secondary. Changes cards carry the producer's subject and open the Timeline filtered to it; the relationships card already opened its root.
Alert instances relate to the target only through a workload label or a pod a producer already tied to the workload in this projection; a controller- shaped pod name alone also fits a sibling such as api-worker. A re-read of the same rule advances the card, so a firing rule that goes inactive shows as resolved instead of the stale firing observation, and rules are keyed by expression and static labels since names repeat within a group. The alert receipt now requires an unfiltered active-state query whose every instance sits in another namespace. Permission subjects that no captured resource ties to the target are broader context, not supporting evidence, and the newest same-turn read of the target decides its ServiceAccount. Helm releases keep their storage namespace in identity and withhold a resource link the ref cannot express; an owned resource without an API version establishes nothing. Rule, count, truncation, effective-rule and zero-pod payload shapes are validated before projection. The Timeline link moves the namespace scope in state together with the URL, as the Timeline's own namespace prompt does, so the URL write and read effects cannot alternate.
Namespace exclusion can only prove an alert instance is elsewhere when the target itself is namespaced, so cluster-scoped targets no longer receive a receipt from other namespaces' instances. Identical rule rows within one response are distinct rules (group names repeat across rule files the producer does not return), told apart by order rather than merged as reads. A PodSpec is read only from kinds that carry one, so an ApplicationSet's template no longer implies the default ServiceAccount. A cluster-scoped changes subject opens the Timeline under the current namespace scope.
latestRelevance carried a raw NUL byte where observe spells the separator as an escape; both now go through groupKey, so a re-read of an inactive rule finds its firing card. The workload-logs subject map mirrors every kind the producer resolves, including rollouts. Owned Helm resources keep the producer's summary and message so a row without an issue still says what state it is in. The workload-logs fixture comment describes the shape rather than a run.
Two identical rule definitions in one response cannot be told apart, so they no longer share an identity with rows from an earlier read in arrival order; each read keeps its own cards and carries no history across. A rule whose evaluation health is anything but a reported ok, including an unrecognized value, counts as unevaluated for a negative.
… reads A citation can promote any single-subject broader card, so helm, alerts and permissions join resource, logs and crash in the focused set, and the pane says how many broader results of any kind it withheld. Repeated get_changes reads of one resource are revisions of one card whose title names the window, instead of look-alike cards per call. The producer serializes an empty events or changes result as null; both adapters read it as empty rather than malformed, which removed a false "couldn't summarize" coverage row. A coverage group with one limitation renders it once, with the source link on the summary row.
A DaemonSet that cannot schedule produced a classified Unschedulable issue for the workload and a startup card for its pods with the same reason and message. They are one fact seen from two sides, so the issue card now carries the pod list and the startup card is not emitted; a blocker no issue repeats keeps its own card.
… order A merged startup card listed its pods only in a body the card never opened, and a step's copy buttons put fenced blocks before the inline commands that preceded them in the text.
…ne where the change is Two ways this pane could show a complete-looking answer over an incomplete read. The Helm adapter recorded only the values error. The producer reports four — values, values diff, notes diff and resource diff — and three of them are Cloud role denials, so a viewer saw a release card whose diff had been withheld with nothing saying so. All four now become coverage limitations. Opening the Timeline for a cluster-scoped change carried the current namespace filter with it. The Timeline drops every event whose namespace is not in that filter, and a cluster-scoped event has none, so the destination could not contain the change that was clicked and read as "nothing happened". A cluster-scoped subject now widens to all namespaces, moving scope and URL together the way the namespaced branch already does. Both found by Codex in the merge sign-off review.
nadaverell
force-pushed
the
feat/findings-adapters
branch
from
September 9, 2026 13:43
79e0941 to
7d8db5b
Compare
… it is The producer reports five failures, not four: values, values diff, manifest diff, notes diff and resource diff. The values diff was dropped, so a withheld values comparison still read as a complete card, and the manifest diff was labelled "Helm values diff", pointing a reader at the wrong comparison. Found by Cursor Bugbot.
A SubjectAccessReview whose authorizer returned partial data establishes neither answer. The card titled it "<subject> cannot <verb> <resource>" and summarized it as "No RBAC rule allows it", which is a denial. The evaluation error reached the coverage strip, but the card is what an operator acts on, and it was sending them after a grant that may already exist. An unresolved check now says so in the title and the summary, and is no longer counted as denied for tone or tier. The coverage limitation stays. Found by Codex in the merge sign-off review.
Contributor
Author
|
Triaged every open comment against the rebased branch. Two were real and are fixed; the rest are stale, false on current code, or decisions this PR documents. Fixed
Not fixed — false on the current code
Not fixed — documented decisions in the PR description
Deferred, tracked for the next round
|
Two corrections to the fixes above. The cluster-scoped Timeline handoff set local namespace state and mutated the server in the same turn. clearAllNamespaces exists to avoid exactly that: it moves local state only once the server confirms, so queries cannot refetch under the empty key while the server still returns the previous pick, and it refuses to widen a namespace-locked session at all. The handoff now goes through it. An access check was treated as unresolved whenever an evaluation error was present. Kubernetes returns that error alongside a real verdict too — one webhook failing while another allows — so a decided allow or deny was being rewritten as "could not check", losing the answer and putting an allow in the warning list. Unresolved now means the authorizer decided nothing. Both found by Cursor Bugbot on the previous push.
Opening the Timeline for a cluster-scoped change keeps the current namespace filter, which can hide the very change that was clicked: the Timeline drops every event whose namespace is outside the filter, and a cluster-scoped event has none. Widening instead is the right answer and this is the wrong place to do it. Scope lives in three coupled places — local state, the URL, and the server pick — each with its own ordering rule: flipping state before the server confirms caches scoped data under the cluster-wide key, and writing the URL before state follows sets the two sync effects against each other. Two attempts here hit one hazard each. The behaviour returns to what it was, and the gap is recorded for a change made against that machinery rather than beside it.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort 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 e412428. Configure here.
nadaverell
added a commit
that referenced
this pull request
Sep 9, 2026
The pane heads two lines with these counts — "results about other resources" and "broader metric results" — and a withheld chart was incrementing both, so one withheld result was announced twice and the two numbers stopped summing to what was actually held back. The counters are now disjoint: a metrics group goes to the metrics count and everything else to the broader one, which is what each line already claims. Introduced resolving this branch's conflict with #1681, where the two counters first met. Found by Cursor Bugbot.
This was referenced Sep 9, 2026
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
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.
Summary
Findings dropped four results the agent already fetches: workload logs (read four times in the saved cloud-provisioner run, never shown), Prometheus alert rules, Helm release state, and ServiceAccount permissions. Each now becomes Radar-authored evidence built strictly from the producer's documented payload. Collapsed events cards lead with what happened instead of a group count, changes cards open the Timeline for the resource they are about, and a citation can promote any single-subject card about another resource.
Track A of the Findings next layer; frontend only.
What changed
get_workload_logsreuses thelogskind: one observation per pod/container from one source, so a multi-container read stays one citable result.logsErroris a limitation, a zero-pod read is a receipt scoped to the workload,narrowHintis a coverage note. Relevance comes from theworkloadthe producer names (every kind it resolves, Rollouts included); rows relate to a target workload the way diagnose relates to the pods it resolved.get_prometheus_rulesadds thealertskind (rule, instances, annotations). An instance names the target only through an exact workload label (deployment=,statefulset=,workload=…) or apodlabel naming a pod a producer already tied to the target in this projection; rule-level labels reach only producer-related. Re-reads of the same rule advance the card, so a rule that goes inactive shows as resolved. Rules are keyed by expression plus static labels; identical rows within one response stay scoped to that read. A receipt is emitted only for an unfilteredstate=firing|pendingquery that is complete, fully evaluated, and either empty or with every instance placed in another namespace. Any evaluation health other than a reportedokis a limitation row.get_helm_releaseadds thehelmkind, adverse when status is notdeployed, a health issue is set, or the last operation failed or is stuck. Storage namespace is part of the identity; the resource link is withheld when it differs because the ref cannot carry it. An owned resource without an API version establishes nothing; owned rows keep the producer's summary and message. Every comparison the producer could not finish becomes a coverage limitation — values, values diff, notes diff and resource diff — because three of those are role denials, and a release card that quietly omits a withheld diff reads as a complete one.get_subject_permissionsadds thepermissionskind: access checks render the verdict and attributes, subject reads render bindings and effective-rule counts. Target relevance requires the newest same-turn resource observation of the target to name the ServiceAccount (fromresourceContext.uses.serviceAccountor a PodSpec on a kind that carries one); any other subject is broader context. No blast-radius inference.helm,alertsandpermissionsjoinresource,logsandcrashas single-subject kinds a citation can promote into the main list. The pane states how many broader results of any kind it withheld and that they appear when the assessment cites them.get_eventsresult (serialized asnull) is an empty result, not a malformed one; the same forget_changes.get_changesreads of one resource are revisions of one card whose title names the window.App.tsxmoves namespace state and the server pick together with the URL, as the Timeline's own namespace prompt does, so the URL write/read effects cannot alternate; a cluster-scoped subject keeps the current scope. That is a known gap rather than a design: the Timeline drops every event whose namespace is outside the filter and a cluster-scoped event has none, so the destination can be a view the cited change cannot appear in. Widening means moving local state, the URL and the server pick together, each with its own ordering rule, and belongs in a change made against that machinery rather than this one. The relationships card already opened its root via the existing subject ref, now pinned by a test.groupKeybuilder and alatestRelevanceaccessor on the projection builder. Shared-file edits are new cases and entries.Testing
internal/mcp/tools.gogains theaccessDeniedmarker andtools_filter_test.gocovers it, so the Go suites are part of this PR's verification rather than a formality.New coverage per adapter: success, error, truncated, empty, relevance, and malformed payloads. The decisive case — one workload-logs result with several containers staying one source with several observations, so the assessment's source link still resolves — is
keeps one multi-container result as one source with several observations. The cited-broader cards, merged change reads, null results and single-row coverage groups each have a regression test, as do the four Helm comparison failures and the cluster-scoped Timeline scope.Visually checked in light, dark and at 480 px against a saved run, and against a copy of that run with alert-rule, Helm, permissions and changes results injected into a fixture copy of the run store — a second Radar instance launched against the copy, leaving the real store untouched. Workload-logs cards, the events lead, and the new alert, Helm, access-check and permissions bodies render in both themes and at narrow width, and the Timeline button navigated to the expected scoped view.
web/src/api/client.authRedirect.test.tsis flaky in this repo — a timing-sensitive 401 single-flight test, unrelated to this PR. It passes on rerun.Scope and tradeoffs
get_prometheus_rulesbeforediagnosetherefore yields context, never attribution.ok/err/unknown) is Prometheus's own enum about evaluation, not a resource health, so it is shown as reported rather than mapped through the resource HealthLevel vocabulary.scopeNamespaces; that parameter only acts alongsideappinTimelineView, so the link uses the app-widenamespacesparameter.Note
Medium Risk
Touches investigation evidence projection and MCP response shape (
accessDenied), which affects how operators interpret empty events and cited evidence; changes are heavily tested but span UI navigation and RBAC-sensitive read paths.Overview
Investigation Findings now turns four MCP tool results the agent already calls—
get_workload_logs,get_prometheus_rules,get_helm_release, andget_subject_permissions—into first-class evidence cards with dedicated bodies, relevance rules, and citation support alongside existing resource/log/crash types.UX and navigation: Events summaries lead with the newest warning text; changes cards expose a Timeline deep link (namespace scope kept in sync with URL in
App.tsx); withheld “broader” results are counted and can be promoted when the assessment cites them; coverage limitations dedupe when a single row is the whole message.API honesty:
get_eventssetsaccessDenied: truewhen the requested namespace is outside the caller’s allowed set, so an empty list is not mistaken for “no events” (documented indocs/mcp.md, tested in Go).Evidence semantics (frontend projection): Workload logs split per pod/container but stay one citable source; alert rules match the target only via strict label/pod linkage; Helm releases surface operations, health, and comparison gaps as limitations; permissions distinguish unevaluated vs decided access checks and bind to the workload’s ServiceAccount from same-turn resource reads. Startup blockers can merge across pods; denied events read as coverage errors, not empty receipts.
Reviewed by Cursor Bugbot for commit e412428. Bugbot is set up for automated code reviews on this repo. Configure here.