--- title: 'Address ████████████████ #162 review findings' type: task status: completed description: >- Bundled cleanup of code-quality, partial-migration, and entity-hygiene findings surfaced during post-merge review of mistakia/league#162. Scope: league-repo (CLI bug, missing is_main guard, dead exports, partial 14.5.7 migration, magic numbers in specs) and user-base entity hygiene (multi-line [worktree] observation, stale Last Review section on the #162 task entity). base_uri: user:task/league/address-external-leagues-162-review-findings.md created_at: '2026-05-08T00:25:35.551Z' entity_id: 0fbddc08-97d9-4889-9600-b3b0b2dfb765 finished_at: '2026-05-08T00:40:33.000Z' owner_identity_uri: █████████████████████████ priority: Medium public_read: true relations: - follows [[user:task/github/mistakia/league/162-import-and-sync-external-leagues.md]] tags: - user:tag/league-xo-football.md updated_at: '2026-05-08T00:40:33.000Z' visibility_analyzed_at: '2026-11-05T23:48:21.353Z' --- # Address ████████████████ #162 Review Findings ## Context Post-merge review of mistakia/league#162 (commit 399557d1, merged 2026-05-08T00:09Z) surfaced a coherent cluster of code-quality, partial-migration, and entity-hygiene findings. This task bundles those findings for execution. The original task (`user:task/github/mistakia/league/162-import-and-sync-external-leagues.md`) is closed and should not be reopened. Out of scope here: the `teams.activate.reserve` test failure also discovered during the review — handled separately in `[[user:task/league/investigate-teams-activate-reserve-test-failure.md]]`. ## Findings ### Critical (league-repo) - **CLI parsing bug** — `scripts/sync-external-league.mjs` rejects `--platform=value` syntax (only `--platform value` works). Standard yargs accepts both forms. Fix: use yargs' default parsing or accept both. - **Missing `is_main()` guard** — `scripts/generate-expected-outputs.mjs` calls `main()` unconditionally at module top-level (lines 250–253). Sibling scripts `update-fixtures.mjs:175` and `sync-external-league.mjs:181` use the canonical `if (is_main(import.meta.url)) { main() }` guard. Without it, importing this module triggers side-effectful file writes. ### Mid-severity (league-repo) - **Partial 14.5.7 migration** — `test/external-fantasy-leagues-sync-orchestrator.spec.mjs:25` still uses raw `fs.readFileSync` with manual `path.join(process.cwd(), ...)` for `player-mapper-test-data.json`, sitting alongside `load_platform_response` calls in the same file. Either route through the loader or document why this fixture is exempt. - **Dead export** — `test/utils/fixture-loader.mjs:99-101` exports `clear_fixture_cache` but no caller invokes it. Remove or document its purpose. - **Over-engineered helper** — `test/utils/fixture-loader.mjs:24-48` `find_fixture_file` does recursive `readdir` walk for a flat directory (`expected-outputs/` has 4 files, no sub-dirs). Simplify to a direct `path.join` matching `load_platform_response`'s shape. - **Magic numbers in spec assertions** — `test/external-fantasy-leagues-mappers-authentic.spec.mjs:79-84` and `test/external-fantasy-leagues-sleeper-integration.spec.mjs:104,143` hard-code `12` (teams), `296` (transactions), `33` (players) as numeric literals. The specs already load `sync_results_expected` — derive these counts from that object so a fixture refresh produces clear failures rather than opaque assertion mismatches. ### Entity hygiene (user-base) - **Multi-line `[worktree]` observation** — on `user:task/github/mistakia/league/162-import-and-sync-external-leagues.md`, the `[worktree]` observation spans 3 semantic clauses (creation, node_modules verify, readiness). Per `[[user:guideline/knowledge-base/write-observations.md]]` "one sentence per observation". Path also dangles (worktree was deleted as part of merge cleanup). Rewrite to a single sentence reflecting the post-merge state. - **Stale `## Last Review` section** — task body retains a `## Last Review` cache from 2026-05-07T20:55Z (pre-completion). On a Completed task, this documents obsolete planning context and may mislead future readers. Recommended: remove the section. ## Implementation Plan ### Overview Address the eight post-merge review findings for mistakia/league#162 in two distinct scopes: code/test fixes inside the league repo (under a worktree) and entity-hygiene edits to the closed #162 task entity (directly on user-base main). All work is mechanical cleanup; no behavior changes to production code paths. ### Background Findings were surfaced during post-merge review of commit `399557d1` and recorded verbatim in the `## Findings` section above. The league repo is at `/Users/trashman/user-base/repository/active/league`; the closed parent task entity is at `/Users/trashman/user-base/task/github/mistakia/league/162-import-and-sync-external-leagues.md`. Two issues are explicitly out of scope and tracked elsewhere: the `teams.activate.reserve` test failure and the TransactionMapper 0/296 add/drops bug. ### Design - **League-repo scope** lands on a single worktree branch `feat/162-review-findings-cleanup` under `repository/active/league-worktrees/`, then merges via `[[user:workflow/software/merge-worktree.md]]`. - **User-base scope** edits `task/github/mistakia/league/162-import-and-sync-external-leagues.md` directly on main (small entity-hygiene edits per CLAUDE.md), and is committed in its own commit so `cli/sync-all.sh` pushes it. - **Magic-number cleanup**: replace numeric literals (12, 296, 33) with reads off the already-loaded `sync_results_expected` object — specifically `sync_results_expected.canonical.{league.teams,rosters,transactions,players}.length` (and `league_config_expected.expected_output.league_format.num_teams` where `sync_results` is not loaded). Do NOT re-run the suite to discover counts; the source of truth is the loaded fixture object. - **Fixture-loader simplification**: collapse `find_fixture_file`'s recursive walk to a direct `path.join(expected_outputs_dir, ...)` matching `load_platform_response`'s shape — the directory is flat with four files. Drop the unused `clear_fixture_cache` export. - **CLI parsing bug**: extend the manual argv loop in `sync-external-league.mjs` to split tokens on the first `=` so `--platform=value` and `--platform value` both parse. Keep the existing manual loop (no yargs dependency added — the rest of the script is hand-rolled and matches the pattern). - **Sync-orchestrator spec migration**: route the `player-mapper-test-data.json` read through the fixture loader by adding a small helper (or extend the loader with a generic `load_fixture(relative_path)` accessor). Choice: extend the loader with a thin `load_test_fixture(relative_path)` to keep the spec free of `fs`/`path` imports — the existing loader already encapsulates `fixtures_root`. - **Knowledge Base Impact**: no domain entry doc, ABOUT.md, tag, or relation changes. The two entity-hygiene edits update an existing task entity in place; the league-repo edits don't introduce new modules or patterns. No new observations or guidelines warranted. ### Notes - All league-repo changes must keep the external-fantasy-leagues suite at 128 passing / 0 failing / 0 pending. Run `yarn test --reporter min test/external-fantasy-leagues-*.spec.mjs` after each finding. - ESLint must stay clean (`yarn lint`). - Do not touch `[bug]` observation on task #162 — it documents the TransactionMapper 0/296 known gap and stays put. - Out of scope and explicitly do-not-touch: `teams.activate.reserve` failure, TransactionMapper add/drops bug. - Magic-number replacements should keep the dual-assertion pattern ergonomic — where the spec already does both `should.have.length(12)` and `should.have.length(expected.teams.length)`, drop the literal and keep only the derived form. ### Tasks #### League-repo: Critical Fixes - [x] **Fix `--platform=value` CLI parsing** - File: `scripts/sync-external-league.mjs` - Purpose: Accept both `--flag value` and `--flag=value` forms (standard yargs behavior). - Changes: - In the argv loop (around lines 109-142), add a pre-step that splits each token on the first `=` when it begins with `--`, yielding `[name, value]`. If `value` is present, use it directly; otherwise consume the next token as today. - Preserve unknown-arg detection and exit code 1 behavior. - Smoke-test all three forms manually: `--help`, `--platform=sleeper --external-league-id=X --internal-league-id=Y --dry-run`, and the existing space-separated form. - [x] **Add `is_main()` guard to `generate-expected-outputs.mjs`** - File: `scripts/generate-expected-outputs.mjs` - Purpose: Match sibling scripts (`update-fixtures.mjs`, `sync-external-league.mjs`); prevent side-effectful file writes when imported. - Changes: - Add `import { is_main } from '#libs-server'` to the import block. - Wrap the bottom-of-file `main().catch(...)` invocation in `if (is_main(import.meta.url)) { ... }`. #### League-repo: Mid-severity Cleanup - [x] **Migrate sync-orchestrator spec off raw `fs.readFileSync`** - File: `test/external-fantasy-leagues-sync-orchestrator.spec.mjs` - Purpose: Complete the 14.5.7 fixture-loader migration; eliminate the only remaining direct fs read in the ████████████████ spec set. - Changes: - Add a generic `load_test_fixture(relative_path)` export to `test/utils/fixture-loader.mjs` that resolves `relative_path` under `fixtures_root` and returns parsed JSON (with module-level cache). - Replace the inline `fs.readFileSync(path.join(fixtures_path, 'player-mapper-test-data.json'), 'utf8')` block in the spec's `before` hook with `await load_test_fixture('player-mapper-test-data.json')`. - Remove now-unused `fs` and `path` imports from the spec. - [x] **Remove dead `clear_fixture_cache` export** - File: `test/utils/fixture-loader.mjs` - Purpose: No callers; dead surface area. - Changes: Delete the exported `clear_fixture_cache` function (lines 99-101). - [x] **Simplify `find_fixture_file` to direct `path.join`** - File: `test/utils/fixture-loader.mjs` - Purpose: Recursive walk is unjustified for a flat 4-file directory; matches `load_platform_response`'s shape. - Changes: - Delete the `find_fixture_file` helper (lines 24-48). - Inline `load_expected_output` to construct `file_path = path.join(fixtures_root, 'expected-outputs', `${test_scenario}.json`)` directly and call the existing `read_json` helper, mirroring `load_platform_response`. - Preserve `ENOENT` -> "Expected output fixture not found" error message. - [x] **Replace magic-number assertions with derived counts** - Files: - `test/external-fantasy-leagues-sleeper-integration.spec.mjs` - `test/external-fantasy-leagues-mappers-authentic.spec.mjs` - Purpose: Fixture-refresh failures should produce clear count-derived messages, not opaque literal mismatches. - Changes (sleeper-integration spec): - Drop the `result.teams.should.have.length(12)` literal at the team-count assertion (already paired with `expected.teams.length`). - Replace `result.settings.should.have.property('num_teams', 12)` with the value derived from `sync_results_expected.canonical.league.settings.num_teams` (or read off `expected` already in scope). - Drop the `result.should.have.length(12)` literal in the rosters block (already paired with `expected.length`). - Drop the `result.should.have.length(296)` literal in the transactions block (already paired with `expected.length`). - Drop the `result.should.have.length(33)` literal in the players block (already paired with `expected.length`). - Changes (mappers-authentic spec): - Replace `result.league_format.should.have.property('num_teams', 12)` with a comparison against `league_config_expected.expected_output.league_format.num_teams`. - Leave the other roster-shape assertions (`sqb`, `srb`, `swr`, `ste`, `srbwr`, `sqbrbwrte`, `sdst`, `sk`, `bench`) unchanged — they are explicit format-shape documentation, not "counts" in the finding's sense; keep them as the test docstring describes. #### League-repo: Verification - [x] **Suite + lint pass after each finding** - Run: `yarn test --reporter min test/external-fantasy-leagues-*.spec.mjs` (expect 128/0/0). - Run: `yarn lint` on touched files. - Smoke-test: `node scripts/sync-external-league.mjs --help`; `node scripts/sync-external-league.mjs --platform=sleeper` (verify it parses far enough to fail on missing required args, not on unknown `--platform=` token). - Smoke-test: import `scripts/generate-expected-outputs.mjs` from a node REPL or a one-liner — confirm no file writes occur. #### User-base entity hygiene - [x] **Rewrite multi-line `[worktree]` observation** - File: `/Users/trashman/user-base/task/github/mistakia/league/162-import-and-sync-external-leagues.md` - Purpose: Per `[[user:guideline/knowledge-base/write-observations.md]]`, one sentence per observation. Current observation spans 3 clauses and references a deleted worktree path. - Changes: Replace the `[worktree]` observation with a single-sentence post-merge form, e.g. `[worktree] 2026-05-07 Implementation worktree feat/162-external-leagues-test-migration was created, used through tasks 14.5.4-14.6/15, and removed during merge cleanup.` Use `base entity observe` (or direct frontmatter edit per `[[user:guideline/write/write-entity.md]]`). - [x] **Remove stale `## Last Review` section** - File: `/Users/trashman/user-base/task/github/mistakia/league/162-import-and-sync-external-leagues.md` - Purpose: Section reflects 2026-05-07T20:55Z pre-completion planning state on a now-Completed task; misleading to future readers. - Changes: Delete the entire `## Last Review` heading and its body (last section of the file). No replacement. #### Knowledge Base Updates - [ ] **None.** No domain entry doc, ABOUT.md, tag, or guideline edits needed for this cleanup. The user-base scope is itself entity hygiene on an existing entity. #### Commit & Merge - [x] **League-repo commit** - Commit on the worktree branch with a single commit covering all five league-repo findings (or split if review prefers per-finding commits — confirm with lead at the pre-commit gate). - Show `git diff --stat` before commit. - Run `[[user:workflow/software/merge-worktree.md]]`. - [ ] **User-base commit** - Single commit on `user-base` main covering the two entity-hygiene edits to task #162. - Show `git diff --stat` before commit. - Let `cli/sync-all.sh` push (do not run `git push` manually).