Skip to content

Prepare investigation package compatibility and stream reliability - #1672

Merged
nadaverell merged 2 commits into
mainfrom
fix/cloud-investigation-release-prep
Sep 7, 2026
Merged

nadaverell merged 2 commits into
mainfrom
fix/cloud-investigation-release-prep

Conversation

@nadaverell

@nadaverell nadaverell commented Sep 7, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Prepare the investigation UI for the next patch packages and reduce reconnect pressure during transient transcript-storage failures.

Changes

  • Require @skyhook-io/k8s-ui >=1.14.2 in radar-app and its lockfile. Findings uses the defaultConditionTone export, which is absent from published 1.14.1.
  • Send a 10-second EventSource retry delay for retryable history hydration failures; preserve terminal corrupt-history behavior.
  • Cover tool-step merging, result/provenance metadata, replay/live animation, and API-group filtering before timeline limits in the shared store conformance suite.

Validation

  • make build (includes frontend type-check and embedded Go binary)
  • 225 focused frontend tests across four investigation suites
  • go test ./internal/server ./internal/timeline
  • From pkg: go test ./timeline/...
  • git diff --check
  • Visual test: skipped (no rendered UI change).
  • PostgreSQL-backed integration cases were not exercised without a test DSN.

Release ordering

Publish k8s-ui 1.14.2 from code containing the export before radar-app 1.13.2. Both are patch releases. This PR does not publish packages or enable hosted Findings; Hub protocol alignment and its required target-group migration remain separate integration work.


Note

Low Risk
Mostly dependency alignment, SSE reconnect tuning, and test coverage; no auth or data-mutation logic changes in production UI code beyond the stream retry line.

Overview
Prepares the investigation stack for @skyhook-io/k8s-ui >=1.14.2 (lockfile and web/package.json peer) so Findings can rely on exports such as defaultConditionTone that are missing from 1.14.1.

For retryable AI run transcript hydration failures on the diagnose SSE stream, the server now emits retry: 10000 before the existing history_unavailable frame so EventSource backs off instead of hammering the store on a few-second default reconnect. Corrupt / non-retryable history still ends with the explicit closed frame; only the transient path changes.

Adds shared timeline store conformance coverage that APIGroups is applied before Limit and rows with empty APIVersion stay visible under any group filter. Adds frontend unit tests for upsertTool (append, in-place merge preserving running input, provenance fields, animation flags).

Reviewed by Cursor Bugbot for commit 993a48f. Bugbot is set up for automated code reviews on this repo. Configure here.

@nadaverell
nadaverell requested a review from hisco as a code owner September 7, 2026 19:42
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Back off investigation retries and require k8s-ui 1.14.2

🐞 Bug fix 🧪 Tests ⚙️ Configuration changes 🕐 20-40 Minutes

Grey Divider

AI Description

• Back off retryable transcript hydration reconnects to reduce storage pressure during outages.
• Require k8s-ui 1.14.2 for the Findings condition-tone export.
• Expand investigation merge and timeline API-group conformance coverage.
Diagram

graph TD
  Client["EventSource Client"] -->|Connects| Handler["Run Stream"] -->|Hydrates| Store["Transcript Store"]
  Store -->|Transient failure| Handler
  Handler -->|Emits retry| Retry["10s Retry Frame"] -->|Delays reconnect| Client
  App["Radar App"] -->|Requires| UI["k8s-ui 1.14.2"]
Loading
High-Level Assessment

The targeted approach is appropriate: a standard SSE retry directive avoids custom client reconnection logic, the minimum peer version accurately captures the required export, and shared conformance tests protect every timeline-store implementation. Custom exponential backoff and an exact dependency pin were considered but would add frontend complexity or unnecessarily restrict compatible future patches.

Files changed (6) +143 / -2

Bug fix (1) +9 / -0
ai_diagnose.goDelay retries after transient transcript hydration failures +9/-0

Delay retries after transient transcript hydration failures

• Adds a ten-second SSE retry directive when investigation history cannot be hydrated for a retryable reason. Corrupt history remains terminal and continues to emit the existing closed frame.

internal/server/ai_diagnose.go

Tests (3) +132 / -0
ai_diagnose_stream_test.goVerify retryable streams advertise the reconnect delay +1/-0

Verify retryable streams advertise the reconnect delay

• Extends the transient hydration failure test to require the 'retry: 10000' SSE directive alongside the retryable error event.

internal/server/ai_diagnose_stream_test.go

conformance.goCover API-group filtering before timeline limits +38/-0

Cover API-group filtering before timeline limits

• Adds shared store conformance cases proving API-group filters run before result limits. The cases also preserve unknown API versions and distinguish core-group resources.

pkg/timeline/storetest/conformance.go

parts.test.tsxExpand investigation tool-step merge coverage +93/-0

Expand investigation tool-step merge coverage

• Adds tests for tool-call insertion and completion merging, including preserved inputs, replaced result and provenance fields, immutability, idempotency, and replay/live animation behavior.

web/src/components/diagnose/parts.test.tsx

Other (2) +2 / -2
package-lock.jsonLock the updated k8s-ui peer requirement +1/-1

Lock the updated k8s-ui peer requirement

• Updates the Radar App lockfile metadata to require '@skyhook-io/k8s-ui' version 1.14.2 or newer.

package-lock.json

package.jsonRequire the k8s-ui Findings export patch +1/-1

Require the k8s-ui Findings export patch

• Raises Radar App's minimum '@skyhook-io/k8s-ui' peer version to 1.14.2, ensuring consumers receive the required condition-tone export.

web/package.json

@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (1) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. A redundant comment burdens maintainers 📘 Rule violation ⚙ Maintainability
Description
historyUnavailableRetryMillis is documented as the EventSource reconnect delay for retryable
hydration failures, which its name and immediate use already state. Any future retry-policy change
must update both the executable value and this duplicate prose, creating another source of drift.
Code

internal/server/ai_diagnose.go[R516-517]

+// historyUnavailableRetryMillis is the EventSource reconnect delay sent with a
+// retryable hydration failure.
Relevance

●●● Strong

Recent precedent accepts refining redundant comments; this directly matches the active
comment-quality rule.

PR-#1636

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Compliance rule 3036542 prohibits added comments that only translate obvious code behavior into
prose. The added comment repeats the meaning evident from the descriptive constant name and its use
in the EventSource retry field.

Rule 3036542: Avoid explanatory comments that restate obvious code behavior
internal/server/ai_diagnose.go[516-518]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Remove the comment that merely restates the purpose already conveyed by `historyUnavailableRetryMillis` and its call site.

## Issue Context
The nearby retry-handling comment already records the non-obvious rationale for the ten-second delay, so the constant-level comment adds no distinct intent or constraint.

## Fix Focus Areas
- internal/server/ai_diagnose.go[516-517]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources
✅ Compliance rules (platform): 41 rules
✅ Cross-repo context — repo relationships
  Explored: repo: skyhook-dev/radar-hub (sha: 34e22e7a)
  Explored: repo: skyhook-dev/radar-hub-web (sha: a79fb2e8)
Review mode: ⚖️ Balanced: This PR combines a runtime SSE retry behavior change, package compatibility constraints, shared timeline conformance semantics, and substantial frontend behavior tests, so it carries multiple meaningful but reviewable risks without warranting redundant extended passes.

Grey Divider

Tip of the day
💡 Did you know, you can copy the agent prompt from any finding and feed it to your IDE agent

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment on lines +516 to +517
// historyUnavailableRetryMillis is the EventSource reconnect delay sent with a
// retryable hydration failure.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

1. A redundant comment burdens maintainers 📘 Rule violation ⚙ Maintainability

historyUnavailableRetryMillis is documented as the EventSource reconnect delay for retryable
hydration failures, which its name and immediate use already state. Any future retry-policy change
must update both the executable value and this duplicate prose, creating another source of drift.
Agent Prompt
## Issue description
Remove the comment that merely restates the purpose already conveyed by `historyUnavailableRetryMillis` and its call site.

## Issue Context
The nearby retry-handling comment already records the non-obvious rationale for the ten-second delay, so the constant-level comment adds no distinct intent or constraint.

## Fix Focus Areas
- internal/server/ai_diagnose.go[516-517]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

@nadaverell
nadaverell merged commit fa326f3 into main Sep 7, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant