fix(engine): warn once about a bad allowlist flag, not per tool call #10

Merged

thklein merged 1 commit from fix/flag-warn-once into main 2026-09-13 09:22:33 +00:00

Conversation 0 Commits 1 Files changed 2 +37 -2

thklein commented 2026-09-13 09:22:24 +00:00

Owner

Copy link

Found while reviewing the bump of gptscript#9 into bpai, before anything has set the flag — which is the window worth fixing it in, since enabling it is the entire point of that bump.

strictToolEnv is re-evaluated on every tool invocation. That is deliberate: it keeps the check cheap and lets t.Setenv drive it in tests. But the misconfiguration warning sat inside it, so an operator writing GPTSCRIPT_TOOL_ENV_ALLOWLIST: "yes" in a values file would get a WARN line per tool call, forever — turning a one-line mistake into a log flood, at the exact moment they are reading those logs to work out why their tools changed behaviour.

The decision stays per call. Only the warning moves behind a sync.Once.

The test asserts on the sync.Once itself rather than on captured output: the Once is the mechanism, and a test that swapped the logger would pass against a version that logged every time through some other path.

Verified locally (this fork's CI is red on main and gates nothing): go build ./..., go test ./pkg/engine/... -count=1, golangci-lint run ./pkg/engine/... — the only lint finding is the pre-existing import-ordering one on cmd.go, byte-identical on main.

Found while reviewing the bump of gptscript#9 into bpai, before anything has set the flag — which is the window worth fixing it in, since enabling it is the entire point of that bump. `strictToolEnv` is re-evaluated on **every tool invocation**. That is deliberate: it keeps the check cheap and lets `t.Setenv` drive it in tests. But the misconfiguration warning sat inside it, so an operator writing `GPTSCRIPT_TOOL_ENV_ALLOWLIST: "yes"` in a values file would get a `WARN` line **per tool call, forever** — turning a one-line mistake into a log flood, at the exact moment they are reading those logs to work out why their tools changed behaviour. The decision stays per call. Only the warning moves behind a `sync.Once`. The test asserts on the `sync.Once` itself rather than on captured output: the Once *is* the mechanism, and a test that swapped the logger would pass against a version that logged every time through some other path. Verified locally (this fork's CI is red on `main` and gates nothing): `go build ./...`, `go test ./pkg/engine/... -count=1`, `golangci-lint run ./pkg/engine/...` — the only lint finding is the pre-existing import-ordering one on `cmd.go`, byte-identical on `main`.

thklein added 1 commit 2026-09-13 09:22:24 +00:00

fix(engine): warn once about a bad allowlist flag, not per tool call ... 8aeb56e41f

strictToolEnv is re-evaluated on every tool invocation — deliberately,
so it stays cheap and testable — and the misconfiguration warning sat
inside it. An operator writing "yes" in a values file would therefore
get a WARN line for every tool call the instance ever makes, flooding
the exact logs they are reading to find out what went wrong.

The decision stays per call; only the warning moves behind a sync.Once.

Found reviewing the bump of this commit into bpai, before anything set
the flag — which is the window worth fixing it in, since enabling it is
the whole point of that bump.

thklein merged commit 7738a4cabb into main 2026-09-13 09:22:33 +00:00

thklein deleted branch fix/flag-warn-once 2026-09-13 09:22:33 +00:00

thklein referenced this pull request from a commit 2026-09-13 09:22:34 +00:00

fix(engine): warn once about a bad allowlist flag, not per tool call (#10)

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

area/engine

  

clarity/clear

  

enhancement

  

security

  

size/M

No labels

area/engine

clarity/clear

enhancement

security

size/M

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

basepeak-ai/gptscript!10

No description provided.