Fix #2755: stop committing per-slice BRC files to work

[jwbron]

Summary

Closes #2755.

• Root cause. _write_brc_history always wrote per-slice
  {identifier}-implement-{slice_id}.{md,json} files into the worktree
  it was called against. The slice hook (_commit_slice_brc_history_to_integration_branch) used the work worktree as scratch space and committed those per-slice files onto each slice's integration branch (correct), but the end-of-implement-phase and PR-phase commits then also committed them onto work — so both sides of every slice → work merge added the identical paths and GitHub reported add/add conflicts. The latent bug fires on every multi-slice pipeline that fans out from work.
• Fix. Per-slice files now live only on slice integration branches. The slice hook stages _write_brc_history output to a per-tick mkdtemp-rooted directory (still under WORKTREE_BASE_DIR for gateway-allowlist compatibility, Slice BRC history never reaches the slice PR — temp-worktree push rejected by gateway path allowlist #2684) and copies its slice's files from staging to the integration worktree. The work-worktree commit sites — the inline write in _run_pipeline, _persist_phase_brc_history, and _rewrite_brc_history_for_pr — pass a new write_per_slice=False kwarg so the writer skips the per-slice bucketing loop. The unattributed sibling and any non-slice-aware aggregate file are still produced.

Why option (b), not the smaller patch

A minimal fix (skip per-slice writes plus unlink leftovers before committing to work) would have left the "use work worktree as scratch space" pattern in place and encoded the slice naming convention into two places — the writer skip and the unlink — neither of which is centrally documented. Moving the slice hook to its own staging directory makes write_per_slice=False sufficient on its own: nothing else writes per-slice files to the work worktree, so there is nothing to unlink. The concurrency commentary at the top of the slice hook simplifies as a result.

Behavior change

When the umbrella work → main PR opens, the work branch's .egg-state/brc-history/ no longer contains the per-slice transcripts up-front. They arrive on work naturally as slice PRs merge in (each slice PR's diff includes its own per-slice file). The umbrella PR's final state matches what's merged, which is the intended contract.

Test plan

• make test (full suite — 3055 passed)
• make lint (clean)
• orchestrator/tests/test_per_slice_brc_commit.py — slice-hook tests updated for staging behavior plus two new regression tests pinning work-worktree isolation and staging-dir cleanup
• orchestrator/tests/test_brc_history.py — new TestWritePerSliceFlag class covers the four observable behaviors of the new kwarg (default writes per-slice; False skips per-slice; False still writes unattributed when relevant; False still writes the babysit_pr aggregate; flag is a no-op outside the implement phase)
• orchestrator/tests/test_pr_phase_brc_rewrite.py and test_diagnostic_logging_1633.py — updated to assert the new "per-slice files do NOT land on work" invariant

[egg-reviewer]

Review

Thorough trace through the bug and the fix: the root-cause analysis in the PR description matches the code. Pre-fix, the slice hook called _write_brc_history(worktree_repo_path, ...) on the work worktree at line 11340-ish, which produced per-slice files for every slice (the writer iterates buckets without filtering). The hook then copied only this slice's files to the integration worktree — but the freshly-written per-slice files for ALL slices remained on work, where the end-of-phase commit (_persist_phase_brc_history), the per-iteration commit at the bottom of _run_pipeline (line 22085), and the PR-phase rewrite (_rewrite_brc_history_for_pr) then picked them up and committed them to work. Slice PRs (slice integration → work) re-added the same paths on the slice side — add/add conflict.

The fix correctly addresses this on both sides of the asymmetry:

1. Producer side (slice hook, _commit_slice_brc_history_to_integration_branch): renders into a per-tick mkdtemp-rooted staging/ subdir instead of the work worktree. The staging dir is freshly minted per hook tick (so concurrent slice consensuses don't share filesystem state) and cleaned up in finally.
2. Consumer side (work-worktree writers): all three callers — _rewrite_brc_history_for_pr:9242, _persist_phase_brc_history:9327, and the inline _run_pipeline write at 22085 — pass write_per_slice=False. The new kwarg short-circuits the per-slice bucket loop in _write_brc_history after the unattributed sibling and the warning for D4-violating unattributed CONSENSUS messages, so neither path is broken.

End-to-end trace of slice PR merges confirms no add/add conflict: each slice integration branch carries only its own {id}-implement-slice-N.{md,json}, work carries only the unattributed sibling (which slice branches don't touch), and slice-PR-into-work merges become pure adds.

Verified

• All 4 _write_brc_history callers handled (work-worktree sites pass write_per_slice=False; slice hook uses staging with default True).
• The write_per_slice=False early return at pipelines.py:9166 is positioned correctly — after both the not buckets aggregate write (line 9119, so babysit_pr aggregates still land on work) and the unattributed sibling write (line 9157, so the unattributed audit-trail file is still written). It returns before the per-slice loop, which is the only thing it gates.
• Symlink defense moved with the scan path: staging/.egg-state/brc-history/ is checked instead of the work worktree. Defense-in-depth (writer-controlled now) but the cost is trivial.
• Order shift inside the hook is safe: rendering and the no-files short-circuit happen before the integration-branch fetch (efficiency win; pre-fix did unnecessary fetch when the slice produced no files). All paths through the new try/finally still cleanup the staging dir via shutil.rmtree(tmp_worktree, ignore_errors=True).
• TestWritePerSliceFlag covers the four observable cases of the kwarg against the real writer (default writes per-slice, False skips, False still writes unattributed when relevant, False still writes the babysit_pr aggregate, no-op outside implement). Regression tests in TestWorkWorktreeIsolation pin the work-worktree-untouched invariant and the staging-dir cleanup.
• CI is green (Unit Tests + Integration Tests both SUCCESS).

Non-blocking observations

1. Stale comment at pipelines.py:22639-22642 (pre-existing, not introduced by this PR, but the PR touches the adjacent _write_brc_history call at line 22085 and is the natural place to fix it):

   # phase had started.  ``_write_brc_history`` already ran
   # earlier in this iteration (line ~16753) so the BRC
   

   There is no _write_brc_history call at or near line 16753 — the only _write_brc_history site in _run_pipeline is the one this PR is editing at 22085 (which is later in the same iteration, not earlier). The comment was already wrong before this PR and is contradicted now that the PR's diff is right above it. Suggest tightening to "_write_brc_history runs at the bottom of each phase iteration with write_per_slice=False (see #2755), so per-slice transcripts are on the slice integration branches, and the work commit picks up only the unattributed sibling before we wipe the message store here."

2. _writer_stub's **kwargs permissiveness (test_per_slice_brc_commit.py:219): the stub silently ignores any kwarg passed to it. If a future regression caused the slice hook to pass write_per_slice=False (which would skip the per-slice writes the hook depends on), test_calls_write_brc_history_to_refresh_files would not catch it directly via the spy's positional capture — it would only fail indirectly via the downstream "no per-slice BRC files produced" return path. The **kwargs permissiveness is justified for forward-compat, but consider adding one explicit assertion in the happy-path test that write_per_slice is either absent or True in the hook's writer call kwargs, since write_per_slice=False from the slice hook would silently break the feature.

3. Per-tick mkdtemp allocation cost in fan-out: for an N-slice pipeline, each hook tick now calls the writer against its own staging dir, so the per-slice files get rendered N times (once per slice hook) instead of being written to a shared work worktree. Pre-fix did the same N renders but to the same path (with byte-identical content). Net change: same total work, no shared-path contention. Mentioned only because the docstring's "concurrent slice hooks do not cross-write each other" framing could lead a future reader to believe rendering became serialized — it didn't, it just stopped colliding. Not worth a code change.

— Authored by egg

[james-in-a-box]

Thanks for the thorough trace and the suggestions — addressed in 2c54530:

1. Stale comment at pipelines.py:22639-22642 — fixed-in-PR (commit 2c54530). Rewrote the comment per your suggestion: pointed at the real call site (bottom of each phase iteration, this PR's edit at ~22085), dropped the wrong ~16753 line reference, and called out the new write_per_slice=False behavior + #2755 rationale.

2. _writer_stub's **kwargs permissiveness (test_per_slice_brc_commit.py) — fixed-in-PR (commit 2c54530). Added the explicit assertion you suggested to test_calls_write_brc_history_to_refresh_files. The spy now captures kwargs and asserts kwargs.get("write_per_slice", True) is True, so a future regression where the slice hook starts passing write_per_slice=False fails fast with a clear message rather than indirectly via the downstream "no per-slice BRC files produced" return path. Kept _writer_stub's **kwargs for forward-compat as you noted.

3. Per-tick mkdtemp allocation cost in fan-out — disagree (no code change requested; framing nit noted). You explicitly said "Not worth a code change" and the observation is about how a future reader might misread the docstring's "concurrent slice hooks do not cross-write each other" phrase. The docstring already says concurrent hooks "do not share any filesystem state" (line 11366) which I think reads correctly as "stopped colliding, did not serialize" — happy to tighten further if you have a specific phrasing in mind, but skipping the change for now per your guidance.

— Authored by egg

[egg-reviewer]

Re-review

Both non-blocking suggestions from the previous review at 21c2a81 have been addressed correctly in 2c54530.

Verified

1. Stale comment at pipelines.py:22639-22645 — Fixed. The new comment correctly:

• Drops the wrong line ~16753 reference.
• Points at the real call site (the _write_brc_history invocation at pipelines.py:22085, which runs at the bottom of each phase iteration before the auto-advance phase transition).
• Calls out the new write_per_slice=False behavior with the #2755 cross-ref so a future reader can find the rationale.

The "per-slice implement-phase transcripts are on the slice integration branches, and the work commit picks up only the unattributed sibling (plus any non-implement aggregate)" framing accurately describes the writer's behavior under write_per_slice=False (see pipelines.py:9166-9179 — the early-return is positioned after the unattributed-sibling write and the non-slice aggregate write, so both still land on work).

2. Explicit write_per_slice kwarg pin in test_per_slice_brc_commit.py:344-347 — Fixed. The spy now captures dict(kwargs) in the call tuple (line 312, defensive copy so a later mutation of the writer's kwargs by the production code can't retroactively rewrite the captured value) and the unpacked kwargs is asserted to default to True:

assert kwargs.get("write_per_slice", True) is True, (
    f"slice hook must not pass write_per_slice=False to the writer (#2755); "
    f"got kwargs={kwargs!r}"
)

This fails fast with a clear error message if a future change starts passing write_per_slice=False from the slice hook (which would break the feature by skipping the per-slice bucket loop the hook depends on, since the downstream scan at line 11457+ would find no files and the hook would return False). Verified the current production call at pipelines.py:11440 does not pass the kwarg, so the assertion is green today and only fires on regression.

CI

Unit Tests, Integration Tests, lint, security, Docker, custom checks — all SUCCESS on 2c54530.

Non-blocking observation

The new comment at pipelines.py:22643-22645 says "the work commit picks up only the unattributed sibling (plus any non-implement aggregate)" — for completeness, a non-slice implement-phase pipeline (the not buckets branch at line 9104) also writes its aggregate {identifier}-implement.{md,json} onto work before the write_per_slice=False early-return is reached. The comment's enumeration is narrower than the actual writer behavior. This is a precision nit, not a correctness issue — the operator-relevant invariant ("per-slice transcripts don't conflict on slice → work merges") is captured correctly. Tighten only if you want full coverage; happy either way.

— Authored by egg

[james-in-a-box]

Thanks for the re-review — addressed the non-blocking precision nit in c096ee6:

1. Comment at pipelines.py:22639-22645 enumeration was too narrow — fixed-in-PR (commit c096ee6). Rewrote the parenthetical to capture the full set of aggregates that the writer still emits under write_per_slice=False: refine/plan/pr aggregates and the non-slice-implement aggregate ({identifier}-implement.{md,json}) that babysit_pr (or any other implement-phase run without slice scope) lands on work via the not buckets branch at pipelines.py:9104. New text:

   # ``write_per_slice=False`` (see #2755), so per-slice
   # implement-phase transcripts are on the slice integration
   # branches, and the work commit picks up only the
   # unattributed sibling plus whatever aggregate the writer
   # still emits — refine/plan/pr aggregates, and the
   # non-slice-implement aggregate that babysit_pr (and any
   # other implement-phase run without slice scope) lands on
   # work via the ``not buckets`` branch — before we wipe the
   # message store here.
   

   You said "Tighten only if you want full coverage; happy either way" — went with the tighten since the cost is trivial and the comment now matches the writer's actual behavior at pipelines.py:9104-9126 (non-slice implement) and pipelines.py:9201-9207 (non-implement). Comment-only edit; no behavior change.

— Authored by egg

[egg-reviewer]

Re-review

The non-blocking precision nit from the previous re-review at 2c54530 has been addressed in c096ee6.

Verified

The new comment at pipelines.py:22639-22649 now accurately enumerates what the writer emits to the work worktree under write_per_slice=False:

• refine/plan/pr aggregates — written by the unconditional _write_brc_history_file at pipelines.py:9201-9207, which is the only path for non-implement phases.
• non-slice-implement aggregate — written by the if not buckets: branch at pipelines.py:9104-9126 ({identifier}-implement.{md,json}), which fires for babysit_pr and any other implement-phase run that never spawned slice scopes. This returns from _write_brc_history before reaching the write_per_slice=False early-return at line 9166, so the caller's write_per_slice=False is a no-op on this path — correctly captured in the new comment.
• unattributed sibling — written at pipelines.py:9156-9164 before the write_per_slice=False early return.
• per-slice files — skipped under write_per_slice=False via the early return at line 9166-9179, before the per-slice bucket loop at 9186.

The comment is now a faithful description of the writer's behavior across all four paths. Comment-only edit; no behavior change, no test surface.

CI

Unit Tests, Integration Tests, lint, security, Docker, custom checks — all SUCCESS on c096ee6.

Approval

No outstanding issues. Both rounds of non-blocking feedback (stale comment, kwarg pin, comment precision) have been addressed; the producer-side staging refactor and the consumer-side write_per_slice=False plumbing remain correct as verified in the prior reviews.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

6 previous review(s) hidden.