mirror of
https://github.com/Dvorinka/Devour.git
synced 2026-06-05 04:53:02 +00:00
update
This commit is contained in:
Tomas Dvorak
@@ -0,0 +1 @@
|
||||
{"batch":"Architecture & Coupling","batch_index":1,"assessments":{"cross_module_architecture":100.0},"dimension_notes":{"cross_module_architecture":{"evidence":["All assigned `internal/quality/*.go` files stay within a single package boundary (`package quality`) and only import standard library packages (`time`, `testing`, `strings`), with no cross-package dependency fan-out from this slice.","`pkg/rustdocs/parser_test.go` is isolated to `package rustdocs` and imports only stdlib plus `github.com/PuerkitoBio/goquery`; it does not couple into `internal/quality` types or helpers.","No `init()` functions, package-level mutable singleton wiring, or import-time execution patterns were found in the reviewed files; behavior is test-function scoped and constructor-invoked (`NewParser`, `NewScorer`, `NewNarrativeGenerator`).","Type declarations in `internal/quality/types.go` and `internal/quality/enhanced_types.go` are cohesive data-model definitions within one module boundary rather than cross-module shims or compatibility layers."],"impact_scope":"local","fix_scope":"single_edit","confidence":"high","unreported_risk":"This batch covers only five files; architectural hotspots could still exist in non-assigned packages (e.g., runtime wiring or broader dependency graph) outside this evidence window."}},"findings":[]}
|
||||
@@ -0,0 +1,97 @@
|
||||
{
|
||||
"batch": "Abstractions & Dependencies",
|
||||
"batch_index": 2,
|
||||
"assessments": {
|
||||
"abstraction_fitness": 68.0
|
||||
},
|
||||
"dimension_notes": {
|
||||
"abstraction_fitness": {
|
||||
"evidence": [
|
||||
"Language-to-doc behavior is spread across multiple large switches: URL construction in cmd/get.go:78-173, type mapping in cmd/get.go:175-205, and term derivation in cmd/ask.go:205-260+.",
|
||||
"External scraper implementations repeat the same transport/change-detection scaffold (config+parser+http client fields, URL check, fetchPage, generateHash, DetectChanges) across multiple files, e.g. internal/scraper/external/godocs.go:17-121, internal/scraper/external/javadocs.go:16-115, internal/scraper/external/nuxtdocs.go:16-120, internal/scraper/external/cloudflaredocs.go:16-105.",
|
||||
"Vector store abstraction exposes implementations that are selected by default config but intentionally unimplemented: internal/config/config.go:121-125 defaults to chromem, while internal/vector/store.go:221-243 returns \"chromem store not implemented\" for all operations.",
|
||||
"Configuration defaults are duplicated in two representations (typed defaults and hand-written YAML template), increasing drift risk: cmd/init.go:92-149 and internal/config/config.go:104-160."
|
||||
],
|
||||
"impact_scope": "subsystem",
|
||||
"fix_scope": "architectural_change",
|
||||
"confidence": "high",
|
||||
"sub_axes": {
|
||||
"abstraction_leverage": 62.0,
|
||||
"indirection_cost": 71.0,
|
||||
"interface_honesty": 60.0
|
||||
}
|
||||
}
|
||||
},
|
||||
"findings": [
|
||||
{
|
||||
"dimension": "abstraction_fitness",
|
||||
"identifier": "language_catalog_scattered_switches",
|
||||
"summary": "Language routing logic is duplicated across CLI flows instead of one catalog abstraction",
|
||||
"related_files": [
|
||||
"cmd/get.go",
|
||||
"cmd/ask.go"
|
||||
],
|
||||
"evidence": [
|
||||
"cmd/get.go:78-173 defines a large language switch for URL building; cmd/get.go:175-205 defines a second switch for source type mapping.",
|
||||
"cmd/ask.go:205-260+ adds a third language switch for term heuristics, creating three independent sources of truth for one domain model."
|
||||
],
|
||||
"suggestion": "Introduce a single `LanguageSpec` registry (aliases, source type, URL builder, optional query-term strategy) in one package and have both `get` and `ask` consume it; keep per-language behavior as data/functions attached to that registry.",
|
||||
"confidence": "high",
|
||||
"impact_scope": "subsystem",
|
||||
"fix_scope": "architectural_change"
|
||||
},
|
||||
{
|
||||
"dimension": "abstraction_fitness",
|
||||
"identifier": "external_scraper_scaffold_duplication",
|
||||
"summary": "External scraper adapters reimplement the same transport/hash lifecycle repeatedly",
|
||||
"related_files": [
|
||||
"internal/scraper/external/godocs.go",
|
||||
"internal/scraper/external/javadocs.go",
|
||||
"internal/scraper/external/nuxtdocs.go",
|
||||
"internal/scraper/external/cloudflaredocs.go"
|
||||
],
|
||||
"evidence": [
|
||||
"Each file defines near-identical struct fields (`config`, `parser`, `client`), constructor wiring, URL-required guard, `fetchPage`, `generateHash`, and `DetectChanges` flow (e.g., godocs.go:17-121 and javadocs.go:16-115).",
|
||||
"Duplication scales linearly with each new source adapter, increasing edit surface for cross-cutting behavior (timeouts, headers, error mapping)."
|
||||
],
|
||||
"suggestion": "Extract a shared `HTTPDocScraperBase` (or composable helper functions) for request execution, status handling, hashing, and change detection; keep each adapter focused on parser invocation and domain-specific document mapping.",
|
||||
"confidence": "high",
|
||||
"impact_scope": "subsystem",
|
||||
"fix_scope": "multi_file_refactor"
|
||||
},
|
||||
{
|
||||
"dimension": "abstraction_fitness",
|
||||
"identifier": "default_selects_unimplemented_store",
|
||||
"summary": "Store interface contract is dishonest because default backend is not operational",
|
||||
"related_files": [
|
||||
"internal/vector/store.go",
|
||||
"internal/config/config.go"
|
||||
],
|
||||
"evidence": [
|
||||
"internal/config/config.go:121-125 sets default vector DB type to `chromem`.",
|
||||
"internal/vector/store.go:221-243 returns `chromem store not implemented` for all `Store` operations after `NewStore` can select that backend (store.go:63-72)."
|
||||
],
|
||||
"suggestion": "Either implement `ChromemStore` before exposing it as default, or switch default to a working backend and gate chromem behind explicit opt-in plus capability check at startup.",
|
||||
"confidence": "high",
|
||||
"impact_scope": "module",
|
||||
"fix_scope": "architectural_change"
|
||||
},
|
||||
{
|
||||
"dimension": "abstraction_fitness",
|
||||
"identifier": "config_defaults_double_encoded",
|
||||
"summary": "Initialization defaults are encoded twice with different abstractions",
|
||||
"related_files": [
|
||||
"cmd/init.go",
|
||||
"internal/config/config.go"
|
||||
],
|
||||
"evidence": [
|
||||
"cmd/init.go:92-149 hardcodes YAML defaults as a template string.",
|
||||
"internal/config/config.go:104-160 hardcodes defaults again in typed structs, requiring synchronized updates across two representations."
|
||||
],
|
||||
"suggestion": "Generate init YAML from `config.Default()` via marshal + small post-processing/comments, or maintain a single canonical defaults schema consumed by both loader and init command.",
|
||||
"confidence": "high",
|
||||
"impact_scope": "module",
|
||||
"fix_scope": "multi_file_refactor"
|
||||
}
|
||||
]
|
||||
}
|
||||
@@ -0,0 +1,88 @@
|
||||
{
|
||||
"batch": "Governance & Contracts",
|
||||
"batch_index": 3,
|
||||
"assessments": {
|
||||
"cross_module_architecture": 82.0,
|
||||
"test_strategy": 74.0
|
||||
},
|
||||
"dimension_notes": {
|
||||
"cross_module_architecture": {
|
||||
"evidence": [
|
||||
"`internal/quality/types.go` defines a typed status contract (`type Status string` with constants like `StatusOpen`, `StatusFixed`, `StatusWontfix`).",
|
||||
"`internal/quality/types.go` also defines `Scorecard.StatusByType map[string]int`, which bypasses the typed status contract at the module boundary.",
|
||||
"`internal/quality/scoring_test.go` asserts raw string keys (`\"open\"`, `\"fixed\"`) instead of using `Status` constants, reinforcing stringly-typed cross-component coupling.",
|
||||
"`README.md` claims quality features include tracking resolution states, but the in-code state transport for scorecards is weakly typed."
|
||||
],
|
||||
"impact_scope": "subsystem",
|
||||
"fix_scope": "multi_file_refactor",
|
||||
"confidence": "high"
|
||||
},
|
||||
"test_strategy": {
|
||||
"evidence": [
|
||||
"`internal/quality/narrative_test.go` validates exact headline/action prose and directly tests internal helper behavior (e.g., `determinePhase`, `generateHeadline`, `classifyDimension`), creating high implementation-coupling.",
|
||||
"`internal/quality/scoring_test.go` similarly focuses on exact internal scoring details and string key literals, which makes refactors noisy and discourages safe design changes.",
|
||||
"`pkg/rustdocs/parser_test.go` is heavily happy-path: it checks successful parses and minimal field presence but has no malformed-input/error-path cases for parser resilience.",
|
||||
"`README.md` marks parts of the CLI as unstable/stubbed, but assigned tests do not provide cross-module contract/integration safety nets for those runtime boundaries."
|
||||
],
|
||||
"impact_scope": "subsystem",
|
||||
"fix_scope": "multi_file_refactor",
|
||||
"confidence": "high"
|
||||
}
|
||||
},
|
||||
"findings": [
|
||||
{
|
||||
"dimension": "cross_module_architecture",
|
||||
"identifier": "status_contract_string_map_boundary",
|
||||
"summary": "Scorecard state uses string keys instead of shared Status type, weakening module contracts.",
|
||||
"related_files": [
|
||||
"internal/quality/types.go",
|
||||
"internal/quality/scoring_test.go",
|
||||
"README.md"
|
||||
],
|
||||
"evidence": [
|
||||
"`internal/quality/types.go` defines `Status` constants but `Scorecard.StatusByType` is `map[string]int`.",
|
||||
"`internal/quality/scoring_test.go` asserts `card.StatusByType[\"open\"]` and `card.StatusByType[\"fixed\"]` directly.",
|
||||
"README promises resolution-state tracking, but this boundary is not type-safe."
|
||||
],
|
||||
"suggestion": "Change `Scorecard.StatusByType` to `map[Status]int` (or a dedicated typed struct), update serialization adapters if needed, and update tests to assert using `StatusOpen`/`StatusFixed` constants.",
|
||||
"confidence": "high",
|
||||
"impact_scope": "subsystem",
|
||||
"fix_scope": "multi_file_refactor"
|
||||
},
|
||||
{
|
||||
"dimension": "test_strategy",
|
||||
"identifier": "brittle_private_and_copy_assertions_in_quality_tests",
|
||||
"summary": "Quality tests are tightly coupled to private helpers and exact copy text, reducing refactor safety.",
|
||||
"related_files": [
|
||||
"internal/quality/narrative_test.go",
|
||||
"internal/quality/scoring_test.go"
|
||||
],
|
||||
"evidence": [
|
||||
"`narrative_test.go` directly asserts exact strings for generated headlines/actions and tests helper internals rather than stable external behavior.",
|
||||
"`scoring_test.go` anchors on specific internal weighting outputs and literal status strings, which can fail on benign internal redesigns."
|
||||
],
|
||||
"suggestion": "Shift to contract-level tests against exported APIs with invariant assertions (phase category, presence of required fields, monotonic score behavior), and keep only a small set of snapshot/copy tests for user-facing text.",
|
||||
"confidence": "high",
|
||||
"impact_scope": "module",
|
||||
"fix_scope": "multi_file_refactor"
|
||||
},
|
||||
{
|
||||
"dimension": "test_strategy",
|
||||
"identifier": "rust_parser_missing_negative_and_boundary_cases",
|
||||
"summary": "Rust parser tests miss malformed-input and degradation-path coverage.",
|
||||
"related_files": [
|
||||
"pkg/rustdocs/parser_test.go",
|
||||
"README.md"
|
||||
],
|
||||
"evidence": [
|
||||
"`parser_test.go` cases are successful parses with valid fixture HTML and only basic assertions.",
|
||||
"No tests verify behavior for malformed HTML, missing selectors, empty documents, or unsupported result rows.",
|
||||
"README positions docs ingestion as core functionality, so parser failure behavior is a critical path."
|
||||
],
|
||||
"suggestion": "Add table-driven negative tests for malformed/partial HTML, empty search blocks, and missing headings; assert stable fallback behavior (explicit error or safe zero-value output) for each parser entrypoint.",
|
||||
"confidence": "high",
|
||||
"impact_scope": "module",
|
||||
"fix_scope": "single_edit"
|
||||
}
|
||||
]
|
||||
}
|
||||
@@ -0,0 +1,79 @@
|
||||
{
|
||||
"batch": "Design Coherence — Mechanical Concern Signals",
|
||||
"batch_index": 4,
|
||||
"assessments": {
|
||||
"design_coherence": 66.0
|
||||
},
|
||||
"dimension_notes": {
|
||||
"design_coherence": {
|
||||
"evidence": [
|
||||
"Parallel implementations of the same scorecard pipeline exist in `cmd/devour_scorecard.py` and `cmd/scorecard_generator.py` with near-identical function layouts (`ScorecardData`, `score_color`, `draw_left_panel`, `draw_right_panel`, `generate_scorecard`, `main`) and only minor line-level differences.",
|
||||
"Three variants of enhanced generator (`cmd/devour_enhanced.py`, `cmd/devour_enhanced_fixed.py`, `cmd/devour_enhanced_v2.py`) repeat almost the full rendering stack (`draw_header_section`, `draw_enhanced_left_panel`, `draw_enhanced_right_panel`, `draw_trends_section`, `load_enhanced_devour_data`), creating branch-by-copy evolution.",
|
||||
"Scraper adapters across providers (`internal/scraper/external/astrodocs.go`, `internal/scraper/external/cloudflaredocs.go`, `internal/scraper/external/reactdocs.go`) duplicate fetch/hash/change-detection and document assembly patterns with provider-specific data glued inline, indicating repeated structural pattern without shared orchestration abstraction.",
|
||||
"Within `cmd/devour_lighthouse.py`, `load_font` is defined twice (once near top and again later), showing local design drift and utility ownership ambiguity."
|
||||
],
|
||||
"impact_scope": "codebase",
|
||||
"fix_scope": "architectural_change",
|
||||
"confidence": "high"
|
||||
}
|
||||
},
|
||||
"findings": [
|
||||
{
|
||||
"dimension": "design_coherence",
|
||||
"identifier": "scorecard_variant_sprawl",
|
||||
"summary": "Scorecard generation is maintained as multiple copy-variants instead of one composable pipeline.",
|
||||
"related_files": [
|
||||
"cmd/devour_scorecard.py",
|
||||
"cmd/scorecard_generator.py",
|
||||
"cmd/devour_enhanced.py",
|
||||
"cmd/devour_enhanced_fixed.py",
|
||||
"cmd/devour_enhanced_v2.py"
|
||||
],
|
||||
"evidence": [
|
||||
"Both `cmd/devour_scorecard.py` and `cmd/scorecard_generator.py` declare the same major functions and data model in the same order with only minor stylistic deltas.",
|
||||
"Enhanced variants repeat the same section render functions and data loading flow, then diverge by ad-hoc edits, increasing change fan-out for any layout or scoring rule update."
|
||||
],
|
||||
"suggestion": "Extract a shared rendering core module (palette/fonts/layout primitives + data normalization), keep one canonical CLI entrypoint, and convert variant behavior into explicit theme/feature flags rather than duplicated files.",
|
||||
"confidence": "high",
|
||||
"impact_scope": "codebase",
|
||||
"fix_scope": "architectural_change"
|
||||
},
|
||||
{
|
||||
"dimension": "design_coherence",
|
||||
"identifier": "external_scraper_template_duplication",
|
||||
"summary": "Provider scrapers repeat the same orchestration flow with per-provider copy/paste adapters.",
|
||||
"related_files": [
|
||||
"internal/scraper/external/astrodocs.go",
|
||||
"internal/scraper/external/cloudflaredocs.go",
|
||||
"internal/scraper/external/reactdocs.go",
|
||||
"internal/scraper/external/godocs.go",
|
||||
"internal/scraper/external/vuedocs.go"
|
||||
],
|
||||
"evidence": [
|
||||
"Each scraper reimplements nearly identical `Scrape`, `DetectChanges`, `fetchPage`, and `generateHash` scaffolding, then inlines provider-specific conversion methods.",
|
||||
"The repeated constructor/client/parser wiring pattern appears across multiple files, indicating systemic pattern duplication rather than isolated differences."
|
||||
],
|
||||
"suggestion": "Introduce a shared `DocAdapter` contract and a generic `HTTPDocScraper` that owns fetch/hash/change-detect; keep provider files focused on mapping parsed domain objects to `Document`.",
|
||||
"confidence": "high",
|
||||
"impact_scope": "subsystem",
|
||||
"fix_scope": "architectural_change"
|
||||
},
|
||||
{
|
||||
"dimension": "design_coherence",
|
||||
"identifier": "utility_ownership_drift_in_lighthouse_script",
|
||||
"summary": "Duplicate utility definition in one file shows mixed responsibility boundaries.",
|
||||
"related_files": [
|
||||
"cmd/devour_lighthouse.py",
|
||||
"cmd/devour_enhanced.py"
|
||||
],
|
||||
"evidence": [
|
||||
"`cmd/devour_lighthouse.py` defines `load_font` twice with effectively the same fallback behavior, creating hidden override risk and unclear source of truth.",
|
||||
"Comparable font utility exists in other renderer scripts, reinforcing that shared utility concerns are spread instead of centralized."
|
||||
],
|
||||
"suggestion": "Remove the duplicate in `cmd/devour_lighthouse.py` and move font-loading helpers into a shared module imported by all renderer scripts.",
|
||||
"confidence": "high",
|
||||
"impact_scope": "module",
|
||||
"fix_scope": "multi_file_refactor"
|
||||
}
|
||||
]
|
||||
}
|
||||
@@ -0,0 +1,79 @@
|
||||
{
|
||||
"batch": "Cross-cutting Sweep",
|
||||
"batch_index": 5,
|
||||
"assessments": {
|
||||
"error_consistency": 71.0
|
||||
},
|
||||
"dimension_notes": {
|
||||
"error_consistency": {
|
||||
"evidence": [
|
||||
"Raw error passthrough is common in core flows (e.g., `return nil, err` in `internal/search/engine.go:114`, `internal/search/engine.go:122`, `internal/scraper/openapi.go:45`, `internal/scraper/openapi.go:50`) while nearby code wraps with operation context (e.g., `internal/search/engine.go:111`, `internal/scraper/openapi.go:153`).",
|
||||
"Failure handling style diverges between aborting, propagating, and suppressing in similar backend paths: `panic(...)` in `internal/quality/plugins/go/plugin.go:363`, warning print-and-continue in `internal/indexer/indexer.go:239`, and plain returns in `cmd/scrape.go:90`/`cmd/get.go:59`.",
|
||||
"Some call paths lose caller context at command boundaries (`cmd/scrape.go:90`, `cmd/scrape.go:125`, `cmd/get.go:59`) despite contextual wrapping being used in other command-layer branches (`cmd/scrape.go:131`, `cmd/scrape.go:145`)."
|
||||
],
|
||||
"impact_scope": "subsystem",
|
||||
"fix_scope": "multi_file_refactor",
|
||||
"confidence": "high"
|
||||
}
|
||||
},
|
||||
"findings": [
|
||||
{
|
||||
"dimension": "error_consistency",
|
||||
"identifier": "mixed_error_wrapping_in_scrape_and_search_paths",
|
||||
"summary": "Related scrape/search paths mix raw passthrough and contextual wrapping.",
|
||||
"related_files": [
|
||||
"internal/search/engine.go",
|
||||
"internal/scraper/openapi.go",
|
||||
"internal/scraper/localsearch.go",
|
||||
"cmd/scrape.go"
|
||||
],
|
||||
"evidence": [
|
||||
"`internal/search/engine.go` frequently returns raw errors (`:114`, `:117`, `:122`, `:170`) but also uses contextual errors (`:111`, `:230`).",
|
||||
"`internal/scraper/openapi.go` propagates raw errors from `readSpec`/`parseOpenAPISpec` (`:45`, `:50`, `:123`, `:141`, `:149`, `:157`, `:164`) while also defining wrapped errors (`:135`, `:153`, `:217`).",
|
||||
"`internal/scraper/localsearch.go` returns raw errors from helper boundaries (`:79`, `:164`, `:191`, `:222`) mixed with rich wrapped messages in the same workflow (`:196`, `:203`, `:209`, `:217`)."
|
||||
],
|
||||
"suggestion": "Define a package-level rule: public methods must wrap downstream errors with operation context (using `%w`), and helper internals may return raw errors. Apply this consistently to `Rebuild/EnsureIndexed`, `OpenAPIScraper.Scrape/DetectChanges/readSpec`, and `LocalSearchScraper` methods.",
|
||||
"confidence": "high",
|
||||
"impact_scope": "subsystem",
|
||||
"fix_scope": "multi_file_refactor"
|
||||
},
|
||||
{
|
||||
"dimension": "error_consistency",
|
||||
"identifier": "inconsistent_failure_channel_panic_vs_error_vs_warning",
|
||||
"summary": "Failure signaling varies between panic, error return, and warning-only logging.",
|
||||
"related_files": [
|
||||
"internal/quality/plugins/go/plugin.go",
|
||||
"internal/indexer/indexer.go",
|
||||
"cmd/scrape.go"
|
||||
],
|
||||
"evidence": [
|
||||
"`internal/quality/plugins/go/plugin.go:363` panics on plugin registration failure.",
|
||||
"`internal/indexer/indexer.go:239` prints a warning and suppresses deletion errors instead of returning them.",
|
||||
"`cmd/scrape.go` is structured around returned errors (`:131`, `:145`, `:207`) and has no panic-based handling, creating inconsistent contracts across subsystems."
|
||||
],
|
||||
"suggestion": "Standardize on explicit error returns for recoverable startup/runtime failures; replace plugin `panic` with registration error propagation or controlled process-exit at the command entrypoint, and make indexer deletion behavior explicit (either aggregate and return partial-failure errors or document/encode best-effort mode).",
|
||||
"confidence": "high",
|
||||
"impact_scope": "codebase",
|
||||
"fix_scope": "architectural_change"
|
||||
},
|
||||
{
|
||||
"dimension": "error_consistency",
|
||||
"identifier": "command_boundary_context_loss",
|
||||
"summary": "CLI command boundaries sometimes return raw errors without command context.",
|
||||
"related_files": [
|
||||
"cmd/get.go",
|
||||
"cmd/scrape.go",
|
||||
"internal/config/config.go"
|
||||
],
|
||||
"evidence": [
|
||||
"`cmd/get.go:59` and `cmd/scrape.go:90`/`:125` return raw errors directly from downstream calls.",
|
||||
"Other branches in the same command wrap with explicit context (`cmd/scrape.go:131`, `cmd/scrape.go:145`, `cmd/scrape.go:154`).",
|
||||
"Config layer already emits contextual wrapped errors (`internal/config/config.go:177`, `internal/config/config.go:181`), so command-layer inconsistency creates uneven user-facing diagnostics."
|
||||
],
|
||||
"suggestion": "At CLI entrypoints, wrap all returned downstream errors with command/action context (e.g., `run get`, `load config`, `scrape source`) and preserve root cause with `%w`; keep user-readable validation errors as direct messages.",
|
||||
"confidence": "high",
|
||||
"impact_scope": "module",
|
||||
"fix_scope": "multi_file_refactor"
|
||||
}
|
||||
]
|
||||
}
|
||||
@@ -0,0 +1,167 @@
|
||||
{
|
||||
"batch": "Full Codebase Sweep",
|
||||
"batch_index": 6,
|
||||
"assessments": {
|
||||
"cross_module_architecture": 74.0,
|
||||
"error_consistency": 68.0,
|
||||
"abstraction_fitness": 62.0,
|
||||
"test_strategy": 55.0,
|
||||
"design_coherence": 64.0
|
||||
},
|
||||
"dimension_notes": {
|
||||
"cross_module_architecture": {
|
||||
"evidence": [
|
||||
"`cmd/root.go` relies on blank import `_ \"github.com/yourorg/devour/internal/scraper/external\"` to activate runtime registration side effects.",
|
||||
"`internal/scraper/external/register.go` mutates global scraper registry in `init()` for all language scrapers.",
|
||||
"`internal/scraper/registry_simple.go` uses global mutable registry (`globalRegistry`) shared process-wide, increasing hidden coupling and order sensitivity."
|
||||
],
|
||||
"impact_scope": "subsystem",
|
||||
"fix_scope": "architectural_change",
|
||||
"confidence": "high"
|
||||
},
|
||||
"error_consistency": {
|
||||
"evidence": [
|
||||
"`cmd/root.go` and `cmd/scorecard.go` terminate with `os.Exit`, while most command flows return wrapped errors.",
|
||||
"`internal/quality/plugins/go/plugin.go` panics during plugin registration (`panic(fmt.Sprintf(...))`) instead of surfacing an error contract.",
|
||||
"`cleanup_unused.go` uses `log.Fatal` and mixed logging/exit style unlike the rest of the codebase's `fmt.Errorf(...%w...)` propagation."
|
||||
],
|
||||
"impact_scope": "codebase",
|
||||
"fix_scope": "multi_file_refactor",
|
||||
"confidence": "high"
|
||||
},
|
||||
"abstraction_fitness": {
|
||||
"evidence": [
|
||||
"Language-specific scraper implementations (`internal/scraper/external/godocs.go`, `internal/scraper/external/rustdocs.go`, `internal/scraper/external/reactdocs.go`, and peers) repeat near-identical HTTP fetch/hash/error scaffolding with only parser/document mapping differences.",
|
||||
"`internal/scraper/external/types.go` is a thin alias layer over `internal/scraper` types and does not enforce additional policy or invariants.",
|
||||
"High repeated constructor/scrape boilerplate across external scrapers indicates abstraction cost is paid repeatedly without shared leverage."
|
||||
],
|
||||
"impact_scope": "subsystem",
|
||||
"fix_scope": "architectural_change",
|
||||
"confidence": "high",
|
||||
"sub_axes": {
|
||||
"abstraction_leverage": 58.0,
|
||||
"indirection_cost": 72.0,
|
||||
"interface_honesty": 70.0
|
||||
}
|
||||
},
|
||||
"test_strategy": {
|
||||
"evidence": [
|
||||
"Critical runtime surfaces have no direct tests: `internal/server/server.go`, `pkg/client/client.go`, `internal/vector/store.go`, `internal/search/engine.go`, `internal/indexer/indexer.go`.",
|
||||
"`pkg/client/client.go` has TODO stubs returning `nil, nil` for `Query` and `Status`, but there are no tests asserting failure behavior or contract correctness.",
|
||||
"`internal/server/server.go` Start methods are TODO/no-op `nil` returns and remain unvalidated by tests, creating false-green behavior for unimplemented paths."
|
||||
],
|
||||
"impact_scope": "codebase",
|
||||
"fix_scope": "multi_file_refactor",
|
||||
"confidence": "high"
|
||||
},
|
||||
"design_coherence": {
|
||||
"evidence": [
|
||||
"`cmd/quality.go` (695 LOC) mixes CLI wiring, scan orchestration, status persistence, scoring output, resolution updates, fixer execution, and review import/export in one file.",
|
||||
"`cmd/scrape.go` (444 LOC) combines source parsing, source-type inference, profile heuristics, scrape orchestration, persistence, and source-state hashing.",
|
||||
"These large command files show recurring multi-responsibility patterns rather than cohesive command/use-case units."
|
||||
],
|
||||
"impact_scope": "module",
|
||||
"fix_scope": "multi_file_refactor",
|
||||
"confidence": "high"
|
||||
}
|
||||
},
|
||||
"findings": [
|
||||
{
|
||||
"dimension": "cross_module_architecture",
|
||||
"identifier": "init_side_effect_registration_coupling",
|
||||
"summary": "Scraper registration depends on import-time side effects and global mutable registry state.",
|
||||
"related_files": [
|
||||
"cmd/root.go",
|
||||
"internal/scraper/external/register.go",
|
||||
"internal/scraper/registry_simple.go"
|
||||
],
|
||||
"evidence": [
|
||||
"Blank import in root command triggers registration implicitly rather than explicit bootstrap wiring.",
|
||||
"Registration happens in `init()` and mutates shared global registry."
|
||||
],
|
||||
"suggestion": "Replace import-time registration with explicit bootstrap registration (e.g., `RegisterExternalScrapers()` called from startup), and pass registry instances through constructors to remove hidden global coupling.",
|
||||
"confidence": "high",
|
||||
"impact_scope": "subsystem",
|
||||
"fix_scope": "architectural_change"
|
||||
},
|
||||
{
|
||||
"dimension": "error_consistency",
|
||||
"identifier": "mixed_process_termination_and_error_propagation",
|
||||
"summary": "Error handling mixes panic/log.Fatal/os.Exit with returned errors across adjacent layers.",
|
||||
"related_files": [
|
||||
"cmd/root.go",
|
||||
"cmd/scorecard.go",
|
||||
"internal/quality/plugins/go/plugin.go",
|
||||
"cleanup_unused.go"
|
||||
],
|
||||
"evidence": [
|
||||
"`Execute()` exits process directly; scorecard helper exits inside utility flow; plugin registration panics on failure.",
|
||||
"Most other command paths return wrapped errors, creating inconsistent failure semantics."
|
||||
],
|
||||
"suggestion": "Standardize on returning errors from library/command internals and only perform process exit in one top-level entrypoint; replace panic/log.Fatal in shared code with typed/wrapped errors.",
|
||||
"confidence": "high",
|
||||
"impact_scope": "codebase",
|
||||
"fix_scope": "multi_file_refactor"
|
||||
},
|
||||
{
|
||||
"dimension": "abstraction_fitness",
|
||||
"identifier": "external_scraper_boilerplate_without_shared_base",
|
||||
"summary": "External scraper implementations duplicate fetch/hash/error/document plumbing instead of sharing a base abstraction.",
|
||||
"related_files": [
|
||||
"internal/scraper/external/godocs.go",
|
||||
"internal/scraper/external/rustdocs.go",
|
||||
"internal/scraper/external/reactdocs.go",
|
||||
"internal/scraper/external/nuxtdocs.go",
|
||||
"internal/scraper/external/cloudflaredocs.go",
|
||||
"internal/scraper/external/types.go"
|
||||
],
|
||||
"evidence": [
|
||||
"Each scraper repeats `fetchPage`, status code checks, hash generation, and near-identical scrape control flow.",
|
||||
"Alias-only types file adds indirection without behavior."
|
||||
],
|
||||
"suggestion": "Introduce a shared external-scraper base/helper for HTTP fetch, retries, hashing, and common error mapping; keep only parser-specific extraction and document shaping in each language scraper.",
|
||||
"confidence": "high",
|
||||
"impact_scope": "subsystem",
|
||||
"fix_scope": "architectural_change"
|
||||
},
|
||||
{
|
||||
"dimension": "test_strategy",
|
||||
"identifier": "untested_unimplemented_runtime_paths",
|
||||
"summary": "Core runtime paths are both under-tested and partially stubbed, leaving high-risk behavior unvalidated.",
|
||||
"related_files": [
|
||||
"internal/server/server.go",
|
||||
"pkg/client/client.go",
|
||||
"internal/vector/store.go",
|
||||
"internal/search/engine.go",
|
||||
"internal/indexer/indexer.go"
|
||||
],
|
||||
"evidence": [
|
||||
"Server/client/store contain TODO or not-implemented branches without direct tests.",
|
||||
"No direct test files exist for several core modules that govern querying, indexing, and serving."
|
||||
],
|
||||
"suggestion": "Add table-driven tests for client/server/store/indexer contracts first (error behavior and non-nil results), then implement missing paths behind those tests; prioritize integration tests that exercise scrape->index->query flow.",
|
||||
"confidence": "high",
|
||||
"impact_scope": "codebase",
|
||||
"fix_scope": "multi_file_refactor"
|
||||
},
|
||||
{
|
||||
"dimension": "design_coherence",
|
||||
"identifier": "command_files_mix_multiple_responsibilities",
|
||||
"summary": "Large CLI command files blend orchestration, domain logic, persistence, and formatting concerns.",
|
||||
"related_files": [
|
||||
"cmd/quality.go",
|
||||
"cmd/scrape.go",
|
||||
"cmd/ask.go"
|
||||
],
|
||||
"evidence": [
|
||||
"`cmd/quality.go` combines scan setup, scoring/status persistence, resolve/fix/review workflows.",
|
||||
"`cmd/scrape.go` combines config parsing, source detection/profiling, scrape execution, indexing, and source-state updates.",
|
||||
"`cmd/ask.go` includes query derivation, source URL heuristics, ranking, summarization, and output formatting in one command module."
|
||||
],
|
||||
"suggestion": "Split command files into focused packages (transport/CLI binding vs service layer vs persistence helpers) and keep Cobra handlers as thin adapters invoking composable use-case functions.",
|
||||
"confidence": "high",
|
||||
"impact_scope": "module",
|
||||
"fix_scope": "multi_file_refactor"
|
||||
}
|
||||
]
|
||||
}
|
||||
Reference in New Issue
Block a user