fix: v2 QA fixes — 12 bugs, 7 groups, 708 tests pass - #560
Conversation
… version Group 1: Extract shared file-lock utility (src/lib/file-lock.ts) - Deduplicate lock pattern from agent-directory, wish-state, agent-registry - All 3 modules now import from file-lock.ts Group 4: Spawn & team fixes - Fix spawn CWD to use team worktree path for built-in agents (#546) - Add validateBranchName() in team-manager (#551) - Make parseWishGroups() case-insensitive (#554) Group 5: Global team configs (#558) - Move team configs from <repo>/.genie/teams/ to ~/.genie/teams/ - Drop repoPath param from getTeam/listTeams/listMembers - All team commands resolve repo from stored config, not CWD - Update all callers across codebase Group 6: Fix genie update (#559) - Read version from package.json at runtime instead of hardcoded 3 test failures remain (team-manager signature changes in test mocks) Groups 2 (concurrency) and 3 (wish state hardening) still pending. Co-Authored-By: Paperclip <noreply@paperclip.ing>
- Fix 3 failing tests: isolate GENIE_HOME in team.test.ts and msg.test.ts so team manager reads from test-local directories - Add file-lock concurrency protection to mailbox.send(), mailbox.markDelivered(), and team-chat.postMessage() - Add wish-state validation: self-dep, dangling dep, and cycle detection (Kahn's algorithm) in createState() - Enforce strict completeGroup() transitions (must be in_progress) - Add resetGroup() to revert in_progress groups back to ready - Add JSDoc to getState() documenting lockless-by-design reads - Add reset CLI command in state.ts - Add comprehensive tests for validation and resetGroup
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 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 |
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 addresses 12 identified bugs and design issues from the v2 redesign, significantly enhancing the stability, robustness, and usability of the system. The changes focus on improving concurrent data access through shared file locking, strengthening wish state management with advanced validation and recovery mechanisms, and refining team and agent interactions by centralizing configurations and improving command-line flexibility. These fixes ensure a more reliable and consistent user experience across various core functionalities. Highlights
Changelog
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}
There was a problem hiding this comment.
Code Review
This is an impressive pull request that addresses a wide range of bugs and makes significant improvements to the system's robustness and maintainability. The extraction of the file locking mechanism into a shared module is a great refactoring. The addition of concurrency controls, state machine hardening with cycle detection, and moving team configurations to be global are all excellent changes. The code is well-structured and includes thorough tests for the new functionality. I have one critical concern regarding the new file lock implementation, which could lead to race conditions.
| if (Date.now() > deadline) { | ||
| try { | ||
| await unlink(lockPath); | ||
| } catch { | ||
| throw new Error(`Lock timeout: could not remove stale lock at ${lockPath}`); | ||
| } | ||
| continue; | ||
| } |
There was a problem hiding this comment.
This timeout logic introduces a critical race condition. Forcefully unlinking a lock file simply because this process has timed out waiting for it can lead to a state where two processes believe they hold the lock, potentially causing data corruption. If another process holds the lock for a legitimate, long-running operation, this code will break that lock and proceed, violating the mutual exclusion guarantee.
The safer approach is to simply throw an error when the timeout is exceeded, and rely on the existing stale lock cleanup (tryCleanStaleLock) to handle locks left behind by dead processes.
if (Date.now() > deadline) {
throw new Error(`Lock timeout: could not acquire lock for ${lockPath} within ${LOCK_TIMEOUT_MS}ms.`);
}![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4fd5fc220a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const filePath = teamFilePath(name); | ||
| if (existsSync(filePath)) { | ||
| const content = await readFile(filePath, 'utf-8'); | ||
| return JSON.parse(content); |
There was a problem hiding this comment.
createTeam now treats a team name as globally unique by checking only teamFilePath(name) under ~/.genie/teams, so creating feat/foo in repo B after repo A will return repo A's stored config instead of creating a new worktree for repo B. This makes subsequent hire/fire/disband operations act on the wrong repository whenever branch-style team names are reused across repos, which is a common workflow.
Useful? React with 👍 / 👎.
|
|
||
| const teamManager = await getTeamManager(); | ||
| const teams = await teamManager.listTeams(repoPath); | ||
| const teams = await teamManager.listTeams(); |
There was a problem hiding this comment.
checkSendScope still receives repoPath from genie send, but it now ignores it and evaluates membership against listTeams() globally. If an agent name exists in a team from another repo, sends from the current repo can be incorrectly rejected with a scope violation even though no local team relationship exists.
Useful? React with 👍 / 👎.
| // Try paths relative to this module | ||
| const candidates = [ | ||
| // From src/lib/version.ts → ../../package.json | ||
| resolve(dirname(import.meta.dir ?? __dirname), '..', '..', 'package.json'), |
There was a problem hiding this comment.
The first candidate path uses dirname(import.meta.dir) before walking ../.., which overshoots by one directory in source mode and can select a parent workspace package.json instead of this project's file. In that setup, VERSION reports the outer package version, so the CLI can display an unrelated version after update.
Useful? React with 👍 / 👎.
Add build + publish steps to version.yml so dev merges publish to npm under the `next` dist-tag. Install with: bun add -g @automagik/genie@next Co-Authored-By: Paperclip <noreply@paperclip.ing>
genie update --next Switch to dev builds (@next npm tag) genie update --stable Switch to stable releases (@latest npm tag) genie update Uses last selected channel (default: latest) Channel preference persisted in ~/.genie/config.json (updateChannel field). Co-Authored-By: Paperclip <noreply@paperclip.ing>
1 participant
Summary
Fixes 12 bugs (#546-#555, #558, #559) found during comprehensive QA of the v2 redesign. Zero plan changes — all fixes match the original v2 spec.
Changes by Group
Group 1: Shared File Lock Extraction (#550)
src/lib/file-lock.ts— deduplicated from 3 identical copiesGroup 2: Concurrency Fixes (#547, #555)
mailbox.send()andmarkDelivered()— was losing 9/10 messages under concurrent writesteam-chat.postMessage()Group 3: Wish State Hardening (#548, #549, #552, #553)
createState()via Kahn's algorithmcompleteGroup()requiresin_progressstateresetGroup()+genie reset slug#groupCLI for leader recoverygetState()documenting lockless readsGroup 4: Spawn & Team Fixes (#546, #551, #554)
validateBranchName()rejects git-unsafe team namesparseWishGroups()now case-insensitiveGroup 5: Global Team Configs (#558)
<repo>/.genie/teams/to~/.genie/teams/process.cwd()dependencygetTeam(),listTeams(),listMembers()no longer take repoPath paramGroup 6: Fix genie update (#559)
Issues Fixed
Test plan
bun run typecheck— passesbun run lint— passes (5 pre-existing warnings)bun test— 708 pass, 0 failbun run build— succeeds