refactor: close open findings and act on a maintainability review - #6
Merged
Merged
Conversation
Everything scripts/site.sh runs — mdoc, Laika through scala-cli, and scaladoc — is launched through Coursier rather than through Mill, so each one inherited whatever `java` happened to be first on PATH. The classpath they are handed is Java 25 bytecode, because build.mill sets `-java-output-version:25`. On a machine whose ambient java is older, the two met two steps into the build as: UnsupportedClassVersionError: com/worxbend/codeberg4s/auth/Auth has been compiled by a more recent version of the Java Runtime (class file version 69.0) … which names neither this script, nor the version pin, nor what to do next. CI never hit it: the toolchain action installs JDK 25 globally, so the ambient java there is already correct. The failure was reserved for a contributor on their first local run — the reader least equipped to decode it. The script now reads .mill-jvm-version, which is already the one place this project records which JDK it builds with, and either confirms the ambient java satisfies that pin or resolves exactly that JDK through Coursier. Coursier's JVM index is the same one Mill uses to honour the pin, so wherever ./mill has run the JDK is already unpacked in the cache and the lookup takes milliseconds. The version number is derived from the pin file and never written literally, so there is still a single source of truth. When the ambient java already satisfies the pin nothing is exported, which is the path CI takes — it keeps the JDK it installed and downloads no second copy. Resolving is announced with a `note` rather than an `announce`, because `announce` numbers the pipeline steps and those numbers are referenced by name in site/README.md and in --help.
The identifier types gained compile-time constructors, so a literal now
goes straight in: `Owner("forgejo")` is checked while the code compiles
and *is* the `Owner`, with no `Either` to unwrap. `Owner.from` is
unchanged and is still the way in for a value known only at run time.
Seven places in the documentation still said the opposite — that
`Owner("forgejo")` "does not compile" because the types are opaque and
have no public apply. That was true when it was written and is now
false. mdoc compiles every tagged Scala snippet on the site against the
real library, which is what caught the stale *imports* in the previous
commit, but prose and Scaladoc are not compiled, so these rotted
silently.
One of them was actively harmful rather than merely stale: the
troubleshooting table listed the compile error "Owner does not take
parameters" and told the reader to replace `Owner("x")` with
`Owner.from("x")` — instructions for repairing code that now works. It
is replaced by the two errors the literal constructor can actually
produce, both quoted verbatim from SegmentLiteral: "not a valid owner:
…" for an invalid literal, and "has to be a string literal; use `.from`"
for a run-time value handed to the literal form.
The rule is stated the same way everywhere: a literal gets the
constructor and is the value, a run-time value gets `from` and an
`Either`, an invalid literal is a compile error, and the literal form
refuses surrounding whitespace where `from` trims it. The two example
programs keep using `from`, with the Scaladoc now saying why that is the
right choice there — their values come from the environment.
`CodebergError` has six cases. `WalkTruncated` — a walk over every page stopped at its page cap with pages still to come — is one of them, and it carries no `CallContext` because nothing went wrong on the wire. Five documents still said five and listed only the other five: the landing page heading and its family list, Getting Started, the errors guide, RELEASING.md's breaking-change checklist, and CONTRIBUTING.md's Scaladoc rule. README.md was already correct at six, which is how the discrepancy showed up. The errors guide was the worst of them. It said "There is no sixth, and the compiler will tell you if you forget one in a match" — a promise that is exactly backwards for a reader writing an exhaustive match, since the compiler will indeed tell them, about the case the guide told them did not exist. Its table gains the missing row. RELEASING.md matters for a different reason: it lists adding an `enum` case as a breaking change, so an undercount there understates what a future case costs.
The pre-release quality pass changed the public surface in four ways and the changelog recorded none of them. That entry is the release note the 0.1.0 tag will carry, and its own preamble says the "Changed" section exists for anyone who built against a 0.1.0-SNAPSHOT jar before the surface froze — which is exactly the audience these four affect. Added: the compile-time literal constructors for path identifiers, and the root re-exports that make `import com.worxbend.codeberg4s.*` enough for the quick start. Changed: `Owner` and `RepoName` moving to the package root, `pageParams` becoming `params`, and the six `RepositoryApi` sub-resource listings losing their `list` prefix. The six method names are taken from the rename commit rather than reconstructed from the new names, and the entry says which `list…` methods were deliberately left alone, so a reader can tell the rename was scoped rather than partial.
The landing page opened with four teaser cards and then, immediately
below them on the same screen, the section "The four properties that
matter" — the same four claims, same order, same titles, written twice.
A reader scrolled past the argument, then past a compressed copy of the
argument.
The prose is the version worth keeping. It is a strict superset: it
carries the `Owner("forgejo")` versus `Owner.from(raw)` contrast, the
"pick one per call site, not per project" instruction, and the in-page
link to the clamp hazard. A Helium `Teaser` is a title and a description
rendered as plain text, so it can carry none of those — it cannot even
link to the guide it summarises. Cutting the prose down to card-sized
blurbs instead would have deleted exactly the material that turns a claim
into something a reader can act on, and would have moved front-page copy
out of a Markdown file a prose contributor can edit into a Scala string
literal in the build driver.
The header still offers three entry points — Get started, Examples, API
reference — so the fold is not left empty.
The teasers had also gone stale: the third one still described the
identifier types as having "Either-returning smart constructors", which
stopped being the whole truth when the compile-time literal constructors
landed. That is one fewer copy of a claim to keep in step.
`body > main` takes 56px of top padding where it had 8px, because the
teaser band supplied that gap and is gone. The dead `.teasers`/`.teaser`
rules and their two responsive blocks are removed with it, and the
`landingPage` call keeps a note saying why `teasers` is absent, so the
next person to read it does not restore them as an oversight.
`OrganizeImports` was set to `groupedImports = Explode`, which writes one import per line. When the shared codec helpers were promoted into common modules, that turned 108 files in `codec` into 108 copies of the same seven-line preamble. PMD's copy-paste detector has no import filter for Scala, so it counted every pair of those preambles as duplicated code. The duplication gate in verify.sh went from 363 groups to 408 — above its baseline, so `--with-slow` was failing — while the duplication that actually matters went *down*: measured at the baseline commit and at HEAD with the same tool, groups containing real code fell from 222 to 183. Of the 408, 324 contained nothing but a package clause and imports. The gate had stopped measuring what it claims to measure. `Merge` collects several names from one package into one braced import. That is still the explicit style SCALA_CODE_STYLE.md asks for — every name is written out, no wildcards appear, and `removeUnused` still prunes names inside the braces, so an unused helper stays visible. The count drops to 161, comfortably below the recorded baseline. `trailingCommas` moves from `multiple` to `keep` because the two formatters otherwise rewrite each other: scalafix emits a merged import with no trailing comma, scalafmt adds one to any list long enough to wrap, and `mill __.fix --check` removes it again — a gate that fails whichever order its steps run in. `keep` preserves what is written rather than adding or removing, so they converge. It costs the automatic enforcement the old setting gave on newly written multiline lists; every list already in the tree keeps its comma, so no existing code changes.
Mechanical output of `./mill __.fix && ./mill __.reformat` after the previous commit changed `groupedImports` to `Merge`. No logic changes. 309 files, 967 lines shorter overall. Verified mechanical rather than trusted: for all 307 changed Scala files, the set of fully-qualified names each file imports is byte-identical before and after, so nothing was dropped or added — including any `given`, whose loss would change implicit resolution rather than fail the build. `./verify.sh` passes, with the same 3604 tests.
`verify.sh --with-slow` was failing: 408 groups against a baseline of 363. It now measures 161 and passes. The comment block explains what the number means and, more importantly, what it cannot be compared to. The drop was not earned by deleting code. Most of the 408 was file preamble — 324 of those groups held nothing but a package clause and imports, because one-per-line imports gave 108 codec files an identical seven-line header and PMD counts that as duplication. Merging same-prefix imports collapsed it. The part that *was* earned is recorded too: duplication containing real code fell from 222 groups to 183 across the refactor, measured at both commits with the same tool. That is the number the gate was always trying to watch, and it moved the right way. The old measurements stay below as history, with a line saying they form a series only among themselves. Without it, the next reader sees 363 -> 161 and banks a win that half belongs to a formatter setting.
`modules/transport` declared `moduleDeps = Seq(core, codec)` and never referenced codec: grepping the module's source and tests for "codec" returns nothing. ADR-0003 states the rule as a fact — the transport reads every response body as a `String` and hands it to the codec module, precisely so the JSON library stays out of the transport. With the dependency edge left in place, that rule was enforced by nothing but a grep in verify.sh step 6, which matches `^import com.github.plokhotnyuk…` and so misses a fully-qualified `com.github.plokhotnyuk.jsoniter_scala.core.readFromString` written inline. Every other module in this build gets its boundary from the dependency graph and therefore from the compiler. Removing the edge makes the compiler the enforcement, the same way it already is for domain and core, which declare no `mvnDeps` at all. The grep becomes a second line of defence rather than the only one. It also corrects the published POM: `codeberg4s-transport` made consumers pull `codeberg4s-codec` and jsoniter for a module that uses neither. After 0.1.0 that edge would read as a commitment.
…move `PathSegment` moved to the package root, but nine Scaladoc references in eight files still spelled `com.worxbend.codeberg4s.repositories.PathSegment`. These are the links that explain why an identifier's validation is a security boundary rather than a convenience — the reader who follows one is the reader trying to understand why they cannot build an `Owner` from an arbitrary string. They resolved to nothing. The build cannot catch this: `scalaDocOptions` deliberately strips `-Werror` so an unresolved link cannot fail `docJar`, because Maven Central rejects a bundle with no Javadoc jar. That decision is left alone. The warnings it still prints were used instead to confirm the fix is complete — `./mill __.docJar` now reports no unresolvable member links anywhere in the tree.
Thirteen command and query types paired a validating smart constructor
with a fully public case-class constructor, so the validation was
optional. Demonstrated against the built library, from outside it:
AddTrackedTime.of(1500.millis) // Left(spent, must be a whole
// number of seconds)
AddTrackedTime(1500.millis, None, None)
// constructs, no complaint
CreateIssue.of(" ") // Left(title, must not be blank)
goodIssue.copy(title = "") // constructs, no complaint
That second `AddTrackedTime` is not merely unvalidated, it is silently
wrong on the wire: `AddTimeOptionDto` renders `command.spent.toSeconds`,
and its own comment explains that truncation is safe *because*
`AddTrackedTime` refuses a sub-second duration. It did not. 1500ms was
sent as 1 second.
The site's front page promises "Illegal requests are unrepresentable".
For these thirteen types that was not true, and every Scaladoc saying a
field is "validated by [[X.of]]" was describing one of several ways to
build the value rather than the only one.
The constructors are now `private[codeberg4s]`, which is the modifier all
131 response models already carry, so there is one convention across
model types rather than two. The generated `apply` and `copy` become
inaccessible outside the library while `of` and the in-class `with…`
builders — which call `copy` from inside the class — are untouched.
Nothing in the library constructs these types, so nothing internal
changes; `modules/codec` only reads their fields.
The types: AddTrackedTime, CreateComment, CreateIssue, CreateMilestone,
EditComment, CreatePullRequest, BranchProtectionSettings,
CreateDeployKey, DeployKeyQuery, CreateWikiPage, EditWikiPage,
ActivityFeedQuery, TrackedTimeWindow.
BREAKING CHANGE: the public `apply` and `copy` of the thirteen command
and query types listed above are no longer accessible outside the
library. Build them with the companion's `of`, which returns
`Either[ValidationError, A]`, and adjust them with the `with…` builders
the types already provide. Code that used the generated constructor was
skipping validation the Scaladoc promised, and in the `AddTrackedTime`
case was sending a truncated duration.
…ation `AttemptParitySuite` promises in its own Scaladoc that "the convenience rail and its hand-written `Attempt` mirror cannot disagree". Every check it makes is structural — same method name, same erased parameters, the return type with `Either[CodebergError, _]` spliced in. Nothing it does invokes a mirror, so the one line inside each mirror is unchecked. A mirror that delegates to the wrong rail method passes all of it, provided the target shares the parameter list and element type. Several pairs do: `IssueSubscriptionApi.Attempt.subscribe` and `.unsubscribe` both read `(Owner, RepoName, IssueNumber, Owner)` returning `Future[Either[CodebergError, Unit]]`, and the same shape recurs across the follow/unfollow and star/unstar families. Measured rather than argued. With `unsubscribe` edited to delegate to `rail.subscribe`, `AttemptParitySuite` passed all six of its tests and the full 3604-test suite stayed green — a caller asking to unsubscribe would have subscribed. This check fails, naming the file and the line. It lives in verify.sh step 6 beside the other source-level invariants because that is what this is: the mirrors are hand-written by design, so the property is a property of the source. The alternative — invoking all 437 mirrors reflectively and comparing the operation id each reports — needs a synthesised argument for every parameter type in the library, which is a larger and more brittle mechanism than the bug it catches. The check fails loudly if it matches no mirrors at all, so the pattern going stale cannot turn it into a silent pass.
`GET /user/list_blocked` and `GET /orgs/{org}/list_blocked` return the
same Forgejo `BlockedUser` — two properties, `block_id` and
`created_at`. The library modelled it twice: two `BlockedUser` case
classes, two `opaque type BlockId = Long` whose companions both read
`PositiveId.from("blockId", value)`, two DTOs differing only in whether
the field was called `createdAt` or `created`, and two test suites
asserting the same conversions.
docs/LEDGER.md already forbids this in its own words — "the first wave
that needs a shared model owns it. Later waves import it and must not
redefine, fork, or 'temporarily' copy it. A duplicated model is a
review-blocking defect — and it is what PMD CPD catches." It was not
caught, because the duplication gate was at the time mostly counting
identical import blocks rather than identical models.
`users.social` keeps the pair, being wave 1; `organizations` imports it.
The organizations copies of the model, the DTO and their tests are
deleted.
No coverage is lost. The organizations suite tested three cases the
social suite did not — the Go zero-time sentinel decoding as absence,
the "at least 1" wording on a non-positive `block_id`, and an empty
listing decoding as an empty vector — and those three moved into
`SocialDtoSuite` rather than leaving with the file.
BREAKING CHANGE: `com.worxbend.codeberg4s.organizations.BlockedUser` and
`com.worxbend.codeberg4s.organizations.BlockId` are gone. Import
`com.worxbend.codeberg4s.users.social.BlockedUser` and `BlockId`
instead; the type is identical, so no other code changes.
`OrganizationApi.blockedUsers` keeps its name, its signature shape and
its operation id.
Both land before the tag freezes the surface, so they belong in the same "Changed" section as the renames: the thirteen command types whose constructors became `private[codeberg4s]`, and the removal of the duplicated `organizations.BlockedUser` and `BlockId`. The `AddTrackedTime` case is spelled out rather than summarised, because a caller who used the public constructor was not merely skipping a check — they were sending a silently truncated duration.
Collapsing the duplicated `BlockedUser` model removed five groups, and the gate's own rule is to bank a reduction rather than leave headroom that a future copy-paste could grow into.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
What this does
Closes the open findings left after the pre-release merge, then acts on a
multi-agent review of the codebase against the clean-code, clean-architecture,
refactoring, DDD, PEAA and Scala-3 disciplines. Fifteen commits, each one
buildable and independently reviewable.
Two of them fix defects that were shipping wrong behaviour. The rest are
maintainability work.
Why
The bug: validation was optional on thirteen types
Thirteen command and query types paired a validating
ofwith a publiccase-class constructor, so the validation could be walked around. Demonstrated
against the built library, from outside it:
The
AddTrackedTimecase is not merely unvalidated.AddTimeOptionDtorenderscommand.spent.toSeconds, and its own comment says that truncation is safebecause
AddTrackedTimerefuses a sub-second duration. It did not. 1500mswent out as 1 second.
The site's front page promises "Illegal requests are unrepresentable". For these
thirteen types it was not true.
The bug: the two-rail parity test could not see the likeliest mistake
AttemptParitySuitepromises the two rails "cannot disagree", and checks names,erased parameters and return types. It never invokes a mirror, so the one line
inside each is unchecked. Mirrors with matching signatures are common —
subscribe/unsubscribe,follow/unfollow,star/unstar.Measured: with
Attempt.unsubscribeedited to callrail.subscribe,AttemptParitySuitepassed all six tests and the full 3604-test suite stayedgreen. A caller asking to unsubscribe would have subscribed.
The gate that had stopped measuring what it claimed
verify.sh --with-slowwas failing: 408 duplication groups against a baseline of363. But 324 of those 408 contained no code beyond a package clause and imports.
.scalafix.confsetgroupedImports = Explode, and promoting the shared codechelpers gave 108 files the same seven-line preamble, which PMD counts as
duplication.
Duplication that contains real code had fallen, 222 groups to 183, measured at
both commits with the same tool. The gate was reporting the opposite of what
happened.
How it works
Defects
c708519— the thirteen constructors becomeprivate[codeberg4s], themodifier all 131 response models already carry.
ofand the in-classwith…builders are untouched. Nothing in the library constructs these types.
fdf3a2f— a check in verify.sh step 6 asserting everyAttemptmirrordelegates to the rail operation of the same name. 437 mirrors, all currently
correct. It fails loudly if it matches no mirrors, so a stale pattern cannot
become a silent pass.
The duplication gate
57c468b/9c878e6—groupedImports = Merge, and the mechanical rewrite.trailingCommasmoves tokeepbecause scalafix and scalafmt otherwiserewrite each other's imports and
--checkfails whichever order they run in.760fd3d/9efb325— the baseline re-recorded at 161, then 156, with acomment saying explicitly that the number is not comparable to the older ones
and why.
Architecture and models
e431680—modules/transportdeclared a dependency oncodecit neverused. ADR-0003 states the transport knows no JSON as a fact; with the edge in
place that was enforced only by a grep that a fully-qualified call walks past.
It also put jsoniter on the published POM of an artifact that never calls it.
635c0c4—BlockedUser/BlockIdwere declared twice, inorganizationsand
users.social, with byte-identical bodies. docs/LEDGER.md already calls aduplicated model "a review-blocking defect".
Documentation that had gone wrong
70dd55f— seven places saidOwner("forgejo")"does not compile". Itcompiles now. The troubleshooting table told readers to repair working code.
86816ad— five documents saidCodebergErrorhas five cases. It has six.The errors guide promised "there is no sixth, and the compiler will tell you
if you forget one in a match" — backwards for anyone writing an exhaustive
match.
3c86482—scripts/site.shresolves the JDK from.mill-jvm-versioninstead of inheriting whatever is on PATH, so a contributor's first local
build stops failing with an
UnsupportedClassVersionErrorthat names neitherthe script nor the pin.
b7f66c0,ba92eff,2e59f42,1cf4472— stale Scaladoc links, changelogentries for the renames, and the landing-page teasers that restated the prose
directly beneath them.
How to test it
Both green. 3596 tests (down from 3604: eleven duplicated
BlockedUsertestsremoved, three unique ones moved rather than dropped), zero warnings under
-Werror, coverage above its floors, duplication 156 against a baseline of 156,CRAP worst 28.0 against a limit of 30.
To see the parity check bite, edit any
exec.attempt(rail.X(…))to name adifferent operation and re-run
./verify.sh; it fails at step 6 with the fileand line.
Notes for reviewers
trusted: for all 307 changed Scala files, the set of fully-qualified names each
imports is byte-identical before and after — including any
given, whose losswould change implicit resolution silently rather than failing the build.
commits: measured before and after, the uncovered-statement set is the same 14
either way. The earlier figure was inflated by scoverage data accumulated
across runs.
repositories.admin.CreateRepositoryandusers.account.CreateRepositoryare the same twelve-field command for the same endpoint, but carry different
builder vocabularies (
asPrivatevskeptPrivate,usingvsusingObjectFormat). Collapsing them means choosing which public vocabularysurvives.
QuotaInfomodels disagree:organizationskeeps the wire nesting,users.accountflattens it and documents the flattening as deliberate. Oneof the two projections has to win.
PageWalkimplements the pagination policy overFuturein the clientmodule, where ADR-0005 and PLAN.md §135 both place it in
coreoverExec[F]. Moving it would let the deletedPaginationPropscome back.RepositoryAdminApi(1411 lines, 44 operations) splits fromRepositoryApiby verb rather than by subject.
mutation step because Stryker4s is not declared in
build.mill; verify.sh'sown header says so.
--with-slownow passes, so switching the schedule to itis newly viable — but reversing the "it stays red until the plugin is wired"
stance written in
nightly.ymlis your call, not a CI-hygiene fix.