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

pkg/mcp/cloud/allowlist.go

@@ -19,11 +19,10 @@ var defaultCommandsJSON []byte
 // Command is one entry in the command allowlist. Path is the normalized
 // subcommand path the allowlist matches against (for example "projects list" or
 // "compute firewall-rules list"). Description carries the investigative axis the
-// command serves (prose only). Redact marks output that needs secret-scrubbing.
+// command serves (prose only).
 type Command struct {
 	Path        string `json:"path"`
 	Description string `json:"description,omitempty"`
-	Redact      bool   `json:"redact,omitempty"`
 }
 
 // CommandAllowlist is the decoded allowlist document: the positive set of

pkg/mcp/cloud/harness.go

@@ -1,7 +1,6 @@
 package cloud
 
 import (
-	"bytes"
 	"context"
 	"errors"
 	"os/exec"
@@ -11,40 +10,63 @@ import (
 // the agent's context budget. Output beyond it is dropped and flagged.
 const defaultOutputLimit = 64 * 1024
 
+// limitedWriter retains at most limit bytes of everything written to it and
+// records whether any write pushed it past that cap. It never grows past limit,
+// so a command emitting an arbitrarily large response cannot consume unbounded
+// memory: bytes past the cap are counted for the overflow flag and discarded.
+type limitedWriter struct {
+	buf      []byte
+	limit    int
+	overflow bool
+}
+
+// Write retains up to the remaining capacity in the buffer and discards the
+// rest, flagging overflow whenever a write carries more bytes than the buffer
+// can still hold. It always reports the full length written so the child
+// process is never blocked on a short write.
+func (w *limitedWriter) Write(p []byte) (int, error) {
+	room := w.limit - len(w.buf)
+	if len(p) > room {
+		w.overflow = true
+	}
+	if room > 0 {
+		take := len(p)
+		if take > room {
+			take = room
+		}
+		w.buf = append(w.buf, p[:take]...)
+	}
+	return len(p), nil
+}
+
 // execCLI runs binPath with argv via execve — no shell, ever. The argv tokens
 // reach the binary as literal arguments, so shell metacharacters are inert. The
 // subprocess runs with exactly the supplied env (never the parent environment,
 // so a poisoned PATH cannot redirect the binary and ambient secrets do not
-// leak), closed stdin (no interactive prompt), and stdout capped at limit. A
-// non-zero exit is a normal result carried in ExitCode, not a Go error; a Go
-// error means the process could not be run at all. Stderr — where gcloud/aws
-// write their error context — is captured alongside stdout and capped at the
-// same limit, so a non-zero exit carries an explanation instead of an empty
-// result.
+// leak), closed stdin (no interactive prompt), and stdout/stderr captured
+// through bounded writers that retain at most limit bytes each — the cap is
+// effective during the run, so a command emitting a very large response can
+// never buffer it all in memory. A non-zero exit is a normal result carried in
+// ExitCode, not a Go error; a Go error means the process could not be run at
+// all. Stderr — where gcloud/aws write their error context — is captured
+// alongside stdout and capped at the same limit, so a non-zero exit carries an
+// explanation instead of an empty result.
 func execCLI(ctx context.Context, binPath string, argv []string, env []string, limit int) (CLIResult, error) {
 	cmd := exec.CommandContext(ctx, binPath, argv...)
 	cmd.Env = env
 	cmd.Stdin = nil
 
-	var stdout, stderr bytes.Buffer
-	cmd.Stdout = &stdout
-	cmd.Stderr = &stderr
+	stdout := &limitedWriter{limit: limit}
+	stderr := &limitedWriter{limit: limit}
+	cmd.Stdout = stdout
+	cmd.Stderr = stderr
 	err := cmd.Run()
 
-	res := CLIResult{}
-	out := stdout.Bytes()
-	if len(out) > limit {
-		out = out[:limit]
-		res.Truncated = true
-	}
-	res.Stdout = string(out)
-
-	errOut := stderr.Bytes()
-	if len(errOut) > limit {
-		errOut = errOut[:limit]
-		res.Truncated = true
+	res := CLIResult{
+		Stdout:    string(stdout.buf),
+		Stderr:    string(stderr.buf),
+		Truncated: stdout.overflow || stderr.overflow,
 	}
-	res.Stderr = string(errOut)
 
 	if err != nil {
 		var exitErr *exec.ExitError

pkg/mcp/cloud/harness_test.go

@@ -41,3 +41,19 @@ func TestExecCLITruncatesStderr(t *testing.T) {
 	require.NoError(t, err)
 	assert.LessOrEqual(t, len(r.Stderr), 10, "stderr exceeded limit")
 }
+
+// TestExecCLICapsLargeOutputWithoutBuffering drives a payload orders of
+// magnitude past the limit through a shell-free command (head reading 8MB from
+// /dev/zero) and asserts the captured stdout is capped at the limit with
+// Truncated set, so a command emitting a very large response cannot retain
+// unbounded bytes in memory. The cap is effective during the run, not a
+// post-hoc slice of a fully buffered output.
+func TestExecCLICapsLargeOutputWithoutBuffering(t *testing.T) {
+	t.Parallel()
+	const limit = 1024
+	r, err := execCLI(context.Background(), "/usr/bin/head",
+		[]string{"-c", "8388608", "/dev/zero"}, nil, limit)
+	require.NoError(t, err)
+	assert.True(t, r.Truncated, "an output far larger than limit must be flagged truncated")
+	assert.LessOrEqual(t, len(r.Stdout), limit, "captured stdout must be capped at limit, not the full 8MB payload")
+}

pkg/mcp/cloud/probe.go

@@ -30,18 +30,22 @@ func Probe(ctx context.Context, p Provider, expected string, env []string) (Iden
 	st, err := p.Identity(ctx, run, expected)
 	if err != nil {
 		return IdentityStatus{
-			Provider: p.Name(),
-			Valid:    false,
-			Hint:     err.Error(),
+			Provider:        p.Name(),
+			AssumedIdentity: expected,
+			Valid:           false,
+			Hint:            err.Error(),
 		}, nil
 	}
 	if st.Provider == "" {
 		st.Provider = p.Name()
 	}
 	if st.AssumedIdentity == "" {
 		// A whoami that resolved no identity is not a valid session, whatever
-		// the provider reported.
+		// the provider reported. Report the pinned identity so the degraded
+		// session names which credential the operator must fix instead of an
+		// empty one.
 		st.Valid = false
+		st.AssumedIdentity = expected
 	}
 	return st, nil
 }

pkg/mcp/cloud/probe_test.go

@@ -111,3 +111,31 @@ func TestProbeInvalidWhenIdentityEmpty(t *testing.T) {
 	require.NoError(t, err)
 	assert.False(t, st.Valid, "an empty resolved identity must not be reported valid")
 }
+
+// TestProbeDegradedReportsPinnedIdentity proves a degraded probe still names
+// WHICH pinned identity is degraded: when the provider errors and resolves no
+// identity, Probe falls back to the expected identity the caller pinned, so
+// session_status stays actionable instead of showing an empty identity.
+func TestProbeDegradedReportsPinnedIdentity(t *testing.T) {
+	t.Parallel()
+	const pinned = "ro-sa@proj.iam.gserviceaccount.com"
+	p := &fakeProvider{name: "gcp", identityErr: errors.New("token expired")}
+	st, err := Probe(context.Background(), p, pinned, nil)
+	require.NoError(t, err, "Probe should degrade, not error")
+	assert.False(t, st.Valid)
+	assert.Equal(t, pinned, st.AssumedIdentity,
+		"a degraded probe must report the pinned identity so the operator knows what to fix")
+}
+
+// TestProbeFallsBackToExpectedWhenProviderOmitsIdentity covers the valid path:
+// a provider that resolves to valid but reports no identity (an unusual but
+// possible projection gap) still shows the pinned identity rather than empty.
+func TestProbeFallsBackToExpectedWhenProviderOmitsIdentity(t *testing.T) {
+	t.Parallel()
+	const pinned = "arn:aws:iam::111122223333:role/triage-ro"
+	p := &fakeProvider{name: "aws", identity: IdentityStatus{Provider: "aws", Valid: true}}
+	st, err := Probe(context.Background(), p, pinned, nil)
+	require.NoError(t, err)
+	assert.Equal(t, pinned, st.AssumedIdentity,
+		"an empty resolved identity must fall back to the pinned identity")
+}

pkg/mcp/cloud/provider.go

@@ -69,8 +69,10 @@ type IdentityStatus struct {
 	Hint            string `json:"hint,omitempty"`
 }
 
-// CLIResult is the shaped result of one run_cli invocation. Raw provider JSON
-// is never surfaced; the harness caps output and reports truncation.
+// CLIResult is the result of one run_cli invocation. It carries the provider
+// CLI's raw stdout (and stderr), each capped at the output limit with Truncated
+// set when the output exceeded it. The bytes are not otherwise shaped or
+// redacted; callers must not assume any projection beyond truncation.
 type CLIResult struct {
 	Stdout    string `json:"stdout"`
 	Stderr    string `json:"stderr,omitempty"`

pkg/mcp/cloud/providers/aws/provider.go

@@ -13,8 +13,10 @@ package aws
 import (
 	_ "embed"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"os/exec"
+	"path/filepath"
 
 	"github.com/sourcehawk/triagent/pkg/mcp/cloud"
 )
@@ -50,13 +52,20 @@ type Provider struct {
 }
 
 // New constructs the AWS provider, resolving aws to an absolute path once via
-// exec.LookPath so a poisoned PATH cannot redirect the binary at run time.
+// exec.LookPath so a poisoned PATH cannot redirect the binary at run time. A
+// PATH with relative entries makes LookPath return a relative path (flagged with
+// exec.ErrDot); the path is made absolute so a later subprocess env/PATH change
+// cannot reinterpret it against a different working directory.
 func New() (*Provider, error) {
 	bin, err := exec.LookPath("aws")
-	if err != nil {
+	if err != nil && !errors.Is(err, exec.ErrDot) {
 		return nil, fmt.Errorf("aws: resolve aws binary: %w", err)
 	}
-	return newWithBinary(bin)
+	abs, err := filepath.Abs(bin)
+	if err != nil {
+		return nil, fmt.Errorf("aws: resolve aws binary to absolute path: %w", err)
+	}
+	return newWithBinary(abs)
 }
 
 // newWithBinary builds the provider against an already-resolved binary path. It

pkg/mcp/cloud/providers/aws/provider_test.go

@@ -23,6 +23,31 @@ func TestNewResolvesProvider(t *testing.T) {
 	assert.Equal(t, "/usr/bin/aws", p.Binary())
 }
 
+// TestNewResolvesBinaryToAbsolutePath proves New stores an absolute binary path
+// even when PATH resolution would yield a relative one, so a later subprocess
+// env/PATH change cannot redirect what executes. The CLI is dropped into a temp
+// dir reachable through a relative PATH entry; the resolved binary must come
+// back absolute.
+func TestNewResolvesBinaryToAbsolutePath(t *testing.T) {
+	dir := t.TempDir()
+	bin := filepath.Join(dir, "aws")
+	require.NoError(t, os.WriteFile(bin, []byte("#!/bin/sh\n"), 0o755))
+
+	cwd, err := os.Getwd()
+	require.NoError(t, err)
+	t.Cleanup(func() { _ = os.Chdir(cwd) })
+	require.NoError(t, os.Chdir(dir))
+
+	// "." is a relative PATH entry; exec.LookPath("aws") resolves to "aws"
+	// (relative) under it.
+	t.Setenv("PATH", ".")
+
+	p, err := New()
+	require.NoError(t, err)
+	assert.True(t, filepath.IsAbs(p.Binary()),
+		"New must store an absolute binary path, got %q", p.Binary())
+}
+
 func TestDefaultAllowlistCoversReadOnlyAxes(t *testing.T) {
 	p, err := newWithBinary("/usr/bin/aws")
 	require.NoError(t, err)

pkg/mcp/cloud/providers/gcp/provider.go

@@ -8,8 +8,10 @@ package gcp
 import (
 	_ "embed"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"os/exec"
+	"path/filepath"
 
 	"github.com/sourcehawk/triagent/pkg/mcp/cloud"
 )
@@ -36,13 +38,20 @@ type Provider struct {
 }
 
 // New constructs the gcp provider, resolving gcloud to an absolute path once via
-// exec.LookPath so a poisoned PATH cannot redirect the binary at run time.
+// exec.LookPath so a poisoned PATH cannot redirect the binary at run time. A
+// PATH with relative entries makes LookPath return a relative path (flagged with
+// exec.ErrDot); the path is made absolute so a later subprocess env/PATH change
+// cannot reinterpret it against a different working directory.
 func New() (*Provider, error) {
 	bin, err := exec.LookPath("gcloud")
-	if err != nil {
+	if err != nil && !errors.Is(err, exec.ErrDot) {
 		return nil, fmt.Errorf("gcp: resolve gcloud binary: %w", err)
 	}
-	return newWithBinary(bin)
+	abs, err := filepath.Abs(bin)
+	if err != nil {
+		return nil, fmt.Errorf("gcp: resolve gcloud binary to absolute path: %w", err)
+	}
+	return newWithBinary(abs)
 }
 
 // newWithBinary builds the provider against an already-resolved binary path. It

pkg/mcp/cloud/providers/gcp/provider_test.go

@@ -20,6 +20,31 @@ func TestNewResolvesBinaryAndName(t *testing.T) {
 	assert.Equal(t, "/usr/bin/gcloud", p.Binary())
 }
 
+// TestNewResolvesBinaryToAbsolutePath proves New stores an absolute binary path
+// even when PATH resolution would yield a relative one, so a later subprocess
+// env/PATH change cannot redirect what executes. The provider's CLI is dropped
+// into a temp dir reachable through a relative PATH entry; the resolved binary
+// must come back absolute.
+func TestNewResolvesBinaryToAbsolutePath(t *testing.T) {
+	dir := t.TempDir()
+	bin := filepath.Join(dir, "gcloud")
+	require.NoError(t, os.WriteFile(bin, []byte("#!/bin/sh\n"), 0o755))
+
+	cwd, err := os.Getwd()
+	require.NoError(t, err)
+	t.Cleanup(func() { _ = os.Chdir(cwd) })
+	require.NoError(t, os.Chdir(dir))
+
+	// "." is a relative PATH entry; exec.LookPath("gcloud") resolves to "gcloud"
+	// (relative) under it.
+	t.Setenv("PATH", ".")
+
+	p, err := New()
+	require.NoError(t, err)
+	assert.True(t, filepath.IsAbs(p.Binary()),
+		"New must store an absolute binary path, got %q", p.Binary())
+}
+
 func TestDefaultAllowlistLoadsEmbeddedJSON(t *testing.T) {
 	t.Parallel()
 	p, err := newWithBinary("/usr/bin/gcloud")

pkg/mcp/cloud/providers/probe.go

@@ -45,7 +45,12 @@ type Source struct {
 func ProbeSource(ctx context.Context, src Source) cloud.IdentityStatus {
 	p, err := New(src.Provider)
 	if err != nil {
-		return cloud.IdentityStatus{Provider: src.Provider, Valid: false, Hint: err.Error()}
+		return cloud.IdentityStatus{
+			Provider:        src.Provider,
+			AssumedIdentity: src.AssumedIdentity,
+			Valid:           false,
+			Hint:            err.Error(),
+		}
 	}
 	return probeProvider(ctx, p, src.AssumedIdentity, sourceEnvFor(p, src))
 }

pkg/mcp/cloud/providers/probe_test.go

@@ -84,6 +84,21 @@ func TestProbeSourceUnknownProviderDegrades(t *testing.T) {
 	assert.NotEmpty(t, st.Hint)
 }
 
+// TestProbeSourceConstructionFailureKeepsPinnedIdentity proves a provider
+// construction failure (here an unknown provider, which never reaches New's CLI
+// lookup but exercises the same construction-error path) still reports the
+// pinned identity, so preflight and connections name the degraded source's
+// identity the operator must fix instead of an empty one.
+func TestProbeSourceConstructionFailureKeepsPinnedIdentity(t *testing.T) {
+	const pinned = "arn:aws:iam::111122223333:role/triage-ro"
+	st := ProbeSource(context.Background(), Source{Provider: "azure", AssumedIdentity: pinned})
+	assert.False(t, st.Valid)
+	assert.Equal(t, "azure", st.Provider)
+	assert.Equal(t, pinned, st.AssumedIdentity,
+		"a construction failure must still carry the pinned identity")
+	assert.NotEmpty(t, st.Hint)
+}
+
 // fakePassthroughProvider exposes a fixed EnvPassthrough so sourceEnv's
 // carry-and-overlay behaviour can be asserted without a real cloud CLI.
 type fakePassthroughProvider struct{ passthrough []string }