chore: rolling promotion dev -> main - #644
Merged
Merged
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a dependency-injected inbox watcher and spawn daemon, extensive tests across inbox/spawn/router/cli, dependency-injection refactors (team-auto-spawn, protocol-router, msg commands), session-per-project/session resolution, new agent-registry team-lead APIs, shell/tmux helper scripts, many tests, and several manifest version bumps and CI changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Watcher as InboxWatcher
participant Claude as ClaudeConfig (listTeamsWithUnreadInbox)
participant Activity as ActivityChecker (isTeamActive)
participant Spawner as TeamAutoSpawn (ensureTeamLead)
participant Registry as AgentRegistry
participant Logger as Logger/Warn
Watcher->>Claude: listTeamsWithUnreadInbox()
Claude-->>Watcher: [{teamName, unreadCount, workingDir}, ...]
loop per team with unread
Watcher->>Activity: isTeamActive(teamName, session)
Activity-->>Watcher: active? (true/false)
alt not active & workingDir present & failures < MAX
Watcher->>Spawner: ensureTeamLead(teamName, workingDir)
Spawner-->>Watcher: {created: boolean}
alt created
Spawner->>Registry: saveTeamLeadEntry(teamName, paneId, session, window, repoPath)
Registry-->>Spawner: ok
end
else skip or warn
Watcher->>Logger: warn(...)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request represents an automated rolling promotion from the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request contains automated version bumps across several configuration files, advancing the version from 3.260317.16 to 3.260317.18. My review has identified a potential issue with the new version number. It appears to be based on a future date (March 17, 2026), which contradicts the date-based versioning scheme (3.YYMMDD.N) defined in your scripts/version.ts file. This is likely due to a misconfigured system clock in the build environment. Using future-dated versions can be misleading and I've flagged this as a high-severity issue in package.json.
| { | ||
| "name": "@automagik/genie", | ||
| "version": "3.260317.16", | ||
| "version": "3.260317.18", |
Contributor
There was a problem hiding this comment.
The new version 3.260317.18 appears to be based on a future date (March 17, 2026), according to the versioning scheme 3.YYMMDD.N described in scripts/version.ts. This might be caused by an incorrect system clock in the environment where the version was generated. Using a future date can be misleading and may cause issues with tools or processes that rely on semantic or time-based versioning. It would be best to ensure the version numbers reflect the actual date of creation.
- Wire isPaneAlive() into isTeamActive() so dead team-lead processes are detected - Store team-lead pane ID in agent-registry for tracking and auto-respawn - Add 30s grace period to prevent false negatives during slow startup - New inbox-watcher daemon polls native inboxes every 30s, spawns offline team-leads - Backoff after 3 failed spawn attempts to prevent crash loops - CI coverage gate enforces 68% minimum line coverage threshold - Add saveTeamLeadEntry/getTeamLeadEntry to agent-registry - Add listTeamsWithUnreadInbox to claude-native-teams
feat: team-lead liveness checks, inbox watcher daemon, CI coverage gate
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/term-commands/ship.test.ts (3)
88-92: 🧹 Nitpick | 🔵 TrivialStale comment after the fix.
Line 90 says "Repo starts on main branch (or master depending on git version)" but with the
-b mainfix applied tocreateTempGitRepo, the branch is now alwaysmain. The assertion on line 92 can be simplified.Suggested cleanup
describe('main branch protection', () => { it('should reject ship/push from main branch', async () => { - // Repo starts on main branch (or master depending on git version) + // Repo starts on main branch (forced via -b main in createTempGitRepo) const branch = await getCurrentBranch(testRepo); - expect(['main', 'master']).toContain(branch); + expect(branch).toBe('main');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/term-commands/ship.test.ts` around lines 88 - 92, The test comment and assertion are outdated because createTempGitRepo now always sets the branch to main; update the test in ship.test.ts by removing the stale comment and simplify the assertion in the 'main branch protection' case to assert a single expected branch (e.g., expect(branch).toBe('main')) instead of checking both 'main' and 'master'; locate the branch retrieval via getCurrentBranch(testRepo) to change the expectation accordingly.
186-189: 🧹 Nitpick | 🔵 TrivialSame stale comment pattern here.
The comment "Git version dependent - could be main or master" no longer applies since the helper now forces
main.Suggested cleanup
it('should correctly detect main branch', async () => { const branch = await getCurrentBranch(testRepo); - // Git version dependent - could be main or master - expect(['main', 'master']).toContain(branch); + expect(branch).toBe('main'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/term-commands/ship.test.ts` around lines 186 - 189, The test contains a stale comment and loose assertion: update the test in ship.test.ts that calls getCurrentBranch (the helper now forces "main") to remove the outdated comment and tighten the expectation to assert "main" explicitly instead of checking ['main','master']; locate the it block with getCurrentBranch(testRepo) and replace the array containment assertion with a direct equality/assertion for "main" and delete the stale comment line.
22-36: 🧹 Nitpick | 🔵 TrivialExtract git test repository setup to a shared utility.
Both
team-manager.test.tsandteam.test.tsduplicate the same git repository initialization pattern (init, config, commit, branch creation). Extracting this to a shared utility insrc/__tests__/would reduce duplication and ensure consistent behavior across tests. Note thatship.test.tsuses a different branch strategy (-b mainvs. default branch +dev), so the extracted function should support configurable initial branch naming.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/term-commands/ship.test.ts` around lines 22 - 36, Extract the duplicated git repo setup into a shared helper (e.g., createTempGitRepo) under src/__tests__/ and update team-manager.test.ts, team.test.ts, and ship.test.ts to call it; modify the helper signature (currently createTempGitRepo(basePath: string, name: string): Promise<string>) to accept an optional initialBranch parameter (e.g., createTempGitRepo(basePath, name, initialBranch = 'main')) so callers can specify 'main', 'dev', or default behavior, and move git commands (git init -b, config user.email, config user.name, add, commit) into that helper to remove duplication while preserving existing behavior in ship.test.ts which uses a different branch strategy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Line 72: The coverage parsing step that sets LINE_COV from COVERAGE_OUTPUT
using grep/awk is fragile and can silently fail if `bun test --coverage` output
changes; update the logic around LINE_COV so it first validates that
COVERAGE_OUTPUT contains the expected "All files" table row (or a known regex)
and provide a clear fallback path (e.g., attempt alternate parsing patterns or
emit a warnings + fail the job) when the primary grep/awk extraction returns
empty; reference and update the variables and command strings LINE_COV,
COVERAGE_OUTPUT, and the `bun test --coverage` invocation so the workflow either
robustly extracts the coverage percentage or fails fast with a documented
message.
- Around line 74-77: The current CI step silently exits 0 when coverage parsing
fails (the if [ -z "$LINE_COV" ] branch), which can mask broken coverage
reports; change this to fail the job or at minimum produce a clear error:
replace the exit 0 with exit 1 and update the echo to a more prominent error
(e.g., echo "ERROR: Could not parse coverage — failing build") so that when
LINE_COV is empty the workflow fails and surfaces the parsing problem.
In `@src/lib/agent-registry.ts`:
- Around line 323-326: The object literal sets startedAt and lastStateChange
using separate new Date().toISOString() calls which can produce slightly
different timestamps; compute a single timestamp (e.g., const now = new
Date().toISOString()) and reuse it for both startedAt and lastStateChange when
creating the agent record (the object with keys worktree, startedAt, state,
lastStateChange) so they are consistent.
In `@src/lib/claude-native-teams.ts`:
- Around line 346-356: The loop over teamDirs currently assumes each entry is a
directory before constructing inboxFile and reading it; add an explicit
directory check by calling fs.stat or fs.lstat on the team path (join(base,
name)) and skip entries that are not directories (continue) or when stat throws;
keep the existing try/catch around readFile but perform the stat check first to
avoid attempting to read inbox files from regular files.
In `@src/lib/inbox-watcher.test.ts`:
- Around line 34-41: The tests currently set process.env.GENIE_INBOX_POLL_MS =
undefined in beforeEach and afterEach which stores the string "undefined"
instead of removing the env var; update both hooks (the beforeEach block that
calls resetSpawnFailures() and the afterEach block) to remove the variable using
delete process.env.GENIE_INBOX_POLL_MS so the environment key is truly unset for
each test.
In `@src/lib/inbox-watcher.ts`:
- Around line 17-22: InboxWatcherDeps currently simplifies types (omits optional
deps params and uses a plain { created: boolean } instead of
EnsureTeamLeadResult) which may confuse maintainers; add a brief JSDoc above the
InboxWatcherDeps declaration that states this simplification is intentional,
references the original types (e.g., EnsureTeamLeadResult and the optional deps
parameter on listTeamsWithUnreadInbox/ensureTeamLead), and/or update the type
signatures to use the real types (replace the inline { created: boolean } with
EnsureTeamLeadResult and restore optional deps where applicable) so consumers of
listTeamsWithUnreadInbox, isTeamActive, ensureTeamLead, and warn see the
intended contract.
- Around line 129-136: The startInboxWatcher currently calls setInterval using
getInboxPollIntervalMs() even when the poll interval is 0, which can create a
tight loop; before calling setInterval in startInboxWatcher, read the interval
via getInboxPollIntervalMs() and if it is 0 (or <= 0), do not start the interval
— either return a no-op/undefined/clearable timeout or log and return
immediately; update startInboxWatcher to use that guard so checkInboxes is not
scheduled repeatedly when GENIE_INBOX_POLL_MS is 0 (reference startInboxWatcher,
getInboxPollIntervalMs, checkInboxes, InboxWatcherDeps, defaultDeps).
In `@src/lib/team-auto-spawn.test.ts`:
- Around line 166-167: The variables savedPaneId and savedTeam use redundant
assertions ('' as string); remove the unnecessary "as string" and initialize
them simply as empty strings (e.g., let savedPaneId = '' and let savedTeam =
''), or if you prefer explicit typing use let savedPaneId: string = '' and let
savedTeam: string = '' to avoid the redundant type assertion in the test file.
- Around line 37-45: The test stub for ensureNativeTeam uses an unsafe `as any`
cast; change it to return the proper typed object instead: import or reference
the Team (or native team) interface used by the codebase and construct the
returned object to satisfy that type (or annotate the function as returning
Promise<Team> and return a value matching Team). Update the ensureNativeTeam
mock signature and returned fields (e.g., name, description, createdAt,
leadAgentId, leadSessionId, members) so the compiler can verify correctness
instead of silencing errors with `as any`.
In `@src/lib/team-auto-spawn.ts`:
- Around line 193-196: The registry save happening after ensureTeamWindow
creates a race where a crash leaves a live window unregistered; update the
sequence to avoid this by writing the team-lead entry before or atomically with
window creation: call deps.saveTeamLeadEntry(session, teamWindow?.paneId ??
'pending', teamName, windowName, workingDir) before or immediately as part of
ensureTeamWindow, or wrap ensureTeamWindow + saveTeamLeadEntry in a try/catch
that on failure cleans up the newly created window (using the same deps) and
rethrows; reference the functions ensureTeamWindow, saveTeamLeadEntry and
isTeamActive and ensure the stale-window cleanup path remains compatible with
the new ordering.
---
Outside diff comments:
In `@src/term-commands/ship.test.ts`:
- Around line 88-92: The test comment and assertion are outdated because
createTempGitRepo now always sets the branch to main; update the test in
ship.test.ts by removing the stale comment and simplify the assertion in the
'main branch protection' case to assert a single expected branch (e.g.,
expect(branch).toBe('main')) instead of checking both 'main' and 'master';
locate the branch retrieval via getCurrentBranch(testRepo) to change the
expectation accordingly.
- Around line 186-189: The test contains a stale comment and loose assertion:
update the test in ship.test.ts that calls getCurrentBranch (the helper now
forces "main") to remove the outdated comment and tighten the expectation to
assert "main" explicitly instead of checking ['main','master']; locate the it
block with getCurrentBranch(testRepo) and replace the array containment
assertion with a direct equality/assertion for "main" and delete the stale
comment line.
- Around line 22-36: Extract the duplicated git repo setup into a shared helper
(e.g., createTempGitRepo) under src/__tests__/ and update team-manager.test.ts,
team.test.ts, and ship.test.ts to call it; modify the helper signature
(currently createTempGitRepo(basePath: string, name: string): Promise<string>)
to accept an optional initialBranch parameter (e.g., createTempGitRepo(basePath,
name, initialBranch = 'main')) so callers can specify 'main', 'dev', or default
behavior, and move git commands (git init -b, config user.email, config
user.name, add, commit) into that helper to remove duplication while preserving
existing behavior in ship.test.ts which uses a different branch strategy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2b59b260-88c2-4c32-a31a-57fd9de8d6b0
📒 Files selected for processing (9)
.github/workflows/ci.ymlknip.jsonsrc/lib/agent-registry.tssrc/lib/claude-native-teams.tssrc/lib/inbox-watcher.test.tssrc/lib/inbox-watcher.tssrc/lib/team-auto-spawn.test.tssrc/lib/team-auto-spawn.tssrc/term-commands/ship.test.ts
| run: | | ||
| COVERAGE_OUTPUT=$(bun test --coverage 2>&1) | ||
| echo "$COVERAGE_OUTPUT" | ||
| LINE_COV=$(echo "$COVERAGE_OUTPUT" | grep "All files" | awk -F'|' '{print $2}' | tr -d ' ') |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Coverage parsing is fragile — depends on exact bun test --coverage output format.
If Bun changes the coverage table format, this grep/awk chain will silently fail. Consider adding a fallback or documenting the expected format.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml at line 72, The coverage parsing step that sets
LINE_COV from COVERAGE_OUTPUT using grep/awk is fragile and can silently fail if
`bun test --coverage` output changes; update the logic around LINE_COV so it
first validates that COVERAGE_OUTPUT contains the expected "All files" table row
(or a known regex) and provide a clear fallback path (e.g., attempt alternate
parsing patterns or emit a warnings + fail the job) when the primary grep/awk
extraction returns empty; reference and update the variables and command strings
LINE_COV, COVERAGE_OUTPUT, and the `bun test --coverage` invocation so the
workflow either robustly extracts the coverage percentage or fails fast with a
documented message.
Comment on lines
+74
to
+77
| if [ -z "$LINE_COV" ]; then | ||
| echo "WARNING: Could not parse coverage — skipping threshold check" | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
Silent pass when coverage parsing fails may mask broken coverage reports.
If coverage output format changes or bun test --coverage fails to produce parseable output, the step exits 0 and the build passes without coverage enforcement. Consider failing the build or at least emitting a more prominent warning.
Suggested alternative
if [ -z "$LINE_COV" ]; then
- echo "WARNING: Could not parse coverage — skipping threshold check"
- exit 0
+ echo "ERROR: Could not parse coverage from output"
+ exit 1
fi📝 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
| if [ -z "$LINE_COV" ]; then | |
| echo "WARNING: Could not parse coverage — skipping threshold check" | |
| exit 0 | |
| fi | |
| if [ -z "$LINE_COV" ]; then | |
| echo "ERROR: Could not parse coverage from output" | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 74 - 77, The current CI step silently
exits 0 when coverage parsing fails (the if [ -z "$LINE_COV" ] branch), which
can mask broken coverage reports; change this to fail the job or at minimum
produce a clear error: replace the exit 0 with exit 1 and update the echo to a
more prominent error (e.g., echo "ERROR: Could not parse coverage — failing
build") so that when LINE_COV is empty the workflow fails and surfaces the
parsing problem.
Comment on lines
+323
to
+326
| worktree: null, | ||
| startedAt: new Date().toISOString(), | ||
| state: 'spawning', | ||
| lastStateChange: new Date().toISOString(), |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor: new Date() called twice.
Both startedAt and lastStateChange call new Date().toISOString() separately. Could share a single timestamp for consistency.
Share timestamp
+ const now = new Date().toISOString();
await withRegistry((reg) => {
reg.workers[id] = {
id,
paneId,
session,
worktree: null,
- startedAt: new Date().toISOString(),
+ startedAt: now,
state: 'spawning',
- lastStateChange: new Date().toISOString(),
+ lastStateChange: now,
repoPath,📝 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
| worktree: null, | |
| startedAt: new Date().toISOString(), | |
| state: 'spawning', | |
| lastStateChange: new Date().toISOString(), | |
| const now = new Date().toISOString(); | |
| await withRegistry((reg) => { | |
| reg.workers[id] = { | |
| id, | |
| paneId, | |
| session, | |
| worktree: null, | |
| startedAt: now, | |
| state: 'spawning', | |
| lastStateChange: now, | |
| repoPath, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/agent-registry.ts` around lines 323 - 326, The object literal sets
startedAt and lastStateChange using separate new Date().toISOString() calls
which can produce slightly different timestamps; compute a single timestamp
(e.g., const now = new Date().toISOString()) and reuse it for both startedAt and
lastStateChange when creating the agent record (the object with keys worktree,
startedAt, state, lastStateChange) so they are consistent.
Comment on lines
+346
to
+356
| for (const name of teamDirs) { | ||
| // Read inbox messages | ||
| const inboxFile = join(base, name, 'inboxes', 'team-lead.json'); | ||
| let messages: NativeInboxMessage[]; | ||
| try { | ||
| const content = await readFile(inboxFile, 'utf-8'); | ||
| messages = JSON.parse(content); | ||
| } catch { | ||
| continue; // No inbox or invalid JSON | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
No directory validation before reading inbox file.
readdir returns all entries including files. If a non-directory entry exists in ~/.claude/teams/, the code handles it gracefully via try/catch, but this is implicit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/claude-native-teams.ts` around lines 346 - 356, The loop over
teamDirs currently assumes each entry is a directory before constructing
inboxFile and reading it; add an explicit directory check by calling fs.stat or
fs.lstat on the team path (join(base, name)) and skip entries that are not
directories (continue) or when stat throws; keep the existing try/catch around
readFile but perform the stat check first to avoid attempting to read inbox
files from regular files.
Comment on lines
+34
to
+41
| beforeEach(() => { | ||
| resetSpawnFailures(); | ||
| process.env.GENIE_INBOX_POLL_MS = undefined; | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| process.env.GENIE_INBOX_POLL_MS = undefined; | ||
| }); |
There was a problem hiding this comment.
Bug: Assigning undefined to process.env property coerces to string "undefined".
process.env.GENIE_INBOX_POLL_MS = undefined sets the value to the string "undefined", not actually unset. Use delete instead.
Fix
beforeEach(() => {
resetSpawnFailures();
- process.env.GENIE_INBOX_POLL_MS = undefined;
+ delete process.env.GENIE_INBOX_POLL_MS;
});
afterEach(() => {
- process.env.GENIE_INBOX_POLL_MS = undefined;
+ delete process.env.GENIE_INBOX_POLL_MS;
});📝 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
| beforeEach(() => { | |
| resetSpawnFailures(); | |
| process.env.GENIE_INBOX_POLL_MS = undefined; | |
| }); | |
| afterEach(() => { | |
| process.env.GENIE_INBOX_POLL_MS = undefined; | |
| }); | |
| beforeEach(() => { | |
| resetSpawnFailures(); | |
| delete process.env.GENIE_INBOX_POLL_MS; | |
| }); | |
| afterEach(() => { | |
| delete process.env.GENIE_INBOX_POLL_MS; | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/inbox-watcher.test.ts` around lines 34 - 41, The tests currently set
process.env.GENIE_INBOX_POLL_MS = undefined in beforeEach and afterEach which
stores the string "undefined" instead of removing the env var; update both hooks
(the beforeEach block that calls resetSpawnFailures() and the afterEach block)
to remove the variable using delete process.env.GENIE_INBOX_POLL_MS so the
environment key is truly unset for each test.
Comment on lines
+37
to
+45
| ensureNativeTeam: async () => | ||
| ({ | ||
| name: 'test-team', | ||
| description: '', | ||
| createdAt: Date.now(), | ||
| leadAgentId: '', | ||
| leadSessionId: '', | ||
| members: [], | ||
| }) as any, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
as any cast bypasses type safety.
The cast on line 45 silences type errors. If ensureNativeTeam return type changes, this test won't catch the mismatch.
Consider returning the exact type
ensureNativeTeam: async () =>
- ({
+ ({
name: 'test-team',
description: '',
createdAt: Date.now(),
leadAgentId: '',
leadSessionId: '',
members: [],
- }) as any,
+ }),📝 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
| ensureNativeTeam: async () => | |
| ({ | |
| name: 'test-team', | |
| description: '', | |
| createdAt: Date.now(), | |
| leadAgentId: '', | |
| leadSessionId: '', | |
| members: [], | |
| }) as any, | |
| ensureNativeTeam: async () => | |
| ({ | |
| name: 'test-team', | |
| description: '', | |
| createdAt: Date.now(), | |
| leadAgentId: '', | |
| leadSessionId: '', | |
| members: [], | |
| }), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/team-auto-spawn.test.ts` around lines 37 - 45, The test stub for
ensureNativeTeam uses an unsafe `as any` cast; change it to return the proper
typed object instead: import or reference the Team (or native team) interface
used by the codebase and construct the returned object to satisfy that type (or
annotate the function as returning Promise<Team> and return a value matching
Team). Update the ensureNativeTeam mock signature and returned fields (e.g.,
name, description, createdAt, leadAgentId, leadSessionId, members) so the
compiler can verify correctness instead of silencing errors with `as any`.
Comment on lines
+166
to
+167
| let savedPaneId = '' as string; | ||
| let savedTeam = '' as string; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant type assertion.
'' as string is unnecessary — '' is already typed as string.
Simplify
- let savedPaneId = '' as string;
- let savedTeam = '' as string;
+ let savedPaneId = '';
+ let savedTeam = '';📝 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
| let savedPaneId = '' as string; | |
| let savedTeam = '' as string; | |
| let savedPaneId = ''; | |
| let savedTeam = ''; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/team-auto-spawn.test.ts` around lines 166 - 167, The variables
savedPaneId and savedTeam use redundant assertions ('' as string); remove the
unnecessary "as string" and initialize them simply as empty strings (e.g., let
savedPaneId = '' and let savedTeam = ''), or if you prefer explicit typing use
let savedPaneId: string = '' and let savedTeam: string = '' to avoid the
redundant type assertion in the test file.
Comment on lines
+193
to
+196
| const teamWindow = await deps.ensureTeamWindow(session, windowName, workingDir); | ||
|
|
||
| // Save team-lead pane ID to agent registry | ||
| await deps.saveTeamLeadEntry(teamName, teamWindow.paneId, session, windowName, workingDir); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Registry save happens after window creation — small race window.
If the process crashes between ensureTeamWindow (line 193) and saveTeamLeadEntry (line 196), the window exists but isn't tracked. On restart, isTeamActive would return false (no registry entry for grace period), potentially leading to double spawn attempts. The stale window cleanup at lines 164-178 mitigates this, so impact is low.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/team-auto-spawn.ts` around lines 193 - 196, The registry save
happening after ensureTeamWindow creates a race where a crash leaves a live
window unregistered; update the sequence to avoid this by writing the team-lead
entry before or atomically with window creation: call
deps.saveTeamLeadEntry(session, teamWindow?.paneId ?? 'pending', teamName,
windowName, workingDir) before or immediately as part of ensureTeamWindow, or
wrap ensureTeamWindow + saveTeamLeadEntry in a try/catch that on failure cleans
up the newly created window (using the same deps) and rethrows; reference the
functions ensureTeamWindow, saveTeamLeadEntry and isTeamActive and ensure the
stale-window cleanup path remains compatible with the new ordering.
Derive tmux session name from cwd basename instead of hardcoded 'genie'. Each project directory now gets its own tmux session with hash-based disambiguation for same-basename collisions (e.g., project-a vs project-a-c7b1). - Add resolveSessionName() for cwd-based session name derivation - Set GENIE_SESSION env var on every window in createSession() and focusTeamWindow() - Use switch-client for cross-project session switching when inside tmux - handleReset() naturally scopes to current project via derived session name - Remove DEFAULT_SESSION_NAME constant
Enable tmux 3.2+ dual status bar: top bar shows project session tabs (via genie-projects.sh) with branding and system info, bottom bar shows task window tabs (via genie-tasks.sh) for the active session. Includes version guard fallback to single bar on tmux < 3.2, and Ctrl+)/( keybindings for project session switching.
…rs.json Upgrade genie-tasks.sh and genie-projects.sh stubs with: - Agent count per window (×N format) from workers.json - Worst-state-wins emoji aggregation per window (error > permission > working > spawning > idle > done > suspended) - State-to-emoji mapping: spawning→⏳ working→🔨 idle→⏸ done→✓ error→✗ permission→❓ suspended→💤 - Single jq pass for performance (14ms measured) - $GENIE_WORKERS env var override for isolated testing - Graceful fallback when workers.json missing or empty - genie-projects.sh merges tmux sessions with workers.json agent counts
Replace inline session filtering with registry.filterBySession() call, reducing cognitive complexity from 16 to under the biome threshold.
…t scopedWorkers helper
…orts Tests were passing deps as the second argument but isTeamActive now requires sessionName as a string parameter before deps.
Git sets GIT_DIR in hook environments, which leaks into child processes when hooks run bun test. Test setupTestRepo() uses `git -C /tmp/...` but GIT_DIR takes precedence over -C, causing test commits to land on the real repo branch — wiping all tracked files with test fixtures. Unset GIT_DIR and GIT_WORK_TREE at the top of all three husky hooks so child processes discover their own repositories correctly.
Git sets GIT_DIR in hook environments, which leaks into child processes when hooks run bun test. Test setupTestRepo() uses `git -C /tmp/...` but GIT_DIR takes precedence over -C, causing test commits to land on the real repo branch — wiping all tracked files with test fixtures. Unset GIT_DIR and GIT_WORK_TREE at the top of all three husky hooks so child processes discover their own repositories correctly.
Enable tmux 3.2+ dual status bar: top bar shows project session tabs (via genie-projects.sh) with branding and system info, bottom bar shows task window tabs (via genie-tasks.sh) for the active session. - genie.tmux.conf: status 2, status-format[0]/[1], if-shell version guard - genie-projects.sh: stub listing sessions with window counts - genie-tasks.sh: stub listing windows for given session - Ctrl+)/Ctrl+( keybindings for session switching - Graceful fallback to single bar on tmux < 3.2
Adds session-scoped messaging: agents in one project session cannot message agents in another. Uses GENIE_SESSION env var and registry filterBySession() to enforce isolation at the checkSendScope gate.
…3.2 for dual status bars
feat: dual tab bar — per-project tmux session isolation
fix: unset GIT_DIR in husky hooks to prevent test pollution
Add comprehensive tests across 16 modules: - inbox-watcher: PID management, isProcessAlive, getInboxPollIntervalMs - protocol-router: recipient resolution, template matching, auto-spawn - genie entrypoint: --reset flag, error handling, session edge cases - patterns/state-detector: ANSI stripping, pattern matching, state detection - mosaic-layout, claude-settings, codex-config, genie-config, genie-dir - system-detect, tmux-wrapper, shortcuts, state parseRef, type schemas Coverage: 56.40% → 68.73% (threshold: 68%) Tests: 791 → 920 (+129 tests)
4 tasks done
Add tests for claude-logs (readLogFile, getLogsForPane) and tmux (isPaneAlive validation). Provides 1.25% headroom above 68% threshold to account for local/CI measurement variance.
test: boost CI coverage to 68% threshold
There was a problem hiding this comment.
Actionable comments posted: 25
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/protocol-router-spawn.ts (1)
105-109:⚠️ Potential issue | 🟠 MajorSanitize the team name before
ensureTeamWindow()in auto-spawn too.Line 108 still sends the raw team name to tmux. Auto-spawning
ops.botwill hit the samewindow.paneparsing bug and can fall back into the wrong pane/session instead of the team window.Based on learnings, "Apply sanitizeWindowName() from session.ts to teamName wherever it is passed to ensureTeamWindow or used to build tmux target strings. This prevents dots in teamName from being interpreted as pane separators (pane-like paths) and causing 'can't find pane' errors, mirroring the fix already implemented in session.ts."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/protocol-router-spawn.ts` around lines 105 - 109, The team name is not sanitized before calling ensureTeamWindow in auto-spawn, so dots can be treated as tmux pane separators; call the existing sanitizeWindowName function (from session.ts) on the team name returned by resolveSpawnSession (the variable used in the ensureTeamWindow(session, team, repoPath) call) and use that sanitized name wherever tmux target strings or window identifiers are constructed in this file (including any usages that build window/pane paths), ensuring ensureTeamWindow and any tmux target builders receive the sanitized teamName instead of the raw team value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/tmux/genie-projects.sh`:
- Around line 18-24: The session-count aggregation is including daemon
"team-lead" rows written by saveTeamLeadEntry(), so update the jq pipeline that
produces worker_data to exclude entries where .value.role == "team-lead" before
grouping by session; specifically modify the pipeline starting from ".workers //
{} | to_entries" to filter out team-lead entries (e.g., using
map(select(.value.role != "team-lead"))) then continue with
group_by(.value.session) and the existing count logic so only real worker tasks
are counted.
- Line 1: The script uses associative arrays via "declare -A" (seen at the spots
that fail) but doesn't enforce Bash >=4; add a Bash version guard at the top
using BASH_VERSINFO (e.g., check BASH_VERSINFO[0] < 4) and print a clear error
and exit if too old, or alternatively refactor the associative-array usage (the
"declare -A" occurrences) to indexed arrays and map keys to indices; implement
the version-check approach by adding the guard before any "declare -A" usage so
the script fails fast with a helpful message if the shell is older than 4.
In `@scripts/tmux/genie-tasks.sh`:
- Around line 58-83: The jq pipeline ignores the registry's alternate fields:
add support for the AgentState 'question' and the 'window' alias so agents with
question state get an emoji and entries using window (not windowName) are
grouped correctly. Update the state_priority and state_emoji defs to include
"question" (with appropriate priority and emoji), and change the selectors used
where window name and state are read to prefer .value.window //
.value.windowName and to compute worst_state from (.value.state //
.value.question // "idle"); keep the rest of the pipeline intact (look for the
map/group_by that builds window and worst_state in the worker_data jq
expression).
- Line 1: The script uses associative arrays via "declare -A" (e.g., the
associative arrays created with declare -A and subscript expansion) which
requires Bash 4+, so add an explicit runtime version guard at the top: check
BASH_VERSINFO[0] and exit with a clear message if it's less than 4 (or
alternatively change the shebang to a stricter interpreter that guarantees Bash
4+); ensure the check runs before any use of declare -A or associative
subscripts so the script fails fast with a helpful error when running under Bash
3.x.
In `@scripts/tmux/genie.tmux.conf`:
- Around line 112-115: The current bindings bind -n C-) and bind -n C-( are not
portable because many terminals don't emit distinct shifted-key sequences;
update the configuration by replacing those bindings with portable alternatives
(e.g., bind -n M-Right and bind -n M-Left) or explicitly enable extended keys by
adding set -g extended-keys on at the top and document that a compatible
terminal (xterm/iTerm2/foot) is required; update the session-switching bind
lines (bind -n C-) and (bind -n C-() or their surrounding comments to reflect
the chosen approach so maintainers know which method is used.
- Around line 97-99: The if-shell version check in genie.tmux.conf uses a sed
regex that only matches lines beginning with "tmux" so OpenBSD's "openbsd-6.x"
tmux -V output fails and falls back to 'set -g status on'; update the if-shell
condition (the line containing if-shell 'tmux -V | sed ... | awk ...') to either
(a) broaden the regex to accept the OpenBSD prefix (match either "tmux X.Y" or
"openbsd-* X.Y") or (b) replace the version parse with a capability probe that
checks for the presence of status-format (i.e., run a tmux query for the
status-format capability and use that result to choose between 'set -g status 2'
and 'set -g status on'); modify that exact if-shell invocation accordingly so
OpenBSD tmux instances that support status-format use the two-line status.
In `@src/genie-commands/__tests__/session.test.ts`:
- Around line 194-228: The tests call the real tmux-backed resolver causing
flakes; instead mock the tmux session lookup used by resolveSessionName so tests
control whether a session exists: for the plain basename and dot-sanitization
tests stub the tmux listing function (e.g., the module/method resolveSessionName
calls to enumerate tmux sessions) to return an empty array, and for the
collision/hash behavior drive the collision path through resolveSessionName
itself by stubbing tmux to return a conflicting basename (to force the resolver
to append the deterministic hash) and asserting on resolveSessionName outputs
rather than reimplementing the hash locally; update tests that reference
sanitizeWindowName and the local hash to call resolveSessionName with
appropriate tmux stubs to assert uniqueness and determinism.
In `@src/genie-commands/session.ts`:
- Around line 58-73: resolveSessionName reads GENIE_CWD using
tmux.getWindowEnv(baseName) but the code that writes GENIE_CWD uses a
target-scoped name like `${sessionName}:${windowName}`, so the read misses the
stored cwd and creates a hashed session; update resolveSessionName to read
GENIE_CWD from the exact tmux target used when writing (use tmux.getWindowEnv
with the same `${baseName}:${windowName}` target or derive the correct window
via tmux helpers), and ensure the writers that set GENIE_CWD (the code
referencing sessionName and windowName) and resolveSessionName all use the
identical target string; reference resolveSessionName, tmux.getWindowEnv,
sessionName/windowName and shortPathHash when making the change.
In `@src/lib/claude-logs.test.ts`:
- Around line 300-301: The test currently uses a CommonJS require to import
getLogsForPane; update the test to use the ES module import style by adding
getLogsForPane to the existing top-level import list (alongside the other
imports at the top of the file) and remove the inline
require('./claude-logs.js') call in the describe block so the test consistently
uses ES imports for getLogsForPane.
- Around line 272-294: The test creates a temp directory and mixes CommonJS
require with duplicate imports; fix by adding readLogFile to the existing
top-level ES import (remove the local require('./claude-logs.js')), remove the
redundant re-import of writeFile (delete the wf import at line ~280 and reuse
the already-imported writeFile), and ensure the temp directory created from
TEST_DIR/readlogfile-test is cleaned up after each test (use os.tmpdir or
TEST_DIR and add an afterEach that removes the tempDir via fs.rm or rimraf).
Keep references to readLogFile, sampleLogEntries, TEST_DIR, mkdir, and writeFile
when making these changes.
In `@src/lib/claude-settings.test.ts`:
- Around line 13-25: Tests for hookScriptExists and removeHookScript currently
operate against the real home directory because they resolve via homedir();
update the tests to isolate global state by setting process.env.GENIE_HOME to a
temporary directory (use the test framework's tmpdir or fs.mkdtemp) before each
test and restore/cleanup in an afterEach; ensure hookScriptExists() and
removeHookScript() calls in the tests operate against this tmp GENIE_HOME so
removeHookScript() cannot delete real files, and remove any reliance on the
developer's actual ~/.claude state.
In `@src/lib/codex-config.test.ts`:
- Around line 10-23: Tests rely on global environment and only assert types;
isolate them by creating a tmpdir fixture, set process.env.GENIE_HOME = tmpdir
in beforeEach, and restore/cleanup in afterEach, then assert concrete behavior:
for getCodexConfigPath() assert the returned path is inside
`${GENIE_HOME}/.codex` and ends with `config.toml`; for isCodexConfigured()
assert false when the config file does not exist and true after creating a
config file at the path returned by getCodexConfigPath(); reference the
functions getCodexConfigPath and isCodexConfigured and ensure the temp directory
is removed and env restored in cleanup.
In `@src/lib/genie-config.test.ts`:
- Around line 60-74: The current tests for loadGenieConfig and
loadGenieConfigSync only check for existence and type; update them to assert
stable default fields and shape so regressions are caught. For both
loadGenieConfig and loadGenieConfigSync add assertions (using toMatchObject or
toEqual) that verify expected keys and default values/types (e.g., required
top-level keys like name, version/env, port or host, providers array/object, and
logging settings) and/or add a snapshot test of the returned config; if your
code exposes a DEFAULT_GENIE_CONFIG constant or schema, compare against that
instead of loose type checks so any shape/default change fails the test.
- Around line 1-17: The tests import genie-config at module scope which reads
homedir() before tests can set up isolation; change the test to set
process.env.GENIE_HOME to a fresh tmpdir before importing or to create/clean a
tmpdir in a beforeEach/afterEach and then dynamically import './genie-config.js'
(so module-level constants pick up the env), and ensure cleanup of the tmpdir in
afterEach; apply this to the test file so functions like getGenieDir,
getGenieConfigPath, genieConfigExists, loadGenieConfig, loadGenieConfigSync,
isSetupComplete, getTerminalConfig and contractPath are exercised against the
isolated GENIE_HOME.
In `@src/lib/mosaic-layout.test.ts`:
- Around line 7-17: The tests in mosaic-layout.test.ts use double-quoted
expected strings which violates the TypeScript string-quote rule; update the
three expectations to use single-quoted strings. Locate the three test cases
that call buildLayoutCommand (the tests titled 'builds even-horizontal layout
for vertical mode', 'defaults to mosaic (tiled) layout', and the first test that
asserts "select-layout -t 'session:0' tiled") and change the expected values
passed to expect(...).toBe(...) to use single quotes (e.g., select-layout -t
'@4' tiled) to comply with the project's TypeScript quoting guideline.
In `@src/lib/orchestrator/patterns.test.ts`:
- Around line 35-39: The test is asserting on matches[0] which makes it
order-dependent; update the test in patterns.test.ts to assert presence of a
match with type 'bash_permission' regardless of order by using
matchPatterns('Allow bash command? [Y/n]', permissionPatterns) then checking
matches.some(m => m.type === 'bash_permission') (and similarly replace the
matches[0] assertion in the other case around lines 74–78) so the test only
verifies that a match with the expected type exists, not its index.
- Around line 52-57: The test for "matches global patterns multiple times" is
too loose (expect(numbered.length).toBeGreaterThanOrEqual(2)); update it to
assert the exact count for the given fixture by replacing that assertion with
expect(numbered.length).toBe(3) so matchPatterns(content, questionPatterns) is
verified to find all three 'claude_code_numbered_options' occurrences.
In `@src/lib/protocol-router-session.test.ts`:
- Around line 36-37: The beforeEach currently sets process.env.TMUX_PANE and
process.env.GENIE_SESSION to "undefined as unknown as string"; change this to
remove those environment variables like the afterEach does by using the delete
operator on process.env.TMUX_PANE and process.env.GENIE_SESSION so the test
setup and teardown are consistent and won't leave invalid string values in the
environment.
In `@src/lib/tmux-wrapper.test.ts`:
- Around line 24-30: The test currently only asserts that
executeTmux(['-v','list-commands']) returns a string, which doesn't ensure the
'-v' flag was stripped; update the test to spy or mock the exec boundary used by
executeTmux (e.g., the child_process exec/execFile wrapper or the internal
runner function called by executeTmux) and assert the actual executed
command/args do not contain '-v' but do contain 'list-commands'. Specifically,
add a mock/spying assertion on the underlying exec call invoked by executeTmux
to verify the final args array or command string passed excludes '-v' so the
test will fail if flag-stripping regresses.
- Around line 7-12: The tests currently swallow all errors in the three
try/catch blocks around executeTmux in tmux-wrapper.test.ts; change each bare
catch to inspect the caught error (e.g., if (err && (err.code === 'ENOENT' ||
/not found|command not found|No such file or
directory/i.test(String(err.message)))) return; else throw err) so only the
“tmux not installed” case is ignored and any other failures from executeTmux are
rethrown; update the three catch blocks that wrap the executeTmux calls
accordingly.
In `@src/lib/tmux.test.ts`:
- Around line 19-22: The test "returns false for non-existent pane" calls
isPaneAlive('%999999') which proceeds to call capturePaneContent and invokes
real tmux commands; move this case out of the unit test into an integration test
suite or add an explicit tmux-available guard so it is skipped when tmux isn't
present. Locate the test in src/lib/tmux.test.ts and either relocate it to your
integration tests or wrap it with a precondition that checks tmux availability
(e.g., a helper isTmuxAvailable or executing a lightweight 'tmux' check) before
calling isPaneAlive or else mock capturePaneContent so the unit test does not
execute real tmux commands. Ensure the test name and behavior remain the same
when running under the integration environment.
In `@src/term-commands/agents.ts`:
- Around line 491-499: The resolveSpawnTeamWindow function currently passes the
raw team name into tmux.ensureTeamWindow which can break when team contains
dots; sanitize the tmux window name before calling ensureTeamWindow by importing
and applying the sanitizeWindowName function (from session.ts) to the team value
used in tmux targets while keeping the original logical team variable untouched,
i.e., call tmux.ensureTeamWindow(session, sanitizeWindowName(team), cwd) (or
otherwise pass the sanitized name into any tmux target-building logic) so dots
are not interpreted as pane separators.
- Around line 831-833: The code is defaulting interactive tmux spawns to
session: process.env.GENIE_SESSION ?? 'genie', which is incorrect because the
current pane may not have GENIE_SESSION set; instead resolve the actual tmux
session (or derive it from cwd) before falling back. Replace the hardcoded
fallback with a call to the tmux/session resolver used elsewhere (e.g., reuse
the session resolution logic from the session module or a helper like
getTmuxSession/getSessionFromCwd) and assign that result to the session field
(keep spawnIntoCurrentWindow, teamWasExplicit and insideTmux logic intact);
ensure the lookup queries tmux for the session name when insideTmux and only
fall back to a safe default if resolution fails.
In `@src/term-commands/shortcuts.test.ts`:
- Around line 15-29: Replace inline require('node:fs') calls with a single ES
module import at the top (e.g., import { writeFileSync, unlinkSync } from
'node:fs') and stop manually unlinking inside each test; instead create temp
files using a tmpdir/mkdtemp-based filename per test and add an afterEach that
unlinks any file created by the tests. Update the two tests that call
isShortcutsInstalled to use the shared tmp filename variable and rely on
afterEach cleanup so failures don’t leak files, and keep the
isShortcutsInstalled references unchanged.
In `@src/types/genie-config.test.ts`:
- Around line 56-63: Add a new test named like "rejects invalid launcher values"
that calls WorkerProfileSchema.parse with an invalid launcher string (e.g.,
launcher: 'not-a-launcher', claudeArgs: []) and asserts that parse throws (use
expect(() => WorkerProfileSchema.parse(...)).toThrow() or toThrowError()); this
ensures WorkerProfileSchema.parse rejects non-'claude' launcher values and locks
schema strictness.
---
Outside diff comments:
In `@src/lib/protocol-router-spawn.ts`:
- Around line 105-109: The team name is not sanitized before calling
ensureTeamWindow in auto-spawn, so dots can be treated as tmux pane separators;
call the existing sanitizeWindowName function (from session.ts) on the team name
returned by resolveSpawnSession (the variable used in the
ensureTeamWindow(session, team, repoPath) call) and use that sanitized name
wherever tmux target strings or window identifiers are constructed in this file
(including any usages that build window/pane paths), ensuring ensureTeamWindow
and any tmux target builders receive the sanitized teamName instead of the raw
team value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6687de41-fd34-45f1-b7ec-a0ab3249209c
📒 Files selected for processing (38)
.husky/commit-msg.husky/pre-commit.husky/pre-pushknip.jsonscripts/tmux/genie-projects.shscripts/tmux/genie-tasks.shscripts/tmux/genie.tmux.confsrc/genie-commands/__tests__/session.test.tssrc/genie-commands/session.tssrc/genie-commands/shortcuts.test.tssrc/genie.test.tssrc/genie.tssrc/lib/agent-registry.test.tssrc/lib/agent-registry.tssrc/lib/claude-logs.test.tssrc/lib/claude-settings.test.tssrc/lib/codex-config.test.tssrc/lib/genie-config.test.tssrc/lib/genie-dir.test.tssrc/lib/inbox-watcher.test.tssrc/lib/inbox-watcher.tssrc/lib/mosaic-layout.test.tssrc/lib/orchestrator/patterns.test.tssrc/lib/orchestrator/state-detector.test.tssrc/lib/protocol-router-session.test.tssrc/lib/protocol-router-spawn.tssrc/lib/protocol-router.tssrc/lib/system-detect.test.tssrc/lib/team-auto-spawn.test.tssrc/lib/team-auto-spawn.tssrc/lib/tmux-wrapper.test.tssrc/lib/tmux.test.tssrc/term-commands/agents.tssrc/term-commands/msg-routing.test.tssrc/term-commands/msg.tssrc/term-commands/shortcuts.test.tssrc/term-commands/state.test.tssrc/types/genie-config.test.ts
💤 Files with no reviewable changes (1)
- knip.json
| @@ -0,0 +1,70 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
Guard the Bash 4+ dependency.
declare -A requires Bash 4.0 or later for associative-array support, but #!/usr/bin/env bash does not enforce a minimum version. Systems with Bash 3.x will fail at lines 16 and 44 when the script attempts to declare associative arrays. Add an explicit version check (e.g., [[ ${BASH_VERSINFO[0]} -lt 4 ]]) or refactor to use indexed arrays instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/tmux/genie-projects.sh` at line 1, The script uses associative arrays
via "declare -A" (seen at the spots that fail) but doesn't enforce Bash >=4; add
a Bash version guard at the top using BASH_VERSINFO (e.g., check
BASH_VERSINFO[0] < 4) and print a clear error and exit if too old, or
alternatively refactor the associative-array usage (the "declare -A"
occurrences) to indexed arrays and map keys to indices; implement the
version-check approach by adding the guard before any "declare -A" usage so the
script fails fast with a helpful message if the shell is older than 4.
Comment on lines
+18
to
+24
| worker_data=$(jq -r ' | ||
| .workers // {} | to_entries | ||
| | group_by(.value.session) | ||
| | map({session: .[0].value.session, count: length}) | ||
| | map(.session + "\t" + (.count | tostring)) | ||
| | .[] | ||
| ' "$workers_file" 2>/dev/null) || worker_data="" |
There was a problem hiding this comment.
Exclude team-lead rows from the session count.
saveTeamLeadEntry() now writes role: 'team-lead' entries into workers for the same session (src/lib/agent-registry.ts:333-356). This aggregation counts those daemon rows too, so a project with no actual tasks can still render as (1).
Suggested fix
- worker_data=$(jq -r '
- .workers // {} | to_entries
+ worker_data=$(jq -r '
+ .workers // {} | to_entries
+ | map(select(.value.role != "team-lead"))
| group_by(.value.session)
| map({session: .[0].value.session, count: length})📝 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
| worker_data=$(jq -r ' | |
| .workers // {} | to_entries | |
| | group_by(.value.session) | |
| | map({session: .[0].value.session, count: length}) | |
| | map(.session + "\t" + (.count | tostring)) | |
| | .[] | |
| ' "$workers_file" 2>/dev/null) || worker_data="" | |
| worker_data=$(jq -r ' | |
| .workers // {} | to_entries | |
| | map(select(.value.role != "team-lead")) | |
| | group_by(.value.session) | |
| | map({session: .[0].value.session, count: length}) | |
| | map(.session + "\t" + (.count | tostring)) | |
| | .[] | |
| ' "$workers_file" 2>/dev/null) || worker_data="" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/tmux/genie-projects.sh` around lines 18 - 24, The session-count
aggregation is including daemon "team-lead" rows written by saveTeamLeadEntry(),
so update the jq pipeline that produces worker_data to exclude entries where
.value.role == "team-lead" before grouping by session; specifically modify the
pipeline starting from ".workers // {} | to_entries" to filter out team-lead
entries (e.g., using map(select(.value.role != "team-lead"))) then continue with
group_by(.value.session) and the existing count logic so only real worker tasks
are counted.
| @@ -0,0 +1,138 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
❓ Verification inconclusive
Script executed:
# First, check if the file exists and get its line count
wc -l scripts/tmux/genie-tasks.shRepository: automagik-dev/genie
Repository: automagik-dev/genie
Exit code: 0
stdout:
138 scripts/tmux/genie-tasks.sh
Script executed:
# Read the entire file to verify the shebang, lines 86-87, and any version checks
cat -n scripts/tmux/genie-tasks.shRepository: automagik-dev/genie
Repository: automagik-dev/genie
Exit code: 0
stdout:
1 #!/usr/bin/env bash
2 # Genie TUI — task window tabs for tmux bottom status bar
3 # Reads workers.json for agent counts + status emoji per window.
4 #
5 # Usage: genie-tasks.sh <session_name>
6 #
7 # Output format per window:
8 # Active: #[bg=#7b2ff7,fg=#e0e0e0,bold] name ×count emoji #[bg=#16213e,fg=#7b2ff7]
9 # Inactive: #[fg=#b8a9c9,bg=#16213e] name ×count emoji
10 #
11 # Env: GENIE_WORKERS — override path to workers.json (for testing)
12
13 set -euo pipefail
14
15 session_name="${1:-}"
16 if [[ -z "$session_name" ]]; then
17 echo -n ""
18 exit 0
19 fi
20
21 # Verify session exists (skip check when running with mock data and no tmux)
22 has_tmux=true
23 if ! command -v tmux &>/dev/null; then
24 has_tmux=false
25 elif ! tmux has-session -t "$session_name" 2>/dev/null; then
26 # Session doesn't exist in tmux — but if we have workers data, still render
27 has_tmux=false
28 fi
29
30 workers_file="${GENIE_WORKERS:-${HOME}/.genie/workers.json}"
31
32 # If workers file doesn't exist, fall back to plain tmux window list
33 if [[ ! -f "$workers_file" ]]; then
34 if [[ "$has_tmux" == "false" ]]; then
35 echo -n ""
36 exit 0
37 fi
38 # Fallback: plain window list without agent data
39 output=""
40 while IFS=$'\t' read -r window_index window_name window_active; do
41 [[ -z "$window_index" ]] && continue
42 if [[ "$window_active" == "1" ]]; then
43 output+="#[bg=#7b2ff7,fg=#e0e0e0,bold] ${window_index}:${window_name} #[bg=#16213e,fg=#7b2ff7] "
44 else
45 output+="#[fg=#b8a9c9,bg=#16213e] ${window_index}:${window_name} "
46 fi
47 done < <(tmux list-windows -t "$session_name" -F "#{window_index} #{window_name} #{window_active}" 2>/dev/null)
48 echo -n "$output"
49 exit 0
50 fi
51
52 # State priority for worst-state-wins aggregation (higher = worse)
53 # error > permission > working > spawning > idle > done > suspended
54 #
55 # Single jq pass: filter workers by session, group by windowName,
56 # compute count + aggregate state per window.
57 # Output: tab-separated lines: windowName\tcount\tworst_state
58 worker_data=$(jq -r --arg sess "$session_name" '
59 # State priority mapping
60 def state_priority:
61 {"error": 7, "permission": 6, "working": 5, "spawning": 4, "idle": 3, "done": 2, "suspended": 1};
62
63 # Emoji mapping
64 def state_emoji:
65 {"spawning": "⏳", "working": "🔨", "idle": "⏸", "done": "✓", "error": "✗", "permission": "❓", "suspended": "💤"};
66
67 .workers // {} | to_entries
68 | map(select(.value.session == $sess))
69 | group_by(.value.windowName // .value.team // "unknown")
70 | map({
71 window: (.[0].value.windowName // .[0].value.team // "unknown"),
72 count: length,
73 worst_state: (
74 map(.value.state // "idle")
75 | map(. as $s | state_priority[$s] // 0)
76 | max
77 | . as $max_pri
78 | state_priority | to_entries | map(select(.value == $max_pri)) | .[0].key // "idle"
79 ),
80 })
81 | map(.window + "\t" + (.count | tostring) + "\t" + (state_emoji[.worst_state] // ""))
82 | .[]
83 ' "$workers_file" 2>/dev/null) || worker_data=""
84
85 # Build lookup table from worker data (avoid subshells in loop)
86 declare -A window_counts=()
87 declare -A window_emojis=()
88
89 if [[ -n "$worker_data" ]]; then
90 while IFS=$'\t' read -r wname wcount wemoji; do
91 [[ -z "$wname" ]] && continue
92 window_counts["$wname"]="$wcount"
93 window_emojis["$wname"]="$wemoji"
94 done <<< "$worker_data"
95 fi
96
97 # If tmux is available, render with live window info
98 if [[ "$has_tmux" == "true" ]]; then
99 output=""
100 while IFS=$'\t' read -r window_index window_name window_active; do
101 [[ -z "$window_index" ]] && continue
102
103 count="${window_counts[$window_name]:-}"
104 emoji="${window_emojis[$window_name]:-}"
105
106 # Build enriched label: name ×count emoji
107 label="${window_name}"
108 if [[ -n "$count" && "$count" -gt 0 ]]; then
109 label+=" ×${count}"
110 fi
111 if [[ -n "$emoji" ]]; then
112 label+=" ${emoji}"
113 fi
114
115 if [[ "$window_active" == "1" ]]; then
116 output+="#[bg=#7b2ff7,fg=#e0e0e0,bold] ${label} #[bg=#16213e,fg=#7b2ff7] "
117 else
118 output+="#[fg=#b8a9c9,bg=#16213e] ${label} "
119 fi
120 done < <(tmux list-windows -t "$session_name" -F "#{window_index} #{window_name} #{window_active}" 2>/dev/null)
121 echo -n "$output"
122 else
123 # No tmux available (testing mode): render from workers.json data only
124 output=""
125 for wname in "${!window_counts[@]}"; do
126 count="${window_counts[$wname]}"
127 emoji="${window_emojis[$wname]:-}"
128 label="${wname}"
129 if [[ -n "$count" && "$count" -gt 0 ]]; then
130 label+=" ×${count}"
131 fi
132 if [[ -n "$emoji" ]]; then
133 label+=" ${emoji}"
134 fi
135 output+="#[fg=#b8a9c9,bg=#16213e] ${label} "
136 done
137 echo -n "$output"
138 fi
Add Bash 4+ version check or declare the dependency explicitly.
This script uses associative arrays (declare -A at lines 86–87, with subscript expansion at lines 103–104, 125–126) which require Bash 4.0+, but the shebang #!/usr/bin/env bash does not enforce a minimum version. The script will fail outright on Bash 3.x with no warning. Either add an explicit version check at the start (e.g., if [[ ${BASH_VERSINFO[0]} -lt 4 ]]; then echo "Bash 4+ required"; exit 1; fi) or update the shebang to reflect the actual requirement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/tmux/genie-tasks.sh` at line 1, The script uses associative arrays
via "declare -A" (e.g., the associative arrays created with declare -A and
subscript expansion) which requires Bash 4+, so add an explicit runtime version
guard at the top: check BASH_VERSINFO[0] and exit with a clear message if it's
less than 4 (or alternatively change the shebang to a stricter interpreter that
guarantees Bash 4+); ensure the check runs before any use of declare -A or
associative subscripts so the script fails fast with a helpful error when
running under Bash 3.x.
Comment on lines
+58
to
+83
| worker_data=$(jq -r --arg sess "$session_name" ' | ||
| # State priority mapping | ||
| def state_priority: | ||
| {"error": 7, "permission": 6, "working": 5, "spawning": 4, "idle": 3, "done": 2, "suspended": 1}; | ||
|
|
||
| # Emoji mapping | ||
| def state_emoji: | ||
| {"spawning": "⏳", "working": "🔨", "idle": "⏸", "done": "✓", "error": "✗", "permission": "❓", "suspended": "💤"}; | ||
|
|
||
| .workers // {} | to_entries | ||
| | map(select(.value.session == $sess)) | ||
| | group_by(.value.windowName // .value.team // "unknown") | ||
| | map({ | ||
| window: (.[0].value.windowName // .[0].value.team // "unknown"), | ||
| count: length, | ||
| worst_state: ( | ||
| map(.value.state // "idle") | ||
| | map(. as $s | state_priority[$s] // 0) | ||
| | max | ||
| | . as $max_pri | ||
| | state_priority | to_entries | map(select(.value == $max_pri)) | .[0].key // "idle" | ||
| ), | ||
| }) | ||
| | map(.window + "\t" + (.count | tostring) + "\t" + (state_emoji[.worst_state] // "")) | ||
| | .[] | ||
| ' "$workers_file" 2>/dev/null) || worker_data="" |
There was a problem hiding this comment.
Handle the full registry schema here.
AgentState includes question, and the registry type also allows window as an alias of windowName (src/lib/agent-registry.ts:1-90). This query ignores both, so question-blocked agents lose their emoji and any window-only entry will not join the live tmux window name when the bar is rendered.
Suggested fix
- def state_priority:
- {"error": 7, "permission": 6, "working": 5, "spawning": 4, "idle": 3, "done": 2, "suspended": 1};
+ def state_priority:
+ {"error": 8, "permission": 7, "question": 6, "working": 5, "spawning": 4, "idle": 3, "done": 2, "suspended": 1};
- def state_emoji:
- {"spawning": "⏳", "working": "🔨", "idle": "⏸", "done": "✓", "error": "✗", "permission": "❓", "suspended": "💤"};
+ def state_emoji:
+ {"spawning": "⏳", "working": "🔨", "idle": "⏸", "done": "✓", "error": "✗", "permission": "❓", "question": "❔", "suspended": "💤"};
- | group_by(.value.windowName // .value.team // "unknown")
+ | group_by(.value.windowName // .value.window // .value.team // "unknown")
| map({
- window: (.[0].value.windowName // .[0].value.team // "unknown"),
+ window: (.[0].value.windowName // .[0].value.window // .[0].value.team // "unknown"),📝 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
| worker_data=$(jq -r --arg sess "$session_name" ' | |
| # State priority mapping | |
| def state_priority: | |
| {"error": 7, "permission": 6, "working": 5, "spawning": 4, "idle": 3, "done": 2, "suspended": 1}; | |
| # Emoji mapping | |
| def state_emoji: | |
| {"spawning": "⏳", "working": "🔨", "idle": "⏸", "done": "✓", "error": "✗", "permission": "❓", "suspended": "💤"}; | |
| .workers // {} | to_entries | |
| | map(select(.value.session == $sess)) | |
| | group_by(.value.windowName // .value.team // "unknown") | |
| | map({ | |
| window: (.[0].value.windowName // .[0].value.team // "unknown"), | |
| count: length, | |
| worst_state: ( | |
| map(.value.state // "idle") | |
| | map(. as $s | state_priority[$s] // 0) | |
| | max | |
| | . as $max_pri | |
| | state_priority | to_entries | map(select(.value == $max_pri)) | .[0].key // "idle" | |
| ), | |
| }) | |
| | map(.window + "\t" + (.count | tostring) + "\t" + (state_emoji[.worst_state] // "")) | |
| | .[] | |
| ' "$workers_file" 2>/dev/null) || worker_data="" | |
| worker_data=$(jq -r --arg sess "$session_name" ' | |
| # State priority mapping | |
| def state_priority: | |
| {"error": 8, "permission": 7, "question": 6, "working": 5, "spawning": 4, "idle": 3, "done": 2, "suspended": 1}; | |
| # Emoji mapping | |
| def state_emoji: | |
| {"spawning": "⏳", "working": "🔨", "idle": "⏸", "done": "✓", "error": "✗", "permission": "❓", "question": "❔", "suspended": "💤"}; | |
| .workers // {} | to_entries | |
| | map(select(.value.session == $sess)) | |
| | group_by(.value.windowName // .value.window // .value.team // "unknown") | |
| | map({ | |
| window: (.[0].value.windowName // .[0].value.window // .[0].value.team // "unknown"), | |
| count: length, | |
| worst_state: ( | |
| map(.value.state // "idle") | |
| | map(. as $s | state_priority[$s] // 0) | |
| | max | |
| | . as $max_pri | |
| | state_priority | to_entries | map(select(.value == $max_pri)) | .[0].key // "idle" | |
| ), | |
| }) | |
| | map(.window + "\t" + (.count | tostring) + "\t" + (state_emoji[.worst_state] // "")) | |
| | .[] | |
| ' "$workers_file" 2>/dev/null) || worker_data="" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/tmux/genie-tasks.sh` around lines 58 - 83, The jq pipeline ignores
the registry's alternate fields: add support for the AgentState 'question' and
the 'window' alias so agents with question state get an emoji and entries using
window (not windowName) are grouped correctly. Update the state_priority and
state_emoji defs to include "question" (with appropriate priority and emoji),
and change the selectors used where window name and state are read to prefer
.value.window // .value.windowName and to compute worst_state from (.value.state
// .value.question // "idle"); keep the rest of the pipeline intact (look for
the map/group_by that builds window and worst_state in the worker_data jq
expression).
Comment on lines
+97
to
+99
| if-shell 'tmux -V | sed -nE "s/^tmux ([0-9]+)\\.([0-9]+).*/\\1 \\2/" | awk "{exit !($1 > 3 || ($1 == 3 && $2 >= 2))}"' \ | ||
| 'set -g status 2' \ | ||
| 'set -g status on' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let me check the actual file content to understand the context better
cat -n scripts/tmux/genie.tmux.conf | sed -n '95,115p'Repository: automagik-dev/genie
Length of output: 1095
🌐 Web query:
tmux version output format OpenBSD
💡 Result:
The tmux version output format on OpenBSD, obtained by running tmux -V, is a string in the form "openbsd-X.Y" where X.Y corresponds to the OpenBSD release version (e.g., "openbsd-6.8" on OpenBSD 6.8). This differs from portable tmux versions, which output "tmux 3.3a" style numbers. The OpenBSD man page confirms the -V flag "Report the tmux version." As of 2026-03-17, OpenBSD current likely uses a format like "openbsd-7.6" or newer, based on ongoing development.
Citations:
- 1: https://man.openbsd.org/tmux
- 2: Clarify definition of "server version" tmux/tmux#2509
- 3: https://man.openbsd.org/?apropos=0&arch=default&manpath=OpenBSD-7.7&query=tmux&sec=1
- 4: https://man.openbsd.org/cgi-bin/man.cgi/OpenBSD-current/man1/tmux.1
- 5: https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.bin/tmux/
- 6: https://repo.or.cz/tmux-openbsd.git/shortlog
🌐 Web query:
tmux status-format support version history
💡 Result:
tmux status-format support was introduced in tmux 2.9. It is an array option that configures the format of each line in a multi-line status bar (when status is set to 2, 3, 4, or 5). The default status-format references existing status-* options like status-left, window-status-format, and status-right. Prior to 2.9, the status bar was configured using individual options such as status-left, status-right, window-status-format, and window-status-current-format, which supported format expansions (#[...] and #{...}). status-format enables per-line customization for multi-line status bars and full configuration using expanded formats and styles. Examples from documentation: - set -g status 2 - set -g status-format[0] '#[align=left] #{status-left}' - set -g status-format[1] '#[align=centre] #{window-list}' - set -g status-format[2] '#[align=right] #{status-right}' It has been available and unchanged in all subsequent versions up to the latest (3.4+ as of 2024), with ongoing format enhancements like mathematical operations in 3.2.
Citations:
- 1: https://raw.githubusercontent.com/tmux/tmux/2.9/CHANGES
- 2: https://raw.githubusercontent.com/tmux/tmux/3.0/CHANGES
- 3: https://github.com/tmux/tmux/wiki/Formats
- 4: How to use status-format[1] tmux/tmux#1886
- 5: https://man7.org/linux/man-pages/man1/tmux.1.html
Fix the tmux version gate.
tmux -V on OpenBSD reports openbsd-6.x instead of tmux X.Y, so this regex never matches. The sed pattern expects ^tmux but gets openbsd, producing no output to awk. This causes the version check to fail and forces OpenBSD systems into the single-line fallback (set -g status on) even though they support status 2 and status-format[] (available since tmux 2.9).
Parse both version formats or probe the status-format[] capability directly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/tmux/genie.tmux.conf` around lines 97 - 99, The if-shell version
check in genie.tmux.conf uses a sed regex that only matches lines beginning with
"tmux" so OpenBSD's "openbsd-6.x" tmux -V output fails and falls back to 'set -g
status on'; update the if-shell condition (the line containing if-shell 'tmux -V
| sed ... | awk ...') to either (a) broaden the regex to accept the OpenBSD
prefix (match either "tmux X.Y" or "openbsd-* X.Y") or (b) replace the version
parse with a capability probe that checks for the presence of status-format
(i.e., run a tmux query for the status-format capability and use that result to
choose between 'set -g status 2' and 'set -g status on'); modify that exact
if-shell invocation accordingly so OpenBSD tmux instances that support
status-format use the two-line status.
Comment on lines
+19
to
+22
| test('returns false for non-existent pane', async () => { | ||
| // %999999 is very unlikely to exist | ||
| expect(await isPaneAlive('%999999')).toBe(false); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Confirm this test case and the corresponding runtime path.
rg -n "%999999|non-existent pane" src/lib/tmux.test.ts
rg -n "export async function isPaneAlive|capturePaneContent\\(" src/lib/tmux.tsRepository: automagik-dev/genie
Length of output: 461
🏁 Script executed:
# Check isPaneAlive implementation
sed -n '365,375p' src/lib/tmux.ts
# Check capturePaneContent implementation
sed -n '174,190p' src/lib/tmux.ts
# Check test file setup and guards
head -n 30 src/lib/tmux.test.tsRepository: automagik-dev/genie
Length of output: 1660
Move this test case to an integration test or guard with an explicit tmux-available precondition.
The test at lines 19–22 calls isPaneAlive('%999999'), which validates the pane format and then executes capturePaneContent, calling real tmux commands (capture-pane). Unlike the first three test cases (which validate early via format checks), this case is environment-coupled and will fail if tmux is unavailable. Unit tests should not depend on external services.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/tmux.test.ts` around lines 19 - 22, The test "returns false for
non-existent pane" calls isPaneAlive('%999999') which proceeds to call
capturePaneContent and invokes real tmux commands; move this case out of the
unit test into an integration test suite or add an explicit tmux-available guard
so it is skipped when tmux isn't present. Locate the test in
src/lib/tmux.test.ts and either relocate it to your integration tests or wrap it
with a precondition that checks tmux availability (e.g., a helper
isTmuxAvailable or executing a lightweight 'tmux' check) before calling
isPaneAlive or else mock capturePaneContent so the unit test does not execute
real tmux commands. Ensure the test name and behavior remain the same when
running under the integration environment.
Comment on lines
+491
to
499
| async function resolveSpawnTeamWindow( | ||
| team: string | undefined, | ||
| cwd: string, | ||
| session: string, | ||
| ): Promise<TeamWindowInfo | null> { | ||
| if (!team) return null; | ||
| try { | ||
| return await tmux.ensureTeamWindow('genie', team, cwd); | ||
| return await tmux.ensureTeamWindow(session, team, cwd); | ||
| } catch (err) { |
There was a problem hiding this comment.
Sanitize the tmux window name before ensureTeamWindow().
Line 498 still passes the raw team name into tmux window creation. A team like ops.bot recreates the session:window.pane parsing bug fixed in src/genie-commands/session.ts, so window resolution/layout can fail or hit the wrong pane. Keep the logical team name raw, but sanitize the tmux target first.
Based on learnings, "Apply sanitizeWindowName() from session.ts to teamName wherever it is passed to ensureTeamWindow or used to build tmux target strings. This prevents dots in teamName from being interpreted as pane separators (pane-like paths) and causing 'can't find pane' errors, mirroring the fix already implemented in session.ts."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/term-commands/agents.ts` around lines 491 - 499, The
resolveSpawnTeamWindow function currently passes the raw team name into
tmux.ensureTeamWindow which can break when team contains dots; sanitize the tmux
window name before calling ensureTeamWindow by importing and applying the
sanitizeWindowName function (from session.ts) to the team value used in tmux
targets while keeping the original logical team variable untouched, i.e., call
tmux.ensureTeamWindow(session, sanitizeWindowName(team), cwd) (or otherwise pass
the sanitized name into any tmux target-building logic) so dots are not
interpreted as pane separators.
Comment on lines
831
to
833
| spawnIntoCurrentWindow: !teamWasExplicit && insideTmux, | ||
| session: process.env.GENIE_SESSION ?? 'genie', | ||
| }; |
There was a problem hiding this comment.
Do not default interactive tmux spawns to 'genie'.
Lines 831-833 assume the current shell already exports GENIE_SESSION, but src/genie-commands/session.ts Lines 267-268 and 299-300 only write that value via tmux environment updates. The existing pane running genie spawn usually will not see that change, so per-project spawns can be created and registered under the wrong session. Resolve the session from tmux or cwd here instead of hardcoding 'genie'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/term-commands/agents.ts` around lines 831 - 833, The code is defaulting
interactive tmux spawns to session: process.env.GENIE_SESSION ?? 'genie', which
is incorrect because the current pane may not have GENIE_SESSION set; instead
resolve the actual tmux session (or derive it from cwd) before falling back.
Replace the hardcoded fallback with a call to the tmux/session resolver used
elsewhere (e.g., reuse the session resolution logic from the session module or a
helper like getTmuxSession/getSessionFromCwd) and assign that result to the
session field (keep spawnIntoCurrentWindow, teamWasExplicit and insideTmux logic
intact); ensure the lookup queries tmux for the session name when insideTmux and
only fall back to a safe default if resolution fails.
Comment on lines
+15
to
+29
| test('returns false for file without genie marker', () => { | ||
| const { writeFileSync } = require('node:fs'); | ||
| const path = `/tmp/shortcuts-test-${Date.now()}.txt`; | ||
| writeFileSync(path, 'some content without markers\n'); | ||
| expect(isShortcutsInstalled(path)).toBe(false); | ||
| require('node:fs').unlinkSync(path); | ||
| }); | ||
|
|
||
| test('returns true for file with genie marker', () => { | ||
| const { writeFileSync } = require('node:fs'); | ||
| const path = `/tmp/shortcuts-test-${Date.now()}.txt`; | ||
| writeFileSync(path, '# generated by genie-cli\nbind-key stuff\n'); | ||
| expect(isShortcutsInstalled(path)).toBe(true); | ||
| require('node:fs').unlinkSync(path); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use afterEach cleanup and consistent imports.
Two issues:
- Inline
require('node:fs')is inconsistent with the ES module import on line 2. Import at the top instead. - Manual
unlinkSyncin each test violates the guideline to "use tmpdir with cleanup in afterEach". If an assertion fails before cleanup, temp files remain.
♻️ Proposed fix
-import { describe, expect, test } from 'bun:test';
+import { afterEach, describe, expect, test } from 'bun:test';
+import { mkdtempSync, rmSync, writeFileSync } from 'node:fs';
+import { tmpdir } from 'node:os';
+import { join } from 'node:path';
import { displayShortcuts, isShortcutsInstalled } from './shortcuts.js';
describe('displayShortcuts', () => {
test('does not throw', () => {
expect(() => displayShortcuts()).not.toThrow();
});
});
describe('isShortcutsInstalled', () => {
+ let tempDir: string;
+
+ afterEach(() => {
+ if (tempDir) {
+ rmSync(tempDir, { recursive: true, force: true });
+ }
+ });
+
test('returns false for non-existent file', () => {
expect(isShortcutsInstalled('/tmp/nonexistent-file-12345')).toBe(false);
});
test('returns false for file without genie marker', () => {
- const { writeFileSync } = require('node:fs');
- const path = `/tmp/shortcuts-test-${Date.now()}.txt`;
+ tempDir = mkdtempSync(join(tmpdir(), 'shortcuts-test-'));
+ const path = join(tempDir, 'test.txt');
writeFileSync(path, 'some content without markers\n');
expect(isShortcutsInstalled(path)).toBe(false);
- require('node:fs').unlinkSync(path);
});
test('returns true for file with genie marker', () => {
- const { writeFileSync } = require('node:fs');
- const path = `/tmp/shortcuts-test-${Date.now()}.txt`;
+ tempDir = mkdtempSync(join(tmpdir(), 'shortcuts-test-'));
+ const path = join(tempDir, 'test.txt');
writeFileSync(path, '# generated by genie-cli\nbind-key stuff\n');
expect(isShortcutsInstalled(path)).toBe(true);
- require('node:fs').unlinkSync(path);
});
});As per coding guidelines: "Use tmpdir with cleanup in afterEach for test fixtures"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/term-commands/shortcuts.test.ts` around lines 15 - 29, Replace inline
require('node:fs') calls with a single ES module import at the top (e.g., import
{ writeFileSync, unlinkSync } from 'node:fs') and stop manually unlinking inside
each test; instead create temp files using a tmpdir/mkdtemp-based filename per
test and add an afterEach that unlinks any file created by the tests. Update the
two tests that call isShortcutsInstalled to use the shared tmp filename variable
and rely on afterEach cleanup so failures don’t leak files, and keep the
isShortcutsInstalled references unchanged.
Comment on lines
+56
to
+63
| test('migrates legacy claudio launcher to claude', () => { | ||
| const profile = WorkerProfileSchema.parse({ | ||
| launcher: 'claudio', | ||
| claudeArgs: [], | ||
| }); | ||
| expect(profile.launcher).toBe('claude'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Add a rejection test for invalid launcher values.
Current coverage validates success and migration paths, but not the failure path. Add a test that asserts non-claude values are rejected, so schema strictness is locked.
Proposed test addition
describe('WorkerProfileSchema', () => {
@@
test('migrates legacy claudio launcher to claude', () => {
@@
expect(profile.launcher).toBe('claude');
});
+
+ test('rejects unsupported launcher values', () => {
+ expect(() =>
+ WorkerProfileSchema.parse({
+ launcher: 'other',
+ claudeArgs: [],
+ }),
+ ).toThrow();
+ });
});📝 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
| test('migrates legacy claudio launcher to claude', () => { | |
| const profile = WorkerProfileSchema.parse({ | |
| launcher: 'claudio', | |
| claudeArgs: [], | |
| }); | |
| expect(profile.launcher).toBe('claude'); | |
| }); | |
| }); | |
| test('migrates legacy claudio launcher to claude', () => { | |
| const profile = WorkerProfileSchema.parse({ | |
| launcher: 'claudio', | |
| claudeArgs: [], | |
| }); | |
| expect(profile.launcher).toBe('claude'); | |
| }); | |
| test('rejects unsupported launcher values', () => { | |
| expect(() => | |
| WorkerProfileSchema.parse({ | |
| launcher: 'other', | |
| claudeArgs: [], | |
| }), | |
| ).toThrow(); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/types/genie-config.test.ts` around lines 56 - 63, Add a new test named
like "rejects invalid launcher values" that calls WorkerProfileSchema.parse with
an invalid launcher string (e.g., launcher: 'not-a-launcher', claudeArgs: [])
and asserts that parse throws (use expect(() =>
WorkerProfileSchema.parse(...)).toThrow() or toThrowError()); this ensures
WorkerProfileSchema.parse rejects non-'claude' launcher values and locks schema
strictness.
bash -e causes the script to exit immediately when bun test --coverage returns non-zero, before the output can be printed or parsed. Adding || true lets the script capture output regardless of exit code.
1 task
fix(ci): prevent set -e from aborting coverage capture
4 tasks done
namastex888
added a commit
that referenced
this pull request
Mar 18, 2026
revert: undo PR #644 promotion (broken tmux-split-tabbar)
4 tasks
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
2 participants
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
Tests
Chores