fix: ten rounds of bug-hunting across geometry, input handling, and failure modes (#21)

* fix: stop the spiral from drawing avatars on top of each other

The radii come from a running area total: place each avatar at the radius
whose disc holds everything placed so far at a fixed density. That is an
estimate, and it assumes the avatars behind it are spread out at exactly
that density — which nothing enforces.

The centre avatar breaks it on its own. Everyone on the first turn has to be
pushed out past where the area says they go, to clear an avatar larger than
the taper's average; the estimate never learns, keeps counting the hole they
left as filled, and hands the next turn a radius that lands it on the turn
before. At the default sizes that costs a pixel, and it grows with the
avatars: `max_size: 216, min_size: 210` — a legal config, and only a flat
taper — overlapped by eighteen. The default taper itself overlapped past
sixty contributors.

The exact fix was already here, written for `scale` and gated behind it:
slide each avatar along its own ray until it clears everything already
placed. Run it for every avatar instead. The area total stays as the opening
guess, so the wall keeps its shape and its size — the golden grows by 2% and
one avatar of twelve actually moves — but the guarantee no longer depends on
the sizes being close to the defaults.

The overlap spec allowed a half pixel of slack for two-decimal rounding,
which is more than the rounding can produce and was hiding the default
spiral's own overlap. Tightened, and extended over the size range that used
to fail.

* fix: keep orbit rings, and the people on them, off each other

Two ways the orbit laid avatars on top of one another, both reachable from
an ordinary config.

A ring was filled by arc length: as many avatars as `2*pi*r` divided by the
pitch. What actually has to clear between two neighbours is the straight line
between them, and a chord is always shorter than its arc — by a percent once
a ring holds a dozen, and by 36% when it holds two, where half a
circumference of arc is one diameter of chord. So a ring took on one avatar
more than fits, which nobody notices on a crowded ring and which overlaps
outright on a sparse one. A wide `center_size` puts the first ring exactly
there. Counted by chord now, which is also what makes `gap` mean the
clearance it is documented to be: the default wall was leaving 7 where it
promised 8.

Stepping out to the next ring reserved this ring's half, the gap, and half of
the ring *after* the next one. The taper takes six pixels off each ring, so
every step landed three pixels short, every ring, and any `ring_gap` below
that put two rings on top of each other. The taper now comes from one place
and is asked for the ring being stepped to.

Both fixes are geometry, not tuning: 3500 fuzzed configs across the full
range of `center_size`/`avatar_size`/`min_size`/`ring_gap`/`gap`, with and
without per-user `scale`, now come back clean. 49 of them overlapped before.

* fix: check a repo or org name before pasting it into an API path

`repo` was accepted as anything of the shape `<no slash or space>/<no slash
or space>`, and `org` as anything without a slash. Both then went straight
into a request path, where the characters neither check excluded decide what
the path means:

  repo: owner/repo?per_page=1  ->  /repos/owner/repo
                                   ?per_page=1/contributors&per_page=100&page=1

The request reaches the repository endpoint instead of the contributors one,
with this client's own pagination folded into the query string it inherited.
The array walker then meets a JSON object and reports "unexpected response
from GitHub" — an error about GitHub for a config typo. `#` truncates the
path outright; `..` walks up out of it; `org: my org` puts a raw space in the
request line and comes back 400.

Names are now held to the character set GitHub actually issues — letters,
digits, hyphen, and dot and underscore for repository names — with `.` and
`..` excluded, and each segment percent-encoded on the way into the path.

Checked twice on purpose. A written-down `repo` or `org` is checked at config
load, where the error names the file and the line; a `repo` that came from
GITHUB_REPOSITORY has no config to name and is checked where the URL is
built.

* fix: report a file it cannot read or write, instead of a stack trace

Four ordinary mistakes reached the top of the program as unhandled
exceptions. In a workflow log that means a Crystal stack trace through the
standard library and — because nothing went through `Annotations` — no
annotation at all, so the step failed with nothing rendered next to the file
that caused it:

  - `config` pointing at a directory. `File.exists?` says yes; the read then
    fails with "Is a directory".
  - a config file the container user cannot open.
  - an output path whose parent is an existing file, so the directory cannot
    be created.
  - an output path that is itself a directory.

All four now surface as `::error::` with the path and the reason, and exit 1.
The shape of an output path is checked at config load, but nothing there can
know what is already on disk, so this belongs where the write happens.

GITHUB_OUTPUT gets the opposite treatment: it is written after the mural
already is, so an unwritable one warns and lets the run stand rather than
discarding finished, correct work over a downstream step's convenience. The
warning is what explains an empty `steps.*.outputs`.

* fix: stop reading an oversized avatar instead of measuring it afterwards

`MAX_BYTES` was checked against `response.body` — a string the HTTP client
had already built. The check could only ever reject a body small enough to
have fit in memory first, which is not the case it exists for. A URL
answering with a gigabyte took the runner's memory with it before anything
looked at the size, and on a workflow that builds a mural from a pull request
that address is the contributor's to choose.

The body now comes off the socket through the pool's streaming form, one byte
past the limit is enough to know, and the read stops there. A local server
pushing 128 MiB with no Content-Length gets cut off just past 8 MiB;
previously it was buffered whole.

Stopping early leaves the rest of the body queued on the connection, so it
must not be pooled — whoever picked it up next would read the tail of someone
else's avatar as the head of their own response. That is why the limit raises
rather than returns, and why the streaming form drops the connection it
happened on. Bodies that fit are read to the end, so the ordinary path still
hands a clean socket back and keep-alive is unaffected.

The body is read before the status is looked at, for the same reason: a
redirect's body has to be drained too, and every status now leaves the
connection either clean or closed.

* fix: let `sort` reach the two styles that were re-sorting behind it

`sort` is documented as "weight | login | none (none keeps list order)", and
the notes recommend `sort: none` for keeping the API's own order — stargazers
arrive oldest-first with no weight, so under the default they trail everyone.
Five styles honour it. Spiral and orbit re-sorted the list by weight on their
way to placing it, so those two settings did nothing at all: the option
looked broken rather than inapplicable, which is the failure mode with no
error message attached to it.

Both now place the list as it arrives, the way mosaic and voronoi already do:
the list decides who goes where, and `prepare` — which ranks by weight
regardless — decides how large. The comparator they imposed was the same one
`sort: weight` applies, so the default wall is unchanged down to the golden
files.

Covered for all seven styles rather than the two that were wrong. The promise
is made once in the reference and kept in seven places.

* fix: refuse config the parser would read past and throw away

Two shapes of legal YAML lose half the file without a word:

  users:
    - login: a
  ---            # everything below here is a second document
  style: mosaic  # and never reaches the mural
  users:
    - login: b

and a key written twice, where YAML keeps the last one and silently discards
whatever was under the first — the easy mistake to make when appending to a
config by hand.

Neither is an error to the parser, so the run succeeded and the only symptom
was a wall missing people. That reads as a problem with the sources, or with
this action, rather than as three lines to go and fix in a file. Both are now
rejected before parsing, with the line to look at.

A single document that merely opens with `---` is unaffected; that is one
document, and a common way to write one.

* fix: line every grid section up on the same column

Labels are centred on their cell and can be wider than it, so a block takes an
inset on both sides to leave room for the overhang. The inset was measured
against the section being drawn, and each section got its own — so a group of
short names started its avatars tens of pixels to the left of the group above
it. One wall, with its columns visibly out of true, for no reason a reader
could see: the names in one group happened to be longer.

The overhang is now measured once across the whole document, in `prepare`,
and every section is drawn against that. A section can never need more room
than the document does, so its own figure survives only as the floor for a
caller that renders without `prepare` — where it goes on keeping labels inside
the canvas rather than letting them run off it.

`title_inset` was adding a gutter it always computed as zero, since the only
caller could not supply a user list. Dropped: a section title belongs at the
margin, which is where the widest label's own left edge lands.

The grid spec was the one renderer spec that skipped `prepare`, which is why
nothing caught this — it was measuring each section on its own, exactly as the
bug did. It now renders the way the runner does.

* fix: mark the workspace safe once, not once per run

Inside the runner the workspace is owned by a different uid than the container
user, so git refuses to touch it until it is listed under `safe.directory`.
That entry was added with `--add` on every run, which appends the line whether
or not it is already there.

On a hosted runner nobody sees it: HOME is thrown away with the job. A docker
action's HOME is `/github/home`, mounted from a directory the runner keeps —
so on a self-hosted runner the file grew by one identical line per mural run,
without bound, and git re-reads it on every command it runs. Four runs, four
lines; a nightly workflow, a thousand a year.

Added only when it is not already listed. A wildcard entry counts as listed,
since that is what it means.

* fix: let an avatar source or API client hand back what it opened

Both take an optional pool and build one when they are not given one. Nothing
could then close it. Keep-alive means the pool holds a socket open to every
host the source spoke to, so each such source kept a connection alive until
the garbage collector happened to reach the pool — a source per repository in
a loop keeps them all.

Invisible to the action, which owns one pool for the whole run and closes it
in an `ensure`. Very visible to the spec suite, which builds 54 of them:
forty sources against one local server opened forty connections and were
still holding nine of them at exit.

`close` releases the pool when the source built it, and does nothing when the
pool was handed in — closing that one would hang up on whoever else is using
it. The spec suite routes its constructions through a helper that closes them
when the example ends.

Author: hahwul

spec/annotations_spec.cr

@@ -46,4 +46,16 @@ describe ContributorMural::Annotations do
     ENV.delete("GITHUB_OUTPUT")
     ContributorMural::Annotations.output("k", "v")
   end
+
+  # The outputs are written after the mural already is, so an unwritable
+  # GITHUB_OUTPUT used to throw away finished work with a stack trace.
+  it "warns rather than raises when GITHUB_OUTPUT cannot be written" do
+    ENV["GITHUB_OUTPUT"] = File.join(File.tempname("missing_dir"), "out.txt")
+    begin
+      ContributorMural::Annotations.output("paths", "wall.svg")
+      ContributorMural::Annotations.io.to_s.should contain("::warning::could not write the `paths` step output")
+    ensure
+      ENV.delete("GITHUB_OUTPUT")
+    end
+  end
 end

spec/committer_spec.cr

@@ -66,6 +66,36 @@ describe ContributorMural::Committer do
     end
   end
 
+  # Inside the runner the workspace is owned by a different uid than the
+  # container user, so git will not touch it until it is marked safe. A docker
+  # action's HOME is a directory the runner keeps, so `--add` on every run
+  # appended another identical line to it, forever — invisible on a hosted
+  # runner, unbounded on a self-hosted one.
+  it "marks the workspace safe once, however many times it runs" do
+    with_git_repo do |clone, _remote|
+      home = File.tempname("mural_home")
+      Dir.mkdir_p(home)
+      previous_home = ENV["HOME"]?
+      previous_actions = ENV["GITHUB_ACTIONS"]?
+      begin
+        ENV["HOME"] = home
+        ENV["GITHUB_ACTIONS"] = "true"
+        committer = ContributorMural::Committer.new(clone, "msg")
+        3.times do |run|
+          File.write(File.join(clone, "wall.svg"), "<svg id=\"#{run}\"/>")
+          committer.commit(["wall.svg"])
+        end
+
+        entries = git_output("config", "--global", "--get-all", "safe.directory").lines.map(&.strip)
+        entries.count(clone).should eq(1)
+      ensure
+        previous_home ? (ENV["HOME"] = previous_home) : ENV.delete("HOME")
+        previous_actions ? (ENV["GITHUB_ACTIONS"] = previous_actions) : ENV.delete("GITHUB_ACTIONS")
+        FileUtils.rm_rf(home)
+      end
+    end
+  end
+
   it "raises a CommitError when the push is impossible" do
     with_git_repo do |clone, remote|
       FileUtils.rm_rf(remote)

spec/config_spec.cr

@@ -54,6 +54,22 @@ describe ContributorMural::Config do
       end
     end
 
+    # `exists?` is not `readable?`. A `config` input pointing at a directory —
+    # or at a file the container user cannot open — used to reach the top of
+    # the program as an unhandled exception, printing a Crystal stack trace
+    # into the workflow log and no annotation at all.
+    it "fails with a config error when the file cannot be read" do
+      directory = File.tempname("mural_cfg_dir")
+      Dir.mkdir_p(directory)
+      begin
+        expect_raises(ContributorMural::ConfigError, /could not be read/) do
+          ContributorMural::Config.load(directory)
+        end
+      ensure
+        Dir.delete(directory)
+      end
+    end
+
     it "fails on unknown keys (typo protection)" do
       expect_raises(ContributorMural::ConfigError, /avatarsize/) do
         ContributorMural::Config.load(SpecHelper.fixture("configs", "invalid_unknown_key.yml"))
@@ -98,12 +114,55 @@ describe ContributorMural::Config do
       end
     end
 
+    # Both of these are legal YAML that quietly throws half the file away. The
+    # only symptom used to be a wall missing people, which reads as a problem
+    # with the sources rather than with the config.
+    it "refuses a second YAML document instead of ignoring it" do
+      error = expect_raises(ContributorMural::ConfigError, /second YAML document/) do
+        ContributorMural::Config.parse("style: honeycomb\nusers:\n  - login: a\n---\nusers:\n  - login: b\n")
+      end
+      error.line.should eq(4)
+    end
+
+    it "still accepts a single document that opens with the marker" do
+      config = ContributorMural::Config.parse("---\nstyle: mosaic\nusers:\n  - login: a\n")
+      config.style.should eq(ContributorMural::Style::Mosaic)
+      config.users.map(&.login).should eq(["a"])
+    end
+
+    it "refuses a key set twice instead of keeping only the last" do
+      error = expect_raises(ContributorMural::ConfigError, /`users` is set twice/) do
+        ContributorMural::Config.parse("style: grid\nusers:\n  - login: a\nusers:\n  - login: b\n")
+      end
+      error.line.should eq(4)
+    end
+
     it "asks for an org when `members` is written bare" do
       expect_raises(ContributorMural::ConfigError, /`members` needs an `org`/) do
         ContributorMural::Config.parse("members:")
       end
     end
 
+    # Caught here rather than at the first request, so the error names the file
+    # and the line instead of arriving as a puzzling report about GitHub.
+    it "rejects a source name that is not a plain GitHub name" do
+      {
+        "members:\n  org: my-org?x=1"        => /members `org` must be a plain organization name/,
+        "members:\n  org: my org"            => /members `org` must be a plain organization name/,
+        "contributors:\n  repo: owner/repo?" => /contributors `repo` must look like owner\/name/,
+        "contributors:\n  repo: owner"       => /contributors `repo` must look like owner\/name/,
+        "stargazers:\n  repo: owner/.."      => /stargazers `repo` must look like owner\/name/,
+      }.each do |yaml, message|
+        expect_raises(ContributorMural::ConfigError, message) do
+          ContributorMural::Config.parse(yaml).validate!
+        end
+      end
+    end
+
+    it "accepts the punctuation GitHub allows in a repository name" do
+      ContributorMural::Config.parse("contributors:\n  repo: my-org/some.repo_name").validate!
+    end
+
     it "names the accepted values for a misspelled enum" do
       error = expect_raises(ContributorMural::ConfigError) do
         ContributorMural::Config.load(SpecHelper.fixture("configs", "invalid_style.yml"))