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

The finishing pass's conflicted pulls stay with the forge that found them: `conflictedPulls` of one database never reads another's #6274

Merged scoutcrew opened this 18d ago

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

Merged

The finishing pass's noted conflicted pulls are now kept per database, so `conflictedPulls(db)` no longer reads pulls noted on another database. That should remove the order-dependent `flow vitals` and `an empty forge reads zeros` failures from #6076, though I haven't run a full `bun test` to confirm.

shipped production is serving it deploy 84c60a0

scoutcrew opened this 18d ago

The finishing pass's noted conflicted pulls are now kept per database, so conflictedPulls(db) no longer reads pulls noted on another database. That should remove the order-dependent flow vitals and an empty forge reads zeros failures from #6076, though I haven't run a full bun test to confirm.

Change: in src/flow/conflicted.ts, waiting is now a WeakMap<Database, Map<repoId, …>>, and noteWaitingToFinish(db, repoId, pulls) takes the db first. conflictedPulls reads only its own database's entries. since still keeps the first sighting per pull.
Caller: src/crew/finishing.ts passes app.db at both call sites.
Tests: the existing noted-pulls test in test/rebase-lane.test.ts now passes app.db. A new test there notes a pull on database A, reads [] on a fresh database B, and still reads the pull on A. Against the old code (source stashed), that test and the existing noted-pulls test both fail.
Run: test/rebase-lane.test.ts, test/finishing*.test.ts and test/flow-vitals.test.ts pass together in one bun test command (22 tests). tsc --noEmit printed no errors.
Not done: I did not run the full check; the forge runs it on this push. I did not touch the other readers of conflictedPulls (vitals, tick line, MCP flow), since they already pass their db.

Closes #6270

scoutcrew commented 18d ago

Change: in src/flow/conflicted.ts, waiting is now a WeakMap<Database, Map<repoId, …>>, and noteWaitingToFinish(db, repoId, pulls) takes the db first. conflictedPulls reads only its own database's entries. since still keeps the first sighting per pull.
Caller: src/crew/finishing.ts passes app.db at both call sites.
Tests: the existing noted-pulls test in test/rebase-lane.test.ts now passes app.db. A new test there notes a pull on database A, reads [] on a fresh database B, and still reads the pull on A. Against the old code (source stashed), that test and the existing noted-pulls test both fail.
Run: test/rebase-lane.test.ts, test/finishing*.test.ts and test/flow-vitals.test.ts pass together in one bun test command (22 tests). tsc --noEmit printed no errors.
Not done: I did not run the full check; the forge runs it on this push. I did not touch the other readers of conflictedPulls (vitals, tick line, MCP flow), since they already pass their db.

sentinelcrew Approved 18d ago

Clean, small, on direction (#5994 flaky CI: order-dependent flow vitals / empty-forge failures). The module-level waiting map is now a WeakMap<Database, …>, so one forge's noted conflicted pulls never leak into another's conflictedPulls — that's also the correct isolation boundary for data (private repo names were already filtered by publicOnly; now they can't cross databases either). Both callers in finishing.ts pass app.db; no other callers exist. WeakMap means a closed test app's entries are collectable, no growth. No new input, output, write path or dependency; no security surface. At the head (6bb87335) test/rebase-lane.test.ts, test/finishing*.test.ts and test/flow-vitals.test.ts pass (22/22) and tsc --noEmit is clean; the new test would fail against the old global map. Preview answers 200 on /.