From 0b3f9c7151ae6a3e2bb0db065a34a820d13b30b2 Mon Sep 17 00:00:00 2001 From: "linh.doan" Date: Thu, 17 Sep 2026 09:07:27 +0700 Subject: [PATCH] devagent(TASK-mu4vqg88-5vdd): auto-cleanup snapshot --- docs/PRD.md | 32 ++++++++++-- internal/lessons/guard.go | 83 +++++++++++++++++++++++++++---- internal/lessons/guard_test.go | 60 ++++++++++++++++++++++ internal/loopdriver/state.go | 16 +++++- internal/loopdriver/state_test.go | 30 +++++++++++ 5 files changed, 206 insertions(+), 15 deletions(-) diff --git a/docs/PRD.md b/docs/PRD.md index 214fe572..2c2556b8 100644 --- a/docs/PRD.md +++ b/docs/PRD.md @@ -1071,6 +1071,28 @@ Webhook-triggered runs with HMAC verification and dedup, run dashboard/status co > head-gone, reopen-refused) and > `TestVerifyAndMergeRescueRefusesNonReopenablePR` > (`internal/loopdriver/run.go`, `issues.go`). +> +> 2026-09-17 (issue #361: the lessons ratchet dedupes on content, so a repeat +> is not a write): the propose→evaluate→accept guard, the Q39 impact telemetry +> and the held-out must-beat tier all shipped in the Go port +> (`internal/lessons`, FR-GO-07 #194), but the loop's own write path did not +> use them: the ratchet merge of the state branch collapsed only *exact* +> duplicate lines (`stateSync.mergeLessons`), so a lesson re-landed with +> reworded punctuation — `Lessons eval guard` vs `Lessons-eval-guard`, the +> live `.selfbuild/lessons.md` regression — survived as a second copy and +> spent the 4000-char `lessonsMaxChars` budget the digest is bounded by. +> `lessons.DedupeLessonContent` now applies the guard's own similarity policy +> (normalized word-trigram Jaccard at the 0.8 +> `DefaultLessonsDedupeSimilarity`, first occurrence wins) to the merged body, +> so appending an existing lesson — verbatim or reworded — is a no-op; +> structural lines (blank, `---`, headings) always survive so the ratchet +> keeps its dated sections, and the merge is idempotent. Measured on the live +> file: 123,009 → 122,448 bytes, and the digest's newest-40-line window +> 15,320 → 14,759 bytes. Pinned by `TestDedupeLessonContent` and +> `TestMergeLessonLinesNearDuplicateAppendIsNoop`. Named ceiling, unchanged +> from the guard: the 0.4–0.7 near-dup band still lands on purpose, so a v2 +> rewrite of a shipped lesson can be re-measured +> (`internal/lessons/guard.go`, `internal/loopdriver/state.go`). ~~- **Cross-board retry memory beyond the SHA guard** — commit 60638d3 stops re-issuing shipped goals, but re-queued failures still get a fresh attempt budget; carry the prior board's failure class onto the re-bridged goal so the scout deprioritizes until the root-cause fix lands (Q27).~~ ~~- **Regression oracle before board merge** — gates judge single PRs and PR #108's committed STRIDE allowlist widens suppression paths; add a board-level "is the system at least as good?" check (full suite on the merged result) ahead of `autoMerge`, per the Kitchen Loop zero-regression rule.~~ @@ -1109,10 +1131,11 @@ Direction addendum 2026-09-03 (section 20): DevAgent becomes the local-first, BY > `herdr-sweep --orphan-brokers` remains Node-only per the HERDR.md status > note (#360); FR-CTX-01..05 remain unimplemented (#368, see §20.1); and > docs/PRODUCTION-READINESS.md still describes the retired Node tree, so no -> Go-era readiness verdict exists (#362). Lower-priority convergence work, -> also issue-first: lessons-digest dedupe + eval guard (#361), Windows -> parity (#364), parity-shim convergence + vestigial exit-3 machinery -> (#365), ClaudeCode adapter hardening (#366), internal/cli + +> Go-era readiness verdict exists (#362); the lessons-digest dedupe + eval +> guard gap (#361) closed 2026-09-17 on the ratchet write path (Phase 4 +> history entry above). Lower-priority convergence work, also issue-first: +> Windows parity (#364), parity-shim convergence + vestigial exit-3 +> machinery (#365), ClaudeCode adapter hardening (#366), internal/cli + > cmd/devagent test coverage (#367), War Room as a first-class Go command > (#369). - **Easy handoff / control plane (FR-HAND, #145)** — Shipped 2026-09-08: cold path is exactly `devagent init` → `devagent tui` (optional `start` alias); TUI dispatch sheet (`n`, FR-HAND-02) + approve sheet (`g`, FR-HAND-07); `POST /dispatch` threads `autoPr` into the spawned `task --auto-pr` argv gated on `GITHUB_TOKEN` (FR-HAND-03); init adds worker-detect chips (FR-HAND-04), herdr default-on advisory (FR-HAND-05), Orca repo registration without worktree provisioning (FR-HAND-06). Look/feel bar: #146. @@ -1756,6 +1779,7 @@ existing driver with validation surfaces (test, command, telemetry, chaos schedule) so "the driver works perfectly" is a checkable claim, not a hope. --- +*Last updated: 2026-09-17 (issue #361: the lessons ratchet dedupes on content, not just on exact lines) — the selfbuild lessons file is no longer write-only on the loop's own path: `stateSync.mergeLessons` collapsed only exact duplicate lines, so a lesson re-landed with reworded punctuation (`Lessons eval guard` vs `Lessons-eval-guard` — the live `.selfbuild/lessons.md` regression) landed as a second copy and spent the 4000-char `lessonsMaxChars` digest budget. The merge now runs `lessons.DedupeLessonContent` — the guard's own normalized word-trigram Jaccard policy at the 0.8 `DefaultLessonsDedupeSimilarity`, first occurrence wins, structural lines always kept, idempotent — so appending an existing lesson, verbatim or reworded, is a no-op (measured on the live file: 122,448 bytes vs 123,009; the newest-40-line digest window 14,759 vs 15,320). The guard, the Q39 accept/reject × loop-result telemetry and the held-out must-beat tier were already shipped in the Go port; this closes the convergence gap on the loop's write path. Pinned by `TestDedupeLessonContent` and `TestMergeLessonLinesNearDuplicateAppendIsNoop` (`internal/lessons/guard.go`, `internal/loopdriver/state.go`). *Last updated: 2026-09-14 (the PRD became an input, and `up` became a health receipt: FR-SIMPLE-07/08, #370 + #371) — `devagent up` now covers the whole setup-to-running path (prerequisite gate through the `init` checks → state/queue dirs → `docs/PRD.md` intake → a `lane` census of pending rows / open PRD checkboxes / open `selfbuild` issues → daemon → detached driver launched from its own executable with `SELFBUILD_DEVAGENT_BIN` pinned to match) and **proves** the start rather than reporting a pid: `up` is the driver's parent, so a driver that halts at its own gate exits 0 and stays a zombie whose pid answers any liveness probe (measured 2026-09-14: `max iterations reached` on stderr while `up` printed a healthy pid for fifteen more seconds), so it now reaps its own child and waits (bounded by `--wait`, default 15s) until the loop lock AND a phase-naming heartbeat exist — `driver running (pid N) — holds the loop lock, iteration 2, phase queue-empty` — or fails the run with the halt line read out of `.selfbuild/logs/`. Work intake flipped the other way too: an open `- [ ]` checkbox in `docs/PRD.md` is now an instruction (`internal/prdintake` queues it with its heading as context, its sub-bullets as criteria and its section body as the task-PRD sidecar; the driver ingests at iteration head after the PRD-currency gate, `SELFBUILD_PRD_INTAKE=0` opts out) and the shipped PR ticks the checkbox it built, so a built item cannot re-enter the lane. Queue-first already outranked the tracker, so operator intent now outranks LLM self-selection — the value half of #355, whose other half (an empty lane logged and breadcrumb'd as `queue-empty` with the refill instructions instead of reading as progress) landed in the same change. `devagent create --scout/--tracker` also stopped baking the deleted `dist/src/cli.js` into LaunchAgent argv, and the scout cycle itself is live in Go at last (#372). *Last updated: 2026-09-13 (caveat on #349's revision stamp: linked-worktree builds mis-stamp — [#352](https://github.com/FreePeak/devagent/issues/352)) — while verifying #349's landing we found Go's `-buildvcs` resolves `vcs.revision` from the shared common gitdir in a linked worktree, so a `devagent` binary built inside a per-task worktree stamps the primary checkout's `refs/heads/main` tip rather than its own HEAD (`vcs.modified=true`). Proven A/B at the same commit `8ca0688`: a plain clone stamps `8ca0688`, a linked worktree stamps `50b03e8`. Scope: this is a latent defect confined to *worktree-built* binaries — the shipped driver is built by `scripts/self-update.sh` from the primary non-worktree checkout (`go build -trimpath ./cmd/devagent` after `git pull --ff-only`), so it stamps correctly and `RunLoop`'s stale-binary guard behaves as intended in the live loop; only a manually-built worktree binary writes a wrong `release-created` revision and false-trips the (advisory-only) WARN. #349's "a stale binary is loud" holds for the normal-checkout build path; the worktree edge is tracked in #352. *Last updated: 2026-09-13 (release-created rows carry `tag`/`sha` again: PR #349's Revision stamp landed as a silent data-loss regression) — #349 added a `Revision` field to the Go `ReleaseRecord` but dropped `Tag`/`SHA` from the `record release` literal while still printing `(tag @ sha)` to stdout, so every Go-written release row persisted `"tag":""` `"sha":""` (the Q24 fields the record exists to carry) and no test covered the write site, so it shipped green. Restored both fields; `internal/cli/actions_release_test.go` now pins the CLI row shape (proven red on the regressed literal, green after); regenerated `docs/PRD.html` so the styled mirror matches `PRD.md`. diff --git a/internal/lessons/guard.go b/internal/lessons/guard.go index bfd64ca6..01402c96 100644 --- a/internal/lessons/guard.go +++ b/internal/lessons/guard.go @@ -713,8 +713,14 @@ func LessonShingles(text string) map[string]bool { // so an empty candidate can never bypass the guard by containing nothing to // compare. func LessonSimilarity(a string, b string) float64 { - sa := LessonShingles(a) - sb := LessonShingles(b) + return shingleSimilarity(LessonShingles(a), LessonShingles(b)) +} + +// shingleSimilarity is the trigram-Jaccard score of two already-built +// shingle sets — the body of LessonSimilarity, split out so a caller +// comparing one line against many (the ratchet dedupe below) reuses the kept +// lines' shingles instead of rebuilding them for every pair. +func shingleSimilarity(sa, sb map[string]bool) float64 { if len(sa) == 0 && len(sb) == 0 { return 1 } @@ -731,6 +737,70 @@ func LessonSimilarity(a string, b string) float64 { return float64(inter) / float64(union) } +// isLessonContentLine reports whether a raw lessons-file line is lesson +// content — the granularity the guard compares — rather than structure: +// blank lines, `---` fences, and markdown headings carry no lesson, so they +// are never dedupe candidates and always survive verbatim. The single filter +// rule behind ReadLessonEntries and DedupeLessonContent. +func isLessonContentLine(line string) bool { + trimmed := strings.TrimSpace(line) + if trimmed == "" || trimmed == "---" { + return false + } + return !lessonHeaderRe.MatchString(trimmed) +} + +// DedupeLessonContent collapses content-similarity repeats in a lessons file +// body (issue #361). The lessons ratchet merge is the self-build loop's write +// path for machine appends; exact-line dedupe alone let near-duplicate +// rewordings of a lesson the digest already carries accumulate until they +// spent the LessonsMaxChars budget on repeats instead of on new lessons. +// +// The first occurrence of each content line wins (the ratchet's +// `awk '!seen[$0]++'` convention: remote content merges ahead of local +// appends); a later line whose normalized word-trigram similarity to an +// already-kept content line is at or above threshold is dropped as a repeat. +// Structural lines (see isLessonContentLine) always survive, so the merged +// file keeps its dated section layout. threshold <= 0 means +// DefaultLessonsDedupeSimilarity. +// +// Pure function. Line joining and the trailing newline mirror the line merge +// it guards: "" for an empty result, otherwise a trailing "\n". +func DedupeLessonContent(text string, threshold float64) string { + if threshold <= 0 { + threshold = DefaultLessonsDedupeSimilarity + } + lines := strings.Split(text, "\n") + if len(lines) > 0 && lines[len(lines)-1] == "" { + lines = lines[:len(lines)-1] + } + kept := make([]map[string]bool, 0, len(lines)) + out := make([]string, 0, len(lines)) + for _, line := range lines { + if !isLessonContentLine(line) { + out = append(out, line) + continue + } + shingles := LessonShingles(line) + repeat := false + for _, other := range kept { + if shingleSimilarity(shingles, other) >= threshold { + repeat = true + break + } + } + if repeat { + continue + } + kept = append(kept, shingles) + out = append(out, line) + } + if len(out) == 0 { + return "" + } + return strings.Join(out, "\n") + "\n" +} + // LessonsDedupeResult is the result of a lessons dedupe check (and, with // the gated-append fields set, of AppendLessonGuarded). type LessonsDedupeResult struct { @@ -773,14 +843,7 @@ func ReadLessonEntries(lessonsPath string) []string { } out := []string{} for _, line := range strings.Split(string(raw), "\n") { - trimmed := strings.TrimSpace(line) - if trimmed == "" { - continue - } - if trimmed == "---" { - continue - } - if lessonHeaderRe.MatchString(trimmed) { + if !isLessonContentLine(line) { continue } out = append(out, strings.TrimRight(line, " \t\r\n")) diff --git a/internal/lessons/guard_test.go b/internal/lessons/guard_test.go index e074f7c9..e7f9189b 100644 --- a/internal/lessons/guard_test.go +++ b/internal/lessons/guard_test.go @@ -294,6 +294,66 @@ func TestCheckLessonsDedupe(t *testing.T) { }) } +// --------------------------------------------------------------------------- +// dedupeLessonContent (the ratchet merge's content-similarity dedupe, #361) +// --------------------------------------------------------------------------- + +func TestDedupeLessonContent(t *testing.T) { + lesson := "- **Lessons eval guard is the single best next backlog item**: the digest is write-only, so a repeat spends the budget." + hyphenDup := strings.Replace(lesson, "Lessons eval guard", "Lessons-eval-guard", 1) + distinct := "- Fencing tokens kill double dispatch even after kill -9 and lease reclaim." + + t.Run("appending an existing lesson is a no-op", func(t *testing.T) { + file := "## 2026-09-02\n\n" + lesson + "\n" + if got := DedupeLessonContent(file+lesson+"\n", 0); got != file { + t.Fatalf("exact repeat changed the file:\ngot %q\nwant %q", got, file) + } + }) + + t.Run("a near-duplicate reword is dropped, the first occurrence wins", func(t *testing.T) { + file := "## 2026-09-02\n\n" + lesson + "\n" + if got := DedupeLessonContent(file+hyphenDup+"\n", 0); got != file { + t.Fatalf("near-duplicate changed the file:\ngot %q\nwant %q", got, file) + } + }) + + t.Run("keeps a distinct lesson and every structural line", func(t *testing.T) { + file := "## 2026-09-02\n\n" + lesson + "\n---\n" + distinct + "\n" + if got := DedupeLessonContent(file, 0); got != file { + t.Fatalf("distinct content changed:\ngot %q\nwant %q", got, file) + } + }) + + t.Run("honors the threshold override", func(t *testing.T) { + // The near-dup-band pair from TestCheckLessonsDedupe: admitted at the + // 0.8 default (a v2 rewrite is allowed to land so its effect can be + // re-measured), collapsed at 0.5. + first := "one two three four five six seven eight" + second := "one two three four five six seven nine" + file := first + "\n" + second + "\n" + if got := DedupeLessonContent(file, 0); got != file { + t.Fatalf("default threshold dropped a near-dup-band lesson: %q", got) + } + if got, want := DedupeLessonContent(file, 0.5), first+"\n"; got != want { + t.Fatalf("threshold 0.5 should collapse the near-dup band:\ngot %q\nwant %q", got, want) + } + }) + + t.Run("is idempotent: the merged file is a fixed point of the next merge", func(t *testing.T) { + file := "## 2026-09-02\n\n" + lesson + "\n" + hyphenDup + "\n" + distinct + "\n" + once := DedupeLessonContent(file, 0) + if twice := DedupeLessonContent(once, 0); twice != once { + t.Fatalf("second merge changed the file:\ngot %q\nwant %q", twice, once) + } + }) + + t.Run("empty input stays empty", func(t *testing.T) { + if got := DedupeLessonContent("", 0); got != "" { + t.Fatalf("got %q, want empty", got) + } + }) +} + // --------------------------------------------------------------------------- // appendLessonGuarded (eval-gated append: impact → dedupe → evaluate) // --------------------------------------------------------------------------- diff --git a/internal/loopdriver/state.go b/internal/loopdriver/state.go index 402598c5..c4460abe 100644 --- a/internal/loopdriver/state.go +++ b/internal/loopdriver/state.go @@ -15,6 +15,7 @@ import ( "time" "github.com/FreePeak/devagent/internal/git" + "github.com/FreePeak/devagent/internal/lessons" ) const ( @@ -175,16 +176,29 @@ func extractField(re *regexp.Regexp, line, marker string) string { return strings.TrimPrefix(line[loc[0]:loc[1]], marker) } +// mergeLessons is the lessons ratchet's write path: the union of the remote +// state-branch file and the local one. func (s *stateSync) mergeLessons() { remote := s.fileAtState(".selfbuild/lessons.md") local, _ := os.ReadFile(s.lessonsPath()) _ = os.MkdirAll(filepath.Dir(s.lessonsPath()), 0o755) // Bash touches the lessons file first, so an empty merge still leaves an // (empty) file behind — mirror that. - merged := dedupeLines(remote + string(local)) + merged := mergeLessonLines(remote, string(local)) _ = os.WriteFile(s.lessonsPath(), []byte(merged), 0o644) } +// mergeLessonLines is the pure body of the lessons merge: the remote file +// first, then the local one, with repeats collapsed — exact duplicates (the +// `awk '!seen[$0]++'` ratchet port) and, since issue #361, near-duplicates on +// content similarity, so an append that only rewords a lesson the digest +// already carries is a no-op instead of a repeat that spends the +// LessonsMaxChars budget. First occurrence wins; structural lines always +// survive (DedupeLessonContent). +func mergeLessonLines(remote, local string) string { + return lessons.DedupeLessonContent(dedupeLines(remote+local), lessons.DefaultLessonsDedupeSimilarity) +} + // dedupeLines ports `awk '!seen[$0]++'`: keep the first occurrence of every // line. Only the final empty element produced by a trailing newline is // dropped; interior blank lines are ordinary lines to awk and stay. diff --git a/internal/loopdriver/state_test.go b/internal/loopdriver/state_test.go index 1556b641..51ad3403 100644 --- a/internal/loopdriver/state_test.go +++ b/internal/loopdriver/state_test.go @@ -58,6 +58,36 @@ func TestDedupeLinesKeepsFirstOccurrence(t *testing.T) { } } +// Issue #361: the ratchet merge is the loop's write path for machine appends, +// so re-appending a lesson the file already carries — verbatim or reworded — +// must leave the merged file byte-identical instead of spending the digest +// budget on a repeat. +func TestMergeLessonLinesNearDuplicateAppendIsNoop(t *testing.T) { + lesson := "- Lessons eval guard is the single best next backlog item: the digest is write-only, so repeats spend the budget." + // The live .selfbuild/lessons.md shape: the same lesson appended again as + // a hyphen-only reword. + reword := strings.Replace(lesson, "Lessons eval guard", "Lessons-eval-guard", 1) + distinct := "- Fencing tokens kill double dispatch even after kill -9 and lease reclaim." + remote := "## 2026-09-02\n\n" + lesson + "\n" + + t.Run("appending an existing lesson is a no-op", func(t *testing.T) { + if got := mergeLessonLines(remote, remote); got != remote { + t.Fatalf("merge changed the file:\ngot %q\nwant %q", got, remote) + } + if got := mergeLessonLines(remote, lesson+"\n"); got != remote { + t.Fatalf("re-appending the lesson changed the file:\ngot %q\nwant %q", got, remote) + } + }) + + t.Run("a reworded repeat is merged away, a distinct lesson lands", func(t *testing.T) { + got := mergeLessonLines(remote, reword+"\n"+distinct+"\n") + want := remote + distinct + "\n" + if got != want { + t.Fatalf("merge mismatch:\ngot %q\nwant %q", got, want) + } + }) +} + func TestStatePullFreshRemote(t *testing.T) { repo := initFixtureRepo(t) addBareOrigin(t, repo)