fix: switch tag removal to index-based JSON Patch ops#30
Merged
Conversation
Replace the undocumented value-based tag removal
({"op":"remove","path":"/tags","value":tag}) with standard RFC-6902
index-based removal. No public LeanKit docs describe value-based
removal, and RFC 6902's remove op has no `value` member -- only an
index-based path. Indices are computed from the card's raw tags
array and removed in descending order within the same batched PATCH
so earlier removals never shift later ops' target indices.
op_tag is now add-only (its `add` kwarg is dropped, not kept as dead
API surface); a new ops_tag_remove builds the index-based remove ops
and raises loudly if a name isn't found on the card, since that can
only mean an upstream invariant broke. sync.py's tag-ops assembly now
accumulates removal names into one set and defers to ops_tag_remove
for the batch, instead of building remove ops per-name inline.
API-VALIDATION.md's tag-removal entry is corrected: the index-based
form is now documented as the implemented behavior, and the earlier
unsupported "LeanKit's documented value-based removal" citation is
removed. A live add-then-remove round-trip on a disposable card still
needs a human to run it -- not attempted here (no live AgilePlace
credentials available/authorized for this task).
Closes #3
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds three tests targeting sync_metadata's tag-ops assembly block (issue #3): the card input is never mutated (both an add and a remove op fire in the same pass without writing back into card), the tags_to_remove accumulator is a fresh local set on every call (a removal computed for one issue's card cannot leak into the next call's queued ops), and a pass needing both a tag add and a tag remove combines them into exactly one queue() call so they land in a single versioned PATCH per card instead of being fragmented across two calls. Each test was confirmed to fail for the right reason against a deliberately reintroduced regression (in-place tags mutation; a split queue() call) before being left green against the current implementation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds a regression test asserting op_tag("foo", add=False) raises
TypeError. Without it, reintroducing a def op_tag(tag, add=True) with
a value-based remove branch would default every existing call site
(sync.py's two callers and the existing append-op test) to add=True
and pass silently, resurrecting the undocumented value-based
{"op":"remove","path":"/tags","value":tag} shape issue #3 removed.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
get_card() only checked for None before calling data.get("card", data),
so any non-dict, non-null AgilePlace GET response (a bare list, string,
number, or bool) crashed with an opaque AttributeError instead of the
intended SystemExit. Since the single-card GET's exact shape is
unconfirmed (VALIDATE LIVE), this was a plausible live failure mode
reached from every PATCH via _card_with_version's refetch.
Validate both the top-level response and the unwrapped card value are
dicts, failing loud with the card id and observed type otherwise.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughChangesThe patch validates AgilePlace card responses, switches tag removals to descending RFC-6902 index operations, protects against stale tag snapshots, batches sync updates, and expands documentation and tests. Tag removal and validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant sync_metadata
participant agileplace_ops
participant AgilePlace
sync_metadata->>agileplace_ops: Build tag additions and removals
agileplace_ops->>agileplace_ops: Resolve current tag indices descending
sync_metadata->>AgilePlace: Queue one combined tag patch
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
Contributor
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Index-based tag removes (ops_tag_remove) encode positions in a specific
tags snapshot. patch_card's version-refetch path (_card_with_version)
fills in a fresh resource version for a version-less card but keeps the
original tags snapshot, so pairing stale /tags/{i} ops with a version the
server may have bumped since would pass optimistic concurrency yet delete
the WRONG tag if the tags array shifted meanwhile.
Guard the refetch path: when the batch carries index-based tag removes and
the refetched tags differ from the snapshot, refuse the PATCH with a WARN
(fail closed, matching the module's ambiguity philosophy) rather than risk
a silent mis-deletion. Append-only batches and value-carrying ops are
position-independent and unaffected.
Found by Codex (GPT-5.5) adversarial review of PR #30.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
Codex (GPT-5.5) adversarial review — additional eyesRan Codex as a second reviewer alongside CodeRabbit. One confirmed [P1] finding, now fixed: [P1] Stale tag indices paired with a fresh resource version ( Fix: guard the refetch path in The fix lives entirely in 🤖 Codex finding triaged and fixed by Claude Opus 4.8. |
Contributor
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
thewrz
marked this pull request as ready for review
July 19, 2026 06:39
thewrz
added a commit
that referenced
this pull request
Jul 19, 2026
…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>
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's JSON Patch API for card tag removal was operating on tag values rather than array indices, and the write path carried a dead
addkwarg that no caller used now that removal has its own function. This left the removal path fragile against duplicate tag values and carried unused API surface.What
addkwarg entirely fromop_tag— removal now has its own dedicated function, so the combined kwarg was dead surface.removeops instead of value-based ones, removing ALL occurrences of a duplicated tag value rather than only the first.ValueErrorinstead of silently no-oping: tracing the data flow confirms bothr.ap_removeandstaleare subsets ofcard_tags(card)for the same card snapshot, so a miss means a real bug (stale snapshot, race, etc.), not a legitimate idempotent replay.ops_tag_removeis now called unconditionally (it no-ops safely on empty input), simplifying the call site.get_cardagainst non-dict JSON responses from AgilePlace.Design decisions
Followed the pre-supplied design essentially as-is: dropped the
addkwarg entirely (dead API surface once remove has its own function); fail-loudValueErroron a tag miss (traced data flow confirms bothr.ap_removeandstaleare subsets ofcard_tags(card)for the same card snapshot, so a miss signals a real bug, not idempotent replay); remove ALL occurrences of a duplicated tag value; unconditional call toops_tag_removesince it no-ops on empty input.One deviation found during implementation:
tests/test_ghkit_labels.pyhad 3 pre-existing tests hardcoded to the old value-based op shape (path=='/tags', value=tag) — these needed fixing (not anticipated in the todos, which only mentionedtest_agileplace.py). Added a_removed_tag_namestest helper there to map index-based ops back to tag names for assertions; one of the three was a genuine behavioral check that failed until fixed, not merely cosmetic.Testing
🤖 Co-authored by Claude Sonnet 5. Closes #3
Summary by CodeRabbit
Bug Fixes
Documentation
{op:"remove", path:"/tags/{i}"}semantics and ordering rules.Tests