feat(compass): SEA-1519 make ask answer state server-owned on the wire - #31
Conversation
Greptile SummaryAdds server-owned ask-answer state to the wire contract.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| proto/compass/v1/comms.proto | Adds the additive answered field and documents ask and per-question answer state as server-owned. |
| go/internal/comms/mapping.go | Projects persisted answered state outbound while omitting all server-owned answer fields from newly posted inbound asks. |
| go/internal/comms/ask_mapping_test.go | Exhaustively verifies field ownership and both values of the outbound answered projection. |
| go/internal/comms/comms_test.go | Adds end-to-end coverage for list and stream projections and rejection of forged inbound answer state. |
| go/gen/compass/v1/comms.pb.go | Regenerates the Go protobuf binding with the additive answered field. |
| packages/compass-agent/src/gen/compass/v1/comms_pb.ts | Regenerates the agent TypeScript binding with the answered field. |
| packages/compass-client/src/gen/compass/v1/comms_pb.ts | Regenerates the client TypeScript binding with the answered field. |
Reviews (4): Last reviewed commit: "test(comms): guard ask answer-state owne..." | Re-trigger Greptile
|
Cross-branch integration verified locally. This repo has no CI yet, so this is the only integration evidence there is — and it is the check that caught a real defect in the sealed copies of these same diffs (a deleted symbol still cited by a sibling branch, invisible to any single-diff review). Merged all five of this lane's branches together onto All five merge clean, no conflicts. On the combined tree, against a live Worth stating plainly: A per-package throwaway Postgres container also starves under this suite's parallelism (the store package hit its 600s timeout four times over); one shared database runs the same suite in ~30s. Relevant when CI is set up here. |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
8106f9a to
92432af
Compare
compass.v1.Ask could not express that an ask was already answered: askToWire dropped the store's Answered flag. The server accepts exactly one RespondToAsk per ask, so a client that cannot see the flag renders an answered ask as answerable and the second participant's click is fired at a server guaranteed to reject it. Reachable the moment two people share a channel. Answered is the only trustworthy signal -- a fully-skipped ask leaves every question's answer fields empty, indistinguishable from pending. Project it outbound as additive field 7, and close the mirror gap inbound: askFromWire dropped the server-owned ask_id but honored chosen_option_ids, custom_text and timed_out, so a caller could post an ask that arrived pre-populated and rendered as settled while the server held it answerable. Answer state is now dropped by construction on the way in. Reported by the UI lane. Co-Authored-By: seal <noreply@sealedsecurity.com>
askFromWire drops server-owned ask fields by omitting them from a keyed composite literal, which compiles and vets clean with any subset set. A field added to the proto and wired into askToWire would therefore be silently honored inbound, letting a caller post an ask that arrives looking already-settled. Add an untagged descriptor-driven guard: it classifies every field of Ask/AskQuestion/AskOption as content or server-owned answer state, fails when a descriptor field appears in neither set, populates a wire Ask exhaustively by protobuf reflection, and asserts the mapped store Ask carries only content. Pin the outbound Answered projection alongside it. Extend the PostMessage round-trip test to pin the survivors too — header, allow_multiple, recommended, and per-option description/preview — so an over-broad drop cannot silently discard agent-supplied question content. Document the ownership in the proto, the contract a client author reads: answered and the AskQuestion answer-state fields are set only by RespondToAsk, and values supplied on an inbound Ask are ignored. Co-Authored-By: seal <noreply@sealedsecurity.com>
92432af to
5d6b924
Compare

compass.v1.Askcould not tell a client that an ask was already answered, so the client rendered a spent ask as answerable and the second person's click was silently discarded.The bug
Askcarriedask_id+questionsand nothing else, andaskToWire(internal/comms/mapping.go:143) did not project the store'sAnsweredflag — the flag whose own doc comment (internal/store/types.go:250-255) says it is "the only reliable answered-signal".The server accepts exactly one
RespondToAskper ask, forever (messages.go:400-406,ErrConflictonceAnsweredis set). So an answered ask arrived over the wire indistinguishable from a pending one. A second participant sees an answerable ask, clicks, and the client fires an RPC the server is guaranteed to reject. Their answer vanishes.Not hypothetical — it is reachable the moment two people share a channel, which is exactly the dogfood setup.
Why inspecting the questions is not a workaround: a fully-skipped ask leaves every question's answer fields empty, which reads identically to pending. That is precisely why the store keeps a separate flag.
The fix, both directions
bool answered = 7onAsk(fields 2..5 are reserved from the compass-0.5 single-question shape, so 7 is the next free slot), projected inaskToWire. Additive, non-breaking.Inbound, the flag is dropped — whether an ask is spent is the server's to decide from its own row, never a client's to assert. A client that could send
answered: truecould mark another participant's ask closed.The review then found the mirror of the same gap, one layer down and pre-existing:
askFromWiredropped the server-ownedask_idbut honoredchosen_option_ids,custom_textandtimed_out. So a caller couldPostMessagean ask that arrived pre-populated — rendering as settled-and-locked to any client reading the per-question fields, while the server still held it pending and answerable. Same class as the bug this PR closes, arriving through the door instead of the window, and it slightly undercut the new field's claim to be the only trustworthy signal while the forgeable ones stayed reachable.Answer state is now dropped by construction on the way in — omitted from the composite literal rather than stripped afterwards, so there is no line to delete without a compile-visible change. That is the rule
ask_idalready followed; it just wasn't applied to the fields beside it.Verification
TestAskAnsweredFlagReachesTheWireasserts both poles from the same ask — pending readsfalse, and afterRespondToAskthe same ask readstrue— through theListMessageswire read a UI actually consumes, not the store row. Asserting only the true pole would pass against a hard-codedAnswered: true, which is the failure mode worth designing against here.Proven genuinely red→green, and mutation-proven both ways by the review: reverting the projection fails the true pole, and hard-coding
Answered: truefails the false pole. Neither could pass for an unrelated reason.TestPostMessageDropsCallerSuppliedAnswerStateposts an ask with every server-owned field set to a value it must not keep — forgedask_id,answered: true,chosen_option_ids,custom_text,timed_out— and asserts all are dropped, the id is server-minted, and the ask is still genuinely answerable afterwards (a realRespondToAsksucceeds), so the drop left real pending state rather than a cosmetically-blanked row. Verified red against the old mapping:chosen_option_ids = [opt-a],custom_text = "forged answer",timed_out=trueall honored before the fix.The existing
MessageUpdatedtest now also assertsansweredon the live stream — the surface a UI actually consumes to flip an ask from answerable to spent.go build,go vet,gofmt -l(empty),-raceunit suite, and the real-Postgresinternal/comms+internal/storesuites all pass.golangci-lintreports zero findings in hand-written code (theduplhits are pre-existing in generated.pb.go). Regenerated both Go and TS viamoon run compass-proto:gen, so the committed generated files match the.proto— no drift.Credit and scope
Found and diagnosed by the UI lane, who identified the free field number and the exact two-line shape, then declined to edit another lane's proto and mapping — correct call, and the report was accurate on every point I checked. They shipped the client-side floor (surface the refusal rather than let the click vanish) separately; this is the server half that makes the floor unnecessary.
Spec-impact: none
No externally-visible behavior beyond an additive field that reports state the server already tracked; the ask lifecycle itself is unchanged.
Targets
mainfor review; lands insealedsecurity/compassafter the port.Ported from
sealedsecurity/sealed#997, unchanged in content — Compass code now lives here, so this is where it ships. The sealed PR is closed in favour of this one.Verification (this repo has no CI yet, so a local run is the only evidence there is):
go build ./...,go vet -tags "pgtest unix" ./...,gofmt -lclean,go test -race ./internal/... ./server/..., and thepgtestsuites for./internal/store/,./internal/comms/,./server/against a livepostgres:17— all green on this branch.