test(routes): /threads/state is protected by segment count, not by ordering - #1227
test(routes): /threads/state is protected by segment count, not by ordering#1227lilyshen0722 wants to merge 1 commit into
Conversation
…dering
@sprint-review's sweep found `/threads/following` has exactly one hit
repo-wide — the comment naming it. Pulling that thread found the comment's
central claim is false, not just its route name.
"Registered above the greedy GET /:podId on purpose" — it is not.
`/:podId` is registered at :85 and `/threads/state` at :152, sixty-seven
lines below, and Express matches in registration order. If ordering were
the protection the route would already be dead.
What protects it is segment count: `/:podId` compiles to a single-segment
pattern, `/threads/state` is two. The comment listed that as the
incidental reason ("does not currently collide") and the false one as
load-bearing — so a reader tidying the path to one segment is told they
are safe by an ordering that does not exist. `getMessages` would swallow
it under `auth` rather than `dualAuth`, giving every agent caller a 401 on
a route that looks registered and reads correct.
`/threads/following` never existed on this router. The nearest real path
is `/following/threads` on posts.ts — different router, reversed
segments, `auth`, different controller — so a reader chasing the name
lands somewhere plausible and wrong.
Test executes rather than greps, per rule 18: "does this path reach that
handler" is behavioural. Real router, real registration order, real path
patterns; only the controllers are stubbed, to identify themselves. Three
discriminating cases: /threads/state reaches listThreadState, a CONTROL
single-segment path proves the greedy route is live and would have taken
it, and /threadstate demonstrates the swallow directly. Toggles covered
too — they survive for a second independent reason (their verbs are not
ones the greedy route registers).
27 tests green across this and threadUserState.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
lilyshen0722
left a comment
There was a problem hiding this comment.
The central finding is correct and I verified it at origin/main: router.get('/:podId', ..., auth, getMessages) is :85, router.get('/threads/state', ..., dualAuth, listThreadState) is :152 — sixty-seven lines below, so ordering is not what protects it, and segment count is. The auth-vs-dualAuth consequence is real. The suite runs 6 passed.
One blocking-ish correction: the replacement comment introduces a false reason of its own, in the sentence that replaces the false one. It says the toggles survive for "a second independent reason — their verbs (POST/DELETE/PUT on two segments) are not ones the greedy route registers." The router registers greedy single-segment routes for two of those three verbs, both above the toggles:
:85 router.get('/:podId', readMessageRateLimit, auth, getMessages)
:86 router.post('/:podId', sendMessageRateLimit, auth, createMessage)
:87 router.delete('/:id', auth, deleteMessage)
Probed executing, to the same rule-18 standard this PR sets, against the real router:
POST /api/messages/threads -> createMessage (greedy POST is live)
DELETE /api/messages/42 -> deleteMessage (greedy DELETE is live)
PUT /api/messages/42 -> 404 (no greedy PUT)
So the verb reason holds for PUT /:messageId/collapsed only. For POST /:messageId/follow and DELETE /:messageId/follow there is exactly one protection — segment count — precisely as for /threads/state. "Two independent reasons, neither of them ordering" is true of one of the three.
Two things make this worth changing rather than noting:
- The test's own comment names the counterexamples in the sentence that denies them: "they carry a VERB the greedy route does not register — GET/POST
/:podIdand DELETE/:id." - The parameterized test passes whether the claim is true or false, because segment count carries all three cases. The suite cannot detect that its own stated reason is wrong — which is the same unfalsifiable-reason shape this PR exists to remove, one paragraph further down.
Suggested: drop the "second independent reason" clause, or scope it to collapsed and say plainly that both follow toggles rest on segment count alone. If the verb distinction is worth keeping, it needs the control above (single-segment POST/DELETE landing on the greedy handlers) or it is asserted rather than pinned.
Everything else here is right, and the /threads/following provenance note is a good catch.
@sprint-review's sweep:
/threads/followinghas exactly one hit repo-wide — the comment atroutes/messages.ts:142naming it. Pulling that thread found the comment's central claim is false, not just its route name.The claim
It is registered sixty-seven lines below it, and Express matches in registration order. If ordering were the protection, this route would already be dead.
What actually protects it
Segment count.
/:podIdcompiles to a single-segment pattern;/threads/stateis two. The comment lists that as the incidental reason ("is two segments so it does not currently collide") and the false one as load-bearing — so a reader tidying the path to one segment is told they are safe by an ordering that does not exist.getMessagesswallows it underauthrather thandualAuth: every agent caller gets a 401 on a route that looks registered and reads correct./threads/followingnever existed on this router. The nearest real path is/following/threadsonposts.ts:29— different router, reversed segments,auth, different controller — so a reader chasing the name lands somewhere plausible and wrong.The test
Executes rather than greps, per rule 18 — "does this path reach that handler" is behavioural, and only the real router can answer it. Real router, real registration order, real path patterns; only the controllers are stubbed, to identify themselves.
GET /threads/statelistThreadStateGET /threadsgetMessageswithpodId: 'threads'— the greedy route is live and would have taken itGET /threadstategetMessages— the latent bug, demonstratedThe CONTROL is what makes the first assertion discriminating rather than vacuous — without it, "reaches
listThreadState" is equally consistent with a greedy route that never matches anything.Comment rewritten per rule 15: the rule stands (keep it above, keep it two segments), the stated reason was false. 27 tests green across this and
threadUserState.