basecamp / basecamp/basecamp-sdk

Event feed: three suppressed review findings (dedupe recency, tier-2 advance determinism, null-vs-absent frame fields)

Open
#765 1 comment 0 reactions 0 assignees View on GitHub
bug conformance spec
Dominant language
Go
Stars
49
Forks
12
Avg merge
20h 47m
Merged PRs (30d)
89

Description

Three findings Copilot raised on #705 in a **suppressed** block. They never
became review threads, so they never moved the unresolved count and would have
been lost when the PR merged. Filing them together because they arrived
together and none is being fixed in #705; each has its own disposition below.

All three are correct as far as I can verify them. None can cause a skip or a
hang, which is the line #705 has been using for fix-here versus file.

---

## 1. `dedupe.go` — the LRU tracks *observed* ids, not *delivered* ones

A duplicate hit calls `MoveToFront`, refreshing recency for an id that was not
delivered. SPEC §23 specifies an "LRU of actually-delivered event ids", and
Copilot's counterexample is exact: deliver 1 and 2, observe a duplicate 1, then
deliver 3 — the refresh of 1 makes 2 the eviction victim, so a later duplicate
2 is **delivered a second time** even though 2 is the more recently *delivered*
id.

**Real, and it contradicts the spec's own wording.** The fix is small — suppress
without touching recency — but it changes eviction order, so the existing
recency test encodes the wrong behaviour and has to change with it. That is
worth a moment's care rather than a quick patch: a test that must be edited to
accept a fix is exactly where a wrong invariant gets re-blessed.

**Why not in #705:** the consequence is a duplicate delivery, which is the safe
direction for an at-least-once feed. It cannot skip.

Cross-SDK: §23's wording is normative, so whatever is decided applies to all six
dedupe implementations.

---

## 2. `scenario_conformance_test.go` — the tier-2 `advance` directive is not the normative algorithm

The fixture driver's `advance` calls plain `Advance`, which can return before the
connector goroutine has handled a firing and armed a follow-on timer. That
follow-on is then anchored at the window's end and is not fired inside the same
window, so a chained-timer scenario is not deterministic. `AdvanceSettling`
exists and implements the rendezvous, but only the dedicated clock test uses it
— the fixture driver never does, and the schema offers no way for a fixture to
request it.

**This is the most consequential of the three**, and not for #705's Go code: the
tier-2 fixtures are the cross-language conformance contract, so a directive that
does not implement its own normative algorithm means a schema-valid
chained-timer scenario can pass in one language and fail in another for reasons
neither implementation controls. It should be settled before more languages
implement the family.

---

## 3. `cable.go` — `*string` cannot distinguish absent from JSON `null`

`{"type":null}` decodes with `Type == nil` and routes through the typeless path.
The same absent-vs-null ambiguity applies to the `identifier`, `reason` and
`reconnect` fields the selected frame shapes read.

**Third time this has surfaced on #705**, and my earlier decline of it stands on
capability grounds — a peer that wants to deliver an event through the typeless
path can simply omit `type`, so distinguishing null from absent closes nothing.
Copilot adds a fair point I had not addressed: `parseFrame`'s stated contract
says a *wrong-typed* `type` is an invalid frame, and `null` is wrong-typed, so
the implementation and its own doc disagree even though no capability turns on
it.

**Disposition: consistency, not security.** Either the contract says
"unparseable or wrong-typed, null excepted", or presence is tracked with
`json.RawMessage`. It is a §23 envelope-grammar statement binding six SDKs, not
a Go-local parser bug — which is why it is filed rather than patched into a
parser that has already been reworked once this round.

---

Follows #606, #614, #645, #696; siblings #753, #758, #759, #760, #761, #762,
#763, #764.

Contributor guide

Open the contributing guide

Research direction

Start with SPEC §23 and the three named entry points: dedupe.go, scenario_conformance_test.go, and cable.go. Run the existing recency and dedicated clock tests, then inspect the fixture driver's tier-2 advance path and parseFrame contract. Done means each disposition is settled and the relevant tests, schema behavior, and cross-SDK conformance expectations agree.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
backend-api-design, testing
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.