fix: approve listed pairing requests - #46584
Conversation
|
Non-blocking coverage suggestion: I would still prefer one focused CLI regression test that exercises the actual CLI path: generate a pending request, assert that |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for addressing the listed-request approval path. The premise is confirmed on current main: gateway/pairing.py:453 creates a separate pending entry id, while gateway/pairing.py:562-565 exposes only a hash prefix that hermes_cli/pairing.py:69 and hermes_cli/web_server.py:11213 submit to full-code verification.
Problems
- The new CLI routing is not directly regression-tested. A current-tree search found no test for
pairing_command,_cmd_list, or_cmd_approve; this matches the non-blocking suggestion in the existing review comment.
Suggested changes
- Add a CLI list → approve test that uses the displayed request id, and keep an assertion that bot-delivered code approval still works.
Automated hermes-sweeper review.
| platform = platform.lower().strip() | ||
| code = code.upper().strip() | ||
| code = code.strip() |
There was a problem hiding this comment.
Please add a focused CLI-path regression test for this dispatch: generate a pending request, capture the request id printed by _cmd_list, then pass it to _cmd_approve and assert approval. The existing store/API tests do not exercise this branch.
5cdb2c9 to
f25d101
Compare
f25d101 to
6711b46
Compare
|
Reviewed the latest update: the new CLI regression test covers the requested list-to-approve dispatch and keeps bot-delivered code approval covered; required CI is green. I cannot submit an approval review because this account does not have explicit repository review access, so leaving this confirmation here for the author/maintainer to resolve the thread. |
|
Your request-id direction is the right fix and it's cherry-picked as the base of #74427 — authorship preserved in git history. Consolidated there with #62145 and #66761, which hit the same defect from other angles. Thanks for catching this one — the hash prefix going out under a |
Summary
Validation
Refs #46580