Repository navigation
ci: isolate new ordinary AMD64 image contracts from legacy pools - #5174
ktechmidas merged 3 commits into
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: dashpay/platform/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe selector now uses fingerprinted, kind-specific ordinary labels for non-legacy AMD64 manifests. Legacy AMD64 and ARM64 retain generic labels. Tests cover routing cases and shared toolchain comparisons. Documentation describes the rollout sequence and source-change boundaries. ChangesOrdinary Image Routing
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to Non-legacy AMD64 jobs require a compatible fingerprinted pool and queue if that capacity is unavailable, rather than using an incompatible image. No additional concrete merge risk is established. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The new routing separates incompatible runner images rather than relaxing image checks. Its main remaining risk is operational: compatible new capacity must be provisioned and verified before the new contract becomes the branch default, or jobs will queue. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
✅ Final review complete — no blockers (commit 44384d7) · triage: low |
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
The ordinary AMD64 routing change preserves the existing candidate provenance checks and isolates non-legacy contracts as intended. All 41 focused Python tests and the range-level whitespace check pass, but the fixture conversion introduces one confirmed, non-blocking regression in the cross-architecture consistency test. Actual runner provisioning and execution were not validated locally.
🟡 1 suggestion(s)
1 finding(s) not shown inline (the lines are not part of this PR's diff)
🟡 Suggestion: Keep the cross-architecture consistency test on live manifests
.github/scripts/tests/test_runner_image.py:315-316
The setUp change makes self.manifest a frozen legacy fixture, so this test now compares historical AMD64 requirements against the live ARM64 requirements instead of comparing the two current contracts. I reproduced both consequences by mocking manifest reads: setting java_major to 21 in both live manifests fails with '17 != 21', while changing only the live AMD64 manifest incorrectly passes. The frozen fixture is appropriate for historical publisher and routing tests, but this consistency test must load the live AMD64 manifest directly. This is a non-blocking test-coverage regression under the review severity policy; it does not change runtime routing or consensus behavior.
amd = runner.read_manifest(ROOT / runner.MANIFEST)["requirements"]
arm = runner.read_manifest(ROOT / runner.ARM64_MANIFEST)["requirements"]
source: glm-5.3-flash (phase1-reviewer: general, architecture-layering, security-auditor); gpt-6-astra (phase2-reviewer: general)
Review provenance
Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: glm-5.3-flash (agent: phase1-reviewer, role: architecture-layering); reviewer 3: glm-5.3-flash (agent: phase1-reviewer, role: security-auditor); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
lowbygpt-6-astra(effort low) — The diff makes a small, contained CI routing change to select fingerprint-specific AMD64 runner labels while preserving legacy and ARM64 paths, with focused regression tests and no changes to critical application surfaces. - Phase 1 reviewers:
glm-5.3-flash— general (completed, effort high); agentphase1-reviewer,glm-5.3-flash— architecture-layering (completed, effort high); agentphase1-reviewer,glm-5.3-flash— security-auditor (completed, effort high); agentphase1-reviewer - Phase 1 model:
glm-5.3-flash— zai quota: 5h 87% left, weekly 91% left; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 13% left, 5h 100% left) - Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort medium); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort medium); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort medium); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `.github/scripts/tests/test_runner_image.py`:
- [SUGGESTION] .github/scripts/tests/test_runner_image.py:315-316: Keep the cross-architecture consistency test on live manifests
The setUp change makes self.manifest a frozen legacy fixture, so this test now compares historical AMD64 requirements against the live ARM64 requirements instead of comparing the two current contracts. I reproduced both consequences by mocking manifest reads: setting java_major to 21 in both live manifests fails with '17 != 21', while changing only the live AMD64 manifest incorrectly passes. The frozen fixture is appropriate for historical publisher and routing tests, but this consistency test must load the live AMD64 manifest directly. This is a non-blocking test-coverage regression under the review severity policy; it does not change runtime routing or consensus behavior.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- ARM64 ordinary pool still routes via generic labels — Outside scope and intentionally unchanged: the PR explicitly preserves ARM64 provisioning and validation behavior. Extending fingerprinted ordinary pools to a future ARM64 rollout is an adjacent design task, not a current regression or an exceptional follow-up requiring review output.
- Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
|
Fixed the live-manifest consistency regression in Two committed regression tests fail before the fix and pass afterward. All 43 focused CI-script tests pass with both the current manifest and staged PR5172 recipe, plus Python syntax and diff checks. The incremental diff is one CI test file only; no application test, runtime selector, manifest, workflow permission, runner registration or trust boundary changed. This is an ordinary fast-forward of the existing repair branch. Prior-head reviews are not counted as current-head coverage. Fresh hosted CI/independent review, owner approval, real candidate execution and compatible ordinary capacity remain required; PR5172 merge holds remain intact. |
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The selector correctly preserves legacy generic labels only for the exact provisioned AMD64 manifest and routes other ordinary AMD64 jobs to full-manifest, kind-specific Linux/X64 labels while preserving candidate and ARM64 paths. The focused 43-test suite, Python syntax checks, and diff validation pass at the exact reviewed head; no in-scope defects remain.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: glm-5.3-flash (agent: phase1-reviewer, role: architecture-layering); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
lowbygpt-6-astra(effort low) — The behavioral change is a small, contained CI runner-selection update keyed by an exact manifest fingerprint, with focused regression tests and no changes to application logic or critical surfaces. - Phase 1 reviewers:
glm-5.3-flash— general (completed, effort high); agentphase1-reviewer,glm-5.3-flash— architecture-layering (completed, effort high); agentphase1-reviewer - Phase 1 model:
glm-5.3-flash— zai quota: 5h 99% left, weekly 90% left; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 13% left, 5h 100% left) - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort medium); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort medium); agentphase2-reviewer,gpt-6-astra— general (completed, effort medium); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort medium); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
|
@coderabbitai review Ordinary included review request for exact head |
✅ Action performedReview finished.
|
|
Ready for review — needs QuantumExplorer or shumkov. |
Summary
Prevent a recipe rollout from mixing incompatible ordinary AMD64 runner images across active Platform branches. This is a CI-only prerequisite for the staged recipe in #5172, not a runner rollout or application change.
Read-only live checks on both Linux hosts and the separate NPM validation container prove that all three accept the current
d272d01b…contract and reject the staged recipe withRunner image mismatch: recipe_revision. Merely adding the new image under existingrust-ci/kotlin-ci/npm-prlabels would let old and new branches land on incompatible images.d272d01bcf3dfa620bbab1e9f31c3e1987862d33ec876bbad83c80f29de41a58.Validation
git diff --checkpass. Required hosted CI and independent owner/bot review remain required; local tests are not live execution proof.Rollout boundary
Merge this scoped source repair only through normal protected review/CI gates. Before merging PR5172, first verify exact new immutable-image provenance and actual candidate Rust/Kotlin/NPM execution, stage separate compatible ordinary capacity with the new labels and unchanged isolation/repository scope, and verify the selector adoption. Keep old registrations/images available for older branches and heads. Missing compatible capacity should queue rather than silently use a different recipe.
Checklist
PR Hygiene ·
44384d7.github/ORDINARY_RUNNER_IMAGES.md,.github/scripts/runner-image.py,.github/scripts/tests/fixtures/legacy-runner-requirements.jsonand 2 more) — QuantumExplorer or shumkovWhen every box is checked the
PR Hygienecheck passes and this can merge.Summary by CodeRabbit