Skip to content

fix: preserve API prefix in session Location header (fixes #247)#248

Open
Flamki wants to merge 2 commits intojenkinsci:mainfrom
Flamki:fix/session-location-header
Open

fix: preserve API prefix in session Location header (fixes #247)#248
Flamki wants to merge 2 commits intojenkinsci:mainfrom
Flamki:fix/session-location-header

Conversation

@Flamki
Copy link
Contributor

@Flamki Flamki commented Mar 5, 2026

Fixes #247.

Description

start_chat was setting Location via a hardcoded path (/sessions/{id}/message), which ignores the API router prefix when mounted (default: /api/chatbot).

This PR switches Location generation to route resolution so the header always matches mounted routes.

Changes

  • chatbot-core/api/routes/chatbot.py
    • add Request parameter to start_chat
    • replace hardcoded Location with:
      • request.url_for("chatbot_reply", session_id=session_id).path
  • chatbot-core/tests/unit/routes/test_chatbot.py
    • add regression test test_start_chat_location_includes_router_prefix
    • keep existing non-prefixed behavior test

Why this is aligned / scoped

  • Small backend correctness fix
  • No architecture or auth design changes
  • Improves API contract reliability for clients following Location

Testing done

  • pytest tests/unit/routes/test_chatbot.py -q -> 7 passed
  • pylint api/routes/chatbot.py tests/unit/routes/test_chatbot.py -> 10.00/10

Submitter checklist

  • Topic branch used
  • PR title represents intended changelog entry
  • Changes described
  • Linked issue
  • Added regression test

@Flamki Flamki requested a review from a team as a code owner March 5, 2026 09:55
@@ -11,6 +17,20 @@ def test_start_chat(client, mock_init_session):
assert response.headers["location"] == "/sessions/test-session-id/message"
Copy link
Contributor

Choose a reason for hiding this comment

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

You should change this line, instead of creating a new test

from api.routes.chatbot import router


def test_start_chat(client, mock_init_session):
Copy link
Contributor

Choose a reason for hiding this comment

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

response.headers["Location"] = (
f"/sessions/{session_id}/message"
)
response.headers["Location"] = request.url_for(
Copy link
Contributor

Choose a reason for hiding this comment

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

In my mind, I thought Location would be used by UI to navigate the page.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Location

Content-Location may be more appropriate

https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Content-Location

@berviantoleo berviantoleo added the enhancement For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted label Mar 6, 2026
@sharma-sugurthi
Copy link
Contributor

Location on a 201 should point to the created resource itself (/sessions/{id}), not the action endpoint (/sessions/{id}/message). Content-Location fits better here since it's telling the client where to send messages next rather than identifying the resource itself. @Flamki ,its just suggestions...But nice fix bro..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] start_chat Location header ignores API router prefix

3 participants