chore: audit remediation — dead code, real bugs, test gaps, doc drift#8
Merged
Conversation
The registry field was injected via WithRegistry and stored but never dereferenced: callerIdentity resolves the caller from the as value alone (Option B — attribution only, no tool-layer authz). The accompanying doc comments described a caller-resolution gate that does not exist. The agent roster sync that genuinely needs a registry runs in cmd/mcp/main.go via agent.SyncToTable on its own agentRegistry, untouched here.
- activity.DiffStats: never constructed, serialized, or read (Event.Metadata carries the GitHub payload as raw json.RawMessage). - activity.ErrNotFound / ErrConflict: no handler branches on them and the package is read-only (two SELECT :many queries), so neither condition arises. - agent.RegistryRow.SyncedAt / RetiredAt: populated from the DB but read by no consumer; the SQL columns stay (the upsert/retire SQL maintains them). - todo.ProjectCompletion: type referenced nowhere in the tree.
Residue from retired features (learning subsystem, rss-highlight aggregates). Each removed method had zero callers tree-wide; each removed query had no remaining s.q.* consumer after the method went. Regenerated sqlc. - feed/entry: RecentFeedEntries, LatestFeedEntries, TopUnreadFeedEntriesRecent, DeleteOldIgnored, TopItems (+ their queries) - feed: EnabledFeeds no-schedule variant (kept EnabledFeedsBySchedule) - todo: PendingItemsByTitle, PendingItems, ItemsCreatedSince, StaleSomedayCount, InboxCount, InboxItems, PendingItemsWithProject (+ queries + orphaned Pending type) - goal: legacy CreateGoal 7-arg, bare GoalByID, GoalByTitle, CreateMilestoneSimple (kept shared CreateGoal/GoalByID/CreateMilestoneWithPosition queries) - daily: ItemByID method + query + dead ErrNotFound sentinel Shared queries still consumed by live code were deliberately preserved.
signal.NotifyContext registered only os.Interrupt (SIGINT), which arrives from a TTY Ctrl-C. Docker/VPS deployment (CLAUDE.md tech stack) stops containers with SIGTERM, which was not caught — so production stops would skip the drain path and hard-kill mid-request/mid-pass. Register SIGTERM alongside SIGINT in all three entrypoints (app run, app embed-backfill, mcp).
The run() select returned immediately on the errCh branch, before srv.Shutdown and wg.Wait. The scheduler and reconciler goroutines select on ctx, which the errCh branch never cancels (only a signal does), so they kept running while the deferred pool.Close() closed the pool out from under their in-flight queries. Cancel ctx via stop() on the error branch so both workers observe cancellation and drain before the pool closes; the captured error is still returned.
mapWriteError mapped only unique (23505) and foreign-key (23503) violations, so a blank title hitting chk_goal_title_not_blank (23514) fell through to a wrapped 500. The sibling mapProposeError already maps CheckViolation to ErrInvalidInput; mirror it so the owner create/update paths return 400 for a blank title. A handler-level trim+reject and the asserting integration case follow in the goal test pass.
…lter The Project input field was declared in the wire schema only to be rejected as unsupported_filter on every non-empty value — the canonical catalog never advertised it and content.SearchFilter has no project dimension. Per the owner's call, drop the reserved field (and its reject-stub validator + test) rather than carry a parameter that can only ever error. The result struct's Project (parent project title) is unrelated and kept.
The HTTP plan-write guard rejected only inbox, so the admin API accepted a done/someday/archived todo onto today's plan while the MCP plan_day tool (execution.go) rejected the same todo via a todo|in_progress allowlist — despite PutPlan's doc calling itself the human equivalent of plan_day. Mirror the MCP allowlist and fix the doc. Integration test seeds a someday todo and asserts 400 + zero plan rows (real Postgres).
The MCP write path (propose_content/revise_content) and admin SendBack already reject control characters, but admin Create/Update validated only length, type, and status — so a control char smuggled in via a JSON \u escape reached the DB through the admin boundary while the MCP boundary caught it. Apply the same strict-for-title/excerpt, prose-for-body check to both handlers, matching the input-validation-checklist handler-consistency rule. Table-driven tests assert a 400 per field.
ConsumeRefreshToken only deletes the exact presented token, so expired-but- unconsumed rows accumulated forever — the expires_at index and the schema 'eligible for cleanup' comment promised a sweep that did not exist. Add DeleteExpiredRefreshTokens (served by that index) and a background worker wired through the same wg.Go/ctx drain as the scheduler and reconciler: it purges once at startup, then hourly, and exits on ctx cancellation. Integration test asserts only the expired token is removed.
The owner's review_note (set when sending content back for changes) was selected by ContentsByCreator (the MCP list_content readback) but not by ContentByID, so reopening a changes_requested item in admin showed no note. Add review_note to the ContentByID query and hydrate Content.ReviewNote in the detail read. ListContents is left unchanged (owner decided detail-read only). Integration test sends a draft back with a note and asserts ContentByID returns it.
…al pages, zoneless server - app.config.ts: make the HTTP transfer cache explicit via withHttpTransferCacheOptions so Server-rendered pages do not refetch their API data on hydration (the v22 default, now self-documented; withHttpTransferCache does not exist in v22). - app.routes.server.ts: prerender /privacy and /terms (static legal text) instead of paying per-request SSR via the ** fallback, matching /about. - main.server.ts: drop provideZoneChangeDetection — it contradicted the zoneless browser config and required zone.js, which is not installed; the server now inherits provideZonelessChangeDetection from the shared config.
home had only a loading flag, so a failed content/topics load fell through to the @else branch and rendered the false empty-state 'Nothing published yet' — a backend outage looked like 'the author published nothing'. Add a hasError computed (mirroring the articles page) and an error/retry branch, and gate the recent band on !hasError. Spec flushes a 500 and asserts the error UI renders.
The palette is role=dialog aria-modal but had no focus trap, so Tab escaped to the background page. Add cdkTrapFocus + cdkTrapFocusAutoCapture (A11yModule), mirroring the modal component, and mark the backdrop aria-hidden.
…dead graph API The shared/components barrel held a Claude Design ingest of 41 components; 33 had zero template usage and zero TS imports anywhere in the app (this app is a consumer, not a component-library producer). Delete them and trim the barrel to the 8 actually used (data-table, empty-state, energy-meter, form-field, kbd, loading-spinner, modal, status-badge). Also remove content.service.getKnowledgeGraph and its now-orphaned ApiKnowledgeGraph/ApiGraphNode/ApiGraphLink types (zero callers).
…ic-page specs markdown.service binds DOMPurify output to [innerHTML] but had no sanitization test — add cases asserting a <script> tag, an onerror attribute, and a javascript: scheme are stripped from rendered output. Delete the about/privacy/ terms specs whose entire body was expect(component).toBeTruthy() (zero-logic static pages).
validateProposeContent gates untrusted agent input (required title/type/body, content-type enum, C0/C1 control-char rejection per field, slug derivation, topic-UUID parsing) and had no direct test. Add a table-driven test naming the bug each case catches, plus fuzz over the slug-derivation and parseTopicIDs paths (must not panic). The C1 case uses string(rune(0x80)) — a raw \x80 byte is invalid UTF-8 and would decode to RuneError, never exercising the C1 check.
…erage scheduler_test.go drove a hand-rolled feedTestHandler — a parallel reimplementation of the production handler (via a test-only feedHandlerStore interface) that had diverged, so the real handler's behavior was asserted nowhere. Delete feedTestHandler/feedHandlerStore and the TestFeedHandler_* tests, plus the TestMaxConsecutiveFailures const==5 tautology. Add real *feed.Handler Create/Update/Delete/Fetch tests and an IncrementFailure auto-disable test against a testcontainers store. Extract the scheduler's due predicate into a pure due() function (zero behaviour change — the skip condition is identical) and unit-test it directly via TestDue.
…ation stats_test.go hand-rolled a db.DBTX fake (stubDBTX/emptyRows/zeroRow) and the handler tests asserted zero-in/zero-out through it — a forbidden DB mock that verified nothing about the real SQL aggregation. Delete the fake and those tests; add an integration test seeding real rows and asserting Overview/ SystemHealth/ProcessRuns/Drift produce correct non-zero counts. Extract the days-clamp into a pure parseDays() (zero behaviour change) and keep the clamp/fuzz + computeAreaDrift + state-mapper unit tests against it.
The only interval-recurrence seed had last_completed_on=NULL (the trivial branch), so the interval-boundary computation was untested. Add cases with last_completed_on exactly interval-days ago (due), interval-1 days ago (not due), and weeks/months units, with hand-computed dates.
Pins the CHECK-violation mapping fix: a whitespace-only title hits chk_goal_title_not_blank (23514) and must surface as ErrInvalidInput (400), not a wrapped 500.
The security-critical persistence half of auth had no test. Add consume happy-path, double-consume returns ErrNotFound (single-use), expired-token rejection, and rotation (old hash gone). Delete the skipped stub TestRefresh_InvalidToken_ReturnsUnauthorized, which asserted nothing while claiming coverage that now actually exists.
- content: TestRegression_BySlug_PrivateContentReturns404 only asserted a Content zero value has IsPublic==false — a struct-zero tautology that never invokes the handler (real coverage is TestHandler_PublicBySlug_HidesNonPublic). - api: TestStatusCapturingWriter_Decision re-implemented the commit predicate in the test body and compared it to itself; TestActorMiddleware_TxAvailableInContext asserted only that context.WithValue round-trips, not the middleware (the real wiring is covered by the integration test's ActorMiddleware+TxFromContext path).
Comment-only fixes where the comment contradicted the code (code is authority): - stats/topic/todo handler doc comments named wrong routes / query params (per_page not limit; /api/admin/system/stats*; /api/admin/knowledge/topics). - ops/types.go Destructive example referenced the removed learning-plan feature. - stats process_runs.kind comments claimed 'agent_schedule' but the CHECK allows only 'crawl'. - migrations refresh_tokens.token_hash comment said 'Bcrypt or SHA256'; the code only stores a SHA256 base64url hash. - mcp.go package doc described a per-mutation authz gate that does not exist (Option B is attribution-only; access control is the MCP transport). - brief.go/catalog.go 'tasks' section docs omitted the emitted recurring_todos. - api integration test request path literal matched the real content route.
TestIntegration_SearchKnowledge_ProjectRejected exercised the search_knowledge project field removed earlier; it is dead now that the field is gone. The field-removal commit dropped the unit test but missed this integration one (behind the integration build tag, so the unit gate did not catch it).
The error path overwrote a captured listener error with a shutdown error only when the former was nil, silently dropping one of two independent failures. Use errors.Join so both surface (errors.Join(nil, x) is just x).
Create/Update validated control chars in title/excerpt/body but not slug, which becomes a URL path segment. The DB chk_content_slug_format CHECK rejects whitespace and slashes but not non-whitespace control chars (C0-non-ws, DEL, C1), so a control-char slug passed both the handler and the DB. Add slug to the strict control-char check (which still allows the intentionally-supported Unicode/CJK slugs). Surfaced by L2 review; tests cover slug rejection in Create and Update.
- feed: rename the unexported due() predicate to isDue() (clearer boolean predicate; avoids a future name collision in the package). - content: co-locate containsProseControlChars with containsControlChars and checkContentControlChars so the three control-char helpers sit together (they cross-reference each other; this stops them drifting 375 lines apart). - content: drop the dead wantTag table field and the obsolete 'tag' filter test case + benchmark param (tags were removed in favour of topics; parsePublicFilter no longer parses tag).
Both READMEs described a three-axis authorization gate in a non-existent internal/mcp/authz.go — the removed Option-A design. The actual model is Option B: as is attribution only (created_by / activity actor / caller-scope), and access control is the MCP transport (admin-email OAuth + bearer on HTTP /mcp; OS process boundary on stdio); a fabricated as is caught by the created_by FK. Also: the tool count was stale (fourteen → fifteen) and the toolset table omitted set_todo_recurrence.
|
This pull request is abnormally large and would use a significant amount of tokens to review. If you still wish to review it, comment "augment review" and we will review it. |
The doc-comment pass edited the refresh_tokens.token_hash column comment and the StatsProcessRunsByStatus query comment; sqlc propagates SQL comments into the generated Go doc comments, so the generated files needed regeneration. Comment-only — no type or query change.
The hero narrative led with a planner-on-Cowork morning briefing as if it ran daily; the actual drivers today are Koopa (decisions/router), Claude Code (dev sessions), and hermes (Obsidian curation). Lead with those three, present the planner as the daily-driver the system is designed around — wired but executed by an external scheduler, not yet a daily habit — and reorder the roster to put the active actors first. Roster facts unchanged (all agents are registered); this is emphasis, matching the 'README describes what-is' rule. EN + zh-TW.
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.
Summary
A full read-only audit (backend: 18 package/concern agents; frontend: 6 agents; every consequential finding independently re-derived from source) followed by gated remediation. 29 atomic commits, each verified by re-running the build/test gates — not subagent self-reports. No CRITICAL/HIGH from L1 (
go-reviewer) or L2 (review-code).Net: +2,090 / −9,751 across backend (Go) and frontend (Angular 22).
Dead code / tech debt (the bulk of the deletions)
Server.registry(write-only field), dead types/sentinels, 18 zero-reference store methods + their orphaned sqlc queries (regenerated), and the never-supportedsearch_knowledgeprojectfilter.Real bugs
SIGTERM(was SIGINT-only — Docker/VPS stops skipped the drain); drain background workers on a listener error instead of leaking them and closing the pool out from under in-flight queries.goalCHECK violation → 400 (was a wrapped 500); narrowedPutPlanto theplan_daystate allowlist (admin could plan done/someday/archived todos); reject control chars in admin content Create/Update including slug; surfacereview_notein the admin detail read; purge expired refresh tokens periodically.Test gaps filled (real coverage, not padding)
validateProposeContenttable + fuzz; replaced the fictional feed handler test with real*feed.Handlerintegration coverage; replaced the stats DB-mock with testcontainers; todo interval-recurrence arithmetic; goal blank-title; auth refresh-token consume cycle (single-use / expiry / rotation); markdown XSS sanitization.Doc drift
internal/mcp/authz.go)" paragraph described a removed Option-A gate in a non-existent file — corrected to the actual transport-boundary + attribution model; fixed the stale tool count (14 → 15) and added the missingset_todo_recurrence.Test Plan
Backend (all green, run locally):
go build ./...·go vet ./...·golangci-lint run ./...→ 0 issuesgo test ./...(unit) → all passgo test -tags integration ./...→ all 22 packages pass (real PostgreSQL via testcontainers)Frontend (all green):
ng build·ng lint→ pass ·ng test→ 570/570 passReview:
go-reviewer+ L2review-codeindependent passes: 0 CRITICAL, 0 HIGH. Surfaced items (errors.Joinon the shutdown path; adminslugcontrol-char gap) addressed with tests.