fix: P1 review findings from the PR-feedback stack merges - #76
Conversation
…e at call time _media_delivery_allowed_roots() only ever returned cache roots from MEDIA_DELIVERY_SAFE_ROOTS, a module-level constant frozen at import time from _HERMES_HOME = get_hermes_home() -- a profile switch after this module's import left the PREVIOUS profile's cache dirs as the only allowlisted media-delivery destination, silently failing delivery for anything the new active profile generated. Added _active_profile_cache_roots(), calling get_hermes_home() at request time instead of relying on the import-time snapshot, and included it in _media_delivery_allowed_roots()'s return list alongside the existing static/per-profile/kanban/operator sources. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…r bootstrap step hermes_bootstrap.py's own trailing comment says "entry points only need import hermes_bootstrap first", and every other bootstrap function (apply_windows_utf8_bootstrap, suppress_platform_ver_console, activate_durable_lazy_target) is applied there -- but harden_import_path() was left out, so a foreign utils.py in the launch cwd could still shadow Hermes's own utils module for any importer that didn't explicitly call it (only 3 of the 4+ entry points did). Idempotent (repositions the same root each call), so the 3 existing explicit call sites are harmless now-redundant belt-and-braces. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…telegram topic test The test's switch_session mock fixture and assertion used task_owned_cwd=/conversation_worktree= -- a signature switch_session() never has on this branch (gateway/session.py's actual signature is conversation_kind=/persisted_cwd=). The production call site (GatewayRunner._process_handoff) already calls it correctly; only the test's stale kwargs needed updating. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…id, not 0 stream_events() defaulted an absent `since` query param to 0 via _int_param(), so a fresh Desktop WebSocket connection replayed the ENTIRE task_events ledger on connect instead of streaming only new events. Added _EventTail.baseline() (SELECT MAX(id)) and use it as the starting cursor when `since` is absent; an explicit `since` (including "0", a client deliberately resuming from the start) is still honored. Moved the cursor resolution inside the existing try/except so a cancellation during baselining is handled the same way as cancellation during a normal poll. The test's own connect mock patched the wrong module attribute (kanban_db.connect, a deprecated compat forwarding shim, instead of kbc.connect -- the reference _EventTail actually calls, matching every other test in this file); fixed to match. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
hermes_state_holders.py's module split (sqlite_sidecar_identity(), deleted_sqlite_sidecar_holders()) duplicated hermes_state_dbfile.py's older _stat_sqlite_sidecar_identity()/iter_deleted_sqlite_sidecar_holders() -- the same /proc/<pid>/fd deleted-WAL scan and os.stat-based identity snapshot, reimplemented rather than reused, with the newer functions never wired into SessionDB's actual open path (refuse_deleted_wal_generation(), called before every sqlite3.connect() specifically to stop a second opener from minting a replacement WAL inode over a still-held deleted generation). Verified equivalence before delegating: hermes_state_holders. sqlite_sidecar_identity()'s inline os.stat + dev/ino-truthy check is the same logic as hermes_state_common.stat_db_file_identity() (which the old function called per-suffix), and deleted_sqlite_sidecar_holders(include_self=True) (the default) matches iter_deleted_sqlite_sidecar_holders()'s always-include-self contract exactly. Made the two hermes_state_dbfile.py functions (kept, since iter_deleted_sqlite_sidecar_holders is an external plugin-compat pointer target) delegate to hermes_state_holders instead of reimplementing the scan, so SessionDB's real open path now exercises the newer module and there's one scan implementation instead of two that could silently drift apart. No production callers changed their observable behavior; verified against the full hermes_state and zeroed_state_db suites plus real-SessionDB-opening smoke tests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…to ready block_task() always parked a needs_input block behind the human-only blocked/triage lanes, even for legacy github-pr-feedback intake cards using needs_input to mean "start validation" -- now role-owned machine work per recompute_ready's auto_triage docstring. That recovery path only fires once a task is already stuck in triage (after hitting the block-loop recurrence limit), so a first-time block never reached it. Wire the same _is_machine_recoverable_pr_feedback_triage() check into block_task() itself: when it matches, skip normal block/triage routing and return the task straight to ready with a machine_handoff event, so the dispatcher can retry it immediately instead of waiting on a human. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…eason _should_skip_fallback_candidate() and try_activate_fallback() accepted a FailoverReason but never special-cased the two reasons that need it: - egress_policy_blocked: the local privacy firewall already rejected the outbound payload, so retrying the same request against another remote provider cannot succeed and only produces noisy false provider failures. _fallback_destination_class()/classify_destination() already existed to answer "is this candidate local?" but nothing called them; now egress-blocked failover skips any non-local (non-loopback, non-local-process) candidate and only activates a local fallback. - unsupported_thinking: the selected model itself lacks thinking capability, a property of the model config a remote fallback chain can't fix, so no candidate is walked at all (matches the classifier's should_fallback=False intent for this specific reason). Also fills in the missing "egress_policy_blocked" -> "local egress policy blocked the request" label in _FALLBACK_REASON_LABELS. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…nt default test_worktree_add_without_base_uses_remote_default failed only in CI, not locally. The fixture's bare `origin.git` was created with plain `git init --bare`, so its HEAD symref followed the runner's ambient init.defaultBranch -- unset on some CI images, where git's legacy built-in default is "master". Since the fixture only ever pushes main/feature, a "master"-pointing HEAD symref is dangling, so `git remote show origin`'s "HEAD branch:" line resolves to "(unknown)" and resolve_worktree_base() falls through to using local HEAD (the just-checked-out parked-feature branch) instead of origin's default branch -- exactly the mismatch the test caught. Pin the bare repo's initial branch to "main" explicitly (matching the clone's own `-b main`) so the fixture no longer depends on the runner's git config. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ckup writes cron/jobs.py: _epoch_file_age() clamped any negative age (stamp in the future) to 0.0, the same value a genuinely fresh heartbeat produces. A restored/replayed ticker_heartbeat file or gross clock skew therefore read as "just ticked" instead of "cannot determine liveness" -- the exact false-positive a liveness check must not produce. Now an age more than 1s in the future (small skew still tolerated) returns None instead of a clamped 0.0. hermes_cli/backup.py: _write_full_zip_backup_locked() (the unattended automatic-backup path) called `list(_iter_backup_files(...))` before opening the archive, materializing the entire home directory tree in memory before writing a single zip entry. Stream the walk generator straight into _write_zip_entries() instead (peeking one entry up front to preserve the empty/no-files-to-back-up short circuit), so each directory's files are archived before the walk descends into the next. The interactive `hermes backup` CLI path (_run_backup_locked) keeps its eager list — it prints an upfront file count for progress UX, which the unattended path has no use for. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ersation
_handle_voice_channel_input() routed every transcript from an allowlisted
fast-lane speaker into the tool-free private lane, with no check on what
the transcript actually said -- an explicit work request spoken into a
fast-lane voice channel ("fix the voice delay", "run the tests and
commit the patch") would silently lose task capability instead of being
acted on, indistinguishable from idle chat ("can you hear me?").
Add _voice_fast_lane_requests_work() (a verb-based heuristic matching
inspect/fix/run/commit/write/edit/... operation verbs) and gate the
fast-lane branch on it: casual conversation still gets a private,
tool-free turn, but an explicit operation keeps full task capability
even from a fast-lane speaker.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…er call
agent/kanban_stop.py defined successful_kanban_terminal_transition() --
matching the current tool batch against a durable {"ok": true}
kanban_complete/kanban_block result -- but nothing in the conversation
loop ever called it. A kanban worker that finished its task therefore
kept looping: run_tool_round() had no way to recognize the terminal
transition, so it fell through to "continue" and burned another API
call after the task was already durably complete.
Wire the check into run_tool_round() right after tool execution/
persistence succeeds (matching the function's own contract: call only
after the executor has persisted every tool-result row). On a match,
end the turn with turn_exit_reason="kanban_terminal_transition" instead
of continuing to the next iteration. final_response is normalized to ""
(not left None) so the finalizer's `completed` computation reads this as
a clean completion rather than a turn stuck mid-flight on a pending tool
result.
tests/run_agent/test_tool_call_incremental_persistence.py: the target
test's fabricated HERMES_KANBAN_TASK/RUN_ID have no real board row, so
_touch_activity's auto-heartbeat/comment-injection bridges
(heartbeat_current_worker_from_env / inject_new_comments_from_env) would
reach for a real kanban board -- either false-positive "lease lost" and
hard-interrupt the turn, or block waiting on a real DB connection.
Stub both bridges to no-ops; they are unrelated to what this test
exercises (kanban_complete tool-call persistence ordering).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![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: fbaa791278
ℹ️ 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".
| "-m", | ||
| "hermes_cli.main", |
There was a problem hiding this comment.
When secure-worker run is invoked from an untrusted checkout containing a hermes_cli package, this python -m child prepends the current directory to sys.path and executes that checkout's hermes_cli.main before the audited configuration takes effect, with the preserved host environment and staging token. I reproduced the import hijack locally, and python3 --help describes -P as preventing this unsafe-path prepend; add safe-path isolation here and to the similarly admitted broker module invocation. The repository also requires security boundaries to be exercised through real imports rather than mocked subprocesses.
AGENTS.md reference: AGENTS.md:L84-L86
Useful? React with 👍 / 👎.
| if isinstance(value, int) and not isinstance(value, bool) and value < 0: | ||
| _issue( | ||
| issues, "error", | ||
| f"code_execution.max_tool_calls is negative ({value!r})", | ||
| "Use zero to disable the limit, or a positive integer to cap tool calls", |
There was a problem hiding this comment.
Fresh evidence after the earlier runtime-validator fix is that this new startup validator still labels every negative code_execution.max_tool_calls value as an error, while cli-config.yaml.example explicitly documents all values <= 0 as unlimited and _configured_max_tool_calls() now accepts them. A valid -1 configuration therefore produces an error on every startup and makes hermes doctor report a configuration failure despite executing correctly; align this validator with the runtime contract.
Useful? React with 👍 / 👎.
| if (!prior || (priorNumber !== null && eventNumber !== null && eventNumber > priorNumber)) { | ||
| next.bySource[scope] = event.id |
There was a problem hiding this comment.
When external snapshot events use the arbitrary string IDs allowed by ExternalWorldEventInput—for example UUIDs—only the first event for a source scope is ever persisted: after prior is set, both numeric conversions are null and this condition can never advance the cursor. On every subsequent world reopen, wasSeen() recognizes only that first exact ID and replays all later snapshot events as new transitions, so opaque-ID sources repeatedly animate and notify stale events.
Useful? React with 👍 / 👎.
| if exc.code == 429: | ||
| retry_after = exc.headers.get("Retry-After") if exc.headers else None | ||
| print(f"\n Rate limited by GitHub — waiting {retry_after or interval}s before retrying...", | ||
| end="", flush=True) | ||
| continue |
There was a problem hiding this comment.
When GitHub responds to the Copilot device-token poll with 429 Retry-After: 120, this branch prints that it will wait 120 seconds but neither sleeps for nor assigns that value; continue returns to the loop's unchanged interval + safety-margin delay, commonly only a few seconds. The added test hides the behavior by making the deadline expire immediately after the first 429, while a real login keeps polling too early and can prolong the rate limit until the overall timeout.
Useful? React with 👍 / 👎.
| if current is not candidate: | ||
| # A candidate may have acquired a root lease before another route | ||
| # won publication. Do not retain an unpublished root. | ||
| self.reconcile_conversation_root_transition(candidate, current) |
There was a problem hiding this comment.
When a route changes while the candidate is being bootstrapped outside the session lock, _bind_conversation_worktree_for_new_entry() has already created a Git worktree and durable database binding, but this losing branch calls reconcile_conversation_root_transition(), which only releases its liveness lease. The same pattern exists for concurrent resets, leaving an unpublished random session root, branch, worktree directory, and binding permanently unreachable through the session cleanup UI despite the comment saying not to retain it.
Useful? React with 👍 / 👎.
| source_venv = repo_root / ".venv" | ||
| if not source_venv.is_dir() or (wt_path / ".venv").exists(): | ||
| return | ||
| try: | ||
| (wt_path / ".venv").symlink_to(source_venv, target_is_directory=True) |
There was a problem hiding this comment.
When the source checkout uses the supported legacy venv/ layout, or its .venv directory lacks the platform's executable, this bespoke helper silently releases the subagent worktree without the required .venv runtime link. The file already imports bootstrap_worktree_environments(), which implements the venv fallback, executable verification, linked-worktree handling, and native Windows interpreter layout, but never calls it; use that helper instead of accepting only repo_root/.venv.
AGENTS.md reference: AGENTS.md:L7-L10
Useful? React with 👍 / 👎.
| // the tree changes; null (no named project) falls back to the cwd leaf below. | ||
| const projectTree = useStore($projectTree) | ||
| const projectName = useMemo(() => projectNameForCwd(currentCwd), [currentCwd, projectTree]) | ||
| const projectName = useMemo(() => projectNameForCwd(currentCwd), [currentCwd]) |
There was a problem hiding this comment.
When the project tree hydrates or a project's membership or name changes while the selected cwd remains constant, this memo does not recompute because projectNameForCwd() reads $projectTree.get() nonreactively and the previous useStore($projectTree) subscription was removed. The status bar therefore keeps displaying the cwd leaf or a stale project name until the cwd changes; restore the tree subscription and dependency so backend project truth repaints the rendering hook.
AGENTS.md reference: AGENTS.md:L299-L302
Useful? React with 👍 / 👎.
…ride ee39240 bumped website/package.json's js-yaml override from 4.3.1 to 4.3.2 (GHSA-2883-xcg3-v3hh) but only refreshed the root/apps/desktop lockfiles -- website/package-lock.json still pinned js-yaml@4.3.1, so `npm ci` failed closed with EUSAGE ("lock file's js-yaml@4.3.1 does not satisfy js-yaml@4.3.2") in the Docs Site CI job. Refreshed via `npm install --package-lock-only` inside website/. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…nts shutdown test_stream_events_closes_connection_when_cancelled failed intermittently in CI (never locally) with an unhandled asyncio.CancelledError surfacing from `await task`, traced to plugin_api.py's `finally: await tail.shutdown()`. A `finally` block's own exceptions are never caught by its enclosing try's `except` clauses -- so when cancelling a task that's mid-await on a `run_in_executor()` future (baseline/poll) races with that future's underlying thread completing, asyncio can deliver a second CancelledError at the very next await checkpoint, landing unguarded inside the `finally`. The existing `except asyncio.CancelledError: return` sibling already treats cancellation as normal shutdown; extend that same treatment to the cleanup call itself. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The prior fix (0478d5e) stopped the crash but left the connection leaking: CI showed close_calls==0 after a cancelled stream. Once task.cancel() has landed on stream_events()'s task (the common path -- shutdown() runs from its `finally`), an internal `await run_in_executor(...)` inside shutdown() can be skipped by asyncio before _close() ever runs, even though the executor thread itself can't actually be interrupted. Replace the await with a blocking `executor.submit(...).result(timeout)` call: a coroutine with no internal await point can't be interrupted by task cancellation mid-body, so the connection is guaranteed to close exactly once regardless of any pending cancellation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 574236c6d4
ℹ️ 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".
| fail_closed = spec.fail_closed and blocking_event | ||
| # Completion is a durable safety boundary: a missing or broken policy must | ||
| # not be converted to None and filtered out by invoke_hook(). | ||
| fail_closed = (spec.fail_closed and blocking_event) or spec.event == "pre_kanban_complete" |
There was a problem hiding this comment.
When a configured pre_kanban_complete shell hook exits with any non-2 failure code and no stdout—or exits successfully without producing a decision—parsed remains None and the stdout condition prevents the fail-closed branch from running. invoke_hook() then filters out that result, allowing the task to be marked complete despite its completion policy crashing or returning nothing; return a block for every missing/invalid completion decision and cover the real subprocess path.
AGENTS.md reference: AGENTS.md:L84-L86
Useful? React with 👍 / 👎.
| const projection = { | ||
| ...emptyProjection(), | ||
| recentEvents: events.slice(-MAX_RECENT_EVENTS), | ||
| transitions: transitions.slice(-MAX_TRANSITIONS_PER_REOPEN) |
There was a problem hiding this comment.
After the initial snapshot populates active task conditions and recent history, every live Kanban event or agent notice constructs a projection from emptyProjection(). The first such event therefore clears all task conditions and replaces the entire event feed with only that frame, making the command center report zero active conditions until the next periodic board refresh; merge the incoming events into the current projection rather than resetting it.
Useful? React with 👍 / 👎.
| if not had_identity: | ||
| result["refreshed_existing"].append(role.id) |
There was a problem hiding this comment.
With --refresh-existing --apply, a profile that already has federation_role.json is fully rewritten and synchronized but is omitted from refreshed_existing solely because had_identity is true. The JSON result and human output consequently continue to classify an actually mutated profile only as skipped/already present, which misleads operators and automation about whether the refresh succeeded; append the role after every successful refresh.
Useful? React with 👍 / 👎.
| if isinstance(params.get("enabled_toolsets"), list): | ||
| applied["toolsets"] = _best_effort(lambda: _save_toolset_pin(cfg, params["enabled_toolsets"], save_config)) | ||
| wanted = sorted(_clean_names(params["enabled_toolsets"])) | ||
| updates[("tools", "enabled_toolsets")] = wanted |
There was a problem hiding this comment.
The Desktop profile editor deliberately sends enabled_toolsets: [] when all toolsets are selected to mean “clear the pin,” but this path now persists tools.enabled_toolsets: [] instead of removing the key as _save_toolset_pin() previously did. On the next profiles.describe, _describe_toolsets() treats any list—including the empty list—as an explicit pin and reports every toolset disabled, so reopening and saving the profile can lose the user's effective selection; preserve the documented delete-on-empty behavior in the atomic writer path.
Useful? React with 👍 / 👎.
| if any(token in {"post", "-xpost", "--request=post"} for token in lowered): | ||
| return True | ||
| if any(token in {"-x", "--request"} for token in lowered): | ||
| for index, token in enumerate(lowered[:-1]): | ||
| if token in {"-x", "--request"} and lowered[index + 1] == "post": |
There was a problem hiding this comment.
For a Kanban worker issuing curl --data ... https://api.github.com/repos/o/r/pulls (or using --json), the endpoint matches but none of these explicit method tokens do, so this function returns false. curl --help all documents -d, --data as “HTTP POST data”; in an isolated container or yolo mode the command therefore proceeds past the intended unconditional guard and can create a PR without the exact-head CI receipt. Treat curl's data options as POST mutations too.
AGENTS.md reference: tools/AGENTS.md:L102-L105
Useful? React with 👍 / 👎.
… suite apps/desktop :: check:test:ui was crashing outright in CI four consecutive runs (445-650s, never producing a single test-file result or assertion failure -- just silence, then a bare FAIL from the task runner). Vitest's default pool sizing (one thread per CPU) has no memory cap, and each thread pays its own jsdom environment for this suite; on a memory-constrained CI runner that plausibly exceeds the available budget and the whole process is killed before anything can report. Capped poolOptions.threads.maxThreads to 4 for the ui project, trading some wall-clock time for staying inside the runner's memory budget. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…olOptions
The prior fix used poolOptions.threads.{max,min}Threads, deprecated and
inert in Vitest 4.1.10 (installed here) -- CI's deprecation warning
confirmed the value was silently ignored, so the crash persisted. Use
the current top-level maxWorkers/minWorkers, which replaced
poolOptions across all pool types in the 4.x migration.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a2af07388
ℹ️ 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".
| stopBackendChild(primary) | ||
| const pooledStops = stopAllPoolBackends() | ||
|
|
||
| const backgroundServicesStop = stopDesktopBackgroundServices({ |
There was a problem hiding this comment.
On every normal Desktop quit, this now invokes gateway stop --all --drain; the command parser defines --all as stopping every gateway across all profiles, including detached messaging gateways. Closing Desktop therefore takes bots, cron delivery, and Kanban dispatch offline even though those services are explicitly intended to survive the app; limit teardown to Desktop-owned serve and pooled backend processes.
AGENTS.md reference: apps/desktop/src/AGENTS.md:L23-L24
Useful? React with 👍 / 👎.
| onError(`${command.label} exceeded ${timeoutMs}ms; terminating the stop helper`) | ||
|
|
||
| try { | ||
| child.kill('SIGTERM') |
There was a problem hiding this comment.
When any gateway turn or Kanban worker takes longer than the default 20 seconds, this sends SIGTERM to the Python gateway stop --all --drain helper. That helper has already written .drain_request.json for every profile, but it relies on atexit to clear them; Python's default SIGTERM termination does not run those handlers. The gateways consequently remain alive but refuse new turns until the markers expire an hour later, so the timeout path must explicitly cancel the drain or allow the helper to perform cleanup.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| await advanceBootProgress('backend.dispatcher', 'Verifying Kanban dispatcher readiness', 92) | ||
| await ensureKanbanDispatcherReady(baseUrl, authToken, fetchJson) |
There was a problem hiding this comment.
When the bundled kanban plugin is explicitly disabled, web_server_dashboard._plugin_api_mount_skip_reason() skips mounting its API, so this request receives a 404 and ensureKanbanDispatcherReady() throws. The same occurs with a supported older managed backend that predates this endpoint, causing an otherwise healthy local Desktop backend to fail startup; treat the missing capability as disabled/compatible rather than a fatal readiness failure.
AGENTS.md reference: apps/desktop/AGENTS.md:L104-L116
Useful? React with 👍 / 👎.
| fetch_head = Path(repo_root) / fetch_head | ||
| if not fetch_head.exists(): | ||
| return None | ||
| if f"'{branch}'" not in fetch_head.read_text(encoding="utf-8", errors="replace"): |
There was a problem hiding this comment.
Fresh evidence after the branch-content fix is that FETCH_HEAD still records only the branch name here, not the selected remote. If upstream/main is fetched within five minutes while origin/main remains stale, the file contains 'main', this shortcut labels origin/main freshly fetched, and a new worktree starts from the stale ref; match both the remote and branch or fetch the selected ref.
Useful? React with 👍 / 👎.
|
|
||
| import { DispatcherReadinessError, ensureKanbanDispatcherReady } from './dispatcher-readiness' | ||
|
|
||
| const mainSource = fs.readFileSync(path.join(path.dirname(fileURLToPath(import.meta.url)), 'main.ts'), 'utf8') |
There was a problem hiding this comment.
This test reads main.ts and searches its text rather than executing the startup integration, so a comment or dead call can satisfy it while the real boot path remains miswired, and correct refactors can fail despite preserving behavior. Extract the readiness sequencing behind an importable dependency-injected function and invoke that function in the test.
AGENTS.md reference: AGENTS.md:L409-L414
Useful? React with 👍 / 👎.
|
|
||
| import { test } from 'vitest' | ||
|
|
||
| const mainSource = fs.readFileSync(path.join(__dirname, 'main.ts'), 'utf8') |
There was a problem hiding this comment.
This test also validates raw main.ts text instead of executing spawnPoolBackend, so merely placing HERMES_DESKTOP_POOL: '1' in an unused object or comment passes while the spawned process can still omit it. Move the environment construction into a pure/importable helper and assert on its returned spawn configuration.
AGENTS.md reference: AGENTS.md:L409-L414
Useful? React with 👍 / 👎.
| const known = Promise.all(rooms.map(({ owner, id }) => hidePersistedBotSession(owner, id).catch(() => undefined))) | ||
|
|
||
| return Promise.all([known, sweepBotProfileSessions().catch(() => undefined)]) | ||
| return known |
There was a problem hiding this comment.
On plugin load and reconnect, this now hides only session IDs already recorded in group-room state; visible legacy Bot Chat, Agent Inbox, and Group: rows in every unopened bot profile are no longer title-reconciled. Because the replacement sweep runs only after that exact bot is opened, these plumbing sessions can remain exposed indefinitely in the global Sessions sidebar, violating the canonical-chat invariant; retain a non-activating persisted-state reconciliation for all roster profiles.
AGENTS.md reference: apps/desktop/src/AGENTS.md:L68-L71
Useful? React with 👍 / 👎.
| "kanban_request_review", | ||
| "kanban_request_changes", |
There was a problem hiding this comment.
When kanban_request_review or kanban_request_changes returns {"ok": false} and the model then stops, successful_kanban_terminal_transition() correctly refuses to end the tool round, but these newly added names make session_called_kanban_terminal() true. The subsequent stop gate therefore suppresses its corrective nudge merely because the failed call was attempted, allowing the worker to exit cleanly while the card remains running and producing a protocol violation; only a durable successful result should suppress the nudge.
Useful? React with 👍 / 👎.
| env["GH_CONFIG_DIR"] = os.devnull | ||
| env["GIT_CONFIG_GLOBAL"] = os.devnull |
There was a problem hiding this comment.
Every local terminal subprocess now receives GH_CONFIG_DIR=/dev/null and GIT_CONFIG_GLOBAL=/dev/null, regardless of whether it is an untrusted worker. gh help environment defines GH_CONFIG_DIR as its configuration directory, and with this value gh auth status fails immediately because /dev/null/config.yml is not a directory; stripping token variables at the same boundary leaves no normal authentication path. Disabling global Git config also removes commonly relied-on identity and credential-helper settings, breaking the bundled gh pr and git commit workflows for ordinary users; scope this isolation to the protected worker context instead of applying it universally.
AGENTS.md reference: AGENTS.md:L107-L108
Useful? React with 👍 / 👎.
| - '**/*.tsx' | ||
| - 'package.json' | ||
| - 'package-lock.json' | ||
| workflow_dispatch: |
There was a problem hiding this comment.
With this workflow reduced to workflow_dispatch, JavaScript and TypeScript changes merged to main no longer start the documented npm run fix and bot-PR loop, and a repository-wide search finds no caller that dispatches this workflow automatically. Fixable lint and formatting issues can therefore remain on main indefinitely unless an operator manually starts the workflow; restore the existing path-filtered push trigger.
Useful? React with 👍 / 👎.
The test docblock was updated to say "The bot row's only activation side
effect is opening" and the warm tests were flipped to assert warmProfile /
warmAgent are NOT called on pointerEnter, but the implementation was never
updated to match.
Root cause: the warm() function and onPointerEnter={warm} were left in
bot-row.tsx after the tests were updated to document the new no-warm
contract. A roster can contain hundreds of rows and can reflow under a
stationary pointer, so pointer-entry warming behaves like roster-wide
warming in practice and must not start profile backends.
Fix: delete the warm() function and the onPointerEnter prop entirely.
Verified: bot-row.test.tsx 6/6 pass locally.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aed86fe2e5
ℹ️ 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".
| if not profile_existed_before and expected_profile_dir.is_dir() and not expected_profile_dir.is_symlink(): | ||
| try: | ||
| shutil.rmtree(expected_profile_dir) |
There was a problem hiding this comment.
When two federation seed --apply processes target the same missing role, both can record profile_existed_before = False; after one creates the profile, the other's create_profile() raises FileExistsError, and this cleanup recursively deletes the successful process's new profile, including its config and credentials. Only remove a directory proven to have been created by this invocation, such as via an atomic ownership marker or staging directory.
Useful? React with 👍 / 👎.
| def test_tui_gateway_server_has_no_unresolved_git_conflict_markers(): | ||
| source = Path(server.__file__).read_text(encoding="utf-8") | ||
|
|
||
| markers = ("<<<<<<<", "=======", ">>>>>>>") | ||
| assert not any(line.lstrip().startswith(markers) for line in source.splitlines()) |
There was a problem hiding this comment.
This test reads tui_gateway/server.py as text and checks implementation syntax rather than executing behavior, so it is a prohibited source-shape change detector that can pass on dead text and fail on harmless refactors. Remove it or replace it with an import/runtime-boundary test; unresolved conflict markers are already caught when the module is parsed.
AGENTS.md reference: AGENTS.md:L409-L414
Useful? React with 👍 / 👎.
| if reason == FailoverReason.egress_policy_blocked: | ||
| from agent.llm_egress_firewall import DestinationClass | ||
| destination = _fallback_destination_class(fb) | ||
| if destination not in (DestinationClass.LOCAL_PROCESS, DestinationClass.LOOPBACK): |
There was a problem hiding this comment.
When the new firewall raises EgressBlocked, turn_api_error.py:308 invokes _try_activate_fallback() without classified.reason, so reason is None here and this remote-destination filter never runs. A protected request consequently walks every configured remote fallback—being blocked repeatedly—before reaching a local candidate, rather than taking the intended direct local fallback; pass the classified reason through the nonretryable error path and cover the real resolution chain.
AGENTS.md reference: agent/AGENTS.md:L92-L93
Useful? React with 👍 / 👎.
| - name: Run npm high audit | ||
| id: audit | ||
| run: | | ||
| set +e | ||
| npm audit --audit-level=high --json > npm-audit.json |
There was a problem hiding this comment.
When a PR changes website/package-lock.json, plugins/platforms/photon/sidecar/package-lock.json, or scripts/whatsapp-bridge/package-lock.json, classify_changes.py:224 sets npm_lock=true, but this command runs from the repository root and audits only the root package/workspaces; none of those three projects appears in the root workspaces list. I checked npm audit --help, which lists -w, --workspace as the explicit workspace selector; no workspace or per-project working directory is supplied here, so a high-severity advisory introduced solely into one of those standalone locks does not fail this new gate.
Useful? React with 👍 / 👎.
| case 'inspect': | ||
|
|
||
| case 'inspect_blocker': | ||
|
|
||
| case 'show_source': | ||
| return completed(intent.target) |
There was a problem hiding this comment.
Whenever a user clicks Inspect or Inspect blocker in DialogueTray, these branches immediately return a successful result without invoking a backend capability or changing renderer navigation; the caller discards result.value and merely displays “Hermes accepted the action.” Thus every inspection button is a silent no-op. Route these intents to the appropriate backend/query or renderer navigation callback instead of reporting completion locally.
AGENTS.md reference: apps/desktop/AGENTS.md:L19-L25
Useful? React with 👍 / 👎.
| return _general("invalid_confidence") | ||
|
|
||
| if kind is RouteKind.SPECIALIST: | ||
| if not isinstance(profile, str) or profile not in SPECIALIST_PROFILES: |
There was a problem hiding this comment.
When a specialist capability declaration is expired, revoked, or absent, this static-name check still accepts the profile and the Discord adapter proceeds directly to create_specialist_handoff(). A repository-wide search finds CapabilityRegistry only in its defining module and tests, so none of its fail-closed resolution or revocation state affects production routing; resolve the selected profile's configured capability signature before returning a dispatchable decision.
AGENTS.md reference: AGENTS.md:L112-L114
Useful? React with 👍 / 👎.
| # inputs.upload-sarif is unset (not false) on the schedule/push/workflow_dispatch | ||
| # triggers below, where the upload must still happen — only an explicit `false` | ||
| # (ci.yaml's PR call) skips it. | ||
| if: ${{ !cancelled() && inputs.upload-sarif != false }} |
There was a problem hiding this comment.
On direct schedule, push, and workflow_dispatch runs, inputs.upload-sarif is absent and evaluates as an empty string; GitHub Actions' loose equality coerces both that value and false to zero, so inputs.upload-sarif != false is false and this step is skipped. Since the reporter also uses --fail-on-vuln=false and no caller consumes review_status on these direct runs, weekly and main-branch scans remain green with only a short-lived artifact instead of publishing findings to Code Scanning; branch on the event type or explicitly default the direct-run path to upload.
Useful? React with 👍 / 👎.
| return handle | ||
|
|
||
| @staticmethod | ||
| def _workforce_context(request: SubagentLaunchRequest) -> Optional[str]: |
There was a problem hiding this comment.
When a plugin launches a subagent with a constitution, job contract, or governance envelope, launch() still gives _build_child_preserving_parent_tools() the raw request.context; this new _workforce_context() method is referenced only by tests, and the three attributes assigned to the child are never read in production. The worker therefore never sees the validated obligations, forbidden actions, assumptions, or role-separation instructions that this helper renders; call it when constructing the child and exercise the actual launch path.
AGENTS.md reference: AGENTS.md:L112-L114
Useful? React with 👍 / 👎.
| for row in conn.execute("SELECT * FROM tasks WHERE status = 'running'"): | ||
| self.record(row, row["assignee"]) |
There was a problem hiding this comment.
When two boards dispatch workers using the same provider/model, each WorkerCapacity instance counts only the running rows in its own SQLite connection. Consequently max_in_progress_per_model, the per-model overrides, and the implicit one-worker local-model cap are multiplied by the number of active boards, allowing concurrent local loads that the cap is intended to prevent; aggregate route counts across the other board databases just as the global host-cap path already does.
Useful? React with 👍 / 👎.
| for owner in conn.execute( | ||
| "SELECT id, workspace_path FROM tasks WHERE status = 'running' AND id != ? " | ||
| "AND workspace_path IS NOT NULL", (claimed.id,), |
There was a problem hiding this comment.
When tasks on two different boards resolve to the same dir workspace, this query sees only running owners in the current board's database, so both dispatchers can pass the new collision guard and start workers that concurrently modify the same checkout. The existing cross-board counting helper confirms that each board has a separate connection; extend the ownership check across all active board databases before spawning to preserve the stated one-worker-per-physical-checkout invariant.
Useful? React with 👍 / 👎.
Thread 1 (plugins_discovery): exclude github-pr-feedback from bundled auto-discovery per plugins/AGENTS.md vendor SaaS connector policy. Thread 2 (egress split): extract terminal classifier functions and constants from llm_egress_runtime.py into new llm_egress_terminal.py before adding github-pr-feedback behavior. Reduces the god-file by ~2,150 lines; runtime re-imports all moved symbols. Thread 3 (session.py _supports_conversation_worktree): broaden the platform allowlist to accept any non-None SessionSource, not just the two originally hard-coded values. Thread 4 (session.py _conversation_worktree_manager): route to the profile-specific database via _db_for_key(session_key) instead of always using the ambient self._db. Thread 5 (session-sweep.ts): extend the Bot Chat hide sweep to cover profile-wide sessions (bot-to-bot/CLI flows) on load/reconnect via reconcileAllBotProfileSessions(), in addition to the existing group-chat-known-id sweep. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 588a345b99
ℹ️ 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".
| @@ -820,7 +1801,7 @@ def _complete_current_ci_task(receipt: CIAuditReceipt) -> None: | |||
| if not task_id: | |||
| return | |||
| board = os.environ.get("HERMES_KANBAN_BOARD", "").strip() | |||
| argv = ["hermes", "kanban"] | |||
| argv = [sys.executable, "-m", "hermes_cli.main", "kanban"] | |||
There was a problem hiding this comment.
When an exact-head CI audit runs from its PR worktree, this python -m invocation puts that untrusted working directory on sys.path, so a PR containing a replacement hermes_cli package can execute code during task completion instead of invoking the installed control plane. _block_current_ci_task() repeats the same unsafe invocation; add -P or route both callbacks through the governed command helper, as was done for repair completion.
AGENTS.md reference: AGENTS.md:L66-L67
Useful? React with 👍 / 👎.
| index = _best_structural_match(item, loaded_expanded, used_loaded) | ||
| if index is None: | ||
| index = len(preserved) |
There was a problem hiding this comment.
When an unnamed list gains a new entry while an existing env-backed entry is both edited and reordered, the new entry can claim the old entry through this structural/positional fallback, leaving the real old entry unmatched and writing its expanded secret into config.yaml. This is reproducible with hooks.pre_tool_call: prepend a new hook sharing the old timeout, then modify the old hook's command and timeout; saving materializes ${HOOK_SECRET} as its plaintext value. Matching should leave unrelated insertions unclaimed and preserve the edited entry's template through the real save_config() path.
AGENTS.md reference: hermes_cli/AGENTS.md:L64-L68
Useful? React with 👍 / 👎.
| def compile_profile_routes( | ||
| profiles: Sequence[str], artifact: Mapping[str, Any] | ||
| ) -> CompiledRouteTable: |
There was a problem hiding this comment.
No non-test production path calls compile_profile_routes(), resolve_route(), or compile_profile_configs(); the only production import is between the two newly added modules, and that compiler itself has no caller. Consequently a compiled benchmark artifact never changes primary, auxiliary, review, Kanban, or fallback model selection, so this entire routing surface is speculative/dead rather than enforcing the advertised profile-specific routes. Wire it through the real provider/model resolution chain with an E2E path or remove it.
AGENTS.md reference: AGENTS.md:L95-L97
Useful? React with 👍 / 👎.
| if isinstance(value, int) and not isinstance(value, bool) and value < 0: | ||
| _issue( | ||
| issues, "error", | ||
| f"code_execution.max_tool_calls is negative ({value!r})", | ||
| "Use zero to disable the limit, or a positive integer to cap tool calls", |
There was a problem hiding this comment.
When an existing configuration uses code_execution.max_tool_calls: -1, this validator now reports an error even though _configured_max_tool_calls() explicitly accepts every value <= 0, _tool_call_limit_reached() treats it as unlimited, the tool test asserts -1 is valid, and the updated example documents <= 0 = unlimited. Startup and hermes doctor therefore misdiagnose a supported setting; either allow negative values here or consistently remove that behavior and documentation.
Useful? React with 👍 / 👎.
| outcome="crashed", | ||
| release_claim=True, | ||
| end_run=True, |
There was a problem hiding this comment.
When a worker loses its lease and the dispatcher starts a replacement before the old process reaches this finalizer, this call identifies only the task ID. _record_task_failure() then reads the replacement's current run, clears its claim, and _end_run() marks that new run crashed. Pass the originating HERMES_KANBAN_RUN_ID and claim lock through a fenced compare-and-swap, as the budget-exhaustion path immediately above already does, so a stale guardrail halt cannot terminate another worker's work.
Useful? React with 👍 / 👎.
| payload = repr(value).encode("utf-8", errors="replace") | ||
| return sha256(payload).hexdigest() | ||
|
|
||
| def _typed_payload( |
There was a problem hiding this comment.
This new _typed_payload() function spans 1,048 lines, and its containing module is 2,329 lines, exceeding both repository decomposition thresholds in one security-critical classifier. The many unrelated terminal, source-provenance, Kanban, GitHub, and provider cases need to be split into topical siblings before further behavior is added so each policy boundary remains reviewable and testable.
AGENTS.md reference: AGENTS.md:L264-L266
Useful? React with 👍 / 👎.
| <h1 className="text-lg font-semibold">World disabled</h1> | ||
| <p className="mt-2 text-sm text-(--ui-text-tertiary)"> | ||
| Enable World in Settings → Plugins to make Lunar City available again. | ||
| </p> |
There was a problem hiding this comment.
When Desktop is running with the Chinese locale, the entire new Lunar City page—including disabled state, onboarding, buttons, dialogue prompts, scene labels, and error feedback—still renders hard-coded English because none of these strings were added to the translation interface or locale tables. Move the new surface copy into i18n and provide every supported locale rather than shipping a large untranslated page.
AGENTS.md reference: apps/desktop/AGENTS.md:L208-L209
Useful? React with 👍 / 👎.
| return False | ||
| now = dt.datetime.now(dt.timezone.utc).replace(microsecond=0) | ||
| payload = { | ||
| "schema": "exampleapp_agent_report_v1", |
There was a problem hiding this comment.
This gateway module hard-codes another product's report schema and command protocol directly into core progress handling. The integration has no generic provider boundary or standalone plugin ownership, so Hermes now carries a vendor-specific connector and its compatibility burden despite the repository policy requiring third-party product integrations to ship as standalone plugins. Move this bridge behind the existing plugin surface instead of importing it from gateway/progress_queries.py.
AGENTS.md reference: AGENTS.md:L115-L120
Useful? React with 👍 / 👎.
| updates[("mcp_servers",)] = mcp_cfg | ||
| if updates: | ||
| try: | ||
| _write_raw_config_values(updates) |
There was a problem hiding this comment.
When one editor save contains multiple dirty sections and only one destination is administrator-managed—for example, managed skills.disabled plus an editable toolset selection—this batched writer validates every path before writing and raises on the managed leaf, so the editable sections are discarded and all are reported as failed. The endpoint explicitly promises independent sections, and the previous implementation saved each separately; split the write by section or filter managed destinations so one refused setting does not prevent unrelated changes.
Useful? React with 👍 / 👎.
Thread PRRT_kwDOT_eOos6gzdSH (dispatcher-readiness.ts): - Treat HTTP 404 from disabled Kanban plugin as non-blocking (returns status: 'disabled' instead of throwing DispatcherReadinessError). Thread PRRT_kwDOT_eOos6gzdSU (dispatcher-readiness.test.ts): - Replace text-scan test (reads main.ts as raw string) with a behavioral test for runDispatcherReadinessGate() that verifies advancePhase fires before the readiness check, and a 404 test case. Thread PRRT_kwDOT_eOos6g03Mi (secure_worker.py): - Include symlinks in rglob inventory: path.is_file() or path.is_symlink(). Thread PRRT_kwDOT_eOos6g03Mb (secure_worker.py): - Deny 'hooks' and 'hooks_auto_accept' keys in secure-worker audit profiles. Thread PRRT_kwDOT_eOos6g03M0 (supply-chain-audit.yml): - Detect changed lock-file directories and run npm audit per directory. Thread PRRT_kwDOT_eOos6g2OkE (federation.py): - Write toolsets to platform_toolsets["cli"] in addition to toolsets. Thread PRRT_kwDOT_eOos6g03Ms (federation.py): - Snapshot config/identity files before refresh; roll back on exception. Thread PRRT_kwDOT_eOos6g2OkK (federation.py): - Use canonical manifest _role_to_groups map to replace stale memberships. Thread PRRT_kwDOT_eOos6g2OkG (specialist_routing.py): - Wire CapabilityRegistry.is_profile_declared() into route decisions. Thread PRRT_kwDOT_eOos6g4YKr (worktree_environment.py + test): - Add _platform kwarg to _venv_python_path; use it in tests instead of monkeypatching sys.platform, making both branches testable on any host. Thread PRRT_kwDOT_eOos6g03MJ (world-sync.ts): - publishEvents now merges incoming events into the current projection instead of clobbering it with emptyProjection(). Add getProjection() to WorldSyncSink; store it in storeWorldSyncSink(). Thread PRRT_kwDOT_eOos6g03MS (index.tsx + world-sync.ts): - Guard refreshWorldProjection with optional isCancelled predicate; index.tsx effect sets cancelled=true on cleanup to discard stale fetches. Thread PRRT_kwDOT_eOos6g03MX (world-actions.ts): - Route inspect/inspect_blocker/show_source through context.inspect door when provided; report as unavailable otherwise instead of silently succeeding. Thread PRRT_kwDOT_eOos6g03Mn (discord/adapter.py): - Check remaining configured users before disconnecting from voice channel. Thread PRRT_kwDOT_eOos6g4YKu (model_performance_router + llm_egress_runtime): - Add install_performance_route_table() to llm_egress_runtime; _route_for_agent consults the compiled table when agent.performance_surface is set. - GatewayRunner._init_runtime_settings loads artifact from agent.performance_route_artifact config key at startup. - End-to-end tests verify _route_for_agent uses the installed table. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Body was at class-method def level (8 spaces) rather than inside the def (12 spaces), causing ruff to raise invalid-syntax and blocking CI. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…gistry The method was indented at 8 spaces (inside is_profile_declared()) rather than 4 spaces (class level), so it was unreachable dead code. The previous commit re-indented the body but left the def at the wrong level, causing AttributeError in all resolve() call-sites. Dedent the entire method by 4 spaces so it is a proper CapabilityRegistry instance method. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
hermes-agent/apps/desktop/src/plugins/kanban/completion-notify.ts
Lines 193 to 194 in 563299f
Handle events from the implicit current board
When the selected board is the normal default, $boardSlug is '' and means the server's current board (api.ts:44-45), but every /events frame is passed here with that empty slug and immediately discarded. Consequently in-app/OS completion notifications and all Lunar City event listeners remain silent until the user explicitly selects a named board; use a stable key or resolve the current board instead of suppressing these frames.
AGENTS.md reference: apps/desktop/AGENTS.md:L74-L75
ℹ️ 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 open = (slug: string) => { | ||
| close?.() | ||
| close = socket(slug ? `/events?board=${encodeURIComponent(slug)}` : '/events', data => onEventsFrame(slug, data)) | ||
| close = socket(slug ? `/events?board=${encodeURIComponent(slug)}` : '/events', data => |
There was a problem hiding this comment.
When a user switches away from a named board and later returns, or the socket reconnects, the client retains seenEventIdByBoard but opens a new /events socket without since. The server explicitly baselines such sockets at the current maximum (plugin_api.py:1740-1745), so terminal events created while disconnected are never sent and cannot be recovered by the client-side cursor; include the saved cursor in the reopened URL.
AGENTS.md reference: apps/desktop/AGENTS.md:L74-L75
Useful? React with 👍 / 👎.
| function sourceScope(source: WorldSource, board?: string): string { | ||
| return `${source}:${board ?? 'global'}` | ||
| } |
There was a problem hiding this comment.
When Desktop performs a live profile or connection swap, this persisted cursor key still contains only the source and board slug. Independent profiles can both have a main board with unrelated event-ID sequences, so a high-water mark from the first profile causes events from the second profile to be treated as already seen until its IDs overtake that value; include connection/profile identity in the cursor scope.
AGENTS.md reference: apps/desktop/AGENTS.md:L43-L47
Useful? React with 👍 / 👎.
| try { | ||
| const result = reconcileWorldSnapshot(await snapshot(), [], sink.getCursors()) | ||
|
|
||
| if (isCancelled?.()) { | ||
| return | ||
| } | ||
|
|
||
| sink.publish(result.projection, result.cursors) |
There was a problem hiding this comment.
If a Kanban or notice event arrives while snapshot() is pending, bindWorldSources first publishes it, but this continuation reconciles with an empty incoming list and then replaces the projection, silently removing the newer event. Because its cursor was already advanced, the transition may never reappear; reconcile the snapshot with the sink's current recentEvents and protect against stale refresh completion.
AGENTS.md reference: apps/desktop/AGENTS.md:L64-L70
Useful? React with 👍 / 👎.
| if fast_lane and not self._voice_fast_lane_requests_work(transcript): | ||
| # Delivery stays in the bound text channel; only storage identity changes. | ||
| source._voice_fast_lane = True | ||
| source._session_key_lane = f"discord-voice:{voice_channel_id}" | ||
| event.metadata["voice_fast_lane"] = True |
There was a problem hiding this comment.
When an allowlisted speaker makes a conversational fast-lane utterance, this only annotates the source/event and changes its session key. A repository-wide search finds _voice_fast_lane read only by the busy-turn queue logic and finds no consumer of metadata["voice_fast_lane"], so agent construction still supplies the normal full toolset despite _voice_fast_lane_requests_work() promising that these turns remain tool-free; enforce the bounded toolset in the real agent setup path.
Useful? React with 👍 / 👎.
| _run_git(source_root, "ls-files", "--error-unmatch", "--", rel) | ||
| source = source_root / relative | ||
| try: | ||
| mode = source.lstat().st_mode | ||
| except OSError as exc: | ||
| raise SecurityBoundaryError(f"selected file unavailable: {rel}") from exc | ||
| if stat.S_ISLNK(mode): | ||
| raise SecurityBoundaryError(f"symlink denied: {rel}") | ||
| if not stat.S_ISREG(mode): | ||
| raise SecurityBoundaryError(f"non-regular file denied: {rel}") | ||
| data = source.read_bytes() |
There was a problem hiding this comment.
When another editor or process modifies a selected tracked file after _assert_clean_repository() returns but before this read, the pack contains the new working-tree bytes while its manifest still records the previously verified HEAD and tree. This TOCTOU can send uncommitted or private source to the remote worker under a false clean-commit receipt; read each blob from the captured commit or revalidate the repository and hashes atomically before admitting the pack.
Useful? React with 👍 / 👎.
| _snapshots: list[tuple[Path, bytes | None]] = [] | ||
| for snap_path in (config_path, identity_path): | ||
| _snapshots.append((snap_path, snap_path.read_bytes() if snap_path.is_file() else None)) |
There was a problem hiding this comment.
When --refresh-existing reaches _write_role_identity() and _sync_role_skills() later fails, the rollback snapshots only config.yaml and federation_role.json. The role addendum already appended to SOUL.md, and any skills copied before the failure remain installed, so a command reported as failed still partially mutates the user's existing profile; snapshot or transactionally stage the soul and skill tree as well.
Useful? React with 👍 / 👎.
| for dir in ${{ steps.lockdirs.outputs.dirs }}; do | ||
| (cd "$dir" && npm audit --audit-level=high --json) >> npm-audit.json || rc=$? | ||
| done |
There was a problem hiding this comment.
When a PR changes two standalone lockfile directories and either audit finds a high-severity advisory, this loop appends two complete npm audit --json documents to one file. The following json.load() then raises on the concatenated documents, so review_status is never emitted and the workflow fails with a JSON parser error rather than the actionable advisory summary; write one result per directory or aggregate the parsed documents explicitly.
Useful? React with 👍 / 👎.
| candidate_base_url = getattr(client, "base_url", "") | ||
| if not isinstance(candidate_base_url, str) or not candidate_base_url.startswith( | ||
| ("http://", "https://") | ||
| ): | ||
| candidate_base_url = raw_runtime.get("base_url") |
There was a problem hiding this comment.
When a protected Kanban turn uses a local main model but a separately configured remote auxiliary OpenAI client, OpenAI.base_url is an httpx.URL rather than a str, so this rejects the auxiliary URL and substitutes the main runtime's loopback URL. dispatch_authorized_agent_request() then classifies the route as local and invokes the callback without authorization even though that callback sends the private payload to the remote auxiliary endpoint; normalize the client URL to a string before falling back to main-runtime metadata.
AGENTS.md reference: agent/AGENTS.md:L89-L93
Useful? React with 👍 / 👎.
1 participant
Fixes 6 P1-severity findings surfaced by automated review on the recently-merged
PR-feedback stack (#39-#49), plus carries forward all of that stack's changes
since it wasn't reachable from main independently:
_governed_command_prefix()(adds-P) instead of a barepython -minvocation, closing an untrusted-worktree sys.path injection.
worker_completion_policynow fails whenHERMES_SAFE_MODEis active, since dispatched workers inherit it and plugin discovery is
skipped entirely under it.
namenow defaults to its directoryname before comparison, matching
parse_manifest_file()'s actual runtimebehavior.
still contains an unexpanded
${VAR}reference rather than trusting anexpansion against the wrong process's environment.
SystemExit(not justException) around_write_raw_config_values(), which raises it for managed-scope keys;previously this could kill the shared TUI/Desktop/dashboard RPC backend.
_preserve_env_ref_templates(): matches a modified+reorderedunnamed list entry to the loaded item it structurally resembles instead of
positionally, so a sibling's unchanged
${VAR}template isn't dropped intoplaintext on save.
🤖 Generated with Claude Code