Security hardening: MarketPulse validation, safer JSON IPC, git-only deps - #8
Conversation
- Pin metabolic-ledger (and optional kinetic/neuromod) by git rev; drop path clones - Own MarketPulse/ReadoutPacket in-tree (corpus-ipc public main lacks binary_wire) - Reject non-finite/non-positive MarketPulse prices; soft-clamp vols to [0,1] - Default JSON IPC under XDG_RUNTIME_DIR or /tmp/limen-capital-$USER (LIMEN_JSON_IPC) - Add docs/SECURITY.md loopback threat model; refresh deps/PRE_PUSH/README Closes #1 #2 #3 #6
🤖 CodeAnt AI — Review Status
|
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
the
size:XXL
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change adds in-tree wire contracts, MarketPulse validation, scoped JSON IPC, pinned dependencies, security guidance, CI, and Beads repository automation. ChangesRuntime and dependency integration
Beads repository workflow
Continuous integration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 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 |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Security Hardening Review
This PR implements important security improvements for MarketPulse validation and IPC path handling. The changes move in the right direction, but several critical security vulnerabilities and validation gaps must be addressed before merge.
Critical Issues Found (7)
Security Vulnerabilities (3):
MarketPulse::decodeinbinary_wire.rsbypasses validation - defense-in-depth failureLIMEN_JSON_IPCenvironment variable lacks path traversal validation (both Rust and Julia)- Missing validation for ReadoutPacket float arrays (NaN/Inf)
Crash Risks (1):
4. Silent failure on IPC directory creation could cause confusing bind errors
Data Integrity (2):
5. Non-price MarketPulse fields not validated for finite values in Julia
6. Validation order inconsistency between Julia and Rust implementations
Required Actions
All identified issues must be resolved before merge:
- Add validation directly in
MarketPulse::decode(not just wrapper) - Validate
LIMEN_JSON_IPCto prevent path traversal attacks - Add finite checks for all MarketPulse signal fields
- Add finite checks for ReadoutPacket arrays
- Surface directory creation failures with clear errors
- Align validation order between Julia and Rust
The core security approach is sound, but these gaps could allow malformed data or malicious paths to compromise system integrity.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
![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: b2bd3104a1
ℹ️ 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".
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 33 files
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
🤖 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 @.beads/config.yaml:
- Around line 11-13: Update the configuration template near the no-db setting to
remove the `no-db` option or explicitly mark it unsupported, ensuring users
cannot configure JSONL-only mode and that the local Dolt database remains the
sole issue source.
- Around line 47-55: Update the secret-key documentation in the Integration
settings comment to require linear.api_key from LINEAR_API_KEY and github.token
from GITHUB_TOKEN only. Remove any wording that permits or describes storing
these secrets in the tracked config file, and explicitly state they must not be
stored there.
In @.beads/hooks/pre-commit:
- Around line 3-5: Update the pre-commit hook’s scanner invocation to resolve
aikido-local-scanner from a documented environment variable or PATH instead of
the hard-coded /home/raulmc/.local/bin path. Preserve the existing fail-fast
validation and error message behavior when the scanner cannot be found, and
continue passing REPO_ROOT to the pre-commit-scan command.
In @.beads/README.md:
- Around line 29-30: Update the “Sync with Dolt remote” workflow in the README
to run `bd dolt pull` before `bd dolt push`, documenting bidirectional
synchronization while preserving the existing publish step.
- Around line 63-64: Replace the mutable main-branch installer command in the
Beads installation instructions with a versioned release download, and add
checksum or signature verification before executing the script. Update the
installation flow in the “Install Beads” section so the remote content is never
piped directly into bash.
In @.gitignore:
- Around line 39-43: Update the Beads/Dolt ignore entries in the root .gitignore
by removing the global *.db pattern and replacing it with a pattern scoped to
the .beads/ directory. Preserve the existing .dolt/ and .beads-credential-key
entries.
In `@AGENTS.md`:
- Around line 32-42: Surround each affected Markdown fenced shell block with
blank lines to satisfy MD031: add spacing before and after the non-interactive
shell block in AGENTS.md lines 32-42, the mandatory Git block in AGENTS.md lines
81-88, and the mandatory Git block in CLAUDE.md lines 36-42; do not alter the
commands or prose.
- Around line 78-86: Update the mandatory session-close workflows in AGENTS.md
lines 78-86 and CLAUDE.md lines 33-41 to include bd dolt pull before issue work
and bd dolt push afterward, using the local Dolt database as the source of
truth. Ensure both workflows verify Dolt synchronization before the final git
pull --rebase and git push sequence.
In `@brain/market_types.jl`:
- Around line 38-44: Update the docstring for validate_market_pulse_fields to
remove the mutation marker and state that it returns a new validated vector.
Keep the existing validation behavior and function signature unchanged.
In `@brain/Project.toml`:
- Around line 21-23: The code uses the [sources] dependency feature which
requires Julia 1.11+, but the current documentation states Julia 1.10+
compatibility. Update the minimum Julia version requirement in two places:
brain/Project.toml at line 27 where the version constraint is specified, and
README.md at line 65 where the documentation states the minimum version. Change
both from Julia 1.10 to Julia 1.11+ to align with the [sources] block introduced
in brain/Project.toml lines 21-23. If Julia 1.10 compatibility must be
maintained instead, remove the [sources] section entirely and use a Julia
1.10-compatible dependency workflow.
In `@brain/test/runtests.jl`:
- Around line 242-256: Restrict the exception handling in the “TemporalFocus
(optional, Pkg git dep)” testset to only the package-not-found/load failure,
such as the relevant ArgumentError or Base.PkgId condition. Keep the
optional-package skip for that specific case, but rethrow all other exceptions
so failures in RegionRouter, ActivityRegion, or update_routing! propagate as
test failures.
In `@CLAUDE.md`:
- Around line 54-70: Replace the unfinished Build & Test, Architecture Overview,
and Conventions & Patterns placeholders in CLAUDE.md with repository-specific
guidance: document the actual Rust and Julia quality-gate commands, relevant
architecture, and project conventions. Ensure the instructions tell agents to
run the documented checks whenever code changes, or remove sections that cannot
be completed.
In `@docs/PRE_PUSH.md`:
- Line 17: Update the local test checklist entry in PRE_PUSH.md to use separate
repository-root subshell commands: run cargo test from execution, then invoke
Julia with the brain project while targeting brain/test/runtests.jl. Replace the
combined command with explicit subshells so both tests are runnable from the
repository root.
In `@docs/SECURITY.md`:
- Around line 55-57: Update the “Reporting” section in SECURITY.md to provide a
concrete private reporting channel, such as a direct GitHub Security Advisory
link or an explicit maintainer contact address. Keep the warning against
disclosing live credentials or private keys in public issues.
In `@docs/wire-protocol-v1.md`:
- Around line 62-63: Update the sibling wire README’s consumer guidance to
identify execution/src/binary_wire.rs as the source of MarketPulse and
ReadoutPacket instead of Limen-Neural corpus-ipc, keeping it consistent with the
ownership statement in the wire protocol documentation.
In `@execution/src/binary_wire.rs`:
- Around line 93-134: Share the duplicated 4-byte float decoding logic by adding
one private read_f32 helper near the packet decoding implementations, then use
it in both MarketPulse::decode and ReadoutPacket::decode for all float fields,
including readout and relevance. Remove the local closure and inline
byte-reading conversions while preserving the existing Result<String> error
behavior.
In `@execution/src/main.rs`:
- Around line 7-16: Document both JSON IPC endpoint fallback branches in
execution/src/main.rs around the usage comments: describe the XDG runtime path
and the /tmp/limen-capital-<uid>/signals.ipc fallback, including the UID
component. Update wire/README.md at the documented endpoint section to state
both defaults and the required directory mode.
In `@execution/src/wire.rs`:
- Around line 27-46: Extend decode_market_pulse to validate finiteness for every
decoded MarketPulse float, including confidence_signal, funding_rate,
liquidation_vol, liquidity_delta, l3_order_imbalance, gpu_temp_c, gpu_power_w,
gpu_util_pct, basys_buffer_load, dydx_oi_delta, and dydx_funding_rate, in
addition to prices and vols. Preserve the existing price positivity checks and
volume clamping, and return an error identifying any non-finite field before
returning the pulse.
- Around line 48-77: Update json_ipc_endpoint and default_tmp_json_ipc_dir to
construct fallback /tmp IPC directories using the current UID rather than $USER.
Before returning an ipc:// endpoint, validate that the directory exists as a
real directory, is not a symlink, and is owned by the current user; treat
create_dir_all or permission-setting failures as errors instead of returning an
endpoint. Ensure the socket is only bound after these ownership and permission
checks succeed.
In `@README.md`:
- Around line 70-74: Update the README integration-test command documentation by
removing the stale “corpus-ipc” fetch claim from the comment on the execution
Cargo test step, while retaining the valid metabolic-ledger dependency reference
and all commands unchanged.
In `@strategy/signal_broadcaster.jl`:
- Around line 37-42: Harden endpoint directory setup across both sites: in
strategy/signal_broadcaster.jl lines 37-42, create or validate an owned,
non-symlink directory with mode 0700 and abort on any failure instead of
continuing; in execution/src/main.rs lines 118-119, handle json_ipc_endpoint()
as fallible before calling connect, with execution/src/wire.rs updated so
endpoint setup errors are returned to the caller.
- Around line 34-35: Replace the USER-based fallback in
strategy/signal_broadcaster.jl:34-35 with the numeric OS UID rule and make the
Rust resolver identical. Update the fallback documentation in
strategy/signal_broadcaster.jl:8-10 and docs/wire-protocol-v1.md:49 to describe
the UID-based path. Revise docs/SECURITY.md:11 to identify the OS UID trust
boundary, docs/SECURITY.md:35-36 to state the safe JSON IPC default and required
permissions, and docs/SECURITY.md:40-46 to reflect reliable user scoping in
multi-user deployments.
- Around line 58-65: Update the SignalBroadcaster constructor’s endpoint
selection to use a conditional expression that calls default_json_ipc_endpoint()
only when endpoint is nothing; preserve explicit endpoint values unchanged
before binding.
🪄 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: 412f6bc4-071b-47df-a55f-a83dbd862f0e
⛔ Files ignored due to path filters (1)
execution/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
.beads/.gitignore.beads/README.md.beads/config.yaml.beads/hooks/post-checkout.beads/hooks/post-commit.beads/hooks/post-merge.beads/hooks/pre-commit.beads/hooks/pre-push.beads/hooks/prepare-commit-msg.beads/metadata.json.claude/settings.json.gitignoreAGENTS.mdCLAUDE.mdREADME.mdbrain/Project.tomlbrain/adapters.jlbrain/market_types.jlbrain/reservoir.jlbrain/test/runtests.jldocs/PRE_PUSH.mddocs/SECURITY.mddocs/deps.mddocs/wire-protocol-v1.mdexecution/Cargo.tomlexecution/src/bin/market_sim.rsexecution/src/binary_wire.rsexecution/src/lib.rsexecution/src/main.rsexecution/src/wire.rsstrategy/signal_broadcaster.jlwire/README.md
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (2)
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Usebdfor all task tracking; do not use TodoWrite, TaskCreate, markdown TODO lists, or MEMORY.md files. Runbd primefor workflow context and usebd rememberfor persistent knowledge.
Treat the local Dolt database under.beads/dolt/as the source of truth for issues. Usebd dolt push/pullfor synchronization; do not treat.beads/issues.jsonlas the wire protocol or runbd importduring normal operation.
Use non-interactive flags for shell operations to prevent prompts:cp -f,mv -f,rm -f/rm -rf,scp -o BatchMode=yes,ssh -o BatchMode=yes,apt-get -y, andHOMEBREW_NO_AUTO_UPDATE=1for Homebrew.
Before completing a work session, file follow-up issues, run quality gates when code changed, update issue statuses, pull with rebase, push successfully, verify status and committed changes, clean up, and provide handoff context.
Files:
execution/src/bin/market_sim.rsexecution/src/lib.rsbrain/Project.tomlexecution/src/main.rsCLAUDE.mdbrain/reservoir.jlwire/README.mddocs/wire-protocol-v1.mdstrategy/signal_broadcaster.jlexecution/src/wire.rsREADME.mdexecution/Cargo.tomlAGENTS.mddocs/SECURITY.mddocs/deps.mdbrain/adapters.jlexecution/src/binary_wire.rsdocs/PRE_PUSH.mdbrain/market_types.jlbrain/test/runtests.jl
*.{md,mdx}
📄 CodeRabbit inference engine (CLAUDE.md)
*.{md,mdx}: Usebdfor all task tracking; do not useTodoWrite,TaskCreate, or markdown TODO lists.
Runbd primeto obtain the detailed Beads command reference and session-close protocol.
Usebd rememberfor persistent knowledge; do not useMEMORY.mdfiles.
Files:
CLAUDE.mdREADME.mdAGENTS.md
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: rmems/Limen-Capital
Timestamp: 2026-07-31T14:54:16.843Z
Learning: Before ending a work session, file issues for remaining work, run quality gates when code changed, update issue statuses, pull with rebase, push successfully, verify the repository is up to date, clean up local state, and provide handoff context.
Learnt from: CR
Repo: rmems/Limen-Capital
Timestamp: 2026-07-31T14:54:16.843Z
Learning: Work is not complete until `git push` succeeds; if pushing fails, resolve the problem and retry.
🪛 markdownlint-cli2 (0.23.1)
CLAUDE.md
[warning] 37-37: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 41-41: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
AGENTS.md
[warning] 33-33: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 82-82: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 86-86: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🔇 Additional comments (31)
execution/src/binary_wire.rs (1)
1-92: LGTM!Also applies to: 136-153, 200-217
execution/src/lib.rs (1)
14-14: LGTM!execution/src/bin/market_sim.rs (1)
7-7: LGTM!brain/market_types.jl (1)
45-60: LGTM!Also applies to: 68-75
execution/src/wire.rs (2)
1-26: LGTM!Also applies to: 79-158
174-179: 🩺 Stability & AvailabilityNo change needed for edition compatibility.
execution/Cargo.tomltargets Rust edition 2021, wherestd::env::set_varandstd::env::remove_varare not newly unsafe.brain/test/runtests.jl (1)
32-63: LGTM!strategy/signal_broadcaster.jl (1)
153-153: LGTM!docs/SECURITY.md (3)
5-10: LGTM!Also applies to: 12-25, 30-34, 37-53
1-4: 🔒 Security & PrivacyREADME link to the security policy is present.
26-29: 🔒 Security & PrivacyNo change needed.
The documented CURVE behavior matches the Julia implementation:
ZMQ_CURVE=1can only enable whenZMQ_SERVER_KEYandZMQ_CLIENT_KEYare missing or invalid; otherwise the socket config and default plain TCP path are unchanged..beads/.gitignore (1)
1-71: LGTM!.beads/metadata.json (1)
1-7: LGTM!.claude/settings.json (1)
1-26: LGTM!AGENTS.md (1)
50-70: 📐 Maintainability & Code Quality | ⚡ Quick winKeep one authoritative Beads instruction block.
The manual block at Lines 3-14 and this generated block repeat
bd prime, quick references, and source-of-truth rules. Remove the duplicate Beads content while retaining the separate non-interactive shell guidance.
[ suggest_recommended_refactor].beads/hooks/post-checkout (1)
5-27: LGTM!.beads/hooks/post-commit (1)
1-3: LGTM!.beads/hooks/post-merge (1)
5-27: LGTM!.beads/hooks/pre-commit (1)
8-30: LGTM!.beads/hooks/pre-push (2)
5-27: LGTM!
2-3: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve Git LFS failures before running Beads.
Each hook can return success after
git lfsfails because the later Beads block becomes the script status. In particular, pre-push can publish references after required LFS transfer fails.
.beads/hooks/pre-push#L2-L3: exit immediately whengit lfs pre-push "$@"fails..beads/hooks/post-checkout#L2-L3: exit immediately whengit lfs post-checkout "$@"fails..beads/hooks/post-merge#L2-L3: exit immediately whengit lfs post-merge "$@"fails.Proposed status handling
git lfs pre-push "$@" +_lfs_exit=$? +[ "$_lfs_exit" -eq 0 ] || exit "$_lfs_exit"> Likely an incorrect or invalid review comment..beads/hooks/prepare-commit-msg (1)
1-25: LGTM!brain/adapters.jl (2)
5-15: LGTM!Also applies to: 19-25
32-36: 🗄️ Data Integrity & IntegrationNo change needed.
No repository callers read
limen_neural, serializeadapter_status(), or destructure the returned named tuple.brain/reservoir.jl (1)
18-25: LGTM!README.md (1)
10-23: LGTM!Also applies to: 182-182
docs/deps.md (2)
7-43: LGTM!Also applies to: 52-54, 56-76
55-55: 🗄️ Data Integrity & IntegrationNo docs change needed for the JSON IPC fallback.
All referenced layers use the same fallback path:
$XDG_RUNTIME_DIR/limen-capital/signals.ipcwhen available, otherwise/tmp/limen-capital-$USER/signals.ipc.> Likely an incorrect or invalid review comment.docs/PRE_PUSH.md (1)
15-15: LGTM!Also applies to: 19-32
execution/Cargo.toml (1)
9-15: 🗄️ Data Integrity & IntegrationNo action needed.
corpus-ipcreferences are limited to comments, whileipc-zmqandcorpus_ipcimports/features are absent from Rust and Cargo files.brain/Project.toml (1)
21-21: 🎯 Functional CorrectnessNo duplicate
[sources]header is present.
brain/Project.tomlhas a single[sources]table containing both package sources.> Likely an incorrect or invalid review comment.
Mirror corinth-canal / Limen-Neural CI style: pinned action SHAs, concurrency, rustfmt + clippy -D warnings + cargo test in execution/, and Julia matrix CPU unit tests for brain/ (no GPU). Also clean clippy/fmt so the new gates pass.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
- Validate all MarketPulse/ReadoutPacket floats inside binary_wire decode - Soft-clamp vols; reject non-finite signals and non-positive prices - JSON IPC: require ipc:// absolute paths, UID-scoped /tmp, fail on dir setup - Beads hooks: PATH-based Aikido, optional LFS, propagate LFS failures - Docs: binary_wire ownership, SECURITY advisory URL, UID contract
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 37 files
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
|
Fixed all 3 security/quality issues:
All fixes have been applied to the current PR branch. Pushed commits to |
- Kelly: fail-closed behavior for non-finite inputs (P1) - Rust CI: compile-check all features including kinetic/snn (P2) - Julia CI: activate brain project to validate dependencies (P2)
- Run brain CPU tests on Julia 1.12 only (drop 1.10/1.11) - Pin dtolnay/rust-toolchain to current stable tip (1.97.1+)
- Secure JSON IPC dirs (no symlink, UID ownership, getuid fallback) - Keep spike_helm loop alive on invalid MarketPulse frames - Align Julia compat/README/deps/CI on 1.12; rust-version 1.97 - Fail closed when AIKIDO_LOCAL_SCANNER is set but unusable - Stale ipc socket cleanup; scope .gitignore DBs to .beads/ - Docs: CURVE incomplete E2E, wire readout path, CLAUDE build cmds - Integration gate uses git pins instead of requiring sibling clones
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
docs/wire-protocol-v1.md (1)
62-63: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the readout-mapping reference.
Line 55 still directs readers to
corpus_ipc::readout_to_trade. Lines 62-63 state that the in-treeexecution/src/binary_wire.rsowns these wire types until corpus-ipc publishes equivalents. Point the mapping reference toexecution/src/binary_wire.rs::readout_to_tradeor remove the corpus-ipc reference.🤖 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/wire-protocol-v1.md` around lines 62 - 63, Update the readout-mapping reference in the documentation to point to execution/src/binary_wire.rs::readout_to_trade, matching the in-tree ownership of the wire types; alternatively, remove the outdated corpus_ipc::readout_to_trade reference while preserving the surrounding wire-type guidance.strategy/signal_broadcaster.jl (1)
85-89: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate explicit endpoints and resolve the default lazily.
SignalBroadcaster("tcp://127.0.0.1:1")andSignalBroadcaster("ipc://relative")can bind outside Rust’s accepted format, whileSignalBroadcaster(default_endpoint)still evaluatesdefault_json_ipc_endpoint()before selecting the explicit endpoint. Useendpoint === nothing ? default_json_ipc_endpoint() : validate_json_ipc_endpoint(endpoint).🤖 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 `@strategy/signal_broadcaster.jl` around lines 85 - 89, Update the SignalBroadcaster constructor to resolve the default endpoint lazily: call default_json_ipc_endpoint() only when endpoint === nothing, and otherwise pass the explicit endpoint through validate_json_ipc_endpoint(endpoint) before ZMQ.bind.docs/SECURITY.md (1)
26-29: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winClarify
ZMQ_CLIENT_KEY’s role.
docs/SECURITY.mdcurrently implies thatZMQ_CLIENT_KEYis a client public key, butbrain/spike_helm.jluses it as the server public key passed toZMQ.set_curve_serverkeybefore connecting/receiving. Document it as the server public key or rename the env variable to avoid unsafe key configuration.🤖 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/SECURITY.md` around lines 26 - 29, Update the CURVE environment-variable guidance in SECURITY.md and the corresponding brain/spike_helm.jl usage so ZMQ_CLIENT_KEY is consistently documented and configured as the server public key passed to ZMQ.set_curve_serverkey; alternatively rename the variable consistently if that is the established configuration direction, ensuring the documented key roles match the runtime behavior..beads/hooks/post-checkout (1)
13-24: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftEnforce
BEADS_HOOK_TIMEOUTfor the non-timeoutfallthrough in hook scripts.Several hooks add
BEADS_HOOK_TIMEOUTand runtimeoutwhen available, but theelsebranch still callsbd hooks runwithout any bound. A Beads command that runs longer can block checkout, merge, commit, push, or message preparation. Add a portable timeout path or fail clearly when the timeout cannot be enforced.🤖 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 @.beads/hooks/post-checkout around lines 13 - 24, Ensure the timeout configured by BEADS_HOOK_TIMEOUT is enforced in every hook’s fallback path: update .beads/hooks/post-checkout lines 13-24, .beads/hooks/post-merge lines 13-24, .beads/hooks/pre-commit lines 24-35, and .beads/hooks/pre-push lines 13-24. Replace each unbounded bd hooks run call with a portable timeout mechanism, or fail clearly before running when no timeout can be enforced; preserve the existing timeout exit handling and continuation behavior.
🤖 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/SECURITY.md`:
- Line 11: Update the security documentation for the JSON TradeSignal adapter
and the LIMEN_JSON_IPC override section to clarify that the same-UID isolation
guarantee applies only to default endpoints; alternatively, document and
implement ownership and permission validation for override sockets. Keep the
existing validated absolute-path and no-.. requirements accurate.
In `@execution/src/bin/market_sim.rs`:
- Line 51: Replace the u64::is_multiple_of call in the market simulation loop
with the existing-compatible modulo condition t % 50 == 0, unless the project
explicitly declares Rust 1.87+ support through its Cargo/toolchain
configuration.
In `@execution/src/kelly.rs`:
- Around line 57-61: Update the return documentation for the function containing
kelly_frac to state that the result is constrained to 0.0..=1.0, replacing the
outdated indication that values above 1.0 may be returned. Keep the
implementation and its existing clamp behavior unchanged.
- Around line 57-61: Update KellyCriterion::new and calculate_fraction to reject
invalid inputs before computing or clamping: require a positive payoff ratio b
and a probability within [0.0, 1.0], returning the existing safe zero result for
invalid values. Keep the finite-value validation and add regression tests
covering negative payoff ratios and out-of-range probabilities.
In `@execution/src/wire.rs`:
- Around line 211-219: Refactor json_ipc_endpoint to delegate endpoint selection
to a pure resolution helper that accepts the override value, while retaining
environment-variable access only inside json_ipc_endpoint. Update
json_ipc_respects_env to call the helper with the custom IPC URL instead of
mutating process-global environment state.
---
Outside diff comments:
In @.beads/hooks/post-checkout:
- Around line 13-24: Ensure the timeout configured by BEADS_HOOK_TIMEOUT is
enforced in every hook’s fallback path: update .beads/hooks/post-checkout lines
13-24, .beads/hooks/post-merge lines 13-24, .beads/hooks/pre-commit lines 24-35,
and .beads/hooks/pre-push lines 13-24. Replace each unbounded bd hooks run call
with a portable timeout mechanism, or fail clearly before running when no
timeout can be enforced; preserve the existing timeout exit handling and
continuation behavior.
In `@docs/SECURITY.md`:
- Around line 26-29: Update the CURVE environment-variable guidance in
SECURITY.md and the corresponding brain/spike_helm.jl usage so ZMQ_CLIENT_KEY is
consistently documented and configured as the server public key passed to
ZMQ.set_curve_serverkey; alternatively rename the variable consistently if that
is the established configuration direction, ensuring the documented key roles
match the runtime behavior.
In `@docs/wire-protocol-v1.md`:
- Around line 62-63: Update the readout-mapping reference in the documentation
to point to execution/src/binary_wire.rs::readout_to_trade, matching the in-tree
ownership of the wire types; alternatively, remove the outdated
corpus_ipc::readout_to_trade reference while preserving the surrounding
wire-type guidance.
In `@strategy/signal_broadcaster.jl`:
- Around line 85-89: Update the SignalBroadcaster constructor to resolve the
default endpoint lazily: call default_json_ipc_endpoint() only when endpoint ===
nothing, and otherwise pass the explicit endpoint through
validate_json_ipc_endpoint(endpoint) before ZMQ.bind.
🪄 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: c43e8974-f5b1-4986-afb2-4cc5b4e3624c
📒 Files selected for processing (18)
.beads/hooks/post-checkout.beads/hooks/post-merge.beads/hooks/pre-commit.beads/hooks/pre-push.github/workflows/ci.ymldocs/SECURITY.mddocs/deps.mddocs/wire-protocol-v1.mdexecution/src/bin/market_sim.rsexecution/src/bin/train_ghost.rsexecution/src/binary_wire.rsexecution/src/dydx.rsexecution/src/kelly.rsexecution/src/lib.rsexecution/src/main.rsexecution/src/wire.rsstrategy/signal_broadcaster.jlwire/README.md
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (1)
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Usebdfor all task tracking; do not use TodoWrite, TaskCreate, markdown TODO lists, or MEMORY.md files. Runbd primefor workflow context and usebd rememberfor persistent knowledge.
Treat the local Dolt database under.beads/dolt/as the source of truth for issues. Usebd dolt push/pullfor synchronization; do not treat.beads/issues.jsonlas the wire protocol or runbd importduring normal operation.
Use non-interactive flags for shell commands that may prompt:cp -f,mv -f,rm -f/rm -rf,scp -o BatchMode=yes,ssh -o BatchMode=yes,apt-get -y, andHOMEBREW_NO_AUTO_UPDATE=1for Homebrew.
Before ending a work session, file follow-up issues, run quality gates when code changed, update issue status, pull with rebase, push successfully, verify repository status, clean up, and provide handoff context. Work is not complete untilgit pushsucceeds.
Files:
execution/src/bin/market_sim.rsdocs/wire-protocol-v1.mdexecution/src/bin/train_ghost.rsexecution/src/main.rswire/README.mdexecution/src/kelly.rsdocs/SECURITY.mdexecution/src/dydx.rsexecution/src/lib.rsstrategy/signal_broadcaster.jldocs/deps.mdexecution/src/wire.rsexecution/src/binary_wire.rs
🪛 zizmor (1.28.0)
.github/workflows/ci.yml
[info] 38-38: action functionality is already included by the runner (superfluous-actions): use rustup and/or cargo in a script step
(superfluous-actions)
🔇 Additional comments (20)
.github/workflows/ci.yml (1)
1-89: LGTM!execution/src/bin/train_ghost.rs (1)
16-16: LGTM!Also applies to: 45-45, 69-70
execution/src/dydx.rs (1)
19-19: LGTM!Also applies to: 81-86, 176-185, 213-218
execution/src/kelly.rs (1)
65-65: LGTM!Also applies to: 156-159, 183-186
execution/src/binary_wire.rs (1)
13-13: LGTM!Also applies to: 115-173, 233-242, 273-276
execution/src/lib.rs (1)
14-14: LGTM!Also applies to: 112-113, 141-143
execution/src/bin/market_sim.rs (1)
7-7: LGTM!Also applies to: 38-38
execution/src/wire.rs (1)
27-100: LGTM!Also applies to: 196-209, 221-226
execution/src/main.rs (1)
7-10: LGTM!Also applies to: 120-120
wire/README.md (1)
6-6: LGTM!Also applies to: 35-35, 64-64
docs/deps.md (5)
7-9: 📐 Maintainability & Code QualityMake the sibling-checkout policy unambiguous.
Line 8 limits the prohibition to the “default workflow”. Confirm whether sibling path clones are fully unsupported after sibling-tree probing was removed. If they are unsupported, state that directly. If they remain a supported development exception, document the exact exception and setup steps.
28-43: LGTM!
56-76: LGTM!
55-55: 🗄️ Data Integrity & IntegrationNo documentation change needed.
Both runtimes prefer
$XDG_RUNTIME_DIR/limen-capital/signals.ipcand fall back to/tmp/limen-capital-<numeric os uid>/signals.ipc, so the documented default matches the implementations.
7-26: 🩺 Stability & AvailabilityVerify lockfile coverage for reproducible dependency resolution.
The Git revisions pin direct Limen-Neural dependencies, but registry dependencies can still resolve to different transitive versions. Confirm that
Cargo.lock,execution/Cargo.toml, andbrain/Manifest.toml/Brain.tomlare committed and that CI uses locked or manifest-based resolution.docs/SECURITY.md (1)
59-63: LGTM!.beads/hooks/post-checkout (1)
2-7: LGTM!.beads/hooks/post-merge (1)
2-7: LGTM!.beads/hooks/pre-commit (1)
2-18: LGTM!.beads/hooks/pre-push (1)
2-6: 🗄️ Data Integrity & IntegrationPreserve the pre-push ref stream for the downstream hook.
Git supplies pre-push ref updates through stdin, and
git lfs pre-pushreads that stream. The subsequentbd hooks run pre-push "$@"may therefore receive EOF. Confirm that the Beads pre-push handler does not read stdin; otherwise buffer the input and replay it to both commands. (git-scm.com)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4648290e07
ℹ️ 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".
PR babysit update (
|
| Gate | State |
|---|---|
| Rust (execution) | ✅ success |
| Julia 1.12 (brain CPU) | ✅ success |
| qlty check | ✅ no blocking issues |
| Unresolved review threads | 0 |
| CodeRabbit | ⏳ re-review queued |
| cubic | ⏳ in progress |
reviewDecision |
still CHANGES_REQUESTED (stale CodeRabbit opinionated review until re-review lands) |
@coderabbitai review
Local verification: cargo clippy/test --locked 22 pass; julia --project=brain test/runtests.jl pass; ./test_integration.sh 27 pass.
- Exclusive *.owner.lock before ipc bind (libzmq unlink steal) - Relevance must sum ~1 (±0.05); tests for range and sum - Ignore live dYdX network test in default cargo test / CI - Exact git/url pin compare (fixed-string, not loose regex) - neutral_trade uses overflow pair index; mag comment [0,1]
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
brain/test/runtests.jl (1)
242-256: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winNarrow the
catchto package-load failure only.The
catch eblock reports@test truefor every exception. It covers the optionalusing TemporalFocusload failure and also any real failure inRegionRouter,ActivityRegion, orupdate_routing!.TemporalFocusis now a pinned git dependency, so a regression in it passes silently.Wrap only the load in the
try, then run the assertions outside it.♻️ Proposed fix
`@testset` "TemporalFocus (optional, Pkg git dep)" begin - try + loaded = try `@eval` using TemporalFocus + true + catch e + `@info` "TemporalFocus not loaded (optional); skip" exception = e + false + end + if loaded router = TemporalFocus.RegionRouter(; n_regions=4, n_out=16) regions = [TemporalFocus.ActivityRegion(Float32(0.1 * i), Float32.(randn(16))) for i in 1:4] TemporalFocus.update_routing!(router, regions) w = router.routing_weights `@test` length(w) == 4 `@test` abs(sum(w) - 1.0f0) < 1.0f-3 `@test` all(w .>= 0.0f0) - catch e - `@info` "TemporalFocus not loaded (optional); skip" exception = e + else `@test` true end end🤖 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 `@brain/test/runtests.jl` around lines 242 - 256, Restrict the try/catch in the “TemporalFocus (optional, Pkg git dep)” testset to only the `@eval` using TemporalFocus package-load operation. Move RegionRouter construction, ActivityRegion creation, update_routing!, and all assertions outside the catch so failures in those symbols propagate and fail the test instead of being converted to `@test` true.
🤖 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/SECURITY.md`:
- Around line 48-50: Update acquire_ipc_owner_lock and the IPC owner-lock format
to store a per-process start identity alongside the PID, then validate both
against the current process entry when determining whether an existing owner is
alive. Ensure PID reuse is treated as stale without requiring
LIMEN_JSON_IPC_REPLACE=1, and update the documented lock behavior accordingly.
In `@strategy/signal_broadcaster.jl`:
- Line 109: Update the endpoint selection around `something` to avoid eagerly
calling `default_json_ipc_endpoint()` when `endpoint` is provided; use
conditional evaluation so explicit endpoints are used directly and the default
is computed only when `endpoint` is `nothing`.
- Around line 181-198: Update the exclusive lock creation in the `open` `ccall`
to use Julia’s variadic C-call form, passing the mode argument only when
required by `O_CREAT`. Replace the hard-coded `o_creat` and `o_excl` values with
platform-derived constants, or explicitly gate this path to Linux before using
them; preserve the existing `fd < 0` race-handling behavior.
- Around line 106-137: Update the SignalBroadcaster constructor to wrap all
setup after creating context and socket in a catch-based cleanup path. On any
failure from acquire_ipc_owner_lock, remove_unix_socket_if_present!, or
ZMQ.bind, close the socket and terminate the context, release owner_lock_path
when acquired, then rethrow the original exception; preserve successful
construction and existing bind error reporting.
---
Outside diff comments:
In `@brain/test/runtests.jl`:
- Around line 242-256: Restrict the try/catch in the “TemporalFocus (optional,
Pkg git dep)” testset to only the `@eval` using TemporalFocus package-load
operation. Move RegionRouter construction, ActivityRegion creation,
update_routing!, and all assertions outside the catch so failures in those
symbols propagate and fail the test instead of being converted to `@test` true.
🪄 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: fc396f66-d9fa-4345-aa49-395379d3d036
📒 Files selected for processing (8)
brain/market_types.jlbrain/test/runtests.jldocs/SECURITY.mdexecution/src/binary_wire.rsexecution/src/dydx.rsexecution/src/wire.rsstrategy/signal_broadcaster.jltest_integration.sh
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Rust (execution)
🧰 Additional context used
📓 Path-based instructions (5)
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Usebdfor all task tracking; do not use TodoWrite, TaskCreate, or markdown TODO lists. Runbd primefor detailed workflow context and usebd rememberfor persistent knowledge instead ofMEMORY.mdfiles.
Treat the local Dolt database under.beads/dolt/as the source of truth for issues. Usebd dolt push/pullfor synchronization viarefs/dolt/data; do not treat.beads/issues.jsonlas the wire protocol or runbd importduring normal operation.
Use non-interactive flags for shell operations to prevent confirmation prompts:cp -f,mv -f,rm -f, recursiverm -rf/cp -rf,scp -o BatchMode=yes,ssh -o BatchMode=yes,apt-get -y, andHOMEBREW_NO_AUTO_UPDATE=1for Homebrew.
Before ending a work session, file follow-up issues, run quality gates when code changed, update issue statuses, pull with rebase, push successfully, verify repository status and committed changes, clean up, and provide handoff context. Work is not complete untilgit pushsucceeds.
**/*: Usebdfor all task tracking; do not use TodoWrite, TaskCreate, or markdown TODO lists.
Runbd primefor the detailed Beads workflow and session-close protocol.
Usebd rememberfor persistent knowledge instead ofMEMORY.mdfiles.
Before completing a work session, file follow-up issues, run applicable quality gates, update issue statuses, pull with rebase, push successfully, verify repository status, clean up, and provide handoff context.
Files:
brain/test/runtests.jlexecution/src/dydx.rsbrain/market_types.jldocs/SECURITY.mdtest_integration.shexecution/src/binary_wire.rsstrategy/signal_broadcaster.jlexecution/src/wire.rs
brain/**/*.jl
📄 CodeRabbit inference engine (CLAUDE.md)
For Julia changes, run
julia --project=. test/runtests.jlfrombrain/.
Files:
brain/test/runtests.jlbrain/market_types.jl
execution/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
For Rust changes, run
cargo fmt --all -- --check,cargo clippy --all-targets --all-features --locked -- -D warnings, andcargo test --lockedfromexecution/.
Files:
execution/src/dydx.rsexecution/src/binary_wire.rsexecution/src/wire.rs
test_integration.sh
📄 CodeRabbit inference engine (CLAUDE.md)
Run
./test_integration.shas the structure and wiring quality gate.
Files:
test_integration.sh
execution/src/binary_wire.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain wire contracts in
execution/src/binary_wire.rsuntil they are exported bycorpus-ipc.
Files:
execution/src/binary_wire.rs
🔇 Additional comments (17)
docs/SECURITY.md (5)
1-20: LGTM!
22-33: LGTM!
53-60: LGTM!
62-66: LGTM!
68-74: LGTM!execution/src/binary_wire.rs (3)
285-299: LGTM!Also applies to: 315-317
337-355: LGTM!
238-257: 🗄️ Data Integrity & IntegrationNo change needed. The current in-tree NERO publisher normalizes relevance before aggregation/wire packing, and
wire/fixtures/readout.binsatisfies the 1.0 ± 0.05 sum check.> Likely an incorrect or invalid review comment.brain/market_types.jl (2)
38-60: LGTM!
62-79: LGTM!brain/test/runtests.jl (1)
32-63: LGTM!execution/src/dydx.rs (1)
226-241: LGTM!strategy/signal_broadcaster.jl (3)
26-91: LGTM!
140-180: LGTM!Also applies to: 208-238
295-307: LGTM!Also applies to: 328-328
test_integration.sh (2)
155-155: LGTM!
55-119: 🎯 Functional CorrectnessNo change needed.
_HEX7already accepts full 40-character hexadecimal revisions.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2262de45f
ℹ️ 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".
There was a problem hiding this comment.
12 issues found across 11 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="execution/src/wire.rs">
<violation number="1" location="execution/src/wire.rs:291">
P3: The new readout_overflow_scores_fail_closed test does not actually exercise the score-overflow path in isolation. The packet it builds has `relevance: [1.0; 4]` (sum = 4.0), which independently trips the defense-in-depth relevance check in readout_to_trade and guarantees a Neutral/zero-confidence result regardless of how overflow is handled. As written, the test would still pass even if the overflow guard were deleted, so it gives false confidence in the new fail-closed logic. Consider building the packet with normalized relevance (e.g. `[0.5, 0.25, 0.125, 0.125]`) so that only the bull/bear overflow drives the failure, making the test actually cover the intended path.</violation>
<violation number="2" location="execution/src/wire.rs:302">
P3: These out-of-range tests also violate the normalization rule, and `is_err()` cannot tell which validator rejected the frame; deleting the range check would leave both tests passing via the sum check. Checking the returned range error would make the new validation coverage effective.</violation>
</file>
<file name="execution/src/binary_wire.rs">
<violation number="1" location="execution/src/binary_wire.rs:253">
P3: The nominal lower boundary of the stated ±0.05 tolerance is rejected because f32 subtraction represents `abs(0.95 - 1.0)` slightly above `0.05`. Inclusive bounds such as `(0.95..=1.05).contains(&rel_sum)` in both decode and defense checks preserve the documented symmetric interval and also reject NaN/Inf.</violation>
<violation number="2" location="execution/src/binary_wire.rs:286">
P3: Ticker selection is now duplicated in `neutral_trade` and the normal return path, leaving two mappings to keep synchronized. A shared `ticker_for_pair(pair_index)` helper would remove the repeated branch and allocation policy.</violation>
</file>
<file name="docs/SECURITY.md">
<violation number="1" location="docs/SECURITY.md:13">
P3: Out-of-range volume frames do not fail closed; both decoders accept them after clamping. The trust-assumption cell should distinguish rejected non-finite/non-positive fields from normalized volumes.</violation>
</file>
<file name="test_integration.sh">
<violation number="1" location="test_integration.sh:60">
P2: The pin gate can accept an unpinned path dependency because `extract_quoted(..., "git")` and `extract_quoted(..., "rev")` also match suffixes such as `notgit` and `notrev`. Adding a left field boundary to `re` in both helper copies would make the exact-field assertion effective.</violation>
</file>
<file name="strategy/signal_broadcaster.jl">
<violation number="1" location="strategy/signal_broadcaster.jl:116">
P3: The positional `endpoint` argument to `SignalBroadcaster` bypasses `validate_json_ipc_endpoint`. Validation (ipc:// prefix, absolute, no `..` segments) is only applied inside `default_json_ipc_endpoint()` when resolving the env var; when a caller passes an endpoint directly, `ep[7:end]` is taken as `sock_path` and fed straight into the lock/socket-path handling with no path checks. If a caller passes `"ipc:///tmp/x/../victim"` the owner lock and unlink logic operate outside the intended directory. Consider validating any non-default endpoint in the constructor so the hardened path guarantees hold for every entry point.</violation>
<violation number="2" location="strategy/signal_broadcaster.jl:120">
P2: A rejected or unremovable socket path leaves the owner lock behind, so correcting the path and retrying in the same process is refused as a live owner. Release the acquired lock when stale-socket cleanup throws.</violation>
<violation number="3" location="strategy/signal_broadcaster.jl:143">
P2: PID reuse can block startup for the lifetime of an unrelated process because `/proc/<pid>` existence does not prove it created this lock. Persist and verify process start identity, or use a kernel lock whose ownership ends with the process.</violation>
<violation number="4" location="strategy/signal_broadcaster.jl:155">
P1: `LIMEN_JSON_IPC_REPLACE=1` can leave two active publishers: existing subscribers remain connected to the old ZMQ socket while new subscribers reach the replacement. A live takeover should coordinate shutdown and wait for the prior owner to exit, rather than only unlinking its paths.</violation>
<violation number="5" location="strategy/signal_broadcaster.jl:176">
P1: A replaced publisher can delete the current publisher's lock during its later shutdown, allowing a third process to steal the live endpoint without `REPLACE=1`. Release needs ownership tied to the acquired lock instance, not an unconditional pathname unlink.</violation>
<violation number="6" location="strategy/signal_broadcaster.jl:176">
P2: The owner-lock reclaim path has a check-then-rm-then-create (TOCTOU) race that can defeat the very exclusivity guarantee this PR adds. The flow reads the existing lock, decides it is stale, `rm`s it, and only then re-acquires it with `O_CREAT|O_EXCL`. Two contenders that both read a stale/absent PID can both pass the alive check; contender B's `rm` can then delete contender A's freshly-created lock (and A's live socket), after which B takes over and binds — i.e. the endpoint-steal scenario described in the header/docs is still reachable under concurrent startup or while a just-starting owner's PID isn't written yet (a lock read before the small `write` window yields `old_pid === nothing`, so the owner is treated as not live and is reclaimed). Since preventing a second publisher from stealing a live endpoint is the core goal of this change, the liveness check and the O_EXCL acquisition should be atomic: attempt `O_EXCL` create first, and only on EEXIST re-read the PID and decide whether to reclaim — never `rm` a lock you did not just acquire.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
- Hold flock(LOCK_EX|NB) for process lifetime (no PID/TOCTOU steal) - Cleanup ZMQ + release lock on all constructor failure paths - Lazy default endpoint; validate explicit ipc endpoints - Field-bounded git/rev extract; inclusive relevance sum bounds - Overflow test uses valid relevance simplex; range errs asserted
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be8942dfcb
ℹ️ 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".
There was a problem hiding this comment.
9 issues found across 11 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/ci.yml">
<violation number="1" location=".github/workflows/ci.yml:119">
P2: The new integration gate can pass dependencies that are not pinned to an unambiguous commit: `test_integration.sh` accepts any hex-looking `rev` of at least seven characters rather than a full commit SHA. Consider requiring exactly 40 hex characters (and ideally checking lockfile-resolved commits) so this security gate enforces the fixed-revision policy it advertises.</violation>
</file>
<file name="test_integration.sh">
<violation number="1" location="test_integration.sh:59">
P3: The awk `extract_quoted` helper is duplicated verbatim in both `assert_cargo_git_rev` and `assert_julia_source_rev`. Since this is a validation gate that must keep both crates and Julia sources in lockstep, keeping two copies invites drift — a fix applied to one (say, a stricter URL/quote matcher) won't be reflected in the other, silently weakening one side of the gate. Consider defining the shared awk helper once (e.g. a single `_AWK_EXTRACT` snippet interpolated into both invocations, or a small shared script) so both assertions parse fields identically.</violation>
<violation number="2" location="test_integration.sh:61">
P1: These checks can report git pins for unpinned TOML because comments and nested/dotted fields are treated as top-level `git`/`url` and `rev` fields. A TOML-aware parser, or strict extraction of only the outer inline table's real fields, would keep the integration gate fail-closed.</violation>
<violation number="3" location="test_integration.sh:71">
P2: The Cargo pin gate can pass based on an unrelated TOML table because this match is not restricted to `[dependencies]`. Tracking the active dependency table or inspecting parsed Cargo metadata would ensure the reported pin belongs to the actual dependency.</violation>
</file>
<file name="strategy/signal_broadcaster.jl">
<violation number="1" location="strategy/signal_broadcaster.jl:127">
P1: Rolling restart can disconnect a still-live pre-upgrade or foreign publisher: an unlocked sidecar proves nothing about owners that do not use it, yet this path unlinks every Unix socket. Check socket liveness or fail closed unless an explicit replacement opt-in is set.</violation>
<violation number="2" location="strategy/signal_broadcaster.jl:176">
P2: Forked/execed children can retain this raw descriptor; if the broadcaster exits without `shutdown`, that child keeps the flock live and blocks endpoint restart. Set close-on-exec atomically when opening the lock file.</violation>
<violation number="3" location="strategy/signal_broadcaster.jl:176">
P2: acquire_ipc_owner_lock hardcodes O_CREAT as 64, which is the Linux/glibc value (octal 0100). On macOS/BSD, O_CREAT is 0x200, so this open() call won't actually create the lock file on those platforms, causing the very first SignalBroadcaster at a fresh endpoint to fail with a missing lock file. Use a portable constant (e.g. Base.Filesystem flags or platform-conditional values) instead of the raw Linux value.</violation>
<violation number="4" location="strategy/signal_broadcaster.jl:177">
P2: The `.owner.lock` file is opened with O_RDWR|O_CREAT but no O_NOFOLLOW/O_EXCL, and is then immediately `ftruncate`d to 0 and overwritten with the PID. If an attacker can create a symlink at `<socket>.owner.lock` (possible in a LIMEN_JSON_IPC override directory, which — per the security model — is not auto-secured like the default 0700 dirs), `open` follows the symlink and the subsequent `ftruncate(fd, 0)` truncates the attacker-chosen target file. This is the same symlink-hardening the rest of this PR applies to the socket and directory paths, but is missing here. Opening with O_NOFOLLOW (and rejecting ELOOP) closes the gap.</violation>
<violation number="5" location="strategy/signal_broadcaster.jl:215">
P2: The IPC ownership lock can be stolen right at shutdown, which defeats the guarantee the docstring promises. `release_ipc_owner_lock` does flock(LOCK_UN) → close(fd) → then `rm` the `.owner.lock` file. Between the close and the rm, a second broadcaster can open the same lock file and acquire LOCK_EX and consider itself the owner; this process's rm then unlinks that file, leaving the second owner holding a lock on an *unlinked inode*. A third broadcaster subsequently opens a brand-new file at that path (O_CREAT) and acquires a lock on a different inode — so two 'owners' coexist and both proceed to `ZMQ.bind` the same `ipc://` endpoint, reintroducing exactly the live-socket-steal that flock was meant to prevent. The standard fix is to never delete the flock file (the flock/existence is the ownership), or to unlink it while still holding the fd open, before LOCK_UN.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…HA pin gate Do not unlink *.owner.lock on release (avoids dual-owner race). Portable O_CREAT plus Linux O_CLOEXEC|O_NOFOLLOW. Integration gate requires 40-char revs, [dependencies] scope, shared comment-stripping awk helper.
|
| Verdict | Items |
|---|---|
| Fixed | Lock release race (no unlink of *.owner.lock); portable O_CREAT + Linux O_CLOEXEC|O_NOFOLLOW; full 40-char SHA pin gate; [dependencies] section scope; comment-stripping + shared awk helper |
| Pushback | Unlink socket after flock held — intentional for cooperative Capital owners / crash recovery; non-flock foreign publishers out of protocol (docs/SECURITY.md) |
| Stale cubic | 8 older issues triaged false_positive / intended_behavior (CI integration job, soft-clamp wording, SECURITY override docs, MSRV comment, mawk, optional crate asserts, etc.) |
Verify: ./test_integration.sh 30/0; isolated flock smoke (second acquire blocked; lock file remains after release).
All 11 threads replied + resolved via GitHub MCP.
— Grok Build: Grok 4.5 (high)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ea80ec644
ℹ️ 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".
| try | ||
| ZMQ.close(broadcaster.socket) | ||
| ZMQ.term(broadcaster.context) | ||
| release_ipc_owner_lock(broadcaster.owner_lock_path, broadcaster.owner_lock_fd) |
There was a problem hiding this comment.
When ZMQ.close or ZMQ.term raises during shutdown, control jumps directly to the catch before this call, leaving owner_lock_fd open and the endpoint locked for the rest of the Julia process. A caller that handles the shutdown error cannot construct a replacement broadcaster at the same endpoint because acquire_ipc_owner_lock will continue rejecting it; move lock release and field reset into a finally block so every shutdown path releases ownership.
Useful? React with 👍 / 👎.
|
test |
1 participant
User description
Summary
First hardening PR for residual findings from the pre-push security audit.
[0, 1](Julia + Rust)/tmp/spikenaut_signals.ipc; default to$XDG_RUNTIME_DIR/limen-capital/signals.ipcor/tmp/limen-capital-$USER/withLIMEN_JSON_IPCoverridemetabolic-ledger(+ optional kinetic/neuromod) pinned byrev; no sibling path clones. Wire types live in-tree (execution/src/binary_wire.rs) because publiccorpus-ipcmain does not yet export MarketPulse/ReadoutPacketTest plan
cd execution && cargo test(19 tests pass)julia --project=brain test/runtests.jl(all Capital unit tests pass)../Limen-Neuralon the dependency pathCloses #1
Closes #2
Closes #3
Closes #6
Leaves open: #4 (ZAP allowlist), #5 (confidence docs), #7 (crate rename).
Summary by cubic
Hardens market/readout validation and isolates JSON IPC with lifetime owner-locks, safer defaults, and stricter endpoint checks. Owns wire types in-tree and pins Limen‑Neural deps by git
rev; CI sticks to Rust 1.97.1 and Julia 1.12; Kelly sizing fails closed.New Features
MarketPulse/ReadoutPacket: reject NaN/Inf and non‑positive prices; clamp vols to [0, 1]; relevance in [0, 1] and sum ~1 (±0.05) with inclusive bounds; fail closed on overflowed scores; tests updated;spike_helmonly advances the tick after a successful decode.ipc://+ absolute path with no..; default to$XDG_RUNTIME_DIR/limen-capital/signals.ipcor/tmp/limen-capital-$UID/signals.ipc; secure dir (0700, owner UID, no symlink); hold exclusive*.owner.lockviaflock(LOCK_EX|NB)for the process lifetime; do not unlink lock files on release; open locks withO_CLOEXEC|O_NOFOLLOW(portableO_CREAT); release lock and clean up ZMQ on constructor failures; lazy default endpoint; only unlink stale Unix sockets; never replace a live socket unlessREPLACE=1.-D warnings+ tests (all features; skip live dYdX); Julia 1.12 CPU tests; integration gate enforces git URLs with 40‑charrevpins (scoped to[dependencies]).Dependencies
metabolic-ledger(required) plus optionalkinetic-signals/neuromodby fixedrev; dropcorpus-ipc; ownMarketPulse/ReadoutPacketinexecution/src/binary_wire.rs.[sources]pinLiquidCortexandTemporalFocusbyurl+rev;[compat] julia = "1.12".Written for commit 8ea80ec. Summary will update on new commits.
CodeAnt-AI Description
Harden market data validation, isolate JSON IPC, and remove local dependency checkout requirements
What Changed
$XDG_RUNTIME_DIRor/tmp, with validatedLIMEN_JSON_IPCoverrides and no automatic replacement of active sockets.Impact
✅ Fewer malformed market frames reaching trading logic✅ Safer multi-user JSON signal IPC✅ No sibling dependency checkout required💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.