PR Review: feat: component attribution to telemetry (#78)

Overall this is a solid, well-scoped feature with good test coverage and a thoughtful backward-compatible migration approach. A few issues worth addressing before merge.

Issues

1. schema.sql not updated — minor correctness issue

schema.sql still defines the old table without the two new columns. New installations will create the schema via CREATE TABLE IF NOT EXISTS, then immediately run migrateComponentColumns() to add the columns via ALTER TABLE. The migration was designed for existing databases — for fresh installs the columns should be defined in the base DDL.

-- schema.sql should have these two lines added:
components_used TEXT,
component_errors TEXT

This avoids the migration running on every fresh install and keeps schema.sql as the canonical source of truth for the table structure.

2. SQL injection pattern in migrateComponentColumns() (sqlite.go:56)

_, err = s.db.Exec(fmt.Sprintf(`ALTER TABLE generation_events ADD COLUMN %s TEXT`, col))

The column name is string-formatted directly into a DDL statement. The values are currently hardcoded so there's no actual injection risk, but this pattern is fragile — if someone extends the slice with a value from user input, it becomes exploitable. SQLite doesn't support parameterized DDL, so consider at least adding a safety guard:

// Validate col is an expected identifier before using in DDL
allowed := map[string]bool{"components_used": true, "component_errors": true}
if !allowed[col] {
    return fmt.Errorf("unexpected column name %q", col)
}

3. KnownComponents is exported but never used (attribution.go:9)

KnownComponents is declared as a package-level exported var but doesn't appear in any logic path — AttributeErrors and matchComponent only operate on componentsUsed (the dynamically-detected list), not on KnownComponents. If this is intended as a reference for callers (e.g., for validation or documentation), add a comment explaining its purpose. If it's unused, remove it.

4. toFieldData silently drops fields (commands/gen.go:684)

parser.Field has 10 fields; toFieldData copies only 4 (Name, GoType, SQLType, IsSelect). Missing: IsReference, ReferencedTable, OnDelete, IsTextarea, SelectOptions.

Today this is fine — DetectUsedComponents only checks f.IsSelect. But if component detection is extended (e.g., to detect datepicker from a date field), the incomplete conversion will silently produce wrong results with no compile error. Consider copying the full struct or adding a comment noting which fields are intentionally omitted and why.

5. ComponentsFromUsage includes non-"Use"-prefixed bool fields (attribution.go:106-111)

name := field.Name
if strings.HasPrefix(name, "Use") {
    name = name[3:]
}
components = append(components, strings.ToLower(name))

If a bool field doesn't have the Use prefix, the field name itself is lowercased and added. For example, Modal bool would produce "modal". Today ComponentUsage only has Use-prefixed fields so this doesn't matter, but the behavior is inconsistent with the documented contract ("bool fields prefixed 'Use'"). Consider skipping non-Use-prefixed fields:

if !strings.HasPrefix(name, "Use") {
    continue
}

6. Alignment change in events.go is inconsistent

The diff reformatted DurationMs and FilesGenerated with extra trailing spaces to align with the longer new field names, but the fields above (ID, Command, Errors, etc.) are left unaligned. Either align all fields in the struct or leave existing fields untouched — the mixed approach is harder to read than either extreme.

Minor notes

• Double AttributeComponentErrors() calls in gen.go (lines ~229 and ~244): These are on mutually exclusive code paths (failure vs. success), so it's correct. A brief comment like // attribute errors regardless of success/failure path would clarify intent.

• Save() marshals nil slices as "null": Consistent with existing fields, and the scan correctly handles it. Just noting that json.Marshal(nil) → "null" stored as a string, not SQL NULL — this is fine but worth being aware of when querying the DB directly.

• matchComponent first-match semantics: An error that matches multiple component names returns only the first. This seems intentional but isn't documented.

Test coverage

Tests are comprehensive — file path matching, message matching, case-insensitivity, empty inputs, pointer handling, round-trip persistence, and backward compat. The only gap is a test for ComponentsFromUsage with a bool field that lacks the Use prefix (to document/enforce the expected behavior).

Summary

The most impactful fix is updating schema.sql to include the new columns so fresh installs don't go through the migration path unnecessarily.

PR Review: Component Attribution to Telemetry (#78)

Overall this is a solid, well-tested feature addition. The backward-compatible migration, consistent noop pattern, and thorough test coverage are all strong. A few issues worth addressing before merge.

Issues

1. False-positive risk in matchComponent — message matching

File: internal/telemetry/attribution.go:55

The check strings.Contains(msgLower, compLower+".") can match when a component name is a prefix of a longer token. For example: if "toast" is a tracked component and the error is "toaster.New: invalid config", then strings.Contains(msg, "toast.") is true because "toast." is a substring of "toaster.".

Suggestion: Add a test documenting this behavior explicitly (or fix with a word-boundary check). Example fix:

func isWordBoundary(s string, end int) bool {
    return end >= len(s) || (!unicode.IsLetter(rune(s[end])) && !unicode.IsDigit(rune(s[end])))
}

Also note: strings.HasPrefix(msgLower, compLower+" ") only catches errors whose first word is the component name. Errors like "failed to initialize toast: nil pointer" fall through all three message patterns. This asymmetry between file-path and message matching is worth documenting in a comment.

2. Reflection-based ComponentsFromUsage creates implicit coupling

File: internal/telemetry/attribution.go:68

Using reflection on the "Use" prefix convention works today but silently breaks if ComponentUsage adds fields with a different naming scheme — there is no compile-time enforcement.

Since commands/gen.go already imports both packages, a simpler alternative is a ToSlice() []string method on generator.ComponentUsage:

// generator/components.go
func (u ComponentUsage) ToSlice() []string {
    var out []string
    if u.UseModal    { out = append(out, "modal") }
    if u.UseToast    { out = append(out, "toast") }
    if u.UseDropdown { out = append(out, "dropdown") }
    return out
}

Then in gen.go: capture.RecordComponentsUsed(compUsage.ToSlice()) and ComponentsFromUsage in the telemetry package can be removed. Adding a new component to ComponentUsage then requires explicitly updating ToSlice(), making omissions impossible.

If you prefer to keep the reflection approach (reasonable for keeping telemetry independent of generator internals), add a test verifying that bool fields without a Use prefix are correctly ignored.

3. Minor: alignment changes in events.go add diff noise

File: internal/telemetry/events.go:16-21

The PR re-aligns DurationMs and FilesGenerated to match the new wider field names, but leaves Errors, ValidationJSON, etc. unaligned. The result is mixed alignment. Consider either leaving existing alignment unchanged, or aligning all fields uniformly.

Minor Notes

• Two-path AttributeComponentErrors() placement: Correct — one call in the early-return error branch, one on the success/validation path, no double-attribution possible. The comment // attribute errors on success/validation path is slightly misleading when there are no errors at all; something like // attribute any captured errors to components before completing would be clearer.

• Migration guard in migrateComponentColumns: The allowed map validates literal column names — technically unnecessary since there is no user input. That said, it is a reasonable defensive pattern for DDL code and the comment explains the intent well.

Test Coverage

The test suite is thorough:

• File-path matching, message matching, no-match, empty inputs, case-insensitivity, and field preservation all covered
• Round-trip integration test (TestCollector_ComponentDataRoundTrip) is solid
• Backward-compatibility test (TestCollector_EventWithoutComponentData) covers the important existing-DB scenario

Suggested additions:

• TestComponentsFromUsage with a struct containing non-Use-prefixed bool fields — verifies the ignore behavior and documents the contract
• TestAttributeErrors with a component name that is a prefix of another word in the error (e.g., "toast" vs "toaster.New") — documents current behavior or motivates a fix

Summary

Items 1 and 2 are the most worth addressing before merge. Everything else is minor.

PR Review: Component Attribution to Telemetry

Good feature overall: solid test coverage, clean backward-compatible migration, and consistent noop handling.

Issue: matchComponent message-matching false positive

File: internal/telemetry/attribution.go:59

The strings.Contains(msgLower, compLower+".") check can produce false positives when a component name appears as a suffix of a longer identifier. For example, if "modal" is a tracked component and an error mentions "premodal.init: failed", this matches because "modal." is a substring of "premodal.".

With realistic component names (modal, toast, dropdown) this is unlikely in practice, but a test documenting the edge case — either accepting it or motivating a word-boundary fix — would strengthen the contract.

Minor: nil slices stored as "null" TEXT, not SQL NULL

json.Marshal(nil) produces the string "null" stored as TEXT, consistent with other nullable fields here. scanInto handles it correctly. But direct DB queries using WHERE components_used IS NULL will not find events without component data — the check needs WHERE components_used IS NULL OR components_used = 'null'. A brief comment near the marshal calls would help future readers.

Minor: migrateComponentColumns runs on every open

Since schema.sql now includes both columns, fresh installs get them from CREATE TABLE IF NOT EXISTS. But migrateComponentColumns() still runs on every OpenSQLite call and issues two pragma_table_info queries. Fast and harmless, just logically redundant for new installs — no action needed.

Corrections to an earlier review comment on this PR

One previous review contains inaccuracies worth noting:

• schema.sql IS updated with both columns. The claim they were missing is incorrect.
• The allowed map IS already present in migrateComponentColumns before fmt.Sprintf. The suggestion to add it was already implemented.
• ComponentsFromUsage correctly ignores non-Use fields via if !strings.HasPrefix(name, "Use") { continue } on line 101. The claim that non-prefixed bool fields get included is incorrect.

What looks good

• Migration handles repeated calls safely via pragma_table_info existence check
• Noop path consistently honored in both new methods
• Two AttributeComponentErrors() call sites are on mutually exclusive paths — correct as written
• Test coverage is thorough: file/message matching, case-insensitivity, pointer structs, round-trip persistence, backward compat
• toFieldData intentional incompleteness is clearly documented in the code comment

The message-matching false positive is the only real item worth addressing before merge.

Code Review: feat: component attribution to telemetry (#78)

Overall this is a well-structured addition. The separation of concerns is clean (attribution logic isolated to attribution.go), backward compatibility is handled correctly, and the test coverage is solid. A few observations below.

Observations and Suggestions

1. allowed map in migrateComponentColumns — misleading comment

The column names come from a hardcoded literal slice defined in the same function body, so !allowed[col] is unreachable dead code at runtime. The actual DDL injection protection comes from the hardcoded literals, not the runtime check. The map does serve as a useful guard against future programmer error (extending the loop without updating the allowlist), but the comment should say so:

// Allowed columns — guards against future additions to the loop without
// a corresponding allowlist entry. ALTER TABLE cannot use parameters.

2. AttributeComponentErrors() silently overwrites on repeated calls

It is currently called twice in GenResource — once on the failure path and once unconditionally before Complete. This is idempotent since cap.event.Errors does not change between the two calls, but a reader has to trace through the code to confirm that. A short doc note — "safe to call multiple times; last call wins" — would remove the ambiguity.

3. Message-based matching has a false-positive risk for short or common component names

strings.Contains(msgLower, compLower+".")

For current component names (modal, toast, dropdown) this is fine. But a component named "form" would match errors containing "malformed." or "platform.". For telemetry, false-negatives (missed attribution) are preferable to false-positives (wrong attribution). Worth documenting this assumption, or adding a word-boundary guard requiring the component name to be preceded by /, ., :, space, or start of string.

4. No direct test for the upgrade/migration path

The round-trip tests exercise migration indirectly, but there is no test that:

1. Creates a DB without the new columns
2. Calls OpenSQLite / migrateComponentColumns
3. Verifies the columns are added and existing rows survive

This is the most likely failure scenario in the field (user upgrading from an older install). A dedicated test would be valuable.

5. toFieldData comment is slightly inaccurate

The comment says "Only fields needed by DetectUsedComponents are mapped" but Name, GoType, and SQLType are also populated. Either map only IsSelect (minimal) or update the comment to say the extra fields are included defensively in case DetectUsedComponents expands. Minor either way.

What is done well

• Backward compatibility: migrateComponentColumns with pragma_table_info is the right pattern for SQLite — clean and idiomatic.
• Test coverage: Broad — file-path matching, message matching, case insensitivity, no-match, empty inputs, pointer structs, non-Use prefix fields, phase/message preservation, round-trips, and backward compat. The false-positive prevention test (TestAttributeErrors_PrefixNoFalsePositive) is a nice touch.
• Reflection usage: ComponentsFromUsage via reflect is justified (telemetry path, called once, decouples attribution from a concrete struct type). All nil/pointer/non-struct guards are present.
• Attribution timing: Detecting components before generation and attributing after error accumulation is the correct ordering.
• noop guards: Both new Capture methods respect the noop path — telemetry cannot affect generation behaviour regardless of what happens inside.

Summary

No blocking issues. Main asks: clarify the allowed map comment, add a dedicated migration upgrade test, and document AttributeComponentErrors idempotency. Good to merge after addressing those.

PR Review: feat: component attribution to telemetry (#78)

Note: This review corrects some inaccuracies from earlier comments on this PR and focuses on what's genuinely outstanding.

What's actually correct (correcting prior review inaccuracies)

Several earlier comments raised issues that don't exist in the diff:

• schema.sql IS updated — both components_used TEXT and component_errors TEXT columns are present in the diff.
• The allowed map IS already present in migrateComponentColumns before the fmt.Sprintf call — the SQL injection guard is implemented.
• ComponentsFromUsage correctly ignores non-Use-prefixed fields via if !strings.HasPrefix(name, "Use") { continue } (attribution.go line ~101).
• TestSQLiteStore_MigrationUpgrade IS present — there is a dedicated test that creates an old schema, inserts a row, opens with OpenSQLite, and verifies existing rows survive with nil component fields.
• AttributeComponentErrors IS documented as "Safe to call multiple times; last call wins" in its godoc.

One genuine issue: prefix false positive in message matching

File: internal/telemetry/attribution.go:59

The check strings.Contains(msgLower, compLower+".") can false-positive when a component name appears as a suffix of a longer token (the reverse of the toaster test).

For example, if "form" were a component, the error "malformed.init: failed" would match because "form." is a substring of "malformed.". With current components (modal, toast, dropdown) this is unlikely in practice, but the contract is only half-tested: TestAttributeErrors_PrefixNoFalsePositive covers the suffix case (toaster vs toast) but not the prefix case (premodal vs modal).

Suggested test (or a comment documenting the known limitation):

func TestAttributeErrors_SuffixNoFalsePositive(t *testing.T) {
    // "modal" must not match "premodal.init" (component is a suffix of a longer token)
    errors := []GenerationError{{Phase: "runtime", Message: "premodal.init: failed"}}
    result := AttributeErrors(errors, []string{"modal"})
    if len(result) != 0 {
        t.Fatalf("expected 0, got %d", len(result))
    }
}

If that test fails, a word-boundary guard fixes it:

re := regexp.MustCompile(`(?:^|[^a-zA-Z0-9])` + regexp.QuoteMeta(compLower) + `\.`)
if re.MatchString(msgLower) { return comp }

Minor: nil slices stored as "null" TEXT, not SQL NULL

json.Marshal(nil) produces "null" stored as TEXT (consistent with existing fields). scanInto handles it correctly. Worth a one-line comment near the marshal calls for future DB query authors:

// json.Marshal(nil) → "null" as TEXT, not SQL NULL.
// Direct queries need: WHERE components_used IS NULL OR components_used = 'null'

What looks good

• Migration: pragma_table_info check before ALTER TABLE is idiomatic and correctly re-entrant.
• Ordering: components detected before generation, errors attributed after accumulation — correct pipeline sequencing.
• Noop path: both RecordComponentsUsed and AttributeComponentErrors return early on noop — telemetry cannot affect generation behavior.
• Two-path attribution: the two AttributeComponentErrors() calls are on mutually exclusive branches — no double-attribution possible.
• Reflection approach: appropriate for keeping telemetry decoupled from generator.ComponentUsage. All nil/pointer/non-struct guards are present.
• Test coverage: comprehensive — file/message matching, case-insensitivity, empty inputs, pointer structs, non-Use prefix fields, round-trip persistence, backward compat, phase/message preservation.

Summary

No blocking issues. Good to merge after adding the prefix false-positive test or a comment documenting the known limitation.

PR Review: feat: component attribution to telemetry (#78)

Note: A previous review comment on this PR contains several factual errors (claims schema.sql is not updated, suggests adding an allowed map guard that is already present, and suggests adding a non-Use prefix skip that is already present). This review is based on the actual diff.

Summary

Well-scoped feature with solid test coverage. The backward-compat migration is handled correctly. A few things worth addressing:

Issues

1. toFieldData silently under-maps fields (commands/gen.go)

The comment explains the intent well. But if DetectUsedComponents is extended to detect additional components (e.g., a textarea from IsTextarea), callers will not get a compile error — toFieldData silently passes zero values and component detection misses them.

Consider either copying the full struct, or adding a test that fails if DetectUsedComponents reads a field not covered by toFieldData. This creates a compile-time-equivalent safety net for future extension.

2. matchComponent false positive is enshrined rather than guarded

TestAttributeErrors_SuffixFalsePositive documents and asserts that "premodal.init: failed" matches the "modal" component. The comment says "If it becomes a problem, add a word-boundary guard."

For telemetry data quality, the guard is worth adding now before data accumulates with incorrect attributions. A simple fix: before matching compLower+".", verify the character immediately before the match position is not a letter/digit/underscore. This would not change any other matching behavior.

3. Duplicate AttributeComponentErrors() calls — potential footgun

gen.go calls AttributeComponentErrors() on two mutually exclusive paths (failure and success). The logic is correct today, but it is a footgun: if RecordError is added after an AttributeComponentErrors() call during future edits, that error silently goes unattributed.

Moving AttributeComponentErrors() into Complete(), or calling it once on the shared path just before both Complete() calls, would be safer and remove the duplication.

4. RecordComponentsUsed naming is inconsistent with its replace semantics

The Record* family (RecordError, RecordFileGenerated) accumulates. RecordComponentsUsed replaces on repeated calls. Renaming it SetComponentsUsed would signal the different semantics and prevent a subtle bug if the call site is ever extended.

Minor notes

• allowed map in migrateComponentColumns: The check is logically redundant because the hardcoded slice always matches the map, but it is good defensive practice. No action needed — this is correct.
• json.Marshal(nil) stores "null" string, not SQL NULL: Well-commented and consistently handled in scanInto. Just ensure direct SQL queries on this table use WHERE col IS NULL OR col = 'null' as the comment says.
• schema.sql is correctly updated: Fresh installs create the columns from the DDL; migrateComponentColumns then no-ops. Old installs get them via ALTER TABLE. Both paths are correct.

Test coverage

Comprehensive: file path matching, message matching, case-insensitivity, empty inputs, pointer types, full round-trip, migration upgrade, and backward compat with old databases. The TestAttributeErrors_SuffixFalsePositive test could be inverted to assert rejection of the false positive once a word-boundary guard is added.

Overall this is clean and well-structured. Main asks: the toFieldData extensibility risk (#1) is the most important to address. The AttributeComponentErrors call placement (#3) and naming (#4) are quick wins.

PR Review: feat: component attribution to telemetry

Overall this is a well-structured addition with good test coverage, a backward-compatible migration, and clear comments. A few issues are worth addressing before merging.

[HIGH] auth.go silently swallows a real error

The diff changes a hard error into a printed warning:

// Before
return fmt.Errorf("failed to update dependencies: %w\n%s", err, output)
// After
fmt.Fprintf(os.Stderr, "Warning: could not fetch some dependencies...\n%s\n", output)

This is a significant, unannounced behavior change. If go get fails after auth generation, the code previously aborted. Now it silently continues, potentially leaving the generated auth code in a broken state that only surfaces when the user tries to build. If intentional (e.g. for offline/CI environments), it should be called out in the PR description.

[MEDIUM] Known false positive in matchComponent is documented but not fixed

TestAttributeErrors_SuffixFalsePositive asserts that "premodal.init" does match component "modal" and labels it a "known false positive". Testing known-wrong behavior to document it is understandable, but this means real telemetry data will silently misattribute errors when a component name appears as a suffix of a longer token. A left-boundary check (ensure the character before the match is non-alphanumeric or start-of-string) would fix it with minimal risk. Better to fix now before misattributed data accumulates.

[MEDIUM] migrateComponentColumns allowlist check is unreachable dead code

allowed := map[string]bool{"components_used": true, "component_errors": true}
for _, col := range []string{"components_used", "component_errors"} {
    if !allowed[col] { // can never be true
        return fmt.Errorf("unexpected column name %q", col)
    }

col only takes values from the same literal set as the allowlist keys, so the guard is always false. The comment says it protects against "future additions to the loop without a corresponding allowlist entry", but a developer adding to the slice and forgetting the map still compiles and silently skips the check. Consider a switch with a default: panic(...) for a real compile-visible guard, or just drop the check and comment that the names are pre-vetted string literals.

[LOW] ComponentsFromUsage uses reflection where a type method would be simpler

ComponentUsage is an internal project type. A func (c ComponentUsage) Components() []string method would be clearer, testable without the any interface, and provide compile-time safety. The reflection approach silently ignores non-bool fields and non-Use-prefixed fields, which can surprise future maintainers if the struct evolves. If the decoupling from the generator package is the goal, consider having DetectUsedComponents return []string directly to skip the struct-to-slice reflection hop.

[GOOD] JSON null vs SQL NULL handling

The comment in Save explaining that json.Marshal(nil) produces the string "null" (not SQL NULL) and the advice for direct queries is clear. The sql.NullString use in scanInto correctly distinguishes old-schema SQL NULL from "null" TEXT. Well handled.

[MINOR] AttributeComponentErrors called on every exit path manually

Both call sites in GenResource are correct. A future improvement: fold the call into Complete itself so callers cannot forget it on new exit paths.

[GOOD] Test quality

The test suite is thorough — edge cases (nil, empty, pointer receivers, case-insensitivity), round-trip integration, and TestSQLiteStore_MigrationUpgrade are all solid. The migration upgrade test in particular gives strong confidence in the backward-compatibility story.

The auth.go behavior change should be addressed before merge. The others can be tracked as follow-ups.

PR Review: feat: component attribution to telemetry (#78)

Several reviews have already been left on this PR, some of which contain inaccuracies (e.g., claiming schema.sql is missing columns, the allowed map is absent, or ComponentsFromUsage does not skip non-Use fields — all of these are correctly implemented in the diff). This review focuses on what is genuinely worth addressing.

Issues

1. Known false positive is asserted, not fixed (attribution.go:59, attribution_test.go:428)

TestAttributeErrors_SuffixFalsePositive documents and asserts that "premodal.init: failed" matches the "modal" component, labelling it a "known false positive". Testing known-wrong behavior is a red flag for a telemetry store: misattributed errors will silently accumulate in production data, making component health metrics unreliable. A simple left-boundary guard fixes this without breaking any legitimate matches:

// In matchComponent, replace:
strings.Contains(msgLower, compLower+".")
// With a positional check that the match is not preceded by a word character:
idx := strings.Index(msgLower, compLower+".")
idx != -1 && (idx == 0 || !isWordChar(msgLower[idx-1]))

Current components (modal, toast, dropdown) make the false positive unlikely in practice, but the guard prevents the test from needing to assert incorrect behavior.

2. allowed map in migrateComponentColumns is structurally dead code (sqlite.go:551)

allowed := map[string]bool{"components_used": true, "component_errors": true}
for _, col := range []string{"components_used", "component_errors"} {
    if !allowed[col] { // can never be true — same literals drive both

col only takes values from the same literal set as the allowlist keys, so the guard can never trigger. The comment says it protects against a developer extending the loop without updating the map, but that scenario still compiles and silently passes — the developer just adds to both simultaneously. Either remove the check (the hardcoded literals are already the protection), or invert the pattern so the map drives the iteration and any omission becomes a genuine runtime guard:

// Single source of truth drives iteration; guard is now reachable
cols := map[string]bool{"components_used": true, "component_errors": true}
for col := range cols { ... }

Note: map iteration is non-deterministic in Go, so if order matters for the pragma_table_info queries, keep a separate ordered slice and validate each element against the map.

3. auth.go behavior change is absent from the PR description (auth.go:42)

The diff changes a hard return fmt.Errorf(...) into a printed warning and continues. The commit message explains the rationale (CI, offline environments, workspace sub-modules) but the PR description says nothing about this change. Users upgrading will see generation appear to succeed while dependencies are silently unfetched, with errors only surfacing at build time. If intentional, it should be called out in the PR description, and the warning message could be clearer about the remediation:

fmt.Fprintf(os.Stderr, "Warning: could not fetch dependencies — run 'go mod tidy' in %s to resolve:\n%s\n", projectRoot, output)

4. RecordComponentsUsed naming conflicts with Record* accumulation convention (telemetry.go:664)

RecordError and RecordFileGenerated append. RecordComponentsUsed replaces. A caller who calls it twice expecting accumulation silently loses the first call. Renaming it SetComponentsUsed makes the replace semantics unambiguous at the call site.

5. AttributeComponentErrors() on separate exit paths is a future footgun (gen.go:229, 244)

Both call sites are correct today (mutually exclusive code paths). But any new exit path added to GenResource must remember to call AttributeComponentErrors() before Complete() or attribution silently goes missing. Moving the call inside Complete() eliminates that requirement entirely:

func (cap *Capture) Complete(success bool, validationJSON string) {
    cap.AttributeComponentErrors() // attribute before persisting, regardless of exit path
    // ...existing persist logic
}

What looks good

• FieldDataFromFields refactoring (last commit): Cleanly removes ~30 lines of duplicated struct conversion from resource.go and schema.go. Good cleanup.
• Migration correctness: pragma_table_info check before ALTER TABLE is idiomatic SQLite — handles repeated opens, fresh installs, and upgrades without error.
• TestSQLiteStore_MigrationUpgrade: Creates an old-schema DB, inserts a row, opens with OpenSQLite, and verifies both backward compat and forward writes. Exactly the right test.
• Noop guards: Both new Capture methods return early on noop — telemetry cannot affect generation behavior.
• json.Marshal(nil) comment: Correctly documents that nil slices store as "null" TEXT, not SQL NULL, with advice for direct DB queries.
• Test breadth: File/message matching, case-insensitivity, pointer structs, non-Use prefix fields, round-trip persistence, backward compat — comprehensive.

Summary

Items 1 and 3 from the PR description are the most worth addressing before merge.

PR Review: feat/component-attribution-telemetry - Summary of findings after reviewing all commits and the full diff.

RESOLVED from prior reviews: schema.sql now has the new columns in base DDL; componentColumns var replaces the unreachable allowed-map guard; KnownComponents removed; FieldDataFromFields extracts the duplicated conversion; isWordChar boundary guard fixes the premodal false-positive.

ITEM 1 (medium) - auth.go warning omits location: The non-fatal go-get change is correct in intent but the Fprintf warning does not include projectRoot so users do not know where to run go mod tidy. Add projectRoot to the format string. Also missing from PR description - worth one line in the summary since it is a user-visible behavior change.

ITEM 2 (low) - RecordComponentsUsed should be SetComponentsUsed: RecordError and RecordFileGenerated append; RecordComponentsUsed replaces. The Record prefix implies accumulation. Rename to SetComponentsUsed to make the overwrite semantics obvious at call sites.

ITEM 3 (low) - AttributeComponentErrors at each exit path is a footgun: Both call sites in gen.go are correct today but any new early-return in GenResource will silently skip attribution. Move the attribution call inside Complete so it is unconditional. AttributeComponentErrors can remain public for tests that inspect attribution mid-flight.

DESIGN NOTE (no action): DetectUsedComponents runs before GenerateResource so ComponentsUsed is predictive, not observational. Acceptable for v1 but worth noting for future generator changes.

WORKING WELL: pragma_table_info migration pattern, isWordChar boundary guard, comprehensive test suite (TestSQLiteStore_MigrationUpgrade is particularly solid), noop guards on both new Capture methods, null-TEXT storage comment, and FieldDataFromFields deduplication.

Overall the feature is correct and well-tested. Items 1 and 3 are the highest priority before merge.

PR Review: feat: component attribution to telemetry (#78)

Note: Several prior reviews on this PR contain inaccuracies. This review is based on the actual diff.

What is correctly implemented (prior reviews were wrong about these):

• schema.sql IS updated with both new columns in the base DDL
• ComponentsFromUsage correctly skips non-Use-prefixed bool fields via continue
• matchComponent DOES have a left-boundary word guard (isWordChar) -- the premodal false positive IS prevented, and TestAttributeErrors_SuffixNoFalsePositive correctly asserts 0 results
• TestSQLiteStore_MigrationUpgrade IS present and tests the upgrade path end-to-end
• FieldDataFromFields copies all 9 fields -- no silent field dropping

Genuine issues

[MEDIUM] auth.go behavior change is absent from PR description

The diff changes a hard error into a printed warning in auth.go:

Before: return fmt.Errorf("failed to update dependencies: %w\n%s", err, output)
After: fmt.Fprintf(os.Stderr, "Warning: could not fetch some dependencies...")

This is a user-visible behavior change: auth generation now appears to succeed even when go get fails, with the breakage only surfacing at build time. If intentional (CI, offline, workspace sub-modules), it should be called out explicitly in the PR description.

[LOW] RecordComponentsUsed naming conflicts with the Record* accumulation convention

RecordError and RecordFileGenerated append. RecordComponentsUsed replaces on repeated calls. A caller who calls it twice expecting accumulation silently loses the first call. Renaming to SetComponentsUsed would make the overwrite semantics unambiguous at call sites.

[LOW] AttributeComponentErrors() placement is a future footgun

Both call sites in gen.go are on mutually exclusive paths -- correct today. But any new early-return added to GenResource must remember to call AttributeComponentErrors() before Complete() or attribution silently goes missing. Moving the attribution call inside Complete() would eliminate this requirement.

What looks good

• Migration: pragma_table_info check before ALTER TABLE is idiomatic SQLite -- re-entrant, handles fresh installs and upgrades
• TestSQLiteStore_MigrationUpgrade: Creates old-schema DB, inserts a row, opens with OpenSQLite, verifies backward compat and forward writes -- exactly the right test
• Noop guards: Both new Capture methods return early on noop -- telemetry cannot affect generation behavior
• isWordChar boundary guard: Correctly prevents prefix/suffix false positives in message matching
• FieldDataFromFields refactor: Cleanly removes ~28 lines of duplicated struct conversion from resource.go and schema.go
• json.Marshal(nil) comment: Correctly documents that nil slices store as 'null' TEXT and advises on direct DB queries
• Test breadth: File/message matching, case-insensitivity, empty inputs, pointer structs, non-Use prefix fields, round-trip persistence, backward compat, phase/message preservation -- comprehensive

Summary

The auth.go item is the most important to address before merge. The others are clean-up worth doing but not blocking.