Skip to content

fix(skills): stop ambient env leaking into generated SKILL.md output - #15948

Open
BenjaminAronsson wants to merge 3 commits into
diegosouzapw:release/v3.8.52from
BenjaminAronsson:fix/skills-generator-determinism
Open

BenjaminAronsson wants to merge 3 commits into
diegosouzapw:release/v3.8.52from
BenjaminAronsson:fix/skills-generator-determinism

Conversation

@BenjaminAronsson

@BenjaminAronsson BenjaminAronsson commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Makes the SKILL.md generator's output a pure function of its inputs. fix(skills): regenerate the 20 SKILL.md files drifted since #14381 #15945 regenerates the 20 drifted files; this PR stops them drifting again.
  • apiOperationExample.ts / cliConfigurationExample.ts called resolveOmniRouteBaseUrl(), which reads process.env. Their only consumer is the generator, whose output is committed and then diff-gated by check:agent-skills-sync — so the committed artifact was a function of whoever last ran the generator. Anyone with PORT, API_PORT, DASHBOARD_PORT, BASE_URL or OMNIROUTE_BASE_URL set baked their own host into the repo; CI, which sets none of them, then reported drift. Because Merge integrity is .mergify.yml's always-on merge anchor, that drift blocked every PR into the branch.
  • baseUrl is now an explicit argument defaulting to DEFAULT_OMNIROUTE_BASE_URL, so regeneration is reproducible on any machine.
  • fix(docs): resolve generated skill curl examples to the configured loopback port #15778's capability is preserved, not reverted — a caller that wants the operator's configured host passes resolveOmniRouteBaseUrl() as that argument. What changes is that it must be asked for rather than picked up from the ambient environment.

Related Issues

Validation

  • Change type: other — documentation generator (src/lib/agentSkills/**), no request-path or DB code
  • Focused tests and category gates from the golden path — the contract suite plus both other generator suites; TDD red→green recorded below
  • npm run lint — exit 0
  • Reconciled with the current active release base — worktree cut fresh from the release/v3.8.52 tip (61e07fb7e0); focused checks rerun afterward
  • Production-code changes include a new or updated automated test in this PR
check result
tests/unit/cli-configuration-consumer-contract.test.ts 5/5
agentSkills-generator + cli-api-generator-ref-params 30/30
typecheck:core exit 0
npm run lint exit 0
generated output vs #15945's committed files byte-identical (git diff empty)

TDD record (Hard Rule #18). The new determinism test was confirmed RED against the previous code before the fix was restored:

AssertionError [ERR_ASSERTION]: env {"PORT":"37128"} leaked into the example
ℹ pass 0
ℹ fail 2

Tests Added Or Updated

  • tests/unit/cli-configuration-consumer-contract.test.ts (updated) — three changes:
    • New: generated examples ignore ambient base-url env so committed output is reproducible — asserts the default output is identical across all five env vars (PORT, API_PORT, DASHBOARD_PORT, BASE_URL, OMNIROUTE_BASE_URL). This is the regression guard, and the one confirmed red above.
    • New: configuration-API examples are reproducible and still accept an explicit host — pins the /api/cli-tools/* branch, which routes through cliConfigurationExample.ts, on both paths.
    • Rewritten: fix(docs): resolve generated skill curl examples to the configured loopback port #15778's configuration examples track the configured loopback port… → …when given one. Same assertion (http://localhost:37128, no 20128), now via the explicit argument, so fix(docs): resolve generated skill curl examples to the configured loopback port #15778's behaviour stays covered rather than being silently dropped.
    • Added a withBaseUrlEnv() helper so each test restores all five vars in a finally, replacing the previous three-variable save/restore block.

Coverage Notes

  • Touched production files are src/lib/agentSkills/apiOperationExample.ts and src/lib/agentSkills/cliConfigurationExample.ts. Both are fully exercised by cli-configuration-consumer-contract.test.ts, which covers every branch: bearer, dashboard-session (login / mutation / read), and the two /api/cli-tools/* paths — each now on both the default and explicit-baseUrl paths, so the new parameter adds covered branches rather than uncovered ones.
  • No coverage moves down; the change is a signature change plus removal of two process.env reads.

Reviewer Notes

The curl-example builders called resolveOmniRouteBaseUrl(), which reads
process.env. Their only consumer is the SKILL.md generator, whose output is
committed and then diff-gated by check:agent-skills-sync — so the committed
artifact was a function of whoever last ran the generator. Anyone with PORT,
API_PORT, DASHBOARD_PORT, BASE_URL or OMNIROUTE_BASE_URL set wrote their own
host into the files, and CI, which sets none of them, reported drift. That is
the trap that left all 20 skills drifted and blocked every PR into the branch
(the Merge integrity job is .mergify.yml's always-on merge anchor).

baseUrl is now an explicit argument defaulting to DEFAULT_OMNIROUTE_BASE_URL,
so regeneration is reproducible on any machine.

diegosouzapw#15778's capability is preserved, not dropped: a caller that wants the
operator's configured host passes resolveOmniRouteBaseUrl() as that argument,
and its test now asserts exactly that. What changes is that it must be asked
for rather than picked up from the environment.

Verified byte-identical to the regenerated output in diegosouzapw#15945, so the two land
in either order without conflict. The determinism test was confirmed RED
against the previous code ('env {"PORT":"37128"} leaked into the example')
before the fix was restored. 5/5 in the contract suite, 30/30 across
agentSkills-generator and cli-api-generator-ref-params, typecheck and lint 0.
Refresh against the current release base, preserve canonical generator defaults and explicit host support, and remove an inaccurate reference to a generator CLI flag.

Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
@diegosouzapw

Copy link
Copy Markdown
Owner

Hi @BenjaminAronsson — closing this pull request to clear the open queue. The branch fix/skills-generator-determinism is kept, so the commits stay. Comment here if it should be reopened.

@diegosouzapw

Copy link
Copy Markdown
Owner

Owner has explicitly requested reopening and merging the campaign PRs. This PR was already reopened when checked. The relay-15948 reconciliation worktree belongs to another session, so this campaign will not overwrite or merge over that session. A fresh read-only VPS probe on generated-output candidate 6718424 confirms the independent determinism defect: PORT=37128 makes sync exit 2 (19 stale / 27 unchanged). This PR remains necessary unless a new current-base execution proves supersession. Fresh required checks are pending; no inherited failure or pending result is being treated as PASS.

Preserve deterministic examples and contributor history while refreshing the exact release base and correcting the changelog fragment format.

Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
@diegosouzapw

Copy link
Copy Markdown
Owner

Reopened at the repository owner's explicit request, retaining the original contribution. Thank you @BenjaminAronsson for fixing environment-dependent generated examples.

Normal fast-forward update: head b9b3c20f671005eb7a3efd841010e4be649a255b, tree 936fbfb66074c7dc2f1ce8cb2ac0c1dfe2f8060d, real merge of the previous contributor head and pinned release base 7a0bb8754d7432c9fc6b899464d88adbb63d0bd0. Benjamin remains author, maintainer co-author. The only additional correction was the required bullet marker in this PR's changelog fragment.

With dependencies matching the current lock: exact-base-helper RED 3 PASS / 2 FAIL; restored fix GREEN 35 PASS across the contract, skill-generator and CLI-reference-parameter suites. Focused lint, final format, test-masking, changelog, typecheck:core and normal hooks exit 0. The physical cache is from a completed clean npm ci; no separate clean install per worktree is claimed.

Admission remains HOLD until current-candidate applicable CI succeeds and the separately owned generated-content repair #15945 lands. This PR intentionally does not copy that contribution: its standalone generator still reports 20 pending / 26 unchanged / zero errors. No merge, bypass, force push or baseline increase occurred.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants