Skip to content

feat(compass): SEA-1519 make ask answer state server-owned on the wire - #31

Merged
mattwilkinsonn merged 2 commits into
mainfrom
compass-server-port-ask
Jul 29, 2026
Merged

feat(compass): SEA-1519 make ask answer state server-owned on the wire#31
mattwilkinsonn merged 2 commits into
mainfrom
compass-server-port-ask

Conversation

@seal-agent

Copy link
Copy Markdown
Contributor

compass.v1.Ask could 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

Ask carried ask_id + questions and nothing else, and askToWire (internal/comms/mapping.go:143) did not project the store's Answered flag — 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 RespondToAsk per ask, forever (messages.go:400-406, ErrConflict once Answered is 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 = 7 on Ask (fields 2..5 are reserved from the compass-0.5 single-question shape, so 7 is the next free slot), projected in askToWire. 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: true could mark another participant's ask closed.

The review then found the mirror of the same gap, one layer down and pre-existing: askFromWire dropped the server-owned ask_id but honored chosen_option_ids, custom_text and timed_out. So a caller could PostMessage an 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_id already followed; it just wasn't applied to the fields beside it.

Verification

TestAskAnsweredFlagReachesTheWire asserts both poles from the same ask — pending reads false, and after RespondToAsk the same ask reads true — through the ListMessages wire read a UI actually consumes, not the store row. Asserting only the true pole would pass against a hard-coded Answered: 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: true fails the false pole. Neither could pass for an unrelated reason.

TestPostMessageDropsCallerSuppliedAnswerState posts an ask with every server-owned field set to a value it must not keep — forged ask_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 real RespondToAsk succeeds), 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=true all honored before the fix.

The existing MessageUpdated test now also asserts answered on the live stream — the surface a UI actually consumes to flip an ask from answerable to spent.

go build, go vet, gofmt -l (empty), -race unit suite, and the real-Postgres internal/comms + internal/store suites all pass. golangci-lint reports zero findings in hand-written code (the dupl hits are pre-existing in generated .pb.go). Regenerated both Go and TS via moon 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 main for review; lands in sealedsecurity/compass after 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 -l clean, go test -race ./internal/... ./server/..., and the pgtest suites for ./internal/store/, ./internal/comms/, ./server/ against a live postgres:17 — all green on this branch.

@linear-code

linear-code Bot commented Jul 29, 2026

Copy link
Copy Markdown

SEA-1519

@greptile-apps

greptile-apps Bot commented Jul 29, 2026

Copy link
Copy Markdown

Greptile Summary

Adds server-owned ask-answer state to the wire contract.

  • Adds the additive Ask.answered protobuf field and regenerates Go and TypeScript bindings.
  • Projects the persisted answered state on outbound message reads and stream events.
  • Drops caller-supplied ask IDs and answer-state fields when accepting newly posted asks.
  • Adds focused mapping and end-to-end regression coverage for pending, answered, and forged inbound states.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

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

@seal-agent

Copy link
Copy Markdown
Contributor Author

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 main#29, #30, #31, #32, #33 — in one tree:

merge boundary rc=0   merge drop rc=0   merge ask rc=0   merge t3 rc=0   merge schema rc=0

All five merge clean, no conflicts. On the combined tree, against a live postgres:17:

gofmt -l           clean
go build ./...     BUILD_OK
go vet -tags…      VET_OK
go test -race ./internal/... ./server/...        all pass
go test -tags pgtest ./internal/store/...        ok  39.358s
go test -tags pgtest ./internal/comms/...        ok  23.879s
go test -tags pgtest ./server/...                ok  20.286s

Worth stating plainly: ./server/ only goes green here because #29 is in the tree. On main today TestServeShutdownWithLiveCommsSubscriberReturnsClean fails against a live Postgres — pre-existing, confirmed on a clean main checkout — and #29 is the fix. The other four each show that same failure alone, and it is not theirs.

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.

Copy link
Copy Markdown
Contributor

mattwilkinsonn and others added 2 commits July 29, 2026 13:21
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>
@seal-agent
seal-agent force-pushed the compass-server-port-ask branch from 92432af to 5d6b924 Compare July 29, 2026 17:22
@mattwilkinsonn
mattwilkinsonn merged commit 428b802 into main Jul 29, 2026
4 checks passed
@mattwilkinsonn
mattwilkinsonn deleted the compass-server-port-ask branch July 29, 2026 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants