Skip to content

Fix agent lacks an upfront threat-model checklist for secret-scanning merge commits #1547

Open

Description

@fullsend-ai-retro

What happened

PR #1520 (implementing issue #1518's forge-conflict reconciliation, which introduces merge-based conflict resolution in the fix agent) went through 15 review-agent rounds. Of those, 6 rounds (1, 2, 7, 8, 9, 12) each surfaced a new, narrower gitleaks/secret-scanning bypass specific to merge commits, discovered one at a time rather than anticipated together: round 1 found the merge secret-scan used an unscoped two-dot diff range; round 2 found the pre-commit-gate autofix re-scan still used that same unscoped range; round 7 found a GitLab reconstructed-merge trust gap plus PUSH_TOKEN log exposure; round 8 found that switching to git show --cc for combined-diff scanning silently omits any path that cleanly matches either parent (e.g. a newly-added file with no conflict is never scanned); round 9 found the round-8 fix only validated the second parent's trust, so a 3+-parent octopus merge with an untrusted third parent still bypassed the scan; round 12 found that merging old, already-trusted history could still reintroduce since-deleted secrets undetected. Each round required a full review ($5-10) plus fix cycle before the next edge case surfaced. See the review-agent's sticky comment on PR #1520 (#1520) for the full round-by-round record.

What could go better

I'm fairly confident this is a real, addressable gap rather than normal iteration noise: the same underlying question (how to correctly secret-scan a merge commit) was re-litigated 6 separate times in one PR, each time narrowing to a more specific edge case, which is the signature of a missing threat-model/checklist rather than of genuinely unpredictable requirements. I checked for existing coverage and found none -- the closest adjacent issues are #318 (gitleaks scan-range correctness after rebase, not merge-parent trust), #1518 (the parent issue that spawned this PR, whose acceptance criteria never mention secret scanning), and the fix-agent skill-decomposition epic #1521/#1522 (organizational refactor, doesn't address this content gap). This is speculative in the sense that I can't prove the code agent would have gotten it right in fewer rounds even with a checklist -- merge-commit secret scanning is inherently subtle -- but codifying the four concrete pitfalls found here should catch at least the first 2-3 rounds' worth of findings on the next similar PR.

Proposed change

Add a 'Merge-commit secret-scanning checklist' to the fix-agent guidance that governs conflict/merge handling (skills/fix-review/SKILL.md or agents/fix.md, whichever ends up owning merge-commit logic after the #1521/#1522 skill decomposition). Given the skillsaw token-budget pressure already documented in open issue #1414, prefer adding this as a new file under a references/ directory (linked from the skill) rather than inline prose, to avoid worsening that budget problem. The checklist should cover, at minimum, the four pitfalls found on PR #1520: (1) git show --cc/combined-diff scanning omits paths that don't conflict with any parent -- scan each parent-diff separately or union git diff <merge>^1..<merge> across all parents; (2) every non-first parent of a multi-parent (octopus) merge must have its trust independently validated, not just the second; (3) a merge can reintroduce old secrets from trusted-but-secret-containing history even when the merge itself adds no new diff lines; (4) ancestry/ trust checks used for skip-logic must be re-validated after any fetch, since a fetch can move the remote tip past the point the check was computed against. Link PR #1520 as the canonical example for each pitfall.

Validation criteria

On the next fix-agent PR (or the #1522 history-rewrite skill extraction, if it lands first) that introduces or modifies merge-commit-based conflict resolution or any gitleaks/secret-scan logic touching merges, the review agent should not raise more than 1-2 distinct secret-scan-bypass findings before reaching a clean verdict on that category, compared to the 6 rounds observed on PR #1520. Track this by counting review rounds tagged with a secret-exposure/secret-scan-bypass finding on the next such PR within 90 days.


Generated by retro agent from #1520

Activity

  1. fullsend-ai-triage commented on Sep 29, 2026

    @fullsend-ai-triage

    🤖 Finished Triage · ✅ Success · Started 10:46 AM UTC · Completed 10:50 AM UTC

    Commit: 513b6e4 · View workflow run →

    Runtime: claude · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $0.51

  2. fullsend-ai-triage commented on Sep 29, 2026

    @fullsend-ai-triage

    This retro-generated request is well-documented and directly actionable: it names four concrete, previously-discovered pitfalls (combined-diff parent-skew, unvalidated octopus-merge parent trust, stale-secret reintroduction via trusted history, and post-fetch trust re-validation) and links PR #1520's round-by-round record as evidence.

    One placement question the issue itself raised has been resolved from repository context: the merge-conflict reconciliation procedure currently lives in agents/fix.md (skills/fix-review/SKILL.md only points to it), and the repo already has a precedent for relocating detailed reference material into skills/fix-review/references/ to stay under the skillsaw token budget (see how references/timing.md was carved out while resolving #1414). The recommended approach is a new file under skills/fix-review/references/, linked from both agents/fix.md and skills/fix-review/SKILL.md, rather than inlining the checklist into either.

    Related context checked: PR #1520 is merged and #1518 is resolved, so there is no open blocker. The #1521/#1522 skill-decomposition epic is still open but covers pure prose relocation (rebase/rewrite procedures) and does not overlap with this content gap, so it is not a hard prerequisite for adding the checklist now. No duplicate issue or in-flight PR covers this proposal.


    Labels: Matches repo convention for retro-agent-originated checklist/guidance requests targeting the fix agent (e.g. #1069, #1264, #232): documentation for the checklist addition, security for the secret-scanning subject matter, and fix-agent/retro-agent to track ownership and origin.


    Next steps:

    • /fs-code — agent creates a PR to implement this issue
    • /fs-code <your instruction> — agent implements with your specific guidance
  3. fullsend-ai-coder commented on Sep 29, 2026

    @fullsend-ai-coder
    Contributor

    🤖 Finished Code · ✅ Success · Started 10:52 AM UTC · Completed 11:05 AM UTC

    Commit: 513b6e4 · View workflow run →

    Runtime: pi · Model: xai/grok-4.6 · Effort: high · Cost: $6.68

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions