feat(skills): handoff routing and driver skills, wrap-up port - #299
Conversation
…o 1.0.38 gt:handoff had grown two jobs — deciding where work goes, and driving the receiver. Split into a gt:handoff-to router plus per-target drivers, so a new target can be added without touching the router. Retires gt:handoff.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
🐉 eve review — ✅ APPROVE · 0 findings
Previous runs (4)
|
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 5 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds Codex handoff and supervision workflows, durable SESSION/PLAN wrap-up state documentation and a resolver CLI, plus plugin version, ignore-rule, and lint configuration updates. ChangesAgent workflow and wrap-up features
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Orchestrator
participant Driver
participant Codex
participant ToolsBus
Orchestrator->>Driver: Provide brief, scope, write policy, and verification command
Driver->>ToolsBus: Login and monitor worker messages
Driver->>Codex: Spawn session with the brief
Codex-->>Driver: Emit status, checkpoints, approvals, and verification results
Driver->>Codex: Steer, interrupt, approve, deny, or stop
Driver->>Orchestrator: Send VERDICT, CHANGED, VERIFY, STEERS, and OPEN
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![eve-bot-lovinka[bot]](https://avatars.githubusercontent.com/in/4092731?s=60&v=4){kind=small}
There was a problem hiding this comment.
🐉 eve review — 🔴 Changes requested
a4b60da· 9 actionable findings · view run ↗
| Severity | Count |
|---|---|
| 🟠 High | 1 |
| 🟡 Medium | 2 |
| 🔵 Low | 6 |
|
The review has been completed and posted to GitHub. The Review outcome:
The operator has been notified via Telegram. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 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.
Inline comments:
In `@biome.json`:
- Around line 122-131: Restrict the biome.json linter override for
noRestrictedGlobals to only the specific plugin skill script(s) that require
native JSON, rather than the entire plugins/*/skills/*/scripts/** tree. Update
the affected scripts to use SafeJSON where possible, and remove the broad
exclusion so future unsafe JSON usage remains linted.
In `@plugins/genesis-tools/agents/agent-driver.md`:
- Around line 1-6: Add the missing top-level Markdown heading immediately after
the closing front-matter delimiter in agent-driver.md, using the title
“agent-driver” so markdownlint recognizes the document’s first-level heading.
- Around line 84-90: Update the report template code fence surrounding VERDICT,
CHANGED, VERIFY, STEERS, and OPEN to specify the text language, preserving the
existing non-executable content and formatting.
In `@plugins/genesis-tools/skills/handoff-to-codex/SKILL.md`:
- Around line 68-70: Update the session-storage documentation near the flags
list to state that sessions use ~/.genesis-tools/codex/sessions by default,
while --home changes the effective CODEX_HOME and therefore the session
directory. Keep the existing authentication and session-file details intact.
- Around line 28-35: Add the `text` language identifier to the fenced
pseudo-call block in the handoff-to-codex skill documentation, while preserving
the existing Agent example content.
- Around line 103-112: Fix the approval ownership contract by making request
routing and forwarding explicit: in
plugins/genesis-tools/skills/handoff-to-codex/SKILL.md lines 103-112, route
approval_request messages to driver_<task> or document lead-to-driver
forwarding; in plugins/genesis-tools/agents/agent-driver.md lines 55-70,
document how the driver receives forwarded requests and request IDs before
applying scope checks and resolving approvals.
- Around line 161-165: Update the event-wait command in the handoff instructions
to distinguish a detected terminal event from the 600-second timeout, and
explicitly stop with a timeout error when neither "turn.completed" nor
"turn.failed" is found before the deadline. Preserve the existing terminal-event
matching and polling behavior.
- Line 167: Update the resume command in the handoff instructions to include
--ignore-user-config alongside the existing resume options, preserving that
setting when continuing a thread and leaving the sandbox and output arguments
unchanged.
In `@plugins/genesis-tools/skills/wrap-up/scripts/resolve.ts`:
- Around line 108-157: Add unit tests covering the pure resolver functions slug,
matches, and derivedDocPath in resolve.ts. Verify slug normalization and its
main fallback, matches’ branch-optional behavior and worktree-specific scoring,
and derivedDocPath’s explicit docPath usage versus generated fallback path.
- Around line 104-106: Protect the read-modify-write sequence in cmdRegister
with a cross-process advisory lock covering both loadRegistry and saveRegistry.
Implement lock acquisition with retry and ensure the lock is released in a
finally block, including when registration fails, so concurrent register calls
merge their entries without clobbering each other.
- Around line 369-389: Update the write path in cmdLog to make replacing the
wrap-up file atomic: write the fully constructed body to a temporary file in the
same directory, then rename that temporary file over the target only after the
write succeeds. Preserve the existing rewritten content and section generation,
and clean up the temporary file if writing or renaming fails.
In `@plugins/genesis-tools/skills/wrap-up/SKILL.md`:
- Line 31: Update the Markdown in SKILL.md by adding blank lines around the
headings at lines 31, 34, 58, and 73, and before and after the fenced code
blocks around lines 79–81. Preserve the existing content and structure while
satisfying markdownlint rules MD022 and MD031.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f32c2fe9-54a8-4260-8275-4784f4062b8d
⛔ Files ignored due to path filters (1)
handoff-tab-lightbox.pngis excluded by!**/*.png
📒 Files selected for processing (11)
.claude-plugin/marketplace.json.gitignorebiome.jsonplugins/genesis-tools/.claude-plugin/plugin.jsonplugins/genesis-tools/agents/agent-driver.mdplugins/genesis-tools/skills/agents-talk/SKILL.mdplugins/genesis-tools/skills/handoff-to-codex/SKILL.mdplugins/genesis-tools/skills/handoff-to/SKILL.mdplugins/genesis-tools/skills/handoff/SKILL.mdplugins/genesis-tools/skills/wrap-up/SKILL.mdplugins/genesis-tools/skills/wrap-up/scripts/resolve.ts
💤 Files with no reviewable changes (1)
- plugins/genesis-tools/skills/handoff/SKILL.md
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Never read
process.envdirectly in application TypeScript code; useenvfrom@genesiscz/utils/envand its typed accessors.
Files:
plugins/genesis-tools/skills/wrap-up/scripts/resolve.ts
🧠 Learnings (15)
📚 Learning: 2026-05-17T18:12:35.585Z
Learnt from: genesiscz
Repo: genesiscz/GenesisTools PR: 168
File: src/utils/ai/transcription/TranscriptionManager.ts:138-139
Timestamp: 2026-05-17T18:12:35.585Z
Learning: In genesiscz/GenesisTools, avoid raising a “silent no-op” review finding in src/utils/ai/transcription/TranscriptionManager.ts about TranscriptionOptions.speakers not being forwarded in buildProviderOptions. speakers is intentionally wired for the local diarization path (Transcriber.ts → maybeDiarizeLocal → diarizeLocal({ speakers })) and is not meant to be passed to cloud providers (e.g., Deepgram/AssemblyAI) via TranscriptionManager.buildProviderOptions; downstream usage occurs in Transcriber.ts.
Applied to files:
.gitignore
📚 Learning: 2026-05-18T14:01:18.072Z
Learnt from: genesiscz
Repo: genesiscz/GenesisTools PR: 171
File: src/utils/fs/file-tailer.ts:66-73
Timestamp: 2026-05-18T14:01:18.072Z
Learning: In genesiscz/GenesisTools `src/utils/fs/file-tailer.ts` (class `FileTailer`), the code intentionally sets `this.started = true` before calling `writeFileSync(...)` and then calls `writeFileSync(path, "", { flag: "wx" })` inside a bare `catch {}`.
During review, do not flag this as an incorrect “swallowing errors” pattern or as a misordered `started` assignment. This is deliberate create-or-skip bootstrap logic: `fs.watch` requires the file to exist at registration time, and `{ flag: "wx" }` ensures exclusive creation; the only expected failure is `EEXIST` when another writer already created the file, so ignoring that error is correct for this use case.
Applied to files:
.gitignore
📚 Learning: 2026-02-25T23:00:07.620Z
Learnt from: genesiscz
Repo: genesiscz/GenesisTools PR: 56
File: plugins/genesis-tools/commands/github-pr.md:131-131
Timestamp: 2026-02-25T23:00:07.620Z
Learning: Adopt the style: use lowercase 'markdown' (not 'Markdown') in the GenesisTools documentation. Apply this consistently across all Markdown files in the repository (any .md file), including generated docs and READMEs.
Applied to files:
plugins/genesis-tools/skills/agents-talk/SKILL.mdplugins/genesis-tools/skills/handoff-to/SKILL.mdplugins/genesis-tools/skills/wrap-up/SKILL.mdplugins/genesis-tools/agents/agent-driver.mdplugins/genesis-tools/skills/handoff-to-codex/SKILL.md
📚 Learning: 2026-05-22T18:53:48.562Z
Learnt from: genesiscz
Repo: genesiscz/GenesisTools PR: 183
File: scripts/benchmarks/clones/microbenches/README.md:4-4
Timestamp: 2026-05-22T18:53:48.562Z
Learning: In GenesisTools markdown docs, do not treat `macos` (lowercase) inside backticks as a capitalization error. When `macos` is used as a literal CLI subcommand token (e.g., `tools macos clones ...`) corresponding to the `src/macos/` command implementation, keep it lowercase and ignore capitalization suggestions such as LanguageTool rule `MAC_OS` (it would document the wrong command). Only capitalize `MacOS`/`macOS` when it is clearly prose about the operating system, not when it’s a code-formatted command token.
Applied to files:
plugins/genesis-tools/skills/agents-talk/SKILL.mdplugins/genesis-tools/skills/handoff-to/SKILL.mdplugins/genesis-tools/skills/wrap-up/SKILL.mdplugins/genesis-tools/agents/agent-driver.mdplugins/genesis-tools/skills/handoff-to-codex/SKILL.md
📚 Learning: 2026-02-24T15:32:44.925Z
Learnt from: genesiscz
Repo: genesiscz/GenesisTools PR: 54
File: src/github/lib/review-output.ts:18-20
Timestamp: 2026-02-24T15:32:44.925Z
Learning: In TypeScript files, do not require a blank line between the opening brace of a function and the first statement if the first statement is the if statement immediately after the signature. The blank-line rule applies to separating an if from unrelated preceding code within the same block, not to spacing after the function opening brace. Apply this rule to all TS functions across the codebase.
Applied to files:
plugins/genesis-tools/skills/wrap-up/scripts/resolve.ts
📚 Learning: 2026-03-12T01:26:03.611Z
Learnt from: genesiscz
Repo: genesiscz/GenesisTools PR: 95
File: src/ask/lib/ChatSessionManager.ts:0-0
Timestamp: 2026-03-12T01:26:03.611Z
Learning: Use SafeJSON.parse(text, { strict: true }) for strict RFC 8259 validation in all non-config boundaries (API responses, JSONL, cache, subprocess output). The 3-arg form SafeJSON.parse(text, null, { strict: true }) is invalid and should not be used. Only lenient default (no options) is appropriate for user-authored config files that may contain comments/trailing commas. Apply this guideline across TypeScript files (src/**/*.ts) wherever SafeJSON.parse is used.
Applied to files:
plugins/genesis-tools/skills/wrap-up/scripts/resolve.ts
📚 Learning: 2026-03-12T01:26:18.985Z
Learnt from: genesiscz
Repo: genesiscz/GenesisTools PR: 95
File: src/claude/lib/history/search.ts:0-0
Timestamp: 2026-03-12T01:26:18.985Z
Learning: When using SafeJSON.parse in TypeScript code, prefer the two-argument form SafeJSON.parse(text, { strict: true }) to enable strict RFC 8259 validation via the native JSON.parse. Do NOT use the three-argument form SafeJSON.parse(text, null, { strict: true }). Apply strict parsing at remote/third-party API boundaries, JSONL parsing points, and subprocess output. Fall back to the lenient/default form only for user-authored config files that may legitimately contain comments or trailing commas. This pattern keeps strict validation where appropriate and preserves leniency for internal/config data.
Applied to files:
plugins/genesis-tools/skills/wrap-up/scripts/resolve.ts
📚 Learning: 2026-03-12T01:26:27.000Z
Learnt from: genesiscz
Repo: genesiscz/GenesisTools PR: 95
File: src/debugging-master/commands/tail.ts:0-0
Timestamp: 2026-03-12T01:26:27.000Z
Learning: In the genesiscz/GenesisTools repository, prefer using SafeJSON.parse(text, { strict: true }) (2-argument form) at all non-config JSON boundaries such as API responses, JSONL parsers, cache files, and subprocess stdout. Reserve the lenient default (SafeJSON.parse(text) with no options) only for user-authored config files that may legitimately contain comments or trailing commas.
Applied to files:
plugins/genesis-tools/skills/wrap-up/scripts/resolve.ts
📚 Learning: 2026-03-12T01:26:24.859Z
Learnt from: genesiscz
Repo: genesiscz/GenesisTools PR: 95
File: src/azure-devops/commands/history-sync.ts:0-0
Timestamp: 2026-03-12T01:26:24.859Z
Learning: In GenesisTools, ensure SafeJSON.parse is called with exactly two arguments. Use SafeJSON.parse(text, { strict: true }) for strict RFC 8259 validation, or pass a reviver function as the second argument. Do not call SafeJSON.parse(text, null, { strict: true }) since the function signature does not support a three-argument form. Apply this guideline to all TypeScript files that use SafeJSON.parse (e.g., src/utils/json.ts) and other related code.
Applied to files:
plugins/genesis-tools/skills/wrap-up/scripts/resolve.ts
📚 Learning: 2026-03-17T01:30:56.939Z
Learnt from: genesiscz
Repo: genesiscz/GenesisTools PR: 107
File: src/utils/macos/tts.ts:130-139
Timestamp: 2026-03-17T01:30:56.939Z
Learning: In genesiscz/GenesisTools, do not suggest converting two-argument functions with an optional second parameter (for example setMute(muted: boolean, app?: string)) to an object-parameter form. The project prefers simple positional parameters for short utility functions, even when an optional argument is present. The object-parameter guideline should only apply when a function has 3 or more parameters.
Applied to files:
plugins/genesis-tools/skills/wrap-up/scripts/resolve.ts
📚 Learning: 2026-03-22T22:19:49.876Z
Learnt from: genesiscz
Repo: genesiscz/GenesisTools PR: 119
File: src/indexer/index.ts:41-56
Timestamp: 2026-03-22T22:19:49.876Z
Learning: When using Bun projects, treat `import.meta.dir` as an absolute directory path provided by Bun. If you build paths by concatenating with `import.meta.dir` (e.g., `import.meta.dir + "/file.ts"`), do not require `path.resolve()` as it would be redundant. Only apply `path.resolve()` guidance when the base path is relative (not when the base is already an absolute `import.meta.dir`).
Applied to files:
plugins/genesis-tools/skills/wrap-up/scripts/resolve.ts
📚 Learning: 2026-06-30T19:44:04.852Z
Learnt from: genesiscz
Repo: genesiscz/GenesisTools PR: 227
File: src/agents/tests/matrix-e2e.test.ts:0-0
Timestamp: 2026-06-30T19:44:04.852Z
Learning: In the GenesisTools repo, do not flag code that passes `env: { ...process.env, ... }` into `Bun.spawn()` (i.e., forwarding the inherited environment to a child process) as a violation of the env-helper guideline by itself. Forwarding inherited environment to a subprocess is not the same as application/test logic directly reading configuration from `process.env`. Continue to flag direct `process.env` reads used in TypeScript logic (e.g., feature gates) per the env-helper guideline.
Applied to files:
plugins/genesis-tools/skills/wrap-up/scripts/resolve.ts
📚 Learning: 2026-03-15T16:03:42.969Z
Learnt from: genesiscz
Repo: genesiscz/GenesisTools PR: 106
File: plugins/genesis-tools/skills/github/scripts/actions-cost.ts:9-12
Timestamp: 2026-03-15T16:03:42.969Z
Learning: In genesiscz/GenesisTools, for standalone bun scripts under plugins/genesis-tools (e.g., plugins/genesis-tools/skills/github/scripts/actions-cost.ts), use the invocation bun file.ts, not bun run file.ts. Bun run is reserved for package.json script entries. The shebang #!/usr/bin/env bun marks such scripts as standalone. Do not suggest replacing bun file.ts with bun run file.ts in usage/help text for standalone scripts.
Applied to files:
plugins/genesis-tools/skills/wrap-up/scripts/resolve.ts
📚 Learning: 2026-05-05T11:58:33.420Z
Learnt from: genesiscz
Repo: genesiscz/GenesisTools PR: 163
File: src/indexer/lib/sources/mail-source.dateSent.probe.test.ts:0-0
Timestamp: 2026-05-05T11:58:33.420Z
Learning: This repo uses Biome 2.x. The console lint rule is `noConsole` (located at `lint/suspicious/noConsole`), not `noConsoleLog`. In this codebase, `noConsole` is disabled in `biome.json`, so adding a `// biome-ignore lint/suspicious/noConsole:<...>` suppression comment is a no-op and should be avoided (CI flags it as having no effect). When reviewing, do not suggest adding Biome suppression comments for console usage; if a `console.*` call must remain, leave it without a `biome-ignore` comment.
Applied to files:
plugins/genesis-tools/skills/wrap-up/scripts/resolve.ts
📚 Learning: 2026-05-18T14:02:30.445Z
Learnt from: genesiscz
Repo: genesiscz/GenesisTools PR: 171
File: src/utils/ui/layouts/AuthLayout.tsx:34-34
Timestamp: 2026-05-18T14:02:30.445Z
Learning: When reviewing a PR, before leaving any comment on a specific file and hunk, verify that the file (and the relevant lines) actually exist in the PR’s current diff. For example, use `git diff --name-only <base>...<head>` (or the PR’s file list) to confirm the file is part of the diff, since pre-rebase/stale hunk references can lead to incorrect or outdated comments.
Applied to files:
plugins/genesis-tools/skills/wrap-up/scripts/resolve.ts
🪛 LanguageTool
plugins/genesis-tools/skills/wrap-up/SKILL.md
[grammar] ~71-~71: Ensure spelling is correct
Context: ...olute, ~/, or relative to the project toplevel) wins over vaultDir, which resolves t...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
plugins/genesis-tools/agents/agent-driver.md
[style] ~6-~6: Consider using “who” when you are referring to a person instead of an object.
Context: ... the architect — you are the supervisor that keeps a worker on task and reports hone...
(THAT_WHO)
[style] ~27-~27: Consider using “who” when you are referring to a person instead of an object.
Context: ...lock. Refusing here is cheap. A worker that runs 20 minutes in the wrong direction ...
(THAT_WHO)
plugins/genesis-tools/skills/handoff-to-codex/SKILL.md
[style] ~12-~12: Who is ‘not sure’? Consider being more precise.
Context: ...r at all? gt:handoff-to decides that. Not sure how the bus works? gt:agents-talk. Ev...
(WHO_NOT_SURE)
🪛 markdownlint-cli2 (0.23.1)
plugins/genesis-tools/skills/wrap-up/SKILL.md
[warning] 31-31: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 34-34: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 58-58: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 73-73: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 79-79: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 81-81: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
plugins/genesis-tools/agents/agent-driver.md
[warning] 6-6: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
[warning] 84-84: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
plugins/genesis-tools/skills/handoff-to-codex/SKILL.md
[warning] 28-28: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 SkillSpector (2.4.4)
plugins/genesis-tools/skills/agents-talk/SKILL.md
[warning] 81: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 82: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
plugins/genesis-tools/skills/handoff-to/SKILL.md
[warning] 28: [EA2] Autonomous Decision Making: Skill enables autonomous high-impact decisions without human-in-the-loop verification. Critical operations (destructive commands, financial transactions, data deletion) should require explicit user confirmation.
Remediation: Add human-in-the-loop confirmation for destructive, irreversible, or high-impact operations. Never auto-execute commands that modify files, send data, or alter system state.
(Excessive Agency (EA2))
[error] 6: [RA1] Self-Modification: Skill modifies its own code, configuration, or behavior at runtime. Self-modification enables an agent to escalate privileges, disable safety constraints, or install persistent backdoors.
Remediation: Prevent the skill from modifying its own code, SKILL.md, or configuration files. Treat skill files as read-only at runtime.
(Rogue Agent (RA1))
🔇 Additional comments (9)
plugins/genesis-tools/skills/handoff-to/SKILL.md (1)
1-69: LGTM!plugins/genesis-tools/skills/handoff-to-codex/SKILL.md (1)
1-27: LGTM!Also applies to: 36-67, 71-102, 114-160
plugins/genesis-tools/skills/agents-talk/SKILL.md (1)
137-142: LGTM!plugins/genesis-tools/agents/agent-driver.md (1)
7-54: LGTM!Also applies to: 71-83, 91-97
.claude-plugin/marketplace.json (1)
4-4: LGTM!Also applies to: 14-14
plugins/genesis-tools/.claude-plugin/plugin.json (1)
3-3: LGTM!.gitignore (1)
234-236: LGTM!plugins/genesis-tools/skills/wrap-up/SKILL.md (1)
1-313: Doc is internally consistent withresolve.ts.Registry/config shapes, CLI invocations, and the
YOU-ARE-HEREmarkers all line up with the resolver script's actual implementation.plugins/genesis-tools/skills/wrap-up/scripts/resolve.ts (1)
159-220: LGTM!Also applies to: 393-423
Review fixes — round 1 (PR #299)9 threads from @eve-bot-lovinka. 4 accepted and fixed, 5 rebutted with evidence. No thread was skipped as a formatting nit. Commits:
One fact decides five of the nine findings, so it is worth stating once up front: t1 + t9 — No tests for 423 lines of business logicContext: Why there were no tests (and it wasn't neglect): the file was structurally untestable. The dispatcher ran at import time, so importing the module executed the CLI and called Code before: const [cmd, ...rest] = process.argv.slice(2);
switch (cmd) {
case "resolve":
await cmdResolve();
break;
...
default:
console.error("usage: resolve.ts <resolve | register ...>");
process.exit(1);
}Code after: // Guarded so the pure helpers above can be imported by tests without the CLI
// dispatcher running (and calling process.exit) on import.
if (import.meta.main) {
const [cmd, ...rest] = process.argv.slice(2);
switch (cmd) {
...
}
}How fixed: guarded the dispatcher, exported the pure helpers, and extracted the two highest-risk behaviors as I/O-free pure functions:
Confidence: 95% —
t2 — Bare
|
|
Excellent work on the review response—thorough rebuttals grounded in hard evidence, and the accepted findings are fixed with surgical precision. Key strengthsThe import-constraint argument (scripts ship to t1 (tests) — the t5 (stderr) — preserving the fallback instead of throwing is the right call; the failure message closes the "silent degradation to wrong file" hazard. t7 (handoff) — the grep showing zero dangling references is dispositive; the migration note in One tightening suggestiont3 (spec mismatch) — your rebuttal is factually correct (the plan file is on master, this PR doesn't touch it), but the finding likely came from the reviewer seeing What's next? Do you want me to:
|
…o wrap-up scripts
…t, keep --ignore-user-config on resume
There was a problem hiding this comment.
🐉 eve review — 🟡 Review comments
b705ada· 4 actionable findings · view run ↗
| Severity | Count |
|---|---|
| 🟡 Medium | 1 |
| 🔵 Low | 3 |
|
Delta review completed and posted.
|
Review fixes — round 2 (PR #299)11 threads from @coderabbitai (t10–t20). 9 accepted and fixed, 1 partly accepted, 2 declined with reasons. Also posted a follow-up correction on eve's thread #2, where round 2 produced evidence that changed part of my earlier answer. Commits:
t10 — biome override disables the SafeJSON guard too broadlyContext: The glob matched three directories, not just the new script:
Code before / after: -"includes": ["plugins/*/skills/*/scripts/**"],
+"includes": ["plugins/genesis-tools/skills/wrap-up/scripts/**"],Verified by probe, not by reading the config — a bare Both probe files were removed afterwards; the working tree is clean.
Confidence: 97% — the probe output above is the proof. t19 + t18 — Non-atomic writes can destroy the permanent recordContext: The sharpest finding of the round. Code after — one helper, used by both writers: async function writeAtomic(path: string, body: string): Promise<void> {
const tmp = `${path}.tmp-${process.pid}-${Date.now()}`;
await Bun.write(tmp, body);
await rename(tmp, path);
}The pid is in the temp name so two concurrent processes cannot collide on the temp path itself. On t18's advisory lock — declined, with reasons:
Last-writer-wins therefore remains, deliberately. Confidence: 90% — atomic write verified end to end (header rewritten, prior section preserved, new section appended, no t15 — Approval ownership contract is ambiguousContext: Checked the code before acting. The recipient is hardcoded: I documented the real path rather than changing the routing (that is a code change outside this docs PR, and
Worth adding for accuracy: an unanswered approval cannot cause a silent wrong action. The worker stays paused, so the failure mode is a visible stall, not a bypassed scope check. Confidence: 92% t16 — Wait loop cannot distinguish timeout from successContext: Code after: SECONDS=0; until rg -q '"type":"turn.completed"|"type":"turn.failed"' /tmp/codex-<task>.log 2>/dev/null || [ $SECONDS -ge 600 ]; do sleep 5; done
rg -q '"type":"turn.completed"|"type":"turn.failed"' /tmp/codex-<task>.log || { echo "TIMEOUT after ${SECONDS}s — turn never terminated"; tail -20 /tmp/codex-<task>.log; }How fixed: the quiet part matters here — a timed-out run still leaves a stale Confidence: 95% t17 —
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Commit availability and summaries =='
git show --no-patch --format='%H%n%s%n' 2d6bdf7cedbae75fa2cd40fae4769b5a67a2549b
git show --no-patch --format='%H%n%s%n' 87a9541d611e0ca576f6821a7617bc7c91909942
printf '%s\n' '== Targeted changed files =='
git diff --stat 2d6bdf7cedbae75fa2cd40fae4769b5a67a2549b^ 2d6bdf7cedbae75fa2cd40fae4769b5a67a2549b
git diff --stat 87a9541d611e0ca576f6821a7617bc7c91909942^ 87a9541d611e0ca576f6821a7617bc7c91909942
printf '%s\n' '== Relevant current-file outlines =='
ast-grep outline plugins/genesis-tools/skills/wrap-up/scripts/resolve.ts --items all --type function --match 'writeAtomic|saveRegistry|cmdLog'
ast-grep outline src/codex/lib/paths.ts --items all --type function --match 'codexRoot|sessionsDir'
printf '%s\n' '== Biome override and atomic-write call sites =='
rg -n -A10 -B3 'plugins/genesis-tools/skills/wrap-up/scripts|function writeAtomic|writeAtomic\(' \
biome.json plugins/genesis-tools/skills/wrap-up/scripts/resolve.ts
printf '%s\n' '== Approval, timeout, resume, and fence documentation =='
rg -n -A8 -B4 'approval_request|always `lead`|tail --follow|TIMEOUT|turn.completed|ignore-user-config|^# Agent Driver|^```text' \
plugins/genesis-tools/skills/handoff-to-codex/SKILL.md \
plugins/genesis-tools/agents/agent-driver.md
printf '%s\n' '== Session directory and CODEX_HOME references =='
sed -n '1,100p' src/codex/lib/paths.ts
rg -n -A4 -B4 'CODEX_HOME|sessionsDir\(' src/codex plugins/genesis-tools/skills/handoff-to-codex/SKILL.mdLength of output: 26103
The category correction for ✏️ Learnings added
|
There was a problem hiding this comment.
🐉 eve review — 🟡 Review comments
87a9541· 4 actionable findings · view run ↗
| Severity | Count |
|---|---|
| 🟡 Medium | 2 |
| 🔵 Low | 2 |
…verride, survive missing git
|
PR #299 delta review completed and posted.
|
Review fixes — round 3 (PR #299)4 new threads from @eve-bot-lovinka on the round-2 commits. 3 accepted, 1 rebutted with a concrete alternative. This round also reverses a position I argued in rounds 1 and 2, and turned up a real bug that no reviewer had flagged. Commit:
t13 + t14 —
|
|
This is a materially stronger round than the earlier rebuttals.
The running tally is credible because every accepted item names the behavioral change and every rebuttal names a concrete runtime or repository constraint. One optional tightening for the PR/thread reply: phrase t12’s follow-up as “deferred design work, not rejected testability”. That makes clear you accept the long-term value of testing the cache behavior while reasonably declining API expansion in this bug-fix commit. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Commit summary and changed paths =='
git show --no-patch --format='%H%n%s%n' 6a6ab0a28a0ad061bb678a7e970600d2bca55226
git diff --stat 6a6ab0a28a0ad061bb678a7e970600d2bca55226^ 6a6ab0a28a0ad061bb678a7e970600d2bca55226
printf '%s\n' '== Targeted commit diff =='
git diff --find-renames --unified=4 \
6a6ab0a28a0ad061bb678a7e970600d2bca55226^ \
6a6ab0a28a0ad061bb678a7e970600d2bca55226 -- \
biome.json \
plugins/genesis-tools/skills/wrap-up/scripts/resolve.ts \
plugins/genesis-tools/skills/wrap-up/scripts/resolve.test.ts
printf '%s\n' '== Relevant function outlines =='
ast-grep outline plugins/genesis-tools/skills/wrap-up/scripts/resolve.ts \
--items all --type function --match 'sh|loadPluginConfig|readPluginConfig|registryPath'
ast-grep outline plugins/genesis-tools/skills/wrap-up/scripts/resolve.test.ts \
--items all --type function --match 'sh|loadPluginConfig'
printf '%s\n' '== Current suppressions, config override references, implementation, and tests =='
rg -n -A8 -B4 \
'noRestrictedGlobals|JSON\.(parse|stringify)|export async function sh|Bun\.spawn|loadPluginConfig|could not run|missing binary|no-such-binary' \
biome.json \
plugins/genesis-tools/skills/wrap-up/scripts/resolve.ts \
plugins/genesis-tools/skills/wrap-up/scripts/resolve.test.tsLength of output: 29287
Your decision not to expand the caching change solely to inject test paths is reasonable given its small scope and the current module-level filesystem design. I also note the correction about the stale scoped-exemption learning. I do not have its learning UUID in this thread, so I cannot remove it programmatically; it can be deleted from the CodeRabbit Learnings UI. 🐇✓ ✏️ Learnings added
|
…n up temp on failure
Review fixes — round 4 (PR #299, final round)4 new threads from @eve-bot-lovinka on the round-3 commits. All 4 accepted and fixed. Two of them (t16, t17) were regressions I introduced in round 2 — the review caught my own fixes going wrong, which is the loop working as intended. Commits:
t16 — Atomic write silently widened file permissionsContext:
The round-2 change added temp-file + Code after: const existed = await Bun.file(path).exists();
await Bun.write(tmp, body);
if (existed) {
// rename() swaps in a brand-new inode created under the current
// umask, so a private 0600 registry or wrap-up doc would silently
// widen to 0644. Carry the destination's mode over to the temp file.
const { mode } = await stat(path);
await chmod(tmp, mode & 0o777);
}
await rename(tmp, path);Deliberately not done: imposing a restrictive mode on new files. Wrap-up docs are created inside the user's Obsidian vault and should match the rest of it; a plugin script inventing Confidence: 95% — pinned by a regression test asserting t17 — §1 and §6 gave contradictory approval instructionsContext: Round 2 added a §6 paragraph saying approvals do not arrive on the driver's login stream, but left §1 asserting that login is "how ... approvals reach you". A driver reading top-to-bottom would follow §1 and wait on the wrong stream — precisely the stall the §6 paragraph was added to prevent. Code after (§1):
This keeps the login mandatory for the reasons it genuinely is (steering, plus forwarded approvals) without claiming approvals originate there. Confidence: 93% t15 — Timeout guard returned exit status 0Context: The fallback group's last command was -... tail -20 /tmp/codex-<task>.log; }
+... tail -20 /tmp/codex-<task>.log; exit 1; }Noted in the reply that this is written for the agent's own non-interactive invocation, where Confidence: 96% t18 — No coverage for the atomic write pathContext:
That last case drove a code change, not just coverage: the previous version leaked the temp file next to the real one on any rename failure. All tests use Confidence: 94% Verification
Final tally across four rounds
I am treating this as the final review round. The branch is up to date at |
|
This is a strong final state: all four round-4 findings were accepted, fixed, and backed by regression coverage. The atomic-write work is especially solid now—content replacement, permission preservation, and temp-file cleanup are all exercised in isolated temp directories. One wording correction before closing the PR:
is not quite accurate. t16 explicitly flagged the
Or, if you want to distinguish the review signal:
Otherwise, the final tally is clear and credible: 28 threads addressed, 20 fixes, 8 evidence-backed rebuttals, and coverage increased from zero to 42 tests. |
There was a problem hiding this comment.
🐉 eve review — 🟡 Review comments
f7c10c8· 3 actionable findings · view run ↗
| Severity | Count |
|---|---|
| 🟡 Medium | 2 |
| 🔵 Low | 1 |
|
Delta review completed and posted for
|
Review fixes — round 5 (PR #299)3 threads from @eve-bot-lovinka, all on code I added in earlier rounds. All 3 taken; one with a correction to the stated mechanism. One of them was a genuine bug with a test that was lying about its own contract. Commit:
t19 —
|
|
Round 5 is sound, and the corrections are appropriately precise.
One tally wording adjustment: the “test whose name overstated what it covered” was flagged by t19, so it should not be included among bugs “that no reviewer flagged.” More accurate alternatives:
Or, preserving the distinction:
At this point, the branch has a very defensible final state: the standalone script has moved from no coverage to 43 targeted tests, while each subsequent review round has tightened real runtime contracts rather than merely satisfying lint or style concerns. |
There was a problem hiding this comment.
🐉 eve review — ✅ Approved
7448809· 0 actionable findings · view run ↗
|
Delta review completed and posted for
Clean delta — no actionable concerns in the new changes. |
1 participant
What
Ports the handoff/wrap-up workflow into the
genesis-toolsplugin and retires the oldgt:handoffskill.gt:handoff-to— routing skill that picks the right receiving end for a handoff.gt:handoff-to-codex— driver skill for handing a bounded task to a Codex CLI session.agent-driversubagent — drives one external worker session end to end (spawn, watch, steer, resolve approvals, verify, tear down) so the worker's event stream stays out of the orchestrator's context.gt:wrap-up— moved into the plugin, gained a plan-handoff mode, and now shares the plugins config.gt:handoffis retired in its favour..claude-plugin/marketplace.json×2 and the plugin's ownplugin.json).Why
gt:handoffhad grown two jobs — deciding where work goes and actually driving the receiver. Splitting it into a router (handoff-to) plus per-target drivers makes each piece testable and lets a new target be added without touching the router.Scope
plugins/**,.claude-plugin/**,biome.json,.gitignore,handoff-tab-lightbox.png.Split out of #296. File-disjoint from the sibling PRs, so it can merge in any order.
Summary by CodeRabbit
New Features
Documentation
Chores