Skip to content

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

Closed
philluiz2323 wants to merge 2 commits into
JSONbored:mainfrom
philluiz2323:attempt-cli-feasibility-signals-8543
Closed

feat(miner): capture feasibility-verdict rule-fired signals in attempt-cli (#8543)#8565
philluiz2323 wants to merge 2 commits 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

Closes #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.
@superagent-security

Copy link
Copy Markdown
Contributor

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

@codecov

codecov Bot commented Jul 24, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 86.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.69%. Comparing base (0c19927) to head (953bbd3).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
packages/loopover-miner/lib/attempt-cli.ts 86.66% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8565      +/-   ##
==========================================
+ Coverage   89.58%   89.69%   +0.11%     
==========================================
  Files          97       98       +1     
  Lines       22706    22968     +262     
  Branches     3872     3956      +84     
==========================================
+ Hits        20341    20601     +260     
- Misses       2187     2188       +1     
- Partials      178      179       +1     
Flag Coverage Δ
backend 99.23% <86.66%> (?)

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

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

loopover-orb Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Caution

🛑 LoopOver review result - fixes required

Review updated: 2026-07-24 21:15:05 UTC

2 files · 1 AI reviewer · no blockers · CI failing · unstable

🛑 Suggested Action - Fix Blockers

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 with a new SignalStore seam, best-effort try/catch discipline, and no metadata. The implementation is placed correctly (right after the ready:false check, before the console output), and the four new tests directly verify the claimed behavior: exact ruleId/outcome/targetKey shape, zero calls on the ready:true path, and that both an init-throw and a per-write rejection never change exit code/output/JSON. The change is narrow, well-scoped to the linked issue (#8543), and touches only attempt-cli.ts and its test file as claimed.

Nits — 5 non-blocking
  • The codecov/patch check failed at 86.66% (target 99%) — likely the catch branches (init failure, write failure) or an uncovered arm somewhere in recordFeasibilityVerdictSignals; worth checking which specific line/branch is uncovered even though both failure-mode tests exist.
  • attempt-cli.ts is now ~571 lines per the external size-smell note; consider whether recordFeasibilityVerdictSignals and initDefaultSignalTrackingStore belong in a small dedicated module mirroring how discover-cli.ts's own signal-recording helper is organized, though this is a style call not required by this diff.
  • nowMs is threaded through as the signal-capture timestamp but is captured once at the top of runAttempt (options.nowMs ?? Date.now()) rather than at the moment the infeasible branch is reached — functionally fine here since it's injectable for tests, just worth confirming this matches discover-cli.ts's own timestamp semantics.
  • Check the codecov/patch report directly to see which line(s) in attempt-cli.ts are the uncovered 86.66%-vs-99% gap, since the four new tests look like they should cover both catch paths.
  • If SignalStore capture logic keeps growing (eligibility signals, feasibility signals, and potentially more gates later), consider extracting a shared recordRuleFiredSignals(reasons, outcome, targetKey, store) helper used by both discover-cli.ts and attempt-cli.ts to avoid the 'mirrored verbatim' duplication becoming a maintenance burden across two files.

CI checks failing

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

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ✅ Gate result — Passing (No configured blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #8543
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: 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
Linked issue satisfaction

Addressed
The diff wires capture into the existing `!codingTaskSpec.ready` branch via a new `initSignalTrackingStore` seam mirroring discover-cli's default and best-effort try/catch discipline, recording one RuleFiredEvent per avoid/raise reason with the exact ruleId/outcome/targetKey shape and no metadata, and tests cover both-reasons, ready:true zero-events, throwing-init, and rejecting-write cases with i

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: not available
  • 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)). 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: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.

miner: record feasibility-verdict reasons as rule-fired signals on attempt's infeasible path

2 participants