docs: clarify slice-1 context-PR base and restore #2548 cross-ref - #3325
Conversation
…oc-updater] Update two broken anchor links that were missed when the de-ledgering PR renamed orchestrator.md sections: - #wrapper-side-heartbeat-2036-migration-completed-in-slice-4 → #heartbeat-single-foreground-ping-per-event - #wrapper-side-gateway-session-keep-alive-2451-migration-completed-in-slice-4 → #gateway-session-keep-alive Triggered by: #3319 Authored-by: egg
This comment has been minimized.
This comment has been minimized.
Autofix tracking{"Lint/Python": 1, "Test/Unit Tests": 1} |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Review: PR #3325 — fix stale orchestrator.md anchors
Verdict: Approve. The documentation fix is correct and complete. One non-blocking scope note below.
Verified ✅
The anchor fix is correct. Both replacement anchors resolve to live headings in docs/architecture/orchestrator.md, per GitHub's slug rules (lowercase, drop parentheses, spaces→hyphens):
#heartbeat-single-foreground-ping-per-event→### Heartbeat (single foreground ping per event)(orchestrator.md:841) ✅#gateway-session-keep-alive→### Gateway-session keep-alive(orchestrator.md:873) ✅
The two old anchors (#wrapper-side-heartbeat-2036-…, #wrapper-side-gateway-session-keep-alive-2451-…) no longer exist in orchestrator.md — confirming this fixes a genuine break introduced by the #3319 rename, not a phantom one.
The fix is complete. I checked every orchestrator.md#… reference in concurrent-execution.md — all six resolve (brc-consensus-wrapper, the two fixed anchors, wake-conditions-pre-confirm-vs-post-confirm ×2, api-endpoints). No other stale wrapper-side- / migration-completed-in-slice- anchors remain in the file. Nothing was missed.
Non-blocking: undocumented out-of-scope change
orchestrator/routes/pipelines.py (lines ~14798–14804) is included in this diff but not mentioned in the PR description, which describes only the anchor-link fixes. The change is a pure quote-style swap inside _build_agent_prompt — escaped double quotes in double-quoted strings converted to single-quoted strings:
-"- Include historical context (issue links, \"previously X\" rationale, "
+'- Include historical context (issue links, "previously X" rationale, 'I verified the resulting string content is byte-identical before and after, so there is no behavior change — this is almost certainly a stray ruff quote-style (Q) autofix that rode along. It's harmless, but it's unrelated to a docs-anchor fix and undocumented, which works against the PR-hygiene goal of keeping unrelated concerns in separate PRs. Either drop it from this PR, or add a one-line note to the description acknowledging the incidental lint fix. Not blocking.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…-230237-3123/work # Conflicts: # docs/guides/concurrent-execution.md
Conflict Resolution SummaryResolved merge conflicts with
Detail: This PR fixed two stale anchor links on the list item describing the wrapper's "two background subshells" (a heartbeat emitter + gateway-session keep-alive). Between this PR and now, base #3319/#3288 rewrote that entire section — converting the numbered list to a bullet list and deleting the background-subshell item altogether, consistent with the surrounding narrative which now states there is "no background heartbeat subshell." Because the lines this PR patched no longer exist on Verification:
Please review: This PR's stated purpose — fixing two anchors — is now fully superseded by main's section rewrite. The merge keeps the PR alive but the diff vs main is now empty for the docs change. Consider whether this PR is still needed, or close it as obsolete. — Authored by egg |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review: PR #3325
Verdict: Approve (content correct) — but the title/description no longer match the diff.
I re-reviewed against the net diff vs main (gh pr diff 3325), since the branch was rebased and my prior review commit 5d63585 is no longer reachable.
What actually changed
The original anchor fixes this PR was opened for are now net no-ops. Base #3319/#3288 (merged in via b299cd8) rewrote the entire "How it works" section in concurrent-execution.md, deleting the background-subshell list item that carried the two stale anchors — grep for both the old (wrapper-side-heartbeat…) and the new (heartbeat-single-foreground-ping-per-event) anchors in the current branch file returns nothing. The conflict-resolution comment already called this out correctly.
So the live diff against main is only the two hunks from commit 0160724, neither of which is an anchor fix:
concurrent-execution.md:1019— clarifies that slice-1's base resolves toegg/<id>/work.orchestrator-cli.md:650— restores the#2548provenance cross-ref.
Verified ✅
Both surviving hunks are technically accurate:
- Slice-1 base — matches the authoritative architecture doc verbatim (
orchestrator.md:358, point 4: "The first slice'sparent_branchresolves toegg/<id>/work(the pipeline work branch is itself the context PR head)") and the code (concurrent_executor.py:385: "Roots base off the pipeline branch directly (egg/issue-N/work)"). This is rationale, not just chronology — it explains the consequence of there being no separate context branch. Good addition. - #2548 cross-ref — consistent with
orchestrator.md:361(the v1.1pr.context_*framing fields originated in #2548; #2777 hardened the mechanism). The[#2548]link is well-formed and the two existing cross-ref anchors (#context-pr-slice-aware-mode-2777,#slice-pr-stack) both resolve to live headings.
No correctness, link-integrity, or security issues in the landing docs.
Non-blocking
-
Stale title and description (please fix before merge). The title ("fix stale orchestrator.md anchors") and the entire PR body — including the Test Plan ("verify the two links resolve") — describe a change that no longer exists in the diff. The PR was repurposed (commit
0160724, "restore #2548 cross-ref and tie slice-1 base…") into two unrelated context-PR doc tweaks after the anchor fix was superseded. A merger reading this PR would be misled about what they're merging. Either update the title/description to describe the actual change, or split these doc tweaks into a fresh PR and close this one as obsolete (the conflict-resolution comment already suggested the latter). -
#2548sentence is chronology-heavy. "The up-front context-PR mechanism originated in #2548 and was hardened … by #2777" is exactly the "when it changed" provenance that this repo's own doc-writing guidance (the very prompt text touched by this PR's formatting commit inpipelines.py:_build_agent_prompt) says to include "ONLY when tangibly valuable to a reader of the current system." It's consistent with the house style of per-section issue links, so it's fine to keep — just flagging that its reader value is marginal.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
| egg is addressing review feedback... |
|
Thanks for the thorough re-review. Per-item disposition:
The first-review note about the — Authored by egg |
|
egg feedback addressed. View run logs 8 previous review(s) hidden. |
Revert the bundled #2548 doc edits (concurrent-execution.md, orchestrator-cli.md) that an autofixer added to chase two doc-test failures pre-existing on main. Those failures are unrelated to #3328 and are owned by the separate, green PR #3325 — keeping the edits here duplicates #3325 and would conflict on whichever lands second. Refresh the ledger baseline so the self-test stays green in both merge orderings: concurrent-execution.md goes 15->16, matching #3325's single added slice-1 token (the context-PR base clarification). With the baseline at the post-#3325 count and this PR's own tree doc-free, net-new is empty whether or not #3325 has landed yet. This PR is now scoped to its three ledger files. It still needs #3325 on main to clear the #2548 doc tests on the merge tree.
Two small context-PR documentation clarifications. (This PR was originally
opened to fix two stale
orchestrator.mdanchors inconcurrent-execution.md, but base #3319/#3288 rewrote that entire sectionand deleted the list item carrying those anchors, making the anchor fix a
net no-op. The PR was repurposed to the two surviving doc tweaks below.)
concurrent-execution.md— clarify that, because there is no separateegg/<id>/contextdoc-only branch, slice-1's base resolves toegg/<id>/work(the context PR head) rather than stacking onto anegg/<id>/contextbranch. Matchesorchestrator.mdandconcurrent_executor.py.orchestrator-cli.md— restore the#2548provenance cross-ref forthe up-front context-PR mechanism (hardened into its current
orchestrator-authored, idempotent form by #2777).
Issue: none
Test Plan
(
#context-pr-slice-aware-mode-2777,#slice-pr-stack) and that theslice-1 base statement matches
orchestrator.mdpoint 4.Authored-by: egg