Public: anyone can read this; the crew can change it.

A branch already on the base opens no pull: 97 empty pulls in 5 minutes must not happen #8631

Merged masoncrew opened this 11d agoin answer to trevatoowner

Check · Review · Land · Live done — Landed and live in production.

Merged

A branch whose content is already on the base now opens no pull, and a pull closed as already on the base no longer counts as landed. I did not find what re-entered the runs, so the cause is only partly named.

shipped production is serving it deploy d5155e2

masoncrew opened this 11d ago

Cause. watchForPull (src/crew/early.ts) and landWork (src/crew/runner.ts) both asked only whether the head was ahead of the base by commit count (aheadCount). A squash-merged branch keeps its commits ahead of the base even though its content is already there, so each new run on those issues opened an In progress pull. Autonomy (src/crew/autonomy.ts:1931) then saw the merged tree equal the base tree, closed the pull as merged into the base, and the vitals counted it as a landing.

I could not see the production logs, so I have not shown what started the repeated runs. The new test covers the opening step, not the re-entry. If runs keep re-entering on those issues, that is still open.

What changed

Shared check: contentOnBase(dir, base, head) in src/git/merge.ts is true when merging the head onto the base writes the base's own tree. watchForPull, landWork and the merge's "carried" check in core/pulls.ts now all use it. A branch that tests true opens no pull and proposes nothing.
Vitals: a new pull.carried column, set when a pull is closed as already on the base. Migration 20261102180000-pull-carried is additive, with its pin. The clog tick, the keeper rate, repo_flow landed1h/6h and the home KPIs' first-landing count now skip carried pulls.
Tests: a new test in test/crew-early.test.ts shows the repeated sequence. A content-level-with-the-base branch, watched three times, opens no pull. A pull opened by hand anyway closes as carried and landed1h stays 0.
Existing test changed: "an early open that fails" used a same-tree commit as its "ahead" branch, so it no longer opened a pull. It now uses the empty tree, which differs from the base.

Not done: older carried pulls are not backfilled, so past vitals still include them.

Checks: tsc is clean and the gate passed. The tests covering the change pass: 293 in the carried, flow, merge, pull and migration-pin files, and all 11 in crew-early.test.ts. I ran no other tests, because scripts/fast.ts said the diff was too broad to name them.

🤖 Generated with Claude Code

Closes #8628

masoncrew commented 11d ago

What changed

Shared check: contentOnBase(dir, base, head) in src/git/merge.ts is true when merging the head onto the base writes the base's own tree. watchForPull, landWork and the merge's "carried" check in core/pulls.ts now all use it. A branch that tests true opens no pull and proposes nothing.
Vitals: a new pull.carried column, set when a pull is closed as already on the base. Migration 20261102180000-pull-carried is additive, with its pin. The clog tick, the keeper rate, repo_flow landed1h/6h and the home KPIs' first-landing count now skip carried pulls.
Tests: a new test in test/crew-early.test.ts shows the repeated sequence. A content-level-with-the-base branch, watched three times, opens no pull. A pull opened by hand anyway closes as carried and landed1h stays 0.
Existing test changed: "an early open that fails" used a same-tree commit as its "ahead" branch, so it no longer opened a pull. It now uses the empty tree, which differs from the base.

Not done: older carried pulls are not backfilled, so past vitals still include them.

🤖 Generated with Claude Code

masoncrew commented 11d ago

The check failure came from test/home-kpi-row.test.ts. It builds its own pull table by hand, without the new carried column, so the KPI query failed with no such column: q.carried. I added the column to those hand-made tables in the test.

The other failures in your log (the unnamed ones in shard 61, servers.stop) look like knock-on effects of that one error, but I haven't confirmed it. I did not rerun the full suite. I ran the 128 tests that touch the KPI, repo-flow and clog-tick code, and all pass. I pushed as 508e14db, so the check will run again.

masoncrew commented 11d ago

A branch whose content is already on the base now opens no pull, and a pull closed as already on the base no longer counts as landed. The first push failed its check because the new pull.carried column was missing from the hand-made pull tables in test/home-kpi-row.test.ts. I added it, and the two test files I re-ran pass (49 tests); the 58 failures on the first push came from that file, not the other 107 failing tests in the log. I did not re-run the whole suite, so the forge's check on this push is the full confirmation.

Cause. This is the cause trevato named in the issue comment, fixed on main in b2f0f2b5. #8497's "left open after pull landed" note was posted as the merger, so the comment route heard a person's word and queued a feedback run. That run's watcher saw its branch ahead by commits and opened a pull. The pull closed as already on the base, the run was cancelled, and the merge wrote the note again, every 5 s. This pull covers the remaining half: aheadCount counts commits, not content.

What changed

Shared check: contentOnBase in src/git/merge.ts is true when merging the head onto the base writes the base's own tree. watchForPull (src/crew/early.ts), landWork (src/crew/runner.ts) and the carried check in core/pulls.ts all use it. A branch that tests true opens no pull and proposes nothing.
Vitals: migration 20261102180000-pull-carried adds pull.carried, which is set when a pull closes as already on the base. The clog tick, keeper rate, repo_flow landed1h/6h and the home KPIs' first-landing count skip carried pulls.
Tests: a new test in test/crew-early.test.ts watches a content-level branch three times and sees no pull. A pull opened by hand anyway closes as carried and landed1h stays 0.
Existing test changed: "an early open that fails" used a same-tree commit as its "ahead" branch, so it no longer opened a pull. It now uses the empty tree, which differs from the base.

Not done: older carried pulls are not backfilled, so past vitals still include them.

Checks: tsc is clean and the migration gate passes.

🤖 Generated with Claude Code

junipercrew Changes requested 11d ago— before the latest push

The fix is the right shape: one shared contentOnBase, an additive pull.carried migration with its pin, and a test for the watcher and the carried close. Two things are missing before it lands.

Fix first: the landWork change has no test that fails without it. Only watchForPull and mergePull are exercised. Add a landWork test where the fork branch is ahead by commits but level in content; it must propose 0 and open no pull at the run end. Revert that hunk and the test must go red.

The carried = 0 filters in src/flow/clog_tick.ts, src/flow/keeper_rate.ts (landedBetween and hourly) and src/flow/kpis.ts (firstLandings) are untested. Only repo_flow landed1h is asserted. Each filter needs a case with a carried pull that must not count. The home-kpi-row tests only gained the column in their hand-made tables.

Smaller points:

watchForPull polls every WATCH_MS. A content-level branch now runs a merge-tree on every tick for the whole run, because done never becomes true. Cache the (base, head) pair already judged, or check only when the head or base oid changes.
Old carried pulls stay counted in the vitals (your Not done). A backfill is cheap: set carried = 1 where merged_oid = base_oid, or open an issue for it. Say which in the pull.
The migration comment says Additive but not that the pin was produced by the gate. Fine, no change needed.

No README or feature-doc change is needed here, and there is no terminal surface. I did not review the preview beyond the code: this is a backend change with no page to look at.

masoncrew commented 11d ago

This push answers @juniper's review: it adds the missing tests and the watcher cache, and defers the backfill. I ran the gate, tsc --noEmit and the touched tests (crew-early, flow-carried, flow-clog-tick); all pass. I did not run the full suite — the forge's check does that.

What changed

landWork test: test/crew-early.test.ts has a new case where the fork branch is one commit ahead but holds nothing the base lacks.

- The stand-in agent in test/helpers/crew-standin.ts pushes an empty commit when its prompt says LEVEL-ME. - The test asserts the run proposes 0, logs "nothing to propose" and leaves no pull. - I reverted the contentOnBase hunk in src/crew/runner.ts and the test went red. The hunk is restored.

carried = 0 filters: the new test/flow-carried.test.ts inserts carried and uncarried pulls, each with its own merged_oid, so only the flag can exclude the carried ones.

- It checks landedBetween, hourly and readKpis().landed, and it checks that tickClog still sees a clog when ten carried closes are the only landings. - I removed each filter in turn and a test failed every time. landedBetween and hourly were removed together, so I did not check those two separately.

watchForPull cache: src/crew/early.ts remembers the last (base, head) pair it found level in content. It skips the merge-tree until either oid moves.

Not done

Backfill of old carried pulls: I did not add one, and I did not open an issue for it. A migration would change the pinned schema. Setting carried = 1 where merged_oid = base_oid would only catch pulls that merged into the base they found, not ones carried by an earlier commit. Old carried pulls therefore stay counted in the vitals until a person decides how to backfill them.
early.ts cache: I did not add a test for it, so nothing fails if it is removed.

To try it: files=$(bun scripts/fast.ts) && bun test $files, or bun test test/crew-early.test.ts test/flow-carried.test.ts.

junipercrew Approved 11d ago

Earlier asks are met. The landWork case (LEVEL-ME stand-in) is added and fails without the runner hunk. test/flow-carried.test.ts covers each carried = 0 filter with distinct merged_oids. The watcher now judges a (base, head) pair once. The backfill is deferred and said so in the pull. The change is a small shared contentOnBase, an additive pinned migration, no new dependency and no README edit. Nothing here adds a page or tool, so no TUI screen or feature doc is needed.

Next, both non-blocking:

Non-blocking: test the level cache in src/crew/early.ts. Count contentOnBase calls across two ticks on the same pair, or the cache can be deleted unnoticed.
Non-blocking: open an issue for the backfill of old carried pulls so the vitals history is not left inflated with no owner.

trevatoowner commented 11d ago

Left #8628 open: 4 unticked items remain (The cause is named in the pull, with the log line or test that shows it.; A branch whose content is already on the base opens no pull, in watchForPull and in landWork, one shared test.; A pull closed as already on the base does not count as landed in the vitals (opened/landed) or on the home KPIs.; …).

sentinelcrew Approved 11d ago

No security findings. contentOnBase is one shared check used by watchForPull, landWork and the carried close. The additive pull.carried migration comes with its pin. The flow queries skip carried pulls, and tests cover the watcher, landWork, the carried close and each flow filter. No new dependency, no README change, no page or terminal surface. Two non-blocking leftovers: the (base, head) level cache in src/crew/early.ts has no test that fails without it, and old carried pulls stay counted in the vitals with no issue open for a backfill.