Fix #1986: auto-discover host worktree path; derive EGG_HOST_REPO_MAP from config by jwbron · Pull Request #1991 · jwbron/egg

jwbron · 2026-04-24T08:15:42Z

Summary

Agent pods land in unwritable worktrees because the gateway hands the orchestrator its own in-pod path as the hostPath.path source for agent-pod mounts. Kubelet DirectoryOrCreates an empty root-owned dir at the wrong host location, the agent mounts that instead of its real worktree, and every producer stalls on EACCES trying to write draft artifacts — the failure mode reported in #1986.

While fixing that, also removed every hardcoded reference to the PR author's specific username/layout from the local overlay and deploy tooling, so other contributors can make deploy without editing YAML first.

Root cause

translate_to_host_path() in gateway/gateway.py relied on a single HOST_HOME env var for translation. The base sets it to /home/egg (the container-side path), so translation was an identity map and the orchestrator built agent pods with hostPath /home/egg/.egg-worktrees/... — a path the kernel obligingly created, empty, owned by root.

Verified live against the running gateway pod:

/proc/self/mountinfo shows /home/egg/.egg-worktrees mount root=/home/jwies/.egg-worktrees. The kernel already knows the correct mapping; the code just wasn't reading it.
Writing a file inside the pod at /home/egg/.egg-worktrees/test landed at /home/jwies/.egg-worktrees/test on the host — kubelet's nested mounts work fine.
translate_to_host_path() with HOST_HOME=/home/egg returned the in-pod path unchanged; the orchestrator plugged that into the agent pod's hostPath.

Fix

1. Auto-discover via mountinfo (gateway/gateway.py)
translate_to_host_path() now reads /proc/self/mountinfo at import time, builds a longest-first mount_point → host_root table, and translates by finding the most specific mount_point that prefixes the input. Kubelet records every hostPath bind source in the root field, so this works without any env-var configuration for every hostPath volume in every overlay. HOST_HOME remains as an explicit fallback for test environments.

2. Zero jwies in overlay YAML (k8s/overlays/local/patches/*.yaml)
Every /home/jwies/... becomes ${EGG_HOST_HOME}. The deploy-time default is $HOME, overridable via make deploy EGG_HOST_HOME=/data/egg.

3. EGG_HOST_REPO_MAP is derived from ~/.config/egg/repositories.yaml (scripts/build-host-repo-map.py)
Previously the orchestrator overlay hand-maintained a JSON blob mapping specific owner/repo pairs to one contributor's local checkout layout. That entire blob is now replaced with a ${EGG_HOST_REPO_MAP} placeholder. At deploy time, a new helper script reads local_repos.paths from repositories.yaml, runs git config --get remote.origin.url on each path, parses owner/repo from the remote URL (SCP, ssh://, https:// shapes all handled), and emits the JSON mapping. Every contributor's map is now produced from their own repositories.yaml — no YAML editing required.

4. Makefile pipes it all together
Pre-check for envsubst, compute defaults, echo the resolved values, pipe kubectl kustomize → envsubst '$EGG_HOST_HOME $EGG_HOST_REPO_MAP' → a brief sed that single-quotes the brace-delimited JSON value (kustomize strips quotes from the unexpanded placeholder, so the YAML parser would otherwise mistake it for a flow-style mapping) → image-tag sed → kubectl apply.

5. Docstring / test-fixture cleanup
Swapped /home/jwies and jwbron placeholders to /home/user and my-org in docstrings, test mocks, and arch docs where they were arbitrary example values. Canonical jwbron/egg references (schema $ids, GitHub Action refs, release image names, issue links) are intentionally left alone — those point at the actual project.

Verification

New gateway/tests/test_translate_host_path.py: 12 tests covering longest-prefix selection, sibling-path safety, HOST_HOME fallback precedence, and mountinfo parsing edge cases. All pass.
New scripts/tests/test_build_host_repo_map.py: 21 tests covering every remote URL shape the parser accepts (plus rejected forms), missing config, missing directories, missing origin, empty/missing local_repos, and sorted JSON output. All pass.
Pre-existing orchestrator/tests/test_container_spawner.py / tests/shared/egg_container/test_phase_mounts.py still pass after fixture rename.
Live-cluster probe of the new logic (same pod, same mountinfo):
- /home/egg/.egg-worktrees/issue-test/egg → /home/jwies/.egg-worktrees/issue-test/egg ✓
- /home/egg/repos/egg → the contributor's local egg checkout ✓ (kubelet resolved the symlink)
Dry-run of the full deploy pipeline with a synthetic user (EGG_HOST_HOME=/home/someuser) renders clean YAML with no jwies anywhere; kubectl apply --dry-run=client accepts the output.
scripts/build-host-repo-map.py run against the author's ~/.config/egg/repositories.yaml produces exactly the same map that was previously hardcoded, auto-detected from origin remote URLs.
grep -rn "jwies\|jwbron" k8s/ Makefile scripts/build-host-repo-map.py → nothing.

What this deliberately doesn't do

Does not touch the base deployment's HOST_HOME=/home/egg. It's now redundant (auto-discovery takes precedence) but removing it is an orthogonal cleanup.
Does not rewrite canonical jwbron/egg references (schema $ids, image refs, Action refs, doc examples). Those are the actual project identity, not developer-specific hardcoding.
Does not address the secondary gaps the issue calls out (overseer silence on the EACCES failure mode, slow orchestrator stall escalation). Those belong in separate PRs.

Test plan

make deploy with no env-var overrides expands EGG_HOST_HOME to $HOME and EGG_HOST_REPO_MAP from repositories.yaml
make deploy EGG_HOST_HOME=/data/egg EGG_HOST_REPO_MAP='{"o/r":"/p"}' overrides cleanly
Fresh SDLC pipeline: refiner can mkdir .egg-state/drafts/ and write its analysis artifact
All existing + new tests pass

Closes #1986.

🤖 Generated with Claude Code

egg-reviewer

Reviewed every aspect of this change: the Kustomize strategic merge behavior, the translate_to_host_path() function in gateway/gateway.py:422-447, the orchestrator's _host_to_local_volumes() in kubernetes_spawner.py:205-220, the _find_missing_worktrees sanity check (line 300-317), the base deployment at k8s/base/gateway-deployment.yaml:77-78, and the orchestrator overlay at k8s/overlays/local/patches/orchestrator-volumes.yaml.

No blocking issues.

The fix is correct:

Strategic merge on the env list merges by name, so the patch correctly overrides the base's HOST_HOME=/home/egg with /home/jwies without duplicating or clobbering other env vars.
With HOST_HOME=/home/jwies, translate_to_host_path() (module-level HOST_HOME evaluated at process start) now correctly maps /home/egg/.egg-worktrees/... → /home/jwies/.egg-worktrees/..., which matches the overlay's hostPath backing directories already declared in the same file.
The orchestrator doesn't need a corresponding HOST_HOME override because: (a) its filesystem operations use the hardcoded WORKTREE_BASE_DIR = Path("/home/egg/.egg-worktrees") which is already the mount point, and (b) the host paths from the gateway go directly into agent pod hostPath.path volume specs.
The 12-line comment is justified for a config knob whose absence caused a non-obvious cascading failure.

One minor non-blocking style nit:

value: /home/jwies (line 31) is unquoted while the base uses value: "/home/egg". Both are valid YAML, but quoting the value would be consistent with the base deployment style.

james-in-a-box

Agreed. Quoted the HOST_HOME value to match the base deployment style (value: "/home/jwies"). Fixed in 3f0980d.

— Authored by egg

egg-reviewer

Re-review of changes since 60be94c.

The only new commit (3f0980d) quotes the HOST_HOME value — value: "/home/jwies" — matching the base deployment's value: "/home/egg" style. This addresses the style nit from the previous review.

Verified:

The full overlay file is well-formed YAML with correct indentation.
Kustomize strategic merge on env still merges by name, so the quoted string changes nothing about the runtime behavior.
The kustomization.yaml correctly references patches/gateway-volumes.yaml.
No other files changed.

No new issues. Previous feedback addressed.

— Authored by egg

Producers in every phase stall on EACCES when trying to write draft artifacts into their worktree. Root cause: the gateway's `translate_to_host_path()` relied on `HOST_HOME=/home/egg` (the container-side path) as an identity map, so when the overlay actually backed the worktrees volume with a different host path, the orchestrator ended up passing the in-pod path to kubelet as a hostPath source. `DirectoryOrCreate` then created an empty root-owned directory at the wrong host location, and every agent mounted that empty dir instead of its real worktree. Fix in three parts: 1. gateway/gateway.py — auto-discover the host path for any in-pod path from `/proc/self/mountinfo`. Kubelet records the bind source as the mount's `root` field for every hostPath volume, so the gateway can translate reliably without any env-var configuration. `HOST_HOME` is preserved as an explicit fallback for test environments. Verified live in the cluster: the pod's mountinfo yields `/home/egg/.egg-worktrees/...` → `/home/jwies/.egg-worktrees/...`, which is the correct host path. 2. k8s/overlays/local/patches/*.yaml — replace every hardcoded `/home/jwies/...` with `${EGG_HOST_HOME}` / `${EGG_HOST_REPOS_DIR}` placeholders. The overlay no longer needs per-developer edits to the YAML. 3. Makefile — pipe kustomize output through envsubst with those two variables in the deploy target. Defaults: `EGG_HOST_HOME=$HOME` and `EGG_HOST_REPOS_DIR=$HOME/khan`. Override at invocation: `make deploy EGG_HOST_HOME=/data/egg EGG_HOST_REPOS_DIR=/srv/repos`. Secondary gaps the issue calls out (overseer silence on this failure mode, slow orchestrator stall escalation) are left for separate PRs. Tests: new `gateway/tests/test_translate_host_path.py` covers longest-prefix selection, sibling-path safety, HOST_HOME fallback precedence, and mountinfo parsing edge cases. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

The overlay previously hand-maintained a JSON map of `owner/repo → host_path` with specific Khan/* and jwbron/* entries pinned to one contributor's ``/home/jwies/khan/*`` layout. Another developer would have had to edit the YAML before `make deploy` worked. Replace the hardcoded map with a deploy-time lookup: - ``scripts/build-host-repo-map.py`` reads ``local_repos.paths`` from ``~/.config/egg/repositories.yaml``, runs ``git config --get remote.origin.url`` on each, parses ``owner/repo`` from the remote URL (SCP, ssh://, https:// forms all handled), and emits the JSON mapping to stdout. Missing config / missing dirs / broken remotes are skipped with a stderr warning. - The orchestrator overlay now uses ``${EGG_HOST_REPO_MAP}`` as a placeholder. A follow-up ``sed`` in the Makefile adds single quotes around the expanded JSON value — kustomize strips quotes from the unexpanded placeholder, so the YAML parser would otherwise mistake the brace-delimited JSON for a flow-style mapping literal. - ``Makefile`` computes ``EGG_HOST_REPO_MAP`` from the helper (with the standard env-var override), echoes the resolved value, and pipes through envsubst + the JSON-quoting sed. PyYAML is already a project dependency. Tests: ``scripts/tests/test_build_host_repo_map.py`` covers every remote URL shape the parser accepts (plus rejected forms), missing config, missing directories, missing origin, empty/missing ``local_repos`` section, and sorted JSON output from the CLI entry point. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Several docstrings and test mocks used /home/jwies and jwbron as arbitrary example values — not runtime hardcoding, but they suggest the code assumes a specific developer's setup. Swap to neutral placeholders (/home/user, my-org) so other contributors reading the code don't have to pattern-match around one person's username. Canonical references to the jwbron/egg project (schema $ids, GitHub Action refs, release image names, issue links) are intentionally left alone — those point at the actual project and would be wrong as placeholders. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

egg-reviewer

Re-review of the rewritten PR (previous review at 3f0980d covered a different approach — simple HOST_HOME overlay override — that no longer exists).

The PR was rewritten as three coordinated changes: mountinfo auto-discovery in the gateway, envsubst-templated overlays, and Makefile integration. Reviewed the full delta.

No blocking issues.

The core translation logic in translate_to_host_path is correct: longest-prefix matching against the mountinfo table handles the exact failure mode from #1986, and the startswith(mount_point + "/") guard correctly prevents partial path-component matches (e.g., /home/egg vs /home/egg-other). The envsubst integration is clean — restricting expansion to $EGG_HOST_HOME $EGG_HOST_REPOS_DIR prevents accidental substitution of other $-sigils in the rendered YAML. Tests are well-structured with proper teardown in fixtures.

Verified: all 12 new tests pass, no jwies remains in k8s/ or Makefile, _load_bind_mount_mapping gracefully handles missing /proc/self/mountinfo.

Non-blocking suggestions (4):

_load_bind_mount_mapping includes all mount types — procfs, tmpfs, sysfs, cgroup entries all land in _BIND_MOUNT_MAPPING alongside the hostPath bind mounts. This doesn't cause wrong results for the known use case (longest-prefix matching ensures the specific /home/egg/.egg-worktrees entry wins over less specific mounts), but the function name says "bind_mount" while it collects everything. Consider either filtering to bind mounts (check for the shared: or master: optional field, or filter by filesystem type) or renaming to _load_mount_mapping to match what it actually does.
mountinfo root field = host path assumption — The code assumes fields[3] (the root field) is the absolute host path. This holds on single-partition systems (ext4/xfs at /) where the filesystem root IS the namespace root, but breaks on multi-partition setups (root is relative to the partition's filesystem root, e.g., /egg-worktrees instead of /data/egg-worktrees if the host mount is at /data). The HOST_HOME fallback is documented as the escape hatch for this, but it can't actually override mountinfo when mountinfo has a matching entry — mountinfo always wins. A brief code comment at fields[3] noting this assumption would help future maintainers understand the scope. For a true escape hatch, an env var like EGG_DISABLE_MOUNTINFO=1 that skips the loop would let operators fall through to HOST_HOME on unusual systems.
Octal escapes in mountinfo — mountinfo uses octal escapes for special characters in paths (\040 for space, \011 for tab, \134 for backslash). The parser doesn't decode these. Unlikely to matter for /home/.../.egg-worktrees/... paths but worth a one-line comment noting the limitation.
builtins.open mock scope in TestLoadBindMountMapping — The patch("builtins.open", ...) in the parser tests replaces open() globally rather than targeting gateway.open. This works today because _load_bind_mount_mapping is the only caller, but it's fragile — if a future change adds logging or validation that opens a file during the call, the mock will intercept it. patch("gateway.open", ...) would scope the mock to just the gateway module.

— Authored by egg

egg-reviewer

No agent-mode design concerns.

This PR is entirely infrastructure: gateway path translation (translate_to_host_path via mountinfo), k8s overlay portability (envsubst replacing hardcoded paths), and a build-time helper (build-host-repo-map.py). None of the changes touch agent prompts, agent workflows, LLM invocations, or output formats — the agent-mode design guidelines don't apply here.

— Authored by egg

egg-reviewer

Re-review of changes since 1da0593. Two new commits: e5703d1 (auto-derive EGG_HOST_REPO_MAP) and c538393 (neutralize user-specific example values).

No blocking issues.

The build-host-repo-map.py script is well-structured: parse_owner_repo handles all three remote-URL families (SCP, ssh://, https://), the isinstance(local_repos, dict) guard on line 79 correctly protects .get() against non-dict YAML values, and json.dumps(…, separators=(",",":"), sort_keys=True) produces deterministic compact output. The Makefile pipeline (kustomize → envsubst → sed) is correct — the sed re-quoting step handles both the expanded-JSON case and the empty-{} fallback. Tests cover the essential URL forms, graceful degradation on missing/malformed config, and sorted CLI output.

Commit c538393 is a clean mechanical sweep — all jwies/jwbron references in docstrings and test fixtures replaced with neutral placeholders; canonical project references (GitHub URLs, schema $ids) correctly left alone; test assertions updated consistently.

Previous review's 4 non-blocking suggestions (bind-mount function naming, mountinfo root-field assumption, octal escapes, builtins.open mock scope) were not addressed — acceptable since they were advisory.

Non-blocking suggestions (3):

Unhandled yaml.YAMLError in build_map — build-host-repo-map.py:76 calls yaml.safe_load(fh) without catching parse errors. A corrupted repositories.yaml crashes the script with a traceback during make deploy instead of emitting {} and a warning. The docstring says "Emits {} (and exits 0) when the config is missing or lists no repos" — a parse error violates that contract. A try/except yaml.YAMLError that prints a warning to stderr and returns {} would match the documented behavior and keep the deploy from failing on a typo in the config.
Duplicate owner/repo silently last-wins — If two paths in local_repos.paths resolve to the same owner/repo (e.g., two checkouts of the same repo), the later entry silently overwrites the earlier one (line 100: mapping[owner_repo] = str(path)). This is fine behavior but worth a one-line code comment documenting the choice, since a user might not realize one of their configured repos is being shadowed.
$HOME/repos assumption in EGG_HOST_HOME fallback — The gateway overlay at gateway-volumes.yaml:30 uses ${EGG_HOST_HOME}/repos and orchestrator-volumes.yaml:55 does the same. The EGG_HOST_HOME default is $HOME. If a developer's repos live at ~/khan/ (or anywhere besides ~/repos/), the repos hostPath mount is wrong even though EGG_HOST_REPO_MAP now auto-discovers individual repo paths correctly. This was a pre-existing assumption, not introduced by this PR, so it's non-blocking — but worth noting since the EGG_HOST_REPO_MAP automation may lead developers to assume make deploy is now fully auto-configured when the repos volume still requires a conventional layout.

— Authored by egg

…ML errors - Rename _load_bind_mount_mapping -> _load_mount_mapping (and _BIND_MOUNT_MAPPING -> _MOUNT_MAPPING) since the function collects all mount types, not just bind mounts - Document mountinfo root-field assumption (single-partition) and octal escape limitation in the docstring - Scope test mocks to gateway.open instead of builtins.open to prevent intercepting unrelated file opens - Handle yaml.YAMLError in build_map so a corrupted repositories.yaml emits a warning and returns {} instead of crashing - Document last-wins behavior for duplicate owner/repo entries - Add test for corrupted YAML graceful degradation

james-in-a-box

Addressed all non-blocking suggestions from both review rounds in 3094ed7:

From the latest review (c538393):

yaml.YAMLError handling — build_map now catches yaml.YAMLError, prints a warning to stderr, and returns {} — matching the documented contract. Added a test for corrupted YAML.
Duplicate owner/repo comment — Added a comment documenting the last-wins behavior at the assignment site.
$HOME/repos assumption — Acknowledged as pre-existing, no change needed.

From the earlier review (1da0593), previously deferred:

Function naming — Renamed _load_bind_mount_mapping → _load_mount_mapping (and _BIND_MOUNT_MAPPING → _MOUNT_MAPPING) since the function collects all mount types.
mountinfo root-field assumption — Documented in the docstring that fields[3] is relative to the filesystem root and may not equal the absolute host path on multi-partition setups. Noted the HOST_HOME escape hatch.
Octal escapes — Added a docstring note about mountinfo's octal encoding and that we don't decode it.
builtins.open mock scope — Changed to patch("gateway.open", ...) so the mock is scoped to the gateway module.

— Authored by egg

egg-reviewer

No agent-mode design concerns.

Re-review delta (changes since c538393): Single commit 3094ed7 addresses prior review feedback — function renaming (_load_bind_mount_mapping → _load_mount_mapping), mock scoping (gateway.open instead of builtins.open), yaml.YAMLError handling in build-host-repo-map.py, and documentation comments. All infrastructure-only changes with no impact on agent prompts, workflows, or LLM interactions.

— Authored by egg

egg-reviewer

Re-review of changes since c538393. One new commit: 3094ed7 (address review feedback).

No blocking issues.

All seven suggestions from the two prior review rounds are addressed correctly:

Function/variable rename (_load_bind_mount_mapping → _load_mount_mapping, _BIND_MOUNT_MAPPING → _MOUNT_MAPPING): Complete — no stale references remain anywhere in the repo (verified via GitHub code search).
mountinfo root-field assumption docstring: Accurate — correctly notes fields[3] is relative to the filesystem root and identifies the multi-partition limitation with HOST_HOME as the escape hatch.
Octal escapes docstring note: Present and appropriately scoped — acknowledges the limitation without over-engineering a fix for /home/... paths.
Mock scope change (builtins.open → gateway.open): Correct. Verified the conftest registers the gateway module as sys.modules["gateway"] via _load_module_with_replaced_imports, so patch("gateway.open", ...) correctly targets the module namespace where _load_mount_mapping resolves open. The four tests in TestLoadMountMapping now mock only the gateway module's open, leaving other modules unaffected.
yaml.YAMLError handling in build_map: Catches parse errors, prints a diagnostic to stderr, returns {} — matches the function's documented contract ("Emits {} when the config is missing or lists no repos"). New test test_corrupted_yaml_returns_empty confirms the behavior with an unterminated flow sequence.
Duplicate owner/repo comment: Clear one-liner documenting the last-wins semantics at the assignment site.
$HOME/repos assumption: Correctly left as-is (pre-existing, not introduced by this PR).

Non-blocking (1):

The test fixture is still named override_bind_mounts while the variable it operates on is now _MOUNT_MAPPING. Renaming to override_mounts would complete the consistency sweep. Not worth a round-trip.

— Authored by egg

james-in-a-box

Agreed. Renamed override_bind_mounts fixture to override_mounts to match the _MOUNT_MAPPING variable it operates on. Fixed in 5f283bc.

— Authored by egg

james-in-a-box · 2026-04-24T17:13:17Z

All review feedback from both review rounds has been addressed. The latest re-review at ad50a66 confirms no blocking or new issues remain. CI is green.

PR is ready for merge.

— Authored by egg

`on-review-feedback.yml` previously defaulted `authorized_users` (both the reusable input and the `EGG_AUTHORIZED_USERS` repo variable) to `jwbron`. That made the canonical repo owner an implicit ambient authorizer for any fork that didn't set the variable. Make the setting explicit: - `authorized_users` input is now `required: true` with no default. - `EGG_AUTHORIZED_USERS` joins `EGG_BOT_USERNAME` in the validate-config step — event-triggered runs fail fast with a clear error listing the missing variable(s) rather than silently authorising the canonical owner. - The resolve-inputs step no longer inlines the `|| 'jwbron'` fallback. - Docs in `github-automation.md` and `reusable-workflows.md` reflect the new required-and-no-default contract. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

The repo was dotted with illustrative references to this project's owner (`jwbron/egg`), the author's home path (`/home/jwies`), and a specific downstream org (`Khan/webapp`). They were never runtime dependencies — just arbitrary examples inside docstrings, CLI usage tables, schema descriptions, agent-output log snippets, test fixtures, and code comments — but they implied the code assumed one developer's setup. Swap them to neutral placeholders (`owner/repo`, `owner/repo-checkpoints`, `/home/user`, `my-org`, `owner--repo`) across: - Test fixture data and assertions (gateway, orchestrator, sandbox, shared) — retained the canonical string only where tests exercise the `EGG_REPO` runtime gate itself (`reviewer_agent_design` filter, `test_git_remote_takes_precedence_over_egg_repo`). - Code-comment examples in `orchestrator/kubernetes_spawner.py` and `sandbox/entrypoint.py`. - Docstring examples (`gateway/gateway.py`, `config/repo_config.py`, `shared/egg_contracts/checkpoints.py`). - MCP tool schema descriptions (`orchestrator/mcp_tools.py`). - JSON Schema `description` fields under `.egg/schemas/`. - Arch/guide/reference docs (logging, orchestrator, checkpoint-access, custom-phase, mcp-deployment-tools, checkpoint-browser, sdlc-pipeline). - Skill files (`skills/sdlc/SKILL.md`, `skills/babysit-pr/SKILL.md`). - `action/generate-config.sh` comment. Canonical references to the `jwbron/egg` project are intentionally left alone: - GitHub issue / PR URLs - Schema `$id` URLs - Reusable-workflow invocation paths (`jwbron/egg/.github/workflows/…@…`) - GitHub Action refs (`jwbron/egg/action@main`, `jwbron/egg@main`) - GHCR image names (`ghcr.io/jwbron/egg-*`) - The `EGG_REPO` runtime constant that gates the `reviewer_agent_design` role — `docs/reference/agent-roles.md` now calls out that this is a hardcoded scope tied to this project's identity. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

egg-reviewer

No agent-mode design concerns.

Re-review delta (changes since ad50a66): Two new commits — 2d8e363 (make authorized_users required in the review-feedback workflow, removing the implicit jwbron default) and 4b95168 (mechanical sweep replacing user-specific example values with neutral placeholders across tests, docs, MCP tool descriptions, skill files, and code comments).

Neither commit touches agent prompts, agent workflows, LLM invocations, or output formats. The skill file changes (SKILL.md for sdlc and babysit-pr) are limited to replacing example --repo values in usage tables — no behavioral or structural changes.

— Authored by egg

egg-reviewer

Re-review of the squashed PR (branch was rebased into single commit 4b95168 since last review at ad50a66).

Reviewed the complete diff: gateway mountinfo auto-discovery, build-host-repo-map.py, envsubst-templated k8s overlays, Makefile deploy pipeline, workflow authorized_users enforcement, and the cross-repo genericization sweep.

No blocking issues.

All seven suggestions from prior review rounds are correctly incorporated into the squash. Verified: function rename (_load_mount_mapping), mock scope (gateway.open), fixture rename (override_mounts), yaml.YAMLError handling, duplicate owner/repo comment, mountinfo docstrings, octal escape note — all present.

Non-blocking observations (3):

HOST_HOME escape hatch is effectively dead code. This was noted in the prior review (suggestion #2 at 1da0593) and documented, but the underlying issue remains. In any container where /proc/self/mountinfo exists (which is all Linux containers), the rootfs overlay mount (/ → /) matches every path via longest-prefix as a last resort, producing an identity translation. This means the HOST_HOME fallback on gateway.py:499 can never execute — mountinfo always matches first. The docstring calls HOST_HOME an "escape hatch for test environments and unusual setups" but it only works if mountinfo is entirely absent (i.e., OSError on open). In tests this is fine because the override_mounts fixture replaces _MOUNT_MAPPING. In production k8s pods, the escape hatch is non-functional. Consider adding EGG_DISABLE_MOUNTINFO=1 support as originally suggested — a one-line if before the mountinfo loop would make the escape hatch real.
Partially genericized parsing example in skills/babysit-pr/SKILL.md:47. The line reads:
```
Parse the `owner/name` from the URL (e.g. `https://github.com/jwbron/egg.git` → `owner/repo`)
```
The input URL was intentionally kept as a canonical project reference, but the output was genericized from jwbron/egg to owner/repo. This makes the example misleading — the parsing result of https://github.com/jwbron/egg.git is jwbron/egg, not owner/repo. Either leave both specific (since the input is specific) or genericize both.
Broad sed re-quoting in Makefile. The sed -E "s|^(\s*value: )(\{.*\})$$|\1'\2'|" matches any YAML value line starting with { and ending with }, not just EGG_HOST_REPO_MAP. Works today but is fragile — a future env var with a flow-mapping value would be silently re-quoted. A more targeted approach would anchor on the variable name (e.g., match EGG_HOST_REPO_MAP on the preceding line), though the complexity trade-off may not be worth it for a local-dev-only target.

— Authored by egg

james-in-a-box · 2026-04-24T17:45:13Z

Automated feedback loop has reached the maximum of 5 rounds. Human review is needed to make further progress on this PR.

1. Make the HOST_HOME escape hatch actually reachable. Real Linux containers expose a rootfs ``/ → /`` entry in ``/proc/self/mountinfo`` that matches every path under longest-prefix lookup as an identity translation, so the mountinfo branch never falls through to the env-var fallback in production. Add ``EGG_DISABLE_MOUNTINFO=1`` to short-circuit the mountinfo read at module import — the flag lets operators force the explicit ``HOST_HOME`` path in environments where mountinfo doesn't reflect the real layout (multi-partition hosts, exotic bind-mount namespaces). 2. Fix mismatched example in ``skills/babysit-pr/SKILL.md``. The repo detection section showed a canonical input URL (``github.com/jwbron/egg.git``) paired with a generic output (``owner/repo``) — parsing the former yields ``jwbron/egg``, not ``owner/repo``. Make both sides of the example use the same placeholder so the transformation is accurate. 3. Scope the Makefile JSON-quoting sed to ``EGG_HOST_REPO_MAP`` only. The previous regex matched any YAML value line of the shape ``value: {...}``, which would silently re-quote any future env var whose value happens to be a flow-style mapping. Anchor on the preceding ``- name: EGG_HOST_REPO_MAP`` line via ``N;s|…|…|`` so the substitution only triggers for the one entry we intend. Tests: ``test_translate_host_path.py`` gains coverage for the disable flag (honored, accepts standard truthy spellings, lets ``HOST_HOME`` take over), bringing the suite to 25 tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

… from config (#1991) * Fix #1986: auto-discover host worktree path; remove jwies hardcoding Producers in every phase stall on EACCES when trying to write draft artifacts into their worktree. Root cause: the gateway's `translate_to_host_path()` relied on `HOST_HOME=/home/egg` (the container-side path) as an identity map, so when the overlay actually backed the worktrees volume with a different host path, the orchestrator ended up passing the in-pod path to kubelet as a hostPath source. `DirectoryOrCreate` then created an empty root-owned directory at the wrong host location, and every agent mounted that empty dir instead of its real worktree. Fix in three parts: 1. gateway/gateway.py — auto-discover the host path for any in-pod path from `/proc/self/mountinfo`. Kubelet records the bind source as the mount's `root` field for every hostPath volume, so the gateway can translate reliably without any env-var configuration. `HOST_HOME` is preserved as an explicit fallback for test environments. Verified live in the cluster: the pod's mountinfo yields `/home/egg/.egg-worktrees/...` → `/home/jwies/.egg-worktrees/...`, which is the correct host path. 2. k8s/overlays/local/patches/*.yaml — replace every hardcoded `/home/jwies/...` with `${EGG_HOST_HOME}` / `${EGG_HOST_REPOS_DIR}` placeholders. The overlay no longer needs per-developer edits to the YAML. 3. Makefile — pipe kustomize output through envsubst with those two variables in the deploy target. Defaults: `EGG_HOST_HOME=$HOME` and `EGG_HOST_REPOS_DIR=$HOME/khan`. Override at invocation: `make deploy EGG_HOST_HOME=/data/egg EGG_HOST_REPOS_DIR=/srv/repos`. Secondary gaps the issue calls out (overseer silence on this failure mode, slow orchestrator stall escalation) are left for separate PRs. Tests: new `gateway/tests/test_translate_host_path.py` covers longest-prefix selection, sibling-path safety, HOST_HOME fallback precedence, and mountinfo parsing edge cases. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Auto-derive EGG_HOST_REPO_MAP from repositories.yaml The overlay previously hand-maintained a JSON map of `owner/repo → host_path` with specific Khan/* and jwbron/* entries pinned to one contributor's ``/home/jwies/khan/*`` layout. Another developer would have had to edit the YAML before `make deploy` worked. Replace the hardcoded map with a deploy-time lookup: - ``scripts/build-host-repo-map.py`` reads ``local_repos.paths`` from ``~/.config/egg/repositories.yaml``, runs ``git config --get remote.origin.url`` on each, parses ``owner/repo`` from the remote URL (SCP, ssh://, https:// forms all handled), and emits the JSON mapping to stdout. Missing config / missing dirs / broken remotes are skipped with a stderr warning. - The orchestrator overlay now uses ``${EGG_HOST_REPO_MAP}`` as a placeholder. A follow-up ``sed`` in the Makefile adds single quotes around the expanded JSON value — kustomize strips quotes from the unexpanded placeholder, so the YAML parser would otherwise mistake the brace-delimited JSON for a flow-style mapping literal. - ``Makefile`` computes ``EGG_HOST_REPO_MAP`` from the helper (with the standard env-var override), echoes the resolved value, and pipes through envsubst + the JSON-quoting sed. PyYAML is already a project dependency. Tests: ``scripts/tests/test_build_host_repo_map.py`` covers every remote URL shape the parser accepts (plus rejected forms), missing config, missing directories, missing origin, empty/missing ``local_repos`` section, and sorted JSON output from the CLI entry point. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Replace user-specific example values in docstrings and test fixtures Several docstrings and test mocks used /home/jwies and jwbron as arbitrary example values — not runtime hardcoding, but they suggest the code assumes a specific developer's setup. Swap to neutral placeholders (/home/user, my-org) so other contributors reading the code don't have to pattern-match around one person's username. Canonical references to the jwbron/egg project (schema $ids, GitHub Action refs, release image names, issue links) are intentionally left alone — those point at the actual project and would be wrong as placeholders. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Address review feedback: rename mount mapping, scope mocks, handle YAML errors - Rename _load_bind_mount_mapping -> _load_mount_mapping (and _BIND_MOUNT_MAPPING -> _MOUNT_MAPPING) since the function collects all mount types, not just bind mounts - Document mountinfo root-field assumption (single-partition) and octal escape limitation in the docstring - Scope test mocks to gateway.open instead of builtins.open to prevent intercepting unrelated file opens - Handle yaml.YAMLError in build_map so a corrupted repositories.yaml emits a warning and returns {} instead of crashing - Document last-wins behavior for duplicate owner/repo entries - Add test for corrupted YAML graceful degradation * Rename override_bind_mounts fixture to override_mounts for consistency * Fix checks: apply automated formatting fixes * Require authorized_users; remove repo-owner default `on-review-feedback.yml` previously defaulted `authorized_users` (both the reusable input and the `EGG_AUTHORIZED_USERS` repo variable) to `jwbron`. That made the canonical repo owner an implicit ambient authorizer for any fork that didn't set the variable. Make the setting explicit: - `authorized_users` input is now `required: true` with no default. - `EGG_AUTHORIZED_USERS` joins `EGG_BOT_USERNAME` in the validate-config step — event-triggered runs fail fast with a clear error listing the missing variable(s) rather than silently authorising the canonical owner. - The resolve-inputs step no longer inlines the `|| 'jwbron'` fallback. - Docs in `github-automation.md` and `reusable-workflows.md` reflect the new required-and-no-default contract. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Genericize example values in tests, docs, and code comments The repo was dotted with illustrative references to this project's owner (`jwbron/egg`), the author's home path (`/home/jwies`), and a specific downstream org (`Khan/webapp`). They were never runtime dependencies — just arbitrary examples inside docstrings, CLI usage tables, schema descriptions, agent-output log snippets, test fixtures, and code comments — but they implied the code assumed one developer's setup. Swap them to neutral placeholders (`owner/repo`, `owner/repo-checkpoints`, `/home/user`, `my-org`, `owner--repo`) across: - Test fixture data and assertions (gateway, orchestrator, sandbox, shared) — retained the canonical string only where tests exercise the `EGG_REPO` runtime gate itself (`reviewer_agent_design` filter, `test_git_remote_takes_precedence_over_egg_repo`). - Code-comment examples in `orchestrator/kubernetes_spawner.py` and `sandbox/entrypoint.py`. - Docstring examples (`gateway/gateway.py`, `config/repo_config.py`, `shared/egg_contracts/checkpoints.py`). - MCP tool schema descriptions (`orchestrator/mcp_tools.py`). - JSON Schema `description` fields under `.egg/schemas/`. - Arch/guide/reference docs (logging, orchestrator, checkpoint-access, custom-phase, mcp-deployment-tools, checkpoint-browser, sdlc-pipeline). - Skill files (`skills/sdlc/SKILL.md`, `skills/babysit-pr/SKILL.md`). - `action/generate-config.sh` comment. Canonical references to the `jwbron/egg` project are intentionally left alone: - GitHub issue / PR URLs - Schema `$id` URLs - Reusable-workflow invocation paths (`jwbron/egg/.github/workflows/…@…`) - GitHub Action refs (`jwbron/egg/action@main`, `jwbron/egg@main`) - GHCR image names (`ghcr.io/jwbron/egg-*`) - The `EGG_REPO` runtime constant that gates the `reviewer_agent_design` role — `docs/reference/agent-roles.md` now calls out that this is a hardcoded scope tied to this project's identity. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Address review feedback (PR #1991 round 3) 1. Make the HOST_HOME escape hatch actually reachable. Real Linux containers expose a rootfs ``/ → /`` entry in ``/proc/self/mountinfo`` that matches every path under longest-prefix lookup as an identity translation, so the mountinfo branch never falls through to the env-var fallback in production. Add ``EGG_DISABLE_MOUNTINFO=1`` to short-circuit the mountinfo read at module import — the flag lets operators force the explicit ``HOST_HOME`` path in environments where mountinfo doesn't reflect the real layout (multi-partition hosts, exotic bind-mount namespaces). 2. Fix mismatched example in ``skills/babysit-pr/SKILL.md``. The repo detection section showed a canonical input URL (``github.com/jwbron/egg.git``) paired with a generic output (``owner/repo``) — parsing the former yields ``jwbron/egg``, not ``owner/repo``. Make both sides of the example use the same placeholder so the transformation is accurate. 3. Scope the Makefile JSON-quoting sed to ``EGG_HOST_REPO_MAP`` only. The previous regex matched any YAML value line of the shape ``value: {...}``, which would silently re-quote any future env var whose value happens to be a flow-style mapping. Anchor on the preceding ``- name: EGG_HOST_REPO_MAP`` line via ``N;s|…|…|`` so the substitution only triggers for the one entry we intend. Tests: ``test_translate_host_path.py`` gains coverage for the disable flag (honored, accepts standard truthy spellings, lets ``HOST_HOME`` take over), bringing the suite to 25 tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> Co-authored-by: egg <egg@localhost>

egg-reviewer

No agent-mode design concerns.

Re-review delta (changes since 4b95168): Single commit abeceac addresses prior review feedback — EGG_DISABLE_MOUNTINFO=1 escape hatch for the gateway's mountinfo auto-discovery, scoped Makefile sed to EGG_HOST_REPO_MAP only, and fixed a mismatched example in skills/babysit-pr/SKILL.md. All infrastructure and documentation changes with no impact on agent prompts, workflows, or LLM interactions.

— Authored by egg

egg-reviewer

Re-review of changes since 4b95168. One new commit: abeceac (address round-3 review feedback).

No blocking issues.

All three non-blocking suggestions from the prior review are addressed correctly:

EGG_DISABLE_MOUNTINFO=1 escape hatch — _mountinfo_disabled() short-circuits _load_mount_mapping() before touching /proc/self/mountinfo, making the HOST_HOME fallback reachable on real Linux containers. The function is called at module import time (via the module-level _MOUNT_MAPPING = _load_mount_mapping()), consistent with how HOST_HOME itself is evaluated. Truthy-value set (1, true, yes, on) is reasonable; the .strip().lower() normalization handles case and whitespace. Comment block at lines 428–437 clearly documents the interaction between mountinfo, HOST_HOME, and the disable flag.
SKILL.md genericization — Both input and output of the parsing example are now consistent: https://github.com/my-org/my-repo.git → my-org/my-repo. The prior review noted the mismatch where the input was specific but the output was generic.
Makefile sed anchoring — The re-quoting sed is now a two-line address pattern anchored to name: EGG_HOST_REPO_MAP: /{N;s|...|...|}. Only the value: line immediately following EGG_HOST_REPO_MAP is touched. Future env vars with flow-mapping values won't be silently re-quoted.

Test coverage is thorough:

test_returns_empty_when_disable_flag_set verifies the short-circuit by asserting open() is never called when the flag is set.
Parametrized truthy/falsy tests cover the boundary between accepted and rejected values.
TestDisableMountinfoWithHostHome verifies the integration: empty mount table + HOST_HOME set produces the correct translation.

All prior feedback (7 items across two earlier rounds + 3 from round 3) is now addressed. CI is green.

— Authored by egg

james-in-a-box · 2026-04-24T18:05:53Z

egg review completed. View run logs

23 previous review(s) hidden.

…dater] (#1992) * docs: update deploy docs for EGG_HOST_REPO_MAP auto-discovery Document changes introduced by a8b1324 (#1991): - `make deploy` now requires `envsubst` (GNU gettext) and auto-derives `EGG_HOST_HOME` and `EGG_HOST_REPO_MAP` from repositories.yaml via scripts/build-host-repo-map.py. Update the Deployment Commands table in deployment.md to reflect the actual behavior. - Add a note in local-quickstart.md that `local_repos.paths` entries are used to auto-derive EGG_HOST_REPO_MAP at deploy time, so no manual editing of k8s overlays is needed. Authored-by: egg * docs: move make deploy details from table cell to subsection Address review feedback: the dense ~350-char table cell is now a concise one-liner linking to a dedicated subsection with the defaults, overrides, and prerequisite info. --------- Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>

* Initialize SDLC contract for issue #1973 * refine(#1973): analysis for changeset-aware `make test` Draft analysis covering selection mechanism tradeoffs (grimp vs custom AST vs pytest-testmon vs hybrid), LKG storage/update/merge semantics, dynamic-import handling, CI coverage-gate interaction, shallow-checkout constraints, and target naming. Recommends grimp-based static reverse import graph with a non-tracked sidecar LKG, flipping the tracked-file default from the issue proposal. Registers 9 HITL decisions and 12 open-ended feedback questions via egg-contract for human input in the refine-approval step. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * refine(#1973): address reviewer_refine non-blocking nits - Tighten `gateway/gateway.py:309-317` → `:309-322` (the spec_from_file_location block runs through exec_module + except at 321-322). - Soften the testmon weak-spot claim in Option C: testmon sees in-process dynamic imports via coverage.py; the true miss-mode is subprocess-crossing coverage, not in-process importlib. - Flag the extra `feedback-1/Q10` (Fallback-trigger list completeness) in the prose intro so the prose-vs-contract drift is explicit. No recommendation change. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * docs: update deploy docs for EGG_HOST_REPO_MAP auto-discovery [doc-updater] (#1992) * docs: update deploy docs for EGG_HOST_REPO_MAP auto-discovery Document changes introduced by a8b1324 (#1991): - `make deploy` now requires `envsubst` (GNU gettext) and auto-derives `EGG_HOST_HOME` and `EGG_HOST_REPO_MAP` from repositories.yaml via scripts/build-host-repo-map.py. Update the Deployment Commands table in deployment.md to reflect the actual behavior. - Add a note in local-quickstart.md that `local_repos.paths` entries are used to auto-derive EGG_HOST_REPO_MAP at deploy time, so no manual editing of k8s overlays is needed. Authored-by: egg * docs: move make deploy details from table cell to subsection Address review feedback: the dense ~350-char table cell is now a concise one-liner linking to a dedicated subsection with the defaults, overrides, and prerequisite info. --------- Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * Fix #1993: default agent cwd to $EGG_REPO_PATH (#1996) * Fix #1993: default agent cwd to $EGG_REPO_PATH Sandbox agents started at HOME (/home/egg) instead of the repo directory (/home/egg/repos/<repo>), so early relative-path tool calls against .egg-state/... failed and agents wasted tokens rediscovering the layout. EGG_REPO_PATH was already in the container env; wire it in as the default cwd when no explicit cwd is passed. Covers both SDK paths that flow through run_agent_async (claude-sdk and the opt-in egg harness) and the harness factory's project CLAUDE.md lookup. Explicit cwd arguments still take precedence, and os.getcwd() remains the final fallback for local CLI use. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Address review feedback: empty-string defense, redundancy comment, stronger assertions - Guard against empty-string EGG_REPO_PATH with 'or None' in both client.py and harness_factory.py so an empty env var is treated the same as unset. - Add comment documenting intentional redundancy of the EGG_REPO_PATH fallback at harness_factory.py:168 for direct callers. - Strengthen cwd tests to assert on the ClaudeAgentOptions.cwd actually passed to query(), not just the logged value. * Add test for empty EGG_REPO_PATH treated as unset --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * Fix #1995: thread cursor through BRC wait endpoint to close wait→wait race (#2001) * Fix #1995: thread cursor through BRC wait endpoint to close wait→wait race `mcp__brc__wait_loop` could deadlock when peer ACKs arrived in the window between a returning `wait_loop` call and the subsequent `wait_loop` call. Each call snapped to a fresh stream tip (`from_tip=True` when `since_id is None`), so any event that fired in the gap was invisible to the producer. Port the cursor-threading contract from the host-side `wait_for_status_change` (docs/reference/agent-wait-patterns.md §7) to the agent-side message bus wait: - `/messages/wait` returns `cursor` on every response. On match: the ID of the last delivered message. On timeout: the current stream tip (via the existing `MessageStore.get_latest_id`). `null` only when the stream is empty. - `message_wait` surfaces `cursor` in its return dict. - `message_wait_loop` threads `cursor` into the next `since` between iterations, and surfaces the final cursor for agents to chain across successive tool invocations. - `mcp__brc__wait_for_event` / `mcp__brc__wait_loop` schema documents the `since` input as the cursor threading point. Documented the new contract in agent-wait-patterns.md §3 and agent-tools.md. Regression test `test_wait_cursor_threading_closes_ between_call_race` reproduces the #1995 scenario end-to-end through the endpoint. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Use 'is not None' for cursor guard to match comment semantics --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * Fix #1994: push via mcp__brc__propose and fix auto-filter ref collision (#2002) * Fix #1994: add push step to mcp__brc__propose; push by SHA in filtered_push MCP-only agents couldn't publish BRC artifacts because mcp__brc__propose only sent the CONSENSUS_PROPOSE signal — there was no push step and no mcp__brc__push tool. Direct git push was blocked by the gateway's concurrent-mode check, whose error steered agents toward the CLI. Even agents who guessed the right branch hit an auto-filter dead end: a sibling worktree's refs/heads/egg/<pid>/work directory-style ref blocks creating refs/heads/egg/<pid> as a leaf ref in the shared ref store, which execute_filtered_push did via update-ref before pushing. Changes: - sandbox/egg_agent_tools/push.py (new): shared consensus_push() helper pulled out of orch_cli._consensus_push so MCP and CLI surfaces share one implementation that routes through the gateway push API with the consensus_push marker set. The agent sandbox still never holds git credentials; all pushes go through the gateway sidecar. - sandbox/egg_agent_tools/tools/brc.py: mcp__brc__propose now takes a push boolean (default true) and calls consensus_push before the handler. Push failure short-circuits the handler so no PROPOSE is broadcast for an un-pushed artifact. - sandbox/egg_lib/orch_cli.py: _consensus_push is kept as a thin alias so the CLI and existing tests keep working. - gateway/filtered_push.py: drop the pre-push update-ref refs/heads/<branch>; push the rewritten tip SHA via a Callable[[str], ...] push_fn and let it build <tip>:refs/heads/<branch>. Update-ref becomes a best-effort post-push local-sync that logs and continues if a directory-style sibling ref blocks it. The remote push is the source of truth. - gateway/gateway.py: _inner_push builds the SHA-to-refspec push target. Concurrent-mode push enforcement now runs before push-target enforcement so BRC agents on per-role /work branches see the actionable mcp__brc__propose hint first. Both error messages point at mcp__brc__propose with the CLI as a fallback. - sandbox/agent-config/rules/mission.md, docs/guides/agent-teams.md, docs/guides/concurrent-execution.md, docs/architecture/git-isolation.md: primary BRC-push guidance now names mcp__brc__propose; CLI retained as fallback. - Tests updated for the new push_fn signature, the new error copy, and the relocated _consensus_push. Added three tests that pin the new push-then-propose behavior (push=true, push=false, push-failure short-circuit). * Address review feedback: propagate push errors, assert last_tip, add comment --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * refine(#1973): align analysis Open Questions with contract IDs The prose Open Questions section referenced 9 decisions + 11 feedback items, but the SDLC contract held different ones. This commit reconciles by: - registering the 4 missing decisions (LKG storage medium, dynamic- import handling, CI checkout depth, graph granularity) as decision-9..12 so every decision the analysis recommendations depend on is actually on the contract. - rewriting the Open Questions section to cite decision-1..12 and feedback-3 Q1..Q16 by ID, so the human reviewer can cross-check the prose against the machine-readable contract without ambiguity. No change to problem statement, current behavior, constraints, options, or recommended approach — only cross-reference cleanup. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Persist statefiles after refine phase * Persist HITL resolution after refine phase gate * architect: plan analysis for #1973 changeset-aware make test Architecture analysis for changeset-aware make test using grimp reverse import graph + gitignored per-branch LKG sidecar. Covers: - Scope (in/out of scope incl. integration/e2e/security kept out) - Decisions d1-d13 + Q1-Q16 from refine phase carried forward - Current-state survey (Makefile, pyproject, conftest layout, dynamic-import inventory, .gitignore) - Proposed directory layout: scripts/egg_test_selector/ package with baseline/diff/graph/selector/fallback/lkg/canary/logging/ introspect modules + tests/tools/test_selector_*.py - Algorithm walkthrough + Make recipe shapes - Alternatives considered (grimp chosen over testmon / path map / hand-rolled ast) with rejection rationale - Full risk list handed to risk_analyst - Task breakdown suggestions for task_planner - 19 concrete acceptance-criteria proposals Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * plan(#1973): changeset-aware `make test` via grimp + sidecar LKG Decompose the architect's refine-phase analysis into a single-PR implementation plan across six commit-phases: foundation (grimp dev dep + targeted gitignore), core selector script (grimp graph construction, baseline resolution, fallback triggers, LKG/canary /logging/--why), Makefile wiring (test narrow-default, test-all full suite, test-record-good manual override), CI switch to make test-all, tests (9 parametrized pytest modules under tests/tools/), and documentation (docs/guides/testing.md + CONTRIBUTING.md pointer). All 13 refine-phase HITL decisions and 16 feedback answers are carried forward as locked-in constraints (d3=grimp, d12=module- level, d9=non-tracked sidecar, d1=auto-after-test-all, d5=full- suite-on-non-py, d10=scan-during-graph-construction, d2=CI on make-test-all, d7=test-narrow-default + test-all-full, Q4=canary every-10th, Q5=intersect with PYTEST_ARGS, Q6=--why flag, Q15= stderr + JSON log, Q16=branch-only keying). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * plan(risk_analyst): risk assessment for issue 1973 (make test changeset-aware) Assesses 15 technical risks for the grimp-based changeset-aware test selector with sidecar LKG storage. Flags high-severity correctness risks around: - gateway/tests/conftest.py importlib-based loading (static-graph blind spot) - cross-root grimp invocation (AC-2 hinges on enumerating all roots) - PYTEST_ARGS parsing ambiguity (recommend EGG_TEST_SELECT=off env-var opt-out) - backward compatibility (audit `make test` call sites before merge) Overall risk: MEDIUM. Recommendation: PROCEED_WITH_MITIGATIONS. CI full-suite (decision-2) plus canary (Q4) caps worst-case blast radius. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * plan(#1973): address reviewer_plan NACK — 3 blocking + 10 non-blocking Blocking: 1. Grimp PACKAGES now includes the four test roots (tests, gateway.tests, orchestrator.tests, shared.tests) so the test-file mapping step can return non-empty. Create empty shared/tests/__init__.py (no-op for pytest collection but required for grimp package registration). TASK-5-4 strengthened to assert every test_*.py file is a node in the graph — staleness guard. 2. Fail-open exit contract. Wrap main() in try/except BaseException; on unhandled exception emit full test-root list on stdout + trace on stderr + exit 0. Only --record-good validation failure may exit non-zero. TASK-5-2 adds a synthetic grimp-failure regression test that pins this contract. 3. EGG_AGENT_ROLE read-only handling (Q13). Baseline resolution now checks the env var first: reviewer_* / refiner skip the sidecar entirely (never read, never write). Default (unset / coder / tester) uses the LKG-preferred path. TASK-5-3 parametrizes over reviewer_plan, refiner, coder, unset. Non-blocking adoptions: - schema_version: 1 added to selection JSON (TASK-2-4b + TASK-6-1). - grimp pin tightened to >=3.14,<4.0 for Rust backend (TASK-1-1). - --record-good sha validation: 40-char hex + cat-file -e + ancestor (TASK-2-4a); distinct non-zero exits per validation kind. - as_package mixed strategy: True for __init__.py edits, False for leaf edits (algorithm §6 + TASK-2-1 + TASK-5-1 case). - Q2 shared/tests coarse-rule rationale recorded. - TASK-2-4 split into 2-4a (LKG + canary) and 2-4b (logging + --why) for reviewability. - Stacked -m "not functional" + user -m composition test (TASK-5-4). - Detached-HEAD stderr notice + test coverage (§8, TASK-2-2, TASK-5-3). - New TASK-5-5: subprocess-level end-to-end test against a synthetic mini-monorepo that exercises make test / make test-all / fallback through the real Makefile — closes the gap between unit tests and manual verification. - Task-dependency graph refined to reflect TASK-4-1 depending specifically on TASK-3-2 (not all of Phase 3). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * plan(#1973): address reviewer_plan v2 NACK — R1 + R2 + R6/R7/R10/R14/R5 Blocking fixes from risk_analyst cross-check: 1. Risk_analyst R1 (gateway/tests importlib test-loader). gateway/tests/conftest.py's _load_module_with_replaced_imports hides test→production edges from grimp — a change to gateway/policy.py would have selected zero tests. Added hard-coded fallback trigger in TASK-2-3: any `gateway/*.py` production edit (not under gateway/tests/) widens to full suite with explicit trigger "gateway source change (importlib test-loader)". Test case in TASK-5-2 covers positive and negative (gateway/tests/* edits do NOT fire). Blind-spot documented in TASK-6-1. 2. Risk_analyst R2 (source-file staleness beyond tests). Added runtime source-file integrity check to TASK-2-3: walk every non-test .py under gateway/shared/orchestrator/sandbox and confirm each is a grimp graph node; widen to full suite with trigger "source file missing from graph: <path>" on any missing file. This catches PACKAGES drift in production, not just the TASK-5-4 CI test. Tested in TASK-5-2. Non-blocking adoptions (also from risk_analyst): - R5 ambiguous PYTEST_ARGS classifier: TASK-5-4 gets a golden-case matrix pinning bypass/intersect/ambiguous classification for flag values like `--hypothesis-seed=gateway/tests/helper.py`. - R6 grimp cache: TASK-2-1 configures `cache_dir=.egg-state/grimp-cache/` for warm-graph reuse; TASK-1-2 gitignores the directory. - R7 selection/LKG accumulation: explicit decision to keep architect's no-pruning stance; `rm -rf` documented as recovery in manual_steps and TASK-6-1 housekeeping section. - R10 backward-compat audit: inlined into Risk summary (only three call sites; all handled: workflows → TASK-4-1, CONTRIBUTING → TASK-6-2, help → TASK-3-2). - R14 `.egg-readonly` marker: TASK-2-2 detects EITHER EGG_AGENT_ROLE=reviewer_*/refiner OR .egg-readonly marker file in repo root. TASK-5-3 parametrizes both signals. - TASK-5-5 runtime relaxed to <60s + @pytest.mark.slow tag so it deselects on inner-loop runs. Risk summary section now references .egg-state/agent-outputs/1973-risk_analyst-output.json and enumerates R1-R15 mitigations explicitly. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * plan(#1973): adopt reviewer_plan v3 ACK polish items Cheap quality improvements from reviewer_plan's v3 ACK (all marked non-blocking / forward-looking): - "What does NOT change" list now names gateway/tests/conftest.py so reviewers reading the PR diff don't wonder about conftest changes. - Task Dependencies section confirms R1 + R2 mitigations landed inside existing tasks without changing the graph. - TASK-2-1 argparse enumeration now lists `--patch-selection-json --head <sha> --pytest-ms <int>` for discoverability (implementation still in TASK-2-4b; Makefile wrapper invokes it in TASK-3-1). - TASK-5-2 promotes the fail-open verification-by-removal comment from a suggestion to a required AC line — reviewer can gate on it at PR time. - TASK-6-1 AC now explicitly requires Section 7 (Known Limits) to name the gateway importlib test-loader blind spot and the `gateway/*.py → full suite` mitigation with a pointer to `_load_module_with_replaced_imports`. R5 env-var opt-out (EGG_TEST_SELECT=off) deferred as forward-looking / follow-up-issue material per reviewer guidance. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Persist statefiles after plan phase * Persist HITL resolution after plan phase gate * implement(#1973): foundation — grimp dev dep + sidecar gitignore TASK-1-1 + TASK-1-2 of the plan. Add `grimp>=3.14,<4.0` to the `[project.optional-dependencies].dev` section in pyproject.toml. v3.14 introduced the Rust-backed graph builder that is meaningfully faster on this monorepo's ~770 source files; the inline TOML comment locks the floor against future maintainers loosening it without re-benchmarking. uv.lock is intentionally not regenerated in this commit — the agent sandbox lacks `uv` and outbound PyPI access so `uv lock` cannot run locally. CI's `uv sync --extra dev` step (no `--frozen`/`--locked`) will reconcile the lockfile when the workflow runs against this branch. The reviewer should expect a follow-up commit (or the CI job's lockfile delta) to land the resolved grimp + transitive package entries; the static analysis we ship in TASK-2-x is fail-open so the absence of grimp at runtime degrades to "full suite" rather than a hard error (matches the documented fail-open exit contract). Append three targeted entries to .gitignore under a "changeset-aware test selection" section header: .egg-state/last-known-good/ per-branch sidecar LKG sha files .egg-state/selection/ per-invocation JSON decision logs .egg-state/grimp-cache/ grimp's on-disk graph cache `.egg-state/` itself stays tracked — drafts, contracts, reviews, brc-history, oversight, agent-outputs all live there and remain under version control. Only these three subdirectories flip to ignored. * docs(#1973): add testing guide for changeset-aware make test Adds the canonical testing guide that documents the changeset-aware make test model planned in #1973: grimp-backed reverse import-graph selection, sidecar LKG semantics, the full fallback-trigger list (including the gateway/*.py importlib blind-spot mitigation), --why introspection, the JSON selection-log schema (schema_version=1), role-aware read-only behavior, the fail-open exit contract, no-pruning housekeeping, and a troubleshooting section. - New: docs/guides/testing.md (10 sections per TASK-6-1) - CONTRIBUTING.md: one-line pointer to the new guide (TASK-6-2) - docs/index.md: index entry under the Guides table - README.md: distinguish make test (narrow default) from make test-all (full suite), with a pointer to the testing guide All ten sections required by TASK-6-1 are present: overview, how selection works, sidecar LKG, fallback triggers, introspection, role-aware behavior + fail-open, known limits (gateway importlib test-loader called out as a specific blind spot pointing to gateway/tests/conftest.py's _load_module_with_replaced_imports), CI, housekeeping, troubleshooting. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * implement(#1973): scripts/select_tests.py — changeset-aware test selector TASK-2-1, TASK-2-2, TASK-2-3, TASK-2-4a, TASK-2-4b of the plan. Adds scripts/select_tests.py — a single-file standalone CLI that narrows `make test` to the transitive reverse-import closure of files touched since a Last-Known-Good (LKG) commit, with a fail-open contract that ALWAYS degrades to the full suite on any analysis failure rather than blocking iteration. Highlights from the plan (sections "Approach" / "Architecture" of .egg-state/drafts/1973-plan.md): * PACKAGES — single source of truth covering all 15 source packages plus the four test roots (tests, gateway.tests, orchestrator.tests, shared.tests). TASK-5-4 will lock this down with an exhaustive "every test_*.py is a node" check. * shared/tests/__init__.py — empty file added so grimp can register the package; pytest already treats shared/tests/ as a testpath, so the empty init is a no-op for collection. * Baseline resolution (TASK-2-2) — three precedence layers with the read-only override (Q13/R14) on top: EGG_AGENT_ROLE starting with `reviewer_` / equal to `refiner` OR the .egg-readonly marker file SKIPS the LKG sidecar entirely; else the per-branch sidecar (validated as 40-hex AND ancestor of HEAD); else `git merge-base HEAD origin/$BASE_BRANCH`; else UNRESOLVABLE → full-suite trigger. * changed_files — union of `git diff --name-only <base>...HEAD` AND `git status --porcelain` (uncommitted always participates so a dirty tree cannot have a clean LKG effect). Detached HEAD emits the documented stderr notice and falls through to base- branch. * Fallback triggers (TASK-2-3) — explicit, priority-ordered: canary → unresolvable baseline → LKG-not-ancestor → empty diff → conftest → shared/tests → Makefile / pyproject.toml / uv.lock / .python-version / workflow → gateway/*.py (R1 importlib test-loader blind spot) → non-.py change → source- file staleness (R2) → unresolvable module → dynamic-import reachability. Each trigger is a distinct stderr string so operators read the most-informative reason rather than a generic catch-all. * Mixed `as_package` strategy in reverse_closure() — `__init__.py` edits widen to the whole package; leaf-module edits narrow to just that module's downstream. * LKG sidecar I/O (TASK-2-4a) — atomic tempfile + os.replace at `.egg-state/last-known-good/<branch>.sha`; `--record-good` validates 40-hex regex AND `git cat-file -e` AND `git merge-base --is-ancestor` (refuses non-zero on any failure); detached HEAD / read-only role / missing branch skip-with-notice (exit 0). Per-branch canary counter at `<branch>.canary` fires on every 10th narrow invocation, resets after fire AND on `--full-suite`. * Structured logging (TASK-2-4b) — stderr one-liner + JSON record at `.egg-state/selection/<head>.json` carrying schema_version=1 plus baseline, branch, mode, trigger, selected_count/total_count, compute_ms (pytest_ms patched in later by the Makefile wrapper), timestamp, canary_fired, changed_files / changed_modules / dynamic_import_seeds_hit. * `--why <test>` introspection — uses grimp.find_shortest_chain to print the import path from any changed module to the named test; falls back gracefully when the test isn't selected or no path exists. * `--patch-selection-json --head <sha> --pytest-ms <int>` — write-side helper for the Makefile `test` wrapper to append `pytest_ms` to the existing JSON record after pytest returns. * Fail-open wrapper — main() catches BaseException, prints the traceback to stderr, emits the full test-root list on stdout, and exits 0. A selector bug must NEVER block iteration — correctness is preserved by widening to the full suite. Only `--record-good`'s explicit RecordGoodValidationError path is allowed to exit non-zero (because silent success on bad input would poison LKG). * pyproject.toml — adds a mypy override for the `grimp` import so mypy --strict on scripts/select_tests.py passes; grimp ships no type stubs. Branch-name caveat: `git symbolic-ref` is blocked by the egg gateway sidecar's git allowlist, so `_git_current_branch` uses `git rev-parse --abbrev-ref HEAD` and canonicalises the literal "HEAD" string back to None for detached HEAD. Same observable behaviour, different command. Lint: `ruff check` + `ruff format` + `mypy --strict scripts/select_tests.py` all clean. Tests live in TASK-5-* and land in a follow-up commit by the tester role. Auto-Filtered: true * implement(#1973): Makefile narrow-default + CI full-suite switch TASK-3-1, TASK-3-2, TASK-4-1 of the plan. Makefile: * `make test` is rewritten as the changeset-aware narrow default. The recipe captures `select_tests.py` stdout into a tempfile, runs pytest with the selected paths plus the existing `-v -m "not functional" $(PYTEST_ARGS)` flags, and surfaces pytest's exit code untouched. Empty selection prints a clear "no tests selected" message and exits 0 without calling pytest. Selector failure (non-zero exit, which only happens on argparse syntax errors thanks to the fail-open contract) falls back to the full test-root list. After pytest returns, the recipe times the wall-clock pytest duration and invokes `select_tests.py --patch-selection-json --head <sha> --pytest-ms <int>` to append `pytest_ms` to the existing `.egg-state/selection/<head>.json` record. LKG sidecar is NEVER updated by `make test` (Q12). * `make test-all` is the full-suite escape hatch. Runs the historical `pytest tests/ gateway/tests/ orchestrator/tests/ shared/tests/ -v -m "not functional"` command, then on green exit calls `select_tests.py --record-good` to atomically write the LKG sidecar. On red exit, the sidecar is NOT updated (partial / failing runs cannot become a future LKG baseline) and the failure exit code surfaces cleanly. * `make test-record-good` is the manual override — unconditionally writes the LKG sidecar (with full validation on the sha: 40-hex regex + cat-file existence + ancestor of HEAD). * `make help` lists `test`, `test-all`, `test-record-good` under the CI-checks section with one-line descriptions; the .PHONY directive picks up the two new targets. CI (.github/workflows/test.yml): * The unit job's "Run unit tests" step switches from `make test PYTEST_ARGS=...` to `make test-all PYTEST_ARGS=...` so the 80% coverage gate stays enforced unchanged. Narrowing in CI would compute coverage over only the selected tests and silently drop aggregate coverage below the threshold — decision-d2 explicitly forbids that. Coverage args (`--cov=gateway --cov=shared --cov=sandbox --cov-report= term-missing --cov-fail-under=80`) are preserved verbatim. No fetch-depth change (full-suite path doesn't need the base-branch ref); no matrix change; security and aggregate jobs are byte-identical. Auto-Filtered: true * implement(#1973): address reviewer_contract polish — reverse_closure aligned tuples Two non-blocking suggestions from reviewer_contract on the v1 proposal that are cheap to fix in-line: 1. reverse_closure() now takes a single iterable of aligned (module, path) tuples instead of two parallel lists zipped internally with strict=False. This eliminates the theoretical possibility of __init__.py detection misfiring if a future caller ever passes mismatched lengths. Both call sites (`explain_why`, `_run_narrow_or_fallback`) build the pairs in a single loop now, with the leftover `changed_modules_list` derived from the tuple set so JSON logging is unchanged. 2. The gateway/*.py importlib trigger gains an inline doc comment naming the "flat layout" assumption explicit: gateway/ production source is currently FLAT (every .py file directly under gateway/<file>.py). If that ever changes (gateway/api/foo.py etc.), the `"/" not in raw_path[len("gateway/"):]` guard would NOT widen on subdirectory edits — extend the guard at that point. Lint clean: ruff + ruff format + mypy --strict still pass. * implement(#1973): address reviewer_code NACK — sys.path + PYTEST_ARGS + __import__ + JSON reviewer_code NACK on v2 (commit ff48695b3) flagged four blockers plus a handful of non-blocking polish items. Addressed below. BLOCKING #1 — orchestrator/sandbox bare-name imports: build_graph()'s sys.path tweak previously only added `root` and `root/shared`. This left grimp's resolver unable to follow the bare-name `from models import ...`, `from egg_lib.config import ...`, `from egg_agent_tools import ...` patterns that orchestrator/, sandbox/, sandbox/tools/, and tests/ rely on at runtime — every such import was filtered as external by `include_external_packages=False`, leaving the graph empty of test→production edges for those source roots. Fix: mirror the per-conftest sys.path injections. Now adding root, root/shared, root/orchestrator, root/sandbox, root/sandbox/tools, and root/config — exactly what tests/conftest.py:13-16 and orchestrator/tests/conftest.py:25-29 inject. Inline doc comment at the call site lists each entry's source-of-truth conftest. Belt-and-braces: a "no downstream tests for changed module" fallback trigger fires when narrowing IS possible (graph built, no other trigger fired) but the closure for any non-test changed module returns zero downstream tests. This catches any remaining bare-name resolution gap (e.g., grimp-version-specific resolver quirks) and widens to full suite with the explicit trigger string `no downstream tests for changed module: <id>` rather than silently selecting zero tests. BLOCKING #2 — PYTEST_ARGS bypass was dead code: pytest_args_have_explicit_path() existed but was never called. docs/guides/testing.md documented `mode: "bypass"` that the selector could never emit. Plan §7 explicitly required this. Fix: - Selector reads PYTEST_ARGS_RAW env var (shlex-split, fail-open on parse error) and runs the path-vs-flag classifier BEFORE the fallback evaluator. On match: emits nothing on stdout, writes a `mode="bypass"` selection record with trigger "PYTEST_ARGS explicit path". - Makefile `test` recipe sets PYTEST_ARGS_RAW="$(PYTEST_ARGS)" when invoking the selector, then checks the JSON record for `"mode": "bypass"` to decide between `pytest <selected> -v -m "not functional" $(PYTEST_ARGS)` (narrow / full-suite) and `pytest -v -m "not functional" $(PYTEST_ARGS)` (bypass — pytest sees only the user's args). BLOCKING #3 — __import__ regex was anchored to start-of-string: `r"^\s*__import__\s*\("` with default flags only matches at start-of-STRING (not start-of-line), so it never matched real callers like `mod = __import__(name)` or `_X = __import__("re").compile(...)`. Fix: change to `r"\b__import__\s*\("` — matches the token anywhere in the file. Inline comment names the bug + the examples that now match. BLOCKING #4 — uv.lock not regenerated: Sandbox limitation; covered by PMC-2 in the v2 proposal. NON-BLOCKING addressed in this commit: * Read-only roles no longer write the canary counter (was: always wrote; now: gated on `not is_role_readonly`). Sidecar dir is per-branch and shouldn't be mutated by cross-sandbox roles. * Full-suite-fallback JSON records now include changed_modules_list + dynamic_import_seeds_hit (computed once before the trigger evaluator and reused on both branches). Telemetry consumers no longer lose the "why" detail when a fallback fires. * cannot-resolve-HEAD path now writes a best-effort JSON record (with head=000…0) so the telemetry trail is consistent across all fallback paths. * `_TEST_ROOT_PREFIXES` simplified to a single set-union (POSIX vs non-POSIX duplication factored out). * `conftest.py` match tightened to literal-or-`/conftest.py` so files like `myconftest.py` no longer false-fire. * `--record-good` (called by `make test-all` on green) now resets the canary counter, so the developer doesn't get a canary-fired full-suite re-run on the very next `make test` after they already exercised the full suite. LINT/TYPECHECK: ruff check + ruff format --check + mypy --strict on scripts/select_tests.py — clean. Smoke: PYTEST_ARGS_RAW="tests/test_python_syntax.py" python3 scripts/select_tests.py emits zero stdout + writes mode=bypass JSON record with trigger "PYTEST_ARGS explicit path". PYTEST_ARGS_RAW="-k foo" emits the normal full-suite fallback (flag value, not a positional path arg). * implement(#1973): thread repo_root through sidecar I/O (tester NACK) tester's v3 NACK blocking #2: write_sidecar_lkg / read_sidecar_lkg / write_canary_count / read_canary_count and their helpers _sidecar_path / _canary_path took no repo_root parameter, so writes always landed under os.getcwd(). The caller-side `record_good(..., repo_root=...)` accepted the parameter but silently dropped it before the sidecar write — plan §8 says LKG sidecar lives under the repo root, not the caller's CWD. A subagent invoking the script from a non- repo-root CWD would silently land the sidecar in the wrong place and never advance LKG. Fix: - new `_resolve_root(repo_root)` helper centralises the `repo_root or _git_repo_root()` fallback so every call site goes through the same default. - _sidecar_path / _canary_path / read_sidecar_lkg / write_sidecar_lkg / read_canary_count / write_canary_count all gain a `repo_root: Path | None = None` parameter and resolve all paths under the repo root. - call sites in record_good, resolve_baseline, lkg_is_stale, _run_narrow_or_fallback, and the --full-suite reset path all thread repo_root through. - inline doctring on read_sidecar_lkg names the bug + fix so future callers don't reintroduce it. LINT/TYPECHECK: ruff + mypy --strict — clean. Smoke: from /tmp, `python3 /home/egg/repos/egg/scripts/ select_tests.py --record-good --sha <head>` writes .egg-state/last-known-good/<branch>.sha under /home/egg/repos/egg, NOT under /tmp/ (gateway-blocked invocation verified separately — local-CWD test under /tmp confirmed the repo-root fallback is engaged). * test(#1973): add tests/tools/test_select_tests_*.py for changeset-aware selector TASK-5-1 through TASK-5-5 of the implement-phase plan. Twelve new files in tests/tools/ exercise scripts/select_tests.py, the changeset- aware test selector, plus a shared helper module and a conftest.py that loads the selector and patches its git invocations to bypass the sandbox gateway wrapper. Coverage by task: TASK-5-1 test_select_tests_graph.py Synthetic mini-monorepo grimp graph cases — leaf vs mid-layer change, cross-package edges, TYPE_CHECKING imports, mixed `as_package` strategy (`__init__.py` vs leaf). Skips gracefully when grimp isn't installed (the sandbox doesn't have grimp; CI does via `uv sync --extra dev`). TASK-5-2 test_select_tests_fallbacks.py Every fallback trigger from algorithm §5: canary, unresolvable baseline, LKG-not-ancestor, empty diff, conftest at any level, shared/tests/, Makefile, pyproject.toml, uv.lock, .python-version, workflow file, gateway/*.py R1 mitigation (with negative case for gateway/tests/), source-file staleness guard (R2), unresolvable module path, dynamic-import reachability via upstream. Plus the fail-open regression test (TASK-2-1's blanket try/except), including the inline AC-required note on how to verify the contract by removing the try/except. TASK-5-3 test_select_tests_lkg.py test_select_tests_baseline.py test_select_tests_canary.py Sidecar atomic-write semantics (concurrent reader sees no half-written file), `read_sidecar_lkg` validation against malformed contents, --record-good validation failures (regex / cat-file / ancestor) each with distinct exit codes, --record-good no-op paths (detached HEAD, read-only role, marker file), per-branch isolation, baseline resolution across all EGG_AGENT_ROLE values + .egg-readonly marker, the BASE_BRANCH env override, the lkg_is_stale helper, the changed_files diff helper (committed + uncommitted, renames, empty-tree), canary modulo contract (parametrized), counter increment / fire / reset semantics, --full-suite resets the counter. TASK-5-4 test_select_tests_pytest_args.py test_select_tests_why.py test_select_tests_logging.py test_select_tests_monorepo.py PYTEST_ARGS classifier — bypass class (positional test-root path), intersect class (pure flags + stacked-marker composition), ambiguous class (R5 — flag values like `--hypothesis-seed=...`), mixed (positional wins). --why introspection wired through `_main_inner` (skips without grimp). Selection-record JSON envelope — every documented key including schema_version=1, baseline {sha, source}, branch-can-be-null, ISO-8601 timestamp, pytest_ms is null initially, atomic-write replaces. patch_selection_record handles missing/malformed files with stderr notice + exit 0. Stderr decision- line format pinned to a regex for both narrow and full-suite cases. Monorepo staleness guard against the live PACKAGES constant (skips without grimp) — every test_*.py is a graph node, every source root yields nodes, gateway is marked as a dynamic-import seed. TASK-5-5 test_select_tests_e2e.py Subprocess-level invocations of the selector — default mode exits 0 on a real diff, --full-suite emits the four test-root paths and resets canary, --record-good writes the sidecar, --record-good --sha <bad> exits non-zero, --patch-selection-json appends pytest_ms, --patch-selection-json missing args is fail-open, --help lists all flags, unknown flag exits non-zero. Bypasses the sandbox gateway git wrapper by prepending a private bin dir with a symlink to /opt/.egg-internal/git on PATH for the subprocess. Test infrastructure: - `_select_tests_helpers.py`: shared module loader (SourceFileLoader pattern), real-git wrapper (gateway-bypass), git fixture builders (init_git_repo, commit_file), in-process chdir context manager. - `conftest.py`: `real_git` fixture that monkeypatches `selector._run_git` to use the real /opt/.egg-internal/git binary so synthetic tmp_path repos work despite the sandbox's git wrapper. Local verification: $ python3 -m pytest tests/tools/test_select_tests_*.py ====== 173 passed, 3 skipped, 1 warning in 2.17s ====== 3 skipped: TASK-5-1 graph + TASK-5-4 monorepo + TASK-5-4 why all pytest.importorskip on grimp (not installed in this sandbox; CI picks them up via the dev extras). Open blockers in the coder's proposal flagged via NACK and HANDOFF: 1. .github/workflows/test.yml is DELETED on origin/egg/issue-1973 instead of modified per TASK-4-1. 2. write_sidecar_lkg / write_canary_count ignore the repo_root parameter and write CWD-relative. 3. shared/tests/__init__.py breaks pytest collection with PYTHONPATH=shared (which the Makefile sets). Tests above currently work around 2 by monkeypatch.chdir; once the coder re-proposes with the fixes they can drop the workarounds and validate the spec directly. * Persist statefiles after implement phase * Remove ephemeral agent-output handoff artifacts (#1731) * Address review feedback: fix tests, restore CI, regenerate lockfile - Restore .github/workflows/test.yml from origin/main and swap make test -> make test-all (B1 / decision-d2) - Create shared/tests/__init__.py for grimp package registration (B3) - Configure pytest importlib mode + consider_namespace_packages to resolve conftest collision between tests/ and shared/tests/ - Regenerate uv.lock with grimp>=3.14 entry (B2) - Fix _build_synthetic_graph() in graph tests: add sys.path insertion and sys.modules isolation so grimp resolves synthetic packages instead of pytest-cached ones (C4 / TASK-5-1) - Fix reverse_closure() call signature to pass module_path_pairs as tuples instead of separate lists - Add EGG_AGENT_ROLE isolation to real_git fixture so sandbox- inherited role values don't cause record_good() to short-circuit (C5 / TASK-5-3) - Fix vacuous tautology assertion in test_select_tests_logging.py (C1) - Replace deprecated SourceFileLoader.load_module() with importlib.util.spec_from_file_location() (C2) * Fix ImportPathMismatchError: remove shared/tests/__init__.py With PYTHONPATH=shared (set by Makefile test targets), this file creates a second 'tests' package visible from the shared/ path entry, colliding with the top-level tests/ package. Removing it makes shared/tests/ a namespace package again, which grimp >=3.14 handles natively and pytest importlib mode discovers correctly. * Fix ImportPathMismatchError: remove gateway/tests and orchestrator/tests __init__.py With PYTHONPATH=shared:gateway:orchestrator (set by Makefile test targets), gateway/tests/__init__.py creates a second 'tests' package visible from the gateway/ path entry, colliding with the top-level tests/ package and causing ImportPathMismatchError for conftest.py. Same root cause as shared/tests/__init__.py (fixed in f0fc437). Remove both gateway/tests/ and orchestrator/tests/ __init__.py to make them namespace packages, which pytest importlib mode discovers correctly with consider_namespace_packages=true. * Fix gateway.tests collection: set __path__ on loaded gateway module After gateway/tests/__init__.py was removed (to fix ImportPathMismatchError under PYTHONPATH=shared:gateway:orchestrator), pytest collection of gateway/tests/test_*.py started failing with: AttributeError: module 'gateway' has no attribute '__path__' The gateway/tests/conftest.py registers gateway/gateway.py (a single file FastAPI app) as sys.modules['gateway'], which replaces the real gateway package. With gateway/tests/ now a namespace subpackage, pytest's importlib-mode collector (consider_namespace_packages=true) walks up through gateway to resolve gateway.tests.test_*, and needs gateway.__path__ to find subpackages. The single-file module had no __path__, causing 64 collection errors. Set gateway.__path__ = [GATEWAY_DIR] right after the module is loaded so the namespace subpackage gateway.tests resolves correctly. Tests that do 'import gateway' to access the FastAPI app still get the same module object. * Set __spec__ on loaded gateway module for find_spec compat The previous fix set gateway.__path__ so pytest could collect tests under gateway/tests/, but importlib.util.find_spec("gateway") still raised "gateway.__spec__ is None" because the manually-constructed ModuleType has no spec. This broke tests/tools/test_select_tests_monorepo.py: scripts/select_tests.py calls grimp.build_graph("gateway", ...), and grimp resolves package locations via importlib.util.find_spec, which raises ValueError when the target module's __spec__ is None. Construct a ModuleSpec with submodule_search_locations pointing at GATEWAY_DIR and assign it to gateway.__spec__ so find_spec returns a valid package spec. * Remove silently-ignored import_mode pytest setting (N1) import_mode is not registered as an INI option — pytest emitted PytestConfigWarning: 'Unknown config option: import_mode' on every invocation, and prepend mode (the default) was used regardless. The PR description and conftest comment claimed importlib mode was active but it was not. Resolution: remove the dead setting and update both comments to match runtime behavior. consider_namespace_packages=true alone is sufficient to discover shared/tests, gateway/tests, and orchestrator/tests as namespace subpackages. We deliberately stay on prepend mode because scripts/select_tests.py monorepo tests build a real grimp graph and importlib mode triggers grimp.NotATopLevelModule for gateway.tests and orchestrator.tests subpackages. Verified: PytestConfigWarning no longer fires; all 294 tools tests pass (including the 9 monorepo tests that errored when --import-mode=importlib was actually applied). --------- Co-authored-by: egg-orchestrator <egg@localhost> Co-authored-by: egg <egg@example.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> 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>

…dater] (#1992) * docs: update deploy docs for EGG_HOST_REPO_MAP auto-discovery Document changes introduced by a8b1324 (#1991): - `make deploy` now requires `envsubst` (GNU gettext) and auto-derives `EGG_HOST_HOME` and `EGG_HOST_REPO_MAP` from repositories.yaml via scripts/build-host-repo-map.py. Update the Deployment Commands table in deployment.md to reflect the actual behavior. - Add a note in local-quickstart.md that `local_repos.paths` entries are used to auto-derive EGG_HOST_REPO_MAP at deploy time, so no manual editing of k8s overlays is needed. Authored-by: egg * docs: move make deploy details from table cell to subsection Address review feedback: the dense ~350-char table cell is now a concise one-liner linking to a dedicated subsection with the defaults, overrides, and prerequisite info. --------- Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>

* Initialize SDLC contract for issue #1973 * refine(#1973): analysis for changeset-aware `make test` Draft analysis covering selection mechanism tradeoffs (grimp vs custom AST vs pytest-testmon vs hybrid), LKG storage/update/merge semantics, dynamic-import handling, CI coverage-gate interaction, shallow-checkout constraints, and target naming. Recommends grimp-based static reverse import graph with a non-tracked sidecar LKG, flipping the tracked-file default from the issue proposal. Registers 9 HITL decisions and 12 open-ended feedback questions via egg-contract for human input in the refine-approval step. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * refine(#1973): address reviewer_refine non-blocking nits - Tighten `gateway/gateway.py:309-317` → `:309-322` (the spec_from_file_location block runs through exec_module + except at 321-322). - Soften the testmon weak-spot claim in Option C: testmon sees in-process dynamic imports via coverage.py; the true miss-mode is subprocess-crossing coverage, not in-process importlib. - Flag the extra `feedback-1/Q10` (Fallback-trigger list completeness) in the prose intro so the prose-vs-contract drift is explicit. No recommendation change. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * docs: update deploy docs for EGG_HOST_REPO_MAP auto-discovery [doc-updater] (#1992) * docs: update deploy docs for EGG_HOST_REPO_MAP auto-discovery Document changes introduced by a8b1324 (#1991): - `make deploy` now requires `envsubst` (GNU gettext) and auto-derives `EGG_HOST_HOME` and `EGG_HOST_REPO_MAP` from repositories.yaml via scripts/build-host-repo-map.py. Update the Deployment Commands table in deployment.md to reflect the actual behavior. - Add a note in local-quickstart.md that `local_repos.paths` entries are used to auto-derive EGG_HOST_REPO_MAP at deploy time, so no manual editing of k8s overlays is needed. Authored-by: egg * docs: move make deploy details from table cell to subsection Address review feedback: the dense ~350-char table cell is now a concise one-liner linking to a dedicated subsection with the defaults, overrides, and prerequisite info. --------- Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * Fix #1993: default agent cwd to $EGG_REPO_PATH (#1996) * Fix #1993: default agent cwd to $EGG_REPO_PATH Sandbox agents started at HOME (/home/egg) instead of the repo directory (/home/egg/repos/<repo>), so early relative-path tool calls against .egg-state/... failed and agents wasted tokens rediscovering the layout. EGG_REPO_PATH was already in the container env; wire it in as the default cwd when no explicit cwd is passed. Covers both SDK paths that flow through run_agent_async (claude-sdk and the opt-in egg harness) and the harness factory's project CLAUDE.md lookup. Explicit cwd arguments still take precedence, and os.getcwd() remains the final fallback for local CLI use. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Address review feedback: empty-string defense, redundancy comment, stronger assertions - Guard against empty-string EGG_REPO_PATH with 'or None' in both client.py and harness_factory.py so an empty env var is treated the same as unset. - Add comment documenting intentional redundancy of the EGG_REPO_PATH fallback at harness_factory.py:168 for direct callers. - Strengthen cwd tests to assert on the ClaudeAgentOptions.cwd actually passed to query(), not just the logged value. * Add test for empty EGG_REPO_PATH treated as unset --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * Fix #1995: thread cursor through BRC wait endpoint to close wait→wait race (#2001) * Fix #1995: thread cursor through BRC wait endpoint to close wait→wait race `mcp__brc__wait_loop` could deadlock when peer ACKs arrived in the window between a returning `wait_loop` call and the subsequent `wait_loop` call. Each call snapped to a fresh stream tip (`from_tip=True` when `since_id is None`), so any event that fired in the gap was invisible to the producer. Port the cursor-threading contract from the host-side `wait_for_status_change` (docs/reference/agent-wait-patterns.md §7) to the agent-side message bus wait: - `/messages/wait` returns `cursor` on every response. On match: the ID of the last delivered message. On timeout: the current stream tip (via the existing `MessageStore.get_latest_id`). `null` only when the stream is empty. - `message_wait` surfaces `cursor` in its return dict. - `message_wait_loop` threads `cursor` into the next `since` between iterations, and surfaces the final cursor for agents to chain across successive tool invocations. - `mcp__brc__wait_for_event` / `mcp__brc__wait_loop` schema documents the `since` input as the cursor threading point. Documented the new contract in agent-wait-patterns.md §3 and agent-tools.md. Regression test `test_wait_cursor_threading_closes_ between_call_race` reproduces the #1995 scenario end-to-end through the endpoint. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Use 'is not None' for cursor guard to match comment semantics --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * Fix #1994: push via mcp__brc__propose and fix auto-filter ref collision (#2002) * Fix #1994: add push step to mcp__brc__propose; push by SHA in filtered_push MCP-only agents couldn't publish BRC artifacts because mcp__brc__propose only sent the CONSENSUS_PROPOSE signal — there was no push step and no mcp__brc__push tool. Direct git push was blocked by the gateway's concurrent-mode check, whose error steered agents toward the CLI. Even agents who guessed the right branch hit an auto-filter dead end: a sibling worktree's refs/heads/egg/<pid>/work directory-style ref blocks creating refs/heads/egg/<pid> as a leaf ref in the shared ref store, which execute_filtered_push did via update-ref before pushing. Changes: - sandbox/egg_agent_tools/push.py (new): shared consensus_push() helper pulled out of orch_cli._consensus_push so MCP and CLI surfaces share one implementation that routes through the gateway push API with the consensus_push marker set. The agent sandbox still never holds git credentials; all pushes go through the gateway sidecar. - sandbox/egg_agent_tools/tools/brc.py: mcp__brc__propose now takes a push boolean (default true) and calls consensus_push before the handler. Push failure short-circuits the handler so no PROPOSE is broadcast for an un-pushed artifact. - sandbox/egg_lib/orch_cli.py: _consensus_push is kept as a thin alias so the CLI and existing tests keep working. - gateway/filtered_push.py: drop the pre-push update-ref refs/heads/<branch>; push the rewritten tip SHA via a Callable[[str], ...] push_fn and let it build <tip>:refs/heads/<branch>. Update-ref becomes a best-effort post-push local-sync that logs and continues if a directory-style sibling ref blocks it. The remote push is the source of truth. - gateway/gateway.py: _inner_push builds the SHA-to-refspec push target. Concurrent-mode push enforcement now runs before push-target enforcement so BRC agents on per-role /work branches see the actionable mcp__brc__propose hint first. Both error messages point at mcp__brc__propose with the CLI as a fallback. - sandbox/agent-config/rules/mission.md, docs/guides/agent-teams.md, docs/guides/concurrent-execution.md, docs/architecture/git-isolation.md: primary BRC-push guidance now names mcp__brc__propose; CLI retained as fallback. - Tests updated for the new push_fn signature, the new error copy, and the relocated _consensus_push. Added three tests that pin the new push-then-propose behavior (push=true, push=false, push-failure short-circuit). * Address review feedback: propagate push errors, assert last_tip, add comment --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * refine(#1973): align analysis Open Questions with contract IDs The prose Open Questions section referenced 9 decisions + 11 feedback items, but the SDLC contract held different ones. This commit reconciles by: - registering the 4 missing decisions (LKG storage medium, dynamic- import handling, CI checkout depth, graph granularity) as decision-9..12 so every decision the analysis recommendations depend on is actually on the contract. - rewriting the Open Questions section to cite decision-1..12 and feedback-3 Q1..Q16 by ID, so the human reviewer can cross-check the prose against the machine-readable contract without ambiguity. No change to problem statement, current behavior, constraints, options, or recommended approach — only cross-reference cleanup. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Persist statefiles after refine phase * Persist HITL resolution after refine phase gate * architect: plan analysis for #1973 changeset-aware make test Architecture analysis for changeset-aware make test using grimp reverse import graph + gitignored per-branch LKG sidecar. Covers: - Scope (in/out of scope incl. integration/e2e/security kept out) - Decisions d1-d13 + Q1-Q16 from refine phase carried forward - Current-state survey (Makefile, pyproject, conftest layout, dynamic-import inventory, .gitignore) - Proposed directory layout: scripts/egg_test_selector/ package with baseline/diff/graph/selector/fallback/lkg/canary/logging/ introspect modules + tests/tools/test_selector_*.py - Algorithm walkthrough + Make recipe shapes - Alternatives considered (grimp chosen over testmon / path map / hand-rolled ast) with rejection rationale - Full risk list handed to risk_analyst - Task breakdown suggestions for task_planner - 19 concrete acceptance-criteria proposals Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * plan(#1973): changeset-aware `make test` via grimp + sidecar LKG Decompose the architect's refine-phase analysis into a single-PR implementation plan across six commit-phases: foundation (grimp dev dep + targeted gitignore), core selector script (grimp graph construction, baseline resolution, fallback triggers, LKG/canary /logging/--why), Makefile wiring (test narrow-default, test-all full suite, test-record-good manual override), CI switch to make test-all, tests (9 parametrized pytest modules under tests/tools/), and documentation (docs/guides/testing.md + CONTRIBUTING.md pointer). All 13 refine-phase HITL decisions and 16 feedback answers are carried forward as locked-in constraints (d3=grimp, d12=module- level, d9=non-tracked sidecar, d1=auto-after-test-all, d5=full- suite-on-non-py, d10=scan-during-graph-construction, d2=CI on make-test-all, d7=test-narrow-default + test-all-full, Q4=canary every-10th, Q5=intersect with PYTEST_ARGS, Q6=--why flag, Q15= stderr + JSON log, Q16=branch-only keying). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * plan(risk_analyst): risk assessment for issue 1973 (make test changeset-aware) Assesses 15 technical risks for the grimp-based changeset-aware test selector with sidecar LKG storage. Flags high-severity correctness risks around: - gateway/tests/conftest.py importlib-based loading (static-graph blind spot) - cross-root grimp invocation (AC-2 hinges on enumerating all roots) - PYTEST_ARGS parsing ambiguity (recommend EGG_TEST_SELECT=off env-var opt-out) - backward compatibility (audit `make test` call sites before merge) Overall risk: MEDIUM. Recommendation: PROCEED_WITH_MITIGATIONS. CI full-suite (decision-2) plus canary (Q4) caps worst-case blast radius. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * plan(#1973): address reviewer_plan NACK — 3 blocking + 10 non-blocking Blocking: 1. Grimp PACKAGES now includes the four test roots (tests, gateway.tests, orchestrator.tests, shared.tests) so the test-file mapping step can return non-empty. Create empty shared/tests/__init__.py (no-op for pytest collection but required for grimp package registration). TASK-5-4 strengthened to assert every test_*.py file is a node in the graph — staleness guard. 2. Fail-open exit contract. Wrap main() in try/except BaseException; on unhandled exception emit full test-root list on stdout + trace on stderr + exit 0. Only --record-good validation failure may exit non-zero. TASK-5-2 adds a synthetic grimp-failure regression test that pins this contract. 3. EGG_AGENT_ROLE read-only handling (Q13). Baseline resolution now checks the env var first: reviewer_* / refiner skip the sidecar entirely (never read, never write). Default (unset / coder / tester) uses the LKG-preferred path. TASK-5-3 parametrizes over reviewer_plan, refiner, coder, unset. Non-blocking adoptions: - schema_version: 1 added to selection JSON (TASK-2-4b + TASK-6-1). - grimp pin tightened to >=3.14,<4.0 for Rust backend (TASK-1-1). - --record-good sha validation: 40-char hex + cat-file -e + ancestor (TASK-2-4a); distinct non-zero exits per validation kind. - as_package mixed strategy: True for __init__.py edits, False for leaf edits (algorithm §6 + TASK-2-1 + TASK-5-1 case). - Q2 shared/tests coarse-rule rationale recorded. - TASK-2-4 split into 2-4a (LKG + canary) and 2-4b (logging + --why) for reviewability. - Stacked -m "not functional" + user -m composition test (TASK-5-4). - Detached-HEAD stderr notice + test coverage (§8, TASK-2-2, TASK-5-3). - New TASK-5-5: subprocess-level end-to-end test against a synthetic mini-monorepo that exercises make test / make test-all / fallback through the real Makefile — closes the gap between unit tests and manual verification. - Task-dependency graph refined to reflect TASK-4-1 depending specifically on TASK-3-2 (not all of Phase 3). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * plan(#1973): address reviewer_plan v2 NACK — R1 + R2 + R6/R7/R10/R14/R5 Blocking fixes from risk_analyst cross-check: 1. Risk_analyst R1 (gateway/tests importlib test-loader). gateway/tests/conftest.py's _load_module_with_replaced_imports hides test→production edges from grimp — a change to gateway/policy.py would have selected zero tests. Added hard-coded fallback trigger in TASK-2-3: any `gateway/*.py` production edit (not under gateway/tests/) widens to full suite with explicit trigger "gateway source change (importlib test-loader)". Test case in TASK-5-2 covers positive and negative (gateway/tests/* edits do NOT fire). Blind-spot documented in TASK-6-1. 2. Risk_analyst R2 (source-file staleness beyond tests). Added runtime source-file integrity check to TASK-2-3: walk every non-test .py under gateway/shared/orchestrator/sandbox and confirm each is a grimp graph node; widen to full suite with trigger "source file missing from graph: <path>" on any missing file. This catches PACKAGES drift in production, not just the TASK-5-4 CI test. Tested in TASK-5-2. Non-blocking adoptions (also from risk_analyst): - R5 ambiguous PYTEST_ARGS classifier: TASK-5-4 gets a golden-case matrix pinning bypass/intersect/ambiguous classification for flag values like `--hypothesis-seed=gateway/tests/helper.py`. - R6 grimp cache: TASK-2-1 configures `cache_dir=.egg-state/grimp-cache/` for warm-graph reuse; TASK-1-2 gitignores the directory. - R7 selection/LKG accumulation: explicit decision to keep architect's no-pruning stance; `rm -rf` documented as recovery in manual_steps and TASK-6-1 housekeeping section. - R10 backward-compat audit: inlined into Risk summary (only three call sites; all handled: workflows → TASK-4-1, CONTRIBUTING → TASK-6-2, help → TASK-3-2). - R14 `.egg-readonly` marker: TASK-2-2 detects EITHER EGG_AGENT_ROLE=reviewer_*/refiner OR .egg-readonly marker file in repo root. TASK-5-3 parametrizes both signals. - TASK-5-5 runtime relaxed to <60s + @pytest.mark.slow tag so it deselects on inner-loop runs. Risk summary section now references .egg-state/agent-outputs/1973-risk_analyst-output.json and enumerates R1-R15 mitigations explicitly. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * plan(#1973): adopt reviewer_plan v3 ACK polish items Cheap quality improvements from reviewer_plan's v3 ACK (all marked non-blocking / forward-looking): - "What does NOT change" list now names gateway/tests/conftest.py so reviewers reading the PR diff don't wonder about conftest changes. - Task Dependencies section confirms R1 + R2 mitigations landed inside existing tasks without changing the graph. - TASK-2-1 argparse enumeration now lists `--patch-selection-json --head <sha> --pytest-ms <int>` for discoverability (implementation still in TASK-2-4b; Makefile wrapper invokes it in TASK-3-1). - TASK-5-2 promotes the fail-open verification-by-removal comment from a suggestion to a required AC line — reviewer can gate on it at PR time. - TASK-6-1 AC now explicitly requires Section 7 (Known Limits) to name the gateway importlib test-loader blind spot and the `gateway/*.py → full suite` mitigation with a pointer to `_load_module_with_replaced_imports`. R5 env-var opt-out (EGG_TEST_SELECT=off) deferred as forward-looking / follow-up-issue material per reviewer guidance. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Persist statefiles after plan phase * Persist HITL resolution after plan phase gate * implement(#1973): foundation — grimp dev dep + sidecar gitignore TASK-1-1 + TASK-1-2 of the plan. Add `grimp>=3.14,<4.0` to the `[project.optional-dependencies].dev` section in pyproject.toml. v3.14 introduced the Rust-backed graph builder that is meaningfully faster on this monorepo's ~770 source files; the inline TOML comment locks the floor against future maintainers loosening it without re-benchmarking. uv.lock is intentionally not regenerated in this commit — the agent sandbox lacks `uv` and outbound PyPI access so `uv lock` cannot run locally. CI's `uv sync --extra dev` step (no `--frozen`/`--locked`) will reconcile the lockfile when the workflow runs against this branch. The reviewer should expect a follow-up commit (or the CI job's lockfile delta) to land the resolved grimp + transitive package entries; the static analysis we ship in TASK-2-x is fail-open so the absence of grimp at runtime degrades to "full suite" rather than a hard error (matches the documented fail-open exit contract). Append three targeted entries to .gitignore under a "changeset-aware test selection" section header: .egg-state/last-known-good/ per-branch sidecar LKG sha files .egg-state/selection/ per-invocation JSON decision logs .egg-state/grimp-cache/ grimp's on-disk graph cache `.egg-state/` itself stays tracked — drafts, contracts, reviews, brc-history, oversight, agent-outputs all live there and remain under version control. Only these three subdirectories flip to ignored. * docs(#1973): add testing guide for changeset-aware make test Adds the canonical testing guide that documents the changeset-aware make test model planned in #1973: grimp-backed reverse import-graph selection, sidecar LKG semantics, the full fallback-trigger list (including the gateway/*.py importlib blind-spot mitigation), --why introspection, the JSON selection-log schema (schema_version=1), role-aware read-only behavior, the fail-open exit contract, no-pruning housekeeping, and a troubleshooting section. - New: docs/guides/testing.md (10 sections per TASK-6-1) - CONTRIBUTING.md: one-line pointer to the new guide (TASK-6-2) - docs/index.md: index entry under the Guides table - README.md: distinguish make test (narrow default) from make test-all (full suite), with a pointer to the testing guide All ten sections required by TASK-6-1 are present: overview, how selection works, sidecar LKG, fallback triggers, introspection, role-aware behavior + fail-open, known limits (gateway importlib test-loader called out as a specific blind spot pointing to gateway/tests/conftest.py's _load_module_with_replaced_imports), CI, housekeeping, troubleshooting. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * implement(#1973): scripts/select_tests.py — changeset-aware test selector TASK-2-1, TASK-2-2, TASK-2-3, TASK-2-4a, TASK-2-4b of the plan. Adds scripts/select_tests.py — a single-file standalone CLI that narrows `make test` to the transitive reverse-import closure of files touched since a Last-Known-Good (LKG) commit, with a fail-open contract that ALWAYS degrades to the full suite on any analysis failure rather than blocking iteration. Highlights from the plan (sections "Approach" / "Architecture" of .egg-state/drafts/1973-plan.md): * PACKAGES — single source of truth covering all 15 source packages plus the four test roots (tests, gateway.tests, orchestrator.tests, shared.tests). TASK-5-4 will lock this down with an exhaustive "every test_*.py is a node" check. * shared/tests/__init__.py — empty file added so grimp can register the package; pytest already treats shared/tests/ as a testpath, so the empty init is a no-op for collection. * Baseline resolution (TASK-2-2) — three precedence layers with the read-only override (Q13/R14) on top: EGG_AGENT_ROLE starting with `reviewer_` / equal to `refiner` OR the .egg-readonly marker file SKIPS the LKG sidecar entirely; else the per-branch sidecar (validated as 40-hex AND ancestor of HEAD); else `git merge-base HEAD origin/$BASE_BRANCH`; else UNRESOLVABLE → full-suite trigger. * changed_files — union of `git diff --name-only <base>...HEAD` AND `git status --porcelain` (uncommitted always participates so a dirty tree cannot have a clean LKG effect). Detached HEAD emits the documented stderr notice and falls through to base- branch. * Fallback triggers (TASK-2-3) — explicit, priority-ordered: canary → unresolvable baseline → LKG-not-ancestor → empty diff → conftest → shared/tests → Makefile / pyproject.toml / uv.lock / .python-version / workflow → gateway/*.py (R1 importlib test-loader blind spot) → non-.py change → source- file staleness (R2) → unresolvable module → dynamic-import reachability. Each trigger is a distinct stderr string so operators read the most-informative reason rather than a generic catch-all. * Mixed `as_package` strategy in reverse_closure() — `__init__.py` edits widen to the whole package; leaf-module edits narrow to just that module's downstream. * LKG sidecar I/O (TASK-2-4a) — atomic tempfile + os.replace at `.egg-state/last-known-good/<branch>.sha`; `--record-good` validates 40-hex regex AND `git cat-file -e` AND `git merge-base --is-ancestor` (refuses non-zero on any failure); detached HEAD / read-only role / missing branch skip-with-notice (exit 0). Per-branch canary counter at `<branch>.canary` fires on every 10th narrow invocation, resets after fire AND on `--full-suite`. * Structured logging (TASK-2-4b) — stderr one-liner + JSON record at `.egg-state/selection/<head>.json` carrying schema_version=1 plus baseline, branch, mode, trigger, selected_count/total_count, compute_ms (pytest_ms patched in later by the Makefile wrapper), timestamp, canary_fired, changed_files / changed_modules / dynamic_import_seeds_hit. * `--why <test>` introspection — uses grimp.find_shortest_chain to print the import path from any changed module to the named test; falls back gracefully when the test isn't selected or no path exists. * `--patch-selection-json --head <sha> --pytest-ms <int>` — write-side helper for the Makefile `test` wrapper to append `pytest_ms` to the existing JSON record after pytest returns. * Fail-open wrapper — main() catches BaseException, prints the traceback to stderr, emits the full test-root list on stdout, and exits 0. A selector bug must NEVER block iteration — correctness is preserved by widening to the full suite. Only `--record-good`'s explicit RecordGoodValidationError path is allowed to exit non-zero (because silent success on bad input would poison LKG). * pyproject.toml — adds a mypy override for the `grimp` import so mypy --strict on scripts/select_tests.py passes; grimp ships no type stubs. Branch-name caveat: `git symbolic-ref` is blocked by the egg gateway sidecar's git allowlist, so `_git_current_branch` uses `git rev-parse --abbrev-ref HEAD` and canonicalises the literal "HEAD" string back to None for detached HEAD. Same observable behaviour, different command. Lint: `ruff check` + `ruff format` + `mypy --strict scripts/select_tests.py` all clean. Tests live in TASK-5-* and land in a follow-up commit by the tester role. Auto-Filtered: true * implement(#1973): Makefile narrow-default + CI full-suite switch TASK-3-1, TASK-3-2, TASK-4-1 of the plan. Makefile: * `make test` is rewritten as the changeset-aware narrow default. The recipe captures `select_tests.py` stdout into a tempfile, runs pytest with the selected paths plus the existing `-v -m "not functional" $(PYTEST_ARGS)` flags, and surfaces pytest's exit code untouched. Empty selection prints a clear "no tests selected" message and exits 0 without calling pytest. Selector failure (non-zero exit, which only happens on argparse syntax errors thanks to the fail-open contract) falls back to the full test-root list. After pytest returns, the recipe times the wall-clock pytest duration and invokes `select_tests.py --patch-selection-json --head <sha> --pytest-ms <int>` to append `pytest_ms` to the existing `.egg-state/selection/<head>.json` record. LKG sidecar is NEVER updated by `make test` (Q12). * `make test-all` is the full-suite escape hatch. Runs the historical `pytest tests/ gateway/tests/ orchestrator/tests/ shared/tests/ -v -m "not functional"` command, then on green exit calls `select_tests.py --record-good` to atomically write the LKG sidecar. On red exit, the sidecar is NOT updated (partial / failing runs cannot become a future LKG baseline) and the failure exit code surfaces cleanly. * `make test-record-good` is the manual override — unconditionally writes the LKG sidecar (with full validation on the sha: 40-hex regex + cat-file existence + ancestor of HEAD). * `make help` lists `test`, `test-all`, `test-record-good` under the CI-checks section with one-line descriptions; the .PHONY directive picks up the two new targets. CI (.github/workflows/test.yml): * The unit job's "Run unit tests" step switches from `make test PYTEST_ARGS=...` to `make test-all PYTEST_ARGS=...` so the 80% coverage gate stays enforced unchanged. Narrowing in CI would compute coverage over only the selected tests and silently drop aggregate coverage below the threshold — decision-d2 explicitly forbids that. Coverage args (`--cov=gateway --cov=shared --cov=sandbox --cov-report= term-missing --cov-fail-under=80`) are preserved verbatim. No fetch-depth change (full-suite path doesn't need the base-branch ref); no matrix change; security and aggregate jobs are byte-identical. Auto-Filtered: true * implement(#1973): address reviewer_contract polish — reverse_closure aligned tuples Two non-blocking suggestions from reviewer_contract on the v1 proposal that are cheap to fix in-line: 1. reverse_closure() now takes a single iterable of aligned (module, path) tuples instead of two parallel lists zipped internally with strict=False. This eliminates the theoretical possibility of __init__.py detection misfiring if a future caller ever passes mismatched lengths. Both call sites (`explain_why`, `_run_narrow_or_fallback`) build the pairs in a single loop now, with the leftover `changed_modules_list` derived from the tuple set so JSON logging is unchanged. 2. The gateway/*.py importlib trigger gains an inline doc comment naming the "flat layout" assumption explicit: gateway/ production source is currently FLAT (every .py file directly under gateway/<file>.py). If that ever changes (gateway/api/foo.py etc.), the `"/" not in raw_path[len("gateway/"):]` guard would NOT widen on subdirectory edits — extend the guard at that point. Lint clean: ruff + ruff format + mypy --strict still pass. * implement(#1973): address reviewer_code NACK — sys.path + PYTEST_ARGS + __import__ + JSON reviewer_code NACK on v2 (commit ff48695b3) flagged four blockers plus a handful of non-blocking polish items. Addressed below. BLOCKING #1 — orchestrator/sandbox bare-name imports: build_graph()'s sys.path tweak previously only added `root` and `root/shared`. This left grimp's resolver unable to follow the bare-name `from models import ...`, `from egg_lib.config import ...`, `from egg_agent_tools import ...` patterns that orchestrator/, sandbox/, sandbox/tools/, and tests/ rely on at runtime — every such import was filtered as external by `include_external_packages=False`, leaving the graph empty of test→production edges for those source roots. Fix: mirror the per-conftest sys.path injections. Now adding root, root/shared, root/orchestrator, root/sandbox, root/sandbox/tools, and root/config — exactly what tests/conftest.py:13-16 and orchestrator/tests/conftest.py:25-29 inject. Inline doc comment at the call site lists each entry's source-of-truth conftest. Belt-and-braces: a "no downstream tests for changed module" fallback trigger fires when narrowing IS possible (graph built, no other trigger fired) but the closure for any non-test changed module returns zero downstream tests. This catches any remaining bare-name resolution gap (e.g., grimp-version-specific resolver quirks) and widens to full suite with the explicit trigger string `no downstream tests for changed module: <id>` rather than silently selecting zero tests. BLOCKING #2 — PYTEST_ARGS bypass was dead code: pytest_args_have_explicit_path() existed but was never called. docs/guides/testing.md documented `mode: "bypass"` that the selector could never emit. Plan §7 explicitly required this. Fix: - Selector reads PYTEST_ARGS_RAW env var (shlex-split, fail-open on parse error) and runs the path-vs-flag classifier BEFORE the fallback evaluator. On match: emits nothing on stdout, writes a `mode="bypass"` selection record with trigger "PYTEST_ARGS explicit path". - Makefile `test` recipe sets PYTEST_ARGS_RAW="$(PYTEST_ARGS)" when invoking the selector, then checks the JSON record for `"mode": "bypass"` to decide between `pytest <selected> -v -m "not functional" $(PYTEST_ARGS)` (narrow / full-suite) and `pytest -v -m "not functional" $(PYTEST_ARGS)` (bypass — pytest sees only the user's args). BLOCKING #3 — __import__ regex was anchored to start-of-string: `r"^\s*__import__\s*\("` with default flags only matches at start-of-STRING (not start-of-line), so it never matched real callers like `mod = __import__(name)` or `_X = __import__("re").compile(...)`. Fix: change to `r"\b__import__\s*\("` — matches the token anywhere in the file. Inline comment names the bug + the examples that now match. BLOCKING #4 — uv.lock not regenerated: Sandbox limitation; covered by PMC-2 in the v2 proposal. NON-BLOCKING addressed in this commit: * Read-only roles no longer write the canary counter (was: always wrote; now: gated on `not is_role_readonly`). Sidecar dir is per-branch and shouldn't be mutated by cross-sandbox roles. * Full-suite-fallback JSON records now include changed_modules_list + dynamic_import_seeds_hit (computed once before the trigger evaluator and reused on both branches). Telemetry consumers no longer lose the "why" detail when a fallback fires. * cannot-resolve-HEAD path now writes a best-effort JSON record (with head=000…0) so the telemetry trail is consistent across all fallback paths. * `_TEST_ROOT_PREFIXES` simplified to a single set-union (POSIX vs non-POSIX duplication factored out). * `conftest.py` match tightened to literal-or-`/conftest.py` so files like `myconftest.py` no longer false-fire. * `--record-good` (called by `make test-all` on green) now resets the canary counter, so the developer doesn't get a canary-fired full-suite re-run on the very next `make test` after they already exercised the full suite. LINT/TYPECHECK: ruff check + ruff format --check + mypy --strict on scripts/select_tests.py — clean. Smoke: PYTEST_ARGS_RAW="tests/test_python_syntax.py" python3 scripts/select_tests.py emits zero stdout + writes mode=bypass JSON record with trigger "PYTEST_ARGS explicit path". PYTEST_ARGS_RAW="-k foo" emits the normal full-suite fallback (flag value, not a positional path arg). * implement(#1973): thread repo_root through sidecar I/O (tester NACK) tester's v3 NACK blocking #2: write_sidecar_lkg / read_sidecar_lkg / write_canary_count / read_canary_count and their helpers _sidecar_path / _canary_path took no repo_root parameter, so writes always landed under os.getcwd(). The caller-side `record_good(..., repo_root=...)` accepted the parameter but silently dropped it before the sidecar write — plan §8 says LKG sidecar lives under the repo root, not the caller's CWD. A subagent invoking the script from a non- repo-root CWD would silently land the sidecar in the wrong place and never advance LKG. Fix: - new `_resolve_root(repo_root)` helper centralises the `repo_root or _git_repo_root()` fallback so every call site goes through the same default. - _sidecar_path / _canary_path / read_sidecar_lkg / write_sidecar_lkg / read_canary_count / write_canary_count all gain a `repo_root: Path | None = None` parameter and resolve all paths under the repo root. - call sites in record_good, resolve_baseline, lkg_is_stale, _run_narrow_or_fallback, and the --full-suite reset path all thread repo_root through. - inline doctring on read_sidecar_lkg names the bug + fix so future callers don't reintroduce it. LINT/TYPECHECK: ruff + mypy --strict — clean. Smoke: from /tmp, `python3 /home/egg/repos/egg/scripts/ select_tests.py --record-good --sha <head>` writes .egg-state/last-known-good/<branch>.sha under /home/egg/repos/egg, NOT under /tmp/ (gateway-blocked invocation verified separately — local-CWD test under /tmp confirmed the repo-root fallback is engaged). * test(#1973): add tests/tools/test_select_tests_*.py for changeset-aware selector TASK-5-1 through TASK-5-5 of the implement-phase plan. Twelve new files in tests/tools/ exercise scripts/select_tests.py, the changeset- aware test selector, plus a shared helper module and a conftest.py that loads the selector and patches its git invocations to bypass the sandbox gateway wrapper. Coverage by task: TASK-5-1 test_select_tests_graph.py Synthetic mini-monorepo grimp graph cases — leaf vs mid-layer change, cross-package edges, TYPE_CHECKING imports, mixed `as_package` strategy (`__init__.py` vs leaf). Skips gracefully when grimp isn't installed (the sandbox doesn't have grimp; CI does via `uv sync --extra dev`). TASK-5-2 test_select_tests_fallbacks.py Every fallback trigger from algorithm §5: canary, unresolvable baseline, LKG-not-ancestor, empty diff, conftest at any level, shared/tests/, Makefile, pyproject.toml, uv.lock, .python-version, workflow file, gateway/*.py R1 mitigation (with negative case for gateway/tests/), source-file staleness guard (R2), unresolvable module path, dynamic-import reachability via upstream. Plus the fail-open regression test (TASK-2-1's blanket try/except), including the inline AC-required note on how to verify the contract by removing the try/except. TASK-5-3 test_select_tests_lkg.py test_select_tests_baseline.py test_select_tests_canary.py Sidecar atomic-write semantics (concurrent reader sees no half-written file), `read_sidecar_lkg` validation against malformed contents, --record-good validation failures (regex / cat-file / ancestor) each with distinct exit codes, --record-good no-op paths (detached HEAD, read-only role, marker file), per-branch isolation, baseline resolution across all EGG_AGENT_ROLE values + .egg-readonly marker, the BASE_BRANCH env override, the lkg_is_stale helper, the changed_files diff helper (committed + uncommitted, renames, empty-tree), canary modulo contract (parametrized), counter increment / fire / reset semantics, --full-suite resets the counter. TASK-5-4 test_select_tests_pytest_args.py test_select_tests_why.py test_select_tests_logging.py test_select_tests_monorepo.py PYTEST_ARGS classifier — bypass class (positional test-root path), intersect class (pure flags + stacked-marker composition), ambiguous class (R5 — flag values like `--hypothesis-seed=...`), mixed (positional wins). --why introspection wired through `_main_inner` (skips without grimp). Selection-record JSON envelope — every documented key including schema_version=1, baseline {sha, source}, branch-can-be-null, ISO-8601 timestamp, pytest_ms is null initially, atomic-write replaces. patch_selection_record handles missing/malformed files with stderr notice + exit 0. Stderr decision- line format pinned to a regex for both narrow and full-suite cases. Monorepo staleness guard against the live PACKAGES constant (skips without grimp) — every test_*.py is a graph node, every source root yields nodes, gateway is marked as a dynamic-import seed. TASK-5-5 test_select_tests_e2e.py Subprocess-level invocations of the selector — default mode exits 0 on a real diff, --full-suite emits the four test-root paths and resets canary, --record-good writes the sidecar, --record-good --sha <bad> exits non-zero, --patch-selection-json appends pytest_ms, --patch-selection-json missing args is fail-open, --help lists all flags, unknown flag exits non-zero. Bypasses the sandbox gateway git wrapper by prepending a private bin dir with a symlink to /opt/.egg-internal/git on PATH for the subprocess. Test infrastructure: - `_select_tests_helpers.py`: shared module loader (SourceFileLoader pattern), real-git wrapper (gateway-bypass), git fixture builders (init_git_repo, commit_file), in-process chdir context manager. - `conftest.py`: `real_git` fixture that monkeypatches `selector._run_git` to use the real /opt/.egg-internal/git binary so synthetic tmp_path repos work despite the sandbox's git wrapper. Local verification: $ python3 -m pytest tests/tools/test_select_tests_*.py ====== 173 passed, 3 skipped, 1 warning in 2.17s ====== 3 skipped: TASK-5-1 graph + TASK-5-4 monorepo + TASK-5-4 why all pytest.importorskip on grimp (not installed in this sandbox; CI picks them up via the dev extras). Open blockers in the coder's proposal flagged via NACK and HANDOFF: 1. .github/workflows/test.yml is DELETED on origin/egg/issue-1973 instead of modified per TASK-4-1. 2. write_sidecar_lkg / write_canary_count ignore the repo_root parameter and write CWD-relative. 3. shared/tests/__init__.py breaks pytest collection with PYTHONPATH=shared (which the Makefile sets). Tests above currently work around 2 by monkeypatch.chdir; once the coder re-proposes with the fixes they can drop the workarounds and validate the spec directly. * Persist statefiles after implement phase * Remove ephemeral agent-output handoff artifacts (#1731) * Address review feedback: fix tests, restore CI, regenerate lockfile - Restore .github/workflows/test.yml from origin/main and swap make test -> make test-all (B1 / decision-d2) - Create shared/tests/__init__.py for grimp package registration (B3) - Configure pytest importlib mode + consider_namespace_packages to resolve conftest collision between tests/ and shared/tests/ - Regenerate uv.lock with grimp>=3.14 entry (B2) - Fix _build_synthetic_graph() in graph tests: add sys.path insertion and sys.modules isolation so grimp resolves synthetic packages instead of pytest-cached ones (C4 / TASK-5-1) - Fix reverse_closure() call signature to pass module_path_pairs as tuples instead of separate lists - Add EGG_AGENT_ROLE isolation to real_git fixture so sandbox- inherited role values don't cause record_good() to short-circuit (C5 / TASK-5-3) - Fix vacuous tautology assertion in test_select_tests_logging.py (C1) - Replace deprecated SourceFileLoader.load_module() with importlib.util.spec_from_file_location() (C2) * Fix ImportPathMismatchError: remove shared/tests/__init__.py With PYTHONPATH=shared (set by Makefile test targets), this file creates a second 'tests' package visible from the shared/ path entry, colliding with the top-level tests/ package. Removing it makes shared/tests/ a namespace package again, which grimp >=3.14 handles natively and pytest importlib mode discovers correctly. * Fix ImportPathMismatchError: remove gateway/tests and orchestrator/tests __init__.py With PYTHONPATH=shared:gateway:orchestrator (set by Makefile test targets), gateway/tests/__init__.py creates a second 'tests' package visible from the gateway/ path entry, colliding with the top-level tests/ package and causing ImportPathMismatchError for conftest.py. Same root cause as shared/tests/__init__.py (fixed in f0fc437). Remove both gateway/tests/ and orchestrator/tests/ __init__.py to make them namespace packages, which pytest importlib mode discovers correctly with consider_namespace_packages=true. * Fix gateway.tests collection: set __path__ on loaded gateway module After gateway/tests/__init__.py was removed (to fix ImportPathMismatchError under PYTHONPATH=shared:gateway:orchestrator), pytest collection of gateway/tests/test_*.py started failing with: AttributeError: module 'gateway' has no attribute '__path__' The gateway/tests/conftest.py registers gateway/gateway.py (a single file FastAPI app) as sys.modules['gateway'], which replaces the real gateway package. With gateway/tests/ now a namespace subpackage, pytest's importlib-mode collector (consider_namespace_packages=true) walks up through gateway to resolve gateway.tests.test_*, and needs gateway.__path__ to find subpackages. The single-file module had no __path__, causing 64 collection errors. Set gateway.__path__ = [GATEWAY_DIR] right after the module is loaded so the namespace subpackage gateway.tests resolves correctly. Tests that do 'import gateway' to access the FastAPI app still get the same module object. * Set __spec__ on loaded gateway module for find_spec compat The previous fix set gateway.__path__ so pytest could collect tests under gateway/tests/, but importlib.util.find_spec("gateway") still raised "gateway.__spec__ is None" because the manually-constructed ModuleType has no spec. This broke tests/tools/test_select_tests_monorepo.py: scripts/select_tests.py calls grimp.build_graph("gateway", ...), and grimp resolves package locations via importlib.util.find_spec, which raises ValueError when the target module's __spec__ is None. Construct a ModuleSpec with submodule_search_locations pointing at GATEWAY_DIR and assign it to gateway.__spec__ so find_spec returns a valid package spec. * Remove silently-ignored import_mode pytest setting (N1) import_mode is not registered as an INI option — pytest emitted PytestConfigWarning: 'Unknown config option: import_mode' on every invocation, and prepend mode (the default) was used regardless. The PR description and conftest comment claimed importlib mode was active but it was not. Resolution: remove the dead setting and update both comments to match runtime behavior. consider_namespace_packages=true alone is sufficient to discover shared/tests, gateway/tests, and orchestrator/tests as namespace subpackages. We deliberately stay on prepend mode because scripts/select_tests.py monorepo tests build a real grimp graph and importlib mode triggers grimp.NotATopLevelModule for gateway.tests and orchestrator.tests subpackages. Verified: PytestConfigWarning no longer fires; all 294 tools tests pass (including the 9 monorepo tests that errored when --import-mode=importlib was actually applied). --------- Co-authored-by: egg-orchestrator <egg@localhost> Co-authored-by: egg <egg@example.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> 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>

… masking failures (#3305) * fix(#3302): guard against orphan test roots + stop lint short-circuit Two toolchain-hygiene gaps from #3298 (class 2): a new test root could be silently uncollected by CI and left untyped, and `make lint-python` short-circuited so one lint failure hid another. Guard: add scripts/check-test-roots.py (runs in `make lint-custom`, i.e. the CI lint job). It discovers every `*/tests` dir containing `test_*.py` and asserts each is wired into all four test-root lists — pyproject testpaths, the `make test-all` roots, the `make test` full-suite fallback, and `TEST_ROOT_DIRS` — plus, for roots under a mypy package root, the `mypy --exclude` list. A new test root now fails CI until every list is updated atomically. Wiring: the guard surfaced four pre-existing orphan roots that CI never ran — sandbox/tests, scripts/tests, shared/egg_anchor/tests, shared/egg_contracts/tests (added by #3200/#3077/#1991, never wired). Wired all four into the lists above. This re-collected ~27 test files whose code had rotted while invisible; fixed them: - egg_contracts: bump fake commit hashes to valid 7-char hex (the AgentExecutionModel.commit pattern now requires ^[a-f0-9]{7,40}$). - sandbox/test_overseer_alert_cli: patch the handler's orchestrator_request (cmd_overseer_alert delegates to progress_overseer_alert now); the posted payload is unchanged so assertions stand. - sandbox/test_brc_cli_args: --reason moved from argparse-required to handler-layer enforcement (#2741/#2908); assert at the command layer. Lint: rewrite `make lint-python` to run ruff check, ruff format --check, and mypy independently and aggregate failures instead of aborting on the first, so one failure no longer masks another. * Fix checks: align newly-wired test roots with CI environment The #3302 orphan-test-root guard wired previously-orphaned roots (sandbox/tests, scripts/tests, shared/egg_anchor/tests, shared/egg_contracts/tests) into pytest testpaths, so make test-all now collects pre-existing tests there. Three surfaced failures under CI: - test_ci_config: expand the expected testpaths set to the full list now in pyproject.toml so the pinned set matches the wired roots. - test_brc_slice_routing: propose tests omitted commit_sha, so brc_propose fell back to 'git rev-parse HEAD' in EGG_REPO_PATH, absent on the runner. Supply an explicit commit_sha; these tests cover slice_id routing, not HEAD resolution. - test_build_host_repo_map: the CLI test ran the script via its 'env python3' shebang, which can resolve to an interpreter without PyYAML. Invoke it through sys.executable instead. * Fix checks: align retry test with post-#2270 overseer spawn path The #2270 §1.5 refactor replaced the bespoke spawn_overseer_container method with the generic _spawn_overseer_agent -> spawner.spawn_agent_job path, but TestWorktreeCreationRetry.test_retry_succeeds_on_second_attempt still asserted the removed spawn_overseer_container was called, so the Unit Tests job failed. Assert on spawn_agent_job instead. * Harden fallback-root regex anchor + document mypy exclude assumption Address review suggestions on #3302 guard: - Anchor parse_fallback_roots on the >"$selected_file" redirect so it cannot match the unrelated printf '%s\n' "$cur_id" marker write if Makefile recipes are reordered. - Document that mypy --exclude values are regexes but compared literally (every current entry is a plain literal path). --------- Co-authored-by: james-in-a-box[bot] <246424927+james-in-a-box[bot]@users.noreply.github.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>

egg-reviewer Bot approved these changes Apr 24, 2026

View reviewed changes