Skip to content

fix(replay,maintainability): thread the recorded decision clock into three unrecorded reads, delete a dead wire, and add a reachability check (#9492) - #9538

Merged
JSONbored merged 1 commit into
mainfrom
fix/replay-determinism-and-dead-wire
Jul 28, 2026
Merged

fix(replay,maintainability): thread the recorded decision clock into three unrecorded reads, delete a dead wire, and add a reachability check (#9492)#9538
JSONbored merged 1 commit into
mainfrom
fix/replay-determinism-and-dead-wire

Conversation

@JSONbored

@JSONbored JSONbored commented Jul 28, 2026

Copy link
Copy Markdown
Owner

Summary

Two of #9492's three findings, both structural rather than cosmetic: the recorded decision clock reached exactly one consumer while three other clock reads still decided outcomes, and a dead feature file sat green in CI for months with no mechanism that could have noticed.

Closes #9539
Closes #9540

#9539 — replay determinism: three unrecorded reads decided outcomes

#9028/#9256 shipped "replayable time": one Date.now() per decision pass, recorded into the replay input. Its value is destroyed entirely by a second, unrecorded read in the same pass — the recorded instant is then not the instant that decided.

Worse, this failed silently in the direction that matters. replayDecision pins action and only re-derives evaluateGateCheck, so replaying a decision made on an unrecorded read reports verdict: "match" — a false certification, not a caught divergence. The feature would have reported the pipeline deterministic precisely where it wasn't.

Three reads now take the recorded instant:

  • activeMergeBlockedSha — fed the plan and the recorded reason code, from a second Date.now() in the very function whose caller captures decisionClock. Threaded as a required decisionNowMs field, deliberately: optionality is how this class drifts (the approval-queue: accept path builds a thinner executor context — #9159 mutex, moderation ladder and #9055 base guard all silently inert #9482 lesson), and an optional clock lets a future call site reintroduce the gap with no type error.
  • Account age → isNewAccount — halves the contributor cap and can therefore flip a cap close. decisionClock was already in scope, unused. The twin helper isBelowAccountAgeThreshold had no nowMs seam at all; it has one now.
  • resolveUnlinkedIssueMatchDisposition — its velocity-exception gap chooses close vs hold. All three of its clock reads now share one instant, so the DB lookback windows and the gap agree.

The honest-scope note in decision-replay.ts is updated to name what is still live-clock — the three isBelowAccountAgeThreshold callers outside this pass (each its own pass, no captured instant, nothing to be consistent with) and the SQL-side datetime('now', ?) in submitter-reputation.ts, a class no JS-side seam can cover. A scope claim that silently widens is worse than a narrow one.

#9540 — a dead wire, and the missing check for the class

src/review/issue-rag-wire.ts had zero production importers: both real consumers imported the engine module it re-exported directly, and only its own test imported the wire. Coverage cannot catch this — a dead module's own test exercises it perfectly well.

  • Deleted it.
  • scripts/check-dead-source-files.ts (new, in test:ci and CI): every src/** file must have at least one importer outside its own test. Deliberately narrow — no re-export chasing, no unused-export analysis — it answers only the question maintainability: processors.ts (16.2k lines, 437 touches) is the structural cause of this audit's drift and determinism gaps; plus a confirmed dead wire #9492 actually found, cheaply enough to run per-PR.
  • Running it surfaced two more dead files and one genuinely dead feature:
    • src/review/content-lane/index.ts — a barrel with no importers, and already drifted: it omits survivingExistingEntries, which predates this PR. Deleted.
    • src/miner/soft-claim.ts was a shipped-but-unregistered feature. buildSoftClaimSpec landed in feat(miner-plan): soft-claim local-write action-spec builder (#2315) #2813 and no MCP tool ever exposed it, so the capability simply did not exist at runtime. Wired up as loopover_post_soft_claim, following the identical local-write-tools pattern its nine siblings already use.
  • The checker's four apparent false positives were fixed at the root rather than allowlisted: scripts/** is a legitimate importer root (check-migrations.ts and draft-issue.ts are the sole consumers of two live src/** modules), and server.ts plus the two esbuild alias targets are declared entry points with reasons.

Scope

#9492 originally bundled three findings in one issue, which is why this PR had nothing it could close in full. It is now an epic over three sub-issues — #9539 and #9540, both closed here in their entirety, and #9541, the processors.ts splits.

#9541 is deliberately not in this PR. Extracting ~9 command handlers behind a shared dispatch, three advisory spend gates behind one precondition struct, and the plan-and-execute pass into its own typed-context module is a large behaviour-preserving refactor of a 16.2k-line file. Bundling it with behavioural fixes would make both unreviewable, and the risk profiles are entirely different.

Both fixes here reduce that refactor's risk: the required decisionNowMs field is a first instance of exactly the "typed decision-pass context both paths must construct" #9541 calls for, and the reachability check makes a module accidentally orphaned during a split fail CI instead of silently dying.

Validation

  • npx tsc --noEmit, dead-source-files:check, db:migrations:check, selfhost:env-reference:check, actionlint, git diff --check — all clean
  • 264 passed across the guardrail, account-age, clock-threading, checker, merge-block, MCP write-tools, soft-claim and queue-3 suites
  • Patch coverage against this diff: 0 uncovered changed lines

Regressions, verified to fail against the unfixed code: the velocity exception evaluated at a supplied instant diverges from the live clock (using a confirmed miner, the only configuration where the gap is decisive — an earlier draft of this test passed for the wrong reason and was fixed); the repeat lookback shares that instant; account age judged at a supplied instant flips isNewAccount; and the checker flags both the "only its own test imports it" and "no importer at all" shapes.

Invariants: omitting nowMs preserves live-clock behaviour byte-identically at every new seam, so the callers outside the decision pass are untouched; one production importer is always enough; a non-own test still counts as a real importer (a shared helper is alive); scripts/** rescues a module that is otherwise reported dead; declared entry points are never flagged; barrels keep their members alive via export ... from; specifiers reaching outside src/** are ignored rather than miscounted; and the pass still captures exactly one clock instant.

The two processors.ts-internal sites have no export seam, so they are pinned by a source-parity guard — the same convention db-parsers.test.ts already uses for STALE_RECHECK_DENIAL_DETAIL_PATTERN, and the honest alternative to exporting internals purely for a test.

@loopover-orb

loopover-orb Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Warning

⏸️ LoopOver review result - manual review recommended

Review updated: 2026-07-28 08:17:41 UTC

17 files · 1 AI reviewer · 1 blocker · CI green · clean

⏸️ Suggested Action - Manual Review

Review summary
This PR threads the recorded decisionClock into three previously-unrecorded Date.now() reads (activeMergeBlockedSha, isNewAccount, resolveUnlinkedIssueMatchDisposition's velocity exception) so replay reflects the actual instant that decided each outcome, and adds a CI-enforced dead-source-file checker that catches the class of bug that let issue-rag-wire.ts sit unused for months. The clock threading is done carefully: decisionNowMs is required on the plan-input type (preventing silent re-drift) while the guardrail/throttle functions keep nowMs optional with Date.now() fallback to avoid disturbing unrelated callers, and each site is backed by a targeted regression test proving the recorded instant, not the live clock, decides the outcome. The one clearly out-of-scope bundled change is the wholesale deletion of src/review/content-lane/index.ts (135 lines, a public barrel re-exporting a large surface) and the new MCP tool wiring for buildSoftClaimSpec — neither is mentioned in the PR title/description (which is scoped to #9539/#9540) and both are unrelated to replay-clock threading or dead-file detection.

Blockers

Nits — 6 non-blocking
  • src/queue/account-age-throttle.ts and processors.ts hardcode `24 * 60 * 60 * 1000` for a day-in-ms multiple times across files; consider a shared constant, though this is pre-existing style repeated rather than newly introduced by this diff.
  • scripts/check-dead-source-files.ts's specifier regex/resolution is a best-effort text match (no real TS resolver) — worth a one-line callout in CI failure guidance for the rare false-negative (e.g. computed/dynamic imports) even though the header comment already flags this as deliberate.
  • The account-age throttle test comment block is thorough, but consider trimming the multi-line dissertations in decision-clock-threading.test.ts's file-header comment per the repo's own stated preference for concise WHY-only comments.
  • Split the content-lane deletion and the soft-claim MCP wiring into separate PRs (or link an issue for each) so the replay-determinism fix and dead-source-file tooling — the PR's actual stated scope — can be reviewed and merged independently of these two unrelated changes.
  • Confirm no other module still imports from `src/review/content-lane/index.ts` directly (rather than its submodules) before merging the deletion — the dead-source-file checker itself would have caught this had it existed sooner, but it's worth a manual grep pass now.
  • PR author also opened the linked issue — Link an issue that was opened by a different contributor, or provide a rationale for why this self-authored issue represents genuine discovery work.

Concerns raised — review before merging

  • The PR silently removes the public content-lane barrel `src/review/content-lane/index.ts` (isContentLaneEnabled, AWESOME_CLAUDE_CONTENT_SPEC, runSurfaceReview, etc. — a public entrypoint) with no mention in the title/description of a content-lane removal; if any external or same-repo consumer still imports from this barrel path (as opposed to individual submodules), this is a breaking change bundled into an unrelated replay-determinism/dead-file PR with no visible justification in the diff.
📋 Copy for AI agents — paste into your coding agent
Fix the following blocker(s) from this PR review:

1. The PR silently removes the public content-lane barrel \`src/review/content-lane/index.ts\` \(isContentLaneEnabled, AWESOME\_CLAUDE\_CONTENT\_SPEC, runSurfaceReview, etc. — a public entrypoint\) with no mention in the title/description of a content-lane removal; if any external or same-repo consumer still imports from this barrel path \(as opposed to individual submodules\), this is a breaking change bundled into an unrelated replay-determinism/dead-file PR with no visible justification in the diff.

Decision drivers

  • ❌ Code review — 1 blocker (1 reviewer)
  • ❌ Gate result — Blocking (Repo-configured hard blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #9539, #9540
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ❌ 8/20 High review scope from cached public metadata (2 linked issues).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 14 registered-repo PR(s), 13 merged, 326 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 14 PR(s), 326 issue(s).
Improvement ✅ Minor risk: clean · value: minor
Linked issue satisfaction

Addressed
The diff threads decisionClock.nowMs into all three cited reads (activeMergeBlockedSha, account-age isNewAccount, and resolveUnlinkedIssueMatchDisposition), makes the plan-input clock field required as requested, adds a nowMs seam to isBelowAccountAgeThreshold, and extends the honest-scope note in decision-replay.ts to name remaining live-clock reads including submitter-reputation.ts's SQL-side cl

Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Python, TypeScript, Ruby, Go, MDX, Shell, Solidity, JavaScript
  • Official Gittensor activity: 14 PR(s), 326 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Start here: Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Then work through the remaining 2 steps in the Signals table above.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
🧪 Chat with LoopOver

Ask LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @loopover ask <question> answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat <question> answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

Decision record
  • action: hold · clause: ai_consensus_defect
  • config: 9d58aa591e39af66e420511987aad4a42d70b87d9bc98df237032a877b010fdd · pack: oss-anti-slop · ci: failed
  • model: claude-code · prompt: fb2e25403180ae7550fe55742b1d5051563f41abd7fea6c267e6b6c8ae65552d · confidence: 0.55
  • record: 93087ab7dd538cefe3978de1bc0d8e246fe76e23d25af221f7e0469a2bc11709 (schema v5, head a49ab77)

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.

  • Re-run LoopOver review

@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Logic backtest

Replayed 0 historical case(s) for linked_issue_scope_mismatch through the base (8751b9a) and head (208948d) versions of its detection logic (corpus checksum 4f53cda18c2b).

Backtest comparison: linked_issue_scope_mismatch

Verdict: unchanged — no comparable axis moved.

Advisory only — this check never blocks merge (#8105).

@JSONbored JSONbored self-assigned this Jul 28, 2026
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jul 28, 2026

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
loopover-ui 208948d Commit Preview URL

Branch Preview URL
Jul 28 2026, 08:00 AM

@codecov

codecov Bot commented Jul 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.58%. Comparing base (8751b9a) to head (208948d).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #9538   +/-   ##
=======================================
  Coverage   89.58%   89.58%           
=======================================
  Files         846      846           
  Lines      110384   110388    +4     
  Branches    26295    26297    +2     
=======================================
+ Hits        98889    98893    +4     
  Misses      10231    10231           
  Partials     1264     1264           
Flag Coverage Δ
backend 95.29% <100.00%> (-0.01%) ⬇️
control-plane 99.86% <ø> (ø)
rees 89.62% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/mcp/server.ts 97.14% <100.00%> (+<0.01%) ⬆️
src/miner/soft-claim.ts 100.00% <ø> (ø)
src/queue/account-age-throttle.ts 100.00% <100.00%> (ø)
src/queue/processors.ts 94.83% <100.00%> (ø)
src/review/decision-replay.ts 100.00% <ø> (ø)
src/review/unlinked-issue-guardrail.ts 100.00% <100.00%> (ø)

@codecov

codecov Bot commented Jul 28, 2026

Copy link
Copy Markdown

Bundle Report

Bundle size has no change ✅

@loopover-orb loopover-orb Bot added the manual-review Gittensor contributor context label Jul 28, 2026
@JSONbored
JSONbored force-pushed the fix/replay-determinism-and-dead-wire branch from a49ab77 to 6147e30 Compare July 28, 2026 07:40
@JSONbored

Copy link
Copy Markdown
Owner Author

CI failure fixed and pushed.

loopover_post_soft_claim — the tool this PR wires up after finding it shipped-but-unregistered — needed an entry in MCP_TOOL_CATEGORIES. The mcp-tool-categories drift guard caught it, which is the guard working exactly as intended: a newly registered tool with no category is precisely what it exists to block.

That is a fair hit on me. The irony is not lost — this PR adds a reachability check for silently dead code, and I introduced a silently uncategorized tool doing it. The lesson is the same one #9540 encodes: a registry with no compile-time totality (this map is a plain Record, not a total Record<ToolName, Category>) drifts the moment a producer is added elsewhere. feature-activation.ts's total-Record-over-a-key-union pattern is the fix for that class, and it is worth applying to this map — noted in #9541, since it belongs with the structural work rather than bolted onto this PR.

I also checked whether the stdio MCP server (packages/loopover-mcp/bin/loopover-mcp.ts) needed the same entry. It maintains its own independent tool list, and the full CI suite flagged only the categories map, so no parity check spans the two servers — that hand-mirroring is exactly what #9537 tracks. Left alone deliberately rather than widened here.

Rebased onto current main; mcp-tool-categories and both write-tools suites green.

@JSONbored
JSONbored force-pushed the fix/replay-determinism-and-dead-wire branch from 6147e30 to c351acd Compare July 28, 2026 07:40
…three unrecorded reads, delete a dead wire, and add a reachability check (#9539, #9540)
@JSONbored
JSONbored force-pushed the fix/replay-determinism-and-dead-wire branch from c351acd to 208948d Compare July 28, 2026 07:57
@JSONbored
JSONbored merged commit 78061a0 into main Jul 28, 2026
11 checks passed
@JSONbored
JSONbored deleted the fix/replay-determinism-and-dead-wire branch July 28, 2026 08:18
@github-actions github-actions Bot mentioned this pull request Jul 28, 2026
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

manual-review Gittensor contributor context

Projects

None yet

1 participant