Capture native Codex exec context - #671
Merged
Merged
Conversation
Signed-off-by: Bernardo Donadio <bcdonadio@bcdonadio.com>
Signed-off-by: Bernardo Donadio <bcdonadio@bcdonadio.com>
Signed-off-by: Bernardo Donadio <bcdonadio@bcdonadio.com>
Signed-off-by: Bernardo Donadio <bcdonadio@bcdonadio.com>
Signed-off-by: Bernardo Donadio <bcdonadio@bcdonadio.com>
Signed-off-by: Bernardo Donadio <bcdonadio@bcdonadio.com>
PR Summary by QodoCapture bounded Codex native exec context from PostToolUse events
AI Description
Diagram
High-Level Assessment
Files changed (17)
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR fixes missing passive-learning capture for native Codex shell tool calls by normalizing functions.exec / functions.exec_command PostToolUse payloads into the existing “Bash” extractor semantics, while enforcing a strict boundary that avoids persisting raw stdout/stderr/response fields. It also strengthens the Codex connector doctor flow with structural validation plus a pure in-memory functional probe, and documents the new capture boundary as a patch release.
Changes:
- Add a Codex-specific PostToolUse normalizer that bounds commands, projects only allowlisted status fields, and suppresses
lcm storefeedback-loop commands. - Extend Codex connector diagnostics (“doctor”) to verify exact PostToolUse hook structure and run a no-write functional coverage probe.
- Add comprehensive regression tests, update docs, and update Codecov component metadata/counts.
Reviewed changes
Copilot reviewed 15 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/hooks/post-tool.test.ts | Adds integration-style persistence assertions for native Codex exec normalization and bounded storage behavior. |
| test/hooks/post-tool-normalization.test.ts | New table-driven tests covering normalization, status precedence, bounding, and secret-boundary guarantees. |
| test/hooks/dispatch.test.ts | Verifies Codex client propagation through direct post-tool dispatch. |
| test/connectors/codex-hooks.test.ts | Adds tests for exact PostToolUse structural inspection and canonical hooks path resolution. |
| test/codecov-config.test.ts | Updates expected production file counts to reflect the new production module. |
| test/bin/lcm-run-cli.test.ts | Adds CLI tests for post-tool client injection and Codex doctor structural/functional output paths. |
| src/hooks/post-tool.ts | Routes all PostToolUse inputs through the new normalizer before extraction. |
| src/hooks/post-tool-normalization.ts | New normalization module for Codex native exec payloads + pure functional probe helper. |
| src/hooks/extractors.ts | Exports PostToolInput to support typed normalization tests. |
| src/connectors/codex-hooks.ts | Adds exact PostToolUse hook inspection and canonical path resolution helper. |
| bin/lcm.ts | Implements targeted Codex doctor structural + pure functional checks with aggregated exit status. |
| codecov.yml | Clarifies src/hooks/ component ownership for the new adapter module. |
| docs/vscode-codex.md | Documents the Codex PostToolUse capture boundary and doctor behavior. |
| docs/passive-learning.md | Documents native Codex command capture semantics and boundary constraints. |
| docs/hook-protocol.md | Documents Codex native PostToolUse contract and doctor checks (needs a small correction per comments). |
| docs/superpowers/plans/2026-08-11-codex-post-tool-capture.md | Updates the implementation plan to reflect the final rules and probe behavior. |
| .changeset/capture-codex-exec-context.md | Patch release metadata for the Codex capture/diagnostics change. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+171
to
+185
| ```json | ||
| { | ||
| "PostToolUse": [ | ||
| { | ||
| "matcher": "*", | ||
| "hooks": [ | ||
| { | ||
| "type": "command", | ||
| "command": "lcm post-tool --client codex" | ||
| } | ||
| ] | ||
| } | ||
| ] | ||
| } | ||
| ``` |
Comment on lines
+273
to
+277
| /** Resolve the Codex hooks path using the same `~/` convention as installation. */ | ||
| export function resolveCodexHooksPath(cwd: string = process.cwd()): string { | ||
| void cwd; | ||
| return join(homedir(), CODEX_HOOKS_PATH.slice(2)); | ||
| } |
Code Review by Qodo
1. Codex health check skipped without explicit agent
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+284
to
+286
| return postToolUse.some((group) => { | ||
| if (!isObject(group) || group.matcher !== "*" || !Array.isArray(group.hooks)) return false; | ||
| return group.hooks.some((hook) => |
There was a problem hiding this comment.
1. Implicit any in hook scan 📘 Rule violation ⚙ Maintainability
The new hasExactPostToolHook() logic relies on Array.isArray() narrowing to any[], causing callback parameters like group/hook to be inferred as any. This violates the requirement to avoid inferred any in TypeScript function/callback signatures.
Agent Prompt
## Issue description
New callbacks infer `any` (via `Array.isArray` → `any[]`), violating the no-inferred-`any` requirement.
## Issue Context
`hasExactPostToolHook()` uses `Array.isArray(postToolUse)` which narrows to `any[]`, so `.some((group) => ...)` and nested `.some((hook) => ...)` infer `any`.
## Fix Focus Areas
- src/connectors/codex-hooks.ts[279-291]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| } | ||
| } | ||
|
|
||
| if ((agent as any).id !== "codex" || agentName === undefined) continue; |
There was a problem hiding this comment.
2. Undocumented as any cast 📘 Rule violation ⚙ Maintainability
A new (agent as any) cast was added without an adjacent justification comment. This violates the requirement that each as any usage in changed TypeScript code be explicitly justified.
Agent Prompt
## Issue description
A newly added `as any` type assertion is missing the required justification comment.
## Issue Context
The checklist requires an immediately-adjacent comment explaining why `as any` is necessary.
## Fix Focus Areas
- bin/lcm.ts[2393-2395]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| @@ -0,0 +1,7 @@ | |||
| --- | |||
| "@donadiosolutions/lcm": patch | |||
There was a problem hiding this comment.
3. @donadiosolutions/lcm patch bump too low 📘 Rule violation § Compliance
The changeset declares a patch release, but the PR introduces new user-facing behavior (native Codex exec capture and new connector diagnostics). Per the checklist, this should be at least a minor bump.
Agent Prompt
## Issue description
The changeset uses a `patch` bump even though the change adds new user-facing behavior.
## Issue Context
This PR adds native Codex `functions.exec` / `functions.exec_command` capture behavior and new `lcm connectors doctor codex` diagnostics, which is more than a patch-level internal fix.
## Fix Focus Areas
- .changeset/capture-codex-exec-context.md[1-3]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+2401
to
+2403
| const inspection = inspectCodexPostToolHook( | ||
| resolveCodexHooksPath(opts.global ? homedir() : process.cwd()), | ||
| ); |
There was a problem hiding this comment.
4. Doctor ignores hooks feature flag 🐞 Bug ≡ Correctness
Targeted Codex doctor declares PostToolUse installed after checking only hooks.json, while Codex also requires [features].hooks = true in config.toml. If that flag is missing or disabled, the pure adapter probe still passes and doctor reports healthy even though Codex will not dispatch the hook.
Agent Prompt
## Issue description
`lcm connectors doctor codex` checks the PostToolUse entry and an in-memory adapter probe but does not verify that Codex hook execution is enabled in `config.toml`. Extend structural health validation to require `[features].hooks = true`, reporting failure and skipping the functional probe when the runtime feature is absent or disabled.
## Issue Context
Codex connector installation enables the feature separately from writing `hooks.json`, so either file can drift independently. Resolve and inspect the canonical config path using the same installation scope and keep the check pure/no-write.
## Fix Focus Areas
- bin/lcm.ts[2396-2422]
- src/connectors/codex-hooks.ts[273-307]
- src/connectors/codex-hooks.ts[173-238]
- test/connectors/codex-hooks.test.ts[104-154]
- test/bin/lcm-run-cli.test.ts[886-946]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| } | ||
| } | ||
|
|
||
| if ((agent as any).id !== "codex" || agentName === undefined) continue; |
There was a problem hiding this comment.
5. Codex health check skipped without explicit agent 🐞 Bug ≡ Correctness
The connectors doctor command's new Codex structural/functional check is guarded by `agentName === undefined, so running the documented broad lcm connectors doctor` (no agent argument) silently skips both the exact PostToolUse structural inspection and the native-exec functional probe, even though the loop still iterates Codex within the AGENTS list. This causes the new health verification introduced by this PR to be effectively unreachable for the command's most common invocation form.
Agent Prompt
## Issue description
The Codex-specific PostToolUse structural/functional health check added in this PR is skipped whenever `lcm connectors doctor` is invoked without an explicit agent name (broad mode over all agents), because of an added `agentName === undefined` condition in the skip check. This means the new diagnostics never run for the most common/documented usage of `lcm connectors doctor`.
## Issue Context
`agents` is populated with all agents (including Codex) when `agentName` is undefined (broad mode). The loop over `agents` should run the Codex-specific checks whenever the current `agent.id === "codex"`, regardless of whether the user passed an explicit agent name.
## Fix Focus Areas
- bin/lcm.ts[2394-2394]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+39
to
+45
| function normalizeStatus(toolOutput: unknown, toolResponse: unknown): { isError?: boolean } | undefined { | ||
| const outputStatus = isRecord(toolOutput) ? statusFromRecord(toolOutput) : undefined; | ||
| if (outputStatus !== undefined) return { isError: outputStatus }; | ||
|
|
||
| const responseStatus = isRecord(toolResponse) ? statusFromRecord(toolResponse) : undefined; | ||
| return responseStatus === undefined ? undefined : { isError: responseStatus }; | ||
| } |
There was a problem hiding this comment.
6. Status precedence ignores tool_response when output invalid 🐞 Bug ≡ Correctness
normalizeStatus in post-tool-normalization.ts only checks statusFromRecord(toolOutput) and
falls back to toolResponse only when the *entire* output-derived status is undefined; but per
the documented policy this fallback should also occur when tool_output is a record with no valid
recognized field, which is exactly what happens — statusFromRecord returns undefined when no
field matches, so this path is actually followed correctly. However, the same function returns early
with the tool_output result even when it's a valid false/0, which is correct per docs ("A
valid false or zero value is authoritative"), so on closer inspection the implementation matches the
documented precedence rules; this specific concern converges to no defect and should not be
reported.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
functions.execandfunctions.exec_commandpayloads into bounded, scrubbed passive-learning semanticsTesting
npm run buildnpm run typechecknpm run lintnpm run test:ci(261 files passed, 2 skipped; 6430 tests passed, 12 skipped; 100% statements, branches, functions, and lines)Closes #604