Type lifecycle hook resources - #876
Conversation
Arrange package sections and feature entries so related changes are easier to scan. Preserve package-specific help options and consolidate the LogTape formatter additions from the same pull request. Assisted-by: Codex:gpt-5.6-sol
Thread the caller-defined resource type through lifecycle hook contexts, command definitions, run options, and the internal dispatcher. Keep the resource optional while making producer and consumer mismatches visible to TypeScript. Add compile-time coverage for inferred and explicit resources, and update the lifecycle documentation and examples to avoid casts. Fixes #875 #876 Assisted-by: Codex:gpt-5.6-sol
|
@codex review |
![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 introduces generic type parameters to the lifecycle hooks of @optique/discover (runProgram and defineCommand). This allows the caller-defined resource type to flow from beforeEach to later hooks (afterEach, onError) and the command handler without requiring unsafe type casts. Documentation, examples, and tests have been updated to reflect and verify these type-safety improvements. There are no review comments to assess, so I have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThe discovery lifecycle API now accepts a caller-defined generic resource type. That type flows through hook contexts, command definitions, handlers, Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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: 0f3b5a2e7d
ℹ️ 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".
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #876 +/- ##
==========================================
+ Coverage 93.41% 93.43% +0.01%
==========================================
Files 76 76
Lines 38567 38591 +24
Branches 9592 9607 +15
==========================================
+ Hits 36029 36056 +27
+ Misses 1733 1731 -2
+ Partials 805 804 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Static commands without their own beforeEach hook receive the program-level context at runtime, so accepting an erased handler resource type could pair a handler with a context it cannot safely consume. Preserve that relationship through runProgram's static command and command-entry paths while allowing a command-level beforeEach hook to establish an independent resource type. Cover explicit, inferred, and command-entry mismatches with compile-time regressions, and document how program and command resources interact. #876 (comment) Assisted-by: Codex:gpt-5.6-sol
|
/gemini review |
|
@codex review |
There was a problem hiding this comment.
Code Review
This pull request introduces generic type safety to the lifecycle hooks of runProgram() and defineCommand() in @optique/discover. By adding a generic type parameter R to ProgramHookContext, ProgramHooks, and related command interfaces, the caller-defined resource type now flows safely from beforeEach to subsequent hooks (afterEach, onError) and command handlers without requiring manual type assertions. The documentation, cookbook, examples, and tests have been updated to reflect and verify these type-safe improvements. There are no review comments, so I have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7f47e8be6
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/cookbook.md (1)
2373-2374: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse tight em dash spacing.
As per coding guidelines, markdown documents should use tight em dash spacing rules without surrounding spaces.
📝 Proposed fix
When only one command needs its own setup—for example, a `deploy` command that -always refreshes an auth token — put the hooks on the command definition instead +always refreshes an auth token—put the hooks on the command definition instead🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/cookbook.md` around lines 2373 - 2374, Update the cookbook guidance sentence to use a tight em dash with no surrounding spaces, while preserving the existing wording and meaning.Source: Coding guidelines
packages/discover/src/index.ts (1)
628-670: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy liftPrevent silently dropping the program context when a command defines
afterEachbut nobeforeEach.When a command defines an
afterEachoronErrorhook but omitsbeforeEach,runHookScopecreates a new{}context for the command. This causes the command's later hooks to receive an empty context, silently dropping the program-level resource.Passing a fallback context to
runHookScopeensures the program context correctly flows through to the command'safterEachandonErrorin this scenario, while seamlessly passing it to the handler.🐛 Proposed fix to thread the fallback context
- await runHookScope( - programHooks, - invocation, - (programContext) => - runHookScope(commandHooks, invocation, (commandContext) => { - // Pass the hook context only when a beforeEach actually produced one, - // using the most specific scope. When no beforeEach ran, call the - // handler with just the value so handlers see the exact single-argument - // call shape of a plain runProgram() without hooks. - if (commandHooks?.beforeEach != null) { - return invocation.handler(invocation.value, commandContext); - } - if (programHooks?.beforeEach != null) { - return invocation.handler(invocation.value, programContext); - } - return invocation.handler(invocation.value); - }), - ); + await runHookScope( + programHooks, + invocation, + {}, + (programContext) => + runHookScope( + commandHooks, + invocation, + programContext, + (commandContext) => { + // Pass the hook context only when a beforeEach actually produced one, + // using the most specific scope. When no beforeEach ran, call the + // handler with just the value so handlers see the exact single-argument + // call shape of a plain runProgram() without hooks. + if (commandHooks?.beforeEach != null || programHooks?.beforeEach != null) { + return invocation.handler(invocation.value, commandContext); + } + return invocation.handler(invocation.value); + }, + ), + ); } /** * Runs an inner step wrapped in a single set of lifecycle hooks. * * `@param` hooks The hooks for this scope, if any. * `@param` invocation The selected command invocation passed to `beforeEach`. + * `@param` fallbackContext The context to use if this scope lacks a `beforeEach`. * `@param` inner The step to wrap; receives the context from `beforeEach`. * `@returns` The value returned by `inner`. * `@throws` The original error thrown by `beforeEach`, `inner`, or `afterEach`, * re-thrown after `onError` runs. An error thrown by `onError` itself * is suppressed so it cannot mask the original failure. */ async function runHookScope<R>( hooks: ProgramHooks<R> | undefined, invocation: ProgramInvocation, + fallbackContext: ProgramHookContext<R>, inner: (context: ProgramHookContext<R>) => unknown | Promise<unknown>, ): Promise<unknown> { - let context: ProgramHookContext<R> = {}; + let context: ProgramHookContext<R> = fallbackContext; try { // Default a nullish beforeEach result to an empty context so afterEach, // onError, and the handler never receive null/undefined. if (hooks?.beforeEach != null) { context = (await hooks.beforeEach(invocation)) ?? {}; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/discover/src/index.ts` around lines 628 - 670, Update runHookScope to accept an optional fallback context and initialize its scope context from that fallback when no beforeEach exists. Pass programContext as the fallback when invoking the nested commandHooks scope, so command afterEach and onError receive the program resource instead of a new empty object. Preserve contexts explicitly returned by beforeEach and the existing handler argument-shape behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs/cookbook.md`:
- Around line 2373-2374: Update the cookbook guidance sentence to use a tight em
dash with no surrounding spaces, while preserving the existing wording and
meaning.
In `@packages/discover/src/index.ts`:
- Around line 628-670: Update runHookScope to accept an optional fallback
context and initialize its scope context from that fallback when no beforeEach
exists. Pass programContext as the fallback when invoking the nested
commandHooks scope, so command afterEach and onError receive the program
resource instead of a new empty object. Preserve contexts explicitly returned by
beforeEach and the existing handler argument-shape behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c5e07355-3260-4fc2-b0bb-492e8ccf18e0
📒 Files selected for processing (5)
docs/concepts/discover.mddocs/cookbook.mdpackages/discover/src/command.tspackages/discover/src/index.test.tspackages/discover/src/index.ts
Command-level afterEach and onError hooks run inside the program hook scope. When the command has no beforeEach hook of its own, reuse the program context instead of replacing it with an empty object so those hooks observe the same resource as the handler. Add regressions for both success and failure paths, and correct the cookbook's em dash spacing. #876 (review) Assisted-by: Codex:gpt-5.6-sol
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces generic type parameters to runProgram(), defineCommand(), commandsFromModules(), and related types in @optique/discover to enable type-safe threading of caller-defined resources through lifecycle hooks and command handlers. The documentation, cookbook examples, and tests have been updated to reflect these changes. The reviewer recommends adding explicit array validation using Array.isArray() when verifying that modules, command, and options are plain objects, throwing terse TypeErrors with trailing periods to ensure consistency with the repository's style guidelines.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26079d6bdd
ℹ️ 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".
These APIs require structured option or definition objects, but arrays could reach later validation and produce misleading errors or even behave like an empty module map. Reject arrays and other non-objects at the boundary with the same terse diagnostics used elsewhere in the repository. #876 (comment) #876 (comment) #876 (comment) Assisted-by: Codex:gpt-5.6-sol
The program context is a fallback only for command hook scopes without their own beforeEach hook. Starting a scope with an empty context when beforeEach exists prevents a rejected command preflight from exposing the program resource to command onError. #876 (comment) Assisted-by: Codex:gpt-5.6-sol
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces generic type safety for lifecycle hooks and command handlers in @optique/discover. By parameterizing runProgram, defineCommand, and related types with a caller-defined resource type R, the resource returned by beforeEach is statically checked and flows seamlessly to later hooks (afterEach, onError) and command handlers without requiring manual type assertions. The changes also include robust runtime validation to reject non-object arguments across key entry points (defineCommand, commandsFromModules, and runProgram), updated documentation and examples, and comprehensive test coverage verifying type inference, mismatch rejections, and context propagation behaviors. I have no feedback to provide as there are no review comments.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ 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". |
1 participant
ProgramHookContext.resourcewas introduced asunknown, which made lifecycle resources available at runtime but erased the relationship between the value created bybeforeEachand the value later consumed by command handlers,afterEach, andonError. Callers consequently had to cast every read, and TypeScript could not catch a producer/consumer mismatch. Declaration merging cannot narrow an already declared property, so there was no type-safe escape hatch for applications. Since these APIs have not appeared in a stable release, the upcoming 1.2.0 release is the right point to correct the contract before users begin depending on the untyped form.This change threads one caller-defined resource type through
ProgramHookContext,ProgramHooks, command definitions, run options, andrunProgram(). The default remainsunknownto preserve unannotated usage, whileresourceremains optional becausebeforeEachmay returnnullorvoid. Command-local hooks infer the resource type frombeforeEach; file-discovered commands use the same explicit type withrunProgram<R>()andProgramHookContext<R>because inference cannot cross module boundaries.Discovered and statically registered command collections may contain commands with different command-local resource types. The erased-command boundary in packages/discover/src/command.ts therefore treats hook producers covariantly and consumers contravariantly, while packages/discover/src/index.ts restores the relationship only inside the hook scope that owns it. This keeps the public API sound without using
anyor pretending that every command shares one resource type.Compile-time tests in packages/discover/src/index.test.ts cover inference through
beforeEach,afterEach,onError, and command handlers, and reject mismatched producer and consumer types. The examples in docs/concepts/discover.md, docs/cookbook.md, and examples/patterns/program-hooks.ts now show the typed flow without casts while still handling an absent resource. Because the lifecycle APIs are still unreleased, CHANGES.md amends their existing 1.2.0 entry instead of presenting the corrected type design as a separate change.Fixes #875.