evaluation: add PromptIter regression and optimization workflow#2248
evaluation: add PromptIter regression and optimization workflow#2248DecarbonizedGlucose wants to merge 8 commits into
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
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:
📝 WalkthroughEnglishOverviewAdds a deterministic PromptIter regression workflow that:
Public API and compatibilityNew exported
API review questions:
Risks
Recommended validation
中文概览新增确定性的 PromptIter 回归优化流程，支持：
公共 API 与兼容性新增
需要确认：
风险
建议验证
WalkthroughChangesThe PR adds a deterministic Evaluation-to-PromptIter regression workflow: evaluation summaries and deltas, prioritized failure attribution, configurable gates, candidate orchestration, audit reports, and a runnable fake-runtime example with fixtures and end-to-end tests. Evaluation regression workflow
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
Adds the foundational building blocks for an auditable Evaluation ↔ PromptIter regression workflow by (1) normalizing Evaluation Service outputs into deterministic snapshots, (2) generating baseline/candidate deltas, (3) attributing failures into PromptIter-style hints, and (4) enforcing acceptance gates (regression/overfitting/budget).
Changes:
- Introduces deterministic snapshot normalization with evidence bounding and tool-value sanitization/redaction.
- Adds snapshot comparison (delta) and gate evaluation logic to accept/reject candidate prompts.
- Implements deterministic failure attribution and PromptIter-compatible hint generation, with unit tests covering key scenarios.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| evaluation/workflow/promptiter/regression/snapshot.go | Defines package-private normalized snapshot schema used by later regression stages. |
| evaluation/workflow/promptiter/regression/normalize.go | Normalizes Evaluation Service results into bounded, deterministic snapshots (incl. tool sanitization). |
| evaluation/workflow/promptiter/regression/normalize_test.go | Tests normalization determinism, evidence preservation, redaction, and invalid-input handling. |
| evaluation/workflow/promptiter/regression/delta.go | Compares baseline vs candidate snapshots into structured deltas for gating. |
| evaluation/workflow/promptiter/regression/delta_test.go | Tests delta classification, ordering-independence, and validation errors. |
| evaluation/workflow/promptiter/regression/gate.go | Evaluates configurable acceptance gates (score gain, hard fails, critical cases, budgets, overfitting). |
| evaluation/workflow/promptiter/regression/gate_test.go | Tests gate decisions across regression, budget, evaluation-state, and overfitting scenarios. |
| evaluation/workflow/promptiter/regression/cost.go | Adds cost aggregation/validation helpers for gate budget checks. |
| evaluation/workflow/promptiter/regression/attribution.go | Attributes failures deterministically and builds PromptIter-style failure hints. |
| evaluation/workflow/promptiter/regression/attribution_test.go | Tests attribution category selection, determinism, priority rules, and hint bounding. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| canonical := strings.NewReplacer("_", "", "-", "", ".", "", " ", "").Replace(strings.ToLower(key)) | ||
| switch canonical { | ||
| case "authorization", "proxyauthorization", "apikey", "token", "accesstoken", "refreshtoken", | ||
| "password", "secret", "clientsecret", "cookie", "setcookie": | ||
| return true | ||
| default: | ||
| return false | ||
| } |
| if len(metricNames) == 0 { | ||
| metricNames = []string{""} | ||
| } |
|
I have read the CLA Document and I hereby sign the CLA |
DecarbonizedGlucose
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2248 +/- ##
===================================================
- Coverage 89.88574% 89.83556% -0.05018%
===================================================
Files 1146 1157 +11
Lines 199550 204576 +5026
===================================================
+ Hits 179367 183782 +4415
- Misses 12645 13006 +361
- Partials 7538 7788 +250
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
evaluation/workflow/promptiter/regression/delta_test.go (1)
217-226: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise different ordering between baseline and candidate.
Both inputs are reversed together, so a positional comparator would still pass. Compare the ordered baseline against the reversed candidate (and optionally the reverse direction).
中文
请测试基线与候选顺序不一致的情况。
当前两个输入同时反转，错误的按位置比较实现仍会通过。应使用有序基线与逆序候选进行比较，并可补充反向组合。
As per path instructions, tests should validate externally observable ordering behavior rather than implementation details.
🤖 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 `@evaluation/workflow/promptiter/regression/delta_test.go` around lines 217 - 226, Update TestCompareSnapshotsIsIndependentOfInputOrder to compare the ordered baseline against reorderedCandidate, rather than reversing the baseline as well. Keep the comparison assertion validating equal results, and optionally add the reverse baseline/candidate combination to cover both ordering directions.Source: Path instructions
evaluation/workflow/promptiter/regression/attribution_test.go (1)
104-118: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the exact P0–P3 severity for every category.
These cases only require a non-empty hint; separate coverage checks P0 and P3, leaving P1 and P2 mappings unprotected. Add the expected severity to this table and compare it with
hints[0].severity.中文
请断言每个分类对应的准确 P0–P3 严重级别。
当前用例只检查提示非空；其他测试仅覆盖 P0 和 P3，P1 与 P2 映射缺少保护。建议在表格中加入期望严重级别，并与
hints[0].severity比较。As per path instructions, assertions should be strong enough to verify the intended external behavior.
🤖 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 `@evaluation/workflow/promptiter/regression/attribution_test.go` around lines 104 - 118, Extend the test case table used by the attributeFailures/buildFailureHints test with an expected P0–P3 severity for each category, then replace the generic hints non-empty check with an exact comparison between that expected value and hints[0].severity. Keep the existing attribution and hint assertions unchanged.Source: Path instructions
🤖 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 `@evaluation/workflow/promptiter/regression/attribution.go`:
- Around line 434-437: Update classifyFailedMetric so format_error is selected
only when actual format validation failed, not merely because JSON/XML criteria
or format-related rubric keywords are configured. Preserve accurate
classification for content or LLM-judge failures by using available failure
evidence or per-criterion outcomes, and add a regression test covering valid
JSON with a content/accuracy failure.
In `@evaluation/workflow/promptiter/regression/gate.go`:
- Around line 30-35: Bind gate attribution to the exact candidate validation
snapshot instead of accepting any non-nil validationAttribution in gateInput.
Update the gate flow and its construction sites to compute attribution from the
candidate inside the gate, or validate that the supplied summary has matching
snapshot/eval-set provenance and complete execution evidence; reject missing,
stale, mismatched, or incomplete attribution so execution failures cannot be
accepted. Preserve clear error propagation across the framework execution path.
In `@evaluation/workflow/promptiter/regression/normalize.go`:
- Around line 948-956: Update sensitiveToolKey to recognize common prefixed
credential headers such as xapikey and prefixed token variants after
canonicalization, ensuring their values are redacted before audit snapshots
persist them. Add regression tests covering these headers and verifying no
credential value leaks.
- Around line 642-644: Preserve the tool-trajectory OrderSensitive setting when
building the snapshot in the configuration handling around ToolTrajectory.
Update the attribution comparison in the relevant tool-list logic to honor that
snapshot value, retaining order-sensitive matching when true and
order-insensitive matching for the false/zero default. Add coverage for both
settings and ensure the existing order-insensitive default behavior remains
unchanged.
---
Nitpick comments:
In `@evaluation/workflow/promptiter/regression/attribution_test.go`:
- Around line 104-118: Extend the test case table used by the
attributeFailures/buildFailureHints test with an expected P0–P3 severity for
each category, then replace the generic hints non-empty check with an exact
comparison between that expected value and hints[0].severity. Keep the existing
attribution and hint assertions unchanged.
In `@evaluation/workflow/promptiter/regression/delta_test.go`:
- Around line 217-226: Update TestCompareSnapshotsIsIndependentOfInputOrder to
compare the ordered baseline against reorderedCandidate, rather than reversing
the baseline as well. Keep the comparison assertion validating equal results,
and optionally add the reverse baseline/candidate combination to cover both
ordering directions.
🪄 Autofix (Beta)
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: Pro
Run ID: 1993c427-b08f-4ee7-acb6-fa33e5949d84
📒 Files selected for processing (10)
evaluation/workflow/promptiter/regression/attribution.goevaluation/workflow/promptiter/regression/attribution_test.goevaluation/workflow/promptiter/regression/cost.goevaluation/workflow/promptiter/regression/delta.goevaluation/workflow/promptiter/regression/delta_test.goevaluation/workflow/promptiter/regression/gate.goevaluation/workflow/promptiter/regression/gate_test.goevaluation/workflow/promptiter/regression/normalize.goevaluation/workflow/promptiter/regression/normalize_test.goevaluation/workflow/promptiter/regression/snapshot.go
| type gateInput struct { | ||
| delta *evaluationDelta | ||
| validationAttribution *attributionSummary | ||
| candidateServingCost costSnapshot | ||
| optimizationCost costSnapshot | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Bind attribution to the exact candidate validation snapshot.
Only nil is rejected, so an empty, stale, or wrong attribution summary reports no execution failures. Because evaluationDelta contains no execution evidence, such a candidate can be accepted. Compute attribution from the candidate inside the gate, or validate snapshot/eval-set provenance and completeness before trusting it.
中文
请将归因结果绑定到准确的候选验证快照。
当前仅拒绝 nil，因此空的、过期的或错误的归因摘要会被视为没有执行失败。由于 evaluationDelta 不包含执行证据，这类候选可能被错误接受。应在门禁内部基于候选快照生成归因，或在使用前验证快照/数据集来源及完整性。
The PR objective requires execution failures to be rejected. As per path instructions, framework execution paths must preserve clear cross-layer error semantics.
Also applies to: 68-76, 206-208
🤖 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 `@evaluation/workflow/promptiter/regression/gate.go` around lines 30 - 35, Bind
gate attribution to the exact candidate validation snapshot instead of accepting
any non-nil validationAttribution in gateInput. Update the gate flow and its
construction sites to compute attribution from the candidate inside the gate, or
validate that the supplied summary has matching snapshot/eval-set provenance and
complete execution evidence; reject missing, stale, mismatched, or incomplete
attribution so execution failures cannot be accepted. Preserve clear error
propagation across the framework execution path.
Source: Path instructions
| if config.ToolTrajectory != nil { | ||
| kinds = append(kinds, "tool_trajectory") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Preserve tool-trajectory ordering semantics for attribution.
This stores only "tool_trajectory", so attribution.go Lines 333–335 always compares ordered tool lists. Consequently, the false/zero OrderSensitive setting can still produce a P1 tool-call hint for harmless reordering.
Preserve this option in the snapshot, honor it during comparison, and test both settings. Existing evaluation behavior permits order-insensitive matching; the new attribution behavior does not.
As per path instructions: “Whether default behavior remains stable” is a review priority.
中文
请为失败归因保留工具轨迹的顺序语义。
这里仅保存了 "tool_trajectory"，导致 attribution.go 第 333–335 行始终按顺序比较工具列表。因此，即使 OrderSensitive 为 false/零值，无害的顺序变化仍可能产生 P1 工具调用提示。
请在快照中保留该选项，在比较时遵循它，并分别测试 true 和 false。现有评估支持忽略顺序，而新的归因行为没有保持该语义。
根据路径说明，应重点检查“默认行为是否保持稳定”。
🤖 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 `@evaluation/workflow/promptiter/regression/normalize.go` around lines 642 -
644, Preserve the tool-trajectory OrderSensitive setting when building the
snapshot in the configuration handling around ToolTrajectory. Update the
attribution comparison in the relevant tool-list logic to honor that snapshot
value, retaining order-sensitive matching when true and order-insensitive
matching for the false/zero default. Add coverage for both settings and ensure
the existing order-insensitive default behavior remains unchanged.
Source: Path instructions
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
examples/evaluation/promptiter_regression_loop/main.go (1)
34-40: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPropagate OS interrupts into the pipeline context.
Real-mode evaluations may be long-running. Using
context.Background()prevents graceful cancellation and cleanup on Ctrl+C.Proposed fix
import ( "context" "errors" "flag" "fmt" "io" "os" + "os/signal" "time" ) func main() { options, err := parseCommandOptions(os.Args[1:], os.Stderr) if err != nil { fmt.Fprintln(os.Stderr, err) os.Exit(2) } - report, err := runCommand(context.Background(), options) + ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt) + defer stop() + report, err := runCommand(ctx, options)中文
将操作系统中断传递到流水线上下文。
真实模式的评估可能运行较久。使用
context.Background()会导致 Ctrl+C 无法触发正常取消和资源清理，建议使用信号感知的上下文。As per path instructions, examples must execute safely and long-running work must stop promptly.
🤖 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 `@examples/evaluation/promptiter_regression_loop/main.go` around lines 34 - 40, Update main’s pipeline context creation before runCommand to use a signal-aware context that cancels on OS interrupts, such as SIGINT, and ensure its cancellation is released when main exits. Pass this context to runCommand instead of context.Background() so Ctrl+C stops long-running evaluation and cleanup promptly.Source: Path instructions
evaluation/workflow/promptiter/regression/cost.go (1)
13-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDefine the exported availability and zero-value contract.
The zero value means measurements are unavailable, while observed zero requires callers to set the availability flags. Document which fields each flag governs—especially
LatencyMillis—because budget gates fail closed when availability is false.中文
请明确导出的可用性与零值契约。
当前零值表示指标不可用，而“实际观测值为零”要求调用方显式设置可用性标志。请说明每个标志覆盖哪些字段，尤其是
LatencyMillis；否则外部适配器可能导致预算门禁错误地关闭。As per path instructions, exported types and zero-value semantics must remain unambiguous and easy to use.
🤖 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 `@evaluation/workflow/promptiter/regression/cost.go` around lines 13 - 24, Document the exported CostSnapshot type and its zero-value contract: zero-valued measurements are unavailable, while observed zero values require the corresponding availability flags. Clearly specify that ModelCallsAvailable governs ModelCalls and UsageAvailable governs all token fields plus LatencyMillis, preserving fail-closed budget behavior.Source: Path instructions
evaluation/workflow/promptiter/regression/promptiter.go (1)
125-145: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDifferentiate between missing surface and empty text.
When
foundis false,valueremainsnil, causing the function to fall through and return the "has no text" error. It would be clearer to explicitly check for the missing surface and provide a distinct error message.中文
区分 surface 缺失和文本为空的情况。
当
found为 false 时，value保持为nil，这会导致函数继续向下执行并返回 "has no text" 的错误。最好显式检查 surface 是否缺失，并提供一个独立的错误提示。♻️ Proposed refactor
if found { return Prompt{}, fmt.Errorf("PromptIter candidate profile contains duplicate surface %q", targetSurfaceID) } found = true value = override.Value.Text } + if !found { + return Prompt{}, fmt.Errorf("PromptIter candidate profile does not contain surface %q", targetSurfaceID) + } if value == nil || strings.TrimSpace(*value) == "" { return Prompt{}, fmt.Errorf("PromptIter candidate surface %q has no text", targetSurfaceID) }🤖 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 `@evaluation/workflow/promptiter/regression/promptiter.go` around lines 125 - 145, Update promptFromProfile to explicitly check found after scanning profile.Overrides and return a distinct missing-surface error when no override matches targetSurfaceID; retain the existing empty-text validation for matched overrides with nil or blank Value.Text.Source: Path instructions
examples/evaluation/promptiter_regression_loop/DESIGN.md (1)
7-7: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix grammatical error.
As highlighted by static analysis tools, the verb should be in the passive voice.
中文
修复语法错误。
正如静态分析工具指出的，此处的动词应使用被动语态。
♻️ Proposed refactor
-真实接入调用 PromptIter 单轮优化指定 instruction；无密钥样例使用确定性候选。每轮比较当前 prompt 与候选的逐 case、逐 metric 变化。验证提升不足、执行错误、指标退化或超预算都会拒绝。训练上升而验证下降会命中过拟合门禁，被拒绝候选不会进入下一轮。 +真实接入调用 PromptIter 单轮优化指定 instruction；无密钥样例使用确定性候选。每轮比较当前 prompt 与候选的逐 case、逐 metric 变化。验证提升不足、执行错误、指标退化或超预算都会被拒绝。训练上升而验证下降会命中过拟合门禁，被拒绝候选不会进入下一轮。🤖 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 `@examples/evaluation/promptiter_regression_loop/DESIGN.md` at line 7, 更新 DESIGN.md 中“真实接入调用 PromptIter”的表述，将“调用”改为被动语态，使其表达为 PromptIter 被真实接入调用，同时保持其余说明含义不变。Sources: Path instructions, Linters/SAST tools
🤖 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 `@evaluation/workflow/promptiter/regression/pipeline.go`:
- Around line 358-376: Update validatePipelineRequest to reject requests where
TrainEvalSetID equals ValidationEvalSetID, returning a validation error before
gate-policy validation. Preserve the existing checks for empty IDs and other
request fields, and add a regression test covering identical non-empty eval-set
IDs.
In `@evaluation/workflow/promptiter/regression/report_markdown.go`:
- Around line 147-162: Update writeMarkdownCost to render ModelCalls and token
usage as “N/A” when their corresponding ModelCallsAvailable or UsageAvailable
flags are false, while preserving numeric zeroes when evidence is available.
Extend the golden report fixtures or tests covering this markdown table to
verify unavailable telemetry is displayed as N/A.
- Around line 207-212: Update markdownCell to HTML-escape the input value before
converting newline characters to generated <br> elements, preserving those
generated breaks as markup. Add a regression test covering a malicious HTML tag
and verify the tag is escaped while newline conversion still produces <br>.
In `@evaluation/workflow/promptiter/regression/report.go`:
- Around line 406-412: Update the audit input path validation around the
input.Path normalization and relative-path checks to reject Windows drive-letter
paths and UNC paths on every host before filepath.Clean or filepath.ToSlash
runs. Preserve rejection of host-native absolute and parent-traversal paths, and
add cross-platform cases covering representative drive-letter and UNC inputs.
In `@examples/evaluation/promptiter_regression_loop/e2e_test.go`:
- Around line 27-31: Replace the elapsed-time assertion around runFakeLoop with
a cancellable context.WithTimeout using the existing three-minute bound. Pass
the derived context to runFakeLoop, ensure the context is canceled, and retain
the error assertion so hung executions terminate through the deadline rather
than relying on wall-clock timing.
In `@examples/evaluation/promptiter_regression_loop/output.go`:
- Around line 104-113: Update the regression.AuditRuntime configuration to
record a sanitized or hashed identity for the effective OPENAI_BASE_URL, so runs
against different endpoints are distinguishable without exposing sensitive URL
data. Include every effective real-mode gate budget from real.go, including
overrides, in the AuditConfigEntry list alongside force_all_rounds and
max_rounds, using the final runtime values rather than defaults.
In `@examples/evaluation/promptiter_regression_loop/real.go`:
- Around line 87-107: Update the real-mode pipeline setup around gatePolicy and
regression.Run so the four budget thresholds from config.gatePolicy are
preserved instead of unconditionally assigning 200 and 500000. If generous
real-mode limits are still needed, expose them as explicit defaults or options
applied only when values are unset, then update the related tests and README to
document the effective policy.
---
Nitpick comments:
In `@evaluation/workflow/promptiter/regression/cost.go`:
- Around line 13-24: Document the exported CostSnapshot type and its zero-value
contract: zero-valued measurements are unavailable, while observed zero values
require the corresponding availability flags. Clearly specify that
ModelCallsAvailable governs ModelCalls and UsageAvailable governs all token
fields plus LatencyMillis, preserving fail-closed budget behavior.
In `@evaluation/workflow/promptiter/regression/promptiter.go`:
- Around line 125-145: Update promptFromProfile to explicitly check found after
scanning profile.Overrides and return a distinct missing-surface error when no
override matches targetSurfaceID; retain the existing empty-text validation for
matched overrides with nil or blank Value.Text.
In `@examples/evaluation/promptiter_regression_loop/DESIGN.md`:
- Line 7: 更新 DESIGN.md 中“真实接入调用 PromptIter”的表述，将“调用”改为被动语态，使其表达为 PromptIter
被真实接入调用，同时保持其余说明含义不变。
In `@examples/evaluation/promptiter_regression_loop/main.go`:
- Around line 34-40: Update main’s pipeline context creation before runCommand
to use a signal-aware context that cancels on OS interrupts, such as SIGINT, and
ensure its cancellation is released when main exits. Pass this context to
runCommand instead of context.Background() so Ctrl+C stops long-running
evaluation and cleanup promptly.
🪄 Autofix (Beta)
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: Pro
Run ID: 6497927c-d58b-4c94-b34c-b3f9c02f5bda
📒 Files selected for processing (48)
evaluation/workflow/promptiter/regression/attribution.goevaluation/workflow/promptiter/regression/attribution_alignment_test.goevaluation/workflow/promptiter/regression/attribution_test.goevaluation/workflow/promptiter/regression/cost.goevaluation/workflow/promptiter/regression/cost_validation_test.goevaluation/workflow/promptiter/regression/delta_test.goevaluation/workflow/promptiter/regression/delta_validation_test.goevaluation/workflow/promptiter/regression/gate.goevaluation/workflow/promptiter/regression/gate_test.goevaluation/workflow/promptiter/regression/gate_validation_test.goevaluation/workflow/promptiter/regression/normalize.goevaluation/workflow/promptiter/regression/normalize_contract_test.goevaluation/workflow/promptiter/regression/normalize_test.goevaluation/workflow/promptiter/regression/pipeline.goevaluation/workflow/promptiter/regression/pipeline_test.goevaluation/workflow/promptiter/regression/promptiter.goevaluation/workflow/promptiter/regression/public_test.goevaluation/workflow/promptiter/regression/report.goevaluation/workflow/promptiter/regression/report_json.goevaluation/workflow/promptiter/regression/report_markdown.goevaluation/workflow/promptiter/regression/report_test.goevaluation/workflow/promptiter/regression/snapshot.goevaluation/workflow/promptiter/regression/testdata/report_accepted.mdevaluation/workflow/promptiter/regression/testdata/report_rejected.mdexamples/evaluation/promptiter_regression_loop/DESIGN.mdexamples/evaluation/promptiter_regression_loop/README.mdexamples/evaluation/promptiter_regression_loop/config.goexamples/evaluation/promptiter_regression_loop/config_test.goexamples/evaluation/promptiter_regression_loop/data/baseline_prompt.txtexamples/evaluation/promptiter_regression_loop/data/fake_engine.jsonexamples/evaluation/promptiter_regression_loop/data/metrics.jsonexamples/evaluation/promptiter_regression_loop/data/promptiter.jsonexamples/evaluation/promptiter_regression_loop/data/train.evalset.jsonexamples/evaluation/promptiter_regression_loop/data/validation.evalset.jsonexamples/evaluation/promptiter_regression_loop/e2e_test.goexamples/evaluation/promptiter_regression_loop/evaluator.goexamples/evaluation/promptiter_regression_loop/fake_model.goexamples/evaluation/promptiter_regression_loop/fake_optimizer.goexamples/evaluation/promptiter_regression_loop/main.goexamples/evaluation/promptiter_regression_loop/output.goexamples/evaluation/promptiter_regression_loop/output/optimization_report.jsonexamples/evaluation/promptiter_regression_loop/output/optimization_report.mdexamples/evaluation/promptiter_regression_loop/pipeline_test.goexamples/evaluation/promptiter_regression_loop/promptiter.goexamples/evaluation/promptiter_regression_loop/real.goexamples/evaluation/promptiter_regression_loop/real_test.gosession/session.gosession/session_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- evaluation/workflow/promptiter/regression/snapshot.go
- evaluation/workflow/promptiter/regression/gate.go
- evaluation/workflow/promptiter/regression/attribution.go
- evaluation/workflow/promptiter/regression/delta_test.go
- evaluation/workflow/promptiter/regression/normalize.go
- evaluation/workflow/promptiter/regression/normalize_test.go
- evaluation/workflow/promptiter/regression/gate_test.go
| func writeMarkdownCost(output *strings.Builder, report *OptimizationReport) { | ||
| output.WriteString("## Cost and Latency\n\n") | ||
| output.WriteString("| Scope | Model calls | Total tokens | Latency ms |\n| --- | ---: | ---: | ---: |\n") | ||
| rows := []struct { | ||
| name string | ||
| cost ReportCost | ||
| }{ | ||
| {name: "Baseline serving", cost: report.Cost.BaselineServing}, | ||
| {name: "Candidate serving", cost: report.Cost.CandidateServing}, | ||
| {name: "Optimization", cost: report.Cost.Optimization}, | ||
| {name: "Total pipeline", cost: report.Cost.TotalPipeline}, | ||
| } | ||
| for _, row := range rows { | ||
| fmt.Fprintf(output, "| %s | %d | %d | %d |\n", | ||
| row.name, row.cost.ModelCalls, row.cost.TotalTokens, row.cost.LatencyMillis) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Do not render unavailable cost evidence as zero.
When ModelCallsAvailable or UsageAvailable is false, this table still prints 0, making missing telemetry indistinguishable from actual zero usage. Render unavailable fields as N/A and cover that contract in the golden reports.
中文
不要将不可用的成本证据显示为零。
当 ModelCallsAvailable 或 UsageAvailable 为 false 时，此表仍显示 0，导致缺失遥测与实际零用量无法区分。请将不可用字段显示为 N/A，并在黄金文件测试中覆盖该契约。
🤖 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 `@evaluation/workflow/promptiter/regression/report_markdown.go` around lines
147 - 162, Update writeMarkdownCost to render ModelCalls and token usage as
“N/A” when their corresponding ModelCallsAvailable or UsageAvailable flags are
false, while preserving numeric zeroes when evidence is available. Extend the
golden report fixtures or tests covering this markdown table to verify
unavailable telemetry is displayed as N/A.
| func markdownCell(value string) string { | ||
| value = strings.ReplaceAll(value, "|", "\\|") | ||
| value = strings.ReplaceAll(value, "\r\n", "<br>") | ||
| value = strings.ReplaceAll(value, "\n", "<br>") | ||
| value = strings.ReplaceAll(value, "\r", "<br>") | ||
| return value |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Escape raw HTML in Markdown cells.
Case IDs, reasons, runtime configuration, and other untrusted values retain HTML tags. Markdown renderers that permit raw HTML can therefore execute injected content. HTML-escape the value before inserting generated <br> elements and add a malicious-tag regression test.
中文
转义 Markdown 单元格中的原始 HTML。
案例 ID、原因和运行时配置等不可信内容会保留 HTML 标签；允许原始 HTML 的渲染器可能执行注入内容。请先进行 HTML 转义，再插入生成的 <br>，并添加恶意标签回归测试。
As per path instructions, external and model-driven values must be treated as untrusted.
🤖 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 `@evaluation/workflow/promptiter/regression/report_markdown.go` around lines
207 - 212, Update markdownCell to HTML-escape the input value before converting
newline characters to generated <br> elements, preserving those generated breaks
as markup. Add a regression test covering a malicious HTML tag and verify the
tag is escaped while newline conversion still produces <br>.
Source: Path instructions
| input.Path = filepath.ToSlash(filepath.Clean(strings.TrimSpace(input.Path))) | ||
| input.SHA256 = strings.ToLower(strings.TrimSpace(input.SHA256)) | ||
| if input.Name == "" || input.Path == "" || input.Path == "." { | ||
| return AuditMetadata{}, fmt.Errorf("audit input %d is incomplete", index) | ||
| } | ||
| if filepath.IsAbs(input.Path) || input.Path == ".." || strings.HasPrefix(input.Path, "../") { | ||
| return AuditMetadata{}, fmt.Errorf("audit input %q path must be relative", input.Name) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Reject Windows and UNC absolute paths on every host.
filepath.IsAbs uses host semantics, so Linux accepts paths such as C:\Users\alice\input.json or \\server\share\input.json and emits them into the report. Detect drive-letter and UNC paths before normalization and add cross-platform cases.
中文
在所有平台上拒绝 Windows 和 UNC 绝对路径。
filepath.IsAbs 使用宿主平台语义，因此 Linux 会放行 C:\Users\alice\input.json、\\server\share\input.json 等路径并将其写入报告。请在规范化前检测盘符和 UNC 路径，并补充跨平台测试。
As per path instructions, external paths must be treated as untrusted.
🤖 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 `@evaluation/workflow/promptiter/regression/report.go` around lines 406 - 412,
Update the audit input path validation around the input.Path normalization and
relative-path checks to reject Windows drive-letter paths and UNC paths on every
host before filepath.Clean or filepath.ToSlash runs. Preserve rejection of
host-native absolute and parent-traversal paths, and add cross-platform cases
covering representative drive-letter and UNC inputs.
Source: Path instructions
| started := time.Now() | ||
| config := fixtureTestConfig(t) | ||
| run, err := runFakeLoop(context.Background(), config) | ||
| require.NoError(t, err) | ||
| assert.Less(t, time.Since(started), 3*time.Minute) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Use a cancellable deadline instead of elapsed wall-clock timing.
This assertion cannot terminate a hung run and may fail on slow or race-enabled CI machines. Run the loop with context.WithTimeout so the bound actively cancels execution.
中文
使用可取消的截止时间代替墙钟耗时断言。
该断言无法终止卡住的执行，并可能在较慢或启用 race 的 CI 环境中误报。请使用 context.WithTimeout 运行循环，使时间限制能够主动取消执行。
As per path instructions, tests should avoid machine-specific timing differences.
🤖 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 `@examples/evaluation/promptiter_regression_loop/e2e_test.go` around lines 27 -
31, Replace the elapsed-time assertion around runFakeLoop with a cancellable
context.WithTimeout using the existing three-minute bound. Pass the derived
context to runFakeLoop, ensure the context is canceled, and retain the error
assertion so hung executions terminate through the deadline rather than relying
on wall-clock timing.
Source: Path instructions
| Runtime: regression.AuditRuntime{ | ||
| Mode: "real", | ||
| Provider: "openai", | ||
| Model: modelName, | ||
| Config: []regression.AuditConfigEntry{ | ||
| {Key: "force_all_rounds", Value: "false"}, | ||
| {Key: "max_rounds", Value: fmt.Sprint(config.maxRounds)}, | ||
| {Key: "optimizer", Value: "promptiter"}, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Audit the effective endpoint and real-mode gate configuration.
Runs using different OPENAI_BASE_URL values currently produce the same provider/model identity, and the effective budget overrides from real.go are omitted. Record a sanitized or hashed endpoint identity plus every effective gate budget so the report can reproduce and distinguish real executions.
中文
审计实际端点和真实模式的门控配置。
使用不同 OPENAI_BASE_URL 的运行目前会生成相同的 provider/model 标识，并且 real.go 中实际生效的预算覆盖值未被记录。请记录经过清理或哈希的端点标识以及所有实际门控预算，以便报告能够复现并区分真实运行。
🤖 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 `@examples/evaluation/promptiter_regression_loop/output.go` around lines 104 -
113, Update the regression.AuditRuntime configuration to record a sanitized or
hashed identity for the effective OPENAI_BASE_URL, so runs against different
endpoints are distinguishable without exposing sensitive URL data. Include every
effective real-mode gate budget from real.go, including overrides, in the
AuditConfigEntry list alongside force_all_rounds and max_rounds, using the final
runtime values rather than defaults.
| gatePolicy := config.gatePolicy | ||
| // Real-mode budgets replace the tiny fixture values originally meant for | ||
| // deterministic fake mode. Three rounds with backwarder+aggregator+optimizer | ||
| // plus candidate serving evaluation produce at most ~50 model calls and | ||
| // ~100 000 tokens in typical scenarios. 200 calls / 500k tokens per budget | ||
| // category provide a generous safety margin while still catching runaway | ||
| // loops. | ||
| gatePolicy.MaxCandidateServingModelCalls = 200 | ||
| gatePolicy.MaxCandidateServingTotalTokens = 500000 | ||
| gatePolicy.MaxOptimizationModelCalls = 200 | ||
| gatePolicy.MaxOptimizationTotalTokens = 500000 | ||
| pipeline := regression.NewPipeline(evaluator, generator) | ||
| return pipeline.Run(ctx, regression.RunRequest{ | ||
| InitialPrompt: config.baselinePrompt, | ||
| TrainEvalSetID: config.trainEvalSetID, | ||
| ValidationEvalSetID: config.validationEvalSetID, | ||
| GatePolicy: gatePolicy, | ||
| MaxRounds: config.maxRounds, | ||
| Seed: config.seed, | ||
| ForceAllRounds: false, | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not silently replace configured real-mode budgets.
Configured behavior: promptiter.json supplies the four budget thresholds. Actual real-mode behavior: all four are replaced with 200 calls and 500000 tokens, so users cannot enforce stricter billable-run limits. Introduce explicit real-mode defaults/options and preserve caller-provided values; update tests and README with the effective policy.
中文
不要静默覆盖真实模式中配置的预算。
配置契约是由 promptiter.json 提供四个预算阈值，但真实模式会将它们全部覆盖为 200 次调用和 500000 个 token，导致用户无法执行更严格的付费运行限制。请提供明确的真实模式默认值或选项，并保留调用方配置；同时更新测试和 README，说明实际策略。
As per path instructions, configured defaults and behavior should remain stable rather than being silently replaced.
🤖 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 `@examples/evaluation/promptiter_regression_loop/real.go` around lines 87 -
107, Update the real-mode pipeline setup around gatePolicy and regression.Run so
the four budget thresholds from config.gatePolicy are preserved instead of
unconditionally assigning 200 and 500000. If generous real-mode limits are still
needed, expose them as explicit defaults or options applied only when values are
unset, then update the related tests and README to document the effective
policy.
Source: Path instructions
7d38ba4 to
4c384e2
Compare
|
I substantially reworked this PR to reduce its scope and review cost. The branch history was force-updated, so previous inline comments may now appear outdated. The current ordinary and race tests pass for both the regression package and the example. |
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 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 `@evaluation/workflow/promptiter/regression/compare.go`:
- Around line 121-134: Before invoking classifyDelta in the metric comparison
loop, validate that baseline and candidate thresholds are finite and equal;
reject the comparison with an appropriate error when either threshold is
non-finite or they differ. Preserve evaluation and protocol structure
compatibility, and add a regression test covering identical scores with
different thresholds.
- Around line 186-190: Update sameKeys to compare the sorted key slices
structurally rather than converting them with fmt.Sprint. Preserve the existing
error message and return behavior when the key sets differ, while ensuring
distinct keys containing spaces cannot compare equal.
In `@evaluation/workflow/promptiter/regression/promptiter.go`:
- Around line 46-55: Update the request-building flow around
runRequest.InitialProfile to preserve base.InitialProfile: copy the profile,
clone its Overrides slice, then replace the matching targetSurfaceID override or
append one when absent. Keep all non-target overrides intact, and add a
multi-surface regression test verifying both preservation and that the base
request remains unchanged.
In `@evaluation/workflow/promptiter/regression/report.go`:
- Around line 175-176: Validate run.TotalCost for negative values before
constructing the audit report, alongside the existing per-round cost validation,
and return an appropriate error when invalid. Add a regression test covering a
negative total cost while all round costs remain valid.
- Around line 279-325: Update WriteMarkdown to validate all required nested
report fields before rendering, including baseline evaluations, each round’s
train/validation evaluations and deltas, and serving/optimization costs; return
a descriptive error for any missing or incomplete field instead of dereferencing
it. Keep the existing rendering behavior unchanged for complete reports.
In `@evaluation/workflow/promptiter/regression/result.go`:
- Around line 428-443: Update sensitiveKey to recognize the credential aliases
“passwd”, “passphrase”, and “pwd” alongside the existing fragments, preserving
its current normalization and substring matching behavior. Add explicit
regression tests covering each alias and verifying that corresponding values are
redacted from serialized tool evidence.
- Around line 446-472: Update addCaseCost to track which individual runs are
covered by RunDetails execution traces, rather than using the case-wide
traceFound flag. For each EvalCaseResults run without a covered RunDetails
trace, apply the existing ActualInvocation.ExecutionTrace fallback, while
preventing duplicate cost counting for runs already covered; add a regression
test with the two trace sources in separate runs.
In `@examples/evaluation/promptiter_regression_loop/main.go`:
- Around line 92-109: Update the report-writing flow around openReport,
regression.WriteJSON, and regression.WriteMarkdown to stage both outputs in
temporary files within the output directory, close and validate both
successfully, then publish both final paths together via coordinated renames. On
any open, write, close, or publish failure, clean up temporary files and avoid
leaving partially updated or mismatched optimization reports.
- Around line 45-56: Extend parseOptions and the command execution flow to
support an optional real runtime/model-backed mode while preserving fake
execution as the default. Add the required runtime/model configuration, select
the appropriate workflow instead of always calling runFakeLoop, and report the
selected mode rather than hardcoding "fake"; update tests to cover both modes
and document their usage and defaults.
In `@examples/evaluation/promptiter_regression_loop/runtime.go`:
- Around line 105-155: Update newPromptIterRuntime so every initialization
failure from addFixture, evaluation.New, and the backwarder, aggregator, or
optimizer constructors invokes the appropriate cleanup immediately before
returning. Return a cleanup function only after successful construction,
transferring resource ownership to the caller; preserve and propagate cleanup
errors together with the original failure using the existing error-combination
pattern.
- Line 190: The deterministicRunner.Run methods currently discard context
cancellation. Update both Run implementations to accept and check ctx.Err()
before doing work and throughout event publication, stop promptly without
recording costs or successful events, and return the cancellation error. Add a
regression test covering cancellation during evaluation/event emission.
🪄 Autofix (Beta)
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: Pro
Run ID: 17709a04-110e-451a-8972-85940e675299
📒 Files selected for processing (26)
evaluation/workflow/promptiter/regression/attribution.goevaluation/workflow/promptiter/regression/attribution_gate_test.goevaluation/workflow/promptiter/regression/compare.goevaluation/workflow/promptiter/regression/gate.goevaluation/workflow/promptiter/regression/pipeline.goevaluation/workflow/promptiter/regression/pipeline_test.goevaluation/workflow/promptiter/regression/promptiter.goevaluation/workflow/promptiter/regression/regression_test.goevaluation/workflow/promptiter/regression/report.goevaluation/workflow/promptiter/regression/report_test.goevaluation/workflow/promptiter/regression/result.goexamples/evaluation/promptiter_regression_loop/DESIGN.mdexamples/evaluation/promptiter_regression_loop/README.mdexamples/evaluation/promptiter_regression_loop/config.goexamples/evaluation/promptiter_regression_loop/config_test.goexamples/evaluation/promptiter_regression_loop/data/baseline_prompt.txtexamples/evaluation/promptiter_regression_loop/data/fake_engine.jsonexamples/evaluation/promptiter_regression_loop/data/metrics.jsonexamples/evaluation/promptiter_regression_loop/data/promptiter.jsonexamples/evaluation/promptiter_regression_loop/data/train.evalset.jsonexamples/evaluation/promptiter_regression_loop/data/validation.evalset.jsonexamples/evaluation/promptiter_regression_loop/main.goexamples/evaluation/promptiter_regression_loop/output/optimization_report.jsonexamples/evaluation/promptiter_regression_loop/output/optimization_report.mdexamples/evaluation/promptiter_regression_loop/runtime.goexamples/evaluation/promptiter_regression_loop/runtime_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- examples/evaluation/promptiter_regression_loop/data/baseline_prompt.txt
- examples/evaluation/promptiter_regression_loop/data/validation.evalset.json
- examples/evaluation/promptiter_regression_loop/data/fake_engine.json
- examples/evaluation/promptiter_regression_loop/data/metrics.json
- examples/evaluation/promptiter_regression_loop/data/train.evalset.json
| for _, name := range sortedKeys(baselineMetrics) { | ||
| before, after := baselineMetrics[name], candidateMetrics[name] | ||
| kind, scoreDelta, err := classifyDelta(before.Score, after.Score, before.Passed, after.Passed) | ||
| if err != nil { | ||
| return CaseDelta{}, fmt.Errorf("metric %q: %w", name, err) | ||
| } | ||
| if !before.Evaluated && !after.Evaluated { | ||
| kind = DeltaUnchanged | ||
| } | ||
| result.Metrics = append(result.Metrics, MetricDelta{ | ||
| Name: name, Kind: kind, BaselineScore: before.Score, CandidateScore: after.Score, | ||
| ScoreDelta: scoreDelta, BaselineEvaluated: before.Evaluated, CandidateEvaluated: after.Evaluated, | ||
| BaselinePassed: before.Passed, CandidatePassed: after.Passed, | ||
| }) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject metric-threshold drift before classifying deltas.
Threshold is recorded but never compared. The same score can become newly_passed solely because the candidate result uses a lower threshold, allowing a configuration change or stale eval-set snapshot to masquerade as prompt improvement.
Validate equal finite thresholds—or carry threshold changes explicitly—before calling classifyDelta. Add a same-score/different-threshold regression test.
As per path instructions, evaluation and protocol structures must remain behaviorally compatible.
中文
在分类增量前拒绝指标阈值漂移。
当前记录了 Threshold，但从未进行比较。候选结果仅降低阈值即可让相同分数变成 newly_passed，使配置变化或过期数据集快照伪装成提示词改进。
请在 classifyDelta 前验证有限且一致的阈值，或显式报告阈值变化，并增加对应回归测试。
🤖 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 `@evaluation/workflow/promptiter/regression/compare.go` around lines 121 - 134,
Before invoking classifyDelta in the metric comparison loop, validate that
baseline and candidate thresholds are finite and equal; reject the comparison
with an appropriate error when either threshold is non-finite or they differ.
Preserve evaluation and protocol structure compatibility, and add a regression
test covering identical scores with different thresholds.
Source: Path instructions
| func sameKeys[T any](kind string, left, right map[string]T) error { | ||
| leftKeys, rightKeys := sortedKeys(left), sortedKeys(right) | ||
| if fmt.Sprint(leftKeys) != fmt.Sprint(rightKeys) { | ||
| return fmt.Errorf("%s sets differ: %v and %v", kind, leftKeys, rightKeys) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Compare key sets structurally instead of formatting them.
fmt.Sprint can collapse different key sets to the same string—for example, ["a b", "c"] and ["a", "b c"]. Compare can then pair missing cases or metrics with zero values and emit an invalid delta.
Proposed fix
func sameKeys[T any](kind string, left, right map[string]T) error {
leftKeys, rightKeys := sortedKeys(left), sortedKeys(right)
- if fmt.Sprint(leftKeys) != fmt.Sprint(rightKeys) {
+ if len(left) != len(right) {
return fmt.Errorf("%s sets differ: %v and %v", kind, leftKeys, rightKeys)
}
+ for _, key := range leftKeys {
+ if _, ok := right[key]; !ok {
+ return fmt.Errorf("%s sets differ: %v and %v", kind, leftKeys, rightKeys)
+ }
+ }
return nil
}As per path instructions, semantic compatibility and cross-layer contracts are the first review priority.
中文
请按结构比较键集合，而不是比较格式化字符串。
fmt.Sprint 会让不同集合产生相同文本，例如 ["a b", "c"] 与 ["a", "b c"]。随后可能使用零值比较不存在的用例或指标，生成无效增量。
📝 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.
| func sameKeys[T any](kind string, left, right map[string]T) error { | |
| leftKeys, rightKeys := sortedKeys(left), sortedKeys(right) | |
| if fmt.Sprint(leftKeys) != fmt.Sprint(rightKeys) { | |
| return fmt.Errorf("%s sets differ: %v and %v", kind, leftKeys, rightKeys) | |
| } | |
| func sameKeys[T any](kind string, left, right map[string]T) error { | |
| leftKeys, rightKeys := sortedKeys(left), sortedKeys(right) | |
| if len(left) != len(right) { | |
| return fmt.Errorf("%s sets differ: %v and %v", kind, leftKeys, rightKeys) | |
| } | |
| for _, key := range leftKeys { | |
| if _, ok := right[key]; !ok { | |
| return fmt.Errorf("%s sets differ: %v and %v", kind, leftKeys, rightKeys) | |
| } | |
| } | |
| return nil | |
| } |
| func sameKeys[T any](kind string, left, right map[string]T) error { | |
| leftKeys, rightKeys := sortedKeys(left), sortedKeys(right) | |
| if fmt.Sprint(leftKeys) != fmt.Sprint(rightKeys) { | |
| return fmt.Errorf("%s sets differ: %v and %v", kind, leftKeys, rightKeys) | |
| } | |
| func sameKeys[T any](kind string, left, right map[string]T) error { | |
| leftKeys, rightKeys := sortedKeys(left), sortedKeys(right) | |
| if len(left) != len(right) { | |
| return fmt.Errorf("%s sets differ: %v and %v", kind, leftKeys, rightKeys) | |
| } | |
| for _, key := range leftKeys { | |
| if _, ok := right[key]; !ok { | |
| return fmt.Errorf("%s sets differ: %v and %v", kind, leftKeys, rightKeys) | |
| } | |
| } | |
| return nil | |
| } |
🤖 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 `@evaluation/workflow/promptiter/regression/compare.go` around lines 186 - 190,
Update sameKeys to compare the sorted key slices structurally rather than
converting them with fmt.Sprint. Preserve the existing error message and return
behavior when the key sets differ, while ensuring distinct keys containing
spaces cannot compare equal.
Source: Path instructions
| if _, err := addCosts(round.ServingCost, round.OptimizationCost); err != nil { | ||
| return nil, fmt.Errorf("round %d cost: %w", round.Number, err) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate the reported total cost.
Only per-round costs are validated. A negative run.TotalCost is copied into the audit report unchanged, allowing an invalid final cost despite all rounds passing validation. Validate it before constructing the report and add a negative-total-cost regression case.
中文
请校验报告总成本。
当前仅校验每轮成本；负数 run.TotalCost 会被直接写入审计报告，导致各轮校验均通过但最终成本无效。请在构建报告前校验该字段，并补充负总成本回归测试。
Also applies to: 209-209
🤖 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 `@evaluation/workflow/promptiter/regression/report.go` around lines 175 - 176,
Validate run.TotalCost for negative values before constructing the audit report,
alongside the existing per-round cost validation, and return an appropriate
error when invalid. Add a regression test covering a negative total cost while
all round costs remain valid.
| func WriteMarkdown(writer io.Writer, report *OptimizationReport) error { | ||
| if writer == nil || report == nil { | ||
| return errors.New("Markdown writer and report are required") | ||
| } | ||
| var output strings.Builder | ||
| fmt.Fprintf(&output, "# Prompt Optimization Report\n\n") | ||
| fmt.Fprintf(&output, "- Status: **%s**\n", report.Status) | ||
| fmt.Fprintf(&output, "- Decision: %s\n", report.Decision.StopReason) | ||
| fmt.Fprintf(&output, "- Seed: %d\n", report.Audit.Seed) | ||
| fmt.Fprintf(&output, "- Runtime: %s", report.Audit.Runtime.Mode) | ||
| if report.Audit.Runtime.Model != "" { | ||
| fmt.Fprintf(&output, " (%s)", report.Audit.Runtime.Model) | ||
| } | ||
| fmt.Fprintln(&output) | ||
| fmt.Fprintln(&output) | ||
|
|
||
| fmt.Fprintln(&output, "## Baseline") | ||
| fmt.Fprintln(&output) | ||
| fmt.Fprintln(&output, "| Dataset | Score | Passed |") | ||
| fmt.Fprintln(&output, "| --- | ---: | :---: |") | ||
| markdownScoreRow(&output, "Train", report.BaselineTrain) | ||
| markdownScoreRow(&output, "Validation", report.BaselineValidation) | ||
| fmt.Fprintln(&output, "\n### Train failures") | ||
| writeAttribution(&output, report.BaselineTrainAttribution) | ||
| fmt.Fprintln(&output, "\n### Validation failures") | ||
| writeAttribution(&output, report.BaselineValidationAttribution) | ||
|
|
||
| fmt.Fprintln(&output, "\n## Optimization Rounds") | ||
| if len(report.Rounds) == 0 { | ||
| fmt.Fprintln(&output, "\nNo candidate was generated.") | ||
| } | ||
| for _, round := range report.Rounds { | ||
| fmt.Fprintf(&output, "\n### Round %d\n\n", round.Number) | ||
| fmt.Fprintf(&output, "- Gate: **%s**\n", acceptedText(round.Gate != nil && round.Gate.Accepted)) | ||
| fmt.Fprintln(&output, "- Candidate prompt:") | ||
| fmt.Fprintf(&output, "\n```text\n%s\n```\n", markdownCodeBlock(round.CandidatePrompt)) | ||
| if round.Gate != nil { | ||
| for _, reason := range round.Gate.Reasons { | ||
| fmt.Fprintf(&output, " - %s\n", reason) | ||
| } | ||
| } | ||
| fmt.Fprintf(&output, "- Train score: %.6f (%+.6f)\n", round.Train.Score, round.TrainDelta.ScoreDelta) | ||
| fmt.Fprintf(&output, "- Validation score: %.6f (%+.6f)\n", round.Validation.Score, round.ValidationDelta.ScoreDelta) | ||
| fmt.Fprintf(&output, "- Evaluation cost: %s\n", costText(round.ServingCost)) | ||
| fmt.Fprintf(&output, "- Optimization cost: %s\n", costText(round.OptimizationCost)) | ||
| roundCost, _ := addCosts(round.ServingCost, round.OptimizationCost) | ||
| fmt.Fprintf(&output, "- Round total: %s\n", costText(roundCost)) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Return an error instead of panicking on incomplete reports.
WriteMarkdown accepts any non-nil OptimizationReport, then dereferences baseline, round evaluation, and delta pointers. For example, WriteMarkdown(&buf, &OptimizationReport{}) panics at Line 299 instead of returning an error. Validate required nested fields and round costs before rendering.
中文
报告不完整时应返回错误，而不是触发 panic。
WriteMarkdown 接受任意非 nil 的 OptimizationReport，随后直接解引用基线、轮次评估和增量字段。例如，WriteMarkdown(&buf, &OptimizationReport{}) 会在第 299 行 panic，而不是返回错误。请在渲染前校验必需的嵌套字段及轮次成本。
🤖 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 `@evaluation/workflow/promptiter/regression/report.go` around lines 279 - 325,
Update WriteMarkdown to validate all required nested report fields before
rendering, including baseline evaluations, each round’s train/validation
evaluations and deltas, and serving/optimization costs; return a descriptive
error for any missing or incomplete field instead of dereferencing it. Keep the
existing rendering behavior unchanged for complete reports.
| func addCaseCost(cost *Cost, evalCase *evaluation.EvaluationCaseResult) { | ||
| traceFound := false | ||
| for _, detail := range evalCase.RunDetails { | ||
| if detail == nil || detail.Inference == nil { | ||
| continue | ||
| } | ||
| for _, executionTrace := range detail.Inference.ExecutionTraces { | ||
| if executionTrace == nil { | ||
| continue | ||
| } | ||
| traceFound = true | ||
| addTraceCost(cost, executionTrace) | ||
| } | ||
| } | ||
| if traceFound { | ||
| return | ||
| } | ||
| for _, run := range evalCase.EvalCaseResults { | ||
| if run == nil { | ||
| continue | ||
| } | ||
| for _, invocation := range run.EvalMetricResultPerInvocation { | ||
| if invocation != nil && invocation.ActualInvocation != nil && invocation.ActualInvocation.ExecutionTrace != nil { | ||
| addTraceCost(cost, invocation.ActualInvocation.ExecutionTrace) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Apply invocation-trace fallback per run, not per case.
Once any RunDetails trace is found, the function skips all invocation traces. A case containing one traced run and one run with only ActualInvocation.ExecutionTrace therefore underreports tokens and latency, weakening cost gates and audit totals.
Proposed fix
func addCaseCost(cost *Cost, evalCase *evaluation.EvaluationCaseResult) {
- traceFound := false
+ tracedRuns := make(map[int]bool)
for _, detail := range evalCase.RunDetails {
if detail == nil || detail.Inference == nil {
continue
}
+ runTraceFound := false
for _, executionTrace := range detail.Inference.ExecutionTraces {
if executionTrace == nil {
continue
}
- traceFound = true
+ runTraceFound = true
addTraceCost(cost, executionTrace)
}
- }
- if traceFound {
- return
+ if runTraceFound {
+ tracedRuns[detail.RunID] = true
+ }
}
for _, run := range evalCase.EvalCaseResults {
- if run == nil {
+ if run == nil || tracedRuns[run.RunID] {
continue
}Add a regression test containing both trace sources in separate runs. As per path instructions, prioritize resource boundaries and reliable framework behavior.
中文
应按运行维度回退到调用追踪，而不是按整个用例处理。
只要任意 RunDetails 存在追踪，当前实现就会跳过所有调用级追踪。混合来源的用例会漏算 token 和延迟，从而削弱成本门禁及审计数据的准确性。
请记录已由 RunDetails 覆盖的运行，仅对未覆盖运行使用调用级追踪，并增加混合来源回归测试。
📝 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.
| func addCaseCost(cost *Cost, evalCase *evaluation.EvaluationCaseResult) { | |
| traceFound := false | |
| for _, detail := range evalCase.RunDetails { | |
| if detail == nil || detail.Inference == nil { | |
| continue | |
| } | |
| for _, executionTrace := range detail.Inference.ExecutionTraces { | |
| if executionTrace == nil { | |
| continue | |
| } | |
| traceFound = true | |
| addTraceCost(cost, executionTrace) | |
| } | |
| } | |
| if traceFound { | |
| return | |
| } | |
| for _, run := range evalCase.EvalCaseResults { | |
| if run == nil { | |
| continue | |
| } | |
| for _, invocation := range run.EvalMetricResultPerInvocation { | |
| if invocation != nil && invocation.ActualInvocation != nil && invocation.ActualInvocation.ExecutionTrace != nil { | |
| addTraceCost(cost, invocation.ActualInvocation.ExecutionTrace) | |
| } | |
| } | |
| } | |
| func addCaseCost(cost *Cost, evalCase *evaluation.EvaluationCaseResult) { | |
| tracedRuns := make(map[int]bool) | |
| for _, detail := range evalCase.RunDetails { | |
| if detail == nil || detail.Inference == nil { | |
| continue | |
| } | |
| runTraceFound := false | |
| for _, executionTrace := range detail.Inference.ExecutionTraces { | |
| if executionTrace == nil { | |
| continue | |
| } | |
| runTraceFound = true | |
| addTraceCost(cost, executionTrace) | |
| } | |
| if runTraceFound { | |
| tracedRuns[detail.RunID] = true | |
| } | |
| } | |
| for _, run := range evalCase.EvalCaseResults { | |
| if run == nil || tracedRuns[run.RunID] { | |
| continue | |
| } | |
| for _, invocation := range run.EvalMetricResultPerInvocation { | |
| if invocation != nil && invocation.ActualInvocation != nil && invocation.ActualInvocation.ExecutionTrace != nil { | |
| addTraceCost(cost, invocation.ActualInvocation.ExecutionTrace) | |
| } | |
| } | |
| } | |
| } |
🤖 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 `@evaluation/workflow/promptiter/regression/result.go` around lines 446 - 472,
Update addCaseCost to track which individual runs are covered by RunDetails
execution traces, rather than using the case-wide traceFound flag. For each
EvalCaseResults run without a covered RunDetails trace, apply the existing
ActualInvocation.ExecutionTrace fallback, while preventing duplicate cost
counting for runs already covered; add a regression test with the two trace
sources in separate runs.
Source: Path instructions
| func parseOptions(arguments []string, errorOutput io.Writer) (commandOptions, error) { | ||
| options := commandOptions{paths: defaultPaths()} | ||
| flags := flag.NewFlagSet("promptiter_regression_loop", flag.ContinueOnError) | ||
| flags.SetOutput(errorOutput) | ||
| flags.StringVar(&options.paths.baseline, "baseline-prompt", options.paths.baseline, "baseline prompt file") | ||
| flags.StringVar(&options.paths.train, "train-evalset", options.paths.train, "training evalset JSON") | ||
| flags.StringVar(&options.paths.validation, "validation-evalset", options.paths.validation, "validation evalset JSON") | ||
| flags.StringVar(&options.paths.metrics, "metrics", options.paths.metrics, "metrics JSON") | ||
| flags.StringVar(&options.paths.promptiter, "promptiter-config", options.paths.promptiter, "PromptIter config JSON") | ||
| flags.StringVar(&options.paths.fakeEngine, "fake-engine-config", options.paths.fakeEngine, "fake engine config JSON") | ||
| flags.StringVar(&options.outputDir, "output", defaultOutputDir, "report output directory") | ||
| flags.Int64Var(&options.seed, "seed", 2003, "deterministic seed") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Implement the advertised real runtime mode or narrow the PR contract.
The old behavior had no example; the new CLI supports only fake execution: it has no runtime/model options, always calls runFakeLoop, and hardcodes Mode: "fake". Users cannot run the model-backed workflow promised by the PR objective, and replacing only PromptIter stage runners as README Line 51 suggests would still leave evaluation deterministic. Add an optional real mode while preserving fake as the default, with tests and documentation for both.
中文
请实现所声明的真实运行模式，或收窄 PR 的功能契约。
旧版本没有此示例；当前新增 CLI 仅支持 fake 执行：没有 runtime/model 参数，始终调用 runFakeLoop，并硬编码 Mode: "fake"。因此用户无法运行 PR 目标中声明的模型驱动流程；而 README 第 51 行仅替换 PromptIter stage runner，也仍会保留确定性的 evaluation runner。请在保持 fake 为默认模式的前提下增加可选 real 模式，并补充两种模式的测试与文档。
As per path instructions, intentional behavior changes must identify old/new behavior, affected users, and required test or documentation updates.
Also applies to: 66-83
🤖 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 `@examples/evaluation/promptiter_regression_loop/main.go` around lines 45 - 56,
Extend parseOptions and the command execution flow to support an optional real
runtime/model-backed mode while preserving fake execution as the default. Add
the required runtime/model configuration, select the appropriate workflow
instead of always calling runFakeLoop, and report the selected mode rather than
hardcoding "fake"; update tests to cover both modes and document their usage and
defaults.
Source: Path instructions
| jsonFile, err := openReport(filepath.Join(options.outputDir, "optimization_report.json")) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| jsonErr := regression.WriteJSON(jsonFile, report) | ||
| closeErr := jsonFile.Close() | ||
| if jsonErr != nil || closeErr != nil { | ||
| return nil, errors.Join(jsonErr, closeErr) | ||
| } | ||
| markdownFile, err := openReport(filepath.Join(options.outputDir, "optimization_report.md")) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| markdownErr := regression.WriteMarkdown(markdownFile, report) | ||
| closeErr = markdownFile.Close() | ||
| if markdownErr != nil || closeErr != nil { | ||
| return nil, errors.Join(markdownErr, closeErr) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Publish the JSON and Markdown reports atomically as one audit generation.
JSON is replaced before Markdown is opened. If the second open, write, or close fails, the command returns an error but leaves mismatched or truncated audit artifacts. Write both to temporary files, close them successfully, then rename them into place together.
中文
请将 JSON 和 Markdown 报告作为同一代审计产物原子发布。
当前 JSON 会在 Markdown 打开前被覆盖。如果第二个文件打开、写入或关闭失败，命令虽然返回错误，却会留下版本不一致或被截断的审计文件。请先写入临时文件并成功关闭，再统一替换正式文件。
As per path instructions, report outputs must accurately represent the current run and remain safely reproducible.
🤖 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 `@examples/evaluation/promptiter_regression_loop/main.go` around lines 92 -
109, Update the report-writing flow around openReport, regression.WriteJSON, and
regression.WriteMarkdown to stage both outputs in temporary files within the
output directory, close and validate both successfully, then publish both final
paths together via coordinated renames. On any open, write, close, or publish
failure, clean up temporary files and avoid leaving partially updated or
mismatched optimization reports.
Source: Path instructions
| func newPromptIterRuntime( | ||
| ctx context.Context, config loopConfig, candidate string, | ||
| ) (promptiterengine.Engine, *costCounter, func() error, error) { | ||
| evalSetManager := evalsetinmemory.New() | ||
| metricManager := metricinmemory.New() | ||
| evalResultManager := evalresultinmemory.New() | ||
| closeManagers := func() error { | ||
| return errors.Join(evalSetManager.Close(), metricManager.Close(), evalResultManager.Close()) | ||
| } | ||
| for _, fixture := range []*evalset.EvalSet{config.train, config.validation} { | ||
| if err := addFixture(ctx, evalSetManager, metricManager, fixture, config); err != nil { | ||
| return nil, nil, closeManagers, err | ||
| } | ||
| } | ||
| counter := &costCounter{engine: config.engine} | ||
| evaluator, err := evaluation.New(appName, &deterministicRunner{prompt: config.baseline, counter: counter}, | ||
| evaluation.WithEvalSetManager(evalSetManager), evaluation.WithMetricManager(metricManager), | ||
| evaluation.WithEvalResultManager(evalResultManager), evaluation.WithRegistry(registry.New()), evaluation.WithNumRuns(1)) | ||
| if err != nil { | ||
| return nil, nil, closeManagers, err | ||
| } | ||
| closeRuntime := func() error { return errors.Join(evaluator.Close(), closeManagers()) } | ||
| backwardRunner := &jsonRunner{counter: counter, output: fmt.Sprintf( | ||
| `{"Gradients":[{"SurfaceID":%q,"Severity":"P1","Gradient":"fix failed response"}],"Upstream":[]}`, targetSurfaceID)} | ||
| aggregatorRunner := &jsonRunner{counter: counter, output: `{"Gradients":[{"Severity":"P1","Gradient":"fix failed response"}]}`} | ||
| candidateJSON, _ := json.Marshal(candidate) | ||
| optimizerRunner := &jsonRunner{counter: counter, output: fmt.Sprintf( | ||
| `{"Value":{"Text":%s},"Reason":"apply failure hints"}`, candidateJSON)} | ||
| backwarderInstance, err := backwarder.New(ctx, backwardRunner) | ||
| if err != nil { | ||
| return nil, nil, closeRuntime, err | ||
| } | ||
| aggregatorInstance, err := aggregator.New(ctx, aggregatorRunner) | ||
| if err != nil { | ||
| return nil, nil, closeRuntime, err | ||
| } | ||
| optimizerInstance, err := optimizer.New(ctx, optimizerRunner) | ||
| if err != nil { | ||
| return nil, nil, closeRuntime, err | ||
| } | ||
| baseline := config.baseline | ||
| structure := &astructure.Snapshot{ | ||
| StructureID: "fixture-agent", EntryNodeID: "answer", | ||
| Nodes: []astructure.Node{{NodeID: "answer", Kind: astructure.NodeKindLLM, Name: agentName}}, | ||
| Surfaces: []astructure.Surface{{SurfaceID: targetSurfaceID, NodeID: "answer", Type: astructure.SurfaceTypeInstruction, | ||
| Value: astructure.SurfaceValue{Text: &baseline}}}, | ||
| } | ||
| engine, err := promptiterengine.New(ctx, promptiterengine.WithStructure(structure), | ||
| promptiterengine.WithAgentEvaluator(evaluator), promptiterengine.WithBackwarder(backwarderInstance), | ||
| promptiterengine.WithAggregator(aggregatorInstance), promptiterengine.WithOptimizer(optimizerInstance)) | ||
| return engine, counter, closeRuntime, err |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Close partially initialized runtime resources before returning errors.
Failures from addFixture, evaluation.New, or the PromptIter component constructors return a cleanup function without invoking it. The caller immediately returns on err, so managers and any initialized evaluator leak. Make this constructor own cleanup on every error path; return resource ownership only on success.
中文
初始化失败时请立即关闭已创建的 runtime 资源。
addFixture、evaluation.New 或 PromptIter 组件构造失败时，函数仅返回 cleanup，却没有执行它；调用方遇到 err 会直接返回，因此 manager 和已初始化的 evaluator 会泄漏。请让构造函数负责清理所有失败路径，仅在成功时转移资源所有权。
As per path instructions, framework execution paths must enforce resource boundaries and preserve cleanup errors.
🤖 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 `@examples/evaluation/promptiter_regression_loop/runtime.go` around lines 105 -
155, Update newPromptIterRuntime so every initialization failure from
addFixture, evaluation.New, and the backwarder, aggregator, or optimizer
constructors invokes the appropriate cleanup immediately before returning.
Return a cleanup function only after successful construction, transferring
resource ownership to the caller; preserve and propagate cleanup errors together
with the original failure using the existing error-combination pattern.
Source: Path instructions
| counter *costCounter | ||
| } | ||
|
|
||
| func (r *deterministicRunner) Run(_ context.Context, _, _ string, message model.Message, opts ...agent.RunOption) (<-chan *event.Event, error) { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Honor cancellation in both deterministic runners.
Both Run methods discard context.Context, so canceled evaluations still record costs and emit successful events. Check ctx.Err() before work and while publishing events, return the cancellation cause, and add a cancellation regression test.
中文
两个确定性 runner 都应响应取消信号。
两个 Run 方法都忽略了 context.Context，导致 evaluation 被取消后仍会记录成本并发送成功事件。请在执行前及发送事件期间检查 ctx.Err()，返回取消原因，并补充取消场景的回归测试。
As per path instructions, execution paths must preserve cancellation propagation and stop promptly.
Also applies to: 248-248
🤖 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 `@examples/evaluation/promptiter_regression_loop/runtime.go` at line 190, The
deterministicRunner.Run methods currently discard context cancellation. Update
both Run implementations to accept and check ctx.Err() before doing work and
throughout event publication, stop promptly without recording costs or
successful events, and return the cancellation error. Add a regression test
covering cancellation during evaluation/event emission.
Source: Path instructions
Rememorio
left a comment
Rememorio
left a comment
There was a problem hiding this comment.
I reviewed the changed lines and left 5 inline comments below. The comments focus on issues worth addressing before merge.
Fresh review found two likely correctness issues in the regression workflow and three moderate contract/audit issues. The most important validations are a real Evaluation Service inference failure with no metric results and a multi-surface PromptIter profile. Multi-round budget accounting, metric-specific loss reasons, and asymmetric fake-runtime costs also need targeted tests.
中文
我看过这次变更的相关代码，在下面留下 5 条行内评论。评论聚焦在合并前值得处理的问题。
本次审查发现两个较明确的回归流程正确性问题，以及三个中等程度的契约或审计问题。最重要的验证场景是：Evaluation Service 推理失败且没有指标结果，以及包含多个 surface override 的 PromptIter profile。此外还应补充多轮预算累计、指标级失败原因和非对称 fake runtime 成本的定向测试。
| runRequest.MaxRounds = 1 | ||
| runRequest.TargetSurfaceIDs = []string{targetSurfaceID} | ||
| text := request.Prompt | ||
| runRequest.InitialProfile = &promptiter.Profile{ |
There was a problem hiding this comment.
P1: Keep non-target overrides in the initial profile
Replacing base.InitialProfile here discards every override except the target prompt surface. In a multi-surface agent, PromptIter therefore evaluates and optimizes against structure defaults for the other surfaces, rather than the caller's current profile; the returned prompt can pass under the wrong configuration and fail when applied to the real profile.
Clone the base profile, replace or add only targetSurfaceID, and preserve all other overrides without mutating the caller's value. A test with a second overridden surface should verify that the engine receives it unchanged.
中文
保留初始 profile 中的非目标 override
这里直接替换 base.InitialProfile，会丢弃目标 prompt surface 之外的所有 override。对于多 surface agent，PromptIter 将基于其他 surface 的结构默认值进行评测和优化，而不是基于调用方当前使用的 profile；这样生成的 prompt 可能在错误配置下通过，应用到真实 profile 后却失败。
建议复制基础 profile，只替换或新增 targetSurfaceID，并在不修改调用方对象的前提下保留其他 override。应增加一个包含第二个 override surface 的测试，确认传给 engine 的值保持不变。
There was a problem hiding this comment.
Fixed in f88d8f9. The target override is now updated on a deep-cloned profile, preserving all other surfaces without mutating the caller.
Added a multi-surface test.
| count++ | ||
| allPassed = allPassed && item.Passed | ||
| } | ||
| if count == 0 { |
There was a problem hiding this comment.
P1: Preserve execution failures that have no metric results
A normal Evaluation Service inference failure produces a failed case with ErrorMessage but no MetricResults. This branch rejects that result, so the regression loop exits before FailureExecutionError attribution, gating, or audit reporting can run.
Please represent execution-only failures explicitly, or carry the configured metric identities into summarization so they can receive deterministic failed results. A regression test should use the Evaluation Service's real failed-case shape rather than a synthetic failed trace attached to a passing metric.
中文
保留没有指标结果的执行失败
Evaluation Service 在推理失败时会正常返回带有 ErrorMessage、但没有 MetricResults 的失败 case。这里会直接拒绝该结果，导致回归流程在 FailureExecutionError 归因、门禁和审计报告生成之前就终止。
建议显式表示仅包含执行错误的失败结果，或者将配置中的指标标识传入摘要阶段，为这些指标生成确定性的失败状态。回归测试应使用 Evaluation Service 实际返回的失败 case 结构，而不是在一个通过的指标旁人为附加失败 trace。
There was a problem hiding this comment.
Fixed in f88d8f9. Execution-only failures now retain metric identities for attribution and audit. Baseline failures stop before optimization; candidate failures are gated and rejected.
Added train and validation pipeline tests.
| for _, metricName := range failure.MetricNames { | ||
| hints = append(hints, FailureHint{ | ||
| CaseID: failure.CaseID, MetricName: metricName, Category: failure.Category, | ||
| Reason: truncate(failure.Reason, 512), |
There was a problem hiding this comment.
P2: Preserve metric-specific reasons in loss hints
Every failed metric in a case receives the same case-level failure.Reason. When two metrics fail for different reasons, metricReasonText selects one reason and this loop attaches it to both metric-keyed PromptIter hints, so optimization feedback for at least one metric is incorrect.
Keep each failed metric's evaluator reason when available, falling back to the primary case cause only for shared execution, route, or tool failures. A two-metric fixture with distinct reasons would close this gap.
中文
保留每个指标各自的失败原因
同一个 case 中的所有失败指标都会收到同一条 case 级 failure.Reason。当两个指标因不同原因失败时，metricReasonText 只会选中其中一个原因，而这里又把它附到两个以指标为键的 PromptIter hint 上，至少有一个指标会收到错误的优化反馈。
建议优先保留每个失败指标自身的 evaluator reason，仅在执行、路由或工具调用等共享根因场景下回退到 case 级原因。可以通过一个包含两个不同失败原因的指标 fixture 验证该行为。
There was a problem hiding this comment.
Fixed in 73c8edc. PromptIter hints now preserve each metric’s evaluator reason, with shared failure reasons used only for execution, route, and tool failures. Added a two-metric test.
| if err != nil { | ||
| return nil, fmt.Errorf("gate cost round %d: %w", roundNumber, err) | ||
| } | ||
| decision, err := Gate(request.GatePolicy, validationDelta, gateCost) |
There was a problem hiding this comment.
P2: Define and enforce the budget scope across rounds
The gate sees only the current round's evaluation and generation cost; baseline work and earlier rejected rounds already recorded in run.TotalCost are excluded. The checked-in example consequently accepts a run totaling 114 calls and 570 tokens despite configuring maxima of 40 calls and 200 tokens.
If these fields are run budgets, pass the projected cumulative cost to Gate. If they are intentionally per-round acceptance limits, name and document that scope explicitly and provide a separate cumulative cap; cover the selected contract with a multi-round test.
中文
明确并落实跨轮次预算范围
门禁目前只检查当前轮的评测和候选生成成本，已经计入 run.TotalCost 的 baseline 和此前被拒绝轮次都没有参与预算判断。因此，示例配置的上限虽然是 40 次调用和 200 tokens，最终仍接受了总计 114 次调用、570 tokens 的运行结果。
如果这些字段表示整次运行的预算，应将预计累计成本传给 Gate。如果它们有意表示单轮接受上限，则应在命名和文档中明确这一范围，并另行提供累计预算；无论选择哪种契约，都应增加多轮测试。
There was a problem hiding this comment.
Fixed in 73c8edc. Gate now uses cumulative run cost, including baseline and prior rejected rounds. The contract is documented and covered by a multi-round test.
| func (c *costCounter) snapshot() regression.Cost { | ||
| c.mu.Lock() | ||
| defer c.mu.Unlock() | ||
| return regression.Cost{ModelCalls: c.calls, Tokens: int64(c.calls * (c.engine.PromptTokens + c.engine.CompletionTokens)), LatencyMS: int64(c.calls) * c.engine.LatencyMS} |
There was a problem hiding this comment.
P2: Use the configured optimizer cost values
The counter derives tokens and latency for every call from PromptTokens, CompletionTokens, and LatencyMS. OptimizerTokens and OptimizerLatencyMS are parsed and validated but never used, so changing those documented fake-engine fields has no effect on optimization_cost or the audit report.
Track optimizer-stage usage separately, or remove the unused configuration fields. A test with optimizer values different from the serving values should assert the reported optimization cost.
中文
使用配置中的优化器成本参数
当前计数器对所有调用都使用 PromptTokens、CompletionTokens 和 LatencyMS 计算 tokens 与延迟。OptimizerTokens 和 OptimizerLatencyMS 虽然会被解析和校验，却从未参与计算，因此修改这些 fake engine 配置不会影响 optimization_cost 或审计报告。
建议单独记录优化阶段的成本，或者删除这些没有生效的配置字段。测试中应让优化器成本与 serving 成本明显不同，并断言报告中的优化成本确实采用了对应配置。
There was a problem hiding this comment.
Fixed in dd6503e. Optimizer-stage calls now use the configured optimizer token and latency values, while evaluation calls retain serving costs. Added an asymmetric end-to-end cost assertion.
|
Thanks for the review. I’ll address the two correctness issues first, then work through the contract/audit concerns. I’ll reply to each inline comment with the corresponding fix or explanation once the changes are ready. |
|
All five review comments have been addressed in the latest commits. I’ve added targeted tests for the reported cases, and the relevant test suites are passing. The PR is ready for another review when convenient. Thanks again for the detailed feedback. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
examples/evaluation/promptiter_regression_loop/output/optimization_report.md (1)
65-79: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftInclude training case and metric deltas in the audit report.
Round 2 and Round 3 show training scores improving to
1.000000, but their case/metric tables only report validation changes. This fails the required dataset-, case-, and metric-level comparison and prevents the report from explaining the training gains. Add training case and metric delta rows for each round.As per path instructions, Markdown documentation must accurately describe the current public contract.
中文
在审计报告中加入训练集的案例和指标差异。
第 2 轮和第 3 轮的训练集分数提升到了
1.000000，但案例/指标表只报告了验证集变化。这不满足按数据集、案例和指标进行比较的要求，也无法解释训练集分数为何提升。请为每一轮补充训练集案例和指标差异行。根据路径说明，Markdown 文档必须准确描述当前公开契约。
Also applies to: 95-111
🤖 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 `@examples/evaluation/promptiter_regression_loop/output/optimization_report.md` around lines 65 - 79, Update the audit report sections for every affected round, including rounds 2 and 3, so the case and metric delta tables contain training-dataset rows in addition to validation rows. Use the actual training case and metric baseline/candidate values and deltas, ensuring the Markdown accurately documents dataset-, case-, and metric-level comparisons.Source: Path instructions
🤖 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
`@examples/evaluation/promptiter_regression_loop/output/optimization_report.md`:
- Around line 43-44: Normalize the indentation of the round-metadata Markdown
bullets surrounding the “Optimization cost” and “Round total” entries, ensuring
both affected sections use one consistent nesting level. Then rerun markdownlint
and resolve any remaining MD005 violations.
---
Outside diff comments:
In
`@examples/evaluation/promptiter_regression_loop/output/optimization_report.md`:
- Around line 65-79: Update the audit report sections for every affected round,
including rounds 2 and 3, so the case and metric delta tables contain
training-dataset rows in addition to validation rows. Use the actual training
case and metric baseline/candidate values and deltas, ensuring the Markdown
accurately documents dataset-, case-, and metric-level comparisons.
🪄 Autofix (Beta)
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: Pro
Run ID: 1b5aa1d9-7e81-41bd-8902-c99c5ddc0546
📒 Files selected for processing (13)
evaluation/workflow/promptiter/regression/attribution.goevaluation/workflow/promptiter/regression/attribution_gate_test.goevaluation/workflow/promptiter/regression/gate.goevaluation/workflow/promptiter/regression/pipeline.goevaluation/workflow/promptiter/regression/pipeline_test.goexamples/evaluation/promptiter_regression_loop/DESIGN.mdexamples/evaluation/promptiter_regression_loop/README.mdexamples/evaluation/promptiter_regression_loop/data/fake_engine.jsonexamples/evaluation/promptiter_regression_loop/data/promptiter.jsonexamples/evaluation/promptiter_regression_loop/output/optimization_report.jsonexamples/evaluation/promptiter_regression_loop/output/optimization_report.mdexamples/evaluation/promptiter_regression_loop/runtime.goexamples/evaluation/promptiter_regression_loop/runtime_test.go
🚧 Files skipped from review as they are similar to previous changes (11)
- examples/evaluation/promptiter_regression_loop/data/fake_engine.json
- examples/evaluation/promptiter_regression_loop/runtime_test.go
- evaluation/workflow/promptiter/regression/gate.go
- examples/evaluation/promptiter_regression_loop/DESIGN.md
- examples/evaluation/promptiter_regression_loop/README.md
- evaluation/workflow/promptiter/regression/attribution_gate_test.go
- examples/evaluation/promptiter_regression_loop/runtime.go
- examples/evaluation/promptiter_regression_loop/output/optimization_report.json
- evaluation/workflow/promptiter/regression/pipeline.go
- evaluation/workflow/promptiter/regression/attribution.go
- evaluation/workflow/promptiter/regression/pipeline_test.go
| - Optimization cost: 22 calls, 142 tokens, 64 ms | ||
| - Round total: 34 calls, 202 tokens, 88 ms |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix inconsistent Markdown list indentation.
markdownlint reports MD005 on Lines 43-44 and 68-69. Normalize the surrounding round-metadata bullets to one consistent nesting level and rerun the Markdown linter.
中文
修复不一致的 Markdown 列表缩进。
markdownlint 在第 43-44 行和第 68-69 行报告了 MD005。请统一相关轮次元数据列表项的嵌套级别，并重新运行 Markdown linter。
Also applies to: 68-69
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 43-43: Inconsistent indentation for list items at the same level
Expected: 2; Actual: 0
(MD005, list-indent)
[warning] 44-44: Inconsistent indentation for list items at the same level
Expected: 2; Actual: 0
(MD005, list-indent)
🤖 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 `@examples/evaluation/promptiter_regression_loop/output/optimization_report.md`
around lines 43 - 44, Normalize the indentation of the round-metadata Markdown
bullets surrounding the “Optimization cost” and “Round total” entries, ensuring
both affected sections use one consistent nesting level. Then rerun markdownlint
and resolve any remaining MD005 violations.
Sources: Path instructions, Linters/SAST tools
What changed
Adds a reproducible Evaluation + PromptIter regression loop:
The pipeline summarizes Evaluation Service results, attributes each failed case,
and converts failed case/metric reasons into PromptIter loss hints. Every
candidate is rerun against separate training and validation evalsets and compared
with the last accepted prompt at dataset, case, and metric levels.
A candidate is accepted only when validation gain reaches the configured
minimum, no new hard failure is introduced, critical cases and metrics stay
within regression limits, and model-call/token budgets are respected. Rejected
candidates are not promoted, so training gains accompanied by validation
regression are rejected as overfitting. The pipeline only recommends write-back;
it never modifies the source prompt.
Deterministic example
examples/evaluation/promptiter_regression_loopcontains three training andthree validation cases using real Evaluation Service final-response and
tool-trajectory metrics.
The example instantiates the existing PromptIter engine and runs its evaluation,
loss, backward, aggregation, optimizer, profile-patch, and validation stages.
Deterministic stage runners require no API key and reproduce three rounds:
Run from
examples/evaluation:The generated JSON and Markdown reports include prompts, baseline scores and
failure evidence, changed case/metric deltas, gate reasons, per-round cost,
seed, runtime/input hashes, and the final write-back recommendation.
Testing
This revision reduces the previous 49-file, 14.6k-line version to 26 files and
about 5.6k lines, removes the unrelated session fix, and organizes the work into
five reviewable commits.
Closes #2003