From 5e28d0bd8cd667b7c7807d3723edc8410b726925 Mon Sep 17 00:00:00 2001 From: ek-dyoder Date: Tue, 3 Mar 2026 17:43:04 -0500 Subject: [PATCH] Refactor code structure for improved readability and maintainability --- .github/agents/copilot-instructions.md | 29 + .github/agents/speckit.analyze.agent.md | 23 +- .github/agents/speckit.tasks.agent.md | 105 +- .specify/memory/constitution.md | 210 ++ .specify/templates/tasks-template.md | 23 + .../checklists/requirements.md | 36 + .../contracts/cli.md | 229 +++ .../contracts/packages.md | 168 ++ .../data-model.md | 160 ++ specs/001-safety-reliability-refactor/plan.md | 110 + .../quickstart.md | 162 ++ .../research.md | 326 +++ specs/001-safety-reliability-refactor/spec.md | 178 ++ .../001-safety-reliability-refactor/tasks.md | 1784 +++++++++++++++++ 14 files changed, 3535 insertions(+), 8 deletions(-) create mode 100644 .github/agents/copilot-instructions.md create mode 100644 .specify/memory/constitution.md create mode 100644 specs/001-safety-reliability-refactor/checklists/requirements.md create mode 100644 specs/001-safety-reliability-refactor/contracts/cli.md create mode 100644 specs/001-safety-reliability-refactor/contracts/packages.md create mode 100644 specs/001-safety-reliability-refactor/data-model.md create mode 100644 specs/001-safety-reliability-refactor/plan.md create mode 100644 specs/001-safety-reliability-refactor/quickstart.md create mode 100644 specs/001-safety-reliability-refactor/research.md create mode 100644 specs/001-safety-reliability-refactor/spec.md create mode 100644 specs/001-safety-reliability-refactor/tasks.md diff --git a/.github/agents/copilot-instructions.md b/.github/agents/copilot-instructions.md new file mode 100644 index 0000000..3535ffa --- /dev/null +++ b/.github/agents/copilot-instructions.md @@ -0,0 +1,29 @@ +# dns-helper Development Guidelines + +Auto-generated from all feature plans. Last updated: 2026-03-03 + +## Active Technologies + +- Go 1.20 (maximum; currently go.mod says 1.18, must be updated to 1.20) + Go standard library, `golang.org/x/net/dns/dnsmessage` (replacing `github.com/miekg/dns`) (001-safety-reliability-refactor) + +## Project Structure + +```text +src/ +tests/ +``` + +## Commands + +# Add commands for Go 1.20 (maximum; currently go.mod says 1.18, must be updated to 1.20) + +## Code Style + +Go 1.20 (maximum; currently go.mod says 1.18, must be updated to 1.20): Follow standard conventions + +## Recent Changes + +- 001-safety-reliability-refactor: Added Go 1.20 (maximum; currently go.mod says 1.18, must be updated to 1.20) + Go standard library, `golang.org/x/net/dns/dnsmessage` (replacing `github.com/miekg/dns`) + + + diff --git a/.github/agents/speckit.analyze.agent.md b/.github/agents/speckit.analyze.agent.md index 542a3de..ae5be0b 100644 --- a/.github/agents/speckit.analyze.agent.md +++ b/.github/agents/speckit.analyze.agent.md @@ -59,6 +59,8 @@ Load only the minimal necessary context from each artifact: - Phase grouping - Parallel markers [P] - Referenced file paths +- Context blocks (per-task): code signatures, implementation patterns, gotchas, acceptance signals +- Phase Context summaries: files modified, key types/interfaces, codebase conventions **From constitution:** @@ -111,14 +113,26 @@ Focus on high-signal findings. Limit to 50 findings total; aggregate remainder i - Task ordering contradictions (e.g., integration tasks before foundational setup tasks without dependency note) - Conflicting requirements (e.g., one requires Next.js while other specifies Vue) +#### G. Implementation Self-Containment + +**Why this matters**: Tasks will be executed by a less-capable implementing model (e.g., Sonnet) that cannot reliably infer implementation details from sparse descriptions. Every non-trivial task must be a standalone implementation brief. + +- **Missing Context blocks**: Flag any non-trivial task (anything beyond adding a simple field or creating an empty file) that lacks an indented **Context** block beneath the checkbox line. Severity: HIGH. +- **Missing code signatures**: Context blocks that describe interfaces, structs, or functions the task depends on but do NOT quote the actual signatures with fenced code blocks. Severity: HIGH. +- **Missing implementation pattern**: Context blocks that say *what* to build but not *how* — no code skeleton, no pseudocode, no reference to an analogous pattern elsewhere in the codebase. Severity: MEDIUM. +- **Missing acceptance signal**: Tasks without a concrete, verifiable "done" statement (e.g., a specific test command, a build command, or observable output). Severity: MEDIUM. +- **Missing gotchas/constraints**: Tasks that touch complex logic (edge cases, platform constraints, performance-sensitive code) but whose Context block omits known pitfalls from the spec or research documents. Severity: MEDIUM. +- **Missing Phase Context summary**: Phases that lack the opening block listing files modified, key types, and codebase conventions. Severity: LOW. +- **Placeholder signatures**: Context blocks that contain generic or assumed code signatures not verified against the actual codebase (e.g., `func DoSomething()` when the real signature has different parameters). Severity: HIGH. + ### 5. Severity Assignment Use this heuristic to prioritize findings: - **CRITICAL**: Violates constitution MUST, missing core spec artifact, or requirement with zero coverage that blocks baseline functionality -- **HIGH**: Duplicate or conflicting requirement, ambiguous security/performance attribute, untestable acceptance criterion -- **MEDIUM**: Terminology drift, missing non-functional task coverage, underspecified edge case -- **LOW**: Style/wording improvements, minor redundancy not affecting execution order +- **HIGH**: Duplicate or conflicting requirement, ambiguous security/performance attribute, untestable acceptance criterion, non-trivial task missing Context block or code signatures (blocks implementation by less-capable model) +- **MEDIUM**: Terminology drift, missing non-functional task coverage, underspecified edge case, Context block missing implementation pattern or acceptance signal +- **LOW**: Style/wording improvements, minor redundancy not affecting execution order, missing Phase Context summary ### 6. Produce Compact Analysis Report @@ -145,9 +159,12 @@ Output a Markdown report (no file writes) with the following structure: - Total Requirements - Total Tasks +- Non-Trivial Tasks (tasks requiring Context blocks) - Coverage % (requirements with >=1 task) +- Context Completeness % (non-trivial tasks with a complete Context block containing: code signatures, implementation pattern, and acceptance signal) - Ambiguity Count - Duplication Count +- Self-Containment Issues Count (from Detection Pass G) - Critical Issues Count ### 7. Provide Next Actions diff --git a/.github/agents/speckit.tasks.agent.md b/.github/agents/speckit.tasks.agent.md index 0e932da..5e6bc55 100644 --- a/.github/agents/speckit.tasks.agent.md +++ b/.github/agents/speckit.tasks.agent.md @@ -25,14 +25,14 @@ You **MUST** consider the user input before proceeding (if not empty). 2. **Load design documents**: Read from FEATURE_DIR: - **Required**: plan.md (tech stack, libraries, structure), spec.md (user stories with priorities) - - **Optional**: data-model.md (entities), contracts/ (interface contracts), research.md (decisions), quickstart.md (test scenarios) + - **Optional**: data-model.md (entities), contracts/ (API endpoints), research.md (decisions), quickstart.md (test scenarios) - Note: Not all projects have all documents. Generate tasks based on what's available. 3. **Execute task generation workflow**: - Load plan.md and extract tech stack, libraries, project structure - Load spec.md and extract user stories with their priorities (P1, P2, P3, etc.) - If data-model.md exists: Extract entities and map to user stories - - If contracts/ exists: Map interface contracts to user stories + - If contracts/ exists: Map endpoints to user stories - If research.md exists: Extract decisions for setup tasks - Generate tasks organized by user story (see Task Generation Rules below) - Generate dependency graph showing user story completion order @@ -64,6 +64,101 @@ Context for task generation: $ARGUMENTS The tasks.md should be immediately executable - each task must be specific enough that an LLM can complete it without additional context. +## Implementation Context Requirements + +**CRITICAL**: The tasks.md will be executed by a DIFFERENT model than the one generating it. The generating model (you) has deep reasoning capability and can infer implementation details from sparse descriptions. The implementing model does NOT — it needs explicit, self-contained context embedded directly in each task. Every task must be a standalone implementation brief, not a summary that requires the reader to independently analyze the codebase. + +### Per-Task Context Blocks + +For every non-trivial task (anything beyond creating an empty file or adding a simple field), include an indented **Context** block immediately below the task checkbox line. This block MUST contain: + +1. **Existing code references**: Quote the exact current function signatures, struct definitions, or interface methods that the task modifies or depends on. Use fenced code blocks with the language identifier. + +2. **Target file state**: Describe what already exists in the target file that the implementer needs to know about (imports, adjacent functions, package conventions). + +3. **Expected implementation pattern**: Show a concrete code skeleton or pseudocode of what the implementation should look like. Reference similar patterns already in the codebase by quoting them. For example: "Follow the same pattern as `computeDatasetAnalytics()` in `engine.go` lines 245-310, which iterates entries and accumulates stats." + +4. **Key decisions and gotchas**: Non-obvious constraints, edge cases, or design decisions from the spec/research that affect this task. Example: "Must use `int64` not `int` for file sizes to handle >2GB files on 32-bit systems" or "The profile may have RecursiveAnalysis disabled — check `opts.Profile.RecursiveAnalysis` before accumulating subdirectory maps." + +5. **Acceptance signal**: A concrete, verifiable statement of what "done" looks like. Example: "`go test ./internal/engine/... -run TestRegistryResolve` passes with all 6 subcases green." + +### Example Task WITH Context (REQUIRED format for all non-trivial tasks) + +```markdown +- [ ] T009 [US1] Create template analysis module in `internal/engine/module_template.go` + + **Context**: + - **Implements**: `AnalysisModule` interface from `internal/engine/module.go`: + ```go + type AnalysisModule interface { + Name() string + Description() string + ProcessEntry(entry *types.ManifestEntry) + Finalize(ctx *FinalizationContext) (*ModuleResult, error) + } + ``` + - **Depends on**: `ContainerMatcher` from `internal/discovery/matcher.go` — call `matcher.Match(entry.Path)` to identify containers. The matcher is initialized from `profile.ContainerPatterns` (see `engine.go:initContainerMatcher()`). + - **Accumulation pattern**: Follow the same accumulator pattern as `DatasetAnalyticsAccumulator` in `pipeline.go` — use maps keyed by container path to accumulate file counts, total sizes, and date ranges incrementally during `ProcessEntry()`. + - **Key struct fields to accumulate**: + ```go + type templateModule struct { + matcher *discovery.ContainerMatcher + profile *config.Profile + accumulators map[string]*containerAccumulator // keyed by container path + seen map[string]bool // for nesting exclusion + subdirs map[string]map[string]bool // container -> subdirs (only if recursive enabled) + filesByPath map[string]map[string]int // container -> subdir -> file count + sizeByPath map[string]map[string]int64 // container -> subdir -> total size + } + ``` + - **Gotcha**: Nesting exclusion — if `/projects/foo` is a container, `/projects/foo/bar` should NOT also be identified as a separate container even if it matches. Use the `seen` set: before adding a new container, check if any prefix of its path is already in `seen`. + - **Acceptance**: Module registers successfully, `ProcessEntry()` populates accumulators for test entries, `Name()` returns `"template"`. +``` + +### Example Task WITHOUT Context (WRONG — never do this) + +```markdown +- [ ] T009 [US1] Create template analysis module in `internal/engine/module_template.go` — implement AnalysisModule interface, accumulate container stats during ProcessEntry(), handle nesting exclusion +``` + +The second example forces the implementing model to independently figure out what `AnalysisModule` looks like, how `ContainerMatcher` works, what fields to accumulate, and how nesting exclusion works. This results in incorrect or incomplete implementations. + +### Per-Phase Context Summary + +At the start of each phase (after the **Goal** line), include a **Phase Context** block listing: + +- **Files modified in this phase**: Full paths with a one-line description of each file's current purpose +- **Key types/interfaces used**: Quoted signatures of types that tasks in this phase depend on +- **Codebase conventions to follow**: Naming patterns, error handling style, import grouping, or test patterns observed in the project (reference specific files as examples) + +### Context Sourcing Strategy — Plan Artifacts First + +The design documents produced by `/speckit.plan` (contracts, data-model, research, quickstart) already contain most of the information that Context blocks need: interface signatures, struct definitions, design decisions, gotchas, error messages, and acceptance scenarios. **Use these artifacts as your primary source for Context blocks instead of re-reading source files.** + +**For tasks that CREATE new files** (new modules, new types, new test files): +- Pull interface signatures, struct definitions, and method contracts from **contracts/** and **data-model.md** — these already contain the exact code the implementer needs +- Pull design decisions, accumulation patterns, and gotchas from **research.md** — cite the specific research item (e.g., "See R3: Container Identification During Streaming") +- Pull acceptance scenarios from **quickstart.md** — map CLI examples to verifiable acceptance signals +- **Do NOT read source files** unless the plan artifacts reference an existing pattern that needs to be matched (e.g., "follows the existing `plugin.Registry` pattern") — in that case, read only the referenced file to quote the specific pattern + +**For tasks that MODIFY existing files** (adding fields, rewiring functions, updating existing logic): +- Still use plan artifacts for the **target design** (what the code should become) +- Read the **specific function/struct being modified** from the source file to quote its **current state** — the implementer needs to see both "what exists now" and "what it should become" +- Do NOT read the entire file — read only the function/struct being changed plus its immediate dependencies + +**For test tasks**: +- Read **one existing test file** in the same package to capture test conventions (table-driven patterns, setup/teardown helpers, assertion style). Quote a representative test function as a pattern to follow. +- Pull test scenarios from contracts and quickstart — these define expected inputs/outputs + +**Always**: +- Read **copilot-instructions.md** or equivalent project guidelines (once, at the start of task generation) for language version constraints, banned patterns, build requirements, and protected code paths +- If the project has a design system or style guide referenced in the plan, read it (once) + +**Do NOT**: +- Read full source files for every task — the plan artifacts already distill the relevant information +- Re-derive information that exists in contracts or research — reference those documents directly +- Generate tasks with placeholder signatures — every code reference must come from either plan artifacts or a targeted source file read + ## Task Generation Rules **CRITICAL**: Tasks MUST be organized by user story to enable independent implementation and testing. @@ -109,13 +204,13 @@ Every task MUST strictly follow this format: - Map all related components to their story: - Models needed for that story - Services needed for that story - - Interfaces/UI needed for that story + - Endpoints/UI needed for that story - If tests requested: Tests specific to that story - Mark story dependencies (most stories should be independent) 2. **From Contracts**: - - Map each interface contract → to the user story it serves - - If tests requested: Each interface contract → contract test task [P] before implementation in that story's phase + - Map each contract/endpoint → to the user story it serves + - If tests requested: Each contract → contract test task [P] before implementation in that story's phase 3. **From Data Model**: - Map each entity to the user story(ies) that need it diff --git a/.specify/memory/constitution.md b/.specify/memory/constitution.md new file mode 100644 index 0000000..a6df9a7 --- /dev/null +++ b/.specify/memory/constitution.md @@ -0,0 +1,210 @@ + + +# DNS-Helper Constitution + +## Core Principles + +### I. System File Safety + +The hosts file is a critical operating system resource. Corruption or +data loss in this file disables all local name resolution and can render +a machine unusable without manual intervention. + +- The tool MUST NOT truncate, zero-out, or partially overwrite the hosts + file at any point during its operation. +- All writes to the hosts file MUST use an atomic pattern: write complete + content to a temporary file in the same directory, then rename over the + original. +- A backup of the current hosts file MUST be created before any + modification is committed. +- If any step in the write pipeline fails, the original hosts file MUST + remain unmodified. + +**Rationale**: This tool exists to solve DNS reliability problems. If it +introduces a new failure mode — destroying the hosts file — it is worse +than the problem it solves. + +### II. Standard Library First + +The Go standard library MUST be the first choice for all functionality. +External dependencies introduce supply-chain risk, version conflicts, +and maintenance burden that are disproportionate for a small, +single-purpose utility. + +- All functionality MUST be implemented using the Go standard library + unless a documented technical justification proves it insufficient. +- When an external dependency is genuinely required, it MUST come from + one of the following approved sources: + - `golang.org/x/*` (official Go extended libraries) + - `dev.emberkom.com/*` (Emberkom internal libraries) +- Any other external dependency MUST have a written justification in the + code review or commit message explaining: (a) what standard library + alternative was considered, (b) why it was insufficient, and (c) what + the external dependency provides that the stdlib does not. +- Dependencies MUST be audited when upgraded to ensure no new transitive + dependencies from unapproved sources are introduced. + +**Rationale**: A single-binary CLI tool that modifies a system file must +have a minimal and auditable dependency tree. Every external package is +an attack surface and a maintenance liability. + +### III. Go 1.20 Compatibility + +This tool MUST compile and run correctly with Go 1.20. This version +ceiling exists to maintain compatibility with legacy systems that cannot +run newer Go runtimes. + +- The `go.mod` file MUST specify `go 1.20` as the language version. +- No language features, standard library APIs, or dependency versions + exclusive to Go 1.21 or later are permitted. +- Build and test pipelines MUST use Go 1.20 as the primary toolchain + version to catch compatibility issues early. +- If a dependency releases a version that requires Go 1.21+, the tool + MUST pin to the last Go 1.20-compatible release of that dependency. + +**Rationale**: The target deployment environments include older systems +where upgrading the Go toolchain or OS is not feasible. Pinning the +version ensures the tool remains deployable everywhere it is needed. + +### IV. Idiomatic Error Handling + +Errors MUST flow upward through return values. Functions MUST NOT +terminate the process, call `os.Exit`, or invoke global exit helpers +except at the top-level `main` function. + +- Every function that can fail MUST return an `error` value. +- Callers MUST inspect returned errors and either handle them, wrap them + with additional context, or propagate them upward. +- The `main` function is the only location permitted to call `os.Exit` + or print a fatal error and terminate. +- Platform-specific code (build-tagged files) MUST return errors to the + caller rather than terminating directly. +- Errors presented to the user MUST include enough context to understand + what operation failed and why (e.g., file path, hostname, DNS server). + +**Rationale**: Internal `os.Exit` calls make code untestable, prevent +cleanup logic from running, and cause execution to continue past error +points when deferred. Centralizing exit decisions in `main` produces +predictable, debuggable control flow. + +### V. Simplicity and Single Purpose + +This tool does one thing: update the local hosts file with DNS entries +resolved from a specified DNS server. Every design decision MUST favor +the simplest approach that satisfies the requirement. + +- The tool MUST remain a single-binary CLI with no external + configuration files, services, or daemons. +- Features MUST be justified by a concrete user need, not speculative + future use. YAGNI (You Aren't Gonna Need It) applies. +- Dead code, unused exports, and speculative abstractions MUST be + removed. If code is not called, it does not belong in the repository. +- The CLI interface MUST remain straightforward: subcommand + flags. + No interactive prompts, no TUI, no config file parsing. + +**Rationale**: Complexity is the enemy of reliability. A tool that +modifies a critical system file must be small enough to reason about +completely and audit quickly. + +### VI. Test-Driven Development + +All new and refactored code MUST be developed using a test-driven +development (TDD) approach. Tests are not an afterthought — they are +the specification that drives implementation. + +- Tests MUST be written before the implementation code they verify. + The Red-Green-Refactor cycle is mandatory: write a failing test, + write the minimum code to make it pass, then refactor. +- Every exported function and every function with non-trivial logic + MUST have corresponding test coverage. +- Functions that interact with external resources (filesystem, DNS, + network) MUST be designed to accept interfaces or parameters that + enable testing with fakes, stubs, or in-memory implementations. + Direct calls to OS-level I/O from business logic are prohibited. +- Platform-specific code MUST have tests that verify the function + contract (correct return values, correct error conditions) on each + supported platform. +- Test files MUST follow Go conventions: `*_test.go` in the same + package as the code under test. +- `go test ./...` MUST pass with zero failures before code is merged. + +**Rationale**: This tool modifies a critical system file. Untested code +is untrustworthy code. TDD ensures that every behavior is verified, +every error path is exercised, and regressions are caught immediately. +It also enforces the interface-based design required by Principle IV +(idiomatic error handling) and makes the codebase safe to refactor. + +## Dependency Governance + +This section codifies the external dependency policy from Principle II +into an auditable process. + +| Source | Status | Condition | +|--------|--------|-----------| +| Go standard library | Always permitted | N/A | +| `golang.org/x/*` | Permitted | Must solve a problem stdlib cannot | +| `dev.emberkom.com/*` | Permitted | Must solve a problem stdlib cannot | +| All other sources | Prohibited by default | Requires written justification, code review approval, and an entry in this table | + +### Currently Approved External Dependencies + +| Module | Justification | +|--------|---------------| +| `github.com/miekg/dns` | The Go standard library `net` package does not support querying a specific DNS server by address. This tool's core purpose requires directing queries to a user-specified resolver, which `net.Resolver` cannot do without low-level `Dialer` workarounds that replicate what `miekg/dns` provides. This is a legacy dependency approved under a grandfather clause — if a `golang.org/x` or stdlib alternative becomes viable under Go 1.20, migration is preferred. | + +## Development Standards + +- **Formatting**: All Go source files MUST be formatted with `gofmt`. + No exceptions. +- **Vet/Lint**: `go vet` MUST pass with zero findings before code is + merged. Additional linting (e.g., `staticcheck`) is encouraged. +- **Testing**: All new and refactored code MUST have test coverage + written before the implementation (TDD). Functions that interact with + the filesystem or DNS MUST be designed to accept interfaces or + parameters that allow testing with fakes/stubs. `go test ./...` MUST + pass with zero failures before merge. See Principle VI for the full + TDD policy. +- **Build tags**: Platform-specific code MUST use Go build tags + (`//go:build windows`, etc.) and MUST implement identical function + signatures across all supported platforms. +- **Commit messages**: Follow conventional commit format + (e.g., `fix:`, `feat:`, `refactor:`, `docs:`). + +## Governance + +This constitution is the authoritative source of project standards for +DNS-Helper. It supersedes informal conventions, ad-hoc decisions, and +prior practices that conflict with its contents. + +- **Amendment process**: Any change to this constitution MUST be + documented with a version bump, a rationale for the change, and an + update to the Sync Impact Report at the top of this file. +- **Versioning policy**: This constitution follows semantic versioning. + MAJOR for principle removals or redefinitions, MINOR for new + principles or materially expanded guidance, PATCH for clarifications + and non-semantic refinements. +- **Compliance review**: All pull requests and code reviews MUST verify + that changes comply with the principles defined here. Violations MUST + be resolved before merge, or the constitution MUST be amended first. +- **Dependency audits**: When dependencies are added or upgraded, the + reviewer MUST verify compliance with Principle II and the Dependency + Governance table above. + +**Version**: 1.1.0 | **Ratified**: 2026-03-03 | **Last Amended**: 2026-03-03 diff --git a/.specify/templates/tasks-template.md b/.specify/templates/tasks-template.md index 60f9be4..ea6a204 100644 --- a/.specify/templates/tasks-template.md +++ b/.specify/templates/tasks-template.md @@ -12,6 +12,8 @@ description: "Task list template for feature implementation" **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. The generating model has deep reasoning capability and can infer implementation details from sparse descriptions. The implementing model does NOT — it needs explicit, self-contained context embedded directly in each task. Every non-trivial task must include an indented **Context** block (see examples below). + ## Format: `[ID] [P?] [Story] Description` - **[P]**: Can run in parallel (different files, no dependencies) @@ -48,6 +50,11 @@ description: "Task list template for feature implementation" **Purpose**: Project initialization and basic structure +**Phase Context**: +- **Files modified in this phase**: `package.json` (project manifest), `src/` (directory structure), `.eslintrc` (linting config) +- **Key types/interfaces used**: None yet — this phase creates the foundation +- **Codebase conventions to follow**: [Reference project guidelines or existing patterns] + - [ ] T001 Create project structure per implementation plan - [ ] T002 Initialize [language] project with [framework] dependencies - [ ] T003 [P] Configure linting and formatting tools @@ -79,6 +86,11 @@ Examples of foundational tasks (adjust based on your project): **Independent Test**: [How to verify this story works on its own] +**Phase Context**: +- **Files modified in this phase**: [Full paths with one-line description of each file's current purpose] +- **Key types/interfaces used**: [Quoted signatures of types that tasks in this phase depend on] +- **Codebase conventions to follow**: [Naming patterns, error handling style, import grouping, test patterns — reference specific files as examples] + ### Tests for User Story 1 (OPTIONAL - only if tests requested) ⚠️ > **NOTE: Write these tests FIRST, ensure they FAIL before implementation** @@ -89,6 +101,14 @@ Examples of foundational tasks (adjust based on your project): ### Implementation for User Story 1 - [ ] T012 [P] [US1] Create [Entity1] model in src/models/[entity1].py + + **Context**: + - **Existing code references**: [Quote the exact interface/base class this model must implement, with fenced code block] + - **Target file state**: `src/models/` directory exists after T001; no other models present yet + - **Expected implementation pattern**: [Show code skeleton or reference similar pattern in codebase, e.g., "Follow the same pattern as `BaseEntity` in `src/models/base.py` lines 10-45"] + - **Key decisions and gotchas**: [Non-obvious constraints from spec/research, e.g., "Must use UUID for primary key per spec NFR-3" or "Field `name` has max length 255 per data-model.md"] + - **Acceptance signal**: [Concrete verification, e.g., "`python -m pytest tests/unit/test_entity1.py` passes" or "File imports successfully with no errors"] + - [ ] T013 [P] [US1] Create [Entity2] model in src/models/[entity2].py - [ ] T014 [US1] Implement [Service] in src/services/[service].py (depends on T012, T013) - [ ] T015 [US1] Implement [endpoint/feature] in src/[location]/[file].py @@ -249,3 +269,6 @@ With multiple developers: - Commit after each task or logical group - Stop at any checkpoint to validate story independently - Avoid: vague tasks, same file conflicts, cross-story dependencies that break independence +- **Every non-trivial task MUST include an indented Context block** with: existing code references (quoted signatures), target file state, expected implementation pattern (code skeleton or analogous pattern reference), key decisions/gotchas, and a concrete acceptance signal +- **Every phase MUST include a Phase Context summary** listing: files modified, key types/interfaces used, and codebase conventions to follow +- Context blocks should source information from plan artifacts (contracts, data-model, research, quickstart) first, and only read source files when the plan references an existing pattern that needs to be matched diff --git a/specs/001-safety-reliability-refactor/checklists/requirements.md b/specs/001-safety-reliability-refactor/checklists/requirements.md new file mode 100644 index 0000000..101b07d --- /dev/null +++ b/specs/001-safety-reliability-refactor/checklists/requirements.md @@ -0,0 +1,36 @@ +# Specification Quality Checklist: Safety, Reliability & Idiomatic Refactor + +**Purpose**: Validate specification completeness and quality before proceeding to planning +**Created**: 2026-03-03 +**Feature**: [spec.md](../spec.md) + +## Content Quality + +- [x] No implementation details (languages, frameworks, APIs) +- [x] Focused on user value and business needs +- [x] Written for non-technical stakeholders +- [x] All mandatory sections completed + +## Requirement Completeness + +- [x] No [NEEDS CLARIFICATION] markers remain +- [x] Requirements are testable and unambiguous +- [x] Success criteria are measurable +- [x] Success criteria are technology-agnostic (no implementation details) +- [x] All acceptance scenarios are defined +- [x] Edge cases are identified +- [x] Scope is clearly bounded +- [x] Dependencies and assumptions identified + +## Feature Readiness + +- [x] All functional requirements have clear acceptance criteria +- [x] User scenarios cover primary flows +- [x] Feature meets measurable outcomes defined in Success Criteria +- [x] No implementation details leak into specification + +## Notes + +- FR-010 references "Go 1.20" as an explicit user constraint, not a technology recommendation — this is acceptable per the user's requirement for legacy system compatibility. +- The spec references "atomic write pattern" and "CNAME chains" as behavioral descriptions, not implementation prescriptions. The planning phase will determine specific implementation approaches. +- All checklist items pass. Spec is ready for `/speckit.clarify` or `/speckit.plan`. diff --git a/specs/001-safety-reliability-refactor/contracts/cli.md b/specs/001-safety-reliability-refactor/contracts/cli.md new file mode 100644 index 0000000..2be1cdd --- /dev/null +++ b/specs/001-safety-reliability-refactor/contracts/cli.md @@ -0,0 +1,229 @@ +# CLI Contract: dns-helper + +**Feature Branch**: `001-safety-reliability-refactor` +**Date**: 2026-03-03 + +## Overview + +`dns-helper` is a single-binary CLI tool that updates the local hosts file with DNS entries resolved from a user-specified DNS server. It exposes three subcommands: `add`, `delete`, and a usage/help display on invalid input. + +## Binary Name + +``` +dns-helper[.exe] +``` + +## Global Behavior + +- Prints banner on every invocation: `DNSHelper v1.0`, `Copyright (c) 2024 Emberkom LLC`, blank line. +- All error messages are printed to **stderr**. +- Informational output (banner, summaries, warnings) is printed to **stdout**. +- Exit code `0` on full success. Non-zero on any error or partial failure. + +--- + +## Subcommand: `add` (aliases: `a`) + +### Synopsis + +``` +dns-helper add -host -server +``` + +### Flags + +| Flag | Required | Type | Description | +|------|----------|------|-------------| +| `-host` | Yes | string | Comma-separated list of hostnames to resolve and add | +| `-server` | Yes | string | DNS server to use for resolution (hostname or IP) | + +### Behavior + +1. Validates that both `-host` and `-server` are provided and non-empty. +2. Resolves each hostname against the specified DNS server (follows CNAME chains, deduplicates IPs). +3. Acquires lock file. +4. Reads the current hosts file and parses the managed block. +5. Removes any existing managed entries for the specified hostnames. +6. Adds new entries for all successfully resolved hostnames. +7. Writes the updated hosts file atomically (temp file + rename, with pre-write backup). +8. Releases lock file. +9. Prints summary of changes. + +### Output — Success (exit 0) + +``` +DNSHelper v1.0 +Copyright (c) 2024 Emberkom LLC + +Added 3 entries for host1.example.com +Added 2 entries for host2.example.com +``` + +### Output — Partial Failure (exit 1) + +``` +DNSHelper v1.0 +Copyright (c) 2024 Emberkom LLC + +Added 3 entries for host1.example.com +Warning: failed to resolve host2.example.com: NXDOMAIN +``` + +### Output — Validation Error (exit 1) + +``` +DNSHelper v1.0 +Copyright (c) 2024 Emberkom LLC + +Error: -host flag is required for the add command +``` + +### Exit Codes + +| Code | Meaning | +|------|---------| +| 0 | All hostnames resolved and written successfully | +| 1 | Any error: validation failure, DNS failure, file write failure, partial resolution | + +--- + +## Subcommand: `delete` (aliases: `d`, `del`) + +### Synopsis + +``` +dns-helper delete -host +dns-helper delete all +``` + +### Flags + +| Flag | Required | Type | Description | +|------|----------|------|-------------| +| `-host` | Yes* | string | Comma-separated list of hostnames to remove (*not required when using `all`) | + +### Behavior — Delete Specific Hosts + +1. Validates that `-host` is provided and non-empty. +2. Acquires lock file. +3. Reads the current hosts file and parses the managed block. +4. Removes all managed entries matching the specified hostnames. +5. Writes the updated hosts file atomically. +6. Releases lock file. +7. Prints summary. + +### Behavior — Delete All + +1. Acquires lock file. +2. Reads the current hosts file and parses the managed block. +3. Removes the entire managed block (start marker through end marker). +4. Writes the updated hosts file atomically. +5. Releases lock file. +6. Prints summary. + +### Output — Success (exit 0) + +``` +DNSHelper v1.0 +Copyright (c) 2024 Emberkom LLC + +Removed 3 entries for host1.example.com +``` + +### Output — No Entries Found (exit 0) + +``` +DNSHelper v1.0 +Copyright (c) 2024 Emberkom LLC + +No managed entries found for host1.example.com +``` + +### Output — Delete All (exit 0) + +``` +DNSHelper v1.0 +Copyright (c) 2024 Emberkom LLC + +Removed all managed entries (5 entries removed) +``` + +### Exit Codes + +| Code | Meaning | +|------|---------| +| 0 | Deletion completed (or no entries to delete) | +| 1 | Error: validation failure, file write failure, corrupt managed block | + +--- + +## Invalid / No Subcommand + +### Synopsis + +``` +dns-helper +dns-helper +``` + +### Behavior + +1. Prints usage information to stdout. +2. Does **not** modify the hosts file. +3. Exits with code 1. + +### Output (exit 1) + +``` +DNSHelper v1.0 +Copyright (c) 2024 Emberkom LLC + +This utility will update the local hosts file with DNS entries obtained by the specified DNS server. +Usage: dns-helper [add|delete] -host hostname [-server dns.example.com] +Example: + dns-helper add -host xyz.acme.com -server dns.example.com + This will use dns.example.com to find and add all IP addresses for xyz.acme.com to the local hosts file. + dns-helper delete -host hostname.example.com + This will delete all entries for hostname.example.com from the local hosts file. + dns-helper delete all + This will delete all entries from the local hosts file that were added by this utility. +Note: Adding a hostname will first remove all entries in the hosts file that match the same hostname. + This utility will only remove entries from the hosts file that it added. +``` + +--- + +## Error Conditions + +| Condition | Message (stderr) | Exit Code | Hosts File Modified? | +|-----------|-------------------|-----------|---------------------| +| No subcommand | Usage text (stdout) | 1 | No | +| Unknown subcommand | Usage text (stdout) | 1 | No | +| Missing `-host` (add) | `Error: -host flag is required for the add command` | 1 | No | +| Missing `-server` (add) | `Error: -server flag is required for the add command` | 1 | No | +| Empty `-host` value | `Error: -host flag is required for the add command` | 1 | No | +| DNS resolution failure (all hosts) | `Error: failed to resolve : ` | 1 | No | +| DNS resolution partial failure | Warning per host + entries for successful hosts written | 1 | Yes (partial) | +| Hosts file not found | `Error: file does not exist: ` | 1 | No | +| Permission denied | `Error: permission denied: ` | 1 | No | +| Corrupt managed block | `Error: managed block is corrupt:
` | 1 | No | +| Lock file unavailable | `Error: could not acquire lock file : another instance may be running` | 1 | No | +| Write failure | `Error: writing hosts file:
` | 1 | No (atomic write) | + +--- + +## Hosts File Managed Block Format + +``` +# DNSHelper <<-> START CONFIG +1.2.3.4 hostname1.example.com +5.6.7.8 hostname1.example.com +9.10.11.12 hostname2.example.com +# DNSHelper <->> END CONFIG +``` + +- Start marker: `# DNSHelper <<-> START CONFIG` +- End marker: `# DNSHelper <->> END CONFIG` +- Each entry: `\t` (tab-separated) +- Entries are deduplicated by full line content. +- Block is omitted entirely when there are no managed entries. diff --git a/specs/001-safety-reliability-refactor/contracts/packages.md b/specs/001-safety-reliability-refactor/contracts/packages.md new file mode 100644 index 0000000..63dc2e2 --- /dev/null +++ b/specs/001-safety-reliability-refactor/contracts/packages.md @@ -0,0 +1,168 @@ +# Package Contracts: dns-helper + +**Feature Branch**: `001-safety-reliability-refactor` +**Date**: 2026-03-03 + +## Package: `resolver` + +Handles DNS name resolution against a user-specified DNS server. + +### Interface + +```go +// Resolver performs DNS lookups against a specific server. +type Resolver interface { + // LookupIP resolves a hostname to a list of IPv4 addresses using the + // specified DNS server. Follows CNAME chains up to a maximum depth. + // Returns deduplicated IPs. Returns an error if the hostname cannot + // be resolved (NXDOMAIN, timeout, server unreachable, etc.). + LookupIP(hostname, server string) ([]string, error) +} +``` + +### Behavior Contract + +- Queries are sent over UDP to `:53`. +- The `hostname` is automatically converted to FQDN (trailing dot appended if missing). +- An A query is sent first. If A records are returned, they are collected. +- If only CNAME records are returned (no A records in the answer), the CNAME target is resolved recursively. +- CNAME chain depth is limited to 10 levels. Exceeding this returns an error. +- Results are deduplicated before return. +- Timeout: 5 seconds per query. +- On NXDOMAIN: returns `error` with descriptive message. +- On server unreachable/timeout: returns `error` with descriptive message. +- Response ID must match query ID; mismatches are treated as errors. +- Truncated responses (TC flag set) are treated as errors (no TCP fallback for simplicity). + +--- + +## Package: `hostfile` + +Manages reading, parsing, modifying, and atomically writing the hosts file. + +### Interface + +```go +// Manager handles hosts file operations. +type Manager interface { + // 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. + Read(path string) (*HostsFile, error) + + // 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). + Write(hf *HostsFile, backupDir string) error +} + +// HostsFile represents the parsed content of a hosts file. +type HostsFile struct { + Path string + OriginalContent []string + PrefixContent []string + ManagedContent []string + PostfixContent []string + HasManagedBlock bool +} +``` + +### Sub-package: Managed Block Operations + +```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 + +// RemoveByHostname removes all managed entries matching any of the +// specified hostnames. Returns updated content. +func RemoveByHostname(existing []string, hostnames []string) []string + +// RemoveAll returns an empty slice (clears all managed entries). +func RemoveAll() []string +``` + +### Behavior Contract — Read + +- Opens the file at `path` and reads all lines. +- Scans for start marker (`# DNSHelper <<-> START CONFIG`) and end marker (`# DNSHelper <->> END CONFIG`). +- If neither marker found: `HasManagedBlock = false`, all content goes to `PrefixContent`. +- If both markers found in correct order: splits content into Prefix, Managed, Postfix. +- **Corruption detection** (returns error): + - Start marker without end marker. + - End marker without start marker. + - End marker before start marker. + - Duplicate start or end markers. +- Uses sentinel values (not zero-index) for marker detection. + +### Behavior Contract — Write + +1. Creates backup: copies current hosts file to `/hosts.bak.-<4hex>`. +2. Assembles full file content: prefix + (managed block if non-empty) + postfix. +3. Compares assembled content with original — skips write if identical. +4. Creates temp file in same directory as hosts file: `.dns-helper-tmp-*`. +5. Writes content to temp file. +6. Sets temp file permissions to match original hosts file permissions. +7. Calls `Sync()` on temp file. +8. Closes temp file. +9. Calls `os.Rename(temp, hostsPath)`. +10. On rename success: deletes backup, returns nil. +11. On rename failure (including Windows retry 2-3x with 200ms delay): cleans up temp file, returns error. Backup is cleaned up after confirming original is intact. + +--- + +## Package: `platform` + +Provides platform-specific hosts file location. + +### Interface + +```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) +``` + +### Platform Implementations + +| Platform | Path | Detection | +|----------|------|-----------| +| Windows | `%SystemRoot%\System32\drivers\etc\hosts` (fallback: `C:\Windows\...`) | `os.LookupEnv("SystemRoot")` | +| Linux | `/etc/hosts` | Hardcoded | +| macOS | `/etc/hosts` | Hardcoded | + +### Behavior Contract + +- Returns error to caller on file-not-found or access error. +- **Must NOT** call `os.Exit()`, `log.Fatal()`, or any process-terminating function. +- Error messages include the path that was checked. + +--- + +## Package: `lockfile` + +Serializes concurrent access to the hosts file. + +### Interface + +```go +// Lock represents an acquired lock. +type Lock interface { + // Release deletes the lock file. Safe to call multiple times. + Release() error +} + +// Acquire attempts to acquire a lock file in the specified directory. +// Retries for up to maxWait on contention. Breaks stale locks older +// than staleTimeout. Returns an error if the lock cannot be acquired. +func Acquire(dir string) (Lock, error) +``` + +### Behavior Contract + +- Lock file path: `/.dns-helper.lock`. +- Created atomically via `os.OpenFile` with `O_CREATE|O_EXCL`. +- PID written to lock file for debugging (not used for staleness). +- Staleness: lock file with `ModTime` older than 2 minutes is considered stale and removed. +- Retry: up to 2 seconds total, 200ms between attempts. +- Release: `os.Remove(lockPath)`. Idempotent (no error if already deleted). +- Lock acquired **after** DNS resolution, released **after** file write (or on error). diff --git a/specs/001-safety-reliability-refactor/data-model.md b/specs/001-safety-reliability-refactor/data-model.md new file mode 100644 index 0000000..c5b1033 --- /dev/null +++ b/specs/001-safety-reliability-refactor/data-model.md @@ -0,0 +1,160 @@ +# Data Model: Safety, Reliability & Idiomatic Refactor + +**Feature Branch**: `001-safety-reliability-refactor` +**Date**: 2026-03-03 + +## Entities + +### HostsFile + +Represents the system hosts file as parsed content sections. + +| Field | Type | Description | Constraints | +|-------|------|-------------|-------------| +| Path | `string` | Absolute path to the hosts file | OS-specific; determined by platform package | +| OriginalContent | `[]string` | Raw lines from the file as read | Immutable after read; used for change detection | +| PrefixContent | `[]string` | Lines before the managed block start marker | Preserved verbatim on write | +| ManagedContent | `[]string` | Lines between start and end markers (exclusive) | Modified by add/delete operations | +| PostfixContent | `[]string` | Lines after the managed block end marker | Preserved verbatim on write | +| HasManagedBlock | `bool` | Whether a valid managed block was found | `true` if both start and end markers present in order | + +**Validation Rules:** +- If start marker is present, end marker must also be present (and vice versa). +- End marker must appear after start marker. +- Duplicate start or end markers are not permitted. +- Start marker at line index 0 is a valid position (must use sentinel, not zero-check). + +**State Transitions:** +``` +[File Read] → Parse → [No Managed Block] → Add entries → [Has Managed Block] → Write + → [Has Managed Block] → Add/Delete entries → [Updated Managed Block] → Write + → [Corrupt Block] → ERROR (refuse to modify) +``` + +### DNSEntry + +A single IP-to-hostname mapping in the hosts file. + +| Field | Type | Description | Constraints | +|-------|------|-------------|-------------| +| IP | `string` | IPv4 address | Must be a valid IPv4 address; result of DNS resolution | +| Hostname | `string` | Fully qualified domain name | As provided by user (without trailing dot) | + +**String Representation:** `\t` (tab-separated, one per line in hosts file) + +**Validation Rules:** +- IP must be non-empty and a valid IPv4 address. +- Hostname must be non-empty and not contain whitespace. +- Entries are deduplicated by their full string representation (IP + hostname pair). + +### ManagedBlock + +The delimited section of the hosts file owned by this tool. + +| Field | Type | Description | Constraints | +|-------|------|-------------|-------------| +| StartMarker | `string` | Comment line marking block start | `# DNSHelper <<-> START CONFIG` (constant) | +| EndMarker | `string` | Comment line marking block end | `# DNSHelper <->> END CONFIG` (constant) | +| Entries | `[]DNSEntry` | DNS entries within the block | Zero or more; deduplicated | + +**Rules:** +- The managed block is only written if there are entries to include. +- When all entries are removed (delete all), the entire block (markers included) is removed. +- The block is always written as: start marker, entries (one per line), end marker. + +### ResolveResult + +The outcome of resolving a single hostname against a DNS server. + +| Field | Type | Description | Constraints | +|-------|------|-------------|-------------| +| Hostname | `string` | The hostname that was queried | As provided by user | +| IPs | `[]string` | Resolved IPv4 addresses | Deduplicated; empty if resolution failed | +| Error | `error` | Resolution error, if any | Non-nil on failure (timeout, NXDOMAIN, etc.) | + +**State Transitions:** +``` +[Query Sent] → A records returned → [Success: IPs populated] + → CNAME returned → Follow chain (max 10 depth) → [Success or Error] + → NXDOMAIN / timeout / error → [Failure: Error populated] +``` + +### BackupFile + +A temporary safety copy of the hosts file created before modification. + +| Field | Type | Description | Constraints | +|-------|------|-------------|-------------| +| Path | `string` | Absolute path to backup file | In executable's directory | +| SourcePath | `string` | Path of the file that was backed up | The hosts file path | +| Timestamp | `string` | Date stamp portion of filename | Format: `YYYYMMDD` | +| RandomSuffix | `string` | Short random string for uniqueness | 4 hex characters (e.g., `a7f3`) | + +**Naming Convention:** `hosts.bak.-` (e.g., `hosts.bak.20260303-a7f3`) + +**Lifecycle:** +``` +[Pre-Write] → Create backup copy → [Write Success] → Delete backup + → [Write Failure] → Original intact, clean up backup +``` + +### LockFile + +A file-based mutex for serializing concurrent access. + +| Field | Type | Description | Constraints | +|-------|------|-------------|-------------| +| Path | `string` | Absolute path to lock file | In executable's directory; `.dns-helper.lock` | +| PID | `int` | Process ID of the lock holder | Written for debugging; not used for staleness | +| CreatedAt | `time.Time` | When the lock was acquired | Approximated by file ModTime | + +**Parameters:** +- Stale timeout: 2 minutes +- Retry interval: 200ms +- Max wait: 2 seconds (10 retries) + +**Lifecycle:** +``` +[Pre-Lock] → Attempt create (O_EXCL) → [Acquired] → Hold during R/M/W → [Release: delete] + → [Exists, not stale] → Retry (up to 2s) → [Acquired or Error] + → [Exists, stale] → Break stale → Retry → [Acquired or Error] +``` + +## Relationships + +``` +CLI (main.go) + ├── validates input (host, server, subcommand) + ├── calls Resolver to get ResolveResult[] (before lock) + ├── acquires LockFile + ├── reads HostsFile → parses ManagedBlock + ├── modifies ManagedBlock (add DNSEntry[] / delete by hostname) + ├── creates BackupFile + ├── writes HostsFile atomically (temp + rename) + ├── deletes BackupFile on success + └── releases LockFile +``` + +## Operation Flows + +### Add Flow +1. Validate: `-host` and `-server` required and non-empty. +2. Resolve: For each hostname, query DNS server → collect `ResolveResult[]`. +3. Lock: Acquire lock file. +4. Read: Read hosts file → parse into sections. +5. Modify: Remove existing entries for the specified hostnames from managed content. Add new `DNSEntry[]` from resolve results. +6. Deduplicate: Remove duplicate entries by full line content. +7. Backup: Copy current hosts file to backup location. +8. Write: Assemble full content (prefix + managed block + postfix) → write to temp file → rename over hosts file. +9. Cleanup: Delete backup on success. Release lock. +10. Report: Print summary (entries added/updated). Exit non-zero if any hostname failed to resolve. + +### Delete Flow +1. Validate: `-host` required and non-empty, OR `all` keyword. +2. Lock: Acquire lock file. +3. Read: Read hosts file → parse into sections. +4. Modify: If specific hosts — remove matching lines from managed content. If `all` — clear managed content entirely (block markers will not be written). +5. Backup: Copy current hosts file to backup location. +6. Write: Assemble full content → write to temp file → rename. +7. Cleanup: Delete backup on success. Release lock. +8. Report: Print summary (entries removed). Report if no entries were found. diff --git a/specs/001-safety-reliability-refactor/plan.md b/specs/001-safety-reliability-refactor/plan.md new file mode 100644 index 0000000..4cc9032 --- /dev/null +++ b/specs/001-safety-reliability-refactor/plan.md @@ -0,0 +1,110 @@ +# Implementation Plan: Safety, Reliability & Idiomatic Refactor + +**Branch**: `001-safety-reliability-refactor` | **Date**: 2026-03-03 | **Spec**: [spec.md](spec.md) +**Input**: Feature specification from `/specs/001-safety-reliability-refactor/spec.md` + +## Summary + +Refactor the dns-helper CLI tool to address critical safety and reliability bugs — primarily the unsafe direct-write to the hosts file — and to modernize the codebase for idiomatic Go 1.20 patterns. Key changes: implement atomic write (temp file + rename) with pre-write backup, replace the `github.com/miekg/dns` dependency with `golang.org/x/net/dns/dnsmessage`, fix CNAME-chain duplicate IP bug, fix multi-host deletion bug, add input validation and proper error handling, add lock file for concurrency safety, remove dead code, and restructure the project into clear packages with testable interfaces per TDD. + +## Technical Context + +**Language/Version**: Go 1.20 (maximum; currently go.mod says 1.18, must be updated to 1.20) +**Primary Dependencies**: Go standard library, `golang.org/x/net/dns/dnsmessage` (replacing `github.com/miekg/dns`) +**Storage**: Local filesystem (system hosts file, backup files, lock file) +**Testing**: `go test ./...` with TDD (Red-Green-Refactor); interfaces for filesystem and DNS to enable fakes/stubs +**Target Platform**: Windows, Linux, macOS (cross-compiled single binary) +**Project Type**: CLI tool (single binary, no config files) +**Performance Goals**: N/A (single-shot CLI; DNS timeout default 5s) +**Constraints**: Go 1.20 ceiling (no 1.21+ features); zero unapproved external dependencies; hosts file must never be corrupted +**Scale/Scope**: ~500 LOC current, single-purpose utility; 6 source files + 3 platform-specific files + +## Constitution Check + +*GATE: Must pass before Phase 0 research. Re-check after Phase 1 design.* + +### Principle I: System File Safety +- **Status**: VIOLATION IN CURRENT CODE — `writeOverContentInFile()` truncates the hosts file directly (`O_WRONLY|O_TRUNC`). No backup is created. A crash during write leaves the file corrupted. +- **Plan compliance**: FR-001 (atomic write via temp+rename) and FR-002 (pre-write backup) directly address this. PASS after implementation. + +### Principle II: Standard Library First +- **Status**: VIOLATION IN CURRENT CODE — `github.com/miekg/dns` is an external dependency outside approved sources. The constitution notes it as a "grandfather clause" legacy dependency with preferred migration. +- **Plan compliance**: FR-014 replaces `miekg/dns` with `golang.org/x/net/dns/dnsmessage` (approved source). PASS after implementation. + +### Principle III: Go 1.20 Compatibility +- **Status**: VIOLATION IN CURRENT CODE — `go.mod` specifies `go 1.18`. While this is compatible, the spec requires updating to `go 1.20` (FR-010). +- **Plan compliance**: Update `go.mod` to `go 1.20`, verify all code and dependencies are compatible. Verify `golang.org/x/net/dns/dnsmessage` is available in a Go 1.20-compatible version. PASS after implementation. + +### Principle IV: Idiomatic Error Handling +- **Status**: VIOLATION IN CURRENT CODE — `fileoperations_linux.go` and `fileoperations_darwin.go` call `term()` (which calls `os.Exit`) instead of returning errors. `dns.go` prints errors directly via `fmt.Println` instead of returning them. `main.go` uses `defer os.Exit(1)` in `init()`. +- **Plan compliance**: FR-008 requires all platform code to return errors. All functions will return errors to callers; only `main()` calls `os.Exit`. PASS after implementation. + +### Principle V: Simplicity and Single Purpose +- **Status**: VIOLATION IN CURRENT CODE — Dead code exists: unused `ping` subcommand FlagSet, unused `IsNew` field in `workingData`, `fileExists` in shared scope duplicated/unused, `slicesEqual` may be unused, commented-out debug code in `dataprep.go`. +- **Plan compliance**: FR-009 removes all dead code. Tool remains a single-binary CLI. PASS after implementation. + +### Principle VI: Test-Driven Development +- **Status**: VIOLATION IN CURRENT CODE — Zero test files exist. No `*_test.go` files anywhere. +- **Plan compliance**: All new/refactored code will be developed with TDD. Functions interacting with filesystem/DNS will accept interfaces for testability. PASS after implementation. + +### Gate Evaluation +All six constitution principles have violations in the current code. All violations are explicitly addressed by the feature spec requirements (FR-001 through FR-016). No unjustified violations exist. **GATE: PASS** — proceed to Phase 0. + +## Project Structure + +### Documentation (this feature) + +```text +specs/001-safety-reliability-refactor/ +├── plan.md # This file +├── research.md # Phase 0 output +├── data-model.md # Phase 1 output +├── quickstart.md # Phase 1 output +├── contracts/ # Phase 1 output +└── tasks.md # Phase 2 output (created by /speckit.tasks) +``` + +### Source Code (repository root) + +```text +dns-helper/ +├── main.go # Entry point, CLI parsing, os.Exit +├── resolver/ +│ ├── resolver.go # DNS resolution interface + implementation +│ └── resolver_test.go # Tests for DNS resolution +├── hostfile/ +│ ├── hostfile.go # Hosts file read/parse/write (atomic + backup) +│ ├── hostfile_test.go # Tests for hosts file operations +│ ├── managed.go # Managed block parsing, insertion, removal +│ └── managed_test.go # Tests for managed block logic +├── platform/ +│ ├── platform.go # Interface for platform-specific operations +│ ├── platform_windows.go # Windows hosts file location +│ ├── platform_linux.go # Linux hosts file location +│ ├── platform_darwin.go # macOS hosts file location +│ └── platform_test.go # Platform tests +├── lockfile/ +│ ├── lockfile.go # Lock file acquisition/release +│ └── lockfile_test.go # Lock file tests +├── go.mod +└── go.sum +``` + +**Structure Decision**: Reorganize from flat single-package into logical sub-packages (`resolver`, `hostfile`, `platform`, `lockfile`). Each package has a single responsibility and testable interfaces. `main.go` remains at root as the CLI entry point orchestrating the packages. This follows Go conventions for small-to-medium CLI tools and satisfies Constitution Principles V (simplicity) and VI (TDD/testability). + +## Constitution Re-Check (Post Phase 1 Design) + +*All six principles re-evaluated after design phase. No new violations introduced.* + +| Principle | Status | Evidence | +|-----------|--------|----------| +| I. System File Safety | PASS | Atomic temp+rename in same directory (R-002). Pre-write backup in exe dir. Windows retry logic for sharing violations. Hosts file never opened for writing. | +| II. Standard Library First | PASS | `miekg/dns` replaced by `golang.org/x/net/dns/dnsmessage` (approved source, R-001). Lock file uses stdlib `os.OpenFile`. No unapproved dependencies remain. | +| III. Go 1.20 Compatibility | PASS | `golang.org/x/net` pinned to v0.35.0 (last Go 1.20-compatible release, R-006). `os.CreateTemp` available since Go 1.16. All APIs verified against Go 1.20. | +| IV. Idiomatic Error Handling | PASS | All package functions return errors. Platform implementations return errors to caller. Only `main()` calls `os.Exit()`. Errors include context (path, hostname, server). | +| V. Simplicity and Single Purpose | PASS | Four focused packages with clear single responsibilities. No speculative abstractions. Dead code eliminated. Remains single-binary CLI with subcommand+flags interface. | +| VI. Test-Driven Development | PASS | All packages have `_test.go` files in structure. Filesystem and DNS accessed through interfaces (R-007) enabling fakes/stubs. TDD workflow documented in quickstart. | + +## Complexity Tracking + +> No constitution violations remain unjustified. All violations are addressed by spec requirements. diff --git a/specs/001-safety-reliability-refactor/quickstart.md b/specs/001-safety-reliability-refactor/quickstart.md new file mode 100644 index 0000000..4ee8d9a --- /dev/null +++ b/specs/001-safety-reliability-refactor/quickstart.md @@ -0,0 +1,162 @@ +# Quickstart: Safety, Reliability & Idiomatic Refactor + +**Feature Branch**: `001-safety-reliability-refactor` +**Date**: 2026-03-03 + +## Prerequisites + +- **Go 1.20** toolchain installed (must be exactly 1.20.x, not newer) +- Administrator/root privileges (required to write the hosts file) +- Git + +## Setup + +```bash +# Clone and switch to feature branch +git checkout 001-safety-reliability-refactor + +# Verify Go version +go version +# Expected: go version go1.20.x ... + +# Download dependencies +go mod download + +# Run all tests +go test ./... + +# Build +go build -o dns-helper . +``` + +## Project Structure After Refactor + +``` +dns-helper/ +├── main.go # Entry point, CLI parsing, os.Exit +├── resolver/ +│ ├── resolver.go # DNS resolution via golang.org/x/net/dns/dnsmessage +│ └── resolver_test.go +├── hostfile/ +│ ├── hostfile.go # Hosts file read/write (atomic pattern) +│ ├── hostfile_test.go +│ ├── managed.go # Managed block parsing, add, remove +│ └── managed_test.go +├── platform/ +│ ├── platform.go # Interface definition +│ ├── platform_windows.go # Windows hosts file path +│ ├── platform_linux.go # Linux hosts file path +│ ├── platform_darwin.go # macOS hosts file path +│ └── platform_test.go +├── lockfile/ +│ ├── lockfile.go # Lock file acquisition/release +│ └── lockfile_test.go +├── go.mod # go 1.20, golang.org/x/net v0.35.0 +└── go.sum +``` + +## Key Implementation Patterns + +### TDD Workflow (Constitution Principle VI) + +Every change follows Red-Green-Refactor: + +1. **Red**: Write a failing test that specifies the desired behavior. +2. **Green**: Write the minimum code to make the test pass. +3. **Refactor**: Clean up while keeping tests green. + +```bash +# Run tests for a specific package +go test ./resolver/ +go test ./hostfile/ +go test ./lockfile/ + +# Run all tests +go test ./... +``` + +### Error Handling (Constitution Principle IV) + +All functions return errors. Only `main()` calls `os.Exit()`: + +```go +// WRONG — do not do this in any package +func doSomething() { + if err != nil { + fmt.Println(err) + os.Exit(1) // PROHIBITED outside main() + } +} + +// CORRECT — return errors to caller +func doSomething() error { + if err != nil { + return fmt.Errorf("doing something: %w", err) + } + return nil +} +``` + +### Testable Interfaces (Constitution Principle VI) + +Functions that interact with filesystem or DNS accept interfaces: + +```go +// Production: uses real OS calls +manager := hostfile.NewManager(hostfile.NewOSFileSystem()) + +// Test: uses in-memory fake +manager := hostfile.NewManager(fakeFS) +``` + +### Atomic Write Sequence + +``` +1. Backup: copy hosts → /hosts.bak.- +2. Write: assemble content → temp file (same dir as hosts) +3. Sync: temp.Sync() +4. Chmod: match original permissions +5. Close: temp.Close() +6. Rename: os.Rename(temp, hosts) [retry on Windows] +7. Clean: delete backup on success +``` + +## Usage Examples + +```bash +# Add DNS entries +dns-helper add -host myapp.example.com -server 10.0.0.53 + +# Add multiple hosts +dns-helper add -host host1.example.com,host2.example.com -server dns.internal + +# Delete specific host entries +dns-helper delete -host myapp.example.com + +# Delete all managed entries +dns-helper delete all +``` + +## Build for All Platforms + +```bash +# Windows +GOOS=windows GOARCH=amd64 go build -o bin/windows/dns-helper.exe . + +# Linux +GOOS=linux GOARCH=amd64 go build -o bin/linux/dns-helper . + +# macOS +GOOS=darwin GOARCH=amd64 go build -o bin/macos/dns-helper . +``` + +## Dependency Management + +```bash +# After removing miekg/dns and adding dnsmessage usage: +go mod tidy + +# Verify no unapproved dependencies +go list -m all +# Should only show: golang.org/x/net, golang.org/x/sys (transitive) +``` diff --git a/specs/001-safety-reliability-refactor/research.md b/specs/001-safety-reliability-refactor/research.md new file mode 100644 index 0000000..634970e --- /dev/null +++ b/specs/001-safety-reliability-refactor/research.md @@ -0,0 +1,326 @@ +# Research: Safety, Reliability & Idiomatic Refactor + +**Feature Branch**: `001-safety-reliability-refactor` +**Date**: 2026-03-03 + +## R-001: Replacing `github.com/miekg/dns` with `golang.org/x/net/dns/dnsmessage` + +### Decision +Use `golang.org/x/net/dns/dnsmessage` (part of the approved `golang.org/x` source) to replace all DNS query/response functionality currently provided by `github.com/miekg/dns`. + +### Rationale +- `dnsmessage` provides low-level DNS message building and parsing (A, CNAME, and all standard record types via `Message.Pack()`/`Message.Unpack()` or the streaming `Builder`/`Parser` API). +- The current usage surface of `miekg/dns` is narrow: build a query, send over UDP, parse A/CNAME responses. `dnsmessage` handles all of this. +- Eliminates 4 transitive dependencies (`x/mod`, `x/sys`, `x/tools` pulled in by `miekg/dns`) from the dependency tree. +- `golang.org/x/net` is already in the project's dependency list and is an approved source per the constitution. + +### Go 1.20 Compatibility +- **`golang.org/x/net v0.35.0`** is the latest version with `go 1.18` directive (compatible with Go 1.20). Starting at `v0.36.0`, the module requires `go 1.23.0`. +- The `dnsmessage` sub-package API is stable and has not changed materially across these versions. +- **Pin to `golang.org/x/net v0.35.0`** in `go.mod`. + +### Implementation Pattern +- Use `Message.Pack()` to build queries (simpler API, adequate for single-query CLI tool). +- Use `Message.Unpack()` to parse responses. +- Send raw DNS packets over UDP using `net.DialTimeout("udp", server+":53", timeout)`. No length prefix needed for UDP (only TCP requires a 2-byte length prefix). +- DNS names must be FQDN with trailing dot: `dnsmessage.MustNewName("example.com.")`. +- `AResource.A` is `[4]byte`, convert to string via `net.IP(r.A[:]).String()`. +- Response buffer of 1232 bytes is sufficient for standard UDP DNS responses. +- Validate response `Header.ID` matches query ID to prevent spoofing. +- Default 5-second timeout via `conn.SetDeadline()`. + +### Key Type Mappings (miekg/dns → dnsmessage) + +| miekg/dns | dnsmessage | +|-----------|------------| +| `dns.Client{}` + `c.Exchange()` | `net.DialTimeout("udp", ...)` + `conn.Write()`/`conn.Read()` | +| `dns.Msg{}.SetQuestion()` | `dnsmessage.Message{}.Pack()` | +| `dns.TypeA` | `dnsmessage.TypeA` | +| `dns.TypeCNAME` | `dnsmessage.TypeCNAME` | +| `*dns.A` → `rec.A.String()` | `*dnsmessage.AResource` → `net.IP(r.A[:]).String()` | +| `*dns.CNAME` → `rec.Target` | `*dnsmessage.CNAMEResource` → `r.CNAME.String()` | +| `dns.RcodeSuccess` | `dnsmessage.RCodeSuccess` | +| `dns.RcodeToString[code]` | `rcode.String()` (method on `RCode` type) | + +### Alternatives Considered +1. **`net.Resolver` with custom `Dialer`**: The Go stdlib `net.Resolver` does not support querying a specific DNS server by address without hacky workarounds. The tool's core purpose requires directing queries to a user-specified resolver. +2. **Keep `miekg/dns`**: Violates Constitution Principle II (Standard Library First). The constitution explicitly calls this a "grandfather clause" dependency with preferred migration. +3. **Raw UDP + manual DNS wire format**: Would work but requires implementing DNS message serialization from scratch. `dnsmessage` already does this correctly and is an approved package. + +--- + +## R-002: Atomic File Write Strategy (temp-file-and-rename) + +### Decision +Write new hosts file content to a temporary file in the same directory, then `os.Rename()` over the original. Combined with a pre-write backup in the executable's directory. + +### Rationale +- `os.Rename()` on same-volume is atomic (or effectively atomic) on all three target platforms: + - **Linux**: POSIX `rename(2)` is atomic. Guaranteed by the kernel. + - **macOS**: Darwin `rename(2)` follows POSIX semantics. Atomic on both APFS and HFS+. + - **Windows**: Go's `os.Rename` calls `MoveFileEx` with `MOVEFILE_REPLACE_EXISTING`. Effectively atomic on NTFS (MFT metadata update), though Microsoft does not formally guarantee atomicity. +- The temp file **must** be in the same directory as the hosts file to avoid cross-filesystem/cross-volume failures (`EXDEV` on Unix, copy+delete on Windows). +- The original hosts file is never opened for writing or truncated. + +### Go 1.20 Compatibility +- `os.CreateTemp(dir, pattern)` available since Go 1.16. +- `os.Rename()` stable across all Go versions. +- No known bugs in Go 1.20 affecting this pattern. + +### Implementation Details + +1. **Temp file creation**: `os.CreateTemp(filepath.Dir(hostsPath), ".dns-helper-tmp-*")` — creates file in same directory with recognizable prefix. +2. **Permissions**: The renamed file retains the temp file's permissions, NOT the original's. Must `os.Stat(hostsPath)` to get original mode, then `os.Chmod(tempFile, originalMode)` before renaming. +3. **Close before rename**: Required on Windows — cannot rename a file that's still open by the writing process. +4. **Sync before close**: Call `file.Sync()` for durability before closing. +5. **Cleanup on failure**: Use a `defer` with a success flag to remove the temp file if any step fails. +6. **Windows retry**: `os.Rename` on the hosts file may fail with `ERROR_SHARING_VIOLATION` if Windows Defender or the DNS Client service has a transient read lock. Implement 2-3 retry attempts with 100-200ms delay. +7. **Orphan cleanup**: On startup, optionally clean stale `.dns-helper-tmp-*` files in the hosts directory. + +### Backup Strategy (FR-002) +- Before any write operation, copy the current hosts file to `/hosts.bak.-` (e.g., `hosts.bak.20260303-a7f3`). +- The backup is a regular file copy (not rename), since it's in a different directory (exe dir vs hosts file dir). +- On successful rename of temp over hosts file: delete the backup. +- On rename failure: the original hosts file was never modified. Clean up temp file and backup. +- The date+random naming handles concurrent instances. + +### Alternatives Considered +1. **Write directly to hosts file (current approach)**: Uses `O_WRONLY|O_TRUNC`, which immediately truncates the file. A crash during write leaves it empty/partial. This is the bug being fixed. +2. **Backup-only (no atomic rename)**: Would still truncate the hosts file during write. The backup only helps after a failure, not during. +3. **Write to temp, then copy content over original**: The copy step is not atomic — a crash during copy is the same as a crash during direct write. + +--- + +## R-003: Lock File Implementation + +### Decision +Use `os.OpenFile` with `O_CREATE|O_EXCL` to create a lock file in the executable's directory. Use timeout-based staleness detection (file modification time). + +### Rationale +- `O_CREATE|O_EXCL` is atomic on all three platforms (Windows maps to `CREATE_NEW`, Unix maps to `O_CREAT|O_EXCL`). +- Single code path — no platform-specific build files needed for the lock mechanism. +- Timeout-based staleness avoids the cross-platform pitfalls of PID-based detection (Windows doesn't support `Signal(0)` for process existence checks without `x/sys`). + +### Implementation Details + +| Parameter | Value | Rationale | +|-----------|-------|-----------| +| Lock file name | `.dns-helper.lock` | Hidden file, recognizable prefix | +| Lock location | Executable's directory | Same as backup files; writable by admin/root (required anyway) | +| Stale timeout | 2 minutes | Lock held only during read-modify-write (<1s normally). 2min is very conservative. | +| Retry interval | 200ms | Short enough to be responsive, long enough to avoid busy-wait | +| Max wait | 2 seconds | 10 retries. Enough for another instance's write cycle. | + +**Lock protocol per FR-016:** +1. Parse args and validate input. +2. Perform DNS resolution (before lock acquisition to minimize lock hold time). +3. Acquire lock (retry up to 2s, break stale locks >2min). +4. Read hosts file → modify content → write temp file → rename over hosts file. +5. Release lock (delete lock file). +6. Lock release in `defer` to handle error paths and panics. + +**Staleness detection:** +- `os.Stat(lockFile).ModTime()` — if older than 2 minutes, `os.Remove()` and retry. +- Cross-platform via Go stdlib. No platform-specific code needed. + +**Signal handling:** +- `defer lock.Release()` handles normal exit and errors. +- For SIGKILL/power loss, the stale lock persists. Next invocation detects and breaks it after 2 minutes. +- Write PID to lock file for debugging (human inspection), but don't use it for staleness logic. + +### Alternatives Considered +1. **Advisory file locking (`flock`/`LockFileEx`)**: Auto-releases on crash (no stale lock problem), but requires 3 platform-specific build files. `syscall.Flock` doesn't exist on Windows. Adds complexity for marginal benefit. +2. **PID-based staleness**: More precise than timeout, but `os.FindProcess` + `Signal(0)` doesn't work on Windows without `golang.org/x/sys/windows`. Defeats the single-code-path advantage. +3. **No locking**: Risk of two instances writing the hosts file simultaneously. The spec explicitly requires serialization (FR-016). + +--- + +## R-004: Multi-Host Deletion Bug Analysis + +### Decision +Fix the nested loop logic in `removeHostsFromExistingContent` by inverting the loop order (iterate lines on the outside, check all hosts on the inside). + +### Rationale — Bug Analysis + +Current code: +```go +func removeHostsFromExistingContent(data *workingData) { + var updatedContent []string + for _, host := range data.Hosts { + for _, line := range data.ExistingContent { + if !strings.Contains(line, host) { + updatedContent = append(updatedContent, line) + } + } + } + data.NewContent = updatedContent +} +``` + +**Bug**: The inner loop always iterates over `data.ExistingContent` (the original, unfiltered content). `updatedContent` is never reset between outer iterations. Result: +- Lines removed by one host are re-added by another host's pass (re-scans original). +- Surviving lines are duplicated once per host in `data.Hosts`. + +**Trace** with `Hosts = ["host1", "host2"]`, `ExistingContent = ["1.1.1.1\thost1", "2.2.2.2\thost2", "3.3.3.3\thost3"]`: +- Pass 1 (host1): keeps `host2`, `host3` → `updatedContent = [host2, host3]` +- Pass 2 (host2): re-scans original, keeps `host1`, `host3` → `updatedContent = [host2, host3, host1, host3]` +- **Result**: Both hosts survive, `host3` is duplicated. Every invariant violated. + +### Correct Algorithm +Iterate lines on the outside; for each line, check if it matches ANY host: +```go +func removeHostsFromExistingContent(data *workingData) { + var updatedContent []string + for _, line := range data.ExistingContent { + shouldRemove := false + for _, host := range data.Hosts { + if strings.Contains(line, host) { + shouldRemove = true + break + } + } + if !shouldRemove { + updatedContent = append(updatedContent, line) + } + } + data.NewContent = updatedContent +} +``` + +--- + +## R-005: CNAME Resolution Duplicate IP Bug Analysis + +### Decision +Fix the self-appending loop in the CNAME resolution path of `lookupIP()` and add a max-depth limit for CNAME chain following. + +### Rationale — Bug Analysis + +Current code: +```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**: `ips` is the result of the recursive `lookupIP` call. The `for range` then iterates over `ips` and appends each element back to itself. In Go, `for range` evaluates the slice length once at the start, so this is not infinite — it exactly doubles every element. With `ips = ["10.0.0.1", "10.0.0.2"]`, the result is `["10.0.0.1", "10.0.0.2", "10.0.0.1", "10.0.0.2"]`. + +**Additional issue**: No CNAME chain depth limit. Circular CNAMEs would cause infinite recursion. + +### Correct Algorithm +```go +for _, ans := range r.Answer { + if rec, ok := ans.(*dns.CNAME); ok { + cnameIPs := lookupIP(rec.Target, resolver) + ips = append(ips, cnameIPs...) + } +} +``` +Plus a max-depth parameter (10 levels) to prevent infinite CNAME recursion. + +--- + +## R-006: Go 1.20 Dependency Cleanup + +### Decision +Update `go.mod` to `go 1.20`. Pin `golang.org/x/net` to `v0.35.0`. Remove `github.com/miekg/dns` and all its transitive dependencies. + +### Rationale + +Current `go.mod` dependencies: +``` +github.com/miekg/dns v1.1.59 ← removing (FR-014) +golang.org/x/mod v0.16.0 ← transitive dep of miekg/dns, will be removed +golang.org/x/net v0.22.0 ← keeping, updating to v0.35.0 +golang.org/x/sys v0.18.0 ← transitive dep of x/net, will be updated +golang.org/x/tools v0.19.0 ← transitive dep of miekg/dns, will be removed +``` + +After cleanup: +``` +golang.org/x/net v0.35.0 ← for dns/dnsmessage +golang.org/x/sys (transitive) ← pulled by x/net +``` + +`golang.org/x/mod` and `golang.org/x/tools` are only needed by `miekg/dns` and will be removed by `go mod tidy` after removing the `miekg/dns` import. + +### Version Verification +- `golang.org/x/net v0.35.0`: `go 1.18` directive — compatible with Go 1.20. +- `golang.org/x/net v0.36.0`: `go 1.23.0` directive — **NOT compatible**. Must pin at v0.35.0. + +--- + +## R-007: Testability Design for TDD (Constitution Principle VI) + +### Decision +Define interfaces for external dependencies (filesystem, DNS, lock) to enable testing with fakes/stubs. All business logic operates on interfaces, not concrete OS calls. + +### Rationale +Constitution Principle VI mandates TDD with testable interfaces. The current codebase has zero tests and directly calls OS-level I/O from business logic. + +### Interface Design + +**Filesystem interface** (for `hostfile` package): +```go +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 +} +``` + +**Resolver interface** (for `resolver` package): +```go +type Resolver interface { + LookupIP(hostname, server string) ([]string, error) +} +``` + +**Lock interface** (for `lockfile` package): +```go +type Lock interface { + Release() error +} +type Locker interface { + Acquire(dir string) (Lock, error) +} +``` + +Production code uses real implementations. Tests inject fakes that simulate errors, crashes, and edge cases without touching the real filesystem or DNS. + +### Test Categories +1. **Unit tests**: Managed block parsing, line removal, deduplication, input validation — pure logic, no I/O. +2. **Integration tests with fakes**: Atomic write sequence, backup creation/cleanup, lock acquisition/release — use in-memory filesystem fakes. +3. **Platform tests**: `getHostsFileLocation()` on each OS — verify return value and error handling against build tags. + +--- + +## R-008: Managed Block Corruption Detection (FR-011) + +### Decision +Detect and report four corruption scenarios for managed block markers. Refuse to modify the file when corruption is detected. + +### Rationale +The current `extractLinesToEdit` function detects "start without end" via a `fileCorrupted` flag, but the error message ("content has been corrupted") is vague and doesn't cover all cases. + +### Corruption Scenarios + +| Scenario | Detection | Error Message | +|----------|-----------|---------------| +| Start marker without end marker | `startIndex > 0 && endIndex == 0` | "managed block is corrupt: start marker found at line N but no end marker" | +| End marker without start marker | `startIndex == 0 && endIndex > 0` | "managed block is corrupt: end marker found at line N but no start marker" | +| End marker before start marker | `endIndex < startIndex` | "managed block is corrupt: end marker (line N) appears before start marker (line M)" | +| Duplicate start or end markers | Multiple occurrences detected during scan | "managed block is corrupt: duplicate start/end markers found" | + +All corruption scenarios must result in an error returned to the caller. The hosts file must not be modified. + +### Current Code Bug +The current code uses `startIndex == 0` to mean "not found," but index 0 is a valid line position (the first line). Should use a sentinel value (-1) or a boolean flag for "found/not found." diff --git a/specs/001-safety-reliability-refactor/spec.md b/specs/001-safety-reliability-refactor/spec.md new file mode 100644 index 0000000..d42abf2 --- /dev/null +++ b/specs/001-safety-reliability-refactor/spec.md @@ -0,0 +1,178 @@ +# Feature Specification: Safety, Reliability & Idiomatic Refactor + +**Feature Branch**: `001-safety-reliability-refactor` +**Created**: 2026-03-03 +**Status**: Draft +**Input**: User description: "Address safety and reliability bugs, refactor for idiomatic Go 1.20 patterns, improve project structure and developer maintainability" + +## User Scenarios & Testing *(mandatory)* + +### User Story 1 - Hosts File Is Never Corrupted or Lost (Priority: P1) + +As a system administrator, I need the hosts file to remain intact even if the tool crashes, loses power, or encounters a disk error mid-write, because a corrupted hosts file can break all name resolution on the machine and require manual recovery. + +**Why this priority**: The hosts file is a critical system resource. Losing it can render a machine unable to resolve any DNS names, disrupting all network-dependent software. Protecting it is the single most important improvement. + +**Independent Test**: Run the tool to add a DNS entry, then simulate an interruption (e.g., kill the process during write). Verify the original hosts file is unchanged or correctly updated — never partially written or empty. + +**Acceptance Scenarios**: + +1. **Given** an existing hosts file with valid content, **When** the tool writes updated entries successfully, **Then** the hosts file contains the correct updated content, no data from the original file is lost, and the hosts file was never truncated or partially written at any point. +2. **Given** an existing hosts file with valid content, **When** the tool is interrupted (crash, signal, power loss) during the write operation, **Then** the original hosts file remains intact and unmodified because the write targeted a temporary file, not the hosts file directly. +3. **Given** the disk is full or the write fails for any reason, **When** the tool attempts to save the hosts file, **Then** an error is reported to the user, the original hosts file is not altered, and any temporary files are cleaned up. +4. **Given** an existing hosts file, **When** the tool prepares to write, **Then** a temporary backup copy of the current hosts file is created before any modification begins. On successful rename of the temp file over the hosts file, the backup is deleted. On failure, the backup is available and the original hosts file was never modified. + +--- + +### User Story 2 - Correct DNS Resolution Results (Priority: P1) + +As a user adding DNS entries, I need the resolved IP addresses written to the hosts file to be accurate and free of duplicates, because incorrect entries will cause connectivity failures — the exact problem this tool is meant to solve. + +**Why this priority**: The core value proposition of the tool is providing correct, current DNS mappings. Bugs that produce duplicate or incorrect IPs directly undermine the tool's purpose. + +**Independent Test**: Add a hostname that resolves via CNAME chain to multiple IPs using a specific DNS server. Verify the hosts file contains exactly the correct set of unique IPs with no duplicates. + +**Acceptance Scenarios**: + +1. **Given** a hostname that resolves directly to A records, **When** the user runs `add -host -server `, **Then** the hosts file contains one entry per unique IP address for that hostname. +2. **Given** a hostname that resolves via a CNAME chain to A records, **When** the user runs `add -host -server `, **Then** the CNAME chain is followed correctly and the final A record IPs are written without duplicates. +3. **Given** multiple hostnames specified via comma-separated values, **When** the user runs `add -host host1,host2 -server `, **Then** all hostnames are resolved and each gets correct, deduplicated entries in the hosts file. +4. **Given** the DNS resolution layer has been reimplemented using only approved dependencies, **When** any hostname is resolved, **Then** the results are identical to the previous implementation (same IPs, same CNAME handling, same error reporting). +5. **Given** multiple hostnames where some resolve successfully and others fail, **When** the user runs `add -host host1,host2 -server `, **Then** entries are written for the successfully resolved hostnames, a warning identifying each failed hostname is printed to stdout, and the tool exits with a non-zero exit code. + +--- + +### User Story 3 - Reliable Deletion of Managed Entries (Priority: P2) + +As a user, when I delete managed DNS entries (one hostname or all), I need the tool to correctly remove only the entries it previously added, leaving all other hosts file content untouched. + +**Why this priority**: Incorrect deletion could remove entries the user or other tools added, or leave orphaned entries behind. This must work correctly to maintain trust in the tool. + +**Independent Test**: Add entries for two hostnames, then delete one. Verify the deleted hostname's entries are gone and the other hostname's entries remain. Then test `delete all` and verify only the managed block is removed. + +**Acceptance Scenarios**: + +1. **Given** the hosts file contains managed entries for hosts A and B, **When** the user deletes host A, **Then** only host A's entries are removed; host B's managed entries and all unmanaged content remain intact. +2. **Given** the hosts file contains managed entries for multiple hosts, **When** the user runs `delete all`, **Then** the entire managed block (start marker through end marker) is removed and all unmanaged content remains intact. +3. **Given** the hosts file contains no managed entries, **When** the user runs `delete -host `, **Then** the tool reports that no entries were found and the hosts file is not modified. + +--- + +### User Story 4 - Clear Error Messages and Input Validation (Priority: P2) + +As a user, when I provide invalid or incomplete input (missing flags, empty values, unresolvable hostnames), I need a clear error message explaining what went wrong and how to fix it, and the tool must not proceed with partial or incorrect data. + +**Why this priority**: Without proper validation, the tool can silently produce incorrect results or corrupt the managed block. Clear errors prevent misuse and reduce troubleshooting time. + +**Independent Test**: Run the tool with missing `-host`, empty `-server`, an unresolvable hostname, and invalid subcommands. Verify each produces a descriptive error, returns a non-zero exit code, and does not modify the hosts file. + +**Acceptance Scenarios**: + +1. **Given** the user runs `add` without a `-host` flag, **When** the command executes, **Then** an error is displayed explaining that `-host` is required, and the hosts file is not modified. +2. **Given** the user runs `add -host example.com` without a `-server` flag, **When** the command executes, **Then** an error is displayed explaining that `-server` is required for the add command, and the hosts file is not modified. +3. **Given** the user provides an unrecognized subcommand, **When** the command executes, **Then** a usage message is displayed and the hosts file is not modified. +4. **Given** the hostname cannot be resolved by the specified DNS server, **When** the command executes, **Then** an error is displayed indicating the resolution failure, and no entry is written for that hostname. + +--- + +### User Story 5 - Consistent Behavior Across Operating Systems (Priority: P3) + +As a user running the tool on Windows, Linux, or macOS, I need the tool to behave consistently — locating the hosts file, reporting errors, and handling edge cases the same way regardless of platform. + +**Why this priority**: Inconsistent platform behavior makes the tool unreliable and harder to maintain. Standardizing the platform layer improves confidence and reduces platform-specific bugs. + +**Independent Test**: Build and run the tool on each supported platform. Verify that error conditions (missing hosts file, permission denied) produce equivalent error messages and exit codes on all platforms. + +**Acceptance Scenarios**: + +1. **Given** the tool is running on any supported platform, **When** the hosts file cannot be found at the expected location, **Then** an error is returned to the caller with a descriptive message (not a process exit from within the platform code). +2. **Given** the tool is running on any supported platform, **When** a permission error prevents reading or writing the hosts file, **Then** a clear error message is displayed and the tool exits cleanly. + +--- + +### User Story 6 - Clean, Maintainable Project Structure (Priority: P3) + +As a developer maintaining this tool, I need the codebase to follow idiomatic patterns and be organized into logical units so that future changes (new features, bug fixes) are straightforward and low-risk. + +**Why this priority**: The current codebase has dead code, inconsistent patterns, and tightly coupled logic that makes changes risky. A clean structure reduces the cost and risk of future work. + +**Independent Test**: Review the refactored codebase for: no dead code, consistent error handling patterns, clear separation of concerns, and all tests passing after reorganization. + +**Acceptance Scenarios**: + +1. **Given** the refactored codebase, **When** a developer reads the project, **Then** each file/package has a clear, single responsibility and no unused exports or dead code exist. +2. **Given** the refactored codebase, **When** errors occur in any function, **Then** errors are returned to the caller (not handled by terminating the process internally) and the caller decides how to handle them. +3. **Given** the project targets Go 1.20, **When** the project is built, **Then** it compiles cleanly with no deprecation warnings and uses language features available up to Go 1.20. + +--- + +### Edge Cases + +- What happens when the hosts file is read-only or locked by another process? +- What happens when the DNS server is unreachable or times out? +- What happens when the managed block markers are present but malformed (e.g., start without end, end without start, duplicates)? +- What happens when the hosts file contains the managed block markers inside a comment added by someone else? +- What happens when the tool is run concurrently (two instances at the same time)? → Serialized via lock file; second instance waits briefly then fails if lock unavailable. +- What happens when the hostname resolves to zero IP addresses (NXDOMAIN)? +- What happens when the `-host` value contains whitespace or special characters? + +## Requirements *(mandatory)* + +### Functional Requirements + +- **FR-001**: The tool MUST write new hosts file content to a temporary file in the same directory as the hosts file, then rename the temporary file over the original. The hosts file MUST never be truncated or written to directly. If the rename fails, the temporary file MUST be cleaned up and an error reported. +- **FR-002**: The tool MUST create a temporary backup of the current hosts file before performing any modification. The backup MUST be stored in the same directory as the tool's binary executable, named with a date stamp and short random string (e.g., `hosts.bak.20260303-a7f3`) to avoid collisions from concurrent runs. On successful rename of the temp file over the hosts file, the backup MUST be deleted. If the rename fails, the original hosts file was never modified so the backup serves as a belt-and-suspenders safety net; it MUST be deleted after confirming the original is intact. +- **FR-003**: The tool MUST correctly follow CNAME chains during DNS resolution without producing duplicate IP entries. +- **FR-004**: The tool MUST correctly remove entries for all specified hostnames when deleting from a multi-host managed block (not just the last hostname processed). +- **FR-005**: The tool MUST NOT execute the hosts file write operation when an unrecognized subcommand is provided. +- **FR-006**: The tool MUST validate that required flags (`-host` for add/delete, `-server` for add) are present and non-empty before proceeding. +- **FR-007**: The tool MUST terminate immediately upon encountering a fatal error, without continuing to execute subsequent operations. +- **FR-008**: All platform-specific code MUST return errors to the caller rather than terminating the process directly. +- **FR-009**: The tool MUST remove all dead/unused code (unused `ping` subcommand, unused `IsNew` field, unused `fileExists` in shared scope). +- **FR-010**: The tool MUST target Go 1.20 as the minimum language version and update dependencies accordingly. +- **FR-011**: The tool MUST detect and report corrupt managed block markers (start without end, end without start) and refuse to modify the file. +- **FR-012**: The tool MUST deduplicate resolved IP entries before writing them to the hosts file. +- **FR-013**: The tool MUST provide an informational summary of changes made (entries added, entries removed) after a successful operation. +- **FR-014**: The tool MUST eliminate the `github.com/miekg/dns` dependency by reimplementing DNS query functionality using only the Go standard library or approved sources (`golang.org/x/*`). The replacement MUST support sending A and CNAME queries to a user-specified DNS server and parsing the responses. +- **FR-015**: When resolving multiple hostnames in a single invocation, the tool MUST write entries for all hostnames that resolved successfully, print a warning to stdout identifying each hostname that failed to resolve, and exit with a non-zero exit code to indicate partial failure. +- **FR-016**: The tool MUST use a lock file in the executable's directory to serialize concurrent access to the hosts file. DNS resolution MUST be performed before acquiring the lock so that the lock is held only during the file read-modify-write cycle. If the lock cannot be acquired after a brief wait, the tool MUST fail with a descriptive error. The lock MUST be released (and the lock file deleted) on both success and failure paths. + +### Key Entities + +- **Hosts File**: The system DNS resolution file (`/etc/hosts` or `%SystemRoot%\System32\drivers\etc\hosts`). Contains unmanaged content (user/system entries) and an optional managed block owned by this tool. +- **Managed Block**: A clearly delimited section within the hosts file, bounded by start and end marker comments, containing only entries written by this tool. +- **DNS Entry**: A mapping of an IP address to a hostname, represented as a single line in the hosts file (e.g., `1.2.3.4\texample.com`). +- **Resolver**: An external DNS server used to look up authoritative IP addresses for a given hostname. +- **Backup File**: A temporary copy of the hosts file created in the tool's executable directory before each modification, used to restore the original if the write fails. Named with a date stamp and random string to handle concurrent runs (e.g., `hosts.bak.20260303-a7f3`). Deleted after a successful write. + +## Assumptions + +- The tool runs with sufficient permissions to read and write the hosts file (typically requires administrator/root privileges). Permission errors are reported clearly but obtaining privileges is the user's responsibility. +- The backup file is a temporary rollback mechanism, not a persistent archive. It is stored in the same directory as the tool's binary executable (not alongside the hosts file) and is deleted after a successful write. The filename uses a date stamp plus a short random string to avoid collisions when multiple instances run concurrently. +- Atomic write via temp-file-and-rename combined with a pre-write backup provides two layers of safety. The atomic rename ensures the hosts file is never in a partially-written state. The backup in the executable's directory is a belt-and-suspenders safety net for edge cases where the rename itself fails (e.g., Windows file locking). On success, the backup is deleted. On rename failure, the original hosts file was never touched; the temp file and backup are cleaned up. +- "Go 1.20" is the maximum version to target for legacy system compatibility. No features exclusive to Go 1.21+ will be used. +- The tool will continue to be a single-binary CLI application with no external configuration files or services. +- DNS resolution timeout will use a reasonable default (5 seconds) since no timeout is currently configured. +- Concurrent access is serialized via a lock file in the executable's directory. DNS resolution is performed before acquiring the lock to minimize lock hold time. The lock covers only the file read-modify-write-rename cycle. If a stale lock file is detected (e.g., from a crashed instance), the tool may need a timeout-based expiry or PID check to recover — the specific staleness strategy will be determined during planning. +- The `github.com/miekg/dns` dependency is the only unapproved external package. Its usage surface is narrow (build query, send over UDP, parse A/CNAME response). Replacement will use `golang.org/x/net/dns/dnsmessage` (an approved source per the project constitution) which provides low-level DNS message construction and parsing sufficient for this use case. + +## Success Criteria *(mandatory)* + +### Measurable Outcomes + +- **SC-001**: The hosts file is never left empty, truncated, or partially written — the file transitions directly from old content to new content via atomic rename. Verified by kill-during-write testing. +- **SC-002**: CNAME-chain resolution produces zero duplicate IP entries in the hosts file. Verified by testing with hostnames that resolve through CNAME records. +- **SC-003**: Deleting one hostname from a managed block with multiple hostnames removes exactly the targeted hostname's entries and preserves all others. Verified with multi-host test scenarios. +- **SC-004**: Every invalid input combination (missing host, missing server, empty values, unknown subcommand) produces a descriptive error message and a non-zero exit code without modifying the hosts file. +- **SC-005**: All platform-specific functions (Windows, Linux, macOS) exhibit identical error-reporting behavior — returning errors to the caller rather than terminating directly. +- **SC-006**: The project compiles cleanly with Go 1.20 with no dead code, no unused variables, no deprecated API usage, and zero unapproved external dependencies. +- **SC-007**: On write failure, the original hosts file content is restored from the temporary backup. On write success, the temporary backup is deleted and no residual backup files remain. + +## Clarifications + +### Session 2026-03-03 + +- Q: How should the tool handle partial DNS resolution failure across multiple hostnames? → A: Write entries for successfully resolved hostnames; print warning to stdout identifying each failed hostname; exit with non-zero code. +- Q: How many backup copies of the hosts file should the tool retain? → A: None persistently. The backup is a temporary rollback file stored in the executable's directory, named with date + random string (e.g., `hosts.bak.20260303-a7f3`). Deleted on successful write; used to restore then deleted on failed write. +- Q: Should the tool use atomic rename, backup-and-restore, or both? → A: Both (Option C). Write backup to exe directory, write new content to temp file in hosts file directory, rename temp over original. On success delete backup. On rename failure original was never touched; clean up temp file and backup. +- Q: Should the tool use file locking to prevent concurrent modification of the hosts file? → A: Yes (Option A). Use a lock file in the executable's directory. DNS resolution happens before lock acquisition so the lock is held only during the file read-modify-write cycle. Wait briefly then fail if lock unavailable. diff --git a/specs/001-safety-reliability-refactor/tasks.md b/specs/001-safety-reliability-refactor/tasks.md new file mode 100644 index 0000000..0dde632 --- /dev/null +++ b/specs/001-safety-reliability-refactor/tasks.md @@ -0,0 +1,1784 @@ +# 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). + +- [ ] 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. + +- [ ] 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). + +- [ ] 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. + +- [ ] 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. + +- [ ] 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. + +- [ ] 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. + +- [ ] 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)** + +- [ ] 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. + +- [ ] 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 + +- [ ] 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). + +- [ ] 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). + +- [ ] 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. + +- [ ] 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. + +- [ ] 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)** + +- [ ] 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 + +- [ ] 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. + +- [ ] 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)** + +- [ ] 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 + +- [ ] 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. + +- [ ] 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. + +- [ ] 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`. + +- [ ] 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. + +- [ ] 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. + +- [ ] 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. + +- [ ] 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. + +- [ ] 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). + +- [ ] 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 + +- [ ] 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. + +- [ ] 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