feat: 0.7.0 — say what it did, and give the terminal back - #96
Merged
Conversation
The three header fields were printed exactly as the file wrote them, while every other piece of untrusted text went through `printable()` at its entry point — the rule `src/lib.rs` states in as many words. A `.art` file is the one thing this project asks strangers to send. #57 is an open invitation to contribute templates and CONTRIBUTING §11 makes the review path "open the file with `mossaic-art`", so a reviewer running `--matrix theirs.art`, or anyone running `--list-templates`, executed whatever the header said: a window-title change, an `OSC 52` clipboard write, or a cursor-position query whose reply is typed back into the shell once the tool exits. Nothing warned, because the point of an escape sequence is that it is not displayed, and `--no-colour` suppressed it on no path. It travelled, too: through `--save` into the plan and back out raw on reload, through `--format json`'s `headline` and `text`, through `--format markdown` into `$GITHUB_STEP_SUMMARY`, and out of the Action's `headline` output where the consumer republishes it. `build.rs` embeds `art/templates/*.art`, so a merged template would have shipped its payload to every user on every listing. Cleaned in the `Canvas` meta reader, which covers the report header, the template listing, both report formats, the saved plan and the editor title in one place — and bounded to 200 characters, because an unbounded `# name:` produced a 200,061-byte first output line. Cleaning rather than refusing: a file authored on Windows carries a trailing CR on every line, and refusing any control character would reject it for something nobody typed. The regression test covers an OSC-and-SGR name, a bare carriage return, and the length bound. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
…ed pipe Two of the four ways out of a TUI had no guard at all. **Signals.** Under a pty, `kill -INT`, `-TERM` or `-HUP` on either binary emitted *zero bytes* to the tty: the shell was left inside the alternate screen with mouse tracking on, ECHO, ICANON and ISIG off, and no working Ctrl-C. The cure is to type `reset` blind, which a first-time user does not know, and nothing on screen says so — the failure is in what was not written. Whoever reaches for a signal is already having a bad time: the chart is sitting on "loading" because `gh` is slow, so they `kill %1` from another pane, or a script signalled the process group. The project had already decided this state was unacceptable — `main` installs a panic hook and says why in as many words — and `mossaic-art --draw`, which also takes the alternate screen and also enables mouse reporting, had neither guard. Both now share one `restore` module. `signal-hook` rather than an `unsafe_code` exception: `sigaction` is an unsafe call and `SECURITY.md` advertises the forbid as posture. The crate registers safely, does its work on a thread rather than in a handler (a real handler may call only async-signal-safe functions, and writing escapes through Rust's stdout is not one), and re-raises with the default disposition so the exit still reports as death-by-signal. It is also already in the tree — crossterm pulls it through ratatui at the same version — so this is an import, not a dependency. **Broken pipes.** `mossaic-art --font | less` and quitting, `--list-templates | head`, `--format json | jq -e` with a jq that exits early: all ordinary, all a panic at exit 101, over a message nobody was listening to, with a backtrace note that reads like a crash in mossaic. Which commands escaped was a pipe-buffer accident — the coloured glyph sheet is 53 KB and always went, the uncoloured one is 9 KB and survived on Linux but not macOS — so it read as a flake. `--png` is the one with a price: a valid, complete PNG is already on disk and the caller was handed 101, so a wrapper that checks the status deletes the file and retries. That is why this is a write-side hook rather than restoring the default SIGPIPE disposition, which would give the wrong answer there — and which `unsafe_code = "forbid"` bars anyway. A hook rather than a rewrite of 141 `println!` sites, because this is a property of every printing path and a hook cannot be forgotten at the site somebody adds next. Tests: the three signals against the editor in a real pty, asserting the emulated terminal state the shell would inherit rather than the bytes (the emulator consumes them) — confirmed red with the guard removed; and the writer's *own* status through a reader that closes first, which is the thing a pipeline hides. Nothing in CI could see either: `install.yml` only pipes into `tee`, which never exits early, and Actions' `bash -e` has no `pipefail`. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
… not be typed Four findings in the argument surface, all of them the tool accepting something and then doing other than what was asked. **Flags honoured in one mode and ignored in the rest.** `--png` was taken in every mode and wrote the file in exactly one; `-o` without `--draw` and `--format` without `--track` were the same shape. #26 settled the principle — "They are side effects and inputs the user explicitly asked for … Either honour them or refuse the combination" — and enumerated three flags, all refused; these were missed. Refused rather than honoured, deliberately: honouring `--png` would mean deciding what a PNG of a preview, a template list, a tracking report and a backfill each *are*, and `--snapshot` plus `mossaic --file --png` already covers the one people want. The cost of the silence was a script's: `--template dragon --png preview.png` printed a cheerful report, exited 0 and produced nothing. **The hyphen the font draws could not start a text.** `-` is listed three times — README's punctuation line, `action.yml`'s `text:` description, and the binary's own no-glyph message — and `mossaic-art -` was `unknown option "-"`. Reaching for `--` gave `unknown option "--"`, so the escape hatch named itself as the mistake, and no quoting helped; the only route through was a hand-written plan JSON that nothing documents. You reach for `mossaic-art -- "$TEXT"` precisely when the text is not yours to control, which is what the Action does. `--` now means the rest is positional, in the shared parser, so a dash-led login works in the chart too — and an argument that could have been the text names `--` in its error instead of just "try --help". **`--track --save --format json` wrote prose into a machine document.** The `saved …` confirmation was the single `println!` that could run ahead of the document, at exit 0 with an empty stderr, so there was no signal anywhere — and it bit only on the run that writes the plan, so it read as a flake on first use. It is on stderr now, where every other note a track run makes already goes, and still shown in all three formats. **`--help` priced the wrong thing.** "commits per lit day" applied to the shipped dragon predicts 584 where the tool prints 442 — a 32% overestimate from following the help exactly. `--commits` prices the *brightest* day; docs/ART.md and the source comment both say so. The help is the only description a `cargo install` user gets, since `docs/*` is excluded from the published archive and README never mentions the flag. The test checks the help line against a real report's own arithmetic. And the three plural sites 0.6.3's pass missed — the two preview headers and the editor's per-level row, which read "1 days · 1 commits" on the first line a new user sees — plus the markdown `holed` sentence it reworded into nonsense: the verb moved in front of "inside the letters", so "already lit" became a clause on *the letters* rather than the days. Only the path the Action publishes regressed; the text renderer still read correctly. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
Two figures the tool computes from the wrong set. **The cost table counted cells, not days.** A full-width picture is 7 x 53 = 371 cells against a year of 365, so the preview table disagreed with the header directly above it and with what `--write` makes — immediately after a note saying cells had been dropped. Measured: a gradient image priced out 322 days and 742 commits while the header said 317 and 722, and `--write` made 722. README says of the editor panel "the same arithmetic `--write` uses, not an estimate of it", and it was not. The tracking renderer always did it correctly, which is why the shipped docs disagree with themselves — ART.md prints one figure for a preview and another for the tracking table of the same plan, exactly the out-of-year cells apart. The legibility verdict came from the same raw canvas, so a picture that drew literally nothing inside the year still reported `shades 0 4 · closest pair 0 and 4 · ΔE 70, clear`: the one check this project tells you to read twice, passing on a drawing that does not exist. It now prints no verdict at all there. The editor panel had the same split. `estimate()` already filtered by `date_at`, so the rows said `level 4 1 day 4 commits each` four lines above `0 commits in total` — for a cell the panel itself draws as `·` and describes as costing nothing. The four shipped templates cannot catch any of this: all of them are drawn clear of the partial weeks across 2000-2100, so the test plants its own ink in the leading and trailing cells and asserts header == table == the days the year has. **The chart header counted the whole year.** Every figure honoured `--today` except the biggest one and the only one a screenshot carries. It took `Calendar::total` — GitHub's number for Jan-1..Dec-31 — while everything below came from `elapsed()`, so reading the shipped calendar as of March printed 9,527 above a grid showing 2,043, beside "23 active days". It also broke the two features `--today` was built for together: the `--snapshot` preview flow advertised the whole plan's cost as already paid while the planner, on the same data, said thousands were still owed. It now uses the elapsed sum when the calendar holds any future day, which is the same `any(future)` test that gates the "blank = still to come" legend, so the two turn on together. A finished year and a current year fetched with no `--today` are unchanged: `Calendar::total` is GitHub's own figure and can legitimately exceed the sum of visible days, which is why the switch is on future days rather than on comparing the two. The test asserts the header against the footer, so it cannot be satisfied by a header that is merely different. Two editor PTY tests now move the cursor a column right before painting: the cursor starts on a cell outside 2027, and painting there correctly moves no row any more. That distinction is the fix. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
Three loaders that were loud about one kind of wrong and silent about another. **A template that does not parse was invisible.** The skip is deliberate policy — one broken file must not take out `--list-templates` — but `templates::read_dir`'s own doc comment calls that command "the command you would reach for to find out which one is broken", and it named nothing, counted nothing and wrote no byte to stderr. So the same file got "a canvas is exactly 7 rows" from `--matrix` and "no template named sixer" from `--template`: an error about a *name*, for a file sitting right there under that name, which sends the search to the wrong place. #57 — this project's own good-first-issue walkthrough — puts a first-time contributor on exactly that path. The listing now names each skipped file and why, `--template <stem>` gives the parse error rather than a name miss, and a broken local file no longer silently shadows a built-in — the same command used to draw two different pictures depending on whether the user's file happened to parse, with nothing printed either way. The skip-rather-than-fail policy is unchanged. **A plan's mistyped key was applied at its default.** `validate()` bounds every value and no part of the shape, so `background: 99` was refused by name and `backgruond: 2` was accepted in silence — turning about 290 background days into keep-dark days, the one kind the Action's README calls "do not commit today". Dropping `art` turned a 146-day picture into a 79-day text, and `--backfill` then asked for a different date range and a different total. A plan is the input to `--backfill --write`, and contributions cannot be unlit. `deny_unknown_fields`, which costs the forward direction CHANGELOG 0.6.0 left open: a plan written by a newer mossaic is now refused. That is the honest trade — the file is version-locked to the tool that wrote it in practice already — and docs/ART.md's "Saving the plan" section says so. **A malformed `--file` blamed gh.** `--file` and the network share one parser whose only wording was "unexpected response from gh", so a truncated local file was reported as the GitHub CLI returning something odd, on a run where gh was never executed — and `{"data":{}}` was reported as GitHub having no such login. The reader then checks `gh auth status`, the username and the network: everything except the JSON in front of them. `--file` is the flag most likely to be handed a file another process is still writing, which is exactly the truncated case, and the plan loader two commands away already got this right. The source is threaded through `parse` now, and `--merge` stops quoting the path where nothing else does. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
…people copy Five findings in the parts of this repository that nothing here executes. **The release never triggered the workflows that build its binaries.** `binaries.yml` and `install.yml` both sat on `on: release`, and GitHub does not start workflow runs from events raised by `GITHUB_TOKEN` — the anti-recursion rule — which is the token `release.yml` creates the release with. Neither has ever run from that trigger: every run through 0.6.3 was a manual dispatch. Between the tag and somebody remembering, the release had no downloadable binaries for any platform, `brew install` still served the previous version, and nothing verified the published crate installs. That window existed for 0.6.0, 0.6.1, 0.6.2 and 0.6.3 and closed each time only because the release was being watched. `workflow_call` from `release.yml`, per the issue's option 1: the artifacts become part of the release rather than a thing that happens near it, and the trigger that cannot work is deleted rather than left in place. The tap token is named rather than inherited — a called workflow with `secrets: inherit` gets every secret this repository has, and that job needs exactly one. **An empty CHANGELOG section published the crate and then failed the release.** The extractor ran only in `github-release`, which `needs: publish`, so a section that was present and blank put the version permanently on crates.io and died afterwards on a message saying the section was missing — and RELEASING.md hung that failure on `verify-version`, the one job that never opened CHANGELOG.md. It runs there now, before anything irreversible, so `ci`, `semver` and `publish` all descend from a job that has proved the notes exist. The script tells "absent" from "present and empty" — the empty one is the likely accident, since the runbook's step 2 is a hand edit — and stops at the link block, so the oldest section no longer absorbs 15 lines of URLs. It is the only piece of the release path with no test at all, which is why three behaviours could sit in it; there is one now, confirmed red against the old script. **The workflow the project tells you to paste was the one file its audit never read.** `track.example.yml`'s first line says to copy it into a public repository of your own, and it is not under `.github/workflows/`, so neither the CI glob nor GitHub itself ever parsed it — CI was green and the file the gate existed for was the one it was not pointed at. Whoever followed the instructions inherited workflow-level `issues: write` for every step, including one running a mutable tag on somebody else's repository, in a workflow designed to run unattended. The scope moves to the single step that needs it, the third-party action is SHA-pinned, the job gets a name and a concurrency block, and pinning *this* project's action by tag is a recorded exemption with the rationale action/README.md already gives. Confirmed: 11 findings before, none after, at the version CI pins. **One action run asked GitHub twice.** Each `--format` call made its own query and its own `Local::now()`, so across a local midnight the json half reported `today-short=2` while the markdown body said "36 to go" — and the markdown never printed today's date, so the split was invisible. With auto-commit that back-dates commits for a day that has already ended. The markdown call is pinned to the date the json call reported, which is the rule CONTRIBUTING §3 already states. **`fail-on: holed` printed an annotation with no subject.** Its only message interpolated `text:`, which is empty by construction for `template:`, `matrix:` and `image:` — and a picture is where `holed` is most likely, since it uses the whole week. The subject comes from the report now, which the step had already parsed two lines earlier. Closed #9 fixed the same shape in the neighbouring gate. Also: the tracking header carries the placement in both formats, and `start-week` and `columns` are outputs. action/README.md told readers "the report prints the placement it used on its second line" and no line in either format carried it — the only guard the project offers against the one failure it calls silent and confident, adopted by the consumer as its stated safety net, describing something that was not there. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
**Sixteen documented blocks had drifted.** 0.6.3's plural pass changed the wording of the lines these pages quote and regenerated none of them, so every figure matched and every line was worded differently. They are not illustrations: `--today` exists precisely so "a documented sample stops being true overnight" cannot happen, and two of the blocks are labelled "to reproduce this exactly". A reader diffing their own output against the page could not tell wording drift from a real regression, and a contributor copying the documented wording into a new message reintroduces the plural the release removed. Regenerated from the real commands, and the backfill warning gets its verb back — 0.6.3 dropped the "are", so it read "61 days inside the letters already lit". `grep -rn 'day(s)\|commit(s)\|pixel(s)' README.md docs/ action/` is now empty. The durable half is the check AGENTS.md already claims exists — "Documentation is checked, not maintained. Where a README states a fact the code owns, there is usually a test asserting the two agree." A test now runs each documented invocation and asserts, in both directions, that the tool still prints the quoted line *and* that the docs still quote it. A test that fails is worth more than a regenerating script nobody runs. **CONTRIBUTING named two of five test files.** Its §2 table and §3 three-layer policy covered 39 of 223 tests, and its §1 count was 39% under. `art_cli.rs` — the largest file in the repository and where `mossaic-art`'s entire command-line contract lives — `chart_cli.rs` and `canvas_pty.rs` appeared in no contributor document at all. So a contributor adding a flag or changing an error message read §3, found three layers and two files, and either wrote a PTY test for something that prints and exits or wrote none. The table lists all five, §3 is four layers with the file named for each, the count is real, and README and AGENTS mirror it. The files always knew better than the docs did: `art_cli.rs:23` cites "CONTRIBUTING §3 asks for hermetic tests" and `canvas_pty.rs:3` explains why it is not `art_cli.rs`. The layering was real and deliberate; only the description of it was stale. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
Twenty findings and one security advisory, all reported against 0.6.3 with a measured reproduction. Also here: docs/ART.md's "Saving the plan" section gains the two caveats it was missing — that a plan is version-locked to the tool that wrote it, now that every key is checked, and that a picture plan needs a mossaic that understands `art`, since an older build ignores the key and silently draws the `text` field instead. That second one has always been true and was never written down. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
Two defects CI caught, and the first is mine to own plainly: **I wrote a commit SHA I had not verified** into a supply-chain pin. It happened to be a real commit in that repository — pointing at v3.12.0, not the v18 the comment claimed — so nothing locally objected. zizmor runs offline by default and cannot check a pin against the network, which is exactly the check that matters for a pin; CI runs it online and named it. The SHA is now `94de994a9f6fffee200243214e17002e2920bb59`, read from `repos/dawidd6/action-send-mail/git/ref/tags/v18` and confirmed to resolve. A fabricated hash in the file that tells strangers what to copy is worse than the mutable tag it replaced. And `signal-hook` is Unix-only — Windows has no `SIGHUP` and no `signal_hook::iterator` — so it moves under `[target.'cfg(unix)']` beside `libc`, `on_signal` is a documented no-op elsewhere, and the three signal tests are `#[cfg(unix)]`. The panic hook still covers the other unguarded exit on every platform. Windows has no POSIX signals to restore from, so claiming the guarantee there would have been a claim with nothing behind it. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #73, closes #78, closes #79, closes #80, closes #81, closes #82, closes #83, closes #84, closes #85, closes #86, closes #87, closes #88, closes #89, closes #90, closes #91, closes #92, closes #93, closes #94, closes #95. Fixes GHSA-jp9f-97rv-j4hx.
Nineteen of the twenty open issues, plus the draft security advisory, plus the 0.7.0 release. #57 stays open on purpose — it is an invitation to contribute templates, not a defect.
The security advisory first
The three
.artheader fields were printed exactly as the file wrote them, while every other piece of untrusted text went throughprintable()at its entry point — the rulesrc/lib.rsstates in as many words. A.artfile is the one thing this project asks strangers to send: a reviewer running--matrix theirs.art, or anyone running--list-templates, executed whatever the header said. It rode through--saveinto the plan, through--format json'sheadline, into$GITHUB_STEP_SUMMARY, and out of the Action's output;build.rsembedsart/templates/*.art, so a merged template would have shipped its payload to every user.Cleaned in the
Canvasmeta reader — one place, covering the report header, the listing, both report formats, the saved plan and the editor title — and bounded to 200 characters. Cleaning rather than refusing, deliberately: a.artfile authored on Windows carries a trailing CR on every line, and refusing any control character would reject it for something nobody typed.The recurring shape
Something the tool accepted, printed or counted, and then did other than what it said:
--pngwas taken in every mode and wrote the file in one.--commitswas described as pricing every lit day; it prices the brightest.--writemakes — right after a note saying cells had been dropped.--todaydid not move.Watched going red first
alternate_screenstill true).zizmoragainst the pre-fixtrack.example.yml: 11 findings before, 0 after, at the version CI pins.test-extract-changelog.shagainst the old script: 3 of its 5 shapes wrong.Notable scope calls
unsafe_code = "forbid"is kept. Killing the chart or the editor leaves the terminal in the alternate screen with mouse reporting on #82 and Every printing path panics when its reader has gone, and --png reports failure after writing the file #83 both named it as the obstacle and asked for one decision. Signals go throughsignal-hook, which registers safely, does its work on a thread rather than in a handler, and — the part that matters — is already in the tree: crossterm pulls it through ratatui at the same version, so this is an import, not a dependency. Broken pipes go through a panic hook rather thansignal(SIGPIPE, SIG_DFL), which would also give the wrong answer for--png, where the file is already written and the correct status is 0.--is strict POSIX: everything after it is the text.mossaic-art -- '-' --year 2027is therefore an error, and says so in as many words ("everything after -- is the text, so put the options before it") rather than silently taking--yearas a flag.deny_unknown_fieldscosts the forward direction CHANGELOG 0.6.0 deliberately left open. Taken anyway: the file is version-locked to the tool that wrote it in practice already, and applying a default nobody chose to the input of--backfill --writeis worse. docs/ART.md now says so, along with the picture-plan caveat that has always been true and was never written down.workflow_call), not the PAT: it deletes the trigger that cannot work rather than leaving one that does not, and adds no stored secret to a repository that deliberately has none. The tap token is named rather than inherited.github-releaserather than passing them through an artifact — two more third-party actions for acat. The gate is what moved beforecargo publish.What this leaves out
action/action.yml— is not built. Nothing in this repository runs those ~300 lines of bash, which is why a one-line interpolation bug could sit in the file's only failure path; that job is a bigger change than the one-line fix and deserves its own issue.--also-markdownthat would halve the GraphQL calls is not built.