fix(cli): send the cleanup percent in the policy target the server reads - #2937
Conversation
`ocx storage policy set --percent N` serialized a top-level `percent` field. The
PUT contract has no such field: `normalizeStorageCleanupPolicy` reads only
`target`, so the value was dropped and the previously stored target survived.
The request still answered 200 with a policy body, so the operator saw success
while the stored target was unchanged. On a policy holding the 25% default,
`--percent 10` left cleanup authorized to remove considerably more data than was
asked for.
Send `target: { removeOldestPercent: N }` instead. Out-of-range values are still
forwarded so the server answers with its named 400 rather than the CLI
duplicating the 1-100 vocabulary; a rejected write is the correct outcome, and
the silent accepted write is what this removes.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
✅ Deterministic PR hygiene checks passed. |
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request is already Ready for Review. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe storage policy CLI now sends ChangesStorage policy percent update
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The CLI now sends the cleanup percentage in the policy target the server reads, preventing successful no-op updates while preserving server-side range validation; no actionable merge-blocking risk remains. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Title checkExplanation The title clearly and concisely describes the main change: sending the CLI cleanup percentage inside the policy target field that the server reads. It matches the changes in src/cli/storage.ts and the related regression tests.
✨ 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 |
리뷰 · 우선순위 73 / 80이 PR은 지금 쉽게 말하면 이렇다. 저장소 자동 정리 정책은 "오래된 보관 세션을 몇 퍼센트까지 지울지"를 지금 HEAD 의 대시보드는 이미 맞는 모양이다. 고치는 방법은 한 줄이다. 라인 176 - 고친 뒤 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Approved exact head 69aec67 against dev@d882caed5eb212bf5737d3cb0022dace2dab418e.
This fixes a real accepted-no-op: the CLI sent percent at the top level while the management contract reads target.removeOldestPercent. The patch is limited to the CLI serializer and focused regression coverage, preserves server-owned range validation, and introduces no security-boundary or configuration-schema change.
My isolated review evidence remains green: 23/23 focused CLI/storage-policy tests and typecheck. Exact-head Cross-platform CI, React Doctor, macOS, storage-policy, API-usage, keyring, npm-global, hygiene, enforce-target, and CodeRabbit checks are all successful; there are no unresolved review threads.
3 participants
Summary
ocx storage policy set --percent Nastarget: { removeOldestPercent: N }, the shape the cleanup-policy PUT route actually reads.The defect
The CLI serialized a top-level
percentfield. That field is not part of the PUT contract:parseStorageCleanupPolicyInputspreads the body, butnormalizeStorageCleanupPolicyreads the percent only fromtarget, so an absenttargetkey leaves the previously stored target in place.The request still returned 200 with a policy body, so the operator saw success while nothing changed. On a policy holding the 25% default,
--percent 10reported success and left cleanup authorized to remove considerably more data than was requested. Reducing the percent is the direction an operator reaches for when they want to delete less, which is what makes the silent no-op consequential rather than cosmetic.Scope
Client-side only. No server contract, validation vocabulary, or policy semantics change, and omission behavior for
enabled,mode, andscheduleis untouched.An out-of-range percent is still forwarded rather than rejected locally. The server owns the 1-100 range and answers with a named 400; duplicating that vocabulary in the CLI would be a second thing to keep in sync, and a refused write is already the correct outcome. The failure being removed here is the silent accepted one.
Verification
dev@d882caed5eb2.1.4.0+34cbb9a40,tests/cli-storage-inspect.test.ts:21passed,0failed.3tests fail — the updated shape assertion plus both new regressions.1.4.0+34cbb9a40, withtests/storage-policy-config-race.test.ts:23passed,0failed (68expectations).bun run typecheck: passed.bun run privacy:scan: passed.Checklist
--percentusage is unchanged; only the wire shape it produces is corrected.Review readiness checklist
Summary by CodeRabbit
Bug Fixes
Tests