docs(adr): ADR 0047 — agent configuration env var convention - #2334
Conversation
Site previewPreview: https://0ad3e5ea-site.fullsend-ai.workers.dev Commit: |
|
🤖 Review · |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
🤖 Finished Review · ❌ Failure · Started 3:11 PM UTC · Completed 3:23 PM UTC |
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 4:18 PM UTC · Completed 4:32 PM UTC |
ReviewFindingsHigh
Medium
Info
Previous runReviewFindingsHigh
Medium
Low
Previous run (2)ReviewFindingsHigh
Medium
Low
Info
Previous run (3)ReviewFindingsMedium
Low
Info
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
|
|
||
| - `{ROLE}` is the agent's role in uppercase: `REVIEW`, `CODE`, `TRIAGE`, | ||
| `FIX`, `PRIORITIZE`, `RETRO`, etc. | ||
| - `{SETTING_NAME}` is `SCREAMING_SNAKE_CASE` describing the setting. |
There was a problem hiding this comment.
[medium] internal-consistency
The ADR lists CODE as the role prefix for the code agent (example: CODE_MAX_FILE_SIZE), but the actual harness role for the code agent is coder (see internal/scaffold/fullsend-repo/harness/code.yaml role field). Similarly, the fix agent shares role: coder. The ADR does not clarify whether the env var prefix should match the harness role field value (which would be CODER_MAX_FILE_SIZE) or a human-friendly shorthand. Two agents with the same role value but different env var prefixes (CODE_ vs FIX_) creates further inconsistency.
Suggested fix: Either (a) update the examples to use CODER_ to match the actual role value, or (b) explicitly state that the prefix is a human-readable short name that may differ from the harness role field, and document the canonical prefix for each agent.
fullsend-ai-review
Bot
added
the
requires-manual-review
| ### Naming | ||
|
|
||
| ``` | ||
| {ROLE}_{SETTING_NAME} |
There was a problem hiding this comment.
Role or agent name? For example for fix agent we would be using CODE as a prefix, no FIX. I don't think there is a real fix role.
There was a problem hiding this comment.
Yeah - I really mean agent "name". +1
There was a problem hiding this comment.
Yep. Renamed to {AGENT}_{SETTING_NAME} — prefix comes from the harness filename. Pushed 6470ad8.
| secrets, repo variables, or hardcoded defaults. This is the same mechanism | ||
| used for all other env vars — no change needed. | ||
|
|
||
| ### Defaults |
There was a problem hiding this comment.
Which is the reationale to not have the defaults on the harness?
There was a problem hiding this comment.
Oh, the very nature of a default is that it isn't present in the config file, right? How could they be in the harness and still be default values - values that are used when the harness doesn't supply them?
There was a problem hiding this comment.
The fact that something is a default depends on the level of abstraction your are at. A default value for a flag on a command is a default value if it lives on the command flag, or should be resolved just before execution on the same function it is making use of it? The standard level to look at this is the user level, so it is a default if the user does not see it. In our future the user won't see the harness, it will inherit from it and add values to it. So it is a default value to them. If they choose to write the full harness without inheriting, then it is required. We move the defaults to the agent .md, what if they want to overwrite the full agent .md file? Then it is not a default anymore. It is the problem of having too many levels. If the standard use case is to have users inherit the harness, I would have them the defaults on the harness.
There was a problem hiding this comment.
I see - move defaults to the canonical harness file.
There was a problem hiding this comment.
Makes sense — rewrote the Defaults section. Defaults now live in the canonical harness, downstream layers override via base composition. 6470ad8.
| - **Agent system prompts stay flexible.** There is no required section | ||
| structure for how `agents/<role>.md` references config vars. Agent | ||
| authors place references where they make sense in the prompt flow. | ||
| - **Each new config var requires updates in up to five places:** the |
There was a problem hiding this comment.
[Medium] This bullet acknowledges up to five update locations but offers no mitigation for keeping them in sync. ADR 0024 introduces required_env (planned, not yet implemented) as a contract for env var validation — when implemented, it could serve as a safety net for config vars too. Consider referencing required_env here as a future validation mechanism, and filing an issue to track the required_env implementation (it's mentioned in ADR 0024 but has no tracked issue yet).
Additionally, the enumeration misses two more consumers: skills (e.g., skills/code-implementation/SKILL.md already references TIMEOUT_SECONDS, MAX_RETRIES) and deterministic tools bundled in the sandbox image that read env vars directly. Consider expanding the list to cover the full surface.
There was a problem hiding this comment.
I think the ADR's scope paragraph (lines 39-42) already limits this to behavioral knobs, and ADR 0024's required_env is still unimplemented. Going to leave this as-is for now.
There was a problem hiding this comment.
I'm ok with the required_env for now.
But regarding the consumers, skills and tools would handle config env vars and they are not mentioned in the list.
Imagine that we want to use a code agent that is able to push. To control the push (making security checks and doing only certain pushes) we implement a certain tool that reads config vars. This vars would be hardcoded in the tool, meaning that it is a place where they have to be updated if the name of the var is changed.
Isn't that what this point is covering?
| environment or when env files are sourced together. It also makes `grep` and | ||
| audit trivial: `grep ^REVIEW_ env/review.env` shows every knob for that agent. | ||
|
|
||
| ### Where config vars live in the harness |
There was a problem hiding this comment.
[Low] This section enumerates three delivery paths but doesn't mention providers (OpenShell credential injection). While providers are for credentials rather than behavioral config knobs, explicitly noting them as out of scope would make the coverage complete and prevent readers from wondering whether config vars could use the provider mechanism.
There was a problem hiding this comment.
The scope paragraph already excludes credentials/infrastructure vars, which is what providers carry. Leaving as-is.
There was a problem hiding this comment.
ok, I thought that mentioning the word providers would help for someone defining a new agent, because it is a different way to get env vars set in the agent. When reading this ADR I had to review how they work and understand that they are out of the scope here.
It's only for clarification so if you think it's ok as is, it's not a blocker for me :)
| inference time, while `runner_env` makes vars available to pre/post scripts | ||
| on the host. A config var needed by both must appear in both places. | ||
|
|
||
| 1. **For sandbox access (inference time):** Add the variable to the agent's |
There was a problem hiding this comment.
[Low] The ADR doesn't explicitly recommend keeping all config vars for an agent in its existing .env file (e.g., env/review.env). Nothing prevents spreading config vars across multiple files (e.g., env/review-severity.env, env/review-inline.env), each with its own host_files entry. Consider recommending that config vars for an agent live in that agent's existing .env file to preserve the current one-file-per-agent pattern and keep discoverability simple.
There was a problem hiding this comment.
The text already says "the agent's .env file" (singular) at lines 79-80 — I think that's clear enough without an explicit recommendation.
There was a problem hiding this comment.
ok, when I read it I thought of this because it wasn't clear enough. I guess that I was reading "the .env file" as one of multiple possible env files, because we currently use several env files per agent (because of the vertex env vars and other contextual env vars) so it might be confusing talking in singular here. I agree everything is there already: config vars, talking in singular, previously define that config vars are behavioural knobs... so if you think it's clear it's ok for me :)
|
🤖 Finished Review · ✅ Success · Started 6:29 PM UTC · Completed 6:40 PM UTC |
| @@ -0,0 +1,186 @@ | |||
| --- | |||
There was a problem hiding this comment.
[high] adr-number-collision
ADR number collision: this PR adds 0047-agent-configuration-env-var-convention.md, but docs/ADRs/0047-vendored-installs-with-vendor-flag.md already exists on main. ADR 0048 also exists. Must renumber to 0049.
Suggested fix: Renumber this ADR to 0049. Update the title, filename, docs/architecture.md reference, and the heading inside the file accordingly.
| runner_env) from platform-neutral fields. Forge blocks inherit from | ||
| top-level defaults and override only deltas | ||
| ([ADR 0045](ADRs/0045-forge-portable-harness-schema.md)). | ||
| - Agent configuration env vars: behavioral knobs use `{ROLE}_{SETTING_NAME}` |
There was a problem hiding this comment.
[medium] internal-consistency
Terminology mismatch: the ADR defines the convention as {AGENT}{SETTING_NAME} (prefix derived from harness filename), but docs/architecture.md uses {ROLE}{SETTING_NAME}. Since role is an existing harness YAML field with different values (e.g., code.yaml has role: coder), using {ROLE} implies the prefix comes from the role field, contradicting the ADR.
Suggested fix: Change {ROLE}{SETTING_NAME} in architecture.md to {AGENT}{SETTING_NAME} to match the ADR, or pick one term and use it consistently.
|
|
||
| - `{AGENT}` is the agent's **name** in uppercase, derived from the harness | ||
| filename: `REVIEW`, `CODE`, `TRIAGE`, `FIX`, `PRIORITIZE`, `RETRO`, etc. | ||
| - `{SETTING_NAME}` is `SCREAMING_SNAKE_CASE` describing the setting. |
There was a problem hiding this comment.
[low] internal-consistency
The ADR states the prefix is derived from the harness filename but does not explicitly contrast this with the role field. Both code.yaml and fix.yaml share role: coder, which could cause confusion about whether the prefix is CODE/FIX (filename) or CODER (role).
Suggested fix: Add an explicit statement: The prefix is the harness YAML filename stem (e.g., code from code.yaml), NOT the role field value.
|
|
||
| See [Customizing with AGENTS.md](../guides/user/customizing-with-agents-md.md) and | ||
| [Customizing with Skills](../guides/user/customizing-with-skills.md). | ||
|
|
There was a problem hiding this comment.
[low] runtime-mechanism-correctness
The severity filtering mechanism at inference time relies entirely on the LLM faithfully following system prompt instructions. There is no programmatic enforcement. The ADR does not acknowledge that inference-time config var handling is best-effort and non-deterministic.
| **For pre/post scripts,** use standard shell defaulting, which already handles | ||
| both empty and unset: `${REVIEW_SEVERITY_THRESHOLD:-low}`. | ||
|
|
||
| ### Documentation |
There was a problem hiding this comment.
[info] edge-case-handling
The default for REVIEW_SEVERITY_THRESHOLD is low, meaning info-level findings are suppressed by default. Internally consistent but the ADR does not call out that the default filters out a severity level.
fullsend-ai-review
Bot
removed
the
requires-manual-review
|
🤖 Finished Review · ✅ Success · Started 8:51 PM UTC · Completed 9:04 PM UTC |
|
|
||
| - `{AGENT}` is the agent's **name** in uppercase, derived from the harness | ||
| filename: `REVIEW`, `CODE`, `TRIAGE`, `FIX`, `PRIORITIZE`, `RETRO`, etc. | ||
| - `{SETTING_NAME}` is `SCREAMING_SNAKE_CASE` describing the setting. |
There was a problem hiding this comment.
[medium] internal-consistency
The ADR states the {AGENT} prefix is derived from the harness filename but does not contrast this with the harness role field, which uses different values: code.yaml and fix.yaml both have role: coder. ROLE_APP_IDS in the codebase uses the role field value as its key, so a reader may assume config var prefixes also derive from role.
Suggested fix: Add a brief note: The prefix is always the harness filename stem, not the role field value. For example, code.yaml and fix.yaml both have role: coder, but their config var prefixes are CODE_ and FIX_ respectively.
| @@ -91,6 +91,11 @@ The harness draws its configuration from the adopting organization's **`.fullsen | |||
| runner_env) from platform-neutral fields. Forge blocks inherit from | |||
| top-level defaults and override only deltas | |||
| ([ADR 0045](ADRs/0045-forge-portable-harness-schema.md)). | |||
There was a problem hiding this comment.
[low] naming-convention-ambiguity
The ADR addresses the overlap with existing credential vars (FIX_GH_TOKEN) by noting the distinction is by purpose and documentation, but does not explain why credential vars use a FULLSEND_ namespace prefix while config vars do not.
|
|
||
| See [Customizing with AGENTS.md](../guides/user/customizing-with-agents-md.md) and | ||
| [Customizing with Skills](../guides/user/customizing-with-skills.md). | ||
|
|
There was a problem hiding this comment.
[low] runtime-mechanism-correctness
The severity filtering example at inference time relies entirely on the LLM following system prompt instructions with no programmatic enforcement. The ADR does not acknowledge that inference-time config var handling is best-effort rather than deterministic.
Establish naming convention for agent behavioral configuration
environment variables: {ROLE}_{SETTING_NAME} in SCREAMING_SNAKE_CASE.
Uses existing delivery mechanisms (env files, runner_env) with no
runner changes required.
Refs: #2333
Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Ralph Bean <rbean@redhat.com>
Make explicit that .env files and runner_env serve different audiences (sandbox vs host) and a var needed by both must appear in both. Update consequences to list all five potential touchpoints per config var. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
- Rename {ROLE}_ to {AGENT}_ prefix, derived from harness filename
- Move shared-settings rule into Decision/Naming section
- Rewrite Defaults: defaults live in canonical harness, downstream
overrides via base composition (ADR 0045)
- Handle empty-string-vs-unset: expand: true resolves unset vars to
empty string, so agents and scripts must treat both the same
- Fix precedence reference: ADR 0006 → ADR 0045
- Acknowledge grep overlap with existing context/credential vars
- Replace echo with printenv for accuracy
- Fold duplicated pre/post scripts section into Defaults
- Add audience signposting in Defaults section
- Reformat dense consequences bullet into numbered sub-list
Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com>
Signed-off-by: Ralph Bean <rbean@redhat.com>
The ADR established {AGENT}_{SETTING_NAME} as the convention but four
references still used the old {ROLE} placeholder.
Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com>
Signed-off-by: Ralph Bean <rbean@redhat.com>
0047 is already taken on main by vendored-installs-with-vendor-flag. 0048 is also taken. Next available is 0049. Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
d66eee8 to
6cf0bb0
Compare
|
🤖 Finished Review · ✅ Success · Started 2:08 PM UTC · Completed 2:18 PM UTC |
|
|
||
| - `{AGENT}` is the agent's **name** in uppercase, derived from the harness | ||
| filename: `REVIEW`, `CODE`, `TRIAGE`, `FIX`, `PRIORITIZE`, `RETRO`, etc. | ||
| - `{SETTING_NAME}` is `SCREAMING_SNAKE_CASE` describing the setting. |
There was a problem hiding this comment.
[medium] internal-consistency
The ADR states the {AGENT} prefix is derived from the harness filename, but does not contrast this with the harness role field, which uses different values: code.yaml and fix.yaml both have role: coder. A reader familiar with ROLE_APP_IDS may assume config var prefixes also derive from role.
Suggested fix: Add a brief note in the Naming section clarifying the prefix is the harness filename stem, not the role field value.
|
|
||
| See [Customizing with AGENTS.md](../guides/user/customizing-with-agents-md.md) and | ||
| [Customizing with Skills](../guides/user/customizing-with-skills.md). | ||
|
|
There was a problem hiding this comment.
[info] runtime-mechanism-correctness
The severity filtering example at inference time relies entirely on the LLM following system prompt instructions with no programmatic enforcement. This is inherent to the agent architecture rather than a gap specific to this ADR.
|
🤖 Finished Retro · ✅ Success · Started 2:42 PM UTC · Completed 2:51 PM UTC |
Retro: PR #2334 — ADR 0049 agent config env var conventionTimelineThis was a human-authored, docs-only PR (2 files: an ADR and architecture.md update) that went through 6 review agent runs over 2 days before being merged with the bot still in CHANGES_REQUESTED state.
AssessmentReview quality: Mixed. The ADR number collision catch (run 4) was valuable but should have been caught on run 3. Human reviewers provided more actionable feedback than the bot on this docs PR. The bot's persistent CHANGES_REQUESTED for a documentation suggestion ("add a note contrasting prefix with role field") after the author had already addressed the underlying concern by renaming ROLE to AGENT created unnecessary friction. Rework rate: The author made 3 fix pushes, which is reasonable given the volume of substantive human feedback. However, 2 of those pushes were driven by bot findings that could have been caught earlier (collision) or were already addressed (repeated role/agent finding). Token cost: 6 review runs for a 2-file docs PR is high. The repeated finding across runs 4–6 represents wasted compute. Existing coverageThe main patterns observed are already tracked by open issues:
No new proposals are warranted — the improvement opportunities are well-covered by existing issues. Prioritizing #1500, #2115, and #992 would have the highest impact on preventing the friction observed in this PR. |
4 participants
Summary
{ROLE}_{SETTING_NAME}inSCREAMING_SNAKE_CASE.envfiles,runner_env, CI workflowenv:) — no runner changes neededdocs/agents/<role>.mdgets a### Variablessubsection under "Configuration and extension" for discoverabilitydocs/architecture.mdAgent Harness "Decided:" sectionRefs: #2333
Test plan
make lintpasses (ADR status, number, frontmatter linters all green)🤖 Generated with Claude Code