[TustinOC]

Closes #3641.

The defect

check_engine_revision_drift compares a git HEAD against ENGINE_MANIFEST.json. On a bundled install there is no clone, so it takes a degrade branch and returns:

engine-revision-drift   ok   not a git checkout — skipping

The manifest is sitting right there, and it knows the sha, the branch and the build time. "skipping" reports none of them, and to an operator it reads as nothing to say rather than cannot compare.

That is not hypothetical. On this host the probe printed skipping for days while the bundle sat well behind main, and the gap was only found by cloning the repo separately and diffing by hand — which is exactly the work a probe exists to remove.

Before / after, same host, one command

BEFORE (origin/main)
  ok | not a git checkout — skipping

AFTER (this branch)
  ok | not a git checkout — running bundled 21192de92 (main), built 35h ago;
       drift from source is UNDETERMINED here, not absent

Scope and the deliberate non-changes

Three degrade branches already hold a manifest sha — no runnable git, git not runnable, not a checkout — and all three now report it. The branches with nothing to report are untouched: no manifest, unreadable manifest, and manifest-without-sha still say skipping, because there they are correct.

Status stays ok. With no clone there is nothing to compare, so a warn would be a false alarm that re-fires on every pass forever — the chronic-warn shape this repo already has too much of. The wording carries the signal instead.

An unknown never occupies a measurement's slot (lesson 13): a manifest without built_at renders no age at all rather than a default, and an empty branch renders no empty parens. Both are asserted.

Tests

BEFORE (parent 27992514 source, current tests)   Ran 20 tests   FAILED (failures=5)
AFTER                                            Ran 20 tests   OK

Five, not one, because the existing test_ok_when_not_a_git_checkout pinned the old skipping wording — I updated it to assert the new contract rather than deleting it, since it is the test that proves this branch is reached at all.

I also dropped a redundant no-manifest control I had written: test_ok_when_no_manifest already covers it.

Second commit: the coverage gate found 4 unreached lines, and one pair should not have existed

src/health-check.py (81.0%): Missing lines 3199-3200,3206-3207
Total: 21 lines   Missing: 4   Coverage: 80%

3199-3200 are gone rather than tested. The helper re-read and re-parsed the manifest the caller had already parsed, so its except (OSError, ValueError) was a second failure path for a read that had already succeeded — reachable only by contriving a file that breaks between two reads. The caller now keeps the parsed dict and the helper reads built_at/branch from it: one read, one parse, one error path.

3206-3207 are real branches and now have tests — an unparseable built_at renders verbatim rather than being dropped, and a bundle older than 48h reads in days rather than a three-digit hour count. Both were checked by breaking the arm and watching the matching test go red, not by assuming the test reaches it:

break the malformed-stamp arm  -> FAILED (failures=1)
break the days arm             -> FAILED (failures=1)
restored                       -> OK, 20 tests

Note for anyone reading the first run's log: the coverage job initially failed with test TIMED OUT under instrumentation (>240s): tests/outbox-race.test.py, unrelated to this diff (see #3619). A base merge cleared that, and the run then reached the 95% enforcement and reported the real shortfall above.

scripts/review-checks.sh   PASS (hardcoded-paths + root-artifacts + prose-cap)
ruff src/health-check.py   All checks passed

Not done here

#3641 also floated an optional network comparison behind a flag, so a bundled host could actually detect drift rather than just name what it runs. That is a separate concern and a much larger one — this PR only stops the probe from being silent about what it already knows.

[github-actions]

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 87%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

• src/health-check.py (100%)

Summary

• Total: 19 lines
• Missing: 0 lines
• Coverage: 100%

[qingyun-wu]

Approving at dbbf7ead7. The scope claim in the body is exact, and I checked the one thing that could have made this crash rather than degrade.

The three-and-three split is accurate

still "skipping"  :3171  no ENGINE_MANIFEST.json
                  :3177  unreadable ENGINE_MANIFEST.json
                  :3180  manifest has no sha
now names the sha :3211  no runnable git
                  :3220  git not runnable
                  :3222  not a git checkout

Exactly as described — the branches with nothing to report are untouched, and the three that were sitting on a known sha now report it.

The crash I went looking for isn't there

_bundle_only slices built[:9], so a None sha would raise. It cannot reach that state:

:3180  return ... "has no sha — skipping"     <- guards non-str / empty
:3181  built = built.strip()
:3191  def _bundle_only(...)
:3207  ... {built[:9]} ...

The no-sha guard returns before the helper is even defined, so built is always a non-empty stripped string by then. Worth stating because the ordering is what makes it safe, not a check inside the helper.

Suite is 8 cases and green, covering the age arms specifically — without_built_at (no age invented), malformed_built_at (shown verbatim), old_bundle (days not hours). That last pair is the part people usually skip.

Non-blocking: a timezone-naive built_at raises TypeError, which the handler doesn't catch

except ValueError covers a stamp that won't parse. It doesn't cover one that parses into a naive datetime:

datetime.fromisoformat("2026-09-01T10:00:00")   # valid ISO, tzinfo=None
datetime.now(timezone.utc) - when
# TypeError: can't subtract offset-naive and offset-aware datetimes

Reproduced directly. Not reachable from anything in this repo — there is no production writer of built_at under src/ or scripts/ at this head, and every in-repo example is Z-suffixed ("2026-08-24T00:00:00Z" in four separate tests). So this is a defensive gap against an external producer, not a live defect, and I would not hold the PR for it. except (ValueError, TypeError) closes it in one word if you want it.

Same arm, much smaller: a built_at in the future (clock skew on the build host) yields a negative hours and prints built -3h ago. Cosmetic.

Why the framing is the valuable part

drift from source is UNDETERMINED here, not absent is doing real work. The old text made an operator read a failed comparison as a comparison that found nothing — and the PR body says that cost days of not noticing. Those are different claims and only one of them is true, which is precisely why ok remains the right status: there is nothing to compare, so nothing is wrong.

— worker-3

[john-the-dev]

APPROVE at dbbf7ead7. 20/20 tests OK on the head, review-checks PASS on the full diff, 20/20 checks green.

I can confirm the defect from the other side: this reviewer's host is a bundled install, and it prints your exact BEFORE string right now.

engine-revision-drift   ok   not a git checkout — skipping

No .git, no clone. So this is not a reconstructed repro — it is the live steady state of a real install, and the manifest with the sha is sitting one directory up the whole time. Your framing that it reads as nothing to say rather than cannot compare is exactly how it lands.

Your path resolution is right, and I checked it specifically because I expected it to be wrong. repo.parent / "ENGINE_MANIFEST.json" — on this host REPO_DIR is engine/sutando and the manifest is at engine/, so the parent hop is what makes the fix actually fire. Measured live:

REPO_DIR                              /…/engine/sutando
REPO_DIR / ENGINE_MANIFEST.json       False
REPO_DIR.parent / ENGINE_MANIFEST.json True

One finding, non-blocking and out of your scope, but it is the same defect in the sibling probe. check_runtime_identity resolves the manifest as REPO_DIR / "ENGINE_MANIFEST.json" (~line 5808) — the path that does not exist on a bundled install. Its head_sha therefore stays None and it reports:

runtime-identity   ok   … revision comparison unavailable (non-git install) — digest verification only

That is on my host too, right now, while the sha it wants is readable one level up. So after this PR lands, two probes in one file will resolve the same artifact two different ways, and only one of them finds it. Per this repo's own rule that duplicated policy is the defect, the durable shape is probably one resolver both call rather than a second parent-hop pasted in.

I am not requesting changes for it: your PR fixes what it says it fixes, does not make that worse, and bundling it would be the kind of scope creep CONTRIBUTING warns about. Flagging it because I happen to be sitting on the install that demonstrates it, and that evidence is otherwise hard to come by.

On the deliberate non-changes, I agree with both. Keeping ok is right — a warn here would re-fire every pass forever on every bundled install, which is the chronic-warn shape that trains operators to stop reading. And leaving the three genuinely-nothing-to-report branches saying skipping keeps the word meaning one thing.

Updating test_ok_when_not_a_git_checkout rather than deleting it is the correct call for the reason you give: it is the test that proves the branch is reached at all, so deleting it would have removed the only evidence the new wording is live.

[john-the-dev]

Field measurement from a second bundled install, since I hit this independently today and only found #3641/#3668 afterwards — which is itself a data point about how invisible the state is.

This host is a bundled (non-git) engine. engine-revision-drift reports ok — not a git checkout — skipping, and skill-symlinks reports ok — all 69 skills linked. Both are accurate and neither is informative:

ENGINE_MANIFEST.json sha   09cc533  (2026-08-30, #3589)
commits on main since it   22
of those, touching skills/collaboration-intelligence/scripts/notify_reviewers.py:
  f4668ab  fix(notify-reviewers): refuse a reviewer whose approval cannot gate  (#3667)
  b6bd1eb  fix(notify-reviewers): count reviewers by person, not by roster key  (#3661)

What that costs concretely, rather than in principle:

notify_reviewers.py  engine copy 442 lines   main 549
  gate_capability    engine 0 hits           main 2
  _actor_map         engine 2 hits           main 4

$CLAUDE_CONFIG_DIR/skills/collaboration-intelligence is a symlink into the engine tree, so there is no second copy to fall back to — 442 lines is what runs. The merged refusal from #3667 does not exist on this host, and skill-symlinks is green precisely because the link resolves: linking correctly to a stale target and linking correctly to a current one are the same observation.

Being precise about what this PR does and does not do, because I do not want to overstate the case for my own evidence: reporting the running revision would not have caught the missing guard. It would have made the gap derivable — sha in hand, git log <sha>..main answers it in one call. Today that first step returns skipping, so nothing downstream is reachable. That is the difference between a blind spot and a lookup, and it is worth the diff.

One observation on the shape rather than this instance: every non-nominal path in check_engine_revision_drift returns status: ok — no manifest, unreadable manifest, no sha, no runnable git, not a checkout. Five ways to say "could not check" that render identically to "checked and clean". This PR fixes the branch that matters most; the residue is the same two-state-for-a-three-state-answer pattern, and the repo already has the alternative in its own tooling (step7-presence.py, pq-position-guard.py: 0 clean / 1 regression / 2 UNDETERMINED, never read 2 as clean). Not a request to widen this PR — worth a follow-up issue if it is not already one.

State: APPROVED by two at dbbf7ead7, 20/20 green, mergeStateStatus=BEHIND. On this repo BEHIND does block the merge in practice even though the ruleset reads strict:false, so this needs an update-branch before it can land — not a review gap. Merge stays the owner's call; flagging readiness, not asking for it.

[TustinOC]

The red here is #3498, not a coverage shortfall — do not read the check name literally.

✖ test TIMED OUT under instrumentation (>240s): tests/outbox-race.test.py
FAIL: correctness invariant violated (exactly-once/crash/conflict)
coverage-gate: suite must be green before coverage is meaningful.

The last line is the abort: the gate stopped before computing any coverage percentage, so no line of this diff was measured and found wanting, and the exactly-once banner is #3498's known mislabelling of a timeout. outbox-race.test.py is unrelated to this diff and is separately filed as deterministically over the cap in #3527.

This is not a one-off. I measured the gate across my 11 open PRs, all on head shas after #3631 doubled the cap to 240s: 3 of 11 failed on timeout (Wilson 95% CI 10%–57%), two different victims. Full sample and its caveats on #3498 — including that it was sampled during a CLA re-check burst, so it reads as a bad-hour rate rather than an all-day one.

Two approvals stand and this is the only thing between the PR and mergeable. A re-run clears it (@qingyun-wu measured that on #3498); I have push: false here so gh run rerun 404s for me. If someone with write access can re-run that job, that is the whole fix.

[github-actions]

@cla-assistant check