Skip to content

chore(query-runs): clarify user_public_id documentation - #153

Closed
hotdata-automation[bot] wants to merge 1 commit into
mainfrom
openapi-update-30506453699
Closed

chore(query-runs): clarify user_public_id documentation#153
hotdata-automation[bot] wants to merge 1 commit into
mainfrom
openapi-update-30506453699

Conversation

@hotdata-automation

Copy link
Copy Markdown
Contributor

Auto-generated from the updated HotData OpenAPI spec.
Source: https://github.com/hotdata-dev/www.hotdata.dev/pull/289

@hotdata-automation
hotdata-automation Bot requested a review from a team as a code owner July 30, 2026 01:44
@hotdata-automation
hotdata-automation Bot requested review from rohan-hotdata and removed request for a team July 30, 2026 01:44
@hotdata-automation
hotdata-automation Bot enabled auto-merge (squash) July 30, 2026 01:44
Comment thread hotdata/configuration.py
'SessionId',
),
}
return auth

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Blocking: dropping the SessionId scheme here turns the hand-maintained session_id kwarg/property into a silent no-op.

hotdata/configuration.py still ships (from .openapi-generator-templates/configuration.mustache, which is not regenerated by this PR):

  • line 145 docstring: :param session_id: Public id of an active sandbox, sent as `X-Session-Id`.
  • lines 240–241: if session_id is not None: self.api_keys["SessionId"] = session_id
  • lines 543–557: the session_id property/setter, documented as "sent as X-Session-Id"

With SessionId gone from auth_settings() and from every operation's _auth_settings list, Configuration(session_id="sb_xyz") still accepts the value and config.session_id still reads it back, but no X-Session-Id header is ever sent. Callers relying on sandbox scoping lose isolation with no error and no type failure — reads/writes silently hit unscoped data. The AuthSettings TypedDict no longer declares the key either, so nothing catches it statically.

Pick one:

  1. If SessionId was intentionally removed from the spec, also remove session_id (kwarg, api_keys wiring, property, docstrings, the :Example: block at line 180) from .openapi-generator-templates/configuration.mustache and regenerate, and record the removal under ### Removed in CHANGELOG.md.
  2. If the removal is unintentional spec drift, restore the SessionId security scheme in the OpenAPI source and regenerate.

Either way this should not merge as-is, since the current state is "documented parameter that quietly does nothing."

Comment thread CHANGELOG.md

### Changed

- chore(query-runs): clarify user_public_id documentation

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

nit: the changelog entry only covers the doc-string change, but this PR also removes the SessionId / X-Session-Id API-key auth scheme from Configuration.auth_settings() and from the query, get_result, and list_results operations. That is a user-facing breaking change and warrants its own entry under ### Removed, alongside the existing uploads note. (not blocking on its own — but it needs to happen as part of resolving the session_id issue in hotdata/configuration.py)

@claude claude 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.

Review

Blocking Issues

  • hotdata/configuration.py:601auth_settings() no longer emits the SessionId / X-Session-Id scheme, and no operation lists it in _auth_settings anymore, but the hand-maintained template still exposes session_id:

    • hotdata/configuration.py:145 documents :param session_id: ... sent as X-Session-Id
    • hotdata/configuration.py:240-241 writes it into self.api_keys["SessionId"]
    • hotdata/configuration.py:543-557 exposes the session_id property/setter

    Result: Configuration(session_id="sb_xyz") is accepted and round-trips through config.session_id, but the header is never sent. Sandbox scoping silently degrades to unscoped access — no exception, and no static error since AuthSettings no longer declares the key. The source of the stale code is .openapi-generator-templates/configuration.mustache:337,669-676, which this PR does not touch.

Action Required

  1. Decide whether the SessionId scheme removal is intentional:
    • Intentional → remove session_id (kwarg, api_keys wiring, property/setter, docstrings at lines 145-149, and the :Example: block at line 180) from .openapi-generator-templates/configuration.mustache and regenerate. Also drop the now-dangling SandboxesApi.create_sandbox references in those docstrings — there is no SandboxesApi in this SDK.
    • Unintentional spec drift → restore the SessionId security scheme in the OpenAPI source and regenerate.
  2. Add a ### Removed entry to CHANGELOG.md for the dropped SessionId / X-Session-Id auth scheme. The current entry only mentions the user_public_id doc change, which understates a breaking change.

The user_public_id description update itself (hotdata/models/query_run_info.py:48, docs/QueryRunInfo.md) is fine and matches the PR intent.

@zfarrell

Copy link
Copy Markdown
Contributor

Closing in favor of #154, a fresh regen on top of main.

This PR branched from 5526588 and was generated with the pre-#152 configuration template, so its blocking review (the session_id no-op) was already fixed on main by the time it landed — #152 merged at 01:52:54, six minutes after the review at 01:46:12. The CHANGES_REQUESTED state is stale, and the branch can't clear it without a rebase.

#154 is generated from current main (bbd4c52) with the fixed template and reduces to just the genuine change: CHANGELOG.md, docs/QueryRunInfo.md, hotdata/models/query_run_info.py.

@zfarrell zfarrell closed this Jul 30, 2026
auto-merge was automatically disabled July 30, 2026 02:04

Pull request was closed

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