Skip to content

Port the rebase conflict-marker scan to Go #714

Description

@tbhb-agent

Problem

packages/agents/source/skills/rebase/shared/scripts/marker-scan.sh scans paths for leftover conflict markers. continue-rebase.sh, rebase-status.sh and verify-rebase.sh call it, each masking its exit status with || true and deciding from whether its standard output is empty. It is a standalone shell algorithm, so moving it to tested Go removes a fork-heavy path and one more shell script from the rebase workflow (feature 13 of #588, the rebase and conflict-resolution shell). The output contract is what keeps a rebase from being declared clean while markers remain, so it must survive exactly, and a scanner that cannot run must not look like a clean tree to callers that ignore its status.

Acceptance criteria

  1. AC1: Given a file with lines that begin with <<<<<<<, >>>>>>> or ||||||| followed by a space or the end of the line, when it is scanned, then the file is reported with the count of those lines; a bare ======= line, a marker not at the start of a line, and marker characters followed by other text are not counted.
  2. AC2: Given a path ending in skills/resolve-rebase-conflicts/SKILL.md, or a path whose conflict-markers Git attribute is documented, when it is scanned, then it is not reported even if it contains marker lines.
  3. AC3: Given the attribute lookup fails or returns any other value, when a path is scanned, then the path is treated as not exempt: it is reported if it contains markers and not reported if it does not, and a failed lookup never exempts it.
  4. AC4: Given a file whose index content differs from its working tree content, when it is scanned with --staged, then the report reflects the index content, and without --staged it reflects the working tree content; a path missing from the index (staged mode) or not a regular file (worktree mode) is skipped. In both modes the conflict-markers attribute is resolved the way ordinary Git attribute lookup does, with the working tree .gitattributes first and the index copy used only when that file is absent from the working tree: given a .gitattributes whose staged and working tree versions disagree about a path, then the working tree version decides whether that path is exempt, in either mode; given a .gitattributes that is in the index but absent from the working tree, then the index version decides.
  5. AC5: Given any set of paths, when the scan finishes, then it prints one line <path> <n> marker line(s) per reported file in argument order and exits 1, or prints nothing and exits 0 when nothing is reported; empty path arguments are skipped, and with no path it prints the usage line on standard error and exits 2.
  6. AC6: Given a path that exists but cannot be read, when it is scanned, then it produces no line and does not change the exit status, as today (the choice is to preserve current behaviour; hardening it is a separate follow-up).
  7. AC7: Given the Go scanner is unavailable or fails to run, when a caller invokes the scanner entry point, then a line naming the failure appears on standard output and the exit status is non-zero, so each of the three callers reports a problem instead of a clean tree.
  8. AC8: Given the rebase workflow suite, when it runs against the migrated entry point, then it passes unchanged, so the three callers pass the same arguments and receive the same results.
  9. AC9: After the documented generation sequence (mise run generate-skills, then mise run skill-tokens, then apm install), the committed generated outputs for the rebase skill match their sources and mise run check-generated-skills passes.
  10. AC10: Non-functional: scanning 10,000 tracked files of 50 lines each, with no exemptions and a few files carrying markers, in worktree mode through the entry point on the CI runner, finishes in under 2 s.

Scope and non-goals

In scope: the marker scanner, its compatibility entry point and the test that extracts its exempt function.
Non-goal: the separate marker check inside guard-resolve.sh; it stays as it is and is covered by the guard migration (#722).
Non-goal: rebase orchestration (preflight, start, continue, verify, status), a later wave.
Out of scope: changing which markers or exemptions count, or the callers' own masking of the scanner's status.

Approach

Add a scan subcommand to the Go agentrepo command tree that implements the recognition, exemption and staged versus worktree rules above. Batch the attribute lookups (one Git call for all paths, using Git's ordinary attribute resolution as the current script's git check-attr does: working tree attribute files first, the index copy only when the working tree file is absent, and no cached-index option) so AC10 holds, treating a failed batch as no exemptions (AC3). Port the scanner cases of the Bats suites to Go tests as the oracle. The case "marker exemptions reject documented attributes printed before git fails" in tests/skills/source/lib/masked-status.bats extracts the shell function exempt from marker-scan.sh, so removing that function breaks the harness: move its marker-scan half to a Go test with a failing attribute reader and keep the guard-resolve.sh half. Keep marker-scan.sh as a forwarding shim of at most 10 lines that runs the Go command, prints a failure line on standard output and exits non-zero when the command is missing or fails to start (AC7), and delete the shell algorithm. The three callers and their arguments stay as they are. Regenerate skills from canonical sources in the documented order. The existing rebase coverage is one workflow suite (tests/skills/rebase/scripts/rebase-workflow.bats), not three caller suites. Neither the script nor the new Go files match the owner-protected path patterns, so no protection change is needed.

Dependencies

Blocked by #333 (binary release assets) and #544 (consumer installation of those assets), both as native links: the shim runs in consumer repositories, where the Go command exists only once it is published and installed. This also follows #588's sequence of installing binaries before deploying callers. No dependency on its siblings.

Size

S. One PR: the Go scanner, its tests, the shim, the harness change and regenerated outputs.

Verify plan

  • AC1: Go table tests over marker boundaries (each of the three markers with a space and with end of line; =======; indented; mid-line; marker followed by other text) plus the existing scan cases.
  • AC2: Go tests for the built-in SKILL.md path and for a documented attribute, each with marker lines present.
  • AC3: Go tests with a stubbed attribute reader that fails after printing documented, returns another value, and returns nothing: a file with markers is reported and a clean one is not; plus the migrated masked-status case.
  • AC4: Go test with a fixture repository whose index and working tree differ, run in both modes, including a path absent from the index and a directory; plus three attribute fixtures run in both modes, each containing a file with marker lines: (a) the staged .gitattributes marks the file documented but the working tree .gitattributes does not, so the file is reported; (b) the working tree .gitattributes marks it documented but the staged one does not, so the file is not reported. (c) the .gitattributes marks the file documented in the index only and is deleted from the working tree, so the file is not reported. A scanner that read attributes only from the index would fail (a) or (b), and one that read only the working tree would fail (c).
  • AC5: golden-output test comparing the shim's standard output, standard error and exit status with the recorded results of the old script on the same fixtures, covering no findings, one and several findings, repeated and empty arguments, and no arguments.
  • AC6: Go test with a stubbed reader that fails for one path, asserting no output line and the unchanged exit status.
  • AC7: Bats test that runs the shim with the Go command absent and with a command that exits non-zero, asserting a standard output line and a non-zero status, then runs each of the three callers against it and asserts each reports a problem.
  • AC8: tests/skills/rebase/scripts/rebase-workflow.bats passing against a shim backed by the branch binary.
  • AC9: the documented sequence run on the branch, git diff --exit-code afterwards, and mise run check-generated-skills.
  • AC10: a timed test that builds the 10,000-file fixture, runs the shim, and fails if the wall time is 2 s or more.

Risks

A missed marker or a false exemption would let a broken rebase pass; AC1 to AC4 pin the rules, and AC3 and AC7 close the two ways a failure could read as clean. Changing the output format would silently break callers that test for non-empty output; AC5 pins it. A binary missing or older than the shim would otherwise read as clean; AC7 covers it. The undo is reverting the PR, which restores the shell scanner and regenerated outputs.

Operator authority

None

Activity

  1. changed the title [-]Port the gate runner and pass cache from Python to Go[/-] [+]Port the rebase conflict-marker scan to Go[/+] on Oct 25, 2026
  2. updated Priority Standard and Work type Planned on Oct 25, 2026
  3. tbhb-agent commented on Oct 25, 2026

    @tbhb-agent
    ContributorAuthor

    Refinement: NOT READY — #714 has no issue body

    Checked

    Read #714, parent epic #588, native dependency links, and siblings #713–#720. #714’s body is empty, its native parent is #588, and its native blocked-by list is empty.

    Read the shared shell library, scanner, three callers, related guard, existing regression coverage, and Go command structure. No checks were executed.

    The title fits #588’s rebase migration. Sibling titles describe distinct migrations; #713–#719 also have empty bodies, so their detailed boundaries cannot be compared.

    Gaps

    Definition of Ready, in order:

    1. Problem: Missing. Explain why migrating this scanner matters and which existing behavior must survive.
    2. Acceptance criteria: Missing. Add at most 12 numbered criteria with matching AC<n>: prefixes, each stating one testable outcome without prescribing implementation. Cover marker recognition, exemptions, worktree versus staged content, output and statuses, and applicable compatibility and safety requirements. Existing behavior is documented in packages/agents/source/skills/rebase/shared/scripts/marker-scan.sh:12 and implemented at packages/agents/source/skills/rebase/shared/scripts/marker-scan.sh:40.
    3. Scope and non-goals: Missing. Bound the scanner migration and state a Non-goal or Out of scope line. Explicitly address the separate guard implementation at packages/agents/source/skills/resolve-rebase-conflicts/claude/scripts/guard-resolve.sh:116.
    4. Approach: Missing, so alignment with code cannot be approved. The scanner is standalone and does not source the shared library. Three scripts invoke its existing executable path: packages/agents/source/skills/rebase/shared/scripts/continue-rebase.sh:115, packages/agents/source/skills/rebase/shared/scripts/rebase-status.sh:141, and packages/agents/source/skills/rebase/shared/scripts/verify-rebase.sh:188. The existing Go command tree has no rebase scanner command (internal/agentrepo/command.go:85). Explain command availability before deployed callers require it, deletion of the replaced algorithm, and canonical generation.
    5. Dependencies: Missing. The native blocked-by list is empty; no specific missing blocker is established by the available sibling bodies. Document whether binary rollout and replacement enforcement protections require prerequisites, as Replace complex shell with tested Go, starting with gates and merging #588 requires. Do not add unrelated siblings as blockers.
    6. Size: Missing. Declare XS, S, M, or L and confirm one PR, accounting for Go implementation, behavior coverage, forwarding compatibility, and generated outputs.
    7. Verify plan: Missing. Map every proposed AC ID to evidence that proves it. Include staged/worktree divergence, marker boundaries, exemptions, output/status compatibility, and failed attribute reads. Existing coverage rejects exemption output produced by a failing Git command (tests/skills/source/lib/masked-status.bats:116).
    8. Risks: Missing. Identify missed markers, false exemptions, caller failure handling, and deployment incompatibility; state the undo. Callers suppress scanner failure statuses and inspect stdout, making the output/error contract consequential (packages/agents/source/skills/rebase/shared/scripts/continue-rebase.sh:115). The scanner also suppresses content-read failures (packages/agents/source/skills/rebase/shared/scripts/marker-scan.sh:71); explicitly settle preservation versus intentional change.
    9. Operator authority: Missing. Add None or an applicable line beginning with a reserved authority phrase. The epic supplies no operational write authority.

    Split

    No split is established as necessary. A scanner-only migration appears suitable for one small PR, subject to an explicit contract and rollout plan. Keep broader rebase orchestration and guard migration outside this issue unless their inclusion is justified. No direct overlap with the stated sibling scopes was found.

    Definition of done

    No item can be ticked: #714 contains neither AC nor a Verify plan.

    • D1 Unmet planning coverage; add AC-by-AC merged-head evidence requirements to PR Verification.
    • D2 Pending; exact merged-head independent approval and finding disposition are not specified.
    • D3 Pending; required checks and any exact-head owner approval are not specified.
    • D4 Pending; gitleaks, private-string leak scanning, and post leak review are not specified.
    • D5 Pending; signed commits and squash are not specified.
    • D6 Unmet planning coverage; documentation updates or justified “No doc impact” are absent.
    • D7 Pending; Assisted-by trailers and the provenance label are not specified.
    • D8 Unmet planning coverage; no size declaration or size-limit verification exists.
    • D9 Pending; follow-up issue requirements, Related references, and code issue links are not specified.
    • D10 Pending; closing reference, Done state, and final merge-commit status are not specified.
    • devtools-1 Unmet planning coverage; mise run check, mise run cover-go-check, and mise run cover-py-check are absent.
    • devtools-2 Unmet planning coverage; generation, token measurement, deployment, committed outputs, and generated-output verification are absent.
    • devtools-3 Unmet planning coverage; consumer migration documentation or justified “No consumer impact” is absent.

    Round: 1 of 3

  4. tbhb-agent commented on Oct 25, 2026

    @tbhb-agent
    ContributorAuthor

    Refinement: NOT READY — marker semantics contradict the code; error handling and rollout remain unsettled

    Checked

    Read #714, its round 1 verdict, #588, native blockers (empty), siblings #713–#720, and distribution issues #333 and #544. Read the scanner, all three callers, separate resolve guard, relevant Bats coverage, Go command registration, and generation instructions. No mechanical checks or operational writes were run.

    Gaps

    Definition of Ready, in order:

    1. Problem: Present, but rebase migration is Feature 13 of Replace complex shell with tested Go, starting with gates and merging #588, not Feature 11. The standalone scanner and three callers are correctly identified.

    2. Acceptance criteria: Eight correctly numbered criteria meet the count limit, but several require correction:

      • AC1 contradicts the scanner. It deliberately ignores =======, recognizes <<<<<<<, >>>>>>>, and |||||||, and requires a following ASCII space or end of line. It emits per-file counts, not individual matching lines (packages/agents/source/skills/rebase/shared/scripts/marker-scan.sh:40, packages/agents/source/skills/rebase/shared/scripts/marker-scan.sh:79). Specify the actual contract, including ancestor markers, delimiters, and built-in exempt paths.
      • AC2 overstates failed attribute lookup behavior. Failure makes the file non-exempt; a clean file still produces no finding and succeeds. Condition the expected finding on actual marker content (packages/agents/source/skills/rebase/shared/scripts/marker-scan.sh:61).
      • AC3 matches content selection, but staged mode still uses ordinary worktree attribute lookup. Preserve that distinction explicitly (packages/agents/source/skills/rebase/shared/scripts/marker-scan.sh:61, packages/agents/source/skills/rebase/shared/scripts/marker-scan.sh:70).
      • AC4 matches stdout behavior, subject to correcting AC1. Missing criteria cover exit statuses and usage: 0 without findings, 1 with findings, and 2 with stderr usage when no paths are supplied (packages/agents/source/skills/rebase/shared/scripts/marker-scan.sh:48, packages/agents/source/skills/rebase/shared/scripts/marker-scan.sh:84).
      • AC5 defers a required decision. Choose suppression or failure before implementation, then specify observable behavior for each caller. Recording a choice in PR prose does not establish runtime behavior.
      • AC6 combines several outcomes and implementation details. Keep caller compatibility as the AC; move the shim line limit and algorithm-removal prescription to Approach.
      • AC7 names a mechanical check instead of an observable outcome. State generated-entry-point consistency, then retain the command in Verify.
      • AC8 supplies non-functional coverage, but needs a defined corpus, scan mode, environment, and timing boundary. File count alone does not bound content size.
    3. Scope and non-goals: Explicit, but the Replace the commit and pull request workflow guards with Go hooks #719 cross-reference is false: Replace the commit and pull request workflow guards with Go hooks #719 expressly excludes guard-resolve.sh. Its separate implementation exists at packages/agents/source/skills/resolve-rebase-conflicts/claude/scripts/guard-resolve.sh:124. Selecting new caller failure handling under AC5 would also require adjusting the scanner-only scope.

    4. Approach: The Go subcommand and compatibility path are plausible, but the actual caller boundaries matter:

      • Continue supplies staged changed paths with --staged, masks scanner status, and refuses only on nonempty stdout (packages/agents/source/skills/rebase/shared/scripts/continue-rebase.sh:105, packages/agents/source/skills/rebase/shared/scripts/continue-rebase.sh:115).
      • Status scans the sorted union of unmerged/staged paths in worktree mode, masks status, and prints (none) for empty stdout (packages/agents/source/skills/rebase/shared/scripts/rebase-status.sh:131, packages/agents/source/skills/rebase/shared/scripts/rebase-status.sh:141).
      • Verify selects candidates from HEAD, then scans their worktree content. It suppresses candidate-read and scanner failures (packages/agents/source/skills/rebase/shared/scripts/verify-rebase.sh:183, packages/agents/source/skills/rebase/shared/scripts/verify-rebase.sh:188).

      Therefore, a new failure exit status alone cannot prevent a clean result.

      The cited masked-status test extracts the shell exempt function. Removing that function breaks the test harness itself; plan command-level replacement coverage while retaining the separate guard case (tests/skills/source/lib/masked-status.bats:116). The identified rebase coverage is one workflow suite, not three independent caller suites (tests/skills/rebase/scripts/rebase-workflow.bats:11).

    5. Dependencies: Native blockers are empty despite the stated distribution prerequisite. Ship the Go binaries as release assets installed through mise's github backend #333 publishes assets; feat(distribution): migrate consumer tool pins to release assets #544 explicitly owns consumer installation and explains that publishing assets alone does not install them. Record the actual rollout blocker or establish and verify an existing installation path. Resolve the contradictory promises to delete the algorithm while retaining existing scanners until distribution. Carry forward Replace complex shell with tested Go, starting with gates and merging #588’s replacement-enforcement protection requirement before rollout.

    6. Size: XS and one PR are stated, but reassess after accounting for behavior tests, extraction-test migration, caller integration, generated outputs, and rollout. S appears more credible; no mandatory implementation split is established.

    7. Verify plan: Every AC ID is cited, but coverage is incomplete:

      • AC1: Correct the oracle; explicitly cover ancestor markers, separators, delimiters, and built-in exemptions.
      • AC2: Test successful exemptions, other attribute values, failed lookup, and valid-looking partial output before failure through the complete scanner.
      • AC3: Divergent staged/worktree fixtures prove content selection; also pin attribute-source behavior.
      • AC4: Differential goldens are appropriate after correcting AC1. Include empty results, multiple/repeated paths, counts, argument order, statuses, and stderr usage.
      • AC5: Scanner tests and PR prose do not prove caller behavior. Exercise the chosen outcome through each caller, including failure with empty stdout.
      • AC6: Line counts and grep prove structural constraints only. Add explicit forwarding assertions for arguments, streams, and statuses against the branch binary and both generated harnesses.
      • AC7: Generated-output checking does not establish token metadata, deployment, or committed rewritten outputs. Include the documented sequence (tools/generate-skills.sh:30).
      • AC8: A benchmark alone does not assert the two-second limit. Specify an end-to-end workload including attribute and filesystem costs, then compare its measured result against the threshold.
    8. Risks: Missed markers, exemptions, stdout drift, deployment failure, and revert are addressed. However, calling AC1–AC3 “fail-closed tests” overlooks swallowed failures and suppressed reads (packages/agents/source/skills/rebase/shared/scripts/marker-scan.sh:71). Include binary/version mismatch and restoration of compatible deployed artifacts in the undo.

    9. Operator authority: None meets the required format. It grants no operational authority and does not waive ordinary owner protections.

    Split

    Keep one scanner migration PR after settling the contract and rollout prerequisite. No direct implementation overlap with siblings was found. Correct the claim that #719 covers the resolve guard. If implementation must land before binary availability, define the separately blocked caller switch explicitly.

    Definition of done

    These marks assess AC/Verify coverage, not completed implementation. No complete item is covered.

    • D1 Unmet: AC-linked tests are planned, but merged-head author evidence in PR Verification is unspecified.
    • D2 Pending: exact-head independent approval and finding disposition are unspecified.
    • D3 Pending: required checks and exact-head approval for loosened gates are unspecified.
    • D4 Pending: all three required leak-review obligations are absent.
    • D5 Pending: signed commits and squash are unspecified.
    • D6 Unmet: documentation updates or justified “No doc impact” are absent.
    • D7 Pending: Assisted-by trailers and provenance label are unspecified.
    • D8 Unmet: XS is declared, but size-limit verification or exact-head exception is absent.
    • D9 Pending: follow-up issue requirements, Related references, and code links are absent.
    • D10 Pending: closing reference, Done state, and merge-commit Status comment are absent.
    • devtools-1 Unmet: check and both coverage-floor commands are absent.
    • devtools-2 Partially covered: regeneration and generated-output checking are planned; ordered token measurement, deployment, and committed outputs are absent.
    • devtools-3 Unmet: consumer migration instructions or justified “No consumer impact” are absent.

    Round: 2 of 3

  5. tbhb-agent commented on Oct 25, 2026

    @tbhb-agent
    ContributorAuthor

    Refinement: NOT READY — AC4’s attribute-source guarantee lacks a proving check

    Checked

    Read #714’s current body, both prior verdicts, #588, siblings #713–#722, and distribution issues #333 and #544. Fetch was refused; code reads used the remote contents API at main. No files were edited, checks executed, or content posted.

    The Definition of Ready is satisfied except for the verification gap below:

    All three callers mask scanner status and decide from non-empty stdout: packages/agents/source/skills/rebase/shared/scripts/continue-rebase.sh:115, packages/agents/source/skills/rebase/shared/scripts/rebase-status.sh:141, and packages/agents/source/skills/rebase/shared/scripts/verify-rebase.sh:188. AC7’s stdout failure line is therefore necessary and sufficient when those callers invoke the scanner. Its proposed caller integration tests cover that boundary.

    Gaps

    1. AC4’s Verify check does not prove attribute-source compatibility. The current scanner calls ordinary git check-attr before selecting staged or worktree content; --staged changes only the content read (packages/agents/source/skills/rebase/shared/scripts/marker-scan.sh:61, :68, :70). The proposed AC4 fixture varies file content, missing index entries, and directories, but does not vary attribute definitions. An implementation using cached attributes only in staged mode could pass the listed checks while violating AC4.

      Add an AC4 fixture whose index and worktree .gitattributes disagree, and assert that both scan modes use ordinary worktree attribute lookup while retaining their respective content sources. Explicitly naming that attribute source in AC4 would also remove ambiguity from “the same way.”

    No other readiness-blocking gap found.

    Split

    No split needed. The scope fits one S PR and does not overlap the sibling migrations. Keep the separate guard marker check with #722.

    Definition of done

    These marks assess coverage already present in the AC and Verify plan, not completed implementation.

    • D1 Partial: every AC has planned evidence, but merged-head author evidence in PR Verification remains pending.
    • D2 Pending: independent exact-head approval and finding disposition.
    • D3 Pending: required checks and any exact-head owner approval.
    • D4 Pending: gitleaks, private-string leak scan, and leak review of posts.
    • D5 Pending: signed commits and squash.
    • D6 Unmet planning coverage: documentation updates or justified “No doc impact.”
    • D7 Pending: Assisted-by trailers and provenance label.
    • D8 Partial: S and one PR are stated; size-limit verification remains pending.
    • D9 Pending: follow-up issue content, Related references, and code links.
    • D10 Pending: closing reference, Done state, and merge-commit Status comment.
    • devtools-1 Unmet planning coverage: mise run check and both coverage-floor commands are absent.
    • devtools-2 AC9 and Verify specify ordered generation, token measurement, deployment, committed outputs, clean-diff verification, and generated-output checking.
    • devtools-3 Unmet planning coverage: consumer adoption documentation or justified “No consumer impact.”

    Round: 3 of 3

  6. tbhb-agent commented on Oct 25, 2026

    @tbhb-agent
    ContributorAuthor

    Status: redesigned after the three-round cap. Round 3 left one gap: AC4 did not name the attribute source and no check varied it. AC4 now states that the conflict-markers attribute comes from the working tree .gitattributes in both modes (the current behaviour), and its Verify check adds fixtures where the staged and working tree .gitattributes disagree, in each direction. No split was needed. Refinement restarts at Round 1 of 3.

  7. tbhb-agent commented on Oct 25, 2026

    @tbhb-agent
    ContributorAuthor

    Refinement: NOT READY — AC4 contradicts Git’s attribute fallback behavior

    Checked

    Reviewed the current #714 body, #588, native blockers, and the scope of #713–#722. Repository code was read from remote main through the contents API. No checks or tests were run.

    Definition of Ready, in order:

    • Problem: Clear motivation and compatibility boundary.
    • Acceptance criteria: Ten correctly numbered ACs cover recognition, exemptions, content sources, output, failures, integration, generation and performance. AC4 has the substantive contradiction below.
    • Scope and non-goals: Explicit and bounded.
    • Approach: Concrete, including batching, the forwarding shim and test migration.
    • Dependencies: Native blocked-by links are exactly Ship the Go binaries as release assets installed through mise's github backend #333 and feat(distribution): migrate consumer tool pins to release assets #544. Their release and consumer-installation responsibilities justify both links.
    • Size: S is credible for one PR.
    • Verify plan: Every AC has an identified check. The AC4 disagreement fixtures distinguish worktree preference from index-only lookup, but miss the fallback case below.
    • Risks: Identifies false negatives, exemptions, output compatibility and binary availability; reverting the PR supplies the undo.
    • Operator authority: None is explicit.

    Code comparison:

    • Marker recognition, built-in exemption, failed attribute lookup, staged/worktree reads, ordering and output match packages/agents/source/skills/rebase/shared/scripts/marker-scan.sh:40, :54, :61 and :65.
    • All three callers mask scanner status and decide from non-empty stdout: packages/agents/source/skills/rebase/shared/scripts/continue-rebase.sh:115, packages/agents/source/skills/rebase/shared/scripts/rebase-status.sh:141 and packages/agents/source/skills/rebase/shared/scripts/verify-rebase.sh:188. AC7’s stdout failure diagnostic is necessary and sufficient for those branches to surface a problem when invoked.
    • The extraction claim is correct: tests/skills/source/lib/masked-status.bats:116 extracts exempt from the scanner and documented from the resolve guard. Moving only the scanner half preserves Replace the rebase and conflict-resolution guards with Go hooks #722’s coverage.
    • The existing workflow coverage is one suite, as stated: tests/skills/rebase/scripts/rebase-workflow.bats:1.

    Gaps

    1. AC4’s “never from the index copies” conflicts with the existing scanner and the proposed approach.
      packages/agents/source/skills/rebase/shared/scripts/marker-scan.sh:61 invokes ordinary git check-attr, without --cached. That prefers working tree attributes, but Git falls back to the index when a .gitattributes file is absent from the working tree. This is documented Git attribute behavior.

      For example, stage a .gitattributes assigning conflict-markers=documented, then remove that attribute file from the working tree without staging its deletion. With a marker-bearing target present in both content sources, today’s scanner exempts it in both modes. AC4’s absolute prohibition says otherwise.

      The implementer must therefore choose between preserving the existing command’s behavior and satisfying AC4. Both proposed disagreement fixtures can pass while this contradiction remains.

      Required refinement: State that both modes use normal Git attribute resolution, preferring working tree .gitattributes with Git’s index fallback when absent. Add a fixture with the attribute file present in the index and absent from the working tree, checking exemption in both modes. If eliminating fallback is intentional, identify it as a behavior change and reconcile the preservation non-goal and Approach.

    Split

    No split needed. The scanner remains distinct from #722’s guard-marker check, #719’s commit/PR guards and #721’s merge guard. No sibling overlap or additional blocker was found.

    Definition of done

    The definition of done for #714 (Feature). Tick each item and give its evidence.

    • D1 The author verified each AC on the merged head, and the Verification section of the pull request cites each AC ID with its evidence: the command and its result, or a link. Covered by the ACs and Verify plan; merged-head evidence remains.
    • D2 An independent reviewer approved the exact merged head, and the author resolved each finding or filed it as a follow-up.
    • D3 The required checks pass on that head, and any loosened gate has the owner's approval of that exact head.
    • D4 The leak scans come back clean: gitleaks, devtools' check-leaks.sh with the repository's private-strings list, and the leak review of each post.
    • D5 The author signed each commit, the squash included.
    • D6 The docs match the change (README, docs/, CHANGELOG or help text), or the Verification section says "No doc impact" and why. Unmet in the plan: no documentation update or "No doc impact" statement.
    • D7 The commits and the squash have Assisted-by trailers, and the pull request has the provenance:agent-assisted label.
    • D8 The pull request stays within the size limits, or the owner approved its exact head over the hard limit.
    • D9 The author files follow-ups and deferred findings as issues with at least a Problem and numbered AC and lists them under the Related section of the pull request. Any work left in the code links its issue.
    • D10 The merge closes the issue through a closing reference and the board shows Done. The final Status: comment names the merge commit.
    • devtools-1 mise run check is green, and so are the coverage floors mise run cover-go-check and mise run cover-py-check, which check doesn't run. Unmet in the plan: the full check and both coverage floors are not specified.
    • devtools-2 A change to skill sources under packages/agents/source/ commits what mise run generate-skills, then mise run skill-tokens, then apm install rewrite, in that order, and check-generated-skills (part of lint) passes. Covered by AC9 and its Verify check.
    • devtools-3 A consumer-visible change (the vendored payload, a skill, a reusable workflow, a task or a pin) has its consumer steps in docs/agent-skills-migration.md or the adoption docs, or the PR's Verification says "No consumer impact" and why. Unmet in the plan: neither consumer steps nor "No consumer impact" is stated.

    Round: 1 of 3

  8. tbhb-agent commented on Oct 25, 2026

    @tbhb-agent
    ContributorAuthor

    Refinement: READY — AC4 now preserves ordinary Git attribute resolution

    Checked

    1. Problem: Identifies the scanner, its three callers, the migration purpose and the risk of masked failures appearing clean.

    2. Acceptance criteria: AC1–AC10 are numbered correctly, within the limit and testable. AC1–AC6 preserve the scanner’s observable contract: marker recognition, exemptions, failed attribute lookup, content selection, output ordering, statuses and unreadable-file handling. These match packages/agents/source/skills/rebase/shared/scripts/marker-scan.sh:40, packages/agents/source/skills/rebase/shared/scripts/marker-scan.sh:48 and packages/agents/source/skills/rebase/shared/scripts/marker-scan.sh:65.

      AC4’s correction resolves the previous gap. The scanner calls git check-attr without --cached, independently of content mode, at packages/agents/source/skills/rebase/shared/scripts/marker-scan.sh:61. Index content is selected separately at packages/agents/source/skills/rebase/shared/scripts/marker-scan.sh:70. The revised criterion correctly preserves working tree attribute precedence and index fallback when the attribute file is absent.

      Each attribute fixture rejects an incorrect implementation, in both modes:

      • (a) Rejects index-only lookup that would wrongly exempt the marked file.
      • (b) Rejects index-only lookup that would wrongly report the exempt file.
      • (c) Rejects lookup that ignores the index fallback and would wrongly report the exempt file.

      AC7 addresses the callers’ actual non-empty-output decisions: packages/agents/source/skills/rebase/shared/scripts/continue-rebase.sh:115, packages/agents/source/skills/rebase/shared/scripts/rebase-status.sh:141 and packages/agents/source/skills/rebase/shared/scripts/verify-rebase.sh:188. AC10 supplies a measurable performance requirement.

    3. Scope and non-goals: Consistent with AC4. Preserving existing attribute behavior does not expand scope. The separate guard remains excluded; its independent attribute lookup is visible at packages/agents/source/skills/resolve-rebase-conflicts/claude/scripts/guard-resolve.sh:124.

    4. Approach: Matches the corrected contract, including ordinary attribute resolution, rejecting partial output from failed attribute reads and retaining caller arguments. The extraction claim is accurate: tests/skills/source/lib/masked-status.bats:116 extracts exempt from the generated scanner and documented from the guard. Moving only the scanner half preserves the remaining guard test.

    5. Dependencies: Native blocked-by links are exactly Ship the Go binaries as release assets installed through mise's github backend #333 and feat(distribution): migrate consumer tool pins to release assets #544. Their current scopes cover publishing binary assets and installing them for consumers, respectively.

    6. Size: S is credible for one leaf algorithm, its compatibility shim, tests, harness adjustment and generated outputs.

    7. Verify plan: Every AC has an explicitly identified check. AC4 now exercises all three attribute arrangements in both content modes. The unchanged workflow suite is correctly identified as one suite; additional AC7 integrations exercise failure reporting through all three callers.

    8. Risks: Covers missed markers, false exemptions, output compatibility and unavailable binaries, with reverting the PR as the undo. Its wording is consistent with AC4 and the Approach.

    9. Operator authority and epic fit: None is appropriate. Existing protection patterns distinguish guard scripts from this scanner at .agents/agent-skills.conf:4 and .agents/agent-skills.conf:9. This is a bounded part of Replace complex shell with tested Go, starting with gates and merging #588’s rebase migration, without overlapping Port the gate runner and pass cache from Python to Go #713–Replace the rebase and conflict-resolution guards with Go hooks #722; particularly, Replace the commit and pull request workflow guards with Go hooks #719 owns commit/PR guards, Replace the merge workflow guard with a Go hook #721 the merge guard and Replace the rebase and conflict-resolution guards with Go hooks #722 the rebase/resolve guards.

    Gaps

    None that require an implementer to guess or leave an AC without a proving check.

    Split

    No further split needed.

    Definition of done

    The definition of done for #714 (Feature). Tick each item and give its evidence.

    • D1 The author verified each AC on the merged head, and the Verification section of the pull request cites each AC ID with its evidence: the command and its result, or a link.
    • D2 An independent reviewer approved the exact merged head, and the author resolved each finding or filed it as a follow-up.
    • D3 The required checks pass on that head, and any loosened gate has the owner's approval of that exact head.
    • D4 The leak scans come back clean: gitleaks, devtools' check-leaks.sh with the repository's private-strings list, and the leak review of each post.
    • D5 The author signed each commit, the squash included.
    • D6 The docs match the change (README, docs/, CHANGELOG or help text), or the Verification section says "No doc impact" and why.
    • D7 The commits and the squash have Assisted-by trailers, and the pull request has the provenance:agent-assisted label.
    • D8 The pull request stays within the size limits, or the owner approved its exact head over the hard limit.
    • D9 The author files follow-ups and deferred findings as issues with at least a Problem and numbered AC and lists them under the Related section of the pull request. Any work left in the code links its issue.
    • D10 The merge closes the issue through a closing reference and the board shows Done. The final Status: comment names the merge commit.
    • devtools-1 mise run check is green, and so are the coverage floors mise run cover-go-check and mise run cover-py-check, which check doesn't run.
    • devtools-2 A change to skill sources under packages/agents/source/ commits what mise run generate-skills, then mise run skill-tokens, then apm install rewrite, in that order, and check-generated-skills (part of lint) passes.
    • devtools-3 A consumer-visible change (the vendored payload, a skill, a reusable workflow, a task or a pin) has its consumer steps in docs/agent-skills-migration.md or the adoption docs, or the PR's Verification says "No consumer impact" and why.

    Round: 2 of 3

  9. added
    refinement:readyPassed the Definition of Ready; may be picked up
    and removed
    refinement:activeA refinement round is open; not ready for implementation
    on Oct 25, 2026
  10. moved this from Backlog to In progress in devtoolson Oct 25, 2026
  11. moved this from In progress to Done in devtoolson Oct 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    refinement:readyPassed the Definition of Ready; may be picked up

    Fields

    Priority

    Standard

    Work type

    Planned

    Projects

    • Status
      Done

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions