Skip to content

Fix #2196: anchor overseer mount path + soften prompt framing - #2284

Merged
jwbron merged 5 commits into
mainfrom
egg/issue-2196
Apr 29, 2026
Merged

Fix #2196: anchor overseer mount path + soften prompt framing#2284
jwbron merged 5 commits into
mainfrom
egg/issue-2196

Conversation

@jwbron

@jwbron jwbron commented Apr 29, 2026

Copy link
Copy Markdown
Owner

Summary

  • Anchors /opt/egg-runtime/sandbox/overseer_monitor.py to its in-repo source (sandbox/overseer_monitor.py) in both sandbox/agent-config/rules/overseer.md and the spawner-injected prompt in orchestrator/kubernetes_spawner.py, so the overseer can verify provenance instead of classifying its own runbook as a prompt injection.
  • Rephrases the prompt's CRITICAL / DO NOT write your own loop imperative stack as plain guidance — same constraints, no urgency-framing trigger.
  • Updates test_overseer_prompt_mentions_prebuilt_monitor to assert the new wording and the provenance anchor.
  • Bumps .pre-commit-config.yaml ruff-pre-commit rev from v0.15.0 → v0.14.14: surfaced during this fix that v0.15.0's hook invocation corrupts except (AttributeError, TypeError): into invalid Py2 syntax except AttributeError, TypeError: for orchestrator/kubernetes_spawner.py:913 (standalone v0.15.0 doesn't reproduce — only the pre-commit invocation does). 0.14.14 matches the local venv pin and produces clean runs.

The original bug: sandbox/agent-config/rules/overseer.md:30-37 told the overseer to python3 /opt/egg-runtime/sandbox/overseer_monitor.py --once under a CRITICAL: ... DO NOT write your own loop heading. The Sonnet-backed overseer grepped its working directory, only found sandbox/overseer_monitor.py (the in-repo source, not the runtime mount), classified the instruction as a prompt injection, filed unauthorized-overseer-action [high], and exited after one decision turn. Respawns hit the same wall up to the cap (3) and the overseer was offline for the rest of the pipeline.

The respawn-3 transcript on issue #2196 showed the same agent could observe and classify correctly via MCP tools alone — so it's the prompt that's tripping injection-detection heuristics, not the agent's competence. (1)+(3) from the issue's suggested-fix list are the smallest change that addresses both halves.

Test plan

  • .venv/bin/pytest orchestrator/tests/test_overseer_spawn.py — 38 passed (incl. the updated prompt test that now asserts the provenance anchor).
  • make lint — all checks passed.
  • pre-commit run ruff-format --files orchestrator/kubernetes_spawner.py — passes with the new ruff pin (was corrupting line 913 with the prior pin).
  • Live pipeline run with overseer_enabled: true: verify the overseer survives past turn 2 and emits real cycle JSON. Tracked in the verification follow-up issue (link below) since it requires a deployed pipeline and isn't covered by unit tests.

Notes

Closes #2196.

The overseer prompt told the agent to run
`python3 /opt/egg-runtime/sandbox/overseer_monitor.py --once`, then
combined that with "CRITICAL" / "DO NOT write your own loop" framing.
The Sonnet-backed overseer grepped its working directory, only found
`sandbox/overseer_monitor.py` (the in-repo source, not the runtime
mount), classified the instruction as a prompt injection, filed
`unauthorized-overseer-action [high]`, and exited. Respawns hit the
same wall, taking the overseer offline for the rest of the pipeline.

Anchor `/opt/egg-runtime/sandbox/overseer_monitor.py` to its in-repo
source (`sandbox/overseer_monitor.py`) in both the rules file and the
spawner-injected prompt, and rephrase the "CRITICAL"/"DO NOT" stack as
plain guidance so the prompt no longer trips injection-detection
heuristics. Update the spawn-prompt test to assert the new wording and
the provenance anchor.

Also pin pre-commit's ruff-pre-commit rev to v0.14.14: surfaced
during this fix that the v0.15.0 hook invocation corrupts
`except (X, Y):` into invalid Py2 syntax `except X, Y:` for
orchestrator/kubernetes_spawner.py:913 (standalone v0.15.0 doesn't
reproduce — only the pre-commit invocation does). 0.14.14 matches
the local venv pin.
@james-in-a-box

This comment has been minimized.

@james-in-a-box

This comment has been minimized.

@egg-reviewer egg-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No agent-mode design concerns. The changes here actually move the prompt toward the guidelines: softening the "CRITICAL / DO NOT write your own loop" imperative stack to plain guidance is the right direction (less rigid procedural framing), and anchoring /opt/egg-runtime/sandbox/overseer_monitor.py to sandbox/overseer_monitor.py plus the suggested diff confirmation is informing the agent (provenance context it can verify), not pre-fetching or constraining its exploration. Telling the overseer to run the pre-built script remains an appropriate "how" since the monitor protocol (polling cadence, heartbeats, JSON line shape) is a shared-infrastructure contract on the orchestrator side, not a procedure the agent should reinvent.

— Authored by egg

@james-in-a-box

This comment has been minimized.

@james-in-a-box

This comment has been minimized.

@egg-reviewer egg-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The fix direction is right — softening the CRITICAL / DO NOT urgency stack and giving the overseer a provenance anchor for /opt/egg-runtime/sandbox/overseer_monitor.py is exactly what the issue-#2196 transcript pointed at. CI is green, the unit test covers the new wording, and make lint passes. Approving, with a few non-blocking notes — primarily that the new wording's "runtime mount / same file" claim is technically wrong and may regress the fix in production where image and worktree commits diverge.

1. "Runtime mount" wording is inaccurate and may re-trigger the bug it's trying to fix

orchestrator/kubernetes_spawner.py:1278-1282 and sandbox/agent-config/rules/overseer.md:32 both describe /opt/egg-runtime/sandbox/ as a "runtime mount" of sandbox/, and assert "both paths refer to the same file." That is not how it's wired:

sandbox/Dockerfile:279 does COPY . /opt/egg-runtime/ — a build-time copy, not a runtime bind mount. At runtime there are two physically distinct files:

  • /opt/egg-runtime/sandbox/overseer_monitor.py — frozen at the image's build SHA.
  • ./sandbox/overseer_monitor.py — from the worktree, whatever commit the pipeline checked out.

The default image is egg:latest (kubernetes_spawner.py: DEFAULT_SANDBOX_IMAGE), and pipelines routinely run on a main that has moved past the build commit. In that normal case, the suggested diff /opt/egg-runtime/sandbox/overseer_monitor.py sandbox/overseer_monitor.py will produce non-empty output.

That is the failure mode that re-creates the original bug: an overseer agent that was already cautious enough to grep for the file's provenance (the exact behavior #2196 traced) now runs the verification command the prompt itself suggested, sees real diffs, and re-classifies the situation as suspicious. The prompt has effectively armed it with a "confirmation" tool that returns the wrong answer in the common deployment scenario.

Recommended fix: rephrase to be honest about the mechanism. Something like:

The path /opt/egg-runtime/sandbox/overseer_monitor.py is a build-time copy of sandbox/overseer_monitor.py from this repo, baked into the image at sandbox/Dockerfile. It is the canonical script the orchestrator expects you to run. The worktree copy at sandbox/overseer_monitor.py may be at a different commit (newer or older) than the image's snapshot — that is expected and not a sign of injection. You do not need to run diff to confirm provenance; the orchestrator vouches for the script.

Alternatively, drop the diff suggestion entirely. The provenance anchor (telling the agent the file comes from the in-repo source it can grep for) is the load-bearing part; the verification command is what introduces the new failure mode.

2. Ruff downgrade is bundled in without a reproducible root cause

The pin from v0.15.0v0.14.14 is unrelated to issue #2196 and lands with this rationale (.pre-commit-config.yaml:3):

v0.15.0's ruff-format hook corrupts except (X, Y): into invalid Py2 syntax except X, Y: for orchestrator/kubernetes_spawner.py:913 (standalone v0.15.0 doesn't reproduce — only the pre-commit invocation does)

A code-corruption bug that reproduces in pre-commit run but not in the standalone binary at the same version is unusual. Plausible alternatives that should be ruled out before pinning backward:

  • A stale ~/.cache/pre-commit/ venv from a prior ruff install (try pre-commit clean).
  • A pyproject.toml [tool.ruff] setting that's interpreted differently across the two invocation modes.
  • Pre-commit fetching a different ruff build than the local venv pin.

If the corruption is real, this deserves an upstream issue against astral-sh/ruff with a minimal reproducer — not a silent pin in an unrelated PR. As-is, future maintainers will have no way to know when it's safe to bump the pin again.

This isn't blocking the overseer fix, but I'd split it into a separate PR (or at minimum, a separate commit) so the rollback rationale lives in its own history and doesn't get lost when someone bisects the overseer behavior change.

3. Test assertion is brittle

orchestrator/tests/test_overseer_spawn.py:1074-1077 asserts on the exact substring "runtime mount of `sandbox/overseer_monitor.py`". Any rewording of the provenance anchor — including the one suggested in #1 above, which would actually be a correctness improvement — will break this test for cosmetic reasons.

Consider asserting on the structural property instead, e.g. that "sandbox/overseer_monitor.py" is mentioned alongside /opt/egg-runtime/sandbox/overseer_monitor.py (the anchor relationship), without pinning the connector phrase.

4. Live verification is deferred

The "Live pipeline run with overseer_enabled: true" checkbox in the test plan is unchecked, deferred to a follow-up issue. That is the only test that would actually catch the failure mode described in #1 above (the diff regression). The unit test only asserts the prompt string is correct; it cannot tell you whether the prompt works. Worth flagging in the merge-time obligations so the calibration restart from #1962 (mentioned in the PR notes) is paired with an actual end-to-end check.

— Authored by egg

@james-in-a-box

This comment has been minimized.

@egg-reviewer egg-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-review: no new agent-mode design concerns.

The delta since my last review at 9141f08 is just a merge of main to absorb the ruff 0.15.12 / PEP 758 sweep from #2297. The single semantic-looking diff to a PR-touched file (kubernetes_spawner.py: except (AttributeError, TypeError):except AttributeError, TypeError:) is a mechanical formatter change for Python 3.14, not a real code change. The three PR-content files (anchor mount-path fix in kubernetes_spawner.py + test_overseer_spawn.py, prompt softening in overseer.md) are otherwise unchanged from my prior approval.

Standing by the original verdict — the prompt softening still moves toward the guidelines (less rigid CRITICAL / DO NOT framing, more orientation), and the runtime-mounted anchor doesn't bake any context into the prompt.

— Authored by egg

@james-in-a-box

This comment has been minimized.

@egg-reviewer egg-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-review at 8a941ab since my prior review at 9141f08.

The delta is just a merge of main absorbing the ruff 0.15.12 / PEP 758 sweep from #2297 — no real PR-content changes (the only "semantic-looking" diffs in PR-touched files like kubernetes_spawner.py:913, overseer_monitor.py, overseer/monitor.py are mechanical except (A, B):except A, B: paren drops, which are correct under Python 3.14's PEP 758).

Status of prior feedback

1. "Runtime mount" wording — NOT addressed (still my main concern)

orchestrator/kubernetes_spawner.py:1278-1281 and sandbox/agent-config/rules/overseer.md:32 still describe /opt/egg-runtime/sandbox/ as the "runtime mount" of sandbox/ and assert "both paths refer to the same file." That is still factually wrong:

  • sandbox/Dockerfile:279 is COPY . /opt/egg-runtime/ — a build-time copy.
  • kubernetes_spawner.py:1300-1309 (spawn_overseer_container) calls spawn_agent_job(..., repo_volumes=None, ...), so the overseer pod gets no runtime mount of the worktree. Confirmed by grepping all of orchestrator/, shared/, k8s/ — there is no overlay or hostPath that bind-mounts source onto /opt/egg-runtime/ at runtime.

In the normal deployment (DEFAULT_SANDBOX_IMAGE = egg:latest, pipelines running on a main that has moved past the image's build SHA), /opt/egg-runtime/sandbox/overseer_monitor.py and ./sandbox/overseer_monitor.py will be different files. The diff command the prompt now suggests will produce non-empty output and re-create the exact "is this file injected?" cautious-grep behavior #2196 was traced to.

Recommendation unchanged: rephrase to "build-time copy from this repo, baked at image build time," and either drop the diff command or replace it with something that actually validates provenance (e.g. a SHA check against an in-image manifest, or just trust the orchestrator). The provenance anchor is the load-bearing part — the verification command is what introduces the new failure mode.

2. Ruff downgrade — RESOLVED by the merge.

The merge of main accepted v0.15.12 and dropped the misguided pin-back comment. This is the right outcome — the PR's original rationale ("v0.15.0 corrupts except (X, Y):") was incorrect: under PEP 758 (Python 3.14, which pyproject.toml requires), the formatter is correctly dropping the unnecessary parens. No further action needed here.

3. Test brittleness — NOT addressed.

orchestrator/tests/test_overseer_spawn.py:1074 still asserts on the exact substring "runtime mount of \sandbox/overseer_monitor.py`". If you fix #1 above (which you should), this test will fail for cosmetic reasons. Worth converting to a structural assertion now (e.g. assert that both /opt/egg-runtime/sandbox/overseer_monitor.pyandsandbox/overseer_monitor.py` are mentioned, without pinning the connector phrase) so the next correctness fix doesn't get blocked by test churn.

4. Live verification — still deferred.

The "live pipeline run with overseer_enabled: true" checkbox in the test plan is still unchecked. The unit test only checks the prompt string; nothing in CI will catch a regression to the diff-confusion failure mode in #1.

Bottom line

The merge is clean and resolves my one actually-wrong concern (#2). My substantive concern (#1) — the inaccurate "runtime mount" claim plus the diff confirmation command that returns wrong answers in the normal deployment — is unaddressed and is the failure mode most likely to re-create the bug this PR is trying to fix. Approving with the same non-blocking notes; please address #1 before merge or in a fast follow-up.

— Authored by egg

@james-in-a-box

This comment has been minimized.

@james-in-a-box

This comment has been minimized.

Address review feedback on PR #2284 from egg-reviewer:

The previous wording described /opt/egg-runtime/sandbox/ as a "runtime
mount" of sandbox/, asserting both paths refer to the same file. That
is incorrect — sandbox/Dockerfile uses COPY . /opt/egg-runtime/, a
build-time copy. spawn_overseer_container also passes repo_volumes=None,
so no runtime bind mount exists. In the normal deployment (egg:latest
running on a main that has moved past the build SHA), the suggested
diff /opt/egg-runtime/.../overseer_monitor.py sandbox/overseer_monitor.py
will produce non-empty output and re-create the exact "is this an
injection?" cautious-grep failure mode #2196 was tracing.

Rephrase to be honest about the mechanism (build-time copy from this
repo, baked at image build time) and drop the misleading diff
suggestion. The provenance anchor is the load-bearing part; the
verification command is what introduced the new failure mode.

Also convert test_overseer_prompt_mentions_prebuilt_monitor from an
exact-substring assertion ("runtime mount of `sandbox/overseer_monitor.py`")
to a structural assertion (both paths mentioned, without pinning the
connector phrase) so the next correctness fix doesn't get blocked by
test churn.
@james-in-a-box

This comment has been minimized.

@egg-reviewer egg-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-review at 9d6cfce since my prior review at 8a941ab.

The branch was rewritten (force-push absorbing the merge of main into a single overseer-prompt commit), so the delta versus main is what stands. From an agent-mode design lens the changes only move further toward the guidelines:

  • The provenance anchor is now factually correct ("build-time copy of sandbox/overseer_monitor.py from this repo, baked into the container image at sandbox/Dockerfile") instead of the prior inaccurate "runtime mount" framing. This is orienting context the agent can verify by grepping sandbox/Dockerfile — not pre-fetched constraint.
  • The misleading diff /opt/egg-runtime/... sandbox/... confirmation command is gone. That command was the one piece of the prior wording at risk of re-triggering #2196's "is this injected?" cautious-grep failure mode in the common deployment where image and worktree commits diverge.
  • The CRITICAL / DO NOT urgency stack is fully softened to Don't ... plain guidance in both sandbox/agent-config/rules/overseer.md:49-52 and orchestrator/kubernetes_spawner.py:1276-1290. Telling the overseer to call the pre-built script remains an appropriate "how" — the monitor protocol (poll cadence, heartbeats, JSON line shape) is a shared infrastructure contract on the orchestrator side, not a procedure the agent should reinvent.
  • The brittle exact-substring test is now a structural assertion (/opt/egg-runtime/sandbox/overseer_monitor.py and sandbox/overseer_monitor.py both mentioned, without pinning the connector phrase) — orchestrator/tests/test_overseer_spawn.py:1073-1080. This is the right shape for a prompt assertion.

No new agent-mode design concerns. Approving.

— Authored by egg

@james-in-a-box

This comment has been minimized.

@egg-reviewer egg-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-review at 9d6cfce since my prior review at 8a941ab.

The new commit (9d6cfce) is a direct response to my earlier review and addresses the substantive concern. The prompt text in both orchestrator/kubernetes_spawner.py:1278-1287 and sandbox/agent-config/rules/overseer.md:32 now accurately describes /opt/egg-runtime/sandbox/overseer_monitor.py as a "build-time copy of sandbox/overseer_monitor.py from this repo, baked into the container image at sandbox/Dockerfile (the COPY . /opt/egg-runtime/ layer)" — verified against sandbox/Dockerfile:279. The misleading diff confirmation command is dropped, replaced with "the orchestrator vouches for it, and you do not need to diff it against your worktree." This is the right fix and removes the failure mode that would have re-created bug #2196 in the normal egg:latest-on-newer-main deployment.

Approving with two non-blocking notes — neither blocks merge, but worth a follow-up.

Status of prior feedback

1. "Runtime mount" wording — RESOLVED. The build-time-copy framing is accurate, and dropping the diff suggestion removes the verification command that would return wrong answers in normal deployment. Exactly as suggested.

2. Ruff downgrade — RESOLVED (by the merge of main, as noted in my last review). .pre-commit-config.yaml:3 is at v0.15.12. The PR body still narrates the now-reverted downgrade in the "## Summary" bullet list (.pre-commit-config.yaml ruff-pre-commit rev from v0.15.0 → v0.14.14...) — that bullet is stale and worth striking on the next push so future bisecters don't get misled, but it's a description-only issue.

3. Test brittleness — PARTIALLY ADDRESSED. Converting test_overseer_prompt_mentions_prebuilt_monitor from one exact-substring assertion to a structural pair was the right direction, but the assertion at orchestrator/tests/test_overseer_spawn.py:1077 does not actually verify what its docstring claims:

assert "/opt/egg-runtime/sandbox/overseer_monitor.py" in prompt, (...)  # line 1074
assert "sandbox/overseer_monitor.py" in prompt, (
    "Prompt must reference the in-repo source path so the agent "
    "can verify provenance (issue #2196)"
)  # line 1077

/opt/egg-runtime/sandbox/overseer_monitor.py contains sandbox/overseer_monitor.py as a suffix substring, so line 1077 passes any time line 1074 does — it cannot independently prove the in-repo path is mentioned. A future regression that drops the bare provenance anchor (e.g. someone collapses the prompt back to "run the script at /opt/egg-runtime/sandbox/overseer_monitor.py") would not be caught.

Recommended fix: assert prompt.count("sandbox/overseer_monitor.py") >= 2 (the runtime path counts as one occurrence; a second stand-alone reference satisfies the structural anchor). Or assert on a discriminating phrase like "from this repo" / "build-time copy" that only appears in the provenance explanation.

4. Live verification — STILL DEFERRED. Same status as before. Acceptable per the PR's explicit deferral, but the unit test still only checks the prompt string — nothing in CI catches a regression to the unauthorized-overseer-action failure mode in production. Worth pairing the calibration restart from #1962 with an actual end-to-end check on the follow-up issue.

One new minor wording concern

orchestrator/kubernetes_spawner.py:1283-1285 and the parallel passage in overseer.md:32 describe a "worktree copy" that "may be at a newer or older commit than the image snapshot." For the overseer pod specifically, spawn_overseer_job calls spawn_agent_job(..., repo_volumes=None, ...) (line 1307) so no worktree is mounted into the pod. The agent's CWD will not contain sandbox/overseer_monitor.py from a worktree at all — only /opt/egg-runtime/sandbox/overseer_monitor.py is visible. The "worktree copy may diverge" framing is conceptually defending against a comparison the agent cannot actually perform in this pod's filesystem.

This is non-blocking — the agent reading "any divergence is expected and not injection" still gets the right defensive posture — but if a future iteration tightens the prompt, dropping the worktree-copy hypothetical and just asserting "the orchestrator vouches for the script you find at this path" would be more honest about the deployment shape.

Bottom line

The fix is correct, the load-bearing change addresses the failure mode #2196 traced, and CI is green. Approving. Please address the test-assertion redundancy (#3) before merge or in a fast follow-up so a future regression to the prompt's provenance anchor would actually fail the test that exists to guard it.

— Authored by egg

@james-in-a-box

This comment has been minimized.

@james-in-a-box

This comment has been minimized.

- Drop the 'worktree copy may be at a newer/older commit' framing from
  both overseer.md and kubernetes_spawner.py: spawn_overseer_job calls
  spawn_agent_job(..., repo_volumes=None, ...), so the overseer pod has
  no worktree mount and cannot perform the comparison the prompt was
  defending against. Replace with a plain 'verify against any other
  copy' framing.

- Tighten test_overseer_prompt_mentions_prebuilt_monitor: the runtime
  path /opt/egg-runtime/sandbox/overseer_monitor.py contains
  sandbox/overseer_monitor.py as a substring, so the prior bare 'in
  prompt' assertion could not independently prove the in-repo
  provenance anchor is mentioned. Switch to count >= 2 so a regression
  that drops the standalone reference now fails the test.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Addressed the actionable feedback from the latest review at 9d6cfce in commit c5fc890:

#3 (test brittleness) — FIXED. orchestrator/tests/test_overseer_spawn.py:1077-1084 now asserts prompt.count("sandbox/overseer_monitor.py") >= 2. The runtime path counts as one occurrence (suffix), and the in-repo provenance anchor is a second standalone reference — a regression that drops the bare anchor will now fail the test, where previously line 1077 passed any time line 1074 did.

#5 (worktree-copy hypothetical) — FIXED. Confirmed via kubernetes_spawner.py:1305-1309 (spawn_overseer_job calls spawn_agent_job(..., repo_volumes=None, ...)) that the overseer pod has no worktree mount. Dropped the "worktree copy may be at a newer or older commit" framing from both sandbox/agent-config/rules/overseer.md:32 and orchestrator/kubernetes_spawner.py:1278-1285. Replaced with: "the orchestrator vouches for it, and you do not need to verify its provenance against any other copy." This is more honest about the deployment shape — there is no other copy in the pod's filesystem.

#2 (stale ruff bullet in PR description) — Cannot self-edit. The PR is human-owned (author: jwbron), so the description bullet about the ruff v0.15.0 → v0.14.14 pin remains stale. Flagging here so the description can be updated on the next push by the maintainer; the actual .pre-commit-config.yaml is correct (at v0.15.12 from the main merge).

#4 (live verification) — Acknowledged, deferred. Same status as before; tracked in the verification follow-up issue per the PR notes.

.venv/bin/pytest orchestrator/tests/test_overseer_spawn.py — 38 passed. make lint — clean.

— Authored by egg

@james-in-a-box

This comment has been minimized.

@egg-reviewer egg-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-review at c5fc890 since my prior review at 9d6cfce.

The delta is exactly the two non-blocking notes from last round, both addressed cleanly:

  • Test brittleness (prior #3) — FIXED. orchestrator/tests/test_overseer_spawn.py:1077-1084 is now assert prompt.count("sandbox/overseer_monitor.py") >= 2, with a comment explaining that the runtime path contributes one suffix occurrence and the standalone provenance anchor is the required second. A future regression that collapses the prompt back to a single runtime-path mention will now actually fail this test, where previously line 1077 was satisfied any time line 1074 was.
  • Worktree-copy hypothetical (prior #5) — FIXED. orchestrator/kubernetes_spawner.py:1278-1287 and sandbox/agent-config/rules/overseer.md:32 no longer reason about a worktree copy that "may be at a newer or older commit." Replaced with "the orchestrator vouches for it, and you do not need to verify its provenance against any other copy." This matches the actual pod shape — spawn_overseer_job calls spawn_agent_job(..., repo_volumes=None, ...) at kubernetes_spawner.py:1305-1309, so there is no worktree copy visible to the overseer agent for it to compare against. The prompt is now honest about the deployment.

From an agent-mode design lens this is purely a tightening — less speculative framing in the prompt, a structural assertion that actually guards the property it claims to. No new concerns.

— Authored by egg

@james-in-a-box

This comment has been minimized.

@egg-reviewer egg-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-review at c5fc890 since my prior review at 9d6cfce.

The new commit is a direct response to my last review and correctly addresses both actionable items. Approving.

Status of prior feedback

#3 (test brittleness) — RESOLVED. orchestrator/tests/test_overseer_spawn.py:1080-1084 now asserts prompt.count("sandbox/overseer_monitor.py") >= 2. Traced through the prompt at kubernetes_spawner.py:1274-1294:

  1. The runtime path /opt/egg-runtime/sandbox/overseer_monitor.py contributes one occurrence (suffix substring).
  2. The standalone "build-time copy of sandbox/overseer_monitor.py from this repo" reference contributes the second.

Count is exactly 2 today, so a regression that drops the bare in-repo anchor (e.g. someone collapses the prompt back to "run the script at /opt/egg-runtime/sandbox/overseer_monitor.py") will fail the test — which is what the previous bare in assertion could not catch.

#5 (worktree-copy hypothetical) — RESOLVED. Confirmed at kubernetes_spawner.py:1305-1309: spawn_overseer_job calls spawn_agent_job(..., repo_volumes=None, ...), so the overseer pod has no worktree mount. The new wording at kubernetes_spawner.py:1282-1283 and sandbox/agent-config/rules/overseer.md:32"the orchestrator vouches for it, and you do not need to verify its provenance against any other copy" — drops the misleading hypothetical comparison the agent could not actually perform from this pod's filesystem.

The two prompt sources stay in sync because sandbox/claude-rules is a symlink to sandbox/agent-config/rules (verified via ls -la sandbox/claude-rules), and sandbox/Dockerfile:194-196 is what materializes them at /opt/claude-rules/ in the image. The PR-touched file is the one the runtime sees.

#2 (stale ruff bullet in PR description) — STILL STALE. The ## Summary bullet still says .pre-commit-config.yaml ruff-pre-commit rev from v0.15.0 → v0.14.14, but .pre-commit-config.yaml:3 is at v0.15.12 from the main merge. PR author can't self-edit; flagging for the maintainer to strike before merge so a future bisecter doesn't get misled.

#4 (live verification) — STILL DEFERRED. Same status as before. Acceptable per the PR's explicit deferral; the calibration restart from #1962 should be paired with an end-to-end check on the verification follow-up issue.

No new issues

The delta is small (4 lines added / 6 deleted in the spawner, 7/3 in the test, 1/1 in the rules file) and surgically scoped. CI is green across all checks. The fix for #2196 — the load-bearing piece of this PR — is intact: provenance anchor is accurate, no diff-against-worktree confirmation command, no CRITICAL / DO NOT urgency stack to trip injection-detection heuristics, and the test now actually guards the standalone in-repo reference.

— Authored by egg

@james-in-a-box

This comment has been minimized.

@james-in-a-box

Copy link
Copy Markdown
Contributor
egg is addressing review feedback...

@james-in-a-box

Copy link
Copy Markdown
Contributor

Re-checking the latest review at c5fc890 — both actionable items were already addressed in that commit, and CI is green. No further code changes warranted.

Status of remaining notes from the latest review:

  • Phase 1.5: Documentation extraction #3 (test brittleness) — RESOLVED in c5fc890. test_overseer_prompt_mentions_prebuilt_monitor now asserts prompt.count("sandbox/overseer_monitor.py") >= 2, which independently guards the standalone in-repo provenance anchor.
  • Phase 3: Container extraction #5 (worktree-copy hypothetical) — RESOLVED in c5fc890. Both prompt sources drop the "worktree copy may be at a newer or older commit" framing in favor of "the orchestrator vouches for it, and you do not need to verify its provenance against any other copy" — matches the actual pod shape (spawn_overseer_job calls spawn_agent_job(..., repo_volumes=None, ...)).
  • Phase 1: Repository setup and CI infrastructure #2 (stale ruff bullet in PR description) — Confirmed unable to self-edit: gh pr edit 2284 --body-file ... returns ERROR: Edit denied: PR #2284 is not owned by james-in-a-box or configured user (author: jwbron). The gateway scopes description edits to the PR author. Flagging for the maintainer to strike the v0.15.0 → v0.14.14 bullet (and the corresponding test-plan checkbox) before merge — actual .pre-commit-config.yaml:3 is at v0.15.12 from the main merge.
  • Phase 2: Gateway module extraction #4 (live verification) — Still deferred per the PR notes; tracked in the verification follow-up issue alongside the Improve overseer escalation/issue opening behavior #1962 calibration restart.

— Authored by egg

@james-in-a-box

Copy link
Copy Markdown
Contributor

egg feedback addressed. View run logs

15 previous review(s) hidden.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Overseer agent rejects its own monitoring script as a prompt injection

1 participant