Skip to content

docs(ax): entry 44 — the instrument that counts sentinels misses the worst sentinels - #1291

Merged
lilyshen0722 merged 7 commits into
mainfrom
docs/ax-44-sentinel-catch
Aug 30, 2026
Merged

lilyshen0722 merged 7 commits into
mainfrom
docs/ax-44-sentinel-catch

Conversation

@lilyshen0722

Copy link
Copy Markdown
Contributor

Files the sentinel-catch defect class agreed with @sprint-review, and the measurement mistake that nearly shipped instead of it.

The class. A catch returning a value that is also reachable on the success path makes the loudest condition — backend unreachable, auth revoked — render as the quietest and most common one. Two confirmed instances: readLongTerm returning '' on a transport failure (#1275) and findLiveIntegration returning null when the Integration lookup throws (#1287 item 2).

Why the entry is about the instrument. We swept by enumerating sentinel literals and widened the set twice, reaching a confident 21 sites at 994a963f. Re-running the census with the literal filter removed found 16 more sites, and the two cleanest instances of the class in the repo are both non-literal:

site catch returns success path returns
systemExchangeTriggers.ts:354 'default' inst?.instanceId || 'default'
discordService.ts:501 'error' integration.status || 'unknown' — and 'error' is in the IntegrationStatus enum (Integration.ts:118)

And the reciprocal, which is why the count is not a defect count: avatarService.ts:47, agentMessageService.ts:474, skillsCatalogService.ts:134 and pods.ts:180 share the syntax exactly and are correct by design — all four are "normalise this URL, keep the original if it will not parse", where the sentinel being reachable on success is the specification.

The discriminator was never the literal.


Numbering. Entry 44 was the single free number below 48: main runs 1 … 38, 41, 42, 43, and open PRs claim 39 (#1122), 40 (#1132), 42 (#1212 and #1142), 43 (#1143), 45 (#1204), 46 (#1213), 47 (#1221), 48 (#1264). That contention is itself filed as #1288, whose proposed heading-uniqueness check would catch the two live duplicates and the 39/40 gap. This entry appends past the current tail and does not touch any contended heading.

Docs-only.

🤖 Generated with Claude Code

@lilyshen0722

Copy link
Copy Markdown
Contributor Author

Checked at f9f2bd70 / 994a963f. Your avatarService.ts correction is right — both files exist, the URL shape is in avatarService.ts, and I'd looked at agentAvatarService.ts. Your detect.ts:132 is right too; my :133 was off by one. Exact lines: catch at avatarService.ts:47, its return value at :48; detect.ts pairs are 92/98 and 132/138.

But that correction refutes the specimen heuristic I proposed, and I'd rather the entry say so than carry it.

I suggested detect.ts should lead because the tell is visual: identical expression returned from the guard and from the catch, within one screenful, no call-site read needed. avatarService.ts — the site you cite as a correct reciprocal — has the same tell, harder:

: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, one of them the catch, all within thirteen lines. By the heuristic I proposed it is the most flagrant instance in the repo. It is in fact correct by design. So "success path and catch return the identical expression in view of each other" is necessary and not sufficient — it is a fifth failed proxy, and it fails in the direction that matters least when you're writing a sweep and most when you're picking a specimen.

What separates the two is not visible in either snippet:

Collapsing conditions is a defect when the collapsed conditions call for different action by the caller — not when they merely have different causes.

In avatarService every path means "use value as the avatar"; a caller has nothing to do differently, so the conditions are genuinely equivalent at the boundary and the collapse is the specification. In detect.ts, "no Dockerfile present" is a configuration state and "read threw" is a fault — a caller that wanted to surface or retry one of them cannot. Same in discordService.getStatus: 'error' as a real integration status and 'error' as "the lookup threw" call for different handling, which is exactly why the pre-existing out-of-band 'unknown' on the line above was the available right answer.

detect.ts still earns the lead — it just earns it as an instance whose verdict needs the caller question answered, not as one that shows the answer. Suggest pairing the two snippets in the entry: the tell, fired identically, once on a defect and once on a correct site. That is a stronger teaching pair than the defect alone, and it inoculates the next reader against the proxy I nearly handed them.

@lilyshen0722

Copy link
Copy Markdown
Contributor Author

Your acquittal argument is right and my rule as phrased fails on exactly the two sites the entry is anchored on. Making "call for" normative fixes the verdict, but it moves the work into a judgement call the reader has to make about a hypothetical caller — which is the same kind of appeal the five failed proxies were an attempt to avoid.

There's a version that stays local and checkable, and it separates every specimen we have. Look at what is inside the try, and ask whether the exception is being used as a predicate or is reporting a fault.

Exception-as-predicate: throwing is the only way to ask the question, so the catch is the answer, not an error path. All four correct-by-design sites are this, and all four are the same construct — verified at cccddef7:

avatarService.ts:42          new URL(value)
agentMessageService.ts:474   new URL(url)
skillsCatalogService.ts:125  new URL(sourceUrl)
pods.ts:177                  new URL(rawUrl)

"Is this parseable?" has no non-throwing form in use here, so the catch is the else branch. Collapsing it into the success value is the specification.

Fault-collapsed-into-a-domain-value: the code asked a question, and the catch answers a different one with the same value:

detect.ts:95   fs.readdirSync    → "is it there?" was already answered at :91
detect.ts:135  fs.readFileSync   → already answered at :131
discordService.ts:499  initialize()      → 'error' is an IntegrationStatus enum member
telegramBridgeService.ts:105  findOne(…)  → null already means "no live integration"
systemExchangeTriggers.ts:349 findOne(…)  → 'default' is a real instanceId

That cross-tabs 4/4 and 5/5 on the specimens in the entry, and it explains why the paired snippets look identical: in detect.ts the guard above the try already answered the question the catch is answering, which is what makes the repetition damning. In avatarService the guards at :37/:38 answer different questions (legacy colour, data-URI) and the catch answers the one only a throw can answer. Same shape, different content, and the content is one line up in both cases.

Caveat it needs, in the spirit of the entry: this is not sufficient either. try { JSON.parse(trustedInput) } catch { return {} } is exception-as-predicate by construction and still collapses a fault when the input was supposed to be valid. So it belongs on the list as a sixth entry that is better, not as the answer — otherwise the entry ends by handing the reader the proxy it spent five examples teaching them to distrust.

What I think survives without caveat is the procedure rather than any predicate: read what is inside the try, name the question it asks, then check whether the returned value already answers a different question elsewhere in the same function. Every one of the nine sites resolves under that, and it never requires imagining a caller.

(Also: main is at cccddef7 now, past the e35d89e6 we were both quoting.)

@lilyshen0722

Copy link
Copy Markdown
Contributor Author

Head moved 9da949c956aa3d0e. Docs only, one character pair changed: this entry is renumbered ## 44.## 50.. No prose edited, no cross-references (the number appears exactly once in the file).

Why: #1143 is open and also adds a ## 44."A known-red suite absorbs the next failure", opened 2026-08-25, so it is the older claim. Verified on both PR diffs just now rather than recalled; both are OPEN and both add a header numbered 44. This one moves.

50 was picked by enumeration: main carries 1–38, 41, 42, 43; open PRs claim 39 (#1122), 40 (#1132), 43 (#1142), 44 (#1143), 45 (#1204), 46 (#1213), 47 (#1221), 48 (#1264), 49 (#1325).

Worth stating because it is this entry's own subject: nothing would have gone red. Two AX entries claiming one number conflict only by textual adjacency, and main has carried two ADR-018s for 22 days on exactly that mechanism. The collision was found by enumerating the open set, which is the only instrument that can see it.

Still conflicts with #1325 (entry 49) — both append at EOF, which is the one case where AX numbering does produce a git conflict. Both are mine; I rebase whichever presses second.

@lilyshen0722

Copy link
Copy Markdown
Contributor Author

Head moved twice in the last few minutes and the second move was mine, so here is the delta.

Someone rebased this branch onto current main (5c7790b42f8b13b6, base distance 11 → 0). That is the right remedy and I am not undoing it. But replaying the 44 -> 50 renumber commit through the rebase added the new header beside the old one instead of replacing it — the file came out carrying

## 44. The instrument that counts sentinels misses the worst sentinels (…)
## 50. The instrument that counts sentinels misses the worst sentinels (…)

on consecutive lines, one body between them. That silently re-took the number this branch had deliberately ceded to the older #1143, and a duplicate AX number produces nothing red — the exact collision class this entry's own file documents (main has carried two ADR-018 files for 22 days by the same mechanism).

f7ed83b8 deletes the stale line. One line, 2929 → 2928, no conflict markers, tail intact, ## [0-9]+\. headers now unique.

Ordering note, deliberately not "fixed": the rebase appends 50 after 51, so the tail reads 43 51 50. My earlier merge had put them in numeric order. Out-of-order is cosmetic and re-ordering costs another head move, which re-runs CI and invalidates any gate pinned to this sha. Left as is unless a reviewer wants it.

lilyshen0722 and others added 6 commits August 29, 2026 19:13
…worst sentinels

A catch block that returns a value also reachable on the success path makes
the loudest failure render as the quietest success. Two confirmed defects
(#1275 readLongTerm, #1287 findLiveIntegration).

The sweep that found them enumerated sentinel literals and could not reach
the two cleanest instances in the repo, both of which return a non-literal:
systemExchangeTriggers.ts:354 collapses three conditions into 'default', and
discordService.ts:501 returns 'error', a member of the IntegrationStatus enum.
Four syntactically identical sites are correct by design (URL normalisers), so
the shape count is not a defect count.
… directions

sprint-review's review found the specimen that shows the class without a
call-site read: registry/detect.ts returns the identical object from the
guard at :92 and the catch at :98, six lines apart, and again at :132/:138.

Adds the sharper reading of discordService: 'unknown' on :500 is NOT an enum
member, so the correct out-of-band sentinel was already on the line above the
catch that reached past it for an in-band one.

Records every proxy that failed, including the two we committed ourselves --
bound-vs-bare misses three of the four sites, non-literal over-counts loud
res.status(400) returns, and 'the value looks like an error' is what let both
detect.ts sites through the first hand pass.
…matively

sprint-review's review of f9f2bd7: the specimen tell I leaned on --
guard and catch returning the identical expression in view of each other --
fires harder on avatarService.ts, which is correct by design. Four
'return value' in thirteen lines, one of them the catch. Fifth failed
proxy, and the second one invented while writing up how proxies fail.

Replaces it with their rule: a collapse is a defect when the collapsed
conditions call for different action by the caller.

Adds the caveat that rule needs. Read observationally it acquits both
anchor defects -- findLiveIntegration's caller does 'if (!integration)
return' either way, and readLongTerm's does 'memoryLongTerm || ""' either
way. The identical handling is the bug.
…ot the caller

sprint-review: 'call for different action by the caller' fixes the verdict
but relocates the work into a judgement about a hypothetical caller -- the
same appeal the five proxies were trying to escape. Their local version
separates all nine sites: name the question inside the try, then check
whether the returned value already answers a different question elsewhere
in the same function. Predicate (only a throw can ask) vs fault report.

Records two further things it does NOT discriminate on, both measured:
try-scope (detect.ts wraps one statement and is a defect; skillsCatalog
wraps eleven and is correct) and caller branching. Keeps the JSON.parse
counter-example so the entry does not close by handing over a sixth proxy.
this one moves. Verified live on both PR diffs rather than recalled: both
are OPEN and both add a header numbered 44.

Number 50 picked by enumeration — main carries 1-38, 41, 42, 43; open PRs
claim 39, 40, 43, 44, 45, 46, 47, 48, and 49 (#1325). Single occurrence in
this file; no cross-references to renumber.

This PR's own subject is instruments that miss what they were not shaped to
look for, and a duplicate ADR/AX number is exactly that class: disjoint or
non-adjacent additions merge clean and nothing goes red. main has carried
two ADR-018s for 22 days on the same mechanism.
The 44 -> 50 renumber commit replaced one header line; replaying it during
the rebase onto current main added the new line beside the old one instead,
so the file carried both `## 44.` and `## 50.` on consecutive lines titling
one entry. That re-took the number this branch had deliberately ceded to the
older #1143, and nothing goes red on a duplicate AX number — which is the
collision class this file already documents.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@samxu01
samxu01 force-pushed the docs/ax-44-sentinel-catch branch from 0703e9d to 74d7e78 Compare August 30, 2026 02:13
# Conflicts:
#	docs/development/agent-experience-audit.md
@lilyshen0722

Copy link
Copy Markdown
Contributor Author

Re-resolved at 766bb26c. This is the second time the same conflict has been fixed on this branch tonight: I merged main in and resolved it at 0703e9d5 (02:0xZ), that commit is no longer an ancestor of the head, and the branch came back as 74d7e785 on a base 2 behind main — i.e. a base that predates #1325's merge (6a72187d), so the audit's EOF collision reappeared exactly as before.

Not attributing the rebase; recording it because a rebase silently discards a conflict resolution, and the only visible symptom is the PR going DIRTY again.

Resolution is identical and verified by diff shape, which is the check that matters here — it is what proves main's entry 49 survived rather than being clobbered:

  • git diff origin/main --numstat159 0 (zero deletions)
  • exactly one +## <n>. header in the diff
  • 44 headers, 44 unique, no duplicate numbers
  • tail order … 42, 43, 51, 49, 50

Anyone rebasing this branch again: the resolution keeps both EOF entries in numeric order (main's 49 first, then 50). Dropping either side is the failure mode.

@lilyshen0722
lilyshen0722 merged commit 12d8807 into main Aug 30, 2026
10 checks passed
@lilyshen0722
lilyshen0722 deleted the docs/ax-44-sentinel-catch branch August 30, 2026 02:24
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.

1 participant