Fix scheduled slow-suite regressions - #413
Conversation
Greptile SummaryThis PR fixes several scheduled slow-suite regressions. The main changes are:
Confidence Score: 4/5Ambiguous stale-app config matches can leave live workloads running.
lib/command/cleanup_stale_apps.rb Important Files Changed
Reviews (1): Last reviewed commit: "Fix scheduled slow-suite regressions" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Warning Review limit reached
Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe changes update stale-app stop-mode workload selection, handle forbidden upstream image requests, retry workload image updates, and add regression coverage for these behaviors and related slow-suite test execution. ChangesReliability fixes
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CleanupStaleApps
participant Config
participant Controlplane
participant Cpflow
CleanupStaleApps->>Config: Read configured workloads
CleanupStaleApps->>Controlplane: Fetch live workloads
CleanupStaleApps->>Cpflow: Stop matching workloads
sequenceDiagram
participant DeployImage
participant Controlplane
participant Shell
DeployImage->>Controlplane: Update workload image reference
Controlplane->>Shell: Execute update command
Shell-->>Controlplane: Return success or failure
DeployImage->>Controlplane: Retry failed update
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
Review: Fix scheduled slow-suite regressions (PR 413) Overview: this PR addresses four independent regressions surfaced by the scheduled slow-suite run — (1) Code quality
Potential issues
Security Tests |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Review: Fix scheduled slow-suite regressions (#413)OverviewThis PR fixes four independent regressions surfaced by the scheduled slow-suite run (issue #409):
Code quality
Potential issues (posted inline)
Security
Performance
Test coverageSolid — each of the four fixes has a matching unit spec that exercises both success and bounded-failure paths, and the PR body documents the validation that was run (49 examples passing, lint/docs clean). The main gap is that end-to-end (real Overall this is a well-scoped, well-tested bug-fix PR. The inline notes above are minor/edge-case observations rather than blockers. |
ReviewFocused, well-scoped fix for the four regressions found by the scheduled slow-suite run (issue #409). Each fix is narrowly targeted and backed by a new/updated unit spec that pins the exact failure mode it addresses. Overview
Strengths
Minor suggestions (left as inline comments)
Risk/CorrectnessNo security concerns beyond the token-leak check above, which is already covered. The Nothing blocking — nice, surgical PR. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
spec/command/cleanup_stale_apps_spec.rb (1)
132-141: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdapt test to allow optional
additional_workloads.If
additional_workloadsis relaxed to be an optional configuration key as suggested inlib/command/cleanup_stale_apps.rb, this test should be updated to verify that the method succeeds (or falls back to an empty array) rather than expecting it to raise an error.♻️ Proposed test update
- it "raises when the stale app config does not define additional_workloads" do + it "succeeds when the stale app config does not define additional_workloads" do allow(config).to receive(:find_app_config) .with("stale-app") .and_return({ app_workloads: ["rails"] }) - expect { command.send(:process_app, "stale-app") } - .to raise_error("Can't find option 'additional_workloads' for app 'stale-app' in 'controlplane.yml'.") - expect(cp).not_to have_received(:fetch_workloads) - expect(command).not_to have_received(:run_cpflow_command) + command.send(:process_app, "stale-app") + + expect(cp).to have_received(:fetch_workloads) end🤖 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 `@spec/command/cleanup_stale_apps_spec.rb` around lines 132 - 141, Update the “raises when the stale app config…” example around command.send(:process_app, "stale-app") to reflect optional additional_workloads: expect processing to succeed and use an empty workload list when the key is absent, while preserving assertions that no workload fetch or cpflow command occurs.
🤖 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.
Nitpick comments:
In `@spec/command/cleanup_stale_apps_spec.rb`:
- Around line 132-141: Update the “raises when the stale app config…” example
around command.send(:process_app, "stale-app") to reflect optional
additional_workloads: expect processing to succeed and use an empty workload
list when the key is absent, while preserving assertions that no workload fetch
or cpflow command occurs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 643b4f88-6ab0-4d0c-8fe5-863c3c51a971
📒 Files selected for processing (10)
CHANGELOG.mdlib/command/cleanup_stale_apps.rblib/command/copy_image_from_upstream.rblib/command/deploy_image.rblib/core/controlplane.rbspec/command/build_image_spec.rbspec/command/cleanup_stale_apps_spec.rbspec/command/copy_image_from_upstream_unit_spec.rbspec/command/deploy_image_unit_spec.rbspec/core/controlplane_spec.rb
|
Address review is complete at exact head Implemented findings:
Verified decisions:
Skipped as non-actionable: the unreachable ambiguous-prefix scenario and unchanged endpoint-nil semantics. All nine review threads are now resolved, and the independent exact-head code audit is clean. Merge remains blocked only by external GVC capacity: exact-head run 29492232103, artifact |
|
Canonical blocked-batch handoff for Immediate maintainer attention
FYI / decisions made
Independent checker verdict: |
Target stale-app stops to configured live workloads, contain upstream authorization failures inside the progress step, and retry transient workload image updates before reporting endpoints. Update the build spec to compare shell tokens instead of raw escaping.
5309f20 to
261d849
Compare
Review SummaryThis PR fixes four independent regressions surfaced by the scheduled slow-suite run for issue #409. Each fix is narrowly scoped and backed by new/updated unit tests. Overall the change is well-reasoned and low-risk; I traced through the control flow for each fix and did not find correctness bugs, only a few things worth being aware of. What the PR does
Correctness
Minor points (posted inline)
Test coverageGood coverage of the new behavior: workload-filtering (including both "missing required option" error paths), the forbidden-error/profile-cleanup path (with an explicit assertion that the token itself is never printed), and all three retry outcomes for the image-update path (success-after-retry, endpoint-resolution failure after a successful update, and exhausting the retry budget). The SecurityNo concerns -- the upstream-token handling change actually improves things (test explicitly asserts the token never appears in printed output), and no new user input is passed unsanitized to shell commands. No blocking issues found. |
Address-review summaryScan scope: since previous summary at 2026-07-16T11:10:42Z. Mattered
Optional
Skipped
All three exact-head threads were replied to and resolved. No branch change was made; candidate SHA remains Next default scan starts after this comment. Say |
|
Exact-head Slow replay classification for
Merge remains blocked. Per the batch contract, I will rerun only the failed PR Fast job after these Slow resources are released and capacity is clear, then obtain a clean exact-head Slow replay before merging. |
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 `@lib/command/cleanup_stale_apps.rb`:
- Around line 114-115: Update the configured_workloads assignment to normalize
nil results from required_app_option for both :app_workloads and
:additional_workloads to empty arrays before concatenation, using Array(...) or
an equivalent fallback. Preserve concatenation of configured workload arrays
when values are present.
🪄 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: CHILL
Plan: Pro
Run ID: 21925559-c4a4-4fbd-a8e9-809ab4803cb2
📒 Files selected for processing (11)
CHANGELOG.mdlib/command/cleanup_stale_apps.rblib/command/copy_image_from_upstream.rblib/command/deploy_image.rblib/core/controlplane.rbspec/command/build_image_spec.rbspec/command/cleanup_stale_apps_spec.rbspec/command/copy_image_from_upstream_unit_spec.rbspec/command/deploy_image_unit_spec.rbspec/command/run_spec.rbspec/core/controlplane_spec.rb
🚧 Files skipped from review as they are similar to previous changes (9)
- lib/core/controlplane.rb
- spec/core/controlplane_spec.rb
- spec/command/build_image_spec.rb
- spec/command/deploy_image_unit_spec.rb
- spec/command/cleanup_stale_apps_spec.rb
- lib/command/copy_image_from_upstream.rb
- spec/command/copy_image_from_upstream_unit_spec.rb
- lib/command/deploy_image.rb
- CHANGELOG.md
ReviewScoped, well-tested fix for the four scheduled slow-suite regressions from #409. Traced each change against its rationale and existing conventions (e.g. the What it does
Inline notes posted (all minor, non-blocking):
Test coverage: solid — new specs cover the stale-workload intersection logic (including both missing-config-key error paths), the forbidden-upstream-token flow (and confirms the token itself isn't leaked to output), and all three retry outcomes (immediate success-after-retry, exhausted retries, and endpoint-resolution failure not re-triggering the image update). No security issues found — the |
Address-review summaryScan scope: since previous summary at 2026-07-18T11:30:20Z. Mattered
Optional
Skipped
All four newly opened threads were replied to and resolved. No branch change was made; candidate SHA remains Next default scan starts after this comment. Say |
Address-review summaryScan scope: since previous summary at 2026-07-19T08:18:11Z. Mattered
Skipped
The fresh Next default scan starts after this comment. Say |
Review: Fix scheduled slow-suite regressions (#413)OverviewThis PR fixes four regressions surfaced by the scheduled slow suite (issue #409):
Code quality
Security
Minor, non-blocking notes (posted inline)
Test coverageGood — the PR adds targeted regression tests for all four fixes, including negative/error-path cases (missing config keys, forbidden upstream token, bounded-retry exhaustion). No test coverage gaps identified for the described scope. No blocking issues found. |
Address-review summaryScan scope: since previous summary at 2026-07-22T00:17:45Z. Mattered
Optional
Skipped
Both fresh threads were replied to and resolved. No branch change was made; candidate SHA remains Next default scan starts after this comment. Say |
1 participant
Summary
Fixes #409
Diagnosis
The scheduled slow suite exposed four regressions addressed by this PR: a stale raw-string matcher after shell escaping changed, an unscoped stale-app stop against a configured-but-absent workload, an API-specific authorization exception escaping the command step, and an eventually-consistent image update being treated as permanently failed. The fix is intentionally narrow to those seams.
The latest scheduled run on current
main, 29631251499, reproduced all four issue-linked failures. Its other three failures were distinct live-service flakes: a reset post-create hook connection, a reused runner command returning status 64, and a successful image run missing expected log output.Validation
261d849efbdcc6d120a93c268f8d3bdb24f79fdeonmaine73d8483029183a3c85a176187d9ad103c23bb52.agents/bin/lint— 209 files, 0 offenses.agents/bin/docs— passed.agents/bin/validate— attempted; local integration setup stops before patched behavior becauseCPLN_ORGis unavailablecodex review --base origin/mainat the candidate head — no actionable regressionsDecision log
app_workloadsandadditional_workloads; making either optional is outside issue Scheduled slow suite failing #409.QA Evidence
Status: exact-current-head hosted and independent gates in progress.
Summary by CodeRabbit
Bug Fixes
Documentation