Skip to content

feat: add session supervisor config plumbing#212

Open
ndycode wants to merge 1 commit intodevfrom
git-clean/pr147-supervisor-plumbing
Open

feat: add session supervisor config plumbing#212
ndycode wants to merge 1 commit intodevfrom
git-clean/pr147-supervisor-plumbing

Conversation

@ndycode
Copy link
Owner

@ndycode ndycode commented Mar 21, 2026

Summary

  • extract the low-risk supervisor plumbing out of feat: add experimental Codex session supervisor #207 into its own PR
  • add the config/schema flag and abort-aware quota probe support needed by the supervisor runtime
  • cover the new config and quota abort behavior with focused tests

Validation

  • npm exec vitest run test/plugin-config.test.ts test/quota-probe.test.ts
  • npm run typecheck

Notes

note: greptile review for oc-chatgpt-multi-auth. cite files like lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.

Greptile Summary

this PR extracts the low-risk supervisor plumbing from #207 into a focused, reviewable unit. it adds the codexCliSessionSupervisor boolean config flag (schema + default + accessor) and wires abort-signal support into fetchCodexQuotaSnapshot, covering all yield points in the model-probe loop.

  • lib/schemas.ts / lib/config.ts: codexCliSessionSupervisor flag added consistently with all other boolean config accessors — synchronous, no filesystem I/O, no token exposure
  • lib/quota-probe.ts: signal?: AbortSignal added to ProbeCodexQuotaOptions; abort checks at loop entry, post-getCodexInstructions, post-unsupported-model, and in the catch block; outer signal is correctly forwarded to the inner AbortController and listener is removed in finally
  • test/plugin-config.test.ts: 5 existing snapshots updated + focused 4-case describe block for env/config precedence
  • test/quota-probe.test.ts: 4 new tests covering pre-aborted, mid-fetch abort, post-instructions abort, and abort-before-fallback scenarios
  • minor: consecutive throwIfQuotaProbeAborted calls at lines 347 and 349 have no yield point between them — the inner call is redundant; also the catch block re-throws whatever error was caught (not guaranteed to be an AbortError) when the outer signal is aborted, which may mislead callers that detect abort by error.name

Confidence Score: 5/5

  • safe to merge — all issues are non-blocking P2 style/polish items
  • core abort wiring is correct: signal forwarded to inner controller, cleaned up in finally, checked at every yield point. config plumbing is minimal and consistent with existing patterns. test coverage is comprehensive for both the new flag and the abort paths. remaining findings (redundant double-check, non-AbortError re-throw in a race, missing blank line) are style/polish only and don't affect correctness or the primary user path.
  • lib/quota-probe.ts — catch block error type and redundant abort check worth cleaning before the runtime wrapper PR lands on top

Important Files Changed

Filename Overview
lib/quota-probe.ts adds signal?: AbortSignal to ProbeCodexQuotaOptions and wires it through fetchCodexQuotaSnapshot with abort checks before each yield point; abort signal is correctly forwarded to the inner AbortController and cleaned up in finally; minor: redundant double-check at loop entry and possible non-AbortError surfacing when abort races a network failure
lib/config.ts adds codexCliSessionSupervisor: false default and getCodexCliSessionSupervisor accessor using resolveBooleanSetting — consistent with all other boolean config accessors; synchronous, no filesystem I/O, no token exposure
lib/schemas.ts adds codexCliSessionSupervisor: z.boolean().optional() to PluginConfigSchema — minimal, correct, consistent with adjacent boolean flags
test/plugin-config.test.ts updates 5 existing snapshot assertions and adds a focused 4-case describe block covering default, config-only, env-disable, and env-enable paths for getCodexCliSessionSupervisor; all env keys properly cleaned up via delete process.env
test/quota-probe.test.ts adds 4 new abort-signal tests covering: mid-fetch abort, pre-aborted signal, post-instructions abort, and abort-before-fallback; good coverage of the new control flow; minor: missing blank line before pre-existing test at line 299

Sequence Diagram

sequenceDiagram
    participant Supervisor
    participant fetchCodexQuotaSnapshot
    participant AbortController (inner)
    participant fetch

    Supervisor->>fetchCodexQuotaSnapshot: call(options.signal)
    loop for each model
        fetchCodexQuotaSnapshot->>fetchCodexQuotaSnapshot: throwIfAborted(signal) [loop guard]
        fetchCodexQuotaSnapshot->>fetchCodexQuotaSnapshot: await getCodexInstructions(model)
        fetchCodexQuotaSnapshot->>fetchCodexQuotaSnapshot: throwIfAborted(signal) [post-instructions]
        fetchCodexQuotaSnapshot->>AbortController (inner): new AbortController()
        alt signal already aborted
            fetchCodexQuotaSnapshot->>AbortController (inner): controller.abort(signal.reason)
        else
            Supervisor-->>fetchCodexQuotaSnapshot: signal "abort" event → onAbort()
            fetchCodexQuotaSnapshot->>AbortController (inner): controller.abort(signal.reason)
        end
        fetchCodexQuotaSnapshot->>fetch: fetch(url, {signal: controller.signal})
        alt fetch succeeds
            fetch-->>fetchCodexQuotaSnapshot: Response
            fetchCodexQuotaSnapshot->>fetchCodexQuotaSnapshot: clearTimeout + removeEventListener
            fetchCodexQuotaSnapshot-->>Supervisor: CodexQuotaSnapshot
        else fetch aborted / timeout
            fetch-->>fetchCodexQuotaSnapshot: AbortError
            fetchCodexQuotaSnapshot->>fetchCodexQuotaSnapshot: catch: if signal.aborted → rethrow
            fetchCodexQuotaSnapshot-->>Supervisor: AbortError
        end
    end
Loading

Fix All in Codex

Prompt To Fix All With AI
This is a comment left during a code review.
Path: lib/quota-probe.ts
Line: 346-350

Comment:
**Redundant consecutive abort check (same microtask tick)**

lines 347 and 349 both call `throwIfQuotaProbeAborted` with no `await` between them. since there's no yield point between the two, the signal cannot change state between them. the one at line 347 (outside the `try`) already exits the loop by propagating — line 349 will never catch anything the outer check missed.

suggest keeping only the outer check (line 347) for loop-entry and removing line 349:

```suggestion
	for (const model of models) {
		throwIfQuotaProbeAborted(options.signal);
		try {
			const instructions = await getCodexInstructions(model);
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: lib/quota-probe.ts
Line: 432-435

Comment:
**Non-abort error may propagate when abort races a network failure**

when `options.signal` is aborted at the same time a genuine network error rejects `fetch`, the catch block hits `options.signal?.aborted === true` and re-throws the network error as-is (since it's an `Error` instance). callers that detect abort by checking `error.name === "AbortError"` will silently miss it and may log/retry unnecessarily.

wrapping the re-throw to always surface an `AbortError` when the outer signal is aborted makes the contract explicit:

```suggestion
		} catch (error) {
			if (options.signal?.aborted) {
				const isAbortError =
					error instanceof Error && error.name === "AbortError";
				throw isAbortError ? error : createAbortError("Quota probe aborted");
			}
			lastError = error instanceof Error ? error : new Error(String(error));
		}
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: test/quota-probe.test.ts
Line: 298-299

Comment:
**Missing blank line before pre-existing test case**

the last new `it(...)` block closes at line 298 and the pre-existing `it("parses reset-at…")` starts immediately at line 299 with no blank line separator. every other test in this file has a blank line between `it(...)` blocks — this makes the boundary harder to spot in diffs.

```suggestion
	});

	it("parses reset-at values expressed as epoch seconds and epoch milliseconds", async () => {
```

How can I resolve this? If you propose a fix, please make it concise.

Last reviewed commit: "feat: add session su..."

@chatgpt-codex-connector
Copy link

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 21, 2026

Important

Review skipped

Auto reviews are limited based on label configuration.

🚫 Review skipped — only excluded labels are configured. (1)
  • skip-review

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: e3b07675-cfba-463a-901d-6a02b1ab64ee

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch git-clean/pr147-supervisor-plumbing
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch git-clean/pr147-supervisor-plumbing

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

Comment @coderabbitai help to get the list of available commands and usage tips.

@ndycode ndycode added the needs-human-review Flagged by automated PR quality screening; maintainer review required. label Mar 21, 2026
@ndycode ndycode added passed and removed passed labels Mar 21, 2026
@ndycode ndycode changed the base branch from main to dev March 21, 2026 19:57
@ndycode ndycode removed the needs-human-review Flagged by automated PR quality screening; maintainer review required. label Mar 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant