ROB-288: Relax system message requirement in chat history - #2165
Open
moshemorad wants to merge 3 commits into
Open
moshemorad wants to merge 3 commits into
moshemorad wants to merge 3 commits into
Conversation
ChatRequestBaseModel rejected any non-empty conversation_history whose first message wasn't role="system" (HTTP 422 -> robusta-runner error 5201), which broke follow-up chats after relay PR #577 stopped prepending a system message. The contract was obsolete: build_chat_messages() -> add_or_update_system_prompt() already owns position 0. - models.py: drop the role check from the mode="before" validator (now parse_raw_json_body); keep the bytes/str JSON-body parsing that lets clients omitting Content-Type still validate. - conversations.py: add_or_update_system_prompt now always installs the generated prompt at position 0 (overwrite a leading system message or insert one), using .get("role") so a role-less first message can't KeyError. Drops the old branch that silently discarded the prompt when a system message existed elsewhere; non-leading system messages (e.g. compaction's trailing marker) are left intact. - tests: flip the two "must raise" validator tests to "tolerated"; add build_chat_messages tests covering the relay stub overwrite, insert when first is user, role-less first message, and compaction-marker preservation. https://claude.ai/code/session_011N5WiSiCoDiTxhTKiwjRQT Signed-off-by: Claude <noreply@anthropic.com>
Contributor
📂 Previous Runs
|
| Status | Test case | Time | Turns | Tools | Cost | Total tokens | Input | Max input | Output | Max output | Cached | Non-cached | Reasoning | Compactions | Denied commands | Src |
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| ✅ | 09_crashpod | 28.0s | 4 | 8 | $0.2090 | 69,332 | 67,726 | 19,521 | 1,606 | 488 | 47,292 | 20,434 | 94 | — | — | src |
| ✅ | 101_loki_historical_logs_pod_deleted | 48.0s | 5 | 11 | $0.2631 | 90,563 | 87,740 | 21,142 | 2,823 | 845 | 65,886 | 21,854 | 354 | — | — | src |
| ✅ | 112_find_pvcs_by_uuid | 16.4s | 2 | 2 | $0.1571 | 33,224 | 32,273 | 17,951 | 951 | 500 | 14,319 | 17,954 | 287 | — | — | src |
| ✅ | 12_job_crashing | 32.1s | 4 | 10 | $0.2267 | 72,750 | 70,813 | 20,724 | 1,937 | 576 | 49,234 | 21,579 | 183 | — | — | src |
| ✅ | 176_network_policy_blocking_traffic_no_skills | 25.2s | 5 | 9 | $0.2085 | 86,607 | 85,211 | 19,208 | 1,396 | 385 | 65,655 | 19,556 | 154 | — | — | src |
| ✅ | 227_count_configmaps_per_namespace[0] | 14.1s | 3 | 6 | $0.1501 | 46,356 | 45,643 | 16,658 | 713 | 438 | 28,981 | 16,662 | 29 | — | — | src |
| ✅ | 243_pod_names_contain_service | 29.3s | 3 | 7 | $0.1889 | 49,725 | 48,077 | 18,068 | 1,648 | 571 | 29,343 | 18,734 | 241 | — | — | src |
| ✅ | 24_misconfigured_pvc | 28.8s | 5 | 9 | $0.2239 | 91,696 | 90,202 | 20,140 | 1,494 | 351 | 68,894 | 21,308 | 113 | — | — | src |
| ✅ | 254_elasticsearch_dr_test_log_check | 60.3s | 9 | 13 | $0.2858 | 121,965 | 118,128 | 17,743 | 3,837 | 1,040 | 99,754 | 18,374 | 268 | — | — | src |
| ✅ | 259_wrong_cluster_logs_confusion | 57.3s | 6 | 9 | $0.2564 | 85,900 | 82,421 | 16,978 | 3,479 | 1,223 | 63,820 | 18,601 | 563 | — | — | src |
| ✅ | 260_global_es_remote_cluster_logs | 53.9s | 8 | 9 | $0.2387 | 108,078 | 105,361 | 16,480 | 2,717 | 747 | 88,509 | 16,852 | 199 | — | — | src |
| ✅ | 43_current_datetime_from_prompt | 4.0s | 1 | — | $0.0112 | 14,428 | 14,306 | 14,306 | 122 | 122 | 14,303 | 3 | 78 | — | — | src |
| ✅ | 51_logs_summarize_errors | 19.3s | 3 | 2 | $0.1595 | 47,252 | 46,398 | 17,465 | 854 | 476 | 28,929 | 17,469 | 33 | — | — | src |
| ✅ | 61_exact_match_counting | 7.2s | 2 | 1 | $0.1146 | 29,148 | 28,927 | 14,641 | 221 | 152 | 14,283 | 14,644 | 34 | — | — | src |
| Total | 30.3s avg | 4.3 avg | 7.4 avg | $2.6936 | 947,024 | 923,226 | 21,142 | 23,798 | 1,223 | 679,202 | 244,024 | 2,630 | — | — |
Benchmark Comparison Details
Master baseline: latest master-* experiment (post-merge regression eval)
Status: 13 test/model combinations loaded
- master-27546008976 (created: 2026-06-15)
Benchmark baseline: latest ci-benchmark experiment on master
Status: 17 test/model combinations loaded
- ci-benchmark-27491953079 (created: 2026-06-14)
Time comparison (seconds):
| Test case | This branch | master (5h ago) | Δ vs master | benchmark (1d ago) | Δ vs benchmark |
|---|---|---|---|---|---|
| 09_crashpod (opus-4.6) 📄 | 28.0s | 25.2s | ↑11% | — | — |
| 101_loki_historical_logs_pod_deleted (opus-4.6) 📄 | 48.0s | 62.7s | ↓23% | — | — |
| 112_find_pvcs_by_uuid (opus-4.6) 📄 | 16.4s | 16.0s | ±0% | — | — |
| 12_job_crashing (opus-4.6) 📄 | 32.1s | 29.9s | ±0% | — | — |
| 176_network_policy_blocking_traffic_no_skills (opus-4.6) 📄 | 25.2s | 32.3s | ↓22% | — | — |
| 227_count_configmaps_per_namespace[0] (opus-4.6) 📄 | 14.1s | 16.3s | ↓14% | — | — |
| 243_pod_names_contain_service (opus-4.6) 📄 | 29.3s | 31.7s | ±0% | — | — |
| 24_misconfigured_pvc (opus-4.6) 📄 | 28.8s | 40.0s | ↓28% | — | — |
| 254_elasticsearch_dr_test_log_check (opus-4.6) 📄 | 60.3s | 69.1s | ↓13% | — | — |
| 259_wrong_cluster_logs_confusion (opus-4.6) 📄 | 57.3s | 74.9s | ↓24% | — | — |
| 260_global_es_remote_cluster_logs (opus-4.6) 📄 | 53.9s | 69.3s | ↓22% | — | — |
| 43_current_datetime_from_prompt (opus-4.6) 📄 | 4.0s | 4.3s | ±0% | — | — |
| 51_logs_summarize_errors (opus-4.6) 📄 | 19.3s | 21.0s | ±0% | — | — |
| 61_exact_match_counting (opus-4.6) 📄 | 7.2s | — | — | — | — |
| Total (all, n=14) | 30.3s | 37.9s | — | — | — |
| Comparable (m=13, b=0) | 32.1s | 37.9s | ↓15% | — | — |
Cost comparison:
| Test case | This branch | master (5h ago) | Δ vs master | benchmark (1d ago) | Δ vs benchmark |
|---|---|---|---|---|---|
| 09_crashpod (opus-4.6) 📄 | $0.2090 | $0.1874 | ↑12% | — | — |
| 101_loki_historical_logs_pod_deleted (opus-4.6) 📄 | $0.2631 | $0.3039 | ↓13% | — | — |
| 112_find_pvcs_by_uuid (opus-4.6) 📄 | $0.1571 | $0.1564 | ±0% | — | — |
| 12_job_crashing (opus-4.6) 📄 | $0.2267 | $0.2171 | ±0% | — | — |
| 176_network_policy_blocking_traffic_no_skills (opus-4.6) 📄 | $0.2085 | $0.2255 | ±0% | — | — |
| 227_count_configmaps_per_namespace[0] (opus-4.6) 📄 | $0.1501 | $0.1482 | ±0% | — | — |
| 243_pod_names_contain_service (opus-4.6) 📄 | $0.1889 | $0.1967 | ±0% | — | — |
| 24_misconfigured_pvc (opus-4.6) 📄 | $0.2239 | $0.2596 | ↓14% | — | — |
| 254_elasticsearch_dr_test_log_check (opus-4.6) 📄 | $0.2858 | $0.2989 | ±0% | — | — |
| 259_wrong_cluster_logs_confusion (opus-4.6) 📄 | $0.2564 | $0.3040 | ↓16% | — | — |
| 260_global_es_remote_cluster_logs (opus-4.6) 📄 | $0.2387 | $0.2910 | ↓18% | — | — |
| 43_current_datetime_from_prompt (opus-4.6) 📄 | $0.0112 | $0.0112 | ±0% | — | — |
| 51_logs_summarize_errors (opus-4.6) 📄 | $0.1595 | $0.1577 | ±0% | — | — |
| 61_exact_match_counting (opus-4.6) 📄 | $0.1146 | — | — | — | — |
| Total (all, n=14) | $0.1924 | $0.2121 | — | — | — |
| Comparable (m=13, b=0) | $0.1984 | $0.2121 | ±0% | — | — |
Total tokens comparison:
| Test case | This branch | master (5h ago) | Δ vs master | benchmark (1d ago) | Δ vs benchmark |
|---|---|---|---|---|---|
| 09_crashpod (opus-4.6) 📄 | 69,332 | 50,526 | ↑37% | — | — |
| 101_loki_historical_logs_pod_deleted (opus-4.6) 📄 | 90,563 | 114,727 | ↓21% | — | — |
| 112_find_pvcs_by_uuid (opus-4.6) 📄 | 33,224 | 33,193 | ±0% | — | — |
| 12_job_crashing (opus-4.6) 📄 | 72,750 | 71,934 | ±0% | — | — |
| 176_network_policy_blocking_traffic_no_skills (opus-4.6) 📄 | 86,607 | 71,718 | ↑21% | — | — |
| 227_count_configmaps_per_namespace[0] (opus-4.6) 📄 | 46,356 | 46,348 | ±0% | — | — |
| 243_pod_names_contain_service (opus-4.6) 📄 | 49,725 | 66,980 | ↓26% | — | — |
| 24_misconfigured_pvc (opus-4.6) 📄 | 91,696 | 96,438 | ±0% | — | — |
| 254_elasticsearch_dr_test_log_check (opus-4.6) 📄 | 121,965 | 147,358 | ↓17% | — | — |
| 259_wrong_cluster_logs_confusion (opus-4.6) 📄 | 85,900 | 117,247 | ↓27% | — | — |
| 260_global_es_remote_cluster_logs (opus-4.6) 📄 | 108,078 | 124,237 | ↓13% | — | — |
| 43_current_datetime_from_prompt (opus-4.6) 📄 | 14,428 | 14,428 | ±0% | — | — |
| 51_logs_summarize_errors (opus-4.6) 📄 | 47,252 | 46,847 | ±0% | — | — |
| 61_exact_match_counting (opus-4.6) 📄 | 29,148 | — | — | — | — |
| Total (all, n=14) | 67,645 | 77,075 | — | — | — |
| Comparable (m=13, b=0) | 70,606 | 77,075 | ±0% | — | — |
Cached tokens comparison:
| Test case | This branch | master (5h ago) | Δ vs master | benchmark (1d ago) | Δ vs benchmark |
|---|---|---|---|---|---|
| 09_crashpod (opus-4.6) 📄 | 47,292 | 29,451 | ↑61% | — | — |
| 101_loki_historical_logs_pod_deleted (opus-4.6) 📄 | 65,886 | 87,946 | ↓25% | — | — |
| 112_find_pvcs_by_uuid (opus-4.6) 📄 | 14,319 | 14,319 | ±0% | — | — |
| 12_job_crashing (opus-4.6) 📄 | 49,234 | 48,877 | ±0% | — | — |
| 176_network_policy_blocking_traffic_no_skills (opus-4.6) 📄 | 65,655 | 47,680 | ↑38% | — | — |
| 227_count_configmaps_per_namespace[0] (opus-4.6) 📄 | 28,981 | 28,982 | ±0% | — | — |
| 243_pod_names_contain_service (opus-4.6) 📄 | 29,343 | 46,635 | ↓37% | — | — |
| 24_misconfigured_pvc (opus-4.6) 📄 | 68,894 | 71,055 | ±0% | — | — |
| 254_elasticsearch_dr_test_log_check (opus-4.6) 📄 | 99,754 | 125,614 | ↓21% | — | — |
| 259_wrong_cluster_logs_confusion (opus-4.6) 📄 | 63,820 | 93,553 | ↓32% | — | — |
| 260_global_es_remote_cluster_logs (opus-4.6) 📄 | 88,509 | 101,638 | ↓13% | — | — |
| 43_current_datetime_from_prompt (opus-4.6) 📄 | 14,303 | 14,303 | ±0% | — | — |
| 51_logs_summarize_errors (opus-4.6) 📄 | 28,929 | 28,930 | ±0% | — | — |
| 61_exact_match_counting (opus-4.6) 📄 | 14,283 | — | — | — | — |
| Total (all, n=14) | 48,514 | 56,845 | — | — | — |
| Comparable (m=13, b=0) | 51,148 | 56,845 | ↓10% | — | — |
Turns comparison:
| Test case | This branch | master (5h ago) | Δ vs master | benchmark (1d ago) | Δ vs benchmark |
|---|---|---|---|---|---|
| 09_crashpod (opus-4.6) 📄 | 4 | 3 | ↑33% | — | — |
| 101_loki_historical_logs_pod_deleted (opus-4.6) 📄 | 5 | 6 | ↓17% | — | — |
| 112_find_pvcs_by_uuid (opus-4.6) 📄 | 2 | 2 | ±0% | — | — |
| 12_job_crashing (opus-4.6) 📄 | 4 | 4 | ±0% | — | — |
| 176_network_policy_blocking_traffic_no_skills (opus-4.6) 📄 | 5 | 4 | ↑25% | — | — |
| 227_count_configmaps_per_namespace[0] (opus-4.6) 📄 | 3 | 3 | ±0% | — | — |
| 243_pod_names_contain_service (opus-4.6) 📄 | 3 | 4 | ↓25% | — | — |
| 24_misconfigured_pvc (opus-4.6) 📄 | 5 | 5 | ±0% | — | — |
| 254_elasticsearch_dr_test_log_check (opus-4.6) 📄 | 9 | 11 | ↓18% | — | — |
| 259_wrong_cluster_logs_confusion (opus-4.6) 📄 | 6 | 8 | ↓25% | — | — |
| 260_global_es_remote_cluster_logs (opus-4.6) 📄 | 8 | 9 | ↓11% | — | — |
| 43_current_datetime_from_prompt (opus-4.6) 📄 | 1 | 1 | ±0% | — | — |
| 51_logs_summarize_errors (opus-4.6) 📄 | 3 | 3 | ±0% | — | — |
| 61_exact_match_counting (opus-4.6) 📄 | 2 | — | — | — | — |
| Total (all, n=14) | 4.3 | 4.8 | — | — | — |
| Comparable (m=13, b=0) | 4.5 | 4.8 | ±0% | — | — |
Tool calls comparison:
| Test case | This branch | master (5h ago) | Δ vs master | benchmark (1d ago) | Δ vs benchmark |
|---|---|---|---|---|---|
| 09_crashpod (opus-4.6) 📄 | 8 | 7 | ↑14% | — | — |
| 101_loki_historical_logs_pod_deleted (opus-4.6) 📄 | 11 | 12 | ±0% | — | — |
| 112_find_pvcs_by_uuid (opus-4.6) 📄 | 2 | 2 | ±0% | — | — |
| 12_job_crashing (opus-4.6) 📄 | 10 | 9 | ↑11% | — | — |
| 176_network_policy_blocking_traffic_no_skills (opus-4.6) 📄 | 9 | 11 | ↓18% | — | — |
| 227_count_configmaps_per_namespace[0] (opus-4.6) 📄 | 6 | 6 | ±0% | — | — |
| 243_pod_names_contain_service (opus-4.6) 📄 | 7 | 7 | ±0% | — | — |
| 24_misconfigured_pvc (opus-4.6) 📄 | 9 | 13 | ↓31% | — | — |
| 254_elasticsearch_dr_test_log_check (opus-4.6) 📄 | 13 | 13 | ±0% | — | — |
| 259_wrong_cluster_logs_confusion (opus-4.6) 📄 | 9 | 11 | ↓18% | — | — |
| 260_global_es_remote_cluster_logs (opus-4.6) 📄 | 9 | 11 | ↓18% | — | — |
| 43_current_datetime_from_prompt (opus-4.6) 📄 | — | — | — | — | — |
| 51_logs_summarize_errors (opus-4.6) 📄 | 2 | 2 | ±0% | — | — |
| 61_exact_match_counting (opus-4.6) 📄 | 1 | — | — | — | — |
| Total (all, n=14) | 6.9 | 8.7 | — | — | — |
| Comparable (m=12, b=0) | 7.9 | 8.7 | ±0% | — | — |
Comparison indicators:
±0%— diff under 10% (within noise threshold)↑N%/↓N%— diff 10-25%↑N%/↓N%— diff over 25% (significant)
📖 Legend
| Icon | Meaning |
|---|---|
| ✅ | The test was successful |
| ➖ | The test was skipped |
| The test failed but is known to be flaky or known to fail | |
| 🚧 | The test had a setup failure (not a code regression) |
| 🔧 | The test failed due to mock data issues (not a code regression) |
| 🚫 | The test was throttled by API rate limits/overload |
| ❌ | The test failed and should be fixed before merging the PR |
🔄 Re-run evals manually
⚠️ Warning:/evalcomments always run using the workflow from master, not from this PR branch. If you modified the GitHub Action (e.g., added secrets or env vars), those changes won't take effect.To test workflow changes, use the GitHub CLI or Actions UI instead:
gh workflow run eval-regression.yaml --repo HolmesGPT/holmesgpt --ref claude/tender-cray-73dxge -f markers=regression -f filter=
Option 1: Comment on this PR with /eval:
/eval
tags: regression
Or with more options (one per line):
/eval
model: gpt-4o
tags: regression
id: 09_crashpod
iterations: 5
Run evals on a different branch (e.g., master) for comparison:
/eval
branch: master
tags: regression
| Option | Description |
|---|---|
model |
Model(s) to test (default: same as automatic runs) |
tags |
Pytest tags / markers (no default - runs all tests!) |
id |
Eval ID / pytest -k filter (use /list to see valid eval names) |
iterations |
Number of runs, max 10 |
branch |
Run evals on a different branch (for cross-branch comparison) |
Quick re-run: Use /rerun to re-run the most recent /eval on this PR with the same parameters.
Option 2: Trigger via GitHub Actions UI → "Run workflow"
Option 3: Add PR labels to include extra evals (applies to both automatic runs and /eval comments):
| Label | Effect |
|---|---|
evals-tag-<name> |
Run tests with tag <name> alongside regression |
evals-id-<name> |
Run a specific eval by test ID |
evals-model-<name> |
Override the model (use model list name, e.g. sonnet-4.5) |
Examples: evals-tag-easy, evals-id-09_crashpod, evals-model-sonnet-4.5
🏷️ Valid tags
benchmark, chain-of-causation, compaction, confluence, context_window, conversation_worker, coralogix, counting, database, datadog, datetime, db-connectors, easy, elasticsearch, embeds, fast, frontend, grafana, hard, images, integration, kafka, kubernetes, leaked-information, logs, loki, manual, mcp, medium, metrics, multi-cluster, network, newrelic, no-cicd, numerical, one-test, port-forward, prometheus, question-answer, regression, skills, slackbot, storage, token-limit, toolset-limitation, traces, transparency, victorialogs
🤖 Valid models
deepseek-chat, deepseek-r1-reasoner, deepseek-reasoner, deepseek-v3.2-chat, fable-5, gemini-3-flash-preview, gemini-3-pro-preview, gemini-3.1-pro-preview, gpt-4.1, gpt-5.2-high-reasoning, gpt-5.3-codex, gpt-5.4, gpt-5.5, haiku-4.5, kimi-2.5, kimi-2.5-openrouter, opus-4.5, opus-4.6, opus-4.7, opus-4.8, qwen-next-80B-instruct, qwen-next-80B-thinking, sonnet-4.5, sonnet-4.6
Commands: /eval · /rerun · /list
CLI: gh workflow run eval-regression.yaml --repo HolmesGPT/holmesgpt --ref claude/tender-cray-73dxge -f markers=regression -f filter=
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
WalkthroughThe PR enforces that the generated system prompt is placed at conversation index 0, relaxes the request-model pre-validator to only parse raw JSON without requiring a system-first message, and updates/adds tests to cover relaxed parsing and message-building edge cases. ChangesSystem Prompt Positioning and Validation Relaxation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Contributor
|
✅ Docker images ready for
Use these tags to pull the images for testing. 📋 Copy commandsgcloud auth configure-docker us-central1-docker.pkg.dev
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:5ce7401ad
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:5ce7401ad me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:5ce7401ad
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:5ce7401ad
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:5ce7401ad
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:5ce7401ad me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:5ce7401ad
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:5ce7401adPatch Helm values in one line (choose the chart you use): HolmesGPT chart: helm upgrade --install holmesgpt ./helm/holmes \
--set registry=me-west1-docker.pkg.dev/robusta-development/development \
--set image=holmes-dev:5ce7401ad \
--set operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set operator.image=holmes-operator-dev:5ce7401adRobusta wrapper chart: helm upgrade --install robusta robusta/robusta \
--reuse-values \
--set holmes.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.image=holmes-dev:5ce7401ad \
--set holmes.operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.operator.image=holmes-operator-dev:5ce7401ad |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
holmes/core/conversations.py (1)
52-61:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate docstring to reflect relaxed input handling.
Line 54 states "Expects conversation_history in OpenAI format (system message first)," but after ROB-288, the function no longer strictly requires this format at validation time.
add_or_update_system_promptnow handles non-system-first histories by inserting/overwriting to ensure the system prompt occupies position 0.Consider clarifying:
"""Build messages for general chat conversation. Ensures conversation_history has HolmesGPT's generated system prompt at position 0, inserting or overwriting as needed to maintain OpenAI format (system message first). For new conversations, creates system prompt via build_system_prompt. For existing conversations, updates the system prompt. ...🤖 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 `@holmes/core/conversations.py` around lines 52 - 61, Update the docstring for the function that "Build messages for general chat conversation" to reflect that conversation_history is no longer strictly required to be in OpenAI format (system message first); instead explain that add_or_update_system_prompt will insert or overwrite the HolmesGPT system prompt at position 0 to enforce OpenAI format, and retain notes about new conversations using build_system_prompt and that context window management is handled by call_stream() -> compact_if_necessary(). Mentioning add_or_update_system_prompt, build_system_prompt, and call_stream() provides anchors to the related logic.
🤖 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.
Outside diff comments:
In `@holmes/core/conversations.py`:
- Around line 52-61: Update the docstring for the function that "Build messages
for general chat conversation" to reflect that conversation_history is no longer
strictly required to be in OpenAI format (system message first); instead explain
that add_or_update_system_prompt will insert or overwrite the HolmesGPT system
prompt at position 0 to enforce OpenAI format, and retain notes about new
conversations using build_system_prompt and that context window management is
handled by call_stream() -> compact_if_necessary(). Mentioning
add_or_update_system_prompt, build_system_prompt, and call_stream() provides
anchors to the related logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2f271b60-9a35-41f1-95a4-ac86d78cfb59
📒 Files selected for processing (4)
holmes/core/conversations.pyholmes/core/models.pytests/core/test_models.pytests/core/test_prompt.py
✅ Deploy Preview for holmes-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
…ndling Signed-off-by: Claude <noreply@anthropic.com>
This branch has not been deployed
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
2 participants
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
Removes the strict requirement that
conversation_historymust start with a system message, allowing HolmesGPT to manage its own system prompt generation. This change supports the relay's fix (PR #582) which sends content-less system message stubs that HolmesGPT should overwrite.Key Changes
holmes/core/models.py: Renamedcheck_first_item_rolevalidator toparse_raw_json_bodyand removed the validation that required the first message to haverole: "system". The validator now only handles JSON parsing for raw request bodies (bytes/str without Content-Type header).holmes/core/conversations.py: Simplifiedadd_or_update_system_prompt()to always install HolmesGPT's generated system prompt at position 0:.get("role")for safer access to handle malformed messagestests/core/test_models.py: Updated test class and cases to reflect the new behavior:TestCheckFirstItemRoletoTestParseRawJsonBodytests/core/test_prompt.py: Added four comprehensive test cases covering the new system prompt handling:test_chat_messages_overwrites_empty_system_stub: Verifies empty stubs are replacedtest_chat_messages_inserts_system_when_first_is_user: Verifies insertion when missingtest_chat_messages_tolerates_first_message_without_role: Verifies robustness with malformed messagestest_chat_messages_preserves_trailing_compaction_marker: Verifies non-leading system messages are preservedImplementation Details
The change shifts responsibility for system prompt management from the request validator to the message builder. This allows:
https://claude.ai/code/session_011N5WiSiCoDiTxhTKiwjRQT
Summary by CodeRabbit
Bug Fixes
Tests