[issue-3064][slice-1/6] Ownership flag + one-shot wrapper arm... - #3167
Conversation
…spawning Refine analysis at canonical .egg-state/drafts/3064-analysis.md: current spawn-up-front + in-pod event-pump lifecycle (verified file:line), the already-landed foundation (#2908), the #3023 hard constraint (guard+spawner land together), design questions for plan, and scope options A/B/C (HITL cq-1, recommended B) plus failure-supervision policy (HITL cq-2). Rebuilds refiner BRC memory for the correct pipeline (prior memory was stale issue-3077 content).
…gations Replaces the misnumbered issue-3077 draft committed from stale BRC memory (24dddb9) with the canonical risk-analyst-output artifact for this pipeline at the path the phase gate reads. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ce plan Option B per HITL: event loop + flag (default pod) + dedupe, one-shot pod mode with worktree re-attach and per-spawn sessions, cq-2 bounded respawn supervision, lifecycle-owner-aware monitors, docs + flip-follow-up window. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…n-demand one-shot agent pods (Option B, flag-gated) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nto egg/issue-3064-task_planner/work
…re-attach contract Addresses risk_analyst NACK (sole blocker): re-attach validation now includes an explicit dirty-state policy — discard-on-attach (reset --hard + clean -fd, hard-sync to role branch tip) so a predecessor pod killed mid-event can never leak uncommitted residue into a successor's proposal; discard failure falls back to recreate. ac-4 gains an induced dirty-worktree case alongside corruption/branch-mismatch; r-6 mitigation names the policy. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nto egg/issue-3064-task_planner/work
…R6 dirty-state) 13 tasks across the architect's serialized chain: flag + dormant one-shot wrapper arm (golden-file pinned), event loop + spawner with sha256 dedupe and stateless restart, cq-2 supervision via shared supervision_policy constants, worktree re-attach with R6 discard+hard-sync policy and session reuse, lifecycle-aware monitors, docs + flip follow-up package. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…per arm (#3064 slice-1) Adds the ownership flag and the guard that slices 2+ build on, with the in-pod loop byte-identical by default (the #3023 deadlock guard). env_config.py: - get_event_loop_owner() accessor: pod (default) | orchestrator, case-insensitive. Unlike the other knobs, an invalid value is rejected LOUDLY (ValueError) — there is no safe silent fallback for an ownership mode (#3023: a wrong mode deadlocks BRC or duplicates pods). consensus_wrapper.py: - One-shot arm spliced ahead of the main loop ONLY when EGG_EVENT_LOOP_OWNER=orchestrator. Pod mode returns the template untouched (byte-identical to main; splice, not a format placeholder, so empty pod-mode leaves no residue). - Arm engages only with an injected event (EGG_EVENT_ACTION in propose|ack|nack); skips the wait-loop and background heartbeat; re-checks next-action once (stale ⇒ exit 0, no agent); else runs invoke_agent_for_event exactly once and exits with the #2908-classified rc. confirm/complete injected ⇒ loud non-zero rejection (exit 64). - Dormant by design: nothing sets EGG_EVENT_ACTION until slice-2. Verified: pod-mode output byte-identical to HEAD (opus + sonnet/effort); orchestrator-mode arm spliced before a single main-loop marker; generated bash passes `bash -n`; invalid flag raises; ruff clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…t arm + owner accessor task-1-2 (test-first). Pins the slice-1 contract the coder's parallel task-1-1 implementation must satisfy: * Golden snapshot (tests/golden/event_pump_wrapper_pod_default.sh) of the pod-default wrapper rendering — the R1 guard fails on ANY byte-drift of the in-pod event-pump path (EGG_EVENT_LOOP_OWNER unset/pod), so the #3023-style silent-deadlock regression can never land unnoticed. * env_config.get_event_loop_owner() accessor matrix: default pod, pod/orchestrator, trim+case-insensitivity, blank→pod, and invalid value rejected loudly (warn + fall back to pod) per the module convention. * One-shot arm structure (owner=orchestrator): differs from pod default, omits the blocking wait-loop + background heartbeat, reads the injected EGG_EVENT_ACTION, re-checks brc next-action once, reuses invoke_agent_for_event. * One-shot arm behavior via PATH stubs: stale event → exit 0 with no agent invocation (dedupe backstop); fresh event → exactly one invocation; agent exit code passed through (#2908 classification); injected confirm/complete rejected loudly (non-zero, no agent, no consensus confirmed). Golden + pod-default tests pass now; the owner-accessor and one-shot arm tests go green once the coder's slice-1 implementation integrates. Existing wrapper tests are untouched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…/issue-3064-slice-1-tester/work
…ract Merged the coder's slice-1 implementation and ran the full task-1-2 suite against it; 21/21 green. Adjusted three over-specified pins to the implemented (and plan-correct) contract: * Invalid EGG_EVENT_LOOP_OWNER is rejected loudly by RAISING ValueError (the deliberate exception to the module's never-raise convention — #3023: an ownership-mode typo must not silently default), not warn+fallback. Assert the message names the var and the bad value. * "Absence of wait-loop / background-heartbeat in the one-shot PATH" (task-1-2 wording) is now scoped to the spliced arm segment rather than the whole script: the coder reuses the shared helpers and keeps the in-pod loop as the dormant no-event fall-through, so the helper DEFINITIONS legitimately remain — the arm's execution path just never reaches them. Also assert the arm is spliced before the main loop. The five behavioral PATH-stub tests (stale-exit, one-invocation, exit- code passthrough, confirm/complete loud reject) were unchanged and passed against the real arm. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
Autofix tracking{"Lint/Python": 1, "Lint/Shell": 3, "Test/Unit Tests": 2} |
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.
This comment has been minimized.
This comment has been minimized.
Review feedback addressed — commit
|
|
egg feedback addressed. View run logs 25 previous review(s) hidden. |
![james-in-a-box[bot]](https://avatars.githubusercontent.com/u/8340608?s=60&v=4){kind=small}
There was a problem hiding this comment.
Contract Verification — Re-review (slice-1)
Verdict: Approve. No contract violations; all slice-1 acceptance criteria objectively met.
Scope of this re-review
The last-reviewed commit bda2fa5c is no longer reachable — the PR branch was rebased/squashed onto the now-absorbed base egg/issue-3064/work, leaving a single PR-authored commit 2e589496 ("one-shot arm: reserve exit 75/EX_TEMPFAIL"). I therefore re-verified the full slice-1 delta (orchestrator/env_config.py, orchestrator/consensus_wrapper.py, the golden fixture, and test_consensus_wrapper.py), with focus on the exit-75 change flagged by the commit message.
task-1-1 — ownership flag + dormant one-shot arm
env_config.get_event_loop_owner(): defaultpodwhen unset/blank, case-insensitive + trimmed, loudValueErroron an unrecognised value (deliberate departure from the warn-and-default convention, justified in-comment by the #3023 deadlock post-mortem). ✓consensus_wrapper.py: the one-shot arm is spliced only whenEGG_EVENT_LOOP_OWNER=orchestrator(str.replace(_MAIN_LOOP_MARKER, …, 1)); pod/default mode returns the template untouched. ✓- One-shot behaviors all present:
confirm/completeand unknown actions → exit 64 (loud reject, never invoke an agent); confirmed-stale (derived action moved on) → exit 0, no invoke; fresh event → exactly oneinvoke_agent_for_event, exit with the #2908-classified agent rc; single foreground heartbeat, no background emitter, no wait-loop. ✓
Exit-75/EX_TEMPFAIL reservation (the delta)
This adds a third, distinct outcome to the freshness re-check: when egg-orch brc next-action returns non-zero (409 / 5xx / transport), fetch_next_action falls back to {"action":"wait"} but propagates the underlying rc (return "$rc"), and the arm exits 75 (EX_TEMPFAIL) without invoking rather than reporting a clean handoff (exit 0). This correctly prevents a transient blip from silently dropping a live event, and the in-code comment reserves 75 for the slice-3 supervisor contract.
- Correctness confirmed: the path depends on
fetch_next_actionnot swallowing the rc — verified it returns the original rc. - No regression: the exit-75 branch precedes the confirmed-stale check, so genuinely-stale events still exit 0 and fresh events still invoke exactly once — previously-verified behaviors are intact.
task-1-2 — tests
- Golden-file byte-equality test pins the pod-default rendering, with
test_golden_snapshot_is_the_in_pod_event_pumpguarding against a vacuous/empty golden. ✓ - All one-shot behaviors covered behaviourally (rendered bash run against PATH stubs), including the new
test_inconclusive_recheck_exits_ex_tempfail_without_invoking(rc=22 → exit 75, zero invocations). ✓ - Accessor default/blank/case/invalid all tested. ✓
Notes for the human reviewer
- CI on HEAD is green (Unit Tests, Python, Shell, Security, Integration all SUCCESS).
- The orchestrator was unreachable during this run, so I could not write
verify-criterionstate; the contract snapshot also carries no top-levelac-Nentries (criteria live per-task and are satisfied). This is a tooling-availability note, not a compliance gap. - The two large
.egg-state/brc-history/3064-*.{json,md}files in the diff follow existing repo convention (177 brc-history files already tracked on base) — not an accidental inclusion.
— Authored by egg
|
egg contract-verification completed. View run logs 25 previous review(s) hidden. |
There was a problem hiding this comment.
Re-review — [issue-3064][slice-1/6] Ownership flag + one-shot wrapper arm
Both findings from my prior review are properly fixed, and the exit-75/EX_TEMPFAIL work added since then is well-designed and rigorously tested. Approving. One forward-looking non-blocking note for slice-3.
Prior findings — both resolved (verified, not just superficially)
1. Golden self-seeding / pod rendering not byte-identical to main (was blocking) — FIXED.
You took the preferred route: restored the template opener to r""" + newline + #!/bin/bash (consensus_wrapper.py:129) and renamed the fixture to event_pump_wrapper_pod_default.sh.golden so make lint-shell's *.sh glob no longer lints it (resolving the SC1128 tension that forced the one-byte edit). I re-rendered and confirmed against the live code:
pod-default render == golden : True
golden starts with : b'\n#!/bin/bash' # main's bytes, leading newline restored
golden has in-pod markers : True # while true / wait-loop / start_background_heartbeat
golden contains one-shot arm : False # dormant arm does not leak into the pod path
The R1 guard now pins main's actual rendering, and test_golden_snapshot_is_the_in_pod_event_pump guards against a vacuous/empty golden. This is the genuine byte-identity guarantee the slice promised.
2. Test docstrings contradicted the implementation on invalid-value handling (was should-fix) — FIXED.
The slice-1 contract comment (test_consensus_wrapper.py:2164-2169) and the TestEventLoopOwnerAccessor docstring (2257-2265) now correctly state that an unrecognised value raises ValueError (the deliberate #3023 exception), distinguishing it from unset/blank/whitespace → pod. Consistent with env_config.get_event_loop_owner and test_invalid_value_rejected_loudly.
New work since last review — exit-75/EX_TEMPFAIL contract
The three-way re-check disposition in the one-shot arm is correct and addresses a real data-loss risk:
- inconclusive (
fetch_next_actionrc != 0 — 409/5xx/transport) →exit 75, no invocation. Without this, a transient blip would collapse to the{"action":"wait"}fallback and be reported as a clean handoff, silently dropping a live event. - confirmed-stale (rc 0, derived != injected) →
exit 0, no invocation. - fresh (rc 0, derived == injected) → invoke once, pass through agent rc.
fetch_next_action correctly propagates the original rc (consensus_wrapper.py:341), and the arm captures it via $? after command substitution (1022-1025) — sound.
The behavioral test test_inconclusive_recheck_exits_ex_tempfail_without_invoking (2523) is the right kind of test: it returns {"action":"propose"} (matching the injected action) with next_action_rc=22, so an implementation that only checked staleness and ignored the rc would invoke the agent and fail the assertion. It genuinely pins the rc-propagation path, drives the real rendered bash through PATH stubs, and is not self-seeded. The stale/fresh/passthrough/confirm-reject behavioral tests are equally solid.
The latest commit (2e589496) is a pure documentation comment reserving 75 for the slice-3 supervisor — no behavioral change.
Non-blocking (for slice-3)
The reservation comment (consensus_wrapper.py:1071-1077) claims "the #2908 classification codes the agent path emits do not include 75, so the two meanings do not collide today." That holds only as long as python3 -m egg_agent never exits 75 itself — the agent rc is passed through raw at 1078 (exit "$one_shot_rc"), and that rc is not limited to the #2908 classifier codes; it's whatever egg_agent.main() returns. If the agent ever exits 75 (EX_TEMPFAIL is a plausible code for an agent to adopt), the slice-3 supervisor could not distinguish "freshness inconclusive, re-derive" from "agent signalled tempfail." This is not a live bug — the whole one-shot arm is dormant (engages only under EGG_EVENT_LOOP_OWNER=orchestrator, default pod) and slice-3 doesn't exist yet — but slice-3 should back the reservation with an explicit assertion/test that egg_agent's exit-code surface never includes 75, rather than leaving it as a comment-level invariant.
— Authored by egg
|
egg review completed. View run logs 25 previous review(s) hidden. |
| egg is addressing review feedback... |
…ce test The one-shot arm reserves exit 75 (EX_TEMPFAIL) for the inconclusive freshness re-check and passes the agent rc through raw on the fresh path. For the slice-3 supervisor to tell the two apart, egg_agent must never itself emit 75. This was a comment-level invariant; back it with a real assertion (reviewer note, PR #3167): - AST-scan egg_agent.client for every returncode= literal and assert the bounded surface {-1, 0, 1} excludes 75 (anti-vacuity guard included). - Pin that main() propagates result.returncode verbatim (no remap to 75) for the full surface, end-to-end through sys.exit(main()).
Review feedback addressed — commit
|
|
egg feedback addressed. View run logs 25 previous review(s) hidden. |
There was a problem hiding this comment.
Contract re-verification — APPROVE (incremental)
Re-reviewed the delta since my last verification at 2e589496. The PR branch was rebased onto a freshly-recreated base (egg/issue-3064/work), so the prior review SHA is no longer a reachable ancestor; I reconstructed the precise delta from the GitHub commit API.
Delta since last review
A single, test-only commit (889c02d2, +87/−0) touching one file:
tests/shared/egg_agent/test_client.py— addsTestExitCodeSurfaceExcludesExTempfail(two tests).
No production code changed in this delta, so no previously-verified acceptance criterion can have regressed.
Verification of the new work
The new tests back the one-shot arm's exit-75 reservation (a reviewer note I raised on this PR) with real assertions. I confirmed each assertion against the actual production surface:
test_returncode_literals_exclude_ex_tempfail— AST-scansegg_agent.clientforreturncode=literals. Verified the live surface is exactly{-1, 0, 1}(shared/egg_agent/client.py:254,682,708,730,752,773) — 75 is correctly absent. The scan handles bothConstantandUnaryOp(USub)forms (so-1is captured), and the anti-vacuity guard (assert codes) prevents a silent empty-set pass.test_main_propagates_returncode_without_remapping_to_ex_tempfail— pins thatmain()returnsresult.returncodeverbatim. Confirmed atshared/egg_agent/__main__.py:71(return result.returncode) undersys.exit(main())at:75. No remap to 75 anywhere.- The invariant being defended genuinely exists:
orchestrator/consensus_wrapper.py:1041reservesexit 75(EX_TEMPFAIL) for the inconclusive freshness re-check, and:1078passes the agent rc through raw (exit "$one_shot_rc"); the:1072-1078comment already notes the agent codes exclude 75. The tests turn that comment-level invariant into an enforced one.
Ran the two new tests in isolation (targeted, per review policy — not the full suite): 2 passed. Imports resolve (AgentResult from shared/egg_agent/result.py).
Contract status
This is purely additive test hardening that strengthens slice-1's exit-code contract rather than altering it. No contract violations, no orphaned code, no regressions.
Note: the orchestrator was unreachable during this review (
egg-orch health→UNREACHABLE), so I could not writeverify-criterionflags. None are newly satisfiable by this delta — the slice-1 criteria were already verified at the prior review and remain intact, since no production behavior changed.
— Authored by egg
There was a problem hiding this comment.
Re-review — [issue-3064][slice-1/6] exit-75/EX_TEMPFAIL invariant now backed by a real test
The delta since my last review (2e58949..889c02d) is a single test-only commit that addresses the one non-blocking note from my prior approval — and does so correctly. Approving.
Prior note — resolved, properly (not superficially)
My note asked that the exit-75 reservation in the one-shot arm (consensus_wrapper.py:1071-1078) stop resting on a comment-level invariant, since the agent rc is passed through raw (exit "$one_shot_rc") and is not limited to the #2908 classifier codes — it's whatever egg_agent.main() returns. The producer chose to back it now rather than punt to slice-3. Good call; that's the kind of invariant that drifts silently once the arm goes live.
I traced the full rc chain to confirm the new tests target the right thing:
one-shot arm: exit "$one_shot_rc" (consensus_wrapper.py:1078)
← one_shot_rc=$? after invoke_agent_for_event (1062-1063)
← invoke_agent_for_event ends: {agent_command_prefix} "$prompt" (= python3 -m egg_agent) (497)
← sys.exit(main()) (__main__.py)
← main() returns result.returncode verbatim (__main__.py:74)
← AgentResult.returncode set in client.py ({-1, 0, 1})
The two new tests pin both ends of that chain:
test_returncode_literals_exclude_ex_tempfail— AST-scansegg_agent.clientfor everyreturncode=<int>literal (correctly handling the-1UnaryOp(USub, …)case) and asserts75 ∉the set, with an anti-vacuity guard so an empty scan can't pass trivially. I reproduced the scan independently: it returns exactly{-1, 0, 1}and would catch areturncode=75if one were ever added. Non-vacuous and correct.test_main_propagates_returncode_without_remapping_to_ex_tempfail— runs the realmain()with onlyrun_agent/sys.argvmocked (not a bypass fixture), asserting it returnsresult.returncodeverbatim for the full(-1, 0, 1)surface and never invents/remaps to 75. This exercises the production return path.
Together they convert the comment-level invariant into an enforced one: egg_agent's exit-code surface provably excludes 75, so the slice-3 supervisor can keep 75 reserved for "freshness re-check inconclusive" without collision.
Verification
- Both new tests pass; the full file collects (74 tests);
ruff checkclean on the file. - Change is test-only — the one-shot arm and the pod-default golden are untouched, so the byte-identity guarantee from earlier slices is preserved.
- The added
from egg_agent.result import AgentResultimport is used (test 2).
Non-blocking (minor, optional)
The AST scan keys on ast.keyword nodes named returncode, i.e. it catches only keyword literal assignments. It would not catch a positional construction (AgentResult(False, "", "", 75)) or a dynamic value (returncode=some_var). The file's convention is uniformly keyword + literal, so this is purely a forward-drift consideration — the realistic regression (someone typing returncode=75) is caught, and the feature is dormant regardless. Not worth changing for slice-1; just noting the scan is slightly narrower than the prose claim of "provably excludes 75."
— Authored by egg
|
egg contract-verification completed. View run logs 25 previous review(s) hidden. |
|
egg review completed. View run logs 25 previous review(s) hidden. |
| egg is addressing review feedback... |
…rals The AST scan that proves egg_agent's exit-code surface excludes 75 (EX_TEMPFAIL) keyed only on keyword `returncode=` nodes, so a positional construction `AgentResult(False, "", "", 75)` would have slipped past the guard. Extend the scan to also inspect the 4th positional arg of `AgentResult(...)` calls, and factor literal extraction into a shared `_int_literal` helper covering both Constant and UnaryOp(USub) forms. Addresses reviewer note on PR #3167.
Review feedback addressed — commit
|
|
egg feedback addressed. View run logs 25 previous review(s) hidden. |
There was a problem hiding this comment.
Contract re-verification — incremental (since 889c02d)
Verdict: Approve. The delta is clean and introduces no contract violations or regressions.
Delta reviewed
Exactly one commit since my last review (889c02d):
6bc8468— test: harden exit-75 surface scan to catch positional returncode literals- One file:
tests/shared/egg_agent/test_client.py(+54/-16). Test-only — no production code touched.
(Confirmed via gh api .../compare/889c02d...6bc8468: ahead 1, single modified file.)
What changed
In TestExitCodeSurfaceExcludesExTempfail._returncode_literals, the AST scan that proves egg_agent's exit-code surface never emits 75 (EX_TEMPFAIL — the value the one-shot arm reserves for an inconclusive freshness re-check) previously matched only keyword returncode=<int> nodes. The commit extends it to also inspect the 4th positional arg of AgentResult(...) calls, and factors literal extraction into a shared _int_literal helper covering both Constant and UnaryOp(USub) forms.
Verification
- Positional index correctness:
_RETURNCODE_POSITIONAL_INDEX = 3matches theAgentResultdataclass field order (success, stdout, stderr, returncode) atshared/egg_agent/result.py:24-27. The "kept in sync" comment is accurate. - Literal extraction:
_int_literalcorrectly handlesConstant(int)andUnaryOp(USub, Constant)(negative codes like-1); dynamic values returnNone, which the docstring honestly scopes out as inherently unscannable. - Guard remains non-vacuous:
test_returncode_literals_exclude_ex_tempfailasserts the scan set is non-empty before asserting75 ∉ set, so the exclusion can't pass on an empty scan. - Tests pass: ran
tests/shared/egg_agent/test_client.py::TestExitCodeSurfaceExcludesExTempfail→ 2 passed.
Contract impact
- No regression: Slice-1's verified acceptance criteria concern
consensus_wrapper.py(one-shot arm, byte-identical pod-default wrapper),env_config.py(EGG_EVENT_LOOP_OWNERaccessor), andtest_consensus_wrapper.py(golden + one-shot behaviors). None of those files are touched by this delta; all previously verified criteria still hold. - Consistent with intent: The change strengthens a supporting guard that backs task-1-1's "exit with a #2908-classified code" reservation of exit 75. It addresses a prior reviewer note on this PR and is correctly scoped as a test hardening.
- No new tasks were completed since last review; slice-1 (
task-1-1,task-1-2) remains complete.
Note: the orchestrator/egg-contract endpoint was unreachable during this review, and the contract carries no top-level ac-N criteria (acceptance criteria are per-task strings on already-complete tasks), so there were no verify-criterion marks to apply. This verdict is based on direct inspection of the diff, the contract state file, and the targeted test run.
— Authored by egg
There was a problem hiding this comment.
Re-review — commit 6bc8468 (delta since 889c02d)
The only PR-authored change since my last review is the single test commit 6bc8468, which addresses the one non-blocking note from my prior approving review (the AST exit-75 scan caught only keyword returncode= literals, not positional construction). I traced the delta in full and verified it end-to-end.
Verified ✅
- Positional index is correct.
_RETURNCODE_POSITIONAL_INDEX = 3matchesshared/egg_agent/result.py(success=0,stdout=1,stderr=2,returncode=3). Confirmed against the dataclass definition. - The scan exercises the real production path.
_returncode_literalsreadsegg_agent.clientviainspect.getsourceand parses the live module — not a self-seeded golden or a hand-built fixture. The anti-vacuityassert codesguard is intact. - The gap is genuinely closed. I ran the scan logic against the real
client.py(literals{-1, 0, 1}, 75 excluded) and against a synthetic injection ofAgentResult(False, "", "", 75)— the positional 75 is now caught, where the keyword-only scan would have missed it. All current constructions inclient.py(lines 250/678/704/726/748/769) use keyword args, so the positional branch is correctly scoped as a forward-drift guard. - Logic is sound. The
if/elifoverast.walkhandles keyword nodes andAgentResult(...)Call nodes independently with no double-counting; mixed forms (AgentResult(False, "", "", returncode=75)) are still caught via the keyword branch sincelen(node.args) == 3skips the positional path._int_literalcorrectly coversConstantandUnaryOp(USub, Constant). - Syntax +
ruff checkclean on the changed file. Change is test-only; the one-shot arm and pod-default golden are untouched, preserving byte-identity.
Non-blocking observations
_RETURNCODE_POSITIONAL_INDEXis manually kept in sync withresult.pyvia a comment. A reorder of the dataclass fields would silently make the scan read the wrong arg. You could derive it self-validatingly ([f.name for f in dataclasses.fields(AgentResult)].index("returncode")) to remove the coupling — but the field order is stable public API and the comment documents it, so this is optional.- The scan matches only bare-name
AgentResult((ast.Name); an attribute-stylemod.AgentResult(...)would be missed.client.pyimports and uses the bare name, so this is covered today, and the docstring is honest that dynamic values are out of scope. Fine as-is.
Neither is blocking. The change does exactly what it claims and properly resolves my prior note. Approving.
— Authored by egg
|
egg contract-verification completed. View run logs 25 previous review(s) hidden. |
|
egg review completed. View run logs 25 previous review(s) hidden. |
|
Automated feedback loop has reached the maximum of 5 rounds. Human review is needed to make further progress on this PR. |
1 similar comment
|
Automated feedback loop has reached the maximum of 5 rounds. Human review is needed to make further progress on this PR. |
#3202) * docs: document EGG_EVENT_LOOP_OWNER flag (#3064 slice-1) [doc-updater] Update documentation to reflect changes from 17102bc: - Add EGG_EVENT_LOOP_OWNER to orchestrator-cli.md env vars table - Update STRUCTURE.md consensus_wrapper.py entry to note the dormant one-shot arm Triggered by: #3167 Authored-by: egg * docs: note EGG_EVENT_LOOP_OWNER is case-insensitive Address review feedback: the accessor lowercases the raw value (env_config.py:511), so values like Orchestrator/POD are accepted. Make the doc faithful to that behavior. --------- Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>
…r HITL cq-1 Both refine reviewers NACked v1: the analysis (and the issue body itself) falsely claimed 'nothing from #3064 is on main; clean re-run'. Verified against origin/main @74838edb4 that all six #3064 slices are merged (PRs #3167/#3169/#3181/#3192/#3198 + docs), so the full orchestrator-owned on-demand spawning mechanism already exists behind EGG_EVENT_LOOP_OWNER (default 'pod'). - Rewrite current-state to inventory the landed #3064 mechanism as the foundation (event_loop.py, spawn_event_job, JobSupervisor, worktree re-attach, health-monitor orchestrator-mode, ownership flag). - Re-derive the real gap: only the default flip + live proving run remain, and the issue defers those to #3164. - Reframe scope + ACs from greenfield build to adopt/verify/gap-fill. - Register HITL cq-1 for the adopt-vs-reimplement conflict (operator must arbitrate before plan). - Fix v1 nit: build_consensus_wrapped_command is defined at consensus_wrapper.py:1216, not concurrent_executor. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
EGG_EVENT_LOOP_OWNER (pod | orchestrator, default pod) exists; the consensus wrapper gains a one-shot arm driven by injected event env, dormant until slice 2 sets it; pod-default generated wrapper is byte-identical to today, pinned by a golden-file test.
Base PR: #3165
What's in this PR
Commits (4):
This slice
Ownership flag + one-shot wrapper arm (guard, dormant by default)
Files affected:
orchestrator/consensus_wrapper.pyorchestrator/env_config.pyorchestrator/tests/test_consensus_wrapper.pyTasks (2) + acceptance criteria
EGG_EVENT_LOOP_OWNERaccessor toorchestrator/env_config.py(valuespod/orchestrator, defaultpod, loud rejection of invalid values, following the module's existing accessor conventions). Inorchestrator/consensus_wrapper.py, add the one-shot arm to the generated wrapper: when EGG_EVENT_LOOP_OWNER=orchestrator AND an injected event is present (EGG_EVENT_ACTION in propose|ack|nack, EGG_EVENT_DEDUPE_KEY, payload refs), skip the wait-loop (≈379) and the background heartbeat (≈209-230), re-check next-action ONCE viaegg-orch brc next-action— if the derived action no longer matches the injected event (stale), exit 0 WITHOUT invoking the agent — otherwise fetch the composed event prompt via the existing path and runinvoke_agent_for_eventexactly once, exiting with the BRC consensus: replace agent-held waits with deterministic event-pump + durable agent memory (durable fix for #2906) #2908-classified code. confirm/complete must never reach the one-shot arm (reject loudly if injected). With the flag unset orpod, the generated wrapper text is byte-identical to today. Dormant by design: nothing sets the event env until slice 2.pod: generated wrapper byte-identical to main. - One-shot arm: stale event ⇒ exit 0, no agent invocation; fresh event ⇒ exactly one invoke_agent_for_event, then exit with a BRC consensus: replace agent-held waits with deterministic event-pump + durable agent memory (durable fix for #2906) #2908-classified code. - No wait-loop iteration and no background heartbeat process in one-shot mode. - Injected confirm/complete ⇒ loud rejection, non-zero exit. - env_config accessor: default pod; invalid value fails loudly.orchestrator/tests/test_consensus_wrapper.py: a golden-file (or string-equality snapshot) test pinning the pod-default generated wrapper byte-identical to the pre-change rendering; one-shot arm cases — stale-event exit 0 with no invocation, exactly-one-invocation on fresh event, BRC consensus: replace agent-held waits with deterministic event-pump + durable agent memory (durable fix for #2906) #2908 exit-code classification passthrough, absence of wait-loop and background-heartbeat constructs in the one-shot path, loud rejection of injected confirm/complete; env_config accessor default/invalid-value behavior.Stack
issue-3064egg/issue-3064/work