Use durable Codex startup instructions - #39
Conversation
📝 WalkthroughWalkthroughThis PR refactors Codex instruction delivery from detached tmux prompt injection to durable config-injected developer instructions. It introduces managed instruction file blocks that preserve user content, implements preamble config injection via ChangesCodex Developer Instructions Configuration & Delivery
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 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 docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/session-bootstrap.test.ts (1)
1-92: ⚖️ Poor tradeoffConsider adding test coverage for new continuity preamble methods.
The new
buildCodexForkContinuityPreambleandbuildCodexMigrationContinuityPreamblemethods inSessionBootstrapServicedon't have direct unit tests in this file. The integration is tested viasession-launch.test.ts, but unit tests would help verify the preamble content directly.🤖 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 `@src/session-bootstrap.test.ts` around lines 1 - 92, Add unit tests in this file for SessionBootstrapService.buildCodexForkContinuityPreamble and buildCodexMigrationContinuityPreamble: instantiate SessionBootstrapService with the existing deps, call each method with representative args (sessionId, parent/target info, any team/role flags), and assert the returned preamble contains expected key phrases (e.g., session IDs, migration/fork guidance, teammate/continuity warnings) and does not contain disallowed strings; reference the method names buildCodexForkContinuityPreamble and buildCodexMigrationContinuityPreamble to locate where to add the tests.src/session-bootstrap.ts (1)
335-359: 💤 Low valueInconsistent instruction formatting between fork and migration preambles.
In
buildCodexForkContinuityPreamble(line 328), the instruction is prefixed with"Instruction: ", but here inbuildCodexMigrationContinuityPreamble(line 354), the instruction is added raw without the prefix.Suggested fix for consistency
activitySummary ? `Recent session activity: ${activitySummary}` : undefined, - instruction?.trim(), + instruction?.trim() ? `Instruction: ${instruction.trim()}` : undefined, "After reading them, briefly summarize what we were doing in the new worktree and continue from that context.",🤖 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 `@src/session-bootstrap.ts` around lines 335 - 359, The migration preamble built by buildCodexMigrationContinuityPreamble appends instruction?.trim() without the "Instruction: " prefix used in buildCodexForkContinuityPreamble, causing inconsistent UX; update buildCodexMigrationContinuityPreamble so that when instruction is present it is prefixed the same way (e.g., `Instruction: ${instruction.trim()}`) before filtering and joining, referencing the function buildCodexMigrationContinuityPreamble and the instruction variable to locate the change.
🤖 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.
Nitpick comments:
In `@src/session-bootstrap.test.ts`:
- Around line 1-92: Add unit tests in this file for
SessionBootstrapService.buildCodexForkContinuityPreamble and
buildCodexMigrationContinuityPreamble: instantiate SessionBootstrapService with
the existing deps, call each method with representative args (sessionId,
parent/target info, any team/role flags), and assert the returned preamble
contains expected key phrases (e.g., session IDs, migration/fork guidance,
teammate/continuity warnings) and does not contain disallowed strings; reference
the method names buildCodexForkContinuityPreamble and
buildCodexMigrationContinuityPreamble to locate where to add the tests.
In `@src/session-bootstrap.ts`:
- Around line 335-359: The migration preamble built by
buildCodexMigrationContinuityPreamble appends instruction?.trim() without the
"Instruction: " prefix used in buildCodexForkContinuityPreamble, causing
inconsistent UX; update buildCodexMigrationContinuityPreamble so that when
instruction is present it is prefixed the same way (e.g., `Instruction:
${instruction.trim()}`) before filtering and joining, referencing the function
buildCodexMigrationContinuityPreamble and the instruction variable to locate the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f0200a7f-0e8d-404e-8368-0df821d872db
📒 Files selected for processing (12)
README.mddocs/tool-integration.mdpackage.jsonscripts/verify-codex-developer-instructions.shsrc/config.tssrc/multiplexer/index.tssrc/multiplexer/runtime-lifecycle-methods.test.tssrc/multiplexer/runtime-lifecycle-methods.tssrc/multiplexer/session-launch.test.tssrc/multiplexer/session-launch.tssrc/session-bootstrap.test.tssrc/session-bootstrap.ts
Summary
Verification
Summary by CodeRabbit
New Features
Bug Fixes
Documentation