feat(#244): add merge-conflict-first ordering to fix agent

[fullsend-ai-coder]

When a /fs-fix instruction includes merge conflict resolution, the fix agent now resolves the merge before making any other code changes. This prevents redundant fix iterations caused by stale-branch conflicts.

Changes:

• agents/fix.md: new Task ordering section establishing the
  behavioral rule and its rationale
• skills/fix-review/SKILL.md: new conditional step after step 3
  with procedural commands for detecting divergence, merging the
  base branch, resolving conflicts, and verifying the merge

---

Closes #244

Post-script verification

• Branch is not main/master (agent/244-fix-merge-conflict-ordering)
• Secret scan passed (gitleaks — c776c248ac9a91f10998ca7107e974a6c9d76514..HEAD)
• Pre-commit hooks passed (authoritative run on runner)
• Tests ran inside sandbox

[github-actions]

E2E tests did not run

E2E tests run automatically for org/repo members and collaborators on pull requests.

For other contributors, a maintainer must add the ok-to-test label after the latest push.

See E2E testing guide for details.

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 12:58 PM UTC · Completed 12:58 PM UTC
Commit: c776c24 · View workflow run →

[guyoron1]

/fs-review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 3:57 PM UTC · Completed 4:08 PM UTC
Commit: d8e3df7 · View workflow run →

[fullsend-ai-review]

Review — comment

Well-scoped change that directly addresses the incident described in #244. The split between behavioral guidance in agents/fix.md and procedural commands in skills/fix-review/SKILL.md follows the existing architectural pattern. Two medium findings below — neither is blocking, but both would improve the procedure's reliability.

Findings

1. Contradictory merge-skip logic when BEHIND == 0 and human explicitly requests merge — medium

File: internal/scaffold/fullsend-repo/skills/fix-review/SKILL.md

The procedure states:

If the human instruction explicitly mentions merge conflicts, rebase, or merging the base branch, this is mandatory regardless of BEHIND count.

But later:

If the branch is up to date (BEHIND == 0), skip to step 4.

These contradict each other when BEHIND == 0 and the human said /fs-fix resolve merge conflicts. An AI agent reading sequentially would likely follow the last instruction (skip), ignoring the explicit human request.

Remediation: Qualify the skip instruction: "If the branch is up to date (BEHIND == 0) and the human instruction does not explicitly mention merge conflict resolution, skip to step 4."

2. No error-handling guidance for git fetch / git merge non-conflict failures — medium

File: internal/scaffold/fullsend-repo/skills/fix-review/SKILL.md

The procedure runs git fetch origin "${BASE_BRANCH}" without describing what to do if it fails (network error, deleted branch, auth issue). A silent git fetch failure would leave a stale remote-tracking ref, causing git rev-list --count to produce an incorrect BEHIND count — the agent might skip a needed merge or attempt one against the wrong state. Similarly, git merge can fail for reasons beyond conflicts (shallow clone, unrelated histories) that the procedure doesn't address.

Remediation: Add a note: "If git fetch fails, emit a warning and skip the merge step — the post-script's push will surface the divergence. If git merge fails with an error other than merge conflicts, stop and report the failure."

3. Redundant API call for baseRefName — low

File: internal/scaffold/fullsend-repo/skills/fix-review/SKILL.md

Step 1 already fetches baseRefName via gh pr view "${PR_NUMBER}" --json number,title,body,headRefName,baseRefName,.... The merge-conflict step fetches it again with a separate gh pr view call. This wastes an API request and introduces a subtle inconsistency risk if the PR is retargeted between the two calls.

Remediation: Instruct the agent to reuse the baseRefName from the step 1 output rather than making a second API call.

4. Frontmatter description understates actual behavior — low

File: internal/scaffold/fullsend-repo/agents/fix.md

The frontmatter description says "resolves merge conflicts when requested" but the SKILL.md procedure triggers merge resolution automatically whenever BEHIND > 0, even on bot-triggered runs where nobody requested it. The "Task ordering" section does explain the implicit trigger, but the brief description could mislead someone scanning agent definitions.

Remediation: Consider "resolves merge conflicts when the branch is behind the base branch" to match the actual behavior.

Previous run

Review

Verdict: comment · 1 medium, 2 low

This PR adds merge-conflict-first ordering to the fix agent, implementing the guidance proposed in #244. The change is well-scoped: the agent definition (fix.md) states the behavioral principle, and the skill procedure (SKILL.md) provides the implementation steps — matching the established architecture. The procedural commands (fetch, merge, scan, verify, commit) are correctly ordered and use permitted tools.

Three findings worth noting, none blocking.

---

Findings

1. Merge procedure does not reference protected-path constraints

Severity: medium · Category: edge-case · File: internal/scaffold/fullsend-repo/skills/fix-review/SKILL.md

The new merge-conflict procedure instructs the agent to "Preserve the PR's intended changes while incorporating updates from the base branch" when resolving conflicts. However, it does not reference the existing protected-path constraints from agents/fix.md (lines 93–117): "Never modify files under any of the following paths, even if they appear in merge conflicts."

When a merge from main produces conflicts in protected-path files (.github/, CLAUDE.md, agents/, skills/, etc.), the agent has no explicit guidance in this procedure to skip or accept the base branch version for those files. If the agent resolves conflicts in protected paths, post-fix.sh discards the entire run — including legitimate non-protected-path work — wasting the agent's iteration budget.

The existing constraint in the agent definition technically governs (agent definition wins over skill procedure), but reinforcing it in the merge procedure would prevent the failure mode this PR is designed to eliminate: wasted fix iterations.

Remediation: Add a step before conflict resolution: "If merge conflicts involve protected-path files (see Protected paths section), accept the base branch version of those files (git checkout origin/${BASE_BRANCH} -- <protected-path>) rather than attempting to resolve them."

2. Agent-level and skill-level guidance diverge on behind-but-no-conflicts case

Severity: low · Category: internal-consistency · Files: internal/scaffold/fullsend-repo/agents/fix.md, internal/scaffold/fullsend-repo/skills/fix-review/SKILL.md

The agent definition says to resolve the merge "implicitly (the branch is behind the base branch and other tasks are also requested)." The skill procedure says: "If the branch is behind but merge conflict resolution was not requested and there are no actual conflicts, note the divergence but proceed with the requested fixes."

These contradict each other when the branch is behind, the human didn't explicitly ask for a merge, and there are no conflicts. The agent definition says merge; the skill says skip. Per the skill's own rule ("If a step in this skill appears to conflict with the agent definition, the agent definition wins"), the agent definition prevails — but contradictory guidance may cause the LLM to pick the wrong interpretation.

Remediation: Align the skill text. When BEHIND > 0, always perform the merge (it completes cleanly if there are no conflicts, so there's no cost). Only skip the section when BEHIND == 0.

3. Unsanitized BASE_BRANCH in GHA notice output

Severity: low · Category: gha-workflow-command-injection · File: internal/scaffold/fullsend-repo/skills/fix-review/SKILL.md

echo "Branch is ${BEHIND} commits behind origin/${BASE_BRANCH}" interpolates BASE_BRANCH (from gh pr view --json baseRefName) into a GHA log line. Git ref names can contain :: sequences (permitted by git check-ref-format), which could inject GHA workflow commands like ::warning:: or ::error::. Practical risk is low — an attacker needs write access to the repo to create the base branch, and dangerous commands (::set-env::) are disabled by default — but this codebase's own security review agent specifically checks for this pattern.

Remediation: Sanitize before echoing: echo "Branch is ${BEHIND} commits behind origin/${BASE_BRANCH//::/_}", or avoid interpolating BASE_BRANCH into echo output entirely.

[guyoron1]

/fs-fix

[fullsend-ai-coder]

🤖 Finished Fix · ✅ Success · Started 5:38 PM UTC · Completed 5:47 PM UTC
Commit: d8e3df7 · View workflow run →

[fullsend-ai-coder]

🔧 Fix agent — iteration 1 (human-triggered)

Addressed all 3 review findings (1 medium, 2 low). Added protected-path awareness to merge procedure, aligned behind-branch handling between agent definition and skill, and sanitized branch name in echo output.

Fixed (3):

1. Merge procedure does not reference protected-path constraints (internal/scaffold/fullsend-repo/skills/fix-review/SKILL.md): Added step before conflict resolution to accept the base branch version for protected-path files via git checkout, with a warning that modifying them causes post-fix.sh to discard the entire run
2. Agent-level and skill-level guidance diverge on behind-but-no-conflicts case (internal/scaffold/fullsend-repo/skills/fix-review/SKILL.md): Aligned skill procedure with agent definition: when BEHIND > 0, always merge (clean merge completes instantly). Removed the skip-if-not-requested clause that contradicted the agent definition
3. Unsanitized BASE_BRANCH in GHA notice output (internal/scaffold/fullsend-repo/skills/fix-review/SKILL.md): Applied bash parameter substitution (${BASE_BRANCH//::/_}) to strip :: sequences before echoing, preventing GHA workflow command injection

Tests: passed

_{Updated by fullsend fix agent}

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 5:50 PM UTC · Completed 6:01 PM UTC
Commit: d8e3df7 · View workflow run →