Skip to content

feat(mcp): wire gittensory_feasibility_gate's claimStatus to the local claim ledger (#5157)#5389

Merged
loopover-orb[bot] merged 4 commits into
JSONbored:mainfrom
joaovictor91123:feat/mcp-feasibility-gate-ledger-5157
Jul 12, 2026
Merged

feat(mcp): wire gittensory_feasibility_gate's claimStatus to the local claim ledger (#5157)#5389
loopover-orb[bot] merged 4 commits into
JSONbored:mainfrom
joaovictor91123:feat/mcp-feasibility-gate-ledger-5157

Conversation

@joaovictor91123

Copy link
Copy Markdown
Contributor

Summary

  • gittensory_feasibility_gate in packages/gittensory-mcp/bin/gittensory-mcp.js took claimStatus as a purely caller-supplied string -- the pure feasibility calculator was never actually connected to real local claim state, even though packages/gittensory-miner/lib/claim-ledger.js already tracks it.
  • Adds optional repoFullName/issueNumber inputs to feasibilityGateShape. When both are supplied AND a local gittensory-miner install's claim ledger DB file exists, claimStatus is now read from that ledger (an active claim on the exact issue -> "claimed", otherwise "unclaimed") instead of trusting the caller-supplied value.
  • Genuinely, driver-enforced read-only: adds openClaimLedgerReadOnly to claim-ledger.js, which opens the DB with node:sqlite's own readOnly: true mode and touches the filesystem in no other way (no mkdirSync/chmodSync, no CREATE TABLE IF NOT EXISTS, no schema migrations). Never calls recordClaim/releaseClaim/expireClaim, and never gains any ability to block, cancel, or override a claim or attempt -- real claim-conflict authority stays entirely with the maintainer-only path (Wire claim-conflict resolution end-to-end #4848).
  • Falls back to today's caller-supplied-string behavior, completely unchanged, when repoFullName/issueNumber are omitted, the ledger DB file doesn't exist, or @jsonbored/gittensory-miner isn't resolvable at all.
  • When the ledger DB file does exist but reading it fails -- corrupt, empty/uninitialized, permission denied -- this reports the existing claimStatus: "unknown" value instead of silently trusting a caller-supplied string that might contradict ground truth we can't currently read.
  • The feasibility calculator's own decision logic (buildFeasibilityVerdict) is completely untouched.

Resubmission history

Fourth submission for #5157, each round fixing a real, distinct finding from the review gate rather than resubmitting unchanged:

  1. feat(mcp): wire gittensory_feasibility_gate's claimStatus to the local claim ledger (#5157) #5361: the original catch swallowed every ledger error, not just "package not found" -- fixed by returning "unknown" for an existing-but-unreadable ledger.
  2. feat(mcp): wire gittensory_feasibility_gate's claimStatus to the local claim ledger (#5157) #5374 (held, not closed): resolveLedgerClaimStatus still opened via the writable openClaimLedger (schema-init DDL on every open -- a write, even against an empty file) -- fixed by adding a genuinely read-only opener. Also caught and fixed a node:sqlite footgun in the process: the lowercase readonly option key is silently ignored; only camelCase readOnly actually enforces it.
  3. feat(mcp): wire gittensory_feasibility_gate's claimStatus to the local claim ledger (#5157) #5383: codecov/patch failed at 0% -- packages/gittensory-miner/** turned out to now be inside Codecov's coverage.include scope (a recent milestone change), and my new openClaimLedgerReadOnly was only exercised indirectly through a spawned MCP subprocess, which coverage instrumentation can't see. Fixed by adding direct unit tests for it in the same test process. Writing those tests surfaced one more real bug in my own code: openClaimLedgerReadOnly's db.prepare() throws when the expected table is missing, and the already-opened DatabaseSync handle was never closed on that path -- a file-handle leak. Fixed with a try/catch that closes the connection before rethrowing, with a regression test (an existing-but-empty SQLite file) that reproduced it.

Test plan

  • npx vitest run test/unit/mcp-feasibility-gate.test.ts -- 14/14 passing (E2E stdio-subprocess tests).
  • npx vitest run test/unit/miner-claim-ledger.test.ts -- 33/35 passing; 2 pre-existing, unrelated Windows-only failures (a path.join separator mismatch and a POSIX-permission-bit check that doesn't apply on NTFS), confirmed via git stash to exist identically without this PR. New describe("openClaimLedgerReadOnly (#5157)", ...) block directly unit-tests: listing active claims scoped by repo, an empty-result case, malformed-input rejection, opening a nonexistent path, a regression pinning the exact readOnly vs. readonly key gotcha (asserts a write against a {readOnly: true} connection throws), and a regression for the file-handle leak (an existing-but-empty SQLite file gains zero tables and the process doesn't error on cleanup).
  • npm run typecheck -- clean.
  • npm run build:mcp / npm run build:miner -- clean.
  • npm run docs:drift-check -- clean.
  • Swept the full diff for secret-shaped patterns -- zero matches.
  • Manually verified isolated coverage of the new openClaimLedgerReadOnly function is 100% within the full (minus the 2 known pre-existing failures) miner-claim-ledger.test.ts run -- the only uncovered lines belong to pre-existing, untouched code (expireClaim and unused top-level ledger aliases).
  • Did not run the full unsharded npm run test:coverage locally (shared/resource-contended machine); relying on the targeted test runs above plus the isolated-coverage check.

Fixes #5157.

…l claim ledger (JSONbored#5157)

gittensory_feasibility_gate took claimStatus as a purely caller-supplied
string, never actually connected to real local claim state, even though
packages/gittensory-miner/lib/claim-ledger.js already tracks it.

Adds optional repoFullName/issueNumber inputs. When both are supplied
and a local gittensory-miner install's claim ledger DB file exists,
claimStatus is now read from that ledger (an active claim on the exact
issue -> "claimed", otherwise "unclaimed") instead of trusting the
caller-supplied value. The DB file's existence is checked BEFORE
opening anything, so this advisory-only tool never creates the ledger
as a side effect -- it stays strictly read-only, never calls
recordClaim/releaseClaim/expireClaim, and never gains any ability to
block, cancel, or override a claim or attempt; real claim-conflict
authority remains entirely with the maintainer-only path. Falls back
to today's caller-supplied-string behavior unchanged when repo/issue
are omitted, the ledger file doesn't exist, or gittensory-miner isn't
resolvable at all. The feasibility calculator's own decision logic is
untouched -- this only changes where claimStatus is sourced from.
…ad of silently trusting the caller (JSONbored#5157)

resolveLedgerClaimStatus's catch block previously swallowed every
error from opening/querying the ledger, not just "package not found" --
so a real local install whose ledger DB file exists but is corrupt,
locked, or unreadable would silently fall back to a caller-supplied
claimStatus that might contradict the (unreadable) ground truth.

Narrows the fallback: module-resolution failure or a missing DB file
still return null (fall back to the caller-supplied value -- correct,
since there's genuinely nothing local to check). A DB file that exists
but fails to open/query now returns the existing "unknown" claimStatus
value instead, which the calculator already treats as a neutral
signal -- honest about the read failure rather than guessing.
…writable openClaimLedger (JSONbored#5157)

resolveLedgerClaimStatus opened the claim ledger via openClaimLedger,
which always runs CREATE TABLE IF NOT EXISTS plus a schema-version
stamp on open -- a write, even against a file that merely exists but
is empty/uninitialized. That contradicted this advisory-only tool's
strictly-read-only guarantee: existsSync only confirms a file is
there, not that its schema has already been created.

Adds openClaimLedgerReadOnly to claim-ledger.js: opens the DB with
node:sqlite's own `readOnly: true` mode (verified empirically that the
lowercase `readonly` key is silently ignored and opens read-write
anyway -- a real footgun, now called out in a comment) and touches the
filesystem in no other way -- no mkdir/chmod, no CREATE TABLE, no
migrations. This keeps the schema/query logic centralized in
claim-ledger.js while getting a guarantee enforced by the SQLite
driver itself, not just by which methods happen to get called.
gittensory-mcp.js's resolveLedgerClaimStatus now uses this instead of
openClaimLedger.
…t unit coverage for openClaimLedgerReadOnly (JSONbored#5157)

openClaimLedgerReadOnly's db.prepare() can throw when the expected
table is missing (a file exists but isn't a real claim ledger) --
before this fix, that left the already-opened DatabaseSync handle
unclosed, leaking a file handle every time this path was hit.

Also adds direct unit tests for openClaimLedgerReadOnly in the same
test process (the prior tests only exercised it indirectly through a
spawned MCP subprocess, which coverage instrumentation can't see) so
Codecov's patch-coverage gate can actually measure this new function:
listing active claims, scoping by repo, an empty-result case,
malformed-input rejection, opening a nonexistent path, opening an
existing-but-empty file (the resource-leak repro), and a regression
pinning the exact readOnly-vs-readonly key gotcha the read-only
guarantee depends on.
@superagent-security

Copy link
Copy Markdown
Contributor

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

@codecov

codecov Bot commented Jul 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.44%. Comparing base (56d901d) to head (dc52111).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5389   +/-   ##
=======================================
  Coverage   94.43%   94.44%           
=======================================
  Files         551      551           
  Lines       44223    44233   +10     
  Branches    14654    14654           
=======================================
+ Hits        41764    41774   +10     
  Misses       1784     1784           
  Partials      675      675           
Flag Coverage Δ
shard-1 44.16% <0.00%> (-0.01%) ⬇️
shard-2 34.63% <0.00%> (+0.05%) ⬆️
shard-3 31.53% <0.00%> (-0.01%) ⬇️
shard-4 31.29% <100.00%> (+0.01%) ⬆️
shard-5 33.32% <0.00%> (-0.01%) ⬇️
shard-6 43.37% <0.00%> (-0.01%) ⬇️

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

Files with missing lines Coverage Δ
packages/gittensory-miner/lib/claim-ledger.js 95.45% <100.00%> (+0.58%) ⬆️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@loopover-orb loopover-orb Bot added the gittensor:feature Gittensor-scored feature linked to a feature issue — scores a 0.25x multiplier. label Jul 12, 2026
@loopover-orb

loopover-orb Bot commented Jul 12, 2026

Copy link
Copy Markdown

Tip

🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩

✅ Gittensory review result - approve/merge recommended

Review updated: 2026-07-12 16:17:51 UTC

5 files · 2 AI reviewers · no blockers · readiness 100/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This PR wires gittensory_feasibility_gate's claimStatus to a real local claim-ledger read when repoFullName/issueNumber are supplied and a local install's ledger DB exists, using a driver-enforced readOnly:true sqlite connection (openClaimLedgerReadOnly) that never runs the write-on-open schema DDL that openClaimLedger always does. Fallback behavior is preserved when inputs are omitted, the DB file doesn't exist, or the miner package isn't resolvable, and unreadable/corrupt DB files honestly report 'unknown' rather than silently trusting the caller-supplied string. The implementation is careful and the new tests are substantive (they exercise real sqlite files, verify the readOnly vs readonly key gotcha, and assert no schema is created on an empty file), giving good confidence the claimed behavior actually holds.

Nits — 5 non-blocking
  • The external brief's 'magic numbers' and 'master' terminology flags are false positives — Wire gittensory_feasibility_gate's claimStatus input to the local claim-ledger instead of a caller-supplied string #5157 is an issue reference in a comment, not a magic number, and no 'master' terminology appears in the diff shown.
  • packages/gittensory-mcp/bin/gittensory-mcp.js: the resolveLedgerClaimStatus try/catch around the dynamic import silently swallows any import error (not just module-not-found), so a real bug in the miner package's claim-ledger.js (e.g. a thrown syntax error) would also fall back to caller-supplied claimStatus rather than surfacing as 'unknown' — worth confirming this is intentional.
  • The tool description addition is quite long for an MCP description string; consider trimming to the essential behavior change plus the advisory-only guarantee.
  • Consider whether resolveLedgerClaimStatus's blanket catch on the dynamic import should be narrowed (e.g. checking error.code === 'ERR_MODULE_NOT_FOUND') so that genuine bugs in claim-ledger.js don't silently look identical to 'miner not installed'.
  • The PR description claims this is the fourth resubmission fixing distinct review findings — worth double-checking the linked issue Wire gittensory_feasibility_gate's claimStatus input to the local claim-ledger instead of a caller-supplied string #5157 is still open/unclosed at merge time per the repo's issue-linking requirement.
Signal Result Evidence
Code review ✅ No blockers 2 reviewers, synthesized
Linked issue ✅ Linked #5157
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 127 registered-repo PR(s), 74 merged, 11 issue(s).
Contributor context ✅ Confirmed Gittensor contributor joaovictor91123; Gittensor profile; 127 PR(s), 11 issue(s).
Gate result ✅ Passing No configured blocker found.
Improvement ✅ Minor risk: clean · value: minor — Code changes are accompanied by test evidence. LLM value judgment: moderate — The diff closes a real gap (a 'pure' feasibility tool silently trusting a caller-supplied claimStatus instead of consulting ground-truth local claim state) with a narrowly-scoped, well-tested, genuinely read-only implementation that doesn't expand the tool's authority beyond advisory reporting.
Linked issue satisfaction

Addressed
The PR adds repoFullName/issueNumber inputs, sources claimStatus from a genuinely read-only claim-ledger connection (openClaimLedgerReadOnly) when a local install's DB file is detected, falls back to the caller-supplied string otherwise, leaves buildFeasibilityVerdict untouched, and ships extensive unit/regression/invariant tests covering both branches plus the read-only/no-write guarantee.

Review context
  • Author: joaovictor91123
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: not available
  • Official Gittensor activity: 127 PR(s), 11 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Keep the PR focused and include validation evidence before maintainer review.
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.
[BETA] Chat with Gittensory

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

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

Full command reference: https://gittensory.aethereal.dev/docs/gittensory-commands

🟩 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 Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@loopover-orb loopover-orb Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gittensory approves — the gate is satisfied and CI is green.

@loopover-orb
loopover-orb Bot merged commit 91b235d into JSONbored:main Jul 12, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:feature Gittensor-scored feature linked to a feature issue — scores a 0.25x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Wire gittensory_feasibility_gate's claimStatus input to the local claim-ledger instead of a caller-supplied string

1 participant