Conversation
|
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:
📝 WalkthroughWalkthroughParses artifact metadata from JUnit XML (spec/test/attempt), copies and hashes files into Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Convert as Convert Service
participant XML as XML Parser
participant FS as File System
participant Artifacts as ".currents/artifacts"
User->>Convert: handleConvert(junit.xml)
Convert->>XML: parse test suites/cases properties
XML-->>Convert: artifact metadata (spec/test/attempt)
Convert->>FS: resolve & validate artifact paths
FS-->>Convert: exists / missing
Convert->>FS: compute hash & copyFileAsync(src, Artifacts/hashed)
FS-->>Artifacts: write hashed file
Convert->>Convert: update instance.artifacts and _stdout
Convert->>User: write instance JSONs
sequenceDiagram
actor Client
participant Upload as Upload Service
participant Director as Director API
participant Storage as Pre-signed Storage
participant FS as File System
Client->>Upload: uploadResults(chunks)
Upload->>Director: POST /v1/runs (artifact metadata)
Director-->>Upload: ArtifactUploadInstruction[], stdoutUploadUrls[]
loop per instruction
Upload->>FS: read artifact file & detect content-type
FS-->>Upload: file bytes, content-type
Upload->>Storage: PUT to pre-signed URL (artifact)
Storage-->>Upload: 200 OK
end
alt stdout present
Upload->>Upload: collect instance stdout
Upload->>Storage: PUT stdout (text/plain) to pre-signed URL
Storage-->>Upload: 200 OK
end
Upload-->>Client: upload complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
.gitignore (1)
54-54: Scope theartifactsignore rule more narrowly (or remove it).
artifactsis broader than needed and may ignore unrelated paths namedartifacts. Since Line 52 already ignores.currents, this rule is likely redundant for.currents/artifacts. Consider removing it, or replacing with a scoped path if you intend a specific directory.Proposed cleanup
# Currents reports .currents - -artifactsIf you specifically want a root-level directory ignored:
+ /artifacts/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore at line 54, The .gitignore currently contains a broad rule "artifacts" which may unintentionally ignore unrelated paths; narrow or remove it by either deleting the "artifacts" entry or replacing it with a scoped pattern such as ".currents/artifacts" or "/artifacts" to target the intended directory (refer to the existing ".currents" rule for scope); update the .gitignore to only ignore the specific artifacts directory you intend.packages/cmd/src/commands/upload/artifacts.md (1)
48-51: Consider documenting allArtifactUploadInstructionfields.The documentation lists only
pathanduploadUrl, but the type definition increate-run.tsincludes additional fields (artifactIdandreadUrl). Consider documenting all fields for completeness, or explicitly note which fields are relevant to the upload workflow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cmd/src/commands/upload/artifacts.md` around lines 48 - 51, The docs currently list only path and uploadUrl for ArtifactUploadInstruction; update the markdown to document all fields returned by the service (including artifactId and readUrl) or explicitly state which fields are required for upload. Locate the type/shape in create-run.ts (ArtifactUploadInstruction or the function that returns it) and add short descriptions for artifactId, readUrl, path, and uploadUrl in the artifacts.md entry so the docs match the runtime shape and clarify which fields callers must use for uploading versus read access.packages/cmd/src/helpers/index.ts (1)
44-46: Prefer a binary-safe fallback MIME type for unknown extensions.Defaulting to
text/plaincan mislabel binary files;application/octet-streamis safer for generic attachments.♻️ Proposed tweak
} else { type = type || 'attachment'; - contentType = contentType || 'text/plain'; + contentType = contentType || 'application/octet-stream'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cmd/src/helpers/index.ts` around lines 44 - 46, The fallback MIME for unknown extensions should be binary-safe: update the default for the contentType variable in the helper (where contentType = contentType || 'text/plain') to use 'application/octet-stream' instead; leave the attachment type default (type = type || 'attachment') unchanged and ensure any callers that rely on the helper still receive the new fallback when contentType is absent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cmd/src/commands/convert/artifacts.md`:
- Around line 29-44: The example XML uses the same artifact index for two
different artifacts which will group them incorrectly; update the video artifact
property names (the properties using currents.artifact.attempt.0.path,
currents.artifact.attempt.0.type, currents.artifact.attempt.0.contentType) to
use index 1 instead of 0 so the screenshot remains attempt.0 and the video
becomes attempt.1, ensuring grouping by attempt index is correct.
In `@packages/cmd/src/services/convert/utils.ts`:
- Around line 119-127: The current use of
log.matchAll(/currents\.artifact\.(\{.*?\})/g) truncates nested JSON; replace it
with a brace-aware extraction: iterate occurrences of the literal
"currents.artifact." in the log, for each occurrence locate the first '{' and
scan forward counting braces (and honoring string escapes/quotes) until the
matching closing '}' to obtain the full JSON substring, then JSON.parse that
substring and pass the resulting object to isValidArtifact before pushing to
artifacts; implement the scanning logic as a small helper (e.g.,
extractBalancedJson or parseBalancedObject) and use it instead of the regex so
jsonMatches/json parsing is robust.
In `@packages/cmd/src/services/upload/index.ts`:
- Around line 306-310: The axios PUT calls that upload data (the calls that use
instruction.uploadUrl and aggregatedStdout, and the similar PUT at the other
occurrence) lack a timeout and can hang indefinitely; update both axios.put
invocations to include a timeout option (e.g., timeout: 30000) in the config
object passed as the 3rd argument, or define a single UPLOAD_TIMEOUT_MS constant
(or env-driven value) and use it for both calls so they share the same
configurable timeout setting.
- Around line 390-394: The code currently uses path.join(reportDir,
instruction.path) to build filePath which allows path traversal via
instruction.path; replace this by resolving both the base and target (e.g., base
= path.resolve(reportDir) and target = path.resolve(reportDir,
instruction.path)) and then verify containment (e.g., path.relative(base,
target) does not start with '..' and target.startsWith(base)); also reject
absolute instruction.path values or paths that escape via symlinks and log/warn
and return if the containment check fails before calling fs.pathExists or
uploading the file (refer to filePath, reportDir, instruction.path, path.join,
fs.pathExists, warn).
---
Nitpick comments:
In @.gitignore:
- Line 54: The .gitignore currently contains a broad rule "artifacts" which may
unintentionally ignore unrelated paths; narrow or remove it by either deleting
the "artifacts" entry or replacing it with a scoped pattern such as
".currents/artifacts" or "/artifacts" to target the intended directory (refer to
the existing ".currents" rule for scope); update the .gitignore to only ignore
the specific artifacts directory you intend.
In `@packages/cmd/src/commands/upload/artifacts.md`:
- Around line 48-51: The docs currently list only path and uploadUrl for
ArtifactUploadInstruction; update the markdown to document all fields returned
by the service (including artifactId and readUrl) or explicitly state which
fields are required for upload. Locate the type/shape in create-run.ts
(ArtifactUploadInstruction or the function that returns it) and add short
descriptions for artifactId, readUrl, path, and uploadUrl in the artifacts.md
entry so the docs match the runtime shape and clarify which fields callers must
use for uploading versus read access.
In `@packages/cmd/src/helpers/index.ts`:
- Around line 44-46: The fallback MIME for unknown extensions should be
binary-safe: update the default for the contentType variable in the helper
(where contentType = contentType || 'text/plain') to use
'application/octet-stream' instead; leave the attachment type default (type =
type || 'attachment') unchanged and ensure any callers that rely on the helper
still receive the new fallback when contentType is absent.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonpackages/cmd/src/services/convert/__tests__/__snapshots__/instances.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (15)
.gitignorepackages/cmd/package.jsonpackages/cmd/src/api/create-run.tspackages/cmd/src/commands/convert/artifacts.mdpackages/cmd/src/commands/upload/artifacts.mdpackages/cmd/src/helpers/index.tspackages/cmd/src/lib/fs.tspackages/cmd/src/services/convert/__tests__/convert-artifacts.test.tspackages/cmd/src/services/convert/__tests__/fixtures/index.tspackages/cmd/src/services/convert/index.tspackages/cmd/src/services/convert/postman/instances.tspackages/cmd/src/services/convert/utils.tspackages/cmd/src/services/upload/index.tspackages/cmd/src/types.tspackages/cmd/tsup.config.ts
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
packages/cmd/src/services/convert/__tests__/convert-artifacts.test.ts (1)
36-220: Add a regression case for marker extraction boundaries.Current tests validate copy/update flow, but not marker extraction rules. Please add a case that includes markers in both
system-outandsystem-errand asserts onlysystem-outcontributes artifacts (with[[CURRENTS.ATTACHMENT|...]]format).Based on learnings: marker extraction behavior is intentionally constrained to
system-outand to the[[CURRENTS.ATTACHMENT|...]]marker format.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cmd/src/services/convert/__tests__/convert-artifacts.test.ts` around lines 36 - 220, Add a regression test in convert-artifacts.test.ts that verifies marker extraction only reads markers from system-out and only recognizes the [[CURRENTS.ATTACHMENT|...]] format: create an InstanceReport where a test attempt has stdout containing a valid marker ([[CURRENTS.ATTACHMENT|/path/to/file|content-type]]) and stderr containing the same marker, run handleConvert(), then assert that only the artifact from stdout was written and referenced in the output JSON (check attempt.artifacts entries and artifact.path/contentType) and that no artifact was created from stderr; place this alongside the existing "processes attachment artifacts" test and reuse temporary dir setup (baseDir/artifacts and baseDir/instances) so it exercises the same copy/update flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/junit/junit-attempts.xml`:
- Around line 6-16: Move the suite-level artifact <property> elements so they
are children of a <properties> element under the <testsuite>, not direct
children of <testsuite>; specifically wrap the repeated
currents.artifact.instance.* <property> entries inside a single <properties>
node so parsers that expect testsuite > properties > property will discover them
(look for the <property> entries with names like
currents.artifact.instance.path, .type, .contentType, .name and place them under
a newly created <properties> element within the same <testsuite>).
In `@examples/vitest/junit.xml`:
- Line 3: The fixture contains machine-specific identifiers: replace the
hostname attribute on the <testsuite> element (currently "Miguels-Laptop.local")
with a deterministic placeholder such as "localhost" and sanitize all absolute
user paths under testcases (the `/Users/...` entries shown in the file) to
repo-relative artifact paths or a placeholder like "path/to/artifact" so the XML
is deterministic; search for the "hostname" attribute on the <testsuite> element
and for any absolute paths in <testcase>, <system-out>, <system-err>, stacktrace
or artifact-like elements and update them to use the placeholders consistently.
In `@packages/cmd/src/commands/convert/artifacts.md`:
- Around line 20-26: Update the documentation to mention the indexed attempt key
format in addition to the unindexed form: explicitly document that keys of the
form currents.artifact.attempt.{index}.{property} are supported (where {index}
is a numeric attempt index) and describe how they map to attempt elements when
parsing (use numeric index to target a specific attempt; if no attempts
structure exists, unindexed keys default to attempt 0). Reference the existing
documented symbols currents.artifact.attempt.{property} and the properties
(path, type, contentType, name) and add a short note about precedence/merging
behavior between indexed and unindexed keys.
In `@packages/cmd/src/services/convert/__tests__/utils.artifacts.test.ts`:
- Around line 209-238: The test's expectations are wrong for how indexed
artifacts are assigned; update the assertions around getTestCase so they expect
artifacts to remain on their indexed attempts rather than collapsed into attempt
0: assert result.attempts[0].artifacts contains 'a0p' and
result.attempts[1].artifacts contains 'a1p' (instead of both in attempts[0]),
and similarly adjust the other related test block (the assertions in the 240-270
region) to check per-index assignment using result.attempts[n].artifacts for the
matching attempt index.
- Around line 114-167: This test block is a no-op (no assertions) and should be
removed or converted into a proper assertion-based test; either delete the
entire 'it' case or replace it with a completed scenario that calls getTestCase
(and indirectly getTestAndAttemptArtifacts/getTestAttempts) with a constructed
TestCase covering the intended behavior (e.g., two failures to exercise multiple
attempts or one failure to assert single attempt) and add assertions on the
returned result (attempts, retries, and artifacts) to validate that
attemptArtifacts from getTestAndAttemptArtifacts are applied correctly;
reference getTestCase, getTestAndAttemptArtifacts, and getTestAttempts when
implementing the finished test.
- Around line 297-309: The test "ignores logs for artifact collection" is wrong:
system-out markers should be parsed, so update the assertions to expect the
parsed artifact(s) instead of empty/undefined; call out getTestCase (which uses
getTestAndAttemptArtifacts) and extractArtifactsFromLog as the relevant helpers
to locate the behavior. Change the expectation to assert that result.artifacts
contains the attachment parsed from testCase['system-out'] (and that
result.attempts[0].artifacts reflects the same parsed artifacts or an empty
array only if that is the intended attempt-level contract), removing the brittle
use of undefined vs [] and asserting the concrete parsed artifact structure
instead. Ensure the test name and description reflect that system-out markers
are parsed, not ignored.
In `@packages/cmd/src/services/convert/utils.ts`:
- Around line 104-124: The loop that builds attemptProperties currently skips
Property objects when prop.value is missing, but Property can carry text-node
data in prop._; update the guard in the properties-processing loop (and the
analogous block at the 141-162 range) to treat a property as present when either
prop.value or prop._ exists, and when pushing into attemptProperties use the
actual text value (prop.value ?? prop._) as the value passed into parseArtifacts
so XML text-node fields aren’t dropped; reference: the for (const prop of
properties) loop, attemptProperties map, and parseArtifacts usage.
- Around line 68-95: getTestAndAttemptArtifacts currently only parses XML
properties and misses artifact markers embedded in testCase['system-out'];
update getTestAndAttemptArtifacts to also scan testCase['system-out'] (but not
system-err) for marker strings like
currents.artifact.../[[CURRENTS.ATTACHMENT|...]] and feed those into the
existing parsing flow: extract marker lines from testCase['system-out'], convert
or pass them to parseArtifacts/parseIndexedAttemptArtifacts (or a small helper
that reuses those parsers) so that returned testArtifacts and attemptArtifacts
include artifacts found in system-out; keep the same assignment rules (unindexed
-> attempt 0, indexed -> their numeric attempt) and use the same
attemptArtifacts Map handling already in the function.
---
Nitpick comments:
In `@packages/cmd/src/services/convert/__tests__/convert-artifacts.test.ts`:
- Around line 36-220: Add a regression test in convert-artifacts.test.ts that
verifies marker extraction only reads markers from system-out and only
recognizes the [[CURRENTS.ATTACHMENT|...]] format: create an InstanceReport
where a test attempt has stdout containing a valid marker
([[CURRENTS.ATTACHMENT|/path/to/file|content-type]]) and stderr containing the
same marker, run handleConvert(), then assert that only the artifact from stdout
was written and referenced in the output JSON (check attempt.artifacts entries
and artifact.path/contentType) and that no artifact was created from stderr;
place this alongside the existing "processes attachment artifacts" test and
reuse temporary dir setup (baseDir/artifacts and baseDir/instances) so it
exercises the same copy/update flow.
ℹ️ Review info
Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c20f8f3e-fc7a-4973-beb5-d55cc3068057
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
examples/junit/junit-attempts.xmlexamples/junit/junit.xmlexamples/junit/package.jsonexamples/junit/test-output/config.jsonexamples/junit/test-output/fullTestSuite.jsonexamples/junit/test-output/instances/2-FtWFIn.jsonexamples/vitest/artifacts.test.tsexamples/vitest/junit.xmlexamples/vitest/package.jsonexamples/vitest/properties-junit.xmlexamples/vitest/vitest.config.tspackages/cmd/src/commands/convert/artifacts.mdpackages/cmd/src/services/convert/__tests__/convert-artifacts.test.tspackages/cmd/src/services/convert/__tests__/utils.artifacts.test.tspackages/cmd/src/services/convert/utils.ts
✅ Files skipped from review due to trivial changes (3)
- examples/junit/test-output/fullTestSuite.json
- examples/junit/test-output/config.json
- examples/junit/junit.xml
packages/cmd/src/services/convert/__tests__/utils.artifacts.test.ts
Outdated
Show resolved
Hide resolved
packages/cmd/src/services/convert/__tests__/utils.artifacts.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/cmd/src/services/convert/postman/instances.ts (1)
51-57: Keep_stdoutstrictly stdout (excludesystem-err)
_stdoutis later persisted as atype: 'stdout'artifact, so includingsystem-errhere mixes streams and can mislabel stderr as stdout.Suggested patch
- const stdOut = [ - suite['system-out'], - suite['system-err'], - ...testcases.flatMap((t) => [t['system-out'], t['system-err']]), - ] + const stdOut = [suite['system-out'], ...testcases.map((t) => t['system-out'])] .filter((i) => i) .join('\n');Based on learnings: In
packages/cmd/src/services/convert/utils.ts, artifact log details are designed to come fromsystem-out, notsystem-err.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cmd/src/services/convert/postman/instances.ts` around lines 51 - 57, Stdout currently aggregates both suite['system-out'] and suite['system-err'] (and the same from testcases) into the stdOut variable, which mislabels stderr as stdout; update the construction of stdOut to only include suite['system-out'] and t['system-out'] from testcases, and if stderr needs to be preserved create a separate stdErr (or similar) that collects suite['system-err'] and t['system-err']; ensure later persistence uses stdOut for type:'stdout' artifacts and stdErr (if created) for stderr artifacts (references: stdOut variable, suite['system-out'], suite['system-err'], testcases, t['system-out'], t['system-err']).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cmd/src/services/convert/index.ts`:
- Around line 95-117: The current code uses generateShortHash(report.spec) and
writeFileAsyncIfNotExists which can cause filename collisions and stale content
and also leaves report._stdout undeleted when empty; fix by creating a
uniquely-named artifact file (e.g., combine generateShortHash(report.spec) with
a per-instance identifier or a content-hash/timestamp like
`${generateShortHash(report.spec)}-${generateShortHash(report._stdout ||
Date.now().toString())}.stdout.txt`) so filenames are unique, call an
overwrite-safe write helper (replace writeFileAsyncIfNotExists with a
writeFileAsync/overwrite variant) and ensure report.artifacts is initialized
before any file operations, only push the artifact entry when report._stdout has
content, and always delete report._stdout at the end of handling (remove it even
if empty) so transient stdout never leaks into persisted reports; refer to
generateShortHash, writeFileAsyncIfNotExists, report._stdout, report.artifacts,
artifactsDir, and join when making these edits.
- Around line 71-80: The current boundary check uses lexical paths
(resolvedPath/relativePath) which can be bypassed by symlinks; before allowing
copyFileAsync to follow links, canonicalize paths with realpath() (call realpath
on workspaceRoot once and on each resolvedPath) and then compute relative
against the canonical workspaceRoot to ensure the target is still inside the
workspace; if realpath of the artifact target fails or the canonical relative
startsWith('..') or isAbsolute, log/skip the artifact (as you do now) to prevent
symlink traversal escaping the workspace.
---
Nitpick comments:
In `@packages/cmd/src/services/convert/postman/instances.ts`:
- Around line 51-57: Stdout currently aggregates both suite['system-out'] and
suite['system-err'] (and the same from testcases) into the stdOut variable,
which mislabels stderr as stdout; update the construction of stdOut to only
include suite['system-out'] and t['system-out'] from testcases, and if stderr
needs to be preserved create a separate stdErr (or similar) that collects
suite['system-err'] and t['system-err']; ensure later persistence uses stdOut
for type:'stdout' artifacts and stdErr (if created) for stderr artifacts
(references: stdOut variable, suite['system-out'], suite['system-err'],
testcases, t['system-out'], t['system-err']).
ℹ️ Review info
Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1ac75a51-03ff-4020-8027-ecc20d3724b5
📒 Files selected for processing (5)
examples/junit/junit-attempts.xmlexamples/junit/package.jsonpackages/cmd/src/services/convert/index.tspackages/cmd/src/services/convert/postman/instances.tspackages/cmd/src/types.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/junit/junit-attempts.xml
- examples/junit/package.json
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/junit/junit-attempts.xml`:
- Around line 63-67: The suite-level artifact <property> entries (e.g.,
properties with names like "currents.artifact.instance.path",
"currents.artifact.instance.type", "currents.artifact.instance.contentType",
"currents.artifact.instance.name") are placed directly under the <testsuite>;
wrap these <property> elements inside a <properties> element nested under the
<testsuite> so the JUnit parser can read them (locate the <testsuite> block
containing those currents.artifact.instance.* <property> tags and move them into
a new <properties> container).
ℹ️ Review info
Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d60bbdc7-44ab-4e85-911e-c7ed99787d12
📒 Files selected for processing (1)
examples/junit/junit-attempts.xml
Contributor
Author
|
@coderabbitai if there's not outstanding issues, approve the PR |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 60801 🏁 Script executed: Length of output: 0
[approve] 🧠 Learnings used |
vCaisim
reviewed
Mar 4, 2026
Collaborator
vCaisim
left a comment
vCaisim
left a comment
There was a problem hiding this comment.
@miguelangaranocurrents overall looks good. Please solve the refactor notes and I will re-test and merge.
vCaisim
requested changes
Mar 4, 2026
vCaisim
approved these changes
Mar 5, 2026
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
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Description: Enhanced Artifact Upload Reliability and XML Parsing
Overview
This branch improves the reliability and compatibility of the Currents reporter, specifically focusing on how artifacts are discovered and uploaded. It addresses potential CI hangs and ensures broader compatibility with different JUnit XML report formats.
Key Improvements
1. Prevent Hanging Uploads
Introduces a safeguard against indefinite hanging during artifact uploads. By enforcing a 30-second timeout on upload requests, the reporter now fails gracefully rather than stalling the CI pipeline if the upload service becomes unresponsive. This ensures that network issues do not cause builds to hang indefinitely.
2. Expanded XML Support
Broadens support for various JUnit XML report formats. The reporter can now correctly parse artifact properties defined as text nodes (e.g.,
<property name="...">value</property>) in addition to the standard attribute-based format. This ensures that artifacts are correctly discovered and uploaded regardless of the specific XML reporter implementation used to generate the reports.3. Documentation Accuracy
Updates the internal documentation to accurately reflect the current artifact handling workflows for both
convertanduploadcommands, including visual workflow diagrams and clarified syntax requirements for artifact properties.Verification
The following verification steps were performed to ensure correctness:
convertcorrectly parses XMLs with text-node properties via updated test fixtures.npx vitest run packages/cmd/src/services/convert/__tests__/instances.test.tsnpx vitest run packages/cmd/src/services/convert/__tests__/utils.test.tsSummary by CodeRabbit