fix(pipeline): distinguish intended local-main deliveries (#1003)

* fix(rebase): distinguish intended local-main work and no-op fixes

* no-mistakes(review): Report unreported fix attempts honestly in PR summaries

* no-mistakes(review): Classify PR fix headers by recorded results

* no-mistakes(review): Canonicalize no-op fixer results for PR summaries

* no-mistakes(review): Document local-default rebase ambiguity policy

* no-mistakes(review): Classify PR fixes from commit results

* no-mistakes(document): Document honest PR fix outcomes

* no-mistakes(lint): Preserve generated skill LF line endings

* no-mistakes(ci): Fixed PR fix-result rendering: descriptive recorded outcomes now render as applied fixes and retain their text; explicit no-op and missing-result labels remain distinct. Updated stale expectations for canonical fix outcomes and added a descriptive-result regression. Focused Docker race tests passed. Full steps race run still has baseline TestPRStep_GhNotAvailable plus Docker Unix reaping failures unrelated to this change

* no-mistakes(ci): Fixed PR-summary rendering: only canonical commit-backed `changes applied` is reported as applied; descriptive fix text is now an unverified attempt. Added regression coverage. Docker Go 1.25.14 targeted tests passed

* no-mistakes(ci): Fixed the CI regression by removing the obsolete home-path fixture for raw fix summaries, which are intentionally no longer rendered. Docker Go 1.25.14 race tests passed for the failing PR-content test and related fix-result/rebase regressions
This commit is contained in:
Cristian Rosescu
2026-09-08 05:17:40 +12:00
committed by GitHub
parent 4603016d04
commit 33327b7946
21 changed files with 399 additions and 113 deletions
+1
View File
@@ -0,0 +1 @@
skills/no-mistakes/SKILL.md text eol=lf
@@ -176,11 +176,11 @@ If the overwrite is intentional, push manually to the actual remote after review
### Rebase pauses because the branch carries unpushed default-branch commits
This means the branch was created from a local default branch that is ahead of `origin/<default_branch>`, so its history includes commits that exist only on your local default branch.
`no-mistakes` pauses with an `ask-user` finding instead of silently bundling that unrelated local work into the PR.
This means a local default branch ahead of `origin/<default_branch>` is a strict ancestor of your branch, so the branch may contain unrelated local-default work.
`no-mistakes` pauses with an `ask-user` finding instead of silently bundling that ambiguous work into the PR. If the local default tip and your branch `HEAD` are equal, it treats the commits as the intended delivery work and continues.
Push the default branch to `origin` if those commits belong in the shared base, or rebase your feature branch onto `origin/<default_branch>` to remove the unrelated work before running the gate again.
Approve the finding only when you intentionally want that local default-branch work to stay in the branch.
Push the default branch to `origin` if those commits belong in the shared base, or rebuild the feature branch from `origin/<default_branch>` to remove the unrelated work before running the gate again.
Approve the finding only when you have confirmed the local default-branch work belongs in the delivery branch.
## `git push no-mistakes` doesn't start a pipeline
@@ -65,8 +65,9 @@ The integration branch used below is the [PR base branch](/no-mistakes/reference
- If the branch is not the PR base branch, tries rebasing onto the pushed-branch target first, then `origin/<PR base branch>`
- If the push rewrote branch history, skips the pushed-branch rebase target so prior remote autofix commits do not get reintroduced
- If the push rewrote the PR base branch and `origin/<PR base branch>` advanced after that rewrite, pauses for manual approval before updating the branch
- If the branch carries commits from the contributor's local default branch that are not on `origin/<PR base branch>`, pauses with an `ask-user` finding instead of silently bundling that local work into the PR
- The local-default check is best-effort and only fires when the local default tip is ahead of `origin/<PR base branch>` and is an ancestor of the branch `HEAD`
- If the local default tip equals the branch `HEAD`, treats those local-only commits as the intended delivery work and continues
- If the local default tip is a strict ancestor of the branch `HEAD`, pauses with an `ask-user` finding instead of silently bundling potentially unrelated local work into the PR
- The local-default check is best-effort and only fires when the local default tip is ahead of `origin/<PR base branch>` and a strict ancestor of the branch `HEAD`
- Skips targets that don't exist or are already ancestors
- If a fast-forward is possible, does a hard-reset instead of a rebase
- If the diff against the PR base branch is empty after rebase, completes rebase and skips all remaining pipeline steps
@@ -246,7 +247,7 @@ Creates or updates a pull request.
- PR title: agent-generated from the final branch delta with user intent when available, in conventional commit format (`type(scope): description` or `type: description`); user-facing product impact should use `feat` or `fix` so release automation can pick it up; when a scope is used, it should be the primary affected real module/package from the changed paths and kept broad rather than file-level. If drafting fails, the fallback uses the neutral title `chore: update pull request` rather than inferring scope from earlier commits.
- Bounds the PR-drafting agent with [`agent_timeout`](/no-mistakes/reference/global-config/#agent_timeout): an expired budget cancels the agent and uses that same fallback rather than leaving the run active indefinitely; a late successful title after the deadline is not used
- The PR stage exclusively owns the complete branch-scope description. It drafts `## What Changed` from the actual final diff after local mutating stages finish, and its fallback lists the final changed paths and statuses.
- PR body includes a `## Intent` section when user intent is available, the final-diff `## What Changed`, and regenerated `## Risk Assessment`, `## Testing`, and `## Pipeline` sections from recorded step results and rounds. Only `## What Changed` describes the complete final branch scope; the deterministic sections remain evidence for the commit each step inspected. Auto-fix results in `## Pipeline` render as an issue -> fix -> verification narrative using captured fix summaries, re-check success text, and any still-open findings. Test details show the live-validation verdict, scenario table, and recorded commands in both `## Testing` and the relevant Pipeline rounds.
- PR body includes a `## Intent` section when user intent is available, the final-diff `## What Changed`, and regenerated `## Risk Assessment`, `## Testing`, and `## Pipeline` sections from recorded step results and rounds. Only `## What Changed` describes the complete final branch scope; the deterministic sections remain evidence for the commit each step inspected. Auto-fix results in `## Pipeline` render as an issue -> fix -> verification narrative using recorded outcomes: applied changes, a confirmed no-change attempt, or an attempt whose result was not recorded. Test details show the live-validation verdict, scenario table, and recorded commands in both `## Testing` and the relevant Pipeline rounds.
- `## Pipeline` keeps the existing human-readable signature and includes the stable structured step attestation documented below. Bitbucket Cloud PR descriptions omit HTML-only features (`<details>`, `<code>`, `<video>`, and the attestation comment) because Cloud renders Python-Markdown and escapes raw HTML.
- Generated PR bodies are capped at 63,488 bytes, leaving a 2 KB safety buffer below GitHub's 65,536-character body limit.
- When a body would exceed that cap, the PR step first omits older `## Pipeline` update rounds at clean update boundaries, keeps the newest rounds when possible, and points reviewers to the run log for the full pipeline history.
+1 -1
View File
@@ -649,7 +649,7 @@ func TestRenderDriveResult_ChecksPassedWithFixes(t *testing.T) {
"outcome: checks-passed",
"fixes[2]{step,summary}:",
"review,handle nil pointer in executor",
"test,fix applied (no summary recorded)",
"test,fix attempted (no result recorded)",
"Summarize this pipeline run for the user",
"acknowledge the misses and list each fix so the user can review them",
} {
+4 -5
View File
@@ -320,16 +320,15 @@ func (rv runView) findingsTally() string {
return joinComma(parts)
}
// fixRows flattens the fixes the pipeline applied across all steps into
// renderable rows, in step then round order. A fix round that recorded no
// summary still produced a fix commit, so it gets an explicit placeholder
// rather than being dropped.
// fixRows flattens fix-attempt summaries in step then round order. Dispatching
// a fix round does not prove a change was applied; legacy empty summaries
// must not manufacture that claim.
func (rv runView) fixRows() []fixRow {
var rows []fixRow
for _, s := range rv.Steps {
for _, summary := range s.FixSummaries {
if summary == "" {
summary = "fix applied (no summary recorded)"
summary = "fix attempted (no result recorded)"
}
rows = append(rows, fixRow{Step: s.Name, Summary: summary})
}
+2 -5
View File
@@ -52,9 +52,7 @@ type StepRound struct {
// deliberately left unselected.
SelectedFindingIDs *string
SelectionSource *string
// FixSummary, when non-nil, is the agent's one-line commit summary for
// the fix attempt performed during this round. It is only set when the
// round itself was a fix round (trigger=="auto_fix").
// FixSummary, when non-nil, records a fix round's result.
FixSummary *string
DurationMS int64
CreatedAt int64
@@ -83,8 +81,7 @@ func (r *StepRound) IsFixRound() bool {
return r.Trigger == "auto_fix" || r.Trigger == "user_fix"
}
// StepFixSummaries returns one entry per fix round for a step, in round order:
// the agent's one-line fix summary, or "" when the round recorded none.
// StepFixSummaries returns one result per fix round for a step, in round order.
func (d *DB) StepFixSummaries(stepResultID string) ([]string, error) {
rounds, err := d.GetRoundsByStep(stepResultID)
if err != nil {
+1 -4
View File
@@ -109,10 +109,7 @@ type StepOutcome struct {
// CI repairs use it when policy requires revalidation or continuity cannot be
// proven, sending the new local head back through review before push.
RestartFrom types.StepName
// FixSummary, when non-empty, is the agent's one-line commit summary for
// the fix attempt performed during this round. Steps populate it in fix
// mode so the executor can persist it on the round record and later
// rounds can reference what was previously attempted.
// FixSummary, when non-empty, records the result of a fix attempt.
FixSummary string
// ReviewApprovedHeadSHA is set only by a successfully executed full review
// round. The executor durably records it only when the review step actually
+31 -17
View File
@@ -47,6 +47,11 @@ type commitSummary struct {
var errRejectedCommitSummary = errors.New("rejected commit summary")
const (
noChangesAppliedSummary = "no changes applied"
changesAppliedSummary = "changes applied"
)
const fixerRemovalRule = `
Removal-first rule:
@@ -189,17 +194,22 @@ func commitPipelineCorrectionWithCleanup(
}
func commitAgentFixes(sctx *pipeline.StepContext, stepName types.StepName, summary, fallbackSummary string) error {
_, err := commitAgentFixesWithResult(sctx, stepName, summary, fallbackSummary)
return err
}
func commitAgentFixesWithResult(sctx *pipeline.StepContext, stepName types.StepName, summary, fallbackSummary string) (bool, error) {
ctx := sctx.Ctx
if err := assertPipelineHeadContinuity(sctx, stepName); err != nil {
return err
return false, err
}
status, err := git.Run(ctx, sctx.WorkDir, "status", "--porcelain")
if err != nil {
return fmt.Errorf("check %s changes: %w", stepName, err)
return false, fmt.Errorf("check %s changes: %w", stepName, err)
}
if strings.TrimSpace(status) == "" {
sctx.Log("no agent changes to commit")
return nil
return false, nil
}
if summary == "" {
summary = fallbackSummary
@@ -209,24 +219,24 @@ func commitAgentFixes(sctx *pipeline.StepContext, stepName types.StepName, summa
}
commitMessage, err := sctx.Config.Commit.RenderFixMessage(stepName, summary)
if err != nil {
return fmt.Errorf("render %s fix commit message: %w", stepName, err)
return false, fmt.Errorf("render %s fix commit message: %w", stepName, err)
}
if err := stagePipelineChanges(sctx); err != nil {
return fmt.Errorf("stage %s changes: %w", stepName, err)
return false, fmt.Errorf("stage %s changes: %w", stepName, err)
}
if err := commitPipelineCorrection(ctx, sctx.WorkDir, commitMessage, sctx.Log); err != nil {
return fmt.Errorf("commit %s changes: %w", stepName, err)
return false, fmt.Errorf("commit %s changes: %w", stepName, err)
}
headSHA, err := git.HeadSHA(ctx, sctx.WorkDir)
if err != nil {
return fmt.Errorf("resolve head after %s commit: %w", stepName, err)
return false, fmt.Errorf("resolve head after %s commit: %w", stepName, err)
}
if err := assertPipelineHeadContinuity(sctx, stepName); err != nil {
return err
return false, err
}
ref := normalizedBranchRef(sctx.Run.Branch)
if _, err := git.Run(ctx, sctx.WorkDir, "update-ref", ref, headSHA); err != nil {
return fmt.Errorf("update local branch ref: %w", err)
return false, fmt.Errorf("update local branch ref: %w", err)
}
startingHead := strings.TrimSpace(sctx.ReviewStartingHeadSHA)
if startingHead == "" {
@@ -234,13 +244,20 @@ func commitAgentFixes(sctx *pipeline.StepContext, stepName types.StepName, summa
}
sctx.Run.HeadSHA = headSHA
if err := sctx.DB.UpdateRunHeadSHA(sctx.Run.ID, headSHA); err != nil {
return err
return false, err
}
if stepName == types.StepReview {
pipeline.PersistUncertifiedPipelineRange(sctx, startingHead, headSHA)
}
sctx.Log(fmt.Sprintf("committed agent fixes: %s", commitMessage))
return nil
return true, nil
}
func fixResultSummary(committed bool) string {
if committed {
return changesAppliedSummary
}
return noChangesAppliedSummary
}
func extractCommitSummary(result *agent.Result) (string, error) {
@@ -262,10 +279,6 @@ func extractCommitSummary(result *agent.Result) (string, error) {
return cleaned, nil
}
// executeFixMode runs the fix agent and commits any resulting changes. It
// returns the agent's one-line fix summary (empty when the agent returned
// nothing parseable), which the caller should place on StepOutcome.FixSummary
// so the executor can persist it on the round record.
func executeFixMode(sctx *pipeline.StepContext, stepName types.StepName, opts fixExecutionOptions) (string, error) {
if !sctx.Fixing {
return "", nil
@@ -317,8 +330,9 @@ func executeFixMode(sctx *pipeline.StepContext, stepName types.StepName, opts fi
}
sctx.Log(fmt.Sprintf("warning: could not parse fix summary: %v", err))
}
if err := commitAgentFixes(sctx, stepName, summary, opts.FallbackSummary); err != nil {
committed, err := commitAgentFixesWithResult(sctx, stepName, summary, opts.FallbackSummary)
if err != nil {
return "", err
}
return summary, nil
return fixResultSummary(committed), nil
}
+86
View File
@@ -14,6 +14,7 @@ import (
"github.com/kunchenguid/no-mistakes/internal/agent"
"github.com/kunchenguid/no-mistakes/internal/config"
"github.com/kunchenguid/no-mistakes/internal/db"
"github.com/kunchenguid/no-mistakes/internal/forgecontext"
"github.com/kunchenguid/no-mistakes/internal/git"
"github.com/kunchenguid/no-mistakes/internal/pipeline"
@@ -970,6 +971,91 @@ func TestCommitAgentFixes_NoChanges(t *testing.T) {
}
}
func TestExecuteFixMode_NoWorktreeChangesCanonicalizesAgentSummary(t *testing.T) {
t.Parallel()
dir, baseSHA, headSHA := setupGitRepo(t)
gitCmd(t, dir, "checkout", "--detach", headSHA)
ag := &mockAgent{
name: "test",
runFn: func(context.Context, agent.RunOpts) (*agent.Result, error) {
return &agent.Result{Output: json.RawMessage(`{"summary":"no changes were necessary"}`)}, nil
},
}
sctx := newTestContextWithDBRecords(t, ag, dir, baseSHA, headSHA, config.Commands{})
sctx.Fixing = true
summary, err := executeFixMode(sctx, types.StepReview, fixExecutionOptions{FallbackSummary: "apply review fix"})
if err != nil {
t.Fatal(err)
}
if summary != noChangesAppliedSummary {
t.Fatalf("fix summary = %q, want %q", summary, noChangesAppliedSummary)
}
if got := gitCmd(t, dir, "rev-parse", "HEAD"); got != headSHA {
t.Fatalf("HEAD after no-op fix = %q, want %q", got, headSHA)
}
findings := `{"findings":[{"id":"review-1","severity":"warning","description":"accepted warning"}],"summary":"1 warning"}`
md, _ := BuildPipelineSummary(
[]*db.StepResult{{ID: "s1", StepName: types.StepReview, Status: types.StepStatusCompleted}},
map[string][]*db.StepRound{"s1": {
{Round: 1, Trigger: "initial", FindingsJSON: &findings},
{Round: 2, Trigger: "auto_fix", FixSummary: &summary},
}},
testPipelineHeadSHA,
)
if !strings.Contains(md, "🔧 **Review** - 1 issue found → no changes applied ✅") {
t.Fatalf("expected canonical no-change result in PR summary, got:\n%s", md)
}
if strings.Contains(md, "auto-fixed") {
t.Fatalf("did not expect no-op fix to be called auto-fixed, got:\n%s", md)
}
}
func TestExecuteFixMode_WorktreeChangesCanonicalizesMisleadingAgentSummary(t *testing.T) {
t.Parallel()
dir, baseSHA, headSHA := setupGitRepo(t)
gitCmd(t, dir, "checkout", "--detach", headSHA)
ag := &mockAgent{
name: "test",
runFn: func(_ context.Context, opts agent.RunOpts) (*agent.Result, error) {
if err := os.WriteFile(filepath.Join(opts.CWD, "fix.go"), []byte("package fix\n"), 0o644); err != nil {
return nil, err
}
return &agent.Result{Output: json.RawMessage(`{"summary":"no changes applied: checked formatting"}`)}, nil
},
}
sctx := newTestContextWithDBRecords(t, ag, dir, baseSHA, headSHA, config.Commands{})
sctx.Fixing = true
summary, err := executeFixMode(sctx, types.StepReview, fixExecutionOptions{FallbackSummary: "apply review fix"})
if err != nil {
t.Fatal(err)
}
if summary != changesAppliedSummary {
t.Fatalf("fix summary = %q, want %q", summary, changesAppliedSummary)
}
if got := gitCmd(t, dir, "rev-parse", "HEAD"); got == headSHA {
t.Fatal("expected committed fix to advance HEAD")
}
findings := `{"findings":[{"id":"review-1","severity":"warning","description":"fixable warning"}],"summary":"1 warning"}`
md, _ := BuildPipelineSummary(
[]*db.StepResult{{ID: "s1", StepName: types.StepReview, Status: types.StepStatusCompleted}},
map[string][]*db.StepRound{"s1": {
{Round: 1, Trigger: "initial", FindingsJSON: &findings},
{Round: 2, Trigger: "auto_fix", FixSummary: &summary},
}},
testPipelineHeadSHA,
)
if !strings.Contains(md, "🔧 **Review** - 1 issue found → auto-fixed ✅") {
t.Fatalf("expected committed fix in PR summary, got:\n%s", md)
}
if strings.Contains(md, "no changes applied") {
t.Fatalf("did not expect misleading agent prose in PR summary, got:\n%s", md)
}
}
func TestCommitAgentFixes_InvalidTemplateDoesNotStageChanges(t *testing.T) {
t.Parallel()
dir, baseSHA, headSHA := setupGitRepo(t)
+3 -2
View File
@@ -149,7 +149,8 @@ func (s *DocumentStep) Execute(sctx *pipeline.StepContext) (*pipeline.StepOutcom
if combinedLint {
fallbackSummary = "update documentation and fix lint"
}
if err := commitAgentFixes(sctx, s.Name(), commitSummary, fallbackSummary); err != nil {
committed, err := commitAgentFixesWithResult(sctx, s.Name(), commitSummary, fallbackSummary)
if err != nil {
return nil, err
}
@@ -187,7 +188,7 @@ func (s *DocumentStep) Execute(sctx *pipeline.StepContext) (*pipeline.StepOutcom
NeedsApproval: needsApproval,
AutoFixable: false,
Findings: string(findingsJSON),
FixSummary: docFindings.Summary,
FixSummary: fixResultSummary(committed),
}, nil
}
+6
View File
@@ -46,6 +46,9 @@ func TestDocumentStep_AgentManaged_FixesAndCommitsWithoutApproval(t *testing.T)
if outcome.AutoFixable {
t.Error("expected no auto-fix loop in agent-managed document mode")
}
if outcome.FixSummary != changesAppliedSummary {
t.Fatalf("fix summary = %q, want %q", outcome.FixSummary, changesAppliedSummary)
}
if status := gitStatusPorcelain(t, dir); status != "" {
t.Fatalf("expected clean worktree after doc commit, got %q", status)
}
@@ -134,6 +137,9 @@ func TestDocumentStep_AgentManaged_UnresolvedFindingsNeedApprovalWithoutAutoFixL
if outcome.AutoFixable {
t.Error("expected unresolved documentation findings not to trigger an auto-fix round")
}
if outcome.FixSummary != noChangesAppliedSummary {
t.Fatalf("fix summary = %q, want %q", outcome.FixSummary, noChangesAppliedSummary)
}
var findings Findings
if err := json.Unmarshal([]byte(outcome.Findings), &findings); err != nil {
t.Fatalf("unmarshal findings: %v", err)
@@ -252,6 +252,9 @@ func TestLintStep_RunsOwnPassWithoutCombinedResult(t *testing.T) {
if outcome.NeedsApproval {
t.Fatal("clean lint pass must not park")
}
if outcome.FixSummary != noChangesAppliedSummary {
t.Fatalf("fix summary = %q, want %q", outcome.FixSummary, noChangesAppliedSummary)
}
}
func TestDocumentStep_CombinedRetryDropsPriorLintResultWhenOutputIsUntrusted(t *testing.T) {
+3 -2
View File
@@ -94,7 +94,8 @@ Previous lint findings to address:
}
sctx.Log(fmt.Sprintf("warning: could not parse lint summary: %v", err))
}
if err := commitAgentFixes(sctx, s.Name(), summary, "fix lint issues"); err != nil {
committed, err := commitAgentFixesWithResult(sctx, s.Name(), summary, "fix lint issues")
if err != nil {
return nil, err
}
@@ -104,7 +105,7 @@ Previous lint findings to address:
NeedsApproval: needsApproval,
AutoFixable: false,
Findings: string(findingsJSON),
FixSummary: summary,
FixSummary: fixResultSummary(committed),
}, nil
}
+2 -18
View File
@@ -88,7 +88,6 @@ type homePathLeakCase struct {
reviewFindings string
testFindings string
testStepError string
fixSummary string
userIntent string
agentTitle string
agentBody string
@@ -248,15 +247,6 @@ func TestPRStep_BuildPRContentRedactsAbsoluteHomePaths(t *testing.T) {
}),
wantVisible: []string{"the generated config still points at ~/.config/svc.toml"},
},
{
name: "auto-fix round summary",
evidenceDir: fixtureEvidenceDir,
reviewFindings: findingsJSON(t, types.Findings{
Items: []types.Finding{{Severity: types.FindingSeverityWarning, Description: "hard-coded path"}},
}),
fixSummary: "replaced the hard-coded " + fixtureHome + "/data path with a config key",
wantVisible: []string{"replaced the hard-coded ~/data path with a config key"},
},
{
name: "failed step error text",
evidenceDir: fixtureEvidenceDir,
@@ -416,18 +406,12 @@ func buildHomePathLeakPRContentWithLimit(t *testing.T, tc homePathLeakCase, body
testFindings = strings.ReplaceAll(testFindings, "%EVIDENCEFILE%", strings.ReplaceAll(evidenceFile, `\`, `\\`))
}
if tc.reviewFindings != "" || tc.fixSummary != "" {
if tc.reviewFindings != "" {
reviewFindings := tc.reviewFindings
if reviewFindings == "" {
reviewFindings = findingsJSON(t, types.Findings{})
}
step := insertCompletedStep(t, sctx, types.StepReview, reviewFindings, "")
if tc.fixSummary != "" {
fix := tc.fixSummary
if _, err := sctx.DB.InsertStepRound(step.ID, 2, "auto_fix", nil, &fix, 200); err != nil {
t.Fatal(err)
}
}
insertCompletedStep(t, sctx, types.StepReview, reviewFindings, "")
}
if testFindings != "" || tc.testStepError != "" {
insertCompletedStep(t, sctx, types.StepTest, testFindings, tc.testStepError)
+1 -1
View File
@@ -873,7 +873,7 @@ func TestPRStep_AppendsTestingSectionFromTestStep(t *testing.T) {
}
ghLog := string(logData)
wantOrder := "## Risk Assessment\n\n⚠ Medium: touches critical error handling\n\n## Testing\n\n- 🔧 **Test** - 1 issue found → auto-fixed ✅\n\n## Pipeline"
wantOrder := "## Risk Assessment\n\n⚠ Medium: touches critical error handling\n\n## Testing\n\n- 🔧 **Test** - 1 issue found → fix attempted; result not reported ✅\n\n## Pipeline"
if !strings.Contains(ghLog, wantOrder) {
t.Fatalf("expected testing section between risk assessment and pipeline, got:\n%s", ghLog)
}
+57 -21
View File
@@ -1166,7 +1166,7 @@ func buildStepEntry(sr *db.StepResult, rounds []*db.StepRound, flavor prBodyFlav
hasRoundParseFailure := roundsHaveParseFailure(rounds)
hadAnyFindings := hadFindings || hasFinalFindings || hasAnyRoundFindings
hasUnreadableFinalFindings := sr.FindingsJSON != nil && !finalFindingsParsed
wasFixed := hadFindings && len(rounds) > 1 && !hasUnreadableFinalFindings && !hasFinalFindings
findingsCleared := hadFindings && len(rounds) > 1 && !hasUnreadableFinalFindings && !hasFinalFindings
riskLevel := ""
if sr.StepName == types.StepReview {
src := finalFindings
@@ -1198,7 +1198,7 @@ func buildStepEntry(sr *db.StepResult, rounds []*db.StepRound, flavor prBodyFlav
return buildDetail(fmt.Sprintf("⚠️ **%s** - findings unavailable", name))
}
if wasFixed {
if findingsCleared {
result := buildFixResultText(rounds)
line := fmt.Sprintf("🔧 **%s** - %s ✅", name, result)
return buildDetail(line)
@@ -1317,11 +1317,18 @@ func buildFixResultText(rounds []*db.StepRound) string {
}
}
// Categorize fix rounds. Legacy "user_fix" rounds are rendered as auto-fix.
autoFixRounds := 0
var autoFixRounds, noChangeRounds, unreportedRounds int
for _, r := range rounds[1:] {
if r.IsFixRound() {
if !r.IsFixRound() {
continue
}
switch fixRoundOutcome(r) {
case fixOutcomeNoChange:
noChangeRounds++
case fixOutcomeApplied:
autoFixRounds++
case fixOutcomeUnreported:
unreportedRounds++
}
}
@@ -1332,10 +1339,19 @@ func buildFixResultText(rounds []*db.StepRound) string {
parts := []string{fmt.Sprintf("%d %s found", initialCount, noun)}
if autoFixRounds > 1 {
parts = append(parts, fmt.Sprintf("auto-fixed (%d)", autoFixRounds))
} else if autoFixRounds == 1 {
parts = append(parts, "auto-fixed")
for _, result := range []struct {
count int
text string
}{
{autoFixRounds, "auto-fixed"},
{noChangeRounds, "no changes applied"},
{unreportedRounds, "fix attempted; result not reported"},
} {
if result.count == 1 {
parts = append(parts, result.text)
} else if result.count > 1 {
parts = append(parts, fmt.Sprintf("%s (%d)", result.text, result.count))
}
}
return strings.Join(parts, " → ")
@@ -1344,9 +1360,8 @@ func buildFixResultText(rounds []*db.StepRound) string {
// buildStepDetails renders the collapsible body for a step as an
// issue -> fix -> outcome narrative rather than a round-by-round log. Each
// round is shown as the review state observed at its end; a fix round is
// prefixed with the fix the agent applied (its commit summary) so a reader can
// see what was wrong and what was done about it without mentally replaying
// "rounds".
// prefixed with its recorded outcome so a reader can follow the result without
// mentally replaying rounds.
func buildStepDetails(summaryLine string, sr *db.StepResult, rounds []*db.StepRound, flavor prBodyFlavor) string {
var inner strings.Builder
if len(rounds) == 0 {
@@ -1361,7 +1376,7 @@ func buildStepDetails(summaryLine string, sr *db.StepResult, rounds []*db.StepRo
for _, r := range rounds {
isFixRound := r.IsFixRound()
if isFixRound {
inner.WriteString(fixRoundLine(r, flavor))
inner.WriteString(fixRoundLine(r))
inner.WriteString("\n")
}
@@ -1439,17 +1454,38 @@ func isTautologicalStepInner(inner string) bool {
}
}
// fixRoundLine renders the one-line summary of the fix the agent applied in a
// fix round, falling back to a generic note when no summary was captured.
func fixRoundLine(r *db.StepRound, flavor prBodyFlavor) string {
summary := ""
if r.FixSummary != nil {
summary = strings.TrimSpace(*r.FixSummary)
type fixOutcome uint8
const (
fixOutcomeUnreported fixOutcome = iota
fixOutcomeNoChange
fixOutcomeApplied
)
func fixRoundOutcome(r *db.StepRound) fixOutcome {
if r.FixSummary == nil || strings.TrimSpace(*r.FixSummary) == "" {
return fixOutcomeUnreported
}
if summary == "" {
switch strings.TrimSpace(*r.FixSummary) {
case noChangesAppliedSummary:
return fixOutcomeNoChange
case changesAppliedSummary:
return fixOutcomeApplied
default:
return fixOutcomeUnreported
}
}
// fixRoundLine renders the one-line result of a fix round.
func fixRoundLine(r *db.StepRound) string {
switch fixRoundOutcome(r) {
case fixOutcomeNoChange:
return "🔧 No changes applied."
case fixOutcomeApplied:
return "🔧 Fix applied."
default:
return "🔧 Fix attempted; result not reported."
}
return fmt.Sprintf("🔧 Fix: %s", escapePRText(summary, flavor))
}
// writeFindingItems renders each finding as a `file:line - description` bullet,
@@ -72,8 +72,11 @@ func TestBuildPipelineSummary_ReviewUsesFinalCleanState(t *testing.T) {
if !strings.Contains(md, "🔧 **Review**") {
t.Errorf("expected fixed review status, got:\n%s", md)
}
if !strings.Contains(md, "auto-fixed") {
t.Errorf("expected auto-fixed in review line, got:\n%s", md)
if !strings.Contains(md, "🔧 **Review** - 1 issue found → fix attempted; result not reported ✅") {
t.Errorf("expected unreported fix result in review line, got:\n%s", md)
}
if strings.Contains(md, "auto-fixed") {
t.Errorf("did not expect unreported fix result to be called auto-fixed, got:\n%s", md)
}
if strings.Contains(md, "user-fixed") {
t.Errorf("did not expect user-fixed in review line, got:\n%s", md)
@@ -90,7 +93,7 @@ func TestBuildPipelineSummary_ReviewUsesFinalCleanState(t *testing.T) {
if !strings.Contains(risk, "✅") {
t.Errorf("expected checkmark for low risk, got: %q", risk)
}
if !strings.Contains(md, "🔧 Fix applied.") || !strings.Contains(md, "✅ Re-checked - no issues remain.") {
if !strings.Contains(md, "🔧 Fix attempted; result not reported.") || !strings.Contains(md, "✅ Re-checked - no issues remain.") {
t.Errorf("expected the fix and verification to remain visible for multi-round review, got:\n%s", md)
}
}
@@ -23,21 +23,26 @@ func TestBuildPipelineSummary_AutoFix(t *testing.T) {
}
md, _ := BuildPipelineSummary(steps, rounds, testPipelineHeadSHA)
// Should show wrench emoji for auto-fixed
// Should show wrench emoji for a fix attempt.
if !strings.Contains(md, "🔧") {
t.Errorf("expected wrench emoji for auto-fixed step, got:\n%s", md)
t.Errorf("expected wrench emoji for fix attempt, got:\n%s", md)
}
// Status line should mention auto-fixed
if !strings.Contains(md, "auto-fixed") {
t.Errorf("expected 'auto-fixed' in status line, got:\n%s", md)
if !strings.Contains(md, "🔧 **Lint** - 2 issues found → fix attempted; result not reported ✅") {
t.Errorf("expected unreported fix result in status line, got:\n%s", md)
}
if strings.Contains(md, "auto-fixed") {
t.Errorf("did not expect an unreported fix result to be called auto-fixed, got:\n%s", md)
}
// Details should show the issue, then the fix, then the verification -
// not a round-by-round log.
if !strings.Contains(md, "unused import") {
t.Errorf("expected finding description in details, got:\n%s", md)
}
if !strings.Contains(md, "🔧 Fix applied.") {
t.Errorf("expected a fix line in details, got:\n%s", md)
if !strings.Contains(md, "🔧 Fix attempted; result not reported.") {
t.Errorf("expected an honest no-result fix line in details, got:\n%s", md)
}
if strings.Contains(md, "🔧 Fix applied.") {
t.Errorf("did not expect an unsupported applied-fix claim, got:\n%s", md)
}
if !strings.Contains(md, "✅ Re-checked - no issues remain.") {
t.Errorf("expected a verification line in details, got:\n%s", md)
@@ -66,7 +71,7 @@ func TestBuildPipelineSummary_BitbucketCloudKeepsFixNarrativeWithoutHTML(t *test
for _, want := range []string{
"### ⚠️ **Lint** - 1 warning",
"unused import",
"🔧 Fix applied.",
"🔧 Fix attempted; result not reported.",
"1 warning still open:",
"missing error check",
} {
@@ -81,10 +86,10 @@ func TestBuildPipelineSummary_BitbucketCloudKeepsFixNarrativeWithoutHTML(t *test
}
}
func TestBuildPipelineSummary_AutoFixShowsFixSummary(t *testing.T) {
func TestBuildPipelineSummary_DescriptiveFixSummaryDoesNotClaimAppliedFix(t *testing.T) {
t.Parallel()
findings1 := `{"findings":[{"id":"doc-1","severity":"warning","file":"internal/agent/server.go","line":129,"description":"waitForHealth doc comment still references the old 30s deadline"}],"summary":"1 warning"}`
fixSummary := "reference the configured 60s health-check deadline in the waitForHealth comment"
fixSummary := "updated the waitForHealth doc comment"
steps := []*db.StepResult{
{ID: "s1", StepName: types.StepDocument, Status: types.StepStatusCompleted},
}
@@ -96,10 +101,17 @@ func TestBuildPipelineSummary_AutoFixShowsFixSummary(t *testing.T) {
}
md, _ := BuildPipelineSummary(steps, rounds, testPipelineHeadSHA)
// The fix the agent actually applied must be surfaced - this is the data
// the old round-by-round layout dropped on the floor.
if !strings.Contains(md, "🔧 Fix: "+fixSummary) {
t.Errorf("expected the fix summary to be surfaced, got:\n%s", md)
// A descriptive result is not evidence that a change was committed. Only
// the canonical result written after commitAgentFixes succeeds can support
// an applied-fix claim.
if !strings.Contains(md, "🔧 Fix attempted; result not reported.") {
t.Errorf("expected unreported fix result, got:\n%s", md)
}
if !strings.Contains(md, "🔧 **Document** - 1 issue found → fix attempted; result not reported ✅") {
t.Errorf("expected unreported fix in status line, got:\n%s", md)
}
if strings.Contains(md, "Fix applied") || strings.Contains(md, "auto-fixed") {
t.Errorf("did not expect descriptive fix summary to claim an applied fix, got:\n%s", md)
}
// The original problem must still be shown next to its fix.
if !strings.Contains(md, "waitForHealth doc comment still references the old 30s deadline") {
@@ -115,6 +127,34 @@ func TestBuildPipelineSummary_AutoFixShowsFixSummary(t *testing.T) {
}
}
func TestBuildPipelineSummary_AutoFixRetainsExplicitNoChangeSummary(t *testing.T) {
t.Parallel()
findings := `{"findings":[{"id":"rebase-1","severity":"warning","file":"cmd/no-mistakes/main.go","line":1,"description":"local default branch requires manual reconciliation"}],"summary":"1 warning"}`
noChanges := "no changes applied"
steps := []*db.StepResult{{ID: "s1", StepName: types.StepRebase, Status: types.StepStatusCompleted}}
rounds := map[string][]*db.StepRound{
"s1": {
{Round: 1, Trigger: "initial", FindingsJSON: &findings, DurationMS: 800},
{Round: 2, Trigger: "auto_fix", FixSummary: &noChanges, DurationMS: 600},
},
}
md, _ := BuildPipelineSummary(steps, rounds, testPipelineHeadSHA)
if !strings.Contains(md, "🔧 No changes applied.") {
t.Errorf("expected explicit no-change result, got:\n%s", md)
}
if !strings.Contains(md, "🔧 **Rebase** - 1 issue found → no changes applied ✅") {
t.Errorf("expected explicit no-change result in status line, got:\n%s", md)
}
if strings.Contains(md, "auto-fixed") {
t.Errorf("did not expect an explicit no-change result to be called auto-fixed, got:\n%s", md)
}
if strings.Contains(md, "🔧 Fix attempted; result not reported.") {
t.Errorf("did not expect an explicit no-change result to be treated as unreported, got:\n%s", md)
}
}
func TestBuildPipelineSummary_MultiRoundWithFollowUpFix(t *testing.T) {
t.Parallel()
findings1 := `{"findings":[{"id":"test-1","severity":"error","file":"pkg/handler_test.go","line":42,"description":"expected 429 got 200"},{"id":"test-2","severity":"error","file":"pkg/handler_test.go","line":78,"description":"context deadline exceeded"}],"summary":"2 failures"}`
@@ -145,8 +185,8 @@ func TestBuildPipelineSummary_MultiRoundWithFollowUpFix(t *testing.T) {
if strings.Contains(md, "user-fix") || strings.Contains(md, "user-fixed") {
t.Errorf("did not expect user-fix wording, got:\n%s", md)
}
if !strings.Contains(md, "auto-fixed (2)") {
t.Errorf("expected consolidated auto-fix count, got:\n%s", md)
if !strings.Contains(md, "🔧 **Test** - 2 issues found → fix attempted; result not reported (2)") {
t.Errorf("expected consolidated unreported-fix count, got:\n%s", md)
}
}
@@ -164,12 +204,15 @@ func TestBuildPipelineSummary_LegacyUserFixRoundsRenderAsAutoFix(t *testing.T) {
}
md, _ := BuildPipelineSummary(steps, rounds, testPipelineHeadSHA)
if !strings.Contains(md, "auto-fixed") {
t.Errorf("expected legacy user_fix round to render as auto-fixed, got:\n%s", md)
if !strings.Contains(md, "🔧 **Review** - 1 issue found → fix attempted; result not reported ✅") {
t.Errorf("expected legacy user_fix round to preserve its unreported result, got:\n%s", md)
}
if strings.Contains(md, "auto-fixed") {
t.Errorf("did not expect legacy user_fix round without a result to be called auto-fixed, got:\n%s", md)
}
// A legacy user_fix round must render as a normal fix, not surface the
// "user" trigger wording anywhere.
if !strings.Contains(md, "🔧 Fix applied.") || !strings.Contains(md, "✅ Re-checked - no issues remain.") {
if !strings.Contains(md, "🔧 Fix attempted; result not reported.") || !strings.Contains(md, "✅ Re-checked - no issues remain.") {
t.Errorf("expected legacy user_fix round to render as an auto-fix, got:\n%s", md)
}
if strings.Contains(md, "user-fix") || strings.Contains(md, "user-fixed") {
+49 -7
View File
@@ -72,8 +72,8 @@ func (s *RebaseStep) Execute(sctx *pipeline.StepContext) (*pipeline.StepOutcome,
// Stop before rebasing when the gated branch carries commits that live on
// the contributor's local default branch but were never pushed to
// origin/<default>. Rebasing onto the fresh remote default keeps those
// commits in the branch's history, so the PR would silently bundle another
// workstream's unpushed work. Surface it for a human decision instead.
// commits in the branch's history, so the PR may bundle another
// workstream's unpushed work. Surface the ambiguity for a human decision.
if outcome := detectBundledLocalDefaultCommits(ctx, sctx, branch, defaultBranch); outcome != nil {
return outcome, nil
}
@@ -99,12 +99,26 @@ func (s *RebaseStep) Execute(sctx *pipeline.StepContext) (*pipeline.StepOutcome,
}
if sctx.Fixing {
before, err := git.HeadSHA(ctx, sctx.WorkDir)
if err != nil {
return nil, err
}
for _, target := range targets {
if err := rebaseWithAgent(ctx, sctx, target); err != nil {
return nil, err
}
}
return updateHeadSHA(ctx, sctx)
outcome, err := updateHeadSHA(ctx, sctx)
if err == nil {
if sctx.Run.HeadSHA == before {
outcome.FixSummary = noChangesAppliedSummary
sctx.Log("no changes applied: branch already up to date")
} else {
outcome.FixSummary = changesAppliedSummary
sctx.Log("rebased branch onto upstream")
}
}
return outcome, err
}
// Normal mode: try all rebases, track which targets had conflicts
@@ -192,7 +206,9 @@ func effectivePRBaseBranch(sctx *pipeline.StepContext) string {
//
// It only flags commits the branch actually carries: it reads the local default
// tip from the working repo, confirms that tip is ahead of origin/<default> and
// is an ancestor of the branch HEAD, then enumerates the unpushed commits.
// is a strict ancestor of the branch HEAD, then enumerates the unpushed commits.
// Equal tips are the common commit-on-main-then-name-a-branch workflow, not
// evidence of an additional bundled workstream.
// Detection is best-effort - if the local default tip advanced past the branch
// point, or the working repo cannot be read, it returns nil rather than guess.
func detectBundledLocalDefaultCommits(ctx context.Context, sctx *pipeline.StepContext, branch, defaultBranch string) *pipeline.StepOutcome {
@@ -229,21 +245,46 @@ func detectBundledLocalDefaultCommits(ctx context.Context, sctx *pipeline.StepCo
return nil
}
// A delivery branch created at local main's tip carries only that intended
// work, not an additional workstream beneath its own commits (#998).
head, err := git.HeadSHA(ctx, sctx.WorkDir)
if err == nil && head == localTip {
return nil
}
subjects, err := git.Run(ctx, sctx.WorkDir, "log", "--oneline", "--no-decorate", remoteRef+".."+localTip)
if err != nil || strings.TrimSpace(subjects) == "" {
return nil
}
commits := strings.Split(strings.TrimSpace(subjects), "\n")
files, _ := git.DiffNameOnly(ctx, sctx.WorkDir, remoteRef, localTip)
// Report the proposed PR, not a two-dot comparison that can count
// upstream-only changes as removals from an outdated local default tip.
base, baseErr := git.Run(ctx, sctx.WorkDir, "merge-base", remoteRef, "HEAD")
var files []string
var filesErr error
if baseErr == nil {
files, filesErr = git.DiffNameOnly(ctx, sctx.WorkDir, base, "HEAD")
}
fileEvidence := "PR file count unavailable"
if baseErr == nil && filesErr == nil {
fileEvidence = fmt.Sprintf("proposed PR changes %d file(s)", len(files))
}
firstFile := ""
if len(files) > 0 {
firstFile = files[0]
}
description := fmt.Sprintf(
"branch carries %d commit(s) that exist on your local %s branch but were never pushed to origin/%s; rebasing would bundle this unrelated work (%d file(s)) into the PR:\n- %s\n\nPush %s to origin, or rebase your branch onto origin/%s, before gating.",
len(commits), defaultBranch, defaultBranch, len(files), strings.Join(commits, "\n- "), defaultBranch, defaultBranch,
"branch carries %d commit(s) that exist on your local %s branch but were never pushed to origin/%s; these may be unintended bundled work (%s):\n- %s\n\nConfirm these commits belong in this PR before approving, or manually separate the intended work onto origin/%s before gating.",
len(commits), defaultBranch, defaultBranch, fileEvidence, strings.Join(commits, "\n- "), defaultBranch,
)
fixSummary := ""
if sctx.Fixing {
fixSummary = noChangesAppliedSummary
const explanation = "no changes applied: bundled local-default commits require manual separation or explicit approval"
description += "\n\n" + explanation + "; the rebase conflict resolver cannot safely select commits to discard."
sctx.Log(explanation)
}
findingsJSON, _ := json.Marshal(Findings{
Items: []Finding{{
Severity: "warning",
@@ -261,6 +302,7 @@ func detectBundledLocalDefaultCommits(ctx context.Context, sctx *pipeline.StepCo
NeedsApproval: true,
AutoFixable: false,
Findings: string(findingsJSON),
FixSummary: fixSummary,
}
}
@@ -58,6 +58,15 @@ func TestRebaseStep_DetectsUnpushedLocalDefaultBranchCommits(t *testing.T) {
gitCmd(t, dir, "commit", "-m", "my fix")
headSHA := gitCmd(t, dir, "rev-parse", "HEAD") // D0 + U + M
// Upstream-only files must not inflate the proposed PR evidence.
gitCmd(t, working, "checkout", "-b", "upstream-advance", d0)
if err := os.WriteFile(filepath.Join(working, "aaa_upstream_only.txt"), []byte("upstream"), 0o644); err != nil {
t.Fatal(err)
}
gitCmd(t, working, "add", "-A")
gitCmd(t, working, "commit", "-m", "advance upstream independently")
gitCmd(t, working, "push", "origin", "HEAD:main")
ag := &mockAgent{name: "test"}
sctx := newTestContextWithDBRecords(t, ag, dir, d0, headSHA, config.Commands{})
sctx.Run.Branch = "refs/heads/feature"
@@ -89,6 +98,17 @@ func TestRebaseStep_DetectsUnpushedLocalDefaultBranchCommits(t *testing.T) {
if findings.Items[0].Action != types.ActionAskUser {
t.Fatalf("finding action = %q, want %q", findings.Items[0].Action, types.ActionAskUser)
}
if !strings.Contains(outcome.Findings, "proposed PR changes 3 file(s)") || findings.Items[0].File != "my_fix.txt" {
t.Fatalf("expected actual PR file evidence: %s", outcome.Findings)
}
sctx.Fixing = true
outcome, err = step.Execute(sctx)
if err != nil || !outcome.NeedsApproval || !strings.Contains(outcome.FixSummary, "no changes applied") {
t.Fatalf("expected explicit unsupported fix: %#v, %v", outcome, err)
}
if got := gitCmd(t, dir, "rev-parse", "HEAD"); got != headSHA {
t.Fatalf("unsupported fix moved HEAD to %s", got)
}
}
func TestRebaseStep_DetectsUnpushedLocalDefaultBranchCommitsOnForcePush(t *testing.T) {
@@ -151,3 +171,55 @@ func TestRebaseStep_DetectsUnpushedLocalDefaultBranchCommitsOnForcePush(t *testi
t.Fatalf("expected findings to mention the bundled local main commit, got: %s", outcome.Findings)
}
}
// Issue #998: committing intended work on main before naming its delivery
// branch is not evidence of a separate bundled workstream.
func TestRebaseStep_LocalDefaultTipIsIntendedDelivery(t *testing.T) {
t.Parallel()
upstream := t.TempDir()
gitCmd(t, upstream, "init", "--bare")
dir := t.TempDir()
gitCmd(t, dir, "init")
gitCmd(t, dir, "config", "user.name", "test")
gitCmd(t, dir, "config", "user.email", "test@test.com")
gitCmd(t, dir, "checkout", "-b", "main")
for _, name := range []string{"package.json", "package-lock.json"} {
if err := os.WriteFile(filepath.Join(dir, name), []byte("base"), 0o644); err != nil {
t.Fatal(err)
}
}
gitCmd(t, dir, "add", "-A")
gitCmd(t, dir, "commit", "-m", "base")
base := gitCmd(t, dir, "rev-parse", "HEAD")
gitCmd(t, dir, "remote", "add", "origin", upstream)
gitCmd(t, dir, "push", "origin", "main")
for _, name := range []string{"package.json", "package-lock.json"} {
if err := os.WriteFile(filepath.Join(dir, name), []byte("upgrade"), 0o644); err != nil {
t.Fatal(err)
}
}
gitCmd(t, dir, "add", "-A")
gitCmd(t, dir, "commit", "-m", "intended dependency upgrade")
head := gitCmd(t, dir, "rev-parse", "HEAD")
gitCmd(t, dir, "checkout", "-b", "feature")
sctx := newTestContextWithDBRecords(t, &mockAgent{name: "test"}, dir, base, head, config.Commands{})
sctx.Run.Branch = "refs/heads/feature"
sctx.Repo.UpstreamURL = upstream
sctx.Repo.WorkingPath = dir
for _, fixing := range []bool{false, true} {
sctx.Fixing = fixing
outcome, err := (&RebaseStep{}).Execute(sctx)
if err != nil {
t.Fatal(err)
}
if outcome.NeedsApproval || outcome.SkipRemaining {
t.Fatalf("intended work rejected: %#v", outcome)
}
if fixing && !strings.Contains(outcome.FixSummary, "no changes applied") {
t.Fatalf("no-op misreported: %#v", outcome)
}
if got := gitCmd(t, dir, "rev-parse", "HEAD"); got != head {
t.Fatalf("HEAD moved: %s", got)
}
}
}
+1 -1
View File
@@ -334,7 +334,7 @@ func TestReviewFix_PostAgentCommitUsesStepParentContext(t *testing.T) {
if err != nil {
t.Fatalf("post-agent commit inherited invocation context: %v", err)
}
if summary != "fix timeout ownership" {
if summary != changesAppliedSummary {
t.Fatalf("summary = %q", summary)
}
if invocationDeadline.IsZero() {