Every topmost row carried dmarc=fail, so it failed on its verdict no
matter what the pin check did: restoring subdomain alignment on the
topmost field kept the test green (gate 2ab Low finding). The subdomain
and comment-smuggled rows now say dmarc=pass, so only the exact pin
rejects them and that mutation goes red.
Every sibling signature in the adapter spells the type through the
existing email_lib import; the bare Message import served one annotation
(gate quality Low finding).
The authserv_id pin now matches the topmost Authentication-Results host
exactly; main also accepted a subdomain of the pin. Operators who pinned a
parent domain for a provider that stamps from changing MX hostnames would
see every message dropped with no guidance (gate 2c Medium finding).
The adapter now trusts only the topmost Authentication-Results field and
requires a set authserv_id to match it exactly (no subdomain alignment),
so operators whose relay stamps a different top authserv-id see the
untrusted-authserv warning. Say so where the warning is documented.
The adapter fetched the full RFC822 payload (and decoded attachments) for
every unseen UID before deciding whether the sender was allowed, and
_verify_sender_authentication searched every Authentication-Results field
for one whose authserv-id matched (or was a subdomain of) the pin. A
forged lower field or an attacker-chosen child.<authserv_id> therefore
authenticated a spoofed From:.
- Only the topmost Authentication-Results field is authoritative; a set
authserv_id must match it exactly (RFC 8601 CFWS comments allowed via
_ar_clauses). Main's _NO_AUTH_RESULTS_REASON/_UNTRUSTED_AUTHSERV_REASON
constants are kept so _DROP_HINTS still resolves.
- _check_inbox fetches a bounded BODY.PEEK[HEADER.FIELDS ...] preflight
and runs _sender_accepted on the event loop before requesting RFC822;
rejected UIDs are marked \Seen without downloading the body.
Salvaged from #129772 and rebuilt on main: the PR skipped the preflight
under open access via self._open_access(), which main removed in
c2f595d6af (open access now also authenticates From:), so the gate always
runs here. Poll-loop test mocks now authorize their unauthenticated test
senders instead of forcing open access.
reclaim_worktrees loaded the git-cherry merge cache on every call, dry
runs included, though a dry run never revalidates. It also never saved
the verdicts revalidation added, so the next audit re-ran `git cherry`
for any tree whose head moved after the audit (gate r2 efficiency, Low).
Load it only for a real run and save it once after the loop when it
grew, as audit_worktrees already does. Verdicts are unchanged: the cache
is only a memo.
The attended reclaim's plain `worktree remove` (no --force) reads the
linked tree's index, so it re-hashes stat-dirty files through clean
filters. It ran under noninteractive_repo_git_env(repo_root), which only
discovers filters visible from the main checkout: a filter defined in the
tree's config.worktree (extensions.worktreeConfig) ran its command
(gate r2 2c, Medium). Build the env from the tree itself, as the startup
path already does, keep the tree when discovery fails, and run both the
plain remove and the submodule --force retry under it.
The security test now plants a tree-scoped clean filter on the trees the
startup sweep and the attended reclaim remove.
Gate finding (Low, quality): the "state changed since audit" message
printed only the fresh classification reason. When the branch or the
untracked list had changed, the verdict was still reap, so the message
read "state changed since audit: clean and fully merged/pushed", which
explains nothing. It now names the field that changed (verdict /
untracked files / branch) and gives the current reason. Also fixes the
stale "`remove --force` refuses" comment: removal is now usually a plain
`worktree remove`. The dead `if not refreshed` guard was dropped in the
previous commit.
Gate findings (Medium efficiency + Low reuse, convergent): the startup
sweep revalidated each reap tree with a fresh _classify_prune_candidates
call. Each call reloaded (and could rewrite) the on-disk merge cache and
built its own ls-remote memo, so every pushed-tier tree paid another
network ls-remote. A 3-tree sweep made 4 loads and 4 ls-remotes; it now
makes 1 of each. The attended reclaim seeded an empty cache and re-ran
`git cherry` for verdicts the audit had just computed.
_classify_prune_candidates takes keyword merge_cache / get_remote_heads.
When the caller supplies them it uses them and leaves the save to the
caller. _prune_stale_worktrees loads once, threads both through
_reap_prune_verdicts, and saves once. reclaim_worktrees seeds from
_load_worktree_merge_cache(). Cache keys include both SHAs, so post-audit
commits still miss. Revalidation still runs per tree, right before
removal. The dead `if not refreshed` guard goes away: a one-item list
always yields one verdict.
Gate finding (Medium, 2c + quality convergent): plain `git worktree
remove` always refuses a tree with a checked-out submodule ("working
trees containing submodules cannot be moved or removed"). Once --force
was dropped, clean merged trees like that got a reap verdict on every
startup and every `hermes worktree prune`, and were never removed.
Both paths still try the plain remove first. Only when its stderr names
submodules and an immediate re-check finds the tree still clean
(_worktree_is_dirty False on startup; _dirty_split == (False, []) in the
attended path) do they retry with --force, under the same safe env.
Adds a submodule row to the existing parametrized startup test.
Gate finding (Low, 2c): the stack carried three test functions against a
two-invariant budget. The include-symlink-only tree is one more row of
the startup reclaim invariant (removed when only our scaffolding is
untracked, kept when real work was added), so it becomes a parametrize
case of the existing startup test. It still goes red with 01364366b5
reverted.
Gate finding (High, 2ab): dropping --force from the startup reap's
`git worktree remove` makes git read the tree's index, which executes the
repository's core.fsmonitor (GHSA-7x36-8jrh-v4pw) on the unattended
startup sweep. tests/security/test_gitspawn_config_injection.py went red.
Route the remove through noninteractive_repo_git_env(entry) +
stdin=DEVNULL, the same pattern as _worktree_is_dirty; when the env
cannot be built, keep the tree. Extend the security test to also drive
the attended reclaim_worktrees remove (worktree_gc._git already uses the
safe env) so both plain-remove paths are pinned.
The startup pruner now runs `git worktree remove` without --force so a
tree that gained work after classification is refused. But git treats
our own .worktreeinclude directory symlinks (node_modules, .venv) as
untracked content, so every include-symlink-only tree that the
classifier already reaps was refused and kept forever.
Unlink exactly the paths _include_symlink_paths() recognises (the same
classification _worktree_is_dirty uses) right before the remove; any
other untracked file still blocks the removal.
Gate BR3 r1 reuse (Low): build_update_processor hand-rolled the int-or-
default parse that gateway.platforms._shared.coerce_port already does.
Neither caught OverflowError, so `max_concurrent_updates: .inf` in YAML
aborted connect(). coerce_port now treats non-finite values as unparseable
(as gateway.config._coerce_num already does), which also hardens its other
port/int callers; the telegram setting warns and falls back to 32.
Gate BR3 r1 quality (Low): PTB's SimpleUpdateProcessor already provides the
no-op initialize/shutdown the class copied, and config.extra is always a
dict, so the `(extra or {})` guard was dead. The wiring test only compared
PTB's concurrent_updates property with itself (true for any processor);
assert the PerChatUpdateProcessor type and DEFAULT_MAX_CONCURRENT_UPDATES
instead - now red when a plain SimpleUpdateProcessor(32) is wired in.
PerChatUpdateProcessor awaited the predecessor's tail future directly, so
cancelling a waiting same-chat update also cancelled that future (gate
BR3 r1 2c + quality, Medium). The running predecessor then raised
InvalidStateError from done.set_result() after its handler succeeded, and
the next waiter started while the predecessor still ran (probe order:
start A, start C). Shield the await and, for a cancelled waiter, release
its turn only once its predecessor finishes.
connect() now calls builder.concurrent_updates(PerChatUpdateProcessor),
which broke 14 existing connect tests (gate BR3 r1 2ab, High):
- MagicMock builders returned a fresh mock from .concurrent_updates(), so
.build() no longer returned the test app (7 in test_telegram_conflict,
1 in test_gateway_platform_event_hook - the unidentified 8th);
- _LifecycleBuilder/_Builder had no such method (6 in polling_progress);
- the gateway conftest's MagicMock telegram stub made BaseUpdateProcessor a
mock, so the PerChatUpdateProcessor subclass was itself a MagicMock whose
second instantiation raised StopIteration. Give the stub a real base class.
Plain PTB concurrency (concurrent_updates=32) unblocks other chats while one
chat's handler is slow, but also lets that chat's NEXT update dispatch and
deliver before the first one finishes (download/vision/callback work happens
inline, before text batching can merge them).
PerChatUpdateProcessor (update_admission.py, not the 7k-line adapter) chains
each update behind the previous update of the same chat while keeping the
bounded cross-chat pool. The builder keeps the instance, so the connect-retry
rebuild in _initialize_app_with_retries reuses it. An invalid
max_concurrent_updates now logs a warning instead of silently falling back
(as #74652 did), and the dispatch-stall watchdog comment no longer claims PTB
dispatches sequentially.
Co-authored-by: fortona-space <299084105+fortona-space@users.noreply.github.com>
Gate finding BR4 r1 quality (Low cleanups): started_at is REAL NOT NULL, so
`r[3] or 0` in the sort key is dead; the user rank is a class constant, so
read it from _TITLE_SOURCE_RANK instead of the NULL-aware _title_rank helper.
The ranked repair test now closes its raw sqlite3 handle
(contextlib.closing) before SessionDB reopens the file -- `with connect()`
only commits, leaving a Windows file-lock / ResourceWarning hazard.
Gate finding BR4 r1 quality (Low): the repair used to be one atomic UPDATE;
it is now a loop of per-row UPDATEs inside a try/except that only logs. A
failure part-way left the rows already renamed/cleared in the open init
transaction, which _init_schema then committed without the unique index --
a half-repaired store persisted without the guarantee.
Wrap the repair in SAVEPOINT title_repair; RELEASE after the CREATE INDEX,
ROLLBACK TO + RELEASE in the except before logging. The ranked repair test
gains a broken-rename row that pins the all-or-nothing outcome.
Gate finding BR4 r1 2c (Medium): when two user-owned rows shared a title the
repair kept the base title on the newest row and renamed the older one
"Title #2". resolve_session_by_title prefers any "Title #N" continuation, so
`hermes -c Title`, gateway /resume and session_search then opened the OLDER
session (on main they reached the newer one).
The oldest user row now keeps the base title and newer user rows are renamed
in ascending started_at via next_title_in_lineage, so the "#N"-preferring
lookup lands on the newest session again. User still beats llm/derived and
lower-ranked auto titles are still cleared.
The duplicate-title repair that runs before creating idx_sessions_title_unique
kept the highest-rowid row and NULLed every other title, including names the
user typed (and pre-provenance NULL-source rows, which _title_rank treats as
user). A newer session with a lower rowid also lost its title.
Per duplicate title, keep the row with the highest _title_rank, newest
started_at first; clear lower-ranked auto (llm/derived) titles and rename
user-owned duplicates via the existing lineage numbering ("Title #2"), logging
each change. The index is then always created. get_next_title_in_lineage now
delegates to a connection-level helper so the schema init can reuse it.
Fixes#126764.
Gate 2c finding (Low): control verbs with trailing words now win, so goal
text that starts with a control word needs `/goal -- <text>`. The docs
covered it but autocomplete and /help did not; add it to args_hint.
Gate reuse + quality findings (Low, convergent): is_goal_control checked
_BARE_CONTROL_VERBS plus an inline {"wait", "gate"} literal. Hoist a single
module constant _CONTROL_FIRST_WORDS so the busy-path classifier reads one
verb set next to the dispatcher's. No behaviour change.
Gate quality finding (Low): the trailing-words test only checked the goal
text and a conditional hint, so mapping `continue` (or `pause ...`) to
_status would still pass. Each param now names the expected status
(resume/continue on a paused goal -> active, pause -> paused,
status -> active) and every case carries trailing words, so the
ignored-words hint is asserted unconditionally with the exact text.
Gate 2c finding (Low): the widened is_goal_control (used by the gateway's
busy path to dispatch rather than reject `/goal pause for now` mid-run)
had no test; reverting it stayed green. Assert it per param in the
existing trailing-words test.
Gate quality finding (Low): `arg.startswith("--")` also matched goal text
that starts with a flag-like word, so `/goal --dry-run the migration`
silently set "dry-run the migration". Match only the standalone `--`
token (the documented `/goal -- <text>` form) and reuse the parsed rest,
so flag-like text is kept literally as on main.
Gate quality finding (Medium): _set read mgr.state.goal regardless of status,
so `/goal clear` (or a done goal) followed by `/goal <text>` printed
"(replaced the previous goal) was: ..." about a goal that was already gone.
Use mgr.has_goal() (active/paused) to decide what counts as replaced.
Control verbs with trailing words now run the verb instead of replacing
the goal, so goal text that starts with one of those words needs the
`--` escape. Without a docs line the change is only discoverable from the
in-chat hint.
The rebuild only checked that the goal text survives; dropping the
'(ignored ...)' hint from _bare_control survived mutation (review on
#127367). Assert the hint appears exactly when trailing words were given.
`/goal resume last goal`, `/goal continue` on a paused goal and
`/goal pause for now` fell through to the set path and silently
overwrote a standing goal with that sentence. The non-destructive
control verbs (resume/continue/unpause/pause/status/show/unwait) now win
over goal text and report the ignored words; `/goal -- <text>` sets goal
text that really starts with a control word, and a plain replace names
the goal it replaced. clear/stop/done keep their existing behaviour.
Hand rebuild of the goal_command.py part of #127367 (criteria parsing,
/goal history and the clear/stop refusal are left out).
(cherry picked from commit 9a9731437369138bbf4f84362556ae634ab27d6f)
Gate reuse + quality finding (Low, convergent): start_owner_watchdog's
loop was a line-for-line copy of start_code_skew_watchdog's (confirmation
counter, max_polls bound, fence prepare -> commit -> should_exit, daemon
thread). A change to the fence handshake would have had to land twice.
Move it into web_server_skew_exit._run_retirement_watchdog(server, *,
observe, fence, poll_s, max_polls, thread_name), where observe returns
the retirement log reason while retirement is warranted. Both public
start_* functions keep their signatures and become thin callers with
their own predicate and message. Behaviour is unchanged; the existing
skew and owner tests stay green and both go red if the shared
confirmation count drops to 1.
Gate 2ab + 2c finding (Low, convergent): both owner-exit tests drive
start_owner_watchdog / should_retire_superseded directly, so dropping the
ssh_lock_path threading in cmd_dashboard or the start_owner_watchdog arm
in _on_server_started left every kept test green and the fix unwired.
Extend two existing tests instead of adding new ones:
- test_web_server_ws_ping's SSH bootstrap test asserts start_server arms
the watchdog with lock_path/nonce when ssh_lock_path is given, and not
when it is None.
- test_resource_limits' serve-startup test runs a Desktop SSH serve and
asserts the lock path handed to start_server is the token's
desktop-ssh/<ownershipId>/backend.lock.json.
Mutation-checked: removing the web_server arm, the ssh_lock_path pass to
_on_server_started, or main.py's ssh_lock_path each turns one red.
Gate reuse finding (Low): web_server_owner_exit had its own MIN_AGE_S=90
for the same thing dashboard_procs._REAP_MIN_AGE_SECONDS (180s) already
names: the gap between a serve process starting and the Desktop writing
backend.lock.json. Two constants for one window can drift apart. Use the
reaper's constant (already imported cross-module by web_server, so the
private name is kept rather than renaming every use). This only delays a
lingering superseded backend by up to 90s; it cannot cause a false
retirement. The existing verdict test now pins the shared window.
Gate quality finding (Low): the bare "backend.lock.json" literal now sat
in main.py (the owner watchdog's lock path) and dashboard_procs (the
reaper), both of which must stay in step with the Desktop writer. Add
BACKEND_LOCK_NAME next to _REMOTE_LOCK_SUBDIR and use it in both.
windows_ssh_runtime keeps its own literal (separate Win32 runtime; left
as is, optional per the review).
Gate efficiency finding (Low): read_valid_backend_lock read the whole file
and only then rejected anything over 64 KiB. It now also runs on every
15s owner-watchdog poll for the life of an SSH-isolated backend, so an
oversized/garbage backend.lock.json was read in full each time. Read at
most 65537 bytes; >65536 still returns None, smaller files read the same,
and the is_file() pre-check stays (it keeps the read off a FIFO).
Gate quality finding (Low): the web_server_owner_exit docstring carried
contributor credit and prior-art history. Docstrings keep the WHY; the
credit (@VexterZ, #89452) already lives in the fix commit's body.
A Desktop reconnect that cannot prove the recorded pid is its own (dead-looking
pid, FOREIGN argv proof) drops desktop-ssh/<ownershipId>/backend.lock.json
without signalling that pid and spawns a new nonce. The superseded serve had no
signal that it lost ownership: it lived until the idle watchdog fired (900s
grace, gated on zero WebSockets, which a half-open tunnel keeps non-zero for up
to the 600s ping timeout), so a reconnect storm stacked detached backends, each
an extra state.db writer (#132034).
web_server_owner_exit mirrors web_server_skew_exit: a pure decision (retire only
when a VALID lock names another spawnNonce and the process is past the reaper's
180s settle window; missing, unreadable or skewed locks keep it up) and a daemon
watchdog that needs two consecutive observations, then exits through the
retirement fence so it never tears down mid-turn and admission is closed first.
The lock is read through dashboard_procs' existing validator (split out as
read_valid_backend_lock, shared with _lock_owned_serve_pids) rather than a
second parser, and its path is the validated --ssh-session-token-file's
ownership directory. Server-side only: cleanupStale is untouched, so this
composes with #126035.
Root-cause analysis and watchdog design from the #132034 report by @VexterZ;
lock-ownership watchdog first drafted in #89452 by @pmwanje2 (prior art).
Refs #132034 (POSIX SSH hosts; Windows-host locks are not validated yet)
The /undo|rewind filter rewrote only messages; timings had been built from the
unfiltered rows and still named the dropped rows' ids, roles and gaps. One
drop_undone_rows() helper in session_export now filters and rebuilds timings for
both /save paths (import is unaffected: import_sessions ignores timings).
The E4 2c review found the round-trip test stays green when /save json
uses the display projection (include_compacted) instead of the transfer
projection: on the old fixture both hold the same (role, content) pairs.
Once /save drops rewound rows, the two projections differ only on rows the
display read hides, so the fixture gains a model-only live row (a
micro-compaction merge) and the test asserts the snapshot's ids equal every
stored active-or-archived row. The always-include_compacted mutant is now
red on both cli and gateway rows.
The E4 2c review found the transfer projection also carries rows /undo,
rewind or edit removed (active=0, compacted=0), and the gateway sends the
/save file into the chat, so a turn the user deliberately took back rode
along in the document. On main /save json never held such a turn. Filter
the /save JSON (CLI and gateway) to active or compaction-archived rows;
import_sessions restores each row's flags, so archived turns still come
back as archived history. `hermes sessions export` keeps full-backup rows.
The E4 2c review (convergent with quality) found /save json now loads the
transfer projection -- every stored row, archived included -- with no
sessions.max_export_messages guard, while `hermes sessions export` and the
console export refuse oversized sessions before materializing that same
projection. A heavily compacted session could load its whole archive into
memory from one chat command. Both /save surfaces now call the existing
assert_export_safe() before a JSON export and reply with the shared
SessionExportTooLargeError text; md/html transcripts are unaffected.
