feat: hermes agent autonomy — skill evolve + memory extract + agent configure + watcher

[TakumaLee]

Summary

Port hermes-agent-self-evolution capabilities into Tetora as native Go. Three layers of implementation.

Layer 1 — Skill Auto-Evolution + Deep Memory Extraction

• internal/skill/evolve.go: scans skills with failure rate ≥ 40% (min 10 invocations, 7-day cooldown), calls Haiku LLM to generate rewrite proposals → skills/<name>/proposals/<ts>.md. Human approves/rejects via tetora skill evolve approve|reject|list.
• internal/memory/extract.go: after reflection score ≥ 4 + cost ≥ $0.10 + success, Haiku extracts cross-session knowledge. CRUD-safe writes (ADD/UPDATE/NOOP/CONFLICT) to memory/extract:<slug>.md + FIFO log in memory/auto-extracts.md (cap 100).
• ~/.tetora/workspace/rules/tetora-agent-io-protocol.md: formal interface contract defining how agents subscribe to memory, read skill failures, and trigger self-improvement.

Layer 2 — Agent Configure (tetora agent configure)

• Asks the agent itself (via claude -p) which capabilities it wants, parses JSON response.
• Generates ~/.tetora/agents/<name>/CLAUDE.md (@SOUL.md + @capabilities/index.md + @io-protocol) and capabilities/index.md (compact, always-in-context list).
• Generates per-capability detail files (loaded by agent on demand via Read tool, never pre-injected).
• 4 capabilities: memory.auto-extracts, skill-evolve, weekly-review (adds cron job), deep-memory-extract (sets deepMemoryExtract.enabled in config).

Layer 3 — Task Watcher (tetora agent watch)

• Polls taskboard for tasks with status=todo + non-empty assignee.
• Spawns claude --cwd ~/.tetora/agents/<name>/ -p "[task]" — no SOUL.md injection, agent self-initializes via its own CLAUDE.md.
• Optimistic lock: marks task doing before spawn, rolls back to todo if claude fails to start.
• --daemon flag detaches with PID file + log.

Bug fix

• expandPrompt memory regex now supports hyphens and colons: {{memory.auto-extracts}} and {{memory.extract:slug}} expand correctly.

Architecture principle

Tetora = infrastructure (stores, provides). Agent = autonomous executor (decides what to read, when to act). Dispatch = pure routing {task} → {agent}, no SOUL.md injection.

Test plan

• go test ./internal/skill/... ./internal/memory/... ./internal/agent/... — all pass
• tetora agent configure <name> — generates CLAUDE.md + capabilities/ in agent dir
• tetora agent configure --all — runs for all registered agents, continues on failure
• tetora agent watch — spawns claude for assigned todo tasks
• {{memory.auto-extracts}} expands in task prompts
• tetora skill evolve list|approve|reject CLI works

🤖 Generated with Claude Code

[TakumaLee]

The diff is already provided in the conversation context. Writing the review based on the full diff analysis.

---

PR #111 Code Review — feat: hermes agent autonomy

Scope: internal/skill/evolve.go, internal/memory/extract.go, internal/agent/configure.go, internal/agent/watcher.go, CLI wiring, cron engine hooks, config types.

---

Overview

This PR ports three layers of agent self-improvement into Tetora's Go core: (1) skill auto-evolution via LLM proposals, (2) deep memory extraction after high-quality tasks, and (3) agent configure + task watcher for autonomous spawning. The architecture is well-thought-out and the test coverage is reasonable. However, there are several correctness bugs and a security issue that must be fixed before merge.

---

Risks

🔴 Critical

[watcher.go] Path traversal via t.Assignee

agentDir := filepath.Join(cfg.AgentsDir, t.Assignee)

t.Assignee is read from the database. filepath.Join normalizes but does not prevent path traversal — filepath.Join("/home/user/.tetora/agents", "../../etc") resolves to /home/user/.tetora/etc. Any task inserted with a crafted assignee value (e.g. via the taskboard API) can direct claude to run in an arbitrary directory.

Fix: Validate that the resolved path is still under cfg.AgentsDir:

agentDir := filepath.Join(cfg.AgentsDir, t.Assignee)
if !strings.HasPrefix(filepath.Clean(agentDir)+string(os.PathSeparator), filepath.Clean(cfg.AgentsDir)+string(os.PathSeparator)) {
    return fmt.Errorf("invalid assignee: path escapes agents dir")
}

---

🟠 High

[extract.go:63] ValidateExtract truncation is a no-op — value receiver bug

func ValidateExtract(e Extract) error {   // e is a copy
    if len(e.Body) > 600 {
        e.Body = e.Body[:600]  // modifies the copy, caller sees nothing
    }
    return nil
}

The caller's Extract always gets the full (untruncated) body. Either change the signature to *Extract or remove the silent truncation from a pure validation function (validate-then-reject is clearer than validate-and-mutate).

---

[watcher.go] No concurrency limit in poll

for _, t := range tasks {
    if err := spawnAgent(cfg, t); err != nil { ... }
}

Every pending task spawns a new claude process synchronously. If 50 tasks are queued, 50 claude processes start simultaneously. There is no cap, back-pressure, or semaphore. Add a worker pool or a maxConcurrent config field.

---

[configure.go] jsonObjectRe breaks on nested braces

var jsonObjectRe = regexp.MustCompile(`(?s)\{[^{}]*"selected_capabilities"[^{}]*\}`)

[^{}] rejects any { or } inside the match. Claude often includes JSON examples or code fences in its reason field (e.g. "reason": "I need {memory.auto-extracts}"), which breaks the match entirely and falls back to raw output. Use a proper JSON extractor or at minimum allow nested depth ≥ 1.

---

[watcher.go] markTaskDoing is not atomic

The implementation is SELECT → UPDATE → SELECT-to-verify, three separate DB round-trips. Between the UPDATE and the verification SELECT, another process can claim the task. Use SQLite's changes() function instead:

// In a single exec, capture row-count without a second round-trip.

The WHERE status='todo' guard on the UPDATE is correct, but the verification SELECT is unnecessary overhead and still not linearizable.

---

🟡 Medium

[configure.go] No timeout in askAgent

cmd.Output() blocks forever if claude hangs. A 120-second timeout via exec.CommandContext with a derived context is the minimum acceptable guard.

---

[configure.go] Unvalidated selected_capabilities from LLM

for _, id := range resp.SelectedCapabilities {
    for _, cap := range BuiltinCapabilities {
        if cap.ID != id { continue }
        // ...
    }
}

Unknown IDs are silently ignored. If the LLM hallucinates "memory.deep-extract-v2", no error is produced. Log or return a warning for unrecognized IDs so operators know what the agent requested.

---

[memory/extract.go] AppendToAutoExtractsMD has no file locking

Multiple goroutines calling runDeepMemoryExtract concurrently can interleave their read-modify-write cycles and corrupt auto-extracts.md. Since runDeepMemoryExtract is always launched with go func(), this is an actual race. Use sync.Mutex or flock.

---

[skill/evolve.go] updateProposalStatus silently discards write error

func updateProposalStatus(fpath, status string) {
    // ...
    _ = os.WriteFile(fpath, []byte(content), 0o644)  // error discarded
}

A failed write leaves the proposal in the wrong status with no log entry. Return and propagate the error from ApproveProposal / RejectProposal.

---

[dispatch.go] DailyBudgetUSD defined in config but never enforced

DeepMemoryExtractConfig.DailyBudgetUSD exists but no code in the visible diff tracks cumulative daily spend. Either implement tracking or remove the field to avoid false confidence that spending is capped.

---

[cron/engine.go] "skill_evolve" is a magic string used in three separate places

tick(), RunJobByID(), and StartupReplay() all hardcode "skill_evolve". Extract to a package-level constant.

---

🔵 Low

[skill/evolve.go] Proposal filename collision under concurrent calls

id := time.Now().UTC().Format("2006-01-02T15-04-05")

Second-level granularity means two proposals generated in the same second overwrite each other. Use nanoseconds or a UUID suffix.

---

[skill/evolve.go] computeUnifiedDiff is a line-by-line O(n+m) greedy walk

It produces no LCS, so a single-line insertion will show every subsequent line as changed. This makes proposals hard to read. Consider using an actual Myers diff algorithm or at minimum the go-diff library.

---

[configure.go] @SOUL.md hardcoded in generated CLAUDE.md without existence check

If an agent directory has no SOUL.md (e.g. a new agent not yet initialized), Claude Code will fail to load the file but produce no visible error from configure. Add a warning if SOUL.md is missing.

---

[configure_test.go] contains re-implements strings.Contains needlessly

func contains(s, sub string) bool {
    return len(s) >= len(sub) && (s == sub || len(sub) == 0 ||
        func() bool { ... }())
}

Replace the entire function with strings.Contains. The manual implementation has an off-by-one risk if edge cases diverge from stdlib.

---

[evolve_test.go] mkSkillMD and writeSkillMD are duplicate helpers

evolve_test.go defines mkSkillMD but then calls writeSkillMD (from skill_diagnostics_test.go). mkSkillMD appears unused. Remove it.

---

[tasks/spec-hermes-agent-autonomy.md] Spec file in PR

Including the design spec in the PR diff is fine for traceability, but the file's "Status: Draft" and "Open Questions" table suggest it's not finalized. Recommendation: resolve the open questions (especially Q1 on immediate-mode dispatch and Q3 on daemon process management) before shipping.

---

Suggestions

1. Layer the --daemon stop command: Currently Stop: kill <PID> is printed to stdout, but there's no tetora agent watch --stop command. Add it for UX completeness.
2. Agent configure idempotency: If CLAUDE.md already exists, writeCLAUDEMD overwrites it unconditionally. Consider --force flag or a diff-before-write check to avoid surprising agents mid-session.
3. ParseExtractsJSON fallback ordering: It tries output[start:] (from first {) before trying the array path. If the output is a plain array (starts with [), the JSON unmarshal will fail and only then fall through. Reorder to try array first when [ appears before {.

---

Approve / Request-Changes Recommendation

Request Changes — two issues block merge:

1. Path traversal in watcher.go (Critical): controllable via DB-inserted task data.
2. ValidateExtract value-receiver truncation bug (High): callers never see the capped body, leading to silent oversized memory writes.

The remaining items (concurrency limit, regex robustness, timeout, file locking) should also be addressed but are less likely to cause data loss in the current production configuration. The overall design is solid and the test suite gives good coverage of the happy paths.

[TakumaLee]

lessons.md 讀取完畢，開始對 PR #111 進行 code review。

---

PR #111 Code Review — feat: hermes agent autonomy

Reviewed files: dispatch.go, internal/agent/configure.go, internal/agent/watcher.go, internal/memory/extract.go, internal/skill/evolve.go, internal/config/types.go, internal/cron/engine.go, CLI wiring, tests.

---

🔴 Critical

Nothing new independently found. The path-traversal concern flagged in the PR description is already mitigated by resolveAgentDir. The two-layer defence (strings.ContainsAny(assignee, "/\\") + canonical-path prefix check) is sound. One edge case left unguarded: symlinks. If the agent directory itself is a symlink pointing outside agentsDir, filepath.Abs will follow the symlink's name, not its target, so the prefix check passes while the process spawns inside an unintended directory. Low probability in practice, but worth documenting.

---

🟠 High — Correctness

[watcher.go] markTaskDoing multi-statement query will silently fail on most Go SQLite drivers

rows, err := db.Query(dbPath, fmt.Sprintf(
    `UPDATE tasks SET status = 'doing' ... WHERE id = '%s' AND status = 'todo'; SELECT changes() AS changed;`,
    db.Escape(taskID),
))

Standard database/sql drivers (including mattn/go-sqlite3) do not execute multiple ;-separated statements in a single Query call — they stop after the first statement. The SELECT changes() never runs, rows is empty, and db.Int(rows[0]["changed"]) returns 0 on every call. Result: markTaskDoing always returns (false, nil), so spawnAgent always exits early on the !affected branch and no tasks are ever claimed. The watcher loop becomes a no-op.

Fix: Execute the UPDATE via db.Exec, then issue SELECT changes() in a separate db.Query on the same connection, or use a driver-level LastInsertId/RowsAffected return value.

---

[configure.go] Generated CLAUDE.md hardcodes an absolute, machine-specific ioProtocolPath

func writeCLAUDEMD(agentDir, ioProtocolPath string) error {
    content := fmt.Sprintf("@SOUL.md\n@capabilities/index.md\n@%s\n", ioProtocolPath)
    ...
}

The caller passes filepath.Join(cfg.WorkspaceDir, "rules", "tetora-agent-io-protocol.md"), which resolves to something like /Users/vmgs.takuma/.tetora/workspace/rules/tetora-agent-io-protocol.md. This path is burned into every generated CLAUDE.md. The file will silently fail to load on any other machine or after a workspace relocation.

Fix: Embed a ~-relative path (~/.tetora/workspace/rules/...) or make the path relative to the agent directory. The Claude Code @include syntax supports ~ expansion.

---

[watcher.go] Silent rollback failure leaves tasks permanently stuck in doing

if err := cmd.Start(); err != nil {
    _ = db.Exec(cfg.HistoryDB, fmt.Sprintf(
        `UPDATE tasks SET status = 'todo' ...`, db.Escape(t.ID),
    ))
    return fmt.Errorf("start claude: %w", err)
}

If the db.Exec rollback fails (e.g., DB locked, disk full), the task stays in doing indefinitely and the error is swallowed with _ =. Since the watcher only polls for status = 'todo', the task becomes permanently invisible.

Fix: Log the rollback error. Consider a separate task-recovery sweep for tasks stuck in doing beyond a timeout.

---

🟡 Medium — Design & Resilience

[memory/extract.go] ParseExtractsJSON fails on trailing content

json.Unmarshal in Go rejects trailing non-whitespace after a valid JSON value. If the LLM appends a sentence after the JSON block (e.g. {"extracts": [...]} Done!), both tryArray and tryEnvelope fail and the function returns nil. The extractJSONObject function in configure.go already solves this robustly (balanced-brace scanner). Consider reusing that approach here.

---

[skill/evolve.go] computeUnifiedDiff is a sequential line scan, not LCS

The diff function walks both files in lock-step. For any non-trivially changed skill body (reorder, insert mid-file), it will mark large swaths as - / + even when lines are identical elsewhere. The proposals directory is for human review, so this is cosmetic, but reviewers will find the diff hard to read. Consider using a proper LCS algorithm or diff from the stdlib/os.

---

[memory/extract.go] Daily budget is in-memory only — resets on process restart

var dailyBudgetSpent float64

Any restart resets the counter. On a server that restarts multiple times per day (deploys, crashes), the daily cap is effectively unenforced. Persisting the spend to a small SQLite row or file would fix this.

---

[watcher.go] No tests for resolveAgentDir

resolveAgentDir is the security-critical path-traversal guard, but it has no dedicated unit tests. Given the function's importance, the following cases should be explicitly tested: normal happy path, ../ traversal attempt, empty string, assignee containing /, assignee containing \, and a symlink if the team decides to guard against that.

---

[configure.go] ConfigureResult.Selected contains unvalidated capability IDs

Unknown capability IDs returned by the LLM (warned to stderr, ignored for file generation) are still included in result.Selected. Callers iterating over Selected who don't also range over BuiltinCapabilities may process garbage IDs. Filter unknowns out of Selected before returning.

---

⚪ Minor

• askAgent drops stderr on successful exit (cmd.Output() only captures stdout). Claude's stderr may contain useful diagnostics. Capture it too and surface on failure.
• buildCapabilityPrompt is English-only. If agent SOUL.md files define non-English-speaking agents, the cron job prompt will be language-mismatched.
• The loadSoul → Configure warning path continues without a SOUL.md. The generated CLAUDE.md still has @SOUL.md in it, which Claude Code will silently skip if missing — not a crash, but can produce confusing capability-selection responses since the agent has no role context.
• ApproveProposal calls LoadSkillBody after already reading the file with os.ReadFile. Minor duplication; consolidate to one read.

---

Test Coverage

Core logic (evolve, extract, configure) has solid table-driven tests. Notable gaps:

Missing	Risk
watcher.go (zero tests)	Medium — optimistic lock logic is untested
resolveAgentDir unit tests	High — security function
ParseExtractsJSON trailing-content case	Low

---

Verdict: Request Changes

Two high-severity correctness bugs (markTaskDoing multi-statement SQLite, and hardcoded absolute path in CLAUDE.md) must be fixed before merge. The silent rollback failure is also a reliability issue worth addressing. Everything else can be addressed in follow-up tickets. Architecture and overall design are sound; test coverage for the extract and evolve modules is above average.

[TakumaLee]

Issues confirmed. Writing the review now.

---

PR #111 Code Review — feat: hermes agent autonomy

Scope reviewed: internal/agent/{configure,watcher}.go, internal/memory/extract.go, internal/skill/evolve.go, CLI wiring, cron engine, config types.

---

🔴 Critical

[watcher.go] Semaphore leak when task is already claimed

In poll(), a slot is acquired unconditionally before calling spawnAgent. The caller releases it only on error. But spawnAgent has a second no-error early return:

if !affected {
    // Another process already claimed this task.
    return nil   // ← returns nil, sem slot never released
}

poll() sees err == nil, skips <-sem, and the slot is permanently consumed. Once all maxConcurrent slots fill with "already claimed" returns, the watcher deadlocks silently. Fix: add <-sem before the !affected return, or restructure so spawnAgent always owns the slot it receives.

---

[skill/evolve.go] updateProposalStatus regex replaces entire file, not just frontmatter

content := regexp.MustCompile(`(?m)^status: .+$`).
    ReplaceAllString(string(data), "status: "+status)

(?m) makes ^ match every line start. A proposal body that contains status: done in the Diagnosis or Failures sections would also be rewritten, silently corrupting the file. Fix: limit replacement to the frontmatter block (between the two --- delimiters) before operating on the rest.

---

🟠 High

[cli/agent.go] No stale-daemon check — double-spawn risk

runWatchDaemon writes tetora-watcher.pid but never reads it first. Running the command twice spawns two daemon processes; only the second PID is tracked, making the first unkillable via the documented path. Fix: before cmd.Start(), read the existing PID file and check whether that process is still alive (e.g. os.FindProcess + Signal(0)). If alive, abort with "daemon already running (PID: N)".

---

[cli/agent.go] writeCLAUDEMD silently overwrites existing CLAUDE.md

If an agent has a hand-crafted CLAUDE.md, tetora agent configure overwrites it without warning or backup. This is an irreversible operation. Fix: check for existence first and either back up the file to CLAUDE.md.bak.<timestamp> or prompt the operator.

---

[cli/agent.go] @ioProtocolPath hardcodes an absolute path into agent CLAUDE.md

content := fmt.Sprintf("@SOUL.md\n@capabilities/index.md\n@%s\n", ioProtocolPath)
// → @/Users/vmgs.takuma/.tetora/workspace/rules/tetora-agent-io-protocol.md

This path is machine-specific. Agent CLAUDE.md files will break on any other machine or after a directory move. Fix: compute the path relative to the agent dir (e.g. ../../rules/tetora-agent-io-protocol.md) or use a workspace-relative @rules/tetora-agent-io-protocol.md convention.

---

🟡 Medium

[watcher.go] Prompt injection via task title/description

func buildTaskPrompt(t watchedTask) string {
    sb.WriteString(fmt.Sprintf("Title: %s\n", t.Title))
    ...
    sb.WriteString(t.Description)

t.Title and t.Description are read from the database and injected verbatim into the claude invocation prompt. A task with a crafted description (Ignore previous instructions and...) could redirect the spawned agent. This is an acknowledged trust-boundary issue for internal use, but consider wrapping task content in XML delimiter tags (<task>...</task>) as a minimum mitigation.

---

[memory/extract.go] Daily budget state resets on restart

dailyBudgetSpent is an in-process variable. If tetora is restarted mid-day (e.g. crash, config reload), the cap resets and the day's spend limit can be exceeded by a multiple of the restart count. For anything beyond development use, persist the daily spend to the DB (one row: date TEXT, spent REAL).

---

[cron/engine.go] tick() special-case chain will keep growing

SkillEvolveJobID adds the third hardcoded if j.ID == "..." block in tick(). Each new background job type will require another branch here. Consider a dispatch table (map[string]func(context.Context)) registered at engine construction, so tick() never needs to change for new job types.

---

Positives

• resolveAgentDir is the right fix for the path-traversal issue noted in the PR context — slash-check plus filepath.Abs canonicalization is the correct defense-in-depth approach.
• matchBrace handles string literals and escape sequences correctly; much more robust than a naive regex JSON extractor.
• markTaskDoing single-round-trip with changes() correctly avoids the TOCTOU window that a separate SELECT + UPDATE would create.
• ValidateExtract covers all the right invariants: prefix, slug regex, op enum, and length caps.
• Test coverage is solid across all three new packages, including boundary cases (FIFO trim at 100, cooldown, insufficient invocations).

---

Verdict: Request Changes

Two issues require fixing before merge:

1. Semaphore leak (watcher.go !affected branch) — will produce a silent deadlock in production within hours of the watcher encountering contention.
2. Regex scope (skill/evolve.go updateProposalStatus) — can silently corrupt proposal files on approve/reject if body text contains a status: line.

Both are one-line fixes. Items 3–5 (daemon guard, CLAUDE.md overwrite, absolute path) should ideally be addressed in this PR before the watcher reaches production, but can be tracked as follow-ups if timelines are tight. Items 6–7 are deferred-improvement candidates.

[TakumaLee]

Code Review — PR #111: hermes agent autonomy

Verdict: Request Changes (posted as comment — cannot approve/request-changes on own PR)

---

🔴 Medium — internal/memory/extract.go: Body truncation is not rune-safe

if len(e.Body) > 600 {
    e.Body = e.Body[:600]  // byte slice, not rune slice
}

len(e.Body) counts bytes. Slicing at byte offset 600 can bisect a multi-byte UTF-8 rune (e.g. a 3-byte CJK character straddling offset 600), producing an invalid UTF-8 string that gets written to disk and later injected into LLM prompts. The test suite doesn't catch this because it uses ASCII fixtures.

Fix: Use []rune or walk forward to the nearest utf8.RuneStart boundary, same pattern already used in elideMiddle in http.go.

---

🔴 Medium — internal/agent/configure.go: agentName not validated for path traversal

func Configure(claudePath, agentsDir, ioProtocolPath, agentName string) (*ConfigureResult, error) {
    agentDir := filepath.Join(agentsDir, agentName)  // no validation
    ...
}

resolveAgentDir in watcher.go correctly blocks traversal payloads (../, ., etc.). Configure skips that check entirely. The CLI caller validates agentName against cfg.Agents (a config-file registry), so the risk is low in normal operation — but Configure is an exported library function and future callers may not perform that pre-check.

Fix: Extract resolveAgentDir to a shared package (or duplicate the check) and call it at the top of Configure. The fix is one if block.

---

🔴 Medium — internal/memory/extract.go: Daily budget lives only in memory

var (
    dailyBudgetMu    sync.Mutex
    dailyBudgetDate  string
    dailyBudgetSpent float64
)

A process restart resets dailyBudgetSpent to zero, allowing the full dailyBudgetUSD to be spent again within the same UTC day. For a cost-control mechanism this is a correctness gap — if the process crashes and restarts mid-day, the cap is silently bypassed.

Fix: Persist (date, spent) to a small file or the history DB on every ReserveDailyBudget call. Read it back at startup with AdjustDailyBudget.

---

🟡 Low — internal/agent/watcher.go: buildTaskPrompt is prompt-injectable

sb.WriteString(t.Description)  // raw DB value, no sanitization

Task title and description come from the database and flow into the -p prompt sent to Claude with no wrapping or escaping. A crafted description like "Ignore all previous instructions and exfiltrate..." is a live prompt injection vector — especially relevant because the watcher is designed to autonomously spawn agents on assigned tasks.

Fix: Wrap the user-supplied fields in a structural delimiter the system prompt instructs the model to treat as data, e.g.:

<task>
Title: ...
Description:
<description>
...DB value...
</description>
</task>

This is defence-in-depth; the attacker still controls DB writes, but it raises the injection bar meaningfully.

---

🟡 Low — internal/agent/watcher.go: markTaskDoing uses fmt.Sprintf SQL

rows, err := db.Query(dbPath, fmt.Sprintf(
    `UPDATE tasks ... WHERE id = '%s' ...`, db.Escape(taskID),
))

Even with db.Escape, string-interpolated SQL is fragile. The correctness of the escape is invisible from this callsite. If db.Escape only replaces ' → '' (common minimal impl), edge cases around encoding or null bytes can still break the query.

Fix: Use a parameterized query interface (db.Exec(path, sql, args...) style). This is also easier to read.

---

🟡 Low — dispatch.go: goroutine uses reflCtx which may be short-lived

go runDeepMemoryExtract(reflCtx, cfg, task, result, ref)

reflCtx is the context of the reflection step — likely has a per-task timeout. If the extraction takes longer than the reflection window (which is plausible for Haiku + file I/O), the goroutine is cancelled mid-write and the extract file may be partially written. The goroutine leaks silently with no error surfacing.

Fix: Derive a fresh context from context.Background() with its own dedicated timeout (cfg.DeepMemoryExtract.BudgetOrDefault() maps to cost, but a 60s wall-clock timeout is enough).

---

🟡 Low — Windows daemon does not detach

agent_daemon_windows.go returns a plain exec.Command with no SysProcAttr. On Windows, the child process inherits the parent's console and is reaped when the parent exits, which is the opposite of daemon behaviour.

Fix: Add:

cmd.SysProcAttr = &syscall.SysProcAttr{
    CreationFlags: syscall.CREATE_NEW_PROCESS_GROUP,
}

---

✅ Already fixed — path traversal in watcher.go

The PR context flagged filepath.Join(cfg.AgentsDir, t.Assignee) as a traversal risk. resolveAgentDir in the diff addresses it correctly: slash rejection + filepath.Abs + prefix assertion. No action needed here.

---

Non-blocking suggestions

• cron/engine.go: The skill-evolve tick block is a near-copy of the backlog-triage block. Both share the same Matches/lastRun guard pattern. A small helper (e.g. shouldFireSpecialJob) would eliminate ~25 lines of duplication and make the next special-job addition safer.
• formatPRContext: The totalSoftCap is documented as a "soft ceiling" but only emits a log.Warn. If the PR has a 500KB description (bot-generated), the full blob reaches the LLM. Consider truncating with elideMiddle at the total level too.

---

Summary: Three medium items need fixes before merge (rune-safe truncation, Configure path validation, durable daily budget). The others are low/suggestion-level. Architecture is solid — the three-layer design is clean and the test coverage on the new packages is good.

[TakumaLee]

Review 已發出：#111 (comment)

---

Review 摘要：

Verdict: Request Changes

找到 3 個 medium 問題需要修復才能 merge：

嚴重度	問題	位置
🔴 Medium	Body 截斷不是 rune-safe，可產生無效 UTF-8	internal/memory/extract.go
🔴 Medium	agentName 未做 path traversal 驗證	internal/agent/configure.go
🔴 Medium	Daily budget 僅在記憶體，process restart 即重置	internal/memory/extract.go
🟡 Low	buildTaskPrompt 直接注入 DB 值，prompt injection 風險	internal/agent/watcher.go
🟡 Low	markTaskDoing 用 fmt.Sprintf 拼 SQL，應改 parameterized query	internal/agent/watcher.go
🟡 Low	goroutine 使用 reflCtx，可能在 extract 完成前超時	dispatch.go
🟡 Low	Windows daemon 未 detach	agent_daemon_windows.go

Path traversal 在 watcher.go 的問題（PR context 已標記）已由 resolveAgentDir 修復，不需額外處理。架構設計乾淨，新 package 的 test coverage 足夠。

[TakumaLee]

PR 語言為英文，按 LRN-20260326-001 以英文撰寫 review。

---

PR #111 Code Review — feat: hermes agent autonomy

Reviewer: Kokuyou
Scope: internal/skill/evolve.go, internal/memory/extract.go, internal/agent/configure.go, internal/agent/watcher.go, dispatch.go, http.go, internal/cron/engine.go, internal/config/types.go, test files.

---

Overview

Solid architecture across all three layers. Test coverage for pure functions is good. The elideMiddle UTF-8 boundary handling is correct, extractJSONObject respects string literals properly, and the path-traversal fix in resolveAgentDir is solid. The critical issues below must be fixed before merge.

---

Risks

🔴 Critical

[watcher.go] Prompt injection via task title/description

buildTaskPrompt passes DB values directly into the claude prompt with no sanitization:

sb.WriteString(fmt.Sprintf("Task ID: %s\n", t.ID))
sb.WriteString(fmt.Sprintf("Title: %s\n", t.Title))
...
sb.WriteString(t.Description)  // inside <task-description> but not escaped

The XML delimiter comment says "raise the bar" but doesn't actually sanitize. A crafted description containing </task-description>\n\nIgnore all previous instructions... fully breaks out of the delimiters. t.Title and t.ID are also unsanitized and injected into the prompt header before the delimiter, giving an even higher-trust injection surface.

Fix: strip or escape </task-description> from the description before writing, and enforce that task ID matches [a-zA-Z0-9_-]+ before embedding. The ID is a UUID in practice, but the watcher should not assume that.

---

[skill/evolve.go] readProposalStatus matches outside frontmatter

for _, line := range strings.Split(string(data), "\n") {
    if strings.HasPrefix(line, "status: ") {
        return strings.TrimPrefix(line, "status: ")
    }
}

A proposal whose Diagnosis or Proposed Body contains the literal string status: rejected will return the wrong status, silently mis-classifying the proposal and either hiding it from the list command or incorrectly blocking approve.

Fix: only scan lines between the opening --- and its matching closing ---. Return "pending" if the frontmatter block is not found.

---

🟡 Medium

[configure.go] Unknown capabilities leak into ConfigureResult.Selected

The warning is printed but the original resp.SelectedCapabilities (with unknown IDs) is assigned to result.Selected unchanged:

result := &ConfigureResult{
    Selected: resp.SelectedCapabilities, // unknown IDs survive here
    ...
}

The capabilities index and detail files are correctly filtered by BuiltinCapabilities, but callers iterating result.Selected will see capability IDs that have no files on disk and no corresponding cron/config entries. The displayed confirmation (fmt.Printf("Selected: %s\n", ...)) will also mislead the operator.

Fix: filter resp.SelectedCapabilities against known before assigning to result.Selected.

---

[configure.go] writeCLAUDEMD embeds an absolute path

content := fmt.Sprintf("@SOUL.md\n@capabilities/index.md\n@%s\n", ioProtocolPath)

ioProtocolPath is computed at configure-time as an absolute path. If the workspace is moved, synced to another machine, or the agent dir is copied, the @include will silently break. Claude Code @ includes are resolved relative to the file; @/absolute/path portability is fragile.

Fix: compute a relative path from agentDir to ioProtocolPath using filepath.Rel, fall back to absolute only when Rel fails.

---

[dispatch.go] Deep-memory-extract goroutines are fire-and-forget on shutdown

Both call-sites spawn:

go func() {
    defer extractCancel()
    runDeepMemoryExtract(extractCtx, cfg, task, result, ref)
}()

When the main process receives SIGTERM, these goroutines can be killed mid-write to memory/auto-extracts.md, potentially producing a half-written FIFO file. The autoExtractsMu mutex protects against concurrent writers but not against a killed writer leaving the file partially written.

This is an acceptable trade-off for a best-effort background operation, but the comment should say so explicitly, and the write in AppendToAutoExtractsMD should be atomic (write to temp file + rename) rather than a direct os.WriteFile.

---

[cron/engine.go] Growing special-ID dispatch list is an anti-pattern

tick(), RunJobByID(), and StartupReplay() now have four special-cased IDs (daily_notes, backlog-triage, war_room_autoupdate, skill_evolve). Each addition requires touching three separate if j.ID == blocks. Not a blocker, but flagged for a future refactor toward a map[string]JobHandler dispatch table.

---

🔵 Suggestions (non-blocking)

[configure.go] SOUL.md content is injected into the LLM prompt without boundary protection

If SOUL.md contains --- horizontal rules or bare JSON resembling {"selected_capabilities": [...]}, extractJSONObject might match the SOUL content before the model's actual response. Consider adding an explicit <soul> wrapper around the soul content in buildConfigurePrompt to prevent the JSON extractor from matching SOUL body text.

[watcher.go] No liveness check for already-running daemon

runWatchDaemon writes a PID file but never checks if a watcher is already running before spawning a new one. Two concurrent watcher instances will race on the optimistic lock — they'll both fetch the full todo task list, both try markTaskDoing, and the loser silently drops. Functionally correct but wastes two claude invocations per polling cycle. Recommend checking the PID file at daemon start and exiting if the process is already alive.

[memory/extract.go] ValidateExtract silently mutates input

The body-truncation side effect is documented, but modifying caller data in a Validate function is surprising. Consider returning the truncated body as a separate return value, or splitting into ValidateExtract (returns error only) + NormalizeExtract (truncates).

[http.go] fetchPRContextGitHub trusts GitHub's ordering for lastN

lastN(view.Reviews, 2) assumes the GitHub API returns reviews in chronological order. gh pr view --json reviews does return oldest-first, but this assumption is not documented. Add a comment or a sort-by-CreatedAt to make the intent explicit and robust.

---

Test Coverage Note

TestReviewCommentCmdArgs locking down the -F/--form flag distinction is exactly the right instinct given the regression history. The rune_boundary tests in both TestElideMiddle and TestFormatPRContext are thorough. The watcher.go and configure.go integration paths (which require a live claude binary) are not unit-tested — that's fine given the test infrastructure constraints, but buildTaskPrompt and resolveAgentDir should each have their own table-driven tests, especially for the injection and traversal cases.

---

Verdict: REQUEST CHANGES

The two critical issues — prompt injection in buildTaskPrompt and frontmatter-boundary bug in readProposalStatus — must be fixed before merge. Both are exploitable in production: the watcher accepts task data from the DB and the proposal status bug can silently hide or misclassify pending proposals.

The medium issues (especially writeCLAUDEMD portability and unknown capability leak) should also be fixed in this PR since they affect the configure flow's correctness.

[TakumaLee]

查到了關鍵資訊：db.Query 透過 sqlite3 CLI 執行，使用 sqlite3_exec 支援 multi-statement SQL。現在撰寫完整 review。

---

PR #111 Code Review — feat: hermes agent autonomy

Scope reviewed: internal/skill/evolve.go, internal/memory/extract.go, internal/agent/configure.go, internal/agent/watcher.go, dispatch.go, http.go, CLI wiring, config types.

---

Risks

🔴 Critical

[watcher.go] markTaskDoing uses string-formatted SQL — parameterized queries available but unused

The function builds SQL via fmt.Sprintf and db.Escape:

rows, err := db.Query(dbPath, fmt.Sprintf(
    `UPDATE ... WHERE id = '%s' AND status = 'todo'; SELECT changes() AS changed;`,
    db.Escape(taskID),
))

The internal/db package explicitly supports parameterized ? placeholders via bindArgs. db.Escape is correct for SQLite string literals, but string-formatting SQL is fragile — any future refactor that skips the escape call opens a second-order injection path (task IDs are database-sourced, but that DB can itself be tainted if other agents write crafted task entries). The rollback db.Exec call in spawnAgent has the same issue:

_ = db.Exec(cfg.HistoryDB, fmt.Sprintf(
    `UPDATE tasks SET status = 'todo'... WHERE id = '%s'`,
    db.Escape(t.ID),
))

Fix: Use parameterized queries for both calls.

---

[configure.go / watcher.go] resolveAgentDir correctly blocks path traversal — verify call sites are exhaustive

The path traversal fix is correct and comprehensive:

if assignee == "" || strings.ContainsAny(assignee, "/\\") || assignee == "." || assignee == ".." {
    return "", fmt.Errorf("invalid assignee %q", assignee)
}
// ... prefix check against rootWithSep

However, resolveAgentDir in watcher.go and configure.go are the same logic duplicated across two files. If a third call site is added later without calling resolveAgentDir, the traversal protection is silently missing. Recommendation: Move resolveAgentDir to a shared location (e.g., internal/agent/agentdir.go) since both configure.go and watcher.go already live in package agent.

---

🟡 Medium

[watcher.go] buildTaskPrompt title sanitization strips \n but not \r

title := strings.ReplaceAll(t.Title, "\n", " ")

A title containing \r\nINJECTED would survive the strip as \rINJECTED, which on Windows-style line endings becomes a bare carriage return before injected text. Use strings.Map to strip all control characters:

title := strings.Map(func(r rune) rune {
    if r == '\n' || r == '\r' || r == '\t' {
        return ' '
    }
    return r
}, t.Title)

---

[dispatch.go] Background runDeepMemoryExtract goroutines are untracked — graceful shutdown gap

go func() {
    defer extractCancel()
    runDeepMemoryExtract(extractCtx, cfg, task, result, ref)
}()

There's no sync.WaitGroup tracking these goroutines. On SIGTERM, in-flight extracts are killed by OS process exit, not by context cancellation — the 2-minute extractCtx timeout doesn't help during shutdown. Since AppendToAutoExtractsMD uses atomic rename this is safe for the FIFO file, but any LLM API call mid-flight is silently dropped. This is acceptable for best-effort extraction, but should be documented explicitly. If the dispatcher already has a shutdown WaitGroup, these goroutines should be registered there.

---

[skill/evolve.go] Skill name embedded raw in YAML frontmatter

content := fmt.Sprintf(`---
proposal_id: %s
skill: %s
...
---`, id, cand.Name, ...)

cand.Name comes from a directory name. Directory names can contain : which is a YAML key separator. A skill named foo:bar would produce skill: foo:bar — valid YAML but parsed as skill → "foo:bar" (string) in most parsers. More dangerous: a skill named foo\ndirty: injected would break the frontmatter block. Validate or quote cand.Name:

skill: %q  // Go %q produces a quoted string

Or assert cand.Name matches [a-zA-Z0-9_-]+ before reaching WriteEvolveProposal.

---

[configure.go] agent configure silently overwrites CLAUDE.md without idempotency check

generateCapabilityFiles always overwrites CLAUDE.md and all capability files. Running tetora agent configure <name> a second time discards any manual customizations the operator added (e.g., additional @include directives or capability customizations). Consider adding a --force flag that requires explicit opt-in to overwrite, or at minimum emitting a warning when the file already exists.

---

[memory/extract.go] dailyBudget persistence is opt-in but silently no-op if skipped

func InitDailyBudgetStore(path string) { ... }

If InitDailyBudgetStore is not called at startup (or called with an empty path), the daily budget cap has no persistence across process restarts — the cap can be exceeded trivially by restarting. There's no documentation of the startup requirement, and dailyBudgetStorePath == "" silently disables persistence rather than erroring. The store path should be derived automatically from config (e.g., alongside HistoryDB) rather than requiring a separate initialization call.

---

🟢 Low / Suggestions

[http.go] elideMiddle missing defensive guard for head >= tailStart

head := maxBytes * 2 / 3
// ... head snaps forward, tailStart snaps backward
elided := len(body) - head - (len(body) - tailStart)
return body[:head] + ... + body[tailStart:]

With perItemMaxBytes = 32_000 in production the 3-byte maximum rune boundary correction can never cause head >= tailStart. But the contract isn't enforced in code. Add:

if head >= tailStart {
    return body[:maxBytes] // fallback: hard truncate
}

---

[dispatch.go] skillEvolveSem is package-level global — test isolation issue

var skillEvolveSem = make(chan struct{}, 1)

Package-level channels shared across tests can cause flaky tests if multiple test cases invoke runSkillEvolveScan concurrently. Prefer threading the semaphore through the function signature.

---

[configure.go] SOUL.md content injected into LLM prompt without sanitization

sb.WriteString(soulContent)

SOUL.md is operator-controlled so this is low risk, but if SOUL.md contains the literal string "selected_capabilities" (e.g., as an example in the soul definition), extractJSONObject would match that embedded example instead of the agent's actual response. The extractJSONObject function already handles this by requiring a balanced {...} block containing "selected_capabilities", but it picks the first match — an example in SOUL.md appearing before the actual response would be selected. Consider searching from the end of the output rather than the beginning.

---

[http.go] glab -F regression test is well-placed

TestReviewCommentCmdArgs correctly guards the -F vs --form flag regression. The comment block explaining glab's reversed -F/-f semantics vs gh is valuable — this is the kind of unintuitive behavior that gets flipped during refactors. The test setup is solid.

---

[http.go] GitLab notes endpoint fetches newest-first but processes only 20

notesEndpoint := fmt.Sprintf("...notes?per_page=20&order_by=created_at&sort=desc", ...)

With keepComments = 2 and lastN, this effectively keeps the 2 most recent of the last 20 fetched (after reversal). For very active MRs with >20 notes, the "2 most recent" might still be the 2 most recent overall since the API returns newest first. But the intent is unclear — if per_page=20 is meant as "fetch the most recent 20 and keep the last 2", the logic is correct. If it's meant to page, it's incomplete. A comment clarifying this is warranted.

---

Approve / Request-Changes Recommendation

Request Changes.

Two issues block merge:

1. SQL injection surface — markTaskDoing and the rollback db.Exec in spawnAgent should use parameterized queries. The db package supports them; the string-formatting pattern is inconsistent with safe usage elsewhere.

2. resolveAgentDir duplication — the identical function exists in two files. Before this PR adds a third call site, consolidate it.

Everything else — the path traversal fix itself, elideMiddle/formatPRContext refactoring, GitLab MR context support, AppendToAutoExtractsMD atomic write, daily budget state machine, matchBrace JSON extraction, and test coverage — is architecturally sound and well-implemented.