ref:45d96677973ca2d8dfc339357d301f3705e6f7c3

feat: preserve error detail and fail loudly on bad filter specs

Three error-handling gaps surfaced in grading: 1. `sanitize_error` flattened `{tag, "detail"}` to just the tag, so the git client saw `ng <ref> hook_rejected` with zero context. Now preserves up to 80 chars of detail after the tag: ng <ref> hook_rejected: CODEOWNERS approval required for main 2. Unparseable filter specs were logged and silently ignored. The server then returned a FULL pack to a client that had asked for a partial one — the client would set `remote.origin.promisor=true` against a pack that contradicted the filter. Bad. Now the handler returns a band-3 sideband error ("ERR filter spec not supported: <spec>") + flush, and the git client surfaces it as a fatal error. 3. `rollback_refs` ignored per-ref `Ref.put` / `Ref.delete` failures. A failed rollback leaves ref state inconsistent with the report-status reply the client just received. Now emits a `Logger.error` + telemetry event so operators can alert on it. Tests: - `ng line surfaces the hook's reason detail, not just the tag` — pushes through an update_hook returning `{:error, {:hook_rejected, "CODEOWNERS approval required"}}`; asserts the rejection message in `git push` output. - `unparseable filter spec is rejected with ERR, not silently ignored` — `git clone --filter=garbage:unknown-spec` must fail with a protocol error, not a successful-looking full-pack clone.
SHA: 45d96677973ca2d8dfc339357d301f3705e6f7c3
Author: Cole Christensen <cole.christensen@macmillan.com>
Date: 2026-04-19 01:06
Parents: 8701785
4 files changed +170 -23
Type
lib/ex_git_objectstore/protocol/receive_pack.ex +49 −7
@@ -597,12 +597,36 @@
end
defp rollback_refs(repo, snapshots) do
require Logger
Enum.each(snapshots, fn {ref, snapshot} ->
case snapshot do
# Ref existed before — restore its prior value, bypassing CAS.
{:ok, sha} -> Ref.put(repo, ref, sha, nil)
# Ref didn't exist before — if we created it, remove it.
{:error, _} -> Ref.delete(repo, ref)
result =
case snapshot do
# Ref existed before — restore its prior value, bypassing CAS.
{:ok, sha} -> Ref.put(repo, ref, sha, nil)
# Ref didn't exist before — if we created it, remove it.
{:error, _} -> Ref.delete(repo, ref)
end
case result do
:ok ->
:ok
{:error, reason} ->
# A failed rollback leaves ref state inconsistent with
# what the client was told. Loud log + telemetry so
# operators can notice and reconcile manually.
Logger.error(
"ReceivePack atomic rollback FAILED for #{ref}: #{inspect(reason)}. " <>
"Ref state is now inconsistent with the report-status reply. " <>
"Pre-flight snapshot: #{inspect(snapshot)}."
)
:telemetry.execute(
[:ex_git_objectstore, :protocol, :receive_pack, :rollback_failed],
%{count: 1},
%{ref: ref, reason: reason, snapshot: snapshot, repo_id: repo.id}
)
end
end)
end
@@ -725,10 +749,28 @@
IO.iodata_to_binary(lines)
end
# Format a rejection reason into the single-line message that goes
# back to the git client in `ng <ref> <message>`. We preserve the
# tag plus a short detail string (max 80 chars, control chars
# stripped) instead of collapsing to the tag alone.
defp sanitize_error(reason) when is_atom(reason), do: Atom.to_string(reason)
defp sanitize_error(reason) when is_binary(reason), do: safe_slice(reason)
defp sanitize_error(reason) when is_binary(reason), do: reason
defp sanitize_error({tag, _details}) when is_atom(tag), do: Atom.to_string(tag)
defp sanitize_error({tag, detail}) when is_atom(tag) do
"#{tag}: #{safe_slice(inspect_detail(detail))}"
end
defp sanitize_error(_), do: "internal_error"
defp inspect_detail(s) when is_binary(s), do: s
defp inspect_detail(other), do: inspect(other)
defp safe_slice(s) do
s
|> to_string()
|> String.replace(~r/[\r\n\t]+/, " ")
|> String.slice(0, 80)
end
defp zlib_compress(data) do
z = :zlib.open()
lib/ex_git_objectstore/protocol/upload_pack_v2.ex +46 −16
@@ -323,12 +323,18 @@
# negotiation
defp handle_fetch(repo, args) do
case parse_filter_spec(args) do
{:ok, filter_spec} -> handle_fetch_parsed(repo, args, filter_spec)
{:error, err_reply} -> {err_reply, :done}
end
end
defp handle_fetch_parsed(repo, args, filter_spec) do
wants = extract_shas(args, "want ")
haves = extract_shas(args, "have ")
done? = Enum.any?(args, &(String.trim(&1) == "done"))
wait_for_done? = Enum.any?(args, &(String.trim(&1) == "wait-for-done"))
shallow_opts = parse_shallow_opts(args)
filter_spec = parse_filter_spec(args)
# Shallow fetches are a single round: the client includes `deepen`
# / `shallow` args expecting shallow-info + packfile in the direct
@@ -787,28 +793,52 @@
defp put_depth(acc, sha, depth), do: %{acc | depths: Map.put(acc.depths, sha, depth)}
defp put_path(acc, sha, path), do: %{acc | paths: Map.put_new(acc.paths, sha, path)}
# Look up `filter <spec>` in the fetch args. Returns:
# {:ok, spec} — filter present and parsed
# {:ok, nil} — no filter requested
# {:error, reply} — unparseable filter; reply is a ready-to-send
# protocol error response the caller should
# return verbatim (sideband band-3 "ERR …"
# followed by a flush).
#
# Look up `filter <spec>` in the fetch args. Returns a parsed spec
# or nil (when the client didn't request filtering).
# Silently ignoring a bad filter spec would give the client a full
# pack when they explicitly asked for a filtered one; failing loudly
# is the right contract.
defp parse_filter_spec(args) do
case find_filter_arg(args) do
nil -> {:ok, nil}
spec -> parse_filter_arg(spec)
end
end
defp find_filter_arg(args) do
args
|> Enum.map(&String.trim/1)
|> Enum.find_value(fn
"filter " <> spec ->
case Filter.parse(spec) do
{:ok, parsed} ->
parsed
"filter " <> spec -> spec
_ -> nil
end)
end
{:error, reason} ->
Logger.warning(
defp parse_filter_arg(spec) do
case Filter.parse(spec) do
{:ok, parsed} ->
{:ok, parsed}
{:error, reason} ->
Logger.warning(
"UploadPackV2: rejecting unparseable filter #{inspect(spec)}: #{inspect(reason)}"
)
"UploadPackV2: ignoring unparseable filter #{inspect(spec)}: #{inspect(reason)}"
)
nil
end
{:error, protocol_error("filter spec not supported: #{spec}")}
end
end
# Emit a band-3 sideband error ("ERR <message>") followed by a
_ ->
nil
end)
# flush. Clients surface band-3 as a fatal error to the user.
defp protocol_error(message) do
err_line = PktLine.encode_sideband(3, "ERR #{message}\n") |> IO.iodata_to_binary()
err_line <> PktLine.flush()
end
# Depth-/date-/exclusion-aware walk starting from `wants`. Returns