Skip to content

fix(engine): remove the dead AMS_POLICY_SPEC_FILENAMES discovery constant - #9004

Closed
shin-core wants to merge 1 commit into
JSONbored:mainfrom
shin-core:fix/drop-dead-ams-policy-filenames-8863-v2
Closed

fix(engine): remove the dead AMS_POLICY_SPEC_FILENAMES discovery constant#9004
shin-core wants to merge 1 commit into
JSONbored:mainfrom
shin-core:fix/drop-dead-ams-policy-filenames-8863-v2

Conversation

@shin-core

Copy link
Copy Markdown
Contributor

What & why

Closes #8863.

packages/loopover-engine/src/ams-policy-spec.ts exported AMS_POLICY_SPEC_FILENAMES, claiming a
documented 4-path first-match-wins discovery order "mirroring MINER_GOAL_SPEC_FILENAMES". But the
real consumer, packages/loopover-miner/lib/ams-policy.ts, resolves AMS policy from a single
operator-config-dir filename (.loopover-ams.yml), never imports the constant, and has no fallback
chain — so an operator using .github/ or .json variants silently got defaults. The constant was
dead and its comment misdescribed real behavior.

Choice (issue option a)

AMS policy is deliberately operator-local, not repo-discovered (the resolver reads only the
operator config dir), so I removed the misleading constant rather than wiring a repo-discovery chain
the resolver is not meant to have.

Change

  • Delete AMS_POLICY_SPEC_FILENAMES and its doc comment from ams-policy-spec.ts.
  • Drop the barrel re-export from packages/loopover-engine/src/index.ts.
  • Remove the two barrel tests' assertions of the literal array.

Confirmation

Grep-verified the constant had no other references anywhere in the tree (only its declaration,
the barrel export, and the two removed test assertions). ams-policy.ts is unchanged. The engine
typecheck and both barrel test suites pass with the constant removed.

…tant

AMS_POLICY_SPEC_FILENAMES claimed a documented 4-path first-match-wins discovery
order "mirroring MINER_GOAL_SPEC_FILENAMES", but the real consumer resolves AMS
policy from a single operator-config-dir filename and never imported the constant
-- so it was dead (referenced only by its own declaration, the barrel export, and
tests asserting the literal array) and its comment misdescribed the actual
behavior. AMS policy is deliberately operator-local, not repo-discovered, so the
misleading constant is removed rather than wiring a discovery chain that the
resolver is not meant to have.

Delete the constant and its doc comment, drop the barrel export, and remove the
two barrel tests' assertions of it. No other code referenced it.

Closes JSONbored#8863
@shin-core
shin-core requested a review from JSONbored as a code owner July 26, 2026 15:36
@superagent-security

Copy link
Copy Markdown
Contributor

🚨 Contributor flagged. Click here for more info: Superagent Dashboard

@superagent-security superagent-security Bot added the contributor:flagged Contributor flagged for review by trust analysis. label Jul 26, 2026
@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 26, 2026
@loopover-orb

loopover-orb Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Caution

🛑 LoopOver review result - fixes required

Review updated: 2026-07-26 15:57:52 UTC

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

🛑 Suggested Action - Fix Blockers

Review summary
This PR deletes the dead `AMS_POLICY_SPEC_FILENAMES` constant and its barrel re-export, along with the two test assertions that only verified the literal array value. The confirmed-real consumer, `packages/loopover-miner/lib/ams-policy.ts`, resolves policy from a single hardcoded `.loopover-ams.yml` path and never imported this constant, so removal is a clean, low-risk deletion of unused surface area with no behavior change. The PR is narrowly scoped, closes #8863 as required, and the remaining test assertions in both barrel test files still verify the real exported functions.

Nits — 4 non-blocking
  • The FAILED validate/validate-tests checks show no detail and this branch is 7 commits behind the default branch, so the failures are plausibly caused by unrelated changes that landed upstream after divergence rather than this diff itself — worth rebasing to confirm.
  • The Contributor trust flag is a process signal, not a code defect in this diff.
  • Rebase onto the current default branch to rule out the undetailed validate/validate-tests failures being caused by post-divergence changes rather than this PR.
  • Code changes lack test evidence — Add focused regression tests or explain why existing coverage is sufficient.

CI checks failing

  • validate
  • validate-tests

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 #8863
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: 39 registered-repo PR(s), 23 merged, 0 issue(s).
Contributor context ✅ Confirmed Gittensor contributor shin-core; Gittensor profile; 39 PR(s), 0 issue(s).
Improvement ℹ️ None detected risk: low · value: none · LLM: minor
Linked issue satisfaction

Addressed
The PR chooses option (a) from the issue, deleting AMS_POLICY_SPEC_FILENAMES and its misleading comment, removing the barrel re-export, and cleaning up the tests that asserted the literal array, while leaving ams-policy.ts unchanged; it also states the rationale (AMS policy is deliberately operator-local) as the issue requires when choosing (a).

Review context
  • Author: shin-core
  • 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: 39 PR(s), 0 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 <question> answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat <question> 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 added the manual-review Gittensor contributor context label Jul 26, 2026

@JSONbored JSONbored left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Failing CI, closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contributor:flagged Contributor flagged for review by trust analysis. gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. manual-review Gittensor contributor context

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(engine): AMS_POLICY_SPEC_FILENAMES documents a 4-path discovery order the real resolver never uses

2 participants