Skip to content

fix(core): make successThreshold bind from every status that records a failure - #12031

Merged
os-warren merged 1 commit into
mainfrom
claude/issue-11955-success-threshold-read
Aug 25, 2026
Merged

fix(core): make successThreshold bind from every status that records a failure#12031
os-warren merged 1 commit into
mainfrom
claude/issue-11955-success-threshold-read

Conversation

@os-warren

@os-warrenos-warren commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Fixes#11955

PluginHealthMonitor consulted successThreshold only while a plugin's status was
unhealthy or degraded. The first success in a recovery wrote recovering — a status
that gate did not name — so the second success took the outer else and reached
healthy without the counter being read at all. failed was in neither set either, so a
plugin whose check threw recovered on its first success.

Status when the successes startConsecutive successes actually required
unhealthy / degraded2, whatever successThreshold said
failed / recovering1, whatever successThreshold said

A declared successThreshold: 5 was indistinguishable from 2. The default is 1
exactly the value at which the defect is invisible — so every case below declares 3.

Which reading, and why the declaration decides it

The card was explicit that it does not decide between (a) counting from every
non-healthy status and (b) keeping recovering non-counting, and that no ADR, comment
or test states an intent. It asked for the reading the declaration supports, or a fork if
no declared text distinguishes them. Declared text does distinguish them, in three places:

  1. recovering is declared as a process, not a resting place.PluginHealthStatusSchema
    annotates it "Plugin is in recovery process" — recovery under way, not finished. Under
    (b) that is the one status in which the recovery criterion is never evaluated, which
    contradicts its own annotation.
  2. The count is declared as a number of successes, and (b) cannot honour it.
    .describe('Consecutive successes needed to mark healthy') renders verbatim into
    content/docs/references/kernel/plugin-lifecycle-advanced.mdx:112. Under (b) that
    published sentence is false for every declared value above 2.
  3. failed's exit is already treated as counted by landed code.autoRestart never fires for a health check that throws or times out — only for one that returns a failure #11852's fix (landed in
    983edf1) clears successCounters on the thrown route. Zeroing a counter that is never
    read from the status that route writes is dead code; the reset is only meaningful if the
    exit from failed is gated on it. That landed diff is committed intent, and it is the
    interlock the card names.

So: (a), scoped to what the declaration actually says. The counter is now consulted on
the way out of every status that records an observed failuredegraded, unhealthy,
failed, recovering.

healthy and unknown deliberately still promote on the first success. The key's own
JSDoc scopes it to recovery — "Number of consecutive successes to recover from unhealthy
state"
— and unknown ("Health status cannot be determined", the status registerPlugin
writes) records no failure to recover from. Making unknown count would also have no
truthful label to sit in while counting: it would report a never-failed plugin as
recovering, or need a new status — i.e. widening the public surface, which this card is
not. That boundary is pinned by a test so it cannot drift silently.

What changed

packages/core/src/health-monitor.ts — the gate is now a map that is exhaustive over
PluginHealthStatus
. A status added to PluginHealthStatusSchema fails to compile until
this file places it on one side or the other, so the gate cannot quietly acquire a second
bypass the way recovering did. No spec, schema or public surface changed; this is the
enforcement side of an already-declared, already-documented config member catching up.

Non-vacuity — two ablations, direction predicted before running

Both mutations were proven on disk by anchored grep -c counts read before any result,
rebuilt, and restored under trap … EXIT INT TERM, with the tree verified clean after.

AblationPredictedObserved
Revert the gate to unhealthy || degraded (from the base commit)5 red — the four entry-point pins plus the reset pin; the unknown boundary test stays greenTests 5 failed | 13 passed, exactly those five
Delete #11852's successCounters reset from recordFailedRound1 red — only the reset pinTests 1 failed | 17 passed, only starts the count over after a throw interrupts a recovery

The second one is the point of the ⭐ in the card: #11852 shipped its reset with no
behavioural pin because the counter's only read site was unreachable with a stale non-zero
value, so any test would have passed for the wrong reason. Gating failed on the counter
makes that reset observable, and the ablation shows it is now load-bearing.

The subject is imported by the relative specifier ./health-monitor.js inside its own
package, so vitest resolves src/ and no dist/ sits on its path; @objectstack/core was
rebuilt in each leg anyway, and scripts/ablation-dist-preflight.mjs confirmed the first
mutation's marker was absent from all 12 built files.

One honest note, on my own harness rather than on the change. Leg 1's restore was written as
git checkout -- FILE, which restores from the index — and the mutation step,
git checkout BASE_SHA -- FILE, had written the mutated content to the index as well as the
worktree. So the trap ran, reported success, and left the file mutated. The post-ablation
clean-tree check is what caught it; the file was restored with git checkout HEAD -- FILE,
and leg 2 used that form from the start. No reading was taken on a mutated tree.

(Placeholders above are spelled without angle brackets deliberately: this body's first
revision wrote them as bracketed placeholders and GitHub's sanitizer ate every one of them,
turning that paragraph into a sentence about git checkout -- and git checkout -- .)

Verification — all at b797b30566

  • pnpm --filter @objectstack/core test38 files, 950 tests passed (12 pre-existing
    health-monitor tests unchanged: at the default successThreshold: 1 every route is
    byte-for-byte what it was).
  • Gate union derived, not recalled:
    node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack against the actual
    diff (3 paths). All 14 path-matched families plus the 6 convention-triggered ones ran
    green
    , exit codes captured before any pipe — including
    check:type-check-debt, whose own verdict line reads
    "32 ledger entr(ies) re-measured in 360.6s, 1898 raw tsc error(s) total, none above its
    recorded number"
    (@objectstack/core is a ledger entry, so this diff could have moved it).
    check:nul-bytes green, plus a direct control-byte scan of the three changed files.

Generated by Claude Code

…, not just two of them
`PluginHealthMonitor` read `successThreshold` only while status was
`unhealthy` or `degraded`. The first success wrote `recovering` — a status
that gate did not name — so the second success took the outer `else` and
reached `healthy` without consulting the counter; `failed` was skipped the
same way and recovered on its first success. A declared value of 5 was
indistinguishable from 2, and from 1 when recovery started at `failed`.
The gate is now a map exhaustive over `PluginHealthStatus`: `degraded`,
`unhealthy`, `failed` and `recovering` all keep the counter in force, and a
status added to the spec fails to compile until it picks a side. `healthy`
and `unknown` still promote on the first success — the count is declared as a
recovery criterion and neither records a failure to recover from.
Ref #11955
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W6HFzyH98W1YaQXhJUJt6o
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

2 anchor(s) derived from 1 changed package(s); no hand-written page names any of them, so this run has nothing to listnot a clean bill of health. This check sees only pages that NAME a derived anchor: one that documents this change in prose, or enumerates it in an authoring dialect, names none and stays invisible to it on every run.

What this run could not see
  • 6 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 45 of 222 client-bound route-ledger rows — the other 177 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 23 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 1e79aa4f812708d5d097ca33d9b7040e02b958c6packageMentionDocs.

Which tree this was computed on

This run read content/docs from b19eb25dab0674650f49ab5dee7556161d0db9a6 — the merge of head b797b30566f607154e20d60929df13c5c40279d6 into base 1e79aa4f812708d5d097ca33d9b7040e02b958c6, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin b19eb25dab0674650f49ab5dee7556161d0db9a6 && git checkout b19eb25dab0674650f49ab5dee7556161d0db9a6
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 1e79aa4f812708d5d097ca33d9b7040e02b958c6 b797b30566f607154e20d60929df13c5c40279d6 && git checkout -B drift-repro 1e79aa4f812708d5d097ca33d9b7040e02b958c6 && git merge --no-ff b797b30566f607154e20d60929df13c5c40279d6
node scripts/docs-audit/affected-docs.mjs --json 1e79aa4f812708d5d097ca33d9b7040e02b958c6

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/mteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

successThreshold stops being read the moment recovery starts, so it can never require more than 2 consecutive successes

2 participants

@os-warren@claude