Repository navigation
feat(helm): auto-roll deployment on agent-config change (checksum annotation) - #606
Merged
Merged
Conversation
…ation Subpath-mounted ConfigMap changes do not refresh in a running pod and the Deployment had no config-checksum annotation, so config edits required a manual `kubectl rollout restart`. Add the standard checksum/config pod annotation hashing configmap-agent-config.yaml so any config change triggers a rollout automatically. Pre-existing podAnnotations still merge in. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
Warning Review limit reached
More reviews will be available in 35 minutes and 53 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe Deployment pod template annotations block is updated to always emit a ChangesConfigMap Checksum Annotation
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
coding-agent/tests/deploy/test_helm_chart.py (1)
181-193: ⚡ Quick winAdd a regression check for
podAnnotations["checksum/config"]collision behavior.This test validates checksum shape/sensitivity, but it doesn’t assert precedence when users set
podAnnotations.checksum/config. Adding that case will lock in the rollout contract and catch key-collision regressions.🤖 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 `@coding-agent/tests/deploy/test_helm_chart.py` around lines 181 - 193, The test function test_helm_agent_config_checksum_annotation_changes_with_config currently only validates the format and sensitivity of the auto-generated checksum/config annotation, but does not test precedence when users explicitly set podAnnotations.checksum/config themselves. Extend this test by adding a third _render() call that includes a --set flag to explicitly set podAnnotations.checksum/config to a specific value, then extract the annotations from that rendered deployment and assert that the user-provided value takes precedence over any auto-generated checksum annotation. This will ensure the rollout contract is locked in and catch future key-collision regressions.
🤖 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 `@coding-agent/helm/templates/deployment.yaml`:
- Around line 17-20: The checksum/config annotation computed on line 17 can be
overridden by user-provided values in podAnnotations (lines 18-19), breaking the
automatic rollout guarantee. Fix this by reordering the annotations so the
computed checksum/config value is rendered after the podAnnotations block,
ensuring the computed checksum cannot be overridden by user input.
Alternatively, filter out any checksum/config key from the podAnnotations before
rendering with toYaml.
---
Nitpick comments:
In `@coding-agent/tests/deploy/test_helm_chart.py`:
- Around line 181-193: The test function
test_helm_agent_config_checksum_annotation_changes_with_config currently only
validates the format and sensitivity of the auto-generated checksum/config
annotation, but does not test precedence when users explicitly set
podAnnotations.checksum/config themselves. Extend this test by adding a third
_render() call that includes a --set flag to explicitly set
podAnnotations.checksum/config to a specific value, then extract the annotations
from that rendered deployment and assert that the user-provided value takes
precedence over any auto-generated checksum annotation. This will ensure the
rollout contract is locked in and catch future key-collision regressions.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7a49a143-95e2-4e45-8a4c-a7b1a8494cd3
📒 Files selected for processing (2)
coding-agent/helm/templates/deployment.yamlcoding-agent/tests/deploy/test_helm_chart.py
Render the computed checksum/config annotation after .Values.podAnnotations so a user-supplied checksum/config key cannot override it and silently defeat the auto-rollout guarantee. Add a precedence regression test. Addresses CodeRabbit review on #606. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
delete-cloud
added a commit
that referenced
this pull request
Jun 16, 2026
… GitOps (0071) (#608) * docs(adr): propose restart-safe sessions (0070) + ArgoCD GitOps (0071) ADR-0070: wire the existing durable layer (ADR-0055/0067/0068) into a restart-recovery closed loop so single-replica live HTTP sessions survive a process restart — create-time persistence (P1), graceful drain (P2), boot rehydration + interrupted-turn resume (P3). Multi-replica HA explicitly out of scope. Motivated by live session loss on REVISION 6 rollout + the #606 config-checksum auto-roll putting restart on the hot path. ADR-0071: adopt ArgoCD pull-based GitOps as the target deploy mechanism (private values repo, immutable :sha promotion via Image Updater, manual sync with diff preview), replacing manual SSH helm upgrade. Includes the deployment contract. P2 graceful drain is the prerequisite. Both Proposed; implementation pending. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(adr): correct 0070 root cause to idle-GC destructive delete Independent code review (and codex re-review) found ADR-0070's original P1 premise wrong: POST /sessions already persists synchronously at create (session_manager.py:2754), and neither shutdown nor startup deletes session rows. The real cause of the lost durable session row is the 30-min idle GC: cleanup_idle_sessions -> close_session -> _remove_session_async -> delete_session destructively deletes durable sessions. Restart-independent. Rewrite 0070 around that: D1 (primary) idle GC must only shut down runtime + release lease, never delete durable metadata/tape/runs/checkpoints — destructive delete only on explicit user/admin close. P2 kept as in-flight-turn graceful- drain hardening (not the row-loss cause). P3 downgraded to optional runtime rehydration/resume (no promise to recover a killed turn process). Drop the create-time-persistence proposal (already implemented). Minor 0071 fixes: dependency wording -> 'D1 idle-GC fix + D2 graceful drain'; replace the nonexistent test_deploy_script_fails_closed_on_placeholder_values with the real apply-mode placeholder-rejection test names. Docs only; no runtime code changes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(adr): address codex review nits on 0070/0071 - 0070: restart surfaces (not immediately triggers) the loss — cleanup task sleeps 60s before first pass. - 0070 D1: add ownership note — if idle cleanup releases the owner lease, the next prompt must reacquire (current path only _assert_owners). - 0070 D3: resume is best-effort from durable history/context per ADR-0055, not a checkpoint rollback (was overstated). - 0070 acceptance: fix test path tests/coding_agent -> tests/ui. - 0071: separate the idle-GC row-loss cause (D1) from the drain gap (D2); drop the claim that manual SSH 'caused' the session loss (idle GC did). Docs only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(adr): address CodeRabbit nits on 0071 acceptance criteria - Clarify the ArgoCD sync namespace: coding-agent-deepseek is the o6n-prod override (chart/deploy-script default is coding-agent), not a hardcoding error. - Rephrase the bare pytest command into a full acceptance-criterion sentence. Docs only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
delete-cloud
added a commit
that referenced
this pull request
Jun 16, 2026
So a rolling-update/restart (incl. the #606 config-checksum auto-roll) drains in-flight turns instead of being SIGKILLed. Helm: values-driven lifecycle — terminationGracePeriodSeconds (default 60, was the k8s default 30) + a gated preStop sleep (preStopSleepSeconds default 5, 0 disables). Rendered in deployment.yaml. App: graceful shutdown previously cancelled the in-flight turn (run status cancelled, indistinguishable from a user cancel) and startup recovery only rewrites still-running runs to interrupted, so the run stayed cancelled. lifespan now calls shutdown_session_runtime(interrupt_active_turn=True); that path rewrites the active stopped run to interrupted with reclaimable metadata. Idle cleanup keeps the default (no interrupt marking). Tests: test_helm_deployment_sets_graceful_drain; test_lifespan_shutdown_marks_active_turn_interrupted. Gate: 388 passed. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
SubPath-mounted ConfigMap changes do not refresh in a running pod, and the Deployment carried no config-checksum annotation. So editing agent config (e.g.
kb.maxDistance,shell.sandboxMode) required a manualkubectl rollout restartto take effect — a footgun we hit during the o6n Phase C cutover.What
checksum/configpod annotation hashing the renderedconfigmap-agent-config.yaml. Any config change now changes the checksum → Deployment rolls automatically.spec.template.metadata.annotationsnow always renders; pre-existing.Values.podAnnotationsstill merge in.Test
uv run pytest tests/deploy/test_helm_chart.py -k 'checksum or helm_chart_lints'→ 2 passed.helm templaterenders the annotation correctly with empty podAnnotations.🤖 Generated with Claude Code
Summary by CodeRabbit
Chores