fix(toolbox-core): Prevent rebinding of parameters in ToolboxTool

[anubhav756]

This pull request updates the bind_parameters helper function within the ToolboxTool of the toolbox-core package to enhance parameter binding management.

• The helper now throws an explicit error if a user attempts to bind a parameter that has already been bound. This change aims to prevent unexpected behavior and improve the clarity of parameter assignments.
• The error that previously occurred when a user tried to bind a parameter not present in the tool's schema has been removed. This validation will be handled by a strict mode implementation in a future PR.

[kurtisvg]

I don't think the PR matches the behavior we discussed previously.

I think we want the 'default' behavior to be you can't bind a parameter if it doesn't exist. This behavior is the current functionality. This covers "rebinding" of parameters because once a param is bound, it "no longer exists" from the normal parameters perspective.

We discussed a small semantic improvement in the return error -- that if the parameter existed but was already bound, we would return a different error. IMO, this is a low priority.

Separately we discussed potentially needing to implement a strict flag. I thought when we left it we decided to implement the behavior that would error if "any of the parameters provided were attached to exactly 0 tools", and implement that consistently across both the single and group functions.

[anubhav756]

I don't think the PR matches the behavior we discussed previously.

I think we want the 'default' behavior to be you can't bind a parameter if it doesn't exist. This behavior is the current functionality. This covers "rebinding" of parameters because once a param is bound, it "no longer exists" from the normal parameters perspective.

We discussed a small semantic improvement in the return error -- that if the parameter existed but was already bound, we would return a different error. IMO, this is a low priority.

Separately we discussed potentially needing to implement a strict flag. I thought when we left it we decided to implement the behavior that would error if "any of the parameters provided were attached to exactly 0 tools", and implement that consistently across both the single and group functions.

Just wanted to clarify my approach here:

You're totally right – the main goal is still the default behavior we discussed: error if you try to bind a parameter that doesn't exist. That core logic eventually isn't changing.

The reason it might look different in this PR is I was in the middle of moving this "parameter existence" check.

The plan is to shift it from the bind_parameters helper into the ToolboxTool constructor.

This is based on our offline chat (link) about making __init__ consistently error if any specified param exists in zero tools.

So, this PR was just removing the check from bind_parameters as step 1. For now, to ensure the check remains active and isn't lost between PRs, I've added it back into bind_parameters in this current PR. I will then remove it from bind_parameters and simultaneously add it to __init__ of ToolboxTool in a subsequent PR. This way, it will effectively be a "move" of the check from one place to another within a single future PR, rather than removing it now only to add it back later.

Regarding the other check I added in bind_parameters (for trying to re-bind an already-bound parameter):

I put that specific check here because attempting a rebind can only really happen when calling bind_parameters dynamically, not during __init__. So it seemed like the right place for that specific validation.

I can see how it looked like I swapped the high-priority "existence" check for the lower-priority "rebinding" check within this function 🤔. But really, the main existence check will just move to the constructor for consistency, not removed.

The end state will be:

1. __init__ errors if a param doesn't exist in any tool.
2. bind_parameters errors if you try to re-bind an existing bound param.

Note

Since these two states are mutually exclusive, I suggest using distinct error messages for each, rather than the same "unable to bind parameters: no parameter named <ARG>" for both, which could be ambiguous.

Hope that clarifies the plan! Let me know if that makes sense or if you have other concerns. 🙂

[anubhav756]

Here’s a summary of the updated plan:

We've decided not to introduce the strict flag into the ToolboxTool itself. The primary reason is that methods like add_auth_token_getters and add_bound_params operate on a single tool instance at a time (#205). In this context, a strict flag wouldn't change their behavior – they would operate identically whether the flag was True or False. The complexities around group tools and the strict flag don't apply to these specific ToolboxTool class methods.

This decision has a positive cascading effect:

1. Since we're not storing or using the strict flag in ToolboxTool, we don't need to pass it to the constructor or these methods.
2. The validation checks within add_auth_token_getters and add_bound_params become much simpler. We no longer need to replicate or refactor the more complex strict flag logic from the client into ToolboxTool.
   • For add_auth_token_getters, in fix: Add validation to ensure added auth token getters are used by the tool #220 we start leveraging the additional return value from the existing identify_required_authn_params helper from utils, which helps handle the necessary validation and avoids code duplication.
   • For add_bound_params, the primary validation needed is to ensure the parameter exists in the schema. This check is already in place.
3. The additional check I had previously added to bind_parameters to specifically detect if a parameter was already bound is, as we discussed, a lower priority. The existing check (that the parameter must exist to be bound) implicitly covers this because once a parameter is bound, it's no longer "available" in the same way. However, a more specific error message for re-binding could slightly improve UX.
4. My original plan to move the "parameter existence" check from bind_parameters into the ToolboxTool constructor (as part of making __init__ error consistently) is no longer necessary. Given the simplified validation (essentially a single-line check to see if a parameter exists in the schema for add_bound_params), it feels like over-engineering to move it. Keeping this straightforward check within add_bound_params is simpler and sufficient for now.

Therefore, with the strict flag being out of scope for ToolboxTool's direct method, we won't be moving this check now.