fix(a2a): carry #778 reply deadline to production seat - #1
Merged
Merged
Conversation
Collaborator
Author
|
Reviewer gate: reviewer-a inspected the identical upstream candidate at 5419150 and confirmed the parsing, reply-wait, store, and watchdog behavior; discussion: NousResearch#91686 (comment). Production cherry-pick c6374cc is 111/111 green. |
RuniThomsen
added a commit
that referenced
this pull request
Aug 23, 2026
Reviewed once and verified 111/111. Makes the 900-second Hermes-seat reply deadline and matching orphan watchdog config-native.
RuniThomsen
pushed a commit
that referenced
this pull request
Sep 24, 2026
…es for the review in that repo's issue #1
RuniThomsen
pushed a commit
that referenced
this pull request
Sep 24, 2026
…, with or without the multiplex flag Two authority gaps in served_profile_child_env (NousResearch#111617 review, andrexibiza P1 #1/#2, kvnloo finding 1): - The base was hermes_subprocess_env(inherit_credentials=True) = the launch environ's provider credentials; strip_launch_profile_env only knows names with .env/source provenance, so a key systemd/Compose/the shell injected into the launch process survived into profile B's child whenever B did not define the same name. Now a ROUTED target scrubs every Tier-1/Tier-2 credential from the base regardless of provenance before B's own scope is overlaid (the child boundary gets get_secret's contract: a scoped miss is no credential, never ambient fallback). The launch profile's own child keeps its env. bot_relay's base=os.environ goes through the same scrub. - strip_launch_profile_env / the scrub keyed on is_multiplex_active(); the Desktop and dashboard backends serve ?profile=B by installing the HERMES_HOME override without that flag, so B's slash worker / helper children kept A's .env and settings. The authority test is now "is the target a routed home" (target != process home). - _build_browser_env resolved the passthrough keys via get_secret, which falls through to os.environ on a scoped miss while multiplexing is inactive: a routed B with no Firecrawl key got A's. Under serves_routed_profile() the bound scope is the only source. - served_profile_child_env(inherit_credentials=True) with no target and no scope bound under multiplex minted with the launch credentials (key_cmd TTL refresh on a worker thread); it now raises UnscopedSecretError like get_secret. tests/tui_gateway/test_served_profile_child_env_authority.py: ambient-only A key + B missing it (mux on), flag-off routed B (helper child + browser), real child observation. 3/3 red on base.
RuniThomsen
pushed a commit
that referenced
this pull request
Sep 24, 2026
…mple misses after delivery (NousResearch#105861) The post-delivery fire-claim check is a single sample of jobs.json. When it missed on a run whose notice had already reached the platform, _record_fire_ownership_lost() overwrote the delivered success with 'Interrupted by shutdown before terminal completion.' -> last_status: error, and the operator's health watchdog then alerted on every tick for a job that was working (NousResearch#105861, 3 confirmed incidents). Suggested fix #1: when the run's delivery actually completed (delivery attempted, no delivery error, non-empty terminal response), log the ownership loss as a warning and fall through to _finish_completed_run() -- whose owner-fenced mark_job_run() is the authoritative claim check, recording ok while the claim is still held and nothing at all when it really is gone. A claim lost *during* a side-effect fence (delivery did not complete), a failed delivery, a run that never delivered, and a claim lost before delivery all keep the ownership-loss error path unchanged. Review follow-up: fire_claim_lost ORs the sampled claim with the transport-level cancel_event, so the flag alone can't say which one fired. _run_one_job_body now also receives transport_cancel and the post-delivery exemption is scoped to the sampled path only -- an explicit transport cancel (dashboard drain / shutdown) stays fail-closed and still records the interrupted run. Regression added: test_transport_cancel_during_delivery_stays_fail_closed sets the external event during delivery and asserts last_status=error / 'Interrupted by shutdown before terminal completion.' Review follow-up 2: the transport side of that flag can't be read off the event either. _FireOwnership.lost() latched a sampled loss by calling set() on fire_claim_lost, which with a transport cancel in play is _CombinedCancelEvent(lost_ownership, cancel_event) -- and _CombinedCancelEvent.set() sets every source it ORs. A pure sampled miss therefore made the transport event read as cancelled, the exemption above was unreachable, and the drain event the *caller* owns was mutated by this run's bookkeeping. _FireOwnership now also receives the sampled source itself (sampled_claim_lost, threaded from run_one_job's lost_ownership) and latches that one event: the combined wrapper keeps its OR semantics and is_set() still reports the loss, so the in-flight agent / pre-run script is interrupted exactly as before -- only the source identity is preserved. Regression added: test_sampled_miss_leaves_an_idle_transport_event_alone (real claimed job, successful delivery, miss_at_sample=2, initially unset threading.Event) asserts last_status=ok, last_error=None and the transport event untouched; it fails on the previous head with the reported symptom.
RuniThomsen
pushed a commit
that referenced
this pull request
Sep 24, 2026
compact_ledger, gc_blobs and list_entries each stamped the same prelude: read the ledger, catch (OSError, UnicodeError), warn "skill_ledger: ledger unreadable (%s); <what>", bail (tools/skill_ledger.py:305-310, :341-346, :412-417 — simplify reuse #1). Fold them into a private sibling _read_ledger(what, *, quiet_missing=False) -> Optional[bytes] that reads bytes, validates the UTF-8 decode and warns once unless quiet_missing and the ledger is merely absent (list_entries: a fresh install has no ledger). Returns bytes rather than str so compact_ledger keeps reporting the on-disk size in bytes_before instead of re-encoding. Warning texts ("compaction skipped", "blob GC skipped", "listing empty") are unchanged, so the existing invariant tests pass verbatim.
RuniThomsen
pushed a commit
that referenced
this pull request
Sep 24, 2026
test_list_entries_is_silent_when_the_ledger_is_merely_missing checked `"listing empty" not in caplog.text` (tests/tools/test_skill_ledger_delta.py:40-44; simplify quality #1) — a negative substring match that goes vacuously green if the message is ever reworded, and stays green if a *different* warning fires. Assert on caplog.records at WARNING or above instead, which checks the actual invariant (no warning for a merely absent ledger) independent of wording.
RuniThomsen
pushed a commit
that referenced
this pull request
Sep 24, 2026
…sweeps
gc_events and gc_worker_logs each carried the same three lines (int()
coerce, `< 0` check, raise ValueError(_NEGATIVE_RETENTION_MSG.format(...)));
only the message string was hoisted. Replace the constant with a
sibling-local helper `_retention_seconds(older_than_seconds) -> int` that
owns coerce + check + raise, and call it from both sweeps.
WHY: the comment explaining the rule ("a negative window puts the cutoff in
the future, so 'older than cutoff' matches everything") is the reason for
the check, so it belongs on the check rather than on a format string; and
a third sweep now has one place to reuse instead of a block to paste.
Message text is unchanged, so the existing
`pytest.raises(ValueError, match="older_than_seconds")` tests pass as-is.
Finding: simplify/D.reuse.md #1 + simplify/D.quality.md #1
(hermes_cli/kanban_db.py:4285-4287, :4303-4305).
Proof: mutating the helper's `< 0` to `< -10**12` fails
test_gc_events_rejects_negative_window and
test_gc_worker_logs_rejects_negative_window (DID NOT RAISE); head green.
RuniThomsen
pushed a commit
that referenced
this pull request
Sep 24, 2026
Loop run 7 at 22e8184 went red on "no sandbox process survives quit #1": a `git fetch origin --filter=blob:none --stdin` (+ its index-pack) was still running 60 s after quit. That is git's lazy promisor fetch: the dev checkout is a blob:none partial clone, and a git read by the backend needed a missing blob, so git went to the network, and the fetch outlived the backend (reported as a finding; same class as the gh probe). CI checkouts are not partial clones, so the spawned app gets GIT_NO_LAZY_FETCH=1: dev runs stay offline and deterministic like CI.
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
1 participant
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.
Production lane for NousResearch#778
Carries the already-reviewed upstream candidate from NousResearch#91686 onto the user-owned fork that backs this Mac's Hermes seat.
Change
gateway.platforms.a2a.extra.reply_timeoutReview
One independent reviewer examined the upstream diff at
5419150c839698c493958b66be5f477dffa1ee16; the upstream PR records the review discussion. This fork commit is a clean cherry-pick of that exact change onto current forkmain.Verification
scripts/run_tests.sh tests/plugins/test_a2a_plugin.py— 111 passedgateway.platforms.a2a.extra.reply_timeout: 900Deployment
After merge, the exact merged adapter/protocol will be loaded into
ai.hermes.gateway-wolf, then the running process and effective 900-second/watchdog values will be read back.