[codex] migrate Tripo MCP to v3 API#4
Conversation
📝 WalkthroughWalkthroughThis PR migrates the Tripo integration to v3, adds new image/model/mesh/animation/account provider methods, updates MCP server capability wiring and tool registration, refreshes request/response types and model catalog data, and revises docs, skills, templates, and tests to match the new surface. ChangesTripo v3 Provider and MCP Tooling Migration
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant MCPServer as MCP Server
participant Provider as TripoProvider
participant API as Tripo v3 API
Client->>MCPServer: call generation or status tool
MCPServer->>Provider: invoke capability method
Provider->>API: POST /generation/* or GET /tasks/{taskID}
API-->>Provider: task or status payload
Provider-->>MCPServer: typed result
MCPServer-->>Client: task started or status details
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
062f3da to
1548aaa
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Dockerfile (1)
9-12: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winRun the runtime stage as a non-root user.
The final Alpine image still defaults to root. If this container is ever run with mounted volumes or a compromised binary, root-in-container widens the blast radius.
🛠 Suggested fix
FROM alpine:3.21 RUN apk add --no-cache ca-certificates +RUN addgroup -S trident && adduser -S -G trident trident COPY --from=build /trident-mcp /usr/local/bin/trident-mcp +USER trident ENTRYPOINT ["trident-mcp"]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Dockerfile` around lines 9 - 12, The runtime stage in the Dockerfile still runs as root, so update the final Alpine stage to create and switch to a non-root user before the ENTRYPOINT. Keep the existing build artifact copy and certificate install, then add the user/group setup and ensure the copied binary remains executable when invoked through trident-mcp.Source: Linters/SAST tools
🧹 Nitpick comments (4)
internal/provider/tripo/e2e_test.go (1)
192-195: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRetry transient
Statuserrors inwaitForE2ETaskinstead of failing immediately.A single transient network error or temporary API hiccup during a 10-minute polling loop will
t.Fatalfand waste credits already spent on task creation. The loop should tolerate intermittent failures and continue polling until the deadline.♻️ Proposed fix: continue polling on transient errors
func waitForE2ETask(t *testing.T, p *TripoProvider, taskID string, timeout time.Duration) *provider.ModelTaskStatus { t.Helper() deadline := time.Now().Add(timeout) + var lastErr error for time.Now().Before(deadline) { status, err := p.Status(context.Background(), taskID) if err != nil { - t.Fatalf("Status(%s): %v", taskID, err) + lastErr = err + time.Sleep(2 * time.Second) + continue } + lastErr = nil switch status.Status { case "success": return status case "failed", "cancelled", "banned", "expired": t.Fatalf("task %s ended with status %s: %s", taskID, status.Status, status.Error) } time.Sleep(2 * time.Second) } - t.Fatalf("task %s did not complete within %s", taskID, timeout) + if lastErr != nil { + t.Fatalf("task %s did not complete within %s (last error: %v)", taskID, timeout, lastErr) + } + t.Fatalf("task %s did not complete within %s", taskID, timeout) return nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/provider/tripo/e2e_test.go` around lines 192 - 195, The polling loop in waitForE2ETask currently exits immediately on any Status error, which makes transient API/network failures fail the whole task. Update the Status call handling in the e2e_test polling logic to tolerate temporary errors by retrying until the deadline instead of calling t.Fatalf right away, and keep the existing success path based on the status/taskID checks.internal/provider/tripo/models.go (1)
187-204: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAliases are declared twice and can drift.
The hardcoded alias entries here (
tripo-v3.1,v3.1,h3.1,v3.0,v2.5,p1, …) duplicate theAliasesfields already on the catalog entries. Two consequences:
- Divergence risk: adding an alias to a catalog entry won't affect resolution unless it's also added here, and vice-versa.
turboandv2.0are accepted by the resolver but are not listed in any catalog entry'sAliases, soListModels/get_configwon't advertise them even though they resolve.Consider deriving the map from the catalog (including a lowercased ID key so canonical IDs resolve consistently) and moving
turbo/v2.0into the respective entries'Aliases.♻️ Sketch
func buildModelVersionMap() map[string]string { versions := make(map[string]string, len(modelCatalog)*2) for _, model := range modelCatalog { versions[strings.ToLower(model.ID)] = model.APIVersion for _, alias := range model.Aliases { versions[strings.ToLower(alias)] = model.APIVersion } } return versions }This requires
turboandv2.0to be added to theTripo Turbo/Tripo v2.0Aliasesfields.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/provider/tripo/models.go` around lines 187 - 204, The alias resolution in buildModelVersionMap is duplicating the Aliases already defined on modelCatalog, which can drift and leaves some accepted names undiscoverable. Update buildModelVersionMap to derive entries from modelCatalog by mapping each model’s ID and its Aliases (normalized consistently, e.g. lowercased) to APIVersion, and remove the hardcoded alias list. Also move the resolver-only names like turbo and v2.0 into the corresponding model entries’ Aliases so ListModels/get_config advertise them through the catalog.skills/3d-gen/SKILL.md (1)
3-3: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor style tweaks for compound adjective and sentence variety.
Line 20: "High quality generation" should be "High-quality generation" (compound adjective). Line 176: Three successive sentences begin with "Run" — consider rewording for variety.
✏️ Proposed style fixes
-| **Tripo H3.0** | `v3.0-20250812` (`tripo-v3.0`, `v3.0` aliases) | High quality generation | Yes | +| **Tripo H3.0** | `v3.0-20250812` (`tripo-v3.0`, `v3.0` aliases) | High-quality generation | Yes |-1. Run `rig_check` before rigging to confirm whether the model is suitable. -2. Run `rig_model` with the desired rig type and output format (`glb` or `fbx`). -3. Run `retarget_animation` to apply one or more animation presets. +1. Run `rig_check` before rigging to confirm whether the model is suitable. +2. Use `rig_model` with the desired rig type and output format (`glb` or `fbx`). +3. Call `retarget_animation` to apply one or more animation presets.Also applies to: 18-46, 55-99, 111-111, 121-125, 136-136, 150-151, 160-178
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/3d-gen/SKILL.md` at line 3, Update the wording in SKILL.md to fix the compound adjective by changing the "High quality generation" phrase to "High-quality generation", and revise the sequence of repeated "Run" sentence openings near the end of the document to improve variety. Use the existing section text around the description and the later procedural steps as anchors when editing, and keep the tone and meaning unchanged.docs/DEVELOPMENT.md (1)
23-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin
govulncheckto keep the gate reproducible.
@latestmeans the documented security check can change underneath contributors and CI over time. Please pin the version here (and keep it aligned with CI) if this is meant to be the canonical verification step.🛠 Suggested fix
-go run golang.org/x/vuln/cmd/govulncheck@latest ./... +go run golang.org/x/vuln/cmd/govulncheck@<pinned-version> ./...🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/DEVELOPMENT.md` around lines 23 - 29, The verification step in the development docs uses an unpinned govulncheck invocation, which makes the security gate non-reproducible. Update the documented command to use a fixed golang.org/x/vuln/cmd/govulncheck version instead of `@latest`, and keep that version aligned with the corresponding CI check so contributors and automation run the same tool.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/provider-boundaries.md`:
- Line 10: Update the `MODEL_OUTPUT_DIR` guidance in `provider-boundaries.md` to
match `OPERATIONS.md`: replace the “must exist before downloads are attempted”
wording with language that says the directory is created automatically if
missing. Keep the instruction aligned with the `MODEL_OUTPUT_DIR` mention so it
reads as an implementation behavior rather than a manual precondition, and make
sure the surrounding text reflects that downloads do not require the directory
to be pre-created.
In `@internal/provider/tripo/models.go`:
- Around line 148-156: The Tripo animation default is still set on the legacy
rig model, so update the model entry in the Tripo models list to make rig-v2.0
the default instead of rig-v1.0. Locate the rig model definitions in the models
slice and move the Default: true flag from the rig-v1.0 entry to the rig-v2.0
entry so ListModels advertises the newer rig as the default choice.
In `@internal/provider/tripo/postprocess.go`:
- Around line 74-87: The normalizeExportOrientation helper is validating against
a normalized switch value but still returning the original input for direct
matches, so canonicalization is inconsistent. Update normalizeExportOrientation
to return the normalized orientation string for the direct-match cases in
addition to the x_up/y_up aliases, and keep the function’s callers in
postprocess.go using the normalized value before the /models/convert request.
In `@internal/provider/tripo/v3_features_test.go`:
- Around line 147-149: The test’s missing-input guard in v3_features_test is
ineffective because checking captured["input"] against an empty string won’t
catch a absent key when the map value is nil. Update the assertion around
captured and the request body check so it explicitly verifies the "input" key
exists before validating its contents, using the same test block that inspects
captured["input"] and r.URL.Path.
In `@internal/server/server.go`:
- Around line 70-96: The common MCP tools are currently tied to the status
registration path even though they only use s.common, so they are skipped
whenever ModelStatus is nil. Update server.go so the common tool registration
lives in its own helper or is invoked from registerStatusTools only when
s.common is non-nil, and make Server setup call that helper independently of
status while keeping the existing conditional registration pattern used by
registerGenerationTools, registerImageTools, registerModelProcessTools,
registerMeshTools, and registerAnimationTools.
---
Outside diff comments:
In `@Dockerfile`:
- Around line 9-12: The runtime stage in the Dockerfile still runs as root, so
update the final Alpine stage to create and switch to a non-root user before the
ENTRYPOINT. Keep the existing build artifact copy and certificate install, then
add the user/group setup and ensure the copied binary remains executable when
invoked through trident-mcp.
---
Nitpick comments:
In `@docs/DEVELOPMENT.md`:
- Around line 23-29: The verification step in the development docs uses an
unpinned govulncheck invocation, which makes the security gate non-reproducible.
Update the documented command to use a fixed golang.org/x/vuln/cmd/govulncheck
version instead of `@latest`, and keep that version aligned with the corresponding
CI check so contributors and automation run the same tool.
In `@internal/provider/tripo/e2e_test.go`:
- Around line 192-195: The polling loop in waitForE2ETask currently exits
immediately on any Status error, which makes transient API/network failures fail
the whole task. Update the Status call handling in the e2e_test polling logic to
tolerate temporary errors by retrying until the deadline instead of calling
t.Fatalf right away, and keep the existing success path based on the
status/taskID checks.
In `@internal/provider/tripo/models.go`:
- Around line 187-204: The alias resolution in buildModelVersionMap is
duplicating the Aliases already defined on modelCatalog, which can drift and
leaves some accepted names undiscoverable. Update buildModelVersionMap to derive
entries from modelCatalog by mapping each model’s ID and its Aliases (normalized
consistently, e.g. lowercased) to APIVersion, and remove the hardcoded alias
list. Also move the resolver-only names like turbo and v2.0 into the
corresponding model entries’ Aliases so ListModels/get_config advertise them
through the catalog.
In `@skills/3d-gen/SKILL.md`:
- Line 3: Update the wording in SKILL.md to fix the compound adjective by
changing the "High quality generation" phrase to "High-quality generation", and
revise the sequence of repeated "Run" sentence openings near the end of the
document to improve variety. Use the existing section text around the
description and the later procedural steps as anchors when editing, and keep the
tone and meaning unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 49aeb3d0-c42d-4e8a-906d-f4bea5faba70
⛔ Files ignored due to path filters (1)
docs/assets/potion-bottle-collage.pngis excluded by!**/*.png
📒 Files selected for processing (43)
.github/ISSUE_TEMPLATE/bug_report.md.github/ISSUE_TEMPLATE/feature_request.md.github/PULL_REQUEST_TEMPLATE.md.gitignoreAGENTS.mdARCHITECTURE.mdDockerfileREADME.mdSECURITY.mdTHREAT_MODEL.mddocs/DEVELOPMENT.mddocs/MCP_TOOLS.mddocs/OPERATIONS.mddocs/adr/0001-provider-capability-interfaces.mddocs/adr/0002-static-model-catalog.mddocs/adr/0003-task-oriented-tool-contract.mddocs/design/provider-boundaries.mddocs/patterns/common-changes.mdgo.modinternal/config/config.gointernal/config/config_test.gointernal/provider/interfaces.gointernal/provider/tripo/e2e_test.gointernal/provider/tripo/generation.gointernal/provider/tripo/generation_test.gointernal/provider/tripo/helpers_test.gointernal/provider/tripo/models.gointernal/provider/tripo/postprocess.gointernal/provider/tripo/postprocess_test.gointernal/provider/tripo/provider.gointernal/provider/tripo/provider_test.gointernal/provider/tripo/status.gointernal/provider/tripo/v3_features.gointernal/provider/tripo/v3_features_test.gointernal/provider/types.gointernal/server/server.gointernal/server/server_test.gointernal/server/tools_generation.gointernal/server/tools_image.gointernal/server/tools_v3_processing.goskills/3d-gen/SKILL.mdskills/3d-to-blender/SKILL.mdskills/multiview-3d/SKILL.md
1548aaa to
ab2762d
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab2762ddbd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ab2762d to
58eec0c
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Dockerfile (1)
1-11: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winAdd a non-root
USERto the final image. The runtime stage still defaults to root; set a non-root user (or equivalent) beforeENTRYPOINTto avoid shipping the container with root privileges.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Dockerfile` around lines 1 - 11, Add a non-root runtime user to the final Alpine stage so the container does not run as root by default. Update the final image section in the Dockerfile to create or use an unprivileged user and switch to it before the entrypoint/runtime starts, keeping the build stage unchanged.Source: Linters/SAST tools
🧹 Nitpick comments (1)
internal/provider/tripo/generation.go (1)
190-212: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the P1 model identifier and guarding
orderedMultiviewInputs.The string
"P1-20260311"is hardcoded in bothsetNonP1BoolandsetNonP1String. If the P1 model version changes, both must be updated in sync. Extracting it to a named constant (e.g.,const p1ModelVersion = "P1-20260311") would prevent drift.Additionally,
orderedMultiviewInputsaccessesviews[i]without a bounds check. The caller validates 2–4 inputs, but the function itself has no guard againstlen(values) > 4, which would cause an index-out-of-range panic. A brief comment or minimal guard would make the constraint explicit.♻️ Optional refactor
+const p1ModelVersion = "P1-20260311" + func setNonP1Bool(body map[string]any, key, model string, value *bool) { - if value == nil || model == "P1-20260311" { + if value == nil || model == p1ModelVersion { return } body[key] = *value } func setNonP1String(body map[string]any, key, model, value string) { - if model == "P1-20260311" { + if model == p1ModelVersion { return } setOptionalString(body, key, value) } func orderedMultiviewInputs(values []string) []any { + // values must contain 2-4 elements; caller validates this. views := []string{"front", "left", "back", "right"} inputs := make([]any, 0, len(values)) for i, value := range values { + if i >= len(views) { + break + } inputs = append(inputs, map[string]any{views[i]: value}) } return inputs }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/provider/tripo/generation.go` around lines 190 - 212, Extract the hardcoded P1 version string used by setNonP1Bool and setNonP1String into a shared named constant so both checks stay in sync, and update those helpers to reference it. Also harden orderedMultiviewInputs by adding an explicit guard for len(values) > 4 (or otherwise constraining the loop) before indexing views[i], so the function cannot panic if called with too many inputs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Dockerfile`:
- Around line 1-11: Add a non-root runtime user to the final Alpine stage so the
container does not run as root by default. Update the final image section in the
Dockerfile to create or use an unprivileged user and switch to it before the
entrypoint/runtime starts, keeping the build stage unchanged.
---
Nitpick comments:
In `@internal/provider/tripo/generation.go`:
- Around line 190-212: Extract the hardcoded P1 version string used by
setNonP1Bool and setNonP1String into a shared named constant so both checks stay
in sync, and update those helpers to reference it. Also harden
orderedMultiviewInputs by adding an explicit guard for len(values) > 4 (or
otherwise constraining the loop) before indexing views[i], so the function
cannot panic if called with too many inputs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 683e5d46-f4ec-4274-b426-cba43fd0a3b4
⛔ Files ignored due to path filters (1)
docs/assets/potion-bottle-collage.pngis excluded by!**/*.png
📒 Files selected for processing (43)
.github/ISSUE_TEMPLATE/bug_report.md.github/ISSUE_TEMPLATE/feature_request.md.github/PULL_REQUEST_TEMPLATE.md.gitignoreAGENTS.mdARCHITECTURE.mdDockerfileREADME.mdSECURITY.mdTHREAT_MODEL.mddocs/DEVELOPMENT.mddocs/MCP_TOOLS.mddocs/OPERATIONS.mddocs/adr/0001-provider-capability-interfaces.mddocs/adr/0002-static-model-catalog.mddocs/adr/0003-task-oriented-tool-contract.mddocs/design/provider-boundaries.mddocs/patterns/common-changes.mdgo.modinternal/config/config.gointernal/config/config_test.gointernal/provider/interfaces.gointernal/provider/tripo/e2e_test.gointernal/provider/tripo/generation.gointernal/provider/tripo/generation_test.gointernal/provider/tripo/helpers_test.gointernal/provider/tripo/models.gointernal/provider/tripo/postprocess.gointernal/provider/tripo/postprocess_test.gointernal/provider/tripo/provider.gointernal/provider/tripo/provider_test.gointernal/provider/tripo/status.gointernal/provider/tripo/v3_features.gointernal/provider/tripo/v3_features_test.gointernal/provider/types.gointernal/server/server.gointernal/server/server_test.gointernal/server/tools_generation.gointernal/server/tools_image.gointernal/server/tools_v3_processing.goskills/3d-gen/SKILL.mdskills/3d-to-blender/SKILL.mdskills/multiview-3d/SKILL.md
✅ Files skipped from review due to trivial changes (12)
- .github/ISSUE_TEMPLATE/feature_request.md
- .gitignore
- .github/PULL_REQUEST_TEMPLATE.md
- .github/ISSUE_TEMPLATE/bug_report.md
- docs/patterns/common-changes.md
- docs/design/provider-boundaries.md
- docs/adr/0001-provider-capability-interfaces.md
- SECURITY.md
- docs/adr/0002-static-model-catalog.md
- go.mod
- README.md
- docs/MCP_TOOLS.md
🚧 Files skipped from review as they are similar to previous changes (22)
- docs/adr/0003-task-oriented-tool-contract.md
- internal/config/config_test.go
- internal/config/config.go
- internal/provider/tripo/postprocess.go
- internal/server/tools_image.go
- skills/3d-to-blender/SKILL.md
- internal/provider/interfaces.go
- internal/provider/tripo/helpers_test.go
- skills/multiview-3d/SKILL.md
- internal/server/tools_generation.go
- internal/server/tools_v3_processing.go
- internal/provider/tripo/v3_features_test.go
- internal/provider/tripo/e2e_test.go
- internal/provider/tripo/provider_test.go
- internal/provider/tripo/provider.go
- internal/server/server_test.go
- AGENTS.md
- internal/server/server.go
- internal/provider/tripo/models.go
- internal/provider/tripo/postprocess_test.go
- internal/provider/types.go
- internal/provider/tripo/v3_features.go
Summary
Migrates the Tripo provider from the older API surface to the v3 API and expands the MCP tool coverage for the current Tripo feature set.
The branch intentionally keeps the work split into two commits:
docs: improve agent readiness documentationMigrate Tripo provider to v3 APIImplementation
Documentation
Validation
go build ./cmd/trident-mcpgo test ./... -count=1go vet ./...golangci-lint rungo run golang.org/x/vuln/cmd/govulncheck@latest ./...go test -tags=e2e -count=1 -run 'TestE2E_(UploadFile|CreateFileUpload|AccountBalanceAndUsage)$' ./internal/provider/tripo/ -v -timeout 10magentready assess . -o .agentreadyreported69.6/100Silvermmdcvalidated both Mermaid diagrams inARCHITECTURE.mdCredit-spending generation paths were left behind explicit E2E flags and were also exercised manually through Claude Code during migration testing.
Summary by CodeRabbit
get_task.