Skip to content

Commit 3d947c9

Browse files
committed
fix(gate): fail—not skip—telemetry regression and proxy-side registry errors
Codex review round 1 (PR #819): - checkCountersMoved skipped the telemetry sub-check whenever the AFTER snapshot was 503, masking an endpoint that regressed under matrix traffic after a healthy baseline. Skip now only when telemetry was unavailable at the boot baseline; a healthy-then-503 transition FAILS the invariant. - test-api-e2e.sh treated any registry success:false as an external outage skip. The handler returns the same writeError envelope for its own bugs, so a proxy-side regression was silently skipped. Skip now requires transport evidence (curl rc, empty body, or upstream/transport error text).
1 parent f7acd42 commit 3d947c9

3 files changed

Lines changed: 92 additions & 17 deletions

File tree

‎cmd/release-gate/invariants.go‎

Lines changed: 27 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -203,9 +203,11 @@ func takeCounterSnapshot(ctx context.Context, c *Client) (*counterSnapshot, erro
203203
// - /api/v1/telemetry/payload: pinned to builtin_tool_calls only — pure
204204
// in-process counters that move with call_tool_*/retrieve_tools traffic
205205
// regardless of whether heartbeat SENDING is enabled. Network-dependent
206-
// fields are deliberately excluded. If the telemetry service itself is
207-
// unavailable (503) at both snapshots, that sub-check is recorded as
208-
// skipped with a reason (FR-004) rather than asserted.
206+
// fields are deliberately excluded. If the telemetry service is unavailable
207+
// (503) at the boot baseline, that sub-check is recorded as skipped with a
208+
// reason (FR-004 environment skip). If it is healthy at the baseline but
209+
// disappears under matrix traffic, that is a regression and the sub-check
210+
// FAILS rather than being masked as a skip.
209211
func checkCountersMoved(ctx context.Context, c *Client, before *counterSnapshot, timeout time.Duration) ([]gatereport.Step, *counterSnapshot, error) {
210212
if before == nil {
211213
return nil, nil, fmt.Errorf("no baseline counter snapshot recorded by the matrix run")
@@ -218,9 +220,15 @@ func checkCountersMoved(ctx context.Context, c *Client, before *counterSnapshot,
218220
if err != nil {
219221
return nil, nil, err
220222
}
223+
// The telemetry sub-check only skips when telemetry was unavailable at
224+
// the baseline (no reference point). If it was available at baseline we
225+
// must wait for it to actually move — a mid-run disappearance is a
226+
// regression, not a reason to stop polling early.
227+
telemetryMoved := !before.TelemetryAvailable ||
228+
(after.TelemetryAvailable && after.TelemetryBuiltin > before.TelemetryBuiltin)
221229
moved := after.TokensToolListSize > before.TokensToolListSize &&
222230
after.UsageCalls > before.UsageCalls &&
223-
(!before.TelemetryAvailable || !after.TelemetryAvailable || after.TelemetryBuiltin > before.TelemetryBuiltin)
231+
telemetryMoved
224232
if moved || time.Now().After(deadline) {
225233
break
226234
}
@@ -250,16 +258,25 @@ func checkCountersMoved(ctx context.Context, c *Client, before *counterSnapshot,
250258
fmt.Sprintf("usage call total %d -> %d", before.UsageCalls, after.UsageCalls))
251259

252260
switch {
253-
case before.TelemetryAvailable && after.TelemetryAvailable:
254-
addStep("telemetry-builtin-tool-calls-increase",
255-
after.TelemetryBuiltin > before.TelemetryBuiltin,
256-
fmt.Sprintf("builtin_tool_calls total %d -> %d", before.TelemetryBuiltin, after.TelemetryBuiltin))
257-
default:
261+
case !before.TelemetryAvailable:
262+
// Telemetry was already unavailable at the boot baseline: this
263+
// environment has telemetry disabled, so there is nothing to assert
264+
// (FR-004 environment skip).
258265
steps = append(steps, gatereport.Step{
259266
Name: "telemetry-builtin-tool-calls-increase",
260267
Status: gatereport.StatusSkipped,
261-
Reason: "telemetry payload unavailable (503) — telemetry service disabled in this environment; counter not asserted",
268+
Reason: "telemetry payload unavailable (503) at baseline — telemetry service disabled in this environment; counter not asserted",
262269
})
270+
case after.TelemetryAvailable:
271+
addStep("telemetry-builtin-tool-calls-increase",
272+
after.TelemetryBuiltin > before.TelemetryBuiltin,
273+
fmt.Sprintf("builtin_tool_calls total %d -> %d", before.TelemetryBuiltin, after.TelemetryBuiltin))
274+
default:
275+
// Telemetry was healthy at the baseline but the endpoint went
276+
// unavailable (503) under matrix traffic — a real regression, not an
277+
// environment skip. Fail rather than mask it.
278+
addStep("telemetry-builtin-tool-calls-increase", false,
279+
fmt.Sprintf("telemetry payload became unavailable after a healthy baseline (builtin_tool_calls baseline %d) — endpoint regressed under matrix traffic", before.TelemetryBuiltin))
263280
}
264281

265282
if len(failures) > 0 {

‎cmd/release-gate/invariants_test.go‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -246,6 +246,44 @@ func TestCheckCountersMoved_TelemetryUnavailable_SkippedNotFailed(t *testing.T)
246246
}
247247
}
248248

249+
func TestCheckCountersMoved_TelemetryRegressesAfterHealthyBaseline_Fails(t *testing.T) {
250+
// Telemetry is healthy at the boot baseline but goes 503 under matrix
251+
// traffic. That is a regression, not an environment skip — the sub-check
252+
// must FAIL rather than be masked as skipped.
253+
fake := &countersFake{toolList: 0, calls: 0, builtin: 3, telemetry: 0}
254+
c := fake.client(t)
255+
before, err := takeCounterSnapshot(context.Background(), c)
256+
if err != nil {
257+
t.Fatal(err)
258+
}
259+
if !before.TelemetryAvailable {
260+
t.Fatal("fake telemetry should be available at baseline")
261+
}
262+
fake.mu.Lock()
263+
fake.toolList, fake.calls = 100, 5
264+
fake.telemetry = http.StatusServiceUnavailable
265+
fake.mu.Unlock()
266+
267+
steps, _, err := checkCountersMoved(context.Background(), c, before, 0)
268+
if err == nil || !strings.Contains(err.Error(), "flat counters") {
269+
t.Fatalf("expected failure when telemetry disappears after a healthy baseline, got %v", err)
270+
}
271+
var telemetryFailed bool
272+
for _, s := range steps {
273+
if s.Name == "telemetry-builtin-tool-calls-increase" {
274+
if s.Status == gatereport.StatusSkipped {
275+
t.Errorf("telemetry regression must not be skipped: %+v", s)
276+
}
277+
if s.Status == gatereport.StatusFail {
278+
telemetryFailed = true
279+
}
280+
}
281+
}
282+
if !telemetryFailed {
283+
t.Errorf("telemetry sub-check must be recorded as failed, steps=%+v", steps)
284+
}
285+
}
286+
249287
// ============================================================================
250288
// FR-013 negative: a pre-approved (never-quarantined) server fails the
251289
// quarantine invariant.

‎scripts/test-api-e2e.sh‎

Lines changed: 27 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -786,19 +786,39 @@ else
786786
"jq -e '.success == true and .data.registries != null and .data.total > 0' < '$TEST_RESULTS_FILE' >/dev/null"
787787
fi
788788

789+
# Tests 26/27 proxy to the LIVE public "official" MCP registry. When that
790+
# third-party service is slow/down we log_skip instead of log_fail — a
791+
# release-blocking gate must not depend on a third party's uptime. But a
792+
# success:false is ONLY treated as an outage when there is transport-level
793+
# evidence: curl itself failed/timed out (rc != 0 or empty body), or the proxy's
794+
# error envelope names an upstream fetch/transport failure. A success:false whose
795+
# .error is anything else is a real proxy-side regression (same writeError path
796+
# the handler uses for its own bugs) and must hard-fail — otherwise the gate
797+
# would silently skip an API regression it exists to catch.
798+
registry_is_outage() {
799+
# $1 = curl rc, $2 = response body
800+
local rc="$1" body="$2"
801+
if [ "$rc" -ne 0 ] || [ -z "$body" ]; then
802+
return 0
803+
fi
804+
if echo "$body" | jq -e '.success == false' >/dev/null 2>&1; then
805+
local err
806+
err=$(echo "$body" | jq -r '.error // ""' 2>/dev/null)
807+
if echo "$err" | grep -qiE 'timeout|timed out|deadline exceeded|connection refused|no such host|no route to host|network is unreachable|i/o timeout|tls handshake|EOF|temporarily|unreachable|dial tcp|refused|reset by peer|502|503|504|bad gateway|gateway timeout|upstream|server misbehaving'; then
808+
return 0
809+
fi
810+
fi
811+
return 1
812+
}
813+
789814
# Test 26: Search registry servers (Phase 7)
790-
# NOTE: this proxies to the LIVE public "official" MCP registry. When that
791-
# third-party service is slow/down (curl times out → empty body, or mcpproxy
792-
# returns success:false because the upstream fetch failed) we log_skip instead of
793-
# log_fail — a release-blocking gate must not depend on a third party's uptime. A
794-
# reachable registry returning a malformed body still hard-fails below.
795815
log_test "GET /api/v1/registries/{id}/servers"
796816
RESPONSE=$(curl -s --max-time 10 $CURL_CA_OPTS -H "X-API-Key: $API_KEY" "${API_BASE}/registries/official/servers?limit=5")
797817
CURL_RC=$?
798818
echo "$RESPONSE" > "$TEST_RESULTS_FILE"
799819
if echo "$RESPONSE" | jq -e '.success == true and .data.servers != null and .data.registry_id == "official"' >/dev/null 2>&1; then
800820
log_pass "GET /api/v1/registries/{id}/servers - Response has servers array and registry_id"
801-
elif [ $CURL_RC -ne 0 ] || [ -z "$RESPONSE" ] || echo "$RESPONSE" | jq -e '.success == false' >/dev/null 2>&1; then
821+
elif registry_is_outage "$CURL_RC" "$RESPONSE"; then
802822
log_skip "GET /api/v1/registries/{id}/servers - external 'official' registry unreachable (curl rc=$CURL_RC); not a proxy regression"
803823
else
804824
log_fail "GET /api/v1/registries/{id}/servers - Expected server search results" \
@@ -812,7 +832,7 @@ CURL_RC=$?
812832
echo "$RESPONSE" > "$TEST_RESULTS_FILE"
813833
if echo "$RESPONSE" | jq -e '.success == true and .data.servers != null and .data.query == "github"' >/dev/null 2>&1; then
814834
log_pass "GET /api/v1/registries/{id}/servers?q=github - Response has query field"
815-
elif [ $CURL_RC -ne 0 ] || [ -z "$RESPONSE" ] || echo "$RESPONSE" | jq -e '.success == false' >/dev/null 2>&1; then
835+
elif registry_is_outage "$CURL_RC" "$RESPONSE"; then
816836
log_skip "GET /api/v1/registries/{id}/servers?q=github - external 'official' registry unreachable (curl rc=$CURL_RC); not a proxy regression"
817837
else
818838
log_fail "GET /api/v1/registries/{id}/servers?q=github - Expected query parameter in response" \

0 commit comments

Comments
 (0)