-
Notifications
You must be signed in to change notification settings - Fork 25
.pr_agent_accepted_suggestions
| PR 60 (2026-04-08) |
[reliability] Missing sitemap gem dependency
Missing sitemap gem dependency
`_config.yml` declares the `jekyll-sitemap` plugin, but the gem isn’t present in `Gemfile`/`Gemfile.lock`, so Bundler won’t install it. The Pages build (`bundle exec jekyll build`) will fail when Jekyll tries to load an uninstalled plugin.jekyll-sitemap is configured as a plugin in _config.yml but is not included in Bundler dependencies, so the build will fail when Jekyll attempts to load the plugin.
The GitHub Pages workflow uses bundler-cache: true and runs bundle exec jekyll build, which requires all configured plugins to be available as installed gems.
- Gemfile[1-6]
- Gemfile.lock[170-175]
- _config.yml[23-27]
- Add the dependency:
gem "jekyll-sitemap"- Run
bundle installto updateGemfile.lock. - Ensure the
_config.ymlplugin entry is correctly indented as a peer list item.
[correctness] Broken plugins YAML list
Broken plugins YAML list
`jekyll-sitemap` is indented more than the other `plugins` items, which makes the YAML list invalid (or mis-parsed as a nested list) and can prevent Jekyll from loading `_config.yml`. This will break the Pages build when `bundle exec jekyll build` reads the config._config.yml has an incorrectly indented plugins list item (jekyll-sitemap), which can make the YAML invalid or mis-structured.
Jekyll reads _config.yml during bundle exec jekyll build (GitHub Pages workflow), so config parsing must be valid YAML.
- _config.yml[23-27]
Align the sitemap plugin with the other list items:
plugins:
- jekyll-seo-tag
- jekyll-relative-links
- jekyll-sitemap| PR 54 (2026-03-31) |
[reliability] Logging setup can crash
Logging setup can crash
`setup_logging()` runs at import time and unconditionally creates the directory and opens `LOG_FILE_PATH` via `logging.FileHandler()`, so an invalid/unwritable path raises and prevents the server from starting. This makes startup fragile in restricted environments or when `LOG_FILE_PATH` is accidentally set to an empty/invalid value.setup_logging() can crash the entire app during import if LOG_FILE_PATH is invalid, empty, or points to an unwritable directory because os.makedirs(...) / logging.FileHandler(...) exceptions are not handled.
This module initializes logging at import time (logger = setup_logging()), so any exception prevents the FastAPI app from importing and the server from starting.
-
Add validation/sanitization for
LOG_LEVEL/LOG_FILE_PATH(e.g.,.strip(), reject empty path) -
Use
os.makedirs(..., exist_ok=True) -
Wrap directory creation and
FileHandlercreation intry/except OSErrorand fall back to stdout-only logging (StreamHandler) if file logging cannot be initialized -
src/open_responses_server/common/config.py[29-57]
| PR 52 (2026-03-31) |
[reliability] Heartbeat task leak
Heartbeat task leak
`_with_heartbeat()` creates an `asyncio` Task per `__anext__()` but never cancels/awaits it or closes the wrapped async iterator when the downstream consumer stops (e.g., client disconnect), which can leak tasks and keep the upstream HTTP stream active. Over time this can exhaust resources and degrade streaming reliability._with_heartbeat() creates a Task (ensure_future(aiter.__anext__())) but does not cancel/await it or close the underlying async iterator if the consumer disconnects or stops iterating early. This can leave pending tasks and keep the upstream stream active.
This wrapper is used for the /responses SSE streaming path, so it will run under client disconnects/cancellations.
- src/open_responses_server/api_controller.py[16-33]
- src/open_responses_server/api_controller.py[285-293]
- Wrap the main loop in
try/finally. - Track the current
taskand infinallycancel it if pending andawaitit (suppressingCancelledError). - Also
await aiter.aclose()(orasync_gen.aclose()) infinallyto release upstream resources. - Consider switching to
asyncio.create_task()(modern) and guarding against double-cancel.
[correctness] Trailing whitespace in `config.py`
Trailing whitespace in `config.py`
`src/open_responses_server/common/config.py` contains trailing whitespace (and appears to be missing a final newline), which will cause `flake8 src/` to fail (e.g., W291/W292). This breaks the PR requirement that `src/` passes flake8 linting.src/open_responses_server/common/config.py has trailing whitespace at the end of the final logger.info(...) line and the file appears to be missing a terminating newline. This commonly triggers flake8 W291 (trailing whitespace) and W292 (no newline at end of file).
PR compliance requires flake8 src/ to pass cleanly.
- src/open_responses_server/common/config.py[61-63]
[performance] Zero interval spins loop
Zero interval spins loop
If `HEARTBEAT_INTERVAL` is set to `0` or negative, `_with_heartbeat()` will repeatedly call `asyncio.wait(..., timeout=interval)` with a non-positive timeout and yield heartbeats in a tight loop, causing high CPU and flooding SSE output. This is currently possible because the interval is read from env without any validation.A non-positive HEARTBEAT_INTERVAL can cause _with_heartbeat() to spin and emit heartbeats continuously.
HEARTBEAT_INTERVAL is env-configurable and used as the timeout parameter to asyncio.wait().
- src/open_responses_server/common/config.py[25-28]
- src/open_responses_server/api_controller.py[16-33]
- Validate/clamp
HEARTBEAT_INTERVALat config load (e.g., if<= 0, disable heartbeats). - In
create_response, if heartbeats are disabled, skip wrapping with_with_heartbeat()and iterate the underlying generator directly. - Optionally add a reasonable minimum (e.g.,
max(interval, 0.5)) to prevent accidental too-small values.
| PR 51 (2026-03-30) |
[reliability] Publish runs without release
Publish runs without release
In publish.yml, `create-release` can determine a GitHub Release already exists and then perform no release/tag creation, but `release-build` and `pypi-publish` still run and attempt to publish anyway. This makes the workflow non-idempotent and can fail on reruns or on changes to version.py that don’t introduce a new version/release.release-build and pypi-publish run even when create-release finds the release already exists and does not create a new release/tag. This makes the workflow non-idempotent and can cause duplicate publish attempts.
create-release only conditionally runs the steps that create a release, but the job still completes successfully when the release already exists. Downstream jobs don’t know whether a release was created.
- .github/workflows/publish.yml[21-70]
- .github/workflows/publish.yml[67-113]
- Add a job-level output from
create-release(e.g.,release_created) derived fromsteps.check.outputs.exists. - Gate
release-build/pypi-publishwith anif:such as:github.event_name == 'release' || needs.create-release.outputs.release_created == 'true'
- Optionally, for
workflow_dispatch, require an explicit input likeforce_publishto allow republishing behavior intentionally.
| PR 49 (2026-03-30) |
[reliability] Unpinned Python in publish
Unpinned Python in publish
`.github/workflows/publish.yml` no longer sets up a Python version even though the package declares `requires-python = ">=3.12"`, so `uv build`/`uvx twine check` can fail if the runner default Python is older. This creates a fragile release pipeline that can start failing without code changes when GitHub runner images update.The publish workflow no longer pins a Python version, but the project requires Python >= 3.12. This can break uv build and uvx twine check when GitHub runner default Python versions change.
Other workflows in this repo pin Python 3.12, and pyproject.toml sets requires-python = ">=3.12".
- .github/workflows/publish.yml[15-26]
Add a actions/setup-python@v5 step (python-version: '3.12') before running uv build/uvx, or configure astral-sh/setup-uv@v4 to provision/use Python 3.12 if supported.
| PR 48 (2026-03-30) |
[reliability] Streaming error NameError locked-in
Streaming error NameError locked-in
A newly added test explicitly expects `NameError` when consuming a streaming error response, which effectively codifies a real production bug where `_handle_streaming_request()`’s `error_stream` generator closes over the `except` variable `e` after the block ends. In production, this can crash streaming error responses instead of returning an SSE error payload.src/open_responses_server/chat_completions_service.py defines error_stream() inside except Exception as e: and references e inside the generator. Because generators are lazy, the yield executes after the except block has exited, which can cause NameError and crash the response stream.
A newly added unit test currently expects this crash, which will lock in broken runtime behavior.
- src/open_responses_server/chat_completions_service.py[183-187]
- tests/test_chat_completions_service.py[526-549]
- Capture the error message before defining the generator (e.g.,
err = str(e)), and have the generator close overerr(or pass it as a default argument). - Update the test to assert the SSE error payload is yielded (and no
NameErroroccurs), instead of expectingNameError.
[maintainability] Omitted runtime entrypoint coverage
Omitted runtime entrypoint coverage
Coverage config omits `server_entrypoint.py`, but the CLI starts uvicorn using `open_responses_server.server_entrypoint:app`, so the omission contradicts the PR’s claim that omitted files are “not imported at runtime” and can inflate coverage metrics. This makes the new fail-under threshold less meaningful.server_entrypoint.py is omitted from coverage even though it is referenced by the runtime CLI entrypoint. This contradicts the PR description’s rationale and can make coverage gating misleading.
server_entrypoint.py may be thin, but it is invoked via uvicorn.run("open_responses_server.server_entrypoint:app", ...).
- pyproject.toml[60-67]
- src/open_responses_server/cli.py[24-43]
Choose one:
- Remove
server_entrypoint.pyfromomit(preferred if you want accurate “runtime code” coverage), or - Keep it omitted but update the PR description (and/or add a brief comment near the omit list) to accurately state it’s a thin entrypoint wrapper rather than “not imported at runtime.”
| PR 47 (2026-03-29) |
[maintainability] `_get_session_id` assigned unused
`_get_session_id` assigned unused
`_get_session_id` is assigned but never used in `MCPServer.initialize()`, which will trigger Flake8 `F841` and violate the requirement that `flake8 src/` passes cleanly._get_session_id is assigned but unused in MCPServer.initialize(), which is a Flake8 F841 violation.
This PR adds support for streamable-http transport and unpacks a 3-tuple return, but the third value is not used.
- src/open_responses_server/common/mcp_manager.py[59-61]
[correctness] Stdio missing command TypeError
Stdio missing command TypeError
In `MCPServer.initialize()` the stdio branch calls `shutil.which(self.config.get('command'))` without validating that `command` exists, so a stdio config missing `command` will raise `TypeError` before the intended `ValueError` path. The same bug exists in the legacy duplicate `src/open_responses_server/server.py` MCPServer implementation.MCPServer.initialize() can still crash with TypeError when a stdio config is missing the command key because shutil.which() is invoked with None.
Configs are loaded from JSON and passed through without validation; invalid/missing fields should produce a controlled ValueError with a clear message.
- src/open_responses_server/common/mcp_manager.py[31-42]
- src/open_responses_server/server.py[128-139]
- tests/test_mcp_transport.py[40-50]
- In the
stdiobranch, fetchcmd = self.config.get('command')and if falsy, raiseValueError(e.g., "type 'stdio' requires a 'command'") before callingshutil.which. - Then resolve
npxviacmd_to_find = 'npx' if cmd == 'npx' else cmd. - Update/add a unit test for config
{}(or{ "type": "stdio" }) to assert aValueErrorrather than aTypeError.
[security] Headers leaked via config logging
Headers leaked via config logging
`MCPManager.startup_mcp_servers()` logs the entire server config dict at INFO; with this PR adding `headers` support for HTTP transports, secrets like `Authorization` tokens will be written to logs. This can expose credentials in log aggregation and crash reports.startup_mcp_servers() logs full MCP server configs, which now may include sensitive headers (e.g., Authorization tokens). These secrets will be persisted in logs.
This risk becomes much more likely now that the code explicitly supports a headers field for HTTP-based transports.
- src/open_responses_server/common/mcp_manager.py[124-138]
- Avoid logging the full config dict; log only safe fields (e.g., name, type, command, url) and omit
headers/env. - If you must log the dict, create a sanitized copy that replaces header values with
"<redacted>"(or only logs header keys). - Consider also redacting/omitting URLs if they may contain credentials in query strings.
| PR 46 (2026-03-29) |
[correctness] Bash -e breaks reporting
Bash -e breaks reporting
`.claude/skills/markdownlint/scripts/lint-docs.sh` enables `set -e` and then tries to capture `markdownlint-cli2`’s exit code; on any lint failure the script exits immediately at the `markdownlint-cli2` call and never reaches the `EXIT_CODE=$?` handling. This breaks the script’s intended output/exit-path and makes failures harder to diagnose.lint-docs.sh uses set -e but also tries to inspect markdownlint-cli2’s exit code after running it. On any non-zero return code, set -e will terminate the script before the EXIT_CODE=$? branch, so the summary messaging and explicit exit are skipped.
This script is intended to report whether linting passed vs failed and exit with the appropriate status code.
- .claude/skills/markdownlint/scripts/lint-docs.sh[11-64]
Wrap the lint invocation in an if ...; then ... else ... fi block (or temporarily disable set -e around the command) so you can reliably capture the exit code and print the intended messages, e.g.:
if markdownlint-cli2 ...; then echo "All files pass."; exit 0; else code=$?; echo "Found lint issues..."; exit $code; fi