docs(adr): add ADR 0080 and 0081 for config.yaml vs. agent env scope - #5798
Conversation
fullsend-ai/agents#567's review thread (discussion_r3686020058) found no rule for when a knob belongs in config.yaml vs. an agent's env.runner/ env.sandbox. ADR 0080 amends ADR 0049: cross-agent knobs are config.yaml fields (no {AGENT}_ prefix, no env var), single-agent knobs are {AGENT}_-prefixed env vars (no config.yaml field). Cross-references added to ADR 0049 and docs/architecture.md. Assisted-by: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
PR Summary by Qododocs: Add ADR 0080 clarifying config.yaml vs agent env var scope
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
|
🤖 Review · |
Follows the Status-section annotation convention used elsewhere (e.g. ADR 0024's "Amended by" note) instead of duplicating the cross-reference in Context. Assisted-by: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
Site previewPreview: https://d5982529-site.fullsend-ai.workers.dev Commit: |
Code Review by Qodo
1.
|
|
🤖 Review · |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Cross-reference the {AGENT}_ prefix convention and the config.yaml
scope boundary in the harness env reference, since bring-your-own-agent.md
had no mention of either despite being the main custom-agent authoring guide.
Assisted-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Ralph Bean <rbean@redhat.com>
|
🤖 Review · |
The link to registering-your-agent just isn't helpful here. Signed-off-by: Ralph Bean <rbean@redhat.com>
f17576c to
9dcdffe
Compare
|
🤖 Review · |
|
🤖 Finished Review · ✅ Success · Started 2:28 PM UTC · Completed 2:41 PM UTC |
ReviewFindingsLow
Previous runReviewFindingsLow
Previous run (2)ReviewFindingsLow
Previous run (3)ReviewFindingsHigh
Low
Next steps:
Previous run (4)ReviewFindingsLow
Previous run (5)ReviewFindingsMedium
Next steps:
Previous run (6)Looks good to me Previous run (7)ReviewFindingsLow
Labels: PR adds a new ADR and updates documentation files only (ADRs, architecture.md, user guide) |
|
/fs-fix |
|
🤖 Finished Fix · ✅ Success · Started 2:48 PM UTC · Completed 2:55 PM UTC |
- ADR 0080: correct the passthrough-syntax failure mode — env
validation rejects `${VAR:-default}` at harness load (it treats the
whole expression as a host variable name), rather than os.Expand
silently resolving it to an empty string. Also fix the premise: env
vars do support `${VAR}` host-variable expansion (ADR 0055), just not
shell default-value syntax.
- ADR 0080: state the override-convention paragraph as a decision
ADR 0081 makes, not a pre-existing fact, and note it overrides ADR
0049's "CI workflow injection" item for behavior knobs specifically.
- ADR 0081: tighten "other values sourced from CI-native inputs" to
"other infrastructure values..." so it doesn't re-admit behavior
knobs set from a repo variable.
- ADR 0081: add Consequences follow-ups — the agents repo's own
review.md still needs the same fix as fullsend's copy, and
`CODE_ALLOWED_TARGET_BRANCHES` is still hardcoded in workflow env:
blocks despite ADR 0053 already deciding it belongs in harness
runner_env.
Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Ralph Bean <rbean@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 3:15 PM UTC · Completed 3:31 PM UTC |
maruiz93 asked why single-agent behavior tuning vars still need the
{AGENT}_ prefix if they only live in that agent's own harness. ADR 0049
already answers this (shared .env sourcing and shared runner_env/
env.sandbox contexts can collide) but ADR 0080 didn't restate it, so the
rationale wasn't self-contained.
Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Ralph Bean <rbean@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 4:21 PM UTC · Completed 4:35 PM UTC |
waynesun09
left a comment
There was a problem hiding this comment.
Approving after a full convention pass on both ADRs at head (30c7d80) plus verification of the round-2/3 fix claims against main:
host variable … is not setfail-fast wording matchesinternal/harness/harness.go:591— the afc0e1a failure-mode correction is accurate.- ADR 0081's
CODE_ALLOWED_TARGET_BRANCHESconsequence checks out: ADR 0053's migration section and Consequences do place it in harnessrunner_env, not workflow YAML, so pre-existing-non-conformance framing (rather than a new exception category) is right. - The ADR 0055 citation now points at a real heading (§ Runner behavior).
- No 0080/0081 numbering collisions on current main;
architecture.mdand the 0049 annotation are in the diff. - One-decision rule holds for both — the 0081 split was the right resolution of the earlier scope finding.
All of my earlier findings are addressed. Two non-blocking notes: ADR 0080 is ~107 content lines against the <80 target — review-driven additions account for the growth and each carries decision-relevant content, but the passthrough-syntax mechanics in the override paragraph could compress to a sentence plus the ADR 0055 link if you want it shorter; and the open fullsend-ai-review [low] on the "narrows" phrasing re-litigates the settled per-agent-vars thread — fine to resolve as-is.
From slack: lgtm, added a small comment here: https://github.com/fullsend-ai/fullsend/pull/5798/changes#r3705550910
|
Thanks @maruiz93 , @waynesun09 ! |
|
🤖 Finished Retro · ✅ Success · Started 5:37 PM UTC · Completed 6:04 PM UTC |
Retro: PR #5798 — ADR 0080/0081 for config.yaml vs. agent env scopeWorkflow shape: 12 commits (7 human, 5 bot) · 11 review runs (3 cancelled, 8 successful) · 3 fix runs · 3 human reviewers · merged in ~3 days Overall assessment: The workflow produced a high-quality outcome — two well-scoped ADRs with thorough human review that caught design-level issues and technical inaccuracies. The review bot added value on cross-reference consistency and naming conventions. However, two systemic issues consumed extra iterations and human attention. Key findings1. Review bot asserted incorrect code behavior; challenger confirmed the same error (evidence for agents#420, agents#549) The correctness sub-agent correctly identified that the ADR's stated passthrough failure mode was wrong ("treated as a literal value"). However, its own correction — that 2. Review bot re-litigated the settled "narrows" wording 3 times (evidence for #1013, #5265, #1672) The word "narrows" in ADR 0080 was the deliberate outcome of a human review discussion between waynesun09 and ralphbean. The review bot then raised a 3. Fix agent accepted incorrect technical claim without independent verification (evidence for agents#420) The fix agent took the review bot's incorrect 4. Autonomy readiness: human review was essential for design-level quality Human reviewers caught all design-level issues: ADR 0049 contradiction (waynesun09), override convention needing its own ADR (maruiz93 → led to ADR 0081 split), TRIAGE_AUTO_CODE classification being asserted rather than derived (waynesun09), and cross-repo consistency with agents#567 (waynesun09). The review bot's value was concentrated on formatting/consistency (wrong verb, missing cross-refs, wrong section heading). For ADR/design-doc PRs, the current autonomy level is appropriate — the bot catches mechanical issues while humans handle architectural reasoning. Proposal filed1 proposal targeting No proposal filed (covered by existing issues)
Proposals filed
|
Summary
config.yamlfield (never also an env var); a knob that tunes one specific agent's own behavior is an{AGENT}_-prefixed env var in that agent'senv.runner/env.sandbox(never aconfig.yamlfield).TRIAGE_AUTO_CODEcorrectly stays a harness env var (called out as a boundary case), and the per-repo/org override path is documenting the existing harness composition mechanism, not adding aconfig.yamlfield.env:block for infrastructure plumbing and CI-runtime-only values, never static agent behavior defaults. Split out during review after finding agents#567's docs already drifting toward workflow-env overrides for behavior knobs.docs/architecture.md.auto_mergeforkill_switchas ADR 0080's config.yaml example —auto_mergeturned out to be dead code (parsed but never consumed), tracked separately in internal/config: defaults.auto_merge is parsed but never consumed #5808.Test plan
make lintpasses (ADR status/number/frontmatter checks)Assisted-by: Claude Opus 4.6 noreply@anthropic.com