Add tool-calling scoping note to the models CI-stub section - #621
Open
heskew wants to merge 2 commits into
Open
Conversation
Follow-up to review feedback on #597: defineBackend defaults the tools capability to false, and generate() calls that declare tools auto-require it — so the simple reply stub fails a tool-calling application up front, before its generate() runs. Call this out as a third scoping note: tool tests need tools: true plus a generate() that returns the expected tool-call sequence; the plain stub stays flagless. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vq8zum5E6fbjzmLsdDDkJh
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates the local development documentation for models to include a third scoping note explaining tool-calling capabilities and how defineBackend defaults the tools capability to false. The reviewer suggested formatting these three scoping notes as a bulleted list to improve readability and make each point more distinct.
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-621 This preview will update automatically when you push new commits. |
Per review: the three-notes paragraph had grown too dense to scan. Reuse the bold-led pattern from the parity-caveats section rather than a bulleted list, keeping the page's prose style. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vq8zum5E6fbjzmLsdDDkJh
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-621 This preview will update automatically when you push new commits. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to @kriszyp's post-approval comment on #597, which merged before it was addressed.
The "Offline and CI stubs" section framed the reply stub as a drop-in offline replacement under the application's existing logical name, but
defineBackenddefaults thetoolscapability tofalse, and agenerate()call that declares tools automatically requires that capability (Models.tsbuildRequires) — so a tool-calling application fails against the stub up front, before itsgenerate()runs.This adds the third scoping note as suggested: tool-calling tests need a stub defined with
tools: truewhosegenerate()returns the tool-call sequence the test expects, and the simple reply stub deliberately stays flagless — the up-front failure is the honest signal.One-sentence doc change; verified against
resources/models/backendRegistry.ts(tools = falsedefault) andresources/models/Models.ts(tools auto-require) on harper main. Prettier and the Docusaurus build pass.🤖 Generated with Claude Code
https://claude.ai/code/session_01Vq8zum5E6fbjzmLsdDDkJh