diff --git a/docs/development/agent-experience-audit.md b/docs/development/agent-experience-audit.md index df0fb76a..e1888c8c 100644 --- a/docs/development/agent-experience-audit.md +++ b/docs/development/agent-experience-audit.md @@ -2769,6 +2769,7 @@ declared missing; verifying it myself rather than accepting it is what turned "the comments API is incomplete" into the `commit_id` asymmetry, and sweeping the other seven PRs is what found `#1330`, where both APIs are silent and the gate is real anyway. + ## 49. A progress note re-claimed a row a human had re-pointed two days earlier (2026-08-28, pod-architect, found by the seat) **The surface:** `commonly_update_task`, whose tool description is *"Append an @@ -2861,3 +2862,161 @@ support the sentence they were cited for. status"* is a promise about the common path stated as a property of the tool. The seat that most needs the exception is the one deliberately avoiding a claim, and that is the seat the description reassures. + +## 50. The instrument that counts sentinels misses the worst sentinels (2026-08-26, pod-architect + sprint-review) + +**Surface:** `catch { return }` across `backend/`, and the two +confirmed defects it has already produced — `readLongTerm` returning `''` on a +transport failure (#1275) and `findLiveIntegration` returning `null` when the +Integration lookup throws (#1287 item 2). + +**The defect class:** a catch block returns a value that is *also reachable on +the success path*. The caller cannot distinguish "this failed" from "this +legitimately has nothing", so the loudest condition — backend unreachable, auth +revoked, index missing — renders as the quietest and most common one. In #1275 +the guard `err?.status && err.status !== 404` skipped the error branch exactly +when `err.status` was undefined, which is precisely the transport-failure case. + +**The specimen, because it needs no call-site read to see.** +`backend/routes/registry/detect.ts`, twice in one file: + +``` +:92 if (!skillsDir) return { status: 'unavailable', skills: [] }; +:98 catch (error) return { status: 'unavailable', skills: [] }; + +:132 if (!dockerfilePath) return { status: 'unavailable', aptPackages: [], pythonPackages: [] }; +:138 catch (error) return { status: 'unavailable', aptPackages: [], pythonPackages: [] }; +``` + +Identical expression, six lines apart, guard and catch. "The directory is not +there" and "`readdirSync` threw" return the same object. The success path is +*visible in the same screenful*, which is what makes this the clearest statement +of the class: you do not need to know what the caller does with it. + +**A second instance where the correct value was already on the line above.** +`backend/services/discordService.ts`: + +``` +:500 return (this.integration as IntegrationDoc).status || 'unknown'; +:502 catch (error) { return 'error'; } +``` + +`'error'` is a member of the `IntegrationStatus` enum +(`backend/models/Integration.ts:118`). `'unknown'` is not. The function already +had an out-of-band sentinel for "no answer", one line above the catch, and the +catch reached past it for an in-band one. + +**A third, collapsing three conditions into one string.** +`backend/services/systemExchangeTriggers.ts`: + +``` +:353 return inst?.instanceId || 'default'; +:355 catch { return 'default'; } +``` + +The query threw, no active installation exists, and `instanceId` is literally +`'default'` are indistinguishable at the call site. + +**And the reciprocal — which fires the same tell harder, and is correct.** +`backend/services/avatarService.ts` normalises an avatar reference: + +``` +:37 if (LEGACY_COLOR_AVATARS.has(value)) return value; +:38 if (/^data:/i.test(value) || value.startsWith('/')) return value; +:47 } catch { +:48 return value; +:50 return value; +``` + +Four `return value` in thirteen lines, one of them the catch. By "the success +path and the catch return the identical expression, in view of each other" — +the tell `detect.ts` seems to teach — this is the most flagrant site in the +repo. It is also exactly right. `services/agentMessageService.ts:474`, +`services/skillsCatalogService.ts:134` and `routes/pods.ts:180` are the same +shape: normalise this thing; keep the original if it will not parse. + +**So the discriminator is not syntactic, and not about the value. It is a +question about the `try`:** + +> Name the question the code inside the `try` is asking. Then check whether the +> value the catch returns already answers a *different* question elsewhere in +> the same function. + +Where throwing is the only way to ask, the catch is the `else` and the collapse +is the specification. All four correct sites are the same construct — `new +URL(...)` at `avatarService.ts:42`, `agentMessageService.ts:474`, +`skillsCatalogService.ts:125`, `pods.ts:177`. Where the code asked one question +and the catch answers a different one with the same value, it is a defect: + +``` +detect.ts:96 readdirSync — "is it there?" was already answered at :91 +detect.ts:136 readFileSync — already answered at :131 +discordService.ts:499 initialize() — 'error' is an IntegrationStatus enum member +telegramBridgeService findOne — null already means "no live integration" +systemExchangeTriggers findOne — 'default' is a real instanceId +``` + +This is why the paired snippets look identical and are not: in `detect.ts` the +guard *above* the `try` has already answered the question the catch answers, and +in `avatarService` the guards at `:37`/`:38` answer different questions while the +catch answers the one only a throw can ask. The difference is one line up, in +both. + +**Not sufficient either, and the entry would be dishonest to close on it.** +`try { JSON.parse(trustedInput) } catch { return {} }` is predicate-shaped and +still collapses a fault. Two further things it does *not* discriminate on, +checked against these nine sites: + +- **How tightly the `try` is scoped.** `detect.ts:95-97` wraps exactly one + statement and is a defect; `skillsCatalogService.ts:124-134` wraps eleven, ten + of which cannot throw, and is correct. Scope is orthogonal — though a wide + `try` is a latent hazard: add one throwing call inside those ten lines and the + site becomes a defect with the catch never edited. (Mirror of the `void + asyncFn()` hazard, where *narrowing* someone else's `try` is what breaks it.) +- **Whether the caller branches.** See above — no caller here does. + +The procedure survives; every predicate offered as a shortcut to it has failed. + +**How we found the class, and every proxy that failed on the way.** The sweep +started by enumerating sentinel *literals* and widened the set twice — `false`, +`null`, `''`, `[]`, `{}`, then `0` and `""` — reaching a confident 21 sites at +`994a963f`. `0` and `""` contributed nothing; only `{}` ever moved the count. + +- **Literal-only** missed every site above: all four return a non-literal. +- **Non-literal** is not closer to the class, it errs the other way — the same + sweep pulls in `return res.status(400).json({ error: err.message })` from + several controllers, which is maximally *loud*. +- **Bare `catch {` vs bound `catch (err) {`** misses too: the `discordService` + and both `detect.ts` sites are bound, with the binding unused. +- **"The value looks like an error"** was the proxy that made us wave the two + `detect.ts` sites through on the first pass. They were in the census output, + read as explicit failure values, and were classified out by hand — by the + people writing this entry about proxies failing. + +**Rules earned:** +- The discriminator is a question about the `try`, not about the value, the + syntax, or the caller: name what the code inside it is asking, then check + whether the catch's return value already answers a different question in the + same function. +- Do not close a write-up about failed proxies by handing over a new one. Six + were tried here; the one that separates all nine sites is stated as a + procedure with its counter-example attached, not as a rule. +- Six proxies were tried and all six failed, three of them invented during this + write-up: sentinel-literal, non-literal, bare-vs-bound catch, "the value looks + like an error", "guard and catch return the same expression in view of each + other", and "the caller branches differently". A syntactic tell can be + necessary; none was sufficient. +- A shape count is the number that gets quoted, and it is not a defect count. + Publish it as "N sites share the shape; K confirmed defects; the rest + unclassified" or do not publish it. +- Widening an enumerated set feels like rigour and cannot escape the axis you + enumerated on. When a sweep is defined by a value set, run one census with the + filter removed and classify by hand — that pass is the only one that can tell + you the axis was wrong. Here the count turned out to fail in *both* directions, + which is the finding; a proxy that only over-counts is a much smaller problem. +- Publish the correct site next to the defect, not the defect alone. Both + `detect.ts` and `avatarService.ts` fire the same tell; showing only the first + installs the proxy that the second refutes. +- No lint rule can see this. `@typescript-eslint/no-floating-promises` has no + analogue, because the defect is a relation between two return sites rather + than a property of either.