Repository navigation
fix(hitl): no URLs or machine names in the decision card's body (V36) - #6281
liususan091219 wants to merge 3 commits into
Conversation
The owner called the card 「非常 raw」 and 「像在读 log」. Two of the things she named are produced here: - projector.fallback_body() appended " (on <host> · <seat>)" to every card headline. The seat still rides the wire device, where a client that wants it can read it; it is just not what she decides from. - pending_questions_ask.card_message() appended "Or set its Status on the row: <url>". The card carries the buttons, so the raw URL bought nothing; the text message still carries the link. Commit hashes, the third thing she named, come from neither: callers write them into the question and context. That needs a rule on the asking side and is not in this PR. Both tests that pinned the removed text are replaced rather than deleted, and the projector one now also asserts the seat is still on the wire device, so the information is shown to have moved rather than been dropped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ByiYdfob1ZYnq5PHYxo8qb
|
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
left a comment
There was a problem hiding this comment.
Requesting changes at e78111a, for one thing that should fail CI and that I confirmed in the source (I did not run the suite).
src/hitl/projector.py changed but its vendored copy did not. packages/ag2-sparrow/ag2_sparrow/hitl/projector.py is mapped in packages/ag2-sparrow/tools/sync_from_src.py, and tests/ag2-sparrow-hitl-vendored.test.py::test_every_hitl_module_is_vendored_byte_identical asserts the two files are byte-identical. On this head the vendored fallback_body still builds where = f" (on {host} · {name})", so that test should now fail, and wherever the sparrow package is what renders the plain-text body, the machine name stays on the card. Please run tools/sync_from_src.py and commit the vendored file. (The ag2-sparrow publish gate and the vendored/drift tests were still pending when I looked.)
The rest checks out against the description:
- The
card_messagechange drops only the "Or set its Status on the row: " line; the text message path still buildsReply here, or set its Status on the row: <link>(line ~346), so the row link is still reachable for anyone who wants it. fallback_bodykeeps the seat on the wire device, and the replaced tests assert both that it is gone from the body and still present atwire["device"]["name"].- Each removal has its own test, and the two old tests were replaced, not deleted.
Worth the owner's eye, not blocking: with several seats sharing one owner (this host and another), the card body no longer says which one is asking. That is the V36 decision and the seat is on the wire, but it only helps if the client renders device; I did not check the web client for that. The PR itself notes commit hashes and the card hierarchy are separate follow-ups.
…tail (V36/V111)
Second half of V36, per the owner's order (V111: 一句话 + 选项按钮,细节收起来).
Order: the ask, then the options she acts on, then why (only with a proposed
default), then background last. Context used to sit between the question and
the buttons, so she read background before her own choices.
Prose: card_plain() takes URLs, commit hashes and the host's name out of the
question, reason and context. The word that introduced the detail goes with it
("green on <sha>" -> "green"), and a clause left too short to say anything is
dropped, so removal cannot leave "— built".
Text with nothing to strip is returned unchanged, byte for byte. That is the
control worth keeping: an earlier cut rewrote ';' to ',' and merged sentences,
which is worse than the detail it removed. A hex-letter word ("facade") and a
plain number ("19700000") are not hashes; a hash needs both letters and digits.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ByiYdfob1ZYnq5PHYxo8qb
sonichi
left a comment
There was a problem hiding this comment.
Re-reviewed at c481d943 (one commit over e78111ad, the second half of V36: card order and card_plain). Comment only; my earlier changes-requested stands. Read from the diff and files, nothing run.
Still open from last round. src/hitl/projector.py is still changed without its vendored copy: the file hashes on this head differ (src vs packages/ag2-sparrow/ag2_sparrow/hitl/projector.py), so tests/ag2-sparrow-hitl-vendored.test.py::test_every_hitl_module_is_vendored_byte_identical should still fail. Please run tools/sync_from_src.py and commit it.
New in this commit. The card is reordered to ask → options → why → context, which is a plain improvement and matches V111. card_plain strips URLs, 7-40 character hex tokens that mix digits and letters, and the host name from the question, the reason and the context. It is careful in the ways the message says (text with nothing to strip comes back byte for byte; a bare hex word or number is not a hash).
One concern with it, because this card is an approval surface:
- The stripping is applied to the question as well as the background, but the buttons' labels and consequence lines (
card_choices,card_consequences) are not touched. When the decision is about a specific commit or link ("Merge 9ef5fef?", "Open https://… for me?"), the owner now reads "Merge?" or "Open for me?" above buttons that still name the thing, or none of them do. A card should not make someone approve an action whose target it has hidden. - Suggestion: strip only from the context and reason (the background), and leave the question and the option text as the asker wrote them; the rule for asking sides ("no hashes or URLs in the question") can live in the ask-side guidance, which the PR already says is the place for it.
- Smaller: dropping a clause once it is under two words can delete a short but meaningful clause ("No." after a removal), and the tests added (43 lines) cover the happy path and the byte-identical control; I did not see a test where the stripped item was the subject of the question.
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
|
@cla-assistant check |
sonichi
left a comment
There was a problem hiding this comment.
Re-reviewed at ffda8e85. Comment only. Read from the diff and files, nothing run.
Resolved: the vendored copy. The new commit mirrors the projector change into packages/ag2-sparrow/ag2_sparrow/hitl/projector.py; the two files have the same content hash on this head, so the byte-identical vendored test should pass. That was my changes-requested reason, so I consider it cleared.
Still open, from my last comment: card_plain strips URLs and commit hashes from the question while the option labels and consequence lines keep theirs, so a decision about a specific commit or link can read "Merge?" above buttons that name it, or none of them do. Please strip only the reason and background, or confirm that asking-side text will never name the thing being decided. I have not approved over this; it is the one thing between this PR and an approval from me.
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 93%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
skills/pending-questions/scripts/pending_questions_ask.pyLines 420-428 420 # of a clause once it is too short to say anything.
421 kept, tail = [], ""
422 for piece in re.split(r"([,;\u2014]|\.\s|\.$)", out):
423 if piece is None:
! 424 continue
425 if re.fullmatch(r"[,;\u2014]|\.\s|\.", piece or ""):
426 tail = "." if piece.strip() == "." or piece.strip().startswith(".") else piece
427 continue
428 if len(piece.split()) >= 2: |
What the owner said
The decision card reads 「非常 raw」 and 「像在读 log」. She named four things: full URLs, machine names ("on qingyun-mac-mini"), commit hashes, and "Or set its Status on the row".
What this PR removes
Two of the four are produced here, and both come out:
projector.fallback_body()appended" (on <host> · <seat>)"to every card headline. That is the machine name. The seat still rides the wire device, so a client that wants it can read it — it just is not what she decides from.pending_questions_ask.card_message()appended"Or set its Status on the row: <url>". That is both the full URL and the "Or set its Status" line. The card already carries the buttons, so the raw URL bought nothing; the text message still carries the link for anyone who wants the row.What this PR does not do, and why
Commit hashes come from neither function — callers write them into the question and context text. Removing them needs a rule on the asking side, not the formatter, or the card goes raw again from the caller. That is worth doing and is not in this PR.
"A clear hierarchy" is the remaining half of her ask. I have not guessed at it: the current order is question → context → "Why Approve" → options, and I would rather show her a concrete before/after than invent a structure.
V44 (one status after resolve) is deliberately separate. Of the three post-resolve statuses she counted, two are text here and the third is the green card rendered web-side by another agent — removing all three would leave zero, so that one is waiting on coordination.
Regression evidence
Regression test: tests/hitl-projector.test.py
Before fix: FAIL — restoring the machine-name suffix fails
test_the_plain_text_names_no_machineandtest_create_then_edit; restoring the URL line failstest_the_card_does_not_carry_the_row_link. Run:python3 tests/hitl-projector.test.pyandpython3 tests/pending-questions-decision-card.test.pyAfter fix: PASS — both suites green. Same commands.
Each removal was reverted independently to confirm its test actually fails. Both tests that pinned the old text are replaced rather than deleted, and the projector one now additionally asserts
wire["device"]["name"]still carries the seat — so the change is shown to have moved the information rather than dropped it.🤖 Generated with Claude Code
https://claude.ai/code/session_01ByiYdfob1ZYnq5PHYxo8qb