Skip to content

fix: optional Table.ownership and explicit provider timeouts - #1162

Merged
cbcoutinho merged 1 commit into
masterfrom
fix/model-nullability-provider-timeouts
Jul 26, 2026
Merged

fix: optional Table.ownership and explicit provider timeouts#1162
cbcoutinho merged 1 commit into
masterfrom
fix/model-nullability-provider-timeouts

Conversation

@cbcoutinho

Copy link
Copy Markdown
Owner

Two independent hardening fixes, both the same "one missing thing breaks
everything" shape.

1. Table.ownership failed the whole listing

server/tables.py splats each raw dict straight in:

tables = [Table(**t) for t in tables_data]

So a required ownership means a Tables app that stops emitting the key fails
the entire nc_tables_list_tables call — not one row, all of them. That is
precisely the failure mode owner_display_name had in #728, on the field
directly above it; only that one got fixed at the time.

Nothing in this codebase reads ownership (grep only finds an unrelated
build_ownership_filter in search/), so Optional[str] = None costs nothing.

TableSchema.columns / views get a default_factory for the same reason — an
empty table has no views, and a response omitting either list should degrade to
an empty list rather than failing.

2. Two providers inherited very long SDK default timeouts

Provider Before After
AnthropicProvider SDK default 600s 120s read / 5s connect
BedrockProvider botocore default 60s read × 3 retries 120s read / 5s connect
ollama / openai already 120/5 unchanged

Both defaults are long enough that a wedged endpoint reads as a hang rather
than a failure. The values are class constants so the tests assert against the
convention rather than a duplicated literal, and both constructors still accept
an explicit override.

Bedrock needed botocore.config.Config rather than an httpx timeout — retries
are left at botocore's default, since Bedrock throttling is expected and handled
upstream.

Deliberately not included

Mistral. providers/mistral.py has the same gap, but #864 already covers
it
— I found that PR while surveying and left it alone rather than duplicating
someone else's open work.

Test coverage

  • Table constructs with neither ownership nor owner_display_name
  • TableSchema constructs with neither columns nor views
  • new tests/unit/providers/test_provider_timeouts.py: each provider pins the
    convention values, and Anthropic honours an explicitly supplied timeout

Tiers executed locally: pytest -m unit ✅ 2713 passed (CI's exact selection).

Contract (Pact): not applicable — provider verification covers only
/api/v1/*, which imports neither models/tables.py nor providers/.

Provenance

Table.ownership was reported downstream on itsablabla/nextcloud-mcp-server
(they fixed the sibling field). That fork's author has not signed the CLA, so no
fork code was copied
— the gap was re-verified here and the fix written from
scratch.


This PR was generated with the help of AI, and reviewed by a Human

Two independent hardening fixes, both the "one missing thing breaks everything"
shape.

`Table.ownership` was required, and `server/tables.py` splats each raw dict
straight into `Table(**t)` — so a Tables app that stops emitting the key fails
the *entire* nc_tables_list_tables call, not just one row. Exactly the failure
mode `owner_display_name` had in #728, on the field right above it. Nothing in
this codebase reads `ownership`, so making it Optional costs nothing.
`TableSchema.columns`/`views` get a default_factory for the same reason: an empty
table has no views, and a response omitting either list should degrade to an
empty list rather than failing.

`AnthropicProvider` and `BedrockProvider` pinned no timeout, so they inherited
their SDK defaults: 600s for the Anthropic SDK, and 60s read with 3 retries for
botocore. Both are long enough that a wedged endpoint reads as a hang rather
than a failure. Both now use the 120s read / 5s connect the other providers
already use (ollama.py, openai.py), exposed as class constants so the tests
assert against the convention rather than a duplicated literal.

Mistral is deliberately untouched — #864 already covers it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sonarqubecloud

Copy link
Copy Markdown

@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown

Claude finished @cbcoutinho's task in 1m 49s —— View job


Claude finished reviewing

  • Fetch context and read changed files
  • Review models/tables.py changes
  • Review providers/anthropic.py changes
  • Review providers/bedrock.py changes
  • Review new/updated tests
  • Post final review

Note: Bash tool access was denied in this session (sandbox/permission restriction), so I could not execute ruff/ty/pytest myself. Review below is based on static reading of the diff and surrounding code. The PR description states pytest -m unit was run locally (2713 passed) and SonarCloud's Quality Gate has already passed on this PR.


Summary

Two small, well-isolated hardening fixes. Both are backward-compatible relaxations (required → optional / default-factory), not breaking changes, so no BREAKING CHANGE: footer is needed per the repo convention. I confirmed via grep -rn ownership nextcloud_mcp_server/ that nothing in this codebase reads Table.ownership, so the change is safe.

nextcloud_mcp_server/models/tables.py

  • ownership: Optional[str] = None and columns/views via default_factory=list correctly mirror the existing owner_display_name pattern from nc_tables_list_tables fails with Pydantic ValidationError: owner_display_name field required #728. Good, targeted fix for the "one absent key nukes the whole list" failure mode described in the PR body.
  • Minor caveat (not a bug in this PR, just worth being aware of): default_factory=list only kicks in when the key is absent from the payload — if the Tables API ever sent "columns": null explicitly rather than omitting the key, Pydantic would still raise, since the field type is List[TableColumn] rather than List[TableColumn] | None. That matches the actual observed failure mode (the app stops emitting the key), so I don't think this needs changing, just flagging for the record.

nextcloud_mcp_server/providers/anthropic.py

  • Clean: default timeout constructed only when caller doesn't supply one, constants extracted for the tests to assert against instead of duplicating literals. Matches ollama.py's existing convention (httpx.Timeout(timeout=120, connect=5)).

nextcloud_mcp_server/providers/bedrock.py

  • The botocore.config.Config(connect_timeout=..., read_timeout=...) approach is correct — httpx timeouts don't apply to a boto3 client, and retries are intentionally left at botocore's default per the inline comment.
  • One inconsistency vs. the PR description: the description says "both constructors still accept an explicit override," but BedrockProvider.__init__ (bedrock.py:39-46) has no config/timeout parameter — unlike AnthropicProvider, which added timeout: httpx.Timeout | None = None. Nothing in the repo currently needs to override Bedrock's timeout, so this isn't blocking, but it's worth either fixing the PR description or adding a matching config: BotoConfig | None = None param for symmetry (and to unblock a future caller without another PR).
  • Nit: DEFAULT_TIMEOUT_SECONDS/DEFAULT_CONNECT_TIMEOUT_SECONDS are float in anthropic.py (120.0/5.0) vs. int in bedrock.py (120/5). Harmless (botocore/httpx both accept either), just a small stylistic inconsistency between the two new constant pairs added in the same PR.
  • registry.py doesn't currently wire up AnthropicProvider at all (only Bedrock → Ollama → Simple auto-detection, confirmed via grep) — pre-existing, unrelated to this PR, just noting it's not a gap introduced here.

Tests

  • tests/unit/providers/test_provider_timeouts.py pins the convention via the class constants (good — avoids asserting a duplicated literal) and separately verifies the explicit-override path for Anthropic. The Bedrock test correctly guards with pytest.importorskip + BOTO3_AVAILABLE skip.
  • tests/unit/test_response_models.py additions directly cover the two "missing field" scenarios described in the PR body (Table without ownership, TableSchema without columns/views).
  • No end-to-end/contract coverage — correctly justified in the PR body ("provider verification covers only /api/v1/*, which imports neither models/tables.py nor providers/"), consistent with the CLAUDE.md e2e/contract gate for API-surface changes (these files aren't API surface).

Overall

No blocking issues found. The Bedrock constructor/description mismatch and the int/float constant nit are both minor — up to the author whether to address them in this PR or leave as-is.

Fix these →

@cbcoutinho
cbcoutinho merged commit 69cdc3b into master Jul 26, 2026
29 checks passed
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.

1 participant