OLS-3667: Render escalation context in run detail view - #194
Conversation
|
@onmete: This pull request references OLS-3667 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe console watches escalation results, maps them into ChangesAgentic run escalation support
Sequence Diagram(s)sequenceDiagram
participant KubernetesAPI
participant useAgenticRun
participant RunDetailPage
participant EscalationSummary
KubernetesAPI->>useAgenticRun: watch EscalationResultK8s resources
useAgenticRun->>useAgenticRun: mapEscalation and mapTimeline
useAgenticRun->>RunDetailPage: provide AgenticRunView escalation data
RunDetailPage->>EscalationSummary: render terminal escalation details
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/runs/RunDetailPage.tsx (1)
239-245: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd stable selectors for the new escalation states.
The new escalation UI has no
data-testattributes. End-to-end tests cannot select these states without relying on text or layout.
src/components/runs/RunDetailPage.tsx#L239-L245: add a stable selector around the in-progress escalation view.src/components/runs/detail/EscalationSummary.tsx#L17-L68: add stable selectors for the escalation summary and its relevant detail sections.As per coding guidelines, use
data-testattributes for end-to-end test selectors.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/runs/RunDetailPage.tsx` around lines 239 - 245, Add stable data-test selectors for the escalation UI: wrap the in-progress escalation view rendered by RunDetailPage around StageInProgress, and add selectors to the escalation summary and its relevant detail sections in EscalationSummary. Update both specified files and use consistent, state-specific selector names without relying on text or layout.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/hooks/useAgenticRun.ts`:
- Around line 480-485: Update the resultsError handling in useAgenticRun so an
escalationError is not reported as an analysis-results failure. Prefer returning
resource-specific error state for escalation failures; otherwise update
RunDetailPage’s alert to use the resource-neutral “Unable to load run results”
message while preserving the existing precedence for other errors.
---
Nitpick comments:
In `@src/components/runs/RunDetailPage.tsx`:
- Around line 239-245: Add stable data-test selectors for the escalation UI:
wrap the in-progress escalation view rendered by RunDetailPage around
StageInProgress, and add selectors to the escalation summary and its relevant
detail sections in EscalationSummary. Update both specified files and use
consistent, state-specific selector names without relying on text or layout.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ad11ce6f-c04f-4cc8-8adb-23b022831fc6
📒 Files selected for processing (6)
src/components/runs/RunDetailPage.tsxsrc/components/runs/detail/EscalationSummary.tsxsrc/hooks/useAgenticRun.test.tssrc/hooks/useAgenticRun.tssrc/models/agenticrun-views.tssrc/models/agenticrun.ts
|
image from console with changes from this PR |
onmete
left a comment
onmete
left a comment
There was a problem hiding this comment.
Adversarial review — structurally sound, follows established patterns. Three items worth addressing before merge.
| {escalation.failureReason && ( | ||
| <FlexItem> | ||
| <Title headingLevel="h5">{t('Failure reason')}</Title> | ||
| <MarkdownContent text={escalation.failureReason} /> |
There was a problem hiding this comment.
failureReason rendered as markdown — potential XSS / inconsistency
failureReason is passed through MarkdownContent here, but the other summary components (AnalysisSummary, ExecutionSummary) display their failure reasons as plain text or inside an Alert. Two concerns:
- If
MarkdownContentdoesn't sanitize HTML, this is an injection vector —failureReasoncomes from a CRD status field that could contain LLM-generated or sandbox-echoed content. - Even if sanitized, it's inconsistent with how failure reasons are rendered elsewhere.
Consider using a plain-text Alert with variant="danger" to match the other summaries, or verify that MarkdownContent sanitizes.
There was a problem hiding this comment.
Fixed in 374d5c6. Dropped the in-card Failure reason markdown block — failureReason is a system-error string and is already shown via the page-level danger Alert. Also removed the invented Details heading so summary/content render as unmarked freeform markdown (DOMPurify still applies via MarkdownContent).
| execution?: ExecutionResultK8s, | ||
| verification?: VerificationResultK8s, | ||
| approval?: AgenticRunApprovalK8s, | ||
| escalation?: EscalationResultK8s, |
There was a problem hiding this comment.
nit: positional parameter explosion
mapTimeline now takes 8 positional parameters, most of them optional K8s resources of similar types. The test already shows the cost:
mapTimeline(run, 'Escalated', t, undefined, undefined, undefined, undefined, escalation)Seven undefineds to reach the escalation slot. One swap and you silently feed the wrong CR to the wrong stage. Not blocking, but a single options object would eliminate the risk. Same applies to mapToAgenticRunView (7 params, line 312).
There was a problem hiding this comment.
Skipping — out of scope for this PR. Valid maintainability nit; mapTimeline / mapToAgenticRunView options-object refactor belongs in a follow-up.
| )?.lastTransitionTime, | ||
| verificationSandbox: mapSandbox(run.status?.steps?.verification?.sandbox), | ||
| escalationStartedAt: (escalation?.status?.conditions ?? []).find((c) => c.type === 'Started') | ||
| ?.lastTransitionTime, |
There was a problem hiding this comment.
nit: duplicated escalationStartedAt extraction
escalationStartedAt is computed here on the top-level view and identically inside mapEscalation (line 153). Both extract Started condition timestamp from the same escalation object. If someone changes one and forgets the other, they'll drift. The same duplication exists for execution/verification — so it's a pre-existing pattern — but worth noting.
There was a problem hiding this comment.
Skipping — pre-existing pattern not introduced by this PR. The same *StartedAt extraction duplication already exists for execution and verification.
|
|
||
| {resultsError && ( | ||
| <Alert isInline title={t('Unable to load analysis results.')} variant="warning" /> | ||
| <Alert isInline title={t('Unable to load run results.')} variant="warning" /> |
There was a problem hiding this comment.
we might need to regenerate the translations
There was a problem hiding this comment.
Fixed in cf76b54. Regenerated locales — added Escalation / Escalated / Escalation summary and replaced Unable to load analysis results. with Unable to load run results.. Also rebased onto latest main (merged with the manual-stage approval work; Escalating now shows the approval banner when needed, otherwise StageInProgress + sandbox logs).
374d5c6 to
cf76b54
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/runs/detail/EscalationSummary.tsx`:
- Around line 38-60: Update EscalationSummary to avoid rendering an empty card
when the escalation has no renderable summary, content, or escalationSandbox.
Guard the card using those fields, or include escalation.failureReason within
the card body so failure-only escalations display meaningful content.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 841090c1-5022-49d8-9dd3-eec8ce0a391a
📒 Files selected for processing (7)
locales/en/plugin__lightspeed-agentic-console-plugin.jsonsrc/components/runs/RunDetailPage.tsxsrc/components/runs/detail/EscalationSummary.tsxsrc/hooks/useAgenticRun.test.tssrc/hooks/useAgenticRun.tssrc/models/agenticrun-views.tssrc/models/agenticrun.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/components/runs/RunDetailPage.tsx
- src/models/agenticrun-views.ts
- src/models/agenticrun.ts
- src/hooks/useAgenticRun.test.ts
- src/hooks/useAgenticRun.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.ai/spec/how/k8s-data-layer.md:
- Line 37: Complete the Proposal-to-AgenticRun documentation migration: in
.ai/spec/how/k8s-data-layer.md lines 37-37, update the result-correlation
example to use useAgenticRun, filterLatest, and run.status instead of
getLatestResult and proposal.status; at lines 66-66, replace useProposal
approval references with the current useAgenticRun API; and in
.ai/spec/how/project-structure.md lines 28-30, replace PROPOSAL_NAMESPACE,
PROPOSAL_LABEL_SOURCE, and RESULT_LABEL_PROPOSAL with the current AgenticRun
symbols.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3db94287-6e12-4cac-8ebd-cc858ff80abc
📒 Files selected for processing (4)
.ai/spec/how/k8s-data-layer.md.ai/spec/how/project-structure.md.ai/spec/what/run-lifecycle.mdsrc/components/runs/detail/EscalationSummary.tsx
Fetch and display EscalationResult CR data when an AgenticRun reaches the Escalating/Escalated phase. Previously the console only showed an orange badge with no explanation. - Add EscalationResultModel, GVK, and K8s type alias - Add EscalationView and wire it into AgenticRunView - Watch EscalationResult CRs via label selector - Add mapEscalation mapper (mirrors mapVerification) - Show StageInProgress with sandbox logs during Escalating - Show EscalationSummary card (summary, content, failure reason, sandbox logs) for Escalated and other terminal phases - Add escalation events to the run timeline - Include escalation failureReason in page-level alert chain - Add unit tests for mapEscalation and timeline escalation events Co-authored-by: Cursor <cursoragent@cursor.com>
Use resource-neutral error message "Unable to load run results" instead of misleading "Unable to load analysis results" since resultsError aggregates errors from all result CRs. Co-authored-by: Cursor <cursoragent@cursor.com>
Show summary and content as unmarked markdown only. Drop the invented Details heading and in-card Failure reason section — failureReason remains on the page-level danger alert. Co-authored-by: Cursor <cursoragent@cursor.com>
Add Escalation/Escalated/Escalation summary keys and replace Unable to load analysis results. with Unable to load run results. Co-authored-by: Cursor <cursoragent@cursor.com>
Skip rendering EscalationSummary when the result has no summary, content, or sandbox — avoids an empty card on failure-only CRs. Co-authored-by: Cursor <cursoragent@cursor.com>
Update run-lifecycle, project-structure, and k8s-data-layer specs to match EscalationResult watching, EscalationSummary freeform rendering, Escalating StageInProgress/approval, and timeline events. Co-authored-by: Cursor <cursoragent@cursor.com>
Rename leftover proposal.ts, useProposal, ProposalView, etc. to their current agenticrun.ts, useAgenticRun, AgenticRunView counterparts across all spec files. Co-authored-by: Cursor <cursoragent@cursor.com>
51b21db to
5a44970
Compare
3 participants
Summary
Fixes OLS-3667 — when an AgenticRun reaches the Escalating/Escalated phase, the console now shows meaningful context instead of just an orange badge.
EscalationResultModel, GVK, and K8s type. The hook now watchesEscalationResultCRs via the same label-selector pattern used for Analysis/Execution/Verification results.StageInProgresswith streaming sandbox logs (matching Executing/Verifying behavior).EscalationSummarycard with the agent's markdown summary, detailed content, failure reason, and sandbox log viewer.failureReason.Testing
npm run lint— passesnpx vitest run— 115 tests pass (6 new:mapEscalation+ timeline escalation events)Made with Cursor