feat(memory): temporal facts, knowledge history, cmd timeout + cancel fix

[chinkan]

Summary

• Knowledge history — SQLite UPDATE/DELETE triggers auto-archive prior values; knowledge_timeline / knowledge_as_of
• Temporal facts — (entity, relation, value, valid_from, valid_to); one active per pair; auto-close on new value
• MCP tools — add_fact, query_facts, close_fact, fact_history
• Shell timeout — sandbox.execute_timeout_secs default 90, 0 = off
• Cancel button fix — CallbackQuery no longer chat-serialized behind in-flight execute_command (root cause of dead Cancel)

Deferred

• Agentic RAG → feat: agentic RAG for search_memory #52
• Harness evolution → feat: harness evolution (SafeEvolve-inspired) #53

Docs

• CONTEXT.md memory glossary
• ADR 0004 temporal facts / knowledge history
• ADR 0005 callback concurrency

Test plan

• cargo test (incl. 6 new memory/knowledge unit tests)
• cargo clippy -- -D warnings
• Manual: run long sleep 120 → timeout ~90s
• Manual: Cancel button during running command → toast + kill
• Manual: remember A→B → fact_history(category,key) shows archive
• Manual: add_fact Nike then Adidas → query as_of past/present

[opencode-agent]

Both axes' key claims verified against source (idx_facts_active is non-unique at src/memory/mod.rs:364; pull-requests: read at .github/workflows/opencode-review.yml:13; button shown at src/command_tool.rs:105/113 before register at :122). Aggregated review:

Standards

SQL injection: none found. All new queries in knowledge.rs/mod.rs are parameterized via rusqlite::params!/bound placeholders; search_facts builds format!("%{query}%") but passes it as a bind param (safe; unescaped %/_ in user input are wildcards, cosmetic only). Triggers are static SQL.

Security (timeout/cancel): Default 90s cap tightens sandbox behavior; 0 = disable is documented in config.example.toml. kill_child SIGKILLs the process group on timeout — sound. Cancel path semantics preserved (ADR-0005 documents the distribution change).

Documented-standard issues

• CI action pinning (hard-ish): CLAUDE.md CI conventions use pinned tags (@stable, @v2); existing workflows use checkout@v4. New workflows use actions/checkout@v6 (inconsistent) and anomalyco/opencode/github@latest — an unpinned mutable tag, a supply-chain deviation from every existing workflow.
• Missing permission: opencode-review.yml posts PR reviews but grants only pull-requests: read / issues: read. If the action comments, it needs write.
• opencode.yml: no trailing newline at EOF.
• Tests: comply — #[cfg(test)] mod tests in knowledge.rs. CLAUDE.md's "No automated tests exist yet" is now stale; docs should be updated.
• default_execute_timeout_secs() follows the existing default_model() pattern — compliant.

Baseline smells (judgement calls)

• Duplicated Code: identical UPDATE facts SET valid_to = ?1 WHERE entity = ?2 AND relation = ?3 AND valid_to IS NULL in add_fact and close_fact; fact format string "{} —{}→ {} [{}..{}] conf={}" duplicated across query_facts and fact_history.
• Duplicated Code (CI): the two new workflows repeat the same checkout+run steps; consider a reusable workflow.
• Speculative Generality: search_facts has zero callers (no tool binding, no test); fact_history's four optional params with no required is a flag-arg two-tools-in-one shape.
• Primitive Obsession: change_type: String in KnowledgeVersion vs. the SQL CHECK enum. (Dates-as-String exempt: documented in CONTEXT.md.)
• Mysterious Name: // ponytail: LIKE, not FTS comment in search_facts.

Spec

(a) Missing / partial

1. knowledge_as_of unreachable by the agent — exists (src/memory/knowledge.rs:266) and is tested, but no MCP tool exposes it; fact_history only calls knowledge_timeline (src/memory_tools.rs:253). Spec line: "knowledge_timeline / knowledge_as_of".
2. "One active per pair" enforced only in Rust — idx_facts_active (src/memory/mod.rs:364) is a non-unique partial index; no UNIQUE constraint, so the invariant breaks for any other write path.
3. Timeout applies only to execute_command — supervisor's shell backend still runs unbounded Command::output() (src/supervisor/backend/shell.rs:61). Spec line: "Shell timeout".
4. ADR-0004 says "bi-temporal" but only valid-time is modelable — created_at is stored, never surfaced on Fact or used in queries.
5. ✓ Verified present: UPDATE and DELETE triggers, exact fact columns, auto-close, all four tools registered, CallbackQuery None key (src/platform/telegram.rs:293), exactly 6 new passing tests.

(b) Scope creep

• .github/workflows/opencode.yml + opencode-review.yml — not in the PR summary.
• search_facts() — not requested, no tool, no test = dead code.
• Cancel arm now breaks the loop and three UI-branch comments were deleted — churn not described.

(c) Implemented but looks wrong

1. Inverted windows on backfill: add_fact sets valid_to = new.valid_from unconditionally; if the new valid_from is older than the active fact's start, valid_to < valid_from makes the prior fact invisible at every as-of point.
2. Mixed precision string compares: changed_at uses datetime('now') (YYYY-MM-DD HH:MM:SS) while tools/ADR advertise 2024-09-01 granularity; h.changed_at <= as_of and valid_to > ?2 mis-order same-day values.
3. Cancel race: show_cancel_button (src/command_tool.rs:105/113) publishes the button before cancel_registry.register (:122) — now that callbacks run concurrently, early clicks hit "Command already finished" and the kill is lost.
4. Timeout discards output: result is only "⚠️ Command timed out after Ns"; buffered partial output is dropped, so the LLM gets no evidence.
5. Dedup short-circuit returns the existing id and silently ignores the new valid_from.

---

Summary: Standards — 6 findings, worst: unpinned @latest action + pull-requests: read-only permission in opencode-review.yml (the review workflow can't actually post reviews). Spec — 12 findings, worst: the cancel-button registration race (src/command_tool.rs:105 vs :122), which undermines the PR's headline Cancel fix under exactly the concurrency it just enabled.

opencode session  |  github run

[chinkan]

Addressed opencode review findings:

Finding	Fix
Cancel register-after-show race	register before show_cancel_button
Timeout drops output	append partial buffer to tool result
Inverted valid windows on backfill	historical insert closes at active start; active untouched
One-active only in Rust	UNIQUE partial index idx_facts_one_active
Date-only vs datetime compares	normalize YYYY-MM-DD → start/end of day
knowledge_as_of not exposed	fact_history(category,key,as_of=…)
Supervisor shell unbounded	tokio::time::timeout(job.timeout_secs)
Dead search_facts	removed
CI perms / checkout	pull-requests: write, issues: write, checkout@v4
ADR “bi-temporal”	corrected to valid-time

Skipped (YAGNI): pin @latest (opencode docs), reusable workflow extract, change_type enum.

[opencode-agent]

Both sub-agent reports verified against source. Spot-checks confirmed: cancel register (command_tool.rs:98) now precedes show_cancel_button (:111/:119); idx_facts_one_active is a proper partial UNIQUE index (memory/mod.rs:365); normalize_ts (knowledge.rs:486) is byte-identical to normalize_from (:469); execute_timeout_secs is plumbed only to main.rs:244/CommandTool — the supervisor still uses job.timeout_secs (default 600).

PR #54 Re-Review (post-fix, @ 4406413)

Standards

Prior-finding verification

#	Finding	Status
3	workflow perms pull-requests/issues: write	FIXED (opencode-review.yml:19-20)
4	checkout@v4	FIXED (both workflows)
5	dead search_facts	FIXED (absent)
6	SQL injection	PASS — all new queries parameterized via params![…]/?N; triggers interpolate only OLD./NEW. column refs, never user input
7	trailing newline / fmt	FIXED — cargo fmt --all --check clean
1a	Duplicate UPDATE facts SET valid_to…	PARTIAL — add_fact/close_fact no longer identical (close_fact adds AND valid_from <= ?1) but the WHERE entity=? AND relation=? AND valid_to IS NULL shape still repeats 3× with no shared helper
1b	Fact format string dup	PARTIAL — still near-duplicated across query_facts/fact_history
2	change_type: String vs SQL CHECK enum	NOT FIXED (knowledge.rs:27) — author marked YAGNI

Hard documented-standard violation

• CLAUDE.md:144 "No automated tests exist yet" is now false — knowledge.rs adds ~8 #[test]/#[tokio::test] cases. The PR should update this line (Testing section).

Baseline smells (judgement)

• Duplicated Code: normalize_ts (knowledge.rs:486) is byte-identical to normalize_from (:469) — one is redundant; collapse.
• Duplicated Code: two near-identical empty JobOutput { status: Failed, … } literals in shell.rs — a JobOutput::failed(errors) ctor would help.
• Repeated Switches: fact_history (memory_tools.rs) is a 3-level if let ladder over four optional args.

Spec

Prior-finding verification

#	Finding	Status
1	knowledge_as_of unreachable	FIXED — fact_history(as_of=…) dispatches to it
2	one-active per pair in DB	FIXED — partial UNIQUE index, NULL-dup rejected/closed-dup allowed
4	ADR "bi-temporal" wording	FIXED — ADR-0004 now "valid-time … created_at is audit only"
5	inverted backfill window	FIXED — historical row closes at active start, active untouched (tested)
6	mixed date/datetime precision	FIXED — date-only normalized to start/end-of-day
7	cancel register-after-show race	FIXED — register precedes show
8	timeout discarded partial output	FIXED — buffered output appended to result
3	supervisor shell timeout	PARTIAL
9	dedup ignores new valid_from	PARTIAL (intentional, documented+tested)

Worst — finding 3 (spec line: "sandbox.execute_timeout_secs default 90, 0 = off"): the timeout was added to shell.rs but wired to job.timeout_secs (hardcoded default 600, job.rs:89), not the execute_timeout_secs config key — which is plumbed only to the chat CommandTool. So execute_timeout_secs has no effect on the supervisor, and 0 = off is unconfigurable there. Secondary: tokio::time::timeout around Command::output() has no process-group kill (unlike command_tool.rs), so the child can survive the timeout. This is a fix that addresses the letter of the prior finding but not the spec's intent.

Minor gaps

• is_date_only/normalize leaves T-separated ISO (2025-06-01T12:00) unnormalized → mis-compares vs space format (finding 6 residual).
• CREATE UNIQUE INDEX (no IF NOT EXISTS-safe dedup migration) will abort startup on a dev DB that has duplicate active rows from the branch's earlier non-unique index. Main is unaffected (unreleased); flag for anyone who ran intermediate commits.

Scope creep (unchanged)

• .github/workflows/opencode.yml + opencode-review.yml are unrelated to the memory/timeout/cancel spec.

---

Summary: Standards — 3 open (1 hard: stale CLAUDE.md test claim; worst: duplicated normalize_ts/normalize_from + 3×-repeated SQL). Spec — 7 of 9 prior findings fully fixed; worst: supervisor timeout wired to hardcoded job.timeout_secs, not execute_timeout_secs, so the spec's config knob does nothing on that path.

opencode session  |  github run

[chinkan]

Re-review follow-up

Finding	Fix
Supervisor timeout ≠ execute_timeout_secs	ShellBackend takes config; effective = min(job, sandbox) (0 sandbox = job only)
No process-group kill on timeout	spawn + killpg/kill (mirrors CommandTool)
normalize_ts == normalize_from	collapsed; ISO T/Z/offset → SQLite space form
UNIQUE index fails on dup actives	dedupe (keep newest) then create index
Stale CLAUDE.md “no tests”	Testing section updated
Dup failed JobOutput	JobOutput::failed()

Tests: knowledge 8 + shell 4 green.

[opencode-agent]

No gh auth in this run; the opencode infra posts my response automatically. Review below (verified against source at a07e334: config wiring main.rs:425-427, byte-identical kill_child shell.rs:49-59 vs command_tool.rs:307-317, dedupe SQL memory/mod.rs:371-389; cargo test 486 pass, fmt/clippy -D warnings clean).

PR #54 Round-3 Re-Review (post-fix, @ a07e334)

Claimed-fix verification

#	Claim	Status
1	Supervisor timeout = min(job, sandbox), config wired	VERIFIED — shell.rs:25-31 + main.rs:425-427; 0 edges correct + unit-tested
2	Process-group kill on supervisor timeout	VERIFIED — process_group(0) (shell.rs:108) + killpg (:49-59)
3	Normalize collapse + ISO T/Z/offset	VERIFIED w/ defect — normalize_ts gone; see Standards §3
4	Dedupe before UNIQUE index	VERIFIED w/ gap — memory/mod.rs:371-389; see Spec §2
5	CLAUDE.md Testing section	VERIFIED
6	JobOutput::failed()	VERIFIED — used at shell.rs:94/114/152

Standards

1. Duplicated Code (worst, cross-file): kill_child is byte-identical in src/supervisor/backend/shell.rs:49-59 and src/command_tool.rs:307-317; the timeout_secs == 0 → pending guard repeats too (command_tool.rs:169 / shell.rs:121). Extract one shared helper.
2. Duplicated Code: WHERE entity=? AND relation=? AND valid_to IS NULL shape repeats across knowledge.rs/mod.rs; fact-format string near-duplicated (memory_tools.rs:229 vs :286) — give Fact a Display.
3. Timezone truncation: normalize_from (knowledge.rs:471-484) splits on +/Z and discards the offset — 12:00:00+08:00 → 12:00:00 vs UTC datetime('now'), mis-comparing by hours. A test asserts the truncation as correct. Convert to UTC or document the assumption.
4. Silent kills: let _ = killpg(...) with no warn! in both files (CLAUDE.md logging convention). Minor.

Spec

1. Worst — pipe-drain deadlock → false timeouts (regression introduced by the round-2 fix): shell.rs:131-144 select!s on child.wait() and calls Self::drain only after wait/kill. A healthy command writing >~64KB blocks in write, wait() never returns, and it's SIGKILLed at the timeout — old .output() drained concurrently. Move drains into the select! (join the drain futures with the wait).
2. Dedupe tie gap breaks startup: memory/mod.rs:378 closes losers via created_at < d.mx; datetime('now') is second-resolution, so same-second duplicate actives both survive → CREATE UNIQUE INDEX fails → store open errors on every startup for affected dev DBs. Add a rowid tiebreak.
3. knowledge_as_of can't express absence: triggers archive only on overwrite/delete, so a delete→recreate gap returns the new value (knowledge.rs:298-305). Acceptable this phase — document in ADR-0004/CONTEXT.md.
4. Test gaps: no CommandTool timeout-path test; shell_backend_timeout_kills_and_respects_config never asserts the child is dead; memory_tools.rs as_of dispatch untested. Equal-valid_from zero-length window is semantically correct under valid-time-only ADR-0004 — not a bug.
5. Scope creep (unchanged): opencode.yml + opencode-review.yml remain outside the PR summary.

---

Summary: Standards — 4 findings, worst: kill_child + timeout-guard duplicated byte-identical across shell.rs/command_tool.rs. Spec — 5 findings, worst: shell.rs drains pipes only after child.wait(), so large-output commands deadlock into false timeouts — a regression introduced by the round-2 kill fix.

opencode session  |  github run

[chinkan]

Round-3 fixes

Finding	Fix
Pipe-drain deadlock (large stdout)	concurrent drain tasks while wait/timeout; test with 200KB output
kill_child duplicated	utils::process::{kill_child, optional_timeout} + warn! on failure
Dedupe same-second tie	keep MAX(rowid) per pair, close the rest
TZ truncation	chrono RFC3339 → UTC YYYY-MM-DD HH:MM:SS
Fact format dup	impl Display for Fact
knowledge_as_of delete gap	documented in ADR-0004

Tests: knowledge 8 + shell 5 (incl. large stdout) green.

[opencode-agent]

Round-4 re-review (@ c8a8af7). Both axes' claims verified against source; cargo test 487 pass, fmt/clippy -D warnings clean; I reproduced the spec's worst finding live (probe test: sh -c 'sleep 30 & echo hi' — direct child exits, run() still had not returned after 6s).

Standards

Claim	Verdict
kill_child → utils::process	FIXED — only killpg site is src/utils/process.rs:18; both callers use it
shared optional_timeout	FIXED (command_tool.rs:168, shell.rs:120)
impl Display for Fact	FIXED — but fact_history output now includes conf= it previously omitted (cosmetic behavior change)
UTC normalize	PARTIAL — see 3
warn! on kill failures	FIXED (utils/process.rs:22,25,28)

Judgement-call findings:

1. Duplicated Code (new, ironic): drain_pipe/drain_err (shell.rs:50-72) are byte-identical except stream type — the commit that removed the kill_child pair added a fresh one. One generic async fn drain<R: AsyncReadExt + Unpin>(…) fixes it.
2. Swallowed errors (against CLAUDE.md logging style): h.await.unwrap_or_default() (shell.rs:138,142), Err(_) => break in drains, let _ = tokio::join!(…) (command_tool.rs:206) — drain-task panics vanish silently.
3. Normalize fallback unsound: ts.replace('T', " ") (knowledge.rs:495) turns seconds-less ISO 2025-06-01T12:00Z into 2025-06-01 12:00Z, which sorts greater than 2025-06-01 12:00:30 under lexicographic compare → wrong as-of results. Offset/t/z/fractional shapes are fine; add seconds-less + fractional formats, else pass through unchanged.
4. COALESCE(valid_to, valid_from) (memory/mod.rs:372) is dead under WHERE valid_to IS NULL; dedupe migration has no test.
5. No stdout cap in shell.rs (summary = stdout, :153/:165) while command_tool caps at MAX_BUFFER_CHARS — chatty command × long job timeout = unbounded RAM. Pre-existing per-chunk from_utf8_lossy also mangles multibyte chars at 8192 boundaries (both files) — collect Vec<u8>, decode once.
6. Inconsistent-semantics third timeout site left behind: backend/mod.rs:154 uses raw tokio::time::timeout, where timeout_secs == 0 means instant fail, vs no timeout in the new optional_timeout (shared CLI/script path; pre-existing, but the PR created the canonical helper).

Spec

Claim	Verdict
Pipe-drain deadlock	PARTIAL — >64KB case fixed + tested; see worst
Dedupe rowid tiebreak	VERIFIED — SQL run on sqlite 3.45: MAX(rowid) winner survives, losers closed zero-length, UNIQUE index builds; subquery materializes pre-UPDATE
ADR-0004 delete→recreate gap documented	VERIFIED — matches trigger code
TZ normalization	VERIFIED (Standards §3 residual)
"knowledge 8 + shell 5 tests"	VERIFIED counts — but prior gaps persist: no CommandTool timeout test, timeout test still doesn't assert child death, no as_of dispatch test
CI-workflow scope creep	NOT FIXED — opencode.yml/opencode-review.yml still ride this PR

Worst — unbounded post-wait drain await (proven by repro): shell.rs:136-143 awaits drain JoinHandles with no timeout after child.wait() returns. sh -c 'sleep 30 & echo hi': sh reaps, hi was already captured, but the grandchild (same process group) holds the write end → run() blocked ≥6s in my probe (until sleep exits, or forever if sandbox cap 0/execute_timeout_secs exceeds its life — the timeout_fut was consumed by the select! on the wait branch, so there's no remaining guard). Same shape at command_tool.rs:206. Fix: tokio::time::timeout(few_secs, h.await) after wait, or killpg on the success path too. The round-3 deadlock is genuinely gone, but the fix traded a false-timeout for a true hang in the daemon-spawning case.

Minor: shell_backend_large_stdout_does_not_deadlock adds an undeclared python3 test dependency with an 8s-vs-10s flake margin (dd if=/dev/zero … avoids both).

---

Summary: Standards — 6 findings, worst: normalize fallback produces mis-orderable …Z-suffixed timestamps (knowledge.rs:495) — plus a new byte-identical duplication introduced by the dedup commit itself. Spec — 6 findings, worst: proven unbounded hang when a spawned-offspring command inherits stdout (shell.rs:136-143), so the Cancel/timeout headline guarantee can still wedge a job forever.

opencode session  |  github run

[opencode-agent]

Both axis reports verified against source (26a3a36, 488 tests pass confirmed by me — one sub-agent's "470" was wrong; fmt/clippy failures confirmed live; shell.rs hang repro consistent with code at shell.rs:126-144). Aggregated review:

PR #54 Round-5 Re-Review (@ 26a3a36)

⚠️ This commit does not pass CI. cargo fmt --all --check fails (hand-compacted enum SendMode { Verbose, Minimal, Silent } + struct literal + match arms in src/command_tool.rs:12,28,79+) and cargo clippy -D warnings fails (manual_pattern_char_comparison at src/memory/knowledge.rs:493 — lib does not compile). Both were clean at c8a8af7.

Standards

Round-4 claim verification (commit message vs actual):

Claim	Verdict	Evidence
"shared drain_pipe helper in utils::process"	PARTIAL	Helper exists (src/utils/process.rs:20) and command_tool.rs:106/111 uses it — but shell.rs:50-72 still has the byte-identical drain_pipe/drain_err pair, untouched by this commit.
"explicit Z/UTC handling" in normalize	PARTIAL	"2025-06-01T12:00Z" → 12:00:00 ✓ (tested). But seconds-less no-Z "2025-06-01T12:00" still falls through knowledge.rs:503/506 to the raw ts.replace('T', " ") fallback (:509) → "2025-06-01 12:00", breaking the function's own doc contract (:479 "Canonicalize to UTC YYYY-MM-DD HH:MM:SS") — boundary compares vs 12:00:00 mis-order; untested.
Swallowed errors	NOT FIXED (not claimed)	h.await.unwrap_or_default() (shell.rs:138,142), Err(_) => break (shell.rs:55/67, process.rs:27); at command_tool.rs:164 is_err() only catches timeout — a drain-task panic still vanishes. Contradicts CLAUDE.md logging style.
Dead COALESCE / untested dedupe	NOT FIXED	memory/mod.rs:371 — valid_to is always NULL under WHERE valid_to IS NULL; still no migration test.
stdout cap / multibyte lossy	NOT FIXED	shell.rs:165 uncapped vs command_tool's MAX_BUFFER_CHARS; per-chunk from_utf8_lossy splits multibyte at boundaries (shell.rs:56 @8192, process.rs @4096).
backend/mod.rs:154 0 = instant fail vs optional_timeout 0 = never	NOT FIXED	Inconsistent semantics on the shared CLI/script path.

New in this commit: drain_pipe_timeout (process.rs:40-51) has zero callers — the guard was open-coded at command_tool.rs:163 instead of using the helper just added. Duplicated Code: the SendMode match cascade is repeated verbatim in both result branches (command_tool.rs:191-201 vs 213-222). Primitive Obsession: normalize_from is now a 5-branch string-parse cascade; one chrono parse-with-format-list retires it.

Spec

Round-4 finding	Status
Worst: unbounded post-wait drain hang	NOT FIXED on the supervisor path. 26a3a36 doesn't touch shell.rs; shell.rs:137-144 still awaits drain handles unguarded. Re-probe on current HEAD: ShellBackend::run("sh -c 'sleep 12 & echo hi'") did not return within 8s (grandchild holds write-end EOF). The timeout_fut is consumed by the select! on the wait branch — no guard remains. 4406413→26a3a36 claimed-fix still incomplete for this PR's own "Shell timeout" spec line.
Same shape in command_tool.rs	FIXED — 5s tokio::time::timeout around join! + warn! (:163-166), then try_recv flush (:168). Cost: bytes still in-flight at 5s are dropped for live-but-slow-EOF commands — acceptable vs hang, but undocumented behavior change.
Test gaps	OPEN — command_tool.rs has no #[cfg(test)] (no timeout-path test); shell_backend_timeout_kills_and_respects_config still asserts neither try_wait nor /proc death; memory_tools.rs has 0 tests (as_of dispatch untested).
python3 + 8s/10s flake in large-stdout test	UNCHANGED (shell.rs:254).
Dedupe rowid tiebreak + idx_facts_one_active	VERIFIED intact (memory/mod.rs:369-378); still untested.
CI-workflow scope creep	UNCHANGED — opencode.yml/opencode-review.yml still ride this memory PR.

---

Summary: Standards — 8 findings, worst: commit ships fmt-red and clippy-red code while its message overstates two fixes (drain_pipe collapse, Z normalization). Spec — 6 findings, worst: the round-4 headline hang is fixed only in command_tool — ShellBackend::run still wedges forever on a stdout-inheriting grandchild (shell.rs:137-144), untested. run cargo fmt && cargo clippy and finish the shell.rs guard before merge.

opencode session  |  github run

[opencode-agent]

Both axis reports verified against source at 8bec0d6. My independent ground-truth: cargo fmt --all --check clean, cargo clippy --all-targets -- -D warnings clean (round-5 red → fixed ✓). However, the first cargo test run failed 1/470 (469 passed); I could not reproduce the failure in 46+ subsequent runs — including 8 concurrent full-suite harnesses with --test-threads=4 on a 4-core box — all green. Suspects are the two wall-clock assertions below (Spec 4/5).

Standards

Round-5 claim verification for 8bec0d6

Claim	Verdict	Evidence
collapse drain_pipe/drain_err in shell.rs	FIXED — one generic drain<R> now	shell.rs:57-70
post-wait drain timeout guard	bounded, but see Spec §1	shell.rs:74-86
clippy & fmt	VERIFIED (run centrally)	clean at HEAD

Hard (documented-standard) issues

• Logging / silent errors (CLAUDE.md): Ok(0) | Err(_) => break conflates I/O error with EOF and swallows it (shell.rs:65, process.rs:27); let _ = tx.send(..) (process.rs:29); let _ = edit_message / delete_message (command_tool.rs:222/225/253/256) — while the streaming loop logs the identical edit call with warn! at :155-157. Inconsistent within one function's blast radius.

Judgement calls (baseline smells)

• Duplicated Code / Repeated Switches: SendMode match cascade still verbatim-duplicated in both result branches (command_tool.rs:212-228 vs 236-262). Round-5 finding, untouched.
• Speculative Generality (ironic — from this very commit): drain_pipe_timeout (process.rs:42-53) has zero callers; the guard was instead open-coded twice more — timeout(5s, join!) (command_tool.rs:180-184) and drain_with_timeout (shell.rs:74-86). Three post-wait guards, three implementations, one dead helper.
• Broken contract: normalize_from doc says "Canonicalize to UTC YYYY-MM-DD HH:MM:SS" (knowledge.rs:480) but seconds-less no-Z 2025-06-01T12:00 falls through all six branches to ts.replace('T', " ") (:509) → 2025-06-01 12:00, which mis-sorts against 2025-06-01 12:00:00 in every lexicographic compare. Add a %Y-%m-%dT%H:%M branch (the Z-branch at :491-496 already appends :00 — reuse it). Untested.
• Dead COALESCE(valid_to, valid_from) under WHERE valid_to IS NULL (memory/mod.rs:371); double-default map(...unwrap_or(-1)).unwrap_or(-1) (shell.rs:142).

Spec

Round-5 finding	Verdict
Unbounded post-wait drain hang (shell path)	FIXED (bounded) — probe-verified premise (parent exits, pipe holds no EOF) and guard placement
Seconds-less ISO no-Z	NOT FIXED (Standards §broken-contract)
stdout cap / multibyte lossy in shell path	NOT FIXED (shell.rs:168/180 uncapped vs MAX_BUFFER_CHARS=100_000 in command_tool; lossy per-chunk at shell.rs:66)
All 4 test gaps (a-d)	NOT FIXED — command_tool.rs 0 tests; timeout test asserts no child death; memory_tools.rs 0 tests; dedupe SQL untested
python3 dep + 8s/10s margin	NOT FIXED (shell.rs:254-280)
backend/mod.rs timeout(0s) = instant-fail vs optional_timeout 0=never	NOT FIXED — verified at mod.rs:154
CI-workflow scope creep + @latest	OPEN (hygiene: newline + permissions now OK; /oc triggers on any commenter — burns OPENCODE_API_KEY)

Worst — the new guard fails the scenario it was built for: drain_with_timeout returns String::new() on expiry (shell.rs:83), so for sh -c 'sleep 30 & echo hi' the hi is already captured in the task's String and is discarded purely because EOF never arrives. That contradicts the round-2 praised fix on the chat path, which retains partial output (output_buffer + try_recv flush, command_tool.rs:187-189). The drain task is also never aborted — its fd + growing buffer leak for the daemon's lifetime. Mirror command_tool's mpsc-sink shape (or handle.abort() after the guard and hand back what the shared buffer holds).

• Secondary: shell.rs awaits the stdout and stderr guards serially (up to 5+5s post-wait); command_tool joins them concurrently (5s). Contributes to the next item.
• Flake hazard (I observed 1/470 fail, unreproducible in 46 runs): shell_backend_timeout_kills_and_respects_config asserts elapsed < 5s while the post-kill drain guards allow 5s serially (1s timeout + kill + up to 10s drain is within the asserted budget only under luck); shell_backend_large_stdout_does_not_deadlock runs python3 on an 8s assert vs 10s job timeout. Loosen margins, drop python3 (dd if=/dev/zero bs=1k count=200 | tr '\0' 'x'), and assert child death (child.id()//proc) in the timeout test.

---

Summary: Standards — 6 findings, worst: normalize_from breaks its own canonicalization contract for seconds-less ISO timestamps that then mis-sort in every string compare. Spec — 8 findings, worst: the new drain guard discards already-captured output on the exact daemon-inherits-stdout case it exists for (shell.rs:83), plus an unreproducible 1/470 test flake pointing at the two wall-clock assertions. Fix those two, then this is mergeable.

opencode session  |  github run

[opencode-agent]

PR #54 Round-6 Re-Review (@ 3b3c6f0)

Ground truth run centrally: cargo fmt --all --check clean, cargo clippy --all-targets -- -D warnings clean, cargo test all pass (round-5's fmt/clippy redness was fixed at 8bec0d6 ✓).

⚠️ Commit-message mismatch on the only new commit: 3b3c6f0 claims "knowledge Z-suffix trim, process drain" but git show --stat proves it touches only src/command_tool.rs (+5/-3). Neither the normalize gap nor the supervisor drain gap it implies were addressed.

Standards

Prior-finding verification (S1–S8):

#	Finding	Status
S1	drain_pipe_timeout dead helper	NOT FIXED — src/utils/process.rs:42, zero callers repo-wide; guards still open-coded two ways (shell.rs:74 drain_with_timeout, command_tool.rs:182 timeout(5s, join!))
S2	normalize_from contract break	PARTIAL — Z-suffix handled (knowledge.rs:491-501), but seconds-less no-Z "2025-06-01T12:00" still falls through all parses to ts.replace('T', " ") (:509) → "2025-06-01 12:00", mis-compares vs "…12:00:00" at minute boundaries; test only covers …T12:00Z
S3	Swallowed errors (CLAUDE.md logging)	NOT FIXED — Ok(0) | Err(_) => break conflates I/O error with EOF (shell.rs:65, process.rs:27); let _ = tx.send (process.rs:29); let _ = edit/delete_message (command_tool.rs:223/226/258/261) while the identical call warn!s at :157
S4	SendMode cascade duplicated	NOT FIXED (command_tool.rs:216-229 vs :243-264)
S5	Dead COALESCE(valid_to, valid_from) + untested dedupe	NOT FIXED (memory/mod.rs:371)
S6	Double .unwrap_or(-1)	NOT FIXED (shell.rs:142)
S7	shell path stdout uncapped + per-chunk from_utf8_lossy splits multibyte	NOT FIXED (shell.rs:62-66; 3b3c6f0 capped only the chat-path display, not the backend capture)
S8	backend/mod.rs:154 0 = instant-fail vs optional_timeout 0 = never	NOT FIXED (latent; job default 600 masks it)

New in 3b3c6f0: magic number 3500 hardcoded at three sites (command_tool.rs:154/204/237) — extract a const. The labeled capped partial-output block itself is a genuine improvement on the chat path.

Spec

Prior-finding verification (P1–P7):

#	Finding	Status
P1	Worst: drain guard discards already-captured output	NOT FIXED on the supervisor path — shell.rs:81-84 still returns String::new() on expiry and never aborts the handle (fd + buffer leak); sh -c 'sleep 30 & echo hi' still loses hi. Guards still awaited serially (shell.rs:152-159, 5+5s). Chat path is sound (mpsc + try_recv flush)
P2	Wall-clock flake margins	PARTIAL — <5s assert vs 5s serial post-wait budget (shell.rs:253); 8s vs 10s python3 large-stdout test (:274)
P3	Test gaps	NOT FIXED — memory_tools.rs and command_tool.rs have 0 #[cfg(test)] blocks (verified grep -c); as_of dispatch, CommandTool timeout path, dedupe SQL untested; timeout test asserts no child death
P4	execute_timeout_secs 90 default, 0=off, min(job, sandbox)	VERIFIED (config.rs:205, shell.rs:29-31, main.rs:244/427)
P5	Cancel fix: None key + register-before-show	VERIFIED (telegram.rs:294; command_tool.rs:91 < :100, matches ADR-0005)
P6	Temporal facts: UNIQUE partial index, dedupe, auto-close, backfill, fact_history(as_of=)	VERIFIED (memory/mod.rs:368-381, knowledge.rs:367-379, memory_tools.rs:248-252)
P7	CI-workflow scope creep	UNCHANGED — opencode.yml/opencode-review.yml still ride this memory PR; @latest action remains unpinned vs CLAUDE.md CI convention (author previously marked YAGNI)

Spec-worst restated: the PR's "Shell timeout" line still under-delivers on the supervisor path — a timed-out/hung-EOF job reports empty output, the exact failure mode 3b3c6f0's message claims to fix.

---

Summary: Standards — 8 open (1 partial), worst: dead drain_pipe_timeout + three divergent timeout/drain idioms with silently swallowed errors against CLAUDE.md logging conventions. Spec — 3 open + 1 partial, worst: drain_with_timeout discards captured output and leaks the drain task (shell.rs:81-84) — the round-5 headline remains unfixed, and the new commit's message overstates its scope (command_tool.rs only).

opencode session  |  github run