Skip to content

fix(config): reject >3 fuse phases and duplicate site meters - #844

Merged
frahlg merged 1 commit into
srcfl:masterfrom
Sanjin-Maker:agent/config-validate-phases-sitemeter
Aug 16, 2026
Merged

fix(config): reject >3 fuse phases and duplicate site meters#844
frahlg merged 1 commit into
srcfl:masterfrom
Sanjin-Maker:agent/config-validate-phases-sitemeter

Conversation

@Sanjin-Maker

Copy link
Copy Markdown
Contributor

Summary

  • fuse.phases: 4+ was accepted and then silently truncated to 3 by the dispatch freshness gate (site_dispatch_safety.go), while Fuse.MaxPowerW() kept multiplying by the configured phase count — the operator believes the extra phases are protected when they aren't. Now rejected at Validate() with fuse.phases must be 1, 2 or 3.
  • A second is_site_meter: true driver was silently ignored (SiteMeterDriver() returns the first match) — dispatch could be trusting a different meter than the operator intends. Now rejected with an explicit count in the message.
  • The defensive truncation in the freshness gate is left in place as belt-and-braces.

Both are load-time failures for configs that were already broken in surprising ways; the changeset calls this out.

Verification

  • go test ./internal/config/ -count=1 — new validate_site_test.go covers 1–3 phases accepted, 4 rejected, single site meter accepted, duplicate rejected.
  • go build ./..., plus ./cmd/ftw/ and ./internal/control/ suites green.

🤖 Generated with Claude Code

@Sanjin-Maker
Sanjin-Maker force-pushed the agent/config-validate-phases-sitemeter branch from ffbad9a to 79bb695 Compare August 6, 2026 20:09
@frahlg

frahlg commented Aug 7, 2026

Copy link
Copy Markdown
Member

Maintainer review: the validation change is small and the tests cover both failure cases. I am holding the PR rather than rewriting or merging it now because older open PRs #728, #732, #735, #736, and #797 also change go/internal/config/config.go and have file right of way under the repository rules. This branch is also behind current master and has no current check run. Once the older config work is resolved, this is a good candidate to rebase, rerun, and merge as an isolated change.

@miravoss26 miravoss26 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rejects fuse.phases > 3 (previously silently truncated to 3 by the freshness gate while MaxPowerW() kept using the configured value) and rejects a second is_site_meter: true driver (previously the first match silently won). Both are load-time validation errors for configs that were already broken in surprising ways.

  • Clean, isolated change. Tests cover all four new branches (1-3 phases accepted, 4 rejected, single/duplicate site meter).
  • No secrets, no injection surface, no new deps, no network changes.

Safe to merge from my read.

frahlg commented Aug 8, 2026

Copy link
Copy Markdown
Member

Thank you @Sanjin-Maker. This is a focused fix with a clear failure mode and useful tests, and we want to keep it open.

Several older open PRs already own go/internal/config/config.go (#728, #732, #735, #736 and #797), so they have right of way. Please do not rebase yet and do not stack more work on this branch. Once those owners are resolved, we will ask for a clean rebase onto current master, CI and the same narrow scope.

We appreciate this contribution and will keep you included as the blocker clears.

frahlg commented Aug 16, 2026

Copy link
Copy Markdown
Member

Update to the Aug 8 hold: I rechecked #728, #732, #735, #736, and #797, plus the newer #914/#915, including all reviews and inline threads. #844 only adds two checks in Config.Validate: reject duplicate site meters and fuse.phases > 3. None of those PRs changes either hunk or adds a fixture that these checks reject.

#728 is the only older config PR with current review work; it is behind but mergeable. #732/#735/#736 are conflicted drafts, and #797 is paused with failing Windows CI. #914/#915 are newer and have unresolved P1 threads.

I propose giving this narrow safety fix the next rebase-and-CI slot. @HuggeK, please confirm that landing these two checks first will not disrupt your open branches. Once confirmed, @Sanjin-Maker may rebase onto current master and rerun CI with the same narrow scope. This is not merge approval; we still need the rebased checks to pass.

fuse.phases above 3 was silently truncated to 3 by the dispatch
freshness gate while MaxPowerW kept multiplying by the configured
count, and a second is_site_meter driver was silently ignored in
favor of the first. Both misconfigurations now fail at load with a
clear message.

Co-Authored-By: Claude Fable 5 <[email protected]>
Signed-off-by: Sanjin Naidu <[email protected]>
@frahlg
frahlg force-pushed the agent/config-validate-phases-sitemeter branch from 79bb695 to 7713d17 Compare August 16, 2026 18:29
@frahlg
frahlg requested a review from miravoss26 August 16, 2026 18:29
@frahlg

frahlg commented Aug 16, 2026

Copy link
Copy Markdown
Member

Rebased onto current master 098a0951 without expanding scope.

Head: 7713d17d. The diff remains the two startup validation errors, tests and one patch changeset (3 files, 94 added lines).

Local checks passed: go test ./internal/config -count=100, go test -race ./internal/config -count=20, and make verify. CI is running.

@miravoss26 Please give this rebased config-safety change a fresh review when CI is green. There is no CODEOWNER entry for go/internal/config.

@frahlg

frahlg commented Aug 16, 2026

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 👍

Reviewed commit: 7713d17d74

ℹ️ 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".

@frahlg
frahlg merged commit c86c9e8 into srcfl:master Aug 16, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants