Fedora RPM builds, flaky test fix, dependency advisories, and a docs correctness pass - #140
Open
samueltuyizere wants to merge 5 commits into
Open
Fedora RPM builds, flaky test fix, dependency advisories, and a docs correctness pass#140samueltuyizere wants to merge 5 commits into
samueltuyizere wants to merge 5 commits into
Conversation
…uilds Four pieces of maintenance and one new deliverable. Flaky test — TestWatchConfig_DetectsFileChange failed roughly 1 in 4 full-suite race runs and blocked pushes via the pre-push hook. Two races were involved: WatchConfig registers its fsnotify watch asynchronously, so the test's sleep-then-write could lose the write entirely; and AtomicConfig.Reload runs reload callbacks before publishing the new pointer, so asserting via Get() could read the old config. Adds WatchConfigWithReady, which signals once the watch is registered, letting the test wait for readiness, write once, and assert on the config handed to the callback. WatchConfig keeps its signature and delegates, so both production call sites are unchanged. OnReload now documents the callback-before-publish ordering. Dependency advisories — postcss 8.5.18 to 8.5.26 (GHSA-fxqj-rqcc-2cmp, Dependabot alert #2) and nanoid to 3.3.18 (GHSA-2v37-7h3g-55p8, high severity, never alerted on). Both were in range of the existing tailwindcss constraint, so no breaking upgrade. npm audit is clean and the checked-in compiled-tailwind.css is byte-identical after a rebuild. Adds .github/dependabot.yml covering gomod, npm and github-actions, since the unreported nanoid advisory showed the gap. Fedora packaging — builds x86_64 and aarch64 RPMs with nfpm (pinned v2.47.0) and attaches them to both release channels. The RPMs are produced inside the existing release job and passed to the single gh release create call: this repo publishes immutable releases, so assets added afterwards would be rejected. A verification step reads the ELF header directly (magic, class, e_machine) rather than grepping file output, which words things differently on macOS and would silently always pass. Package layout follows Fedora conventions: binary in /usr/bin, config in /etc marked noreplace so upgrades keep local edits, license in /usr/share/licenses, and an opt-in systemd *user* unit in /usr/lib/systemd/user — a system unit would wrongly imply a machine-level daemon for a loopback developer proxy. Adds a make rpm target for local inspection, since rpm -qip is unavailable on the macOS runner. Docs — corrections found by auditing every doc against the source: the long-context threshold is 100K not 80K, the vision tier was missing from the documented scenario priority, there are four endpoint types not two, the Anthropic endpoint covers Qwen as well as MiniMax, model tables now match modelMetadata, the scenario defaults now match the shipped default_config.json, OpenRouter's API-key env vars were undocumented, and config.example.json gained the six real config blocks it was missing. Removes a documented request_dedup option that does not exist in the code and documents the three analytics routes that do. Fixes stale beta-version formats and broken links, and flags the zh translations that lag.
Follow-ups on the four decisions taken after the previous commit. Routing reasons now read the resolved model from config. The Reason strings in scenarios.go hardcoded model names that had drifted (GLM-5.1, Kimi K2.6) because ScenarioResult.Reason had zero production consumers — nothing ever displayed them, so nobody noticed. DetectScenario now explains only why a scenario matched, and ModelRouter appends the model at the point it is actually resolved, covering scenario, cost-based catalog selection, respect_requested_model and both override paths. The routing log line now carries it, and a test fails if a detector reason ever names a model again. Also fixes ScenarioPolicy.Evaluate, which was formatting the scenario name twice as a placeholder instead of using the reason. golangci-lint is now configured rather than implied. Adds a .golangci.yml in v2 schema that passes with zero issues on the current tree: the standard set plus bodyclose, copyloopvar, misspell, nolintlint, whitespace, and revive with an explicit 23-rule list. gosec, errorlint and gocritic were evaluated and rejected — their findings are either inherent to a local CLI or need code changes, and suppressing them wholesale would make the linter meaningless. Adds make lint-strict, pins the CI lint action by SHA, and the committed pre-push hook now picks the config up with no change. make lint stays as the fast gofmt+vet check. RPM packaging moves to a dedicated Linux job so CI can actually inspect the packages. rpm runs on ubuntu-latest before release and hands over the rpm-packages artifact, which still enters the single atomic gh release create — immutable releases forbid adding assets afterwards. Verification asserts name, version, architecture, license, every payload path, the noreplace flag on the config, and that the packaged binary is an executable ELF of the right machine type; it collects all failures before exiting. The same scripts run in ci.yml on every PR, so packaging breaks at review time rather than at release time. Documentation. docs/models.md was a second, thinner copy of MODELS.md introduced by #116; it is now a pointer, because the duplication had already caused real drift — an earlier pass corrected the capability numbers in one file and left the same errors in the other. Those errors are fixed: MiniMax M2.5 is 200K not 1M (so long-context advice now points at MiniMax M3, which is what the shipped config uses), all four Qwen3.x models are 1M not 128K and are vision-capable, Kimi vision was undocumented, gpt-5.5-mini/nano were missing, and a documented fallback chain routed long context through a 200K model. The Chinese translations of CONFIGURATION, MODELS and INSTALLATION are brought up to date with their English originals, and English TROUBLESHOOTING gains the sections that only existed in Chinese.
…I installs The Kimi K2.6 row in the cost comparison table said ~1,150 requests per $12 while its own profile, the decision tree and the quick reference all said 1,850. Nothing in the repo can settle it — the model catalog carries no rate data for any of its 353 models — but the row duplicated GLM-5's figure exactly, which reads as a copy-paste, and 1,850 is what the other three places and the model's value rating agree on. Both files now say 1,850, and the table gained a note that these figures are indicative estimates rather than a machine-readable price list, so the next reader knows not to budget against them. INSTALLATION.md documented Homebrew, Scoop, source, raw binaries and Docker but never mentioned the RPMs, even though it is the canonical install doc that README points at. It now covers dnf installation, the noreplace config template, the opt-in systemd user unit, the missing GPG signature, and the fact that `routatic-proxy update` is for standalone binaries rather than RPM installs. The macOS DMG was documented only in Chinese. Ported to English, describing what the menu bar item actually offers per internal/tray/tray_darwin.go — status, Open Console, start/stop, and Start on Boot — with the CLI equivalents. The Chinese install guide gained the RPM section in turn, so the two stay in sync.
The RPM Packaging job failed on ubuntu-latest with "cpio: cannot make directory '/etc/routatic-proxy': Permission denied". RPM payload members are absolute paths, and whether cpio strips the leading "/" by default differs between distributions — Fedora's does, Ubuntu's does not, so the extraction step aimed at the runner's real /usr and /etc instead of the temp dir. Passing --no-absolute-filenames makes it explicit rather than relying on the local default. This is why the job exists: the same script passed on a Fedora workstation and only failed on a real Linux CI runner.
Contributor
Code Review Roast 🔥Verdict: No Issues Found | Recommendation: Merge Oh wait, this PR is actually clean. I need to sit down. I had my flamethrower warmed up and everything. 📊 Overall: Like finding a unicorn in production — I didn't think clean PRs existed anymore, but here we are. Files Reviewed (9 files)
Previous Review Summary (commit 82b2e1d)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 82b2e1d)Verdict: No Issues Found | Recommendation: Merge Oh wait, this PR is actually clean. I need to sit down. I had my flamethrower warmed up and everything. 📊 Overall: Like finding a unicorn in production — I didn't think clean PRs existed anymore, but here we are. Files Reviewed (9 files)
Reviewed by free · Input: 184.5K · Output: 13.8K · Cached: 141.5K Review guidance: REVIEW.md from base branch |
Ubuntu's rpm2cpio exits 1 even when it has written the payload in full, while Fedora's exits 0. Under `set -o pipefail` that sank the whole extraction pipeline, so the RPM job failed on ubuntu-latest immediately after the previous fix let cpio run at all. The status we care about is cpio's, so the pipeline runs with pipefail off and its subshell status is captured instead. Because that means neither exit code is fully trusted, the script now also compares the extracted binary's size against the size recorded in the RPM header — a mid-stream extraction failure would otherwise leave a truncated binary whose ELF header still looks valid. Reproduced and fixed against ubuntu:24.04 in a container rather than guessing: both arches now pass on Ubuntu and Fedora, and the wrong-arch and wrong-version negative controls still exit 1.
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.
Four pieces of maintenance plus one new deliverable. Three commits, each self-contained.
Fix the flaky watcher test
TestWatchConfig_DetectsFileChangefailed roughly 1 in 4 full-suite race runs and blocked pushes through the pre-push hook. Two races were involved:WatchConfigregisters its fsnotify watch asynchronously, so the test'ssleep(200ms)-then-write could lose the write entirely.AtomicConfig.Reloadruns reload callbacks before publishing the new pointer, so asserting viaGet()could read the old config. This is the one that actually reproduced.Adds
WatchConfigWithReady, which signals once the watch is registered. The test now waits for readiness, writes once, and asserts on the config handed to the callback.WatchConfigkeeps its signature and delegates, so both production call sites are untouched.OnReloadnow documents the callback-before-publish ordering.An earlier attempt used a write-retry loop; that was only less flaky, since coalesced events can still restart the 500ms debounce. The readiness signal removes the race instead of narrowing it.
Verified: 30x on the test, 3x full-suite
-race, all clean. Baseline was 1 failure in 4 runs.Patch dependency advisories
Both were within the existing tailwindcss constraint, so no breaking upgrade.
npm auditis clean and the checked-incompiled-tailwind.cssis byte-identical after a rebuild.Adds
.github/dependabot.ymlcovering gomod, npm and github-actions — the unreported nanoid advisory is exactly the gap it closes.Fedora RPM builds
x86_64 and aarch64 RPMs on both release channels, built with pinned nfpm.
rpmjob onubuntu-latest, then handed toreleaseas an artifact. They still enter the single atomicgh release create, because this repo publishes immutable releases and rejects assets added afterwards.noreplaceflag on the config, and that the packaged binary is an executable ELF of the right machine type. It collects all failures before exiting.ci.ymlon every PR, so packaging breaks at review time rather than at release time./usr/bin, config in/etcmarkednoreplaceso upgrades keep local edits, license in/usr/share/licenses, and an opt-in systemd user unit in/usr/lib/systemd/user. A system unit would wrongly imply a machine-level daemon for a loopback developer proxy.make rpmbuilds locally.Not included, deliberately: GPG signing, COPR, EPEL submission, SELinux policy. The docs say the RPMs are unsigned and point at
checksums.txt.Configure golangci-lint
It was invoked by the committed pre-push hook but had no config and never ran in CI. Adds a
.golangci.yml(v2 schema) that passes with zero issues: the standard set plus bodyclose, copyloopvar, misspell, nolintlint, whitespace, and revive with an explicit 23-rule list. gosec (25 findings), errorlint and gocritic were evaluated and rejected — their findings are either inherent to a local CLI or need code changes, and blanket-suppressing them would make the linter meaningless. Addsmake lint-strictand a SHA-pinned CI lint action;make lintstays the fast gofmt+vet check.Routing reasons read from config
The
Reasonstrings inscenarios.gohardcoded model names that had drifted (GLM-5.1, Kimi K2.6). Root cause:ScenarioResult.Reasonhad zero production consumers, so nothing ever displayed the drift.DetectScenarionow explains only why a scenario matched;ModelRouterappends the resolved model where resolution actually happens — covering scenario, cost-based catalog selection,respect_requested_modeland both override paths. The routing log line carries it, and a test fails if a detector reason ever names a model again. Also fixesScenarioPolicy.Evaluate, which was formatting the scenario name twice as a placeholder instead of using the reason.Documentation correctness pass
Every doc was audited against the source. The errors were real, not cosmetic:
gpt-5.5-mini/nanowere missingconfig.example.jsonwas missing six real config blocks, including all of OpenRouterrequest_dedupoption does not exist in the code (removed); three analytics routes do (documented)INSTALLATION.mdnever mentioned the RPMs, and the macOS DMG was documented only in Chinesedocs/models.mdwas a second, thinner copy ofMODELS.mdadded by #116. It is now a pointer, because the duplication had already caused drift: an earlier pass corrected the capability numbers in one file and left the same errors in the other.Chinese translations of CONFIGURATION, MODELS and INSTALLATION are brought up to date with their English originals; English TROUBLESHOOTING gained the sections that existed only in Chinese.
Verification
go test ./... -raceclean (repeated),go vetclean,golangci-lint0 issues,gofmtcleannpm auditcleanAGPL-3.0-onlymatching LICENSE, FHS layout,cn(config|noreplace) on the config, and a static ELF that runs and reports its versionKnown limitations
rpmandreleasejobs. The PR-timeci.ymljob exercises the build and verification, but not the cross-job transfer.docs/zh/TROUBLESHOOTING.mdis still ahead of English in places beyond what was ported; the remaining zh drift is noted rather than silently reconciled.