🐛 fix(agent): stop agent reports from clearing controller-set update policy overrides - #624
Conversation
…policy overrides
Agent watcher normalization stamps updatePolicyOverrides={} on every
container it reports; the controller persisted that empty layer verbatim,
wiping controller-set runtime overrides (maturity mode/min-age, skip
lists, snooze) on every agent sync or manual recheck.
- 🐛 fix(agent): reapply the controller's stored overrides in
buildContainerReport before persisting agent reports
- 🐛 fix(store): treat a present-but-empty updatePolicyOverrides layer as
carrying no intent in updateContainer unless the caller passes
authoritativeEmptyOverrides (update-policy PATCH clears still stick)
- ✅ test: regression coverage for maturity/skip/snooze preservation,
clear-then-report stickiness, and PATCH clear authority
Refs: #565
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughContainer override merging now distinguishes authoritative empty layers from empty agent reports, preserving stored controller overrides unless explicit authority is provided. The container API propagates this option, and unsnooze marks cleared layers as authoritative. Agent report ingestion reapplies existing controller-owned policy overrides before persistence. Tests cover maturity, skip/snooze, stale agent values, authoritative clearing, undefined layers, and first writes. Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/store/container.ts (1)
1142-1150: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated "empty-layer is non-authoritative" rule — consider a shared predicate.
The same rule ("a present-but-empty
updatePolicyOverrideslayer carries no controller intent") is now implemented twice: here and inrestoreRetainedUpdatePolicy(Lines 1044-1049), which even comments that it "mirrors the rule ... above." Two independent implementations of the same policy invite drift if one is tweaked later without the other (which is exactly the class of bug#565fixes).♻️ Suggested consolidation
+function isAuthoritativeOverrideLayer( + overrides: unknown, + authoritativeEmptyOverrides: boolean, +): boolean { + return authoritativeEmptyOverrides || Object.keys(overrides ?? {}).length > 0; +}Then use
isAuthoritativeOverrideLayer(container.updatePolicyOverrides, options.authoritativeEmptyOverrides === true)in bothupdateContainerandrestoreRetainedUpdatePolicy.🤖 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 `@app/store/container.ts` around lines 1142 - 1150, Extract the duplicated empty-layer authority check into a shared `isAuthoritativeOverrideLayer` predicate, accepting the overrides layer and the authoritative-empty flag. Replace the inline logic in both `updateContainer` and `restoreRetainedUpdatePolicy`, while preserving the distinction between absent, non-empty, and explicitly authoritative empty overrides.
🤖 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 `@app/store/container.ts`:
- Around line 1142-1150: Extract the duplicated empty-layer authority check into
a shared `isAuthoritativeOverrideLayer` predicate, accepting the overrides layer
and the authoritative-empty flag. Replace the inline logic in both
`updateContainer` and `restoreRetainedUpdatePolicy`, while preserving the
distinction between absent, non-empty, and explicitly authoritative empty
overrides.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d8279893-d99f-4cb1-a633-6666d797801a
⛔ Files ignored due to path filters (1)
CHANGELOG.mdis excluded by!CHANGELOG.md
📒 Files selected for processing (7)
app/agent/AgentClient.test.tsapp/agent/AgentClient.tsapp/api/container.tsapp/api/container/update-policy.test.tsapp/api/container/update-policy.tsapp/store/container.test.tsapp/store/container.ts
…ween update and insert paths CodeRabbit review: the '#565 empty layer carries no intent' rule was implemented independently in updateContainer and restoreRetainedUpdatePolicy; extract isAuthoritativeOverrideLayer so the two paths cannot drift.
1 participant
Problem
Agent-managed containers lose their controller-set update policy overrides (maturity mode/min-age days, skip lists, snooze) on every agent report. The agent resolves its own declarative (env/label) policy but never learns controller-side runtime overrides, so every container report it sends carries an explicit
updatePolicyOverrides: {}layer. The controller persisted that layer verbatim, and the store'supdateContainertreats any presentupdatePolicyOverrideskey as authoritative — even when empty. Any manual recheck or periodic agent sync wiped the settings.This is the settings-deletion half of #565. The soak-clock resets in that thread were real but separate mechanisms, fixed in #566/#568 and the rc.7 identity batch (#607). The reporter runs the controller-agent architecture (confirmed in their own words in #496, reconfirmed in #623), which is why the symptom survived all four prior fixes: those fixed the clock, this deletes the actual override values — matching the original before/after screenshots.
The insert/recreate path has had exactly this protection since #496/#497 (
restoreRetainedUpdatePolicy: "an empty layer carries no controller intent"); the same-idupdateContainerpath never got it.Fix (two layers)
AgentClient.buildContainerReportreapplies the controller's stored runtime overrides onto the incoming agent report before persistence. Covers all five agent ingestion call sites (handshake, single-container, pending watcher-cycle, bulk reports, recheck) — they all funnel through this one method.storeContainer.updateContainernow encodes the invariant directly, mirroring the insert-path rule: a present-but-empty override layer carries no intent and falls back to the stored overrides, unless the caller passesauthoritativeEmptyOverrides: true. The update-policy PATCH handler passes it, so deliberate clears from the UI still stick.Safety was verified adversarially before landing: overrides can only be set via the controller's PATCH endpoint, there is no controller→agent policy push that could echo stale values back, expired snoozes can't be resurrected (evaluated live at read time), and the local (non-agent) watcher path already preserves overrides by construction (it mutates the live store doc).
Tests
Full app suite: 394 files / 12,395 tests, 100% coverage. Full lefthook pre-push gate green.
Refs #565
Changelog
authoritativeEmptyOverridessupport for intentional clears from update-policy PATCH requests.Validation