feat(#188): 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 step 3b with procedural
  commands for detecting divergence, merging the base branch,
  resolving conflicts, and verifying the merge

---

Closes #188

Post-script verification

• Branch is not main/master (agent/188-fix-merge-conflict-ordering)
• Secret scan passed (gitleaks — f96750babbed5ada406a9ae04e8068449701d9c7..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]

Review

Findings

Medium

• [procedure-ordering] internal/scaffold/fullsend-repo/skills/fix-review/SKILL.md — The new merge-conflict section invokes scan-secrets <resolved-files> between steps 3 and 4, but the existing secret-scanning setup instruction (lines 34–41) says to verify scan-secrets exists "Before starting step 7." The agent has not yet verified the tool exists when the merge-conflict section asks it to run. If scan-secrets is missing from the sandbox image, the agent would fail with an unhelpful error rather than the clean hard-stop specified in the agent definition.
  Remediation: Move the command -v scan-secrets existence check earlier — either to step 1 or to the beginning of the merge-conflict section — with the same hard-stop behavior.

• [missing-capability-doc] docs/agents/fix.md:5 — The user-facing fix agent description says "implements targeted fixes, runs tests and linters, and commits the result" without mentioning merge conflict resolution. This PR adds that capability to the agent definition and skill procedure but does not update the user-facing documentation.
  Remediation: Update docs/agents/fix.md line 5 and the "How the agent works" section to mention merge conflict resolution.

Low

• [internal-consistency] internal/scaffold/fullsend-repo/skills/fix-review/SKILL.md — The conditions for implicit merge triggering differ between fix.md and SKILL.md. The agent definition triggers merge when "the branch is behind the base branch and other tasks are also requested." The SKILL.md procedure triggers when "the branch is behind and has actual merge conflicts." These are different conditions: a branch 10 commits behind main with no textual conflicts but a /fs-fix fix the nil check instruction would trigger under fix.md but skip under SKILL.md. The SKILL.md constraints section (line 468) says fix.md wins in conflicts, which provides a safety net. See also: [scope-clarity] finding.

• [edge-case] internal/scaffold/fullsend-repo/skills/fix-review/SKILL.md — When the only task is merge-conflict resolution (e.g., /fs-fix resolve merge conflicts with no review findings), the agent must still produce a valid fix-result.json with at least one action (minItems: 1). The new section does not explain how to record merge-conflict resolution in structured output.
  Remediation: Add a note to step 9 that merge-conflict resolution should be recorded as a fix action.

• [scope-clarity] internal/scaffold/fullsend-repo/agents/fix.md — The "Task ordering" section lists "rebase" as a trigger word (line ~198), but git rebase is in disallowedTools (line 17). The ambiguity is cosmetic — the harness hard-enforces disallowedTools and the SKILL.md procedure only shows git merge commands — but a reader unfamiliar with the enforcement mechanism could be confused.
  Remediation: Add a parenthetical noting that rebase requests are fulfilled via merge.

• [naming-consistency] internal/scaffold/fullsend-repo/skills/fix-review/SKILL.md:206 — The progress marker ::notice::Merge conflict check doesn't follow the STEP N: <title> convention used by all other progress markers in this file (STEP 1, STEP 2, STEP 4, etc.).
  Remediation: Use ::notice::STEP 3b: Merge conflict resolution.

• [process-adherence] internal/scaffold/fullsend-repo/skills/fix-review/SKILL.md — The build check (step 3 of the merge section) runs after the merge is committed (step 2.iv). If the build fails, the agent has committed a broken merge. Practical risk is limited since the post-script controls the push, but reordering would be cleaner.

• [stale-status-reference] docs/guides/user/bugfix-workflow.md:24 — Pre-existing: the document describes the fix agent as "planned" future work, but the agent already exists with a full definition and skill procedure. Not introduced by this PR, but this is a good moment to address it.

Previous run

Review

Findings

Medium

• [api-contract] internal/scaffold/fullsend-repo/skills/fix-review/SKILL.md — Step 3b introduces a new commit (the merge commit) but does not run scan-secrets before committing. The agent definition states "Secret scanning is non-negotiable" and "the scan-secrets helper runs before tests on every verification pass." During manual conflict resolution, the agent edits files with Write — those edits could inadvertently introduce secrets that the agent should catch immediately rather than deferring to the post-script.
  Remediation: Add a scan-secrets <resolved-files> call after conflict resolution (sub-step 3) and before the merge commit in step 3b.

• [scope-exceeded] internal/scaffold/fullsend-repo/agents/fix.md — The new "Task ordering" section expands the fix agent's responsibility to include merge conflict detection and resolution. The agent's description defines it as a "Review-feedback specialist" that "implements targeted fixes." Merge conflict resolution is a branch synchronization task, not review feedback. Issue Fix agent should resolve merge conflicts as a prerequisite before code changes #188 authorizes this change, but the agent's description and purpose statement are not updated to reflect the expanded responsibility. This also introduces runtime orchestration logic inside the agent skill (conditional merge-first ordering), whereas ADR 0018 establishes scripted pipelines for deterministic sequencing.
  Remediation: If merge handling stays in the fix agent, update the agent's description (lines 3–7) to reflect this expanded responsibility. Consider whether the merge-first decision could be made in the dispatch workflow layer instead.

Low

• [edge-case] internal/scaffold/fullsend-repo/skills/fix-review/SKILL.md — Step 3b's sub-steps 2–4 (resolve conflicts, verify, stage and commit) are listed sequentially without branching on whether the merge in sub-step 1 succeeded cleanly or produced conflicts. When git merge completes without conflicts, the merge commit is created automatically — sub-steps 2–4 would be redundant. While an LLM agent will likely handle this conditional flow, making it explicit would improve clarity.
  Remediation: Wrap sub-steps 2–4 in an explicit conditional: "If the merge produced conflicts (non-zero exit):" followed by the resolve/verify/commit steps.

• [internal-consistency] internal/scaffold/fullsend-repo/skills/fix-review/SKILL.md — The progress markers section (line 51) lists markers at steps 1, 2, 4, 7a, 7b, 7c, and 8. The new step 3b — which performs significant work (git fetch, merge, conflict resolution, build verification) — has no corresponding progress marker.
  Remediation: Add step 3b to the progress markers list and add echo "::notice::STEP 3b: Resolve merge conflicts" at the beginning of step 3b.

• [fail-open] internal/scaffold/fullsend-repo/skills/fix-review/SKILL.md — The disallowedTools list blocks git rebase but not git merge. Per ADR 0027, disallowedTools serves steering (not security), and git rebase is blocked to prevent history rewriting that would break plain push. The asymmetry is intentional by design, but worth noting for consistency awareness.

• [naming-convention] internal/scaffold/fullsend-repo/skills/fix-review/SKILL.md:202 — The new step is numbered "3b" but step 3 has no "3a" substep, and the content (merge conflicts) is unrelated to step 3 (repo conventions). The sequence 1, 2, 3, 3b, 4 is inconsistent with the existing flat numbering pattern.
  Remediation: Renumber as step 4 and shift subsequent steps, or use a self-contained heading that does not imply a substep relationship to step 3.

• [pattern-inconsistency] internal/scaffold/fullsend-repo/agents/fix.md — The standalone "Steps:" heading with a colon is novel in the scaffold agent files. Other sections use H2/H3 headings or direct numbered lists without a colon-terminated label.
  Remediation: Remove the "Steps:" line and place the numbered list directly under the H2 heading.

Previous run (2)

Review — comment

The PR correctly implements the merge-conflict-first ordering requested in #188. The architectural split between the agent definition (behavioral rule with rationale) and the skill procedure (step-by-step commands) follows established project patterns. Style, documentation currency, and security all check out clean. Two medium-severity consistency issues are worth addressing before or after merge.

Findings

1. Cross-document inconsistency on implicit merge trigger scope · medium

Files: internal/scaffold/fullsend-repo/agents/fix.md · internal/scaffold/fullsend-repo/skills/fix-review/SKILL.md

fix.md defines an "implicit" merge trigger that fires when "the branch is behind the base branch and other tasks are also requested." This is broad — any /fs-fix with multiple tasks on a behind branch would trigger merging. SKILL.md step 3b is narrower: it only merges when explicitly requested or when the branch is behind and has actual conflicts. It adds an escape hatch: "If the branch is behind but merge conflict resolution was not requested and there are no actual conflicts, note the divergence but proceed."

These two instructions genuinely conflict for the case where the branch is behind, there are no actual merge conflicts, but multiple tasks are requested. The skill's own constraints section (line 468) says "the agent definition wins," which makes the SKILL.md conditional escape effectively dead code.

Remediation: Either narrow the implicit trigger in fix.md to match SKILL.md's condition (behind + actual conflicts), or remove the conditional skip from SKILL.md so both documents agree on always-merge-when-behind behavior. The issue description suggests the conditional (SKILL.md) behavior is the intended one.

2. No guidance for merge conflicts in protected-path files · medium

File: internal/scaffold/fullsend-repo/skills/fix-review/SKILL.md (step 3b, sub-step 2)

Step 3b instructs the agent to resolve conflicts using Read and Write. However, the existing protected-paths rule in fix.md (lines 95–117) says: "Never modify files under any of the following paths, even if they appear in merge conflicts." Protected paths include .github/, agents/, skills/, etc.

If a merge conflict occurs in a protected-path file, the agent cannot resolve it (blocked by protected-paths) but also cannot complete the merge (unresolved conflicts prevent committing). Step 3b gives no guidance for this deadlock.

Remediation: Add a clause to step 3b: if a merge conflict occurs in a protected-path file, accept the base branch version (e.g., git checkout --theirs <file>) or stop and note the conflict in structured output for human resolution.

3. git diff --check comment overstates its purpose · low

File: internal/scaffold/fullsend-repo/skills/fix-review/SKILL.md (step 3b, sub-step 3)

The inline comment # no conflict markers implies git diff --check only detects conflict markers. It also flags whitespace errors (trailing whitespace, mixed tabs/spaces), which could cause a non-zero exit unrelated to conflicts. Since the command runs before staging (step 3 before step 4), the ordering is correct and the conflict-marker detection works — the comment is just slightly misleading.

Remediation: Amend the comment to # checks for conflict markers and whitespace errors or replace with an explicit grep for <<<<<<< markers.

4. Missing progress marker for step 3b · low

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

The progress markers section lists steps 1, 2, 4, 7a, 7b, 7c, and 8 as requiring echo "::notice::STEP N: title" markers. Step 3b involves substantial work (fetching, merging, conflict resolution, build verification) but has no progress marker, and the markers list was not updated.

Remediation: Add echo "::notice::STEP 3b: Resolve merge conflicts" at the start of step 3b and update the progress markers list.

5. PR title uses feat but the change is an internal behavior fix · low

File: PR title / commit message

Per COMMITS.md, feat is reserved for user-facing functionality. "Tightening internal heuristics, adjusting prompts, or tuning agent behavior that users don't directly control" should use refactor or fix. Since issue #188 describes wasted agent time from incorrect task ordering, fix(#188) is more accurate.

Remediation: Rename PR title and squash-commit message to fix(#188): add merge-conflict-first ordering to fix agent.

[guyoron1]

/fs-review

[guyoron1]

/fs-fix

[fullsend-ai-coder]

🔧 Fix agent — iteration 1 (human-triggered)

Addressed 6 of 7 review findings. Fixed: missing scan-secrets before merge commit, updated agent description for expanded responsibility, added conditional branching for clean merges, added progress marker, removed novel heading style, fixed step numbering. Disagreed with 1 finding (disallowedTools asymmetry is intentional by design).

Fixed (6):

1. Missing scan-secrets before merge commit in step 3b (internal/scaffold/fullsend-repo/skills/fix-review/SKILL.md): Added scan-secrets call as sub-step 2.2 within the conflict resolution conditional, after resolving conflicts and before verifying/committing the merge.
2. Agent description not updated for merge conflict responsibility (internal/scaffold/fullsend-repo/agents/fix.md): Updated both the frontmatter description and the body purpose statement to mention merge conflict resolution as part of the agent's responsibilities.
3. Sub-steps 2-4 not wrapped in conditional for clean merge case (internal/scaffold/fullsend-repo/skills/fix-review/SKILL.md): Restructured step 3b so sub-step 2 is explicitly conditional on merge producing conflicts (non-zero exit). When merge succeeds cleanly, the commit is automatic and only build verification (step 3) runs.
4. Missing progress marker for merge conflict check (internal/scaffold/fullsend-repo/skills/fix-review/SKILL.md): Added progress marker echo at the start of the merge conflict section and updated the progress markers list to reference it.
5. Step 3b numbering inconsistent (no 3a exists) (internal/scaffold/fullsend-repo/skills/fix-review/SKILL.md): Changed heading from '### 3b. Resolve merge conflicts (prerequisite)' to '### Resolve merge conflicts (conditional — after step 3)' to remove the substep numbering that implied a non-existent 3a.
6. Novel 'Steps:' heading in scaffold agent files (internal/scaffold/fullsend-repo/agents/fix.md): Removed the standalone 'Steps:' label, placing the numbered list directly after the preceding paragraph as done in other sections.

Disagreed (1):

1. git rebase blocked but not git merge (asymmetry in disallowedTools): The reviewer noted this as intentional by design ('worth noting for consistency awareness'). No remediation was suggested. The asymmetry is correct: rebase rewrites history breaking plain push, while merge preserves history and is needed for the merge-conflict-first workflow.

Tests: passed

_{Updated by fullsend fix agent}

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

[low] naming-consistency

The progress marker '::notice::Merge conflict check' doesn't follow the 'STEP N: title' convention used by all other progress markers in this file.

Suggested fix: Use '::notice::STEP 3b: Merge conflict resolution'.