Repository navigation
refactor(service): split service.ts into platform leaves - #4492
Conversation
Pure-move split of src/service.ts (5558 lines) into src/service/ leaves: state (paths/install-state), guards (ownership/auth/manager-command), health (port resolution + serving checks), launchd, systemd, windows-scheduler (schtasks + elevation finalize), windows-taskxml (task XML build/verify), windows-ops (Windows install/stop/uninstall + stage tracking), repair, orchestration (install/uninstall/stop dispatch), diagnostics, cli (arg parsing/dispatch). src/service.ts is now a facade re-exporting every original export. One semantic-preserving adjustment: import.meta.dir at the old file depth is carried by serviceSourceDir in state.ts.
|
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. |
|
github-actions
Bot
added
intake: hygiene-blocked
✅ READY
Hygiene✅ Deterministic PR hygiene checks passed. |
|
This PR is a pure-move refactor with zero behavior change: the original file stays as a facade re-exporting every name it exported before, so no consumer and no test is edited. The hygiene gate counts any non-comment change under Adding a new focused test here would be the wrong artifact. The oracle for a pure move is the existing suite for the moved subsystem, unchanged, which is exactly what proves the relocation preserved behavior. Those suites run on the stack tip (#4493). Fidelity was verified statically instead, and the checks found real defects rather than rubber-stamping: every baseline top-level declaration resolves to exactly one leaf with a comment-normalized identical body, the facade's export set matches the baseline exactly, every module-level mutable binding and test hook has a single declaration site, and every relative import specifier resolves. Local product suite, typecheck, and build were NOT RUN by operator instruction. |
lidge-jun
added
the
test-exception-approved
리뷰 · 우선순위 66 / 80설명 여기서 제일 위험한 실패는 "테스트 훅 싱글톤이 둘로 갈라지는 것"입니다. 타입체크로는 안 잡힙니다. PR은 한 줄은 일부러 바꿨습니다. 원래 세 곳이 구조 문서도 같이 갑니다. 우선순위 66은 cleanup(#4491)보다 파일이 크고 플랫폼 훅이 많아서 실수 비용이 더 크기 때문입니다. 그래도 사용자 기능 추가는 아니고, open-PR 경합 0인 타이밍의 분할이라 전략 점수는 높습니다. base는 #4491 브랜치(
심볼 테스트 훅 네 개 - 선언이 한곳인지,
심볼 hygiene 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
github-actions
Bot
removed
the
intake: hygiene-blocked
![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: 11e11795cf
ℹ️ 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".
| import { writeServiceDefinitionFile } from "./windows-ops"; | ||
|
|
||
| /** The `--port <n>` baked into the installed systemd user unit. Linux only. */ | ||
| export function systemdListenPort(deps: { readUnit?: () => string } = {}): number | null { |
There was a problem hiding this comment.
After this split, a subsequent change confined to src/service/systemd.ts or another src/service/*.ts leaf will not match either the pull-request/push filters in .github/workflows/service-lifecycle.yml:6-34 or the release gate regex in .github/workflows/release.yml:223-225, because both recognize only the exact src/service.ts path. Such service changes will therefore skip the cross-platform lifecycle workflow, and a release will not require a successful lifecycle run for them; add src/service/** consistently to both mechanisms.
Useful? React with 👍 / 👎.
| process.exitCode = 1; | ||
| } | ||
| } | ||
| rem |
There was a problem hiding this comment.
Making src/service.ts a barrel leaves existing source-oracle tests reading a file that no longer contains the implementation they assert on. Focused runs confirmed failures in tests/server/startup-prompt.test.ts:191, tests/service/systemd-install-cleanup-hardening.test.ts:6, and tests/windows/windows-deploy-close-regressions.test.ts:68; many additional assertions in tests/service/service.test.ts use the same path. Retarget each oracle to the relevant leaf module, or aggregate the leaf sources explicitly, so the full test gate is not unconditionally red and the assertions remain effective.
AGENTS.md reference: AGENTS.md:L220-L225
Useful? React with 👍 / 👎.
…eaves [skip ci] Hosted CI on the stack tip went red because source-contract tests still read the 30-line facade at src/service.ts. Point each assertion at the owning leaf so the existing oracles keep their original strings, slices, and negative matches. Import-specifier depth is the only assertion text that changed, because the leaves sit one directory lower.
github-actions
Bot
removed
the
test-exception-approved
lidge-jun
added
the
test-exception-approved
… [skip ci] Hosted CI shard 2/4 still read src/service.ts from launchd-repair.test.ts. Point each slice at the owning leaf and keep the original assertion strings.
github-actions
Bot
removed
the
test-exception-approved
lidge-jun
added
the
test-exception-approved
Summary
src/service.tswas 5558 lines holding every OS service backend at once: launchd, systemd, the Windows Scheduler pipeline, Windows task XML generation, ownership and auth guards, health probing, install/uninstall/repair orchestration, diagnostics, and CLI argument parsing. This splits it into twelve leaves undersrc/service/and leaves the original path as a 30-line facade re-exporting all 141 previously exported names, so no consumer and none of the eight test files that name the path is edited.The four module-level test seams stay single-instance, which is the invariant that matters most here:
querySchtasksForTestsandfinalizeHooksinwindows-scheduler.ts,ownedWindowsSchedulerStagesinwindows-ops.ts, anduninstallServiceHooksForTestsinorchestration.ts. Each has exactly one declaration site and every reader imports from that module, so thesetXForTestshelpers thattests/service/service.test.tsresets inafterEachstill reach the binding production code reads.One line is not verbatim and is deliberate. Three sites resolved paths from
import.meta.dir, which changes meaning one directory down.state.tsnow exportsserviceSourceDir = dirname(import.meta.dir)and those three sites consume it, reproducing the originalsrc/anchoring exactly.structure/manifest.jsongainssrc/service/inruntime.md'sdocumentslist andstructure/runtime.mdnames the new leaves, becausescripts/structure-ssot.tsfails on a new top-levelsrc/area that no doc claims and names.Second of a three-PR stack, on top of the
storage/cleanup.tssplit.Verification
serviceSourceDirsubstitution described above.src/lab/import.constants as fsConstantswas dropped fromstate.ts'snode:fsimport.Checklist