Reconcile docs with reality; fix module graph, token compare, health
Acts on the 2026-07-30 senior review (REVIEW.md findings 1, 2, 4, 5, 7).
Docs (finding 1): CLAUDE.md and AGENTS.md both claimed Design B "has zero
clients - no worker binary exists". cmd/orchestra-worker/main.go is the
deployed worker, and the non-local-herdr guardrail has landed in
Coordinator.adapterFor. Both sections rewritten; AUDIT.md gains a matching
federation-status record. The Phase 5 retention / Phase 6 deletion decision
for Design A is preserved, not flattened.
clients/ un-ignored and tracked, including the .service unit and README:
deployed code belongs in version control. Design A is NOT deleted here.
progress.md (finding 2): the file was deleted after 636ed8a, yet CLAUDE.md
instructed every session to cross-check against it. References removed from
CLAUDE.md, AGENTS.md, internal/orchestrator/rotation_test.go (comment only)
and deploy/hooks/orchestra-codex-poll.sh; AUDIT.md now carries the log role.
web/go.mod (finding 4): a module stub ends the parent package graph at the
directory boundary, so go list ./... no longer yields
web/node_modules/flatted/golang/pkg/flatted. A build tag cannot work - the
package is in the package list before tags are evaluated. Local/CI-only
breakage: Dockerfile.api builds ./cmd/orchestra by explicit path and
.dockerignore already excluded node_modules.
orchestra-worker (finding 5): untracked (8.9MB, mode 100755, still on disk);
both binaries now gitignored.
Token compare (finding 7): cmd/orchestra/main.go:139,582 use
subtle.ConstantTimeCompare, matching the authz.go idiom. The token != ""
guard stays first, so an empty configured token still means auth-disabled
rather than auth-bypass. Three further plain != secret compares remain in
internal/federation/federation.go:343,346,368 - tracked, not fixed here.
Also included from the review pass: orchestrator.go records adapter-resolution
failures in SessionHealth.LastError instead of dropping them on a bare
continue, plus an Observed flag so lease-seeded health is not mistaken for a
live reading, with a covering test. GET /v1/tasks/<id>/health now returns a
record with last_error where it previously returned a bare 404.
REVIEW.md's own second pass claimed every checkable fact held up; four did
not. AUDIT.md never contained the false Design B claim (AGENTS.md was the
second copy), the guardrail is at orchestrator.go:312 not :309, the
progress.md site list missed the codex-poll hook, and only orchestra-worker
was tracked. Verified: go build, go vet, go test, and
go list ./... | grep node_modules all clean with every change applied
together. No live herdr or pane was touched; nothing was deployed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GEugbHVYfAXFpTqDYbByEB
This commit is contained in:
@@ -287,6 +287,11 @@ type SessionHealth struct {
|
||||
// silently treated as "not time to rotate yet" by a bare continue.
|
||||
Occupancy float64 `json:"occupancy,omitempty"`
|
||||
OccupancyError string `json:"occupancy_error,omitempty"`
|
||||
// Observed distinguishes a health entry this coordinator actually read
|
||||
// from a live adapter this pass from one it could not reach (remote-owned
|
||||
// session, unresolvable adapter) or has merely seeded at lease time. A
|
||||
// consumer must not read Status as current unless Observed is true.
|
||||
Observed bool `json:"observed"`
|
||||
}
|
||||
|
||||
// adapterFor resolves the herdr adapter for a session. Session.HerdrID (the
|
||||
@@ -356,9 +361,25 @@ func (c *Coordinator) refreshSessionHealth(ctx context.Context) {
|
||||
for taskID, session := range sessions {
|
||||
a, err := c.adapterFor(taskID, session)
|
||||
if err != nil {
|
||||
// Adapter resolution failing is the normal case for a session
|
||||
// owned by a remote worker's herdr, and the abnormal case for a
|
||||
// misregistered local one. Either way the previous entry — often
|
||||
// the Status:"running" rememberSession writes at lease time — must
|
||||
// not be left standing as if it were freshly observed, or
|
||||
// /v1/tasks/<id>/health reports a dead pane as running forever.
|
||||
// Record the resolution failure so it is observable, per the
|
||||
// contract SessionHealth documents.
|
||||
c.healthMu.Lock()
|
||||
prev := c.health.Sessions[taskID]
|
||||
prev.LastError = err.Error()
|
||||
prev.UpdatedAt = time.Now().UTC()
|
||||
prev.Observed = false
|
||||
c.health.Sessions[taskID] = prev
|
||||
c.healthMu.Unlock()
|
||||
continue
|
||||
}
|
||||
var h SessionHealth
|
||||
h.Observed = true
|
||||
h.UpdatedAt = time.Now().UTC()
|
||||
if occ, occErr := a.Occupancy(session); occErr != nil {
|
||||
h.OccupancyError = occErr.Error()
|
||||
@@ -993,6 +1014,8 @@ func (c *Coordinator) Start(ctx context.Context, e domain.Event) error {
|
||||
if c.health.Sessions == nil {
|
||||
c.health.Sessions = map[string]SessionHealth{}
|
||||
}
|
||||
// Seeded at lease time, not observed from the harness: Observed stays
|
||||
// false until refreshSessionHealth reads a live adapter.
|
||||
c.health.Sessions[t.ID] = SessionHealth{Status: "running", UpdatedAt: time.Now().UTC()}
|
||||
c.healthMu.Unlock()
|
||||
return err
|
||||
|
||||
@@ -174,7 +174,7 @@ func TestCoordinatorRefusesRemoteHerdrOperations(t *testing.T) {
|
||||
}
|
||||
|
||||
// TestRotationEmitsValidReleaseWithAnchorSHA guards the highest-priority spec
|
||||
// defect noted in progress.md: automated rotation must emit a TaskReleased
|
||||
// defect noted in AUDIT.md: automated rotation must emit a TaskReleased
|
||||
// event that satisfies domain.ValidatePayload (handoff_ref + anchor_sha), not
|
||||
// a payload missing anchor_sha that silently fails to append.
|
||||
func TestRotationEmitsValidReleaseWithAnchorSHA(t *testing.T) {
|
||||
@@ -1041,3 +1041,55 @@ func TestConventionsDriftNotifiesActiveSession(t *testing.T) {
|
||||
t.Fatal("session was never notified of the conventions-doc update")
|
||||
}
|
||||
}
|
||||
|
||||
// TestUnresolvableAdapterRecordsObservableSessionHealth pins the fix for the
|
||||
// recurring silent-continue pattern this repo keeps regrowing (CLAUDE.md,
|
||||
// AUDIT.md P1 "Observability"): refreshSessionHealth used to `continue` on an
|
||||
// adapter-resolution failure, discarding the reason entirely. The operator
|
||||
// endpoint /v1/tasks/<id>/health then 404s with no way to tell "no such task"
|
||||
// from "this coordinator cannot see the harness that owns it", and any health
|
||||
// already seeded at lease time is left standing as if freshly observed.
|
||||
func TestUnresolvableAdapterRecordsObservableSessionHealth(t *testing.T) {
|
||||
s, err := store.Open(t.TempDir())
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if err := s.Append(domain.Event{ID: domain.NewID(), Type: "TaskCreated", TaskID: "handover", Surface: string(authz.System), Payload: mustJSON(map[string]any{
|
||||
"source": "qa", "external_id": "handover", "project": "p",
|
||||
})}); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
lease, err := s.Lease("handover", "local", time.Minute)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
statePath := t.TempDir() + "/sessions.json"
|
||||
a := &fakeAdapter{}
|
||||
owner := &orchestrator.Coordinator{Store: s, Worktrees: worktrees{path: t.TempDir()}, Adapters: adapters{a}, StatePath: statePath,
|
||||
LocalHerdr: func(id string) bool { return id == "local" }}
|
||||
if err := owner.Start(context.Background(), lease); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if h, ok := owner.MonitorHealth().Sessions["handover"]; !ok || h.Observed {
|
||||
t.Fatalf("lease-time seeded health = %+v, present=%v; want present and Observed=false", h, ok)
|
||||
}
|
||||
|
||||
// Same durable session mapping, but this coordinator no longer owns that
|
||||
// herdr — the live federated case, and the case where a registry change
|
||||
// leaves a persisted HerdrID unresolvable.
|
||||
foreign := &orchestrator.Coordinator{Store: s, Worktrees: worktrees{path: t.TempDir()}, Adapters: adapters{a}, StatePath: statePath,
|
||||
LocalHerdr: func(id string) bool { return false }}
|
||||
if err := foreign.Reconcile(context.Background()); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
h, ok := foreign.MonitorHealth().Sessions["handover"]
|
||||
if !ok {
|
||||
t.Fatal("unresolvable session recorded no health at all; the resolution failure was swallowed")
|
||||
}
|
||||
if h.Observed {
|
||||
t.Fatalf("health = %+v; want Observed=false for a session this coordinator cannot read", h)
|
||||
}
|
||||
if h.LastError == "" {
|
||||
t.Fatalf("health = %+v; want the adapter-resolution error recorded in LastError", h)
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user