fix(miner): discover --dry-run must not create/migrate/prune the event ledger (#9679) - #9905
Merged
loopover-orb[bot] merged 1 commit intoJul 29, 2026
Conversation
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Contributor
|
Tip ✅ LoopOver review result - approve/merge recommendedReview updated: 2026-07-29 21:19:36 UTC
Review summary Nits — 4 non-blocking
Decision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionAddressed Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk 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.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. 🟩 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.
|
…t ledger (JSONbored#9679) `runDiscover`'s `--dry-run` contract (discover-cli.ts:228, :537-540) is "zero writes", but the min-rank-override block that sits above the dry-run branch called `initEventLedger` unconditionally. initEventLedger creates the ledger file, applies schema migrations, AND runs retention pruning (which can delete rows) -- three classes of write on a command whose whole point is to make none, and the only store the dry-run path still touched. The min-rank value is genuinely wanted on the dry-run preview, so read it WITHOUT creating the store: guard the initEventLedger call with `existsSync(resolveEventLedgerDbPath(env))` when parsed.dryRun is true, falling back to AMS_MIN_RANK_SHIPPED when the file is absent -- exactly the value an empty/new ledger would yield. Same "skip a file that doesn't exist yet" discipline as migrate-cli.ts / store-maintenance.ts. The non-dry-run path is unchanged (unconditional initEventLedger). The existing try/catch/finally fail-open shape is preserved. Three named cases: --dry-run against a config dir with no ledger leaves no ledger file (fails against current code); --dry-run against an EXISTING ledger still consumes the earned min-rank override (0.2), proving the preview isn't degraded; the non-dry-run path still creates the ledger. Closes JSONbored#9679 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
loopover-orb
Bot
added
the
gittensor:bug
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9905 +/- ##
==========================================
+ Coverage 79.29% 79.36% +0.06%
==========================================
Files 281 282 +1
Lines 58566 58817 +251
Branches 6785 6874 +89
==========================================
+ Hits 46440 46678 +238
- Misses 11843 11844 +1
- Partials 283 295 +12
Flags with carried forward coverage won't be shown. Click here to find out more.
|
![loopover-orb[bot]](https://avatars.githubusercontent.com/in/4139483?s=60&v=4){kind=small}
7 tasks
JSONbored
added a commit
that referenced
this pull request
Jul 30, 2026
Refs #9948. release-please DROPS a commit whose title it cannot parse -- no version bump, no changelog entry -- and the job still reports success. Two shipped fix(miner) commits were invisible to the release when this was found, and the only evidence was buried in the run's logs. The shape is a double reference suffix, '(#9679) (#9905)', which this repo produces whenever a PR title already carries the issue number and the merge appends the PR number. Checked BEFORE release-please runs rather than by scraping its logs afterwards: a title is a fact available at commit time, and a guard on the input is one nobody has to remember to read. Scoped to the commits a branch adds over its base, NOT to everything since the last release tag. Git history is immutable and main already carries two titles of this shape, so a check that failed on those would be permanently red, would block every unrelated PR, and would be switched off within a day. Guarding new commits stops the class going forward, which is the only thing a pre-merge gate can honestly do about titles that already shipped. The pattern is deliberately narrow -- it matches the one shape observed to break the parser rather than reimplementing the conventional-commits grammar, which would drift from the real parser and start rejecting titles release-please handles fine.
JSONbored
added a commit
that referenced
this pull request
Jul 30, 2026
Refs #9948. release-please DROPS a commit it cannot parse -- it contributes to neither the version bump nor the changelog -- and the run still reports success. Two shipped fix(miner) commits were invisible to the release when this was found. The first cut of this guard matched the double issue/PR suffix in the subject, '(#9679) (#9905)', because that is what the failing commits visibly had in common. Driving release-please's actual parser proves that shape parses fine. The real trigger is NESTED PARENTHESES IN THE BODY, and it is position-sensitive -- the parser reports a column, and the same nested text parses when it sits further along the line. That guard would have caught nothing while reading as protection, which is worse than no guard. So this matches nothing. It runs the parser release-please itself uses and asserts every commit on the branch survives it, with release-please added as a devDependency and its own exported type used for the signature so an upgrade that changes the API is a build error rather than a silently-wrong cast. Scoped to the branch's own commits: history is immutable, and a check that failed on what already shipped would be permanently red and switched off within a day. A fixture holds the real dropped message, with a regression test asserting it STILL fails to parse -- so if an upgrade ever fixes the parser, that test goes red and says the guard can be relaxed, rather than silently protecting nothing.
JSONbored
added a commit
that referenced
this pull request
Jul 30, 2026
… provision the anchor key without displaying it (#9957) * ci(release): fail on a commit title release-please cannot parse Refs #9948. release-please DROPS a commit whose title it cannot parse -- no version bump, no changelog entry -- and the job still reports success. Two shipped fix(miner) commits were invisible to the release when this was found, and the only evidence was buried in the run's logs. The shape is a double reference suffix, '(#9679) (#9905)', which this repo produces whenever a PR title already carries the issue number and the merge appends the PR number. Checked BEFORE release-please runs rather than by scraping its logs afterwards: a title is a fact available at commit time, and a guard on the input is one nobody has to remember to read. Scoped to the commits a branch adds over its base, NOT to everything since the last release tag. Git history is immutable and main already carries two titles of this shape, so a check that failed on those would be permanently red, would block every unrelated PR, and would be switched off within a day. Guarding new commits stops the class going forward, which is the only thing a pre-merge gate can honestly do about titles that already shipped. The pattern is deliberately narrow -- it matches the one shape observed to break the parser rather than reimplementing the conventional-commits grammar, which would drift from the real parser and start rejecting titles release-please handles fine. * feat(ledger): provision the anchor key without ever displaying it Refs #9940. The keygen printed the private key to stdout, so provisioning meant a human copying a signing key out of a terminal -- where it lands in scrollback, in shell history if piped, and in whatever captured the session. One of ours had to be discarded for exactly that reason. `--secret-file <path>` writes the private half directly at 0600 and prints only the public half, which is meant to be published. Nothing to copy, nothing to leak. Point LOOPOVER_LEDGER_ANCHOR_PRIVATE_KEY_FILE at the path -- the generic <NAME>_FILE mechanism every self-host secret already supports -- and recreate the container. Refuses to overwrite an existing file: replacing a live signing key strands every anchor published under it, which is the same reason a retired key is closed rather than removed. The printing mode stays for reading the shape before provisioning. * ci(release): fail on a commit release-please would silently drop Refs #9948. release-please DROPS a commit it cannot parse -- it contributes to neither the version bump nor the changelog -- and the run still reports success. Two shipped fix(miner) commits were invisible to the release when this was found. The first cut of this guard matched the double issue/PR suffix in the subject, '(#9679) (#9905)', because that is what the failing commits visibly had in common. Driving release-please's actual parser proves that shape parses fine. The real trigger is NESTED PARENTHESES IN THE BODY, and it is position-sensitive -- the parser reports a column, and the same nested text parses when it sits further along the line. That guard would have caught nothing while reading as protection, which is worse than no guard. So this matches nothing. It runs the parser release-please itself uses and asserts every commit on the branch survives it, with release-please added as a devDependency and its own exported type used for the signature so an upgrade that changes the API is a build error rather than a silently-wrong cast. Scoped to the branch's own commits: history is immutable, and a check that failed on what already shipped would be permanently red and switched off within a day. A fixture holds the real dropped message, with a regression test asserting it STILL fails to parse -- so if an upgrade ever fixes the parser, that test goes red and says the guard can be relaxed, rather than silently protecting nothing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
runDiscover's--dry-runcontract (discover-cli.ts:228,:537-540) is "fetches + ranks exactly as a real run, but makes zero writes". The min-rank-override block that sits above the dry-run branch calledinitEventLedgerunconditionally, andinitEventLedgercreates the ledger file, applies schema migrations, and runspruneLedgerByRetention(which can delete rows whenLOOPOVER_MINER_LEDGER_RETENTION_DAYS/_MAX_ROWSare set). That is three classes of write on a command whose entire point is to make none — and the event ledger was the only store the dry-run path still touched.How
The min-rank value is genuinely wanted on the dry-run preview (so it shows the exact skip set a real run would produce), so read it without creating the store: guard the
initEventLedger(...)call withexistsSync(resolveEventLedgerDbPath(env))whenparsed.dryRunis true, falling back toAMS_MIN_RANK_SHIPPEDwhen the file is absent — exactly the value an empty/new ledger would yield. Same "skip a file that doesn't exist yet" discipline asmigrate-cli.ts:104/store-maintenance.ts:117. The non-dry-run path keeps its unconditionalinitEventLedger, and the existingtry/catch/finallyfail-open shape is preserved.Tests
Three named cases in
test/unit/miner-discover-cli.test.ts:--dry-runagainst a config dir with no event ledger leaves no ledger file afterwards (fails against the current code).--dry-runagainst an existing ledger carrying a min-rank override still consumes it (minRankScore=0.2), proving the preview isn't silently degraded.Together they cover all three arms of the new
!parsed.dryRun || existsSync(...)condition. The consumer test fileminer-discover-cli-eligibility-metadata.test.tsstill passes.Closes #9679