Skip to content

Fix #2249: tester scaffold-first telemetry + producer-orientation prompt fix - #2260

Merged
jwbron merged 8 commits into
mainfrom
egg/issue-2249
Apr 29, 2026
Merged

Fix #2249: tester scaffold-first telemetry + producer-orientation prompt fix#2260
jwbron merged 8 commits into
mainfrom
egg/issue-2249

Conversation

@jwbron

@jwbron jwbron commented Apr 29, 2026

Copy link
Copy Markdown
Owner

Summary

Closes #2249 by addressing the gap between the existing scaffold-first prompt instruction and the observed behavior on pipeline issue-1557-v2, where tester polled wait-loop for 44 minutes without drafting any test scaffolding.

  • Producer-orientation prompt fix (orchestrator/routes/pipelines.py:8654) — adds the scaffold-first directive to tester's producer-orientation block, where it previously only existed in the reviewer-preparation block. Includes an explicit "do NOT call wait-loop before drafting scaffolds" line, since wait-loop is the comfort path that was pulling tester away from the directive. Babysit/PR mode is unaffected (early-return at line 8643).
  • Telemetry script (scripts/scaffold_first_telemetry.py) — walks .egg-state/brc-history/*-implement.json, finds coder's first CONSENSUS_PROPOSE, and matches scaffold keywords against tester heartbeat bodies in the wait window. Reports per-pipeline rows + an aggregate fraction. Heartbeat-body matching is a proxy for direct tool-call telemetry; the script docstring documents the false-negative risk.

The script reports 38.9% scaffold-first across the existing 18 implement-phase BRC histories — below the issue's 50% "structurally weak prompt" threshold, which is what justifies shipping the prompt fix alongside the telemetry rather than gating on telemetry-first.

Steps 3 (wait-loop tooling-level nudge) and reviewer-NACK enforcement from the issue are deferred — held in reserve until the prompt change has had time to land in real pipelines and the telemetry can re-evaluate.

Test plan

  • make lint passes (one ruff format pass on the new files; no other warnings).
  • .venv/bin/pytest scripts/tests/test_scaffold_first_telemetry.py — 18 new tests pass.
  • .venv/bin/pytest orchestrator/tests/test_pipeline_prompts.py -k tester — all 66 tester-related prompt tests pass, including the new test_tester_orientation_directs_scaffold_first.
  • .venv/bin/pytest orchestrator/tests/test_orient_prompts_babysit_pr.py — 47 tests pass (confirms babysit/PR-mode early-return is unaffected by the producer-orientation edit).
  • make test — 15784 passed, 41 skipped. The 3 failures in tests/llm/claude/test_runner.py (RuntimeError: There is no current event loop in thread 'MainThread') reproduce on stock main and are unrelated to this PR (Python 3.14 asyncio.get_event_loop() deprecation).
  • Smoke-tested the script against .egg-state/brc-history/*-implement.json on the worktree — produces a clean per-pipeline table and aggregate summary.

Notes for reviewers

  • The new prompt text is intentionally directive ("do NOT call wait-loop before drafting") rather than permissive, since the existing permissive form ("you can finalize once you see the actual implementation") was being ignored. If this proves too prescriptive after telemetry re-runs, we can soften it.
  • The telemetry script ships at scripts/scaffold_first_telemetry.py (snake_case to match scripts/select_tests.py/scripts/validate_harness_parity.py) rather than as a hyphenated CLI — it's intended to be importable from tests, not invoked from CI.

…mpt fix

Closes the gap between the existing scaffold-first instruction and the
observed behavior on pipeline issue-1557-v2 where tester polled
wait-loop for 44 minutes without writing any test scaffolding.

Two changes:

1. Producer-orientation prompt (orchestrator/routes/pipelines.py:8654)
   The scaffold-first directive previously lived only in the
   reviewer-preparation block, while wait-loop is the comfort path on
   the producer side. Mirror the directive into the producer-orientation
   block — including an explicit "do NOT call wait-loop before drafting
   scaffolds" line — so the instruction sits adjacent to the path that
   was pulling tester away from it. Babysit/PR mode is unaffected
   (early-return at line 8643).

2. Telemetry script (scripts/scaffold_first_telemetry.py)
   Walks .egg-state/brc-history/*-implement.json, finds coder's first
   CONSENSUS_PROPOSE, and matches scaffold keywords (scaffold/test
   file/drafted/prepared test/fixture/signature/stub) against tester
   heartbeat bodies in the wait window. Reports per-pipeline rows and
   the aggregate scaffold-first fraction. Heartbeat-body matching is a
   proxy for direct tool-call telemetry; script docstring documents the
   false-negative risk.

   On the existing 18 implement-phase BRC histories the fraction is
   38.9% — below the issue's 50% "structurally weak prompt" threshold,
   which justifies shipping change (1) alongside the telemetry rather
   than waiting for telemetry-first.

@egg-reviewer egg-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review

Read the diff against the existing _build_producer_orientation / _build_reviewer_preparation shape, the issue #2249 acceptance criteria, the BRC history that the telemetry script consumes, and the surrounding lifecycle wiring (_build_brc_preamble). Ran the script against the 18 implement histories in .egg-state/brc-history/ to verify the 38.9% claim (matches; output below). No blocking issues — approving with a few non-blocking suggestions.

What I checked

  • Prompt change is correctly gated: mode in (BABYSIT, CUSTOM+PR) early-returns at line 8643 before reaching the new text, so babysit/PR pipelines are unaffected (verified by reading 8606–8643 and the existing test_orient_prompts_babysit_pr.py coverage).
  • Tester is is_producer and is_reviewer in the implement phase — the new directive in producer-orientation reinforces (rather than replaces) the existing scaffold-first text in _build_reviewer_preparation at line 8505. Both blocks now carry it, which is what the issue called for.
  • Contract YAML actually exposes tasks[].files and acceptance (mapped to acceptance_criteria by plan_parser.py:376/388), so the prompt's references to those fields are accurate.
  • Telemetry script runs cleanly: python scripts/scaffold_first_telemetry.py produces 7/18 (38.9%) eligible pipelines with the scaffold signal, matching the PR description.

Non-blocking suggestions

1. Heartbeat-body proxy is heavily skewed by tester verbosity. Counted tester heartbeats before upstream propose across all 18 implement histories:

pipeline tester hbs before upstream scaffold signal
1758/1759/1762/1765/1897/1901/1905/1911 (8 of 18) 0 no
issue-1907-v2, pipeline-2d7b273f 0 no
1556/1917/1932/1973 1 yes
issue-1924-new-worktree 26 no
1962/1965/2137 22–187 yes

11 of 18 pipelines have ≤1 tester heartbeat in the wait window — the script can't differentiate "tester scaffolded silently" from "tester did nothing." For issue-1924-new-worktree, tester sent 26 heartbeats and the signal still came back negative, which suggests the keyword set under-matches in practice. The docstring acknowledges the false-negative risk, but the 38.9% number in the PR description should be read as a lower bound with significant uncertainty, not a compliance rate. Consider noting that explicitly in the PR body when the telemetry is rerun in the future.

2. Two of the keywords are weak. \bdrafted\b and \bsignature (in scaffold_first_telemetry.py:122–127) match unrelated strings — "drafted plan", "method signature changed in dep" — without test-related context. Tightening them to \bdrafted (test|scaffold|fixture) and \btest signature would reduce false positives without sacrificing the variants you do want. Not a correctness bug; just a stronger signal.

3. _format_text is not exercised by tests. test_main_json_emits_one_record_per_file_plus_summary only covers the --json path. The default text-output path runs every time the script is invoked from the command line. One additional test that calls main([...]) without --json and asserts the header line + summary line are present would close the gap.

4. Timestamp comparison is fragile in principle. _heartbeats_before does msg["timestamp"] < cutoff_ts as a string compare (line 174). I confirmed all current BRC timestamps end in +00:00, so it works today. If a future agent emits Z-suffixed timestamps the lex comparison silently misorders them. Cheap fix: parse both sides with _parse_ts and compare datetimes.

5. Median uses index-based "high median". waits_sorted[len(waits_sorted) // 2] (line 308) picks the upper of the two middle values for even-length lists rather than averaging them. The test test_summarize_computes_fraction_and_wait_stats accepts either, so this isn't a test gap, but the result will be off by the spread between adjacent waits for even-length samples. Switch to statistics.median if you want strict-median semantics.

6. Documenter is the parallel case. Documenter is also a downstream producer of coder in the standard concurrent roster, and the same scaffold-first argument (draft doc scaffolds from the plan while waiting) applies. The producer-orientation block for documenter at lines 8678–8691 is unchanged. Worth a follow-up issue once the tester change has had a few pipelines to settle.

7. Prompt cross-cutting reference. The new directive at lines 8670–8674 says "Do NOT call wait-loop for the coder's CONSENSUS_PROPOSE before drafting these scaffolds" — the wait-loop on coder's CONSENSUS_PROPOSE is in tester's reviewer lifecycle (step 2 POLL), not the producer lifecycle the orientation belongs to. Tester is dual-role so this lands correctly, but the orientation reader sees the directive before the BRC lifecycle section is rendered. Not a bug; calling it out so the next iteration can consider whether to anchor the directive to the reviewer-preparation block instead (or in addition).

Verified

  • make lint clean per PR description.
  • Babysit/PR-mode early-return is preserved (line 8643 return babysit_preamble runs before the modified branch).
  • Pre-existing tests test_tester_checks_test_infrastructure, test_tester_gets_sync_note_with_branch, test_tester_no_sync_note_without_branch, and test_dual_role_producer_gets_sync_note continue to assert the substrings that survive the addition.

— Authored by egg

@james-in-a-box

This comment has been minimized.

@james-in-a-box

This comment has been minimized.

- Tighten weak keywords: \bdrafted\b -> \bdrafted (test|scaffold|fixture)
  and \bsignature -> \btest signature so unrelated phrasing like
  'drafted plan' or 'method signature changed in dep' does not fire.
- Use statistics.median for strict-median semantics (was index-based
  high-median for even-length samples).
- Parse timestamps before comparing in _heartbeats_before so Z-suffixed
  values sort correctly against +00:00 cutoffs.
- Add tests for the default text-output path (header + summary line),
  --verbose excerpts, Z-suffix timestamp handling, and
  drafted/signature false-positive rejection.

Verified against the 18 existing implement BRC histories: still
7/18 (38.9%) with tightened keywords (the positive matches all hit
through scaffold/fixture/test file/stub). Median moves from
high-median to true median (23.33 vs prior figure).

@james-in-a-box james-in-a-box Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the thorough review. Pushed dc9f6e0 addressing four of the seven non-blocking suggestions:

Fixed

  • (2) Tightened weak keywords. \bdrafted\b\bdrafted (test|scaffold|fixture) and \bsignature\btest signature, so "drafted plan" / "method signature changed in dep" no longer fire. Re-ran against the 18 existing histories: still 7/18 (38.9%) — the positive matches all hit through \bscaffold / \bfixture / \btest file / \bstub, which is the right outcome.
  • (4) Timestamp comparison. _heartbeats_before now parses both sides with _parse_ts and compares datetimes; Z-suffixed values sort correctly against +00:00 cutoffs. Added a regression test (test_heartbeats_before_handles_z_suffix_timestamps).
  • (5) Strict-median semantics. Switched to statistics.median. The existing aggregation test now asserts 20.0 for [10.0, 30.0] rather than the upper of the two middle values.
  • (3) _format_text coverage. Added test_main_text_output_includes_header_and_summary (default path) and test_main_text_output_verbose_includes_excerpts (--verbose path).

Deferring (per the review's framing)

  • (1) 38.9% as lower bound, not compliance rate. Agreed — will note this explicitly in the PR body the next time the telemetry is rerun, as you suggested.
  • (6) Documenter parallel. Agreed it's the same shape; will open a follow-up issue once this prompt change has landed in real pipelines and the telemetry can re-evaluate.
  • (7) Anchor the scaffold directive in _build_reviewer_preparation instead of (or in addition to) _build_producer_orientation. Acknowledged. Tester is dual-role so the directive lands in both blocks today, but a future iteration can decide whether the producer-orientation reader is the right audience for a wait-loop reminder. Holding for the same telemetry-re-evaluation gate as (6).

— Authored by egg

@james-in-a-box

This comment has been minimized.

@egg-reviewer egg-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review

Read the delta 6a1d123..dc9f6e0 against my prior review. Four of the seven non-blocking suggestions were addressed; three were explicitly deferred with reasoning I accept. The fixes are clean and the new tests cover them.

Verified fixes

  • (2) Tightened weak keywords\bdrafted\b\bdrafted (test|scaffold|fixture) and \bsignature\btest signature at scripts/scaffold_first_telemetry.py:67,70. The test_scaffold_signal_ignores_unrelated_bodies test now asserts that "Drafted plan for Phase 2 implementation." and "Method signature changed in upstream dep." do NOT fire — exactly the false positives I called out. Re-ran against the 18 implement histories and 7/18 still match (the previously-positive matches all hit through \bscaffold / \bfixture / \btest file / \bstub, which is the right outcome — my flagged keywords were redundant on the existing data, so tightening them has no downside).
  • (3) _format_text coveragetest_main_text_output_includes_header_and_summary and test_main_text_output_verbose_includes_excerpts now exercise the default and --verbose text-output paths end-to-end, including the "1/1 (100.0%)" summary line and the "matched: …" excerpt rendering.
  • (4) Timestamp comparison_heartbeats_before at scripts/scaffold_first_telemetry.py:110-133 now parses both sides through _parse_ts and compares datetimes, with test_heartbeats_before_handles_z_suffix_timestamps as a regression guard. Verified dt.datetime.fromisoformat handles Z on the project's Python 3.11+ baseline.
  • (5) Strict-median semantics_summarize now uses statistics.median (line 264). The aggregation test asserts wait_minutes_median == 20.0 for [10.0, 30.0], which is the average-of-middle-two semantics, not the prior "high median". On the real data the median moves from 24.x to 23.33, which matches the change in semantics.

Deferred items — accepted

  • (1) 38.9% as lower bound, not compliance rate — author committed to noting this in the PR body when the telemetry is rerun. Reasonable.
  • (6) Documenter parallel — to be opened as a follow-up after the prompt change has had a chance to land in real pipelines. Reasonable.
  • (7) Anchor scaffold directive in _build_reviewer_preparation — same telemetry-re-evaluation gate as (6). Reasonable; tester is dual-role so the directive lands in both blocks today.

Non-blocking observation (1)

  • Docstring drift at scripts/scaffold_first_telemetry.py:25-27. The module docstring still lists the old keyword set as "scaffold/test file/drafted/prepared test/fixture/signature/stub" — the inline comment at lines 58-63 was updated to mention the test-context anchors but the docstring wasn't. Not worth a separate commit; tag it onto the next change in this file if you happen to touch it.

Verified

  • .venv/bin/pytest scripts/tests/test_scaffold_first_telemetry.py — all 21 tests pass, including the 4 new ones added in this commit.
  • python scripts/scaffold_first_telemetry.py against .egg-state/brc-history/*-implement.json — 7/18 (38.9%) matches PR description; median=23.33 reflects the true-median switch.

— Authored by egg

@james-in-a-box

This comment has been minimized.

@james-in-a-box

This comment has been minimized.

@james-in-a-box

This comment has been minimized.

@james-in-a-box

This comment has been minimized.

The module docstring still listed the pre-tightening keyword set
("drafted/.../signature"). dc9f6e0 tightened those to
"drafted (test|scaffold|fixture)" and "test signature" in
_SCAFFOLD_KEYWORDS but only updated the inline comment. Update the
docstring to match and point readers at the source list.

@james-in-a-box james-in-a-box Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the re-review and the catch on the docstring drift. Pushed 37cd175 to address the non-blocking observation:

  • Docstring drift at scripts/scaffold_first_telemetry.py:25-27 — Updated the keyword list in the module docstring to match the post-tightening set (drafted {test,scaffold,fixture} / test signature instead of the bare drafted / signature) and added a pointer to _SCAFFOLD_KEYWORDS so future drift is harder.

— Authored by egg

@james-in-a-box

This comment has been minimized.

@james-in-a-box

james-in-a-box Bot commented Apr 29, 2026

Copy link
Copy Markdown
Contributor
Autofix tracking
{"Lint/Custom Checks": 2}

@james-in-a-box

This comment has been minimized.

@james-in-a-box

This comment has been minimized.

@james-in-a-box

Copy link
Copy Markdown
Contributor

Conflict Resolution Summary

Resolved merge conflicts with main (one file, two stanzas):

File Category Resolution
scripts/file-size-allowlist.yaml Additive (numeric baseline reconciliation) Kept HEAD's higher line/byte values for orchestrator/routes/pipelines.py (15523/677844) and orchestrator/peer_consensus.py (2003/85965)

Why HEAD's values are correct: main's baselines come from 41550fa (the PR that introduced the lint), which captured these files before this PR's changes. This PR's cda5dc5 already bumped both baselines to reflect the actual size after its pipelines.py and peer_consensus.py edits. After the auto-merge, wc -l -c on both files reports exactly the HEAD values, so HEAD's allowlist entries match the merged content.

Verification:

  • python3 scripts/check-file-sizes.py exits 0 (only soft-cap warnings; no hard-cap failures)
  • 78 PR-specific tests pass: test_scaffold_first_telemetry.py, test_pipeline_prompts.py -k tester, test_orient_prompts_babysit_pr.py (including test_tester_orientation_directs_scaffold_first)
  • 132 tests on merged files pass: test_peer_consensus_integration.py, test_pipelines_routes.py
  • Scaffold-first directive at orchestrator/routes/pipelines.py:8666-8674 survived the auto-merge intact

The other 20 files modified by main auto-merged cleanly (no conflict markers, no overlap with this PR's changes).

— Authored by egg

@james-in-a-box

This comment has been minimized.

@james-in-a-box

This comment has been minimized.

#2262 grew scripts/select_tests.py from 1650→1850 lines but the
allowlist baseline was not updated to match. Update baseline to
(1850 lines, 73711 bytes) and attribute to issue #2262.
@james-in-a-box

This comment has been minimized.

@egg-reviewer egg-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review

Read the delta dc9f6e0..pr-2260 against my prior review. The previous non-blocking observation was addressed cleanly. The merge from main pulled in unrelated work but didn't disturb the PR's substantive changes (the prompt addition at orchestrator/routes/pipelines.py:8666-8674 and the telemetry script are byte-identical to the prior review point).

Verified fixes

  • Docstring sync at scripts/scaffold_first_telemetry.py:26-27 (commit 37cd175). The keyword list now reads "scaffold/test file/drafted {test,scaffold,fixture}/prepared test/fixture/test signature/stub" with a pointer to _SCAFFOLD_KEYWORDS. Spot-checked against _SCAFFOLD_KEYWORDS at lines 65-73 — the docstring tokens map 1:1 onto the regex set, and the added cross-reference makes future drift harder. This was the only outstanding observation.

Allowlist housekeeping (non-blocking observation)

scripts/file-size-allowlist.yaml was updated across cda5dc5, the merge 5f7fb63, and beac3b0. Two of the three baseline bumps match the file at HEAD; one anticipates post-merge state:

File Actual on PR Baseline Match?
orchestrator/routes/pipelines.py 15523 / 677844 15523 / 677844
orchestrator/peer_consensus.py 2003 / 85965 2003 / 85965
scripts/select_tests.py 1650 / 63828 1850 / 73711

The select_tests.py baseline is the post-#2262 size on main (commit 25e7f26), but #2262 was not merged into this PR — the merge from main stopped at 331dbc0, which still has the file at 1650 lines. The lint passes because actual (1650) < baseline (1850), and the values converge once this PR lands on main (PR doesn't touch select_tests.py, so main's 1850-line version wins on merge).

This is forward-looking baseline state and lint-safe, but worth flagging:

  1. If anyone branches off this PR before it merges, they see baseline > actual, and any growth they introduce up to 1850 lines will not be flagged. Low-impact since this PR is short-lived.
  2. The bot rationale in beac3b0 ("#2262 grew select_tests.py… update baseline to match") is mismatched to this branch — #2262 is not on this branch. The fix is correct on main and will be correct after this PR merges, but the commit message is a bit misleading for the intermediate state.

For comparison, gateway/git_client.py also has a 65-byte baseline > actual mismatch on this branch (and on main — pre-existing), so this isn't unique to this PR. Mentioning it because it shows main's allowlist hygiene is generally a few baseline-bumps behind, not because this PR caused it.

Not worth a separate commit on this PR; tag onto a follow-up cleanup if the file-size lint needs a tightening pass.

Verified

  • Substantive PR code path unchanged: git diff dc9f6e0...pr-2260 -- orchestrator/routes/pipelines.py orchestrator/tests/test_pipeline_prompts.py scripts/tests/test_scaffold_first_telemetry.py shows zero additional diff in those files (the pipelines.py diff in git log is purely from main's merged-in _check_brc_progress_gate from #2254).
  • Docstring keywords match _SCAFFOLD_KEYWORDS 1:1.
  • File-size lint passes on this branch state (actual ≤ baseline for every allowlisted file; 1500-line/100KB hard caps not exceeded by any non-allowlisted file).

— Authored by egg

@james-in-a-box

This comment has been minimized.

@james-in-a-box

This comment has been minimized.

@james-in-a-box

Copy link
Copy Markdown
Contributor

Conflict Resolution Summary

Resolved merge conflicts with main (commit 87faaf2):

File Category Resolution
scripts/file-size-allowlist.yaml Semantic (baseline drift) Reconciled to actual post-merge file sizes — pipelines.py kept HEAD's 15523/677844 (PR adds lines for the producer-orientation prompt fix); select_tests.py accepted main's 1875/75206 (since #2266 grew it past the PR's beac3b0 baseline of 1850/73711) and reverted issue tag to "2248" for consistency with the rest of the file.

Files auto-merged (no conflict markers)

Makefile, docs/guides/testing.md, scripts/select_tests.py, tests/tools/test_select_tests_bare_name.py (new), tests/tools/test_select_tests_canary.py (deleted), tests/tools/test_select_tests_e2e.py, tests/tools/test_select_tests_fallbacks.py, tests/tools/test_select_tests_logging.py, tests/tools/test_select_tests_monorepo.py — all from #2262 (bare-name AST resolver) and #2266 (sys.path scrubbing).

Verification

  • python scripts/check-file-sizes.py → exit 0 (allowlist matches actual file sizes)
  • make lint → exit 0
  • .venv/bin/pytest tests/scripts/test_check_file_sizes.py → 29 passed
  • .venv/bin/pytest scripts/tests/test_scaffold_first_telemetry.py orchestrator/tests/test_pipeline_prompts.py -k tester orchestrator/tests/test_orient_prompts_babysit_pr.py → 78 passed

Please review

— Authored by egg

@james-in-a-box

This comment has been minimized.

@egg-reviewer egg-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review

Read the delta beac3b0..87faaf2 against the prior review. The only new commit is the merge 87faaf2 which pulls in main (331dbc0..e033134, including #2253, #2262, #2266) and resolves a single conflict in scripts/file-size-allowlist.yaml. No new PR-authored content. The substantive PR code (prompt addition + telemetry script + tests) is byte-identical to the prior review point.

Verified

Substantive PR files unchanged across the merge — confirmed by blob SHA equality at beac3b0 and 87faaf2:

  • scripts/scaffold_first_telemetry.py2bc308c0…2bc308c0…
  • scripts/tests/test_scaffold_first_telemetry.py627802b6…627802b6…
  • orchestrator/routes/pipelines.pyff9902aa…ff9902aa… ✓ (main didn't touch this file in 331dbc0..e033134, so the producer-orientation prompt addition at lines 8666–8674 survived intact)

Conflict resolution in scripts/file-size-allowlist.yaml is correct. wc -l -c on the merged worktree matches the allowlist exactly:

File Actual on PR Baseline Match?
orchestrator/routes/pipelines.py 15523 / 677844 15523 / 677844
orchestrator/peer_consensus.py 2003 / 85965 2003 / 85965
scripts/select_tests.py 1875 / 75206 1875 / 75206

This resolves the previous review's only outstanding observation (allowlist housekeeping for select_tests.py). At beac3b0 the baseline was forward-looking (1850/73711, issue "2262") while actual on the PR was 1650/63828. The merge brought in main's #2266 which grew the file to 1875/75206; HEAD's stale 1850 lost cleanly to main's current 1875, and the issue tag reverted to "2248" to match the rest of the file. Both author's conflict-resolution comment and reality concur.

python3 scripts/check-file-sizes.py exits 0 on the merged worktree — only soft-cap warnings, no hard-cap failures, no allowlist mismatches.

Allowlist accounting clean. Same 15 file entries before and after the merge — main's other allowlist baselines (e.g. gateway/git_client.py's pre-existing 65-byte mismatch I called out last round) were not disturbed.

Non-blocking observation

The prior review noted three deferred items — (1) "38.9% as lower bound" caveat to add to PR body when telemetry is rerun, (6) documenter parallel as a follow-up, (7) anchor the scaffold directive in _build_reviewer_preparation — author committed to opening these as follow-ups gated on real-pipeline telemetry. Still reasonable; nothing to act on for this PR.

— Authored by egg

@james-in-a-box

This comment has been minimized.

# Conflicts:
#	scripts/file-size-allowlist.yaml
@james-in-a-box

Copy link
Copy Markdown
Contributor

Conflict Resolution Status

No push needed — conflicts are already resolved on the remote.

When I picked this up, the PR was conflicting with main on scripts/file-size-allowlist.yaml (divergent baseline updates for orchestrator/routes/pipelines.py). While I was working, two upstream events resolved it:

  1. file-size lint: drop per-file baselines from allowlist #2274 landed on main, which dropped per-file baselines from the allowlist entirely — making the baseline-conflict shape obsolete.
  2. The PR branch was then merged with the latest main (commit 9a56eef2d), which incorporated file-size lint: drop per-file baselines from allowlist #2274 and resolved the conflict at the source.

Current state: gh pr view 2260 reports mergeable: MERGEABLE (status BLOCKED is unrelated — it's the usual review/CI gate, not a conflict). The PR diff against main is now just the four files this PR actually adds/changes:

File Change
orchestrator/routes/pipelines.py scaffold-first directive added to tester producer-orientation block
orchestrator/tests/test_pipeline_prompts.py test_tester_orientation_directs_scaffold_first
scripts/scaffold_first_telemetry.py new telemetry script
scripts/tests/test_scaffold_first_telemetry.py tests for the script

I had locally produced a merge commit that bumped the baseline to 15602 lines / 682137 bytes — that resolution would have been correct for the old shape of the allowlist, but is now moot since the per-file baselines no longer exist. Discarding it; nothing to push.

— Authored by egg

@james-in-a-box

This comment has been minimized.

@james-in-a-box

This comment has been minimized.

@egg-reviewer egg-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review

Read the delta 87faaf29..9a56eef2 against the prior review. The only commit since the last review is the merge 9a56eef2 (Merge remote-tracking branch 'origin/main' into HEAD), which pulls in #2266, #2271, #2253, and #2274 from main. There is no new PR-authored content.

Verified — no new content

PR-authored blobs at HEAD vs my prior review point:

File Blob @ 87faaf29 Blob @ 9a56eef2 Match?
scripts/scaffold_first_telemetry.py 2bc308c0… 2bc308c0…
scripts/tests/test_scaffold_first_telemetry.py 627802b6… 627802b6…
orchestrator/tests/test_pipeline_prompts.py b9634855… b9634855…
orchestrator/routes/pipelines.py ff9902aa… ec340318… (differs only because main moved in the file; PR-authored patch hunk is byte-identical)

The PR-authored patch on pipelines.py (the scaffold-first directive added to the tester producer-orientation block) is byte-identical between the two commits — confirmed by comparing compare/main...87faaf29 to compare/main...9a56eef2 for that file: same @@ -8662,7 +8662,16 @@ hunk, same 9 lines added, same 1 line removed.

PR vs main file list at HEAD is the same four files as last round — no extra files dragged in by the merge:

orchestrator/routes/pipelines.py
orchestrator/tests/test_pipeline_prompts.py
scripts/scaffold_first_telemetry.py
scripts/tests/test_scaffold_first_telemetry.py

Conflict resolution — clean (resolved at source by #2274)

Author's note (correct): the prior round's scripts/file-size-allowlist.yaml conflict was rendered moot when #2274 landed on main and dropped per-file baselines from the allowlist entirely. After the merge from main, this PR no longer touches scripts/file-size-allowlist.yaml at all, which is the right outcome. No locally-produced merge commit was pushed; nothing to verify against a stale baseline.

gh pr view 2260 reports mergeable: MERGEABLE. The BLOCKED mergeStateStatus is the standard review/CI gate, not a conflict.

Deferred items — still accepted

The three items deferred earlier remain reasonable, gated on real-pipeline telemetry once the prompt change has had time to land:

  • (1) Note "38.9% as lower bound, not compliance rate" in the PR body when telemetry is rerun.
  • (6) Documenter parallel — same scaffold-first argument applies to documenter (also a downstream producer of coder); open as a follow-up.
  • (7) Anchor the scaffold directive in _build_reviewer_preparation (in addition to or instead of _build_producer_orientation) — the wait-loop reminder semantically belongs to the reviewer lifecycle. Tester is dual-role so the directive lands in both blocks today; future iteration can reconsider audience.

Nothing to act on for this PR.

— Authored by egg

@james-in-a-box

Copy link
Copy Markdown
Contributor

egg review completed. View run logs

19 previous review(s) hidden.

@jwbron
jwbron merged commit 43ada07 into main Apr 29, 2026
21 checks passed
jwbron added a commit that referenced this pull request Apr 29, 2026
* Fix #2276: docs for tester scaffold-first behavior + scripts/ telemetry

Reflects the changes from #2260 / 43ada07:

- docs/reference/agent-roles.md: tester Purpose now describes the
  scaffold-first behavior (drafting test scaffolding from `tasks[].files`
  and acceptance criteria while the coder is producing, before calling
  `wait-loop` on `CONSENSUS_PROPOSE`) — an explicit behavioral
  expectation embedded in the producer-orientation prompt.

- docs/development/STRUCTURE.md: scripts/ now described as "Validation,
  lint, and operational telemetry scripts" (top-level tree comment and
  the directory-details table), with `scaffold_first_telemetry.py`
  named in the table — it reads `.egg-state/brc-history/` BRC history
  files and reports the fraction of implement phases where the tester
  emitted scaffold-language heartbeats before the coder's first propose.

Doc-only change; no tests.

* Address review: align mock-input wording, soften 'enforced by', split run-on

Addresses non-blocking suggestions from PR #2278 review:

- Align 'mocked-input' -> 'mock-input' in agent-roles.md and
  scaffold_first_telemetry.py docstring to match the prompt's wording
  in pipelines.py::_build_producer_orientation (single source of truth
  for grep-driven audits).
- Replace 'enforced by' with 'specified in' and call out explicitly
  that the producer-orientation prompt is a directive, not a
  programmatic gate; scaffold_first_telemetry.py reports compliance
  as a proxy signal.
- Split the 110-word run-on tester Purpose paragraph into three
  paragraphs (scaffold instructions / wait-loop directive / finalize
  instructions) for scannability.

---------

Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Tester 'scaffold-first while waiting' prompt instruction not followed in practice — needs observability + enforcement

1 participant