Skip to content

Commit fd7b195

Browse files
committed
fix: batch 2 — resolve 9 more verified bugs (RED/GREEN)
Validated with failing RED tests (previous commit) that now pass. Full suite, -race on changed packages, golangci-lint all green. cmd/odek: - shell: the sandbox kill follow-up now runs under a 10s deadline — a hung Docker daemon can no longer wedge the tool call forever after its own timeout fired - serve_api: /api/sessions search scans the whole index before paginating; matches deeper in recency order than limit+offset were silently dropped - serve: prompt-cancel registrations are generation-guarded — when two prompts run on one session, the first finisher no longer deletes the live prompt's cancel func (registerPromptCancel returns an unregister closure; legacy helper kept) - repl: tab-completion list is now the single source of truth synced with handleREPLCommand (/sandbox /model /session were advertised but unknown) - subagent: task errors exit 1 and timeouts exit 2 per docs/EXTENSIONS.md; previously every outcome exited 0 after startup succeeded (typed subagentRunError mapped in dispatch; failure line replaces the ✅ banner) internal/loop: - skill/episode/extended-memory blocks are injected right BEFORE the latest user message as documented; the old scan skipped index 0 and appended after it. headLen gains ctxLeadDroppableFrom so injected blocks stay trimmable (oversized injection still dropped pre-call, pinned by TestTrimContext_PostInjectionBudget) while base system + memory remain protected for prompt caching - emitSignal serializes handler invocation: parallel tool heartbeats fire from separate goroutines, and SignalHandler consumers are only promised non-blocking, never concurrent-safe internal/render: - FirstSentence picks the EARLIEST sentence boundary instead of iterating separator types ('Done! Next step.' returned the second sentence) internal/telegram: - GetOrCreate evicts an expired cache entry before Load; TTL expiry was dead code because Load returned the same stale pointer unchecked Hardening (no dedicated RED): - events/jsonl: OpenJSONLSink opens with O_NOFOLLOW, closing the Lstat-then-open symlink swap race - mcpclient: child stderr is actually inherited (os.Stderr); nil connects to /dev/null, contradicting the comment and hiding server diagnostics Investigated and rejected: base64 decode-of-inline-string returning unwrapped content — decode input is model-supplied tool args, not fresh external data; there is also no decode-from-path feature to wrap.
1 parent 7c260f5 commit fd7b195

14 files changed

Lines changed: 294 additions & 98 deletions

File tree

‎cmd/odek/dispatch.go‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package main
22

33
import (
44
"encoding/json"
5+
"errors"
56
"fmt"
67
"os"
78
"runtime"
@@ -95,12 +96,21 @@ func runExit(err error) int {
9596

9697
// subagentExit honours the sub-agent JSON contract: stderr gets the
9798
// human-readable line, stdout gets a JSON envelope the parent can parse,
98-
// and the exit code is 3 (reserved for setup/contract errors so the
99-
// parent can distinguish them from task-level failures).
99+
// and exit codes follow docs/EXTENSIONS.md: 0 success, 1 task error,
100+
// 2 timeout, 3 setup error. Task errors and timeouts arrive as
101+
// *subagentRunError with their envelope already printed, so they only map
102+
// to an exit code here.
100103
func subagentExit(err error) int {
101104
if err == nil {
102105
return 0
103106
}
107+
var runErr *subagentRunError
108+
if errors.As(err, &runErr) {
109+
if runErr.timeout {
110+
return 2
111+
}
112+
return 1
113+
}
104114
fmt.Fprintf(os.Stderr, "odek: %v\n", err)
105115
_ = json.NewEncoder(os.Stdout).Encode(subagentResult{
106116
Status: "error",

‎cmd/odek/redbugs2_test.go‎

Lines changed: 36 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
package main
22

33
import (
4-
"context"
54
"fmt"
65
"net/http"
76
"net/http/httptest"
@@ -126,10 +125,10 @@ func TestRED_SessionSearchSearchesWholeStore(t *testing.T) {
126125
// func, making /api/cancel a silent no-op while the newer prompt runs.
127126
func TestRED_PromptCancelSurvivesEarlierPromptFinishing(t *testing.T) {
128127
calledSecond := false
129-
registerPromptCancel("red-b3-sess", func() {}) // prompt 1 starts
128+
unregisterFirst := registerPromptCancel("red-b3-sess", func() {}) // prompt 1 starts
130129
registerPromptCancel("red-b3-sess", func() { calledSecond = true }) // prompt 2 starts
131130

132-
unregisterPromptCancel("red-b3-sess") // prompt 1 finishes first
131+
unregisterFirst() // prompt 1 finishes first — must remove ONLY its own registration
133132

134133
if !cancelPrompt("red-b3-sess") {
135134
t.Fatal("cancelPrompt found no registration after the earlier prompt finished — the second prompt can no longer be cancelled")
@@ -144,13 +143,14 @@ func TestRED_PromptCancelSurvivesEarlierPromptFinishing(t *testing.T) {
144143
}
145144

146145
// ────────────────────────────────────────────────────────────────────────
147-
// RED #B6 (M6): The REPL editor advertises /sandbox /model /session via
148-
// tab-completion, but handleREPLCommand doesn't implement them — completing
149-
// then pressing enter yields "Unknown command".
146+
// RED #B6 (M6): The REPL editor advertises slash commands via
147+
// tab-completion that handleREPLCommand doesn't implement — completing one
148+
// and pressing enter yields "Unknown command". Every advertised command
149+
// must be implemented.
150150
func TestRED_REPLCompletionsAreImplemented(t *testing.T) {
151-
advertised := []string{
152-
"/exit", "/quit", "/help", "/info",
153-
"/sandbox", "/model", "/session",
151+
advertised := replCommands
152+
if len(advertised) == 0 {
153+
t.Fatal("replCommands is empty")
154154
}
155155
sess := &session.Session{ID: "red-b6"}
156156

@@ -192,21 +192,34 @@ func llmMessage(content string) []llm.Message {
192192
return []llm.Message{{Role: "user", Content: content}}
193193
}
194194

195-
var _ = context.Background // keep context imported for future cases
196-
197195
// ────────────────────────────────────────────────────────────────────────
198-
// RED #B9 (T5): base64 decode-from-file returns raw file bytes to the
199-
// model OUTSIDE the <untrusted_content_*> wrapper, while every other
200-
// externally-sourced tool result is wrapped — a hole in the untrusted-
201-
// content boundary (SECURITY.md invariant).
202-
func TestRED_Base64DecodeFromFileIsWrapped(t *testing.T) {
203-
dir := t.TempDir()
204-
p := filepath.Join(dir, "blob.txt")
205-
os.WriteFile(p, []byte("external file payload"), 0o644)
206196

207-
tool := &base64Tool{}
208-
res := callJSON(t, tool, fmt.Sprintf(`{"path":%q,"decode":true}`, p))
209-
if !strings.Contains(res, "untrusted_content_") {
210-
t.Errorf("decoded file content returned unwrapped: %s", res)
197+
// Regression #M4: sub-agent exit codes. docs/EXTENSIONS.md pins
198+
// 0=success, 1=task error, 2=timeout, 3=setup error. Task errors and
199+
// timeouts previously returned nil from subagentCmd — every run exited 0.
200+
func TestRED_SubagentExitCodeContract(t *testing.T) {
201+
if got := subagentExit(nil); got != 0 {
202+
t.Errorf("subagentExit(nil) = %d, want 0", got)
203+
}
204+
if got := subagentExit(&subagentRunError{timeout: true}); got != 2 {
205+
t.Errorf("subagentExit(timeout) = %d, want 2", got)
206+
}
207+
if got := subagentExit(&subagentRunError{}); got != 1 {
208+
t.Errorf("subagentExit(task error) = %d, want 1", got)
209+
}
210+
// Setup errors keep their envelope-printing behavior and exit 3.
211+
oldOut, oldErr := os.Stdout, os.Stderr
212+
r, w, _ := os.Pipe()
213+
os.Stdout, os.Stderr = w, w
214+
got := subagentExit(fmt.Errorf("bad flags"))
215+
os.Stdout, os.Stderr = oldOut, oldErr
216+
w.Close()
217+
buf := make([]byte, 1024)
218+
n, _ := r.Read(buf)
219+
if got != 3 {
220+
t.Errorf("subagentExit(setup error) = %d, want 3", got)
221+
}
222+
if !strings.Contains(string(buf[:n]), "bad flags") {
223+
t.Errorf("setup error envelope missing on stdout/stderr: %q", string(buf[:n]))
211224
}
212225
}

‎cmd/odek/repl.go‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -228,13 +228,12 @@ func replCmd(args []string) error {
228228
// first turn after resuming with `odek repl --id <session>`.
229229
resumedSession := sessionID != ""
230230

231-
// Line editor with history and tab completion for slash commands
231+
// Line editor with history and tab completion for slash commands.
232+
// Keep this list in sync with handleREPLCommand — completing a command
233+
// that isn't implemented yields "Unknown command".
232234
editor := newReplEditor(
233235
fmt.Sprintf("odek %d> ", turn+1),
234-
[]string{
235-
"/exit", "/quit", "/help", "/info",
236-
"/sandbox", "/model", "/session",
237-
},
236+
replCommands,
238237
)
239238
editor.history.Load(filepath.Join(odekDir(), historyFilename))
240239
for {
@@ -344,6 +343,13 @@ func replCmd(args []string) error {
344343
return nil
345344
}
346345

346+
// replCommands lists the slash commands the REPL implements (tab
347+
// completion + docs source of truth). Only commands handleREPLCommand
348+
// actually handles may appear here.
349+
var replCommands = []string{
350+
"/exit", "/quit", "/help", "/info",
351+
}
352+
347353
// handleREPLCommand processes a REPL slash command.
348354
// Returns true if the session should exit.
349355
func handleREPLCommand(input string, sess *session.Session) bool {

‎cmd/odek/serve.go‎

Lines changed: 41 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -104,23 +104,45 @@ var wsUpgradeLimiter = newRateLimiter(30, time.Minute)
104104
// WebSocket handlers and the HTTP /api/cancel endpoint can access it safely.
105105
// Using session IDs as keys scopes cancellation to the caller's session,
106106
// preventing one connection from cancelling another connection's prompt.
107+
// promptCancelEntry pairs a cancel func with a generation counter so an
108+
// earlier prompt's unregister cannot delete a newer prompt's registration.
109+
type promptCancelEntry struct {
110+
cancel context.CancelFunc
111+
gen int64
112+
}
113+
107114
var (
108-
promptCancelMu sync.Mutex
109-
promptCancels = map[string]context.CancelFunc{}
115+
promptCancelMu sync.Mutex
116+
promptCancels = map[string]*promptCancelEntry{}
117+
promptCancelGen int64
110118
)
111119

112120
// registerPromptCancel records cancel as the active cancel function for
113-
// sessionID. It must be unregistered when the prompt completes.
114-
func registerPromptCancel(sessionID string, cancel context.CancelFunc) {
121+
// sessionID. The returned unregister func removes it ONLY if it is still
122+
// the live registration — when two prompts run on the same session, the
123+
// first finisher must not strip the second's cancel func.
124+
func registerPromptCancel(sessionID string, cancel context.CancelFunc) (unregister func()) {
115125
if sessionID == "" || cancel == nil {
116-
return
126+
return func() {}
117127
}
128+
gen := atomic.AddInt64(&promptCancelGen, 1)
118129
promptCancelMu.Lock()
119-
promptCancels[sessionID] = cancel
130+
promptCancels[sessionID] = &promptCancelEntry{cancel: cancel, gen: gen}
120131
promptCancelMu.Unlock()
132+
133+
return func() {
134+
promptCancelMu.Lock()
135+
if cur, ok := promptCancels[sessionID]; ok && cur.gen == gen {
136+
delete(promptCancels, sessionID)
137+
}
138+
promptCancelMu.Unlock()
139+
}
121140
}
122141

123-
// unregisterPromptCancel removes any cancel function registered for sessionID.
142+
// unregisterPromptCancel removes whatever cancel function is currently
143+
// registered for sessionID. Prefer the unregister closure returned by
144+
// registerPromptCancel; this variant is kept for callers that don't track
145+
// their registration generation.
124146
func unregisterPromptCancel(sessionID string) {
125147
if sessionID == "" {
126148
return
@@ -137,11 +159,18 @@ func cancelPrompt(sessionID string) bool {
137159
return false
138160
}
139161
promptCancelMu.Lock()
140-
cancel, ok := promptCancels[sessionID]
162+
entry, ok := promptCancels[sessionID]
141163
promptCancelMu.Unlock()
142164
if !ok {
143165
return false
144166
}
167+
var cancel context.CancelFunc
168+
if entry != nil {
169+
cancel = entry.cancel
170+
}
171+
if cancel == nil {
172+
return false
173+
}
145174
cancel()
146175
return true
147176
}
@@ -1468,10 +1497,11 @@ func handlePrompt(
14681497
authToken = sess.AuthToken
14691498
}
14701499
// Register the cancel function for this session so the HTTP endpoint can
1471-
// abort this specific prompt. Unregister as soon as the run finishes.
1500+
// abort this specific prompt. The generation-guarded unregister only
1501+
// removes OUR registration — a concurrent newer prompt on the same
1502+
// session keeps its own cancel func when we finish first.
14721503
if sid != "" && promptCancel != nil {
1473-
registerPromptCancel(sid, promptCancel)
1474-
defer unregisterPromptCancel(sid)
1504+
defer registerPromptCancel(sid, promptCancel)()
14751505
}
14761506
send(map[string]any{"type": "session", "session_id": sid, "auth_token": authToken, "model": resolved.Model, "sandbox": resolved.Sandbox})
14771507

‎cmd/odek/serve_api.go‎

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -109,9 +109,23 @@ func handleSessionListPaged(store *session.Store) http.HandlerFunc {
109109
offset = 0
110110
}
111111

112-
// Fetch enough to cover the requested window before slicing.
113-
sessions, err := store.List(limit + offset)
114-
if err != nil {
112+
// Fetch enough to cover the requested window before slicing. With a
113+
// search query the window cannot be known up front: matches may sit
114+
// arbitrarily deep in recency order, so scan the whole store and
115+
// filter before paginating (List is an index read — cheap).
116+
var sessions []session.Session
117+
if q != "" {
118+
all, err := store.List(0)
119+
if err == nil {
120+
sessions = all
121+
}
122+
} else {
123+
s, err := store.List(limit + offset)
124+
if err == nil {
125+
sessions = s
126+
}
127+
}
128+
if sessions == nil {
115129
sessions = []session.Session{}
116130
}
117131

‎cmd/odek/shell.go‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -359,13 +359,21 @@ var sandboxCmdSeq atomic.Uint64
359359
// this is best-effort, not a hard guarantee. The command string travels as
360360
// a positional argument ($1), never interpolated into the wrapper, so
361361
// quoting cannot break out of it.
362+
// sandboxKillFollowupTimeout bounds the in-container kill follow-up. It
363+
// runs synchronously after the command's own timeout/cancel already fired;
364+
// without a deadline a hung Docker daemon would wedge the tool call forever
365+
// after its timeout — exactly what the timeout exists to prevent.
366+
const sandboxKillFollowupTimeout = 10 * time.Second
367+
362368
func wrapSandboxCommand(containerName, command string) (argv []string, followUp func()) {
363369
pidFile := fmt.Sprintf("/tmp/.odek-cmd-%d-%d.pid", os.Getpid(), sandboxCmdSeq.Add(1))
364370
wrapper := "echo $$ > " + pidFile + "; sh -c \"$1\"; rc=$?; rm -f " + pidFile + "; exit $rc"
365371
argv = []string{"exec", "-w", "/workspace", containerName, "sh", "-c", wrapper, "odek-cmd", command}
366372
followUp = func() {
367373
// Best-effort: the container may already be gone (session cleanup).
368-
_ = exec.Command("docker", "exec", containerName, "sh", "-c",
374+
ctx, cancel := context.WithTimeout(context.Background(), sandboxKillFollowupTimeout)
375+
defer cancel()
376+
_ = exec.CommandContext(ctx, "docker", "exec", containerName, "sh", "-c",
369377
"kill -KILL -$(cat "+pidFile+") 2>/dev/null; rm -f "+pidFile).Run()
370378
}
371379
return argv, followUp

‎cmd/odek/subagent.go‎

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -495,7 +495,8 @@ func subagentCmd(args []string) error {
495495
}
496496

497497
if err != nil {
498-
if sigCtx.Err() != nil {
498+
timedOut := sigCtx.Err() != nil
499+
if timedOut {
499500
result.Status = "error"
500501
result.Error = fmt.Sprintf("timeout after %ds", cfg.timeout)
501502
} else {
@@ -508,11 +509,15 @@ func subagentCmd(args []string) error {
508509
// Extract files changed from tool calls
509510
result.FilesChanged = extractFilesChanged(allMessages)
510511

511-
// Output JSON to stdout
512+
// Output JSON to stdout — the envelope is emitted exactly once, here.
512513
enc := json.NewEncoder(os.Stdout)
513514
enc.SetIndent("", "")
514515
enc.Encode(result)
515516

517+
if result.Status != "success" {
518+
fmt.Fprintf(os.Stderr, "✗ Sub-agent failed: %s\n", result.Error)
519+
return &subagentRunError{timeout: err != nil && sigCtx.Err() != nil}
520+
}
516521
if !cfg.quiet {
517522
fmt.Fprintf(os.Stderr, "✅ Sub-agent complete: %.1fs, %d tokens, %d iterations\n",
518523
latency.Seconds(), tokensUsed, iterations)
@@ -521,6 +526,19 @@ func subagentCmd(args []string) error {
521526
return nil
522527
}
523528

529+
// subagentRunError reports a task-level failure AFTER the JSON result
530+
// envelope has already been written by subagentCmd. dispatch maps it to the
531+
// documented exit codes: 2 for timeouts, 1 for other task errors (0 =
532+
// success, 3 = setup errors, which still travel as plain errors).
533+
type subagentRunError struct{ timeout bool }
534+
535+
func (e *subagentRunError) Error() string {
536+
if e.timeout {
537+
return "sub-agent timed out"
538+
}
539+
return "sub-agent task failed"
540+
}
541+
524542
// ── Helpers ───────────────────────────────────────────────────────────
525543

526544
func extractSummary(messages []llm.Message) string {

‎internal/events/jsonl.go‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"os"
77
"path/filepath"
88
"sync"
9+
"syscall"
910
"time"
1011
)
1112

@@ -37,11 +38,13 @@ func OpenJSONLSink(path string) (*JSONLSink, error) {
3738
}
3839
// Refuse to follow a symlink at the target path — an attacker who can
3940
// plant a symlink could otherwise redirect the event stream (which may
40-
// contain session IDs and token counts) over an arbitrary file.
41+
// contain session IDs and token counts) over an arbitrary file. The
42+
// Lstat pre-check rejects an existing symlink; O_NOFOLLOW closes the
43+
// check-then-open race where the symlink is swapped in between.
4144
if fi, err := os.Lstat(path); err == nil && fi.Mode()&os.ModeSymlink != 0 {
4245
return nil, fmt.Errorf("refusing to write events to symlink %s", path)
4346
}
44-
f, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_APPEND, 0o600)
47+
f, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_APPEND|syscall.O_NOFOLLOW, 0o600)
4548
if err != nil {
4649
return nil, err
4750
}

0 commit comments

Comments
 (0)