Skip to content

fix(#483): defer target_branch until after default branch discovery - #488

Merged
rh-hemartin merged 3 commits into
mainfrom
agent/483-defer-target-branch
Jul 31, 2026
Merged

fix(#483): defer target_branch until after default branch discovery#488
rh-hemartin merged 3 commits into
mainfrom
agent/483-defer-target-branch

Conversation

@fullsend-ai-coder

Copy link
Copy Markdown
Contributor

Summary

Fixes branch validation failures on repos with non-main default branches by:

  • skills/code-implementation/SKILL.md (step 3): Adding fallback branch discovery commands (git symbolic-ref, gh repo view) so the agent tries multiple methods before writing target_branch. Documents that the post-script auto-corrects when no explicit allowed list is configured.
  • scripts/post-code.src.sh: When CODE_ALLOWED_TARGET_BRANCHES is unset and the agent's target doesn't match the API-discovered default, auto-correcting to the default branch with a warning instead of hard-failing. Hard failure is preserved when an explicit allowed list is set.
  • scripts/post-code.sh: Regenerated via make script-build.
  • scripts/post-code-test.sh: 8 new test cases covering the branch validation decision matrix (auto-correct, explicit list reject, match, wildcard, no-target fallback, etc.).

Testing

  • All 76 post-code tests pass (68 existing + 8 new)
  • make check-bundle confirms bundled script is in sync
  • Pre-existing failure in post-triage-test.sh (prerequisites-creates-allowed-issue) is unrelated to these changes

Closes #483

Post-script verification

  • Branch is not main/master (agent/483-defer-target-branch)
  • Secret scan passed (gitleaks — 3760b3bb70de32596c1922332fb96e76cd9ebcd8..HEAD)
  • PR body secret scan passed (gitleaks — no-git)
  • Pre-commit hooks passed (authoritative run on runner)
  • Tests ran inside sandbox

@rh-hemartin

Copy link
Copy Markdown
Member

/fs-fix make the command gh repo view --json defaultBranchRef --jq '.defaultBranchRef.name' the first one to be tested and then the other ones.

@fullsend-ai-coder

fullsend-ai-coder Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 9:34 AM UTC · Completed 9:39 AM UTC
Commit: de7ace2 · View workflow run →

fullsend-ai-coder Bot added a commit that referenced this pull request Jul 28, 2026
Reorder branch discovery commands in SKILL.md so `gh repo view` is tried
first, followed by `git rev-parse` and `git symbolic-ref` as fallbacks.

Addresses review feedback on #488
@fullsend-ai-coder

Copy link
Copy Markdown
Contributor Author

🔧 Fix agent — iteration 1 (human-triggered)

Reordered branch discovery commands in SKILL.md per human instruction: gh repo view is now the first method tried, with git rev-parse and git symbolic-ref as fallbacks.

Fixed (1):

  1. reorder branch discovery to prioritize gh repo view (skills/code-implementation/SKILL.md): Reordered the branch discovery commands so gh repo view --json defaultBranchRef --jq '.defaultBranchRef.name' is tried first, followed by git rev-parse --abbrev-ref origin/HEAD and git symbolic-ref refs/remotes/origin/HEAD as fallbacks.

Tests: passed

Updated by fullsend fix agent

@rh-hemartin

Copy link
Copy Markdown
Member

/fs-review

@fullsend-ai-review

fullsend-ai-review Bot commented Jul 28, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 9:41 AM UTC · Completed 9:56 AM UTC
Commit: c321286 · View workflow run →

@fullsend-ai-review

fullsend-ai-review Bot commented Jul 28, 2026

Copy link
Copy Markdown

Review

Findings

Medium

  • [protected-path] scripts/post-code.src.sh, scripts/post-code.sh, scripts/post-code-test.sh, skills/code-implementation/SKILL.md — All four changed files are under protected paths (scripts/, skills/). The PR links to issue fix(code): defer target_branch until after default branch discovery #483 and provides clear rationale for the changes. Human approval is always required for protected-path changes, regardless of context.

  • [behavior-description-incomplete] docs/code.md:44 — The Default column for CODE_ALLOWED_TARGET_BRANCHES describes the default value as "Repo default branch (auto-detected via GitHub API; falls back to main)" but does not explain the new auto-correct behavior. After this PR, when CODE_ALLOWED_TARGET_BRANCHES is unset, the post-script auto-corrects the agent's chosen branch to the default branch rather than validating and rejecting a mismatch. The current description conflates the default-branch discovery mechanism with the variable's behavioral default.
    Remediation: Update the Default column to clarify: "When unset: auto-corrects agent's choice to repo default branch. When set: validates agent's choice against the specified allowlist."

Low

  • [test-integrity] scripts/post-code-test.sh:1107 — Tests reimplement the branch validation logic in a standalone function (validate_target_branch) rather than exercising the production code. The test helper faithfully mirrors the production logic at PR head, but logic drift between the two would not be caught. This matches existing test patterns in the file, so severity remains low.
Previous run

Review

Findings

Medium

  • [protected-path] scripts/post-code.src.sh, scripts/post-code.sh, scripts/post-code-test.sh, skills/code-implementation/SKILL.md — All four changed files are under protected paths (scripts/, skills/). The PR links to issue fix(code): defer target_branch until after default branch discovery #483 and provides clear rationale for the changes. Human approval is always required for protected-path changes, regardless of context.

Low

  • [authorization-change] scripts/post-code.src.sh:135 — When CODE_ALLOWED_TARGET_BRANCHES is unset, the behavior changed from fail-closed (hard-fail on mismatch) to auto-correct (silently use the API-discovered default branch). This is the intended fix per issue fix(code): defer target_branch until after default branch discovery #483 and the auto-corrected value is always the safe default. Operators who relied on the old implicit "reject mismatches" behavior should be aware of the change.

  • [documentation-coherence] skills/code-implementation/SKILL.md:204 — The bolded text says "Do not hardcode main as a fallback" but the jq template two lines later uses ${DEFAULT_BRANCH:-main}, which falls back to main. Consider rewording to "Do not skip discovery and assume main" to avoid confusing the agent.

  • [edge-case] scripts/post-code-test.sh:1139 — Consider adding a test for substring matching (e.g., agent_target='release', allowed_list='release-1,release-2'). The comma-wrapping technique correctly rejects partial matches; codifying this in a test guards against regressions.

  • [test-integrity] scripts/post-code-test.sh:1053 — Tests reimplement the branch validation logic in a standalone function rather than exercising the production code. This matches existing test patterns in the file but means logic drift between the test helper and production code would not be caught.


Labels: PR fixes a bug in the code agent's branch validation logic (scripts/post-code.src.sh, skills/code-implementation/SKILL.md)

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review fullsend-ai-review Bot added requires-manual-review Review requires human judgment bug Something isn't working code-agent labels Jul 28, 2026
@rh-hemartin

Copy link
Copy Markdown
Member

/fs-fix

@fullsend-ai-coder

fullsend-ai-coder Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 9:59 AM UTC · Completed 10:05 AM UTC
Commit: c321286 · View workflow run →

@fullsend-ai-coder

Copy link
Copy Markdown
Contributor Author

🔧 Fix agent — iteration 2 (human-triggered)

Addressed 2 of 5 review findings with code changes: rewrote contradictory documentation wording in SKILL.md and added a substring-matching edge-case test. Disagreed with 3 informational findings (protected-path requires human approval, authorization-change is intentional per #483, test-integrity refactor is out of scope).

Fixed (2):

  1. documentation-coherence: contradictory wording about hardcoding main (skills/code-implementation/SKILL.md): Reworded the bolded guidance from 'Do not hardcode main as a fallback' to 'Do not skip discovery and assume main' to align with the ${DEFAULT_BRANCH:-main} fallback in the jq template two lines below.
  2. edge-case: missing substring matching test (scripts/post-code-test.sh): Added substring-not-accepted test case verifying that agent_target='release' is rejected when allowed_list='release-1,release-2', confirming the comma-wrapping technique correctly rejects partial matches.

Disagreed (3):

  1. test-integrity: tests reimplement logic rather than exercising production code: The reviewer notes this matches existing test patterns in the file. Changing the test approach to exercise production code directly would require a significant refactor of the test harness, which is out of scope for this PR.
  2. protected-path: all changed files are under protected paths: Informational finding. Human approval is always required for protected-path changes — no code fix applies.
  3. authorization-change: behavior changed from fail-closed to auto-correct: Informational finding. The behavior change is the intended fix per issue fix(code): defer target_branch until after default branch discovery #483. No code modification needed.

Tests: passed

Updated by fullsend fix agent

fullsend-ai-coder Bot added a commit that referenced this pull request Jul 28, 2026
- Reword documentation-coherence issue in SKILL.md: replace "Do not
  hardcode main as a fallback" with "Do not skip discovery and assume
  main" to align with the ${DEFAULT_BRANCH:-main} fallback in the jq
  template (documentation-coherence finding).
- Add substring-not-accepted test case to verify comma-wrapping
  rejects partial matches like "release" when only "release-1" and
  "release-2" are in the allowed list (edge-case finding).

Addresses review feedback on #488
@rh-hemartin

Copy link
Copy Markdown
Member

/fs-fix rebase and resolve conflicts

@fullsend-ai-coder

fullsend-ai-coder Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 10:09 AM UTC · Completed 10:14 AM UTC
Commit: b050b0f · View workflow run →

@fullsend-ai-coder

Copy link
Copy Markdown
Contributor Author

🔧 Fix agent — iteration 3 (human-triggered)

Rebased agent/483-defer-target-branch onto origin/main, resolving one conflict in skills/code-implementation/SKILL.md where main renamed code-result.json to agent-result.json (PR #377). Resolution keeps our branch's ${DEFAULT_BRANCH:-main} variable substitution with the new filename. All 76 post-code tests pass. Bundle check passes.

Fixed (1):

  1. rebase and resolve conflicts (skills/code-implementation/SKILL.md): Rebased branch onto origin/main (3 commits from PR fix(#376): standardize agent output filename to agent-result.json #377 for agent-result.json rename). Resolved merge conflict in skills/code-implementation/SKILL.md by keeping the branch's ${DEFAULT_BRANCH:-main} logic while adopting the new agent-result.json filename from main. All other files (post-code scripts) merged cleanly.

Tests: passed

Updated by fullsend fix agent

fullsend-ai-coder Bot added a commit that referenced this pull request Jul 28, 2026
Reorder branch discovery commands in SKILL.md so `gh repo view` is tried
first, followed by `git rev-parse` and `git symbolic-ref` as fallbacks.

Addresses review feedback on #488
fullsend-ai-coder Bot added a commit that referenced this pull request Jul 28, 2026
- Reword documentation-coherence issue in SKILL.md: replace "Do not
  hardcode main as a fallback" with "Do not skip discovery and assume
  main" to align with the ${DEFAULT_BRANCH:-main} fallback in the jq
  template (documentation-coherence finding).
- Add substring-not-accepted test case to verify comma-wrapping
  rejects partial matches like "release" when only "release-1" and
  "release-2" are in the allowed list (edge-case finding).

Addresses review feedback on #488
@fullsend-ai-coder
fullsend-ai-coder Bot force-pushed the agent/483-defer-target-branch branch from b050b0f to bb81f63 Compare July 28, 2026 10:14
@rh-hemartin

Copy link
Copy Markdown
Member

/fs-fix rebase and resolve conflicts.

@fullsend-ai-coder

fullsend-ai-coder Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ❌ Failure · Started 9:53 AM UTC · Completed 10:01 AM UTC
Commit: bb81f63 · View workflow run →

The code agent hardcoded target_branch to "main" when branch discovery
failed in the sandbox. The post-code script then rejected the output on
repos with a non-main default branch (e.g. "master").

Two changes fix this:

1. SKILL.md step 3: Add fallback discovery commands (git symbolic-ref,
   gh repo view) so the agent tries multiple methods before writing
   target_branch. Document that the post-script auto-corrects when no
   explicit allowed list is configured.

2. post-code.src.sh: When CODE_ALLOWED_TARGET_BRANCHES is unset and
   the agent's target doesn't match the API-discovered default, auto-
   correct to the default branch with a warning instead of hard-failing.
   Hard failure is preserved when an explicit allowed list is set.

Bundled script (post-code.sh) regenerated via make script-build.
8 new test cases cover the branch validation decision matrix.

Note: pre-commit could not run in the sandbox (network restrictions).
The post-script runs an authoritative pre-commit check on the runner.

Closes #483
Reorder branch discovery commands in SKILL.md so `gh repo view` is tried
first, followed by `git rev-parse` and `git symbolic-ref` as fallbacks.

Addresses review feedback on #488
- Reword documentation-coherence issue in SKILL.md: replace "Do not
  hardcode main as a fallback" with "Do not skip discovery and assume
  main" to align with the ${DEFAULT_BRANCH:-main} fallback in the jq
  template (documentation-coherence finding).
- Add substring-not-accepted test case to verify comma-wrapping
  rejects partial matches like "release" when only "release-1" and
  "release-2" are in the allowed list (edge-case finding).

Addresses review feedback on #488
@fullsend-ai-coder

Copy link
Copy Markdown
Contributor Author

⚠️ Post-fix script failed — Structured output processing failed (exit code 1)

The fix agent completed, but the post-fix script failed before finishing.

Workflow run: https://github.com/fullsend-ai/.fullsend/actions/runs/30532374812

Details:
process-fix-result.py failed with exit code 1 (bad input) for PR #488 in fullsend-ai/agents
Please check the workflow logs for full details and retry with /fs-fix if appropriate.

@fullsend-ai-coder
fullsend-ai-coder Bot force-pushed the agent/483-defer-target-branch branch from bb81f63 to 5793196 Compare July 30, 2026 10:01
@rh-hemartin

Copy link
Copy Markdown
Member

/fs-review

@fullsend-ai-review

fullsend-ai-review Bot commented Jul 31, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 6:49 AM UTC · Completed 7:07 AM UTC
Commit: 5793196 · View workflow run →

@fullsend-ai-review fullsend-ai-review Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the review comment for full details.

Comment thread scripts/post-code-test.sh
@rh-hemartin
rh-hemartin added this pull request to the merge queue Jul 31, 2026
Merged via the queue into main with commit 95a6b43 Jul 31, 2026
14 checks passed
@rh-hemartin
rh-hemartin deleted the agent/483-defer-target-branch branch July 31, 2026 07:27
@fullsend-ai-retro

fullsend-ai-retro Bot commented Jul 31, 2026

Copy link
Copy Markdown

🤖 Finished Retro · ✅ Success · Started 7:29 AM UTC · Completed 7:40 AM UTC
Commit: 5793196 · View workflow run →

@fullsend-ai-retro

Copy link
Copy Markdown

Retro: PR #488 — fix(#483): defer target_branch until after default branch discovery

The workflow for issue #483 / PR #488 was largely successful. The code agent produced a solid 4-file fix with 8 test cases in ~8 minutes. The review agent found actionable issues (documentation-coherence, edge-case test coverage) that led to concrete code improvements via the fix agent. The human reviewer added value by requesting branch discovery command reordering (prioritizing gh repo view over git commands) and managing two rebases due to upstream changes.

Timeline summary: 8 agent runs (1 triage, 1 code, 4 fix, 2 review) over 3 days, with ~1 hour of active agent/human work on day 1. One fix run failed due to infrastructure (not agent error). PR merged after human approval.

Fix run 4 failure (infrastructure): The fix agent's rebase and push succeeded, but process-fix-result.py failed because it resolved the schema path (scripts/../schemas/fix-result.schema.json) relative to its own location in the content-addressed cache, where scripts/ and schemas/ live under different sha256 directories. This is already tracked by #551. Additionally, the failure comment didn't distinguish that the push had already succeeded — evidence supporting #534.

Review nondeterminism: The second review (on identical code for the relevant file) surfaced a new Medium finding ([behavior-description-incomplete] on docs/code.md:44) that the first review missed. This is the nondeterministic coverage pattern tracked by #511.

Documentation gap merged unaddressed: The review agent's second pass correctly identified that docs/code.md describes CODE_ALLOWED_TARGET_BRANCHES behavior as fail-on-mismatch, but the PR changed it to auto-correct-to-default. The human approved without addressing this finding — a conscious trade-off. The code agent not proactively updating this documentation is consistent with existing issues fullsend#4866 and fullsend#1733 about code agent documentation updates.

Branch discovery and sandbox compatibility: The human's first intervention was reordering branch discovery to prioritize gh repo view (API-based) over git rev-parse (git-based). This aligns with #489, which notes that git commands may not work under sandbox network policies.

No novel proposals. All identified improvement opportunities are already tracked by existing open issues. The workflow demonstrates that the triage → code → review → fix pipeline works well for this class of bug fix.

ggallen pushed a commit that referenced this pull request Aug 19, 2026
Reorder branch discovery commands in SKILL.md so `gh repo view` is tried
first, followed by `git rev-parse` and `git symbolic-ref` as fallbacks.

Addresses review feedback on #488
ggallen pushed a commit that referenced this pull request Aug 19, 2026
- Reword documentation-coherence issue in SKILL.md: replace "Do not
  hardcode main as a fallback" with "Do not skip discovery and assume
  main" to align with the ${DEFAULT_BRANCH:-main} fallback in the jq
  template (documentation-coherence finding).
- Add substring-not-accepted test case to verify comma-wrapping
  rejects partial matches like "release" when only "release-1" and
  "release-2" are in the allowed list (edge-case finding).

Addresses review feedback on #488
ggallen pushed a commit to ggallen/agents that referenced this pull request Aug 19, 2026
Reorder branch discovery commands in SKILL.md so `gh repo view` is tried
first, followed by `git rev-parse` and `git symbolic-ref` as fallbacks.

Addresses review feedback on fullsend-ai#488
ggallen pushed a commit to ggallen/agents that referenced this pull request Aug 19, 2026
- Reword documentation-coherence issue in SKILL.md: replace "Do not
  hardcode main as a fallback" with "Do not skip discovery and assume
  main" to align with the ${DEFAULT_BRANCH:-main} fallback in the jq
  template (documentation-coherence finding).
- Add substring-not-accepted test case to verify comma-wrapping
  rejects partial matches like "release" when only "release-1" and
  "release-2" are in the allowed list (edge-case finding).

Addresses review feedback on fullsend-ai#488
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working code-agent requires-manual-review Review requires human judgment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(code): defer target_branch until after default branch discovery

1 participant