fix(#6257): remove credential mode model, make inference secrets unconditional - #6258
Conversation
|
🤖 Finished Review · ✅ Success · Started 12:57 PM UTC · Completed 1:13 PM UTC Commit: |
Site previewPreview: https://792f110d-site.fullsend-ai.workers.dev Commit: |
ReviewFindingsMedium
Low
Next steps:
Previous runReviewFindingsHigh
Medium
Low
Labels: PR modifies repos install/manifest subsystem and scaffold templates with mint mode changes Next steps:
Previous run (2)ReviewFindingsMedium
Low
Previous run (3)ReviewFindingsLow
Previous run (4)ReviewFindingsLow
Labels: PR modifies repos install/manifest subsystem, updates documentation, and fixes a bug in inference secret provisioning Previous run (5)ReviewFindingsCritical
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.
|
All review findings have been addressed in the squashed commit: Critical
Medium
Low
|
af21633 to
a666a40
Compare
|
🤖 Review · ❌ Terminated · Started 2:38 PM UTC · Ended 2:57 PM UTC Commit: |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Superseded by updated review
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
ready-for-merge
|
🤖 Finished Review · ✅ Success · Started 2:38 PM UTC · Completed 2:57 PM UTC Commit: |
a666a40 to
cc5c97d
Compare
|
🤖 Finished Review · ✅ Success · Started 3:06 PM UTC · Completed 3:21 PM UTC Commit: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
cc5c97d to
1a687ab
Compare
|
🤖 Finished Review · ✅ Success · Started 8:29 PM UTC · Completed 8:50 PM UTC Commit: |
fullsend-ai-review
Bot
added
requires-manual-review
Response to review findings on 1a687abContext: no breaking changesThe Medium findings
Low findings
|
1a687ab to
842d15e
Compare
|
🤖 Finished Review · ✅ Success · Started 10:26 PM UTC · Completed 10:45 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/mint
Review findings evaluation — 842d15eNot actionable (repos is UNRELEASED — no breaking changes, no deployed installs):
Fixed:
|
Remove the dead credential_mode/WIF bot-token code path from repos management (Part A of #6257) and add MintMode (public/private) to the manifest model (Part B). Part A — credential_mode removal: - Remove CredentialMode from GitHubForgeInfra, GitLabForgeInfra, RepoEntry, ResolvedConfig, and InstallConfig structs - Remove credential_mode YAML parsing, validation, and CLI flags - Remove WIF bot-token Secret Manager code from repos_gitlab.go - Remove FULLSEND_CREDENTIAL_MODE from uninstall variable lists - Update GitLab scaffold templates to use FULLSEND_FORGE_TOKEN only - Update ADR-0067 to reflect simplified credential model Part B — mint_mode model: - Add MintModePublic/MintModePrivate constants and DefaultPublicMintURL - Add MintMode field to GitHubForgeInfra (forge default) and RepoEntry (per-repo override) with NullableString 3-level fallback - Add cross-field validation: private requires explicit mint_url, public rejects null mint_url, invalid values rejected - Restrict mint_mode/mint_url to GitHub repos only - Default migrate.go inference region to config.DefaultPerRepoInferenceRegion ("global") when not discovered Signed-off-by: Greg Allen <greg@fullsend.ai> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Greg Allen <gallen@redhat.com>
842d15e to
df491ae
Compare
All findings evaluated — not actionable. repos is unreleased (no backward compatibility concerns, no deployed installs, no token-mode GitLab repos). DefaultPublicMintURL vs DefaultPerRepoMintURL divergence is intentional architecture. See evaluation: #6258 (comment)
|
🤖 Finished Review · ✅ Success · Started 10:54 PM UTC · Completed 11:33 PM 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:
internal/repos/manifest.go:41: [medium] logic-error
DefaultPublicMintURL is set to https://mint.fullsend.sh while DefaultPerRepoMintURL (in internal/config/defaults.go) and DefaultMintURL (in internal/cli/admin.go) are both set to https://fullsend-mint-gljhbkcloq-uc.a.run.app. These constants represent the default hosted mint URL but diverge, causing repos install and admin install to write different FULLSEND_MINT_URL values.
Suggested fix: Unify all three constants to the same value — either all mint.fullsend.sh or all the Cloud Run URL.
internal/cli/admin.go:206: [medium] logic-error
validateMintURL() now rejects any host not ending in .run.app or .cloudfunctions.net. The documented public mint URL https://mint.fullsend.sh would be rejected if a user explicitly passes it as --mint-url.
Suggested fix: Re-add mint.fullsend.sh to the validation allowlist, or update all documentation and DefaultPublicMintURL to use the Cloud Run URL exclusively.
internal/repos/uninstall.go:14: [medium] incomplete-cleanup
FULLSEND_CREDENTIAL_MODE was removed from both uninstallVariables and gitlabUninstallVars. Repos previously installed with this variable will retain it as an orphaned CI/CD variable. Other legacy variables are kept for cleanup but CREDENTIAL_MODE was not.
Suggested fix: Keep FULLSEND_CREDENTIAL_MODE in both uninstall lists for legacy cleanup.
internal/repos/manifest.go:74: [medium] backward-compatibility
Existing repos.yaml manifests with credential_mode at the forge level will fail to parse due to KnownFields(true). The field was removed from the struct and the YAML strict decoder will reject it.
Suggested fix: Add credential_mode back as an ignored field with yaml tag, or use a custom unmarshaler that silently drops it.
internal/repos/install.go:176: [low] edge-case
Install() allows GitHub repos without inference secrets when InferenceProject is empty, but checkInstallComponents always checks for inference secrets. Mitigated by batch_install pre-validation but could surface via direct CLI invocation.
internal/repos/install.go(file-level): Line 262 · [low] variable-contract
Inference secrets are now unconditionally required. Authorized by issue #6257, but is a breaking change for repos previously installed in OIDC/token mode without inference.
internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-agent.yml:41: [low] security-posture-degradation
Removing WIF-mode bot token retrieval eliminates defense-in-depth against CI_DEBUG_TRACE-based PAT exposure. CI_DEBUG_TRACE guard is now sole defense. ADR-0067 security table updated to acknowledge this.
internal/repos/status.go(file-level): Line 30 · [low] exported-type-field-removal
RepoStatus struct dropped CredentialMode and ExpectedCredentialMode JSON fields, changing repos status JSON output shape. CLI JSON output may be consumed by scripts.
All findings dismissed — repos is unreleased (no backward compatibility, no breaking changes, no existing installs). credential_mode removal is intentional with no compat path needed. DefaultPublicMintURL vs DefaultPerRepoMintURL divergence is intentional architecture. admin.go/github setup changes are out of scope for this evaluation. See prior evaluation: #6258 (comment)
Review findings evaluation — df491ae (round 2)All findings dismissed — same issues as prior round, all not actionable. Key context the review agent keeps missing:
No code changes made this round. |
|
🤖 Finished Retro · ✅ Success · Started 11:49 PM UTC · Completed 12:09 AM UTC Commit: |
Retro: PR #6258 — Remove credential mode modelPR #6258 (fixing #6257) removed the credential mode model from What went well
Improvement areas (all with existing issue coverage)1. Repeated dismissed findings — Reviews #5–#7 repeated the same HIGH/MEDIUM findings (DefaultPublicMintURL divergence, backward compatibility, incomplete cleanup) after the human explicitly dismissed them with detailed rationale in 4 separate comments. This cost ~1 hour of human time writing dismissal responses. Evidence supports the existing cluster: fullsend#2816 (umbrella design issue for finding cache/dedup), fullsend#1672 (honor author dismissals), fullsend#5265 (re-review anchoring with author response context), agents#721 (content-based stable IDs for cross-round dedup), agents#685 (resolve prior findings on re-review). 2. Wasted fix dispatches — Three fix agent runs were dispatched by the review agent's CHANGES_REQUESTED/DISMISSED verdicts but all failed identically at the human-authored PR guard. Each spawned a GitHub Actions runner only to exit immediately. Evidence supports fullsend#5811 (move the human-authored guard to the dispatch routing level to avoid spawning runners). 3. Bot-authored PR classified as human-authored — PR #6258 was opened by 4. Breaking-change false positives on undeployed code — The review agent flagged backward-compatibility and breaking-change concerns for Novel finding → proposalThe repo's AGENTS.md has no subsystem deployment-status annotations. The review agent cannot distinguish modifications to shipped subsystems (where backward compatibility matters) from modifications to undeployed subsystems (where it does not). Adding deployment-status context to AGENTS.md would give the review agent the information it needs to calibrate findings today, without waiting for agent-layer improvements. Proposals filed
|
1 participant
Summary
Removes the credential mode model (wif/oidc/token) introduced in PR #6224 and makes inference WIF secrets unconditional. The core bug:
needsWIFwas gated oncredMode == CredModeWIF, so GitHub repos using the default OIDC mode silently skipped writingFULLSEND_GCP_PROJECT_IDandFULLSEND_GCP_WIF_PROVIDERsecrets — breaking Vertex AI inference for default-mode repos.Changes
CredModeWIF/CredModeOIDC/CredModeTokenconstants,CredentialModefields fromGitHubForgeInfra,GitLabForgeInfra,RepoEntry, andResolvedConfig; remove validation functions (IsValidCredentialMode,ValidCredentialModesFor)resolveCredentialMode(), make inference secrets unconditional (written whenInferenceProjectis set), removeFULLSEND_CREDENTIAL_MODEvariable from installs, simplifyrequiredSecretsForForge()to always return secrets--inference-project"global"when not discovered (instead of warning with no value)fullsend-agent.ymlandfullsend-poll.yml; bot PAT always fromFULLSEND_FORGE_TOKENCI/CD variable; inference WIF setup unconditional whenFULLSEND_GCP_WIF_PROVIDERis setsetupGitLabBotTokento always store as CI/CD variable (remove Secret Manager path)Testing
internal/repos/tests pass (169 tests)internal/scaffold/tests passgo build ./...)TestPostScriptEnv_NoSchemaAppendedWhenNoValidationLoop— missingopenshellbinary in sandbox)Closes #6257
Post-script verification
agent/6257-remove-credential-mode)53456d472dcba81bcea79f8d715e9d6159cefa5c..HEAD)