[marcuswhitaker-c1]

Summary

The China section added in f0afde3 documents regions, env vars, and the self-hosted constraint, but the page gives an aws-cn admin no usable IAM setup. Every trust policy names the commercial principal arn:aws:iam::765656841499:role/ConductorOneService, which IAM in an aws-cn account rejects at save time (MalformedPolicyDocument: Invalid principal in policy). The permissions policies also hardcode arn:aws: in Resource ARNs, which can save cleanly and silently match nothing.

This adds an AWS China IAM setup section and a pointer from the C1-hosted trust-policy section:

• Explains why there is no C1-hosted trust relationship in China (sts:AssumeRole cannot cross partitions).
• arn:aws-cn: replacements for the resource-scoped permissions statements.
• Option A: static keys, no trust policy.
• Option B: keys or instance profile assuming a connector role (external ID optional, 32-64 char rule not enforced in single-hop).
• Option C: IRSA on China EKS, web identity trust policy with no sts:ExternalId; issuer and audience read from the user's own cluster.
• Organizations child-account roles: must be OrganizationAccountAccessRole (or BATON_IAM_ASSUME_ROLE_NAME) and must not carry an sts:ExternalId condition.

Docs only. Connector behavior was checked, not changed: aws_client_factory.go builds the child role ARN in the connector's partition and sets only session name and duration; connector.go skips external-ID validation unless global-role-arn is set.

Open items

• The IRSA audience in aws-cn is unconfirmed. The doc tells users to match the ClientIDList on their own OIDC provider. Worth confirming with a China account.
• None of the three options has been run against a real aws-cn account.
• The commercial policies and the Terraform copies are intentionally unchanged; the new section lists the ARN substitutions rather than duplicating each policy.

🤖 Generated with Claude Code

[github-actions]

Superseded — see the current review report for commit 9aa755120346

Connector PR Review: docs: add AWS China (aws-cn) IAM setup

Blocking Issues: 1 | Suggestions: 1 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base af8cec838b0f.
Review mode: full
View review run

Review Summary

This PR changes only docs/connector.mdx. It adds a note above the commercial trust-policy section and a new "AWS China IAM setup" section with arn:aws-cn: resource ARNs, three authentication options (static keys, assume role, IRSA) and trust policies for Organizations child-account roles. I scanned the full diff for security and correctness, then checked the documented behavior against the source:

• Single-hop mode does not validate the external ID (connector.go:255-258).
• The external ID is sent only when it is set (connector.go:165).
• The child-account role ARN uses the connector's partition and sets only session name and duration (aws_client_factory.go:83-98).

Those claims are accurate. However, Option C's IRSA configuration does not work as written. The trusted repo-local criteria loaded. Their code-focused sections (log levels, error wrapping, spans, JSON types, sync data loss, provisioning) do not apply to a docs-only change. I applied the documentation (D1-D4) and breaking-change process checks: no code, config, scope or resource-type behavior changes. No Go code, go.mod or go.sum changed. Nothing was tested against a real aws-cn account, and the PR description says so.

Security Issues

None found.

Correctness Issues

• New — docs/connector.mdx:1153-1154 (Option C, IRSA): IRSA gives the pod web-identity credentials for BatonAWSConnector through the default chain (connector.go:390). Because BATON_USE_ASSUME: "true" is set, single-hop mode then calls sts:AssumeRole into BATON_ROLE_ARN (connector.go:159-170), which is that same role. Since 2022, a role can only assume itself if its trust policy explicitly allows it, and the documented trust policy grants only sts:AssumeRoleWithWebIdentity. Following the doc as written therefore fails at startup with AccessDenied. It also contradicts the existing China IRSA example at line 1008, which omits BATON_USE_ASSUME.

Suggestions

• New — docs/connector.mdx:1162-1172: the child-account trust policy names role/BatonAWSConnector as principal. In Option A, the child-account AssumeRole (aws_client_factory.go:95) is signed by the IAM user's static keys, so Option A users with Organizations enabled need the IAM user ARN as the principal instead. The doc should say so.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Correctness Issues

In `docs/connector.mdx`:
- Around line 1120-1157 (Option C: IRSA on a China EKS cluster): The env block sets BATON_USE_ASSUME "true" and BATON_ROLE_ARN to the same BatonAWSConnector role whose trust policy only allows sts:AssumeRoleWithWebIdentity. With IRSA, LoadDefaultConfig already yields credentials for that role, and single-hop mode (pkg/connector/connector.go:159-170) then calls sts:AssumeRole into the same role. That self-assume is denied unless the trust policy explicitly allows it. Pick one fix: (a) remove BATON_USE_ASSUME (keep BATON_GLOBAL_REGION and optionally BATON_ROLE_ARN for partition derivation) so the connector uses the IRSA credentials directly, consistent with the existing China IRSA example near line 1008; or (b) add a second trust statement allowing "AWS": "arn:aws-cn:iam::<ACCOUNT_ID>:role/BatonAWSConnector" to call sts:AssumeRole; or (c) document separate IRSA and connector roles, with the connector role trusting the IRSA role.

## Suggestions

In `docs/connector.mdx`:
- Around line 1162-1180 (AWS Organizations: child-account roles): The trust policy principal is role/BatonAWSConnector, which only fits Options B and C. In Option A the child-account AssumeRole is signed by the IAM user's static credentials. Add a sentence saying Option A users should use arn:aws-cn:iam::<MANAGEMENT_ACCOUNT_ID>:user/<user name> as the principal.

[github-actions]

Superseded — see the current review report for commit 520ad7133e16

Connector PR Review: docs: add AWS China (aws-cn) IAM setup

Blocking Issues: 1 | Suggestions: 1 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base af8cec838b0f.
Review mode: full
View review run

Review Summary

This PR changes only docs/connector.mdx. It adds a note above the commercial trust-policy section and a new "AWS China IAM setup" section with arn:aws-cn: resource ARNs, three authentication options (static keys, assume role, IRSA) and trust policies for Organizations child-account roles. I scanned the full diff for security and correctness, then checked the documented behavior against the source:

• Single-hop mode does not validate the external ID (connector.go:255-258).
• The external ID is sent only when it is set (connector.go:165).
• The child-account role ARN uses the connector's partition and sets only session name and duration (aws_client_factory.go:83-98).

Those claims are accurate. However, Option C's IRSA configuration does not work as written. The trusted repo-local criteria loaded. Their code-focused sections (log levels, error wrapping, spans, JSON types, sync data loss, provisioning) do not apply to a docs-only change. I applied the documentation (D1-D4) and breaking-change process checks: no code, config, scope or resource-type behavior changes. No Go code, go.mod or go.sum changed. Nothing was tested against a real aws-cn account, and the PR description says so.

Security Issues

None found.

Correctness Issues

• New — docs/connector.mdx:1153-1154 (Option C, IRSA): IRSA gives the pod web-identity credentials for BatonAWSConnector through the default chain (connector.go:390). Because BATON_USE_ASSUME: "true" is set, single-hop mode then calls sts:AssumeRole into BATON_ROLE_ARN (connector.go:159-170), which is that same role. Since 2022, a role can only assume itself if its trust policy explicitly allows it, and the documented trust policy grants only sts:AssumeRoleWithWebIdentity. Following the doc as written therefore fails at startup with AccessDenied. It also contradicts the existing China IRSA example at line 1008, which omits BATON_USE_ASSUME.

Suggestions

• New — docs/connector.mdx:1162-1172: the child-account trust policy names role/BatonAWSConnector as principal. In Option A, the child-account AssumeRole (aws_client_factory.go:95) is signed by the IAM user's static keys, so Option A users with Organizations enabled need the IAM user ARN as the principal instead. The doc should say so.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Correctness Issues

In `docs/connector.mdx`:
- Around line 1120-1157 (Option C: IRSA on a China EKS cluster): The env block sets BATON_USE_ASSUME "true" and BATON_ROLE_ARN to the same BatonAWSConnector role whose trust policy only allows sts:AssumeRoleWithWebIdentity. With IRSA, LoadDefaultConfig already yields credentials for that role, and single-hop mode (pkg/connector/connector.go:159-170) then calls sts:AssumeRole into the same role. That self-assume is denied unless the trust policy explicitly allows it. Pick one fix: (a) remove BATON_USE_ASSUME (keep BATON_GLOBAL_REGION and optionally BATON_ROLE_ARN for partition derivation) so the connector uses the IRSA credentials directly, consistent with the existing China IRSA example near line 1008; or (b) add a second trust statement allowing "AWS": "arn:aws-cn:iam::<ACCOUNT_ID>:role/BatonAWSConnector" to call sts:AssumeRole; or (c) document separate IRSA and connector roles, with the connector role trusting the IRSA role.

## Suggestions

In `docs/connector.mdx`:
- Around line 1162-1180 (AWS Organizations: child-account roles): The trust policy principal is role/BatonAWSConnector, which only fits Options B and C. In Option A the child-account AssumeRole is signed by the IAM user's static credentials. Add a sentence saying Option A users should use arn:aws-cn:iam::<MANAGEMENT_ACCOUNT_ID>:user/<user name> as the principal.

---

Reviewed commit: 9aa755120346

[github-actions]

Blocking issues found — see the full review report

[github-actions]

Superseded — see the current review report for commit 520ad7133e16

Connector PR Review: docs: add AWS China (aws-cn) IAM setup

Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base af8cec838b0f.
Review mode: incremental since 9aa7551
View review run

Review Summary

This PR changes docs only (docs/connector.mdx). It adds a note to the commercial trust-policy section and a new "AWS China IAM setup" section. The new section covers arn:aws-cn: permissions ARNs, three self-hosted auth options (static keys, keys or an instance profile assuming a role, IRSA), and the trust-policy requirements for Organizations child-account roles. The new commits change two things:

• Option C (IRSA) no longer sets BATON_USE_ASSUME, and the doc now explains why.
• A note says that under Option A (static keys) the child-account trust principal must be the IAM user.

I scanned the full PR diff for security and correctness. I checked the documented env-var combinations against connector.go (getCallingConfig, ValidateConfig), partition.go, and aws_client_factory.go. The repo-local criteria loaded. Most of them target connector code that this PR doesn't touch (logging, sync data loss, spans, JSON, provisioning). I applied the documentation-accuracy and breaking-change-process sections. There is no behavior change, so no breaking-change gate applies.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.

Resolved prior findings

• Option C IRSA self-assume AccessDenied (blocking): fixed. docs/connector.mdx:1153-1159 no longer sets BATON_USE_ASSUME. With useAssumeRole false, getCallingConfig returns baseConfig, which holds the IRSA web-identity credentials (pkg/connector/connector.go:151-152). BATON_ROLE_ARN still sets the partition (pkg/connector/partition.go:51-56) and the connector-account comparison (pkg/connector/aws_client_factory.go:157-165), and ValidateConfig accepts an aws-cn role ARN without use-assume (connector.go:285-291). Option C now also matches the earlier China IRSA example at line 1008.
• Option A child-account principal (suggestion): fixed. docs/connector.mdx:1181 tells Option A users to put the IAM user ARN in the child-account trust policy. That matches the child AssumeRole call, which is signed with baseConfig (the static keys) when use-assume is off.

[github-actions]

Superseded — see the current review report for commit 5a968cbc8eef

Connector PR Review: docs: add AWS China (aws-cn) IAM setup

Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base af8cec838b0f.
Review mode: incremental since 9aa7551
View review run

Review Summary

This PR changes docs only (docs/connector.mdx). It adds a note to the commercial trust-policy section and a new "AWS China IAM setup" section. The new section covers arn:aws-cn: permissions ARNs, three self-hosted auth options (static keys, keys or an instance profile assuming a role, IRSA), and the trust-policy requirements for Organizations child-account roles. The new commits change two things:

• Option C (IRSA) no longer sets BATON_USE_ASSUME, and the doc now explains why.
• A note says that under Option A (static keys) the child-account trust principal must be the IAM user.

I scanned the full PR diff for security and correctness. I checked the documented env-var combinations against connector.go (getCallingConfig, ValidateConfig), partition.go, and aws_client_factory.go. The repo-local criteria loaded. Most of them target connector code that this PR doesn't touch (logging, sync data loss, spans, JSON, provisioning). I applied the documentation-accuracy and breaking-change-process sections. There is no behavior change, so no breaking-change gate applies.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.

Resolved prior findings

• Option C IRSA self-assume AccessDenied (blocking): fixed. docs/connector.mdx:1153-1159 no longer sets BATON_USE_ASSUME. With useAssumeRole false, getCallingConfig returns baseConfig, which holds the IRSA web-identity credentials (pkg/connector/connector.go:151-152). BATON_ROLE_ARN still sets the partition (pkg/connector/partition.go:51-56) and the connector-account comparison (pkg/connector/aws_client_factory.go:157-165), and ValidateConfig accepts an aws-cn role ARN without use-assume (connector.go:285-291). Option C now also matches the earlier China IRSA example at line 1008.
• Option A child-account principal (suggestion): fixed. docs/connector.mdx:1181 tells Option A users to put the IAM user ARN in the child-account trust policy. That matches the child AssumeRole call, which is signed with baseConfig (the static keys) when use-assume is off.

---

Reviewed commit: 520ad7133e16

[github-actions]

No blocking issues found — see the full review report

Superseded by a completed review with no blocking issues; current audit evidence persisted in CI.

[github-actions]

Superseded — see the current review report for commit 5a968cbc8eef

Connector PR Review: docs: add AWS China (aws-cn) IAM setup

Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base af8cec838b0f.
Review mode: incremental since 520ad71
View review run

Review Summary

This PR changes only docs/connector.mdx. I scanned the full diff for security and correctness issues; the incremental artifact was complete, with nothing dropped or truncated.

The new commits make these changes:

• Soften the MalformedPolicyDocument wording.
• Add BATON_GLOBAL_AWS_SSO_REGION to every China example.
• Option B: add explicit access keys and an instance-profile principal variant.
• Option C: add a variant where an IRSA bootstrap role assumes the connector role (BATON_USE_ASSUME).
• Explain that the auto-created OrganizationAccountAccessRole already works, and that roles you create yourself need the permissions policy.

I checked each claim against the connector code:

• The global-aws-sso-region field exists: pkg/config/conf.gen.go:12.
• When the keys are omitted, credentials fall back to the ambient chain: GetAwsConfigOptions, pkg/connector/connector.go:725-735.
• In single-hop mode, the connector assumes roleARN with baseConfig and an optional external ID, which matches the bootstrap variant: connector.go:159-168.
• Child-account roles are assumed with only a session name and duration: aws_client_factory.go:95-99.
• The permissions policy already grants sts:AssumeRole on OrganizationAccountAccessRole, so the "nothing to create" guidance holds.

I applied the repo-local criteria. The S, L, J, T and PR sections cover code paths, so they don't apply to a docs-only change. The D (documentation staleness) and BP (breaking-change gate) checks raised nothing: no behavior changed, and the docs match the current config fields and auth flows.

Coverage gap: none of these claims was checked against a real aws-cn account. The PR body says the same.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.

Resolved prior findings

• Option C IRSA self-assume AccessDenied (prior blocking): fixed. The Option C YAML at docs/connector.mdx:1166-1172 leaves BATON_USE_ASSUME unset, so getCallingConfig returns the IRSA baseConfig (pkg/connector/connector.go:151-152). The new bootstrap-role variant (docs/connector.mdx:1176) correctly uses use-assume only when the assumed role is a different role.
• Option A child-account principal (prior suggestion): fixed. docs/connector.mdx:1198 tells Option A users to put the IAM user ARN in the child-account trust policy. That matches the child AssumeRole call, which is signed with baseConfig when use-assume is off.

[github-actions]

Superseded — see the current review report for commit 07ae5e4b99e8

Connector PR Review: docs: add AWS China (aws-cn) IAM setup

Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base af8cec838b0f.
Review mode: incremental since 520ad71
View review run

Review Summary

This PR changes only docs/connector.mdx. I scanned the full diff for security and correctness issues; the incremental artifact was complete, with nothing dropped or truncated.

The new commits make these changes:

• Soften the MalformedPolicyDocument wording.
• Add BATON_GLOBAL_AWS_SSO_REGION to every China example.
• Option B: add explicit access keys and an instance-profile principal variant.
• Option C: add a variant where an IRSA bootstrap role assumes the connector role (BATON_USE_ASSUME).
• Explain that the auto-created OrganizationAccountAccessRole already works, and that roles you create yourself need the permissions policy.

I checked each claim against the connector code:

• The global-aws-sso-region field exists: pkg/config/conf.gen.go:12.
• When the keys are omitted, credentials fall back to the ambient chain: GetAwsConfigOptions, pkg/connector/connector.go:725-735.
• In single-hop mode, the connector assumes roleARN with baseConfig and an optional external ID, which matches the bootstrap variant: connector.go:159-168.
• Child-account roles are assumed with only a session name and duration: aws_client_factory.go:95-99.
• The permissions policy already grants sts:AssumeRole on OrganizationAccountAccessRole, so the "nothing to create" guidance holds.

I applied the repo-local criteria. The S, L, J, T and PR sections cover code paths, so they don't apply to a docs-only change. The D (documentation staleness) and BP (breaking-change gate) checks raised nothing: no behavior changed, and the docs match the current config fields and auth flows.

Coverage gap: none of these claims was checked against a real aws-cn account. The PR body says the same.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.

Resolved prior findings

• Option C IRSA self-assume AccessDenied (prior blocking): fixed. The Option C YAML at docs/connector.mdx:1166-1172 leaves BATON_USE_ASSUME unset, so getCallingConfig returns the IRSA baseConfig (pkg/connector/connector.go:151-152). The new bootstrap-role variant (docs/connector.mdx:1176) correctly uses use-assume only when the assumed role is a different role.
• Option A child-account principal (prior suggestion): fixed. docs/connector.mdx:1198 tells Option A users to put the IAM user ARN in the child-account trust policy. That matches the child AssumeRole call, which is signed with baseConfig when use-assume is off.

---

Reviewed commit: 5a968cbc8eef

[github-actions]

No blocking issues found — see the full review report

[github-actions]

Superseded — see the current review report for commit 07ae5e4b99e8

Connector PR Review: docs: add AWS China (aws-cn) IAM setup

Blocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base af8cec838b0f.
Review mode: incremental since 5a968cb
View review run

Review Summary

The new commit adds a table to docs/connector.mdx mapping each C1 connector setting to its environment variable and its China-specific guidance. It also lists the env-only variables that select the partition and credentials (BATON_GLOBAL_REGION, the keys, BATON_USE_ASSUME) and says BATON_GLOBAL_ROLE_ARN must stay unset. I checked the table against pkg/config/config.go:

• Display names, env var names, the OrganizationAccountAccessRole default and the us-east-1 SSO region default all match.
• The env-only fields are the ExportTargetOps ones.
• The startup rejection of a us-east-1 SSO region matches validatePartitionConsistency (pkg/connector/connector.go:319-323).

I scanned the full PR diff (one file, docs only) for security and correctness and found no issues. On the trusted criteria: the diff touches no client, resource builder, provisioning, logging or JSON code, so sections A–S had nothing to check beyond confirming that the documented config and partition behavior matches the code. The diff changes no breaking or data-loss surface.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• New — docs/connector.mdx:1072: The External ID row says the value is used "only" in Option B. The Option C bootstrap-role variant also sets BATON_USE_ASSUME, and in single-hop mode the connector passes a non-empty external ID to AssumeRole (pkg/connector/connector.go:159-168). (medium confidence)
• New — docs/connector.mdx:1078: The row links to the self-hosted Secret example, but that example has no BATON_SYNC_ONLY_ATTACHED_POLICIES entry. "Each needs the matching permission" also does not apply to Sync Only Attached Policies or Account Provisioning Target. (medium confidence)

Resolved prior findings

• Option C IRSA self-assume AccessDenied (prior blocking): fixed. docs/connector.mdx:1188-1191 leaves BATON_USE_ASSUME unset when IRSA supplies the connector role, so getCallingConfig returns baseConfig (connector.go:151-152). The bootstrap variant at line 1193 sets use-assume only for a distinct role.
• Option A child-account trust principal (prior suggestion): fixed. docs/connector.mdx:1215 tells Option A users to name the IAM user as the child-account principal.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `docs/connector.mdx`:
- Around line 1072: The External ID row says it is "Only used when the connector assumes a role (Option B)". The Option C bootstrap-role variant (line 1193) also sets BATON_USE_ASSUME, and the connector sends BATON_EXTERNAL_ID on that single-hop AssumeRole too. Change it to "(Option B, or the Option C bootstrap-role variant)".
- Around line 1078: The "Sync ..." row points to the Step 2 self-hosted config, but that YAML has no BATON_SYNC_ONLY_ATTACHED_POLICIES. List the env var names inline (BATON_SYNC_SECRETS, BATON_SYNC_IAM_USER_CONSOLE_ACCESS, BATON_SYNC_RESOURCE_TAGS, BATON_SYNC_SSO_USER_LAST_LOGIN, BATON_SYNC_ONLY_ATTACHED_POLICIES, BATON_CREATE_ACCOUNT_RESOURCE_TYPE) or add the missing one to the Step 2 example. Also limit "Each needs the matching permission" to the settings that actually need an extra permission.

[github-actions]

Superseded — see the current review report for commit 9e29d738aaa6

Connector PR Review: docs: add AWS China (aws-cn) IAM setup

Blocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base af8cec838b0f.
Review mode: incremental since 5a968cb
View review run

Review Summary

The new commit adds a table to docs/connector.mdx mapping each C1 connector setting to its environment variable and its China-specific guidance. It also lists the env-only variables that select the partition and credentials (BATON_GLOBAL_REGION, the keys, BATON_USE_ASSUME) and says BATON_GLOBAL_ROLE_ARN must stay unset. I checked the table against pkg/config/config.go:

• Display names, env var names, the OrganizationAccountAccessRole default and the us-east-1 SSO region default all match.
• The env-only fields are the ExportTargetOps ones.
• The startup rejection of a us-east-1 SSO region matches validatePartitionConsistency (pkg/connector/connector.go:319-323).

I scanned the full PR diff (one file, docs only) for security and correctness and found no issues. On the trusted criteria: the diff touches no client, resource builder, provisioning, logging or JSON code, so sections A–S had nothing to check beyond confirming that the documented config and partition behavior matches the code. The diff changes no breaking or data-loss surface.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• New — docs/connector.mdx:1072: The External ID row says the value is used "only" in Option B. The Option C bootstrap-role variant also sets BATON_USE_ASSUME, and in single-hop mode the connector passes a non-empty external ID to AssumeRole (pkg/connector/connector.go:159-168). (medium confidence)
• New — docs/connector.mdx:1078: The row links to the self-hosted Secret example, but that example has no BATON_SYNC_ONLY_ATTACHED_POLICIES entry. "Each needs the matching permission" also does not apply to Sync Only Attached Policies or Account Provisioning Target. (medium confidence)

Resolved prior findings

• Option C IRSA self-assume AccessDenied (prior blocking): fixed. docs/connector.mdx:1188-1191 leaves BATON_USE_ASSUME unset when IRSA supplies the connector role, so getCallingConfig returns baseConfig (connector.go:151-152). The bootstrap variant at line 1193 sets use-assume only for a distinct role.
• Option A child-account trust principal (prior suggestion): fixed. docs/connector.mdx:1215 tells Option A users to name the IAM user as the child-account principal.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `docs/connector.mdx`:
- Around line 1072: The External ID row says it is "Only used when the connector assumes a role (Option B)". The Option C bootstrap-role variant (line 1193) also sets BATON_USE_ASSUME, and the connector sends BATON_EXTERNAL_ID on that single-hop AssumeRole too. Change it to "(Option B, or the Option C bootstrap-role variant)".
- Around line 1078: The "Sync ..." row points to the Step 2 self-hosted config, but that YAML has no BATON_SYNC_ONLY_ATTACHED_POLICIES. List the env var names inline (BATON_SYNC_SECRETS, BATON_SYNC_IAM_USER_CONSOLE_ACCESS, BATON_SYNC_RESOURCE_TAGS, BATON_SYNC_SSO_USER_LAST_LOGIN, BATON_SYNC_ONLY_ATTACHED_POLICIES, BATON_CREATE_ACCOUNT_RESOURCE_TYPE) or add the missing one to the Step 2 example. Also limit "Each needs the matching permission" to the settings that actually need an extra permission.

---

Reviewed commit: 07ae5e4b99e8

[github-actions]

No blocking issues found — see the full review report

[github-actions]

Connector PR Review: docs: add AWS China (aws-cn) IAM setup

⏳ Provisional — deeper review still in progress.

Blocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base af8cec838b0f.
Review mode: incremental since 07ae5e4
View review run

Review Summary

The new commit adds an "Other China-specific things to check" list to docs/connector.mdx. It covers region values, the Identity Center region, AWS managed policies, the container image, network egress and STS actions. This is a docs-only PR. The claims are being checked against the connector code.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• Prior — still present — docs/connector.mdx:1072: External ID "only used" in Option B. Verification is in progress.
• Prior — still present — docs/connector.mdx:1078: settings row links to a Secret example that has no BATON_SYNC_ONLY_ATTACHED_POLICIES entry. Verification is in progress.

[github-actions]

Superseded — see the current review report for commit 9e29d738aaa6

Connector PR Review: docs: add AWS China (aws-cn) IAM setup

Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base af8cec838b0f.
Review mode: incremental since 07ae5e4
View review run

Review Summary

This PR changes only docs/connector.mdx. It adds an AWS China IAM setup section: the arn:aws-cn: permission ARNs, three auth options (static keys, assume role, IRSA), and the trust rules for Organizations child-account roles. The new commits do four things:

• Split the combined "Sync …" settings row into one row per setting, each with its env var and permission.
• Widen the External ID note to cover the Option C bootstrap-role variant.
• Refine the China settings table.
• Add a list of "Other China-specific things to check".

I scanned the full PR diff for security and correctness and checked the new claims against the source:

• Env var names match the config fields in pkg/config/config.go:117-156.
• CloudTrail and Identity Center clients are created only when both Organizations and Identity Center are enabled (pkg/connector/connector.go:535-547).
• The Identity Center region check runs whenever Identity Center is enabled (connector.go:319).
• The partition is taken from the region prefix (partition.go:25-34).
• AWS-managed policies are treated as global objects (iam_policy.go:54-58).

Trusted repo-local criteria loaded. Sections A–G and S cover code (log levels, error wrapping, spans, JSON types, provisioning, IDs, empty-result data loss). This PR changes no Go code, so none of them apply. I checked the D1–D4 docs rules and the B-series breaking-change rules too: nothing in the connector's behavior, scopes or identifiers changes.

I did not cover the open items listed in the PR description: the IRSA audience in aws-cn, and whether any option works against a real China account. Neither can be checked from source.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.

Resolved prior findings

• Option C IRSA self-assume AccessDenied (prior blocking): fixed. docs/connector.mdx:1193 now leaves BATON_USE_ASSUME unset when IRSA supplies the connector role. The bootstrap-role variant sets it only when the target is a distinct role.
• Option A child-account trust principal (prior suggestion): fixed. docs/connector.mdx:1220 tells Option A users to name the IAM user as the principal.
• External ID "only used" in Option B (from the earlier summary): fixed. docs/connector.mdx:1072 now covers the Option C bootstrap-role variant as well.
• Settings row linking to a Secret example without BATON_SYNC_ONLY_ATTACHED_POLICIES (from the earlier summary): fixed. docs/connector.mdx:1078-1083 now gives one row per setting, each with its own env var and the permission it needs, or "No extra permission".

[github-actions]

Superseded — see the current review report for commit 7ca263fe666a

Connector PR Review: docs: add AWS China (aws-cn) IAM setup

Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base af8cec838b0f.
Review mode: incremental since 07ae5e4
View review run

Review Summary

This PR changes only docs/connector.mdx. It adds an AWS China IAM setup section: the arn:aws-cn: permission ARNs, three auth options (static keys, assume role, IRSA), and the trust rules for Organizations child-account roles. The new commits do four things:

• Split the combined "Sync …" settings row into one row per setting, each with its env var and permission.
• Widen the External ID note to cover the Option C bootstrap-role variant.
• Refine the China settings table.
• Add a list of "Other China-specific things to check".

I scanned the full PR diff for security and correctness and checked the new claims against the source:

• Env var names match the config fields in pkg/config/config.go:117-156.
• CloudTrail and Identity Center clients are created only when both Organizations and Identity Center are enabled (pkg/connector/connector.go:535-547).
• The Identity Center region check runs whenever Identity Center is enabled (connector.go:319).
• The partition is taken from the region prefix (partition.go:25-34).
• AWS-managed policies are treated as global objects (iam_policy.go:54-58).

Trusted repo-local criteria loaded. Sections A–G and S cover code (log levels, error wrapping, spans, JSON types, provisioning, IDs, empty-result data loss). This PR changes no Go code, so none of them apply. I checked the D1–D4 docs rules and the B-series breaking-change rules too: nothing in the connector's behavior, scopes or identifiers changes.

I did not cover the open items listed in the PR description: the IRSA audience in aws-cn, and whether any option works against a real China account. Neither can be checked from source.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.

Resolved prior findings

• Option C IRSA self-assume AccessDenied (prior blocking): fixed. docs/connector.mdx:1193 now leaves BATON_USE_ASSUME unset when IRSA supplies the connector role. The bootstrap-role variant sets it only when the target is a distinct role.
• Option A child-account trust principal (prior suggestion): fixed. docs/connector.mdx:1220 tells Option A users to name the IAM user as the principal.
• External ID "only used" in Option B (from the earlier summary): fixed. docs/connector.mdx:1072 now covers the Option C bootstrap-role variant as well.
• Settings row linking to a Secret example without BATON_SYNC_ONLY_ATTACHED_POLICIES (from the earlier summary): fixed. docs/connector.mdx:1078-1083 now gives one row per setting, each with its own env var and the permission it needs, or "No extra permission".

---

Reviewed commit: 9e29d738aaa6

[github-actions]

No blocking issues found — see the full review report

[github-actions]

Superseded — see the current review report for commit 7ca263fe666a

Connector PR Review: docs: add AWS China (aws-cn) IAM setup

Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base af8cec838b0f.
Review mode: incremental since 9e29d73
View review run

Review Summary

This PR changes only docs. It adds an AWS China (aws-cn) IAM setup section to docs/connector.mdx. The new commit adds four notes that steer China users to the self-hosted path:

• the Gather AWS credentials note now says China is self-hosted only;
• the Cloud-hosted tab says it does not support aws-cn;
• the Self-hosted tab says to do Step 1 and skip the C1 External ID and trust role;
• the Terraform tab says its script creates a commercial-partition trust and cannot be used for China.

I scanned the full PR diff (one file, docs/connector.mdx) for security and correctness. I checked every in-page anchor the new notes use (#configure-the-aws-connector, #gather-aws-credentials, #aws-china-aws-cn-partition, #aws-china-iam-setup), and each one resolves to an existing heading (lines 829, 236, 1001, 1064). The new notes agree with the existing China section at docs/connector.mdx:1068. The trusted repo-local criteria loaded. Their connector-code rules (log levels, error wrapping, spans, JSON types, sync data loss, provisioning) do not apply to a change with no code. The documentation-staleness check (D1–D4) is met because this PR is itself the docs update, and no connector behavior changes. The incremental artifact was not partial.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.

Resolved prior findings

• Option C IRSA self-assume AccessDenied (prior blocking, human-engaged thread): fixed. docs/connector.mdx:1201 and :1204 now say to leave BATON_USE_ASSUME unset when IRSA supplies the connector role. :1206 sets it only in the variant where a bootstrap role assumes a different connector role.
• Option A child-account trust principal (prior suggestion, human-engaged thread): fixed. docs/connector.mdx:1228 tells Option A users to name the IAM user as the principal.

[github-actions]

Connector PR Review: docs: add AWS China (aws-cn) IAM setup

Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base af8cec838b0f.
Review mode: incremental since 9e29d73
View review run

Review Summary

This PR changes only docs. It adds an AWS China (aws-cn) IAM setup section to docs/connector.mdx. The new commit adds four notes that steer China users to the self-hosted path:

• the Gather AWS credentials note now says China is self-hosted only;
• the Cloud-hosted tab says it does not support aws-cn;
• the Self-hosted tab says to do Step 1 and skip the C1 External ID and trust role;
• the Terraform tab says its script creates a commercial-partition trust and cannot be used for China.

I scanned the full PR diff (one file, docs/connector.mdx) for security and correctness. I checked every in-page anchor the new notes use (#configure-the-aws-connector, #gather-aws-credentials, #aws-china-aws-cn-partition, #aws-china-iam-setup), and each one resolves to an existing heading (lines 829, 236, 1001, 1064). The new notes agree with the existing China section at docs/connector.mdx:1068. The trusted repo-local criteria loaded. Their connector-code rules (log levels, error wrapping, spans, JSON types, sync data loss, provisioning) do not apply to a change with no code. The documentation-staleness check (D1–D4) is met because this PR is itself the docs update, and no connector behavior changes. The incremental artifact was not partial.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.

Resolved prior findings

• Option C IRSA self-assume AccessDenied (prior blocking, human-engaged thread): fixed. docs/connector.mdx:1201 and :1204 now say to leave BATON_USE_ASSUME unset when IRSA supplies the connector role. :1206 sets it only in the variant where a bootstrap role assumes a different connector role.
• Option A child-account trust principal (prior suggestion, human-engaged thread): fixed. docs/connector.mdx:1228 tells Option A users to name the IAM user as the principal.

---

Reviewed commit: 7ca263fe666a

[github-actions]

No blocking issues found — see the full review report