Update agent-mode design guide for LLM boundary rules - #877
Conversation
Add architectural conventions from PR #868 (LLM-calls-only-in-sandbox, EGG200 linter) and PR #873 (Claude Code headless pathway, model alias enforcement, EGG201 linter) to the agent-mode design guide, shared review criteria, GHA prompt builder inline fallback, orchestrator inline fallback, and workflow trigger paths.
There was a problem hiding this comment.
No agent-mode design concerns.
The new criteria items (#6–#8) and the "Architectural Conventions" section in the design guide are well-aligned with agent-mode principles. They codify that LLM calls must stay within sandbox containers and model references should use aliases — both reinforcing the core "sandbox is the constraint" model with linter-enforced boundaries (EGG200, EGG201) rather than prompt-level instructions. The criteria are consistent across all three locations (shared file, GHA fallback, orchestrator fallback).
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Thorough review of all five changed files. This is a documentation-and-criteria update — low risk, and the changes are well-structured. One issue to fix and one advisory note.
Issue: Misplaced linter reference in Architectural Conventions
File: docs/guides/agent-mode-design.md, lines ~98-99 (the "Use Claude Code headless mode" subsection)
The "Enforced by" line reads:
Enforced by:
EGG200linter (no direct Anthropic API imports/calls in orchestrator/gateway/shared) andEGG201linter (model alias form).
EGG201 enforces model alias form — it has nothing to do with Claude Code headless mode vs. direct API calls. The EGG201 reference belongs exclusively under the "Use model aliases" subsection (where it already appears correctly). Mentioning it here conflates two separate rules.
Fix: Remove the EGG201 reference from this line:
**Enforced by:** `EGG200` linter (no direct Anthropic API imports/calls in orchestrator/gateway/shared).Advisory: Items 6 and 7 overlap significantly
Items 6 ("Direct LLM API calls outside sandbox") and 7 ("Direct API calls bypassing Claude Code") describe very similar violations. Item 6 says "don't call the Anthropic API from orchestrator/gateway/shared." Item 7 says "use claude --print instead of raw HTTP calls to the Anthropic API." In practice, any violation of item 7 inside those components is also a violation of item 6.
This is a documentation choice, not a bug — the two rules come from different linters (EGG200 and EGG201 respectively) and have different scopes (item 6 is location-scoped to infra code; item 7 is about the calling mechanism regardless of location). But a reviewer encountering both for the first time may find the distinction unclear. Consider adding a brief note to item 7 clarifying how it differs from item 6 (e.g., item 7 also applies within sandbox code, not just infra).
Not blocking — just something to consider for clarity.
Everything else looks correct
- Workflow triggers (
sandbox/bin/**,shared/prompts/**): Appropriate additions given the new criteria reference files in those paths. - Three criteria locations stay in sync: The shared file (
shared/prompts/agent-design-criteria.md) is the most detailed; the action script and orchestrator fallbacks are reasonable simplifications with no semantic divergence. - Document structure: The new "Architectural Conventions" section and anti-patterns 3–4 are placed logically in the guide's flow.
- Anti-pattern code examples: The "wrong" and "right" examples in anti-patterns 3 and 4 are clear and accurate.
- Implementation checklist: New checklist items match the new criteria.
- Related links: PR #868 and #873 both exist and are merged with matching titles.
- Python string in
pipelines.py: Syntactically correct, properly continues the existing string concatenation pattern.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Both items addressed in a14ee0f:
-
EGG201 reference removed from the "Use Claude Code headless mode" enforced-by line. It now reads:
**Enforced by:** EGG200 linter (no direct Anthropic API imports/calls in orchestrator/gateway/shared). -
Item 6 vs 7 scope clarified. Added a note to item 7 across all three locations (shared criteria, GHA fallback, orchestrator fallback) explaining: "Unlike item 6 (scoped to infra code), this applies everywhere including sandbox code."
— Authored by egg
There was a problem hiding this comment.
No new agent-mode design concerns.
The delta since my last review (a14ee0f) addresses the base review bot's two suggestions: the misplaced EGG201 reference is removed from the "Use Claude Code headless mode" enforced-by line, and item 7 now clarifies its scope distinction from item 6 across all three criteria locations. Both fixes are correct and consistent.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review of changes since 4c40d96 (commit a14ee0f).
Previous feedback: both items addressed correctly
-
Misplaced EGG201 reference — Removed from the "Use Claude Code headless mode" enforced-by line in
agent-mode-design.md. The line now correctly references onlyEGG200. Fixed. -
Item 6 vs 7 scope clarification — All three criteria locations (shared file, GHA fallback, orchestrator fallback) now include the sentence explaining that item 7 applies everywhere including sandbox code, unlike item 6 which is scoped to infra code. Fixed.
New changes: no issues
The delta is 6 insertions / 4 deletions across 4 files. All changes are consistent and correct:
docs/guides/agent-mode-design.md: Clean removal of the strayEGG201reference.shared/prompts/agent-design-criteria.md: Scope clarification added to item 7, wording is clear.action/build-agent-mode-design-review-prompt.sh: Matching clarification in the GHA fallback.orchestrator/routes/pipelines.py: Python string concatenation is syntactically correct, matching clarification added.
The three criteria locations remain semantically consistent. Minor wording variation exists (shared file uses "which is scoped to infra code" while the other two use "scoped to infra code") but this is cosmetic and not worth a change.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
| egg is addressing review feedback... |
|
egg feedback addressed. View run logs 5 previous review(s) hidden. |
Update agent-mode design guide and reviewers for LLM boundary rules
PRs #868 and #873 introduced two new architectural boundaries enforced
by AST-based linters: LLM calls must only happen inside sandbox containers
(EGG200), all LLM calls should use Claude Code headless mode instead of
direct API calls, and model references must use short aliases instead of
pinned identifiers (EGG201). This PR updates the agent-mode design guide,
shared review criteria, and both the GHA and local SDLC reviewer inline
fallbacks to account for these new rules.
Changes span five files: the design guide gains a new "Architectural
Conventions" section and two new anti-patterns with code examples; the
shared criteria, GHA prompt builder fallback, and orchestrator fallback
each gain three new "What to Look For" items (#6–#8); and the GHA workflow
adds
sandbox/bin/**andshared/prompts/**to its trigger paths.Issue: none
Test plan:
pytest tests/action/test_build_agent_mode_design_review_prompt.py— 13 passedPYTHONPATH=shared pytest orchestrator/tests/test_pipeline_prompts.py— 153 passedpython scripts/check-llm-api-calls.py— OKpython scripts/check-model-versions.py— OKAuthored-by: egg