feat(catalog-plugin): import music-catalog-diligence via subtree (3/3)#27
feat(catalog-plugin): import music-catalog-diligence via subtree (3/3)#27sidneyswift wants to merge 2 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
6 issues found across 112 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="plugins/music-catalog-diligence/scripts/build-manual-review-queue.py">
<violation number="1" location="plugins/music-catalog-diligence/scripts/build-manual-review-queue.py:65">
P2: Filename fallback uses suffix matching (`endswith(name)`), which can create false positives for contributed files.</violation>
</file>
<file name="plugins/music-catalog-diligence/scripts/validate-normalized-ledger.py">
<violation number="1" location="plugins/music-catalog-diligence/scripts/validate-normalized-ledger.py:37">
P2: Avoid loading the entire ledger CSV into memory; iterate rows as a stream to prevent unnecessary memory growth on large files.</violation>
</file>
<file name="plugins/music-catalog-diligence/scripts/calculate-concentration.py">
<violation number="1" location="plugins/music-catalog-diligence/scripts/calculate-concentration.py:54">
P2: `top_n_pct` uses signed amounts after absolute-value ranking, which can misstate concentration when negative entries exist.</violation>
</file>
<file name="plugins/music-catalog-diligence/scripts/extract-pdf-statement.py">
<violation number="1" location="plugins/music-catalog-diligence/scripts/extract-pdf-statement.py:285">
P2: Path-based template detection is not OS-agnostic; Windows-style paths can prevent expected template matches.</violation>
<violation number="2" location="plugins/music-catalog-diligence/scripts/extract-pdf-statement.py:314">
P1: Amount parsing drops comma decimals, which can inflate extracted values for locale-formatted statements.</violation>
</file>
<file name="plugins/music-catalog-diligence/scripts/normalize-royalty-statement.py">
<violation number="1" location="plugins/music-catalog-diligence/scripts/normalize-royalty-statement.py:441">
P1: When no "Total" column exists and all component columns are also missing, `sum()` over an empty generator returns `0`, which then formats as `"0.00"` for `owner_net_amount`. This creates a false data point implying a computed zero-dollar amount in a row that has no financial data at all. Guard against the empty-components case so the output is `""` instead.</violation>
</file>
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed. cubic prioritizes the most important files to review.
On a pro plan you can use ultrareview for larger PRs.
| text = new_text | ||
| detected_currency = currency | ||
| break | ||
| text = text.replace(",", "").replace("$", "").replace("€", "").replace("£", "").replace("¥", "").strip() |
There was a problem hiding this comment.
P1: Amount parsing drops comma decimals, which can inflate extracted values for locale-formatted statements.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At plugins/music-catalog-diligence/scripts/extract-pdf-statement.py, line 314:
<comment>Amount parsing drops comma decimals, which can inflate extracted values for locale-formatted statements.</comment>
<file context>
@@ -0,0 +1,607 @@
+ text = new_text
+ detected_currency = currency
+ break
+ text = text.replace(",", "").replace("$", "").replace("€", "").replace("£", "").replace("¥", "").strip()
+ if text.startswith("(") and text.endswith(")"):
+ text = "-" + text.strip("()")
</file context>
| "other": _to_float(first(row, "Other Income", "Other")), | ||
| } | ||
| total_value = _to_float(first(row, "Total")) | ||
| if total_value is None: |
There was a problem hiding this comment.
P1: When no "Total" column exists and all component columns are also missing, sum() over an empty generator returns 0, which then formats as "0.00" for owner_net_amount. This creates a false data point implying a computed zero-dollar amount in a row that has no financial data at all. Guard against the empty-components case so the output is "" instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At plugins/music-catalog-diligence/scripts/normalize-royalty-statement.py, line 441:
<comment>When no "Total" column exists and all component columns are also missing, `sum()` over an empty generator returns `0`, which then formats as `"0.00"` for `owner_net_amount`. This creates a false data point implying a computed zero-dollar amount in a row that has no financial data at all. Guard against the empty-components case so the output is `""` instead.</comment>
<file context>
@@ -0,0 +1,797 @@
+ "other": _to_float(first(row, "Other Income", "Other")),
+ }
+ total_value = _to_float(first(row, "Total"))
+ if total_value is None:
+ total_value = sum(v for v in components.values() if v is not None)
+ components_note = ", ".join(
</file context>
| return True | ||
| # Try common alternate forms — bare filename, source/-prefixed. | ||
| name = str(manifest_entry.get("filename") or "") | ||
| if name and any(src.endswith("/" + name) or src.endswith(name) for src in ledger_sources): |
There was a problem hiding this comment.
P2: Filename fallback uses suffix matching (endswith(name)), which can create false positives for contributed files.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At plugins/music-catalog-diligence/scripts/build-manual-review-queue.py, line 65:
<comment>Filename fallback uses suffix matching (`endswith(name)`), which can create false positives for contributed files.</comment>
<file context>
@@ -0,0 +1,218 @@
+ return True
+ # Try common alternate forms — bare filename, source/-prefixed.
+ name = str(manifest_entry.get("filename") or "")
+ if name and any(src.endswith("/" + name) or src.endswith(name) for src in ledger_sources):
+ return True
+ return False
</file context>
| reader = csv.DictReader(handle) | ||
| fieldnames = set(reader.fieldnames or []) | ||
| missing = sorted(REQUIRED_COLUMNS - fieldnames) | ||
| rows = list(reader) |
There was a problem hiding this comment.
P2: Avoid loading the entire ledger CSV into memory; iterate rows as a stream to prevent unnecessary memory growth on large files.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At plugins/music-catalog-diligence/scripts/validate-normalized-ledger.py, line 37:
<comment>Avoid loading the entire ledger CSV into memory; iterate rows as a stream to prevent unnecessary memory growth on large files.</comment>
<file context>
@@ -0,0 +1,69 @@
+ reader = csv.DictReader(handle)
+ fieldnames = set(reader.fieldnames or [])
+ missing = sorted(REQUIRED_COLUMNS - fieldnames)
+ rows = list(reader)
+
+ errors: list[str] = []
</file context>
| head = sum(amount for _, amount in ranked[:n]) | ||
| return round(head / total * 100, 2) |
There was a problem hiding this comment.
P2: top_n_pct uses signed amounts after absolute-value ranking, which can misstate concentration when negative entries exist.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At plugins/music-catalog-diligence/scripts/calculate-concentration.py, line 54:
<comment>`top_n_pct` uses signed amounts after absolute-value ranking, which can misstate concentration when negative entries exist.</comment>
<file context>
@@ -0,0 +1,219 @@
+def top_n_pct(ranked: list[tuple[str, float]], total: float, n: int) -> float | None:
+ if total == 0 or not ranked:
+ return None
+ head = sum(amount for _, amount in ranked[:n])
+ return round(head / total * 100, 2)
+
</file context>
| head = sum(amount for _, amount in ranked[:n]) | |
| return round(head / total * 100, 2) | |
| head = sum(abs(amount) for _, amount in ranked[:n]) | |
| denominator = sum(abs(amount) for _, amount in ranked) | |
| return round(head / denominator * 100, 2) if denominator else None |
|
|
||
| def detect_template(rel_path: str, headers: list[str | None]) -> PDFTemplate | None: | ||
| header_text = _normalize_header(headers).lower() | ||
| rel_lower = rel_path.lower() |
There was a problem hiding this comment.
P2: Path-based template detection is not OS-agnostic; Windows-style paths can prevent expected template matches.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At plugins/music-catalog-diligence/scripts/extract-pdf-statement.py, line 285:
<comment>Path-based template detection is not OS-agnostic; Windows-style paths can prevent expected template matches.</comment>
<file context>
@@ -0,0 +1,607 @@
+
+def detect_template(rel_path: str, headers: list[str | None]) -> PDFTemplate | None:
+ header_text = _normalize_header(headers).lower()
+ rel_lower = rel_path.lower()
+ candidates: list[tuple[int, PDFTemplate]] = []
+ for template in TEMPLATES:
</file context>
| rel_lower = rel_path.lower() | |
| rel_lower = rel_path.replace("\\", "/").lower() |
cc7923a to
afe7858
Compare
f293c51 to
73718a6
Compare
afe7858 to
5d6d34c
Compare
73718a6 to
94e6046
Compare
5d6d34c to
ad0dffb
Compare
94e6046 to
725bab7
Compare
ad0dffb to
972245c
Compare
725bab7 to
69afe7a
Compare
972245c to
6794f24
Compare
After the subtree import, two follow-ups make the catalog plugin
production-ready:
1. Register the plugin in marketplace.source.json so customers can
/plugin install music-catalog-diligence@recoupable. Regenerated the
three platform marketplace files.
2. Catalog-only hardening from the original PR's bot-review passes:
Plugin manifests:
- .{claude,codex,cursor}-plugin/plugin.json: repository field now
points at recoupable/skills (the actual home repo), not the
pre-merge recoupable/music-catalog-diligence repo.
Hooks:
- hooks.json: quote ${CLAUDE_PLUGIN_ROOT} so paths with spaces survive
shell tokenization.
- protect-source-files.sh: extend deny glob to match relative
(deals/.../source), dot-relative (./deals/.../source), and absolute
(/abs/deals/.../source) paths so the immutable-evidence guard
cannot be bypassed.
Diligence script hardening:
- calculate-nps-nls-bridge.py: switch monetary math to Decimal,
reject non-object top-level and non-dict adjustment entries.
- extract-pdf-statement.py: guard period_index against short rows;
return summary status "partial" and exit 1 when any PDF errored.
- normalize-royalty-statement.py: treat zero normalized rows as
partial, never ok.
- validate-deal-workspace.py: catch OSError and UnicodeDecodeError
so unreadable files surface as validation errors instead of crashes.
- validate-evidence-ledger.py: validate non-object top-level JSON
and require evidence_id to be a string before deduping.
- validate-normalized-ledger.py: flag blank ledger_line_id rows so
unidentified rows do not pass silently.
- test-normalize-royalty-statement.py: guard write_csv against
empty rows.
- requirements.txt: pin pillow>=12.2.0 to avoid vulnerable
transitive versions pulled in by pdfplumber.
Docs and templates:
- evidence-ledger.json: ship empty entries (no fake placeholder
row with confidence="example").
- skills/catalog-analysis/references/output-templates.md: align
quality-of-earnings bridge with the canonical adjustment ladder
from catalog-analysis SKILL.md and add the separate
sensitivity-table section the skill requires.
- references/normalization.md: document expected-status.json for
partial fixtures.
- references/tooling-landscape.md: fix awkward "needs standardized"
phrasing.
- commands/catalog-analyze.md: show the required positional input
args for calculate-concentration.py and calculate-nps-nls-bridge.py.
- skills/{royalty-audit,catalog-analysis}/SKILL.md: move
pro-performance-income.md to plugin-level references/ so the two
skills no longer cross-depend.
Tests pass:
- python3 plugins/music-catalog-diligence/scripts/test-helpers.py
- python3 plugins/music-catalog-diligence/scripts/test-validate-deal-workspace.py
- python3 plugins/music-catalog-diligence/scripts/test-normalize-royalty-statement.py
- python3 scripts/validate-manifests.py
Co-authored-by: Cursor <cursoragent@cursor.com>
69afe7a to
9100d3d
Compare
The problem
The
music-catalog-diligenceplugin (royalty audits, rights diligence, valuation workpapers, IC memos for music catalog deals — 9 skills + 5 agents + 6 commands + scripts) lives in a separate GitHub repo. After PR #25, we have a unified marketplace — but the catalog plugin isn't in it. Customers can't install it from the new marketplace.The fix
Imports the entire
music-catalog-diligenceplugin intoplugins/viagit subtree(preserving full upstream history at commita95f3dc). Registers it inmarketplace.source.jsonso customers can install it.After this lands:
works in Claude Code, Codex, and Cursor.
How to review (mostly mechanical)
This PR has 2 commits, by design:
Commit 1 — the subtree import (~89 files)
a95f3dc.Commit 2 — register + harden (~25 files)
marketplace.source.json, regenerates the 3 platform marketplace files.Recoupbranding + therepositoryfield pointed atrecoupable/skills(was the pre-merge repo).Files (113)
89 files come in mechanically via the subtree. Real review surface: ~25 files in commit 2.
What you should focus on
a95f3dc.marketplace.source.jsonlooks right.Test plan
python3 scripts/validate-manifests.pyexits 0python3 plugins/music-catalog-diligence/scripts/test-helpers.py— 7 passpython3 plugins/music-catalog-diligence/scripts/test-validate-deal-workspace.py— 2 passpython3 plugins/music-catalog-diligence/scripts/test-normalize-royalty-statement.py— 7 passrecoupable/music-catalog-diligence@a95f3dcand the in-tree subtree at the same SHA/plugin install music-catalog-diligence@recoupworking in Claude CodeMerge order
PR 3 of 3. Stacked on #25 — auto-rebases when #25 lands. Save for last.
Merge preference
Please merge yourself when satisfied — I'd rather you push the button than approve.
🤖 Made with Cursor