mirror of
https://github.com/Twigpine/zero.git
synced 2026-11-10 13:11:31 -10:00
fix/opencode-session-header
10
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 KRATOS
|
6fe0d1edcd |
fix(lockutil): use kernel-held advisory locks (#950)
* fix(lockutil): use kernel-held advisory locks * fix(lockutil): harden cross-platform lock files * test(lockutil): require one concurrent holder |
||
|
euxaristiaandClaude Sonnet 5
|
97ef90d27a |
chore: remove code unreachable on every supported platform (#706)
* chore: remove code unreachable on every supported platform The Security and code health job's advisory deadcode step reports 152 unreachable functions on main. Running the same check under GOOS=linux, windows, and darwin shows 13 of those are false positives whose callers live in windows build-tagged files, so only functions unreachable on all three platforms are touched. Functions with no references anywhere are deleted, including the internal/reasoning package, which nothing imports. Functions that only tests reference move into per-package export_test.go files so tests keep their seams while the production binary sheds the code. Test call sites of deleted convenience wrappers are ported to the live APIs (scoped tool constructors, tools.DeferredSource, FinishReason checks). Kept despite the report: cron.Store.Runs (only reader of runs.jsonl, which production writes), and agenteval.TraceEventKeys and MissingTraceEvents (used across packages by the trace contract test). * docs(config): correct ValidateFile's errors.As claim Message is a flattened string (.Error() already called on the parse error), not a wrapped error, so callers can't recover the concrete json error type via errors.As. Fix the comment to match. Co-Authored-By: Claude Sonnet 5 <[email protected]> * chore(trace,skills): drop obsolete Sink adapters and fix stale doc references The dead-code pass removed the Emit methods but left Sink, NDJSONSink, and TextSink exported with a contract nothing implements; remove the types with their contract. Also remove the stray blank line at EOF of trace.go flagged by gofmt, and update skills package docs and HOW_ZERO_WORKS.md to stop directing callers to DiscoveryRoots/Duplicates, which are now test seams. Co-Authored-By: Claude Fable 5 <[email protected]> * chore: clean up remaining review nits on dead code removal Replace a pointless fmt.Errorf(...%w...).Error() with fmt.Sprintf using %v, since the wrapped error was discarded immediately anyway. Remove a comment block that was duplicated verbatim. * docs(skills): drop remaining references to test-only List/Get Load and MergedSkills comments still named List/Get as production entry points after they moved to export_test.go. Point callers at Load and ListFromRoots instead. * chore: remove now-unreferenced export_test.go seams execSessionManager.len() and likelySandboxDenied/sandboxDenialKind had no remaining callers after the sandbox unify-command-execution rebase; the production code they wrapped for tests no longer exists. --------- Co-authored-by: Claude Sonnet 5 <[email protected]> |
||
|
PierrunoYTandClaude Sonnet 5
|
d9b882eb0a |
fix(cron): reserve job IDs atomically (#686)
* fix(cron): reserve job IDs atomically * fix(cron): hold the per-ID lock from reservation through persistence Address code review on PR #686: reserveID made the timestamp directory visible via os.Mkdir before metadata.json existed, but Remove (which takes the per-ID lock before deleting) never coordinated with that window. A concurrent `zero cron rm <id>` could delete the bare reservation and report success right after Add reserved it; writeJob's MkdirAll would then silently resurrect the directory and persist the job anyway. A second Add could also re-reserve the same id while the first writer was still running, putting two writers on the same metadata path. reserveID now acquires the same per-ID lock (used by Get/Update/ Mutate/Remove/AppendRun) for each candidate id before attempting os.Mkdir, and returns it still held; Add releases it only after writeReservedJob commits (or tears down) the reservation. A concurrent Remove for that id now blocks until the reservation is resolved one way or the other, and a concurrent Add attempting the same candidate id serializes through the lock rather than only racing the Mkdir. Adds two regression tests: one proving Remove waits for and removes the committed job rather than the bare reservation, and one proving a second reserveID call for the same candidate id blocks on the shared lock and moves on to the next candidate. Co-Authored-By: Claude Sonnet 5 <[email protected]> --------- Co-authored-by: Claude Sonnet 5 <[email protected]> |
||
|
euxaristia
|
6fc1220f6a |
fix(sandbox): scrub sensitive credentials from sandbox environment (#660)
* fix(windows): retry atomic file renames on Access is denied / Sharing violation * test(swarm): add unit tests for rename retry and non-retryable errors * test(swarm): skip TestMailboxRenameRetry on non-Windows platforms * fix(sandbox): scrub sensitive provider credentials from sandbox env * refactor(sandbox): unify environment creation and reduce lines of code * fix(sandbox): forward workspaceRoot through sandboxEnvironment, gate Windows env on backend * style(sandbox,swarm): remove stray blank lines flagged by gofmt * refactor(fsutil): extract shared rename retry helper, derive scrub list from provider catalog Address CodeRabbit review feedback: - Add internal/fsutil.RenameWithRetry with an injectable rename override and move the four duplicated renameWithRetry/isWindowsSharingViolation copies (cron, sessions, sandbox, swarm) onto it. Mailbox keeps its rename test seam by passing its field through. Add fsutil tests for the Windows retry and non-retryable paths. - Remove the orphaned metadata.json.tmp in cron writeJob when the rename permanently fails, matching the sibling writers. - Derive scrubSensitiveEnv keys from providercatalog AuthEnvVars plus a static list of non-catalog secrets, so new providers are scrubbed automatically. AWS_PROFILE and GOOGLE_APPLICATION_CREDENTIALS stay because they are pointers, not secrets. * fix(sandbox): deny sandboxed reads of cloud credential stores by default Address CodeRabbit finding that scrubbed env pointers still left the underlying credential files readable under the read-all workspace posture: - Add credentialDenyReadPaths: default profile-level deny-read entries for ~/.aws, ~/.config/gcloud, ~/.azure, and the GOOGLE_APPLICATION_CREDENTIALS target file, wired into both permission profile builders so the seatbelt and Linux helper backends enforce them. Entries are stat-filtered and a store nested under a user AllowRead entry is dropped, keeping an explicit opt-out. - Keep the defaults out of Policy.DenyRead, whose emptiness gates escalated execution, and skip Windows so the profile deny list does not flip the runner off the WRITE_RESTRICTED token path. - Scrub GOOGLE_APPLICATION_CREDENTIALS from the sandbox env; keep AWS_PROFILE since the deny-read rule on ~/.aws is the real protection and the SDKs fall back to the default profile regardless. - Add credential deny path tests, extend the scrub test, relax the profile test's exact deny count to containment. * test(cli): pin HOME in sandbox policy golden test The default credential-store deny-read entries depend on what exists in the real home directory (the macOS CI image ships ~/.aws), which leaked host paths into the policy JSON golden comparison. Point HOME at an empty temp directory so the golden stays hermetic. * fix(sandbox): protect GOOGLE_APPLICATION_CREDENTIALS without a home, retry lock violations Address CodeRabbit review feedback: - credentialDenyReadPaths no longer returns early when the home lookup fails; home-based candidates become conditional and the GOOGLE_APPLICATION_CREDENTIALS target is always considered. - fsutil.RenameWithRetry also retries ERROR_LOCK_VIOLATION (errno 33), matching the mcp lock handling; helper renamed to isWindowsSharingOrLockViolation. - Extend tests: homeless GOOGLE_APPLICATION_CREDENTIALS case and a lock violation step in the retry sequence. * fix(sandbox): tighten env-scrub test assertion and add missing secret keys TestScrubSensitiveEnv only checked an allowlist plus a length count, so a duplicate allowed key silently replacing a required one would still pass. Compare the exact expected key/value map instead. Also add ZERO_WEBSEARCH_API_KEY and ZERO_DAEMON_REMOTE_TOKEN to scrubSensitiveEnv's sensitiveKeys list: both are real bearer secrets read from the environment but were missing from the scrub list, leaving them readable by sandboxed commands when set in the parent shell. |
||
|
euxaristia
|
da41c3a75b |
fix(lock): prevent POSIX lock file overwrite and leak on Windows/Unix (#628)
* fix(lock): prevent POSIX lock file overwrite and leak on Windows/Unix During lock reclamation, reclaiming a suspected stale lock renames the file aside and renames it back if not stale. On POSIX systems, the rename back silently overwrites any new lock file created in the gap. On Windows, it fails to rename back and leaks the stale file. Introduce a cross-platform restoreLockFile helper: - On POSIX: Use os.Link to link the file back (which atomic-fails if the target already exists), then remove the temporary link. - On Windows: Use os.Rename (which safely fails if the target exists). If restoring fails, delete the temporary file immediately to prevent leaks. * fix(lockutil): fix Windows build by using golang.org/x/sys/windows Addresses jatmn's follow-up review: the required Windows smoke check was failing because syscall.MoveFileEx is not exported by the standard library's syscall package on this platform, so internal/lockutil (and everything importing it: cron, daemon, hooks, oauth) failed to compile on Windows. Switched to golang.org/x/sys/windows, which does export MoveFileEx with the same signature, already a direct module dependency. Verified on a real Windows machine that the no-flags call still fails with ERROR_ALREADY_EXISTS when the destination exists, and that this satisfies errors.Is(err, os.ErrExist), which cron/daemon/hooks/oauth's reclaim paths all depend on to decide whether the sidelined lock was genuinely superseded (safe to discard) versus some other, unrelated failure (must be preserved). TestRestoreLockFile already covered both the successful-restore and fail-when-target-exists cases; it now passes on Windows. * fix(lockutil): route lock file deletes through RemoveLockFile with Windows retry loop * fix(lockutil): close restore split-brain and unify RemoveLockFile RestoreLockFile could fail with errors other than os.ErrExist (hardlink- incapable filesystems such as FAT or some FUSE/network mounts, ENOSPC, Windows sharing violations). Callers only clean up on os.ErrExist, so any other failure left the lock path missing with the live holder's lock stranded in the sidelined file; the next O_EXCL create then succeeded and two processes held the lock. RestoreLockFile now falls back to an O_EXCL copy in that case, which preserves the no-overwrite guarantee, and a successful restore with a failed sidelined-file cleanup reports nil instead of a failed restore. RemoveLockFile now has one cross-platform contract: removing an already missing file reports nil on POSIX too, matching Windows, and daemon release() drops its caller-side ErrNotExist filter. Tests: TestRestoreByCopy and TestRemoveLockFile run on all platforms; Windows-tagged tests pin the ERROR_ALREADY_EXISTS mapping to os.ErrExist and exercise the RemoveLockFile retry loop to success and to bounded exhaustion. The package doc comment documents the shared contract. * refactor(swarm): extract reclaimStaleLock to mirror other lock callers The stale-lock reclaim in acquireLock was inline, so its restore path (a holder rotated a fresh lock in the gap between the stale check and the rename) could not be regression-tested the way cron, daemon, hooks, and oauth test theirs. Extract it into reclaimStaleLock with the same shape as the other callers, behavior unchanged, and add the mirrored regression test. * test(hooks,oauth): add stale lock reclaim regression tests Mirror the cron reclaim test in the two packages that lacked coverage for reclaimStaleLock: a genuinely stale lock is reclaimed, a fresh lock (reacquired in the gap) is restored intact rather than stolen, and a missing lock reports no reclaim. daemon already had equivalent tests in lock_test.go. * fix(lock): fail closed and drop sidelined file when restore fails A non-os.ErrExist error from RestoreLockFile means both the no-replace restore and its O_EXCL copy fallback failed, so the lock path may be missing while the sidelined holder is live. Callers previously swallowed that error and their next O_EXCL create would succeed, breaking mutual exclusion. reclaimStaleLock in cron, daemon, hooks, oauth, and swarm now returns an error for that case and the acquire loops fail closed instead of re-acquiring. The sidelined .stale.* file is now removed on every restore failure, not only os.ErrExist: once the restore has failed it has no protocol function (release only consults the lock path), so keeping it only leaked files. Also replace the numeric errno literals in the Windows RemoveLockFile retry loop with windows.ERROR_SHARING_VIOLATION and windows.ERROR_ACCESS_DENIED. * refactor(lockutil): extract shared ReclaimStaleLock helper Consolidate the duplicated reclaimStaleLock functions from cron, hooks, oauth, and swarm into lockutil.ReclaimStaleLock. Each caller now passes an mtime-based isLive predicate to the shared helper, which owns the rename-aside, restore-if-live, cleanup, ErrExist handling, and fail-closed error propagation. Daemon keeps its own reclaimStaleLock because it uses PID-based liveness (different predicate shape and no staleAfter duration). The per-package reclaim regression tests are removed since the shared helper is thoroughly tested in lockutil/reclaim_test.go (dead lock reclaimed, live lock restored, missing lock, fail-closed on restore error, sidelined file dropped when new holder wins). * fix(lockutil): propagate non-contention rename failures from ReclaimStaleLock A rename-aside failure that is not a lost race previously mapped to the benign lost-race result, so acquirers spun until their deadline and reported a timeout instead of the real failure. Keep only a missing source and the Windows sharing-violation/access-denied contention errnos benign (neither Windows errno maps to os.ErrExist, and ERROR_SHARING_VIOLATION does not map to os.ErrPermission either, so the classification compares raw errnos) and return everything else so callers fail closed. Also convert daemon's private reclaimStaleLock to the shared helper so it picks up the same contract, and reword the caller error messages from "restore reclaimed" to "reclaim stale" since the error can now come from the rename aside as well. * fix(lockutil): publish restore-by-copy atomically and honor lost mailbox reclaims restoreByCopy created the destination lock file empty before copying into it, so a crash mid-copy could leave a live holder's lock permanently unrepresented at the lock path. Stage the copy under a private name and publish it with the platform's no-replace primitive instead. swarm's mailbox lock discarded the reclaimed boolean and always retried immediately, so a lost reclaim race (or Windows sharing-violation contention) could hot-spin past the lock timeout instead of falling through to the bounded wait like the other lock callers. * fix(lockutil): shrink the reclaim/restore window with a fast replacing rename Restoring a lock that turned out live went through the no-replace path (plus its copy fallback on some failures) first, which is slow enough that a fourth process's unrelated O_EXCL create could win lockPath while it was absent for the whole call. RestoreLockFile's no-replace contract would then report os.ErrExist and the caller silently dropped the sidelined lock, orphaning the still-running original holder. restoreLiveLock now tries a single replacing rename straight back first, falling back to the no-replace path only if that fails. It shrinks the window from a whole restore call down to roughly one syscall; it does not close it, since the fast path can silently overwrite a competing lock rather than detect it. A fully race-free reclaim would need an OS-level advisory lock checked non-destructively instead of by moving the file, which is a larger change than this. |
||
|
KRATOS
|
b0a280eaf6 |
audit: fix four high-severity concurrency/correctness bugs (#261)
Surfaced by a full-tree audit; all are latent (the suite passed) and live in race windows / error edges the tests don't exercise. Each ships a regression test. - swarm: liveAgents() now counts queued specs, so AdoptOrphans can no longer re-dispatch (double-execute) a task whose member is merely waiting for a free slot over the concurrency cap. - cron/hooks/oauth: stale-lock reclaim is now atomic (rename aside, then verify-and-restore if it turns out fresh) instead of a blind Remove that let two racers both reclaim and hold the lock; also falls through to the bounded wait instead of hot-spinning when a reclaim never wins. - lsp: a dead language-server session is evicted and restarted instead of being returned forever, so one server crash no longer permanently breaks diagnostics (and spuriously fails self-correct every iteration). - config: an API-key env var no longer overwrites a same-named compatible-transport provider's kind when no base URL is supplied. |
||
|
KRATOS
|
1c30f4c117 |
audit fixes: cron lock, accounting race, O(n²) hot paths, /style wiring (#260)
* audit fixes: cron lock, accounting race, O(n^2) hot paths, /style wiring
Fixes five verified findings from a code audit. Two related findings were already
addressed upstream and three need larger/separate work (noted below).
H-1 cron store: cross-process lock for the read-modify-write.
Two schedulers seeing a job due raced the metadata read-modify-write; fireJob
even documented it ("full atomicity needs file locking"). Adds a per-job O_EXCL
lock (mirroring internal/oauth's pattern, stale-reclaim included) and a Mutate
primitive doing the re-read+modify+write atomically. Update/Remove lock too. The
lock is held only for the millisecond RMW, never across a job's exec.
M-5 specialist accounting: close the check-then-append race.
specialistEventExists (unlocked ReadEvents) + a later AppendEvent let two
finishers (TaskOutput poll vs onExit, even cross-process) both pass the check and
write duplicate stop/usage events. Adds sessions.Store.AppendEventUnlessExists
(check + append atomic under the session lock) and routes the stop and usage
paths through it.
M-2 hooks audit: O(1) sequence via tail read.
append re-read and scanned the whole audit log every call to find the max
sequence (O(n^2) over a session). Reads only the file tail now; multi-process
safe (the log is a shared global file), unlike an in-memory counter which would
collide sequences across processes.
M-4 transcript rehydration: O(n^2) -> O(n).
The three resume/rewind/compact rehydration loops called appendTranscriptRow per
event, each re-scanning every existing row for the keyed-row dedup. A bulk
appendTranscriptRowsDedup builds the key set once; semantics are identical.
M-6 /style: actually shape responses.
The TUI /style command stored a style but never threaded it anywhere. Adds
agent.Options.ResponseStyle, a system-prompt directive for concise/explanatory/
review, and wires m.responseStyle through the run. "balanced" is a no-op (prompt
byte-identical to before).
Already addressed upstream (no change):
- M-1 forked-session double-count: Store.Fork already skips EventUsage events,
with a comment naming the exact double-count it prevents.
- M-3 lineage O(N^2): Tree already does a single store.List() scan and indexes
children in memory; no path calls List per node.
Deferred (need larger/separate changes):
- M-7 escalate_model in the TUI needs a ModelSwitcher that builds a provider
mid-run; registering the tool alone yields a tool that errors on call.
- L-1 MCP SSE reconnect is a stream-lifecycle change (low-likelihood after the
8 MiB per-event cap).
- L-2 dead-code removal belongs in a dedicated cleanup sweep.
Tests: per-fix unit tests including concurrency (cron Mutate, sessions
AppendEventUnlessExists) under -race; full go test ./... green.
* fix(windows): treat ERROR_ACCESS_DENIED as lock contention (cron + oauth)
The Windows smoke run failed TestMutateIsAtomicUnderConcurrency with
"...lock: Access is denied" (FireCount 19/20). On Windows a concurrent holder's
os.Remove leaves the lock file in a delete-pending state, so an O_EXCL create
races it with ERROR_ACCESS_DENIED (os.ErrPermission) rather than ErrExist — the
retry loop only treated ErrExist as contention and hard-failed.
Treat os.ErrPermission as contention (retry) in the new cron lock and in the
oauth lock it mirrors (oauth's own concurrent test exposed the same pre-existing
bug under the added load). Unix is unaffected — it returns ErrExist.
* fix: address review — cross-process atomic audit append + AppendRun guard
Addresses the review feedback on this PR.
P1 (blocking): AuditStore.append is now cross-process atomic.
The O(1) tail read observes another process's prior appends, but the read-then-
append was not atomic across processes — two could both read sequence N and write
N+1. Wraps lastSequence()+append in a per-file O_EXCL lock (hooks/lock.go, same
stale-reclaim + Windows ErrPermission handling as cron/oauth); store.mu stays as
the in-process fast path. Adds TestAuditStoreSequenceConcurrent (4 stores x 12
goroutines on one file -> unique dense sequences), which the prior sequential
test could not catch. Walked back the comment that oversold the tail read.
P2: AppendRun no longer resurrects a removed job.
fireJob's AppendRun ran after Mutate released the per-job lock; a Remove in that
window let AppendRun's MkdirAll recreate the dir with an orphaned runs.jsonl.
AppendRun now takes the per-job lock and bails if metadata.json is gone. Adds
TestAppendRunDoesNotResurrectRemovedJob.
P2: lastSequence ReadAt tolerates a short final read.
ReadAt may return io.EOF with n < len(buf) if the file shrank between Stat and
the read; treat EOF as non-fatal and use buf[:n] (matches lastEventSequence).
P3: oauth stale-reclaim no-op removed.
The double os.ReadFile "stale == data" guard compared the file to itself read
twice in a row (always true); staleness is decided by the mtime check, so the
guard is dropped (behavior-preserving).
Tests under -race; cron/hooks/oauth green. A shared filelock helper would DRY the
three near-identical locks — left as a follow-up to avoid churning the
already-green cron/oauth packages here.
|
||
|
KRATOS
|
474f494b93 |
Deep-audit remediation + sandbox hardening (batches 1–35) (#189)
* audit batch 1: redact nested secrets fully; count image tokens in compaction estimate
- secrets.Redact: replace longest matches first so a containing PRIVATE KEY
block is redacted before a nested shorter match corrupts its exact string
(audit medium internal/secrets/scanner.go:81) + regression test.
- agent compaction estimateTokens: add a flat per-image token cost so an
image-heavy context trends toward compaction instead of reading as ~0
(audit medium internal/agent/compaction.go:58) + test.
- ledger: re-verify against current main; mark scanner.go:35 and
redaction.go:362 already-fixed-on-main, record this batch's fixes.
* audit batch 2: worktrees defaultRunGit separates stdout/stderr
Capture git stdout and stderr into separate buffers instead of CombinedOutput,
so parsed rev-parse output is not polluted by git warnings and CommandResult.Stderr
carries the real error text (audit medium internal/worktrees/worktrees.go:247) + test.
* audit: re-verify all 87 pending findings against current main
Independent agent per file re-checked each open/partial finding against the
actual current code (the 06-11 ledger was verified on a since-diverged branch),
with an adversarial second pass on every already_fixed verdict. Authoritative
result: 78 genuinely pending (66 still_open + 12 partial); 9 rows were already
fixed on main and are reclassified. Full table in 2026-06-13-reverification.md.
* audit batch 3 (agent): cancellation identity, always-allow w/o sandbox, compaction usage, dedup
- loop.go: check ctx.Err() before collected.Error so a cancelled run returns
context.Canceled (errors.Is holds) instead of a plain stringified error.
- loop.go: 'Always allow' with no sandbox engine now allows THIS call instead of
denying it (persistence is skipped when there's nowhere to persist).
- loop.go: remove a duplicate schema["items"] assignment in propertyToRuntimeMap.
- compaction.go: forward OnUsage (token accounting) through the summarizer while
still omitting OnText, so compaction's token cost is counted in usage/budgets.
- tests for all three behavioral fixes (each fails pre-fix).
* audit batch 4 (cli): list-tools -o json, interrupted -o json terminal, surface session-record failures
- exec --list-tools -o json now emits a JSON tool listing instead of falling
through to the human-readable text (audit medium exec.go:182) + test.
- interrupted run with -o json now emits a terminal error+done on stdout instead
of only printing to stderr, so a JSON consumer sees a clean end (medium exec.go:500) + test.
- execSessionRecorder: surface a latched session-append failure to stderr once at
run end (deferred) instead of silently dropping it (medium exec_sessions.go:114/115) + test.
- left app.go trailing-arg ignore as-is: documented-intentional, TUI has no argv prompt.
* audit batch 5 (zerogit): status -z porcelain (renames + non-ASCII paths); document env-drop on plain Runner
- parseStatus: switch `git status` to `--porcelain -z` and parse the
NUL-delimited form (audit medium zerogit.go:212). Previously the default
--short output was split on newlines, so a rename `R old -> new` was kept as
one bogus path and a non-ASCII/whitespace filename arrived C-quoted+escaped
(e.g. `"caf\303\251.txt"`). The -z form never quotes paths and emits a
rename/copy as `XY <dest>\0<src>`, so we now record the destination verbatim
and consume the source field. + TestParseStatusZHandlesRenamesAndSpecialPaths.
- resolveRunners: document that the plain-Runner EnvRunner adapter intentionally
drops env (audit low zerogit.go:417). The branch is reached only when a caller
supplies a custom Runner without a RunGitEnv — in practice fake-Runner tests.
Production leaves both nil and gets defaultRunGit/defaultRunGitEnv, which honor
GIT_INDEX_FILE, so snapshot-diff isolation holds. Routing env calls to real git
instead would break the fake-Runner test pattern and fix nothing in production.
- update Inspect/Commit mock tests to the -z status format and the new command.
* audit batch 6 (config): reject negative maxTurns; drop dead contract-gap helpers
- FileConfig.UnmarshalJSON now rejects a negative maxTurns instead of letting the
`MaxTurns > 0` merge gates silently drop it and fall back to the default,
hiding the misconfiguration (audit medium resolver.go:136). 0 is left as the
"unset" sentinel (omitempty) and still falls back to the default; the CLI
--max-turns flag continues to reject 0 too since there it is distinguishable
from absent. + TestResolveRejectsNegativeMaxTurns / ...ZeroFallsBackToDefault.
- remove dead config/contracts.go (ContractGap, DefaultContractGaps,
FindContractGapsByMilestone, ContractOwnerRuntime) and its test — referenced
only by that test, never by production (audit low contracts.go:12).
- NOT changed: ResolveMCP intentionally does not run the provider command
(TestResolveMCPDoesNotRunProviderCommand guards this), so the "ZERO_PROVIDER_COMMAND
MCP servers dropped" row is by-design, not a defect.
* audit batch 7 (mcp): bound captured MCP server stderr
connectStdio attached a plain bytes.Buffer as cmd.Stderr that is only ever read
on initialize failure, yet it kept growing for the entire process lifetime as a
long-lived server logged to stderr (audit medium client.go:98). Replace it with
a concurrency-safe boundedBuffer capped at 64 KiB that keeps the head (enough for
init-error diagnostics) and discards the rest, reporting full write lengths so
os/exec never sees a short write. + TestBoundedBufferCapsRetainedBytes.
Left as-is (not defects worth the risk in the MCP client):
- client.go writer.write under client.mu: already designed to release the lock
before the unbounded response wait; the residual concern is a server that stops
draining stdin, an involved async rewrite for a theoretical deadlock.
- readLoop not answering server-initiated requests (ping): responding safely
needs re-locking around the shared writer; no configured server relies on it.
* audit batch 8 (tools/providers): read_file truncation marker; drop dead provider branch
- read_file: when a max_lines cut occurs, append an explicit "[truncated: N more
line(s)... set start_line=X to continue]" marker to the output. Previously only
the Result.Truncated flag was set, which is invisible in the rendered text, so
the model could not distinguish a cut read from a complete one (audit low
read_file.go:89). + TestReadFileToolMarksTruncation.
- providers/factory: simplify `if !explicitProvider || isImplicitOpenAI(...)` to
`if !explicitProvider` and delete the now-unused isImplicitOpenAI. The clause
was dead: explicitProvider==true implies ProviderKind or Provider is set, but
isImplicitOpenAI required both empty, so it could never contribute a case
(audit low factory.go:118). Behavior-preserving; providers tests still green.
* audit batch 9 (sessions): Tree builds from a single List() snapshot (no N+1 disk scans)
Store.Tree recursed via ListChildren per node, and each ListChildren ran a full
store.List() that re-read every metadata.json from disk — O(nodes * total-sessions)
reads for one tree (audit medium lineage.go:140). Snapshot all sessions once, index
children by parent in memory, and recurse over that. Child ordering is factored into
sortChildSessions so ListChildren and Tree stay identical; path-scoped cycle
detection (delete(seen,...)) is preserved. Existing tree/lineage tests still pass.
Left as-is: store.go timestamp() second precision (RFC3339) — switching to
RFC3339Nano would break lexical UpdatedAt ordering against existing second-precision
records ("...00.5Z" sorts before "...00Z"), so it is not a safe one-line change.
* audit batch 10 (specialist): serialize accounting dedup-then-append
recordSpecialistStop and appendSpecialistUsageRollup checked for an existing event
(specialistEventExists → ReadEvents) and then appended under SEPARATE store locks,
so two concurrent finishers — a foreground onExit racing a TaskOutput poll, or a
background reaper — could both pass the dedup check and write duplicate stop/usage
events (audit medium accounting.go:72). Guard each check-then-append pair with a
package-level accountingMu (Executor is used by value, so a struct field would not
be shared). + TestRecordSpecialistStopDedupesUnderConcurrency (passes under -race).
* audit batch 11 (zeroruntime/gemini): O(1) text accumulation; gemini cached-token counts
- zeroruntime CollectStreamWithOptions accumulated assistant text with
`collected.Text += event.Content`, reallocating the whole string on every chunk
(O(n^2) over a long streamed response). Accumulate in a strings.Builder on the
collector and materialize it once in flush, the single finalization point hit
before every return (audit low helpers.go:104). Existing collect tests still pass.
- gemini decoded usageMetadata without cachedContentTokenCount, so a cached-prompt
response reported zero cached input tokens while the other providers report them
(audit medium gemini/types.go:88). Decode the field and thread it through
streamState into NormalizeUsage as CachedInputTokens. + extended
TestStreamCompletionEmitsTextUsageAndReasoningTokens.
* audit batch 12 (specialist): parse child stream without a line-length cap; drop dead helper
- ParseStream used a bufio.Scanner capped at 1 MiB, so one large stream-json line
from a child (a big tool result or final answer) hit bufio.ErrTooLong and aborted
the entire specialist run (audit medium streamer.go:43). Switch to a bufio.Reader
with ReadString, which has no per-line limit; the child is our own trusted
subprocess, so its output length is the legitimate bound. Line numbering and
blank-line/EOF handling are preserved. + TestParseStreamHandlesLineLargerThan1MiB.
- remove dead formatTaskOutput (output_tool.go:244): zero callers and zero tests;
it only forwarded to formatTaskOutputSummary, which is the one actually used.
* audit batch 13 (background/specialist): kill background tasks as a process group
Background children were launched in the parent's process group and terminated by
signalling only the single PID, so any process the task forked was orphaned and
outlived a cancel; the signal also risked hitting a recycled PID after the child
was reaped (audit medium process_posix.go:38/50).
- ConfigureChildProcessGroup (Setpgid on POSIX, no-op on Windows where taskkill /T
already tree-kills) is applied at launch so the child leads its own group.
- terminateProcess now SIGTERM/SIGKILLs the negative PID (the whole group), with a
pid>1 guard so a bogus 0/1 can never expand to "our own group" or "everything",
and treats ESRCH as already-gone. Liveness polls the group. Targeting a group
(rarely recycled vs. a bare PID) also shrinks the reaped-then-recycled window.
- specialist launchBackgroundProcess opts every child into its own group; on a
SetPID failure it now kills the child + group, marks the task errored, and records
the stop (deduped) instead of leaking an untracked orphan.
- tests launch as group leaders; new TestTerminateProcessKillsForkedChildren proves
a forked grandchild dies with the group.
* audit batch 14 (sandbox): quote-aware shell segment splitting
splitShellSegments used a quote-unaware strings.Replacer, so a shell operator
inside quotes was treated as a separator: `git commit -m "use top | less"` split
into a `less` segment and `echo "a; vim b"` into a `vim` segment, both falsely
flagged as interactive (audit medium safe_command.go:138).
Replace it with a quote-aware scanner that mirrors the shell: single quotes make
everything literal; double quotes keep $(...)/`...` substitution active but treat
|, ;, && as literal; unquoted text splits on the same operator set as before
(;, |, ||, &&) plus substitution boundaries. Real unquoted operators still split,
so an interactive program behind a separator — or inside a live `"$(...)"`
substitution — is still caught (no new false negatives, the security-relevant
direction). + TestDetectInteractiveQuoteAwareSeparators covering both directions.
* audit batch 15 (cron): collapse DST fall-back repeated hour to one fire
On a fall-back day the clock repeats an hour (e.g. 01:30 EDT then 01:30 EST), so
Next("30 1 * * *", <01:30 EDT>) returned the same-day 01:30 EST — firing a daily
job twice (audit medium schedule.go:171, which previously guarded only the
spring-forward gap).
Next now skips a match whose local wall-clock minute equals `after`'s but whose
absolute time is later (the repeated-hour duplicate) and keeps searching. Normal
forward search always advances the wall-clock, so this only triggers on the
fall-back repeat; when the scheduler passes the last fire time as `after`, the
repeated hour collapses to a single fire. + TestNextDSTFallBackDoesNotRepeatHour
(spring-forward termination test still passes).
Note: Next is stateless, so it dedupes relative to `after`; a caller that passes
an arbitrary mid-hour "now" instead of the last fire time is out of scope here.
* audit batch 16 (mcp): skip SSE notifications before the response; larger SSE buffer
- decodeSSERPCMessage returned the first non-empty `message` event unconditionally,
so a server-initiated notification/request arriving before the response made the
caller see an id mismatch and fail (audit medium network_client.go:621). It now
skips any message carrying a method (notifications/requests have one; a response
does not) and returns the first real response; the caller's rpcIDMatches still
validates the id. + TestDecodeSSERPCMessageSkipsNotifications.
- raise the per-event SSE scanner cap from 1 MiB to a bounded 8 MiB so a large but
legitimate MCP message no longer hits bufio.ErrTooLong and fails the request
(audit medium network_client.go:637); still bounded against an unbounded server.
Out of scope (kept): full reconnect of a persistent SSE stream after a fatal read
error — a larger change than this remediation.
* audit batch 17 (sessions): fsync session metadata before the atomic rename
writeMetadata wrote the temp file with os.WriteFile (no sync) and renamed it into
place, so a crash right after the rename could expose a metadata file whose bytes
were never flushed — a torn or empty file that corrupts the whole session (audit
medium store.go:632). Write+fsync the temp via a new writeFileSync helper before
renaming, closing that window. + TestWriteFileSyncRoundTrips.
Deliberately scoped:
- Event-log appends (appendEventLocked) are NOT fsynced per append: that would add
a disk flush to every event (every tool call), and a hard-crash loss of the log's
unsynced tail is acceptable, whereas torn METADATA is not. Metadata is the
integrity-critical part and is what this syncs.
- Directory fsync is omitted for cross-platform simplicity (it errors on Windows);
the file fsync already prevents the torn-contents failure, the serious case.
Left for follow-up (in this bucket but not safe/clear wins here):
- hooks append sequence still re-reads the file per append; the in-memory cache is
cross-process-unsafe and the safe last-line read is fiddly for a low-volume log.
- Fork event re-append/de-dup is nuanced usage-accounting and needs its own change.
* audit batch 18 (agent): count tool-defs in compaction estimate; don't deny an always-allow on persist failure
- maybeCompact's estimate omitted the tool definitions, which ride on every
request, so the real input could exceed the model limit while the message-only
estimate stayed under threshold and compaction never fired (audit medium
compaction.go:58). partitionTools is now computed before maybeCompact (it does
not depend on the messages) and the exposed tools' estimated tokens are added to
both the threshold check and the post-compaction shrink check. + estimateToolDefTokens
and TestEstimateToolDefTokensCountsDefinitions.
- PermissionDecisionAlwaysAllow denied the call when persistPermissionGrant failed,
overriding what the user explicitly allowed (audit medium loop.go:468). Persisting
is now best-effort: a failure (or no sandbox engine) just means the grant is not
remembered and the user is re-prompted next time, not denied now. This also drops
the dead requestEvent.GrantMatched/Grant writes (audit low loop.go:473) — the
emitted event derives them from the sandbox decision via buildPermissionEvent.
Left intentional (documented, not defects): the reactive-retry path omits OnText to
avoid duplicating mid-stream text (loop.go:183); OnContext/MeasureContext is a
complete, tested context-breakdown scaffold awaiting a consumer (types.go:183).
* audit batch 19 (usage): price escalated usage from the event's own model
exec persists payload["model"] on --allow-escalation runs (the model can change
mid-run), but BuildReport priced every event from the session's Metadata.ModelID
and never read the payload, so usage from an escalated model was mis-priced at the
base model (audit medium report.go:17). usageEventPayload now carries Model and
BuildReport prefers it, falling back to the session model when absent (older
events stay byte-identical and behave as before). + TestBuildReportPricesFromEventModelWhenPresent.
* audit batch 20 (sessions): don't copy usage events into a fork (no double-count)
Fork re-appended every parent event, including provider_usage, into the child, so
a usage report aggregating the parent and the fork double-counted the parent's
usage (audit medium store.go:387). Skip EventUsage when copying — it already
counted against the parent and is not part of the conversation history a fork
replays — and record the actual copied count in the fork marker. + TestStoreForkSkipsUsageEvents.
* audit batch 21 (cli/skills): warn on duplicate skill names instead of silently dropping
skills.Load deduplicates same-named skills (first directory wins) and records the
dropped collisions via skills.Duplicates, but nothing surfaced them, so a shadowed
skill just vanished with no signal — and Duplicates had no production caller (audit
low skills.go:91). `skills list` now reports each collision to stderr (keeping
stdout/--json clean), using Duplicates. + TestRunSkillsListWarnsOnDuplicateNames.
Note: skills.Get is kept — it is a by-name accessor used throughout the package's
tests, not dead code.
* audit batch 22 (cli/cron): capture the run error from stream-json stdout
Cron jobs run exec with --output-format stream-json, so a failure is reported as an
`error` event on STDOUT, but the run record's Error was set only from stderr — so a
failed job often recorded an empty/uninformative error while the real message sat
unread in outBuf (audit low cron_run.go:150). On a non-zero exit, scan the stdout
stream-json for the error event's message and prefer it, falling back to stderr.
+ extractStreamJSONError and TestExtractStreamJSONError.
* audit batch 23 (sandbox): richer macOS seatbelt profile (signal + mach-lookup)
The generated sandbox-exec profile was default-deny with only process/sysctl/file
allowances, so two real gaps remained on macOS (sandbox quality report H1):
- A sandboxed script could not signal the children it spawned — `kill` returned
"Operation not permitted" under seatbelt, breaking common `cmd & ... kill`,
test-runner, and timeout patterns. Add `(allow signal (target self) (target pgrp))`,
which stays scoped to the command's own process group.
- XPC to system daemons was denied, so keychain (securityd/trustd), user/group
lookup (opendirectoryd), preferences (cfprefsd), network config
(SystemConfiguration), launch services, and the pasteboard all failed. Add a
curated `(allow mach-lookup ...)` allowlist (`sandboxMachServices`).
Neither touches the file-write or network rules, so the workspace boundary is
unchanged — verified on a real macOS host: `security`/`scutil`/`pbpaste`/self-kill
now succeed while writes to $HOME and /etc are still denied. iokit-open was tried
and dropped as unnecessary. + unit assertions (TestSandboxExecProfileGrantsSignalAndMachLookup)
and extended the darwin integration test with a self-kill case and an
outside-workspace deny check.
* audit batch 24 (sandbox/tools): remove dead OnSandboxDecision hook
RunOptions.OnSandboxDecision had no setter anywhere, yet registry.RunWithOptions
spawned a goroutine (with panic recovery) for it on every sandboxed tool call —
pure per-call overhead for an unreachable callback (sandbox report M1). The sandbox
decision is already surfaced to observers via the permission event
(buildPermissionEvent reads the decision), so this hook is redundant; delete the
field and its call site.
* audit batch 25 (sandbox/cli): exact-path grants — create and revoke by path
The engine already matched ScopeFile grants and DeriveScope already produced them
from path/file tool args, but the CLI could neither create one explicitly nor
revoke a single one (sandbox report L3 / prompt 7). Add:
- GrantStore.RevokePath(tool, path): revokes only the grant whose scope matches the
given file/dir, leaving tool-wide and other-path grants intact (path canonicalized
to absolute like stored scopes). + TestGrantStoreRevokePathRemovesOnlyMatchingScope.
- `zero sandbox grants allow <tool> --path <p>`: persists an exact-file (ScopeFile)
grant, path resolved to absolute so it matches lookups and revokes.
- `zero sandbox grants revoke <tool> --path <p>`: revokes just that grant (default
still revokes all grants for the tool). + TestRunSandboxGrantsCreateAndRevokeByPath.
The TUI permission-card "Allow always (exact path)" option is intentionally left for
the TUI feature branches (it overlaps #187/#188); the engine + CLI now cover the
full create/match/revoke round-trip for file-scoped grants.
* audit batch 26 (sandbox): Engine.Precheck — report violations before execution
Add Engine.Precheck(ctx, request) []Violation, which reports the sandbox
violations that would block a tool request before it runs (sandbox report / prompt 6).
It reuses Evaluate so policy is not duplicated: an allowed or merely-prompted
request yields no violations; a denied request yields its violation (via the shared
violationsFromDecision helper, with a ViolationPolicyDenied fallback for a deny that
carries no structured violation). A nil engine yields nothing.
+ TestEnginePrecheckReportsViolationsBeforeExecution.
Wiring Precheck into the batch-confirmation prompt is intentionally left to the TUI
branch: it touches prompt rendering, and short-circuiting the agent loop's deny path
would skip the before/after-tool hooks that currently fire around a policy-denied
call. The engine API is in place for that consumer.
* audit batch 27 (sandbox): scoped-egress domain prompt with timeout + caching
The scoped-egress proxy failed closed on any host outside the allowlist. Add an
opt-in DomainPrompt callback (prompt 8): an UNKNOWN host (neither allowed nor
explicitly denied) is authorized at request time via the callback instead of being
refused outright. An explicitly denied host is still refused without prompting; the
callback is bounded by PromptTimeout (default 60s, timeout/error => deny); decisions
are cached for the proxy's lifetime so a host is prompted at most once. With no
callback set the strict fail-closed behavior is unchanged. Wired through both the
CONNECT and HTTP handlers via authorizeTarget.
+ TestEgressProxyAuthorizeDomainPrompt / ...TimeoutDenies / ...NilPromptFailsClosed.
The user-facing prompt that supplies the callback is left to the permission UI (TUI)
branch; the egress mechanism (callback + timeout + cache) is in place and tested.
* audit batch 28 (sandbox): AST-based shell analysis (AnalyzeCommand)
Add AnalyzeCommand(script) AnalysisResult, a static shell analyzer built on the
mvdan.cc/sh/v3 parser (prompt 12). Walking the parsed command tree makes it a more
precise second opinion than the regex detector: a program name only counts when it
is an actual command, so `echo "vim ..."`, `printf 'open with vim'`, and
`git commit -m "rm -rf /"` are clean, while real commands are flagged —
interactive (editors/pagers + REPLs, with the same -e/script suppression as
nonInteractiveREPLFlags), destructive (rm -rf/-fr, dd, mkfs, shred, ...), and
network (curl/wget/ssh/nc/...). An unparseable script yields TooComplex so callers
can treat it as higher-risk. + TestAnalyzeCommand (16 cases) / ...EmptyIsClean.
Dependency: mvdan.cc/sh/v3 pinned to v3.11.0 — compatible with the existing go 1.24
directive (the latest v3.13.1 would have bumped the module to go 1.25). It is kept
as a standalone analyzer for callers to consult; feeding it into Engine.Evaluate is
deliberately not done here, since that would change allow/deny classification.
* audit batch 29 (sandbox): unify proxy-env generation (ProxyEnv)
Export ProxyEnv as the single source of truth for proxy-env injection (prompt 9
req 1), replacing the unexported scopedProxyEnv used by the sandboxed-shell path.
+ TestProxyEnv.
Investigation that bounds the rest of prompt 9 (documented in the ProxyEnv doc):
- web_fetch already honors HTTP_PROXY/HTTPS_PROXY — its transport clones
http.DefaultTransport, whose Proxy is http.ProxyFromEnvironment.
- MCP children already inherit the parent env (mergeProcessEnv appends os.Environ),
so they pick up any proxy vars the agent has.
So the only missing link to scope FetchUrl/MCP is a SESSION-level egress proxy whose
address reaches the agent process. That is deliberately NOT wired here: doing it
safely requires allowlisting the active LLM provider's own domain first, or the
agent's provider calls would route through the scoped proxy and be denied — a
correctness pitfall that also can't be verified end-to-end on this host. Flagged
rather than shipped blind.
* audit batch 30 (sandbox): Linux seccomp Unix-socket block — UNVERIFIED, needs Linux CI
Adds a pure-Go (no cgo, no vendored binary) seccomp BPF filter that denies
socket(AF_UNIX) with EPERM on x86-64/arm64, closing the Unix-socket gap bubblewrap's
filesystem/network isolation leaves open (prompt 11):
- internal/sandbox/seccomp.go: the classic-BPF program as a platform-neutral
[]sockFilter, built and UNIT-TESTED (structure + every jump offset in range — the
classic way a hand-written BPF filter goes silently wrong).
- seccomp_linux.go: ApplyUnixSocketBlock installs it via prctl(NO_NEW_PRIVS) +
prctl(SET_SECCOMP, MODE_FILTER). seccomp_other.go is a no-op everywhere else.
- cmd/zero-seccomp: a tiny linux-only exec wrapper that applies the filter then
execs the real command (the standard way to install seccomp in a child with
os/exec); degrades gracefully (warns + runs) if the kernel lacks seccomp.
⚠️ HONEST CAVEAT: I developed this on macOS and CANNOT run the actual AF_UNIX block
here. darwin build, `GOOS=linux` cross-compile, vet, and the structure test all pass,
but the unit test verifies the program's SHAPE, not that the kernel actually blocks
AF_UNIX — that MUST be verified on Linux CI (create a Unix socket under the wrapper
and assert EPERM). Safe to ship as-is: it is linux-only, no-op on every other
platform, and not wired into the bwrap command yet (opt-in prefixing the sandboxed
command with zero-seccomp is the remaining integration), so it cannot regress any
current path.
* audit batch 31 (sandbox): macOS denial log monitor (opt-in, default off)
Re-adds the macOS seatbelt-denial monitor as an opt-in feature (prompt 10). When
Policy.MonitorDenials is set, the sandbox-exec profile tags its default-deny with a
marker and bash tails `log stream` for that tag during the command, appending any
captured denials as a <sandbox_violations> block to the command's stderr so the
model can see what was blocked.
- DenialMonitor (darwin) tails/parses tagged denials with a capped, deduped ring
buffer and noise filtering; no-op stub on other platforms. parseSandboxDenyLine is
unit-tested. + profile-tag test + appendSandboxViolations test.
- Default OFF: with MonitorTag empty, StartDenialMonitor is a no-op and the command
path is byte-for-byte unchanged, so there is no regression or per-command overhead
unless enabled.
CAVEAT: on at least one macOS host (verified live) seatbelt denials are NOT delivered
to the `log stream`-queryable unified log, so the monitor captures nothing there. It
is the standard mechanism and works on macOS versions that do log denials; shipping
it default-off means it never affects whether a command runs.
* audit batch 32 (sandbox): wire seccomp Unix-socket block into bubblewrap (opt-in)
Completes prompt 11: the zero-seccomp helper (which installs the AF_UNIX seccomp
filter) is now actually invoked by the sandbox instead of only existing as a
standalone binary.
- bubblewrapCommandPlan, when Policy.BlockUnixSockets is set, ro-binds the
discovered zero-seccomp helper into the sandbox at its host path and prefixes the
command with it so the filter is installed before the real argv runs.
- findSeccompHelper discovers the helper next to the running executable first, then
on PATH; it returns "" when absent, so the sandbox degrades gracefully (runs
without the extra filter — bubblewrap's fs/net isolation still applies) rather
than failing the command. Exposed as a package var so the wiring is unit-tested
on any OS.
- Reachable from config: SandboxConfig.BlockUnixSockets and .MonitorDenials now map
onto the policy in applyConfiguredSandboxPolicy (opt-in only — config can turn a
hardening flag on, never off). Both default OFF.
- Tests: helper-prefix present when enabled / absent by default; config flags apply
independently and stay off when omitted.
Default OFF and Linux-only effect; the runtime seccomp enforcement still needs a
Linux host/CI to exercise end-to-end (the wiring and BPF program are unit-tested).
* audit batch 33 (sandbox): enforce scoped/deny network policy in web_fetch & web_search
Completes prompt 9: the built-in network tools now honor the SAME allow/deny/scoped
policy the sandboxed shell egress proxy enforces, instead of bypassing it. Before
this, web_fetch/web_search would reach any public host even when the policy was
NetworkDeny or NetworkScoped — the egress proxy only constrained shell commands.
- New shared gate Engine.NetworkHostAllowed(host) reuses the egress domain matcher
(domainAllowed/normalizeDomains/effectiveNetwork) as the single source of truth:
allow → all hosts; deny → none; scoped → only AllowedDomains minus DeniedDomains
(empty allowlist collapses to deny). Disabled policy / nil engine impose nothing.
- web_fetch is now sandbox-aware: the host is checked before the request AND on every
redirect, so a denied/unlisted host fails closed before any dial. Plumbed via a
networkPolicyGate so the no-sandbox path is byte-for-byte unchanged.
- web_search is sandbox-aware: the configured backend's endpoint host is checked
before the query leaves the machine; blocked under deny, and under scoped unless the
backend host is allowlisted.
- The registry already routes sandbox-aware tools through RunWithSandbox when an
engine is present, so this activates automatically wherever the sandbox is wired.
- Tests: engine gate matrix (deny/allow/scoped/denylist/empty/disabled/nil + host:port);
web_fetch & web_search block denied/unlisted hosts without dialing and allow listed
ones; endpoint-host parsing.
* audit batch 34 (sandbox): prove network-policy wiring end-to-end + document opt-in flags
Verification follow-up to batches 31-33; no behaviour change.
- Adds a registry-level test that exercises the FULL chain for the scoped-network
enforcement: registry.RunWithOptions -> engine.Evaluate (passes a scoped,
egress-capable sandbox-exec backend) -> routes web_fetch through RunWithSandbox ->
per-host allowlist blocks an unlisted host before any dial. This proves the
Tool-interface value actually satisfies sandboxAwareTool at the registry boundary,
not just in a direct method call.
- Documents the two opt-in hardening flags (sandbox.blockUnixSockets,
sandbox.monitorDenials) and the scoped web_fetch/web_search behaviour in the README
so the config keys are discoverable. Both remain off by default.
Confirmed wiring: agent loop forwards options.Sandbox to the registry; the registry
routes sandbox-aware tools (web_fetch, web_search) through RunWithSandbox when an
engine is present; MonitorTag is set only on the macOS sandbox-exec plan; the
seccomp helper prefix is added only on the bubblewrap plan when BlockUnixSockets is
set; both config flags map onto the policy via applyConfiguredSandboxPolicy.
* audit batch 35 (sessions): fix Windows CI failure in TestWriteFileSyncRoundTrips
The exact-permission assertion (perm == 0o600) added in batch 17 fails on
windows-latest because Windows does not honor Unix permission bits: Go reports
0o666 for a writable file regardless of the mode passed to OpenFile. This was the
sole cause of the failing Smoke (windows-latest) CI job; macOS and Ubuntu passed.
Guard the perm check with runtime.GOOS != "windows" so it still verifies the mode
on the platforms that enforce it, while the round-trip (write + read-back) is still
asserted on every platform.
* audit batch 36: address CodeRabbit review (5 findings)
- background/process_posix: after SIGKILL, poll until the process group is
actually gone (SIGKILL is async) and return an error if it survives past a
second deadline, instead of returning nil while descendants still race.
- sandbox/safe_command: only treat ')' as a substitution boundary when it closes
an active $(...). A literal ')' (e.g. echo "a) less") no longer splits into a
fake `less` segment and is no longer misflagged as interactive. Depth-tracked so
nested $(...) still isolate their inner command. Direct splitShellSegments test
(fails pre-fix: yields ["echo \"a", "less\""]).
- cron/schedule: the DST fall-back collapse now only applies when `after` sits
exactly on the minute boundary (its "last fire time" form). With sub-minute
precision (e.g. 01:30:30 EDT) the first 01:30 fire already passed, so the repeated
01:30 EST is the legitimate next fire and is returned — restoring strictly-after
semantics. New test fails pre-fix (skipped to next day).
- config/types: maxTurns error message now reads "must be >= 0" to match the check
(0 is accepted; only < 0 is rejected). Negative-maxTurns test asserts the message.
- docs/audit: header date/branch metadata now matches the body (2026-06-13,
fix/audit-batch) instead of the stale 2026-06-11/fix/audit-critical-high snapshot.
Verified: gofmt clean; go vet clean; host + linux + windows builds; affected suites
green. Each behavioural fix ships with a test confirmed to fail without the change.
* audit batch 37: address CodeRabbit re-review (12 findings)
Findings on the audit code from batches 21-36; each behavioural fix ships with a
regression test (several confirmed to fail without the change).
sandbox/analyzer (AnalyzeCommand):
- Unwrap launcher prefixes (sudo/env/nice/bash -c/...) and recurse into `sh -c`
payloads so `sudo rm -rf`, `env curl …`, `bash -c 'vim x'` classify on the real
command, not the launcher. Mirrors DetectInteractiveCommand.
- Honor rm's `--` end-of-options marker so `rm -- -rf` (delete a file named -rf) is
not flagged destructive.
sandbox/engine: NetworkHostAllowed now shares one backend-aware effective-mode
helper with Evaluate, so scoped collapses to deny when the backend can't enforce
scoped egress — the per-tool gate can no longer permit traffic the engine-level
decision would fail closed.
sandbox/egress: cache domain-prompt decisions by host:port, not host, so one
"allow" can't authorize other ports on the same host.
sandbox/safe_command: the segment splitter is now escape-aware (\| , \" ) so an
escaped operator/quote can't manufacture a fake interactive segment.
sandbox/log_monitor (darwin): Wait() the `log stream` child after draining stdout
so the cancelled process isn't leaked as a zombie.
tools/web_search: confine the search backend to same-host redirects (with a limit)
so a redirect chain can't egress to a host the scoped/deny policy never authorized.
cli/sandbox: an explicit empty --path (`--path=` / `--path ""`) now fails closed
instead of silently widening an allow to tool-wide or a revoke to all-grants.
cli/skills: warn on stderr when duplicate-skill detection fails instead of
swallowing the error.
specialist/exec: don't mask a failed orphan-kill — join the TerminateProcess error
with the SetPID error so a surviving process is visible.
background/process_posix: resolve the PGID and only group-signal when pid leads its
own group; otherwise signal the individual PID. Avoids both a false-success on a
non-leader ESRCH and the hazard of signalling an unrelated (possibly our own) group.
Verified: gofmt + go vet clean; host + linux + windows builds; full go test ./...
green.
* audit batch 38: address CodeRabbit re-review round 2 (3 findings)
Follow-ups to batch 37, each with a regression test:
- safe_command (splitter): a ')' is now only a substitution boundary when it
closes an active, UNQUOTED $(...). A quote-state stack saved at each '$(' lets a
substitution run in a fresh quoting context and restores the outer quoting on
its matching ')'. Fixes `echo $(printf "a) less")` splitting into a fake `less`
segment, while still isolating the inner command of `echo "$(vim x)"`.
- egress: deduplicate in-flight domain prompts per host:port. Concurrent requests
for the same unknown target now wait on the first prompt and reuse its cached
decision instead of each firing a prompt (a prompt storm) and racing to write
the final decision. Race-tested with 25 concurrent authorizers → one prompt.
- analyzer + safe_command: wrapper option-value consumption is now wrapper-specific
(wrapperValueOptionsByProg) instead of a global option set. A valueless flag like
`sudo -n` no longer swallows the real payload, so `sudo -n rm -rf` /
`sudo -n curl …` are correctly classified destructive/network; `sudo -u root vim`
still consumes its value. Shared by the AST analyzer and the regex detector so
they can't diverge.
Verified: gofmt + go vet clean; host + linux + windows builds; go test ./... and
-race on sandbox/background all green.
* audit batch 39: address CodeRabbit re-review round 3 (12 findings)
All verified against current code; each behavioural fix has a regression test.
Security / correctness:
- tools/web_search: redirect policy now also refuses a scheme change, blocking a
same-host https→http downgrade that would send the query + bearer token over
plaintext.
- sandbox/safe_command: a ')' is only a substitution boundary when it closes an
active UNQUOTED $(...), and backticks now get the same quote-state save/restore
as $( — so `echo "`a | less`"` and `echo "$(true | less)"` no longer hide the
inner pager from interactive detection.
- sandbox/analyzer + safe_command: wrapper option-value consumption stays
wrapper-specific, and unwrapping no longer aborts on a dynamic ($x) wrapper arg
once a wrapper is active, so `env "$opts" curl` / `sudo "$x" rm -rf` are still
classified.
- sandbox/egress: in-flight prompts deduped per host:port (carried from batch 38);
the prompt callback is now cancellable (context cancelled on timeout) so a stuck
prompt can abort instead of leaking; signature threaded through.
- sandbox/runner: per-plan unique denial tag (pid+counter) so concurrent monitored
sandbox-exec runs can't ingest each other's denials; helper selection now
requires an executable regular file so a non-exec sibling degrades gracefully.
- mcp/network_client: aggregate SSE event-size cap (not just per-line) so a server
can't grow memory with many unterminated data: lines.
- specialist/accounting: a previously-recorded stop/usage with an empty runId is a
catch-all and now suppresses a later runId-bearing duplicate.
- sessions/lineage: Tree fetches the root via Get() first so a corrupt root
surfaces its real error instead of degrading to "not found".
Docs/tests:
- background/terminate: docstring now states the leader-only group-kill (PID-only
fallback for non-leaders); test child-liveness probe uses ESRCH specifically.
- cli/sandbox: grant usage strings include --path.
Verified: gofmt + go vet clean; host + linux + windows builds; go test ./... and
-race on sandbox/background all green.
* audit batch 40: address CodeRabbit re-review round 4 (8 fixed, 1 skipped)
Verified each against current code.
Fixed:
- sessions/lineage: PANIC FIX — store.Get returns (nil, nil) for a missing
session, so the batch-39 root prefetch could nil-deref. Guard root == nil and
return a not-found error. Regression test added (was a panic).
- agent/compaction: the reactive recover() now records lowWaterMark in the SAME
combined (messages + tool-defs) token domain maybeCompact uses, so the proactive
shrink-guard compares like with like. recover() takes tools; callers pass
request.Tools.
- mcp/network_client: SSE aggregate counter no longer counts a joining newline for
the first data line (only when prior content exists), avoiding premature rejects.
- sessions/store: fsync the parent directory after the metadata rename so the
rename is durable across a crash (no-op on Windows, which can't fsync a dir).
- sandbox/egress: parse the target port with strconv.Atoi instead of
net.LookupPort (no context-unaware resolver use).
- sandbox/runner: escape the per-plan denial tag through sandboxProfileString when
embedding it in the seatbelt profile.
- sandbox/seccomp_linux: guard against an empty filter program before taking
&kernelFilters[0] (defensive; the program is fixed non-empty today).
- cli/cron_run_test: assert last-error-wins with two error events.
- background/terminate, cli/sandbox usage, process_posix_test ESRCH (carried).
Skipped (with reason):
- zerogit worktree-rename (Y='R') test: git porcelain v1 -z only emits the
rename-source field when the INDEX column is R/C; the worktree column never
carries 'R' with a source field in v1, so the fixture would test output git
doesn't produce, and making parseStatus consume on code[1]=='R' would risk
mis-consuming the following entry for a plain ` R` status. Left as-is.
Verified: gofmt + go vet clean; host + linux + windows builds; go test ./... and
-race on sandbox/background all green.
* audit batch 41: address CodeRabbit re-review round 5 (4 findings)
All verified against current code; code fixes have regression tests.
- agent/compaction: summarizeWithFallback's reduce pass no longer swallows ALL
errors. A context-limit error still falls back to the joined partial summaries
(documented "extreme" case), but a non-context failure (auth/network/provider)
now surfaces unchanged, per the function's contract. Two tests (propagate vs.
joined-fallback); the propagate test fails without the fix.
- sandbox/safe_command + analyzer: wrapper value-consuming options now include the
long spellings (sudo --user root, env --unset NAME, timeout --signal …), so a
long flag's value is no longer mis-read as the program (interactive/destructive/
network detection bypass). A `--flag=value` token carries its own value and never
consumes the next token. Tests for space and = forms in both the AST analyzer and
the regex detector.
- docs/audit: the status-ledger summary table's open/partial cells (64/14) were the
lone outlier — the ledger's own narrative and 2026-06-13-reverification.md both
state 66/12. Corrected to 66/12 (66+12+97 = 175); derivation: body 69 open + 18
partial minus the 9 already-fixed-on-main (3 open + 6 partial) = 66/12 = 78
pending, fixed 88+9 = 97. Added a comment documenting the summary-vs-body
relationship. The reverification doc already states 66/12, so both now agree.
Verified: gofmt + go vet clean; host + linux + windows builds; full go test ./...
green.
* audit batch 42: address CodeRabbit re-review round 6 (1 fixed, 1 kept w/ proof)
CodeRabbit re-surfaced two "duplicate" refinements; CI was already green (its 6th
pass was a COMMENT with no new findings).
- cli/sandbox_test (TestRunSandboxGrantsRejectsEmptyPath): now seeds a tool-wide
grant and asserts grant-store immutability after EACH rejected empty-`--path`
call (reflect.DeepEqual before/after), so a mutation in one call can't be masked
by a later one — and a buggy "revoke all" is actually observable (it's a no-op on
an empty store). Stronger than the suggested patch.
- zerogit.parseStatus: KEPT consuming the rename source on the index column only
(code[0]), with an explanatory comment. Verified empirically that git porcelain
v1 -z reports a rename/copy (and emits the extra source field) only in the index
column: `git mv` → "R new\0old\0"; a worktree-only rename → " D old\0?? new\0"
(delete + untracked, never "R" in code[1]). Consuming on code[1]=='R'/'C' would
match no real git output and would only risk mis-consuming the next entry on
malformed input, so the suggestion is declined as incorrect for v1.
Verified: gofmt + go vet clean; host + linux + windows builds; affected suites green.
|
||
|
gnanam1990
|
0c37093974 |
audit: address PR review findings (security, correctness, tests)
Fixes from the code review on the critical/high audit branch: - tools/apply_patch: parseHunkCounts now reads only the range section between the opening and closing "@@". A crafted section heading like "@@ -1,1 +1,1 @@ +1,999999" could otherwise overwrite the line count, wedge the parser in hunk mode, and swallow later file headers so they escaped validatePatchPaths / recheckPatchWriteTargets — a workspace-confinement bypass. - providerhealth: the SSRF-hardened dial now tries every validated address in order instead of pinning addrs[0], so a dual-stack/multi-record provider isn't failed by a single dead record. Extracted dialValidatedAddrs for coverage. - hooks/dispatch: a beforeTool hook killed by its deadline now fails CLOSED (vetoes the tool) instead of fail-open. A launch failure (missing binary) still fails open so a misconfigured hook can't wedge every tool call. - cron: Store.Get returns a distinct ErrJobNotFound sentinel; cron_run only treats that as "removed during run" and still records/persists on a transient read error instead of silently dropping the run. - providers/anthropic: closeOpen finalizes thinking buffers still open at stream end (SSE ended after thinking_delta/signature_delta but before content_block_stop), so they survive in the done event's ReasoningBlocks for replay. Finalized in index order. - providers/gemini: corrected the stale thinkingConfig comment (no IncludeThoughts field; thought summaries are suppressed by skipping part.Thought parts). Tests: regression coverage for each fix, plus the requested nitpicks — EOF-terminated MCP line, recipe-prompt propagation assertion, checked file.Close() in session store tests, and a shortened bash process-group timeout test via the bashWaitDelay hook. Note: declined the suggestion to replay a "signature" on redacted_thinking blocks — Anthropic's redacted_thinking carries only an opaque "data" field; the signature is for regular thinking blocks, and the stream never populates one for redacted blocks. |
||
|
KRATOS
|
0f8ea50101 |
zero cron: dep-free file-backed scheduled agents (foreground runner) (#155)
* cron: dep-free 5-field cron expression parser * cron: next-run evaluator with Vixie DOM/DOW semantics * cron: preset recipes + loop.md prompt resolution (cap + symlink reject) * cron: file-backed job store (metadata.json + runs.jsonl) * cli: zero cron add/list/rm/pause/resume over the cron store * cli: zero cron run foreground scheduler (--once/--catch-up, exec per fire) * cron: fix adversarial-review findings (DST loop, traversal, runaway re-fire, +) - Next: fix infinite loop on DST spring-forward gaps (advance hours by absolute addition + a forward-progress guard); widen the search window to 9 years so a Feb-29 schedule across a century non-leap year (2096->2104) isn't reported as impossible. - store: reject path-traversal job ids (Get/Update/Remove/AppendRun) so 'cron rm ../..' can't delete outside the store; List now surfaces corrupt jobs as a warning instead of silently dropping them. - runner: reject impossible schedules at add even with --run-now; auto-pause a job whose schedule can no longer advance (was re-firing every tick); only skip-reschedule STRICTLY-overdue jobs on startup (keep exactly-due ones); capture stderr into RunRecord.Error on non-zero exit; pass the prompt via the inline --prompt= form so dash-leading prompts aren't misparsed. - Regression tests for each (DST, leap gap, traversal, corrupt-list, pause, reconcile, dash-prompt). * cron: address #155 review (store-error surfacing, resume guard, Runs traversal, +) - fireJob/reconcile/cronRun now surface AppendRun/Update failures to stderr instead of discarding them (a failed write no longer silently re-fires/loses history); fireJob takes stderr. - cron add rejects extra positional arguments instead of silently ignoring them. - cronResume rejects an unparseable/impossible schedule instead of reactivating a job with a stale/zero NextRunAt. - store.AppendRun returns the Close() error (buffered-write failures surface on close); store.Runs now validates the id (path-traversal guard, matching the other store methods). - tests: Runs traversal rejection, cron add extra-args, resume-impossible. * cron: check store errors in cron CLI tests CodeRabbit (critical): the cron CLI tests ignored errors from store.List/Add/Get, so a store failure would surface as a misleading assertion on a zero-value job/slice instead of a clear failure. Check every store op's error across the test file (not only the two flagged sites). * cron: DefaultRoot falls back to os.UserHomeDir when HOME is unset Vasanth (P1): cron's DefaultRoot mirrored neither sessions.DefaultRoot nor reality — with no XDG_DATA_HOME and no HOME it produced a RELATIVE ".local/share/zero/cron" under the caller's cwd, so on Windows/restricted shells cron jobs scattered per working directory and could write repo-local data. Now falls back to os.UserHomeDir() like sessions.DefaultRoot. Adds TestDefaultRootEmptyHomeFallsBackToUserHome (asserts the path stays absolute). |