stabilize Matrix voice caching, Slack connect handlers, website policy cache key, and Codex prefligh - #3923
Conversation
…gnore supply-chain comment failures
…bust Slack auth_test; cache key uses Hermes home
…cy-cache-key fix(website-policy): key blocklist cache on real default config path (salvage #3923)
The cache used a '__default__' sentinel as its path key, so switching HERMES_HOME (profiles, tests) within one process kept serving the stale policy loaded from the previous home. Key the cache on the actual resolved default config path instead, so a home/config-path change naturally misses the cache. Trimmed from bundled PR NousResearch#3923 (the other sub-fixes are superseded on main); authored by @aydnOktay.
The cache used a '__default__' sentinel as its path key, so switching HERMES_HOME (profiles, tests) within one process kept serving the stale policy loaded from the previous home. Key the cache on the actual resolved default config path instead, so a home/config-path change naturally misses the cache. Trimmed from bundled PR NousResearch#3923 (the other sub-fixes are superseded on main); authored by @aydnOktay.
…website-policy-cache-key fix(website-policy): key blocklist cache on real default config path (salvage NousResearch#3923)
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the bundled stability fixes. Current main has already absorbed or superseded several parts, but two stated guarantees need rebasing onto the current architecture.
Problems
- The Slack adapter moved out of
gateway/platforms/slack.pyin560010547; the active code still unconditionally awaitsclient.auth_test()atplugins/platforms/slack/adapter.py:1060, so the sync-mock compatibility does not reach current code. - The Codex request builder moved behind
run_agent.py:5328.agent/agent_init.py:384retains an empty model, andagent/codex_responses_adapter.py:802-804rejects it at preflight. The current gateway has a catalog-based fallback atgateway/run.py:3842-3855, so the hardcodedgpt-5.3-codexfallback should be reworked against that mechanism. - The Matrix behavior is now stronger (
plugins/platforms/matrix/adapter.py:2862-2937caches AUDIO and VOICE), the CI comment is already non-fatal (.github/workflows/supply-chain-audit.yml:155), and the website-policy portion landed ind91083b2f.
Suggested changes
- Port only the Slack/Codex goals to the active paths and add focused regression coverage; omit the landed or superseded hunks.
Automated hermes-sweeper review.
|
|
||
| # Get our own bot user ID for mention detection | ||
| auth_response = await self._app.client.auth_test() | ||
| # `auth_test()` may be an awaitable coroutine or a sync method |
There was a problem hiding this comment.
This adapter was moved to plugins/platforms/slack/adapter.py by 560010547; port the sync-or-awaitable handling to the active connect loop there (plugins/platforms/slack/adapter.py:1060) or this change will not affect current main.
| reasoning_effort = self.reasoning_config["effort"] | ||
|
|
||
| # Codex Responses contract requires a non-empty `model` field. | ||
| # Some callers (notably tests and runtime-provider resolution) |
There was a problem hiding this comment.
Current run_agent.py now delegates _build_api_kwargs to agent.chat_completion_helpers (run_agent.py:5328), so this fallback must be relocated. Prefer the existing provider-catalog fallback pattern in gateway/run.py:3842-3855 over a hardcoded stale Codex model.
The cache used a '__default__' sentinel as its path key, so switching HERMES_HOME (profiles, tests) within one process kept serving the stale policy loaded from the previous home. Key the cache on the actual resolved default config path instead, so a home/config-path change naturally misses the cache. Trimmed from bundled PR NousResearch#3923 (the other sub-fixes are superseded on main); authored by @aydnOktay.
…website-policy-cache-key fix(website-policy): key blocklist cache on real default config path (salvage NousResearch#3923)
The cache used a '__default__' sentinel as its path key, so switching HERMES_HOME (profiles, tests) within one process kept serving the stale policy loaded from the previous home. Key the cache on the actual resolved default config path instead, so a home/config-path change naturally misses the cache. Trimmed from bundled PR NousResearch#3923 (the other sub-fixes are superseded on main); authored by @aydnOktay.
…website-policy-cache-key fix(website-policy): key blocklist cache on real default config path (salvage NousResearch#3923)
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Summary
Two PRs are associated with this complex, but they address distinct causes: #3923 bundles several stability fixes, most of which are already landed or target obsolete paths, while #53653 introduces an opt-in URL allowlist whose current diff does not enforce the policy across redirects and sibling navigation paths.
Related pull requests
- #3923
related— (+59/-7) — rebase and narrow: Matrix handling is superseded by the stronger current adapter, the CI behavior and website-policy cache fix have already landed, and the Slack/Codex changes target moved code or use an unsuitable hardcoded fallback. Consistent with the keep_open review on #3923, retain only the Slack sync/async compatibility and Codex non-empty-model goals, ported to the active architecture and catalog-based fallback. - #53653
related— (+122/-0) — revise before merge: the diff adds only a pre-navigation allowlist check, so redirects, browser_back, and raw CDP navigation can bypass the claimed boundary; it also lacks configuration exposure, documentation, and the stated tests. Consistent with the keep_open review on #53653, preserve the feature proposal but require enforcement on final URLs and every sibling navigation path.
Suggested consolidation
Do not consolidate or merge either PR as submitted: #3923 should be rebased and reduced to its two still-relevant fixes, while #53653 should remain a separate feature PR and complete end-to-end navigation enforcement, configuration, documentation, and tests. Neither PR can be closed as a duplicate of the other.
Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 12 kB of PR diffs, 2 kB of issue/PR text, 3 kB of discussion (2 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.
The cache used a '__default__' sentinel as its path key, so switching HERMES_HOME (profiles, tests) within one process kept serving the stale policy loaded from the previous home. Key the cache on the actual resolved default config path instead, so a home/config-path change naturally misses the cache. Trimmed from bundled PR NousResearch#3923 (the other sub-fixes are superseded on main); authored by @aydnOktay.
…website-policy-cache-key fix(website-policy): key blocklist cache on real default config path (salvage NousResearch#3923)
|
Closing to concentrate review on a single linked product bug: #89979. |
Caching Matrix voice notes (AUDIO) as local files instead of leaving them as HTTP MXC URLs.
Making Slack auth_test() handling robust to sync vs async return types in tests/mocks.
Ensuring website_policy cache invalidation respects HERMES_HOME.
Preventing Codex Responses preflight failures by guaranteeing a non-empty model in request kwargs.
Changes are intentionally scoped to only the files required for the failing tests + CI supply-chain comment non-critical robustness.