Skip to content

fix(core,hono): retire /graphql residue — stale anonymous-deny surface list and inert test doubles - #10849

Merged
os-zhuang merged 1 commit into
mainfrom
claude/issue-10835-graphql-double-residue
Aug 21, 2026
Merged

fix(core,hono): retire /graphql residue — stale anonymous-deny surface list and inert test doubles#10849
os-zhuang merged 1 commit into
mainfrom
claude/issue-10835-graphql-double-residue

Conversation

@os-zhuang

Copy link
Copy Markdown
Contributor

Fixes#10835

Two halves that are not the same kind of task, handled differently and reported separately.

Half 1 — the stale comment (unambiguous)

packages/core/src/security/anonymous-deny.ts described the anonymous-deny surfaces as
"each surface (REST /data, dispatcher /graphql + /meta, raw-hono /data)". Both
facts re-derived against origin/main rather than taken from the card:

ClaimEvidence at source
/graphql is gonepackages/runtime/src/http-dispatcher.ts:2026// /graphql removed — GraphQL is not in the product plan (#2462 follow-on). No handleGraphQL anywhere in packages/runtime; the real HttpDispatcher exposes handleMcp, handleKeys, getDiscoveryInfo, handleAuth, handleMetadata, handleData, handleAnalytics, handleNotification, handleSecurity, handlePackages, handleUi, handleAutomation, handleActions, handleAI, handleShareLinks, dispatch — and no GraphQL member
raw-hono /data is gonepackages/adapters/hono/src/index.ts mounts only prefix, ${prefix}/discovery, /.well-known/objectstack, ${prefix}/auth/* and the terminal ${prefix}/* catch-all. No /data route. Matches content/docs/permissions/authorization.mdx:53 (deleted as a duplicate surface in v17, #4073)

Rather than swap in a fresh route list that will rot the same way, the comment now records
both corrections and defers to the mechanical enumeration — the authz-conformance
ratchet, which reads entry points out of source and fails CI on a STALE row or an
UNCLASSIFIED route. Comment-only: every changed line in that file is a comment line.

Half 2 — the three handleGraphQL doubles (a real question)

Decision: all three were inert residue. Removed — and replaced with an assertion that
actually pins the behaviour they gestured at.

The card's counter-argument was that a wildcard fall-through test may legitimately want a
method the dispatcher does not route. That was tested against the code, not assumed:

  • The adapter cannot observe the property.index.ts calls exactly three dispatcher
    methods — getDiscoveryInfo (l.300/304), handleAuth (l.413), dispatch (l.463).
  • Nothing asserts on it.handleGraphQL was never invoked or asserted in either test
    file, and no test requests a /graphql path. Fall-through in
    hono-wildcard-fallthrough.test.ts is driven entirely by handled: false returns from
    handleAuth/dispatch on /auth/* paths.
  • The repo already shows the correct shape./storage is a genuinely retired mount
    (dispatcher 的 /storage/upload 用 upload(file, {request}) 调用契约里的 upload(key, data, options?) —— 对任何实现都会 TypeError #4087) whose fall-through is pinned — by POST /api/storage/upload/presigned reaches the catch-all, not a storage mount, a request-level test that needs no handleStorage
    double
    . That is the in-repo precedent for how an unrouted domain is demonstrated.

So /graphql had the opposite of what it needed: an inert double and no test. This PR adds
POST /api/graphql reaches the catch-all — GraphQL is not a routed domain, modelled on the
/storage case.

handleMetadata and handleData stay in src/__mocks__/runtime.ts — both still exist on
the real dispatcher (http-dispatcher.ts:1677, :1682), so they are not residue. A stub may
declare more than this adapter calls; it may not declare what the subject lacks. That
invariant is now written at the top of the stub.

Reverse verification — both directions, on a rebuild-free path

These tests import the subject as source (from './index') and alias @objectstack/runtime
to a source stub; the package has no dist/ at all, so the mutation on disk is what runs
and no rebuild can hide a leg. Every mutation was confirmed on disk by grep count, never by an
editor's exit code.

1. The new test fails for the right reason. Ablation: re-introduce a /graphql mount
ahead of the catch-all (app.all(`${prefix}/graphql`, …)) — the exact regression it guards.

baseline EXIT=0 Tests 74 passed (74)
+ /graphql mount EXIT=1 Tests 1 failed | 73 passed (74)
× POST /api/graphql reaches the catch-all — GraphQL is not a routed domain
restored (marker count 0) EXIT=0 Tests 74 passed (74)

Exactly one failure, and it is the new test.

2. The removed doubles really were unreachable — proved positively, not by absence of
evidence. mockDispatcher was wrapped in a Proxy that throws on any string property outside
the allowed set:

Probe A allow {getDiscoveryInfo, handleAuth, dispatch} EXIT=0 74 passed probe trips: 0
Probe B CONTROL: same guard, 'dispatch' removed EXIT=1 41 failed probe trips: 14

Probe A green with zero trips ⇒ nothing ever reads a fourth property, so handleGraphQL could
never have been observed. Probe B is the control proving the instrument can detect a read — a
green Probe A would otherwise be indistinguishable from a dead probe.

Gates

Derived with node scripts/pm/dispatch-gates.mjs (no paths passed — it takes its own change
set from the merge base) at d2375ea9c1, which is the final commit. All green:

GateIts own verdict line
check:cross-package-test-inputsOK: 13 package(s) read outside themselves, all declared, and turbo.json hashes every declared glob.
check:kernel-hook-pairs✓ kernel hook pin pairing: 4 dispatched kernel:* hook(s), each pinned in both kernel.test.ts and lite-kernel.test.ts
check:slot-lookup✓ slot-lookup ratchet holds: 107 unswept site(s) in 25 file(s), none new
check:test-source-aliascheck-test-source-alias OK — 72 packages with tests scanned
check:type-source-resolutioncheck-type-source-resolution OK — 76 packages with a tsconfig.json scanned
check-ci-filter-parity.mjsOK: all 82 declared cross-package glob(s) (71 unique) are covered
check-cross-package-test-inputs.mjsOK: 13 package(s) read outside themselves, all declared
check-plugin-teardown-shape.mjs✓ check:plugin-teardown-shape: 57 Plugin implementation(s) across 4390 source(s)
check-affected-docs.mjsexit 0
check:query-options-erasure✓ query-options-erasure ratchet holds: 67 unswept non-test site(s) in 17 file(s), none new
check:engine-double-contractcheck-engine-double-contract: OK — 371 pinned, 133 in the DEBT ledger, 2 exempt.
check:where-matcher✓ where-matcher conformance holds: 272 matcher(s) discovered, 272 answer the combinator battery correctly or refuse it loudly
check:type-check-coveragecheck-type-check-coverage: OK — 64/77 workspace packages type-checked
check:nul-bytescheck-nul-bytes: OK (scanned 6266 text file(s) … no raw ASCII control bytes)

Suite: pnpm --filter @objectstack/hono test --maxWorkers=2Test Files 2 passed (2),
Tests 74 passed (74), EXIT=0.

One declared narrowing.check:type-check-debt --re-measure wants the whole workspace
closure built. Instead of that full build I measured the only two ledger entries this diff can
reach. @objectstack/hono (errors: 3, "all code-tier (TS2769/TS18046)") — its tsconfig is
include: ["src/**/*"], so test files really are in the program:

with this change 3 errors (1 × TS18046, 2 × TS2769)
origin/main baseline 3 errors (1 × TS18046, 2 × TS2769)

Unmoved, same composition as the ledger records. @objectstack/core's entry cannot move
either — that change is comment-only, and comments do not participate in type checking. CI
runs the full re-measure regardless.

No changeset

Real file list: three test/mock files under packages/adapters/hono/src/ and one
comment-only change in packages/core/src/security/anonymous-deny.ts. No behavioural or
API change in either published package, so there is nothing to describe in release notes;
skip-changeset applied.

Serial constraints

PR #10824 (perf(security,protocol)) does not hold anonymous-deny.ts — checked
against its actual file list, which is .changeset/count-opt-out-and-permission-set-memo.md,
packages/metadata-protocol/src/{protocol.ts, protocol.count-opt-out.test.ts} and
packages/plugins/plugin-security/src/{security-plugin.ts, permission-set-resolution-memo.test.ts}.
Disjoint. PR #10828 is content/docs/** only — also disjoint. Rebased onto 37ba31a4b8;
the five commits main gained mid-run touch none of my four files.


Generated by Claude Code

…t test doubles
Two halves of #10835, deliberately different in kind.
anonymous-deny.ts named three Phase-1 surfaces, two of them long gone:
/graphql (removed with the GraphQL surface, #2462 follow-on) and the
raw-hono /data routes (deleted as a duplicate surface in v17, #4073).
Replaced the inline route list with the two corrections and a pointer to
the authz-conformance ratchet, which enumerates the live entry points
from source. Comment-only; no code touched.
The three handleGraphQL test doubles were inert, not deliberate: the
hono adapter calls exactly getDiscoveryInfo, handleAuth and dispatch, so
a fourth key on the mock is unreachable from index.ts. Removed, and
replaced with a request-level test that /api/graphql reaches the
catch-all — the same shape the retired /storage mount is pinned with,
which needs no handleStorage double either.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DdCnBGcHeufjrq7drTD3wt
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

Nothing in this diff resolved to a documentable surface (no symbol, route or SDK anchor derived from 1 changed package(s)), so this run has no opinion about the docs.

What this run could not see
  • 1 changed file(s) yielded no anchor (packages/core/src/security/anonymous-deny.ts) — pages documenting those are invisible to this run

Coarse fallback — 23 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 23502649659115a98a80544f4b80f7bf21568601packageMentionDocs.

@claude

claudeBot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

✅ ACCEPT — reviewer of record: domain:devx PM seat (#6023, session session_01DdCnBGcHeufjrq7drTD3wt). Reviewed against the diff, not the report.

⭐ Half 2 was decided by measurement, and the measurement had a control

This dispatch's one real question was whether the handleGraphQL doubles were residue or load-bearing — the card's own counter-argument being that a wildcard fall-through test may want a method the dispatcher does not route. You did not wave it off, and you did not settle it by absence of evidence:

probeallow-setresultreads detected
AgetDiscoveryInfo, handleAuth, dispatch74 passed, exit 00
B — controlsame, minus dispatch41 failed, exit 114

Probe A alone would have been a dead-probe risk: green because nothing reads a fourth property, or green because the instrument is broken. Probe B is what makes A a measurement. That is the discipline this lane keeps asking for and rarely gets — proving an absence positively.

Verified independently: packages/adapters/hono/src/index.ts calls exactly three dispatcher methods — getDiscoveryInfo (:300, :304), handleAuth (:413), dispatch (:463). No handleGraphQL.

⭐ And the decisive evidence came from inside the repo rather than from argument: /storage is a genuinely retired mount whose fall-through is pinned — by a request-level test that needs no handleStorage double. So /graphql had the inverse of what it needed: an inert double and no test. Removing the three and adding POST /api/graphql reaches the catch-all modelled on the /storage case converts a green-by-construction stub into an actual assertion.

The invariant you wrote into the stub is the right generalisation, and correctly excludes handleMetadata/handleData (both still on the real dispatcher, http-dispatcher.ts:1677/:1682): a stub may declare more than one consumer calls; it may not declare what the subject lacks.

Half 1

Comment-only — verified mechanically: 0 non-comment changed lines in anonymous-deny.ts. Rather than substituting a fresh route list that would rot identically, the comment defers to the authz-conformance ratchet, which enumerates live entry points from source. That is the better answer than the one my briefing implied ("correct the comment to name the surfaces that actually exist") — a hand-kept list is what created this card.

skip-changeset is right and I verified it rather than taking it: both packages publish only dist, README.md, CHANGELOG.md, so neither the test files nor __mocks__ nor a docblock reaches an npm consumer. I applied the label from the PM seat while you were finishing; you then read it back and correctly declined to whole-set PUT — which would have erased size/s and tests. Both halves of that behaved.

⚠️ My briefing was wrong, and it was wrong in the exact way I warn about

I wrote pnpm --filter @objectstack/adapter-hono test. The package is @objectstack/hono. Reproduced live from the PM seat:

$ pnpm --filter @objectstack/adapter-hono test --maxWorkers=2
No projects matched the filters in "/home/user/objectstack"
$ echo $?
0

Exit 0. Every discipline that makes a run trustworthy — capture the code before any pipe, quote the gate's own verdict — faithfully reports success for a run that never happened. It composes with the ---separator trap already on the books: both end at exit 0, nothing measured, output that reads like success.

⭐ You caught it on your own initiative; my briefing never told you to check that the filter matched. Had you trusted the exit code, your entire test section would have been fiction with genuine exit codes in it.

Filed as #10853, including the measured fact that no guard exists for this anywhere in scripts/ or .github/workflows/. Fixing the template example is named there as governed work, not done here.

Flipping ready and arming as soon as the GraphQL quota returns (~14:16Z) — ⛔ no manual merge.


Generated by Claude Code

@os-zhuang
os-zhuang marked this pull request as ready for review August 21, 2026 14:18
@os-zhuang
os-zhuang added this pull request to the merge queueAug 21, 2026
Merged via the queue into main with commit fbcfb55Aug 21, 2026
34 of 35 checks passed
@os-zhuang
os-zhuang deleted the claude/issue-10835-graphql-double-residue branch August 21, 2026 14:34
@github-actions

Copy link
Copy Markdown
Contributor

⛔ merge queue 构建失败 — 先分诊,再决定要不要重排

队列构建 32491504511 红了。队列跑的是全量套件(PR 侧 CI 只跑 affected 子集),
所以失败的测试可能在本 PR 没碰过的包里 —— 那不是重排能修的。每次盲目重排都会让排在后面的所有 PR 重建一轮。

失败的 job(日志抽取,best effort):

  • Console Pin Gate — 失败步骤: Build the Console SPA at the pinned objectui SHA

    ✗ Build failed in 5.81s
    

↳ 失败原因 是判读的关键:超时Test timed out in … / Hook timed out in …)多半是负载/时序,不是本 PR 的回归;
断言AssertionError: …)才指向真实的行为改变。两者的 FAIL 行长得一模一样,只有这一行能区分。

跨 PR 相同签名(24h,按失败测试文件聚合):

  • ⚠️本次没有可用的聚合签名(日志里没有能解析出测试文件名的 FAIL 行)—— 这不是「没有同签名的其他 PR」,是这一轮没测到。跨 PR 聚合本次不可用,请手工比对其他 PR 的同类评论。
  • ⚠️ 24h 评论账本没读完(超过 5 页仍未读到窗口尽头),所以上面的「不同 PR 数」是下界,不是全量。

历史信号:

  • 本 PR 过去 24h 无队列失败记录(首次)。
  • 过去 24h 队列共有 14 个失败构建(不含本次)。

分诊清单:

  1. 失败测试在本 PR 改动的包里 → 真回归,修 PR。
  2. 失败测试与本 PR 无关 → 看上面的「跨 PR 相同签名」;已有汇总 issue ⇒ flaky/环境问题实锤,去那张 issue 上谈,修好前重排只会再烧一轮全队列。
  3. 两者都不是 → 可能与同组 PR 语义冲突;等前面的 PR 落地或失败出队后再重排一次即可,不要连续重排。

Generated by Claude Code · merge-queue-triage workflow (#4859)

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/sskip-changesetPR has no user-facing published change; bypasses the changeset gatetests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[finding] handleGraphQL test doubles and a stale anonymous-deny comment outlive the removed /graphql route

2 participants

@os-zhuang@claude