Skip to content

Fix three bugs in step_manager (missing return, wrong error text, header array shape) - #28

Closed
benjithompson wants to merge 3 commits into
Runscope:masterfrom
benjithompson:fix/step-manager-bugs
Closed

benjithompson wants to merge 3 commits into
Runscope:masterfrom
benjithompson:fix/step-manager-bugs

Conversation

@benjithompson

Copy link
Copy Markdown
Contributor

Three small, independent fixes to StepManager (src/tools/step_manager.py), each with a
regression test.

1. Missing return for unsupported body_type in add_body_to_step

The unsupported-body_type arm constructed a BaseResult(error=...) but never returned it, so
execution fell through and PUT the step with an empty body and no Content-Type header instead of
surfacing the validation error. Added the return.

2. Copy-pasted error message in add_assertion_to_step

The non-request-step guard reported "...cannot have a body added" when rejecting an assertion.
Corrected the message to refer to an assertion.

3. Emit Content-Type header values as arrays

The REST API documents header values as arrays of strings
(e.g. {"Content-Type": ["application/json"]}); the code sent bare strings and relied on
undocumented server leniency. Now emits arrays for all body types (json/xml/html/text).

Testing

pytest tests/test_step_manager.py — 11 passed. flake8 / black / isort clean.

benjithompson and others added 3 commits July 8, 2026 13:19
An unsupported body_type constructed the error BaseResult but did not
return it, so execution fell through and PUT the step with an empty
body and no Content-Type header.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S2Ym1SshabyXjWNdeGFDfx
The non-request-step guard reported "cannot have a body added" when
rejecting an assertion.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S2Ym1SshabyXjWNdeGFDfx
The REST API documents header values as arrays of strings
(e.g. {"Content-Type": ["application/json"]}); bare strings relied
on undocumented server leniency.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S2Ym1SshabyXjWNdeGFDfx
@codecov-commenter

Copy link
Copy Markdown

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

Copilot AI 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.

Pull request overview

This PR tightens StepManager request-step mutation behavior in src/tools/step_manager.py and adds regression coverage in tests/test_step_manager.py, addressing validation fall-through, a misleading error message, and request header payload shape to better match the documented REST contract.

Changes:

  • Fixes a missing return so unsupported body_type errors short-circuit before any API calls.
  • Corrects a copy-pasted error message when adding assertions to non-request steps.
  • Changes emitted Content-Type header values to arrays of strings (e.g. ["application/json"]) when adding a body.

Reviewed changes

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

File Description
src/tools/step_manager.py Returns early on unsupported body_type, fixes assertion guard error text, and emits Content-Type header values as arrays.
tests/test_step_manager.py Adds regression tests for unsupported body_type, non-request assertion guard messaging, and verifies Content-Type header array shape (JSON case).
Comments suppressed due to low confidence (1)

src/tools/step_manager.py:161

  • The register() tool description for add_assertion_to_step claims assertions can be added to non-request step types (Ghost Inspector/subtest/conditional), but the implementation hard-rejects anything except step_type == 'request'. Please either broaden the implementation to support the documented step types, or update the description to reflect the actual restriction.
        request_result = await self.read(bucket_key, test_id, step_id, result_formatter=None)
        if not request_result or request_result.get("step_type") != "request":
            return BaseResult(
                error=f"Step {step_id} is not a request step and cannot have an assertion added."
            )

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/tools/step_manager.py
except json.JSONDecodeError as e:
return BaseResult(error=f"Invalid JSON content provided for body_content: {str(e)}")
request_headers["Content-Type"] = "application/json"
request_headers["Content-Type"] = ["application/json"]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Comment thread src/tools/step_manager.py
Comment on lines 93 to +104
@@ -101,7 +101,7 @@ async def add_body_to_step(
return BaseResult(error=f"Invalid XML content provided for body_content: {str(e)}")
except Exception as e:
return BaseResult(error=f"Error processing XML content: {str(e)}")
request_headers["Content-Type"] = "application/xml"
request_headers["Content-Type"] = ["application/xml"]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@pjain05

pjain05 commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

@copilot resolve the merge conflicts in this pull request

@pjain05

pjain05 commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Since this PR was from a forked repo, creating a similar PR #33 and closing this one.

@pjain05 pjain05 closed this Aug 12, 2026
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.

4 participants