Skip to content

Commit ddd82c0

Browse files
authored
fix(diagnostics): make the trace answer the question it exists for (#122)
Three defects, all found by reading a customer's capture of a failing ChatGPT connection rather than by reading the code. The panel recorded its own polling. targets_mcp() was a bare strpos() prefix test and the admin API shares the saddle/v1 namespace, so /saddle/v1/mcp-diagnostics matched — and the panel polls it every five seconds while recording. At 25 entries the ring buffer therefore turned over every ~125 seconds. The capture that arrived spanned 114 seconds and held two real rows among twenty-three of the panel watching itself. The instrument was evicting the evidence it exists to collect. Matched exactly now, and the buffer raised to 100 for margin. The credential scheme was recorded and rendered nowhere. Every row has carried Saddle_Connection::credential_scheme() since the recorder shipped; neither report() nor the table printed it. So the one field that distinguishes "the key was rejected" from "no key arrived" — a 401 either way, and opposite fixes — was written to the option and shown to no one, and a customer had to ask us what his own trace already knew. Now first on the row, ahead of session and protocol, which are trivia by comparison. Paired with an explicit auth present/absent/unknown, because '' and 'unknown' are different answers and neither is "present". And the HTTP method is recorded, so a row carrying no MCP method is no longer ambiguous between a GET, a POST with the wrong content type, and an empty body. The credential-leak test was passing for the wrong reason: it set an Authorization header on a synthetic WP_REST_Request, which populates no $_SERVER, so the recorder never saw a credential to leak. The helper now sets both, which is what made the two new scheme assertions meaningful — and what proves the existing assertion was load-bearing rather than vacuous. Closes #120
1 parent bdb7ff9 commit ddd82c0

6 files changed

Lines changed: 176 additions & 14 deletions

File tree

admin/build/index.asset.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
<?php return array('dependencies' => array('react', 'react-dom', 'wp-api-fetch', 'wp-element', 'wp-hooks', 'wp-i18n'), 'version' => '3a8e65d912ca9ca5e467');
1+
<?php return array('dependencies' => array('react', 'react-dom', 'wp-api-fetch', 'wp-element', 'wp-hooks', 'wp-i18n'), 'version' => '8404554d959f53432915');

admin/build/index.js

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

admin/src/components/McpDiagnostics.jsx

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -187,6 +187,7 @@ export default function McpDiagnostics() {
187187
<tr>
188188
<th>{ __( 'When', 'saddle' ) }</th>
189189
<th>{ __( 'Asked for', 'saddle' ) }</th>
190+
<th>{ __( 'Signed in with', 'saddle' ) }</th>
190191
<th>{ __( 'Result', 'saddle' ) }</th>
191192
<th>{ __( 'App', 'saddle' ) }</th>
192193
</tr>
@@ -202,7 +203,25 @@ export default function McpDiagnostics() {
202203
<td>
203204
{ ( entry.methods || [] ).join(
204205
', '
205-
) || '—' }
206+
) ||
207+
entry.method ||
208+
'—' }
209+
</td>
210+
<td>
211+
{ /* The column that answers "was it
212+
refused because the key was wrong,
213+
or because none arrived?" — which a
214+
401 alone cannot. */ }
215+
{ entry.auth === 'absent' ? (
216+
<Badge tone="warning">
217+
{ __(
218+
'nothing sent',
219+
'saddle'
220+
) }
221+
</Badge>
222+
) : (
223+
entry.scheme || '—'
224+
) }
206225
</td>
207226
<td>
208227
{ entry.status >= 200 &&

includes/class-saddle-mcp-diagnostics.php

Lines changed: 54 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -73,8 +73,15 @@ public static function note_compat_missing() {
7373

7474
/**
7575
* How many requests the ring buffer keeps.
76+
*
77+
* Was 25, which sounds ample and was not: paired with the panel recording
78+
* its own 5-second poll (see targets_mcp()), the buffer turned over every
79+
* ~125 seconds. A customer's capture of a failing connection spanned 114
80+
* seconds and held two real rows among twenty-three of the panel watching
81+
* itself. The poll is fixed; this is the margin, so a trace outlives the
82+
* round trip of someone reading it, copying it and sending it on.
7683
*/
77-
const MAX_ENTRIES = 25;
84+
const MAX_ENTRIES = 100;
7885

7986
/**
8087
* Default recording window, in minutes.
@@ -283,12 +290,38 @@ public static function snapshot( $response, $handler, $request ) {
283290
}
284291
}
285292

293+
// 'scheme' and 'auth' are the pair that answers "was this refused because
294+
// the credential was rejected, or because none arrived?" — the question
295+
// a 401 cannot answer on its own, and the one a host that strips the
296+
// Authorization header turns into a fortnight of guesswork. Both report
297+
// the SHAPE of the credential and never a byte of it; see
298+
// tests/mcp-diagnostics-test.php.
299+
//
300+
// 'method' matters because a row with no MCP method in it is ambiguous
301+
// between a GET, a POST with the wrong content type, and an empty body.
302+
// Those need different answers and used to look identical here.
303+
// credential_scheme() returns '' for "nothing arrived" and 'unknown' only
304+
// when Saddle_Connection is somehow absent. Those are different answers
305+
// and neither is "present" — collapsing them would put a confident word
306+
// on the row that decides whether the owner goes to their host.
307+
$scheme = class_exists( 'Saddle_Connection' ) ? Saddle_Connection::credential_scheme() : 'unknown';
308+
if ( '' === $scheme ) {
309+
$scheme = 'none';
310+
$auth = 'absent';
311+
} elseif ( 'unknown' === $scheme ) {
312+
$auth = 'unknown';
313+
} else {
314+
$auth = 'present';
315+
}
316+
286317
self::$pending = array(
287318
'time' => time(),
319+
'method' => $request->get_method(),
288320
'methods' => $methods,
289321
'session' => is_string( $session ) && '' !== $session ? 'sent' : 'absent',
290322
'protocol' => self::header_or_absent( $request, 'Mcp-Protocol-Version' ),
291-
'scheme' => class_exists( 'Saddle_Connection' ) ? Saddle_Connection::credential_scheme() : 'unknown',
323+
'scheme' => $scheme,
324+
'auth' => $auth,
292325
'user' => get_current_user_id(),
293326
'client' => self::client_name( $request, $body ),
294327
);
@@ -430,11 +463,18 @@ public static function report() {
430463
$lines[] = '';
431464
$lines[] = 'Recent requests (newest first):';
432465

466+
// auth + scheme come BEFORE session and protocol, because on a refused
467+
// row they are the answer and the other two are trivia. Their absence
468+
// from this line is the whole reason a customer had to ask us what his
469+
// own trace already knew.
433470
foreach ( array_reverse( $entries ) as $entry ) {
434471
$lines[] = sprintf(
435-
'%s %-28s session:%-7s protocol:%-11s status:%-4s%s%s %s',
472+
'%s %-6s %-28s auth:%-8s scheme:%-8s session:%-7s protocol:%-11s status:%-4s%s%s %s',
436473
gmdate( 'Y-m-d H:i:s', isset( $entry['time'] ) ? (int) $entry['time'] : 0 ),
474+
isset( $entry['method'] ) ? $entry['method'] : '?',
437475
implode( ',', isset( $entry['methods'] ) ? $entry['methods'] : array() ),
476+
isset( $entry['auth'] ) ? $entry['auth'] : '?',
477+
isset( $entry['scheme'] ) ? ( '' === $entry['scheme'] ? 'none' : $entry['scheme'] ) : '?',
438478
isset( $entry['session'] ) ? $entry['session'] : '?',
439479
isset( $entry['protocol'] ) ? $entry['protocol'] : '?',
440480
isset( $entry['status'] ) ? $entry['status'] : '?',
@@ -483,13 +523,22 @@ private static function transport_description() {
483523
/**
484524
* Whether the request is aimed at the MCP endpoint.
485525
*
526+
* The route, or something below it — never merely something that starts
527+
* with the same characters. This was a bare strpos() prefix test, and the
528+
* admin API shares the `saddle/v1` namespace, so `/saddle/v1/mcp-diagnostics`
529+
* matched: the panel below recorded its own 5-second poll as MCP traffic and
530+
* pushed the real rows out of the ring buffer within about two minutes. A
531+
* customer's capture of a failing connection came back twenty-three parts
532+
* panel to two parts evidence.
533+
*
486534
* @param WP_REST_Request $request The request.
487535
* @return bool
488536
*/
489537
private static function targets_mcp( $request ) {
490-
$route = '/' . Saddle_MCP::REST_NAMESPACE . Saddle_MCP::ROUTE;
538+
$mcp = '/' . Saddle_MCP::REST_NAMESPACE . Saddle_MCP::ROUTE;
539+
$route = (string) $request->get_route();
491540

492-
return 0 === strpos( (string) $request->get_route(), $route );
541+
return $route === $mcp || 0 === strpos( $route, $mcp . '/' );
493542
}
494543

495544
/**

languages/saddle.pot

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,14 +2,14 @@
22
# This file is distributed under the GPL-2.0-or-later.
33
msgid ""
44
msgstr ""
5-
"Project-Id-Version: Saddle – Control Your Site with AI (MCP Server) 1.0.0-rc5\n"
5+
"Project-Id-Version: Saddle – Control Your Site with AI (MCP Server) 1.0.0-rc6\n"
66
"Report-Msgid-Bugs-To: https://wordpress.org/support/plugin/saddle\n"
77
"Last-Translator: FULL NAME <EMAIL@ADDRESS>\n"
88
"Language-Team: LANGUAGE <LL@li.org>\n"
99
"MIME-Version: 1.0\n"
1010
"Content-Type: text/plain; charset=UTF-8\n"
1111
"Content-Transfer-Encoding: 8bit\n"
12-
"POT-Creation-Date: 2026-08-17T19:37:27+00:00\n"
12+
"POT-Creation-Date: 2026-08-18T03:18:22+00:00\n"
1313
"PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n"
1414
"X-Generator: WP-CLI 2.12.0\n"
1515
"X-Domain: saddle\n"
@@ -5331,25 +5331,33 @@ msgid "Asked for"
53315331
msgstr ""
53325332

53335333
#: admin/src/components/McpDiagnostics.jsx:190
5334-
msgid "Result"
5334+
msgid "Signed in with"
53355335
msgstr ""
53365336

53375337
#: admin/src/components/McpDiagnostics.jsx:191
5338+
msgid "Result"
5339+
msgstr ""
5340+
5341+
#: admin/src/components/McpDiagnostics.jsx:192
53385342
msgid "App"
53395343
msgstr ""
53405344

5345+
#: admin/src/components/McpDiagnostics.jsx:217
5346+
msgid "nothing sent"
5347+
msgstr ""
5348+
53415349
#. translators: %d: number of tools sent.
5342-
#: admin/src/components/McpDiagnostics.jsx:214
5350+
#: admin/src/components/McpDiagnostics.jsx:233
53435351
#, js-format
53445352
msgid "%d tools sent"
53455353
msgstr ""
53465354

5347-
#: admin/src/components/McpDiagnostics.jsx:220
5355+
#: admin/src/components/McpDiagnostics.jsx:239
53485356
msgid "OK"
53495357
msgstr ""
53505358

53515359
#. translators: %d: HTTP status code.
5352-
#: admin/src/components/McpDiagnostics.jsx:226
5360+
#: admin/src/components/McpDiagnostics.jsx:245
53535361
#, js-format
53545362
msgid "refused (%d)"
53555363
msgstr ""

tests/mcp-diagnostics-test.php

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,16 @@ private function rpc( $method, array $headers = array() ) {
6060

6161
foreach ( $headers as $name => $value ) {
6262
$request->set_header( $name, $value );
63+
64+
// And into $_SERVER, because that is where the credential actually
65+
// lives as far as every consumer is concerned. Saddle_Connection
66+
// reads $_SERVER on purpose: the failure this whole surface exists
67+
// to diagnose is a host stripping the header BEFORE PHP, and only
68+
// $_SERVER can tell you that. A WP_REST_Request built in a test is
69+
// synthetic and populates neither.
70+
if ( 0 === strcasecmp( $name, 'Authorization' ) ) {
71+
$_SERVER['HTTP_AUTHORIZATION'] = $value;
72+
}
6373
}
6474

6575
$request->set_body(
@@ -171,6 +181,82 @@ public function test_a_recorded_request_contains_no_credential() {
171181

172182
$this->assertStringNotContainsString( 'super-secret-token-value', $serialized );
173183
$this->assertStringNotContainsString( 'Authorization', $serialized );
184+
185+
// The row now names the SHAPE of the credential, which is the whole
186+
// point — and must still name nothing else. Asserted here rather than
187+
// in its own test so the two can never drift apart: whatever new field
188+
// describes a credential gets added above this line and is covered by
189+
// the assertions above it.
190+
$entry = Saddle_MCP_Diagnostics::entries()[0];
191+
$this->assertSame( 'bearer', $entry['scheme'] );
192+
$this->assertSame( 'present', $entry['auth'] );
193+
}
194+
195+
/**
196+
* The pair that answers a 401. "The key was rejected" and "no key arrived"
197+
* are the same HTTP status and opposite fixes — one is reconnect the app,
198+
* the other is talk to your host about a stripped Authorization header. A
199+
* customer lost two weeks inside that ambiguity.
200+
*/
201+
public function test_a_request_with_no_credential_is_recorded_as_such() {
202+
Saddle_MCP_Diagnostics::start_recording();
203+
204+
$this->rpc( 'tools/list' );
205+
206+
$entry = Saddle_MCP_Diagnostics::entries()[0];
207+
208+
$this->assertSame( 'absent', $entry['auth'], 'No Authorization header must read as absent, never as present.' );
209+
$this->assertSame( 'none', $entry['scheme'] );
210+
$this->assertSame( 'POST', $entry['method'], 'The HTTP method disambiguates a row that carried no MCP method.' );
211+
}
212+
213+
/**
214+
* Both facts have to survive into the text that actually gets pasted into a
215+
* support reply. They were recorded on every row for a month and rendered
216+
* nowhere, so the one question the trace could answer was the one question
217+
* we kept asking the customer to answer for us.
218+
*/
219+
public function test_the_report_names_the_credential_scheme() {
220+
Saddle_MCP_Diagnostics::start_recording();
221+
222+
$this->rpc( 'tools/list', array( 'Authorization' => 'Bearer super-secret-token-value' ) );
223+
224+
$report = Saddle_MCP_Diagnostics::report();
225+
226+
$this->assertStringContainsString( 'auth:present', $report );
227+
$this->assertStringContainsString( 'scheme:bearer', $report );
228+
$this->assertStringNotContainsString( 'super-secret-token-value', $report );
229+
}
230+
231+
/**
232+
* The panel polls this route every 5 seconds while recording, and the admin
233+
* API shares the `saddle/v1` namespace — so a prefix test matched it and the
234+
* instrument filled its own ring buffer with itself. At 25 entries that was
235+
* a ~125-second memory; a customer's capture of a failing connection came
236+
* back 23 parts panel to 2 parts evidence.
237+
*/
238+
public function test_the_diagnostics_route_is_not_recorded_as_mcp_traffic() {
239+
Saddle_MCP_Diagnostics::start_recording();
240+
241+
// Through rest_post_dispatch, not rest_do_request() alone. The recorder
242+
// closes an entry on that filter, and dispatch() does not fire it — so
243+
// the obvious version of this test passes against the BUG, because
244+
// nothing ever gets written either way. Verified red before the fix
245+
// only in this form.
246+
$request = new WP_REST_Request( 'GET', '/saddle/v1/mcp-diagnostics' );
247+
$response = rest_do_request( $request );
248+
apply_filters( 'rest_post_dispatch', $response, rest_get_server(), $request );
249+
250+
$this->assertSame(
251+
array(),
252+
Saddle_MCP_Diagnostics::entries(),
253+
'The panel must not record its own polling as MCP traffic.'
254+
);
255+
256+
// And the real endpoint still is recorded — a filter that records
257+
// nothing would also pass the assertion above.
258+
$this->rpc( 'tools/list' );
259+
$this->assertCount( 1, Saddle_MCP_Diagnostics::entries() );
174260
}
175261

176262
/**

0 commit comments

Comments
 (0)