fix: resolve 27 findings from merged PR reviews - #106
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
૮ >ﻌ< ა ci reviewran on 63540a6 — Merge branch 'main' into codex/merged-review-repairs-2026091
|
![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: a2fbcdabdc
ℹ️ 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".
- plugin_validate.py: accept re-exported register() via ImportFrom; exclude test/tests/_test/_tests dirs from capability scanning - run_profile_reconcile.py: move evict_profile_plugins() off the gateway event loop via asyncio.to_thread (importlib locks) - git_probe.py: fold --separate-git-dir worktrees into one repo identity by recognizing the worktrees/ segment of --git-common-dir Co-authored-by: mrkillbob <mrkillbob@users.noreply.github.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f184138d9a
ℹ️ 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".
| from hermes_cli.plugins_loader import _plugin_home_scope | ||
|
|
||
| key = home.expanduser().resolve() | ||
| with plugins._plugin_managers_lock: |
There was a problem hiding this comment.
When a deleted profile's on_unload callback blocks, this worker holds _plugin_managers_lock throughout manager.unload(). Gateway hook paths synchronously call get_plugin_manager(), which needs the same process-wide lock, so the event-loop thread can still block and message delivery for unrelated profiles can stall indefinitely. Fresh evidence in this revision is that teardown was moved off-loop while the newly added lifecycle helper keeps the global lock around the unbounded callback execution.
Useful? React with 👍 / 👎.
| for service in desired_services: | ||
| if service not in current_services: | ||
| _service_op(*service, "install", default_home) | ||
| _service_op(*service, "start", default_home) |
There was a problem hiding this comment.
When both user and system service files exist for the default profile but only one was active before migration, _installed_services() records both without their running state and this rollback loop starts both. An explicit rollback or failed automatic migration can therefore launch two gateways for the same profile, causing credential/port contention and leaving the fleet unlike its pre-migration state; the manifest needs to preserve service activity or rollback must select the previously active scope.
Useful? React with 👍 / 👎.
| # directory exists. Recover only a provably abandoned reservation and | ||
| # retry the atomic claim once; never remove a live owner's lock. | ||
| if profile_dir.is_dir() or not _federation_seed_reservation_is_stale(profile_dir): | ||
| if (profile_dir.is_dir() and not refresh) or not _federation_seed_reservation_is_stale(profile_dir): |
There was a problem hiding this comment.
When two --refresh-existing seeders concurrently encounter the same stale reservation, both can pass this stale check; after the first process unlinks and recreates the lock, the second can unlink that newly live reservation before performing its own O_EXCL create. Both processes may then believe they own the refresh, and their config, identity, skill-sync, snapshot rollback, and final lock removal operations can interleave. Reclaiming a stale lock needs to verify that the inode or reservation payload being removed is still the one that was inspected.
Useful? React with 👍 / 👎.
- Fix import sort order in voice-live.test.ts (add blank line between vitest and local import) - Fix import sort order in kanban/api.source.test.ts (nanostores before vitest) - Auto-fix padding-line-between-statements warnings via eslint --fix - Fix react-hooks/exhaustive-deps: add missing VAULT_QUERY_KEY/VAULT_SOURCES_QUERY_KEY deps - Remove unnecessary state?.phase dep in build.tsx useMemo
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
An empty or unparseable .federation_seed.lock file has no PID and therefore no live owner. Treat it as stale immediately instead of waiting out the 300s age floor, so seed_federation with refresh_existing=True can reclaim a half-written lock left by a crashed process. Fixes test_seed_refresh_repairs_incomplete_profile_and_clears_artifacts.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
mrkillbobbot
added
area/automation
2 participants
Repairs the 27 unresolved findings left on merged PRs #99, #101, #97, #91, and #88.
Validation: 352 tests passed across all 18 changed Python test files through scripts/run_tests.sh; targeted desktop Vitest coverage passes (46 tests across seven files), including five regressions verified red on the original base. Additional worktree/Projects integration coverage passed. Linux/Windows-only checks require their hosted lanes. Full desktop TypeScript checking is blocked by existing missing Three.js declarations in world-glb-scene.tsx in the shared dependency installation; no changed-file errors were reported.