Skip to content

Conversation

@tgrunnagle
Copy link
Contributor

Adds docs for #2592

@tgrunnagle tgrunnagle requested a review from JAORMX November 14, 2025 20:11
@codecov
Copy link

codecov bot commented Nov 14, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 55.84%. Comparing base (4ceed40) to head (4647eba).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2593   +/-   ##
=======================================
  Coverage   55.84%   55.84%           
=======================================
  Files         312      312           
  Lines       29541    29541           
=======================================
  Hits        16498    16498           
  Misses      11601    11601           
  Partials     1442     1442           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tgrunnagle tgrunnagle enabled auto-merge (squash) November 14, 2025 22:27
Copy link
Contributor

@jhrozek jhrozek left a comment

Choose a reason for hiding this comment

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

Documentation Review Findings

I've reviewed the composite workflow documentation against the actual CRD implementation and found critical issues where the examples don't match the code. These issues will cause workflows to fail or behave incorrectly if users copy-paste the examples.

Critical Issues Summary

  1. Parameter structure - Using incorrect JSON Schema format (2 files)
  2. Step type value - Using tool instead of tool_call (41 occurrences)
  3. Field naming - Using depends_on instead of dependsOn - THIS WILL CAUSE SILENT FAILURE (42 occurrences)
  4. Error action values - Using continue_on_error instead of continue (5 occurrences)
  5. Error field names - Wrong field names and non-existent retry_delay field (8 occurrences)
  6. Retry backoff formula - Claims 2^n but code uses 1.5^n (3 occurrences)
  7. Broken file links - Wrong relative paths (2 occurrences)

Questions for Author

  • CLI workflow command: What was intended? (doesn't exist)
  • Max steps validation: Add CRD validation or keep runtime-only?
  • Max retry validation: Add webhook validation or keep runtime capping?
  • Template features: Document additional features?
  • API endpoint: Clarify programmatic vs HTTP access?

All findings verified against:

  • cmd/thv-operator/api/v1alpha1/virtualmcpcompositetooldefinition_types.go
  • cmd/thv-operator/api/v1alpha1/virtualmcpserver_types.go
  • pkg/vmcp/composer/workflow_engine.go
  • pkg/vmcp/composer/dag_executor.go

@tgrunnagle tgrunnagle force-pushed the issue_156_docs_2025-11-14 branch from a03b1cd to 4647eba Compare November 17, 2025 16:29
@tgrunnagle tgrunnagle disabled auto-merge November 17, 2025 17:02
@tgrunnagle
Copy link
Contributor Author

@claude use this repo's doc-review skill to review the documentation changes in this pull request

@claude
Copy link
Contributor

claude bot commented Nov 17, 2025

Claude encountered an error —— View job


I'll analyze this and get back to you.

Copy link
Contributor

@jhrozek jhrozek left a comment

Choose a reason for hiding this comment

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

✅ Approved - All Issues Resolved

All review comments have been addressed! Here's the final status:

Issues Fixed

  1. Parameter structure - Correct flat map format in both files
  2. Step type - Documentation was correct (), CRD was fixed in commit 107ea07
  3. Field naming - All 42 occurrences changed to (camelCase)
  4. Error action values - All changed from to
  5. Error field names - Changed to , removed non-existent
  6. Backoff formula - Corrected to 1.5x multiplier with ±50% randomization
  7. File links - Fixed to use correct relative paths (../kind/)

My Correction

I initially flagged type: tool as wrong, but I was mistaken. The documentation was correct - the CRD had the wrong enum value (tool_call) which was fixed in commit 107ea07 to match the actual runtime code (StepTypeTool = "tool").

Questions for Future Consideration

The review raised some questions about validation strategy and documentation that can be addressed in future work if desired:

  • Max retry/steps validation at CRD vs runtime
  • Documenting additional template features
  • Clarifying API access types

Great work on the thorough fixes! The documentation now accurately reflects the implementation. 🎉

@tgrunnagle tgrunnagle merged commit a0f94a9 into main Nov 17, 2025
29 checks passed
@tgrunnagle tgrunnagle deleted the issue_156_docs_2025-11-14 branch November 17, 2025 19:10
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