Full-repo cleanup: robustness, completeness, dead-code, doc-drift, tests#90
Merged
Conversation
…L6/L7/L25) From the full-repo audit (adversarially verified): - graapprove kill-switch now fails CLOSED on any stat error other than not-exist (a permission/I/O fault must never read as "kill-switch off"). [M2] - wakeFeeder delivers the wake onto the queue BEFORE disarming (+ an in-flight set), so a shutdown mid-handoff leaves the wake armed to re-fire next boot (at-least-once) instead of disarming-then-losing it; the in-flight guard still prevents a re-queue. [M3] - SelfImproveGate guards a nil human func (deny-default) instead of panicking. [N1] - Telegram + Slack ask-tap now claim (atomic lookup+delete) for terminal taps, closing the TOCTOU that let two concurrent taps double-resolve one question. [L6] - autosrc dequeue cancel-watcher Broadcasts UNDER the queue lock, closing the lost-wakeup window (previously backstopped only by close()). [L7] - clipSelfacc is rune-safe (never splits a multi-byte UTF-8 rune). [L25] make build + vet green; affected package tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…t env (audit H2/M6/M7) - H2: final_verify (orchestrator) and verify (native) events now carry a structural fail_class (verify.FailClass, build/test/lint/browser/other) on a FAILURE. The distiller/lessons learning pipeline read this key but no producer ever emitted it, so every failure bucketed as "unknown" — the LRN-T01 producer half was missing. Derived from the report SHAPE, never raw output (I7); added only on a failure. - M6/M7: NILCORE_AUTOAPPROVE_PRESET (documented in ARCHITECTURE.md §0 but read nowhere) now seeds cfg.AutoApprove from graapprove.Preset when no explicit envelope is set — giving graapprove.Preset a production consumer and making the documented opt-in real. An explicit config envelope always wins; an unknown preset is reported + ignored (fail-closed: auto-approval stays off). make build + vet green; agent/backend/flywheel/cmd tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…L9/L11-L18/L24)
Stale references to symbols removed earlier this session, corrected to the live ones:
- mcp/client.go pkg doc: per-tool "calls are gated" → authorization at the codegen-
descriptor boundary (Client.Gate was removed). [L11]
- trust/{selector,ledger,replay}.go: comments referencing the deleted trust.Router /
router.go now name the live trust.Selector. [L12/L13/L14/L22]
- graapprove/presets.go: bind-self-authored scope comments say id@<cmd-hash> (matching
the wired command-scoped trust), not "the verifier id". [L24]
- docs/REFERENCE.md: NILCORE_TRUST → NILCORE_TRUST_DEFAULT (the env the code reads). [L9]
- docs/MULTI-AGENT.md: drop removed `blackboard` from "Reused unchanged". [L15]
- docs/ARCHITECTURE.md / ROADMAP-COMPUTER-USE.md / ROADMAP-EXTERNAL-INFRA.md /
UPGRADE-PATH.md / TASKS.md: mcp.Gate / trust.Router references point at the live
boundaries; one corrective note added to the deep EXT-07 plan (~10 historical
citations) rather than churning each. [L16/L17/L18]
- docs/SELFACC.md: unattended path uses the now-wired NILCORE_AUTOAPPROVE_PRESET.
Docs/comments only (the CHANGELOG's historical trust.Router mentions are left as record).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- internal/verify/vcache: delete hash.go (HashWorktree + hashTree/entryLine/ fileContentDigest) + hash_test.go. It exactly duplicated verify.ContentHashWorktree and had no production caller — the cmd wiring injects verify.ContentHashWorktree as the Hasher; vcache only referenced its own copy in a comment (now corrected). [L3] - internal/graapprove: remove the unused WithSentinel Option + the now-write-never sentinel field, collapsing killSwitchEngaged(root, sentinel) → killSwitchEngaged(root) over the default path (no caller, no test used the override). [L1] Both verified zero-production-caller before removal; package tests pass. (The designed- but-unwired seams — Orchestrator.Gate/policy.Gate, OracleEscalateAfter/EscalateAfterFn, project.DeriveAcceptance, policy.EgressWith — are left for an explicit wire-or-delete decision rather than removed in a sweep.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…M10/L19/L20/L21/L23)
Tests (lock the recently-changed paths so a regression can't stay green):
- selfacc: a FAILING gated check emits boundary_outcome{passed:false} at the (id+cmd)
scope; a passing one emits {passed:true}; denied/un-admissible/pre-approved emit NONE
(the I2 trust hinge). [H3]
- orchestrator: the SelfAccept hook receives env.Box and a gate that routes to
o.Approver (nil approver ⇒ deny-default); a self-acceptance failure skips OnSuccess
and preserves no branch. [H4/M8]
- eventlog: a panicking OnAppend hook does not corrupt the log (Verify still clean). [M9]
- wakeFeeder: a not-yet-due wake is skipped; a due wake fires at-most-once. [M10]
- policy: String()/Class() cover OpenPR + BindSelfAuthored. [L19]
- selfacc Admit: case/position marker-evasion (HOST:EXEC, GO:In-Process, mid-line). [L20]
Consistency:
- dedup the byte-identical describe(): export policy.GateAction.Describe and have
graapprove call it (one source, no drift). [L21]
- DefaultBacklogPriority is now -1 (below PriorityFile=0) so every reactive source
preempts idle backlog work, matching the documented contract. [L23]
(M11 graapprove rate-counter test deferred — the full case needs custom-time log infra;
the gate decision matrix is already covered.)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
TestGateReprompt timed out (10s) under full-suite parallel load — a pre-existing tight bound, now more likely to bite since the cleanup added git-using orchestrator tests that run concurrently. The wait helpers return the instant their condition holds, so the bound only ever bites a genuine hang; raised testWaitBudget 10s→30s (and pointed waitPhase at the same const) so a loaded CI runner can't flake the merge-gating suite. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…take races (audit H1/M4)
Both backends had Receive (the serve loop) AND a drive's gate Ask reading the SAME
socket/poll concurrently — a real data race over Slack's b.src/WebSocket (reproduced
with -race) and Telegram's offset read-call-write across the getUpdates network call
(every update double-delivered). A mutex can't fix it: holding a lock across the
blocking Next()/long-poll would deadlock Receive against a waiting Ask.
Fix: ONE intake goroutine owns the socket/poll and DEMUXes each event —
- a gate button/callback answer → the waiting Ask, via a per-gate buffered channel
(registered before the answer can arrive; authorizer-checked; first answer wins);
- an ask_user tap or a message → the task queue Receive drains (taskWake poke).
Receive and Ask now only wait on channels — never read the socket — so any number of
concurrent Asks + the Receive loop never race. Behavior preserved: unauthorized clicks
are ignored+logged (gate keeps waiting), ask taps/messages during a gate are still
delivered, gate ids/buttons unchanged.
Added concurrent Receive+Ask tests for BOTH backends (gate answer → Ask, message →
Receive) that pass under `go test -race`. Existing channel tests unchanged + green.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The per-day MaxPerDay rate window depends on countAutoApprovalsToday folding ONLY auto_approve events for the exact (action,scope) on today — not boundary_outcome / auto_deny, not another scope/action/day. Now tested (today-match, wrong-kind/scope exclusion, non-today day = 0, missing log = 0). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…audit L2/L8) OracleEscalateAfter + native.EscalateAfterFn (RTE-T05) were built end-to-end but never connected in production: EscalateAfterFn was set nowhere, OracleEscalateAfter had no caller, and TrustRouteOracle.EscalateAfter was a no-op (returns the default). Removed the whole dead capability coherently — the TrustOracle.EscalateAfter interface method + its impl, the OracleEscalateAfter helper, native.EscalateAfterFn (+ the dynamic branch), and the never-set RoutePlan.EscalateAfter field + tests. The STATIC native.EscalateAfter (wired via chat/serve/roster) and the wired RaceN/Plan oracle path are unchanged. make build + vet green; agent + backend tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…p+document gate/egress primitives
Resolves the 4 remaining design-decision seams per the agreed leans:
WIRE — project.DeriveAcceptance + SeedCriteria + PromotionPermitted (the project-loop
analogue of the self-acceptance loop). buildStack now, at setup:
- derives the acceptance bar (strong advisor PROPOSES machine-checkable criteria, each
DRY-RUN in the sandbox + dropped if unrunnable) and SeedCriteria's it into the loop
ADD-ONLY, so JudgeProject gates on it from iteration 0 (the model proposes the bar;
the sandbox decides it — never lowers it). Gated on a strong advisor being present
(else empty bar = VerifyCmd only, byte-identical) AND on a budget-headroom probe
(the one model call respects the same wall the loop's per-iteration probe enforces,
so a budget-exhausted run still makes no call before the rail).
- asserts PromotionPermitted on a GREENFIELD-bootstrapped repo (its intended phase): a
bootstrap that left a vacuously-green verifier is refused (a real currently-red check
must exist before promotion). Checked only when bootstrapped — an existing -dir whose
tests already pass has a real green verifier, where the guard does not apply.
KEEP + DOCUMENT — Orchestrator.Gate / policy.Gate (the free-text gate primitive paired
with Classify; the live integration path uses the typed GateStructured) and
policy.EgressWith (the public sandbox-egress-allowlist composer). Annotated as retained
primitives so a future audit does not re-flag them as dead.
make build + vet green; project/policy/agent/cmd tests pass (incl. the budget-ceiling
runaway guard, which still sees 0 model calls before the rail).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.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.
A comprehensive cleanup pass driven by a multi-agent audit (8 dimensions × adversarial verify over the whole tree). The audit found 0 invariant breaks — the codebase is sound. Confirmed findings were remediated in themed commits; a handful of designed-but-unwired seams were deliberately left as wire-or-delete decisions (see below) rather than removed in a sweep.
Robustness / fail-closed
not-existstat error (a permission/I-O fault must never read as "off").wakeFeederdelivers-then-disarms (+ an in-flight set) so a shutdown leaves the wake armed to re-fire (at-least-once) instead of disarming-and-losing it.SelfImproveGatedeny-defaults a nil human instead of panicking.claim(lookup+delete), closing the double-resolve TOCTOU.clipSelfaccis rune-safe.Completeness / wiring
final_verify/verifyevents now carry a structuralfail_class— the LRN-T01 producer half the distiller/lessons pipeline read but nothing emitted (failures were all bucketed "unknown").NILCORE_AUTOAPPROVE_PRESET(documented in ARCHITECTURE §0 but read nowhere) now seedscfg.AutoApproveviagraapprove.Preset— giving the presets a real consumer.Dead-code (verified zero-caller before removal)
vcache.HashWorktree(exact duplicate ofverify.ContentHashWorktree) +graapprove.WithSentinel(+ the write-never sentinel field).Consistency / doc-drift
describe()→ exportedpolicy.GateAction.Describe.DefaultBacklogPriority→-1(belowPriorityFile=0) so reactive sources preempt idle backlog work.Tests
{passed:false}, denied/un-admissible/pre-approved→none), the orchestrator hook's box/gate threading + OnSuccess/KeepBranch skip-on-redden, eventlog OnAppend panic-recovery,wakeFeederat-most-once/not-due, GateActionType String/Class for the new types, Admit marker-evasion.The 2 HIGH-severity bugs — now FIXED (channel-intake demux)
Both backends had
Receive(serve loop) AND a drive's gateAskreading the SAME socket/poll concurrently — a real data race over Slack's WebSocket (-race-reproduced) and Telegram's offset read-call-write acrossgetUpdates(every update double-delivered). Fixed with a single intake goroutine that owns the socket/poll and DEMUXes each event: a gate answer → the waitingAsk(per-gate buffered channel, registered before the answer can arrive, authorizer-checked); an ask-tap/message → the task queueReceivedrains.Receive/Asknow only wait on channels (a mutex would deadlock Receive against the long-poll Ask). Behavior preserved; concurrent Receive+Ask tests for both backends pass undergo test -race.The 4 design-decision seams — now RESOLVED (per agreed leans)
project.DeriveAcceptance+SeedCriteria+PromotionPermitted— WIRED (the project-loop analogue of the self-acceptance loop):buildStackderives the advisor-proposed, sandbox-vetted, ADD-ONLY acceptance bar and seeds it soJudgeProjectgates on it from iteration 0 (budget-headroom-gated, so it makes no model call before the budget rail); and assertsPromotionPermittedon a greenfield-bootstrapped repo so a vacuously-green verifier is refused (a real currently-red check must exist before promotion). An existing-dirwith passing tests is unaffected.OracleEscalateAfter/native.EscalateAfterFn— DELETED: the dynamic escalate-after seam was unwired end-to-end and a no-op. Removed the interface method + impl + helper + native field + the deadRoutePlan.EscalateAfter; the wired staticEscalateAfterandRaceN/Planstay.Orchestrator.Gate/ free-textpolicy.Gate, andpolicy.EgressWith— KEPT + documented as retained public primitives (the live paths use the typedGateStructured/ the egress-profile builder), annotated so a future audit doesn't re-flag them.Verification
make verify✅ ·make tui-verify✅ · full-repogolangci-lint(clean cache) 0 issues ✅ ·-raceon the touched concurrency packages ✅ · Linux cross-build ✅.🤖 Generated with Claude Code