Repository navigation
fix(task-expand): preserve userCollapsed on expand:N (so collapse→expand-N actually isolates) - #683
Merged
Merged
Conversation
…lapse-all) Sequence "collapse all → expand task N" was broken: collapse-all set userCollapsed=true and cleared the set. expand:N then added the target but reset userCollapsed=false. On the next 3s API poll, the auto-expand block at line ~1212 saw userCollapsed=false and re-added all working tasks → "expand:N" looked like a no-op because everything ended up expanded again. Fix: don't touch userCollapsed in the per-task expand path. If the user just collapsed all, that intent stays sticky; expand:N adds ONLY the target. If userCollapsed is false (no prior collapse), auto-expand keeps working as before.
liususan091219
pushed a commit
to liususan091219/sutando-fork
that referenced
this pull request
May 28, 2026
…lapse-all) (sonichi#683) Sequence "collapse all → expand task N" was broken: collapse-all set userCollapsed=true and cleared the set. expand:N then added the target but reset userCollapsed=false. On the next 3s API poll, the auto-expand block at line ~1212 saw userCollapsed=false and re-added all working tasks → "expand:N" looked like a no-op because everything ended up expanded again. Fix: don't touch userCollapsed in the per-task expand path. If the user just collapsed all, that intent stays sticky; expand:N adds ONLY the target. If userCollapsed is false (no prior collapse), auto-expand keeps working as before.
john-the-dev
pushed a commit
that referenced
this pull request
Aug 19, 2026
* fix(sparrow): reconcile takes the DeliveryAttempt, not bare ids The (item_id, key) signature could not express reconciliation-by-safe-resend: a keyed-dedup provider whose only receipt store IS the send needs the payload inside reconcile. Field-hit on the AG2 Space provider work and canonized in the cloud delivery doc (ag2space-backend#687 amendment 1, issue #683). DeliveryAttempt(item_id, payload, idempotency_key) is frozen and core-built, so the derived-key invariant rides inside it — the enforcement scan now looks through the constructor (mutation-tested: a raw key inside DeliveryAttempt goes red). New contract case pins the previously inexpressible shape and that both offers share one dedup identity. Providers in open PRs (#3110, #3095 chain) implement reconcile or omit it capability-gated; branch adaptations ride their rebases. Signed: @sutando-qingyun-001:ag2.space Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(delivery-core): classify reconcile's typed failures, or the claim leaks Review finding (@keweichen), reproduced before fixing. This PR licenses reconcile() to perform a safe re-send — so it can raise the same typed failures as deliver(). The call was unclassified, so ProviderIndeterminate or ProviderRefused escaped deliver_one BEFORE backend.complete(): the claim stayed owned and retry accounting never moved. ProviderIndeterminate escaped=ProviderIndeterminate successor_blocked=True attempts=0 ProviderRefused escaped=ProviderRefused successor_blocked=True attempts=0 _reconcile() applies the same taxonomy _attempt() does: a second ambiguity reaches OUTCOME_UNKNOWN (park — ambiguity is not relabelled), a definite refusal reaches NOT_DELIVERED (retry accounting), and anything untyped still propagates loudly rather than being laundered into a delivery outcome. It takes (item_id, payload, key) and builds the DeliveryAttempt itself rather than receiving one. That is not stylistic: delivery-core-enforcement's static scan requires every deliver/reconcile call site to show a key derived from idempotency_key(), and passing a pre-built attempt hides it — the guard caught the first shape, correctly. Three cases, mutation-checked against the pre-fix call: both typed cases ERROR with the exception escaping, exactly as reported; the untyped case pins that a KeyError still propagates. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(delivery): converge every reconcile on the DeliveryAttempt contract keweichen's cumulative-main blocker, verified before fixing. This topic branched at 75fd2ec; main has since gained two implementations of the OLD `reconcile(item_id, key)` interface while this branch's contract requires `reconcile(attempt)`: src/discord_delivery_provider.py:55 (self, item_id, idempotency_key) tests/proactive-dm-failure-keeps-file-behaviour.test.py:129 (self, item_id, key) Merged current main and converted both. The tree now has zero bare-id reconciles. Why a Protocol check could not catch it: `isinstance(obj, DeliveryProvider)` is True regardless of signature, so the mismatch surfaces only as a TypeError at call time — after the claim is held and before backend.complete(), wedging the item. It is also invisible to a topic's own CI, because the stale adapter lands on main while the contract change sits on the branch. Two guards, because converting these two sites does not stop the third: - tests/delivery-core-enforcement.test.py gains a tree-wide AST scan: every `reconcile` under src/, packages/ and tests/ must take (self, attempt). It asserts it scanned more than one definition, so a narrowed scanner cannot pass vacuously. Mutating the provider back names the exact file and line. - tests/discord-delivery-provider.test.py calls `reconcile` directly with a real DeliveryAttempt — positionally and by keyword, so the parameter NAME is pinned too — and asserts the stale two-arg call raises TypeError. Suites green: delivery-core-contract, delivery-core-enforcement (15), discord-delivery-provider, sparrow-result-guard, proactive-dm-failure-keeps-file-behaviour. review-checks PASS. Stand: Echo Act IV Mini * fix(delivery-core): a refused reconcile must not relabel the original send keweichen and yixuan independently found this at 4da48ab; yixuan withdrew an earlier APPROVED because it had missed it. Both are right, and the defect is mine: `_reconcile` mapped ProviderRefused -> NOT_DELIVERED, and deliver_one overwrote OUTCOME_UNKNOWN with it unconditionally. yixuan named the mechanism exactly: two questions were being answered with one value — "how should this reconcile call be classified" and "may that classification replace the prior outcome". The first was answered correctly; the second was never asked. A refusal proves the SECOND call never dispatched. It is not evidence about a first send that may already have crossed the side-effect boundary, which is what docs/sparrow-v1-contract.md:61-62 forbids relabeling. Fix: a raise from reconcile resolves NOTHING, so the caller keeps the outcome it had. Only a reconciliation RECEIPT is a positive statement about the original attempt, and only a receipt may replace OUTCOME_UNKNOWN. keweichen's end-to-end control, real DesignA backend, reconcile_capable=True and idempotent_send=False, first send crossing the boundary then refused reconcile: before first=not_delivered attempts=1 READY second=confirmed effects=2 after first=outcome_unknown attempts=- PARKED second=not_claimed effects=1 Two deliveries became one. That control is now a permanent case, not a probe. Tests: the case that pinned the unsafe behaviour asserted NOT_DELIVERED — my own test locking in my own bug. Replaced with the pair that distinguishes the two situations (refusal must NOT resolve; an explicit NOT_DELIVERED receipt MAY), plus the E2E above. Restoring the old classification fails 2 of 38. Stand: Echo Act IV Mini --------- Co-authored-by: qingyun-wu (sutando-001) <qingyun@ag2.ai> 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
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.
Summary
After "collapse all → expand task N", the user expected ONLY task N to be visible. Actual behavior: all working tasks visible (collapse undone within 3s).
Root cause: my handler at
src/web-client.ts:1002(introduced in #674) setuserCollapsed = falseafter adding the target. The auto-expand block at line 1212 readsuserCollapsedand re-adds all working tasks every 3s API poll. Setting false → auto-expand wins → other tasks come back.Fix
Don't touch
userCollapsedin expand:N. Let collapse-all's sticky intent ride.true→ auto-expand suppressed → only the target shows.false→ auto-expand keeps working as before.Test plan
🤖 Generated with Claude Code