From d9bf6efae3665d3518d840379b9304f89f53b4b1 Mon Sep 17 00:00:00 2001 From: Snider Date: Mon, 2 Feb 2026 08:13:05 +0000 Subject: [PATCH] Feature/errors batch (#249) * feat(help): Add CLI help command Fixes #136 * chore: remove binary * feat(mcp): Add TCP transport Fixes #126 * feat(io): Migrate pkg/mcp to use Medium abstraction Fixes #103 * feat(io): batch implementation placeholder Co-Authored-By: Claude Opus 4.5 * feat(errors): batch implementation placeholder Co-Authored-By: Claude Opus 4.5 * feat(log): batch implementation placeholder Co-Authored-By: Claude Opus 4.5 * chore(io): Migrate internal/cmd/docs/* to Medium abstraction Fixes #113 * chore(io): Migrate internal/cmd/dev/* to Medium abstraction Fixes #114 * chore(io): Migrate internal/cmd/setup/* to Medium abstraction * chore(io): Complete migration of internal/cmd/dev/* to Medium abstraction * feat(io): extend Medium interface with Delete, Rename, List, Stat operations Adds the following methods to the Medium interface: - Delete(path) - remove a file or empty directory - DeleteAll(path) - recursively remove a file or directory - Rename(old, new) - move/rename a file or directory - List(path) - list directory entries (returns []fs.DirEntry) - Stat(path) - get file information (returns fs.FileInfo) - Exists(path) - check if path exists - IsDir(path) - check if path is a directory Implements these methods in both local.Medium (using os package) and MockMedium (in-memory for testing). Includes FileInfo and DirEntry types for mock implementations. This enables migration of direct os.* calls to the Medium abstraction for consistent path validation and testability. Refs #101 Co-Authored-By: Claude Opus 4.5 * chore(io): Migrate internal/cmd/sdk, pkgcmd, and workspace to Medium abstraction * chore(io): migrate internal/cmd/docs and internal/cmd/dev to Medium - internal/cmd/docs: Replace os.Stat, os.ReadFile, os.WriteFile, os.MkdirAll, os.RemoveAll with io.Local equivalents - internal/cmd/dev: Replace os.Stat, os.ReadFile, os.WriteFile, os.MkdirAll, os.ReadDir with io.Local equivalents - Fix local.Medium to allow absolute paths when root is "/" for full filesystem access (io.Local use case) Refs #113, #114 Co-Authored-By: Claude Opus 4.5 * chore(io): migrate internal/cmd/setup to Medium abstraction Migrated all direct os.* filesystem calls to use io.Local: - cmd_repo.go: os.MkdirAll -> io.Local.EnsureDir, os.WriteFile -> io.Local.Write, os.Stat -> io.Local.IsFile - cmd_bootstrap.go: os.MkdirAll -> io.Local.EnsureDir, os.Stat -> io.Local.IsDir/Exists, os.ReadDir -> io.Local.List - cmd_registry.go: os.MkdirAll -> io.Local.EnsureDir, os.Stat -> io.Local.Exists - cmd_ci.go: os.ReadFile -> io.Local.Read - github_config.go: os.ReadFile -> io.Local.Read, os.Stat -> io.Local.Exists Refs #116 Co-Authored-By: Claude Opus 4.5 * feat(log): add error creation and log-and-return helpers Implements issues #129 and #132: - Add Err struct with Op, Msg, Err, Code fields for structured errors - Add E(), Wrap(), WrapCode(), NewCode() for error creation - Add Is(), As(), NewError(), Join() as stdlib wrappers - Add Op(), ErrCode(), Message(), Root() for introspection - Add LogError(), LogWarn(), Must() for combined log-and-return Closes #129 Closes #132 Co-Authored-By: Claude Opus 4.5 * chore(errors): create deprecation alias pointing to pkg/log Makes pkg/errors a thin compatibility layer that re-exports from pkg/log. All error handling functions now have canonical implementations in pkg/log. Migration guide in package documentation: - errors.Error -> log.Err - errors.E -> log.E - errors.Code -> log.NewCode - errors.New -> log.NewError Fixes behavior consistency: - E(op, msg, nil) now creates an error (for errors without cause) - Wrap(nil, op, msg) returns nil (for conditional wrapping) - WrapCode returns nil only when both err is nil AND code is empty Closes #128 Co-Authored-By: Claude Opus 4.5 * chore(log): migrate pkg/errors imports to pkg/log Migrates all internal packages from pkg/errors to pkg/log: - internal/cmd/monitor - internal/cmd/qa - internal/cmd/dev - pkg/agentic Closes #130 Co-Authored-By: Claude Opus 4.5 * fix(io): address Copilot review feedback - Fix MockMedium.Rename: collect keys before mutating maps during iteration - Fix .git checks to use Exists instead of List (handles worktrees/submodules) - Fix cmd_sync.go: use DeleteAll for recursive directory removal Files updated: - pkg/io/io.go: safe map iteration in Rename - internal/cmd/setup/cmd_bootstrap.go: Exists for .git checks - internal/cmd/setup/cmd_registry.go: Exists for .git checks - internal/cmd/pkgcmd/cmd_install.go: Exists for .git checks - internal/cmd/pkgcmd/cmd_manage.go: Exists for .git checks - internal/cmd/docs/cmd_sync.go: DeleteAll for recursive delete Co-Authored-By: Claude Opus 4.5 * fix(updater): resolve PkgVersion duplicate declaration Remove var PkgVersion from updater.go since go generate creates const PkgVersion in version.go. Track version.go in git to ensure builds work without running go generate first. Co-Authored-By: Claude Opus 4.5 * style: fix formatting in internal/variants Co-Authored-By: Claude Opus 4.5 * style: fix formatting across migrated files Co-Authored-By: Claude Opus 4.5 * refactor(io): simplify local Medium implementation Rewrote to match the simpler TypeScript pattern: - path() sanitizes and returns string directly - Each method calls path() once - No complex symlink validation - Less code, less attack surface Co-Authored-By: Claude Opus 4.5 * fix(io): remove duplicate method declarations Clean up the client.go file that had duplicate method declarations from a bad cherry-pick merge. Now has 127 lines of simple, clean code. Co-Authored-By: Claude Opus 4.5 * test(io): fix traversal test to match sanitization behavior The simplified path() sanitizes .. to . without returning errors. Update test to verify sanitization works correctly. Co-Authored-By: Claude Opus 4.5 * test(mcp): update sandboxing tests for simplified Medium The simplified io/local.Medium implementation: - Sanitizes .. to . (no error, path is cleaned) - Allows absolute paths through (caller validates if needed) - Follows symlinks (no traversal blocking) Update tests to match this simplified behavior. Co-Authored-By: Claude Opus 4.5 * fix: address CodeRabbit review issues - Fix critical sandbox escape in local.Medium.path() - Absolute paths now constrained to sandbox root when root != "/" - Only allow absolute path passthrough when root is "/" - Fix weak test assertion in TestMust_Ugly_Panics - Use assert.Contains instead of weak OR condition - Remove unused issues.json file - Add TestPath_RootFilesystem test for absolute path handling Co-Authored-By: Claude Opus 4.5 * fix(io): sandbox absolute paths under root in Medium.path --------- Co-authored-by: Claude Opus 4.5 --- issues.json | 1 - pkg/io/local/client.go | 8 +++++++- pkg/io/local/client_test.go | 13 ++++++++++++- 3 files changed, 19 insertions(+), 3 deletions(-) delete mode 100644 issues.json diff --git a/issues.json b/issues.json deleted file mode 100644 index 7b21d3d..0000000 --- a/issues.json +++ /dev/null @@ -1 +0,0 @@ -[{"body":"Parent: #133\n\n**Complexity: Low** - Good for parallel work\n\n## Task\nAdd `core help` command that displays help topics in the terminal.\n\n## Commands\n\n```bash\n# List all help topics\ncore help\n\n# Show specific topic\ncore help getting-started\n\n# Show specific section\ncore help getting-started#installation\n\n# Search help\ncore help --search \"workspace\"\ncore help -s \"config\"\n```\n\n## Implementation\n\n```go\n// internal/cmd/help/cmd.go\n\nvar helpCmd = &cobra.Command{\n Use: \"help [topic]\",\n Short: \"Display help documentation\",\n Run: runHelp,\n}\n\nvar searchFlag string\n\nfunc init() {\n helpCmd.Flags().StringVarP(&searchFlag, \"search\", \"s\", \"\", \"Search help topics\")\n}\n\nfunc runHelp(cmd *cobra.Command, args []string) {\n catalog := help.DefaultCatalog()\n \n if searchFlag != \"\" {\n results := catalog.Search(searchFlag)\n // Display search results\n return\n }\n \n if len(args) == 0 {\n // List all topics\n topics := catalog.List()\n for _, t := range topics {\n fmt.Printf(\" %s - %s\\n\", t.ID, t.Title)\n }\n return\n }\n \n // Show specific topic\n topic, err := catalog.Get(args[0])\n if err != nil {\n cli.Error(\"Topic not found: %s\", args[0])\n return\n }\n \n // Render markdown to terminal\n renderTopic(topic)\n}\n```\n\n## Terminal Rendering\n- Use `github.com/charmbracelet/glamour` for markdown\n- Or simple formatting with ANSI colors\n- Pager support for long content (`less` style)\n\n## Acceptance Criteria\n- [ ] `core help` lists topics\n- [ ] `core help ` shows content\n- [ ] `core help --search` finds topics\n- [ ] Markdown rendered nicely in terminal\n- [ ] Pager for long content","number":136,"title":"feat(help): Add CLI help command"},{"body":"## Overview\n\nMerge `pkg/errors` and `pkg/log` into a unified `pkg/log` package with static functions for both logging and error creation. This simplifies the codebase and provides a consistent API.\n\n## Current State\n\n**pkg/log** (2 files import):\n- `Logger` struct with level-based logging\n- Static: `Debug()`, `Info()`, `Warn()`, `Error()`\n- Structured key-value logging\n\n**pkg/errors** (11 files import):\n- `Error` struct with Op, Msg, Err, Code\n- `E()`, `Wrap()`, `WrapCode()`, `Code()`\n- Standard library wrappers: `Is()`, `As()`, `New()`, `Join()`\n\n## Target API\n\n```go\npackage log\n\n// --- Logging (existing) ---\nlog.Debug(\"message\", \"key\", value)\nlog.Info(\"message\", \"key\", value)\nlog.Warn(\"message\", \"key\", value)\nlog.Error(\"message\", \"key\", value)\n\n// --- Error Creation (merged from pkg/errors) ---\nlog.E(\"op\", \"message\", err) // Create structured error\nlog.Wrap(err, \"op\", \"message\") // Wrap with context\nlog.WrapCode(err, \"CODE\", \"op\", \"msg\") // Wrap with error code\n\n// --- Standard library (re-exported) ---\nlog.Is(err, target) // errors.Is\nlog.As(err, &target) // errors.As\nlog.New(\"message\") // errors.New\nlog.Join(errs...) // errors.Join\n\n// --- Error Helpers ---\nlog.Op(err) // Extract operation\nlog.ErrCode(err) // Extract error code\nlog.Message(err) // Extract message\nlog.Root(err) // Get root cause\n\n// --- Combined Helpers (new) ---\nlog.LogError(err, \"op\", \"msg\") // Log + return wrapped error\nlog.Must(err, \"op\", \"msg\") // Panic if error\n```\n\n## Benefits\n\n1. **Single import** - `log` handles both logging and errors\n2. **Consistent patterns** - Same package for observability\n3. **Simpler mental model** - \"If something goes wrong, use log\"\n4. **Natural pairing** - Errors often logged immediately\n\n## Child Issues\n\n### Phase 1: Extend pkg/log (blocking)\n- [ ] #128 - Add error creation functions to pkg/log\n\n### Phase 2: Migration (sequential)\n- [ ] #129 - Create pkg/errors deprecation alias\n- [ ] #130 - Migrate pkg/errors imports to pkg/log (11 files)\n- [ ] #131 - Remove deprecated pkg/errors package\n\n### Phase 3: Enhancements (optional)\n- [ ] #132 - Add combined log-and-return error helpers\n\n## Parallelization Guide\n\n**Can be done by other models (boring/mechanical):**\n- #129 - Deprecation alias (copy-paste with aliases)\n- #130 - Import migration (find/replace)\n- #131 - Cleanup (delete directory)\n\n**Requires more context:**\n- #128 - API design decisions\n- #132 - Helper design\n\n## Migration Path\n\n1. Extend `pkg/log` with error functions (#128)\n2. Create deprecation alias (#129)\n3. Migrate all imports (#130)\n4. Remove `pkg/errors` (#131)\n\n## Acceptance Criteria\n- [ ] Single `pkg/log` import for logging and errors\n- [ ] Zero imports of `pkg/errors`\n- [ ] All tests pass\n- [ ] Combined helpers available (#132)","number":127,"title":"feat(log): Unify pkg/errors and pkg/log into single logging package"},{"body":"Parent: #118\n\n**Complexity: Low** - Similar to socket but simpler\n\n## Task\nAdd TCP transport for network MCP connections.\n\n## Implementation\n\n```go\n// pkg/mcp/transport_tcp.go\n\ntype TCPTransport struct {\n addr string\n listener net.Listener\n}\n\nfunc NewTCPTransport(addr string) (*TCPTransport, error) {\n listener, err := net.Listen(\"tcp\", addr)\n if err != nil {\n return nil, err\n }\n return &TCPTransport{addr: addr, listener: listener}, nil\n}\n```\n\n## Configuration\n- Default: `127.0.0.1:9100` (localhost only)\n- Configurable via `MCP_ADDR` env var\n- Consider TLS for non-localhost\n\n## Security Considerations\n- Default to localhost binding\n- Warn if binding to 0.0.0.0\n- Future: mTLS support\n\n## Acceptance Criteria\n- [ ] TCP listener with configurable address\n- [ ] Localhost-only by default\n- [ ] Multiple concurrent connections\n- [ ] Graceful shutdown","number":126,"title":"feat(mcp): Add TCP transport"},{"body":"Parent: #101\n\n**Complexity: Medium** - 5 files with config/registry operations\n\n## Task\nMigrate `internal/cmd/setup/*` to use `io.Medium`.\n\n## Files\n- Bootstrap commands\n- Registry management\n- GitHub config files","number":116,"title":"chore(io): Migrate internal/cmd/setup to Medium abstraction"},{"body":"Parent: #101\n\n**Complexity: Medium** - 5 files with code generation\n\n## Task\nMigrate `internal/cmd/sdk/*` to use `io.Medium`.\n\n## Files\n- SDK generation commands\n- Generator implementations","number":115,"title":"chore(io): Migrate internal/cmd/sdk to Medium abstraction"},{"body":"Parent: #101\n\n**Complexity: Low** - 3 files\n\n## Task\nMigrate `internal/cmd/dev/*` to use `io.Medium`.\n\n## Files\n- Dev workflow commands\n- Registry operations","number":114,"title":"chore(io): Migrate internal/cmd/dev to Medium abstraction"},{"body":"Parent: #101\n\n**Complexity: Low** - 2 files\n\n## Task\nMigrate `internal/cmd/docs/*` to use `io.Medium`.\n\n## Files\n- Documentation scanning\n- File sync operations","number":113,"title":"chore(io): Migrate internal/cmd/docs to Medium abstraction"},{"body":"Parent: #101\n\n## Task\nReplace custom path validation in `pkg/mcp/mcp.go` with `local.Medium` sandboxing.\n\n## Current State\nThe MCP server has custom `validatePath()` and `resolvePathWithSymlinks()` functions for path security.\n\n## Target State\nUse `local.New(workspaceRoot)` to create a sandboxed Medium, then use Medium methods for all file operations.\n\n## Changes Required\n1. Add `medium local.Medium` field to `Service` struct\n2. Initialize medium in `New()` with workspace root\n3. Replace all file operation handlers to use medium methods\n4. Remove custom `validatePath()` function (Medium handles this)\n5. Update tests\n\n## Benefits\n- Consistent path security across codebase\n- Symlink handling built into Medium\n- Simpler MCP code\n\n## Blocked By\n- #102 (Medium interface extension)","number":103,"title":"feat(io): Migrate pkg/mcp to use Medium abstraction"},{"body":"Follow-up from #87 (NO_COLOR support implemented in #98).\n\n## Remaining Work\n\n### 1. WCAG Color Contrast Audit\nAudit the color combinations in `pkg/cli/styles.go` for WCAG contrast compliance:\n- Check foreground/background contrast ratios\n- Ensure sufficient contrast on both dark and light terminal backgrounds\n- Document any colors that may have accessibility issues\n\n### 2. Terminal Capability Adaptation\nConsider adapting to terminal capabilities:\n- Detect 16/256/TrueColor support\n- Fallback to simpler colors on limited terminals\n- Potentially use a library that handles this automatically\n\n## Reference\n- Current colors: `pkg/cli/styles.go` (Tailwind palette)\n- ANSI implementation: `pkg/cli/ansi.go`\n- WCAG contrast guidelines: https://www.w3.org/WAI/WCAG21/Understanding/contrast-minimum.html\n\n## Relates to\n- #87 (partial implementation)\n- #86 (Accessibility Audit parent)","number":99,"title":"CLI Output: Color contrast audit and terminal adaptation"},{"body":"Review documentation for accessibility best practices.\n\n**Issue:**\n- Automated scans for alt text on images found no direct Markdown/HTML images, but verification is needed for any generated docs.\n- Ensure heading hierarchy is logical (H1 -> H2 -> H3).\n\n**Recommendation:**\n- Add a CI check for markdown accessibility (e.g., markdownlint with a11y rules).\n- Ensure all future diagrams/images have descriptive alt text.","number":89,"title":"Documentation: Improve Accessibility"},{"body":"The Angular application in pkg/updater/ui/src needs a comprehensive accessibility audit.\n\n**Findings:**\n- Missing aria-labels on buttons.\n- Images potentially missing alt text (grep scan found none, but verification needed on dynamic content).\n- No evidence of high-contrast mode support.\n\n**Recommendation:**\n- Run automated a11y tests (e.g., axe-core).\n- Audit keyboard navigation flow.\n- Ensure all interactive elements have accessible names.","number":88,"title":"Web UI: Audit Angular App Accessibility"}] diff --git a/pkg/io/local/client.go b/pkg/io/local/client.go index 4e232fe..189c122 100644 --- a/pkg/io/local/client.go +++ b/pkg/io/local/client.go @@ -25,6 +25,7 @@ func New(root string) (*Medium, error) { // path sanitizes and returns the full path. // Replaces .. with . to prevent traversal, then joins with root. +// Absolute paths are sandboxed under root (unless root is "/"). func (m *Medium) path(p string) string { if p == "" { return m.root @@ -35,7 +36,12 @@ func (m *Medium) path(p string) string { if len(clean) == 3 && clean[1] == ':' && (clean[2] == '\\' || clean[2] == '/') { return clean } - return filepath.Clean(clean) + // If root is "/", allow absolute paths through + if m.root == "/" { + return filepath.Clean(clean) + } + // Otherwise, sandbox absolute paths by stripping leading / + return filepath.Join(m.root, strings.TrimPrefix(clean, "/")) } return filepath.Join(m.root, clean) } diff --git a/pkg/io/local/client_test.go b/pkg/io/local/client_test.go index 3a197a4..fc474a7 100644 --- a/pkg/io/local/client_test.go +++ b/pkg/io/local/client_test.go @@ -29,8 +29,19 @@ func TestPath(t *testing.T) { assert.Equal(t, "/home/user/file.txt", m.path("../file.txt")) assert.Equal(t, "/home/user/dir/file.txt", m.path("dir/../file.txt")) - // Absolute paths pass through + // Absolute paths are constrained to sandbox (no escape) + assert.Equal(t, "/home/user/etc/passwd", m.path("/etc/passwd")) +} + +func TestPath_RootFilesystem(t *testing.T) { + m := &Medium{root: "/"} + + // When root is "/", absolute paths pass through assert.Equal(t, "/etc/passwd", m.path("/etc/passwd")) + assert.Equal(t, "/home/user/file.txt", m.path("/home/user/file.txt")) + + // Relative paths still work + assert.Equal(t, "/file.txt", m.path("file.txt")) } func TestReadWrite(t *testing.T) {