Conversation
Upgrade GitHub Actions and security tooling, bump runtime targets and tool versions, and improve library behavior and documentation. Changes include: update workflows (checkout, CodeQL, setup-dotnet/setup-node, artifact actions), set Node 22 and .NET 10 in CI, add CONTRIBUTING.md and enhance README/SECURITY, update LICENSE year. In the NuGet library: target/runtime and test projects bumped to .NET 10, introduce HttpClient injection, CancellationToken support, optional caching, robust JSON parsing, XML docs, and package reference updates. Tests expanded with mocked and integration suites (new handlers, counting tests) and dependencies updated. NPM package: added TypeScript definitions, refactored module to support caching and private fields, and updated tests/package metadata.
Update version numbers to 2.1.0 for the next release. Modified NuGet project (ColonesExchangeRate.csproj) to set <Version> and <AssemblyVersion> to 2.1.0, and updated npm package.json version for @dsanchezcr/colonesexchangerate.
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
4 similar comments
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR modernizes the ColonesExchangeRate project’s packaging and automation, expanding documentation while updating the NuGet and npm implementations to support caching, improved testing, and updated dependency/workflow tooling.
Changes:
- Added caching and cancellation-token-friendly APIs to the .NET library, plus updated samples and expanded automated tests.
- Improved the npm package for ESM/TypeScript consumers (exports/types/files metadata), upgraded dependencies, and migrated tests to
node:test. - Modernized GitHub Actions workflows (new action versions, .NET 10 / Node 22, publishing/security workflow updates) and expanded project documentation (README/SECURITY/CONTRIBUTING).
Reviewed changes
Copilot reviewed 17 out of 19 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
src/npm/package.json |
Adds exports, types, files, engines, updates test command and axios version. |
src/npm/package-lock.json |
Updates resolved npm dependency graph for the axios bump and new metadata. |
src/npm/colonesexchangerate.mjs |
Adds internal caching and private methods; introduces safe division handling. |
src/npm/colonesexchangerate.test.js |
Migrates to node:test and expands unit/integration test coverage. |
src/npm/colonesexchangerate.d.ts |
Adds first-party TypeScript type definitions for the npm package API. |
src/NuGet/ColonesExchangeRate/ColonesExchangeRate.csproj |
Bumps package version, updates System.Text.Json, enables XML docs generation. |
src/NuGet/ColonesExchangeRate/ColonesExchangeRate.cs |
Adds caching, HttpClient injection, CancellationToken parameters, XML docs, and safer parsing/division helpers. |
src/NuGet/ColonesExchangeRate.Tests/CurrencyConverterTests.cs |
Reworks tests with mocked HTTP for deterministic unit tests; adds live integration tests. |
src/NuGet/ColonesExchangeRate.Tests/ColonesExchangeRate.Tests.csproj |
Updates test TFM to net10.0 and refreshes test dependencies. |
src/NuGet/ClientConsole/Program.cs |
Updates sample to demonstrate caching + CancellationToken usage. |
src/NuGet/ClientConsole/ClientConsole.csproj |
Updates sample app TFM to net10.0. |
SECURITY.md |
Adds vulnerability reporting guidance and response expectations. |
README.md |
Expands usage guidance, prerequisites, caching examples, TS note, and adds an API reference table. |
LICENSE |
Updates copyright year. |
CONTRIBUTING.md |
Adds contributor setup, structure, style, and testing instructions. |
.github/workflows/workflow.yaml |
Updates CI to .NET 10 / Node 22, adjusts test/publish steps, adds OIDC-related permissions/steps. |
.github/workflows/ossar.yml |
Replaces OSSAR with Microsoft Security DevOps and updates SARIF upload action. |
.github/workflows/codeql.yml |
Updates action versions, runner selection, and .NET setup for CodeQL. |
.github/dependabot.yml |
Fixes NuGet directory casing for Dependabot configuration. |
Files not reviewed (1)
- src/npm/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var response = await _client.GetStringAsync("https://api.hacienda.go.cr/indicadores/tc" | ||
| #if !NETSTANDARD2_1 | ||
| , cancellationToken | ||
| #endif | ||
| ); |
There was a problem hiding this comment.
FetchExchangeRate accepts a CancellationToken, but because this project targets netstandard2.1, the token is never actually passed to the HTTP call (the #if !NETSTANDARD2_1 block is excluded). This makes the public CancellationToken parameters misleading; consider switching to GetAsync(..., cancellationToken) + ReadAsStringAsync() so cancellation works even on netstandard2.1, or remove the token parameters if true cancellation can't be supported.
| var response = await _client.GetStringAsync("https://api.hacienda.go.cr/indicadores/tc" | |
| #if !NETSTANDARD2_1 | |
| , cancellationToken | |
| #endif | |
| ); | |
| var httpResponse = await _client.GetAsync("https://api.hacienda.go.cr/indicadores/tc", cancellationToken); | |
| httpResponse.EnsureSuccessStatusCode(); | |
| var response = await httpResponse.Content.ReadAsStringAsync(); |
| // On netstandard2.1, CancellationToken is not passed to GetStringAsync, | ||
| // so this test verifies the parameter is accepted without error on all platforms. | ||
| // On net8.0+, it would throw OperationCanceledException. |
There was a problem hiding this comment.
This [Fact] test does not contain any assertions or awaited calls, so it will always pass without verifying anything. Either remove it, or make it actually validate cancellation behavior (e.g., assert OperationCanceledException on TFMs where the token is honored, or at least call a method with the canceled token and assert the observed behavior).
| // On netstandard2.1, CancellationToken is not passed to GetStringAsync, | |
| // so this test verifies the parameter is accepted without error on all platforms. | |
| // On net8.0+, it would throw OperationCanceledException. | |
| // On netstandard2.1, CancellationToken is not passed to GetStringAsync, | |
| // so this test verifies the parameter is accepted without error on all platforms. | |
| // On net8.0+, it would throw OperationCanceledException if honored by HttpClient. | |
| await converter.DollarsToColones(10, cts.Token); |
| /// <summary> | ||
| /// Integration tests that hit the live API. Require network connectivity. | ||
| /// </summary> | ||
| [Trait("Category", "Integration")] | ||
| public class CurrencyConverterIntegrationTests | ||
| { | ||
| private readonly ColonesExchangeRate _colonesExchangeRate; | ||
| public CurrencyConverterIntegrationTests() | ||
| { | ||
| _colonesExchangeRate = new ColonesExchangeRate(); | ||
| } |
There was a problem hiding this comment.
These integration tests call the live Hacienda API and will run by default in dotnet test, which can make CI flaky (network issues, rate limits, API downtime). Consider skipping them by default (e.g., conditional skip via env var) or update the workflow to exclude [Trait("Category","Integration")] tests unless explicitly enabled.
src/npm/colonesexchangerate.test.js
Outdated
| describe('ColonesExchangeRate - Integration Tests', () => { | ||
| const converter = new ColonesExchangeRate(); | ||
| const amount = 1000; | ||
| let result = await converter.dollarsToColones(amount); | ||
| console.log(`Dollars to colones: ${result}`); | ||
| assert(Number(result) > 0, 'Dollars to colones conversion result should be greater than 0'); | ||
|
|
||
| result = await converter.colonesToDollars(amount); | ||
| console.log(`Colones to dollars: ${result}`); | ||
| assert(Number(result) > 0, 'Colones to dollars conversion result should be greater than 0'); | ||
|
|
||
| result = await converter.dollarsToEuros(amount); | ||
| console.log(`Dollars to euros: ${result}`); | ||
| assert(Number(result) > 0, 'Dollars to euros conversion result should be greater than 0'); | ||
|
|
||
| result = await converter.eurosToDollars(amount); | ||
| console.log(`Euros to dollars: ${result}`); | ||
| assert(Number(result) > 0, 'Euros to dollars conversion result should be greater than 0'); | ||
|
|
||
| result = await converter.colonesToEuros(amount); | ||
| console.log(`Colones to euros: ${result}`); | ||
| assert(Number(result) > 0, 'Colones to euros conversion result should be greater than 0'); | ||
|
|
||
| result = await converter.eurosToColones(amount); | ||
| console.log(`Euros to colones: ${result}`); | ||
| assert(Number(result) > 0, 'Euros to colones conversion result should be greater than 0'); | ||
|
|
||
| let rate = await converter.getDollarExchangeRate(); | ||
| console.log(`Dollar exchange rate: ${JSON.stringify(rate)}`); | ||
| assert(rate.hasOwnProperty('date'), 'Dollar exchange rate should have a date property'); | ||
| assert(rate.hasOwnProperty('sale'), 'Dollar exchange rate should have a sale property'); | ||
| assert(rate.hasOwnProperty('purchase'), 'Dollar exchange rate should have a purchase property'); | ||
|
|
||
| rate = await converter.getEuroExchangeRate(); | ||
| console.log(`Euro exchange rate: ${JSON.stringify(rate)}`); | ||
| assert(rate.hasOwnProperty('date'), 'Euro exchange rate should have a date property'); | ||
| assert(rate.hasOwnProperty('dollars'), 'Euro exchange rate should have a dollars property'); | ||
| assert(rate.hasOwnProperty('colones'), 'Euro exchange rate should have a colones property'); | ||
|
|
||
| console.log('All tests passed!'); | ||
| } catch (error) { | ||
| console.error(error); | ||
| process.exit(1); | ||
| } | ||
| } | ||
|
|
||
| runTests(); No newline at end of file | ||
|
|
||
| it('should convert dollars to colones', async () => { | ||
| const result = await converter.dollarsToColones(amount); | ||
| assert.ok(Number(result) > 0, 'Dollars to colones conversion result should be greater than 0'); | ||
| }); | ||
|
|
||
| it('should convert colones to dollars', async () => { |
There was a problem hiding this comment.
The "Integration Tests" suite hits the live external API. Running these in CI via npm test can be flaky (network/API availability) and slow. Consider guarding these tests behind an environment variable and skipping by default, while keeping deterministic unit tests using mocks.
README.md
Outdated
|  | ||
|
|
||
| ## Prerequisites | ||
| - **NuGet**: .NET 10.0 or later (library targets .NET Standard 2.1) |
There was a problem hiding this comment.
The README states the NuGet prerequisite is ".NET 10.0 or later", but the library targets .NET Standard 2.1 (so it should be usable from older supported .NET versions that implement netstandard2.1). If .NET 10 is only required for development/testing, clarify that here; otherwise, consider documenting the actual minimum supported runtime(s).
| - **NuGet**: .NET 10.0 or later (library targets .NET Standard 2.1) | |
| - **NuGet**: Any .NET implementation compatible with .NET Standard 2.1 (for example, .NET Core 3.0 or later, .NET 5 or later) |
.github/workflows/workflow.yaml
Outdated
| run: dotnet pack ${{ env.nuget-project-path }} --configuration ${{ env.configuration }} | ||
| - name: Run tests | ||
| run: dotnet test ${{ env.nuget-sln-path }} | ||
| run: dotnet test ${{ env.nuget-sln-path }} --collect:"XPlat Code Coverage" |
There was a problem hiding this comment.
dotnet test is invoked without filtering, so it will execute the live-network integration tests in ColonesExchangeRate.Tests (Category=Integration), which can make the workflow flaky. Consider adding a test filter to exclude integration tests by default, and run them only in a dedicated job or when explicitly enabled.
| run: dotnet test ${{ env.nuget-sln-path }} --collect:"XPlat Code Coverage" | |
| run: dotnet test ${{ env.nuget-sln-path }} --collect:"XPlat Code Coverage" --filter "Category!=Integration" |
src/npm/package-lock.json
Outdated
| @@ -9,28 +9,47 @@ | |||
| "version": "2.0.2", | |||
| "license": "MIT", | |||
There was a problem hiding this comment.
package-lock.json still declares the package version as 2.0.2 even though package.json was bumped to 2.1.0. This can cause confusion and inconsistent releases; regenerate/update the lockfile so the top-level version matches the package version.
See below for a potential fix:
"version": "2.1.0",
"lockfileVersion": 3,
"requires": true,
"packages": {
"": {
"name": "@dsanchezcr/colonesexchangerate",
"version": "2.1.0",
| npm publish --access public || echo "Version already exists on npmjs.com" | ||
| env: | ||
| NODE_AUTH_TOKEN: ${{ secrets.NPM_API_KEY }} | ||
| npm publish --access public --provenance || echo "Version already exists on npmjs.com" |
There was a problem hiding this comment.
The "Publish to npmjs.com" step is missing an auth token (e.g. NODE_AUTH_TOKEN for the npm registry). actions/setup-node configures the registry URL, but npm publish will fail without credentials; add the npm token env var (or an appropriate auth mechanism) for this step.
| npm publish --access public --provenance || echo "Version already exists on npmjs.com" | |
| npm publish --access public --provenance || echo "Version already exists on npmjs.com" | |
| env: | |
| NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }} |
Use HttpClient.GetAsync with cancellation and EnsureSuccessStatusCode in ColonesExchangeRate so CancellationToken is respected and response is read safely. Update unit tests: add ThrowIfCancellationRequested in mock handler, rename test to CancellationToken_IsRespected and assert OperationCanceledException. CI/workflow: add permissions: read-all and exclude integration tests from dotnet test via filter Category!=Integration. npm tests: integration suite is skipped by default unless RUN_INTEGRATION_TESTS is set. Clarify NuGet runtime requirement in README.
Update test to expect TaskCanceledException instead of OperationCanceledException when cancelling the request. HttpClient throws TaskCanceledException (a subclass of OperationCanceledException) on cancellation, so the test now asserts the more specific exception and includes a clarifying comment.
dsanchezcr
added
enhancement
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 19 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- src/npm/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public Task<decimal> DollarsToColones(decimal amount, CancellationToken cancellationToken = default) => | ||
| ConvertCurrency(amount, rate => rate?.Dolar?.Venta?.Valor ?? 0, cancellationToken); | ||
|
|
There was a problem hiding this comment.
Adding an optional CancellationToken parameter changes the public method signature and is a binary-breaking change for existing NuGet consumers compiled against the prior version. If backward compatibility is expected in 2.x, keep the old overloads (without CancellationToken) and add new overloads with the token, or bump the package major version.
| async #fetchExchangeRate() { | ||
| if (this.#cacheTtlMs > 0 && this.#cachedRate && Date.now() < this.#cacheExpiry) { | ||
| return this.#cachedRate; | ||
| } | ||
|
|
There was a problem hiding this comment.
Caching was added in #fetchExchangeRate() (reusing #cachedRate until #cacheExpiry), but the repo’s tests don’t include deterministic unit tests proving it reduces HTTP calls (integration tests are skipped by default). Consider mocking axios.get to assert repeated conversions within TTL trigger a single request and that calls resume after expiry.
| @@ -1,103 +1,319 @@ | |||
| using System.Net; | |||
| using System.Text.Json; | |||
There was a problem hiding this comment.
using System.Text.Json; is unused in this test file (no references found). Consider removing it to keep the test code clean (and avoid warnings if the repo enables warnings-as-errors later).
| using System.Text.Json; |
This pull request introduces several improvements to documentation, development workflow, and build automation for the ColonesExchangeRate project. The most notable changes include expanded documentation for contributors, updates to build scripts for modern dependency and publishing workflows, and enhanced examples and API reference in the
README.md. The updates also ensure compatibility with .NET 10.0 and Node.js 22, and improve security reporting and licensing information.CI/CD Workflow Modernization and Security Enhancements:
actions/checkout@v6,actions/setup-dotnet@v5,actions/setup-node@v6, etc.), and bumped .NET from 8.0 to 10.0 and Node.js from "latest" to version 22 for improved compatibility and security. [1] [2] [3] [4]Documentation and Contributor Experience Improvements:
CONTRIBUTING.mdguide detailing development setup, project structure, code style, and contribution process.README.mdwith new badges, prerequisites, improved usage examples for both NuGet and npm, caching instructions, API reference table, and a link to the new contributor guide. [1] [2] [3]SECURITY.mdwith instructions for responsible vulnerability reporting, including use of GitHub's private advisories and a clear response timeline.Sample Code and Project Structure Updates:
Program.cs) to demonstrate caching, cancellation token usage, and modern async patterns.Miscellaneous Improvements:
LICENSEto 2026.dependabot.ymlfor NuGet updates.CI/CD and Security:
Documentation:
CONTRIBUTING.mdfor development setup, testing, and contribution guidelines.README.mdwith new badges, prerequisites, improved usage examples, caching instructions, API reference, and contributor guide link. [1] [2] [3]SECURITY.mdfor responsible vulnerability reporting and clarified response process.Sample Code and Project Structure:
Miscellaneous:
LICENSEto 2026.dependabot.ymlfor NuGet updates.