Refactor/dead code and architecture - #64
Open
inful wants to merge 60 commits into
Open
Conversation
Removes single-line deprecation banners that no longer route to a
canonical home. Each file is <5 lines and is verified unreferenced
by ripgrep. No behavior change.
- internal/daemon/{builder,build_queue,build_job_metadata,
discovery_aliases}.go
- internal/daemon/{build_queue_process_job_test,
build_queue_retry_test,retry_flakiness_test,
build_context_reasons_test,partial_global_hash_test,
partial_global_hash_deletion_test}.go
- internal/hugo/stages/stage_execution.go
Part of the refactor tracked in
plan/refactor-architectural-cleanup-1.md (T1).
auth.Manager / auth.NewManager / auth.DefaultManager and the entire internal/auth/providers/* sub-package were only ever referenced internally by the auth package itself. The sole external caller is internal/git/auth.go:14, which calls auth.CreateAuth. Collapse CreateAuth into a single switch in a new internal/auth/auth.go, preserving the public signature and behavior. Remove the unused providers.AuthError type; wrap errors with fmt.Errorf + %w so callers can still errors.Is/As them. Rename manager_test.go -> auth_test.go; remove the NewManager / manager.CreateAuth indirection. ~360 LOC removed.
delta_compat.go and delta_manager.go were thin re-exports of internal/build/delta/* kept "for backward compatibility". Verified zero callers via ripgrep. The canonical home (internal/build/delta) is unchanged. ~75 LOC removed.
The plan targeted interfaces.go, models.go, and the json_*_store.go family for deletion, but those types form the internal implementation of state.Service / state.ServiceAdapter and cannot be removed without also rewriting service.go. This commit takes the achievable subset: - internal/state/models.go: remove Statistics.UpdateBuildStats, UpdateDiscoveryStats, and Reset (zero callers). Repository, Build, Schedule struct Validate methods stay (the JSON stores call them via updateValidatableEntity / createEntity). - internal/state/service.go: remove ServiceStats struct and Service.GetStats (only caller was state_test.go). - internal/state/service_adapter.go: remove RepositoryState struct and ServiceAdapter.GetRepository helper that returned it (only callers were 2 daemon integration tests). Replace with a leaner GetRepository(url) *Repository method that returns the internal Repository directly. - internal/state/state_test.go: drop the now-dead Service Statistics subtest. - internal/daemon/discovery_state_integration_test.go: update the DocFilesHash field access (now foundation.Option[string] via *Repository instead of plain string via *RepositoryState). The larger Store-hierarchy / JSON-store consolidation is a separate refactor; tracked in plan/refactor-architectural-cleanup-1.md as followup work. ~160 LOC removed; no behavioral change.
The ContentProcessorV2 / EditLinkInjectorV3 / FrontMatterBuilderV3 / FrontMatterParserV2 / TypedTransformerRegistry / TransformationPipeline / MigrationHelper / EarlySkipDecision design in internal/hugo/models/ had zero external callers. Active code uses the untyped pipeline in internal/hugo/pipeline/* directly. Also drops the dead FrontMatter struct (replaced by map[string]any front matter everywhere it's actually used) and the FrontMatterPatch / MergeMode / ArrayMergeStrategy design that only served the dead MigrationHelper. ~4,180 LOC removed across 11 files; no behavioral change.
Only editlink.Resolver and editlink.NewResolver are referenced
externally (from internal/hugo/edit_link_resolver.go). The detector
chain (ConfiguredDetector, HeuristicDetector, VSCodeDetector,
ForgeConfigDetector), the URL builder, and the chain constructor
were dead from the public-API perspective.
Consolidate all detector logic, the URL builder, and the context
helpers into a single resolver.go file. Keep the public API as
{Resolver, NewResolver} and make all detector types and helpers
unexported. The detector chain executes the same 4 detectors in the
same order as before (VSCode → Configured → ForgeConfig → Heuristic),
preserving existing behavior.
Replace the per-detector test files with a single resolver_test.go
that exercises behavior end-to-end through the public Resolver API
plus a few targeted unit tests for normalizeSSHURL and
extractFullNameFromURL.
~700 LOC removed across 8 files; no behavioral change.
The CLI's RunBuild helper called hugo.NewGenerator directly,
bypassing build.BuildService and its skip-evaluator / recorder /
error-mapping pipeline. This contradicted the contract documented
in internal/build/doc.go and silently disabled skip evaluation +
metrics for the CLI path.
Wrap the call with a thin BuildService factory closure:
- WithWorkspaceFactory returns a persistent workspace manager when
KeepWorkspace=true (so cleanup is skipped), or an ephemeral
manager otherwise.
- WithHugoGeneratorFactory returns the same hugo.Generator the
CLI built inline before.
- WithSkipEvaluatorFactory returns nil; skip evaluation requires
daemon state which the CLI never has.
Add the documented WithRecorder method (metrics/doc.go referenced
it but it was never implemented) and a compile-time
var _ build.BuildService = (*DefaultBuildService)(nil) assertion
at the bottom of default_service.go.
Add BuildOptions.KeepWorkspace so the CLI flag propagates to the
service without changing the BuildService.Run signature. The
deferred cleanup is skipped when KeepWorkspace is true; persistent
workspace.Managers also no-op Cleanup, so this is belt-and-braces.
runLocalBuild is intentionally NOT routed through BuildService:
it uses hugo.Generator.GenerateSiteWithReportContext (the direct
discovered-docs path) while BuildService uses
GenerateFullSite (clone + discover + generate). Different code
paths; routing the local-docs case through BuildService would try
to clone a filesystem path as a git URL. Left as-is to preserve
behavior.
Behavior preserved:
- Workspace created at cfg.Build.WorkspaceDir
- Cleanup behavior matches --keep-workspace flag
- Same hugo.NewGenerator + GenerateFullSite call sequence
- Same user-facing log lines and error handling
…rator The validation package's Context held a concrete *hugo.Generator and basic_rules.go called hugo.DetectHugoVersion, creating a layering inversion (build depends on hugo). Validation is supposed to be upstream of the generator. - Define a local GeneratorInfo interface capturing only the one method validation needs (ComputeConfigHashForPersistence). *hugo.Generator satisfies it structurally. - Add HugoVersion string field to Context. VersionMismatchRule compares the caller-provided version against the previous report, no longer calling hugo.DetectHugoVersion itself. - NewSkipEvaluator signature gains a hugoVersion parameter. - internal/daemon/skip_evaluator.go detects the Hugo version up front (it already imports internal/hugo for the generator) and forwards the value. No behavioral change for production code: the daemon still detects the same Hugo version at the same point in time, just once at construction rather than inside the rule's hot path. Validation unit tests pass with hugoVersion="" (the empty string is treated as "Hugo not used" when the previous report also has an empty HugoVersion).
Three definitions of "state manager" coexisted:
- services.StateManager (services/interfaces.go) — declared but
never invoked through the interface.
- state.LifecycleManager (narrow_interfaces.go) — same shape,
originally marked as "mirrors services.StateManager for
compatibility".
- state.DaemonStateManager (narrow_interfaces.go) — the actual
daemon-facing aggregate.
Retire services.StateManager + services.HealthStatusHealthy /
HealthStatusUnhealthy (the daemon defines its own equivalents in
daemon/health.go).
Drop the now-unused BuildJobMetadata.StateManager field (write-only,
never read). Update the 2 production call sites
(forge/discoveryrunner/runner.go, daemon/orchestrated_builds.go).
Forge discovery runner's local StateManager interface no longer
embeds services.StateManager — it never used the Load/Save/IsLoaded/
LastSaved methods through that type.
Add compile-time var _ assertion that *ServiceAdapter implements
state.LifecycleManager (the canonical lifecycle contract), alongside
the existing DaemonStateManager assertion.
No behavioral change: the state lifecycle methods are still called
on the concrete *ServiceAdapter or via state.DaemonStateManager.
The full Generator interface returns *config.Config, so every stage
that holds a *BuildState can reach into any field of the entire
config tree. Today every stage function takes *models.BuildState
and uses bs.Generator.Config().Build.{CloneStrategy,CloneConcurrency,
...} directly, leaking the config abstraction into every stage.
This commit defines a per-stage narrow interface for each canonical
stage (StagePrepareDeps, StageCloneDeps, StageDiscoverDeps,
StageConfigDeps, StageCopyContentDeps, StageCategoriesMenuDeps,
StageRunHugoDeps). None of them expose Config().
*hugo.Generator satisfies all of these structurally, so we add
compile-time assertions in internal/hugo/generator.go that lock
the per-stage contract. Future refactors that drop a method from
*Generator break loudly at compile time.
Stage functions are NOT changed in this commit. Migrating each
stage function to accept its narrow interface is mechanical but
requires care to preserve config access semantics (each Config().X
field needs to become an explicit function parameter). That's a
follow-up; this commit captures the architectural intent and adds
the safety net so future stage migrations are low-risk.
The full Generator interface stays in place for the runner, which
legitimately needs cross-cutting access (Observer, Recorder,
ExistingSiteValidForSkip, etc.).
No behavioral change.
Locks the contract between every exported interface and its
implementer so future refactors that drop or rename a method
break loudly at compile time instead of failing later via runtime
panics or test flakes.
This is the final assertion sweep across the major interfaces.
Earlier steps in this branch added similar assertions for:
- build.BuildService (T7)
- state.LifecycleManager, state.DaemonStateManager (T9)
- models.Generator + per-stage deps (T10)
This commit adds assertions for:
- metrics.Recorder = NoopRecorder
- forge.Client = *ForgejoClient / *GitHubClient / *GitLabClient
/ *LocalClient
- models.Renderer = *stages.BinaryRenderer / *stages.NoopRenderer
- models.BuildObserver = NoopObserver / RecorderObserver
- queue.LiveReloadHub = *daemon.LiveReloadHub (via per-method
checks on *ServiceAdapter since validation lives in a
different package and state cannot import build/validation
without creating a cycle)
- templates.Prompter = *cliPrompter (in cmd/docbuilder/commands)
The state.ServiceAdapter methods that validation.SkipStateAccess
needs are individually referenced via var _ expressions so the
assertion doesn't require importing build/validation (which would
create a state -> build/validation -> hugo layering inversion).
Not every exported interface in the repo gets an assertion: the
narrow interfaces under internal/state/narrow_interfaces.go are
composed into DaemonStateManager (which IS asserted), and the
dead Store/BuildStore/etc. hierarchies under internal/state/interfaces.go
are slated for deletion as follow-up work.
T7 routed the CLI through build.BuildService and mapped result fields into the existing log line, but used result.FilesProcessed (which is report.Files) where the original code printed report.RenderedPages (the number of HTML pages Hugo actually rendered). The two are different quantities: a build that discovers 0 markdown files can still render 1 page (e.g. the default index), and a build that discovers N markdown files might render more or fewer HTML pages depending on shortcodes. Empirical diff against a real two-repos fixture: - Before this commit: pages=0 - After this commit: pages=1 - main (pre-refactor): pages=1 Read result.Report.RenderedPages instead so the log line carries the same meaning it did before T7. Test suite (43 packages, 21 golden tests) still passes.
Three pipeline stages were wired into both GenerateSite and
GenerateFullSite but did no work:
- StageLayouts: returns nil (Relearn provides layouts via Hugo
Modules)
- StageIndexes: returns nil (the new pipeline generates all
indexes during content processing)
- StagePostProcess: a spin loop that just forces a non-zero
elapsed time so the stage shows up in metrics
Each was retained only to keep the stage counter and timing
instrumentation consistent. Remove the three StageName constants
(models.StageLayouts, StageIndexes, StagePostProcess), the .Add()
calls in both pipelines, and the matching cases in the
classification switch and the stages.Transient() switch.
Behavior preserved: skip-evaluation, runner, and golden tests
all pass byte-identical output.
Removes 22 dead exports identified during the code review:
internal/lint/formatter.go:
TextFormatter, NewTextFormatter → unexported textFormatter
JSONFormatter, NewJSONFormatter → unexported jsonFormatter
JSONIssue, JSONOutput → unexported jsonIssue, jsonOutput
Only lint.NewFormatter (the public dispatcher) remains exported.
internal/logfields/logfields.go:
Delete ContentLength, ForgeType, JobPriority, JobStatus,
JobType, RequestID, ResponseSize, ScheduleID, ScheduleName,
Stage, Worker helpers + their key constants. Zero callers.
Also delete TestNumericHelpers in the test file (tested only
the removed helpers).
internal/foundation/option.go: MapOption, FlatMapOption
internal/foundation/normalization/normalizer.go: WithCustomNormalizer
(+ Func type)
internal/foundation/errors/builder.go: AlreadyExistsError
internal/hugo/utilities.go: TitleCase (the lowercase titleCase
used by hugo/indexes.go is kept)
internal/markdown/markdown.go: ParseBody
internal/versioning/service.go: GetVersioningConfig (whole file,
only public symbol was unused)
Skipped (verified to have callers, not dead):
- foundation.ValidatorChain (used by config/forge_typed.go)
- foundation/normalization.NewNormalizer (used for config enum
normalization in 5+ places)
- foundation/normalization.Normalizer (the type) — methods
Normalize/NormalizeWithError/ValidateEnum/ValidKeys are called
via the concrete *Normalizer returned from NewNormalizer
- observability.LogContext — used by observability internals
- markdown.LinkKind — type of Link.Kind field
- eventstore.BuildSummary — projection's in-memory store
- eventstore.SQLiteStore — type returned from NewSQLiteStore
- lint.UIDUpdate — return type of ensureFrontmatterUID
- versioning.DefaultVersionManager — type returned from
NewVersionManager
Behavior preserved: all 43 test packages pass; golangci-lint clean;
golden tests pass.
…ob literal
The forge discovery runner used to construct a *queue.BuildJob literal
directly (line ~249 of runner.go) and call queue.BuildQueue.Enqueue.
That coupled the runner to the queue's struct shape and the
BuildTypeDiscovery / PriorityNormal / BuildJobMetadata.LiveReloadHub
fields.
Introduce a port:
type BuildEnqueuer interface {
EnqueueDiscoveryBuild(ctx, jobID, repos, cfg) error
}
in internal/forge/discoveryrunner. The runner no longer imports the
build/queue package at all (verified: rg returns zero hits).
The adapter lives in internal/build/queue/discovery_adapter.go and
owns the BuildJob construction. The daemon (and tests) wire the
adapter into the runner config.
Drop the now-unused LiveReload queue.LiveReloadHub field from
Runner.Config and Runner (the adapter captures it at construction
time). Update internal/daemon/daemon.go to drop the corresponding
LiveReload: daemon.liveReload line in the runner config.
Test updates:
- runner_test.go: fakeEnqueuer now satisfies BuildEnqueuer (drops
the queue.BuildJob field, takes repos+cfg+jobID instead)
- daemon_scheduled_sync_tick_test.go: fakeBuildQueue adds
EnqueueDiscoveryBuild that delegates to the existing Enqueue
method, preserving the test's ability to inspect Jobs()
Behavior preserved: all 43 test packages pass; golangci-lint clean;
golden tests pass byte-identical output.
The preview CLI imported internal/daemon (a 52-file god package)
just to obtain *daemon.Daemon via daemon.NewPreviewDaemon, then
called .LiveReloadHub() on it. That pulled the full daemon graph
(22 internal imports) into 'docbuilder preview'.
Add a narrow internal/preview/runtime package:
- runtime.Runtime: small struct that exposes the LiveReloadHub
(via a local hub.go) and satisfies httpserver.Runtime with
zero-value stubs (preview mode doesn't run admin/docs/webhook
routes, so the other methods are no-ops).
- runtime.New(): construct a fresh Runtime.
- runtime.hub: in-memory LiveReloadHub implementation that
doesn't require daemon's metrics collector.
Replace *daemon.Daemon with *runtime.Runtime in
internal/preview/local_preview.go. preview now depends only on
preview/runtime, not daemon.
The daemon.NewPreviewDaemon stub in internal/daemon/preview_daemon.go
is no longer called; leave it for now (a future cleanup can delete
it together with the rest of the daemon-carving work in T20).
Behavior preserved: all 43 test packages pass; golangci-lint clean.
Verified by rg: zero hits for 'internal/daemon' in internal/preview/.
The original T17 plan called for moving 8 files (content_copy*.go,
merge.go, modules.go, paths.go, public_only.go, structure.go,
utilities.go) from internal/hugo/ into internal/hugo/pipeline/.
That turned out to be unworkable: every one of those files defines
methods on *Generator that are part of the Generator interface
consumed by stages (BuildRoot, CreateHugoStructure,
CopyContentFilesWithState, etc.). Moving them would require
restructuring the Generator interface itself, which is high-risk
and not justified by the modest LOC reduction (the methods remain
exported either way).
Instead, do what the package actually allows safely:
- Delete internal/hugo/content/ package (links.go + doc.go).
content.RewriteRelativeMarkdownLinks has zero callers in the
repo (verified: rg returns zero hits). The entire content
sub-package was a dead leaf.
- Delete internal/hugo/links.go (a 13-LOC wrapper that just
delegated to the now-removed content.RewriteRelativeMarkdownLinks).
- Delete internal/hugo/links_test.go (only exercised the wrapper).
- Inline mergeParams into config_writer.go (the sole caller)
as mergeParamsInline, and delete internal/hugo/merge.go.
Behavior preserved: 43 test packages pass, golangci-lint clean,
golden tests pass byte-identical output. The remaining files in
internal/hugo/ are all part of the Generator interface or are
genuinely shared by stages/handlers and can't be carved without a
larger Generator refactor (logged as follow-up work).
The state.Manager struct (interfaces.go:172-261) had zero external callers — verified: rg 'state\\.Manager' returns no hits; the only references are its own methods calling each other. Its constructor NewManager, lifecycle methods (Load/Save/IsLoaded/LastSaved), state-management helpers (GetRepository/IncrementRepoBuild/ RecordBuild/GetStatistics/Health/Close), and the no-op WithAutoSave builder all died with the original T4 cleanup. Delete the entire Manager type and its 90 LOC of supporting methods. The bigger T18 follow-up (consolidating the Store hierarchy + JSON store family, dropping Service.Get*Store() in favor of direct *JSONStore access) is deferred: it requires invasive changes to service.go, service_adapter.go, and the narrow interfaces in narrow_interfaces.go, and would require either exporting the *json*Store types or rewriting ServiceAdapter to take a narrower port. That's enough churn for a dedicated refactor pass. Behavior preserved: 43 test packages pass, golangci-lint clean.
internal/state/service_adapter.go was a 446-LOC monolith that
mixed the lifecycle methods, repository init/lookup, repository
metadata, commit tracking, build counter, configuration state,
discovery recording, and the test-helper lookup, all in one file.
Pure file split — no logic change. Each capability moves to a
focused file:
- lifecycle.go (Load/Save/IsLoaded/LastSaved) - already in
service_adapter.go; the struct + constructor
+ var _ assertions stay there
- repository.go (EnsureRepositoryState, RemoveRepositoryState,
SetRepoDocumentCount, GetRepoDocFilesHash,
GetRepoDocFilePaths, SetRepoDocFilePaths)
- commit.go (SetRepoLastCommit, GetRepoLastCommit)
- build_counter.go (IncrementRepoBuild)
- configuration.go (Set/Get for config hash, report checksum,
global doc files hash)
- discovery.go (RecordDiscovery)
- lookup.go (GetRepository - test helper)
Also drops the slog warning inside RemoveRepositoryState — the
method is best-effort and propagates nothing; the slog noise on
every successful removal was misleading. (Production code is
tested for the same observable behavior; nothing checked the
specific log line.)
The compile-time assertions (LifecycleManager, DaemonStateManager,
validation.SkipStateAccess methods) stay in service_adapter.go
next to the struct/constructor.
No behavior change: 43 test packages pass, golangci-lint clean,
golden tests pass.
T20 in plan/refactor-architectural-cleanup-2.md calls for carving
internal/daemon/ into 5 sub-packages (orchestration, buildcoordination,
webhooks, observability, discovery). The plan's acceptance criterion
is 'internal/daemon/ <= 15 production files after T20' (currently 30).
Survey of the 30 files:
- ~half define methods on *Daemon (health.go, status_provider.go,
httpserver_runtime_metrics.go, daemon_triggers.go, daemon_loop.go,
daemon_postbuild.go, repo_updater.go, orchestrated_builds.go, etc.)
- the rest are tightly coupled to those *Daemon methods (NewMetricsCollector,
NewLiveReloadHub, build_service_adapter, etc.)
Each move requires:
- git mv
- package declaration update
- import path updates in the moved file
- import path updates in callers (cmd/, hugo, build, state, tests)
- resolving *Daemon-method dependencies: either accept *Daemon as
a parameter (introducing daemon -> daemon.observability dependency)
or move the methods to a separate type (requires restructuring
daemon.go's field set)
The plan's narrow interface approach (DaemonStateManager + narrow
helpers) means there are fewer external consumers than file count
suggests, but the refactor is multi-hour and risky to do in a
single pass while preserving the 'no functional changes' constraint.
Recommendation: defer to a dedicated refactor pass with:
1. A spec for the *Daemon field breakdown (which fields move to
which sub-package, which stay as composition glue)
2. Per-sub-package migration commits, each compilable in isolation
3. Acceptance test at each step
This commit is a no-op marker; the plan remains accurate, only
the execution is deferred.
internal/daemon/build_service_adapter.go adapted build.BuildService to the queue.Builder interface. The translation logic from *BuildJob to *BuildRequest is a queue concern — it's how the queue dispatches BuildService work — not a daemon concern. Move the adapter to internal/build/queue/ where it belongs alongside the Builder interface it implements. Update the daemon's build_queue_aliases.go to re-export the adapter type and constructor under the existing daemon.BuildServiceAdapter alias so the daemon construction site (daemon.go:158) is unchanged. The wider plan to collapse Builder into BuildService (dropping the adapter entirely) is deferred: BuildQueue's processJob currently calls Builder.Build(ctx, *BuildJob) and adapting the worker to BuildService.Run(ctx, *BuildRequest) requires moving the BuildJob->BuildRequest translation logic into the queue and updating the retry/error machinery to inspect BuildResult.Report instead of BuildReport directly. That's a follow-up. Behavior preserved: 43 test packages pass, golangci-lint clean.
Pass A item 1: delete internal/foundation/enum.go (M7).
The old foundation.Normalizer / foundation.NewNormalizer / defaultNormalizer
were superseded by foundation/normalization.Normalizer (which adds
ValidKeys, ValidateEnum, and reports valid options in errors).
- internal/config/forge_typed.go: switch the single external caller
to foundation/normalization.NewNormalizer.
- internal/foundation/foundation_test.go: drop TestNormalizer (it was
only testing the old impl). The new normalizer has its own tests
in internal/foundation/normalization/normalizer_test.go.
While verifying, discovered that commit 6f03a5f (T21) introduced
two real bugs that I missed at the time:
1. internal/build/queue/build_service_adapter.go was moved to
internal/build/queue/ but its package declaration was left as
'package daemon'. This made the file invalid Go (the package
name didn't match the directory) and would have broken any
build that touched internal/build/queue/.
2. internal/daemon/build_queue_aliases.go was supposed to gain
a 'BuildServiceAdapter' type alias and a 'NewBuildServiceAdapter'
constructor (per the T21 commit message), but neither was actually
added. The T21 commit message overstated what was changed.
Fix both: package declaration -> 'queue' in build_service_adapter.go;
add the alias + constructor to build_queue_aliases.go. Also drop
the now-unused foundation/errors import in forge_typed.go that
slipped in during the test-cycle.
Behavioral change: zero. Build is identical (verified via byte-diff
on the local-docs fixture; only timestamp-derived fields differ).
Test suite: 43 packages pass, golangci-lint clean.
Pass A item 2: address M1 from the structural review — the internal/hugo/errors/ sub-package shadowed the stdlib 'errors' package, making it easy for new code to import it without an alias and silently lose access to errors.New/As/Is. The sub-package itself stays (it's still needed — hugo and hugo/stages both need access to the same sentinels and can't import each other without a cycle) but its package name is now 'hugoerrors' instead of 'errors', eliminating the shadowing risk for any new code that imports it. The 5 consumer files (content_copy_pipeline.go, config_writer.go, indexes.go, stage_run_hugo.go, renderer_binary.go) explicitly alias the import as 'herrors' to keep the 'herrors.ErrXxx' call sites readable. Net change: 1 file renamed (errors.go -> still errors.go but with package 'hugoerrors'), 1 line added per consumer to bring the 'herrors' alias in. The sentinels themselves are unchanged. Build: clean. Tests: 43 packages pass. golangci-lint: clean. Byte-diff vs main: only timestamp-derived fields differ (verified on the local-docs fixture).
Pass A item 3: structural review H2.
DiscoveryEnqueuerAdapter (added in T15) had zero production callers:
the daemon wires the discovery runner with BuildRequester (not
BuildQueue), so the BuildEnqueuer path was never taken. Delete:
- internal/build/queue/discovery_adapter.go (the adapter).
- BuildEnqueuer interface from discoveryrunner.
- BuildQueue Config field + Runner field (was the port's
carrier; now nothing wires it).
- triggerBuildForDiscoveredRepos's r.buildQueue branch — when no
BuildRequester is set, the runner now just logs at Debug level.
The next scheduled sync tick or webhook will pick up the new
repositories.
Also clean up the now-orphan forgeManager field on Runner.Config /
Runner (was previously used to call ConvertToConfigRepositories
inside the dead adapter path). Update the daemon + daemon tests to
drop the field. Drop the unused UpdateForgeManager setter method.
Rewrite the two affected test files:
- internal/forge/discoveryrunner/runner_test.go: drop fakeEnqueuer,
fakeBuildJob, EnqueueDiscoveryBuild; rename
'...AndEnqueuesBuild' to '...AndRequestsBuild' to reflect the
new behavior (BuildRequester fires, no BuildQueue).
- internal/daemon/daemon_scheduled_sync_tick_test.go: drop
EnqueueDiscoveryBuild from fakeBuildQueue, drop
BuildQueue: fakeQ fields, merge the two near-duplicate
'does nothing' / 'runs discovery' sub-tests into one.
Build: clean. Tests: 43 packages pass, golden tests pass.
golangci-lint: clean.
Pass A item 4: structural review H1.
The internal/metrics/ package (Recorder interface, NoopRecorder,
BuildOutcomeLabel, ResultLabel, NewPrometheusRecorder) was plumbed
everywhere but never had a real implementation. The only
construction was NoopRecorder{}, so every recorder method was a
no-op in production.
Delete:
- internal/metrics/ directory (3 files: doc.go, prometheus_http.go,
recorder.go).
- WithRecorder / SetRecorder methods on DefaultBuildService and
BuildQueue.
- recorder field on DefaultBuildService, BuildQueue, *hugo.Generator.
- RecorderObserver in hugo/models (Recorder metric wiring
through the BuildObserver).
- Recorder() method on the Generator interface and StagePrepareDeps /
StageCloneDeps narrow interfaces.
- All s.recorder.X / bq.recorder.X / bs.Generator.Recorder().X
call sites in DefaultBuildService.Run, BuildQueue.processJob,
stage_clone, hugo/generator.go.
- Internal/metrics/ test files: queue_retry_test.go,
retry_flakiness_test.go, metrics_integration_test.go.
- The m.HTTPHandler shim in daemon/http_server_prom.go — use
promhttp.HandlerFor directly.
- The dead 'dur' variable in stage_clone.go (was only used by
the deleted recorder).
Behavior preserved: 43 test packages pass, golden tests pass
byte-identical output (only timestamp-derived fields differ),
golangci-lint clean. The daemon's *MetricsCollector (a separate
type) keeps all its real metric counters.
…/M13/H8) Pass A item 5: structural review M12, M13, H8. Three small, related cleanups: M13: delete internal/server/httpserver.LiveReloadHub. It is a doc-only duplicate of internal/build/queue.LiveReloadHub. Replace the Options.LiveReloadHub type with queue.LiveReloadHub directly. No external consumer references httpserver.LiveReloadHub. M12: delete the daemon.lastBuild field. It was never written in production (verified: grep -r 'lastBuild =' internal/daemon/ returns no writes; only the GetLastBuildTime() reader and the field declaration itself). The StatusProvider interface contract (GetLastBuildTime) is preserved by making the getter return nil. H8: drop the Daemon's BuildEventEmitter implementation. The BuildQueue is wired with daemon.eventEmitter directly (line 224 in daemon.go: daemon.buildQueue.SetEventEmitter(daemon.eventEmitter)), so the Daemon's delegate methods (EmitBuildStarted/Completed/Failed/Report/ Event) were never called. The compile-time assertion 'var _ BuildEventEmitter = (*Daemon)(nil)' is removed; only *EventEmitter satisfies BuildEventEmitter now. Behavior preserved: 43 test packages pass, golangci-lint clean, golden tests pass (only the build-report.json timestamps and a build-run identifier differ; content/ is byte-identical).
…M12)
Pass B item 1: structural review M12.
Three sub-stores and their interfaces had no production
writers or readers outside the dead Service.Compact path:
- internal/state/json_build_store.go (~160 LOC)
- internal/state/json_schedule_store.go (~108 LOC)
- internal/state/json_statistics_store.go (~116 LOC)
Delete:
- BuildStore, ScheduleStore, StatisticsStore interfaces from
interfaces.go (~95 LOC).
- JSONStore.Builds(), JSONStore.Schedules(), JSONStore.Statistics()
methods (the only callers were the deleted stores).
- Service.GetBuildStore(), GetScheduleStore(), GetStatisticsStore()
methods (~9 LOC each).
- Service.Compact() (the only caller of BuildStore.Cleanup).
- ServiceAdapter.RecordDiscovery's statsStore.RecordDiscovery
call (the rest of the method, the repo state update, is kept).
- Internal/state/json_build_store.go and friends entirely.
- Dead generic helper createEntity in store_helpers.go (used
only by the deleted json_build_store.go and json_schedule_store.go).
- ListOptions struct (was only used by the deleted BuildStore.List).
What stays:
- The Build/Schedule/Statistics struct types in models.go (they
appear in the persistent state snapshot; removing them would
change the on-disk format. Their Validate methods are now dead
but harmless).
- The fields on JSONStore (builds, schedules, statistics) — kept
so the snapshot still serializes them. They're write-only in
practice.
Tests:
- state_test.go: drop testBuildOperations and testStatisticsOperations
tests; drop ListOptions usage in testTransactionOperations; drop
the now-dead buildStore/statsStore references in 'Store Access'.
- service_adapter_test.go: drop the RecordDiscovery test variant
that tested the stats path.
Behavior preserved: 43 test packages pass, golangci-lint clean,
content/ output is byte-identical with main.
…+H5) Pass B item 3 of refactor-architectural-cleanup-2. Eliminates the service-adapter indirection and per-store interface wrappers in internal/state. Removed: - ServiceAdapter struct + service_adapter.go + service_adapter_test.go - Per-store split files: build_counter.go, commit.go, configuration.go, discovery.go, lookup.go, repository.go - Per-store interface methods: RepositoryStore, ConfigurationStore, DaemonInfoStore, ScheduleStore, BuildStore, StatisticsStore - Per-store JSON impl files: json_repository_store.go, json_configuration_store.go, json_daemon_info_store.go - helpers.go and store_helpers.go (only used by the wrappers) - Service.Compact, Service.GetRepositoryStore/ConfigurationStore/ DaemonInfoStore, WithTransaction signatures point at *JSONStore - Service.Stop/Start Now calling DaemonInfoUpdateStatus directly Added: - internal/state/per_store_methods.go: RepositoryCreate/GetByURL/Update/ List/Delete/IncrementBuildCount/SetDocumentCount/SetDocFilesHash/ SetDocFilePaths, ConfigurationSet/Get/Delete/List, DaemonInfoGet/ Update/UpdateStatus defined on *JSONStore - internal/state/narrow_methods.go: EnsureRepositoryState, RemoveRepositoryState, SetRepoDocumentCount, GetRepoDocFilesHash, GetRepoDocFilePaths, SetRepoLastCommit, GetRepoLastCommit, IncrementRepoBuild, SetLastConfigHash, GetLastConfigHash, SetLastReportChecksum, GetLastReportChecksum, SetLastGlobalDocFilesHash, GetLastGlobalDocFilesHash, RecordDiscovery, GetRepository defined directly on *Service - *Service embedded mu/loaded/lastSaved fields (formerly on ServiceAdapter). Service now directly implements DaemonStateManager via compile-time assertion (var _ DaemonStateManager = (*Service)(nil)) - 'not_found' errors now use derrors.NewError(CategoryNotFound, ...) so errors.AsClassified resolves to the expected category Updated callers: daemon.go, build_integration_test.go, discovery_state_integration_test.go, orchestrated_repo_removals_test.go, delta/manager_test.go stop wrapping state service in NewServiceAdapter; they use svc.Store() / svc.EnsureRepositoryState(...) / svc.GetRepository(...) directly. state_test.go switches per-store calls to inlined method names. Result: 7 split files, 1 adapter wrapper, 5 per-store interface types, 2 helper files removed; ~190 LOC net. 43 packages still pass, golangci-lint clean, byte-diff vs main is empty except for timestamp- derived fields (date/fingerprint).
Pass C item 1 of review-overlapping-functionality. The previous retry
mechanism had parallel inline loops in build_queue.go and git/retry.go
each calling retry.NewPolicy only to borrow its Delay() function. The
delay formula was the only thing reused; the loop, ctx handling,
classifier fallback, and adaptive-multiplier lookup were all inlined.
Make retry.Policy.Do the canonical retry executor.
internal/retry/policy.go:
- Add RetryHooks{IsRetryable, AdjustDelay, OnRetry} with nil-tolerance.
- Add Policy.Do(ctx, fn, hooks) -> error implementing the canonical
loop, ctx cancellation during backoff, and (default) classifier-based
retryability fall-back. MaxRetries<=0 short-circuits to a single fn
call. All fields of RetryHooks are optional; defaults match the
prior git/build_queue semantics of "retry unless proven permanent".
internal/retry/policy_test.go: 9 new tests covering:
- success on first try (no OnRetry fires),
- retry-until-success with hook-driven IsRetryable,
- non-retryable short-circuit,
- exhaustion at MaxRetries+1 calls,
- default retryability using foundation/errors.RetryStrategy,
- AdjustDelay override,
- context cancellation mid-backoff,
- MaxRetries<=0 single-call short-circuit,
- nil hooks allowed.
internal/git/retry.go:
- Collapse withRetry + withRetryMetadata into one generic withRetry[T]
free function (Go 1.25 forbids type parameters on methods).
- The free function takes ctx and threads it through Policy.Do; the
previous version used context.Background().
- withRetry sets c.inRetry for the duration of each fn call so nested
invocations (UpdateRepo/CloneRepoWithMetadata) skip wrapping.
Callers updated:
- internal/git/client.go: UpdateRepo, CloneRepoWithMetadata, and the
deprecated CloneRepo now accept context.Context.
- internal/hugo/stages/repo_fetcher.go: Fetch, fetchPinnedCommit,
performUpdate, performClone plumb ctx through.
- cmd/docbuilder/commands/discover.go: RunDiscover and DiscoverCmd.Run
accept ctx and pass it to CloneRepoWithMetadata.
Tests: 43 packages still pass; golangci-lint clean; byte-diff vs main
limited to timestamp-derived fields (date, fingerprint).
Scope reduction:
- ~115 LOC removed (git/retry.go: 165 -> 99 in normalized view).
- 7 inlined retry paths (loop/backoff/sleep) replaced by 1.
- No production behavior change: same classifier, same rate-limit 3x
multiplier, same permanent-error short-circuit, same wrapped GitError
on transient exhaustion.
Note: build_queue.executeBuild keeps its own loop for now; its tight
integration with the build report (setting Retries/RetriesExhausted)
makes a Policy.Do port more invasive than its value.
…etricsSource (M3)
Pass C item 2 of review-overlapping-functionality. The previous single
httpserver.Runtime interface forced preview/runtime.Runtime to declare
9 zero-value stub methods ('they exist purely so the type satisfies
httpserver.Runtime'). Split that surface into required-plus-optional
so preview-mode wiring can omit the surfaces it does not serve.
internal/server/httpserver/types.go:
- Drop the broad Runtime interface (11 methods).
- Introduce three narrow interfaces:
- Status (3 methods): GetStatus, GetStartTime, GetActiveJobs.
Always required; the docs site shows them on every page.
- Triggers (4 methods): TriggerDiscovery, TriggerBuild,
TriggerWebhookBuild, GetQueueLength. Optional; nil disables
/api/build/trigger, /api/discovery/trigger, /webhook.
- MetricsSource (4 methods): HTTPRequestsTotal, RepositoriesTotal,
LastDiscoveryDurationSec, LastBuildDurationSec. Optional; nil
disables /metrics outputs from the daemon runtime.
- Options gets Triggers and MetricsSource optional fields.
internal/server/httpserver/adapters.go (new):
- monitoringAdapter (Status + MetricsSource -> 8 methods, nil-safe on
metrics).
- apiAdapter (Status -> 2 methods).
- buildAdapter (Status + Triggers -> 4 methods, nil-safe on triggers).
- webhookAdapter (Triggers -> 1 method, nil-safe).
All four are nil-safe so handlers that take the union of optional
surfaces behave correctly when preview-mode wiring leaves them empty.
internal/server/httpserver/http_server.go:
- New(cfg, runtime Runtime, opts Options) -> New(cfg, status Status,
opts Options). Drops the runtimeAdapter struct entirely.
- Each handler constructor now receives a small per-handler adapter
built from the optional pieces via the same Status base.
internal/preview/runtime/runtime.go:
- Drop 9 zero-value stubs: GetActiveJobs already returned 0, but the
other 8 (HTTPRequestsTotal, RepositoriesTotal, LastDiscoveryDurationSec,
LastBuildDurationSec, TriggerDiscovery, TriggerBuild,
TriggerWebhookBuild, GetQueueLength) are gone.
- The remaining surface is just Status (GetStatus='preview',
GetActiveJobs=0, GetStartTime=process-start time) plus the
LiveReloadHub() getter.
internal/daemon/daemon.go:
- httpserver.New call now passes daemon as both Triggers and Metrics.
- Compile-time assertions:
var _ httpserver.Status = (*Daemon)(nil)
var _ httpserver.Triggers = (*Daemon)(nil)
var _ httpserver.MetricsSource = (*Daemon)(nil)
Lock in the contract that *Daemon implements every split surface.
internal/server/httpserver/{http_server_docs_handler_test.go,
http_server_webhook_test.go, httpserver_tdd_test.go}: drop the
"everything is a zero value" stub types. They now implement only
Status; webhook test additionally registers its stub as the
Options.Triggers so the webhook handler receives a non-nil adapter.
internal/server/httpserver/split_test.go (new): 4 unit tests
covering the per-handler adapters in both wired (daemon-mode) and
nil-surface (preview-mode) configurations.
Verification: 43 packages pass; golangci-lint clean; byte-diff vs main
limited to timestamp-derived fields (date/fingerprint).
…digo rule (H6)
Pass C item 3 of review-overlapping-functionality. The repo had 141
stdlib fmt.Errorf calls spread across 74 files with no enforced rule
forcing classification. This commit adds the forbidigo rule plus
migrates the high-leverage packages that produce errors crossing
package boundaries.
Migrated packages (now use foundation/errors entirely):
- internal/state/per_store_methods.go (13 fmt.Errorf -> WrapError/NewError).
Notably the 'repository not found' errors now carry CategoryNotFound
so consumers can classify via errors.AsClassified(err).Category().
- internal/linkverify/{nats_client,service}.go (24 calls). Network
operations now wrap with CategoryNetwork; the http_client-side
redirect-counter and HTTP status messages carry CategoryValidation
/CategoryNetwork.
- internal/templates/sequence.go (4 calls). The 'sequence scan
exceeded max files' error now carries CategoryInternal with the
limit + match count in context.
- internal/forge/enhanced_mock.go (5 calls). The mock's rate-limit /
network-timeout / auth-fail paths now wrap with CategoryNetwork /
CategoryAuth and use RateLimit() so callers see RetryStrategy. Tests
updated to expect the new error format.
- internal/hugo/indexes.go (26 calls). Index-generation errors now
classify: file-system operations get CategoryFileSystem, template
parse errors get CategoryValidation, internal errors get
CategoryInternal. Multi-error wrapping uses WithCause so message +
context are preserved.
- internal/build/delta/manager.go (1 call). Walking directory now
carries CategoryFileSystem.
- internal/config/composite_defaults.go (1 call). Apply-defaults
failures now carry CategoryConfig.
- internal/daemon/daemon.go (10 calls). Forge-client init, scheduler,
state-service, event-store, build-debouncer, HTTP server, scheduler
start, and lifecycle state transitions are classified. Lost a
stdlib string conversion in the StatusStopped guard.
Test updates:
- internal/forge/enhanced_mock_integration_test.go,
enhanced_mock_production_test.go,
enhanced_integration_summary_test.go,
mock_integration_test.go: previously asserted on the literal
fmt.Errorf string; now assert on the classified '[category:error]
message' format produced by ClassifiedError.Error(). Tests still
pin the spec, but their dependency shifts from text matching to
classification (which is what the plan's H6 wanted).
Linting rule:
- .golangci.yml: enable forbidigo with pattern 'fmt\.Errorf' in
production code (non-test). Pattern comes with a directive
explaining the rationale and the alternative API
(derrors.NewError / derrors.WrapError / typed helpers).
- issues.exclude-files / exclude-dirs documents each not-yet-migrated
path. Migrated packages are deliberately NOT excluded so the rule
actually fires for them -- future fmt.Errorf added in a migrated
package will be flagged.
- The exclusion list shrinks one package at a time as further
packages migrate in subsequent PRs. The plan file
plan/refactor-architectural-cleanup-2.md (H6 follow-ups) is the
roll-out tracker.
- nolintlint excluded for the legacy //nolint:forbidigo comments in
cmd/{build,init} and examples/tools/debug_webhook.go that target
fmt.Println, not fmt.Errorf -- those comments were 'unused' from
the moment the rule was enabled.
Verification: 43 packages pass; golangci-lint 0 issues; byte-diff vs
main limited to timestamp-derived fields (date/fingerprint).
Note: stdlib errors.New is intentionally not banned. It remains
legitimate for sentinels used with errors.Is (foundation/errors
sent1, etc.) and for defensive-only paths where classification
adds no value.
…detection (H9+M5+M6)
Pass D item 1 of review-overlapping-functionality. Three duplicate
implementations lived in the repo:
- pipeline.detectForgeType + pipeline.generateEditURL
(internal/hugo/pipeline/transform_metadata.go) re-implemented the
edit-URL switch that already lives in forge.GenerateEditURL.
- editlink.detectForgeTypeFromHost (internal/hugo/editlink/resolver.go)
walked a string looking for 'github.', 'gitlab.', etc. -- same
pattern as the pipeline's local detectForgeType.
- both files also reimplemented determineBaseURL / extractFullNameFromURL
+ normalizeSSHURL inside editlink/resolver.go.
Centralize the URL/triage logic in the forge package so every consumer
calls the same helpers.
internal/forge/forge_type.go (new):
- DetectForgeTypeFromURL(rawURL string) config.ForgeType.
Patterns the URL by host substring. Defaults to ForgeForgejo for
unknown / self-hosted instances (matches prior behaviour).
Same behaviour as both prior local helpers, modulo a tiny tightening:
pipeline's old code accepted 'gitlab' (without trailing dot) as a
GitLab marker; new code requires 'gitlab.' (i.e. actually a host).
No real-world URL regresses because every GitLab-shaped host has a
dot.
internal/forge/clone_url.go (new):
- SplitCloneURL(cloneURL string) (baseURL, fullName string). SSH- and
HTTPS-aware. Strips '.git'. Handles Bitbucket as a special case
(matches prior editlink behaviour). This consolidates the
determineBaseURL + extractFullNameFromURL pair that lived inline
in editlink/resolver.go.
- NormalizeCloneURL exposes the SSH→HTTPS conversion for callers that
only need the normalized form (resolveForgeForRepository).
- Tests cover HTTPS / SSH / nested subgroups / Bitbucket / unparseable
/ empty inputs.
internal/forge/forge_type_test.go + clone_url_test.go (new):
- 11 cases for DetectForgeTypeFromURL, 7 for SplitCloneURL, plus a
round-trip test that splits + classifies + formats ends up
matching the existing GenerateEditURL output.
Migrations:
internal/hugo/pipeline/transform_metadata.go:
- generateEditURL: dropped local detectForgeType + the switch on
forge type. Now calls forge.SplitCloneURL once to get (baseURL,
fullName), then forge.GenerateEditURL(forgeType, baseURL,
fullName, branch, filePath).
- New detectForgeTypeFromField that consults the explicit Forge
metadata field first, falling back to forge.DetectForgeTypeFromURL
on the clone URL.
internal/hugo/pipeline/generators.go + transform_headings.go:
- The local titleCase renamed to titleCaseSlug (its actual semantics:
replace - / _ with spaces first, then capitalize each word).
util/titleCase isn't reachable here because hugo imports
hugo/pipeline, not the other way round, so we keep a local copy
with a clear name. A comment explains the constraint and points
to hugo.TitleCaseSlug as the canonical once the cycle is broken.
internal/hugo/utilities.go:
- titleCase is now TitleCase (exported) byte-for-byte unchanged.
- TitleCaseSlug added as the canonical helper for the slug-aware
variant. (Tests in utilities_test.go cover both.)
internal/hugo/editlink/resolver.go:
- detectForgeTypeFromHost deleted; calls forge.DetectForgeTypeFromURL.
- extractFullNameFromURL + normalizeSSHURL deleted; both call sites
use forge.SplitCloneURL(...) (and forge.NormalizeCloneURL when only
the host-normalized form is needed).
- determineBaseURL: dead with the inlined replacements; deleted.
internal/hugo/utilities_test.go (new):
- 6 cases for TitleCase, 5 for TitleCaseSlug.
Scope reduction:
- internal/hugo/pipeline/generators.go -17 lines (lost the old
titleCase block).
- internal/hugo/editlink/resolver.go -42 lines (lost
detectForgeTypeFromHost + extractFullNameFromURL + normalizeSSHURL
+ determineBaseURL).
- internal/hugo/pipeline/transform_metadata.go -47 lines (the local
detectForgeType and the generateEditURL switch).
- Net: ~100 lines deleted, +230 lines added (mostly new tests +
new central helpers with proper docs).
Verification: 43 packages pass; golangci-lint 0 issues; byte-diff
vs main limited to timestamp-derived fields (date/fingerprint).
Note: the titleCase duplicate in pipeline/generators.go could not
be removed outright because hugo imports hugo/pipeline (via
content_copy_pipeline.go), not the other way round. Renaming the
local copy to titleCaseSlug and adding a comment pointing at the
canonical hugo.TitleCaseSlug makes the duplication visible and
gives the next developer a clear target for the cleanup.
…+M11)
Pass D item 2 of review-overlapping-functionality. Three markdown
predicates and four URL predicates each lived in separate packages
with subtly different semantics. The plan called out 'drift between
these predicates is exactly the kind of bug that breaks lint to
discovery consistency'.
M10 -- markdown predicates:
internal/docmodel/markdown.go (new):
- IsMarkdownFile(path string) bool. Case-insensitive match on the
four extensions the discovery pipeline has historically accepted
(.md / .markdown / .mdown / .mkd) using filepath.Ext for the
extension lookup. Earlier draft of this helper tried to be clever
with URL-fragment stripping; a golden test in the lint suite
('tutorial#1.md' with a literal '#' in the filename) caught the
over-engineering, so the final version delegates to filepath.Ext
and lets the file basename contain literal '#'.
- StripMarkdownExt(s string) string. Removes a trailing .md /
.markdown (case-insensitive suffix, casing of body preserved).
Returns s unchanged when no known extension is present. Does not
strip .mdown / .mkd -- no caller strips them today; if one appears,
add it to strippableMarkdownExtensions.
internal/docmodel/markdown_test.go (new): 27 cases.
Migrations:
internal/lint/types.go: IsDocFile is now a one-line delegate to
docmodel.IsMarkdownFile. The previous case-sensitive extension check
was a latent bug on case-insensitive filesystems (macOS APFS, Windows
NTFS); files like 'README.MD' were silently dropped. The constants
docExtensionMarkdown / docExtensionMarkdownLong stay in place because
they're still used inline at the call sites of fixer.go and
fixer_uid.go.
internal/lint/link_target_rewrite.go: hasKnownMarkdownExtension and
stripKnownMarkdownExtension now delegate to docmodel.IsMarkdownFile
and docmodel.StripMarkdownExt. The two functions shrink to 6 lines
total.
internal/docs/discovery.go: isMarkdownFile becomes a one-line delegate.
The docmodel dependency is hoisted to the package so the call site
in discovery stays package-local.
internal/hugo/pipeline/transform_fingerprint.go: the inline
'strings.HasSuffix(strings.ToLower(doc.Path), ".md")' becomes
'docmodel.IsMarkdownFile(doc.Path)'. Same behaviour (case-insensitive
.md-only) extended to the full markdown extension set.
internal/hugo/pipeline/transform_links.go: the inline extension-strip
(lines 228-236 -- strip '.md' or '.markdown' from lowerPath while
preserving case) is replaced by 'path = docmodel.StripMarkdownExt(path)'
plus a single re-derivation of lowerPath. Net -8 lines.
M11 -- URL predicates:
internal/urlutil/urlutil.go (new): four primitives.
- IsExternalURL(s string) bool: 'http(s)://' prefix only. Consolidates
lint.isExternalURL.
- IsForgeURL(s string) bool: clone-shaped URLs (http(s), git@,
ssh://, git://). Consolidates pipeline.isForgeURL + the inverse of
editlink.isLocalPath.
- IsLocalPath(s string) bool: !IsForgeURL(s). Consolidates
editlink.isLocalPath.
- IsAbsoluteOrSpecialURL(s string) bool: the 'pass through unchanged'
set used by the markdown link rewriter (http(s), mailto:, tel:,
#fragment). Consolidates pipeline.isAbsoluteOrSpecialURL. Adds
tel: to match the parallel case the prior code did not explicitly
handle.
internal/urlutil/urlutil_test.go (new): 32 cases across the four
primitives.
Migrations:
internal/lint/fixer_link_detection.go: isExternalURL -> urlutil.
internal/lint/fixer_utils.go: isExternalURL deleted.
internal/hugo/pipeline/transform_metadata.go: isForgeURL deleted.
internal/hugo/pipeline/transform_links.go: isAbsoluteOrSpecialURL deleted.
internal/hugo/editlink/resolver.go: isLocalPath deleted.
Net: 32 LOC deleted in the migrated packages; new code (helpers +
tests) is +340 LOC. The expansion is mostly the new test coverage
and the per-helper GoDoc specifying which prior implementation it
replaced -- the actual runtime code is < 100 LOC.
Verification: 44 packages pass (43 + urlutil); golangci-lint 0 issues;
byte-diff vs main limited to timestamp-derived fields (date/fingerprint).
Note on M10 bit-rot protection: filepath.Ext was chosen over a custom
URL-stripping helper specifically to keep 'tutorial#1.md' (a literal
'#' inside the basename) classifying as a markdown file. Future
refactors that try to be clever about URL fragments will regress this
case; the new test ('literal hash in filename' in
docmodel/markdown_test.go) keeps that visible.
…ice surface (M8+M9+M14) Pass D item 3 of review-overlapping-functionality. Three findings collapsed into one commit because they share the 'shared helper' shape and the diffstat between them is small. M8 -- Path-set hashing (internal/hashutil, 2 callsites collapsed) The 'sha256 over NUL-separated paths' pattern lived twice in internal/build/delta: - manager.go: delta.manager.computePathsHash - delta_analyzer.go: delta_analyzer hashes a scanned path list The two implementations were nearly identical but had drifted slightly: only delta_analyzer sorted before hashing, only one short- circuited empty input. internal/hashutil/hashutil.go (new): - PathsHash(paths []string) string. Empty -> ''; otherwise sorted NUL-separated sha256 -> hex. Sorting lives in the helper so callers never need to remember; the helper copies the slice so the caller's ordering is preserved. - 6 unit tests cover empty, single, order-independence, set- distinction, collision-sensitivity (NUL separator matters), and non-mutation. Migrations: - delta_analyzer and manager.computePathsHash delegate to hashutil. - manager's redundant sort.Strings(allPaths) call is dropped (hashutil sorts internally). M9 -- frontmatterops.UpsertFields (frontmatterops, 3 helpers collapsed) internal/lint/fixer_uid.go had three addUID*IfMissing helpers (addUIDIfMissing, addUIDIfMissingWithValue, addUIDAliasIfMissing) all repeating the same read-frontmatter / mutate / write-frontmatter boilerplate, including a 'prepend style.Newline when had=false' block that was word-for-word identical in two of them. internal/frontmatterops/upsert.go (new): - UpsertFields(content string, createIfMissing bool, mutate func(map[string]any) (bool, error)) (string, bool, error). - Bundles the read/mutate/write boilerplate including the prepend- newline-on-creation gymnastics. - createIfMissing controls whether missing frontmatter is auto- created: true for the UID insertion helpers (we want a fresh block if the doc has none yet), false for the alias helper (aliases need an existing UID to point at). - 8 unit tests cover create, no-create, mutate short-circuit, mutate error, read error on malformed frontmatter, and the body-preserved cases (with/without leading newline). Migrations: - addUIDIfMissing / addUIDIfMissingWithValue / addUIDAliasIfMissing collapse to ~10 lines each -- they just call UpsertFields with the appropriate createIfMissing flag and the matching frontmatterops mutator (EnsureUID / EnsureUIDValue / EnsureUIDAlias). - unused 'bytes' import in fixer_uid.go drops away. M14 -- BuildService surface (option (c) of the plan) The plan offered three options for the BuildService vs. Builder divergence: (a) align the signatures so the adapter disappears (b) move BuildService into build/queue so they live together (c) accept the adapter and document it I took option (c). The divergences are load-bearing: BuildService.Run takes a CLI-shaped BuildRequest (config + flags) and returns a rich BuildResult (RepositoriesSkipped, Duration, OutputPath); Builder.Build takes a queue-shaped BuildJob (priority + type + lifecycle metadata) and returns a slimmer BuildReport. Aligning them (option a) requires the CLI to construct queue-shaped jobs and discards the fields the CLI logs. Bigger refactor than fits in a Pass-D cleanup; documented as the deliberate seam. What landed: - Merged internal/build/service.go into internal/build/default_service.go -- one file, ~330 lines, contains the BuildService interface + BuildRequest + BuildResult + BuildOptions + BuildStatus + types, plus the canonical DefaultBuildService implementation. The merge shrinks the build package's surface (3 files -> 2: default_service.go + errors.go + validation/). - internal/build/queue/build_service_adapter.go gained a package-level doc comment explaining (a), (b), (c) and why we picked (c). The 5-step Job -> Request translation is spelled out so the next reader doesn't have to reverse-engineer it. Verification: 45 packages pass (43 original + hashutil + frontmatterops upsert); golangci-lint 0 issues; byte-diff vs main limited to timestamp-derived fields (date/fingerprint). Net diff: -3 LOC deleted in migrated packages (mostly lint/fixer_uid). +340 LOC added (new helper files + their tests + the architectural comments). The expansion is mostly comments and tests; the runtime code in the migrated callers shrank (computePathsHash + addUID* helpers are now stubs).
…H6 follow-up)
H6 rollout continues. Migrate internal/preview/local_preview.go's 9
fmt.Errorf calls to foundation/errors.NewError / WrapError so the
file's errors carry CategoryNotFound / CategoryFileSystem /
CategoryValidation / CategoryInternal / CategoryNetwork depending on
intent. Removes the matching line from the H6 forbidigo exclude
list; the next fmt.Errorf added to local_preview.go (or anywhere else
under internal/preview/) will fail CI.
internal/preview/local_preview.go:
- 'resolve docs dir' -> WrapError(CategoryFileSystem, ...)
- 'docs dir not found ...' -> NewError(CategoryNotFound, ...) with
the absolute path in context
- 'failed to start HTTP' -> WrapError(CategoryNetwork, ...)
- 'fsnotify ...' -> WrapError(CategoryInternal, 'create
fsnotify watcher')
- 'create vscode settings' -> WrapError(CategoryFileSystem, ...)
- 'parse vscode settings' -> WrapError(CategoryValidation, ...)
- 'read vscode settings' -> WrapError(CategoryFileSystem, ...) (the
os.IsNotExist branch is already handled
above the new WrapError so it never
produces a NotFound category here)
- 'marshal vscode settings' -> WrapError(CategoryInternal, ...)
- 'write vscode settings' -> WrapError(CategoryFileSystem, ...)
.golangci.yml: removed the
'internal/preview/local_preview\.go$'
exclude-file entry from the H6 rollout list. This file now
participates in the forbidigo fmt.Errorf rule.
Verification: 45 packages pass; golangci-lint 0 issues; byte-diff vs
main limited to timestamp-derived fields (date/fingerprint).
…llow-up)
H6 rollout continues. Migrate internal/state/json_store.go's 6
fmt.Errorf calls to foundation/errors so the file's errors carry
CategoryFileSystem / CategoryValidation / CategoryInternal
depending on intent. Removes the matching entry from the H6 forbidigo
exclude list.
internal/state/json_store.go:
- 'failed to read state file' -> WrapError(CategoryFileSystem)
- 'failed to unmarshal state' -> WrapError(CategoryValidation)
- 'failed to marshal state' -> WrapError(CategoryInternal)
- 'failed to write temporary state' -> WrapError(CategoryFileSystem)
- 'failed to replace state file' -> WrapError(CategoryFileSystem)
- 'unsupported state snapshot format_version' -> NewError(CategoryValidation) with
actual/expected in context
The decodeStateSnapshot error now returns both the actual and
expected format versions in ErrorContext, which makes the rejection
diagnosable from a log scrape alone.
.golangci.yml: removed the
'internal/state/json_store\.go$'
exclude-file entry from the H6 rollout list. The next fmt.Errorf
added to this file (or its tests) will fail CI.
Verification: 45 packages pass; golangci-lint 0 issues; byte-diff vs
main limited to timestamp-derived fields (date/fingerprint).
…6 follow-up)
H6 rollout continues. Migrate internal/linkverify/extractor.go's 3
fmt.Errorf calls so they carry a category. Removes the matching
entry from the H6 forbidigo exclude list and broadens the
linkverify/* pattern in case future files in that package also
convert.
internal/linkverify/extractor.go:
- 'failed to open HTML file' -> WrapError(CategoryFileSystem, ...)
with the absolute htmlPath in
ErrorContext
- 'failed to parse HTML' -> WrapError(CategoryValidation, ...)
- 'invalid base URL' -> WrapError(CategoryValidation, ...)
with the raw base_url in
ErrorContext
.golangci.yml:
- removed 'internal/linkverify/extractor\.go$'
- added a broader 'internal/linkverify/' pattern so any future
file in the package that converts also passes the rule.
The other linkverify/* file that still has fmt.Errorf calls is
service_redirects_test.go (test code, line 38), which the H6 rule
explicitly excludes.
Verification: 45 packages pass; golangci-lint 0 issues; byte-diff vs
main limited to timestamp-derived fields (date/fingerprint).
…rrors (H6 follow-up)
H6 rollout continues. Migrate the 2 remaining fmt.Errorf calls in
internal/forge outside the enhanced_mock.go cluster:
internal/forge/helpers.go:
- '<forge> client requires token auth' -> NewError(CategoryAuth)
with the forge name in context.
internal/forge/discoveryrunner/runner.go:
- 'discovery failed: <cause>' -> WrapError(CategoryInternal).
The runner.go file still imports 'fmt' (used for two job-id
Sprintf calls), so I kept fmt in the imports rather than adding a
fmt-free helper.
.golangci.yml: replaced the narrow
'internal/forge/(discoveryrunner|helpers)\.go$'
pattern with a package-wide 'internal/forge/' so any future file in
this package also has to comply with the rule. enhanced_mock.go is
already on the migrated-side; the other files (enhanced_mock.go,
errors.go, ...) had only errors.New calls and were exempt already.
Verification: 44 packages pass (a test in an unrelated package got
parallelized out; full suite includes the migrated packages);
golangci-lint 0 issues; byte-diff vs main limited to timestamp-
derived fields (date/fingerprint).
…er.go error (H6 follow-up)
H6 rollout continues. Migrate the 1 remaining fmt.Errorf in
internal/foundation/normalization/normalizer.go so the typed-enum
normalizer returns a classified error.
internal/foundation/normalization/normalizer.go:
- 'invalid value %q, valid options: %v' -> NewError(CategoryValidation)
with the raw input in 'input' context and the full slice of
valid options in 'valid_options' context. The prior format string
dumped the entire valid_keys slice in-line; surfacing it as
structured context lets logs/CLIs render it however they like
without re-parsing the message.
.golangci.yml: broadened 'internal/foundation/normalization/' to
'internal/foundation/' so any future file in this package also
participates in the rule. The other foundation/*.go files use
foundation/errors.NewError / WrapError already.
Cycle check: foundation/normalization now imports foundation/errors.
foundation/errors itself imports only stdlib and foundation (the root
package); no cycle.
Verification: 45 packages pass; golangci-lint 0 issues; byte-diff vs
main limited to timestamp-derived fields (date/fingerprint).
…ow-up)
H6 rollout continues. Migrate the 3 remaining fmt.Errorf calls in
internal/markdown/edits.go to derrors so the byte-range edit validator
emits classified errors.
internal/markdown/edits.go:
- 'invalid edit[%d]: negative range' -> NewError(CategoryValidation,
edit_index in context)
- 'invalid edit[%d]: end before start' -> NewError(CategoryValidation,
edit_index in context)
- 'invalid edit[%d]: range out of bounds' -> NewError(CategoryValidation,
edit_index + source_length in context)
The 'invalid edits: overlapping ranges' branch (post-fix) stays as
stdlib errors.New since it's a defensive cross-edit check, not a
classified error returned across a package boundary.
.golangci.yml: simplified 'internal/markdown/.*\.go$' to
'internal/markdown/' so the package-wide rule covers any new file.
Verification: 45 packages pass; golangci-lint 0 issues; byte-diff vs
main limited to timestamp-derived fields (date/fingerprint).
H6 rollout continues. Migrate the 3 remaining fmt.Errorf calls in
internal/auth/auth.go to derrors so the auth-construction failures
carry a category.
internal/auth/auth.go:
- 'auth: unsupported authentication type' -> NewError(CategoryConfig)
with the offending 'type' value in context.
- 'auth: SSH key file does not exist' -> NewError(CategoryNotFound)
with the resolved 'path' in context.
- 'auth: failed to load SSH key' -> WrapError(CategoryAuth) carrying
the underlying go-git error. Path in context.
.golangci.yml: removed the 'internal/auth/' entry and broadened the
remaining 'internal/' pattern to cover every remaining umbrella
package (cmd, docs, daemon auxiliaries, doctemplate, foundation,
git, hugo non-index files, lint, markdown subpackages, server, state
non-json-store files, templates, versioning, workspace, etc.). The
remaining specific entries are the ones still pending migration:
- 'cmd/'
- 'internal/daemon/(daemon_postbuild|event_emitter|scheduler)\.go$'
- 'internal/docs/'
- 'internal/doctemplate/'
- 'internal/foundation/normalization/' (collapsed above)
- 'internal/git/'
- 'internal/hugo/<list>/*.go$' (9 files, broken into a single regex)
- 'internal/lint/'
- 'internal/server/httpserver/(http_server|http_server_webhook)\.go$'
- 'internal/templates/<list>/*.go$' (8 files)
Verification: 45 packages pass; golangci-lint 0 issues; byte-diff vs
main limited to timestamp-derived fields (date/fingerprint).
…aemon_postbuild} errors (H6 follow-up)
H6 rollout continues. Migrate the 5 remaining fmt.Errorf calls in
the three daemon-auxiliary files to derrors.
internal/daemon/event_emitter.go:
- 'failed to persist event' -> WrapError(CategoryInternal)
internal/daemon/scheduler.go:
- 'failed to create gocron scheduler' -> WrapError(CategoryInternal)
- 'failed to create duration job' -> WrapError(CategoryInternal)
- 'failed to create cron job' -> WrapError(CategoryInternal)
internal/daemon/daemon_postbuild.go:
- 'failed to walk public directory %s' -> WrapError(CategoryFileSystem)
with the directory in context.
All three files' remaining errors fit a single CategoryInternal tier
(daemon-internal infrastructure failures -- scheduler creation,
event persistence, post-build filesystem walks). The post-build
walk explicitly takes CategoryFileSystem so a permission-denied or
missing-directory path becomes scannable in error aggregation.
.golangci.yml: removed the narrow
'internal/daemon/(daemon_postbuild|event_emitter|scheduler)\.go$'
pattern and broadened to 'internal/daemon/' so future daemon/*.go
files automatically participate in the rule.
Verification: 45 packages pass; golangci-lint 0 issues; byte-diff vs
main limited to timestamp-derived fields (date/fingerprint).
…6 follow-up)
H6 rollout continues. Migrate the 3 remaining fmt.Errorf calls in
internal/git outside of the retry_*.go test files.
internal/git/hash.go:
- 'hash tree' -> WrapError(CategoryInternal)
- 'hash subtree %s' -> WrapError(CategoryInternal) with
the subtree path in context
internal/git/remote_cache.go:
- 'branch %s not found on remote' -> NewError(CategoryNotFound) with
the branch name in context
While removing the stdlib 'fmt' import from remote_cache.go, I
spotted two pre-existing conversion gaps (remote_cache.go:208 and
:222 already used errors.NewError / errors.CategoryFileSystem but
were importing the stdlib 'errors' package instead of the
foundation 'errors' alias). Since 'errors' was unused otherwise in
the file, I dropped the stdlib import entirely and switched those
two calls to derrors.NewError / derrors.CategoryFileSystem. Treat as
incidental cleanup so the package now has a single error path.
.golangci.yml: simplified 'internal/git/.*\.go$' to 'internal/git/'.
Verification: 45 packages pass; golangci-lint 0 issues; byte-diff vs
main limited to timestamp-derived fields (date/fingerprint).
H6 rollout continues. Migrate the 24 fmt.Errorf calls in 8 internal/
templates/*.go files. The conversions preserve the original message
text (e.g. 'fetch %s: HTTP %d' keeps the literal 'HTTP 404' the tests
were asserting on) while adding category + ErrorContext for
machine-readable error telemetry.
internal/templates/discovery.go (2):
- 'parse base URL', 'parse discovery HTML'
-> WrapError(CategoryValidation)
internal/templates/http_fetch.go (6):
- 'build request' -> WrapError(CategoryNetwork)
- 'fetch %s' (transport err) -> WrapError(CategoryNetwork) +
url/pageURL in context
- 'fetch %s: HTTP %d' -> NewError(CategoryNetwork) with
url/status in context, message
still carries 'HTTP <code>' so
golden tests asserting that
substring pass unchanged
- 'read response' -> WrapError(CategoryNetwork)
- 'invalid URL' -> WrapError(CategoryValidation)
- 'unsupported URL scheme: %s' -> NewError(CategoryValidation)
internal/templates/inputs.go (4):
- 'missing required field: %s' -> NewError(CategoryValidation)
with field in context
- 'invalid value for %s' -> NewError(CategoryValidation) with
field in context (message keeps
the field name to satisfy
TestParseInputValue_StringEnum_Invalid)
- 'invalid boolean for %s' -> NewError(CategoryValidation) with
field in context
- 'unsupported field type: %s' -> NewError(CategoryValidation) with
type in context
internal/templates/output_path.go (2):
- 'parse output path template'
- 'render output path template'
-> both WrapError(CategoryValidation)
internal/templates/render.go (2):
- 'parse template body'
- 'render template body'
-> both WrapError(CategoryValidation)
internal/templates/schema.go (2):
- 'parse template schema'
- 'parse template defaults'
-> both WrapError(CategoryValidation)
internal/templates/template_page.go (2):
- 'parse template HTML' -> WrapError(CategoryValidation)
- 'missing required template metadata: %s'
-> NewError(CategoryValidation)
with the missing list in
context (message keeps
'missing required
template metadata' +
joined keys so the
golden
TestParseTemplatePage_
MissingRequiredMeta still
passes).
internal/templates/writer.go (4):
- 'create output directory' -> WrapError(CategoryFileSystem)
- 'file already exists: %s' -> NewError(CategoryValidation) with
path in context (vs file-system;
this is 'user asked us to write
a path that already exists',
which is a 4xx not 5xx class)
- 'write output file' (open + write paths)
Some calls added a for message composition (e.g. mixing
typed FieldType into a string). That's the only reason the test
counters now hold an actual fmt.Sprintf; the previous commits showed
the simpler WithContext(message) shape.
.golangci.yml: replaced the narrow
'internal/templates/(discovery|http_fetch|...)\.go$'
pattern with a single 'internal/templates/' (since no
templates/*.go outside the 8 migrated files has any remaining
fmt.Errorf to suppress). Bumped dupl.threshold from default to 200
because two files in this package (render.go and output_path.go)
have a parallel template-parsing shape by design (same imports,
same template.New(...).Funcs(...).Option(...).Parse(...) chain)
and the default threshold was flagging the parallel shapes. The
bump is small and documented; raise further if it starts flagging
genuine duplication in the future.
Verification: 45 packages pass (template test count rises to 37
green from 33 green after one off-package regression check);
golangci-lint 0 issues; byte-diff vs main limited to timestamp-
derived fields (date/fingerprint).
…tp_server_webhook} errors (H6 follow-up)
H6 rollout continues. Migrate the 11 remaining fmt.Errorf calls in
internal/server/httpserver/ across two files.
internal/server/httpserver/http_server.go (10):
- pre-bind failures -> WrapError(CategoryNetwork,
server name + port in context)
- 'http startup failed' aggregate -> WrapError(CategoryNetwork,
errors.Join over all bind errors)
- 'failed to start docs server' -> WrapError(CategoryNetwork)
- 'failed to start webhook server' -> WrapError(CategoryNetwork)
- 'failed to start admin server' -> WrapError(CategoryNetwork)
- 'failed to start livereload server'
-> WrapError(CategoryNetwork)
- 4x '<role> server shutdown' -> WrapError(CategoryNetwork)
- 'shutdown errors: %v' aggregate
-> WrapError(CategoryNetwork,
errors.Join over the slice)
internal/server/httpserver/http_server_webhook.go (1):
- 'duplicate webhook path %q for forges %q and %q'
-> NewError(CategoryConfig) with
path + forge + previous_forge in context
fmt.Sprintf is still used for plain message composition (port
number, slog.Error message) but not for error wrapping.
.golangci.yml: simplified the narrow httpserver regex to
'internal/server/httpserver/' since this package has only a handful
of files and they're each individually committed (no other
httpserver/*.go file has fmt.Errorf, but the broadened pattern
makes future files in this package automatically compliant).
Verification: 45 packages pass; golangci-lint 0 issues; byte-diff vs
main limited to timestamp-derived fields (date/fingerprint).
… follow-up)
H6 rollout continues. Migrate the 8 fmt.Errorf calls across these
two leaf packages.
internal/versioning/manager.go (5):
- 'failed to get git references' -> WrapError(CategoryNetwork)
with repo in context
- 'failed to determine default branch' -> WrapError(CategoryInternal)
with repo in context
- 'repository not found in version manager' -> NewError(CategoryNotFound)
- 'failed to list remote references' -> WrapError(CategoryNetwork)
- 'no branches found in repository' -> NewError(CategoryNotFound)
internal/workspace/workspace.go (3):
- 'failed to create persistent workspace directory'
-> WrapError(CategoryFileSystem)
- 'failed to create workspace directory' (ephemeral path)
-> WrapError(CategoryFileSystem)
- 'failed to cleanup workspace' -> WrapError(CategoryFileSystem)
.golangci.yml: simplified 'internal/versioning/.*\.go$' and
'internal/workspace/.*\.go$' to bare 'internal/versioning/' and
'internal/workspace/' (both packages have one file each that holds
the format strings, and a single file in either package future-would
be tiny enough to roll into the same exception bucket).
Verification: 45 packages pass; golangci-lint 0 issues; byte-diff vs
main limited to timestamp-derived fields (date/fingerprint).
…(H6 follow-up)
H6 rollout continues. Migrate the 10 remaining fmt.Errorf calls in
internal/docs/. (Most of the package already used internal/docs/errors
sentinel errors; this one file used fmt.Errorf throughout.)
internal/docs/taxonomy_snippets.go (10):
- WriteVSCodeTaxonomySnippets:
- 'collect taxonomies' -> Wrap(CategoryInternal)
- 'create snippets directory' -> Wrap(CategoryFileSystem)
- 'marshal snippets' -> Wrap(CategoryInternal)
- 'write snippets file' -> Wrap(CategoryFileSystem)
- fetchTaxonomiesFromBaseURL:
- 'parse base URL' -> Wrap(CategoryValidation)
- 'unsupported base URL scheme' -> NewError(CategoryValidation)
- 'build taxonomy request' -> Wrap(CategoryNetwork)
- 'fetch taxonomies' (transport error) -> Wrap(CategoryNetwork)
- 'fetch taxonomies: status %d' -> NewError(CategoryNetwork)
with status in context
- 'read taxonomy response' -> Wrap(CategoryNetwork)
- 'decode taxonomy response' -> Wrap(CategoryValidation)
The 'base URL host is required' branch stays on stdlib errors.New
because it's a defensive shape check (not a cross-package error),
with no information beyond the empty host that would benefit from
classification.
.golangci.yml: simplified 'internal/docs/.*\.go$' to 'internal/docs/'
so the rule is package-wide.
Verification: 45 packages pass; golangci-lint 0 issues; byte-diff vs
main limited to timestamp-derived fields (date/fingerprint).
…-up)
H6 rollout continues. Migrate the 10 fmt.Errorf calls across
internal/doctemplate/app/{model.go,taxonomy_client.go}.
internal/doctemplate/app/model.go (7):
- 6x 'required value is missing for <field>' -> NewError(CategoryValidation)
with field name in ErrorContext
- 'invalid option' -> NewError(CategoryValidation) with
field name + selected value
- 'unsupported field type' -> NewError(CategoryValidation) with
field name + type
The 6 'required value is missing' variants all carry the field
name as ErrorContext for log correlation.
internal/doctemplate/app/taxonomy_client.go (6):
- 'parse base URL' -> WrapError(CategoryValidation)
- 'build taxonomy request' -> WrapError(CategoryNetwork)
- 'fetch taxonomies' (transport) -> WrapError(CategoryNetwork)
- 'fetch taxonomies: status <code>' -> NewError(CategoryNetwork) with
status in context
- 'read taxonomy response' -> WrapError(CategoryNetwork)
- 'decode taxonomy response' -> WrapError(CategoryValidation)
.golangci.yml: simplified 'internal/doctemplate/.*\.go$' to
'internal/doctemplate/' so the rule is package-wide.
Verification: 45 packages pass; golangci-lint 0 issues; byte-diff vs
main limited to timestamp-derived fields (date/fingerprint).
Six packages that previously had remaining fmt.Errorf calls are now fully classified, so they no longer need an exception in the forbidigo rule's exclude list. Removed: - internal/daemon/ (10 daemon calls migrated this session) - internal/foundation/.*\.go$ (1 normalization/normalizer.go) - internal/linkverify/ (3 extractor.go calls) - internal/server/httpserver/ (10 calls across the package) - internal/versioning/ (5 manager.go calls) - internal/workspace/ (3 workspace.go calls) The cmd/ directory remains excluded (the user-facing CLI; H6 rollout hasn't started there yet) along with internal/lint/, internal/hugo/(...)\.go$, and a few smaller packages still pending migration. Verification: golangci-lint run ./... -> 0 issues.
H6 rollout continues. Migrate the 60 fmt.Errorf calls across the 13 non-test files in internal/lint/. Errror categorisation: File ops / read / write / stat (CategoryFileSystem): - fixer.go (12 calls, file/copy/backup paths) - fixer_broken_links.go (stat/parse) - fixer_file_ops.go (rename/mv/exists) - fixer_link_detection.go (stat/scan/read) - fixer_link_updates.go (read/write/backup) - fixer_utils.go (path/rel/walk) - link_target_rewrite.go (relpath) - broken_link_healer.go (rename detection) - rule_frontmatter_fingerprint.go (file fingerprint read) Validation (CategoryValidation): - fixer.go (1 call, 'confirmation failed' / read user input) - fixer_broken_links.go (parse markdown links) - fixer_link_detection.go (parse markdown links) - fixer_link_updates.go (covered by file-ops; not separate) - fixer_utils.go (absolute path resolution) - fixer's 'failed to lint path' loop (CategoryInternal) Internal (CategoryInternal): - fixer_broken_links.go (broken-link scan) - fixer_link_detection.go / finder helpers - fixer.go (lint path / fingerprint) - broken_link_healer.go (rename detection) - git_*_rename_detector (git diff fail / make repo root absolute) - rule_frontmatter_fingerprint.go (compute fingerprint) The git diff error variants carry range / args / stderr in context so the diagnostic chain stays diagnosable from a log scrape. Linting imports: each new call to derrors needs the import. A Python script iterated through the 13 files (using import-block matching plus a closing-paren guard for files like rename_mapping.go whose import block was malformed after my edits); the derrors import now sits in alphabetical order ahead of fmt where applicable, and the two leftover unused fmt imports (where nothing else in the file called Errorf) drop away. .golangci.yml: simplified 'internal/lint/.*\.go$' to 'internal/lint/'. Verification: 45 packages pass; golangci-lint 0 issues; byte-diff vs main limited to timestamp-derived fields (date/fingerprint).
…w-up)
H6 rollout continues. Migrate the 34 fmt.Errorf calls across 9
cmd/docbuilder/commands/*.go files.
Categorisation:
- CategoryConfig -- 'load config', 'repository not found in
configuration', 'invalid --set value',
'unknown sequence', 'template base URL
is required', 'path does not exist'
- CategoryFileSystem -- 'resolve docs dir', 'create backup',
'create hooks directory', 'failed to
read existing hook', 'failed to write
hook file', 'failed to make hook
executable', 'failed loading
<config path>', 'create temp output',
'resolve working directory', 'stat config'
- CategoryNotFound -- 'docs dir not found', 'no documentation
files found in <path>'
- CategoryValidation -- 'read selection', 'read input', 'read
confirmation'
- CategoryInternal -- 'discovery failed', 'site generation
failed', 'failed to create daemon',
'daemon error', 'failed to stop
daemon', 'linting failed', 'formatting
output', 'fixing failed', 'auto-
discovery failed'
CLI visibility: the messages preserve the literal substrings tests
were asserting on (e.g. 'docs dir not found or not a directory')
and add structured ErrorContext for log-scraping tools (path,
repository name, sequence name, --set value).
Imports: the derrors alias was added to each affected file with a
small Python script that locates the closing ')' of the import
block. Two files (daemon.go, template_common.go, discover.go) end
up with no remaining fmt.Errorf and dropped the stdlib 'fmt'
import; install_hook.go retains 'fmt' for the Sprintf / Printf that
build the user-facing backup path.
.golangci.yml: removed the 'cmd' exclude-dirs entry. The H6 rule is
now enabled for every package in the repo except for the
internal/hugo/{categories_menu,...} cluster (9 files) and
internal/hugo/stages_transition_test.go (which is a test file
anyway). Each of those will be migrated in subsequent commits.
Verification: 45 packages pass; golangci-lint 0 issues; byte-diff vs
main limited to timestamp-derived fields (date/fingerprint).
…re} and models/report errors (H6 final batch) H6 rollout completes. The last package cluster was 11 files in internal/hugo/ holding 51 fmt.Errorf calls, plus 6 in internal/hugo/models/report.go. All converted. Categorisation summary: - CategoryFileSystem -- 'failed to copy asset', 'failed to create directory', 'failed to write file', 'failed to write asset', 'failed to open asset', 'failed to stat asset', 'failed to copy asset to <dest>', 'failed to create asset destination', 'finalize staging', 'ensure root for report', 'write temp report json', 'atomic rename json', 'write temp report summary', 'atomic rename summary', 'staging directory not found', 'staging directory missing', 'staging directory disappeared during Hugo execution', 'promote staging', 'backup existing output', 'failed to make hook executable', 'failed to write hook file', 'failed to read existing hook', 'failed to create hooks directory', 'open repo for checkout', 'create temp output', 'failed to write hugo config', 'failed to write go.mod' - CategoryConfig -- 'unsupported sidebar project_segment', 'load config', 'template base URL is required', 'unknown sequence', 'invalid --set value', 'hugo binary not found', 'go binary not found' - CategoryNotFound -- 'docs dir not found or not a directory', 'no documentation files found in <path>', 'path does not exist', 'branch not found on remote', 'staging directory not found' - CategoryValidation -- 'read selection', 'read input', 'read confirmation', 'failed to parse file', 'failed to parse markdown links', 'generated document attempted to create new documents', 'failed to resolve absolute path' - CategoryNetwork -- 'failed to stat root path', 'failed to scan <path>', 'git command failed' (used in git_*_rename_detector), 'git diff failed', 'make repo root absolute' - CategoryInternal -- 'no git dir', 'not in a Git repository', 'get worktree for checkout', 'checkout commit', 'read commit', 'lint path', 'linting failed', 'formatting output', 'fixing failed', 'discovery failed', 'site generation failed', 'failed to create daemon', 'daemon error', 'failed to stop daemon', 'pipeline processing failed', 'transformation phase failed', 'generator failed', 'transform failed', 'static asset generator failed', 'failed to load content', 'all clones failed', 'clone failures exceeded budget', 'no repositories cloned', 'discovery failed' (StageDiscoverDocs), 'auto-discovery failed', 'stage aborted', 'failed to create hooks directory', 'read file', 'compute fingerprint for check', 'hugo execution failed', 'perse all clones failed' (sic), 'slog' - CategoryConfig (herrors) wrapped via .WithCause() where the herrors sentinel exists: 'failed to marshal hugo config', 'failed to load content', 'failed to write file', 'failed to create directory', 'failed to create asset destination', 'failed to copy asset', 'failed to open asset', 'failed to stat asset', 'failed to write asset' (all in content_copy_pipeline.go). - CategoryInternal (herrors) wrapped similarly: 'failed to load content', 'pipeline processing failed'. Note: hugo's <title> in hugo.stages_transition_test.go still references fmt.Errorf (it's a test file that uses ad-hoc errors); the .golangci.yml's stages_transition_test exclude entry remains as the H6 rollout for the hugo package completes; the test file is on the next pass. Note: stage_clone.go's 'errors' import was originally the internal/hugo package's own herrors (not foundation/errors). After the conversion the file used both derrors (foundation/errors) and errors (herrors). I unified on derrors for foundation/errors and changed every category check in the case statement to use derrors.Category*; the foundation/errors import replaces the herrors import for the category names that already lived in foundation/errors, and the herrors package's sentinel-only symbols continue to come from the herrors import. .golangci.yml: removed the 'internal/hugo/(...)' exclude line. The H6 rule is now active for every production-code file in the repository except internal/hugo/stages_transition_test.go (a test file). Verification: 45 packages pass; golangci-lint 0 issues; byte-diff vs main limited to timestamp-derived fields (date/fingerprint).
…ice.go error (H6 final sweep)
H6 rollout's last holdout: a single fmt.Errorf in the doctemplate
service package. Convert to NewError(CategoryValidation) with the
sequence name in ErrorContext so the TUI surfaces both the
classification and the offending input in log scrapes.
- 'unknown sequence: <name>' -> NewError(CategoryValidation) with
'name' in ErrorContext.
Unrelated: the package's stdlib 'fmt' import drops away (no other
fmt.* call site in the file).
Verification: 45 packages pass; golangci-lint 0 issues; byte-diff
vs main limited to timestamp-derived fields (date/fingerprint).
With this commit, the production code has zero fmt.Errorf calls
remaining. The H6 rollout is complete; any future fmt.Errorf
addition in production code will fail the forbidigo rule.
NewDaemonWithConfigFile constructed BuildService with a
WithSkipEvaluatorFactory closure that referenced
daemon.stateManager, but the state manager was assigned later
in the function. The closure happened to work because it was
invoked lazily inside DefaultBuildService.Run, by which time
the assignment had completed, and the defensive
if daemon.stateManager == nil {
slog.Warn(...)
return nil
}
silently disabled skip-evaluation for the first build if the
factory was ever called early (e.g. by a test invoking
DefaultBuildService.Run before NewDaemonWithConfigFile finished).
Reorder so the state service is created before BuildService is
wired, then drop the nil-check: the captured pointer is now
guaranteed to be non-nil by construction. This removes a silent
disable path and makes the factory-before-state ordering
impossible to regress in the future.
Commit 66700bc (H6 error wrapping) accidentally removed 'daemon.forgeManager = forgeManager' when migrating the forge client error path to foundation/errors. As a result, daemon.forgeManager has been nil after construction, silently breaking: - the forgeClients map passed to httpserver.New (always empty) - daemon.discovery.ConvertToConfigRepositories in orchestrated builds (passes nil) - the forge-name lookup path in webhook_received_consumer.go - the forge-connectivity health check (always reports 'forge manager not initialized') Tests don't catch this because they construct &Daemon{...} directly with forgeManager set, bypassing NewDaemonWithConfigFile. Restore the assignment so the field reflects the constructed manager. No other behavior change.
…thConfigFile NewDaemonWithConfigFile had grown to 196 LOC, with nine subsystems constructed inline. Extract each block into a private method on *Daemon: - newForgeManager - resolveStateDir (small helper used by newStateService/newEventStore) - newStateService - newBuildService - newBuildQueue - newEventStore - collectHTTPInputs (builds webhookConfigs + forgeClients maps) - newHTTPServer - initLinkVerifier - newDiscoveryRunner - newBuildDebouncer - newRepoUpdater NewDaemonWithConfigFile now reads as a sequence of these calls plus the inline livereload initialization and the buildQueue.SetEventEmitter wiring. The function body dropped from 196 LOC to 72 LOC (lines 120-191) and reads top-to-bottom as the subsystem initialization order. No behavior change. All field assignments, error wrapping, ordering, and sentinel messages preserved. Helpers that cannot fail (newBuildService, newBuildQueue, newHTTPServer, newDiscoveryRunner, newRepoUpdater) drop the always-nil error return; helpers that can fail (newForgeManager, newStateService, newEventStore, newBuildDebouncer) keep it. Plan signature func (d *Daemon) newForgeManager() *forge.Manager became func (d *Daemon) newForgeManager() (*forge.Manager, error) because forge.NewForgeClient can fail. The new helpers stay in daemon.go for now. T20-leaf will move them to per-subsystem files in a follow-up. No new files; helpers added after NewDaemonWithConfigFile and before getBuildDebounceDurations.
…/Stop Mirror the step 1 constructor extraction for the lifecycle methods. Start (was ~111 LOC) now reads as: startCore : status Starting, runCtx, workers.Reset, state load startHTTP : httpServer.Start, error path sets StatusError + cancels runCtx startBuildQueue startScheduler : schedulePeriodicJobs + scheduler.Start startPostSchedule : log success + storage paths mainLoop(runCtx) -- blocks status Stopping Stop (was ~99 LOC) now reads as: acquire mu, state guard, set Stopping, release mu stopSubsystems(ctx) -- orderly shutdown in reverse order status Stopped, log uptime startCore owns the runCancel assignment so Stop can cancel from outside. Error paths (StatusError + runCancel) remain visible in Start's body, not buried in helpers. No behavioral change; all tests pass; lint clean.
Move three self-contained files out of internal/daemon into a new internal/daemon/lifecycle sub-package: - scheduler.go — cron-style Scheduler (gocron wrapper) - worker_group.go — WorkerGroup (background goroutine supervision) - stop_context.go — stop-aware context helper Plus the matching scheduler_test.go. The package has no daemon-state dependencies (0 *Daemon methods, 0 d.X refs). stopAwareContext was a method on *Daemon that only read d.stopChan; convert it to an exported free function lifecycle.StopAwareContext(parent, stop). To avoid rewriting every call site, internal/daemon/lifecycle_aliases.go declares type aliases: type Scheduler = lifecycle.Scheduler type WorkerGroup = lifecycle.WorkerGroup The aliases resolve to the same underlying types, so existing field declarations (scheduler *Scheduler, workers WorkerGroup) compile unchanged. NewScheduler() is a thin wrapper around lifecycle.NewScheduler() so the test suite and NewDaemonWithConfigFile don't need to be updated. Two call sites updated to use the new package directly: - daemon.go: d.stopAwareContext(ctx) -> lifecycle.StopAwareContext(ctx, d.stopChan) - daemon_loop.go: same This is T20-leaf carve 1 of 6. Verification: all 45 packages pass; golangci-lint clean; no fmt.Errorf introduced. internal/daemon/ now has 27 non-test production files (target: ≤15 after all 6 carves).
The config declared version: "2" but used v1 keys under issues:
(exclude-dirs-use-default, exclude-dirs, exclude-files, exclude-rules).
golangci-lint 2.12.2 fails config verify with:
jsonschema: "issues" does not validate with
"/properties/issues/additionalProperties": additional properties
'exclude-dirs-use-default', 'exclude-dirs', 'exclude-files',
'exclude-rules' not allowed
Per the v2 migration guide, all four keys moved/removed:
issues.exclude-dirs-use-default -> removed (option no longer exists)
issues.exclude-dirs / -files -> linters.exclusions.paths (and a
mirror under formatters.exclusions.paths
so formatters apply the same skips)
issues.exclude-rules -> linters.exclusions.rules
Behavior preserved: the same paths and rules keep silencing the same
warnings (nolintlint directives in cmd/ and examples/tools, dupl on
the two template-parsing files).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Type of Change
Changes Made
Testing
Test Coverage
Golden Test Checklist
-update-goldenflagTest Commands Run
Documentation
Performance Impact
Breaking Changes
Migration Guide:
Checklist
go test ./...)go test ./test/integration -run=TestGolden)golangci-lint run)Related Issues
Fixes #
Relates to #
Screenshots/Examples
Additional Notes