Single-source README integrations and gate roadmap current marker#576
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb892c4aa7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
dgenio
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES — two quick fixes; the data and table-dedup work itself is clean.
🔴 Blocker — CI is red on test (3.12) (make llms-check). The failure is not the new tests (those pass); it's llms-full.txt drift. That generated file embeds README.md and docs/agent-context/workflows.md verbatim, both edited here, so it's now stale. Run make llms and commit the regenerated llms.txt/llms-full.txt. This is also a stated #531 acceptance criterion ("make ci green").
🟠 Major — the roadmap guard only covers one of the two ✅ current tables (inline on scripts/check_readme_version.py). The regex matches the summary row (L619) but not the dated "Recent milestones" row (L795) — which is the exact table #531 cited as stale evidence. As written, the guard wouldn't catch the bug it was built to prevent. One-line regex relaxation + a dated-row fixture closes it.
🟡 Minor (non-blocking) — leftover precise test count. #531 task 4 asked to reconcile 1100+ vs 1150+. L456 was de-precised ("Broad regression suite") but 1150+ tests passing still sits at L42, mixing a vague claim with a precise one. Worth aligning while here; not a blocker.
What's good: framework table is cleanly single-sourced (mirror + "keep in sync" comment removed), roadmap data is corrected to v0.14.1, CHANGELOG + workflows.md updated, and the new checker logic has passing unit tests (7/7 locally). This overlaps with the existing Codex thread, which raised the same regex gap.
Generated by Claude Code
dgenio
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES — the prior blocker (make llms-check) and major (dated-row regex) are both resolved and verified locally (make llms-check clean, 8/8 unit tests pass, regex now matches both the summary and dated ✅ current rows). One blocker remains, drift-induced:
🔴 Blocker — stale base / version drift. This branch is 17 commits behind main, which is now 0.15.0; it conflicts in README.md, CHANGELOG.md, docs/agent-context/workflows.md, and llms-full.txt, and mergeable_state is dirty. After rebasing, the v0.14.1 markers here will lag — I simulated find_drift("0.15.0", README) and the new roadmap guard plus the existing "Current package version" and comparison self-reference checks all fail. Please:
- Rebase onto
origin/mainand resolve the 4 conflicts. - Bump
v0.14.1 → 0.15.0in: the "Current package version" line, the comparison self-reference, and both✅ current (v0.14.1)roadmap rows. - Re-run
make readme-version-check && make llms-check.
🟡 Minor (non-blocking). README.md:42 still reads 1150+ tests passing while L456 was de-precised to "Broad regression suite" (#531 task 4). Worth de-precising L42 (and its llms-full.txt:74 mirror) in the same pass.
What's good: the framework table is cleanly single-sourced (mirror table + "keep in sync" comment removed; the #framework-integrations and docs/interop.md links both resolve), the roadmap is corrected to v0.14.1, and the guard now catches exactly the dated-row staleness #531 was filed about — locked in by the new dated-row fixture. Nice work.
Generated by Claude Code
dgenio
left a comment
There was a problem hiding this comment.
Re-review of 6b64c3f — my two prior blockers are resolved. ✅
Verified locally on the new head:
- 🔴→✅
llms-full.txtdrift —python scripts/gen_llms.py --checknow reports "llms.txt and llms-full.txt are up to date." CI'smake llms-checkgate clears. - 🟠→✅ roadmap guard missed the dated row —
_ROADMAP_CURRENT_REnow allows an optional(date)before the cell delimiter and matches both✅ currentrows (findall→ the summary row L619 and the dated "Recent milestones" row L795). The added dated-row fixture locks it in; 8/8 unit tests pass.
I concur with the standing version/base-drift blocker (already detailed in the other review) — no need to re-litigate it; just confirming it's real and not double-counting my items:
- Branch is 17 commits behind
main(now0.15.0);git merge-treeshows real conflicts inCHANGELOG.md,README.md,docs/agent-context/workflows.md,llms-full.txt(mergeable_state: dirty). - Simulating the post-rebase state,
find_drift("0.15.0", README)returns 6 failures — the new guard correctly catches that thev0.14.1markers will lag once rebased. After rebasing, bump the "Current package version" line, the comparison self-reference, and both✅ current (v0.14.1)rows to0.15.0.
Leaving this as a comment (not an approval) only because of that drift — the #531 work itself is solid. Nice turnaround on the regex + llms fixes.
Generated by Claude Code
|
Addressed the base/version drift blocker in cdc2150. What changed:
Validation:
|
dgenio
left a comment
There was a problem hiding this comment.
APPROVE — all prior REQUEST_CHANGES items are resolved as of cdc2150. Verified locally on the merged head:
- 🔴→✅ Base/version drift. Branch now merges
origin/maincleanly (0 commits behind, no conflict markers). Version bumped to0.15.0inpyproject.toml, the "Current package version" line, the comparison self-reference, and both✅ current (v0.15.0)roadmap rows.find_drift("0.15.0", README)→[], so the new guard now tracks the correct version. - 🔴→✅ llms-check.
make llms-check→ "up to date";llms.txt/llms-full.txtregenerated. - 🟠→✅ Dated-row guard. Regex matches both
✅ currentrows; 8/8 unit tests pass. - 🟡→✅ Test-count nit. De-precised to "Broad regression suite" at both occurrences.
- Single-source table intact. Exactly one canonical Framework Integrations table (the merge correctly kept main's 4-column version); the mirror table + "keep in sync" comment remain removed, replaced by the
#framework-integrations+docs/interop.mdlink.
Gates green locally: readme-version-check ✅ · llms-check ✅ · pytest tests/test_check_readme_version.py 8/8 ✅ · git diff --check clean ✅. All #531 acceptance criteria met. Nice work on the rebase and the clean conflict resolution.
Generated by Claude Code
cdc2150 to
25d8d7c
Compare
|
Rebased onto current Kept both sides of the drift guard work:
Local validation:
|
dgenio
left a comment
There was a problem hiding this comment.
APPROVE (re-affirmed on 25d8d7c after the rebase onto current main). Verified locally on the new head:
- 0 commits behind
main(nowc407343); clean rebase, no conflict markers;git diff --checkclean. python3 scripts/check_readme_version.py→ "references and Python classifiers are in sync (0.15.0)" — the guard scope main added (classifier ↔ CI matrix) is satisfied, and your roadmap-current-marker additions are intact (find_drift("0.15.0")→[], both✅ current (v0.15.0)rows correct).make llms-check→ up to date.pytest tests/test_check_readme_version.py→ 12/12 pass.- Single-source Framework Integrations table preserved (no mirror/keep-in-sync comment;
#framework-integrations+docs/interop.mdlink intact).
No new findings. All #531 acceptance criteria still met post-rebase — good to merge.
Generated by Claude Code
Closes #531
Summary
readme-version-checkto fail when roadmapcurrentrows omit or lag the pyproject versionVerification
python3 scripts/check_readme_version.pyuvx --from ruff ruff format --check scripts/check_readme_version.py tests/test_check_readme_version.pyuvx --from ruff ruff check scripts/check_readme_version.py tests/test_check_readme_version.pyuvx --with . --with pytest --with pytest-asyncio python -m pytest tests/test_check_readme_version.py -qtmpdir=$(mktemp -d); ln -s /Users/wuyangfan/.local/bin/python3.11 "$tmpdir/python"; PATH="$tmpdir:$PATH" make readme-version-checkgit diff --check