feat(codex): Group E — lifecycle-truth integration (wish codex-plugin-dogfood-remediation) - #2630
Conversation
…ation Group E lifecycle-truth integration, doctor half: - new codex-doctor-observation: ONE bounded 'codex plugin list --json' feeds the check-list probe, the integration summary (replay runner), and the sanitized advisory; Decision 11 applied once at the seam so the real sandbox PATH advisory can no longer contradict the fail-closed parser - new codex-lifecycle-truth: shared Decision-9 delivery gate (snapshot record vs canonical target) + typed route-layer classifier (route-collision, route-shadowed, global-route-same-key, untrusted-config, project-trust-required) - doctor: delivery-incomplete presentation when a green state lacks a matching authenticated record; typed routeLayers/advisory JSON riders; Codex project context check reuses the MCP server's resolveProjectContext
… a typed outcome Group E lifecycle-truth integration, setup half: - Decision 9: assess the authenticated delivery record BEFORE the first consent prompt or activation-owned mutation (the executor's beginActivation inner guard stays as defense in depth); missing/invalid/mismatched records exit delivery-incomplete with the one update/install recovery command and the machine-readable trailer - Decision 12: every invocation ends in one typed SetupCodexOutcomeCode; the green saved banner and the wizard summary line flow from THIS run's outcome, never from historical codex.configured state
… real-PTY lifecycle flow The new PTY flow test caught a Decision-1 violation: setup's post-activation reconcile still used the pre-Group-A synthetic 'usable plugin' probe, whose documented behavior removed the project fallback — leaving a repository with NO Codex route at all now that the plugin ships no MCP declaration. The post-activation probe now declares route-unusability honestly and reconciles the stable absolute GENIE_HOME facade route (Decision 2), exactly as trusted init writes it. tests/integration/codex-lifecycle-pty.test.ts drives the REAL CLI through repair handoff (missing record refusal) → record published via the real deep store → consent on a real pty (expect(1) on darwin, script(1) on linux) → activation with one plugin add → doctor current, plus PATH advisory, stale-historical-config, route collision/shadowing, and hard query-failure consistency. The route-layer classifier now compares physical path identity (macOS /var vs /private/var) for both the shadow chain walk and trust entries.
Group E carry-forward (Decision 14 wiring decision): uninstallInstallVersionMarker was exported+tested but never called, and the digest analysis shows there is no safe insertion point — the marker is a snapshotted removable child of genieHomeRemovalDigest, so deleting it inside the plan-to-execute window poisons the commitment and aborts home removal; before the window it is redundant (wholesale removal already deletes it, symlink-safe); after, a no-op. The function is removed, the module header documents the decision, and two new uninstall tests pin the regular-file and symlink legacy layouts through the REAL digest-verified batch path.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![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: ed75b4327d
ℹ️ 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".
| const codexHome = deps.codexHome ?? resolveCodexDir(); | ||
| const cacheFamily = witnessCodexCacheFamily(codexHome); | ||
| const runner = deps.runner ?? runBoundedIntegrationCommand; | ||
| const result = runner(codexCommand, ['plugin', 'list', '--json'], { |
There was a problem hiding this comment.
When genie doctor runs in a workspace whose PATH resolves codex to a repository-owned executable, this new single-observation path executes that binary directly before the existing probe replay has a chance to call validateTrustedExecutablePath; that regresses the trust boundary the rest of the integration uses to avoid running repo-local CLIs just by inspecting a project. Bind and validate the codexCommand before passing it to the runner, or route the first observation through the same trusted executable resolver.
AGENTS.md reference: AGENTS.md:L37-L37
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| const MAX_CONFIG_BYTES = 512 * 1024; | ||
| const GENIE_SERVER_HEADER_RE = /^\s*\[mcp_servers\.(?:genie|"genie")\]\s*(?:#.*)?$/m; |
There was a problem hiding this comment.
For nested or global Codex configs that define the Genie route using TOML dotted keys, e.g. the same mcp_servers.genie.command = ... shape generated by Genie's own marker block, this regex does not match anything because it only recognizes [mcp_servers.genie] table headers. In that case classifyRouteLayers() misses a nearer same-key route and doctor can report the root marker as healthy even though Codex will read the shadowing layer; parse the TOML or also recognize dotted-key assignments.
Useful? React with 👍 / 👎.
…in the PTY flow Two LOW findings from the independent execution review of PR #2630: - the route-layer classifier only matched the [mcp_servers.genie] section header, missing the dotted-key spelling (mcp_servers.genie.command = ...) that the marker block itself uses — a nested or global route written in that form was a silent false negative - the PTY flow's stage-4 context assertion tightened from toBeDefined() to the exact typed 'project-database-unavailable' warn, exercising the typed context state end-to-end
Ticks Group E's five acceptance criteria and appends the timestamped execution-review block (reviewer SHIP verdict, orchestrator-reproduced validation, the Decision-1 route defect found and fixed in-wave, and the resolved carry-forwards) per the orchestrator-persists-state contract.
1 participant
Wave 3 / Group E: lifecycle-truth-integration
Wires the merged Group A (route/context), B (host observation), and D (delivery) contracts into truthful setup/doctor surfaces — without changing any A/B/D contract (the two consumer seams already existed: the probe's
runreplay and the observer'srunner).Doctor — one bounded observation (Decision 11, AC4/AC5)
src/lib/codex-doctor-observation.ts: doctor previously spawnedcodex plugin list --jsonTWICE (probe + activation observer) with contradictory stderr policy — a benign sandbox PATH advisory produced a healthy check list next to a query-failed summary and exit 1. Now ONE bounded query is classified through B'sparseCodexHostObservationand replayed into both consumers; the advisory rides as sanitized metadata (checks[].advisory), never a second policy decision.src/lib/codex-lifecycle-truth.ts: the shared Decision-9 delivery gate + the deferred typed config-layer classifier (route-collision,route-shadowed,global-route-same-key,untrusted-config,project-trust-required) surfaced as achecks[].routeLayersrider; physical-path identity (macOS/varvs/private/var) for the shadow chain walk and trust entries.delivery-incomplete(exit 1, recovery command) whenever a green state lacks a matching authenticated record; a harder exit-1 state keeps its own presentation withdeliveryComplete:false. NewCodex project contextcheck reuses the MCP server'sresolveProjectContextso doctor and the server can never disagree.Setup — record-gated activation + typed outcome (Decisions 9/12, AC1–AC3)
beginActivationinner guard stays as defense in depth). Missing/invalid/mismatched → one consistentdelivery-incompleterefusal + trailer + the update/install recovery command — even on an already-current machine.SetupCodexOutcomeCode; the green banner and wizard summary flow from THIS run's outcome, never historicalcodex.configuredstate.Real bug found & fixed by the new PTY flow (Decision 1)
Setup's post-activation reconcile still used the pre-Group-A synthetic "usable plugin" probe whose documented behavior removed the project fallback — leaving a repository with NO Codex route at all now that the plugin ships no MCP declaration. Post-activation now reconciles the stable absolute GENIE_HOME facade route exactly as trusted init writes it.
Real-PTY lifecycle flow (deliverable 5)
tests/integration/codex-lifecycle-pty.test.tsdrives the REAL CLI: missing-record refusal → record published via the real deep store under the real lease → consent on a real pty (expect(1)on darwin,script(1)on linux CI) → activation with exactly one plugin add → doctorcurrent; plus PATH advisory, stale historical config, route collision/shadowing, and hard query-failure consistency. Brings its own fake codex (CI has none); skips cleanly where no pty can be allocated.Carry-forward: uninstall marker (Decision 14)
uninstallInstallVersionMarkerwas unwired, and the digest trace shows no safe insertion point (any delete inside the plan→execute window poisonsgenieHomeRemovalDigest; outside it is redundant/no-op). Removed with documented rationale; two new uninstall tests pin the regular-file and symlink legacy layouts through the REAL digest-verified batch path.Validation (all reproduced locally)
bun testfull suite: 2528 tests, sole failure is the pre-existing Linux-onlyss-based ui-bridge socket test (macOS-local; green in CI)bun run smoke:codex: exit 0bun run checkstages (typecheck/lint/dead-code/complexity budget): pass