SDCICD-1735: update krknai prompt and markdown-to-HTML conversion#3128
SDCICD-1735: update krknai prompt and markdown-to-HTML conversion#3128minlei98 wants to merge 1 commit intoopenshift:mainfrom
Conversation
|
There are test jobs defined for this repository which are not configured to run automatically. Comment |
openshift-ci-robot
added
the
jira/valid-reference
|
@minlei98: This pull request references SDCICD-1735 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 sub-task to target the "4.22.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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: minlei98 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 |
|
krknai-22report.html |
minlei98
force-pushed
the
SDCICD-1735
branch
2 times, most recently
from
22b5db7 to
f5cd52d
Compare
pkg/krknai/analysisengine/engine.go
Outdated
| func markdownToHTML(content string) string { | ||
| htmlTemplate, err := krknPrompts.ReadFile(htmlTemplatePath) | ||
| if err != nil { | ||
| return content |
There was a problem hiding this comment.
should this return or log an error?
minlei98
force-pushed
the
SDCICD-1735
branch
from
f5cd52d to
a66b16a
Compare
| ## Executive Summary (2-3 sentences) | ||
| ## Cluster Under Test (ID, name, version, type, provider, region, hypershift yes/no) | ||
| ## Test Configuration (GA params, scenarios, health check targets) | ||
| ## Run Statistics (table: totals, generations, fitness scores, types) |
There was a problem hiding this comment.
The sample html here doesn't show fitness function used. Could you add that?
| ## Test Configuration (GA params, scenarios, health check targets) | ||
| ## Run Statistics (table: totals, generations, fitness scores, types) | ||
| ## Genetic Algorithm Evolution (fitness trends, convergence, most disruptive generation) | ||
| ## Top Vulnerabilities (top 3-5 by fitness: target, impact, severity [Critical/High/Medium/Low], why it matters) |
There was a problem hiding this comment.
Is there a way to include information about type of node (eg infra) in case of "Node CPU Hog" vulnerability?
|
The generated HTML file is really nice. Super easy to read, straight to the point. Almost doesn't even read like it's generated by AI :D The only thing I could potentially ask to add would be that somewhere in that file there's a link, or a reference to how I would figure out where the must-gather is for that run. Specifically - in the generated HTML document it references a node that caused the highest fitness score. However, without knowing any more details of that node I don't know what to do with that information. I assume that it was an infra node, but without being able to know what workloads were on it I can't really do much with the information this report generated. That's my only "problem" with this - and I use the term "problem" very loosely. This is amazing! |
Thanks for the suggestion! Created card https://issues.redhat.com/browse/SDCICD-1766 |
|
|
krknai-49report.html |
|
Can we increase the font size for this highlighted code style to normal size? |
|
krknai-44report.html |
WalkthroughThis pull request adds cluster metadata support and HTML report generation capabilities to the KrknAI analysis engine. It introduces ClusterInfo data structures across the aggregator and engine packages, refactors prompt loading through a central template registry, adds HTML output formatting with markdown-to-HTML conversion, and updates the prompt template with a Markdown-focused report structure. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Engine
participant Aggregator
participant PromptStore
participant MarkdownRenderer
participant HTMLTemplate
Client->>Engine: WithClusterInfo(info)
Engine->>Aggregator: WithClusterInfo(info)
Aggregator->>Aggregator: Store clusterInfo
Client->>Engine: Run(ctx, config)
Engine->>Aggregator: Collect()
Aggregator->>Aggregator: Initialize KrknAIData with ClusterInfo
Aggregator-->>Engine: Return KrknAIData with cluster metadata
Engine->>PromptStore: LoadTemplates
PromptStore-->>Engine: Return prompt + config
Engine->>Engine: RenderPrompt(variables)
Note over Engine: variables include ClusterInfo, Summary, TopScenarios
Engine-->>Engine: Get rendered prompt content
alt ReportFormat == "html"
Engine->>MarkdownRenderer: Convert markdown content to HTML
MarkdownRenderer-->>Engine: Return HTML string
Engine->>HTMLTemplate: Render(report.html, htmlContent)
HTMLTemplate-->>Engine: Return final HTML report
else ReportFormat == "json" or "markdown"
Engine-->>Engine: Use content as-is
end
Engine-->>Client: Return result with formatted content
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@minlei98: This pull request references SDCICD-1735 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 sub-task to target the "4.22.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. |
![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
♻️ Duplicate comments (1)
pkg/krknai/analysisengine/engine.go (1)
254-260:⚠️ Potential issue | 🔴 CriticalSanitize rendered markdown before casting to
template.HTML.Line 259 marks renderer output as trusted HTML. With LLM/log-derived markdown, this enables HTML/script injection in the generated report.
🔒 Suggested hardening
import ( "bytes" "context" "embed" "fmt" "html/template" "io/fs" "os" "path/filepath" "time" "github.com/gomarkdown/markdown" mdhtml "github.com/gomarkdown/markdown/html" "github.com/gomarkdown/markdown/parser" + "github.com/microcosm-cc/bluemonday" "github.com/openshift/osde2e/internal/analysisengine" "github.com/openshift/osde2e/internal/llm" "github.com/openshift/osde2e/internal/llm/tools" "github.com/openshift/osde2e/internal/prompts" @@ - body := markdown.ToHTML([]byte(content), p, renderer) + unsafeBody := markdown.ToHTML([]byte(content), p, renderer) + safeBody := bluemonday.UGCPolicy().SanitizeBytes(unsafeBody) @@ - if err := tmpl.Execute(&buf, struct{ Body template.HTML }{Body: template.HTML(body)}); err != nil { + if err := tmpl.Execute(&buf, struct{ Body template.HTML }{Body: template.HTML(string(safeBody))}); err != nil { return "", fmt.Errorf("failed to execute HTML template: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/krknai/analysisengine/engine.go` around lines 254 - 260, The rendered markdown byte slice stored in variable body (from markdown.ToHTML called alongside parser.NewWithExtensions and mdhtml.NewRenderer) is currently cast directly to template.HTML before tmpl.Execute, allowing untrusted HTML/script injection; fix this by sanitizing the renderer output before casting: add a trusted sanitizer (e.g., bluemonday.StrictPolicy or a policy that allows safe markdown tags) and run it on body (or string(body)), then cast the sanitized result to template.HTML when building the struct passed to tmpl.Execute; ensure imports are added and replace the direct template.HTML(body) usage with template.HTML(sanitizedBody).
🧹 Nitpick comments (3)
internal/prompts/prompts_test.go (1)
10-18: Add a focused test forRegisterTemplatesoverwrite behavior.Good assertion for default availability. Since this PR adds template overlay semantics, add one test that registers a same-ID template and verifies the default gets replaced.
🧪 Proposed test addition
import ( "testing" + "testing/fstest" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +func TestRegisterTemplates_OverridesExistingTemplate(t *testing.T) { + store, err := NewPromptStore(DefaultTemplates()) + require.NoError(t, err) + + overrideFS := fstest.MapFS{ + "default.yaml": &fstest.MapFile{ + Data: []byte("system_prompt: override-system\nuser_prompt: override-user\n"), + }, + } + + require.NoError(t, store.RegisterTemplates(overrideFS)) + + tmpl, err := store.GetTemplate("default") + require.NoError(t, err) + assert.Equal(t, "override-system", tmpl.SystemPrompt) + assert.Equal(t, "override-user", tmpl.UserPrompt) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/prompts/prompts_test.go` around lines 10 - 18, Add a focused unit test that verifies RegisterTemplates overwrites existing templates: create a PromptStore via NewPromptStore(DefaultTemplates()), then call store.RegisterTemplates(...) with a template object using the same ID (e.g., "default") but different content, retrieve it via store.GetTemplate("default") and assert the template's content now matches the newly registered one (and not the original). Reference NewPromptStore, DefaultTemplates, RegisterTemplates, and GetTemplate in the test to locate the code paths to exercise.pkg/krknai/analysisengine/prompts/krknai.yaml (1)
17-17: Reduce hallucination risk for expected status codes.You require expected status codes in output; make the artifact-read instruction explicit for that field so the model doesn’t infer missing values.
✍️ Suggested prompt tweak
- ## Test Configuration (GA params; list all enabled chaos scenarios; health check targets with name, endpoint URL, and expected status code) + ## Test Configuration (GA params; list all enabled chaos scenarios; health check targets with name, endpoint URL, and expected status code; fetch from artifacts when not present in prompt variables) @@ - Use read_file on relevant artifacts. Generate the full markdown report per system prompt structure. + Use read_file on relevant artifacts (especially krkn-ai.yaml for health-check expected status codes). Generate the full markdown report per system prompt structure.Also applies to: 63-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/krknai/analysisengine/prompts/krknai.yaml` at line 17, The prompt's "## Test Configuration" section currently asks for expected status codes but doesn't force the model to read the artifact; update the krknai.yaml prompt to add an explicit artifact-read requirement for the expected status code field (e.g., require "expected status code" or "expected_status_code" be populated only from the provided artifact and never guessed), and mirror this change for the other occurrence around line 63 so the model must extract that value from the artifact rather than infer or fabricate it.pkg/krknai/aggregator/aggregator.go (1)
103-107: Defensively copyClusterInfoto avoid pointer aliasing side effects.
WithClusterInfoandCollectcurrently share the same pointer instance. A caller mutatinginfolater can unintentionally change previously collected output.♻️ Proposed defensive-copy update
func (a *KrknAIAggregator) WithClusterInfo(info *ClusterInfo) *KrknAIAggregator { - a.clusterInfo = info + if info == nil { + a.clusterInfo = nil + return a + } + copied := *info + a.clusterInfo = &copied return a } func (a *KrknAIAggregator) Collect(ctx context.Context, resultsDir string) (*KrknAIData, error) { @@ - data := &KrknAIData{ - ClusterInfo: a.clusterInfo, - } + data := &KrknAIData{} + if a.clusterInfo != nil { + copied := *a.clusterInfo + data.ClusterInfo = &copied + }Also applies to: 117-119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/krknai/aggregator/aggregator.go` around lines 103 - 107, WithClusterInfo currently stores the provided *ClusterInfo pointer directly causing aliasing; change it to store a defensive copy instead (e.g., implement or call a ClusterInfo.Clone/Copy and assign that to KrknAIAggregator.clusterInfo) so later mutations by the caller don't change stored data; also ensure any use in Collect reads from that copied instance (or makes another copy before including in output) to avoid sharing the original pointer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/krknai/analysisengine/engine.go`:
- Around line 156-162: Validate ReportFormat explicitly before conversion: if
e.config.ReportFormat is "html" call markdownToHTML(content) as you already do,
but add a guard such as if e.config.ReportFormat != "" && e.config.ReportFormat
!= "html" { return nil, fmt.Errorf("unsupported ReportFormat: %q",
e.config.ReportFormat) } so typos or invalid values on e.config.ReportFormat are
rejected instead of silently falling back to raw; keep the markdownToHTML call
and its error handling unchanged.
---
Duplicate comments:
In `@pkg/krknai/analysisengine/engine.go`:
- Around line 254-260: The rendered markdown byte slice stored in variable body
(from markdown.ToHTML called alongside parser.NewWithExtensions and
mdhtml.NewRenderer) is currently cast directly to template.HTML before
tmpl.Execute, allowing untrusted HTML/script injection; fix this by sanitizing
the renderer output before casting: add a trusted sanitizer (e.g.,
bluemonday.StrictPolicy or a policy that allows safe markdown tags) and run it
on body (or string(body)), then cast the sanitized result to template.HTML when
building the struct passed to tmpl.Execute; ensure imports are added and replace
the direct template.HTML(body) usage with template.HTML(sanitizedBody).
---
Nitpick comments:
In `@internal/prompts/prompts_test.go`:
- Around line 10-18: Add a focused unit test that verifies RegisterTemplates
overwrites existing templates: create a PromptStore via
NewPromptStore(DefaultTemplates()), then call store.RegisterTemplates(...) with
a template object using the same ID (e.g., "default") but different content,
retrieve it via store.GetTemplate("default") and assert the template's content
now matches the newly registered one (and not the original). Reference
NewPromptStore, DefaultTemplates, RegisterTemplates, and GetTemplate in the test
to locate the code paths to exercise.
In `@pkg/krknai/aggregator/aggregator.go`:
- Around line 103-107: WithClusterInfo currently stores the provided
*ClusterInfo pointer directly causing aliasing; change it to store a defensive
copy instead (e.g., implement or call a ClusterInfo.Clone/Copy and assign that
to KrknAIAggregator.clusterInfo) so later mutations by the caller don't change
stored data; also ensure any use in Collect reads from that copied instance (or
makes another copy before including in output) to avoid sharing the original
pointer.
In `@pkg/krknai/analysisengine/prompts/krknai.yaml`:
- Line 17: The prompt's "## Test Configuration" section currently asks for
expected status codes but doesn't force the model to read the artifact; update
the krknai.yaml prompt to add an explicit artifact-read requirement for the
expected status code field (e.g., require "expected status code" or
"expected_status_code" be populated only from the provided artifact and never
guessed), and mirror this change for the other occurrence around line 63 so the
model must extract that value from the artifact rather than infer or fabricate
it.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
go.modinternal/analysisengine/engine.gointernal/prompts/prompts.gointernal/prompts/prompts_test.gopkg/krknai/aggregator/aggregator.gopkg/krknai/analysisengine/engine.gopkg/krknai/analysisengine/engine_test.gopkg/krknai/analysisengine/prompts/krknai.yamlpkg/krknai/analysisengine/prompts/report.html
| if e.config.ReportFormat == "html" { | ||
| var err error | ||
| content, err = markdownToHTML(content) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to convert markdown to HTML: %w", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Reject unsupported ReportFormat values explicitly.
Line 156 only handles "html". Typos or invalid values silently fall back to raw content, which is hard to diagnose.
💡 Suggested guard
content := result.Content
- if e.config.ReportFormat == "html" {
- var err error
- content, err = markdownToHTML(content)
- if err != nil {
- return nil, fmt.Errorf("failed to convert markdown to HTML: %w", err)
- }
- }
+ switch e.config.ReportFormat {
+ case "", "json", "markdown":
+ // keep markdown content as-is
+ case "html":
+ var err error
+ content, err = markdownToHTML(content)
+ if err != nil {
+ return nil, fmt.Errorf("failed to convert markdown to HTML: %w", err)
+ }
+ default:
+ return nil, fmt.Errorf("unsupported report format %q", e.config.ReportFormat)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if e.config.ReportFormat == "html" { | |
| var err error | |
| content, err = markdownToHTML(content) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to convert markdown to HTML: %w", err) | |
| } | |
| } | |
| switch e.config.ReportFormat { | |
| case "", "json", "markdown": | |
| // keep markdown content as-is | |
| case "html": | |
| var err error | |
| content, err = markdownToHTML(content) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to convert markdown to HTML: %w", err) | |
| } | |
| default: | |
| return nil, fmt.Errorf("unsupported report format %q", e.config.ReportFormat) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/krknai/analysisengine/engine.go` around lines 156 - 162, Validate
ReportFormat explicitly before conversion: if e.config.ReportFormat is "html"
call markdownToHTML(content) as you already do, but add a guard such as if
e.config.ReportFormat != "" && e.config.ReportFormat != "html" { return nil,
fmt.Errorf("unsupported ReportFormat: %q", e.config.ReportFormat) } so typos or
invalid values on e.config.ReportFormat are rejected instead of silently falling
back to raw; keep the markdownToHTML call and its error handling unchanged.
|
@minlei98: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
5 participants
Summary by CodeRabbit
Release Notes
New Features
Tests