Repository navigation
chore: rolling promotion dev -> main - #1124
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughBumps genie plugin/package versions to 4.260410.3; adds skills lint/audit scripts; adds SpawnParams.skipHooks and per-chat agent scoping; replaces in-process omni-bridge singleton with pidfile + NATS ping IPC, queue-group subscriptions, message deduplication, stricter pidfile lifecycle, and updated doctor/serve integrations; exports scaffold helper and seeds auto-memory files. Changes
Sequence Diagram(s)mermaid Serve->>Bridge: start() → create pidfile (O_EXCL) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
- Make omni-bridge a mandatory subsystem of genie serve - Add pidfile at ~/.genie/state/omni-bridge.json with stale-PID recovery - Subscribe to omni.bridge.ping NATS subject for out-of-process status - Add src/lib/bridge-status.ts IPC helper (2s timeout, returns running/stopped/stale) - On start, probe existing pidfile via process.kill(pid, 0); reclaim if ESRCH - Fail fast with clear error when pidfile is locked by a live PID Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- scripts/skills-lint.ts: parses bash fences in plugins/genie/skills/*, validates genie/omni/workit commands against the real CLI reference. - scripts/skills-audit.ts: enforces <30% line-churn per skill file against origin/dev baseline. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…eanup, dedup, orphan-all
fix: bridge session isolation — NATS queues, per-chat agents, dedup, cleanup
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/services/omni-bridge.ts`:
- Around line 591-607: The current dedup logic in the message handler uses
this.recentMessageIds and does a synchronous O(n) prune when size > 1000, which
can block the hot path; replace this with an LRU-style bounded cache (e.g., swap
this.recentMessageIds for an LRU cache implementation or use the lru-cache
package) or move pruning off the hot path by scheduling periodic cleanup
(setInterval) that prunes entries older than TTL, and update uses in the message
loop (the code referencing parsed.messageId, messageId check,
recentMessageIds.has, and recentMessageIds.set) to use the new cache API so
message processing no longer performs full-map iteration.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 80a55726-9004-4730-ab80-55dc17406da6
📒 Files selected for processing (7)
.claude-plugin/marketplace.jsonpackage.jsonplugins/genie/.claude-plugin/plugin.jsonplugins/genie/package.jsonsrc/lib/provider-adapters.tssrc/services/executors/claude-code.tssrc/services/omni-bridge.ts
- Delete unused omni start/stop/status subcommands (bridge managed exclusively by serve) - Update doctor checkBridge() to use IPC-based getBridgeStatus() instead of getBridge() - Now reports bridge state from pidfile + NATS ping instead of in-process singleton - Handles running, stopped, and stale pidfile states correctly Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Bridge-status is a public API module used by both tests and doctor command. Adding to entry points allows knip to treat exported types as intentional API surface. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Avoid awkward 'Bridge running not running' output by using 'Bridge' as the label and putting the running/stopped state in the message. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
fix(bridge,doctor,omni): unify-bridge-revamp-skills wish — Group 5 review complete
BREAKING: `genie omni bridge`, `genie omni start`, `genie omni stop`, and `genie omni status` are gone. The bridge is managed exclusively by `genie serve` and health is reported through `genie doctor`, which now probes the bridge across processes via pidfile + NATS ping. - src/term-commands/omni.ts: deleted; genie.ts no longer registers it - src/services/omni-bridge.ts: removed getBridge() and bridgeInstance singleton (start/stop no longer mutate module-scoped state) - src/genie-commands/doctor.ts: checkBridge() now calls getBridgeStatus from bridge-status.ts. Output includes pid, uptime, NATS ping latency, and subject list. Stale pidfiles are removed on detection so the next serve start can retake cleanly. PingOptions/BridgeStatusResult are consumed via typed parameters - src/term-commands/serve.ts: printBridgeStatus() uses IPC helper so the out-of-process `genie serve status` view is accurate even when the serve daemon runs in a separate process. Stale GENIE_EXECUTOR_TYPE comment dropped - CHANGELOG.md: record the breaking change
All groups validated successfully: - Wave 1: Bridge IPC + pidfile infrastructure (merged from origin/unify-bridge) - Wave 2: Fallback config, skills audit/lint, legacy command removal, doctor IPC integration - E2E checks: All passing - Validation: typecheck ✓, lint ✓, tests ✓ Ready for ship. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Avoid 'Bridge running bridge is stopped' duplication by using 'Bridge' as the label and putting the running/stopped state in the message. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/services/omni-bridge.ts (1)
303-325:⚠️ Potential issue | 🔴 CriticalTake the pidfile lock before exposing this instance as a live bridge.
This instance subscribes to the
genie-bridgequeue group, starts the idle timer, and assignsbridgeInstancebefore ownership is proven. If a second bridge loses the pidfile race, it can briefly steal live traffic from the real bridge, then its rollback clears the winner's singleton and leaves those pre-lock handles behind. Acquire the pidfile immediately after NATS connect, then registerbridgeInstanceand start consumers only after the lock succeeds.Also applies to: 356-425
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/omni-bridge.ts` around lines 303 - 325, The code currently assigns the singleton bridgeInstance and starts consumers/timers (this.sub & processSubscription(), processTurnEvents(), processSessionResetEvents(), idleCheckTimer) before acquiring the pidfile lock; change the flow so that immediately after NATS connect you attempt to acquire the pidfile lock and only on successful lock do you (1) set bridgeInstance, (2) subscribe and call processSubscription(), (3) create turn subscriptions and call processTurnEvents(sub) for each, (4) subscribe and call processSessionResetEvents(sessionResetSub), and (5) start the idleCheckTimer; if the lock fails, ensure no subscriptions/timers are registered and the singleton is not set so rollback cannot clear a winner’s instance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.genie/wishes/genie-wishes-backup/unify-bridge-revamp-skills/WISH.md:
- Around line 233-241: Remove the duplicate NATS subject collision entry in the
risk table: keep one of the two rows that describe the "NATS ping subject
collides with existing omni subjects" / "NATS subject `omni.bridge.ping`
collides with existing Omni subscriber" items and delete the other, ensuring the
table pipes and alignment remain valid and that the remaining row preserves the
chosen severity ("Low") and mitigation text; also scan the same table for any
other duplicated rows and collapse them similarly to avoid repeated risk
entries.
In `@scripts/skills-lint.ts`:
- Around line 35-43: The getGenieCommands function currently shells out to
whatever "genie" is on PATH; change it to run the repo's CLI entry point instead
(resolve the package's bin from the repo ROOT and execute that file with
"--help"), e.g. compute the repo root (or use the existing ROOT constant),
resolve the package "bin" entry or join ROOT with the local CLI file, and call
execFileSync/execSync with the resolved file and ["--help"] (avoid shell
redirections and plain "genie --help"); update getGenieCommands to use that
resolved path so CI probes the repo's CLI rather than a global binary.
In `@src/lib/bridge-status.test.ts`:
- Around line 27-30: The afterEach cleanup in bridge-status.test.ts currently
restores GENIE_HOME by assigning undefined which sets the string "undefined";
instead, when origHome is undefined remove the environment variable using delete
process.env.GENIE_HOME, otherwise restore the original value; update the
afterEach block that references origHome, process.env.GENIE_HOME and tmpDir (and
keeps the rmSync(tmpDir, { recursive: true, force: true }) call) so unset state
is properly restored.
In `@src/lib/bridge-status.ts`:
- Around line 136-143: The pong parse uses a bare type assertion which can yield
undefined fields; mirror the defensive parsing used in readBridgePidfile by
validating the decoded JSON (sc.decode(msg.data)) against the BridgePong shape
before using pong.pid: check that the parsed object has a numeric pid (and any
other required fields), and if validation fails, handle it gracefully (e.g.,
return a non-running status or include an explicit error detail) rather than
interpolating possibly undefined pong.pid into the detail string.
In `@src/services/omni-bridge.ts`:
- Around line 750-766: The current dedup logic records messageId into
this.recentMessageIds before validation and before calling
enqueue()/routeMessage(), which can cause permanently dropped messages if
processing later throws; remove the early this.recentMessageIds.set(messageId,
...) and instead add the messageId to this.recentMessageIds only after the
message has been processed successfully (i.e., after enqueue() / routeMessage()
completes), or alternatively wrap processing in a try/catch and ensure you
delete the messageId from this.recentMessageIds on any failure; keep the
existing pruning logic for recentMessageIds and continue to check for duplicates
with the existing messageId variable.
- Around line 663-680: recoverSessions() currently calls
BridgeSessionStore.markAllOrphaned(), which abandons surviving tmux panes;
change this so startup either rehydrates live sessions or ensure shutdown tears
them down: replace the single markAllOrphaned() call with logic that queries the
active session rows from BridgeSessionStore (e.g., a getActive/getAllActive
method) and for each row attempt to reattach/rehydrate the bridge runtime using
the existing tmux/bridge attach routine (or, if reattach fails, mark that
specific row orphaned), or alternatively add explicit teardown in stop() to kill
remaining tmux panes so recoverSessions() can safely markAllOrphaned; update
uses of recoverSessions(), BridgeSessionStore.markAllOrphaned, and stop()
accordingly.
In `@src/term-commands/serve.ts`:
- Around line 584-590: The bridge startup catch block currently calls
process.exit(1) after logging, which bypasses the shared shutdown/cleanup (and
leaves serve.pid and orphaned services); instead, keep the existing
console.error but do not call process.exit — either rethrow the caught error
(throw err) so the common cleanup path can run, or invoke the project's central
shutdown/cleanup function (the same path used elsewhere) before exiting; update
the catch in serve.ts around bridge.start() to log the error and then propagate
it via throw err (or call the central shutdown function) rather than calling
process.exit(1).
---
Outside diff comments:
In `@src/services/omni-bridge.ts`:
- Around line 303-325: The code currently assigns the singleton bridgeInstance
and starts consumers/timers (this.sub & processSubscription(),
processTurnEvents(), processSessionResetEvents(), idleCheckTimer) before
acquiring the pidfile lock; change the flow so that immediately after NATS
connect you attempt to acquire the pidfile lock and only on successful lock do
you (1) set bridgeInstance, (2) subscribe and call processSubscription(), (3)
create turn subscriptions and call processTurnEvents(sub) for each, (4)
subscribe and call processSessionResetEvents(sessionResetSub), and (5) start the
idleCheckTimer; if the lock fails, ensure no subscriptions/timers are registered
and the singleton is not set so rollback cannot clear a winner’s instance.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 14e4080b-8a6a-482b-a655-f9511a74ef9d
📒 Files selected for processing (106)
.claude-plugin/marketplace.json.genie/brainstorm.md.genie/wishes/genie-wishes-backup/agent-flexibility-guide/WISH.md.genie/wishes/genie-wishes-backup/agent-stability-hardening/WISH.md.genie/wishes/genie-wishes-backup/auto-orchestrate/WISH.md.genie/wishes/genie-wishes-backup/daily-metrics-agent/WISH.md.genie/wishes/genie-wishes-backup/docs-overhaul/WISH.md.genie/wishes/genie-wishes-backup/docs-readme-review/AUDIT-REPORT.md.genie/wishes/genie-wishes-backup/docs-readme-review/WISH.md.genie/wishes/genie-wishes-backup/dx-800-closeout/WISH.md.genie/wishes/genie-wishes-backup/feature-matrix-page/WISH.md.genie/wishes/genie-wishes-backup/fire-and-forget/WISH.md.genie/wishes/genie-wishes-backup/fix-agent-join-delay/WISH.md.genie/wishes/genie-wishes-backup/fix-cli-polish/WISH.md.genie/wishes/genie-wishes-backup/fix-depends-parser/WISH.md.genie/wishes/genie-wishes-backup/fix-dispatch-initial-prompt/WISH.md.genie/wishes/genie-wishes-backup/fix-first-run/WISH.md.genie/wishes/genie-wishes-backup/fix-genie-v4-stability/WISH.md.genie/wishes/genie-wishes-backup/fix-metrics-agent/WISH.md.genie/wishes/genie-wishes-backup/fix-native-inbox/WISH.md.genie/wishes/genie-wishes-backup/fix-omni-bridge-hardening/WISH.md.genie/wishes/genie-wishes-backup/fix-release-blockers/WISH.md.genie/wishes/genie-wishes-backup/fix-session-continue-fallback/WISH.md.genie/wishes/genie-wishes-backup/fix-session-hook-channel/WISH.md.genie/wishes/genie-wishes-backup/fix-session-uuid-resume/WISH.md.genie/wishes/genie-wishes-backup/fix-short-name-resolver/WISH.md.genie/wishes/genie-wishes-backup/fix-task-project/WISH.md.genie/wishes/genie-wishes-backup/fix-team-lead-exit/WISH.md.genie/wishes/genie-wishes-backup/fix-team-lead-polling/WISH.md.genie/wishes/genie-wishes-backup/fix-test-safety/WISH.md.genie/wishes/genie-wishes-backup/fix-tmux-dual-statusbar/WISH.md.genie/wishes/genie-wishes-backup/fix-tmux-session-explosion/WISH.md.genie/wishes/genie-wishes-backup/fix-trust-prompt/WISH.md.genie/wishes/genie-wishes-backup/genie-app-v2-ui/WISH.md.genie/wishes/genie-wishes-backup/genie-base-skill/WISH.md.genie/wishes/genie-wishes-backup/genie-boards-workflows/WISH.md.genie/wishes/genie-wishes-backup/genie-dx-polish/WISH.md.genie/wishes/genie-wishes-backup/genie-export-import/WISH.md.genie/wishes/genie-wishes-backup/genie-final-polish/WISH.md.genie/wishes/genie-wishes-backup/genie-hacks-community-docs/WISH.md.genie/wishes/genie-wishes-backup/genie-hardening/WISH.md.genie/wishes/genie-wishes-backup/genie-item-registry/WISH.md.genie/wishes/genie-wishes-backup/genie-metaverse/WISH.md.genie/wishes/genie-wishes-backup/genie-observability/WISH.md.genie/wishes/genie-wishes-backup/genie-omni-marriage/WISH.md.genie/wishes/genie-wishes-backup/genie-orchestration-fix/WISH.md.genie/wishes/genie-wishes-backup/genie-resume/WISH.md.genie/wishes/genie-wishes-backup/genie-scheduler/WISH.md.genie/wishes/genie-wishes-backup/genie-simulations/WISH.md.genie/wishes/genie-wishes-backup/genie-stats-command/WISH.md.genie/wishes/genie-wishes-backup/genie-task-tables/WISH.md.genie/wishes/genie-wishes-backup/genie-workflow-engine/WISH.md.genie/wishes/genie-wishes-backup/hook-only-first-install/WISH.md.genie/wishes/genie-wishes-backup/inbox-driven-sessions/WISH.md.genie/wishes/genie-wishes-backup/messaging-refresh/WISH.md.genie/wishes/genie-wishes-backup/multi-agent-session-isolation/WISH.md.genie/wishes/genie-wishes-backup/omni-session-isolation/WISH.md.genie/wishes/genie-wishes-backup/parallel-execution/WISH.md.genie/wishes/genie-wishes-backup/pg-state-migration/WISH.md.genie/wishes/genie-wishes-backup/pg-total-migration/WISH.md.genie/wishes/genie-wishes-backup/pgserve-embed/WISH.md.genie/wishes/genie-wishes-backup/qa-dev-to-main/WISH.md.genie/wishes/genie-wishes-backup/readme-v4-agent-first/WISH.md.genie/wishes/genie-wishes-backup/resilient-messaging/WISH.md.genie/wishes/genie-wishes-backup/resilient-resume/WISH.md.genie/wishes/genie-wishes-backup/session-auto-create/WISH.md.genie/wishes/genie-wishes-backup/session-continue-by-name/WISH.md.genie/wishes/genie-wishes-backup/skills-v4-upgrade/WISH.md.genie/wishes/genie-wishes-backup/stable-release-fixes/WISH.md.genie/wishes/genie-wishes-backup/task-auto-close-on-merge/WISH.md.genie/wishes/genie-wishes-backup/task-external-linking/WISH.md.genie/wishes/genie-wishes-backup/task-lifecycle-foundation/WISH.md.genie/wishes/genie-wishes-backup/task-projects/WISH.md.genie/wishes/genie-wishes-backup/team-lead-minimal/WISH.md.genie/wishes/genie-wishes-backup/test-pg-ram-isolation/TRACE.md.genie/wishes/genie-wishes-backup/test-pg-ram-isolation/WISH.md.genie/wishes/genie-wishes-backup/test-schema-isolation/WISH.md.genie/wishes/genie-wishes-backup/tmux-split-tabbar/WISH.md.genie/wishes/genie-wishes-backup/tmux-tui/WISH.md.genie/wishes/genie-wishes-backup/transcript-docs/WISH.md.genie/wishes/genie-wishes-backup/unified-executor-layer/AUDIT.md.genie/wishes/genie-wishes-backup/unified-executor-layer/WISH.md.genie/wishes/genie-wishes-backup/unified-omni-bridge/INTEGRATION-FROM-OMNI.md.genie/wishes/genie-wishes-backup/unified-omni-bridge/WISH.md.genie/wishes/genie-wishes-backup/unify-bridge-revamp-skills/WISH.md.genie/wishes/genie-wishes-backup/unique-leader-names/WISH.md.genie/wishes/genie-wishes-backup/v3-fixes-release/WISH.md.genie/wishes/genie-wishes-backup/v4-critical-fixes/WISH.md.genie/wishes/genie-wishes-backup/voice-personality-pass/WISH.md.genie/wishes/genie-wishes-backup/work-fire-forget/WISH.md.genie/wishes/genie-wishes-backup/workflow-engine-runtime/WISH.md.genie/wishes/genie-wishes-backup/worktree-out-of-repo/WISH.md.genie/wishes/unify-bridge-revamp-skills/WISH.mdknip.jsonpackage.jsonplugins/genie/.claude-plugin/plugin.jsonplugins/genie/package.jsonscripts/skills-audit.tsscripts/skills-lint.tssrc/genie-commands/doctor.tssrc/lib/bridge-status.test.tssrc/lib/bridge-status.tssrc/services/__tests__/omni-bridge-pidfile.test.tssrc/services/omni-bridge.tssrc/term-commands/omni.tssrc/term-commands/serve.ts
Comment on lines
+233
to
+241
| | NATS ping subject collides with existing omni subjects | Low | Use `omni.bridge.ping` — not under `omni.message.>`; verified via `grep` on subject literals in omni repo before implementing. | | ||
| | Pidfile becomes stale after a crash and confuses status | Medium | Status checks `kill -0 pid` before trusting the file; stale → report "stale", not "running". | | ||
| | Removing `genie omni bridge` commands breaks automation scripts in user environments | Medium | Breaking change is explicitly in scope per user directive; CHANGELOG entry + release notes mitigate surprise. | | ||
| | Turn-monitor migration touches a hot table and causes downtime | Low | Drizzle adds columns with defaults — no rewrite, no lock beyond metadata. | | ||
| | Skills lint script's CLI grep is fragile against multi-line commands | Medium | Start with strict "first token on a bash line" matching; add bailout escape hatch `<!-- skills-lint:ignore -->` for known-complex examples. | | ||
| | Skills audit scope creep into rewrites | High | Reviewer rejects any skill diff that isn't a reference fix; `bun run skills:audit` enforces a <30% line-churn limit per file; conceptual changes get their own follow-up wish. | | ||
| | Turn-monitor caches instance config and ignores live CLI updates | Medium | Group 2 deliverable #8 mandates no-cache (or ≤5s TTL) reads on each tick; live-update integration test in Group 2 acceptance criteria. | | ||
| | `getBridge()`/`bridgeInstance` references linger in test mocks | Medium | Group 4 grep covers both `src/` and `*.test.ts`; CI fails on any match. | | ||
| | NATS subject `omni.bridge.ping` collides with existing Omni subscriber | Low | Pre-implementation grep check is a Group 1 acceptance criterion; evidence captured in the PR. | |
There was a problem hiding this comment.
Duplicate risk entry in the table.
Lines 233 and 241 both document the same NATS subject collision risk:
- Line 233: "NATS ping subject collides with existing omni subjects"
- Line 241: "NATS subject
omni.bridge.pingcollides with existing Omni subscriber"
Remove one of these duplicate entries for clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.genie/wishes/genie-wishes-backup/unify-bridge-revamp-skills/WISH.md around
lines 233 - 241, Remove the duplicate NATS subject collision entry in the risk
table: keep one of the two rows that describe the "NATS ping subject collides
with existing omni subjects" / "NATS subject `omni.bridge.ping` collides with
existing Omni subscriber" items and delete the other, ensuring the table pipes
and alignment remain valid and that the remaining row preserves the chosen
severity ("Low") and mitigation text; also scan the same table for any other
duplicated rows and collapse them similarly to avoid repeated risk entries.
Comment on lines
+35
to
+43
| function getGenieCommands(): Set<string> { | ||
| const out = execSync('genie --help', { encoding: 'utf8' }); | ||
| return collectSubcommands(out); | ||
| } | ||
|
|
||
| function getOmniCommands(): Set<string> { | ||
| let out: string; | ||
| try { | ||
| out = execSync('omni --help --all 2>/dev/null || omni --help', { encoding: 'utf8' }); |
There was a problem hiding this comment.
Probe the repo’s genie CLI, not whichever binary is on PATH.
execSync('genie --help') validates skills against the caller's globally installed CLI, so CI can pass or fail based on the wrong command surface. This should execute the current repo entry point from ROOT; otherwise the lint result is not tied to this PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/skills-lint.ts` around lines 35 - 43, The getGenieCommands function
currently shells out to whatever "genie" is on PATH; change it to run the repo's
CLI entry point instead (resolve the package's bin from the repo ROOT and
execute that file with "--help"), e.g. compute the repo root (or use the
existing ROOT constant), resolve the package "bin" entry or join ROOT with the
local CLI file, and call execFileSync/execSync with the resolved file and
["--help"] (avoid shell redirections and plain "genie --help"); update
getGenieCommands to use that resolved path so CI probes the repo's CLI rather
than a global binary.
Comment on lines
+27
to
+30
| afterEach(() => { | ||
| if (origHome === undefined) process.env.GENIE_HOME = undefined; | ||
| else process.env.GENIE_HOME = origHome; | ||
| rmSync(tmpDir, { recursive: true, force: true }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
bun -e "delete process.env.GENIE_HOME; console.log('after delete:', process.env.GENIE_HOME); process.env.GENIE_HOME = undefined; console.log('after assign:', JSON.stringify(process.env.GENIE_HOME))"Repository: automagik-dev/genie
Length of output: 106
🏁 Script executed:
# First, check if the test file exists and read the relevant section
fd bridge-status.test.tsRepository: automagik-dev/genie
Length of output: 92
🏁 Script executed:
# Test the process.env behavior with Node.js (since bun is not available)
node -e "delete process.env.TEST_VAR; console.log('after delete:', process.env.TEST_VAR); process.env.TEST_VAR = undefined; console.log('after assign undefined:', process.env.TEST_VAR); console.log('type:', typeof process.env.TEST_VAR); console.log('JSON.stringify:', JSON.stringify(process.env.TEST_VAR));"Repository: automagik-dev/genie
Length of output: 161
🏁 Script executed:
# Read the test file to see the current state
cat -n src/lib/bridge-status.test.ts | head -40Repository: automagik-dev/genie
Length of output: 1649
🏁 Script executed:
# Confirm the process.env behavior more explicitly - check if the variable actually gets deleted vs remains
node -e "
process.env.GENIE_HOME = '/some/path';
console.log('1. Set to path:', process.env.GENIE_HOME);
delete process.env.GENIE_HOME;
console.log('2. After delete:', process.env.GENIE_HOME);
console.log('3. Variable exists?', 'GENIE_HOME' in process.env);
process.env.GENIE_HOME = undefined;
console.log('4. After assign undefined:', process.env.GENIE_HOME);
console.log('5. Variable exists?', 'GENIE_HOME' in process.env);
console.log('6. String comparison:', process.env.GENIE_HOME === 'undefined');
"Repository: automagik-dev/genie
Length of output: 231
Use delete instead of assigning undefined to restore unset GENIE_HOME.
Assigning process.env.GENIE_HOME = undefined sets it to the string "undefined" rather than removing the variable, leaking test state into subsequent tests. Use delete process.env.GENIE_HOME to properly restore the unset state.
Patch
afterEach(() => {
- if (origHome === undefined) process.env.GENIE_HOME = undefined;
+ if (origHome === undefined) delete process.env.GENIE_HOME;
else process.env.GENIE_HOME = origHome;
rmSync(tmpDir, { recursive: true, force: true });
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| afterEach(() => { | |
| if (origHome === undefined) process.env.GENIE_HOME = undefined; | |
| else process.env.GENIE_HOME = origHome; | |
| rmSync(tmpDir, { recursive: true, force: true }); | |
| afterEach(() => { | |
| if (origHome === undefined) delete process.env.GENIE_HOME; | |
| else process.env.GENIE_HOME = origHome; | |
| rmSync(tmpDir, { recursive: true, force: true }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/bridge-status.test.ts` around lines 27 - 30, The afterEach cleanup in
bridge-status.test.ts currently restores GENIE_HOME by assigning undefined which
sets the string "undefined"; instead, when origHome is undefined remove the
environment variable using delete process.env.GENIE_HOME, otherwise restore the
original value; update the afterEach block that references origHome,
process.env.GENIE_HOME and tmpDir (and keeps the rmSync(tmpDir, { recursive:
true, force: true }) call) so unset state is properly restored.
Comment on lines
+136
to
+143
| const pong = JSON.parse(sc.decode(msg.data)) as BridgePong; | ||
| return { | ||
| state: 'running', | ||
| detail: `pong from pid ${pong.pid}`, | ||
| pidfile, | ||
| pong, | ||
| latencyMs: Date.now() - t0, | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider validating the pong response shape.
Unlike readBridgePidfile which validates the parsed JSON structure, the pong response uses a bare type assertion. If the bridge ever returns malformed data, pong.pid could be undefined, producing a confusing detail message.
Low risk since this is internal IPC, but adding validation would be consistent with the defensive approach elsewhere in this file.
🔧 Optional: Add pong validation
const msg = await nc.request(PING_SUBJECT, sc.encode('{}'), { timeout: timeoutMs });
- const pong = JSON.parse(sc.decode(msg.data)) as BridgePong;
+ const parsed = JSON.parse(sc.decode(msg.data));
+ if (
+ typeof parsed?.ok !== 'boolean' ||
+ typeof parsed?.pid !== 'number' ||
+ typeof parsed?.uptimeMs !== 'number'
+ ) {
+ return {
+ state: 'stale',
+ detail: 'invalid pong response',
+ pidfile,
+ };
+ }
+ const pong = parsed as BridgePong;
return {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/bridge-status.ts` around lines 136 - 143, The pong parse uses a bare
type assertion which can yield undefined fields; mirror the defensive parsing
used in readBridgePidfile by validating the decoded JSON (sc.decode(msg.data))
against the BridgePong shape before using pong.pid: check that the parsed object
has a numeric pid (and any other required fields), and if validation fails,
handle it gracefully (e.g., return a non-running status or include an explicit
error detail) rather than interpolating possibly undefined pong.pid into the
detail string.
Comment on lines
+663
to
680
| * Clean up stale sessions from previous bridge runs. | ||
| * | ||
| * Queries PG for active sessions, checks whether their tmux panes are still | ||
| * alive, and re-populates the in-memory sessions Map for live ones. Dead | ||
| * panes and SDK sessions (which can't survive a process restart) are marked | ||
| * orphaned. This runs once during start(), after sessionStore is initialized. | ||
| * Orphans ALL active sessions on startup. Live panes will be re-created | ||
| * on demand when the next message arrives. This prevents duplicate key | ||
| * errors and misrouting from stale session state. | ||
| */ | ||
| private async recoverSessions(): Promise<void> { | ||
| if (!this.sessionStore) return; | ||
|
|
||
| const activeSessions = await this.safePgCall( | ||
| 'recover_list_active', | ||
| (sql) => new BridgeSessionStore(sql).list('active'), | ||
| [], | ||
| const orphanedCount = await this.safePgCall( | ||
| 'recover_orphan_all', | ||
| (sql) => new BridgeSessionStore(sql).markAllOrphaned(), | ||
| 0, | ||
| ); | ||
|
|
||
| if (activeSessions.length === 0) return; | ||
|
|
||
| console.log(`[omni-bridge] Found ${activeSessions.length} active session(s) from previous run, recovering...`); | ||
|
|
||
| const orphanIds: string[] = []; | ||
|
|
||
| for (const row of activeSessions) { | ||
| const key = `${row.agent_name}:${row.chat_id}`; | ||
|
|
||
| // SDK sessions can't survive a process restart — always orphan them. | ||
| if (!row.tmux_pane_id) { | ||
| orphanIds.push(row.id); | ||
| continue; | ||
| } | ||
|
|
||
| // Duplicate guard: if another row already recovered this chat, orphan the older one. | ||
| if (this.sessions.has(key)) { | ||
| orphanIds.push(row.id); | ||
| continue; | ||
| } | ||
|
|
||
| // Check if the tmux pane is still alive. | ||
| let alive = false; | ||
| try { | ||
| alive = await isPaneAlive(row.tmux_pane_id); | ||
| } catch { | ||
| // tmux not available or pane check failed — treat as dead. | ||
| } | ||
|
|
||
| if (!alive) { | ||
| orphanIds.push(row.id); | ||
| continue; | ||
| } | ||
|
|
||
| // Pane is alive — reconstruct the in-memory session entry. | ||
| const session: ExecutorSession = { | ||
| id: row.executor_id ?? row.id, | ||
| agentName: row.agent_name, | ||
| chatId: row.chat_id, | ||
| executorType: 'tmux', | ||
| createdAt: new Date(row.started_at).getTime(), | ||
| lastActivityAt: new Date(row.last_activity_at).getTime(), | ||
| tmux: { session: '', window: '', paneId: row.tmux_pane_id }, | ||
| }; | ||
|
|
||
| const entry: SessionEntry = { | ||
| session, | ||
| instanceId: row.instance_id, | ||
| spawning: false, | ||
| buffer: [], | ||
| idleTimer: null, | ||
| pgBridgeSessionId: row.id, | ||
| }; | ||
|
|
||
| this.sessions.set(key, entry); | ||
| this.resetIdleTimer(key); | ||
|
|
||
| console.log(`[omni-bridge] Recovered session ${key} (pane=${row.tmux_pane_id})`); | ||
| } | ||
|
|
||
| // Mark dead/SDK sessions as orphaned in one batch. | ||
| if (orphanIds.length > 0) { | ||
| await this.safePgCall( | ||
| 'recover_mark_orphaned', | ||
| (sql) => new BridgeSessionStore(sql).markOrphaned(orphanIds), | ||
| undefined, | ||
| ); | ||
| console.log(`[omni-bridge] Marked ${orphanIds.length} stale session(s) as orphaned`); | ||
| } | ||
|
|
||
| if (this.sessions.size > 0) { | ||
| console.log(`[omni-bridge] Recovered ${this.sessions.size} live session(s)`); | ||
| if (orphanedCount > 0) { | ||
| console.log(`[omni-bridge] Startup cleanup: orphaned ${orphanedCount} stale session(s) from previous run`); | ||
| } |
There was a problem hiding this comment.
This recovery path abandons surviving tmux sessions on restart.
stop() intentionally leaves tmux sessions alive, but startup now only marks every active bridge row orphaned. The next message will spawn a brand-new session instead of reattaching, which loses continuity and leaks the old pane. Either rehydrate live sessions here or explicitly tear them down during shutdown.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/omni-bridge.ts` around lines 663 - 680, recoverSessions()
currently calls BridgeSessionStore.markAllOrphaned(), which abandons surviving
tmux panes; change this so startup either rehydrates live sessions or ensure
shutdown tears them down: replace the single markAllOrphaned() call with logic
that queries the active session rows from BridgeSessionStore (e.g., a
getActive/getAllActive method) and for each row attempt to reattach/rehydrate
the bridge runtime using the existing tmux/bridge attach routine (or, if
reattach fails, mark that specific row orphaned), or alternatively add explicit
teardown in stop() to kill remaining tmux panes so recoverSessions() can safely
markAllOrphaned; update uses of recoverSessions(),
BridgeSessionStore.markAllOrphaned, and stop() accordingly.
Comment on lines
+750
to
+766
| // Dedup: skip messages already processed (JetStream redelivery protection) | ||
| // biome-ignore lint/suspicious/noExplicitAny: NATS payload has extra fields beyond OmniMessage | ||
| const messageId = (parsed as any).messageId as string | undefined; | ||
| if (messageId && this.recentMessageIds.has(messageId)) { | ||
| console.log(`[omni-bridge] Dedup: skipping duplicate messageId=${messageId}`); | ||
| continue; | ||
| } | ||
| if (messageId) { | ||
| this.recentMessageIds.set(messageId, Date.now()); | ||
| // Prune stale entries when cache grows large (TTL 60s) | ||
| if (this.recentMessageIds.size > 1000) { | ||
| const cutoff = Date.now() - 60_000; | ||
| for (const [id, ts] of this.recentMessageIds) { | ||
| if (ts < cutoff) this.recentMessageIds.delete(id); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Only mark a message as deduped after it has been handled successfully.
messageId is recorded before validation and before enqueue() / routeMessage() finishes. If processing throws, a redelivery of the same message is now treated as a duplicate and dropped permanently. Move the insert to the success path, or remove it again when handling fails.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/omni-bridge.ts` around lines 750 - 766, The current dedup logic
records messageId into this.recentMessageIds before validation and before
calling enqueue()/routeMessage(), which can cause permanently dropped messages
if processing later throws; remove the early
this.recentMessageIds.set(messageId, ...) and instead add the messageId to
this.recentMessageIds only after the message has been processed successfully
(i.e., after enqueue() / routeMessage() completes), or alternatively wrap
processing in a try/catch and ensure you delete the messageId from
this.recentMessageIds on any failure; keep the existing pruning logic for
recentMessageIds and continue to check for duplicates with the existing
messageId variable.
Comment on lines
+584
to
+590
| try { | ||
| await bridge.start(); | ||
| } catch (err) { | ||
| const msg = err instanceof Error ? err.message : String(err); | ||
| console.error(` Omni bridge: FAILED — ${msg}`); | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
Fail through the shared shutdown path instead of exiting here.
By this point serve.pid is already written and pgserve/scheduler/TUI/approval startup may already have happened. process.exit(1) skips teardown, so a bridge startup failure can leave orphaned services and a stale PID file behind. Throw here and let a common cleanup path unwind, or explicitly run the same shutdown logic before exiting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/term-commands/serve.ts` around lines 584 - 590, The bridge startup catch
block currently calls process.exit(1) after logging, which bypasses the shared
shutdown/cleanup (and leaves serve.pid and orphaned services); instead, keep the
existing console.error but do not call process.exit — either rethrow the caught
error (throw err) so the common cleanup path can run, or invoke the project's
central shutdown/cleanup function (the same path used elsewhere) before exiting;
update the catch in serve.ts around bridge.start() to log the error and then
propagate it via throw err (or call the central shutdown function) rather than
calling process.exit(1).
feat(bridge): IPC pidfile + NATS ping, skills audit lint
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (3)
src/services/omni-bridge.ts (2)
734-749:⚠️ Potential issue | 🔴 CriticalRecord
messageIdonly after successful handling.If validation,
enqueue(), orrouteMessage()throws after Line 742, the redelivery hitsrecentMessageIdsand gets dropped permanently. Add the dedup entry only after the message is processed, or remove it again on failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/omni-bridge.ts` around lines 734 - 749, The code currently writes parsed messageId into recentMessageIds before processing, which causes permanent drops if validation/enqueue()/routeMessage() later throw; change the flow so recentMessageIds.set(messageId, Date.now()) is performed only after the message has been successfully handled (i.e., after validation, enqueue(), and routeMessage() complete), or alternatively wrap the processing logic (validation/enqueue()/routeMessage()) in a try/catch and remove the entry via recentMessageIds.delete(messageId) on any failure; update references to parsed/messageId, recentMessageIds, enqueue(), and routeMessage() accordingly to ensure dedup only marks successfully-processed messages.
656-664:⚠️ Potential issue | 🟠 MajorStartup recovery still abandons surviving tmux sessions.
stop()intentionally leaves tmux panes alive, but this blanketmarkAllOrphaned()severs every active row without reattaching or tearing those panes down. The next message will spawn duplicates and leak the old session.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/omni-bridge.ts` around lines 656 - 664, The startup cleanup currently calls BridgeSessionStore.markAllOrphaned() via safePgCall which indiscriminately marks every session orphaned and causes live tmux panes to be abandoned; instead, replace this single markAllOrphaned call with logic that loads active sessions from BridgeSessionStore, iterates them and for each session uses the tmux-check helper (or add a TmuxManager.isPaneAlive / checkTmuxPaneAlive utility) to verify the corresponding tmux pane/process is gone, and only call markOrphaned (or update that specific row) for sessions whose tmux panes are truly dead; ensure this preserves the existing stop() behavior of leaving panes alive and prevents duplicate/reparented sessions on startup.src/term-commands/serve.ts (1)
583-588:⚠️ Potential issue | 🟠 MajorBridge startup failure still bypasses teardown.
By Line 583,
serve.pid, pgserve, scheduler, and possibly the TUI are already up. Exiting here leaves orphaned services and a stale PID file instead of unwinding through a cleanup path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/term-commands/serve.ts` around lines 583 - 588, The catch in the bridge.start() block logs the error and calls process.exit(1), which bypasses the existing teardown path and leaves serve.pid, pgserve, scheduler, and the TUI orphaned; instead, remove the direct process.exit(1) and ensure the error is forwarded to the central cleanup logic (either by rethrowing the caught error or by awaiting the existing teardown/shutdown function) so the global cleanup/finally path runs and removes the PID file and stops pgserve/scheduler/TUI before exiting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.genie/wishes/unify-bridge-revamp-skills/WISH.md:
- Around line 5-8: The file has a contradictory Status: the top header "Status"
is set to DRAFT while the review section marks the wish COMPLETE; update the
single source of truth by choosing the correct lifecycle state and making both
places consistent (e.g., change the header "Status" value to COMPLETE if the
review declares it ready to ship, or change the review section to DRAFT if it’s
still in-flight), and also fix the duplicate/related occurrence referenced (the
other instance around the same block) so all "Status" mentions and the review
section reflect the same state.
In `@package.json`:
- Around line 25-27: Add enforcement for the existing npm scripts "skills:lint"
and "skills:audit" by invoking them from the places that gate merges: update the
pre-push hook (referenced as .husky/pre-push which currently runs the "check"
script) to also run "bun run skills:lint" and "bun run skills:audit" (or add
them into the "check" script so "check" runs them), and modify the CI
quality-gate job in .github/workflows/ci.yml (which currently runs typecheck,
lint, dead-code, tests) to include execution of "bun run skills:lint" and "bun
run skills:audit" so skill validation runs both locally before push and in CI.
In `@src/genie-commands/doctor.ts`:
- Around line 341-351: The code in the getBridgeStatus handling currently calls
removeBridgePidfile() for every 'stale' state (in the if block around res.state
=== 'stale'), which deletes a live pidfile when the bridge is reachable but
NATS/ping failed; change the logic in that branch so removeBridgePidfile() is
only invoked when the res.detail explicitly indicates the process PID is not
running (e.g., contains or matches "pid ... not running" or similar) and not
when the detail is a ping/NATS failure, and also update the results.push
suggestion text from "genie serve restart" to the correct command "genie serve
stop && genie serve" so the Bridge result message points to the registered
subcommand; keep these changes in the same function handling res.state ===
'stale' (where removeBridgePidfile and results.push are used).
In `@src/lib/bridge-status.ts`:
- Around line 122-143: The current t0 timer is started before connectFn(), so
latencyMs on the returned object measures connection + ping; to measure pure
ping RTT move the t0 assignment (remove the earlier Date.now() at variable t0)
so it is set immediately after the successful connectFn() call and just before
nc.request(...), then compute latencyMs = Date.now() - t0; alternatively, if you
want to keep measuring connection+ping, rename latencyMs to totalLatencyMs or
add a clarifying comment near the t0/latencyMs variables; references: t0,
connectFn, nc.request, and latencyMs.
In `@src/services/omni-bridge.ts`:
- Around line 343-411: The catch for pidfile write failure doesn't undo all
started components — ensure start()'s rollback also stops/cleans this.queue,
this.sub, and the idleCheckTimer so no background work remains: unsubscribe and
null out this.sub, stop or close and null this.queue (call its shutdown/stop
method), clearInterval on idleCheckTimer and set it to null, and perform any
other per-start cleanup (in addition to the existing pingSub unsubscribe and
nc.drain) before throwing; reference the start() flow and the symbols
this.queue, this.sub, idleCheckTimer, pingSub, and nc when adding these cleanup
steps.
- Around line 327-331: The BridgePong metadata currently lists only
'omni.turn.open.>' among turn-related subjects, but the bridge also subscribes
to 'omni.turn.done.>', 'omni.turn.nudge.>', and 'omni.turn.timeout.>'—update the
BridgePong.subjects arrays to include those three missing subjects wherever the
pong/pidfile is built (e.g., the BridgePong construction that defines const pong
and the other similar pong construction later in the file), ensuring the
subjects list accurately advertises omni.message.>, omni.turn.open.>,
omni.turn.done.>, omni.turn.nudge.>, omni.turn.timeout.>, and
BRIDGE_PING_SUBJECT.
---
Duplicate comments:
In `@src/services/omni-bridge.ts`:
- Around line 734-749: The code currently writes parsed messageId into
recentMessageIds before processing, which causes permanent drops if
validation/enqueue()/routeMessage() later throw; change the flow so
recentMessageIds.set(messageId, Date.now()) is performed only after the message
has been successfully handled (i.e., after validation, enqueue(), and
routeMessage() complete), or alternatively wrap the processing logic
(validation/enqueue()/routeMessage()) in a try/catch and remove the entry via
recentMessageIds.delete(messageId) on any failure; update references to
parsed/messageId, recentMessageIds, enqueue(), and routeMessage() accordingly to
ensure dedup only marks successfully-processed messages.
- Around line 656-664: The startup cleanup currently calls
BridgeSessionStore.markAllOrphaned() via safePgCall which indiscriminately marks
every session orphaned and causes live tmux panes to be abandoned; instead,
replace this single markAllOrphaned call with logic that loads active sessions
from BridgeSessionStore, iterates them and for each session uses the tmux-check
helper (or add a TmuxManager.isPaneAlive / checkTmuxPaneAlive utility) to verify
the corresponding tmux pane/process is gone, and only call markOrphaned (or
update that specific row) for sessions whose tmux panes are truly dead; ensure
this preserves the existing stop() behavior of leaving panes alive and prevents
duplicate/reparented sessions on startup.
In `@src/term-commands/serve.ts`:
- Around line 583-588: The catch in the bridge.start() block logs the error and
calls process.exit(1), which bypasses the existing teardown path and leaves
serve.pid, pgserve, scheduler, and the TUI orphaned; instead, remove the direct
process.exit(1) and ensure the error is forwarded to the central cleanup logic
(either by rethrowing the caught error or by awaiting the existing
teardown/shutdown function) so the global cleanup/finally path runs and removes
the PID file and stops pgserve/scheduler/TUI before exiting.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0cb21f48-109d-4178-bef2-c4f7ef9a05af
⛔ Files ignored due to path filters (1)
CHANGELOG.mdis excluded by!*.md
📒 Files selected for processing (11)
.claude-plugin/marketplace.json.genie/wishes/unify-bridge-revamp-skills/WISH.mdpackage.jsonplugins/genie/.claude-plugin/plugin.jsonplugins/genie/package.jsonsrc/genie-commands/doctor.tssrc/genie.tssrc/lib/bridge-status.tssrc/services/omni-bridge.tssrc/term-commands/omni.tssrc/term-commands/serve.ts
💤 Files with no reviewable changes (2)
- src/genie.ts
- src/term-commands/omni.ts
Comment on lines
+5
to
+8
| | **Status** | DRAFT | | ||
| | **Slug** | `unify-bridge-revamp-skills` | | ||
| | **Date** | 2026-04-09 | | ||
| | **Design** | N/A — direct from debug session | |
There was a problem hiding this comment.
Status is self-contradictory.
The header still says DRAFT, but the review section says the wish is COMPLETE and ready to ship. Pick one source of truth here, otherwise downstream readers cannot tell whether this spec is still in-flight.
Also applies to: 247-247
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.genie/wishes/unify-bridge-revamp-skills/WISH.md around lines 5 - 8, The
file has a contradictory Status: the top header "Status" is set to DRAFT while
the review section marks the wish COMPLETE; update the single source of truth by
choosing the correct lifecycle state and making both places consistent (e.g.,
change the header "Status" value to COMPLETE if the review declares it ready to
ship, or change the review section to DRAFT if it’s still in-flight), and also
fix the duplicate/related occurrence referenced (the other instance around the
same block) so all "Status" mentions and the review section reflect the same
state.
Comment on lines
25
to
27
| "skills:lint": "bun run scripts/skills-lint.ts", | ||
| "skills:audit": "bun run scripts/skills-audit.ts", | ||
| "check": "bun run typecheck && bun run lint && bun run dead-code && (bun test || bun test)" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Expected: at least one gated command or workflow invokes skills:lint and skills:audit.
# If the only hits are this package.json entry or docs, the new checks are not enforced.
rg -n --hidden -S 'skills:(lint|audit)|bun run check' .Repository: automagik-dev/genie
Length of output: 18374
🏁 Script executed:
cat -n .husky/pre-pushRepository: automagik-dev/genie
Length of output: 443
🏁 Script executed:
fd -type f \( -name ".github" -o -name ".gitlab-ci.yml" -o -name "Makefile" -o -name "*.yml" -o -name "*.yaml" \) | grep -E "(github|gitlab|workflow)" | head -20Repository: automagik-dev/genie
Length of output: 233
🏁 Script executed:
ls -la scripts/skills-*.ts 2>/dev/null || echo "Scripts not found"Repository: automagik-dev/genie
Length of output: 207
🏁 Script executed:
find .github -type f -name "*.yml" -o -name "*.yaml" 2>/dev/null | head -20Repository: automagik-dev/genie
Length of output: 213
🏁 Script executed:
rg -n 'skills:lint|skills:audit' --type yaml --type yml 2>/dev/null || rg -n 'skills:lint|skills:audit' 2>/dev/null | grep -E '\.(yml|yaml|sh|md):' | head -20Repository: automagik-dev/genie
Length of output: 45
🏁 Script executed:
cat -n scripts/skills-lint.ts | head -30Repository: automagik-dev/genie
Length of output: 1357
🏁 Script executed:
cat -n .github/workflows/ci.ymlRepository: automagik-dev/genie
Length of output: 4309
skills:lint and skills:audit scripts are defined but not enforced.
These are intended as guardrails for stale skill command references and scope creep, but they are wired into neither the pre-push hook (.husky/pre-push runs only bun run check) nor CI (.github/workflows/ci.yml quality-gate job runs typecheck, lint, dead-code, and tests—no skills validation). Broken skill examples can merge green.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` around lines 25 - 27, Add enforcement for the existing npm
scripts "skills:lint" and "skills:audit" by invoking them from the places that
gate merges: update the pre-push hook (referenced as .husky/pre-push which
currently runs the "check" script) to also run "bun run skills:lint" and "bun
run skills:audit" (or add them into the "check" script so "check" runs them),
and modify the CI quality-gate job in .github/workflows/ci.yml (which currently
runs typecheck, lint, dead-code, tests) to include execution of "bun run
skills:lint" and "bun run skills:audit" so skill validation runs both locally
before push and in CI.
Comment on lines
+341
to
+351
| if (res.state === 'stale') { | ||
| // Stale pidfile = owning process is dead or ping timed out. Clean up | ||
| // the pidfile so the next `genie serve` start can retake cleanly. | ||
| if (res.pidfile) { | ||
| removeBridgePidfile(); | ||
| } | ||
| results.push({ | ||
| name: 'Bridge', | ||
| status: 'fail', | ||
| message: `stale: ${res.detail}`, | ||
| suggestion: 'Restart the bridge with: genie serve restart', |
There was a problem hiding this comment.
Don't let genie doctor delete a live bridge pidfile.
getBridgeStatus() returns stale for both pid ... not running and ping failed: .... This branch unlinks the pidfile in both cases, so a transient NATS failure turns a live bridge into “stopped” on the next probe. Also, genie serve restart is not a registered subcommand in src/term-commands/serve.ts; the suggestion should point to genie serve stop && genie serve.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/genie-commands/doctor.ts` around lines 341 - 351, The code in the
getBridgeStatus handling currently calls removeBridgePidfile() for every 'stale'
state (in the if block around res.state === 'stale'), which deletes a live
pidfile when the bridge is reachable but NATS/ping failed; change the logic in
that branch so removeBridgePidfile() is only invoked when the res.detail
explicitly indicates the process PID is not running (e.g., contains or matches
"pid ... not running" or similar) and not when the detail is a ping/NATS
failure, and also update the results.push suggestion text from "genie serve
restart" to the correct command "genie serve stop && genie serve" so the Bridge
result message points to the registered subcommand; keep these changes in the
same function handling res.state === 'stale' (where removeBridgePidfile and
results.push are used).
Comment on lines
+122
to
+143
| const timeoutMs = options.timeoutMs ?? PING_TIMEOUT_MS; | ||
| const connectFn = options.natsConnectFn ?? connect; | ||
| const sc = StringCodec(); | ||
| const t0 = Date.now(); | ||
|
|
||
| let nc: Awaited<ReturnType<typeof connect>> | null = null; | ||
| try { | ||
| nc = await connectFn({ | ||
| servers: pidfile.natsUrl || DEFAULT_NATS_URL, | ||
| name: 'genie-bridge-status', | ||
| reconnect: false, | ||
| timeout: timeoutMs, | ||
| }); | ||
| const msg = await nc.request(PING_SUBJECT, sc.encode('{}'), { timeout: timeoutMs }); | ||
| const pong = JSON.parse(sc.decode(msg.data)) as BridgePong; | ||
| return { | ||
| state: 'running', | ||
| detail: `pong from pid ${pong.pid}`, | ||
| pidfile, | ||
| pong, | ||
| latencyMs: Date.now() - t0, | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
latencyMs includes connection establishment time.
The timer starts at line 125 before connectFn(), so latencyMs measures total round-trip including NATS connection setup, not just the ping RTT. If this is intentional for "total health check latency", consider renaming to totalLatencyMs or adding a brief comment. If you want pure ping latency, move t0 after the connection is established.
🔧 Option: measure pure ping latency
let nc: Awaited<ReturnType<typeof connect>> | null = null;
try {
nc = await connectFn({
servers: pidfile.natsUrl || DEFAULT_NATS_URL,
name: 'genie-bridge-status',
reconnect: false,
timeout: timeoutMs,
});
+ const t0 = Date.now();
const msg = await nc.request(PING_SUBJECT, sc.encode('{}'), { timeout: timeoutMs });
const pong = JSON.parse(sc.decode(msg.data)) as BridgePong;
return {
state: 'running',
detail: `pong from pid ${pong.pid}`,
pidfile,
pong,
latencyMs: Date.now() - t0,
};And remove line 125.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const timeoutMs = options.timeoutMs ?? PING_TIMEOUT_MS; | |
| const connectFn = options.natsConnectFn ?? connect; | |
| const sc = StringCodec(); | |
| const t0 = Date.now(); | |
| let nc: Awaited<ReturnType<typeof connect>> | null = null; | |
| try { | |
| nc = await connectFn({ | |
| servers: pidfile.natsUrl || DEFAULT_NATS_URL, | |
| name: 'genie-bridge-status', | |
| reconnect: false, | |
| timeout: timeoutMs, | |
| }); | |
| const msg = await nc.request(PING_SUBJECT, sc.encode('{}'), { timeout: timeoutMs }); | |
| const pong = JSON.parse(sc.decode(msg.data)) as BridgePong; | |
| return { | |
| state: 'running', | |
| detail: `pong from pid ${pong.pid}`, | |
| pidfile, | |
| pong, | |
| latencyMs: Date.now() - t0, | |
| }; | |
| const timeoutMs = options.timeoutMs ?? PING_TIMEOUT_MS; | |
| const connectFn = options.natsConnectFn ?? connect; | |
| const sc = StringCodec(); | |
| let nc: Awaited<ReturnType<typeof connect>> | null = null; | |
| try { | |
| nc = await connectFn({ | |
| servers: pidfile.natsUrl || DEFAULT_NATS_URL, | |
| name: 'genie-bridge-status', | |
| reconnect: false, | |
| timeout: timeoutMs, | |
| }); | |
| const t0 = Date.now(); | |
| const msg = await nc.request(PING_SUBJECT, sc.encode('{}'), { timeout: timeoutMs }); | |
| const pong = JSON.parse(sc.decode(msg.data)) as BridgePong; | |
| return { | |
| state: 'running', | |
| detail: `pong from pid ${pong.pid}`, | |
| pidfile, | |
| pong, | |
| latencyMs: Date.now() - t0, | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/bridge-status.ts` around lines 122 - 143, The current t0 timer is
started before connectFn(), so latencyMs on the returned object measures
connection + ping; to measure pure ping RTT move the t0 assignment (remove the
earlier Date.now() at variable t0) so it is set immediately after the successful
connectFn() call and just before nc.request(...), then compute latencyMs =
Date.now() - t0; alternatively, if you want to keep measuring connection+ping,
rename latencyMs to totalLatencyMs or add a clarifying comment near the
t0/latencyMs variables; references: t0, connectFn, nc.request, and latencyMs.
Comment on lines
+327
to
+331
| const pong: BridgePong = { | ||
| ok: true, | ||
| pid: process.pid, | ||
| uptimeMs: Date.now() - pingStart, | ||
| subjects: ['omni.message.>', 'omni.turn.open.>', 'omni.session.reset.>', BRIDGE_PING_SUBJECT], |
There was a problem hiding this comment.
Ping metadata omits three active turn subjects.
The bridge subscribes to omni.turn.open.>, omni.turn.done.>, omni.turn.nudge.>, and omni.turn.timeout.>, but the pong/pidfile only advertise omni.turn.open.>. genie doctor will under-report the bridge’s subject surface.
Also applies to: 386-390
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/omni-bridge.ts` around lines 327 - 331, The BridgePong metadata
currently lists only 'omni.turn.open.>' among turn-related subjects, but the
bridge also subscribes to 'omni.turn.done.>', 'omni.turn.nudge.>', and
'omni.turn.timeout.>'—update the BridgePong.subjects arrays to include those
three missing subjects wherever the pong/pidfile is built (e.g., the BridgePong
construction that defines const pong and the other similar pong construction
later in the file), ensuring the subjects list accurately advertises
omni.message.>, omni.turn.open.>, omni.turn.done.>, omni.turn.nudge.>,
omni.turn.timeout.>, and BRIDGE_PING_SUBJECT.
Comment on lines
+343
to
+411
| // Write the pidfile AFTER NATS is live and the ping handler is armed so | ||
| // any reader who sees the file can trust the IPC channel to answer. | ||
| // Uses O_EXCL so two concurrent `genie serve` starts cannot both claim it. | ||
| this.pidfilePath = getBridgePidfilePath(); | ||
| try { | ||
| mkdirSync(dirname(this.pidfilePath), { recursive: true }); | ||
| // Stale-pidfile recovery: if a pidfile exists whose owning process is | ||
| // gone, the previous holder crashed. Unlink it and retake. If the PID | ||
| // is alive, fail fast — another bridge is legitimately holding the lock. | ||
| if (existsSync(this.pidfilePath)) { | ||
| let stalePid: number | null = null; | ||
| try { | ||
| const raw = readFileSync(this.pidfilePath, 'utf8'); | ||
| const parsed = JSON.parse(raw) as { pid?: unknown }; | ||
| if (typeof parsed.pid === 'number' && Number.isFinite(parsed.pid)) { | ||
| stalePid = parsed.pid; | ||
| } | ||
| } catch { | ||
| // Unreadable/corrupt pidfile — treat as stale and unlink below. | ||
| } | ||
| if (stalePid !== null) { | ||
| let alive = false; | ||
| try { | ||
| process.kill(stalePid, 0); | ||
| alive = true; | ||
| } catch (probeErr) { | ||
| // ESRCH = no such process → stale. Any other errno (e.g. EPERM) | ||
| // means the process exists and we must not steal the lock. | ||
| const code = (probeErr as NodeJS.ErrnoException).code; | ||
| alive = code !== 'ESRCH'; | ||
| } | ||
| if (alive) { | ||
| throw new Error(`pidfile locked by PID ${stalePid}`); | ||
| } | ||
| } | ||
| try { | ||
| unlinkSync(this.pidfilePath); | ||
| } catch { | ||
| // Another process may have cleaned it up already — openSync wx below | ||
| // will still arbitrate the race atomically. | ||
| } | ||
| } | ||
| const fd = openSync(this.pidfilePath, 'wx'); | ||
| const payload = JSON.stringify({ | ||
| pid: process.pid, | ||
| startedAt: this.startedAtMs, | ||
| subjects: ['omni.message.>', 'omni.turn.open.>', 'omni.session.reset.>', BRIDGE_PING_SUBJECT], | ||
| natsUrl: this.natsUrl, | ||
| }); | ||
| writeSync(fd, payload); | ||
| closeSync(fd); | ||
| } catch (err) { | ||
| // Roll back the NATS connection so a locked pidfile causes a clean, | ||
| // non-zero exit in serve instead of a half-initialized bridge. | ||
| this.pidfilePath = null; | ||
| const detail = err instanceof Error ? err.message : String(err); | ||
| try { | ||
| if (this.pingSub) this.pingSub.unsubscribe(); | ||
| } catch { | ||
| // ignore | ||
| } | ||
| this.pingSub = null; | ||
| try { | ||
| await this.nc?.drain(); | ||
| } catch { | ||
| // ignore | ||
| } | ||
| this.nc = null; | ||
| throw new Error(`[omni-bridge] pidfile locked at ${getBridgePidfilePath()}: ${detail}`); |
There was a problem hiding this comment.
Pidfile-lock failure doesn't fully roll back startup.
By the time this catch runs, the queue worker, main subscriptions, and idleCheckTimer are already live. Draining NATS is not enough: this.queue, this.sub, and the interval stay initialized on a failed start(), which can leak background work and hang lock-contention tests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/omni-bridge.ts` around lines 343 - 411, The catch for pidfile
write failure doesn't undo all started components — ensure start()'s rollback
also stops/cleans this.queue, this.sub, and the idleCheckTimer so no background
work remains: unsubscribe and null out this.sub, stop or close and null
this.queue (call its shutdown/stop method), clearInterval on idleCheckTimer and
set it to null, and perform any other per-start cleanup (in addition to the
existing pingSub unsubscribe and nc.drain) before throwing; reference the
start() flow and the symbols this.queue, this.sub, idleCheckTimer, pingSub, and
nc when adding these cleanup steps.
When `genie init agent <name>` scaffolds a new agent, the brain/memory/
directory was created but the auto-memory config was never wired:
- `.claude/settings.local.json` only had `{ agentName: name }`, missing
`autoMemoryEnabled` and `autoMemoryDirectory` keys.
- `brain/memory/MEMORY.md` index was never seeded (empty directory).
This commit wires both during scaffold:
1. settings.local.json now writes `autoMemoryEnabled: true` and
`autoMemoryDirectory: './brain/memory'` alongside `agentName`.
2. A minimal `brain/memory/MEMORY.md` index is seeded on scaffold.
3. Console output advertises the new files.
4. `scaffoldAgentInWorkspace` is exported so it can be unit tested.
5. New test case in init.test.ts exercises the scaffold and asserts
both files land with the expected content.
Note: autoMemoryEnabled is a claude-agent-sdk setting — stock Claude
Code CLI ignores it. Seeding is harmless for stock CLI users and
functional for SDK users.
Closes #1106
`genie brain --help` only listed install/uninstall/upgrade/version,
hiding the fact that unknown subcommands transparently forward to the
`@khal-os/brain` CLI via `.allowUnknownOption().allowExcessArguments()`.
Users hit the help text, saw no search/status/health/ingest, and filed
bugs claiming the feature was missing — even though `genie brain status`
and `genie brain search "query"` already worked at runtime.
This commit:
1. Adds an `.addHelpText('after', ...)` footer listing the most commonly
used forwarded commands (status, health, init, search, ingest,
analyze, config, mount, graph, traces).
2. Shows three concrete usage examples.
3. Updates the top-level description to mention passthrough.
4. Leaves the action handler, the four real subcommands, and runtime
passthrough untouched — this is a pure help-text change.
Closes #1118
fix: auto-configure memory when scaffolding agent workspace
docs(brain): advertise passthrough commands in --help output
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
package.json (1)
25-27:⚠️ Potential issue | 🟠 MajorWire new skills checks into
checkto make them enforceable.Lines 25-26 add
skills:lint/skills:audit, but Line 27 omits both, sobun run checkcan pass without running the new validations.Suggested fix
- "check": "bun run typecheck && bun run lint && bun run dead-code && (bun test || bun test)" + "check": "bun run typecheck && bun run lint && bun run dead-code && bun run skills:lint && bun run skills:audit && (bun test || bun test)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 25 - 27, The "check" npm script currently runs "typecheck", "lint", "dead-code", and "test" but omits the new skills checks; update the "check" script to include "skills:lint" and "skills:audit" so they are executed as part of bun run check—modify the "check" value to chain or sequence "bun run skills:lint" and "bun run skills:audit" (alongside existing "bun run typecheck", "bun run lint", "bun run dead-code", and tests) ensuring the new scripts "skills:lint" and "skills:audit" are invoked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/term-commands/init.test.ts`:
- Around line 42-65: The test for scaffoldAgentInWorkspace leaks global state
because it doesn't set process.env.GENIE_HOME; update the test in init.test.ts
to explicitly set process.env.GENIE_HOME = testDir (or the temp fixture path)
before calling scaffoldAgentInWorkspace and restore the original value after the
test (or use beforeEach/afterEach) so scaffoldAgentInWorkspace,
getWorkspaceConfig, and any other code that reads GENIE_HOME operate against the
isolated temp fixture rather than shared global paths.
In `@src/term-commands/init.ts`:
- Line 46: The function scaffoldAgentInWorkspace is an internal scaffolding
helper that was exported (scaffoldAgentInWorkspace) only for tests; remove the
export so it becomes module-private and stop widening the CLI public surface.
Update any tests that import scaffoldAgentInWorkspace to exercise the behavior
via the public CLI invocation path (e.g., call the CLI entry function or the
init command runner used by the user-facing command) instead of importing this
internal helper, and adjust test setup to capture filesystem effects or
stdout/stderr as needed.
- Around line 91-92: Remove the two console.log calls in
src/term-commands/init.ts that print "brain/memory/MEMORY.md (seeded)" and
".claude/settings.local.json (auto-memory enabled)" and replace them with the
project's approved logging mechanism (e.g., use logger.info or
processLogger.info used elsewhere) or omit them entirely; ensure you call the
project's logger API rather than console.log so the change conforms to the
src/**/*.ts no-console rule and linting.
---
Duplicate comments:
In `@package.json`:
- Around line 25-27: The "check" npm script currently runs "typecheck", "lint",
"dead-code", and "test" but omits the new skills checks; update the "check"
script to include "skills:lint" and "skills:audit" so they are executed as part
of bun run check—modify the "check" value to chain or sequence "bun run
skills:lint" and "bun run skills:audit" (alongside existing "bun run typecheck",
"bun run lint", "bun run dead-code", and tests) ensuring the new scripts
"skills:lint" and "skills:audit" are invoked.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e07bee0e-5553-456a-8bec-95d0e7454d67
📒 Files selected for processing (7)
.claude-plugin/marketplace.jsonpackage.jsonplugins/genie/.claude-plugin/plugin.jsonplugins/genie/package.jsonsrc/term-commands/brain.tssrc/term-commands/init.test.tssrc/term-commands/init.ts
Comment on lines
+42
to
+65
| describe('scaffoldAgentInWorkspace', () => { | ||
| test('writes settings.local.json with auto-memory enabled and seeds MEMORY.md', () => { | ||
| // Minimal workspace.json so getWorkspaceConfig doesn't crash | ||
| mkdirSync(join(testDir, '.genie'), { recursive: true }); | ||
| writeFileSync(join(testDir, '.genie', 'workspace.json'), `${JSON.stringify({ name: 'test-ws' }, null, 2)}\n`); | ||
|
|
||
| scaffoldAgentInWorkspace(testDir, 'atlas'); | ||
|
|
||
| const settingsPath = join(testDir, 'agents', 'atlas', '.claude', 'settings.local.json'); | ||
| expect(existsSync(settingsPath)).toBe(true); | ||
| const settings = JSON.parse(readFileSync(settingsPath, 'utf-8')); | ||
| expect(settings).toEqual({ | ||
| agentName: 'atlas', | ||
| autoMemoryEnabled: true, | ||
| autoMemoryDirectory: './brain/memory', | ||
| }); | ||
|
|
||
| const memoryPath = join(testDir, 'agents', 'atlas', 'brain', 'memory', 'MEMORY.md'); | ||
| expect(existsSync(memoryPath)).toBe(true); | ||
| const memory = readFileSync(memoryPath, 'utf-8'); | ||
| expect(memory).toContain('# Memory Index'); | ||
| expect(memory).toContain('auto-memory system'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Isolate GENIE_HOME in this suite to prevent global-state leakage.
This test creates workspace-like state but never pins process.env.GENIE_HOME to the temp fixture, so it can interact with shared/global config paths.
🔧 Proposed fix
let testDir: string;
+let prevGenieHome: string | undefined;
beforeEach(() => {
testDir = join(tmpdir(), `genie-init-test-${Date.now()}-${Math.random().toString(36).slice(2)}`);
mkdirSync(testDir, { recursive: true });
+ prevGenieHome = process.env.GENIE_HOME;
+ process.env.GENIE_HOME = testDir;
});
afterEach(() => {
rmSync(testDir, { recursive: true, force: true });
+ if (prevGenieHome === undefined) delete process.env.GENIE_HOME;
+ else process.env.GENIE_HOME = prevGenieHome;
});As per coding guidelines: "Set process.env.GENIE_HOME to tmpdir in tests to isolate global state".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/term-commands/init.test.ts` around lines 42 - 65, The test for
scaffoldAgentInWorkspace leaks global state because it doesn't set
process.env.GENIE_HOME; update the test in init.test.ts to explicitly set
process.env.GENIE_HOME = testDir (or the temp fixture path) before calling
scaffoldAgentInWorkspace and restore the original value after the test (or use
beforeEach/afterEach) so scaffoldAgentInWorkspace, getWorkspaceConfig, and any
other code that reads GENIE_HOME operate against the isolated temp fixture
rather than shared global paths.
| } | ||
|
|
||
| function scaffoldAgentInWorkspace(workspaceRoot: string, name: string, agentsDir?: string): void { | ||
| export function scaffoldAgentInWorkspace(workspaceRoot: string, name: string, agentsDir?: string): void { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid exporting internal scaffolding helpers only for tests.
This expands the core CLI module’s exported surface and makes internals harder to evolve safely. Prefer testing through CLI invocation paths instead of exporting internals for test access.
As per coding guidelines: "src/**: Core CLI source. Check for breaking changes to exported types." and "CLI commands should be tested as a user would invoke them, not just as unit tests exercise them".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/term-commands/init.ts` at line 46, The function scaffoldAgentInWorkspace
is an internal scaffolding helper that was exported (scaffoldAgentInWorkspace)
only for tests; remove the export so it becomes module-private and stop widening
the CLI public surface. Update any tests that import scaffoldAgentInWorkspace to
exercise the behavior via the public CLI invocation path (e.g., call the CLI
entry function or the init command runner used by the user-facing command)
instead of importing this internal helper, and adjust test setup to capture
filesystem effects or stdout/stderr as needed.
Comment on lines
+91
to
+92
| console.log(' brain/memory/MEMORY.md (seeded)'); | ||
| console.log(' .claude/settings.local.json (auto-memory enabled)'); |
There was a problem hiding this comment.
Do not add new console.log calls in src/**/*.ts.
These new log lines violate the source no-console rule.
🔧 Proposed fix
- console.log(' brain/memory/MEMORY.md (seeded)');
- console.log(' .claude/settings.local.json (auto-memory enabled)');
+ process.stdout.write(' brain/memory/MEMORY.md (seeded)\n');
+ process.stdout.write(' .claude/settings.local.json (auto-memory enabled)\n');As per coding guidelines: "src/**/*.ts: No console.log in source code (enforced by biome, relaxed in tests)".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| console.log(' brain/memory/MEMORY.md (seeded)'); | |
| console.log(' .claude/settings.local.json (auto-memory enabled)'); | |
| process.stdout.write(' brain/memory/MEMORY.md (seeded)\n'); | |
| process.stdout.write(' .claude/settings.local.json (auto-memory enabled)\n'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/term-commands/init.ts` around lines 91 - 92, Remove the two console.log
calls in src/term-commands/init.ts that print "brain/memory/MEMORY.md (seeded)"
and ".claude/settings.local.json (auto-memory enabled)" and replace them with
the project's approved logging mechanism (e.g., use logger.info or
processLogger.info used elsewhere) or omit them entirely; ensure you call the
project's logger API rather than console.log so the change conforms to the
src/**/*.ts no-console rule and linting.
Bulk migration of wish artifacts that had accumulated in an agent workspace instead of the repo. Moves 47 wishes to .genie/wishes/ and updates the living brainstorm jar (.genie/brainstorm.md) to match workspace state. Key wish: restores genie-simulations (agent eval via real multi-turn Omni conversations, 9-dimension scoring, 100 scenarios per agent). The wish existed only in genie-wishes-backup/ — promoted to active since sprint planning targets it. Also flushes several in-flight wishes from the sprint: - brain-help-passthrough (READY, closes #1118) - scaffold-auto-memory (READY, closes #1106) - brain-cag-v2-polish (from brain-cag-v2 brainstorm) Note: .genie/brainstorms/ subdirectories are gitignored by design (workspace-local state), so individual brainstorm folders are not part of this commit. The brainstorm.md jar remains the canonical index. See AGENTS.md rule: wish artifacts belong in the repo folder, never in an agent's private workspace.
Housekeeping pass after the bulk migration (PR #1130). Per brainstorm.md "Poured (shipped)" and "Killed" sections, 30 wishes are no longer active and have been moved under .genie/wishes/_archive/ to reduce clutter in the active backlog. Shipped (27): - agent-stability-hardening (PR #1112) - brain-foundation, brain-embeddings, brain-intelligence, brain-observability, brain-identity-impl, brain-init-skill (all 6 brain-obsidian children, @khal-os/brain v1.22.0) - env-defaults-local-mode - genie-layout-migration, genie-model-resolution, genie-onboarding-flow (onboarding overhaul) - omni-lifecycle-hardening (PR #359), omni-skill-upgrade, omni-version-unify - os-services-orphan-leak (PR #272 + #274) - pgserve-daemon-ownership (PR #827) - remove-openclaw - rlmx-v02, rlmx-v03-cag, rlmx-v04-gemini3 (npm v0.260331.5) - session-capture-v2 (PR #825) - v4-database-layer, v4-hook-cli-safety, v4-message-routing, v4-session-executor, v4-spawn-resilience, v4-team-lifecycle (v4 Stability Sprint) Killed (3): - brain-obsidian (parent spec — all 6 children shipped, reference only) - genie-app (superseded by genie-app-v1 / genie-studio) - session-ingester-perf (superseded by session-capture-v2) Also updates brainstorm.md links to point at wishes/_archive/<name>/WISH.md and fixes a duplicate v4 Stability Sprint entry. Active backlog drops from 47 → 20 wishes + 1 backup folder.
Group 1 + Group 2 of wish velocity-dashboard. Collector (collect-stats.sh / backfill.sh): - Per-day metrics from git history across ALL branches - Commits count, LoC added/removed, releases, contributors - Cumulative mode for all-time totals - Pathspec exclusions for LoC: drops node_modules, dist, build, lockfiles, minified bundles, worktree artifacts, generated assets. Without these, a cleanup commit (e.g. 9bf6634 removing 524k lines of .claude/worktrees artifacts on 2026-03-14) dominates the 30d window and destroys the signal. Chart engine (generate-charts.py): - Pure SVG string templating, zero external dependencies (no matplotlib, no plotly) - Dark theme (#0d1117) matching GitHub's default - Bar charts for commits and releases - Stacked bar chart (green/red) for LoC additions/deletions - 800x200px, system-ui font fallback - Axis labels, date range markers, max-value ticks Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…uthor aliases Group 3 of velocity-dashboard wish: generate-velocity.py writes the full dashboard with summary table, SVG chart refs, and contributor leaderboard. generate-readme-hero.py updates README METRICS block with a compact 7d hero line. author-aliases.json normalizes duplicate git author names. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Rewrite run-metrics.sh to use git-based pipeline: collect-stats → backfill → generate-charts → generate-velocity → generate-readme-hero. Rewrite AGENT.md as complete autonomous execution guide. Extend state.json with daily_stats_count, charts_generated, velocity_md_updated. Remove 10 old GitHub-API-based tools (batch-commit, cached-fetch, commit-formatter, fast-parse, generate-tools, github-api, parse-metrics, perf-analyzer, self-refine, update-readme). Dry-run completes in <1s. All outputs validated. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Two fixes + one feature addition on top of the initial velocity-dashboard implementation: 1. LoC pathspec exclusions tightened Changed ':(exclude)X' to ':(exclude,glob)X/**' so nested paths are matched recursively. Added `.claude/worktrees/**` to the exclusion set — commit 9bf6634 deleted 524,508 lines of accidentally committed claude worktree artifacts on 2026-03-14, which was dominating the 30d LoC window and showing -357K net. With the fix, 30d LoC is now +157.2K — a realistic and positive signal of actual source growth. 2. Major Version Milestones section added to VELOCITY.md New compute_version_journey() helper runs git log with the same pathspec exclusions and measures days, commits (linear + all-branch), PRs merged, LoC added, and contributors between two refs. generate-velocity.py now emits a "Major Version Milestones" table with two rows: - Genesis → v4 stable: 52 days, 110 PRs, +58.1K LoC, 22 contribs - v3 → v4 (version sprint): 15 days, 65 PRs, +22.2K LoC, 11 contribs Extensible — drop more find_first_major_tag() calls to track v5, v6, etc. as they ship. 3. All outputs regenerated with the fixed filter: - .genie/agents/metrics-updater/daily-stats.jsonl (31 entries) - .genie/assets/commits-30d.svg, loc-30d.svg - VELOCITY.md (summary + charts + leaderboard + milestones) - README.md METRICS block Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
feat(metrics): revamp metrics-updater into velocity dashboard
The previous README hero block had two rendering bugs:
1. `<!-- METRICS:END --> · [Full dashboard](VELOCITY.md)` put the
"Full dashboard" link on the same line as the closing HTML
comment, which GitHub markdown rendered as a stray floating
"· [Full dashboard]" paragraph that ran into the next section
with no separation.
2. The README had no visual — only a numbers line — despite the
brainstorm asking for visible charts in the README.
Fix:
- Move the dashboard link to its own line INSIDE the METRICS
markers with proper blank-line separation
- Embed the commits-30d.svg chart between the hero line and the
link as a visual showpiece
- Tighten the regex to `<!-- METRICS:START -->.*?<!-- METRICS:END -->[^\n]*`
so re-runs self-heal from the prior broken format by consuming
any stray text that sat on the END line
Rendered block now:
<!-- METRICS:START -->
**🚀 234 commits** this week · **40 releases** · **+49.3K LoC** · **7 contributors**
[📊 Full velocity dashboard →](VELOCITY.md)
<!-- METRICS:END -->
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…yntax fix(metrics): clean README hero markdown + embed commits chart
All six wishes have their deliverables merged to dev via the PRs listed below. Moving them from active tracking into _archive/ keeps the wish directory scoped to in-flight and draft work. | Wish | Shipped via | |-----------------------------|------------------------| | velocity-dashboard | #1142 + #1143 (hotfix) | | fix-ghost-approval-p0 | #1134 | | perfect-spawn-hierarchy | #1133 | | brain-help-passthrough | #1129 | | scaffold-auto-memory | #1128 | | unify-bridge-revamp-skills | #1125 + #1126 + #1127 | No code changes — purely a directory reorganization. Wish artifacts remain readable at .genie/wishes/_archive/<slug>/WISH.md for historical reference. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
chore(wishes): archive 6 shipped wishes from recent dev cycle
Wish #1132. Three defense layers + one infrastructure prerequisite to make broken brainstorm links impossible to create and impossible to ship. 5 execution groups across 3 waves: Wave 1 (parallel): G1 wishes-lint.ts standalone + G2 .gitignore narrow Wave 2 (parallel): G3 /brainstorm auto-commit + G4 /wish pre-flight + backfill Wave 3 (sequential): G5 wire wishes:lint into bun run check Includes a plan-review pass that caught + fixed the CI wiring sequencing issue (G1 can't wire into check before G4 backfill runs; split into G1 standalone + G5 wire). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ention The prose-style "Group X (explanation)" form on depends-on lines was parsed as a literal group name, causing `genie status` to error with: Group "4" depends on non-existent group "2. **NOT dependent..." Normalized all depends-on lines to machine-readable form (comma- separated group refs, no parens) and moved explanations to separate "Dependency rationale" paragraphs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…nstorm links Closes #1132. Three defense layers + one infrastructure prerequisite prevent broken `[DESIGN.md](../../brainstorms/<slug>/DESIGN.md)` links from being created or shipped. Write-side (G3) — /brainstorm skill auto-commits on crystallize: skills/brainstorm/SKILL.md — Crystallize step now `git add`s both DESIGN.md and DRAFT.md. Author forgetfulness is no longer a failure mode. Create-side (G4) — /wish skill pre-flights before linking: skills/wish/SKILL.md — Pre-flight check verifies .genie/brainstorms/<slug>/DESIGN.md exists. Falls back to `_No brainstorm — direct wish_` stub when absent (valid for hotfixes, trivial changes). Broken link at creation time is no longer possible. Read-side (G1+G5) — wishes-lint.ts + CI enforcement: scripts/wishes-lint.ts — New linter (121 lines, modeled on skills-lint.ts). Walks .genie/wishes/**/*.md, extracts markdown link syntax, resolves relative targets, exits non-zero on any broken brainstorms/* reference. Honors stub text and <!-- wishes-lint:ignore --> markers. Wired into `bun run check` so both the local pre-push hook and CI Quality Gate fail fast on any broken link. Infrastructure (G2) — .gitignore narrow: .gitignore — Allows DESIGN.md and DRAFT.md under .genie/brainstorms/*/ (required for Layer 1 to work without force-add). Everything else stays private — session notes, transcripts, AI scratchpads. Convention doc: .genie/brainstorms/README.md — One-paragraph statement of the rule: crystallized outputs are committed, WIP is private, wishes link to DESIGN.md for rationale. Backfill pass (G4): 19 wish files across _archive/ and active wishes had their broken brainstorm links replaced with the stub `_Design not recovered — this wish pre-dates the brainstorm-commit convention (see #1132)._` Category B (unrecoverable). Category A recoveries were attempted but the historical DESIGN.md files from archived sessions were genuinely not in any git blob history. Validation: bun run wishes:lint → OK (56 files scanned, 0 broken links) Negative: temporarily delete any linked DESIGN.md → exit 1 with the exact wish file + line + target reported. Shape: Wave 1 (parallel): G1 wishes-lint standalone + G2 gitignore narrow Wave 2 (parallel): G3 /brainstorm auto-commit + G4 /wish pre-flight + backfill Wave 3 (sequential): G5 wire wishes:lint into `bun run check` Plan-review caught the CI wiring sequencing issue before /work: G1 could not wire into `check` before G4's backfill ran, or the PR's own CI would fail on itself. Split resolved that gap before it shipped. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Merge commit resolving a WISH.md parallelism-note conflict between dev and the feature branch. Both sides described the same G3/G4 independence rationale in different words; kept the clearer "**Parallelism note:**" phrasing.
feat(brainstorms): closed-loop enforcement — no more broken wish→brainstorm links
CI Quality Gate failed on scripts/wishes-lint.ts with a biome
useOptionalChain error at line 76 (FIXABLE) and formatter nits
for single-line array literal and function-call collapsing.
Applied:
- `target && target.includes('brainstorms/')` → `target?.includes('brainstorms/')`
- `bunx biome check --write` for the formatter fixes
Verified:
- `bunx biome check .` exits 0 (11 pre-existing complexity warnings
on serve.ts/provider-adapters.ts/scheduler-daemon.ts/agents.ts
remain — not introduced by this fix)
- `bun run wishes:lint` still OK (56 files scanned, 0 broken links)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
namastex888
pushed a commit
that referenced
this pull request
Apr 13, 2026
The preceding auto-version bump used the legacy workflow marker that suppresses CI, leaving the rolling PR stale. This empty commit carries no such marker and exists solely to fire fresh push and pull_request events so the rollup reflects the proven green state from 7c91daf (all 4 runs SUCCESS). Follow-up resolves automatically once PR #1124 merges to main — the auto-version marker fix from #1140 will then take effect on main and this paper cut stops recurring. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
namastex888
pushed a commit
that referenced
this pull request
Apr 14, 2026
The preceding auto-version bump used the legacy workflow marker that suppresses CI, leaving the rolling PR stale. This empty commit carries no such marker and exists solely to fire fresh push and pull_request events so the rollup reflects the proven green state from d8bf000 (all 4 runs SUCCESS). Follow-up resolves automatically once PR #1124 merges to main — the auto-version marker fix from #1140 will then take effect on main and this paper cut stops recurring. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
namastex888
added a commit
that referenced
this pull request
Apr 14, 2026
chore: rolling promotion dev -> main
5 of 6 tasks
Merged
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.
Rolling Promotion PR
Auto-maintained rolling promotion PR from
devtomain.Process:
ready-to-mergeadded when all checks passSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Chore (breaking)