Add the CI pipeline (Vikunja task #5) #10
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/ci-pipeline"
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 Vikunja task #5. Replaces the temporary runner probe with a real pipeline.
This PR is itself the test. Everything below was verified locally; the runner-side behaviour could not be, because pushing is the only way to trigger it. The checks list on this PR is the first real evidence.
The pipeline
One job on
ubuntu-24.04:restore→build --no-restore -c Release→test --no-build→format --verify-no-changes.Constraints encoded, each learned the expensive way earlier
ubuntu-24.04, notubuntu-latest. No runner here advertisesubuntu-latest; a workflow requesting it waits indefinitely with no error.global.jsonviasetup-dotnet'sglobal-json-file, single-sourcing the pin.--treat-no-tests-as-error. All four test projects are empty;dotnet testexits 0 while printing "No test is available".dotnet format --verify-no-changesincluded — see the correction below.verify-runner.ymldeleted.Two corrections to the brief I gave the agent
1. My brief omitted two of the ticket's four acceptance criteria. I wrote it from memory. The agent read the ticket itself and implemented both anyway: the README build badge and NuGet caching. My error; the ticket wins.
2. A UTF-8 BOM does not currently fail
dotnet format. I asserted it did. The agent tested: a BOM on a.csfile givesBUILD EXIT=0,FORMAT EXIT=0. TheCHARSETrule only exists once an.editorconfigdeclares a charset — so the protection arrived with #8, not before. Reproduced independently.Worse for my earlier claim: no
.csfile in the repo ever had a BOM. They were on the.csprojfiles, whichdotnet formatdoes not inspect at all — I verified this by injecting a BOM into a.csprojand getting exit 0. So the.csprojBOM normalisation from #8 is enforced by nothing. That needs a different mechanism and I will raise it separately.Decisions
One job. With a single runner, parallel jobs execute serially anyway while restoring and rebuilding per job — strictly slower for more YAML.
Static analysis is the build, since #8 makes analyser violations build errors. A separate analysis step would run the same analysers twice.
dotnet formatcovers what the build cannot see: layout and encoding.Release only. Debug is continuously exercised locally and #8 makes warnings errors there too; CI should cover what local development does not.
Caching included, after measuring. The agent intended to argue against it, then found a cold restore pulls 240 MB (82 MB mono browser-wasm runtime, 36 MB codecoverage, 34 MB devserver). It is also an explicit acceptance criterion. Taken from the Forgejo mirror (
code.forgejo.org/actions/cache) rather than GitHub, because the action talks to the runner's own cache server and the GitHub v4.2+ line moved to a protocol self-hosted implementations lag on. Markedcontinue-on-errorso a missing cache server yields a slow green run, not a red one.Fully qualified action URLs.
setup-dotnetis not mirrored oncode.forgejo.org(15 repos, checked), so a bareactions/setup-dotnet@v5would 404 against the default action registry.No
concurrencyblock. Forgejo cancels superseded runs by default forpushandpull_request; this instance is 16.0.1, well past the version that added it. An explicit block would restate the default.Verified locally, from zero build artefacts
Run in exactly that order from a clean copy, so
--no-restore/--no-buildare not hiding a stale-artefact assumption.Failure modes proven, not assumed: an invalid
.csgiveserror CS1519,BUILD EXIT=1; a deliberately failing test givesFailed: 1,TEST EXIT=1..slnxbuilds, tests and formats on SDK 10.0.110 — the criterion carried over from task #8, as far as it can be proven off-runner. Blazor WASM builds with no workloads installed, so the runner will not needdotnet workload restore.Branch protection — needs your action after this merges
The pipeline is advisory until it is a required check, so "build failures block the merge" is not yet true of anything in the repository.
Expected context:
ci.yml / build (pull_request). Copy it verbatim from this PR's checks list before saving the rule — a required pattern matching nothing blocks every merge permanently with no obvious cause. Forgejo accepts globs, soci.yml / *is the safer form.What to watch on this PR's first run
setup-dotnetresolves — the largest unknown. If it 404s, the runner cannot reachgithub.comand there is no mirror; fallback isdotnet-install.sh.@v4.global.jsonis not being honoured.nuget-,hashFilesis unsupported.main; that is expected, not broken.Verdict: changes needed
The pipeline is red. Run #115 on
9ff3076(https://git.robware.uk/rob/PlaceMark/actions/runs/29) finishedfailurein 7 seconds — created 20:37:23, started :24, stopped :31. This PR correctly nominated its own checks list as the acceptance evidence, and that evidence came back negative. Nothing else in the review changes that.What I could and could not establish about the failure
I could not read the step logs. No Forgejo MCP tool exposes job logs;
get_workflow_runreturns run-level metadata only. The repo is private, so the run page, the badge and the raw log endpoints all return 404 anonymously, and the Forgejo instance and runner are both remote (no containers on this host). Someone authenticated needs to open that run URL. I am stating this plainly rather than dressing up an inference as a diagnosis.What I did establish narrows it usefully:
verify-runner.ymlbeing deleted here contains zerouses:steps — onlyechoandunameviarun:. Every green run in the history (#96–#114, all 1–2s) exercisedrun:steps onubuntu-24.04and nothing else. Action resolution, download and execution have never once been exercised on this runner.github.com/actions/setup-dotnet@v5exists (v5.4.0; v6 is current),github.com/actions/checkout@v5exists,code.forgejo.org/actions/cache@v5exists (v6.1.0 is current).setup-dotnetgenuinely is not mirrored. Theactionsorg on the Forgejo mirror has 15 repos;checkoutandcacheare there,setup-dotnetis not. Alsodata.forgejo.org302-redirects tocode.forgejo.org, so the ci.yml comment and the commit message are describing the same host and are both correct.ActionRunsUsingNode24), so that is only a problem on an older runner.One inference worth recording, because it constrains the fix: a total egress blackout cannot be the explanation.
dotnet restoreneeds nuget.org for the 240 MB this PR measured, so if the runner had no outbound access the pipeline would be unworkable in principle, not merely red. The live question is whethergithub.comspecifically is reachable.The fix
checkoutto the mirror —https://code.forgejo.org/actions/checkout@v5. It is mirrored, this is the first step and therefore the most likely failure point, and it costs nothing. Sourcing it from github.com when a mirror exists is gratuitous risk.setup-dotnetis what failed, there is no mirror, so replace it withdotnet-install.shin arun:step —run:steps are the only mechanism ever proven to work on this runner.Status check context — could not confirm
The PR predicts
ci.yml / build (pull_request). I could not verify it: the commits API does not surface statuses and the checks list is not readable anonymously. Copy it verbatim from this PR's checks list. Use the glob formci.yml / *, and do not add it as a required check until a green run exists — a required pattern matching nothing makes the repo permanently unmergeable, which is the exact failure mode that is hardest to diagnose later.Local verification — all four commands pass, as claimed
Run in order from the PR head with no prior artefacts:
Nine projects restored and the
.slnxparsed — the criterion carried over from task #8. The--no-restore/--no-buildchaining is correct and will not break on a fresh runner; each step's prerequisite is produced by the one before it. I also confirmed baredotnet format --verify-no-changesresolves the.slnxfrom the repo root, so the README instruction is valid.Judgements on the design
code.forgejo.orghonours/{owner}/{repo}/badges/workflows/{file}/badge.svg?branch=mainand 303s to a shields.io "Not found" fallback for an unknown workflow, which is exactly the pre-first-run behaviour the PR predicts. Worth knowing that because the repo is private, the badge will render as a broken image for anyone not signed in.concurrencyblock — the claim checks out. Forgejo did historically not cancel superseded pull request runs (issue #2581), but that was fixed by PR #9434 in December 2025, well before 16.0.1. The justification is correct as written.Non-blocking
continue-on-erroron the cache is not yet proven to do what it claims. It protects against a missing cache server; whether it also survives a failure to fetch the cache action itself is untested. Confirm once the run is green — and check the key does not render as a barenuget-, which would meanhashFilesis unsupported.**/*.csprojand**/*.propsbut notglobal.jsonor the.slnx. Thenuget-restore-key makes this cosmetic.Acceptance criteria (from Vikunja task #5)
mainmainpath unprovenTwo of four are unproven for the same reason: the run is red. Get it green and three of them verify themselves.
Verdict: mergeable
Supersedes my review at
9ff3076. Run #123 onf8d148cissuccessin 52s, verified independently via the API rather than taken from the summary. Therun:-only rewrite is the right call and the pipeline now does in CI what it was always able to do locally.Status check context — the prediction was wrong, and it would have bricked the repo
The verbatim context is:
Read from
/api/v1/repos/rob/PlaceMark/commits/f8d148c.../statuses— three entries, allCI / build (pull_request), transitioning pending → pending → success.Not
ci.yml / build (pull_request). Forgejo builds the context from the workflow'sname:field, not the filename. This PR's description predicts the filename form and recommends the globci.yml / *— that pattern matches nothing, and saving it as a required check is precisely the permanent-merge-block failure mode the description warns about. UseCI / *(which also coversCI / build (push)), or the exact string above.One consequence worth knowing: the required check is coupled to
name: CIinsideci.yml. Renaming the workflow silently breaks branch protection, with the same signature. Worth a line in the header comment.Is the
run:-only rewrite an over-reaction?No — and there is no sixth experiment worth running in the workflow file. Five runs eliminated every variable reachable from YAML: node runtime version, action host, cache directory and
uses:syntax. What remains — the container runtime, the docker socket, the runner version — cannot be addressed from a workflow at all.The run durations independently corroborate the elimination log: #115 and #116 both died at 7s (manifest parse, before any download — consistent with node24 rejection), while #117, #119 and #120 ran 48s / 40s / 51s (actions cloned, then failed during execution). Two distinct failure signatures, exactly matching item 1 versus items 2/3/5. The log is telling the truth.
act_runner v6.4.0 is roughly five majors behind — the current line is
code.forgejo.org/forgejo/runner/v11. A defect this fundamental in a version that old is entirely plausible and not worth further archaeology. The genuine next experiment is on the runner host: upgrade it, or run the job without a container (host/LXC mode), which sidestepsdocker cpentirely. Please raise that as its own ticket — it recovers actions, the NuGet cache and the SDK cache in one move.The checkout script
Two things to harden, neither blocking:
git remote add originis not idempotent. If the workspace is ever reused between runs — act_runner does reuse/workspaceunder some configurations — this fails with "remote origin already exists" andset -eaborts the job. #123 is a single run in a fresh container, so this is untested. It is the most likely future breakage in the whole file. Cheap fix:git remote add origin "$URL" 2>/dev/null || git remote set-url origin "$URL".base64defeats the runner's log masking. The runner masks the literal secret value; the base64 of it is a different string and will not be masked. Nothing prints it today (-q, noset -x), but the first person who addsset -xto debug this step drops a trivially reversible credential into the log. Worth a warning comment beside theprintf.Things you got right that I want to record so they are not "tidied up" later:
git -c http.extraheader=rather thangit config --localis correct here.actions/checkoutwrites the header into.git/configand removes it in a post step; this workflow has no post step, so--localwould persist the credential in the workspace. Argv exposure inside a single-tenant ephemeral container is the lesser risk. Deliberate and right.--depth 1is safe against force-push. Each run does a freshgit initand fetches the ref by name, resolving to wherever it currently points, so there is no stale local state to conflict — subject only to point 1 above. Nothing in build, test or format reads history.PULL_REQUEST_NUMBERunderset -uis safe, because theenv:key is always defined (empty on push) and only read in thepull_requestbranch.pushpath has never executed.SOURCE_REF="$GITHUB_REF"is logically sound and will fail loudly rather than silently if it is wrong, but merging this PR is its first run. Watch the post-merge run onmain.$GITHUB_PATH— right decision, wrong reasonThe stated justification is inaccurate and should be reworded, because it teaches the next person something false.
$GITHUB_PATHis a file the runner appends to and reads between steps; the failure here is materialising a node action into the container viadocker cp. Different mechanisms. The comment's own logic refutes itself: if runner-managed state propagation were broken, the job-levelenv: DOTNET_ROOTthis workflow does depend on would be broken too.The decision is still correct, for a better reason: an absolute
DOTNET_ROOTis explicit and greppable, andDOTNET_ROOTis the documented variable .NET itself uses to locate the runtime — it is doing real work, not standing in forPATH. Say that instead.NuGet caching and the README
The README states the cost honestly. It quantifies 240 MB of packages per run plus a full SDK install per run, says runs take "minutes where it should take seconds", and explicitly frames it as unreachable rather than "a shortcut taken for simplicity". That is the right standard and it meets it.
But ticket criterion four is not met — unmeetable is not the same as met, and it should not be recorded only in a README paragraph. Please update Vikunja task #5 to record the deferral and link it to the runner-upgrade ticket. One option not considered, for completeness rather than as a request: a pull-through NuGet proxy configured via
NuGet.configwould restore package caching without fixing the runner. Almost certainly not worth it at hobby scale, but better as a rejected option than an unknown one.Header comment accuracy
Accurate on the substance; three amendments:
$GITHUB_PATHrationale, above./data/.cache/actdirectory") is the one claim I cannot corroborate from outside. I am neither endorsing nor disputing it.actions/setup-dotnet@v5would 404 against the default registry — was disproven by #120, where the bare form resolvedsetup-dotnetfromdata.forgejo.orgdespite its web listing not showing that repository. I could not confirm this independently (the host 403s me), but a successful clone is stronger evidence than my probing. The revert instruction already uses the bare form, so the file is self-consistent; recording the disproof stops someone reinstating URL pinning on a false premise.Local verification
Only
.forgejo/workflows/ci.ymlandREADME.mdchanged between9ff3076andf8d148c(diff --stat: 2 files). No source, project or.slnxfile was touched, so my verification at the previous head carries over unchanged: restore of 9 projects, Release build with 0 warnings and 0 errors, tests exit 0,format --verify-no-changesexit 0.Acceptance criteria (Vikunja task #5)
mainMerge it, then set branch protection using
CI / *— notci.yml / *.f8d148cbd2e4f1e84149Verdict: mergeable
Re-issued for
e4f1e84. You were right not to bend the rule; the check was cheap and it did confirm the load-bearing item against the new head.The rebase is clean
ci.ymlis byte-identical, not merely diff-clean: the blob hash is1d246d04c0e6f892de2ade9cf806cf48fb60eec9at bothf8d148cande4f1e84. Everything I established about the workflow carries over untouched.grepfor markers across all paths: none).main..e4f1e84is exactly three files —ci.ymladded,verify-runner.ymldeleted,README.md+41. No stray content picked up from the rebase.e4f1e84: success, 33s.README resolution — both sections survived, coherently
The heading outline is
main's outline with two headings inserted and nothing displaced:### Configurationfrom #9 is intact under## Getting started;## Continuous integrationsits at the top level after the secrets material and before## Contributing. Heading levels are consistent, ordering reads correctly, no duplication. The badge is still on line 3.Status check context, re-confirmed on
e4f1e84I read the statuses API myself rather than taking it from your summary. Three entries, all:
Unchanged from
f8d148c, as expected — it derives fromname: CI, which the rebase did not touch. UseCI / *.The three deferred items — none of them block, and here is the test I applied
The question for a merge gate is narrow: can this cause CI to report success when it should report failure, or lose a secret without anyone noticing? None of the three can.
git remote add originnon-idempotence — fails loudly on step one with an explicit "remote origin already exists" and a red run. It cannot produce a false green. Deferring is safe.base64versus log masking — requires someone to deliberately addset -xfirst. Worth putting in the follow-up ticket rather than leaving purely in review history, though, because the repository is now public: a job token in a public log is a worse outcome than it was when I first raised it. Still not blocking — the token is job-scoped and ephemeral — but that changed while this PR was open and is worth a line on the ticket. If the repository is meant to go back to private, that also closes the exposure.$GITHUB_PATHreasoning — prose in a comment. Zero runtime effect.Agreed on all three as follow-up.
One line for the follow-up ticket, while you are there
The README states "a failing run blocks the merge" as fact. It becomes true the moment you save the branch protection rule and not before — which is minutes away, so I am flagging it only so it does not sit un-actioned if the rule gets deferred. If you do defer it, soften the sentence.
Merge it, then set the required check to
CI / *.