claude-headless: first draft - #19
Conversation
bbkrr
left a comment
There was a problem hiding this comment.
Review-only pass (comments only, no code changed, nothing approved or merged)
Marked as a first draft, so I have reviewed it as one: the shape of the thing, not the polish. There is real work here. Five generators, a section-registry pattern that skips sections when the data is not there, and design context written down next to the code that uses it. The adaptive-section approach in the two report scripts is genuinely nice, and the caption builder correctly follows the repo's realrate.ai/rankings/... rule instead of linking the archive.
The blocker below is mechanical, not conceptual.
1. Nothing here runs as documented: every path says claude_practice/, the directory is claude_headless/
Across the six new skill files plus one context file there are 51 references to claude_practice/ and zero to claude_headless/. The directory was renamed but the docs were not. So the documented setup step creates a venv somewhere that does not exist, and the run step cds into a missing directory:
python3 -m venv "${CLAUDE_PROJECT_DIR}/claude_practice/.venv" # skills/auto-practice-cycle.md:38
cd "${CLAUDE_PROJECT_DIR}/claude_practice" # skills/auto-practice-cycle.md:52Anyone following the skills gets an error on the first command. A find-and-replace fixes it, but until it happens nobody can use this. The word "practice" also appears in prose throughout (the "learning copy" notes, the Practice build script docstrings, the argparse help text, and the RealRate-Report-Practice/1.0 User-Agent the scripts send to the archive), so the rename is worth doing deliberately rather than blindly.
2. Nine of the thirteen context files are byte-identical copies of files already in this repo
I fetched both sides and compared. These are exact duplicates of what is already at context/:
audience.md · brand-core.md · brand-voice.md · competitive-landscape.md · design-system.md · product-offering.md · sources.md · RealRate_logo_horizontal.svg · RealRate_logo_light.svg
That is roughly 800 duplicated lines, including two copies of the logo. skills/mindmap.md is not a copy (85 lines vs the existing 1872) so that one is fine.
This matters more here than in a normal repo, because this repo is the single source of truth for brand rules. Two copies of brand-voice.md means the day someone updates the tone rules, half the generators keep using the old ones, and nothing errors. Options in the inline comment.
3. Housekeeping that bites harder because this repo is public
- A compiled Python file is committed:
claude_headless/scripts/__pycache__/generate_mindmap.cpython-310.pyc(12 KB)..gitignorehas no__pycache__entry, so this will keep happening. output/is not ignored either. All five scripts write intooutput/us_<slug>/...(PDFs, PNGs, GIFs, caption.txtfiles). Onegit add -Apublishes generated client-facing material to a public repo..gitignorecurrently coversposts/*/*.pngbut nothing here.- Dependencies are not declared. The scripts need
cairosvgandmatplotlib;requirements.txtlists onlyrequests,Pillow,playwright,urllib3. The skills work around this with an ad-hocpip install Pillow cairosvg matplotlib.cairosvgalso needs system Cairo, which is the kind of thing that belongs in a README rather than being discovered at run time. CLAUDE.mdis not updated. It is the repo's map, with a folder-structure block and a skill lookup table. A new 25-file top-level tree is invisible to it, which means it is invisible to Claude.- No README in the new tree, so there is no single place that explains what
claude_headless/is or how it relates to the rootskills/.
4. Five copies of the same helpers, already in three shapes
INDUSTRY_SLUGS, resolve_slug, fetch_ranking_data, ecr_pct, clean_archive_text, resolve_company, fetch_svg, _load_font, load_logo and the colour constants are copy-pasted across the five scripts. I hashed each one: most are still identical or differ only in a docstring, so this is not yet a correctness problem. But fetch_ranking_data already has three variants, and one of them has a different return contract. Details inline.
5. Every network failure is swallowed
except Exception: continue in the fetch loops and except Exception: return None in fetch_svg. A typo, a schema change and a genuine outage all look the same: the script quietly walks back a year, or renders without a logo, and reports success. Inline comment has the smallest fix that keeps the walk-back behaviour.
CI as observed (reported, not waited on)
The only check is auto-merge, skipped (Dependabot workflow, not applicable). There is no test, lint or build workflow on this repo, so nothing verified that these scripts import, let alone run. Given finding 1, that gap is not theoretical: a one-line smoke test (--help on each script) would have caught the broken paths.
Security check
No credentials, no secrets, no shell execution, no user input reaching a dangerous sink. The network calls are urllib.request over HTTPS to one hardcoded host, with certificate verification left at the default. Nothing alarming. Three things worth naming:
- Remote SVGs are rendered locally.
fetch_svg()pulls company logos and causal-graph SVGs from realrate-archive and hands them tocairosvg.svg2png(bytestring=...). SVG is XML, and XML renderers are a classic path for external-entity and local-file-read tricks. The host is first-party, so this is fine as long as that host is trustworthy — worth remembering that realrate-archive has had a compromise before (realrate-archive#16), so "our own domain" is not the same as "safe input". - The generated industry-report PDF names the internal archive twice (
generate_industry_report.py:186and:413), and its own caveats text contemplates "using this report externally".CLAUDE.mdhas a standing rule: "Archive is internal only — data verification only, never shared publicly" and "Never link to sales pages, pricing, or the archive in public posts." Inline comment. - This repo is public. Everything added here (positioning, ICP profiles, competitor analysis, pricing-adjacent product notes) is now public a second time. That was already true of the originals, so this PR does not change the exposure, but duplicating it is a good moment to confirm that was a deliberate choice.
The design decision, in plain language
The docs call this a "learning copy" and a "practice build"; the directory name says claude_headless, which sounds like production tooling. Those are two different things and they want different answers to everything above. Three ways to go:
- (a) It is a scratchpad. Then say so in a README, leave the duplicated context alone (isolation is the point), and nobody needs to care about the drift. Cheapest, but a scratchpad with 3,700 lines in the main branch of a public repo will be mistaken for the real thing within a month.
- (b) It is the new headless pipeline. Then the duplicated context files should become references to the existing ones, the shared helpers should be one module, and
CLAUDE.mdshould describe it. More work now, but it is the version that survives. - (c) Merge it as a draft on a long-lived branch, fix the
claude_practicepaths so it at least runs, and decide (a) vs (b) once someone has used it for real.
My read is (c) now, (b) later. The blocking item is only finding 1, everything else is a judgement call about what this directory is for. Worth answering that question in the PR description before merge, because every other review comment depends on the answer.
On existing bot reviews
There are none. Copilot did not review this PR, so there is nothing to adjudicate. Given the size (3,727 lines, 25 files), it may be worth requesting one.
Handing back
No approval, no merge, no code changed. Finding 1 is the only one I would call blocking.
Reviewed by Claude Code (review-only mode).
| All five scripts share one venv. Create it if it doesn't already exist: | ||
|
|
||
| ```bash | ||
| python3 -m venv "${CLAUDE_PROJECT_DIR}/claude_practice/.venv" |
There was a problem hiding this comment.
This is the blocking one. Every path in the skill files points at claude_practice/, but the directory this PR adds is claude_headless/.
I counted across the new markdown: 51 occurrences of claude_practice, 0 of claude_headless.
| file | claude_practice refs |
|---|---|
skills/top10-infographic.md |
10 |
skills/auto-practice-cycle.md |
9 |
skills/company-report.md |
8 |
skills/industry-report.md |
8 |
skills/mindmap.md |
8 |
skills/top5-reveal-gif.md |
7 |
context/mindmap-design.md |
1 |
So this line creates a venv under a directory that does not exist, line 52's cd fails, and every documented run command points into nothing. Anyone following these skills hits an error on the first command.
The mechanical fix is a find-and-replace, but do it deliberately rather than globally, because "practice" also appears where it is prose rather than a path:
- the
This is a **learning copy** — isolated in claude_practice/notes in several skills - the
Practice build — RealRate ... Generatorscript docstrings description="Practice: generate a RealRate company report PDF."in argparse- the User-Agent the scripts actually send to the archive:
RealRate-Report-Practice/1.0in four scripts,RealRate-Infographic-Practice/1.0in the fifth
That last one is worth a decision rather than a blind replace: it is the string realrate-archive sees in its access logs. If this becomes real tooling, a stable, consistent agent string is more useful than five variations on "practice".
A --help smoke test on each script in CI would have caught the directory mismatch before review.
| @@ -0,0 +1,76 @@ | |||
| # RealRate — Brand Core | |||
There was a problem hiding this comment.
This file is byte-identical to context/brand-core.md already in this repo. I fetched both and compared; same for eight of its neighbours:
| duplicated file | identical to root context/? |
|---|---|
audience.md |
yes |
brand-core.md |
yes |
brand-voice.md |
yes |
competitive-landscape.md |
yes |
design-system.md |
yes |
product-offering.md |
yes |
sources.md |
yes |
RealRate_logo_horizontal.svg |
yes |
RealRate_logo_light.svg |
yes |
mindmap.md (skills) |
no — 85 lines vs 1872, genuinely new |
That is about 800 duplicated lines, including two copies of the logo SVG.
The four genuinely new context files (company-report-design.md, industry-report-design.md, infographic-design.md, mindmap-design.md) are the ones that earn their place here, and they are good.
Why this matters more in this repo than most: CLAUDE.md positions context/ as the single base layer that every skill reads from. Two copies of brand-voice.md means the first time someone tightens a tone rule, half the generators silently keep the old one, and nothing anywhere errors. The failure is invisible and it lands in published marketing copy.
Options, cheapest first:
- Delete the nine duplicates and point the skills at
../context/. The scripts already do relative-path traversal (RR_LOGO_SVG_PATHin three scripts isos.path.join(..., "..", "context", ...)), so it would resolve to the rootcontext/with one fewer..segment. Smallest diff, one source of truth. - Symlink them. Works on the developer machines here, but is a papercut in Git and on Windows.
- Keep the copies deliberately because isolation is the point of a practice sandbox. Defensible, but then say so in a README next to them, so the next person does not "helpfully" deduplicate and break the isolation.
Which one is right depends on whether this directory is a sandbox or the new pipeline, which is the open question in my review.
| @@ -0,0 +1,343 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
Anchoring the housekeeping here since the compiled artefact belongs to this file.
1. A .pyc is committed: claude_headless/scripts/__pycache__/generate_mindmap.cpython-310.pyc, 12 KB. .gitignore currently has no __pycache__ entry, so this will recur every time someone runs the scripts. Worth adding:
__pycache__/
*.py[cod]2. output/ is not ignored either, and that one is riskier. All five scripts write into os.getcwd()/output/us_<slug>/<skill>/: PDFs, PNGs, an animated GIF, and LinkedIn caption .txt files. This repo is public. One git add -A after a run publishes generated client-facing material, for real named companies, to a public repo. Current .gitignore covers posts/*/*.png and posts/*/*.psd but nothing under output/.
output/3. Dependencies are undeclared. These scripts import cairosvg and matplotlib; requirements.txt has only requests, Pillow, playwright, urllib3. The skills paper over it with an inline pip install Pillow cairosvg matplotlib, which means the dependency list lives in six markdown files instead of one manifest. cairosvg additionally needs system Cairo (libcairo2 / brew install cairo), which is exactly the sort of thing that should be written down once.
4. CLAUDE.md has not been updated. It opens with a folder-structure diagram and a skill-lookup table, and it is the file Claude actually reads to find its way around. A new 25-file top-level tree that is absent from it is, functionally, invisible.
|
|
||
| # --- Stage 1: FETCH ---------------------------------------------------------- | ||
|
|
||
| def fetch_ranking_data(slug: str, year: int | None): |
There was a problem hiding this comment.
Flagging the duplication here rather than on one of the four copies, because this version is the best one and it is the odd one out.
fetch_ranking_data exists in all five scripts in three shapes:
| variant | scripts | returns |
|---|---|---|
| A | company_report, mindmap, top5_reveal_gif |
data |
| B | industry_report |
data (docstring differs only) |
| C | this one | data, y |
Only this one tells the caller which year actually came back after the walk-back loop, and it is the only caller that can therefore do:
year = data.get("year", fetch_year) # generate_infographic.py:312The other four do a bare year = data.get("year"), so if the payload ever lacks a year field they carry None straight into the output filename (..._None_report.pdf) and the PDF cover. Small today, because the archive does emit year, but it is the kind of thing a shared helper fixes once instead of four times.
The wider picture, from hashing each duplicated block across the five scripts:
| helper | copies | shapes |
|---|---|---|
INDUSTRY_SLUGS (26 entries) |
5 | 1 (identical) |
resolve_slug |
5 | 2 (line wrapping only) |
fetch_ranking_data |
5 | 3 |
ecr_pct |
4 | 2 (docstring only) |
clean_archive_text |
3 | 2 (docstring only) |
resolve_company |
2 | 1 (identical) |
fetch_svg |
2 | 2 |
_load_font |
3 | 3 |
colour constants, RR_LOGO_SVG_PATH |
5 / 3 | — |
Credit where due: most of it is still identical, so the drift is early rather than entrenched. That is the good moment to pull it into claude_headless/scripts/_common.py — one INDUSTRY_SLUGS, one fetch, one palette. The tell that it is already costing something is that the best fetch_ranking_data and the best _load_font live in different files, and no script has both.
| data = json.loads(resp.read().decode("utf-8")) | ||
| if data.get("company_details"): | ||
| return data | ||
| except Exception: |
There was a problem hiding this comment.
except Exception: continue here, and except Exception: return None in fetch_svg at line 118, make three very different situations indistinguishable:
- the archive is down or the year genuinely does not exist (expected, the walk-back is the right response)
- the payload shape changed and
json.loadsordata.getnow behaves differently (a real bug, silently retried against three more years, then reported as "could not fetch") - a typo in this function (
urllib.reqest, a renamed variable) — caught by the sameexcept, invisible forever
The same pattern in fetch_svg means a company simply renders without its logo and without its causal-graph section, and the run still prints success. In a script whose whole output is a document someone publishes, "quietly produced a smaller report" is a worse failure than "stopped and told me".
Smallest change that keeps the walk-back exactly as it is:
except (urllib.error.URLError, TimeoutError, json.JSONDecodeError) as e:
tried[-1] += f" ({type(e).__name__}: {e})"
continueThen the final RuntimeError reports why each year failed rather than just listing four URLs, and anything not in that tuple, which is by definition a bug rather than a fetch problem, surfaces immediately.
Related, in main: the all loop catches bare Exception per company and collects failures, which is the right call there since one bad company should not kill a 50-company batch. That one I would keep, just consider printing type(e).__name__ alongside the message so a KeyError is distinguishable from a network blip in the summary.
| f"{label} currently tracks {n} companies in RealRate's archive. " | ||
| f"Aggregate figures across the tracked companies: {fields}. " | ||
| f"These are archive-reported totals, not RealRate's own estimate — " | ||
| f"verify against realrate-archive.com before publishing." |
There was a problem hiding this comment.
This string, and the matching one in sec_caveats_text at line 413, put realrate-archive.com into the body of the generated PDF:
"These are archive-reported totals, not RealRate's own estimate - verify against realrate-archive.com before publishing." # line 186
"...verify all figures at realrate-archive.com before using this report externally." # line 413
CLAUDE.md in this repo carries two standing rules that this runs into:
- Never link to sales pages, pricing, or the archive in public posts
- Archive is internal only — data verification only, never shared publicly
The second caveat sentence explicitly contemplates "using this report externally", so the intent is that these PDFs can leave the building, and the internal archive domain travels with them.
I do not think either instruction is wrong to have — they are genuinely useful notes for whoever reviews the draft. The problem is that they are baked into the artefact rather than shown to the operator. Two clean ways out:
- Print them to the console instead of rendering them into the PDF. The person running the script is the audience for "verify this before publishing", not the reader of the finished report.
generate_for_companyin the company script already prints aNOTE:line for the ECR discrepancy, so the pattern exists. - Keep them in the PDF but drop the domain, e.g. "verify against RealRate's internal archive before publishing". Same instruction, nothing external to leak.
Worth deciding either way before these get generated in bulk, since the wording is duplicated across two sections and would otherwise have to be chased down later.
First Draft to claude headless