Repository navigation
feat: add retained-message replies and agent routing context - #1
Conversation
leoli-dev
left a comment
leoli-dev
left a comment
There was a problem hiding this comment.
Reviewed against the final collaboration-friction plan (R1a scope + R2/R4 fixtures).
Verdict: no blocking correctness issues. Re-ran on 69d91f6: go vet ./..., go test -race -count=1 ./... (all pass), gofmt -l . (clean), generated OpenAPI equality, and python3 scripts/smoke.py (passed).
Plan conformance checked and OK:
- Reply runs entirely under the state lock; parent retention, channel authorization and private peer/host
Enabledare rechecked there (thedirect()-only target check is replicated, as the plan required). - Multipart reply never reaches
parseSend/staging (legacyUploadRoute/replyRoutegating inserve()); reply rejects on media type before reading the body. - Strict input:
DisallowUnknownFieldsplus the duplicate/case-variant key scan closes the"Body"/repeated-key holes inencoding/json. message_unavailableis mapped to 404,invalid_target400,text_too_longcarries limit/revision; body validation precedes parent lookup, so no existence leak.- Context is attached only in the agent handlers;
historyMatchingand admin history are untouched;UseNumberkeeps historysequenceexact. Private context exposes only the one peer. - ACK keeps status order/token/expiry;
readis null only without a Delivery. - No schema change, no persisted parent, no new record kind;
requirements.mduntouched.
Three non-blocking comments inline (security-doc gap for reply allowlisting, unknown-outcome classification for gateway JSON errors, private phase labels in public docs/test names).
| and prevent alternate access paths. | ||
|
|
||
| `agent_post_reply` accepts text only and preserves the retained original | ||
| channel. It still needs an argument-level policy for the parent’s authorized |
There was a problem hiding this comment.
Security doc gap (medium). "It still needs an argument-level policy for the parent's authorized private peer or group" — but the tool's only routing argument is an opaque message_id. With no R1b GET /messages/{id}, a gateway cannot derive the audience from the call's arguments. In practice, allowlisting agent_post_reply lets the responder post to any private peer or group the account can read a retained message from, which is broader than a group_send allowlist pinned to one group_id.
Suggest saying explicitly how to enforce this today: the gateway must resolve message_id against the context.channel it recorded from its own claim/history responses and deny IDs it has not seen (or whose recorded channel is outside the profile). Otherwise, constrained responders should not be given reply. Same wording applies to docs/mcp.md ("argument-level recipient/channel policy using the retained parent's route"), the zh-CN security doc, and the SKILL. Not a code bug; the server-side authorization is correct.
| } | ||
| if err != nil { | ||
| var apiErr *APIError | ||
| if errors.As(err, &apiErr) { |
There was a problem hiding this comment.
Unknown-outcome classification (low). Any non-2xx response with a valid JSON body becomes *APIError and is reported as a definite failure. That holds for this server (every error path rolls state back before responding). It does not hold for an intermediary: a reverse proxy or API gateway that returns a JSON-bodied 502/504 after forwarding the request may already have let the reply commit. The agent then sees a definite failure and may resend. That is exactly the unknown-send case the docs say must not be re-POSTed.
Cheap fix: treat the error as definite only when the payload matches the service envelope {"error":{"code":...}}. Alternatively, always classify 502/504 as reply outcome is unknown. HTML/non-JSON gateway errors are already handled correctly via the invalid-JSON path.
| validation, reference local components, and persistent threads require their own | ||
| fixtures before their capabilities can be advertised. | ||
|
|
||
| ## R1a checks and call-count fixtures |
There was a problem hiding this comment.
Public-doc hygiene (low). R0a/R0b/R1a/R1b/R4 are phase labels from the maintainer's private plan. No public document defines them (git grep R0a 54e58cc is empty). They now appear in conformance.md, validation.md, and test names (TestR0aLegacy…, TestR1…, TestR4SelectedRouteReplyCallCounts). AGENTS.md asks that public docs stay usable without the private development log.
Suggest neutral names, for example ## Route-reply and context checks, TestLegacyWorkflowCallCounts, TestRouteReplyCallCounts, TestRouteReply*/TestAgentContext*. Update the -run regexes in this file to match. The alternative is to define the phases in docs/implementation-plan.md.
leoli-dev
left a comment
leoli-dev
left a comment
There was a problem hiding this comment.
Re-review of eb692f4: all three earlier comments are resolved; LGTM to merge.
- Reply allowlisting:
agent-security(en/zh),mcp.md,agent-operations(en/zh) and the SKILL now state that an opaquemessage_idcannot constrain the audience. They require a trusted gateway to keep identity-boundmessage_id → context.channelmappings from its own claim/history responses and to deny unseen IDs. Without that,replyis omitted for constrained responders. The docs are explicit that no gateway or key scope ships. - Unknown outcomes:
isServiceErrorEnvelopeplus forcing 502/504 to unknown is correct. Null, empty, numeric and duplicate-keycode/errorvalues all fall to unknown because of Go's last-key-wins rule. The unknown error drops bothrawand the*APIErrortype, and legacysendbehavior is pinned unchanged. The new CLI/MCP tests assert exactly one POST. - Phase labels: removed from public docs and test names (
git grep 'R0a\|R1a\|R1b\|R0b\|TestR4'over docs/internal/skills is empty). The-runregexes inconformance.mdmatch the renamed tests.
Re-ran locally on eb692f4: go vet, go test -race -count=1 ./..., gofmt, OpenAPI equality and scripts/smoke.py all pass. Hosted checks (contract, documents, admin-ui, native Linux/macOS/Windows) are green. The server runtime is unchanged since the previous review.
Problem and resulting behavior
Replying from a retained message previously required resolving its channel before choosing a direct or group send. Add text-only
POST /messages/{message_id}/replyand matching CLI/MCP operations that preserve the original audience, recheck current access, and return one sent/duplicate result. Agent claim/history include computed routing context, ACK results include a nullable account read observation, and limits advertises the three implemented features.Reply gateway HTTP 502/504 and non-service error JSON remain explicitly unknown in CLI/MCP, with no automatic resend. Constrained responder guidance requires a trusted gateway to map IDs from its own authenticated claim/history context to authorized audiences; otherwise it must omit opaque-ID reply.
Requirement and design impact
Adds deterministic baseline/comparison fixtures and capability-specific conformance checks. Preserves key plus exact-IP authentication, polling-only access, claim/ACK fencing, fan-out privacy, retention/deduplication, administrator history, and database schema 1 (REQ-10, REQ-13, REQ-16–17, REQ-24–46, REQ-54). Reply stores no parent/thread association and does not make sends idempotent. Multipart reply is rejected before staging files.
Updates the generated request contract, bilingual agent manuals/security/schedules/quickstarts, MCP guide, bundled skill, runtime and validation records. Conformance documents a future read-only doctor specification; no doctor command or gateway implementation is added. Public docs and test names describe behavior without private-plan phase labels.
Verification
go test -race -count=1 ./..., build/vet, four Node i18n checks and actual baseline54e58ccCLI/MCP compatibility checks passed for69d91f6. Its hosted native Linux/macOS/Windows, Chromium, contract/smoke and documentation jobs all passed.go vet ./..., real-servicescripts/smoke.py, documentation, gofmt, generated OpenAPI equality and whitespace checks passed locally.Assumptions and limitations
The selected synthetic channel-enumeration/reply workflows use 2 HTTP calls before and 1 after; known-recipient raw direct sends already need only 1. No actual collaboration, latency or model-wakeup benefit is inferred. A fresh history miss is not proof of non-delivery.
Actual-profile pilots, production-helper measurements, arbitrary third-party closed response parsers and actual gateway permission enforcement remain untested. Lookup/probe APIs, a reference local component, persistent threads, production deployment and release/platform claims are outside this delivery.