Fix A1 of plans/2026-07-11-nomos-agent-code-review.md. isAssent and
isTypedConfirmation used a space-padded word-boundary check for negation
words but a bare strings.Contains for assent/confirm words — confirmed live
via test probes: isAssent("...maybe yesterday's logs...") returned true
("yes" matched inside "yesterday"), and isTypedConfirmation("I haven't
confirmed anything yet") returned true ("confirm" matched inside "confirmed",
and "haven't" wasn't in negationWords — only "don't"/"do not" were).
isTypedConfirmation is the sole gate for DESTRUCTIVE actions, so the second
case meant a message merely stating something hadn't been confirmed could
read as an explicit confirmation.
- Replaced the ad-hoc space-padding/prefix-check negation logic with proper
tokenization (wordTokenRe) + containsPhrase, matching WHOLE tokens/phrases
only — never a mid-word substring. Handles curly apostrophes too (a
pre-existing gap: the old straight-quote-only check would have missed
"don't" typed with a smart quote).
- Added contracted negatives (haven't, hasn't, isn't, wasn't, aren't, can't,
cannot, won't, wouldn't, shouldn't, didn't, doesn't) to negationWords.
Deliberately did NOT add a bare "not" — too broad, would false-negative
ordinary assent like "go ahead, this is not risky".
- Added regression tests for both confirmed cases plus a couple of adjacent
ones (eyesight/isn't, can't confirm) so a future change can't silently
reintroduce either bug.
All existing assent/confirmation tests pass unchanged — this is a pure
robustness fix, not a behavior change for any previously-correct case.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
181 lines
6.9 KiB
Go
181 lines
6.9 KiB
Go
package main
|
||
|
||
import (
|
||
"bytes"
|
||
"context"
|
||
"encoding/json"
|
||
"fmt"
|
||
"net/http"
|
||
"regexp"
|
||
"strings"
|
||
)
|
||
|
||
// Chat-assent approval: the operator authorizes a proposed action by
|
||
// replying normally in chat ("go ahead", "yes", "do it") instead of clicking
|
||
// a separate Approve button. This is deterministic (not LLM-judged) so it
|
||
// can't be talked around by a model that misreads intent, and it only ever
|
||
// looks at the assistant turn immediately preceding the operator's reply —
|
||
// an old "yes" from three messages ago can never retroactively approve
|
||
// something new. Destructive-risk actions are excluded: they always need the
|
||
// explicit typed-confirmation flow, never loose assent.
|
||
|
||
// pendingApproval is one gated action proposed in the immediately-preceding
|
||
// assistant turn, extracted from its tool_result text.
|
||
type pendingApproval struct {
|
||
execID string
|
||
destructive bool
|
||
}
|
||
|
||
// executionQueuedRE matches the "execution <uuid> queued" phrasing shared by
|
||
// the run and request_execution/pct_create tool result messages.
|
||
var executionQueuedRE = regexp.MustCompile(`(?i)execution\s+([0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12})\s+queued`)
|
||
|
||
// extractPendingApprovals scans the tool results of one assistant turn for
|
||
// gated actions that are still awaiting a decision.
|
||
func extractPendingApprovals(calls []persistedCall) []pendingApproval {
|
||
var out []pendingApproval
|
||
for _, c := range calls {
|
||
text := c.resultText()
|
||
m := executionQueuedRE.FindStringSubmatch(text)
|
||
if m == nil {
|
||
continue
|
||
}
|
||
out = append(out, pendingApproval{
|
||
execID: m[1],
|
||
destructive: strings.Contains(strings.ToUpper(text), "DESTRUCTIVE"),
|
||
})
|
||
}
|
||
return out
|
||
}
|
||
|
||
// negationWords, checked first: any of these anywhere in the message means
|
||
// the reply is NOT assent, even if a positive word also appears (e.g. "no,
|
||
// don't restart it yet" contains neither "yes" nor "go ahead", but "wait"
|
||
// alone should also block a stray "yes" a sentence later — checking negation
|
||
// first and returning false errs toward re-confirming rather than assuming
|
||
// consent, per "when in doubt, escalate"). Includes contracted negatives
|
||
// ("haven't", "isn't", ...) alongside "don't"/"do not" — found live: "I
|
||
// haven't confirmed anything yet" was reading as an explicit confirmation
|
||
// because none of the contracted forms were covered, only "don't"/"do not".
|
||
// Deliberately does NOT include a bare "not": that's broad enough to false-
|
||
// negative ordinary assent ("go ahead, this is not risky") — the specific
|
||
// contracted-verb forms below are unambiguous negation on their own.
|
||
var negationWords = []string{
|
||
"no", "nope", "don't", "do not", "stop", "wait", "hold on", "hold off",
|
||
"not yet", "cancel", "nevermind", "never mind", "actually don't", "skip that",
|
||
"haven't", "hasn't", "isn't", "wasn't", "aren't", "can't", "cannot",
|
||
"won't", "wouldn't", "shouldn't", "didn't", "doesn't",
|
||
}
|
||
|
||
// assentWords, checked only if no negation matched.
|
||
var assentWords = []string{
|
||
"go ahead", "goahead", "yes", "yep", "yeah", "yup", "do it", "proceed",
|
||
"approve", "approved", "confirm", "confirmed", "ship it", "sounds good",
|
||
"lgtm", "run it", "execute", "ok go", "okay go", "please do",
|
||
}
|
||
|
||
// wordTokenRe splits a message into lowercase word tokens. Apostrophes
|
||
// (straight ' and curly ’) stay attached to their word so "don't"/"haven't"
|
||
// tokenize as one token, not two.
|
||
var wordTokenRe = regexp.MustCompile(`[a-z0-9'’]+`)
|
||
|
||
func tokenize(msg string) []string {
|
||
return wordTokenRe.FindAllString(strings.ToLower(strings.ReplaceAll(msg, "’", "'")), -1)
|
||
}
|
||
|
||
// containsPhrase reports whether phrase (one or more words) appears as a
|
||
// consecutive run of WHOLE tokens in tokens — never a mid-word substring
|
||
// match. This is the fix for a real false positive found live: the old
|
||
// substring check (`strings.Contains(m, "yes")`) matched "yes" inside
|
||
// "yesterday", and "confirm" inside "confirmed"/"unconfirmed" without regard
|
||
// for word boundaries. Negation already used a word-boundary check
|
||
// (space-padded); assent/confirm words didn't — this brings both onto the
|
||
// same, more robust tokenized comparison instead of ad-hoc string padding.
|
||
func containsPhrase(tokens []string, phrase string) bool {
|
||
words := strings.Fields(phrase)
|
||
if len(words) == 0 || len(words) > len(tokens) {
|
||
return false
|
||
}
|
||
for i := 0; i+len(words) <= len(tokens); i++ {
|
||
match := true
|
||
for j, w := range words {
|
||
if tokens[i+j] != w {
|
||
match = false
|
||
break
|
||
}
|
||
}
|
||
if match {
|
||
return true
|
||
}
|
||
}
|
||
return false
|
||
}
|
||
|
||
// isAssent reports whether msg is a plain-language authorization of a
|
||
// pending proposal. Deliberately simple and auditable: a fixed word list,
|
||
// not a model judgment call, so behavior is predictable and can't be
|
||
// prompt-injected via the pending action's own content.
|
||
func isAssent(msg string) bool {
|
||
tokens := tokenize(msg)
|
||
for _, w := range negationWords {
|
||
if containsPhrase(tokens, w) {
|
||
return false
|
||
}
|
||
}
|
||
for _, w := range assentWords {
|
||
if containsPhrase(tokens, w) {
|
||
return true
|
||
}
|
||
}
|
||
return false
|
||
}
|
||
|
||
// isTypedConfirmation reports whether msg is an explicit confirmation strong
|
||
// enough to grant a DESTRUCTIVE pending action. Deliberately a separate,
|
||
// stricter check from isAssent: a bare "yes"/"go ahead"/"proceed" must never
|
||
// grant something destructive, only an explicit "confirm" statement does —
|
||
// this is the typed-confirmation phrase SOUL.md tells the operator to use
|
||
// ("I confirm destroy 135"). Still negation-aware for the same reason as
|
||
// isAssent: "don't confirm yet" must not accidentally match.
|
||
func isTypedConfirmation(msg string) bool {
|
||
tokens := tokenize(msg)
|
||
for _, w := range negationWords {
|
||
if containsPhrase(tokens, w) {
|
||
return false
|
||
}
|
||
}
|
||
return containsPhrase(tokens, "confirm") || containsPhrase(tokens, "confirmed")
|
||
}
|
||
|
||
// approveExecution grants (or denies) a pending execution via the same HTTP
|
||
// endpoint the chat UI's Approve button calls, so both paths share one code
|
||
// path server-side (executeApprovedAction) and one audit trail. Returns the
|
||
// decided status, or an error if the request failed outright (a 4xx for an
|
||
// already-decided/expired approval is reported via ok=false, not a hard err,
|
||
// since that's an expected race, not a bug).
|
||
func (a *agent) approveExecution(ctx context.Context, execID string) (ok bool, status string, err error) {
|
||
if a.apiBase == "" {
|
||
return false, "", fmt.Errorf("no API base configured")
|
||
}
|
||
body, _ := json.Marshal(map[string]string{"decision": "approve"})
|
||
req, err := http.NewRequestWithContext(ctx, http.MethodPost,
|
||
a.apiBase+"/api/v1/approvals/"+execID+"/decision", bytes.NewReader(body))
|
||
if err != nil {
|
||
return false, "", err
|
||
}
|
||
req.Header.Set("Content-Type", "application/json")
|
||
resp, err := a.httpClient.Do(req)
|
||
if err != nil {
|
||
return false, "", err
|
||
}
|
||
defer resp.Body.Close()
|
||
if resp.StatusCode != http.StatusOK {
|
||
return false, "", nil // already decided / expired / not found — not a hard failure
|
||
}
|
||
var out struct {
|
||
Status string `json:"status"`
|
||
}
|
||
json.NewDecoder(resp.Body).Decode(&out)
|
||
return true, out.Status, nil
|
||
}
|