chore: sync upstream main - #5
Merged
Merged
Conversation
…time ensureRegistryBackend delegated kind==='local' to ensureBackend(), which follows the v1 connection.json routing table — under a v1 REMOTE global mode (the migration keeps the mandatory 'local' entry AND makes that remote the registry primary) the roster's 'This device' rows enumerated and dialed the REMOTE primary: every profile appeared twice (forcing -slug handles) and clicking a local agent talked to the remote box. resolveRegistryLocalRoute() (pure, colocated with the registry helpers) now decides the local entry's path: delegate to the legacy route only when v1 is itself local (single-source behavior byte-identical); otherwise spawn/reuse a forced-local pool child via spawnPoolBackend's new forceLocal option, pooled under the composite conn:local::<profile> key so it cannot collide with the v1 remote descriptor cached at the bare profile key.
Secondary-gateway events were tagged with connectionId (store/gateway fan-out) but no consumer read it: working/attention tracking, the pruneSecondaryGateways keep-set, and the profile-scoped event gates (skin.changed / change-watcher broadcasts / approval-mode reconcile) all keyed by session id + bare profile name. Every registered source exposes a 'default' profile (the roster force-unshifts it), so two connected gateways collided — gateway B's 'default' activity was attributed to gateway A's 'default', keeping the wrong socket alive and applying the wrong source's config/skin/cron changes. Thread connectionId through consumption using the existing composite backendScopeKey helper: - session-states records each registry-tagged event's (connectionId, profile) scope per runtime session; liveSessionScopes() projects the busy/needs-input ones as composite keys for the gateway keep-set. - recomputeKeptGateways (use-gateway-boot) seeds the keep-set with those scopes; pruneSecondaryGateways matches registry-scoped entries ONLY on their composite key, while local entries keep matching bare profile names (single-source path unchanged). - gateway-event's 'from the active profile' gates now compare the event's composite scope against the active gateway's connection via the new activeGatewayConnectionId(); untagged local/primary events behave byte-identically. Display-only surfaces that already use roster handles are untouched.
…tionist/sort-imports)
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Follow-up to NousResearch#87400: drop the max_iterations and prompt_file knobs from auxiliary.background_review. The aux model routing (provider/model/ base_url/...) predates NousResearch#87400 and stays; the enabled switch and the usage telemetry stay. The fork's iteration budget returns to the historical hardcoded 16.
…pull hermes update runs in the PRE-pull Python process. After git pull updates source files on disk, modules already in sys.modules still hold the OLD code. The existing _reload_config_modules() reloaded only config modules, but the post-update dashboard cleanup path (_finish_dashboard_update_cleanup -> _scan_dashboard_processes) imports hermes_cli._subprocess_compat lazily; a new symbol added there (e.g. bounded_probe_run) is invisible to the cached module object, causing ImportError during the cleanup step. Extend the reload list to include hermes_cli._subprocess_compat and hermes_cli.dashboard_procs so the cleanup uses freshly-pulled code.
…ry point Widen PR NousResearch#87757 to cover the ZIP path: _update_via_zip() also calls _finish_dashboard_update_cleanup() but never runs _reload_config_modules, so the Windows git-broken fallback would still crash with the same ImportError (cannot import name 'bounded_probe_run' from the stale cached hermes_cli._subprocess_compat). - new _reload_process_scan_modules() called inside _finish_dashboard_update_cleanup itself, so every current and future call site is covered; reloads dependency-first (_subprocess_compat, then dashboard_procs) - reload failures log at warning (a miss surfaces seconds later as an ImportError in the same process) - regression tests: reload-before-kill ordering, node-failure skip, stale-module symbol restoration (the exact NousResearch#87134 boundary state), nonfatal reload failure, and the NousResearch#87757 reload-list contract
… across IDE and macOS terminals (NousResearch#87854) * fix(tui): send atomic CSI u for modified Enter in IDE terminals VS Code/Cursor/Windsurf terminals bound Shift/Ctrl/Cmd+Enter to the legacy \\r\n sequence, which Ink's parse-keypress split into a backslash keypress plus a plain Return — inserting a stray backslash and submitting instead of adding a newline. Emit Kitty CSI u sequences that encode the modifier atomically, and migrate keybindings users already have on disk. Co-authored-by: yatesjalex <yatesjalex@users.noreply.github.com> * fix(tui): treat a bare LF as a newline in macOS composer terminals Terminals that can't send a distinct Shift+Enter collapse a modified Enter / Ctrl+J down to a bare LF. shouldPreserveCtrlJNewline() already handles the env-detectable cases (SSH, Windows Terminal, Ghostty, WSL), but plain macOS terminals (Terminal.app, iTerm2 defaults) do the same and aren't env-detectable, leaving no keyboard-driven newline there. Fold the return-key decision into shouldInsertNewlineOnReturn() and accept a bare LF as a multiline fallback on macOS too, keeping CR as submit everywhere. Co-authored-by: LeonSGP43 <LeonSGP43@users.noreply.github.com> --------- Co-authored-by: yatesjalex <yatesjalex@users.noreply.github.com> Co-authored-by: LeonSGP43 <LeonSGP43@users.noreply.github.com>
…cause 'database disk image is malformed' contains the word 'disk', so classify_persistence_error bucketed SQLITE_CORRUPT / SQLITE_NOTADB failures as 'disk' and the turn-completion explainer told users to free disk space for a structurally damaged state.db (the NousResearch#77386-family misdiagnosis, reproduced in the v0.20.0 malformed-DB incident report). - hermes_state: new 'corrupt' bucket in PERSISTENCE_ERROR_CAUSES, matched via _DB_CORRUPTION_MARKERS BEFORE the locked/disk buckets - run_agent: explainer text for 'corrupt' points at hermes doctor and explicitly says freeing space will not help - cron explainer-variant suppression picks the new variant up automatically (it iterates PERSISTENCE_ERROR_CAUSES)
…ousResearch#78153) (cherry picked from commit 7afd991)
f1ea4a5 ("cover the remaining setup-time .env reads with utf-8-sig", following 75afc47 for mem0/hindsight) swept this class; openviking's _write_env_vars was missed and still reads with strict utf-8. It copies every existing line through on each update, so the read decides whether a credential update lands: BOM'd .env -> the first key never matches, so the old line survives and the new value is appended as a duplicate. .env loaders keep the first occurrence, so the update silently does nothing. cp1252 .env -> UnicodeDecodeError aborts setup outright. Read exactly like the canonical hermes_cli/config.py save_env_value (utf-8-sig + errors="replace"). A plain UTF-8 file rewrites byte-identically. Scope: hermes_cli/memory_setup.py has the same read but is already the subject of NousResearch#30281 / NousResearch#60587, so it is left alone here. (cherry picked from commit 175c685)
Clarify that the Desktop backend can add Hermes venv packages to PYTHONPATH and that current .env loaders use the last duplicate value.
This reverts commit 26b2b47.
This reverts commit 8fc4189.
hermes update discovered and restarted hermes-gateway* systemd units but never looked for hermes-serve* — the Desktop app's backend — so it kept running stale pre-update code until the user restarted it by hand (NousResearch#83438). Extend the systemd unit discovery/restart loop to also match hermes-serve* units. They don't wire SIGUSR1 to a graceful drain (only gateway/run.py does), so restart eligibility for the graceful path is now gated on unit name via a small, directly-tested helper; hermes-serve units fall straight to the existing blunt systemctl restart path, matching the workaround the issue already documents.
…tarts Review on NousResearch#83595 flagged two service-lifecycle gaps in the hermes-serve restart support: - The unit-name gate accepted anything starting with "hermes-serve", which also matched the unrelated hermes-server.service. Require the exact base unit or the hyphenated profile family instead. - The fleet-restart loop and _finish_dashboard_update_cleanup() could both restart the same hermes-serve unit — the loop restarts it directly, then cleanup's PID scan finds the fresh process and restarts its owning unit again. Thread the fleet loop's restarted unit names through to _kill_stale_dashboard_processes() so it skips units already handled.
Mirror the strict unit-name shape from the hermes-serve gate (review on PR NousResearch#83595) on the gateway side too: the discovery gate and the SIGUSR1 eligibility helper now accept only `hermes-gateway.service` or the `hermes-gateway-<profile>` family, so a near-prefix unit like `hermes-gatewayd.service` can neither enter the restart path nor be sent a SIGUSR1 it does not handle.
pluginSocket (hermes.ts) is documented as "the live twin of pluginRest, scoped the same way", but it calls window.hermesDesktop.getConnection() with no profile argument, while pluginRest passes the active profile via profileScoped(). getConnection's IPC handler (ensureBackend in electron/main.ts) falls back to the primary profile whenever the profile argument is empty, so an unscoped call always resolves to the primary profile's backend regardless of which profile is actually active. For a plugin used from a non-primary profile (e.g. kanban), this means REST calls go to the correct pooled backend while the plugin's WebSocket silently connects to the wrong one — a multi-profile user sees one profile's data with another profile's live events. Fix (adapted to the post-NousResearch#87600 registry-agent store shape during salvage): resolve the plugin socket's connection through the same (connectionId, profile) source of truth ensureGatewayProfile/ensureGatewayAgent maintain for $connection — store/gateway's setActive now pushes the active scope's registry connection id into the hermes module (setApiRequestConnection, the no-store-import twin of setApiRequestProfile), and pluginSocket resolves via getConnectionFor for registry-agent scopes and getConnection(profile) for the local pool. The plugin socket therefore follows registry-agent activations too, not just profile switches. voice-playback.ts's resolveSpeakStreamUrl had the same gap originally, but main has since fixed it independently (via the getApiRequestProfile() getter rather than direct store access) — dropped from this PR as redundant, keeping only the still-open pluginSocket gap. Co-authored-by: Hermes Agent <hermes@nousresearch.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
The Aug 8 default-on upscaling policy (66ea4e6) chained the Clarity Upscaler after every sub-2MP generation. Clarity is an SD1.5 creative tile-diffusion enhancer (creativity 0.35, "masterpiece" prompt prefix) — it redraws content, which degraded output on 100% of generations for models like GPT Image 2 and Ideogram whose value is precise text rendering, CJK, and photorealistic detail. Policy now: no model upscales by default, on FAL or Krea. The `upscale` tool param remains as a per-call opt-in (`upscale: true`); explicit requests still chain Clarity (FAL) / Krea Enhance as before. - FAL catalog: all 17 default-on entries flipped to upscale=False - Krea plugin: medium + medium-turbo per-model defaults flipped off - Tool schema: upscale param described as opt-in with a fidelity warning - Tests updated: catalog invariant now pins all-off; default-on cases now assert no upscaler call - Docs (en + zh) updated to the opt-in policy
…user interrupt The sequential tool path only noticed a user interrupt after the running tool returned: with the deadline disabled it ran the tool inline (fully blocking), and with a deadline it waited in 5s slices without ever checking agent._interrupt_requested. Any tool without cooperative is_interrupted() polling (image_generate, tts, transcription, skills sync, ...) held the whole turn hostage — the reported symptom was a redirect queued ~40s behind a FAL image generation + upscale pass. Executor backstop (class fix, covers ALL tools): - _run_sequential_tool_execution_middleware always dispatches on the daemon worker (timeout None no longer means inline blocking) and polls the interrupt flag every 1s. - On interrupt: 3s cooperative grace (mirrors the concurrent path), then synthesize a cancelled tool result (_ToolCancelledResult), emit the terminal post_tool_call with status=cancelled, and abandon the worker. - _ToolCancelledResult suppresses downstream post-hook double emission exactly like _ToolTimeoutResult, so an abandoned worker finishing late cannot report success for a cancelled call. - clarify (interactive, _NEVER_PARALLEL_TOOLS) keeps the inline path — it owns its own human wait. Cooperative layer in the reported offender: - image_generation_tool: blind handler.get() (generation + Clarity upscale) replaced with _wait_fal_result(), which polls is_interrupted() in 0.5s slices and raises ImageGenerationInterrupted immediately. - _upscale_image propagates the interrupt instead of swallowing it into the "upscale failed, use original" fallback. Message alternation is preserved: the cancelled result is a normal tool result for the call_id. Sabotage-verified: with the old wait loop restored, the new tests fail (tool blocks full runtime); with the fix they pass in ~4s.
Live-testing the Cua Driver 0.20 convergence on Windows 11 (session 2, cua-driver 0.20.0) surfaced three defects in the existing-profile browser path and in install status. 1. The config grant was silently nullified by an approval bypass. `--yolo` / `-z` map onto a private unrestricted daemon, which answers every browser_prepare. Because the host delegated the entire existing-profile decision to the driver, that bypass also nullified `computer_use.grant_existing_profile: false`: a plain `hermes -z` attached to the user's real Chrome profile and read live page content over CDP, with the driver reporting it as "the approved existing Chromium profile". It was never approved. An approval bypass is consent to skip prompts, not consent to read an existing profile's pages, cookies, and storage. CuaTypedBrowserRoute.prepare now enforces the key itself, regardless of permission mode. bounded stays exempt - its reviewed capability manifest is the authorization boundary. The authorization inputs are resolved in the backend from config and the backend's immutable mode, never from model-supplied kwargs. 2. The grant, once set, still could not be used. With `grant_existing_profile: true` the runtime is launched `--grant existing-profile` correctly, but cua_browser_prepare then hit a runtime approval prompt anyway - re-asking the user to authorize what the config already authorized, and making the documented opt-in unusable on any non-interactive run, where the prompt has nobody to answer it and the call dies on approval timeout. The durable, file-backed grant now stands in for that prompt. Scope is narrow: only the existing-profile prepare, only when the grant is present; isolated launches still prompt and any resolution failure falls closed to prompting. 3. `computer-use status` hid a custom override and spliced its output. With HERMES_CUA_DRIVER_CMD pointed at cmd.exe, status printed the child's multi-line banner and prompt inside the one-line version field, never mentioned the override, and advised `hermes computer-use install` - which install itself (correctly) refuses to run against an overridden path. It now names the override and mirrors install's update-or-unset guidance, and version output is reduced to one bounded line. Verified on the reported host: `-z` existing-profile attach now refuses and names the key; `grant: true` no longer prompts (33s vs a 300s approval timeout); status names the override and prints one line. No change to the reconciliation path - driver SHA256 unchanged end to end. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-ups on the NousResearch#87261 salvage: - cron/jobs.py: the persisted-error re-arm now respects schedule legality. Re-arming to `now` fired CRON jobs at times their expression excludes — a weekday-only 9am job whose Friday run errored would fire on SATURDAY (croniter measures a 24h cadence on Saturday, so 27h > cadence+grace and the guard tripped). Cron jobs re-arm to compute_next_run(schedule, now) — the next LEGAL occurrence — and only when that actually moves next_run_at earlier; interval jobs (the 2026-08-14 incident class) keep the immediate now re-arm, which is always legal for intervals. - cron/jobs.py: cache _schedule_cadence_seconds' croniter measurement per expr (mirrors scheduler.py's _cron_interval_cache) — it runs inside _jobs_lock on every tick for every stale-errored job. - tests/cron/test_persisted_error_rearm_legality.py (new): weekday job errored Friday re-arms to Monday (not Saturday), correctly-parked cron value untouched, interval job still due immediately.
…adence cache /simplify-code findings on the salvage stack: - reuse HIGH: _compute_grace_seconds duplicated the exact croniter two-fire period measurement _schedule_cadence_seconds implements (interval minutes*60 branch included) — grace is now derived from the shared helper, so cadence is measured in exactly one place (and grace computations now benefit from the per-expr cache too). - efficiency: _cron_cadence_cache was unbounded in principle (deleted/ edited exprs never evicted) — hard 256-entry bound with full clear; rebuild cost is two croniter evals per live expr. 80 recovery/rearm/jobs tests + 94 scheduler tests green; ruff clean.
…ntly (NousResearch#87644) tick() swallowed a real OSError at tick-lock acquisition as 'another instance holds the lock', so fd exhaustion (EMFILE/ENFILE) made the scheduler return 0 — recorded as a successful tick — while no job ever ran again. Heartbeat and success markers stayed fresh, masking the stall. - propagate lock-acquisition OSError to the ticker loop (records + backs off) - detect fd exhaustion, attempt gc.collect() + raise soft nofile limit - exponential backoff so an exhausted process stops hammering the store - preserve genuine lock contention (EWOULDBLOCK) silent-skip behavior - 11 regression tests
…ck failure Follow-ups on the NousResearch#87796 salvage: - cron/scheduler.py: drop the _reclaim_fds_best_effort call at tick()'s lock-failure raise site — the ticker loop's except handler already runs reclamation once per failed tick, so the raise-site call doubled the gc.collect() pause on every EMFILE failure. - cron/scheduler_provider.py: extract the exponential-backoff math duplicated verbatim in start() and _start_multiplex() into a module-level _backoff_wait_seconds() helper. - hermes_cli/cron.py: `hermes cron tick` now reports a propagated OSError cleanly (exit 1) instead of dumping a traceback — tick() raising on real lock-acquisition failures is new behavior from this fix.
…ustion text matcher /simplify-code findings on the salvage stack: - the classify+reclaim+counter block was pasted verbatim into both ticker loops (_start and _start_multiplex) along with duplicated function-local imports — extracted _note_tick_failure() next to _backoff_wait_seconds so both loops share one implementation. - hermes_cli/cron.py's EMFILE hint reimplemented the text half of _is_fd_exhaustion with a case-SENSITIVE variation (drift risk) — split _is_fd_exhaustion_text() out and use it from both. 11 EMFILE tests + 54 provider/ticker tests green; ruff clean.
…(t_8b5480b3) The age-only stale-claim sweep (t_3778a491, already on main) force-releases an in-memory _running_job_ids claim only once it is older than max(2*interval, 30m). A leaked claim that is YOUNG (inside its allowance) while the durable executions ledger already proves the last run ended stays wedged: the job is returned as due every tick, _submit_with_guard short- circuits on 'already running', and next_run_at keeps fast-forwarding with no execution — the exact 2026-08-14 recurring-router incident (t_20e23f84), which survived a gateway restart because the in-memory age bound alone could not see a run the ledger had already finished. sweep_stale_inflight now reconciles each in-flight claim against the durable executions ledger (cron/executions.db): if the job's MOST RECENT execution row is terminal (completed/failed/unknown), the run provably ended, so the claim is stale by construction regardless of its in-memory age and is force- released. This is a persisted-state recovery path: the ledger is written by the worker that ran the job and read by ANY ticker process (including one that started AFTER the leak), so a leaked claim is recoverable without force-run/resume and without depending on which process holds it in memory. A ledger-terminal release is authoritative — it does not write a synthetic mark_job_run failure (the ledger already records the outcome). Added TestLedgerTerminalReconciliation (4 tests): young+terminal -> released (RED on main, GREEN here), no-ledger-row -> not released, running-row -> not released, old+terminal -> released once without synthetic failure.
Follow-ups on the NousResearch#87259 salvage: - cron/scheduler.py: the ledger-terminal reconciliation now requires the terminal execution row's claimed_at to be >= the in-memory claim's registration time (_running_since). Without this, the latest terminal row for a recurring job is usually the PREVIOUS run's outcome — a fresh claim in the try_register_running_job -> create_execution window (or a finished run whose worker finally block hasn't released yet) would be force-released and the job double-dispatched. Unparseable/missing claimed_at fails closed to the age-based bound. - cron/scheduler.py: take the _running_job_ids snapshot for the ledger query under _running_lock — list() over a set concurrently mutated by try_register/release_running_job can raise RuntimeError. - tests: existing reconciliation tests updated to the claimed_at contract; two new race-guard tests (previous-run terminal row never releases a fresh claim; missing claimed_at fails closed). Mutation-verified: removing the ownership guard fails both.
…timestamp helpers
/simplify-code findings on the salvage stack:
- efficiency HIGH: latest_executions() ran a SQLite connect + DDL + query
every tick for the whole duration of ANY running job, even when every
claim had a live future and the result was never consulted. Two-phase
now: snapshot (job_id, future) under _running_lock, query the ledger
only for claims whose future is missing/pending/done — the healthy
steady state pays zero DB work per tick.
- quality: inline ("completed", "failed", "unknown") tuple duplicated
cron/executions._TERMINAL_STATES (drift risk) — import the constant.
- reuse: hand-rolled naive-timestamp normalization in _row_belongs_to_claim
duplicated cron.jobs._ensure_aware's legacy-naive policy — reuse it.
- quality: dropped the tautological 'if fut is None or pending or done'
re-check (control only reaches it after the live-future continue) and
collapsed the two copy-pasted release blocks into one with a computed
reason.
24/24 tests green; mutation check re-verified on the final stack
(defeating the ownership guard fails exactly the 2 race-guard tests).
Healthy IPv4-first connect is the new default path, so two transports were warning on every successful initialize. Keep warning only when a literal actually failed first. Also restates the transport docstring and docs to match IPv4-first, hostname last.
…-map-kstawiski chore: add kstawiski to AUTHOR_MAP
…sResearch#86522) get_due_jobs() stamps a run_claim on one-shot jobs before returning them as due, and mark_job_run() clears it on successful completion. When dispatch itself fails (interpreter shutdown, executor submit error, execution-creation error) the job never reaches mark_job_run and the stale claim blocks re-dispatch until the TTL expires (default 30 min). Add clear_run_claim() to jobs.py and call it on every early-exit path in _submit_with_guard so the job stays due and fires on the next healthy tick — matching the existing scheduler comment's promise. Fixes NousResearch#86522
…ion tests Follow-ups on the NousResearch#87591 salvage: - cron/scheduler.py: wrap the three clear_run_claim call sites in a best-effort helper — clear_run_claim does load_jobs/save_jobs file I/O, and on the interpreter-shutdown path (or with a corrupt store) it could itself raise, defeating the skip-cleanly purpose of these early exits. A claim that can't be cleared simply expires at the TTL, as before. - tests/cron/test_oneshot_dispatch_failure_run_claim.py (new): 8 tests — clear_run_claim unit contract (one-shot cleared / already-clear noop / recurring never touched / unknown id), all three dispatch-failure paths through a real tick() clear the claim, and a raising clear_run_claim does not crash the tick. Mutation-verified: reverting the fix makes the suite fail.
/simplify-code finding: only one-shots carry a run_claim, yet the three dispatch-failure paths called clear_run_claim unconditionally — each call acquires _jobs_lock (blocking cross-process flock) and does a full load_jobs read just to return False for any non-'once' job. The trigger is exactly a failure storm (interpreter shutdown, EMFILE with N due jobs): N serialized flock+file reads at the moment the process can least afford I/O, all guaranteed no-ops for the majority job kind. Gate at the call site on schedule.kind == 'once'; new mutation-checked test proves recurring dispatch failures skip the claim I/O entirely. 9/9 tests green; ruff clean.
The PR's guard used `(job.get('schedule') or {}).get('kind')` which
crashes with AttributeError when schedule is a raw string (e.g.
'every 5m'), as happens in test_parallel_pool.py fixtures and any
job created via create_job(schedule='every 1h'). Use the
isinstance guard pattern already used at lines 5181 and 5325.
…cleanup-race fix: prevent handoff leg data loss + surface state.db corruption to users
… could not publish The rotation path flushes its un-persisted transcript to the parent (NousResearch#47202) and only then calls publish_compression_child. The abort handler rolls back the in-memory transcript and keeps agent.session_id on the parent - its own comment says "keep the parent live and discard the stale compacted snapshot" - but the rows the flush just wrote are not part of what it discards. Every failed rotation therefore leaves the parent transcript longer than it found it, whatever the failure was. That is survivable for a one-off failure and pathological for a sticky one. A parent row carrying ended_at fails the publish on every attempt and nothing in this path clears it, so each auto-compaction appends another copy of the current turn to the transcript it was supposed to shrink. Worse, the growth then satisfies conversation_compression's own len(durable_parent) > len(messages) check, so the next attempt adopts the inflated snapshot as if it were genuine concurrent activity and the in-memory transcript doubles too. Check that one precondition before writing. It is a plain read of the row the publish is about to read anyway, and it raises the publish's own message, so split_status=aborted, failure_class=session_split_failed and the rollback path are all unchanged; a live parent reaches the flush exactly as before. Deliberately not extended to the compression lease, which is re-acquirable - a transient miss there would abort a rotation that would otherwise have committed. old_session_id moves above the flush so a failure raised from here takes the same in-memory rollback as any other pre-publish failure. Scope: this fixes the amplification for every abort cause. It does not fix what marks a live session as ended in the first place (NousResearch#88197 Bug 1), which needs a maintainer decision on end-reason taxonomy and is tracked on the issue; an affected session still aborts every attempt, it just stops making itself larger while it does. Refs NousResearch#88197
finalize_subagent_worktree() treated a non-zero exit from its rev-list or status probes as proof of the payload defaults (commits=0, clean), then pruned on them: git worktree remove --force plus branch -D permanently deleted a child's uncommitted work whenever git could not inspect the tree (e.g. a corrupted index) (NousResearch#88113). A destructive cleanup now requires affirmative proof of zero commits plus a clean tree. Any non-zero inspection result keeps the worktree and branch for manual review, with a warning naming both.
…cted The preserved worktree is invisible to the only consumer that can act on it. Completes the NousResearch#88113 fix. That change correctly stops the destructive prune when a git probe fails, but still returns commits=0 / dirty=False -- values that were never measured. Those are the defaults the prune used to delete on, so the failure payload is byte-identical to "inspected fine, child left nothing": inspection FAILED, uncommitted work kept -> {commits: 0, dirty: False, pruned: False} inspected OK, child produced nothing -> {commits: 0, dirty: False, pruned: False} The only failure signal was a logger.warning, and the sole consumer of this payload is the parent agent reading the serialized delegate_task entry -- it cannot read logs (no in-repo code reads the key back). So the parent's rational reading of the failure case is "the child produced no work", which is the exact wrong conclusion: a worktree possibly full of uncommitted work is preserved and then never looked at. The data survives but nobody is told to recover it. Changes: - subagent_worktree: one _unproven() helper stamps inspection_failed + a note naming the worktree/branch, warns, and returns the payload. Both unproven exits route through it, so they cannot drift apart again. - subagent_worktree: the pre-existing exception path (timeout, OSError, a non-numeric rev-list stdout) produced the same unproven payload but logged at DEBUG -- effectively silent. It now takes the same flagged path as a non-zero exit; identical outcomes get identical reporting. - delegate_tool: the caller's finalize-raised fallback assigned the creation-side metadata dict (path/branch/repo_root/base_commit) -- a disjoint schema missing commits/dirty/pruned. It now emits the same flagged shape, and logs at WARNING. - Docs + docstring + module contract now state that pruning requires affirmative proof, so a future cleanup doesn't "fix" the preserved worktree by restoring the unconditional prune and reintroducing this P1. Purely additive: the happy-path payload shape is unchanged, so no existing reader can break. Validation: - 18/18 tests/tools/test_subagent_worktree.py; 127 passed across the delegation suites (test_delegate, batch_validation, control_actions, timeout_diagnostic). - 3 new guards mutation-checked: neutering the flag fails all three; reverting the production file to pre-fix main fails all three. Restores checksum-verified. - E2E on real git: inspection-failure now returns inspection_failed=true with work intact on disk; proven-clean still prunes (pruned=true).
Review fold on the NousResearch#88113 follow-up. The new guards asserted implementation details that a strictly-better future change would break, and the second producer of the payload schema had no coverage at all. - The distinguishability test asserted the failure payload was byte-identical to the genuinely-clean one (`for key in commits/dirty/pruned: assertEqual`). That freezes the AMBIGUITY as a required property: emitting `commits: None` for "unknown" would improve exactly what NousResearch#88113 is about and fail the test. Now asserts what the parent actually depends on -- both keep the worktree, and only the flag separates them. - `assertNotIn("inspection_failed", ok_payload)` pinned key ABSENCE on the happy path, forbidding an always-present-but-False flag (a legitimately better JSON contract: stable key set for serializers). Now `assertFalse(...get("inspection_failed", False))` -- same coverage, tolerant of that refactor. - `assertIn("UNKNOWN", note)` coupled tests to one word of English prose, and was not even a cross-producer contract: delegate_tool's note said "state unknown" (lowercase), so a copy-edit broke the implied convention. Tests now assert the note names the worktree AND branch -- the actionable part for a human -- and both producers' notes were aligned to read as one contract. - The raises test never proved its patched seam ran (a future short-circuit before any git call would keep it green while proving nothing). Now checks `call_count` and mirrors the branch-survival + note-names-path legs its sibling had. - NEW `WorktreePayloadSchemaTests`: commit 2's whole point is the schema the parent reads, but delegate_tool's fallback -- the second producer -- was verified only by reading. It now AST-parses the real fallback dict literal and compares against live `finalize_subagent_worktree()` output, so the two producers cannot drift and the pre-fix leak (repo_root/base_commit, missing commits/dirty/pruned) cannot come back. - Docs/docstring drift: the flag has a second trigger (finalization itself raising, handled in delegate_tool), and the module docstring listed `inspection_failed` without `note`. Both corrected. - Extracted the duplicated 5-line "corrupt the index" setup into `_break_git_index()` beside the file's other module-level helpers. Validation: 19/19 tests/tools/test_subagent_worktree.py; ruff clean. New schema guard mutation-checked -- reverting delegate_tool's fallback to the pre-fix `dict(_worktree_info)` shape fails it. Restores checksum-verified.
…urce-reading test
Phase 2c fold. The schema guard added in the previous commit read and
AST-parsed delegate_tool's source, which AGENTS.md:1514 bans outright ("Never
read source code in tests" -- it passes when the implementation is subtly
broken and fails on a correct refactor). Extracting the shared factory the rule
prescribes removes the duplication the AST test was invented to police, so one
change resolves both.
- subagent_worktree: new module-level `mark_worktree_payload_unproven()` +
`unproven_worktree_payload()`. Both producers of this schema now call them,
so the payload cannot drift and the note string exists once.
- delegate_tool: the finalize-raised fallback calls the factory instead of
hand-building the dict (-16 lines). The re-import is guarded: the outer
`except` can be entered because the `from tools import subagent_worktree`
itself failed, in which case the name is unbound -- an inline fallback keeps
the flag rather than raising NameError and losing it.
- Test replaced with a BEHAVIORAL equivalent: it calls the real factory and
compares its key set against live `finalize_subagent_worktree()` output. Same
contract, no source reading, refactor-proof, and it actually executes the
code.
Also folded from the same review:
- Fail-closed on an unmeasurable commit count. With no `base_commit` the
rev-list probe never ran, `commits` kept its unproven 0 default, and a clean
tree still reached `git worktree remove --force` + `git branch -D` -- the
exact bug class NousResearch#88113 is about, on a public function that takes a
caller-supplied dict. Now returns un-inspected instead, with a test driving a
real child commit.
- Per-probe diagnostics: the note said only "rev-list/status non-zero". It now
names WHICH probe failed, its exit code, and a bounded git stderr tail, so
the parent (and the human) can act on first read.
- Dropped the redundant `inspection_ok` bool for a `failed: list` of reasons;
removed the duplicated index-corruption block in favor of the existing
`_break_git_index()` helper.
Validation: 21/21 tests/tools/test_subagent_worktree.py; ruff clean; ty clean
on subagent_worktree.py and 64-vs-64 unchanged on delegate_tool.py (all
pre-existing, verified against the base commit). All 6 guards mutation-checked
twice -- neutering the flag fails 6, reverting production to pre-fix main fails
the same 6. E2E on real git: clean still prunes; corrupt index keeps the work
and reports the real stderr; empty base_commit keeps a committed child.
…unmeasured /simplify-code residual. The note hard-coded "'commits' and 'dirty' are UNKNOWN", but the two probes fail independently: a bad base_commit fails rev-list while `git status` still succeeds, so `dirty` is a REAL measurement being reported as unknown. Safety was never affected (the worktree is preserved either way), but telling the parent a measured value is untrustworthy is its own kind of misreport — and it would push a human toward re-inspecting something already proven. `mark_worktree_payload_unproven()` now takes an `unmeasured` argument, and finalize tracks which probe actually failed. The raising path still disclaims both, because which probe raised is unknowable there. Validation: 22/22 tests/tools/test_subagent_worktree.py; ruff + ty clean. New guard mutation-checked (hard-coding "commits/dirty" back fails it).
…e roster The union agent roster (host.agents) enumerates EVERY registered connection, including the active gateway that already answered profiles.list. The plugin merger treated the active gateway's own agents as rows from other sources because a remote-primary desktop reports them with connectionKind 'remote', so every bot appeared twice (baseline) and kept growing with each refetch. Match union agents to the active gateway via the new primaryConnectionId field on the roster RPC response and annotate the local rows in place instead of appending phantom copies. Same-named profiles on genuinely separate sources (This device, other remotes) still get their own tagged rows, preserving the @name-device disambiguation rule. Fall back to the legacy connectionKind==='local' rule when primaryConnectionId is absent (older Electron builds), so single-source behavior is byte-identical. Fixes NousResearch#88344
…source-qualified row keys Layers on the salvaged NousResearch#88489 (@29206394) and NousResearch#88341 (@frizikk): - sdk: host.activeConnectionId() — registry id of the LIVE active gateway. The salvaged fix classifies against the registry primary; after the user activates a non-primary source's agent, profiles.list answers from THAT source and primary-based matching would duplicate the active source's agents again. Live id wins, primaryConnectionId is the fallback, the legacy kind==='local' rule covers older desktops. - plugin: roster/chip/picker list keys are botRowKey(bot) — source-qualified (connectionId, name) — so same-named agents on two genuine sources can never collide as duplicate React keys (the render half of the dupe-bots smear: name-keyed rows + duplicate names = repeated blocks every poll). Annotated active-source rows keep the plain-name key, so nothing remounts when a desktop gains the union roster. - tests: live-id-beats-primary regression, botRowKey stability, source-shape anchor refresh.
The live-checkout git mutation guard blocked history-rewriting git ops (checkout, reset --hard, rebase, cherry-pick, ...) in the running source checkout and its worktrees on every platform. The hazard it protects against is only real on Windows, where NTFS locks loaded module files and an in-place rewrite can corrupt the running process. On POSIX, open file handles pin the old inodes, so a checkout swap under a running process is safe, and the guard mostly taxed normal dev/salvage workflows with clone workarounds. - tools/self_repo_guard.py: add guard_active() -> os.name == "nt" - tools/terminal_tool.py: consult guard_active() before running the detector; detector logic and block message unchanged for Windows - tests: wiring tests force the guard on; new tests cover the POSIX pass-through and the platform predicate
Preserve owner-only recommendation isolation while adopting upstream dispatch concurrency and memory-pressure changes.
૮ >ﻌ< ა ci reviewran on 7e490ed — chore: sync upstream main
|
mtbitcr
marked this pull request as ready for review
August 17, 2026 18:15
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
20 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.
Summary
c86197e60798801f62986e4e59460b1272d0c687Verification
7e490edb0e6063eebdd9faf48b680f5b2f620449The unrelated case-colliding contributor-email checkout artifact on macOS is not staged and is absent from this commit.