Repository navigation
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. CodeRabbit Verify
|
|
@codex review for the current head: correctness, IAM/bucket-removal safety, and any regression introduced by this change. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c36c29c508
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| def plan(before, member): | ||
| require(isinstance(before, dict), "Invalid policy") | ||
| require(set(before) <= {"bindings", "etag", "version"}, "Unknown policy fields") |
There was a problem hiding this comment.
Accept the Storage API's documented policy metadata
The workflow reads the Cloud Storage JSON API directly, whose documented getIamPolicy response includes kind: "storage#policy" and resourceId alongside the allowed fields (API reference). This allowlist therefore rejects a valid bucket policy with Unknown policy fields, preventing both dry-run evidence generation and grant removal even with correct Environment configuration. I reproduced this using the extracted plan() function and a synthetic response matching the documented schema. Accept and validate these metadata fields, or consistently strip them before planning and readback comparison.
Useful? React with 👍 / 👎.
AI assistance
Codex (GPT-6, host-selected configuration).
What changed
Add one manual, default-dry-run workflow for the owner-approved Firstrade step-5 grant removal. It removes only the configured deployment service account's unconditional storage.objectCreator and storage.objectViewer grants from the configured bucket. No other files or workflows change.
Configuration is required before execution. The repository currently has zero GitHub Environments, and no repository variable
FIRSTRADE_STEP5_ENVIRONMENT. Existing FT workflows hardcode their WIF bindings; this workflow does not reuse those literals. This PR does not create an Environment or change secrets, variables, IAM, or production settings.Required configuration:
FIRSTRADE_STEP5_ENVIRONMENT: the owner-selected, existing GitHub Environment name.GCP_PROJECT_ID,GCP_WORKLOAD_IDENTITY_PROVIDER,GCP_WORKLOAD_IDENTITY_SERVICE_ACCOUNT,FIRSTRADE_STEP5_BUCKET, andFIRSTRADE_STEP5_SERVICE_ACCOUNT.The Environment WIF principal needs bucket IAM read/write permission. Authentication uses short-lived tokens without credential files. The default-branch and configuration checks precede authentication. Missing, duplicate, conditional, malformed, or unexpected grants fail closed. The write carries the freshly read etag, never retries, and compares the returned policy and fresh readback against the exact planned policy. Logs/artifacts contain masked policies and explicit two-removal evidence only. A workflow-level concurrency group serializes this operation; the etag protects against other writers.
How verified
NO_GCE_CHECK=true. The first unconfigured run was interrupted after 1098 passes because an existing notification test attempted GCE metadata discovery; the supported google-auth setting disables only that local discovery. No tests were removed or skipped.id-tokenpermission andadd-maskcommand syntax; scanner rules were not changed.Why
Execute the existing bounded owner approval through Environment-backed GitHub Actions without copying production credentials to the box or publishing resource identifiers.
Risk
High: this adds a privileged manual IAM workflow. Owner approval is limited to the existing exact two-grant removal. Required native checks and review/merge policy remain intact. No labels or approvals are changed by this job.
Execution blocked on missing Environment configuration. The worker stops after PR publication under the task's missing-configuration rule. The established review-and-merge bot owns review and merge. After configuration and merge, resume with a default-branch
dry_run=truedispatch, inspect exact-two-removal evidence, then and only then dispatchdry_run=falseand verify the masked before/after policy diff. No dispatch from an unmerged branch.