fix(gateway): expose agent exit metadata to hooks - #2
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Warning Review limit reached
Next review available in: 8 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughGateway와 Proxy 실행 결과에 Changes에이전트 종료 메타데이터
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GatewayRunner
participant Agent
participant AgentEndHook
GatewayRunner->>Agent: 에이전트 실행 및 종료 정보 수집
Agent->>GatewayRunner: 결과, 중단 정보, API 호출 횟수 반환
GatewayRunner->>AgentEndHook: 정규화된 turn_exit_reason과 api_call_count 전달
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
![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: 7aa5bef627
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
tests/gateway/test_agent_end_hook_metadata.py (1)
122-126: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
agent:end컨텍스트 추출 로직을 헬퍼로 묶는 편이 좋습니다.동일한 리스트 컴프리헨션이 이 파일에서 9회 반복됩니다. 또한
call.args[0]인덱싱은 프로덕션 호출이 키워드 인자로 바뀌면IndexError로 실패하므로, 헬퍼에서call.args/call.kwargs를 함께 다루면 더 견고해집니다.♻️ 제안 리팩터
def _agent_end_contexts(runner): contexts = [] for call in runner.hooks.emit.await_args_list: event = call.args[0] if call.args else call.kwargs.get("event_type") if event != "agent:end": continue contexts.append( call.args[1] if len(call.args) > 1 else call.kwargs.get("context") ) return contexts🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/gateway/test_agent_end_hook_metadata.py` around lines 122 - 126, Extract the repeated agent:end context list comprehension into a shared _agent_end_contexts(runner) test helper and replace all nine occurrences with it. Make the helper read the event and context from positional arguments when present, otherwise fall back to the event_type and context keyword arguments, while preserving the existing filtering and ordering.tests/gateway/test_proxy_mode.py (3)
590-596: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value인접 bytes 리터럴이 하나의 청크로 결합됩니다.
콤마가 없어 두 줄이 단일
bytes로 이어집니다. 동작에는 문제가 없지만 위쪽test_proxy_stale_generation_returns_empty_result(콤마로 2개 청크)와 형태가 달라 의도가 모호합니다. 다중 청크를 의도했다면 콤마를 추가해 주세요.♻️ 제안
sse_chunks=[ - b'data: {"choices":[{"delta":{"content":"stale"}}]}\n\n' - b"data: [DONE]\n\n" + b'data: {"choices":[{"delta":{"content":"stale"}}]}\n\n', + b"data: [DONE]\n\n", ],🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/gateway/test_proxy_mode.py` around lines 590 - 596, Update the sse_chunks list in the _FakeSSEResponse setup to add a comma between the two adjacent bytes literals, ensuring the stale content and [DONE] events are represented as separate chunks like test_proxy_stale_generation_returns_empty_result.
405-410: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSSE 버퍼 캡 위반이
gateway_proxy_connection_error와 같은 사유로 뭉쳐집니다.버퍼 초과는 업스트림 프로토콜 위반(줄 경계 없는 무한 스트림)이고 실제 연결 실패와 원인·대응이 다릅니다. 이 PR이 사유 어휘를 세분화하는 목적이라면
gateway_proxy_protocol_error같은 별도 사유가 관측성에 유리합니다. 프로덕션 분류 변경이 필요하므로 후속 작업으로 남겨도 무방합니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/gateway/test_proxy_mode.py` around lines 405 - 410, Separate SSE buffer-cap violations from connection failures in the proxy handling flow: classify buffer overflow as gateway_proxy_protocol_error instead of gateway_proxy_connection_error, while preserving the existing reason for genuine connection failures. Update the assertions for result["turn_exit_reason"] accordingly.
599-603: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
side_effect=[True, False]는_is_session_run_current호출 횟수에 취약합니다.구현이 세대 확인을 3회 이상 호출하도록 바뀌면(예: 스트림 컨슈머 활성화 경로 추가)
StopIteration으로 테스트가 깨져 원인 파악이 어렵습니다. "첫 호출만 현재, 이후는 stale" 의미를 함수로 표현하면 호출 횟수와 무관하게 동작합니다.♻️ 제안 리팩터
+ calls = {"n": 0} + + def _current_then_stale(*_args, **_kwargs): + calls["n"] += 1 + return calls["n"] == 1 + with patch.object( runner, "_is_session_run_current", - side_effect=[True, False], + side_effect=_current_then_stale, ):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/gateway/test_proxy_mode.py` around lines 599 - 603, 테스트의 `_is_session_run_current` 패치가 고정된 호출 횟수에 의존하고 있습니다. `side_effect=[True, False]` 대신 호출 상태를 추적하는 함수형 side effect를 사용해 첫 호출에는 현재 상태를 반환하고 이후 모든 호출에는 stale 상태를 반환하도록 수정하세요. 이렇게 `_is_session_run_current` 호출 횟수가 늘어나도 테스트가 동일한 의미를 유지하게 하세요.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@gateway/run.py`:
- Line 22042: Update the exception path returning
"gateway_agent_runtime_resolution_failed" to include "failed": True in the same
result object. Keep the existing turn_exit_reason and provider authentication
error details unchanged so _handle_message_with_agent recognizes this as an
early agent failure and avoids persisting the assistant error text.
In `@website/docs/user-guide/features/hooks.md`:
- Line 95: Update the finalizer-reason documentation to distinguish preserved
reason semantics from normalized delivery text: in
website/docs/user-guide/features/hooks.md lines 95-95, replace “pass through”
with wording that states the reason is preserved while the delivered string is
normalized; make the corresponding change from “原样传递” to “规范化后保留” in
website/i18n/zh-Hans/docusaurus-plugin-content-docs/current/user-guide/features/hooks.md
lines 91-92, keeping both documents consistent with one-line folding and
200-character truncation.
---
Nitpick comments:
In `@tests/gateway/test_agent_end_hook_metadata.py`:
- Around line 122-126: Extract the repeated agent:end context list comprehension
into a shared _agent_end_contexts(runner) test helper and replace all nine
occurrences with it. Make the helper read the event and context from positional
arguments when present, otherwise fall back to the event_type and context
keyword arguments, while preserving the existing filtering and ordering.
In `@tests/gateway/test_proxy_mode.py`:
- Around line 590-596: Update the sse_chunks list in the _FakeSSEResponse setup
to add a comma between the two adjacent bytes literals, ensuring the stale
content and [DONE] events are represented as separate chunks like
test_proxy_stale_generation_returns_empty_result.
- Around line 405-410: Separate SSE buffer-cap violations from connection
failures in the proxy handling flow: classify buffer overflow as
gateway_proxy_protocol_error instead of gateway_proxy_connection_error, while
preserving the existing reason for genuine connection failures. Update the
assertions for result["turn_exit_reason"] accordingly.
- Around line 599-603: 테스트의 `_is_session_run_current` 패치가 고정된 호출 횟수에 의존하고 있습니다.
`side_effect=[True, False]` 대신 호출 상태를 추적하는 함수형 side effect를 사용해 첫 호출에는 현재 상태를
반환하고 이후 모든 호출에는 stale 상태를 반환하도록 수정하세요. 이렇게 `_is_session_run_current` 호출 횟수가 늘어나도
테스트가 동일한 의미를 유지하게 하세요.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 986a765c-cbe8-4ddb-8942-ea28635c10ae
📒 Files selected for processing (6)
gateway/hooks.pygateway/run.pytests/gateway/test_agent_end_hook_metadata.pytests/gateway/test_proxy_mode.pywebsite/docs/user-guide/features/hooks.mdwebsite/i18n/zh-Hans/docusaurus-plugin-content-docs/current/user-guide/features/hooks.md
📜 Review details
🧰 Additional context used
🪛 ast-grep (0.45.0)
tests/gateway/test_proxy_mode.py
[warning] 235-235: Do not make http calls without encryption
Context: "http://host:8642"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
[warning] 415-415: Do not make http calls without encryption
Context: "http://host:8642"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
[warning] 446-446: Do not make http calls without encryption
Context: "http://host:8642"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
[warning] 584-584: Do not make http calls without encryption
Context: "http://host:8642"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
🔇 Additional comments (21)
website/docs/user-guide/features/hooks.md (1)
83-93: LGTM!Also applies to: 96-107
website/i18n/zh-Hans/docusaurus-plugin-content-docs/current/user-guide/features/hooks.md (1)
83-89: LGTM!Also applies to: 94-100, 1347-1347
gateway/run.py (7)
2656-2749: 정규화 헬퍼 및_gateway_turn_exit_reason분류 로직 — 테스트 케이스와 정확히 일치함
_normalize_turn_exit_reason,_is_generic_agent_interrupt_exit_reason,_gateway_turn_exit_reason의 우선순위 로직(명시적 사유 우선 → 제네릭interrupt*사유는 인터럽트 마커로 재분류 → 폴백)을test_agent_end_hook_metadata.py의 세 가지 케이스(가드레일 정지 사유 보존,Stop requested→interrupted_by_user, 제어 문자 스트리핑)로 직접 추적 검증했고 모두 일치합니다.
14724-14738: LGTM!
20396-20397: LGTM!Also applies to: 20407-20408
20539-20539: 프록시 스트리밍 경로의turn_exit_reason파생 로직 확인 완료
proxy_partial은 예외 발생 시점에 이미full_response가 비어있지 않은 경우에만True로 설정되고, 최종 반환부의turn_exit_reason삼항 분기(partial/complete/empty)는 이 불변조건과 일치합니다."partial": False를 항상 반환하는 것은 주석에 명시된 대로 기존 다운스트림 제어 흐름을 보존하기 위한 의도적 설계이며,full_response가 항상 채워지므로("(No response from remote agent)"폴백 포함)_normalize_empty_agent_response등 하위 소비자에 영향이 없음을 확인했습니다.Also applies to: 20561-20562, 20582-20584, 20626-20630, 20656-20657, 20675-20687
23182-23182: LGTM!Also applies to: 23310-23313
23624-23624: LGTM!
23812-23812: LGTM!gateway/hooks.py (1)
33-43: LGTM!tests/gateway/test_agent_end_hook_metadata.py (6)
18-76: LGTM!
79-161: LGTM!
164-225: LGTM!
258-350: LGTM!
353-476: LGTM!
479-661: LGTM!tests/gateway/test_proxy_mode.py (5)
43-59: LGTM!
234-268: LGTM!
346-348: LGTM!Also applies to: 379-381
412-467: LGTM!
537-539: LGTM!
Review for the governed merge — conditional, one change neededThis pull request is being reviewed as a compensating control, not as a formality. The governance sandbox cannot meaningfully validate this repository: its toolchain image has no pip and no network, while production code imports 32 exact-pinned third-party packages. Measured directly — Finding — the stated problem remains in its most severe case
The catch-all at line 15169 does not:
That is the exact phenomenon this pull request describes as a silent abnormal exit, still present in the most abnormal case there is: a crash. It also breaks the The 815-line test file uses Fix: emit Secondary observationsDocumentation is inaccurate about proxy mode. The new text says proxy mode reports A raw exception message can now reach the hook. Stale reclassification is asymmetric. Only What was verified, and howThe twelve deleted lines are a refactor of The new payload keys are purely additive; the eight existing keys are untouched, so current consumers are unaffected. The classification helpers were extracted from the diff and run against twenty-one scenarios covering normal completion, five system interrupt markers, unclassified interrupts, user correction text, generic-interrupt override, an unstringable reason, an exploding count, ANSI and NUL control characters, and negative counts. All matched expectations. User correction text returns the fixed literal Every new function is pure and reads only from the per-call Not covered
|
|
Superseded by #3. The blocker was not the review finding — it was the base. This PR targets So merging this PR and deploying the result would have rolled the running gateway back 538 commits. Issue NousResearch#15's remaining acceptance criteria require proving a deployed SHA and a live hook artifact — which that deploy could not honestly satisfy. #3 re-derives this change on All five test functions this PR newly introduced in Leaving this open pending the merge decision on #3. |
|
Superseded by #3, merged as `34845717347e4b696667a14ff20f6a4c1f1c05f8` and deployed. That branch carries a strictly more evolved version of this change — |
1 participant
Summary
Expose trustworthy turn-exit metadata in the Gateway
agent:endhook for theowned Hermes fork.
turn_exit_reasonand non-negativeapi_call_count.restart, unclassified interrupt, runtime-resolution failure, and proxy exit
classes.
partial-stream evidence through
turn_exit_reason.unknown.Root cause
The Gateway emitted only the response text to
agent:end. Finalizer metadatawas available in the agent result but did not reach the hook artifact and
conductor. Early Gateway and proxy exits also lacked a stable termination
classification.
Scope
158e9a99779428245c7f524aade8ab398e44676c7aa5bef627a0a815628b3b321c235f254da17200https://github.com/ComBba/hermes-agent--gemma4-12b-mlx/issues/15
hermes-governed-autonomy-rel-01Validation
py_compile: passgit diff --check: passNO ACTIONABLE FINDINGSGovernance and deployment
This PR targets only
ComBba/hermes-agent. No upstream Issue or PR is part ofthis workflow. Merge, live deployment, Gateway restart, configuration changes,
and production writes remain outside this Draft PR and require separate
exact-SHA approval.
Rollback
Before merge, close the PR or revert the single candidate commit. Before any
approved deployment, preserve the live checkout SHA, Gateway PID, and hashes of
the deployed hook/conductor files. A deployment rollback must restore that
pre-state and prove PID, Discord reconnection, and hook/conductor read-back.
Summary by CodeRabbit
새로운 기능
agent:end이벤트에 에이전트 종료 사유(turn_exit_reason)와 API 호출 횟수(api_call_count)가 추가됩니다.문서