feat(rmp): comparative quorum modules — quorum-in-module + distributed quorum #4
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/rmp-quorum-modules"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Builds the two comparative quorum review modules over the RMP layer, plus the cross-process container split and a reliability-weighted comparison harness.
Modules
src/rmp/quorum-module.ts): runs the wholeQuorum(2 reviewers + arbiter) inside one process, emitting one event stream + one terminal result. Leg-tagged activity for the status column.src/rmp/distributed-quorum.ts): fans two single-reviewer legs out as separate module runs and reconciles centrally. Reliability-first per-leg failure isolation — both-fail → fail-closed; one-fail → DEGRADE to the surviving leg's verdict. Heterogeneous-leg capable (per-leg model). Pluggable leg-runner: in-process or cross-process (transport).Shared + infra
quorum-reconcile.ts:isQuiescent/mergeQuiescentextracted fromQuorum(behaviour-preserving — 34 quorum tests still pass) so both modules reconcile identically.RmpEvent(additiveleg) + leg-awareDriverSink.module-hostconsume-loop,module-bootstrap+bin/module-claude{,-quorum}.tscontainer entrypoints, 3docker-composeservices (rmp-modules profile).compare.ts+bin/compare-modules.ts: reliability-weighted comparison (bucketed verdict agreement: NO_NEW_FINDINGS ≈ APPROVE) → structured report.Tests
+~30 tests across the new modules/transport/host/compare; full suite 2135 green; typecheck clean. The container split is exercised live via the rmp-modules profile + the compare driver.
compare.ts runs each topology over the same ReviewRequest, reduces its event stream to a ModuleRunReport (verdict + bucket, finding breakdown, latency, tokens/cost, rate-limits, arbiter-fired, degraded, per-leg metrics), and diffs the two — reliability first (did each produce a verdict? did a leg degrade/fail? do they agree?), cost/latency last. Verdict agreement is BUCKETED (blocking {NEEDS_WORK,BLOCKED} vs non-blocking {APPROVE,CONDITIONAL_APPROVE,NO_NEW_FINDINGS}), so a NO_NEW_FINDINGS round and an APPROVE round AGREE; exact verdict equality is recorded alongside for the fine-grained view. renderComparisonMarkdown emits a reliability-first report. A `(emit) => Promise<void>` topology-run shape so it drives the in-process modules now and a cross-process event reader later. Pure + clock-injectable; 10 deterministic tests. (bin/compare-modules.ts live runner lands with P5.)hib-pr-reviewer review — PR #4 (hib/hib-pr-reviewer)
Round 1 — head
062414e95196, basemain, triggeropenedTL;DR: CONDITIONAL_APPROVE — kept 1 agreed minor (ensureRequestGroup), 1 verified unique-to-A minor (unstructured logging, corrected Rule 13 rationale), 1 verified unique-to-A info (double-cast, corrected line 99→107); no blocking issues.
Summary
Arbitration — round 1
Verified all 3 findings from A and the 1 finding from B.
Finding 1 (A + B agreed,
transport.ts:172): Both reviewers independently flagged thatrunReviewViaTransportenqueues directly without callingensureRequestGroup, whilemakeTransportLegRunnerindistributed-quorum.ts:286explicitly guards with that call first.Readconfirms line 172 goes straight toenqueueReviewRequest;GrepconfirmsensureRequestGroupis called at distributed-quorum.ts:286 and exported from transport.ts:213 but never invoked insiderunReviewViaTransport. Kept as agreed minor.Finding 2 (unique to A,
module-bootstrap.ts:63+65):Readconfirms both lines useconsole.log. Reviewer A's rationale cited "Rule 13 requires stderr" but Rule 13 actually states "App writes to stdout only (12-Factor XI)" — the rule mandates stdout, not stderr. The legitimate violation is the unstructured free-form template-string format, which contravenes Rule 13's "Structured logging (JSON or key-value) — no free-form strings" requirement. There is also an inconsistency withcompare-modules.ts:75which usesconsole.error. Kept as minor but rationale corrected: the fix should be structured JSON output to stdout, not a switch toconsole.error.Finding 3 (unique to A,
bin/compare-modules.ts):Readconfirms the double-castas unknown as ReviewJobexists at line 107 (not 99 as cited — 99 is the first field of the object literal). The cast is present and the concern about suppressed structural checking is valid. Corrected line to 107. Kept as info.Blast Radius
The PR adds 22 files spanning a new distributed module topology (src/rmp/), two new binary entrypoints (bin/), a shared reconcile extracted from src/quorum.ts, six test suites, and a Docker Compose profile extension. The transport.ts export surface (
runReviewViaTransport,makeTransportLegRunner,ensureRequestGroup) is consumed by multiple orchestration paths. Changes are additive and gated behind thermp-modulesCompose profile, limiting blast radius to opt-in deployments.BLAST_SCORE: 6/10
Risk Indicators
ensureRequestGroup,enqueueReviewRequest,runReviewViaTransport,makeTransportLegRunner,runDistributedQuorum,collectModuleRunCI status (head
062414e95196)Overall: ✓ success
3 checks: 3 pending
Findings (3)
[MINOR]
runReviewViaTransportenqueues without theensureRequestGroupcold-start guardsrc/rmp/transport.ts:172
makeTransportLegRunner(distributed-quorum.ts:286) explicitly callsensureRequestGroup(redis, keys, leg.moduleId)beforeenqueueReviewRequest, with the inline comment that the group must precede the enqueue forXREADGROUP >to deliver.runReviewViaTransportskips this step entirely — line 172 goes straight toenqueueReviewRequest.In
compare-modules.ts, Module A is driven exclusively viarunReviewViaTransport({ …, moduleId: "claude-quorum" }). If thehib-pr-reviewer-module-quorumcontainer restarts between the operator starting a compare run and this XADD, the re-created consumer group (XGROUP CREATE … $) places its cursor at or after the already-enqueued entry;XREADGROUP >will not deliver it. The event-read loop runs tomaxIdleReadsandcompareModulesrecords Module A asoutcome: "silent"with no actionable error surface.ensureRequestGroupis idempotent (BUSYGROUP is swallowed), so the fix is safe to add unconditionally:This mirrors the pattern already established in
makeTransportLegRunner.[MINOR] Redis error handlers use unstructured free-form template strings (Rule 13 violation)
src/rmp/module-bootstrap.ts:63
Lines 63 and 65 attach Redis error handlers using
console.logwith template strings:Rule 13 mandates structured logging — "Structured logging (JSON or key-value) — no free-form strings." Template-string messages are unstructured and cannot be reliably parsed, indexed, or alerted on by a log aggregator.
Note: Rule 13 also states "App writes to stdout only (12-Factor XI)," so the channel (
console.log→ stdout) is correct; the fix is to emit structured key-value or JSON objects, not to switch toconsole.error.Also note the inconsistency:
compare-modules.ts:75usesconsole.errorfor its equivalent Redis error handler whilemodule-bootstrap.tsusesconsole.log. Standardise onconsole.log(stdout) with structured output per Rule 13:[INFO]
as unknown as ReviewJobdouble-cast bypasses TypeScript's structural checkbin/compare-modules.ts:107
The
jobobject literal constructed at lines 98–107 is escaped viaas unknown as ReviewJobon the closing line. Test fixtures intests/rmp-distributed-quorum.test.tsandtests/rmp-module-host.test.tsconstruct an identical shape and assign it directly asconst JOB: ReviewJob = { … }without any cast, proving the literal satisfies the type.The double-cast silently suppresses TypeScript's structural check at this site. If
ReviewJobgains a required field in the future, this site will compile cleanly but throw at runtime.Replace with a direct typed assignment:
Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 1 • 3 findings (2m/1i) • 2026-06-02T22:17:03.756Z → 2026-06-02T22:19:07.782Z • posted-as: pr-reviewer-bot • model: auto
Show previous round
hib-pr-reviewer review — PR #4 (hib/hib-pr-reviewer)
Round 1 — head
26fd8db7a2bf, basemain, triggersynchronizeTL;DR: CONDITIONAL_APPROVE — kept 1 agreed finding (signal drop, A1+B1) and verified 3 unique findings (A2: latent hang from redundant guard; B2: double-cast removes type safety; B3: missing cold-start ensureRequestGroup in runReviewViaTransport); no blocking or major issues.
Summary
Arbitration summary
4 findings kept, 0 dropped.
makeTransportLegRunneromits the 4thsignalparameter.Readof line 283 confirmsreturn async (leg, request, emit) => {— signal silently dropped.LegRunnertype at lines 56–61 declaressignal: AbortSignal | undefinedas the 4th arg. Kept.if (survivor.ok && !dead.ok)is a tautology within the!outA.ok || !outB.okbranch (both-fail already returned at 141–148,survivor/deadare assigned to exactly the ok/not-ok sides).Readof lines 149–164 confirms the structure exactly as described; the unconditional barereturnon line 163 creates a latent no-terminal-event hang if the guard were ever false after a refactor. Kept.as unknown as ReviewJobdouble-cast atbin/compare-modules.ts:107confirmed byRead. Suppresses structural checking for futureReviewJobfield additions. Kept.runReviewViaTransportatsrc/rmp/transport.ts:172confirmed to callenqueueReviewRequestwith no precedingensureRequestGroup, whilemakeTransportLegRunner(line 286) explicitly guards withensureRequestGroupfirst for cold-start safety. Asymmetry is real and can cause silent"silent"outcome on container restart. Kept.Blast Radius
The diff is almost entirely additive (~1800 lines of new code across new files) behind the
rmp-modulescompose profile. The existing production review path (src/rmp/driver.ts,src/quorum.ts) is touched only minimally (driver: +17/-10, quorum: +11/-74 refactor into quorum-reconcile.ts). New exports fromtransport.ts,distributed-quorum.ts, andcompare.tsextend the public RMP surface but are not yet wired into the serving path.BLAST_SCORE: 4/10
Risk Indicators
runReviewViaTransport,makeTransportLegRunner,enqueueReviewRequest,ensureRequestGroup,publishCancelCI status (head
26fd8db7a2bf)Overall: ✓ success
3 checks: 3 pending
Related PRs
Findings (4)
[MINOR]
makeTransportLegRunnersilently dropssignal— XREAD loop not cancellable on abortsrc/rmp/distributed-quorum.ts:283
The
LegRunnertype (line 56–61) declares a 4th parametersignal: AbortSignal | undefined, but the closure returned bymakeTransportLegRunneronly destructures three:The
for awaitloop overreadReviewEventshas no abort path; it only exits on a terminal event ormaxIdleReadsexhaustion. When the outer distributed quorum is cancelled (e.g. viaAbortControlleron a server timeout), the transport leg will block for up toblockMs × maxIdleReads— in the CLI config that ceiling equals the full job timeout (20 min). The in-process runner (makeInProcessLegRunner) correctly forwardssignaltorunClaudeModule, so the two runners diverge on cancellation semantics.Current impact is low (CLI path; process exit cleans up), but the broken API contract will silently hang if
makeTransportLegRunneris wired into a long-lived server path.Suggested fix: at minimum add abort awareness inside the loop:
Alternatively, publish a cancel on abort:
signal?.addEventListener('abort', () => publishCancel(redis, keys, legRequest.requestId).catch(() => undefined)).[MINOR] Redundant always-true inner guard in degradation branch creates a latent no-terminal hang path
src/rmp/distributed-quorum.ts:154
Inside the
!outA.ok || !outB.okbranch (the both-fail case was already returned at lines 141–148),survivoris explicitly assigned the ok leg anddeadthe not-ok leg, sosurvivor.ok && !dead.okat line 154 is a tautology and can never be false:If the inner guard were ever false after a refactor (e.g. adding a third leg, changing the both-fail bail-out),
finalizeis skipped and the unconditionalreturnexits with no terminal event emitted, causing the serving driver andcompareModulesto hang indefinitely.Suggested fix: remove the inner
ifand callfinalizeunconditionally, adding an unreachable backstop:[MINOR]
as unknown as ReviewJobdouble-cast is unnecessary and removes compile-time safetybin/compare-modules.ts:107
The constructed literal satisfies every required
ReviewJobfield (trigger: "opened"is a valid union member;AllowedOwner = string;prBodyis optional), yet the double-cast bypasses TypeScript's structural check:If
ReviewJobgains a new required field, TypeScript will not flag the omission here. Replace with a typed binding to restore structural checking:Or use
satisfies ReviewJobif you want to keep the narrowed literal type.[MINOR]
runReviewViaTransportmissingensureRequestGroup— cold-start race can silently lose Module A's requestsrc/rmp/transport.ts:172
makeTransportLegRunner(the Module B leg path) explicitly callsensureRequestGroupbeforeenqueueReviewRequestwith a// cold-start safetycomment (lines 286–287).runReviewViaTransport(the Module A path incompare-modules.ts) does not:If the
hib-pr-reviewer-module-quorumcontainer has just restarted, the host'sensureRequestGroupruns with$as the last-delivered-id after the already-enqueued message's stream ID.XREADGROUP >therefore never delivers the message, Module A produces outcome"silent"in the comparison report with no diagnostic, and the asymmetry with Module B is invisible.Fix: add
await ensureRequestGroup(opts.redis, opts.keys, opts.moduleId)immediately before line 172, mirroringmakeTransportLegRunner's defensive pattern. The module containers have nohealthcheck:in the compose file, so adepends_on: service_healthygate is not currently available as a compensating control.Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 1 • 4 findings (4m) • 2026-06-02T22:21:50.634Z → 2026-06-02T22:23:27.373Z • posted-as: pr-reviewer-bot • model: auto
Show previous round
hib-pr-reviewer review — PR #4 (hib/hib-pr-reviewer)
Round 2 — head
2cf595082fb2, basemain, triggersynchronizeTL;DR: CONDITIONAL_APPROVE — 3 minor findings kept this round.
Summary
Arbiter reconciled 1 (A) + 3 (B) → 3 findings.
Risk Indicators
CI status (head
2cf595082fb2)Overall: ✓ success
3 checks: 3 pending
Findings (3)
[MINOR]
runner: ModuleReviewerrequired inDistributedLegSpecforces a double-cast stub for the transport-leg casesrc/rmp/distributed-quorum.ts:45
Line 45:
runner: ModuleReviewer;is non-optional. The inline JSDoc already concedes "may be a no-op stub there" for transport legs, yet the type forces callers to supply something.bin/compare-modules.ts:123-125works around it with:The
as unknown asdouble-cast silences the compiler without enforcing correctness. If a future caller omitslegRunnerand falls through to the in-process path,stubRunner.run()returns an opaque rejected promise — hard to diagnose.Fix:
src/rmp/distributed-quorum.ts:45— changerunner: ModuleReviewer;→runner?: ModuleReviewer;.makeInProcessLegRunnerwhereleg.runneris invoked:bin/compare-modules.ts— replacerunner: stubRunnerwithrunner: undefined(or omit the field) and delete thestubRunnerconstant.No runtime behaviour change; the guard converts a silent thrown promise into a legible
LegOutcomefailure and removes the need for the unsafe cast.[MINOR]
"both-failed"reliability verdict fires only on exactfailed+failed;silent+silent(orsilent+failed) misroutes to"one-failed"src/rmp/compare.ts:230
Lines 230-232 at HEAD:
When both legs time out / produce no terminal (outcome
"silent"), the first guard is false (neither is"failed") and the second guard fires — yielding"one-failed"for a total-outage scenario. An operator reading the headline headline sees one failure when both topologies produced nothing.Fix — key on "neither produced a verdict" rather than the exact
"failed"token:This maps
failed+failed,silent+silent, andfailed+silentall to"both-failed", matching semantic intent.[MINOR] Startup, signal, and stop logs are free-form template strings — violates rule 13 (structured JSON logging)
src/rmp/module-bootstrap.ts:100
Lines 100-103, 106, and 115 emit unstructured template literals:
Rule 13 (
.claude/rules/13-logging-observability.md) requires structured logging (JSON or key-value) with no free-form strings. The NASjson-filedriver captures this container's stdout verbatim, and the Redis-error handlers in the same file already useJSON.stringify. These three paths are inconsistent.Concrete fix:
Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 2 • 3 findings (3m) • 2026-06-02T22:35:08.801Z → 2026-06-02T22:36:30.691Z • posted-as: pr-reviewer-bot • model: auto • [bookkeeping fallback]
Round-3 review (3 minor): - DistributedLegSpec.runner is now optional (the transport leg-runner ignores it; makeInProcessLegRunner guards a missing runner → {ok:false}), removing the `as unknown as ModuleReviewer` stub double-cast in compare-modules.ts + the transport test. - compare.ts reliability verdict: a `silent` outcome (no terminal) now counts as a failure, so both-silent → "both-failed" (was misclassified "one-failed"). + a test. - module-bootstrap.ts startup/signal/stop logs emit structured JSON (rule 13), matching the redis-error handlers.hib-pr-reviewer review — PR #4 (hib/hib-pr-reviewer)
Round 3 — head
ea9c956eef72, basemain, triggersynchronizeTL;DR: CONDITIONAL_APPROVE — 1 agreed finding (A+B, module-host.ts free-form log calls) and 1 verified unique-to-A finding (compare-modules.ts stderr progress strings); both minor rule-13 violations, no blocking issues.
Summary
Arbitration — Round 3
Actions taken:
/var/lib/pr-reviewer/repos/hib/hib-pr-reviewer/1526/src/rmp/module-host.tsin full — verified lines 52, 79, 87, 116, 132 exactly as both A and B described./var/lib/pr-reviewer/repos/hib/hib-pr-reviewer/1526/bin/compare-modules.tsin full — verified lines 82, 88, 139, 160 exactly as A described.Finding 1 (module-host.ts): Both A and B flag the same file and same four call-sites (79, 87, 116, 132) for free-form template-literal log calls. Agreed finding — kept. A and B cite different primary line anchors (A→79, B→52). Both are accurate: line 52 is the
log?: (message: string) => voidtype declaration that enables the violation; lines 79/87/116/132 are the actual violation call-sites. Merged into one finding anchored at line 52 (root cause) with all four call-sites documented. B's additional recommendation to change the type signature and updatemodule-bootstrap.ts:97is incorporated as it makes the fix more complete.Finding 2 (compare-modules.ts): Unique to A. Lines 82, 88, 139, 160 all confirmed to contain the exact free-form
console.error(\...`)` template literals A cited. Rule 13 violation confirmed. Unique-to-A finding verified — kept.Result: 1 agreed finding, 1 unique-to-A verified finding — both minor. No findings dropped.
Blast Radius
The PR adds a substantial new subsystem (23 files, ~2000 LOC) spanning
src/rmp/,bin/,deploy/, and tests. However the CHANGELOG explicitly states the engine's normal review path is untouched, and the new RMP module infrastructure is behind armp-modulescompose profile. The two log-format violations are confined to a daemon consume-loop and an operator CLI tool respectively — neither touches the core review pipeline.BLAST_SCORE: 4/10
Risk Indicators
registerModule,subscribeCancellations,ensureRequestGroup,ackRequest,claimNextRequest,runReviewViaTransport,runDistributedQuorumCI status (head
ea9c956eef72)Overall: ✓ success
3 checks: 3 pending
Findings (2)
[MINOR] Four
log(…)call-sites emit free-form template strings — rule 13 violation in the long-lived daemon (agreed: A+B)src/rmp/module-host.ts:52
The
ModuleHostOpts.logtype at line 52 is(message: string) => void, and all four operational log call-sites pass plain template literals:module-bootstrap.tswires this aslog: (m) => console.log(m)(line 97), so all four paths land as opaque strings in the dockerjson-filelog driver — a rule 13 violation ("no free-form strings"). This is inconsistent with every other log site inmodule-bootstrap.ts, which was fixed in round 2 to emitJSON.stringify({…}).Recommended fix (three-part):
module-bootstrap.tsline 97:[MINOR] Four
console.error(…)progress lines use free-form template strings — rule 13 violation (unique-to-A, verified)bin/compare-modules.ts:82
Lines 82, 88, 139, and 160 write unstructured template literals to stderr:
Rule 13 requires structured (JSON or key-value) output — no free-form strings. The redis-error path at line 73 already correctly uses
console.log(JSON.stringify({…})), making the inconsistency clear. The error path at line 139 is the highest-impact site: a module-A failure landing as an opaque string cannot be filtered or alerted on by log tooling.Recommended fix:
Note: all four lines also write to stderr instead of stdout, which conflicts with 12-Factor XI ("app writes to stdout only; environment handles routing"). Consider switching to
console.logwith the structured JSON form above.Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 3 • 2 findings (2m) • 2026-06-02T22:44:56.704Z → 2026-06-02T22:46:10.836Z • posted-as: pr-reviewer-bot • model: auto