[WRONG BRANCH] fix(tests): walk the Lab activation chain instead of its first hop (#4704) - #4754
Conversation
…4704) [skip ci] The synchrony guard followed the direct callees of startServer. That catches activateLab becoming async, but activateLab calls installLabAutomationRuntime and startAutomationIfEnabled without awaiting them, so making either async with an await before its registration call left activateLab parsing as perfectly synchronous. Every assertion stayed green while startServer returned before Lab was registered, which is the one ordering the window exists to protect. walkActivationChain now follows the chain from activateLab to a bounded depth, failing on any node that is declared async or carries a body-level await. What keeps a recursive walk from becoming the false-positive machine the depth-one comment warned about is what it refuses to follow. Nested functions are already skipped by collectBodyLevelCalls, so timer callbacks, shutdown hooks, promise continuations and the deferred route executor are not activation edges. Receiver calls are not followed: a method that turns async cannot suspend its caller unless the caller awaits it, and that await is reported on the caller's own body. Names imported from outside this repository are classified automatically, so the six names that need human judgement are not buried under every join and readFileSync in the chain. inspectActivationDeclaration extends inspection to const arrows. The function-only inspector reported activationKey as missing, and a walk that read missing as fine would skip every const-arrow node. That needed its own return-type skipper: the existing one treats a top-level => as part of a function-type annotation and keeps scanning for a body brace a concise arrow never has. Four mutation cases run against the real sources through an injected loader: a nested callee declared async, an await added inside that callee, a suspension three hops down in startLabAutomationScheduler, and the arrow forms the previous inspector could not see. The async case also asserts what the depth-one scan reports on the same mutated source -- still green, which is the defect. Closes #4704
|
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 |
|
✅ Deterministic PR hygiene checks passed. |
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. |
…4706) [skip ci] GENERATED_PATHS exempted twelve files from every size cap, and eleven of them were hand-maintained: nine i18n catalogues, a hand-curated benchmarks snapshot, and the model-metadata generator's INPUT. Its output, src/generated/model- metadata.ts, is 108 lines and was scanned normally, so the one file the list was named after was the one file it did not describe. Exempting catalogues and data snapshots is a reasonable policy. Calling them generated is what invites the next hand-written file onto the list, because a name is a claim nothing checks. The exemption is now three exact allowlists, each carrying its own reason. GENERATED_PATHS holds only agent_pb.ts, which opens with a protoc-gen-es banner. I18N_CATALOG_PATHS holds the nine locale catalogues, exempt because they grow by one line per UI string in nine locales at once, so a cap would block every new string in the GUI rather than any oversized module. DATA_SNAPSHOT_PATHS holds the two records whose size tracks how much was recorded. EXEMPT_PATHS is their union, and the verdict and baseline field are renamed from GENERATED to EXEMPT to match. loadBaseline still accepts the old key so a branch written before the rename loads instead of failing with a shape error that explains nothing. The classification is now checkable against the files themselves: every path in GENERATED_PATHS must carry a generator banner, and no path in the other two lists may. The positive control is the generator's real output, which carries the banner, is not exempt, and is scanned under a cap. The exemption relies on --update never turning an exemption into a cap, so that is asserted directly alongside the Math.min rule that only ever lowers one. Closes #4706
![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: 21a3f18ace
ℹ️ 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".
| ]; | ||
| while (queue.length > 0) { | ||
| const item = queue.shift()!; | ||
| if (nodes.has(item.name)) continue; |
There was a problem hiding this comment.
The visited check conflates same-named functions from different modules. The current graph already reaches both src/lab/automation/persistence.ts and config-persistence.ts, which each declare sleepLockRetry and pidDefinitelyDead; because the persistence declaration is visited first, changing the config-persistence sleepLockRetry to an async function with a body-level await still produces failures: []. Key nodes by resolved file plus declaration/exported name so every distinct activation callee is inspected.
AGENTS.md reference: AGENTS.md:L74-L82
Useful? React with 👍 / 👎.
…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
리뷰 · 우선순위 78 / 80이 PR은 Lab 동기 활성화 가드가 베이스는
메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
|
Cascading downward. The lab synchronous-activation guard now walks the activation chain recursively instead of stopping one hop after startServer, with boundaries that keep it from reporting nested callbacks and receiver calls that cannot suspend the caller. 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 |
github-actions
Bot
changed the title
⏳ DRAFT
What to do
Its title has been prefixed with |
1 participant
Summary
The synchrony guard followed the direct callees of
startServer. That catchesactivateLabbecoming async, butactivateLabcallsinstallLabAutomationRuntimeandstartAutomationIfEnabledwithout awaiting them (src/lib/lab-activation.ts:176,200), so making either one async with an await before its registration call leavesactivateLabparsing as perfectly synchronous. Every assertion stayed green whilestartServerreturned before Lab was registered — the one ordering the window exists to protect, and the reason AGENTS.md pins it.walkActivationChainnow follows the chain fromactivateLabto a bounded depth, failing on any node declared async or carrying a body-level await. At the current tip it reaches 61 nodes with zero failures.The depth-one comment warned that a deeper walk would treat dynamic dispatch as startup callees and produce false positives. What avoids that is what this walk refuses to follow:
collectBodyLevelCalls, so timer callbacks, shutdown hooks, promise continuations and the deferred route executor are not activation edges. They run later by construction.Map.getorrecord.staticDetach.pushwould add noise and catch nothing.joinandreadFileSyncin the chain would bury the six names that actually need human judgement, which are listed with a reason each and asserted to still be reachable.inspectActivationDeclarationextends inspection to const arrows. The function-only inspector reportedactivationKeyas missing, and a walk that read missing as fine would skip every const-arrow node in the chain. That needed its own return-type skipper: the existingskipReturnTypetreats a top-level=>as part of a function-type annotation and keeps scanning for a body brace that a concise arrow never has.The walk is bounded three ways: a 200-node ceiling that fails rather than running away, a visited set, and an eight-hop limit on re-export resolution.
Closes #4704
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:
body: " a"value asserted for the arrow inspector.src/lib/lab-activation.tsandsrc/lab/automation/orchestrator.ts: a nested callee declared async, an await added inside that callee, a suspension three hops down instartLabAutomationScheduler, and the arrow forms. Each is reported; on the same mutated source the depth-one scan still returns{ found: true, async: false, awaitLines: [] }foractivateLab, which is the defect being fixed.[skip ci]under the maintainer-approved DEV-STACK-08 tip-only policy.Checklist