Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds reusable GitHub Actions (composite actions and workflows) for .NET build/test/release, a Cobertura coverage-check composite action with optional badge publishing, README/.gitignore updates, two templates (FrendsToolkitTemplate, FrendsTaskTemplate) plus new library/task code, validation attributes, handlers, tests, and solution/project scaffolding. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant GH as GitHub Actions
participant Dotnet as dotnet-setup
participant TestJob as Test Job
participant Check as check-coverage
participant Badge as Badge Service
Dev->>GH: push / workflow_dispatch triggers toolkit_test
GH->>Dotnet: run dotnet-setup (checkout + setup-dotnet + restore)
Dotnet-->>GH: SDK & deps ready
GH->>TestJob: run dotnet test --collect:"XPlat Code Coverage"
TestJob->>Check: produce coverage file (`coverage.cobertura.xml`)
Check->>Check: parse line-rate, compute rounded percent
alt coverage >= min-line-coverage
Check->>Badge: POST badge payload (if API key set)
Badge-->>Check: 200 OK
Check-->>GH: set output `coverage` and success summary
else coverage < min-line-coverage or file missing
Check-->>GH: write failure summary and fail the step
GH-->>Dev: workflow failed
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+13
to
+16
| uses: FrendsPlatform/FrendsTasks/.github/workflows/toolkit_test.yml@main | ||
| with: | ||
| working-directory: 'Frends.Common.Toolkit' | ||
| project-directory: 'Frends.Common.Toolkit' |
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 26 days ago
In general, the fix is to add an explicit permissions block that restricts the GITHUB_TOKEN to the minimal scopes required. This can be done at the workflow root (applies to all jobs that don’t override it) or per job. Since this workflow consists of a single build job that reuses another workflow, and we don’t see any need for write access in this wrapper, the safest default is to set permissions: contents: read for the job, which is the standard minimal starting point.
Concretely, in .github/workflows/common_toolkit_test.yml, under jobs:, within the build: job, add a permissions: mapping before uses:. For example:
jobs:
build:
permissions:
contents: read
uses: ...This constrains the GITHUB_TOKEN available to the reusable workflow invocation to read-only repository contents (unless the called workflow needs and requests more; if it does, it can define its own permissions block). No imports or additional methods are needed since this is purely a YAML configuration change within the shown file.
Suggested changeset
1
.github/workflows/common_toolkit_test.yml
| @@ -10,6 +10,8 @@ | ||
|
|
||
| jobs: | ||
| build: | ||
| permissions: | ||
| contents: read | ||
| uses: FrendsPlatform/FrendsTasks/.github/workflows/toolkit_test.yml@main | ||
| with: | ||
| working-directory: 'Frends.Common.Toolkit' |
Copilot is powered by AI and may make mistakes. Always verify output.
Unable to commit as this autofix suggestion is now outdated
Contributor
There was a problem hiding this comment.
Probably a good idea to only have read-only
Contributor
Author
There was a problem hiding this comment.
It is already set to read on workflow level, so will be also aplied to all jobs
…into fspw-732-CommonLib-PoC # Conflicts: # .github/actions/dotnet-setup/action.yml # .github/workflows/common_toolkit_release.yml # .github/workflows/toolkit_release.yml # Frends.Common.Toolkit/Frends.Common.Toolkit/Attributes/NotEmptyStringAttribute.cs # Frends.Common.Toolkit/Frends.Common.Toolkit/Attributes/RequiredIfAttribute.cs # Frends.Common.Toolkit/Frends.Common.Toolkit/Definitions/ITaskError.cs # Frends.Common.Toolkit/Frends.Common.Toolkit/Definitions/ITaskOptions.cs # Frends.Common.Toolkit/Frends.Common.Toolkit/Definitions/ITaskResult.cs # Frends.Common.Toolkit/Frends.Common.Toolkit/Frends.Common.Toolkit.csproj # Frends.Common.Toolkit/Frends.Common.Toolkit/Handlers/ValidationHandler.cs
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (1)
.github/workflows/common_toolkit_test.yml (1)
1-16:⚠️ Potential issue | 🟡 MinorAdd explicit
permissionsblock to restrictGITHUB_TOKENscope.The workflow doesn't specify permissions for the
GITHUB_TOKEN, which means it uses default permissions. Following the principle of least privilege, add an explicit permissions block.🛡️ Suggested fix
name: Common Toolkit Test on: push: branches: - '**' paths: - 'Frends.Common.Toolkit/**' workflow_dispatch: +permissions: + contents: read + jobs: build: uses: FrendsPlatform/FrendsTasks/.github/workflows/toolkit_test.yml@main🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/common_toolkit_test.yml around lines 1 - 16, Add an explicit top-level permissions block to the "Common Toolkit Test" workflow to restrict GITHUB_TOKEN scope (instead of relying on defaults); update the workflow YAML that defines name: "Common Toolkit Test" (the file containing the build job that uses FrendsPlatform/FrendsTasks/.github/workflows/toolkit_test.yml@main and the build job named "build") to include a minimal permissions mapping (for example at minimum permissions: contents: read, and remove/write scopes you don't need) so the GITHUB_TOKEN only has the least-privileged access required for the job.
🧹 Nitpick comments (3)
Frends.Common.Toolkit/Frends.Common.Toolkit/Handlers/ValidationHandler.cs (1)
20-21: ConsiderStringBuilderfor aggregating validation messages.Using
Aggregatewith string concatenation creates intermediate strings on each iteration. For a small number of validation errors this is fine, butStringBuilderorstring.Joinwould be more efficient if validation error counts grow.♻️ Alternative using string.Join
- var validationMessage = objects.Select(obj => obj.Validate()) - .Aggregate(string.Empty, (current, message) => current + message); + var validationMessage = string.Join(string.Empty, objects.Select(obj => obj.Validate()));Or in the
Validatemethod:- return validateResults.Aggregate(string.Empty, (current, error) => current + $"{error.ErrorMessage}\n"); + return string.Join("\n", validateResults.Select(e => e.ErrorMessage)) + (validateResults.Count > 0 ? "\n" : "");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Frends.Common.Toolkit/Frends.Common.Toolkit/Handlers/ValidationHandler.cs` around lines 20 - 21, The current aggregation uses objects.Select(obj => obj.Validate()).Aggregate(...) which concatenates strings repeatedly and causes intermediate allocations; replace this with a more efficient approach such as collecting the individual messages and using string.Join or a StringBuilder: call objects.Select(obj => obj.Validate()).Where(m => !string.IsNullOrEmpty(m)).ToList() and then string.Join(Environment.NewLine, ...) (or build with a StringBuilder) to produce validationMessage; update references to Validate and validationMessage accordingly so empty messages are excluded and concatenation is performed once..github/workflows/common_toolkit_release.yml (1)
11-18: Add explicit permissions block to limit GITHUB_TOKEN scope.The workflow lacks a
permissionsblock. Adding one follows the principle of least privilege and prevents unintended token usage.🔒 Proposed fix
jobs: build: + permissions: + contents: read uses: FrendsPlatform/FrendsTasks/.github/workflows/toolkit_release.yml@main🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/common_toolkit_release.yml around lines 11 - 18, The workflow job "build" that uses FrendsPlatform/FrendsTasks/.github/workflows/toolkit_release.yml@main is missing an explicit permissions block for GITHUB_TOKEN; add a permissions section under the top-level job (or workflow) restricting scopes to only what the toolkit release needs (for example: contents: read and packages: write) so the wrapped action invoked by uses has minimal token privileges; update the job "build" definition to include this permissions block adjacent to the existing uses/with/secrets entries.Frends.Common.Toolkit/Frends.Common.Toolkit/Attributes/RequiredIfAttribute.cs (1)
12-13: Fail fast on invalid attribute configuration.If
targetValuesis empty (valid withparams) ordependentPropertyis blank, the attribute can silently become a no-op or produce misleading runtime errors. Validate these inputs in the constructor.Proposed fix
public class RequiredIfAttribute(string dependentProperty, params object[] targetValues) : ValidationAttribute { + private readonly string _dependentProperty = !string.IsNullOrWhiteSpace(dependentProperty) + ? dependentProperty + : throw new ArgumentException("Dependent property name must be provided.", nameof(dependentProperty)); + + private readonly object[] _targetValues = targetValues is { Length: > 0 } + ? targetValues + : throw new ArgumentException("At least one target value must be provided.", nameof(targetValues)); + /// <inheritdoc/> protected override ValidationResult IsValid(object value, ValidationContext validationContext) { - var field = validationContext.ObjectType.GetProperty(dependentProperty); + var field = validationContext.ObjectType.GetProperty(_dependentProperty); @@ - if (!targetValues.Contains(dependentValue)) return ValidationResult.Success; + if (!_targetValues.Contains(dependentValue)) return ValidationResult.Success;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Frends.Common.Toolkit/Frends.Common.Toolkit/Attributes/RequiredIfAttribute.cs` around lines 12 - 13, The RequiredIfAttribute constructor currently accepts dependentProperty and params object[] targetValues without validation; add guard clauses in the RequiredIfAttribute(string dependentProperty, params object[] targetValues) constructor to throw ArgumentNullException or ArgumentException if dependentProperty is null/empty/whitespace and to throw ArgumentException if targetValues is null or has zero length, ensuring the attribute fails fast on invalid configuration and avoids becoming a silent no-op.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/actions/check-coverage/action.yml:
- Around line 49-57: lineRatePct's Python heredoc interpolates $file directly
which can break if the filename contains quotes or special chars; instead export
the path into a safe environment variable (e.g., COVERAGE_FILE) or pass it via
stdin and read it inside the Python snippet (use os.environ['COVERAGE_FILE'] or
sys.stdin) so the Python code no longer relies on interpolated shell quoting;
update the heredoc that produces lineRatePct to read the filename from that
environment variable or stdin before parsing with xml.etree.ElementTree.
In @.github/actions/dotnet-setup/action.yml:
- Around line 10-14: The composite action input "working-directory" in
action.yml includes an unsupported "type" field; remove the "type: string" line
from the "working-directory" input and keep only the supported keys (required,
default, description) so the composite action input conforms to composite action
schema.
In @.github/workflows/common_toolkit_release.yml:
- Around line 17-18: The secret name passed to the reusable workflow is wrong:
update the secrets map in this workflow so the key used matches what
toolkit_release.yml expects (change the secrets entry NUGET_API_KEY to
NUGETORG_API_KEY) so the reusable workflow receives the expected
NUGETORG_API_KEY value when calling the toolkit_release.yml reusable workflow.
In @.github/workflows/toolkit_release.yml:
- Around line 6-8: The reusable workflow currently declares the required secret
as NUGETORG_API_KEY but the caller sends NUGET_API_KEY; update the reusable
workflow's required secret name from NUGETORG_API_KEY to NUGET_API_KEY (or
alternatively change the caller to pass NUGETORG_API_KEY) so the secret mapping
matches exactly; look for the secret declaration named NUGETORG_API_KEY and make
it identical to the secret name used by the caller (NUGET_API_KEY) to prevent
invocation failure.
In @.github/workflows/toolkit_test.yml:
- Around line 64-69: The upload-artifact path incorrectly assumes
project-directory is repo-root-relative; update the Upload artifact step to join
working-directory and project-directory so the path points to the built .nupkg
under the actual working tree (e.g. use '${{ inputs.working-directory }}/${{
inputs.project-directory }}/bin/Release/*.nupkg' or equivalent expression).
Modify the step that uses actions/upload-artifact@v7.0.0 (the "Upload artifact"
step) to use the combined path expression so artifacts are found when
working-directory is set.
- Around line 98-99: The reusable workflow is referencing
${BADGE_SERVICE_API_KEY} but does not declare it in the on.workflow_call.secrets
block; update toolkit_test.yml to add BADGE_SERVICE_API_KEY as an optional or
required secret under on.workflow_call.secrets, or alternatively modify the
caller common_toolkit_test.yml to pass secrets by adding secrets: inherit to the
uses call so BADGE_SERVICE_API_KEY is available at runtime; ensure you reference
the BADGE_SERVICE_API_KEY key name exactly and update whichever file contains
the workflow_call or uses invocation accordingly.
In
`@Frends.Common.Toolkit/Frends.Common.Toolkit.Tests/AttributesTests/NotEmptyStringAttributeTests.cs`:
- Around line 1-4: The test file is missing the NUnit namespace import required
for attributes and assertions; add the directive "using NUnit.Framework;" to the
top of NotEmptyStringAttributeTests (or the test file containing the [Test]
methods and Assert.That calls) so the NUnit attributes and assertion helpers
resolve and the file compiles.
In
`@Frends.Common.Toolkit/Frends.Common.Toolkit.Tests/AttributesTests/RequiredIfAttributeTests.cs`:
- Around line 34-46: The test method ShouldValidateWithFailureWhenConditionMet
is parameterized with name but never uses it—test always sets test.Name =
string.Empty; update the test to assign the parameter to the TestClass instance
(test.Name = name) and adjust TestCase values to meaningful inputs (e.g., remove
"foobar" which would succeed; keep "" and null and optionally add a
whitespace-only case if RequiredIfAttribute should reject whitespace). Ensure
the assertion still checks Validate(test) and ErrorMessage contains "Name is
required".
In `@Frends.Common.Toolkit/Frends.Common.Toolkit/Definitions/ITaskOptions.cs`:
- Line 1: Remove the unused using directive by deleting "using
System.ComponentModel;" from the file containing the ITaskOptions interface
(ITaskOptions.cs); ensure the file still compiles and contains only the
necessary using directives for the ITaskOptions definition and keep the
interface name ITaskOptions unchanged.
In `@README.md`:
- Line 3: Update the README sentence that currently reads "Frends Tasks related
things" to use a hyphenated compound adjective; replace it with "Frends
Tasks-related things" (locate the phrase "Frends Tasks related things" in the
README and apply the hyphenation).
---
Duplicate comments:
In @.github/workflows/common_toolkit_test.yml:
- Around line 1-16: Add an explicit top-level permissions block to the "Common
Toolkit Test" workflow to restrict GITHUB_TOKEN scope (instead of relying on
defaults); update the workflow YAML that defines name: "Common Toolkit Test"
(the file containing the build job that uses
FrendsPlatform/FrendsTasks/.github/workflows/toolkit_test.yml@main and the build
job named "build") to include a minimal permissions mapping (for example at
minimum permissions: contents: read, and remove/write scopes you don't need) so
the GITHUB_TOKEN only has the least-privileged access required for the job.
---
Nitpick comments:
In @.github/workflows/common_toolkit_release.yml:
- Around line 11-18: The workflow job "build" that uses
FrendsPlatform/FrendsTasks/.github/workflows/toolkit_release.yml@main is missing
an explicit permissions block for GITHUB_TOKEN; add a permissions section under
the top-level job (or workflow) restricting scopes to only what the toolkit
release needs (for example: contents: read and packages: write) so the wrapped
action invoked by uses has minimal token privileges; update the job "build"
definition to include this permissions block adjacent to the existing
uses/with/secrets entries.
In
`@Frends.Common.Toolkit/Frends.Common.Toolkit/Attributes/RequiredIfAttribute.cs`:
- Around line 12-13: The RequiredIfAttribute constructor currently accepts
dependentProperty and params object[] targetValues without validation; add guard
clauses in the RequiredIfAttribute(string dependentProperty, params object[]
targetValues) constructor to throw ArgumentNullException or ArgumentException if
dependentProperty is null/empty/whitespace and to throw ArgumentException if
targetValues is null or has zero length, ensuring the attribute fails fast on
invalid configuration and avoids becoming a silent no-op.
In `@Frends.Common.Toolkit/Frends.Common.Toolkit/Handlers/ValidationHandler.cs`:
- Around line 20-21: The current aggregation uses objects.Select(obj =>
obj.Validate()).Aggregate(...) which concatenates strings repeatedly and causes
intermediate allocations; replace this with a more efficient approach such as
collecting the individual messages and using string.Join or a StringBuilder:
call objects.Select(obj => obj.Validate()).Where(m =>
!string.IsNullOrEmpty(m)).ToList() and then string.Join(Environment.NewLine,
...) (or build with a StringBuilder) to produce validationMessage; update
references to Validate and validationMessage accordingly so empty messages are
excluded and concatenation is performed once.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5ddac057-1193-412f-9e99-5bb978359a9a
📒 Files selected for processing (23)
.gitattributes.github/actions/check-coverage/action.yml.github/actions/dotnet-setup/action.yml.github/workflows/common_toolkit_release.yml.github/workflows/common_toolkit_test.yml.github/workflows/toolkit_release.yml.github/workflows/toolkit_test.yml.gitignoreFrends.Common.Toolkit/Frends.Common.Toolkit.Tests/AttributesTests/AttributeTestBase.csFrends.Common.Toolkit/Frends.Common.Toolkit.Tests/AttributesTests/NotEmptyStringAttributeTests.csFrends.Common.Toolkit/Frends.Common.Toolkit.Tests/AttributesTests/RequiredIfAttributeTests.csFrends.Common.Toolkit/Frends.Common.Toolkit.Tests/Frends.Common.Toolkit.Tests.csprojFrends.Common.Toolkit/Frends.Common.Toolkit.Tests/HandlersTests/ValidationHandlerTests.csFrends.Common.Toolkit/Frends.Common.Toolkit.slnFrends.Common.Toolkit/Frends.Common.Toolkit/Attributes/NotEmptyStringAttribute.csFrends.Common.Toolkit/Frends.Common.Toolkit/Attributes/RequiredIfAttribute.csFrends.Common.Toolkit/Frends.Common.Toolkit/Definitions/ITaskError.csFrends.Common.Toolkit/Frends.Common.Toolkit/Definitions/ITaskOptions.csFrends.Common.Toolkit/Frends.Common.Toolkit/Definitions/ITaskResult.csFrends.Common.Toolkit/Frends.Common.Toolkit/Frends.Common.Toolkit.csprojFrends.Common.Toolkit/Frends.Common.Toolkit/GlobalSuppressions.csFrends.Common.Toolkit/Frends.Common.Toolkit/Handlers/ValidationHandler.csREADME.md
Comment on lines
+64
to
+69
| - name: Upload artifact | ||
| uses: actions/upload-artifact@v7.0.0 | ||
| with: | ||
| name: nuget-packages | ||
| path: '${{ inputs.project-directory }}/bin/Release/*.nupkg' | ||
| retention-days: 7 |
Contributor
There was a problem hiding this comment.
Artifact upload path doesn't account for working-directory.
The path is relative to the repository root, but project-directory is documented as relative to working-directory. When working-directory: 'Frends.Common.Toolkit' and project-directory: 'Frends.Common.Toolkit', the actual path should be Frends.Common.Toolkit/Frends.Common.Toolkit/bin/Release/*.nupkg.
🐛 Proposed fix
- name: Upload artifact
uses: actions/upload-artifact@v7.0.0
with:
name: nuget-packages
- path: '${{ inputs.project-directory }}/bin/Release/*.nupkg'
+ path: '${{ inputs.working-directory }}/${{ inputs.project-directory }}/bin/Release/*.nupkg'
retention-days: 7📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - name: Upload artifact | |
| uses: actions/upload-artifact@v7.0.0 | |
| with: | |
| name: nuget-packages | |
| path: '${{ inputs.project-directory }}/bin/Release/*.nupkg' | |
| retention-days: 7 | |
| - name: Upload artifact | |
| uses: actions/upload-artifact@v7.0.0 | |
| with: | |
| name: nuget-packages | |
| path: '${{ inputs.working-directory }}/${{ inputs.project-directory }}/bin/Release/*.nupkg' | |
| retention-days: 7 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/toolkit_test.yml around lines 64 - 69, The upload-artifact
path incorrectly assumes project-directory is repo-root-relative; update the
Upload artifact step to join working-directory and project-directory so the path
points to the built .nupkg under the actual working tree (e.g. use '${{
inputs.working-directory }}/${{ inputs.project-directory }}/bin/Release/*.nupkg'
or equivalent expression). Modify the step that uses
actions/upload-artifact@v7.0.0 (the "Upload artifact" step) to use the combined
path expression so artifacts are found when working-directory is set.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
README.md (1)
3-3:⚠️ Potential issue | 🟡 MinorHyphenate "Tasks-related" as compound adjective.
The phrase "Frends Tasks related things" should use a hyphen: "Frends Tasks-related things." This issue was previously flagged but remains unaddressed.
📝 Suggested fix
-This repository serves as a central place for sharing Frends Tasks related things. Currently, it means: +This repository serves as a central place for sharing Frends Tasks-related things. Currently, it means:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 3, Replace the phrase "Frends Tasks related things" in README.md with the hyphenated compound adjective "Frends Tasks-related things" so the sentence reads "This repository serves as a central place for sharing Frends Tasks-related things."; update the single occurrence in the document to ensure consistency with the compound-adjective rule.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@README.md`:
- Line 3: Replace the phrase "Frends Tasks related things" in README.md with the
hyphenated compound adjective "Frends Tasks-related things" so the sentence
reads "This repository serves as a central place for sharing Frends
Tasks-related things."; update the single occurrence in the document to ensure
consistency with the compound-adjective rule.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ebaa7f1f-4ef7-45e9-854b-dd6bc76268f4
📒 Files selected for processing (11)
.github/actions/dotnet-setup/action.yml.github/workflows/common_toolkit_release.yml.github/workflows/common_toolkit_test.yml.github/workflows/toolkit_release.yml.github/workflows/toolkit_test.ymlFrends.Common.Toolkit/Frends.Common.Toolkit.Tests/AttributesTests/NotEmptyStringAttributeTests.csFrends.Common.Toolkit/Frends.Common.Toolkit.Tests/AttributesTests/RequiredIfAttributeTests.csFrends.Common.Toolkit/Frends.Common.Toolkit.Tests/Frends.Common.Toolkit.Tests.csprojFrends.Common.Toolkit/Frends.Common.Toolkit.Tests/HandlersTests/ValidationHandlerTests.csFrends.Common.Toolkit/Frends.Common.Toolkit/Definitions/ITaskOptions.csREADME.md
✅ Files skipped from review due to trivial changes (7)
- .github/workflows/common_toolkit_test.yml
- .github/actions/dotnet-setup/action.yml
- .github/workflows/common_toolkit_release.yml
- Frends.Common.Toolkit/Frends.Common.Toolkit.Tests/Frends.Common.Toolkit.Tests.csproj
- Frends.Common.Toolkit/Frends.Common.Toolkit/Definitions/ITaskOptions.cs
- .github/workflows/toolkit_release.yml
- Frends.Common.Toolkit/Frends.Common.Toolkit.Tests/HandlersTests/ValidationHandlerTests.cs
🚧 Files skipped from review as they are similar to previous changes (3)
- Frends.Common.Toolkit/Frends.Common.Toolkit.Tests/AttributesTests/NotEmptyStringAttributeTests.cs
- .github/workflows/toolkit_test.yml
- Frends.Common.Toolkit/Frends.Common.Toolkit.Tests/AttributesTests/RequiredIfAttributeTests.cs
There was a problem hiding this comment.
Pull request overview
Introduces a new Frends.Common.Toolkit library with reusable validation helpers/attributes and standardized task result/error interfaces, plus CI workflows/actions to build, test, check coverage, and release the package.
Changes:
- Added
Frends.Common.Toolkitlibrary (validation handler, validation attributes, and task result/error option interfaces). - Added
Frends.Common.Toolkit.Testswith NUnit unit tests for validation behaviors. - Added reusable GitHub Actions workflows/composite actions for .NET setup, coverage checking, test/build, and release; plus repo hygiene updates (.gitignore/.gitattributes/README).
Reviewed changes
Copilot reviewed 21 out of 23 changed files in this pull request and generated 19 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Clarifies repo purpose and links to key directories. |
| Frends.Common.Toolkit/Frends.Common.Toolkit/Handlers/ValidationHandler.cs | Adds a central handler to run DataAnnotations validations across multiple objects. |
| Frends.Common.Toolkit/Frends.Common.Toolkit/Attributes/RequiredIfAttribute.cs | Adds conditional-required validation attribute. |
| Frends.Common.Toolkit/Frends.Common.Toolkit/Attributes/NotEmptyStringAttribute.cs | Adds a string-required (non-empty) validation attribute. |
| Frends.Common.Toolkit/Frends.Common.Toolkit/Definitions/ITaskResult.cs | Adds a standardized task result interface. |
| Frends.Common.Toolkit/Frends.Common.Toolkit/Definitions/ITaskOptions.cs | Adds default options interface (throw vs return failure result, etc.). |
| Frends.Common.Toolkit/Frends.Common.Toolkit/Definitions/ITaskError.cs | Adds standardized task error interface. |
| Frends.Common.Toolkit/Frends.Common.Toolkit/GlobalSuppressions.cs | Adds StyleCop suppressions aligned with existing Frends guidelines. |
| Frends.Common.Toolkit/Frends.Common.Toolkit/Frends.Common.Toolkit.csproj | Adds the toolkit project packaging/build configuration. |
| Frends.Common.Toolkit/Frends.Common.Toolkit.sln | Adds solution including toolkit + tests and workflow solution items. |
| Frends.Common.Toolkit/Frends.Common.Toolkit.Tests/Frends.Common.Toolkit.Tests.csproj | Adds test project configuration and dependencies. |
| Frends.Common.Toolkit/Frends.Common.Toolkit.Tests/HandlersTests/ValidationHandlerTests.cs | Adds unit tests for ValidationHandler behavior. |
| Frends.Common.Toolkit/Frends.Common.Toolkit.Tests/AttributesTests/AttributeTestBase.cs | Adds shared DataAnnotations validation helper for tests. |
| Frends.Common.Toolkit/Frends.Common.Toolkit.Tests/AttributesTests/RequiredIfAttributeTests.cs | Adds tests for RequiredIfAttribute scenarios. |
| Frends.Common.Toolkit/Frends.Common.Toolkit.Tests/AttributesTests/NotEmptyStringAttributeTests.cs | Adds tests for NotEmptyStringAttribute scenarios. |
| .gitignore | Ignores build artifacts and user files. |
| .gitattributes | Defines line ending normalization and file diff settings. |
| .github/workflows/toolkit_test.yml | Adds reusable workflow for lint/build/test/coverage + artifact upload. |
| .github/workflows/toolkit_release.yml | Adds reusable workflow for pack/push NuGet + GitHub release. |
| .github/workflows/common_toolkit_test.yml | Hooks toolkit_test reusable workflow to toolkit path changes. |
| .github/workflows/common_toolkit_release.yml | Hooks toolkit_release reusable workflow to main branch releases. |
| .github/actions/dotnet-setup/action.yml | Adds composite action to checkout/setup .NET/restore. |
| .github/actions/check-coverage/action.yml | Adds composite action to enforce Cobertura line coverage and optionally publish a badge. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+14
to
+19
| build: | ||
| uses: FrendsPlatform/FrendsTasks/.github/workflows/toolkit_test.yml@main | ||
| with: | ||
| working-directory: 'Frends.Common.Toolkit' | ||
| project-directory: 'Frends.Common.Toolkit' | ||
| secrets: inherit |
There was a problem hiding this comment.
Calling the reusable workflow via .../toolkit_test.yml@main means this workflow depends on what is currently on main (and will fail on this PR if main doesn’t yet contain toolkit_test.yml). For workflows within the same repo, prefer uses: ./.github/workflows/toolkit_test.yml so the called workflow matches the commit being built/tested.
Comment on lines
+5
to
+20
| /// <summary> | ||
| /// Object representing an error that should be returned when task execution failed. | ||
| /// Contains a message describing the error and an optional Exception object with additional information about the error. | ||
| /// </summary> | ||
| public interface ITaskError | ||
| { | ||
| /// <summary> | ||
| /// Message describing the error. | ||
| /// </summary> | ||
| string Message { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Additional information about the error. Usually it's an Exception that was thrown | ||
| /// </summary> | ||
| Exception AdditionalInfo { get; set; } | ||
| } |
There was a problem hiding this comment.
Docs describe AdditionalInfo as optional ("optional Exception"), but the property type is non-nullable Exception. Consider making it Exception? (and enabling nullable annotations) or update the docs/contract to state when it is guaranteed to be non-null.
Suggested change
| /// <summary> | |
| /// Object representing an error that should be returned when task execution failed. | |
| /// Contains a message describing the error and an optional Exception object with additional information about the error. | |
| /// </summary> | |
| public interface ITaskError | |
| { | |
| /// <summary> | |
| /// Message describing the error. | |
| /// </summary> | |
| string Message { get; set; } | |
| /// <summary> | |
| /// Additional information about the error. Usually it's an Exception that was thrown | |
| /// </summary> | |
| Exception AdditionalInfo { get; set; } | |
| } | |
| /// <summary> | |
| /// Object representing an error that should be returned when task execution failed. | |
| /// Contains a message describing the error and an optional Exception object with additional information about the error. | |
| /// </summary> | |
| public interface ITaskError | |
| { | |
| /// <summary> | |
| /// Message describing the error. | |
| /// </summary> | |
| string Message { get; set; } | |
| /// <summary> | |
| /// Optional additional information about the error. Usually it's an Exception that was thrown. | |
| /// Can be <c>null</c> if no additional information is available. | |
| /// </summary> | |
| Exception? AdditionalInfo { get; set; } | |
| } |
Comment on lines
+35
to
+39
| - name: Setup | ||
| uses: FrendsPlatform/FrendsTasks/.github/actions/dotnet-setup@main | ||
| with: | ||
| dotnet-version: ${{ inputs.dotnet-version }} | ||
| working-directory: ${{ inputs.working-directory }} |
There was a problem hiding this comment.
Using FrendsPlatform/FrendsTasks/.github/actions/dotnet-setup@main means this workflow will run against whatever is currently on main, not the commit under test. Since this PR adds/changes that action, CI can break or validate the wrong code. Prefer a local reference (./.github/actions/dotnet-setup) so the workflow uses the same revision.
Comment on lines
+63
to
+68
| - name: Upload artifact | ||
| uses: actions/upload-artifact@v7.0.0 | ||
| with: | ||
| name: nuget-packages | ||
| path: '${{ inputs.project-directory }}/bin/Release/*.nupkg' | ||
| retention-days: 7 |
There was a problem hiding this comment.
actions/upload-artifact is pinned to v7.0.0 here, while the rest of the repo pins actions/upload-artifact@v4.x (e.g. v4.6.2). Aligning versions reduces supply-chain surface area and avoids unexpected behavior differences across workflows.
Comment on lines
+14
to
+19
| build: | ||
| uses: FrendsPlatform/FrendsTasks/.github/workflows/toolkit_release.yml@main | ||
| with: | ||
| working-directory: 'Frends.Common.Toolkit' | ||
| project-directory: 'Frends.Common.Toolkit' | ||
| secrets: inherit |
There was a problem hiding this comment.
Same issue here: referencing toolkit_release.yml@main couples this workflow to the state of main rather than the current commit. If toolkit_release.yml is introduced/changed in this PR, this can break or release using mismatched logic. Use a local workflow reference (./.github/workflows/toolkit_release.yml) to keep revisions consistent.
Contributor
There was a problem hiding this comment.
Is this something we would need? Or are we only kind of want that this workflow is hard-tied to main?
Contributor
Author
There was a problem hiding this comment.
yes, it's the same approach we have in tasks - we need to call "global" workflows from repository as it will be in separate, system repository while "global" is in FrendsTask repository
| working-directory: ${{ inputs.working-directory }} | ||
| steps: | ||
| - name: Setup | ||
| uses: FrendsPlatform/FrendsTasks/.github/actions/dotnet-setup@main |
There was a problem hiding this comment.
Using FrendsPlatform/FrendsTasks/.github/actions/dotnet-setup@main can make releases run with an action revision different from the workflow revision (and will fail on this PR if main doesn’t yet include the new action). Prefer ./.github/actions/dotnet-setup (or pin to the same ref) so the workflow and action stay in sync.
Suggested change
| uses: FrendsPlatform/FrendsTasks/.github/actions/dotnet-setup@main | |
| uses: ./.github/actions/dotnet-setup |
jefim
requested changes
Mar 30, 2026
Contributor
jefim
left a comment
jefim
left a comment
There was a problem hiding this comment.
Please review the copilot comment about those workflows. Plus some additional minor comments in general.
Comment on lines
+20
to
+23
| uses: actions/checkout@v6.0.2 | ||
|
|
||
| - name: Setup .NET SDK | ||
| uses: actions/setup-dotnet@v5.2.0 |
Comment on lines
+14
to
+19
| build: | ||
| uses: FrendsPlatform/FrendsTasks/.github/workflows/toolkit_release.yml@main | ||
| with: | ||
| working-directory: 'Frends.Common.Toolkit' | ||
| project-directory: 'Frends.Common.Toolkit' | ||
| secrets: inherit |
Contributor
There was a problem hiding this comment.
Is this something we would need? Or are we only kind of want that this workflow is hard-tied to main?
Comment on lines
+13
to
+16
| uses: FrendsPlatform/FrendsTasks/.github/workflows/toolkit_test.yml@main | ||
| with: | ||
| working-directory: 'Frends.Common.Toolkit' | ||
| project-directory: 'Frends.Common.Toolkit' |
Contributor
There was a problem hiding this comment.
Probably a good idea to only have read-only
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
FrendsToolkitTemplate/Template/.github/workflows/Echo_toolkit_test.yml (1)
15-15: Pin the reusable workflow to an immutable ref.Using
@mainis mutable and can introduce unexpected CI behavior or security drift. Prefer a commit SHA or a controlled release tag.Suggested change
- uses: FrendsPlatform/FrendsTasks/.github/workflows/toolkit_test.yml@main + uses: FrendsPlatform/FrendsTasks/.github/workflows/toolkit_test.yml@<commit-sha>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@FrendsToolkitTemplate/Template/.github/workflows/Echo_toolkit_test.yml` at line 15, Replace the mutable reference in the reusable workflow declaration in Echo_toolkit_test.yml (the line using FrendsPlatform/FrendsTasks/.github/workflows/toolkit_test.yml@main) with an immutable ref: either a specific commit SHA or a released tag (e.g., `@v1.2.3` or @<commit-sha>); update the uses entry accordingly and, if appropriate, add a comment documenting the chosen tag/sha and why it was selected..github/workflows/template_toolkit_release.yml (2)
5-7: Consider scoping release publishing to explicit refs.Line 6 triggers on every push, which will publish for all branches/tags. If this is a release workflow, restrict refs to avoid accidental feed pollution.
Possible refinement
on: push: + branches: + - main + - 'release/**'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/template_toolkit_release.yml around lines 5 - 7, The workflow currently triggers on every push due to the "on: push" event; restrict it to explicit refs by changing the push trigger to include only intended branches/tags (e.g., replace "push:" with "push: branches: [main] tags: ['v*']" or similar) so releases only publish for designated refs; update the "on" block in the workflow YAML (the "on:" / "push" keys) accordingly.
18-23: Use major version tags (v6,v5) instead of specific patch versions for GitHub Actions.Both
actions/checkout@v6.0.1andactions/setup-dotnet@v5.0.1tags are valid and exist. However, the recommended approach per GitHub best practices is to use major version tags (actions/checkout@v6andactions/setup-dotnet@v5). This allows automatic updates to the latest patch versions within those major versions while maintaining stability. The current patch versions specified are outdated (newer patches available: v6.0.2 for checkout, v5.2.0 for setup-dotnet).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/template_toolkit_release.yml around lines 18 - 23, Replace the hard-coded patch tags with major-version tags to allow safe patch upgrades: change the uses entries for actions/checkout@v6.0.1 to actions/checkout@v6 and actions/setup-dotnet@v5.0.1 to actions/setup-dotnet@v5 (the "Pack release version" step remains unchanged); update the two uses lines referencing actions/checkout and actions/setup-dotnet accordingly so the workflow picks up the latest patch releases within those major versions.FrendsToolkitTemplate/Template/Company.Echo.Toolkit/Company.Echo.Toolkit/Handlers/ValidationHandler.cs (1)
20-21: Replace aggregate string concatenation withstring.Concat.Current aggregation repeatedly allocates intermediate strings.
string.Concatkeeps behavior and simplifies the code.♻️ Suggested update
- var validationMessage = objects.Select(obj => obj.Validate()) - .Aggregate(string.Empty, (current, message) => current + message); + var validationMessage = string.Concat(objects.Select(obj => obj.Validate())); @@ - return validateResults.Aggregate(string.Empty, (current, error) => current + $"{error.ErrorMessage}\n"); + return string.Concat(validateResults.Select(error => $"{error.ErrorMessage}\n"));Also applies to: 34-34
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@FrendsToolkitTemplate/Template/Company.Echo.Toolkit/Company.Echo.Toolkit/Handlers/ValidationHandler.cs` around lines 20 - 21, The current aggregation that builds validationMessage uses LINQ.Aggregate to concatenate strings (var validationMessage = objects.Select(obj => obj.Validate()).Aggregate(...)) which causes repeated intermediate allocations; replace that Aggregate+Select pattern with string.Concat(objects.Select(obj => obj.Validate())) to preserve behavior and simplify the code, and apply the same change to the second occurrence that builds the other validation string in ValidationHandler.cs (the other spot at the indicated range).FrendsToolkitTemplate/Template/Company.Echo.Toolkit/Company.Echo.Toolkit.Tests/AttributesTests/NotEmptyStringAttributeTests.cs (1)
34-35: Assert result count before reading the first validation message.Using
First()here can fail with an unhelpful exception. Explicit count assertion gives clearer failures.♻️ Suggested update
var res = Validate(test); - Assert.That(res.First().ErrorMessage, Contains.Substring("Name is required and cannot be empty.")); + Assert.That(res, Has.Count.EqualTo(1)); + Assert.That(res[0].ErrorMessage, Contains.Substring("Name is required and cannot be empty.")); @@ var res = Validate(test); - Assert.That(res.First().ErrorMessage, Contains.Substring("Name is required and cannot be empty.")); + Assert.That(res, Has.Count.EqualTo(1)); + Assert.That(res[0].ErrorMessage, Contains.Substring("Name is required and cannot be empty.")); @@ var res = Validate(test); - Assert.That(res.First().ErrorMessage, Contains.Substring("Name is required and cannot be empty.")); + Assert.That(res, Has.Count.EqualTo(1)); + Assert.That(res[0].ErrorMessage, Contains.Substring("Name is required and cannot be empty."));Also applies to: 45-46, 53-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@FrendsToolkitTemplate/Template/Company.Echo.Toolkit/Company.Echo.Toolkit.Tests/AttributesTests/NotEmptyStringAttributeTests.cs` around lines 34 - 35, The tests in NotEmptyStringAttributeTests access validation messages with res.First() which can throw unhelpful exceptions; update each test (the assertions around res.First().ErrorMessage at the locations shown and the similar checks at lines handling res.First() around lines 45-46 and 53-54) to first assert the expected collection size (e.g., Assert.That(res.Count, Is.EqualTo(1)) or the appropriate expected count) and only then read the first element to assert its ErrorMessage; keep the existing ErrorMessage assertion but precede it with the explicit count assertion to produce clearer failures.FrendsToolkitTemplate/Template/Company.Echo.Toolkit/Company.Echo.Toolkit/Attributes/RequiredIfAttribute.cs (1)
19-20: IncludeMemberNameinValidationResultfor property-level error mapping.Both failure paths return
ValidationResultwithout member metadata, so consumers can lose field-level association.♻️ Suggested update
if (field == null) - return new ValidationResult($"Unknown property: {dependentProperty}"); + return new ValidationResult( + $"Unknown property: {dependentProperty}", + validationContext.MemberName is null ? null : [validationContext.MemberName]); @@ if (value == null || (value is string s && string.IsNullOrWhiteSpace(s))) { - return new ValidationResult(ErrorMessage ?? $"{validationContext.DisplayName} is required."); + return new ValidationResult( + ErrorMessage ?? $"{validationContext.DisplayName} is required.", + validationContext.MemberName is null ? null : [validationContext.MemberName]); }Also applies to: 28-29
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@FrendsToolkitTemplate/Template/Company.Echo.Toolkit/Company.Echo.Toolkit/Attributes/RequiredIfAttribute.cs` around lines 19 - 20, The ValidationResult returned when the dependent property is missing (and the other failure path at lines 28-29) lacks member metadata, so update the two new ValidationResult(...) calls in RequiredIfAttribute (the method handling validation for dependentProperty) to include the offending property name in the MemberNames collection (e.g., pass new[] { dependentProperty } or field.Name as the second argument) so consumers can map the error to the correct property; keep the existing message text while adding the member name array to both return sites.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/template_toolkit_release.yml:
- Around line 20-29: The pack step "Pack release version" is running before the
SDK/feed auth is configured by the "actions/setup-dotnet@v5.0.1" step, causing
nondeterministic restores; move the actions/setup-dotnet@v5.0.1 step (with its
dotnet-version and source-url inputs and NUGET_AUTH_TOKEN env) to run before the
"Pack release version" step so the intended SDK and feed auth are initialized
prior to running dotnet pack, and ensure the "Pack release version" step
inherits the NUGET_AUTH_TOKEN environment variable.
- Around line 31-37: The workflow's dotnet nuget push invocation assumes a
specific relative path and lowercase filename
(bin/Release/frendstasktemplate.*.nupkg) which can fail on case-sensitive
filesystems; change the job to discover packages dynamically (e.g., using find
with case-insensitive name matching or a glob that searches any project
subfolder like */bin/Release/**/* and -iname '*frendstasktemplate*.nupkg'),
iterate over all discovered .nupkg files and call dotnet nuget push for each
using the existing ARGS variable so multiple matches and varying casing/paths
are handled robustly.
In `@FrendsToolkitTemplate/FrendsToolkitTemplate.csproj`:
- Around line 7-9: The NuGet package metadata contains typos in the XML elements
Title and Description; update the <Title> value from "Frends Task Tookit
template" to "Frends Task Toolkit template" and update the <Description> value
from "Template for creating tookit for Frends Tasks per System." to "Template
for creating toolkit for Frends Tasks per System." so the package metadata shows
the correct "Toolkit" spelling in both Title and Description.
In
`@FrendsToolkitTemplate/Template/Company.Echo.Toolkit/Company.Echo.Toolkit/Definitions/ITaskResult.cs`:
- Around line 15-18: The XML docs say Error can be null but the property and
generic constraint don't allow it; update the ITaskResult<T, TError> declaration
so TError is constrained to a reference type (change the generic constraint from
"where TError : ITaskError" to "where TError : class, ITaskError") and make the
Error property nullable (change the Error property on ITaskResult to use TError?
for its type) so the signature matches the documentation; refer to the
ITaskResult interface, the Error property, and the generic constraint on TError
when making the changes.
In `@README.md`:
- Around line 5-8: Replace the root-relative links in README entries for
[Workflows], [Analyzers], [Toolkit Template], and [Task Template] so they are
repo-relative (e.g., prefix with ./ or remove the leading slash) instead of
starting with "/", updating the link targets like "/.github/workflows" ->
"./.github/workflows" and similarly for "/FrendsTaskAnalyzers",
"/FrendsToolkitTemplate", and "/FrendsTaskTemplate" to ensure links resolve
within the repository.
---
Nitpick comments:
In @.github/workflows/template_toolkit_release.yml:
- Around line 5-7: The workflow currently triggers on every push due to the "on:
push" event; restrict it to explicit refs by changing the push trigger to
include only intended branches/tags (e.g., replace "push:" with "push: branches:
[main] tags: ['v*']" or similar) so releases only publish for designated refs;
update the "on" block in the workflow YAML (the "on:" / "push" keys)
accordingly.
- Around line 18-23: Replace the hard-coded patch tags with major-version tags
to allow safe patch upgrades: change the uses entries for
actions/checkout@v6.0.1 to actions/checkout@v6 and actions/setup-dotnet@v5.0.1
to actions/setup-dotnet@v5 (the "Pack release version" step remains unchanged);
update the two uses lines referencing actions/checkout and actions/setup-dotnet
accordingly so the workflow picks up the latest patch releases within those
major versions.
In `@FrendsToolkitTemplate/Template/.github/workflows/Echo_toolkit_test.yml`:
- Line 15: Replace the mutable reference in the reusable workflow declaration in
Echo_toolkit_test.yml (the line using
FrendsPlatform/FrendsTasks/.github/workflows/toolkit_test.yml@main) with an
immutable ref: either a specific commit SHA or a released tag (e.g., `@v1.2.3` or
@<commit-sha>); update the uses entry accordingly and, if appropriate, add a
comment documenting the chosen tag/sha and why it was selected.
In
`@FrendsToolkitTemplate/Template/Company.Echo.Toolkit/Company.Echo.Toolkit.Tests/AttributesTests/NotEmptyStringAttributeTests.cs`:
- Around line 34-35: The tests in NotEmptyStringAttributeTests access validation
messages with res.First() which can throw unhelpful exceptions; update each test
(the assertions around res.First().ErrorMessage at the locations shown and the
similar checks at lines handling res.First() around lines 45-46 and 53-54) to
first assert the expected collection size (e.g., Assert.That(res.Count,
Is.EqualTo(1)) or the appropriate expected count) and only then read the first
element to assert its ErrorMessage; keep the existing ErrorMessage assertion but
precede it with the explicit count assertion to produce clearer failures.
In
`@FrendsToolkitTemplate/Template/Company.Echo.Toolkit/Company.Echo.Toolkit/Attributes/RequiredIfAttribute.cs`:
- Around line 19-20: The ValidationResult returned when the dependent property
is missing (and the other failure path at lines 28-29) lacks member metadata, so
update the two new ValidationResult(...) calls in RequiredIfAttribute (the
method handling validation for dependentProperty) to include the offending
property name in the MemberNames collection (e.g., pass new[] {
dependentProperty } or field.Name as the second argument) so consumers can map
the error to the correct property; keep the existing message text while adding
the member name array to both return sites.
In
`@FrendsToolkitTemplate/Template/Company.Echo.Toolkit/Company.Echo.Toolkit/Handlers/ValidationHandler.cs`:
- Around line 20-21: The current aggregation that builds validationMessage uses
LINQ.Aggregate to concatenate strings (var validationMessage =
objects.Select(obj => obj.Validate()).Aggregate(...)) which causes repeated
intermediate allocations; replace that Aggregate+Select pattern with
string.Concat(objects.Select(obj => obj.Validate())) to preserve behavior and
simplify the code, and apply the same change to the second occurrence that
builds the other validation string in ValidationHandler.cs (the other spot at
the indicated range).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 19c4591a-32b6-426b-b5ce-518071173606
📒 Files selected for processing (22)
.github/workflows/template_toolkit_release.ymlFrendsToolkitTemplate/.gitignoreFrendsToolkitTemplate/FrendsToolkitTemplate.csprojFrendsToolkitTemplate/README.mdFrendsToolkitTemplate/Template/.github/workflows/Echo_toolkit_release.ymlFrendsToolkitTemplate/Template/.github/workflows/Echo_toolkit_test.ymlFrendsToolkitTemplate/Template/.template.config/template.jsonFrendsToolkitTemplate/Template/Company.Echo.Toolkit/Company.Echo.Toolkit.Tests/AttributesTests/AttributeTestBase.csFrendsToolkitTemplate/Template/Company.Echo.Toolkit/Company.Echo.Toolkit.Tests/AttributesTests/NotEmptyStringAttributeTests.csFrendsToolkitTemplate/Template/Company.Echo.Toolkit/Company.Echo.Toolkit.Tests/AttributesTests/RequiredIfAttributeTests.csFrendsToolkitTemplate/Template/Company.Echo.Toolkit/Company.Echo.Toolkit.Tests/Company.Echo.Toolkit.Tests.csprojFrendsToolkitTemplate/Template/Company.Echo.Toolkit/Company.Echo.Toolkit.Tests/HandlersTests/ValidationHandlerTests.csFrendsToolkitTemplate/Template/Company.Echo.Toolkit/Company.Echo.Toolkit.slnFrendsToolkitTemplate/Template/Company.Echo.Toolkit/Company.Echo.Toolkit/Attributes/NotEmptyStringAttribute.csFrendsToolkitTemplate/Template/Company.Echo.Toolkit/Company.Echo.Toolkit/Attributes/RequiredIfAttribute.csFrendsToolkitTemplate/Template/Company.Echo.Toolkit/Company.Echo.Toolkit/Company.Echo.Toolkit.csprojFrendsToolkitTemplate/Template/Company.Echo.Toolkit/Company.Echo.Toolkit/Definitions/ITaskError.csFrendsToolkitTemplate/Template/Company.Echo.Toolkit/Company.Echo.Toolkit/Definitions/ITaskOptions.csFrendsToolkitTemplate/Template/Company.Echo.Toolkit/Company.Echo.Toolkit/Definitions/ITaskResult.csFrendsToolkitTemplate/Template/Company.Echo.Toolkit/Company.Echo.Toolkit/GlobalSuppressions.csFrendsToolkitTemplate/Template/Company.Echo.Toolkit/Company.Echo.Toolkit/Handlers/ValidationHandler.csREADME.md
✅ Files skipped from review due to trivial changes (10)
- FrendsToolkitTemplate/README.md
- FrendsToolkitTemplate/Template/.github/workflows/Echo_toolkit_release.yml
- FrendsToolkitTemplate/Template/.template.config/template.json
- FrendsToolkitTemplate/Template/Company.Echo.Toolkit/Company.Echo.Toolkit/Definitions/ITaskError.cs
- FrendsToolkitTemplate/Template/Company.Echo.Toolkit/Company.Echo.Toolkit.Tests/AttributesTests/RequiredIfAttributeTests.cs
- FrendsToolkitTemplate/Template/Company.Echo.Toolkit/Company.Echo.Toolkit.sln
- FrendsToolkitTemplate/Template/Company.Echo.Toolkit/Company.Echo.Toolkit/Company.Echo.Toolkit.csproj
- FrendsToolkitTemplate/.gitignore
- FrendsToolkitTemplate/Template/Company.Echo.Toolkit/Company.Echo.Toolkit/Definitions/ITaskOptions.cs
- FrendsToolkitTemplate/Template/Company.Echo.Toolkit/Company.Echo.Toolkit/GlobalSuppressions.cs
Comment on lines
+31
to
+37
| run: | | ||
| ARGS="--api-key ${{ env.nuget_api_key }} --source ${{ env.azure_artifacts_feed_url }}" | ||
| if [ "${{ env.skip_duplicate }}" = "true" ]; then | ||
| ARGS="$ARGS --skip-duplicate" | ||
| fi | ||
| dotnet nuget push bin/Release/frendstasktemplate.*.nupkg $ARGS | ||
|
|
Contributor
There was a problem hiding this comment.
Make package push discovery resilient to path and casing differences.
Line 36 assumes packages are at bin/Release/ with lowercase frendstasktemplate.*.nupkg. On Linux, this is case-sensitive and may miss actual outputs (<project>/bin/Release/...), causing push failures.
Proposed fix
- name: Push NuGet package(s)
run: |
ARGS="--api-key ${{ env.nuget_api_key }} --source ${{ env.azure_artifacts_feed_url }}"
if [ "${{ env.skip_duplicate }}" = "true" ]; then
ARGS="$ARGS --skip-duplicate"
fi
- dotnet nuget push bin/Release/frendstasktemplate.*.nupkg $ARGS
+ mapfile -t PACKAGES < <(find . -type f -path '*/bin/Release/*.nupkg' ! -name '*.symbols.nupkg')
+ if [ ${`#PACKAGES`[@]} -eq 0 ]; then
+ echo "No NuGet packages found to push."
+ exit 1
+ fi
+ for pkg in "${PACKAGES[@]}"; do
+ dotnet nuget push "$pkg" $ARGS
+ done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/template_toolkit_release.yml around lines 31 - 37, The
workflow's dotnet nuget push invocation assumes a specific relative path and
lowercase filename (bin/Release/frendstasktemplate.*.nupkg) which can fail on
case-sensitive filesystems; change the job to discover packages dynamically
(e.g., using find with case-insensitive name matching or a glob that searches
any project subfolder like */bin/Release/**/* and -iname
'*frendstasktemplate*.nupkg'), iterate over all discovered .nupkg files and call
dotnet nuget push for each using the existing ARGS variable so multiple matches
and varying casing/paths are handled robustly.
Comment on lines
+15
to
+18
| /// <summary> | ||
| /// Error object containing information about the error if the task failed. Null if the task was successful. | ||
| /// </summary> | ||
| TError Error { get; set; } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify nullable context and existing ITaskResult implementations.
echo "== Nullable settings =="
rg -n --iglob '*.csproj' '<Nullable>\s*(enable|annotations)\s*</Nullable>'
echo
echo "== ITaskResult interface and implementations =="
rg -nP --type=cs 'interface\s+ITaskResult<|:\s*ITaskResult<[^>]+>'
echo
echo "== Error property declarations around result types =="
rg -nP --type=cs '\b(Error|Success)\b\s*\{'Repository: FrendsPlatform/FrendsTasks
Length of output: 3407
🏁 Script executed:
cat -n FrendsToolkitTemplate/Template/Company.Echo.Toolkit/Company.Echo.Toolkit/Definitions/ITaskResult.csRepository: FrendsPlatform/FrendsTasks
Length of output: 767
🏁 Script executed:
rg -n '<Nullable>' FrendsToolkitTemplate/Template/Company.Echo.Toolkit/Company.Echo.Toolkit/Company.Echo.Toolkit.csprojRepository: FrendsPlatform/FrendsTasks
Length of output: 109
🏁 Script executed:
rg -nP --type=cs 'class\s+\w+.*:\s*ITaskResult<[^>]+>' -A 5Repository: FrendsPlatform/FrendsTasks
Length of output: 52
🏁 Script executed:
rg -nP --type=cs ':\s*ITaskResult<' -B 2 -A 5Repository: FrendsPlatform/FrendsTasks
Length of output: 52
🏁 Script executed:
rg -n "ITaskResult<" --type=cs -A 3Repository: FrendsPlatform/FrendsTasks
Length of output: 565
🏁 Script executed:
fd --type f --extension cs -x rg -l "ITaskResult" {} \;Repository: FrendsPlatform/FrendsTasks
Length of output: 171
🏁 Script executed:
rg -n "interface ITaskError" --type=cs -A 5Repository: FrendsPlatform/FrendsTasks
Length of output: 818
Error property type contradicts its XML documentation.
The XML docs explicitly state "Null if the task was successful" (line 16), but the property is declared as non-nullable TError (line 18). Additionally, the generic constraint where TError : ITaskError does not restrict TError to reference types, making nullable syntax invalid. This inconsistency between contract documentation and type signature should be fixed.
Suggested fix
public interface ITaskResult<TError>
- where TError : ITaskError
+ where TError : class, ITaskError
{
/// <summary>
/// Error object containing information about the error if the task failed. Null if the task was successful.
/// </summary>
- TError Error { get; set; }
+ TError? Error { get; set; }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@FrendsToolkitTemplate/Template/Company.Echo.Toolkit/Company.Echo.Toolkit/Definitions/ITaskResult.cs`
around lines 15 - 18, The XML docs say Error can be null but the property and
generic constraint don't allow it; update the ITaskResult<T, TError> declaration
so TError is constrained to a reference type (change the generic constraint from
"where TError : ITaskError" to "where TError : class, ITaskError") and make the
Error property nullable (change the Error property on ITaskResult to use TError?
for its type) so the signature matches the documentation; refer to the
ITaskResult interface, the Error property, and the generic constraint on TError
when making the changes.
Comment on lines
+5
to
+8
| - [Workflows](/.github/workflows) – GitHub Actions workflows files used by all Frends Tasks. | ||
| - [Analyzers](/FrendsTaskAnalyzers) - Coding standards analyzers for Frends Tasks. | ||
| - [Toolkit Template](/FrendsToolkitTemplate) – Template to create a Toolkit solution for reusable code in a system repository. | ||
| - [Task Template](/FrendsTaskTemplate) – Template to create a new Frends Task solution. |
Contributor
There was a problem hiding this comment.
Use repo-relative links instead of root-relative links in README.
On Lines 5-8, links starting with / can resolve outside this repository on GitHub. Use ./... (or no leading slash) for reliable navigation.
Proposed fix
-- [Workflows](/.github/workflows) – GitHub Actions workflows files used by all Frends Tasks.
-- [Analyzers](/FrendsTaskAnalyzers) - Coding standards analyzers for Frends Tasks.
-- [Toolkit Template](/FrendsToolkitTemplate) – Template to create a Toolkit solution for reusable code in a system repository.
-- [Task Template](/FrendsTaskTemplate) – Template to create a new Frends Task solution.
+- [Workflows](./.github/workflows) – GitHub Actions workflow files used by all Frends Tasks.
+- [Analyzers](./FrendsTaskAnalyzers) – Coding standards analyzers for Frends Tasks.
+- [Toolkit Template](./FrendsToolkitTemplate) – Template to create a Toolkit solution for reusable code in a system repository.
+- [Task Template](./FrendsTaskTemplate) – Template to create a new Frends Task solution.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - [Workflows](/.github/workflows) – GitHub Actions workflows files used by all Frends Tasks. | |
| - [Analyzers](/FrendsTaskAnalyzers) - Coding standards analyzers for Frends Tasks. | |
| - [Toolkit Template](/FrendsToolkitTemplate) – Template to create a Toolkit solution for reusable code in a system repository. | |
| - [Task Template](/FrendsTaskTemplate) – Template to create a new Frends Task solution. | |
| - [Workflows](./.github/workflows) – GitHub Actions workflow files used by all Frends Tasks. | |
| - [Analyzers](./FrendsTaskAnalyzers) – Coding standards analyzers for Frends Tasks. | |
| - [Toolkit Template](./FrendsToolkitTemplate) – Template to create a Toolkit solution for reusable code in a system repository. | |
| - [Task Template](./FrendsTaskTemplate) – Template to create a new Frends Task solution. |
🧰 Tools
🪛 LanguageTool
[uncategorized] ~5-~5: The official name of this software platform is spelled with a capital “H”.
Context: ...ings. Currently, it means: - Workflows – GitHub Actions workflows f...
(GITHUB)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 5 - 8, Replace the root-relative links in README
entries for [Workflows], [Analyzers], [Toolkit Template], and [Task Template] so
they are repo-relative (e.g., prefix with ./ or remove the leading slash)
instead of starting with "/", updating the link targets like
"/.github/workflows" -> "./.github/workflows" and similarly for
"/FrendsTaskAnalyzers", "/FrendsToolkitTemplate", and "/FrendsTaskTemplate" to
ensure links resolve within the repository.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (2)
README.md (2)
5-9:⚠️ Potential issue | 🟡 MinorUse repo-relative links for reliable GitHub navigation.
Lines 5–9 still use root-relative paths (
/...), which can resolve unexpectedly outside this repo context.Proposed fix
-- [GitHub Workflows](/.github/workflows) – GitHub workflows files used by all Frends Tasks and templates. -- [GitHub Actions](/.github/actions) – GitHub Actions reused withing GitHub workflows. -- [Analyzers](/FrendsTaskAnalyzers) - Coding standards analyzers for Frends Tasks. -- [Toolkit Template](/FrendsToolkitTemplate) – Template to create a Toolkit solution for reusable code in a system repository. -- [Task Template](/FrendsTaskTemplate) – Template to create a new Frends Task solution. +- [GitHub Workflows](./.github/workflows) – GitHub workflow files used by all Frends Tasks and templates. +- [GitHub Actions](./.github/actions) – GitHub Actions reused within GitHub workflows. +- [Analyzers](./FrendsTaskAnalyzers) - Coding standards analyzers for Frends Tasks. +- [Toolkit Template](./FrendsToolkitTemplate) – Template to create a Toolkit solution for reusable code in a system repository. +- [Task Template](./FrendsTaskTemplate) – Template to create a new Frends Task solution.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 5 - 9, The README link list uses root-relative paths (leading "/") which can break outside the repo; update each link entry (the GitHub Workflows, GitHub Actions, Analyzers, Toolkit Template, Task Template entries in README.md) to use repo-relative/relative paths without the leading slash (e.g., remove the leading "/" so links point to .github/workflows, .github/actions, FrendsTaskAnalyzers, FrendsToolkitTemplate, FrendsTaskTemplate) to ensure reliable navigation within the repository.
3-3:⚠️ Potential issue | 🟡 MinorHyphenate compound adjective in the intro sentence.
Line 3 should use “Tasks-related” for correct compound adjective style.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 3, Update the intro sentence string "This repository serves as a central place for sharing Frends Tasks related things." to hyphenate the compound adjective by changing "Tasks related" to "Tasks-related" so the sentence reads "This repository serves as a central place for sharing Frends Tasks-related things."
🧹 Nitpick comments (3)
FrendsTaskTemplate/Frends.Template/Frends.Echo.Execute/Frends.Echo.Execute/Definitions/Error.cs (1)
20-20: Resolve or track the TODO before release.Line 20 indicates unfinished design for task-specific error details. I can help draft a strongly typed error metadata model and wire it through
Result.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@FrendsTaskTemplate/Frends.Template/Frends.Echo.Execute/Frends.Echo.Execute/Definitions/Error.cs` at line 20, Replace the TODO in Error.cs with a concrete, strongly-typed error metadata model and wire it into the existing Result type: define a class (e.g., ErrorDetails or TaskError) in Error.cs containing fields you need for diagnostics (Code, Message, Details, Hint, Timestamp), add a property of that class to the Result type (e.g., Result.ErrorDetails or Result.TaskError), update any places that construct Result to populate the new property, and ensure the class is serializable/annotated consistently with existing error handling so consumers receive structured error metadata instead of a TODO comment.FrendsTaskTemplate/Frends.Template/Frends.Echo.Execute/Frends.Echo.Execute.Tests/TestBase.cs (1)
11-16: Move environment loading to one-time setup and makeSecretKeynullable.Constructor calls
DotEnv.Load()on every test instance, which is inefficient churn (idempotent but unnecessary per-fixture). More importantly,SecretKeyassigns a nullable value fromEnvironment.GetEnvironmentVariable()into a non-nullablestringproperty. While the project disables nullable reference types, this creates a runtime null-dereference risk if the environment variable is absent. The TODO comment suggests this is template code; either remove unusedSecretKey, or refactor as shown below:Proposed refactor
+ [OneTimeSetUp] + public static void OneTimeSetup() + { + DotEnv.Load(); + } + protected TestBase() { - // TODO: Here you can load environment variables used in tests - DotEnv.Load(); - SecretKey = Environment.GetEnvironmentVariable("FRENDS_SECRET_KEY"); + SecretKey = Environment.GetEnvironmentVariable("FRENDS_SECRET_KEY"); } // TODO: Replace with your secret key or remove if not needed - protected string SecretKey { get; } + protected string? SecretKey { get; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@FrendsTaskTemplate/Frends.Template/Frends.Echo.Execute/Frends.Echo.Execute.Tests/TestBase.cs` around lines 11 - 16, Move the per-instance call to DotEnv.Load() out of the TestBase constructor and into a one-time test setup (e.g., a static initializer or a [OneTimeSetUp] method) so environment is loaded only once; also change the SecretKey property to be nullable (string?) or remove it if unused and update any consumers to handle null, locating the symbol SecretKey in TestBase and the DotEnv.Load() invocation to make the change.FrendsTaskTemplate/Frends.Template/Frends.Echo.Execute/Frends.Echo.Execute/Definitions/Options.cs (1)
15-17: InitializeDelimiterat runtime, not only via metadata.
[DefaultValue(" ")]is metadata only; it does not set the property value on object creation. Add an initializer soDelimiteris consistently non-null.♻️ Suggested change
[DisplayFormat(DataFormatString = "Text")] [DefaultValue(" ")] -public string Delimiter { get; set; } +public string Delimiter { get; set; } = " ";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@FrendsTaskTemplate/Frends.Template/Frends.Echo.Execute/Frends.Echo.Execute/Definitions/Options.cs` around lines 15 - 17, The Delimiter property in Options.cs is only annotated with [DefaultValue(" ")] which is metadata and doesn't set the runtime value; initialize the property directly so it isn't null at construction by giving Delimiter a default value (e.g., set Delimiter = " ") in its declaration or constructor in the Options class so consumers reading Options.Delimiter always get a non-null string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/template_task_release.yml:
- Around line 22-38: The workflow runs dotnet pack before the .NET SDK is
installed and omits the project path and explicit output, causing pack to fail;
reorder to run actions/setup-dotnet@v5.0.1 before any dotnet commands, run
dotnet pack against the FrendsTaskTemplate/FrendsTaskTemplate.csproj (or the
correct project file) with an explicit --configuration Release and --output path
(so artifacts are predictable), then run the existing push step (dotnet nuget
push using ARGS and the --skip-duplicate conditional) after a successful pack;
ensure the ARGS and environment variables (nuget_api_key,
azure_artifacts_feed_url, skip_duplicate) remain unchanged.
In
`@FrendsTaskTemplate/Frends.Template/Frends.Echo.Execute/Frends.Echo.Execute/Definitions/Error.cs`:
- Line 21: The public result type exposes a raw Exception via the AdditionalInfo
property on the Error class; replace that property with a safe, serializable
error DTO (e.g., ErrorDetails/ErrorInfo) or at minimum a string
ErrorMessage/ErrorCode to avoid leaking stack traces, and update any code that
sets Error.AdditionalInfo (handler/mapper) to map the original Exception into
the sanitized DTO (message, code, timestamp) rather than assigning the Exception
instance directly; ensure the new type is used throughout the handler and tests
so consumers receive predictable, non-sensitive error data.
In
`@FrendsTaskTemplate/Frends.Template/Frends.Echo.Execute/Frends.Echo.Execute/Definitions/Input.cs`:
- Around line 16-24: The [DefaultValue] attributes on the Input class do not set
runtime values for the Content and Repeat properties; add actual runtime
defaults by either initializing the properties (e.g., public string Content {
get; set; } = "Lorem ipsum dolor sit amet."; public int Repeat { get; set; } =
3;) or by adding a parameterless constructor for the Input class that assigns
Content = "Lorem ipsum dolor sit amet." and Repeat = 3 so instances have the
intended defaults at runtime.
In
`@FrendsTaskTemplate/Frends.Template/Frends.Echo.Execute/Frends.Echo.Execute/Frends.Echo.Execute.cs`:
- Around line 25-55: The catch block can throw a NullReferenceException if the
parameter options is null because it dereferences options.ThrowErrorOnFailure
and options.ErrorMessageOnFailure; fix by ensuring safe access to options in the
Execute method: either capture required flags/values (e.g., a local bool
throwErrorOnFailure and string errorMessageOnFailure) before entering the try,
or null-coalesce default values inside the catch, then call
ErrorHandler.Handle(ex, throwErrorOnFailure, errorMessageOnFailure); update
references to options in the catch to use these safe locals and ensure
ErrorHandler.Handle is always called with non-null arguments.
In
`@FrendsTaskTemplate/Frends.Template/Frends.Echo.Execute/Frends.Echo.Execute/Helpers/ErrorHandler.cs`:
- Around line 13-15: Replace the direct wraps that lose original exception
type/stack: for the plain rethrow that currently does "throw new
Exception(exception.Message, exception);" use
ExceptionDispatchInfo.Capture(exception).Throw() (and add a return/throw after
it as needed since Throw() does not return) to preserve the original exception
type and stack; for the case that must carry a custom message instead of
wrapping with "throw new Exception(errorMessageOnFailure, exception);", attach
the message to the original exception (e.g.
exception.Data["ErrorMessageOnFailure"]=errorMessageOnFailure) and then call
ExceptionDispatchInfo.Capture(exception).Throw() so the custom info is available
but the original exception/type/stack are preserved (alternatively, if you must
wrap, add a comment documenting why wrapping is required). Use the existing
exception variable and errorMessageOnFailure identifier in ErrorHandler.cs when
making these changes.
In `@FrendsTaskTemplate/Frends.Template/Frends.Echo.Execute/README.md`:
- Line 3: Replace the placeholder "TaskDescription" in the README with a concise
user-facing summary of the Frends task: state what the task
(Frends.Echo.Execute) does, list key inputs/parameters and their expected types,
describe the main behavior and output, and give a short usage/example or link to
further docs; update the single-line header at the top to be meaningful (e.g.,
"Echo task — returns input data" or similar) so the README no longer contains
the unresolved "TaskDescription" token.
In `@FrendsTaskTemplate/README.md`:
- Line 45: The README contains a fenced code block without a language
identifier; update the opening triple-backtick for that block to include a
language tag (e.g., change the opening ``` to ```text) so the block is properly
highlighted/rendered — locate the lone fenced code block in the README and add
the "text" language identifier to its opening fence.
- Line 9: Replace the vague link text "here" with a descriptive phrase so the
README's link is accessible and scannable; update the link anchor that currently
reads "custom Tasks
[here](https://docs.frends.com/en/articles/2206746-custom-tasks)" to something
explicit like "custom Tasks documentation" (e.g., "custom Tasks documentation"
linking to the same URL) while keeping the URL unchanged.
- Around line 82-84: Replace the incorrect initial-setup instruction that says
"git pull https://github.com/FrendsPlatform/FrendsTaskTemplate.git" with a
proper initial-clone instruction using git clone for the repository (and add a
brief note to change into the cloned directory afterwards); update the README.md
text around that command so new contributors run git clone <repo-url> as the
first step instead of git pull.
In `@README.md`:
- Line 6: Fix the typo in the README line that reads "[GitHub
Actions](/.github/actions) – GitHub Actions reused withing GitHub workflows." by
replacing "withing" with "within" so the sentence becomes "[GitHub
Actions](/.github/actions) – GitHub Actions reused within GitHub workflows."
---
Duplicate comments:
In `@README.md`:
- Around line 5-9: The README link list uses root-relative paths (leading "/")
which can break outside the repo; update each link entry (the GitHub Workflows,
GitHub Actions, Analyzers, Toolkit Template, Task Template entries in README.md)
to use repo-relative/relative paths without the leading slash (e.g., remove the
leading "/" so links point to .github/workflows, .github/actions,
FrendsTaskAnalyzers, FrendsToolkitTemplate, FrendsTaskTemplate) to ensure
reliable navigation within the repository.
- Line 3: Update the intro sentence string "This repository serves as a central
place for sharing Frends Tasks related things." to hyphenate the compound
adjective by changing "Tasks related" to "Tasks-related" so the sentence reads
"This repository serves as a central place for sharing Frends Tasks-related
things."
---
Nitpick comments:
In
`@FrendsTaskTemplate/Frends.Template/Frends.Echo.Execute/Frends.Echo.Execute.Tests/TestBase.cs`:
- Around line 11-16: Move the per-instance call to DotEnv.Load() out of the
TestBase constructor and into a one-time test setup (e.g., a static initializer
or a [OneTimeSetUp] method) so environment is loaded only once; also change the
SecretKey property to be nullable (string?) or remove it if unused and update
any consumers to handle null, locating the symbol SecretKey in TestBase and the
DotEnv.Load() invocation to make the change.
In
`@FrendsTaskTemplate/Frends.Template/Frends.Echo.Execute/Frends.Echo.Execute/Definitions/Error.cs`:
- Line 20: Replace the TODO in Error.cs with a concrete, strongly-typed error
metadata model and wire it into the existing Result type: define a class (e.g.,
ErrorDetails or TaskError) in Error.cs containing fields you need for
diagnostics (Code, Message, Details, Hint, Timestamp), add a property of that
class to the Result type (e.g., Result.ErrorDetails or Result.TaskError), update
any places that construct Result to populate the new property, and ensure the
class is serializable/annotated consistently with existing error handling so
consumers receive structured error metadata instead of a TODO comment.
In
`@FrendsTaskTemplate/Frends.Template/Frends.Echo.Execute/Frends.Echo.Execute/Definitions/Options.cs`:
- Around line 15-17: The Delimiter property in Options.cs is only annotated with
[DefaultValue(" ")] which is metadata and doesn't set the runtime value;
initialize the property directly so it isn't null at construction by giving
Delimiter a default value (e.g., set Delimiter = " ") in its declaration or
constructor in the Options class so consumers reading Options.Delimiter always
get a non-null string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e4625da5-52b5-456d-bdc8-7da599362d52
📒 Files selected for processing (30)
.github/workflows/template_task_release.yml.github/workflows/template_toolkit_release.ymlFrendsTaskTemplate/.gitignoreFrendsTaskTemplate/Frends.Template/.github/workflows/Execute_release.ymlFrendsTaskTemplate/Frends.Template/.github/workflows/Execute_test_on_main.ymlFrendsTaskTemplate/Frends.Template/.github/workflows/Execute_test_on_push.ymlFrendsTaskTemplate/Frends.Template/.template.config/template.jsonFrendsTaskTemplate/Frends.Template/Frends.Echo.Execute/CHANGELOG.mdFrendsTaskTemplate/Frends.Template/Frends.Echo.Execute/Frends.Echo.Execute.Tests/.env.exampleFrendsTaskTemplate/Frends.Template/Frends.Echo.Execute/Frends.Echo.Execute.Tests/ErrorHandlerTest.csFrendsTaskTemplate/Frends.Template/Frends.Echo.Execute/Frends.Echo.Execute.Tests/Frends.Echo.Execute.Tests.csprojFrendsTaskTemplate/Frends.Template/Frends.Echo.Execute/Frends.Echo.Execute.Tests/FunctionalTests.csFrendsTaskTemplate/Frends.Template/Frends.Echo.Execute/Frends.Echo.Execute.Tests/GlobalSuppressions.csFrendsTaskTemplate/Frends.Template/Frends.Echo.Execute/Frends.Echo.Execute.Tests/TestBase.csFrendsTaskTemplate/Frends.Template/Frends.Echo.Execute/Frends.Echo.Execute.slnFrendsTaskTemplate/Frends.Template/Frends.Echo.Execute/Frends.Echo.Execute/Definitions/Connection.csFrendsTaskTemplate/Frends.Template/Frends.Echo.Execute/Frends.Echo.Execute/Definitions/Error.csFrendsTaskTemplate/Frends.Template/Frends.Echo.Execute/Frends.Echo.Execute/Definitions/Input.csFrendsTaskTemplate/Frends.Template/Frends.Echo.Execute/Frends.Echo.Execute/Definitions/Options.csFrendsTaskTemplate/Frends.Template/Frends.Echo.Execute/Frends.Echo.Execute/Definitions/Result.csFrendsTaskTemplate/Frends.Template/Frends.Echo.Execute/Frends.Echo.Execute/Frends.Echo.Execute.csFrendsTaskTemplate/Frends.Template/Frends.Echo.Execute/Frends.Echo.Execute/Frends.Echo.Execute.csprojFrendsTaskTemplate/Frends.Template/Frends.Echo.Execute/Frends.Echo.Execute/FrendsTaskMetadata.jsonFrendsTaskTemplate/Frends.Template/Frends.Echo.Execute/Frends.Echo.Execute/GlobalSuppressions.csFrendsTaskTemplate/Frends.Template/Frends.Echo.Execute/Frends.Echo.Execute/Helpers/ErrorHandler.csFrendsTaskTemplate/Frends.Template/Frends.Echo.Execute/Frends.Echo.Execute/migration.jsonFrendsTaskTemplate/Frends.Template/Frends.Echo.Execute/README.mdFrendsTaskTemplate/FrendsTaskTemplate.csprojFrendsTaskTemplate/README.mdREADME.md
✅ Files skipped from review due to trivial changes (16)
- FrendsTaskTemplate/Frends.Template/Frends.Echo.Execute/CHANGELOG.md
- FrendsTaskTemplate/Frends.Template/Frends.Echo.Execute/Frends.Echo.Execute/FrendsTaskMetadata.json
- FrendsTaskTemplate/Frends.Template/Frends.Echo.Execute/Frends.Echo.Execute/migration.json
- FrendsTaskTemplate/Frends.Template/Frends.Echo.Execute/Frends.Echo.Execute/Definitions/Connection.cs
- FrendsTaskTemplate/Frends.Template/.github/workflows/Execute_test_on_main.yml
- FrendsTaskTemplate/Frends.Template/Frends.Echo.Execute/Frends.Echo.Execute/Definitions/Result.cs
- FrendsTaskTemplate/Frends.Template/Frends.Echo.Execute/Frends.Echo.Execute.sln
- FrendsTaskTemplate/Frends.Template/.github/workflows/Execute_release.yml
- FrendsTaskTemplate/Frends.Template/.github/workflows/Execute_test_on_push.yml
- FrendsTaskTemplate/.gitignore
- FrendsTaskTemplate/Frends.Template/Frends.Echo.Execute/Frends.Echo.Execute/Frends.Echo.Execute.csproj
- FrendsTaskTemplate/Frends.Template/.template.config/template.json
- FrendsTaskTemplate/Frends.Template/Frends.Echo.Execute/Frends.Echo.Execute.Tests/Frends.Echo.Execute.Tests.csproj
- FrendsTaskTemplate/Frends.Template/Frends.Echo.Execute/Frends.Echo.Execute.Tests/GlobalSuppressions.cs
- .github/workflows/template_toolkit_release.yml
- FrendsTaskTemplate/Frends.Template/Frends.Echo.Execute/Frends.Echo.Execute/GlobalSuppressions.cs
Comment on lines
+22
to
+38
| - name: Pack release version | ||
| run: dotnet pack --configuration Release | ||
|
|
||
| - uses: actions/setup-dotnet@v5.0.1 | ||
| with: | ||
| dotnet-version: '8.0.x' | ||
| source-url: ${{ env.azure_artifacts_feed_url }} | ||
| env: | ||
| NUGET_AUTH_TOKEN: ${{ env.nuget_api_key }} | ||
|
|
||
| - name: Push NuGet package(s) | ||
| run: | | ||
| ARGS="--api-key ${{ env.nuget_api_key }} --source ${{ env.azure_artifacts_feed_url }}" | ||
| if [ "${{ env.skip_duplicate }}" = "true" ]; then | ||
| ARGS="$ARGS --skip-duplicate" | ||
| fi | ||
| dotnet nuget push bin/Release/frendstasktemplate.*.nupkg $ARGS |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Read the full workflow file to check the exact order
cat -n .github/workflows/template_task_release.ymlRepository: FrendsPlatform/FrendsTasks
Length of output: 1646
🏁 Script executed:
# Check if FrendsTaskTemplate directory exists and contains a .csproj file
find . -path "*FrendsTaskTemplate*" -name "*.csproj" -o -path "*FrendsTaskTemplate*" -type d | head -20Repository: FrendsPlatform/FrendsTasks
Length of output: 939
🏁 Script executed:
# Check the overall project structure at the root
ls -la | head -20Repository: FrendsPlatform/FrendsTasks
Length of output: 890
🏁 Script executed:
# Look for any .csproj files in the root to understand the project layout
find . -maxdepth 2 -name "*.csproj" -type fRepository: FrendsPlatform/FrendsTasks
Length of output: 169
Release workflow will fail: dotnet pack runs before SDK setup and lacks correct project path.
Line 22 runs dotnet pack before the .NET SDK is set up (line 25), and without specifying the project path. Since FrendsTaskTemplate.csproj is nested under FrendsTaskTemplate/ with no .csproj at the repo root, the pack command will fail with "could not find any project file" error.
Additionally, the pack command should complete before pushing, and the output path needs to be explicit since the default bin/Release/ location depends on the working directory.
🐛 Suggested workflow fix
- - name: Pack release version
- run: dotnet pack --configuration Release
-
- uses: actions/setup-dotnet@v5.0.1
with:
dotnet-version: '8.0.x'
source-url: ${{ env.azure_artifacts_feed_url }}
env:
NUGET_AUTH_TOKEN: ${{ env.nuget_api_key }}
+ - name: Pack release version
+ run: dotnet pack ./FrendsTaskTemplate/FrendsTaskTemplate.csproj --configuration Release --output ./artifacts
+
- name: Push NuGet package(s)
run: |
ARGS="--api-key ${{ env.nuget_api_key }} --source ${{ env.azure_artifacts_feed_url }}"
if [ "${{ env.skip_duplicate }}" = "true" ]; then
ARGS="$ARGS --skip-duplicate"
fi
- dotnet nuget push bin/Release/frendstasktemplate.*.nupkg $ARGS
+ dotnet nuget push ./artifacts/frendstasktemplate.*.nupkg $ARGS🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/template_task_release.yml around lines 22 - 38, The
workflow runs dotnet pack before the .NET SDK is installed and omits the project
path and explicit output, causing pack to fail; reorder to run
actions/setup-dotnet@v5.0.1 before any dotnet commands, run dotnet pack against
the FrendsTaskTemplate/FrendsTaskTemplate.csproj (or the correct project file)
with an explicit --configuration Release and --output path (so artifacts are
predictable), then run the existing push step (dotnet nuget push using ARGS and
the --skip-duplicate conditional) after a successful pack; ensure the ARGS and
environment variables (nuget_api_key, azure_artifacts_feed_url, skip_duplicate)
remain unchanged.
Contributor
Author
There was a problem hiding this comment.
it was tested and did not fail
Contributor
There was a problem hiding this comment.
@MatteoDelOmbra, thanks for clarifying! You're right — GitHub-hosted ubuntu-latest runners come with .NET SDK pre-installed, so dotnet pack works correctly before the actions/setup-dotnet step (which in this case is used primarily to configure the NuGet source URL and auth token rather than to install the SDK). I apologize for the false alarm.
(´• ω •`) ʕ•ᴥ•ʔ
| This template can be used to create new .NET (previously .NET Core) Tasks for [Frends](https://frends.com) integration | ||
| platform. This enables you to start writing code without any hassle with project formats etc. | ||
|
|
||
| You can learn more about custom Tasks [here](https://docs.frends.com/en/articles/2206746-custom-tasks). |
Contributor
There was a problem hiding this comment.
Use descriptive link text instead of “here”.
Line 9 should use explicit link text (for accessibility and scanability).
Proposed fix
-You can learn more about custom Tasks [here](https://docs.frends.com/en/articles/2206746-custom-tasks).
+You can learn more in the [Frends custom Tasks documentation](https://docs.frends.com/en/articles/2206746-custom-tasks).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| You can learn more about custom Tasks [here](https://docs.frends.com/en/articles/2206746-custom-tasks). | |
| You can learn more in the [Frends custom Tasks documentation](https://docs.frends.com/en/articles/2206746-custom-tasks). |
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 9-9: Link text should be descriptive
(MD059, descriptive-link-text)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@FrendsTaskTemplate/README.md` at line 9, Replace the vague link text "here"
with a descriptive phrase so the README's link is accessible and scannable;
update the link anchor that currently reads "custom Tasks
[here](https://docs.frends.com/en/articles/2206746-custom-tasks)" to something
explicit like "custom Tasks documentation" (e.g., "custom Tasks documentation"
linking to the same URL) while keeping the URL unchanged.
|
|
||
| ### Example output: | ||
|
|
||
| ``` |
Contributor
There was a problem hiding this comment.
Add language identifier to fenced code block.
Line 45 starts a fenced block without a language tag.
Proposed fix
-```
+```text📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 45-45: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@FrendsTaskTemplate/README.md` at line 45, The README contains a fenced code
block without a language identifier; update the opening triple-backtick for that
block to include a language tag (e.g., change the opening ``` to ```text) so the
block is properly highlighted/rendered — locate the lone fenced code block in
the README and add the "text" language identifier to its opening fence.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/actions/check-coverage/action.yml (2)
41-41: Add-type fand consider deterministic file selection.The
findcommand without-type fcould match directories. Also, if multiple coverage files exist,head -n 1returns whicheverfindoutputs first (filesystem-dependent order), which may lead to inconsistent results across runs.♻️ Suggested improvement
- file="$(find . -name "${{ inputs.coverage-file-pattern }}" 2>/dev/null | head -n 1)" + file="$(find . -type f -name "${{ inputs.coverage-file-pattern }}" 2>/dev/null | sort | head -n 1)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/actions/check-coverage/action.yml at line 41, The find invocation that sets the file variable (file="$(find . -name "${{ inputs.coverage-file-pattern }}" 2>/dev/null | head -n 1)") may return directories and is non-deterministic when multiple matches exist; update the command used to set file to restrict matches to regular files (add -type f) and make selection deterministic (e.g., pipe through sort before taking the first result) so file consistently points to the same coverage file when multiple matches exist; reference the file assignment line and the inputs.coverage-file-pattern variable in your change.
85-90: Add timeout to curl to prevent workflow hangs.The
curlcommand has no timeout. If the badge service is unresponsive, the workflow step could hang indefinitely until GitHub's job timeout kills it.♻️ Suggested fix with timeout
curl --fail --silent --show-error \ + --max-time 30 \ -X POST \ -H "Content-Type: application/json" \ -H "x-api-key: $apiKey" \ -d "$json" \ "https://app-github-custom-badges.azurewebsites.net/Badge"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/actions/check-coverage/action.yml around lines 85 - 90, The curl invocation used to POST the badge payload lacks a timeout and can hang; update the curl command in the action.yml where the POST is sent (the multi-line curl block) to include a timeout option such as --max-time <seconds> (and optionally --connect-timeout <seconds>) so the step fails fast if the badge service is unresponsive; add sensible defaults (e.g., 10–30s) and ensure the options are placed alongside the existing flags (-X POST, -H, -d, etc.).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/actions/check-coverage/action.yml:
- Line 52: The code directly reads root.attrib['line-rate'] into rate which
raises KeyError if the attribute is missing; update the logic around rate (the
assignment using root.attrib['line-rate']) to defensively check for the
attribute (e.g., use root.attrib.get('line-rate')) and validate it before
calling float(), or wrap the access/float conversion in a try/except block to
catch KeyError/ValueError and log a clear error and exit with a non-zero status
(or fallback to a safe default) so malformed Cobertura XML doesn’t crash with an
unhelpful exception.
---
Nitpick comments:
In @.github/actions/check-coverage/action.yml:
- Line 41: The find invocation that sets the file variable (file="$(find . -name
"${{ inputs.coverage-file-pattern }}" 2>/dev/null | head -n 1)") may return
directories and is non-deterministic when multiple matches exist; update the
command used to set file to restrict matches to regular files (add -type f) and
make selection deterministic (e.g., pipe through sort before taking the first
result) so file consistently points to the same coverage file when multiple
matches exist; reference the file assignment line and the
inputs.coverage-file-pattern variable in your change.
- Around line 85-90: The curl invocation used to POST the badge payload lacks a
timeout and can hang; update the curl command in the action.yml where the POST
is sent (the multi-line curl block) to include a timeout option such as
--max-time <seconds> (and optionally --connect-timeout <seconds>) so the step
fails fast if the badge service is unresponsive; add sensible defaults (e.g.,
10–30s) and ensure the options are placed alongside the existing flags (-X POST,
-H, -d, etc.).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c5913e71-cf29-4fdd-ba04-b0d150056d9a
📒 Files selected for processing (3)
.github/actions/check-coverage/action.ymlFrendsToolkitTemplate/Template/Company.Echo.Toolkit/Company.Echo.Toolkit.Tests/HandlersTests/ValidationHandlerTests.csFrendsToolkitTemplate/Template/Company.Echo.Toolkit/Company.Echo.Toolkit.sln
✅ Files skipped from review due to trivial changes (1)
- FrendsToolkitTemplate/Template/Company.Echo.Toolkit/Company.Echo.Toolkit.sln
🚧 Files skipped from review as they are similar to previous changes (1)
- FrendsToolkitTemplate/Template/Company.Echo.Toolkit/Company.Echo.Toolkit.Tests/HandlersTests/ValidationHandlerTests.cs
| tree = ET.parse('$file') | ||
| root = tree.getroot() | ||
| # Cobertura uses 'line-rate' attribute (e.g., 0.85) | ||
| rate = float(root.attrib['line-rate']) |
Contributor
There was a problem hiding this comment.
No error handling if line-rate attribute is missing.
If the Cobertura XML file is malformed or missing the line-rate attribute, the script will fail with an unhelpful KeyError. Consider adding defensive handling.
🛡️ Suggested fix with error handling
root = tree.getroot()
- # Cobertura uses 'line-rate' attribute (e.g., 0.85)
- rate = float(root.attrib['line-rate'])
+ rate_str = root.attrib.get('line-rate')
+ if rate_str is None:
+ print("ERROR: line-rate attribute not found", file=__import__('sys').stderr)
+ __import__('sys').exit(1)
+ rate = float(rate_str)
print(round(rate * 100))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/actions/check-coverage/action.yml at line 52, The code directly
reads root.attrib['line-rate'] into rate which raises KeyError if the attribute
is missing; update the logic around rate (the assignment using
root.attrib['line-rate']) to defensively check for the attribute (e.g., use
root.attrib.get('line-rate')) and validate it before calling float(), or wrap
the access/float conversion in a try/except block to catch KeyError/ValueError
and log a clear error and exit with a non-zero status (or fallback to a safe
default) so malformed Cobertura XML doesn’t crash with an unhelpful exception.
Contributor
Author
There was a problem hiding this comment.
there is default value so it won't be missing
Contributor
There was a problem hiding this comment.
@MatteoDelOmbra, understood — line-rate is indeed a required attribute in the Cobertura XML schema, so any conforming output from tools like coverlet will always include it. Fair enough, I'll defer to your judgment here.
Contributor
Author
|
@jefim I've let myself resolve rabbits tickets related to TaskTemplate logic stuff - I just moved project to this repository and don't want to introduce changes to it right now |
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
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.
Summary by CodeRabbit
New Features
Tests
Chores