[joalves]

Summary

• Initial Elixir SDK implementation with full ABSmartly platform support
• Canonical test parity (295 tests)
• Cross-SDK test compatibility (137/138 passing)
• Phoenix, LiveView, and Plug integration examples
• Application module and project structure

Test plan

• Run mix test to verify all 295 unit tests pass
• Run cross-SDK tests to verify compatibility

Summary by CodeRabbit

• New Features
  • Added SDK context management for experiments, assignments, attributes, goals, event publishing and finalisation.
  • Added HTTP API integration with retries and audience-expression evaluation.
  • Added HTTP server capabilities including HTTP/2, HTTP/3/QUIC, WebSockets, WebTransport, routing, static files, compression and telemetry.
• Documentation
  • Added SDK setup and usage guidance, along with licence information.
• Tests
  • Added coverage for SDK contexts, publishing, audience evaluation, hashing and HTTP integration.
• Chores
  • Added formatting configuration, ignore rules, container support and a CI workflow.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

• Last reviewed commit: 33bff2b
• Latest commit, not reviewed: e99bbdd

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

Adds an ABSmartly.Context GenServer for context loading, experiment assignment, event tracking, publishing, finalisation and refresh. Adds an HTTP client for fetching context data and publishing events, with retry handling. Adds a JSON expression evaluator and tests for SDK context behaviour, HTTP requests, expression evaluation and MD5 hashes. Adds SDK documentation and a GitHub Actions workflow that runs the test suite.

Priority: ➖ Normal

Estimated code review effort: 5 (Critical) | ~90 minutes

Suggested reviewers: pedro-revez-silva

Merge Risk: 🟡 Moderate · up to 33bff

Slow publishing can terminate the calling request instead of returning a retryable error. Fix the timeout boundary before merging, and correct the event documentation.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 3.60% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 111 functions across 7 files. (1 skipped: …	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title accurately summarises the main change: adding the initial Elixir SDK implementation. It is concise and clear.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

Docstring coverage is 3.60% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 111 functions across 7 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

✨ Finishing Touches: 💡 1

📝 Generate docstrings: 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

_{A rabbit checks each queued event,
Then watches HTTP requests be sent.
Expressions hop through paths and trees,
Tests check hashes with careful ease.
The context waits, then loads its data.}

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

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 | 🟠 Major

Handle invalid unpadded length (rem 4 =:= 1) explicitly.

Line 41 currently omits the 1 branch, so malformed input with #{padding := false} crashes with a case_clause instead 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 | 🟠 Major

Reject 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 invalid calendar: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 | 🟠 Major

Remove 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 in mix.exs or managed through mix.lock. Although .gitignore specifies /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 | 🟠 Major

Keep fetched dependency sources out of deps/.

This file appears to be upstream cowlib code checked into Mix’s dependency checkout directory. Unless you’re intentionally maintaining a fork, committing deps/ makes upgrades, CVE tracking, and reproducible installs much harder, and Mix can overwrite local edits there during normal dependency refreshes. Prefer pinning the dependency in mix.exs/mix.lock and 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 | 🟠 Major

Reject 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 that Name is non-empty before emitting the header. That means malformed inputs like <<"Secure">> or <<"=value">> come back as {ok, <<>>, ...}, and callers can also generate =value on 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 | 🟠 Major

Final container runs as root.

From Line 30 onward there is no USER directive, 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 | 🟠 Major

Configuration scope mismatch: :elixir_wrapper should 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 :absmartly in mix.exs. This configuration will not be accessible to the application at runtime if any code attempts to read it via Application.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 | 🟠 Major

Set MIX_ENV=prod in 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 runs mix run without explicitly setting MIX_ENV=prod, causing the compiled code to run with the default environment (typically dev), creating a mismatch between compile-time and runtime configuration.

Add ENV MIX_ENV=prod in the runtime stage before the CMD instruction:

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 | 🟠 Major

Remove vendored dependencies or document the vendoring strategy.

The repository commits 396 tracked files from deps/ to git despite .gitignore excluding /deps/. This contradictory setup creates ongoing maintenance burden:

1. Repository bloat: Unnecessary dependency source files significantly increase clone size and repository size.
2. Update burden: Dependencies require manual synchronisation rather than automatic fetching via mix get deps.
3. Confusion: The presence of deps/ conflicts with the .gitignore rule, misleading contributors about the intended setup.

The project already includes mix.lock for 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 | 🟠 Major

Optional callbacks are currently treated as mandatory.

webtransport_handle/2 and webtransport_info/2 are declared optional, but this call path still invokes them unconditionally. A handler that relies on that documented optionality will crash with undef on 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 of

Also 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 | 🟠 Major

Dynamic-table name reuse never matches the stored tuple shape.

dyn_table entries 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 | 🟠 Major

Implement missing OTP system callbacks required by sys:handle_system_msg/6.

The call to sys:handle_system_msg/6 at line 187 requires this module to define system_continue/3, system_terminate/4, and system_code_change/4. Without these callbacks, the code will fail with an undef error 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 | 🟠 Major

Do not swallow h2spec bootstrap failures.

test-build depends 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/5 crashes before it can return the advertised 501 response.

The only clause hard-matches version := 'HTTP/3', pid, and streamid, so any other request dies with function_clause and never reaches the false branch. 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 | 🟠 Major

Remove vendored deps/ files from source control.

This commit adds 396 files to the deps/certifi/, deps/cowboy/, and deps/cowboy_telemetry/ directories—entire vendored upstream packages that should be managed by Mix. Even though .gitignore contains the /deps/ rule, these files are being explicitly committed. The repository already declares these packages in mix.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 during mix deps.get.

README.md-382-389 (1)

382-389: ⚠️ Potential issue | 🟠 Major

Do not create the context unconditionally in mount/3.

LiveView mounts once for the disconnected HTTP render and again after the websocket connects. A session_id generated in mount/3 is 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 the session_id in 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 | 🟠 Major

Encode PRIORITY weight as wire value (Weight - 1).

The parser exposes weight as 1..256 (Weight + 1), but the builder writes Weight directly. 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 | 🟠 Major

Auto-mode request body length is not accumulated.

Line 95’s TODO is a real bug: body_length is 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 | 🟠 Major

Handle all ranch:recv_proxy_header/2 error variants.

The recv_proxy_header/2 callback 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 a case_clause crash 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 | 🟠 Major

Crash when Shutdown is infinity in per-child shutdown.

The shutdown/2 function at line 82 passes the child's Shutdown value directly to erlang:start_timer(Shutdown, ...). The type specification for the child record (line 28) declares shutdown :: timeout(), which includes both integers and the atom infinity. However, erlang:start_timer/3 only accepts non-negative integers and will raise badarg if passed infinity.

The before_terminate_loop/1 function (lines 120–122) correctly handles this by checking case Time of infinity -> undefined; _ -> erlang:start_timer(...), but shutdown/2 lacks 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 | 🟠 Major

Redact crash logs before emitting them.

make_error_log/5 serialises the full Req, Data, PartialReq, Resp, Opts and handler State. 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 | 🟠 Major

Treat relay fin as an upstream close.

_IsFin is 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 | 🟠 Major

Keep the info/3 result when switching to relay mode.

info(State0, ...) can change flow, mutate the stream state, or remove the stream entirely. Reusing Flow and Streams from State0 discards 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 | 🟠 Major

Surface QUIC listener start failures instead of hanging startup.

The parent waits only for {ok, Listener} with no timeout or error handling. If quicer:listen/2 fails or the spawned listener process crashes before sending that message, start_quic/3 blocks 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 | 🟠 Major

Erase 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 | 🟡 Minor

Use 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 | 🟡 Minor

Support 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 | 🟡 Minor

Add length validation to prevent badarg crash on malformed close_session frames.

When Len1 is 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), AppMsgLen becomes negative. In Erlang, using a negative size in a binary pattern match (AppMsg:AppMsgLen/binary) raises a badarg exception rather than failing the match gracefully. This causes the parser to crash on malformed input instead of returning error.

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 | 🟡 Minor

Duplicate .DS_Store entry.

.DS_Store appears 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 | 🟡 Minor

Close the cowboy:start_clear/3 call 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 | 🟡 Minor

Fix 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 | 🟡 Minor

Use the canonical SPDX licence identifier.

The current form Apache 2.0 is not a valid SPDX identifier. Hex packaging tooling expects SPDX identifiers, and the canonical form for Apache License 2.0 is Apache-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 | 🟡 Minor

Avoid showing publish/1 immediately before finalize/1.

This README already says finalize/1 flushes 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 only finalize/1 makes 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 | 🟡 Minor

Normalise the user-agent header 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 | 🟡 Minor

Strip content-length from 204 replies before sending them.

The sendfile fast-paths add a content-length before the 204/304 guard runs, and do_reply_ensure_no_body/4 keeps caller-supplied headers unchanged. That allows reply(204, ...) to emit an invalid content-length header.

🤖 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 00 or 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.exs deps) 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 whether CHANGELOG.md and CONTRIBUTING.md should be ignored.

CHANGELOG.md and CONTRIBUTING.md are 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 /2 moving average, while Line 68 applies a weighted /8 formula. 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.

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

deps/cowboy/src/cowboy_constraints.erl (1)

34-35: reverse/2 spec 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

📥 Commits

Reviewing files that changed from the base of the PR and between ddbb76e and 8fa0349.

📒 Files selected for processing (6)

• deps/cowboy/src/cowboy_clear.erl
• deps/cowboy/src/cowboy_constraints.erl
• deps/cowboy/src/cowboy_http.erl
• deps/cowboy/src/cowboy_tls.erl
• deps/cowboy_telemetry/LICENSE
• deps/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

[Pedro-Revez-Silva]

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.

[coderabbitai]

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 win

Avoid logging evaluated text on regex timeouts.

text here 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 win

Don't halt and/or on the first nil.

This makes evaluation order-dependent. For example, %{"or" => [%{"var" => "missing"}, %{"value" => true}]} returns nil today, and ABSmartly.Matcher.evaluate/2 then turns that into a non-match because it checks == true. and has the same problem in the other direction with later false operands. Please keep scanning after nil and only return nil when 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 | 🟠 Major

Disable automatic redirects on both requests

with_retry/3 already treats 3xx responses as errors, so follow_redirect: true bypasses that branch. It can also send X-API-Key to 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 win

Apply finalisation checks to attributes and overrides too.

After finalize/1, set_attribute(s) and set_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
   end

Apply the same guard to set_attributes, set_override, and set_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 win

Drop the oldest queued item, not the newest one.

Both queues are prepended with [event | state.queue], so Enum.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()
end

For exposures, also remove the dropped exposure name from exposed_experiments if 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 lift

Move 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 as track, publish, or finalize from 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 win

Invalidate cached assignments when all assignment inputs change.

assign_variant/2 depends on unit_type, seed_hi, seed_lo, traffic seeds, audience, and audience_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 -> false

Also 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 | 🔴 Critical

Keep queued events until publish succeeds

do_publish/1 clears exposures and goals before waiting for publisher.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

📥 Commits

Reviewing files that changed from the base of the PR and between 8fa0349 and 17e3182.

📒 Files selected for processing (7)

• .github/workflows/ci.yml
• lib/absmartly/context.ex
• lib/absmartly/http/client.ex
• lib/absmartly/json_expr/evaluator.ex
• test/absmartly/http_integration_test.exs
• test/absmartly/json_expr/evaluator_test.exs
• test/absmartly/md5_test.exs

[joalves]

@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.

Acceptance criteria satisfied at f43df38; independently verified in review round 21.

[jervasion-absmartly]

APPROVED WITH COMMENTS - The missing-UID regression is fixed, its regression test rejects the old implementation, and exact-head CI and local validation pass.

Reviewed head: f43df38ccd626fb98bf17c9fd55487b8b5b83fae
Base / merge-base: fac6abf0b22e608939a9c66d959d18b77a1e9f6f

Intent and scope

Initial Elixir SDK: supervised sync/async contexts, deterministic assignment and audience evaluation, collector transport, event publication/disposal, tests/CI and Phoenix/LiveView/Plug examples. The author reports canonical/cross-SDK parity and live-collector success. The latest change preserves missing-unit semantics for nil initial UIDs while retaining integer/string normalization. The baseline contains no SDK implementation; no existing Elixir deployment, migration requirement or rollout flag is assumed. No AGENTS.md or CLAUDE.md is present. Historical dependency/server artifacts mentioned in the generated summary are not current SDK features.

Findings and previous acceptance criteria

No blocking defect or measured finding survives this review; no new inline finding is added. The round-20 criterion is satisfied: both initializers use normalization that removes nil entries before converting valid identities to strings. Primary public-SDK execution observes {units, peek} = {%{}, 0} for a nil user, accepts the real UID and then returns treatment 1. Creating with integer 123 stores "123"; both unchanged integer setters return :ok.

Regression sensitivity was independently established in a detached scratch worktree: removing only Enum.reject(fn {_k, v} -> is_nil(v) end) makes the unmodified mix test test/absmartly/fix_plan_test.exs:822 fail at the missing-unit assertion (actual 1, expected 0; 1 selected failure, 63 excluded). No test was edited. The same test passes at the reviewed head as part of the full suite.

Earlier transport/pool/redirect, malformed-data, targeting/custom-assignment, assignment/exposure invalidation, ordering/hash/retry/overflow, readiness/recovery/waiter, disposal and supervisor-shutdown corrections remain present and exercised by the passing suite. The supplied still-active change-request review IDs are being dismissed only because their acceptance criteria are satisfied, not because of round count. Optional simplification and formatting observations below do not prevent merging.

Convergence summary — review round 21

• Unresolved original blockers: 0
• Remediation regressions: 0
• Independent PR-change findings: 0
• Acceptance criteria still open: none
• Human adjudication requested: no

Verification

• mix local.hex --force, mix local.rebar --force, mix deps.get: passed, locked resolution with no advisory warnings. Local Elixir 1.18.3 / OTP 27.
• Primary mix test: 404 tests, 0 failures, seed 414040. Includes real collector GET/PUT wire contract, redirect rejection and queue retention, shared-pool saturation, retry cap, recovery/readiness/waiters, disposal, overflow retention identities and supervisor shutdown.
• Primary public-SDK identity probe and scratch production-only mutation described above.
• Exact-head PR Actions run 39545065604, job 119041269044, and push run 39545061845, job 119041256440: completed/success, including mix test (1.17, 26) and the PR test step. CodeRabbit commit status is success / review paused, not fresh behavioral verification.
• mix hex.build --output /dev/null, mix docs, git diff --check base head: passed. Compilation warns about callback grouping, unused helper/parameter and alias; documentation warns about undefined/private SDKConfig.t(). mix format --check-formatted fails across PR formatting, as previously disclosed; it is not a repository-declared merge gate. No separate lint/typecheck gate is declared.
• Introduced-content helper completed: 1,098 content writes / 906 distinct blobs. Historical artifact classes and credential-pattern matches checked; matches are test/example placeholders, interactive shell prompts and the upstream Plug.Crypto doctest, not deployed credentials.
• External cross-SDK/live-collector tests were not rerun; the author's reported results are not represented as exact-head execution. The hermetic public-SDK integration suite and exact-head declared CI provide the merge evidence here. No material validation remains pending or inaccessible. No cloud queries, deployment or browser authentication used; PR source branch unchanged.

Passes (non-blocking)

run: core runtime/API/security/data/error paths (new executable SDK); tests/CI/packaging (ExUnit, HTTP harness, workflow and dependencies); deployment/operations/observability (publisher/failure/shutdown emissions and lifecycle guidance); behaviour-bearing configuration/fixtures (SDK settings and experiment payloads); cross-cutting simplicity (substantial new source); comment hygiene; diff composition (head and introduced history content).
not run: shared frontend blast radius and interaction complexity (no frontend source changed). Independent read-only runtime and tests/packaging/simplicity passes supplied candidates and evidence; primary verified and judged them.

Comment hygiene (non-blocking)

C=354 comment/documentation-content lines / E=5780 countable source lines, ratio ~1:16.3. Includes compiled tests and executed build/config files; blank/comment-delimiter-only lines excluded; documentation attributes are comment content.
threshold T=58; restatements counted: no (10 × C < E)
promotion declined: unchanged historical labels retain the standing decline as short, harmless annotations; the new nil-identity paragraph records the non-obvious missing-versus-empty identity constraint and is preserved.
Flagged blocks (separate ranges denote separate blocks/paragraphs):

• .formatter.exs:1, config/config.exs:3,6: operation/grouping restatements.
• lib/absmartly/application.ex:6,15: history narration / startup restatement.
• lib/absmartly/assigner.ex:37,44,50: operation restatements.
• lib/absmartly.ex:9,17,20,23,26,29,39,47: example/declaration restatements.
• lib/absmartly/context.ex:287,378-379,732-733,839,1162-1166,1705-1706: grouping/transition restatements; identity-comparison, ownership and lifecycle constraints otherwise preserved.
• lib/absmartly/http/client.ex:5-9,53,85,97-99,112,125,143: history/Fixes labels.
• lib/absmartly/json_expr/evaluator.ex:7-11,59,71,235,245,255,271: history labels; :69: grouping restatement.
• lib/absmartly/matcher.ex:5,41,55,61: history narration.
• lib/absmartly/types.ex:22,56,59,67,149,284: history/validation restatements.
• lib/absmartly/utils.ex:67,99-100: removed-code/history narration.
• test/absmartly/fix_plan_test.exs:99,129,142,175,195,213,227,248,257,265,277,324,338,352,373,383,394,413,423,433,456,465,504,532,543,582,679,705,768: Fix/Phase history labels; :632,656: operation/grouping restatements.
• test/absmartly/http_integration_test.exs:24,68,74,78,87,93,106,112,118-120,174,237: operation/assertion/helper restatements; external wire constraints preserved.
• test/absmartly/failure_semantics_test.exs:569,740,754: assertion/setup restatements.

Diff composition (non-blocking)

No unwanted artifact remains at head. History-only _build/ (build output, 525 writes) and deps/ (dependency cache, 409 writes), introduced beginning in 736a87a57b6573399a87407614d4591a62cb1b6b, were removed and are ignored. Correction: keep untracked/ignored, already achieved. No credential occurrence requiring rotation identified.

Simplifications (non-blocking)

Inspection-backed candidates, not demonstrated blocking alternatives; LOC deltas unmeasured unless specified:

• Duplicate context initialization: share common state construction while retaining async/ready differences.
• Successful load/readiness transitions: share state/index/waiter completion with correct return tuples.
• Async/refresh fetch selection and error normalization: share operation while retaining shutdown/ownership differences.
• Forward-only refresh/conversion helpers: call underlying implementations directly.
• Experiment change maps/predicates: derive shared assignment/exposure invalidation information once, preserving overrides.
• Cached/new custom assignment: resolve the base first and apply one eligibility branch preserving strict targeting.
• Unknown/override exposure constructors and custom-field lookup: share construction/lookup with differing projections.
• Variable resolver: reduce_while can stop traversal at the winner, preserving valid nil values.
• Unused validate_uid/2: delete dead helper; 7 nonblank code lines.
• SDK start-child identity wrapping and success-only config wrapper: return/construct directly without weakening validation.
• Empty-waiter branch: iterate unconditionally and clear.
• Sleeps following synchronous publish/ordered same-sender operations: remove unnecessary delays.
• Attribute filter-then-last: one last-write-wins reduction preserving nil/false.
• Validation index dry-run: reuse validated indexes rather than rebuilding immediately, preserving atomic validation.
• Inert json_library setting: remove or implement configuration semantics.
• Retry-budget arithmetic: share actual backoff inputs/calculation.
• Saturation test: remove unused hog_url and _ = hog_url; 2 code lines.
• types.ex validation wrapper: call ExperimentValidation.validate!/2 directly; 1 net code line.

Run a simplification pass locally before pushing: these candidates cost less to address now than as a further review round.

[joalves]

Pushed SDK parity changes per maintainer decisions (JS reference: javascript-sdk/src/context.ts):

• D1: reads (treatment, peek, variable value/keys, custom fields) before ready or after finalize now return the canonical errors ABsmartly Context is not yet ready. / ABsmartly Context is finalized.; experiments() only requires ready. A failed initial load leaves the context ready+failed with empty data (control/default values).
• D2 finalize event is finalize; D3 a failed final publish keeps events queued, stays retryable and surfaces the error; D4 refresh on a failed context is a successful no-op.
• D5 JS operator semantics (in haystack-first, null operands -> null, invalid regex -> null; contains kept).
• D7 sticky assignments across refresh; D8 a matching assignment rule beats holdout suppression.

Breaking: error behavior/messages on not-ready/finalized reads and operator null semantics. Requires at least a minor version bump over the last release. Native test suite passes.

New commits were added after this approval. Jervasion will reassess the updated head.

[jervasion-absmartly]

❌ Changes requested

The parity changes follow the maintainer's stated contracts, and CI is green. One blocking test issue remains: the revised audience-refresh test passes even when production code breaks the stickiness it claims to protect.

Next step: Make that test distinguish a retained assignment from a newly calculated one, or move that claim to a test demonstrated to catch the regression.

Must fix

1. Pin audience-refresh stickiness with a discriminating assertion — the current test still passes when audience-only refresh incorrectly invalidates its cached assignment. test/absmartly/failure_semantics_test.exs:340–341

Suggestions

• Non-blocking numeric-conversion parity edge cases are noted inline.
• One small simplification candidate and three comment blocks to tidy are listed in the details.

What I checked

• ✅ Full local suite: 407 tests pass; compilation and diff whitespace checks pass.
• ✅ Both exact-head CI runs passed on Elixir 1.17 / OTP 26.
• ✅ A scratch-worktree mutation changes public refresh behavior while the claimed regression test remains green.
• ✅ Compared numeric audience evaluation with the JavaScript reference and checked the approved implementation.

Review details

Head: 02f6cfcfade99794d3ea53d5701e0327ff07224a. Base / merge-base: fac6abf0b22e608939a9c66d959d18b77a1e9f6f. Accepted responsibility baseline: f43df38ccd626fb98bf17c9fd55487b8b5b83fae; only the single intervening commit is responsible for new findings. Round 22.

Intent: align readiness/finalization errors, failed-context empty-data behavior and no-op refresh, assignment stickiness, custom assignments, and expression conversions with maintainer-selected JS semantics. The explicit author comment is treated as the intended contract, not as evidence of deployment. This is an initial SDK relative to the empty base; no existing production Elixir adoption or migration obligation is assumed. No repository AGENTS.md or CLAUDE.md was found. The generated server/QUIC summary is not the current SDK surface.

Convergence summary — review round 22

• Unresolved original blockers: 0
• Remediation regressions: 0
• Independent PR-change findings: 1
• Acceptance criteria still open: the modified audience-refresh stickiness test must reject the demonstrated production mutation, or relinquish that claim to another test demonstrated to reject it.
• Human adjudication requested: no — this is newly demonstrated test evidence in the post-approval delta, not a repeated failed remediation.

Validation:

• mix deps.get: locked versions unchanged.
• mix test: 407 tests, 0 failures (also independently run by the test pass).
• mix compile: passes with existing warnings.
• git diff --check f43df38ccd626fb98bf17c9fd55487b8b5b83fae HEAD: passes.
• mix format --check-formatted: fails across existing and changed files; formatting is not a declared CI gate and is not a merge blocker here.
• CI runs 39928933060 and 39928925295: completed successfully, each with passing mix test (1.17, 26) job; both report the exact reviewed head. Commit status CodeRabbit is successful but paused, not independent code-validation evidence.
• Scratch mutation: add only old_exp.audience != new_exp.audience -> false to invalidate_changed_assignments/3. Unmodified mix test test/absmartly/failure_semantics_test.exs:322: 1 selected test passes, 33 excluded. Entire test file also passes: 34 tests, 0 failures. A direct context probe with an existing user and deterministic variant 1 changes from {before, after} = {1, 1} at head to {1, 0} under this mutation.
• Numeric probe through real Context processes: strict audiences produce variants 1 for numeric 1 versus string 1.e2, and 0 for string 0 versus numeric negative zero and string 1e+21 versus numeric 1e21. The documented JS conversions produce the opposite equality outcomes. The approved evaluator returned false/true/true respectively; current returns true/false/false. Reference checked: javascript-sdk/src/jsonexpr/evaluator.ts, numberConvert and stringConvert, plus direct Node execution of those conversions. These uncommon input boundaries are non-blocking.

Passes (non-blocking): run: core runtime/API/error paths (three executable source files), tests/CI/packaging (five modified test files and observable SDK contracts), deployment/operations/observability (changed invalid-filter log and failure lifecycle), cross-cutting simplicity (substantial executable delta), behaviour-bearing data/policy (changed expression expectations), comment hygiene and diff composition (all eight files and all eight introduced content writes) | not run: shared-component frontend blast radius (no frontend source changed). Independent runtime, test and simplicity passes returned evidence; primary verified the surviving test finding in a scratch worktree.

Comment hygiene (non-blocking): 81 comment / 360 code lines, ratio ~1:4.44, 3 blocks flagged
threshold T=5; restatements counted: yes (10 × C ≥ E)

• lib/absmartly/json_expr/evaluator.ex:22: “Maximum regex pattern length (ReDoS protection)”, restates the adjacent named constant.
• lib/absmartly/json_expr/evaluator.ex:24: “Regex timeout in milliseconds (ReDoS protection)”, restates the adjacent named constant.
• test/absmartly/fix_plan_test.exs:302: “A failed context is ready with empty data so reads return defaults.”, restates the assertions.
  Three qualifying lines, below T. Other added reference comments record the non-obvious external JS contract, including deliberately sticky behavior, and are preserved.

Diff composition (non-blocking): every file is required for the change; none flagged. The pinned introduced-blobs helper completed successfully over approved..head, yielding eight content writes from 02f6cfc; all eight postimages were inspected. No scratch artifacts or credential-bearing values were identified in this range.

Simplifications (non-blocking): 1 candidate

• lib/absmartly/json_expr/evaluator.ex:90–95: replace the new Map.fetch result branch with get_nested(rest, Map.get(value, key)); absent keys already return nil through the existing base/fallback clauses. LOC delta: -3. Independent read-only equivalence harness covered 240 value/path combinations. This is not a ten-line measured simplification.

Run a simplification pass locally before pushing: these candidates cost less to address now than as a further review round.

Round-22 audience-refresh test acceptance criterion satisfied at e99bbdd: exact production cache-invalidation mutation now fails the unmodified regression test; full suite and exact-head CI pass.

[jervasion-absmartly]

✅ Approved with suggestions

The audience-refresh test now catches the regression from the earlier review. All 410 tests and both exact-head CI jobs pass; no blocking issue remains.

Next step: None — ready to merge.

Suggestions

• A few uncommon numeric-conversion boundaries still differ from JavaScript; optional follow-up noted inline.
• One small simplification candidate is listed in the details.

What I checked

• ✅ Full local suite: 410 tests, 0 failures.
• ✅ The revised audience-refresh test passes normally and fails under the exact cache-invalidation mutation from the earlier review.
• ✅ Both CI jobs passed on Elixir 1.17 / OTP 26 at this exact head.
• ✅ Compilation, package build, documentation generation and diff whitespace checks pass.

Review details

Head: e99bbdd2a470c3cf83e29c5825b9e5e6b82481a9. Base / merge-base: fac6abf0b22e608939a9c66d959d18b77a1e9f6f. Accepted responsibility baseline: f43df38ccd626fb98bf17c9fd55487b8b5b83fae; the two subsequent commits are the range for new findings. Round 23.

Intent and scope: initial Elixir SDK with synchronous/asynchronous contexts, assignment, audience evaluation, collector transport, event publication, and Phoenix/LiveView/Plug examples. The follow-up implements the maintainer-selected JS contracts for readiness errors, failed-ready empty data, failed-context refresh no-op, sticky assignments and expression conversions. The latest commit strengthens audience-refresh coverage, fixes the previously noted exponent/negative-zero/large-number cases, simplifies nested-map access and removes three flagged comments. The explicit owner decisions are the intended contract; no existing Elixir deployment or migration obligation is assumed. No applicable AGENTS.md or CLAUDE.md was found. The generated server/QUIC summary does not describe the SDK's public surface. Author-reported cross-SDK/live-collector results were read but not rerun or represented as exact-head execution.

Convergence summary — review round 23

• Unresolved original blockers: 0
• Remediation regressions: 0
• Independent PR-change findings: 0 blocking; one optional numeric-parity observation
• Acceptance criteria still open: none
• Human adjudication requested: no

The round-22 criterion is satisfied: the new test first caches variant 1 under a matching audience, then refreshes to a strict nonmatching audience and asserts the retained 1. In a detached scratch tree, adding only old_exp.audience != new_exp.audience -> false to production invalidate_changed_assignments/3 makes unmodified mix test test/absmartly/failure_semantics_test.exs:342 fail at line 358 (actual 0, expected 1; 35 tests, 1 failure, 34 excluded). At head the same command passes (35 tests, 0 failures, 34 excluded). This both demonstrates changed observable behavior and proves the test detects it.

Validation:

• mix local.hex --force, mix local.rebar --force, mix deps.get: passed, locked dependencies unchanged, no advisory warning.
• mix test: 410 tests, 0 failures, seed 242216, Elixir 1.18.3 / OTP 27. Includes real HTTP collector wire tests, redirect rejection/queue retention, pool saturation, retries, lifecycle/disposal/shutdown, overflow identities, targeting and expression tests.
• mix compile, mix hex.build --output /dev/null, mix docs, git diff --check f43df38 HEAD: passed. Existing compile warnings concern callback grouping, unused helper/parameter and alias; docs warn about undefined/private SDKConfig.t().
• mix format --check-formatted: fails across existing and changed files, as previously disclosed; formatting is not a declared merge gate. No separate lint/typecheck gate is declared.
• CI Actions runs 39993928360 / 39993920964, jobs 120552426644 / 120552400832: completed/success, each mix test (1.17, 26), exact head verified. CodeRabbit status is successful but paused, not fresh behavioral verification. No material check remains pending.
• Primary Elixir/Node probes independently confirmed the optional conversion examples in the inline comment.
• The pinned introduced-blobs helper completed over accepted..head: 12 content writes, each postimage inspected alongside the eight-file cumulative diff; no credentials or unwanted artifacts identified in this responsibility range.

Passes (non-blocking)

run: core runtime/API/security/data/error paths (context, evaluator and matcher changes); tests/CI/packaging (five changed test files and SDK contracts; unchanged unfiltered CI/package configuration checked); deployment/operations/observability (changed invalid-filter logging and failure lifecycle); behaviour-bearing data/fixtures (expression and context expectations); cross-cutting simplicity (substantial executable delta); comment hygiene and diff composition (all eight changed files and 12 introduced writes). Independent read-only runtime and tests/simplicity passes returned evidence; primary verified the surviving observations and mutation.
not run: shared-component frontend blast radius (no frontend source changed).

Comment hygiene (non-blocking)

C=83 comment/documentation-content lines / E=392 countable source lines, ratio ~1:4.72; none flagged.
threshold T=5; restatements counted: yes (10 × C ≥ E)
Counts cover added lines in compiled source/tests; documentation attributes/content are comments, not code; blank and comment-delimiter-only lines excluded. Remaining reference comments record the non-obvious external JS semantics and deliberately sticky alternatives and are preserved. The three blocks flagged in round 22 were removed.

Diff composition (non-blocking)

Every file is required for the change; none flagged in accepted..head. No dependency/build artifact or credential-bearing content was introduced by these two commits. Previously accepted historical artifact cleanup is not reopened.

Simplifications (non-blocking)

1 candidate:

• lib/absmartly/context.ex:706-715: reuse with_ready(state, true, fn -> do_publish(state) end) instead of repeating its ready-check/success/error dispatch; do_publish already returns the expected result/state pair. Estimated LOC delta: -7; not scratch-demonstrated, below the measured threshold. The earlier nested-map lookup suggestion has been implemented.

Run a simplification pass locally before pushing: these candidates cost less to address now than as a further review round.

[jervasion-absmartly]

[nit] Consider pinning the remaining precision and whitespace parity edges (non-blocking)

What differs: The previously reported numeric cases are fixed. Two uncommon boundaries still differ from the stated JavaScript conversion: string_convert(0.6235798317206265) returns "0.623579831720627", while JS toFixed(15).replace(/\.?0{0,15}$/, "") returns "0.623579831720626". Also, number_convert("\uFEFF12") returns nil versus JS parseFloat's 12, whereas "\u008512" returns 12 here versus JS NaN.

Evidence: Independently reproduced with direct Elixir and Node calls; the runtime pass also traced these through Matcher equality. These affect only precision-sensitive string/number comparisons or unusual leading characters, so they do not block merging.

Suggestion: In an optional follow-up, pin these cases and align rounding and the accepted leading-whitespace set if exact conversion parity is required.