Fix config precedence: piloop-config.yaml should outrank env vars (CLI > YAML > env > defaults) #282

New issue

david · 2026-08-19T00:33:05Z

david commented

2026-08-19 00:33:05 +00:00

Summary

pi-loop's configuration precedence is currently CLI flag > env var > YAML (piloop-config.yaml) > built-in defaults (ADR-013). Testing has shown this ordering is a mistake: env vars are meant to be a global fallback (machine/CI-wide), piloop-config.yaml is meant to be the project override (committed, PR-reviewed, team-shared), and CLI flags are the user's per-invocation override. The project override should rank above the global default, not below it.

Fix: change the precedence to:

CLI flag > piloop-config.yaml > env var > built-in defaults

CI/machine environments that need to force a value can use a CLI --flag (which stays top priority under both orderings) instead of relying on an ambient env var to override a committed YAML value.

Background

ADR-013 (docs/adr/013-piloop-config-yaml.md) explicitly considered and rejected this exact ordering under "Alternatives Considered":

"File as repo source of truth (CLI > YAML > env).” Rejected: CI/machine env could no longer override the file (e.g. force auto-merge in CI).

That tradeoff is no longer accepted — CI can force a value via --flag instead. This is being treated as correcting a testing-discovered mistake, not a recorded historical pivot:

ADR-013 should be edited in place (Decision, Rationale, Alternatives Considered sections) — no new ADR, no supersession note.
A changelog entry should still be added (CHANGELOG.md, ## Unreleased → ### Fixed) to warn CI operators that env-var overrides of piloop-config.yaml will stop working after this change.

Scope — settings affected

The reorder only has real effect on settings present in both piloop-config.yaml and an env var. Apply the new precedence uniformly across all of them (no per-key hybrid — mirrors ADR-013's own rejection of per-key hybrid precedence as unnecessary complexity):

Setting	YAML key	Env var
Model	`model`	`PILOOP_MODEL`
Thinking level	`thinking-level`	`PILOOP_THINKING_LEVEL`
Auto-merge enabled	`auto-merge.enabled`	`PILOOP_AUTO_MERGE`
Auto-merge timeout	`auto-merge.timeout-ms`	`PILOOP_AUTO_MERGE_TIMEOUT_MS`
Auto-merge poll	`auto-merge.poll-ms`	`PILOOP_AUTO_MERGE_POLL_MS`
Bash default timeout	`bash.default-timeout`	`PILOOP_BASH_DEFAULT_TIMEOUT_S`
Bash max timeout	`bash.max-timeout`	`PILOOP_BASH_MAX_TIMEOUT_S`
Jira AC field id	`jira.acceptance-criteria-field-id`	`JIRA_ACCEPTANCE_CRITERIA_FIELD_ID`
GitLab base URL	`gitlab.base-url`	`GITLAB_BASE_URL`
Forgejo base URL	`forgejo.base-url`	`FORGEJO_BASE_URL`

Not affected (confirmed via code search — no env-var counterpart exists):

source, git-host, label, max-issues (CLI/YAML-only settings)
docs: section (glob lists, no env-var counterpart)
Credentials/tokens and Jira email (env-only, never in YAML, per ADR-013 exclusion scope)

Implementation details

src/config/services/configLoader.ts — invert mergeEnvSources.
Currently:
```
function mergeEnvSources(env: EnvSource, yamlEnv: EnvSource): EnvSource {
  const merged: EnvSource = { ...yamlEnv };
  for (const [key, value] of Object.entries(env)) {
    if (normalize(value) !== undefined) {
      merged[key] = value;
    }
  }
  return merged;
}
```
This must invert to: YAML values win over env when YAML sets them (non-empty); env only fills keys YAML left unset. Update the associated docblocks (this function, the EnvSource type doc, and loadConfig's doc) which currently describe "env wins over yamlEnv" — they need rewriting to describe the new precedence.
src/cli/services/runCli.ts — rewrite modelSource detection (ADR-010 interaction).
Current logic:
```
const effectiveModel = parsedArgs!.model ?? config.model.model;
const modelFromYaml =
  configFile?.env.PILOOP_MODEL !== undefined &&
  (env.PILOOP_MODEL === undefined || env.PILOOP_MODEL.trim() === '');
const modelSource: ModelSource =
  parsedArgs!.model !== undefined ? 'flag' : modelFromYaml ? 'yaml' : 'env';
```
This assumes "YAML only wins when env is unset" (today's order). Once YAML outranks env, modelSource must report 'yaml' whenever YAML sets PILOOP_MODEL — regardless of whether env also sets it. Update the associated comment referencing ADR-010/ADR-013 precedence wording.

Docs to update

docs/adr/013-piloop-config-yaml.md — edit Decision, Rationale, and Alternatives Considered sections in place to reflect CLI > YAML > env > defaults. Remove/replace the now-obsolete "File as repo source of truth" rejected-alternative entry (it's now the chosen design) and any "12-factor precedence" / "CI/machine env able to override the repo baseline" rationale language.
docs/adr/010-per-run-model-override.md — check and update any wording describing YAML/env/flag precedence for --model.
docs/adr/014-research-stage.md, docs/adr/015-documentation-sync-stage.md — check for precedence mentions and update if present.
README.md, AGENTS.md, DESIGN.md, IMPLEMENTATION_PLAN.md — update the documented precedence chain (CLI flag > env var > YAML > built-in defaults → CLI flag > YAML > env var > built-in defaults).
piloop-config.example.yaml — check/update any precedence-related comments.
docs/design-auto-merge.md, docs/plan-B1-vertex-anthropic-provider.md — check for precedence mentions and update if present.

(.reviews/*.md are historical review notes — out of scope, do not touch.)

Changelog

Add a bullet to CHANGELOG.md under ## Unreleased → ### Fixed, e.g.:

Config precedence: piloop-config.yaml now outranks environment variables. Fixed the precedence order to CLI flag > piloop-config.yaml > env var > built-in defaults (previously CLI flag > env var > piloop-config.yaml > built-in defaults). A committed per-repo config is now correctly treated as the project's baseline/override, with env vars filling only the gaps it leaves unset. This is a behavior change: environments (including CI) that previously relied on an env var to override a value set in piloop-config.yaml must now use the equivalent CLI flag instead.

Test plan

src/config/services/configLoader.test.ts — update mergeEnvSources precedence test cases to assert YAML wins over env when both are set; keep/extend the case where env fills a gap YAML leaves unset.
src/cli/services/runCli.ts model-source tests — update to assert modelSource === 'yaml' when both YAML and env set PILOOP_MODEL (previously asserted 'env' in that case).
Full run: npm run lint && npm test must pass.

Acceptance criteria

mergeEnvSources inverted so YAML values win over env values in src/config/services/configLoader.ts; associated docblocks updated.
modelSource detection in src/cli/services/runCli.ts rewritten to report 'yaml' when both YAML and env set PILOOP_MODEL.
docs/adr/013-piloop-config-yaml.md edited in place (Decision/Rationale/Alternatives Considered) — no new ADR, no supersession note.
docs/adr/010-per-run-model-override.md, docs/adr/014-research-stage.md, docs/adr/015-documentation-sync-stage.md checked and updated if they reference the old precedence.
README.md, AGENTS.md, DESIGN.md, IMPLEMENTATION_PLAN.md, piloop-config.example.yaml updated to state the new precedence.
docs/design-auto-merge.md, docs/plan-B1-vertex-anthropic-provider.md checked and updated if applicable.
CHANGELOG.md updated with a new bullet under ## Unreleased → ### Fixed.
configLoader.test.ts and runCli.ts model-source tests updated to assert the new precedence.
npm run lint && npm test pass.

## Summary pi-loop's configuration precedence is currently `CLI flag > env var > YAML (piloop-config.yaml) > built-in defaults` (ADR-013). Testing has shown this ordering is a mistake: env vars are meant to be a *global* fallback (machine/CI-wide), `piloop-config.yaml` is meant to be the *project* override (committed, PR-reviewed, team-shared), and CLI flags are the *user's* per-invocation override. The project override should rank above the global default, not below it. **Fix:** change the precedence to: ``` CLI flag > piloop-config.yaml > env var > built-in defaults ``` CI/machine environments that need to force a value can use a CLI `--flag` (which stays top priority under both orderings) instead of relying on an ambient env var to override a committed YAML value. ## Background ADR-013 (`docs/adr/013-piloop-config-yaml.md`) explicitly considered and *rejected* this exact ordering under "Alternatives Considered": > **"File as repo source of truth (`CLI > YAML > env`).” Rejected: CI/machine env could no longer override the file (e.g. force auto-merge in CI).** That tradeoff is no longer accepted — CI can force a value via `--flag` instead. This is being treated as **correcting a testing-discovered mistake**, not a recorded historical pivot: - **ADR-013 should be edited in place** (Decision, Rationale, Alternatives Considered sections) — no new ADR, no supersession note. - **A changelog entry should still be added** (`CHANGELOG.md`, `## Unreleased` → `### Fixed`) to warn CI operators that env-var overrides of `piloop-config.yaml` will stop working after this change. ## Scope — settings affected The reorder only has real effect on settings present in **both** `piloop-config.yaml` and an env var. Apply the new precedence **uniformly** across all of them (no per-key hybrid — mirrors ADR-013's own rejection of per-key hybrid precedence as unnecessary complexity): | Setting | YAML key | Env var | |---|---|---| | Model | `model` | `PILOOP_MODEL` | | Thinking level | `thinking-level` | `PILOOP_THINKING_LEVEL` | | Auto-merge enabled | `auto-merge.enabled` | `PILOOP_AUTO_MERGE` | | Auto-merge timeout | `auto-merge.timeout-ms` | `PILOOP_AUTO_MERGE_TIMEOUT_MS` | | Auto-merge poll | `auto-merge.poll-ms` | `PILOOP_AUTO_MERGE_POLL_MS` | | Bash default timeout | `bash.default-timeout` | `PILOOP_BASH_DEFAULT_TIMEOUT_S` | | Bash max timeout | `bash.max-timeout` | `PILOOP_BASH_MAX_TIMEOUT_S` | | Jira AC field id | `jira.acceptance-criteria-field-id` | `JIRA_ACCEPTANCE_CRITERIA_FIELD_ID` | | GitLab base URL | `gitlab.base-url` | `GITLAB_BASE_URL` | | Forgejo base URL | `forgejo.base-url` | `FORGEJO_BASE_URL` | **Not affected** (confirmed via code search — no env-var counterpart exists): - `source`, `git-host`, `label`, `max-issues` (CLI/YAML-only settings) - `docs:` section (glob lists, no env-var counterpart) - Credentials/tokens and Jira email (env-only, never in YAML, per ADR-013 exclusion scope) ## Implementation details 1. **`src/config/services/configLoader.ts` — invert `mergeEnvSources`.** Currently: ```ts function mergeEnvSources(env: EnvSource, yamlEnv: EnvSource): EnvSource { const merged: EnvSource = { ...yamlEnv }; for (const [key, value] of Object.entries(env)) { if (normalize(value) !== undefined) { merged[key] = value; } } return merged; } ``` This must invert to: YAML values win over env when YAML sets them (non-empty); env only fills keys YAML left unset. Update the associated docblocks (this function, the `EnvSource` type doc, and `loadConfig`'s doc) which currently describe "env wins over yamlEnv" — they need rewriting to describe the new precedence. 2. **`src/cli/services/runCli.ts` — rewrite `modelSource` detection (ADR-010 interaction).** Current logic: ```ts const effectiveModel = parsedArgs!.model ?? config.model.model; const modelFromYaml = configFile?.env.PILOOP_MODEL !== undefined && (env.PILOOP_MODEL === undefined || env.PILOOP_MODEL.trim() === ''); const modelSource: ModelSource = parsedArgs!.model !== undefined ? 'flag' : modelFromYaml ? 'yaml' : 'env'; ``` This assumes "YAML only wins when env is unset" (today's order). Once YAML outranks env, `modelSource` must report `'yaml'` whenever YAML sets `PILOOP_MODEL` — regardless of whether env also sets it. Update the associated comment referencing ADR-010/ADR-013 precedence wording. ## Docs to update - `docs/adr/013-piloop-config-yaml.md` — edit **Decision**, **Rationale**, and **Alternatives Considered** sections in place to reflect `CLI > YAML > env > defaults`. Remove/replace the now-obsolete "File as repo source of truth" rejected-alternative entry (it's now the chosen design) and any "12-factor precedence" / "CI/machine env able to override the repo baseline" rationale language. - `docs/adr/010-per-run-model-override.md` — check and update any wording describing YAML/env/flag precedence for `--model`. - `docs/adr/014-research-stage.md`, `docs/adr/015-documentation-sync-stage.md` — check for precedence mentions and update if present. - `README.md`, `AGENTS.md`, `DESIGN.md`, `IMPLEMENTATION_PLAN.md` — update the documented precedence chain (`CLI flag > env var > YAML > built-in defaults` → `CLI flag > YAML > env var > built-in defaults`). - `piloop-config.example.yaml` — check/update any precedence-related comments. - `docs/design-auto-merge.md`, `docs/plan-B1-vertex-anthropic-provider.md` — check for precedence mentions and update if present. (`.reviews/*.md` are historical review notes — out of scope, do not touch.) ## Changelog Add a bullet to `CHANGELOG.md` under `## Unreleased` → `### Fixed`, e.g.: > **Config precedence: `piloop-config.yaml` now outranks environment variables.** Fixed the precedence order to `CLI flag > piloop-config.yaml > env var > built-in defaults` (previously `CLI flag > env var > piloop-config.yaml > built-in defaults`). A committed per-repo config is now correctly treated as the project's baseline/override, with env vars filling only the gaps it leaves unset. **This is a behavior change**: environments (including CI) that previously relied on an env var to override a value set in `piloop-config.yaml` must now use the equivalent CLI flag instead. ## Test plan - `src/config/services/configLoader.test.ts` — update `mergeEnvSources` precedence test cases to assert YAML wins over env when both are set; keep/extend the case where env fills a gap YAML leaves unset. - `src/cli/services/runCli.ts` model-source tests — update to assert `modelSource === 'yaml'` when both YAML and env set `PILOOP_MODEL` (previously asserted `'env'` in that case). - Full run: `npm run lint && npm test` must pass. ## Acceptance criteria - [ ] `mergeEnvSources` inverted so YAML values win over env values in `src/config/services/configLoader.ts`; associated docblocks updated. - [ ] `modelSource` detection in `src/cli/services/runCli.ts` rewritten to report `'yaml'` when both YAML and env set `PILOOP_MODEL`. - [ ] `docs/adr/013-piloop-config-yaml.md` edited in place (Decision/Rationale/Alternatives Considered) — no new ADR, no supersession note. - [ ] `docs/adr/010-per-run-model-override.md`, `docs/adr/014-research-stage.md`, `docs/adr/015-documentation-sync-stage.md` checked and updated if they reference the old precedence. - [ ] `README.md`, `AGENTS.md`, `DESIGN.md`, `IMPLEMENTATION_PLAN.md`, `piloop-config.example.yaml` updated to state the new precedence. - [ ] `docs/design-auto-merge.md`, `docs/plan-B1-vertex-anthropic-provider.md` checked and updated if applicable. - [ ] `CHANGELOG.md` updated with a new bullet under `## Unreleased` → `### Fixed`. - [ ] `configLoader.test.ts` and `runCli.ts` model-source tests updated to assert the new precedence. - [ ] `npm run lint && npm test` pass.

david added the

bug

label

2026-08-19 00:33:05 +00:00

david referenced this issue

2026-08-19 00:45:28 +00:00

282: Fix config precedence: piloop-config.yaml should outrank env vars (CLI > YAML > env > defaults) #283

david closed this issue

2026-08-19 00:45:31 +00:00

david commented

2026-08-19 00:45:31 +00:00

pi-loop opened and merged a pull request for this issue: #283