[WRONG BRANCH] docs(architecture): name the modules that own the code after the facade splits (#4711) - #4766
Conversation
…de splits (#4711) The architecture pages and structure/runtime.md still described the module ownership that existed before the facade splits of the last release train, so a reader following them landed in a file that no longer contains the code. Routing now lives in src/server/index/serve-options.ts, request preparation in src/server/responses/request-prepare.ts, and bridge conversion in src/bridge/ sse.ts and src/bridge/response-json.ts. src/bridge.ts is seven lines of re-exports. The pages named the pre-split files in every locale, so fixing only the English source would have left seven translations contradicting it. The facade paragraph also carried three counts -- seven, nine and five leaf modules -- that were already 26, 42 and 53. Counts that must be recounted on every split are a drift source rather than information, so they are gone; what replaces them is the distinction the counts were standing in for. A facade is the stable import path, not the implementation, and each step of the request flow now names the module that owns the code. structure/runtime.md gets the same correction on five ownership claims, and points at structure/transports/responses.md, which already carries the post-split owner inventory for the Responses surface. Scope is deliberately narrow: only claims about which module owns which responsibility change. No prose about behaviour is rewritten, and no locale receives a translation it did not already have. Closes #4711
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
리뷰 · 우선순위 77 / 80이 PR은 아키텍처 문서와 고치는 방식은 좁고 정확합니다. 영어 runtime 쪽은 더 세밀합니다. facade 문단에서 예전에 박아 둔 leaf 개수(7·9·5)를 지운 것도 좋습니다. 지금 checkout만 봐도 베이스는 architecture.md:1단계 - Live sideband WebSocket join까지 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf851137ef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Adapter output must stay in internal `AdapterEvent` form until `src/bridge/sse.ts` converts it back | ||
| to Responses SSE or WebSocket frames, or `src/bridge/response-json.ts` buffers it into a JSON |
There was a problem hiding this comment.
For the client-facing Responses WebSocket path, src/server/index/websocket-handler.ts passes the SSE response to sendResponseToWebSocket, and src/server/ws-bridge.ts parses those SSE blocks and sends their payloads as WebSocket frames. src/bridge/sse.ts always produces SSE, so assigning both SSE and WebSocket conversion to it gives maintainers the wrong implementation owner; credit src/server/ws-bridge.ts for the WebSocket framing step.
AGENTS.md reference: structure/AGENTS.md:L7-L10
Useful? React with 👍 / 👎.
| history, while `responses/parser.ts` and `bridge/sse.ts` handle remote compaction v2 | ||
| `compaction_trigger` turns by emitting exactly one synthetic `compaction` output item. |
There was a problem hiding this comment.
When a routed compaction_trigger request uses stream: false, the buffered branch in src/server/responses/adapter-delivery.ts calls buildResponseJSON(..., { compaction: true }), and src/bridge/response-json.ts emits the synthetic compaction item. Naming only bridge/sse.ts therefore makes this ownership claim false for non-streaming callers; include bridge/response-json.ts here and in the translated copies.
AGENTS.md reference: docs-site/AGENTS.md:L7-L10
Useful? React with 👍 / 👎.
|
✅ Deterministic PR hygiene checks passed. |
github-actions
Bot
added
the
documentation
|
Landing the repository guard lane, top layer first. Architecture pages now describe the post-facade module ownership, and the facade paragraph drops counts that have to be recounted after every split. Evidence at the verified tip baaf691 (tree
Chained-child stacks merge top-down, so this lands in the parent branch and cascades to Maintainer integration decision under MAINTAINERS.md / AGENTS.md: a maintainer with maintain or admin access may integrate into |
3b52c0a
into
codex/rg3-ratchet-generated-exemptions
github-actions
Bot
changed the title
⏳ DRAFT
What to do
Its title has been prefixed with |
…docs docs(architecture): name the modules that own the code after the facade splits (lidge-jun#4711)
1 participant
Summary
The architecture pages and
structure/runtime.mdstill described the module ownership that existed before the facade splits of the last release train, so a reader following them landed in a file that no longer contains the code.server/index.tsfor route ordering and the Live sidebandsrc/server/index/serve-options.ts, withsrc/server/index/live-sideband.tsfor the bounded upstream dialserver/responses/core.tsfor body parsing and replay expansionsrc/server/responses/request-prepare.tsserver/responses/core.tsfor hosted-search binding and helper admissionsrc/server/responses/passthrough-delivery.ts,src/server/responses/request-sidecar-auth.tsbridge.tsfor SSE and JSON conversionsrc/bridge/sse.ts,src/bridge/response-json.tssrc/bridge.tsis seven lines of re-exports.server/index.tsstill owns the listener and the startup transaction, andresponses/core.tsis still the public ingress, so both keep their entries rather than disappearing — the correction is about which file holds the implementation.All seven translated locales named the same pre-split files, so fixing only the English source would have left them contradicting it. Each locale gets the same correction.
The facade paragraph also carried three counts — seven, nine and five leaf modules — that were already 26, 42 and 53. A count that must be recounted on every split is a drift source rather than information, so the counts are gone and what replaces them is the distinction they were standing in for: a facade is the stable import path, not the implementation, and each step of the request flow now names the module that owns the code. Every page points at
structure/transports/responses.md, which already carries the post-split owner inventory for that surface.Scope is deliberately narrow. Only claims about which module owns which responsibility change; no prose about behaviour is rewritten, and no locale receives a translation it did not already have.
structure/manifest.jsonis untouched, sostructure/INDEX.mdneeds no regeneration.Closes #4711
Verification
Local test suites, individual test files,
bun run typecheck,bun run test,bun run test:changed, the dashboard build and dependency installation were NOT RUN — the maintainer forbids local suite execution in this lane, since a local test run previously deleted real~/.opencodexdata.What was done instead, all of it read-only:
bun scripts/structure-ssot.tspasses (structure/ SSOT checks passed). That is the gate this change is most likely to break: it validates that every backticked repository path exists in the Git index, that relative links and anchors resolve, and that the doc-map, ownership and invariant bindings still hold.bun scripts/file-size-ratchet.tspasses against the working tree, which now includes every file this lane adds.bun run privacy:scanpasses andbun run skill:surface:checkreports the committed surface is current.structure/runtime.md; each page now has exactly two remainingbridge.tsmentions, both deliberate (the module map entry and the facade sentence), and zero remainingserver/index.tsorresponses/core.tsownership claims.git ls-filesbefore being written into a doc, sincestructure:checkresolves backticked paths against the index rather than the filesystem.This is the lane tip, so its CI run is the gate for all four layers. The CI run URL and conclusion are reported to the maintainer once it settles.
Checklist
Lane CI evidence (added after the run settled).
This layer's own diff is documentation only, so the
cipath filter inci.ymlskips the test matrix for apull_requestevent. A green aggregate check on this PR therefore proves nothing about the suite. The lane was gated with aworkflow_dispatchrun instead, which is not apull_requestevent and so runs every job against the cumulative tree:test 1/4,test 2/4,test 3/4,test 4/4— completed / success (executed, not skipped).macos 1/2,macos 2/2,macos control— success.windows 1/6throughwindows 6/6— success.gates,storage policy,api usage,docker smoke,keyring(ubuntu/macos/windows),npm-global(ubuntu/macos/windows) — success.origin/devwas absorbed into the bottom layer and cascaded up the chain, so each PR keeps only its own diff. The dispatch ran onc086fda3a7, whose tree is3728219b8b5c1758a768108e4b69b648fa0828f6; the current tipbaaf691fb4has the same tree, so the run remains the evidence for this head.windows 5/6failed once withEPERMinsideremoveTreeWithRetryand passed on re-run. That is the pre-existing Windows handle racesrc/config/paths.ts:57documents —icacls.exefromhardenConfigDir()holds the config directory open — and the retry loop exists to absorb it. The guard added in the first layer sits outside that loop and never threw; the rethrow came from the pre-existing 50-attempt exhaustion. The same code passed the same test in an earlier dispatch. The load-dependent retry budget is filed separately as #4789.