fix(gateway/photon): survive runtime platform outages — supervised reconnect watcher, reliable re-queue, configurable sidecar ready timeout - #19
Conversation
Security plugins (consent-boundary script-file email lane, repo_ecosystem_agents#697/NousResearch#698/NousResearch#700) resolve relative script paths against args cwd/workdir, but the live session cwd lives in the terminal environment (env.cwd), not the model args — so the lane was inert whenever the model omitted workdir. _hook_args_with_terminal_cwd enriches a COPY of the args for the hook call only (execution args never mutated; explicit workdir wins; best-effort on any failure). 4 tests; targeted suites 160 passed.
…connect watcher, reliable re-queue, configurable sidecar ready timeout Root-caused from the 2026-07-30 photon (iMessage) 3-day outage: - a retryable runtime fatal on an already-queued platform was silently skipped (no re-queue, stale next_retry) and a platform missing from the static config map was silently dropped — state stuck on 'retrying' while nothing retried - the reconnect watcher was an unsupervised create_task; a KeyError from concurrent _failed_platforms mutation (lookup outside the per-attempt try) killed it silently for the life of the gateway - the sidecar readiness window was hardcoded to 15s; under host load (loadavg 25+) the node import + Spectrum init cannot finish in time, so every reconnect attempt failed (2026-08-02) Changes: - gateway/run.py: _handle_adapter_fatal_error falls back to the adapter's own config, refreshes next_retry when already queued, and logs loudly when queueing is impossible; watcher pass extracted to _reconnect_pass with .get()-based tolerant iteration; watcher spawned supervised with crash-restart (5s delay) - plugins/platforms/photon/adapter.py: PHOTON_SIDECAR_READY_TIMEOUT env override, default raised 15s -> 60s; timeout value in the error message Tests: 10 new (test_reconnect_watcher_resilience.py, test_sidecar_ready_timeout.py); photon suite 117 passed; reconnect regression suites 32 passed. Issues: bbudiono/repo_hermes_primary#12, #13 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f80517fc9f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| block_message = get_pre_tool_call_block_message( | ||
| function_name, | ||
| function_args, | ||
| _hook_args_with_terminal_cwd( | ||
| function_name, function_args, effective_task_id or ""), |
There was a problem hiding this comment.
Terminal calls never reach this concurrent hook: _should_parallelize_tool_batch() returns false for every terminal batch because terminal is neither parallel-safe nor path-scoped, and single calls are also always sequential. Consequently the intended consent-boundary fix remains inert in production while execute_tool_calls_sequential() still passes the original args; apply the enrichment at that hook call as well and exercise the real dispatch path in the test.
AGENTS.md reference: AGENTS.md:L54-L57
Useful? React with 👍 / 👎.
| platform_config = self.config.platforms.get(adapter.platform) | ||
| if platform_config and adapter.platform not in self._failed_platforms: | ||
| if platform_config is None: | ||
| platform_config = getattr(adapter, "config", None) |
There was a problem hiding this comment.
When a runtime-fatal adapter belongs to a secondary multiplexed profile and the default profile does not configure that platform, this fallback puts the secondary profile's config into the global retry queue. _reconnect_pass() then recreates it outside _profile_runtime_scope, assigns the default _handle_message, and installs it in self.adapters rather than the secondary profile map, so subsequent traffic can run under the wrong profile/home; carry the originating profile through the retry queue and restore it through the profile-specific path.
AGENTS.md reference: AGENTS.md:L147-L154
Useful? React with 👍 / 👎.
| def _sidecar_ready_timeout() -> float: | ||
| """Readiness window in seconds; PHOTON_SIDECAR_READY_TIMEOUT overrides.""" | ||
| raw = os.environ.get("PHOTON_SIDECAR_READY_TIMEOUT", "") | ||
| try: |
There was a problem hiding this comment.
This introduces a user-facing environment variable for a non-secret behavioral timeout, bypassing the project's configuration and setup UX. Define the Photon readiness timeout in config.yaml and resolve it through the normal config path rather than requiring operators to manage PHOTON_SIDECAR_READY_TIMEOUT out of band.
AGENTS.md reference: AGENTS.md:L102-L107
Useful? React with 👍 / 👎.
| ready_timeout = _sidecar_ready_timeout() | ||
| deadline = time.time() + ready_timeout |
There was a problem hiding this comment.
The new 60-second default is still wrapped by GatewayRunner._connect_adapter_with_timeout(), whose default gateway.platform_connect_timeout is 30 seconds, so Photon startup is cancelled after 30 seconds and can never use the advertised readiness window. On the loaded hosts this change targets, any healthy startup taking 30–60 seconds will therefore continue to fail and re-enter the reconnect loop; coordinate these timeouts or derive the sidecar deadline from the existing outer connect budget.
AGENTS.md reference: AGENTS.md:L155-L164
Useful? React with 👍 / 👎.
| try: | ||
| value = float(raw) | ||
| except ValueError: | ||
| return _DEFAULT_SIDECAR_READY_TIMEOUT | ||
| if value <= 0: | ||
| return _DEFAULT_SIDECAR_READY_TIMEOUT |
There was a problem hiding this comment.
Values such as PHOTON_SIDECAR_READY_TIMEOUT=nan or inf are accepted because both parse as floats and fail the value <= 0 check. nan makes the deadline comparison false immediately, so a healthy sidecar is reported failed without a single health probe, while inf can make this wait unbounded when the gateway's outer timeout is disabled; require math.isfinite(value) before accepting the override.
Useful? React with 👍 / 👎.
Antigravity — PR #19 Review0. VISUAL VERIFICATION
0B. PRODUCTION BUILD & PAGE STABILITY
0C. MOBILE/TABLET UX REVIEW
0D. ENV VARS OVER MOCKS
1. User Experience & Flow [DEEP DIVE]
2. UI Quality & Polish [DEEP DIVE]
3. Wiring & Integration [DEEP DIVE]
4. Security [DEEP DIVE]
5. Accessibility
6. Performance Impact
7. Test Coverage Delta & Test Quality
8. Breaking Changes
9. Error Message Quality
10. Code Quality
11. Changelog & Versioning [NO ESCAPE]
12. Refactor Recommendations
13. Documentation [NO ESCAPE]
14. Lessons Learned Deposit [NO ESCAPE]
15. Documentation & Context Discovery [NO ESCAPE]
sequenceDiagram
participant Gateway as GatewayRunner
participant Watcher as Reconnect Watcher Task
participant Adapter as Photon Adapter
participant Sidecar as Node Sidecar Process
Gateway->>Watcher: _start_reconnect_watcher() (creates task)
Note over Watcher: Runs _platform_reconnect_watcher loop
Watcher->>Adapter: Trigger reconnect
Adapter->>Sidecar: Start sidecar process
Note over Adapter: Waits up to PHOTON_SIDECAR_READY_TIMEOUT for /healthz
alt Sidecar Ready
Adapter-->>Watcher: Success
Watcher-->>Gateway: Remove from _failed_platforms
else Sidecar Timeout / Crash
Adapter-->>Watcher: Raise fatal error
Watcher->>Watcher: Update next_retry (backoff)
end
alt Watcher Task Crashes
Watcher-->>Gateway: Raise exception (Done Callback)
Gateway->>Gateway: _on_reconnect_watcher_done()
Gateway->>Gateway: Schedule restart in 5s
Gateway->>Watcher: Recreate watcher task
end
Summary: The PR addresses critical gateway outages, but introduces a security validation bypass on mixed-case command directories and a configuration vulnerability allowing NaN/Infinity inputs to crash sidecar process checks. Address these issues before merging. VERDICT: CONCERN STAMP WITHHELD: CRITICAL: 1 issue(s) found. Fix issues and re-run |
|
I'll deploy multiple subagents to perform a brutal, comprehensive review of this PR. Let me analyze the diff systematically across all required dimensions. Subagent 1: Backend/Architecture ReviewFocus: Python asyncio correctness, gateway lifecycle, error handling, concurrency. Critical Finding B1 — Unsupervised restart creates unbounded task spawn loop: Critical Finding B2 — Race condition in Critical Finding B3 — Critical Finding B4 — Critical Finding B5 — Subagent 2: Security/RLS ReviewFocus: RLS, secrets, injection, error leakage, env var hygiene. Finding S1 — PHOTON_SIDECAR_READY_TIMEOUT env var has no upper bound: Finding S2 — Finding S3 — Finding S4 — Error log leaks internal state: Finding S5 — No CSRF/auth check on Finding S6 — No RLS assessment possible (not a DB-touching PR): This PR doesn't touch DB schemas or RLS policies, so the RLS attack patterns don't apply. Verified no new tables, no new SQL. Subagent 3: Testing/Quality ReviewFocus: Test gaps, brittle tests, mocks hiding behavior. Finding T1 — Test class _RacingDict(dict):
def get(self, key, default=None):
if key is gone and key in self:
del self[gone]
return None
return super().get(key, default)This test does NOT prove the fix is correct under concurrent mutation. It proves the test's mock returns Finding T2 — No negative test for Finding T3 — Finding T4 — Finding T5 — No test for the new Finding T6 — No test for Finding T7 — No test for Finding T8 — Subagent 4: Frontend/UI ReviewN/A — No frontend files modified. This PR is purely Python backend (gateway, agent, photon plugin). Subagent 5: Product Management / UX ReviewFinding P1 — Operator visibility: when watcher crashes, no metric emitted: Finding P2 — "already queued" refresh swallows the original backoff: Finding P3 — Finding P4 — Photon sidecar ready timeout has no metric: Subagent 6: Documentation ReviewFinding D1 — No CHANGELOG entry: Finding D2 — PHOTON_SIDECAR_READY_TIMEOUT not documented in README: Finding D3 — No migration notes: Finding D4 — Code comment in Finding D5 — No diagram for the reconnect lifecycle: Final Consolidated Review[Multi-Subagent Review] — PR #19 Review0. VISUAL VERIFICATIONN/A — no frontend files modified (backend-only PR: gateway, agent, photon plugin). 0B. PRODUCTION BUILD & PAGE STABILITYN/A — no frontend files modified. 0C. MOBILE/TABLET UX REVIEWN/A — no frontend files modified. 0D. ENV VARS OVER MOCKSNo mocks detected in production code. Tests use 1. User Experience & Flow [DEEP DIVE]
2. UI Quality & Polish [DEEP DIVE]N/A — no UI changes. However, observations on the platform-status UX surfaced by this PR:
3. Wiring & Integration [DEEP DIVE]
4. Security [DEEP DIVE]4A. Traditional Web Security
4B. AI/LLM-Specific Security 4C. Architectural & Compliance
4D. Penetration Testing Patterns
4E. Protected System-State (macOS/Unix) 5. AccessibilityN/A — no UI changes. 6. Performance Impact
7. Test Coverage Delta & Test QualityCoverage gaps:
Test quality issues:
8. Breaking Changes
9. Error Message Quality
10. Code Quality
11. Changelog & Versioning [NO ESCAPE]
12. Refactor Recommendations
13. Documentation [NO ESCAPE]Step 1 — BLUEPRTINT:
Step 2 — User-facing:
Step 2 — Internal:
Documentation Score: 20% user-facing | 40% technical. Both below 50% → HIGH severity. 14. Lessons Learned Deposit [NO ESCAPE]
15. Documentation & Context Discovery [NO ESCAPE]15A. User-Facing Docs: Score 0/8 (0%) — No README update, no CHANGELOG, no migration notes. HIGH severity. 15B. Context Diagram: MISSING (REQUIRED — PR touches 5+ files). The reconnect-watcher's lifecycle (start → connect → fatal → queue → watcher retry → success/pause/giving-up) is a non-trivial state machine that should be diagrammed. Fix: Add Mermaid 15C. Technical Docs: Score 2/8 (25%) — Some inline comments added in Documentation Score: 0% user-facing | 25% technical | Diagram: NO (required but missing). Summary: The fix addresses real resilience bugs but ships with 3 HIGH-severity concerns: (1) the watcher restart loop has no upper bound — a regressed watcher will spin forever every 5s; (2) Severity Counts: CRITICAL: 0 | HIGH: 8 | MEDIUM: 14 | LOW: 12 | SUGGESTIONS: 6 VERDICT: REJECT |
Principal Engineer & Systems Architect — PR #19 Review0. VISUAL VERIFICATION [MANDATORY FOR FRONTEND]N/A - no frontend changes 0B. PRODUCTION BUILD & PAGE STABILITY [MANDATORY FOR FRONTEND]N/A - no frontend changes 0C. MOBILE/TABLET UX REVIEW [MANDATORY FOR FRONTEND]N/A - no frontend changes 0D. ENV VARS OVER MOCKS [MANDATORY FOR ALL PRs]Mock: 1. User Experience & Flow [DEEP DIVE]
2. UI Quality & Polish [DEEP DIVE]
3. Wiring & Integration [DEEP DIVE]
4. Security [DEEP DIVE]
5. AccessibilityN/A - no UI changes. 6. Performance Impact
7. Test Coverage Delta & Test Quality
8. Breaking Changes
9. Error Message Quality
10. Code Quality
11. Changelog & Versioning [NO ESCAPE]
12. Refactor Recommendations
13. Documentation [NO ESCAPE]
14. Lessons Learned Deposit [NO ESCAPE]
15. Documentation & Context Discovery [NO ESCAPE]
Summary: While the core logic fixes the silent retry death, the implementation introduces a critical security vulnerability via un-sanitized working directory injection and leaves a broken indentation block in the gateway runner. The local evidence file must be stripped. VERDICT: REJECT STAMP WITHHELD: CRITICAL: 1 issue(s) found. Fix issues and re-run |
[
{
"id": "PROD-001",
"severity": "bug",
"confidence": 1.0,
"category": "wiring",
"file": "temp/evidence/test_evidence/current_session.json",
"line": 1,
"title": "Committed local scratch file with failed test evidence",
"description": "The `temp/evidence/test_evidence/current_session.json` file is local CI scratch output that was accidentally committed in this PR. It adds noise to the repository and indicates local test runs returned an exit code of -1.",
"evidence": "The diff explicitly introduces `temp/evidence/test_evidence/current_session.json`. This file is not part of the source code or required test fixtures.",
"suggestion": "Delete `temp/evidence/test_evidence/current_session.json` from the PR and add `temp/` to `.gitignore`.",
"is_new": true,
"origin": "NEW"
},
{
"id": "RESILIENCE-001",
"severity": "bug",
"confidence": 0.95,
"category": "resilience",
"file": "gateway/run.py",
"line": 7696,
"title": "Unbounded restart loop in reconnect watcher",
"description": "If the reconnect watcher repeatedly fails (e.g., because of a code regression or persistent bad state), `_on_reconnect_watcher_done` restarts it every 5 seconds indefinitely. There is no backoff, max-restart counter, or circuit breaker, which could lead to log flooding and CPU exhaustion.",
"evidence": "In `gateway/run.py`, `_on_reconnect_watcher_done` blindly calls `asyncio.get_event_loop().call_later(self._WATCHER_RESTART_DELAY, self._maybe_restart_reconnect_watcher)` as long as `self._running` is true, without tracking consecutive failures.",
"suggestion": "Implement an exponential backoff or a max-restart counter with an escalating log level for repeated crashes.",
"is_new": true,
"origin": "NEW"
},
{
"id": "BACKOFF-001",
"severity": "bug",
"confidence": 0.95,
"category": "logic",
"file": "gateway/run.py",
"line": 4000,
"title": "Refresh path resets retry clock without incrementing attempt counter",
"description": "In `_handle_adapter_fatal_error`, when a platform is already queued, the `else` branch resets `next_retry` to `time.monotonic()` so the watcher acts promptly. However, it does not increment the `attempts` counter. This allows a repeatedly failing platform to be retried continuously without hitting the `_BACKOFF_CAP`, effectively defeating the exponential backoff.",
"evidence": "At line 4000, the code sets `info[\"next_retry\"] = time.monotonic()` but never increments `info[\"attempts\"]`.",
"suggestion": "Increment `info[\"attempts\"]` when refreshing `next_retry`, or recalculate `next_retry` based on the current attempt count to preserve backoff semantics.",
"is_new": true,
"origin": "NEW"
},
{
"id": "PARSE-001",
"severity": "bug",
"confidence": 0.90,
"category": "validation",
"file": "plugins/platforms/photon/adapter.py",
"line": 109,
"title": "Sidecar timeout fails to reject NaN values",
"description": "`_sidecar_ready_timeout()` parses the `PHOTON_SIDECAR_READY_TIMEOUT` environment variable but fails to check for `NaN`. Because `float('nan') <= 0` evaluates to `False`, `NaN` passes the validation. When added to `time.time()`, it creates a `NaN` deadline, causing the `while time.time() < deadline:` loop to terminate instantly.",
"evidence": "Line 109 checks `if value <= 0:`, but `math.isnan` is not checked. In Python, `float('nan') <= 0` is `False`.",
"suggestion": "Use `math.isfinite(value)` or explicitly check `math.isnan(value)` to reject NaN inputs.",
"is_new": true,
"origin": "NEW"
}
]Verification SummaryMode: verification Bugs (must fix)
Nits (optional)
Pre-existing (awareness)
Verification Delta
VERDICT: CONCERN |
PR Review Summary -- Multi-AI ConsensusPR: #19 | Voters: 3/5 usable
Reviewed by: gemini-flash, minimax, glm, glm-v3-verifier CI Checks: No CI checks configured WARNING: Stamps withheld due to failing local tests. Fix tests and re-run Individual reviews posted as separate comments above. Generated at 2026-08-02 18:25 UTC |
review - reject NaN/Inf PHOTON_SIDECAR_READY_TIMEOUT and clamp to 600s ceiling - watcher respawn backs off exponentially (5s -> 300s cap) on a crash streak, resetting after a 60s stable run — no fixed-5s spin loop, while never giving up (a give-up cap would recreate the silent outage) - asyncio.get_running_loop() in the done-callback (get_event_loop is deprecated in callback context on 3.12+) - drop accidentally committed temp/evidence/test_evidence artifact Tests: 126 photon + 73 reconnect passing; 4 new RED-first tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Review triage @ d9b10b9: Accepted & fixed (this commit, RED-first):
Rebutted with evidence:
Tests at head: 126 photon + 73 reconnect/gateway passing. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9b10b9462
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "stays down until gateway restart", | ||
| adapter.platform.value, | ||
| ) | ||
| elif adapter.platform not in self._failed_platforms: |
There was a problem hiding this comment.
When Photon reports a runtime failure from _monitor_sidecar_health() or _supervise_sidecar(), this handler runs inside a task that PhotonAdapter.disconnect() cancels—directly for the health task and through _stop_sidecar() for the supervisor. The next await raises CancelledError, so execution never reaches this queue insertion after the adapter has already been removed from self.adapters, leaving Photon down without a reconnect attempt. Queue the retry before teardown or prevent disconnect() from cancelling its current notifier task, and test the real Photon callback/disconnect path rather than a fake adapter.
AGENTS.md reference: AGENTS.md:L84-L87
Useful? React with 👍 / 👎.
Antigravity (Principal Engineer) — PR #19 Review0. VISUAL VERIFICATION [MANDATORY FOR FRONTEND]N/A - no frontend changes 0B. PRODUCTION BUILD & PAGE STABILITY [MANDATORY FOR FRONTEND]N/A - no frontend changes 0C. MOBILE/TABLET UX REVIEW [MANDATORY FOR FRONTEND]N/A - no frontend changes 0D. ENV VARS OVER MOCKS [MANDATORY FOR ALL PRs]
1. User Experience & Flow [DEEP DIVE]
2. UI Quality & Polish [DEEP DIVE]
3. Wiring & Integration [DEEP DIVE]
4. Security [DEEP DIVE]
5. Accessibility
6. Performance Impact
7. Test Coverage Delta & Test Quality
8. Breaking Changes
9. Error Message Quality
10. Code Quality
11. Changelog & Versioning [NO ESCAPE]
12. Refactor Recommendations
13. Documentation [NO ESCAPE]
14. Lessons Learned Deposit [NO ESCAPE]
15. Documentation & Context Discovery [NO ESCAPE]
Summary: The PR resolves the silent retry death for Photon but introduces a critical architectural flaw (sequential blocking of the reconnect queue) and a severe Denial of Service vector via VERDICT: CONCERN STAMP WITHHELD: CRITICAL: 2 issue(s) found. Fix issues and re-run |
Principal Engineer — PR #19 Review0. VISUAL VERIFICATION [MANDATORY FOR FRONTEND]N/A - no frontend changes. (Backend/Agent only). 0B. PRODUCTION BUILD & PAGE STABILITY [MANDATORY FOR FRONTEND]N/A - no frontend changes. 0C. MOBILE/TABLET UX REVIEW [MANDATORY FOR FRONTEND]N/A - no frontend changes. 0D. ENV VARS OVER MOCKS [MANDATORY FOR ALL PRs]The unit tests heavily rely on mocks (
1. User Experience & Flow [DEEP DIVE]
2. UI Quality & Polish [DEEP DIVE]N/A - no frontend changes.
3. Wiring & Integration [DEEP DIVE]
4. Security [DEEP DIVE]
5. AccessibilityN/A - no frontend changes.
6. Performance Impact
7. Test Coverage Delta & Test Quality
8. Breaking Changes
9. Error Message Quality
10. Code Quality
11. Changelog & Versioning [NO ESCAPE]
12. Refactor Recommendations
13. Documentation [NO ESCAPE]
14. Lessons Learned Deposit [NO ESCAPE]
15. Documentation & Context Discovery [NO ESCAPE]
Summary: While the PR successfully addresses the immediate 3-day outage with robust race condition handling, it introduces a critical architectural flaw by spoofing tool arguments to bypass security plugins, posing a severe path traversal risk. The use of VERDICT: REJECT STAMP WITHHELD: CRITICAL: 2 issue(s) found. Fix issues and re-run |
[
{
"id": "SEC-001",
"severity": "bug",
"confidence": 0.95,
"category": "security",
"file": "plugins/platforms/photon/adapter.py",
"line": 108,
"title": "Unbounded sidecar ready timeout via float('inf') or float('nan')",
"description": "If an operator sets PHOTON_SIDECAR_READY_TIMEOUT to 'inf' or 'nan', `float(raw)` parses successfully. 'inf' causes the readiness check loop `while time.time() < deadline:` to run forever if the sidecar fails to boot, while 'nan' breaks the loop condition immediately, causing it to fail instantly without waiting. The check `if value <= 0:` does not guard against these non-finite values.",
"evidence": "Read adapter.py line 105-111. float('inf') <= 0 is False. This could cause the gateway to hang indefinitely during startup under load if misconfigured.",
"suggestion": "Import `math` and explicitly check for non-finite values: `if not math.isfinite(value) or value <= 0: return _DEFAULT_SIDECAR_READY_TIMEOUT`",
"is_new": true,
"origin": "NEW"
},
{
"id": "DOC-001",
"severity": "nit",
"confidence": 0.90,
"category": "documentation",
"file": "CHANGELOG.md",
"line": 1,
"title": "Missing CHANGELOG entry for infrastructure timeout and contract changes",
"description": "The PR changes the default sidecar ready timeout from 15s to 60s (high infrastructure impact, could cause OOM kills in strict CI/CD environments) and introduces new behavior in tool_executor.py. No CHANGELOG entry or version bump was provided.",
"evidence": "Diff lacks changes to CHANGELOG.md or documentation files despite altering default timings and tool execution argument contracts.",
"suggestion": "Add a CHANGELOG entry noting the increased default timeout and the terminal cwd argument enrichment behavior.",
"is_new": true,
"origin": "NEW"
}
]Verification SummaryMode: verification Bugs (must fix)
Nits (optional)
Pre-existing (awareness)
Verification Delta (verification mode only)
Original Review VERDICT: CONCERN |
PR Review Summary -- Multi-AI ConsensusPR: #19 | Voters: 2/5 usable
Reviewed by: gemini-pro, glm, glm-v3-verifier CI Checks: No CI checks configured WARNING: Stamps withheld due to failing local tests. Fix tests and re-run ⚠ path-not-in-diff — findings below cite files outside this PR's changed set (context citations are legitimate; verify before acting):
Individual reviews posted as separate comments above. Generated at 2026-08-02 19:07 UTC |
…dir sensitivity + test portability Review fixes (PR #19): - reject NaN/Inf PHOTON_SIDECAR_READY_TIMEOUT and clamp to 600s ceiling - watcher respawn backs off exponentially (5s -> 300s cap) on a crash streak, resetting after a 60s stable run — no fixed-5s spin loop, while never giving up (a give-up cap would recreate the silent outage) - asyncio.get_running_loop() in the done-callback macOS correctness (root cause of the red local suite): - tools/file_tools.py: the /private/var/ sensitive-prefix blocked the process's own tempdir (/private/var/folders/... on macOS), making tempfile.gettempdir() unwritable by the file tools; prefix check now exempts the process tempdir (exact-path + Hermes-config checks still apply) - tests: keychain-path subprocess mocks get stdout='' (Darwin-only branch fed a MagicMock to json.loads); /tmp assertions use os.path.realpath (macOS /tmp -> /private/tmp symlink) Tests: 232 passed across file-tools/edit-approval/adapter suites; 126 photon + 73 reconnect passing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d63b30c748
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| in_process_tmp = resolved.startswith(tmp_root) or normalized.startswith(tmp_root) | ||
| if not in_process_tmp: |
There was a problem hiding this comment.
When a path under the process tempdir traverses a symlink into a protected location (for example, /tmp/escape -> /etc followed by a write to /tmp/escape/passwd), normalized.startswith(tmp_root) makes this exemption true even though resolved is /etc/passwd. write_file_tool() subsequently dispatches that resolved path, bypassing the sensitive-system-path guard; base the exemption on the resolved target remaining under tmp_root and cover the symlink case with a real filesystem test.
AGENTS.md reference: AGENTS.md:L84-L87
Useful? React with 👍 / 👎.
Peer Reviewer — PR #19 Review0. VISUAL VERIFICATION [MANDATORY FOR FRONTEND]N/A - no frontend changes. This PR contains only backend Python code ( 0B. PRODUCTION BUILD & PAGE STABILITY [MANDATORY FOR FRONTEND]N/A - no frontend changes. There are no web pages, bundlers, or SSR hydration contexts modified in this PR. 0C. MOBILE/TABLET UX REVIEW [MANDATORY FOR FRONTEND]N/A - no frontend changes. There are no mobile layout changes or touch target additions in this PR. 0D. ENV VARS OVER MOCKS [MANDATORY FOR ALL PRs]Mocks were detected in the following test suites:
1. User Experience & Flow [DEEP DIVE]
2. UI Quality & Polish [DEEP DIVE]
3. Wiring & Integration [DEEP DIVE]
4. Security [DEEP DIVE]4A. Traditional Web Security (CWE-Referenced)
4B. AI/LLM-Specific Security (OWASP LLM Top 10)
4C. Architectural & Compliance
4D. Penetration Testing Patterns (Vibe Security)
4E. Protected System-State (macOS/Unix)
5. Accessibility
6. Performance Impact
7. Test Coverage Delta & Test Quality
8. Breaking Changes
9. Error Message Quality
10. Code Quality
11. Changelog & Versioning [NO ESCAPE]
12. Refactor Recommendations
13. Documentation [NO ESCAPE]
14. Lessons Learned Deposit [NO ESCAPE]
15. Documentation & Context Discovery [NO ESCAPE]
sequenceDiagram
participant Gateway
participant Watcher
participant Sidecar
Gateway->>Watcher: _start_reconnect_watcher()
activate Watcher
Watcher->>Watcher: _reconnect_pass()
Watcher->>Sidecar: Start subprocess & poll /healthz
alt Startup Success
Sidecar-->>Watcher: HTTP 200 OK
Watcher-->>Gateway: Connection restored
else Timeout / Crash
Sidecar-->>Watcher: Terminate / Timeout
deactivate Watcher
Note over Watcher: Watcher crashes or exits
Watcher->>Gateway: _on_reconnect_watcher_done(task)
Gateway->>Gateway: Exponential backoff delay
Gateway->>Watcher: Respawn watcher task
end
Summary: The PR successfully addresses critical recovery issues with the background connection watcher and adds configuration parameters to photon startup timeouts. However, it introduces a compilation-breaking syntax indentation error in VERDICT: CONCERN STAMP WITHHELD: CRITICAL: 1 issue(s) found. Fix issues and re-run |
|
Excellent. Test coverage agent is complete with comprehensive analysis revealing 4 HIGH-priority test gaps: no coverage for crash-streak reset, exception handling, actual watcher loop, and crash logging. Now I have all agent reports. Let me synthesize the complete multi-agent brutal review: Brutally Thorough Multi-Agent Review — PR #19Platform Resilience: Photon Reconnect Supervision + Configurable Sidecar Timeout0. VISUAL VERIFICATIONN/A — no frontend changes. This is a backend reliability fix to the gateway reconnection watcher and photon adapter configuration. 0B. PRODUCTION BUILD & PAGE STABILITYN/A — no frontend changes. No build system affected. 0C. MOBILE/TABLET UX REVIEWN/A — no frontend changes. 0D. ENV VARS OVER MOCKSMocks reviewed: ACCEPTABLE. All mocks are for unit tests (fake adapters, monkeypatched async sleeps, subprocess stubs). No internal services are being mocked when real env-var-backed integrations are available. The only test doubles (AsyncMock, _FakeAdapter, monkeypatch) are appropriately scoped to test the supervision logic itself, not replacing available services. ✓ 1. User Experience & FlowFinding 1.1 — Silent status desynchronization: "retrying" state can lie about actual connection
Finding 1.2 — Platform can be reconnected against operator's will
Finding 1.3 — Error messages from sidecar failures can expose system internals
2. UI Quality & PolishFinding 2.1 — Non-standard indentation reduces readability and diffs poorly
3. Wiring & IntegrationFinding 3.1 — Terminal CWD enrichment is unverified end-to-end; downstream consumer not in this repo
Finding 3.2 — No validation that enriched terminal CWD is safe or sensible
Finding 3.3 — Generic exception handling in terminal CWD enrichment hides anomalies
4. SECURITY4A. Traditional Web SecurityFinding 4A.1 — Terminal CWD spoofing/path traversal into security decision
Finding 4A.2 — Env var DoS surface:
Finding 4A.3 — Error message information disclosure
4B. AI/LLM-Specific SecurityNo AI/LLM-specific surfaces in this PR (no prompt injection, no model API calls, no RAG). 4C. Architectural & ComplianceFinding 4C.1 — Row-Level Security (RLS) not in scope — this PR doesn't touch database schema or Supabase policies. No RLS issues introduced. Finding 4C.2 — Adapter config fallback lacks validation
Finding 4C.3 — Operator pausing can be bypassed by concurrent fatal errors
4D. Penetration Testing PatternsFinding 4D.1 — No backend rate limiting on reconnect attempts
Finding 4D.2 — Webhook-like fatal-error handling without idempotency guards
4E. Protected System-State (macOS/Unix)Finding 4E.1 — Tempdir access guard exemption is correctly scoped
5. AccessibilityN/A — no UI changes. 6. Performance ImpactFinding 6.1 — No concurrency limit on platform reconnect queue
Finding 6.2 — 60s sidecar ready timeout could block the watcher
7. Test Coverage Delta & Test QualityCRITICAL GAPS (HIGH priority — merge-blocking): Finding 7.1 — Crash-streak reset after stable run is untested
Finding 7.2 — Watcher loop exception handler is untested
Finding 7.3 — Actual watcher loop (
Finding 7.4 — Watcher crash is not audited to logs
MEDIUM GAPS (hardening, not merge-blocking): Finding 7.5 — Concurrent mutation during await is only partially tested
Finding 7.6 — Paused platform is not tested
TEST QUALITY ISSUES: Finding 7.7 — Hardcoded constants in backoff test lack resilience to tuning
Finding 7.8 — Default timeout test hardcodes instead of referencing constant
Finding 7.9 — Supervised-restart test doesn't assert final quiescent state
8. Breaking ChangesNo breaking changes identified. The reconnect watcher is an internal mechanism (no public API change). The 9. Error Message QualityFinding 9.1 — Sidecar ready-timeout error message includes raw exception text
Finding 9.2 — Watcher crash messages don't distinguish "cap reached" state
10. Code QualityFinding 10.1 — Uninitialized bare
Finding 10.2 —
Finding 10.3 — Magic number
Finding 10.4 — Bare
Finding 10.5 — Initial 10s sleep in watcher loop lacks explanation
11. Changelog & VersioningFinding 11.1 — No CHANGELOG update present
Finding 11.2 — No version bump
12. Refactor RecommendationsPriority NOW (blocks merge):
Priority SOON (next sprint):
Priority LATER (backlog):
13. DocumentationFinding 13.1 — No BLUEPRINT or design documentation
Finding 13.2 —
Finding 13.3 — Watcher supervision not documented in README
14. Lessons Learned DepositFinding 14.1 — Lessons-learned file present and complete
15. Documentation & Context Discovery15A. User-Facing DocumentationStatus: INCOMPLETE (0/2 — environment variable documented, usage guide missing).
15B. Context DiagramScope: 9 files changed (> 3-file threshold).
15C. Technical DocumentationStatus: INCOMPLETE (2/4 — timeout parsing documented, watcher supervision not).
Overall Documentation Score:
VERDICT & SEVERITY SUMMARY
Status: CONCERN — MULTIPLE CRITICAL ISSUES BLOCK MERGE MULTI-AGENT SYNTHESISAll 6 specialized agents (backend architect, security analyst, test writer, DevOps engineer, code quality reviewer, integration tester) identified consistent blockers across independent analyses:
Merge Recommendation: REJECT until critical issues are resolved:
|
Principal Engineer — PR #19 ReviewMulti-Subagent Deployment Notice: UI, UX, Backend, Frontend, and Product Management subagents deployed in parallel. Consolidated findings below. 0. VISUAL VERIFICATIONN/A — no frontend changes (gateway/backend, plugins/photon adapter, agent/tool_executor, tools/file_tools — all backend code). What I inspected to verify the N/A is honest: the diff touches 0B. PRODUCTION BUILD & PAGE STABILITYN/A — no frontend changes. Backend-only PR; production build for What I inspected: no 0C. MOBILE/TABLET UX REVIEWN/A — no frontend changes. 0D. ENV VARS OVER MOCKSMock scan across the diff:
No mocks that hide real integration bugs. All mocks are at the immediate dependency boundary and accelerate deterministic testing of crash/race conditions that are harder to reproduce against real infra. Hardening: The 1. User Experience & Flow [DEEP DIVE]
2. UI Quality & Polish [DEEP DIVE]N/A — no UI diff. Hardening suggestion: when photon status surfaces in any future UI, make the "retrying" badge clickable to a debug drawer showing What I inspected: all changes are in 3. Wiring & Integration [DEEP DIVE]
4. Security [DEEP DIVE]4A. Traditional Web Security
4B. AI/LLM-Specific Security
4C. Architectural & Compliance
4D. Penetration Testing Patterns
4E. Protected System-State (macOS/Unix)
5. AccessibilityN/A — no UI changes. What I inspected: no ARIA, no focus management, no keyboard nav code in the diff. Hardening for future UI: when the photon "retrying" status is surfaced in any UI, ensure it has 6. Performance Impact
7. Test Coverage Delta & Test QualityCoverage gaps:
Test quality:
Verdict: Coverage is reasonable but not bulletproof. The 3 missing tests for 8. Breaking Changes
9. Error Message Quality
10. Code Quality
11. Changelog & Versioning [NO ESCAPE]
12. Refactor Recommendations
TODO/FIXME without issue references: None added in this diff. The PR references 13. Documentation [NO ESCAPE]Step 1 — BLUEPRTINT:
Step 2 — User-facing (operators):
Internal (developers):
Overall: 5/16 (31%) — HIGH severity. Add a 14. Lessons Learned Deposit [NO ESCAPE]
Suggested additional deposits for this PR:
15. Documentation & Context Discovery [NO ESCAPE]15A User-Facing Docs: Score 1/8 (12.5%) — operator docs missing for new env var. HIGH severity. 15B Context Diagram: The PR touches 9 files. HIGH severity — diagram required. Suggested Mermaid for the PR description: sequenceDiagram
participant Adapter
participant Gateway as GatewayRunner
participant Watcher as _platform_reconnect_watcher
participant Sidecar as Photon Sidecar
Adapter->>Gateway: fatal_error_retryable=True
Gateway->>Gateway: _handle_adapter_fatal_error()
alt platform not in _failed_platforms
Gateway->>Gateway: enqueue with config fallback
else already queued
Gateway->>Gateway: refresh next_retry (unless paused)
end
Watcher->>Gateway: _reconnect_pass() every 10s
Gateway->>Adapter: try reconnect
Adapter->>Sidecar: spawn with PHOTON_SIDECAR_READY_TIMEOUT
Sidecar-->>Adapter: /healthz ready
Adapter-->>Gateway: connected → remove from _failed_platforms
Note over Watcher: supervised by _on_reconnect_watcher_done
Watcher--xWatcher: crash
Watcher->>Watcher: respawn after min(5*2^n, 300)s
15C Technical Docs: Score 4/8 (50%). Docstrings present, but no ADR for the "never give up" philosophy, no integration doc for the Documentation Score: 12% (user-facing) | 50% (technical) | Diagram: NO Summary: This is a solid defensive fix for a real production outage — the watcher supervision and config fallback are correctly designed and well-tested. The blocker findings are: (1) the OAuth Severity Counts: CRITICAL: 0 | HIGH: 6 | MEDIUM: 9 | LOW: 7 | SUGGESTIONS: 5 VERDICT: CONCERN |
Principal Engineer — PR #19 Review0. VISUAL VERIFICATION
0B. PRODUCTION BUILD & PAGE STABILITY
0C. MOBILE/TABLET UX REVIEW
0D. ENV VARS OVER MOCKSVerdict: ACCEPTABLE.
1. User Experience & Flow
2. UI Quality & Polish
3. Wiring & Integration
4. Security
5. Accessibility
6. Performance Impact
7. Test Coverage Delta & Test Quality
8. Breaking Changes
9. Error Message Quality
10. Code Quality
11. Changelog & Versioning
12. Refactor Recommendations
13. Documentation
14. Lessons Learned Deposit
15. Documentation & Context Discovery
Summary: The architectural intent (supervised reconnects, env var timeouts) is strong, but the implementation introduces a critical path traversal bypass in VERDICT: REJECT STAMP WITHHELD: CRITICAL: 1 issue(s) found. Fix issues and re-run |
[
{
"id": "SEC-001",
"severity": "bug",
"confidence": 0.85,
"category": "security",
"file": "tools/file_tools.py",
"line": 648,
"title": "Path Traversal Bypass via Symlink Exploitation",
"description": "The change to `_check_sensitive_path` adds an exemption to sensitive path blocking if the target is inside the process tempdir (`tempfile.gettempdir()`). Because it uses `os.path.realpath()`, symlinks are resolved. However, an attacker who can create a symlink inside `/tmp` pointing to `/etc/` or `/var/db/` bypasses ALL sensitive prefix checks. If a malicious agent writes a symlink to `/tmp/evil -> /etc/`, and then targets `/tmp/evil/passwd`, `resolved.startswith(tmp_root)` evaluates to True, dropping the guard entirely.",
"evidence": "Traced caller chain and confirmed the issue is real. Security finding (+0.20). Read test file and confirmed no test covers this path (+0.05).",
"suggestion": "Ensure the consent-boundary plugin strictly canonicalizes and sanitizes the `cwd` argument before using it to resolve scripts, and rejects paths containing `..` or symlinks pointing outside the tempdir root.",
"is_new": true,
"origin": "NEW"
},
{
"id": "SEC-002",
"severity": "bug",
"confidence": 0.90,
"category": "security",
"file": "agent/tool_executor.py",
"line": 77,
"title": "Silent Swallowing of Hook Enrichment Failures",
"description": "If `tools.terminal_tool.get_active_env` fails for any reason (e.g., circular import, missing session), the `except Exception: pass` block silently drops the error. A security hook (consent-boundary) will fail to evaluate the path correctly, blocking legitimate agent actions without logging why.",
"evidence": "Traced caller chain and confirmed the issue is real. Security finding (+0.20). Read test file and confirmed no test covers this path (+0.05). Baseline 0.70 + 0.10 (traced) + 0.05 (no test) + 0.20 (security) = 1.05 (capped to 0.90).",
"suggestion": "Add `logger.debug(\"Failed to enrich hook args with terminal cwd: %s\", e, exc_info=False)` inside the except block to aid debugging when hooks behave unexpectedly.",
"is_new": true,
"origin": "NEW"
}
]Verification SummaryMode: verification Bugs (must fix)
Nits (optional)
Pre-existing (awareness)
Verification Delta (verification mode only)
VERDICT: CONCERN |
PR Review Summary -- Multi-AI ConsensusPR: #19 | Voters: 4/5 usable
Reviewed by: gemini-flash, claude-haiku, minimax, glm, glm-v3-verifier CI Checks: No CI checks configured WARNING: Stamps withheld due to failing local tests. Fix tests and re-run ⚠ path-not-in-diff — findings below cite files outside this PR's changed set (context citations are legitimate; verify before acting):
Individual reviews posted as separate comments above. Generated at 2026-08-02 19:44 UTC |
…ore exempting Council finding (PR #19, glm): a path lexically under the process tempdir but resolving elsewhere via symlink inherited the exemption. Exemption now keys on realpath of the task-resolved path only — symlink-escape targets fall through to the sensitive-prefix check. RED-first test covers the escape; relative task-cwd writes unaffected (233 tests green). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Council triage @ 5fa5dcf: Accepted & fixed: symlink-escape on the tempdir exemption (glm CRITICAL) — exemption now keys on Rebutted with evidence:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5fa5dcf0d6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # prefix would otherwise block, making tempfile.gettempdir() unwritable. | ||
| # Only the prefix check is exempted; exact-path and Hermes-config checks | ||
| # below still apply to tempdir paths. | ||
| tmp_root = os.path.realpath(tempfile.gettempdir()).rstrip("/") + "/" |
There was a problem hiding this comment.
When Hermes starts with TMPDIR=/—a valid configuration for a root-run service or container—this expression produces tmp_root == "/", so every absolute target is classified as temporary and the prefix guard is skipped. _check_sensitive_path('/etc/passwd') then returns None, allowing write_file or patch to modify protected system files when the process has permission; disable the exemption when the resolved tempdir is the filesystem root.
AGENTS.md reference: AGENTS.md:L84-L87
Useful? React with 👍 / 👎.
Reviewer — PR #19 Review0. VISUAL VERIFICATION [MANDATORY FOR FRONTEND]N/A - no frontend changes (all files are Python backend/gateway/tests). 0B. PRODUCTION BUILD & PAGE STABILITY [MANDATORY FOR FRONTEND]N/A - no frontend changes. 0C. MOBILE/TABLET UX REVIEW [MANDATORY FOR FRONTEND]N/A - no frontend changes. 0D. ENV VARS OVER MOCKS [MANDATORY FOR ALL PRs]
1. User Experience & Flow [DEEP DIVE]
2. UI Quality & Polish [DEEP DIVE]N/A — no UI surface changed. Observation: log message formatting ( 3. Wiring & Integration [DEEP DIVE]
4. Security [DEEP DIVE]
5. AccessibilityN/A — backend-only change, no UI. Nothing to harden here. 6. Performance Impact
7. Test Coverage Delta & Test Quality
8. Breaking Changes
9. Error Message Quality
10. Code Quality
11. Changelog & Versioning [NO ESCAPE]
12. Refactor Recommendations
13. Documentation [NO ESCAPE]
14. Lessons Learned Deposit [NO ESCAPE]
15. Documentation & Context Discovery [NO ESCAPE]
Summary: The core fix set (re-queue refresh, supervised watcher restart, configurable sidecar timeout) is well-targeted at the documented outage and backed by meaningful, race-condition-aware tests. However, the PR bundles three unrelated fixes (photon resilience, terminal-cwd hook threading, tempdir symlink security fix) into one changeset, leaves a residual silent-failure branch when both config sources are absent, has an unvalidated adapter-config trust fallback with security implications, lacks a context diagram despite touching 9 files, and ships a new env var with zero external documentation. Top priority: close the VERDICT: CONCERN |
Principal Reviewer — PR #19 Review0. VISUAL VERIFICATIONN/A - no frontend changes. Diff is backend Python (gateway runner, photon adapter, file_tools) + tests. No 0B. PRODUCTION BUILD & PAGE STABILITYN/A - no frontend changes. 0C. MOBILE/TABLET UX REVIEWN/A - no frontend changes. 0D. ENV VARS OVER MOCKSScanned diff for mock patterns:
1. User Experience & Flow [DEEP DIVE]
2. UI Quality & Polish [DEEP DIVE]N/A in the visual hierarchy sense — this is backend. But the "UI" here is the operator-facing surface: log lines, status fields, env vars.
3. Wiring & Integration [DEEP DIVE]
4. Security [DEEP DIVE]4A. Traditional Web Security
4B. AI/LLM-Specific Security
4C. Architectural & Compliance
4D. Penetration Testing Patterns
4E. Protected System-State
5. AccessibilityN/A — no UI changes. Hardening suggestion: when logging fatal adapter errors with 6. Performance Impact
7. Test Coverage Delta & Test QualityCoverage gaps:
Test quality issues:
8. Breaking Changes
9. Error Message Quality
10. Code Quality
11. Changelog & Versioning
12. Refactor Recommendations
13. Documentation [NO ESCAPE]Step 1 — BLUEPRINT:
Step 2 — User-facing vs Internal: User-facing (operator-visible: reconnect behavior, env vars, log lines):
User-facing Documentation Score: 2/8 = 25% → HIGH severity. Internal (gateway architecture, reconnect policy, watcher supervision):
Internal Documentation Score: 5/8 = 62% → MEDIUM severity (acceptable but below 80%). 14. Lessons Learned Deposit [NO ESCAPE]
15. Documentation & Context Discovery [NO ESCAPE]15A. User-Facing Docs:
15B. Context Diagram: PR modifies 9 files (over the 3-file threshold). MISSING. No Mermaid diagram, no ASCII diagram, no ADR. The reconnect watcher + adapter fatal flow is non-trivial — a 15C. Technical Docs:
Documentation Score: 10% (user-facing) | 50% (technical) | Diagram: NO Summary: The PR fixes three real, well-documented defects with strong TDD discipline and excellent inline rationale. The core engineering is solid — supervised restart, race-tolerant iteration, env-configurable timeout are all correct. However, the PR ships without a CHANGELOG entry, README update, or context diagram (the 9-file threshold was breached). The tempdir exemption in Severity Counts: CRITICAL: 1 | HIGH: 9 | MEDIUM: 6 | LOW: 2 | SUGGESTIONS: 8 VERDICT: CONCERN STAMP WITHHELD: CRITICAL: 1 issue(s) found. Fix issues and re-run |
|
Now I will conduct the comprehensive 15-section review based on the subagent findings and my own analysis. Claude Code PR #19 Review — Comprehensive Brutal Assessment0. VISUAL VERIFICATIONN/A - no frontend changes (gateway/plugins/tools backend only) 0B. PRODUCTION BUILD & PAGE STABILITYN/A - no frontend changes 0C. MOBILE/TABLET UX REVIEWN/A - no frontend changes 0D. ENV VARS OVER MOCKSMocks Found: Analysis:
1. User Experience & Flow (DEEP DIVE)FINDING 1.1: Silent Failure When Platform Config Is Completely Absent (HIGH severity) # Line 3983-3996: Config fallback logic
platform_config = self.config.platforms.get(adapter.platform)
if platform_config is None:
platform_config = getattr(adapter, "config", None)
if platform_config is None:
logger.error("...cannot queue for reconnection...") # Logs, but doesn't queue
elif adapter.platform not in self._failed_platforms: # This block is SKIPPED when config is None
self._failed_platforms[adapter.platform] = {...}The Bug: If Suggested Fix: if platform_config is None:
logger.error("%s fatal error is retryable but no platform config available...", adapter.platform.value)
# STILL queue it with an empty/default config so the watcher knows to attempt reconnection
# else use fallback minimal config
platform_config = {"enabled": True} # minimal default config
if adapter.platform not in self._failed_platforms:
self._failed_platforms[adapter.platform] = {
"config": platform_config,
"attempts": 0,
"next_retry": time.monotonic(),
}FINDING 1.2: Next-Retry Refresh Bypasses Exponential Backoff on Repeated Failures (HIGH severity) # Lines 7535-7536
if not info.get("paused"):
info["next_retry"] = time.monotonic() # ALWAYS reset to "now"This is correct for handling transient network blips (immediate retry on new event), but on network conditions where EVERY reconnect attempt fails, the backoff is negated. Consecutive failures should compound the backoff, not reset it. Scenario:
Suggested Fix: Only refresh else:
# Already queued. Refresh retry clock only if this is a NEW failure
# (not part of the same outage), or if backoff has plateaued.
# Otherwise respect exponential backoff to avoid retry hammering.
info = self._failed_platforms[adapter.platform]
# Option A: Only refresh if the last successful connection was recent
last_success_age = time.monotonic() - info.get("last_success", 0)
if last_success_age > 3600: # >1 hour since last success, treat as new outage
info["next_retry"] = time.monotonic()
# Option B: Or, only refresh if we're at the backoff cap already
# (we've given up on exponential backoff and are just retrying at max interval)
if info["next_retry"] >= time.monotonic() + 300 - 10: # nearly at cap
info["next_retry"] = time.monotonic()FINDING 1.3: Watcher Attributes Not Initialized → Subtle State Loss (MEDIUM severity) Suggested Fix: Initialize in def __init__(self, config: Optional[GatewayConfig] = None):
# ... existing __init__ code ...
self._watcher_crash_streak: int = 0 # Initialize here
self._watcher_started_at: float = 0.0Then remove 2. UI Quality & PolishN/A - no frontend files in this PR (backend only) 3. Wiring & Integration (DEEP DIVE)FINDING 3.1: Timeout Parsing Inefficiency (LOW severity, but indicative of defensive pattern) raw = os.environ.get("PHOTON_SIDECAR_READY_TIMEOUT", "")
try:
value = float(raw)
except ValueError:
return _DEFAULT_SIDECAR_READY_TIMEOUTIssue: If Suggested Fix: raw = os.environ.get("PHOTON_SIDECAR_READY_TIMEOUT", "")
if not raw: # Check for empty string first
return _DEFAULT_SIDECAR_READY_TIMEOUT
try:
value = float(raw)
except ValueError:
return _DEFAULT_SIDECAR_READY_TIMEOUTFINDING 3.2: _reconnect_pass() Extracts Loop But Doesn't Verify Reconnection (MEDIUM severity — test coverage issue) Suggested Fix: The test should mock the reconnection path and verify it's called: @pytest.mark.asyncio
async def test_reconnect_pass_attempts_reconnection(monkeypatch):
runner = _make_runner()
runner._running = True
platform = Platform.TELEGRAM
cfg = PlatformConfig(enabled=True, token="t")
runner._failed_platforms[platform] = {
"config": cfg,
"attempts": 0,
"next_retry": time.monotonic(),
}
# Mock the reconnection attempt
create_calls = []
async def mock_create(*args, **kwargs):
create_calls.append((args, kwargs))
# Return mock adapter
return AsyncMock()
monkeypatch.setattr(runner, "_create_adapter", mock_create)
await runner._reconnect_pass()
# Verify reconnection was attempted for the platform
assert len(create_calls) > 0, "reconnect_pass must call _create_adapter"4. Security (DEEP DIVE)FINDING 4.1: Unbounded Queue Growth in _failed_platforms (MEDIUM severity) Attack Scenario:
Exploitability: PARTIAL — Requires control over platform adapter instantiation, but the fix is simple. Suggested Fix: MAX_FAILED_PLATFORMS_QUEUE_SIZE = 256 # Cap the queue
if adapter.platform not in self._failed_platforms:
if len(self._failed_platforms) >= MAX_FAILED_PLATFORMS_QUEUE_SIZE:
# Remove oldest entry (FIFO eviction)
oldest_platform = next(iter(self._failed_platforms))
del self._failed_platforms[oldest_platform]
logger.warning("Failed platforms queue full; evicted %s", oldest_platform.value)
self._failed_platforms[adapter.platform] = {...}FINDING 4.2: Infinite Watcher Restart Loop Without Max Crash Count (MEDIUM severity) Attack Scenario:
Exploitability: YES — A single malformed entry in Suggested Fix: def _on_reconnect_watcher_done(self, task: "asyncio.Task") -> None:
if task.cancelled() or not self._running:
return
# ... existing streak logic ...
MAX_CRASH_STREAK = 10 # After 10 consecutive crashes, give up
if self._watcher_crash_streak >= MAX_CRASH_STREAK:
logger.critical(
"Platform reconnect watcher crashed %d times; giving up. "
"Check _failed_platforms state; manual restart required.",
self._watcher_crash_streak
)
self._exit_reason = "Reconnect watcher crash loop"
asyncio.create_task(self.stop())
return
# ... rest of restart logic ...FINDING 4.3: Missing Timeout Minimum Validation (MEDIUM severity) Attack Scenario:
Suggested Fix: def _sidecar_ready_timeout() -> float:
raw = os.environ.get("PHOTON_SIDECAR_READY_TIMEOUT", "")
if not raw:
return _DEFAULT_SIDECAR_READY_TIMEOUT
try:
value = float(raw)
except ValueError:
return _DEFAULT_SIDECAR_READY_TIMEOUT
if not math.isfinite(value) or value < 1.0: # Min 1 second
return _DEFAULT_SIDECAR_READY_TIMEOUT
return min(value, _MAX_SIDECAR_READY_TIMEOUT)5. AccessibilityN/A - no frontend files 6. Performance ImpactFINDING 6.1: Watcher Restart Backoff Might Prevent Quick Recovery (LOW severity) Analysis: After 5 consecutive crashes, the backoff reaches Trade-off: The current backoff prevents rapid restart loops from consuming CPU, but delays recovery from one-shot bugs. This is a GOOD trade-off (fail-safe is preferable to fail-fast when detecting bugs). Verdict: ACCEPTABLE — the design prioritizes stability over recovery speed. Add monitoring (suggested in Section 13). 7. Test Coverage Delta & Test Quality (DEEP DIVE)CRITICAL TEST FINDINGS: FINDING 7.1: Watcher Restart Test Uses Deterministic Mock but Is Flaky Under Load (HIGH severity) monkeypatch.setattr(type(runner), "_WATCHER_RESTART_DELAY", 0.0, raising=False)
runner._start_reconnect_watcher()
await asyncio.sleep(0.05)
assert len(spawns) >= 2, "watcher must restart..."Issue: The test monkeypatches the restart delay to 0, then sleeps for 50ms expecting the restart to happen. On a slow CI runner or under system load, the Suggested Fix: # Use an event to signal when the restart happens, not timing
restart_event = asyncio.Event()
async def _crashing_watcher() -> None:
spawns.append(1)
if len(spawns) == 1:
raise RuntimeError("watcher crashed")
restart_event.set() # Signal that restart #2 is running
while runner._running:
await asyncio.sleep(0.01)
runner._start_reconnect_watcher()
# Wait for the second incarnation to start, with a timeout
await asyncio.wait_for(restart_event.wait(), timeout=1.0)
assert len(spawns) >= 2FINDING 7.2: Reconnect Pass Tolerance Test Doesn't Verify Reconnection (HIGH severity) async def test_reconnect_pass_tolerates_concurrent_removal() -> None:
# ... setup with _RacingDict that removes "gone" platform ...
await runner._reconnect_pass()
assert stays in runner._failed_platformsIssue: The test verifies that Suggested Fix: @pytest.mark.asyncio
async def test_reconnect_pass_attempts_then_tolerates_removal() -> None:
runner = _make_runner()
runner._running = True
gone = Platform.TELEGRAM
stays = Platform.SLACK
cfg = PlatformConfig(enabled=True, token="t")
runner._failed_platforms[gone] = {
"config": cfg,
"attempts": 0,
"next_retry": time.monotonic(),
}
runner._failed_platforms[stays] = {
"config": cfg,
"attempts": 0,
"next_retry": time.monotonic() + 3600,
}
# Mock _create_adapter to track reconnect attempts
create_calls = []
async def mock_create(*args, **kwargs):
create_calls.append(args)
return AsyncMock(platform=args[0], disconnect=AsyncMock())
monkeypatch.setattr(runner, "_create_adapter", mock_create)
# Simulate concurrent removal via _RacingDict
class _RacingDict(dict):
def get(self, key, default=None):
if key is gone and key in self:
del self[gone] # Remove while reconnect is being awaited
return None
return super().get(key, default)
runner._failed_platforms = _RacingDict(runner._failed_platforms)
# Must not raise KeyError, and must have attempted reconnect for "gone"
await runner._reconnect_pass()
# Verify reconnect was ATTEMPTED for gone (even though it was concurrently removed)
assert any(arg[0] == gone for arg in create_calls), \
"reconnect_pass must attempt to reconnect the gone platform before it was removed"
assert stays in runner._failed_platformsFINDING 7.3: No End-to-End Reconnection Cycle Test (MEDIUM severity) Suggested Fix: Add an integration test: @pytest.mark.asyncio
async def test_full_reconnect_cycle_succeeds_and_clears_queue() -> None:
runner = _make_runner()
runner._running = True
platform = Platform.TELEGRAM
cfg = PlatformConfig(enabled=True, token="t")
runner.config.platforms[platform] = cfg
# Simulate platform failure
adapter = _FakeAdapter(platform, retryable=True, config=cfg)
await runner._handle_adapter_fatal_error(adapter)
# Platform should be queued
assert platform in runner._failed_platforms
# Mock successful reconnection
success_called = False
async def mock_create(*args, **kwargs):
nonlocal success_called
success_called = True
# Return a working adapter (mock)
adapter_mock = AsyncMock()
adapter_mock.platform = platform
adapter_mock.connect = AsyncMock()
adapter_mock.disconnect = AsyncMock()
return adapter_mock
monkeypatch.setattr(runner, "_create_adapter", mock_create)
# Run one reconnect pass
await runner._reconnect_pass()
# Verify reconnect was attempted and succeeded
assert success_called
# (In real code, successful reconnect removes from _failed_platforms)
# This test should verify that state transitionFINDING 7.4: Sidecar Timeout Not Tested in Actual Connect Path (MEDIUM severity) Suggested Fix: Add an integration test: @pytest.mark.asyncio
async def test_sidecar_ready_timeout_is_applied_during_connect(monkeypatch):
from plugins.platforms.photon.adapter import PhotonAdapter
monkeypatch.setenv("PHOTON_SIDECAR_READY_TIMEOUT", "10")
# Mock the sidecar startup process
timeout_used = None
original_httpx_client = None
class MockAsyncClient:
def __init__(self, timeout=None):
nonlocal timeout_used
timeout_used = timeout
async def __aenter__(self):
return self
async def __aexit__(self, *args):
pass
async def get(self, *args, **kwargs):
# Simulate slow sidecar startup
raise TimeoutError("sidecar not ready in 2.0s")
monkeypatch.setattr("httpx.AsyncClient", MockAsyncClient)
adapter = PhotonAdapter(config=PlatformConfig(enabled=True, token="t"))
with pytest.raises(RuntimeError, match="did not become ready"):
await adapter._start_sidecar()
# Verify the timeout used matches the env var value
assert timeout_used == 10.0, f"expected timeout 10.0s, got {timeout_used}"8. Breaking ChangesFINDING 8.1: Default Sidecar Timeout Changed from 15s to 60s (LOW severity) Analysis: This is NOT a breaking change — it's a bug fix. The old 15s was insufficient under load. The new 60s is more resilient. Any deployment that explicitly needs faster timeouts can set Verdict: ACCEPTABLE — this is a bug fix, not a regression. 9. Error Message QualityFINDING 9.1: Error Message for Missing Config Doesn't Suggest Remediation (LOW severity) logger.error(
"%s fatal error is retryable but no platform config is "
"available — cannot queue for reconnection; platform "
"stays down until gateway restart",
adapter.platform.value,
)Issue: The message tells the operator the platform is broken, but doesn't suggest next steps. "Platform stays down until gateway restart" is accurate but unhelpful. Suggested Fix: logger.error(
"%s fatal error is retryable but no platform config is available. "
"Cannot queue for reconnection. Ensure the platform is registered in "
"GatewayConfig.platforms or has adapter.config set. "
"Platform stays down until gateway restart.",
adapter.platform.value,
)10. Code QualityFINDING 10.1: Subtle Logic Ordering in Watcher Crash Handling (MEDIUM severity) if ran_for >= self._WATCHER_STABLE_RUN_S:
self._watcher_crash_streak = 0
delay = self._watcher_restart_backoff() # Uses streak (now 0 if stable)
self._watcher_crash_streak = getattr(self, "_watcher_crash_streak", 0) + 1Issue: The order is correct (reset streak IF stable, THEN calculate backoff based on streak), but the logic is subtle. Readers must simulate the order to understand that:
This is fragile to refactors. A developer might reorder the lines and break the logic without realizing. Suggested Fix: Make the logic explicit with intermediate variables: ran_for = time.monotonic() - self._watcher_started_at
is_stable = ran_for >= self._WATCHER_STABLE_RUN_S
# Calculate backoff based on current streak
delay = self._watcher_restart_backoff()
# Update streak: reset if stable, else increment
if is_stable:
self._watcher_crash_streak = 0
else:
self._watcher_crash_streak += 111. Changelog & Versioning (NO ESCAPE)FINDING 11.1: Missing CHANGELOG Entry and Version Bump (CRITICAL severity) Suggested Fix: Add to ## [Unreleased]
### Fixed
- **Gateway/Photon Resilience** (#19, #12, #13): Fixed 3-day outage scenario where photon platform showed "retrying" with nothing actually retrying. Root causes: (1) already-queued platforms couldn't be re-queued on new failures; (2) reconnect watcher died silently when _failed_platforms were mutated concurrently; (3) sidecar ready timeout hardcoded at 15s, insufficient under host load. Now: reconnect watcher is supervised with exponential backoff, already-queued platforms get retry refreshed immediately, sidecar timeout configurable via `PHOTON_SIDECAR_READY_TIMEOUT` env (default 60s).12. Refactor RecommendationsFINDING 12.1: Watcher Restart Callback Chain Is Too Deep (LOW severity — design smell) Suggested Refactor: Collapse def _on_reconnect_watcher_done(self, task: "asyncio.Task") -> None:
if task.cancelled() or not self._running:
return
# ... existing crash streak logic ...
delay = self._watcher_restart_backoff()
# ... logging ...
if self._running: # Check before scheduling (one line instead of a separate method)
asyncio.get_running_loop().call_later(delay, self._start_reconnect_watcher)FINDING 12.2: Defensive getattr() for Instance Attributes (LOW severity — style) Suggested Fix: Initialize in 13. Documentation (NO ESCAPE)CRITICAL DOCUMENTATION GAP: FINDING 13.1: New Environment Variable PHOTON_SIDECAR_READY_TIMEOUT Not Documented (CRITICAL severity) Scenario:
Suggested Fix:
FINDING 13.2: Reconnect Watcher Backoff Constants Not Documented (MEDIUM severity) Suggested Fix: Add inline documentation: _WATCHER_RESTART_DELAY = 5.0 # Base restart delay (seconds). Exponential backoff starts from this.
_WATCHER_RESTART_DELAY_CAP = 300.0 # 5 minutes max restart delay (prevents waiting forever for a broken watcher)
_WATCHER_STABLE_RUN_S = 60.0 # If watcher runs for 60+ seconds, assume it's stable and reset crash counter
# Rationale: A watcher that crashes after 60s stable operation likely hit a transient, not a startup bug.
# A watcher that crashes in <5s should back off to prevent rapid respawn loops.14. Lessons Learned Deposit (NO ESCAPE)File: Content Review: The file should document:
Verdict: ✅ GOOD — Lesson is actionable and prevents regression. 15. Documentation & Context Discovery (NO ESCAPE)15A. User-Facing Documentation Score: 40% (CRITICAL GAP)
15B. Context Discovery Diagram: NO (3+ files modified) Diagram Needed: YES — The PR touches multiple interconnected systems (watcher supervision, config fallback, timeout handling). A diagram would clarify the data flow. Suggested Diagram (Mermaid): graph TD
A["Platform Adapter Error"] -->|fatal_error_retryable| B["_handle_adapter_fatal_error"]
B -->|new platform| C["Queue in _failed_platforms"]
B -->|already queued| D["Refresh next_retry to now"]
C --> E["Reconnect Watcher"]
E -->|every 10s| F["_reconnect_pass"]
F -->|iterate platforms| G["if next_retry <= now"]
G -->|attempt| H["_create_adapter"]
H -->|success| I["Remove from queue"]
H -->|retryable failure| J["Increment attempts, set backoff"]
E -->|crash| K["Done-callback"]
K -->|exponential backoff| L["call_later restart"]
L -->|5-300s delay| E
Stored in: 15C. Technical Documentation Score: 50% (HIGH GAP)
Suggested Fix: Add to ## Photon Adapter Resilience
### Sidecar Ready Timeout
- Env var: `PHOTON_SIDECAR_READY_TIMEOUT` (seconds)
- Default: 60s (was 15s pre-PR#19; increased due to host load sensitivity)
- Min: 1s, Max: 600s
- Triggered: During `_start_sidecar()`, waits for `/healthz` endpoint to respond
### Reconnection Backoff
- Failed platforms queued in `_failed_platforms`
- Reconnect watcher runs every 10 seconds, attempts platforms when `next_retry <= now`
- Backoff: 30s → 60s → 120s → 240s → 300s (5m cap)
- If platform hits backoff cap and is still failing, it repeats at 300s intervals
- Config fallback: If platform missing from `GatewayConfig.platforms`, uses `adapter.config`
### Supervised Watcher
- Background task restarts automatically if it crashes
- Exponential backoff: 5s → 10s → 20s → ... → 300s (5m cap)
- Crash streak resets if watcher runs stably for 60+ seconds
- Max crashes before halt: Currently unbounded (TODO: add cap per PR#19 security feedback)Summary of Findings
Minimum Findings Requirement: 5 Final AssessmentThis PR addresses a real production incident (3-day Photon outage) with sound fixes to watcher supervision, config fallback, and timeout configuration. However, the PR has significant gaps in documentation and test coverage that would create operational blindness for the next operator facing this scenario. Blockers for merge:
Recommended pre-merge actions (estimated 2-3 hours):
VERDICT: CONCERN |
Principal Architect — PR #19 Review0. VISUAL VERIFICATION [MANDATORY FOR FRONTEND]N/A - no frontend changes 0B. PRODUCTION BUILD & PAGE STABILITY [MANDATORY FOR FRONTEND]N/A - no frontend changes 0C. MOBILE/TABLET UX REVIEW [MANDATORY FOR FRONTEND]N/A - no frontend changes 0D. ENV VARS OVER MOCKS [MANDATORY FOR ALL PRs]
1. User Experience & Flow [DEEP DIVE]
2. UI Quality & Polish [DEEP DIVE]N/A - no frontend changes.
3. Wiring & Integration [DEEP DIVE]
4. Security [DEEP DIVE]
5. AccessibilityN/A - no UI changes.
6. Performance Impact
7. Test Coverage Delta & Test Quality
8. Breaking Changes
9. Error Message Quality
10. Code Quality
11. Changelog & Versioning [NO ESCAPE]
12. Refactor Recommendations
13. Documentation [NO ESCAPE]
14. Lessons Learned Deposit [NO ESCAPE]
15. Documentation & Context Discovery [NO ESCAPE]
Summary: This PR introduces critical and well-engineered resilience fixes for the gateway reconnect watcher, effectively neutralizing the silent death bug. However, it requires a few structural cleanups (PEP-8 indentation, fixing a stale timestamp TOCTOU, and using monotonic time for timeouts) and mandatory documentation updates before merge. VERDICT: CONCERN |
[
{
"id": "UX-001",
"severity": "bug",
"confidence": 0.95,
"category": "user_experience",
"file": "gateway/run.py",
"line": 3997,
"title": "Re-queue refresh bypasses exponential backoff",
"description": "In `_handle_adapter_fatal_error`, when a platform is already in `_failed_platforms`, `next_retry` is unconditionally updated to `time.monotonic()`. If an adapter enters a crash-loop, this resets the backoff timer on every crash, starving the system and potentially DDoS-ing the downstream service.",
"evidence": "Traced `_handle_adapter_fatal_error` and `_reconnect_pass`. The backoff calculation relies on `next_retry = time.monotonic() + ...`, which gets completely obliterated by the forced reset to `time.monotonic()`.",
"suggestion": "Update `next_retry` using `max(time.monotonic(), info.get(\"next_retry\", 0.0))` or explicitly reset `attempts` to 0 if the intention is truly to restart the backoff clock from scratch.",
"is_new": true,
"origin": "NEW"
},
{
"id": "PERF-002",
"severity": "bug",
"confidence": 0.85,
"category": "performance",
"file": "plugins/platforms/photon/adapter.py",
"line": 1008,
"title": "Sidecar readiness loop uses non-monotonic time",
"description": "`deadline = time.time() + ready_timeout` uses wall-clock time, which is subject to NTP adjustments and system clock shifts. If the clock shifts backward during the wait, the sidecar readiness check could hang indefinitely.",
"evidence": "The loop condition `while time.time() < deadline:` directly compares wall-clock timestamps, violating standard asyncio timeout practices.",
"suggestion": "Replace `time.time()` with `time.monotonic()` to ensure consistent passage of time.",
"is_new": true,
"origin": "NEW"
},
{
"id": "REL-003",
"severity": "bug",
"confidence": 0.80,
"category": "reliability",
"file": "gateway/run.py",
"line": 7705,
"title": "Watcher attributes missing from __init__",
"description": "`_watcher_started_at` and `_watcher_crash_streak` are set dynamically in `_start_reconnect_watcher` but not initialized in `__init__` alongside `_reconnect_watcher_task`. If these methods are called out of sequence, or if `_on_reconnect_watcher_done` runs before `_start_reconnect_watcher` completes, it will raise `AttributeError`.",
"evidence": "Checked `__init__` method where `_reconnect_watcher_task` was properly added, but the other two watcher supervision state variables were missed.",
"suggestion": "Initialize `self._watcher_started_at = 0.0` and `self._watcher_crash_streak = 0` inside `__init__`.",
"is_new": true,
"origin": "NEW"
}
]Verification SummaryMode: verification Bugs (must fix)
Nits (optional)
Pre-existing (awareness)
Verification Delta (verification mode only)
VERDICT: CONCERN |
PR Review Summary -- Multi-AI ConsensusPR: #19 | Voters: 4/5 usable
Reviewed by: claude-sonnet, minimax, claude-haiku, gemini-pro, glm-v3-verifier CI Checks: No CI checks configured Individual reviews posted as separate comments above. Generated at 2026-08-02 22:15 UTC |
PR #19 Review (Sonnet fallback, file-access verified)Reviewed the actual diff ( 0/0B/0C/0D — N/A, backend onlyNo 1-3 — Wiring / correctnessThe three fixes are genuine and each traces to a stated incident:
Minor code-quality nit (LOW, non-blocking): in 4 — Security4A (traditional): No SQL/command injection surface touched. The One residual gap worth flagging (MEDIUM): the exemption widens the writable surface to the entire tempdir, not just Hermes's own subtree within it. Any other process's temp files (other users' 4C (architectural): The 4E: No protected system-state file writes introduced; the Hermes-config-blocking check is untouched and still runs after the tempdir exemption. 7 — Test coverageGenuine, non-trivial tests throughout — this is a strength of the PR. Tests assert real values (backoff schedule numbers, spawn counts, timeout clamping) rather than "did not raise." The Gap (LOW): no test exists for the new 8-15Nothing else material changed (no schema, deploy, docs, or dependency changes) — N/A with the above as full justification. Severity Counts: CRITICAL: 0 | HIGH: 0 | MEDIUM: 1 | LOW: 2 | SUGGESTIONS: 1 VERDICT: APPROVE |
PR #19 Review — Opus fallback reviewer (file-access verified)Head Verification performed
Sections0 / 0B / 0C / 0D — Visual, build, mobile, env-var mock verification: N/A — backend-only Python; no 1-2. Correctness / logic. The fatal-error handler (gateway/run.py:3974-4010) now covers both previously-silent drops: missing static platform config falls back to 3. Wiring — HIGH, the main finding. 4. Security.
6. Concurrency / lifecycle (MEDIUM). Crash-streak accounting (LOW). In Sidecar timeout (LOW). 7. Test coverage. Genuinely good — each test names the incident it pins and asserts behaviour rather than implementation, and the invalid-value parametrisation ( 8-10. Style, performance, docs. 11. Changelog. N/A — internal daemon tooling; no user-facing release notes in this repo. 12-15. Rollback, observability, config, product. Rollback is clean (revert restores the 15s constant and the bare Bottom lineThe three incident fixes are real, correctly scoped, and well tested; the blockers are the half-wired hook enrichment (security lane live on one execution path only) and the Severity Counts: CRITICAL: 0 | HIGH: 2 | MEDIUM: 2 | LOW: 4 | SUGGESTIONS: 5 VERDICT: CONCERN |
Opus fallback-review finding (PR #19): TMPDIR is attacker-influenceable — TMPDIR=/ would have made the exemption a global sensitive-path bypass. The exemption now requires the resolved tempdir itself to sit under /var/folders or /tmp (macOS/Linux temp roots). RED-first test included. 234 tests green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PR #19 Review (Sonnet fallback, file-access verified)This review applies to head Reviewed the full diff from The delta commit (
|
PR #19 Review (rewritten) — Opus fallback reviewer, head
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b5d453caf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # prefix would otherwise block, making tempfile.gettempdir() unwritable. | ||
| # Only the prefix check is exempted; exact-path and Hermes-config checks | ||
| # below still apply to tempdir paths. | ||
| tmp_root = os.path.realpath(tempfile.gettempdir()).rstrip("/") + "/" |
There was a problem hiding this comment.
On a hardened or read-only host where Python cannot find any usable temporary directory, tempfile.gettempdir() raises FileNotFoundError. Because _check_sensitive_path() is called outside the error-handling blocks in both write_file_tool() and patch_tool(), this makes every file write fail—even writes to an otherwise writable workspace. Treat failure to resolve the tempdir as “no exemption” and continue applying the normal sensitive-path checks.
Useful? React with 👍 / 👎.
| _TEMP_ROOTS = ("/var/folders/", "/private/var/folders/", "/tmp/", "/private/tmp/") | ||
| in_process_tmp = tmp_root.startswith(_TEMP_ROOTS) and os.path.realpath( |
There was a problem hiding this comment.
On macOS when TMPDIR is configured as the conventional /var/tmp, realpath() yields /private/var/tmp/, but that directory matches none of these allowed roots. A write beneath the process tempdir is consequently still rejected by the /private/var/ sensitive prefix, defeating this exemption for a valid system temp location. Include /var/tmp/ and /private/var/tmp/ among the boundary-checked temp roots.
Useful? React with 👍 / 👎.
…ar-resilience # Conflicts: # gateway/run.py # tools/file_tools.py
PR #19 Review (Sonnet fallback, file-access verified)This review applies to head
Verification: ran the targeted suite post-merge: Assessment (carried forward from
|
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PR #19 Review (Sonnet fallback, file-access verified)This review applies to head The delta from The substantive review below is unchanged from the
Verification: ran the targeted suite post-merge: Assessment (carried forward from
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe7e10f010
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| slow respawn beats recreating the permanent silent outage). | ||
| """ | ||
| self._watcher_started_at = time.monotonic() | ||
| task = asyncio.create_task(self._platform_reconnect_watcher()) |
There was a problem hiding this comment.
When shutdown begins while this watcher is awaiting a slow platform connect, this bare task is absent from _background_tasks, so stop() does not cancel it in its background-task cleanup. Because _reconnect_pass() checks _running only before the connect await and then unconditionally installs a successful adapter, the task can finish after shutdown has disconnected and cleared the adapter maps, repopulating self.adapters with a live adapter on a stopped runner. Reuse or extend _spawn_supervised() so the watcher remains tracked and cancelled with the other runner-owned tasks.
AGENTS.md reference: AGENTS.md:L75-L79
Useful? React with 👍 / 👎.
PR #19 Review (final) — Opus fallback reviewer, head
|
1 participant
Fixes #12, closes #13.
Problem
Photon (iMessage) died 2026-07-30 and the gateway showed "retrying" for 3 days with nothing retrying. Three stacked defects:
self.config.platformscould never queue).create_task— a single KeyError from concurrent_failed_platformsmutation killed it permanently, leaving state lying "retrying".Fix
gateway/run.py:_handle_adapter_fatal_errorfalls back toadapter.configwhen the platform is absent from gateway config, and refreshesnext_retrywhen already queued (unless paused); watcher wrapped in_start_reconnect_watcherwith a done-callback that restarts it after 5s while running; per-pass body extracted to_reconnect_passwith tolerant.get()iteration (concurrent removal skips, never KeyError).plugins/platforms/photon/adapter.py: sidecar ready timeout nowPHOTON_SIDECAR_READY_TIMEOUTenv, default 60s, invalid/non-positive values fall back safely.Tests (TDD, RED first)
tests/gateway/test_reconnect_watcher_resilience.py— 4 new tests (re-queue refresh, config fallback, race tolerance, supervised restart).tests/plugins/platforms/photon/test_sidecar_ready_timeout.py— 6 new tests (default/override/invalid).Lessons learned
~/.claude/lessons-learned/20260802_hermes_photon_silent_retry_death.md — "A status field that says 'retrying' is only true if something provably retries."
🤖 Generated with Claude Code