docs and test case improvements - #5965
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR expands release validation with shared CLI version matrices, retries, artifacts, and R2 reports. It extends provider and CLI E2E harnesses with chained-request handling, multi-turn scenarios, OpenCode Responses coverage, progress reporting, and structured HTML reports. It also updates changelogs and integration configurations. ChangesRelease validation and publication
Provider and CLI harnesses
Release content and integration configuration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ReleaseWorkflow
participant ResolveCliVersions
participant CliHarness
participant ProviderHarness
participant R2
ReleaseWorkflow->>ResolveCliVersions: resolve shared stable CLI versions
ResolveCliVersions->>CliHarness: pass version matrix
CliHarness->>ProviderHarness: run CLI and provider validation
ProviderHarness->>ProviderHarness: retry failed items and archive attempts
CliHarness->>R2: upload per-leg reports
ProviderHarness->>R2: upload provider reports
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 1 | ❌ 4❌ Failed checks (4 warnings)
✅ Passed checks (1 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
dce92dd to
6819533
Compare
6819533 to
c44b750
Compare
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/e2e/clis/clis_test.go (1)
188-199: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInstall the fallback recorder before the first
t.Fatalf.
progress.registerruns at lines 144 and 152, beforerunCell. The guard at lines 189-191 callst.Fatalfbefore the deferred recorder is installed at line 199. That path leaves the cell registered and never recorded, which produces the permanent TOTAL/DONE gap the defer was added to prevent. Move the deferred block above the guard.🐛 Proposed fix
func runCell(t *testing.T, cli CLI, prov Provider, model ModelInfo, sc scenario, effort string, baseURL, apiKey string) { - if effort != "" && len(sc.Turns) > 1 { - t.Fatalf("scenario %q: effort-level testing is only wired for single-turn scenarios", sc.ID) - } - // Every registered cell must also record an outcome, or the progress table // would show a permanent gap between TOTAL and DONE that looks identical to // a hung cell. The paths below that leave via t.Fatalf/t.Skipf never reach // the explicit record after writeReport, so catch them here -- t.Fatalf and // t.Skipf both unwind through runtime.Goexit, which runs deferred calls. recorded := false defer func() { if recorded { return } switch { case t.Skipped(): progress.record(cli.ID, "skip") default: progress.record(cli.ID, "fail") } }() + + if effort != "" && len(sc.Turns) > 1 { + t.Fatalf("scenario %q: effort-level testing is only wired for single-turn scenarios", sc.ID) + }🤖 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 `@tests/e2e/clis/clis_test.go` around lines 188 - 199, Move the fallback recorder setup in runCell above the initial effort and single-turn validation that can call t.Fatalf. Ensure recorded and its deferred cleanup are installed before that guard, while preserving the existing recording behavior for all later exits.
🧹 Nitpick comments (8)
tests/e2e/api/runners/harness-monitor.mjs (1)
116-119: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
stripBase64Blobsis not applied to the URL and name passes.Line 197 strips base64 runs only before the item-JSON fallback. The URL and request-name passes at Lines 194-195 run unstripped. A data URL or an inline base64 image in a request URL can contain
o1oro3and get claimed byopenai.openaiis last inMATCH_ORDER, so the impact is limited, but applying the same guard to all three passes costs nothing.Also applies to: 190-200
🤖 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 `@tests/e2e/api/runners/harness-monitor.mjs` around lines 116 - 119, Apply stripBase64Blobs consistently to the URL, request-name, and item-JSON matching inputs in the relevant matching flow, rather than only the JSON fallback. Preserve the existing MATCH_ORDER and matching behavior while ensuring data URLs and inline base64 content cannot influence any of the three passes..github/workflows/scripts/resolve-cli-versions.sh (1)
104-141: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGuard the partial-resolution path.
resolve_failedis only set when a package resolves to an empty string. If npm returns fewer stable versions thanVERSION_COUNTfor one package but the others return three, the matrix still reports three legs while two legs test the same version of that package.nth_or_lastclamps correctly, so nothing breaks, but the leg labels then overstate coverage. Consider logging a warning when any array is shorter thanVERSION_COUNT.♻️ Proposed warning
while IFS= read -r line; do OPENCODE_VERSIONS+=("$line"); done <<<"$opencode_raw" + for pair in "claude:${`#CLAUDE_VERSIONS`[@]}" "codex:${`#CODEX_VERSIONS`[@]}" "opencode:${`#OPENCODE_VERSIONS`[@]}"; do + if [ "${pair#*:}" -lt "$VERSION_COUNT" ]; then + echo "::warning::${pair%%:*} has only ${pair#*:} stable release(s); legs will repeat the oldest one" + fi + done fi🤖 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 @.github/workflows/scripts/resolve-cli-versions.sh around lines 104 - 141, In the successful-resolution branch of the version matrix construction, add a warning when any of CLAUDE_VERSIONS, CODEX_VERSIONS, or OPENCODE_VERSIONS contains fewer than VERSION_COUNT entries. Keep nth_or_last fallback behavior and matrix generation unchanged, but clearly state that coverage is partial or versions are being repeated so the latest/latest-N labels do not overstate coverage..github/workflows/scripts/test-cli-harness.sh (1)
236-250: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale attempt snapshots survive across runs.
run_diris cleared withrm -rf, butattempt_diris only created. On a second invocation, snapshots from the previous run remain undertmp/cli-harness-attempts/<label>/and get uploaded alongside the current ones. Clear it withrun_dir.♻️ Proposed change
- rm -rf "$run_dir" + rm -rf "$run_dir" "$attempt_dir" mkdir -p "$run_dir" "$(dirname "$run_log")" "$attempt_dir"🤖 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 @.github/workflows/scripts/test-cli-harness.sh around lines 236 - 250, Update run_cases to remove the existing attempt_dir before recreating it, alongside the run_dir cleanup, so stale snapshots from prior invocations cannot be uploaded with current results.tests/e2e/clis/README.md (1)
164-171: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a language to the fenced code blocks.
markdownlint reports MD040 at lines 164, 190, 225, and 253. Add
textto each fence to satisfy the rule.📝 Proposed fix for line 164
-``` +```text >>> claude × anthropic × conversation-memory (model=anthropic/claude-sonnet-5, turns=3)🤖 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 `@tests/e2e/clis/README.md` around lines 164 - 171, Add the text language identifier to the fenced code blocks in README.md at the affected examples, including the block containing the claude conversation output, so all four fences satisfy markdownlint MD040.Source: Linters/SAST tools
tests/e2e/clis/clis_test.go (2)
1049-1064: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCap the raw transcript embedded in the HTML report.
transcriptDetailsinlines the full transcript for every cell. The README states raw stream-JSON output is thousands of lines per turn. With a full matrix the singleindex.htmlgrows to the sum of all transcripts, and that file is uploaded as a CI artifact and published to R2. Embed a bounded tail and keep the full file reachable through the existingtranscriptlink.♻️ Proposed fix
func transcriptDetails(result cellResult) string { transcript, _ := os.ReadFile(result.TranscriptPath) relTranscript := html.EscapeString(filepath.Base(result.TranscriptPath)) relMeta := html.EscapeString(filepath.Base(result.MetaPath)) + + // Keep index.html bounded: the full file stays one link away. + const maxInlineTranscript = 256 << 10 + shown := transcript + if len(shown) > maxInlineTranscript { + shown = shown[len(shown)-maxInlineTranscript:] + } var b strings.Builder b.WriteString(`<div class="panel"><h3>artifacts</h3>`) fmt.Fprintf(&b, `<a href="%s">summary json</a> · <a href="%s">transcript</a>`, relMeta, relTranscript) fmt.Fprintf(&b, `<details class="raw"><summary>raw stream (%d bytes)</summary><pre>%s</pre></details>`, - len(transcript), html.EscapeString(string(transcript))) + len(transcript), html.EscapeString(string(shown)))🤖 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 `@tests/e2e/clis/clis_test.go` around lines 1049 - 1064, Update transcriptDetails to embed only a bounded tail of the transcript in the raw-stream details, while preserving the existing transcript link to the complete file. Apply the size cap before HTML escaping and report the embedded tail’s byte length in the summary, using the existing transcript and result.TranscriptPath symbols.
528-541: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the
logCellResultdoc comment to its own function.The comment block at lines 528-534 describes
logCellResult, but it sits directly above thelogCellStartcomment with no blank line. Go attaches the merged block tologCellStart.logCellResultat line 556 then carries only the fragment at lines 554-555.♻️ Proposed fix
-// logCellResult prints one line per finished cell: the scenario and how it -// went, first, since that is what a reader is scanning for. The cell's identity -// and duration follow, and a failure reason is appended when there is one. -// -// Deliberately one line: cells finish concurrently, so anything multi-line -// would interleave into nonsense. Nothing parses this format - it exists to be -// read by a human and to be the record in CI's artifact log. // logCellStart announces a cell as it begins. Cells run in parallel and aThen prepend the moved text to the
logCellResultdoc comment at line 554.🤖 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 `@tests/e2e/clis/clis_test.go` around lines 528 - 541, Move the descriptive comment beginning with “logCellResult prints” so it directly documents the logCellResult function, keeping it separate from the logCellStart comment. Prepend that text to the existing logCellResult comment and leave logCellStart documented only by its own comment.tests/e2e/clis/progress_test.go (1)
266-274: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
progressEnabledmatches env values case-sensitively, unlikeprogressInterval.
progressIntervalacceptsoffin any case throughstrings.EqualFold.progressEnabledcompares exact lowercase strings. SoBIFROST_E2E_CLIS_PROGRESS=ONor=TRUEfalls through to the mirror-derived default instead of forcing the ticker on. The operator gets the opposite of what they asked for, with no warning.Lower-case the value before the switch.
♻️ Proposed change
func progressEnabled() bool { - switch strings.TrimSpace(os.Getenv(progressEnv)) { + switch strings.ToLower(strings.TrimSpace(os.Getenv(progressEnv))) { case "1", "true", "on": return true case "0", "false", "off": return false } return mirrorWriter() == 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 `@tests/e2e/clis/progress_test.go` around lines 266 - 274, Update progressEnabled to normalize the trimmed progressEnv value to lowercase before the switch, so uppercase and mixed-case values such as ON, TRUE, and OFF follow the same explicit behavior as lowercase values while preserving the mirrorWriter fallback for unrecognized values.tests/e2e/clis/scenarios_test.go (1)
185-190: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
AssertText: []string{"12"}is a weak substring check.
AssertTextmatches a bare substring anywhere in the turn output. The string12appears in timings, token counts, dates, and session identifiers that CLIs print. The turn can pass without the model answering.This file already defines
validateNumber, which anchors on word boundaries and tolerates thousands separators. Use it here for the same reason turn 3 ofreasoning-replayuses it.♻️ Proposed change
{ Send: "Without reading the file again, tell me what that file said the square root of 144 is. " + "Reply with just the number.", - AssertText: []string{"12"}, - Timeout: 120 * time.Second, + Validate: validateNumber(12, "square root of 144"), + Timeout: 120 * time.Second, },🤖 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 `@tests/e2e/clis/scenarios_test.go` around lines 185 - 190, Replace the weak AssertText substring check in the shown turn with the existing validateNumber validator, matching the usage in turn 3 of reasoning-replay. Configure it to require the answer 12 with word-boundary-aware numeric validation while preserving the existing prompt and timeout.
🤖 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 @.claude/skills/changelog-writer/SKILL.md:
- Line 319: Update the changelog output rule to prohibit and rewrite only en
dashes and em dashes in copied issue titles, while preserving ordinary hyphens
in terms such as tool-use, non-WAV, and openai-go.
In @.github/workflows/release-pipeline.yml:
- Line 181: The hardened release jobs currently allow only one hardcoded R2
hostname; update the network allowlist configuration in the release workflow to
permit the endpoint supplied by the R2_ENDPOINT secret for every relevant
release and report job, while retaining the existing PyPI host entries.
In @.github/workflows/scripts/push-mintlify-changelog.sh:
- Around line 72-74: Update the CLI harness table generation in the changelog
script to use the labels actually emitted by resolve-cli-versions and published
by test-cli-harness, including the pinned fallback, instead of unconditionally
generating latest, latest-1, and latest-2 links. Ensure rows are omitted or
paths are adjusted for labels that were not published so every generated link
targets an existing report.
In @.github/workflows/scripts/test-cli-harness.sh:
- Around line 384-385: Update the summary output in the CLI harness script to
reference the actual per-leg artifact name, using CLI_VERSION_LABEL in the
cli-harness-reports-<label> format instead of the obsolete cli-harness-reports
name. Preserve the existing index.html and retry-directory details.
In `@Makefile`:
- Around line 2343-2348: Update the cache-parity filter step in the Makefile to
prevent stale tmp/harness-cache-filtered.json from being reused: remove the
output file before running filter-collection.mjs, and ensure a failed filter
does not continue into the Newman/merge path. Keep the existing success path and
failure message intact.
In `@tests/e2e/api/runners/harness-monitor.mjs`:
- Around line 85-96: Update PROVIDER_KEYWORDS and the provider-attribution logic
in harness-monitor.mjs so Vertex requests are identified using stable folder
context or another substitution-resistant signal for both totals and results.
Remove reliance on the broad /genai match, and do not use /genai/v1beta/models/
alone because it also matches Gemini requests.
In `@tests/e2e/api/runners/lib/chained-vars.mjs`:
- Around line 60-84: The chained dependency flow loses request identity by
storing and resolving producers through non-unique names. Update
buildProducerIndex and bodyDependencies in
tests/e2e/api/runners/lib/chained-vars.mjs to carry the producer object or
stable entry index, and update expandWithProducers in
tests/e2e/api/runners/filter-collection.mjs to consume that identity directly
instead of resolving through the first-entry byName map; apply the corresponding
changes at both listed sites.
In `@tests/e2e/clis/procgroupunix_test.go`:
- Around line 51-60: Update killProcessGroup and the surrounding command
lifecycle so process-group termination is serialized with cmd.Wait and
activeCommands removal. Ensure a command is reaped and removed before
termination can proceed, and prevent sync.Map.Range from invoking
killProcessGroup on entries concurrently being deleted; preserve
os.ErrProcessDone handling and avoid signaling a reused PID or unrelated process
group.
In `@tests/e2e/clis/reasoningreplay_test.go`:
- Around line 46-56: Update the cleanup registration after each successful
opencodePreLaunch and opencodeResponsesPreLaunch call to use t.Cleanup with a
cleanup != nil guard, matching runCell’s behavior. Ensure chatCleanup and
respCleanup are only invoked when non-nil, while preserving the existing error
handling.
- Around line 140-151: Update readConfigFromEnv so its missing-OPENCODE_CONFIG
failure reports only the environment variable names, not the env values; add the
required strings import and derive names from env before calling t.Fatalf, while
preserving the existing successful read and file-error behavior.
- Around line 104-106: Update the reasoning-replay test case around chatOnly so
the selected model passes supportsCLIProviderModel for opencode-responses and
bedrock, leaving ExtendedThinking and AdaptiveThinking as the only failing
condition in supportsReasoningReplay. Add or use a model with the required
provider compatibility, then retain the assertion that reasoning-replay skips
models without reasoning.
In `@tests/e2e/clis/runner_test.go`:
- Around line 68-95: Update the command-wait handling in the turn execution path
to recognize exec.ErrWaitDelay as non-failing when cmd.ProcessState.ExitCode()
== 0, while preserving all non-zero exit errors. Before suppressing
ErrWaitDelay, validate the captured stdout/stderr and only treat the turn as
successful when that output meets the existing expected-result checks; otherwise
retain the failure reporting.
In `@tests/e2e/clis/scenarios_test.go`:
- Around line 570-581: The numeric assertions are too broad and can match
unrelated CLI output. In tests/e2e/clis/scenarios_test.go#L570-L581, update
validateTripSplit so the 2 assertion requires nearby trip wording while
preserving the existing 172 check; in
tests/e2e/clis/scenarios_test.go#L185-L190, replace fileReadScenario turn 2’s
AssertText entry for 12 with Validate using validateNumber(12, "square root of
144") so the value is context-bound.
- Around line 385-394: Update the regex in validateFahrenheitReading to accept
digit-adjacent Fahrenheit forms such as 38F while retaining support for spaced
and symbol-based forms. Remove only the leading word-boundary requirement from
the standalone F alternatives, preserve the trailing boundary, and keep
unrelated unit patterns unchanged.
In `@tests/integrations/python/config.json`:
- Around line 331-343: Update the release workflow’s test-core environment to
export DEEPSEEK_API_KEY from secrets.DEEPSEEK_API_KEY, matching the existing
mapping in config.yml so test-provider-harness.sh can resolve the key configured
by tests/integrations/python/config.json.
In `@tests/integrations/python/config.yml`:
- Around line 222-230: Update the Responses endpoint comment associated with the
deepseek configuration to state that Bifrost uses /chat/completions by default
and /anthropic/v1/messages only when use_anthropic_endpoints is enabled; leave
the valid deepseek-v4-flash and deepseek-v4-pro model mappings unchanged.
---
Outside diff comments:
In `@tests/e2e/clis/clis_test.go`:
- Around line 188-199: Move the fallback recorder setup in runCell above the
initial effort and single-turn validation that can call t.Fatalf. Ensure
recorded and its deferred cleanup are installed before that guard, while
preserving the existing recording behavior for all later exits.
---
Nitpick comments:
In @.github/workflows/scripts/resolve-cli-versions.sh:
- Around line 104-141: In the successful-resolution branch of the version matrix
construction, add a warning when any of CLAUDE_VERSIONS, CODEX_VERSIONS, or
OPENCODE_VERSIONS contains fewer than VERSION_COUNT entries. Keep nth_or_last
fallback behavior and matrix generation unchanged, but clearly state that
coverage is partial or versions are being repeated so the latest/latest-N labels
do not overstate coverage.
In @.github/workflows/scripts/test-cli-harness.sh:
- Around line 236-250: Update run_cases to remove the existing attempt_dir
before recreating it, alongside the run_dir cleanup, so stale snapshots from
prior invocations cannot be uploaded with current results.
In `@tests/e2e/api/runners/harness-monitor.mjs`:
- Around line 116-119: Apply stripBase64Blobs consistently to the URL,
request-name, and item-JSON matching inputs in the relevant matching flow,
rather than only the JSON fallback. Preserve the existing MATCH_ORDER and
matching behavior while ensuring data URLs and inline base64 content cannot
influence any of the three passes.
In `@tests/e2e/clis/clis_test.go`:
- Around line 1049-1064: Update transcriptDetails to embed only a bounded tail
of the transcript in the raw-stream details, while preserving the existing
transcript link to the complete file. Apply the size cap before HTML escaping
and report the embedded tail’s byte length in the summary, using the existing
transcript and result.TranscriptPath symbols.
- Around line 528-541: Move the descriptive comment beginning with
“logCellResult prints” so it directly documents the logCellResult function,
keeping it separate from the logCellStart comment. Prepend that text to the
existing logCellResult comment and leave logCellStart documented only by its own
comment.
In `@tests/e2e/clis/progress_test.go`:
- Around line 266-274: Update progressEnabled to normalize the trimmed
progressEnv value to lowercase before the switch, so uppercase and mixed-case
values such as ON, TRUE, and OFF follow the same explicit behavior as lowercase
values while preserving the mirrorWriter fallback for unrecognized values.
In `@tests/e2e/clis/README.md`:
- Around line 164-171: Add the text language identifier to the fenced code
blocks in README.md at the affected examples, including the block containing the
claude conversation output, so all four fences satisfy markdownlint MD040.
In `@tests/e2e/clis/scenarios_test.go`:
- Around line 185-190: Replace the weak AssertText substring check in the shown
turn with the existing validateNumber validator, matching the usage in turn 3 of
reasoning-replay. Configure it to require the answer 12 with word-boundary-aware
numeric validation while preserving the existing prompt and timeout.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 17296af0-2442-4025-aa65-afac4a7e1e13
📒 Files selected for processing (35)
.claude/skills/changelog-writer/SKILL.md.github/workflows/release-pipeline.yml.github/workflows/scripts/push-mintlify-changelog.sh.github/workflows/scripts/resolve-cli-versions.sh.github/workflows/scripts/test-cli-harness.sh.github/workflows/scripts/test-provider-harness.sh.github/workflows/scripts/upload-test-reports-to-r2.shMakefiledocs/changelogs/ent-v1.5.9.mdxdocs/changelogs/v1.6.9.mdxdocs/docs.jsontests/cmd/e2eseed/go.modtests/cmd/seed/go.modtests/cmd/seedvks/go.modtests/e2e/api/runners/augment-provider-harness.mjstests/e2e/api/runners/filter-collection.mjstests/e2e/api/runners/harness-monitor.mjstests/e2e/api/runners/lib/chained-vars.mjstests/e2e/api/runners/lib/chained-vars.test.mjstests/e2e/api/runners/lib/ci-interval.mjstests/e2e/api/runners/lib/ci-interval.test.mjstests/e2e/api/runners/lib/read-report.mjstests/e2e/clis/README.mdtests/e2e/clis/assertion_output_test.gotests/e2e/clis/clis_test.gotests/e2e/clis/matrix_test.gotests/e2e/clis/procgroupother_test.gotests/e2e/clis/procgroupunix_test.gotests/e2e/clis/progress_test.gotests/e2e/clis/progresstable_test.gotests/e2e/clis/reasoningreplay_test.gotests/e2e/clis/runner_test.gotests/e2e/clis/scenarios_test.gotests/integrations/python/config.jsontests/integrations/python/config.yml
c44b750 to
b762b00
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
tests/e2e/clis/runner_test.go (1)
628-647: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
appendto the caller'senvslice can write into a shared backing array.The fallback branch appends to
envin place. If the caller's slice has spare capacity, that write is visible to the caller and to any other slice sharing the array. Copy before appending.♻️ Proposed change
t.Cleanup(func() { _ = os.RemoveAll(tempHome) }) - env = append(env, "CODEX_HOME="+tempHome) + env = append(append([]string(nil), env...), "CODEX_HOME="+tempHome) }🤖 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 `@tests/e2e/clis/runner_test.go` around lines 628 - 647, In the fallback branch of the driver setup, copy the caller-provided env slice before appending CODEX_HOME so the original slice and shared backing arrays remain unchanged. Update the code around envValue and the subsequent append while preserving existing temp-directory creation and cleanup behavior.Makefile (1)
2183-2187: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
HARNESS_PROVIDERSin the parallel branch.Line 1865 introduces
HARNESS_PROVIDERSso the provider set lives in one place. The parallel branch still restates the same list twice: once in the message on line 2183 and once inPROVIDERSon line 2187. Adding a provider now requires three edits, and the sequential branch on line 2301 would silently diverge from the parallel branch.♻️ Proposed refactor
- say "$(CYAN)Parallel mode (default): forking one newman per provider (openai, anthropic, bedrock, gemini, vertex, azure, passthrough, openrouter). Set PARALLEL=0 to disable.$(NC)"; \ + say "$(CYAN)Parallel mode (default): forking one newman per provider ($(HARNESS_PROVIDERS)). Set PARALLEL=0 to disable.$(NC)"; \ rm -f tmp/newman-report-*.json tmp/newman-cli-*.log tmp/parallel-pids tmp/parallel-status; \ : > tmp/parallel-pids; \ : > tmp/parallel-status; \ - PROVIDERS="openai anthropic bedrock gemini vertex azure passthrough openrouter"; \ + PROVIDERS="$(HARNESS_PROVIDERS)"; \🤖 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 `@Makefile` around lines 2183 - 2187, Update the parallel branch to reuse HARNESS_PROVIDERS for both the informational message and the PROVIDERS iteration list, removing the duplicated hard-coded provider names while preserving the existing parallel behavior..github/workflows/release-pipeline.yml (1)
422-423: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDisable credential persistence for this checkout.
resolve-cli-versionsonly reads the repository and runs a script. Setpersist-credentials: falseso the job does not leave a usable token in.git/config.🔒 Proposed fix
- name: Checkout repository uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + persist-credentials: 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 @.github/workflows/release-pipeline.yml around lines 422 - 423, Update the actions/checkout step in the release pipeline to set persist-credentials to false, ensuring the resolve-cli-versions job’s read-only checkout does not retain credentials in .git/config.Source: 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 @.claude/skills/changelog-writer/SKILL.md:
- Line 327: In the changelog requirements bullet for the “Closed GitHub Issues”
section, update the cross-reference from “Collect Closed GitHub Issues below” to
“above” so it correctly points to the earlier section.
In @.github/workflows/release-pipeline.yml:
- Around line 2773-2784: The CLI_HARNESS_LABELS expression must handle skipped
resolve-cli-versions jobs without parsing an empty output, and
push-mintlify-changelog.sh must distinguish an explicitly empty value from an
unset variable. Guard the fromJSON call in the workflow and update the script’s
fallback logic so empty labels produce no CLI report links while unset labels
retain the default legs.
In @.github/workflows/scripts/push-mintlify-changelog.sh:
- Around line 104-108: Update the CLI_HARNESS_INTRO selection to inspect the
actual single-leg label rather than the row count; only use the pinned-leg
sentence when the label is exactly “pinned,” and retain the multi-release
compatibility text for other labels or multiple legs.
In `@tests/e2e/clis/waitdelayunix_test.go`:
- Around line 28-34: Replace all three exec.Command calls in startOrphanHolder
and the other visible command-launching test paths with exec.CommandContext,
passing the test context so cancellation is honored. Keep the existing command
arguments, process-group setup, and cleanup behavior unchanged; do not alter the
test-local code argument for the command-injection findings.
- Around line 43-48: Update the command setup around the cleanup callback and
cmd.Wait so the process group ID is captured before the child is reaped, then
have t.Cleanup use that stored pgid to send SIGKILL. Preserve the existing pgid
validity check and avoid calling syscall.Getpgid after Wait.
---
Nitpick comments:
In @.github/workflows/release-pipeline.yml:
- Around line 422-423: Update the actions/checkout step in the release pipeline
to set persist-credentials to false, ensuring the resolve-cli-versions job’s
read-only checkout does not retain credentials in .git/config.
In `@Makefile`:
- Around line 2183-2187: Update the parallel branch to reuse HARNESS_PROVIDERS
for both the informational message and the PROVIDERS iteration list, removing
the duplicated hard-coded provider names while preserving the existing parallel
behavior.
In `@tests/e2e/clis/runner_test.go`:
- Around line 628-647: In the fallback branch of the driver setup, copy the
caller-provided env slice before appending CODEX_HOME so the original slice and
shared backing arrays remain unchanged. Update the code around envValue and the
subsequent append while preserving existing temp-directory creation and cleanup
behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 292317c5-21e5-40f6-b781-67dcfc271af8
📒 Files selected for processing (20)
.claude/skills/changelog-writer/SKILL.md.github/workflows/release-pipeline.yml.github/workflows/scripts/push-mintlify-changelog.sh.github/workflows/scripts/test-cli-harness.shMakefiletests/e2e/api/runners/filter-collection.mjstests/e2e/api/runners/harness-monitor.mjstests/e2e/api/runners/lib/chained-vars.mjstests/e2e/api/runners/lib/chained-vars.test.mjstests/e2e/api/runners/lib/provider-attribution.mjstests/e2e/api/runners/lib/provider-attribution.test.mjstests/e2e/clis/assertion_output_test.gotests/e2e/clis/clis_test.gotests/e2e/clis/procgroupother_test.gotests/e2e/clis/procgroupunix_test.gotests/e2e/clis/reasoningreplay_test.gotests/e2e/clis/runner_test.gotests/e2e/clis/scenarios_test.gotests/e2e/clis/waitdelayunix_test.gotests/integrations/python/config.yml
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/e2e/clis/assertion_output_test.go
- tests/e2e/api/runners/filter-collection.mjs
- tests/integrations/python/config.yml
- .github/workflows/scripts/test-cli-harness.sh
- tests/e2e/clis/scenarios_test.go
- tests/e2e/api/runners/lib/chained-vars.mjs
- tests/e2e/clis/clis_test.go
- tests/e2e/api/runners/harness-monitor.mjs
b762b00 to
4424470
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/workflows/release-pipeline.yml (1)
422-423: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueOptional: set
persist-credentials: falsefor this checkout.
resolve-cli-versionsonly reads the repository to run a script. It does not push. zizmor flags the checkout as persisting theGITHUB_TOKENin.git/config. Other checkout steps in this workflow follow the same pattern, so treat this as consistency cleanup rather than an active exploit path.🔒 Proposed change
- name: Checkout repository uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + persist-credentials: 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 @.github/workflows/release-pipeline.yml around lines 422 - 423, Update the checkout step associated with resolve-cli-versions to set persist-credentials to false. Apply the same option to the other read-only actions/checkout steps in the workflow for consistency, without changing their existing repository checkout behavior.Source: Linters/SAST tools
tests/e2e/clis/waitdelayunix_test.go (1)
211-214: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPoll for group death instead of checking once.
The cleanup sends
SIGKILLand returns immediately. Signal delivery and process teardown are asynchronous.syscall.Kill(-pgid, 0)can still succeed for a short window after the cleanup runs, which makes this assertion flaky. Poll with a short deadline. The coding guidelines require deterministic tests.♻️ Proposed fix
- // The subtest has returned, so its t.Cleanup has run. - if err := syscall.Kill(-pgid, 0); err == nil { - t.Error("the backgrounded child survived cleanup; it will outlive the suite") - } + // The subtest has returned, so its t.Cleanup has run. SIGKILL delivery is + // asynchronous, so poll rather than sample once. + deadline := time.Now().Add(2 * time.Second) + for { + if err := syscall.Kill(-pgid, 0); err != nil { + break + } + if time.Now().After(deadline) { + t.Error("the backgrounded child survived cleanup; it will outlive the suite") + break + } + time.Sleep(20 * time.Millisecond) + }🤖 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 `@tests/e2e/clis/waitdelayunix_test.go` around lines 211 - 214, Update the post-cleanup process-group assertion near the subtest cleanup comment to poll syscall.Kill(-pgid, 0) until the group exits or a short deadline expires. Preserve the existing failure message, but only call t.Error if the process group remains alive when the deadline is reached, ensuring the test tolerates asynchronous SIGKILL teardown deterministically.Source: Coding guidelines
.claude/skills/changelog-writer/SKILL.md (1)
143-155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStep 3 and Step 4 disagree about cascade-only modules.
Step 3 tells the agent to ask the user for a bump type for every bumped module, including cascade-only modules. Step 4 (lines 170-174) then forces
patch_bumpfor any module that did not change, so the user's answer for a cascade-only module is discarded. Align the two steps: either restrict the questions to changed modules, or state that a user-chosen bump overrides the cascade default in Step 4.♻️ Proposed clarification
-Ask for **every** module that will be bumped - both modules with code changes and modules with only cascade bumps. Use AskUserQuestion with up to 4 questions at a time (the tool's limit), batching in hierarchy order: +Ask for **every** module that will be bumped - both modules with code changes and modules with only cascade bumps. A user's answer overrides the default cascade bump used in Step 4. Use AskUserQuestion with up to 4 questions at a time (the tool's limit), batching in hierarchy order:🤖 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 @.claude/skills/changelog-writer/SKILL.md around lines 143 - 155, Align the cascade-only handling between the version-bump questions and Step 4: retain questions for every bumped module, and make a user-selected bump type override the default patch_bump for unchanged cascade-only modules. Update the Step 4 instructions near patch_bump to preserve explicitly chosen bump types while applying patch_bump only when no choice was provided.
🤖 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 @.github/workflows/release-pipeline.yml:
- Line 284: Add api.deepseek.com:443 to the allowlist used by the test-core job
for provider-harness requests, while leaving the test-api-integrations allowlist
unchanged.
---
Nitpick comments:
In @.claude/skills/changelog-writer/SKILL.md:
- Around line 143-155: Align the cascade-only handling between the version-bump
questions and Step 4: retain questions for every bumped module, and make a
user-selected bump type override the default patch_bump for unchanged
cascade-only modules. Update the Step 4 instructions near patch_bump to preserve
explicitly chosen bump types while applying patch_bump only when no choice was
provided.
In @.github/workflows/release-pipeline.yml:
- Around line 422-423: Update the checkout step associated with
resolve-cli-versions to set persist-credentials to false. Apply the same option
to the other read-only actions/checkout steps in the workflow for consistency,
without changing their existing repository checkout behavior.
In `@tests/e2e/clis/waitdelayunix_test.go`:
- Around line 211-214: Update the post-cleanup process-group assertion near the
subtest cleanup comment to poll syscall.Kill(-pgid, 0) until the group exits or
a short deadline expires. Preserve the existing failure message, but only call
t.Error if the process group remains alive when the deadline is reached,
ensuring the test tolerates asynchronous SIGKILL teardown deterministically.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f474d57f-3756-43d4-98d0-3f108362dd54
📒 Files selected for processing (4)
.claude/skills/changelog-writer/SKILL.md.github/workflows/release-pipeline.yml.github/workflows/scripts/push-mintlify-changelog.shtests/e2e/clis/waitdelayunix_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/scripts/push-mintlify-changelog.sh
4424470 to
3d7b43b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/release-pipeline.yml:
- Around line 415-421: Add azure.archive.ubuntu.com:80 and esm.ubuntu.com:443 to
the allowed-endpoints list used by the resolve-cli-versions resolver job,
alongside the existing network entries. Verify the entries match the current
Ubuntu runner image’s APT repositories so detect-changes can complete apt-get
update under the blocked egress policy.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d6a7636f-1d36-4793-817f-f4036f99cf65
📒 Files selected for processing (1)
.github/workflows/release-pipeline.yml
3d7b43b to
6193b27
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 @.github/workflows/release-pipeline.yml:
- Line 346: Validate the transport-version value produced by
detect-all-changes.sh before it reaches the release command. Pass
needs.detect-changes.outputs.transport-version through an environment variable,
validate it against the supported version format, and only then use the
validated value in the shell command to prevent command-substitution injection.
In @.github/workflows/scripts/check-egress-allowlist.sh:
- Around line 14-24: Add a pull-request workflow job that is triggered for
changes under .github/workflows/**, installs the PyYAML dependency, and executes
check-egress-allowlist.sh. Ensure the job validates the workflow files and
integrates with the repository’s existing workflow configuration.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7d442254-9e3b-4733-8668-0a3f2f5f5c34
📒 Files selected for processing (2)
.github/workflows/release-pipeline.yml.github/workflows/scripts/check-egress-allowlist.sh
6193b27 to
3a09d4e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/scripts/detect-all-changes.sh:
- Around line 27-45: Update the version validation case in detect-all-changes.sh
to use an anchored Bash regex requiring exactly three numeric MAJOR.MINOR.PATCH
components, with an optional v prefix and any permitted suffix beginning only
after PATCH; reject separators or letters within the numeric components and
extra numeric components. Extend version-format.test.sh with rejection cases for
1x.2.3, 1.2.3rc1, and 1.2.3.4.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8a4359aa-0bf6-48fd-b64b-cb64c1f9144e
📒 Files selected for processing (3)
.github/workflows/scripts/detect-all-changes.sh.github/workflows/scripts/version-format.test.sh.github/workflows/workflow-lint.yml
3a09d4e to
395b2b0
Compare
395b2b0 to
0abf7ba
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/scripts/version-format.test.sh:
- Around line 84-95: Add expect_reject cases for “1.2.3-” and “1.2.3+” in the
malformed-version tests, and update validate_version in detect-all-changes.sh so
suffix character classes require at least one character instead of allowing
empty suffixes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ebdfefe4-af4b-40cc-9b5b-439c96551726
📒 Files selected for processing (2)
.github/workflows/scripts/detect-all-changes.sh.github/workflows/scripts/version-format.test.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/scripts/detect-all-changes.sh
0abf7ba to
ec3ac22
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/changelogs/ent-v1.5.8.mdx`:
- Around line 8-10: Update the SSE heartbeat warnings in
docs/changelogs/ent-v1.5.8.mdx lines 8-10 and docs/changelogs/v1.6.8.mdx lines
19-21 to replace the broad “LangChain, OpenAI Go” wording with the specific
affected conditions: raw passthrough line corruption and openai-go/ssestream
versions before v3.43.0 aborting on heartbeat frames. Keep the upgrade
recommendation unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: becbf7b9-6e68-49df-803e-894b5c6d22fb
📒 Files selected for processing (2)
docs/changelogs/ent-v1.5.8.mdxdocs/changelogs/v1.6.8.mdx
ec3ac22 to
e9bee51
Compare
Merge activity
|
## Summary Briefly explain the purpose of this PR and the problem it solves. ## Changes - What was changed and why - Any notable design decisions or trade-offs ## Type of change - [ ] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Describe the steps to validate this change. Include commands and expected outcomes. ```sh # Core/Transports go version go test ./... # UI cd ui pnpm i || npm i pnpm test || npm test pnpm build || npm run build ``` If adding new configs or environment variables, document them here. ## Screenshots/Recordings If UI changes, add before/after screenshots or short clips. ## Breaking changes - [ ] Yes - [ ] No If yes, describe impact and migration instructions. ## Related issues Link related issues and discussions. Example: Closes maximhq#123 ## Security considerations Note any security implications (auth, secrets, PII, sandboxing, etc.). ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable

Summary
Briefly explain the purpose of this PR and the problem it solves.
Changes
Type of change
Affected areas
How to test
Describe the steps to validate this change. Include commands and expected outcomes.
If adding new configs or environment variables, document them here.
Screenshots/Recordings
If UI changes, add before/after screenshots or short clips.
Breaking changes
If yes, describe impact and migration instructions.
Related issues
Link related issues and discussions. Example: Closes #123
Security considerations
Note any security implications (auth, secrets, PII, sandboxing, etc.).
Checklist
docs/contributing/README.mdand followed the guidelines