refactor(daemon): extract SkillAutoReleaseSettings — AceClawConfig decomposition batch 1#506
Merged
Merged
Conversation
…onfig sub-record
AceClawConfig had grown to 152 instance fields + 85 getters — by far the
widest single class in the repo, a database-schema-masquerading-as-a-class
that's been the central catch-all for every daemon-side knob. This is the
first pilot of the decomposition: lift one thematic cluster into its own
record, leave the rest alone for now.
Pilot target: the 12 skill-auto-release scalars (`skillAutoRelease*`).
Chosen first because:
- biggest single cohesive cluster (12 fields / ~130 LoC of env var loading
alone)
- consumer (AutoReleaseController in aceclaw-learning) already has its own
Config record — reuse it as the tuning payload instead of inventing a new
one
- single call site in AceClawDaemon: 11 inline scalars going to a
constructor — perfect surface for "pass the whole record" simplification
## Shape
```
SkillAutoReleaseSettings(boolean enabled, AutoReleaseController.Config tuning)
```
`enabled` is the feature gate (was `skillAutoReleaseEnabled`); `tuning`
groups the 11 canary / rollback scalars the controller already accepts as
a record. The settings type has a `Builder` for incremental population
during file/env load (kept package-private; only AceClawConfig uses it).
## What moved
| Was in AceClawConfig | Now |
|---|---|
| 13 `DEFAULT_SKILL_AUTO_RELEASE_*` constants | `SkillAutoReleaseSettings.defaults()` |
| 12 instance fields (`skillAutoRelease*`) | 1 builder field |
| 12 constructor-default assignments | none (builder seeds defaults) |
| 12 individual getters | 1 getter (`skillAutoRelease()`) |
| ~130 LoC of env-var try/catch boilerplate | builder.xxx(parsedValue) per env var |
| ~55 LoC of `if (fileConfig.x != null) { ... }` blocks | single builder chain |
## Net diff
| File | LoC change |
|---|---|
| AceClawConfig.java | **2038 → 1879 (-159, -8%)** |
| AceClawDaemon.java | -10 |
| SkillAutoReleaseSettings.java (new) | +139 |
The 139 LoC of the new record is mostly per-field setters on the builder
(needed for the null-safe / range-checked merge) and the defaults factory.
Net file-system delta is roughly zero, but the wide-class surface of
AceClawConfig shrinks by 12 fields + 12 getters — the real win.
## Bonus: applyEnv{Boolean,Int,Double} helpers
Pulled three small helpers out of the env-var-loading boilerplate (was
inlined ~50x across the load() method). Used by the new skillAutoRelease
loaders; future thematic extractions will reuse them. Each handles
null/blank + parse-failure-with-warning centrally. Same observable
behaviour as the inlined version.
## Backward compat
- Config file JSON shape unchanged. Same flat top-level keys
(`skillAutoReleaseEnabled`, `skillAutoReleaseMinCandidateScore`, …).
Nesting under a `skillAutoRelease` object is a future option; not in
this PR to keep the diff narrow.
- All 14 env var names preserved (including the legacy
`ACECLAW_SKILL_AUTO_RELEASE_ACTIVE_MAX_FAILURE_RATE` alias mapping onto
`rollbackMaxFailureRate`).
- AceClawConfigTest passes unchanged (17/0).
## Test plan
- [x] :aceclaw-daemon:compileJava
- [x] :aceclaw-daemon:compileTestJava (-Pno-dashboard to skip unrelated
npm build)
- [x] AceClawConfigTest (17 tests, 0 failures) — covers config file
loading, env var overrides, persistence, and the security.denySensitivePaths
toggle added previously
- [x] No call site changes outside the single AceClawDaemon
AutoReleaseController construction
## Pattern for follow-up batches
Plan was 2-3 PRs over a long weekend. Next thematic clusters teed up:
SkillDraftValidation (5 fields), AdaptiveContinuation (5), Candidate
injection/promotion (7), Retry (4), AgentBudgets + PlanBudgets (6),
AntiPatternGate (2). Each follows the same pattern: small record (+ Builder
when there are >5 fields), single call site in AceClawDaemon migrated to
pass the record, individual getters removed.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
📝 WalkthroughWalkthroughThis PR consolidates 13 scattered skill auto-release configuration fields and public getters from ChangesSkill Auto-Release Settings Consolidation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 7 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (7 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
![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: 4e8f93f0b5
ℹ️ 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".
Comment on lines
+588
to
+589
| applyEnvInt("ACECLAW_SKILL_AUTO_RELEASE_MIN_EVIDENCE", | ||
| v -> config.skillAutoReleaseBuilder.minEvidenceCount(Math.max(1, v))); |
There was a problem hiding this comment.
When ACECLAW_SKILL_AUTO_RELEASE_MIN_EVIDENCE is set to 0, this path now coerces it to 1 via Math.max(1, v), whereas the previous implementation accepted 0 (it used Math.max(0, parsed)). That changes runtime behavior for env-based deployments by making auto-release stricter than configured, and it contradicts the refactor goal of preserving existing semantics.
Useful? React with 👍 / 👎.
Comment on lines
+96
to
+100
| Builder canaryMaxFailureRate(Double v) { if (v != null && v >= 0) this.canaryMaxFailureRate = clampRate(v); return this; } | ||
| Builder canaryMaxTimeoutRate(Double v) { if (v != null && v >= 0) this.canaryMaxTimeoutRate = clampRate(v); return this; } | ||
| Builder canaryMaxPermissionRate(Double v) { if (v != null && v >= 0) this.canaryMaxPermissionRate = clampRate(v); return this; } | ||
| Builder rollbackMaxFailureRate(Double v) { if (v != null && v >= 0) this.rollbackMaxFailureRate = clampRate(v); return this; } | ||
| Builder rollbackMaxTimeoutRate(Double v) { if (v != null && v >= 0) this.rollbackMaxTimeoutRate = clampRate(v); return this; } |
There was a problem hiding this comment.
These setters now ignore negative numeric values because of the v >= 0 guard, but the pre-refactor env parsing path fed parsed doubles directly through clampRate, so negative values were normalized to 0.0. With this change, an env override like -1 no longer tightens a threshold to zero and instead leaves the previous value in place, which is a silent behavior change for existing env-driven configs.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
aceclaw-daemon/src/main/java/dev/aceclaw/daemon/SkillAutoReleaseSettings.java (1)
27-27: ⚡ Quick winEnforce non-null
tuningat record boundary.Because this record is public, allowing
tuningto be null pushes failure to later dereferences. Fail fast in the record constructor.♻️ Proposed fix
public record SkillAutoReleaseSettings(boolean enabled, AutoReleaseController.Config tuning) { + public SkillAutoReleaseSettings { + tuning = java.util.Objects.requireNonNull(tuning, "tuning"); + }As per coding guidelines: "Use
Objects.requireNonNull(param, "param")on method parameters used in.equals()or passed to downstream calls".🤖 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 `@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/SkillAutoReleaseSettings.java` at line 27, The public record SkillAutoReleaseSettings currently allows a null tuning which defers failures; add a compact canonical constructor to the record that calls Objects.requireNonNull(tuning, "tuning") to fail fast and enforce non-null; update imports to include java.util.Objects if missing and ensure the constructor is declared inside the SkillAutoReleaseSettings record definition so all instantiations validate tuning at creation.aceclaw-daemon/src/main/java/dev/aceclaw/daemon/AceClawConfig.java (1)
1854-1878: ⚡ Quick winNull-guard helper parameters before downstream use.
These helpers pass
name/sinkdirectly to downstream calls; add explicitrequireNonNullguards to keep failures deterministic and aligned with project conventions.♻️ Proposed fix
private static void applyEnvBoolean(String name, java.util.function.Consumer<Boolean> sink) { + java.util.Objects.requireNonNull(name, "name"); + java.util.Objects.requireNonNull(sink, "sink"); var raw = System.getenv(name); if (raw == null || raw.isBlank()) return; sink.accept(Boolean.parseBoolean(raw)); } private static void applyEnvInt(String name, java.util.function.IntConsumer sink) { + java.util.Objects.requireNonNull(name, "name"); + java.util.Objects.requireNonNull(sink, "sink"); var raw = System.getenv(name); if (raw == null || raw.isBlank()) return; try { sink.accept(Integer.parseInt(raw)); @@ private static void applyEnvDouble(String name, java.util.function.DoubleConsumer sink) { + java.util.Objects.requireNonNull(name, "name"); + java.util.Objects.requireNonNull(sink, "sink"); var raw = System.getenv(name); if (raw == null || raw.isBlank()) return; try { sink.accept(Double.parseDouble(raw));As per coding guidelines: "Use
Objects.requireNonNull(param, "param")on method parameters used in.equals()or passed to downstream calls".🤖 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 `@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/AceClawConfig.java` around lines 1854 - 1878, Add explicit null checks at the start of the helper methods to match project conventions: in applyEnvBoolean, applyEnvInt, and applyEnvDouble call Objects.requireNonNull(name, "name") and Objects.requireNonNull(sink, "sink") before any downstream use (e.g., before System.getenv and before invoking sink.accept) so parameter nulls fail fast and deterministically; keep the existing parsing and warning behavior unchanged.
🤖 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 `@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/AceClawConfig.java`:
- Around line 1854-1878: Add explicit null checks at the start of the helper
methods to match project conventions: in applyEnvBoolean, applyEnvInt, and
applyEnvDouble call Objects.requireNonNull(name, "name") and
Objects.requireNonNull(sink, "sink") before any downstream use (e.g., before
System.getenv and before invoking sink.accept) so parameter nulls fail fast and
deterministically; keep the existing parsing and warning behavior unchanged.
In
`@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/SkillAutoReleaseSettings.java`:
- Line 27: The public record SkillAutoReleaseSettings currently allows a null
tuning which defers failures; add a compact canonical constructor to the record
that calls Objects.requireNonNull(tuning, "tuning") to fail fast and enforce
non-null; update imports to include java.util.Objects if missing and ensure the
constructor is declared inside the SkillAutoReleaseSettings record definition so
all instantiations validate tuning at creation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6d2b4e71-a083-4c6f-bbb7-ffc92df334d3
📒 Files selected for processing (3)
aceclaw-daemon/src/main/java/dev/aceclaw/daemon/AceClawConfig.javaaceclaw-daemon/src/main/java/dev/aceclaw/daemon/AceClawDaemon.javaaceclaw-daemon/src/main/java/dev/aceclaw/daemon/SkillAutoReleaseSettings.java
This was referenced May 23, 2026
Merged
xinhuagu
added a commit
that referenced
this pull request
May 24, 2026
Two findings, both real: ## Bug — AntiPatternGateSettings.maxFalsePositiveRate negative-value clamp regression Pre-decomp env-var path called `clampRate(parsed)` unconditionally, so a negative env value (e.g. `ACECLAW_ANTI_PATTERN_GATE_MAX_FALSE_POSITIVE_RATE=-0.1`) became 0.0 rather than being silently ignored. The `v >= 0` guard in the new Builder.maxFalsePositiveRate diverged from that — Codex P2 + CodeRabbit minor both flagged it. Fix: any non-null input is now clamped to [0, 1] before being stored, matching the pre-decomp env-var behaviour. File-merge behaviour also unified to clamp (was skip-then-clamp before; now consistent). The same regression pattern (rate fields gated by `v >= 0` instead of clamping unconditionally) likely exists in SkillAutoReleaseSettings' 7 rate fields, already merged in #506. Deferred to a follow-up audit PR so this PR stays narrow. ## Nitpick — Builder seed null-guards (3 records) Per project CLAUDE.md "Use Objects.requireNonNull(param, "param") on method parameters used in .equals() or passed to downstream calls": - AntiPatternGateSettings.Builder(seed) - CandidatePromotionSettings.Builder(seed) - CandidateInjectionSettings.Builder(seed) All three dereference `seed` immediately. Adding the guard so a null gets a clear "seed" NPE message instead of a confusing one at the first accessor call. SkillAutoReleaseSettings / AdaptiveContinuationSettings / SkillDraftValidationSettings (already-merged Builders) have the same shape — same follow-up PR. ## Test plan - [x] AceClawConfigTest 17/0 (covers env-var loading) - [x] Build clean Co-Authored-By: Claude Opus 4.7 (1M context) <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
1 participant
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.
Summary
First pilot of the AceClawConfig decomposition. The class had grown to
152 instance fields + 85 getters — a database-schema-masquerading-as-a-class
that's been the central catch-all for every daemon-side knob.
This PR carves out one thematic cluster (skill auto-release, 12 fields)
into its own record. Same pattern that pulled
aceclaw-learningout ofdaemon: extract a cohesive slice first, validate the pattern works, then
batch the rest.
AceClawConfig.javaAceClawConfiginstance fieldsAceClawConfiggettersPilot target: SkillAutoRelease (12 fields)
Chosen first because:
AutoReleaseControllerinaceclaw-learning) already hasits own
Configrecord — reuse it as the tuning payload instead ofinventing a new one
AceClawDaemon(11 inline scalars to a constructor)Shape
enabledis the feature gate (wasskillAutoReleaseEnabled);tuninggroups the 11 canary/rollback scalars the controller already accepts as a
record. A
Builderlives on the record for incremental population duringfile/env load — kept package-private; only
AceClawConfiguses it.What moved
DEFAULT_SKILL_AUTO_RELEASE_*constantsSkillAutoReleaseSettings.defaults()skillAutoRelease()getterbuilder.xxx(parsed)per env varif (fileConfig.x != null)Daemon call site
Bonus: env-loading helpers
Pulled three small helpers —
applyEnvBoolean,applyEnvInt,applyEnvDouble— out of the env-var-loading boilerplate (was inlined~50× across
load()). Each handles null/blank + parse-failure-with-warningcentrally. Future thematic extractions will reuse them.
Backward compat
(
skillAutoReleaseEnabled,skillAutoReleaseMinCandidateScore, …)ACECLAW_SKILL_AUTO_RELEASE_ACTIVE_MAX_FAILURE_RATEalias mapping ontorollbackMaxFailureRate)Test plan
:aceclaw-daemon:compileJava— clean:aceclaw-daemon:compileTestJava— cleanAceClawConfigTest— 17/0 (covers config file loading, env varoverrides, persistence, security.denySensitivePaths toggle)
Follow-up batches (out of scope for this PR)
Plan: 2-3 more PRs lift remaining cohesive clusters:
RetryConfigFormatJSON companionEach follows this PR's pattern: small record (+ Builder when needed),
single call site in AceClawDaemon migrated, individual getters removed.
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Refactor