Repository navigation
populate --sync-bodies: push file bodies to existing issues (#7) - #10
Conversation
populate was create-only, so editing (or unwrapping) a body in the plan
file could never reach an issue that already exists on GitHub.
--sync-bodies is the per-issue inverse of update: it pushes existing
issues' bodies AND milestone descriptions (decision 2: it was easy)
file -> GitHub, creates nothing, and joins the mutually exclusive stage
group.
Conflict stance per decision 1 — refuse-on-divergence by default,
--force for last-writer-wins — operationalized without a stored base
via a three-way classification per item:
in-sync identical after CRLF/trailing-whitespace normalization
(GitHub stores web edits with CRLF) -> skip
reflow identical after unwrap() of both sides — the content
matches, only line wrapping differs, so pushing cannot
lose anyone's words -> pushed without --force; this is
the case the flag exists for
divergent the content itself differs; with no base the engine
cannot tell which side moved -> refused per item (exit 1)
unless --force
Refusals are per-item, so one divergent body does not block the
reflow-safe ones; --dry-run prints exactly the live plan and mutates
nothing; the confirmation prompt states how many bodies/descriptions
may be rewritten; a note reminds that sync never creates when plan
issues are missing from GitHub. --keep-line-breaks composes (file-side
bodies are the effective, by-default-unwrapped ones, matching what
populate would have pushed at creation).
Decision 3 (how the README/CLAUDE.md direction contract should describe
this deliberate exception) is deferred to new issue #9, as requested.
GitHubClient gains update_issue_body / update_milestone_description;
the recorded fake gh learns 'issue edit' and milestone PATCH. Suite
145 -> 153 (pure classification tests; end-to-end reflow push +
in-sync second run; divergence refusal then forced win; dry-run
mutates nothing; sync-never-creates). The roadmap-upkeep skill
documents the workflow. Engine VERSION 0.5.0.
AI-assisted: Claude Fable 5 (Anthropic)
There was a problem hiding this comment.
Pull request overview
Adds explicit synchronization of existing issue bodies and milestone descriptions from the roadmap file to GitHub.
Changes:
- Adds in-sync, reflow, and divergence classification with optional forced updates.
- Extends the GitHub client and fake CLI with update operations.
- Adds integration coverage, workflow documentation, and a version bump.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
scripts/gh_project_populate.py |
Implements --sync-bodies behavior and CLI options. |
scripts/_gh_project_lib.py |
Adds issue and milestone update methods. |
scripts/tests/test_integration.py |
Tests synchronization scenarios. |
scripts/tests/fake_gh/gh |
Simulates update commands. |
.claude/skills/roadmap-upkeep/SKILL.md |
Documents the synchronization workflow. |
scripts/VERSION |
Bumps the engine to 0.5.0. |
Suppressed comments (1)
scripts/gh_project_populate.py:183
unwrap()is not safe as an equivalence check for Markdown hard breaks: it removes the two trailing spaces before a newline. For example, file texta band a GitHub bodya \nbare classified asreflow, so this path overwrites an intentional<br>without requiring--force, contrary to the refuse-on-divergence guarantee. Treat explicit hard-break syntax as divergent (unless the normalized bodies were already equal), and add a regression case.
if unwrap(ours) == unwrap(theirs):
return "reflow"
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| def _normalized(body: str) -> str: | ||
| """Comparison form: GitHub stores web edits with CRLF, and trailing | ||
| whitespace is never meaningful.""" | ||
| return body.replace("\r\n", "\n").strip() |
There was a problem hiding this comment.
Confirmed — and the short-circuit makes it worse than a wrong label: an in-sync misclassification means even --force cannot push, since force only arbitrates the divergent branch. Fixed in 7829904: _normalized uses rstrip(), so leading whitespace (a four-space-indented first line is a code block) stays meaningful; the indented-vs-unindented pair now classifies divergent, where --force can make the file win. Regression cases pin both the divergence and the genuinely-identical in-sync counterpart. (The 'also line 182' sighting was the same single _normalized helper feeding both comparison sites — one fix covers both.)
…aks diverge Both findings from Copilot's PR #10 review were legitimate holes in the classification's safety guarantee: - _normalized used strip(), which also eats LEADING whitespace: a GitHub body whose first line is four-space-indented (a code block) compared in-sync with an unindented file body — and since in-sync short-circuits, even --force could not push. rstrip() now preserves leading whitespace; such a pair classifies divergent and --force can make the file win. - unwrap() deliberately removes markdown hard breaks (two trailing spaces, or a backslash, before a newline), so an INTENTIONAL <br> on the GitHub side classified as reflow and was overwritten without --force, contrary to refuse-on-divergence. GitHub-side hard-break syntax now forces divergent whenever the normalized bodies differ. Deliberately asymmetric: a hard break on the FILE side alone still reflows, because the push carries it to GitHub intact — nothing GitHub-side is lost. Five regression cases pin both (leading-indent divergence and its in-sync counterpart; two-space and backslash hard breaks; identical hard-break bodies still in-sync; file-side hard break still reflow). Suite 153 -> 156. AI-assisted: Claude Fable 5 (Anthropic)
Response to the suppressed comment (7829904)scripts/gh_project_populate.py:183 — hard breaks vs the reflow equivalence: confirmed. Fixed as suggested, with one deliberate refinement: the check is asymmetric. GitHub-side hard-break syntax forces Regression cases cover the two-space and backslash forms, the identical-bodies case, and the file-side-only case. Suite 153 → 156. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
scripts/gh_project_populate.py:202
- File-side backslash hard breaks do not take the documented safe reflow path.
unwrap("a\\\nb")retains the backslash ("a\\ b"), so comparing it with"a\nb"returnsdivergentand requires--force, even though the push preserves the file-side break and lines 193–194 say this case should not diverge. Strip only the file-side hard-break marker for the equivalence comparison (while retaining the GitHub-side guard), and add the backslash counterpart to the regression test.
if unwrap(ours) == unwrap(theirs):
return "reflow"
| pushed += 1 | ||
| print(f" ~ would push ({tag}): {label}") | ||
| continue | ||
| result = push() |
There was a problem hiding this comment.
Confirmed — the prompt-sized window (plus earlier items' rate-limit delays) sat entirely outside the refuse-on-divergence guarantee. Fixed in 752027a with the revalidate-before-write variant of your suggestion: live runs re-fetch each target immediately before mutating (new get_issue_body / get_milestone_description — one targeted GET per pushed item, which is a different animal from the O(n²) list-per-item pattern #293 banned) and classify against that fresh text; dry runs keep previewing from the run's snapshot without fetching. On the conditional-update alternative: GitHub's REST API offers no If-Match/version predicate on issue or milestone PATCHes, so a milliseconds-wide race remains irreducible — the executor's docstring now says exactly that rather than overclaiming. Pinned by a unit test where the snapshot says reflow-safe but the fresh fetch says divergent: refused, nothing mutated; plus dry-runs-never-fetch.
…eflow
Both round-2 findings were legitimate:
- TOCTOU: the divergence verdict came from the snapshot fetched before
the interactive confirmation, while push() mutated unconditionally —
an edit made while the prompt sat open (or during earlier items'
delays) was overwritten undetected, outside the refuse-on-divergence
guarantee. Live runs now re-fetch each target immediately before
mutating (GitHubClient.get_issue_body / get_milestone_description —
one targeted GET per pushed item, a different beast from the
O(n^2) list-per-item that #293 banned) and classify against the
fresh text; dry runs keep previewing from the snapshot without
fetching. GitHub's API offers no conditional issue update, so a
milliseconds-wide window remains; the docstring says so plainly.
- The documented file-side hard-break reflow only held for the
two-space form: the backslash form survives unwrap's join ('a\\ b')
and spuriously demanded --force for a push that preserves the break
verbatim. The equivalence comparison now defangs file-side
hard-break markers (the GitHub-side guard is untouched).
The recorded fake gh learns the two single-resource GETs. Regression
tests: fresh-divergent-while-snapshot-reflow is refused with nothing
mutated, dry runs never fetch, and the backslash counterpart joins the
file-side reflow case. Suite 156 -> 157.
AI-assisted: Claude Fable 5 (Anthropic)
Response to round 2's suppressed comment (752027a)Backslash file-side hard breaks vs the documented reflow path: confirmed. The two-space form vanished under unwrap's join, but the backslash form survived it ( |
…tems
Reviewing my own diff surfaced one real correctness gap and three
smaller improvements.
The gap: update writes the DEFANGED form of marker-bearing GitHub
bodies into the file ('<!-- END GENERATED (escaped): ... -->'), so the
post-update mirror of any such issue classified divergent forever —
and --force would 'resolve' it by pushing our escape artifacts over
the real body. classify_sync now (a) treats the exact escaped mirror
as in-sync, and (b) gives raw GitHub-side marker text no reflow
shortcut (the file can only ever hold the escaped form, so any push
sends escapes upstream; --force remains true last-writer-wins,
escapes included, and says so). neutralize_markers moves to
_gh_project_lib since both scripts now need it; update re-imports it.
Pinned by classification units plus an end-to-end round trip: GitHub
body edited to contain raw marker text, update defangs it into the
file, sync reports in-sync and GitHub's raw text survives. (The test
also documents why the plan-authoring path cannot produce this: lint
correctly refuses raw markers in the file.)
Polish: --force without --sync-bodies now errors instead of being
silently ignored; execute_sync_bodies drops its never-used client
parameter; the confirmation prompt says 'up to N' since in-sync items
are counted but never rewritten.
Suite 157 -> 161.
AI-assisted: Claude Fable 5 (Anthropic)
Self-review (f2af04f)Reviewed my own diff top to bottom; found one real correctness gap and three polish items, all fixed. The gap — escape-blindness in Polish: Suite 157 → 161; ruff/lint/ |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (3)
scripts/gh_project_populate.py:205
- An empty GitHub issue body is rendered by
updateas_(no description on GitHub)_(gh_project_update.py:163), and the plan parser then treats that sentinel as the desired body. Unlike marker escapes, this update-generated artifact is not recognized here: the next sync reports divergence, and--forcewrites the placeholder into the previously empty issue. Treat this sentinel as an in-sync mirror for issue pairs only (so an intentional milestone description with the same text is not suppressed), and add an empty-body round-trip test.
if ours == neutralize_markers(theirs):
scripts/gh_project_populate.py:285
- In a dry run,
pushedcounts planned mutations, but this summary reports them as already completed (for example,2 pushed) even though nothing was changed. Use dry-run-aware wording such aswould pushso the final status does not contradict the per-item output.
print(f" sync: {pushed} pushed, {in_sync} already in sync, "
f"{refused} divergent (refused), {failed} failed")
scripts/_gh_project_lib.py:1190
- This new pure parser has no unit coverage, although the repository requires tests for every pure function and
scripts/tests/test_gh_project_lib.py:1-7is explicitly the pure-core test suite. The integration tests exercise successful string extraction, but not malformed JSON, a non-object response, a missing/null field, or a non-string field; add focused tests that pin these Result/error contracts.
def _parse_object_field(
stdout: str, field: str, kind: str
) -> Result[str, PipelineError]:
… units
All three suppressed findings were legitimate:
- The empty-body sentinel was the second update-artifact blind spot
(the first was marker escapes, caught in self-review): update
renders an empty GitHub body as '_(no description on GitHub)_', the
parser hands that back as the desired body, sync diverged forever,
and --force would write the placeholder into a deliberately empty
issue. The sentinel is now a shared lib constant
(EMPTY_BODY_PLACEHOLDER, used by update's two renderers), and
classify_sync accepts allow_empty_placeholder — applied to ISSUE
targets only, per Copilot's own refinement, since a milestone
description could legitimately BE that text. Sync targets are now a
frozen SyncTarget dataclass carrying kind (the 5-tuples were at
their limit). End-to-end round trip pinned: empty plan body ->
populate pushes '' -> update writes the sentinel into the file ->
sync reports in-sync and the GitHub body stays empty.
- The dry-run summary reported planned mutations as completed
('2 pushed'); it now says 'would push', matching the per-item lines.
- _parse_object_field gained the pure-core unit tests the house rules
demand — and writing them exposed that the declared
Result[str, ...] contract lied: a non-string field value was
returned as-is. It is now a PARSING_ERROR.
Suite 161 -> 168.
AI-assisted: Claude Fable 5 (Anthropic)
Reply to round 3, suppressed finding 1 — the empty-body sentinel (ca05b20)Confirmed, and a sharp catch: this was the second update-artifact blind spot in Fixed with your refinement adopted exactly: the sentinel is now a shared lib constant ( |
Reply to round 3, suppressed finding 2 — dry-run summary wording (ca05b20)Confirmed — the per-item lines said |
Reply to round 3, suppressed finding 3 —
|
Closes #7, implementing all three recorded decisions.
Decision 1 — refuse-on-divergence,
--forcefor last-writer-winsThe engine stores no base version, so "did GitHub move?" is not directly answerable. The stance is operationalized as a pure three-way classification per item (
classify_sync):in-syncreflowunwrap()of both sides — content matches, only wrapping differs, pushing cannot lose anyone's words--forceneeded — this is the flag's reason to exist)divergent--forcepushes the file's versionRefusals are per-item (one divergent body doesn't block the reflow-safe rest),
--dry-runprints exactly the live plan, and the confirmation prompt states the blast radius.Decision 2 — milestone descriptions: included
It was easy: same classification, same snapshot (zero extra reads), pushed via a milestone
PATCH.Decision 3 — direction-contract docs: deferred to #9
Filed with the concern stated concisely: the contract tables are the template's core teaching device, and the docs need a formulation that keeps them crisp while pinning down when this deliberate exception applies.
Mechanics
--sync-bodiesjoins the mutually exclusive stage group (create nothing);--keep-line-breakscomposes (file-side bodies are the effective ones populate would push at creation);GitHubClientgainsupdate_issue_body/update_milestone_description; the recorded fakeghlearnsissue editand milestonePATCH. Suite 145 → 153: pure classification, end-to-end reflow push with an in-sync second run, divergence refusal then forced win, dry-run mutating nothing, and sync-never-creates. The roadmap-upkeep skill documents the workflow.VERSION→ 0.5.0.nix flake checkgreen; ruff clean.AI-assisted development: Claude Fable 5 (Anthropic)