Pre-review cleanup pass#13
Merged
rowantrollope merged 6 commits intomainfrom May 6, 2026
Merged
Conversation
- Doc/config paper cuts: drop retired Redis-module refs from
.dockerignore/.gitignore, drop phantom afs-server target from
Makefile, align example.afs.config.json with README shape,
archive completed redis-array-backend plan, move
deploy/vercel/auth-plan.md and onboarding-flows.md into plans/,
convert absolute /Users/... paths to relative, dedup duplicated
Phase 4/5 headings in cli-first-ui.md, refresh repo-walkthrough.
- Dead code (-480 LOC): delete afs-situation-room-kit.tsx (zero
importers), no-op BackgroundPatternProvider, six exported
trampoline helpers in file_versions.go, dead parentPath/baseName
in mount/internal/afsfs/fs.go, unused Capabilities type alias,
codex-settings-migration.md (superseded by skill).
- UI deps: convert static jszip import to dynamic await import so
it ships as its own ~96 KB chunk on user action; migrate
lucide-icons.tsx to the canonical @redis-ui/styles name; clear
17 auto-fixable lint errors (baseline 44 -> 27).
- UI helpers: add foundation/sort-compare.ts (replaces six
compareValues copies) and foundation/clipboard-icons.tsx
(replaces two CopyIcon/CheckIcon pairs).
- Renames: cmd/afs/sync_reconcile.go -> sync_full_reconciler.go
and sync_reconciler.go -> sync_event_reconciler.go (plus its
test) so the two halves of the sync pipeline are no longer
confusable neighbors.
- CI: add .github/workflows/ci.yml with parallel jobs for the
root Go module, mount module (with redis-server), sandbox
module, UI build/test, and a non-blocking ui-lint job tracking
the 27 baseline lint errors.
- Sandbox hardening: default --bind to 127.0.0.1, add a
threat-model docstring at the top of cmd/sandbox/main.go, and
log a WARNING at startup if bound externally.
- FUSE correctness fix: mount/internal/afsfs/{handle,file}.go
now route writes and truncates through WriteInodeAtPath /
TruncateInodeAtPath. The legacy no-path entry points wiped
the entire client attribute cache on every FUSE write via
finishRangeWriteCache's invalidatePrefix("/"). NFS already
used the *AtPath variants and was unaffected.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
🛡️ Jit Security Scan Results
✅ No security findings were detected in this PR
Security scan by Jit
|
Type-system mechanical fixes across 12 UI files: drop unnecessary optional chains and ?? fallbacks where TypeScript already guarantees non-null, plus a few small structural fixes. - workspace-table.tsx: pass explicit type param to useStoredViewMode so viewMode is correctly typed as "table" | "cards" rather than the "table" literal it was inferring from the fallback. Clears 6 always-true/false viewMode comparisons. - CreateWorkspaceDialog.tsx: declare preferredDatabase return type explicitly with an early-return on empty list so callers see T | null cleanly. Drop a redundant && startTemplate check now that the startedFromTemplate intermediate is inlined. - database-scope.tsx: drop a dead error != null branch (after the instanceof Error check, the only remaining type is null). - auth-context.tsx: drop redundant ?. on the non-nullable subject field; wrap the auth-config useQuery in queryOptions() to satisfy the @tanstack/query/prefer-query-options rule. - templates.installed.\$workspaceId.tsx: explicit length check instead of foo[0] ?? null so primaryToken's null-or-token type is honest. - lucide-icons, global-drawer, getting-started-onboarding-dialog, agents-table, api/afs.ts, lib/snippets.ts: drop unnecessary ?. and ?? in spots TypeScript already proved non-null. CI: drop continue-on-error from the ui-lint job. Lint now blocks PRs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The sandbox HTTP server runs untrusted shell commands and had several unbounded resources noted in the pre-review pass. - Add ReadHeaderTimeout, ReadTimeout, IdleTimeout to the http.Server. WriteTimeout is intentionally not set: the wait endpoints stream arbitrarily-long-running process completion. - New boundedBuffer caps each process's retained stdout/stderr at 1 MiB so a chatty process can't OOM the sandbox. Bytes past the cap are dropped and a "[output truncated: ...]" marker is appended. - New Manager.GC(maxAge) prunes processes in StateExited/StateKilled/ StateTimedOut whose EndedAt is older than maxAge. main.go runs it every 5 minutes with a 1h TTL, so terminal-state entries no longer accumulate for the lifetime of the sandbox. - Replace cwd[0] != '/' with strings.HasPrefix(cwd, "/"). The pre-existing cwd == "" guard makes this functionally equivalent today but the new form is robust if the order ever changes. - Add buffer_test.go and manager_test.go (4 + 2 tests). Sandbox had zero tests before this change; this seeds the package with coverage for the new behavior plus the existing process lifecycle. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace 60+ ad-hoc errors.New("...") sites in the mount client with a
small set of exported sentinels in mount/internal/client/errors.go,
and rewrite mount/internal/afsfs/errors.go's mapError to check
errors.Is first and fall back to substring matching only for
externally-formatted messages (Redis "ERR ..." replies, redis-go
errors, etc).
The previous implementation relied entirely on strings.Contains over
err.Error(), which silently broke as soon as a caller wrapped the
error with extra context — a fmt.Errorf("inode %d: %w", id,
client.ErrNotFile) would map to EIO instead of EISDIR. With sentinels
the mapping survives wrapping.
Sentinels:
- ErrNotFound, ErrNotFile, ErrNotDir, ErrNotSymlink, ErrAlreadyExists,
ErrDirNotEmpty, ErrUnsupported, ErrCannotWriteRoot, ErrCannotMoveRoot,
ErrCannotRemoveRoot, ErrParentConflict
- ErrLockWouldBlock now exported (was unexported errLockWouldBlock).
mount/internal/afsfs/handle.go drops two literal err.Error() == "lock
would block" comparisons; mapError handles EAGAIN via errors.Is.
- mount/internal/nfsfs/fs.go replaces strings.Contains(err.Error(),
"already exists") with errors.Is(err, client.ErrAlreadyExists).
The substring fallback in mapError is intentionally retained as a
safety net for the long tail of less-common errors that don't yet
have sentinels and for errors that genuinely come from outside the
client package. New TestMapErrorSentinels exercises both bare and
wrapped sentinel paths.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The local CLI MCP server (cmd/afs/afs_mcp.go) and the hosted control-plane MCP surface (internal/controlplane/mcp_hosted*.go) each carried byte-identical copies of ~21 helper functions and 6 shared types: text-patch application, single-match search, line splitting, args parsing for required/optional/typed values, JSON round-trip decoding, etc. Diffing every candidate against its twin showed all 21 functions and 4 of the 6 types were exactly the same; the only divergence was an unused Workspace field on the cmd/afs FilePatchInput. This change moves the canonical implementations to a new package internal/mcptools, then trims each MCP file to its surface-specific logic. The package-local mcp* names are preserved as type aliases (`type mcpFooBar = mcptools.FooBar`) and function-value aliases (`var mcpFooBar = mcptools.FooBar`) so existing call sites in both packages keep working unchanged. A follow-up phase can drop the aliases and rename call sites for full directness. Net file shrinkage: - cmd/afs/afs_mcp.go: 2872 -> 2490 (-382) - internal/controlplane/mcp_hosted.go: 1853 -> 1753 (-100) - internal/controlplane/mcp_hosted_helpers: 642 -> 349 (-293) The new internal/mcptools/mcptools.go is 501 lines plus 161 lines of unit tests (TestRequiredString, OptionalInt coercion across float64/int/int64, OptionalStringSlice for both shapes, SplitTextLines round-trip, ApplyTextPatch replace/insert/delete, TextSHA256 stability). Sandbox-/controlplane-side tests still exercise these helpers transitively; the new tests pin the extracted unit behavior directly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Captures the cleanup items identified during the pre-review pass on PR #13 but deliberately deferred from that PR. Each entry includes a file:line anchor and a recommended approach so the next session can pick up cold. Items are grouped by tier: - Tier A — quick wins under 1 hour: controlplane writeError sentinels, mount cache LRU bound, drop mcptools aliases, redis-go deprecations, per-database error swallowing, panic-on-config-path removals. - Tier B — medium refactors 1–3 hours: mcpDiffOperand* extraction, Redis coercion helper consolidation, use-afs.ts mutation factory, sandbox auth, sandbox tests for pre-existing code, controlplane type-alias cluster, boolean-flag parameter sweep. - Tier C — half-day surgical: Resolved/scoped collapse (~1500 LOC), file splits for the seven 2000+ LOC files, integration-test build tags, mount tests on miniredis, mount/client/ shim, native_* and afs_* prefix drops, MountedFS.bash() sync cost. Held-back item: format-toggle.tsx + lib/snippets.ts are listed as Phase 1 deliverables of the active cli-first-ui.md plan and need an explicit decision before they can be deleted. Co-Authored-By: Claude Opus 4.7 (1M context) <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.
Summary
Pre-review cleanup pass on the AFS codebase. 12 phases of low-risk
cleanup, plus a backlog plan for the larger refactors that were
deliberately deferred. The PR is mergeable from CI's side; remaining
items live in
plans/post-review-followups.md.Commits
Pre-review cleanup passClear UI lint baseline (27 -> 0 errors)Sandbox quick wins: timeouts, output cap, GC, first testsMount client: typed sentinels for filesystem errorsExtract shared MCP helpers to internal/mcptoolsAdd plans/post-review-followups.md backlogHighlights
WriteInodeAt/TruncateInodedefaultedpath == ""intoinvalidatePrefix("/"). Routed through*AtPathvariants. NFS path was already correct. Added regression test for the wrapped-error case so a futurefmt.Errorf("...: %w", ...)doesn't silently break the errno mapping.sh -c <command>with no auth) now defaults to binding127.0.0.1. Externally-bound runs print aWARNING:line at startup. Threat model is documented loudly at the top ofcmd/sandbox/main.go..github/workflows/ci.ymlruns all five surfaces in parallel; total run time is ~1m20s.internal/mcptools/package via Phase 12, with unit tests for the extracted helpers.fmt.Errorf("...: %w", client.ErrNotFile)) now map correctly to errnos; the substring fallback is retained as defense-in-depth for messages that arrive from outside the package (Redis "ERR ..." replies, etc.).continue-on-error: true.boundedBufferand process-mapGC. The pre-existingLaunch/Read/Wait/Killflow is still uncovered (tracked in the follow-up plan).Stats
go-root,go-mount,go-sandbox,ui,ui-lint).Out of scope (tracked for follow-up)
See
plans/post-review-followups.mdfor the full list with file:line anchors. Major remaining items:Resolved/scoped collapse indatabase_manager.go+http.go(~1500 LOC of avoidable duplication).afs.ts,database_manager.go,service.go,http.go, etc.).writeErrorsentinel migration (mount side done in this PR).redis-servertominiredis.@redislabsdev/*package aliases inui/package.jsonwere redundant. They are not — upstream@redis-ui/componentsinternals import@redislabsdev/redis-ui-stylesdirectly, so the aliases are load-bearing shims.Test plan
go-root,go-mount,go-sandbox,ui,ui-lintmake testpassescd mount && go test ./...passescd sandbox && go test ./...passescd ui && npm run build && npm test && npm run lintall cleanTestMapErrorSentinelsregression case for the wrapped-error pathcd ui && npm run dev(background pattern wrapper was a no-op, so no visual change expected)cd sandbox && go run ./cmd/sandboxshould bind127.0.0.1:8090by default; with--bind 0.0.0.0should print the WARNING line🤖 Generated with Claude Code