feat(#4976): store mint and inference settings in per-repo config - #5976
Conversation
|
🤖 Finished Review · ✅ Success · Started 12:13 PM UTC · Completed 12:28 PM UTC |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
ReviewFindingsLow
Previous runReviewFindingsMedium
Low
Previous run (2)ReviewFindingsLow
Info
Previous run (3)ReviewFindingsMedium
Low
Previous run (4)ReviewFindingsLow
Previous run (5)ReviewFindingsMedium
Low
Previous run (6)ReviewFindingsCritical
Medium
Low
Next steps:
Previous run (7)ReviewFindingsMedium
Low
Labels: PR adds mint and inference settings to per-repo config schema Next steps:
Previous run (8)ReviewFindingsMedium
Low
Labels: PR modifies per-repo config schema and CLI setup for mint/inference settings |
|
/fs-fix lets try to group the inference setting in YAML under a single "inference" top level key and add a "type" filed that defaults to and may contain only "vertex" for now but may contain other types of inference providers in the future also deal with the AI review comments |
|
🤖 Finished Fix · ✅ Success · Started 8:19 AM UTC · Completed 8:37 AM UTC Commit: |
Restructure per-repo inference config from flat YAML keys (inference_provider, inference_project, etc.) to a nested inference block (inference.type, inference.project, etc.). Add --inference-type CLI flag defaulting to "vertex" to replace the hard-coded provider value. Fix comment verbosity and update IsPerRepoYAML to allow shared inference key. Addresses human review feedback and bot review findings on #5976
🔧 Fix agent — iteration 1 (human-triggered)Restructured per-repo inference config from flat YAML keys to a nested inference: block with type/project/region/wif_provider fields. Added --inference-type CLI flag (default: vertex) replacing the hard-coded provider value. Fixed comment style issues. Disagreed with 5 doc/informational findings as out of scope. Fixed (7):
Disagreed (5):
Tests: passedNext steps:
|
|
🤖 Finished Review · ✅ Success · Started 8:38 AM UTC · Completed 8:54 AM UTC Commit: |
|
/fs-fix this is a user-visible config change, docs are in scope! also deal with other new AI comments |
|
/fs-fix per review comments: use "provider" instead of "type" for inference; do not write values to variables but keep the reads in place for backwards compatibility; Deal with other comments you did not deal with yet as well. |
|
🤖 Finished Fix · ✅ Success · Started 5:54 AM UTC · Completed 6:13 AM UTC Commit: |
🔧 Fix agent — iteration 2 (human-triggered)Addressed 4 of 8 review findings with code changes: renamed inference Type to Provider for naming coherence with org-mode config, removed variable/secret writes (values now in config.yaml only), added Validate() call for preset overlay path, and fixed setter parameter names. Disagreed with 4 informational/already-addressed findings. Fixed (4):
Disagreed (4):
Tests: passed Decision points
Next steps:
|
Address review feedback on PR #5976: - Rename PerRepoInferenceConfig.Type to Provider (YAML: provider) to align with org-mode InferenceConfig naming (review: naming-coherence, human instruction: use "provider" instead of "type") - Remove FULLSEND_MINT_URL, FULLSEND_GCP_REGION variable writes and FULLSEND_GCP_PROJECT_ID, FULLSEND_GCP_WIF_PROVIDER secret writes from per-repo setup; values are now stored in config.yaml only (human instruction: do not write values to variables) - Keep secret/variable existence reads for backward compatibility - Add Validate() call on preset overlay before Marshal(), matching the non-preset code path (review: error-handling-gap) - Use full-word parameter names in inference setters instead of single letters (review: setter-style) - Rename --inference-type flag to --inference-provider Addresses review feedback on #5976
|
🤖 Finished Review · ✅ Success · Started 6:14 AM UTC · Completed 6:36 AM UTC Commit: |
|
/fs-fix per review comments: restore dual-write of mint/inference repo variables/secrets alongside config.yaml (undo the attempt to stop writing them — workflows still require those vars until #5870/#4977). Keep reading existing secrets for backward compatibility. Address the other open AI review findings as well. |
|
🤖 Finished Fix · ✅ Success · Started 11:18 AM UTC · Completed 11:27 AM UTC Commit: |
Re-add writing FULLSEND_MINT_URL and FULLSEND_GCP_REGION as repo variables, and FULLSEND_GCP_PROJECT_ID / FULLSEND_GCP_WIF_PROVIDER as repo secrets during per-repo setup. Existing workflow templates still reference these vars/secrets until #5870 / #4977 migrate reads to config.yaml. Reading existing secrets for backward compatibility is preserved via the reuseProject / reuseWIF checks. Update tests to assert dual-write behavior: variables and secrets are set alongside config.yaml values, partial-reuse tests verify only the non-reused secret is written. Addresses review feedback on #5976
🔧 Fix agent — iteration 3 (human-triggered)Restored dual-write of mint/inference repo variables and secrets alongside config.yaml per human instruction. This fixes the critical logic-error and scope-violation findings - workflow templates that reference vars/secrets will continue to receive values. The reuseProject/reuseWIF dead-code finding is also resolved. Updated 4 test functions to assert dual-write behavior. Disagreed with 4 low-severity informational findings as pre-existing patterns or out-of-scope. Fixed (4):
Disagreed (4):
Tests: passedNext steps:
|
|
🤖 Finished Review · ✅ Success · Started 11:28 AM UTC · Completed 11:46 AM UTC Commit: |
|
/fs-fix Fix the spelling nit only: in Disagree/close (already handled — do not reopen):
Do not change behavior beyond the spelling fix. |
|
🤖 Finished Fix · ✅ Success · Started 8:02 AM UTC · Completed 8:08 AM UTC Commit: |
Change British "initialises" to American "initializes" in the ensureInference doc comment to match codebase convention. Addresses review feedback on #5976
🔧 Fix agent — iteration 6 (human-triggered)Fixed spelling nit (initialises → initializes) per human instruction. Disagreed with 4 prior review findings: all are either known gaps tracked in existing issues, intentional design decisions, or out of scope for this config-plumbing PR. Fixed (1):
Disagreed (4):
Tests: passedNext steps:
|
|
🤖 Finished Review · ✅ Success · Started 8:09 AM UTC · Completed 8:26 AM UTC Commit: |
|
/fs-fix Fix the medium stale-reference finding only: in Do not reopen or change code for previously closed findings (data-completeness, detection-robustness, Input Validation, spelling, dual-write). |
|
🤖 Finished Fix · ✅ Success · Started 8:38 AM UTC · Completed 8:45 AM UTC Commit: |
The comment in TestSetupStatusNotifier_PerRepoConfigYAML listed "inference" as an org-only IsPerRepoYAML detection key, but inference is now a shared key used in per-repo configs too. Update the comment to match the current detection logic which only checks "defaults", "dispatch", and "repos". Addresses review feedback on #5976
🔧 Fix agent — iteration 7 (human-triggered)Fixed the stale-reference finding: removed "inference" from the org-only detection key comment in run_test.go to match current IsPerRepoYAML logic. Fixed (1):
Tests: passedNext steps:
|
|
🤖 Finished Review · ✅ Success · Started 8:46 AM UTC · Completed 9:00 AM UTC Commit: |
|
🤖 Finished Retro · ✅ Success · Started 10:47 AM UTC · Completed 11:04 AM UTC Commit: |
Retro: PR #5976 — store mint and inference settings in per-repo configWorkflow overview: Issue #4976 was triaged (5 triage runs), coded (1 code run producing the initial PR), then went through 8 review/fix cycles over 5 days (Aug 6–11) before ifireball merged it. The PR added +1054/−26 lines across 11 files implementing ADR 0069 Decision 1. What went well
Rework analysisOf the 9 fix iterations, the root causes break down as:
The costliest rework was the Fix 3 → Fix 4 ping-pong: the human instructed the agent to remove dual-write of repo variables, contradicting the issue's explicit requirement to preserve them. The fix agent followed the instruction, the review agent caught the scope violation, and a second fix was needed to undo the change. This wasted one full cycle (~30 min of agent compute + human review time). Existing issues covering observed patterns
Autonomy assessmentThe review agent demonstrated strong bug-catching capability (the critical dual-write finding alone justified its involvement). However, the human's primary value-add was design-level decisions — YAML structure, naming conventions, defaults layer architecture — that the review agent cannot replicate. The current collaborative model (agent reviews + human design oversight) is appropriate for this class of feature work. Proposals filed
|
Summary
mint_url,inference_provider,inference_project,inference_region, andinference_wif_providerfields to per-repo config schema (ADR 0069 Decision 1)ConfigMintURL(),ConfigInferenceProvider(), etc.) onPerRepoConfigReaderwith layered fallback (overlay → base → code defaults) and corresponding setters onPerRepoConfigWriterfullsend github setupto populate mint/inference values in config: flag values go into config.yaml (no preset) or overlay (with preset), while the base layer stays identical to the fetched presetDesign
Schema fields: Flat YAML keys on
perRepoConfig(mint_url,inference_provider,inference_project,inference_region,inference_wif_provider). Field names are prefixed to avoid colliding with the org-mode detection keys inIsPerRepoYAML(dispatch,inference).Accessor pattern: Follows the existing scalar fallback convention (e.g.,
ConfigRuntime(),ConfigForge()): local value → parent → empty string. Code defaults inperRepoDefaultsreturn""for all mint/inference fields — callers that need a fallback (e.g., the CLI) useDefaultMintURL.Installer wiring: When
--config(preset) is provided, flag-specified values go into the overlay; unspecified flags inherit from the base layer via the accessor chain. ThechangedFlagsmap records which CLI flags were explicitly set.Backward compatibility: Existing repo variable/secret writes (
FULLSEND_MINT_URL,FULLSEND_GCP_REGION,FULLSEND_GCP_PROJECT_ID,FULLSEND_GCP_WIF_PROVIDER) are preserved so existing workflow templates continue to read from vars/secrets when config fields are absent.Testing
IsPerRepoYAMLcorrectly identifies config with new fields as per-repo (not org)buildPresetOverlaycorrectly writes only flag-changed valuesinference_providervaluesCloses #4976
Post-script verification
agent/4976-mint-inference-config)fac93d227b4e30eaa553b16d059eac6624ec9fc2..HEAD)