From 9d0d55aa4407b3608c7d4da71f0d0c75d019b521 Mon Sep 17 00:00:00 2001 From: Rolando Santamaria Maso <4096860+jkyberneees@users.noreply.github.com> Date: Wed, 2 Sep 2026 21:37:58 +0200 Subject: [PATCH 1/4] fix(subagents): route deliverables through the artifact channel Sub-agent responses were commonly truncated: the parent tool description did not document the two-channel result model, the child-side artifact note was too weak to act on, and cut headlines were invisible. The artifact protocol existed (M1-M3) but nothing steered the LLMs into it. - Description(): two-channel result-delivery contract (headline ~2000 chars + artifacts, artifact_read loop) + guidance schema nudge (A/A2) - childArtifactNote(): loss-fact framing, small-task exemption, flat files only with stated consequence, headline shape (B); identity closing line aligned (B2) - visible truncation marker: summary_truncated/summary_runes (omitempty, wire-compatible) + conditional artifact_read hint gated on artifactReadEnabled (C) - artifact ids: first-wins with probe-increment .t aliases that never evict live entries; task provenance on artifact lines and artifact_read output (D1) - 128 KiB per-call inline budget (largest-first) + per-run registry floor so a collating call's artifacts stay resolvable (F) - gitignore .odek-artifacts/ staging (E) RED-first: subagent_delivery_test.go (14 tests) + flipped dup-id pin. go vet clean; scoped -race and full cmd/odek package green. Docs: docs/SUBAGENTS.md envelope + artifact sections synced. --- .gitignore | 3 + cmd/odek/artifact_read_test.go | 14 +- cmd/odek/artifact_read_tool.go | 4 +- cmd/odek/main.go | 22 +- cmd/odek/subagent.go | 79 +++-- cmd/odek/subagent_artifact_registry.go | 148 ++++++++-- cmd/odek/subagent_delivery_test.go | 388 +++++++++++++++++++++++++ cmd/odek/subagent_tool.go | 128 ++++++-- docs/SUBAGENTS.md | 21 +- 9 files changed, 726 insertions(+), 81 deletions(-) create mode 100644 cmd/odek/subagent_delivery_test.go diff --git a/.gitignore b/.gitignore index 2166318..1fd2947 100644 --- a/.gitignore +++ b/.gitignore @@ -28,3 +28,6 @@ SUB_AGENTS_IMPROVEMENTS.md # opencode agent local memory .kai/ .gotmp/ + +# Sub-agent artifact staging (delegate_tasks result channel) +.odek-artifacts/ diff --git a/cmd/odek/artifact_read_test.go b/cmd/odek/artifact_read_test.go index 25a357c..58b0dbc 100644 --- a/cmd/odek/artifact_read_test.go +++ b/cmd/odek/artifact_read_test.go @@ -54,19 +54,25 @@ func TestArtifactRegistry_RegisterLookupEvict(t *testing.T) { } } -func TestArtifactRegistry_DuplicateIDLastWins(t *testing.T) { +func TestArtifactRegistry_DuplicateIDFirstWins(t *testing.T) { resetArtifactRegistryForTest() r1, p1 := regRef(t, "dup", "first") registerSubagentArtifact(artifactEntry{Ref: r1, Path: p1, TaskIdx: 0}) r2, p2 := regRef(t, "dup", "second") - dup := registerSubagentArtifact(artifactEntry{Ref: r2, Path: p2, TaskIdx: 1}) + alias, dup := registerSubagentArtifact(artifactEntry{Ref: r2, Path: p2, TaskIdx: 1}) if !dup { t.Error("duplicate registration must be reported") } got, _ := lookupSubagentArtifact("dup") - if got.Path != p2 || got.TaskIdx != 1 { - t.Errorf("last-wins broken: %+v", got) + if got.Path != p1 || got.TaskIdx != 0 { + t.Errorf("first-wins broken: %+v", got) + } + if alias != "dup.t2" { + t.Errorf("alias = %q, want dup.t2", alias) + } + if got, ok := lookupSubagentArtifact(alias); !ok || got.Path != p2 || got.TaskIdx != 1 { + t.Errorf("alias lookup broken: (%+v, %v)", got, ok) } } diff --git a/cmd/odek/artifact_read_tool.go b/cmd/odek/artifact_read_tool.go index 3df8c73..2d553ee 100644 --- a/cmd/odek/artifact_read_tool.go +++ b/cmd/odek/artifact_read_tool.go @@ -269,8 +269,8 @@ func (t *artifactReadTool) Call(args string) (string, error) { } var b strings.Builder - fmt.Fprintf(&b, "artifact %s (%s, %d bytes, sha256:%s) — bytes %d..%d of %d", - entry.Ref.ID, entry.Ref.MediaType, size, shaPrefix, in.Offset, in.Offset+int64(len(data)), total) + fmt.Fprintf(&b, "artifact %s (%s, %d bytes, sha256:%s) — task %d — bytes %d..%d of %d", + entry.Ref.ID, entry.Ref.MediaType, size, shaPrefix, entry.TaskIdx+1, in.Offset, in.Offset+int64(len(data)), total) if truncated { b.WriteString(" — TRUNCATED, call again with offset to continue") } diff --git a/cmd/odek/main.go b/cmd/odek/main.go index 9b046e1..cd698ae 100644 --- a/cmd/odek/main.go +++ b/cmd/odek/main.go @@ -2332,16 +2332,17 @@ func builtinTools(dc danger.DangerousConfig, sm *skills.SkillManager, approver d tools := []odek.Tool{ shell, &delegateTasksTool{ - maxConcurrency: subConcurrency, - sharedSem: sharedChildSem(subConcurrency), - odekPath: os.Args[0], - apiKey: apiKey, - timeout: time.Duration(subTimeout) * time.Second, - maxDepth: subDepth, - budgetInherit: subInherit, - selfTrust: selfTrust, - profiles: tcfg.Profiles, - artifactsRoot: artifactsRoot, // empty ⇒ no artifact dirs created + maxConcurrency: subConcurrency, + sharedSem: sharedChildSem(subConcurrency), + odekPath: os.Args[0], + apiKey: apiKey, + timeout: time.Duration(subTimeout) * time.Second, + maxDepth: subDepth, + budgetInherit: subInherit, + selfTrust: selfTrust, + profiles: tcfg.Profiles, + artifactsRoot: artifactsRoot, // empty ⇒ no artifact dirs created + artifactReadAvailable: artifactReadEnabled(tcfg), }, &listSubagentProfilesTool{ profiles: tcfg.Profiles, @@ -2421,6 +2422,7 @@ func builtinTools(dc danger.DangerousConfig, sm *skills.SkillManager, approver d return tools } + // lists to a slice of tools. Unknown names are ignored. Required tools are // always preserved. func filterBuiltinTools(tools []odek.Tool, cfg config.ToolConfig, required map[string]bool) []odek.Tool { diff --git a/cmd/odek/subagent.go b/cmd/odek/subagent.go index 8daec41..78c807c 100644 --- a/cmd/odek/subagent.go +++ b/cmd/odek/subagent.go @@ -56,7 +56,8 @@ Tool conventions — use the dedicated tool, NOT shell: - Reserve shell for builds, installs, git, scripts. Don't run uname/pwd/date/whoami — read your Runtime Context header. -Report what you built, what files changed, and any issues. Be concise, then stop.` +End with a short headline: status, artifact file names, key decisions. +The files carry the detail. Be concise, then stop.` // subagentAmendments translates the pillar's principal-facing rules into // sub-agent terms: a child has no channel to the principal and no approval @@ -133,12 +134,12 @@ func neutraliseSubagentInputLiterals(s string) string { // optional — old children ignore the flags (version skew keeps the old // clamping behavior) and old parents simply never emit them. type taskBudget struct { - MaxRuntimeSeconds int64 `json:"max_runtime_seconds,omitempty"` - MaxToolCalls int64 `json:"max_tool_calls,omitempty"` - MaxCostUSD float64 `json:"max_cost_usd,omitempty"` - RuntimeExhausted bool `json:"runtime_exhausted,omitempty"` - ToolCallsExhausted bool `json:"tool_calls_exhausted,omitempty"` - CostExhausted bool `json:"cost_exhausted,omitempty"` + MaxRuntimeSeconds int64 `json:"max_runtime_seconds,omitempty"` + MaxToolCalls int64 `json:"max_tool_calls,omitempty"` + MaxCostUSD float64 `json:"max_cost_usd,omitempty"` + RuntimeExhausted bool `json:"runtime_exhausted,omitempty"` + ToolCallsExhausted bool `json:"tool_calls_exhausted,omitempty"` + CostExhausted bool `json:"cost_exhausted,omitempty"` } // clampLimits narrows the operator limits by the parent-supplied task @@ -537,19 +538,21 @@ type subagentWireContext struct { // subagentResult is the JSON contract written to stdout. type subagentResult struct { - Status string `json:"status"` // "success", "partial", "budget_exhausted" or "error" - Error string `json:"error,omitempty"` // error message - PartialReason string `json:"partial_reason,omitempty"` // time_budget | iteration_budget | execution_budget - Summary string `json:"summary"` // task summary - FilesChanged []string `json:"files_changed,omitempty"` // changed files - TokensUsed int `json:"tokens_used"` // total tokens consumed - Iterations int `json:"iterations"` // think-act cycles used - DurationSeconds float64 `json:"duration_seconds"` // wall-clock runtime - Denials []SubagentDenial `json:"denials,omitempty"` // policy denials observed (capped) - DenialsTotal int `json:"denials_total,omitempty"` // total denials seen - ParentSession string `json:"parent_session,omitempty"` // correlation id from --parent-session - CostUSD float64 `json:"cost_usd,omitempty"` // final server-side cost estimate (omitted when no prices configured) - Artifacts []artifact.Ref `json:"artifacts,omitempty"` // odek.artifact-ref/v1 — runner-scanned, parent-validated + Status string `json:"status"` // "success", "partial", "budget_exhausted" or "error" + Error string `json:"error,omitempty"` // error message + PartialReason string `json:"partial_reason,omitempty"` // time_budget | iteration_budget | execution_budget + Summary string `json:"summary"` // task summary (headline channel — capped) + SummaryTruncated bool `json:"summary_truncated,omitempty"` // headline was cut — parent should fetch artifacts (C) + SummaryRunes int `json:"summary_runes,omitempty"` // ORIGINAL headline rune count before the cap + FilesChanged []string `json:"files_changed,omitempty"` // changed files + TokensUsed int `json:"tokens_used"` // total tokens consumed + Iterations int `json:"iterations"` // think-act cycles used + DurationSeconds float64 `json:"duration_seconds"` // wall-clock runtime + Denials []SubagentDenial `json:"denials,omitempty"` // policy denials observed (capped) + DenialsTotal int `json:"denials_total,omitempty"` // total denials seen + ParentSession string `json:"parent_session,omitempty"` // correlation id from --parent-session + CostUSD float64 `json:"cost_usd,omitempty"` // final server-side cost estimate (omitted when no prices configured) + Artifacts []artifact.Ref `json:"artifacts,omitempty"` // odek.artifact-ref/v1 — runner-scanned, parent-validated } // ── Subagent Command ───────────────────────────────────────────────── @@ -1084,7 +1087,7 @@ func subagentCmd(args []string) error { // Classify the outcome (M1.3/M2.4 contract): typed budget errors map to // budget_exhausted, partial-summary markers to partial (with reason), // hard timeouts to error+timeout, everything else to success/error. - summary := extractSummary(allMessages) + summary, summaryRunes, summaryTruncated := extractSummaryInfo(allMessages) reason, partial := loop.PartialSummaryReason(summary) outcome := classifySubagentRun(err, partial, reason, sigCtx) @@ -1098,6 +1101,10 @@ func subagentCmd(args []string) error { DurationSeconds: latency.Seconds(), ParentSession: cfg.parentSession, } + if summaryTruncated { + result.SummaryTruncated = true + result.SummaryRunes = summaryRunes + } if err != nil { if outcome.TimedOut { @@ -1298,14 +1305,36 @@ func (e *subagentRunError) Error() string { // 500-rune cut. const subagentHeadlineMaxRunes = 2048 -func extractSummary(messages []llm.Message) string { - // Return the last assistant message content as summary +// extractSummaryInfo returns the child's final answer cut to the headline +// cap, the ORIGINAL rune count, and whether it was cut (C — the parent +// render turns this into a visible truncation marker). The bulk channel is +// the artifact protocol; the headline is a status summary. +func extractSummaryInfo(messages []llm.Message) (string, int, bool) { for i := len(messages) - 1; i >= 0; i-- { if messages[i].Role == "assistant" && messages[i].Content != "" { - return truncate(messages[i].Content, subagentHeadlineMaxRunes) + s, total := truncateWithLen(messages[i].Content, subagentHeadlineMaxRunes) + return s, total, total > subagentHeadlineMaxRunes } } - return "" + return "", 0, false +} + +func extractSummary(messages []llm.Message) string { + s, _, _ := extractSummaryInfo(messages) + return s +} + +// truncateWithLen cuts s to n runes (appending "…") and reports the +// ORIGINAL rune count so callers can surface a truncation marker (C). +func truncateWithLen(s string, n int) (string, int) { + runes := []rune(s) + if n <= 0 { + return "…", len(runes) + } + if len(runes) <= n { + return s, len(runes) + } + return string(runes[:n]) + "…", len(runes) } func extractFilesChanged(messages []llm.Message) []string { diff --git a/cmd/odek/subagent_artifact_registry.go b/cmd/odek/subagent_artifact_registry.go index 4f2ccfd..59b8715 100644 --- a/cmd/odek/subagent_artifact_registry.go +++ b/cmd/odek/subagent_artifact_registry.go @@ -47,11 +47,18 @@ type registrySlot struct { seq uint64 } +type origKey struct { + origID string + taskIdx int +} + var artifactRegistry struct { - mu sync.Mutex - byID map[string]*artifactEntry - order []registrySlot - seq uint64 + mu sync.Mutex + byID map[string]*artifactEntry + byOrig map[origKey]string // (original id, task) → effective registered id (D1 aliasing) + order []registrySlot + seq uint64 + marks []uint64 // active per-run floor watermarks (F) } func init() { @@ -64,18 +71,22 @@ func resetArtifactRegistryForTest() { artifactRegistry.mu.Lock() defer artifactRegistry.mu.Unlock() artifactRegistry.byID = map[string]*artifactEntry{} + artifactRegistry.byOrig = map[origKey]string{} artifactRegistry.order = nil artifactRegistry.seq = 0 + artifactRegistry.marks = nil } // registerSubagentArtifact records a validated artifact under its ref id. -// Last-wins on duplicate ids (a later task overwrites an earlier one); -// returns true when the id was already present so the caller can flag the -// ambiguity in the collated summary. Evicts the oldest LIVE entry at cap; -// superseded queue slots are skipped lazily. -func registerSubagentArtifact(e artifactEntry) bool { +// FIRST-WINS on duplicate ids (D1, SUBAGENT_ARTIFACT_DELIVERY_PLAN.md): the +// first occurrence keeps the plain id; a later task's duplicate registers +// under a probe-increment alias ".t", probing past any live +// entry — including real filename stems that collide with the alias +// namespace (dots are valid in ids). Aliasing never evicts a live entry. +// Returns the EFFECTIVE registered id and whether it was a duplicate. +func registerSubagentArtifact(e artifactEntry) (string, bool) { if e.Ref.ID == "" || e.Path == "" { - return false + return "", false } artifactRegistry.mu.Lock() @@ -85,21 +96,120 @@ func registerSubagentArtifact(e artifactEntry) bool { if artifactRegistry.byID == nil { artifactRegistry.byID = map[string]*artifactEntry{} } - _, dup := artifactRegistry.byID[e.Ref.ID] - artifactRegistry.byID[e.Ref.ID] = &e - artifactRegistry.order = append(artifactRegistry.order, registrySlot{id: e.Ref.ID, seq: e.seq}) + if artifactRegistry.byOrig == nil { + artifactRegistry.byOrig = map[origKey]string{} + } + orig := e.Ref.ID + id := orig + if _, taken := artifactRegistry.byID[orig]; taken { + alias := aliasArtifactID(orig, e.TaskIdx) + if alias == "" { + return "", true + } + e.Ref.ID = alias + id = alias + } + artifactRegistry.byID[id] = &e + artifactRegistry.byOrig[origKey{origID: orig, taskIdx: e.TaskIdx}] = id + artifactRegistry.order = append(artifactRegistry.order, registrySlot{id: id, seq: e.seq}) + evictArtifactRegistryLocked() + return id, id != orig +} + +// maxAliasProbes bounds the .t probe walk before falling back to a +// seq-derived id (monotonic, therefore always free). +const maxAliasProbes = 128 + +// aliasArtifactID derives a free alias for a duplicate artifact id: the +// owning task's ".t", probing forward past any live entry. +// Caller must hold the registry mutex. Returns "" only if the fallback is +// somehow taken (cannot happen: seq is monotonic). +func aliasArtifactID(origID string, taskIdx int) string { + for n := taskIdx + 1; n <= taskIdx+1+maxAliasProbes; n++ { + cand := fmt.Sprintf("%s.t%d", origID, n) + if !artifactIDRe.MatchString(cand) { + break // too long/invalid — probing further only gets longer + } + if _, taken := artifactRegistry.byID[cand]; !taken { + return cand + } + } + for { + artifactRegistry.seq++ + cand := fmt.Sprintf("artifact-%d", artifactRegistry.seq) + if _, taken := artifactRegistry.byID[cand]; !taken { + return cand + } + } +} +// evictArtifactRegistryLocked trims the queue to the cap, oldest first — +// but never evicts entries of an ACTIVE run (seq >= the lowest watermark): +// a delegate_tasks call's artifacts must stay resolvable while it is still +// collating (F — per-run registry floor). Caller holds the mutex. +func evictArtifactRegistryLocked() { for len(artifactRegistry.order) > artifactRegistryCap { front := artifactRegistry.order[0] + if m := minActiveMarkLocked(); m != 0 && front.seq >= m { + break + } artifactRegistry.order = artifactRegistry.order[1:] // Lazy eviction: pop the slot unconditionally, but only delete the // live entry when this slot is still its insertion slot (a - // last-wins re-registration owns the id now and has its own slot). + // re-registration owns the id now and has its own slot). if cur, ok := artifactRegistry.byID[front.id]; ok && cur.seq == front.seq { delete(artifactRegistry.byID, front.id) } } - return dup +} + +// beginArtifactRegistryRun opens a per-run floor: entries registered from +// now on (seq >= watermark) are not evicted until the matching +// endArtifactRegistryRun (F). Returns the watermark to pass to the end +// function. +func beginArtifactRegistryRun() uint64 { + artifactRegistry.mu.Lock() + defer artifactRegistry.mu.Unlock() + m := artifactRegistry.seq + 1 + artifactRegistry.marks = append(artifactRegistry.marks, m) + return m +} + +// endArtifactRegistryRun closes the floor opened by beginArtifactRegistryRun. +func endArtifactRegistryRun(mark uint64) { + artifactRegistry.mu.Lock() + defer artifactRegistry.mu.Unlock() + for i, m := range artifactRegistry.marks { + if m == mark { + artifactRegistry.marks = append(artifactRegistry.marks[:i], artifactRegistry.marks[i+1:]...) + return + } + } +} + +// minActiveMarkLocked reports the lowest active watermark (0 = none). +// Caller holds the mutex. +func minActiveMarkLocked() uint64 { + if len(artifactRegistry.marks) == 0 { + return 0 + } + min := artifactRegistry.marks[0] + for _, m := range artifactRegistry.marks[1:] { + if m < min { + min = m + } + } + return min +} + +// lookupEffectiveArtifactID resolves the id a given (original id, task) +// pair registered under — the render side uses it so the parent always +// copies an id artifact_read can actually resolve (D1 provenance). +func lookupEffectiveArtifactID(origID string, taskIdx int) (string, bool) { + artifactRegistry.mu.Lock() + defer artifactRegistry.mu.Unlock() + id, ok := artifactRegistry.byOrig[origKey{origID: origID, taskIdx: taskIdx}] + return id, ok } // lookupSubagentArtifact resolves an id to its validated entry. @@ -145,9 +255,9 @@ func registerTaskArtifacts(raw, dir string, taskIdx int) []string { if err != nil { continue } - dup := registerSubagentArtifact(artifactEntry{Ref: ref, Path: path, TaskIdx: taskIdx}) + id, dup := registerSubagentArtifact(artifactEntry{Ref: ref, Path: path, TaskIdx: taskIdx}) if dup { - notes = append(notes, fmt.Sprintf("[artifact] duplicate id %q — artifact_read now resolves to the task %d copy", ref.ID, taskIdx+1)) + notes = append(notes, fmt.Sprintf("[artifact] duplicate id %q — task %d copy registered as %q", ref.ID, taskIdx+1, id)) } } return notes @@ -195,8 +305,8 @@ func stagingDirFor(cwd, taskID string) string { // childArtifactNote builds the trusted runner instruction appended to the // child's request. It references the workspace-RELATIVE staging path only. func childArtifactNote(stagingRel string) string { - return "\n\nArtifact output: any deliverable larger than a short headline must ALSO be written as a file in " + - stagingRel + "/ (use your file tools; plain files, no subdirectories). Files there are delivered to the parent automatically — do not repeat their contents in your final answer." + return "\n\nResult delivery: your final answer is capped at ~2000 characters — content beyond the cap is lost. If your result fits in a short paragraph, just answer. Otherwise write each deliverable (report, findings, diffs, generated content) as a FLAT file directly in " + + stagingRel + "/ (no subdirectories — nested files are discarded), then end with a short headline: status, artifact file names, key decisions. Files are delivered to the orchestrator automatically; do not repeat their contents in the final answer." } // renameFailureHook lets tests force the copy fallback (rename across diff --git a/cmd/odek/subagent_delivery_test.go b/cmd/odek/subagent_delivery_test.go new file mode 100644 index 0000000..9c003ac --- /dev/null +++ b/cmd/odek/subagent_delivery_test.go @@ -0,0 +1,388 @@ +package main + +// RED-first tests for SUBAGENT_ARTIFACT_DELIVERY_PLAN.md v2 (proposals +// A+A2, B+B2, C, D1, F). Contract pins for the two-channel result-delivery +// model: headline cap + artifact channel, visible truncation marker, +// probe-increment artifact-id aliasing with provenance, inline byte budget, +// and the per-run registry floor. + +import ( + "crypto/sha256" + "encoding/hex" + "encoding/json" + "fmt" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/BackendStack21/odek/internal/artifact" + "github.com/BackendStack21/odek/internal/llm" +) + +// ── A: parent tool description carries the two-channel contract ────── + +func TestDelegateTasksDescription_ArtifactChannel(t *testing.T) { + desc := (&delegateTasksTool{}).Description() + for _, want := range []string{ + "Result delivery", + "Headline:", + "~2000 characters", + "Artifacts:", + "32 KB", + "artifact_read(id)", + "flat file in your artifact dir", + "trailing … means it was cut", + } { + if !strings.Contains(desc, want) { + t.Errorf("Description() missing %q — the parent LLM cannot learn the artifact protocol", want) + } + } +} + +// ── A2: guidance schema nudge ───────────────────────────────────────── + +func TestDelegateTasksSchema_GuidanceNudge(t *testing.T) { + schema, ok := (&delegateTasksTool{}).Schema().(map[string]any) + if !ok { + t.Fatal("Schema() must be a map") + } + tasks, ok := schema["properties"].(map[string]any)["tasks"].(map[string]any) + if !ok { + t.Fatal("schema missing tasks object") + } + items := tasks["items"].(map[string]any)["properties"].(map[string]any) + guidance, ok := items["guidance"].(map[string]any) + if !ok { + t.Fatal("schema missing tasks[].guidance") + } + desc, _ := guidance["description"].(string) + if !strings.Contains(desc, "flat file in your artifact dir") { + t.Errorf("guidance schema description missing artifact-delivery nudge: %q", desc) + } +} + +// ── B: child-side note rewrite ──────────────────────────────────────── + +func TestChildArtifactNote_V2(t *testing.T) { + note := childArtifactNote(".odek-artifacts/task-abc") + for _, want := range []string{ + "Result delivery:", + "~2000 characters", + "content beyond the cap is lost", + "If your result fits in a short paragraph, just answer.", + "FLAT file", + "no subdirectories", + "nested files are discarded", + "short headline: status, artifact file names, key decisions", + } { + if !strings.Contains(note, want) { + t.Errorf("childArtifactNote missing %q", want) + } + } + // Trust invariants from the v1 pin: relative staging path only. + if !strings.Contains(note, ".odek-artifacts/task-abc") { + t.Errorf("note missing relative staging path: %s", note) + } + if strings.Contains(note, "/.odek/") { + t.Errorf("note must not leak the canonical host path: %s", note) + } +} + +// ── B2: identity closing line aligned with the headline shape ──────── + +func TestSubagentIdentity_HeadlineShape(t *testing.T) { + if !strings.Contains(subagentIdentity, "End with a short headline: status, artifact file names, key decisions.") { + t.Error("subagentIdentity must end with the short-headline contract") + } + if !strings.Contains(subagentIdentity, "The files carry the detail.") { + t.Error("subagentIdentity must tell the child the files carry the detail") + } + if strings.Contains(subagentIdentity, "Report what you built, what files changed") { + t.Error("subagentIdentity still asks for a fat multi-part report — contradicts the headline shape") + } +} + +// ── C: truncateWithLen + truncation marker ──────────────────────────── + +func TestTruncateWithLen(t *testing.T) { + s, total := truncateWithLen("short", 2048) + if s != "short" || total != 5 { + t.Errorf("under cap: got (%q, %d)", s, total) + } + long := strings.Repeat("a", 3000) + s, total = truncateWithLen(long, 2048) + if len([]rune(s)) != subagentHeadlineMaxRunes+1 { // cap + ellipsis + t.Errorf("cut length = %d runes, want %d+ellipsis", len([]rune(s)), subagentHeadlineMaxRunes) + } + if total != 3000 { + t.Errorf("original rune count = %d, want 3000", total) + } + if !strings.HasSuffix(s, "…") { + t.Error("cut string must end with the ellipsis") + } + // Multibyte safety: rune-indexed cut must not split runes. + multi := strings.Repeat("🚀", 1000) // 4000 bytes, 1000 runes + s, total = truncateWithLen(multi, 10) + if total != 1000 { + t.Errorf("multibyte total = %d, want 1000", total) + } + if got := len([]rune(s)); got != 11 { + t.Errorf("multibyte cut = %d runes, want 10+ellipsis", got) + } +} + +func TestFormatTaskResult_TruncationMarker(t *testing.T) { + summary := strings.Repeat("a", 2100) + raw := fmt.Sprintf(`{"status":"success","summary":%q,"summary_truncated":true,"summary_runes":3000}`, summary) + + got := formatTaskResultDetailed(raw, 0, true) + if !strings.Contains(got, "headline truncated (2048 of 3000 runes shown)") { + t.Errorf("marker missing:\n%s", got) + } + if !strings.Contains(got, "fetch artifacts via artifact_read or re-run with a narrower goal") { + t.Errorf("next-action hint missing:\n%s", got) + } + + got = formatTaskResultDetailed(raw, 0, false) + if strings.Contains(got, "artifact_read") { + t.Error("hint must be conditional: mid-tree parents have no artifact_read") + } + if !strings.Contains(got, "headline truncated (2048 of 3000 runes shown)") { + t.Errorf("marker must render even without the tool:\n%s", got) + } + if !strings.Contains(got, "re-run with a narrower goal") { + t.Errorf("fallback next-action missing:\n%s", got) + } + + // Untruncated results carry no marker. + plain := `{"status":"success","summary":"tiny","summary_runes":4}` + if got = formatTaskResultDetailed(plain, 0, true); strings.Contains(got, "headline truncated") { + t.Errorf("marker must not appear for untruncated summaries:\n%s", got) + } +} + +func TestSubagentResult_OmitEmptyTruncationFields(t *testing.T) { + plain, err := json.Marshal(subagentResult{Status: "success", Summary: "x"}) + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(plain), "summary_truncated") || strings.Contains(string(plain), "summary_runes") { + t.Errorf("omitempty violated — old clients must see identical JSON:\n%s", plain) + } + marked, err := json.Marshal(subagentResult{Status: "success", Summary: "x", SummaryTruncated: true, SummaryRunes: 3000}) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(marked), `"summary_truncated":true`) || !strings.Contains(string(marked), `"summary_runes":3000`) { + t.Errorf("truncation fields missing from envelope:\n%s", marked) + } +} + +func TestExtractSummaryInfo(t *testing.T) { + long := strings.Repeat("b", 3000) + msgs := []llm.Message{{Role: "assistant", Content: long}} + s, total, truncated := extractSummaryInfo(msgs) + if !truncated || total != 3000 || len([]rune(s)) != subagentHeadlineMaxRunes+1 { + t.Errorf("extractSummaryInfo = (runes %d, total %d, truncated %v)", len([]rune(s)), total, truncated) + } + s, total, truncated = extractSummaryInfo([]llm.Message{{Role: "assistant", Content: "hi"}}) + if truncated || total != 2 || s != "hi" { + t.Errorf("short: = (%q, %d, %v)", s, total, truncated) + } +} + +// ── D1: probe-increment aliasing + provenance ───────────────────────── + +func refForFile(t *testing.T, dir, name, content string) artifact.Ref { + t.Helper() + p := filepath.Join(dir, name) + if err := os.WriteFile(p, []byte(content), 0o600); err != nil { + t.Fatal(err) + } + sum := sha256.Sum256([]byte(content)) + size := int64(len(content)) + return artifact.Ref{Schema: artifact.SchemaArtifactRef, ID: strings.TrimSuffix(name, filepath.Ext(name)), URI: "file://" + p, MediaType: "text/markdown", SHA256: hex.EncodeToString(sum[:]), SizeBytes: &size} +} + +func TestRegisterArtifactAlias_FirstWinsAndAlias(t *testing.T) { + resetArtifactRegistryForTest() + dir := t.TempDir() + r0 := refForFile(t, dir, "report.md", "first") + r1 := refForFile(t, dir, "report.md", "second") + r1.ID = "report" + + id, dup := registerSubagentArtifact(artifactEntry{Ref: r0, Path: strings.TrimPrefix(r0.URI, "file://"), TaskIdx: 0}) + if dup || id != "report" { + t.Errorf("first registration: got (%q, %v), want (report, false)", id, dup) + } + id, dup = registerSubagentArtifact(artifactEntry{Ref: r1, Path: strings.TrimPrefix(r1.URI, "file://"), TaskIdx: 1}) + if !dup { + t.Error("duplicate must be flagged") + } + if id != "report.t2" { + t.Errorf("alias = %q, want report.t2 (task 1 → .t)", id) + } + if e, ok := lookupSubagentArtifact("report"); !ok || e.TaskIdx != 0 { + t.Errorf("first-wins violated: lookup(report) = (%+v, %v)", e, ok) + } + if e, ok := lookupSubagentArtifact("report.t2"); !ok || e.TaskIdx != 1 { + t.Errorf("alias lookup: (%+v, %v)", e, ok) + } +} + +func TestRegisterArtifactAlias_NoEvictionOfLiveEntries(t *testing.T) { + resetArtifactRegistryForTest() + dir := t.TempDir() + // Task 0 claims the plain id first (first-wins owner). + first := refForFile(t, dir, "report.md", "first") + first.ID = "report" + if id, dup := registerSubagentArtifact(artifactEntry{Ref: first, Path: strings.TrimPrefix(first.URI, "file://"), TaskIdx: 0}); dup || id != "report" { + t.Fatalf("first registration: got (%q, %v)", id, dup) + } + // A real file legitimately named report.t2 (artifactIDRe allows dots). + real := refForFile(t, dir, "report.t2.md", "real stem") + real.ID = "report.t2" + if id, dup := registerSubagentArtifact(artifactEntry{Ref: real, Path: strings.TrimPrefix(real.URI, "file://"), TaskIdx: 2}); dup || id != "report.t2" { + t.Fatalf("real stem registration: got (%q, %v)", id, dup) + } + dup1 := refForFile(t, dir, "report.md", "dup") + dup1.ID = "report" + alias, dup := registerSubagentArtifact(artifactEntry{Ref: dup1, Path: strings.TrimPrefix(dup1.URI, "file://"), TaskIdx: 1}) + if !dup { + t.Error("duplicate must be flagged") + } + if alias != "report.t3" { + t.Errorf("alias must probe past the taken report.t2, got %q", alias) + } + // The plain id still belongs to task 0; the real stem was never evicted. + if e, ok := lookupSubagentArtifact("report"); !ok || e.TaskIdx != 0 { + t.Errorf("first-wins owner displaced: (%+v, %v)", e, ok) + } + if e, ok := lookupSubagentArtifact("report.t2"); !ok || e.TaskIdx != 2 { + t.Errorf("live entry evicted by aliasing: (%+v, %v)", e, ok) + } +} + +func TestLookupEffectiveArtifactID(t *testing.T) { + resetArtifactRegistryForTest() + dir := t.TempDir() + r := refForFile(t, dir, "dup.md", "x") + r.ID = "dup" + registerSubagentArtifact(artifactEntry{Ref: r, Path: strings.TrimPrefix(r.URI, "file://"), TaskIdx: 0}) + r2 := refForFile(t, dir, "dup2.md", "y") + r2.ID = "dup" + alias, _ := registerSubagentArtifact(artifactEntry{Ref: r2, Path: strings.TrimPrefix(r2.URI, "file://"), TaskIdx: 1}) + if eff, ok := lookupEffectiveArtifactID("dup", 0); !ok || eff != "dup" { + t.Errorf("task 0 effective id: (%q, %v)", eff, ok) + } + if eff, ok := lookupEffectiveArtifactID("dup", 1); !ok || eff != alias { + t.Errorf("task 1 effective id: (%q, %v), want alias %q", eff, ok, alias) + } + if _, ok := lookupEffectiveArtifactID("missing", 3); ok { + t.Error("unknown orig id must miss") + } +} + +func TestArtifactRead_Provenance(t *testing.T) { + resetArtifactRegistryForTest() + dir := t.TempDir() + r := refForFile(t, dir, "prov.md", "provenance body") + registerSubagentArtifact(artifactEntry{Ref: r, Path: strings.TrimPrefix(r.URI, "file://"), TaskIdx: 1}) + tool := &artifactReadTool{} + tool.SetContext(t.Context()) + got, _ := tool.Call(fmt.Sprintf(`{"id":%q}`, r.ID)) + if !strings.Contains(got, "task 2") { + t.Errorf("artifact_read output missing owning-task provenance:\n%s", got) + } +} + +func TestFormatTaskResult_ArtifactProvenanceLine(t *testing.T) { + resetArtifactRegistryForTest() + dir := t.TempDir() + c1, c2 := "first body", "second body" + p1 := filepath.Join(dir, "dup.md") + p2 := filepath.Join(dir, "dup2.md") + os.WriteFile(p1, []byte(c1), 0o600) + os.WriteFile(p2, []byte(c2), 0o600) + refJSON := func(p, content string) string { + sum := sha256.Sum256([]byte(content)) + return fmt.Sprintf(`{"schema":%q,"id":"dup","uri":"file://%s","media_type":"text/markdown","sha256":%q,"size_bytes":%d}`, + artifact.SchemaArtifactRef, p, hex.EncodeToString(sum[:]), len(content)) + } + registerTaskArtifacts(fmt.Sprintf(`{"status":"success","artifacts":[%s]}`, refJSON(p1, c1)), dir, 0) + raw2 := fmt.Sprintf(`{"status":"success","artifacts":[%s]}`, refJSON(p2, c2)) + registerTaskArtifacts(raw2, dir, 1) + + got := formatTaskResultDetailed(raw2, 1, true, dir) + if !strings.Contains(got, "report") && !strings.Contains(got, "dup.t2") { + t.Errorf("render must show the effective (aliased) id:\n%s", got) + } + if !strings.Contains(got, "task 2") { + t.Errorf("artifact line missing owning-task provenance:\n%s", got) + } +} + +// ── F: inline byte budget (largest-first) ───────────────────────────── + +func TestRenderArtifacts_InlineBudget(t *testing.T) { + resetArtifactRegistryForTest() + dir := t.TempDir() + var refs []string + sizes := []int{30 << 10, 29 << 10, 28 << 10, 27 << 10, 26 << 10, 25 << 10, 24 << 10, 23 << 10} + for i, size := range sizes { + content := strings.Repeat(string(rune('a'+i)), size) + p := filepath.Join(dir, fmt.Sprintf("f%d.md", i)) + os.WriteFile(p, []byte(content), 0o600) + sum := sha256.Sum256([]byte(content)) + refs = append(refs, fmt.Sprintf(`{"schema":%q,"id":"f%d","uri":"file://%s","media_type":"text/markdown","sha256":%q,"size_bytes":%d}`, + artifact.SchemaArtifactRef, i, p, hex.EncodeToString(sum[:]), size)) + } + raw := fmt.Sprintf(`{"status":"success","artifacts":[%s]}`, strings.Join(refs, ",")) + + got := formatTaskResultDetailed(raw, 0, true, dir) + blocks := strings.Count(got, "--- artifact:") + // Budget 128 KiB, largest-first: 30+29+28+27 = 114 KiB inlined; the + // 26 KiB artifact would push past the budget → metadata line only. + if blocks != 4 { + t.Errorf("inlined %d artifacts, want 4 under the 128 KiB per-call budget:\n%s", blocks, got) + } + for i := range sizes { + if !strings.Contains(got, fmt.Sprintf("- f%d (", i)) { + t.Errorf("artifact f%d lost its metadata line (budget must degrade, never drop)", i) + } + } +} + +// ── F: per-run registry floor ───────────────────────────────────────── + +func TestArtifactRegistry_Floor(t *testing.T) { + resetArtifactRegistryForTest() + // Baseline: without a run mark, cap is enforced. + fill := func(prefix string, n, taskIdx int) { + for i := 0; i < n; i++ { + e := artifactEntry{TaskIdx: taskIdx} + e.Ref = artifact.Ref{ID: fmt.Sprintf("%s%d", prefix, i)} + e.Path = "/tmp/nonexistent" + registerSubagentArtifact(e) + } + } + fill("a", 600, 0) + if _, total := listSubagentArtifactIDs(); total != artifactRegistryCap { + t.Fatalf("baseline cap: live = %d, want %d", total, artifactRegistryCap) + } + + // With an active run mark, the current run's entries survive intact. + resetArtifactRegistryForTest() + mark := beginArtifactRegistryRun() + fill("b", 600, 0) + _, total := listSubagentArtifactIDs() + if total != 600 { + t.Errorf("floor violated: live = %d, want 600 (current-run entries must not evict)", total) + } + endArtifactRegistryRun(mark) + fill("c", 1, 0) + if _, total = listSubagentArtifactIDs(); total != artifactRegistryCap { + t.Errorf("after run end: live = %d, want %d (eviction must resume)", total, artifactRegistryCap) + } +} diff --git a/cmd/odek/subagent_tool.go b/cmd/odek/subagent_tool.go index d9d3465..0d4c16b 100644 --- a/cmd/odek/subagent_tool.go +++ b/cmd/odek/subagent_tool.go @@ -12,6 +12,7 @@ import ( "io" "os" "os/exec" + "sort" "strconv" "strings" "sync" @@ -64,6 +65,11 @@ type delegateTasksTool struct { // side-effect free. artifactsRoot string + // artifactReadAvailable mirrors artifactReadEnabled(tcfg) at construction: + // the truncation marker's next-action hint names artifact_read ONLY when + // this process actually has the tool (mid-tree parents don't — R2-3). + artifactReadAvailable bool + // profiles carries the operator's resolved capability profiles (P4) // for parent-side fail-closed validation: an unknown profile name must // fail the task BEFORE a child is spawned. Nil = operator defined no @@ -183,11 +189,16 @@ Key rules: - Delegation depth is capped (subagent.max_depth, default 2) — do leaf work yourself when close to the cap - After all complete, synthesize the results into a cohesive answer -Output format per sub-agent: -- Summary of what was built -- Files changed +Result delivery — two channels per sub-agent: +- Headline: the sub-agent's final answer, capped at ~2000 characters. Treat it as a status summary, not the full result; a trailing … means it was cut. +- Artifacts: deliverables the sub-agent wrote as files (it is instructed to stage anything larger than a headline in its task's artifact dir) are validated and listed under "artifacts:" — id, type, byte size, one-line summary. Text artifacts up to 32 KB are inlined in full; fetch larger ones with artifact_read(id). +- For artifact-heavy tasks (reports, audits, reviews, generated files), put it in ` + "`guidance`" + `: "Write the full deliverable as a flat file in your artifact dir; keep the final answer to a short headline." Then read file-backed artifacts with artifact_read before synthesizing. + +Output format per sub-agent (headline stays SHORT — status, artifact names, key decisions; the files carry the detail): +- Status: built / blocked / failed, one line +- Artifact file names (omit when everything fit inline) - Key decisions made -- Any issues encountered` +- artifacts: file-backed deliverables (inlined when ≤32 KB; artifact_read otherwise)` } func (t *delegateTasksTool) Schema() any { @@ -212,7 +223,7 @@ func (t *delegateTasksTool) Schema() any { }, "guidance": map[string]any{ "type": "string", - "description": "Optional. How the sub-agent should approach the task — delivered as part of its request, NOT as its system prompt. The sub-agent's identity and safety rules are fixed and cannot be overridden. Use this to steer the approach, e.g. \"Review for token-validation gaps and timing attacks\" or \"Find the root cause before changing code\".", + "description": "Optional. How the sub-agent should approach the task — delivered as part of its request, NOT as its system prompt. The sub-agent's identity and safety rules are fixed and cannot be overridden. Use this to steer the approach, e.g. \"Review for token-validation gaps and timing attacks\" or \"Find the root cause before changing code\". For output-heavy tasks, instruct: \"Write the full deliverable as a flat file in your artifact dir; keep the final answer to a short headline.\"", }, "trust_level": map[string]any{ "type": "string", @@ -242,6 +253,11 @@ func (t *delegateTasksTool) Schema() any { } func (t *delegateTasksTool) Call(args string) (string, error) { + // F: per-run registry floor — this call's artifacts stay resolvable for + // the whole collation, even if concurrent sessions pressure the registry. + runMark := beginArtifactRegistryRun() + defer endArtifactRegistryRun(runMark) + var input struct { Tasks []struct { Goal string `json:"goal"` @@ -356,7 +372,7 @@ func (t *delegateTasksTool) Call(args string) (string, error) { buf.WriteString("📋 Sub-agent results:\n\n") for i, r := range results { fmt.Fprintf(&buf, "─── Task %d: %s ───\n", i+1, truncate(input.Tasks[i].Goal, 60)) - buf.WriteString(formatTaskResult(r, dirs[i])) + buf.WriteString(formatTaskResultDetailed(r, i, t.artifactReadAvailable, dirs[i])) // M2: validated refs join the session registry so artifact_read can // resolve them by id later in the turn. if notes := registerTaskArtifacts(r, dirs[i], i); len(notes) > 0 { @@ -616,6 +632,26 @@ const ( // (metadata-only line; small text artifacts inlined). No roots ⇒ every ref // is rejected — a lost root can never become a trust upgrade. func formatTaskResult(raw string, artifactRoots ...string) string { + return formatTaskResultDetailed(raw, -1, true, artifactRoots...) +} + +// formatTaskResultDetailed renders one child's framed result as compact text +// for the parent's context. taskIdx (0-based) drives artifact-id +// provenance: ids render under their EFFECTIVE registered id (D1 aliasing) +// plus the owning task number; pass -1 when the task index is unknown. +// artifactReadAvailable gates the truncation marker's next-action hint — +// mid-tree parents have no artifact_read (R2-3), so pointing them at it +// would send them chasing a tool they don't have. Parsed envelopes render +// as fields (status, headline, files, denials, artifacts); anything +// unparseable falls back to the raw payload capped at +// maxSubagentSummaryResultBytes. Child output is model-controlled and stays +// inside the caller's untrusted wrapper. +// +// artifactRoots carries the per-task artifact dir(s) the parent created: +// every incoming ref is validated fail-closed against them before render +// (metadata-only line; small text artifacts inlined). No roots ⇒ every ref +// is rejected — a lost root can never become a trust upgrade. +func formatTaskResultDetailed(raw string, taskIdx int, artifactReadAvailable bool, artifactRoots ...string) string { var r subagentResult if err := json.Unmarshal([]byte(raw), &r); err != nil { if len(raw) > maxSubagentSummaryResultBytes { @@ -653,6 +689,13 @@ func formatTaskResult(raw string, artifactRoots ...string) string { if r.Summary != "" { fmt.Fprintf(&b, "summary: %s\n", truncate(r.Summary, subagentHeadlineMaxRunes)) } + if r.SummaryTruncated { + hint := " — fetch artifacts via artifact_read or re-run with a narrower goal" + if !artifactReadAvailable { + hint = " — re-run with a narrower goal" + } + fmt.Fprintf(&b, "headline truncated (%d of %d runes shown)%s\n", subagentHeadlineMaxRunes, r.SummaryRunes, hint) + } if len(r.FilesChanged) > 0 { files := r.FilesChanged if len(files) > maxRenderedFiles { @@ -675,19 +718,33 @@ func formatTaskResult(raw string, artifactRoots ...string) string { } fmt.Fprintf(&b, "denials (%d of %d): %s\n", len(shown), total, strings.Join(parts, "; ")) } - b.WriteString(renderArtifacts(r.Artifacts, artifactRoots)) + b.WriteString(renderArtifacts(r.Artifacts, artifactRoots, taskIdx)) return b.String() } +// maxInlinePerCallBytes caps the TOTAL artifact bytes inlined across one +// render (F): 8 tasks × 64 refs × 32 KiB would otherwise inject ~16 MiB +// into the parent's context from a single delegate_tasks call. Eligible +// artifacts are inlined largest-first while the budget lasts; the rest +// degrade to their metadata line (never dropped). +const maxInlinePerCallBytes = 128 << 10 // 128 KiB + // renderArtifacts validates each incoming ref fail-closed against the -// per-task roots and renders metadata-only lines; small text artifacts are +// per-task roots and renders metadata-only lines; text artifacts are // inlined from the VALIDATED path returned by artifact.Validate (symlinks -// resolved, size+sha256 verified). Invalid refs are dropped with a flag — -// never fatal to the summary. Raw absolute paths are never rendered. -func renderArtifacts(refs []artifact.Ref, roots []string) string { +// resolved, size+sha256 verified) within the per-call inline budget, +// largest-first. Invalid refs are dropped with a flag — never fatal to the +// summary. Raw absolute paths are never rendered. +func renderArtifacts(refs []artifact.Ref, roots []string, taskIdx int) string { if len(refs) == 0 { return "" } + type validated struct { + ref artifact.Ref + path string + size int64 + } + var ok []validated var b strings.Builder b.WriteString("artifacts:\n") for _, ref := range refs { @@ -700,23 +757,58 @@ func renderArtifacts(refs []artifact.Ref, roots []string) string { if ref.SizeBytes != nil { size = *ref.SizeBytes } - shaPrefix := ref.SHA256 + ok = append(ok, validated{ref: ref, path: path, size: size}) + } + + // F: pick the inline set largest-first within the per-call budget. + type candidate struct { + idx int + size int64 + } + var elig []candidate + for i, v := range ok { + if strings.HasPrefix(v.ref.MediaType, "text/") && v.size <= maxInlineArtifactBytes { + elig = append(elig, candidate{idx: i, size: v.size}) + } + } + sort.Slice(elig, func(i, j int) bool { return elig[i].size > elig[j].size }) + inlined := make(map[int]bool, len(elig)) + budget := int64(maxInlinePerCallBytes) + for _, c := range elig { + if c.size <= budget { + inlined[c.idx] = true + budget -= c.size + } + } + + for i, v := range ok { + displayID := v.ref.ID + if taskIdx >= 0 { + if eff, found := lookupEffectiveArtifactID(v.ref.ID, taskIdx); found { + displayID = eff + } + } + shaPrefix := v.ref.SHA256 if len(shaPrefix) > 12 { shaPrefix = shaPrefix[:12] } - fmt.Fprintf(&b, " - %s (%s, %d bytes, sha256:%s)", ref.ID, ref.MediaType, size, shaPrefix) - if ref.Summary != "" { - fmt.Fprintf(&b, " — %s", truncate(ref.Summary, maxArtifactSummaryLine)) + if taskIdx >= 0 { + fmt.Fprintf(&b, " - %s (%s, %d bytes, sha256:%s, task %d)", displayID, v.ref.MediaType, v.size, shaPrefix, taskIdx+1) + } else { + fmt.Fprintf(&b, " - %s (%s, %d bytes, sha256:%s)", displayID, v.ref.MediaType, v.size, shaPrefix) + } + if v.ref.Summary != "" { + fmt.Fprintf(&b, " — %s", truncate(v.ref.Summary, maxArtifactSummaryLine)) } b.WriteString("\n") - if strings.HasPrefix(ref.MediaType, "text/") && size <= maxInlineArtifactBytes { + if inlined[i] { // Same read-time verification as artifact_read: the validated // path is re-opened O_NOFOLLOW and re-hashed against the ref // before any byte is inlined; failure just skips the inline // preview (never fatal to the summary). - if data, _, _, err := verifyArtifactWindow(path, ref, maxInlineArtifactBytes, 0, maxInlineArtifactBytes); err == nil { - fmt.Fprintf(&b, " --- artifact: %s ---\n%s\n --- end artifact ---\n", ref.ID, strings.TrimRight(string(data), "\n")) + if data, _, _, err := verifyArtifactWindow(v.path, v.ref, maxInlineArtifactBytes, 0, maxInlineArtifactBytes); err == nil { + fmt.Fprintf(&b, " --- artifact: %s ---\n%s\n --- end artifact ---\n", displayID, strings.TrimRight(string(data), "\n")) } } } diff --git a/docs/SUBAGENTS.md b/docs/SUBAGENTS.md index f5382f8..133e1fc 100644 --- a/docs/SUBAGENTS.md +++ b/docs/SUBAGENTS.md @@ -111,6 +111,8 @@ The `delegate_tasks` tool is available in all odek modes (CLI, REPL, Web UI). Th { "status": "success", // "success" or "error" "summary": "Built JWT auth middleware with HS256 signing", + "summary_truncated": true, // present only when the headline was cut at 2048 runes + "summary_runes": 3120, // original headline length — present only when truncated "files_changed": ["internal/middleware/auth.go"], "tokens_used": 4200, "iterations": 3, @@ -118,6 +120,14 @@ The `delegate_tasks` tool is available in all odek modes (CLI, REPL, Web UI). Th } ``` +The `summary` is the headline channel: the child's final answer, capped at +2048 runes. When `summary_truncated` is set, the parent-side render appends +`headline truncated (2048 of N runes shown) — fetch artifacts via +artifact_read or re-run with a narrower goal` (the `artifact_read` half is +omitted in processes that do not have the tool, i.e. mid-tree parents). +Both fields are `omitempty`: results that fit the headline produce byte-identical +JSON to older versions. + The `parent_session` field is omitted when `--parent-session` was not supplied. Use it to correlate sub-agent results back to the originating parent session in logs, dashboards, or audit pipelines. @@ -480,16 +490,21 @@ Parent synthesizes: "Created 3 files: ## Result artifacts -When a sub-agent produces output too large for the headline summary (large reports, dumps, generated fixtures), it writes plain files into its per-task staging directory (`.odek-artifacts//` inside the workspace). The runner relocates them to `~/.odek/artifacts///`, measures sha256/size, and returns `odek.artifact-ref/v1` references with the result. +The result contract is two-channel: the headline (≤ 2048 runes) carries status; +the bulk rides files. The child is told this at request time: deliverables +larger than a headline go as FLAT files into its per-task staging directory +(`.odek-artifacts//` inside the workspace — nested directories are +discarded). The runner relocates them to `~/.odek/artifacts///`, +measures sha256/size, and returns `odek.artifact-ref/v1` references with the result. -The parent sees one metadata line per artifact — id, media type, size, short hash, first-line summary — plus the inlined content of small text artifacts (≤ 32 KiB). Everything larger is readable on demand via `artifact_read`: +The parent sees one metadata line per artifact — id, media type, size, short hash, owning task, first-line summary — plus inlined content for small text artifacts (≤ 32 KiB) within a 128 KiB per-call inline budget (largest first; the rest degrade to their metadata line, never dropped). Everything larger is readable on demand via `artifact_read`: ``` artifact_read({ "id": "report" }) # first 64 KiB artifact_read({ "id": "report", "offset": 65536 }) # continue paging ``` -`artifact_read` is a parent-side tool; the model passes an id, never a path — resolution goes through the session registry of validated refs. Refs that fail validation (wrong hash, path escape) are dropped with an explicit flag and never rendered. +`artifact_read` is a parent-side tool; the model passes an id, never a path — resolution goes through the session registry of validated refs, and each read reports the owning task. Ids derive from filename stems, and **first occurrence wins**: if two tasks stage files with the same stem (e.g. both write `report.md`), the first task keeps the plain id and later duplicates register under `.t` aliases (`report.t2`, `report.t3`, …) — probing past any live entry, including real stems that collide with the alias namespace. Rendered artifact lines always show the effective id plus the task number, so the parent copies an id `artifact_read` can actually resolve. Refs that fail validation (wrong hash, path escape) are dropped with an explicit flag and never rendered. Lifecycle: deleting a session deletes its artifacts (all paths — CLI, API, Telegram, retention sweep); the storage janitor backstop sweeps orphans after `maintenance.artifacts_max_age_hours` (default 24 hours, `0` = keep forever). From efcdfc86a46950ff9505de23253f3f4bc9229e65 Mon Sep 17 00:00:00 2001 From: Rolando Santamaria Maso <4096860+jkyberneees@users.noreply.github.com> Date: Wed, 2 Sep 2026 21:51:55 +0200 Subject: [PATCH 2/4] =?UTF-8?q?test(ui):=20poll=20instead=20of=20sleeping?= =?UTF-8?q?=20=E2=80=94=20sweep/friction=20timers=20raced=20CI?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ui-js suite failed 2/3 CI runs today (and once on main): four tests slept fixed real-time windows against a 1s setInterval sweep and a 1.5s friction-gate setTimeout. Under runner load the timers starve past the window and the assertions run too early. Replace all four sleeps with a waitFor(cond) poll (20ms cadence, 15s deadline). The wrong-word friction test now first proves the gate passed via the correct word, so its negative assertion is never vacuous. Test-only; no product code touched. --- cmd/odek/ui/js/approvals.test.js | 54 +++++++++++++++++++++++--------- cmd/odek/ui/js/lifecycle.test.js | 17 +++++++++- 2 files changed, 56 insertions(+), 15 deletions(-) diff --git a/cmd/odek/ui/js/approvals.test.js b/cmd/odek/ui/js/approvals.test.js index 8080899..bbb4aa3 100644 --- a/cmd/odek/ui/js/approvals.test.js +++ b/cmd/odek/ui/js/approvals.test.js @@ -313,31 +313,57 @@ test('friction mode: typing the word enables approve; click sends', async () => // Disabled buttons dispatch nothing (shim matches browser behavior). approve.fire('click'); assert.deepEqual(sent, []); - // The input listener attaches after the 1.5s gate. - await new Promise((r) => setTimeout(r, 1600)); - const input = S.activeApprovalCard.querySelector('.ac-friction-input'); - input.value = 'Approve '; // case/whitespace-insensitive per spec - input.fire('input'); + // The input listener attaches after the 1.5s gate — poll, don't sleep. + const input = () => S.activeApprovalCard.querySelector('.ac-friction-input'); + await waitFor(() => { + const el = input(); + el.value = 'Approve '; // case/whitespace-insensitive per spec + el.fire('input'); + return !approve.disabled; + }, 'friction gate to pass on the correct word'); assert.equal(approve.disabled, false, 'correct word enables the button'); approve.click(); assert.deepEqual(sent, [{ type: 'approval_response', id: 'apr-1', action: 'approve' }]); }); +// Polls cond until truthy (20 ms cadence). Replaces fixed real-time sleeps, +// which raced the 1.5s friction gate under CI load. +async function waitFor(cond, label, timeoutMs = 15000) { + const start = Date.now(); + for (;;) { + if (cond()) return; + if (Date.now() - start > timeoutMs) { + assert.ok(cond(), `timed out after ${timeoutMs}ms waiting for: ${label}`); + } + await new Promise((r) => setTimeout(r, 20)); + } +} + test('friction mode: wrong word keeps approve disabled', async () => { const { approve } = queueOne({ friction: true, friction_approvals: 4 }); - await new Promise((r) => setTimeout(r, 1600)); - const input = S.activeApprovalCard.querySelector('.ac-friction-input'); - input.value = 'yes'; - input.fire('input'); + const input = () => S.activeApprovalCard.querySelector('.ac-friction-input'); + // Prove the gate passed first (correct word enables) so the negative + // assertion below is not vacuous while the listener is still detached. + await waitFor(() => { + const el = input(); + el.value = 'approve'; + el.fire('input'); + return !approve.disabled; + }, 'friction gate to pass'); + input().value = 'yes'; + input().fire('input'); assert.equal(approve.disabled, true); }); test('friction mode: Enter in the input approves once the gate passes', async () => { queueOne({ friction: true, friction_approvals: 3 }); - await new Promise((r) => setTimeout(r, 1600)); - const input = S.activeApprovalCard.querySelector('.ac-friction-input'); - input.value = 'approve'; - input.fire('input'); - input.fire('keydown', { key: 'Enter' }); + const input = () => S.activeApprovalCard.querySelector('.ac-friction-input'); + await waitFor(() => { + const el = input(); + el.value = 'approve'; + el.fire('input'); + el.fire('keydown', { key: 'Enter' }); + return sent.length === 1; + }, 'friction gate to pass and Enter to approve'); assert.deepEqual(sent, [{ type: 'approval_response', id: 'apr-1', action: 'approve' }]); }); diff --git a/cmd/odek/ui/js/lifecycle.test.js b/cmd/odek/ui/js/lifecycle.test.js index d92dede..ea79fc4 100644 --- a/cmd/odek/ui/js/lifecycle.test.js +++ b/cmd/odek/ui/js/lifecycle.test.js @@ -387,12 +387,27 @@ test('approval_request honors the frame timeout_seconds for the countdown', () = assert.match(S.activeApprovalCard.querySelector('.ac-deadline').textContent, /expires in 25s/); }); +// Polls cond until truthy (20 ms cadence). Replaces fixed real-time sleeps, +// which raced the 1s sweep interval and the 1.5s friction gate under CI load +// (ui-js failed 2/3 runs on 2026-09-02). +async function waitFor(cond, label, timeoutMs = 15000) { + const start = Date.now(); + for (;;) { + if (cond()) return; + if (Date.now() - start > timeoutMs) { + assert.ok(cond(), `timed out after ${timeoutMs}ms waiting for: ${label}`); + } + await new Promise((r) => setTimeout(r, 20)); + } +} + test('sweep auto-closes only the expired card and shows the next one', async () => { deliver({ type: 'approval_request', id: 'apr-fast', risk: 'safe', command: 'sleep', allow_trust: true, timeout_seconds: 1 }); deliver({ type: 'approval_request', id: 'apr-slow', risk: 'safe', command: 'echo', allow_trust: true }); assert.equal(S.activeApprovalId, 'apr-fast'); - await new Promise((r) => setTimeout(r, 1400)); // one sweep tick past the 1s deadline + // The sweep ticks on a real 1s interval — wait for the state, not the clock. + await waitFor(() => S.activeApprovalId === 'apr-slow', 'expired head autoclosed, next card shown'); assert.equal(S.activeApprovalId, 'apr-slow', 'expired head autoclosed, next card shown'); assert.equal(S.approvalQueue.length, 1, 'the unexpired request survives the sweep'); From 04a90795c58efaa55e0fd96ec43fb272c63af4e5 Mon Sep 17 00:00:00 2001 From: Rolando Santamaria Maso <4096860+jkyberneees@users.noreply.github.com> Date: Wed, 2 Sep 2026 22:09:36 +0200 Subject: [PATCH 3/4] =?UTF-8?q?fix(subagents):=20close=20adversarial-revie?= =?UTF-8?q?w=20findings=20=E2=80=94=20render=20order,=20byOrig=20hygiene,?= =?UTF-8?q?=20stem=20collisions?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Judge pass 1 on the branch diff validated 4 findings: - P1: the collate loop rendered before registering, so an aliased duplicate advertised the plain id and artifact_read(plain) resolved to the WRONG task's bytes. Register first, render second; notes still append after the artifacts block. - P2: byOrig entries now die with their registry entry on eviction (stale alias ids previously rendered as dead ends; long-lived serve leaked one map entry per registration). - P3: same-task stem collisions (report.md + report.txt both report id 'report') now get occurrence-indexed byOrig slots; the render consumes them in envelope order, matching registration order. - P4: the provenance test now uses production order and asserts the effective id positively (- dup.t2 () and the plain id negatively (- dup () — previously vacuous. Adds TestRegisterArtifact_SameTaskStemCollision and TestEvictionCleansEffectiveIDMap. --- cmd/odek/subagent_artifact_registry.go | 38 +++++++++++---- cmd/odek/subagent_delivery_test.go | 66 ++++++++++++++++++++++++-- cmd/odek/subagent_tool.go | 21 ++++++-- 3 files changed, 108 insertions(+), 17 deletions(-) diff --git a/cmd/odek/subagent_artifact_registry.go b/cmd/odek/subagent_artifact_registry.go index 59b8715..59e3358 100644 --- a/cmd/odek/subagent_artifact_registry.go +++ b/cmd/odek/subagent_artifact_registry.go @@ -38,6 +38,9 @@ type artifactEntry struct { TaskIdx int RegisteredAt time.Time seq uint64 + OrigID string // id as the child reported it (pre-alias) + key origKey // this entry's byOrig slot — removed on eviction + occBase origKey // occ-counter key to decrement on eviction } // registrySlot pairs an id with the seq that inserted it; stale slots @@ -50,12 +53,14 @@ type registrySlot struct { type origKey struct { origID string taskIdx int + occ int // occurrence of origID within the task (same stem, two extensions) } var artifactRegistry struct { mu sync.Mutex byID map[string]*artifactEntry - byOrig map[origKey]string // (original id, task) → effective registered id (D1 aliasing) + byOrig map[origKey]string // (original id, task, occurrence) → effective registered id (D1 aliasing) + occ map[origKey]int // registration count per (orig id, task) — assigns occurrence indices order []registrySlot seq uint64 marks []uint64 // active per-run floor watermarks (F) @@ -72,6 +77,7 @@ func resetArtifactRegistryForTest() { defer artifactRegistry.mu.Unlock() artifactRegistry.byID = map[string]*artifactEntry{} artifactRegistry.byOrig = map[origKey]string{} + artifactRegistry.occ = map[origKey]int{} artifactRegistry.order = nil artifactRegistry.seq = 0 artifactRegistry.marks = nil @@ -109,8 +115,15 @@ func registerSubagentArtifact(e artifactEntry) (string, bool) { e.Ref.ID = alias id = alias } + occBase := origKey{origID: orig, taskIdx: e.TaskIdx} + key := occBase + key.occ = artifactRegistry.occ[occBase] + artifactRegistry.occ[occBase]++ + e.OrigID = orig + e.key = key + e.occBase = occBase artifactRegistry.byID[id] = &e - artifactRegistry.byOrig[origKey{origID: orig, taskIdx: e.TaskIdx}] = id + artifactRegistry.byOrig[key] = id artifactRegistry.order = append(artifactRegistry.order, registrySlot{id: id, seq: e.seq}) evictArtifactRegistryLocked() return id, id != orig @@ -156,8 +169,14 @@ func evictArtifactRegistryLocked() { artifactRegistry.order = artifactRegistry.order[1:] // Lazy eviction: pop the slot unconditionally, but only delete the // live entry when this slot is still its insertion slot (a - // re-registration owns the id now and has its own slot). + // re-registration owns the id now and has its own slot). The entry's + // byOrig slot and occ counter go with it — the effective-id map must + // never outlive the ids it maps to. if cur, ok := artifactRegistry.byID[front.id]; ok && cur.seq == front.seq { + delete(artifactRegistry.byOrig, cur.key) + if artifactRegistry.occ[cur.occBase] > 0 { + artifactRegistry.occ[cur.occBase]-- + } delete(artifactRegistry.byID, front.id) } } @@ -202,13 +221,16 @@ func minActiveMarkLocked() uint64 { return min } -// lookupEffectiveArtifactID resolves the id a given (original id, task) -// pair registered under — the render side uses it so the parent always -// copies an id artifact_read can actually resolve (D1 provenance). -func lookupEffectiveArtifactID(origID string, taskIdx int) (string, bool) { +// lookupEffectiveArtifactID resolves the id a given (original id, task, +// occurrence) triple registered under — the render side uses it so the +// parent always copies an id artifact_read can actually resolve (D1 +// provenance). The occurrence index disambiguates same-stem files within +// one task (report.md + report.txt both report id "report"); callers pass +// the ref's position among same-id refs of that task's envelope. +func lookupEffectiveArtifactID(origID string, taskIdx int, occ int) (string, bool) { artifactRegistry.mu.Lock() defer artifactRegistry.mu.Unlock() - id, ok := artifactRegistry.byOrig[origKey{origID: origID, taskIdx: taskIdx}] + id, ok := artifactRegistry.byOrig[origKey{origID: origID, taskIdx: taskIdx, occ: occ}] return id, ok } diff --git a/cmd/odek/subagent_delivery_test.go b/cmd/odek/subagent_delivery_test.go index 9c003ac..34f52de 100644 --- a/cmd/odek/subagent_delivery_test.go +++ b/cmd/odek/subagent_delivery_test.go @@ -273,13 +273,13 @@ func TestLookupEffectiveArtifactID(t *testing.T) { r2 := refForFile(t, dir, "dup2.md", "y") r2.ID = "dup" alias, _ := registerSubagentArtifact(artifactEntry{Ref: r2, Path: strings.TrimPrefix(r2.URI, "file://"), TaskIdx: 1}) - if eff, ok := lookupEffectiveArtifactID("dup", 0); !ok || eff != "dup" { + if eff, ok := lookupEffectiveArtifactID("dup", 0, 0); !ok || eff != "dup" { t.Errorf("task 0 effective id: (%q, %v)", eff, ok) } - if eff, ok := lookupEffectiveArtifactID("dup", 1); !ok || eff != alias { + if eff, ok := lookupEffectiveArtifactID("dup", 1, 0); !ok || eff != alias { t.Errorf("task 1 effective id: (%q, %v), want alias %q", eff, ok, alias) } - if _, ok := lookupEffectiveArtifactID("missing", 3); ok { + if _, ok := lookupEffectiveArtifactID("missing", 3, 0); ok { t.Error("unknown orig id must miss") } } @@ -312,17 +312,73 @@ func TestFormatTaskResult_ArtifactProvenanceLine(t *testing.T) { } registerTaskArtifacts(fmt.Sprintf(`{"status":"success","artifacts":[%s]}`, refJSON(p1, c1)), dir, 0) raw2 := fmt.Sprintf(`{"status":"success","artifacts":[%s]}`, refJSON(p2, c2)) + // Production order (judge P1): register first, THEN render — the render + // must resolve the aliased id through the registry. registerTaskArtifacts(raw2, dir, 1) got := formatTaskResultDetailed(raw2, 1, true, dir) - if !strings.Contains(got, "report") && !strings.Contains(got, "dup.t2") { - t.Errorf("render must show the effective (aliased) id:\n%s", got) + if !strings.Contains(got, "- dup.t2 (") { + t.Errorf("render must advertise the effective (aliased) id:\n%s", got) + } + if strings.Contains(got, "- dup (") { + t.Errorf("plain id must not be advertised for an aliased duplicate — artifact_read(plain) resolves to task 1's bytes:\n%s", got) } if !strings.Contains(got, "task 2") { t.Errorf("artifact line missing owning-task provenance:\n%s", got) } } +func TestRegisterArtifact_SameTaskStemCollision(t *testing.T) { + resetArtifactRegistryForTest() + dir := t.TempDir() + // report.md + report.txt in ONE task both report id "report" (filename + // stems); occurrence indexing must keep both resolvable. + r0 := refForFile(t, dir, "report.md", "markdown body") + r1 := refForFile(t, dir, "report.txt", "text body") + r1.ID = "report" + id0, dup0 := registerSubagentArtifact(artifactEntry{Ref: r0, Path: strings.TrimPrefix(r0.URI, "file://"), TaskIdx: 0}) + id1, dup1 := registerSubagentArtifact(artifactEntry{Ref: r1, Path: strings.TrimPrefix(r1.URI, "file://"), TaskIdx: 0}) + if dup0 || id0 != "report" { + t.Errorf("first occurrence: got (%q, %v)", id0, dup0) + } + if !dup1 || id1 != "report.t1" { + t.Errorf("second occurrence must alias to report.t1, got (%q, %v)", id1, dup1) + } + if eff, ok := lookupEffectiveArtifactID("report", 0, 0); !ok || eff != "report" { + t.Errorf("occ 0: (%q, %v)", eff, ok) + } + if eff, ok := lookupEffectiveArtifactID("report", 0, 1); !ok || eff != "report.t1" { + t.Errorf("occ 1: (%q, %v)", eff, ok) + } + // The second file's bytes are reachable via its rendered id. + if e, ok := lookupSubagentArtifact("report.t1"); !ok || e.TaskIdx != 0 { + t.Errorf("aliased same-task entry: (%+v, %v)", e, ok) + } +} + +func TestEvictionCleansEffectiveIDMap(t *testing.T) { + resetArtifactRegistryForTest() + dir := t.TempDir() + r := refForFile(t, dir, "old.md", "oldest") + registerSubagentArtifact(artifactEntry{Ref: r, Path: strings.TrimPrefix(r.URI, "file://"), TaskIdx: 0}) + if _, ok := lookupEffectiveArtifactID("old", 0, 0); !ok { + t.Fatal("mapping must exist before eviction") + } + // Push past the cap: the oldest entry (and its byOrig slot) must go. + for i := 0; i < artifactRegistryCap+10; i++ { + e := artifactEntry{TaskIdx: i} + e.Ref = artifact.Ref{ID: fmt.Sprintf("fill-%03d", i)} + e.Path = "/tmp/nonexistent" + registerSubagentArtifact(e) + } + if _, ok := lookupSubagentArtifact("old"); ok { + t.Fatal("entry should have been evicted") + } + if _, ok := lookupEffectiveArtifactID("old", 0, 0); ok { + t.Error("byOrig slot leaked past eviction — stale alias ids would render dead ends") + } +} + // ── F: inline byte budget (largest-first) ───────────────────────────── func TestRenderArtifacts_InlineBudget(t *testing.T) { diff --git a/cmd/odek/subagent_tool.go b/cmd/odek/subagent_tool.go index 0d4c16b..c4561da 100644 --- a/cmd/odek/subagent_tool.go +++ b/cmd/odek/subagent_tool.go @@ -372,10 +372,15 @@ func (t *delegateTasksTool) Call(args string) (string, error) { buf.WriteString("📋 Sub-agent results:\n\n") for i, r := range results { fmt.Fprintf(&buf, "─── Task %d: %s ───\n", i+1, truncate(input.Tasks[i].Goal, 60)) + // Register BEFORE rendering (judge P1): the render resolves effective + // (aliased) ids through the registry, so an aliased duplicate must + // already be registered or the artifacts line advertises the plain + // id and artifact_read resolves it to the WRONG task's bytes. + notes := registerTaskArtifacts(r, dirs[i], i) buf.WriteString(formatTaskResultDetailed(r, i, t.artifactReadAvailable, dirs[i])) - // M2: validated refs join the session registry so artifact_read can - // resolve them by id later in the turn. - if notes := registerTaskArtifacts(r, dirs[i], i); len(notes) > 0 { + // M2: ambiguity notes render after the artifacts block, same shape as + // the pre-aliasing output. + if len(notes) > 0 { buf.WriteString(strings.Join(notes, "\n") + "\n") } buf.WriteString("\n\n") @@ -781,10 +786,18 @@ func renderArtifacts(refs []artifact.Ref, roots []string, taskIdx int) string { } } + occSeen := map[string]int{} + for i, v := range ok { displayID := v.ref.ID if taskIdx >= 0 { - if eff, found := lookupEffectiveArtifactID(v.ref.ID, taskIdx); found { + // Occurrence index: same-stem refs within one task (report.md + + // report.txt both report id "report") registered distinct byOrig + // slots; this render consumes them in envelope order, matching + // registration order. + occ := occSeen[v.ref.ID] + occSeen[v.ref.ID]++ + if eff, found := lookupEffectiveArtifactID(v.ref.ID, taskIdx, occ); found { displayID = eff } } From 35de1944b4b5805d0ec9a1ee2cb5d8ae89d5ccac Mon Sep 17 00:00:00 2001 From: Rolando Santamaria Maso <4096860+jkyberneees@users.noreply.github.com> Date: Wed, 2 Sep 2026 22:26:50 +0200 Subject: [PATCH 4/4] =?UTF-8?q?fix(subagents):=20key=20artifact=20ids=20by?= =?UTF-8?q?=20per-call=20task=20id=20=E2=80=94=20cross-call=20collision?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Judge pass 2 caught a hazard in the occurrence fix: byOrig/occ were keyed by (origID, taskIdx), but every delegate_tasks call has a task 0 — a later call's duplicate id would register at occ>=1 while its render looked up occ=0 and resolved to an EARLIER call's plain id (silent wrong content, same class as the P1 finding). The registry key is now (origID, taskID, occ) with taskID the per-call unique id minted at spawn; the collate loop threads it into registration and rendering. Adds TestEffectiveIDs_NoCrossCallCollision pinning both calls resolving to their own ids. --- cmd/odek/artifact_read_test.go | 4 +- cmd/odek/artifact_read_toctou_test.go | 2 +- cmd/odek/subagent_artifact_registry.go | 26 +++++----- cmd/odek/subagent_delivery_test.go | 66 ++++++++++++++++++-------- cmd/odek/subagent_tool.go | 24 +++++----- 5 files changed, 77 insertions(+), 45 deletions(-) diff --git a/cmd/odek/artifact_read_test.go b/cmd/odek/artifact_read_test.go index 58b0dbc..cc58b5e 100644 --- a/cmd/odek/artifact_read_test.go +++ b/cmd/odek/artifact_read_test.go @@ -190,10 +190,10 @@ func TestRegisterTaskArtifacts_DuplicateNote(t *testing.T) { raw2 := fmt.Sprintf(`{"status":"success","summary":"ok","artifacts":[{"schema":%q,"id":"dup","uri":"file://%s","media_type":"text/markdown","sha256":%q,"size_bytes":%d}]}`, artifact.SchemaArtifactRef, p2, expectedSHA(t, c2), size2) - if notes := registerTaskArtifacts(raw1, dir, 0); len(notes) != 0 { + if notes := registerTaskArtifacts(raw1, dir, 0, "task-a"); len(notes) != 0 { t.Errorf("first registration must not note: %v", notes) } - notes := registerTaskArtifacts(raw2, dir, 1) + notes := registerTaskArtifacts(raw2, dir, 1, "task-b") if len(notes) != 1 || !strings.Contains(notes[0], "duplicate") { t.Errorf("duplicate must produce a note: %v", notes) } diff --git a/cmd/odek/artifact_read_toctou_test.go b/cmd/odek/artifact_read_toctou_test.go index d8f1e07..2cd109a 100644 --- a/cmd/odek/artifact_read_toctou_test.go +++ b/cmd/odek/artifact_read_toctou_test.go @@ -41,7 +41,7 @@ func registerArtifactForTOCTOU(t *testing.T, id, content string) (root, path str } raw := fmt.Sprintf(`{"status":"success","summary":"ok","artifacts":[{"schema":%q,"id":%q,"uri":"file://%s","media_type":"text/markdown","sha256":%q,"size_bytes":%d}]}`, artifact.SchemaArtifactRef, id, path, expectedSHA(t, content), len(content)) - if notes := registerTaskArtifacts(raw, root, 0); len(notes) != 0 { + if notes := registerTaskArtifacts(raw, root, 0, "task-a"); len(notes) != 0 { t.Fatalf("clean registration must not produce notes: %v", notes) } // The registration helper silently skips validation failures — make diff --git a/cmd/odek/subagent_artifact_registry.go b/cmd/odek/subagent_artifact_registry.go index 59e3358..d0f1489 100644 --- a/cmd/odek/subagent_artifact_registry.go +++ b/cmd/odek/subagent_artifact_registry.go @@ -36,6 +36,7 @@ type artifactEntry struct { Ref artifact.Ref Path string TaskIdx int + TaskID string RegisteredAt time.Time seq uint64 OrigID string // id as the child reported it (pre-alias) @@ -51,9 +52,9 @@ type registrySlot struct { } type origKey struct { - origID string - taskIdx int - occ int // occurrence of origID within the task (same stem, two extensions) + origID string + taskID string // per-call unique task id — two calls' task 0 must never share slots + occ int // occurrence of origID within the task (same stem, two extensions) } var artifactRegistry struct { @@ -115,7 +116,7 @@ func registerSubagentArtifact(e artifactEntry) (string, bool) { e.Ref.ID = alias id = alias } - occBase := origKey{origID: orig, taskIdx: e.TaskIdx} + occBase := origKey{origID: orig, taskID: e.TaskID} key := occBase key.occ = artifactRegistry.occ[occBase] artifactRegistry.occ[occBase]++ @@ -224,13 +225,16 @@ func minActiveMarkLocked() uint64 { // lookupEffectiveArtifactID resolves the id a given (original id, task, // occurrence) triple registered under — the render side uses it so the // parent always copies an id artifact_read can actually resolve (D1 -// provenance). The occurrence index disambiguates same-stem files within -// one task (report.md + report.txt both report id "report"); callers pass -// the ref's position among same-id refs of that task's envelope. -func lookupEffectiveArtifactID(origID string, taskIdx int, occ int) (string, bool) { +// provenance). taskID is the per-call unique task id (NOT the loop index): +// two delegate_tasks calls each have a task 0, and their registries must +// never share slots. The occurrence index disambiguates same-stem files +// within one task (report.md + report.txt both report id "report"); +// callers pass the ref's position among same-id refs of that task's +// envelope. +func lookupEffectiveArtifactID(origID string, taskID string, occ int) (string, bool) { artifactRegistry.mu.Lock() defer artifactRegistry.mu.Unlock() - id, ok := artifactRegistry.byOrig[origKey{origID: origID, taskIdx: taskIdx, occ: occ}] + id, ok := artifactRegistry.byOrig[origKey{origID: origID, taskID: taskID, occ: occ}] return id, ok } @@ -266,7 +270,7 @@ func listSubagentArtifactIDs() ([]string, int) { // child result against the task's dir, returning human-readable note lines // for ambiguities (duplicate ids). Validation failures are silently skipped // — renderArtifacts already flags them in the summary. -func registerTaskArtifacts(raw, dir string, taskIdx int) []string { +func registerTaskArtifacts(raw, dir string, taskIdx int, taskID string) []string { var r subagentResult if err := json.Unmarshal([]byte(raw), &r); err != nil || len(r.Artifacts) == 0 { return nil @@ -277,7 +281,7 @@ func registerTaskArtifacts(raw, dir string, taskIdx int) []string { if err != nil { continue } - id, dup := registerSubagentArtifact(artifactEntry{Ref: ref, Path: path, TaskIdx: taskIdx}) + id, dup := registerSubagentArtifact(artifactEntry{Ref: ref, Path: path, TaskIdx: taskIdx, TaskID: taskID}) if dup { notes = append(notes, fmt.Sprintf("[artifact] duplicate id %q — task %d copy registered as %q", ref.ID, taskIdx+1, id)) } diff --git a/cmd/odek/subagent_delivery_test.go b/cmd/odek/subagent_delivery_test.go index 34f52de..1e56933 100644 --- a/cmd/odek/subagent_delivery_test.go +++ b/cmd/odek/subagent_delivery_test.go @@ -136,7 +136,7 @@ func TestFormatTaskResult_TruncationMarker(t *testing.T) { summary := strings.Repeat("a", 2100) raw := fmt.Sprintf(`{"status":"success","summary":%q,"summary_truncated":true,"summary_runes":3000}`, summary) - got := formatTaskResultDetailed(raw, 0, true) + got := formatTaskResultDetailed(raw, 0, true, "task-a") if !strings.Contains(got, "headline truncated (2048 of 3000 runes shown)") { t.Errorf("marker missing:\n%s", got) } @@ -144,7 +144,7 @@ func TestFormatTaskResult_TruncationMarker(t *testing.T) { t.Errorf("next-action hint missing:\n%s", got) } - got = formatTaskResultDetailed(raw, 0, false) + got = formatTaskResultDetailed(raw, 0, false, "task-a") if strings.Contains(got, "artifact_read") { t.Error("hint must be conditional: mid-tree parents have no artifact_read") } @@ -157,7 +157,7 @@ func TestFormatTaskResult_TruncationMarker(t *testing.T) { // Untruncated results carry no marker. plain := `{"status":"success","summary":"tiny","summary_runes":4}` - if got = formatTaskResultDetailed(plain, 0, true); strings.Contains(got, "headline truncated") { + if got = formatTaskResultDetailed(plain, 0, true, "task-a"); strings.Contains(got, "headline truncated") { t.Errorf("marker must not appear for untruncated summaries:\n%s", got) } } @@ -269,17 +269,17 @@ func TestLookupEffectiveArtifactID(t *testing.T) { dir := t.TempDir() r := refForFile(t, dir, "dup.md", "x") r.ID = "dup" - registerSubagentArtifact(artifactEntry{Ref: r, Path: strings.TrimPrefix(r.URI, "file://"), TaskIdx: 0}) + registerSubagentArtifact(artifactEntry{Ref: r, Path: strings.TrimPrefix(r.URI, "file://"), TaskIdx: 0, TaskID: "task-a"}) r2 := refForFile(t, dir, "dup2.md", "y") r2.ID = "dup" - alias, _ := registerSubagentArtifact(artifactEntry{Ref: r2, Path: strings.TrimPrefix(r2.URI, "file://"), TaskIdx: 1}) - if eff, ok := lookupEffectiveArtifactID("dup", 0, 0); !ok || eff != "dup" { - t.Errorf("task 0 effective id: (%q, %v)", eff, ok) + alias, _ := registerSubagentArtifact(artifactEntry{Ref: r2, Path: strings.TrimPrefix(r2.URI, "file://"), TaskIdx: 1, TaskID: "task-b"}) + if eff, ok := lookupEffectiveArtifactID("dup", "task-a", 0); !ok || eff != "dup" { + t.Errorf("task a effective id: (%q, %v)", eff, ok) } - if eff, ok := lookupEffectiveArtifactID("dup", 1, 0); !ok || eff != alias { - t.Errorf("task 1 effective id: (%q, %v), want alias %q", eff, ok, alias) + if eff, ok := lookupEffectiveArtifactID("dup", "task-b", 0); !ok || eff != alias { + t.Errorf("task b effective id: (%q, %v), want alias %q", eff, ok, alias) } - if _, ok := lookupEffectiveArtifactID("missing", 3, 0); ok { + if _, ok := lookupEffectiveArtifactID("missing", "task-z", 0); ok { t.Error("unknown orig id must miss") } } @@ -310,13 +310,13 @@ func TestFormatTaskResult_ArtifactProvenanceLine(t *testing.T) { return fmt.Sprintf(`{"schema":%q,"id":"dup","uri":"file://%s","media_type":"text/markdown","sha256":%q,"size_bytes":%d}`, artifact.SchemaArtifactRef, p, hex.EncodeToString(sum[:]), len(content)) } - registerTaskArtifacts(fmt.Sprintf(`{"status":"success","artifacts":[%s]}`, refJSON(p1, c1)), dir, 0) + registerTaskArtifacts(fmt.Sprintf(`{"status":"success","artifacts":[%s]}`, refJSON(p1, c1)), dir, 0, "task-a") raw2 := fmt.Sprintf(`{"status":"success","artifacts":[%s]}`, refJSON(p2, c2)) // Production order (judge P1): register first, THEN render — the render // must resolve the aliased id through the registry. - registerTaskArtifacts(raw2, dir, 1) + registerTaskArtifacts(raw2, dir, 1, "task-b") - got := formatTaskResultDetailed(raw2, 1, true, dir) + got := formatTaskResultDetailed(raw2, 1, true, "task-b", dir) if !strings.Contains(got, "- dup.t2 (") { t.Errorf("render must advertise the effective (aliased) id:\n%s", got) } @@ -336,18 +336,18 @@ func TestRegisterArtifact_SameTaskStemCollision(t *testing.T) { r0 := refForFile(t, dir, "report.md", "markdown body") r1 := refForFile(t, dir, "report.txt", "text body") r1.ID = "report" - id0, dup0 := registerSubagentArtifact(artifactEntry{Ref: r0, Path: strings.TrimPrefix(r0.URI, "file://"), TaskIdx: 0}) - id1, dup1 := registerSubagentArtifact(artifactEntry{Ref: r1, Path: strings.TrimPrefix(r1.URI, "file://"), TaskIdx: 0}) + id0, dup0 := registerSubagentArtifact(artifactEntry{Ref: r0, Path: strings.TrimPrefix(r0.URI, "file://"), TaskIdx: 0, TaskID: "task-a"}) + id1, dup1 := registerSubagentArtifact(artifactEntry{Ref: r1, Path: strings.TrimPrefix(r1.URI, "file://"), TaskIdx: 0, TaskID: "task-a"}) if dup0 || id0 != "report" { t.Errorf("first occurrence: got (%q, %v)", id0, dup0) } if !dup1 || id1 != "report.t1" { t.Errorf("second occurrence must alias to report.t1, got (%q, %v)", id1, dup1) } - if eff, ok := lookupEffectiveArtifactID("report", 0, 0); !ok || eff != "report" { + if eff, ok := lookupEffectiveArtifactID("report", "task-a", 0); !ok || eff != "report" { t.Errorf("occ 0: (%q, %v)", eff, ok) } - if eff, ok := lookupEffectiveArtifactID("report", 0, 1); !ok || eff != "report.t1" { + if eff, ok := lookupEffectiveArtifactID("report", "task-a", 1); !ok || eff != "report.t1" { t.Errorf("occ 1: (%q, %v)", eff, ok) } // The second file's bytes are reachable via its rendered id. @@ -361,7 +361,7 @@ func TestEvictionCleansEffectiveIDMap(t *testing.T) { dir := t.TempDir() r := refForFile(t, dir, "old.md", "oldest") registerSubagentArtifact(artifactEntry{Ref: r, Path: strings.TrimPrefix(r.URI, "file://"), TaskIdx: 0}) - if _, ok := lookupEffectiveArtifactID("old", 0, 0); !ok { + if _, ok := lookupEffectiveArtifactID("old", "", 0); !ok { t.Fatal("mapping must exist before eviction") } // Push past the cap: the oldest entry (and its byOrig slot) must go. @@ -374,11 +374,37 @@ func TestEvictionCleansEffectiveIDMap(t *testing.T) { if _, ok := lookupSubagentArtifact("old"); ok { t.Fatal("entry should have been evicted") } - if _, ok := lookupEffectiveArtifactID("old", 0, 0); ok { + if _, ok := lookupEffectiveArtifactID("old", "", 0); ok { t.Error("byOrig slot leaked past eviction — stale alias ids would render dead ends") } } +func TestEffectiveIDs_NoCrossCallCollision(t *testing.T) { + resetArtifactRegistryForTest() + dir := t.TempDir() + // Two delegate_tasks calls, each with a task 0, both staging report.md. + // The registry key is the per-call unique task id — the loop index + // alone would make call 2's render resolve to call 1's bytes. + r1 := refForFile(t, dir, "report.md", "call one") + r2 := refForFile(t, dir, "report2.md", "call two") + r2.ID = "report" + id1, dup1 := registerSubagentArtifact(artifactEntry{Ref: r1, Path: strings.TrimPrefix(r1.URI, "file://"), TaskIdx: 0, TaskID: "call-1-task-0"}) + id2, dup2 := registerSubagentArtifact(artifactEntry{Ref: r2, Path: strings.TrimPrefix(r2.URI, "file://"), TaskIdx: 0, TaskID: "call-2-task-0"}) + if dup1 || id1 != "report" { + t.Fatalf("call 1: got (%q, %v)", id1, dup1) + } + if !dup2 || id2 != "report.t1" { + t.Fatalf("call 2 must alias, got (%q, %v)", id2, dup2) + } + // Each call's render must advertise ITS OWN id. + if eff, ok := lookupEffectiveArtifactID("report", "call-1-task-0", 0); !ok || eff != "report" { + t.Errorf("call 1 effective id: (%q, %v)", eff, ok) + } + if eff, ok := lookupEffectiveArtifactID("report", "call-2-task-0", 0); !ok || eff != "report.t1" { + t.Errorf("call 2 effective id: (%q, %v), want its own alias", eff, ok) + } +} + // ── F: inline byte budget (largest-first) ───────────────────────────── func TestRenderArtifacts_InlineBudget(t *testing.T) { @@ -396,7 +422,7 @@ func TestRenderArtifacts_InlineBudget(t *testing.T) { } raw := fmt.Sprintf(`{"status":"success","artifacts":[%s]}`, strings.Join(refs, ",")) - got := formatTaskResultDetailed(raw, 0, true, dir) + got := formatTaskResultDetailed(raw, 0, true, "task-a", dir) blocks := strings.Count(got, "--- artifact:") // Budget 128 KiB, largest-first: 30+29+28+27 = 114 KiB inlined; the // 26 KiB artifact would push past the budget → metadata line only. diff --git a/cmd/odek/subagent_tool.go b/cmd/odek/subagent_tool.go index c4561da..6564a70 100644 --- a/cmd/odek/subagent_tool.go +++ b/cmd/odek/subagent_tool.go @@ -295,7 +295,8 @@ func (t *delegateTasksTool) Call(args string) (string, error) { // a private per-call semaphore. Capacity < 1 is normalized to 1: an // unbuffered channel would deadlock the acquire-before-spawn loop. results := make([]string, len(input.Tasks)) - dirs := make([]string, len(input.Tasks)) // per-task artifact dirs (parent-created) + dirs := make([]string, len(input.Tasks)) // per-task artifact dirs (parent-created) + taskIDs := make([]string, len(input.Tasks)) // per-task unique ids (artifact registry keys) sem := t.concurrencySem() var mu sync.Mutex var wg sync.WaitGroup @@ -308,6 +309,7 @@ func (t *delegateTasksTool) Call(args string) (string, error) { // artifact dir can be created serially and correlated with the id // the child echoes on every telemetry record. taskID := newTaskID() + taskIDs[i] = taskID // Wire v2 (P2): record + emit the queued phase BEFORE acquiring a // limiter slot, so clients see every accepted task immediately — // including the ones still waiting for a concurrency slot. @@ -376,8 +378,8 @@ func (t *delegateTasksTool) Call(args string) (string, error) { // (aliased) ids through the registry, so an aliased duplicate must // already be registered or the artifacts line advertises the plain // id and artifact_read resolves it to the WRONG task's bytes. - notes := registerTaskArtifacts(r, dirs[i], i) - buf.WriteString(formatTaskResultDetailed(r, i, t.artifactReadAvailable, dirs[i])) + notes := registerTaskArtifacts(r, dirs[i], i, taskIDs[i]) + buf.WriteString(formatTaskResultDetailed(r, i, t.artifactReadAvailable, taskIDs[i], dirs[i])) // M2: ambiguity notes render after the artifacts block, same shape as // the pre-aliasing output. if len(notes) > 0 { @@ -637,13 +639,13 @@ const ( // (metadata-only line; small text artifacts inlined). No roots ⇒ every ref // is rejected — a lost root can never become a trust upgrade. func formatTaskResult(raw string, artifactRoots ...string) string { - return formatTaskResultDetailed(raw, -1, true, artifactRoots...) + return formatTaskResultDetailed(raw, -1, true, "", artifactRoots...) } // formatTaskResultDetailed renders one child's framed result as compact text -// for the parent's context. taskIdx (0-based) drives artifact-id -// provenance: ids render under their EFFECTIVE registered id (D1 aliasing) -// plus the owning task number; pass -1 when the task index is unknown. +// for the parent's context. taskIdx (0-based) drives the task-provenance +// label; taskID (per-call unique) keys the effective-id registry lookups — +// pass "" when unknown (lookups then miss and refs render as reported). // artifactReadAvailable gates the truncation marker's next-action hint — // mid-tree parents have no artifact_read (R2-3), so pointing them at it // would send them chasing a tool they don't have. Parsed envelopes render @@ -656,7 +658,7 @@ func formatTaskResult(raw string, artifactRoots ...string) string { // every incoming ref is validated fail-closed against them before render // (metadata-only line; small text artifacts inlined). No roots ⇒ every ref // is rejected — a lost root can never become a trust upgrade. -func formatTaskResultDetailed(raw string, taskIdx int, artifactReadAvailable bool, artifactRoots ...string) string { +func formatTaskResultDetailed(raw string, taskIdx int, artifactReadAvailable bool, taskID string, artifactRoots ...string) string { var r subagentResult if err := json.Unmarshal([]byte(raw), &r); err != nil { if len(raw) > maxSubagentSummaryResultBytes { @@ -723,7 +725,7 @@ func formatTaskResultDetailed(raw string, taskIdx int, artifactReadAvailable boo } fmt.Fprintf(&b, "denials (%d of %d): %s\n", len(shown), total, strings.Join(parts, "; ")) } - b.WriteString(renderArtifacts(r.Artifacts, artifactRoots, taskIdx)) + b.WriteString(renderArtifacts(r.Artifacts, artifactRoots, taskIdx, taskID)) return b.String() } @@ -740,7 +742,7 @@ const maxInlinePerCallBytes = 128 << 10 // 128 KiB // resolved, size+sha256 verified) within the per-call inline budget, // largest-first. Invalid refs are dropped with a flag — never fatal to the // summary. Raw absolute paths are never rendered. -func renderArtifacts(refs []artifact.Ref, roots []string, taskIdx int) string { +func renderArtifacts(refs []artifact.Ref, roots []string, taskIdx int, taskID string) string { if len(refs) == 0 { return "" } @@ -797,7 +799,7 @@ func renderArtifacts(refs []artifact.Ref, roots []string, taskIdx int) string { // registration order. occ := occSeen[v.ref.ID] occSeen[v.ref.ID]++ - if eff, found := lookupEffectiveArtifactID(v.ref.ID, taskIdx, occ); found { + if eff, found := lookupEffectiveArtifactID(v.ref.ID, taskID, occ); found { displayID = eff } }