Productize Samsarix Agent Engine and add its Samsarix roadmap - #1
Merged
Conversation
Summary by CodeRabbit
WalkthroughThe project is reorganized as the Samsarix Agent Engine, adding a bounded Python SDK, CLI, deterministic and OpenAI-compatible providers, package metadata, CI and release automation, comprehensive tests, and updated documentation, licensing, security, and contribution guidance. ChangesAgent Engine Productization
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant LLMAgentEngine
participant Agent
participant OpenAICompatibleProvider
participant Endpoint
User->>CLI: run prompt
CLI->>LLMAgentEngine: create bounded agent
LLMAgentEngine->>Agent: invoke prompt
Agent->>OpenAICompatibleProvider: invoke chat messages
OpenAICompatibleProvider->>Endpoint: POST chat/completions
Endpoint-->>OpenAICompatibleProvider: response or retryable error
OpenAICompatibleProvider-->>Agent: ProviderResponse
Agent-->>CLI: content and metrics
CLI-->>User: text or JSON output
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/PRODUCTIZATION.md`:
- Line 91: Correct the documented baseline command in PRODUCTIZATION.md so it
records the actual separate setup.py invocations rather than chaining options
with shell operators. Preserve the reported package name, version, and
classifier findings while showing each executable command explicitly.
In `@docs/RELEASING.md`:
- Around line 46-47: Update the release content requirements in
docs/RELEASING.md to distinguish wheel and sdist validation: require wheels to
contain only the supported package, documentation, and license files, while
allowing the sdist-required CITATION.cff, licensing documentation,
docs/GETTING_STARTED.md, and examples/basic_agent.py. Continue explicitly
forbidding agents/ and services/ in both artifacts.
In `@examples/custom_llm_provider.py`:
- Around line 12-25: Update CustomLLMProvider.invoke to explicitly detect when
messages contains no user-role message and raise ProviderError, matching the
provider contract used by EchoProvider. Preserve the existing prompt selection
and deterministic response behavior when a user message is present.
In `@LICENSING.md`:
- Around line 30-37: Update LICENSING.md lines 30-37 and README.md lines 188-192
to avoid asserting settled MPL-2.0 status until ROADMAP.md’s MPL/provenance
approval and license transition/provenance items are resolved. Apply consistent
wording at both sites stating that provenance, contributor relicensing consent,
and release evidence must be recorded before confirming the MPL-2.0 status.
In `@MANIFEST.in`:
- Around line 10-11: Extend the legacy-package exclusion across source
distributions: add helix_llm_agent_engine to the prune entries in MANIFEST.in,
and add it to the forbidden sdist directory checks in .github/workflows/ci.yml
(lines 103-107) and .github/workflows/release.yml (lines 82-86).
In `@pyproject.toml`:
- Line 26: Update the dependency declarations in pyproject.toml at lines 26-26
and requirements.txt at lines 3-3 to add the direct constraint idna>=3.15
alongside the existing dependencies, ensuring both runtime and requirements
installation paths avoid vulnerable transitive versions.
In `@SECURITY.md`:
- Around line 25-26: Update the network-request guarantee in SECURITY.md to
apply specifically to OpenAICompatibleProvider rather than every provider. State
that custom application-defined providers must document and enforce their own
timeout, retry-count, response-size, cancellation, and redirect controls.
In `@src/samsarix_agent_engine/cli.py`:
- Around line 47-51: Extend argparse lower-bound validation in the run options
alongside max-input-chars so timeout, max-retries, max-output-tokens, and
max-response-chars reject negative values before reaching LLMAgentEngine or
OpenAICompatibleProvider. Update main()’s existing error-handling block to map
unexpected Exception instances to the same clean CLI error output while
preserving the current handling for explicitly supported errors.
- Line 49: Update the --max-input-chars argument definition in the CLI parser to
use a validating non-negative integer converter that raises ArgumentTypeError
mentioning max_input_chars for values below zero. Ensure argparse performs this
validation before the command reads stdin, while preserving the existing default
and valid integer behavior.
In `@src/samsarix_agent_engine/providers.py`:
- Around line 149-185: The retry loop in the provider request method must also
retry timeout and connection failures raised by _read_bounded_json during
response consumption. Extend the existing httpx.TimeoutException and
httpx.RequestError handling to cover the read phase, preserving response cleanup
and the configured retry budget before raising the existing sanitized
ProviderError results.
- Around line 149-185: The trailing ProviderError after the retry loop in the
request flow is unreachable because every iteration continues, returns, or
raises. Remove the redundant raise following the loop, or add a brief comment
explicitly documenting it as defensive future-proofing if it must remain.
- Around line 79-113: Update the constructor’s client setup so headers built
from api_key, including Authorization, are applied when a custom client is
supplied as well as when httpx.AsyncClient is created internally. Preserve the
existing validation, ownership tracking, and default client options, while
ensuring api_key is never silently ignored.
- Around line 116-131: Update _validate_endpoint to reject loopback, private,
link-local, and other internal IP-literal hosts after parsing the URL and before
normalizing or appending /chat/completions. Preserve the existing scheme,
credential, query, fragment, and path validation for public hostnames, and raise
ConfigurationError for disallowed addresses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 534d3fe9-0474-4717-8efe-29e153a93af7
📒 Files selected for processing (53)
.env.example.github/workflows/ci.yml.github/workflows/release.ymlCHANGELOG.mdCITATION.cffCODE_OF_CONDUCT.mdCONTRIBUTING.mdLICENSELICENSE.PROPRIETARYLICENSING.mdMANIFEST.inNOTICEPYPI_PUBLISHING_GUIDE.mdREADME.mdROADMAP.mdSECURITY.mdTRADEMARKS.mddocs/GETTING_STARTED.mddocs/LEGACY_CODE.mddocs/PRODUCTIZATION.mddocs/RELEASING.mdexamples/basic_agent.pyexamples/consciousness_optimization.pyexamples/custom_llm_provider.pyexamples/error_handling.pyexamples/multi_agent_collaboration.pyexamples/streaming_responses.pyinference_client.pyllm_agent_engine.pyllm_config.pyllm_gateway.pypyproject.tomlpytest.inirequirements-test.txtrequirements.txtsetup.pysrc/samsarix_agent_engine/__init__.pysrc/samsarix_agent_engine/__main__.pysrc/samsarix_agent_engine/cli.pysrc/samsarix_agent_engine/engine.pysrc/samsarix_agent_engine/errors.pysrc/samsarix_agent_engine/models.pysrc/samsarix_agent_engine/providers.pysrc/samsarix_agent_engine/py.typedtests/conftest.pytests/test_agents.pytests/test_cli.pytests/test_communication.pytests/test_coordination.pytests/test_engine.pytests/test_models.pytests/test_performance.pytests/test_providers.py
💤 Files with no reviewable changes (15)
- pytest.ini
- PYPI_PUBLISHING_GUIDE.md
- tests/test_agents.py
- LICENSE.PROPRIETARY
- examples/streaming_responses.py
- tests/test_coordination.py
- llm_gateway.py
- tests/test_communication.py
- setup.py
- tests/test_performance.py
- llm_config.py
- llm_agent_engine.py
- examples/consciousness_optimization.py
- tests/conftest.py
- inference_client.py
| | `git status --short --branch` | Clean `main...origin/main`. | | ||
| | `python -m pytest` | Exit 0; 35 passed in 7.88s. Every test exercised fixtures or `MagicMock`, not implementation. | | ||
| | `python -m compileall -q .` | Exit 0. Syntax only. | | ||
| | `python setup.py --name && --version && check` | Reported `helix-llm-agent-engine` 1.0.0 with a deprecated false MIT classifier. | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Record an executable baseline command.
python setup.py --name && --version && check runs the first command, then attempts to execute --version and check as separate shell commands. Record the actual invocations.
Suggested correction
-python setup.py --name && --version && check
+python setup.py --name && python setup.py --version && python setup.py check📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| | `python setup.py --name && --version && check` | Reported `helix-llm-agent-engine` 1.0.0 with a deprecated false MIT classifier. | | |
| | `python setup.py --name && python setup.py --version && python setup.py check` | Reported `helix-llm-agent-engine` 1.0.0 with a deprecated false MIT classifier. | |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/PRODUCTIZATION.md` at line 91, Correct the documented baseline command
in PRODUCTIZATION.md so it records the actual separate setup.py invocations
rather than chaining options with shell operators. Preserve the reported package
name, version, and classifier findings while showing each executable command
explicitly.
Comment on lines
+46
to
+47
| Inspect both wheel and sdist contents. They must include only the supported package, | ||
| documentation, and license files; `agents/` and `services/` must be absent. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Separate wheel and sdist content requirements.
The supplied release workflow and docs/PRODUCTIZATION.md require the sdist to include CITATION.cff, licensing documentation, docs/GETTING_STARTED.md, and examples/basic_agent.py. This wording could reject a valid sdist or encourage stripping required files. State separate rules for wheel contents and sdist contents, while continuing to forbid agents/ and services/.
🧰 Tools
🪛 LanguageTool
[grammar] ~46-~46: Ensure spelling is correct
Context: ...gentEngine" ``` Inspect both wheel and sdist contents. They must include only the su...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/RELEASING.md` around lines 46 - 47, Update the release content
requirements in docs/RELEASING.md to distinguish wheel and sdist validation:
require wheels to contain only the supported package, documentation, and license
files, while allowing the sdist-required CITATION.cff, licensing documentation,
docs/GETTING_STARTED.md, and examples/basic_agent.py. Continue explicitly
forbidding agents/ and services/ in both artifacts.
Comment on lines
12
to
+25
| class CustomLLMProvider(BaseLLMProvider): | ||
| """Example custom LLM provider""" | ||
|
|
||
| async def invoke(self, messages, model, **kwargs): | ||
| """Custom inference logic""" | ||
| # Implement your custom LLM logic here | ||
| return "Response from custom provider" | ||
| """Small deterministic provider used only by this example.""" | ||
|
|
||
| async def invoke( | ||
| self, | ||
| messages: Sequence[ChatMessage], | ||
| model: str, | ||
| *, | ||
| max_tokens: int, | ||
| temperature: float, | ||
| ) -> ProviderResponse: | ||
| del max_tokens, temperature | ||
| prompt = next(message.content for message in reversed(messages) if message.role == "user") | ||
| return ProviderResponse(content=f"{model} handled: {prompt}", model=model) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Missing-user-message path raises RuntimeError, not the documented ProviderError contract.
next(... if message.role == "user") with no default will raise StopIteration if no user message is present; inside this async def, that surfaces as RuntimeError (PEP 479) rather than the ProviderError contract other providers use (see EchoProvider's explicit raise ProviderError in tests/test_providers.py:20-31, matched by async def test_echo_provider_is_explicitly_deterministic() -> None: provider = EchoProvider(prefix="Offline") response = await provider.invoke(_messages(), "echo", max_tokens=10, temperature=0) and its follow-up assertion that a system-only message list raises ProviderError). As example code meant to teach the extension point, it should model the same contract.
♻️ Proposed fix
from samsarix_agent_engine.models import ChatMessage, ProviderResponse
from samsarix_agent_engine.providers import BaseLLMProvider
+from samsarix_agent_engine.providers import ProviderError
@@
- prompt = next(message.content for message in reversed(messages) if message.role == "user")
+ try:
+ prompt = next(
+ message.content for message in reversed(messages) if message.role == "user"
+ )
+ except StopIteration:
+ raise ProviderError("no user message found") from None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| class CustomLLMProvider(BaseLLMProvider): | |
| """Example custom LLM provider""" | |
| async def invoke(self, messages, model, **kwargs): | |
| """Custom inference logic""" | |
| # Implement your custom LLM logic here | |
| return "Response from custom provider" | |
| """Small deterministic provider used only by this example.""" | |
| async def invoke( | |
| self, | |
| messages: Sequence[ChatMessage], | |
| model: str, | |
| *, | |
| max_tokens: int, | |
| temperature: float, | |
| ) -> ProviderResponse: | |
| del max_tokens, temperature | |
| prompt = next(message.content for message in reversed(messages) if message.role == "user") | |
| return ProviderResponse(content=f"{model} handled: {prompt}", model=model) | |
| from samsarix_agent_engine.providers import BaseLLMProvider | |
| from samsarix_agent_engine.providers import ProviderError | |
| class CustomLLMProvider(BaseLLMProvider): | |
| """Small deterministic provider used only by this example.""" | |
| async def invoke( | |
| self, | |
| messages: Sequence[ChatMessage], | |
| model: str, | |
| *, | |
| max_tokens: int, | |
| temperature: float, | |
| ) -> ProviderResponse: | |
| del max_tokens, temperature | |
| try: | |
| prompt = next( | |
| message.content for message in reversed(messages) if message.role == "user" | |
| ) | |
| except StopIteration: | |
| raise ProviderError("no user message found") from None | |
| return ProviderResponse(content=f"{model} handled: {prompt}", model=model) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/custom_llm_provider.py` around lines 12 - 25, Update
CustomLLMProvider.invoke to explicitly detect when messages contains no
user-role message and raise ProviderError, matching the provider contract used
by EchoProvider. Preserve the existing prompt selection and deterministic
response behavior when a user message is present.
Comment on lines
+30
to
+37
| Changing the license for the current tree does not revoke permissions already | ||
| granted for copies of historical revisions under the license files distributed | ||
| with those revisions. The current source tree and the `0.1.0` release candidate | ||
| use MPL-2.0. | ||
|
|
||
| The removed `LICENSE.PROPRIETARY` remains available in Git history for provenance; | ||
| it does not apply to the current source tree. | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | sed -n '1,120p'
echo
echo "== license files =="
git ls-files | grep -E '(^|/)(LICENSING\.md|README\.md|ROADMAP\.md|LICENSE|NOTICE|TRADEMARKS\.md)$' || true
echo
echo "== relevant snippets =="
for f in LICENSING.md README.md ROADMAP.md LICENSE NOTICE TRADEMARKS.md; do
if [ -f "$f" ]; then
echo "--- $f ---"
wc -l "$f"
sed -n '1,240p' "$f"
fi
done
echo
echo "== searches for license and provenance terms =="
rg -n "MPL|Mozilla Public License|MPL-2\.0|license transition|provenance|unresolved|contributor|historical|RELEASE|release candidate|0\.1\.0|LICENSE\.PROPRIETARY" -S .Repository: Deathcharge/helix-hub-shared
Length of output: 50386
Document MPL/provenance approval before asserting settled MPL-2.0 status.
The public docs declare the tree and 0.1.0 candidate as MPL-2.0, but ROADMAP.md still lists approve MPL/provenance and license transition/provenance as unresolved. Apply the same language to both sites once provenance, contributor relicensing consent, and release evidence are recorded.
LICENSING.md#L30-L37README.md#L188-L192
📍 Affects 2 files
LICENSING.md#L30-L37(this comment)README.md#L188-L192
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@LICENSING.md` around lines 30 - 37, Update LICENSING.md lines 30-37 and
README.md lines 188-192 to avoid asserting settled MPL-2.0 status until
ROADMAP.md’s MPL/provenance approval and license transition/provenance items are
resolved. Apply consistent wording at both sites stating that provenance,
contributor relicensing consent, and release evidence must be recorded before
confirming the MPL-2.0 status.
Comment on lines
+10
to
+11
| prune agents | ||
| prune services |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Apply the legacy-package boundary to source distributions.
The wheel checks reject helix_llm_agent_engine, but the MANIFEST and both sdist checks omit it. A published sdist can therefore contain the legacy engine while all artifact checks pass.
MANIFEST.in#L10-L11: addprune helix_llm_agent_engine..github/workflows/ci.yml#L103-L107: includehelix_llm_agent_enginein the forbidden sdist directories..github/workflows/release.yml#L82-L86: includehelix_llm_agent_enginein the forbidden sdist directories.
📍 Affects 3 files
MANIFEST.in#L10-L11(this comment).github/workflows/ci.yml#L103-L107.github/workflows/release.yml#L82-L86
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@MANIFEST.in` around lines 10 - 11, Extend the legacy-package exclusion across
source distributions: add helix_llm_agent_engine to the prune entries in
MANIFEST.in, and add it to the forbidden sdist directory checks in
.github/workflows/ci.yml (lines 103-107) and .github/workflows/release.yml
(lines 82-86).
Comment on lines
+47
to
+51
| run.add_argument("--timeout", type=float, default=30.0) | ||
| run.add_argument("--max-retries", type=int, default=2) | ||
| run.add_argument("--max-input-chars", type=int, default=20_000) | ||
| run.add_argument("--max-output-tokens", type=int, default=1_024) | ||
| run.add_argument("--max-response-chars", type=int, default=200_000) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Extend bounds validation to the other numeric options; consider a catch-all error mapping.
--timeout, --max-retries, --max-output-tokens, and --max-response-chars (Line 47-51) have the same missing lower-bound validation as --max-input-chars. Negative retries/timeouts/limits passed straight into LLMAgentEngine/OpenAICompatibleProvider could produce confusing behavior downstream.
Separately, main()'s try/except (Line 123-133) doesn't catch generic Exception, so any unexpected error (e.g. a bug in a custom provider) surfaces as a raw traceback instead of a clean CLI error line.
Also applies to: 120-133
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/samsarix_agent_engine/cli.py` around lines 47 - 51, Extend argparse
lower-bound validation in the run options alongside max-input-chars so timeout,
max-retries, max-output-tokens, and max-response-chars reject negative values
before reaching LLMAgentEngine or OpenAICompatibleProvider. Update main()’s
existing error-handling block to map unexpected Exception instances to the same
clean CLI error output while preserving the current handling for explicitly
supported errors.
| ) | ||
| run.add_argument("--timeout", type=float, default=30.0) | ||
| run.add_argument("--max-retries", type=int, default=2) | ||
| run.add_argument("--max-input-chars", type=int, default=20_000) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does Python's io.TextIOWrapper.read() treat a negative size as read-until-EOF?
💡 Result:
Yes, Python's io.TextIOWrapper.read method treats a negative size argument as a request to read until EOF [1][2]. According to the official Python documentation, if the size argument is negative or None, the method reads and returns characters from the stream until EOF is reached [1][2]. This behavior is also reflected in the CPython source code implementation, where the docstring explicitly states that "if n is negative or omitted, read until EOF" [3][4][5].
Citations:
- 1: https://docs.python.org/3/library/io.html
- 2: https://docs.python.org/3.10/library/io.html
- 3: https://github.com/python/cpython/blob/2fc46979b8c802675ca7fd51c6f2108a305001c8/Modules/_io/textio.c
- 4: https://github.com/python/cpython/blob/v3.7.12/Modules/_io/textio.c
- 5: https://github.com/python/cpython/blob/0274d830/Modules/_io/textio.c
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate cli.py and test files"
fd -a 'cli\.py|test_cli\.py' . | sed 's#^\./##'
echo
echo "Inspect src/samsarix_agent_engine/cli.py outline and relevant lines"
if [ -f src/samsarix_agent_engine/cli.py ]; then
wc -l src/samsarix_agent_engine/cli.py
ast-grep outline src/samsarix_agent_engine/cli.py || true
sed -n '1,120p' src/samsarix_agent_engine/cli.py | cat -n
fi
echo
echo "Inspect tests/test_cli.py relevant sections"
if [ -f tests/test_cli.py ]; then
wc -l tests/test_cli.py
sed -n '1,120p' tests/test_cli.py | cat -n
fi
echo
echo "Search for max-input-chars / max_input_chars usage"
rg -n "max-input-chars|max_input_chars|TextIOWrapper|stdin|read\\(" -S .Repository: Deathcharge/helix-hub-shared
Length of output: 14894
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect relevant cli.py main/error mapping"
sed -n '120,150p' src/samsarix_agent_engine/cli.py | cat -n
echo
echo "Read-only behavioral probe for TextIOWrapper/TextIOWrapper.read semantics with negative size"
python3 - <<'PY'
import io
s = io.StringIO("abcdef")
print("StringIO.read(0):", repr(s.read(0)))
print("StringIO.read(-1):", repr(s.read(-1)))
s = io.StringIO("abcdef")
wrapped = io.TextIOWrapper(s, encoding="utf-8")
print("TextIOWrapper(over StringIO).read(0):", repr(wrapped.read(0)))
s2 = io.StringIO("abcdef")
s2.seek(0)
wrapped2 = io.TextIOWrapper(s2, encoding="utf-8")
print("TextIOWrapper(over StringIO).read(-1):", repr(wrapped2.read(-1)))
PY
echo
echo "Check current behavior for --max-input-chars <0 with a bounded string"
python3 - <<'PY'
from pathlib import Path
import sys
sys.path.insert(0, str(Path("tests").parent))
from samsarix_agent_engine import cli
import io
old_stdin = cli.sys.stdin
try:
cli.sys.stdin = io.StringIO("hello")
print("exit:", cli.main(["run", "--max-input-chars", "-1"]))
stdout_result = cli.sys.stdout.getvalue()
stderr_result = cli.sys.stderr.getvalue()
print("contains max_input_chars:", "max_input_chars" in stderr_result)
print("stdout:", repr(stdout_result))
print("stderr:", repr(stderr_result))
finally:
cli.sys.stdin = old_stdin
cli.sys = cli.sys
PYRepository: Deathcharge/helix-hub-shared
Length of output: 1411
Validate --max-input-chars before reading stdin.
Python text readers treat a negative size as “read until EOF,” so --max-input-chars <= -2 still enables unbounded stdin reads. Validate the CLI argument with a non-negative int conversion and an ArgumentTypeError containing max_input_chars so the invalid limit is rejected with a clean error before any stdin read occurs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/samsarix_agent_engine/cli.py` at line 49, Update the --max-input-chars
argument definition in the CLI parser to use a validating non-negative integer
converter that raises ArgumentTypeError mentioning max_input_chars for values
below zero. Ensure argparse performs this validation before the command reads
stdin, while preserving the existing default and valid integer behavior.
Comment on lines
+79
to
+113
| def __init__( | ||
| self, | ||
| *, | ||
| api_key: str | None = None, | ||
| base_url: str = "https://api.openai.com/v1", | ||
| timeout: float = 30.0, | ||
| max_retries: int = 2, | ||
| retry_backoff: float = 0.5, | ||
| max_response_bytes: int = 2_000_000, | ||
| client: httpx.AsyncClient | None = None, | ||
| ) -> None: | ||
| self.endpoint = self._validate_endpoint(base_url) | ||
| if api_key is not None and (not isinstance(api_key, str) or not api_key.strip()): | ||
| raise ConfigurationError("api_key must be a non-empty string when supplied") | ||
| if not 0 < timeout <= 300: | ||
| raise ConfigurationError("timeout must be greater than 0 and at most 300 seconds") | ||
| if not isinstance(max_retries, int) or not 0 <= max_retries <= 5: | ||
| raise ConfigurationError("max_retries must be an integer between 0 and 5") | ||
| if not 0 <= retry_backoff <= 10: | ||
| raise ConfigurationError("retry_backoff must be between 0 and 10 seconds") | ||
| if not 1_024 <= max_response_bytes <= 10_000_000: | ||
| raise ConfigurationError("max_response_bytes must be between 1024 and 10000000") | ||
|
|
||
| self.max_retries = max_retries | ||
| self.retry_backoff = retry_backoff | ||
| self.max_response_bytes = max_response_bytes | ||
| self._owns_client = client is None | ||
| headers = {"Accept": "application/json", "User-Agent": "samsarix-agent-engine/0.1"} | ||
| if api_key: | ||
| headers["Authorization"] = f"Bearer {api_key.strip()}" | ||
| self._client = client or httpx.AsyncClient( | ||
| headers=headers, | ||
| timeout=httpx.Timeout(timeout), | ||
| follow_redirects=False, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
api_key is silently dropped when a custom client is also supplied.
Headers (including the Authorization: Bearer header built from api_key) are only applied to the internally-constructed client; if a caller passes client= alongside api_key=, the key is validated but never sent. This will silently produce unauthenticated requests with no error, which is confusing and can leak into production (e.g. dependency-injecting a client for connection pooling while still expecting api_key to be honored).
🐛 Proposed fix to always apply headers regardless of client origin
- self._owns_client = client is None
- headers = {"Accept": "application/json", "User-Agent": "samsarix-agent-engine/0.1"}
- if api_key:
- headers["Authorization"] = f"Bearer {api_key.strip()}"
- self._client = client or httpx.AsyncClient(
- headers=headers,
- timeout=httpx.Timeout(timeout),
- follow_redirects=False,
- )
+ self._owns_client = client is None
+ self._client = client or httpx.AsyncClient(
+ timeout=httpx.Timeout(timeout),
+ follow_redirects=False,
+ )
+ self._client.headers.setdefault("Accept", "application/json")
+ self._client.headers.setdefault("User-Agent", "samsarix-agent-engine/0.1")
+ if api_key:
+ self._client.headers["Authorization"] = f"Bearer {api_key.strip()}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def __init__( | |
| self, | |
| *, | |
| api_key: str | None = None, | |
| base_url: str = "https://api.openai.com/v1", | |
| timeout: float = 30.0, | |
| max_retries: int = 2, | |
| retry_backoff: float = 0.5, | |
| max_response_bytes: int = 2_000_000, | |
| client: httpx.AsyncClient | None = None, | |
| ) -> None: | |
| self.endpoint = self._validate_endpoint(base_url) | |
| if api_key is not None and (not isinstance(api_key, str) or not api_key.strip()): | |
| raise ConfigurationError("api_key must be a non-empty string when supplied") | |
| if not 0 < timeout <= 300: | |
| raise ConfigurationError("timeout must be greater than 0 and at most 300 seconds") | |
| if not isinstance(max_retries, int) or not 0 <= max_retries <= 5: | |
| raise ConfigurationError("max_retries must be an integer between 0 and 5") | |
| if not 0 <= retry_backoff <= 10: | |
| raise ConfigurationError("retry_backoff must be between 0 and 10 seconds") | |
| if not 1_024 <= max_response_bytes <= 10_000_000: | |
| raise ConfigurationError("max_response_bytes must be between 1024 and 10000000") | |
| self.max_retries = max_retries | |
| self.retry_backoff = retry_backoff | |
| self.max_response_bytes = max_response_bytes | |
| self._owns_client = client is None | |
| headers = {"Accept": "application/json", "User-Agent": "samsarix-agent-engine/0.1"} | |
| if api_key: | |
| headers["Authorization"] = f"Bearer {api_key.strip()}" | |
| self._client = client or httpx.AsyncClient( | |
| headers=headers, | |
| timeout=httpx.Timeout(timeout), | |
| follow_redirects=False, | |
| ) | |
| def __init__( | |
| self, | |
| *, | |
| api_key: str | None = None, | |
| base_url: str = "https://api.openai.com/v1", | |
| timeout: float = 30.0, | |
| max_retries: int = 2, | |
| retry_backoff: float = 0.5, | |
| max_response_bytes: int = 2_000_000, | |
| client: httpx.AsyncClient | None = None, | |
| ) -> None: | |
| self.endpoint = self._validate_endpoint(base_url) | |
| if api_key is not None and (not isinstance(api_key, str) or not api_key.strip()): | |
| raise ConfigurationError("api_key must be a non-empty string when supplied") | |
| if not 0 < timeout <= 300: | |
| raise ConfigurationError("timeout must be greater than 0 and at most 300 seconds") | |
| if not isinstance(max_retries, int) or not 0 <= max_retries <= 5: | |
| raise ConfigurationError("max_retries must be an integer between 0 and 5") | |
| if not 0 <= retry_backoff <= 10: | |
| raise ConfigurationError("retry_backoff must be between 0 and 10 seconds") | |
| if not 1_024 <= max_response_bytes <= 10_000_000: | |
| raise ConfigurationError("max_response_bytes must be between 1024 and 10000000") | |
| self.max_retries = max_retries | |
| self.retry_backoff = retry_backoff | |
| self.max_response_bytes = max_response_bytes | |
| self._owns_client = client is None | |
| self._client = client or httpx.AsyncClient( | |
| timeout=httpx.Timeout(timeout), | |
| follow_redirects=False, | |
| ) | |
| self._client.headers.setdefault("Accept", "application/json") | |
| self._client.headers.setdefault("User-Agent", "samsarix-agent-engine/0.1") | |
| if api_key: | |
| self._client.headers["Authorization"] = f"Bearer {api_key.strip()}" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/samsarix_agent_engine/providers.py` around lines 79 - 113, Update the
constructor’s client setup so headers built from api_key, including
Authorization, are applied when a custom client is supplied as well as when
httpx.AsyncClient is created internally. Preserve the existing validation,
ownership tracking, and default client options, while ensuring api_key is never
silently ignored.
Comment on lines
+116
to
+131
| def _validate_endpoint(base_url: str) -> str: | ||
| if not isinstance(base_url, str) or not base_url.strip(): | ||
| raise ConfigurationError("base_url must be a non-empty URL") | ||
| if len(base_url) > 2_048: | ||
| raise ConfigurationError("base_url is too long") | ||
| parsed = urlsplit(base_url.strip()) | ||
| if parsed.scheme not in {"http", "https"} or not parsed.hostname: | ||
| raise ConfigurationError("base_url must be an absolute http or https URL") | ||
| if parsed.username is not None or parsed.password is not None: | ||
| raise ConfigurationError("base_url must not contain credentials") | ||
| if parsed.query or parsed.fragment: | ||
| raise ConfigurationError("base_url must not contain a query string or fragment") | ||
| normalized = base_url.strip().rstrip("/") | ||
| if parsed.path.rstrip("/").endswith("/chat/completions"): | ||
| return normalized | ||
| return f"{normalized}/chat/completions" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether base_url/endpoint configuration for OpenAICompatibleProvider is exposed to
# CLI flags or other user-controllable inputs outside this cohort.
rg -n --type=py -C3 'base_url' src examples 2>/dev/nullRepository: Deathcharge/helix-hub-shared
Length of output: 4307
🏁 Script executed:
#!/bin/bash
set -e
printf '--- cli.py relevant section ---\n'
sed -n '1,130p' src/samsarix_agent_engine/cli.py | cat -n
printf '\n--- argument parser base_url references ---\n'
rg -n --type=py -C3 'add_argument|base_url|provider' src/samsarix_agent_engine 2>/dev/null
printf '\n--- providers.py relevant section ---\n'
sed -n '60,150p' src/samsarix_agent_engine/providers.py | cat -nRepository: Deathcharge/helix-hub-shared
Length of output: 257
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- cli.py relevant section ---'
sed -n '1,130p' src/samsarix_agent_engine/cli.py | cat -n
printf '%s\n' ''
printf '%s\n' '--- argument parser base_url references ---'
rg -n --type=py -C3 'add_argument|base_url|provider' src/samsarix_agent_engine 2>/dev/null
printf '%s\n' ''
printf '%s\n' '--- providers.py relevant section ---'
sed -n '60,150p' src/samsarix_agent_engine/providers.py | cat -nRepository: Deathcharge/helix-hub-shared
Length of output: 45161
Block private/internal endpoints before forming base_url/endpoint
--base-url and SAMSARIX_LLM_BASE_URL are accepted by the CLI and passed directly to OpenAICompatibleProvider, while _validate_endpoint still allows loopback, private, and link-local hosts. Reject IP literal/private ranges in the hostname before creating/appending /chat/completions to avoid using internal/metadata endpoints as the model target.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/samsarix_agent_engine/providers.py` around lines 116 - 131, Update
_validate_endpoint to reject loopback, private, link-local, and other internal
IP-literal hosts after parsing the URL and before normalizing or appending
/chat/completions. Preserve the existing scheme, credential, query, fragment,
and path validation for public hostnames, and raise ConfigurationError for
disallowed addresses.
Comment on lines
+149
to
+185
| for attempt in range(self.max_retries + 1): | ||
| try: | ||
| request = self._client.build_request("POST", self.endpoint, json=payload) | ||
| response = await self._client.send(request, stream=True) | ||
| except httpx.TimeoutException as exc: | ||
| if attempt < self.max_retries: | ||
| await self._sleep_before_retry(attempt) | ||
| continue | ||
| raise ProviderError("provider request timed out", retryable=True) from exc | ||
| except httpx.RequestError as exc: | ||
| if attempt < self.max_retries: | ||
| await self._sleep_before_retry(attempt) | ||
| continue | ||
| raise ProviderError("provider request failed", retryable=True) from exc | ||
|
|
||
| try: | ||
| status = response.status_code | ||
| if status in _RETRYABLE_STATUS_CODES and attempt < self.max_retries: | ||
| retry_after = self._bounded_retry_after(response.headers.get("retry-after")) | ||
| await response.aclose() | ||
| await self._sleep_before_retry(attempt, retry_after) | ||
| continue | ||
| if not 200 <= status < 300: | ||
| request_id = self._safe_request_id(response.headers.get("x-request-id")) | ||
| suffix = f" (request {request_id})" if request_id else "" | ||
| raise ProviderError( | ||
| f"provider returned HTTP {status}{suffix}", | ||
| status_code=status, | ||
| retryable=status in _RETRYABLE_STATUS_CODES, | ||
| ) | ||
| data = await self._read_bounded_json(response) | ||
| finally: | ||
| await response.aclose() | ||
|
|
||
| return self._normalize_response(data, requested_model=model) | ||
|
|
||
| raise ProviderError("provider request exhausted its retry budget", retryable=True) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Read-phase failures during streaming aren't retried.
The retry loop (Lines 149-162) only guards build_request/send for connect/timeout errors. _read_bounded_json (called at Line 179) runs outside that guarded region, so a timeout or connection drop while streaming the response body exits invoke() immediately without consuming the configured retry budget (it's still safely mapped to a sanitized error upstream in engine.py, but resilience is reduced for what is otherwise a fully-retried request path).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/samsarix_agent_engine/providers.py` around lines 149 - 185, The retry
loop in the provider request method must also retry timeout and connection
failures raised by _read_bounded_json during response consumption. Extend the
existing httpx.TimeoutException and httpx.RequestError handling to cover the
read phase, preserving response cleanup and the configured retry budget before
raising the existing sanitized ProviderError results.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Trailing raise at Line 185 looks unreachable.
Tracing every branch of the for attempt in range(self.max_retries + 1) loop, each path either continues, returns, or raises before the loop can exit normally — including the final attempt for both retryable-status and exception cases. If that's intentional defensive code for future refactors, a short comment would help; otherwise it can be removed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/samsarix_agent_engine/providers.py` around lines 149 - 185, The trailing
ProviderError after the retry loop in the request flow is unreachable because
every iteration continues, returns, or raises. Remove the redundant raise
following the loop, or add a brief comment explicitly documenting it as
defensive future-proofing if it must remain.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Merge intent
This PR integrates the reviewed productization branch and its repository-specific roadmap. Merge is distinct from release, publication, deployment, or adoption as a canonical Samsarix Unified subsystem.
Portfolio contract
Verification
The portfolio merge-readiness pass compared this branch with its default, inspected its flagship relationship, and found no merge-blocking regression. Repository CI and any additional local verification remain recorded in the portfolio audit ledger.