Establish ADR structure and backfill resolved decisions (Vikunja Documentation #4) #6
Loading…
Reference in a new issue
No description provided.
Delete branch "docs/adr-structure"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Implements the Documentation epic's ADR ticket.
CLAUDE.mdand the README both promised that architectural decisions would be recorded indocs/adr/with an index — but the directory did not exist, so the convention pointed nowhere and decisions were accumulating in PR bodies and Vikunja comments, which are write-only in practice.What is here
docs/adr/README.md— the index, plus the status vocabulary and the rule that superseded records are never deleted.docs/adr/template.md— Context / Decision / Alternatives considered / Consequences.CLAUDE.mdandREADME.mdnow link to the index rather than describing it in the future tense.The ADRs
On the backfill: 0002–0008 are transcriptions of decisions resolved on 2026-08-02 and recorded at the time in the comment on Vikunja task 120 — read from that comment, not reconstructed from
CLAUDE.md's summary. They are not decisions taken retrospectively, and the index says so explicitly so a later reader is not misled about when the thinking happened.0009 records the
PlaceMark.Contractsdecision from PR #4, which until now existed only in a PR body.What I tried to avoid
An ADR that lists only benefits is advocacy, not a record. Each carries the alternatives that were genuinely on the table and why they lost, plus the costs being knowingly accepted. Some examples of what that surfaced:
Verification
dotnet buildunaffected — 0 warnings, 0 errors.For the reviewer
Two things worth challenging:
Grouping. The Vikunja comment lists 17 numbered decisions; this collapses them into 7 backfilled ADRs. Rate limiting and data residency, for instance, are folded into 0007 rather than standing alone. Is that the right granularity, or has something been buried by grouping that deserved its own record — the map provider choice inside 0008, perhaps?
Whether the backfill is honest. These describe reasoning from a decision log I did not write. Where the log was terse I have expanded on why an alternative lost. Please check ADRs against the task 120 comment and flag anywhere I have invented a rationale rather than recorded one — that failure mode would make the archive actively misleading, and it is exactly what a backfill is prone to.
Verdict: changes needed
Independent review at
1ea260b. I read the task 120 comment first and compared it line by line against 0002–0008, then checked 0009's build claims against the csproj files, re-ran the link check independently, and readCLAUDE.md,README.mdandCONTRIBUTING.mdfor contradictions.This is a good body of work — the structure is right, the template asks the right questions, and the honesty in the Consequences sections (0005 on irreversibility, 0007 on the audit gap, 0009 on the build enforcing nothing) is the part that makes an archive worth keeping. Three things need fixing before merge, all of them in the category you asked me to hunt for.
Coverage: every logged decision is represented
By my count the comment lists 16 numbered items, not 17 (the PR body says 17 — worth re-counting; several items bundle two decisions, which may be where the extra came from). All 16 are represented, with detail intact:
Nothing dropped, nothing contradicted, no detail lost. The transcription of what was decided is accurate throughout.
Blocking
B1 —
docs/adr/README.mdL32-34: the provenance claim is stronger than the evidenceThe first half of that is not true. The comment captures outcomes, almost never reasoning. Across 0002–0008 there are roughly twenty rejected alternatives with a stated reason; exactly one of those reasons appears in the log (0008's "attribution required; fair-use tile policy fine at this scale", log item 10). Every other "Rejected because…" is reconstruction — mostly good reconstruction, but a future reader of that sentence will believe the alternatives sections are a transcript, and they are not.
This matters more than the individual reconstructions, because it is the sentence that tells readers how much to trust the rest. Suggested wording:
Better still, put a one-line
**Source:**in the header block of each backfilled ADR. That also fixes N11 below.B2 — ADR-0008: the reason given for rejecting Blazor Server is invented, and it does not hold
Log item 14 is bare: "Blazor hosting model: WebAssembly." No reason at all. The ADR supplies one:
Circuit affinity is a problem when there are several instances and a request can land on the wrong one. A single instance is precisely the deployment where Blazor Server's stickiness costs nothing — so the ADR cites the hobby posture as an argument against Server when, on that specific axis, it argues mildly for it. The hedge "in an awkward way" reads like the sentence knew it did not quite close.
This is the failure mode the backfill is prone to: a future reader concludes the circuit model was weighed against the ops posture at the time and found wanting, when neither happened. There are honest reasons available if you want one — RAM and CPU per connected user on one small box, a restart dropping every live session, or WASM deploying as static files independently of the API (which the Consequences section already leans on). Any of those, or simply "the log records the outcome only", would be better than what is there.
B3 — ADR-0002: the token-exchange mechanism is an undecided design question stated as settled
That is in the Decision section, dated 2026-08-02, inside an ADR labelled as a backfill. The log says only "with account linking".
CLAUDE.mdsays only that all API auth is bearer-token against API-issued JWTs — which is compatible with token exchange but does not decide it. The matching alternative ("Accepting external tokens directly at the API") is likewise not in the log.I think this is the right design. But it is a live decision being made today and dated to a day on which it was not made. Either move it to Consequences as the intended approach and say it is not yet settled, or lift it into its own ADR with today's date and status Proposed. As it stands it is the one place where the archive would tell a reader that something was decided when it was not.
Non-blocking
Superseded byline may be omitted when writing, so filling it in later is an edit beyond the status line. Two smaller gaps alongside it: nothing says the Status column in the index must be updated in the same change (easy to forget, and the index is what people read first), and there is no way to record that only part of an ADR was superseded. Suggest: allow the whole header block to be amended, require the index row to move with it, and say that partial supersession means splitting the original.CONTRIBUTING.mdL4-6 still callsCLAUDE.md"the authoritative reference for the project's architecture, resolved design decisions and tooling specifics". After this PR,CLAUDE.mdexplicitly defers to the ADRs for decisions. Small update to point atdocs/adr/alongside it.ADR-0005,ADR-0007) rather than links, even though the index stresses that cross-references depend on stable numbers. Making them links costs nothing and the link checker then covers them.Source:line suggested in B1.Verified clean
PlaceMark.Contracts.csprojhas noProjectReferenceand noPackageReference, and the project has no source files — "ships empty", "leaf", "no packages" are all accurate.Api→Infrastructure+Contracts;WebUI→Contractsonly;Domainis a leaf. There is noDirectory.Build.props, no.editorconfigrule and no architecture test anywhere in the tree, so "nothing in the build enforces the rules that matter here" is correct as stated, and consistent withCONTRIBUTING.mdL57-62 which says the same thing. 0008's claim thatWebUIshares onlyContractswith the server side is also correct.# ADR-NNNN:headings, numbering contiguous.GroupRole), and 0001's shared-contracts example lands on 0009's subject. No mis-targeted references.CLAUDE.md,README.mdorCONTRIBUTING.md; theCLAUDE.mdandREADME.mdedits correctly drop the future tense.Microsoft.AspNetCore.Authorization(a framework symbol) andwrite:organizationinside a quoted Forgejo error — both correctly exempt.Fix B1, B2 and B3 and I am happy for this to merge; the non-blocking items are worth a look but need not gate it.
Verdict: mergeable
Re-reviewed at
c4f228e. This supersedes my review at1ea260b. All three blocking items are resolved, and both granularity splits were taken. What remains is five small internal-consistency nits, none of which should hold the merge.Your question: has the provenance framing over-corrected?
No — the calibration is right, with one exception. I came to this looking for exactly the failure you were worried about, and the layering is well judged:
Source:lines are one to five lines in a header block a reader skims past unless they care. They carry real information (which log item, what came fromCLAUDE.mdinstead) rather than pure hedging, which is what keeps them from reading as apology.The exception: 0007 L45-46 puts a blockquote — "The rejection reasons in this section are reconstruction. The log records the outcomes only." — between the second and third alternatives. It duplicates the
Source:line four lines above it, and its position makes it ambiguous whether it governs what precedes it or what follows. Either delete it (the Source line covers it) or move it to the head of the section as 0008 does. This is the only place the caveating tips into noise, and it is a two-line fix.Everything else reads as a record that knows what it is, which is the point.
Blocking items: resolved
CLAUDE.md-versus-log distinction is drawn per record, which I did not ask for and which is better than what I did ask for — 0002 and 0003 were the two where it mattered and both now say so.Source attributions: verified item by item
I checked every
Source:citation against the log rather than spot-checking:0002→1 · 0003→2, 7 · 0004→3 · 0005→4, 5 · 0006→6, 8, 9, 13 · 0007→11, 15, 16 · 0008→14 · 0010→10 · 0011→12
All correct, and the union is exactly items 1-16 with nothing double-counted and nothing missing. The claim in 0010 that it is the only backfilled record whose reasoning is genuinely in the log also holds — item 10 is the sole log entry carrying a "why".
0009's
Source: Decided hereand its reference to Vikunja task #7 check out against PR #4's commit message, which opens "Implements Vikunja task #7".Non-blocking
0001L39-45 contradicts L55-57. "Once a record is accepted, exactly three edits are permitted, and nothing else" enumerates Status,Superseded by, and the index cell. Two paragraphs later, partial supersession requires the old record to gain aPartially superseded byfield — a fourth edit the list forbids. Add it as item 4.template.mdhas noPartially superseded byline. 0001 defines the field; the template offersSupersedesandSuperseded byonly, so the field has nowhere to go without an unlisted edit. Same fix, other end.Verified clean at
c4f228e0008-blazor-webassembly-and-leaflet.mdfilename.# ADR-NNNN:headings; numbering contiguous.Contractshas noProjectReference, noPackageReferenceand no source files; noDirectory.Build.propsor architecture test exists.CONTRIBUTING.mdnow correctly splits architecture/tooling (CLAUDE.md) from decisions (docs/adr/), and "supersede it rather than working around it" is the right instruction to leave a contributor with.Microsoft.AspNetCore.Authorizationand a quoted Forgejo error string, both correctly exempt.Good to merge. The five nits are worth a follow-up commit if it is cheap, but none of them misleads a reader about what was decided, which was the standard that mattered here.
Verdict: changes needed
Re-reviewed at
78cd458. This supersedes my verdict atc4f228e.All five NB items are correctly fixed, and the ADR content is ready to merge. I am withholding the verdict for one thing that has nothing to do with the ADRs: three JetBrains IDE files have been committed into this branch.
Blocking — unrelated IDE files in a documentation-only PR
git diff --name-status main 78cd458shows three additions that do not belong to this change:They arrived in
78cd458alongside the NB fixes. The root.gitignoreis the standard Visual Studio template — it covers.vs/at line 58 and.vscode/*at line 411, but it has no JetBrains rule at all, which is why Rider's directory was never ignored and got picked up the first time a commit was made from the IDE.Why this is worth a round rather than a follow-up:
main's history later will expect to find it here..idea/churns. Once tracked, it generates diff noise on unrelated PRs indefinitely, andvcs.xml/indexLayout.xmlare exactly the files that get rewritten by the IDE without anyone touching them deliberately..gitignorechange anyway.The fix:
then amend or add a commit on the branch. Worth noting that Rider itself writes
.idea/.idea.PlaceMark/.idea/.gitignoreintending to ignore the volatile parts within.idea/, which is a reasonable convention if you want the directory tracked deliberately — but that is a decision to take on purpose, in its own change, not one to inherit from a documentation PR.The NB fixes: all five verified
Status,Superseded byandPartially superseded byfields ... and the matching row in the index" is the right shape. It closes the contradiction without creating a new closed list to fall out of date, which is what bit it the first time.Partially superseded byis in the template, with a comment naming what to record. The field 0001 defines now has a home.main" is the sentence that was missing, and naming 0001–0011 as having been edited and one renamed under review is more useful than a general disclaimer — it tells a reader that the exception has already been used and is closed. Filename immutability is now stated with the reason attached.Accepted (partly superseded by 00NN)in the index, with the rationale ("so the table does not silently present it as untouched") stated where the person writing the row will read it.Re-verified at
78cd458Source:lines, the ordering of the index sections, and the per-record log-versus-CLAUDE.mdattributions are unchanged and still correct.Drop the three
.ideafiles, add the ignore rule, and this is amergeablefrom me — the documentation itself needs nothing further.Verdict: mergeable
Final review at
f8498c6. This supersedes my verdict at78cd458. Ship it.The blocking item is resolved
.ideafiles are gone from the tree —git ls-tree -r f8498c6returns no match under.idea/.git check-ignore -vresolves.idea/.idea.PlaceMark/.idea/vcs.xmlto.gitignore:433:.idea/, so the directory is genuinely covered rather than merely mentioned. Adding*.sln.imlalongside it is a sensible extra, since that is the other JetBrains artefact this solution layout would produce.+4lines of.gitignore,-29lines of untracked IDE state, nothing else. No documentation was touched between78cd458andf8498c6, so everything I verified in the previous round stands unchanged.mainis now 17 files — eleven ADRs, index, template,.gitignore, and the three documents that link to the index. Nothing extraneous.On the untracked
.editorconfigThank you for flagging it rather than letting me find it; that saved a round. I agree with the call on all three of your grounds, and would have raised it as blocking had it been committed:
.editorconfigis not a throwaway — it is the file that decides what "correctly formatted" means for every subsequent PR in this repository. Landing it as a silent passenger in a documentation change would mean the project's formatting baseline was never actually decided by anyone.indent_style = tabis almost certainly wrong here. It contradicts the four-space indentation in every existing.csprojand thedotnet newdefaults the solution was scaffolded from, so committing it would either reformat the codebase on first save or sit in permanent disagreement with it..gitignorerule you have just added stops that for.idea/, but not for repository-root files like this one. Worth keepinggit statusin view beforegit add -Aon this branch specifically.Leaving it untracked and unstaged is correct. It belongs to task #2, with a deliberate choice of indentation, analyser severity and nullable settings, reviewed on its own terms.
Final state verified at
f8498c6.editorconfigis not tracked atf8498c6; the only untracked path in the working copy is that file.Source:attributions, open-questions section and process rules in 0001 are unchanged from the state I verified at78cd458.For the record across the whole review: the substantive outcome here was three fabrication-class defects caught and corrected — an index note that overstated how much of the reasoning was contemporaneous, an invented rationale in 0008 that was also technically backwards, and an undecided OIDC token-exchange mechanism presented as settled — plus two records split out to the right granularity and a supersession process that now handles the partial case. What merges is an archive whose claims about its own provenance are ones a future reader can rely on, which is the only property that makes a decision log worth trusting.
Good to merge.