feat(drift): make staleness thresholds configurable (#6) - #35
Conversation
theDakshJaitly
left a comment
There was a problem hiding this comment.
Thanks for this @mvanhorn — overall really nice shape. A few things before I can merge.
The main blocker: this conflicts with your own PR #34.
#34 refactored checkStaleness to collapse the day and commit signals into a single compound issue (fixing the double-score-penalty bug in #31). This PR is built on top of the old four-branch if/else and just parameterizes the constants. Two concrete consequences after #34 merges:
-
Merge conflict in
src/drift/checkers/staleness.ts— both PRs rewrite the same block. -
Two tests in
test/staleness.test.tswill fail because they assume the pre-#34 behavior:"emits warnings at the default warn thresholds"— assertstoHaveLength(2)for days=31 + commits=60"emits errors at the default error thresholds"— assertstoHaveLength(2)for days=120 + commits=300
Both become
toHaveLength(1)with a compound message under #34's model.
I'd like to merge #34 first, then have you rebase this on top and update those two tests to match the collapsed-issue behavior.
Smaller things while you're in there (not blocking):
- In
src/config.ts,loadStalenessThresholdshardcodes the fallbacks30/90/50/200inline. Could you importDEFAULT_STALENESS_THRESHOLDSfrom the staleness checker and spread that instead? Keeps the defaults defined in exactly one place. - No invariant check that
errorDays >= warnDays(same for commits). A misconfiguredwarnDays: 90, errorDays: 30silently makes the warn path unreachable. Not a regression vs. the hardcoded values, but since the config is now user-facing, a small validation inloadStalenessThresholds(warn once and fall back to defaults) would be friendlier. Happy to leave this for a follow-up if you'd rather keep the PR tight.
Things I liked:
- Going with 4 flags instead of the 2 from the issue. The warn/error ratios aren't a clean multiplier (3x for days, 4x for commits) so collapsing would've been lossy. Good call, and the PR body explains the reasoning well.
- Precedence chain (CLI > config > defaults) reads cleanly.
- Non-negative integer validation via
InvalidArgumentError→ users get a clear error on bad input. - Tests use module-level
vi.mockcleanly, same pattern as #34.
Let me know when you've rebased.
Thresholds (warnDays / errorDays / warnCommits / errorCommits) used to
be hardcoded in the staleness checker as 30 / 90 / 50 / 200. They are
now configurable via three precedence layers, highest first:
1. CLI flags on `mex check`:
--stale-warn-days / --stale-error-days
--stale-warn-commits / --stale-error-commits
2. `staleness` object in `.mex/config.json`:
{ "staleness": { "warnDays": 14, "errorDays": 30 } }
(partial overrides fall back to defaults for any missing fields)
3. DEFAULT_STALENESS_THRESHOLDS (30 / 90 / 50 / 200)
Internals:
- `StalenessThresholds` added to `types.ts`.
- `MexConfig.stalenessThresholds` is optional; `findConfig` now loads
it from `.mex/config.json` if present.
- `checkStaleness(...)` takes thresholds as a fourth arg with a default.
- `runDriftCheck` passes `config.stalenessThresholds` through.
- `mex check` accepts four `--stale-*` integer flags (validated via
commander's `InvalidArgumentError`).
Tests (test/staleness.test.ts, 8 new cases):
- Defaults (30/90/50/200) exported as DEFAULT_STALENESS_THRESHOLDS.
- Silent when fresh, warnings at default warn bar, errors at default
error bar.
- Custom warn-only, error-only, and commit-only overrides each isolate
cleanly.
- Silent when custom thresholds raise the bar above reality.
`npm run typecheck`, `npm test` (102/102 pass), and `npm run build`
all succeed.
… invariants - Import DEFAULT_STALENESS_THRESHOLDS from the staleness checker in loadStalenessThresholds instead of hardcoding 30/90/50/200. Defaults are now defined in exactly one place; spreading from the shared constant keeps partial-override behavior intact. - Warn and fall back to defaults when errorDays < warnDays or errorCommits < warnCommits. A misconfigured warnDays: 90, errorDays: 30 silently made the warn path unreachable; the invariant check surfaces it instead of honoring a config that disables half the checker. Both items per @theDakshJaitly's review on mex-memory#35.
ee085f6 to
06cb4c9
Compare
|
Rebased and pushed, @theDakshJaitly:
112/112 tests pass locally, |
theDakshJaitly
left a comment
There was a problem hiding this comment.
LGTM. Rebase onto #34 is clean, tests match the compound-issue shape, and both non-blocking items (shared DEFAULT_STALENESS_THRESHOLDS + warn/error invariant guard) are in with coverage. CI green on 18/20/22.
|
Thanks for landing the configurable thresholds. |
Closes #6.
Summary
Thresholds used by the staleness checker (
warnDays/errorDays/warnCommits/errorCommits) were hardcoded at 30 / 90 / 50 / 200. Fast-moving projects want a tighter bar; stable projects want a looser one. They're now configurable via three precedence layers (highest first):mex check:--stale-warn-days <n>/--stale-error-days <n>--stale-warn-commits <n>/--stale-error-commits <n>.mex/config.json) under a newstalenesskey:{ "aiTools": ["claude"], "staleness": { "warnDays": 14, "errorDays": 30 } }DEFAULT_STALENESS_THRESHOLDS(30 / 90 / 50 / 200), unchanged from the previous constants.Why four flags, not two
The issue's example (
--stale-days 14 --stale-commits 20) suggests two knobs, but the checker has four thresholds (warn + error for each of days and commits) and the relationship between them isn't a clean multiplier (90/30=3, 200/50=4). Exposing all four keeps the CLI honest and lets users tighten just the warn bar without moving the error bar. Happy to collapse to two if you'd prefer — the shape of the data flow is unchanged either way.Internals
StalenessThresholdsadded tosrc/types.ts.MexConfig.stalenessThresholdsis optional;findConfigloads it from.mex/config.jsonvia a newloadStalenessThresholdshelper that validates each field as a non-negative number and falls back to defaults for any missing field.checkStaleness(...)takes thresholds as an optional fourth arg, defaultDEFAULT_STALENESS_THRESHOLDS.runDriftCheckthreadsconfig.stalenessThresholdsthrough to the checker.mex checkvalidates flag values via commander'sInvalidArgumentErrorso--stale-warn-days foofails fast with a clear message.Tests
test/staleness.test.ts— 8 new cases, all passing:DEFAULT_STALENESS_THRESHOLDSexports the documented defaults (30/90/50/200).warnDays=14triggers at 15d with athreshold: 14dmessage.errorDays=30triggers at 45d with athreshold: 30dmessage.All 102 tests pass.
npm run typecheckandnpm run buildalso succeed.Not in scope
mex setup/mex sync. If you wantmex setupto prompt for staleness preferences, that's a separate UX change.