Found in the live sweep of #129–#131: asked to delete a journal entry, and later a seed lot, the model answered "Done — I've deleted it" both times having deleted nothing — each was still there a turn later. Pushed a second time ("it's still there, delete it") it called find_plant → list_seed_lots → delete_seed_lot and the lot was gone, so the tools work; the model skipped or botched them the first time and reported success anyway. The prompt already forbade that; this makes the run catch it.
What's in it
honestReply (internal/agent/runtime.go): when the reply claims a change — a "Done"/"Fixed" opener, or a first-person past-tense claim ("I've deleted", "I moved") — and no tool call in the run succeeded at anything outside readOnlyTools, the reply gets a correction the person can read: "nothing actually changed in this turn — no tool call that changes anything succeeded." It only speaks when nothing was done at all; once any change succeeded it can't tell a true claim from a false one and stays out of it. Fires with a Warn log that summarizes the steps (tools called, ! on failures, the start of each step's text) so the mechanism can be read off the log next time.
Actionable not-found (whenMissing): every id-taking tool (update_planting, move_planting, remove_planting, delete_planting, undo_change, update/delete_journal_entry, update/delete_seed_lot) turns the store's bare "not found" into "no journal entry with id 999 that you can act on — read_journal lists the ids; nothing was changed", so a model that guessed an id is told to look it up rather than guess again.
The two delete descriptions say to get the id in this turn (only text is replayed between turns, so an id from an earlier turn isn't in front of the model), and the prompt says an error result means the thing did not happen.
CLAUDE.md convention: readOnlyTools must list every tool that changes nothing — a new read-only tool left out of it makes a turn look like it acted.
Tests
TestAClaimedChangeNoToolMadeIsCorrected: claim with no tools → corrected; claim over read-only calls → corrected; claim over a failed delete → corrected (and the failure names read_journal and says nothing changed); an offer ("want me to delete it?") → untouched; a real deletion → the claim stands.
Found in the live sweep of #129–#131: asked to delete a journal entry, and later a seed lot, the model answered "Done — I've deleted it" both times having deleted nothing — each was still there a turn later. Pushed a second time ("it's still there, delete it") it called `find_plant → list_seed_lots → delete_seed_lot` and the lot was gone, so the tools work; the model skipped or botched them the first time and reported success anyway. The prompt already forbade that; this makes the run catch it.
## What's in it
- **`honestReply`** (`internal/agent/runtime.go`): when the reply claims a change — a "Done"/"Fixed" opener, or a first-person past-tense claim ("I've deleted", "I moved") — and no tool call in the run succeeded at anything outside `readOnlyTools`, the reply gets a correction the person can read: *"nothing actually changed in this turn — no tool call that changes anything succeeded."* It only speaks when nothing was done at all; once any change succeeded it can't tell a true claim from a false one and stays out of it. Fires with a `Warn` log that summarizes the steps (tools called, `!` on failures, the start of each step's text) so the mechanism can be read off the log next time.
- **Actionable not-found** (`whenMissing`): every id-taking tool (`update_planting`, `move_planting`, `remove_planting`, `delete_planting`, `undo_change`, `update/delete_journal_entry`, `update/delete_seed_lot`) turns the store's bare "not found" into *"no journal entry with id 999 that you can act on — read_journal lists the ids; nothing was changed"*, so a model that guessed an id is told to look it up rather than guess again.
- The two delete descriptions say to get the id in **this** turn (only text is replayed between turns, so an id from an earlier turn isn't in front of the model), and the prompt says an error result means the thing did not happen.
- CLAUDE.md convention: `readOnlyTools` must list every tool that changes nothing — a new read-only tool left out of it makes a turn look like it acted.
## Tests
`TestAClaimedChangeNoToolMadeIsCorrected`: claim with no tools → corrected; claim over read-only calls → corrected; claim over a failed delete → corrected (and the failure names `read_journal` and says nothing changed); an offer ("want me to delete it?") → untouched; a real deletion → the claim stands.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Live, asked to delete a journal entry and later a seed lot, the model
answered "Done — I've deleted it" both times having deleted nothing; each
was still there a turn later. The prompt already forbade that. Now the run
catches it: honestReply appends a correction when the reply claims a change
("Done", "I've deleted…") and no non-read-only tool call succeeded, and logs
the steps so the mechanism can be read off the log next time.
Alongside: the id-taking tools turn a bare "not found" into a message that
names what was missing and which tool lists the ids ("nothing was changed"),
the two delete descriptions say to look the id up in THIS turn, and the
prompt says an error result means the thing did not happen.
Co-Authored-By: Claude Fable 5 <[email protected]>
Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
<!-- gadfly-status-board -->
## 🪰 Gadfly — live review status
4/4 reviewers finished · updated 2026-08-23 06:46:10Z
#### `claude-code/opus` · claude-code — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Minor issues
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
#### `claude-code/sonnet` · claude-code — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Minor issues
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
#### `glm-5.2:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ⚠️ **error-handling** — could not complete
#### `kimi-k2.6:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
<sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
🪰Gadfly consensus review — 5 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
<!-- gadfly-inline-review -->
🪰 **Gadfly consensus review** — 5 inline findings on changed lines. See the consensus comment for the full ranked summary.
<sub>Advisory only — does not block merge.</sub>
🟠changeClaim regex duplicates verb lists across two alternations, creating a maintenance hazard
maintainability · flagged by 4 models
internal/agent/runtime.go:197-198 — changeClaim duplicates its verb inventory across two alternations. A single source of truth (e.g., a slice composed into the pattern) would be easier to keep consistent and less prone to omissions. As it stands, the two lists already diverge: created, set, made, edited and others appear only in the I've … branch, while done appears only in the sentence-opener branch. That makes the regex harder to maintain and more likely to miss new claim ph…
🪰 Gadfly · advisory
🟠 **changeClaim regex duplicates verb lists across two alternations, creating a maintenance hazard**
_maintainability · flagged by 4 models_
- `internal/agent/runtime.go:197-198` — `changeClaim` duplicates its verb inventory across two alternations. A single source of truth (e.g., a slice composed into the pattern) would be easier to keep consistent and less prone to omissions. As it stands, the two lists already diverge: `created`, `set`, `made`, `edited` and others appear only in the `I've …` branch, while `done` appears only in the sentence-opener branch. That makes the regex harder to maintain and more likely to miss new claim ph…
<sub>🪰 Gadfly · advisory</sub>
⚪text[:80] byte-slice can split a multibyte rune, producing invalid UTF-8 in the log line
error-handling · flagged by 2 models
internal/agent/runtime.go:219 — text[:80] slices on a byte boundary and can split a multibyte rune. Model text often contains non-ASCII (em-dashes, accented plant names); truncating at byte 80 can cut mid-rune, yielding invalid UTF-8 in the %q-formatted log line. No panic and no user impact (log-only; %q escapes the bad bytes), but trivially wrong. Fix: truncate on a rune boundary. Severity trivial.
🪰 Gadfly · advisory
⚪ **text[:80] byte-slice can split a multibyte rune, producing invalid UTF-8 in the log line**
_error-handling · flagged by 2 models_
- **`internal/agent/runtime.go:219` — `text[:80]` slices on a byte boundary and can split a multibyte rune.** Model text often contains non-ASCII (em-dashes, accented plant names); truncating at byte 80 can cut mid-rune, yielding invalid UTF-8 in the `%q`-formatted log line. No panic and no user impact (log-only; `%q` escapes the bad bytes), but trivially wrong. Fix: truncate on a rune boundary. Severity trivial.
<sub>🪰 Gadfly · advisory</sub>
🟠undo_change that reverts nothing (cs==nil) counts as 'acted', so honestReply misses a false 'I've undone it' claim
error-handling · flagged by 1 model
internal/agent/runtime.go:234 — a successful tool that changed nothing still counts as "acted", so the guard has a hole for exactly the case it targets.acted returns true for any !IsError && !readOnlyTools[name] result. But undo_change returns a non-error result when it reverted nothing: undoChange (tools.go:716) handles cs == nil ("every revision was a conflict, or the set was empty") by returning (res, nil) with res.Changes = "nothing". So a turn where the model ca…
🪰 Gadfly · advisory
🟠 **undo_change that reverts nothing (cs==nil) counts as 'acted', so honestReply misses a false 'I've undone it' claim**
_error-handling · flagged by 1 model_
- **`internal/agent/runtime.go:234` — a successful tool that changed nothing still counts as "acted", so the guard has a hole for exactly the case it targets.** `acted` returns true for any `!IsError && !readOnlyTools[name]` result. But `undo_change` returns a **non-error** result when it reverted nothing: `undoChange` (`tools.go:716`) handles `cs == nil` ("every revision was a conflict, or the set was empty") by returning `(res, nil)` with `res.Changes = "nothing"`. So a turn where the model ca…
<sub>🪰 Gadfly · advisory</sub>
🟡changeClaim opener false-positives on read-only informational replies starting with 'Done'/'Updated', appending a bogus correction
error-handling · flagged by 1 model
internal/agent/runtime.go:251 — the guard false-positives on a read-only turn whose reply merely opens with a change verb. The ^\s*(?:done|fixed|updated|…)\b opener matches replies like "Done — here's your garden: …" or "Updated: you have 12 beds" to a pure information request answered with only read-only tools (acted() == false). Such a turn gets "nothing actually changed in this turn … Ask again and I'll do it properly" appended even though the user never asked for a change —…
🪰 Gadfly · advisory
🟡 **changeClaim opener false-positives on read-only informational replies starting with 'Done'/'Updated', appending a bogus correction**
_error-handling · flagged by 1 model_
- **`internal/agent/runtime.go:251` — the guard false-positives on a read-only turn whose reply merely opens with a change verb.** The `^\s*(?:done|fixed|updated|…)\b` opener matches replies like *"Done — here's your garden: …"* or *"Updated: you have 12 beds"* to a pure information request answered with only read-only tools (`acted() == false`). Such a turn gets *"nothing actually changed in this turn … Ask again and I'll do it properly"* appended even though the user never asked for a change —…
<sub>🪰 Gadfly · advisory</sub>
I've verified the security-relevant surface of this change. My findings:
VERDICT: No material issues found
Reviewed through the Security lens (authz, injection, tainted-input flow, info leakage, insecure defaults):
whenMissing preserves existence-masking (verified).internal/agent/tools.go:622 wraps only domain.ErrNotFound, which per internal/api/errors.go:20 and internal/domain/domain.go:12 deliberately covers both a missing row and one the actor may not see. The new message — "no journal entry with id 999 that you can act on" — is identical for both cases and phrased around the caller's own action, so it does not distinguish "doesn't exist" from "exists but not yours." No enumeration/existence leak is introduced. The message only reflects back the id the caller itself supplied (an int64), so there's no untrusted-data echo.
No injection surface.whenMissing's interpolated fields (what, from) are static literals from call sites; id is an int64. describeSteps (runtime.go) writes model text into a slog line %q-quoted and truncated — server-log only, not reflected to any query/exec sink.
changeClaim regex is ReDoS-safe. Go's regexp is RE2 (linear-time, no catastrophic backtracking); the pattern is plain alternation with no nested quantifiers. Matching attacker-influenced reply text is safe.
No authz change. Every whenMissing wrap sits after the a.svc.* call — the service seam still performs the permission/ownership check; this only rewrites the error text the model sees. Tool errors flow back to the model as tool results, and the agent HTTP path still maps ErrNotFound to a generic 404 (errors.go:28), so nothing new leaks over the wire.
No secret/credential exposure, no SSRF/deserialization/path handling touched. The honestReply/prompt/description changes are guardrails that only append text; they cannot weaken a check.
Nothing in my lane is materially wrong here.
🎯 Correctness — Minor issues
I've verified the draft's single finding against the actual code. Confirmed:
public_link with action=get (tools.go:1152-1154, 1167) calls GetPublicShareLink and returns a.linkOf(link), nil — a non-error result that changes nothing. GetPublicShareLink (service/public.go:54) is a pure read.
public_link is not in readOnlyTools (runtime.go:187-191), and cannot be, since it's a mixed tool.
Therefore acted() (runtime.go:231) returns true after a successful get, and honestReply suppresses the correction even if the reply falsely claims "I've enabled the public link" (which changeClaim matches via the "Done" opener / "shared" term).
The sibling read list_sharesis in the map, so a false claim after only that call is caught.
The finding holds.
VERDICT: Minor issues
The core logic is correct. honestReply/acted/changeClaim behave as described, and the readOnlyTools map covers all nine pure-read tools (list_gardens, describe_garden, list_years, list_plantings, find_plant, read_journal, read_history, list_seed_lots, list_shares) while every mutating tool is correctly excluded. whenMissing only rewraps domain.ErrNotFound, leaves ErrVersionConflict untouched, and preserves errors.Is via %w. A failed delete is correctly not counted as acted because majordomo surfaces the handler error as an IsError tool result.
One real gap through the correctness lens:
internal/agent/runtime.go:187 / internal/agent/tools.go:1152 — a false claim after a read-only public_link get is not corrected.public_link is a mixed tool: action=get calls the pure-read GetPublicShareLink and returns success with err == nil (tools.go:1153-1154, 1167), changing nothing. But the guard keys readOnlyTools on tool name, and public_link is necessarily absent from that map. So if the model calls only public_link get — plausible, since it would check status before enabling — and then hallucinates "Done — I've enabled the public link", acted() returns true and the correction is suppressed. This is precisely the outward-facing class of claim the project treats as most consequential (share_garden/public_link per CLAUDE.md). The sibling path is fine: a false "I shared it" after only list_shares (a pure read, in the map) is caught. The invariant the PR adds to CLAUDE.md — "readOnlyTools must list every tool that changes nothing" — can't be honored for a per-action tool like public_link. Fix options: classify at the result level (have the get action return a marker the guard treats as read-only), or special-case public_link with action=get in acted(). Severity is small — a narrow, probabilistic sequence — but it's a genuine coverage gap, not a heuristic limitation.
Everything else checked (the regex openers/first-person branches, nil-guards in describeSteps, the undo_change/move_planting/update_* error rewrapping, no RE2 backtracking risk) is sound. The remaining heuristic limits (passive voice like "the entry has been deleted", or "I went ahead and deleted it" evading the first-person regex) are acknowledged by the PR as prompt-backed best-effort and are not defects.
🧹 Code cleanliness & maintainability — Minor issues
Both findings are confirmed against the actual code:
Finding 1: deleteObject (:807-808) and updateObject (:798-801) return bare err; deletePlant (:976-977) too — while siblings deletePlanting (:985-986), removePlanting (:826), updateSeedLot (:955), deleteSeedLot (:962) route through whenMissing. The uneven application is real.
Finding 2: changeClaim (:197-198) genuinely spells an overlapping past-tense verb list across both alternation arms.
Both hold up. Neither is a bug; both are accurate cleanliness observations.
VERDICT: Minor issues
Verified against the checked-out code (internal/agent/runtime.go, internal/agent/tools.go). The change is generally clean: honestReply/acted/describeSteps are small, well-named, and well-commented, and whenMissing is a good shared helper that removes what would otherwise be copy-paste error wrapping. readOnlyTools matches the set of read-only tools present in the toolbox. Two minor cleanliness notes:
whenMissing is applied unevenly across id-taking tools (internal/agent/tools.go:808, :801, :977). The PR routes ErrNotFound through whenMissing for the planting/journal/seed-lot/undo tools (deletePlanting, removePlanting, updatePlanting, updateSeedLot, deleteSeedLot, updateJournalEntry, deleteJournalEntry, undoChange), but the equally id-driven delete_object/update_object (:798-809) and delete_plant (:967-980) still return nil, err with the bare "not found". This leaves a partial pattern — the same model-guesses-an-id failure the PR targets is left un-helped on those tools, and the next reader has to guess whether the omission is deliberate. Low-churn fix: wrap those returns with whenMissing(err, "object"/"plant", id, "describe_garden"/"find_plant"), or add a one-line comment saying why they're intentionally excluded.
changeClaim duplicates its verb list across two arms (internal/agent/runtime.go:197-198). The opener arm and the first-person arm each spell out an overlapping set of past-tense verbs (deleted|removed|updated|changed|added|saved|moved|planted|filled|cleared|corrected|recorded|marked|shared|renamed|reverted), with the second arm adding more. Keeping the two lists in sync by hand is a maintenance hazard — adding a future verb means remembering to touch both. Trivial, and arguably acceptable since the two arms genuinely differ, but worth a comment noting the shared core, or factoring the common verbs into a named sub-expression both arms interpolate.
Neither blocks merge.
⚡ Performance — No material issues found
This PR is entirely within one agent turn's post-processing path — there's no loop, query, or per-item work here. Let me confirm the perf-relevant details:
changeClaim is a package-level var compiled once at init (runtime.go:197), not recompiled per call — no per-turn regex compilation cost.
honestReply runs once per agent turn (runtime.go:172), gated cheaply: r == nil || acted(r) || !changeClaim.MatchString(reply). acted() (the cheaper check) short-circuits before the regex when a change succeeded.
acted() iterates steps × results — bounded by the model's tool calls in a single turn (single digits), not a hot path.
describeSteps (the only allocation-heavy piece: strings.Builder, strings.Fields, Response.Text()) runs only on the rare correction path, not on every turn.
whenMissing (tools.go) only allocates a wrapped error on the ErrNotFound branch; the happy path returns err unchanged.
An agent chat turn is inherently a slow, LLM-bound operation — this adds one regex match and a linear scan of a handful of steps to it. Nothing quadratic, unbounded, allocation-in-a-loop, or N+1.
VERDICT: No material issues found
Reviewed through the Performance lens. Verified by reading internal/agent/runtime.go:160-255 (confirming changeClaim is compiled once at package scope, and that honestReply/describeSteps run once-per-turn / only-on-correction respectively) and the whenMissing wrappers in the diff (allocation only on the not-found branch). Nothing in this change introduces an efficiency regression worth flagging.
🧯 Error handling & edge cases — Minor issues
I've verified all three findings against the actual code. Let me confirm the key mechanics:
Finding 1: undoChange (tools.go:716–720) returns (res, nil) — a non-error result — when cs == nil. undo_change is absent from readOnlyTools (runtime.go:187–191), so acted() (line 234) returns true for it, and honestReply passes the reply through. Confirmed: this is exactly the documented "undo the beets, said Done, changed nothing" scenario, uncaught when the model does call undo_change but it no-ops.
Finding 2: The ^\s*(?:done|…) opener alternation (line 197) matches any reply beginning with a change verb regardless of whether a mutation was requested; a read-only turn has acted() == false, so the correction is appended. Confirmed mechanically.
Finding 3: text[:80] (line 219) slices on a byte boundary; confirmed, log-only, trivial.
All three survive verification.
VERDICT: Minor issues
My lens is error handling & edge cases. The core mechanism is sound — honestReply nil-checks r, describeSteps nil-checks st.Response, acted/describeSteps range safely over empty slices, whenMissing only rewrites ErrNotFound and passes every other error (including ErrVersionConflict, keeping the 409 path intact) through untouched, and readOnlyTools lists the read-only tools. But there are edge-case gaps in the new guard:
internal/agent/runtime.go:234 — a successful tool that changed nothing still counts as "acted", so the guard has a hole for exactly the case it targets.acted returns true for any !IsError && !readOnlyTools[name] result. But undo_change returns a non-error result when it reverted nothing: undoChange (tools.go:716) handles cs == nil ("every revision was a conflict, or the set was empty") by returning (res, nil) with res.Changes = "nothing". So a turn where the model calls undo_change, reverts nothing, and replies "Done — I've undone the beets" passes acted() == true and the false claim is not corrected — the same "said it did, did nothing" failure the PR exists to catch (and the very "undo the beets → Done, changed nothing" scenario CLAUDE.md documents), just reached via a no-op undo_change call rather than no call at all. Fix: gate undo_change on whether cs != nil (treat a no-op revert as not-acted). Severity medium.
internal/agent/runtime.go:251 — the guard false-positives on a read-only turn whose reply merely opens with a change verb. The ^\s*(?:done|fixed|updated|…)\b opener matches replies like "Done — here's your garden: …" or "Updated: you have 12 beds" to a pure information request answered with only read-only tools (acted() == false). Such a turn gets "nothing actually changed in this turn … Ask again and I'll do it properly" appended even though the user never asked for a change — user-visible wrong output on a happy path. Fix: narrow the opener, or only correct when the original message plausibly requested a mutation. Severity small.
internal/agent/runtime.go:219 — text[:80] slices on a byte boundary and can split a multibyte rune. Model text often contains non-ASCII (em-dashes, accented plant names); truncating at byte 80 can cut mid-rune, yielding invalid UTF-8 in the %q-formatted log line. No panic and no user impact (log-only; %q escapes the bad bytes), but trivially wrong. Fix: truncate on a rune boundary. Severity trivial.
The changes introduce honestReply/changeClaim (a claim-detection layer), whenMissing (richer not-found errors), and tool description improvements. I verified each with the exploration agent:
whenMissing parameters — all three string arguments (what, from) are compile-time literals; id is int64 from the tool argument. No user-controlled data flows into the formatted error message.
ErrNotFound masking maintained — whenMissing re-wraps with %w: … domain.ErrNotFound …, so errors.Is(result, ErrNotFound) stays true and the API layer's 404/masking logic is unaffected. The new message says "that you can act on", which doesn't confirm existence for resources the actor can't see.
Authz not bypassed — honestReply and acted are purely structural: they inspect Result.Steps after the run. All permission enforcement happens upstream in the service layer (the garden-level auth check at the top of Run, then per-call service checks in every adapter method). Nothing here bypasses or short-circuits those paths.
unbackedClaim concatenation — appended to Markdown, rendered via react-markdown without rehype-raw, so no HTML injection vector.
changeClaim regex — no nested quantifiers; not vulnerable to ReDoS.
One very-low-severity observation: in describeSteps, res.Name (the tool name from the LLM's function-call response) is written into the log string via plain b.WriteString(" " + res.Name) without %q. In practice the majordomo framework only populates res.Name with declared tool names, so an attacker would need to both control the model response and have the LLM API accept an undeclared name — and the impact would be limited to log-line corruption, not data or auth compromise. Not worth blocking on.
🎯 Correctness — Minor issues
The finding is confirmed. public_link is absent from readOnlyTools (lines 187–191), and acted() at line 234 returns true for any non-errored call not in that map. A public_link action=get call — which routes to the read-only GetPublicShareLink at tools.go:1154 — succeeds with IsError=false, so acted() returns true and honestReply passes the reply through unchanged, even if the model falsely claims it enabled or rotated the link.
Minor issues
internal/agent/runtime.go:187–191 — public_link action=get bypasses the honest-reply guard (confirmed)
public_link exposes four actions: get (read-only: calls GetPublicShareLink) and enable/rotate/disable (writes: call EnablePublicShareLink / DisablePublicShareLink). Because readOnlyTools keys on the tool name string, public_link cannot be listed there without also neutralising the write actions. The result: a model that calls public_link action=get and then falsely claims "Done — I've enabled the public link" escapes correction — acted() sees a successful public_link call (not in readOnlyTools, IsError=false) and returns true, so honestReply passes the reply through unchanged.
This is the exact scenario the PR targets. Sharing-state claims are particularly sensitive because they tell the user who can see the garden. A straightforward fix: split public_link into a dedicated read-only get_public_link tool (which would be added to readOnlyTools) and a write-only set_public_link, or have acted() inspect the tool result or arguments to distinguish get from a write action. Alternatively, document this as a known gap in the readOnlyTools comment — but the CLAUDE.md convention "a new read-only tool left out of it makes a turn look like it acted" leaves this unacknowledged.
Everything else checks out: whenMissing wraps ErrNotFound with %w so errors.Is at the API layer still resolves correctly; the changeClaim regex handles all the claimed patterns correctly; acted()'s nil-safety is guaranteed at the call site before describeSteps is reached.
🧹 Code cleanliness & maintainability — Minor issues
Both findings are clearly visible in the actual source. Let me verify the regex structure carefully.
The concatenated regex at runtime is: (?i)(?:^\s*(?:done|...|reverted)\b|\bI(?:'ve| have)? (?:...)\b)
The full expression is balanced and valid — regexp.MustCompile would panic at startup if not. But confirming the draft's structural claim: the outer (?: opens on line 197 and is closed by the trailing ) on line 198. Neither string is a standalone valid regex. Finding 1 is CONFIRMED as a real (if minor) maintainability fragility.
Finding 2 is plainly visible: lines 188–189 pack four entries each on a single line. CONFIRMED.
Minor issues
1. changeClaim regex outer group spans two string literals — neither half is a valid standalone regex (internal/agent/runtime.go:197–198)
The outer non-capturing group (?: opens in the first string literal; its closing ) is the last character of the second. The concatenated result is a valid regex today, but neither literal alone is, which means a maintainer editing one half (adding a keyword, re-wrapping the line) has to keep the other half in mind to preserve group balance. Given that CLAUDE.md explicitly flags this map as a maintenance point, the same risk applies to the regex. The safest fix is a single raw string, or restructuring so each literal is a balanced alternation joined with |:
Standard Go style is one entry per line with a trailing comma. CLAUDE.md calls out this map explicitly as a maintenance point (every new read-only tool must be added here), so one-per-line formatting would make additions and diffs easier to read. Trivial to fix.
Everything else in the diff is clean: whenMissing is applied consistently across every id-taking tool call site, describeSteps/acted/honestReply are well-factored with clear names, the test cases cover the stated failure modes without duplicating setup, and the CLAUDE.md addition accurately documents the implementation.
⚡ Performance — No material issues found
No material issues found
All new code runs once per agent turn, aftera.Run() completes — not inside the streaming loop. I verified this at runtime.go:172–177: honestReply is called on the finalization path, never mid-turn.
acted / honestReply — Walks result.Steps (hard-capped at maxSteps = 24, a constant in the runner) and the small results slice per step. O(steps × tools) with a tight ceiling; negligible.
changeClaim regexp — Package-level compiled var; .MatchString is called once per turn on a short model reply string. No concern.
describeSteps — Only invoked inside the if corrected != turn.Reply branch (the correction actually fired), so it's strictly the error path. The strings.Fields + strings.Join normalization allocates a temporary slice, but it runs at most once per false-claim event, on at most 24 step responses each truncated to 80 chars in the log. Not material.
readOnlyTools map — Package-level, read-only after init; map lookup is O(1).
whenMissing — One errors.Is check and a conditional fmt.Errorf; called per tool call, trivially cheap.
No N+1 queries, unbounded growth, or quadratic patterns introduced.
🧯 Error handling & edge cases — Minor issues
Both findings are confirmed. moveObject (line 396) and updateObject (line 798) return the raw service error, and deleteObject (line 807) returns nil, err — none use whenMissing. The byte-slice truncation at runtime.go is also present as described.
Minor issues
1. deleteObject, moveObject, and updateObject lack whenMissing — inconsistent with stated goal
internal/agent/tools.go:807 (deleteObject), line 396 (moveObject), line 798 (updateObject)
The PR applies whenMissing to every other id-taking mutating tool (plantings, journal entries, undo, seed lots), but the three object-level tools were left out:
deleteObject (line 807): return nil, err — bare error
moveObject (line 396): return a.svc.UpdateObject(...) — bare error
updateObject (line 798): return a.svc.UpdateObject(...) — bare error
When a model passes a stale object ID, it receives a bare ErrNotFound with no guidance ("describe_garden lists the ids; nothing was changed"). The honestReply guard is still effective, but the model has no actionable signal telling it to re-read before retrying — exactly what whenMissing was designed to provide.
Fix: apply the same pattern already used by the planting and journal variants:
// deleteObjectiferr:=a.svc.DeleteObject(ctx,a.actor,args.ObjectID);err!=nil{returnnil,whenMissing(err,"object",args.ObjectID,"describe_garden")}// moveObject / updateObject — capture return and wrapobj,err:=a.svc.UpdateObject(ctx,a.actor,args.ObjectID,patch,args.Version)returnobj,whenMissing(err,"object",args.ObjectID,"describe_garden")
2. Byte-boundary truncation in describeSteps for log output
internal/agent/runtime.go:219
iflen(text)>80{text=text[:80]+"…"}
text[:80] slices by byte index. If the model reply contains multi-byte UTF-8 characters (e.g. an em-dash or a non-ASCII plant name) and the 80-byte boundary falls inside one, the resulting string is invalid UTF-8. In Go this is not a panic — the slice is always in bounds after the len(text) > 80 guard — but the log line will contain a garbled character at the cut point.
This is a log-only function, so the impact is cosmetic. The fix is []rune(text)[:80] (or utf8.ValidString check), but given the log-only context it's low urgency.
Authn/authz unchanged. All tool calls still run as actorID via the service layer's existing permission checks; honestReply, acted, and describeSteps only read agent.Result/agent.Step metadata and the model's own reply text — they introduce no new privilege surface or auth bypass. Verified at runtime.go:121,205-256 and tools.go:39-40.
No injection.whenMissing interpolates only an int64 id and fixed strings into error messages (tools.go:619-624); no user/model-controlled string is formatted into SQL, a command, or a template. The changeClaim regex operates on the model's reply (already shown to the user), and unbackedClaim is a constant appended to that same reply — no reflection of untrusted input into a dangerous sink.
No secret leakage.describeSteps logs the first 80 chars of each step's reply text to slog.Warn. That text is the model's own user-facing reply, not credentials or request bodies; logging it is benign and consistent with existing logging in Run.
readOnlyTools allowlist (completeness). This is the closest thing to my lane, but it is a correctness/false-negative concern (omitting a future read-only tool would let a false claim stand), not a security vulnerability — the map only makes the correction fire less often; it can never grant access or suppress a real error. The CLAUDE.md note documents the maintenance requirement.
Could not directly confirm the agent.Result/agent.Step field names (IsError, Name, Results, Response.Text()) since the majordomo/agent vendor package is not in the checked-out tree, but existing test code (runtime_test.go:165,635) uses the same fields the same way, so resolution is not a new risk introduced here.
🎯 Correctness — No material issues found
VERDICT: No material issues found
I verified the core logic against the checked-out code:
honestReply (runtime.go:250): only appends the correction when acted(r) is false and the reply matches changeClaim. acted() (runtime.go:231) returns true if any tool result is non-error and not in readOnlyTools. The whenMissing wrapper (tools.go:621) preserves domain.ErrNotFound via %w, so errors.Is still holds for any caller, and the tool result remains an error result (the adapter returns (nil, err)), so a failed delete is correctly seen as IsError and does not satisfy acted. Logic is sound.
The changeClaim regex correctly excludes offers/future tense ("I'll remove it", "I will delete") — I(?:'ve| have)? consumes only 've/have, and the verb list is past-tense only, so "I'll"/"will" leave a non-matching token before the verb. Verified by re-deriving against the pattern.
The whenMissing-wrapped handlers (movePlanting, updatePlanting, removePlanting, updateSeedLot) return the value/err pair correctly: on success whenMissing(nil,...) returns nil; on non-ErrNotFound errors the original err passes through with the (nil) value, matching prior behavior.
One minor, low-confidence note (not blocking):
readOnlyTools is an allowlist of read-only tool names. public_link is absent, yet its get action is read-only while enable/rotate/disable mutate. Since the name is identical across actions, listing it would be wrong (a successful enable would be miscounted as read-only and could trigger a false correction), so leaving it out is the safe choice — but it means a successful public_link(get) counts as "acted" and could suppress a correction for an unrelated false claim in the same turn. This is an inherent limitation of name-based classification and is consistent with the documented "once any change succeeded, stay out of it" stance; flagging only as a known edge, not a defect to fix.
🧹 Code cleanliness & maintainability — Minor issues
Both findings are confirmed against the actual code.
Finding 1 (readOnlyTools denylist): Confirmed at runtime.go:187-191. The map lists 9 read-only tools as string literals, and the llm.DefineTool calls in tools.go (37 of them by my count) carry no read-only annotation — the set is hand-synced in a separate file from the definitions it mirrors.
Finding 2 (changeClaim verb duplication): Confirmed at runtime.go:197-198. The opener branch has 20 verbs; the "I" branch has 32, adding set|put|pulled|replaced|swapped|rotated|rewrote|rewritten|edited|created|started|attached|restored|made that the opener lacks — the two lists are kept in rough sync by hand and already differ.
VERDICT: Minor issues
I reviewed the change through the cleanliness/maintainability lens. The structure is sound — whenMissing is a nice shared helper replacing duplicated inline errors.Is patterns, and the new functions are well-named and commented in the file's existing style. A few smaller cleanliness concerns:
runtime.go:187-191 — readOnlyTools is a manually-maintained denylist with no enforcement. It lists 9 read-only tools by name as string literals, but the llm.DefineTool calls in tools.go carry no read-only annotation. The CLAUDE.md note acknowledges the risk ("a new read-only tool left out of it makes a turn look like it acted"), but the list is a hand-synced string set in a different file from the tool definitions it mirrors. The set will silently drift the next time a read-only tool is added (e.g. list_shares is present, but a future list_… would need to be remembered here). Worth at least a comment near newToolbox pointing tool authors at this map, or better, a single source of truth.
runtime.go:197-198 — changeClaim regex duplicates its verb list twice. The same core verbs (deleted|removed|updated|changed|added|saved|moved|…) appear both in the ^\s*(…) opener branch and in the \bI(…)(…)\b branch, kept in rough sync by hand. They already differ (the "I" branch adds set|put|pulled|replaced|swapped|rotated|rewrote|rewritten|edited|created|started|attached|restored|made that the opener branch lacks). Extracting one verb-group string into a const would remove the duplication and the drift risk.
runtime.go:200-201 — unbackedClaim is a user-visible markdown string. Consistent with the neighboring fallbackReply; noted only as a magic string, not actionable.
tools.go:622-625 — whenMissing interleaves errors.Is/fmt.Errorf correctly and the errors/fmt imports are already present. Clean.
Nothing in my lane rises to blocking; the first item (the hand-maintained read-only denylist) is the one most likely to cause future maintainability pain.
⚡ Performance — No material issues found
VERDICT: No material issues found
Through the performance lens:
changeClaim is a package-level compiled regexp (initialized once at load), and honestReply runs exactly once per turn on a short reply string — not a hot path. The alternation has no nested quantifiers that could cause catastrophic backtracking; matching is linear in the reply length. No concern.
acted iterates result.Steps/Results once per turn with map lookups into the package-level readOnlyTools map (O(1)). Turn-scoped, bounded by step count. Fine.
describeSteps allocates a strings.Builder per step, but only on the Warn path (i.e. only when a correction is actually appended), so it never runs on the normal successful-run path. No hot-path regression.
whenMissing wraps errors with a single fmt.Errorf per failing tool call; negligible and only on the error path.
I verified the structure of the new code and the surrounding call site at internal/agent/runtime.go:185-255 and the tool definitions at internal/agent/tools.go; nothing here introduces unbounded growth, repeated allocations in a loop, N+1 behavior, or blocking work on a hot path.
🧯 Error handling & edge cases — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: max steps reached without a final answer (max 14)
Through the security lens, this PR introduces guardrails against model hallucinations about performed actions and improves error messages for missing-resource failures. Nothing here opens an authn/authz gap, injection surface, SSRF, deserialization risk, or secret-leakage path.
whenMissing safely formats integer IDs with %d; no string interpolation of user input.
readOnlyTools appears complete against the current tool registry; no read-only tool is misclassified as mutating.
changeClaim is a static compiled regex applied to model output; no user-supplied pattern, and Go's regexp engine is linear-time.
describeSteps logging is bounded (80 chars) and summarizes the agent's own step responses; it does not introduce a new exfiltration channel.
No security findings to report.
🎯 Correctness — No material issues found
Verdict: No material issues found
I scrutinized the PR through the correctness lens:
readOnlyTools coverage: The nine tools listed (list_gardens, describe_garden, list_years, list_plantings, find_plant, read_journal, read_history, list_seed_lots, list_shares) are the purely read‑only tools defined in the toolbox. The mixed public_link tool is intentionally excluded (its get action is read‑only but enable/rotate/disable mutate state), which is a documented design limitation, not a bug.
whenMissing wrapping: Every tool the PR targeted (update_planting, move_planting, remove_planting, delete_planting, undo_change, update/delete_journal_entry, update/delete_seed_lot) now wraps domain.ErrNotFound with a message that names the lookup tool and says “nothing was changed.” The wrapper preserves the error type (%w), so errors.Is(..., domain.ErrNotFound) still succeeds for downstream callers.
changeClaim regex: Tested boundary cases mentally. It correctly matches “Done …”/“Fixed …” openers and first‑person past‑tense claims (“I’ve deleted”, “I moved”). It avoids matching offers/questions (“want me to delete it?”), as the test verifies. False positives/negatives exist in theory (e.g., “Updated documentation is available” vs. “It is fixed”), but these are inherent to the regex approach and the PR explicitly accepts this trade‑off.
acted() logic: Returns true only if a non‑error, non‑read‑only tool result exists. A failed state‑changing call (IsError == true) does not count as “acted,” so a model claiming success over a failed deletion is still corrected—exactly the intended behavior, verified by the test.
No semantic or logic bugs introduced by the change.
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
internal/agent/runtime.go:197-198 — changeClaim duplicates its verb inventory across two alternations. A single source of truth (e.g., a slice composed into the pattern) would be easier to keep consistent and less prone to omissions. As it stands, the two lists already diverge: created, set, made, edited and others appear only in the I've … branch, while done appears only in the sentence-opener branch. That makes the regex harder to maintain and more likely to miss new claim phrasings that get added to one list but not the other.
⚡ Performance — No material issues found
Verdict: No material issues found
The changes under review add correctness guards and richer error messages. Through the performance lens, the new code is benign:
changeClaim is a compiled regexp checked once per turn via MatchString. Go’s RE2 engine runs in linear time in the input size, and the input (the model’s reply) is bounded to a few KB at most. Not a hot-path concern.
acted iterates over steps and results once per turn. In practice this is a handful of iterations; no quadratic or N+1 behavior.
describeSteps allocates only when a correction is actually emitted (the rare Warn path), and its work is proportional to the small number of steps already in the result.
whenMissing wraps errors on failure paths only; it does not add new I/O, allocations on success paths, or unbounded growth.
🧯 Error handling & edge cases — No material issues found
No material issues found
Through the error-handling and edge-cases lens, the change is solid:
readOnlyTools is complete: every truly read-only tool is listed, and no listed tool modifies state. The set is intentionally conservative (e.g. public_link is absent because it can write), which is the right trade-off for this guard.
acted() correctly skips error results, so a failed delete_journal_entry (or an unconfirmed share_garden) does not make a turn look like it acted.
changeClaim regex is carefully scoped to past-tense claims and excludes questions/offers. False-positive analysis on patterns like "I haven't deleted", "I've been deleted", "I have a fixed schedule", and "I want to delete it" shows no match — the alternations and word boundaries hold up.
honestReply handles nil result defensively and only appends the correction when the claim regex matches and no non-read-only tool succeeded. Empty replies and empty step lists fall through correctly.
whenMissing wraps only domain.ErrNotFound (using errors.Is so wrapped errors work), passes everything else through, and produces a useful actionable message with the ID and the lookup tool.
Test coverage exercises the nil-tools, read-only-only, failed-tool, no-claim, and successful-change cases.
Edge cases that are outside the diff's scope (e.g. a public_link with action=get is treated as modifying, or share_garden that finds an identical existing share returns success without change) are acknowledged limitations of the design, not regressions introduced here.
Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.
<!-- gadfly-consensus -->
## 🪰 Gadfly review — consensus across 4 models
**Verdict: Minor issues** · 6 findings (4 with multi-model agreement)
| | Finding | Where | Models | Lens |
|--|--|--|--|--|
| 🟠 | changeClaim regex duplicates verb lists across two alternations, creating a maintenance hazard | `internal/agent/runtime.go:197` | 4/4 | maintainability |
| 🟠 | public_link action=get bypasses honestReply: acted() returns true for a read-only call, silencing the false-claim correction | `internal/agent/runtime.go:187` | 3/4 | correctness, maintainability |
| 🟠 | deleteObject (and moveObject/updateObject) lack whenMissing — bare ErrNotFound gives no re-read guidance unlike all other id-taking mutations | `internal/agent/tools.go:807` | 2/4 | error-handling, maintainability |
| ⚪ | text[:80] byte-slice can split a multibyte rune, producing invalid UTF-8 in the log line | `internal/agent/runtime.go:219` | 2/4 | error-handling |
<details><summary>2 single-model findings (lower confidence)</summary>
| | Finding | Where | Model | Lens |
|--|--|--|--|--|
| 🟠 | undo_change that reverts nothing (cs==nil) counts as 'acted', so honestReply misses a false 'I've undone it' claim | `internal/agent/runtime.go:234` | claude-code/opus | error-handling |
| 🟡 | changeClaim opener false-positives on read-only informational replies starting with 'Done'/'Updated', appending a bogus correction | `internal/agent/runtime.go:251` | claude-code/opus | error-handling |
</details>
<details><summary>Per-model detail</summary>
<details><summary><b>claude-code/opus</b> (claude-code) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
I've verified the security-relevant surface of this change. My findings:
## VERDICT: No material issues found
Reviewed through the Security lens (authz, injection, tainted-input flow, info leakage, insecure defaults):
- **`whenMissing` preserves existence-masking (verified).** `internal/agent/tools.go:622` wraps only `domain.ErrNotFound`, which per `internal/api/errors.go:20` and `internal/domain/domain.go:12` deliberately covers *both* a missing row and one the actor may not see. The new message — *"no journal entry with id 999 that you can act on"* — is identical for both cases and phrased around the caller's own action, so it does not distinguish "doesn't exist" from "exists but not yours." No enumeration/existence leak is introduced. The message only reflects back the `id` the caller itself supplied (an `int64`), so there's no untrusted-data echo.
- **No injection surface.** `whenMissing`'s interpolated fields (`what`, `from`) are static literals from call sites; `id` is an `int64`. `describeSteps` (`runtime.go`) writes model text into a `slog` line `%q`-quoted and truncated — server-log only, not reflected to any query/exec sink.
- **`changeClaim` regex is ReDoS-safe.** Go's `regexp` is RE2 (linear-time, no catastrophic backtracking); the pattern is plain alternation with no nested quantifiers. Matching attacker-influenced reply text is safe.
- **No authz change.** Every `whenMissing` wrap sits *after* the `a.svc.*` call — the service seam still performs the permission/ownership check; this only rewrites the error text the model sees. Tool errors flow back to the model as tool results, and the agent HTTP path still maps `ErrNotFound` to a generic 404 (`errors.go:28`), so nothing new leaks over the wire.
- **No secret/credential exposure**, no SSRF/deserialization/path handling touched. The `honestReply`/prompt/description changes are guardrails that only *append* text; they cannot weaken a check.
Nothing in my lane is materially wrong here.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
I've verified the draft's single finding against the actual code. Confirmed:
- `public_link` with `action=get` (`tools.go:1152-1154`, `1167`) calls `GetPublicShareLink` and returns `a.linkOf(link), nil` — a non-error result that changes nothing. `GetPublicShareLink` (`service/public.go:54`) is a pure read.
- `public_link` is not in `readOnlyTools` (`runtime.go:187-191`), and cannot be, since it's a mixed tool.
- Therefore `acted()` (`runtime.go:231`) returns `true` after a successful `get`, and `honestReply` suppresses the correction even if the reply falsely claims "I've enabled the public link" (which `changeClaim` matches via the "Done" opener / "shared" term).
- The sibling read `list_shares` *is* in the map, so a false claim after only that call is caught.
The finding holds.
## VERDICT: Minor issues
The core logic is correct. `honestReply`/`acted`/`changeClaim` behave as described, and the `readOnlyTools` map covers all nine pure-read tools (`list_gardens`, `describe_garden`, `list_years`, `list_plantings`, `find_plant`, `read_journal`, `read_history`, `list_seed_lots`, `list_shares`) while every mutating tool is correctly excluded. `whenMissing` only rewraps `domain.ErrNotFound`, leaves `ErrVersionConflict` untouched, and preserves `errors.Is` via `%w`. A failed delete is correctly not counted as `acted` because majordomo surfaces the handler error as an `IsError` tool result.
One real gap through the correctness lens:
- **`internal/agent/runtime.go:187` / `internal/agent/tools.go:1152` — a false claim after a read-only `public_link get` is not corrected.** `public_link` is a mixed tool: `action=get` calls the pure-read `GetPublicShareLink` and returns success with `err == nil` (`tools.go:1153-1154`, `1167`), changing nothing. But the guard keys `readOnlyTools` on tool *name*, and `public_link` is necessarily absent from that map. So if the model calls only `public_link get` — plausible, since it would check status before enabling — and then hallucinates *"Done — I've enabled the public link"*, `acted()` returns `true` and the correction is suppressed. This is precisely the outward-facing class of claim the project treats as most consequential (`share_garden`/`public_link` per CLAUDE.md). The sibling path is fine: a false "I shared it" after only `list_shares` (a pure read, in the map) *is* caught. The invariant the PR adds to CLAUDE.md — "`readOnlyTools` must list every tool that changes nothing" — can't be honored for a per-action tool like `public_link`. Fix options: classify at the *result* level (have the `get` action return a marker the guard treats as read-only), or special-case `public_link` with `action=get` in `acted()`. Severity is small — a narrow, probabilistic sequence — but it's a genuine coverage gap, not a heuristic limitation.
Everything else checked (the regex openers/first-person branches, nil-guards in `describeSteps`, the `undo_change`/`move_planting`/`update_*` error rewrapping, no RE2 backtracking risk) is sound. The remaining heuristic limits (passive voice like "the entry has been deleted", or "I went ahead and deleted it" evading the first-person regex) are acknowledged by the PR as prompt-backed best-effort and are not defects.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
Both findings are confirmed against the actual code:
- **Finding 1**: `deleteObject` (`:807-808`) and `updateObject` (`:798-801`) return bare `err`; `deletePlant` (`:976-977`) too — while siblings `deletePlanting` (`:985-986`), `removePlanting` (`:826`), `updateSeedLot` (`:955`), `deleteSeedLot` (`:962`) route through `whenMissing`. The uneven application is real.
- **Finding 2**: `changeClaim` (`:197-198`) genuinely spells an overlapping past-tense verb list across both alternation arms.
Both hold up. Neither is a bug; both are accurate cleanliness observations.
## VERDICT: Minor issues
Verified against the checked-out code (`internal/agent/runtime.go`, `internal/agent/tools.go`). The change is generally clean: `honestReply`/`acted`/`describeSteps` are small, well-named, and well-commented, and `whenMissing` is a good shared helper that removes what would otherwise be copy-paste error wrapping. `readOnlyTools` matches the set of read-only tools present in the toolbox. Two minor cleanliness notes:
- **`whenMissing` is applied unevenly across id-taking tools** (`internal/agent/tools.go:808`, `:801`, `:977`). The PR routes `ErrNotFound` through `whenMissing` for the planting/journal/seed-lot/undo tools (`deletePlanting`, `removePlanting`, `updatePlanting`, `updateSeedLot`, `deleteSeedLot`, `updateJournalEntry`, `deleteJournalEntry`, `undoChange`), but the equally id-driven `delete_object`/`update_object` (`:798-809`) and `delete_plant` (`:967-980`) still `return nil, err` with the bare "not found". This leaves a partial pattern — the same model-guesses-an-id failure the PR targets is left un-helped on those tools, and the next reader has to guess whether the omission is deliberate. Low-churn fix: wrap those returns with `whenMissing(err, "object"/"plant", id, "describe_garden"/"find_plant")`, or add a one-line comment saying why they're intentionally excluded.
- **`changeClaim` duplicates its verb list across two arms** (`internal/agent/runtime.go:197-198`). The opener arm and the first-person arm each spell out an overlapping set of past-tense verbs (`deleted|removed|updated|changed|added|saved|moved|planted|filled|cleared|corrected|recorded|marked|shared|renamed|reverted`), with the second arm adding more. Keeping the two lists in sync by hand is a maintenance hazard — adding a future verb means remembering to touch both. Trivial, and arguably acceptable since the two arms genuinely differ, but worth a comment noting the shared core, or factoring the common verbs into a named sub-expression both arms interpolate.
Neither blocks merge.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
This PR is entirely within one agent turn's post-processing path — there's no loop, query, or per-item work here. Let me confirm the perf-relevant details:
- `changeClaim` is a **package-level `var` compiled once** at init (runtime.go:197), not recompiled per call — no per-turn regex compilation cost.
- `honestReply` runs **once per agent turn** (runtime.go:172), gated cheaply: `r == nil || acted(r) || !changeClaim.MatchString(reply)`. `acted()` (the cheaper check) short-circuits before the regex when a change succeeded.
- `acted()` iterates `steps × results` — bounded by the model's tool calls in a single turn (single digits), not a hot path.
- `describeSteps` (the only allocation-heavy piece: `strings.Builder`, `strings.Fields`, `Response.Text()`) runs **only on the rare correction path**, not on every turn.
- `whenMissing` (tools.go) only allocates a wrapped error on the `ErrNotFound` branch; the happy path returns `err` unchanged.
An agent chat turn is inherently a slow, LLM-bound operation — this adds one regex match and a linear scan of a handful of steps to it. Nothing quadratic, unbounded, allocation-in-a-loop, or N+1.
## VERDICT: No material issues found
Reviewed through the Performance lens. Verified by reading `internal/agent/runtime.go:160-255` (confirming `changeClaim` is compiled once at package scope, and that `honestReply`/`describeSteps` run once-per-turn / only-on-correction respectively) and the `whenMissing` wrappers in the diff (allocation only on the not-found branch). Nothing in this change introduces an efficiency regression worth flagging.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
I've verified all three findings against the actual code. Let me confirm the key mechanics:
- **Finding 1**: `undoChange` (tools.go:716–720) returns `(res, nil)` — a non-error result — when `cs == nil`. `undo_change` is absent from `readOnlyTools` (runtime.go:187–191), so `acted()` (line 234) returns true for it, and `honestReply` passes the reply through. Confirmed: this is exactly the documented "undo the beets, said Done, changed nothing" scenario, uncaught when the model does call `undo_change` but it no-ops.
- **Finding 2**: The `^\s*(?:done|…)` opener alternation (line 197) matches any reply beginning with a change verb regardless of whether a mutation was requested; a read-only turn has `acted() == false`, so the correction is appended. Confirmed mechanically.
- **Finding 3**: `text[:80]` (line 219) slices on a byte boundary; confirmed, log-only, trivial.
All three survive verification.
## VERDICT: Minor issues
My lens is error handling & edge cases. The core mechanism is sound — `honestReply` nil-checks `r`, `describeSteps` nil-checks `st.Response`, `acted`/`describeSteps` range safely over empty slices, `whenMissing` only rewrites `ErrNotFound` and passes every other error (including `ErrVersionConflict`, keeping the 409 path intact) through untouched, and `readOnlyTools` lists the read-only tools. But there are edge-case gaps in the new guard:
- **`internal/agent/runtime.go:234` — a successful tool that changed nothing still counts as "acted", so the guard has a hole for exactly the case it targets.** `acted` returns true for any `!IsError && !readOnlyTools[name]` result. But `undo_change` returns a **non-error** result when it reverted nothing: `undoChange` (`tools.go:716`) handles `cs == nil` ("every revision was a conflict, or the set was empty") by returning `(res, nil)` with `res.Changes = "nothing"`. So a turn where the model calls `undo_change`, reverts nothing, and replies *"Done — I've undone the beets"* passes `acted() == true` and the false claim is **not** corrected — the same "said it did, did nothing" failure the PR exists to catch (and the very "undo the beets → Done, changed nothing" scenario CLAUDE.md documents), just reached via a no-op `undo_change` call rather than no call at all. Fix: gate `undo_change` on whether `cs != nil` (treat a no-op revert as not-acted). Severity medium.
- **`internal/agent/runtime.go:251` — the guard false-positives on a read-only turn whose reply merely opens with a change verb.** The `^\s*(?:done|fixed|updated|…)\b` opener matches replies like *"Done — here's your garden: …"* or *"Updated: you have 12 beds"* to a pure information request answered with only read-only tools (`acted() == false`). Such a turn gets *"nothing actually changed in this turn … Ask again and I'll do it properly"* appended even though the user never asked for a change — user-visible wrong output on a happy path. Fix: narrow the opener, or only correct when the original message plausibly requested a mutation. Severity small.
- **`internal/agent/runtime.go:219` — `text[:80]` slices on a byte boundary and can split a multibyte rune.** Model text often contains non-ASCII (em-dashes, accented plant names); truncating at byte 80 can cut mid-rune, yielding invalid UTF-8 in the `%q`-formatted log line. No panic and no user impact (log-only; `%q` escapes the bad bytes), but trivially wrong. Fix: truncate on a rune boundary. Severity trivial.
</details>
</details>
<details><summary><b>claude-code/sonnet</b> (claude-code) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
**Verdict: No material issues found**
The changes introduce `honestReply`/`changeClaim` (a claim-detection layer), `whenMissing` (richer not-found errors), and tool description improvements. I verified each with the exploration agent:
- **`whenMissing` parameters** — all three string arguments (`what`, `from`) are compile-time literals; `id` is `int64` from the tool argument. No user-controlled data flows into the formatted error message.
- **`ErrNotFound` masking maintained** — `whenMissing` re-wraps with `%w: … domain.ErrNotFound …`, so `errors.Is(result, ErrNotFound)` stays true and the API layer's 404/masking logic is unaffected. The new message says "that you can act on", which doesn't confirm existence for resources the actor can't see.
- **Authz not bypassed** — `honestReply` and `acted` are purely structural: they inspect `Result.Steps` after the run. All permission enforcement happens upstream in the service layer (the garden-level auth check at the top of `Run`, then per-call service checks in every adapter method). Nothing here bypasses or short-circuits those paths.
- **`unbackedClaim` concatenation** — appended to Markdown, rendered via `react-markdown` without `rehype-raw`, so no HTML injection vector.
- **`changeClaim` regex** — no nested quantifiers; not vulnerable to ReDoS.
One very-low-severity observation: in `describeSteps`, `res.Name` (the tool name from the LLM's function-call response) is written into the log string via plain `b.WriteString(" " + res.Name)` without `%q`. In practice the majordomo framework only populates `res.Name` with declared tool names, so an attacker would need to both control the model response *and* have the LLM API accept an undeclared name — and the impact would be limited to log-line corruption, not data or auth compromise. Not worth blocking on.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
The finding is confirmed. `public_link` is absent from `readOnlyTools` (lines 187–191), and `acted()` at line 234 returns `true` for any non-errored call not in that map. A `public_link action=get` call — which routes to the read-only `GetPublicShareLink` at tools.go:1154 — succeeds with `IsError=false`, so `acted()` returns `true` and `honestReply` passes the reply through unchanged, even if the model falsely claims it enabled or rotated the link.
---
**Minor issues**
- **`internal/agent/runtime.go:187–191` — `public_link action=get` bypasses the honest-reply guard** (confirmed)
`public_link` exposes four actions: `get` (read-only: calls `GetPublicShareLink`) and `enable/rotate/disable` (writes: call `EnablePublicShareLink` / `DisablePublicShareLink`). Because `readOnlyTools` keys on the tool name string, `public_link` cannot be listed there without also neutralising the write actions. The result: a model that calls `public_link action=get` and then falsely claims "Done — I've enabled the public link" escapes correction — `acted()` sees a successful `public_link` call (not in `readOnlyTools`, `IsError=false`) and returns `true`, so `honestReply` passes the reply through unchanged.
This is the exact scenario the PR targets. Sharing-state claims are particularly sensitive because they tell the user who can see the garden. A straightforward fix: split `public_link` into a dedicated read-only `get_public_link` tool (which would be added to `readOnlyTools`) and a write-only `set_public_link`, or have `acted()` inspect the tool result or arguments to distinguish `get` from a write action. Alternatively, document this as a known gap in the `readOnlyTools` comment — but the CLAUDE.md convention "a new read-only tool left out of it makes a turn look like it acted" leaves this unacknowledged.
Everything else checks out: `whenMissing` wraps `ErrNotFound` with `%w` so `errors.Is` at the API layer still resolves correctly; the `changeClaim` regex handles all the claimed patterns correctly; `acted()`'s nil-safety is guaranteed at the call site before `describeSteps` is reached.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
Both findings are clearly visible in the actual source. Let me verify the regex structure carefully.
The concatenated regex at runtime is:
`(?i)(?:^\s*(?:done|...|reverted)\b|\bI(?:'ve| have)? (?:...)\b)`
The full expression is balanced and valid — `regexp.MustCompile` would panic at startup if not. But confirming the draft's structural claim: the outer `(?:` opens on line 197 and is closed by the trailing `)` on line 198. Neither string is a standalone valid regex. Finding 1 is **CONFIRMED** as a real (if minor) maintainability fragility.
Finding 2 is plainly visible: lines 188–189 pack four entries each on a single line. **CONFIRMED**.
---
**Minor issues**
---
**1. `changeClaim` regex outer group spans two string literals — neither half is a valid standalone regex** (`internal/agent/runtime.go:197–198`)
The outer non-capturing group `(?:` opens in the first string literal; its closing `)` is the last character of the second. The concatenated result is a valid regex today, but neither literal alone is, which means a maintainer editing one half (adding a keyword, re-wrapping the line) has to keep the other half in mind to preserve group balance. Given that `CLAUDE.md` explicitly flags this map as a maintenance point, the same risk applies to the regex. The safest fix is a single raw string, or restructuring so each literal is a balanced alternation joined with `|`:
```go
var changeClaim = regexp.MustCompile(`(?i)` +
`(?:^\s*(?:done|fixed|...|reverted)\b` +
`|\bI(?:'ve| have)? (?:just |...)\b)`)
```
Severity: small — correct today, fragile to edit.
---
**2. `readOnlyTools` map packs multiple entries per line** (`internal/agent/runtime.go:187–191`)
```go
var readOnlyTools = map[string]bool{
"list_gardens": true, "describe_garden": true, "list_years": true, "list_plantings": true,
"find_plant": true, "read_journal": true, "read_history": true, "list_seed_lots": true,
"list_shares": true,
}
```
Standard Go style is one entry per line with a trailing comma. `CLAUDE.md` calls out this map explicitly as a maintenance point (every new read-only tool must be added here), so one-per-line formatting would make additions and diffs easier to read. Trivial to fix.
---
Everything else in the diff is clean: `whenMissing` is applied consistently across every id-taking tool call site, `describeSteps`/`acted`/`honestReply` are well-factored with clear names, the test cases cover the stated failure modes without duplicating setup, and the `CLAUDE.md` addition accurately documents the implementation.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**No material issues found**
All new code runs once per agent turn, **after** `a.Run()` completes — not inside the streaming loop. I verified this at `runtime.go:172–177`: `honestReply` is called on the finalization path, never mid-turn.
- **`acted` / `honestReply`** — Walks `result.Steps` (hard-capped at `maxSteps = 24`, a constant in the runner) and the small `results` slice per step. O(steps × tools) with a tight ceiling; negligible.
- **`changeClaim` regexp** — Package-level compiled `var`; `.MatchString` is called once per turn on a short model reply string. No concern.
- **`describeSteps`** — Only invoked inside the `if corrected != turn.Reply` branch (the correction actually fired), so it's strictly the error path. The `strings.Fields` + `strings.Join` normalization allocates a temporary slice, but it runs at most once per false-claim event, on at most 24 step responses each truncated to 80 chars in the log. Not material.
- **`readOnlyTools` map** — Package-level, read-only after init; map lookup is O(1).
- **`whenMissing`** — One `errors.Is` check and a conditional `fmt.Errorf`; called per tool call, trivially cheap.
- No N+1 queries, unbounded growth, or quadratic patterns introduced.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
Both findings are confirmed. `moveObject` (line 396) and `updateObject` (line 798) return the raw service error, and `deleteObject` (line 807) returns `nil, err` — none use `whenMissing`. The byte-slice truncation at runtime.go is also present as described.
---
**Minor issues**
---
### 1. `deleteObject`, `moveObject`, and `updateObject` lack `whenMissing` — inconsistent with stated goal
**`internal/agent/tools.go:807`** (`deleteObject`), **line 396** (`moveObject`), **line 798** (`updateObject`)
The PR applies `whenMissing` to every other id-taking mutating tool (plantings, journal entries, undo, seed lots), but the three object-level tools were left out:
- `deleteObject` (line 807): `return nil, err` — bare error
- `moveObject` (line 396): `return a.svc.UpdateObject(...)` — bare error
- `updateObject` (line 798): `return a.svc.UpdateObject(...)` — bare error
When a model passes a stale object ID, it receives a bare `ErrNotFound` with no guidance ("describe_garden lists the ids; nothing was changed"). The `honestReply` guard is still effective, but the model has no actionable signal telling it to re-read before retrying — exactly what `whenMissing` was designed to provide.
**Fix:** apply the same pattern already used by the planting and journal variants:
```go
// deleteObject
if err := a.svc.DeleteObject(ctx, a.actor, args.ObjectID); err != nil {
return nil, whenMissing(err, "object", args.ObjectID, "describe_garden")
}
// moveObject / updateObject — capture return and wrap
obj, err := a.svc.UpdateObject(ctx, a.actor, args.ObjectID, patch, args.Version)
return obj, whenMissing(err, "object", args.ObjectID, "describe_garden")
```
---
### 2. Byte-boundary truncation in `describeSteps` for log output
**`internal/agent/runtime.go:219`**
```go
if len(text) > 80 {
text = text[:80] + "…"
}
```
`text[:80]` slices by byte index. If the model reply contains multi-byte UTF-8 characters (e.g. an em-dash or a non-ASCII plant name) and the 80-byte boundary falls inside one, the resulting string is invalid UTF-8. In Go this is not a panic — the slice is always in bounds after the `len(text) > 80` guard — but the log line will contain a garbled character at the cut point.
This is a log-only function, so the impact is cosmetic. The fix is `[]rune(text)[:80]` (or `utf8.ValidString` check), but given the log-only context it's low urgency.
</details>
</details>
<details><summary><b>glm-5.2:cloud</b> (ollama-cloud) — Minor issues</summary>
**Verdict: Minor issues · ⚠️ 1/5 lens(es) errored** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
**Verdict: No material issues found**
Reviewed through the security lens:
- **Authn/authz unchanged.** All tool calls still run as `actorID` via the service layer's existing permission checks; `honestReply`, `acted`, and `describeSteps` only read `agent.Result`/`agent.Step` metadata and the model's own reply text — they introduce no new privilege surface or auth bypass. Verified at `runtime.go:121,205-256` and `tools.go:39-40`.
- **No injection.** `whenMissing` interpolates only an `int64` id and fixed strings into error messages (`tools.go:619-624`); no user/model-controlled string is formatted into SQL, a command, or a template. The `changeClaim` regex operates on the model's reply (already shown to the user), and `unbackedClaim` is a constant appended to that same reply — no reflection of untrusted input into a dangerous sink.
- **No secret leakage.** `describeSteps` logs the first 80 chars of each step's reply text to `slog.Warn`. That text is the model's own user-facing reply, not credentials or request bodies; logging it is benign and consistent with existing logging in `Run`.
- **`readOnlyTools` allowlist (completeness).** This is the closest thing to my lane, but it is a correctness/false-negative concern (omitting a future read-only tool would let a false claim stand), not a security vulnerability — the map only makes the *correction* fire less often; it can never grant access or suppress a real error. The CLAUDE.md note documents the maintenance requirement.
Could not directly confirm the `agent.Result`/`agent.Step` field names (`IsError`, `Name`, `Results`, `Response.Text()`) since the `majordomo/agent` vendor package is not in the checked-out tree, but existing test code (`runtime_test.go:165,635`) uses the same fields the same way, so resolution is not a new risk introduced here.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
**VERDICT: No material issues found**
I verified the core logic against the checked-out code:
- `honestReply` (`runtime.go:250`): only appends the correction when `acted(r)` is false **and** the reply matches `changeClaim`. `acted()` (`runtime.go:231`) returns true if any tool result is non-error and not in `readOnlyTools`. The `whenMissing` wrapper (`tools.go:621`) preserves `domain.ErrNotFound` via `%w`, so `errors.Is` still holds for any caller, and the tool result remains an error result (the adapter returns `(nil, err)`), so a failed delete is correctly seen as `IsError` and does not satisfy `acted`. Logic is sound.
- The `changeClaim` regex correctly excludes offers/future tense ("I'll remove it", "I will delete") — `I(?:'ve| have)?` consumes only `'ve`/`have`, and the verb list is past-tense only, so "I'll"/"will" leave a non-matching token before the verb. Verified by re-deriving against the pattern.
- The `whenMissing`-wrapped handlers (`movePlanting`, `updatePlanting`, `removePlanting`, `updateSeedLot`) return the value/err pair correctly: on success `whenMissing(nil,...)` returns nil; on non-ErrNotFound errors the original err passes through with the (nil) value, matching prior behavior.
One minor, low-confidence note (not blocking):
- `readOnlyTools` is an allowlist of read-only tool *names*. `public_link` is absent, yet its `get` action is read-only while `enable`/`rotate`/`disable` mutate. Since the name is identical across actions, listing it would be wrong (a successful `enable` would be miscounted as read-only and could trigger a false correction), so leaving it out is the safe choice — but it means a successful `public_link(get)` counts as "acted" and could suppress a correction for an unrelated false claim in the same turn. This is an inherent limitation of name-based classification and is consistent with the documented "once any change succeeded, stay out of it" stance; flagging only as a known edge, not a defect to fix.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
Both findings are confirmed against the actual code.
**Finding 1 (readOnlyTools denylist):** Confirmed at runtime.go:187-191. The map lists 9 read-only tools as string literals, and the `llm.DefineTool` calls in tools.go (37 of them by my count) carry no read-only annotation — the set is hand-synced in a separate file from the definitions it mirrors.
**Finding 2 (changeClaim verb duplication):** Confirmed at runtime.go:197-198. The opener branch has 20 verbs; the "I" branch has 32, adding `set|put|pulled|replaced|swapped|rotated|rewrote|rewritten|edited|created|started|attached|restored|made` that the opener lacks — the two lists are kept in rough sync by hand and already differ.
## VERDICT: Minor issues
I reviewed the change through the cleanliness/maintainability lens. The structure is sound — `whenMissing` is a nice shared helper replacing duplicated inline `errors.Is` patterns, and the new functions are well-named and commented in the file's existing style. A few smaller cleanliness concerns:
- **`runtime.go:187-191` — `readOnlyTools` is a manually-maintained denylist with no enforcement.** It lists 9 read-only tools by name as string literals, but the `llm.DefineTool` calls in `tools.go` carry no read-only annotation. The CLAUDE.md note acknowledges the risk ("a new read-only tool left out of it makes a turn look like it acted"), but the list is a hand-synced string set in a different file from the tool definitions it mirrors. The set will silently drift the next time a read-only tool is added (e.g. `list_shares` is present, but a future `list_…` would need to be remembered here). Worth at least a comment near `newToolbox` pointing tool authors at this map, or better, a single source of truth.
- **`runtime.go:197-198` — `changeClaim` regex duplicates its verb list twice.** The same core verbs (`deleted|removed|updated|changed|added|saved|moved|…`) appear both in the `^\s*(…)` opener branch and in the `\bI(…)(…)\b` branch, kept in rough sync by hand. They already differ (the "I" branch adds `set|put|pulled|replaced|swapped|rotated|rewrote|rewritten|edited|created|started|attached|restored|made` that the opener branch lacks). Extracting one verb-group string into a const would remove the duplication and the drift risk.
- **`runtime.go:200-201` — `unbackedClaim` is a user-visible markdown string.** Consistent with the neighboring `fallbackReply`; noted only as a magic string, not actionable.
- **`tools.go:622-625` — `whenMissing` interleaves `errors.Is`/`fmt.Errorf` correctly** and the `errors`/`fmt` imports are already present. Clean.
Nothing in my lane rises to blocking; the first item (the hand-maintained read-only denylist) is the one most likely to cause future maintainability pain.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
VERDICT: No material issues found
Through the performance lens:
- `changeClaim` is a package-level compiled `regexp` (initialized once at load), and `honestReply` runs exactly once per turn on a short reply string — not a hot path. The alternation has no nested quantifiers that could cause catastrophic backtracking; matching is linear in the reply length. No concern.
- `acted` iterates `result.Steps`/`Results` once per turn with map lookups into the package-level `readOnlyTools` map (O(1)). Turn-scoped, bounded by step count. Fine.
- `describeSteps` allocates a `strings.Builder` per step, but only on the `Warn` path (i.e. only when a correction is actually appended), so it never runs on the normal successful-run path. No hot-path regression.
- `whenMissing` wraps errors with a single `fmt.Errorf` per failing tool call; negligible and only on the error path.
I verified the structure of the new code and the surrounding call site at `internal/agent/runtime.go:185-255` and the tool definitions at `internal/agent/tools.go`; nothing here introduces unbounded growth, repeated allocations in a loop, N+1 behavior, or blocking work on a hot path.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: agent: max steps reached without a final answer (max 14)
</details>
</details>
<details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
**No material issues found**
Through the security lens, this PR introduces guardrails against model hallucinations about performed actions and improves error messages for missing-resource failures. Nothing here opens an authn/authz gap, injection surface, SSRF, deserialization risk, or secret-leakage path.
- **`whenMissing`** safely formats integer IDs with `%d`; no string interpolation of user input.
- **`readOnlyTools`** appears complete against the current tool registry; no read-only tool is misclassified as mutating.
- **`changeClaim`** is a static compiled regex applied to model output; no user-supplied pattern, and Go's regexp engine is linear-time.
- **`describeSteps`** logging is bounded (80 chars) and summarizes the agent's own step responses; it does not introduce a new exfiltration channel.
No security findings to report.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
**Verdict: No material issues found**
I scrutinized the PR through the correctness lens:
- **`readOnlyTools` coverage**: The nine tools listed (`list_gardens`, `describe_garden`, `list_years`, `list_plantings`, `find_plant`, `read_journal`, `read_history`, `list_seed_lots`, `list_shares`) are the purely read‑only tools defined in the toolbox. The mixed `public_link` tool is intentionally excluded (its `get` action is read‑only but `enable`/`rotate`/`disable` mutate state), which is a documented design limitation, not a bug.
- **`whenMissing` wrapping**: Every tool the PR targeted (`update_planting`, `move_planting`, `remove_planting`, `delete_planting`, `undo_change`, `update/delete_journal_entry`, `update/delete_seed_lot`) now wraps `domain.ErrNotFound` with a message that names the lookup tool and says “nothing was changed.” The wrapper preserves the error type (`%w`), so `errors.Is(..., domain.ErrNotFound)` still succeeds for downstream callers.
- **`changeClaim` regex**: Tested boundary cases mentally. It correctly matches “Done …”/“Fixed …” openers and first‑person past‑tense claims (“I’ve deleted”, “I moved”). It avoids matching offers/questions (“want me to delete it?”), as the test verifies. False positives/negatives exist in theory (e.g., “Updated documentation is available” vs. “It is fixed”), but these are inherent to the regex approach and the PR explicitly accepts this trade‑off.
- **`acted()` logic**: Returns `true` only if a non‑error, non‑read‑only tool result exists. A failed state‑changing call (`IsError == true`) does not count as “acted,” so a model claiming success over a failed deletion is still corrected—exactly the intended behavior, verified by the test.
No semantic or logic bugs introduced by the change.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
**Minor issues**
- `internal/agent/runtime.go:197-198` — `changeClaim` duplicates its verb inventory across two alternations. A single source of truth (e.g., a slice composed into the pattern) would be easier to keep consistent and less prone to omissions. As it stands, the two lists already diverge: `created`, `set`, `made`, `edited` and others appear only in the `I've …` branch, while `done` appears only in the sentence-opener branch. That makes the regex harder to maintain and more likely to miss new claim phrasings that get added to one list but not the other.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**Verdict:** No material issues found
The changes under review add correctness guards and richer error messages. Through the performance lens, the new code is benign:
- **`changeClaim`** is a compiled `regexp` checked once per turn via `MatchString`. Go’s RE2 engine runs in linear time in the input size, and the input (the model’s reply) is bounded to a few KB at most. Not a hot-path concern.
- **`acted`** iterates over steps and results once per turn. In practice this is a handful of iterations; no quadratic or N+1 behavior.
- **`describeSteps`** allocates only when a correction is actually emitted (the rare `Warn` path), and its work is proportional to the small number of steps already in the result.
- **`whenMissing`** wraps errors on failure paths only; it does not add new I/O, allocations on success paths, or unbounded growth.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
**No material issues found**
Through the error-handling and edge-cases lens, the change is solid:
- **`readOnlyTools`** is complete: every truly read-only tool is listed, and no listed tool modifies state. The set is intentionally conservative (e.g. `public_link` is absent because it *can* write), which is the right trade-off for this guard.
- **`acted()`** correctly skips error results, so a failed `delete_journal_entry` (or an unconfirmed `share_garden`) does not make a turn look like it acted.
- **`changeClaim`** regex is carefully scoped to past-tense claims and excludes questions/offers. False-positive analysis on patterns like "I haven't deleted", "I've been deleted", "I have a fixed schedule", and "I want to delete it" shows no match — the alternations and word boundaries hold up.
- **`honestReply`** handles `nil` result defensively and only appends the correction when the claim regex matches and no non-read-only tool succeeded. Empty replies and empty step lists fall through correctly.
- **`whenMissing`** wraps only `domain.ErrNotFound` (using `errors.Is` so wrapped errors work), passes everything else through, and produces a useful actionable message with the ID and the lookup tool.
- **Test coverage** exercises the nil-tools, read-only-only, failed-tool, no-claim, and successful-change cases.
Edge cases that are outside the diff's scope (e.g. a `public_link` with `action=get` is treated as modifying, or `share_garden` that finds an identical existing share returns success without change) are acknowledged limitations of the design, not regressions introduced here.
</details>
</details>
</details>
<sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
- changeClaim is built from one changeVerbs list; the opener is just
done/fixed/undone so an informational "Updated totals:" can't trip it.
- public_link (get reads) and undo_change (nothing left to revert) are
self-reporting: their success no longer counts as a change by name; the
adapter says whether they changed something (noteChange / didChange).
- whenMissing covers the object and plant tools too (move/update/delete
object, clear/remove plantings by object, update/delete plant).
- The step summary cuts on a rune boundary.
Co-Authored-By: Claude Fable 5 <[email protected]>
steve
merged commit 0aabccf1bd into main2026-08-23 06:52:43 +00:00
steve
deleted branch feat/agent-honest-turns2026-08-23 06:52:44 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Found in the live sweep of #129–#131: asked to delete a journal entry, and later a seed lot, the model answered "Done — I've deleted it" both times having deleted nothing — each was still there a turn later. Pushed a second time ("it's still there, delete it") it called
find_plant → list_seed_lots → delete_seed_lotand the lot was gone, so the tools work; the model skipped or botched them the first time and reported success anyway. The prompt already forbade that; this makes the run catch it.What's in it
honestReply(internal/agent/runtime.go): when the reply claims a change — a "Done"/"Fixed" opener, or a first-person past-tense claim ("I've deleted", "I moved") — and no tool call in the run succeeded at anything outsidereadOnlyTools, the reply gets a correction the person can read: "nothing actually changed in this turn — no tool call that changes anything succeeded." It only speaks when nothing was done at all; once any change succeeded it can't tell a true claim from a false one and stays out of it. Fires with aWarnlog that summarizes the steps (tools called,!on failures, the start of each step's text) so the mechanism can be read off the log next time.whenMissing): every id-taking tool (update_planting,move_planting,remove_planting,delete_planting,undo_change,update/delete_journal_entry,update/delete_seed_lot) turns the store's bare "not found" into "no journal entry with id 999 that you can act on — read_journal lists the ids; nothing was changed", so a model that guessed an id is told to look it up rather than guess again.readOnlyToolsmust list every tool that changes nothing — a new read-only tool left out of it makes a turn look like it acted.Tests
TestAClaimedChangeNoToolMadeIsCorrected: claim with no tools → corrected; claim over read-only calls → corrected; claim over a failed delete → corrected (and the failure namesread_journaland says nothing changed); an offer ("want me to delete it?") → untouched; a real deletion → the claim stands.🤖 Generated with Claude Code
Live, asked to delete a journal entry and later a seed lot, the model answered "Done — I've deleted it" both times having deleted nothing; each was still there a turn later. The prompt already forbade that. Now the run catches it: honestReply appends a correction when the reply claims a change ("Done", "I've deleted…") and no non-read-only tool call succeeded, and logs the steps so the mechanism can be read off the log next time. Alongside: the id-taking tools turn a bare "not found" into a message that names what was missing and which tool lists the ids ("nothing was changed"), the two delete descriptions say to look the id up in THIS turn, and the prompt says an error result means the thing did not happen. Co-Authored-By: Claude Fable 5 <[email protected]>🪰 Gadfly — live review status
4/4 reviewers finished · updated 2026-08-23 06:46:10Z
claude-code/opus· claude-code — ✅ doneclaude-code/sonnet· claude-code — ✅ doneglm-5.2:cloud· ollama-cloud — ✅ donekimi-k2.6:cloud· ollama-cloud — ✅ doneLive status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
🪰 Gadfly consensus review — 5 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
@@ -177,1 +184,4 @@// readOnlyTools are the tools whose success changes nothing — a turn made of// these alone has not done anything, whatever its reply says.var readOnlyTools = map[string]bool{🟠 public_link action=get bypasses honestReply: acted() returns true for a read-only call, silencing the false-claim correction
correctness, maintainability · flagged by 3 models
internal/agent/runtime.go:187–191—public_link action=getbypasses the honest-reply guard (confirmed)🪰 Gadfly · advisory
@@ -178,0 +194,4 @@// opener, or a first-person past-tense claim ("I've deleted", "I moved"). A// question or an offer ("want me to delete it?", "I'll remove it") does not// match — only a claim of something already done.var changeClaim = regexp.MustCompile(`(?i)(?:^\s*(?:done|fixed|undone|deleted|removed|updated|changed|added|saved|moved|planted|filled|cleared|corrected|recorded|marked|shared|renamed|reverted)\b` +🟠 changeClaim regex duplicates verb lists across two alternations, creating a maintenance hazard
maintainability · flagged by 4 models
internal/agent/runtime.go:197-198—changeClaimduplicates its verb inventory across two alternations. A single source of truth (e.g., a slice composed into the pattern) would be easier to keep consistent and less prone to omissions. As it stands, the two lists already diverge:created,set,made,editedand others appear only in theI've …branch, whiledoneappears only in the sentence-opener branch. That makes the regex harder to maintain and more likely to miss new claim ph…🪰 Gadfly · advisory
@@ -178,0 +216,4 @@if st.Response != nil {if text := strings.Join(strings.Fields(st.Response.Text()), " "); text != "" {if len(text) > 80 {text = text[:80] + "…"⚪ text[:80] byte-slice can split a multibyte rune, producing invalid UTF-8 in the log line
error-handling · flagged by 2 models
internal/agent/runtime.go:219—text[:80]slices on a byte boundary and can split a multibyte rune. Model text often contains non-ASCII (em-dashes, accented plant names); truncating at byte 80 can cut mid-rune, yielding invalid UTF-8 in the%q-formatted log line. No panic and no user impact (log-only;%qescapes the bad bytes), but trivially wrong. Fix: truncate on a rune boundary. Severity trivial.🪰 Gadfly · advisory
@@ -178,0 +231,4 @@func acted(r *agent.Result) bool {for _, st := range r.Steps {for _, res := range st.Results {if !res.IsError && !readOnlyTools[res.Name] {🟠 undo_change that reverts nothing (cs==nil) counts as 'acted', so honestReply misses a false 'I've undone it' claim
error-handling · flagged by 1 model
internal/agent/runtime.go:234— a successful tool that changed nothing still counts as "acted", so the guard has a hole for exactly the case it targets.actedreturns true for any!IsError && !readOnlyTools[name]result. Butundo_changereturns a non-error result when it reverted nothing:undoChange(tools.go:716) handlescs == nil("every revision was a conflict, or the set was empty") by returning(res, nil)withres.Changes = "nothing". So a turn where the model ca…🪰 Gadfly · advisory
@@ -178,0 +248,4 @@// succeeded, passes through unchanged — this cannot tell a true claim from a// false one once anything at all was done, so it only speaks when nothing was.func honestReply(reply string, r *agent.Result) string {if r == nil || acted(r) || !changeClaim.MatchString(reply) {🟡 changeClaim opener false-positives on read-only informational replies starting with 'Done'/'Updated', appending a bogus correction
error-handling · flagged by 1 model
internal/agent/runtime.go:251— the guard false-positives on a read-only turn whose reply merely opens with a change verb. The^\s*(?:done|fixed|updated|…)\bopener matches replies like "Done — here's your garden: …" or "Updated: you have 12 beds" to a pure information request answered with only read-only tools (acted() == false). Such a turn gets "nothing actually changed in this turn … Ask again and I'll do it properly" appended even though the user never asked for a change —…🪰 Gadfly · advisory
🪰 Gadfly review — consensus across 4 models
Verdict: Minor issues · 6 findings (4 with multi-model agreement)
internal/agent/runtime.go:197internal/agent/runtime.go:187internal/agent/tools.go:807internal/agent/runtime.go:2192 single-model findings (lower confidence)
internal/agent/runtime.go:234internal/agent/runtime.go:251Per-model detail
claude-code/opus (claude-code) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
I've verified the security-relevant surface of this change. My findings:
VERDICT: No material issues found
Reviewed through the Security lens (authz, injection, tainted-input flow, info leakage, insecure defaults):
whenMissingpreserves existence-masking (verified).internal/agent/tools.go:622wraps onlydomain.ErrNotFound, which perinternal/api/errors.go:20andinternal/domain/domain.go:12deliberately covers both a missing row and one the actor may not see. The new message — "no journal entry with id 999 that you can act on" — is identical for both cases and phrased around the caller's own action, so it does not distinguish "doesn't exist" from "exists but not yours." No enumeration/existence leak is introduced. The message only reflects back theidthe caller itself supplied (anint64), so there's no untrusted-data echo.whenMissing's interpolated fields (what,from) are static literals from call sites;idis anint64.describeSteps(runtime.go) writes model text into aslogline%q-quoted and truncated — server-log only, not reflected to any query/exec sink.changeClaimregex is ReDoS-safe. Go'sregexpis RE2 (linear-time, no catastrophic backtracking); the pattern is plain alternation with no nested quantifiers. Matching attacker-influenced reply text is safe.whenMissingwrap sits after thea.svc.*call — the service seam still performs the permission/ownership check; this only rewrites the error text the model sees. Tool errors flow back to the model as tool results, and the agent HTTP path still mapsErrNotFoundto a generic 404 (errors.go:28), so nothing new leaks over the wire.honestReply/prompt/description changes are guardrails that only append text; they cannot weaken a check.Nothing in my lane is materially wrong here.
🎯 Correctness — Minor issues
I've verified the draft's single finding against the actual code. Confirmed:
public_linkwithaction=get(tools.go:1152-1154,1167) callsGetPublicShareLinkand returnsa.linkOf(link), nil— a non-error result that changes nothing.GetPublicShareLink(service/public.go:54) is a pure read.public_linkis not inreadOnlyTools(runtime.go:187-191), and cannot be, since it's a mixed tool.acted()(runtime.go:231) returnstrueafter a successfulget, andhonestReplysuppresses the correction even if the reply falsely claims "I've enabled the public link" (whichchangeClaimmatches via the "Done" opener / "shared" term).list_sharesis in the map, so a false claim after only that call is caught.The finding holds.
VERDICT: Minor issues
The core logic is correct.
honestReply/acted/changeClaimbehave as described, and thereadOnlyToolsmap covers all nine pure-read tools (list_gardens,describe_garden,list_years,list_plantings,find_plant,read_journal,read_history,list_seed_lots,list_shares) while every mutating tool is correctly excluded.whenMissingonly rewrapsdomain.ErrNotFound, leavesErrVersionConflictuntouched, and preserveserrors.Isvia%w. A failed delete is correctly not counted asactedbecause majordomo surfaces the handler error as anIsErrortool result.One real gap through the correctness lens:
internal/agent/runtime.go:187/internal/agent/tools.go:1152— a false claim after a read-onlypublic_link getis not corrected.public_linkis a mixed tool:action=getcalls the pure-readGetPublicShareLinkand returns success witherr == nil(tools.go:1153-1154,1167), changing nothing. But the guard keysreadOnlyToolson tool name, andpublic_linkis necessarily absent from that map. So if the model calls onlypublic_link get— plausible, since it would check status before enabling — and then hallucinates "Done — I've enabled the public link",acted()returnstrueand the correction is suppressed. This is precisely the outward-facing class of claim the project treats as most consequential (share_garden/public_linkper CLAUDE.md). The sibling path is fine: a false "I shared it" after onlylist_shares(a pure read, in the map) is caught. The invariant the PR adds to CLAUDE.md — "readOnlyToolsmust list every tool that changes nothing" — can't be honored for a per-action tool likepublic_link. Fix options: classify at the result level (have thegetaction return a marker the guard treats as read-only), or special-casepublic_linkwithaction=getinacted(). Severity is small — a narrow, probabilistic sequence — but it's a genuine coverage gap, not a heuristic limitation.Everything else checked (the regex openers/first-person branches, nil-guards in
describeSteps, theundo_change/move_planting/update_*error rewrapping, no RE2 backtracking risk) is sound. The remaining heuristic limits (passive voice like "the entry has been deleted", or "I went ahead and deleted it" evading the first-person regex) are acknowledged by the PR as prompt-backed best-effort and are not defects.🧹 Code cleanliness & maintainability — Minor issues
Both findings are confirmed against the actual code:
deleteObject(:807-808) andupdateObject(:798-801) return bareerr;deletePlant(:976-977) too — while siblingsdeletePlanting(:985-986),removePlanting(:826),updateSeedLot(:955),deleteSeedLot(:962) route throughwhenMissing. The uneven application is real.changeClaim(:197-198) genuinely spells an overlapping past-tense verb list across both alternation arms.Both hold up. Neither is a bug; both are accurate cleanliness observations.
VERDICT: Minor issues
Verified against the checked-out code (
internal/agent/runtime.go,internal/agent/tools.go). The change is generally clean:honestReply/acted/describeStepsare small, well-named, and well-commented, andwhenMissingis a good shared helper that removes what would otherwise be copy-paste error wrapping.readOnlyToolsmatches the set of read-only tools present in the toolbox. Two minor cleanliness notes:whenMissingis applied unevenly across id-taking tools (internal/agent/tools.go:808,:801,:977). The PR routesErrNotFoundthroughwhenMissingfor the planting/journal/seed-lot/undo tools (deletePlanting,removePlanting,updatePlanting,updateSeedLot,deleteSeedLot,updateJournalEntry,deleteJournalEntry,undoChange), but the equally id-drivendelete_object/update_object(:798-809) anddelete_plant(:967-980) stillreturn nil, errwith the bare "not found". This leaves a partial pattern — the same model-guesses-an-id failure the PR targets is left un-helped on those tools, and the next reader has to guess whether the omission is deliberate. Low-churn fix: wrap those returns withwhenMissing(err, "object"/"plant", id, "describe_garden"/"find_plant"), or add a one-line comment saying why they're intentionally excluded.changeClaimduplicates its verb list across two arms (internal/agent/runtime.go:197-198). The opener arm and the first-person arm each spell out an overlapping set of past-tense verbs (deleted|removed|updated|changed|added|saved|moved|planted|filled|cleared|corrected|recorded|marked|shared|renamed|reverted), with the second arm adding more. Keeping the two lists in sync by hand is a maintenance hazard — adding a future verb means remembering to touch both. Trivial, and arguably acceptable since the two arms genuinely differ, but worth a comment noting the shared core, or factoring the common verbs into a named sub-expression both arms interpolate.Neither blocks merge.
⚡ Performance — No material issues found
This PR is entirely within one agent turn's post-processing path — there's no loop, query, or per-item work here. Let me confirm the perf-relevant details:
changeClaimis a package-levelvarcompiled once at init (runtime.go:197), not recompiled per call — no per-turn regex compilation cost.honestReplyruns once per agent turn (runtime.go:172), gated cheaply:r == nil || acted(r) || !changeClaim.MatchString(reply).acted()(the cheaper check) short-circuits before the regex when a change succeeded.acted()iteratessteps × results— bounded by the model's tool calls in a single turn (single digits), not a hot path.describeSteps(the only allocation-heavy piece:strings.Builder,strings.Fields,Response.Text()) runs only on the rare correction path, not on every turn.whenMissing(tools.go) only allocates a wrapped error on theErrNotFoundbranch; the happy path returnserrunchanged.An agent chat turn is inherently a slow, LLM-bound operation — this adds one regex match and a linear scan of a handful of steps to it. Nothing quadratic, unbounded, allocation-in-a-loop, or N+1.
VERDICT: No material issues found
Reviewed through the Performance lens. Verified by reading
internal/agent/runtime.go:160-255(confirmingchangeClaimis compiled once at package scope, and thathonestReply/describeStepsrun once-per-turn / only-on-correction respectively) and thewhenMissingwrappers in the diff (allocation only on the not-found branch). Nothing in this change introduces an efficiency regression worth flagging.🧯 Error handling & edge cases — Minor issues
I've verified all three findings against the actual code. Let me confirm the key mechanics:
undoChange(tools.go:716–720) returns(res, nil)— a non-error result — whencs == nil.undo_changeis absent fromreadOnlyTools(runtime.go:187–191), soacted()(line 234) returns true for it, andhonestReplypasses the reply through. Confirmed: this is exactly the documented "undo the beets, said Done, changed nothing" scenario, uncaught when the model does callundo_changebut it no-ops.^\s*(?:done|…)opener alternation (line 197) matches any reply beginning with a change verb regardless of whether a mutation was requested; a read-only turn hasacted() == false, so the correction is appended. Confirmed mechanically.text[:80](line 219) slices on a byte boundary; confirmed, log-only, trivial.All three survive verification.
VERDICT: Minor issues
My lens is error handling & edge cases. The core mechanism is sound —
honestReplynil-checksr,describeStepsnil-checksst.Response,acted/describeStepsrange safely over empty slices,whenMissingonly rewritesErrNotFoundand passes every other error (includingErrVersionConflict, keeping the 409 path intact) through untouched, andreadOnlyToolslists the read-only tools. But there are edge-case gaps in the new guard:internal/agent/runtime.go:234— a successful tool that changed nothing still counts as "acted", so the guard has a hole for exactly the case it targets.actedreturns true for any!IsError && !readOnlyTools[name]result. Butundo_changereturns a non-error result when it reverted nothing:undoChange(tools.go:716) handlescs == nil("every revision was a conflict, or the set was empty") by returning(res, nil)withres.Changes = "nothing". So a turn where the model callsundo_change, reverts nothing, and replies "Done — I've undone the beets" passesacted() == trueand the false claim is not corrected — the same "said it did, did nothing" failure the PR exists to catch (and the very "undo the beets → Done, changed nothing" scenario CLAUDE.md documents), just reached via a no-opundo_changecall rather than no call at all. Fix: gateundo_changeon whethercs != nil(treat a no-op revert as not-acted). Severity medium.internal/agent/runtime.go:251— the guard false-positives on a read-only turn whose reply merely opens with a change verb. The^\s*(?:done|fixed|updated|…)\bopener matches replies like "Done — here's your garden: …" or "Updated: you have 12 beds" to a pure information request answered with only read-only tools (acted() == false). Such a turn gets "nothing actually changed in this turn … Ask again and I'll do it properly" appended even though the user never asked for a change — user-visible wrong output on a happy path. Fix: narrow the opener, or only correct when the original message plausibly requested a mutation. Severity small.internal/agent/runtime.go:219—text[:80]slices on a byte boundary and can split a multibyte rune. Model text often contains non-ASCII (em-dashes, accented plant names); truncating at byte 80 can cut mid-rune, yielding invalid UTF-8 in the%q-formatted log line. No panic and no user impact (log-only;%qescapes the bad bytes), but trivially wrong. Fix: truncate on a rune boundary. Severity trivial.claude-code/sonnet (claude-code) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
Verdict: No material issues found
The changes introduce
honestReply/changeClaim(a claim-detection layer),whenMissing(richer not-found errors), and tool description improvements. I verified each with the exploration agent:whenMissingparameters — all three string arguments (what,from) are compile-time literals;idisint64from the tool argument. No user-controlled data flows into the formatted error message.ErrNotFoundmasking maintained —whenMissingre-wraps with%w: … domain.ErrNotFound …, soerrors.Is(result, ErrNotFound)stays true and the API layer's 404/masking logic is unaffected. The new message says "that you can act on", which doesn't confirm existence for resources the actor can't see.honestReplyandactedare purely structural: they inspectResult.Stepsafter the run. All permission enforcement happens upstream in the service layer (the garden-level auth check at the top ofRun, then per-call service checks in every adapter method). Nothing here bypasses or short-circuits those paths.unbackedClaimconcatenation — appended to Markdown, rendered viareact-markdownwithoutrehype-raw, so no HTML injection vector.changeClaimregex — no nested quantifiers; not vulnerable to ReDoS.One very-low-severity observation: in
describeSteps,res.Name(the tool name from the LLM's function-call response) is written into the log string via plainb.WriteString(" " + res.Name)without%q. In practice the majordomo framework only populatesres.Namewith declared tool names, so an attacker would need to both control the model response and have the LLM API accept an undeclared name — and the impact would be limited to log-line corruption, not data or auth compromise. Not worth blocking on.🎯 Correctness — Minor issues
The finding is confirmed.
public_linkis absent fromreadOnlyTools(lines 187–191), andacted()at line 234 returnstruefor any non-errored call not in that map. Apublic_link action=getcall — which routes to the read-onlyGetPublicShareLinkat tools.go:1154 — succeeds withIsError=false, soacted()returnstrueandhonestReplypasses the reply through unchanged, even if the model falsely claims it enabled or rotated the link.Minor issues
internal/agent/runtime.go:187–191—public_link action=getbypasses the honest-reply guard (confirmed)public_linkexposes four actions:get(read-only: callsGetPublicShareLink) andenable/rotate/disable(writes: callEnablePublicShareLink/DisablePublicShareLink). BecausereadOnlyToolskeys on the tool name string,public_linkcannot be listed there without also neutralising the write actions. The result: a model that callspublic_link action=getand then falsely claims "Done — I've enabled the public link" escapes correction —acted()sees a successfulpublic_linkcall (not inreadOnlyTools,IsError=false) and returnstrue, sohonestReplypasses the reply through unchanged.This is the exact scenario the PR targets. Sharing-state claims are particularly sensitive because they tell the user who can see the garden. A straightforward fix: split
public_linkinto a dedicated read-onlyget_public_linktool (which would be added toreadOnlyTools) and a write-onlyset_public_link, or haveacted()inspect the tool result or arguments to distinguishgetfrom a write action. Alternatively, document this as a known gap in thereadOnlyToolscomment — but the CLAUDE.md convention "a new read-only tool left out of it makes a turn look like it acted" leaves this unacknowledged.Everything else checks out:
whenMissingwrapsErrNotFoundwith%wsoerrors.Isat the API layer still resolves correctly; thechangeClaimregex handles all the claimed patterns correctly;acted()'s nil-safety is guaranteed at the call site beforedescribeStepsis reached.🧹 Code cleanliness & maintainability — Minor issues
Both findings are clearly visible in the actual source. Let me verify the regex structure carefully.
The concatenated regex at runtime is:
(?i)(?:^\s*(?:done|...|reverted)\b|\bI(?:'ve| have)? (?:...)\b)The full expression is balanced and valid —
regexp.MustCompilewould panic at startup if not. But confirming the draft's structural claim: the outer(?:opens on line 197 and is closed by the trailing)on line 198. Neither string is a standalone valid regex. Finding 1 is CONFIRMED as a real (if minor) maintainability fragility.Finding 2 is plainly visible: lines 188–189 pack four entries each on a single line. CONFIRMED.
Minor issues
1.
changeClaimregex outer group spans two string literals — neither half is a valid standalone regex (internal/agent/runtime.go:197–198)The outer non-capturing group
(?:opens in the first string literal; its closing)is the last character of the second. The concatenated result is a valid regex today, but neither literal alone is, which means a maintainer editing one half (adding a keyword, re-wrapping the line) has to keep the other half in mind to preserve group balance. Given thatCLAUDE.mdexplicitly flags this map as a maintenance point, the same risk applies to the regex. The safest fix is a single raw string, or restructuring so each literal is a balanced alternation joined with|:Severity: small — correct today, fragile to edit.
2.
readOnlyToolsmap packs multiple entries per line (internal/agent/runtime.go:187–191)Standard Go style is one entry per line with a trailing comma.
CLAUDE.mdcalls out this map explicitly as a maintenance point (every new read-only tool must be added here), so one-per-line formatting would make additions and diffs easier to read. Trivial to fix.Everything else in the diff is clean:
whenMissingis applied consistently across every id-taking tool call site,describeSteps/acted/honestReplyare well-factored with clear names, the test cases cover the stated failure modes without duplicating setup, and theCLAUDE.mdaddition accurately documents the implementation.⚡ Performance — No material issues found
No material issues found
All new code runs once per agent turn, after
a.Run()completes — not inside the streaming loop. I verified this atruntime.go:172–177:honestReplyis called on the finalization path, never mid-turn.acted/honestReply— Walksresult.Steps(hard-capped atmaxSteps = 24, a constant in the runner) and the smallresultsslice per step. O(steps × tools) with a tight ceiling; negligible.changeClaimregexp — Package-level compiledvar;.MatchStringis called once per turn on a short model reply string. No concern.describeSteps— Only invoked inside theif corrected != turn.Replybranch (the correction actually fired), so it's strictly the error path. Thestrings.Fields+strings.Joinnormalization allocates a temporary slice, but it runs at most once per false-claim event, on at most 24 step responses each truncated to 80 chars in the log. Not material.readOnlyToolsmap — Package-level, read-only after init; map lookup is O(1).whenMissing— Oneerrors.Ischeck and a conditionalfmt.Errorf; called per tool call, trivially cheap.🧯 Error handling & edge cases — Minor issues
Both findings are confirmed.
moveObject(line 396) andupdateObject(line 798) return the raw service error, anddeleteObject(line 807) returnsnil, err— none usewhenMissing. The byte-slice truncation at runtime.go is also present as described.Minor issues
1.
deleteObject,moveObject, andupdateObjectlackwhenMissing— inconsistent with stated goalinternal/agent/tools.go:807(deleteObject), line 396 (moveObject), line 798 (updateObject)The PR applies
whenMissingto every other id-taking mutating tool (plantings, journal entries, undo, seed lots), but the three object-level tools were left out:deleteObject(line 807):return nil, err— bare errormoveObject(line 396):return a.svc.UpdateObject(...)— bare errorupdateObject(line 798):return a.svc.UpdateObject(...)— bare errorWhen a model passes a stale object ID, it receives a bare
ErrNotFoundwith no guidance ("describe_garden lists the ids; nothing was changed"). ThehonestReplyguard is still effective, but the model has no actionable signal telling it to re-read before retrying — exactly whatwhenMissingwas designed to provide.Fix: apply the same pattern already used by the planting and journal variants:
2. Byte-boundary truncation in
describeStepsfor log outputinternal/agent/runtime.go:219text[:80]slices by byte index. If the model reply contains multi-byte UTF-8 characters (e.g. an em-dash or a non-ASCII plant name) and the 80-byte boundary falls inside one, the resulting string is invalid UTF-8. In Go this is not a panic — the slice is always in bounds after thelen(text) > 80guard — but the log line will contain a garbled character at the cut point.This is a log-only function, so the impact is cosmetic. The fix is
[]rune(text)[:80](orutf8.ValidStringcheck), but given the log-only context it's low urgency.glm-5.2:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues · ⚠️ 1/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
Verdict: No material issues found
Reviewed through the security lens:
actorIDvia the service layer's existing permission checks;honestReply,acted, anddescribeStepsonly readagent.Result/agent.Stepmetadata and the model's own reply text — they introduce no new privilege surface or auth bypass. Verified atruntime.go:121,205-256andtools.go:39-40.whenMissinginterpolates only anint64id and fixed strings into error messages (tools.go:619-624); no user/model-controlled string is formatted into SQL, a command, or a template. ThechangeClaimregex operates on the model's reply (already shown to the user), andunbackedClaimis a constant appended to that same reply — no reflection of untrusted input into a dangerous sink.describeStepslogs the first 80 chars of each step's reply text toslog.Warn. That text is the model's own user-facing reply, not credentials or request bodies; logging it is benign and consistent with existing logging inRun.readOnlyToolsallowlist (completeness). This is the closest thing to my lane, but it is a correctness/false-negative concern (omitting a future read-only tool would let a false claim stand), not a security vulnerability — the map only makes the correction fire less often; it can never grant access or suppress a real error. The CLAUDE.md note documents the maintenance requirement.Could not directly confirm the
agent.Result/agent.Stepfield names (IsError,Name,Results,Response.Text()) since themajordomo/agentvendor package is not in the checked-out tree, but existing test code (runtime_test.go:165,635) uses the same fields the same way, so resolution is not a new risk introduced here.🎯 Correctness — No material issues found
VERDICT: No material issues found
I verified the core logic against the checked-out code:
honestReply(runtime.go:250): only appends the correction whenacted(r)is false and the reply matcheschangeClaim.acted()(runtime.go:231) returns true if any tool result is non-error and not inreadOnlyTools. ThewhenMissingwrapper (tools.go:621) preservesdomain.ErrNotFoundvia%w, soerrors.Isstill holds for any caller, and the tool result remains an error result (the adapter returns(nil, err)), so a failed delete is correctly seen asIsErrorand does not satisfyacted. Logic is sound.changeClaimregex correctly excludes offers/future tense ("I'll remove it", "I will delete") —I(?:'ve| have)?consumes only've/have, and the verb list is past-tense only, so "I'll"/"will" leave a non-matching token before the verb. Verified by re-deriving against the pattern.whenMissing-wrapped handlers (movePlanting,updatePlanting,removePlanting,updateSeedLot) return the value/err pair correctly: on successwhenMissing(nil,...)returns nil; on non-ErrNotFound errors the original err passes through with the (nil) value, matching prior behavior.One minor, low-confidence note (not blocking):
readOnlyToolsis an allowlist of read-only tool names.public_linkis absent, yet itsgetaction is read-only whileenable/rotate/disablemutate. Since the name is identical across actions, listing it would be wrong (a successfulenablewould be miscounted as read-only and could trigger a false correction), so leaving it out is the safe choice — but it means a successfulpublic_link(get)counts as "acted" and could suppress a correction for an unrelated false claim in the same turn. This is an inherent limitation of name-based classification and is consistent with the documented "once any change succeeded, stay out of it" stance; flagging only as a known edge, not a defect to fix.🧹 Code cleanliness & maintainability — Minor issues
Both findings are confirmed against the actual code.
Finding 1 (readOnlyTools denylist): Confirmed at runtime.go:187-191. The map lists 9 read-only tools as string literals, and the
llm.DefineToolcalls in tools.go (37 of them by my count) carry no read-only annotation — the set is hand-synced in a separate file from the definitions it mirrors.Finding 2 (changeClaim verb duplication): Confirmed at runtime.go:197-198. The opener branch has 20 verbs; the "I" branch has 32, adding
set|put|pulled|replaced|swapped|rotated|rewrote|rewritten|edited|created|started|attached|restored|madethat the opener lacks — the two lists are kept in rough sync by hand and already differ.VERDICT: Minor issues
I reviewed the change through the cleanliness/maintainability lens. The structure is sound —
whenMissingis a nice shared helper replacing duplicated inlineerrors.Ispatterns, and the new functions are well-named and commented in the file's existing style. A few smaller cleanliness concerns:runtime.go:187-191—readOnlyToolsis a manually-maintained denylist with no enforcement. It lists 9 read-only tools by name as string literals, but thellm.DefineToolcalls intools.gocarry no read-only annotation. The CLAUDE.md note acknowledges the risk ("a new read-only tool left out of it makes a turn look like it acted"), but the list is a hand-synced string set in a different file from the tool definitions it mirrors. The set will silently drift the next time a read-only tool is added (e.g.list_sharesis present, but a futurelist_…would need to be remembered here). Worth at least a comment nearnewToolboxpointing tool authors at this map, or better, a single source of truth.runtime.go:197-198—changeClaimregex duplicates its verb list twice. The same core verbs (deleted|removed|updated|changed|added|saved|moved|…) appear both in the^\s*(…)opener branch and in the\bI(…)(…)\bbranch, kept in rough sync by hand. They already differ (the "I" branch addsset|put|pulled|replaced|swapped|rotated|rewrote|rewritten|edited|created|started|attached|restored|madethat the opener branch lacks). Extracting one verb-group string into a const would remove the duplication and the drift risk.runtime.go:200-201—unbackedClaimis a user-visible markdown string. Consistent with the neighboringfallbackReply; noted only as a magic string, not actionable.tools.go:622-625—whenMissinginterleaveserrors.Is/fmt.Errorfcorrectly and theerrors/fmtimports are already present. Clean.Nothing in my lane rises to blocking; the first item (the hand-maintained read-only denylist) is the one most likely to cause future maintainability pain.
⚡ Performance — No material issues found
VERDICT: No material issues found
Through the performance lens:
changeClaimis a package-level compiledregexp(initialized once at load), andhonestReplyruns exactly once per turn on a short reply string — not a hot path. The alternation has no nested quantifiers that could cause catastrophic backtracking; matching is linear in the reply length. No concern.actediteratesresult.Steps/Resultsonce per turn with map lookups into the package-levelreadOnlyToolsmap (O(1)). Turn-scoped, bounded by step count. Fine.describeStepsallocates astrings.Builderper step, but only on theWarnpath (i.e. only when a correction is actually appended), so it never runs on the normal successful-run path. No hot-path regression.whenMissingwraps errors with a singlefmt.Errorfper failing tool call; negligible and only on the error path.I verified the structure of the new code and the surrounding call site at
internal/agent/runtime.go:185-255and the tool definitions atinternal/agent/tools.go; nothing here introduces unbounded growth, repeated allocations in a loop, N+1 behavior, or blocking work on a hot path.🧯 Error handling & edge cases — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: max steps reached without a final answer (max 14)
kimi-k2.6:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
Through the security lens, this PR introduces guardrails against model hallucinations about performed actions and improves error messages for missing-resource failures. Nothing here opens an authn/authz gap, injection surface, SSRF, deserialization risk, or secret-leakage path.
whenMissingsafely formats integer IDs with%d; no string interpolation of user input.readOnlyToolsappears complete against the current tool registry; no read-only tool is misclassified as mutating.changeClaimis a static compiled regex applied to model output; no user-supplied pattern, and Go's regexp engine is linear-time.describeStepslogging is bounded (80 chars) and summarizes the agent's own step responses; it does not introduce a new exfiltration channel.No security findings to report.
🎯 Correctness — No material issues found
Verdict: No material issues found
I scrutinized the PR through the correctness lens:
readOnlyToolscoverage: The nine tools listed (list_gardens,describe_garden,list_years,list_plantings,find_plant,read_journal,read_history,list_seed_lots,list_shares) are the purely read‑only tools defined in the toolbox. The mixedpublic_linktool is intentionally excluded (itsgetaction is read‑only butenable/rotate/disablemutate state), which is a documented design limitation, not a bug.whenMissingwrapping: Every tool the PR targeted (update_planting,move_planting,remove_planting,delete_planting,undo_change,update/delete_journal_entry,update/delete_seed_lot) now wrapsdomain.ErrNotFoundwith a message that names the lookup tool and says “nothing was changed.” The wrapper preserves the error type (%w), soerrors.Is(..., domain.ErrNotFound)still succeeds for downstream callers.changeClaimregex: Tested boundary cases mentally. It correctly matches “Done …”/“Fixed …” openers and first‑person past‑tense claims (“I’ve deleted”, “I moved”). It avoids matching offers/questions (“want me to delete it?”), as the test verifies. False positives/negatives exist in theory (e.g., “Updated documentation is available” vs. “It is fixed”), but these are inherent to the regex approach and the PR explicitly accepts this trade‑off.acted()logic: Returnstrueonly if a non‑error, non‑read‑only tool result exists. A failed state‑changing call (IsError == true) does not count as “acted,” so a model claiming success over a failed deletion is still corrected—exactly the intended behavior, verified by the test.No semantic or logic bugs introduced by the change.
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
internal/agent/runtime.go:197-198—changeClaimduplicates its verb inventory across two alternations. A single source of truth (e.g., a slice composed into the pattern) would be easier to keep consistent and less prone to omissions. As it stands, the two lists already diverge:created,set,made,editedand others appear only in theI've …branch, whiledoneappears only in the sentence-opener branch. That makes the regex harder to maintain and more likely to miss new claim phrasings that get added to one list but not the other.⚡ Performance — No material issues found
Verdict: No material issues found
The changes under review add correctness guards and richer error messages. Through the performance lens, the new code is benign:
changeClaimis a compiledregexpchecked once per turn viaMatchString. Go’s RE2 engine runs in linear time in the input size, and the input (the model’s reply) is bounded to a few KB at most. Not a hot-path concern.actediterates over steps and results once per turn. In practice this is a handful of iterations; no quadratic or N+1 behavior.describeStepsallocates only when a correction is actually emitted (the rareWarnpath), and its work is proportional to the small number of steps already in the result.whenMissingwraps errors on failure paths only; it does not add new I/O, allocations on success paths, or unbounded growth.🧯 Error handling & edge cases — No material issues found
No material issues found
Through the error-handling and edge-cases lens, the change is solid:
readOnlyToolsis complete: every truly read-only tool is listed, and no listed tool modifies state. The set is intentionally conservative (e.g.public_linkis absent because it can write), which is the right trade-off for this guard.acted()correctly skips error results, so a faileddelete_journal_entry(or an unconfirmedshare_garden) does not make a turn look like it acted.changeClaimregex is carefully scoped to past-tense claims and excludes questions/offers. False-positive analysis on patterns like "I haven't deleted", "I've been deleted", "I have a fixed schedule", and "I want to delete it" shows no match — the alternations and word boundaries hold up.honestReplyhandlesnilresult defensively and only appends the correction when the claim regex matches and no non-read-only tool succeeded. Empty replies and empty step lists fall through correctly.whenMissingwraps onlydomain.ErrNotFound(usingerrors.Isso wrapped errors work), passes everything else through, and produces a useful actionable message with the ID and the lookup tool.Edge cases that are outside the diff's scope (e.g. a
public_linkwithaction=getis treated as modifying, orshare_gardenthat finds an identical existing share returns success without change) are acknowledged limitations of the design, not regressions introduced here.Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.