Skip to content

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

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)#8557
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)

@philluiz2323
philluiz2323 requested a review from JSONbored as a code owner July 24, 2026 20:37
@superagent-security

Copy link
Copy Markdown
Contributor

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

…t-cli (JSONbored#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.
@JSONbored
JSONbored force-pushed the attempt-cli-feasibility-signals-8543 branch from 9dad802 to 8fcbdd1 Compare July 24, 2026 20:38
@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 (8fcbdd1).

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    #8557      +/-   ##
==========================================
- Coverage   89.58%   88.56%   -1.03%     
==========================================
  Files          97       98       +1     
  Lines       22706    22968     +262     
  Branches     3872     3956      +84     
==========================================
  Hits        20341    20341              
- Misses       2187     2449     +262     
  Partials      178      178              
Flag Coverage Δ
backend 0.00% <0.00%> (?)

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%> (ø)

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 24, 2026
@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:43:00 UTC

2 files · 1 AI reviewer · 1 blocker · CI failing · unstable

🛑 Suggested Action - Reject/Close

Review summary
This PR wires a new recordFeasibilityVerdictSignals call into attempt-cli.ts's existing infeasible branch, capturing avoid/raise reasons as RuleFiredEvent signals mirroring discover-cli.ts's eligibility-exclusion pattern. The wiring is correct: store acquisition and each write are independently swallowed (init try/catch, per-write .catch), the call happens before the existing console/ledger side effects so exit code/output shape is provably unaffected, and tests directly verify the throwing-init and rejecting-write paths preserve exit code 4 and JSON output. The one gap is codecov/patch failing at 0.00% despite four new describe blocks exercising this exact code — worth checking whether coverage tooling is mis-attributing these lines before merge.

Nits — 5 non-blocking
  • The codecov/patch check reports 0.00% hit on this diff despite the four new tests in miner-attempt-cli.test.ts:1240-1390 directly exercising recordFeasibilityVerdictSignals's success, no-op, throw, and reject paths — worth confirming this is a tooling/path-mapping artifact rather than the tests silently not running against the changed lines.
  • attempt-cli.ts is now ~571 lines (nit per the size-smell brief) — not a blocker since this PR only adds ~40 lines to it, but a future refactor splitting signal-capture helpers into their own module (like signal-tracking-store.ts already is) would help.
  • The inline comment block above recordFeasibilityVerdictSignals (attempt-cli.ts:296-305) restates most of what the PR description already says; a one-line pointer to discover-cli.ts's recordEligibilityExclusionSignals would suffice.
  • Confirm with codecov why patch coverage reports 0% given the four targeted tests at test/unit/miner-attempt-cli.test.ts:1240-1390 — if it's a stale base-commit diff mapping, note that in the PR rather than leaving it as an open red check.
  • Consider whether recordFeasibilityVerdictSignals's occurredAt (attempt-cli.ts:311) should reuse the same `nowMs` used elsewhere in the function for other timestamps, to keep all this attempt's audit timestamps internally consistent — appears already done correctly here, just worth a one-line note in the PR body confirming it.

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.

CI checks failing

  • codecov/patch — 0.00% of diff hit (target 99.00%)

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 (CI is failing (codecov/patch); 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