Audit fixes: 13 of 14 findings - #10
Open
timothyerwin wants to merge 7 commits into
Open
Conversation
grpc v1.82.0 (GO-2026-6061) and x/text v0.38.0 (GO-2026-5970), both indirect, both with fixes available. These ship regardless of which Go toolchain builds the release, unlike the stdlib advisories govulncheck also reports, which the CI toolchain already covers.
config.Mode has always documented itself as "persisted when the user cycles it", and cmd/interactive.go has always READ it at startup. Nothing ever wrote it back, so Shift+Tab and /mode silently reset on every restart — a setting that quietly forgets, which is the hardest kind of bug to notice. Both entry points now route through one setMode that changes the session and saves the config together, with a guard test that fails if a third call site skips the save.
ResolvePin promised the seed was "persisted to BOTH stores before returning, so this is the only run that ever consults default_model". Both writes discarded their errors, and the user-store writer dropped four without returning anything. Best-effort persistence is right and stays: failing to remember a preference must never fail the operation. But silence was wrong. If neither store is writable, every run re-seeds from default_model, and because that value legitimately changes as models are added and retired, a user's model can drift between releases while everything here believes the pin is stable. ResolvePinSeeded reports that case so the caller can say it once; ResolvePin keeps the old signature for callers that cannot act on it.
webjwt is the single verifier for platform-signed inbound webhook tokens (Bot Framework, Google Chat) and had no tests at all. "Fails closed" is a claim worth proving on an authentication boundary, so this exercises each way a token can be wrong: expired, unexpiring, wrong issuer, wrong audience, unknown signing key, unknown kid, alg-none, HMAC against an RSA key set, unconfigured verifier, and an unreachable JWKS. It also pins the stated flood defense — 25 forged kids must not become 25 outbound fetches — and the Bot Framework metadata path. 87% coverage on a package that had none. explore covers the concurrency-cap resolution (0 means default, not zero readers) and the finding indenter.
The same function was written three times, in two map shapes, across prefs clustering, mood's repetition detector, and lesson dedup — and had already begun to diverge: the lessons copy had dropped a guard the other two kept. (Unreachable, as it happens, but divergence is the point.) A metric that decides whether two things are "the same" should have one definition and one set of tests. Generic over the value type so callers keep whichever set spelling they already use.
Reachability analysis across the module, tests included, found ten functions nothing can call. All are in internal/ packages, so there are no external consumers to consider. The interesting group was three package-level wrappers in autonomy whose comment said "used by cmd" — cmd never used them. The methods they wrapped are live and stay. deadcode now reports nothing.
v0.29.0 removed automatic model routing. The code recorded that carefully, with a tombstone comment at every site explaining what went and why. The documents around the code were never given the same treatment, so they kept advertising the deleted system in the present tense. README - "Out of the box it uses cheap models for routine work and strong models when the task is hard or risky" described the deleted ladder. It was also the only false claim a prospective user reads before installing anything. Replaced with what is true and arguably the better pitch: one model, chosen by you, that does not change underneath you, with delegated work able to ride its own pin. - /plan no longer "gets a second model's review before you approve it" on its own; that review is a one-shot the user triggers from the approval card. ROUTING.md — rewritten. Every symbol it documented as current (decideLane, ResolveModel, SteerResolvedModel) has zero references in non-test code, and it carried its own warning that finding those names means something is stale. It now describes the pin chain, the two narrow exceptions (utility plumbing and infrastructure failure), why capability gaps refuse rather than substitute, and which error classes may walk a fallback chain. COMPACTION.md — the budget is window-relative (85% of learned capacity), not the ~45K constant this claimed. Worth correcting beyond accuracy: an absolute default is precisely what the current design rejects, so the stale number misrepresented a deliberate position as a tunable. The summarizer is the catalog's utility_model, not a forced Anthropic call. Two file headers contradicted correct comments lower in the same file: llm/lanes.go said Automatic selection prefers keyed vendors, and llm/resolve.go described steering as live policy while resolveHosted below it explained the deletion.
There was a problem hiding this comment.
Walkthrough
Resolves two indirect dependency advisories, persists TUI permission mode adjustments across restarts, warns if model seed persistence fails across both stores, consolidates duplicate Jaccard set-similarity implementations into a shared package, purges ten unreachable internal functions, adds unit tests for webjwt and explore helpers, and updates documentation to match the current pinned-model architecture.
- Permission mode cycling and the /mode command route through setMode to keep session state and on-disk config in sync, guarded by an AST test.
- setsim.Jaccard replaces three diverging implementations across mood tracking, lesson clustering, and preference grouping.
- webjwt gets test coverage verifying token validation edge cases, alg-none rejection, and JWKS fetch throttling.
- Documentation across README.md, ROUTING.md, and COMPACTION.md is brought into alignment with the v0.29.0 pin model.
The changes are clean, well-factored, and thoroughly covered by existing and added tests.
No findings.
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
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.
Fixes for the repo audit. Seven commits, each one finding or one theme, so they can be taken or dropped independently.
Correctness
The permission mode never persisted.
config.Modedocuments itself as "persisted when the user cycles it", andcmd/interactive.go:33has always read it at startup — but nothing ever wrote it back. Shift+Tab and/modesilently reset on every restart. The read side was fully built; this was unfinished wiring, not a design choice.Both entry points now route through one
setModethat changes the session and saves the config together, with a guard test that fails if a third call site skips the save (verified: it catches the regression when reintroduced).A pin that cannot be recorded now says so.
ResolvePinpromised the seed was "persisted to BOTH stores before returning"; both writes discarded their errors and the user-store writer dropped four without returning anything.Best-effort persistence is correct and stays — failing to remember a preference must never fail the operation. Silence was the bug: if neither store is writable, every run re-seeds from
default_model, and because that value legitimately changes as models come and go, a user's model can drift between releases while everything believes the pin is stable.Security
Both dependency advisories cleared — grpc v1.82.0 (GO-2026-6061), x/text v0.38.0 (GO-2026-5970).
Worth stating precisely:
govulncheckreports 13, but 10 are stdlib from the local go1.26.3 toolchain. CI pinsgo-version: "1.26", which resolves to go1.26.8, so released binaries were never affected by those ten. Only these two shipped.The webhook token verifier now has tests.
webjwtis the single verifier for platform-signed inbound tokens (Bot Framework, Google Chat) and had none. "Fails closed" is worth proving on an authentication boundary, so this covers expired, unexpiring, wrong issuer, wrong audience, unknown signing key, unknown kid,alg:none, HMAC against an RSA key set, unconfigured verifier, and an unreachable JWKS — plus the stated flood defense (25 forged kids must not become 25 outbound fetches) and the metadata-discovery path. 87% coverage on a package that had zero.Documentation
The weakest area, and the pattern was consistent: v0.29.0 removed automatic routing, the code recorded that carefully with a tombstone at every site, and the documents around it kept advertising the deleted system in the present tense.
/plan, which no longer gets a second model's review on its own.decideLane,ResolveModel,SteerResolvedModel— has zero references in non-test code, and it carried its own warning that finding those names means something is stale. It now describes the pin chain, the two narrow exceptions, why capability gaps refuse rather than substitute, and which error classes may walk a fallback chain. Every claim was verified against the code before writing.llm/lanes.go,llm/resolve.go).Consolidation and cleanup
One Jaccard instead of three. Written three times in two map shapes, and already diverging — the
lessonscopy had dropped a guard the others kept. A metric deciding whether two things are "the same" should have one definition and one set of tests.Ten unreachable functions removed.
deadcodenow reports nothing. The interesting group was three package-level wrappers inautonomywhose comment said "used by cmd" — cmd never used them. The methods they wrapped are live and stay.Tests for
explore(concurrency-cap resolution, finding indenter).Landed separately in memcode.ai
The catalog drift guard had never run.
TestEmbeddedCatalogMatchesRootlooked forgateway-cloud/models.json, which has never existed, so it hit its "outside the monorepo" skip on every run and reported success while checking nothing.It had drifted, exactly as designed to prevent: 14 fallback chains differed from the public catalog, including
gemini-flash → gemini-pro— the same-vendor hop behind the 12-minute review failure. All four copies are now byte-identical, and the repaired guard fails on injected drift rather than skipping. (d3d0fa3b)Not fixed, deliberately
fileExists×4,clip×3,firstLine×3,envOr×3. Each is three lines with obvious semantics, and the copies have not diverged. Consolidating would couple four unrelated packages to a new utility package to save nine lines, and Go's own library omits these deliberately. My audit said these were worth a shared package only if the count grew; that reasoning still holds. Happy to do it if you disagree.Verification
gofmt,go vet,go test -race ./...,staticcheck@2026.1, anddeadcodeall clean.