docs(#2345): ADR 0053 — agent-driven branch targeting for code agent - #2346
Conversation
|
🤖 Finished Review · ✅ Success · Started 6:31 PM UTC · Completed 6:42 PM UTC |
ReviewFindingsHigh
Medium
Low
Previous runReviewFindingsMedium
Low
Previous run (2)ReviewReason: stale-head The review agent reviewed commit Previous run (3)ReviewFindingsMedium
Low
Previous run (4)ReviewFindingsCritical
High
Medium
Low
Previous run (5)ReviewFindingsCritical
High
Medium
Low
Labels: PR modifies code agent harness config, CI workflow, ADR, and post-script Previous run (6)ReviewFindingsMedium
Low
Previous run (7)ReviewFindingsCritical
High
Medium
Low
Previous run (8)ReviewFindingsCritical
High
Medium
Low
Previous run (9)ReviewFindingsMedium
Low
Previous run (10)ReviewFindingsCritical
High
Medium
Low
Previous run (11)ReviewFindingsCritical
High
Medium
Low
Previous run (12)ReviewFindingsCritical
High
Medium
Low
Previous run (13)ReviewFindingsMedium
Low
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
fullsend-ai-review
Bot
added
the
requires-manual-review
|
FYI, we talked in slack about re-doing this so that it doesn't add more business logic to the gh workflows. |
Marcusk19
changed the title
|
updated the PR to drop the changes to the workflow file and instead propose a new ADR for addressing this |
|
🤖 Finished Review · ✅ Success · Started 1:42 PM UTC · Completed 1:54 PM UTC |
fullsend-ai-review
Bot
removed
the
requires-manual-review
E2E tests did not runE2E tests run automatically for org/repo members and collaborators on pull requests. For other contributors, a maintainer must add the See E2E testing guide for details. |
|
🤖 Finished Review · ✅ Success · Started 2:19 PM UTC · Completed 2:37 PM UTC |
ralphbean
left a comment
ralphbean
left a comment
There was a problem hiding this comment.
A few things to sort out — see inline comments. One thing I can't comment inline: internal/scaffold/scaffold_test.go:653 expects TARGET_BRANCH in code.yaml's runner_env, so that test will break with the new key names.
|
|
||
| If a `TARGET_BRANCH` environment variable is set, use it. Otherwise, determine | ||
| the default branch: | ||
| Determine the correct target branch from the issue context. If the issue |
There was a problem hiding this comment.
[moderate] The post-script validates the agent's branch choice against CODE_ALLOWED_TARGET_BRANCHES, but the skill instructions don't mention that constraint. If the agent picks a branch not in the allowlist, the post-script exits with an error and the agent won't know why. Worth mentioning the allowlist here so the agent can fall back to the default branch gracefully.
There was a problem hiding this comment.
I don't think the agent needs to know about it, then it could try to implement the solution against a branch that shouldn't, just because it is the "next best branch I could implement this change to".
There was a problem hiding this comment.
If the agent will read the variable, then I think it should exit early if there are any conflicts.
There was a problem hiding this comment.
I'm inclined to agree - the expected behavior in my head should be if someone instructs the agent to use a missing branch from the allowlist it will raise an error instead of falling back a PR against main
|
🤖 Finished Review · ✅ Success · Started 2:05 PM UTC · Completed 2:26 PM UTC |
ralphbean
left a comment
ralphbean
left a comment
There was a problem hiding this comment.
LGTM. One non-blocking note inline.
|
|
||
| Write this output early (during planning, after determining the target | ||
| branch) so it is available even if the agent hits a timeout or error later. | ||
| The post-script validates this against the repo's allowed branches. |
There was a problem hiding this comment.
[nit] Other agents call fullsend-check-output after writing their structured output. Probably fine to skip here since the commit is the primary artifact and the post-script handles missing/malformed output gracefully — just noting the schema isn't validated at runtime.
fullsend-ai-review
Bot
added
the
requires-manual-review
|
🤖 Finished Review · ✅ Success · Started 6:21 PM UTC · Completed 6:37 PM UTC |
fullsend-ai-review
Bot
added
component/harness
Site previewPreview: https://c4b67406-site.fullsend-ai.workers.dev Commit: |
e4bd467 to
2e96f9c
Compare
|
🤖 Finished Review · ✅ Success · Started 1:22 PM UTC · Completed 1:39 PM UTC |
…ueue) Signed-off-by: Marcus Kok <mkok@redhat.com>
…tream) Signed-off-by: Marcus Kok <mkok@redhat.com>
af03ca7 to
e6ecc12
Compare
Marcusk19
changed the title
|
🤖 Finished Review · ✅ Success · Started 7:42 PM UTC · Completed 7:56 PM UTC |
| # These are expanded from the runner environment and NEVER enter the sandbox. | ||
| runner_env: | ||
| TARGET_BRANCH: "${TARGET_BRANCH}" | ||
| CODE_ALLOWED_TARGET_BRANCHES: "${CODE_ALLOWED_TARGET_BRANCHES}" |
There was a problem hiding this comment.
[medium] breaking-env-contract
Removal of TARGET_BRANCH from runner_env breaks enrolled repos with customized code.yaml files that still reference ${TARGET_BRANCH}. ValidateRunnerEnvWith in harness.go checks os.LookupEnv for all ${VAR} references, so a customized harness containing TARGET_BRANCH will fail validation because the workflow no longer provides this variable. ADR 0053 documents this as a consequence with concrete before/after YAML, but does not provide automated detection or a deprecation period.
Suggested fix: Add detection in fullsend admin install --upgrade to scan customized dirs for TARGET_BRANCH references. Consider a deprecation period where both variables are provided.
| fi | ||
| done | ||
| if [ -n "${RESULT_FILE}" ]; then | ||
| AGENT_TARGET="$(jq -r '.target_branch // empty' "${RESULT_FILE}" 2>/dev/null || true)" |
There was a problem hiding this comment.
[low] edge-case
Comma-separated allowlist matching does not handle whitespace in entries. When CODE_ALLOWED_TARGET_BRANCHES is set to 'main, develop' (with a space), the grep -qF check will fail for 'develop' because ', develop,' does not match ',develop,'.
fullsend-ai-review
Bot
added
requires-manual-review
Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 8:35 PM UTC · Completed 8:51 PM UTC |
| # These are expanded from the runner environment and NEVER enter the sandbox. | ||
| runner_env: | ||
| TARGET_BRANCH: "${TARGET_BRANCH}" | ||
| CODE_ALLOWED_TARGET_BRANCHES: "${CODE_ALLOWED_TARGET_BRANCHES}" |
There was a problem hiding this comment.
[medium] breaking-env-contract
Removal of TARGET_BRANCH from runner_env breaks enrolled repos with customized code.yaml files that still reference ${TARGET_BRANCH}. ValidateRunnerEnvWith checks os.LookupEnv for all ${VAR} references. ADR 0053 documents this consequence but provides no automated detection or deprecation period.
| fi | ||
| done | ||
| if [ -n "${RESULT_FILE}" ]; then | ||
| AGENT_TARGET="$(jq -r '.target_branch // empty' "${RESULT_FILE}" 2>/dev/null || true)" |
There was a problem hiding this comment.
[low] edge-case
The comma-separated allowlist matching does not handle whitespace in entries. When CODE_ALLOWED_TARGET_BRANCHES has spaces after commas, the grep -qF check will fail for those entries.
fullsend-ai-review
Bot
removed
the
requires-manual-review
|
🤖 Finished Retro · ✅ Success · Started 9:01 PM UTC · Completed 9:10 PM UTC |
Retro: PR #2346 — ADR 0053 agent-driven branch targetingTimelineThis human-authored PR (by Marcusk19) proposed ADR 0053 over 6 days (June 16–22). It started as a workflow-level fix, was refactored to an ADR-based approach after Slack discussion, and went through 10+ review bot cycles and 2 human review rounds before merging. What went well
What could go betterThe dominant issue on this PR was review noise: the bot ran 10+ full review cycles on a ~270-line documentation PR, repeatedly surfacing the same findings:
This pattern consumed significant tokens (10+ full reviews of a docs PR) and trained the human to ignore bot reviews. Existing issues that cover these improvementsAll major improvement opportunities identified are already tracked in the backlog:
No new proposals filed — the backlog already covers the improvements this retro would recommend. Prioritizing #1013 (finding dedup) and #1922 (don't override human approval) would have the highest impact on workflows like this one. |
The v0 tag (= v0.18.0, released June 16) still hardcodes TARGET_BRANCH: main in reusable-code.yml. Repos with a non-main default branch (e.g. quay-operator uses master) fail at PR creation because `gh pr create --base main` targets a branch that doesn't exist. fullsend-ai/fullsend#2346 (merged June 22) replaced the hardcoded TARGET_BRANCH with agent-driven branch detection (ADR 0053), but this hasn't been released yet. Pin to @main temporarily to pick up the fix. Revert to @v0 once v0.19.0 is released and the v0 tag moves forward. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…-branch docs(fullsend-ai#2345): ADR 0053 — agent-driven branch targeting for code agent
3 participants
Summary
Proposes ADR 0047: move branch-targeting logic from the hardcoded
TARGET_BRANCH: mainworkflow env var to agent-driven selection with post-script policy enforcement.code-result.json(structured output)CODE_ALLOWED_TARGET_BRANCHES(or auto-detected default)Motivation
TARGET_BRANCH: mainis hardcoded inreusable-code.yml, breaking repos with non-main default branches (example failure)Changes
docs/ADRs/0047-agent-driven-branch-targeting.md— the ADR (Proposed status)Design summary
See ADR 0047 for full details. Key points:
schemas/code-result.schema.jsonwithtarget_branchfieldCODE_ALLOWED_TARGET_BRANCHESTARGET_BRANCHfromrunner_envCloses #2345