Repository navigation
Description
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
- 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. - AC2: Given a path ending in
skills/resolve-rebase-conflicts/SKILL.md, or a path whoseconflict-markersGit attribute isdocumented, when it is scanned, then it is not reported even if it contains marker lines. - 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.
- 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--stagedit 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 theconflict-markersattribute is resolved the way ordinary Git attribute lookup does, with the working tree.gitattributesfirst and the index copy used only when that file is absent from the working tree: given a.gitattributeswhose 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.gitattributesthat is in the index but absent from the working tree, then the index version decides. - 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. - 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).
- 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.
- 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.
- AC9: After the documented generation sequence (
mise run generate-skills, thenmise run skill-tokens, thenapm install), the committed generated outputs for the rebase skill match their sources andmise run check-generated-skillspasses. - 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
documentedattribute, 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
.gitattributesmarks the filedocumentedbut the working tree.gitattributesdoes not, so the file is reported; (b) the working tree.gitattributesmarks itdocumentedbut the staged one does not, so the file is not reported. (c) the.gitattributesmarks the filedocumentedin 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.batspassing against a shim backed by the branch binary. - AC9: the documented sequence run on the branch,
git diff --exit-codeafterwards, andmise 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
Metadata
Metadata
Assignees
Labels
Type
Fields
Priority
Work type
Projects
- StatusShow more project fieldsDone
Activity
[-]Port the gate runner and pass cache from Python to Go[/-][+]Port the rebase conflict-marker scan to Go[/+]tbhb-agent commentedon Oct 25, 2026
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:
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 inpackages/agents/source/skills/rebase/shared/scripts/marker-scan.sh:12and implemented atpackages/agents/source/skills/rebase/shared/scripts/marker-scan.sh:40.packages/agents/source/skills/resolve-rebase-conflicts/claude/scripts/guard-resolve.sh:116.packages/agents/source/skills/rebase/shared/scripts/continue-rebase.sh:115,packages/agents/source/skills/rebase/shared/scripts/rebase-status.sh:141, andpackages/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.tests/skills/source/lib/masked-status.bats:116).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.Noneor 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.
mise run check,mise run cover-go-check, andmise run cover-py-checkare absent.Round: 1 of 3
tbhb-agent commentedon Oct 25, 2026
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:
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.
Acceptance criteria: Eight correctly numbered criteria meet the count limit, but several require correction:
=======, 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.packages/agents/source/skills/rebase/shared/scripts/marker-scan.sh:61).packages/agents/source/skills/rebase/shared/scripts/marker-scan.sh:61,packages/agents/source/skills/rebase/shared/scripts/marker-scan.sh:70).packages/agents/source/skills/rebase/shared/scripts/marker-scan.sh:48,packages/agents/source/skills/rebase/shared/scripts/marker-scan.sh:84).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 atpackages/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.Approach: The Go subcommand and compatibility path are plausible, but the actual caller boundaries matter:
--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).(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).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
exemptfunction. 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).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.
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.
Verify plan: Every AC ID is cited, but coverage is incomplete:
tools/generate-skills.sh:30).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.Operator authority:
Nonemeets 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.
checkand both coverage-floor commands are absent.Round: 2 of 3
tbhb-agent commentedon Oct 25, 2026
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:
packages/agents/source/skills/rebase/shared/scripts/marker-scan.sh:40,:48,:54,:65).exempt; moving only its scanner half preserves the separate guard regression (tests/skills/source/lib/masked-status.bats:116).Nonesatisfies the required format.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, andpackages/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
AC4’s Verify check does not prove attribute-source compatibility. The current scanner calls ordinary
git check-attrbefore selecting staged or worktree content;--stagedchanges 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
.gitattributesdisagree, 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.
mise run checkand both coverage-floor commands are absent.Round: 3 of 3
tbhb-agent commentedon Oct 25, 2026
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-markersattribute comes from the working tree.gitattributesin both modes (the current behaviour), and its Verify check adds fixtures where the staged and working tree.gitattributesdisagree, in each direction. No split was needed. Refinement restarts at Round 1 of 3.tbhb-agent commentedon Oct 25, 2026
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
mainthrough the contents API. No checks or tests were run.Definition of Ready, in order:
Noneis explicit.Code comparison:
packages/agents/source/skills/rebase/shared/scripts/marker-scan.sh:40,:54,:61and:65.packages/agents/source/skills/rebase/shared/scripts/continue-rebase.sh:115,packages/agents/source/skills/rebase/shared/scripts/rebase-status.sh:141andpackages/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.tests/skills/source/lib/masked-status.bats:116extractsexemptfrom the scanner anddocumentedfrom the resolve guard. Moving only the scanner half preserves Replace the rebase and conflict-resolution guards with Go hooks #722’s coverage.tests/skills/rebase/scripts/rebase-workflow.bats:1.Gaps
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:61invokes ordinarygit check-attr, without--cached. That prefers working tree attributes, but Git falls back to the index when a.gitattributesfile is absent from the working tree. This is documented Git attribute behavior.For example, stage a
.gitattributesassigningconflict-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
.gitattributeswith 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.
check-leaks.shwith the repository's private-strings list, and the leak review of each post.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.provenance:agent-assistedlabel.Status:comment names the merge commit.mise run checkis green, and so are the coverage floorsmise run cover-go-checkandmise run cover-py-check, whichcheckdoesn't run. Unmet in the plan: the full check and both coverage floors are not specified.packages/agents/source/commits whatmise run generate-skills, thenmise run skill-tokens, thenapm installrewrite, in that order, andcheck-generated-skills(part oflint) passes. Covered by AC9 and its Verify check.docs/agent-skills-migration.mdor 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
tbhb-agent commentedon Oct 25, 2026
Refinement: READY — AC4 now preserves ordinary Git attribute resolution
Checked
Problem: Identifies the scanner, its three callers, the migration purpose and the risk of masked failures appearing clean.
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:48andpackages/agents/source/skills/rebase/shared/scripts/marker-scan.sh:65.AC4’s correction resolves the previous gap. The scanner calls
git check-attrwithout--cached, independently of content mode, atpackages/agents/source/skills/rebase/shared/scripts/marker-scan.sh:61. Index content is selected separately atpackages/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:
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:141andpackages/agents/source/skills/rebase/shared/scripts/verify-rebase.sh:188. AC10 supplies a measurable performance requirement.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.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:116extractsexemptfrom the generated scanner anddocumentedfrom the guard. Moving only the scanner half preserves the remaining guard test.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.
Size: S is credible for one leaf algorithm, its compatibility shim, tests, harness adjustment and generated outputs.
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.
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.
Operator authority and epic fit:
Noneis appropriate. Existing protection patterns distinguish guard scripts from this scanner at.agents/agent-skills.conf:4and.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.
check-leaks.shwith the repository's private-strings list, and the leak review of each post.docs/, CHANGELOG or help text), or the Verification section says "No doc impact" and why.provenance:agent-assistedlabel.Status:comment names the merge commit.mise run checkis green, and so are the coverage floorsmise run cover-go-checkandmise run cover-py-check, whichcheckdoesn't run.packages/agents/source/commits whatmise run generate-skills, thenmise run skill-tokens, thenapm installrewrite, in that order, andcheck-generated-skills(part oflint) passes.docs/agent-skills-migration.mdor the adoption docs, or the PR's Verification says "No consumer impact" and why.Round: 2 of 3
fix(rebase): keep scanning with older agentrepo binaries