feat(deploy): add fail-closed trading ECS host contract - #47
Merged
Conversation
|
Warning Review limit reached
Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughAdds a digest-pinned Tokyo bare-ECS trading host contract, deterministic ACR release packaging, hardened runtime and lifecycle scripts, a static systemd unit, Paper/Shadow policy validation, operational documentation, and comprehensive CI contract tests. ChangesTokyo trading ECS host
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ReleaseWorkflow
participant ACR
participant HostControl
participant Runtime
participant Systemd
ReleaseWorkflow->>ACR: build and publish digest-pinned image
ReleaseWorkflow->>HostControl: upload release manifest and control bundle
HostControl->>ACR: login and pull exact digest
HostControl->>Runtime: run preflight validation
HostControl->>Systemd: install and explicitly start service
Systemd->>Runtime: run hardened trading container
Runtime-->>HostControl: return health, readiness, and identity
HostControl->>Systemd: stop and restore pointer on failure or rollback
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec5db8c840
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 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 @.github/workflows/acr-publish.yml:
- Around line 86-98: Update the registry validation case around
published_registry to require the complete Tokyo Personal Edition hostname
contract, including a crpi-<id> prefix, for both standard and -vpc endpoints.
Reject arbitrary prefixes before generating vpc_registry or rewriting
published_registry, while preserving the existing normalization and error
behavior for valid hostnames.
- Around line 130-138: Update the actions/upload-artifact step for “Upload
bare-metal trading host contract” to reference a full immutable commit SHA
instead of the mutable `@v4` tag, while preserving its existing artifact settings
and conditional execution.
In `@deployment/aliyun/trading-ecs-hostctl.sh`:
- Around line 546-558: Update deployment/aliyun/trading-ecs-hostctl.sh in the
rollback flow to preserve PASSED.sha256 and atomically create append-only
rollback-intent and rolled-back evidence markers instead of renaming or deleting
the success marker. Document this additive revocation state machine in
deployment/aliyun/TRADING_ECS_HOST.md lines 131-134, and update
deployment/aliyun/test-trading-ecs-host-contract.sh lines 325-328 to verify
PASSED.sha256 remains unchanged and both rollback markers are present and
validated.
- Around line 430-434: Update the deployment evidence flow around
candidate_file, previous_file, and PASSED.sha256 to record each snapshot’s
content hash or an explicit absent marker. Make candidate-current.env and
previous-current.env immutable or append-only, then have rollback validate those
recorded values and require the current pointer to match the committed candidate
before invalidating rollback authorization.
- Around line 247-281: Move the enabled-state rejection in
deployment/aliyun/trading-ecs-hostctl.sh:247-281 before Docker pull and all
installation mutations, while retaining the post-staging postcondition. Update
deployment/aliyun/TRADING_ECS_HOST.md:21-23 to state that boot safety also
requires rejecting pre-existing service enablement, and revise
deployment/aliyun/hft-trading-ecs.service:30-31 so it does not claim the missing
[Install] section alone prevents boot startup. Extend
deployment/aliyun/test-trading-ecs-host-contract.sh:338-345 to verify an enabled
service is rejected before any persistent mutation.
- Around line 82-84: Canonicalize the artifact and password paths before
trusted-root containment checks, rejecting unresolved or escaping paths rather
than relying on raw prefixes containing “..”. Update the artifact validation
around artifact_root and artifact_dir to require a canonical direct-child path,
and verify the password path’s actual filesystem with findmnt before accepting
it. Preserve the existing secure_directory checks after canonical validation.
In `@deployment/aliyun/trading-ecs-paper-shadow-policy.jq`:
- Around line 25-35: Update the activation-intent validation in the policy
expression so the selected StartPaper or StartShadow intent is present in both
$signed.envelope.allowed_intent_types and $policy[0].allowed_intent_types.
Preserve the existing approval_class matching and StartLiveSmall exclusions
while ensuring LoadFactor-only policy entries cannot authorize a start intent.
- Around line 1-2: Update the one_json_value jq helper to accept exactly one
input JSON value and raise an error when additional values are emitted, rather
than acting as an identity filter. Preserve the existing single-value output so
jq -e cannot ignore a failing value followed by a passing one.
In `@deployment/aliyun/trading-ecs-runtime.sh`:
- Around line 83-100: Update the actual_names check in the manifest validation
flow to exclude only the root activation.sha256 marker, not nested files with
that name, and reject special filesystem objects by limiting the find results to
regular files. Preserve the exact manifest-to-activation directory comparison
and the existing manifest validation rules.
- Around line 123-130: Harden validate_runtime_secrets by rejecting secret_root
when its permissions allow group or world writes, before validating or consuming
the secret files. Check the directory mode using the existing filesystem
validation flow and return failure unless the directory is owned and writable
only by the expected trusted user, preserving the current tmpfs and non-symlink
checks.
- Around line 239-244: Remove the --env-file "$secret_root/runtime.env" option
from the container launch command in the runtime script. Preserve the existing
read-only secret-file mounts, and update the runtime initialization to load
HFT_GRPC_AUTH_TOKEN and HFT_SECRET_* values from those mounted files instead of
container environment metadata.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e7fb1687-ed80-4a40-9bdc-97d8ad41caf2
📒 Files selected for processing (10)
.github/workflows/acr-publish.yml.github/workflows/ci.ymldeployment/aliyun/README.mddeployment/aliyun/TRADING_ECS_HOST.mddeployment/aliyun/hft-trading-ecs.servicedeployment/aliyun/test-trading-ecs-host-contract.shdeployment/aliyun/trading-ecs-hostctl.shdeployment/aliyun/trading-ecs-paper-shadow-policy.jqdeployment/aliyun/trading-ecs-runtime.shrust_hft/deployment/PRODUCTION_DEPLOYMENT.md
proerror77
force-pushed
the
codex/trading-ecs-host-contract
branch
3 times, most recently
from
July 16, 2026 00:27
5062296 to
72367f9
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@deployment/aliyun/trading-ecs-hostctl.sh`:
- Around line 239-247: Update the archive validation before the tar extraction
in the control-artifact flow to verify that each expected
member—hft-trading-ecs.service, trading-ecs-hostctl.sh,
trading-ecs-paper-shadow-policy.jq, and trading-ecs-runtime.sh—is a regular
file, rejecting FIFOs, devices, symlinks, and other non-regular entries before
tar runs. Preserve the existing exact-name comparison and subsequent checksum
validation.
- Around line 111-120: Update assert_service_boot_disabled_before_stage so the
not-found|'' branch immediately returns failure when systemctl is-enabled
produces empty output with a nonzero status; only perform the unit-path absence
check for a genuine not-found result with the expected status.
- Around line 346-349: Update the orphan-container guard around docker container
inspection to fail closed on Docker daemon/API errors. Reuse the exact-name
`docker container ls --all` fallback and health-check behavior from
`container_state()`, continuing only when Docker is healthy and no
`monday-hft-trading` container exists; otherwise invoke `die`.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9157f5c2-6f20-43bb-bd37-c59e3e46282c
📒 Files selected for processing (10)
.github/workflows/acr-publish.yml.github/workflows/ci.ymldeployment/aliyun/README.mddeployment/aliyun/TRADING_ECS_HOST.mddeployment/aliyun/hft-trading-ecs.servicedeployment/aliyun/test-trading-ecs-host-contract.shdeployment/aliyun/trading-ecs-hostctl.shdeployment/aliyun/trading-ecs-paper-shadow-policy.jqdeployment/aliyun/trading-ecs-runtime.shrust_hft/deployment/PRODUCTION_DEPLOYMENT.md
🚧 Files skipped from review as they are similar to previous changes (6)
- .github/workflows/ci.yml
- rust_hft/deployment/PRODUCTION_DEPLOYMENT.md
- deployment/aliyun/README.md
- deployment/aliyun/hft-trading-ecs.service
- deployment/aliyun/trading-ecs-paper-shadow-policy.jq
- deployment/aliyun/TRADING_ECS_HOST.md
proerror77
force-pushed
the
codex/trading-ecs-host-contract
branch
from
July 16, 2026 00:41
72367f9 to
0bf3a32
Compare
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
2 participants
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.
Outcome
Adds the reviewed bare-ECS deployment contract for a future Tokyo Ubuntu 26.04 trading host. It publishes a digest-pinned ACR artifact and supports only Paper or Shadow activation.
Safety boundary
Verification
Summary by CodeRabbit
New Features
Documentation
Tests