refactor: deepen modules - #179
Merged
Merged
Conversation
chart.ts formatted milestone labels with a hardcoded 'en-US' while svg-chart.ts used formatCount with the run's locale, so the same Milestone read '1,000 star' in the email and '1K star' on the data branch. ChartMilestone now carries value and label; the label is formatted once in visibleMilestones and both adapters draw what they are handed.
- droppedSnapshots is now derived from the History addSnapshot returned instead of recomputing the trim rule, so the count cannot disagree with the array it describes - RepoTotal was SnapshotRepo under a second name in the same layer - toActionInputName is exported from the loader, so action-inputs.test stops keeping its own copy of the kebab-case rule - the eleven action outputs are now asserted against action.yml - the skip-email log names the reason instead of always blaming the threshold
reportParams was a 21-field literal the orchestrator assembled by hand, fifteen of them copied off config. Because it was a variable, TypeScript skipped the excess-property check, so generateMarkdownReport silently accepted and dropped eleven chart-style fields, and the whole markdown vs email split rested on one spread. ReportParams now carries config plus the run's data, both renderers take the same type, and html.ts reads emailTheme itself. A new chart option no longer touches @Application at all. Recorded as ADR 0016.
notify, mailDelivered and notificationDelivered were three lets mutated across a 25-line try/catch, and conflating the last two had already shipped a bug: a successful courtesy send reported notification-sent false. The rules lived in prose across two CLAUDE.md files and ADR 0011. settleNotification takes the run's changed/thresholdReached plus one Delivery and returns shouldNotify, notificationSent and historyToPersist together, so the baseline can only advance on a decision that held and a delivery that did not fail. recordNotification moves to notification.ts where the concept lives, avoiding a cycle with measurement. tracker.test.ts stops mocking it and now exercises the real settlement.
buildStarHistory and resolveChartHistory were each called twice, at two altitudes in two layers: @Application resolved the aggregate, charts.ts resolved every per-repo series, and the two agreed on one instant only because the shell threaded the same Date through both. resolveChartHistories owns both altitudes and creates the instant itself, exposing .aggregate and .forRepo(name). buildChartFiles drops from eight params to four; the tracker loses chartNow, repoTotals, starHistory and fallbackHistory.
The page spread, the paging-ceiling clamp, the threshold comparison and the Covered Stars arithmetic were unexported functions inside fetchAllStargazers, so their edge cases -- rounding collisions, a one-page budget, a non-positive budget, the ceiling -- could only be reached by driving a fake octokit through an await loop. @domain/sampling owns all four as pure functions with 13 colocated tests over plain numbers. The adapter fetches the pages it is handed and reports what came back; stargazers.ts loses 17 lines and every literal it used to restate.
worktree.test.ts and the commitAndPush tests drove execFileSync with positional mockReturnValueOnce chains up to seven deep, so adding or reordering one git call shifted every later mock and broke tests that looked unrelated -- a hazard the guide had to warn about. execute is already a seam and already separately tested, so these mock it and script failures by which git command ran rather than by call ordinal. Assertions read as git argv. Four new cases the old shape made impractical: the fetch+add path, the orphan cwd, the empty-orphan fallback, and no commit when nothing is staged.
writeReport, writeBadge and writeCsv were the same function -- path.join plus writeFileSync -- each behind its own four-line params interface differing only in the name of the content field, and their tests restated the implementation. writeArtefact takes an Artefact and looks the filename up in DATA_FILES; the JSON writers stay separate because they carry the format-version contract, and writeChart because it creates a directory. Translations was re-derived as ReturnType<typeof getTranslations> in four presentation files even though @i18n exports it directly.
smoothing, curve, showPoints, beginAtZero and theme were written as literals in both chart adapters, with no test that the two agreed. They now default from CHART_DEFAULTS. yAxisSide, animate and range stay local because they genuinely are not shared. Also records why the Reports print Star Counts raw while the Badge and the Charts compact them: formatCount trades precision for width, which is right on an axis tick and wrong in a table.
Tracked Set is a CONTEXT.md word, but the rules that produced it lived in the network layer and were written over GitHub's wire shape -- owner.login, stargazers_count -- with three core.info calls interleaved, which is the only thing that kept filterRepos out of the pure core. getRepos now maps to RepoInfo first and hands them to resolveTrackedSet, which returns the surviving repositories plus afterOnlyOrgs, afterOnlyRepos and invalidPatterns as plain data. The shell turns those into log lines; the domain still cannot log. The 21-case spec moves with the rules and drops its @actions/core mock. Three new infrastructure cases cover the logging that is now its job.
The divergence was stated in src/domain/CLAUDE.md as deliberate with no reason attached, which makes it look like drift a reviewer should tidy away. Unifying it produces a wrong number rather than a crash: a Forecast needs plausible spacing across many points and tolerates a synthetic cadence, while a Velocity's denominator is the timestamp difference itself, so the same fallback would fabricate a rate for a week that never happened. ADR 0017 records that, and names routing computeVelocity through calendarDays as the change not to make.
CLAUDE.md pinned pnpm 11.15.1 while packageManager said 11.21.0, in a section whose own heading explained it was not tested. The docs guard now asserts both pins against package.json, verified to fail on drift. Also corrects claims that had gone false: the wiki listed the filter pipeline with only-repos ahead of only-orgs when the contract is the reverse, presented mapRepos after filtering when it now runs before, listed 9 of the 11 outputs while omitting the notification-sent that its own prose contrasts with should-notify, and left only_repos, read_only and email_theme out of the config sample. application/CLAUDE.md still named mailDelivered, a variable that no longer exists. Technical-Stack.md restated the layer map with no indication that ARCHITECTURE.md is normative. Delivery joins the CONTEXT.md glossary.
CLAUDE.md states the tree contains no explanatory comments; it contained three. svg-chart.test.ts explained where 350 and 50 came from, so both now derive from CHART.height, SVG_CHART.margin.bottom and SVG_CHART.margin.top -- which removes the comment and seven magic numbers, and ties the assertions to the canvas constants they are about. star-history.test.ts moves its two into named predicates.
CONTRIBUTING said pnpm run check was 'linting + type checking' when it also runs the coverage gate, and pointed readers at it for a style check; lint is the narrow one. Its directory tree omitted src/shared and src/assets while the tip below it cited the @shared/* alias, and omitted dist/, which is what action.yml actually runs. 'Four artefacts' sat above a five-row table. The i18n page listed only dates as locale-aware on charts; compact star counts on the Y axis and on milestone lines follow the locale too, on both renderers now.
Every other layer had a single entry point -- measureRun for @Domain, withDataBranch for @infrastructure -- while @presentation had five, so the shell called four renderers plus buildChartFiles and assembled the params for each. renderRun returns markdown, HTML, CSV, badge and the chart files as one RenderedRun. It takes chartHistories and the stored History under separate names and derives the chart history itself, which retires the hazard of two adjacent, interchangeable-looking History fields where swapping them turned Velocity into an average over a chart bucket. tracker.test.ts keeps its 17 mocks on purpose: mocking @presentation/run would also stop buildChartFiles running, and the chart-request assertions pinning #148 and the per-repo timelines depend on it.
The renderRun commit staged sources without re-running pnpm build, so the committed bundle -- which is what action.yml executes, per ADR 0003 -- still contained the five-renderer call path.
renderRun called buildReportModel twice, once inside each dialect, and
prepareReportData reads new Date() -- so a run crossing midnight could
stamp the markdown Report and the HTML Report with different dates.
It also accepted topRepoNames from the caller while buildReportModel
derived the same list internally, so the run computed it three times. A
caller passing a different list would have had charts.ts write per-repo
SVGs for one set while markdown.ts linked ./charts/<file> for another --
broken images, no failure. renderRun now derives it from model.topRepos.
The dialects take { model, config }. run.test.ts covers the front door's
contract: one date across both Reports, Velocity from the stored
History, and the charted set matching the linked set.
…l prose namedFallbackField was byte-identical to scalarField but for one template literal, and only two of 34 rows used it; it is now a namesFallback flag on scalarField. action.yml documents a default in prose for every overridable input and nothing checked it. It also told 21 inputs they could come from the config file and stayed silent on the other 16 -- every chart-* option plus velocity-metrics -- which reads as input-only when all of them are file-readable. Both strings are now asserted against DEFAULTS. parseDecimal means finite and greater than zero, so it is now parsePositiveDecimal. loader.test.ts mocked 'fs' while the loader imports 'node:fs'; it worked through an alias, and now matches.
ReportModel exposed both hasChartHistory and chartHistory, equal by construction, so the two dialects each picked one and wrote the same rule two ways. hasChartHistory is gone and showComparisonChart is a model field rather than a condition each dialect assembles from chartHistory and topRepos.length. shared.ts meant 'imported by more than one file', not a concept. The chart windowing and series maths -- selectChartSnapshots, movingAverageSeries, buildForecastChartSeries -- had exactly one consumer, so they move into chart-spec.ts with their tests, and ForecastChartSeries stops being an unused export. shared.ts drops from 212 lines to 117. CHART_CHROME holds the four values both adapters draw identically on the same 800x400 canvas. Series dash patterns stay per-adapter: the SVG uses one for every dashed series, Chart.js uses three.
The module map still listed selectChartSnapshots under shared.ts after it moved to chart-spec.ts, and did not mention run.ts at all. Phase 7 described two report generators with no front door, and credited prepareReportData with deciding sections that buildReportModel decides.
Home.md advertised 'up to 52 weeks of historical star data', but max-history counts snapshots and a run appends one -- so on a daily schedule 52 is seven weeks, not 52. README and Data-Management already said snapshots; the landing page now defers to the schedule, and Data-Management keeps the per-cadence table.
SECURITY.md's 'Secrets Management' example marked
github-token: ${{ secrets.GITHUB_TOKEN }} as the good case. That token
is scoped to the triggering repository and cannot enumerate the owner's
repositories, which is the whole premise of ADR 0002 -- the policy page
was recommending the one value guaranteed to fail.
Its star-tracker.yml example also carried a reporting.email.smtp_password
key. No such key exists: SMTP is read from the workflow inputs and has
no config-file counterpart, and ${{ secrets.* }} does not interpolate
inside a committed repo file, so following it would have silently sent
nothing. It also claimed the data branch is optional and listed
'issues: write' for issues this action never creates.
The PAT page told fine-grained users that Metadata: Read-only was the
minimum. The action pushes the report, data, badge and charts with that
same token, so such a token fails at the push -- it needs
Contents: Read and write.
Read the six large pages end to end for the first time. Beyond the token guidance already fixed, they asserted several things the code does not do: - 'invalid values fall back to their default' -- an unusable input falls to the config file first, and a bad config-file value warns only for the enum keys - send-on-no-changes documented under a precedence chain it is not in; it is the one input with no config-file layer - 'a snapshot at least that old' for windowed compare-against, stated four times, omitting the deliberate 6-hour tolerance - 'only resets when a notification actually fires', in seven places, where settleNotification advances the baseline only on delivery -- Email-Notifications already said 'delivered' two lines away - forecasts 'require 3 runs': they are fitted to the reconstructed history, which has ~30 points on the first run - stargazers.json storing avatar URLs, contradicted six lines later - data colours 'remain unchanged' across themes, and email dark mode 'not supported', when email-theme exists and three series recolour - an unquoted hex advised for the config file, where an all-digit value loads as a number and is silently ignored Email-Notifications raised an IMPORTANT against html_body and then used it twice; Examples piped reports through echo. Two anchors pointed at page roots, one link broke on unescaped parens, and stars-data.csv was missing from both data-branch listings.
Read CONTEXT.md, ARCHITECTURE.md, the nine CLAUDE.md files and all 18
ADRs in full. Contradictions found:
- ADR 0008 says a Sampled Repository 'contributes nothing to the
remembered Stargazer set'. buildStargazerMap carries its previous
logins forward -- ADR 0012 changed that and neither Status said so
- ADR 0017 claims growth.ts owns arithmetic 'both genuinely share',
naming three functions. Velocity imports only latestRateInterval
- ADR 0016 describes tracker.ts calling both renderers with ReportParams;
renderRun does, with { model, config }. Amended in place
- ADR 0014's Decision said the spec carries milestone thresholds while
its own Consequence says they carry labels too
- ADRs 0006 and 0010 both assert the two renderers are independent,
which 0014 narrowed without amending either Status
- ARCHITECTURE listed topRepoNames in renderRun's signature; it has no
such param
- 'the chart trio' in two files for four chart modules
- src/shared/CLAUDE.md called the folder testing/; it is tests/
- presentation/CLAUDE.md used the past tense for a hazard that is still
live, just confined to one file now
- root CLAUDE.md said one test covers no module, then named a second
Removed duplication: ARCHITECTURE restated the whole per-layer guide
table that root CLAUDE.md owns and loads every session.
Also normalised every markdown back to LF -- earlier edits in this
session had written CRLF, which .gitattributes forbids.
chart.test.ts asserted expect(url).toBeDefined() before 38 'if (url)' guards. toBeDefined passes for null, and null is exactly what chartImageUrl returns when it declines to draw -- so in ~20 tests a regression to null passed the assertion and then skipped every real check inside the guard. not.toBeNull() fails at that line instead. Verified: forcing chartImageUrl to return null takes the file from 46 passing to 40 failing. svg-chart.test.ts already had this right via its expectSvg helper.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Two verification agents reviewed the corrections against the code and caught eight of my own. The worst was an overcorrection: I rewrote seven places to say the notification baseline advances 'only when actually delivered'. settleNotification advances it unless a CONFIGURED send failed -- so with no SMTP at all it still advances, which is ADR 0011's deliberate design and the commonest setup. The wording I replaced was closer to right than the one I wrote. Also wrong in my edits: - listed visibility among keys that warn; it throws - stated 'falls to the next layer' as a blanket rule, but enum keys and chart-custom-milestones go straight to the default - called send-on-no-changes the single input with no config-file counterpart, contradicting SECURITY.md about the SMTP inputs - claimed data colours recolour with the media query; they are inline attributes resolved once, so auto keeps the light palette - marked chart-theme as not reaching email, when email-theme defaults to auto meaning 'same as chart-theme' - said email Y-axis counts are localised; Chart.js draws those ticks - ADR 0010's amendment listed the point cap as now shared; it is the one thing that is still email-only, per that ADR's own consequences - ADRs 0012 and 0014 needed their own Status blocks updated, since the contract requires saying so in both
Dialect parity is now a test, not a sentence in CLAUDE.md. A table-driven case in run.test.ts asserts each section the ReportModel can switch on appears in BOTH reports, plus that all optional ones vanish together. Verified: suppressing the HTML New Repositories block fails it. One past commit had to close four such gaps at once. Deleted twelve content assertions duplicated into svg-chart.test and chart.test from chart-spec.test, which owns them. One of them, toBeLessThanOrEqual(10) over a path count, passed at three paths and could not fail. The two files drop 233 lines and lose no coverage. notificationIsDue in @domain/notification is now the single owner of "changed && thresholdReached". The tracker gated the send on its own copy while settleNotification computed the same rule for the output, with nothing asserting the two agreed. Smaller items: movingAverageSeries and buildForecastChartSeries were exported with no consumer outside chart-spec.ts; one forecasts[0] access was unguarded while an identical one 270 lines earlier was not; markdown.ts had reintroduced a local hasChartHistory; setEmptyOutputs duplicated the output contract and emitted '' where every other run emits a CSV header; and the email subject was the only user-facing string the shell composed itself, now rendered by renderRun from a new localised report.noRepositories key in all four bundles.
The previous commit declared an identical EMPTY_SUMMARY in both tracker.ts and run.ts. It is domain data -- a Summary with every figure at zero -- so it now sits in @domain/comparison beside compareStars, which is what produces Summaries.
… stays zero-arg
Two reviews proposed parameterising loadConfig({ inputs, file }). Audited
properly, it deletes 49 lines of loader.test.ts -- 27 of them the same
existsSync mock repeated -- and zero test cases. The readFileSync and
mockInputs sites are not deletions; the YAML and the input object move
from a mock call into an argument.
Worse, it does not remove the mocks it is meant to remove. core.warning
fires from five sites inside the resolution and 18 assertions pin its
wording, so vi.mock('@actions/core') stays either way. And for the
tracker to pass an inputs record it must enumerate the input names,
which means exporting TABLED_KEYS -- the coupling ADR 0016 removed.
Recorded as ADR 0018 so a fourth review does not re-propose it.
parsers.ts had no colocated test: 233 lines of its spec, needing no
mocks at all, sat inside loader.test.ts and overstated how much of that
file the ambient reads were responsible for. It has parsers.test.ts now,
and loader.test.ts drops from 1178 to 933 lines.
Also covers renderEmptyRun and emailSubject, which I added last commit
with no tests at all -- the subject line is what a user reads in their
inbox and nothing pinned it.
Config is projected onto a style bag twice -- inline in charts.ts for the SVG, by emailChartStyle for the email -- and six options appear in both. That has drifted twice: chart-smoothing, and later chart-line-color/chart-line-width, shipped honoured by the SVG alone and each needed a later three-file fix to reach the email. I was going to merge the two projections. An audit killed it: a shared type would save one line per option and assert a parity that is false. chart.ts collapses rounded-step onto Chart.js monotone and both catmull-rom and cubic-bezier onto a plain tension spline, and theme diverges on purpose -- chartTheme for the SVG, emailTheme for the email, which ADR 0010 forces. Merging would turn a benign one-line restatement into a cross-adapter contract the adapters are designed to break. run.test.ts renders a run twice per shared option instead and asserts the change reaches both systems. Writing it caught the collapse immediately -- rounded-step and monotone produce byte-identical email charts -- so that is now pinned as the one deliberate exception rather than left as prose. Also corrects presentation/CLAUDE.md, which called range "email only". charts.ts passes it too; the sentence warning to keep the two lists in step was itself miscounting them.
The ReportModel already carried the header date so the two dialects could not disagree about it, but each footer still called new Date() on its own. The same midnight hazard that fix closed stayed open one layer down: the headers agreed and the footers did not. renderRun now takes an injectable now, as resolveChartHistories already does. prepareReportData derives model.now and model.generatedAt from that single read, and both renderers take the stamp off the model rather than the clock.
Two defects with one cause: ChartHistories stopped at renderRun, so the report dialects could not ask for a repository's own history, and nothing reconciled what the reports linked against what was actually drawn. charts.ts drew each per-repo SVG from forRepo(name) while html.ts drew the same chart from the aggregate. buildStarHistory anchors its earliest edge to the earliest star among the repos it is handed, so the two disagree on every bucket edge and a young repo gets a flat lead-in in the email it does not have in the SVG. markdown.ts linked ./charts/<repo>.svg for every top repository whenever the aggregate had a chart, but renderSvgChart returns null when that repository's own history is too short - a broken image in the published report. renderRun now draws first and builds the model with a hasChartFile predicate. ReportModel.perRepoCharts is the drawn set, each entry carrying the history it was drawn from, and both dialects iterate it.
Verified each against the source before changing it: - How-It-Works said a first run gives every repo isNew: false. compareStars sets isNew: previous === null, so on a first run every repo is new - which is why summary.changed is true at all. - Known-Limitations gave the forecast as slope * (n - 1 + week) + intercept. forecastFromSeries never uses the intercept and anchors on the last observed value; the weighted rate is per day, so both need the factor of seven. - Known-Limitations called the email PNGs light-only with a fixed white background, contradicting the row above it. buildChartUrl paints palette.white, and DARK_PALETTE.white is #0d1117. - Known-Limitations gave the fine-grained token as Metadata: Read-only. The action pushes with that token, so it also needs Contents: Read and write. - The README flow diagram had commit before email and setOutputs in the middle. The run sends before it publishes, and setOutputs is last. - setEmptyOutputs does not exist; there is one setOutputs. - getLastSnapshot and parseNumber are module-private, and the parsers list omitted the ones a key actually picks between. - email-theme is a DEFAULTS key, so it is config-file settable; only email-from and email-to are workflow-only. - only-repos narrows what only-orgs selected rather than overriding it. - velocity skips any pair closer than the minimum rate interval. - CONTRIBUTING asked for JSDoc and 80% coverage, contradicting itself and the 85% the config enforces. - The bug template seeded GITHUB_TOKEN and a nested config block; the loader reads top-level keys only, and ADR 0002 rejects that token. - The PR checklist named CLAUDE.md sections that do not exist.
The notification-threshold accumulation clause was written out verbatim in eleven places and the fresh-branch/raised-threshold paragraph in four. Both now live once, at Configuration#notification-threshold, and the other pages carry a sentence and a link. Eleven copies of one rule is eleven chances to drift. CONTEXT.md is vocabulary only by its own charter, so Sampled Repository and Rate Interval drop the because-clauses that restate ADR 0008 and ADR 0017, and Notification Threshold stops repeating the accrual rule. Delivery keeps its contrast with the notify decision - that is what separates the two terms rather than rationale for either.
- How-It-Works Phase 4 walked through getBaselineSnapshot, compareStars and addSnapshot as if the run called them one by one. ADR 0013 made measureRun the domain's only entry point; the phase now says so before describing what it does inside. - Data-Management listed the comparison and per-repo SVGs as written on every run with charts on. Both are conditional, and the per-repo one now depends on that repository having enough history to plot. - ADR 0018 carried a heading the template does not define; the question it asked belongs at the top of Context.
Nothing asserted there was false, but four of my edits were weaker than what they replaced: - The Data-Management chart table advertised a distinction that does not exist. "at least one tracked repository" can never be false, and the real guard - buildChartFiles returning [] below MIN_SNAPSHOTS_FOR_CHART - applies to every row including star-history.svg, which I had left as "every run". - The parser list omitted parsePositiveDecimal, the only decimal parser, in a sentence whose whole point is that the choice of number parser is deliberate. The module tree a few hundred lines down listed a different five again. - The only-repos cell dropped the half of "overrides other filters" that was true: it does skip the archived/fork/exclude/min-stars filters. - The README decision node was labelled "SMTP configured?" when the guard also requires the notification to be due, and the diagram had no edge for the no-repositories path that returns before the worktree opens.
Two defects of the same family as last round's chart fix. The per-repo forecast was fitted to the aggregate reconstructed history while the chart printed directly above it came from the repository's own. Those are different series - buildStarHistory anchors its earliest edge to the earliest star among the repos it is handed - so a young repository's forecast read the long flat lead-in the aggregate gives it and reported the repo as nearly static. computeForecast now takes historyForRepo and falls back to the aggregate when a repository has fewer than the three snapshots a forecast needs, matching resolveChartHistory's existing candidate/fallback shape. buildStargazerMap built its result only from the repositories observed this run, so any repository that left the Tracked Set - a min-stars boundary, an edited filter, a day spent archived - had its remembered logins erased. On its return every existing stargazer was reported new: exactly the fabricated spike ADR 0012 exists to prevent, reached by a different route than the failed fetch it covers. Seeding the map from previousMap closes it and removes the special case for sampled and incomplete repositories, which the seed now covers. Also: action.yml claimed compact star counts stay English; formatCount builds its formatter from the run locale and a test is named after that. And the two coveredStars formulas are deliberate - the infrastructure guide now says so instead of claiming the domain owns both.
CLAUDE.md states it as absolute, and the tree had drifted in nine places. Three were the shape the rule exists to prevent: cumulativeCounts(number[], number[]), scaleCappedToTrueTotal(counts, trueTotal, reachable) and roundTo(value, decimals) all compile with their arguments swapped, and the first two would still render a plausible-looking chart afterwards. All nine now take a destructured object, and docs-consistency.test.ts asserts the rule over every non-test source file so it cannot drift again. Verified the guard fails on a deliberately reintroduced violation rather than passing vacuously.
The action runs on node24 while engines.node pins 26.2.0 and @types/node tracks the development version. esbuild's target lowers syntax without shimming runtime APIs, so a node: API added after 24.x type-checks, bundles, passes pnpm validate, and throws on a GitHub runner - in a user's workflow rather than in CI, since dist/ is committed and nothing executes the bundle. Nothing exercises that today; the tree reaches only for node:fs, node:path and execFileSync. Recorded where someone bumping the pins will read it.
measureRun accepted a now and threaded it to getBaselineSnapshot only. createSnapshot read the wall clock un-injectably, so injecting a clock produced an updatedHistory whose newest snapshot carried the real time - a baseline resolved as of the injected moment sitting next to a snapshot stamped from another. Two tests already passed a now and neither covered the half that did not honour it. createSnapshot now takes the same optional now, and the domain guide no longer records it as a permanent exception.
Fitting each Top Repository to its own History means a repository whose reconstruction was thin falls back to the Stored History while its neighbour uses the reconstructed one, so two rows of the same table can be fitted over different cadences. That is deliberate and worth stating: days always comes from the same History as values, so each row is internally coherent, and a row right for its own repository beats a table uniformly wrong.
… stored An audit of 4da81a1 found I had made one class of repository worse than before the fix. historyForRepo was wired to chartHistories.forRepo, which resolves to the Stored History when a repository has no usable reconstruction. In that history repoStarSeries yields 0 for every snapshot taken before the repository joined the Tracked Set, so the curve is fitted to a fabricated 0 -> total ramp: a 500-star repo projected to 4,700 in four weeks. A repository whose stargazers cannot be read - a failed fetch, or the admin-only listing of ADR 0002 - has no reconstruction at all, so it took that path every run. Fitted to the aggregate it is held flat at repo.stars by the issue #148 guard, which is the honest answer. resolveChartHistories now exposes reconstructedForRepo, which returns null instead of falling back, and forRepo is defined in terms of it. Charts keep the fallback; the forecast does not get one. Also rewrote the fallback test the audit proved could not fail - every snapshot had repos: [], so both branches produced all-zero forecasts and the assertion compared two identical zeroed objects. It now carries per-repo stars and fails against the mutant that removes the guard. And the wiring line in tracker.ts, which could be deleted with all 971 tests still passing, is now pinned.
- .github/renovate.json was invalid JSON (trailing comma), so Renovate fell back to defaults and silently dropped minimumReleaseAge, pinDigests, the schedule, and the labels renovate-auto-approve.yml gates its if: on. It survived because biome.json's includes list had "*.json", and a Biome glob star does not cross a slash, so nothing under .github was ever linted. Widened includes to cover .github and docs. - test:changed ran "vitest run --changed HEAD", which means uncommitted changes. At pre-push time the tree is clean, so the set is always empty: every push in this branch printed "No test files found, exiting with code 0". Now --changed origin/main, so the hook covers the commits being pushed. - action-inputs.test.ts compared action.yml's outputs against a hand-copied array in the same file, while its name claimed to check what setOutputs emits. Deleting a core.setOutput call passed. tracker.test.ts now compares the emitted names against the manifest. - worktree.test.ts asserted cwd was "any String" on the checkout call only. Dropping options from the commit call - which would commit onto the user's primary branch - passed. It now pins path.resolve(dataDir) on all three. Each was verified by reintroducing the defect and confirming the gate fails. Also adds a dist/ drift check to ci.yml, and removes a dead binding in markdown.ts that the widened lint surfaced.
c5161a0 described widening biome.json's includes and changing test:changed to --changed origin/main. Neither was in the diff. The script that made them validated renovate.json in between and threw, so both later writes never ran while the commit message was already written. The message was wrong; this commit makes it true. Widening the lint immediately earned itself: biome now checks 101 files instead of 99, and docs-consistency.test.ts - previously outside every lint path in the repo despite being typechecked - had unsorted imports.
incomplete was `repo.stars > 0 && stargazers.length === 0` - true only for a total failure. A fetch that died mid-pagination returned a non-empty list, so it was flagged neither incomplete nor sampled, and both domain guards let it through: buildStargazerMap overwrote the stored entry with the partial list and the next successful run diffed the full list against it. Concretely, on the default configuration: a repo with 1,500 stargazers, a secondary rate limit on page 6. The run keeps the 500 oldest, warns, and succeeds. stargazers.json is now wrong on the data branch. Next run reports 1,000 new stargazers and emails a list of people who starred months ago - the exact fabrication ADR 0012 exists to prevent, through a case it did not cover. The signal already existed: a complete fetch leaves coveredStars undefined and a truncated one fills it, so on the unsampled path coveredStars !== undefined means truncated. incomplete now means "this list is not the whole story" rather than "this list is empty".
The gate I added in c5161a0 compared dist/ byte for byte and failed on every PR. dist/index.js is not reproducible across platforms: esbuild embeds node_modules/.pnpm/... paths as comments, and pnpm hashes those directory names on Windows to stay under the path limit while leaving them intact on Linux. So a bundle built on Windows never matches one built on the runner, however faithfully the author rebuilt it. A gate that always fails is worse than none - it teaches people to ignore it. It now compares which files changed rather than their contents: a PR that touches a bundled source without touching dist/ fails, which is the mistake worth catching. Test files are excluded since they are not bundled.
…branch initializeDataBranch ran ls-remote --exit-code inside a bare catch, so any failure - DNS, a GitHub 5xx, or an auth error - meant "the branch does not exist". The run then created an orphan branch, pushed it over the real one, was rejected as non-fast-forward, and told the user that another run had raced it and to add a concurrency group. That remediation cannot fix an auth failure. Dropping --exit-code makes absence what git already reports it as: exit 0 with no output. A genuine failure now propagates git's own text. The systematic version of the same trap: ls-remote and fetch ran with no credentials while only the push carried the token, relying on whatever actions/checkout persisted. On a private repository with persist-credentials: false - what OpenSSF, zizmor and this repo's own six workflows use - the probe failed every run. All three remote commands now share authenticatedArgs, which also owns the core.setSecret masking, because execute puts the full argv in its error message. Note the test-double change: a matcher on args[0] no longer sees an authenticated command, whose argv begins with -c.
Round 8 seeded the stargazer map from the previous one, which fixed a fabricated spike: a repository that fell out of the Tracked Set had its logins erased, and reported every existing stargazer as new on its return. A repo sitting on exactly min-stars produces that from one unstar and one re-star. The cost is that nothing prunes the file, where pruneCharts solves exactly that for charts. A grace period would bound it, but stargazers.json is deliberately a flat unversioned map so no reserved key can collide with the data - there is nowhere to record a last-seen marker without changing the on-disk contract for every user. Hard to reverse, surprising without context, a real trade-off: an ADR rather than a silent edit. The wiki now says untracking a repository does not withdraw its published logins. Its stated remedies - a private data branch, or leaving track-stargazers off - are unaffected.
…object
readHistory destructured whatever JSON.parse returned. A stars-data.json
holding null threw a raw TypeError instead of the actionable message written
for exactly this case, and one holding [], 5 or a string destructured to {},
normalized to { snapshots: [] }, and made the run treat a populated data branch
as a first run: it appended one snapshot, pushed, and discarded the user's
entire tracking record while reporting success.
The module guide already stated the invariant - "invalid JSON throws and does
not fall back, silently resetting corrupt history would destroy a user's
tracking record" - but it only held for text that failed to parse.
A snapshots key that is not an array still normalizes to [], which is a
different case: the surrounding object is intact and starsAtLastNotification
has to survive. That one stays pinned as it was.
writeHtmlReport ran inside setOutputs, which is called after branch.publish. A failed write - a full disk, or RUNNER_TEMP unset so it falls back to a read-only cwd - therefore ended a run that had already committed, pushed and sent the email, with setFailed and nine of eleven outputs unset. Re-running then appends a second snapshot for the same observation. The path is now computed before publish and passed in, so setOutputs does what its name says. tracker.test.ts pins the order, and fails when the write is moved back. Also renames a data-branch test that claimed withDataBranch does not open a worktree when the caller never publishes. It always opens one - initializeDataBranch is called before the try - and the assertion was about commitAndPush.
sync-wiki.yml declared COMMIT_NAME and COMMIT_EMAIL from the head commit's author and never referenced either; the script hardcodes the identity. The indirection was the right pattern for keeping attacker-controllable commit metadata out of a run: block, but the intent changed and the declarations were left behind suggesting an attribution that does not happen. Using them instead would change who the wiki commits are attributed to, which is a maintainer's call rather than a cleanup. zizmor.yml pinned actions/checkout to a different SHA than the other four workflows, with no version comment - and Renovate's pinDigests needs that comment to know what it is looking at, so the security-scanning workflow would have drifted onto an ever-older checkout. All five now share one pin.
…ential 536d858 authenticated ls-remote and fetch by prepending -c http.extraheader=AUTHORIZATION. That was a total outage for the workflow the README recommends, and neither the suite nor this repo's CI could see it. http.extraheader is multi-valued and -c APPENDS rather than overrides - verified locally: two -c flags yield two values. actions/checkout persists its own credential under the URL-scoped http.https://github.com/.extraheader and persist-credentials defaults to true, so every remote command sent GitHub two AUTHORIZATION headers and got back HTTP 400 "Duplicate header" - rejected at the edge before any credential was evaluated, so two valid tokens fail the same way. Worse, the same commit removed the bare catch that used to swallow a failing probe, so the failure went from silently-wrong to fatally-wrong: a read-only run now produced nothing at all. A leading -c http.extraheader= resets the list, which is what git documents for exactly this case. Using the URL-scoped key instead does not help; it accumulates the same way. The test asserting the argv pinned the broken shape and went red on the fix, which is how the audit found it. It now asserts the reset, and the setSecret assertion checks the value instead of just that it was called. Two corrections to 536d858's own message: this repo has five actions/checkout steps across seven workflows, not six; and persist-credentials: false is the norm for this repo's CI, not for users of the action - which is precisely why the collision was invisible here.
3e0b0ed added a leading -c http.extraheader= on the strength of an audit that said actions/checkout's credential and ours combine into two AUTHORIZATION headers, that GitHub answers HTTP 400 Duplicate header, and that the action was therefore broken for every user on the documented workflow. None of that is true. Traced against the real remote with GIT_TRACE_CURL: url-scoped X-Scoped + bare X-Bare -> only X-Scoped is sent bare X-Dup twice -> both are sent url-scoped X-Checkout + reset + bare X-Action -> only X-Checkout is sent Git accumulates multiple values of the same key, but a URL-scoped entry replaces the bare list rather than adding to it. actions/checkout writes the URL-scoped key, so ours was never on the wire at all in the default setup - there was no duplicate, no 400 and no outage. The reset was a no-op, and an empty bare value cannot clear a URL-scoped list anyway. So the shape 536d858 shipped was already right, for a reason neither review identified: our header is a fallback that applies when checkout persisted nothing, which is exactly the persist-credentials: false case it was meant to fix. It does not and cannot override checkout. Removing the reset rather than keeping it harmless: code whose stated justification is false is worse than no code, because the next reader believes the comment. The layer guide now records what was traced, and says not to add a reset back without tracing what git sends.
Owner
Author
|
🎉 This PR is included in version 1.26.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
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.
Description
Type of Change
Related Issue
Fixes #
Changes Made
Testing
pnpm test)pnpm build)Screenshots (if applicable)
Checklist
pnpm check)CLAUDE.mdmy change affectsaction.yml, the wiki and the READMECONTEXT.md; a hard-to-reverse decision has an ADRAdditional Notes
GIF (mandatory)
Thanks for contributing! ✨