Skip to content

fix(engine): cap miner goal list inspection - #2896

Merged
loopover-orb[bot] merged 1 commit into
mainfrom
codex/fix-unbounded-scanning-in-miner-config-parser
Jul 4, 2026
Merged

fix(engine): cap miner goal list inspection#2896
loopover-orb[bot] merged 1 commit into
mainfrom
codex/fix-unbounded-scanning-in-miner-config-parser

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Motivation

  • The MinerGoalSpec string-list parser only enforced MAX_LIST_ENTRIES after accepting valid unique strings, allowing malicious or malformed arrays to be scanned unbounded and generate unbounded warnings and CPU/memory work.
  • The change bounds the amount of input inspected to prevent resource exhaustion while preserving existing normalization behavior.

Description

  • Stop iterating untrusted lists after MAX_LIST_ENTRIES inspected entries by switching to for (const [index, entry] of value.entries()) and breaking with a capped warning when the limit is reached in packages/gittensory-engine/src/miner-goal-spec-parse.ts.
  • Emit a clear cap warning when inspection stops early (MinerGoalSpec field "<field>" is capped at 200 entries; dropping the rest.).
  • Add regression tests exercising hostile arrays of null entries, duplicates, empty strings, and overlong strings to assert parsing and warning growth are bounded in packages/gittensory-engine/test/miner-goal-spec-parse.test.ts.
  • No behavioral changes to accepted normalized entries or exported API surface beyond hardening the input inspection bound.

Testing

  • git diff --check passed locally.
  • npm --workspace @jsonbored/gittensory-engine test ran the package suite and all tests passed (51 passing, 0 failing), including the new regression tests.
  • npm run test:ci could not be completed in this environment due to an external actionlint setup/download DNS failure and the local actionlint WASM fallback flagging a self-hosted runner label, so the full repo gate was not executed here.
  • npm audit --audit-level=moderate could not be completed due to the registry audit endpoint returning a 403 in this environment.

Codex Task

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jul 4, 2026

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
gittensory-ui 33b6b24 Commit Preview URL

Branch Preview URL
Jul 04 2026, 04:24 AM

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

loopover-orb Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Tip

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

✅ Gittensory review result - approve/merge recommended

Review updated: 2026-07-04 04:22:35 UTC

2 files · 1 AI reviewer · no blockers · readiness 93/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
The change correctly bounds `parseStringList` by inspected array position, so hostile arrays made of invalid, duplicate, empty, or overlong entries now stop after 200 inspected items instead of scanning the whole input. The normalized accepted values remain unchanged for the covered cases, and the new tests exercise the real parser path through `parseMinerGoalSpec`. The most notable gap is that the cap warning is only asserted on `wantedPaths`, but the shared helper covers the other list fields through the same call path.

Nits — 4 non-blocking
  • nit: `packages/gittensory-engine/test/miner-goal-spec-parse.test.ts:67` asserts only the warning count and final cap warning for invalid entries, so it would not catch accidental changes to the per-entry warning text or ordering before the cap.
  • nit: `packages/gittensory-engine/test/miner-goal-spec-parse.test.ts:79` covers the shared list parser only through `wantedPaths`; one representative assertion on another list field would make the call-site coverage clearer.
  • In `packages/gittensory-engine/test/miner-goal-spec-parse.test.ts:67`, assert the first invalid-entry warning as well as the final cap warning so the test pins both bounded growth and warning semantics.
  • In `packages/gittensory-engine/test/miner-goal-spec-parse.test.ts:79`, add a small `blockedLabels` or `preferredLabels` case to show all list fields use the same inspected-entry cap.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ⚠️ Missing No linked issue or no-issue rationale found.
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: 60 registered-repo PR(s), 51 merged, 436 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 60 PR(s), 436 issue(s).
Gate result ✅ Passing No configured blocker found.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository registration is not available in the local Gittensory cache.
  • Public profile languages: Python, TypeScript, JavaScript, Ruby, Go, Kotlin, MDX, Shell
  • Official Gittensor activity: 60 PR(s), 436 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Explain no-issue PR.
  • No action.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
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.

🟩 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
Contributor

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 2a0f36c into main Jul 4, 2026
12 checks passed
@loopover-orb
loopover-orb Bot deleted the codex/fix-unbounded-scanning-in-miner-config-parser branch July 4, 2026 04:25
JSONbored added a commit that referenced this pull request Jul 8, 2026
…d duplicate parser (#4318)

Two independently-filed issues (#2652's follow-up to #2293, and #2301)
both specified a MinerGoalSpec parser. Two implementations landed
around the same time: miner-goal-spec.ts became canonical (re-exported
from the package barrel, the only one packages/gittensory-miner's real
caller actually imports), while miner-goal-spec-parse.ts's own PR
explicitly avoided the barrel "while #2628 is open" as a temporary
conflict-avoidance measure that was never reconciled -- it sat
published as its own npm subpath export with zero real callers
anywhere in the repo.

More importantly: the canonical file has the exact resource-exhaustion
bug that #2896 patched in its orphaned sibling and nowhere else.
normalizeStringList's cap check only fired after a candidate was
accepted, so an array of entries that always take the `continue` path
(non-string, duplicate, or empty-after-trim) never hit the cap and got
scanned in full -- a hostile `.gittensory-miner.yml` with thousands of
duplicate/invalid list entries could force unbounded CPU/memory work
and unbounded warning growth on every repo a miner considers, since
this parser runs on untrusted per-repo config.

- Bound normalizeStringList's iteration to the raw array index, before
  any per-entry work, mirroring #2896's fix -- but WITHOUT that fix's
  own latent bug: a trailing `result.length >= cap` break (kept for
  "defense in depth") fires silently, with no warning, whenever the
  input has exactly cap+1 unique valid entries. A pre-existing test
  ("caps oversized string lists and ignores extra entries") caught
  this regression during review. The index-based check alone is both
  necessary and sufficient to bound work; the trailing check is
  removed rather than mirrored.
- Delete the orphaned miner-goal-spec-parse.ts and its dedicated test
  (confirmed zero real callers, in-repo or via its published subpath).
- Remove the ./miner-goal-spec-parse subpath from package.json's
  exports map. BREAKING CHANGE: @jsonbored/gittensory-engine's
  published subpath export is removed. The package is one week old
  with a single in-repo consumer that never used this subpath; no
  known external impact.
- Add hostile-input regression tests (all-non-string, all-duplicate,
  all-empty, all-overlong) to both the package-level and root-level
  test suites for the canonical parser, mirroring the coverage #2896
  added for its now-deleted sibling.
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