Features-completeness review: fix 11 HIGH shipped-but-inert defects, then reconcile all docs#101
Merged
Merged
Conversation
An 11-agent adversarial audit of the whole tree found 11 HIGH defects (all independently verified, four reproduced with runnable proofs), ~48 MED and ~70 LOW. This fixes all of them. Every fix carries a discriminating test: one that fails on the pre-fix code and passes on the fix. The verifier is the only authority on "done" (I2), and it was compromised four independent ways: - code.test_passes built `go test -- '<pkg>'`, which go test reads as test-binary args for the CURRENT directory. The selected package was never tested; a green root greened a red package. Now `go test <pkg>`. - The vcache key under-covered the composite it wrapped: the content hash skipped .nilcore (so a rewritten artifact replayed an old chain-verified pass) and the key omitted the browser-verify command, the verify-pack list, the evidence toggles and the sandbox image. Turning a check on replayed a green recorded with it off. - Evidence verifiers were discovered EAGERLY, at env construction, before the backend ran — when .nilcore/artifacts/ is necessarily empty. The composite froze with zero evidence legs and never checked the run's own artifact. Now lazy, as the build path already was. - swarm --resume re-seeded only failed and passed-unmerged shards. A DAG dependent skipped because its dep failed persists as StatusQueued, which neither source reads, so a resumed run converged exit 0 with planned work never executed. Red budget-exhausted shards now also block a clean converge. Also: the tiered fast path armed on the wrong leg of a compound command and dropped -run/-skip; validateVerifyPacks (the documented "explicit startup signal") had no caller at all. Security and liveness: - browsersession scrubbed typed secrets from page text, title, console and element values but NOT Observation.URL — a secret typed into a GET form reached the model prompt and the append-only event log. Its test never asserted the URL, so it passed blind (I3). - termui's spinner stop closed its channel and joined the animator while holding the console mutex the animator needed. Reproduced; fixed with TryLock so a dropped frame, never a wedge. - MCP's stdio WRITE side was not ctx-cancellable: Encode blocked forever under the transport mutex against a peer that stopped draining stdin, permanently leaking a serve drive slot. Responses are now size-capped (a hostile server could OOM the host) and children are reaped. - session persist ran on the ctx the drive had just cancelled, so conversation state never wrote and the error was swallowed. Detached via context.WithoutCancel. Session.Checkpoint had zero callers; now wired. - An empty-content model reply was appended verbatim and poisoned history, killing the run terminally. Reachable: the Anthropic decoder drops server_tool_use blocks, so a web-search-only turn decodes to empty. Shipped features that were structurally inert: - Graduated auto-approval could never fire. Presets use AllowBranches ["*"], matched by path.Match, whose * never crosses '/' — but every real gate scope is a slash-y branch. Trust and the per-day rate/dollar windows keyed on per-run-unique scopes, so MinSuccesses was unsatisfiable and MaxPerDay and MaxDollarsDay never bound. Now * matches any non-empty scope and the three windows key on a stable scope FAMILY. The protected-base floor, DenyBranches, the kill-switch and the chain-verified requirement are unchanged and still evaluated on the CONCRETE branch. Fixing reachability exposed the dollar-window bug, which is fixed here too. - self-improve had NO merge step: Propose logged self_edit_merged and returned merged=true while nothing merged. A real Flow.Merge seam now lands the verified branch, and merged=true means it landed. - swarm derived each shard's egress and dropped it on the floor; every shard ran --network none, so the research preset could never verify green and --egress-allow was inert. The allowlist proxy now enforces the ROLE-INTERSECTED set (never the operator's wider tree) and the change to what a shard may reach is announced and audited. - memory/lessons and operator steering reached only run/watch, never the doors people use (chat, TUI, serve). -blast-radius fenced neither chat nor serve native drives. A serve-restart-resumed run had no dollar ceiling. serve's --webhook opened a second SQLite handle on the DB serve owned. BEHAVIOR CHANGE, deliberate and double-opt-in: with NILCORE_SELFIMPROVE_AUTOAPPROVE=1 the flywheel previously performed a no-op and now performs a real merge. DefaultScope still forbids the verifier of record, the core loop and every contract file, the execution-time Changed screen still fails closed, and the edit is still verifier-green first. BEHAVIOR CHANGE, announced: code/fix/research swarm shards now reach the hosts their preset declares (previously none). Deny-all presets stay --network none; a proxy that cannot bind fails closed. Docs reconciled to the code as it now is: AGENTS.md's constitution had been corrupted by a find/replace into "the native loop, Codex, and Codex"; PREREQUISITES demanded Go 1.23 while go.mod requires 1.25; four merged PRs had no CHANGELOG entry (CLAUDE.md §6); REFERENCE.md was a full phase stale; dead references to pruned symbols (impact.Localize, policy.Gate, EgressWith, blackboard) and two documented-but-never-implemented env gates were removed; nine dark env vars are now documented. make verify green (119 packages, 0 lint issues); go test -race clean across 17 concurrency-sensitive packages; -tags tui builds; the default binary still links zero Charm (I6); GOOS=linux sandbox cross-compiles. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Extracted a ground-truth pack from the tree (CLI surface + per-command flags,
the full env-var inventory, package inventory + metrics, emitted event kinds,
invariant enforcement sites, shipped-vs-gated status), audited all 30 docs
against it, and had every repair independently REFUTED by a second reader.
Fifteen surviving defects were re-fixed. The code is untouched: markdown only.
Load-bearing corrections:
- router.Classify(goal) returns run|build|swarm — NOT |decompose. Decompose is
a fourth Preset, opt-in only ("Classify never auto-selects it; it is not in
All()"), reached solely via `nilcore decompose` / `do -as decompose`.
CLAUDE.md, AGENTS.md and docs/TASKS.md all asserted the wrong return set.
- docs/ARCHITECTURE.md overstated the container egress control. It claimed an
allowlist proxy "is the only way out" and summarized it as an "SSRF-safe
egress allowlist". In fact Container.AllowEgressVia only sets
--network bridge and HTTP(S)_PROXY, and no iptables/nftables filter exists
anywhere in the tree: enforcement is at the PROXY layer, honored by
proxy-aware clients, not at the packet layer. The proxy is the only
*sanctioned* way out, not a hard network wall. Egress is off by default, and
the namespace backend (the Linux default) has no proxy path and is
unconditionally deny-all. Both the §Execution-model prose and the §Security
summary now say this precisely.
- Metrics refreshed against the code everywhere they appear: 120 packages
(111 under internal/), 375 source + 406 test files, ~89.8K non-test LOC,
~177.5K total, Go 1.25.
- Six operator env vars were documented only in suffix shorthand
(NILCORE_OPENROUTER_{MODELS,REASONING,TRANSFORMS,PLUGINS},
NILCORE_COMPAT_{AUTH_SCHEME,KEY_ENV}) and so were unfindable by their exact
name; they are now spelled out, along with the four prefix-constructed
NILCORE_{CODEX,CLAUDE}_{MODEL,EFFORT} that a literal grep misses entirely.
- docs/REFERENCE.md gained an upgrade note for the two settings whose MEANING
changed at 573a4df: NILCORE_SELFIMPROVE_AUTOAPPROVE=1 was a no-op and now
performs a real merge, and `nilcore swarm` shards now reach their preset's
declared hosts (they previously ran --network none).
- Roadmap and plan docs keep their plans but now carry accurate SHIPPED /
PARTIALLY SHIPPED / NOT BUILT status, and no longer name pruned symbols
(impact.Localize, policy.Gate, EgressWith, internal/blackboard,
emit.NopEmitter, objective.MarkRun, route.SingleRouter) as live. Where a doc
claimed a feature worked that in fact never did — graduated auto-approval
was structurally unreachable; self-improve logged a merge it never performed
— it now says so plainly rather than quietly rewriting itself.
CHANGELOG.md history is untouched (append-only; 224 -> 225 entries, none
removed). No .go, Makefile or CI file changed; `go build ./...` still clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
RNT56
pushed a commit
that referenced
this pull request
Jul 10, 2026
Rebasing the audit-remediation + deferred-hardening branch (#100) onto main pulled in #101's features-review, which independently fixed three of the same defects #100 did. Dedupe to main's canonical implementation (which fully subsumes #100's intent) so the gate passes: - MCP response OOM guard: drop #100's boundedReader/maxMCPResponseBytes in favor of main's capReader/errResponseTooLarge (both cap at 8 MiB). Same guarantee, one implementation. - Slack WS frame cap: take main's maxFramePayload (16 MiB) over #100's maxWSFrameBytes (64 MiB). main checks the raw uint64 before narrowing to int, so it rejects both the merely-huge and the high-bit wrap-negative length before make() — realign #100's discriminating ws_test.go to it. - Artifact hollow-green: keep #100's H4 rename (urlClaim -> quoteClaim, moving the shared helper off the value-blind web.url_resolves onto the value-checking web.quote_exists) and point #101's two new EventSink tests (which only need a well-formed claim, then blank the title) at quoteClaim. Also close a pre-existing CHANGELOG gap: the deferred Rule-of-Two and suspend auto-reattach features shipped in #100 without ledger entries (only egress-hard had one). Added both per CLAUDE.md §6. No behavior change beyond the dedupe; make verify + test-race + tui-verify all green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
RNT56
added a commit
that referenced
this pull request
Jul 12, 2026
…~12 MED) + adversarial re-verification (#100) * fix: remediate 2026-07-10 adversarial audit (1 CRITICAL + ~11 HIGH + ~12 MED) Fix every finding from a fresh 12-agent adversarial audit at 63292d7, each with a discriminating test. All three gates green: make verify, make test-race (0 data races), make tui-verify. CRITICAL: {{secret:NAME}} browser/desktop typing resolved any host env var (confused-deputy exfil) -> per-task secret-name allowlist, default deny-all; capguard axis-B counts a secret-capable session. HIGH: git repo-local-config RCE closed via a .git write-guard + per-command --no-ext-diff; container egress documented as cooperative (+ NILCORE_EGRESS_STRICT fail-closed); vcache no longer replays stale-green past a changed evidence artifact; artifact hollow-green refused; sleep/suspend preserves committed work; race-recovered work keeps its branch under KeepBranch; desktop --mac-host gates every mutation; webhook self-start rate/label-bounded; chat/tui resolve -backend auto; circuit breaker not poisoned by steers/cancels. MEDIUM: promote gate scopes on target base; MCP responses size-bounded; VAR=0 disables (not enables) AUTONOMY/FLYWHEEL/LIVE_INDEX/LESSONS; codeintel O_NOFOLLOW + size cap; advisor metered against the budget wall; swarm -blast-radius wired; memory table bounded; webhook shares serve's store; clean cancel/error task lifecycle; converged build deliverable pinned. LOW: eventlog redaction covers []string/json.RawMessage; Slack WS frame cap; Telegram token can't ride a URL error. Integration-seam fix: -c diff.external= (empty) made git exec "" and broke every git diff -> removed; the .git write-guard is the real defense. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test+fix: adversarial re-verification hardening Second-wave adversarial verification of the remediation diff (30 fixes revert-tested in isolated worktrees + a completeness critic) found one live regression and several under-tested fixes; addressed here. fix(worktreefs): the .git write-guard was case-sensitive (comp == ".git"), so .GIT/config bypassed it on case-insensitive filesystems (macOS APFS, Windows) and landed in the REAL .git/config -- reopening the repo-local-config RCE. Now folds case + strips trailing dots/spaces. Proven discriminating: with the case-sensitive guard the new test plants a filter.*.clean into the real .git/config and fails. docs(agent): make the sleep/suspend comments honest -- committed work is preserved on suspend/<id> as a durable RECOVERY anchor (never lost), but the wake re-engages a fresh drive; auto-reattach + suspend/ GC are documented follow-ups (was over-claiming reattach). test: add discriminating tests for every previously-untested fix -- secret-name allowlist axis-B, egress STRICT fail-closed, meteredAdvisor, swarm blast fence, build deliverable pin, backend auto-resolve, redaction/slack-ws/telegram -- and fix safeRev's test (it passed for the wrong reason in an empty tempdir). All gates green: make verify, make test-race (0 races), make tui-verify. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(capguard): enforce Rule-of-Two trifecta on the main paths (chat/serve/swarm) The lethal trifecta (untrusted web input ∧ private repo data ∧ open egress) was evaluated only on browse/desktop; the main agent paths never checked it. Add a shared enforceRuleOfTwo choke point evaluated once at each front door's startup (where egress/web/repo are known), reusing capguard.Evaluate: - chat/tui/run (attended): a trifecta prompts once at the console. - serve/swarm (headless): a trifecta with no human FAILS CLOSED (Refuse) — the exact combination the Rule of Two exists to refuse; a graduated-auto-approval envelope may still auto-proceed inside its blast fence. Default egress is deny-all/narrow, so the verdict is Allow and every normal run is byte-identical; only a wide/wildcard egress config trips it. The audit event is metadata-only (verdict + axes + labels, never the host list — I3/I7). Enforced by default; NILCORE_RULE_OF_TWO=0 is the documented operator opt-out. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(suspend): auto-reattach a woken drive to its preserved work + GC suspend/ refs The audit fixed the sleep/suspend DATA-LOSS (committed work is pinned to a durable suspend/<id> ref instead of being branch -D'd), but nothing read it back (the wake re-drove from a fresh HEAD worktree) and the refs leaked. AUTO-REATTACH: on a drive whose session has a suspended predecessor with a preserved branch (correlated by the stable <conversation> prefix of the task id; only serve drives can sleep, so run tasks never collide), the orchestrator now bases the resumed worktree on that committed work via worktree.CreateFrom instead of HEAD, retires the predecessor, deletes the consumed ref, and logs task_resumed. A stale/unresolvable ref falls back to HEAD (today's behavior — no regression, no data loss); no predecessor ⇒ byte-identical. GC: Checkpoint.SweepSuspended (wired best-effort on serve boot) prunes suspend/ refs whose task is resolved and caps the survivors to the most-recent N, closing the branch leak. New worktree.ListBranches helper. Backend contract I1 untouched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(sandbox): opt-in HARD container egress boundary via an --internal network + gateway The container backend's egress allowlist was COOPERATIVE (--network bridge + HTTP(S)_PROXY env) — bypassable by a command that ignores the proxy (curl --noproxy, raw sockets, /dev/tcp). Add an OPT-IN hard mode (NILCORE_EGRESS_HARD): - a per-run --internal docker/podman network (NO default route) to which the sandbox attaches ONLY, so the proxy is the sole egress path — unbypassable at --cap-drop=ALL, no host root/NET_ADMIN; - the allowlist proxy runs as a dual-homed GATEWAY container via a new hidden "nilcore egress-gateway" verb (reuses policy.EgressProxy — no new module, I6); - the sandbox's --dns points at the gateway to blackhole in-sandbox DNS. Fails CLOSED to --network none on any setup error (never cooperative-fallback); idempotent teardown + a label-scoped reaper on serve boot reclaim orphans. HONEST residuals (documented in docs/ARCHITECTURE.md + comments): DNS-tunnel exfil is only mitigated, hard mode needs the nilcore/sandbox image (not bare debian), and the lifecycle is Linux-container CI-validated (only the arg assembly + decision seam + gateway proxy are unit-tested on the host). The namespace backend's empty netns stays the recommended hard boundary; NILCORE_EGRESS_STRICT stays the fail-closed cooperative refusal. Default (opt-out) path is byte-identical. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore: satisfy golangci-lint (ineffassign + ST1005) - orchestrator.go: drop the dead resumeBranch/resumedFrom reset on the resume-fallback path (wt stays nil ⇒ HEAD fallback runs; the vars are not read again) — ineffassign. - ruleoftwo.go: lowercase the leading word of two error strings — ST1005. Both are behavior-preserving. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore: reconcile #100/#101 overlapping fixes after rebase onto main Rebasing the audit-remediation + deferred-hardening branch (#100) onto main pulled in #101's features-review, which independently fixed three of the same defects #100 did. Dedupe to main's canonical implementation (which fully subsumes #100's intent) so the gate passes: - MCP response OOM guard: drop #100's boundedReader/maxMCPResponseBytes in favor of main's capReader/errResponseTooLarge (both cap at 8 MiB). Same guarantee, one implementation. - Slack WS frame cap: take main's maxFramePayload (16 MiB) over #100's maxWSFrameBytes (64 MiB). main checks the raw uint64 before narrowing to int, so it rejects both the merely-huge and the high-bit wrap-negative length before make() — realign #100's discriminating ws_test.go to it. - Artifact hollow-green: keep #100's H4 rename (urlClaim -> quoteClaim, moving the shared helper off the value-blind web.url_resolves onto the value-checking web.quote_exists) and point #101's two new EventSink tests (which only need a well-formed claim, then blank the title) at quoteClaim. Also close a pre-existing CHANGELOG gap: the deferred Rule-of-Two and suspend auto-reattach features shipped in #100 without ledger entries (only egress-hard had one). Added both per CLAUDE.md §6. No behavior change beyond the dedupe; make verify + test-race + tui-verify all green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: RNT56 <rnt56.2b@gmail.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two commits: a features-completeness remediation, then a full documentation reconciliation.
573a4df— fix every finding from the features reviewAn 11-agent adversarial audit of the whole tree found 11 HIGH defects (all independently verified, four reproduced with runnable proofs), ~48 MED and ~70 LOW. The recurring theme was features that were built, tested, and documented — and completely inert, with green tests actively masking them. Every fix carries a discriminating test that fails on the pre-fix code.
Verifier integrity (I2) — four false-greens:
code.test_passesbuiltgo test -- '<pkg>', which tests the current directory, not the selected package. Nowgo test <pkg>..nilcoreand omitted the browser-verify command / pack list, so a rewritten artifact or a newly-enabled check replayed a stale green. Key now covers them.swarm --resumedropped skipped DAG dependents, converging exit 0 over work that never ran. Now re-seeds skipped/queued/running shards; red budget-exhausted shards block a clean converge.Security / liveness: typed secret leaking through
Observation.URLinto the prompt and the append-only log (I3); a spinner deadlock that wedged interactive chat; MCP's write side blocking uncancellably under its mutex; conversation state that never persisted (drive()cancelled its own ctx then wrote with it); an empty-content model reply poisoning history and killing the run.Shipped features that were structurally inert: graduated auto-approval could never fire (
path.Match("*", "task/x")is false; trust/rate/$ windows keyed on per-run-unique scopes) — now reachable and bounded, floor still on the concrete branch; self-improve loggedself_edit_mergedwhile nothing merged — now a real merge; swarm shards ran--network none— now reach their preset's declared hosts via a role-intersected allowlist proxy; memory/lessons/steering/blast-fencing reached only run/watch, now every front door.Two deliberate, announced behavior changes:
NILCORE_SELFIMPROVE_AUTOAPPROVE=1was a no-op and now really merges; swarm code/fix/research shards now reach preset hosts.000cccb— reconcile every document with the codeA ground-truth pack was extracted from the code, all 30 docs audited against it, and each repair independently refuted by a second reader (15 surviving defects re-fixed). Headline corrections:
router.Classifyreturnsrun|build|swarmnot|decompose;ARCHITECTURE.mdoverstated container egress as SSRF-proof/IP-layer when it is proxy-env-only; metrics refreshed (120 pkgs, 375 src + 406 test files, ~89.8K non-test LOC); undocumented env vars added; upgrade notes for the two behavior changes.CHANGELOG.mdhistory untouched (append-only). Markdown only.Verification
make verifygreen (119 packages, 0 lint issues);go test -raceclean across 17 concurrency-sensitive packages;-tags tuibuilds; default binary links zero Charm (I6);GOOS=linuxsandbox cross-compiles.🤖 Generated with Claude Code