feat(#5780)!: per-repo manifest overrides and install-time GCP creds - #5781
Conversation
|
🤖 Finished Review · ✅ Success · Started 1:52 AM UTC · Completed 2:09 AM UTC |
ReviewFindingsLow
Labels: PR modifies manifest schema, install-time credential handling, and sync behavior — a breaking-change feature across internal/repos/ and internal/cli/ Previous runReviewFindingsHigh
Medium
Low
Next steps:
Previous run (2)ReviewFindingsLow
Previous run (3)ReviewFindingsLow
Previous run (4)ReviewFindingsMedium
Low
Labels: PR modifies repos install/sync/init infrastructure and manifest schema under internal/repos/ and internal/cli/ Previous run (5)ReviewFindingsMedium
Low
Labels: PR modifies repos install/sync/init infrastructure and manifest schema under internal/repos/ and internal/cli/ Next steps:
Previous run (6)ReviewFindingsHigh
Medium
Low
Labels: PR modifies repos install/sync/init infrastructure and manifest schema under internal/repos/ and internal/cli/ Next steps:
|
67c2a97 to
7deeb28
Compare
|
/fs-review |
|
🤖 Review · ❌ Terminated · Started 2:25 AM UTC · Ended 2:42 AM UTC |
Site previewPreview: https://70bc3378-site.fullsend-ai.workers.dev Commit: |
|
🤖 Finished Review · ✅ Success · Started 2:25 AM UTC · Completed 2:42 AM UTC |
7deeb28 to
22172eb
Compare
|
/fs-review |
|
🤖 Review · ❌ Terminated · Started 2:51 AM UTC · Ended 3:07 AM UTC |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Superseded by updated review
|
🤖 Finished Review · ✅ Success · Started 2:51 AM UTC · Completed 3:07 AM UTC |
22172eb to
52caa22
Compare
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 3:12 AM UTC · Completed 3:30 AM UTC |
|
🤖 Finished Review · ✅ Success · Started 9:42 AM UTC · Completed 10:00 AM UTC |
0ca6497 to
5b71f28
Compare
|
/fs-review |
|
🤖 Review · |
5b71f28 to
1f3c61c
Compare
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 10:11 AM UTC · Completed 10:31 AM UTC |
Add 4 per-repo override fields to RepoEntry using the existing NullableString + resolveField 3-level fallback pattern: - inference_region, fullsend_ref, mint_url (NullableString) - allowed_remote_resources ([]string, nil = inherit) Move inference_project and inference_project_number out of the manifest into install-time-only CLI flags on `repos install` and `repos add --install`. These sensitive GCP values are written as repo secrets at install time but are no longer stored in repos.yaml or reconciled by `repos sync`. BREAKING CHANGE: `repos install` now requires `--inference-project` and `--inference-project-number` CLI flags. `repos sync` no longer reconciles secrets. Changes: - manifest.go: Add override fields to RepoEntry, update UnmarshalYAML to accept them, update resolveWithEntry to use resolveField for per-repo fallback chain. Remove inference_project_number from Validate() requirements. Add MarshalYAML for round-trip correctness. Add IsNumeric helper. Add per-repo mint_url HTTPS validation. Add per-repo fullsend_ref IsValidRef validation. Add IsValidGCPProjectID helper. - sync.go: Empty managedSecrets — sync only reconciles variables going forward. - batch_install.go: Add InferenceProject/InferenceProjectNumber to BatchInstallConfig, source from CLI flags not manifest. Add numeric validation for InferenceProjectNumber. Add GCP project ID format validation for InferenceProject. - cli/repos.go: Add --inference-region, --fullsend-ref, --mint-url, --allowed-remote-resources to repos add. Add --inference-project, --inference-project-number to repos install. Wire values into entries and BatchInstallConfig. Guard cross-forge override comparisons behind forge type check. Add --fullsend-ref IsValidRef and --mint-url HTTPS validation in runReposAdd. Add --inference-project and --inference-project-number CLI-layer validation in runReposInstall. Update repos sync Long description to remove secret references. - init.go: Remove InferenceProject/Number from InitConfig and buildManifest forge section. Generate per-repo overrides where discovered values differ from the forge-level mode. - manifest_edit.go: Populate per-repo overrides from probed state in AddToManifest when values differ from defaults. - Tests updated across all affected files. - Docs updated: repos.md, cli-internals.md, operations.md, repo-management.md. Closes #5780 Signed-off-by: Greg Allen <gallen@redhat.com> Signed-off-by: Claude <noreply@anthropic.com> Signed-off-by: Greg Allen <gallen@redhat.com>
1f3c61c to
c1f8be6
Compare
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 10:40 AM UTC · Completed 10:58 AM UTC |
Superseded by updated review
There was a problem hiding this comment.
Note: The following inline comments could not be posted on the diff (GitHub returned 422) and are included here instead:
internal/cli/repos.go:383: [low] cli-interface-change
repos install now requires --inference-project and --inference-project-number CLI flags for GitHub repos. repos init no longer accepts these flags. Breaking change correctly marked with ! in the PR title.
internal/repos/init.go(file-level): Line 34 · [low] scope-gap
Issue #5780 scope item 5 claims all 4 override fields are auto-discoverable, but allowed_remote_resources discovery from .fullsend/config.yaml is not implemented. The field works via manual override (repos add --allowed-remote-resources).
internal/repos/sync.go:55: [low] behavioral-contract-change
managedSecrets is intentionally empty — sync no longer reconciles secrets. Intentional per issue #5780.
internal/repos/manifest.go(file-level): Line 593 · [low] documentation-comment
Comment groups truly deprecated fields (MintProject, MintRegion) with fields moved to CLI flags (InferenceProject, InferenceProjectNumber) under one 'Deprecated' label. All are deprecated in the manifest, but InferenceProject/InferenceProjectNumber are still used at install time.
|
🤖 Finished Retro · ✅ Success · Started 6:12 PM UTC · Completed 6:30 PM UTC |
Retro Analysis: PR #5781 — per-repo manifest overrides and install-time GCP credsWorkflow OverviewPR #5781 was a breaking-change feature PR (
Total dispatch runs: 16 (3 triage + 1 code + 9 review + 3 fix), plus ~29 CI runs on the PR branch. Agents repo: What Went Well
Issues Found (All Covered by Existing Open Issues)1. Fix agent eligibility failures — evidence for #5536 2. Acknowledged findings re-raised every iteration — evidence for #5265 3. Scope expansion after approval not flagged — evidence for #925 4. Non-deterministic finding coverage — evidence for #1389 No Novel ProposalsAll identified improvement opportunities are covered by existing open issues. The most impactful fix for this workflow would be #5536, which would have allowed the fix agent to assist with the manual fixes the human had to make across 7+ commits. |
Summary
Add per-repo manifest overrides for infrastructure fields (inference_region, fullsend_ref, mint_url, allowed_remote_resources) using the existing NullableString + resolveField 3-level fallback pattern. Move GCP project ID and project number from the manifest into install-time-only CLI flags, and stop managing secrets in
repos sync.Related Issue
Closes #5780
Changes
RepoEntry(3NullableString+ 1[]string), un-deprecateinference_regionandfullsend_refinUnmarshalYAML, updateresolveWithEntryto useresolveFieldfor the new fieldsinference_projectandinference_project_numberout ofGitHubForgeInfrainto CLI flags onrepos installandrepos add --install; removeInferenceProjectNumberrequirement fromValidate()managedSecrets— sync only reconciles variables (FULLSEND_MINT_URL,FULLSEND_GCP_REGION)--inference-region,--fullsend-ref,--mint-url,--allowed-remote-resourcestorepos add; add--inference-project,--inference-project-numbertorepos installbuildManifestgenerates per-repo overrides where discovered values differ from the forge-level mode (mint_url, inference_region, fullsend_ref)AddToManifestpopulates per-repo overrides from probed repo state when values differ from manifest defaultsresolveWithEntry— no changes neededTesting
Checklist
!for breaking changes)Closes #5780
Post-script verification
agent/5780-per-repo-overrides)6df635a109f4951505ae2d7399e7d52fe15f4518..HEAD)