fix(sync): fail closed on zero recognized Project statuses#31
Conversation
…card lanes too
A configured Project whose item-list read technically succeeds but yields zero
recognized statuses (misspelled GH_PROJECT_STATUS_FIELD, or a gh output-shape
change) previously slipped past the fail-closed gate: project_status came back
{} with move_lanes still True, so every issue fell back to label/PR-derived
stages -- the exact mass-move the gate exists to prevent, just reached through
a technically-successful call. Newly created cards were also unconditionally
laned from that same fallback even when the read had outright failed, since
only the existing-card loop respected project_read_failed.
Widen project_read_failed to also trip when the Project has issue-linked items
but none carry a recognized status, with a WARN naming the configured status
field. Gate the new-card creation loop on the same flag so cards created during
a failed read are laneless rather than fallback-laned. README now documents
both the widened gate and laneless creation.
Closes #5
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…tive guard Closes review findings for issue #5: - test_new_card_gets_no_lane_when_project_read_outright_fails now asserts the byte-for-byte outright-failure WARN string is actually printed on the (None, None) call-failed path, not just that lane assignment is skipped. - New test_zero_issue_linked_items_does_not_trip_zero_status_warn pins the invariant that a Project with zero issue-linked items (project_items == {}, e.g. all rows are drafts/PRs) must never trip the zero-status WARN and must leave move_lanes=True, guarding the `bool(project_items) and` conjunct in sync.main() against regression. Verified both tests fail against the mutations described in the findings (collapsing the WARN branches; dropping the bool(project_items) conjunct) and pass against the current, already-correct sync.py -- these were test-coverage gaps, not production bugs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
gh project item-list populates content.url for a linked Pull Request just like it does for an Issue, so parse_items's URL-only check was wrongly counting PR rows as issue-linked items. That inflated project_items/ project_status in sync.py, corrupting zero_status_despite_items and the fail-closed lane-move gate it feeds (issue #5's mass-move failure mode, reachable through a different door). Exclude content.type in {PullRequest, DraftIssue} explicitly; a missing type key still defaults to Issue for backward compatibility with fixtures/gh paths that omit it. Also strengthens test_sync_main.py's project_read_failed lane-gating tests: they previously passed only because the mocked board_layout returned [], so resolve_lane_for_stage produced None regardless of whether the `if not project_read_failed` / `if move_lanes` gates were present. New tests mock resolve_lane_for_stage to return a real lane and assert it is never even called when the Project read failed, directly pinning both gates instead of relying on an empty-lanes coincidence. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughProjects v2 parsing now excludes non-issue content types. Synchronization distinguishes failed and zero-recognized-status reads, blocks lane changes in those cases, and creates new cards without lanes when assignment is deferred. Tests cover parsing, orchestration, warnings, and card writes. ChangesProjects v2 synchronization
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant sync_main
participant GitHubProjectsV2
participant resolve_lane_for_stage
participant agileplace_create_card
sync_main->>GitHubProjectsV2: read project items and statuses
GitHubProjectsV2-->>sync_main: statuses or failed/empty result
sync_main->>resolve_lane_for_stage: resolve lane only after a usable read
resolve_lane_for_stage-->>sync_main: lane
sync_main->>agileplace_create_card: create card with lane or deferred assignment
Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/test_sync_main.py`:
- Line 150: Remove unused mock bindings from the _run_main_once unpacking: in
tests/test_sync_main.py lines 150-150, replace patch_card_mock with _, and at
lines 187-187 replace the unused state and run_mock bindings with _. Keep all
used return values unchanged.
🪄 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: bd21e685-78de-471b-8b17-d1b30b4a84db
📒 Files selected for processing (5)
README.mdghproject.pysync.pytests/test_sync.pytests/test_sync_main.py
CodeRabbit nitpick: drop the never-read patch_card_mock binding in the first-rollout test and the unused state/run_mock bindings in the zero-recognized-statuses test (Ruff RUF059). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@coderabbitai full review |
✅ Action performedFull review finished. You're currently rate limited under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. Your next review will be available in 36 minutes. |
…cts) Both this branch and main (via merged PRs #30/#31) extended the same test_sync_main.py helpers and appended new test sections. Resolution keeps this branch's superset _mock_io/_run_main_once signatures (open_pr_return, lanes_return, card, existing_cards with _UNSET sentinels), main's underscore-bound unused unpacks (RUF059), and both branches' test sections. create_card mock returns {} per main's convention so freshly created cards are not re-processed by the step-2 lane pass in these wiring tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Why
When the GitHub Projects v2 read comes back with items but none of them resolve to a recognized Status value, the sync previously proceeded silently -- treating "we couldn't read the board's Status field" the same as "the board legitimately has no matching lanes." That masks a misconfigured
status_fieldname and can cause cards to be created without the correct lane, with no operator signal that anything went wrong.What
Sync now fails closed when a project has items but zero of them carry a recognized Status: it logs a distinct WARN naming
cfg['gh_project']['status_field']and skips lane resolution for new cards in that pass (falling back tolane_id=None, an already-supported path increate_card) rather than guessing. The pre-existing outright-failure WARN (project unreadable at all) is left byte-for-byte unchanged since a log consumer may key on it. Also excluded Pull Request rows from Projects v2 item parsing so PR rows never masquerade as issue items during this check.Design decisions
Implemented per the pre-spike design with no deviations found necessary during the real build (spike was judged unnecessary and skipped, and nothing surfaced during implementation that contradicted it). Confirmed decisions as executed:
zero_status_despite_itemsis keyed offproject_items(parsed, URL-keyed) rather thanraw_items, since raw_items can hold draft/PR rows lackingcontent.urlthat never populateproject_items-- a project with only such rows must not trip the new WARN.main()rather than extracting aproject_read_gate()helper -- ~10 lines across two existing blocks, single call site, no abstraction to justify.cfg['gh_project']['status_field']for the zero-status case, since the two failure modes have different operator remediations.stageis still computed unconditionally (informational/log-only during a failed read) butlaneis only resolved viaresolve_lane_for_stagewhennot project_read_failed; otherwiselane=Noneandcreate_card's already-supportedlane_id=Nonepath fires (no new deferred-creation queue/state).Testing
python3 -m pytest -q-- 184 passed)status_fieldand confirm the new WARN fires and cards fall back tolane_id=None🤖 Co-authored by Claude Sonnet 5. Closes #5
Summary by CodeRabbit
Bug Fixes
Documentation