Skip to content

Commit 7f2b679

Browse files
fix(orchestrator): withdraw the verdict change, keep the failure shape
The verdict change did not fix the case it was built for, and a wiring test is what showed it. Reverted: both-halves-green no longer licenses reporting success_with_failures over an escalation, and no exit code moves. Traced properly, the measured run was refused by "(tester/QA rejected — plan/tasks rewritten)", not "(escalated tasks need human review)". A tester had found an unresolved defect. That is a different and stronger refusal, it is correct, and it must keep winning — so the change was inert on the only case that motivated it, and would have altered exit codes on the strength of a scenario never actually observed. What is kept is the part that was always safe: a FAILING teams run now names the halves that nonetheless proved themselves against their own acceptance commands. "Everything is broken" and "both halves compile and pass their own tests, and what failed is the join between them" call for completely different next moves, and the summary said only the first. An UNVERIFIED half is never named — a command that could not run said nothing about that half. The test drives the real finalize path with a per-command gate, so the team gates are the ones the run actually computes rather than injected ones. The first two versions of it passed for the wrong reason: one had a green objective gate that licensed the walk-past by the PRE-EXISTING rule, and would have passed with the change reverted.
1 parent 3ebf7a1 commit 7f2b679

5 files changed

Lines changed: 190 additions & 127 deletions

File tree

‎docs/changelog.md‎

Lines changed: 25 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -8,16 +8,9 @@ showed did not mean what it looked like.
88

99
### Behaviour changes
1010

11-
No flag, config key or API shape is removed. What changes is what a run REPORTS,
12-
and two of these move an exit code.
13-
14-
- **A teams run whose halves are all green now reports
15-
`success_with_failures` instead of `failure` when a task escalated.**
16-
`Result.Success` drives the exit code, so a run that exited non-zero for this
17-
can now exit zero. `FailedTasks` still carries the count, the outcome is never
18-
plain `success`, and a warning names both the evidence and the seam it does
19-
not cover. An UNVERIFIED half never counts, so a run with a half that was
20-
never proved still fails flat.
11+
No flag, config key or API shape is removed, and no exit code moves. What
12+
changes is what a run REPORTS.
13+
2114
- **A team gate reports UNVERIFIED, not RED, when the project defines no such
2215
check** — `npm error Missing script: "build"` no longer fails a team. A gate
2316
that used to be red is now grey, and reds are no longer raised as tickets for
@@ -180,34 +173,28 @@ and two of these move an exit code.
180173

181174
### Changed — what a teams run reports
182175

183-
- **Both halves proving themselves is now a license to report
184-
`success_with_failures` instead of flat failure.** Measured live: two
185-
implementation tasks done, `team backend-go is green`, `team frontend-react is
186-
green`, and the run reported **failure** because a seam *tester* escalated
187-
after a local reviewer scored its prose 0. The software was built and each
188-
half was proved by its own command; the user was told it failed.
189-
190-
The escalation refusal already had one exception — the objective gate measured
191-
green — on the stated principle that when a planner's guess at a decomposition
192-
and a measurement disagree, the measurement wins. Every team proving its own
193-
half is a measurement of exactly that kind: an exit status on the real tree,
194-
not a model's opinion of a write-up. It was simply being thrown away.
195-
196-
This is **not** a relaxation of the refusal:
197-
198-
- the escalated task stays escalated on the board, and `FailedTasks` keeps the
199-
count;
200-
- the outcome is never plain `success` — `success_with_failures` exists to be
201-
told apart from a clean board, and the summary names the escalation;
202-
- an **UNVERIFIED** gate is never counted. A command that could not run said
203-
nothing about that half, and rounding it up here would undo that
204-
distinction at the one place it decides the headline verdict;
205-
- fewer than two proved halves proves nothing — one team doing all the work is
206-
not a teams run.
207-
208-
A warning says so explicitly, naming the halves that were proved and the gap
209-
that remains: those commands do not check the **seam**, which is the thing a
210-
seam task existed to verify.
176+
- **A failing teams run now says which halves nonetheless proved themselves.**
177+
The verdict is untouched: the run failed, the exit code stands, and whatever
178+
refused it did so for a reason. What a person needs and did not get is the
179+
SHAPE of the failure — "everything is broken" and "both halves compile and
180+
pass their own tests, and what failed is the join between them" call for
181+
completely different next moves, and the summary said only the first.
182+
Measured live: `team backend-go is green`, `team frontend-react is green`,
183+
two implementation tasks done, reported as a flat failure indistinguishable
184+
from a run where nothing worked at all.
185+
186+
An UNVERIFIED half is never named as proved — a command that could not run
187+
said nothing about that half, which is the whole reason UNVERIFIED is
188+
separate from RED.
189+
190+
*A stronger version of this was written and withdrawn.* It let both-halves-
191+
green license reporting `success_with_failures` instead of `failure` when a
192+
task had escalated, by analogy with the objective gate's existing licence.
193+
A wiring test showed it did not fix the case it was built for: that run was
194+
refused by a **tester rejection**, which is a different and stronger refusal
195+
— the tester found an unresolved defect — and one that should keep winning.
196+
Changing the exit code on the strength of a scenario never actually observed
197+
was not worth the risk, so only the reporting was kept.
211198

212199
### Known limitation — the seam task
213200

‎docs/squads.md‎

Lines changed: 13 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -501,29 +501,24 @@ nothing:
501501
The result is on the board, per team: **proved green**, **half is red**, or
502502
**unverified** with the reason.
503503
504-
### Both halves green changes the run's verdict
504+
### A failing run says which halves were proved
505505
506-
A task that escalates normally fails the whole run — the escalation is exactly
507-
the signal a human should look. There is one long-standing exception: the
508-
objective command measured green, on the principle that when a planner's guess
509-
at a decomposition and a measurement disagree, the measurement wins.
510-
511-
Every team proving its own half is a measurement of that kind. So a run whose
512-
halves are all green reports `success_with_failures` rather than `failure`, and
513-
says why:
506+
A task that escalates fails the whole run — the escalation is exactly the signal
507+
a human should look at. That does not change. What does is the SHAPE of the
508+
failure being visible:
514509
515510
```text
516-
verify reporting success_with_failures over 1 task(s) needing human review:
517-
every half proved itself (backend-go, frontend-react), which is a
518-
measured exit status rather than a planner's guess — but the SEAM
519-
between them is not what those commands check, so look at the
520-
escalation before trusting the join
511+
verify the run failed, but 2 half/halves proved themselves against their own
512+
acceptance command (backend-go, frontend-react) — those halves compile
513+
and pass what they declare, so start at what failed BETWEEN them rather
514+
than inside them
521515
```
522516

523-
Nothing is swallowed. The task stays escalated, the count stays on the result,
524-
the outcome is never plain `success`, and an **unverified** half never counts —
525-
a command that could not run is not evidence, which is the whole reason
526-
UNVERIFIED is separate from RED.
517+
"Everything is broken" and "both halves are sound and the join is not" call for
518+
completely different next moves, and the summary said only the first. An
519+
**unverified** half is never named here: a command that could not run said
520+
nothing about that half, which is the whole reason UNVERIFIED is separate from
521+
RED.
527522

528523
---
529524

‎pkg/orchestrator/halves_test.go‎

Lines changed: 129 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1,28 +1,55 @@
11
package orchestrator
22

33
import (
4+
"context"
5+
"strings"
6+
"sync"
47
"testing"
8+
"time"
59

10+
"github.com/UnicoLab/slmcode/pkg/config"
11+
"github.com/UnicoLab/slmcode/pkg/plan"
12+
"github.com/UnicoLab/slmcode/pkg/quality"
613
"github.com/UnicoLab/slmcode/pkg/squads"
714
)
815

9-
func gatedOrchestrator(t *testing.T, gates ...TeamGate) *Orchestrator {
16+
// selectiveGate answers each command on its own merits, which is the only way
17+
// to build the shape under test: both TEAM commands green while the run as a
18+
// whole fails.
19+
type selectiveGate struct{ green map[string]bool }
20+
21+
func (g *selectiveGate) run(_ context.Context, _, cmd string, _ time.Duration) quality.SmokeResult {
22+
ok := g.green[cmd]
23+
sr := quality.SmokeResult{Ran: true, Command: cmd, OK: ok}
24+
if ok {
25+
sr.Summary = quality.SmokePassedMarker + ": " + cmd
26+
} else {
27+
sr.Summary = quality.SmokeFailedMarker
28+
}
29+
return sr
30+
}
31+
32+
func halvesPlan() *squads.Plan {
33+
return &squads.Plan{Squads: []squads.Squad{
34+
{ID: "backend-go", Owns: []string{"cmd/**"}, Acceptance: "go build ./..."},
35+
{ID: "frontend-react", Owns: []string{"web/**"}, Acceptance: "npm run build"},
36+
}}
37+
}
38+
39+
func gatedPlanOrchestrator(t *testing.T, gates ...TeamGate) *Orchestrator {
1040
t.Helper()
11-
o := &Orchestrator{squadPlan: &squads.Plan{Squads: []squads.Squad{
12-
{ID: "backend-go"}, {ID: "frontend-react"},
13-
}}}
41+
o := &Orchestrator{squadPlan: halvesPlan()}
1442
for _, g := range gates {
1543
o.teamGates.set(g)
1644
}
1745
return o
1846
}
1947

20-
// The measured case: two tasks done, both halves green by their own commands,
21-
// and the run reported flat FAILURE because a seam tester escalated after a
22-
// local reviewer scored its prose 0. The software was built and each half was
23-
// proved; the user was told it failed.
48+
// Two halves, each proved by running its own acceptance command, is measured
49+
// evidence: an exit status on the real tree rather than a model's opinion of a
50+
// write-up.
2451
func TestBothHalvesGreenIsMeasuredEvidence(t *testing.T) {
25-
o := gatedOrchestrator(t,
52+
o := gatedPlanOrchestrator(t,
2653
TeamGate{Team: "backend-go", Ran: true, OK: true},
2754
TeamGate{Team: "frontend-react", Ran: true, OK: true})
2855

@@ -35,9 +62,8 @@ func TestBothHalvesGreenIsMeasuredEvidence(t *testing.T) {
3562
}
3663

3764
// An UNVERIFIED gate is the case this must never round up. A command that could
38-
// not run said nothing about that half, and the whole point of separating
39-
// UNVERIFIED from RED is that it is not evidence in either direction —
40-
// counting it here would undo that at the one place it decides the verdict.
65+
// not run said NOTHING about that half — that is the entire reason UNVERIFIED
66+
// is separate from RED — so it is not evidence in either direction.
4167
func TestAnUnverifiedHalfIsNotEvidence(t *testing.T) {
4268
for name, gates := range map[string][]TeamGate{
4369
"one half never ran its check": {
@@ -52,30 +78,107 @@ func TestAnUnverifiedHalfIsNotEvidence(t *testing.T) {
5278
{Team: "backend-go", Ran: false},
5379
{Team: "frontend-react", Ran: false},
5480
},
81+
"only one half reported at all": {
82+
{Team: "backend-go", Ran: true, OK: true},
83+
},
5584
} {
56-
if gatedOrchestrator(t, gates...).allHalvesProved() {
57-
t.Errorf("%s: was counted as proof", name)
85+
if gatedPlanOrchestrator(t, gates...).allHalvesProved() {
86+
t.Errorf("%s: was counted as proof of both halves", name)
5887
}
5988
}
6089
}
6190

62-
// "Both halves proved" needs both halves to exist and be proved. One team doing
63-
// all the work is not a teams run, and a single green gate must not license
64-
// walking past an escalation.
65-
func TestOneProvedHalfIsNotBothHalves(t *testing.T) {
66-
o := gatedOrchestrator(t, TeamGate{Team: "backend-go", Ran: true, OK: true})
67-
if o.allHalvesProved() {
68-
t.Error("a single green half licensed an escalation walk-past")
69-
}
70-
}
71-
7291
func TestNoSquadPlanProvesNothing(t *testing.T) {
73-
o := &Orchestrator{}
74-
if o.allHalvesProved() {
92+
if (&Orchestrator{}).allHalvesProved() {
7593
t.Error("a run without teams claimed both halves were proved")
7694
}
7795
var nilO *Orchestrator
7896
if nilO.allHalvesProved() {
7997
t.Error("a nil orchestrator claimed proof")
8098
}
8199
}
100+
101+
// escalatedTeamsBoard is the measured shape: both teams' implementation tasks
102+
// done, and a seam TESTER escalated to the human backlog.
103+
func escalatedTeamsBoard() *plan.Board {
104+
b := doneBoard()
105+
b.Tasks[0].Squad = "backend-go"
106+
b.Tasks = append(b.Tasks,
107+
plan.Task{ID: "T2", Title: "render the todos", Column: plan.ColDone,
108+
Role: plan.RoleWorker, Squad: "frontend-react"},
109+
plan.Task{ID: "T3", Title: "verify the halves meet", Column: plan.ColToScope,
110+
Role: plan.RoleTester, Error: "review rejected after max retries"})
111+
return b
112+
}
113+
114+
// A failing teams run must say which halves were nonetheless PROVED.
115+
//
116+
// The verdict is untouched — the run failed and the exit code stands. What a
117+
// person needs is the SHAPE of the failure: "everything is broken" and "both
118+
// halves compile and pass their own tests, and what failed is the join between
119+
// them" call for completely different next moves, and the summary said only the
120+
// first. Measured live: `team backend-go is green`, `team frontend-react is
121+
// green`, reported as a flat failure indistinguishable from a run where nothing
122+
// worked at all.
123+
func TestAFailingRunSaysWhichHalvesWereProved(t *testing.T) {
124+
// Both halves' own commands pass; everything else — including the QA
125+
// objective — does not. That is the measured shape.
126+
gate := &selectiveGate{green: map[string]bool{
127+
"go build ./...": true, "npm run build": true,
128+
}}
129+
o := objectiveOrch(t, gate, &countingExec{},
130+
func(c *config.Config) { c.QAGate = true })
131+
sink := newMsgSink(o)
132+
o.squadPlan = halvesPlan()
133+
134+
res := finalize(t, o, escalatedTeamsBoard())
135+
136+
if res.Success {
137+
t.Fatal("the verdict changed — this must only ADD information to a failure")
138+
}
139+
joined := sink.text()
140+
if !strings.Contains(joined, "backend-go") || !strings.Contains(joined, "frontend-react") {
141+
t.Errorf("a failing run did not name the halves that proved themselves:\n%s", joined)
142+
}
143+
if !strings.Contains(joined, "BETWEEN them") {
144+
t.Errorf("the shape of the failure was not stated:\n%s", joined)
145+
}
146+
}
147+
148+
// Nothing claimed when nothing proved itself.
149+
func TestNoHalvesProvedSaysNothing(t *testing.T) {
150+
// Nothing green anywhere, so no half can claim to have proved itself.
151+
o := objectiveOrch(t, &selectiveGate{green: map[string]bool{}}, &countingExec{}, nil)
152+
sink := newMsgSink(o)
153+
o.squadPlan = halvesPlan()
154+
155+
finalize(t, o, escalatedTeamsBoard())
156+
157+
if joined := sink.text(); strings.Contains(joined, "proved themselves") {
158+
t.Errorf("claimed a half proved itself when none did:\n%s", joined)
159+
}
160+
}
161+
162+
// msgSink collects emitted messages. The orchestrator fires events from several
163+
// goroutines, so this needs a lock — without one the race detector fails the
164+
// test rather than the code.
165+
type msgSink struct {
166+
mu sync.Mutex
167+
msgs []string
168+
}
169+
170+
func newMsgSink(o *Orchestrator) *msgSink {
171+
s := &msgSink{}
172+
o.onEvent = func(e Event) {
173+
s.mu.Lock()
174+
defer s.mu.Unlock()
175+
s.msgs = append(s.msgs, e.Message)
176+
}
177+
return s
178+
}
179+
180+
func (s *msgSink) text() string {
181+
s.mu.Lock()
182+
defer s.mu.Unlock()
183+
return strings.Join(s.msgs, "\n")
184+
}

0 commit comments

Comments
 (0)