[Ingwannu]

Summary

• Keep both race-sensitive targeted schtasks /query /tn ... /xml ownership checks during startup.
• Reuse the expensive full Task Scheduler listing only when the fresh targeted result is byte-for-byte unchanged; changed evidence triggers a fresh fail-closed decision.
• Scope the memo to the synchronous pre-listen phase, leave later ownership retries fresh, and record the decision in structure/04_transports-and-sidecars.md.

Closes #2923

Verification

• Exact PR head 3bcd29770: bun test tests/codex-service-manager-probe-hardening.test.ts tests/codex-service-manager-probe.test.ts tests/native-profile-startup.test.ts --timeout 30000 — 112 pass, 0 fail.
• Exact PR head 3bcd29770: bun run typecheck — passed.
• Full suite before the final non-overlapping dev rebase: bun run test — 16,144 pass, 16 skip, 0 fail. The only two later base commits changed Codex identity-budget and pool replay tests; focused tests and typecheck were rerun after that rebase.
• All commands used isolated HOME, OPENCODEX_HOME, and CODEX_HOME, with CPU affinity and niceness limits.

Checklist

• Scope stays focused and avoids unrelated cleanup.
• Docs or release notes were updated when needed.
• Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

Summary by CodeRabbit

• Performance

  • Improved Windows startup times by reusing Task Scheduler results during startup ownership checks.
  • Avoided repeated scans when relevant task data has not changed.

• Bug Fixes

  • Improved detection when Windows tasks appear or change during startup.
  • Prevented stale results from being reused and maintained safe handling when task-listing checks fail.

• Documentation

  • Added documentation describing Windows startup ownership listing reuse and its scope.

[github-actions]

✅ Deterministic PR hygiene checks passed.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 43744486-9ff9-4c63-b64d-e9a9eb908217

📥 Commits

Reviewing files that changed from the base of the PR and between 3bcd297 and 3daf9c6.

📒 Files selected for processing (2)

• src/service-manager-probe.ts
• tests/codex-service-manager-probe-hardening.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review.

---

📝 Walkthrough

Walkthrough

The change adds a startup-local Windows Task Scheduler listing cache. Fresh targeted queries remain race-sensitive, while identical targeted results reuse the expensive full listing. Server wiring, documentation, and regression tests cover this behavior.

Changes

Windows startup ownership reuse

Layer / File(s)	Summary
Task listing cache implementation src/service-manager-probe.ts	Adds WindowsTaskListingCache. The cache keys listings by status, timeout and spawn flags, stdout, and stderr from the targeted query. Probe dependencies carry the cache through the Windows ownership path.
Startup cache wiring src/integrations/native/ownership-preflight.ts, src/server/index.ts	startServer creates one cache and passes it to both synchronous startup ownership checks. Runtime ownership retries do not receive it.
Behavior validation and documentation tests/codex-service-manager-probe-hardening.test.ts, structure/04_transports-and-sidecars.md	Tests cover listing reuse, changed targeted evidence, task appearance between checks, and cached listing failures. Documentation describes the cache scope and race-check behavior.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Merge Risk: 🟡 Moderate · up to 3daf9

The change can reuse an older Task Scheduler listing after a refresh for a different task identity fails, so a Windows startup ownership check may make a decision from stale evidence. Merge should wait for cache invalidation on failed refresh or explicit owner acceptance of this bounded security risk.

Sequence Diagram(s)

sequenceDiagram
  participant startServer
  participant inspectStartupOwnership
  participant inspectNativeCodexOwnership
  participant WindowsTaskListingCache
  participant TaskScheduler
  startServer->>inspectStartupOwnership: run startup ownership check
  inspectStartupOwnership->>inspectNativeCodexOwnership: pass startup cache
  inspectNativeCodexOwnership->>TaskScheduler: run fresh targeted /xml query
  TaskScheduler-->>inspectNativeCodexOwnership: targeted probe result
  inspectNativeCodexOwnership->>WindowsTaskListingCache: getOrRun(targeted result, full listing)
  WindowsTaskListingCache->>TaskScheduler: run /fo CSV listing when key changes
  TaskScheduler-->>WindowsTaskListingCache: full listing result
  WindowsTaskListingCache-->>inspectNativeCodexOwnership: cached or fresh listing
  inspectNativeCodexOwnership-->>startServer: ownership decision

Loading

Suggested reviewers: lidge-jun

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 22.22% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 4 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: reusing an unchanged Windows startup task listing.
Linked Issues check	✅ Passed	The changes satisfy issue #2923. They preserve both fail-closed ownership checks and bounded targeted queries, reuse only successful full listings when targeted evidence is byte-for-byte unchanged, re…
Out of Scope Changes check	✅ Passed	The changes remain within issue #2923. The implementation, focused regression tests, cache export, and design documentation support startup task-listing reuse. No service installation or GUI behavior …

Full details: Linked Issues check

Explanation

The changes satisfy issue #2923. They preserve both fail-closed ownership checks and bounded targeted queries, reuse only successful full listings when targeted evidence is byte-for-byte unchanged, retry failed or timed-out listings, limit caching to synchronous pre-listen startup, and add regression tests for revalidation, changed evidence, task appearance, and recovery.

Full details: Out of Scope Changes check

Explanation

The changes remain within issue #2923. The implementation, focused regression tests, cache export, and design documentation support startup task-listing reuse. No service installation or GUI behavior changes are present.

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch ingw/fix-2923-schtasks-once

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[lidge-jun]

리뷰 · 우선순위 64 / 80

이 PR도 #2923 을 고친다. 지금 dev HEAD는 1d9b389c1 (#2926 Windows 신원 조회 한도 공유)이다. #2918 이 전체 schtasks 목록에 20초를 준 뒤, startServer 가 듣기 전에 소유권을 두 번 보면서 그 목록이 두 번 나간다. 위치는 src/server/index.ts 609행(시작 캐시 결정)과 788행(네이티브 생명주기 직전)이다. 815행의 듣기 이후 재시도는 이번 메모를 받지 않는다. 본문이 그 점을 명시했고, 코드도 그렇게 되어 있다.

고치는 방법은 #2927 과 같다. 호출자가 캐시를 만들고 두 번의 듣기 전 검사에만 넘긴다. 다른 점은 캐시 키다. #2927 은 성공한 목록의 작업 이름만 기억한다. 이 PR은 방금 끝난 대상 조회 /tn ... /xml 의 status, timedOut, spawnFailed, stdout, stderr 바이트가 완전히 같을 때만 목록 결과를 재사용한다. 대상 조회가 바뀌면 목록을 다시 뽑고, 실패도 같은 지문이면 재사용한다. src/integrations/native/ownership-preflight.ts 에서 createWindowsTaskListingCache 를 다시 내보내고, inspectStartupOwnership 의 테스트 시임에도 캐시를 넣는다. structure/04_transports-and-sidecars.md 에 결정 로그도 적었다. types/config 분리와는 무관하다.

테스트는 #2927 보다 한 층 더 올라간다. startServer 를 실제로 띄워서 대상 조회 2번·전체 목록 1번을 센다. 그래서 헬퍼가 아니라 서버 배선이 잠긴다. 사이에 작업이 생기면 absent 가 아니고, 대상 조회 에러가 바뀌면 목록을 다시 뽑는 것도 있다. 같은 버그를 lidge-jun 의 #2927 이 거의 동시에 열었다. 둘 다 dev 대상이고 enforce-target 도 통과했다. 한쪽에 넣으면 다른 쪽은 leftover 다. 미리보기 배포는 계획에 없다.

점수가 #2927 보다 낮은 이유는 품질이 떨어져서가 아니다. 경쟁 구현이고, 실패 재사용이 #2923 이 원한 방향과 어긋난다. 이슈는 추측으로 unknown 을 absent 로 바꾸지 말라고 했다. 동시에 한 번의 목록 정지가 시작 끝까지 쓰기를 거절하게 굳히는 것도 피하라고 #2927 이 변이를 빨갛게 만들었다. 이 PR의 getOrRun 은 타임아웃 목록도 저장한다. 테스트도 두 검사가 목록 한 번에 unknown, unknown 이 되길 기대한다. 듣기 전 25초를 줄이는 목표에는 맞지만, 첫 목록이 20초에 죽으면 두 번째 검사는 새 증거를 안 본다.

라인 - src/server/index.ts 609행과 788행 - 캐시는 이 두 번의 듣기 전 검사에만 들어간다. 815행 재시도에 안 넣는 선택은 #2927 과 같고 맞다.
라인 - createWindowsTaskListingCache.getOrRun - 대상 조회 지문이 같으면 멈춘 목록도 재사용한다. #2927 의 stalled listing is not cached 변이는 바로 이 분기를 빨갛게 만든다. 실패는 캐시하지 않는 편이 이슈와 더 맞다.
라인 - inspectStartupOwnership 테스트 시임 - { currentHomes, statePaths, windowsTaskListingCache } 를 주입 경로에도 넣는다. #2927 이 빠진 배선을 여기서는 잠갔다. 이 테스트는 가져갈 가치가 있다.
경로/심볼 - src/integrations/native/ownership-preflight.ts - 캐시 생성 함수를 다시 내보내지만 inspectNativeCodexOwnership 은 여전히 ...deps 로 프로브에 넘긴다. 재export는 필수는 아니다.
경로/심볼 - #2927 과 파일 겹침 - src/server/index.ts 와 src/service-manager-probe.ts 와 같은 테스트 파일을 같이 고친다. 둘 다 머지하면 캐시 필드 이름이 싸운다.

메인테이너의 판단이 필요한 지점

• #2927(이름 집합, 실패는 재시도)과 이 PR(대상 조회 지문, 실패도 재사용) 중 제품 계약을 어느 쪽으로 둘지.
• 이 PR의 startServer 배선 테스트를 #2927 에 이식하고 이 PR은 leftover 로 닫을지.
• 문서 structure/04_transports-and-sidecars.md 를 합친 구현 쪽에 남길지.

너의 추천
이 PR을 먼저 넣지 마라. #2927 을 남기고, 여기의 startServer 대상 조회 2번·목록 1번 테스트와 시임 전달만 #2927 에 옮겨라. #2927 이 합쳐지면 Landed via #2927 at <commit> 댓글과 landed-via-maintainer 라벨을 단 뒤 이 PR을 닫아라. 대상 조회 지문 키가 꼭 필요하면 실패 재사용만 뺀 뒤에 다시 논의하라. 둘 다 넣지 마라.

이 댓글은 grok-bot이 작성했습니다

[lidge-jun]

Reviewed at head 3bcd29770. The core design is better than the version I wrote and closed (#2927): keying reuse on the targeted query's exact bytes and status binds the absence proof to the evidence that produced it, rather than to a time window. changed targeted evidence cannot reuse an earlier absence listing covers the fail-open direction the issue warned about more directly than my version did. I want this one to land.

Two findings, one blocking.

Blocking: a stalled listing is cached, so one transient timeout becomes a sticky refusal

getOrRun stores the result unconditionally, including a timeout. a cached listing failure remains fail-closed asserts ["unknown", "unknown"] with fullListings === 1 — which reads as intended behavior, but it means the second inspection never retries. Since the targeted query is byte-identical on both passes (the identity check passes), a single 20 s stall poisons the rest of the startup: ownership cannot be proven, and admission refuses to write for the whole run even though the machine would have answered on the second try.

That is the direction that costs a user a working ocx sync, which is the failure #2914 was about in the first place.

Reproduced against your head:

expect(second.kind).toBe("absent")
Expected: "absent"
Received: "unknown"
listings= 1 secondKind= unknown

The fixture: same GBK targeted stderr on both passes, /fo times out on the first and succeeds on the second with a listing that does not contain our task. Expected is unknown then absent, with two listings.

Suggested fix — only cache a successful listing:

getOrRun(targetedQuery, run) {
  const nextIdentity = rawProbeIdentity(targetedQuery);
  if (result !== null && identity === nextIdentity) return result;
  const next = run();
  // A stall or failure is not evidence, and caching it turns one transient
  // timeout into an unprovable ownership for the rest of the startup.
  if (!next.timedOut && !next.spawnFailed && next.status === 0) {
    identity = nextIdentity;
    result = next;
  }
  return next;
}

Then retarget the existing test at ["unknown", "unknown"] with fullListings === 2 — still fail-closed on both passes, which is the invariant it was written for, while proving the retry happens.

Non-blocking: one test fails locally in a way CI will not show

one startup keeps two targeted queries but shares one unchanged full listing fails on my machine with ["foreign", "foreign"] instead of ["owned", "owned"]. That is the environment dependence OwnershipDeps.statePaths documents — the default state-path set includes the mirror under real homedir(), so the fixture reads my actual installation. Your startServer seam passes resolveServiceHomes but the inner inspectNativeCodexOwnership call does not inject statePaths, so it falls through to the default set.

Not blocking, because CI runners have no such installation and it passes there — but it makes the test unable to fail for the right reason on a developer machine. Injecting statePaths into that inner call would pin it.

Everything else checks out: 61 pass across codex-service-manager-probe and service-probe-docker, the targeted query still runs on both inspections, runtime retries correctly omit the memo, and the structure/ note records the scope. Fix the first finding and I will merge.

[lidge-jun]

I pushed the fix to your branch rather than make you round-trip on it — head is now 3daf9c64a. Revert it if you disagree with any of it.

The blocking finding. getOrRun now retains only a successful listing:

if (!next.timedOut && !next.spawnFailed && next.status === 0) {
  identity = nextIdentity;
  result = next;
}

I retargeted a cached listing failure remains fail-closed at fullListings === 2 — it still asserts ["unknown", "unknown"], so the invariant it was written for is intact, and it now also proves the retry happens. Added a listing that recovers after a stall is not masked by the failed one, where the targeted stderr is byte-identical on both passes so the identity check cannot be what forces the retry; only refusing to cache the stall can. Mutation-proven: restoring the unconditional store turns both red (Expected: 2, Received: 1 and Expected: "absent", Received: "unknown").

The non-blocking one, also fixed. one startup keeps two targeted queries... now pins statePaths: [join(configDir, "service-state.json")]. startServer derives that set from the sandbox home and the default homedir() mirror, so the case was reading my real ~/.opencodex/service-state.json and reporting foreign locally while passing on CI. It failed for me before the pin and passes after.

114 pass / 0 fail across codex-service-manager-probe-hardening, codex-service-manager-probe, service-probe-docker and native-profile-startup; bun x tsc --noEmit clean; privacy scan green. Merging once CI is green on the new head.

The byte-identity keying was the right call and it is why this landed instead of my #2927.