Skip to content

Conversation

@pdeziel
Copy link
Contributor

@pdeziel pdeziel commented Sep 30, 2025

Scope of changes

This fixes a bug in the preflight auth logic to ensure that the credentials are actually being supplied in the header after an endpoint requires preflight authentication. Previously, the self._request_headers was being overwritten since the preflight function is actually called twice for the first request, so this changes it to a local variable.

Type of change

  • new feature
  • bug fix
  • documentation
  • testing
  • technical debt
  • other (describe)

Acceptance criteria

[Copy acceptance criteria checklist from story for reviewer, or add a brief acceptance criteria here]

Definition of Done

  • I have manually tested the change running it locally (having rebuilt all containers) or via unit tests
  • I have added unit and/or integration tests that cover my changes
  • I have added new test fixtures as needed to support added tests
  • I have updated the dependencies list if necessary (including updating yarn.lock and/or go.sum)
  • Check this box if a reviewer can merge this pull request after approval (leave it unchecked if you want to do it yourself)
  • I have notified the reviewer via Shortcut or Slack that this is ready for review

Reviewer(s) checklist

  • Any new user-facing content that has been added for this PR has been QA'ed to ensure correct grammar, spelling, and understandability.
  • Are there any TODOs in this PR that should be turned into stories?

Copilot AI review requested due to automatic review settings September 30, 2025 19:56
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a bug in the preflight authentication logic where credentials were not being properly supplied in request headers. The issue occurred because the _pre_flight method was being called twice for the first request, causing self._request_headers to be overwritten.

  • Changed _pre_flight method to return headers as a local variable instead of modifying instance state
  • Updated all HTTP methods (GET, POST, PUT, DELETE) to use the returned headers directly
  • Added comprehensive test coverage for the preflight authentication functionality

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
guidelight/client.py Modified _pre_flight method to return headers locally and updated all HTTP methods to use returned headers
tests/test_client.py Added new test file with preflight authentication test case
tests/requirements.txt Added pytest-httpserver dependency for testing HTTP interactions

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link
Contributor

@bbengfort bbengfort left a comment

Choose a reason for hiding this comment

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

Good catch!

@bbengfort bbengfort merged commit e9d9898 into main Sep 30, 2025
3 checks passed
@bbengfort bbengfort deleted the fix-preflight branch September 30, 2025 20:04
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.

3 participants