Skip to content

Conversation

@ilopezluna
Copy link
Contributor

@ilopezluna ilopezluna commented Nov 5, 2025

This pull request adds a comprehensive integration test for the model tagging feature in the CLI.
Note: tagging using an ID as source cases are disabled because it's not yet supported

@ilopezluna ilopezluna marked this pull request as ready for review November 5, 2025 10:22
@ilopezluna ilopezluna requested a review from a team November 5, 2025 10:22
Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey there - I've reviewed your changes and they look great!

Prompt for AI Agents
Please address the comments from this code review:

## Individual Comments

### Comment 1
<location> `cmd/cli/commands/integration_test.go:573-574` </location>
<code_context>
+			require.Equal(t, modelID, taggedModel.ID,
+				"Tagged model ID mismatch. Expected: %s, Got: %s", modelID, taggedModel.ID)
+
+			// Verify the digest matches
+			require.Equal(t, digest, taggedModel.ID,
+				"Tagged model digest mismatch. Expected: %s, Got: %s", digest, taggedModel.ID)
+
</code_context>

<issue_to_address>
**issue (testing):** Possible issue: digest is compared to model ID instead of digest.

Please confirm whether taggedModel.ID should represent the digest, or if the test should compare against a dedicated digest field. Using the wrong field may cause inaccurate test outcomes.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@ericcurtin ericcurtin merged commit d8c8f68 into main Nov 5, 2025
10 checks passed
@ericcurtin ericcurtin deleted the test-tag branch November 5, 2025 11:12
Copy link
Contributor

@doringeman doringeman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants