feat: initial Elixir SDK implementation#1
Conversation
fix: correct Context traffic eligibility, variable_value logic,
override handling, refresh cleanup, and JSONExpr array/null
handling. Fix Utils choose_variant boundary and to_number("").
Add context_test (102), evaluator_test (57), md5_test (14),
matcher_test (6). Rewrite murmur3_test (39) and assigner_test
(42) with parameterized canonical vectors.
- Add attrs_seq tracking for audience re-evaluation after attribute changes - Add audience_match_seq to Assignment struct for cache invalidation - Remove non-numeric goal property filtering in do_track - Clear exposed_experiments on set_override and set_custom_assignment - Clear all exposed_experiments on refresh (per cross-SDK spec) - Emit publish event immediately, HTTP call in background Task.start - Simplify publish/finalize handlers to return :ok - Update unit tests for new refresh/publish behavior Results: 295/295 unit tests, 137/138 cross-SDK tests (1 skip)
… module Remove accidentally tracked _build/ directory and unused elixir_wrapper modules. Add ABSmartly.Application module and LICENSE. Fix context and SDK.
WalkthroughAdds GitHub Actions CI, an HTTP client with retry and error handling, and an integration test against a local TCP server. Introduces Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (25)
deps/cowlib/src/cow_base64url.erl-39-45 (1)
39-45:⚠️ Potential issue | 🟠 MajorHandle invalid unpadded length (
rem 4 =:= 1) explicitly.Line 41 currently omits the
1branch, so malformed input with#{padding := false}crashes with acase_clauseinstead of failing in a controlled way.💡 Proposed fix
Enc = case Opts of #{padding := false} -> case byte_size(Enc1) rem 4 of 0 -> Enc1; 2 -> << Enc1/binary, "==" >>; - 3 -> << Enc1/binary, "=" >> + 3 -> << Enc1/binary, "=" >>; + 1 -> erlang:error(badarg) end; _ -> Enc1 end,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deps/cowlib/src/cow_base64url.erl` around lines 39 - 45, The case on "byte_size(Enc1) rem 4" inside the branch for Opts = #{padding := false} currently lacks a clause for the value 1 which causes a case_clause crash for malformed unpadded input; add an explicit branch for rem 4 == 1 inside that case (the scope handling Enc, Opts and Enc1) and handle it in a controlled way (for example by returning an error tuple like {error, invalid_unpadded_length} or raising a descriptive error) instead of letting the function crash.deps/cowlib/src/cow_date.erl-33-38 (1)
33-38:⚠️ Potential issue | 🟠 MajorReject invalid calendar dates in
parse_date/1.Line 34 currently accepts day
00, and the function does not reject impossible dates (for example, 31-Feb). This can return invalidcalendar:datetime()values downstream.💡 Proposed fix
parse_date(DateBin) -> - Date = {{_, _, D}, {H, M, S}} = http_date(DateBin), - true = D >= 0 andalso D =< 31, + Date = {{Y, Mo, D}, {H, M, S}} = http_date(DateBin), + true = D >= 1 andalso D =< 31, + true = calendar:valid_date({Y, Mo, D}), true = H >= 0 andalso H =< 23, true = M >= 0 andalso M =< 59, true = S >= 0 andalso S =< 60, %% Leap second. Date.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deps/cowlib/src/cow_date.erl` around lines 33 - 38, The current parse_date/1 path (where Date = {{_, _, D}, {H, M, S}} = http_date(DateBin)) allows day 0 and doesn't reject impossible dates; update validation to ensure month is 1..12 and day is within the actual days for that month (or simply call calendar:valid_date({Year, Month, Day}) using the year/month/day from http_date/1) and keep the time checks for H/M/S (H 0..23, M 0..59, S 0..60); replace the D >= 0 check with a proper calendar validity check so parse_date/1 rejects dates like 00 or 31-Feb and returns an error instead of an invalid calendar:datetime().deps/cowlib/include/cow_parse.hrl-1-83 (1)
1-83:⚠️ Potential issue | 🟠 MajorRemove third-party dependency source files from version control.
The
deps/directory contains Git-tracked source code for cowlib, cowboy, cowboy_telemetry, certifi, hackney, and others, but these packages are not declared inmix.exsor managed throughmix.lock. Although.gitignorespecifies/deps/should be ignored, previously committed files persist in the repository. Remove these files from Git history and restore normal dependency management through Mix, which will fetch and cache them automatically.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deps/cowlib/include/cow_parse.hrl` around lines 1 - 83, The repository has committed vendored dependency sources (e.g., cowlib, cowboy, certifi, hackney) under deps/, which should be ignored and managed by Mix; remove those files from Git tracking, add/ensure /deps/ is listed in .gitignore, stop tracking them with git rm --cached -r deps (or remove specific packages), commit the removal, and restore dependency management by running mix deps.get (ensuring mix.exs and mix.lock declare the deps) so dependencies are fetched from Hex/Git rather than stored in the repo.deps/cowlib/src/cow_cookie.erl-1-15 (1)
1-15:⚠️ Potential issue | 🟠 MajorKeep fetched dependency sources out of
deps/.This file appears to be upstream
cowlibcode checked into Mix’s dependency checkout directory. Unless you’re intentionally maintaining a fork, committingdeps/makes upgrades, CVE tracking, and reproducible installs much harder, and Mix can overwrite local edits there during normal dependency refreshes. Prefer pinning the dependency inmix.exs/mix.lockand keeping the checkout itself out of version control.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deps/cowlib/src/cow_cookie.erl` around lines 1 - 15, This commit includes upstream dependency source (module cow_cookie) under deps/, which should not be tracked; revert/remove the checked-in file(s) such as module cow_cookie and any other files under deps/, ensure the dependency is declared/pinned in mix.exs and present in mix.lock, stop tracking the deps/ checkout (e.g., remove from git index) and add appropriate entries to .gitignore so Mix-managed dependency checkouts aren’t committed, then run mix deps.get to restore the dependency from Hex/Git rather than keeping files like cow_cookie in the repository.deps/cowlib/src/cow_cookie.erl-175-192 (1)
175-192:⚠️ Potential issue | 🟠 MajorReject empty cookie names on both parse and serialise paths.
Line 182 treats a header with no
=as an empty-name cookie, and Line 357 never checks thatNameis non-empty before emitting the header. That means malformed inputs like<<"Secure">>or<<"=value">>come back as{ok, <<>>, ...}, and callers can also generate=valueon output. Please ignore or reject these cases instead of normalising them into an empty-name cookie.Suggested fix
parse_set_cookie(SetCookie) -> case has_non_ws_ctl(SetCookie) of true -> ignore; false -> {NameValuePair, UnparsedAttrs} = take_until_semicolon(SetCookie, <<>>), - {Name, Value} = case binary:split(NameValuePair, <<$=>>) of - [Value0] -> {<<>>, trim(Value0)}; - [Name0, Value0] -> {trim(Name0), trim(Value0)} - end, - case {Name, Value} of - {<<>>, <<>>} -> - ignore; - _ -> - Attrs = parse_set_cookie_attrs(UnparsedAttrs, #{}), - {ok, Name, Value, Attrs} - end + case binary:split(NameValuePair, <<$=>>) of + [Name0, Value0] -> + case {trim(Name0), trim(Value0)} of + {<<>>, _} -> + ignore; + {Name, Value} -> + Attrs = parse_set_cookie_attrs(UnparsedAttrs, #{}), + {ok, Name, Value, Attrs} + end; + [_] -> + ignore + end end. setcookie(Name, Value, Opts) -> - nomatch = binary:match(iolist_to_binary(Name), [<<$=>>, <<$,>>, <<$;>>, + NameBin = iolist_to_binary(Name), + true = byte_size(NameBin) > 0, + nomatch = binary:match(NameBin, [<<$=>>, <<$,>>, <<$;>>, <<$\s>>, <<$\t>>, <<$\r>>, <<$\n>>, <<$\013>>, <<$\014>>]), nomatch = binary:match(iolist_to_binary(Value), [<<$,>>, <<$;>>, <<$\s>>, <<$\t>>, <<$\r>>, <<$\n>>, <<$\013>>, <<$\014>>]), - [Name, <<"=">>, Value, attributes(maps:to_list(Opts))]. + [NameBin, <<"=">>, Value, attributes(maps:to_list(Opts))].Also applies to: 351-357
Dockerfile-30-49 (1)
30-49:⚠️ Potential issue | 🟠 MajorFinal container runs as root.
From Line 30 onward there is no
USERdirective, so the app process runs with root privileges.Suggested hardening patch
FROM elixir:1.15-alpine RUN apk add --no-cache libgcc libstdc++ +RUN addgroup -S app && adduser -S app -G app WORKDIR /app # Copy compiled build -COPY --from=build /app /app +COPY --from=build --chown=app:app /app /app WORKDIR /app/elixir-wrapper +USER app🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 30 - 49, The container currently runs as root because there's no USER instruction after setting WORKDIR and before CMD; fix by creating a non-root user and group (e.g., app user), chowning /app and /app/elixir-wrapper to that user, setting HOME if needed, and adding a USER <username> before the CMD so mix runs unprivileged; update the Dockerfile around the WORKDIR /app and WORKDIR /app/elixir-wrapper steps and ensure the new user has permissions to run mix and access the application files referenced by CMD ["mix","run","--no-halt"].config/config.exs-4-4 (1)
4-4:⚠️ Potential issue | 🟠 MajorConfiguration scope mismatch:
:elixir_wrappershould be:absmartly.At line 4 in
config/config.exs, the JSON library configuration is scoped to:elixir_wrapper, but the OTP application is defined as:absmartlyinmix.exs. This configuration will not be accessible to the application at runtime if any code attempts to read it viaApplication.get_env/2. While Jason is currently hardcoded throughout the codebase rather than being read from configuration, the incorrect scope should be corrected to match the declared application atom.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/config.exs` at line 4, The config entry is scoped to :elixir_wrapper but the OTP app is :absmartly, so update the application atom in the config call: change the config tuple from :elixir_wrapper to :absmartly while keeping the key :json_library and value Jason so Application.get_env(:absmartly, :json_library) will return the expected value; verify there are no other config entries using :elixir_wrapper and adjust them to :absmartly if present.Dockerfile-27-27 (1)
27-27:⚠️ Potential issue | 🟠 MajorSet
MIX_ENV=prodin runtime stage to match build environment.Line 27 compiles with
MIX_ENV=prod, but the runtime stage (from line 30) is isolated in a multi-stage build and does not inherit environment variables. Line 49 runsmix runwithout explicitly settingMIX_ENV=prod, causing the compiled code to run with the default environment (typicallydev), creating a mismatch between compile-time and runtime configuration.Add
ENV MIX_ENV=prodin the runtime stage before theCMDinstruction:Suggested fix
# Install hex and rebar in runtime (needed for mix) RUN mix local.hex --force && \ mix local.rebar --force +ENV MIX_ENV=prod # Expose port EXPOSE 3000 # Run the application CMD ["mix", "run", "--no-halt"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` at line 27, The runtime stage runs the application with `CMD` but doesn't inherit the build-time `MIX_ENV=prod` used in `RUN MIX_ENV=prod mix compile`, so set the environment explicitly in the runtime stage by adding `ENV MIX_ENV=prod` (before the `CMD` instruction) to ensure `mix run` executes in the same prod environment as the compile step..gitignore-7-8 (1)
7-8:⚠️ Potential issue | 🟠 MajorRemove vendored dependencies or document the vendoring strategy.
The repository commits 396 tracked files from
deps/to git despite.gitignoreexcluding/deps/. This contradictory setup creates ongoing maintenance burden:
- Repository bloat: Unnecessary dependency source files significantly increase clone size and repository size.
- Update burden: Dependencies require manual synchronisation rather than automatic fetching via
mix get deps.- Confusion: The presence of
deps/conflicts with the.gitignorerule, misleading contributors about the intended setup.The project already includes
mix.lockfor reproducible builds. Remove vendored dependencies and rely on Mix's standard dependency resolution, or document this decision if vendoring is genuinely required for your use case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore around lines 7 - 8, The repo currently tracks a large vendored deps/ directory despite .gitignore excluding /deps/; remove the committed vendored dependency files from version control and rely on Mix's dependency resolution (mix.lock + mix deps.get), or if vendoring is intentional, add a short documented policy explaining why deps/ is committed and update .gitignore accordingly; specifically, delete the tracked deps/ files from git (git rm -r --cached deps/ and commit the removal) and ensure mix.lock is present and CI/build scripts run mix deps.get, or add a VENDORING.md and keep deps/ tracked while removing /deps/ from .gitignore to avoid the current contradiction.deps/cowboy/src/cowboy_webtransport.erl-75-85 (1)
75-85:⚠️ Potential issue | 🟠 MajorOptional callbacks are currently treated as mandatory.
webtransport_handle/2andwebtransport_info/2are declared optional, but this call path still invokes them unconditionally. A handler that relies on that documented optionality will crash withundefon its first event or mailbox message.Suggested fix
handler_call(State=#state{handler=Handler}, HandlerState, Callback, Message) -> try case Callback of webtransport_init -> Handler:webtransport_init(HandlerState); - _ -> Handler:Callback(Message, HandlerState) + webtransport_handle when erlang:function_exported(Handler, webtransport_handle, 2) -> + Handler:webtransport_handle(Message, HandlerState); + webtransport_handle -> + {[], HandlerState}; + webtransport_info when erlang:function_exported(Handler, webtransport_info, 2) -> + Handler:webtransport_info(Message, HandlerState); + webtransport_info -> + {[], HandlerState} end ofAlso applies to: 197-205
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deps/cowboy/src/cowboy_webtransport.erl` around lines 75 - 85, The code declares webtransport_handle/2 and webtransport_info/2 as optional but calls them unconditionally; update the call sites (e.g., where webtransport_handle and webtransport_info are invoked in cowboy_webtransport.erl) to first check erlang:function_exported(HandlerModule, webtransport_handle, 2) and erlang:function_exported(HandlerModule, webtransport_info, 2) respectively and only call them when exported, otherwise use the appropriate default behavior (e.g., pass-through/unmodified state or no-op handling) so the process doesn't crash with undef; apply the same pattern for webtransport_init where optional and also fix the other occurrence around the 197-205 region.deps/cowlib/src/cow_hpack.erl-33-38 (1)
33-38:⚠️ Potential issue | 🟠 MajorDynamic-table name reuse never matches the stored tuple shape.
dyn_tableentries are stored as{EntrySize, {Name, Value}}, but this matcher expects[{Name, _}|_]. That clause can never hit, so repeated non-static header names are always re-encoded as brand-new names instead of indexed names, which bloats the block and breaks canonical vectors.Suggested fix
-table_find_name_dyn(Name, [{Name, _}|_], Index) -> Index; +table_find_name_dyn(Name, [{_, {Name, _}}|_], Index) -> Index;Also applies to: 927-929
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deps/cowlib/src/cow_hpack.erl` around lines 33 - 38, The dyn_table stored entries as {EntrySize, {Name, Value}} but the code patterns are matching [{Name, _}|_] so they never match; update all matchers that inspect the record state 'state' field 'dyn_table' (including the similar occurrences around lines 927-929) to expect the actual tuple shape {_, {Name, _}} (or use a variable like {EntrySize, {Name, Value}}) when checking for existing names, and adjust any comparisons that assumed the old shape accordingly so name reuse indexing will succeed.deps/cowboy/src/cowboy_webtransport.erl-186-188 (1)
186-188:⚠️ Potential issue | 🟠 MajorImplement missing OTP system callbacks required by
sys:handle_system_msg/6.The call to
sys:handle_system_msg/6at line 187 requires this module to definesystem_continue/3,system_terminate/4, andsystem_code_change/4. Without these callbacks, the code will fail with anundeferror when any system request is processed (suspend, code change, etc.).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deps/cowboy/src/cowboy_webtransport.erl` around lines 186 - 188, Implement the three OTP sys callbacks required by sys:handle_system_msg/6: add system_continue/3, system_terminate/4, and system_code_change/4 with the exact arities and make them operate on the paired state you pass into sys:handle_system_msg (the {State, HandlerState} tuple). Specifically, add system_continue(_Request, _From, {State, HandlerState}) -> {ok, {State, HandlerState}}; system_terminate(Reason, _Request, {State, HandlerState}, _Extra) -> ok (or perform any needed cleanup using Reason, State, HandlerState); and system_code_change(OldVsn, _Request, {State, HandlerState}, _Extra) -> {ok, {State, HandlerState}} (or transform State/HandlerState for code upgrades). Ensure the function names and arities match exactly so sys:handle_system_msg/6 can call them.deps/cowboy/Makefile-107-112 (1)
107-112:⚠️ Potential issue | 🟠 MajorDo not swallow
h2specbootstrap failures.
test-builddepends on$(H2SPEC), but both bootstrap steps end with|| true. A failed clone or build still marks the prerequisite as complete, so CI can go green without the conformance binary ever existing.Suggested fix
$(H2SPEC): $(gen_verbose) mkdir -p $(GOPATH)/src/github.com/summerwind - $(verbose) git clone --depth 1 https://github.com/summerwind/h2spec $(dir $(H2SPEC)) || true - $(verbose) $(MAKE) -C $(dir $(H2SPEC)) build MAKEFLAGS= || true + $(verbose) test -d $(dir $(H2SPEC))/.git || \ + git clone --depth 1 https://github.com/summerwind/h2spec $(dir $(H2SPEC)) + $(verbose) $(MAKE) -C $(dir $(H2SPEC)) build MAKEFLAGS= + $(verbose) test -x $(H2SPEC)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deps/cowboy/Makefile` around lines 107 - 112, The test-build rule currently swallows failures for the h2spec bootstrap by appending "|| true" to the git clone and the MAKE commands for the $(H2SPEC) prerequisite; remove the "|| true" from the two commands in the $(H2SPEC) recipe so that failures in the git clone or the $(MAKE) -C $(dir $(H2SPEC)) build step propagate and cause the target to fail (leave the mkdir -p as-is); update the $(H2SPEC) recipe that references git clone and the MAKE invocation accordingly so CI cannot mark the prerequisite complete when clone/build fail.deps/cowboy/src/cowboy_webtransport.erl-129-153 (1)
129-153:⚠️ Potential issue | 🟠 Major
upgrade/5crashes before it can return the advertised 501 response.The only clause hard-matches
version := 'HTTP/3',pid, andstreamid, so any other request dies withfunction_clauseand never reaches thefalsebranch. Unsupported requests should be rejected cleanly instead of crashing the stream process.Suggested fix
+upgrade(Req, Env, _Handler, _HandlerState, _Opts) -> + {ok, cowboy_req:reply(501, Req), Env}.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deps/cowboy/src/cowboy_webtransport.erl` around lines 129 - 153, The upgrade/5 clause currently hard-matches Req to include version := 'HTTP/3', pid := Pid and streamid := StreamID which causes a function_clause crash for non-HTTP/3 requests; change the function so it no longer requires those keys in the head (e.g., accept Req as a plain map and extract version, pid and streamid inside the body using maps:get/2 or pattern-match after binding) or add a fallback clause upgrade(Req, Env, Handler, HandlerState, Opts) -> {ok, cowboy_req:reply(501, Req), Env} that returns the 501 reply; ensure the existing successful path still extracts StreamID/Pid for the webtransport_init/2 path and uses is_upgrade_request/1 and webtransport_init(State, HandlerState) unchanged.deps/certifi/rebar.config-1-31 (1)
1-31:⚠️ Potential issue | 🟠 MajorRemove vendored
deps/files from source control.This commit adds 396 files to the
deps/certifi/,deps/cowboy/, anddeps/cowboy_telemetry/directories—entire vendored upstream packages that should be managed by Mix. Even though.gitignorecontains the/deps/rule, these files are being explicitly committed. The repository already declares these packages inmix.lock(certifi 2.15.0 and cowboy via hackney dependencies). Committing the vendored sources turns the repository into a fork of these packages, obscures SDK changes under third-party code, and risks version drift from Mix metadata. Remove these directories from the commit and let Mix fetch them from Hex duringmix deps.get.README.md-382-389 (1)
382-389:⚠️ Potential issue | 🟠 MajorDo not create the context unconditionally in
mount/3.LiveView mounts once for the disconnected HTTP render and again after the websocket connects. A
session_idgenerated inmount/3is not persisted back to the browser session. This results in two contexts being created for a single page view with different unit IDs on reconnects, which skews assignment and exposure data. Populate thesession_idin Plug/controller code and only create the context from that stable value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 382 - 389, The mount/3 implementation currently generates a session_id and unconditionally calls MyApp.ABSmartlyService.get_sdk() and SDK.create_context(sdk, units), which causes duplicate contexts when LiveView reconnects; instead, ensure a stable session_id is set before mount (populate session["session_id"] in your Plug or controller), remove unconditional generate_session_id/SDK.create_context from mount/3, and change mount/3 to only call MyApp.ABSmartlyService.get_sdk() and SDK.create_context(sdk, units) when session["session_id"] already exists (using the existing session_id value), keeping generate_session_id/assignment logic in the controller/plug so reconnects reuse the same context.deps/cowlib/src/cow_http2.erl-418-420 (1)
418-420:⚠️ Potential issue | 🟠 MajorEncode PRIORITY weight as wire value (
Weight - 1).The parser exposes weight as
1..256(Weight + 1), but the builder writesWeightdirectly. This makes encoded priorities off by one and cannot encode 256 correctly.💡 Proposed fix
-priority(StreamID, E, DepStreamID, Weight) -> +priority(StreamID, E, DepStreamID, Weight) when Weight >= 1, Weight =< 256 -> FlagExclusive = exclusive(E), - << 5:24, 2:8, 0:9, StreamID:31, FlagExclusive:1, DepStreamID:31, Weight:8 >>. + WireWeight = Weight - 1, + << 5:24, 2:8, 0:9, StreamID:31, FlagExclusive:1, DepStreamID:31, WireWeight:8 >>.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deps/cowlib/src/cow_http2.erl` around lines 418 - 420, The PRIORITY builder is encoding the Weight directly instead of the HTTP/2 wire value (weight - 1), causing off-by-one and inability to encode weight 256; update the priority/4 function so the final byte is the wire-encoded weight by writing (Weight - 1) into the 8-bit slot (ensure you use the existing variables StreamID, E, DepStreamID, Weight and keep FlagExclusive = exclusive(E)), and validate/clip or document that Weight==256 becomes wire value 255 so the encoded 8-bit field is correct.deps/cowboy/src/cowboy_stream_h.erl-93-97 (1)
93-97:⚠️ Potential issue | 🟠 MajorAuto-mode request body length is not accumulated.
Line 95’s TODO is a real bug:
body_lengthis left unchanged, so total body length can be wrong when the final chunk is delivered in auto mode.💡 Proposed fix
data(StreamID, IsFin, Data, State=#state{read_body_pid=Pid, read_body_ref=Ref, read_body_length=auto, body_length=BodyLen}) -> - send_request_body(Pid, Ref, IsFin, BodyLen, Data), + NewBodyLen = BodyLen + byte_size(Data), + send_request_body(Pid, Ref, IsFin, NewBodyLen, Data), do_data(StreamID, IsFin, Data, [{flow, byte_size(Data)}], State#state{ read_body_ref=undefined, - %% `@todo` This is wrong, it's missing byte_size(Data). - body_length=BodyLen + body_length=NewBodyLen });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deps/cowboy/src/cowboy_stream_h.erl` around lines 93 - 97, The auto-mode branch in do_data is not accumulating the incoming chunk size into State#state.body_length: when read_body_ref=undefined you must update body_length to BodyLen + byte_size(Data) (rather than leaving it as BodyLen) so the total request body is tracked correctly; modify the do_data clause that builds the State#state record (the one setting read_body_ref=undefined and body_length=BodyLen) to set body_length=BodyLen + byte_size(Data) using the existing BodyLen and byte_size(Data).deps/cowboy/src/cowboy_clear.erl-53-56 (1)
53-56:⚠️ Potential issue | 🟠 MajorHandle all
ranch:recv_proxy_header/2error variants.The
recv_proxy_header/2callback specification defines three return types:
{ok, ranch_proxy_header:proxy_info()}{error, closed | atom()}{error, protocol_error, atom()}The current code at lines 53–56 handles only
{ok, ProxyInfo}and{error, closed}. The missing handlers for{error, protocol_error, Reason}and other{error, Reason}atoms will cause acase_clausecrash instead of a controlled shutdown.💡 Proposed fix
get_proxy_info(Ref, #{proxy_header := true}) -> case ranch:recv_proxy_header(Ref, 1000) of {ok, ProxyInfo} -> ProxyInfo; - {error, closed} -> exit({shutdown, closed}) + {error, protocol_error, HumanReadable} -> + exit({shutdown, {protocol_error, HumanReadable}}); + {error, Reason} -> + exit({shutdown, Reason}) end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deps/cowboy/src/cowboy_clear.erl` around lines 53 - 56, The case handling the call to ranch:recv_proxy_header(Ref, 1000) currently only matches {ok, ProxyInfo} and {error, closed} and will crash on other error tuples; update that case to explicitly handle {error, protocol_error, Reason} (e.g., exit({shutdown, {protocol_error, Reason}}) or otherwise perform a controlled shutdown/cleanup) and add a catch‑all {error, Reason} branch that also performs a controlled shutdown (e.g., exit({shutdown, Reason})); locate the case around the call to ranch:recv_proxy_header/2 (the Ref/ProxyInfo usage) and add those two branches instead of leaving them unhandled.deps/cowboy/src/cowboy_children.erl-77-87 (1)
77-87:⚠️ Potential issue | 🟠 MajorCrash when
Shutdownisinfinityin per-child shutdown.The
shutdown/2function at line 82 passes the child'sShutdownvalue directly toerlang:start_timer(Shutdown, ...). The type specification for the child record (line 28) declaresshutdown :: timeout(), which includes both integers and the atominfinity. However,erlang:start_timer/3only accepts non-negative integers and will raisebadargif passedinfinity.The
before_terminate_loop/1function (lines 120–122) correctly handles this by checkingcase Time of infinity -> undefined; _ -> erlang:start_timer(...), butshutdown/2lacks this guard.Add the same infinity check to prevent the crash:
Ref = case Shutdown of infinity -> undefined; _ -> erlang:start_timer(Shutdown, self(), {shutdown, Pid}) end, Child#child{streamid=undefined, timer=Ref}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deps/cowboy/src/cowboy_children.erl` around lines 77 - 87, In shutdown/2, avoid calling erlang:start_timer/3 with Shutdown when it may be the atom infinity: update the clause matching `#child`{pid=Pid, streamid=StreamID, shutdown=Shutdown} inside shutdown/2 to compute Ref with a case on Shutdown (infinity -> undefined; _ -> erlang:start_timer(Shutdown, self(), {shutdown, Pid})) and then return Child#child{streamid=undefined, timer=Ref}; this mirrors the infinity handling in before_terminate_loop/1 and prevents badarg when Shutdown == infinity.deps/cowboy/src/cowboy_stream.erl-157-194 (1)
157-194:⚠️ Potential issue | 🟠 MajorRedact crash logs before emitting them.
make_error_log/5serialises the fullReq,Data,PartialReq,Resp,Optsand handlerState. On real crashes that can leak cookies, auth headers and request/response bodies, and it can also explode log volume on large payloads. Please log a redacted subset instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deps/cowboy/src/cowboy_stream.erl` around lines 157 - 194, The error logger is currently serialising full Req, Data, PartialReq, Resp, Opts and State in make_error_log/5 which can leak sensitive headers and large bodies; change make_error_log for init, data, info, terminate and early_error to emit a redacted summary instead of the raw terms by calling helper functions (e.g. redact_req(Req), redact_partial_req(PartialReq), redact_resp(Resp), redact_data(Data), redact_opts(Opts), redact_state(State)) that return only safe fields (method, path/uri, status, selected headers with authorization/cookie removed or replaced, and trimmed body up to a small byte/char limit). Ensure the helpers strip or mask sensitive header values (Authorization, Cookie, Set-Cookie), truncate long bodies, and are used in the argument lists passed to the log tuples so logs show the redacted summaries rather than full original terms.deps/cowboy/src/cowboy_websocket.erl-486-487 (1)
486-487:⚠️ Potential issue | 🟠 MajorTreat relay
finas an upstream close.
_IsFinis ignored here. When a relayed HTTP/2 or HTTP/3 stream ends after its last DATA frame, this WebSocket process stays alive until idle timeout instead of closing immediately and terminating the handler.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deps/cowboy/src/cowboy_websocket.erl` around lines 486 - 487, The current clause for {'$cowboy_relay_data', {Pid, _StreamID}, _IsFin, Data} ignores the _IsFin flag so an upstream stream EOF doesn't trigger immediate close; update this by matching the IsFin value (e.g., IsFin =:= true) and treat it as an upstream close: add a guarded branch or separate clause that when IsFin =:= true calls parse(?reset_idle_timeout(State), HandlerState, ParseState, Data) in a way that signals EOF (for example by passing an explicit EOF token or setting ParseState/HandlerState to indicate upstream-closed) so parse handles immediate close/termination, and keep the existing behavior when IsFin =:= false to just feed Data to parse. Ensure you reference the same tuple pattern {'$cowboy_relay_data', {Pid, _StreamID}, IsFin, Data} and reuse ?reset_idle_timeout(State), HandlerState and ParseState.deps/cowboy/src/cowboy_http2.erl-892-903 (1)
892-903:⚠️ Potential issue | 🟠 MajorKeep the
info/3result when switching to relay mode.
info(State0, ...)can changeflow, mutate the stream state, or remove the stream entirely. ReusingFlowandStreamsfromState0discards those changes and can accidentally resurrect a stream that just terminated.♻️ Suggested fix
-commands(State0=#state{flow=Flow, streams=Streams}, StreamID, +commands(State0, StreamID, [{switch_protocol, Headers, _Mod, ModState=#{data_delivery := relay}}|Tail]) -> - State1 = info(State0, StreamID, {headers, 200, Headers}), - #{StreamID := Stream} = Streams, + State1=#state{flow=Flow1, streams=Streams1} = + info(State0, StreamID, {headers, 200, Headers}), + #{StreamID := Stream} = Streams1, #{data_delivery_pid := RelayPid} = ModState, %% WINDOW_UPDATE frames updating the window will be sent after %% the first DATA frame has been received. RelayFlow = maps:get(data_delivery_flow, ModState, 131072), - State = State1#state{flow=Flow + RelayFlow, streams=Streams#{StreamID => Stream#stream{ + State = State1#state{flow=Flow1 + RelayFlow, streams=Streams1#{StreamID => Stream#stream{ status={relaying, RelayFlow, RelayPid}, flow=RelayFlow}}}, commands(State, StreamID, Tail);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deps/cowboy/src/cowboy_http2.erl` around lines 892 - 903, The call to info(State0, ...) can change State0 (flow/streams), but the code later reuses Flow and Streams from State0 when constructing State, potentially resurrecting a removed stream; update the code to use the updated State1 values instead (e.g., pattern match State1 = `#state`{flow = NewFlow, streams = NewStreams} and then use NewFlow and NewStreams when fetching Stream and when building State = State1#state{flow = NewFlow + RelayFlow, streams = NewStreams#{StreamID => Stream#stream{...}}}), so all mutations from info/3 are preserved.deps/cowboy/src/cowboy.erl-112-141 (1)
112-141:⚠️ Potential issue | 🟠 MajorSurface QUIC listener start failures instead of hanging startup.
The parent waits only for
{ok, Listener}with no timeout or error handling. Ifquicer:listen/2fails or the spawned listener process crashes before sending that message,start_quic/3blocks indefinitely during application startup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deps/cowboy/src/cowboy.erl` around lines 112 - 141, The listener helper currently spawns a process that only sends Parent ! {ok, Listener} on success which can hang startup; change the spawned listener code around quicer:listen/2 to catch failures and explicitly send Parent ! {error, Reason} (or {exit, Reason}) when quicer:listen/2 fails or the listener process crashes, and update the caller (start_quic/3) to do a guarded receive that accepts {ok, Listener} or {error, Reason} with a sensible timeout (and handle the error case/propagate it) instead of waiting indefinitely; also consider monitoring or trapping exits from the spawned listener and forwarding exit notifications to Parent so startup never blocks.deps/cowboy/src/cowboy_quicer.erl-220-226 (1)
220-226:⚠️ Potential issue | 🟠 MajorErase closed stream refs from the process dictionary.
The connection process stores
{quicer_stream, StreamID}entries when streams are opened (lines 103 and 210), but the stream_closed handler at lines 220–222 never removes them. On long-lived HTTP/3 connections this means memory grows with every completed stream until the connection process terminates.♻️ Suggested fix
handle({quic, stream_closed, StreamRef, #{error := ErrorCode}}) -> {ok, StreamID} = quicer:get_stream_id(StreamRef), + erase({quicer_stream, StreamID}), {stream_closed, StreamID, ErrorCode};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deps/cowboy/src/cowboy_quicer.erl` around lines 220 - 226, The handler for stream_closed events in cowboy_quicer does not remove the related {quicer_stream, StreamID} entry from the process dictionary, causing memory leaks. To fix this, update the handle function matching {quic, stream_closed, StreamRef, #{error := ErrorCode}} to call erase({quicer_stream, StreamID}) after retrieving StreamID from quicer:get_stream_id/1, ensuring the stream reference is removed from the process dictionary when the stream is closed.
🟡 Minor comments (10)
deps/cowlib/README.asciidoc-9-9 (1)
9-9:⚠️ Potential issue | 🟡 MinorUse the standard protocol casing (
WebSocket).Line 9 uses “Websocket”; the canonical spelling is “WebSocket”. Please update for consistency with protocol naming.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deps/cowlib/README.asciidoc` at line 9, Update the documentation text that currently reads "Websocket" to use the standard protocol casing "WebSocket" (replace the occurrence of the token "Websocket" with "WebSocket" in the README.asciidoc content).deps/cowlib/README.asciidoc-16-16 (1)
16-16:⚠️ Potential issue | 🟡 MinorSupport channel reference appears outdated.
Line 16 points to
irc.freenode.net, which is likely stale for current community support and may send users to the wrong place. Please verify and update this endpoint (or remove the IRC line if no longer maintained).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deps/cowlib/README.asciidoc` at line 16, The README line "Official IRC Channel: `#ninenines` on irc.freenode.net" is likely outdated; locate that exact string in deps/cowlib/README.asciidoc and either update the endpoint to the current support channel (e.g., replace irc.freenode.net with the project's actual current IRC/Matrix/Discord server) or remove the IRC line entirely if the channel is no longer maintained; ensure the final text points to a verified, active support channel or omits stale IRC references.deps/cowlib/src/cow_capsule.erl-50-61 (1)
50-61:⚠️ Potential issue | 🟡 MinorAdd length validation to prevent badarg crash on malformed close_session frames.
When
Len1is less than 4 bytes (which can occur since the 6-bit encoding allows values 0–63 and the 14-bit encoding allows 0–1028 without a lower bound check),AppMsgLenbecomes negative. In Erlang, using a negative size in a binary pattern match (AppMsg:AppMsgLen/binary) raises abadargexception rather than failing the match gracefully. This causes the parser to crash on malformed input instead of returningerror.Add a guard clause to validate that
Len1 >= 4:Proposed fix
case LenOrError of - {Len1, Rest2} -> + {Len1, Rest2} when Len1 >= 4 -> AppMsgLen = Len1 - 4, case Rest2 of <<AppCode:32, AppMsg:AppMsgLen/binary, Rest/bits>> -> {ok, {wt_close_session, AppCode, AppMsg}, Rest}; _ -> more - end; + end; + {_, _} -> + error; error -> error end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deps/cowlib/src/cow_capsule.erl` around lines 50 - 61, The parser currently computes AppMsgLen = Len1 - 4 without validating Len1, which can be negative and cause a badarg at the binary pattern for AppMsg in the wt_close_session branch; update the case on LenOrError (the {Len1, Rest2} branch) to only proceed when Len1 >= 4 (e.g., use a guard on Len1) and otherwise return an appropriate failure (error or more as per surrounding logic) so the match won't attempt a negative-sized binary; ensure you reference and protect the wt_close_session handling code that uses AppMsgLen and Rest..gitignore-29-37 (1)
29-37:⚠️ Potential issue | 🟡 MinorDuplicate
.DS_Storeentry.
.DS_Storeappears on both line 29 and line 37. Remove the duplicate.🔧 Proposed fix
# Editor and OS files .DS_Store .vscode/ .idea/ *.swp *.swo *~ .claude/ -.DS_Store AUDIT_REPORT.md🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore around lines 29 - 37, Remove the duplicate .DS_Store entry from the .gitignore so it only appears once; edit the .gitignore and delete the redundant ".DS_Store" line (the second occurrence) to keep the ignore file tidy.deps/cowboy_telemetry/README.md-16-20 (1)
16-20:⚠️ Potential issue | 🟡 MinorClose the
cowboy:start_clear/3call in the example.The snippet is missing the final
)and won’t compile if copied as-is.✏️ Proposed doc fix
cowboy:start_clear(http, [{port, Port}], #{ env => #{dispatch => Dispatch}, stream_handlers => [cowboy_telemetry_h, cowboy_stream_h] -}. +}).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deps/cowboy_telemetry/README.md` around lines 16 - 20, The example call to cowboy:start_clear/3 is missing its closing parenthesis which makes the snippet invalid; update the README example so the cowboy:start_clear(http, [{port, Port}], #{ env => #{dispatch => Dispatch}, stream_handlers => [cowboy_telemetry_h, cowboy_stream_h] }) call is properly closed (ensure the final ')' is added after the options map) so that the example compiles and reflects the correct usage of cowboy:start_clear, Dispatch, Port, cowboy_telemetry_h and cowboy_stream_h.deps/certifi/README.md-11-12 (1)
11-12:⚠️ Potential issue | 🟡 MinorFix intro sentence grammar and hyphenation.
The sentence is missing a verb and reads awkwardly for readers.
✏️ Proposed doc fix
-This an Erlang specific port of [certifi](https://certifi.io/). The CA bundle +This is an Erlang-specific port of [certifi](https://certifi.io/). The CA bundle🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deps/certifi/README.md` around lines 11 - 12, Fix the intro sentence by adding the missing verb and correct hyphenation: change "This an Erlang specific port of [certifi](https://certifi.io/)." to "This is an Erlang-specific port of [certifi](https://certifi.io/)." so the sentence reads correctly and "Erlang-specific" is hyphenated.deps/cowboy_telemetry/hex_metadata.config-7-7 (1)
7-7:⚠️ Potential issue | 🟡 MinorUse the canonical SPDX licence identifier.
The current form
Apache 2.0is not a valid SPDX identifier. Hex packaging tooling expects SPDX identifiers, and the canonical form for Apache License 2.0 isApache-2.0(with hyphen, not space).Proposed metadata fix
-{<<"licenses">>,[<<"Apache 2.0">>]}. +{<<"licenses">>,[<<"Apache-2.0">>]}.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deps/cowboy_telemetry/hex_metadata.config` at line 7, The license metadata entry for the tuple key <<"licenses">> uses a non-SPDX value "Apache 2.0"; update that entry to use the canonical SPDX identifier by replacing the value with "Apache-2.0" so hex packaging recognizes the licence correctly (i.e., change the licenses list element for <<"licenses">> from the space form to the hyphenated SPDX form).README.md-352-353 (1)
352-353:⚠️ Potential issue | 🟡 MinorAvoid showing
publish/1immediately beforefinalize/1.This README already says
finalize/1flushes pending events. Showing both back-to-back here implies callers must do extra work and may trigger duplicate flush attempts if the implementation is not strictly idempotent. Keeping onlyfinalize/1makes the examples clearer.Suggested doc fix
- Context.publish(context) Context.finalize(context)- Context.publish(context) Context.finalize(context)Also applies to: 479-480
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 352 - 353, Example snippets in the README show Context.publish(context) immediately before Context.finalize(context), which implies redundant/duplicate flushing; remove the Context.publish(context) call in those examples (the instances where publish is shown back-to-back with finalize, including the other occurrence noted) and leave only Context.finalize(context) so the examples reflect the intended single flush behavior and avoid suggesting unnecessary duplicate calls.README.md-348-348 (1)
348-348:⚠️ Potential issue | 🟡 MinorNormalise the
user-agentheader before storing it as an attribute.The Phoenix controller example at line 348 stores the direct result of
get_req_header(conn, "user-agent"), which is a list (e.g.["Mozilla/5.0"]), rather than a string. This inconsistency with the correct Plug example shown later (lines 460–466) can break string-based audience rules. Extract and normalise the header value using pattern matching:Suggested doc fix
- Context.set_attribute(context, "user_agent", get_req_header(conn, "user-agent")) + user_agent = + case get_req_header(conn, "user-agent") do + [ua | _] -> ua + [] -> "unknown" + end + + Context.set_attribute(context, "user_agent", user_agent)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 348, The README example stores the raw get_req_header(conn, "user-agent") list into Context.set_attribute which yields a list instead of a string; update the Phoenix controller snippet so you pattern-match the header (e.g., case get_req_header(conn, "user-agent") do [ua | _] -> ua; _ -> "" end) and normalize it (trim and/or downcase as required) before calling Context.set_attribute(context, "user_agent", <normalized_string>) so the attribute is always a string like in the Plug example.deps/cowboy/src/cowboy_req.erl-831-864 (1)
831-864:⚠️ Potential issue | 🟡 MinorStrip
content-lengthfrom 204 replies before sending them.The sendfile fast-paths add a
content-lengthbefore the 204/304 guard runs, anddo_reply_ensure_no_body/4keeps caller-supplied headers unchanged. That allowsreply(204, ...)to emit an invalidcontent-lengthheader.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deps/cowboy/src/cowboy_req.erl` around lines 831 - 864, The sendfile fast-paths can add a content-length header before the 204/304 guard, allowing reply(204, ...) to send an invalid header; fix this by removing the <<"content-length">> header inside do_reply_ensure_no_body/4 (use maps:remove or equivalent) so that when do_reply_ensure_no_body calls do_reply it always passes Headers without content-length (and still perform the existing iolist_size check and exit on non-zero body).
🧹 Nitpick comments (4)
deps/cowlib/src/cow_date.erl (1)
175-186: Add malformed-date rejection tests for parser boundaries.Current tests cover successful parsing well, but they do not assert rejection for invalid inputs like day
00or non-existent dates. Please add explicit negative cases to lock this behaviour down.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deps/cowlib/src/cow_date.erl` around lines 175 - 186, Add explicit negative test cases to the date parser tests: in http_date_test_ (and optionally prop_http_date) include malformed inputs such as <<"Sun, 00 Nov 1994 08:49:37 GMT">>, <<"Sun, 31 Feb 1994 08:49:37 GMT">>, <<"Sun, 06 Nov 1994 08:49:37 GMTX">>, <<"Sun, 06-13-94 08:49:37 GMT">>, etc., and assert the parser rejects them by calling parse_date(DateBin) (or catch http_date(DateBin)) and verifying it returns false/undefined or raises an error rather than producing a valid Date; update the Tests list in http_date_test_ to include these malformed tuples and replace the current fun() -> R = http_date(V) end checks with assertions that the parser fails for those inputs while keeping existing valid cases unchanged.deps/cowlib/src/cow_capsule.erl (1)
1-26: This is vendored dependency code from cowlib.This file is part of the cowlib library by Loïc Hoguin, a well-established Erlang HTTP library. As vendored code, modifications should generally be avoided unless necessary for bug fixes. Consider whether this should be tracked via a dependency manager (e.g.,
mix.exsdeps) rather than committed directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deps/cowlib/src/cow_capsule.erl` around lines 1 - 26, This file is vendored from the cowlib project; do not modify or commit changes directly in cow_capsule. Instead restore the original upstream file and remove local edits, then declare cowlib as a managed dependency (e.g., add/restore the cowlib entry in mix.exs and run mix deps.get) so parse/1, wt_drain_session and wt_close_session remain upstream-managed; if a bugfix is required, apply it upstream or maintain a small patch under version control (e.g., a patch file or fork) rather than editing the vendored cow_capsule module in-tree..gitignore (1)
42-44: Consider whetherCHANGELOG.mdandCONTRIBUTING.mdshould be ignored.
CHANGELOG.mdandCONTRIBUTING.mdare typically version-controlled files that users expect to find in a repository. Ignoring them means contributors cannot see contribution guidelines, and users cannot track version history. If these were generated by tooling and are meant to be temporary, consider using a different naming convention (e.g., prefixed with underscore or placed in a temp directory).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore around lines 42 - 44, The .gitignore currently lists CHANGELOG.md and CONTRIBUTING.md (and DOCUMENTATION_CHECKLIST.md); remove CHANGELOG.md and CONTRIBUTING.md from .gitignore so they remain tracked unless they are intentionally generated/temporary—if they are generated, instead rename or move them (e.g., _CHANGELOG.md or a temp/ directory) and update the .gitignore entry to match that generated name; update the .gitignore to reflect the chosen approach and ensure the repository tracks stable files like CHANGELOG.md and CONTRIBUTING.md.deps/cowboy/src/cowboy_dynamic_buffer.hrl (1)
44-50: Keep the moving-average documentation aligned with the implementation.The comment still describes a
/2moving average, while Line 68 applies a weighted/8formula. Updating the comment will prevent future misreads during tuning.Also applies to: 68-68
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deps/cowboy/src/cowboy_dynamic_buffer.hrl` around lines 44 - 50, The comment describing the moving-average in cowboy_dynamic_buffer.hrl is out of sync with the implementation: update the prose to reflect the weighted /8 formula actually used (the implementation computes the new average by weighting the previous MovAvg heavier and incorporating DataLen, e.g. effectively (MovAvg * 7 + DataLen) div 8) instead of saying it uses (MovAvg + DataLen) div 2; mention the weighted-N form or the specific /8 weighting so future readers tuning the buffer see the real behavior and rationale.
… fix attribute storage to list model
…messages - Add close/1 as alias for finalize/1 - Add is_closed?/1 as alias for is_finalized?/1 - Add is_closing?/1 as alias for is_finalizing?/1 - Standardize finalized error messages to "ABsmartly Context is finalized." - Standardize unit UID error messages to include unit type - Update tests to match standardized error messages
…ublic refresh() Refresh tests were calling Context.refresh(ctx, data) with data directly, bypassing the HTTP data fetching layer. Added data_fetcher option to Context.start_link/4, updated handle_call(:refresh) to use it when set, and refactored do_refresh to accept already-parsed ContextData structs. Updated all refresh tests to use start_context_with_refresh and the no-arg public Context.refresh() API.
…refresh, variable_keys as map
- treatment returns 0 (not error) when context is finalized
- variable_value returns default_value (not error) when context is finalized
- do_refresh_with_context_data selectively invalidates exposed_experiments only for changed/removed experiments instead of blanket reset
- variable_keys returns %{key => [experiment_name, ...]} map instead of flat key list
- build_indexes deduplicates experiments per variable key across variants
- Fix reverse/2 to walk constraints in reverse order in cowboy_constraints - Re-enter parse loop after body-stream crash in cowboy_http - Add exhaustive match for ranch:recv_proxy_header/2 return values in cowboy_tls and cowboy_clear - Reject whitespace before header colon to prevent HTTP smuggling in cow_http1 - Fix copyright attribution in cowboy_telemetry LICENSE
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
deps/cowboy/src/cowboy_constraints.erl (1)
34-35:reverse/2spec can be inconsistent on empty constraints.Line 35 promises
{ok, binary()}, but Line 45 to Line 46 returns{ok, Value}unchanged for[], which can be non-binary. Please make the contract explicit.💡 Proposed fix (spec aligned with current behaviour)
--spec reverse(any(), constraint() | [constraint()]) - -> {ok, binary()} | {error, reason()}. +-spec reverse(any(), constraint() | [constraint()]) + -> {ok, any()} | {error, reason()}.Also applies to: 45-46
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deps/cowboy/src/cowboy_constraints.erl` around lines 34 - 35, The spec for reverse/2 currently promises {ok, binary()} but the implementation returns {ok, Value} unchanged for an empty constraints list which may not be a binary; update the type contract to match actual behaviour by changing the return type to {ok, any()} | {error, reason()} (or a more specific term() if you prefer) so the spec aligns with the implementation of reverse/2 and the constraint() handling for [].
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deps/cowboy/src/cowboy_constraints.erl`:
- Around line 60-61: The guard on apply_constraint(Type, Value, F) currently
uses is_function(F) which allows any arity and can cause a badarity crash when
calling F(Type, Value); change the guard to is_function(F, 2) so only
two-argument functions are accepted, ensuring the call F(Type, Value) is safe
and prevents runtime badarity errors in the apply_constraint/3 clause.
In `@deps/cowboy/src/cowboy_http.erl`:
- Around line 549-556: The guards on the two parse_uri clauses (the clauses that
call parse_uri_authority/2) are using OR (`;`) where AND (`,`) is required,
which lets malformed schemes through; change the guard expressions in those
parse_uri function clauses so each character comparison is combined with commas
(H comparisons, T comparisons, P comparisons, and S comparisons) so the first
clause only matches "http" and the second only matches "https", or alternatively
replace the guards with an explicit check that the scheme equals "http" or
"https" before calling parse_uri_authority; update the two parse_uri clauses
referencing parse_uri_authority to use the corrected guards.
---
Nitpick comments:
In `@deps/cowboy/src/cowboy_constraints.erl`:
- Around line 34-35: The spec for reverse/2 currently promises {ok, binary()}
but the implementation returns {ok, Value} unchanged for an empty constraints
list which may not be a binary; update the type contract to match actual
behaviour by changing the return type to {ok, any()} | {error, reason()} (or a
more specific term() if you prefer) so the spec aligns with the implementation
of reverse/2 and the constraint() handling for [].
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6c6c0f2b-eef7-4e65-8592-63f8613447f6
📒 Files selected for processing (6)
deps/cowboy/src/cowboy_clear.erldeps/cowboy/src/cowboy_constraints.erldeps/cowboy/src/cowboy_http.erldeps/cowboy/src/cowboy_tls.erldeps/cowboy_telemetry/LICENSEdeps/cowlib/src/cow_http1.erl
🚧 Files skipped from review as they are similar to previous changes (2)
- deps/cowboy/src/cowboy_tls.erl
- deps/cowboy_telemetry/LICENSE
| apply_constraint(Type, Value, F) when is_function(F) -> | ||
| F(Type, Value). |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and read the relevant section
if [ -f "deps/cowboy/src/cowboy_constraints.erl" ]; then
echo "File found. Reading lines around 60-61..."
sed -n '50,75p' deps/cowboy/src/cowboy_constraints.erl
else
echo "File not found at expected path"
git ls-files | grep -i constraint
fiRepository: absmartly/elixir-sdk
Length of output: 671
🏁 Script executed:
# Let's also look for any test files related to this
git ls-files | grep -i constraint | head -20Repository: absmartly/elixir-sdk
Length of output: 102
🏁 Script executed:
# Search for usages of apply_constraint to understand the impact
rg "apply_constraint" --type erlang -A 2 -B 2Repository: absmartly/elixir-sdk
Length of output: 212
🏁 Script executed:
# Search for apply_constraint in all files
fd . deps/cowboy/src -type f | xargs grep -l "apply_constraint"Repository: absmartly/elixir-sdk
Length of output: 236
🏁 Script executed:
# Look at more context around the function definition
sed -n '40,100p' deps/cowboy/src/cowboy_constraints.erl | cat -nRepository: absmartly/elixir-sdk
Length of output: 1965
🏁 Script executed:
# Search for test files
fd -e erl_test -o -e test.erl -o -name "*test*" | head -20Repository: absmartly/elixir-sdk
Length of output: 180
🏁 Script executed:
# Check if there are any places where apply_constraint is called with functions
rg "apply_constraint" --type-listRepository: absmartly/elixir-sdk
Length of output: 453
🏁 Script executed:
# Look at the complete test section
sed -n '48,150p' deps/cowboy/src/cowboy_constraints.erl | cat -nRepository: absmartly/elixir-sdk
Length of output: 3311
🏁 Script executed:
# Search for is_function usage elsewhere in the file
grep -n "is_function" deps/cowboy/src/cowboy_constraints.erlRepository: absmartly/elixir-sdk
Length of output: 122
🏁 Script executed:
# Look at the full file to see if there are comments about constraint function requirements
cat -n deps/cowboy/src/cowboy_constraints.erl | head -20Repository: absmartly/elixir-sdk
Length of output: 1089
Guard custom constraint callbacks to arity 2 to avoid badarity crashes.
Line 60 uses is_function(F) which accepts functions of any arity, but calls F(Type, Value) with exactly two arguments. A constraint function with the wrong arity will crash instead of returning a proper error response.
Change the guard to is_function(F, 2) to enforce the correct arity:
Proposed fix
-apply_constraint(Type, Value, F) when is_function(F) ->
- F(Type, Value).
+apply_constraint(Type, Value, F) when is_function(F, 2) ->
+ F(Type, Value);
+apply_constraint(_, _, F) when is_function(F) ->
+ {error, invalid_constraint_arity}.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| apply_constraint(Type, Value, F) when is_function(F) -> | |
| F(Type, Value). | |
| apply_constraint(Type, Value, F) when is_function(F, 2) -> | |
| F(Type, Value); | |
| apply_constraint(_, _, F) when is_function(F) -> | |
| {error, invalid_constraint_arity}. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deps/cowboy/src/cowboy_constraints.erl` around lines 60 - 61, The guard on
apply_constraint(Type, Value, F) currently uses is_function(F) which allows any
arity and can cause a badarity crash when calling F(Type, Value); change the
guard to is_function(F, 2) so only two-argument functions are accepted, ensuring
the call F(Type, Value) is safe and prevents runtime badarity errors in the
apply_constraint/3 clause.
| parse_uri(<< H, T, T, P, "://", Rest/bits >>, State, Method) | ||
| when H =:= $h orelse H =:= $H, T =:= $t orelse T =:= $T; | ||
| P =:= $p orelse P =:= $P -> | ||
| parse_uri_authority(Rest, State, Method); | ||
| parse_uri(<< H, T, T, P, S, "://", Rest/bits >>, State, Method) | ||
| when H =:= $h orelse H =:= $H, T =:= $t orelse T =:= $T; | ||
| P =:= $p orelse P =:= $P; S =:= $s orelse S =:= $S -> | ||
| parse_uri_authority(Rest, State, Method); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify this is the actual guard syntax in the file and check if there are tests covering scheme validation
# Check the current guard implementation
echo "=== Current guard implementation ==="
rg -n -A2 'parse_uri\(<< H, T, T, P' deps/cowboy/src/cowboy_http.erl
# Check for any existing tests that might cover scheme validation
echo -e "\n=== Looking for scheme-related tests ==="
rg -l 'parse_uri|http://' --glob '*test*' deps/cowboy/ 2>/dev/null || echo "No test files found in deps/cowboy/"
# Check if upstream Cowboy has this same code
echo -e "\n=== Check for any scheme validation tests in the codebase ==="
rg -n 'httz://|aaap://' deps/ 2>/dev/null || echo "No malformed scheme tests found"Repository: absmartly/elixir-sdk
Length of output: 631
Guard logic is incorrect — invalid URI schemes would be accepted.
The guard expressions use ; (OR) where , (AND) is required. In Erlang guards, , binds tighter than ;, so the current guards parse as:
- Lines 550–551:
((H=h/H AND T=t/T) OR (P=p/P)) - Lines 554–555:
((H=h/H AND T=t/T) OR (P=p/P) OR (S=s/S))
This incorrectly accepts malformed schemes:
"aaap://"passes because P matches (H and T are not required)"xxxxxs://"passes because S matches (H, T, P are not required)
Per RFC 7230, only http and https schemes are valid in absolute-form request-targets.
Proposed fix
parse_uri(<< H, T, T, P, "://", Rest/bits >>, State, Method)
- when H =:= $h orelse H =:= $H, T =:= $t orelse T =:= $T;
- P =:= $p orelse P =:= $P ->
+ when (H =:= $h orelse H =:= $H),
+ (T =:= $t orelse T =:= $T),
+ (P =:= $p orelse P =:= $P) ->
parse_uri_authority(Rest, State, Method);
parse_uri(<< H, T, T, P, S, "://", Rest/bits >>, State, Method)
- when H =:= $h orelse H =:= $H, T =:= $t orelse T =:= $T;
- P =:= $p orelse P =:= $P; S =:= $s orelse S =:= $S ->
+ when (H =:= $h orelse H =:= $H),
+ (T =:= $t orelse T =:= $T),
+ (P =:= $p orelse P =:= $P),
+ (S =:= $s orelse S =:= $S) ->
parse_uri_authority(Rest, State, Method);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deps/cowboy/src/cowboy_http.erl` around lines 549 - 556, The guards on the
two parse_uri clauses (the clauses that call parse_uri_authority/2) are using OR
(`;`) where AND (`,`) is required, which lets malformed schemes through; change
the guard expressions in those parse_uri function clauses so each character
comparison is combined with commas (H comparisons, T comparisons, P comparisons,
and S comparisons) so the first clause only matches "http" and the second only
matches "https", or alternatively replace the guards with an explicit check that
the scheme equals "http" or "https" before calling parse_uri_authority; update
the two parse_uri clauses referencing parse_uri_authority to use the corrected
guards.
Align the IN operator with the collector and the other ABsmartly SDKs by using haystack-first operand order ([haystack, needle]) — IN is a CONTAINS check. Register a 'contains' operator key as an alias for the same implementation, keeping 'in' for backwards compatibility with existing audiences. Tests updated to the haystack-first convention.
emit_event called the event handler via Task.start (async), so events such as goal tracking were not observable immediately after the producing call returned. Deliver synchronously so track()/treatment() events are collected deterministically.
start_link_async initialized the context with ready=false but never triggered a data fetch, so wait_until_ready blocked until timeout. Kick off the fetch via handle_continue, running it in a spawned process so the context stays responsive (set_unit/set_failed/wait_until_ready) while the request is in flight, then mark ready (or failed) and reply to pending waiters when it completes.
…ndling) A null operand short-circuits eq to null — it does not treat null == null as a match. This aligns with the other ABsmartly SDKs and the collector. Update the operator tests accordingly.
…eady The async init always performed its own HTTP fetch in handle_continue and emitted ready, while callers that also fetched + set_data produced a second ready event. Let start_link_async/3 accept a :data_fetcher option, store it in the async init state, and reuse the existing handle_continue fetch path so the context performs exactly one fetch and emits exactly one ready event.
Pedro-Revez-Silva
left a comment
Pedro-Revez-Silva
left a comment
There was a problem hiding this comment.
Approved per request. The SDK repo has ExUnit tests, but no GitHub workflow, and the Dockerfile depends on cross-sdk-tests/elixir-wrapper rather than shipping a self-contained wrapper here. The cross-SDK wrapper also bypasses the top-level ABSmartly.SDK.create_context* API for normal scenarios by calling ABSmartly.Context.start_link/start_link_async and set_failed directly. Please add CI and align the wrapper with the public SDK API.
publish_events posted to <endpoint>/events, which is not a collector route (the collector exposes PUT /v1/context), and omitted X-Application-Version and X-Agent. So publish never reached the backend. Change to PUT <endpoint>/context and add the two missing headers, matching the canonical wire contract and the other SDKs. Surfaced by the new local-server integration test.
:gen_tcp local server + public SDK exercises the real HTTPoison client doing GET /context (create_context) and PUT /context (publish), asserting the wire contract. This test caught the wrong publish endpoint fixed in the prior commit. Add CI workflow (push + pull_request) running mix test.
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 (8)
lib/absmartly/json_expr/evaluator.ex (2)
263-265: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAvoid logging evaluated text on regex timeouts.
texthere comes from audience variables, so Line 264 can write user attribute data straight into application logs. Please log only non-sensitive metadata, such as text length or a hash.Suggested fix
Logger.error( - "Regex timeout (#{`@regex_timeout_ms`}ms): pattern=#{pattern}, text=#{String.slice(text, 0, 100)}" + "Regex timeout (#{`@regex_timeout_ms`}ms): pattern=#{pattern}, text_length=#{String.length(text)}" )🤖 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 `@lib/absmartly/json_expr/evaluator.ex` around lines 263 - 265, The regex timeout log in Evaluator should not include evaluated audience text, since it may expose user attributes in application logs. Update the Logger.error call in the regex timeout branch to remove String.slice(text, 0, 100) and log only non-sensitive metadata such as text length or a hash, while keeping the pattern and timeout context intact.
72-95: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon't halt
and/oron the firstnil.This makes evaluation order-dependent. For example,
%{"or" => [%{"var" => "missing"}, %{"value" => true}]}returnsniltoday, andABSmartly.Matcher.evaluate/2then turns that into a non-match because it checks== true.andhas the same problem in the other direction with laterfalseoperands. Please keep scanning afterniland only returnnilwhen no later operand decides the result; add regression cases for[nil, true]and[nil, false].Suggested fix
defp eval_and(exprs, vars) when is_list(exprs) do - Enum.reduce_while(exprs, true, fn expr, _acc -> + Enum.reduce_while(exprs, :all_true, fn expr, acc -> case Utils.to_boolean(evaluate(expr, vars)) do false -> {:halt, false} - nil -> {:halt, nil} - _ -> {:cont, true} + nil -> {:cont, :saw_nil} + _ -> {:cont, acc} end - end) + end) + |> case do + :all_true -> true + :saw_nil -> nil + other -> other + end end defp eval_or(exprs, vars) when is_list(exprs) do - Enum.reduce_while(exprs, false, fn expr, _acc -> + Enum.reduce_while(exprs, :all_false, fn expr, acc -> case Utils.to_boolean(evaluate(expr, vars)) do true -> {:halt, true} - nil -> {:halt, nil} - _ -> {:cont, false} + nil -> {:cont, :saw_nil} + _ -> {:cont, acc} end - end) + end) + |> case do + :all_false -> false + :saw_nil -> nil + other -> other + 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 `@lib/absmartly/json_expr/evaluator.ex` around lines 72 - 95, The eval_and and eval_or logic in Evaluator should not stop at the first nil result, since that makes boolean evaluation order-dependent. Update eval_and and eval_or to keep scanning remaining operands after Utils.to_boolean returns nil, only returning nil if no later operand produces a decisive true/false value; preserve the existing short-circuit behavior for true in eval_or and false in eval_and. Add regression coverage around evaluate/2 for cases like [nil, true] and [nil, false] to verify the final result is decided by later operands.lib/absmartly/http/client.ex (1)
31-35: 🔒 Security & Privacy | 🟠 MajorDisable automatic redirects on both requests
with_retry/3already treats 3xx responses as errors, sofollow_redirect: truebypasses that branch. It can also sendX-API-Keyto the redirected host. Handle redirects manually here instead of relying on automatic following.🤖 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 `@lib/absmartly/http/client.ex` around lines 31 - 35, The HTTP request in the anonymous request function currently enables automatic redirects, which bypasses the existing 3xx error handling in with_retry/3 and can leak the X-API-Key to another host. Update the request logic around HTTPoison.get so redirects are not followed automatically, and handle any redirect responses manually in the retry flow for both requests while preserving the existing query-string construction and timeout settings.lib/absmartly/context.ex (5)
467-480: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winApply finalisation checks to attributes and overrides too.
After
finalize/1,set_attribute(s)andset_override(s)still mutate the closed context, unlike units, custom assignments, tracking, publishing, and refresh. That makes the finalised state inconsistent and timing-dependent.Suggested guard pattern
def handle_call({:set_attribute, name, value}, _from, state) do - name_str = to_string(name) - entry = %{name: name_str, value: value, set_at: now_millis()} - state = %{state | attributes: state.attributes ++ [entry], attrs_seq: state.attrs_seq + 1} - {:reply, :ok, state} + if state.finalized do + {:reply, {:error, "ABsmartly Context is finalized."}, state} + else + name_str = to_string(name) + entry = %{name: name_str, value: value, set_at: now_millis()} + state = %{state | attributes: state.attributes ++ [entry], attrs_seq: state.attrs_seq + 1} + {:reply, :ok, state} + end endApply the same guard to
set_attributes,set_override, andset_overrides.Also applies to: 503-521
🤖 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 `@lib/absmartly/context.ex` around lines 467 - 480, The finalize guard is missing from the attribute and override setters, so a closed context can still be mutated after finalize/1. Update the handle_call clauses for set_attribute, set_attributes, set_override, and set_overrides in context.ex to follow the same finalized-state check used by units, custom assignments, tracking, publishing, and refresh, and return the appropriate error without changing state when the context is finalized.
861-879: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDrop the oldest queued item, not the newest one.
Both queues are prepended with
[event | state.queue], soEnum.drop(queue, 1)removes the newest previously queued event. Under queue pressure this keeps stale entries and drops fresher exposure/goal data.Suggested direction
- %{state | goals: Enum.drop(state.goals, 1), goal_count: state.goal_count - 1} + %{state | goals: drop_oldest(state.goals), goal_count: state.goal_count - 1}defp drop_oldest([_ | _] = items) do items |> Enum.reverse() |> tl() |> Enum.reverse() endFor exposures, also remove the dropped exposure name from
exposed_experimentsif you want it to be eligible for re-queueing later.Also applies to: 1032-1040
🤖 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 `@lib/absmartly/context.ex` around lines 861 - 879, The queue trimming logic in queue_exposure (and the similar goal queue path referenced by the review) is dropping the newest previously queued item because the queue is built in reverse order with [event | state.queue]. Update the overflow branch to remove the oldest item from the tail instead of using Enum.drop/1, and make sure the matching exposed_experiments entry is also cleared when an exposure is evicted so it can be queued again later.
650-660: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftMove the refresh fetch out of the GenServer callback.
The default fetch can block on HTTP timeouts/retries while
handle_call(:refresh, ...)owns the GenServer, preventing other context calls such astrack,publish, orfinalizefrom being served. Use the same off-process fetch-and-reply pattern as async initialisation.🤖 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 `@lib/absmartly/context.ex` around lines 650 - 660, Move the refresh fetch out of handle_call(:refresh, ...) in ABSmartly.Context so the GenServer does not block while waiting on HTTP timeouts or retries. Reuse the same off-process fetch-and-reply pattern used by the async initialization path: delegate the work to a separate process/task, then reply back to the GenServer once the fetch completes. Update the :refresh handling in ABSmartly.Context to keep track of the caller and return only after the fetch result is received, preserving access for track, publish, and finalize calls.
1434-1441: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winInvalidate cached assignments when all assignment inputs change.
assign_variant/2depends onunit_type,seed_hi,seed_lo, traffic seeds,audience, andaudience_strict, but refresh invalidation ignores those fields. A refresh that changes any of them can keep stale assignments and exposure state.Suggested fields to include in both comparisons
old_exp.traffic_split != new_exp.traffic_split -> true old_exp.split != new_exp.split -> true + old_exp.unit_type != new_exp.unit_type -> true + old_exp.seed_hi != new_exp.seed_hi -> true + old_exp.seed_lo != new_exp.seed_lo -> true + old_exp.traffic_seed_hi != new_exp.traffic_seed_hi -> true + old_exp.traffic_seed_lo != new_exp.traffic_seed_lo -> true + old_exp.audience != new_exp.audience -> true + old_exp.audience_strict != new_exp.audience_strict -> true true -> falseAlso applies to: 1455-1465
🤖 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 `@lib/absmartly/context.ex` around lines 1434 - 1441, The cached assignment invalidation check is missing several inputs that affect assign_variant/2, so refreshes can reuse stale assignments and exposure state. Update the comparison logic in the old_exp/new_exp refresh path and the related helper around the same conditions to also compare unit_type, seed_hi, seed_lo, traffic seeds, audience, and audience_strict. Make sure any change to those fields causes invalidation just like the existing id, iteration, full_on_variant, traffic_split, and split checks.
1045-1076: 🗄️ Data Integrity & Integration | 🔴 CriticalKeep queued events until publish succeeds
do_publish/1clearsexposuresandgoalsbefore waiting forpublisher.publish/6, then always replies with{:ok, new_state}. If publishing returns{:error, reason}or times out, the queued events are discarded and the caller cannot retry. Return an error and keep the original state until publish succeeds.🤖 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 `@lib/absmartly/context.ex` around lines 1045 - 1076, The queued events are being cleared too early in do_publish/1, so failures from publisher.publish/6 or a timeout still return {:ok, new_state} and drop the pending exposures/goals. Update do_publish/1 so it only resets state after a successful publish, and on {:ok, {:error, reason}} or nil return an error result while preserving the original state for retry. Use the existing do_publish/1 flow, build_publish_event_map/1, and the Task.yield/Task.shutdown handling to ensure failures do not discard queued events.
🤖 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/ci.yml:
- Around line 19-29: The workflow still uses tag-based actions and leaves
checkout credentials enabled. Update the Checkout, Set up Elixir, and Restore
deps cache steps to use full commit SHAs instead of version tags, and configure
the actions/checkout step in the CI workflow to disable persisted credentials
unless a later step explicitly needs repo write access. Use the existing action
names in the workflow to locate the exact steps.
In `@lib/absmartly/context.ex`:
- Around line 1484-1488: The exception log in the event handler failure path
currently includes raw payload data via inspect(data), which can expose user
information. Update the Logger.error block in the event handler crash handling
code to omit or redact the Data field while still keeping the event_type and
formatted exception/stacktrace from Exception.format for debugging. Use the
existing event handler failure logging in context.ex as the place to make this
change and ensure no raw event payload is emitted.
- Around line 267-291: The async fetch flow in handle_info/2 can leave
wait_until_ready/2 callers hanging or crash the GenServer when the fetcher or
Types.ContextData.from_map/1 fails. Update the spawned fetch block and the
{:fetch_async_complete, {:ok, data}} clause in ABSmartly.Context to
rescue/convert both fetcher exceptions and payload-shape conversion errors into
the existing {:fetch_async_complete, {:error, reason}} path, so the server
always completes readiness with a terminal result instead of timing out or
crashing.
- Around line 1480-1489: The emit_event handler in Context currently only
rescues exceptions, so throw and exit values from state.event_handler can still
escape and crash the GenServer. Update the try/catch around
state.event_handler.(event_type, data) to handle both :throw and :exit in
addition to rescue, and log those cases with Logger.error using the same
event_type and data context as the existing exception path.
In `@test/absmartly/http_integration_test.exs`:
- Around line 87-110: The http_integration_test assertions in the /context flow
are too loose and can pass invalid wire-contract shapes. In the test that checks
the GET/PUT context exchange, tighten the checks on the request path and publish
body so they match the exact /context contract, and update the body assertions
to require a hashed payload and a non-empty units list instead of only type
checks; use the existing get, put, and body assertions in
http_integration_test.exs to locate the relevant expectations.
---
Outside diff comments:
In `@lib/absmartly/context.ex`:
- Around line 467-480: The finalize guard is missing from the attribute and
override setters, so a closed context can still be mutated after finalize/1.
Update the handle_call clauses for set_attribute, set_attributes, set_override,
and set_overrides in context.ex to follow the same finalized-state check used by
units, custom assignments, tracking, publishing, and refresh, and return the
appropriate error without changing state when the context is finalized.
- Around line 861-879: The queue trimming logic in queue_exposure (and the
similar goal queue path referenced by the review) is dropping the newest
previously queued item because the queue is built in reverse order with [event |
state.queue]. Update the overflow branch to remove the oldest item from the tail
instead of using Enum.drop/1, and make sure the matching exposed_experiments
entry is also cleared when an exposure is evicted so it can be queued again
later.
- Around line 650-660: Move the refresh fetch out of handle_call(:refresh, ...)
in ABSmartly.Context so the GenServer does not block while waiting on HTTP
timeouts or retries. Reuse the same off-process fetch-and-reply pattern used by
the async initialization path: delegate the work to a separate process/task,
then reply back to the GenServer once the fetch completes. Update the :refresh
handling in ABSmartly.Context to keep track of the caller and return only after
the fetch result is received, preserving access for track, publish, and finalize
calls.
- Around line 1434-1441: The cached assignment invalidation check is missing
several inputs that affect assign_variant/2, so refreshes can reuse stale
assignments and exposure state. Update the comparison logic in the
old_exp/new_exp refresh path and the related helper around the same conditions
to also compare unit_type, seed_hi, seed_lo, traffic seeds, audience, and
audience_strict. Make sure any change to those fields causes invalidation just
like the existing id, iteration, full_on_variant, traffic_split, and split
checks.
- Around line 1045-1076: The queued events are being cleared too early in
do_publish/1, so failures from publisher.publish/6 or a timeout still return
{:ok, new_state} and drop the pending exposures/goals. Update do_publish/1 so it
only resets state after a successful publish, and on {:ok, {:error, reason}} or
nil return an error result while preserving the original state for retry. Use
the existing do_publish/1 flow, build_publish_event_map/1, and the
Task.yield/Task.shutdown handling to ensure failures do not discard queued
events.
In `@lib/absmartly/http/client.ex`:
- Around line 31-35: The HTTP request in the anonymous request function
currently enables automatic redirects, which bypasses the existing 3xx error
handling in with_retry/3 and can leak the X-API-Key to another host. Update the
request logic around HTTPoison.get so redirects are not followed automatically,
and handle any redirect responses manually in the retry flow for both requests
while preserving the existing query-string construction and timeout settings.
In `@lib/absmartly/json_expr/evaluator.ex`:
- Around line 263-265: The regex timeout log in Evaluator should not include
evaluated audience text, since it may expose user attributes in application
logs. Update the Logger.error call in the regex timeout branch to remove
String.slice(text, 0, 100) and log only non-sensitive metadata such as text
length or a hash, while keeping the pattern and timeout context intact.
- Around line 72-95: The eval_and and eval_or logic in Evaluator should not stop
at the first nil result, since that makes boolean evaluation order-dependent.
Update eval_and and eval_or to keep scanning remaining operands after
Utils.to_boolean returns nil, only returning nil if no later operand produces a
decisive true/false value; preserve the existing short-circuit behavior for true
in eval_or and false in eval_and. Add regression coverage around evaluate/2 for
cases like [nil, true] and [nil, false] to verify the final result is decided by
later operands.
🪄 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: CHILL
Plan: Pro
Run ID: 8679eae8-8433-4cd3-a223-8d84331a407c
📒 Files selected for processing (7)
.github/workflows/ci.ymllib/absmartly/context.exlib/absmartly/http/client.exlib/absmartly/json_expr/evaluator.extest/absmartly/http_integration_test.exstest/absmartly/json_expr/evaluator_test.exstest/absmartly/md5_test.exs
| - name: Checkout | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Set up Elixir | ||
| uses: erlef/setup-beam@v1 | ||
| with: | ||
| elixir-version: ${{ matrix.elixir }} | ||
| otp-version: ${{ matrix.otp }} | ||
|
|
||
| - name: Restore deps cache | ||
| uses: actions/cache@v4 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=.github/workflows/ci.yml
echo "Checking action refs and checkout credential persistence in ${file}"
grep -nE 'uses:|persist-credentials' "$file"
echo
echo "Action refs that are still tag-based rather than full SHAs:"
grep -nE 'uses:\s+[^@]+@v?[0-9]+(\.[0-9]+)?$' "$file" || true
echo
echo "Checkout steps without persist-credentials: false:"
awk '
/name: Checkout/ { in_checkout=1; block="" }
in_checkout { block = block $0 "\n" }
in_checkout && /^[[:space:]]*-[[:space:]]*name:/ && $0 !~ /name: Checkout/ {
if (block !~ /persist-credentials:[[:space:]]*false/) print block
in_checkout=0
block=""
}
END {
if (in_checkout && block !~ /persist-credentials:[[:space:]]*false/) print block
}
' "$file"Repository: absmartly/elixir-sdk
Length of output: 659
Pin the workflow actions and disable checkout credentials.
actions/checkout@v4, erlef/setup-beam@v1, and actions/cache@v4 are still tag-based, and checkout does not set persist-credentials: false. Pin each action to a full commit SHA and drop the checkout token unless a later step needs repository write access.
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 19-20: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 20-20: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 23-23: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 29-29: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/ci.yml around lines 19 - 29, The workflow still uses
tag-based actions and leaves checkout credentials enabled. Update the Checkout,
Set up Elixir, and Restore deps cache steps to use full commit SHAs instead of
version tags, and configure the actions/checkout step in the CI workflow to
disable persisted credentials unless a later step explicitly needs repo write
access. Use the existing action names in the workflow to locate the exact steps.
Source: Linters/SAST tools
| spawn(fn -> | ||
| result = | ||
| if fetcher do | ||
| fetcher.() | ||
| else | ||
| ABSmartly.HTTP.Client.fetch_context( | ||
| state.sdk_config.endpoint, | ||
| state.sdk_config.api_key, | ||
| state.sdk_config.application, | ||
| state.sdk_config.environment | ||
| ) | ||
| end | ||
|
|
||
| send(server, {:fetch_async_complete, result}) | ||
| end) | ||
|
|
||
| {:noreply, state} | ||
| end | ||
|
|
||
| @impl true | ||
| def handle_info({:fetch_async_complete, _result}, %{ready: true} = state), do: {:noreply, state} | ||
| def handle_info({:fetch_async_complete, _result}, %{failed: true} = state), do: {:noreply, state} | ||
|
|
||
| def handle_info({:fetch_async_complete, {:ok, data}}, state) do | ||
| context_data = Types.ContextData.from_map(data) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Always complete async readiness on fetcher or payload failures.
If data_fetcher.() raises, the spawned process exits without sending :fetch_async_complete; if Types.ContextData.from_map/1 raises on a bad shape, the GenServer crashes. In both cases, pending wait_until_ready/2 callers are left to time out instead of receiving {:error, reason}.
Suggested direction
spawn(fn ->
result =
- if fetcher do
- fetcher.()
- else
- ABSmartly.HTTP.Client.fetch_context(
- state.sdk_config.endpoint,
- state.sdk_config.api_key,
- state.sdk_config.application,
- state.sdk_config.environment
- )
+ try do
+ if fetcher do
+ fetcher.()
+ else
+ ABSmartly.HTTP.Client.fetch_context(
+ state.sdk_config.endpoint,
+ state.sdk_config.api_key,
+ state.sdk_config.application,
+ state.sdk_config.environment
+ )
+ end
+ rescue
+ exception -> {:error, Exception.message(exception)}
+ catch
+ kind, reason -> {:error, {kind, reason}}
end
send(server, {:fetch_async_complete, result})
end)Also wrap Types.ContextData.from_map(data) in the success handler and route conversion failures through the existing {:error, reason} 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 `@lib/absmartly/context.ex` around lines 267 - 291, The async fetch flow in
handle_info/2 can leave wait_until_ready/2 callers hanging or crash the
GenServer when the fetcher or Types.ContextData.from_map/1 fails. Update the
spawned fetch block and the {:fetch_async_complete, {:ok, data}} clause in
ABSmartly.Context to rescue/convert both fetcher exceptions and payload-shape
conversion errors into the existing {:fetch_async_complete, {:error, reason}}
path, so the server always completes readiness with a terminal result instead of
timing out or crashing.
| try do | ||
| state.event_handler.(event_type, data) | ||
| rescue | ||
| exception -> | ||
| Logger.error(""" | ||
| Event handler crashed for event #{event_type} | ||
| Exception: #{Exception.format(:error, exception, __STACKTRACE__)} | ||
| Data: #{inspect(data)} | ||
| """) | ||
| end |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify emit_event only rescues exceptions and has no catch branch for throw/exit.
rg -n -C8 'defp emit_event|try do|rescue|catch' lib/absmartly/context.exRepository: absmartly/elixir-sdk
Length of output: 1949
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect emit_event and its callers to see whether throw/exit can escape to the GenServer process.
ast-grep outline lib/absmartly/context.ex --view expanded
printf '\n--- callers ---\n'
rg -n -C3 'emit_event\(' lib/absmartly/context.exRepository: absmartly/elixir-sdk
Length of output: 2169
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect emit_event and its callers to see whether throw/exit can escape to the GenServer process.
ast-grep outline lib/absmartly/context.ex --view expanded
printf '\n--- callers ---\n'
rg -n -C3 'emit_event\(' lib/absmartly/context.exRepository: absmartly/elixir-sdk
Length of output: 2169
Catch throw and exit in emit_event (lib/absmartly/context.ex:1476-1489)
rescue only handles exceptions; a handler that throws or exits from the GenServer callback will still escape and can terminate the context. Add catch :throw / catch :exit branches and log them too.
🤖 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 `@lib/absmartly/context.ex` around lines 1480 - 1489, The emit_event handler in
Context currently only rescues exceptions, so throw and exit values from
state.event_handler can still escape and crash the GenServer. Update the
try/catch around state.event_handler.(event_type, data) to handle both :throw
and :exit in addition to rescue, and log those cases with Logger.error using the
same event_type and data context as the existing exception path.
| Logger.error(""" | ||
| Event handler crashed for event #{event_type} | ||
| Exception: #{Exception.format(:error, exception, __STACKTRACE__)} | ||
| Data: #{inspect(data)} | ||
| """) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not log raw event payloads from handler failures.
data can contain attributes and goal properties, so logging inspect(data) on an event handler exception can expose user data in application logs. Redact or omit the payload.
Suggested redaction
Logger.error("""
Event handler crashed for event #{event_type}
Exception: #{Exception.format(:error, exception, __STACKTRACE__)}
- Data: #{inspect(data)}
+ Data: <redacted>
""")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Logger.error(""" | |
| Event handler crashed for event #{event_type} | |
| Exception: #{Exception.format(:error, exception, __STACKTRACE__)} | |
| Data: #{inspect(data)} | |
| """) | |
| Logger.error(""" | |
| Event handler crashed for event #{event_type} | |
| Exception: #{Exception.format(:error, exception, __STACKTRACE__)} | |
| Data: <redacted> | |
| """) |
🤖 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 `@lib/absmartly/context.ex` around lines 1484 - 1488, The exception log in the
event handler failure path currently includes raw payload data via
inspect(data), which can expose user information. Update the Logger.error block
in the event handler crash handling code to omit or redact the Data field while
still keeping the event_type and formatted exception/stacktrace from
Exception.format for debugging. Use the existing event handler failure logging
in context.ex as the place to make this change and ensure no raw event payload
is emitted.
| # ---- GET /context (fetch -> ready) ---- | ||
| assert get | ||
| assert String.starts_with?(get.path, "/context") | ||
| assert get.query =~ "application=website" | ||
| assert get.query =~ "environment=test" | ||
|
|
||
| # ---- PUT /context (publish) ---- | ||
| assert put | ||
| assert String.starts_with?(put.path, "/context") | ||
| refute put.path =~ "?" | ||
|
|
||
| # Required auth / identity headers (exact names per the wire contract). | ||
| assert header(put, "x-api-key") == "test-api-key" | ||
| assert header(put, "x-application") == "website" | ||
| assert header(put, "x-environment") == "test" | ||
| assert header(put, "x-application-version") == "0" | ||
| assert header(put, "x-agent") not in [nil, ""] | ||
| assert header(put, "content-type") =~ "application/json" | ||
|
|
||
| # Body must carry the required publish fields. | ||
| body = Jason.decode!(put.body) | ||
| assert is_boolean(body["hashed"]) | ||
| assert is_list(body["units"]) | ||
| assert is_integer(body["publishedAt"]) and body["publishedAt"] > 0 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Tighten the wire-contract assertions.
The contract says GET /context, PUT /context, and a hashed units payload. starts_with?, is_boolean, and is_list would still pass for /context-extra, hashed: false, or an empty units list.
Suggested change
- assert String.starts_with?(get.path, "/context")
+ assert get.path == "/context"- assert String.starts_with?(put.path, "/context")
+ assert put.path == "/context"- assert is_boolean(body["hashed"])
- assert is_list(body["units"])
+ assert body["hashed"] == true
+ assert is_list(body["units"])
+ assert length(body["units"]) >= 1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # ---- GET /context (fetch -> ready) ---- | |
| assert get | |
| assert String.starts_with?(get.path, "/context") | |
| assert get.query =~ "application=website" | |
| assert get.query =~ "environment=test" | |
| # ---- PUT /context (publish) ---- | |
| assert put | |
| assert String.starts_with?(put.path, "/context") | |
| refute put.path =~ "?" | |
| # Required auth / identity headers (exact names per the wire contract). | |
| assert header(put, "x-api-key") == "test-api-key" | |
| assert header(put, "x-application") == "website" | |
| assert header(put, "x-environment") == "test" | |
| assert header(put, "x-application-version") == "0" | |
| assert header(put, "x-agent") not in [nil, ""] | |
| assert header(put, "content-type") =~ "application/json" | |
| # Body must carry the required publish fields. | |
| body = Jason.decode!(put.body) | |
| assert is_boolean(body["hashed"]) | |
| assert is_list(body["units"]) | |
| assert is_integer(body["publishedAt"]) and body["publishedAt"] > 0 | |
| # ---- GET /context (fetch -> ready) ---- | |
| assert get | |
| assert get.path == "/context" | |
| assert get.query =~ "application=website" | |
| assert get.query =~ "environment=test" | |
| # ---- PUT /context (publish) ---- | |
| assert put | |
| assert put.path == "/context" | |
| refute put.path =~ "?" | |
| # Required auth / identity headers (exact names per the wire contract). | |
| assert header(put, "x-api-key") == "test-api-key" | |
| assert header(put, "x-application") == "website" | |
| assert header(put, "x-environment") == "test" | |
| assert header(put, "x-application-version") == "0" | |
| assert header(put, "x-agent") not in [nil, ""] | |
| assert header(put, "content-type") =~ "application/json" | |
| # Body must carry the required publish fields. | |
| body = Jason.decode!(put.body) | |
| assert body["hashed"] == true | |
| assert is_list(body["units"]) | |
| assert length(body["units"]) >= 1 | |
| assert is_integer(body["publishedAt"]) and body["publishedAt"] > 0 |
🤖 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 `@test/absmartly/http_integration_test.exs` around lines 87 - 110, The
http_integration_test assertions in the /context flow are too loose and can pass
invalid wire-contract shapes. In the test that checks the GET/PUT context
exchange, tighten the checks on the request path and publish body so they match
the exact /context contract, and update the body assertions to require a hashed
payload and a non-empty units list instead of only type checks; use the existing
get, put, and body assertions in http_integration_test.exs to locate the
relevant expectations.
|
@Pedro-Revez-Silva Addressed — and your review caught a real publish bug. CI: added `.github/workflows/ci.yml` (commit `17e3182`) running `mix test` on `push` + `pull_request`. Real HTTP integration test: added `test/absmartly/http_integration_test.exs` — a `:gen_tcp` HTTP server driving the public SDK (`ABSmartly.SDK.create_context → treatment/track → Context.publish`) so the real HTTPoison client makes an actual GET /context and PUT /context. Bug it surfaced (fixed in commit `dcee7ea`): publish was doing `POST /events` — which is not a collector route (the collector exposes `PUT /v1/context`) — and omitted `X-Application-Version` and `X-Agent`. So publish never reached the backend. Changed to `PUT /context` with the full header set; elixir now lands 100/100 in the live e2e. On the wrapper API: noted — the cross-sdk-tests wrapper's e2e path drives the public `ABSmartly.SDK.create_context` against the live collector. |
3 participants
Summary
Test plan
mix testto verify all 295 unit tests passSummary by CodeRabbit