Skip to content

fix(toolset): honour STACKONE_BASE_URL and remove documented fiction - #197

Open
willleeney wants to merge 2 commits into
mainfrom
fix/sdk-conformance-patch-fixes
Open

willleeney wants to merge 2 commits into
mainfrom
fix/sdk-conformance-patch-fixes

Conversation

@willleeney

@willleeney willleeney commented Sep 10, 2026

Copy link
Copy Markdown

Summary

Two Phase 1 patch fixes from the SDK platform rewrite proposal. Rebased onto 2.10.1.

STACKONE_BASE_URL is now honoured. It was ignored entirely, so a consumer pointing at staging silently talked to production (consumer contract §2.4, defect 7). Precedence is explicit arg > env var > default, matching the STACKONE_API_KEY handling on the line above and node's behaviour. The docstring documents it rather than leaving a constructor that reads an undocumented env var.

Three documented-but-untrue claims removed (consumer contract §3), each verified against the source first:

Claim Reality
configure_implicit_feedback + the "Implicit Feedback (Beta)" section Does not exist. models.py carries a # Implicit feedback removed marker where it used to live.
Action-filter exclusions like "!*_delete_*" _filter_by_action is a bare any(fnmatch(...)) — no negation. fnmatch("sf_delete_lead", "!*_delete_*") is False, so it matches nothing and excludes nothing.
"This tool is automatically included in the toolset" Never injected client-side. get_tool("tool_feedback") returned None, so the next line of the shipped example raised AttributeError.

The feedback docs now point at the MCP-served stackone_submit_feedback and the create_feedback_tool path that actually works — verified by executing it.

Scope changes after review

Three things were dropped from earlier revisions of this branch:

  • mcp<2.0.0 pin — already on main via fix(deps): Avoid DOA-pin mcp below 2.0.0 ahead of upstream MCP v2 release #194. My version was byte-identical. Removed.
  • Widening pydantic-ai-slim to <3.0.0 — removed. See below.
  • Client-side feedback-tool auto-injection — removed. The MCP service already registers stackone_submit_feedback as a global, always-present tool (mcp.service.ts:448, unscoped to account or tool mode), so injecting a second tool_feedback gave one capability two names, two schemas and two routes, and it bypassed the provider/action filters. Node's auto-append is the same defect rather than the target state; both hand-built tools go together in the MCP-execution rewrite.

On the pydantic-ai cap

I initially widened pydantic-ai-slim to <3.0.0, having measured that the adapter works fine on 2.x: Tool.from_schema is signature-identical on 1.83.0 and 2.42.0, and all 11 integration tests (including a live Agent.run_sync) pass on both majors.

That is still true, but widening it here was wrong. #195 capped the pydantic-ai and examples extras together deliberately, and the resolver shows why they cannot move apart:

stackone-ai:dev depends on pydantic-ai-slim>=2.18.0
and stackone-ai[examples] depends on pydantic-ai>=1.83.0,<2.0.0
-> incompatible -> your project's requirements are unsatisfiable

So moving to pydantic-ai 2.x means widening both extras and verifying the example scripts (crewai / langgraph / openai) on 2.x — a coupled dependency migration, not a patch. It also reds uv lock --check, which CI runs. Worth doing as its own PR; the evidence above is the starting point.

One related gap worth recording: pydantic_ai is in no dependency group, so pytest.importorskip skips all 11 integration tests silently in CI. Whatever upper bound we settle on, nothing currently exercises it.

Verified

  • Suite: 277 passed / 21 skipped. The 9 failures are pre-existing — identical count on clean origin/main (langgraph and mcp optional deps absent locally).
  • uv lock --check: passes (this is what CI's uv sync --all-extras --locked enforces).
  • ruff check: clean.
  • Precedence: explicit https://x.example.com wins over env; env https://staging.example.com used when no arg; default otherwise.
  • Downstream consumers: stackone-adk-plugin 32/32 green against this branch. pydantic-ai-harness[stackone] does not depend on stackone-ai at all (it uses fastmcp + pydantic-ai-slim directly), so it is unaffected.

Noted, not fixed

Out of scope for this change, but found while verifying it:

  • StackOneTool.call() accepts an options kwarg and drops it — neither models.py:396 nor :398 forwards it to execute().
  • create_feedback_tool is absent from __all__; it is only reachable as stackone_ai.feedback.tool.create_feedback_tool.
  • __init__.py hardcodes __version__ while toolset.py:84 derives it from importlib.metadata — the same drift class as the hardcoded stackone-python/1.0.0 User-Agent in contract §7.
  • .hypothesis/ is not in .gitignore.

🤖 Generated with Claude Code

Copilot AI lite review requested due to automatic review settings September 10, 2026 08:26

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Two moderate toolset issues and one documentation nit remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Aligns the Python SDK with Node SDK behavior for base URLs, feedback tools, dependency constraints, and documentation.

Changes:

  • Adds STACKONE_BASE_URL fallback and automatic feedback-tool injection.
  • Pins mcp below version 2.
  • Updates tests for feedback-tool behavior.
  • Removes or corrects stale documentation.
File summaries
File Summary
uv.lock Records the MCP dependency constraint.
tests/test_semantic_search.py Updates semantic-search expectations.
tests/test_fetch_tools.py Covers automatic feedback-tool inclusion.
stackone_ai/toolset.py Adds base URL fallback and feedback-tool injection. Two moderate issues remain: trailing slashes can produce double-slash feedback URLs, and the pseudo-connector may trigger unnecessary semantic-search requests.
README.md Removes obsolete feedback documentation.
pyproject.toml Pins mcp to <2.0.0.
CLAUDE.md Corrects the tool-filtering example. Nit: the example still lacks standalone imports and initialization.
Review details

Suppressed comments (1)

stackone_ai/toolset.py:597

  • This new environment fallback has no regression test: the existing initialization tests cover the default and explicit base_url, but not STACKONE_BASE_URL or its precedence over the constructor argument. Add coverage for both cases so this parity behavior is verified.
        self.base_url = base_url or os.getenv("STACKONE_BASE_URL") or DEFAULT_BASE_URL
  • Files reviewed: 6/7 changed files
  • Comments generated: 3
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread stackone_ai/toolset.py
self.api_key: str = api_key_value
self.account_id = account_id
self.base_url = base_url or DEFAULT_BASE_URL
self.base_url = base_url or os.getenv("STACKONE_BASE_URL") or DEFAULT_BASE_URL
Comment thread stackone_ai/toolset.py Outdated
if actions:
all_tools = [tool for tool in all_tools if self._filter_by_action(tool.name, actions)]

all_tools.append(self._create_feedback_tool())
Comment thread CLAUDE.md
```python
# Use glob patterns for tool selection
tools = StackOneToolSet(include_tools=["bamboohr_*", "!bamboohr_create_*"])
tools = toolset.fetch_tools(actions=["bamboohr_*"])

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

1 issue found across 7 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="CLAUDE.md">

<violation number="1" location="CLAUDE.md:60">
P3: Make this example standalone by importing and initializing `StackOneToolSet` before calling `fetch_tools`; as written, copying the block raises `NameError` for `toolset`.</violation>
</file>

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread stackone_ai/toolset.py Outdated
Comment thread stackone_ai/toolset.py Outdated
Comment thread CLAUDE.md
```python
# Use glob patterns for tool selection
tools = StackOneToolSet(include_tools=["bamboohr_*", "!bamboohr_create_*"])
tools = toolset.fetch_tools(actions=["bamboohr_*"])

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P3: Make this example standalone by importing and initializing StackOneToolSet before calling fetch_tools; as written, copying the block raises NameError for toolset.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At CLAUDE.md, line 60:

<comment>Make this example standalone by importing and initializing `StackOneToolSet` before calling `fetch_tools`; as written, copying the block raises `NameError` for `toolset`.</comment>

<file context>
@@ -57,7 +57,7 @@ StackOne AI SDK is a Python library that provides a unified interface for access
 ```python
 # Use glob patterns for tool selection
-tools = StackOneToolSet(include_tools=["bamboohr_*", "!bamboohr_create_*"])
+tools = toolset.fetch_tools(actions=["bamboohr_*"])

</file context>


</details>

```suggestion
from stackone_ai import StackOneToolSet

toolset = StackOneToolSet(api_key="your-api-key")
tools = toolset.fetch_tools(actions=["bamboohr_*"])

@willleeney willleeney changed the title fix(sdk): align Python SDK with Node SDK parity (Phase 1 patch fixes) fix(sdk): Phase 1 patch fixes — base URL env var, mcp pin, stale docs Sep 10, 2026

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

0 issues found across 3 files (changes from recent commits).

Requires human review: Auto-approval blocked by 1 unresolved issue from previous reviews.

Re-trigger cubic

Comment thread uv.lock

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

0 issues found across 2 files (changes from recent commits).

Requires human review: Auto-approval blocked by 1 unresolved issue from previous reviews.

Re-trigger cubic

STACKONE_BASE_URL was ignored entirely, so a consumer pointing at staging
silently talked to production. Now read as a fallback, matching the
STACKONE_API_KEY handling directly above it and node's behaviour. Precedence
is explicit arg > env var > default; the docstring says so rather than
leaving the env var undocumented.

Also removes three documented-but-untrue claims (consumer contract §3):

- `configure_implicit_feedback` and the whole "Implicit Feedback (Beta)"
  section. None of it exists in the source; models.py carries a
  `# Implicit feedback removed` marker where it used to live.
- Action-filter "exclusions" like `"!*_delete_*"`. `_filter_by_action` is a
  bare `any(fnmatch(...))` with no negation, so the pattern matches nothing
  and excludes nothing.
- "This tool is automatically included in the toolset." The feedback tool is
  never injected client-side, so the worked example returned None and the
  next line raised AttributeError. Replaced with the MCP-served
  `stackone_submit_feedback` and the `create_feedback_tool` path that
  actually works — verified by running it.
@willleeney
willleeney force-pushed the fix/sdk-conformance-patch-fixes branch from e4a5dac to f67e461 Compare September 11, 2026 13:00
@willleeney willleeney changed the title fix(sdk): Phase 1 patch fixes — base URL env var, mcp pin, stale docs fix(toolset): honour STACKONE_BASE_URL and remove documented fiction Sep 11, 2026
The env-var read had no regression cover: reverting it passed the suite clean,
so the headline behaviour change of this PR was unprotected. Three tests pin
the contract — explicit argument > env var > default.

test_init_with_api_key also asserted the default base_url without clearing the
environment, so this change made it fail for anyone with STACKONE_BASE_URL
exported. It now clears the env for that assertion.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

0 issues found across 1 file (changes from recent commits).

Requires human review: Auto-approval blocked by 1 unresolved issue from previous reviews.

Re-trigger cubic

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.

2 participants