feat(#6226): gate WIF secrets in per-repo shim by credential mode - #6241
feat(#6226): gate WIF secrets in per-repo shim by credential mode#6241fullsend-ai-coder[bot] wants to merge 1 commit into
Conversation
|
🤖 Finished Review · ✅ Success · Started 9:52 PM UTC · Completed 10:11 PM UTC Commit: |
Site previewPreview: https://ce4501f2-site.fullsend-ai.workers.dev Commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
ReviewFindingsMedium
Low
Next steps:
Previous runReviewFindingsHigh
Medium
Low
Next steps:
Previous run (2)ReviewFindingsHigh
Medium
Low
Labels: PR modifies scaffold/install code, dispatch workflow, and documentation Next steps:
Previous run (3)ReviewFindingsHigh
Medium
Low
Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
e3ac5c2 to
e188658
Compare
|
🤖 Finished Review · ✅ Success · Started 10:54 PM UTC · Completed 11:14 PM UTC Commit: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
component/install
e188658 to
b300bb1
Compare
|
🤖 Finished Review · ✅ Success · Started 12:42 AM UTC · Completed 1:22 AM UTC Commit: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Signed-off-by: Greg Allen <gallen@redhat.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Greg Allen <gallen@redhat.com>
b300bb1 to
8ac981b
Compare
|
🤖 Finished Review · ✅ Success · Started 1:39 AM UTC · Completed 2:00 AM UTC Commit: |
There was a problem hiding this comment.
See the review comment for full details.
Note: The following inline comments could not be posted on the diff (GitHub returned 422) and are included here instead:
.github/workflows/reusable-dispatch.yml(file-level): Line 626 · [medium] API contract violation
The setup-gcp composite action is called unconditionally in every stage job, and its action.yml declares gcp_wif_provider as required: true. For OIDC-mode repos, stripWIFSecrets removes FULLSEND_GCP_WIF_PROVIDER from the generated shim, so the secret reaches reusable-dispatch.yml as an empty string. google-github-actions/auth will fail with an empty workload_identity_provider, aborting the stage job. This is a known-deferred item (issue #6226 item 2) and is fail-closed, but the PR claims Closes #6226 while this item remains unaddressed.
Suggested fix: Either add an if condition to each setup-gcp step (e.g., if: secrets.FULLSEND_GCP_WIF_PROVIDER != ''), or change Closes #6226 to a partial reference and file a follow-up for the setup-gcp conditional.
.github/workflows/reusable-dispatch.yml:66: [medium] protected-path
This PR modifies a file under the .github/ protected path. The change is authorized by issue #6226 item 3 (change FULLSEND_GCP_PROJECT_ID to required: false). Human approval is required for all protected-path changes regardless of context.
.github/workflows/reusable-dispatch.yml:66: [low] secret-requirement-relaxation
FULLSEND_GCP_PROJECT_ID changed from required: true to required: false. Combined with FULLSEND_GCP_WIF_PROVIDER already being required: false, the reusable workflow now accepts calls with no GCP credentials. This is fail-closed in practice (setup-gcp still rejects empty values), but a YAML comment explaining why these secrets are optional would aid future maintainers.
internal/scaffold/installfiles.go:49: [low] API-shape-consistency
CollectPerRepoInstallFiles takes 4 positional parameters (bool + 3 strings), while its sibling CollectInstallFiles uses a CollectInstallFilesOptions struct. The new credentialMode parameter deepens this divergence.
internal/repos/remote_scaffold.go:34: [low] growing-positional-parameter-list
FetchRemoteScaffold now accepts 7 parameters including 4 consecutive strings, which is error-prone at call sites.
internal/scaffold/render.go:120: [low] magic-string-vs-constant
stripWIFSecrets compares against the string literal oidc rather than a named constant. The CredModeOIDC constant in the repos package cannot be imported due to circular dependency; consider a package-local constant.
ifireball
left a comment
ifireball
left a comment
There was a problem hiding this comment.
Tihis PR is wrong and should not be merged,
FULLSEND_GCP_WIF_PROVIDER, FULLSEND_GCP_PROJECT_ID are required for WIF authentication for GCP Vertex inference and has nothing to do with the mint.
The confusion must be a result of the move to the public mint that no longer requires enrolment at the mint level, but that change does not imply or require any changes at the inference infrastructure and the credetials used for it.
|
🤖 Finished Retro · ✅ Success · Started 12:25 PM UTC · Completed 12:36 PM UTC Commit: |
Retro: PR #6241 — gate WIF secrets in per-repo shim by credential modeOutcome: PR closed without merging. The entire credential-mode approach was architecturally unsound — WIF secrets are required for GCP Vertex inference regardless of mint mode, so stripping them from the shim would break inference. Timeline
Key Findings1. Review agent altitude gap — design-level premise missed across 4 rounds The review agent performed well at implementation review: it found real bugs (API contract violation between However, across all 4 review rounds, the agent never questioned the fundamental premise — that OIDC-mode repos don't need WIF secrets. It accepted the PR's assumption at face value and reviewed only whether the implementation was internally consistent with that assumption. Notably, the agent's stale-documentation findings (recommending docs say WIF secrets are "WIF mode only") would have introduced factual errors if acted upon, because the agent had internalized the flawed premise. The human reviewer (
2. Code agent ignored triage permission warnings The code agent's run logs show it explicitly acknowledged the triage warning about This evidence corroborates existing open issues:
3. Review agent did not incorporate scope reductions from human instructions In later review rounds, the review agent continued flagging "incomplete scope" (item 2 of #6226 — conditionally skip This evidence corroborates fullsend-ai/agents#447 — review agent should incorporate outstanding human reviews when re-reviewing. Autonomy AssessmentThe review agent's implementation-level findings were consistently valid and actionable — 12 of 13 findings were real issues. The code agent acted on most of them. This demonstrates strong value at the code-review altitude. However, the missed design-level flaw — which rendered all implementation work moot — means increased autonomy is not warranted for PRs involving infrastructure credential configuration or architecture-level changes. The agent cannot substitute for human domain judgment on whether a feature's premise is correct. For straightforward implementation PRs within well-understood subsystems, the agent's review quality supports its current autonomy level. Waste ImpactTotal wasted effort: 2 code agent runs, 4 review agent runs, multiple human iteration cycles, all producing a PR that was discarded entirely. The most impactful improvements would be (a) the review agent questioning feature premises (existing issues #5385, #1535, #543) and (b) the code agent respecting permission boundaries (existing issues #3627, #774). ProposalsAll candidate improvements are covered by existing open issues (referenced above). No new proposals filed. |
2 participants
Summary
FULLSEND_GCP_WIF_PROVIDER,FULLSEND_GCP_PROJECT_ID) in the per-repo shim workflow template by credential mode — OIDC repos no longer get confusing WIF secret references in their generated.github/workflows/fullsend.yamlCredentialModeto scaffoldRenderOptionsand thread it throughCollectPerRepoInstallFiles,BuildScaffoldFiles, andFetchRemoteScaffoldFULLSEND_GCP_PROJECT_IDoptional (required: false) inreusable-dispatch.ymlso OIDC-mode shims don't fail validationwif/oidc/token) in the mint administration guideChanges
internal/scaffold/render.go: AddCredentialModefield toRenderOptions; addstripWIFSecrets()that removes WIF secret lines when credential mode isoidcinternal/scaffold/installfiles.go: AddcredentialModeparameter toCollectPerRepoInstallFilesinternal/repos/install.go: Resolve credential mode viaresolveCredentialMode()inBuildScaffoldFilesbefore passing to scaffold layerinternal/repos/remote_scaffold.go: ThreadcredentialModethroughFetchRemoteScaffoldandfetchRemoteGitHubScaffoldinternal/repos/batch_install.go: Pass resolved credential mode toFetchRemoteScaffoldinternal/cli/github.go: Pass empty credential mode (backward compat for org-mode setup).github/workflows/reusable-dispatch.yml: ChangeFULLSEND_GCP_PROJECT_IDtorequired: falsedocs/guides/infrastructure/mint-administration.md: Add "Credential modes" sectionTesting
TestRenderPerRepoShimWIFMode— verifies WIF secrets present when credential mode iswifTestRenderPerRepoShimOIDCMode— verifies WIF secrets stripped and OTEL secrets preserved when credential mode isoidcTestRenderPerRepoShimDefaultCredMode— verifies empty credential mode defaults to including WIF secretsTestCollectPerRepoInstallFiles_OIDCModeandTestCollectPerRepoInstallFiles_WIFMode— end-to-end tests through install file collectionTestFetchRemoteScaffold_GitHubOIDCMode— remote scaffold path strips WIF secrets in OIDC modeOut of scope
Upgrade SHA-pinning behavior change moved to a separate issue.
Closes #6226