feat(description-sync): sync GH issue descriptions to AgilePlace card details#87
Merged
Conversation
Adds the config/AgilePlace/GitHub client-boundary primitives that description sync (issue #65) will wire up in a later task: env_config()'s ap_description_max_length (safe-int-parse with WARN fallback, default 20000), agileplace.op_description/card_description (description-present path plus lazy get_card fallback, matching the API's list_cards()-never- returns-description shape), and ghkit.edit_issue_body plus list_issues()'s null-safe 'body' field, both following the existing dry-run/apply/ boundary-validation idiom. These additions are inert until description_sync.py and sync.py wire them in -- no existing call site is touched. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ion helpers New pure module description_sync.py implements the GH<->AgilePlace description merge core for issue #65: DescriptionResolution (NamedTuple) and resolve_description(), a 3-way merge over each side's own reference point (desc_base for GitHub, desc_ap_written for AgilePlace so a prior truncation never re-registers as an AP-side edit). Warn-and-skip conflict policy: both sides changing to genuinely different values writes nothing and surfaces a warning; both sides independently converging on the same value is not a conflict. Also adds the two canonicalization helpers with their CORRECTED idempotence invariants from the design spike: _canonicalize_gh_body (genuine md->html->md round trip, self-composition-idempotent) and _canonicalize_ap_description (one-directional html->md, idempotent only through a round trip back through HTML -- feeding its own Markdown output back in as HTML is a type mismatch, not a no-op). Pins all 9 hand-traced resolve_description states (3 seeding variants, steady state, truncated-steady-state, GH-only, AP-only, both-changed-conflict, both-changed-independently-converged) plus None/"" normalization and both canonicalization invariants. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…scriptions Replaces the spike's naive per-word shrink loop (O(n^2), 52+s on a 25k-char body) with a binary search over the markdown cut-length against rendered-HTML length: each candidate is snapped to its preceding whitespace boundary and re-rendered once, giving O(log n) renders regardless of body size (GH issue bodies run up to 65,536 chars). Never negative-length slices; a max_length too small even for TRUNCATION_MARKER degrades to a marker-only result rather than looping forever. Pins: under-limit passthrough, over-limit truncation fitting max_length with TRUNCATION_MARKER appended, graceful degeneration on a tiny max_length, and a wall-clock-bounded test on a >=20k-char body so the O(n^2) regression can never silently creep back in. Part of issue #65 (task 3/7). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…vance gate Wires resolve_description's pure merge into GitHub/AgilePlace writes: sync_description(cfg, apply, issue, card, issues_state, queue). Mirrors sync_dates' (issue #6) merge-base contract exactly -- desc_base and desc_ap_written only ever advance together, gated on apply=True AND a confirmed (or unneeded) GitHub-side write, never on the independently-fired AgilePlace queue write. Verified the coupled-gating regression the design step corrected: a naive apply-only gate lets a failed GitHub write silently advance both merge-base fields, which the new tests catch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds the single sync_description(cfg, apply, issue, card, issues_state, queue) call right after sync_dates in sync.main()'s per-issue loop, so GitHub/AgilePlace descriptions actually reconcile on every run. Wiring this in surfaced two real regressions (per the design spike's flagged CRITICAL blast-radius, not deferred): - Every pre-existing card fixture across nine test files (test_sync_main, test_vetting_latch, test_run, test_hierarchy_ownership, test_retired_issues, test_sync_card_coherence, test_sync_contested_cards, test_sync_intake_call_site) that reaches the per-issue loop now carries a 'description' key, keeping agileplace.card_description() on its zero-I/O path instead of falling back to a real, unmocked agileplace.get_card() (confirmed live: a real host answers HTTP 401 -> SystemExit). - sync_description now short-circuits a dry-run-only "_planOnly" card to an empty AgilePlace-side description instead of calling card_description(), matching the same convention sync.py's dependency-sync loop already uses for plan-only cards -- a freshly planned card has no server-side description yet, so reading one would GET an id that was never created. tests/test_description_sync_wiring_fixtures.py pins the fixture-safety invariant directly: none of the three most complex wired test files may let a card fixture reach a real agileplace.api() call. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Task 6/7 of issue #65: a dedicated, thin wiring test (mirroring test_sync_intake_call_site.py's convention of monkeypatching the collaborator rather than duplicating its own behavior tests) asserting main() calls description_sync.sync_description() exactly once per syncable issue with the expected positional args (cfg, apply, issue, card, issues_state, queue), and that the unresolved-card `continue` guard skips it entirely when no card matches this run. Re-verified test_regression_budget.py: sync.py stays at 728 lines (well within the 726 + 40-line wiring budget and the 800-line hard cap), so no re-anchor of PRE_CHANGE_SYNC_LINES was needed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Exercise agileplace.op_description's exact write shape through the same versioned-PATCH path (patch_card) the sync uses, plus a fact-finding probe of the configured ap_description_max_length against the live server, per the issue #65 design doc's smoke-script validation plan. Flags the untested replace-vs-remove uncertainty for clearing a description (design decision #7) in a code comment rather than silently assuming "replace" covers that case too. Updates test_smoke.py's fake AgilePlace tenant to model the /description PATCH path and the exact confirmed-run write sequence, keeping the existing offline smoke suite green. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…full GH body Critical: resolve_description's "AP changed, GH unchanged" branch always promoted ap_canonical straight to `merged` and pushed it to GitHub. When a prior run had truncated an oversized description for the AgilePlace card (desc_ap_written diverging from the full desc_base), any further edit on top of that truncated stub was blindly treated as the whole authoritative body -- silently overwriting the GitHub issue with the short truncated+marker stub and permanently losing the untruncated tail, with conflict=False so no human ever got a chance to intervene. Detect the truncated-stand-in case (ap_written diverges from base) and degrade it to the same warn-and-skip conflict outcome as a genuine both-sides conflict instead of a blind overwrite. Medium: agileplace.py was already over the 800-line file cap on this branch's base; this PR's new card_description()/op_description() helpers pushed it further over. Extracted both into a new agileplace_description.py module (and their tests into tests/test_agileplace_description.py), returning agileplace.py to its pre-PR line count instead of growing an already-over- budget file. Low: the O(log n)-renders truncation test asserted a 5s wall-clock ceiling, which is only a proxy for the render-count invariant the code actually claims -- a coarser-but-still-polynomial regression could still pass under a generous time budget, and a correct implementation could flake on slow CI. Replaced it with a test that counts real calls to richtext.markdown_to_leankit_html and bounds them logarithmically. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…epaired suites The subprocess canary for issue #65's card-fixture regression (a missing 'description' key falling through to a real agileplace.api() call) only covered test_sync_main.py, test_vetting_latch.py, and test_run.py. This same PR also had to add explicit 'description': '' fixtures to test_hierarchy_ownership.py, test_retired_issues.py, test_sync_card_coherence.py, and test_sync_contested_cards.py to dodge the identical regression -- none of which the canary was watching. Verified red: reverting the fixture fix in test_hierarchy_ownership.py reproduces the real HTTP 401 fallthrough and the old canary (3 files) missed it entirely; adding the file to WIRED_TEST_FILES makes the canary fail as expected, and restoring the fixture makes it pass again. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds bidirectional GitHub issue body and AgilePlace card description synchronization with canonical Markdown/HTML conversion, three-way conflict handling, truncation, configurable limits, GitHub body transport, sync-loop integration, and smoke coverage. ChangesDescription contracts and transport
Canonical merge and truncation
Per-issue orchestration and validation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
thewrz
marked this pull request as ready for review
July 23, 2026 05:55
Contributor
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@description_sync.py`:
- Around line 152-184: The binary search in _truncate_for_agileplace is invalid
because rendered HTML length is not monotonic across markdown prefixes. Replace
the search with a correctness-preserving truncation strategy that evaluates
candidate cuts from the longest prefix downward, using
_snap_to_whitespace_boundary and the existing TRUNCATION_MARKER length check,
and returns the first valid candidate without negative slices or marker-only
regressions.
In `@tests/test_description_sync_wiring_fixtures.py`:
- Around line 35-43: Add "tests/test_sync_intake_call_site.py" to the
WIRED_TEST_FILES tuple alongside the other sync fixture paths so the intake
call-site suite is included in the regression coverage.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f1b76f05-d69b-41fa-afb9-aab02ee7e721
📒 Files selected for processing (22)
.env.exampleagileplace_description.pyconfig.pydescription_sync.pyghkit.pysmoke.pysync.pytests/test_agileplace.pytests/test_agileplace_description.pytests/test_config_env.pytests/test_description_sync.pytests/test_description_sync_wiring_fixtures.pytests/test_ghkit_edit_issue_body.pytests/test_hierarchy_ownership.pytests/test_retired_issues.pytests/test_run.pytests/test_smoke.pytests/test_sync_card_coherence.pytests/test_sync_contested_cards.pytests/test_sync_description_call_site.pytests/test_sync_intake_call_site.pytests/test_sync_main.py
…guard tests/test_sync_intake_call_site.py runs sync.main() through the same mocked I/O harness as the other wired suites, so its card fixtures reach sync_description's card_description() call and belong under the same no-real-API regression guard. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
AgilePlace card details drift from the GitHub issue body they were created from — edits to the GH issue body never propagate to the card, so the two diverge silently over time.
What
Adds
description_sync.pywith aresolve_descriptionmerge + canonicalization helper pair, wiressync_descriptioninto the per-issue sync loop insync.py, and adds binary-search-based truncation for descriptions that exceed AgilePlace's card-detail size limit.Design decisions
All 11 pre-spike decisions stand unchanged (verified against spike results — none were contradicted). Additive decisions from folding in the spike:
Truncation algorithm is binary-search-based, not a linear shrink loop (spike finding Retire cards for issues closed as NOT_PLANNED/DUPLICATE instead of freezing them and blocking dependents forever #1, CRITICAL, supersedes the pre-spike design's literal 'shrink at a whitespace boundary, re-render, re-check' loop wording). The loop was O(n^2) and took 52+s on a 25k-char body; GH issue bodies run up to 65,536 chars, which would be worse. Bisecting the markdown cut-length against rendered-HTML length gives O(log n) renders and is fast enough for the largest realistic body. This is a correction to the interfaces section, not merely an implementation detail, because the pre-spike design's prose literally specified the slow algorithm.
Fixture repair (test_sync_main.py, test_vetting_latch.py, test_run.py) is pulled INTO this issue's own task list, not deferred to a follow-up (spike finding Fix blocked-state JSON Patch paths: write flat /isBlocked + /blockReason, not /blockedStatus/* #2, CRITICAL blast-radius). The spec's 'wiring covered by a sync-level test' undersold the actual mechanical size of wiring sync_description in: every pre-existing card fixture reaching the per-issue loop needs either a 'description' key or an explicit get_card mock, or previously-safe tests attempt real HTTP calls and SystemExit. This is a mandatory, non-deferrable part of landing the wiring safely — it is the wiring being correct, not scope creep.
The N+1 get_card-per-managed-issue-per-run production cost (spike finding Fix blocked-state JSON Patch paths: write flat /isBlocked + /blockReason, not /blockedStatus/* #2's second consequence) is flagged here as a known operational cost of this design, but is NOT addressed in this issue — list_cards() genuinely never returns description (verified: no field-selection params sent), so there is no cheaper option available without a separate API-shape change, which is out of scope here per the spec's own 'one helper hides the choice from wiring' framing (the helper already picks the only working path).
The AP-side canonicalization idempotence test is rewritten as a round-trip-through-HTML property (spike finding Switch tag removal to index-based JSON Patch ops (value-based remove is undocumented and contradicted) #3, MEDIUM), never a literal f(f(x))==f(x) property test against arbitrary input, which would spuriously fail because _canonicalize_ap_description is one-directional (html-to-md only) and does not compose with itself.
Every test path touching sync_description (or card_description's fallback branch) explicitly mocks agileplace.get_card — never relies on an absent cfg['token'] to make api() silently no-op, since api() hard-SystemExits when called without a token in offline/test contexts (spike finding Read existing card children via the connections/children endpoint (include=childCards is not a documented card-list param) #4, confirmed concretely, consistent with the spec's own stated propagation contract but now backed by direct evidence).
Testing
smoke.pyagainst a real AgilePlace board with a long-body issue🤖 Co-authored by Claude Sonnet 5. Closes #65
Summary by CodeRabbit
New Features
Bug Fixes