reviewing-personhog-protocol

Reviewing personhog protocol changes

Protocol defects survive ordinary review because every component looks correct in isolation — the bugs live in compositions: an authority signal nobody observes during one await, a threshold that coincides with a detector's duration, a rarely-taken exit path that skips the fence. This skill encodes the process that has actually caught them, and the full set of layers a personhog change must clear before it ships.

The process: two passes, then verification

1. Author pass. Sweep the full diff against every lens dimension below. Read final files, not hunks — composed state is where the bugs are.
2. Independent adversarial pass. Spawn a cold-context agent on the full diff. Give it the protocol invariants (references/personhog-invariants.md) and the lens dimensions — never your conclusions or your fixes' rationale, which would anchor it. Demand: ranked findings with file:line, a one-sentence defect statement, a concrete failure scenario (inputs/state → wrong outcome), and a CONFIRMED (traced end-to-end) or PLAUSIBLE (missing check named) label. Ask it to state clean dimensions in one line each — silence is not coverage.
3. Author verification. Verify every finding against code before accepting it. Findings get upgraded, downgraded, or killed with evidence — never adopted on authority. A wrong finding fixed is a new defect introduced.
4. After a fix batch, review again. Fresh agent, full scope, with the fix batch named as the primary attack surface: new code is where new bugs live, and a fix can displace a defect instead of closing it. Ask for a per-finding CLOSED / DISPLACED / STILL OPEN verdict.

The lens dimensions

For each, the question to ask — not a checkbox, a hunt:

1. Authority transitions × in-flight work. Enumerate every writer × key × guard. For each transition of serving authority (acquire, release, fence, deregister): what work is in flight across it, and what guarantees it lands on the right side? Never dismiss a race on likelihood — protocol review argues structure, not odds.
2. Observation latency. A prompt failure signal nobody is listening to is a deferred fence. Enumerate every await a component makes while holding authority (lease, registration, serving state) and ask: is the authority-loss signal raced here, or does this await defer detection for its full duration? Backoff naps, drains, and bootstrap sequences are where this hides.
3. Timescale interactions. Build the full constants table (TTLs, heartbeats, margins, budgets × intervals, timeouts, deadlines, watchdogs, produce/message timeouts) and check pairwise interactions. Two smells: a threshold that equals a detector's duration (the detector's failures become structurally exempt from the threshold), and a margin whose consumer can outspend it (a fence bounded by a timeout larger than the runway). Validate required relations at construction, not in comments.
4. Budget and counter semantics. What resets what, and on which evidence? Progress must mean applied work — never a successful read, which stays available in exactly the wedges budgets exist for. Ask both directions: can a wedge class cycle forever without escalating, and can a transient class escalate spuriously?
5. Lifecycle. Every spawned task joined or aborted on every exit path; raced JoinHandles consumed at most once with every later await site guarded; cancel-by-drop assumptions verified (dropping a loop future drops its owned futures — but never its spawned tasks); teardown ordering stated and tested. The bootstrap window — registered but not yet supervised — is an exit-path zoo of its own.
6. Primitive semantics, verified against implementation. What does the primitive actually do — not what its name suggests? select! short-circuits on first ready arm; FuturesUnordered only progresses when polled; a keepalive response proves a reset at send-processing time, not receipt time; stream end and lease expiry are different facts; etcd answers keepalives for a dead lease with TTL 0, not a closed stream. When in doubt, read the dependency's source.
7. Failure-path parity. The rarely-taken exits — budget exhaustion, timeouts, poison paths, fast-shutdown branches — must uphold the same invariants as the hot path. "The fence exists" is not enough; it must run on every path that drops authority, in the right order relative to deregistration.
8. Escalation legibility. Fail loudly and attributably: a crash cascade that shows up as generic restarts is loud but illegible. Every deliberate escalation should carry the specific cause (which partition, which budget, which margin) in its metric labels and logs.