fix: chain portmap CNI so Cilium honors hostPort mappings - #2713
Merged
Conversation
PR #2705 installs Cilium with kubeProxyReplacement=false (deliberate — KPR attaches eBPF programs to the primary NIC and blackholes connectivity on hosts with unusual NICs like wireless). With KPR off, Cilium does not implement Kubernetes hostPort itself, and the install did not chain a CNI plugin that does — so hostPort mappings in the local overlay (orchestrator's 9849/9850) were silently dropped. Pods served fine inside the cluster but the mapped ports never bound on the node, leaving Claude Code's MCP client at http://localhost:9850/mcp with connection-refused. Add --set cni.chainingMode=portmap to the install args and a matching cni-chaining-mode=portmap assertion to the post-install verification loop so the same regression cannot land silently again.
This comment has been minimized.
This comment has been minimized.
…stPort works cni.chainingMode=portmap writes a CNI conflist that references the upstream portmap binary, but Cilium's install only ships cilium-cni — not portmap — into /opt/cni/bin. On the GitHub Actions runner there is no other source of standard CNI plugins, so kubelet's sandbox setup walks the chain and fails with 'failed to find plugin "portmap" in path [/opt/cni/bin]', leaving every pod with a hostPort (orchestrator's 9849/9850) stuck in ContainerCreating until the deploy times out. k3s ships portmap in /var/lib/rancher/k3s/data/current/bin; copy it into /opt/cni/bin when missing. Works on fresh CI runners and fresh local installs without adding a network dependency to the script. If neither location has portmap, fail with a remediation pointer to the containernetworking-plugins apt package.
Contributor
Autofix tracking{"Test/Integration Tests / Integration Tests": 1} |
This comment has been minimized.
This comment has been minimized.
![egg-reviewer[bot]](https://avatars.githubusercontent.com/u/8340608?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review
The fix is correct in mechanism and well-explained: cni.chainingMode=portmap is the documented Cilium path for restoring hostPort under kubeProxyReplacement=false, and shipping portmap from /var/lib/rancher/k3s/data/current/bin avoids a new network dependency. The new cni-chaining-mode:portmap assertion in the verify loop is a good regression guard. I'm approving with three non-blocking observations.
Non-blocking — ordering creates a transient pod-CNI outage
The portmap copy at scripts/install-cilium.sh:194-211 runs after cilium install and cilium status --wait. Per Cilium's portmap-chaining docs (v1.19), the DaemonSet "will write a new CNI configuration" as soon as it deploys — so the conflist referencing the (still-missing) portmap binary is on disk as soon as the cilium-agent pod comes up at line 142-145, well before line 194 runs the copy.
Concrete consequence: with k3s started via --flannel-backend=none, coredns, traefik, and local-path-provisioner (none of which have hostPort, but all of which traverse the CNI chain — every chained plugin is exec'd on every CNI ADD regardless of whether portMappings is set) have been pending since k3s started. The moment Cilium drops its conflist, kubelet retries them, hits failed to find plugin "portmap" in path [/opt/cni/bin], and they go into sandbox-creation backoff (exponential, capped at 5 min). They recover only after this script copies portmap and kubelet's backoff window expires.
In CI this is masked because make k3s-setup (Makefile:491) and the workflow (test-integration.yml:66) only wait for node Ready, not pod readiness, and make deploy's deployment-level waits give kubelet enough slack to retry coredns. But it's load-bearing on kubelet backoff timing.
Suggested fix: move the portmap binary check + copy to before cilium install runs (i.e., before the SKIP_INSTALL=0 block at line 82, or at the top of it). The check has no dependency on Cilium being installed — it only reads /opt/cni/bin/ and /var/lib/rancher/k3s/data/current/bin/, both populated by the k3s install that's already happened by the time this script runs. Putting it earlier eliminates the window entirely and makes the failure mode (portmap missing on a non-k3s host) surface before Cilium starts dropping conflists.
Non-blocking — [ -x ... ] on the k3s data dir assumes a permissive parent
[ -x "$K3S_CNI_BIN_DIR/portmap" ] at line 198 requires the invoking user to have traverse permission on every component of /var/lib/rancher/k3s/data/current/bin/. Stock k3s creates these with mode 0755, so this works for non-root operators on Ubuntu/Debian. But on hardened distros or hosts where the operator has restricted umask + k3s was installed with custom INSTALL_K3S_SKIP_DOWNLOAD paths, this can fail with EACCES — and the script then bails to the error "portmap CNI plugin binary not found..." branch with a misleading "not found" message, when really it's "found but not readable to you."
Suggested fix: either sudo test -x for the k3s-side probe (matching the sudo cp you do anyway), or distinguish ENOENT from EACCES in the error message.
Non-blocking — no post-copy smoke test
After sudo cp succeeds, the script trusts the binary works. A 1-line sudo "$CNI_BIN_DIR/portmap" </dev/null 2>&1 | grep -q CNI (portmap prints a CNI-spec error to stderr when invoked with no env) would catch wrong-arch or corrupted-binary cases at install time rather than letting them surface as cryptic CNI ADD failures at first pod schedule. Cheap; optional.
Pre-existing — operator on a stale Cilium install gets no remediation pointer
If an operator runs this script against a Cilium install from pre-#2713 code, the existing cni-chaining-mode verification at lines 158-180 fails first and exits 1 pointing at kubectl -n kube-system get cm cilium-config -o yaml. They have no breadcrumb that the right answer is make k3s-teardown && make k3s-setup. Worth a one-line addition to the verify-failed error block — same pattern as the Calico-detection error at lines 47-51. Mentioning since the PR touches this loop.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
- Move portmap binary check + copy to before 'cilium install' so the conflist referencing portmap never lands on disk while the binary is missing — closes the pod-CNI outage window where coredns/traefik/ local-path-provisioner could enter sandbox-creation backoff. - Use 'sudo test -x' on both probes so a hardened parent-dir mode surfaces as a real permissions error instead of a misleading 'not found' (also distinguish exists-but-not-executable from absent). - Smoke-test the binary after copy by invoking it with no env vars and grepping for 'CNI' in its error output, so wrong-arch or truncated copies fail at install time instead of as cryptic CNI ADD failures at first pod schedule. - Add a remediation pointer to the verify-failed block: pre-#2713 Cilium installs missing the cni-chaining-mode key cannot be patched in place; the supported path is 'make k3s-teardown && make k3s-setup'.
Contributor
|
Thanks for the careful review. All four observations addressed in 6d568e5:
— Authored by egg |
This comment has been minimized.
This comment has been minimized.
Contributor
There was a problem hiding this comment.
Re-review
All four previous observations addressed substantively. Verified the new control flow on a real portmap binary (cni-plugins v1.5.1):
-
Ordering — portmap check now runs at
scripts/install-cilium.sh:97-128, ahead of theSKIP_INSTALL=0block at line 130 and therefore ahead ofcilium install. The DaemonSet's chained conflist can no longer land on disk while the binary is absent, so coredns/traefik/local-path-provisioner cannot fall into sandbox-creation backoff. Bonus: the check also runs on theSKIP_INSTALL=1audit path, catching a missing portmap on an already-installed cluster. -
sudo test -xon both probes — fixed at lines 99 and 101. The newelif sudo test -ebranch at line 105 distinguishes "exists but not executable" from "absent" on the k3s side, with a permissions-focused error. -
Smoke test — works. I checked empirically: modern portmap (v1.5.1, what k3s and
containernetworking-pluginsboth ship) returns exit 0 with stdoutCNI portmap plugin v1.5.1\nCNI protocol versions supported: …when invoked with no env.2>&1 | grep -q CNImatches and the pipeline exits 0;set -o pipefaildoesn't bite because portmap itself exits clean. A truncated/wrong-arch binary fails (no CNI token, grep exits 1, pipeline exits non-zero,!inverts, error block fires). Running it on the existing-binary path too is a real upgrade — catches a stale broken binary that an operator manually dropped at/opt/cni/bin/portmap. -
Stale-install remediation pointer — added at lines 226-230 with the exact reason (
cilium-configedits can't changechainingModebecause the agent reads it at startup) and the supported path. Matches the Calico-detection error style.
CI run for this commit (job 76626477276) shows the new block executing end-to-end on a fresh runner: Verifying portmap … → portmap missing from /opt/cni/bin; copying … → portmap CNI plugin binary OK, then cilium install proceeds successfully.
Non-blocking — error message for "exists at CNI_BIN_DIR but not executable, K3S source absent"
If /opt/cni/bin/portmap exists with mode 0644 (or otherwise non-x) AND /var/lib/rancher/k3s/data/current/bin/portmap is absent entirely (operator on a non-k3s host where they pre-populated /opt/cni/bin incorrectly), the script bails at scripts/install-cilium.sh:109-115 with portmap CNI plugin binary not found at /opt/cni/bin/portmap or /var/lib/rancher/k3s/data/current/bin/portmap. The file does exist at the first path — it's just not executable. Verified with a stub chmod 644 file. Contrived scenario, but the message points at the wrong remediation. A symmetric elif sudo test -e "$CNI_BIN_DIR/portmap" branch would surface this accurately. Optional.
Non-blocking — smoke-test rationale comment is mildly inaccurate
The comment at lines 118-122 says portmap "writes a CNI-spec error to stderr (e.g. CNI … missing)". In practice portmap exits 0 and writes the protocol-versions banner to stdout; the 2>&1 makes the test work regardless, and the failure mode (broken binary → no CNI token) is still what the test catches. The comment overstates what happens in the success path. Worth a sentence rewrite next time someone is in here. Optional.
Non-blocking — verify-failed remediation message is broad
The new block at lines 224-232 mentions "If this is a Cilium install from before #2713 (no cni-chaining-mode key)" but fires for any of the four for kv in … keys mismatching, including kube-proxy-replacement, enable-bpf-masquerade, enable-host-legacy-routing. The If this is … / Otherwise … framing mitigates, and the previous error "cilium-config[$key] = …" line tells the operator which key actually failed. Targeting the #2713 message specifically when $key = cni-chaining-mode (e.g., a case "$key" in cni-chaining-mode) extra hint ;; esac after the loop) would be cleaner. Optional.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
- Add a fourth elif branch for the 'CNI_BIN_DIR/portmap exists but is not executable AND K3S source is absent' case so the error message matches the actual failure mode instead of saying 'not found'. - Rewrite the smoke-test comment to match modern portmap behavior: v1.5.1 (k3s + containernetworking-plugins) exits 0 with a banner on stdout; older versions print a CNI-spec error to stderr. The 2>&1 merge catches either path; the broken-binary failure mode is what the test actually guards against. - Track which key triggered verify_failed so the #2713 chainingMode remediation hint only fires when cni-chaining-mode mismatched, not when any of the four asserted keys mismatched.
Contributor
|
Addressed all three non-blocking observations in c4ab3fe.
— Authored by egg |
This comment has been minimized.
This comment has been minimized.
Contributor
There was a problem hiding this comment.
Re-review
All three previous non-blocking observations addressed cleanly. Walked through each:
-
Fourth elif branch (
CNI_BIN_DIR/portmapexists but not executable, K3S source absent) — added atscripts/install-cilium.sh:109-113. Branch order is nowK3S executable (copy)→K3S exists, not executable (error: fix K3S perms)→CNI_BIN_DIR exists, not executable (error: fix file mode)→else: install missing. I traced the four reachable states:(CNI absent, K3S absent)→ else;(CNI absent, K3S exists non-x)→ K3S branch;(CNI non-x, K3S absent)→ new branch;(CNI non-x, K3S exists non-x)→ K3S branch (correct precedence — fixing K3S enables the copy path). All four error messages now name the actual failure mode. -
Smoke-test rationale comment rewrite — at lines 123-130 now accurately describes modern portmap behavior (v1.5.1 from k3s and
containernetworking-pluginsexits 0 with aCNI portmap plugin vX.Y.Zbanner on stdout) while preserving the older-version stderr fallback note. Explicitly names the binary-broken failure mode (ENOEXEC/segfault → noCNItoken on either stream) as what the test guards against, which is what matters for future readers. I verified the pipefail interaction holds: a wrong-arch binary returns 126 fromexec, pipefail propagates that as the pipeline status,!inverts to 0, error block fires — caught at install time. -
chaining_mode_failedflag — initialized at line 216, set inside the loop only whenkey = cni-chaining-mode(lines 231-233), gated the #2713 remediation hint at lines 238-243. The other three keys (kube-proxy-replacement,enable-bpf-masquerade,enable-host-legacy-routing) now drop straight to the generickubectl … get cm cilium-config -o yamlpointer without the misleading "this is a pre-#2713 install" framing. For pre-#2713 installs,cni-chaining-modeis absent sogot=""≠want="portmap", the flag flips to 1, and the hint fires — matching the intended scenario.
All checks pass on c4ab3fe (Shell lint, Integration Tests, Unit Tests, Security Scan). No further suggestions.
— Authored by egg
cni.chainingMode=portmap (set above to give us hostPort under kubeProxyReplacement=false) puts cilium-agent into a "chained" CNI mode where it treats itself as a secondary plugin behind a notional primary CNI, and defers iptables masquerade to that primary. But here Cilium IS the primary (it owns IPAM and the datapath); there is no other primary to install the rule. So agent's CILIUM_POST_nat chain stays empty even though cilium-config has enable-ipv4-masquerade=true, and pod traffic leaves the host with its pod-CIDR source IP intact. The internet routes responses to an unrouteable address, and any pod that needs external egress — gateway -> api.github.com for token refresh, sandbox agents -> Anthropic API — silently fails with connection timeouts. Observed locally: orchestrator pod could not reach api.github.com:443 or even 1.1.1.1:443; manually adding the missing MASQUERADE rule in POSTROUTING restored egress immediately and the gateway's token-refresher recovered from CrashLoopBackOff. Install the rule directly in POSTROUTING (not CILIUM_POST_nat, which the agent flushes on every config sync) so it survives cilium-agent restarts and config reloads. The match — `-s POD_POOL_CIDR ! -d POD_POOL_CIDR -j MASQUERADE` — is identical to what cilium-agent would install in non-chained mode. Read the pool CIDR from cilium-config's cluster-pool-ipv4-cidr key rather than hardcoding, so this still works if Cilium IPAM defaults change. Idempotent via `iptables -C ... || iptables -A ...` so re-runs (and the SKIP_INSTALL=1 audit path) are no-ops when the rule is already there.
This comment has been minimized.
This comment has been minimized.
Contributor
There was a problem hiding this comment.
Re-review
The new MASQUERADE-installation logic correctly diagnoses the chained-CNI gap (cilium-agent defers masquerade to a primary that doesn't exist) and the rule shape — -s POD_POOL_CIDR ! -d POD_POOL_CIDR -j MASQUERADE in POSTROUTING, matching what cilium-agent installs in non-chained mode — is right. Installing in POSTROUTING rather than CILIUM_POST_nat correctly survives cilium-agent config syncs.
One blocking issue: the user-facing error branch is dead code.
Blocking — friendly error block at lines 302-306 is unreachable; failure mode is a silent exit
scripts/install-cilium.sh:300-306:
POD_POOL_CIDR=$(kubectl -n kube-system get cm cilium-config -o "jsonpath={.data.cluster-pool-ipv4-cidr}" 2>/dev/null \
| grep -oE '[0-9]{1,3}(\.[0-9]{1,3}){3}/[0-9]{1,2}' | head -1)
if [ -z "$POD_POOL_CIDR" ]; then
error "Could not read cluster-pool-ipv4-cidr from cilium-config — cannot install pod-egress MASQUERADE rule."
error "Pod-to-external traffic (gateway -> GitHub, sandbox agents -> APIs) will fail without it."
exit 1
fiset -euo pipefail is active (line 13). When grep finds no match it exits 1, pipefail propagates that as the pipeline status, and set -e exits the script before the if [ -z "$POD_POOL_CIDR" ] check runs. The two error lines never fire.
Verified empirically against the exact pattern:
$ bash -c '
set -euo pipefail
log() { echo "[log] $*"; }
error() { echo "[err] $*" >&2; }
fake_kubectl() { echo ""; } # simulate cluster-pool-ipv4-cidr missing
log "Installing pod-egress MASQUERADE rule..."
POD_POOL_CIDR=$(fake_kubectl 2>/dev/null \
| grep -oE "[0-9]{1,3}(\.[0-9]{1,3}){3}/[0-9]{1,2}" | head -1)
if [ -z "$POD_POOL_CIDR" ]; then
error "Could not read cluster-pool-ipv4-cidr from cilium-config..."
exit 1
fi
echo "ok: $POD_POOL_CIDR"
'; echo "exit=$?"
[log] Installing pod-egress MASQUERADE rule...
exit=1
The operator sees Installing pod-egress MASQUERADE rule (compensates for chained-CNI mode)... and then nothing — no error, no diagnostic, just exit 1. They have to guess that the next-step verification is broken because a key wasn't readable from cilium-config.
Reachable triggers:
- A future Cilium release renames or restructures the
cluster-pool-ipv4-cidrConfigMap key (Cilium has already changed this surface once between major versions). - An operator on a non-cluster-pool IPAM mode (
ipam.mode=kubernetes,eni, etc.) — the key is absent on those modes. - The value is stored in a format the grep doesn't recognize (e.g., a future helm chart wraps it differently).
This matches the operator-facing misconfiguration produces no signal pattern: the intended loud failure is masked by set -e. The fix is one token:
POD_POOL_CIDR=$(kubectl ... 2>/dev/null \
| grep -oE '[0-9]{1,3}(\.[0-9]{1,3}){3}/[0-9]{1,2}' | head -1 || true)|| true lets the assignment complete with empty POD_POOL_CIDR, so the explicit check fires its diagnostic. Alternatively replace grep with awk (always exits 0).
Non-blocking — inconsistent jsonpath syntax for the hyphenated key
Line 300 uses dot notation: jsonpath={.data.cluster-pool-ipv4-cidr}. The verify loop at line 227 (same script, same ConfigMap) uses bracket notation: jsonpath={.data['$key']}. Bracket notation is the documented-safe form for hyphenated keys — older kubectl versions parsed cluster-pool-ipv4-cidr as subtraction. Modern kubectl handles dot-with-hyphens but the inconsistency invites a regression. Prefer:
... -o "jsonpath={.data['cluster-pool-ipv4-cidr']}"Non-blocking — Cilium's default pod-pool is 10.0.0.0/8
The cilium install invocation (lines 190-198) doesn't pass --set ipam.operator.clusterPoolIPv4PodCIDRList=..., so Cilium uses its default 10.0.0.0/8 — a very broad RFC1918 range. On hosts whose own primary IP is in 10.0.0.0/8 (corporate VPNs, AWS/GCP VPCs with 10.x subnets, etc.) the rule -s 10.0.0.0/8 ! -d 10.0.0.0/8 -j MASQUERADE also matches host-originated traffic from that IP. In practice MASQUERADE rewrites source to the outbound iface IP — usually the same address — so it's functionally a no-op, but it's a footgun for unusual routing topologies. A narrower pool (10.244.0.0/16-style) would scope the rule to actual pod traffic. Worth a comment if not a change.
Non-blocking — rule does not persist across host reboots
iptables -A writes only to the kernel's runtime table; no iptables-save / netfilter-persistent / systemd unit is wired up. After a host reboot the rule is gone and pod egress breaks again until this script (or make k3s-setup) is re-run.
The commit message says the rule "survives cilium-agent restarts and config reloads" — true, but a clarifying "(not host reboots — re-run this script after a reboot, or make k3s-setup)" in the in-script comment at line 294-298 would prevent the predictable confusion when the orchestrator pod silently stops reaching GitHub after a reboot.
Non-blocking — IPv6 / dual-stack silently uncovered
Only cluster-pool-ipv4-cidr is read. If dual-stack ever lands (Cilium populates cluster-pool-ipv6-cidr), the equivalent ip6tables -t nat -A POSTROUTING -s <v6-pool> ! -d <v6-pool> -j MASQUERADE would be needed too, and v6 pod egress would silently fail the same way v4 did pre-PR. Not configured today, so non-blocking — but a TODO comment in this block would help the next person enabling v6.
Non-blocking — iptables-legacy vs iptables-nft skew
If the host's /usr/sbin/iptables points at iptables-legacy while cilium-agent (or kube-proxy embedded in k3s-agent) uses iptables-nft, the rule lands in a different physical table from the rest of the policy graph and is invisible to packet processing. Modern Ubuntu and stock k3s both use nft, so this is rare in the supported configurations. A one-line update-alternatives --display iptables 2>/dev/null | head -3 in the log on install would surface skew at the time it's introduced. Optional.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…ggestions
The pod-egress MASQUERADE block silently swallowed the 'cluster-pool-ipv4-cidr
missing from cilium-config' failure because 'set -euo pipefail' + grep's
exit-1-on-no-match short-circuited the script before the empty-string
check could fire. Add '|| true' to the grep pipeline so the assignment
completes and the explicit diagnostic runs.
Also addressed in this commit:
- Bracket-notation jsonpath ({.data['cluster-pool-ipv4-cidr']}) for
consistency with the verify loop and safety on older kubectl.
- Documented the 10.0.0.0/8 default pod-pool footgun, lack of reboot
persistence, IPv6/dual-stack TODO, and iptables backend skew.
- Log 'update-alternatives --display iptables' so iptables-legacy hosts
surface at install time rather than as silent packet loss.
- Improved diagnostic enumerates likely causes (IPAM mode, key rename)
and points operators at the right kubectl inspection command.
Contributor
|
Thanks for the careful review. All addressed in 564b915.
— Authored by egg |
This comment has been minimized.
This comment has been minimized.
Contributor
There was a problem hiding this comment.
Re-review
All previous feedback addressed cleanly in 564b915. Walked through each concern.
Blocking — silent-failure error block (fixed)
scripts/install-cilium.sh:335 now ends the grep pipeline with || true:
POD_POOL_CIDR=$(kubectl -n kube-system get cm cilium-config -o "jsonpath={.data['cluster-pool-ipv4-cidr']}" 2>/dev/null \
| grep -oE '[0-9]{1,3}(\.[0-9]{1,3}){3}/[0-9]{1,2}' | head -1 || true)
if [ -z "$POD_POOL_CIDR" ]; then
error "Could not read cluster-pool-ipv4-cidr from cilium-config — cannot install pod-egress MASQUERADE rule."
...Verified empirically with the same harness from my prior review — with || true in place, an empty kubectl response now produces the full diagnostic block before exit 1 instead of the previous silent exit. The new error text also enumerates the likely root causes (non-cluster-pool IPAM mode, future Cilium key rename) and points at the right kubectl get cm cilium-config -o yaml | grep -i cidr inspection command — operators hitting this no longer have to guess. Happy path still extracts the CIDR correctly.
Non-blocking observations — all addressed
- Bracket-notation jsonpath — line 334 now uses
{.data['cluster-pool-ipv4-cidr']}, matching the verify-loop pattern at line 227. Same documented-safe form throughout the script. 10.0.0.0/8default pool footgun — comment block at lines 304-312 documents the host-IP-in-10.x edge case, why MASQUERADE source-rewriting makes it a functional no-op, and the narrower-pool remediation. Not changed in-place — narrowing the default would be a breaking config change for any existing cluster.- Reboot persistence note — lines 296-299 now explicitly call out "this is a runtime iptables rule — it is NOT persisted across host reboots. Re-run this script (or
make k3s-setup) after a reboot, or wire the rule intonetfilter-persistent/iptables-restoreat the system level." Exact breadcrumb the next reboot-debugger needs. - IPv6 / dual-stack TODO — lines 314-318 name the symmetric
ip6tables -t nat -A POSTROUTING -s <v6-pool> ! -d <v6-pool> -j MASQUERADErule and thecluster-pool-ipv6-cidrkey the future code would read. Same silent-egress-drop failure mode called out so dual-stack enablement doesn't repeat the v4 bug. - iptables backend skew —
update-alternatives --display iptables 2>/dev/null | head -3 | sed 's/^/ iptables-alt: /' || trueat line 327. Trailing|| truecorrectly handles bothupdate-alternativesnot being present (some minimal images) and non-zero exits when iptables isn't alternatives-managed. Verified the failure modes don't break the script. Modern Ubuntu and stock k3s surfaceiptables-nft; an iptables-legacy host shows up at install time instead of silently sending packets to a different physical table.
Verification
bash -n scripts/install-cilium.sh— syntax OK.shellcheck— only the two pre-existingSC2001warnings in theCNI_FILESblock (unrelated to this PR, lines 268/274).- CI: Shell, Unit Tests, Integration Tests, Security Scan, Docker, YAML, Python, Validate configuration — all green on 564b915.
No new issues. Ready to merge.
— Authored by egg
Contributor
|
egg review completed. View run logs 12 previous review(s) hidden. |
jwbron
added a commit
that referenced
this pull request
May 19, 2026
…ater] (#2716) * docs: document portmap CNI and iptables reboot requirement (#2713) * docs: mention netfilter-persistent as alternative to re-running after reboot Addresses non-blocking review feedback on #2716. The reviewer noted that install-cilium.sh's own comment block calls out netfilter-persistent / iptables-restore as a system-level persistence alternative to re-running the script after every reboot. Mirror that in the operator-facing doc so long-running k3s host operators know they have an option beyond manual re-runs. --------- Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>
jwbron
added a commit
that referenced
this pull request
May 20, 2026
#2724) * docs: add claude-code substrate to index and structure docs [doc-updater] (#2718) * docs: add claude-code substrate to index and structure docs * docs: fix substrate/claude_code listing per reviewer feedback - Drop incorrect '+ Agent tool' from spawner.py description; the spike runs egg_harness.run_agent in-process and does NOT dispatch via the Agent tool (Agent-tool spawner is an ADR follow-up). - Add hook_entry.py to the listing — it is the standalone PreToolUse hook script and the largest file in the package (~31 KB). - Clarify policy.py is the PolicyEnforcer adapter wrapping hook_entry.py. - List settings.template.json for navigability. Addresses egg-reviewer CHANGES_REQUESTED on PR #2718. --------- Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * docs: update deployment guide for Cilium portmap CNI changes [doc-updater] (#2716) * docs: document portmap CNI and iptables reboot requirement (#2713) * docs: mention netfilter-persistent as alternative to re-running after reboot Addresses non-blocking review feedback on #2716. The reviewer noted that install-cilium.sh's own comment block calls out netfilter-persistent / iptables-restore as a system-level persistence alternative to re-running the script after every reboot. Mirror that in the operator-facing doc so long-running k3s host operators know they have an option beyond manual re-runs. --------- Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * docs: add reconcile_autostash_pop_conflict to push diagnostic list (#2720) Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> * slice-1 coder: bridge driver + R2 nested-dispatch fake + loader expansion Implements three #2717 slice-1 coder tasks toward the substrate-swap follow-up rollout per cq-1=Option C hybrid bridge (refine/plan flattened, implement daemon), cq-2 phase-sequential slicing, cq-5 early-spike R2 validation, and cq-3 harness re-host alignment. TASK-1-1 (bin/run_pipeline.py): flattened single-yield stage driver that advances `run_pipeline_in_process` to its next yield, serialises the yielded HITLDecision to .egg-state/contracts/<id>.json under a stable `pending_hitl` envelope schema (version, decision, answer, answer_log, status, result, error), and exits. Cross-process generator state is recovered by replaying `answer_log` on each invocation — viable for refine/plan because the generator is deterministic; slice-3's daemon variant (TASK-3-2) consumes the same envelope schema so the two bridges share a state-serialization contract (risk_analyst R17 mitigation). End-to-end round-trip verified: first invocation yields preflight decision; operator answer round-trips through the contract; second invocation replays and advances to the refine-gate decision. TASK-1-9 (integration_tests/regression/_agent_tool_fake.py): test- only nested-Agent-tool dispatch fake. Simulates Claude Code's Agent tool by spawning a child subprocess with controlled EGG_AGENT_ROLE; the child invokes orchestrator/substrate/claude_code/hook_entry.py `decide(...)` directly. Validates the hook-logic half of R2 — given accurate EGG_AGENT_ROLE propagation, does the hook deny a write that violates the *child's* role pattern even when the parent's role would allow it? Hard import guard prevents production use; the file is coder-owned (underscored helper name, mirroring _helpers.py) rather than tester-owned. Production dispatch stays on ClaudeCodeSpawner (harness re-host) per cq-3. TASK-1-6 (orchestrator/substrate/__init__.py): extends `_load_egg_sdlc_role_rubric` so reviewer_refine and reviewer_agent_design are recognised as supported (alongside the existing refiner). Introduces a `_ROLE_RUBRIC_SLICES` mapping that names which #2717 rollout slice ships each role's rubric (slice-1: refine team; slice-2: plan team; slice-3: implement team) so future slice loaders can extend the set declaratively, and a `_RUBRIC_LANDED_ROLES` set documenting which rubric .md files exist on disk today. Roles outside the landed set raise a structured ValueError citing the correct rollout slice ("deferred to follow-up slice-2 of issue #2717's rollout"). The acceptance criterion's "follow-up slice 2" hint for architect is now produced. Lint clean (ruff check + format); mypy clean on the new files; the pre-existing 6 mypy errors in substrate/__init__.py:180-198 are unrelated to this change. 52 existing claude-code-substrate tests still pass. Refs #2717 (slice-1 coder). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * docs(#2717 slice-1): refine-team rubrics + flattened-bridge docs + ADR rollout deltas Slice 1 of the #2717 substrate-swap rollout adds two refine-team reviewer rubrics, closes the heredoc-HITL bridge gap for refine-phase via a flattened bin/run_pipeline.py stage driver, and adds the cq-5 R2 spike for nested PreToolUse-hook role-routing. This commit lands the documenter-owned half: TASK-1-4: New reviewer rubric files at plugins/egg-sdlc/skills/egg-sdlc/agents/reviewer_refine.md plugins/egg-sdlc/skills/egg-sdlc/agents/reviewer_agent_design.md mirroring the layout of plugins/refine-plan/skills/refine-plan/agents/ so the in-process orchestrator's build_system_prompt(sources) loader picks them up without per-skill custom logic. Both files carry frontmatter (name + description) and the body documents the substrate-specific operational deltas (worktree path, PreToolUse-enforced restrictions, AskUserQuestion surfacing, verdict path) on top of the same rubric the k3s reviewers use. TASK-1-2: SKILL.md is updated to replace the "Walking-skeleton bridge gap" callout with a "How the flattened bridge works" section that names pending_hitl as the single-yield carrier and documents the skill→driver loop. The "What the skill is designed to do" step list moves from aspirational to actually-shipping. The R2 PreToolUse-hook section points at the new test infrastructure (test_pretooluse_hook_nested.py + _agent_tool_fake.py) and the r2-verdict.json file. Frontmatter description re-flows to reflect the slice-1 scope. TASK-1-8: docs/architecture/claude-code-substrate.md is updated for the ADR-level audit trail: - Title and status banner reframe from "spike" to "spike → rollout". - cq-2 / cq-7 / cq-11 table rows reflect what slice 1 lands. - The in-process orchestrator section gets a "The flattened bridge" subsection naming the cq-1 hybrid (Option C) and the slice-3 daemon variant that consumes the same pending_hitl envelope shape (R17 mitigation). - The egg-sdlc plugin section enumerates the three refine-team rubrics and the new bin/run_pipeline.py driver. - The R2 risk-mitigation subsection points at the slice-1 worked example and the slice-5 contingent fallback (cq-6 option 2 + R15 model (b)). - The R15 subsection makes the model (a) → (b) migration contingent on the slice-1 R2 verdict. - The "Open work" + "Follow-up issue draft" sections are replaced with a unified "Rollout deltas" section split into Completed-in-this-rollout (3 slice-1 items, marked with [x] + strikethrough on the obsolete text) and Pending-in-this-rollout (9 items mapped to slices 2-5). The acceptance bar is unchanged. - The primitives table picks up the four new slice-1 modules (bin/run_pipeline.py, _agent_tool_fake.py, test_pretooluse_hook_nested.py, the two new reviewer rubrics). - The conformance-proof section names the slice-1 regression-test additions (test_bridge_flattened_round_trip.py, test_rubric_loader.py). - Stale anchor links to the removed "Follow-up issue draft" section are redirected to the new "Rollout deltas" anchor. These doc changes satisfy TASK-1-2, TASK-1-4, and TASK-1-8 from slice 1 of the #2717 plan; no source or test files are touched. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * slice-1 tester: rubric loader + bridge round-trip + R2 nested-dispatch tests Adds three test files covering #2717 slice-1's tester contract tasks: TASK-1-7 (shared/tests/test_rubric_loader.py): unit tests for `_load_egg_sdlc_role_rubric`. Pins the four AC cases: refiner regression, reviewer_refine load, reviewer_agent_design load, architect raises ValueError with the updated "follow-up slice 2" diagnostic. Adversarial probing layered on: AgentRole enum vs str input equivalence, defense against path-traversal role values, structured fence for unshipped plan-phase roles (reviewer_plan, reviewer_code, task_planner). Eight of 10 tests pass today; two (reviewer_refine, reviewer_agent_design loads) are documenter-dependency failures that flip green once task-1-4 lands the rubric .md files. TASK-1-3 (integration_tests/regression/test_bridge_flattened_round_trip.py): end-to-end round-trip test for the flattened bridge driver. Runs the real `bin/run_pipeline.py` in a fresh subprocess twice against a deterministic pipeline id: stage A captures the preflight HITLDecision into `pending_hitl.decision`, the test writes `answer="approve" + status="answered"`, stage B re-enters the process and replays the answer to advance to the refine-gate decision. Validates that the generator state survives via the contract-state round-trip across process exit. Substrate isolation via a `-c` shim that monkey-patches `orchestrator.substrate.select_substrate` to a MagicMock bundle — no real Claude Code / Anthropic API call. Also pins driver-side idempotency (a re-invocation without a new answer must not silently advance the generator). TASK-1-5 (integration_tests/regression/test_pretooluse_hook_nested.py): cq-5 early-spike R2 verdict test. Uses task-1-9's `_agent_tool_fake` to drive a deterministic nested-dispatch scenario: parent_role=architect + child_role=tester + write_target=orchestrator/foo.py — asserts the hook returns `{"decision": "block", "reason": ...}` and that the deny reason names the child (tester) role rather than the parent. Writes `.egg-state/<pipeline_id>/r2-verdict.json` with the pass verdict per AC. Adds in-role allow control + cross-role probe (parent=coder, child=tester writing orchestrator/* — must deny by the child's role) + dataclass shape pin + EGG_AGENT_ROLE leak guard. Docstring documents the empirical-vs-test-fake limitation cq-3 explicitly accepts (production stays on the harness re-host until slice-5 R15 flips dispatch). All 15 of 17 tests pass today. The 2 failing rubric tests are contracted documenter-dependency failures (task-1-4 not landed yet) and are expected to flip green once the documenter ships. Lint clean (ruff check + format). Refs #2717 (slice-1 tester). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * docs(#2717 slice-1): address reviewer_code v1 NACK on SKILL.md envelope + CLI Reviewer_code NACKed v1 with three blockers against SKILL.md (the two rubric files and the ADR were ACKed as-is). This commit addresses all three blockers plus four non-blocking polish items. Blocking fixes in SKILL.md: 1. CLI invocation example was wrong (used --pipeline-id / --issue, but the driver at plugins/egg-sdlc/skills/egg-sdlc/bin/run_pipeline.py:355-402 takes pipeline_id as a positional arg and --issue-number as the flag). Rewrote the bash loop example to match the actual argparse signature so a copy-paste invocation actually runs. 2. The documented pending_hitl envelope was 5 fields (version, pipeline_id, timestamp, decision, answer) but the driver writes 9 (adds status, result, error, answer_log). Replaced the truncated schema with the full envelope and added per-field semantics (especially the status field, which is the skill's loop predicate: pending / answered / completed / aborted / error). The slice-3 daemon variant inherits all 9 fields. 3. No documented mechanism for the skill body to write pending_hitl.answer (the frontmatter allowed-tools does not include the Write tool). Documented option (a) from the reviewer's NACK: an inline python3 -c "..." invocation, which is covered by the existing Bash(python3 *:*) allowed-tool. The "skill loop" code block now demonstrates the round-trip with a case statement keyed on pending_hitl.status. Non-blocking polish in SKILL.md: - Loop semantics now name "replay" explicitly (the driver spawns a fresh generator and replays answer_log on every invocation; previous text suggested cheap single-step resumption). Added a dedicated "Generator state across invocations (replay semantics)" subsection naming the practical consequence — side effects re-run every call — and pointing at slice 3 as the daemon-variant escape hatch. - Failure-mode bullet for "pending_hitl.decision == null" replaced with the more general "pending_hitl.status ∈ {completed, aborted, error}" bullet so each terminal state has documented diagnostic guidance. Non-blocking polish in docs/architecture/claude-code-substrate.md: - The "Flattened bridge" bullet in the in-process orchestrator section now names the replay path explicitly (promotes answer → answer_log, replays the full log every call, deterministic same-yield-boundary property) and the 5→9 envelope field list mirrors SKILL.md. - Daemon-variant bullet enumerates the same 9-field shape so reviewers comparing slice-1 and slice-3 against the ADR see the full contract. - Schema source-of-truth pointer added to bin/run_pipeline.py:20-46 so future drift triggers fail in one place. Rubric files (reviewer_refine.md, reviewer_agent_design.md) are unchanged — reviewer_code ACKed them in v1. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test_bridge_flattened_round_trip: fix subprocess PYTHONPATH The subprocess shim's PYTHONPATH pointed at `<repo>/orchestrator/` and `<repo>/shared/` directly, which lets the subprocess `import substrate` but NOT `import orchestrator.substrate` — the latter requires the *parent* of `orchestrator/` (the repo root) on the path because ``orchestrator/__init__.py`` makes it a real package. Set PYTHONPATH to ``<repo>/shared`` + ``<repo>`` + ``<repo>/gateway`` so: - ``<repo>/shared`` lets ``egg_contracts`` (transitive import from ``orchestrator.substrate.k3s_adapter``) resolve. - ``<repo>`` lets ``import orchestrator`` resolve. Refs #2717 (slice-1 tester). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * slice-1 tester v2: fix subprocess PYTHONPATH + non-blocking improvements Addresses reviewer_code v1 blocking #1 + non-blocking items: BLOCKING FIX — subprocess PYTHONPATH: Add `<repo>/orchestrator` to the subprocess shim's PYTHONPATH so bare- name top-level imports inside the `orchestrator/` tree resolve cleanly. Without this, `orchestrator/models.py:16` (`from slice_id_validation import SLICE_ID_PATTERN`) and `in_process.py:531-534`'s bare `from models import HITLDecision` fallback both fail, crashing the driver subprocess with `ModuleNotFoundError` before it yields the first HITL decision. Mirrors the Makefile's `PYTHONPATH := shared:gateway: orchestrator` (test target). Verified: tests now pass with `PYTHONPATH=.:shared:orchestrator pytest <files>` (reviewer_code's exact reproduction env). NON-BLOCKING (reviewer_code v1): - test_bridge_flattened_round_trip.py: drop stale "whichever the coder picks" docstring phrasing — driver locked in positional argv[1]. - test_bridge_flattened_round_trip.py: mirror the driver's ISO-8601 UTC timestamp format in _write_answer instead of `str(time.time())`. - test_rubric_loader.py: extend `test_loader_accepts_enum_and_string_role` parametrization to cover the two NEW roles (reviewer_refine, reviewer_agent_design) — not just the regression role. - test_rubric_loader.py: strengthen path-traversal assertion to verify the allowlist's slice-fence branch fires (not the file-missing-on- disk branch) — pinning the structural defence. - test_pretooluse_hook_nested.py: derive the r2-verdict.json content from the dispatch outcome and write it BEFORE the structured assertions so slice-5 sees an accurate empirical record even when a regression fails one of the assertions. Adds {"r2_verdict": "fail", "reason": ...} payload format for the failure path. Tests verified: 21/21 pass under both the canonical Makefile PYTHONPATH shape and reviewer_code's `.:shared:orchestrator` reproduction shape. Refs #2717 (slice-1 tester, v2). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Persist BRC history for slice-1 (#2548) * Address slice-1 review: fix install path, bridge answer-write, silent fallbacks Reviewer findings from PR #2724 (egg-reviewer slice-1 review): Blockers (1-4): * The documented `pip install -r requirements.txt` failed — no top-level requirements.txt exists. Switch SKILL.md, plugin.json's `egg.install_instructions`, and `bin/preflight.py` to `pip install .` against pyproject.toml. * SKILL.md claimed Python 3.11+ but pyproject.toml requires >=3.14. Update SKILL.md and preflight to reflect the real floor; correct the matching docstring in orchestrator/substrate/__init__.py. * The inline `python3 -c "..."` answer-write snippet in SKILL.md was broken (shell-interpolated `${ANSWER}` → NameError, deprecated `datetime.utcnow`, non-atomic write). Replace with a dedicated `bin/write_answer.py` helper that reads the JSON-encoded answer from stdin, uses `datetime.now(UTC)`, and writes atomically via tmp + `os.replace`. SKILL.md's loop now invokes the helper. * `_serialise_decision` silently swallowed `model_dump` failures, and `_read_contract` silently overwrote unparseable contracts (dropping `answer_log`). Log to stderr on serialisation fallbacks; refuse to overwrite a corrupted contract and exit 1 instead. Non-blocking (5-11): * `orchestrator/substrate/__init__.py` docstring rewritten to reflect reality: Python 3.14+ introduced PEP 758 (`except A, B:` without parens); ruff under py314 target strips redundant parens, hence `# fmt: skip` on multi-except lines. * Stderr warning in `_advance_generator`'s `finally` so a teardown failure inside `generator.close()` is at least observable. * `allowed-tools` tightened from `Bash(python3 *:*)` to `Bash(python3 plugins/egg-sdlc/skills/egg-sdlc/bin/*:*)` — the two helper scripts are the entire Python surface the skill can invoke. * Re-spawn cost note expanded in SKILL.md: each driver invocation re-runs every prior subagent spawn (real Anthropic API spend); slice-2 compounds to 8 spawns at the final stage. * `_RUBRIC_LANDED_ROLES` registry collapsed into `_CURRENT_LOADER_SLICE` + filesystem probe; same diagnostics, no parallel registry that can drift from disk state. * Abort vocabulary exported as `ABORT_ANSWERS` from `orchestrator.substrate.in_process`; the driver imports it (with a literal fallback when the orchestrator package is unimportable) so the driver, orchestrator, and slice-3 daemon share a single source of truth. * R2 caveat surfaced as an explicit open question for slice-5 sequencing in SKILL.md. Tests: * New `shared/tests/test_write_answer.py` (6 tests, all passing) pins the JSON-encoding round-trip, the timestamp format match against the driver, the atomic-write contract, and the corrupted-contract refusal. * Existing `test_rubric_loader`, `test_run_pipeline_in_process*`, `test_substrate_interfaces`, `test_bridge_flattened_round_trip`, and `test_pretooluse_hook_nested` all still pass. * The pre-existing `test_empty_diff_subprocess_skips_pytest` failure in `tests/tools/` reproduces against HEAD without these changes (detached-HEAD worktree edge case in the test selector). Authored-by: egg * Move skill-loop python3 -c calls into bin/ helpers Address review feedback on PR #2724: - Add bin/read_status.py and extend write_answer.py with --answer-string so every subcommand in SKILL.md's loop body is a single `python3 plugins/.../bin/<helper>.py` invocation. Honors the tightened allowed-tools pattern `Bash(python3 plugins/.../bin/*:*)` per Claude Code's compound-command permission rules — no separate `Bash(python3 -c *)` or `Bash(printf *)` rule needed, no prompt-injection door left open. - Update SKILL.md step 4 to name bin/write_answer.py directly (matches the new loop body). - Replace `slice_hint != _CURRENT_LOADER_SLICE` rubric-loader fence with `slice_hint not in _LANDED_SLICES` (frozenset) so future slices extend rather than replace the landed set — slice-2 won't fence off slice-1's already-shipped refine roles. - Wire test_bridge_flattened_round_trip's _write_answer through subprocess(write_answer.py) so the integration test exercises the production write path end-to-end. - Add test_read_status.py (7 tests) and --answer-string coverage in test_write_answer.py (2 tests). * Address non-blocking review notes on PR #2724 - write_answer.py: clarify --answer-string docstring — the JSON encoding happens at contract serialisation time (json.dumps(contract)), not as a separate json.dumps(answer) step. Reference the special-characters test as the proof of the round-trip. - SKILL.md / read_status.py: document the case statement's intentional fall-through on empty STATUS. read_status.py prints empty + exit 0 when no pending_hitl envelope exists; the case has no *) arm, so the empty value falls through, the case exits 0, and the outer iteration re-invokes run_pipeline.py — which is the recover path. - test_rubric_loader.py: add test_landed_slices_contains_slice1 to mechanically pin the 'extend, don't replace' invariant on _LANDED_SLICES so a future slice cannot silently regress slice-1 by writing frozenset({'slice-2'}) instead of frozenset({'slice-1', 'slice-2'}). --------- Co-authored-by: james-in-a-box[bot] <246424927+james-in-a-box[bot]@users.noreply.github.com> Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> Co-authored-by: egg <egg@example.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> Co-authored-by: egg-orchestrator <egg@localhost>
jwbron
added a commit
that referenced
this pull request
May 20, 2026
#2726) * docs: add claude-code substrate to index and structure docs [doc-updater] (#2718) * docs: add claude-code substrate to index and structure docs * docs: fix substrate/claude_code listing per reviewer feedback - Drop incorrect '+ Agent tool' from spawner.py description; the spike runs egg_harness.run_agent in-process and does NOT dispatch via the Agent tool (Agent-tool spawner is an ADR follow-up). - Add hook_entry.py to the listing — it is the standalone PreToolUse hook script and the largest file in the package (~31 KB). - Clarify policy.py is the PolicyEnforcer adapter wrapping hook_entry.py. - List settings.template.json for navigability. Addresses egg-reviewer CHANGES_REQUESTED on PR #2718. --------- Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * docs: update deployment guide for Cilium portmap CNI changes [doc-updater] (#2716) * docs: document portmap CNI and iptables reboot requirement (#2713) * docs: mention netfilter-persistent as alternative to re-running after reboot Addresses non-blocking review feedback on #2716. The reviewer noted that install-cilium.sh's own comment block calls out netfilter-persistent / iptables-restore as a system-level persistence alternative to re-running the script after every reboot. Mirror that in the operator-facing doc so long-running k3s host operators know they have an option beyond manual re-runs. --------- Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * docs: add reconcile_autostash_pop_conflict to push diagnostic list (#2720) Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> * slice-1 coder: bridge driver + R2 nested-dispatch fake + loader expansion Implements three #2717 slice-1 coder tasks toward the substrate-swap follow-up rollout per cq-1=Option C hybrid bridge (refine/plan flattened, implement daemon), cq-2 phase-sequential slicing, cq-5 early-spike R2 validation, and cq-3 harness re-host alignment. TASK-1-1 (bin/run_pipeline.py): flattened single-yield stage driver that advances `run_pipeline_in_process` to its next yield, serialises the yielded HITLDecision to .egg-state/contracts/<id>.json under a stable `pending_hitl` envelope schema (version, decision, answer, answer_log, status, result, error), and exits. Cross-process generator state is recovered by replaying `answer_log` on each invocation — viable for refine/plan because the generator is deterministic; slice-3's daemon variant (TASK-3-2) consumes the same envelope schema so the two bridges share a state-serialization contract (risk_analyst R17 mitigation). End-to-end round-trip verified: first invocation yields preflight decision; operator answer round-trips through the contract; second invocation replays and advances to the refine-gate decision. TASK-1-9 (integration_tests/regression/_agent_tool_fake.py): test- only nested-Agent-tool dispatch fake. Simulates Claude Code's Agent tool by spawning a child subprocess with controlled EGG_AGENT_ROLE; the child invokes orchestrator/substrate/claude_code/hook_entry.py `decide(...)` directly. Validates the hook-logic half of R2 — given accurate EGG_AGENT_ROLE propagation, does the hook deny a write that violates the *child's* role pattern even when the parent's role would allow it? Hard import guard prevents production use; the file is coder-owned (underscored helper name, mirroring _helpers.py) rather than tester-owned. Production dispatch stays on ClaudeCodeSpawner (harness re-host) per cq-3. TASK-1-6 (orchestrator/substrate/__init__.py): extends `_load_egg_sdlc_role_rubric` so reviewer_refine and reviewer_agent_design are recognised as supported (alongside the existing refiner). Introduces a `_ROLE_RUBRIC_SLICES` mapping that names which #2717 rollout slice ships each role's rubric (slice-1: refine team; slice-2: plan team; slice-3: implement team) so future slice loaders can extend the set declaratively, and a `_RUBRIC_LANDED_ROLES` set documenting which rubric .md files exist on disk today. Roles outside the landed set raise a structured ValueError citing the correct rollout slice ("deferred to follow-up slice-2 of issue #2717's rollout"). The acceptance criterion's "follow-up slice 2" hint for architect is now produced. Lint clean (ruff check + format); mypy clean on the new files; the pre-existing 6 mypy errors in substrate/__init__.py:180-198 are unrelated to this change. 52 existing claude-code-substrate tests still pass. Refs #2717 (slice-1 coder). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * docs(#2717 slice-1): refine-team rubrics + flattened-bridge docs + ADR rollout deltas Slice 1 of the #2717 substrate-swap rollout adds two refine-team reviewer rubrics, closes the heredoc-HITL bridge gap for refine-phase via a flattened bin/run_pipeline.py stage driver, and adds the cq-5 R2 spike for nested PreToolUse-hook role-routing. This commit lands the documenter-owned half: TASK-1-4: New reviewer rubric files at plugins/egg-sdlc/skills/egg-sdlc/agents/reviewer_refine.md plugins/egg-sdlc/skills/egg-sdlc/agents/reviewer_agent_design.md mirroring the layout of plugins/refine-plan/skills/refine-plan/agents/ so the in-process orchestrator's build_system_prompt(sources) loader picks them up without per-skill custom logic. Both files carry frontmatter (name + description) and the body documents the substrate-specific operational deltas (worktree path, PreToolUse-enforced restrictions, AskUserQuestion surfacing, verdict path) on top of the same rubric the k3s reviewers use. TASK-1-2: SKILL.md is updated to replace the "Walking-skeleton bridge gap" callout with a "How the flattened bridge works" section that names pending_hitl as the single-yield carrier and documents the skill→driver loop. The "What the skill is designed to do" step list moves from aspirational to actually-shipping. The R2 PreToolUse-hook section points at the new test infrastructure (test_pretooluse_hook_nested.py + _agent_tool_fake.py) and the r2-verdict.json file. Frontmatter description re-flows to reflect the slice-1 scope. TASK-1-8: docs/architecture/claude-code-substrate.md is updated for the ADR-level audit trail: - Title and status banner reframe from "spike" to "spike → rollout". - cq-2 / cq-7 / cq-11 table rows reflect what slice 1 lands. - The in-process orchestrator section gets a "The flattened bridge" subsection naming the cq-1 hybrid (Option C) and the slice-3 daemon variant that consumes the same pending_hitl envelope shape (R17 mitigation). - The egg-sdlc plugin section enumerates the three refine-team rubrics and the new bin/run_pipeline.py driver. - The R2 risk-mitigation subsection points at the slice-1 worked example and the slice-5 contingent fallback (cq-6 option 2 + R15 model (b)). - The R15 subsection makes the model (a) → (b) migration contingent on the slice-1 R2 verdict. - The "Open work" + "Follow-up issue draft" sections are replaced with a unified "Rollout deltas" section split into Completed-in-this-rollout (3 slice-1 items, marked with [x] + strikethrough on the obsolete text) and Pending-in-this-rollout (9 items mapped to slices 2-5). The acceptance bar is unchanged. - The primitives table picks up the four new slice-1 modules (bin/run_pipeline.py, _agent_tool_fake.py, test_pretooluse_hook_nested.py, the two new reviewer rubrics). - The conformance-proof section names the slice-1 regression-test additions (test_bridge_flattened_round_trip.py, test_rubric_loader.py). - Stale anchor links to the removed "Follow-up issue draft" section are redirected to the new "Rollout deltas" anchor. These doc changes satisfy TASK-1-2, TASK-1-4, and TASK-1-8 from slice 1 of the #2717 plan; no source or test files are touched. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * slice-1 tester: rubric loader + bridge round-trip + R2 nested-dispatch tests Adds three test files covering #2717 slice-1's tester contract tasks: TASK-1-7 (shared/tests/test_rubric_loader.py): unit tests for `_load_egg_sdlc_role_rubric`. Pins the four AC cases: refiner regression, reviewer_refine load, reviewer_agent_design load, architect raises ValueError with the updated "follow-up slice 2" diagnostic. Adversarial probing layered on: AgentRole enum vs str input equivalence, defense against path-traversal role values, structured fence for unshipped plan-phase roles (reviewer_plan, reviewer_code, task_planner). Eight of 10 tests pass today; two (reviewer_refine, reviewer_agent_design loads) are documenter-dependency failures that flip green once task-1-4 lands the rubric .md files. TASK-1-3 (integration_tests/regression/test_bridge_flattened_round_trip.py): end-to-end round-trip test for the flattened bridge driver. Runs the real `bin/run_pipeline.py` in a fresh subprocess twice against a deterministic pipeline id: stage A captures the preflight HITLDecision into `pending_hitl.decision`, the test writes `answer="approve" + status="answered"`, stage B re-enters the process and replays the answer to advance to the refine-gate decision. Validates that the generator state survives via the contract-state round-trip across process exit. Substrate isolation via a `-c` shim that monkey-patches `orchestrator.substrate.select_substrate` to a MagicMock bundle — no real Claude Code / Anthropic API call. Also pins driver-side idempotency (a re-invocation without a new answer must not silently advance the generator). TASK-1-5 (integration_tests/regression/test_pretooluse_hook_nested.py): cq-5 early-spike R2 verdict test. Uses task-1-9's `_agent_tool_fake` to drive a deterministic nested-dispatch scenario: parent_role=architect + child_role=tester + write_target=orchestrator/foo.py — asserts the hook returns `{"decision": "block", "reason": ...}` and that the deny reason names the child (tester) role rather than the parent. Writes `.egg-state/<pipeline_id>/r2-verdict.json` with the pass verdict per AC. Adds in-role allow control + cross-role probe (parent=coder, child=tester writing orchestrator/* — must deny by the child's role) + dataclass shape pin + EGG_AGENT_ROLE leak guard. Docstring documents the empirical-vs-test-fake limitation cq-3 explicitly accepts (production stays on the harness re-host until slice-5 R15 flips dispatch). All 15 of 17 tests pass today. The 2 failing rubric tests are contracted documenter-dependency failures (task-1-4 not landed yet) and are expected to flip green once the documenter ships. Lint clean (ruff check + format). Refs #2717 (slice-1 tester). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * docs(#2717 slice-1): address reviewer_code v1 NACK on SKILL.md envelope + CLI Reviewer_code NACKed v1 with three blockers against SKILL.md (the two rubric files and the ADR were ACKed as-is). This commit addresses all three blockers plus four non-blocking polish items. Blocking fixes in SKILL.md: 1. CLI invocation example was wrong (used --pipeline-id / --issue, but the driver at plugins/egg-sdlc/skills/egg-sdlc/bin/run_pipeline.py:355-402 takes pipeline_id as a positional arg and --issue-number as the flag). Rewrote the bash loop example to match the actual argparse signature so a copy-paste invocation actually runs. 2. The documented pending_hitl envelope was 5 fields (version, pipeline_id, timestamp, decision, answer) but the driver writes 9 (adds status, result, error, answer_log). Replaced the truncated schema with the full envelope and added per-field semantics (especially the status field, which is the skill's loop predicate: pending / answered / completed / aborted / error). The slice-3 daemon variant inherits all 9 fields. 3. No documented mechanism for the skill body to write pending_hitl.answer (the frontmatter allowed-tools does not include the Write tool). Documented option (a) from the reviewer's NACK: an inline python3 -c "..." invocation, which is covered by the existing Bash(python3 *:*) allowed-tool. The "skill loop" code block now demonstrates the round-trip with a case statement keyed on pending_hitl.status. Non-blocking polish in SKILL.md: - Loop semantics now name "replay" explicitly (the driver spawns a fresh generator and replays answer_log on every invocation; previous text suggested cheap single-step resumption). Added a dedicated "Generator state across invocations (replay semantics)" subsection naming the practical consequence — side effects re-run every call — and pointing at slice 3 as the daemon-variant escape hatch. - Failure-mode bullet for "pending_hitl.decision == null" replaced with the more general "pending_hitl.status ∈ {completed, aborted, error}" bullet so each terminal state has documented diagnostic guidance. Non-blocking polish in docs/architecture/claude-code-substrate.md: - The "Flattened bridge" bullet in the in-process orchestrator section now names the replay path explicitly (promotes answer → answer_log, replays the full log every call, deterministic same-yield-boundary property) and the 5→9 envelope field list mirrors SKILL.md. - Daemon-variant bullet enumerates the same 9-field shape so reviewers comparing slice-1 and slice-3 against the ADR see the full contract. - Schema source-of-truth pointer added to bin/run_pipeline.py:20-46 so future drift triggers fail in one place. Rubric files (reviewer_refine.md, reviewer_agent_design.md) are unchanged — reviewer_code ACKed them in v1. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test_bridge_flattened_round_trip: fix subprocess PYTHONPATH The subprocess shim's PYTHONPATH pointed at `<repo>/orchestrator/` and `<repo>/shared/` directly, which lets the subprocess `import substrate` but NOT `import orchestrator.substrate` — the latter requires the *parent* of `orchestrator/` (the repo root) on the path because ``orchestrator/__init__.py`` makes it a real package. Set PYTHONPATH to ``<repo>/shared`` + ``<repo>`` + ``<repo>/gateway`` so: - ``<repo>/shared`` lets ``egg_contracts`` (transitive import from ``orchestrator.substrate.k3s_adapter``) resolve. - ``<repo>`` lets ``import orchestrator`` resolve. Refs #2717 (slice-1 tester). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * slice-1 tester v2: fix subprocess PYTHONPATH + non-blocking improvements Addresses reviewer_code v1 blocking #1 + non-blocking items: BLOCKING FIX — subprocess PYTHONPATH: Add `<repo>/orchestrator` to the subprocess shim's PYTHONPATH so bare- name top-level imports inside the `orchestrator/` tree resolve cleanly. Without this, `orchestrator/models.py:16` (`from slice_id_validation import SLICE_ID_PATTERN`) and `in_process.py:531-534`'s bare `from models import HITLDecision` fallback both fail, crashing the driver subprocess with `ModuleNotFoundError` before it yields the first HITL decision. Mirrors the Makefile's `PYTHONPATH := shared:gateway: orchestrator` (test target). Verified: tests now pass with `PYTHONPATH=.:shared:orchestrator pytest <files>` (reviewer_code's exact reproduction env). NON-BLOCKING (reviewer_code v1): - test_bridge_flattened_round_trip.py: drop stale "whichever the coder picks" docstring phrasing — driver locked in positional argv[1]. - test_bridge_flattened_round_trip.py: mirror the driver's ISO-8601 UTC timestamp format in _write_answer instead of `str(time.time())`. - test_rubric_loader.py: extend `test_loader_accepts_enum_and_string_role` parametrization to cover the two NEW roles (reviewer_refine, reviewer_agent_design) — not just the regression role. - test_rubric_loader.py: strengthen path-traversal assertion to verify the allowlist's slice-fence branch fires (not the file-missing-on- disk branch) — pinning the structural defence. - test_pretooluse_hook_nested.py: derive the r2-verdict.json content from the dispatch outcome and write it BEFORE the structured assertions so slice-5 sees an accurate empirical record even when a regression fails one of the assertions. Adds {"r2_verdict": "fail", "reason": ...} payload format for the failure path. Tests verified: 21/21 pass under both the canonical Makefile PYTHONPATH shape and reviewer_code's `.:shared:orchestrator` reproduction shape. Refs #2717 (slice-1 tester, v2). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Persist BRC history for slice-1 (#2548) * docs(#2717 slice-2): plan-team rubrics + SKILL.md plan-phase section Land the four plan-team agent rubric files under plugins/egg-sdlc/skills/egg-sdlc/agents/ for the claude-code substrate of the egg SDLC pipeline (task-2-3): architect, task_planner, risk_analyst, reviewer_plan. Each rubric mirrors its k3s-substrate counterpart in plugins/refine-plan/skills/refine-plan/agents/ for body content (the substrate swap is structurally invisible to the role) and follows the reviewer_refine.md / reviewer_agent_design.md shape from slice-1 for the substrate-specific notes (worktree layout, PreToolUse hook enforcement, HITL-via-AskUserQuestion, concurrent peers in this slice, output path stability). Update plugins/egg-sdlc/skills/egg-sdlc/SKILL.md (task-2-7): - Bump the rollout-status callout from "slice 1 landed" to "slices 1 + 2 landed"; enumerate both the refine and plan rosters. - Replace the "What's NOT in this skill > Plan / implement / pr" bullet's plan deferral with a dedicated **Plan phase** subsection naming the four roles, their spawn order (architect solo, then task_planner + risk_analyst concurrently, with reviewer_plan ACK/NACK on each producer edge), output paths, and the four standard plan-HITL gate options (approve / request_changes / change_approach / stop). - Bump step 8 (phase fence) into a 10-step flow that walks the plan stage spawn order and the plan-HITL gate. The fence now triggers on "approve and continue to implement" with a pointer to slice 3. - Refresh stale "refine-only" / "refine-team subagents" / artifact-path and failure-mode strings to cover both phases. * slice-2 coder: plan-phase BRC stage + rubric loader expansion (#2717) Implements TASK-2-1 + TASK-2-2 for slice-2 of the #2717 rollout. TASK-2-5 closes as no-op per slice-1's R2 = pass verdict (the PreToolUse hook resolves the child's role correctly under nested dispatch; structural enforcement stays hook-side, no MCP-validator-side parallel layer needed). TASK-2-1 — `_run_plan_phase` on `_InProcessOrchestrator` ======================================================== After the refine HITL gate's `approve_continue` answer, the in-process generator now dispatches the plan phase: a `ThreadPoolExecutor` spawns architect / task_planner / risk_analyst concurrently through the same `ClaudeCodeSpawner` the refiner uses, then reviewer_plan is dispatched once with the producer artifacts as its input. `PeerConsensusTracker` drives the BRC mechanics (`handle_propose` / `handle_ack` / `handle_confirmed`); after consensus the stage yields a plan-HITL gate (`HITLDecision` with `phase="plan"` and the canonical 4-way options). The walking-skeleton fence still fires on `approve_continue` past the plan gate — its diagnostic now points at slice-3 / slice-4 of the #2717 rollout instead of #2623. Why the orchestrator records BRC transitions on the subagents' behalf: the in-process substrate's spawner is synchronous (returns AFTER the agent finishes). In the production HTTP daemon the subagents would emit `egg-orch consensus propose/ack/confirmed` themselves and the daemon's gateway listener would advance the tracker. In-process the spawn-completion IS the signal that the subagent proposed or reviewed, so the orchestrator drives the BRC transitions deterministically — the test (harness-faked subagents that never emit BRC messages) and production (real harness agents whose emissions would be no-op duplicates in this path) both reach CONSENSUS_CONFIRMED on the same code path. TASK-2-2 — `_load_egg_sdlc_role_rubric` extension ================================================== `_RUBRIC_LANDED_ROLES` now includes architect / task_planner / risk_analyst / reviewer_plan alongside the slice-1 refine roster (refiner + reviewer_refine + reviewer_agent_design). The structured- error contract for unshipped roles is preserved: implement-team roles (coder / tester / documenter + 5 reviewers) still raise `ValueError` with a slice-3 pointer. The "missing on disk" fallback diagnostic mentions both TASK-1-4 (slice-1 refine) and TASK-2-3 (slice-2 plan) so a reviewer hitting the error in a re-run knows which documenter task needs to land first. TASK-2-5 — agent-side restriction enforcement (no-op) ====================================================== Slice-1's `test_pretooluse_hook_denies_nested_child_write` confirmed the PreToolUse hook denies a child write outside the child's role under nested dispatch (R2 = pass, recorded in `.egg-state/<pipeline_id>/r2-verdict.json` when the test runs). Per the contingent task description, no `sandbox/egg_agent_tools/handlers/restrictions.py` change is needed; structural enforcement stays hook-side. Tester's TASK-2-6 becomes a regression guard asserting the validator helper is a no-op for in-allow-list writes — handled in tester's slice-2 commit. Smoke (manual, in-process, fake subagents) ========================================== * preflight → refine gate → plan gate sequence yields the expected decisions; spawner is called 5 times (1 refiner + 3 plan producers + 1 plan reviewer); tracker.evaluate() reports is_complete=True with all 4 plan-team agents in CONFIRMED state. * Terminal answer at refine gate (e.g. "stop") still returns the refine artifact path — plan phase is NOT entered. * `approve_continue` at the plan gate still raises `NotImplementedError` with the slice-3 / slice-4 pointer. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * slice-2 tester: plan-phase BRC E2E + R2-pass restrictions no-op (#2717) TASK-2-4 — integration_tests/regression/test_inprocess_plan_brc.py ================================================================== Plan-phase in-process BRC end-to-end test covering the four AC bullets: * boots `run_pipeline_in_process` against a deterministic pipeline id with harness-faked subagents (no real Anthropic / Claude Code spawn); * advances past the refine HITL gate via `approve` → `approve_continue`; * asserts the plan stage spawns 3 producers (architect, task_planner, risk_analyst) + 1 reviewer (reviewer_plan) — observed via the fake spawner's `.call_args_list`; * asserts the BRC mechanics reach CONSENSUS_CONFIRMED on every producer edge (architect → reviewer_plan, task_planner → reviewer_plan, risk_analyst → reviewer_plan) by reading `_plan_tracker.evaluate()` — the in-process analogue of bus-side CONSENSUS_CONFIRMED messages (the coder's TASK-2-1 implementation drives `PeerConsensusTracker.handle_propose/handle_ack/ handle_confirmed` deterministically since the substrate's spawner is synchronous); * asserts the plan-HITL decision is yielded with `phase="plan"`, `decision_type="phase_gate"`, non-empty `id` / `question` / `options`. Adversarial probing layered on top: * plan stage MUST NOT run when the operator answers `stop` at the refine gate — a regression that fanned into plan on any non-continue answer would burn three unauthorised subagent spawns; * plan stage MUST NOT spawn implement-phase roles — pins the negative invariant against a misrouted `_PHASE_ROLES` lookup; * refiner is spawned exactly once — pins the single-refiner-spawn invariant against an off-by-one role iteration; * every plan-phase spawn carries `EGG_PHASE=plan` in its env — pins the env-propagation contract so spawned subagents see the right phase. The test skips gracefully when the coder's `_run_plan_phase` is absent (scaffold-first per the role's guidance); 7/7 pass against the coder's slice-2 commit 3a46689. TASK-2-6 — tests/sandbox/egg_agent_tools/test_restrictions_validator.py ======================================================================= Contingent test per slice-1's R2 verdict = `pass`. Per the contract task-2-5 description, "If R2 = pass, this task is a no-op (close with note). Tests for this code path land in TASK-2-6 (tester-owned)." Tests for this code path land here as a **no-op regression guard**: * in-allow-list response shape stable (coder/orchestrator, tester/ tests, documenter/docs) — pins the documented gateway-shape fields `{ok, role, path, can_write, reason, alternative_role}` exactly; * cross-role denial shape stable — pins `can_write=False`, denial `reason` references `shared/egg_restrictions/patterns.py`, `alternative_role` names the single producer that can write; * no new validator symbol — asserts `validate_write_target` (and peers) are NOT present on the restrictions handler module, since R2 = pass meant the cq-6 option-2 enforcement work should NOT have landed; * defensive probes — missing `path` raises HandlerError, unknown role raises HandlerError, list-shaped path returns per-path results with documented shapes. 9/9 pass against the unchanged restrictions handler (no slice-2 source edits in `sandbox/egg_agent_tools/handlers/restrictions.py`). Configured-check results: * ruff check . — PASS (all checks passed) * ruff format check . — FAILS on `orchestrator/substrate/in_process.py` (coder's TASK-2-1 file, 5 long-call sites need re-formatting). My test files pass format check cleanly. This is being NACKed to the coder; my proposal will follow once they push the format fix. * mypy on tester-authored files — PASS (251 source files OK). * Custom checks (scripts/check-*.py) — all 13 pass. * `make lint` / `make test` / `make security` cannot complete in this sandbox: the venv sync fails when uv tries to download pinned wheels (flask, oauthlib) — the wheels.pythonhosted.org TLS chain is "UnknownIssuer" inside the sandbox image (same env constraint the slice-1 tester hit). Tests + lint + custom checks were exercised directly via system pytest / ruff / mypy with the Makefile's canonical `PYTHONPATH := shared:gateway:orchestrator`. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * slice-2 coder v2: address NACK blockers (#2717) Addresses 3 NACK reviewers from v1 (commit 3a46689): reviewer_concurrency NACKs: - C1: removed `_write_active_role_sentinel` from `_spawn_plan_producer`'s concurrent path. Each producer carries `EGG_AGENT_ROLE` in its own spawn env (the load-bearing role-resolution channel under concurrent dispatch); the single-valued sentinel cannot disambiguate three concurrent role-holders. The synchronous `_spawn_plan_reviewer` retains the sentinel write because it never overlaps another spawn. - C2: added `self._current_phase` state on `_InProcessOrchestrator` (default "refine"; flipped to "plan" at the top of `_run_plan_phase`). `_publish_heartbeat` reads from it so HEARTBEAT messages carry the right phase across the refine→plan transition. Without this, stuck-phase-transition watchdogs filtering by `phase` would see "refine" while the plan stage is actively running. reviewer_code_holistic NACKs: - H1: architect-first then fanout. `_run_plan_phase_inner` now spawns architect synchronously first, records its CONSENSUS_PROPOSE on the tracker, then fans out task_planner + risk_analyst concurrently through a ThreadPoolExecutor with max_workers=2. The architect's per-role output path is passed into each downstream producer's spawn env (`EGG_ARCHITECT_OUTPUT_PATH`) and prompt_text so they can read its `key_design_decisions` rather than re-deriving them. This matches the role-dependency declarations at `shared/egg_contracts/agent_roles.py:398/422` (TASK_PLANNER_ROLE / RISK_ANALYST_ROLE both list ARCHITECT as their sole dependency) and the architect / task_planner / risk_analyst rubric bodies the documenter shipped. - H2: reviewer_plan verdict-JSON parsing. New helpers `read_plan_reviewer_verdicts` (parses `.egg-state/agent-outputs/<issue>-reviewer_plan-output.json`) and `_apply_reviewer_verdicts` drive per-edge ACK / NACK on the tracker based on the reviewer's actual verdict rather than the exit-code-only heuristic v1 used. Fail-closed when the verdict file is missing AND the reviewer's spawn failed (NACK every edge); optimistic ACK only when the verdict file is missing AND the reviewer's spawn returned exit 0 (harness-faked test path), with the "verdict-not-parsed" status surfaced in the placeholder body so the operator sees the discrepancy at the HITL gate. tester NACK: - T1: ran `ruff format` on the affected files. `_spawn_plan_reviewer` also dropped the dead `EGG_PRODUCER_ARTIFACT_PATHS` env var (reviewer_code_holistic v1 non-blocking #3) in favor of per-role `EGG_<ROLE>_OUTPUT_PATH` env vars that the reviewer_plan rubric actually consumes. Non-blocker polish landed alongside the blockers: - `_synthetic_commit_for(role)` derives a per-role hex SHA so the three concurrent ProposalPayload entries remain commit-distinguishable in the tracker (reviewer_concurrency v1 NB #2). - Tracker-guard rejections (`handle_propose` / `handle_ack` / `handle_nack` / `handle_confirmed`) now log via `logging.getLogger("orchestrator.substrate.in_process").warning` instead of silent `except Exception: pass` (reviewer_code_holistic v1 NB). - `_format_plan_placeholder` now also renders reviewer_plan diagnostics + verdict-parsing status (reviewer_code_holistic v1 NB). File decomposition: - ruff format expanded the v1 diff to 1879 lines, breaching the 1500-line hard cap in `scripts/file-size-allowlist.yaml`. Extracted the plan-phase body (~700 lines) into `orchestrator/substrate/_plan_phase.py` as module-level functions that take the `_InProcessOrchestrator` instance as their first argument. The class's `_run_plan_phase` / `_spawn_plan_producer` / `_spawn_plan_reviewer` / `_plan_producer_output_path` / `_read_plan_reviewer_verdicts` methods stay on the class as thin delegates so the existing test surface (and tester's 16 passing tests against v1) keeps the same method names. `in_process.py` now lands at 1093 lines (under both caps); `_plan_phase.py` at 680 lines. Manual in-process smoke (harness fakes, MagicMock subagents): - Happy path: preflight → refine gate → plan gate; spawner called 5 times in order [refiner, architect, task_planner|risk_analyst, task_planner|risk_analyst, reviewer_plan]; tracker reaches `is_complete=True`. - Refine stop: returns refine artifact path; spawner called 1 time (no plan dispatch). - Mixed verdict: with a per_producer verdict JSON {architect:ACK, task_planner:NACK, risk_analyst:ACK}, the tracker records the NACK on task_planner → reviewer_plan; `is_complete=False`; blocking_agents includes reviewer_plan (unresolved critical NACK) and task_planner (not fully ACKed). - Fail-closed: with reviewer spawn exit_code=1 and no verdict file, the tracker NACKs every critical edge; risk_analyst (advisory edge) still confirms; reviewer_plan blocks. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Address slice-1 review: fix install path, bridge answer-write, silent fallbacks Reviewer findings from PR #2724 (egg-reviewer slice-1 review): Blockers (1-4): * The documented `pip install -r requirements.txt` failed — no top-level requirements.txt exists. Switch SKILL.md, plugin.json's `egg.install_instructions`, and `bin/preflight.py` to `pip install .` against pyproject.toml. * SKILL.md claimed Python 3.11+ but pyproject.toml requires >=3.14. Update SKILL.md and preflight to reflect the real floor; correct the matching docstring in orchestrator/substrate/__init__.py. * The inline `python3 -c "..."` answer-write snippet in SKILL.md was broken (shell-interpolated `${ANSWER}` → NameError, deprecated `datetime.utcnow`, non-atomic write). Replace with a dedicated `bin/write_answer.py` helper that reads the JSON-encoded answer from stdin, uses `datetime.now(UTC)`, and writes atomically via tmp + `os.replace`. SKILL.md's loop now invokes the helper. * `_serialise_decision` silently swallowed `model_dump` failures, and `_read_contract` silently overwrote unparseable contracts (dropping `answer_log`). Log to stderr on serialisation fallbacks; refuse to overwrite a corrupted contract and exit 1 instead. Non-blocking (5-11): * `orchestrator/substrate/__init__.py` docstring rewritten to reflect reality: Python 3.14+ introduced PEP 758 (`except A, B:` without parens); ruff under py314 target strips redundant parens, hence `# fmt: skip` on multi-except lines. * Stderr warning in `_advance_generator`'s `finally` so a teardown failure inside `generator.close()` is at least observable. * `allowed-tools` tightened from `Bash(python3 *:*)` to `Bash(python3 plugins/egg-sdlc/skills/egg-sdlc/bin/*:*)` — the two helper scripts are the entire Python surface the skill can invoke. * Re-spawn cost note expanded in SKILL.md: each driver invocation re-runs every prior subagent spawn (real Anthropic API spend); slice-2 compounds to 8 spawns at the final stage. * `_RUBRIC_LANDED_ROLES` registry collapsed into `_CURRENT_LOADER_SLICE` + filesystem probe; same diagnostics, no parallel registry that can drift from disk state. * Abort vocabulary exported as `ABORT_ANSWERS` from `orchestrator.substrate.in_process`; the driver imports it (with a literal fallback when the orchestrator package is unimportable) so the driver, orchestrator, and slice-3 daemon share a single source of truth. * R2 caveat surfaced as an explicit open question for slice-5 sequencing in SKILL.md. Tests: * New `shared/tests/test_write_answer.py` (6 tests, all passing) pins the JSON-encoding round-trip, the timestamp format match against the driver, the atomic-write contract, and the corrupted-contract refusal. * Existing `test_rubric_loader`, `test_run_pipeline_in_process*`, `test_substrate_interfaces`, `test_bridge_flattened_round_trip`, and `test_pretooluse_hook_nested` all still pass. * The pre-existing `test_empty_diff_subprocess_skips_pytest` failure in `tests/tools/` reproduces against HEAD without these changes (detached-HEAD worktree edge case in the test selector). Authored-by: egg * slice-2 coder v4: support rubric-default single-verdict JSON schema (#2717) Addresses reviewer_code_holistic v3 NACK blocker H3 — the rubric the documenter shipped (plugins/egg-sdlc/skills/egg-sdlc/agents/reviewer_plan.md "Verdict JSON shape", lines 57-80) documents a single top-level verdict object (verdict ∈ {ACK, NACK}, analysis carrying the eight criteria, feedback blob, artifact_references), not the per_producer wrapper v2/v3's parser expected. A rubric-following reviewer's NACK would silently fall into the "verdict file present but no parseable per_producer entries" branch and the orchestrator's optimistic-ACK fallback would mask the NACK from the operator at the plan-HITL gate. v4 makes `read_plan_reviewer_verdicts` accept BOTH schemas: 1. Rubric-default single-verdict (broadcast). When the JSON's top-level `verdict` is "ACK" or "NACK", the verdict is broadcast to every plan producer edge — ACK acks all three, NACK nacks all three with `feedback` propagated as the per-edge `reason` (a synthetic placeholder fires if `feedback` is empty so the tracker's NACK guard doesn't reject the payload). This is "Option (c)" from the v3 NACK; per-edge granularity is lost but the rubric's "ACK only if every criterion passes" semantic IS preserved. 2. Per-producer extension (per-edge). The existing per_producer wrapper still takes precedence when present and well-formed. Reviewers that want explicit edge granularity (ACK architect + NACK task_planner) write the wrapper; the rubric's default shape stays broadcast-compatible. The function now takes an optional `plan_producers` kwarg so the caller (the in-process orchestrator) can broadcast the single verdict to the right role set. The `_read_plan_reviewer_verdicts` class method delegate also propagates the kwarg so tester-side tests that call the method retain their access pattern. Smoke (manual, in-process, MagicMock subagents): - Rubric-default single-verdict NACK: tracker NACKs architect + task_planner (critical edges), risk_analyst still confirms (advisory), reviewer_plan blocks consensus. is_complete=False; blocking_agents=['architect', 'task_planner', 'reviewer_plan']. - Rubric-default single-verdict ACK: every edge confirmed; is_complete=True. - per_producer wrapper still works: mixed ACK/NACK applied per edge. - Harness-fake path (no verdict file, reviewer exit 0): optimistic ACK preserved so tester's existing 16 passing tests keep their access pattern. - Fail-closed path (no verdict file, reviewer exit non-zero): critical edges NACK'd (unchanged from v2/v3). ruff format + ruff check + file-size lint all pass. `_plan_phase.py` is 747 lines; `in_process.py` 1095 lines. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Persist BRC history for slice-2 (#2548) * Move skill-loop python3 -c calls into bin/ helpers Address review feedback on PR #2724: - Add bin/read_status.py and extend write_answer.py with --answer-string so every subcommand in SKILL.md's loop body is a single `python3 plugins/.../bin/<helper>.py` invocation. Honors the tightened allowed-tools pattern `Bash(python3 plugins/.../bin/*:*)` per Claude Code's compound-command permission rules — no separate `Bash(python3 -c *)` or `Bash(printf *)` rule needed, no prompt-injection door left open. - Update SKILL.md step 4 to name bin/write_answer.py directly (matches the new loop body). - Replace `slice_hint != _CURRENT_LOADER_SLICE` rubric-loader fence with `slice_hint not in _LANDED_SLICES` (frozenset) so future slices extend rather than replace the landed set — slice-2 won't fence off slice-1's already-shipped refine roles. - Wire test_bridge_flattened_round_trip's _write_answer through subprocess(write_answer.py) so the integration test exercises the production write path end-to-end. - Add test_read_status.py (7 tests) and --answer-string coverage in test_write_answer.py (2 tests). * Update slice-1 rubric loader tests to match slice-2's loader expansion Slice-1's recent tester commits (831239d / 601df90) added tests pinning 'architect raises ValueError' and 'reviewer_plan/task_planner deferred'. Slice-2's loader extension to the plan team (task-2-2 + task-2-3) makes those roles loadable, so the slice-1 tests fail after the merge. This commit aligns the tests with slice-2's loader reality: - Replace test_load_architect_raises_value_error_with_slice2_hint with test_load_architect_rubric, mirroring the slice-1 success-path tests. - Remove REVIEWER_PLAN and TASK_PLANNER from test_loader_still_rejects_unshipped_roles parameters; keep REVIEWER_CODE (slice-3, still deferred). - Refresh module docstring to reflect architect-loads (vs architect-raises). * Address non-blocking review notes on PR #2724 - write_answer.py: clarify --answer-string docstring — the JSON encoding happens at contract serialisation time (json.dumps(contract)), not as a separate json.dumps(answer) step. Reference the special-characters test as the proof of the round-trip. - SKILL.md / read_status.py: document the case statement's intentional fall-through on empty STATUS. read_status.py prints empty + exit 0 when no pending_hitl envelope exists; the case has no *) arm, so the empty value falls through, the case exits 0, and the outer iteration re-invokes run_pipeline.py — which is the recover path. - test_rubric_loader.py: add test_landed_slices_contains_slice1 to mechanically pin the 'extend, don't replace' invariant on _LANDED_SLICES so a future slice cannot silently regress slice-1 by writing frozenset({'slice-2'}) instead of frozenset({'slice-1', 'slice-2'}). * Address slice-2 review: phase plumbing, doc drift, defensive checks Addresses reviewer_code feedback on PR #2726 (#2717 slice-2): B1 (blocking): thread `phase` through `_write_pending_decision` and `current_phase` so plan-gate decisions persist with `phase: "plan"` instead of the hardcoded `"refine"` left over from the spike. Regression test pins the persisted-vs-yielded phase invariant. B2 (blocking) + N1 + N2 + N3 (SKILL.md doc drift): - Replace plain `approve` with the canonical `approve_continue` so operators following the docs trip the fence instead of silently completing. - Trim overclaim that slice-2 implements `request_changes` / `change_approach` re-spawn loops (it doesn't — they're surfaced but treated as stop). - Document the failure-path `retry` / `abort` option set. - Update the NotImplementedError quote to match the actual raise. N4: delete dead `_SYNTHETIC_PLAN_COMMIT` (no callers — real producers route through `synthetic_commit_for(role)`); fold the "never escape this constant" caveat into `synthetic_commit_for`'s docstring. N5: document the `per_producer` extension shape in `reviewer_plan.md` so reviewers who need per-edge granularity have the documented opt-in instead of guessing. N6: drop unused `pre_merge_condition` plumbing from the plan-phase verdict reader — pre-merge conditions are a PR-merge concept with no consumer in plan-phase. N7 + N8: unlink `<contract>.lock` after the critical section and bound `fcntl.flock` with `LOCK_EX | LOCK_NB` + a 30 s retry deadline so crashed lock-holders surface as `BlockingIOError` instead of hanging the orchestrator forever. N9: defensive `architect_output_path.is_file()` check before the downstream fan-out; surface the broken handoff as a NACK on the architect edge so the operator sees the partial state at the plan-HITL gate instead of debugging chained downstream errors. N10: clear the active-role sentinel at the start of the plan phase so the PreToolUse hook's fallback path doesn't resolve concurrent plan-producers to the stale `refiner` role. N11: drop the misleading `patch.object(restrictions, "get_agent_role", ...)` in `test_unknown_role_raises_handler_error` — `check_file_restriction` short-circuits on the truthy `req["role"]` so the patch never fired; the test still pins the real invariant without the misleading scaffolding. * Address slice-2 v2 review: NB1-NB4 (N9 fail-fast, flock pattern) --------- Co-authored-by: james-in-a-box[bot] <246424927+james-in-a-box[bot]@users.noreply.github.com> Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> Co-authored-by: egg <egg@example.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> Co-authored-by: egg-orchestrator <egg@localhost> Co-authored-by: James Wiesebron <jameswiesebron@khanacademy.org>
jwbron
added a commit
that referenced
this pull request
May 20, 2026
* Initialize SDLC contract for issue #2717
* refine: analysis for #2717 substrate-swap follow-up rollout
Surface 7 multi-choice decisions (cq-1..cq-7) and 6 open-ended feedback
questions covering the bridge-gap design, slice decomposition, Agent-
tool dispatcher migration, R15 model-(b) timing, R2 hook validation
timing, cost cap default, and k3s adapter scope.
* Persist agent statefile writes before refine sync
* Persist statefiles after refine phase
* Persist HITL resolution after refine phase gate
* plan: architect analysis for #2717 substrate-swap follow-up rollout
Maps refine-phase HITL decisions (cq-1 through cq-7 + feedback Q1-Q6)
onto concrete component changes across 5 slices:
1. Bridge gap closure (Option B stage-script MVP) + R2 hook
role-resolution spike (2-subagent worked example).
2. Plan-phase substrate (architect/task_planner/risk_analyst + reviewer_plan).
3. Implement-phase substrate (coder/tester/documenter + 5 reviewers; cq-3
empirical metrics collected here).
4. PR-phase substrate + 5x2 conformance matrix + scope-fence removal.
5. Parallel hardening: EggHarnessSpawner + local-run CLI, cost cap
(EGG_PIPELINE_MAX_AGENT_INVOCATIONS=200), drop v0.x markers, ADR refresh.
Includes runtime-primitive surfacing per #2594: every cited primitive has
file:line evidence and is tagged with purpose (deployed-pod vs test-only)
and execution context (in-sandbox-agent vs trusted-CI-runner).
* plan: risk assessment for #2717 substrate-swap follow-up rollout
Adds risk_analyst output (.egg-state/agent-outputs/2717-risk_analyst-output.json)
covering 18 risks (R17–R34) specific to the post-spike rollout that wires the
remaining 15 roles + plan/implement/pr phases onto the Claude Code substrate.
Key risks called out:
- R17: HITL bridge dual-architecture (cq-1 Option C-hybrid)
- R18: 15-rubric authorship + structural depth-gap closure
- R19/R29: 8-way harness re-host stress on parent session (cq-3 deferred)
- R20: existing reviewer rubrics need substrate-aware extension (Q5 declined)
- R21: 5-issue conformance reproducibility (Q1 fixed set)
- R22: #2261 slice-15 coordination
- R23: cost-cap at 200 (cq-6) needs visibility
- R26: EggHarnessSpawner as 3rd protocol implementation (Q3 Option A)
- R27: MCP-validator fallback structural enforcement gap
- R31: 15-subagent trust-context scaling (Q4 declined extras)
- 11 implementation recommendations with priorities + open questions for
implement-phase HITL.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* plan: 5-slice DAG for #2717 substrate-swap follow-up rollout
Decompose the rollout into the phase-sequential chain settled by
the refine HITL (cq-2 = Option 3):
slice-1 (bridge gap + R2 hook validation + refine reviewers)
-> slice-2 (plan-phase substrate)
-> slice-3 (implement-phase substrate + daemon HITL bridge)
-> slice-4 (pr-phase + 5-issue conformance matrix +
scope-fence removal)
-> slice-5 (hardening: cost cap + EggHarnessSpawner +
R15 contingent + fork primitive + ADR +
v0.x marker drop)
Each slice has exactly one DAG parent (forest constraint per #2137
satisfied). 42 tasks across the five slices; primitives audit per
#2594 cites every named symbol with file:line or marks (NEW —
TASK-X-Y). Trust-boundary scope is named: conformance tests live
under integration_tests/regression/ (substrate-portable), not
integration_tests/local_pipeline/ (kubectl-gated).
* plan v2: address reviewer_plan v1 NACK (3 blockers + non-blockers)
Blocking fixes:
- TASK-1-5 (R2 spike): the harness re-host model bypasses the
PreToolUse hook entirely (shared/egg_harness/client.py uses its
own ToolRegistry.set_permission_callback, no hook_entry import).
Add TASK-1-9 introducing a test-only nested-Agent-tool dispatch
fake at integration_tests/regression/_agent_tool_fake.py
(underscored helper => coder-owned per MCP file-restriction
check). Reframe TASK-1-5 to use the fake; document the empirical-
vs-test-fake limitation in the test docstring. Production stays
on ClaudeCodeSpawner harness re-host per cq-3.
- TASK-4-4 (conformance matrix): switch from "recorded transcripts
that no task produces" to MagicMock-style stubs mirroring
test_substrate_smoke.py:56. Document the trade-off in the test
docstring and note that #2714's closed state is irrelevant per
feedback Q1.
- TASK-4-2 (fence removal): cite both :212 (call site) and :807-826
(method def) so the coder removes both, not just the call.
Non-blocking fixes:
- TASK-2-5: agent-side enforcement target moved from
orchestrator/mcp_tools.py (wrong surface) to
sandbox/egg_agent_tools/handlers/restrictions.py (the in-sandbox
tool handler that exposes check_file_restriction at :70 today).
- TASK-2-6 / TASK-2-7: renumbered to match file order.
- TASK-1-6: explicit dependency note on TASK-1-4.
- TASK-3-2: daemon must detach via start_new_session=True so it
survives the calling Bash exit.
- TASK-1-1: pending_hitl envelope marked as the shared state-
serialization contract between Option B (flattened) and Option A
(daemon), closing risk_analyst R17 dual-bridge concern.
- TASK-5-5 fork primitive: stays on harness re-host (subprocess +
egg_harness.run_agent) instead of Agent-tool dispatch, aligning
with cq-3's "decide empirically post-implement" deferral.
- Primitives table: LocalWorktreeManager line corrected to :59;
_maybe_fence dual-location citation added.
* Populate contract for 2717 (#2629)
* Persist statefiles after plan phase
* [slice-1] Roll out Claude Code substrate to remaining roles + plan/... (#2724)
* docs: add claude-code substrate to index and structure docs [doc-updater] (#2718)
* docs: add claude-code substrate to index and structure docs
* docs: fix substrate/claude_code listing per reviewer feedback
- Drop incorrect '+ Agent tool' from spawner.py description; the spike
runs egg_harness.run_agent in-process and does NOT dispatch via the
Agent tool (Agent-tool spawner is an ADR follow-up).
- Add hook_entry.py to the listing — it is the standalone PreToolUse
hook script and the largest file in the package (~31 KB).
- Clarify policy.py is the PolicyEnforcer adapter wrapping hook_entry.py.
- List settings.template.json for navigability.
Addresses egg-reviewer CHANGES_REQUESTED on PR #2718.
---------
Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com>
Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>
* docs: update deployment guide for Cilium portmap CNI changes [doc-updater] (#2716)
* docs: document portmap CNI and iptables reboot requirement (#2713)
* docs: mention netfilter-persistent as alternative to re-running after reboot
Addresses non-blocking review feedback on #2716. The reviewer noted that
install-cilium.sh's own comment block calls out netfilter-persistent /
iptables-restore as a system-level persistence alternative to re-running
the script after every reboot. Mirror that in the operator-facing doc so
long-running k3s host operators know they have an option beyond manual
re-runs.
---------
Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com>
Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>
* docs: add reconcile_autostash_pop_conflict to push diagnostic list (#2720)
Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com>
* slice-1 coder: bridge driver + R2 nested-dispatch fake + loader expansion
Implements three #2717 slice-1 coder tasks toward the substrate-swap
follow-up rollout per cq-1=Option C hybrid bridge (refine/plan
flattened, implement daemon), cq-2 phase-sequential slicing, cq-5
early-spike R2 validation, and cq-3 harness re-host alignment.
TASK-1-1 (bin/run_pipeline.py): flattened single-yield stage driver
that advances `run_pipeline_in_process` to its next yield,
serialises the yielded HITLDecision to .egg-state/contracts/<id>.json
under a stable `pending_hitl` envelope schema (version, decision,
answer, answer_log, status, result, error), and exits. Cross-process
generator state is recovered by replaying `answer_log` on each
invocation — viable for refine/plan because the generator is
deterministic; slice-3's daemon variant (TASK-3-2) consumes the same
envelope schema so the two bridges share a state-serialization
contract (risk_analyst R17 mitigation). End-to-end round-trip
verified: first invocation yields preflight decision; operator
answer round-trips through the contract; second invocation replays
and advances to the refine-gate decision.
TASK-1-9 (integration_tests/regression/_agent_tool_fake.py): test-
only nested-Agent-tool dispatch fake. Simulates Claude Code's Agent
tool by spawning a child subprocess with controlled EGG_AGENT_ROLE;
the child invokes orchestrator/substrate/claude_code/hook_entry.py
`decide(...)` directly. Validates the hook-logic half of R2 — given
accurate EGG_AGENT_ROLE propagation, does the hook deny a write that
violates the *child's* role pattern even when the parent's role
would allow it? Hard import guard prevents production use; the file
is coder-owned (underscored helper name, mirroring _helpers.py)
rather than tester-owned. Production dispatch stays on
ClaudeCodeSpawner (harness re-host) per cq-3.
TASK-1-6 (orchestrator/substrate/__init__.py): extends
`_load_egg_sdlc_role_rubric` so reviewer_refine and
reviewer_agent_design are recognised as supported (alongside the
existing refiner). Introduces a `_ROLE_RUBRIC_SLICES` mapping that
names which #2717 rollout slice ships each role's rubric (slice-1:
refine team; slice-2: plan team; slice-3: implement team) so future
slice loaders can extend the set declaratively, and a
`_RUBRIC_LANDED_ROLES` set documenting which rubric .md files exist
on disk today. Roles outside the landed set raise a structured
ValueError citing the correct rollout slice ("deferred to follow-up
slice-2 of issue #2717's rollout"). The acceptance criterion's
"follow-up slice 2" hint for architect is now produced.
Lint clean (ruff check + format); mypy clean on the new files; the
pre-existing 6 mypy errors in substrate/__init__.py:180-198 are
unrelated to this change. 52 existing claude-code-substrate tests
still pass.
Refs #2717 (slice-1 coder).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* docs(#2717 slice-1): refine-team rubrics + flattened-bridge docs + ADR rollout deltas
Slice 1 of the #2717 substrate-swap rollout adds two refine-team reviewer
rubrics, closes the heredoc-HITL bridge gap for refine-phase via a flattened
bin/run_pipeline.py stage driver, and adds the cq-5 R2 spike for nested
PreToolUse-hook role-routing. This commit lands the documenter-owned half:
TASK-1-4: New reviewer rubric files at
plugins/egg-sdlc/skills/egg-sdlc/agents/reviewer_refine.md
plugins/egg-sdlc/skills/egg-sdlc/agents/reviewer_agent_design.md
mirroring the layout of plugins/refine-plan/skills/refine-plan/agents/ so the
in-process orchestrator's build_system_prompt(sources) loader picks them up
without per-skill custom logic. Both files carry frontmatter (name +
description) and the body documents the substrate-specific operational
deltas (worktree path, PreToolUse-enforced restrictions, AskUserQuestion
surfacing, verdict path) on top of the same rubric the k3s reviewers use.
TASK-1-2: SKILL.md is updated to replace the "Walking-skeleton bridge gap"
callout with a "How the flattened bridge works" section that names
pending_hitl as the single-yield carrier and documents the skill→driver
loop. The "What the skill is designed to do" step list moves from aspirational
to actually-shipping. The R2 PreToolUse-hook section points at the new test
infrastructure (test_pretooluse_hook_nested.py + _agent_tool_fake.py) and
the r2-verdict.json file. Frontmatter description re-flows to reflect the
slice-1 scope.
TASK-1-8: docs/architecture/claude-code-substrate.md is updated for the
ADR-level audit trail:
- Title and status banner reframe from "spike" to "spike → rollout".
- cq-2 / cq-7 / cq-11 table rows reflect what slice 1 lands.
- The in-process orchestrator section gets a "The flattened bridge"
subsection naming the cq-1 hybrid (Option C) and the slice-3 daemon
variant that consumes the same pending_hitl envelope shape (R17 mitigation).
- The egg-sdlc plugin section enumerates the three refine-team rubrics
and the new bin/run_pipeline.py driver.
- The R2 risk-mitigation subsection points at the slice-1 worked example
and the slice-5 contingent fallback (cq-6 option 2 + R15 model (b)).
- The R15 subsection makes the model (a) → (b) migration contingent on
the slice-1 R2 verdict.
- The "Open work" + "Follow-up issue draft" sections are replaced with a
unified "Rollout deltas" section split into Completed-in-this-rollout
(3 slice-1 items, marked with [x] + strikethrough on the obsolete text)
and Pending-in-this-rollout (9 items mapped to slices 2-5). The acceptance
bar is unchanged.
- The primitives table picks up the four new slice-1 modules
(bin/run_pipeline.py, _agent_tool_fake.py, test_pretooluse_hook_nested.py,
the two new reviewer rubrics).
- The conformance-proof section names the slice-1 regression-test
additions (test_bridge_flattened_round_trip.py, test_rubric_loader.py).
- Stale anchor links to the removed "Follow-up issue draft" section are
redirected to the new "Rollout deltas" anchor.
These doc changes satisfy TASK-1-2, TASK-1-4, and TASK-1-8 from slice 1 of
the #2717 plan; no source or test files are touched.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* slice-1 tester: rubric loader + bridge round-trip + R2 nested-dispatch tests
Adds three test files covering #2717 slice-1's tester contract tasks:
TASK-1-7 (shared/tests/test_rubric_loader.py): unit tests for
`_load_egg_sdlc_role_rubric`. Pins the four AC cases: refiner
regression, reviewer_refine load, reviewer_agent_design load, architect
raises ValueError with the updated "follow-up slice 2" diagnostic.
Adversarial probing layered on: AgentRole enum vs str input
equivalence, defense against path-traversal role values, structured
fence for unshipped plan-phase roles (reviewer_plan, reviewer_code,
task_planner). Eight of 10 tests pass today; two (reviewer_refine,
reviewer_agent_design loads) are documenter-dependency failures that
flip green once task-1-4 lands the rubric .md files.
TASK-1-3 (integration_tests/regression/test_bridge_flattened_round_trip.py):
end-to-end round-trip test for the flattened bridge driver. Runs the
real `bin/run_pipeline.py` in a fresh subprocess twice against a
deterministic pipeline id: stage A captures the preflight HITLDecision
into `pending_hitl.decision`, the test writes `answer="approve" +
status="answered"`, stage B re-enters the process and replays the
answer to advance to the refine-gate decision. Validates that the
generator state survives via the contract-state round-trip across
process exit. Substrate isolation via a `-c` shim that monkey-patches
`orchestrator.substrate.select_substrate` to a MagicMock bundle —
no real Claude Code / Anthropic API call. Also pins driver-side
idempotency (a re-invocation without a new answer must not silently
advance the generator).
TASK-1-5 (integration_tests/regression/test_pretooluse_hook_nested.py):
cq-5 early-spike R2 verdict test. Uses task-1-9's `_agent_tool_fake`
to drive a deterministic nested-dispatch scenario: parent_role=architect
+ child_role=tester + write_target=orchestrator/foo.py — asserts the
hook returns `{"decision": "block", "reason": ...}` and that the
deny reason names the child (tester) role rather than the parent.
Writes `.egg-state/<pipeline_id>/r2-verdict.json` with the pass
verdict per AC. Adds in-role allow control + cross-role probe
(parent=coder, child=tester writing orchestrator/* — must deny by
the child's role) + dataclass shape pin + EGG_AGENT_ROLE leak guard.
Docstring documents the empirical-vs-test-fake limitation cq-3
explicitly accepts (production stays on the harness re-host until
slice-5 R15 flips dispatch).
All 15 of 17 tests pass today. The 2 failing rubric tests are
contracted documenter-dependency failures (task-1-4 not landed yet)
and are expected to flip green once the documenter ships.
Lint clean (ruff check + format).
Refs #2717 (slice-1 tester).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* docs(#2717 slice-1): address reviewer_code v1 NACK on SKILL.md envelope + CLI
Reviewer_code NACKed v1 with three blockers against SKILL.md (the two
rubric files and the ADR were ACKed as-is). This commit addresses all
three blockers plus four non-blocking polish items.
Blocking fixes in SKILL.md:
1. CLI invocation example was wrong (used --pipeline-id / --issue, but the
driver at plugins/egg-sdlc/skills/egg-sdlc/bin/run_pipeline.py:355-402
takes pipeline_id as a positional arg and --issue-number as the flag).
Rewrote the bash loop example to match the actual argparse signature
so a copy-paste invocation actually runs.
2. The documented pending_hitl envelope was 5 fields (version,
pipeline_id, timestamp, decision, answer) but the driver writes 9
(adds status, result, error, answer_log). Replaced the truncated
schema with the full envelope and added per-field semantics
(especially the status field, which is the skill's loop predicate:
pending / answered / completed / aborted / error). The slice-3
daemon variant inherits all 9 fields.
3. No documented mechanism for the skill body to write
pending_hitl.answer (the frontmatter allowed-tools does not include
the Write tool). Documented option (a) from the reviewer's NACK: an
inline python3 -c "..." invocation, which is covered by the existing
Bash(python3 *:*) allowed-tool. The "skill loop" code block now
demonstrates the round-trip with a case statement keyed on
pending_hitl.status.
Non-blocking polish in SKILL.md:
- Loop semantics now name "replay" explicitly (the driver spawns a
fresh generator and replays answer_log on every invocation; previous
text suggested cheap single-step resumption). Added a dedicated
"Generator state across invocations (replay semantics)" subsection
naming the practical consequence — side effects re-run every call —
and pointing at slice 3 as the daemon-variant escape hatch.
- Failure-mode bullet for "pending_hitl.decision == null" replaced with
the more general "pending_hitl.status ∈ {completed, aborted, error}"
bullet so each terminal state has documented diagnostic guidance.
Non-blocking polish in docs/architecture/claude-code-substrate.md:
- The "Flattened bridge" bullet in the in-process orchestrator section
now names the replay path explicitly (promotes answer → answer_log,
replays the full log every call, deterministic same-yield-boundary
property) and the 5→9 envelope field list mirrors SKILL.md.
- Daemon-variant bullet enumerates the same 9-field shape so reviewers
comparing slice-1 and slice-3 against the ADR see the full contract.
- Schema source-of-truth pointer added to bin/run_pipeline.py:20-46 so
future drift triggers fail in one place.
Rubric files (reviewer_refine.md, reviewer_agent_design.md) are
unchanged — reviewer_code ACKed them in v1.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* test_bridge_flattened_round_trip: fix subprocess PYTHONPATH
The subprocess shim's PYTHONPATH pointed at `<repo>/orchestrator/` and
`<repo>/shared/` directly, which lets the subprocess `import substrate`
but NOT `import orchestrator.substrate` — the latter requires the
*parent* of `orchestrator/` (the repo root) on the path because
``orchestrator/__init__.py`` makes it a real package.
Set PYTHONPATH to ``<repo>/shared`` + ``<repo>`` + ``<repo>/gateway`` so:
- ``<repo>/shared`` lets ``egg_contracts`` (transitive import from
``orchestrator.substrate.k3s_adapter``) resolve.
- ``<repo>`` lets ``import orchestrator`` resolve.
Refs #2717 (slice-1 tester).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* slice-1 tester v2: fix subprocess PYTHONPATH + non-blocking improvements
Addresses reviewer_code v1 blocking #1 + non-blocking items:
BLOCKING FIX — subprocess PYTHONPATH:
Add `<repo>/orchestrator` to the subprocess shim's PYTHONPATH so bare-
name top-level imports inside the `orchestrator/` tree resolve cleanly.
Without this, `orchestrator/models.py:16` (`from slice_id_validation
import SLICE_ID_PATTERN`) and `in_process.py:531-534`'s bare `from
models import HITLDecision` fallback both fail, crashing the driver
subprocess with `ModuleNotFoundError` before it yields the first HITL
decision. Mirrors the Makefile's `PYTHONPATH := shared:gateway:
orchestrator` (test target). Verified: tests now pass with
`PYTHONPATH=.:shared:orchestrator pytest <files>` (reviewer_code's
exact reproduction env).
NON-BLOCKING (reviewer_code v1):
- test_bridge_flattened_round_trip.py: drop stale "whichever the coder
picks" docstring phrasing — driver locked in positional argv[1].
- test_bridge_flattened_round_trip.py: mirror the driver's ISO-8601
UTC timestamp format in _write_answer instead of `str(time.time())`.
- test_rubric_loader.py: extend `test_loader_accepts_enum_and_string_role`
parametrization to cover the two NEW roles (reviewer_refine,
reviewer_agent_design) — not just the regression role.
- test_rubric_loader.py: strengthen path-traversal assertion to verify
the allowlist's slice-fence branch fires (not the file-missing-on-
disk branch) — pinning the structural defence.
- test_pretooluse_hook_nested.py: derive the r2-verdict.json content
from the dispatch outcome and write it BEFORE the structured
assertions so slice-5 sees an accurate empirical record even when
a regression fails one of the assertions. Adds {"r2_verdict":
"fail", "reason": ...} payload format for the failure path.
Tests verified: 21/21 pass under both the canonical Makefile PYTHONPATH
shape and reviewer_code's `.:shared:orchestrator` reproduction shape.
Refs #2717 (slice-1 tester, v2).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* Persist BRC history for slice-1 (#2548)
* Address slice-1 review: fix install path, bridge answer-write, silent fallbacks
Reviewer findings from PR #2724 (egg-reviewer slice-1 review):
Blockers (1-4):
* The documented `pip install -r requirements.txt` failed — no top-level
requirements.txt exists. Switch SKILL.md, plugin.json's
`egg.install_instructions`, and `bin/preflight.py` to `pip install .`
against pyproject.toml.
* SKILL.md claimed Python 3.11+ but pyproject.toml requires >=3.14.
Update SKILL.md and preflight to reflect the real floor; correct the
matching docstring in orchestrator/substrate/__init__.py.
* The inline `python3 -c "..."` answer-write snippet in SKILL.md was
broken (shell-interpolated `${ANSWER}` → NameError, deprecated
`datetime.utcnow`, non-atomic write). Replace with a dedicated
`bin/write_answer.py` helper that reads the JSON-encoded answer from
stdin, uses `datetime.now(UTC)`, and writes atomically via
tmp + `os.replace`. SKILL.md's loop now invokes the helper.
* `_serialise_decision` silently swallowed `model_dump` failures, and
`_read_contract` silently overwrote unparseable contracts (dropping
`answer_log`). Log to stderr on serialisation fallbacks; refuse to
overwrite a corrupted contract and exit 1 instead.
Non-blocking (5-11):
* `orchestrator/substrate/__init__.py` docstring rewritten to reflect
reality: Python 3.14+ introduced PEP 758 (`except A, B:` without
parens); ruff under py314 target strips redundant parens, hence
`# fmt: skip` on multi-except lines.
* Stderr warning in `_advance_generator`'s `finally` so a teardown
failure inside `generator.close()` is at least observable.
* `allowed-tools` tightened from `Bash(python3 *:*)` to
`Bash(python3 plugins/egg-sdlc/skills/egg-sdlc/bin/*:*)` — the two
helper scripts are the entire Python surface the skill can invoke.
* Re-spawn cost note expanded in SKILL.md: each driver invocation
re-runs every prior subagent spawn (real Anthropic API spend); slice-2
compounds to 8 spawns at the final stage.
* `_RUBRIC_LANDED_ROLES` registry collapsed into `_CURRENT_LOADER_SLICE`
+ filesystem probe; same diagnostics, no parallel registry that can
drift from disk state.
* Abort vocabulary exported as `ABORT_ANSWERS` from
`orchestrator.substrate.in_process`; the driver imports it (with a
literal fallback when the orchestrator package is unimportable) so
the driver, orchestrator, and slice-3 daemon share a single source
of truth.
* R2 caveat surfaced as an explicit open question for slice-5
sequencing in SKILL.md.
Tests:
* New `shared/tests/test_write_answer.py` (6 tests, all passing) pins
the JSON-encoding round-trip, the timestamp format match against the
driver, the atomic-write contract, and the corrupted-contract refusal.
* Existing `test_rubric_loader`, `test_run_pipeline_in_process*`,
`test_substrate_interfaces`, `test_bridge_flattened_round_trip`, and
`test_pretooluse_hook_nested` all still pass.
* The pre-existing `test_empty_diff_subprocess_skips_pytest` failure
in `tests/tools/` reproduces against HEAD without these changes
(detached-HEAD worktree edge case in the test selector).
Authored-by: egg
* Move skill-loop python3 -c calls into bin/ helpers
Address review feedback on PR #2724:
- Add bin/read_status.py and extend write_answer.py with --answer-string
so every subcommand in SKILL.md's loop body is a single
`python3 plugins/.../bin/<helper>.py` invocation. Honors the tightened
allowed-tools pattern `Bash(python3 plugins/.../bin/*:*)` per Claude
Code's compound-command permission rules — no separate
`Bash(python3 -c *)` or `Bash(printf *)` rule needed, no
prompt-injection door left open.
- Update SKILL.md step 4 to name bin/write_answer.py directly (matches
the new loop body).
- Replace `slice_hint != _CURRENT_LOADER_SLICE` rubric-loader fence
with `slice_hint not in _LANDED_SLICES` (frozenset) so future slices
extend rather than replace the landed set — slice-2 won't fence off
slice-1's already-shipped refine roles.
- Wire test_bridge_flattened_round_trip's _write_answer through
subprocess(write_answer.py) so the integration test exercises the
production write path end-to-end.
- Add test_read_status.py (7 tests) and --answer-string coverage in
test_write_answer.py (2 tests).
* Address non-blocking review notes on PR #2724
- write_answer.py: clarify --answer-string docstring — the JSON encoding
happens at contract serialisation time (json.dumps(contract)), not as
a separate json.dumps(answer) step. Reference the special-characters
test as the proof of the round-trip.
- SKILL.md / read_status.py: document the case statement's intentional
fall-through on empty STATUS. read_status.py prints empty + exit 0
when no pending_hitl envelope exists; the case has no *) arm, so the
empty value falls through, the case exits 0, and the outer iteration
re-invokes run_pipeline.py — which is the recover path.
- test_rubric_loader.py: add test_landed_slices_contains_slice1 to
mechanically pin the 'extend, don't replace' invariant on
_LANDED_SLICES so a future slice cannot silently regress slice-1 by
writing frozenset({'slice-2'}) instead of frozenset({'slice-1',
'slice-2'}).
---------
Co-authored-by: james-in-a-box[bot] <246424927+james-in-a-box[bot]@users.noreply.github.com>
Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com>
Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>
Co-authored-by: egg <egg@example.com>
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Co-authored-by: egg-orchestrator <egg@localhost>
* [slice-2] Roll out Claude Code substrate to remaining roles + plan/... (#2726)
* docs: add claude-code substrate to index and structure docs [doc-updater] (#2718)
* docs: add claude-code substrate to index and structure docs
* docs: fix substrate/claude_code listing per reviewer feedback
- Drop incorrect '+ Agent tool' from spawner.py description; the spike
runs egg_harness.run_agent in-process and does NOT dispatch via the
Agent tool (Agent-tool spawner is an ADR follow-up).
- Add hook_entry.py to the listing — it is the standalone PreToolUse
hook script and the largest file in the package (~31 KB).
- Clarify policy.py is the PolicyEnforcer adapter wrapping hook_entry.py.
- List settings.template.json for navigability.
Addresses egg-reviewer CHANGES_REQUESTED on PR #2718.
---------
Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com>
Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>
* docs: update deployment guide for Cilium portmap CNI changes [doc-updater] (#2716)
* docs: document portmap CNI and iptables reboot requirement (#2713)
* docs: mention netfilter-persistent as alternative to re-running after reboot
Addresses non-blocking review feedback on #2716. The reviewer noted that
install-cilium.sh's own comment block calls out netfilter-persistent /
iptables-restore as a system-level persistence alternative to re-running
the script after every reboot. Mirror that in the operator-facing doc so
long-running k3s host operators know they have an option beyond manual
re-runs.
---------
Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com>
Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>
* docs: add reconcile_autostash_pop_conflict to push diagnostic list (#2720)
Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com>
* slice-1 coder: bridge driver + R2 nested-dispatch fake + loader expansion
Implements three #2717 slice-1 coder tasks toward the substrate-swap
follow-up rollout per cq-1=Option C hybrid bridge (refine/plan
flattened, implement daemon), cq-2 phase-sequential slicing, cq-5
early-spike R2 validation, and cq-3 harness re-host alignment.
TASK-1-1 (bin/run_pipeline.py): flattened single-yield stage driver
that advances `run_pipeline_in_process` to its next yield,
serialises the yielded HITLDecision to .egg-state/contracts/<id>.json
under a stable `pending_hitl` envelope schema (version, decision,
answer, answer_log, status, result, error), and exits. Cross-process
generator state is recovered by replaying `answer_log` on each
invocation — viable for refine/plan because the generator is
deterministic; slice-3's daemon variant (TASK-3-2) consumes the same
envelope schema so the two bridges share a state-serialization
contract (risk_analyst R17 mitigation). End-to-end round-trip
verified: first invocation yields preflight decision; operator
answer round-trips through the contract; second invocation replays
and advances to the refine-gate decision.
TASK-1-9 (integration_tests/regression/_agent_tool_fake.py): test-
only nested-Agent-tool dispatch fake. Simulates Claude Code's Agent
tool by spawning a child subprocess with controlled EGG_AGENT_ROLE;
the child invokes orchestrator/substrate/claude_code/hook_entry.py
`decide(...)` directly. Validates the hook-logic half of R2 — given
accurate EGG_AGENT_ROLE propagation, does the hook deny a write that
violates the *child's* role pattern even when the parent's role
would allow it? Hard import guard prevents production use; the file
is coder-owned (underscored helper name, mirroring _helpers.py)
rather than tester-owned. Production dispatch stays on
ClaudeCodeSpawner (harness re-host) per cq-3.
TASK-1-6 (orchestrator/substrate/__init__.py): extends
`_load_egg_sdlc_role_rubric` so reviewer_refine and
reviewer_agent_design are recognised as supported (alongside the
existing refiner). Introduces a `_ROLE_RUBRIC_SLICES` mapping that
names which #2717 rollout slice ships each role's rubric (slice-1:
refine team; slice-2: plan team; slice-3: implement team) so future
slice loaders can extend the set declaratively, and a
`_RUBRIC_LANDED_ROLES` set documenting which rubric .md files exist
on disk today. Roles outside the landed set raise a structured
ValueError citing the correct rollout slice ("deferred to follow-up
slice-2 of issue #2717's rollout"). The acceptance criterion's
"follow-up slice 2" hint for architect is now produced.
Lint clean (ruff check + format); mypy clean on the new files; the
pre-existing 6 mypy errors in substrate/__init__.py:180-198 are
unrelated to this change. 52 existing claude-code-substrate tests
still pass.
Refs #2717 (slice-1 coder).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* docs(#2717 slice-1): refine-team rubrics + flattened-bridge docs + ADR rollout deltas
Slice 1 of the #2717 substrate-swap rollout adds two refine-team reviewer
rubrics, closes the heredoc-HITL bridge gap for refine-phase via a flattened
bin/run_pipeline.py stage driver, and adds the cq-5 R2 spike for nested
PreToolUse-hook role-routing. This commit lands the documenter-owned half:
TASK-1-4: New reviewer rubric files at
plugins/egg-sdlc/skills/egg-sdlc/agents/reviewer_refine.md
plugins/egg-sdlc/skills/egg-sdlc/agents/reviewer_agent_design.md
mirroring the layout of plugins/refine-plan/skills/refine-plan/agents/ so the
in-process orchestrator's build_system_prompt(sources) loader picks them up
without per-skill custom logic. Both files carry frontmatter (name +
description) and the body documents the substrate-specific operational
deltas (worktree path, PreToolUse-enforced restrictions, AskUserQuestion
surfacing, verdict path) on top of the same rubric the k3s reviewers use.
TASK-1-2: SKILL.md is updated to replace the "Walking-skeleton bridge gap"
callout with a "How the flattened bridge works" section that names
pending_hitl as the single-yield carrier and documents the skill→driver
loop. The "What the skill is designed to do" step list moves from aspirational
to actually-shipping. The R2 PreToolUse-hook section points at the new test
infrastructure (test_pretooluse_hook_nested.py + _agent_tool_fake.py) and
the r2-verdict.json file. Frontmatter description re-flows to reflect the
slice-1 scope.
TASK-1-8: docs/architecture/claude-code-substrate.md is updated for the
ADR-level audit trail:
- Title and status banner reframe from "spike" to "spike → rollout".
- cq-2 / cq-7 / cq-11 table rows reflect what slice 1 lands.
- The in-process orchestrator section gets a "The flattened bridge"
subsection naming the cq-1 hybrid (Option C) and the slice-3 daemon
variant that consumes the same pending_hitl envelope shape (R17 mitigation).
- The egg-sdlc plugin section enumerates the three refine-team rubrics
and the new bin/run_pipeline.py driver.
- The R2 risk-mitigation subsection points at the slice-1 worked example
and the slice-5 contingent fallback (cq-6 option 2 + R15 model (b)).
- The R15 subsection makes the model (a) → (b) migration contingent on
the slice-1 R2 verdict.
- The "Open work" + "Follow-up issue draft" sections are replaced with a
unified "Rollout deltas" section split into Completed-in-this-rollout
(3 slice-1 items, marked with [x] + strikethrough on the obsolete text)
and Pending-in-this-rollout (9 items mapped to slices 2-5). The acceptance
bar is unchanged.
- The primitives table picks up the four new slice-1 modules
(bin/run_pipeline.py, _agent_tool_fake.py, test_pretooluse_hook_nested.py,
the two new reviewer rubrics).
- The conformance-proof section names the slice-1 regression-test
additions (test_bridge_flattened_round_trip.py, test_rubric_loader.py).
- Stale anchor links to the removed "Follow-up issue draft" section are
redirected to the new "Rollout deltas" anchor.
These doc changes satisfy TASK-1-2, TASK-1-4, and TASK-1-8 from slice 1 of
the #2717 plan; no source or test files are touched.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* slice-1 tester: rubric loader + bridge round-trip + R2 nested-dispatch tests
Adds three test files covering #2717 slice-1's tester contract tasks:
TASK-1-7 (shared/tests/test_rubric_loader.py): unit tests for
`_load_egg_sdlc_role_rubric`. Pins the four AC cases: refiner
regression, reviewer_refine load, reviewer_agent_design load, architect
raises ValueError with the updated "follow-up slice 2" diagnostic.
Adversarial probing layered on: AgentRole enum vs str input
equivalence, defense against path-traversal role values, structured
fence for unshipped plan-phase roles (reviewer_plan, reviewer_code,
task_planner). Eight of 10 tests pass today; two (reviewer_refine,
reviewer_agent_design loads) are documenter-dependency failures that
flip green once task-1-4 lands the rubric .md files.
TASK-1-3 (integration_tests/regression/test_bridge_flattened_round_trip.py):
end-to-end round-trip test for the flattened bridge driver. Runs the
real `bin/run_pipeline.py` in a fresh subprocess twice against a
deterministic pipeline id: stage A captures the preflight HITLDecision
into `pending_hitl.decision`, the test writes `answer="approve" +
status="answered"`, stage B re-enters the process and replays the
answer to advance to the refine-gate decision. Validates that the
generator state survives via the contract-state round-trip across
process exit. Substrate isolation via a `-c` shim that monkey-patches
`orchestrator.substrate.select_substrate` to a MagicMock bundle —
no real Claude Code / Anthropic API call. Also pins driver-side
idempotency (a re-invocation without a new answer must not silently
advance the generator).
TASK-1-5 (integration_tests/regression/test_pretooluse_hook_nested.py):
cq-5 early-spike R2 verdict test. Uses task-1-9's `_agent_tool_fake`
to drive a deterministic nested-dispatch scenario: parent_role=architect
+ child_role=tester + write_target=orchestrator/foo.py — asserts the
hook returns `{"decision": "block", "reason": ...}` and that the
deny reason names the child (tester) role rather than the parent.
Writes `.egg-state/<pipeline_id>/r2-verdict.json` with the pass
verdict per AC. Adds in-role allow control + cross-role probe
(parent=coder, child=tester writing orchestrator/* — must deny by
the child's role) + dataclass shape pin + EGG_AGENT_ROLE leak guard.
Docstring documents the empirical-vs-test-fake limitation cq-3
explicitly accepts (production stays on the harness re-host until
slice-5 R15 flips dispatch).
All 15 of 17 tests pass today. The 2 failing rubric tests are
contracted documenter-dependency failures (task-1-4 not landed yet)
and are expected to flip green once the documenter ships.
Lint clean (ruff check + format).
Refs #2717 (slice-1 tester).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* docs(#2717 slice-1): address reviewer_code v1 NACK on SKILL.md envelope + CLI
Reviewer_code NACKed v1 with three blockers against SKILL.md (the two
rubric files and the ADR were ACKed as-is). This commit addresses all
three blockers plus four non-blocking polish items.
Blocking fixes in SKILL.md:
1. CLI invocation example was wrong (used --pipeline-id / --issue, but the
driver at plugins/egg-sdlc/skills/egg-sdlc/bin/run_pipeline.py:355-402
takes pipeline_id as a positional arg and --issue-number as the flag).
Rewrote the bash loop example to match the actual argparse signature
so a copy-paste invocation actually runs.
2. The documented pending_hitl envelope was 5 fields (version,
pipeline_id, timestamp, decision, answer) but the driver writes 9
(adds status, result, error, answer_log). Replaced the truncated
schema with the full envelope and added per-field semantics
(especially the status field, which is the skill's loop predicate:
pending / answered / completed / aborted / error). The slice-3
daemon variant inherits all 9 fields.
3. No documented mechanism for the skill body to write
pending_hitl.answer (the frontmatter allowed-tools does not include
the Write tool). Documented option (a) from the reviewer's NACK: an
inline python3 -c "..." invocation, which is covered by the existing
Bash(python3 *:*) allowed-tool. The "skill loop" code block now
demonstrates the round-trip with a case statement keyed on
pending_hitl.status.
Non-blocking polish in SKILL.md:
- Loop semantics now name "replay" explicitly (the driver spawns a
fresh generator and replays answer_log on every invocation; previous
text suggested cheap single-step resumption). Added a dedicated
"Generator state across invocations (replay semantics)" subsection
naming the practical consequence — side effects re-run every call —
and pointing at slice 3 as the daemon-variant escape hatch.
- Failure-mode bullet for "pending_hitl.decision == null" replaced with
the more general "pending_hitl.status ∈ {completed, aborted, error}"
bullet so each terminal state has documented diagnostic guidance.
Non-blocking polish in docs/architecture/claude-code-substrate.md:
- The "Flattened bridge" bullet in the in-process orchestrator section
now names the replay path explicitly (promotes answer → answer_log,
replays the full log every call, deterministic same-yield-boundary
property) and the 5→9 envelope field list mirrors SKILL.md.
- Daemon-variant bullet enumerates the same 9-field shape so reviewers
comparing slice-1 and slice-3 against the ADR see the full contract.
- Schema source-of-truth pointer added to bin/run_pipeline.py:20-46 so
future drift triggers fail in one place.
Rubric files (reviewer_refine.md, reviewer_agent_design.md) are
unchanged — reviewer_code ACKed them in v1.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* test_bridge_flattened_round_trip: fix subprocess PYTHONPATH
The subprocess shim's PYTHONPATH pointed at `<repo>/orchestrator/` and
`<repo>/shared/` directly, which lets the subprocess `import substrate`
but NOT `import orchestrator.substrate` — the latter requires the
*parent* of `orchestrator/` (the repo root) on the path because
``orchestrator/__init__.py`` makes it a real package.
Set PYTHONPATH to ``<repo>/shared`` + ``<repo>`` + ``<repo>/gateway`` so:
- ``<repo>/shared`` lets ``egg_contracts`` (transitive import from
``orchestrator.substrate.k3s_adapter``) resolve.
- ``<repo>`` lets ``import orchestrator`` resolve.
Refs #2717 (slice-1 tester).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* slice-1 tester v2: fix subprocess PYTHONPATH + non-blocking improvements
Addresses reviewer_code v1 blocking #1 + non-blocking items:
BLOCKING FIX — subprocess PYTHONPATH:
Add `<repo>/orchestrator` to the subprocess shim's PYTHONPATH so bare-
name top-level imports inside the `orchestrator/` tree resolve cleanly.
Without this, `orchestrator/models.py:16` (`from slice_id_validation
import SLICE_ID_PATTERN`) and `in_process.py:531-534`'s bare `from
models import HITLDecision` fallback both fail, crashing the driver
subprocess with `ModuleNotFoundError` before it yields the first HITL
decision. Mirrors the Makefile's `PYTHONPATH := shared:gateway:
orchestrator` (test target). Verified: tests now pass with
`PYTHONPATH=.:shared:orchestrator pytest <files>` (reviewer_code's
exact reproduction env).
NON-BLOCKING (reviewer_code v1):
- test_bridge_flattened_round_trip.py: drop stale "whichever the coder
picks" docstring phrasing — driver locked in positional argv[1].
- test_bridge_flattened_round_trip.py: mirror the driver's ISO-8601
UTC timestamp format in _write_answer instead of `str(time.time())`.
- test_rubric_loader.py: extend `test_loader_accepts_enum_and_string_role`
parametrization to cover the two NEW roles (reviewer_refine,
reviewer_agent_design) — not just the regression role.
- test_rubric_loader.py: strengthen path-traversal assertion to verify
the allowlist's slice-fence branch fires (not the file-missing-on-
disk branch) — pinning the structural defence.
- test_pretooluse_hook_nested.py: derive the r2-verdict.json content
from the dispatch outcome and write it BEFORE the structured
assertions so slice-5 sees an accurate empirical record even when
a regression fails one of the assertions. Adds {"r2_verdict":
"fail", "reason": ...} payload format for the failure path.
Tests verified: 21/21 pass under both the canonical Makefile PYTHONPATH
shape and reviewer_code's `.:shared:orchestrator` reproduction shape.
Refs #2717 (slice-1 tester, v2).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* Persist BRC history for slice-1 (#2548)
* docs(#2717 slice-2): plan-team rubrics + SKILL.md plan-phase section
Land the four plan-team agent rubric files under
plugins/egg-sdlc/skills/egg-sdlc/agents/ for the claude-code substrate
of the egg SDLC pipeline (task-2-3): architect, task_planner,
risk_analyst, reviewer_plan. Each rubric mirrors its k3s-substrate
counterpart in plugins/refine-plan/skills/refine-plan/agents/ for body
content (the substrate swap is structurally invisible to the role) and
follows the reviewer_refine.md / reviewer_agent_design.md shape from
slice-1 for the substrate-specific notes (worktree layout, PreToolUse
hook enforcement, HITL-via-AskUserQuestion, concurrent peers in this
slice, output path stability).
Update plugins/egg-sdlc/skills/egg-sdlc/SKILL.md (task-2-7):
- Bump the rollout-status callout from "slice 1 landed" to
"slices 1 + 2 landed"; enumerate both the refine and plan rosters.
- Replace the "What's NOT in this skill > Plan / implement / pr"
bullet's plan deferral with a dedicated **Plan phase** subsection
naming the four roles, their spawn order (architect solo, then
task_planner + risk_analyst concurrently, with reviewer_plan ACK/NACK
on each producer edge), output paths, and the four standard
plan-HITL gate options (approve / request_changes / change_approach /
stop).
- Bump step 8 (phase fence) into a 10-step flow that walks the plan
stage spawn order and the plan-HITL gate. The fence now triggers on
"approve and continue to implement" with a pointer to slice 3.
- Refresh stale "refine-only" / "refine-team subagents" / artifact-path
and failure-mode strings to cover both phases.
* slice-2 coder: plan-phase BRC stage + rubric loader expansion (#2717)
Implements TASK-2-1 + TASK-2-2 for slice-2 of the #2717 rollout. TASK-2-5
closes as no-op per slice-1's R2 = pass verdict (the PreToolUse hook
resolves the child's role correctly under nested dispatch; structural
enforcement stays hook-side, no MCP-validator-side parallel layer
needed).
TASK-2-1 — `_run_plan_phase` on `_InProcessOrchestrator`
========================================================
After the refine HITL gate's `approve_continue` answer, the in-process
generator now dispatches the plan phase: a `ThreadPoolExecutor` spawns
architect / task_planner / risk_analyst concurrently through the same
`ClaudeCodeSpawner` the refiner uses, then reviewer_plan is dispatched
once with the producer artifacts as its input. `PeerConsensusTracker`
drives the BRC mechanics (`handle_propose` / `handle_ack` /
`handle_confirmed`); after consensus the stage yields a plan-HITL
gate (`HITLDecision` with `phase="plan"` and the canonical 4-way
options). The walking-skeleton fence still fires on
`approve_continue` past the plan gate — its diagnostic now points at
slice-3 / slice-4 of the #2717 rollout instead of #2623.
Why the orchestrator records BRC transitions on the subagents' behalf:
the in-process substrate's spawner is synchronous (returns AFTER the
agent finishes). In the production HTTP daemon the subagents would
emit `egg-orch consensus propose/ack/confirmed` themselves and the
daemon's gateway listener would advance the tracker. In-process the
spawn-completion IS the signal that the subagent proposed or
reviewed, so the orchestrator drives the BRC transitions
deterministically — the test (harness-faked subagents that never
emit BRC messages) and production (real harness agents whose
emissions would be no-op duplicates in this path) both reach
CONSENSUS_CONFIRMED on the same code path.
TASK-2-2 — `_load_egg_sdlc_role_rubric` extension
==================================================
`_RUBRIC_LANDED_ROLES` now includes architect / task_planner /
risk_analyst / reviewer_plan alongside the slice-1 refine roster
(refiner + reviewer_refine + reviewer_agent_design). The structured-
error contract for unshipped roles is preserved: implement-team
roles (coder / tester / documenter + 5 reviewers) still raise
`ValueError` with a slice-3 pointer. The "missing on disk" fallback
diagnostic mentions both TASK-1-4 (slice-1 refine) and TASK-2-3
(slice-2 plan) so a reviewer hitting the error in a re-run knows
which documenter task needs to land first.
TASK-2-5 — agent-side restriction enforcement (no-op)
======================================================
Slice-1's `test_pretooluse_hook_denies_nested_child_write` confirmed
the PreToolUse hook denies a child write outside the child's role
under nested dispatch (R2 = pass, recorded in
`.egg-state/<pipeline_id>/r2-verdict.json` when the test runs).
Per the contingent task description, no
`sandbox/egg_agent_tools/handlers/restrictions.py` change is
needed; structural enforcement stays hook-side. Tester's TASK-2-6
becomes a regression guard asserting the validator helper is a no-op
for in-allow-list writes — handled in tester's slice-2 commit.
Smoke (manual, in-process, fake subagents)
==========================================
* preflight → refine gate → plan gate sequence yields the expected
decisions; spawner is called 5 times (1 refiner + 3 plan producers
+ 1 plan reviewer); tracker.evaluate() reports is_complete=True
with all 4 plan-team agents in CONFIRMED state.
* Terminal answer at refine gate (e.g. "stop") still returns the
refine artifact path — plan phase is NOT entered.
* `approve_continue` at the plan gate still raises
`NotImplementedError` with the slice-3 / slice-4 pointer.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* slice-2 tester: plan-phase BRC E2E + R2-pass restrictions no-op (#2717)
TASK-2-4 — integration_tests/regression/test_inprocess_plan_brc.py
==================================================================
Plan-phase in-process BRC end-to-end test covering the four AC bullets:
* boots `run_pipeline_in_process` against a deterministic pipeline id
with harness-faked subagents (no real Anthropic / Claude Code spawn);
* advances past the refine HITL gate via `approve` → `approve_continue`;
* asserts the plan stage spawns 3 producers (architect, task_planner,
risk_analyst) + 1 reviewer (reviewer_plan) — observed via the fake
spawner's `.call_args_list`;
* asserts the BRC mechanics reach CONSENSUS_CONFIRMED on every
producer edge (architect → reviewer_plan, task_planner →
reviewer_plan, risk_analyst → reviewer_plan) by reading
`_plan_tracker.evaluate()` — the in-process analogue of bus-side
CONSENSUS_CONFIRMED messages (the coder's TASK-2-1 implementation
drives `PeerConsensusTracker.handle_propose/handle_ack/
handle_confirmed` deterministically since the substrate's spawner
is synchronous);
* asserts the plan-HITL decision is yielded with `phase="plan"`,
`decision_type="phase_gate"`, non-empty `id` / `question` / `options`.
Adversarial probing layered on top:
* plan stage MUST NOT run when the operator answers `stop` at the
refine gate — a regression that fanned into plan on any non-continue
answer would burn three unauthorised subagent spawns;
* plan stage MUST NOT spawn implement-phase roles — pins the negative
invariant against a misrouted `_PHASE_ROLES` lookup;
* refiner is spawned exactly once — pins the single-refiner-spawn
invariant against an off-by-one role iteration;
* every plan-phase spawn carries `EGG_PHASE=plan` in its env — pins
the env-propagation contract so spawned subagents see the right
phase.
The test skips gracefully when the coder's `_run_plan_phase` is
absent (scaffold-first per the role's guidance); 7/7 pass against
the coder's slice-2 commit 3a466891e.
TASK-2-6 — tests/sandbox/egg_agent_tools/test_restrictions_validator.py
=======================================================================
Contingent test per slice-1's R2 verdict = `pass`. Per the contract
task-2-5 description, "If R2 = pass, this task is a no-op (close with
note). Tests for this code path land in TASK-2-6 (tester-owned)."
Tests for this code path land here as a **no-op regression guard**:
* in-allow-list response shape stable (coder/orchestrator, tester/
tests, documenter/docs) — pins the documented gateway-shape fields
`{ok, role, path, can_write, reason, alternative_role}` exactly;
* cross-role denial shape stable — pins `can_write=False`, denial
`reason` references `shared/egg_restrictions/patterns.py`,
`alternative_role` names the single producer that can write;
* no new validator symbol — asserts `validate_write_target` (and
peers) are NOT present on the restrictions handler module, since
R2 = pass meant the cq-6 option-2 enforcement work should NOT
have landed;
* defensive probes — missing `path` raises HandlerError, unknown
role raises HandlerError, list-shaped path returns per-path
results with documented shapes.
9/9 pass against the unchanged restrictions handler (no slice-2
source edits in `sandbox/egg_agent_tools/handlers/restrictions.py`).
Configured-check results:
* ruff check . — PASS (all checks passed)
* ruff format check . — FAILS on `orchestrator/substrate/in_process.py`
(coder's TASK-2-1 file, 5 long-call sites need re-formatting). My
test files pass format check cleanly. This is being NACKed to the
coder; my proposal will follow once they push the format fix.
* mypy on tester-authored files — PASS (251 source files OK).
* Custom checks (scripts/check-*.py) — all 13 pass.
* `make lint` / `make test` / `make security` cannot complete in
this sandbox: the venv sync fails when uv tries to download pinned
wheels (flask, oauthlib) — the wheels.pythonhosted.org TLS chain
is "UnknownIssuer" inside the sandbox image (same env constraint
the slice-1 tester hit). Tests + lint + custom checks were
exercised directly via system pytest / ruff / mypy with the
Makefile's canonical `PYTHONPATH := shared:gateway:orchestrator`.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* slice-2 coder v2: address NACK blockers (#2717)
Addresses 3 NACK reviewers from v1 (commit 3a466891e):
reviewer_concurrency NACKs:
- C1: removed `_write_active_role_sentinel` from `_spawn_plan_producer`'s
concurrent path. Each producer carries `EGG_AGENT_ROLE` in its own
spawn env (the load-bearing role-resolution channel under
concurrent dispatch); the single-valued sentinel cannot
disambiguate three concurrent role-holders. The synchronous
`_spawn_plan_reviewer` retains the sentinel write because it
never overlaps another spawn.
- C2: added `self._current_phase` state on `_InProcessOrchestrator`
(default "refine"; flipped to "plan" at the top of
`_run_plan_phase`). `_publish_heartbeat` reads from it so
HEARTBEAT messages carry the right phase across the refine→plan
transition. Without this, stuck-phase-transition watchdogs
filtering by `phase` would see "refine" while the plan stage is
actively running.
reviewer_code_holistic NACKs:
- H1: architect-first then fanout. `_run_plan_phase_inner` now
spawns architect synchronously first, records its
CONSENSUS_PROPOSE on the tracker, then fans out task_planner +
risk_analyst concurrently through a ThreadPoolExecutor with
max_workers=2. The architect's per-role output path is passed
into each downstream producer's spawn env
(`EGG_ARCHITECT_OUTPUT_PATH`) and prompt_text so they can read
its `key_design_decisions` rather than re-deriving them. This
matches the role-dependency declarations at
`shared/egg_contracts/agent_roles.py:398/422`
(TASK_PLANNER_ROLE / RISK_ANALYST_ROLE both list ARCHITECT as
their sole dependency) and the architect / task_planner /
risk_analyst rubric bodies the documenter shipped.
- H2: reviewer_plan verdict-JSON parsing. New helpers
`read_plan_reviewer_verdicts` (parses
`.egg-state/agent-outputs/<issue>-reviewer_plan-output.json`)
and `_apply_reviewer_verdicts` drive per-edge ACK / NACK on the
tracker based on the reviewer's actual verdict rather than the
exit-code-only heuristic v1 used. Fail-closed when the verdict
file is missing AND the reviewer's spawn failed (NACK every
edge); optimistic ACK only when the verdict file is missing AND
the reviewer's spawn returned exit 0 (harness-faked test path),
with the "verdict-not-parsed" status surfaced in the placeholder
body so the operator sees the discrepancy at the HITL gate.
tester NACK:
- T1: ran `ruff format` on the affected files. `_spawn_plan_reviewer`
also dropped the dead `EGG_PRODUCER_ARTIFACT_PATHS` env var
(reviewer_code_holistic v1 non-blocking #3) in favor of per-role
`EGG_<ROLE>_OUTPUT_PATH` env vars that the reviewer_plan rubric
actually consumes.
Non-blocker polish landed alongside the blockers:
- `_synthetic_commit_for(role)` derives a per-role hex SHA so the
three concurrent ProposalPayload entries remain
commit-distinguishable in the tracker
(reviewer_concurrency v1 NB #2).
- Tracker-guard rejections (`handle_propose` / `handle_ack` /
`handle_nack` / `handle_confirmed`) now log via
`logging.getLogger("orchestrator.substrate.in_process").warning`
instead of silent `except Exception: pass`
(reviewer_code_holistic v1 NB).
- `_format_plan_placeholder` now also renders reviewer_plan
diagnostics + verdict-parsing status (reviewer_code_holistic
v1 NB).
File decomposition:
- ruff format expanded the v1 diff to 1879 lines, breaching the
1500-line hard cap in `scripts/file-size-allowlist.yaml`.
Extracted the plan-phase body (~700 lines) into
`orchestrator/substrate/_plan_phase.py` as module-level
functions that take the `_InProcessOrchestrator` instance as
their first argument. The class's `_run_plan_phase` /
`_spawn_plan_producer` / `_spawn_plan_reviewer` /
`_plan_producer_output_path` / `_read_plan_reviewer_verdicts`
methods stay on the class as thin delegates so the existing
test surface (and tester's 16 passing tests against v1) keeps
the same method names. `in_process.py` now lands at 1093 lines
(under both caps); `_plan_phase.py` at 680 lines.
Manual in-process smoke (harness fakes, MagicMock subagents):
- Happy path: preflight → refine gate → plan gate; spawner called
5 times in order [refiner, architect, task_planner|risk_analyst,
task_planner|risk_analyst, reviewer_plan]; tracker reaches
`is_complete=True`.
- Refine stop: returns refine artifact path; spawner called 1
time (no plan dispatch).
- Mixed verdict: with a per_producer verdict JSON {architect:ACK,
task_planner:NACK, risk_analyst:ACK}, the tracker records the
NACK on task_planner → reviewer_plan; `is_complete=False`;
blocking_agents includes reviewer_plan (unresolved critical
NACK) and task_planner (not fully ACKed).
- Fail-closed: with reviewer spawn exit_code=1 and no verdict
file, the tracker NACKs every critical edge; risk_analyst
(advisory edge) still confirms; reviewer_plan blocks.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* Address slice-1 review: fix install path, bridge answer-write, silent fallbacks
Reviewer findings from PR #2724 (egg-reviewer slice-1 review):
Blockers (1-4):
* The documented `pip install -r requirements.txt` failed — no top-level
requirements.txt exists. Switch SKILL.md, plugin.json's
`egg.install_instructions`, and `bin/preflight.py` to `pip install .`
against pyproject.toml.
* SKILL.md claimed Python 3.11+ but pyproject.toml requires >=3.14.
Update SKILL.md and preflight to reflect the real floor; correct the
matching docstring in orchestrator/substrate/__init__.py.
* The inline `python3 -c "..."` answer-write snippet in SKILL.md was
broken (shell-interpolated `${ANSWER}` → NameError, deprecated
`datetime.utcnow`, non-atomic write). Replace with a dedicated
`bin/write_answer.py` helper that reads the JSON-encoded answer from
stdin, uses `datetime.now(UTC)`, and writes atomically via
tmp + `os.replace`. SKILL.md's loop now invokes the helper.
* `_serialise_decision` silently swallowed `model_dump` failures, and
`_read_contract` silently overwrote unparseable contracts (dropping
`answer_log`). Log to stderr on serialisation fallbacks; refuse to
overwrite a corrupted contract and exit 1 instead.
Non-blocking (5-11):
* `orchestrator/substrate/__init__.py` docstring rewritten to reflect
reality: Python 3.14+ introduced PEP 758 (`except A, B:` without
parens); ruff under py314 target strips redundant parens, hence
`# fmt: skip` on multi-except lines.
* Stderr warning in `_advance_generator`'s `finally` so a teardown
failure inside `generator.close()` is at least observable.
* `allowed-tools` tightened from `Bash(python3 *:*)` to
`Bash(python3 plugins/egg-sdlc/skills/egg-sdlc/bin/*:*)` — the two
helper scripts are the entire Python surface the skill can invoke.
* Re-spawn cost note expanded in SKILL.md: each driver invocation
re-runs every prior subagent spawn (real Anthropic API spend); slice-2
compounds to 8 spawns at the final stage.
* `_RUBRIC_LANDED_ROLES` registry collapsed into `_CURRENT_LOADER_SLICE`
+ filesystem probe; same diagnostics, no parallel registry that can
drift from disk state.
* Abort vocabulary exported as `ABORT_ANSWERS` from
`orchestrator.substrate.in_process`; the driver imports it (with a
literal fallback when the orchestrator package is unimportable) so
the driver, orchestrator, and slice-3 daemon share a single source
of truth.
* R2 caveat surfaced as an explicit open question for slice-5
sequencing in SKILL.md.
Tests:
* New `shared/tests/test_write_answer.py` (6 tests, all passing) pins
the JSON-encoding round-trip, the timestamp format match against the
driver, the atomic-write contract, and the corrupted-contract refusal.
* Existing `test_rubric_loader`, `test_run_pipeline_in_process*`,
`test_substrate_interfaces`, `test_bridge_flattened_round_trip`, and
`test_pretooluse_hook_nested` all still pass.
* The pre-existing `test_empty_diff_subprocess_skips_pytest` failure
in `tests/tools/` reproduces against HEAD without these changes
(detached-HEAD worktree edge case in the test selector).
Authored-by: egg
* slice-2 coder v4: support rubric-default single-verdict JSON schema (#2717)
Addresses reviewer_code_holistic v3 NACK blocker H3 — the rubric the
documenter shipped (plugins/egg-sdlc/skills/egg-sdlc/agents/reviewer_plan.md
"Verdict JSON shape", lines 57-80) documents a single top-level
verdict object (verdict ∈ {ACK, NACK}, analysis carrying the eight
criteria, feedback blob, artifact_references), not the per_producer
wrapper v2/v3's parser expected. A rubric-following reviewer's NACK
would silently fall into the "verdict file present but no parseable
per_producer entries" branch and the orchestrator's optimistic-ACK
fallback would mask the NACK from the operator at the plan-HITL gate.
v4 makes `read_plan_reviewer_verdicts` accept BOTH schemas:
1. Rubric-default single-verdict (broadcast). When the JSON's
top-level `verdict` is "ACK" or "NACK", the verdict is broadcast
to every plan producer edge — ACK acks all three, NACK nacks
all three with `feedback` propagated as the per-edge `reason`
(a synthetic placeholder fires if `feedback` is empty so the
tracker's NACK guard doesn't reject the payload). This is
"Option (c)" from the v3 NACK; per-edge granularity is lost
but the rubric's "ACK only if every criterion passes" semantic
IS preserved.
2. Per-producer extension (per-edge). The existing per_producer
wrapper still takes precedence when present and well-formed.
Reviewers that want explicit edge granularity (ACK architect +
NACK task_planner) write the wrapper; the rubric's default
shape stays broadcast-compatible.
The function now takes an optional `plan_producers` kwarg so the
caller (the in-process orchestrator) can broadcast the single
verdict to the right role set. The `_read_plan_reviewer_verdicts`
class method delegate also propagates the kwarg so tester-side
tests that call the method retain their access pattern.
Smoke (manual, in-process, MagicMock subagents):
- Rubric-default single-verdict NACK: tracker NACKs architect + task_planner
(critical edges), risk_analyst still confirms (advisory), reviewer_plan
blocks consensus. is_complete=False; blocking_agents=['architect',
'task_planner', 'reviewer_plan'].
- Rubric-default single-verdict ACK: every edge confirmed; is_complete=True.
- per_producer wrapper still works: mixed ACK/NACK applied per edge.
- Harness-fake path (no verdict file, reviewer exit 0): optimistic ACK
preserved so tester's existing 16 passing tests keep their access pattern.
- Fail-closed path (no verdict file, reviewer exit non-zero): critical
edges NACK'd (unchanged from v2/v3).
ruff format + ruff check + file-size lint all pass. `_plan_phase.py` is
747 lines; `in_process.py` 1095 lines.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* Persist BRC history for slice-2 (#2548)
* Move skill-loop python3 -c calls into bin/ helpers
Address review feedback on PR #2724:
- Add bin/read_status.py and extend write_answer.py with --answer-…
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
--set cni.chainingMode=portmapto the Cilium install args inscripts/install-cilium.shso KuberneteshostPort:mappings actually bind on the node.cni-chaining-mode: portmapassertion to the post-install verification loop so the regression can't land silently again.The bug
#2705 installs Cilium with
kubeProxyReplacement=false— deliberate, because KPR attaches eBPF programs to physical interfaces and blackholes connectivity on hosts with unusual primary NICs (the install script's own comment names wireless as the canonical case). With KPR off, Cilium does not implement KuberneteshostPortitself. The install didn't chain a CNI plugin that does, so everyhostPort:mapping in the local overlay was silently a no-op.Concretely on this host, after
make k3s-teardown && make k3s-setup && make deploy:kubectl get pods -n egg-systemshows orchestrator + gatewayRunning/Ready.mcp_serverstarted on0.0.0.0:9850inside the pod,Uvicorn running on http://0.0.0.0:9850.k8s/overlays/local/patches/orchestrator-volumes.yamldeclareshostPort: 9850(and9849).curl http://localhost:9850/mcpreturnsConnection refused. Same for9849.cilium-configConfigMap has onlykube-proxy-replacement: "false"— nocni-chaining-mode, noenable-host-port.That breaks Claude Code's
eggMCP client (configured to talk tohttp://localhost:9850/mcp) and the SDLC skill'swait-statuslauncher (defaultsEGG_ORCHESTRATOR_URL=http://localhost:9849).The fix
cni.chainingMode=portmaptells Cilium's CNI installer to drop its conflist with the standardportmapCNI plugin chained after it.portmapimplementshostPortvia iptables — compatible withkubeProxyReplacement=falseand the rest of the wireless-NIC-safe datapath (bpf.masquerade=false,bpf.hostLegacyRouting=true). It produces thecni-chaining-mode: portmapkey in the deployedcilium-configConfigMap, which the verification loop now asserts.Why #2705 missed this
#2705's verification block checks datapath flags (
kube-proxy-replacement,enable-bpf-masquerade,enable-host-legacy-routing) and CNI conflist presence, but doesn't end-to-end probe ahostPort. The integration tests touched (integration_tests/test_deployment_validation_logic.py) don't cover the hostPort path either. The new assertion in the verify loop closes the config-side gap; an end-to-end hostPort probe would be a useful follow-up but isn't included here (out of scope for the hotfix).Test plan
make k3s-teardown && make k3s-setupsucceeds on a clean host. Post-install verify loop passes with the newcni-chaining-mode:portmapline.make deploy,curl -sf http://localhost:9849/api/v1/livereturns OK andcurl -sf http://localhost:9850/mcpreaches the MCP server (noConnection refused).kubectl -n kube-system get cm cilium-config -o "jsonpath={.data.cni-chaining-mode}"printsportmap.kubectl -n kube-system get cm cilium-config -o "jsonpath={.data.kube-proxy-replacement}"still printsfalse(regression check against the prior wireless-NIC fix).eggMCP client reconnects tohttp://localhost:9850/mcpafter deploy.