fix(cloud): address review findings in the harness, allowlist, and providers

cmd/triagent-mcp/serve.go

@@ -446,14 +446,18 @@ func runCloud(ctx context.Context, f serveFlags) error {
 	if f.cloudProvider == "" {
 		return fmt.Errorf("--provider is required (gcp or aws) (set --provider or $%s)", cloud.EnvProvider)
 	}
+	scope, err := parseCloudScope(os.Getenv(cloud.EnvScope))
+	if err != nil {
+		return fmt.Errorf("build cloud mcp server: %w", err)
+	}
 	provider, err := providers.New(f.cloudProvider)
 	if err != nil {
 		return err
 	}
 	srv, err := cloud.New(cloud.Options{
 		Provider:         provider,
 		AllowlistPath:    os.Getenv(cloud.EnvAllowlistPath),
-		Scope:            parseCloudScope(os.Getenv(cloud.EnvScope)),
+		Scope:            scope,
 		ExpectedIdentity: os.Getenv(cloud.EnvExpectedIdentity),
 	})
 	if err != nil {
@@ -465,18 +469,18 @@ func runCloud(ctx context.Context, f serveFlags) error {
 
 // parseCloudScope decodes the JSON-encoded target scope the launcher froze into
 // a cloud.ScopeAllowlist. An empty value yields an empty scope, which leaves the
-// target axes unconstrained; a malformed value is logged and treated the same,
-// so a bad profile entry never silently widens scope.
-func parseCloudScope(raw string) cloud.ScopeAllowlist {
+// target axes unconstrained. A malformed value is an error that aborts startup:
+// failing closed, since a misconfigured scope must never silently widen run_cli
+// by dropping the deployment's restrictions.
+func parseCloudScope(raw string) (cloud.ScopeAllowlist, error) {
 	var scope cloud.ScopeAllowlist
 	if raw == "" {
-		return scope
+		return scope, nil
 	}
 	if err := json.Unmarshal([]byte(raw), &scope); err != nil {
-		log.Warn("mcp serve --kind=cloud: ignoring malformed scope", "error", err)
-		return cloud.ScopeAllowlist{}
+		return cloud.ScopeAllowlist{}, fmt.Errorf("malformed cloud scope in $%s: %w", cloud.EnvScope, err)
 	}
-	return scope
+	return scope, nil
 }
 
 func runGit(ctx context.Context, f serveFlags) error {

cmd/triagent-mcp/serve_cloud_test.go

@@ -34,3 +34,34 @@ func TestServeCmd_KnowsCloudKind(t *testing.T) {
 	cmd := serveCmd()
 	assert.Contains(t, cmd.Long, "cloud", "serve --help should list cloud")
 }
+
+func TestParseCloudScope_EmptyYieldsUnconstrained(t *testing.T) {
+	t.Parallel()
+	scope, err := parseCloudScope("")
+	require.NoError(t, err)
+	assert.Empty(t, scope.Projects)
+	assert.Empty(t, scope.Regions)
+	assert.Empty(t, scope.Accounts)
+}
+
+func TestParseCloudScope_ValidJSON(t *testing.T) {
+	t.Parallel()
+	scope, err := parseCloudScope(`{"projects":["prod"],"regions":["us-central1"]}`)
+	require.NoError(t, err)
+	assert.Equal(t, []string{"prod"}, scope.Projects)
+	assert.Equal(t, []string{"us-central1"}, scope.Regions)
+}
+
+func TestParseCloudScope_MalformedFailsClosed(t *testing.T) {
+	t.Parallel()
+	_, err := parseCloudScope(`{"projects":`)
+	require.Error(t, err, "a malformed scope must fail closed, not silently drop restrictions")
+}
+
+func TestRunCloud_MalformedScopeAborts(t *testing.T) {
+	t.Setenv("TRIAGENT_CLOUD_PROVIDER", "gcp")
+	t.Setenv("TRIAGENT_CLOUD_SCOPE", `{"projects":`)
+	err := runCloud(context.Background(), serveFlags{kind: "cloud", cloudProvider: "gcp"})
+	require.Error(t, err, "a malformed scope must abort cloud-server startup")
+	assert.Contains(t, err.Error(), "scope", "the error should name the scope")
+}

pkg/mcp/cloud/allowlist.go

@@ -109,15 +109,17 @@ func filterAllowlist(list *CommandAllowlist, extra DenyFloor) *CommandAllowlist
 	return out
 }
 
-// Allows reports whether argv's positional subcommand path exactly equals an
-// allowlisted command path. Flag tokens and their values do not participate;
-// only the leading positionals do. The match is exact rather than prefix so a
-// surplus positional — a shell metacharacter token, an extra argument — never
-// rides through on the back of an allowed prefix.
+// Allows reports whether an allowlisted command path is a token-wise prefix of
+// argv's leading positional subcommand path. Flag tokens and their values do
+// not participate; only the leading positionals do. Prefix rather than exact
+// match lets a describe/get verb chain carry its trailing resource operand
+// (`compute instances describe my-vm`): there is no shell, so a trailing token
+// is an inert argument to the already-dispatched subcommand. Surplus tokens
+// that are shell-control sequences are caught separately in validateArgv.
 func (a *CommandAllowlist) Allows(argv []string) bool {
 	path := subcommandPath(argv)
 	for _, c := range a.Commands {
-		if pathEqual(path, normalizePath(c.Path)) {
+		if pathHasPrefix(path, normalizePath(c.Path)) {
 			return true
 		}
 	}
@@ -169,16 +171,3 @@ func pathHasPrefix(path, prefix []string) bool {
 	}
 	return true
 }
-
-// pathEqual reports whether two token paths are identical.
-func pathEqual(a, b []string) bool {
-	if len(a) != len(b) {
-		return false
-	}
-	for i := range a {
-		if a[i] != b[i] {
-			return false
-		}
-	}
-	return true
-}

pkg/mcp/cloud/allowlist_test.go

@@ -48,11 +48,37 @@ func TestLoadCommandAllowlistMergesProviderDenyFloorAdditions(t *testing.T) {
 		"compute instances list should remain allowed")
 }
 
-func TestAllowsMatchesLongestPathPrefix(t *testing.T) {
+func TestAllowsMatchesVerbChainAsPrefix(t *testing.T) {
 	t.Parallel()
 	al := &CommandAllowlist{Commands: []Command{{Path: "compute firewall-rules list"}}}
 	assert.True(t, al.Allows([]string{"compute", "firewall-rules", "list", "--project", "prod"}),
 		"argv whose leading tokens match an allowed path should pass")
 	assert.False(t, al.Allows([]string{"compute", "firewall-rules", "delete"}),
 		"a different verb under the same group must not be allowed")
 }
+
+func TestAllowsAcceptsResourceOperandAfterVerbChain(t *testing.T) {
+	t.Parallel()
+	al := &CommandAllowlist{Commands: []Command{{Path: "compute instances describe"}}}
+	// A describe/get command takes a resource operand as a trailing positional;
+	// the allowlisted verb chain must match as a prefix so the operand rides
+	// through. There is no shell, so the operand is an inert argument.
+	assert.True(t, al.Allows([]string{"compute", "instances", "describe", "my-vm", "--project", "prod"}),
+		"an allowlisted verb chain followed by a resource operand must pass")
+	assert.True(t, al.Allows([]string{"compute", "instances", "describe", "my-vm", "us-vm-2"}),
+		"trailing positionals after the verb chain must pass")
+}
+
+func TestAllowsRejectsArgvShorterThanPath(t *testing.T) {
+	t.Parallel()
+	al := &CommandAllowlist{Commands: []Command{{Path: "compute instances describe"}}}
+	assert.False(t, al.Allows([]string{"compute", "instances"}),
+		"an argv shorter than the allowlisted path is not a match")
+}
+
+func TestAllowsRejectsDifferentVerbAtPrefixDepth(t *testing.T) {
+	t.Parallel()
+	al := &CommandAllowlist{Commands: []Command{{Path: "compute instances describe"}}}
+	assert.False(t, al.Allows([]string{"compute", "instances", "delete", "my-vm"}),
+		"a different verb at the same depth must not match")
+}

pkg/mcp/cloud/harness.go

@@ -1,6 +1,7 @@
 package cloud
 
 import (
+	"bytes"
 	"context"
 	"errors"
 	"os/exec"
@@ -16,20 +17,35 @@ const defaultOutputLimit = 64 * 1024
 // 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.
+// 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
 
-	out, err := cmd.Output()
+	var stdout, stderr bytes.Buffer
+	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.Stderr = string(errOut)
+
 	if err != nil {
 		var exitErr *exec.ExitError
 		if errors.As(err, &exitErr) {

pkg/mcp/cloud/harness_test.go

@@ -17,3 +17,27 @@ func TestExecCLIExitCode(t *testing.T) {
 	require.NoError(t, err, "non-zero exit should not be a Go error")
 	assert.Equal(t, 1, r.ExitCode)
 }
+
+// TestExecCLICapturesStderr proves a non-zero exit carries the child's stderr,
+// the context gcloud/aws write errors to. Without it run_cli would surface an
+// empty stdout and no explanation for the failure.
+func TestExecCLICapturesStderr(t *testing.T) {
+	t.Parallel()
+	// /bin/sh here is only the test fixture producing a stderr write + nonzero
+	// exit; the harness itself never shells (see harness_security_test.go).
+	r, err := execCLI(context.Background(), "/bin/sh",
+		[]string{"-c", "echo boom 1>&2; exit 3"}, nil, 4096)
+	require.NoError(t, err, "non-zero exit should not be a Go error")
+	assert.Equal(t, 3, r.ExitCode)
+	assert.Contains(t, r.Stderr, "boom", "stderr must be captured")
+}
+
+// TestExecCLITruncatesStderr caps stderr at the same limit as stdout so a
+// noisy provider error cannot blow the context budget.
+func TestExecCLITruncatesStderr(t *testing.T) {
+	t.Parallel()
+	r, err := execCLI(context.Background(), "/bin/sh",
+		[]string{"-c", "printf '%0.sx' $(seq 1 100) 1>&2; exit 1"}, nil, 10)
+	require.NoError(t, err)
+	assert.LessOrEqual(t, len(r.Stderr), 10, "stderr exceeded limit")
+}

pkg/mcp/cloud/provider.go

@@ -73,6 +73,7 @@ type IdentityStatus struct {
 // is never surfaced; the harness caps output and reports truncation.
 type CLIResult struct {
 	Stdout    string `json:"stdout"`
+	Stderr    string `json:"stderr,omitempty"`
 	Truncated bool   `json:"truncated"`
 	ExitCode  int    `json:"exit_code"`
 }

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

@@ -70,23 +70,56 @@ func evaluateIdentity(arn, expectedRoleARN string) (bool, string) {
 
 // assumedRoleARN reports whether arn is an STS assumed-role ARN and, if so,
 // returns the canonical IAM role ARN behind it. An assumed-role ARN has the
-// shape arn:aws:sts::<account>:assumed-role/<role-name>/<session>; the IAM role
-// it stands for is arn:aws:iam::<account>:role/<role-name>.
+// shape arn:<partition>:sts::<account>:assumed-role/<role-path-and-name>/<session>,
+// across the aws, aws-us-gov, and aws-cn partitions. The role keeps any IAM
+// path: the role-path-and-name is everything between "assumed-role/" and the
+// final "/<session>" segment, so a path-prefixed role like
+// assumed-role/team/sub/Role/session resolves to role/team/sub/Role. The IAM
+// role it stands for is arn:<partition>:iam::<account>:role/<role-path-and-name>.
 func assumedRoleARN(arn string) (string, bool) {
-	const prefix = "arn:aws:sts::"
+	const stsInfix = ":sts::"
 	const marker = ":assumed-role/"
-	if !strings.HasPrefix(arn, prefix) {
+	partition, ok := arnPartition(arn)
+	if !ok {
+		return "", false
+	}
+	if !strings.HasPrefix(arn, "arn:"+partition+stsInfix) {
 		return "", false
 	}
 	idx := strings.Index(arn, marker)
 	if idx < 0 {
 		return "", false
 	}
-	account := arn[len(prefix):idx]
+	account := arn[len("arn:"+partition+stsInfix):idx]
 	rest := arn[idx+len(marker):]
-	roleName, _, found := strings.Cut(rest, "/")
-	if !found || roleName == "" || account == "" {
+	// The session name is the final slash-delimited segment; the role path and
+	// name is everything before it.
+	rolePath, _, found := lastCut(rest, "/")
+	if !found || rolePath == "" || account == "" {
 		return "", false
 	}
-	return fmt.Sprintf("arn:aws:iam::%s:role/%s", account, roleName), true
+	return fmt.Sprintf("arn:%s:iam::%s:role/%s", partition, account, rolePath), true
+}
+
+// arnPartition returns the partition segment of an ARN (the field between the
+// first two colons of "arn:<partition>:...").
+func arnPartition(arn string) (string, bool) {
+	rest, ok := strings.CutPrefix(arn, "arn:")
+	if !ok {
+		return "", false
+	}
+	partition, _, found := strings.Cut(rest, ":")
+	if !found || partition == "" {
+		return "", false
+	}
+	return partition, true
+}
+
+// lastCut splits s around the last instance of sep, returning the text before
+// and after it. found reports whether sep appears in s.
+func lastCut(s, sep string) (before, after string, found bool) {
+	if i := strings.LastIndex(s, sep); i >= 0 {
+		return s[:i], s[i+len(sep):], true
+	}
+	return s, "", false
 }

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

@@ -90,6 +90,60 @@ func TestIdentityRejectsMismatchedExpectedRoleArn(t *testing.T) {
 	assert.NotEmpty(t, st.Hint)
 }
 
+func TestAssumedRoleARNParsesPartitionsAndPaths(t *testing.T) {
+	t.Parallel()
+	cases := []struct {
+		name string
+		arn  string
+		want string
+		ok   bool
+	}{
+		{
+			"commercial",
+			"arn:aws:sts::111122223333:assumed-role/triagent-readonly/session",
+			"arn:aws:iam::111122223333:role/triagent-readonly",
+			true,
+		},
+		{
+			"gov-cloud",
+			"arn:aws-us-gov:sts::111122223333:assumed-role/triagent-readonly/session",
+			"arn:aws-us-gov:iam::111122223333:role/triagent-readonly",
+			true,
+		},
+		{
+			"china",
+			"arn:aws-cn:sts::111122223333:assumed-role/triagent-readonly/session",
+			"arn:aws-cn:iam::111122223333:role/triagent-readonly",
+			true,
+		},
+		{
+			"iam-path",
+			"arn:aws:sts::111122223333:assumed-role/team/sub/triagent-readonly/session",
+			"arn:aws:iam::111122223333:role/team/sub/triagent-readonly",
+			true,
+		},
+		{
+			"plain-user",
+			"arn:aws:iam::111122223333:user/operator",
+			"",
+			false,
+		},
+		{
+			"no-session",
+			"arn:aws:sts::111122223333:assumed-role/triagent-readonly",
+			"",
+			false,
+		},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			got, ok := assumedRoleARN(tc.arn)
+			assert.Equal(t, tc.ok, ok)
+			assert.Equal(t, tc.want, got)
+		})
+	}
+}
+
 func TestIdentityInvalidOnNonZeroExit(t *testing.T) {
 	f := &fakeRun{results: map[string]cloud.CLIResult{
 		"sts get-caller-identity": {ExitCode: 255, Stdout: ""},