Rename PipelinePhase REFINE to ANALYZE - #966
Conversation
The "refine" phase produces an "analysis" document, making the phase name inconsistent with the other phases (plan → plan, implement → implementation, pr → PR). Renaming to "analyze" aligns the phase name with its output and removes the need for special-case display label mapping that was flagged in PR #960 review. Changes across 62 files: - Rename PipelinePhase.REFINE → ANALYZE in all 3 enum definitions - Update JSON schemas, phase-permissions config, and CLI choices - Add backwards-compat Pydantic validators to normalize legacy "refine" values from stored contracts and pipeline state - Add gateway API normalization for "refine" → "analyze" - Preserve agent role names (REFINER, REVIEWER_REFINE) and contract field names (refine_review_cycles, refine_review_feedback) to avoid breaking stored data Authored-by: egg
This comment has been minimized.
This comment has been minimized.
Autofix tracking{"Test/Unit Tests": 2} |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
![egg-reviewer[bot]](https://avatars.githubusercontent.com/u/8340608?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Rename PipelinePhase REFINE to ANALYZE
Thorough review of the 62-file rename. The mechanical rename is largely correct, backward compatibility validators are properly added, and tests are comprehensive. However, there are several issues that should be addressed.
1. CLAUDE.md symlink committed accidentally (Must Fix)
The diff includes a new file CLAUDE.md as a symlink (mode 120000) pointing to /home/egg/.claude/CLAUDE.md. This is a sandbox runtime artifact — that absolute path only exists inside the agent's Docker container. For any developer cloning the repo, this will be a broken symlink. It was picked up by the auto-commit in 8a9ce7b.
Fix: Remove CLAUDE.md from the PR branch (git rm CLAUDE.md). Consider adding it to .gitignore to prevent recurrence.
2. _phase_display_label is a no-op identity function (Bug)
sandbox/egg_lib/sdlc_hitl.py:34-36:
def _phase_display_label(phase: str) -> str:
"""Return human-readable label for a pipeline phase."""
return phaseThis replaced the old mapping phase_label = "analysis" if phase == "refine" else phase in _handle_phase_gate. The old code converted the phase name to a user-facing noun form ("analysis") for messages like "Approved: advancing from analysis" and "Changes requested for analysis".
The new _phase_display_label just returns phase unchanged, so users now see "Approved: advancing from analyze" — grammatically awkward.
Meanwhile, orchestrator/routes/pipelines.py:6217-6219 still has:
phase_label = (
"analysis" if current_phase.value == "analyze" else current_phase.value
)These are inconsistent. Either _phase_display_label should map "analyze" → "analysis" (matching the orchestrator), or the orchestrator mapping should also be removed. The PR description says the rename eliminates the need for the mapping, but the identity function doesn't actually produce the noun form.
3. CheckpointV2.validate_pipeline_phase has no backward compat for "refine" (Bug)
shared/egg_contracts/checkpoints.py:247-254:
valid_phases = {"analyze", "plan", "implement", "pr"}
if v not in valid_phases:
msg = f"pipeline_phase must be one of {valid_phases}"
raise ValueError(msg)The Contract and Pipeline models both have field_validators that normalize "refine" → "analyze", but CheckpointV2 has no such normalization. Loading a historical checkpoint with pipeline_phase: "refine" will raise a ValueError. Either add the same "refine" → "analyze" normalization here, or document that historical checkpoints with the old phase value will fail validation.
4. Stale documentation references (Minor)
Several documentation files still reference "refine" as a phase name:
gateway/phase_api.py:475— docstring lists"refine"as valid phasegateway/README.md:181— API docs show"refine"as valid phase valuedocs/hitl-decisions.md:268— example code usesphase="refine"
These should be updated for consistency.
5. Contract schema does not accept "refine" for backward compat
.egg/schemas/contract.schema.json:57 now has:
"enum": ["analyze", "plan", "implement", "pr"]Any external tooling or scripts that validate contracts against this JSON schema will reject historical contracts with "refine". The Pydantic validators handle this at runtime, but schema-level consumers (e.g., IDE validation, CI schema checks) will break. Consider keeping "refine" in the schema enum or adding a note that the schema is forward-only.
Items 1-3 are blocking. Items 4-5 are advisory.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1. Remove accidentally committed CLAUDE.md symlink and add to .gitignore 2. Fix _phase_display_label to map "analyze" -> "analysis" (was identity fn) 3. Add backward compat for "refine" in CheckpointV2.validate_pipeline_phase 4. Update stale "refine" references in docs and error messages 5. Add "refine" to contract schema enums for backward compatibility
There was a problem hiding this comment.
All five review items addressed in 3e9d11c:
1. CLAUDE.md symlink (Must Fix) — Fixed. Removed from tracking via git rm and added to .gitignore to prevent recurrence.
2. _phase_display_label no-op (Bug) — Fixed. Now maps "analyze" → "analysis" to match the orchestrator's phase_label logic. Users see "Approved: advancing from analysis" as intended.
3. CheckpointV2.validate_pipeline_phase missing backward compat (Bug) — Fixed. Added mode="before" validator that normalizes "refine" → "analyze", consistent with Contract._normalize_legacy_phase. Historical checkpoints with pipeline_phase: "refine" now load correctly.
4. Stale documentation references (Minor) — Fixed. Updated:
gateway/phase_api.pydocstring:refine→analyzegateway/README.mdAPI docs:refine→analyzedocs/hitl-decisions.mdexample:phase="refine"→phase="analyze"shared/egg_container/__init__.pyerror message:refine→analyze
5. Contract schema backward compat (Advisory) — Fixed. Added "refine" to all three phase enum arrays in contract.schema.json so external tooling and schema validators accept historical contracts.
All 450 related tests pass. Lint clean.
— 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.
Re-review: Rename PipelinePhase REFINE to ANALYZE (delta from 8a9ce7b → 3e9d11c)
The five review items from the first pass are addressed. The fixes for items 1–3 are correct. However, the fixes for items 4 and 5 are incomplete — several stale references and a missing normalizer remain.
Status of Previous Items
| # | Issue | Status |
|---|---|---|
| 1 | CLAUDE.md symlink | Fixed — removed and added to .gitignore |
| 2 | _phase_display_label no-op |
Fixed — correctly maps "analyze" → "analysis" |
| 3 | CheckpointV2.validate_pipeline_phase missing backward compat |
Fixed — mode="before" validator added |
| 4 | Stale documentation references | Partially fixed — see below |
| 5 | Contract schema backward compat | Fixed — "refine" added to all three enum arrays |
New Issue 1: CheckpointSummaryV2 missing backward-compat normalizer (Bug)
shared/egg_contracts/checkpoints.py:287 — CheckpointSummaryV2.pipeline_phase has no field_validator to normalize "refine" → "analyze", unlike CheckpointV2 which now has one.
CheckpointSummaryV2 is deserialized from stored JSON via CheckpointIndexV2.model_validate(data) (called in checkpoint_loader.py:212, checkpoint_cli.py:248, checkpoint_handler.py:993). Historical index files containing summaries with pipeline_phase: "refine" will retain the old value.
This breaks phase-based queries: get_by_phase("analyze") (checkpoints.py:405) won't find entries indexed under "refine" in by_phase. Similarly, the by_phase dict keys loaded from a pre-rename index file will contain "refine" entries that are invisible to "analyze" lookups.
Fix: Add the same field_validator from CheckpointV2.validate_pipeline_phase to CheckpointSummaryV2.
New Issue 2: Remaining stale "refine" references in docs and strings
The delta fixed 3 of the 4 doc references I flagged, but missed several others. These fall into two categories:
User-facing error message (should fix):
gateway/phase_filter.py:423—"Cannot create PRs during refine"→ should say "during analyze"
Doc/comment references to the phase name (should fix for consistency):
gateway/README.md:161— phase permissions table:| **refine** |→| **analyze** |docs/hitl-decisions.md:18— "during refine and plan phases" → "during analyze and plan phases"docs/hitl-decisions.md:150— "refine and plan phases" → "analyze and plan phases"orchestrator/models.py:267— field description: "after refine and plan phases" → "after analyze and plan phases"
Comments/docstrings (advisory, lower priority):
gateway/phase_filter.py:5, 484, 605, 876— docstrings list "refine" as a phase namedocs/hitl-decisions.md:8— "Refine phase" header text
New Issue 3: checkpoint.schema.json and usage.schema.json not updated (Advisory)
These schemas still use "refine" as the only phase value with no "analyze":
.egg/schemas/checkpoint.schema.json:73—"enum": ["refine", "plan", "implement", "pr", null].egg/schemas/usage.schema.json:190, 285—"enum": ["refine", "plan", "implement", "pr"]
These files aren't in the PR diff at all. They need the same treatment as contract.schema.json — either replace "refine" with "analyze" or add both values. Since contract.schema.json keeps both for backward compat, these should too.
Issue 1 is blocking — it's the same class of bug as original item #3 and will cause silent data loss in phase-based checkpoint queries. Issues 2–3 are lower priority but straightforward to fix in the same commit.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
- Add backward-compat field_validator to CheckpointSummaryV2 to normalize "refine" -> "analyze", matching the existing CheckpointV2 normalizer. Without this, phase-based queries on historical checkpoint indexes would silently miss entries stored under the old "refine" key. - Fix remaining stale "refine" references in docs and error messages: gateway/phase_filter.py (docstrings + error message), gateway/README.md (phase permissions table), docs/hitl-decisions.md (three references), orchestrator/models.py (two field descriptions). - Update checkpoint.schema.json and usage.schema.json to include "analyze" (keeping "refine" for backward compat), matching contract.schema.json.
There was a problem hiding this comment.
All three review items from the re-review addressed in d0b8cdb:
New Issue 1: CheckpointSummaryV2 missing backward-compat normalizer (Bug) — Fixed. Added the same field_validator("pipeline_phase", mode="before") that normalizes "refine" → "analyze", matching CheckpointV2. Historical index files with pipeline_phase: "refine" now normalize correctly, so get_by_phase("analyze") finds all entries.
New Issue 2: Remaining stale "refine" references — Fixed. Updated:
gateway/phase_filter.py: error message ("Cannot create PRs during analyze") + all 4 docstring referencesgateway/README.md: phase permissions table rowdocs/hitl-decisions.md: 3 references (phase header, decision sync description, phase completion comment)orchestrator/models.py: 2 field descriptions (hitl_gates,allow_short_circuit)
New Issue 3: checkpoint.schema.json and usage.schema.json not updated (Advisory) — Fixed. Added "analyze" to all phase enum arrays, keeping "refine" for backward compat (matching the contract.schema.json approach).
All 1575 related tests pass. Lint clean.
— Authored by egg
There was a problem hiding this comment.
Re-review (3rd pass): Rename PipelinePhase REFINE to ANALYZE
Delta reviewed: 3e9d11c → d0b8cdb
Status of Previous Items
All three issues from the second review are properly fixed:
| # | Issue | Status |
|---|---|---|
| 1 | CheckpointSummaryV2 missing backward-compat normalizer |
Fixed — identical field_validator added, matches CheckpointV2 |
| 2 | Remaining stale "refine" in phase_filter, README, docs, models | Fixed — all flagged references updated |
| 3 | checkpoint.schema.json and usage.schema.json not updated |
Fixed — "analyze" added as primary, "refine" kept for compat |
New Issue 1: CheckpointIndexV2.by_phase dict keys not normalized on load (Bug)
shared/egg_contracts/checkpoints.py:381 — by_phase: dict[str, list[str]] is a plain dict whose keys are not processed by any validator.
When load_checkpoint_index_v2 (checkpoint_loader.py:212) calls CheckpointIndexV2.model_validate(data) on a pre-rename index file, the CheckpointSummaryV2 objects get their pipeline_phase normalized from "refine" to "analyze" (via the new field validator). But the by_phase secondary index dict retains its original keys — so it still has {"refine": ["ckpt-1", "ckpt-2"]}.
This causes get_by_phase("analyze") (checkpoints.py:419) to return an empty list for those entries, while get_by_phase("refine") would find them. The primary index (summaries) and secondary index (by_phase dict) are now inconsistent.
Impact: filter_checkpoints_v2 (checkpoint_loader.py:456) uses index.get_by_phase(pipeline_phase) for phase-based queries. After the rename, egg-checkpoint list --phase analyze silently misses all historical checkpoints that were indexed under "refine".
Fix: Add a model_validator(mode="after") on CheckpointIndexV2 that merges by_phase.get("refine", []) entries into by_phase["analyze"] and deletes the "refine" key. Alternatively, normalize in get_by_phase by checking both keys. The model_validator approach is cleaner since it fixes the inconsistency at load time, once.
New Issue 2: Broad stale "refine" phase references in documentation (Advisory)
The PR correctly updated docs/hitl-decisions.md and gateway/README.md, but many other documentation files still reference "refine" as a phase name. These are outside the current PR diff, so this is advisory — but since the PR's purpose is this rename, they should be addressed either here or in a follow-up:
High-traffic files:
README.md— 3 references ("refine and plan gates", "refine phase signals", "refine → implement")docs/guides/sdlc-pipeline.md— ~15 references throughout (phase descriptions, review cycles, short-circuit docs)docs/guides/local-quickstart.md— 3 references (phase flow, labels)docs/index.md— "refine phase" in template description
Architecture/ADR docs:
docs/architecture/README.md— phase flow diagram, CLI filter exampledocs/architecture/orchestrator.md— draft naming, reviewer descriptionsdocs/adr/implemented/ADR-SDLC-Pipeline.md— phase table
Templates:
docs/templates/analysis.md—Phase: refinedocs/templates/phase-completion.md— phase name list
These don't affect runtime behavior, but having the code say "analyze" while the docs say "refine" will confuse anyone onboarding or referencing the docs. Consider a follow-up PR if you don't want to expand this one's scope further.
Issue 1 is blocking — it's the same class of bug as the CheckpointSummaryV2 normalizer gap from the previous review, but at the index dict level. Issue 2 is advisory.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1. CheckpointIndexV2.by_phase dict keys not normalized (blocking): Added model_validator(mode="after") that merges by_phase["refine"] entries into by_phase["analyze"], fixing silent data loss in phase-based checkpoint queries on historical index files. 2. Stale "refine" phase references in docs (advisory): Updated all documentation files to use "analyze" as the phase name: README.md, sdlc-pipeline.md, local-quickstart.md, index.md, architecture/README.md, orchestrator.md, ADR-SDLC-Pipeline.md, templates/analysis.md, templates/phase-completion.md.
There was a problem hiding this comment.
All three review items from the third review addressed in e6c945b:
New Issue 1: CheckpointIndexV2.by_phase dict keys not normalized on load (Bug) — Fixed. Added model_validator(mode="after") on CheckpointIndexV2 that merges by_phase["refine"] entries into by_phase["analyze"] at load time, with deduplication. Historical index files with by_phase: {"refine": [...]} now normalize correctly, so get_by_phase("analyze") finds all entries including those originally indexed under "refine".
New Issue 2: Remaining stale "refine" references in docs (Advisory) — Fixed. Updated all 9 documentation files:
README.md: Phase diagram, gate references, short-circuit description, tier tabledocs/guides/sdlc-pipeline.md: ~20 references (phase names, agent references, review cycle docs, phase restrictions, decision sync, check descriptions)docs/guides/local-quickstart.md: Phase flow, phase restrictions, issue labelsdocs/index.md: Template descriptiondocs/architecture/README.md: Phase flow, CLI filter, check descriptiondocs/architecture/orchestrator.md: Draft path description, reviewer execution orderdocs/adr/implemented/ADR-SDLC-Pipeline.md: Phase flow, phase tabledocs/templates/analysis.md: Phase metadatadocs/templates/phase-completion.md: Phase name list, example heading
Note: References to the "Refine" reviewer type (e.g., reviewer_refine, "reviewer": "refine") are left unchanged — these refer to the reviewer agent specialization, not the pipeline phase name.
New Issue 3: checkpoint.schema.json and usage.schema.json (Advisory) — Already fixed in d0b8cdb (prior commit). Both schemas already have "analyze" added with "refine" kept for backward compat.
All 7268 tests pass. Lint clean.
— Authored by egg
There was a problem hiding this comment.
No agent-mode design concerns.
This is a mechanical rename (REFINE → ANALYZE) with backwards-compatibility normalization. The changes don't introduce any agent-mode anti-patterns — no pre-fetching, no structured output for humans, no post-processing pipelines, no direct LLM API calls, no hardcoded model identifiers. The Pydantic field_validator normalization for legacy data and the gateway API normalization are infrastructure concerns that don't affect agent autonomy.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review (4th pass): Rename PipelinePhase REFINE to ANALYZE
Delta reviewed: d0b8cdb → e6c945b
Status of Previous Items
All three issues from the third review are properly fixed:
| # | Issue | Status |
|---|---|---|
| 1 | CheckpointIndexV2.by_phase dict keys not normalized on load |
Fixed — model_validator(mode="after") merges "refine" entries into "analyze" with dedup |
| 2 | Remaining stale "refine" in docs | Fixed — all 9 flagged doc files updated |
| 3 | checkpoint.schema.json / usage.schema.json not updated |
Fixed — "analyze" added, "refine" kept for compat |
The CheckpointIndexV2._normalize_legacy_phase_index validator (checkpoints.py:397-407) is well-implemented: it pops "refine" from by_phase, merges into "analyze" without duplicates, and runs at model construction time. This correctly resolves the blocking bug from the previous review.
New Issue 1: phase_filter.py:498 — stale description string (Should Fix)
PipelinePhase.ANALYZE: PhaseFileRestriction(
...
description="Refine phase can only push contracts, analysis drafts, checkpoints, agent outputs, and reviews",
),This description is surfaced in error messages when a push is rejected. It should say "Analyze phase" to match the renamed phase. The PR updated all four docstring references in this file (lines 5, 484, 605, 876) and the blocked-operation description (line 423: "Cannot create PRs during analyze"), but missed this one.
New Issue 2: Missing test coverage for backward-compat normalizers (Should Fix)
The PR adds four backward-compat normalizers but only tests one of them:
| Normalizer | Model | Tested? |
|---|---|---|
Contract._normalize_legacy_phase |
Contract | Yes — test_contract_backward_compatibility_refine_normalizes_to_analyze |
CheckpointV2.validate_pipeline_phase |
CheckpointV2 | No — existing test was updated for forward-compat ("analyze" accepted) but no test passes "refine" and asserts normalization |
CheckpointSummaryV2.validate_pipeline_phase |
CheckpointSummaryV2 | No |
CheckpointIndexV2._normalize_legacy_phase_index |
CheckpointIndexV2 | No |
Pipeline._normalize_legacy_phase |
Pipeline (orchestrator) | No |
The CheckpointIndexV2 normalizer is the most critical — it was the blocking bug from the previous review. A test that constructs an index with by_phase: {"refine": ["ckpt-1"]} and verifies get_by_phase("analyze") returns ["ckpt-1"] would prevent regression.
New Issue 3: Stale references in files touched by this PR (Advisory)
Several files within the PR diff still contain stale "refine" phase references. These are all non-blocking since they're in section headers, field descriptions, or doc prose — but they create inconsistency within the same files that were otherwise updated:
docs/architecture/orchestrator.md (2 lines updated, 5 missed):
- Line 138:
| Directory | Implement phase | Refine/Plan phases |→ should be "Analyze/Plan" - Line 182:
### Refine Phase Roles→ should be "Analyze Phase Roles" - Line 186: table row references the phase section
- Line 188:
Refiner runs first— acceptable (role name), but the sentence context is about the phase - Line 191:
Refine Reviewer— acceptable (reviewer type name)
shared/egg_contracts/models.py (field descriptions, not blocking):
- Line 434:
description="Number of refine phase review cycles"— the field namerefine_review_cyclescan't change (persisted), but the description could say "analyze phase" - Line 436:
description="Feedback from last refine review"— same
docs/guides/sdlc-pipeline.md:
- Line 442: Comment says
# Refine review verdict— could be clearer as "Analyze phase refine-type review verdict"
Out-of-scope references (not in PR diff, for follow-up)
These files were NOT touched by the PR but contain stale phase references:
orchestrator/README.md:49— phase table shows| **refine** |sandbox/egg_lib/data/hitl_editing_rules.md:14—**refine** phasedocs/agentic-feedback-loop.md:74,151,155— "REFINE PHASE" diagram and prosetests/workflows/test_hitl_integration.py:81—"## Refine Phase Complete"tests/workflows/test_multi_reviewer.py:131-133,290— test uses"refine"as phase name inget_default_reviewers
These should be tracked as a follow-up task.
Issues 1–2 are non-blocking but straightforward to fix. Issue 3 is advisory. The core implementation (enum rename, backward-compat validators, schema updates, doc updates) is solid.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Fix stale description, add backward-compat normalizer tests, update remaining doc references in files touched by this PR.
|
Automated feedback loop has reached the maximum of 5 rounds. Human review is needed to make further progress on this PR. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…s and update stale references Resolved conflicts in 4 files and updated stale REFINE phase references in auto-merged coordinator code from main.
Conflict Resolution SummaryResolved merge conflicts with Conflicts Resolved (4 files)
Stale References Fixed (10 additional files)Auto-merged code from main introduced new coordinator features that used the old
Not Changed (intentionally)
Verification
— Authored by egg |
|
egg conflict resolution completed. View run logs |
|
egg is resolving merge conflicts for this PR... |
There was a problem hiding this comment.
No agent-mode design concerns.
The rename is mechanical and clean. Two additional changes worth noting as positive from an agent-design perspective:
-
MCP server auth (
orchestrator/mcp_server.py): Moves from implicit Docker port-mapping security to explicit gateway token validation. This aligns well with the principle that security constraints should be sandbox-enforced via the gateway, not reliant on infrastructure assumptions. Good change. -
Concurrent mode simplification (
docs/guides/sdlc-pipeline.md): Reduces concurrent agents from 6 (coder, tester, documenter, checker, reviewer_code, reviewer_contract) to 4 (coder, tester, documenter, integrator), with the integrator now running as a concurrent participant rather than a separate post-consensus step. Pragmatic simplification that reduces coordination complexity.
— Authored by egg
|
egg agent-mode-design completed. View run logs 25 previous review(s) hidden. |
There was a problem hiding this comment.
Re-review (6th pass): Rename PipelinePhase REFINE to ANALYZE
Delta reviewed: 65e0e0e → 2a1728f (merge of origin/main into PR branch)
Status of Previous Items
All items from reviews 1–5 remain properly addressed. No regressions introduced by the merge.
Merge Analysis
The merge commit (2a1728f) brings in 14 commits from main (#1021–#1047), including substantial new functionality: coordinator agent, concurrent execution, cross-agent messaging, and consensus-driven phase advancement. The merge message states "Resolved conflicts in 4 files and updated stale REFINE phase references in auto-merged coordinator code from main."
I examined every file modified or added by the merge for stale "refine" phase references. The results:
All new code is clean. The following files were verified to use "analyze" correctly for all phase references:
orchestrator/routes/coordinator.py(854 lines, new)orchestrator/concurrent_executor.py,consensus.py,coordinator_executor.py,message_store.py,mcp_server.py,mcp_tools.py(new)orchestrator/routes/messages.py,routes/signals.py,routes/phases.py(new/modified)gateway/phase_filter.py,gateway/agent_restrictions.py,gateway/checkpoint_handler.py,gateway/gateway.py(modified)orchestrator/models.py,orchestrator/routes/pipelines.py,orchestrator/container_spawner.py(modified)- All sandbox command files, docs, and schemas (new/modified)
Backward compat code is correct. The gateway.py normalization (if phase == "refine" → "analyze") is intentional legacy migration and properly placed.
.egg-state/reviews/ files with refine in filenames (e.g., 1027-refine-refine-review.json): These are historical artifacts from completed pipelines. Review file paths are constructed dynamically using the current phase value, so new reviews use {id}-analyze-{type}-review.json. Old files are inert — no issue.
Issue 1: orchestrator/models.py:78 — stale comment (Advisory)
# Refine-phase roles
REFINER = "refiner"The comment says "Refine-phase" but the phase is now "analyze". Should read # Analyze-phase roles. The REFINER role name itself is correct (it's the agent role, not the phase). This was present before the merge and survived all 5 prior rounds. Non-blocking since it's a comment.
Verification Summary
| Area | Status |
|---|---|
| Conflict resolution (4 files) | Correct — phase references use "analyze" |
| New orchestrator code (coordinator, concurrent, messaging) | Clean — no stale phase refs |
| New gateway code (agent restrictions, checkpoint handler) | Clean |
| New sandbox commands/rules | Clean |
| New docs (coordinator guide, sdlc-pipeline additions) | Clean |
| Backward compat normalizers | Still functioning correctly |
| Schema files | Correct — "analyze" primary, "refine" kept for compat |
| Phase-permissions.json | Correct — uses "analyze" |
| All 5 prior review rounds | No regressions |
No new blocking issues. The merge was handled correctly.
— Authored by egg
|
Automated feedback loop has reached the maximum of 5 rounds. Human review is needed to make further progress on this PR. |
|
egg review completed. View run logs 25 previous review(s) hidden. |
Conflict Resolution SummaryResolved merge conflicts with Incoming commits:
Result: Git auto-merged all files cleanly — no manual conflict resolution was needed.
Stale reference check: Verified that the auto-merged files from Verification:
— Authored by egg |
|
egg conflict resolution completed. View run logs |
There was a problem hiding this comment.
No agent-mode design concerns in the delta (2a1728f → ded7fd5).
The only change since my last review is a merge from main bringing in PRs #1046, #1047, and #1048. These were already reviewed in their own PRs. No new code from this branch's author.
Notably, the MCP server auth change (#1048) — replacing gateway token validation with localhost-only Docker port binding — is a positive shift from an agent-design perspective. It moves security enforcement from application-level middleware to infrastructure-level network isolation, which aligns with the principle that security constraints should be sandbox-enforced rather than prompt/app-level.
— Authored by egg
|
egg agent-mode-design completed. View run logs 25 previous review(s) hidden. |
There was a problem hiding this comment.
Re-review (6th pass): Rename PipelinePhase REFINE to ANALYZE
Delta reviewed: 2a1728f8 → ded7fd58 (auto-resolved merge from main)
Status of Previous Items
All items from the 5th review were addressed in eb80398. No regressions.
Merge Analysis
The merge brought in two sets of changes from main:
-
MCP server auth simplification (#1048): Removed gateway token validation, switched to localhost-only security via Docker port binding (
127.0.0.1:9850:9850). Files:orchestrator/mcp_server.py,orchestrator/api.py,docker-compose.yml,docs/guides/coordinator.md, and three test files. -
Concurrent mode 6-agent docs (#1046): Updated
docs/guides/sdlc-pipeline.mdto reflect the expanded agent set (coder, tester, documenter, checker, reviewer_code, reviewer_contract) and the integrator's non-concurrent role. -
Minor fix in
sandbox/.claude/rules/mission.md: Documenter polling description now includes tester PROGRESS.
No new stale "refine" phase references introduced by the merge. All phase terminology in the merged concurrent mode documentation correctly uses "analyze". The MCP changes have no phase-related content.
Verification
Searched the full PR branch state for remaining "refine" references. All surviving instances fall into acceptable categories:
- Backward-compat normalizers (5 validators): Correctly convert legacy
"refine"→"analyze"at deserialization time - Agent role names:
REFINER,REVIEWER_REFINE— these describe the actor, not the phase (per PR description) - Reviewer type strings:
"refine"inorchestrator/routes/pipelines.pyand docs — these are reviewer specialization identifiers, not phase names - Persisted field names:
refine_review_cycles,refine_review_feedback— stored in JSON contracts, cannot be renamed
No new issues found.
— Authored by egg
|
Automated feedback loop has reached the maximum of 5 rounds. Human review is needed to make further progress on this PR. |
|
egg review completed. View run logs 25 previous review(s) hidden. |
|
egg is resolving merge conflicts for this PR... |
…s in gateway and orchestrator models Resolved conflicts where main centralized PipelinePhase into egg_contracts.models while this branch renamed REFINE to ANALYZE. Took main's centralized import approach since the canonical PipelinePhase already has the ANALYZE rename applied.
Conflict Resolution SummaryResolved merge conflicts with
Root cause: Main (PRs #1049–#1053) centralized Verification:
— Authored by egg |
|
egg conflict resolution completed. View run logs |
There was a problem hiding this comment.
No agent-mode design concerns in the delta (ded7fd5 → 767551b).
The changes since my last review are a merge from main plus a formatting fixup. The merged code includes several changes that are positive from an agent-design perspective:
-
Coordinator sandbox enforcement: The coordinator container now mounts a
tmpfsat/home/egg/repos, physically preventing repository access. This is a textbook example of sandbox enforcement over behavioral instructions — the coordinator cannot write code because the capability doesn't exist in its environment. -
Coordinator rules clarification (
sandbox/.claude/rules/coordinator.md): Updated to explicitly state "You do NOT write code, run tests, or touch the repository yourself" with clear constraint boundaries. Goal-oriented, not procedurally micromanaged. -
MCP server upgrade (
orchestrator/mcp_server.py): Migrated from custom Flask/SSE to the officialmcpPython SDK with Streamable HTTP transport. Clean infrastructure improvement, no agent-mode impact.
No pre-fetching, structured output for humans, post-processing pipelines, direct LLM API calls, or hardcoded model identifiers introduced.
Note: This PR is closed (not merged), so this review is for completeness.
— Authored by egg
|
egg agent-mode-design completed. View run logs 25 previous review(s) hidden. |
There was a problem hiding this comment.
Re-review (6th pass): Rename PipelinePhase REFINE to ANALYZE
Delta reviewed: ded7fd58 → 767551b7 (2 commits: merge with main + import formatting fix)
What Changed
Two commits since my last review:
d5957d17— Mergeorigin/mainintoegg/rename-refine-to-analyze. Main centralizedPipelinePhaseintoegg_contracts.models, removing duplicate definitions fromgateway/phase_filter.pyandorchestrator/models.py. Conflicts resolved in gateway and orchestrator.767551b7— Removed extra blank line inorchestrator/models.pyafter the merge.
Merge Conflict Resolution: Verified Correct
The merge correctly handled the interaction between main's centralization and this PR's rename:
| File | Resolution | Status |
|---|---|---|
orchestrator/models.py |
Local PipelinePhase class removed, imports from egg_contracts.models |
Correct — field_validator import preserved for backward-compat normalizer |
gateway/phase_filter.py |
Local PipelinePhase class removed, imports from egg_contracts.models via sys.path manipulation |
Correct — re-export alias PipelinePhase as PipelinePhase enables gateway.py import |
gateway/gateway.py |
PipelinePhase now imported from phase_filter, VALID_PIPELINE_PHASES derived from enum |
Correct — matches main's pattern, includes backward-compat normalizers |
Previous Review Items: All Still Intact
Verified that all fixes from reviews 1–5 survived the merge:
_phase_display_labelmaps"analyze"→"analysis"✅- All 4 backward-compat normalizers (Contract, Pipeline, CheckpointV2, CheckpointSummaryV2) present ✅
CheckpointIndexV2._normalize_legacy_phase_indexmodel validator present ✅- All 6 backward-compat normalizer tests present ✅
phase_filter.py:532description says "Analyze phase" ✅- Schemas include both
"analyze"and"refine"for backward compat ✅ - Docs correctly distinguish phase name ("Analyze") from reviewer type ("Refine") ✅
- Only one
PipelinePhasedefinition exists (inshared/egg_contracts/models.py) ✅ - Zero references to
PipelinePhase.REFINEin code ✅ - No merge conflict markers in any files ✅
No New Issues Found
The merge is clean. The conflict resolution correctly adopted main's centralized import pattern while preserving this PR's REFINE→ANALYZE rename and all backward-compatibility normalizers.
— Authored by egg
|
egg review completed. View run logs 25 previous review(s) hidden. |
1 participant
Rename PipelinePhase from "refine" to "analyze" for consistency
The "refine" phase produces an "analysis" document, but its name didn't match
this output — unlike plan (→ plan), implement (→ implementation), and pr (→ PR).
This inconsistency required a special-case display label mapping in
sdlc_hitl.pythat was flagged in PR #960 review (point 4).
Renaming to "analyze" eliminates the naming mismatch. The phase name now naturally
produces "analysis" as its noun form, matching the existing pattern of other phases.
Scope: 62 files across all components (shared, gateway, orchestrator, sandbox,
schemas, and all test suites). 5653 unit tests pass, lint clean.
Backwards compatibility:
field_validatoronContract.current_phaseandPipeline.current_phasenormalizes legacy
"refine"→"analyze"when loading historical data"refine"→"analyze"before validationREFINER,REVIEWER_REFINE) are unchanged — they describethe actor, not the phase, and are stored in checkpoint/pipeline state
refine_review_cycles,refine_review_feedback) areunchanged — stored in 361+ JSON contract files
Issue: #960 (review)
Test plan:
"current_phase": "refine"andconfirm it normalizes to
PipelinePhase.ANALYZEAuthored-by: egg