Conversation
Introduce a directory-based Allure parser and wire it into the upload pipeline so users can upload Allure 2 results with TMS-link or marker-based matching. Extend CLI, tests, and docs for allure-upload, support force-aware skipping of invalid Allure files, and add AGENTS.md as a symlink to CLAUDE.md. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Claude finished @satvik007's task in 5m 6s —— View job Review Complete
Submitted a review with 7 inline comments covering:
No critical/high severity issues found. No performance regressions introduced. |
Summary of ChangesHello @satvik007, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the QAS CLI's capabilities by integrating native support for Allure test reports. Users can now upload Allure results directories, leveraging a new parser that handles status mapping, folder organization, and attachment resolution. The update also includes enhanced error handling for invalid Allure files and comprehensive documentation, making the CLI more versatile for diverse test automation workflows. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
Good implementation that follows existing codebase patterns well. The Allure parser is well-structured with proper Zod validation, comprehensive status mapping, and a clear marker extraction priority chain (TMS URL → TMS link name → test name).
Key items to address:
-
Path traversal via attachment
sourcefield (medium security) — Allure result JSON files can specify arbitrary file paths inattachments[].source, which are read and uploaded without path validation. Relevant in CI/CD where results may come from untrusted sources. -
Marker regex mismatch —
markerRegexaccepts\d+(any digits) while the rest of the codebase uses\d{3,}(≥3 digits), risking false-positive matches on text like "step-1". -
Missing directory validation —
readdirSyncon a non-existent path produces a raw Node.js error without user-friendly context. An empty directory silently returns zero results. -
CLAUDE.md numbering regression — The numbered list in "Core Upload Pipeline" has item 2 rendered as item 1.
Design notes (non-blocking):
- The
Parsertype'sdataparameter is semantically overloaded (file contents vs. directory path). The comment helps, but this is technical debt to watch. - The
statusDetails.message→ "stdout" /.trace→ "stderr" mapping is imprecise relative to the CLI flag names. - Several test coverage gaps compared to JUnit/Playwright parity (empty dir, stop < start, asymmetric skip options, HTML escaping, no-marker case).
|
|
||
| type AllureResult = z.infer<typeof allureResultSchema> | ||
|
|
||
| const markerRegex = /\b([A-Za-z0-9]{1,5})-(\d+)\b/ |
There was a problem hiding this comment.
Marker regex accepts shorter sequences than the rest of the codebase
The markerRegex here uses \d+ (any number of digits), but ResultUploadCommandHandler.detectProjectCodeFromTCaseNames and getTCaseIds use \d{3,} (at least 3 digits). While getTCaseMarker() normalizes short sequences (e.g., PRJ-1 → PRJ-001), the broader regex could produce false-positive matches on non-marker text (e.g., a test name containing "step-1" or "v2-5").
Consider changing to \d{3,} for consistency:
| const markerRegex = /\b([A-Za-z0-9]{1,5})-(\d+)\b/ | |
| const markerRegex = /\b([A-Za-z0-9]{1,5})-(\d{3,})\b/ |
Alternatively, if matching shorter sequences is intentional (since getTCaseMarker normalizes), add a comment explaining the divergence.
| attachments: [], | ||
| }) - 1 | ||
|
|
||
| const attachmentPaths = (parsedResult.attachments || []).map((attachment) => attachment.source) |
There was a problem hiding this comment.
Path traversal via Allure attachment source field (Medium severity)
The attachment.source field from Allure JSON result files is used directly as a file path without path traversal validation. A malicious result file could set source to ../../../../etc/passwd or an absolute path, and path.join() in the getAttachments utility would resolve the traversal segments, allowing arbitrary file reads. The file contents would then be uploaded to the QA Sphere API as attachments.
This is particularly relevant in CI/CD environments where Allure results may originate from untrusted test code or shared artifact stores.
Consider validating that resolved attachment paths remain within the expected base directory:
const resolvedPath = path.resolve(basePath, filePath)
if (!resolvedPath.startsWith(path.resolve(basePath))) {
throw new Error(`Attachment path "${filePath}" escapes the base directory`)
}Alternatively, add a Zod refinement on the source field to reject values containing path separators or .. segments.
| attachmentBaseDirectory: string, | ||
| options: ParserOptions | ||
| ): Promise<TestCaseResult[]> => { | ||
| const resultFiles = readdirSync(resultsDirectory) |
There was a problem hiding this comment.
No validation that the results directory exists or is readable
If resultsDirectory does not exist or is not a directory, readdirSync will throw a raw Node.js error (e.g., ENOENT: no such file or directory, scandir '/wrong/path'). Unlike the JUnit/Playwright parsers which get their error at the readFileSync call in parseFiles(), the Allure parser's error lacks user-friendly context.
Consider wrapping this in a try/catch with a descriptive error:
let resultFiles: string[]
try {
resultFiles = readdirSync(resultsDirectory)
.filter((f) => f.endsWith('-result.json'))
.sort()
} catch (error) {
throw new Error(`Failed to read Allure results directory "${resultsDirectory}": ${getErrorMessage(error)}`)
}Also, if resultFiles is empty, consider emitting a warning like No Allure result files (*-result.json) found in directory "${resultsDirectory}" to help users who point at the wrong directory.
|
|
||
| for (const file of this.args.files) { | ||
| const fileData = readFileSync(file).toString() | ||
| const fileData = this.type === 'allure-upload' ? file : readFileSync(file).toString() |
There was a problem hiding this comment.
Semantic overloading of the Parser type's data parameter
The Parser type uses data: string to mean "file contents" for JUnit/Playwright but "directory path" for Allure. The conditional branching here and at line 144 (this.type === 'allure-upload' ? ... : ...) creates coupling that must be kept in sync.
The comment on lines 25-26 documents this, which helps. As the number of parser types grows, consider refactoring so each parser receives a uniform input (e.g., always a path, and handles its own reading), which would eliminate these type-checking conditionals. Not blocking, but worth noting as technical debt.
| const buildMessage = ( | ||
| result: AllureResult, | ||
| status: ResultStatus, | ||
| options: ParserOptions | ||
| ): string => { | ||
| const statusDetails = result.statusDetails | ||
| if (!statusDetails) { | ||
| return '' | ||
| } | ||
|
|
||
| const includeStdout = !(status === 'passed' && options.skipStdout === 'on-success') |
There was a problem hiding this comment.
statusDetails.message mapped to "stdout" and .trace mapped to "stderr" is semantically imprecise
The skipStdout option controls whether statusDetails.message (an error message) is included, and skipStderr controls statusDetails.trace (a stack trace). Neither is actually stdout or stderr. This means --skip-report-stdout on-success would suppress error messages for passing tests, which may be unintuitive.
Consider either:
- Always including
statusDetails.message(like the Playwright parser does for errors), applying skip logic only totrace - Adding a comment explaining the mapping rationale so future maintainers understand this design choice
| 1. **`ResultUploader`** — Handles the upload-to-run mechanics: | ||
| - Fetches test cases from the run, maps parsed results to them via marker matching | ||
| - Validates unmatched/missing test cases (respects `--force`, `--ignore-unmatched`) | ||
| - Uploads file attachments concurrently (max 10 parallel), then creates results in batches (max 50 per request) |
There was a problem hiding this comment.
Numbered list regression: The second item in this list changed from 2. to 1. (and the sub-items lost their nested indentation). This appears to be an accidental formatting regression.
| - Uploads file attachments concurrently (max 10 parallel), then creates results in batches (max 50 per request) | |
| 1. **`ResultUploader`** — Handles the upload-to-run mechanics: |
| ) | ||
| }) | ||
|
|
||
| describe('Allure parsing', () => { |
There was a problem hiding this comment.
Test coverage gaps to consider:
A few edge cases tested by the existing JUnit/Playwright test suites are missing from the Allure parser tests:
- Empty results directory (zero
*-result.jsonfiles) — the boundary condition whereresultFilesis empty stop < start— exercises thetimeTaken: nullbranch atallureParser.ts:118- Failed test with skip on-success — verifies message/trace are preserved for non-passing tests (parity with JUnit/Playwright skip tests)
- Asymmetric skip options — one skip option
on-success, the othernever(e.g.,skipStdout: 'on-success', skipStderr: 'never') - HTML escaping / ANSI stripping — verifies
<,>,&are escaped and ANSI codes are stripped in messages (parity with Playwright test) - No marker found — a unit test for results with no TMS links and no marker in name, verifying the raw name is used as-is
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive support for Allure test results, significantly expanding the CLI's capabilities by adding a new allure-upload command and implementing a dedicated Allure parser. While the integration is robust and handles various Allure features effectively, a critical Path Traversal vulnerability was identified in the Allure attachment parsing logic. This vulnerability could allow an attacker to exfiltrate sensitive files from the environment where the CLI tool is run (e.g., a CI/CD runner) by providing malicious Allure result files. It is strongly recommended to implement robust path validation for all attachment sources to address this security concern.
| const attachmentPaths = (parsedResult.attachments || []).map((attachment) => attachment.source) | ||
| attachmentsPromises.push({ | ||
| index, | ||
| promise: getAttachments(attachmentPaths, attachmentBaseDirectory || resultsDirectory), |
There was a problem hiding this comment.
The parseAllureResults function extracts attachment source paths from the Allure result JSON and passes them directly to getAttachments without validation. Since getAttachments (and the underlying getFile function in utils.ts) uses path.join with these paths, an attacker can provide a malicious Allure result file with directory traversal sequences (e.g., ../../etc/passwd) in the source field. This allows reading arbitrary files from the system where the CLI tool is executed and uploading them as attachments to the server.
To remediate this, ensure that all attachment paths are sanitized and verified to be within the expected results directory before processing them.
| private execRegexWithPriority(pattern: string, str: string): RegExpMatchArray | null { | ||
| // Try matching at start first | ||
| const startRegex = new RegExp(`^${pattern}`) | ||
| let match = startRegex.exec(str) | ||
| let match = str.match(startRegex) | ||
| if (match) return match | ||
|
|
||
| // Try matching at end | ||
| const endRegex = new RegExp(`${pattern}$`) | ||
| match = endRegex.exec(str) | ||
| match = str.match(endRegex) | ||
| if (match) return match | ||
|
|
||
| // Fall back to matching anywhere | ||
| const anywhereRegex = new RegExp(pattern) | ||
| return anywhereRegex.exec(str) | ||
| return str.match(anywhereRegex) |
There was a problem hiding this comment.
The change from RegExp.prototype.exec() to String.prototype.match() for non-global regular expressions is generally acceptable as they behave similarly in this context. However, RegExp.prototype.exec() is often preferred when you need to extract capture groups and iterate over matches (though not applicable here without the g flag). While str.match(regex) is valid, sticking to regex.exec(str) would maintain consistency with the RegExpExecArray return type, which is what exec explicitly returns. The current change to RegExpMatchArray is technically correct for str.match(), but it's a subtle shift in API usage.
| private execRegexWithPriority(pattern: string, str: string): RegExpMatchArray | null { | |
| // Try matching at start first | |
| const startRegex = new RegExp(`^${pattern}`) | |
| let match = startRegex.exec(str) | |
| let match = str.match(startRegex) | |
| if (match) return match | |
| // Try matching at end | |
| const endRegex = new RegExp(`${pattern}$`) | |
| match = endRegex.exec(str) | |
| match = str.match(endRegex) | |
| if (match) return match | |
| // Fall back to matching anywhere | |
| const anywhereRegex = new RegExp(pattern) | |
| return anywhereRegex.exec(str) | |
| return str.match(anywhereRegex) | |
| private execRegexWithPriority(pattern: string, str: string): RegExpExecArray | null { | |
| // Try matching at start first | |
| const startRegex = new RegExp(`^${pattern}`) | |
| let match = startRegex.exec(str) | |
| if (match) return match | |
| // Try matching at end | |
| const endRegex = new RegExp(`${pattern}$`) | |
| match = endRegex.exec(str) | |
| if (match) return match | |
| // Fall back to matching anywhere | |
| const anywhereRegex = new RegExp(pattern) | |
| return anywhereRegex.exec(str) | |
| } |
1 participant
Summary
allure-uploadcommand support with a new directory-based Allure parser (*-result.json) including status/folder/message mapping and attachment resolution.AGENTS.mdas a symlink toCLAUDE.md.Test plan
npm testnpm run checkqasphere allure-upload --run-url <run-url> ./allure-resultsMade with Cursor