Skip to content

fix(hitl): a resolved card adds no second "Resolved" line (V44, part 1) - #6283

Open
liususan091219 wants to merge 2 commits into
mainfrom
fix/v44-one-status-after-resolve
Open

liususan091219 wants to merge 2 commits into
mainfrom
fix/v44-one-status-after-resolve

Conversation

@liususan091219

Copy link
Copy Markdown
Collaborator

What

The owner counted three statuses for one resolved card: the green card, a "Resolved" text line beside it, and a separate "✓ Cleared" message. This removes the first of the two text ones.

Only STATUS_RESOLVED goes from _STATUS_LINES. Expired and cancelled stay — no colour conveys those. A cleared sign-in keeps ✓ Signed in, which says something the colour does not.

Why "✓ Cleared" is not in this PR

I was told to delete both text lines and I am deliberately delivering one, because the second is not what it looks like. renotify's ✓ Cleared is not only a status line:

  • it is the worker-seat recovery — "worker <seat> … resuming N queued tasks" — batched one per seat across all that seat's cards (test_one_recovery_per_seat, test_many_cards_for_one_seat_resolved_at_once_send_one_recovery);
  • a resolved record is held live until its recovery is sent, so removing it also changes when records archive (test_recovery_owed_beside_an_open_sibling_stays).

Gating it to sign-ins only breaks 6 tests across two suites, and the breakages are not cosmetic — they encode that batching and that lifecycle. Deleting it would silently drop "worker X resumed, 3 tasks continuing", which is a different fact on a different surface from the decision card the owner was looking at.

That is a decision about seat recoveries, not card copy, so I am asking rather than assuming. If the answer is "drop them too", it is a small follow-up on this branch.

Regression evidence

Regression test: tests/hitl-projector.test.py

Before fix: FAIL — restoring STATUS_RESOLVED to _STATUS_LINES fails test_a_cleared_login_still_reads_signed_in_and_nothing_else_adds_a_line. Run: python3 tests/hitl-projector.test.py

After fix: PASS — 66/66, and tests/hitl-renotify.test.py is untouched and green. Same command.

The replaced test asserted both the sign-in line and the generic "Resolved" line; it now asserts the sign-in line survives and the generic one does not, plus a new test pinning that expired/cancelled still say so — so the narrowing is bounded by tests on both sides.

🤖 Generated with Claude Code

https://claude.ai/code/session_01ByiYdfob1ZYnq5PHYxo8qb

The owner counted three statuses for one resolved card: the green card, a
"Resolved" text line beside it, and a separate "✓ Cleared" message. This drops
the first of the two text ones.

Only the resolved line goes. Expired and cancelled stay, because no colour says
those, and a cleared sign-in keeps "✓ Signed in", which says something the
colour does not.

The "✓ Cleared" message is NOT in this commit. It is not only a status line:
renotify owes one recovery per worker seat, batched across that seat's cards,
and a record is held live until its recovery is sent. Removing it also removes
"worker <seat> resumed, N queued tasks" and changes when records archive. That
is a decision about seat recoveries, not card copy, so it is asked rather than
assumed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ByiYdfob1ZYnq5PHYxo8qb
@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Susan Xueqing Liu seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@sonichi sonichi left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes at 9ef5fef, for two things. I read the source and ran nothing.

1. The vendored copy was not synced. src/hitl/projector.py changed, but packages/ag2-sparrow/ag2_sparrow/hitl/projector.py (mapped in tools/sync_from_src.py) still has STATUS_RESOLVED: "✓ Resolved — Sutando has continued its work." at line 68. tests/ag2-sparrow-hitl-vendored.test.py::test_every_hitl_module_is_vendored_byte_identical asserts the two files are byte-identical, so it should fail on this head. Run tools/sync_from_src.py and commit the vendored file. Note #6281 edits the same file, so whichever lands second needs to rebase and re-sync.

2. The line removed is the only status for the clients that cannot draw the green card. fallback_body is documented as the plain-text rendering "for clients that ignore the hitl content field". For those clients the card colour does not exist, so after this change a resolved card's text body is its original headline with nothing added: no word that it is done. The owner's complaint was three statuses on a client that shows the green card, the text line and the separate "Cleared" message; removing the text line there is right, but the PR removes it for everyone. Either keep the line in fallback_body and have the card-rendering path drop it (the content field already says it is resolved), or state that the fallback clients are gone. As it stands, a resolved card on a plain-text client reads as still waiting.

Worth saying, and good: the description is candid about why "✓ Cleared" stays (it carries the seat recovery and the batching tests), that is the right call, and the two bounding tests (sign-in keeps its line; expired and cancelled keep theirs) are well chosen. The open question on seat recoveries is the owner's, not mine.

projector.py is vendored byte-for-byte into packages/ag2-sparrow, and
tests/ag2-sparrow-hitl-vendored.test.py enforces that. CI caught the drift:
out-of-tree tests plus four standalone shards.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ByiYdfob1ZYnq5PHYxo8qb
liususan091219 pushed a commit that referenced this pull request Nov 7, 2026
Same latent break as #6283: projector.py is vendored byte-for-byte into
packages/ag2-sparrow and a test enforces it. Caught here before CI reached it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ByiYdfob1ZYnq5PHYxo8qb

@sonichi sonichi left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed at b7e3d18f. Comment only; my changes-requested stands on the second point. Read from the diff and files, nothing run.

Resolved: the vendored copy. The new commit mirrors the change into packages/ag2-sparrow/ag2_sparrow/hitl/projector.py; the two files now have the same content hash on this head, so the byte-identical vendored test should pass.

Still open: the plain-text fallback. fallback_body is the rendering "for clients that ignore the hitl content field", and for those clients the green card does not exist. With the "✓ Resolved" line gone, a resolved card's text is its original headline with nothing added, which reads as still waiting. The same goes for the expired and cancelled cases only in reverse (those keep their lines, so the asymmetry is visible: only "resolved" goes silent). Either keep the line in fallback_body and drop it on the card-rendering path, or say in the PR that those clients are no longer supported. I have not seen either in this head.

@github-actions

github-actions Bot commented Nov 7, 2026

Copy link
Copy Markdown
Contributor

@cla-assistant check

@github-actions

github-actions Bot commented Nov 7, 2026

Copy link
Copy Markdown
Contributor

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 93%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

No lines with coverage information in this diff.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants