feat: add downloadable client and atomic production delivery - #12
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe pull request adds a downloadable API client, atomic CircleCI production delivery, Nginx deployment handling, release identity checks, production model-policy validation, and structured provider diagnostics. ChangesDownloadable API client
Atomic production delivery
Runtime contracts
Estimated code review effort: 5 (Critical) | ~120 minutes Mergeability Score: 🔵 Low · up to The PR adds atomic production delivery, but the stale-release path does not yet demonstrate that older jobs are ordered correctly and cannot trigger deployment side effects. This is a bounded deployment-safety risk; the change is mergeable with explicit owner awareness and follow-up to strengthen that verification. Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (6)
client/veetbot_client/api.py (2)
245-254: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the
User-Agentversion from__version__.The literal reports
0.1, but the package version is0.1.0.dev0. The two values will drift on the next release.♻️ Proposed change
+from . import __version__ + ... headers = { "Accept": "application/json", - "User-Agent": "veetbot-client/0.1", + "User-Agent": f"veetbot-client/{__version__}", }🤖 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 `@client/veetbot_client/api.py` around lines 245 - 254, Update the _headers method to derive the User-Agent version from the package’s __version__ symbol instead of the hardcoded “0.1” value, preserving the existing veetbot-client prefix and header behavior.
314-319: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSilence Ruff S310 explicitly, because
make checkruns lint.Ruff reports S310 for the
Requestconstruction here and at lines 389-393. The finding is a false positive on behavior:__init__restricts the scheme tohttporhttps, and_urlrequires an absolute path. The lint gate still fails.Add a scoped suppression with the reason, or add a per-file rule exclusion for the client sources.
♻️ Proposed suppression at both call sites
- request = Request( + request = Request( # noqa: S310 - scheme restricted to http/https in __init__ self._url(path), data=encoded, headers=request_headers, method=method, )- request = Request( + request = Request( # noqa: S310 - scheme restricted to http/https in __init__ self._url(f"/v1/runs/{quote(run_id, safe='')}/events"), headers=headers, method="GET", )🤖 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 `@client/veetbot_client/api.py` around lines 314 - 319, Add a scoped Ruff S310 suppression with an explicit false-positive reason to both Request constructions in the client code, including the shown call site and the one around lines 389-393. Keep the existing Request behavior unchanged and ensure make check no longer reports these findings.Source: Linters/SAST tools
client/veetbot_client/chat.py (1)
353-382: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueBound the total reconnect budget, not only the consecutive one.
Line 360 resets
reconnect_attemptsfor every received event. A peer that delivers one event and then closes the stream resets the counter on each cycle, so_max_reconnect_attemptsis never reached. The loop then repeats until the run reaches a terminal state, with a 0.25 s floor delay.The behavior is acceptable for a long non-terminal run. Add a total attempt count or a wall-clock budget to bound the worst case.
🤖 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 `@client/veetbot_client/chat.py` around lines 353 - 382, The watch_run reconnect logic only bounds consecutive failures because reconnect_attempts resets after every event. Add a separate total reconnect-attempt counter or wall-clock budget that is not reset by _handle_event processing, enforce it alongside _max_reconnect_attempts, and preserve the existing terminal-state and reconnect-delay behavior.client/veetbot_client/__main__.py (1)
37-69: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winScope the token prompt to the readiness probe.
except ApiErrorat line 58 wraps the whole_runcall._runalso opens a session and runs the interactive shell. A 401 raised after the readiness probe, for example duringcreate_session, therefore restarts_runfrom the start. The client then creates a second session, and any interactive progress is lost.Prompt for the token from the probe only, then run the application once.
♻️ Proposed change
-def _run(args: argparse.Namespace, client: ApiClient) -> int: - ready = client.health_ready() +def _check_ready(client: ApiClient) -> None: + ready = client.health_ready() if ready.get("status") != "ready": raise ClientError("API is not ready") + + +def _run(args: argparse.Namespace, client: ApiClient) -> int: console = Console(sys.stdout, sys.stderr) application = ChatApplication( client, console, agent_id=str(args.agent), session_id=str(args.session) if args.session else None, ) return application.run(once=str(args.once) if args.once is not None else None) def main(argv: Sequence[str] | None = None) -> int: args = _parser().parse_args(argv) token = os.environ.get("VEETBOT_API_TOKEN") try: client = ApiClient(str(args.api_url), token=token) try: - return _run(args, client) + _check_ready(client) except ApiError as exc: if exc.status != 401 or client.has_token or not sys.stdin.isatty(): raise supplied = getpass.getpass("API token: ") client.set_token(supplied) - return _run(args, client) + _check_ready(client) + return _run(args, client) except KeyboardInterrupt:🤖 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 `@client/veetbot_client/__main__.py` around lines 37 - 69, Refactor main so the ApiError retry logic covers only the client.health_ready() probe, not the entire _run flow. Prompt for a token and retry the readiness check when an unauthenticated 401 occurs, then invoke _run exactly once with the ready client; preserve existing handling for authenticated clients and other errors..circleci/config.yml (1)
219-229: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winValidate the release identifier before remote interpolation.
deploy-appvalidatesrelease-idagainst^[0-9]{8}-[0-9]{6}-[0-9a-f]{7,40}$at line 146 before it reaches a remote command.deploy-nginxreads the same file and interpolates it into the remote command with no validation. Apply the same check so both jobs enforce one identifier contract.🛡️ Proposed validation
expected_release_id="$(cat /tmp/veetbot-release/release-id)" + [[ "$expected_release_id" =~ ^[0-9]{8}-[0-9]{6}-[0-9a-f]{7,40}$ ]] remote_config="/tmp/veetbot-nginx-$CIRCLE_SHA1.conf"🤖 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 @.circleci/config.yml around lines 219 - 229, Validate expected_release_id in the deploy-nginx flow before interpolating it into the ssh command, using the same ^[0-9]{8}-[0-9]{6}-[0-9a-f]{7,40}$ contract already enforced by deploy-app. Reject invalid values and stop the job before either remote command uses the identifier.tests/unit/test_downloadable_client.py (1)
400-414: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winIsolate the interpreter so the test proves dependency-free execution.
The test runs the artifact with
sys.executable, which is the project interpreter. All project dependencies stay importable, and an inheritedPYTHONPATHalso stays active. The test therefore passes even if the client imports a third-party package. Add-Ito run in isolated mode.💚 Proposed isolation
result = subprocess.run( - [sys.executable, str(artifact), "--version"], + [sys.executable, "-I", str(artifact), "--version"], cwd=tmp_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 `@tests/unit/test_downloadable_client.py` around lines 400 - 414, Update test_client_zipapp_builds_and_runs_without_project_dependencies to invoke the artifact with Python’s isolated interpreter mode by adding the -I option to the subprocess command. Preserve the existing version assertion, working directory, and executable checks.
🤖 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 @.circleci/config.yml:
- Around line 179-196: Update the “Verify public release identity” command to
poll the health endpoint and response headers until the X-Veetbot-Release value
matches VEETBOT_RELEASE_ID, using a bounded retry/time budget. Keep the existing
curl and header validation behavior, but retry transient missing or mismatched
headers and exit nonzero only when the budget expires.
- Line 135: Update both add_ssh_keys steps in the CircleCI configuration to
include the required fingerprints list, replacing the empty mappings at the
referenced deployment-key steps. Use the project’s deployment key fingerprints
consistently in both locations.
In `@client/veetbot_client/api.py`:
- Around line 256-294: Update _api_error so transport exceptions raised while
reading the HTTPError body, including OSError, TimeoutError, and URLError, are
caught and converted to the same sanitized ApiError fallback used for malformed
responses. Keep the existing parsed error response behavior unchanged, and
ensure no raw read exception escapes through _open.
In `@deploy/app/release.sh`:
- Around line 76-80: The stale-release check around RELEASE_PATTERN and
ACTIVE_RELEASE_ID must compare only the YYYYMMDD-HHMMSS timestamp portion, not
the complete release IDs. Extract and compare the timestamp components so
releases sharing a timestamp are not rejected based on revision ordering, and
add a regression case covering equal timestamps with different revisions.
In `@deploy/nginx/deploy.sh`:
- Around line 79-80: Update the deployment flow around the install and symlink
mutations to register an ERR or EXIT trap that invokes rollback after the backup
is established, ensuring failures in either sudo install or sudo ln -sfn restore
the previous configuration. Disable the trap only after the Nginx reload
succeeds, and add tests covering failure of each mutation command.
In `@docs/deployment.md`:
- Around line 37-50: The deployment runbook currently assigns public readiness
verification to the server/application release; update the release workflow
description around the numbered deployment steps and the corresponding
statements near the later verification section to state that CircleCI performs
the public readiness probe after promotion. Keep local readiness header
verification assigned to deploy/app/release.sh and describe public probe
failures as the CircleCI post-promotion boundary.
- Around line 220-230: Update the manual rollback procedure around the commands
that repoint current, retag agent-core-sandbox:production, restart services, and
verify health to acquire and hold /opt/veetbot/shared/deploy.lock with the same
flock mechanism used by deployments. Ensure the lock covers the complete
rollback sequence from before changing the symlink through the service restart
and readiness check.
In `@docs/plan/development-toolchain.md`:
- Around line 278-289: Update the development-toolchain documentation around the
CI job table and `make check` statements to state that deployment validation
(`test-deploy`) and sandbox validation are additional CI gates rather than
claiming the static and contract jobs exactly equal `make check`; preserve the
original verification partition description and clarify the corresponding
`verify` workflow coverage.
In `@src/agent_core/config.py`:
- Around line 530-535: Extract the VEETBOT_RELEASE_ID format check from
load_settings() into a shared release_id validation helper, then invoke that
helper from both load_settings() and validate_settings(). Ensure Settings
instances supplied through build(settings=...) are rejected before create_app()
when release_id is not None and fails RELEASE_ID_PATTERN.
In `@tests/unit/test_downloadable_client.py`:
- Around line 102-106: Add an in-place Ruff S106 suppression to both intentional
bearer-token test fixtures in
test_api_client_refuses_remote_plaintext_bearer_token, covering the failing
constructor calls without changing the test behavior.
---
Nitpick comments:
In @.circleci/config.yml:
- Around line 219-229: Validate expected_release_id in the deploy-nginx flow
before interpolating it into the ssh command, using the same
^[0-9]{8}-[0-9]{6}-[0-9a-f]{7,40}$ contract already enforced by deploy-app.
Reject invalid values and stop the job before either remote command uses the
identifier.
In `@client/veetbot_client/__main__.py`:
- Around line 37-69: Refactor main so the ApiError retry logic covers only the
client.health_ready() probe, not the entire _run flow. Prompt for a token and
retry the readiness check when an unauthenticated 401 occurs, then invoke _run
exactly once with the ready client; preserve existing handling for authenticated
clients and other errors.
In `@client/veetbot_client/api.py`:
- Around line 245-254: Update the _headers method to derive the User-Agent
version from the package’s __version__ symbol instead of the hardcoded “0.1”
value, preserving the existing veetbot-client prefix and header behavior.
- Around line 314-319: Add a scoped Ruff S310 suppression with an explicit
false-positive reason to both Request constructions in the client code,
including the shown call site and the one around lines 389-393. Keep the
existing Request behavior unchanged and ensure make check no longer reports
these findings.
In `@client/veetbot_client/chat.py`:
- Around line 353-382: The watch_run reconnect logic only bounds consecutive
failures because reconnect_attempts resets after every event. Add a separate
total reconnect-attempt counter or wall-clock budget that is not reset by
_handle_event processing, enforce it alongside _max_reconnect_attempts, and
preserve the existing terminal-state and reconnect-delay behavior.
In `@tests/unit/test_downloadable_client.py`:
- Around line 400-414: Update
test_client_zipapp_builds_and_runs_without_project_dependencies to invoke the
artifact with Python’s isolated interpreter mode by adding the -I option to the
subprocess command. Preserve the existing version assertion, working directory,
and executable checks.
🪄 Autofix
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: CHILL
Plan: Pro Plus
Run ID: 3470d292-3c76-47cc-87a1-d97eeb90f7f5
📒 Files selected for processing (52)
.circleci/config.yml.env.example.gitignoreMakefileREADME.mdclient/__init__.pyclient/veetbot_client/__init__.pyclient/veetbot_client/__main__.pyclient/veetbot_client/api.pyclient/veetbot_client/chat.pydeploy/Caddyfile.exampledeploy/app/release.shdeploy/app/release.test.shdeploy/nginx/deploy.shdeploy/nginx/deploy.test.shdeploy/systemd/veetbot-api.servicedeploy/veetbot.env.exampledocs-manifest.yamldocs/adr/0047-downloadable-thin-client.mddocs/adr/0048-atomic-circleci-production-delivery.mddocs/adr/index.mddocs/changelog.mddocs/client.mddocs/deployment.mddocs/index.mddocs/plan/development-toolchain.mddocs/plan/model-gateway.mddocs/plan/readiness.mddocs/security.mddocs/status/citation-ledger.yamlmkdocs.ymlnginx/veetbot.confscripts/architecture_checks.pyscripts/build_client.pyscripts/check_production_deployment.pysecurity/secret-allowlist.yamlsrc/agent_core/adapters/models/common.pysrc/agent_core/adapters/models/openai_responses.pysrc/agent_core/api/app.pysrc/agent_core/bootstrap.pysrc/agent_core/cli/main.pysrc/agent_core/config.pysrc/agent_core/domain/messages.pysrc/agent_core/runtime/loop.pytests/contract/test_model_gateway_contract.pytests/gates/test_api_m5.pytests/gates/test_model_profiles_m3.pytests/gates/test_runtime_m1.pytests/gates/test_secret_scanner.pytests/unit/test_config.pytests/unit/test_downloadable_client.pytests/unit/test_toolchain.py
💤 Files with no reviewable changes (1)
- deploy/Caddyfile.example
Owner
Author
CodeRabbit auto-fix summaryAddressed all 16 actionable findings from the current review in commit
Validation:
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
deploy/nginx/deploy.test.sh (1)
87-95: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winExercise the documented stale-job direction.
The fixture makes the active application release
20260810-152233-abcdef0and the expected release20260810-152244-bcdef01. This models a newer expected release arriving before application promotion, not an older Nginx job arriving after a newer application promotion. It would not catch a stale20260810-152233-abcdef0job overwriting an active20260810-152244-bcdef01configuration.Use the reverse ordering, clear
LOG_FILE, and assert that the stale run does not callinstall,nginx -t, orsystemctl reload nginx.🤖 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 `@deploy/nginx/deploy.test.sh` around lines 87 - 95, Update the stale-deployment fixture around run_deploy so the active application release is 20260810-152244-bcdef01 while VEETBOT_EXPECTED_RELEASE_ID uses the older 20260810-152233-abcdef0 value. Clear LOG_FILE before the run, then assert the stale execution skips deployment and does not invoke install, nginx -t, or systemctl reload nginx, while preserving the existing configuration assertion.
🤖 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/deployment.md`:
- Around line 229-236: Add fail-fast settings at the start of the manual
rollback shell block with set -euo pipefail, and explicitly handle a nonzero
flock -w 900 9 result by emitting an error to stderr and exiting before any
rollback commands run. Keep the deployment lock held for the entire block.
---
Outside diff comments:
In `@deploy/nginx/deploy.test.sh`:
- Around line 87-95: Update the stale-deployment fixture around run_deploy so
the active application release is 20260810-152244-bcdef01 while
VEETBOT_EXPECTED_RELEASE_ID uses the older 20260810-152233-abcdef0 value. Clear
LOG_FILE before the run, then assert the stale execution skips deployment and
does not invoke install, nginx -t, or systemctl reload nginx, while preserving
the existing configuration assertion.
🪄 Autofix
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: CHILL
Plan: Pro Plus
Run ID: 72c37b18-f76b-4b09-bc82-59fc7e9cd79b
📒 Files selected for processing (15)
.circleci/config.ymlclient/veetbot_client/__main__.pyclient/veetbot_client/api.pyclient/veetbot_client/chat.pydeploy/app/release.shdeploy/app/release.test.shdeploy/nginx/deploy.shdeploy/nginx/deploy.test.shdocs/deployment.mddocs/plan/development-toolchain.mdpyproject.tomlsrc/agent_core/config.pytests/unit/test_config.pytests/unit/test_downloadable_client.pytests/unit/test_toolchain.py
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/unit/test_config.py
- deploy/app/release.test.sh
- deploy/app/release.sh
- tests/unit/test_toolchain.py
- client/veetbot_client/main.py
- client/veetbot_client/api.py
- .circleci/config.yml
- client/veetbot_client/chat.py
Owner
Author
|
Addressed the follow-up rollback finding in |
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.
Summary
Verification
make checkgit diff --checkNotes
Summary by CodeRabbit