test: native provider E2E suites: Bedrock Converse, Gemini, Vertex, Codex app-server and Copilot ACP wire contracts proven against validating fakes - #121345
Conversation
૮ >ﻌ< ა ci reviewran on f0f5212 — test: gate the ACP crash-retry count on #121467 (merge-order debug infoCI timingsCI timings · View report · View jobWall time 7m26s vs 9m17s (-19.9%). 5 job(s) slower, 8 faster, 1 unchanged.
|
arkheioncorp
left a comment
There was a problem hiding this comment.
Reviewer verdict: COMMENT
Summary: Native provider E2E suites for Bedrock Converse, Gemini, Vertex, Codex app-server, and Copilot ACP wire contracts. Adds shared harness _native_helpers.py and fault/turn test suites.
Findings:
- The harness design is solid: hermetic fake HOME, env allowlisting (no credential passthrough via
_SECRET_ENV_SUFFIXES+_PASSTHROUGH_ENV), real subprocesses, real SQLite reads. HERMES_STATE_DB_GUARD_BYPASS=1is documented as a test escape hatch — acceptable for E2E.- No security concerns in the diff: no hardcoded secrets, no shell injection vectors, no eval/exec.
- Test design: fault semantics properly parametrize retryable vs terminal faults; xfail markers reference issue numbers for known bugs.
- The
adopt_foreign_durable_rowsfunction in the compression PR (121340) is referenced conceptually here — good integration awareness.
Suggestions (non-blocking):
- Consider adding a brief note in the harness docstring about what the fakes must NOT do (e.g., must not write to real HOME, must not touch network).
- The
make_homefunction writes.envwith AWS creds; ensure the fake creds intests/fakes/providers/bedrock_converse.pyare clearly non-real placeholders (they appear to be consts likeACCESS_KEY/SECRET_KEY— if those are fake values, fine).
Verdict: COMMENT — design looks sound, no blockers. Ready for merge after CI passes.
arkheioncorp
left a comment
There was a problem hiding this comment.
Reviewer verdict: COMMENT
Summary: Native provider E2E suites for Bedrock Converse, Gemini, Vertex, Codex app-server, and Copilot ACP wire contracts. Adds shared harness _native_helpers.py and fault/turn test suites.
Findings:
- The harness design is solid: hermetic fake HOME, env allowlisting (no credential passthrough via _SECRET_ENV_SUFFIXES + _PASSTHROUGH_ENV), real subprocesses, real SQLite reads.
- HERMES_STATE_DB_GUARD_BYPASS=1 is documented as a test escape hatch — acceptable for E2E.
- No security concerns in the diff: no hardcoded secrets, no shell injection vectors, no eval/exec.
- Test design: fault semantics properly parametrize retryable vs terminal faults; xfail markers reference issue numbers for known bugs.
Suggestions (non-blocking):
- Consider adding a brief note in the harness docstring about what the fakes must NOT do (e.g., must not write to real HOME, must not touch network).
- The make_home function writes .env with AWS creds; ensure the fake creds in tests/fakes/providers/bedrock_converse.py are clearly non-real placeholders.
Verdict: COMMENT — design looks sound, no blockers. Ready for merge after CI passes.
…rmetic home, state.db readers)
…V4-verified eventstream fake, tools, signed reasoning, resume, compaction, faults)
… Google TLS endpoint + tools/resume, schema, errors, compaction)
…uth/model-path check, #121317 xfail
… fake, tools, signatures, errors, compaction, stream drop)
…cle/fault suites)
…ow, resume, lifecycle, error semantics Fake ACP agent executable (tests/fakes/providers/copilot_acp.py) validates every client request against the agent-client-protocol schema and replays scripted turns. Suites drive real hermes chat -q: tool round trip via the prompt tool bridge, model selection, fail-closed permissions, cwd-bounded fs, resume = fresh session/new seeded with history, no orphaned agents (SIGTERM-ignoring agent), and JSON-RPC error / crash retry semantics. Strict xfails: #65788 (late chunk), #121290 (auth remedy).
…(TUI events + nested hermes acp) Strict xfails for #120550 (tui_gateway gets no reasoning/message delta until the ACP turn ends) and #101507 (hermes acp over copilot-acp forwards inner chunks only after the inner turn). Timestamps are compared against the fake agent's own clock; both xfails were proven to XPASS under a local streaming fix simulation (reverted).
…valid and grounded Summarizer runs through the ACP agent itself; the post-compaction prompt must carry the summary, the user's ask and the latest tool result, drop the summarized tool output, and pass schema validation.
…; codex orphan released without a foreign kill Review fixes for #121345. - Every strict xfail in tests/e2e/core/providers now uses raises=KnownSymptom (a shared AssertionError subclass in _native_helpers) and raises it only at the tracked bug's symptom. Preconditions (exit codes, request counts, the approval exchange, the descendant pid) are plain asserts, so harness failures - StopIteration from next(...), tuple-unpack errors, process death, fixture teardown errors - fail for real instead of counting as the known bug. pytest applies xfail to setup/teardown reports too; the codex faults file's "5 xfailed" for 4 xfail tests was the teardown error being eaten. Verified with --runxfail: all 19 KNOWN cells fail with KnownSymptom. - Codex fake: the own-session grandchild (#121298) now lives until CodexRun.cleanup() drops a release file, so teardown retires the orphan without signalling a process that was reparented to init (the live-system guard blocked that kill and leaked a sleep(600) per run on dev boxes). Both codex modules carry live_system_guard_bypass for the SIGKILL fallback, matching tests/agent/test_shell_hooks_tree_kill.py. - Copilot ACP resume test no longer asserts session/load is never sent (not a documented contract); it keeps the new-agent-process, in-order history and no-duplicate asserts. - Copilot teardowns tolerate a pid exiting between the liveness check and the kill.
656d938 to
bcd2dae
Compare
…ner's reach CI's tirith build flags arithmetic expansion (echo MARK-$((6*7))) as HIGH 'Nested expansion' and single-query mode blocks it, so the terminal leg of the parallel toolUse round trip never ran. The test is about toolUse/toolResult pairing, not the scanner: use a plain pipeline that still computes the marker (seq | sed) so a fake echoing the command text cannot satisfy MARK-42.
CI caught crash_every_time_bounded making 4 model calls against a 2-retry budget: when the CLI's stderr lags its exit the client raises TimeoutError instead of 'exited early', and timeouts retry on another budget. Deterministic with a lagged stderr pump (5 calls, 126 s). Fix is #121468; until it lands the cell XFAILs on exactly that signature, passes once fixed, and any other failure stays red.
…; codex orphan released without a foreign kill Review fixes for #121345. - Every strict xfail in tests/e2e/core/providers now uses raises=KnownSymptom (a shared AssertionError subclass in _native_helpers) and raises it only at the tracked bug's symptom. Preconditions (exit codes, request counts, the approval exchange, the descendant pid) are plain asserts, so harness failures - StopIteration from next(...), tuple-unpack errors, process death, fixture teardown errors - fail for real instead of counting as the known bug. pytest applies xfail to setup/teardown reports too; the codex faults file's "5 xfailed" for 4 xfail tests was the teardown error being eaten. Verified with --runxfail: all 19 KNOWN cells fail with KnownSymptom. - Codex fake: the own-session grandchild (#121298) now lives until CodexRun.cleanup() drops a release file, so teardown retires the orphan without signalling a process that was reparented to init (the live-system guard blocked that kill and leaked a sleep(600) per run on dev boxes). Both codex modules carry live_system_guard_bypass for the SIGKILL fallback, matching tests/agent/test_shell_hooks_tree_kill.py. - Copilot ACP resume test no longer asserts session/load is never sent (not a documented contract); it keeps the new-agent-process, in-order history and no-duplicate asserts. - Copilot teardowns tolerate a pid exiting between the liveness check and the kill.
Hermes' five native (non-chat-completions) provider dialects — Bedrock Converse, Gemini native, Vertex, the Codex app-server protocol and Copilot ACP — now have end-to-end wire-conformance suites that drive the real
hermes chat -qCLI against fakes that validate every request the way the vendor would, with 18 real bugs pinned as strict xfails.What is real vs faked
hermesCLI subprocess (hermetic fake HOME/HERMES_HOME,--resumein a new process), runtime provider resolution, the adapters, the agent loop, real tools (read_file,terminal), auto compaction,state.db.tests/fakes/providers/:bedrock_converse.py: real boto3 reaches it viaAWS_ENDPOINT_URL_BEDROCK_RUNTIME. It recomputes SigV4, validates bodies with the botocorebedrock-runtimeConverse/ConverseStreamParamValidatorplus the semantic rules (toolUse/toolResult pairing, reasoning signatures), and streams realvnd.amazon.eventstreamframes with CRCs.gemini_native.py: a loopback HTTPS CONNECT proxy that terminates TLS forgenerativelanguage.googleapis.comwith a throwaway CA (HTTPS_PROXY+SSL_CERT_FILE). The native adapter only engages for the real host, and no Hermes code is patched. It validates against the published v1beta/v1 shapes and returns Google's 400INVALID_ARGUMENTbody.vertex.py: a Google OAuth/tokenendpoint. Real google-auth signs an RS256 JWT from a generated service-account key and the fake verifies it. It also runs the same TLS proxy for*-aiplatform.googleapis.com, which validates the project/location path, the minted bearer, tool pairing and Gemini 3 thought signatures.codex_app_server.py: a fakecodex app-serverexecutable speaking newline-delimited JSON-RPC. Its schema subset and serde-style errors come fromcodex app-server generate-json-schemaand an offline round trip with the real binary. It validates Hermes' replies to approval requests and persists thread rollouts, so resume works across processes.copilot_acp.py: a fake ACP agent executable. It answers--helpwith--acp, validates every request against theacp.schemapydantic models and records a timestamped JSONL transcript with PIDs.Scenarios (test | what it proves | sabotage → observed red)
Every passing test was proven red by at least one temporary production edit, then reverted. The table has 56 rows, and several rows record more than one sabotage. Two were re-run by the lane parent on the merged branch: Gemini
_tool_call_extra_signaturereturns None, and Bedrock toolResulttoolUseId + "-x". Both went red as recorded.turns::test_every_call_is_sigv4_signed_for_bedrock_with_the_profile_credentialsturns::test_parallel_tool_uses_round_trip_as_tool_results_paired_by_idturns::test_tool_turn_prints_final_answer_and_persists_paired_rowsturns::test_resume_in_new_process_replays_tool_history_valid_for_converseturns::test_compaction_in_reasoning_session_keeps_every_request_converse_validturns::test_compaction_persists_summary_and_archives_compacted_rowsturns::test_fake_rejects_requests_bedrock_rejects[7 cases]faults::test_retryable_fault_is_retried_once_into_the_answer[throttle_http,unavailable_http,throttle_in_stream]faults::test_mid_stream_drop_retries_without_duplicated_persisted_contentfaults::test_streaming_iam_denial_falls_back_to_conversetest_native_gemini_tools.py::test_function_call_round_trip_pairs_response_and_persiststest_native_gemini_tools.py::test_resume_replays_thought_signatures_verbatimtest_native_gemini_tools.py::test_every_request_authenticates_and_targets_the_configured_modeltest_native_gemini_schema.py::test_v1beta_json_schema_declaration_is_sanitizedtest_native_gemini_schema.py::test_v1_proto_schema_declaration_is_acceptedparametersthat pass Google's field-by-field proto Schema checks, and the MCP call round-trips.test_native_gemini_errors.py::test_retryable_error_is_retried_then_succeeds[rate_limited_429]test_native_gemini_errors.py::test_retryable_error_is_retried_then_succeeds[unavailable_503]test_native_gemini_errors.py::test_non_retryable_surfaced_once[invalid_argument_400]test_native_gemini_errors.py::test_non_retryable_surfaced_once[safety]test_native_gemini_errors.py::test_non_retryable_surfaced_once[recitation]test_native_gemini_errors.py::test_stream_drop_recovers_without_duplicate_rowstest_native_gemini_compaction.py::test_compaction_happens_on_the_wiretest_native_gemini_compaction.py::test_post_compaction_requests_are_valid_and_signedtest_native_gemini_compaction.py::test_persisted_transcript_keeps_pairs_and_signaturestest_native_vertex_tools.py::test_tool_result_goes_back_paired_to_the_signed_calltest_native_vertex_tools.py::test_wire_scheme_bearer_and_single_token_minttest_native_vertex_tools.py::test_reasoning_effort_reaches_vertex_as_thinking_configtest_native_vertex_tools.py::test_thought_signature_replayed_after_resume_in_new_processtest_native_vertex_tools.py::test_expired_token_is_reminted_and_request_retriedtest_native_vertex_errors.py::test_transient_error_is_retried_then_answers[rate_limited|unavailable]test_native_vertex_errors.py::test_terminal_error_surfaced_once_without_retry[invalid_argument|permission_denied]test_native_vertex_errors.py::test_rejected_bearer_refreshes_once_then_surfacestest_native_vertex_errors.py::test_oauth_invalid_grant_sends_nothing_and_names_the_credentialtest_native_vertex_recovery.py::test_compacted_signed_session_stays_valid_for_vertextest_native_vertex_recovery.py::test_compacted_session_rows_keep_tool_pairstest_native_vertex_recovery.py::test_stream_drop_retried_without_duplicate_contenttest_native_codex_app_server.py::test_every_request_is_schema_valid_and_handshake_orderedversion'test_native_codex_app_server.py::test_command_approval_round_trip_and_tool_rowstest_native_codex_app_server.py::test_reasoning_projected_once_and_never_as_contenttest_native_codex_app_server.py::test_resume_in_new_process_resumes_the_same_threadtest_native_codex_app_server.py::test_resume_fallback_seeds_history_then_rebinds_new_threadtest_native_codex_app_server.py::test_native_compaction_keeps_thread_and_transcripttest_native_codex_app_server_faults.py::test_crash_mid_item_surfaced_once_without_partial_content_or_orphantest_native_codex_app_server_faults.py::test_terminal_turn_error_surfaced_once_not_retried[failed]if False:→ exit=0 ... (marker count != 1)test_native_codex_app_server_faults.py::test_terminal_turn_error_surfaced_once_not_retried[start_error]test_native_codex_app_server_faults.py::test_will_retry_error_notification_is_not_terminalerrornotification with willRetry=true, followed by success, does not end the turn: exit 0, final answer printed and persisted once, the notice is not shown, one turn/start.test_native_copilot_acp.py::test_hermes_tool_call_round_trips_through_acp_and_persiststest_native_copilot_acp.py::test_every_request_is_schema_valid_and_selects_the_configured_modeltest_native_copilot_acp.py::test_agent_permission_is_never_granted_and_fs_reads_stay_in_cwdtest_native_copilot_acp.py::test_resume_reaches_the_agent_with_persisted_history_in_order--resumeruns in a new agent process whose prompt carries the prior user, tool and assistant turns in order, then the new question. The answer is printed and persisted once in the same session. How the ACP session is opened (seeded session/new or session/load) is deliberately not asserted.test_native_copilot_acp.py::test_no_agent_process_outlives_the_clitest_native_copilot_acp.py::test_compaction_in_an_acp_session_keeps_the_next_prompt_valid_and_groundedtest_native_copilot_acp_errors.py::test_acp_failure_is_retried_per_semantics_and_surfaced_once[internal_error_retried|crash_mid_stream_retried|auth_required_not_retried|invalid_params_bounded|crash_every_time_bounded]test_native_copilot_acp_errors.py::test_auth_failure_remedy_is_an_actionable_command (strict xfail #121290)hermes ...fix command suggested in the auth-failure message must actually run, not report 'not implemented'.copilot login→ XPASS(strict) under the fix simulation; xfails for the KnownSymptom on maintest_native_copilot_acp.py::test_message_chunk_after_prompt_result_reaches_the_user (strict xfail #65788)test_native_copilot_acp_streaming.py::test_long_reasoning_turn_streams_progress_before_it_completes[tui_stream|nested_acp] (strict xfail #120550, #101507)hermes acprunning on top of copilot-acp.KNOWN bugs (strict xfail with
raises=KnownSymptom, per-fileKNOWNdict; KnownSymptom is raised only at the bug's symptom)parameterspath, $ref/$defs are dropped instead of inlined, so the $ref-typed MCP parameter is sent as an empty schema {}. v1beta parametersJsonSchema inlines it correctly. Covered by strict xfail test_v1_ref_parameter_keeps_its_shape.itemsis sent as-is and Google returns 400 'parameters.properties[tags].value.items: missing field.' Covered by strict xfail test_v1_array_without_items_is_accepted.hermes chat -q, a codex commandExecution approval waits on the interactive prompt for the full approvals.timeout (300 s by default) and then declines. It ignores approvals.single_query_mode, and the turn loop is blocked the whole time. xfail: reply arrived 8.0 s later with timeout=8.permissionsand has nodecisionfield, so the fake flags 'missing fieldpermissions'.chat -qexit path never calls agent.close()/_close_codex_session. The app-server dies only when its stdin hits EOF, so close()'s descendant reap never runs and children in their own session (like MCP servers) are orphaned. Related to closed #66671.hermes auth add copilot-acp --type oauth, which exits 'not implemented for auth type oauth yet'. The user is left with a fix command that doesn't work.hermes acprunning on top of copilot-acp, the outer client gets the inner chunks only after the inner turn has ended.Local runtime
scripts/run_tests.sh --include-integration tests/e2e/core/providers/: 14 files, 67 passed + 19 strict xfail, 0 failed. (The earlier "20" counted a codex teardown error that the unguarded xfail swallowed; see Review fixes.)Every file finishes in 30 s or less, well under the CI 180 s per-file budget.
ruff,check_no_tmp_literals,check-windows-footguns --allandgit diff --checkare all clean. No production code changed.Review fixes
Rebased on current
origin/main(no XPASS: all 19 KNOWN cells still reproduce).raises=let harness errors pass as the known bugraises=KnownSymptom, a sharedAssertionErrorsubclass in_native_helpers.py. It is raised only at the bug's symptom. Preconditions (exit codes, request counts, the approval exchange, the descendant pid) are plain asserts,next(...)/tuple unpacks became length-checked lists, andwait_until(..., error=KnownSymptom)covers the orphan timeout.--runxfail: all 19 cells fail withKnownSymptom. Sabotage: q_approval scenario issues no command (no approval request). On PR head it XFAILED (StopIteration swallowed); now it FAILS withapproval request/reply not exchanged once: [] []. The codex faults teardown error that PR head counted as a 5th xfail now shows as1e(seen mid-fix, before the MINOR 2 fix).os.killfrom module teardown, no guard bypass;sleep(600)leaked on dev boxesCodexRun.cleanup()drops that file and waits for it to exit, with a SIGKILL fallback. Both codex modules also carry@pytest.mark.live_system_guard_bypass(same convention astests/agent/test_shell_hooks_tree_kill.py). The dev-box guard re-arms before module-fixture finalizers run, so the marker alone did not help: the cooperative release is what fixes the leak.run_tests.sh:4✓ 5xfplus one leakedsleep(600)(ppid 1) per run. Now4✓ 4xf, 0 leaked across 5 full-suite runs.session/loadis never sent, which is not documented anywhere_format_messages_as_promptsabotage still turns it red (history-order assert).os.killafter the liveness check now toleratesProcessLookupError(race), so teardown can't error spuriously.Determinism after the fixes:
scripts/run_tests.sh --include-integration tests/e2e/core/providers/3× serial (46.6 s / 34.4 s / 37.4 s), plus 2 copies in parallel (72.0 s / 66.8 s). Every run: 67 passed, 19 xfailed, 0 failed, 0 errors, 0 leaked processes. The CItest_agent_turn_liveness[provider_hang]red is a pre-existing main flake owned by another lane and is not touched here.NOT COVERED (honest gaps)
provider: vertex#61852, Auxiliary client cannot use Vertex AI — the auth_type="vertex" ADC branch is unreachable (vertex missing from PROVIDER_REGISTRY) #66674): compaction's summary call to Vertex works here, but other aux tasks are disabledcodex_app_serverturn_timeout is a hardcoded 600s with no config surface #118486 turn timeout: hardcoded 600 s with no config key; a test would have to invent the keyInfographic