refactor(#266 A1): hoist mcp_call helper + auto-inject NETWORK_ID — test-infra hygiene (does NOT reduce fail count)#277
Merged
Conversation
…est-infra hygiene Following #266 round-1 audit, server commit 5c7bff2 had tightened the UTOK write path to require an explicit network_id on every send_task / send_reply / cancel_task / report_status. tests/docker-e2e.sh predates that tightening: 12 raw `curl POST /mcp` invocations each hardcoded their own JSON payload (bypassing the existing mcp_call helper) and none of them carried network_id. This refactor unifies the test against one entry point: - mcp_call() hoisted from line ~252 to line ~64, immediately after the new NETWORK_ID bootstrap. Now reachable from every tool-call site that follows. - Bootstrap section added: after the user registers, create an e2e-network and capture its network_id (fall back to /api/auth/me networks[0] if creation already exists from prior run). - mcp_call() now injects NETWORK_ID into ARGS if the caller hasn't supplied one (via jq, conditional on `has("network_id")`). Callers that want a different network can still override explicitly. - The 12 raw curls become `mcp_call "TOOL" '{...}'` invocations. Net: 51 ins / 57 del / 36-line reduction. Honest scope: this is test-infra hygiene and a prerequisite for any future server contract tightening (one change point: the helper). It does NOT by itself reduce the Base E2E fail count — local verification confirms: before refactor (after #273): 90 pass / 45 fail after refactor: 90 pass / 45 fail What changed under the hood: the immediate `permission_denied: network_id required` error site is gone (verified by per-tool curl probe), but the cascade has more layers than the round-1 audit hypothesized. Investigating one failing test revealed at least two more roots, both deferred to follow-up: Layer 2 — e2e-agent registration: test 8 launches `agent-node --alias e2e-agent --runtime codex-sdk` in the background without network/token wiring. The agent never registers with CommHub → all downstream alias-targeted tools (send_task, send_ack, send_reply) return alias_not_found. Layer 3 — assertion looseness: many tests grep server output for the literal "ok" (e.g. `echo "$RESP" | grep -q "ok"`). Both `{"ok":true}` and `{"ok":false,"error":"..."}` contain "ok", so those assertions produce false-positive PASS regardless of the actual server verdict. This means some of the 90 baseline passes are not real passes; the count under-states how broken things actually are. The original "Bucket A = 1 root, 25 cascades (~#268 pattern)" framing in the round-1 audit is therefore only partially correct: contracts tightening is one root, but the test set has been quietly accumulating multiple independent rots that the cascade was hiding. Refs: #266 round-1 audit (which this revises), #265 + #269 + #273 + #270
![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: 73548bb34a
ℹ️ 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".
Comment on lines
+52
to
+55
| NET_RESP=$(curl -s -X POST http://127.0.0.1:9200/api/networks \ | ||
| -H "Authorization: Bearer $TOKEN" -H "Content-Type: application/json" \ | ||
| -d '{"name":"e2e-network","description":"e2e bootstrap network"}') | ||
| NETWORK_ID=$(echo "$NET_RESP" | python3 -c "import sys,json;print(json.loads(sys.stdin.read()).get('network_id',''))" 2>/dev/null) |
There was a problem hiding this comment.
On a fresh run this creates a second network and stores that id in NETWORK_ID, but the non-interactive anet login below still persists the first network returned by /api/networks (the auto-created default) into ~/.anet/config.json; agent-node then reports status in that default network while every mcp_call is scoped to e2e-network. Once the registration test is fixed, these sends will still return alias_not_found because the target session is in a different network. Use the register/login network id, or switch the CLI/agent config to this newly created network before starting agents.
Useful? React with 👍 / 👎.
| # #266 A1: inject NETWORK_ID into ARGS unless caller already supplied one. | ||
| # Required since server 5c7bff2 — every utok write needs explicit network_id. | ||
| if [ -n "${NETWORK_ID:-}" ]; then | ||
| ARGS=$(echo "$ARGS" | jq -c --arg n "$NETWORK_ID" 'if has("network_id") then . else . + {network_id: $n} end' 2>/dev/null || echo "$ARGS") |
There was a problem hiding this comment.
This injection only helps tools whose handlers actually consume network_id; several converted/later write calls such as send_ack, send_reply, send_message, ack_inbox, cancel_task, retry_task, and reassign_task have schemas/handlers in server/src/tools.ts that omit it and then call getNetworkId(null), so the injected key is ignored and a utok caller still gets permission_denied once those tests have a real task id. Add network_id support to those tool schemas/handlers or use a network-bound token instead of relying on this generic injection.
Useful? React with 👍 / 👎.
5 tasks
s2agi
pushed a commit
that referenced
this pull request
Jun 28, 2026
…dict Pair with the reset_server fix in this PR. Bucket A (~45 Base E2E fails) is pre-existing test-infra cascade tracked separately (#279) for v0.12 quality work. Hard-gating the merge bus on a 12-day-old red was blocking shipping without buying any product safety net — `anet QA (v0)` is the real merge gate and has been green since #269. This commit adopts the same report-only SOP as `anet QA (v0)` and the release-gate workflow: 1. The regression step captures the docker exit code via `set +e` + `$GITHUB_OUTPUT` instead of failing the job on exit 1. 2. A new verdict step (always() runs) prints the test-all.sh Final Report block + TOTAL line + the captured exit code. Per-suite numbers are visible in the Actions UI without expanding the run-step log. 3. Two failure-mode branches: - test-all.sh emitted no output → real infra/build issue, FAIL the job (don't silently green a workflow that didn't run). - test-all.sh ran + exited non-zero → emit `::warning::` lines with the Bucket A / #279 / #266 reference + an explicit "green tick means harness ran, NOT all 186 passed" disclaimer, then exit 0. The no-silent-cap rule applies: this workflow never says "OK" without the TOTAL line being visible in the same view. The Actions check will show green (job succeeded), but every viewer sees the same TOTAL the maintainer sees, with `::warning::` annotations highlighting any fails. After this merges: - main CI returns green on this workflow for the first time since the Bucket A cascade started (12+ days). - The "no e2e safety net" gap noted by 通信龙 task b4c67c72 is closed in the sense that the workflow now runs to completion + surfaces the verdict every time, instead of failing opaquely on a known-stable red. - Bucket A real-fix work continues on the v0.12 quality timeline per the tracking issue (linked in PR body) — this PR does not foreclose that work, only stops it from blocking the merge bus. Refs: #266 (audit + this PR's report doc), #279 (Layer 2/3 backlog), #273 (original reset_server), #277 (A1 helper refactor).
s2agi
pushed a commit
that referenced
this pull request
Jun 28, 2026
…l/dev-tcp (#291) * fix(#266 PR-2): CI-robust reset_server — explicit PID kill, drop pkill/dev-tcp #273's reset_server fix works locally (V3 Networks 19/3) but silently regresses in CI (V3 Networks "0 ran"). Confirmed by reproducing the exact main commit's image locally vs CI run 28308477515 on 912d642 — the only delta between 134/49 local and 115/47 CI is V3 Networks's suite-crashed report. Root cause: the original reset_server used `pkill -f 'bun.*src/index.ts'` + `(exec 3<>/dev/tcp/127.0.0.1/9200)` bash pseudo-file. Both depend on process/bash behavior that differs in GHA's runner Docker environment vs a workstation Docker Engine. Either failure mode leaves the previous bun bound to :9200 when V3 Networks fires its own `bun run src/index.ts`, which then gets EADDRINUSE in the background; the suite proceeds against the polluted shared server and exits early without reaching its "Results: …" line. Rewrite: capture SERVER_PID on start, `kill -KILL "$SERVER_PID"` + `wait` on the next reset, 0.5s settle, then start the new server in a subshell so `cd /app/server` doesn't leak into test-all.sh's cwd. No pkill/grep, no /dev/tcp, no env-dependent behavior. Local verification on the patched image: Base E2E (137): ❌ 90 / 45 (unchanged — same Bucket A cascade) V3 Auth (25): ✅ 25 / 0 (unchanged) V3 Networks (22): ❌ 20 / 2 (was 19/3 — slightly better isolation) Config Priority:⚠️ 0 ran (Bucket C, unrelated test-design) TOTAL: 135 / 48 PR-2 does NOT make the workflow exit 0. Base E2E 45 fails are pre-existing #266 Bucket A (test-infra cascade per #279, not product bugs). The job will still exit 1 until either Bucket A clears or the workflow converts to report-only — see docs/tests/report-266-... for the full divergence analysis + Bucket A handling plan. Refs: #266 (audit), #273 (original reset_server), #279 (Bucket A follow-up), #277 (A1 helper refactor) * ci(#266 endgame): convert e2e-docker.yml to report-only + visible verdict Pair with the reset_server fix in this PR. Bucket A (~45 Base E2E fails) is pre-existing test-infra cascade tracked separately (#279) for v0.12 quality work. Hard-gating the merge bus on a 12-day-old red was blocking shipping without buying any product safety net — `anet QA (v0)` is the real merge gate and has been green since #269. This commit adopts the same report-only SOP as `anet QA (v0)` and the release-gate workflow: 1. The regression step captures the docker exit code via `set +e` + `$GITHUB_OUTPUT` instead of failing the job on exit 1. 2. A new verdict step (always() runs) prints the test-all.sh Final Report block + TOTAL line + the captured exit code. Per-suite numbers are visible in the Actions UI without expanding the run-step log. 3. Two failure-mode branches: - test-all.sh emitted no output → real infra/build issue, FAIL the job (don't silently green a workflow that didn't run). - test-all.sh ran + exited non-zero → emit `::warning::` lines with the Bucket A / #279 / #266 reference + an explicit "green tick means harness ran, NOT all 186 passed" disclaimer, then exit 0. The no-silent-cap rule applies: this workflow never says "OK" without the TOTAL line being visible in the same view. The Actions check will show green (job succeeded), but every viewer sees the same TOTAL the maintainer sees, with `::warning::` annotations highlighting any fails. After this merges: - main CI returns green on this workflow for the first time since the Bucket A cascade started (12+ days). - The "no e2e safety net" gap noted by 通信龙 task b4c67c72 is closed in the sense that the workflow now runs to completion + surfaces the verdict every time, instead of failing opaquely on a known-stable red. - Bucket A real-fix work continues on the v0.12 quality timeline per the tracking issue (linked in PR body) — this PR does not foreclose that work, only stops it from blocking the merge bus. Refs: #266 (audit + this PR's report doc), #279 (Layer 2/3 backlog), #273 (original reset_server), #277 (A1 helper refactor). --------- Co-authored-by: vansin <smartflowaiteam@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Author
Agent: 通信测试马 (claude-code-cli)
Dispatch: 通信龙 task 63985e29 → push refactor as-is + honest scope, open follow-ups for layers 2/3
Refs: #266 round-1 audit, #273 (Bucket B), #265 + #269 (related #268 pattern)
Honest summary
This is test-infra hygiene only — it does NOT by itself reduce the Base E2E fail count. The numbers go in at 90/45 and come out at 90/45.
What it does:
mcp_call()from line ~252 to right after the newNETWORK_IDbootstrap (~line 64), so every tool-call site can reach it.e2e-networkand capture itsnetwork_id(fallback to/api/auth/me).mcp_call()injectsNETWORK_IDinto ARGS unless caller already passed one.curl POST /mcpinvocations that each hardcoded their own JSON withmcp_call "TOOL" '{...}'calls.Why it's worth merging anyway:
permission_denied: network_id requirederror site IS gone (verified by per-tool curl probe — server happily ACKs withnetwork_idechoed back).Why count doesn't drop
Investigating one specific failing test (test 11 —
task_id not captured) revealed at least two more roots that round-1 audit didn't surface. Both deferred to follow-up.Layer 2 —
e2e-agentnever registersTest 8 (
agent-node --alias e2e-agent --runtime codex-sdkin the background, sleep 5, check/api/status) doesn't wire network/token, soagent-nodenever registers with CommHub → all downstream alias-targeted tools (send_task,send_ack,send_replytoe2e-agent) returnalias_not_found. Earlier in the round-1 audit this was misattributed as "task not delivered / task not in terminal state / etc." cascade — it's really the upstream registration.Layer 3 — assertion looseness gives false-positive passes
Many tests grep server output for the literal
"ok":Both
{"ok":true,"message_id":"..."}and{"ok":false,"error":"..."}contain"ok", so the assertion PASSES regardless of the actual server verdict. Some of the 90 baseline passes are not real passes; the actual broken-ness is worse than the count suggests.Round-1 audit framing revision
The original "Bucket A = 1 root, 25 cascades (#268 pattern)" framing turns out to be only partially correct. There's at least one contract-tightening root (network_id), but the test set has been quietly accumulating multiple independent rots that the cascade was hiding. A proper round-2 audit would need to re-bucket given this finding.
Per 通信龙's call, #266 is de-prioritized —
anet QA (v0)is green andDocker E2E Testsred is test-infra not product. Not blocking v0.11.Verification
Per-tool probe with the new helper (clean server, fresh DB):
Test plan
test-all.shrun → 134/49 total (unchanged from fix(#266 Bucket B): reset server+DB between e2e suites — recover V3 Networks visibility (+19 tests) #273 baseline; numbers preserved across refactor)Follow-up (separate issue)
Layers 2 and 3 require their own work and are NOT in scope here. Opening a follow-up issue to track:
agent-nodestartup needs network/token wiring in test 8grep -q "ok"assertions to check{"ok":true}shape (or use jq)This PR is the foundation for any of those — once mcp_call is the single entry point, layer 2/3 fixes can land incrementally without re-touching 12 raw curl sites.