Skip to content

chore: align DEFAULT_HARD_CAP_MS baseline with computed-default hard cap (currently coincidental) #20

Description

@skwid138

Context

Surfaced as an Unrelated Observation by claude-opus-4.6 during council review of the #17 refactor planning. Filed separately because it is genuinely unrelated to the #17 dead-branch cleanup and deserves its own scoping conversation.

Note: no chore or refactor label exists in this repo; leaving unlabeled. Closest existing label would be enhancement.

The latent fragility

In src/config.ts the exported DEFAULT_HARD_CAP_MS constant is computed from only two of the three phase timeouts plus the 30s buffer:

// src/config.ts:12-14
export const COUNCILLOR_TIMEOUT_MS = 270_000;
export const AGGREGATOR_TIMEOUT_MS = 120_000;
export const DEFAULT_HARD_CAP_MS = COUNCILLOR_TIMEOUT_MS + AGGREGATOR_TIMEOUT_MS + 30_000;
// = 420_000

The runtime computedHardCapMs includes three phase timeouts plus the buffer:

// src/config.ts:131-132
const computedHardCapMs =
  councillorMs + aggregatorMs + quorumGraceMs + 30_000;

These two expressions only produce the same number because the default quorum_grace_ms is currently 0:

// src/config.ts:130
const quorumGraceMs = readNonNegativeMs(timeoutSource, "quorum_grace_ms", 0);

If a future change updates the quorum_grace_ms default to any non-zero value (or another phase timeout is added, e.g. a "warmup" or "settle" window), DEFAULT_HARD_CAP_MS will silently drift away from "hard cap with all-default phase timeouts" — its only honest meaning today.

Why this matters

After #17 lands, DEFAULT_HARD_CAP_MS no longer participates in any runtime decision branch — it is purely a documentation constant representing the all-defaults baseline. Its single remaining responsibility is to be the number it claims to be.

If it silently diverges from the all-defaults computation:

  1. Test scaffolding misleads. src/config.test.ts line 242 asserts default timeouts.hard_cap_ms equals DEFAULT_HARD_CAP_MS. After a default-quorum_grace_ms bump, this test would fail in a way that points at the constant, not at the (likely intentional) default change.
  2. External consumers mislead. DEFAULT_HARD_CAP_MS is exported. Anyone importing it for documentation or fallback purposes would read a stale value.
  3. Code review confusion. Someone reading the constant declaration today cannot tell from the source that + quorumGraceMs is omitted because the default is 0 versus because it was forgotten.

Options

Option A — Derive the constant from named defaults

Add a named DEFAULT_QUORUM_GRACE_MS and include it in the sum:

const DEFAULT_QUORUM_GRACE_MS = 0;
export const DEFAULT_HARD_CAP_MS =
  COUNCILLOR_TIMEOUT_MS + AGGREGATOR_TIMEOUT_MS + DEFAULT_QUORUM_GRACE_MS + 30_000;

Pros: Single source of truth for default values. Adding a new phase timeout to the formula in one place updates both.
Cons: Still requires the author to remember to add new terms to both sites.

Option B — Extract default phase-timeout constants and a shared compute function

Promote default phase timeouts to named exports and compute the hard cap from one helper used by both the constant and parseCouncilConfig:

export const COUNCILLOR_TIMEOUT_MS = 270_000;
export const AGGREGATOR_TIMEOUT_MS = 120_000;
export const QUORUM_GRACE_MS_DEFAULT = 0;
export const HARD_CAP_BUFFER_MS = 30_000;

export function computeHardCapMs(
  councillorMs: number,
  aggregatorMs: number,
  quorumGraceMs: number,
): number {
  return councillorMs + aggregatorMs + quorumGraceMs + HARD_CAP_BUFFER_MS;
}

export const DEFAULT_HARD_CAP_MS = computeHardCapMs(
  COUNCILLOR_TIMEOUT_MS,
  AGGREGATOR_TIMEOUT_MS,
  QUORUM_GRACE_MS_DEFAULT,
);

parseCouncilConfig then uses computeHardCapMs(councillorMs, aggregatorMs, quorumGraceMs) instead of inlining the sum. Single source of truth for the formula itself. Adding a new term forces an update in exactly one function signature, which the type system enforces at call sites.

Pros: Structural prevention of drift. Cleanest.
Cons: Slightly larger refactor; touches parseCouncilConfig body, not just the constants block.

Option C — Delete DEFAULT_HARD_CAP_MS

After #17 it no longer participates in any decision branch. Inline the all-defaults value into the one test that needs it (line 242 of src/config.test.ts), drop the export.

Pros: Smallest surface area; no drift risk because the constant doesn't exist.
Cons: DEFAULT_HARD_CAP_MS may be referenced by external consumers (it is exported); removal is a breaking change for them. Loses the documentation value of "this is what the all-defaults hard cap is."

Recommendation

Option B is the most defensive — it structurally prevents the drift class — but Option A is a reasonable lighter-weight fix that buys most of the safety with a one-line change.

Option C is honest if we conclude the constant has no remaining purpose, but it requires confirming no external consumers (search GitHub for DEFAULT_HARD_CAP_MS imports) and is a breaking change worth a minor-version bump in any SemVer regime.

Scope of changes

Option A

  • src/config.ts: 1 added line (DEFAULT_QUORUM_GRACE_MS const) + reformulate DEFAULT_HARD_CAP_MS line.
  • src/config.test.ts: regression test asserting the constant value matches the all-defaults sum directly.

Option B

  • src/config.ts: introduce QUORUM_GRACE_MS_DEFAULT, HARD_CAP_BUFFER_MS, computeHardCapMs(). Update parseCouncilConfig to call computeHardCapMs instead of inlining the sum. Update readNonNegativeMs call site to use QUORUM_GRACE_MS_DEFAULT for the fallback.
  • src/config.test.ts: regression test pinning DEFAULT_HARD_CAP_MS === computeHardCapMs(...all defaults). Existing tests should be unaffected.

Option C

  • src/config.ts: remove the DEFAULT_HARD_CAP_MS export and its declaration.
  • src/config.test.ts: inline the literal at line 242.
  • Major: search for external consumers; surface as breaking change.

Acceptance criteria

  • Adding a new phase timeout (or changing the default of an existing one) cannot cause DEFAULT_HARD_CAP_MS to silently diverge from "hard cap computed with all-default phase timeouts."
  • A regression test exists that would fail if such a drift were introduced.
  • Decision on Option A / B / C documented in PR description.

Sequencing

Blocked by #17. That PR removes the only runtime decision branch that consults DEFAULT_HARD_CAP_MS, making this constant's role purely documentary and therefore clarifying which option is appropriate. Pick this up after #17 merges.

Related

Metadata

Metadata

Assignees

No one assigned

    Labels

    type/maintenanceChore, refactor, dependency, or repository maintenance.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions