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}
|
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:
Important Approval pendingCodeRabbit has no unresolved comments, but it has not reviewed the latest commit. Use the checkbox below to review the latest commit. CodeRabbit will approve the changes if it finds no blocking issues.
WalkthroughThe changes update Controlplane workload execution, stale-app suspension, image deployment retries, job log draining, upstream error handling, sensitive-data redaction, and scheduled-suite regression coverage. ChangesReliability fixes
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to The PR improves scheduled job cleanup, polling, and credential handling, but the current implementation can poll continuously while active jobs emit logs, risking rate limits and incomplete streaming; a terminal command path may also record credential-bearing arguments without redaction, and cleanup wording remains misleading. Merge should wait for the polling fix and explicit follow-up on the bounded security and documentation risks. Sequence Diagram(s)sequenceDiagram
participant Run
participant Controlplane
participant JobLogs
Run->>Controlplane: Resolve job status
Run->>JobLogs: Poll logs during drain deadline
JobLogs-->>Run: Return changed or finished logs
Run-->>Controlplane: Resolve final exit status
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation The changes address the runtime regressions described for issue Full details: Docstring CoverageExplanation Docstring coverage is 5.41% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 37 functions across 15 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
…low-suite * origin/main: Queue all shared-org RSpec waiters (#457) # Conflicts: # CHANGELOG.md
|
Evidence for the current lane owner (no code pushed from this session): Greptile's P1 on #458 ( Path in
Smallest fix: when Also FYI: |
|
🤖 Codex fixed the terminal-recovery log-drain regression at exact head The regression test failed before the implementation change and now passes. The fix gives terminal recovery a fresh 120-second monotonic drain window while preserving the query boundary frozen during the outage, so late-ingested logs are not excluded. Validation: runner unit suite 68 examples / 0 failures (seed 6674); changed-file RuboCop clean; trusted Actions scan clean; independent corrected-head autoreview clean after 89 examples / 0 failures (seed 40988). Hosted exact-head checks are now running. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f771bc0d27
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Review summaryReviewed the diff (2076 additions, mostly Two correctness issues found in the rewritten
Both look like they'd reintroduce the same class of flakiness this PR is otherwise fixing, specifically under a status-outage-then-recovery scenario, which is plausible given issue #409's description of Control Plane's cron-status endpoint being intermittently unavailable. Lower-severity items (not inline, just noting):
Nothing security-sensitive stood out beyond the intentional redaction work described in the PR body, which looks reasonable on inspection. Test coverage for the new retry/redaction/fixture paths looks thorough per the PR's validation section. |
|
🤖 Codex completed the current-head review pass at All four new review threads are handled and resolved: three fixes were pushed (bounded log reads, finish-marker deadline upgrade, and narrow conflict matching), and the terminal-status latch was retained as intentional because Control Plane terminal status for the exact job ID is authoritative. Local closeout: focused runner/deploy suite 116 examples, 0 failures (seed 34932); RuboCop 218 files, no offenses; generated docs and diff clean; independent autoreview clean. Hosted exact-head checks and fresh automated review are now the remaining non-human gates. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d48032d3a4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Review summaryReviewed the full diff (lib/, spec/support/, and the associated unit specs). This PR is a large, well-tested hardening pass on the scheduled Slow suite: stale-app workload targeting, transient Two lower-confidence points worth a look, left as inline comments:
Everything else I traced through (the |
|
🤖 Codex completed this review pass at 497ef92. The initial-log timeout regression is fixed; the retry-policy and Thor-parser questions are answered without changing their intended behavior. This is a non-cutoff status; the next review pass should use check all reviews. Review outcomes and verification
|
Review summaryThis is a large (2.2k line) refactor of the runner/deploy/cleanup reliability logic plus test infrastructure hardening. I traced the main production-code paths in detail:
Test infra ( No security, performance, or correctness issues found beyond the one minor nit inline. Given the size and the fact that the hosted exact-head Fast/Slow validation is still pending per the PR description, I'd still want to see that pass before merge, but I found nothing in the diff itself that should block it. One process note, not a code issue: the PR description contains an embedded HTML comment block ( |
|
🤖 Codex completed the latest review pass at Review outcomes and live evidenceMattered:
Optional:
Skipped:
Scope: full review inventory fetched with no cutoff suppression; 63 threads were inventoried, with only the optional thread above unresolved before this action. This status records the latest review wave and does not replace earlier item-specific dispositions. Non-cutoff status only. The next review pass must use |
|
🤖 Codex — Justin approved this PR, and it was merged. The review and approval-request history is retained below. Agent-only review and approval history🤖 Codex: Auto-merge is paused because this PR crosses the repository's changed-line and commit-count limits, changes infrastructure/delivery and public compatibility, can cause external effects, and changes credential-handling behavior. A verified human with merge authority must approve these risks for the current commit. This is a policy/authority gate, not a reported code defect or failed check. Exact-head hosted readiness is READY as of 2026-09-04T06:26:47Z: all applicable checks passed, intentional skips remain skips, all 63 review threads are resolved, no viewer-visible pending review drafts exist, and the PR is mergeable. Strict security preflight passed. Head: Risk gates and approval format
Rollback: revert PR #413 to restore prior code. No persistent-data migration is introduced. Any workload changes already executed require separate operational recovery; a source revert alone cannot reverse them. Justin or an authorized human maintainer must review these risks and post the complete decision below as their own PR comment. This template is a request, not an approval by Codex. Any new head invalidates it. <!-- autonomous-merge-risk-decision:v1 -->
---
head_sha: 497ef920c855db6675fa8a00562e62083cab59e2
triggered_gates:
- changed-lines-limit
- commit-count-limit
- infrastructure-delivery
- irreversible-external-effect
- public-compatibility
- security-auth-privacy
rollback_disposition: Revert PR 413 to restore prior code; no data migration. Already-executed external workload effects require separate operational recovery.
decision: approve
approved_by: justin808
source: direct-user-task
evidence: Reviewed this exact head, all six triggered risk gates, passing checks and resolved reviews, and the code rollback plus external-recovery limitation; approve merge when ordinary gates remain clean.
...Evaluator: State: |
|
🤖 Codex — Justin approved the documented risks for this reviewed commit. Approval text preserved below; formatting updated at Justin’s request. Agent-only approval record<!-- autonomous-merge-risk-decision:v1 -->
---
head_sha: 497ef920c855db6675fa8a00562e62083cab59e2
triggered_gates:
- changed-lines-limit
- commit-count-limit
- infrastructure-delivery
- irreversible-external-effect
- public-compatibility
- security-auth-privacy
rollback_disposition: Revert PR 413 to restore prior code; no data migration. Already-executed external workload effects require separate operational recovery.
decision: approve
approved_by: justin808
source: direct-user-task
evidence: Reviewed this exact head, all six triggered risk gates, passing checks and resolved reviews, and the code rollback plus external-recovery limitation; approve merge when ordinary gates remain clean.
... |
1 participant
Why
The scheduled Slow suite exposed runtime and test-infrastructure failures that obscured real regressions and could leak test capacity: stale-app cleanup could target the wrong app, temporary authentication and process captures could survive failures, transient Control Plane state was treated as permanent, delayed one-off logs could disappear, and a half-created shared fixture could be reused across many examples.
What changed
Addresses #409. The issue remains open until a natural scheduled Slow run succeeds on merged
main.Validation
Exact head:
497ef920c855db6675fa8a00562e62083cab59e2Integrated base:
5ff40c95d6f9284f2846968ade82d355ad692937git diff --checkclean; independent autoreview clean9ff35e66finished with 10 failures: nine missing-log/Loki-ingestion symptoms also observed onmainor older heads amid shared quota pressure, plus one unrelated PostgreSQL disconnect; a fresh natural scheduled Slow green remains the post-merge gateThe complete credentialed suite cannot run locally without
CPLN_ORGand Control Plane credentials. Hosted exact-head validation remains a merge gate.Checklist
Agent details
Final review follow-up
QA evidence
cpf-wave2-pr413-maker-r2; active numeric claim for PR Fix scheduled slow-suite regressions #413.497ef920c855db6675fa8a00562e62083cab59e2locally against base5ff40c95d6f9284f2846968ade82d355ad692937.Coordination and scope boundary
cpf-20260901-1219-wave2.codex/409-scheduled-slow-suite.main; Follow-up: Exercise shared-org RSpec queueing from PR #457 #403 owns the separate 180-minute workflow bound and shared-queue evidence.