Repository navigation
fix: align access policy and harden agent loops - #12
Conversation
Blocking: OpenMax billing gate incorrectly blocks externally hosted Hermes agentsI reproduced the source of the following user-visible reply from the PR/runtime code:
This is not a Hermes/model response. It is the hard-coded GET /api/v1/billing/plan-state
usage_snapshot.enforcement_suspended == trueThe bridge then skips For
This produces a false billing reply while the Agent/WebSocket can still be shown as online, and discards the user's message without ever invoking Hermes. There is a second replay issue: the billing notice is emitted for Requested change
I consider this merge-blocking because it causes valid external-agent messages to be consumed without delivery and sends users an inaccurate explanation. |
zylos-luna-coco
left a comment
There was a problem hiding this comment.
Code Review — PR #12: align access policy and harden agent loops
Verdict: Approve
This is a well-structured security-hardening PR that tightens defaults, adds defense-in-depth agent loop guards, and fixes several correctness issues around delivery ordering and billing gate scope. The test coverage is thorough (154+ tests), the changes are internally consistent, and the breaking changes are intentional and documented.
Key Findings
Important (no blockers, but worth tracking)
1. mode=open is silently dropped as a valid group mode
_VALID_GROUP_MODES = {"mention", "smart", "silent"} — any existing group configured with mode=open (e.g., via a historical config event or persisted policy.json) will now be normalized to mention on load, which changes behavior from "receive everything" to "require @mention". The test for invalid config events (test_invalid_config_events_do_not_mutate_persist_report_or_callback) correctly rejects mode=open as invalid. This is fine as a deliberate tightening, but worth a note in migration docs since anyone who was using mode=open will see changed behavior.
2. Recursive _deliver_by_id in the inflight-wait path
When /sync encounters a realtime delivery still in progress and that delivery fails, the sync handler recursively calls _deliver_by_id. The recursion is bounded (it can only recurse once since the retry will either succeed or fail cleanly without re-entering the inflight path), and the _inflight/_inflight_done cleanup in the finally block ensures no leaked state. Verified correct, but worth a comment noting the single-depth recursion invariant for future maintainers.
3. Breaking default changes for existing unconfigured installations
dm_policy: "open" → "owner" and group_policy: "open" → "allowlist" are security improvements, but any installation that relied on the old permissive defaults without explicitly setting CWS_DM_POLICY / CWS_GROUP_POLICY will become restrictive after upgrade. Persisted policy.json state will preserve the old values if they were saved, but fresh starts will lock down. The adapter's _policy_from_env() handles this correctly. Migrations should note this.
4. Agent-to-agent integration now requires CWS_ALLOWED_AGENT_SENDERS
Previously CWS_ALLOW_AGENT_SENDERS=true was sufficient. Now the sender must also appear in CWS_ALLOWED_AGENT_SENDERS (fail-closed empty list). Existing agent-to-agent integrations will break unless reconfigured. This is documented in the PR description and plugin.yaml, and is the right security posture.
Minor
5. send_image_file has a local import of new_client_msg_id
(bridge.py, inside the function body) — new_client_msg_id is already available at module scope via from .codec import .... The local import is harmless but unnecessary.
6. agent_turn_window_s env var is cast through positive_int then float()
(adapter.py) — float(positive_int("CWS_AGENT_TURN_WINDOW_S", 60)) truncates fractional seconds. Operators likely expect integer seconds, but worth noting in the env description or using a direct float parse if sub-second precision matters.
7. Defensive getattr/hasattr for _agent_causation in adapter
The __init__ already initializes _agent_causation, so getattr(self, "_agent_causation", {}) in _with_agent_causation and hasattr in _on_inbound are redundant. Likely defensive for hot-upgrade scenarios where an older __init__ ran; fine to keep but could use a brief comment.
Nit
8. Wrapped line in README
"Group messages therefore\nintentionally have no per-member Hermes user_id" — the sentence break at "therefore" reads slightly awkward across the line wrap.
What I verified
- Policy ordering:
disabledis now absolute (no owner bypass); owner mention bypass applies only for unregistered groups in allowlist mode; owner is exempt from registered groupallowFrom. All three are tested and correct. - Agent loop guards: hop validation is strict (rejects bool, float, 0, negative), duplicates use content fingerprinting, turn budget is per-sender-per-conversation with time expiry and capacity bounds. Defense-in-depth layering is sound.
- Silent mode: Bridge-only observation confirmed — no sender name resolution, no attachment hydration, no ack reaction, no model delivery. History caching is bounded. Watermarks advance correctly.
- Delivery ordering: Realtime frames no longer commit the global inbox cursor;
/syncreplays advance it in server order under the sync lock. The inflight-done event mechanism correctly handles the race between realtime and sync delivery of the same message. - Billing gate: Default flipped to
False, adapter explicitly passesFalse, sync replay never emits billing notice. All three regressions are present. - Config event validation: All six event types validated before mutation/persist/report/callback. Invalid events short-circuit with a warning log.
- Policy persistence normalization: Corrupt
policy.jsonvalues are sanitized on load, falling back to safe defaults. Thetest_corrupt_persisted_policy_falls_back_to_safe_normalized_statetest covers this well. - Boundary-aware mention matching:
@Namedoes not match@NameSuffixor@Name-team; aliases work; Agent group traffic requires structured mentions only. - Test coverage: 15+ new test functions covering defaults, owner ordering, agent guards, config validation, delivery ordering, billing, silent mode, reject notices, persistence normalization, outbound metadata, directory plugin bootstrap, and WS connect-wait.
Clean approve. Solid work.
What & why
Hermes v0.1.5 aligns most human DM/group policy surfaces with Zylos, but still had unsafe default drift, contradictory owner bypass ordering, substring mention matches, non-live local policy tools, model-invoking
silent, and no fine-grained Agent loop circuit breakers.This PR:
owner, group=allowlist,disabledis absolute, owner mention bypasses only missing group registration, and owner remains exempt from registered-groupallowFrom;silentbridge-only observation: bounded admitted text history + watermark advance, without attachment/work hydration, billing, ack reaction, Hermes session/model delivery, or reply;The vendored v1 contract files remain unchanged per
contract/PROVENANCE.md; the local runtime overlay documents thatsilentremains policy-admitted (handle:true) but is consumed before the Hermes host callback.Type of change
How was it tested?
169 passed, 1 live deselected154 passed, 1 live deselected15 passedpython3 -m compileall -q cws_agent_sdk hermes_openmax testsgit diff --checkSecurity checklist
Reviewer notes
CWS_ALLOWED_AGENT_SENDERSare required, and Agent group traffic must directly mention this member.silenthistory is in-memory and bounded; it deliberately does not hydrate attachments or invoke the model.