Skip to content

Commit 5a1863b

Browse files
refactor: trim excessive comments from lockdown upper-bound PR
Condense multi-line narration into short, focused comments. Keep only the non-obvious invariants (lockdown precedence, isolated transport rationale) and drop restated code/step-by-step prose. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
1 parent 53fb8c8 commit 5a1863b

4 files changed

Lines changed: 18 additions & 38 deletions

File tree

pkg/github/dependencies.go

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -439,12 +439,10 @@ func (d *RequestDeps) GetRawClient(ctx context.Context) (*raw.Client, error) {
439439
return rawClient, nil
440440
}
441441

442-
// effectiveLockdownMode reports whether lockdown mode is active for the current
443-
// request. Server-side configuration (d.lockdownMode) is an upper bound: once the
444-
// operator has enabled lockdown mode, no request can disable it. Request-scoped
445-
// configuration (the X-MCP-Lockdown header, surfaced via ghcontext.IsLockdownMode)
446-
// may only tighten restrictions by enabling lockdown when the operator has not
447-
// already done so; it can never be used to relax server-enforced lockdown.
442+
// effectiveLockdownMode reports whether lockdown mode is active for the
443+
// request. d.lockdownMode is an operator-set upper bound: the per-request
444+
// X-MCP-Lockdown header (ghcontext.IsLockdownMode) can only enable lockdown,
445+
// never disable one the operator already turned on.
448446
func (d *RequestDeps) effectiveLockdownMode(ctx context.Context) bool {
449447
return d.lockdownMode || ghcontext.IsLockdownMode(ctx)
450448
}

pkg/github/dependencies_test.go

Lines changed: 7 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -198,13 +198,8 @@ func TestIsFeatureEnabled_EmptyFlagName(t *testing.T) {
198198
assert.False(t, result, "Expected false for empty flag name")
199199
}
200200

201-
// TestRequestDepsLockdownModeIsUpperBound verifies that, in HTTP mode, the
202-
// server operator's --lockdown-mode / GITHUB_LOCKDOWN_MODE configuration is an
203-
// upper bound: request-scoped configuration (the X-MCP-Lockdown header,
204-
// surfaced as ghcontext.WithLockdownMode) may enable or tighten lockdown, but
205-
// can never disable lockdown the operator has already turned on. Lockdown mode
206-
// remains a best-effort content filter, not a security boundary — this test
207-
// only asserts the on/off decision, not any content-filtering guarantee.
201+
// TestRequestDepsLockdownModeIsUpperBound verifies the X-MCP-Lockdown header
202+
// can only enable lockdown, never disable the operator's server-side setting.
208203
func TestRequestDepsLockdownModeIsUpperBound(t *testing.T) {
209204
t.Parallel()
210205

@@ -284,30 +279,25 @@ func TestRequestDepsLockdownModeIsUpperBound(t *testing.T) {
284279
}
285280
}
286281

287-
// TestRequestDepsLockdownModeCannotBeDisabledByOmittingHeader is a focused
288-
// regression test for the specific bug in #3104: previously, server-enabled
289-
// lockdown mode was disabled for any request that did not also send the
290-
// X-MCP-Lockdown header, letting a request silently opt out of an operator's
291-
// security posture. A request simply omitting the header (as opposed to
292-
// explicitly disabling it, which the header format does not support) must not
293-
// relax lockdown mode below what the operator configured.
282+
// TestRequestDepsLockdownModeCannotBeDisabledByOmittingHeader is a regression
283+
// test for #3104: omitting the X-MCP-Lockdown header must not disable
284+
// server-enabled lockdown mode.
294285
func TestRequestDepsLockdownModeCannotBeDisabledByOmittingHeader(t *testing.T) {
295286
t.Parallel()
296287

297288
resolver := newRequestDepsAPIHostResolver(t, "https://example.com")
298289
deps := github.NewRequestDeps(
299290
resolver,
300291
"test",
301-
true, // server operator enabled lockdown mode
292+
true, // server-enabled lockdown
302293
nil,
303294
translations.NullTranslationHelper,
304295
0,
305296
nil,
306297
testExporters(),
307298
)
308299

309-
// No ghcontext.WithLockdownMode call: this is what happens when a request
310-
// does not send the X-MCP-Lockdown header at all.
300+
// No X-MCP-Lockdown header sent.
311301
ctx := ghcontext.WithTokenInfo(context.Background(), &ghcontext.TokenInfo{Token: "request-token"})
312302

313303
flags := deps.GetFlags(ctx)

pkg/http/transport/graphql_features_test.go

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -91,11 +91,9 @@ func TestGraphQLFeaturesTransport(t *testing.T) {
9191
}
9292
}
9393

94-
// TestGraphQLFeaturesTransport_NilTransport asserts the documented fallback to
95-
// http.DefaultTransport, so it must exercise that global rather than an
96-
// isolated transport. It therefore runs serially: httptest.Server.Close closes
97-
// http.DefaultTransport's idle connections, so a parallel test shutting down
98-
// its own server would otherwise be able to break this request.
94+
// TestGraphQLFeaturesTransport_NilTransport exercises the real
95+
// http.DefaultTransport fallback, so it can't run in parallel with tests that
96+
// close their own servers (that closes DefaultTransport's idle conns too).
9997
func TestGraphQLFeaturesTransport_NilTransport(t *testing.T) {
10098
var capturedHeader string
10199

pkg/http/transport/helpers_test.go

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -7,16 +7,10 @@ import (
77

88
// newIsolatedTransport returns an http.Transport owned by a single test.
99
//
10-
// httptest.Server.Close closes the idle connections of the process-global
11-
// http.DefaultTransport, regardless of which server is being shut down. Tests
12-
// here run in parallel and each shut down a server, so sharing
13-
// http.DefaultTransport lets one test's cleanup break another test's request
14-
// with "http: CloseIdleConnections called". Giving every test its own
15-
// transport keeps that global side effect out of reach.
16-
//
17-
// Use this wherever a test just needs a working transport. Tests that assert
18-
// behaviour specific to http.DefaultTransport must use it directly and must
19-
// not run in parallel.
10+
// Sharing http.DefaultTransport across parallel tests is unsafe: closing one
11+
// test's httptest.Server also closes DefaultTransport's idle connections,
12+
// breaking other tests still using it. Tests asserting DefaultTransport
13+
// fallback behavior specifically must use it directly and not run in parallel.
2014
func newIsolatedTransport(t *testing.T) *http.Transport {
2115
t.Helper()
2216

0 commit comments

Comments
 (0)