Skip to content

feat(miner): capture feasibility-verdict rule-fired signals in attempt-cli (#8543) - #8558

Closed
philluiz2323 wants to merge 1 commit into
JSONbored:mainfrom
philluiz2323:attempt-cli-feasibility-signals-8543
Closed

feat(miner): capture feasibility-verdict rule-fired signals in attempt-cli (#8543)#8558
philluiz2323 wants to merge 1 commit into
JSONbored:mainfrom
philluiz2323:attempt-cli-feasibility-signals-8543

Conversation

@philluiz2323

Copy link
Copy Markdown
Contributor

Summary

AMS's calibration capture was one-sided: the four eligibility-exclusion reasons are recorded as rule-fired signals on discovery (recordEligibilityExclusionSignals, discover-cli.ts), but the feasibility verdict — the other deterministic gate an attempt passes through — recorded nothing. In attempt-cli.ts, when buildCodingTaskSpec returns ready: false, the verdict's reasons (codingTaskSpec.feasibility.avoidReasons / raiseReasons) were printed and dropped.

This PR wires capture into that existing infeasible branch only, mirroring discover-cli.ts's recordEligibilityExclusionSignals exactly:

  • Records one RuleFiredEvent per avoidReasons entry (outcome: "avoid") and per raiseReasons entry (outcome: "raise"), with ruleId = the reason string verbatim and targetKey = `${repoFullName}#issue-${issueNumber}` — byte-identical to the eligibility-exclusion format.
  • No metadata (raw-context capture is a separate issue, per the linked issue's own scope note).
  • Store acquisition mirrors discover-cli.ts: an optional initSignalTrackingStore?: () => SignalStore seam, defaulting to createSignalTrackingStore({ appendEvent, readEvents }) over the shared local event ledger.
  • Best-effort discipline, mirrored verbatim: store-init failure and each individual write failure are swallowed (try/catch around init, .catch(() => undefined) per write) and never change the CLI's console output, JSON result shape, or exit code.
  • Every ready: false occurrence records — no dedup across repeated attempts at the same issue.

Only attempt-cli.ts + its tests are touched. No pure-module changes (coding-task-spec.ts / packages/loopover-engine/src/feasibility.ts stay untouched), no override capture, no output-format changes.

Scope

  • packages/loopover-miner/lib/attempt-cli.ts + test/unit/miner-attempt-cli.test.ts only.
  • Narrow, single-purpose change matching the linked issue's explicit boundaries.

Validation

  • npm --workspace @loopover/miner run build:tsc — clean
  • npm run typecheck — clean
  • npx vitest run test/unit/miner-attempt-cli.test.ts — all green except 2 pre-existing, unrelated Windows-local-only EBUSY: resource busy or locked temp-dir cleanup races — reproduced identically on a git stash'd clean baseline before this change (same failures, same tests, before any of this PR's code existed), so confirmed environment noise, not a regression.
  • git diff --check upstream/main HEAD — clean, no trailing whitespace.
  • ui:* checks — skipped (backend/CLI-only change, no apps/gittensory-ui files touched).

New coverage

  • ready: false with both avoid and raise reasons present → exact ruleId/outcome/targetKey asserted per event (3 reasons → 3 calls), no metadata, well-formed ISO occurredAt.
  • ready: true → zero fired events recorded.
  • initSignalTrackingStore throwing → exit code, console output, and JSON result identical to a normal infeasible run.
  • A store whose recordRuleFired rejects → same identity check, plus confirms the call was actually attempted (not silently skipped).
  • The 3 pre-existing ready: false tests not otherwise asserting on signal capture now inject a no-op SignalStore double, keeping them off the real on-disk event-ledger fallback under ~/.config (the same leak class discover-cli.ts's own initDefaultSignalTrackingStore comment documents).

Safety

  • No secrets, tokens, wallets, hotkeys/coldkeys, trust scores, or reward/payout values in code, comments, tests, or this PR text.
  • No auth/session/CORS surface touched.
  • Best-effort discipline covered directly: a throwing store-init and a rejecting write are both asserted to leave the CLI's exit code, output, and JSON result completely unchanged.

Notes

Linked issue: #8543 (parent epic: #8107)

…t-cli (#8543)

AMS's calibration capture was one-sided: eligibility-exclusion reasons were already recorded as rule-fired signals on discovery, but the other deterministic gate an attempt passes through -- the feasibility verdict -- recorded nothing. Wires attempt-cli's existing infeasible branch (ready: false) to record one RuleFiredEvent per avoid/raise reason through createSignalTrackingStore, mirroring discover-cli's recordEligibilityExclusionSignals exactly: same targetKey format, same best-effort discipline (a store-open failure or a single write failure never changes console output, JSON result shape, or exit code), same initSignalTrackingStore seam shape. No pure-module changes, no metadata, no output-format changes -- attempt-cli.ts and its tests only.
@philluiz2323
philluiz2323 requested a review from JSONbored as a code owner July 24, 2026 20:48
@superagent-security

Copy link
Copy Markdown
Contributor

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

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 24, 2026
@codecov

codecov Bot commented Jul 24, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.56%. Comparing base (59faa47) to head (be469c4).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
packages/loopover-miner/lib/attempt-cli.ts 0.00% 15 Missing ⚠️

❌ Your patch status has failed because the patch coverage (0.00%) is below the target coverage (99.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8558      +/-   ##
==========================================
- Coverage   92.50%   88.56%   -3.95%     
==========================================
  Files         794       98     -696     
  Lines       79557    22968   -56589     
  Branches    24037     3956   -20081     
==========================================
- Hits        73597    20341   -53256     
+ Misses       4800     2449    -2351     
+ Partials     1160      178     -982     
Flag Coverage Δ
backend 0.00% <0.00%> (-93.68%) ⬇️

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

Files with missing lines Coverage Δ
packages/loopover-miner/lib/attempt-cli.ts 0.00% <0.00%> (-100.00%) ⬇️

... and 696 files with indirect coverage changes

@loopover-orb

loopover-orb Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Caution

🛑 LoopOver review result - reject/close recommended

Review updated: 2026-07-24 20:54:58 UTC

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

🛑 Suggested Action - Reject/Close

Review summary
This PR wires feasibility-verdict rule-fired signal capture into attempt-cli.ts's existing infeasible branch, mirroring discover-cli.ts's recordEligibilityExclusionSignals pattern exactly: one RuleFiredEvent per avoidReasons/raiseReasons entry, best-effort store init and per-write error swallowing, no output/exit-code changes. The implementation is correct and narrowly scoped — the new recordFeasibilityVerdictSignals call sits right before the existing attemptLog/eventLedger writes in the ready:false branch, and the four new tests (successful capture, ready:true no-op, throwing init, rejecting write) exercise the real code paths with fakes, not fabricated states. The occurredAt uses `nowMs` (options.nowMs ?? Date.now(), captured once at function entry) rather than a fresh timestamp per signal, which is a reasonable and intentional choice for consistency across the batch.

Nits — 5 non-blocking
  • attempt-cli.ts:296-330 the new `initDefaultSignalTrackingStore` and `recordFeasibilityVerdictSignals` functions push this already-571-line file further past the 400-line threshold; consider whether signal-capture logic belongs in a small dedicated helper module the way discover-cli.ts's own recordEligibilityExclusionSignals likely does.
  • test/unit/miner-attempt-cli.test.ts:1276-1279 the metadata/timestamp assertions loop only checks `event` from `store.recordRuleFired.mock.calls`, relying on call order matching insertion order — fine here since Promise.all isn't used (writes are sequential via the for-loop's `await`), but worth a comment noting that ordering guarantee since it's load-bearing for the test's per-call assertions.
  • attempt-cli.ts:317-321 the `fired` array's outcome literal-casting (`as const`) is repeated per map call instead of being defined once alongside the type alias — minor duplication, not worth a refactor on its own.
  • Consider verifying discover-cli.ts's recordEligibilityExclusionSignals doesn't itself live in a separate signal-capture helper module — if it does, extracting recordFeasibilityVerdictSignals to match that same file structure would keep the two capture paths consistent and trim attempt-cli.ts's size.
  • The PR description says 'no metadata (raw-context capture is a separate issue)' — worth double-checking that the linked issue's scope note is still accurate before merge, since AMS's later ability to do rule-precision scoring may eventually need that context.

Why this is blocked

  • No linked issue detected: No closing reference or linked issue number was found in the PR metadata/body. — If this PR is intended to solve an issue, link it explicitly in the PR body.
📋 Copy for AI agents — paste into your coding agent
Fix the following blocker(s) from this PR review:

1. No linked issue detected: No closing reference or linked issue number was found in the PR metadata/body. — If this PR is intended to solve an issue, link it explicitly in the PR body.

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 ✅ No-issue rationale PR body explains why no issue is linked.
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 (no linked issue context).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 1037 registered-repo PR(s), 610 merged, 126 issue(s).
Contributor context ✅ Confirmed Gittensor contributor philluiz2323; Gittensor profile; 1037 PR(s), 126 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: moderate
Review context
  • Author: philluiz2323
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Python, JavaScript, MDX, TypeScript, CSS, Cuda, HTML, Kotlin
  • Official Gittensor activity: 1037 PR(s), 126 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.
🧪 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 &lt;question&gt; answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat &lt;question&gt; 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.

🟩 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

@loopover-orb

loopover-orb Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

LoopOver is closing this pull request on the maintainer's behalf (No linked issue detected). This is an automated maintenance action — to pursue this change, please open a new pull request with the issues resolved. Closed PRs may be analyzed later to improve review accuracy, but they are not automatically reopened or re-reviewed.

@loopover-orb loopover-orb Bot closed this Jul 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant