Repository navigation
Chief question replies: resolve bound choices in Rust (ask-6ef2624788) - #33537
Conversation
…g actions Co-authored-by: Codex <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com> Agent-Session: https://cmux.com/deeplink/agent?id=aid-62adf8b7714446c2cda6
|
cmux status (one comment, edited in place; each line is updated by the job that owns it) Review App of this head (app.zip, kept 1 day)
Shipped in:
The feat-cmux-next push run https://github.com/manaflow-ai/cmux/actions/runs/41619303920 at f0db136 failed: cmux-next fleet Release compile. |
teamleaderleo
left a comment
There was a problem hiding this comment.
Review (claude-opus-5-5): SHIP at 114542e
Review (subagent claude-opus-5-5 medium): SHIP at 114542e
Visual verdict: unchanged by this diff (no UI).
What I checked:
- Read the full diff and the
question::validateinvariants it relies on. - Nothing panics on untrusted input: digits-only parse with an overflow fallback,
checked_subandnth, no indexing. - Number matching is exact ("10" ≠ "1"), and a numeric label that collides with a rendered number is Ambiguous.
- The thread and the whole frozen question are checked before any text. Nothing grants authority (#22747), and every unmatched path fails closed (#22941 re-ask).
Follow-ups (fix-forward; none can resolve a wrong option):
- The PR body describes more than the diff does. The rendering half is not here: no Markdown → plain text, no links on their own lines, no paragraph splitting or length cap.
render()(question_reply.rs:93-115) only does numbered options plus the instruction line. Either trim the body's claim or add it in the next PR. - Spec'd reply forms are missing: "1." and "#1", unique-prefix matching, and yes/no for two-option permissions (never for choice questions).
is_from_me, the message date, time-based staleness and tapback removal (associated types 3000+) are not modelled. Model them here, or document and test the caller's contract at the connector.- Tapbacks need explicit
with_tapbackbindings, so the connector must set them. to_lowercaseis not full Unicode case folding.- A multi-select item resolves to a single option, which is not a complete answer.
- Cap the inbound text length before
one_lineallocates. - Leading zeros ("01") are untested.
Co-authored-by: Codex <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com> Agent-Session: https://cmux.com/deeplink/agent?id=aid-62adf8b7714446c2cda6
Bugbot needs on-demand usage enabledBugbot uses usage-based billing for this team and requires on-demand usage to be enabled. A team admin can enable on-demand usage in the Cursor dashboard. |
teamleaderleo
left a comment
There was a problem hiding this comment.
Review (claude-opus-5-5): SHIP at 0587cae
Review (subagent claude-opus-5-5 medium): SHIP at 0587cae
Visual verdict: unchanged by this diff (no UI).
The delta from my SHIP at 114542e is one commit touching only question_reply_tests.rs: two test-only clippy fixes, a removed .clone() and a slice comparison. No change to the code under test. Land once the tests and clippy at this head (run 41614121649) are green.
Follow-ups: tracked in #33560.
This PR adds reply resolution and numbered-prompt rendering only. A reply to an existing question becomes a precise option selection without treating arbitrary text or a reaction as permission approval. This adds a pure
cmux-conversation::question_replyseam: deterministic numbering across question items, exact option text matching (case/whitespace normalized), and explicitly bound tapbacks.The prompt retains the original request/question IDs and full question snapshot. Resolution returns
WrongThread,Stale,Ambiguous, orUnboundTapbackwhen the reply cannot safely identify a choice; unmatched text never becomes a guessed Other answer. Numeric labels that collide with another displayed number are ambiguous. The result carries the original session, permission, item and option IDs and does not mutate or consume the question.The caller must authenticate the sender, reread the current pending question, and atomically validate/commit a complete answer through the existing owner. Multi-item and multi-select replies return one selection for the caller to accumulate. Free-text/Other submission remains outside this option-selection seam. No transport, sending, permission proof, approval RPC, Feed mapping, or native UI is added here.
Leo-ask: ask-6ef2624788
This preserves #22747 and #22941: no new Chief approval gates, no inferred person provenance, and no relayed permission allow. An offered option literally named “Allow” is still only a question selection, never authorization. The separate native question bridge and adopted #33385 own their existing paths. #28846's Feed mapping is untouched.
Validation: 14 new behavioral tests cover stable rendering, all numbered options across harnesses/items, identity retention, duplicate text and numeric collisions, stale/replaced/closed requests, wrong threads, explicit tapback binding, and no guessed approval. Exact-head fleet tests and strict clippy passed: 26 tests, including all 14 new tests, zero failures;
cargo clippy --locked -p cmux-conversation --all-targets -- -D warningspassed. Run 41614121649 tested0587cae2b97024a14cf6b93a7de984750c7ebe86, snapshot97c736d86d7eb36ce3247b9a1d936652fe60cb7d, matching tree5c4598fe26f051362fd092a3752309840756db63, exit 0. The broader clippy job completes strict clippy but fails its panic baseline ratchet in untouched acpmux indexing/string-slice counts and cmux-host/cloud/github_status.rs:112; those source trees and the baseline are byte-identical to base.rustfmt,git diff --check, andprepush-gate.pypassed. The broadverify-local.pyreports unchanged localization debt and a missing CmuxDeviceHub workspace group already present in base a8d0ea0; no affected files or their checks changed.Remaining work is tracked in #33560 and follows separately: rich-text cleanup, flexible reply forms, connector-envelope freshness and reaction-removal handling, Unicode matching, explicit multi-select completion, and input bounds. This PR claims none of those.
Prior art: #30170, #27016 and #23847 read and acknowledged; their behavior is preserved. This is a library seam only, so it adds no action, default, CLI/MCP surface, or visual variant. Product integration and end-to-end Messages evidence belong to the separate wiring work.
🤖 Generated with Codex
Agent session: agent session