Skip to content

Add transparent NextGen rarity analysis tooling - #4

Merged
punk6529 merged 11 commits into
mainfrom
codex/nextgen-rarity-tooling
Aug 1, 2026
Merged

Add transparent NextGen rarity analysis tooling#4
punk6529 merged 11 commits into
mainfrom
codex/nextgen-rarity-tooling

Conversation

@punk6529

@punk6529 punk6529 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add reusable Museum tooling under scripts/rarity/ for the pinned 6529 NextGen trait-prevalence algorithm
  • preserve raw input snapshots, provenance, normalization, missing/duplicate/orphan observations, score formulas, rank/tie behavior, and deterministic hashes
  • add exact input/output compatibility fixtures and focused tests under tests/rarity/
  • document that trait rarity is descriptive prevalence, not quality or curatorial significance, and keep OpenSea metric data outside the rarity evidence boundary
  • retain source conclusions and unresolved questions in the dated Museum WIP note and INDEX.md

Review follow-up

  • OpenSea-sourced or computed rarity metric fields are rejected when structured as metric keys/provider fields; provenance prose, citation labels, URLs, and descriptive keys such as opensea_trait_source_url remain admissible and are not score inputs.
  • Add the defensive empty-rank guard, Counter-based duplicate/frequency handling, and one-pass duplicate preservation bookkeeping.
  • Distinguish argparse invocation misuse (2) from rejected/invalid data (1) and success (0).
  • Add preserve-mode product/aggregate assertions, the zero-scored-trait Math.min(...[]) / math.inf parity assertion, and an explicit CPython/canonical-float determinism profile and boundary test.
  • Keep runtime metadata visible in output while excluding it from output_sha256, so exact fixtures are not pinned to one CPython patch version while numeric hash reproducibility remains explicitly bounded.

Foundation update

  • Updated the branch from origin/main at 72622a670854cc489330d930136bae7318044e41.
  • Preserved main’s governance evidence, corrected .github/6529bot.yml review policy, governance review policy, CI workflow, and orchestration ledger.
  • Preserved the complete production-archeology note at notes/research/nextgen-rarity-method.md unchanged from origin/main.
  • Retained the rarity tooling, tests, WIP analysis, and index entries; removed one duplicate roadmap index row introduced by earlier conflict resolution.

Source pins

  • 6529seize-backend nextgen_tokens.ts at 902557e9274f03b9851e97ef7ffac4b3c310b8a0
  • 6529seize-backend nextgen_constants.ts at the same commit
  • nextgen contract repository at 73c09d1c07e405ddb9ccdd462283ab98ea68f903

Validation

  • python scripts/bootstrap_validate.py — passed; 11 JSON files checked
  • python -m unittest discover -s tests\\rarity -p 'test_*.py' -v — 11 passed
  • python -m py_compile ... — passed
  • codex-diff-check --cached and codex-diff-check origin/main...HEAD — passed
  • main-owned policy/config/evidence files match origin/main
  • automatic baseline/follow-up review policy from main is in force; no manual specialist review command was issued

Summary by CodeRabbit

  • New Features

    • Added a reproducible NextGen rarity-analysis tool for dated JSON snapshots.
    • Added configurable duplicate handling, data-quality reporting, rarity scores, rankings, provenance details, and reproducibility hashes.
    • Added a command-line interface supporting standard output or file export with clear error reporting.
  • Documentation

    • Documented compatibility rules, formulas, input requirements, determinism, and known open questions.
  • Tests

    • Added comprehensive fixtures and automated coverage for validation, normalization, calculations, CLI behavior, and deterministic results.

@coderabbitai

coderabbitai Bot commented Aug 1, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

This change adds documented NextGen rarity-analysis rules, a compatibility analyzer for dated JSON snapshots, a CLI, deterministic hashing and ranking, validation policies, synthetic fixtures, and comprehensive unittest coverage.

Changes

NextGen rarity analysis

Layer / File(s) Summary
Analysis contract and package surface
INDEX.md, docs/generative-trait-analysis.md, notes/wip/..., scripts/rarity/__init__.py, scripts/rarity/nextgen_compat.py
Documents pinned sources, snapshot rules, scoring formulas, ranking behavior, hashing, CLI semantics, and public analysis exports.
Snapshot validation and normalization
scripts/rarity/nextgen_compat.py
Validates metadata, provenance, collection consistency, finite values, prohibited metrics, duplicate policies, ordering, orphan rows, and missing traits.
Rarity computation and CLI output
scripts/rarity/nextgen_compat.py, scripts/rarity/analyze.py
Computes trait and token metrics, ranks, provenance metadata, reproducibility hashes, and JSON output with standardized error handling.
Compatibility fixtures and test coverage
tests/rarity/*, tests/__init__.py
Adds input and expected-output fixtures plus tests for calculations, validation, duplicate handling, CLI failures, floating-point behavior, and hash stability.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Operator
  participant CLI
  participant Analyzer
  participant Snapshot
  participant Output
  Operator->>CLI: Submit snapshot path and duplicate policy
  CLI->>Snapshot: Load and validate JSON
  CLI->>Analyzer: Analyze normalized snapshot
  Analyzer->>Analyzer: Calculate scores, ranks, and hashes
  Analyzer-->>CLI: Return analysis result
  CLI->>Output: Write JSON to stdout or file
Loading

Possibly related PRs

Suggested reviewers: gelatogenesis, prxt6529, ragnep, simo6529

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 22.73% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: adding transparent NextGen rarity analysis tooling.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/nextgen-rarity-tooling

Comment @coderabbitai help to get the list of available commands.

@6529bot

6529bot Bot commented Aug 1, 2026

Copy link
Copy Markdown

6529bot WCAG 2.2 AA analysis - ec3001c

Verdict: No WCAG findings

This PR adds documentation (Markdown), a Python CLI/module, JSON test fixtures, and unit tests. There is no user-facing UI code (no HTML, JSX/TSX, components, templates, or styling) in the changed files, so WCAG 2.2 AA success criteria do not apply here.

No accessibility regressions or usability barriers are in scope for this change.

@6529bot

6529bot Bot commented Aug 1, 2026

Copy link
Copy Markdown

6529bot crypto security analysis - ec3001c

Verdict: No security findings

This PR adds a self-contained, offline Python analysis tool with no wallet, auth, signature, token, or web3 behavior. I reviewed against the crypto/security focus areas and found nothing exploitable in the changed code.

Notes supporting the verdict:

  • nextgen_compat.py never fetches remote data, opens sockets, or executes untrusted input. load_snapshot reads a local file and rejects non-object roots; canonical_json uses allow_nan=False, so non-finite floats are rejected rather than serialized.
  • No eval/exec/subprocess/pickle/deserialization sinks; json.load is safe against code execution. The deepcopy and dict/list recursion operate on already-shape-validated JSON.
  • analyze.py writes only to a caller-supplied --output path (mkdir(parents=True) then write_text); no shell invocation and no path traversal beyond what the operator explicitly requests. No secrets are read or logged.
  • Recursion in _reject_opensea_references and the canonicalizer is bounded by the parsed JSON depth; a hostile deeply-nested snapshot could in theory hit Python's recursion limit, but this is a local CLI on operator-supplied files — not a realistic exploit path and not a security concern for this tool. No action required.

One correctness caveat worth flagging for the author (not a security issue): the pinned hashes in nextgen-compatibility.expected.json and output_sha256 depend on exact float repr output and dict ordering. This is Python-implementation-dependent and could drift across interpreter versions, which would break the "identical hashes on repeat runs" guarantee across environments. Consider documenting the tested CPython version, but this does not affect the security verdict.

@6529bot

6529bot Bot commented Aug 1, 2026

Copy link
Copy Markdown

6529bot i18n analysis - ec3001c

Verdict: No i18n findings

This PR adds developer/operator-facing artifacts only: Markdown docs and research notes, a Python CLI (analyze.py) and module (nextgen_compat.py), JSON test fixtures, and unit tests. None of these are 6529 frontend React UI, and there is no i18n/messages path in scope.

  • The user-facing strings that exist (CLI argparse help text, InputError messages) belong to a Python operator CLI, which is outside the frontend progressive-i18n standard. No t(locale, key, params) path applies.
  • No React components, JSX/TSX, aria-label/alt/screen-reader text, or locale-sensitive UI formatting were touched, so the visible/accessible-name and i18n/format.ts helper requirements do not apply here.
  • Numeric formatting in nextgen_compat.py (raw float scores, sha256_json) is intentionally locale-independent canonical output for deterministic hashing; using locale-aware helpers there would be incorrect, so no toLocaleString/ad-hoc-formatting finding is warranted.
  • No translatable copy is embedded in images, and no user-generated content is being translated.

No fallback-debt note is required because no frontend surface is touched.

@6529bot

6529bot Bot commented Aug 1, 2026

Copy link
Copy Markdown

6529bot general PR review - ec3001c

Verdict: Needs changes

Important

  • scripts/rarity/nextgen_compat.py:296 (_dense_trait_ranks) crashes on an empty per-category list: previous_value = sorted_rows[0][key] raises IndexError when sorted_rows is empty. It's safe in normal flow because by_trait is only populated from existing rows, but in deduplicate/preserve forensic modes with a category whose only rows were dropped this is still reachable only if a category key exists with zero rows — currently not possible, so low priority. However, the bigger real gap: _dense_trait_ranks is called over per_trait which includes Mint Type rows with -1 scores, mixing them into the same category grouping is fine, but the function assumes sorted_rows non-empty for every group. Add an explicit empty guard to be defensive against future callers.

  • scripts/rarity/nextgen_compat.py:186-199 The OpenSea prohibition is a substring match on "opensea" anywhere in any key or string value. This will reject legitimate provenance snapshots that merely mention OpenSea in prose (e.g. source.note: "migrated off OpenSea in 2024") or a URL in a citation field. Given snapshots are "kept with the analysis" as evidence, a blanket substring reject on descriptive text is likely to block valid forensic records. Consider restricting the check to known metric fields/keys rather than any free-text value, or document that free-text mentions are intentionally rejected.

  • scripts/rarity/analyze.py:47-50 parser.error(str(error)) calls sys.exit(2) internally, so analyze_snapshot failures are surfaced as usage-style errors. This conflates input/data errors with CLI misuse and always exits 2. For a data-integrity tool, distinct exit codes (e.g. 2 for argparse misuse vs. non-2 for a rejected/invalid snapshot) would let callers/scripts distinguish "bad invocation" from "bad data." Minor but worth considering for automation.

Nice-to-have

  • scripts/rarity/nextgen_compat.py:274-285 The deduplicated_rows comprehension recomputes sorted(rows, key=...) twice per row inside the inner if row is not sorted(...)[0] and again in the outer key — O(n·k log k) redundant sorts. Correctness is fine; readability/perf would improve by computing the kept row once.

  • scripts/rarity/nextgen_compat.py:219 token_ids.count(token_id) inside a set comprehension over token_ids is O(n²). Fine for small snapshots, but for production-scale token universes (thousands of tokens) this and the similar patterns will be slow. Use a Counter.

  • tests/rarity/test_nextgen_compat.py: no test covers the preserve-mode statistical/product aggregates or the single_trait_rarity_score_trait_count_normalised empty-list (Math.min(...[])) branch for a token with zero non-Mint Type traits. The sparse-token test (id 5) exercises rarity_score == 0 but does not assert the single_trait_rarity_score_trait_count_normalised value that depends on the math.inf fallback. Adding an assertion there would lock in the intended JS-parity behavior described in the docs.

Suggested next steps

  • Narrow the OpenSea reject to structured metric keys, or explicitly document the free-text reject as intended.
  • Add an empty-group guard in _dense_trait_ranks.
  • Add a test asserting the Math.min(...[])/math.inf fallback output for a token with no scored traits.

Note: I did not independently verify the numeric fixture values against the upstream nextgen_tokens.ts source (not included here); the formulas and expected JSON are internally consistent, but exact backend parity depends on the pinned commit's implementation, which is outside the diff.

Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate.

Review comments:
From 6529bot general PR review on 6529-Collections/6529networkmuseum#4 (ec3001ce2cab):
**Verdict**: Needs changes

### Important

- `scripts/rarity/nextgen_compat.py:296` (`_dense_trait_ranks`) crashes on an empty per-category list: `previous_value = sorted_rows[0][key]` raises `IndexError` when `sorted_rows` is empty. It's safe in normal flow because `by_trait` is only populated from existing rows, but in `deduplicate`/`preserve` forensic modes with a category whose only rows were dropped this is still reachable only if a category key exists with zero rows — currently not possible, so low priority. However, the bigger real gap: `_dense_trait_ranks` is called over `per_trait` which includes `Mint Type` rows with `-1` scores, mixing them into the same category grouping is fine, but the function assumes `sorted_rows` non-empty for every group. Add an explicit empty guard to be defensive against future callers.

- `scripts/rarity/nextgen_compat.py:186-199` The OpenSea prohibition is a substring match on `"opensea"` anywhere in any key or string value. This will reject legitimate provenance snapshots that merely mention OpenSea in prose (e.g. `source.note: "migrated off OpenSea in 2024"`) or a URL in a citation field. Given snapshots are "kept with the analysis" as evidence, a blanket substring reject on descriptive text is likely to block valid forensic records. Consider restricting the check to known metric fields/keys rather than any free-text value, or document that free-text mentions are intentionally rejected.

- `scripts/rarity/analyze.py:47-50` `parser.error(str(error))` calls `sys.exit(2)` internally, so `analyze_snapshot` failures are surfaced as usage-style errors. This conflates input/data errors with CLI misuse and always exits 2. For a data-integrity tool, distinct exit codes (e.g. 2 for argparse misuse vs. non-2 for a rejected/invalid snapshot) would let callers/scripts distinguish "bad invocation" from "bad data." Minor but worth considering for automation.

### Nice-to-have

- `scripts/rarity/nextgen_compat.py:274-285` The `deduplicated_rows` comprehension recomputes `sorted(rows, key=...)` twice per row inside the inner `if row is not sorted(...)[0]` and again in the outer key — O(n·k log k) redundant sorts. Correctness is fine; readability/perf would improve by computing the kept row once.

- `scripts/rarity/nextgen_compat.py:219` `token_ids.count(token_id)` inside a set comprehension over `token_ids` is O(n²). Fine for small snapshots, but for production-scale token universes (thousands of tokens) this and the similar patterns will be slow. Use a `Counter`.

- `tests/rarity/test_nextgen_compat.py`: no test covers the `preserve`-mode statistical/product aggregates or the `single_trait_rarity_score_trait_count_normalised` empty-list (`Math.min(...[])`) branch for a token with zero non-`Mint Type` traits. The sparse-token test (id 5) exercises `rarity_score == 0` but does not assert the `single_trait_rarity_score_trait_count_normalised` value that depends on the `math.inf` fallback. Adding an assertion there would lock in the intended JS-parity behavior described in the docs.

**Suggested next steps**
- Narrow the OpenSea reject to structured metric keys, or explicitly document the free-text reject as intended.
- Add an empty-group guard in `_dense_trait_ranks`.
- Add a test asserting the `Math.min(...[])`/`math.inf` fallback output for a token with no scored traits.

Note: I did not independently verify the numeric fixture values against the upstream `nextgen_tokens.ts` source (not included here); the formulas and expected JSON are internally consistent, but exact backend parity depends on the pinned commit's implementation, which is outside the diff.

@punk6529

punk6529 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

/6529bot followup

@6529bot

6529bot Bot commented Aug 1, 2026

Copy link
Copy Markdown

6529bot follow-up commit review - b17d3b0

Verdict: Needs changes

Important

  • scripts/rarity/nextgen_compat.py:22-27 (determinism_profile) The expected fixture pins "python_version": "3.12.10", and test_canonical_float_boundary_is_explicit asserts profile["python_version"] == platform.python_version(). But test_full_output_matches_exact_compatibility_fixture compares the entire analyze_snapshot output — including determinism.python_version — against the fixture with assertEqual. Any CI or contributor machine not running CPython 3.12.10 exactly will fail the full-output test, even though the intent is only to bound float encoding. This makes the suite non-portable across patch versions (3.12.9, 3.12.11, 3.13, etc.). Either exclude the determinism block from the exact-fixture comparison, or pin only the float-encoding behavior without embedding the runtime version in the hashed output. As written, the output_sha256 also depends on the exact version string, so the hash guarantee breaks on any patch-level bump — the opposite of the stated determinism goal.

Nice-to-have

  • scripts/rarity/nextgen_compat.py:160-215 The OpenSea guard narrowing is a clear improvement, but the metric-key list (_is_opensea_metric_key via OPEN_SEA_METRIC_TERMS) includes "trait", so a benign structured key like opensea_trait_source_url (a citation URL stored under a descriptive key) would be rejected as a metric field, contradicting the documented "citation URLs remain admissible" promise at docs lines 59-63. Only the value path allows OpenSea URLs; an OpenSea-mentioning key containing trait is rejected regardless of value. Consider dropping trait from the OpenSea metric-key term list, since it's the least specific.

Resolved since last review

  • _dense_trait_ranks empty-group IndexError is fixed with an explicit if not sorted_rows: continue guard (line 474) and covered by test_empty_trait_rank_group_is_defensive.
  • OpenSea substring over-rejection is addressed: free-text prose/citations are now admissible while structured metric fields are rejected, with test_opensea_metric_fields_are_prohibited_but_prose_is_allowed covering both paths.
  • CLI exit-code conflation is fixed: data errors now return 1 and argparse misuse stays 2, verified by test_cli_distinguishes_bad_data_from_bad_invocation.
  • The O(n²) token_ids.count(...) and redundant double-sort in the deduplicate branch are replaced with Counter and a single min(...)/kept-row pass.
  • The Math.min(...[])/math.inf fallback and preserve-mode products now have explicit test assertions.

Suggested next steps

  • Decouple the exact-fixture/output_sha256 comparison from the embedded CPython patch version so the suite passes across 3.12.x/3.13 while keeping the documented determinism boundary.
  • Drop trait from the OpenSea metric-key term list to honor the admissible-citation-URL promise.
Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate.

Review comments:
From 6529bot follow-up commit review on 6529-Collections/6529networkmuseum#4 (b17d3b04d590):
**Verdict**: Needs changes

### Important

- `scripts/rarity/nextgen_compat.py:22-27` (`determinism_profile`) The expected fixture pins `"python_version": "3.12.10"`, and `test_canonical_float_boundary_is_explicit` asserts `profile["python_version"] == platform.python_version()`. But `test_full_output_matches_exact_compatibility_fixture` compares the entire `analyze_snapshot` output — including `determinism.python_version` — against the fixture with `assertEqual`. Any CI or contributor machine not running CPython 3.12.10 exactly will fail the full-output test, even though the intent is only to bound float encoding. This makes the suite non-portable across patch versions (3.12.9, 3.12.11, 3.13, etc.). Either exclude the `determinism` block from the exact-fixture comparison, or pin only the float-encoding behavior without embedding the runtime version in the hashed output. As written, the `output_sha256` also depends on the exact version string, so the hash guarantee breaks on any patch-level bump — the opposite of the stated determinism goal.

### Nice-to-have

- `scripts/rarity/nextgen_compat.py:160-215` The OpenSea guard narrowing is a clear improvement, but the metric-key list (`_is_opensea_metric_key` via `OPEN_SEA_METRIC_TERMS`) includes `"trait"`, so a benign structured key like `opensea_trait_source_url` (a citation URL stored under a descriptive key) would be rejected as a metric field, contradicting the documented "citation URLs remain admissible" promise at docs lines 59-63. Only the *value* path allows OpenSea URLs; an OpenSea-mentioning key containing `trait` is rejected regardless of value. Consider dropping `trait` from the OpenSea metric-key term list, since it's the least specific.

### Resolved since last review

- `_dense_trait_ranks` empty-group `IndexError` is fixed with an explicit `if not sorted_rows: continue` guard (line 474) and covered by `test_empty_trait_rank_group_is_defensive`.
- OpenSea substring over-rejection is addressed: free-text prose/citations are now admissible while structured metric fields are rejected, with `test_opensea_metric_fields_are_prohibited_but_prose_is_allowed` covering both paths.
- CLI exit-code conflation is fixed: data errors now return `1` and argparse misuse stays `2`, verified by `test_cli_distinguishes_bad_data_from_bad_invocation`.
- The O(n²) `token_ids.count(...)` and redundant double-sort in the deduplicate branch are replaced with `Counter` and a single `min(...)`/kept-row pass.
- The `Math.min(...[])`/`math.inf` fallback and preserve-mode products now have explicit test assertions.

**Suggested next steps**
- Decouple the exact-fixture/`output_sha256` comparison from the embedded CPython patch version so the suite passes across 3.12.x/3.13 while keeping the documented determinism boundary.
- Drop `trait` from the OpenSea metric-key term list to honor the admissible-citation-URL promise.

@6529bot

6529bot Bot commented Aug 1, 2026

Copy link
Copy Markdown

6529bot follow-up commit review - b17d3b0

Verdict: Needs changes

Important

  • tests/rarity/test_nextgen_compat.py:170-181 (test_full_output_matches_exact_compatibility_fixture via the added determinism block) The prior follow-up finding about the embedded CPython patch version was not resolved — it was made worse. The fixture at tests/rarity/fixtures/nextgen-compatibility.expected.json:24 still pins "python_version": "3.12.10", and test_full_output_matches_exact_compatibility_fixture (line ~60) still compares the entire analyze_snapshot output (now including determinism.python_version) against that fixture with assertEqual. Any contributor/CI on a different CPython patch (3.12.9, 3.12.11, 3.13, PyPy) fails the full-output test, and output_sha256 at line 669 also changes because determinism is inside the hashed payload. The new docs/notes acknowledge the boundary but the test suite still enforces an exact-version match on every machine. Either exclude the determinism block from the exact-fixture comparison and the output_sha256 commitment, or drop the runtime version from the hashed output. As written this is a portability regression that will red-CI most non-maintainer environments.

Nice-to-have

  • scripts/rarity/nextgen_compat.py:159-215 The trait term was retained in OPEN_SEA_METRIC_TERMS, so _is_opensea_metric_key still rejects a benign citation key like opensea_trait_source_url regardless of value, contradicting the docs promise at docs/generative-trait-analysis.md:59-63 that citation URLs/labels mentioning OpenSea remain admissible. The dedicated RARITY_METRIC_TERMS/provider path (lines 174-181) already covers the real metric case; trait is the least specific term and the most likely to collide with citation labels. Consider dropping it from OPEN_SEA_METRIC_TERMS.

Resolved since last review

  • The O(n²) token_ids.count(...) and the redundant double-sort in the deduplicate branch are now replaced with Counter and a single min(...)/kept-row pass (nextgen_compat.py:322, 357-379), and the trait_count_frequencies loop uses Counter (nextgen_compat.py:582).
  • CLI exit-code separation is preserved and now documented (analyze.py:16,48-50; docs 185-190), with test_cli_distinguishes_bad_data_from_bad_invocation covering both the 1 (bad data) and 2 (argparse) paths.
  • The empty-group _dense_trait_ranks guard and Math.min(...[])/preserve-mode assertions from the earlier review remain in place and are exercised by the new tests.

Suggested next steps

  • Decouple the determinism block (and therefore output_sha256) from the exact-fixture comparison so the suite passes across CPython patch levels and other interpreters, while keeping the documented boundary.
  • Drop trait from OPEN_SEA_METRIC_TERMS to honor the admissible-citation-URL promise.

Note: I did not independently verify the pinned float/hash fixture values against upstream nextgen_tokens.ts (not in scope); parity claims depend on the pinned backend commit.

Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate.

Review comments:
From 6529bot follow-up commit review on 6529-Collections/6529networkmuseum#4 (b17d3b04d590):
**Verdict**: Needs changes

### Important

- `tests/rarity/test_nextgen_compat.py:170-181` (`test_full_output_matches_exact_compatibility_fixture` via the added `determinism` block) The prior follow-up finding about the embedded CPython patch version was *not* resolved — it was made worse. The fixture at `tests/rarity/fixtures/nextgen-compatibility.expected.json:24` still pins `"python_version": "3.12.10"`, and `test_full_output_matches_exact_compatibility_fixture` (line ~60) still compares the entire `analyze_snapshot` output (now including `determinism.python_version`) against that fixture with `assertEqual`. Any contributor/CI on a different CPython patch (3.12.9, 3.12.11, 3.13, PyPy) fails the full-output test, and `output_sha256` at line 669 also changes because `determinism` is inside the hashed payload. The new docs/notes acknowledge the boundary but the test suite still enforces an exact-version match on every machine. Either exclude the `determinism` block from the exact-fixture comparison and the `output_sha256` commitment, or drop the runtime version from the hashed output. As written this is a portability regression that will red-CI most non-maintainer environments.

### Nice-to-have

- `scripts/rarity/nextgen_compat.py:159-215` The `trait` term was retained in `OPEN_SEA_METRIC_TERMS`, so `_is_opensea_metric_key` still rejects a benign citation key like `opensea_trait_source_url` regardless of value, contradicting the docs promise at `docs/generative-trait-analysis.md:59-63` that citation URLs/labels mentioning OpenSea remain admissible. The dedicated `RARITY_METRIC_TERMS`/provider path (lines 174-181) already covers the real metric case; `trait` is the least specific term and the most likely to collide with citation labels. Consider dropping it from `OPEN_SEA_METRIC_TERMS`.

### Resolved since last review

- The O(n²) `token_ids.count(...)` and the redundant double-sort in the deduplicate branch are now replaced with `Counter` and a single `min(...)`/kept-row pass (`nextgen_compat.py:322`, `357-379`), and the `trait_count_frequencies` loop uses `Counter` (`nextgen_compat.py:582`).
- CLI exit-code separation is preserved and now documented (`analyze.py:16,48-50`; docs `185-190`), with `test_cli_distinguishes_bad_data_from_bad_invocation` covering both the `1` (bad data) and `2` (argparse) paths.
- The empty-group `_dense_trait_ranks` guard and `Math.min(...[])`/preserve-mode assertions from the earlier review remain in place and are exercised by the new tests.

**Suggested next steps**
- Decouple the `determinism` block (and therefore `output_sha256`) from the exact-fixture comparison so the suite passes across CPython patch levels and other interpreters, while keeping the documented boundary.
- Drop `trait` from `OPEN_SEA_METRIC_TERMS` to honor the admissible-citation-URL promise.

Note: I did not independently verify the pinned float/hash fixture values against upstream `nextgen_tokens.ts` (not in scope); parity claims depend on the pinned backend commit.

@punk6529
punk6529 marked this pull request as ready for review August 1, 2026 15:47
@punk6529
punk6529 requested a review from a team August 1, 2026 15:47
@punk6529

punk6529 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@punk6529

punk6529 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

/6529bot followup

@coderabbitai

coderabbitai Bot commented Aug 1, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@punk6529

punk6529 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

/6529bot review general security media-external privacy-evidence glm-swarm

@punk6529

punk6529 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

/6529bot review general security media-external privacy-evidence glm-swarm

@punk6529

punk6529 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 1, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

punk6529 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 1, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

punk6529 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

/6529bot followup

punk6529 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Exact-head re-review request for independent reviewer task 019fbe15-b70a-7a43-9bff-09b87a3ae30b: please review commit c97bf7a45395bc470f65e73d6858f86d25e3402b against base 6ab83b456f1ad8d1b7b88b79cc960954feb56432. The six requested rarity remediations are implemented and locally validated; PR #4 remains open and must not be merged by this task.

punk6529 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Exact-head re-review for independent reviewer task 019fbe15-b70a-7a43-9bff-09b87a3ae30b: review commit 8423ca979cf9c9e150a370c401b02076b7cffa0b against base 6ab83b456f1ad8d1b7b88b79cc960954feb56432. This narrowly closes provider/issuer/source/origin identity bypasses, propagates nested context, and permits citation-only rarity_provenance; all prior fixes remain intact. PR #4 stays open and must not be merged by this task.

@punk6529

punk6529 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

/6529bot followup

punk6529 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Exact-head re-review for independent reviewer task 019fbe15-b70a-7a43-9bff-09b87a3ae30b: review commit becbc1fc1d897f046b2790e5e00684d4db10d7a6 against base 6ab83b456f1ad8d1b7b88b79cc960954feb56432. The input boundary now rejects all structured precomputed rarity/score/rank/metric fields recursively, including claimed internal providers and wrapped provider URL/metadata values; citation-only provenance remains allowed. PR #4 stays open and unmerged.

@punk6529

punk6529 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

/6529bot followup

punk6529 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Exact-head re-review request for independent reviewer task 019fbe15-b70a-7a43-9bff-09b87a3ae30b: please review commit 653c384eaf151ac9c58f1e383da9bb991863289c against base 6ab83b456f1ad8d1b7b88b79cc960954feb56432. The input boundary now normalizes every mapping key and rejects semantic rarity/score/rank/metric/statistical keys unless the full normalized key is in the explicit citation-only allowlist; citation containers are closed-schema and recursively validated, including nested provider/URL/metadata wrappers and all four reported rarity-field cases. Raw artist trait keys/values are not scanned. PR #4 remains open and unmerged.

@punk6529

punk6529 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

/6529bot followup

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (3)
scripts/rarity/__init__.py (1)

3-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Export InputError from the package surface.

The package exports the four analysis functions but not the exception they raise. A consumer that imports only scripts.rarity cannot catch InputError without reaching into scripts.rarity.nextgen_compat. Add it to the public surface.

♻️ Proposed export addition
 from .nextgen_compat import (
+    InputError,
     analyze_snapshot,
     canonical_json,
     load_snapshot,
     normalize_snapshot,
 )
 
 __all__ = [
+    "InputError",
     "analyze_snapshot",
     "canonical_json",
     "load_snapshot",
     "normalize_snapshot",
 ]
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/rarity/__init__.py` around lines 3 - 15, Export InputError from
scripts.rarity by importing it alongside the existing symbols from
nextgen_compat and adding "InputError" to __all__, so consumers can catch the
package’s public exception directly.
tests/rarity/test_nextgen_compat.py (1)

407-419: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a CLI success-path test.

The CLI tests cover exit code 1 for bad data and exit code 2 for bad invocation. No test covers the documented exit code 0 with a written output file. Add a case that runs main([str(FIXTURE), "--output", str(path)]), asserts the return value is 0, and asserts the written JSON parses and matches analyze_snapshot.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/rarity/test_nextgen_compat.py` around lines 407 - 419, Add a
success-path test alongside test_cli_distinguishes_bad_data_from_bad_invocation
that invokes main with FIXTURE and an --output temporary path, verifies it
returns 0, then reads and parses the output JSON and compares it with
analyze_snapshot’s result for the fixture.
scripts/rarity/nextgen_compat.py (1)

147-159: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Consider rejecting duplicate JSON object keys in load_snapshot.

json.load keeps the last value for a repeated object key and drops the earlier one silently. input_snapshot_sha256 then commits to the parsed object, not to what the file actually contained, while the documentation calls it "the canonical hash of the raw supplied snapshot". An object_pairs_hook closes that gap.

♻️ Proposed duplicate-key guard
     def reject_json_constant(value: str) -> None:
         raise InputError(
             f"non-finite numeric value is prohibited: JSON constant {value}"
         )
 
+    def reject_duplicate_keys(pairs: list[tuple[str, Any]]) -> dict[str, Any]:
+        seen: set[str] = set()
+        for key, _ in pairs:
+            if key in seen:
+                raise InputError(f"duplicate JSON object key is prohibited: {key}")
+            seen.add(key)
+        return dict(pairs)
+
     with Path(path).open("r", encoding="utf-8") as handle:
-        value = json.load(handle, parse_constant=reject_json_constant)
+        value = json.load(
+            handle,
+            parse_constant=reject_json_constant,
+            object_pairs_hook=reject_duplicate_keys,
+        )
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/rarity/nextgen_compat.py` around lines 147 - 159, Update
load_snapshot to pass an object_pairs_hook that detects repeated keys while
parsing and raises InputError identifying the duplicate key, while preserving
the existing UTF-8 loading, non-finite-number rejection, and root-object
validation behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@scripts/rarity/nextgen_compat.py`:
- Around line 181-189: Bound recursion in _reject_non_finite_values,
_reject_precomputed_metric_fields, and _validate_citation_value with a shared
depth parameter and explicit maximum nesting limit; increment it for nested
dict/list traversal and raise InputError when exceeded, so deeply nested
snapshots produce the CLI’s concise error instead of RecursionError. Preserve
existing validation behavior for inputs within the limit.

In `@tests/rarity/test_nextgen_compat.py`:
- Around line 285-294: Remove the version-dependent sum(values) equality
assertion from test_left_fold_matches_javascript_reduce_not_python_sum; preserve
the javascript_reduce and _left_to_right_sum assertions unchanged.

---

Nitpick comments:
In `@scripts/rarity/__init__.py`:
- Around line 3-15: Export InputError from scripts.rarity by importing it
alongside the existing symbols from nextgen_compat and adding "InputError" to
__all__, so consumers can catch the package’s public exception directly.

In `@scripts/rarity/nextgen_compat.py`:
- Around line 147-159: Update load_snapshot to pass an object_pairs_hook that
detects repeated keys while parsing and raises InputError identifying the
duplicate key, while preserving the existing UTF-8 loading, non-finite-number
rejection, and root-object validation behavior.

In `@tests/rarity/test_nextgen_compat.py`:
- Around line 407-419: Add a success-path test alongside
test_cli_distinguishes_bad_data_from_bad_invocation that invokes main with
FIXTURE and an --output temporary path, verifies it returns 0, then reads and
parses the output JSON and compares it with analyze_snapshot’s result for the
fixture.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 3a41b2a6-4297-45e2-b622-590abd0e58a1

📥 Commits

Reviewing files that changed from the base of the PR and between 6ab83b4 and 653c384.

📒 Files selected for processing (11)
  • INDEX.md
  • docs/generative-trait-analysis.md
  • notes/wip/2026-08-01-nextgen-rarity-analysis.md
  • scripts/rarity/__init__.py
  • scripts/rarity/analyze.py
  • scripts/rarity/nextgen_compat.py
  • tests/__init__.py
  • tests/rarity/__init__.py
  • tests/rarity/fixtures/nextgen-compatibility.expected.json
  • tests/rarity/fixtures/nextgen-compatibility.json
  • tests/rarity/test_nextgen_compat.py

Comment on lines +181 to +189
def _reject_non_finite_values(value: Any, path: str = "snapshot") -> None:
if isinstance(value, float) and not math.isfinite(value):
raise InputError(f"non-finite numeric value is prohibited: {path}")
if isinstance(value, dict):
for key, child in value.items():
_reject_non_finite_values(child, f"{path}.{key}")
elif isinstance(value, list):
for index, child in enumerate(value):
_reject_non_finite_values(child, f"{path}[{index}]")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Bound the recursion depth of the input walkers.

_reject_non_finite_values, _reject_precomputed_metric_fields, and _validate_citation_value all recurse once per nesting level of the supplied JSON. A snapshot file with deeply nested arrays or objects raises RecursionError, not InputError. RecursionError is not in the except tuple in scripts/rarity/analyze.py lines 56-62, so the CLI aborts with a traceback instead of the documented concise error: message. Add an explicit depth limit and convert the overflow into InputError.

🛡️ Proposed depth bound
+MAX_INPUT_DEPTH = 64
+
+
-def _reject_non_finite_values(value: Any, path: str = "snapshot") -> None:
+def _reject_non_finite_values(
+    value: Any, path: str = "snapshot", depth: int = 0
+) -> None:
+    if depth > MAX_INPUT_DEPTH:
+        raise InputError(f"snapshot nesting exceeds {MAX_INPUT_DEPTH} levels: {path}")
     if isinstance(value, float) and not math.isfinite(value):
         raise InputError(f"non-finite numeric value is prohibited: {path}")
     if isinstance(value, dict):
         for key, child in value.items():
-            _reject_non_finite_values(child, f"{path}.{key}")
+            _reject_non_finite_values(child, f"{path}.{key}", depth + 1)
     elif isinstance(value, list):
         for index, child in enumerate(value):
-            _reject_non_finite_values(child, f"{path}[{index}]")
+            _reject_non_finite_values(child, f"{path}[{index}]", depth + 1)

Apply the same depth parameter to _reject_precomputed_metric_fields and _validate_citation_value.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/rarity/nextgen_compat.py` around lines 181 - 189, Bound recursion in
_reject_non_finite_values, _reject_precomputed_metric_fields, and
_validate_citation_value with a shared depth parameter and explicit maximum
nesting limit; increment it for nested dict/list traversal and raise InputError
when exceeded, so deeply nested snapshots produce the CLI’s concise error
instead of RecursionError. Preserve existing validation behavior for inputs
within the limit.

Comment on lines +285 to +294
def test_left_fold_matches_javascript_reduce_not_python_sum(self) -> None:
values = [1e16, 1.0, -1e16, 1.0]

javascript_reduce = 0.0
for value in values:
javascript_reduce += value

self.assertEqual(sum(values), 2.0)
self.assertEqual(javascript_reduce, 1.0)
self.assertEqual(_left_to_right_sum(values), javascript_reduce)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🌐 Web query:

Which Python version added Neumaier compensated summation to the built-in sum for floats?

💡 Result:

Python 3.12 is the version that added Neumaier compensated summation to the built-in sum function for floating-point numbers [1][2][3]. This improvement was implemented to enhance the accuracy of floating-point sums while maintaining performance [4][5][1]. Before this change, the standard sum function used a simple running total, which was less precise [5][1]. The addition was contributed by Raymond Hettinger [4][1].

Citations:


🏁 Script executed:

#!/bin/bash
set -eu

printf '%s\n' '--- candidate files ---'
fd -i 'test_nextgen_compat.py|2026-08-01-nextgen-rarity-analysis.md|pyproject.toml|tox.ini|\.python-version|runtime.txt' .

printf '%s\n' '--- relevant test section ---'
test_file="$(fd -i -t f 'test_nextgen_compat.py' | head -n 1)"
sed -n '260,305p' "$test_file"

printf '%s\n' '--- runtime/support declarations ---'
for f in $(fd -i -t f 'pyproject.toml|tox.ini|\.python-version|runtime.txt'); do
  printf '\n### %s\n' "$f"
  rg -n -i 'python|requires-python|py3|3\.[0-9]+' "$f" || true
done

printf '%s\n' '--- referenced note ---'
note="$(fd -i -t f '2026-08-01-nextgen-rarity-analysis.md' | head -n 1)"
if [ -n "$note" ]; then
  sed -n '64,80p' "$note"
fi

printf '%s\n' '--- definitions and usages ---'
rg -n '_left_to_right_sum|javascript_reduce|test_left_fold_matches_javascript_reduce_not_python_sum' .

Repository: 6529-Collections/6529networkmuseum

Length of output: 3988


🏁 Script executed:

#!/bin/bash
set -eu

printf '%s\n' '--- standalone numeric behavior ---'
python3 - <<'PY'
import sys

values = [1e16, 1.0, -1e16, 1.0]
left_fold = 0.0
for value in values:
    left_fold += value

print(f"interpreter={sys.implementation.name} {sys.version.split()[0]}")
print(f"sum={sum(values)!r}")
print(f"left_fold={left_fold!r}")
print(f"sum_equals_left_fold={sum(values) == left_fold}")
PY

printf '%s\n' '--- left-to-right implementation ---'
sed -n '75,95p' scripts/rarity/nextgen_compat.py

printf '%s\n' '--- tracked project/runtime configuration ---'
git ls-files | rg '(^|/)(pyproject\.toml|setup\.cfg|setup\.py|tox\.ini|noxfile\.py|Pipfile|poetry\.lock|\.python-version|runtime\.txt|.*requirements.*|.*Dockerfile.*|.*workflow.*\.ya?ml)$' || true
rg -n -i 'requires-python|python_requires|python-version|python[ ]*[><=~]|py3[0-9]|3\.[0-9]+' \
  --glob '!notes/**' --glob '!tests/**' --glob '!scripts/**' . || true

Repository: 6529-Collections/6529networkmuseum

Length of output: 50391


🏁 Script executed:

#!/bin/bash
set -eu

printf '%s\n' '--- standalone numeric behavior ---'
python3 - <<'PY'
import sys

values = [1e16, 1.0, -1e16, 1.0]
left_fold = 0.0
for value in values:
    left_fold += value

print(f"interpreter={sys.implementation.name} {sys.version.split()[0]}")
print(f"sum={sum(values)!r}")
print(f"left_fold={left_fold!r}")
print(f"sum_equals_left_fold={sum(values) == left_fold}")
PY

printf '%s\n' '--- left-to-right implementation ---'
sed -n '75,95p' scripts/rarity/nextgen_compat.py

printf '%s\n' '--- tracked runtime configuration filenames ---'
git ls-files | rg '(^|/)(pyproject\.toml|setup\.cfg|setup\.py|tox\.ini|noxfile\.py|Pipfile|poetry\.lock|\.python-version|runtime\.txt|.*requirements.*|.*Dockerfile.*)$' || true

Repository: 6529-Collections/6529networkmuseum

Length of output: 997


Remove the version-specific sum() assertion. If the test runs on CPython 3.11 or earlier, sum(values) returns 1.0; CPython 3.12+ returns 2.0. The repository does not pin Python. Preserve the javascript_reduce and _left_to_right_sum assertions, and make the sum() check conditional or remove it.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/rarity/test_nextgen_compat.py` around lines 285 - 294, Remove the
version-dependent sum(values) equality assertion from
test_left_fold_matches_javascript_reduce_not_python_sum; preserve the
javascript_reduce and _left_to_right_sum assertions unchanged.

@punk6529
punk6529 merged commit ff1c582 into main Aug 1, 2026
2 checks passed
@punk6529
punk6529 deleted the codex/nextgen-rarity-tooling branch August 1, 2026 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant