fix(engine)!: bound miner-goal-spec list scanning, remove the orphaned duplicate parser#4318
Merged
Merged
Conversation
…d duplicate parser 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.
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
gittensory-ui | a903dc6 | Commit Preview URL Branch Preview URL |
Jul 08 2026, 08:27 PM |
❌ 1 Tests Failed:
View the full list of 1 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Found while filing Wave 2 Phase 2 miner issues (2026-07-08):
packages/gittensory-engine/src/miner-goal-spec-parse.ts(110 lines) is a second, independent implementation ofparseMinerGoalSpecthat duplicates and had drifted from the canonical one inminer-goal-spec.ts(278 lines) -- different return types, different validation limits (100/256 vs 200/300).Git history explains why two exist: two independently-filed issues (#2652, a follow-up to #2293's type scaffold, and #2658/closes-#2301) both specified building this parser, landing around the same time.
miner-goal-spec.tsbecame canonical -- re-exported from the package barrel, and the only onepackages/gittensory-miner's real caller (opportunity-ranker.js:57) actually imports.miner-goal-spec-parse.ts's own PR description 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 (@jsonbored/gittensory-engine/miner-goal-spec-parse) with confirmed zero real callers anywhere in the repo, in-tree or via the subpath.More importantly, investigating the drift surfaced a real, live security gap: the canonical file has the exact resource-exhaustion bug that
#2896patched only in its orphaned sibling.normalizeStringList's cap check fired only after a candidate was accepted, so an array of entries that always take thecontinuepath (non-string, duplicate, or empty-after-trim) never hit the cap and got scanned in full -- unbounded CPU/memory work and unbounded warning growth on a hostile.gittensory-miner.yml, which this parser reads from untrusted per-repo config.Changes
normalizeStringList's iteration to the raw array index, checked before any per-entry work -- mirroring#2896's fix, but without reproducing its own latent bug: a trailingresult.length >= capbreak (kept there for "defense in depth") fires silently, with no warning, whenever the input has exactlycap+1unique 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, so the trailing check is removed rather than mirrored.miner-goal-spec-parse.tsand its dedicated test../miner-goal-spec-parsesubpath frompackage.json'sexportsmap.Breaking change
@jsonbored/gittensory-engine's published./miner-goal-spec-parsesubpath export is removed. The package is one week old (bootstrap-published 2026-07-08) with a single in-repo consumer that never used this subpath -- no known external impact.Testing
npm --workspace @jsonbored/gittensory-engine test-- 275/275 passing.miner-goal-spec-*,opportunity-ranker,miner-discovery-pipeline, metadata/ranked-opportunity suites) -- 164/164 passing.npm run typecheck-- clean.discoverMinerGoalSpecPath, untouched by this change).#2896added for the now-deleted sibling.