# Tasks: Safety, Reliability & Idiomatic Refactor **Input**: Design documents from `/specs/001-safety-reliability-refactor/` **Prerequisites**: plan.md, spec.md, research.md, data-model.md, contracts/cli.md, contracts/packages.md, quickstart.md **Tests**: Included — the project constitution (Principle VI) mandates TDD. All new code follows Red-Green-Refactor. **Organization**: Tasks are grouped by user story to enable independent implementation and testing of each story. **Self-Containment**: Tasks will be executed by a DIFFERENT model than the one generating them. Every non-trivial task includes an indented **Context** block with existing code references, implementation patterns, key decisions, and acceptance signals. ## Format: `[ID] [P?] [Story] Description` - **[P]**: Can run in parallel (different files, no dependencies) - **[Story]**: Which user story this task belongs to (e.g., US1, US2, US3) - Include exact file paths in descriptions --- ## Phase 1: Setup (Shared Infrastructure) **Purpose**: Create new package directory structure and update Go module configuration **Phase Context**: - **Files modified in this phase**: `go.mod` (module definition, currently Go 1.18 with `miekg/dns`), new directories under repo root - **Key types/interfaces used**: None — this phase creates the skeleton - **Codebase conventions to follow**: Go standard project layout with sub-packages. Module name is `dns-helper` (from current `go.mod`). Build tags for platform-specific files use `//go:build` directive (Go 1.17+ format). - [x] T001 Create package directory structure per implementation plan **Context**: - **Target state**: The project currently has a flat structure with all `.go` files in the root `main` package. Create four new package directories: ``` resolver/ hostfile/ platform/ lockfile/ ``` - **Expected implementation**: Create the four empty directories at the repository root. No files yet — subsequent tasks populate them. - **Key decisions**: These package names were chosen in the plan (see plan.md "Project Structure" section). Each package has a single responsibility: `resolver` = DNS lookups, `hostfile` = hosts file read/parse/write, `platform` = OS-specific paths, `lockfile` = concurrency serialization. - **Acceptance signal**: All four directories exist at the repository root alongside the existing source files. - [x] T002 Update `go.mod` to Go 1.20 and add `golang.org/x/net v0.35.0` **Context**: - **Current go.mod**: ```go.mod module dns-helper go 1.18 require ( github.com/miekg/dns v1.1.59 // indirect golang.org/x/mod v0.16.0 // indirect golang.org/x/net v0.22.0 // indirect golang.org/x/sys v0.18.0 // indirect golang.org/x/tools v0.19.0 // indirect ) ``` - **Expected implementation**: Change `go 1.18` to `go 1.20`. Change `golang.org/x/net v0.22.0` to `golang.org/x/net v0.35.0`. Do NOT remove `miekg/dns` yet — the old code still imports it. Removal happens in Phase 8 (US6) after all old files are deleted. Run `go mod download` to fetch the updated dependency. - **Key decisions (R-006)**: `golang.org/x/net v0.35.0` is the LAST version with `go 1.18` directive (compatible with Go 1.20). Starting at `v0.36.0`, the module requires `go 1.23.0`. MUST pin to v0.35.0. - **Gotcha**: Do not run `go mod tidy` yet — it would break because old source files still import `miekg/dns`. Only run `go mod download`. - **Acceptance signal**: `go mod download` completes without errors. `go.mod` shows `go 1.20` and `golang.org/x/net v0.35.0`. --- ## Phase 2: Foundational (Blocking Prerequisites) **Purpose**: Core interfaces and packages that ALL user stories depend on **⚠️ CRITICAL**: No user story work can begin until this phase is complete **Phase Context**: - **Files modified in this phase**: New files in `hostfile/`, `platform/`, `lockfile/` packages - **Key types/interfaces used**: `os.FileInfo`, `os.FileMode`, `os.File` from Go stdlib - **Codebase conventions to follow**: All package functions return errors to caller (Constitution Principle IV). No `os.Exit()` outside `main()`. Use `fmt.Errorf("context: %w", err)` for error wrapping. Use Go 1.20 `//go:build` directive for build tags (not the legacy `// +build` comment). - [x] T003 [P] Create FileSystem interface and OS implementation in `hostfile/filesystem.go` **Context**: - **Implements**: The FileSystem abstraction from research R-007 for testability (Constitution Principle VI). All Host file I/O goes through this interface so tests can inject fakes. - **Target file state**: `hostfile/` directory exists (empty) after T001. This is the first file in the package. Package declaration: `package hostfile`. - **Expected implementation**: ```go package hostfile import "os" // FileSystem abstracts filesystem operations for testability. type FileSystem interface { ReadFile(path string) ([]byte, error) WriteFile(path string, data []byte, perm os.FileMode) error Stat(path string) (os.FileInfo, error) CreateTemp(dir, pattern string) (*os.File, error) Rename(oldpath, newpath string) error Remove(path string) error Chmod(path string, mode os.FileMode) error } // OSFileSystem implements FileSystem using real OS calls. type OSFileSystem struct{} func (OSFileSystem) ReadFile(path string) ([]byte, error) { return os.ReadFile(path) } func (OSFileSystem) WriteFile(path string, data []byte, perm os.FileMode) error { return os.WriteFile(path, data, perm) } func (OSFileSystem) Stat(path string) (os.FileInfo, error) { return os.Stat(path) } func (OSFileSystem) CreateTemp(dir, pattern string) (*os.File, error) { return os.CreateTemp(dir, pattern) } func (OSFileSystem) Rename(oldpath, newpath string) error { return os.Rename(oldpath, newpath) } func (OSFileSystem) Remove(path string) error { return os.Remove(path) } func (OSFileSystem) Chmod(path string, mode os.FileMode) error { return os.Chmod(path, mode) } ``` - **Key decisions (R-007)**: Production code uses `OSFileSystem{}`. Tests inject a fake struct that records calls and returns configurable errors. `Chmod` is included because atomic write must copy permissions from the original hosts file to the temp file before rename (R-002 step 2). - **Acceptance signal**: `go build ./hostfile/` compiles without errors. - [x] T004 [P] Create platform package with interface and implementations in `platform/` **Context**: - **Implements**: The platform contract from `contracts/packages.md`: ```go // GetHostsFilePath returns the absolute path to the system hosts file. // Returns an error if the file does not exist or cannot be accessed. func GetHostsFilePath() (string, error) ``` - **Target file state**: `platform/` directory exists (empty) after T001. Create four files: - `platform/platform.go` — package doc comment only (no interface needed; the contract is a single package-level function) - `platform/platform_windows.go` — `//go:build windows` - `platform/platform_linux.go` — `//go:build linux` - `platform/platform_darwin.go` — `//go:build darwin` - **Expected implementation for `platform/platform_windows.go`** (reference: current `fileoperations_windows.go`): ```go //go:build windows package platform import ( "fmt" "os" ) func GetHostsFilePath() (string, error) { var path string if val, ok := os.LookupEnv("SystemRoot"); ok { path = val + "\\System32\\drivers\\etc\\hosts" } else { path = "C:\\Windows\\System32\\drivers\\etc\\hosts" } if _, err := os.Stat(path); err != nil { if os.IsNotExist(err) { return "", fmt.Errorf("file does not exist: %s", path) } return "", fmt.Errorf("accessing hosts file: %w", err) } return path, nil } ``` - **Expected implementation for `platform/platform_linux.go` and `platform/platform_darwin.go`**: ```go //go:build linux // (or darwin) package platform import ( "fmt" "os" ) func GetHostsFilePath() (string, error) { path := "/etc/hosts" if _, err := os.Stat(path); err != nil { if os.IsNotExist(err) { return "", fmt.Errorf("file does not exist: %s", path) } return "", fmt.Errorf("accessing hosts file: %w", err) } return path, nil } ``` - **Key decisions**: Current code for Linux/macOS calls `term(err)` which invokes `os.Exit(1)` — this VIOLATES Constitution Principle IV. The new code MUST return errors to the caller. This is the core fix for spec FR-008. `platform/platform.go` contains only the package doc comment: `// Package platform provides the platform-specific hosts file path.` - **Gotcha**: Use `//go:build` directive (Go 1.17+ syntax), NOT the legacy `// +build` comment. Go 1.20 supports both but the new syntax is preferred. - **Acceptance signal**: `go build ./platform/` compiles without errors on the current OS. - [x] T005 [P] Create lockfile package in `lockfile/lockfile.go` **Context**: - **Implements**: The lockfile contract from `contracts/packages.md`: ```go type Lock interface { Release() error } func Acquire(dir string) (Lock, error) ``` - **Target file state**: `lockfile/` directory exists (empty) after T001. Package declaration: `package lockfile`. - **Expected implementation** (based on R-003): ```go package lockfile import ( "fmt" "os" "path/filepath" "strconv" "time" ) const ( lockFileName = ".dns-helper.lock" staleTimeout = 2 * time.Minute retryInterval = 200 * time.Millisecond maxWait = 2 * time.Second ) type fileLock struct { path string } func (l *fileLock) Release() error { return os.Remove(l.path) } func Acquire(dir string) (Lock, error) { lockPath := filepath.Join(dir, lockFileName) deadline := time.Now().Add(maxWait) for { f, err := os.OpenFile(lockPath, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0644) if err == nil { // Write PID for debugging (not used for staleness detection) fmt.Fprintf(f, "%d\n", os.Getpid()) f.Close() return &fileLock{path: lockPath}, nil } if !os.IsExist(err) { return nil, fmt.Errorf("creating lock file %s: %w", lockPath, err) } // Lock file exists — check staleness info, statErr := os.Stat(lockPath) if statErr == nil && time.Since(info.ModTime()) > staleTimeout { os.Remove(lockPath) continue // Retry after breaking stale lock } if time.Now().After(deadline) { return nil, fmt.Errorf("could not acquire lock file %s: another instance may be running", lockPath) } time.Sleep(retryInterval) } } ``` - **Key decisions (R-003)**: - `O_CREATE|O_EXCL` is atomic on all three platforms (Windows maps to `CREATE_NEW`). - Stale timeout 2 minutes — the lock is normally held <1 second (read-modify-write cycle only). - PID written for human debugging but NOT used for staleness logic (Windows doesn't support `Signal(0)` without `x/sys`). - Lock acquired AFTER DNS resolution per FR-016 to minimize lock hold time. - `Release()` is idempotent — `os.Remove` on an already-deleted file just returns an error that can be ignored. - **Gotcha**: `Release()` should not return error if the file is already gone — check for `os.IsNotExist` and suppress that specific error. - **Acceptance signal**: `go build ./lockfile/` compiles without errors. - [x] T006 [P] Create lockfile tests in `lockfile/lockfile_test.go` **Context**: - **Target file state**: `lockfile/lockfile.go` exists from T005 with `Acquire()` and `Lock.Release()`. - **Expected test cases** (from contracts/packages.md behavior contract): 1. **Acquire succeeds**: Call `Acquire(tempDir)` — returns a non-nil `Lock` and lock file exists on disk. 2. **Release deletes lock file**: After `Acquire`, call `Release()` — lock file no longer exists. 3. **Double acquire blocks then fails**: Acquire a lock, then attempt a second `Acquire` on the same dir — should fail with "another instance may be running" after timeout. 4. **Stale lock is broken**: Create a lock file manually with a ModTime >2 minutes in the past, then call `Acquire` — should succeed (stale lock broken). 5. **Release is idempotent**: Call `Release()` twice — second call should not return an error. - **Test pattern**: Use `t.TempDir()` for isolated test directories. Use `os.Chtimes()` to set stale modification times. - **Key decisions**: Keep max wait short in tests or provide a way to configure timeout. Since the constants are package-level, tests may need to accept the 2-second wait for the "double acquire" test case. - **Acceptance signal**: `go test ./lockfile/` passes all test cases. - [x] T007 [P] Create platform tests in `platform/platform_test.go` **Context**: - **Target file state**: `platform/` package exists from T004 with `GetHostsFilePath()` for each OS. - **Expected test cases** (from contracts/packages.md): 1. **Returns valid path on current OS**: Call `GetHostsFilePath()` — returns a non-empty string and nil error. 2. **Returned path exists**: Call `os.Stat()` on the returned path — file exists (test requires admin environment where hosts file is present). 3. **Error messages include path**: If we can simulate a missing file (difficult on real OS), verify the error mentions the path. This may be a documentation-only test note. - **Test pattern**: Since these are platform-specific functions using real OS paths, tests run against the real filesystem. Use `//go:build` tags to write OS-specific test expectations if needed. - **Key decisions**: Platform tests are inherently integration tests (they check the real hosts file location). On CI, ensure the hosts file exists at the expected location. On all three OSes, `/etc/hosts` (Linux/macOS) and `%SystemRoot%\System32\drivers\etc\hosts` (Windows) should exist by default. - **Acceptance signal**: `go test ./platform/` passes on the current OS. **Checkpoint**: Foundation ready — user story implementation can now begin --- ## Phase 3: User Story 1 — Hosts File Is Never Corrupted or Lost (Priority: P1) 🎯 MVP **Goal**: Implement atomic write (temp file + rename) with pre-write backup, managed block parsing, and corruption detection. The hosts file is never truncated, partially written, or lost. **Independent Test**: Run `go test ./hostfile/` — all tests for reading, parsing, corruption detection, atomic write, and backup lifecycle pass. **Phase Context**: - **Files modified in this phase**: `hostfile/hostfile.go` (types, Read, Write), `hostfile/managed.go` (block parsing, AddEntries), `hostfile/managed_test.go` (tests), `hostfile/hostfile_test.go` (tests) - **Key types/interfaces used**: ```go // From hostfile/filesystem.go (T003): type FileSystem interface { ReadFile(path string) ([]byte, error) WriteFile(path string, data []byte, perm os.FileMode) error Stat(path string) (os.FileInfo, error) CreateTemp(dir, pattern string) (*os.File, error) Rename(oldpath, newpath string) error Remove(path string) error Chmod(path string, mode os.FileMode) error } ``` - **Codebase conventions to follow**: Error wrapping with `fmt.Errorf("context: %w", err)`. Constants for managed block markers (see data-model.md). Pure functions for managed block operations (no I/O — accept `[]string`, return `[]string`). ### Tests for User Story 1 ⚠️ > **NOTE: Write these tests FIRST, ensure they FAIL before implementation (TDD Red phase)** - [x] T008 [P] [US1] Write managed block parsing tests in `hostfile/managed_test.go` **Context**: - **Depends on**: T003 (FileSystem interface exists so the package compiles). T009 (types must exist for tests to reference). - **TDD note**: Write tests first. They will FAIL (or not compile) until T012 implements the logic. If types from T009 don't exist yet, create minimal type stubs at the top of managed_test.go's test helpers or coordinate with T009 completion. - **Target file state**: `hostfile/managed_test.go` is a new file. Package: `package hostfile` (same package testing for access to unexported helpers if needed, or `package hostfile_test` for black-box — prefer black-box `package hostfile_test`). - **Test cases to cover** (from data-model.md validation rules and R-008): 1. **No managed block**: Input lines with no markers → `HasManagedBlock = false`, all lines in `PrefixContent`. 2. **Valid managed block**: Lines with both markers in correct order → `PrefixContent`, `ManagedContent`, `PostfixContent` split correctly. 3. **Start marker at line 0**: Start marker is the very first line → `PrefixContent` is empty, block parsed correctly. (Gotcha from R-008: must use sentinel, not zero-check.) 4. **Corruption: start without end**: → error containing "start marker found" and "no end marker". 5. **Corruption: end without start**: → error containing "end marker found" and "no start marker". 6. **Corruption: end before start**: → error containing "end marker" and "before start marker". 7. **Corruption: duplicate markers**: → error containing "duplicate". 8. **AddEntries: adds new entries**: Given existing managed content `["1.1.1.1\thost1"]` and new entries for host2 → returns combined content. 9. **AddEntries: replaces existing hostname**: Given existing entries for host1 and new entries for host1 → old entries removed, new ones added. 10. **AddEntries: deduplicates**: Given duplicate IP+hostname pairs → returns deduplicated list. - **Marker constants** (from data-model.md): ```go StartMarker = "# DNSHelper <<-> START CONFIG" EndMarker = "# DNSHelper <->> END CONFIG" ``` - **Acceptance signal**: Tests compile (with stubs) or fail with clear assertion errors. After T012 implementation, `go test ./hostfile/ -run TestManaged` passes. - [x] T009 [P] [US1] Write hostfile read/write tests in `hostfile/hostfile_test.go` **Context**: - **Depends on**: T003 (FileSystem interface for mocking). - **TDD note**: Write tests first. They will fail until T013/T014 implement Read/Write. Create a `fakeFileSystem` struct in the test file that implements `FileSystem` interface with configurable behavior: ```go type fakeFileSystem struct { files map[string][]byte // path → content statResults map[string]os.FileInfo statErrors map[string]error renameErr error removeErr error tempFiles []*os.File // track calls for assertions writeCalls []writeCall renameCalls []renameCall } ``` - **Test cases to cover** (from contracts/packages.md Write behavior + spec acceptance scenarios): 1. **Read parses file correctly**: Fake FS returns file content with managed block → HostsFile struct populated correctly. 2. **Read handles missing file**: Fake FS returns error from ReadFile → Read returns wrapped error. 3. **Write creates backup before writing**: After Write, verify backup file created in backupDir with pattern `hosts.bak.YYYYMMDD-XXXX`. 4. **Write uses temp file in same directory**: Verify CreateTemp called with `filepath.Dir(hostsPath)` as the dir argument. 5. **Write renames temp over hosts file**: Verify Rename called with (tempPath, hostsPath). 6. **Write preserves original permissions**: Verify Chmod called on temp file with original file's mode before rename. 7. **Write deletes backup on success**: After successful Write, verify backup file was removed. 8. **Write cleans up temp on failure**: If Rename fails, verify temp file is removed. 9. **Write skips if content unchanged**: HostsFile with `OriginalContent` matching assembled content → no write occurs. 10. **Write calls Sync before Close**: Verify file.Sync() is called (this may require a wrapper or accept as implementation detail). - **Acceptance signal**: Tests compile with fake FS and fail on assertion. After T013/T014, `go test ./hostfile/ -run TestHostfile` passes. ### Implementation for User Story 1 - [x] T010 [US1] Create HostsFile and DNSEntry types with Manager interface in `hostfile/hostfile.go` **Context**: - **Implements**: Types from `data-model.md` and Manager interface from `contracts/packages.md`: ```go type HostsFile struct { Path string OriginalContent []string PrefixContent []string ManagedContent []string PostfixContent []string HasManagedBlock bool } type DNSEntry struct { IP string Hostname string } type Manager interface { Read(path string) (*HostsFile, error) Write(hf *HostsFile, backupDir string) error } ``` - **Target file state**: `hostfile/filesystem.go` already exists from T003 with `FileSystem` interface and `OSFileSystem`. This file adds the domain types and the `Manager` implementation struct. - **Expected implementation**: ```go package hostfile const ( StartMarker = "# DNSHelper <<-> START CONFIG" EndMarker = "# DNSHelper <->> END CONFIG" ) // DNSEntry represents a single IP-to-hostname mapping. type DNSEntry struct { IP string Hostname string } // String returns the hosts file line representation: "IP\tHostname". func (e DNSEntry) String() string { return e.IP + "\t" + e.Hostname } // HostsFile represents the parsed content of a hosts file. type HostsFile struct { Path string OriginalContent []string PrefixContent []string ManagedContent []string PostfixContent []string HasManagedBlock bool } // Manager handles hosts file read and atomic write operations. type Manager struct { fs FileSystem } // NewManager creates a Manager with the given FileSystem implementation. func NewManager(fs FileSystem) *Manager { return &Manager{fs: fs} } ``` - **Key decisions**: `Manager` is a concrete struct (not interface) — the interface is defined for documentation/contract purposes. `NewManager` accepts `FileSystem` for dependency injection. `DNSEntry.String()` produces the tab-separated format used in the hosts file (see data-model.md: `\t`). - **Gotcha**: Do NOT implement `Read()` or `Write()` yet — just the types, constants, and constructor. Read/Write are implemented in T013 and T014. - **Acceptance signal**: `go build ./hostfile/` compiles. Tests from T008/T009 can now reference the types (though they'll still fail on logic). - [x] T011 [US1] Implement managed block parsing and corruption detection in `hostfile/managed.go` **Context**: - **Implements**: Managed block parsing that splits raw file lines into prefix/managed/postfix sections. Also implements corruption detection (R-008, FR-011). - **Target file state**: `hostfile/` package has `filesystem.go` (T003) and `hostfile.go` (T010) with types/constants. - **Expected implementation**: ```go package hostfile import ( "fmt" "strings" ) // ParseManagedBlock splits raw file lines into prefix, managed, and postfix sections. // Returns an error if the managed block markers are corrupt. func ParseManagedBlock(lines []string) (prefix, managed, postfix []string, hasManagedBlock bool, err error) { startIdx := -1 // sentinel: not found endIdx := -1 // sentinel: not found startCount := 0 endCount := 0 for i, line := range lines { if strings.Contains(line, "DNSHelper <<-> START CONFIG") { startCount++ if startIdx == -1 { startIdx = i } } if strings.Contains(line, "DNSHelper <->> END CONFIG") { endCount++ if endIdx == -1 { endIdx = i } } } // Corruption detection (R-008) if startCount > 1 || endCount > 1 { return nil, nil, nil, false, fmt.Errorf("managed block is corrupt: duplicate start/end markers found") } if startIdx >= 0 && endIdx < 0 { return nil, nil, nil, false, fmt.Errorf("managed block is corrupt: start marker found at line %d but no end marker", startIdx+1) } if endIdx >= 0 && startIdx < 0 { return nil, nil, nil, false, fmt.Errorf("managed block is corrupt: end marker found at line %d but no start marker", endIdx+1) } if startIdx >= 0 && endIdx >= 0 && endIdx < startIdx { return nil, nil, nil, false, fmt.Errorf("managed block is corrupt: end marker (line %d) appears before start marker (line %d)", endIdx+1, startIdx+1) } if startIdx < 0 && endIdx < 0 { // No managed block return lines, nil, nil, false, nil } // Valid managed block prefix = lines[:startIdx] managed = lines[startIdx+1 : endIdx] postfix = lines[endIdx+1:] return prefix, managed, postfix, true, nil } ``` - **Key decisions (R-008)**: - Uses sentinel value `-1` (not zero) for "not found" because the start marker CAN be at line 0. The current code uses `startIndex == 0` to mean "not found" — this is a BUG being fixed. - Four corruption scenarios detected: start without end, end without start, end before start, duplicate markers. - Error messages include line numbers (1-based for human readability). - Scans ALL lines to detect duplicates before deciding on corruption. - **Gotcha**: The current code's `extractLinesToEdit` in `dataprep.go` (lines 14-51) has the zero-index bug. The new implementation must NOT replicate this. Use `-1` sentinel. - **Acceptance signal**: `go test ./hostfile/ -run TestParseManagedBlock` passes (corruption scenarios and valid parsing). - [x] T012 [US1] Implement AddEntries and content assembly in `hostfile/managed.go` **Context**: - **Implements**: The `AddEntries` function from `contracts/packages.md`: ```go // AddEntries adds DNS entries to the managed content, removing any // existing entries for the same hostnames first. Returns updated content. func AddEntries(existing []string, entries []DNSEntry) []string ``` - **Target file state**: `hostfile/managed.go` exists from T011 with `ParseManagedBlock`. Add `AddEntries` and `AssembleContent` to the same file. - **Expected implementation**: ```go // AddEntries removes existing entries for the same hostnames as the new entries, // then appends the new entries. Returns deduplicated managed content lines. func AddEntries(existing []string, entries []DNSEntry) []string { // Collect hostnames being added hostnames := make(map[string]bool) for _, e := range entries { hostnames[e.Hostname] = true } // Keep existing lines that don't match any new hostname var result []string for _, line := range existing { keep := true for h := range hostnames { if strings.Contains(line, h) { keep = false break } } if keep { result = append(result, line) } } // Append new entries for _, e := range entries { result = append(result, e.String()) } // Deduplicate return deduplicateLines(result) } // AssembleContent builds the full file content from sections. // If managedContent is empty, the managed block (markers included) is omitted. func AssembleContent(prefix, managed, postfix []string) []string { var result []string result = append(result, prefix...) if len(managed) > 0 { result = append(result, StartMarker) result = append(result, managed...) result = append(result, EndMarker) } result = append(result, postfix...) return result } // deduplicateLines removes duplicate lines preserving order. func deduplicateLines(lines []string) []string { seen := make(map[string]bool) var result []string for _, line := range lines { if !seen[line] { seen[line] = true result = append(result, line) } } return result } ``` - **Key decisions (data-model.md)**: - AddEntries first removes existing entries for the SAME hostnames, then adds new entries. This prevents stale IPs from lingering. - Deduplication is by full line content (IP + hostname pair), matching the existing `dedupeSlice` pattern in `dataprep.go:107-117`. - `AssembleContent` omits the managed block entirely when there are zero managed entries (per `data-model.md` ManagedBlock rules: "When all entries are removed, the entire block markers included is removed"). - **Acceptance signal**: `go test ./hostfile/ -run TestAddEntries` and `go test ./hostfile/ -run TestAssembleContent` pass. - [x] T013 [US1] Implement Read function in `hostfile/hostfile.go` **Context**: - **Implements**: The `Read` method on `Manager` from `contracts/packages.md`: ```go // Read reads and parses the hosts file at the given path. // Returns an error if the file cannot be read or the managed block is corrupt. func (m *Manager) Read(path string) (*HostsFile, error) ``` - **Target file state**: `hostfile/hostfile.go` has types (T010). `hostfile/managed.go` has `ParseManagedBlock` (T011). Now add the `Read` method to `Manager`. - **Expected implementation**: ```go func (m *Manager) Read(path string) (*HostsFile, error) { data, err := m.fs.ReadFile(path) if err != nil { return nil, fmt.Errorf("reading hosts file %s: %w", path, err) } lines := strings.Split(string(data), "\n") // Remove trailing empty line from Split if file ends with newline if len(lines) > 0 && lines[len(lines)-1] == "" { lines = lines[:len(lines)-1] } // Trim \r from each line for Windows \r\n line endings for i, line := range lines { lines[i] = strings.TrimRight(line, "\r") } prefix, managed, postfix, hasBlock, err := ParseManagedBlock(lines) if err != nil { return nil, err } return &HostsFile{ Path: path, OriginalContent: lines, PrefixContent: prefix, ManagedContent: managed, PostfixContent: postfix, HasManagedBlock: hasBlock, }, nil } ``` - **Key decisions**: Uses `FileSystem.ReadFile` (not direct `os.ReadFile`) for testability. Splits on `\n` and handles trailing newline. Trims `\r` from each line to handle Windows `\r\n` line endings. Error messages include the file path for diagnostics. - **Gotcha**: The `\r` trim loop MUST be included — without it, all managed block marker matching and hostname matching will fail on Windows because lines will have a trailing `\r`. The current codebase uses `bufio.Scanner` which handles this automatically, but `strings.Split` does not. - **Acceptance signal**: `go test ./hostfile/ -run TestRead` passes — correctly parses files with and without managed blocks. - [x] T014 [US1] Implement atomic Write with backup in `hostfile/hostfile.go` **Context**: - **Implements**: The `Write` method on `Manager` from `contracts/packages.md`: ```go // Write atomically writes the hosts file content to the given path. // Creates a backup before writing. Uses temp-file-and-rename pattern. // backupDir is the directory for the backup file (typically the exe directory). func (m *Manager) Write(hf *HostsFile, backupDir string) error ``` - **Target file state**: `hostfile/hostfile.go` has types (T010) and `Read` (T013). Now add `Write` method. - **Expected implementation** (from R-002): ```go func (m *Manager) Write(hf *HostsFile, backupDir string) error { // 1. Assemble new content newContent := AssembleContent(hf.PrefixContent, hf.ManagedContent, hf.PostfixContent) // 2. Skip write if content unchanged if slicesEqual(newContent, hf.OriginalContent) { return nil } // 3. Build output bytes var buf strings.Builder for _, line := range newContent { buf.WriteString(line) buf.WriteString("\n") } output := []byte(buf.String()) // 4. Get original file permissions info, err := m.fs.Stat(hf.Path) if err != nil { return fmt.Errorf("checking hosts file permissions: %w", err) } originalMode := info.Mode() // 5. Create backup backupPath, err := m.createBackup(hf.Path, backupDir) if err != nil { return fmt.Errorf("creating backup: %w", err) } // 6. Write to temp file in same directory as hosts file success := false tempFile, err := m.fs.CreateTemp(filepath.Dir(hf.Path), ".dns-helper-tmp-*") if err != nil { m.fs.Remove(backupPath) return fmt.Errorf("creating temp file: %w", err) } tempPath := tempFile.Name() defer func() { if !success { m.fs.Remove(tempPath) m.fs.Remove(backupPath) } }() if _, err := tempFile.Write(output); err != nil { tempFile.Close() return fmt.Errorf("writing temp file: %w", err) } if err := tempFile.Sync(); err != nil { tempFile.Close() return fmt.Errorf("syncing temp file: %w", err) } tempFile.Close() // 7. Set permissions to match original if err := m.fs.Chmod(tempPath, originalMode); err != nil { return fmt.Errorf("setting temp file permissions: %w", err) } // 8. Atomic rename (with retry on Windows) if err := m.renameWithRetry(tempPath, hf.Path); err != nil { return fmt.Errorf("renaming temp file to hosts file: %w", err) } // 9. Success — delete backup success = true m.fs.Remove(backupPath) return nil } ``` - **Helper functions to include in same file**: - `createBackup(hostsPath, backupDir string) (string, error)` — copies hosts file to `/hosts.bak.-<4hex>` using `ReadFile` + `WriteFile`. Generate random suffix with `crypto/rand` or `math/rand`. - `renameWithRetry(src, dst string) error` — calls `m.fs.Rename`, retries 2-3 times with 200ms delay on failure (for Windows sharing violations per R-002 step 6). - `slicesEqual(a, b []string) bool` — compares two string slices. - **Key decisions (R-002)**: - Temp file MUST be in the same directory as hosts file to avoid cross-volume rename failures (`EXDEV` on Unix). - `Sync()` before `Close()` for durability. - `Close()` before `Rename()` — required on Windows. - Backup uses `ReadFile` + `WriteFile` (copy, not rename) because backup dir is different from hosts dir. - Backup naming: `hosts.bak.YYYYMMDD-XXXX` where XXXX is 4 hex chars (per data-model.md `BackupFile` entity). - On success: delete backup. On failure: delete both temp and backup (original was never touched). - **Gotcha**: The `defer` cleanup MUST check the `success` flag. If rename succeeds, we must NOT delete the temp file (it IS the new hosts file now). The defer should only clean up temp and backup on failure paths. - **Acceptance signal**: `go test ./hostfile/ -run TestWrite` passes — backup created, temp file used, rename executed, backup deleted on success, cleanup on failure. **Checkpoint**: At this point, User Story 1 should be fully functional and testable independently. `go test ./hostfile/` passes all tests. --- ## Phase 4: User Story 2 — Correct DNS Resolution Results (Priority: P1) **Goal**: Replace `miekg/dns` with `golang.org/x/net/dns/dnsmessage`. Fix CNAME chain duplicate IP bug. Deduplicate resolved IPs. Support A record and CNAME chain queries. **Independent Test**: Run `go test ./resolver/` — all tests for DNS resolution, CNAME following, dedup, and error handling pass. **Phase Context**: - **Files modified in this phase**: `resolver/resolver.go` (interface + implementation), `resolver/resolver_test.go` (tests) - **Key types/interfaces used**: ```go // From contracts/packages.md: type Resolver interface { LookupIP(hostname, server string) ([]string, error) } ``` ```go // From golang.org/x/net/dns/dnsmessage: type Message struct { Header Header; Questions []Question; Answers []Resource; ... } func (m *Message) Pack() ([]byte, error) func (m *Message) Unpack(msg []byte) error type AResource struct { A [4]byte } type CNAMEResource struct { CNAME Name } ``` - **Codebase conventions to follow**: Return errors with context (`fmt.Errorf`). Interfaces for testability. No direct `fmt.Println` for errors (the current `dns.go` prints errors directly — this is being fixed). ### Tests for User Story 2 ⚠️ > **NOTE: Write these tests FIRST, ensure they FAIL before implementation (TDD Red phase)** - [x] T015 [P] [US2] Write resolver tests in `resolver/resolver_test.go` **Context**: - **Depends on**: T016 (interface stubs must exist for tests to compile). - **TDD note**: Create the test file with all test cases. Tests will fail until T017 implements the logic. To make tests compile, coordinate with T016 to have the interface + struct stubs. - **Target file state**: `resolver/` directory is empty. Package: `package resolver_test` (black-box testing). - **Test strategy**: Since DNS resolution involves network I/O, tests should use a fake UDP server (start a `net.ListenPacket("udp", "127.0.0.1:0")` in each test) that returns crafted DNS responses. This avoids external DNS dependencies. - **Test cases to cover** (from contracts/packages.md + spec acceptance scenarios): 1. **A record success**: Fake server returns 2 A records → `LookupIP` returns `["1.1.1.1", "2.2.2.2"]`, nil error. 2. **CNAME chain to A records**: Fake server returns CNAME on first query, then A records on follow-up → returns correct IPs, no duplicates. 3. **CNAME depth limit**: Fake server returns 11 consecutive CNAMEs → returns error mentioning "CNAME chain depth". 4. **NXDOMAIN**: Fake server returns NXDOMAIN rcode → returns error mentioning hostname. 5. **Server timeout**: Use an address that won't respond (or close the connection) → returns error mentioning "timeout" or "unreachable". 6. **Deduplication**: Fake server returns duplicate A records → returns unique IPs only. 7. **FQDN normalization**: Hostname without trailing dot → query still works (trailing dot appended internally). 8. **Response ID mismatch**: Fake server returns response with different ID → returns error. - **Fake DNS server pattern**: ```go func startFakeDNS(t *testing.T, handler func([]byte) []byte) string { conn, err := net.ListenPacket("udp", "127.0.0.1:0") require.NoError(t, err) t.Cleanup(func() { conn.Close() }) go func() { buf := make([]byte, 1232) n, addr, err := conn.ReadFrom(buf) if err != nil { return } resp := handler(buf[:n]) conn.WriteTo(resp, addr) }() return conn.LocalAddr().String() } ``` Note: No external test dependencies — use `t.Fatalf` / `t.Errorf` instead of `require` (no testify). Build DNS messages using `dnsmessage.Message{}.Pack()`. - **Acceptance signal**: Tests compile with resolver stubs. After T017, `go test ./resolver/` passes. ### Implementation for User Story 2 - [x] T016 [US2] Create Resolver interface and DNSResolver struct in `resolver/resolver.go` **Context**: - **Implements**: Resolver interface from `contracts/packages.md`: ```go type Resolver interface { LookupIP(hostname, server string) ([]string, error) } ``` - **Target file state**: `resolver/` directory is empty after T001. This is the first file in the package. - **Expected implementation** (interface + struct + constructor only): ```go package resolver import "time" const ( defaultTimeout = 5 * time.Second maxCNAMEDepth = 10 udpBufSize = 1232 ) // Resolver performs DNS lookups against a specific server. type Resolver interface { LookupIP(hostname, server string) ([]string, error) } // DNSResolver implements Resolver using golang.org/x/net/dns/dnsmessage. type DNSResolver struct{} // New returns a new DNSResolver. func New() *DNSResolver { return &DNSResolver{} } ``` - **Key decisions (R-001)**: Constants match the contract: 5s timeout, max 10 CNAME depth, 1232-byte UDP buffer (standard DNS over UDP limit). - **Acceptance signal**: `go build ./resolver/` compiles. - [x] T017 [US2] Implement LookupIP with dnsmessage, CNAME chain, and dedup in `resolver/resolver.go` **Context**: - **Implements**: The full `LookupIP` method on `DNSResolver`. This replaces the current `lookupIP` function in `dns.go` which has two bugs: 1. **CNAME duplicate bug (R-005)**: The current code appends IPs back to themselves: `for _, ip := range ips { ips = append(ips, ip) }` — this doubles every IP. 2. **No CNAME depth limit**: Circular CNAMEs cause infinite recursion. - **Current buggy code in `dns.go`** (being replaced): ```go for _, ans := range r.Answer { if rec, ok := ans.(*dns.CNAME); ok { ips = lookupIP(rec.Target, resolver) for _, ip := range ips { ips = append(ips, ip) // BUG: doubles every IP } } } ``` - **Expected implementation** (complete, using Parser API from R-001): ```go import ( "fmt" "math/rand" "net" "strings" "time" "golang.org/x/net/dns/dnsmessage" ) func (r *DNSResolver) LookupIP(hostname, server string) ([]string, error) { return r.lookupIPWithDepth(hostname, server, 0) } func (r *DNSResolver) lookupIPWithDepth(hostname, server string, depth int) ([]string, error) { if depth > maxCNAMEDepth { return nil, fmt.Errorf("CNAME chain depth exceeded for %s (max %d)", hostname, maxCNAMEDepth) } // Ensure FQDN (trailing dot) fqdn := hostname if !strings.HasSuffix(fqdn, ".") { fqdn += "." } name, err := dnsmessage.NewName(fqdn) if err != nil { return nil, fmt.Errorf("invalid hostname %q: %w", hostname, err) } // Build A query with random ID id := uint16(rand.Uint32()) msg := dnsmessage.Message{ Header: dnsmessage.Header{ ID: id, RecursionDesired: true, }, Questions: []dnsmessage.Question{{ Name: name, Type: dnsmessage.TypeA, Class: dnsmessage.ClassINET, }}, } packed, err := msg.Pack() if err != nil { return nil, fmt.Errorf("packing DNS query: %w", err) } // Send over UDP addr := server if !strings.Contains(server, ":") { addr = server + ":53" } conn, err := net.DialTimeout("udp", addr, defaultTimeout) if err != nil { return nil, fmt.Errorf("connecting to DNS server %s: %w", server, err) } defer conn.Close() conn.SetDeadline(time.Now().Add(defaultTimeout)) if _, err := conn.Write(packed); err != nil { return nil, fmt.Errorf("sending DNS query to %s: %w", server, err) } buf := make([]byte, udpBufSize) n, err := conn.Read(buf) if err != nil { return nil, fmt.Errorf("reading DNS response from %s: %w", server, err) } // Parse response header using Parser API var parser dnsmessage.Parser respHeader, err := parser.Start(buf[:n]) if err != nil { return nil, fmt.Errorf("parsing DNS response: %w", err) } // Validate response ID if respHeader.ID != id { return nil, fmt.Errorf("DNS response ID mismatch (expected %d, got %d)", id, respHeader.ID) } // Check for truncated response (no TCP fallback) if respHeader.Truncated { return nil, fmt.Errorf("DNS response truncated for %s; TCP fallback not supported", hostname) } // Check for errors if respHeader.RCode != dnsmessage.RCodeSuccess { return nil, fmt.Errorf("DNS query for %s failed: %s", hostname, respHeader.RCode.String()) } // Skip questions section if err := parser.SkipAllQuestions(); err != nil { return nil, fmt.Errorf("parsing DNS response questions: %w", err) } // Collect A records and CNAME targets from answer section var ips []string var cnameTargets []string for { hdr, err := parser.AnswerHeader() if err == dnsmessage.ErrSectionDone { break } if err != nil { return nil, fmt.Errorf("parsing DNS answer header: %w", err) } switch hdr.Type { case dnsmessage.TypeA: r, err := parser.AResource() if err != nil { return nil, fmt.Errorf("parsing A record: %w", err) } ips = append(ips, net.IP(r.A[:]).String()) case dnsmessage.TypeCNAME: r, err := parser.CNAMEResource() if err != nil { return nil, fmt.Errorf("parsing CNAME record: %w", err) } cnameTargets = append(cnameTargets, r.CNAME.String()) default: if err := parser.SkipAnswer(); err != nil { return nil, fmt.Errorf("skipping DNS answer: %w", err) } } } // If no A records but CNAME targets exist, follow the chain if len(ips) == 0 && len(cnameTargets) > 0 { for _, target := range cnameTargets { // Remove trailing dot from CNAME target for display consistency targetHost := strings.TrimSuffix(target, ".") cnameIPs, err := r.lookupIPWithDepth(targetHost, server, depth+1) if err != nil { return nil, err } ips = append(ips, cnameIPs...) } } // Deduplicate IPs before returning seen := make(map[string]bool) var unique []string for _, ip := range ips { if !seen[ip] { seen[ip] = true unique = append(unique, ip) } } return unique, nil } ``` - **Key decisions (R-001)**: - Uses `Parser` API (not `Message.Unpack`) for streaming parse — more memory-efficient and the idiomatic `dnsmessage` approach. - Random query ID via `math/rand` (acceptable for a non-security-critical CLI tool). - Truncated response (TC flag) is treated as an error — no TCP fallback per the resolver behavior contract. - CNAME following: only follows if zero A records were returned (some servers return both A and CNAME in the same response). - `append(ips, cnameIPs...)` — NOT the buggy `for _, ip := range ips { ips = append(ips, ip) }`. - **Gotcha**: `dnsmessage.MustNewName` panics on invalid names — use `dnsmessage.NewName` which returns an error. - **Gotcha**: `parser.AResource()` / `parser.CNAMEResource()` MUST be called immediately after `parser.AnswerHeader()` for the matching type. Calling the wrong resource parser will produce garbled results. - **Gotcha**: CNAME target strings from `dnsmessage` include a trailing dot (FQDN). Strip it with `strings.TrimSuffix(target, ".")` before recursive call to avoid double-dot issues. - **Acceptance signal**: `go test ./resolver/` passes all test cases (A records, CNAME chains, depth limit, dedup, error cases). **Checkpoint**: At this point, User Stories 1 AND 2 should work independently. `go test ./hostfile/ ./resolver/` passes. --- ## Phase 5: User Story 3 — Reliable Deletion of Managed Entries (Priority: P2) **Goal**: Fix the multi-host deletion bug so that removing entries for specific hostnames correctly removes only those entries, and `delete all` removes the entire managed block. **Independent Test**: Run `go test ./hostfile/ -run TestRemove` — all deletion test cases pass, including multi-host scenarios. **Phase Context**: - **Files modified in this phase**: `hostfile/managed.go` (add RemoveByHostname, RemoveAll), `hostfile/managed_test.go` (add deletion tests) - **Key types/interfaces used**: ```go // From hostfile/managed.go (T011-T012): func ParseManagedBlock(lines []string) (prefix, managed, postfix []string, hasManagedBlock bool, err error) func AddEntries(existing []string, entries []DNSEntry) []string func AssembleContent(prefix, managed, postfix []string) []string ``` - **Codebase conventions to follow**: Same pure-function pattern as `AddEntries` — accept `[]string`, return `[]string`. No I/O in managed block functions. ### Tests for User Story 3 ⚠️ > **NOTE: Write these tests FIRST, ensure they FAIL before implementation (TDD Red phase)** - [x] T018 [P] [US3] Write deletion tests in `hostfile/managed_test.go` **Context**: - **Target file state**: `hostfile/managed_test.go` already exists from T008 with parsing and AddEntries tests. Append new test functions for deletion. - **Test cases to cover** (from spec US3 acceptance scenarios + R-004 bug analysis): 1. **RemoveByHostname — single host**: Managed content has entries for host1 and host2. Remove host1 → only host2 entries remain. 2. **RemoveByHostname — multiple hosts**: Managed content has entries for host1, host2, host3. Remove host1 and host2 → only host3 entries remain. 3. **RemoveByHostname — host not found**: Managed content has entries for host1. Remove host2 → content unchanged, no error. 4. **RemoveByHostname — all hosts removed**: Managed content has entries for host1 only. Remove host1 → empty slice returned (block will be omitted on write). 5. **RemoveAll**: Managed content has entries → returns empty slice. 6. **RemoveAll on empty**: Managed content is already empty → returns empty slice. 7. **Regression: multi-host deletion produces no duplicates**: This is the R-004 bug. Given `ExistingContent = ["1.1.1.1\thost1", "2.2.2.2\thost2", "3.3.3.3\thost3"]`, removing host1 and host2 should produce `["3.3.3.3\thost3"]` — NOT a duplicated or incomplete list. - **Current buggy code being replaced** (from `dataprep.go` lines 60-70): ```go func removeHostsFromExistingContent(data *workingData) { var updatedContent []string for _, host := range data.Hosts { // BUG: wrong loop order for _, line := range data.ExistingContent { if !strings.Contains(line, host) { updatedContent = append(updatedContent, line) } } } data.NewContent = updatedContent } ``` The bug: iterates hosts on the outside and re-scans original content each time, producing duplicates and failing to remove entries. See R-004 for detailed trace. - **Acceptance signal**: Tests compile and fail (TDD red). After T019, `go test ./hostfile/ -run TestRemove` passes. ### Implementation for User Story 3 - [x] T019 [US3] Implement RemoveByHostname in `hostfile/managed.go` **Context**: - **Implements**: `RemoveByHostname` from `contracts/packages.md`: ```go // RemoveByHostname removes all managed entries matching any of the // specified hostnames. Returns updated content. func RemoveByHostname(existing []string, hostnames []string) []string ``` - **Target file state**: `hostfile/managed.go` has `ParseManagedBlock`, `AddEntries`, `AssembleContent`, `deduplicateLines` from T011-T012. - **Expected implementation** (the CORRECT algorithm from R-004): ```go // RemoveByHostname removes all managed entries matching any of the // specified hostnames. Returns the remaining content. func RemoveByHostname(existing []string, hostnames []string) []string { var result []string for _, line := range existing { shouldRemove := false for _, host := range hostnames { if strings.Contains(line, host) { shouldRemove = true break } } if !shouldRemove { result = append(result, line) } } return result } ``` - **Key decisions (R-004)**: The critical fix is inverting the loop order — iterate LINES on the outside, check ALL hostnames on the inside. The buggy code iterated hostnames on the outside and re-scanned original content each pass, producing duplicates and failing to remove. - **Acceptance signal**: `go test ./hostfile/ -run TestRemoveByHostname` passes all 7 test cases from T018. - [x] T020 [US3] Implement RemoveAll in `hostfile/managed.go` **Context**: - **Implements**: `RemoveAll` from `contracts/packages.md`: ```go // RemoveAll returns an empty slice (clears all managed entries). func RemoveAll() []string ``` - **Target file state**: `hostfile/managed.go` has all previous functions. Add `RemoveAll`. - **Expected implementation**: ```go // RemoveAll returns nil, indicating all managed entries should be removed. // When AssembleContent receives nil/empty managed content, it omits the // managed block entirely (markers included). func RemoveAll() []string { return nil } ``` - **Key decisions (data-model.md)**: When all entries are removed, `AssembleContent` (T012) omits the entire managed block including markers. This is already handled by T012's `if len(managed) > 0` check. - **Acceptance signal**: `go test ./hostfile/ -run TestRemoveAll` passes. **Checkpoint**: At this point, User Stories 1, 2, AND 3 should all work independently. --- ## Phase 6: User Story 4 — Clear Error Messages and Input Validation (Priority: P2) **Goal**: Rewrite `main.go` to properly validate input, display clear error messages, integrate all packages, and handle partial DNS resolution failures. This is the orchestration layer that wires everything together. **Independent Test**: Build and run the binary with various invalid inputs (missing flags, empty values, unknown subcommands). Each should produce a descriptive error, exit non-zero, and not modify the hosts file. **Phase Context**: - **Files modified in this phase**: `main.go` (complete rewrite) - **Key types/interfaces used**: ```go // From hostfile package (Phase 3): func NewManager(fs FileSystem) *Manager func (m *Manager) Read(path string) (*HostsFile, error) func (m *Manager) Write(hf *HostsFile, backupDir string) error func AddEntries(existing []string, entries []DNSEntry) []string func RemoveByHostname(existing []string, hostnames []string) []string func RemoveAll() []string func AssembleContent(prefix, managed, postfix []string) []string // From resolver package (Phase 4): func New() *DNSResolver func (r *DNSResolver) LookupIP(hostname, server string) ([]string, error) // From platform package (Phase 2): func GetHostsFilePath() (string, error) // From lockfile package (Phase 2): func Acquire(dir string) (Lock, error) type Lock interface { Release() error } ``` - **Codebase conventions to follow**: Only `main()` calls `os.Exit()`. Errors printed to stderr (`fmt.Fprintf(os.Stderr, ...)`). Informational output to stdout. Banner prints on every invocation. See `contracts/cli.md` for exact output format. - [x] T021 [US4] Rewrite `main.go` with CLI parsing, validation, and full package integration **Context**: - **Replaces**: The entire current `main.go` (100 lines). The current code has multiple problems: - `init()` prints banner and calls `defer os.Exit(1)` on missing subcommand (non-idiomatic). - `term()` calls `os.Exit(1)` (violates Principle IV for code called from non-main). - No validation of flag values (empty `-host` or `-server` proceeds silently). - `writeHostsFile()` is called unconditionally on ALL paths including unknown subcommands (FR-005 bug). - Unused `modePing` FlagSet (dead code per FR-009). - **Current `main.go` structure** (being replaced entirely): ```go func init() { fmt.Println("DNSHelper v1.0") // ... if len(os.Args) < 2 { printUsage(); defer os.Exit(1) } } func term(err error) { fmt.Println("Error: " + err.Error()); defer os.Exit(1) } func main() { modePing := flag.NewFlagSet("ping", flag.ExitOnError) // DEAD CODE // ... switch on os.Args[1] ... err = writeHostsFile(data) // CALLED UNCONDITIONALLY — even on unknown subcommand } ``` - **Expected implementation** (following `contracts/cli.md` exactly): ```go package main import ( "flag" "fmt" "os" "path/filepath" "strings" "dns-helper/hostfile" "dns-helper/lockfile" "dns-helper/platform" "dns-helper/resolver" ) func main() { // Banner (always printed to stdout) fmt.Println("DNSHelper v1.0") fmt.Println("Copyright (c) 2024 Emberkom LLC") fmt.Println("") if len(os.Args) < 2 { printUsage() os.Exit(1) } switch strings.ToLower(os.Args[1]) { case "a", "add": os.Exit(runAdd(os.Args[2:])) case "d", "del", "delete": os.Exit(runDelete(os.Args[2:])) default: printUsage() os.Exit(1) } } func runAdd(args []string) int { fs := flag.NewFlagSet("add", flag.ContinueOnError) hostFlag := fs.String("host", "", "Comma separated list of hostnames to add") serverFlag := fs.String("server", "", "DNS resolver to use") if err := fs.Parse(args); err != nil { fmt.Fprintf(os.Stderr, "Error: %v\n", err) return 1 } // Validation (FR-006) if *hostFlag == "" { fmt.Fprintf(os.Stderr, "Error: -host flag is required for the add command\n") return 1 } if *serverFlag == "" { fmt.Fprintf(os.Stderr, "Error: -server flag is required for the add command\n") return 1 } hostnames := strings.Split(*hostFlag, ",") server := *serverFlag // Step 1: DNS resolution (before lock per FR-016) res := resolver.New() var entries []hostfile.DNSEntry var warnings []string for _, h := range hostnames { h = strings.TrimSpace(h) ips, err := res.LookupIP(h, server) if err != nil { warnings = append(warnings, fmt.Sprintf("Warning: failed to resolve %s: %v", h, err)) continue } for _, ip := range ips { entries = append(entries, hostfile.DNSEntry{IP: ip, Hostname: h}) } } if len(entries) == 0 && len(warnings) > 0 { for _, w := range warnings { fmt.Fprintln(os.Stderr, w) } return 1 } // Step 2: Get hosts file path hostsPath, err := platform.GetHostsFilePath() if err != nil { fmt.Fprintf(os.Stderr, "Error: %v\n", err) return 1 } // Step 3: Get executable directory (for lock + backup) exePath, err := os.Executable() if err != nil { fmt.Fprintf(os.Stderr, "Error: determining executable path: %v\n", err) return 1 } exeDir := filepath.Dir(exePath) // Step 4: Acquire lock (FR-016) lock, err := lockfile.Acquire(exeDir) if err != nil { fmt.Fprintf(os.Stderr, "Error: %v\n", err) return 1 } defer lock.Release() // Step 5: Read, modify, write mgr := hostfile.NewManager(hostfile.OSFileSystem{}) hf, err := mgr.Read(hostsPath) if err != nil { fmt.Fprintf(os.Stderr, "Error: %v\n", err) return 1 } hf.ManagedContent = hostfile.AddEntries(hf.ManagedContent, entries) if err := mgr.Write(hf, exeDir); err != nil { fmt.Fprintf(os.Stderr, "Error: %v\n", err) return 1 } // Step 6: Output summary (FR-013) entryCounts := make(map[string]int) for _, e := range entries { entryCounts[e.Hostname]++ } for _, h := range hostnames { h = strings.TrimSpace(h) if count, ok := entryCounts[h]; ok { fmt.Printf("Added %d entries for %s\n", count, h) } } for _, w := range warnings { fmt.Fprintln(os.Stderr, w) } if len(warnings) > 0 { return 1 // Partial failure (FR-015) } return 0 } func runDelete(args []string) int { // Check for "all" keyword first if len(args) > 0 && (strings.ToLower(args[0]) == "all" || strings.ToLower(args[0]) == "a") { return runDeleteAll() } fs := flag.NewFlagSet("delete", flag.ContinueOnError) hostFlag := fs.String("host", "", "Comma separated list of hostnames to delete") if err := fs.Parse(args); err != nil { fmt.Fprintf(os.Stderr, "Error: %v\n", err) return 1 } if *hostFlag == "" { fmt.Fprintf(os.Stderr, "Error: -host flag is required for the delete command\n") return 1 } hostnames := strings.Split(*hostFlag, ",") for i, h := range hostnames { hostnames[i] = strings.TrimSpace(h) } // Step 1: Get hosts file path hostsPath, err := platform.GetHostsFilePath() if err != nil { fmt.Fprintf(os.Stderr, "Error: %v\n", err) return 1 } // Step 2: Get executable directory (for lock + backup) exePath, err := os.Executable() if err != nil { fmt.Fprintf(os.Stderr, "Error: determining executable path: %v\n", err) return 1 } exeDir := filepath.Dir(exePath) // Step 3: Acquire lock lock, err := lockfile.Acquire(exeDir) if err != nil { fmt.Fprintf(os.Stderr, "Error: %v\n", err) return 1 } defer lock.Release() // Step 4: Read and parse hosts file mgr := hostfile.NewManager(hostfile.OSFileSystem{}) hf, err := mgr.Read(hostsPath) if err != nil { fmt.Fprintf(os.Stderr, "Error: %v\n", err) return 1 } // Step 5: Remove entries by hostname beforeLen := len(hf.ManagedContent) hf.ManagedContent = hostfile.RemoveByHostname(hf.ManagedContent, hostnames) afterLen := len(hf.ManagedContent) // Step 6: Write updated file if err := mgr.Write(hf, exeDir); err != nil { fmt.Fprintf(os.Stderr, "Error: %v\n", err) return 1 } // Step 7: Print summary removed := beforeLen - afterLen if removed > 0 { // Per-hostname summary: count entries containing each hostname for _, h := range hostnames { count := 0 // Count how many original managed lines contained this hostname for _, line := range hf.OriginalContent { if strings.Contains(line, h) { count++ } } if count > 0 { fmt.Printf("Removed %d entries for %s\n", count, h) } else { fmt.Printf("No managed entries found for %s\n", h) } } } else { for _, h := range hostnames { fmt.Printf("No managed entries found for %s\n", h) } } return 0 } func runDeleteAll() int { // Step 1: Get hosts file path hostsPath, err := platform.GetHostsFilePath() if err != nil { fmt.Fprintf(os.Stderr, "Error: %v\n", err) return 1 } // Step 2: Get executable directory exePath, err := os.Executable() if err != nil { fmt.Fprintf(os.Stderr, "Error: determining executable path: %v\n", err) return 1 } exeDir := filepath.Dir(exePath) // Step 3: Acquire lock lock, err := lockfile.Acquire(exeDir) if err != nil { fmt.Fprintf(os.Stderr, "Error: %v\n", err) return 1 } defer lock.Release() // Step 4: Read and parse hosts file mgr := hostfile.NewManager(hostfile.OSFileSystem{}) hf, err := mgr.Read(hostsPath) if err != nil { fmt.Fprintf(os.Stderr, "Error: %v\n", err) return 1 } // Step 5: Remove all managed entries entryCount := len(hf.ManagedContent) hf.ManagedContent = hostfile.RemoveAll() // Step 6: Write updated file if err := mgr.Write(hf, exeDir); err != nil { fmt.Fprintf(os.Stderr, "Error: %v\n", err) return 1 } // Step 7: Print summary if entryCount > 0 { fmt.Printf("Removed all managed entries (%d entries removed)\n", entryCount) } else { fmt.Println("No managed entries found") } return 0 } func printUsage() { fmt.Println("This utility will update the local hosts file with DNS entries obtained by the specified DNS server.") fmt.Println("Usage: dns-helper [add|delete] -host hostname [-server dns.example.com]") fmt.Println("Example:") fmt.Println(" dns-helper add -host xyz.acme.com -server dns.example.com") fmt.Println(" This will use dns.example.com to find and add all IP addresses for xyz.acme.com to the local hosts file.") fmt.Println(" dns-helper delete -host hostname.example.com") fmt.Println(" This will delete all entries for hostname.example.com from the local hosts file.") fmt.Println(" dns-helper delete all") fmt.Println(" This will delete all entries from the local hosts file that were added by this utility.") fmt.Println("Note: Adding a hostname will first remove all entries in the hosts file that match the same hostname.") fmt.Println(" This utility will only remove entries from the hosts file that it added.") } ``` - **Key decisions**: - `flag.ContinueOnError` instead of `flag.ExitOnError` — we handle errors ourselves (Constitution Principle IV). - DNS resolution happens BEFORE lock acquisition per FR-016. - `os.Executable()` + `filepath.Dir()` gets the exe directory for lock file and backup storage. - Error messages go to `os.Stderr`, informational output to `os.Stdout` (per CLI contract). - Partial failure (FR-015): if some hostnames resolve but others fail, write entries for the successful ones, print warnings, exit code 1. - **Gotcha**: The `delete all` subcommand uses a positional argument (`delete all`), not a flag. Must check `args[0]` before parsing flags. - **Gotcha**: When `delete -host` finds no matching entries, report "No managed entries found" but exit 0 (not an error per CLI contract). - **Acceptance signal**: `go build .` compiles. Manual testing of `dns-helper add -host ...`, `dns-helper delete -host ...`, `dns-helper delete all`, `dns-helper` (no args), and `dns-helper invalid` all produce expected output per `contracts/cli.md`. - [x] T022 [US4] Verify all CLI helpers compile and match `contracts/cli.md` output **Context**: - **Target file state**: `main.go` now contains complete implementations from T021: `main()`, `runAdd()`, `runDelete()`, `runDeleteAll()`, and `printUsage()`. - **What this task does**: T021 now includes the full code for `runDelete()`, `runDeleteAll()`, and `printUsage()`. This task verifies: 1. `go build .` compiles with all functions present. 2. The `printUsage()` output matches `contracts/cli.md` exactly. 3. `runDeleteAll()` acquires lock → reads → calls `hostfile.RemoveAll()` → writes → prints "Removed all managed entries (N entries removed)" or "No managed entries found". 4. `runDelete()` acquires lock → reads → calls `hostfile.RemoveByHostname()` → writes → prints per-hostname summary "Removed N entries for hostname" or "No managed entries found for hostname". 5. `delete -host` with no matching entries exits 0 (per cli.md: not an error). - **Acceptance signal**: All CLI paths work correctly per `contracts/cli.md` output examples. `go build .` succeeds. **Checkpoint**: At this point, User Stories 1-4 should all work. The tool is functionally complete. --- ## Phase 7: User Story 5 — Consistent Behavior Across Operating Systems (Priority: P3) **Goal**: Verify that all platform-specific code returns errors to callers (never calls `os.Exit`), and that error messages are descriptive and consistent across Windows, Linux, and macOS. **Independent Test**: Review platform package implementations. All error paths return `error` values with descriptive messages including the file path. No process-terminating calls exist outside `main()`. **Phase Context**: - **Files modified in this phase**: `platform/platform_windows.go`, `platform/platform_linux.go`, `platform/platform_darwin.go` (if adjustments needed), `platform/platform_test.go` - **Key types/interfaces used**: `func GetHostsFilePath() (string, error)` from T004 - **Codebase conventions to follow**: Error messages include the path that was checked. Use `fmt.Errorf` with `%w` for wrapping. No `os.Exit`, `log.Fatal`, or `term()` anywhere in the platform package. - [x] T023 [US5] Audit platform implementations for error consistency and fix if needed **Context**: - **Current state**: The platform package was created from scratch in T004 with correct error handling. This task verifies the implementations are consistent and adds any missing error detail. - **What to verify** (from spec US5 acceptance scenarios): 1. Each platform's `GetHostsFilePath()` returns `error` (never calls `os.Exit` or `log.Fatal`). 2. Error messages include the path that was checked (e.g., `"file does not exist: /etc/hosts"`). 3. Permission errors are wrapped with descriptive context. 4. All three implementations follow the same error message patterns. - **Current buggy code being replaced** (the OLD `fileoperations_linux.go` and `fileoperations_darwin.go`): ```go func getHostsFileLocation() (string, error) { hostsFileLocation = "/etc/hosts" exists, err := fileExists(hostsFileLocation) if err != nil { term(err) // BUG: calls os.Exit(1) instead of returning error } if exists == false { term(errors.New("hosts file does not exist at " + hostsFileLocation)) // BUG: same } return hostsFileLocation, nil } ``` - **Expected state after this task**: All three platform files use `os.Stat` + `fmt.Errorf` and return errors. No calls to `term()`, `os.Exit()`, `log.Fatal()`, or any process-terminating function. - **Acceptance signal**: `grep -r "os.Exit\|log.Fatal\|term(" platform/` returns no results. `go vet ./platform/` passes. - [x] T024 [P] [US5] Add cross-platform error message tests in `platform/platform_test.go` **Context**: - **Target file state**: `platform/platform_test.go` exists from T007 with basic happy-path tests. - **Tests to add**: 1. **Error message contains path**: If `GetHostsFilePath()` returns an error (e.g., on a system where hosts file is missing), the error string must contain a file path. This is hard to test on a real system where the hosts file exists. Consider a documentation-only test note, or a test that verifies the function signature returns `(string, error)`. 2. **No process termination**: This is verified by code review (T023), not a runtime test. - **Practical approach**: Since the platform package uses real OS paths and the hosts file exists on all CI environments, the primary value is verifying the happy path. Add a test that confirms `GetHostsFilePath()` returns a path that `os.Stat` can find. Corruption/missing file tests require mocking the OS, which the platform package doesn't support (it uses direct `os.Stat`). This is acceptable — the platform package is thin enough to verify by inspection. - **Acceptance signal**: `go test ./platform/` passes. **Checkpoint**: Platform behavior is consistent across all OSes. No platform code terminates the process directly. --- ## Phase 8: User Story 6 — Clean, Maintainable Project Structure (Priority: P3) **Goal**: Remove all old source files, dead code, and the `miekg/dns` dependency. Verify the project compiles cleanly with the new package structure and Go 1.20. **Independent Test**: `go build .` succeeds with only approved dependencies. `go vet ./...` passes. No dead code or unused imports remain. **Phase Context**: - **Files removed in this phase**: `dns.go`, `dataprep.go`, `fileoperations.go`, `fileoperations_windows.go`, `fileoperations_linux.go`, `fileoperations_darwin.go`, `structs.go` - **Files modified**: `go.mod`, `go.sum` (via `go mod tidy`) - **Codebase conventions to follow**: Run `go vet ./...` and `go build ./...` to verify cleanliness. No unused imports or variables. - [x] T025 [US6] Remove old source files from repository root **Context**: - **Files to delete** (all at repository root): - `dns.go` — old DNS resolution using `miekg/dns`. Replaced by `resolver/resolver.go`. - `dataprep.go` — old data preparation (parsing, removal, addition). Replaced by `hostfile/managed.go`. - `fileoperations.go` — old file read/write (includes the unsafe `writeOverContentInFile` with `O_WRONLY|O_TRUNC`). Replaced by `hostfile/hostfile.go`. - `fileoperations_windows.go` — old Windows platform code. Replaced by `platform/platform_windows.go`. - `fileoperations_linux.go` — old Linux platform code (calls `term()`). Replaced by `platform/platform_linux.go`. - `fileoperations_darwin.go` — old macOS platform code (calls `term()`). Replaced by `platform/platform_darwin.go`. - `structs.go` — old `workingData` struct with unused `IsNew` field. Replaced by types in `hostfile/hostfile.go`. - **Dead code being removed** (FR-009): - `modePing` FlagSet in old `main.go` (unused `ping` subcommand) — already removed in T021's rewrite. - `IsNew` field in `workingData` — removed with `structs.go`. - `fileExists` function in `fileoperations.go` — removed with file. - `slicesEqual` function in `dataprep.go` — reimplemented in `hostfile/hostfile.go` if needed, or removed if unused. - Commented-out debug code in `dataprep.go` lines 46-56 — removed with file. - **Expected implementation**: Delete all 7 files listed above. The new `main.go` (from T021-T022) imports only the new packages (`dns-helper/hostfile`, `dns-helper/resolver`, `dns-helper/platform`, `dns-helper/lockfile`). - **Gotcha**: After deleting these files, `go build .` should succeed because `main.go` no longer imports `miekg/dns` or references any of the old functions. If compilation fails, check that `main.go` doesn't reference any old function names. - **Acceptance signal**: All 7 old files deleted. `go build .` succeeds on the current OS. - [x] T026 [US6] Run `go mod tidy` to remove `miekg/dns` and transitive dependencies **Context**: - **Current go.mod dependencies** (after T002): ``` github.com/miekg/dns v1.1.59 golang.org/x/mod v0.16.0 golang.org/x/net v0.35.0 golang.org/x/sys v0.18.0 (or updated by x/net) golang.org/x/tools v0.19.0 ``` - **Expected go.mod after tidy** (from R-006): ``` module dns-helper go 1.20 require golang.org/x/net v0.35.0 require golang.org/x/sys v... // indirect (pulled by x/net) ``` - **What gets removed**: `github.com/miekg/dns`, `golang.org/x/mod`, `golang.org/x/tools` — these are only needed by `miekg/dns`. - **Expected implementation**: Run `go mod tidy`. Verify `go.mod` no longer contains `miekg/dns`, `x/mod`, or `x/tools`. Run `go list -m all` to confirm only approved dependencies remain. - **Acceptance signal**: `go mod tidy` succeeds. `go list -m all` shows only `golang.org/x/net` and `golang.org/x/sys`. No unapproved dependencies (FR-010, SC-006). - [x] T027 [US6] Verify clean build, all tests pass, and no dead code **Context**: - **Verification checklist** (from spec SC-006): 1. `go build .` — compiles cleanly with Go 1.20. 2. `go test ./...` — all tests pass across all packages. 3. `go vet ./...` — no warnings. 4. `go list -m all` — only approved dependencies (`golang.org/x/net`, `golang.org/x/sys`). 5. No files in root that are unused (only `main.go`, `go.mod`, `go.sum` should remain at root). 6. Cross-compile check: `GOOS=linux go build .`, `GOOS=darwin go build .`, `GOOS=windows go build .` — all succeed. - **Expected outcome**: All six checks pass. The project structure matches the plan.md "Source Code" layout. - **Acceptance signal**: All commands listed above exit with code 0 and produce no error output. **Checkpoint**: Codebase is clean, maintainable, and compiles across all platforms. All old code and dependencies removed. --- ## Phase 9: Polish & Cross-Cutting Concerns **Purpose**: Final validation against quickstart scenarios and acceptance criteria - [x] T028 Run quickstart validation scenarios from `quickstart.md` **Context**: - **Quickstart scenarios** (from `quickstart.md`): 1. `go test ./...` — all tests pass. 2. `go build -o dns-helper .` — builds successfully. 3. Cross-compilation: ```bash GOOS=windows GOARCH=amd64 go build -o bin/windows/dns-helper.exe . GOOS=linux GOARCH=amd64 go build -o bin/linux/dns-helper . GOOS=darwin GOARCH=amd64 go build -o bin/macos/dns-helper . ``` 4. Dependency check: `go list -m all` shows only approved packages. 5. `go mod tidy` produces no changes (already clean). - **Acceptance signal**: All quickstart commands succeed. The tool is ready for manual integration testing. - [x] T029 Final review: verify all success criteria from spec.md **Context**: - **Success criteria checklist** (from spec.md): - **SC-001**: Hosts file never truncated or partially written — verified by atomic write tests (T009/T014). The old `O_WRONLY|O_TRUNC` pattern in `fileoperations.go` is removed (T025). - **SC-002**: Zero duplicate IPs from CNAME resolution — verified by resolver tests (T015/T017). The self-appending bug in `dns.go` is removed (T025). - **SC-003**: Multi-host deletion removes exactly targeted entries — verified by deletion tests (T018/T019). The loop-order bug in `dataprep.go` is removed (T025). - **SC-004**: Invalid inputs produce descriptive errors and non-zero exit — verified by main.go validation logic (T021/T022). - **SC-005**: Platform functions return errors, not `os.Exit` — verified by platform audit (T023). - **SC-006**: Clean Go 1.20 build, no dead code, no unapproved deps — verified by T027. - **SC-007**: Backup created before write, deleted on success — verified by write tests (T009/T014). - **Acceptance signal**: All seven success criteria can be confirmed via test results and code review. Feature is complete. --- ## Dependencies & Execution Order ### Phase Dependencies - **Setup (Phase 1)**: No dependencies — can start immediately - **Foundational (Phase 2)**: Depends on Setup (T001 for directories, T002 for go.mod) - **US1 (Phase 3)**: Depends on Foundational (T003 for FileSystem interface) - **US2 (Phase 4)**: Depends on Setup (T002 for `golang.org/x/net`). Independent of US1. - **US3 (Phase 5)**: Depends on US1 (T011/T012 for managed.go to extend) - **US4 (Phase 6)**: Depends on US1 + US2 + US3 (imports all packages) - **US5 (Phase 7)**: Depends on Foundational (T004 for platform package). Can run in parallel with US1-US3. - **US6 (Phase 8)**: Depends on US4 (main.go must be complete before removing old files) - **Polish (Phase 9)**: Depends on all previous phases ### User Story Dependencies ``` Phase 1 (Setup) ↓ Phase 2 (Foundational) ─────────────────────────────┐ ↓ ↓ ↓ ↓ Phase 3 (US1) Phase 4 (US2) Phase 7 (US5) │ ↓ ↓ │ Phase 5 (US3) │ │ ↓ ↓ │ └────────→ Phase 6 (US4) ←────────────────────────┘ ↓ Phase 8 (US6) ↓ Phase 9 (Polish) ``` ### Within Each User Story - Tests MUST be written and FAIL before implementation (TDD Red-Green) - Types/stubs before tests (so tests compile) - Parsing before reads, reads before writes - Story complete and testable before moving to next priority ### Parallel Opportunities - **Phase 2**: All tasks (T003-T007) can run in parallel — different packages/files - **Phase 3**: T008 and T009 (tests) can run in parallel after T010 (types) - **Phase 4**: T015 (tests) can run in parallel with T016 (stubs) — different files - **Phase 4 + Phase 7**: US2 and US5 are independent and can run in parallel - **Phase 8**: T025 (delete files) can be done before T026 (go mod tidy) --- ## Parallel Example: Foundation Phase ```bash # After Phase 1 completes, launch all foundational tasks together: Task T003: "Create FileSystem interface in hostfile/filesystem.go" Task T004: "Create platform package in platform/" Task T005: "Create lockfile package in lockfile/lockfile.go" Task T006: "Create lockfile tests in lockfile/lockfile_test.go" Task T007: "Create platform tests in platform/platform_test.go" ``` ## Parallel Example: User Story 1 ```bash # After T010 (types created), launch both test files together (TDD Red): Task T008: "Write managed block parsing tests in hostfile/managed_test.go" Task T009: "Write hostfile read/write tests in hostfile/hostfile_test.go" # Then implement sequentially (TDD Green): Task T011: "Implement managed block parsing in hostfile/managed.go" Task T012: "Implement AddEntries in hostfile/managed.go" Task T013: "Implement Read in hostfile/hostfile.go" Task T014: "Implement atomic Write in hostfile/hostfile.go" ``` --- ## Implementation Strategy ### MVP First (User Story 1 + 2 Only) 1. Complete Phase 1: Setup 2. Complete Phase 2: Foundational (CRITICAL — blocks all stories) 3. Complete Phase 3: User Story 1 (atomic write safety) 4. Complete Phase 4: User Story 2 (correct DNS resolution) 5. **STOP and VALIDATE**: `go test ./hostfile/ ./resolver/` passes. Core safety and correctness bugs are fixed. ### Incremental Delivery 1. Setup + Foundational → Foundation ready 2. US1 → Atomic write working → Hosts file can never be corrupted 3. US2 → DNS resolution correct → CNAME/dedup bugs fixed 4. US3 → Deletion working → Multi-host deletion bug fixed 5. US4 → Full CLI working → Tool is end-to-end functional 6. US5 → Platform consistency verified 7. US6 → Old code removed, clean build 8. Polish → All acceptance criteria confirmed ### Each increment adds value without breaking previous stories. --- ## Notes - [P] tasks = different files, no dependencies on incomplete tasks in same phase - [Story] label maps task to specific user story for traceability - Each user story is independently testable via `go test` for its package - TDD workflow: write tests (red), implement (green), refactor - Commit after each task or logical group - Stop at any checkpoint to validate story independently - The `bin/` directory and its contents (test hosts files) should be preserved — they are not part of the refactor - Every non-trivial task includes a **Context** block with implementation details sufficient for a different model to execute