Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Linear integration is disabled

You can enable these sources in your CodeRabbit configuration.

Run ID: 18f715bb-d5c0-468a-9b4a-26657913dd44

pkg/provisioner/source/provision_hook.go (1)

348-351: Include parse-error context in the invalid TTL message.

Line 350 surfaces the invalid TTL, but adding the parse error (and a short format hint) would make troubleshooting faster.

Proposed tweak

-		return true, fmt.Sprintf("Invalid source TTL %q; forcing re-provision to avoid stale cache", ttl)
+		return true, fmt.Sprintf(
+			"Invalid source TTL %q (%v); forcing re-provision to avoid stale cache. Use formats like \"1h\", \"30m\", or \"0s\"",
+			ttl,
+			err,
+		)

As per coding guidelines, "Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/provisioner/source/provision_hook.go` around lines 348 - 351, The invalid
TTL branch should include the parse error and a short format hint in the
returned message: when duration.ParseDuration(ttl) fails (see ttlDuration and
duration.ParseDuration(ttl)), return a message that embeds err.Error() and a
brief example/hint such as "expected formats like '30s' or '5m'" so callers can
quickly diagnose and fix the TTL value.

pkg/provisioner/source/provision_hook.go (1)

346-369: Normalize TTL input before zero-value checks and parsing.

Right now zero TTL detection is exact-match only. Trimming/lowercasing first avoids accidental “invalid TTL” behavior for harmless formatting differences.

🛠️ Suggested adjustment

 func isSourceCacheExpired(ttl string, updatedAt time.Time) (bool, string) {
+	normalizedTTL := strings.ToLower(strings.TrimSpace(ttl))
+
 	// Handle zero TTL explicitly (always expired).
-	if isZeroTTL(ttl) {
-		return true, fmt.Sprintf("Source cache expired (TTL: %s, always re-pull)", ttl)
+	if isZeroTTL(normalizedTTL) {
+		return true, fmt.Sprintf("Source cache expired (TTL: %s, always re-pull)", normalizedTTL)
 	}
 
-	ttlDuration, err := duration.ParseDuration(ttl)
+	ttlDuration, err := duration.ParseDuration(normalizedTTL)
 	if err != nil {
-		return true, fmt.Sprintf("Invalid source TTL %q; forcing re-provision to avoid stale cache", ttl)
+		return true, fmt.Sprintf("Invalid source TTL %q; forcing re-provision to avoid stale cache", ttl)
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/provisioner/source/provision_hook.go` around lines 346 - 369, The TTL
string should be normalized (trimmed and lowercased) before being checked or
parsed to avoid accidental "invalid TTL" or missed zero checks; update
isSourceCacheExpired to call a normalizedTTL :=
strings.ToLower(strings.TrimSpace(ttl)) and use normalizedTTL for the isZeroTTL
check and for duration.ParseDuration, and either adjust isZeroTTL to expect a
normalized value or perform the same normalization inside isZeroTTL so values
like " 0S ", "0M", or " 0 " are correctly treated as zero TTL; keep all existing
return messages but continue to display the original ttl where desirable or
switch to normalizedTTL consistently.

pkg/provisioner/source/extract_test.go (1)

673-738: Consolidate TTL extraction cases into one table-driven test.

These four tests follow the same setup/assertion pattern; one table-driven test will reduce duplication and make future TTL cases easier to add.

♻️ Refactor sketch

-func TestExtractSource_WithTTL(t *testing.T) { ... }
-func TestExtractSource_WithoutTTL(t *testing.T) { ... }
-func TestExtractSource_WithInvalidTTL(t *testing.T) { ... }
-func TestExtractSource_WithZeroTTL(t *testing.T) { ... }
+func TestExtractSource_TTL(t *testing.T) {
+	tests := []struct {
+		name        string
+		source      map[string]any
+		expectErr   error
+		expectedTTL string
+	}{
+		// valid / missing / invalid / zero TTL cases
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			componentConfig := map[string]any{"source": tt.source}
+			result, err := ExtractSource(componentConfig)
+			// assertions...
+		})
+	}
+}

As per coding guidelines, "Use table-driven tests for testing multiple scenarios in Go."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/provisioner/source/extract_test.go` around lines 673 - 738, The four TTL
tests for ExtractSource are duplicated and should be consolidated into a single
table-driven test: create a slice of test cases (with fields like name, ttlValue
(nil/""/string), expectedTTL, expectError bool, expectedErr) and iterate with
t.Run; for each case build the componentConfig with
"source":{"uri":"github.com/example/repo//module","version":..., "ttl":
ttlValue} (omit ttl when nil), call ExtractSource, and assert Uri and Version
always, then check TTL equals expectedTTL when expectError is false and assert
errors (require.Error / assert.ErrorIs with errUtils.ErrSourceInvalidSpec) and
nil result when expectError is true; replace the four individual
TestExtractSource_* functions with this single table-driven test to cover valid,
missing, invalid, and zero TTL scenarios.

pkg/provisioner/source/provision_hook_test.go (1)

335-402: Add an invalid-TTL case to lock in the defensive safety-net behavior.

This suite is strong; one more case (ttl: "invalid") asserting reprovision + reason containing Invalid source TTL would prevent regressions in the fallback path.

As per coding guidelines, "Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/provisioner/source/provision_hook_test.go` around lines 335 - 402, Add a
test case to TestNeedsProvisioning_TTL that uses ttl: "invalid" to verify the
defensive fallback in needsProvisioning; update the tests slice in
TestNeedsProvisioning_TTL to include an entry with name "invalid TTL forces
reprovision", ttl: "invalid", any updatedAt (e.g., time.Now()), expected: true,
and expectExpired: true, then after calling needsProvisioning(dirPath,
sourceSpec, true) assert the result is true and assert.Contains(t, reason,
"Invalid source TTL") to lock in the error-path behavior for
schema.VendorComponentSource.TTL handling.

pkg/provisioner/source/provision_hook.go (1)

355-358: Include the parse error details in the invalid TTL reason.

Current message is good, but dropping err hides the exact parsing failure and slows troubleshooting.

Suggested tweak

 	ttlDuration, err := duration.ParseDuration(ttl)
 	if err != nil {
-		return true, fmt.Sprintf("Invalid source TTL %q; forcing re-provision to avoid stale cache", ttl)
+		return true, fmt.Sprintf(
+			"Invalid source TTL %q (%v); forcing re-provision to avoid stale cache",
+			ttl,
+			err,
+		)
 	}

As per coding guidelines, provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/provisioner/source/provision_hook.go` around lines 355 - 358, The invalid
TTL error message hides the parsing error; update the block that calls
duration.ParseDuration (assigning to ttlDuration, err) so the returned reason
includes the parse error details (err) alongside the TTL value — e.g., include
err.Error() or %v in the fmt.Sprintf used in the return statement where
ttlDuration and err are in scope — so the returned string shows both the invalid
TTL and the underlying parse error for debugging.

pkg/provisioner/source/provision_hook_test.go (1)

450-454: Tighten non-expired-path assertions with empty reason checks.

Right now non-expired cases only check expired/result == false; asserting reason is empty would catch noisy or stale reason regressions.

Optional hardening

 			result, reason := needsProvisioning(dirPath, sourceSpec, true)
 			assert.Equal(t, tt.expected, result)
 			if tt.expectExpired {
 				assert.Contains(t, reason, "Source cache expired")
+			} else {
+				assert.Empty(t, reason)
 			}

 			expired, reason := isSourceCacheExpired(tt.ttl, tt.updatedAt)
 			assert.Equal(t, tt.expected, expired)
 			if tt.expectedReason != "" {
 				assert.Contains(t, reason, tt.expectedReason)
+			} else {
+				assert.Empty(t, reason)
 			}

Also applies to: 1139-1142

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/provisioner/source/provision_hook_test.go` around lines 450 - 454, The
tests call needsProvisioning(dirPath, sourceSpec, true) and currently only
assert result/expired is false for non-expired cases; update the test assertions
to also assert that the returned reason string is empty when tt.expectExpired is
false to catch stray or stale reason messages—i.e., where the table-driven test
checks tt.expectExpired, add an assert.Equal(t, "", reason) (or assert.Empty)
for the non-expired branch; apply the same tightening to the other occurrence
around the 1139-1142 test block so both places validate an empty reason on
non-expired outcomes.