Skip to content

fix(cloud): review round 2 — bounded output, degraded-identity reporting, absolute binary, doc/field cleanup - #66

Merged
sourcehawk merged 6 commits into
feature/cloud-context-mcpfrom
feature/cloud-context-mcp--fix-round2
May 31, 2026
Merged

fix(cloud): review round 2 — bounded output, degraded-identity reporting, absolute binary, doc/field cleanup#66
sourcehawk merged 6 commits into
feature/cloud-context-mcpfrom
feature/cloud-context-mcp--fix-round2

Conversation

@sourcehawk

Copy link
Copy Markdown
Owner

Description

Towards #44

Addresses a round of verified Copilot review findings on the read-only cloud-context MCP. Six independent fixes: the harness no longer risks unbounded memory on a large CLI response, a degraded session now names which pinned identity is broken, the provider CLI is pinned to an absolute path so a later PATH change cannot redirect it, and the output-result doc and an unused field are corrected to match what the code actually does. One commit per finding.

Changes

  • Bound the harness output during the run. execCLI captured stdout/stderr through bytes.Buffer and truncated only after the process returned, so a very large response could buffer in full before being capped. It now captures through a limitedWriter that retains at most limit bytes each and flags overflow, so the cap is effective while the process runs. All existing guarantees hold: no shell, explicit minimal env, closed stdin, Truncated on overflow, stderr captured and capped, non-zero exit as a normal CLIResult, real start/exec failure as a Go error.
  • Report the pinned identity on a degraded probe. cloud.Probe fell back to an empty AssumedIdentity when the provider could not resolve one, so session_status could not say which pinned credential was degraded. It now falls back to the expected identity.
  • Keep the pinned identity when provider construction fails. ProbeSource returned a construction-error status (missing gcloud/aws) without the identity, so preflight and connections reported a degraded source with no identity to fix. It now carries src.AssumedIdentity through.
  • Resolve the provider CLI to an absolute path. exec.LookPath returns a relative path (flagged with exec.ErrDot) when PATH has relative entries. Both providers' New() now pass the result through filepath.Abs, so a later subprocess env/PATH change cannot reinterpret the binary.
  • Correct the CLIResult doc. It claimed output was shaped/redacted; run_cli returns the raw CLI stdout/stderr, only truncated. The doc now says so.
  • Drop the unused Command.Redact field. It was advertised in the allowlist schema and documented as marking output for secret-scrubbing, but nothing read it before returning run_cli output. No shipped default_commands.json sets it.

Testing

make test-go is green and race-clean across the whole module; make lint reports 0 issues. Each fix landed test-first: a shell-free large-output cap assertion for the harness, degraded- and valid-path identity-fallback assertions for cloud.Probe, a construction-failure identity assertion for ProbeSource, and an absolute-path assertion under a relative PATH for both providers. No frontend/ changes.

🤖 Generated with Claude Code

sourcehawk and others added 6 commits May 31, 2026 03:05
…bounded

execCLI buffered the full stdout/stderr in memory and truncated only after
the process returned, so a command emitting a very large response could
consume unbounded memory despite defaultOutputLimit. Capture stdout/stderr
through a bounded limitedWriter that retains at most limit bytes each and
records overflow, so the cap is effective during the run. Every existing
guarantee is preserved: no shell, explicit minimal env, closed stdin,
Truncated set on overflow, stderr captured and capped, non-zero exit as a
normal CLIResult, real start/exec failure as a Go error.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When the provider failed to resolve an identity, Probe returned Valid:false
with an empty AssumedIdentity even though the caller passed the pinned
identity in expected, so session_status no longer named which pinned
identity was degraded. Fall back to expected whenever the resulting status
has an empty AssumedIdentity, on both the degraded and valid paths, so the
displayed identity is always the pinned one. Degrade-never-error semantics
are unchanged.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A provider construction failure (e.g. a missing gcloud/aws binary) returned
IdentityStatus{Provider, Valid:false, Hint} with no AssumedIdentity, so
preflight and connections reported the degraded source without the identity
the operator must fix. Carry src.AssumedIdentity through the construction-error
status, mirroring the probe-path fallback so both ProbeSource exits name the
pinned identity.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The harness relies on a fixed absolute binary path so a later subprocess
env/PATH change cannot redirect what executes, but exec.LookPath returns a
relative path (flagged with exec.ErrDot) when PATH carries relative entries.
Pass the LookPath result through filepath.Abs in each provider's New(),
recovering the relative path on ErrDot and erroring if it still cannot be made
absolute. Applied identically to gcp and aws.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The comment claimed output was shaped/redacted, but run_cli returns the
provider CLI's raw stdout/stderr, only truncated. State that CLIResult carries
the raw CLI stdout (and stderr), capped at the output limit with Truncated set
when exceeded, so callers do not assume shaping or redaction beyond truncation.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Command.Redact was advertised in the allowlist schema and documented as
marking output for secret-scrubbing, but nothing read it before returning
run_cli output, so it promised protection that did not exist. No shipped
default_commands.json sets it. Remove the field and its doc; run_cli is the
gated escape hatch returning raw (truncated) output by design, and typed tools
are where projection lives.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@sourcehawk
sourcehawk merged commit 453a5ac into feature/cloud-context-mcp May 31, 2026
4 checks passed
@sourcehawk
sourcehawk deleted the feature/cloud-context-mcp--fix-round2 branch May 31, 2026 01:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant