chore: back-merge main into develop - #4
Closed
github-actions[bot] wants to merge 98 commits into
Closed
Conversation
`ailoud setup` refused the sudo step, which is what it is supposed to do: with no terminal to answer a password prompt on it reports the exact command rather than hanging. A runner has no terminal, so the apt install is the workflow's job and only the rest -- the whisper release and the model files, neither needing sudo -- is left to setup. Also makes the back-merge workflow say so when there is no develop branch, instead of dying on `fatal: Not a valid object name origin/develop`. That is how its first run failed, and the copy it came from has the same weakness.
…ck-merge PR Two separate failures, both in what I added rather than in the product. The provisioned e2e job had every doctor check green and every transcribe spec exiting 3. The sandbox writes its own config naming only the MODEL and leaves `binary` at its default, so whisper-cli has to be on PATH -- while `setup` installs it under the data directory and records the absolute path in the user's config. Nothing was wrong with provisioning; the two configs simply disagreed about how the binary is found. The job now reads the installed paths back out of that config and adds their directories to PATH, read rather than spelled so bumping a pinned release cannot silently break it. The back-merge workflow could not open its PR: "GitHub Actions is not permitted to create or approve pull requests", a repository setting that is off by default. Enabled with default_workflow_permissions still `read`, so the token gains nothing beyond what this needs. Also adds the author (contact@lorem.dev) to all four manifests, the dev-tag skill and its spec, the branch and tag rules in AGENTS.md, and a publish-time check that every tarball carries the LICENSE and no source or tests -- pnpm copies the repository LICENSE into each workspace tarball, which is why no package holds its own copy, and an invariant worth relying on is worth checking. The `.agents/` skills were also missed by the rename and still said "laud".
The provisioned end-to-end job runs only on push, so requiring it on a branch would leave every pull request waiting for a check that never arrives.
Every transcribe spec on CI failed with "Exceeded timeout of 5000 ms" -- Jest's default, not the 600 s the config asks for. Jest takes the per-test timeout from the GLOBAL config. Splitting jest.config into `projects` moved `testTimeout` into each project, where it appears in `configs[].testTimeout` while `globalConfig.testTimeout` stays undefined, so every test fell back to 5 s. Confirmed both ways with `jest --showConfig`: projects 600000, global undefined; with the value also at the root, global reads 600000. Locally this was invisible. The transcribe specs were already failing fast for a missing model -- mine are still under the pre-rename path -- so nothing ran long enough to hit a 5 s limit. On CI, where setup had just downloaded them, whisper takes tens of seconds and every spec timed out. The bug was mine and the runner was the only place it could be seen. Separately, the build's own config files were in eslint's ignore list, so every editor reported "File ignored because of a matching ignore pattern" on opening one, and a mistake in them was caught by nothing -- including the mistake above. They are linted now, with Node globals and the recommended rules rather than the type-aware config the packages use. Verified by planting an unused variable in jest.config.cjs and watching eslint catch it.
Cutting 1.0.0 after 1.0.0-dev.1, -dev.2 and -rc.1 left four changelog sections describing one release. `fold-prereleases.mjs` merges them into one, keeping the subsection grouping and dropping duplicates -- including a duplicate whose copies were wrapped differently, which is the common case across two dev tags. Only the same version's pre-releases fold, so an entry from an abandoned line cannot reappear under a release it was never part of. `check-changelog.mjs` is a required step on every tag, before the gate and before anything is built: a version number can never be reused and the unpublish window is 72 hours, so every reason to refuse is worth finding while refusing is free. It checks the section exists and has entries, is inside the hard limit, that nothing is stranded under Development, and that a final tag folded its pre-releases. Problems are collected and reported together, because somebody fixing a changelog wants the list rather than one round trip each. The limits were copied into three scripts, so they now live once in scripts/lib/changelog.mjs alongside the parsing all three need. A limit that differs between the script that warns and the script that refuses is worse than no limit: one of them is wrong and nobody knows which. The soft limit is a warning rather than an error -- console.warn locally, a GitHub annotation under Actions. It read as a failure before. The scripts have tests now: 20 over the shared library, and 22 driving all three end to end. Two of them WRITE, so the tests copy scripts/ into a throwaway directory beside a fixture CHANGES.md, and each asserts it is under the temp directory before running anything. One test reads the repository's own changelog afterwards and fails if it changed -- if a script ever resolves the wrong root, that is how it will be caught. They run in ci.yml, which triggers on branches and pull requests but not on tags, so a broken script is caught before a release depends on it. Also lints scripts/ and the build's config files, which were both in eslint's ignore list: every editor reported "File ignored because of a matching ignore pattern" on opening one, and `node --check` was the only gate on scripts/ -- which sees syntax, not an unused variable. Found while writing the tests: execFileSync discards stderr on success, so the soft-limit test could never have seen the warning it was asserting. spawnSync returns both streams.
Cutting a final tag leaves its `-dev.N` snapshots behind: still installable, still holding the `dev` dist-tag, still tagged. `scripts/retire-prereleases.mjs` clears them in one step, printing the plan and changing nothing without --yes. It deprecates rather than unpublishes -- npm allows unpublishing for 72 hours, the version number can never be reused after, and anyone who pinned it has their install broken. A deprecated version keeps working and says why. It deletes a tag only when the tag's commit is reachable from origin/main. The provenance of a published package names both the commit and the tag: losing the name costs convenience, but deleting a tag that holds the only reference to its commit lets the commit be collected, which costs the attestation its subject. The decision of which tags those are is a pure function in the shared lib, so it is tested without a repository; the CLI is tested against a throwaway one with a tag on each side of the line. Splitting the script tests one module per script also turned the single end-of-file "the real changelog is untouched" test into an afterEach that compares its bytes after every test, and catches a stray RELEASE_NOTES.md too -- verified by planting both.
…pm download Two failures on the runner that no local run could produce. The scripts read $GITHUB_REF_NAME as a tag fallback and print warnings as ::warning:: annotations on stdout when $GITHUB_ACTIONS is set. Both leaked into the tests through the inherited environment, so `check-changelog` with no argument found the tag `main` instead of failing, and the warning the retire test looked for on stderr had gone to stdout. The harness now scrubs every GITHUB_ variable, and tests that want that behaviour pass it explicitly -- which also gets both warning channels covered for the first time. Verified by running the suite with the runner's variables set: two failures before, none after. Separately, `corepack enable` only writes shims, leaving the pinned pnpm to be downloaded by whoever invokes it first -- setup-node's cache probe, where the download crashed on an undici assertion inside Node 24.20.0 and nothing could retry it. The setup is now one composite action, shared by all four jobs, that downloads pnpm in a step of our own and retries it three times.
… skipping Chasing the resolver's "multiple projects" warning turned up something worse behind it: `boundaries/dependencies` never saw a cross-package import written as a package name. `@ailoud/providers` resolves through node_modules to `packages/providers/dist/index.js`, which matched no element pattern, so the rule classified the target as unknown and reported nothing -- while the same import written as `../../providers/src/index.js` was caught. Listing dist under the same type as src makes both forms the same violation. Verified by planting each one in packages/core: the package-name form was accepted before this change and is an error after it, the path form and the node:fs restriction still bite, and a clean tree lints silently. The resolver now reads the root tsconfig, which includes every package's sources, instead of a glob over the per-package ones -- one project rather than four, which is what it was asking for.
The v1.0.0-dev.1 release failed at "Run the gate", not at anything about releasing: the gate includes packages/providers/src/audio/ffmpeg.test.ts, which spawns the real binary, and this job never installed it. CI installs it in both jobs that run tests for exactly this reason, and the publish job runs the same gate.
Every `pnpm test` printed "error: unknown option '--bogus'" twice, a full usage block, and one ExperimentalWarning per worker -- so a real error had to be picked out of noise the tests produce on purpose. Commander writes usage errors and no-argument help to stderr, which is right for a CLI. The four tests that provoke it now silence writeErr only; writeOut stays as buildProgram set it, because that is how help reaches context.write and one of the tests asserts on it. The SQLite notice is the flag the installed binary already carries in its shebang: node:sqlite is experimental and this project knowingly depends on it. Set through NODE_OPTIONS rather than the pool's execArgv -- that route works, but switching the pool to forks took doctor.test.ts from 3 seconds to a 225-second timeout.
`npm publish dist-npm/ailoud-core-1.0.0-dev.1.tgz` never looked at the file: npm parsed the slash as the `owner/repo` GitHub shorthand and ran `git ls-remote ssh://git@github.com/dist-npm/ailoud-core-1.0.0-dev.1.tgz.git`, which failed on a missing public key -- an error about ssh keys in a step that has no business talking to git. An absolute path cannot be read that way. The `ls` that found the file is gone too: it existed to expand a name that was never a glob, and it turned a missing tarball into a confusing npm error instead of saying which one was missing.
`ailoud audio import` takes video because a meeting recording usually is one, and domain/mime.ts maps four containers -- but nothing exercised any of them. The audio path was tested against a tone generated at run time; video needs a real container, so scripts/make-fixtures.mjs now wraps en-short.wav in each of the four and the fixtures are committed, through LFS like the audio. They are deliberately dull: 32x32 of black at 5 fps, which keeps each file under 14 kB while still being a real, decodable video stream. Each container gets the codec pair it carries in the wild, and one test per container asserts what actually matters -- that whatever went in, the 16 kHz mono WAV whisper.cpp needs comes out. The video length comes from the audio rather than from `-shortest`, which produced an 18-second mp4 from a 2.5-second source.
…to a branch Re-running the publish workflow on v1.0.0-dev.1 failed with `no "## Version main" section`. The changelog check was the one step that passed no tag and let the script fall back to $GITHUB_REF_NAME -- which is the tag on a tag push but the BRANCH on a workflow_dispatch, so a manual re-run of a good tag checked the wrong version and could never pass. The tag is now resolved once into a job-level TAG and used by all three steps that need it, so there is no second answer to which tag is being released.
Publishing failed with `404 PUT https://registry.npmjs.org/@ailoud%2fcore` even after the organization existed, and the log never mentioned trusted publishing -- because it never happened. setup-node's registry-url writes an .npmrc holding `_authToken=${NODE_AUTH_TOKEN}`; this workflow has no token secret by design, so npm found a credential, sent an empty one, and had no reason to reach for OIDC. The registry answered as it would to any stranger. Without that .npmrc npm sees it has nothing, and does the exchange the `id-token: write` permission is there for. The default registry is registry.npmjs.org regardless, so nothing else changes -- and the `Unknown user config "always-auth"` warning was setup-node's file too.
…t can work npm answers ENEEDAUTH for @ailoud/core however complete the OIDC setup is -- npm 11.19.0, id-token: write, no stale .npmrc -- because a trusted publisher is attached to a package on npmjs.com and there is no page to attach it to until the package exists. The first version of each of the three has to go out on a credential; nothing about the workflow was wrong. So NPM_TOKEN is used when the secret is present and ignored when it is not, which makes removing the secret the whole of the switch to trusted publishing. Provenance is attached either way. The token is a bootstrap, not a fixture: once all three package pages have the publisher attached, deleting the secret leaves the arrangement with nothing to expire, which was the point of using OIDC in the first place.
Four places said only a final tag moves `latest`, without qualification. The first release proved otherwise: npm set `latest` to 1.0.0-dev.1 on all three packages, because it does that on a package's first publish whatever `--tag` says, and `latest` can be moved but never removed. So `npm install ailoud` returns the snapshot until 1.0.0 exists. Nothing in the workflow can prevent it, which is exactly why it belongs in the documentation rather than in a check.
All three npm pages read "This package does not have a README" after 1.0.0-dev.1 -- the first thing anyone arriving from a search saw. npm shows the README from inside the tarball, and no package directory had one. The two libraries get their own, short and specific: what the package is, how it relates to the other two, and that its interfaces are not stable and there is no reason to depend on it directly. The CLI's README is the repository's, so rather than keep a second copy in git that would drift, its prepack script copies the root file in at pack time -- verified identical in the packed tarball -- and the copy is gitignored. The packing guard now fails a release whose tarball has no README, beside the existing licence and no-source checks: the CLI's copy is a script that could stop working quietly, which is exactly the kind of thing that guard is for.
`ailoud --version` on the published 1.0.0-dev.1 answered `0.0.0`, and the MCP server told every client the same, because buildProgram passed commander a hardcoded string. Found by installing the release from the registry rather than by reading the code -- nothing in the repository disagreed with itself. The version now comes from the package's own manifest, resolved relative to the module so `dist/version.js` finds it one level up both here and in an installed package. The manifest is the one copy a release already updates and it ships inside the tarball, so it cannot go stale. The test asserts agreement with the manifest rather than a literal, so it needs no edit per release -- an edit per release is what would rot it into agreeing with whatever is there. Checked both ways: it fails against the old hardcoded value, and the packed tarballs installed into a scratch project report 1.0.0-dev.2.
The token exists to introduce each package to the registry, because a trusted publisher is attached to a package that already exists and there is no page to attach it to before the first publish. That bootstrap is over the moment the three packages exist -- but a token that keeps working is a token nobody gets round to removing, and the whole point of OIDC was having nothing stored. So the rule is enforced rather than remembered: a pre-release published on the secret logs a warning, and a final release refuses before anything is published and names the two steps that clear it. Publishing with no secret goes through OIDC exactly as before. Simulated all three paths -- pre-release with the token, final with the token, final without -- because the first version of this check read $version above the line that assigns it, which under `set -u` would have failed every release rather than only the ones it means to.
1.0.0-dev.2 cannot be republished -- npm's policy is that a version number is never reused, even after an unpublish -- so verifying that a release needs no stored credential takes a new number. Saying so in the changelog beats leaving someone to wonder what changed between two snapshots that are the same code.
…red token Deprecating the snapshots a release supersedes was a manual step because trusted publishing is defined for publishing: `npm deprecate` in the same job has nothing to authenticate with, and I was not willing to guess otherwise halfway through a release. Reading npm's own lib/utils/oidc.js settles it. Publishing gets its credential by exchanging the CI identity for a per-package token -- a GitHub id token with audience `npm:registry.npmjs.org`, posted to `/-/npm/v1/oidc/token/exchange/package/<name>` -- and the exchange is an ordinary request anything can make. So the script makes it, and retiring a release needs no more stored credential than publishing one. The token never reaches a command line or a log: it goes into a temporary 0600 npmrc that is removed in a finally, which the tests check both ways. `retire.yml` carries this, called by publish.yml after a final release and dispatchable alone -- without `confirm` it exchanges a token, uses it for nothing, and reports whether it worked, so the credential path can be checked without waiting for a release to find out.
A dispatchable retirement was two mistakes at once. Retiring snapshots means nothing unless something supersedes them, and the standalone run could not authenticate anyway: npm binds a trusted publisher to a workflow file, so a run entered through retire.yml is a different identity from one entered through publish.yml and the exchange is refused with `OIDC token exchange error - package not found`. The dry run proved that before a release depended on it. So retire.yml is workflow_call only, reached by publish.yml for a final tag, and its `confirm` input is gone -- it could only ever have been true, and a knob with one reachable value describes a choice that is not there. The script keeps its plan-first default for the laptop, where nothing has been decided. It runs after the publish rather than before. Deprecating the snapshots first would, if the publish then failed, leave every -dev.N pointing at a release that does not exist while `dev` is the only thing installable.
The release rules had grown by accretion: credentials were explained in two places, the retirement rules were spread over four paragraphs written as each came up, and a sentence about the changelog check sat at the end of a section about tags. Anyone reading it would have had to assemble the rules themselves. Now "Branches and Tags" covers branches, tags and the changelog fold, and a "Publishing" section covers the rest: the OIDC exchange with the two calls verbatim, the three consequences that constrain anyone changing it, the bootstrap exception with the token as a table, retirement, and the npm facts none of it can work around -- a version number used up forever, `latest` set on first publish and never removable, and trusted publishing covering `npm publish` and nothing else. Every claim states what it costs to get wrong, because that is what makes a rule followed rather than looked up: why the entry workflow cannot be retire.yml, why deprecating happens after the publish and not before, why the token goes to a file and not a command line.
A new `check-dependencies` skill: advisories first, funding second, updates last -- because the first two decide what an update is for. `pnpm audit` and `pnpm audit --prod` are read separately, since a high-severity advisory in a test runner cannot reach a user and one in `commander` is on their machine. The 14-day rule is a script rather than advice, because advice is what gets skipped at the moment it matters. `scripts/check-dependency-age.mjs` refuses any pinned direct dependency published less than 14 days ago: a compromised release is found by other people and that takes days, and there is no urgency in a patch that has been out two weeks that was not there on day one. The rule yields to a critical advisory -- two weeks with a known exploit is worse than a version nobody has audited yet -- through `scripts/dependency-age-exceptions.json`, where an exemption carries its advisory ID. That makes it a decision in the repository rather than an argument someone remembers to pass, and the check reports entries that have aged out so the file does not accumulate permanent holes. It runs second in `pre-release-check`, before the tests: an update it recommends changes what everything below it is testing.
Dependabot with `cooldown: default-days: 14`. Without it Dependabot opens a pull request the moment a version appears -- exactly the window check-dependency-age exists to refuse -- and the check would then fail on Dependabot's own branch, leaving the two arguing on every update. Security updates ignore cooldown, which is the behaviour we want and the same exception the age check records for a human: a known advisory beats an unaudited release. `versioning-strategy: increase` because exact pins are the convention here; `widen` would turn a pin into a range and hand the choice of version to whatever resolved last, which no age check can judge. The dev toolchain arrives as one grouped pull request since it cannot reach a user, while anything that ships gets its own. CodeQL is committed rather than enabled through the repository's default setup, for the reason every other check here is a file: what runs, when, and over what belongs in a diff. Weekly as well as per-push, because most findings arrive when the queries improve, not when the code changes.
…acktrack
CodeQL's first run found ten things; seven were real and this is them.
Two places decided whether an endpoint is a hosted API by substring:
`baseUrl.startsWith('https://api.openai.com')` and
`/api\.(openai|anthropic)\.com/.test(baseUrl)`. Both answer yes for
`https://api.openai.com.example.net/v1`, where the part of a hostname that
decides where the request goes is the end of it, and yes again for
`https://example.net/?upstream=api.openai.com`. They now share `isHostedLlm`,
which parses the URL and compares the hostname exactly.
Four `replace(/\/+$/, '')` calls stripped trailing slashes with a pattern that
backtracks; on a value that is mostly slashes that is a denial of service, and
the value comes from configuration. `withoutTrailingSlashes` is a loop, which
is what the operation always was. Two of the four CodeQL did not flag -- same
defect, below its threshold.
`escapePackageName` used `replace('/', '%2f')`, which substitutes only the
first match. A package name holds at most one slash, so it was right by
accident rather than by what it said; `replaceAll` says it. The age check had
its own copy of the same line and now imports the one function.
The stale-lock takeover in setupLock had a real window: two runs can find the
same stale lock, both remove it, and the loser's `open(path, 'wx')` failed with
a raw EEXIST about a path the user has never heard of. It now refuses the way
every other contended case does. Not covered by a test -- reproducing it needs
two interleaved processes -- so it is one branch converting one error code.
Dependabot's first run proposed TypeScript 7.0.2, and lint failed outright: "typescript-eslint does not support TS 7.0. Please see ... to run typescript-eslint using the TS 6 API." The bump is blocked by a peer, not by anything here, and left alone it would return every Monday with the same failure. Ignored for majors only, with the tracking issue named, so patches and minors keep arriving. Drop the entry when typescript-eslint supports TS >= 7.1.
mcpInstall, reports, setup, template, rm and annotate reported their results with the raw context.write channel, so their lines started at column 0 outside the clack frame while the frame's own lines stayed indented -- the same bug already fixed for self check/update. Route prose through ui.content, passing remarks and "nothing to do" through ui.note, and non-fatal problems or "nothing happened, here is the fix" through ui.warn, so every command's output renders inside the gutter in a terminal while PlainUi still writes it verbatim for a pipe. ls --json's empty-array line is untouched: it is the machine-output contract context.write exists for, same as self check's JSON and every transcript format.
ls --json's empty-array branch is the same machine-output contract as its non-empty one: a reader parses this, so it must stay on context.write like every other JSON payload, not move behind ui.content.
Add Ui.success() for a status line that succeeded, alongside the existing warn(): PrettyUi renders it with clack's log.success, PlainUi with the same "ok " word checks() already uses for a passing check. Apply it only where a command's output genuinely enumerates a status: mcpInstall's per-file and library actions (created/updated succeed, the rest are informational), reports' per-summary deletion outcome (deleted succeeds, already-gone is informational), annotate's "set title, notes" confirmation, template's "Wrote <path>" line (the "Use it with:" line that follows stays a note, not a second success), and setup's provisioning outcome loop, which now uses the marker methods instead of hand-writing "ok"/"FAILED" so the two cannot drift apart. self.test.ts's assertion on a failed sync row is updated for the "warning: " prefix ui.warn now adds ahead of the bare status word.
Two processes appending close together both read the same bytes and built their own new content from it, so whichever wrote last silently replaced the file with only its own line -- the other's was gone with no error. Detect that conflict before committing: build the candidate content, write it to a temp file, then re-read the real log and retry against the fresh content if it no longer matches what was read at the start, only renaming over the target once the two agree.
finish() still gives up on the in-flight check after a short bound and aborts it, so the process can exit right away -- but the resulting rejection was then cached as a genuine "no update", indistinguishable from a completed check that found nothing. Because a fast command's own work is almost always shorter than a real registry round trip, this meant the fetch was aborted, and a false negative cached, on essentially every run, so the notice could never fire. An abort caused by giving up is no longer cached at all: the next run starts fresh and tries again. Only a genuine registry answer or a genuine failure (a bad status, an unreadable body, a connection that fails on its own) is written. The cache is now filled by whichever command happens to run long enough to outlast the round trip (transcribe, summarize, setup), and every fast command in between prints from whatever one of those left behind.
The sweep self update spawns after a successful install called deps.spawn (bound to runInteractive) unconditionally, ignoring deps.interactive entirely -- unlike the install spawn a few lines above, which is correctly gated. runInteractive has no timeout by design and documents that callers must not invoke it non-interactively, so a forced update with no TTY whose sweep stalled (a registered project on a dead network mount, say) could hang the parent forever with no output. Gate the sweep exactly like the install: interactive keeps the unbounded streaming spawn, non-interactive runs it through the bounded runner with a generous but finite timeout instead, printing its output and failing instead of hanging when it stalls.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Owner
|
Closing in favour of recreating develop from main directly: the only commit develop held that main did not was this flow's own earlier merge commit, so there is no work to preserve through a merge. |
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
2 participants
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.
Automated back-merge of
mainintodevelop.Latest commit on main:
0387e9b- fix: make retiring a release one authentication, and stop it hangingTriggered by push to
mainin workflowBack-merge main -> developrun33977145898.Merge this PR (or enable auto-merge on it) to keep
developin sync with the latest release commit onmain. Resolve any conflicts manually before merging.