Make two-pass context phases explicit#786
Conversation
Replace the sentinel-based SourceContext phase contract with an explicit SourceContextRequest object. Two-pass contexts now receive phase: "phase1" or phase: "phase2" requests, which removes the ambiguity between the first collection pass and a real successful parser value of undefined. Update the core runner plumbing, remove the config-specific undefined wrapping workaround, and add regression coverage for the undefined first-pass case. Refresh manual getAnnotations() callers, examples, and documentation so they use the new phase-two request shape. Update the changelog and related docs for the breaking API change. Fixes #271 Co-Authored-By: OpenAI Codex <codex@openai.com>
Bring the custom source context example in line with the new SourceContextRequest API so it no longer demonstrates the removed parsed === undefined phase heuristic. Fixes #271 Co-Authored-By: OpenAI Codex <codex@openai.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request replaces the sentinel-based two-pass SourceContext contract with an explicit SourceContextRequest object. This change makes phase 1 and phase 2 explicit, ensuring that successful first-pass results of undefined are no longer ambiguous and removing the need for context-local workarounds. The feedback focuses on improving type safety and readability in the test suite, specifically regarding the use of type assertions and parameter naming to align with the new interface.
dahlia
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #786 +/- ##
==========================================
- Coverage 87.39% 87.38% -0.01%
==========================================
Files 44 44
Lines 28571 28558 -13
Branches 6935 6936 +1
==========================================
- Hits 24969 24956 -13
+ Misses 3517 3516 -1
- Partials 85 86 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes optique#271 by replacing the implicit sentinel-based two-pass SourceContext contract (parsed === undefined meant “phase 1”) with an explicit SourceContextRequest object that distinguishes { phase: "phase1" } from { phase: "phase2", parsed }, allowing a legitimate phase-2 parsed value of undefined without ambiguity.
Changes:
- Introduces
SourceContextRequestand updatesSourceContext.getAnnotations()/getInternalAnnotations()to receive explicit phase requests. - Updates the runner (
@optique/corefacade) to pass explicit phase-1 and phase-2 request objects and removes the oldfinalizeParsed()escape hatch. - Updates config integration, tests, examples, and docs to use the new request shape and adds regressions covering “undefined first-pass value still triggers phase 2”.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/run/src/run.test.ts | Updates runner integration tests to handle explicit phase requests. |
| packages/inquirer/src/index.test.ts | Updates prompt/inquirer tests to use phase requests and parsed extraction helpers. |
| packages/env/src/index.ts | Clarifies manual getAnnotations() usage for single-pass env contexts. |
| packages/env/src/index.test.ts | Updates env/config dependency tests to pass explicit phase-2 requests when needed. |
| packages/core/src/facade.ts | Implements explicit phase request passing in phase-1/phase-2 annotation collection; removes finalizeParsed path. |
| packages/core/src/facade.test.ts | Updates existing two-pass context tests and adds regressions for undefined first-pass values in async + sync runners. |
| packages/core/src/context.ts | Adds SourceContextRequest types and updates SourceContext API docs/signatures accordingly. |
| packages/core/src/context.test.ts | Updates interface tests for the new request contract; removes finalizeParsed tests. |
| packages/config/src/run.test.ts | Adds regression ensuring config phase-2 loading runs even when the top-level parser returns undefined. |
| packages/config/src/index.ts | Simplifies config context by removing undefined-wrapping workaround and switching to request.phase / request.parsed. |
| packages/config/src/index.test.ts | Updates manual getAnnotations() call sites to use { phase: "phase2", parsed }. |
| examples/patterns/interactive-env-config-composition.ts | Updates example to preload config annotations via an explicit phase-2 request. |
| examples/patterns/custom-source-context.ts | Updates custom two-pass context example to branch on request.phase and use request.parsed. |
| docs/integrations/env.md | Updates guidance around manual getAnnotations() for single-pass env contexts. |
| docs/integrations/config.md | Updates guidance around manual phase-1 vs phase-2 annotation snapshots. |
| docs/cookbook.md | Updates cookbook snippet to use explicit phase-2 request. |
| docs/concepts/runners.md | Updates runner docs to reference getAnnotations({ phase: "phase2", parsed }). |
| docs/concepts/extend.md | Updates API reference and examples to describe/use SourceContextRequest; removes finalizeParsed documentation. |
| CHANGES.md | Documents the breaking API change and links it to the reported issue. |
![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: c106d3ee79
ℹ️ 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".
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR replaces the sentinel-based two-pass SourceContext protocol (where Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/concepts/extend.md (1)
887-919:⚠️ Potential issue | 🟡 MinorTypecheck these updated
SourceContextexamples.The first two snippets now reference
SourceContextRequestbut do not import it, so they are copy-paste broken. Because all three updated examples are still fenced as plaintypescript, this API drift will not be caught automatically either. Please add the missing imports and switch these VitePress examples totypescript twoslash.Suggested doc fix
-~~~~ typescript -import type { SourceContext } from "@optique/core/context"; +~~~~ typescript twoslash +import type { + SourceContext, + SourceContextRequest, +} from "@optique/core/context";-~~~~ typescript -import type { SourceContext, Annotations } from "@optique/core/context"; +~~~~ typescript twoslash +import type { + Annotations, + SourceContext, + SourceContextRequest, +} from "@optique/core/context";-~~~~ typescript +~~~~ typescript twoslashAs per coding guidelines,
docs/**/*.mdexamples should use thetwoslashmodifier in VitePress code blocks to enable TypeScript type checking and hover information.Also applies to: 1135-1171, 1185-1237
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/concepts/extend.md` around lines 887 - 919, The docs examples reference SourceContextRequest but don't import it and are not using twoslash; update the code blocks to use the "typescript twoslash" fence and add the missing import for SourceContextRequest (alongside SourceContext) at the top of each example so the types typecheck; specifically adjust the examples that declare envContext and configContext (and the other two examples noted at lines ~1135-1171 and ~1185-1237) to import both SourceContext and SourceContextRequest and change the fence language to "typescript twoslash".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/facade.test.ts`:
- Around line 64-71: getPhase2ContextParsed currently conflates "not a phase‑2
request" with "phase‑2 with parsed === undefined"; change it to return a
discriminated result object so absence is unambiguous: e.g. return { present:
boolean; parsed?: T } from getPhase2ContextParsed and set present = true
whenever the request is a phase2 request (even if parsed is undefined),
otherwise present = false; update callers to check the present flag (or use an
isPhase2ContextRequest type guard) instead of testing parsed !== undefined.
In `@packages/core/src/facade.ts`:
- Around line 3042-3047: The thrown generic Error for a context returning a
Promise in sync mode should be a TypeError: in the block where result is
obtained from context.getAnnotations (check the instanceof Promise branch that
currently throws new Error using context.id), replace that generic Error with a
TypeError preserving the existing message and guidance ("Use runWith() or
runWithAsync() for async contexts."), so callers receive a more precise
exception type for the sync/async contract violation.
In `@packages/inquirer/src/index.test.ts`:
- Around line 39-53: The helpers currently treat null/undefined and objects
without a "phase" key as phase1 or return raw request, which allows legacy raw
payloads to pass; update isPhase1ContextRequest to only return true when request
is a non-null object that has a "phase" property equal to "phase1" (remove the
request == null branch), and update getPhase2ContextParsed to only return parsed
when the request is a non-null object that explicitly has phase === "phase2" (do
not return the raw request when the phase key is missing); reference the
functions isPhase1ContextRequest and getPhase2ContextParsed to locate and
tighten the type checks so only explicit SourceContextRequest shapes are
accepted.
In `@packages/run/src/run.test.ts`:
- Around line 40-47: getPhase2ContextParsed currently falls back to returning
the raw request for non-object inputs, which lets legacy/sentinel payload shapes
slip through; change it so it only returns a parsed value when the input is an
object with phase === "phase2" and otherwise returns undefined. In practice
update getPhase2ContextParsed to remove the final "return request as T |
undefined" branch and ensure the only return that yields (request as
SourceContextRequest & { parsed: T }).parsed is guarded by the object/phase
check; refer to symbols getPhase2ContextParsed, SourceContextRequest, phase, and
parsed to locate and tighten the type-narrowing logic.
---
Outside diff comments:
In `@docs/concepts/extend.md`:
- Around line 887-919: The docs examples reference SourceContextRequest but
don't import it and are not using twoslash; update the code blocks to use the
"typescript twoslash" fence and add the missing import for SourceContextRequest
(alongside SourceContext) at the top of each example so the types typecheck;
specifically adjust the examples that declare envContext and configContext (and
the other two examples noted at lines ~1135-1171 and ~1185-1237) to import both
SourceContext and SourceContextRequest and change the fence language to
"typescript twoslash".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 73b69378-cbdd-4884-84f7-05935f4794eb
📒 Files selected for processing (19)
CHANGES.mddocs/concepts/extend.mddocs/concepts/runners.mddocs/cookbook.mddocs/integrations/config.mddocs/integrations/env.mdexamples/patterns/custom-source-context.tsexamples/patterns/interactive-env-config-composition.tspackages/config/src/index.test.tspackages/config/src/index.tspackages/config/src/run.test.tspackages/core/src/context.test.tspackages/core/src/context.tspackages/core/src/facade.test.tspackages/core/src/facade.tspackages/env/src/index.test.tspackages/env/src/index.tspackages/inquirer/src/index.test.tspackages/run/src/run.test.ts
Create fresh SourceContextRequest objects for each context invocation in phase-one and phase-two collection so one context cannot mutate the request seen by another. Add async and sync regressions covering cross-context request mutation, and keep the phase-two parsed-value helper explicit in the associated facade tests. Fixes #271 Co-Authored-By: OpenAI Codex <codex@openai.com>
Rename a two-pass test callback parameter from parsed to request so its shape matches the explicit SourceContextRequest contract. Keep the associated parsed-value extraction helper aligned with the updated naming. Fixes #271 Co-Authored-By: OpenAI Codex <codex@openai.com>
Tighten test-only phase request helpers so they only accept explicit SourceContextRequest shapes, and use TypeError for sync/async contract violations when a context returns a Promise in sync mode. This keeps the regression coverage aligned with the new request contract and makes the sync runner's error classification more precise. Fixes #271 Co-Authored-By: OpenAI Codex <codex@openai.com>
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/core/src/facade.ts (1)
2975-2978:⚠️ Potential issue | 🟡 MinorUse
TypeErrorfor sync/async contract violations.A context returning a
PromiseinrunWithSync()is invalid usage, not a generic runtime failure. Both throws should useTypeErrorso callers can distinguish contract violations reliably. As per coding guidelines, prefer specific error types over genericError.Suggested fix
- throw new Error( + throw new TypeError( `Context ${String(context.id)} returned a Promise in sync mode. ` + "Use runWith() or runWithAsync() for async contexts.", );- throw new Error( + throw new TypeError( `Context ${String(context.id)} returned a Promise in sync mode. ` + "Use runWith() or runWithAsync() for async contexts.", );Also applies to: 3043-3047
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/facade.ts` around lines 2975 - 2978, Replace the generic Error throws used for sync/async contract violations with TypeError so callers can detect misuse; specifically change the throw in runWithSync that currently does throw new Error(`Context ${String(context.id)} returned a Promise in sync mode...`) to throw new TypeError(...) (keeping the same message), and likewise update the second similar throw later in the file (the other sync/async contract-check throw around the 3043-3047 region) to use TypeError instead of Error.packages/core/src/facade.test.ts (1)
65-72:⚠️ Potential issue | 🟠 MajorKeep phase-2 presence separate from the parsed value.
getPhase2ContextParsed()still returnsundefinedfor both phase 1 and{ phase: "phase2", parsed: undefined }. Callers then branch onparsed == null/parsed === undefined(for example Line 10338, Line 10417, Line 10501, Line 10630, and Line 10759), so these tests would silently skip a valid phase-two payload ofundefined—the exact ambiguity this PR is supposed to lock down.Suggested fix
-function getPhase2ContextParsed<T>(request: unknown): T | undefined { - if (request != null && typeof request === "object" && "phase" in request) { - return (request as { readonly phase?: unknown }).phase === "phase2" - ? (request as SourceContextPhase2Request).parsed as T - : undefined; - } - return request as T | undefined; -} +function isPhase2ContextRequest<T>( + request: unknown, +): request is SourceContextPhase2Request & { readonly parsed: T } { + return request != null && + typeof request === "object" && + "phase" in request && + (request as { readonly phase?: unknown }).phase === "phase2"; +}Then at call sites:
if (!isPhase2ContextRequest<{ readonly config: string }>(request)) return {}; const phase2ParsedValue = request.parsed;Based on learnings: In two-pass SourceContext tests, phase handling must avoid ambiguous guards so valid phase-two values are not skipped.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/facade.test.ts` around lines 65 - 72, getPhase2ContextParsed conflates "is phase2" with the parsed value and therefore treats a valid phase2 payload of parsed === undefined as non-phase2; change the approach so phase detection is separate from reading the parsed value — implement or use a type-guard like isPhase2ContextRequest(request) / rename getPhase2ContextParsed to isPhase2ContextRequest that only checks request != null && typeof request === "object" && request.phase === "phase2", and update callers (the sites using getPhase2ContextParsed) to first guard with isPhase2ContextRequest(request) and then read request.parsed into a local (allowing parsed to be undefined) rather than relying on a combined helper that returns undefined for both non-phase2 and defined-undefined phase2.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/core/src/facade.test.ts`:
- Around line 65-72: getPhase2ContextParsed conflates "is phase2" with the
parsed value and therefore treats a valid phase2 payload of parsed === undefined
as non-phase2; change the approach so phase detection is separate from reading
the parsed value — implement or use a type-guard like
isPhase2ContextRequest(request) / rename getPhase2ContextParsed to
isPhase2ContextRequest that only checks request != null && typeof request ===
"object" && request.phase === "phase2", and update callers (the sites using
getPhase2ContextParsed) to first guard with isPhase2ContextRequest(request) and
then read request.parsed into a local (allowing parsed to be undefined) rather
than relying on a combined helper that returns undefined for both non-phase2 and
defined-undefined phase2.
In `@packages/core/src/facade.ts`:
- Around line 2975-2978: Replace the generic Error throws used for sync/async
contract violations with TypeError so callers can detect misuse; specifically
change the throw in runWithSync that currently does throw new Error(`Context
${String(context.id)} returned a Promise in sync mode...`) to throw new
TypeError(...) (keeping the same message), and likewise update the second
similar throw later in the file (the other sync/async contract-check throw
around the 3043-3047 region) to use TypeError instead of Error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e1896c50-662a-4e05-aa69-c9d159ef25ed
📒 Files selected for processing (3)
packages/core/src/facade.test.tspackages/core/src/facade.tspackages/inquirer/src/index.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/facade.test.ts`:
- Around line 10241-10288: The mutating context currently reassigns
request.parsed (in the mutatingContext getAnnotations) which only proves a fresh
request wrapper, not isolation of the parsed payload; instead, modify the parsed
payload itself (e.g., set parsed.config = "mutated.json") so the test verifies
the parsed object is not shared across contexts; update the
mutatingContext.getAnnotations to mutate the parsed object returned by
getPhase2ContextParsed<{config:string}> rather than reassigning request.parsed,
and apply the same change to the synchronous variant test that uses the same
pattern.
In `@packages/core/src/facade.ts`:
- Around line 2975-2978: Update the JSDoc for the public runWithSync() function
to document the actual exception type thrown: replace or add `@throws` {TypeError}
(instead of `@throws` {Error}) for the cases where a context returns a Promise in
sync mode and where sync disposal throws; update both occurrences around the
runWithSync declaration and the other branch noted (the block around the second
throw at the later lines referenced) so generated API docs correctly show
TypeError.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 545fbadf-bc5f-48e4-b12d-ca25a59ee61f
📒 Files selected for processing (4)
packages/core/src/facade.test.tspackages/core/src/facade.tspackages/inquirer/src/index.test.tspackages/run/src/run.test.ts
Promote the explicit SourceContextRequest examples in *docs/concepts/extend.md* to twoslash blocks and add the missing fixture declarations and imports they need to typecheck. Co-Authored-By: OpenAI Codex <codex@openai.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/concepts/extend.md`:
- Around line 740-752: The documentation for SourceContextRequest currently
lists `phase: "phase1"` and `phase: "phase2"` as top-level items which confuses
readers; update the `SourceContextRequest` doc block (the entry that mentions
getAnnotations() and getInternalAnnotations()) to state that it is a
discriminated union on the `phase` field and then present the variants as
sub-items (e.g., a bulleted list or inline prose) showing `phase: "phase1"` —
Initial annotation collection before the first parse pass, and `phase: "phase2"`
— Second annotation collection after a usable first pass and mention that
`parsed` holds the first-pass value (possibly undefined); alternatively
reference the actual interface declaration for SourceContextRequest to make the
relationship explicit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ebe1c0aa-cdc4-4176-9583-6669bc0c700d
📒 Files selected for processing (1)
docs/concepts/extend.md
Clarify the discriminated-union shape of SourceContextRequest in *docs/concepts/extend.md* and align runWithSync() JSDoc with the actual TypeError thrown for sync/async contract violations. Co-Authored-By: OpenAI Codex <codex@openai.com>
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/concepts/extend.md`:
- Around line 912-916: The getAnnotations method currently assumes
request.parsed is always an object and does const result = request.parsed as {
config?: string }; which can throw when parsed is legitimately undefined in
phase2; change the guard to check phase explicitly and treat parsed as possibly
undefined (e.g., const parsed = request.parsed as { config?: string } |
undefined) and return early if !parsed?.config; update the same pattern in the
other occurrence referenced (lines ~1164-1170) so phase2 paths use request.phase
=== "phase2" and parsed is accessed via optional chaining.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e8b648a2-2cde-4947-b39d-09a70544d2d5
📒 Files selected for processing (2)
docs/concepts/extend.mdpackages/core/src/facade.ts
There was a problem hiding this comment.
Code Review
This pull request replaces the sentinel-based two-pass SourceContext contract with an explicit SourceContextRequest object. By introducing a discriminated union for phase-1 and phase-2 requests, the system no longer relies on undefined as a sentinel value, effectively removing the need for the finalizeParsed hook and its associated workarounds. This change resolves ambiguity when a parser legitimately returns undefined during the first pass. The feedback provided correctly identifies naming inconsistencies in the test suite following the interface changes and suggests renaming parameters from parsed to request for better clarity and consistency with the new contract.
There was a problem hiding this comment.
Code Review
This pull request replaces the sentinel-based two-pass SourceContext contract with an explicit SourceContextRequest object, resolving ambiguity when a first-pass parse returns undefined. The getAnnotations() and getInternalAnnotations() methods now receive a request object specifying the phase and providing the first-pass result. This change eliminates the need for the finalizeParsed hook and internal undefined wrapping workarounds across the core library, documentation, and tests. I have no feedback to provide.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7bef340eb4
ℹ️ 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".
Treat omitted requests as the only manual phase-1 convenience in createConfigContext(). A null request is malformed input, so reject it through the same TypeError path as other invalid SourceContextRequest shapes. Add a regression test that proves getAnnotations(null, options) no longer fails silently by returning phase-one annotations. Co-Authored-By: OpenAI Codex <codex@openai.com>
|
@codex review |
|
/gemini review |
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
Code Review
This pull request replaces the sentinel-based two-pass SourceContext contract with an explicit SourceContextRequest object, removing the need for finalizeParsed and resolving ambiguity around undefined parse results. Feedback includes improving error messages by including the actual type of invalid values in createConfigContext and renaming parameters in test files for consistency with the new API contract.
Rename remaining two-pass SourceContext callback parameters in packages/core/src/facade.test.ts from parsed to request. These tests now match the explicit SourceContextRequest contract more clearly and no longer suggest that getAnnotations() receives the raw parsed value. Co-Authored-By: OpenAI Codex <codex@openai.com>
Include the received request type in createConfigContext() getAnnotations() TypeError messages for malformed SourceContextRequest inputs. This keeps the public API boundary easier to debug while still rejecting invalid phase-one and phase-two request shapes. Update the input-validation regression tests to assert the new malformed-request messages. Co-Authored-By: OpenAI Codex <codex@openai.com>
|
@codex review |
|
/gemini review |
|
Codex Review: Didn't find any major issues. You're on a roll. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
Code Review
This pull request replaces the sentinel-based two-pass SourceContext contract with an explicit SourceContextRequest object. The getAnnotations() and getInternalAnnotations() methods now receive a discriminated union indicating the phase and providing the parsed value for phase 2, which removes the need for the finalizeParsed hook and related workarounds for ambiguous undefined results. Feedback was provided to simplify the validation logic of the request object in packages/config/src/index.ts by using guard clauses and inlining type checks to align with the repository's style.
Treat an omitted SourceContext request as the documented manual phase-one path in the core and run test helpers. This keeps the helper semantics aligned with the public getAnnotations() contract without reopening the old raw-payload regression coverage. While touching those helpers, rename the remaining test callback parameters from _parsed to _request where they still received the request object. Co-Authored-By: OpenAI Codex <codex@openai.com>
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request replaces the sentinel-based two-pass SourceContext contract with an explicit SourceContextRequest object, resolving ambiguity when first-pass results are undefined. The changes involve updating getAnnotations() and getInternalAnnotations() to accept a request object containing the current phase and parsed data, while removing the now-obsolete finalizeParsed hook. A high-severity issue was identified where getTypeName is used in the config package without being imported or defined, which will cause a runtime error. Additionally, the manual validation logic in the config package could be refactored using guard clauses for improved clarity and more specific error reporting for invalid types.
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Summary
Fixes #271.
This PR removes the old sentinel-based two-pass
SourceContextcontract, where phase 1 was inferred fromparsed === undefined, and replaces it with an explicitSourceContextRequestobject. That ambiguity meant a successful first-pass value ofundefinedwas observationally identical to “phase 1 has not happened yet”, so custom two-pass contexts could silently skip phase-two work. The runner now passes{ phase: "phase1" }and{ phase: "phase2", parsed }explicitly, which makesundefineda valid phase-two payload instead of a hidden control signal.This is an intentional breaking API change for the 1.0.0 line. The core API changes live in packages/core/src/context.ts and packages/core/src/facade.ts. As part of that redesign, the old
finalizeParsed()escape hatch was removed because it is no longer needed once the phase is explicit. The config integration in packages/config/src/index.ts was then simplified by deleting its privateundefinedwrapper workaround and switching directly to the new request model.Example
Custom two-pass contexts should now branch on
request.phaseinstead ofparsed === undefined:This keeps phase detection correct even when the first pass legitimately produces
undefined.Tests and docs
The fix was developed TDD-first. Regression coverage was added in packages/core/src/facade.test.ts to prove that both
runWith()andrunWithSync()still enter phase 2 when the first-pass result isundefined. A config-focused regression was added in packages/config/src/run.test.ts to confirm that phase-two config loading still runs for top-level parsers whose successful value isundefined.Manual
getAnnotations()call sites and examples were updated to use the new request shape in packages/config/src/index.test.ts, packages/env/src/index.test.ts, packages/inquirer/src/index.test.ts, packages/run/src/run.test.ts, examples/patterns/interactive-env-config-composition.ts, and examples/patterns/custom-source-context.ts. The API docs and changelog were updated in docs/concepts/extend.md, docs/concepts/runners.md, docs/cookbook.md, docs/integrations/config.md, docs/integrations/env.md, and CHANGES.md.Validation
I verified this change with
deno task check,mise test, andpnpm buildin docs/.