docs: document green-gate fix: config key [doc-updater] - #3629
Conversation
Cross-references #3409's Stage A auto-remediation (config/repo_config.py, shared/egg_config/validators.py): a check entry in repositories.yaml may now carry an optional fix command that the per-slice green gate runs and, on success, commits + pushes to the integration branch. docs/architecture/slice-dag.md already documents the gate behavior; this fills in the schema docs (config/README.md, docs/guides/sdlc-pipeline.md) and the file-index one-liner (docs/development/STRUCTURE.md) that describe the checks: config option itself.
![egg-reviewer[bot]](https://avatars.githubusercontent.com/u/8340608?s=60&v=4){kind=small}
There was a problem hiding this comment.
No agent-mode design concerns.
Docs-only change (5 additions across config/README.md, docs/guides/sdlc-pipeline.md, docs/development/STRUCTURE.md). No prompt assembly, pre-fetched context, structured-output requirement, post-processing pipeline, or model identifier is touched.
Spot-checked that the documented fix: semantics match orchestrator/slice_green_gate.py (Stage A: run fix → re-run check → commit + push only if every red check re-ran green; checks without fix block like Stage B) and are consistent with the existing references in config/repositories.yaml.example and docs/architecture/slice-dag.md.
Worth noting as a positive: "Only configure deterministic auto-remediations here" is the right steer — it keeps nondeterministic/agent-shaped commands out of a fix: key that the orchestrator runs and commits unattended.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Review: docs — green-gate autofix fix: config key
Docs-only PR (5 additions, 1 deletion). I verified every claim in the new prose against orchestrator/slice_green_gate.py, shared/egg_config/validators.py, and the tester prompt builder rather than reviewing the wording in isolation.
No blocking issues. The fix key is real end-to-end, the two updated files were genuinely the only remaining schema references missing it, and every added statement is true. The findings below are all accuracy/completeness gaps where the new prose states a necessary condition as if it were sufficient — the direction of the imprecision is conservative (the gate refuses to self-heal in cases the docs imply it would; it never self-heals in a case the docs say it wouldn't), which is why these are advisory rather than blocking.
Verified correct
fixsurvives config load and reaches the gate:shared/egg_config/validators.py:191-192→orchestrator/routes/pipelines/__init__.py:475-476→orchestrator/slice_green_gate.py:553-570. Nothing strips it; no schema validator rejects it (scripts/validate-config.pydoes not gate on check keys).- Coverage claim in the PR body holds. Grepping every
checks:schema mention acrossdocs/andconfig/returns exactly four sites:config/repositories.yaml.example:80-86,148,docs/architecture/slice-dag.md:559-571, and the two this PR fixes (config/README.md:194,docs/guides/sdlc-pipeline.md:992). No fifth site was missed. fixis correctly scoped to the green gate: the tester prompt renders onlyname/command(orchestrator/routes/pipelines/_prompt_agent.py:426-429), so the docs are right that this is not a tester-step feature.- The cross-reference anchor at the end of the
sdlc-pipeline.mdsection still resolves —config/README.md:176is### Per-Repo Check Commands. - The YAML example is valid:
fix: npm run lint -- --fix # optional: …has the required whitespace before#, so the value parses as the command, not the comment.
Non-blocking 1 — the commit/push condition is under-stated in all three files
All three new/edited strings say the gate commits and pushes "if every red check's fix turned it green":
config/README.md:196docs/guides/sdlc-pipeline.md:1000docs/development/STRUCTURE.md:113
_autofix_ready (orchestrator/slice_green_gate.py:1054-1098) requires three more things:
final_verification.ranandall_ok— one full re-run of every configured check against the all-fixes-applied tree (slice_green_gate.py:1082-1087). This is not implied by the per-check re-runs: the runner's own comment atslice_green_gate.py:576-583spells out that each per-check re-run validated an intermediate tree (check i re-ran before check i+1's fix, and originally-green checks were never re-run). Concrete case:lintis red,fix: make lint-fixruns, lint re-runs green — the documented condition is satisfied — but the reformat breaks a golden-output test that was green before and never re-ran.final_verification.all_okis false,_autofix_readyreturns(False, "final full re-run of all checks was not green …"), and the slice blocks. Per these docs it should have committed and pushed.final.new_untracked_count == 0, withNonetreated as unsafe (slice_green_gate.py:1089-1097) — the orchestrator stages withgit add -u, which never picks up new files, so anyfixthat emits a file (codegen, snapshot regeneration) is refused._commit_and_push_autofixrefuses whengit add -ustages nothing (slice_green_gate.py:1183-1190).
docs/architecture/slice-dag.md:566-571 — the doc this PR's body cites as already-correct precedent — states conditions 1 and 2 explicitly. The two schema references this PR updates should not be materially weaker than it. Suggested minimum edit for config/README.md:196 and sdlc-pipeline.md:1000:
… it runs the fix, re-runs the check, and — if every red check's fix turned it green and a final full re-run of every configured check against the fixed tree is also green with no newly created untracked files — commits and pushes the result …
Non-blocking 2 — on-mode gating is omitted
Autofix commits and pushes only under EGG_SLICE_GREEN_GATE=on (slice_green_gate.py:1518: if autofix_ready and mode == "on" and repo_host_dir:). Under log the runner still executes the fix — the runner script is mode-blind (slice_green_gate.py:553-554) — but nothing is committed or pushed (slice_green_gate.py:1544-1551). The module docstring states this at slice_green_gate.py:159-161; slice-dag.md:567 states it ("it fires only in on mode").
These two files are what an operator reads while editing repositories.yaml, and neither mentions EGG_SLICE_GREEN_GATE at all. An operator running the documented log soak stage will configure fix:, watch the fix run in the runner logs, and see no commit. Add "under EGG_SLICE_GREEN_GATE=on" plus a pointer to docs/architecture/slice-dag.md for the rollout switch.
Non-blocking 3 — scope of fix relative to the surrounding text (config/README.md)
The new bullet at config/README.md:196 is immediately followed by the pre-existing line "Checks run sequentially during the implement phase tester step." The bullet does say "per-slice green gate", but the juxtaposition invites the reading that fix participates in the tester step. It does not (_prompt_agent.py:426-429), and the green gate only runs for slice-DAG implement pipelines. An operator on a non-slice pipeline who sets fix gets silence. One clause would close it: "fix is used only by the per-slice green gate (slice-DAG pipelines); the tester step ignores it."
Same applies to sdlc-pipeline.md:1000 for a different reason: that section also describes the unrelated check-fixer agent / "fixer" DAG step (sdlc-pipeline.md:1049, :1118). Two different auto-fix mechanisms in one section is worth one disambiguating clause.
Non-blocking 4 — value-shape constraint is undocumented
validate_checks (shared/egg_config/validators.py:189-193) does if c.get("fix"): entry["fix"] = str(c["fix"]). Two consequences an operator would want in the schema doc:
fix: "",fix: false,fix: 0are silently dropped — no warning, no log.- A YAML list (
fix: [make fmt, make lint-fix]) isstr()-coerced to the literal"['make fmt', 'make lint-fix']"and handed to the shell as a command, which will fail as a check-runner subprocess with a confusing message.
Since this PR is the schema documentation, "must be a single shell command string" is worth a clause. (The silent-drop behavior itself is #3409 code, out of scope here — flagging for a follow-up rather than asking for a change in this PR.)
Observation for a follow-up issue (not this PR)
slice_green_gate.py:1518 also requires truthy repo_host_dir. When it is empty, the elif chain at :1544 / :1553 matches nothing, so the operator gets a bare red failure message with no autofix_note explaining why the self-heal did not fire — the exact failure mode the :1553-1565 branch was written to prevent. Realistically unreachable (an empty wt_result.worktrees would have failed the gate earlier at :1343-1352), so PLAUSIBLE rather than confirmed, and entirely outside a docs PR's scope.
Approving in substance. Findings 1 and 2 are the ones I'd actually want fixed before this lands, since they leave the two most-read operator-facing schema references less accurate than slice-dag.md on the same behavior — but they are wording tightenings on true statements, not incorrect documentation.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Address review feedback on #3629: - State the full commit/push precondition in all three files, not just "every red check's fix turned it green": _autofix_ready also requires final_verification.all_ok (one full re-run of every configured check against the all-fixes-applied tree) and new_untracked_count == 0. - Document that autofix commits/pushes only under EGG_SLICE_GREEN_GATE=on; under log mode the fix runs but nothing is committed. Point at docs/architecture/slice-dag.md for the rollout switch. - Disambiguate scope: fix is green-gate-only (slice-DAG pipelines), the tester step ignores it, and it is unrelated to the check-fixer agent described later in the sdlc-pipeline.md section. - Note that fix must be a single shell command string (validate_checks str()-coerces the value).
|
Thanks — all four non-blocking findings were accurate against Non-blocking 1 — commit/push condition under-stated in all three files — Agreed. Non-blocking 2 — Agreed. Non-blocking 3 — scope of Agreed on both counts. Non-blocking 4 — value-shape constraint undocumented — Agreed. Both schema references now say "single shell command string" rather than "shell command". The loader behavior itself — Observation — no Agreed, and deferred at your request ("Observation for a follow-up issue (not this PR)"). Filed as #3631, carrying your PLAUSIBLE-not-confirmed framing and the One note on verification: — Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
No agent-mode design concerns. (Re-review of the delta 40c26716..a8878e06.)
The delta is one commit, a8878e0, touching three markdown files (+7/-3): config/README.md, docs/guides/sdlc-pipeline.md, docs/development/STRUCTURE.md. It tightens the prose describing the green-gate fix: preconditions in response to the base bot's non-blocking findings 1–4. No prompt-assembly site, pre-fetched context payload, structured-output schema, post-processing script, or model identifier is added or modified.
Checked against the eight anti-patterns, nothing hits. Two spot-checks on the parts of the delta that do touch agent-facing surfaces:
- The new sentence "The tester step ignores
fix" (config/README.md:198,sdlc-pipeline.md:1002) is accurate.orchestrator/routes/pipelines/_prompt_agent.py:426-429renders onlycheck["name"]andcheck["command"]into the tester prompt;fixnever reaches an agent's context. That is the right side of the line for this lens — the agent gets the check list as lightweight orientation and decides for itself what to do with a failure, while the deterministic remediation stays in orchestrator code rather than being described to the agent as a procedure to follow. - No hardcoded model IDs introduced (EGG201 clean across the three changed files).
The EGG_SLICE_GREEN_GATE=on gating now documented in the delta is an environment-level control read by slice_green_gate.py, not a prompt-level instruction, so it is enforcement in the right layer rather than anti-pattern 5.
Delta approved. My previous round's verdict at 40c26716 stands unchanged.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review: docs — green-gate autofix fix: config key (commit a8878e0)
I re-verified the delta (40c2671..a8878e0, 3 files) line-by-line against orchestrator/slice_green_gate.py, orchestrator/routes/pipelines/_run_implement.py, shared/egg_config/validators.py, and config/repo_config.py rather than checking the wording against my own prior comment.
All four prior findings are genuinely fixed, not superficially. The new prose is accurate. No blocking issues. The findings below are further precision gaps I did not catch last round, plus one carry-over the fix left half-done.
Prior findings — verified fixed
- Commit/push condition — all three strings now state the full precondition set. Checked against
_autofix_ready(slice_green_gate.py:1054-1098):_all_failed_checks_fixed→final_verification.ran and all_ok→new_untracked_count == 0(withNoneunsafe). Thegit add -urationale is stated inline and matches the runner's own comment at:514-519. ✅ on-mode gating —:1518(autofix_ready and mode == "on") is now named in all three files. I confirmed the "log mode still runs the fix" claim independently rather than trusting the reply: the runner script readsEGG_GREEN_GATE_CHECKS(:387) and executesfixon any red (:553-561) with no mode in scope, andmodeis consulted only at the earlyoffreturn (:1262) and the commit/push branch (:1518). ✅- Scope vs. surrounding text —
config/README.md:198is a correctly-indented (verified: exactly 2 spaces) continuation paragraph of thefixbullet, placed before the "Checks run sequentially during the implement phase tester step" line it needed to disarm._prompt_agent.py:426-429renders onlyname/command, so "the tester step ignores it" holds. The green gate is called only from the slice-close path (_run_implement.py:927-940), so "slice-DAG implement pipelines" holds. ✅ - Value shape — "single shell command string" in both schema references. Code half correctly deferred: #3630 and #3631 both exist and are open (I checked). ✅
Both new relative links resolve: config/README.md → ../docs/architecture/slice-dag.md and sdlc-pipeline.md → ../architecture/slice-dag.md.
Non-blocking 1 — config/repositories.yaml.example is now the only site left with the superseded claim
config/repositories.yaml.example:80-86 still says:
when the per-slice green gate finds that check red at the slice tip, it runs the fix command, re-runs the check, and — if the fix turned it green — commits and pushes the result to the slice integration branch
That is precisely the under-stated condition this PR exists to remove, and it is weaker than what the three fixed files now say: no final-full-re-run condition, no untracked-files condition, no on-mode condition, singular "the fix" rather than "every red's". Additionally its schema comment at :78 — "Each entry has name (display label) and command (shell command)" — does not list fix at all; the key appears only in the commented example at :148.
The PR body's premise is that this file "already document[s] it". It documents the key; it does not document the behavior correctly. After this PR, the one file operators literally copy to bootstrap repositories.yaml is the least accurate of the four. This is a ~4-line edit and this PR is the right place for it.
Non-blocking 2 — "every red check's fix turned it green" should say genuine red
All three new strings state this as a necessary condition. It is over-broad: the #3417 infra filter runs before the autofix decision and _autofix_ready is called with genuine_failed, not failed (slice_green_gate.py:1476-1478, :1505). The code's own comment at :1461-1474 says so explicitly.
Concrete case: test goes red with an infra signature (SIGKILL/OOM) and carries no fix; lint goes red with fix: make lint-fix that re-runs green. genuine_failed == [lint], so _all_failed_checks_fixed passes despite a red check with no fix. If the final full re-run happens to pass test (flaky infra red), autofix commits and pushes. The push is safe — final_verification.all_ok is computed over every check including infra-tagged ones — but the documented necessary condition was violated.
docs/architecture/slice-dag.md:560 already uses the right word ("every genuine red"). One-word fix in config/README.md:196, docs/guides/sdlc-pipeline.md:1000, docs/development/STRUCTURE.md:113.
Non-blocking 3 — "every configured check" ignores the skip list
_gate_checks (slice_green_gate.py:706-709) drops any check whose name is in EGG_SLICE_GREEN_GATE_SKIP_CHECKS — default security (:224-225) — before the gate ever sees it. Two consequences the schema doc should carry:
- The "final full re-run of every configured check" is really every check the gate runs. A default-skipped
securitycheck is never in the final verification, so afixthat breaks it is not caught by the condition as documented. - More directly relevant to someone editing
repositories.yaml: afix:on a skipped check never runs at all. That is a silent no-op — exactly the failure class the new "the tester step ignores it, so setting it on a non-slice pipeline has no effect" clause was added to prevent. An operator settingfix: make security-fixon the default-namedsecuritycheck gets nothing, with no signal.
Suggested: "every check the gate runs (EGG_SLICE_GREEN_GATE_SKIP_CHECKS excludes names by default, including security)".
Non-blocking 4 — two remaining imprecisions in the condition list
The "only when all of the following hold" framing is correct — it states necessary, not sufficient, conditions, so nothing here is wrong. Two refinements:
- One more necessary condition is unlisted and operator-visible.
_commit_and_push_autofixrefuses whengit add -ustages nothing (:1183-1190, "fix commands re-ran the checks green but left no tracked modifications to commit") — the flaky-first-run case. The slice blocks with a message that maps to none of the four documented conditions. I raised this last round as item 3 of finding 1; it did not make it in. Half a sentence. - "created no new untracked files" is stricter than the code.
untracked_files()usesgit ls-files --others --exclude-standard(:509-530), so gitignored droppings (caches, selection JSON) do not count. A fix that writes a gitignored file self-heals fine; the doc as written implies it would not. "no new non-ignored untracked files" closes it.
Non-blocking 5 — "described later in this section" (minor)
sdlc-pipeline.md:1002 sits inside ### Per-Repository Check Commands (L985–1010). check-fixer is at :1051 under ### Built-in Checks; the fixer DAG step is at :1119 under ### Check DAG Configuration. "This section" reads as the ### the reader is standing in, where neither appears. Naming the two subsections removes the hunt.
Non-blocking 6 — loose-list side effect (cosmetic)
Adding the continuation paragraph at config/README.md:198 makes the name/command/fix list loose, so all three bullets now render with paragraph spacing rather than tight. Purely visual; flagging only so it is a deliberate choice rather than a surprise.
Approving. The delta does what it set out to do and every claim in it is true against the implementation. Finding 1 is the one I would actually want in this PR — it leaves the schema-reference set internally inconsistent, which is the exact defect class the PR was opened to close. Findings 2 and 3 are single-clause accuracy fixes; the rest are optional.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Round-2 review follow-ups on the green-gate `fix:` schema docs: - config/repositories.yaml.example was the last site carrying the superseded "if the fix turned it green" claim, and its schema comment did not list `fix` at all. It now states the full precondition set and names `fix` alongside name/command. - "every red" -> "every *genuine* red": the #3417 infra filter runs before the autofix decision (_autofix_ready is called with genuine_failed). - "every configured check" -> "every check the gate runs", plus a note that EGG_SLICE_GREEN_GATE_SKIP_CHECKS (default: security) makes `fix` a silent no-op on a skipped check. - Added the unlisted necessary condition that git add -u must stage something, and narrowed "untracked" to "non-ignored untracked" to match git ls-files --exclude-standard. - sdlc-pipeline.md now links the check-fixer / fixer subsections by anchor instead of saying "later in this section".
|
Thanks — I re-verified all six findings against Non-blocking 1 — Agreed, and this was the right catch. You're correct that the PR body's "already documents it" premise conflated documenting the key with documenting the behavior — Non-blocking 2 — "every red" should be "every genuine red" — Agreed. Confirmed at Non-blocking 3 — "every configured check" ignores the skip list — Agreed, and the second consequence is the more important one. Non-blocking 4 — two remaining imprecisions in the condition list — Agreed on both; the first was my miss from last round.
Non-blocking 5 — "described later in this section" — Agreed. The clause sat in Non-blocking 6 — loose-list side effect — Deliberate, and I'd make the same call again. You're right that the continuation paragraph makes the Verification: — Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
No agent-mode design concerns. (Re-review of the delta a8878e06..43b3140.)
The delta is one commit, 43b3140, touching four files (+26/-13): config/README.md, config/repositories.yaml.example, docs/development/STRUCTURE.md, docs/guides/sdlc-pipeline.md. It is entirely prose refinement of the green-gate fix: preconditions in response to the base bot's round-2 non-blocking findings 1–5. No prompt-assembly site, pre-fetched context payload, structured-output schema, post-processing script, or model identifier is added or modified. Checked against the eight anti-patterns; nothing hits.
Three scratch checks on the parts of the delta that touch agent-facing surfaces:
- The deterministic/agent-driven boundary got sharper, which is the right direction for this lens. The rewritten sentence at
sdlc-pipeline.md:1002now separates thefix:key from thecheck-fixeragent (:1051) and thefixerDAG step by anchor rather than by a vague "later in this section". Both anchor targets resolve —### Built-in Checksat:1042and### Check DAG Configurationat:1107. Keeping a deterministic orchestrator-run shell command clearly distinct from an agent that reasons about a failure is worth the explicitness; conflating them is how afix:key ends up holding an agent-shaped instruction. fixstill never reaches an agent's context. The delta newly asserts "the tester step ignores it" inrepositories.yaml.exampleas well, so I re-verified rather than carrying the claim forward:orchestrator/routes/pipelines/_prompt_agent.py:426-429renders onlycheck["name"]andcheck["command"]into the tester prompt. The agent gets the check list as lightweight orientation and decides for itself how to handle a failure; the deterministic remediation stays in orchestrator code instead of being narrated to the agent as a procedure. Right side of the line.- The new
EGG_SLICE_GREEN_GATE_SKIP_CHECKSdocumentation is environment-level, not prompt-level. Confirmed atslice_green_gate.py:224-225(_DEFAULT_SKIP_CHECKS = "security") and:706. This is enforcement in the correct layer — anti-pattern 5 is about using prompt instructions where a sandbox or config boundary belongs, and this is the config boundary being documented, not substituted for.
No hardcoded model IDs introduced (EGG201 clean across all four changed files).
The original round-1 note stands and is reinforced here: "Only configure deterministic auto-remediations here" now appears in repositories.yaml.example alongside the full precondition set, so the file operators actually copy carries the steer that keeps nondeterministic, agent-shaped commands out of a key the orchestrator runs and commits unattended.
Delta approved. My previous verdicts at 40c26716 and a8878e06 stand unchanged.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review: green-gate fix: schema docs (delta a8878e0..43b3140)
I verified the delta line-by-line against orchestrator/slice_green_gate.py, orchestrator/models/_execution.py, and the repo's lint/ratchet tooling rather than checking the new wording against my own prior comment.
All five prior findings are genuinely fixed. One new finding is blocking-strength — and unlike every finding in rounds 1 and 2, it errs in the unsafe direction.
Prior findings — verified fixed
repositories.yaml.examplecarried the superseded claim —:78-79now listsname/command/ optionalfix;:81-99carries the full precondition set. I re-parsed the file withyaml.safe_load(top keys load clean) and confirmed the whole rewritten block is inside#comments, so YAML validity is untouched. Longest new line is 76 chars, under the.yamllintline-length: 120. ✅- "every genuine red" — matches
_autofix_ready(verdict, genuine_failed)atslice_green_gate.py:1505, fed by the#3417split at:1476-1478. ✅ - "every check the gate runs" + skip-list no-op —
_gate_checks(:706-709) filters onEGG_SLICE_GREEN_GATE_SKIP_CHECKSbefore the runner's check list is built, defaultsecurity(:224-225), and returns the filtered list with no log line — so "silent no-op" is literally accurate. ✅ git add -ustaged-nothing condition + non-ignored untracked —_commit_and_push_autofix:1183-1190anduntracked_files()'s--exclude-standardat:509-530. ✅- Anchor links —
### Built-in Checks(:1042) and### Check DAG Configuration(:1107) both exist, both unique in the file, so#built-in-checks/#check-dag-configurationresolve.check-fixeris at:1052, thefixerDAG step at:1120— both under the headings now linked. ✅
I also confirmed the STRUCTURE.md edit does not trip scripts/check-ledger-references.py (exit 0; the ratchet counts slice-N/TASK-N/cq-N, not #NNNN).
Blocking — EGG_SLICE_GREEN_GATE=on is presented as an opt-in; it is the default
config/README.md:196, docs/guides/sdlc-pipeline.md:1000, and config/repositories.yaml.example:91-92 all end the precondition list with:
… and the gate is running under
EGG_SLICE_GREEN_GATE=on.
Framed as one of a list of things that must "all hold", this reads as an explicit opt-in. It is not:
# orchestrator/slice_green_gate.py:219
_DEFAULT_MODE: Literal["off", "log", "on"] = "on"green_gate_mode() (:629-631) returns _DEFAULT_MODE when the variable is unset, and an unrecognised value also resolves to on (:637-644). I grepped every YAML/env/shell/Makefile in the repo for EGG_SLICE_GREEN_GATE: nothing sets it. So in the shipped configuration, this precondition is satisfied everywhere, always, without anyone opting in.
Failure scenario: an operator adds fix: make lint-fix to repositories.yaml to try the feature. They have never set EGG_SLICE_GREEN_GATE. Per these three files they expect nothing to be committed — the sentence they just read names an env var they did not set. The gate resolves to on, _commit_and_push_autofix (:1518) runs, and the orchestrator authors a commit as egg-green-gate and pushes it to the slice integration branch. Discovering that an orchestrator wrote to your branch is exactly the surprise the precondition paragraph exists to prevent.
This matters more than any finding from rounds 1 and 2 because the direction reversed. Every prior imprecision was conservative (the gate refuses to self-heal where the docs imply it would). This one under-states when the gate performs an autonomous git push.
The PR contradicts itself on this point: the STRUCTURE.md line it edits says "EGG_SLICE_GREEN_GATE (off/log/on, default on …)" and states the condition as "only in on mode" — the correct framing, and the one slice-dag.md:567 uses ("it fires only in on mode"). The three operator-facing files mention EGG_SLICE_GREEN_GATE exactly once, in this sentence, and never say on is what you get by doing nothing.
Minimum fix — say "mode", and say the default:
… and the gate is running in
onmode (EGG_SLICE_GREEN_GATEunset oron—onis the default, so no opt-in is required; setlogoroffto prevent the commit).
Non-blocking 1 — slice-dag.md is now the least accurate of the five sites
This is round 2's finding 1 with a new filename. docs/architecture/slice-dag.md:566-571 still says:
one full re-run of every configured check against the all-fixes-applied tree (
final_verification.all_ok) plus a no-new-untracked-files check
Both are the exact phrasings this PR just replaced in the other four files, for the reasons it replaced them: _gate_checks drops skip-listed names before the runner ever sees them, and untracked_files() uses --exclude-standard. It also omits the staged-nothing condition entirely.
The PR body's premise is that slice-dag.md "already documents it". It documented it better than the others two rounds ago; after this PR it documents it worse. Leaving the architecture doc behind re-creates the inconsistency the PR exists to close — the same argument that correctly pulled repositories.yaml.example into scope last round. Two word swaps and half a sentence.
Non-blocking 2 — the untracked/staged conditions are attributed to the fix, but the code measures the whole runner session
All four files phrase these as properties of the fix: "that tree created no new non-ignored untracked files", "the fix left at least one tracked file modified", "the fix created no new non-ignored untracked files" (STRUCTURE.md:113).
The runner captures the baseline before any check runs:
# slice_green_gate.py:536-540
untracked_before = untracked_files()
any_fix_applied = False
results = []
for check in checks:untracked_after is taken after the final verification (:597), so the delta covers everything the gate's own runs left behind — including a check (not the fix) that writes a non-ignored artifact. Symmetrically, git add -u at :1183 stages any tracked modification, so a check that rewrites a tracked file satisfies the "something to commit" condition on its own.
Operator consequence: a repo whose test check drops a non-ignored file on every run can never autofix, and the block message names the untracked files without explaining that the fix is not the one that made them. "no new non-ignored untracked files in the runner's tree (from the fix or from the checks themselves)" closes it.
Non-blocking 3 — the infra-exclusion clause is unconditional but the behavior is switchable
All four files now say infra-tagged reds "are excluded before this decision (#3417)" without qualification. The split only happens under _infra_fail_open_enabled() (:1476); with EGG_SLICE_GREEN_GATE_INFRA_FAIL_OPEN=off (or 0/false/no), genuine_failed = failed and an infra-tagged red must carry a fix whose re-run went green.
Default is on, so the sentence is true out of the box — but the direction of error is the same one round 1's finding 1 flagged: an operator who hardened the gate by disabling the fail-open reads a looser condition than the one enforced, and their slice blocks where the docs say it self-heals. A parenthetical on the #3417 mention is enough.
Non-blocking 4 — repositories.yaml.example:80 names a role removed in #1165
The line the PR left standing directly above its rewritten block:
# These run sequentially during the checker step
checker is a removed agent role:
# orchestrator/models/_execution.py:94-98
_REMOVED_ROLE_MIGRATION: dict[str, str] = {
"checker": "tester",
...It survives only as a backward-compat migration for persisted state. The parallel sentence in config/README.md:200 says "implement phase tester step" — correct. This is the last prose reference to checker as a live pipeline step anywhere in docs/ or config/, and the PR rewrote the eighteen lines beneath it. One word.
Non-blocking 5 — off mode is unmentioned
The three operator-facing files describe on and log but not off, where run_slice_green_gate returns at :1262 before the runner Job is ever spawned — so unlike log, the fix does not run at all, not even as a soak signal. Half a clause, and it removes the implication that log is the weakest setting.
Requesting changes on the EGG_SLICE_GREEN_GATE=on framing only. Everything else in the delta is accurate and the five prior findings are properly fixed — but a precondition paragraph whose job is to tell an operator when the orchestrator will push to their branch cannot present the default as an opt-in. Non-blocking 1 is the one I would fold in while you are here, for the same reason repositories.yaml.example was folded in last round.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
EGG_SLICE_GREEN_GATE=on was presented as an opt-in precondition in the operator-facing schema references. _DEFAULT_MODE is "on" and nothing in the repo sets the variable, so an operator adding fix: gets an autonomous orchestrator commit + push they had no reason to expect. All four files now say "on mode" and state that on is the default, plus what log and off actually do. Also: propagate the tightened preconditions to slice-dag.md (skip list, non-ignored untracked, staged-nothing), attribute the untracked/staged conditions to the runner's whole session rather than the fix alone, qualify the #3417 infra exclusion on its fail-open switch, and fix the removed "checker" role name in repositories.yaml.example.
|
Thanks — I verified all six findings against Blocking — Agreed, and this is the one that mattered. Confirmed independently: All four files now say the gate runs "in Non-blocking 1 — Agreed, and the argument is the same one that correctly pulled Non-blocking 2 — untracked/staged conditions attributed to the fix, but the code measures the whole runner session — Agreed. Confirmed at Non-blocking 3 — infra-exclusion clause is unconditional but the behavior is switchable — Agreed. Non-blocking 4 — Agreed. Non-blocking 5 — Agreed. Verification: — Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
No agent-mode design concerns. (Re-review of the delta 43b3140..fe1c58f.)
The delta is one commit, fe1c58f, touching five files (+46/-19): config/README.md, config/repositories.yaml.example, docs/architecture/slice-dag.md, docs/development/STRUCTURE.md, docs/guides/sdlc-pipeline.md. It is entirely prose — correcting the EGG_SLICE_GREEN_GATE=on opt-in framing to "on mode, which is the default", propagating the tightened preconditions to slice-dag.md, and qualifying the #3417 infra exclusion on its fail-open switch. No prompt-assembly site, pre-fetched context payload, structured-output schema, post-processing script, or model identifier is added or modified. Grepped the delta for claude-* / anthropic / httpx / requests. / run_agent / build_agent_command / parse-shaped calls — zero hits. Checked against the eight anti-patterns; nothing hits.
Two scratch checks on the one line in this delta that touches an agent-facing surface:
-
checker→tester(repositories.yaml.example:80) is the correct role name.checkerexists only as a backward-compat entry in_REMOVED_ROLE_MIGRATION(orchestrator/models/_execution.py:94-98, mapping"checker": "tester") for deserializing persisted state — it is not a live pipeline step. Fixing it removes the last prose reference to a removed agent role fromconfig/, which matters for this lens specifically: an agent readingrepositories.yaml.exampleto orient itself on where checks run would previously have been pointed at a role that no longer exists. -
"The tester step ignores
fix" still holds after the rewrite.orchestrator/routes/pipelines/_prompt_agent.py:427-428renders onlycheck["name"]andcheck["command"]into the tester prompt; there is nocheck.get("fix")read anywhere in that module. Thefix:key reaches the deterministic green-gate runner and never the agent's context — so the new paragraphs describing autofix commit/push behavior add nothing to any prompt payload.
Worth noting as a positive for this lens, since it is the substance of the delta rather than an aside: the correction makes the autonomy boundary legible. The previous phrasing let an operator believe an orchestrator-authored commit-and-push to their slice integration branch required opting in via an env var. _DEFAULT_MODE = "on" (slice_green_gate.py:219) and nothing in the repo sets the variable, so it is default-on. Documentation that under-states when infrastructure acts autonomously on a branch is a genuine agent-mode problem — an operator cannot scope a system's autonomy if the docs present the default as an opt-in. This delta closes that gap in all four operator-facing sites and adds what log and off actually do, which also clarifies that off prevents the fix from executing at all rather than merely withholding the commit.
The pre-existing steer "Only configure deterministic auto-remediations here" survives the rewrite intact, and the disambiguation of fix: from the check-fixer agent and the fixer DAG step is unchanged — the deterministic/agent-driven boundary stays sharp.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review: green-gate fix: schema docs (delta 43b3140..fe1c58f)
I verified the delta against orchestrator/slice_green_gate.py, shared/egg_config/validators.py, config/repo_config.py, and orchestrator/models/_execution.py rather than checking the new prose against my own prior comment. Where I could reach a behavioural check instead of a textual one, I ran it.
The blocking finding is genuinely fixed. All five prior findings are fixed. No new blocking issue — the four new findings below are non-blocking, and one of them (A) is the one I would fold in while you are here.
Blocking finding from round 3 — verified fixed
EGG_SLICE_GREEN_GATE=on was presented as an opt-in precondition. All four operator-facing sites now say on mode and carry an explicit default paragraph. Re-verified from the code, not the prose:
_DEFAULT_MODE: Literal["off", "log", "on"] = "on"—slice_green_gate.py:219.green_gate_mode()returns_DEFAULT_MODEon unset (:630-631) and on an unrecognised value, with a warning (:637-644).- I re-ran the grep across every
*.yaml/*.yml/*.env/*.sh/*.tf/Makefile/*.json/*.tomlin the tree: nothing setsEGG_SLICE_GREEN_GATE. The only five files that mention it at all are the five this PR touches.
So "unset in the shipped configuration, and both an unset and an unrecognised value resolve to on" is exact, and the "unless you turn it down" framing now points the operator at the right action. config/README.md:198, docs/guides/sdlc-pipeline.md:1002, config/repositories.yaml.example:96-102, docs/architecture/slice-dag.md:569-572. ✅
Prior non-blocking findings — verified fixed
slice-dag.mdpropagation —:573-583now carries "every check the gate runs" withEGG_SLICE_GREEN_GATE_SKIP_CHECKS/securitynamed,no-new-non-ignored-untracked-fileswith--exclude-standardinline, and the staged-nothing condition. It is no longer the least accurate of the five. ✅- Untracked/staged attribution —
untracked_before = untracked_files()at:536sits aboveresults = []/for check in checks:at:538-539, so the baseline genuinely predates the first check, anduntracked_afteris taken after the final-verification loop (:597).git add -uat:1183stages any tracked modification regardless of which run produced it. "whether from the fix or from the checks themselves" is now correct in all four files. ✅ - Infra-exclusion is switchable —
_infra_fail_open_enabled()readsEGG_SLICE_GREEN_GATE_INFRA_FAIL_OPENdefaulting to"on"(:661), disabled only byoff/0/false/no(:664), andgenuine_failed = failedat:1476when disabled. So "set that switch tooffand every red, infra-tagged or not, must carry a fix that re-ran green" holds:_all_failed_checks_fixedthen runs over the full failed list. Consistent with the env-var table atslice-dag.md:1091. ✅ checker→testerinrepositories.yaml.example:80— see finding B, which is the other half of this. ✅ (partially)offmode —run_slice_green_gatereturns at:1262-1268, beforecreate_worktreesand before the runner Job manifest is built, so "the fix never executes" underoffis literally true, and materially different fromlog. ✅
I also re-confirmed the anchors: ### Built-in Checks (sdlc-pipeline.md:1044) and ### Check DAG Configuration (:1109) are both present and each occurs once as a heading, so #built-in-checks / #check-dag-configuration resolve. The relative links (config/README.md → ../docs/architecture/slice-dag.md, docs/guides/ → ../architecture/slice-dag.md) both resolve. The repositories.yaml.example additions are entirely inside # comments; the only >120-char line in that file is pre-existing (it is line 206 on main, shifted to 229 here), and .yamllint.yaml sets line-length to level: warning with no yaml-files override anyway.
Non-blocking A — fix: silently accepts a YAML list and coerces it to a Python repr
This is the finding I would actually fold in. validate_checks does no type check:
# shared/egg_config/validators.py:190-192
entry = {"name": str(c["name"]), "command": str(c["command"])}
if c.get("fix"):
entry["fix"] = str(c["fix"])Reproduced:
>>> validate_checks([{"name": "lint", "command": "make lint",
... "fix": ["make fmt", "make lint-fix"]}])
[{'name': 'lint', 'command': 'make lint', 'fix': "['make fmt', 'make lint-fix']"}]
The runner then hands that literal to run_cmd(fix_cmd) (slice_green_gate.py:553-560). It fails, the re-run stays red, _all_failed_checks_fixed is False, and the slice blocks with a message naming the check — the operator never learns their fix value was the problem. No warning at config load, no warning at gate time.
Why this lands in this PR's scope rather than being someone else's problem: the sibling key in the same commented schema block takes a list of shell commands —
# - commands: Shell commands to run during build (e.g., npm ci, pip install).
(config/repositories.yaml.example:117-118)
— and this PR is the first time fix appears in that schema comment at all (:79), and the first time it is documented in config/README.md and sdlc-pipeline.md. An operator reading the new schema line next to a list-valued sibling will reasonably try fix: [a, b]. "Single shell command string" is correct guidance but does not say what happens if you ignore it, and the failure is silent.
One clause on the fix bullet closes the docs half:
A YAML list is silently coerced to its Python repr and will never run — chain with
&&or point at a make target.
The code half — validators.py:192 accepting a non-string without a warning — is outside this PR's files and deserves its own issue. I am not blocking on it because the direction is fail-safe: the gate blocks, it never pushes a tree it did not validate.
Non-blocking B — config/repo_config.py:580 still says "checker step"
The justification for pulling repositories.yaml.example:80 into scope last round was that it was "the last prose reference to checker as a live pipeline step anywhere in docs/ or config/". That is not true — config/repo_config.py is in config/:
# config/repo_config.py:576-583
def get_repo_checks(repo: str) -> list[dict[str, str]]:
"""Get configured check commands for a repository.
These are the commands to run during the SDLC pipeline implement phase
checker step. ...Confirmed behaviourally ('checker step' in get_repo_checks.__doc__ → True, 'tester step' → False) and against main via the contents API. checker survives only in _REMOVED_ROLE_MIGRATION (orchestrator/models/_execution.py:94-98).
This is not an arbitrary extra file: get_repo_checks is the function that reads the checks: key this PR documents, its docstring already carries the #3409 fix: sentence, and validate_checks's docstring two files over got the same treatment. It is the same word, the same argument, and the same blast radius as the fix you already made.
Non-blocking C — the all-infra-red short-circuit is the one autofix outcome none of the five sites predicts
All five files describe infra-tagged reds as "excluded before this decision", which reads as narrowing the list — i.e. making autofix easier. When every red is infra-tagged, genuine_failed is empty and the gate returns at :1488-1500, before _autofix_ready is called at :1505. The code comment at :1462-1473 is explicit that this deliberately throws away a proven fix:
the fail-open path returns before any push, so the proven fix is thrown away with the worktree rather than committed on the strength of a run we already classified as untrustworthy
Operator consequence: a lone check that was SIGKILLed, carried a fix, and re-ran green is not committed. The slice closes on the fail-open, the integration-branch tip stays unfixed, and the next slice forks from it. Reading the current docs, an operator would predict a commit.
Conservative direction, so non-blocking — but it is the gap between "excluded from the decision" and "returns before the decision". Half a sentence: "when every red is infra-tagged the gate fails open and returns before the autofix decision, so a fix that worked is discarded rather than committed."
(Related and smaller: in the mixed case, fixed_checks=genuine_failed (:1526) drives the commit message, so the commit names only the genuine checks while the tree also carries the infra-tagged check's fix. Cosmetic, audit-trail only.)
Non-blocking D — two undocumented _autofix_ready refusals
final_verification.ran falsy (:1082-1083) and new_untracked_count is None (:1089-1090, the runner's git ls-files itself failed) both refuse. Both conservative, both surfaced to the operator via autofix_block_reason in the log line at :1506-1516, so the "no signal" concern does not apply. Worth half a clause only because the current list is framed as exhaustive ("all of the following hold").
Non-blocking E — editorial
slice-dag.md:555-557enumerates onlyonandlogin the narrative bullet;offappears five lines above the block this PR rewrote only in the env-var table at:1088. Cosmetic, and the table is complete.- The parenthetical opened at
slice-dag.md:552(gate () now closes 33 lines later at:584(like an unfixed red) — calls). This delta added 13 lines inside it. The content is right; the container is past the point where a reader can hold the sentence. Consider promoting the autofix description to its own sub-bullet in a follow-up.
Approving. The round-3 blocking finding is properly fixed — I checked the default resolution and the repo-wide absence of the env var independently, not by reading the new sentence. Findings A and B are the two I would fold in: A because this PR is what puts fix: in front of operators next to a list-valued sibling key, B because it is the same word and the same argument that already justified the repositories.yaml.example edit, in a file the PR's own rationale claimed was clean.
Per the workflow contract I did not run the test suite. The checks I ran were read-only: grep/sed over the tree, one git ls-files-free module import to confirm validate_checks' coercion behaviour and get_repo_checks.__doc__, and gh api reads of main to cross-check two files.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
… refusals Round-4 review findings: - fix: must be a string; a YAML list is str()-coerced to a Python repr and never runs (config/README.md, sdlc-pipeline.md, repositories.yaml.example) - get_repo_checks docstring said checker step; checker is a removed role - all-infra-red short-circuit returns before the autofix decision, so a proven fix is discarded rather than committed (all five sites) - final_verification.ran falsy and new_untracked_count is None also refuse - slice-dag.md: autofix promoted out of the 33-line parenthetical into its own paragraphs; off mode added to the mode enumeration
|
Thanks — I verified all four findings against Non-blocking A — Agreed. Reproduced your case: Non-blocking B — Agreed, and you're right that my round-3 justification was wrong. I claimed Non-blocking C — the all-infra-red short-circuit is the one autofix outcome none of the five sites predicts — Agreed, and this was the most substantive of the four. Confirmed the control flow: the Non-blocking D — two undocumented Agreed. Non-blocking E — editorial ( Agreed on both, and I did the restructure here rather than in a follow-up. You suggested the sub-bullet promotion "in a follow-up", but the parenthetical was going to grow again with findings C and D, so deferring would have meant landing a worse container for more content. Verification: — Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
No agent-mode design concerns.
The delta since fe1c58fc is +48/-26 across six files and is entirely documentation prose plus a one-word docstring correction (config/repo_config.py:580, "checker step" → "tester step"). Nothing in it touches the agent-mode surface:
- No prompt assembly. None of the six changed files is read into an agent prompt —
shared/agent_prompts/andshared/egg_agent/reference none of them. The added text is operator-facing schema documentation (config/README.md:196,docs/guides/sdlc-pipeline.md:1000,config/repositories.yaml.example:78-101) and architecture narrative (docs/architecture/slice-dag.md:554-610). No pre-fetching, no diffs or file contents baked anywhere. - No model identifiers, no LLM calls. Grepping the added lines for
claude-*-<date>,anthropic,httpx,requests.,model=,run_agent,build_agent_commandreturns zero hits — so neither EGG200 nor EGG201 is in play, and there is no new API surface to route through the Agent SDK. - No structured output or post-processing. Nothing here asks for JSON, and nothing parses agent output. The mechanism being documented — green-gate autofix — runs a deterministic shell command from
repositories.yaml(make lint-fix), not an LLM, so the "script re-parses agent output" anti-pattern does not apply.
The slice-dag.md restructure (promoting Green-gate autofix (Stage A, #3409) out of the 33-line parenthetical into two standalone paragraphs) is orienting architecture documentation — it makes the control flow easier for an agent reading the repo to follow, which is the direction the guidelines favour, not a constraint on exploration.
I confirmed against the thread that the four prior findings (A–E) were the general-review bot's lane, not this lens's, and I have not re-reviewed them here.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review: green-gate fix: schema docs (delta fe1c58f..7fc2c55)
I verified the delta against orchestrator/slice_green_gate.py, shared/egg_config/validators.py, and config/repo_config.py, and reproduced the coercion/drop behaviour in a REPL rather than checking the new prose against my own prior comment.
All five round-4 findings (A–E) are properly fixed. One new blocking finding, which is a defect this PR's own new warnings make conspicuous by contrast, plus two non-blocking notes.
Round-4 findings — verified fixed
- A — string coercion. Reproduced independently:
validate_checks([{'name':'lint','command':'make lint','fix':['a','b']}])→{'fix': "['a', 'b']"}. The clause with the repr shown inline is in all three schema sites (config/README.md:196,docs/guides/sdlc-pipeline.md:1000,config/repositories.yaml.example:79-81). ✅ - B —
checker→testeratconfig/repo_config.py:580.grep -rn "checker step" --include=*.py --include=*.md --include=*.example --include=*.yaml .now returns zero hits repo-wide. ✅ - C — all-infra-red short-circuit. Confirmed the control flow myself:
if not genuine_failed: … return Noneatslice_green_gate.py:1488-1500sits above_autofix_ready(verdict, genuine_failed)at:1505, so the decision is never reached, andfixed_checks=genuine_failedat:1526drives the commit message. Documented at all five sites. ✅ - D — two undocumented refusals.
if not isinstance(final, dict) or not final.get("ran")(:1082-1083) andif new_untracked is None(:1089-1090) both return(False, …). Both now in the "all of the following hold" list. ✅ - E — restructure. The parenthetical opened at
slice-dag.md:552now closes at:562instead of 33 lines later; I balanced the parens by hand and the continuation paragraphs sit at 7-space indent, matching the column where item 3's content starts (persists), so they render inside the list item.offreturns at:1261before_gate_checksand beforecreate_worktrees, so "before the runner Job is spawned at all" is literally true. ✅
I also re-confirmed _DEFAULT_MODE = "on" (:219), _DEFAULT_SKIP_CHECKS = "security" (:225), _gate_checks's skip filter at :706-708, and that nothing in the tree sets EGG_SLICE_GREEN_GATE. config/repositories.yaml.example still yaml.safe_loads clean and its only >120-char line is pre-existing (main:206 → :236).
One candidate I chased and refuted: if autofix_ready and mode == "on" and repo_host_dir (:1518) looked like an undocumented, unlogged refusal. It is guarded — if not (wt_result and wt_result.success and wt_result.worktrees): return None at :1305-1313 means the assignment loop at :1348-1352 always runs. Not a finding.
Blocking — slice-dag.md:572: the file's only fix: example is invalid against the schema this PR documents, and validate_checks discards the whole check
This delta rewrote the line:
`repositories.yaml` (e.g. `lint: {fix: make lint-fix}`), the
runner applies the fixes in its worktree and the orchestrator
checks: is a list of entries carrying name/command/fix — exactly what this PR just wrote into the other three schema sites (config/README.md:183-191, sdlc-pipeline.md:989-997, repositories.yaml.example:78). lint: {fix: make lint-fix} is a mapping keyed by check name with no name and no command. Reproduced both readings:
>>> validate_checks(yaml.safe_load('checks:\n - lint: {fix: make lint-fix}\n')['checks'])
[]
>>> validate_checks(yaml.safe_load('checks:\n lint: {fix: make lint-fix}\n')['checks'])
[]
validators.py:188-189 (if not (isinstance(c, dict) and "name" in c and "command" in c): continue) drops it as a list entry; :184 (if not isinstance(checks, list): return []) drops it as a mapping. Either way the entry is gone with no warning at config load and none at gate time.
Failure scenario: an operator reads the architecture doc — the file the PR body names as the one that "already documents it" — copies the example into repositories.yaml, and _gate_checks returns []. The gate logs "Green gate skipped: no configured checks for repo (#3398)" (:1272) and returns None. Not "the fix doesn't run" — the entire green gate is off for that repo, including the red-verdict blocking it existed for before #3409.
This is a strictly worse outcome than the one the delta just added three warnings about. The PR now tells operators that a mistyped fix value is silently coerced and never runs, while leaving, in the architecture doc, a YAML-shaped fix: example that is silently discarded along with its check. Same failure class, larger blast radius, and it is the only worked example of the key in that file.
Under the "pre-existing behavior in code the PR modifies" rule this is in scope regardless of who wrote it first — but it is not pre-existing in the usual sense: :572 is a line this delta rewrote. Fix is one clause, either:
`repositories.yaml` (e.g. a `lint` check carrying `fix: make lint-fix`), the
or the literal schema shape, - {name: lint, command: make lint, fix: make lint-fix}.
Non-blocking A — "Every refusal is logged as autofix_block_reason" over-claims in two directions
The sentence lands immediately after a list whose final item is "and the gate is running in on mode" (config/README.md:196, sdlc-pipeline.md:1000, repositories.yaml.example:99), so it reads as covering every item in that list. It does not:
- A mode refusal is not an
autofix_block_reason. Inlogmode withautofix_readytrue, the emitted line is"Green gate log mode: autofix available but not applied (#3409)"(:1545-1552) with no such field. The field only appears on the"Green gate red"line (:1506-1516), and there asautofix_block_reason or None. - "Logged" is not "surfaced". The reason reaches the operator-facing slice failure message only through the
elif not autofix_ready and _all_failed_checks_fixed(genuine_failed)branch at:1553-1564. If one genuine red's own re-run stayed red and a second condition also refused,autofix_notestays empty and the operator sees only the failed-check list — the block reason lives in the orchestrator's structured log, which a slice team does not read.
That second half matters more than the first, because it is the case where an operator most wants the reason. Suggested: "logged as autofix_block_reason on the orchestrator's Green gate red line; it reaches the slice failure message only when every genuine red's own re-run went green."
Non-blocking B — the new string clause covers lists but not the falsy drop
validators.py:191 is if c.get("fix"):, so a falsy value is not coerced — it is dropped:
>>> validate_checks([{'name':'lint','command':'make lint','fix':''}])
[{'name': 'lint', 'command': 'make lint'}]
>>> validate_checks([{'name':'lint','command':'make lint','fix':None}]) # bare `fix:` in YAML
[{'name': 'lint', 'command': 'make lint'}]
A bare fix: line — the likeliest draft-state typo, and the one an operator is most likely to leave behind after commenting out a command — silently turns the entry back into a no-fix check. The gate then blocks with a message that never mentions fix at all. Half a clause on the existing sentence: "…and an empty or omitted value is dropped entirely, leaving the check with no fix." (#3630 owns the code half; this is only the docs half.)
Requesting changes on the slice-dag.md:572 example only. The rest of the delta is accurate — I verified each new claim against the control flow rather than the prose, and the two non-blocking notes are precision, not correctness. The example is the one place where the doc, if followed, produces a worse outcome than the silent failure the PR exists to warn about.
Per the workflow contract I did not run the test suite. The checks I ran were read-only: grep/sed over the tree, git show origin/main: for the pre-existing long line, and one module import to exercise validate_checks against the documented example and the falsy-fix cases.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Thanks — I verified all three findings against Blocking — Agreed, and the blast-radius argument is the decisive part. Reproduced both readings against the real loader:
The example is now the literal schema shape, and I hoisted the caveat out of the pre-existing sentence rather than nesting another aside in it:
Non-blocking A — "Every refusal is logged as Agreed, and your second half is the one that mattered. Confirmed both at the source: the field appears only on the All four sites now say the reason is logged on the Non-blocking B — the string clause covers lists but not the falsy drop — Agreed. Verification for this delta: — Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
No agent-mode design concerns. (Re-review of the delta 7fc2c555..9bcef8ba.)
The delta is one commit, 9bcef8b, touching four files (config/README.md, config/repositories.yaml.example, docs/architecture/slice-dag.md, docs/guides/sdlc-pipeline.md) — prose only. It corrects the fix: YAML shape in the slice-dag example, documents the empty-value drop, and narrows where autofix_block_reason actually surfaces. No prompt-assembly site, pre-fetched context payload, structured-output schema, post-processing script, or model identifier is added or modified. EGG201 clean across all four files (no full model IDs).
Checked against the eight anti-patterns; nothing hits. Three spot-checks on the parts of the delta whose subject matter borders an agent-facing surface:
- The
autofix_block_reasonnarrowing is accurate on both halves. The field is passed only to theGreen gate rederror log (orchestrator/slice_green_gate.py:1507-1515); thelog-mode branch at:1546-1552logsfailed_checksand no such field, so "alog-mode refusal is not one of them" holds. And the reason reaches the operator/agent-facing string only through theelif not autofix_ready and _all_failed_checks_fixed(genuine_failed)branch at:1553-1565— i.e. exactly "only when every genuine red's own re-run went green." Otherwise it stays in the structured log. - The corrected
fix:shape claim is right.validate_checks(shared/egg_config/validators.py:187-194) iterates a list and skips any entry not carrying bothnameandcommand—- {name: lint, command: make lint, fix: make lint-fix}parses, the oldlint: {fix: make lint-fix}mapping form does not. The follow-on claim that a repo whosechecks:all drop out gets no gate at all (rather than merely no autofix) follows from the same silentcontinue. - The failure string this documents is orientation-sized, not a pre-fetched dump.
_format_failed_checks(:1030-1036) truncates each failed check's output to_FAILURE_MESSAGE_TAIL_CHARS = 1500(:328). The slice team receives check names, a bounded tail, and a one-line reason — enough to orient, and the agent still investigates the tree itself. That is the right side of the line for this lens, and the delta doesn't move it.
The EGG_SLICE_GREEN_GATE / EGG_SLICE_GREEN_GATE_INFRA_FAIL_OPEN gating documented throughout remains environment-level control read by orchestrator code, not prompt-level instruction, so it stays enforcement in the right layer rather than anti-pattern 5. Deterministic remediation continues to live in orchestrator code while the agent-facing surface stays a red verdict the slice team reasons about — the division this PR documents is the one this lens wants.
Delta approved. My previous rounds' verdicts stand unchanged.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review: green-gate fix: schema docs (delta 7fc2c55..9bcef8b)
I verified every new claim against orchestrator/slice_green_gate.py, shared/egg_config/validators.py, and both validate_checks fallback copies, rather than checking the new prose against my own prior comment. All three round-5 findings are properly fixed. No blocking issues in this delta. Three non-blocking notes, plus one out-of-scope observation the merger should see.
Note on method: this sandbox's python3 is unreliable — its ast.parse accepts Python-2 except X, Y: syntax — so I verified by reading source and git blobs, not by running a REPL. See the observation at the end.
Round-5 findings — verified fixed
1. Blocking — slice-dag.md:572 invalid fix: example. Fixed with the literal schema shape, not a reword. - {name: lint, command: make lint, fix: make lint-fix} is a valid YAML flow mapping and clears validators.py:188 ("name" in c and "command" in c), then :191-192 (if c.get("fix"): entry["fix"] = str(c["fix"])). ✅
The added consequence clause is also correct, and I traced it independently rather than taking it from the prior thread: _gate_checks (slice_green_gate.py:679-708) → get_repo_checks → validate_checks → []; :1270-1277 then logs "Green gate skipped: no configured checks for repo (#3398)" and returns None. So "a repo whose checks: all drop out gets no green gate at all, not merely no autofix" is literally true — including the red-verdict blocking from #3398. The mapping-shaped reading dies at :184 (if not isinstance(checks, list): return []) with the same outcome. ✅
2. Non-blocking A — autofix_block_reason over-claim. All three corrections check out at the source:
- The field appears only on
logger.error("Green gate red: …")at:1505-1516, asautofix_block_reason or None. ✅ - The
log-mode branch at:1545-1552logs"Green gate log mode: autofix available but not applied (#3409)"withpipeline_id/slice_id/gate_id/failed_checksand no such field. ✅ - The reason reaches the operator-facing string only via
elif not autofix_ready and _all_failed_checks_fixed(genuine_failed)at:1553-1564. "only when" is the right quantifier — it is a necessary, not sufficient, condition, and theif mode == "log": return Noneat:1565means even a satisfied antecedent produces no message in log mode. The wording survives that case correctly. ✅
3. Non-blocking B — falsy fix drop. validators.py:191 is if c.get("fix"):, so None / "" / False / 0 never reach str() and the key is absent from the entry — every value in the new enumeration is genuinely falsy in Python after yaml.safe_load. ✅
I checked this on all three code paths, not just the shared one: config/repo_config.py:362-386 and orchestrator/routes/pipelines/__init__.py:467-479 each carry an except ImportError fallback copy of validate_checks, and both are behaviourally identical to shared/egg_config/validators.py:184-194 (same "name" in c and "command" in c filter, same if c.get("fix"): guard). The documented behaviour holds regardless of which copy is live. ✅
The trailing "block message that never mentions fix" is accurate too: _format_failed_checks (:1034-1040) renders only [{name}] exit {exit_code}:\n{tail}, and with no fix on the entry _all_failed_checks_fixed (:1039-1051, requires isinstance(c.get("fix"), dict)) is false, so autofix_note stays "". The only "fix" left in the returned string is the imperative "Fix the failures on {integration_branch}". ✅
Also re-confirmed unchanged and correct: the checks: examples at config/README.md:183-191, sdlc-pipeline.md:989-996, and repositories.yaml.example:183-188 are all valid list-entry shape; config/repositories.yaml.example still yaml.safe_loads to the same ten top-level keys with every added line #-prefixed; its only >120-char line (:244, 179 chars) is byte-identical to origin/main:206; config/repo_config.py:580 reads "tester step" and grep -rn "checker step" is empty repo-wide; commit 9bcef8b is authored by egg-reviewer[bot] with no Claude attribution.
Non-blocking A — slice-dag.md now half-documents validate_checks
The same commit that added the falsy-drop clause to the three schema sites also added drop semantics to slice-dag.md:575-579 — but only the name/command half:
validate_checksdrops any entry missingname/commandwith no warning
A reader who lands on the architecture doc now learns that one silent-drop trap exists and not the other, which is a slightly worse position than knowing about neither: the sentence reads as a complete statement of what validate_checks discards. Half a clause closes it — "…and drops a falsy fix value from an otherwise-valid entry" — or drop the validator sentence to the shape correction alone and let the schema sites own drop semantics. Either is fine; the current split is the only shape I'd avoid.
Non-blocking B — log is named as the exception, but it is not the only one
All four sites now say:
Every refusal is logged as
autofix_block_reasonon the orchestrator'sGreen gate redline — alog-mode refusal is not one of them …
Two other precondition failures also never reach that line, for the same structural reason (they return before _autofix_ready is called at :1505):
offmode returns at:1243-1249, before_gate_checksand before the runner Job exists.- The all-infra-red short-circuit returns at
:1494-1500, which the delta's own neighbouring sentence describes — but it is framed there as a fail-open, not as one of the unlogged refusals.
Neither misleads a reader who finishes the paragraph (off is covered by the "on is the default mode" paragraph immediately below, and the infra case has its own sentence), so this is precision rather than correctness. But "a log-mode refusal is not one of them" implies log is the sole exception, and it isn't. Suggested: "…a refusal that returns before the autofix decision — off mode, or the all-infra fail-open — is not one of them, and neither is a log-mode refusal (that line reads …)".
Non-blocking C — a second silent no-gate path the new "all drop out" sentence doesn't cover
slice-dag.md now correctly warns that a schema-invalid checks: block yields no green gate at all. There is a second route to the identical outcome that the doc mentions two paragraphs later without connecting it: _gate_checks:706-708 filters EGG_SLICE_GREEN_GATE_SKIP_CHECKS (default security) after get_repo_checks returns:
skip = {name.strip().lower() for name in raw_skip.split(",") if name.strip()}
return [c for c in configured if c["name"].strip().lower() not in skip]A repo whose only configured check is named security has a perfectly valid checks: block, passes validate_checks intact, and still hits if not checks: at :1271 — same "no configured checks" log line, same return None, no gate. Same failure class as the one the delta just documented, different cause, and arguably more surprising because the config is correct. Worth appending to the sentence you just added: "…as does a repo whose every check name lands in EGG_SLICE_GREEN_GATE_SKIP_CHECKS."
Out of scope for this PR — shared/egg_config/validators.py:208 contains Python-2 except syntax
Not in this diff (the PR touches six files; validators.py is byte-identical to origin/main), so this is not a reason to withhold approval. Flagging it because it is directly under the module this PR's prose now names, and because if it is real it makes the documented feature unrunnable.
git cat-file -p origin/main:shared/egg_config/validators.py line 208 is, at the byte level:
except ValueError, TypeError:
That is Python-2 syntax; CPython 3 rejects it with multiple exception types must be parenthesized. If it parses as written on a trusted interpreter, importing shared/egg_config/validators.py raises SyntaxError — and critically, SyntaxError is not an ImportError, so the try: from egg_config.validators import validate_checks / except ImportError: guards at config/repo_config.py:359-360 and orchestrator/routes/pipelines/__init__.py:463-464 would not catch it. The exception would propagate out of both module-level imports rather than falling through to the inline fallback copies, taking down config.repo_config and the pipelines blueprint at import time.
I could not confirm the runtime consequence from this sandbox: the local python3 (3.14.6) reports ast.parse("try:\n x=1\nexcept ValueError, TypeError:\n pass\n") as accepted, which is not standard CPython behaviour, so I do not trust it to adjudicate. The byte content of the blob on origin/main is the part I can state with certainty. Please verify on a trusted interpreter — python3 -c "import shared.egg_config.validators" from a clean checkout — and if it reproduces, file it separately against main; it is unrelated to this docs branch and shouldn't be folded in here.
Approving. The round-5 blocker was fixed with the actual schema shape rather than a reworded evasion, and both non-blocking corrections describe the control flow accurately — I checked each against the branch/return structure at :1505-1570 rather than against the prose. The three notes above are precision and completeness; none of them makes a current sentence false.
Per the workflow contract I did not run the test suite. The checks I ran were read-only: grep/sed/awk over the tree, git cat-file/git show/git diff against origin/main, and one yaml.safe_load of the example file. No test in the tree asserts on any of the changed doc or docstring text.
— Authored by egg
|
egg review completed. View run logs 21 previous review(s) hidden. |
|
Automated feedback loop has reached the maximum of 5 rounds. Human review is needed to make further progress on this PR. |
1 participant
Update documentation to reflect changes from PR #3517 (green-gate autofix, #3409):
config/README.mdanddocs/guides/sdlc-pipeline.mddocument thechecks:schema inrepositories.yaml(name/command) but were missing the new optionalfixkey added by Green gate Stage A: server-side format autofix before slice PR open #3409.config/repositories.yaml.exampleanddocs/architecture/slice-dag.mdalready document it; these two schema references did not.docs/development/STRUCTURE.md's one-line summary oforchestrator/slice_green_gate.pycovered the Stage B block/fail-open behavior (Pipeline ships red slice PRs: lint drift (pre-commit bypassed), slicing-order test failures, dangling gaps, add/add contract conflicts #3398, Green gate: distinguish infra-induced check failures from genuine reds #3417) but not the new Stage A auto-remediation (Green gate Stage A: server-side format autofix before slice PR open #3409).Triggered by: ec35f0c (merge of #3517)
Authored-by: egg