Add public directory submission preflight - #27
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a8dd3e5b0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return Object.entries(value).every(([key, item]) => { | ||
| const childContext = context === "css" || key === "style" || key === "@_style" | ||
| ? "css" | ||
| : key === "@_href" || key === "@_xlink:href" ? "href" : undefined; | ||
| return inspectSvgValue(item, childContext); |
There was a problem hiding this comment.
When a logo or composer icon contains an inline <script>, an onload handler, or foreign HTML content, this walker only applies special validation to style and href keys and accepts every other element and attribute. svgDimensions therefore returns dimensions and the preflight can mark an executable SVG as safe; reject scripting/foreign-content elements and event-handler attributes before approving the asset.
Useful? React with 👍 / 👎.
| } else if (chunkType === "VP8L") { | ||
| if (chunkLength < 5 || buffer[payload] !== 0x2f) return null; | ||
| const packed = view.getUint32(payload + 1, true); | ||
| detected = { width: (packed & 0x3fff) + 1, height: ((packed >>> 14) & 0x3fff) + 1 }; |
There was a problem hiding this comment.
When a VP8L file contains only the five-byte signature/dimension header and no encoded image data, this branch still returns dimensions, causing rasterAsset to accept a corrupt WebP. The same malformed asset will fail in a real image decoder or the submission portal, so the preflight should validate the complete bitstream rather than treating its header as a successful decode.
Useful? React with 👍 / 👎.
| type TargetType = SubmissionPreflightReport["targetType"]; | ||
|
|
||
| const packageNamePattern = /^[A-Za-z0-9][A-Za-z0-9_-]{0,63}$/; | ||
| const semverPattern = /^(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(?:-[0-9A-Za-z-]+(?:\.[0-9A-Za-z-]+)*)?(?:\+[0-9A-Za-z-]+(?:\.[0-9A-Za-z-]+)*)?$/; |
There was a problem hiding this comment.
When the manifest version is, for example, 1.0.0-01, this regex accepts it even though SemVer forbids leading zeroes in numeric prerelease identifiers. Such a package can receive a passing preflight despite having the invalid semantic version that this check claims to reject, so use strict SemVer parsing or distinguish numeric prerelease identifiers in the pattern.
Useful? React with 👍 / 👎.
PR Summary by QodoAdd offline public directory submission preflight (v1.59.0)
AI Description
Diagram
High-Level Assessment
Files changed (29)
|
Code Review by Qodo
1. Duplicate findings dropped
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| const findings = new Map<string, SubmissionFinding>( | ||
| report.findings.map((finding) => [finding.id, finding]) | ||
| ); |
There was a problem hiding this comment.
1. Duplicate findings dropped 🐞 Bug ≡ Correctness
The submission text/Markdown renderers de-duplicate findings by finding.id, so multiple findings sharing the same id overwrite each other and the report can hide distinct failures (JSON output is unaffected). This commonly happens for missing required assets and multiple URL/unknown-field issues, causing misleading “fix one thing” output.
Agent Prompt
### Issue description
`findingsByCheck()` builds a `Map` keyed by `finding.id`, which assumes ids are unique. The submission preflight can legitimately emit multiple findings with the same id (e.g., one per missing asset field), so text/Markdown reports drop/overwrite earlier findings and may repeat the last instance.
### Issue Context
- The JSON renderer (`renderSubmissionPreflightJson`) serializes `report.findings` directly, so it preserves duplicates.
- Only the human-readable renderers (via `findingsByCheck`) are impacted.
### Fix
Update `findingsByCheck()` to preserve *instances*, not just ids. A minimal fix is to map `id -> SubmissionFinding[]` and consume entries in order when iterating `check.findingIds` (queue semantics), rather than mapping `id -> SubmissionFinding`.
### Fix Focus Areas
- src/reporting/render-submission-report.ts[11-25]
- src/core/submission-preflight.ts[191-205]
- src/core/submission-assets.ts[353-356]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if (dimensions.width < minimumDimension || dimensions.width > maximumDimension) { | ||
| return finding("plugin.submission.asset.dimensions", "Asset dimensions are outside the allowed range.", { ...evidence, limit: minimumDimension }); | ||
| } |
There was a problem hiding this comment.
2. Misleading dimension limit 🐞 Bug ◔ Observability
When an asset is outside the allowed dimension range, the finding evidence always reports `limit: minimumDimension` even when the asset is too large. This produces misleading evidence for oversized assets and complicates debugging/automation consuming the evidence.
Agent Prompt
### Issue description
`dimensionFinding()` rejects dimensions below `minimumDimension` or above `maximumDimension`, but the evidence always sets `limit: minimumDimension`. For upper-bound failures, the evidence misrepresents the violated constraint.
### Issue Context
This affects both raster and SVG asset validation paths, since both funnel through `dimensionFinding()`.
### Fix
Emit accurate bounds in evidence, e.g. `{ min: minimumDimension, max: maximumDimension }`, or conditionally set `limit` to the bound that was violated (min when too small, max when too large).
### Fix Focus Areas
- src/core/submission-assets.ts[11-15]
- src/core/submission-assets.ts[339-350]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
1 participant
Adds the offline doctor submission preflight, bounded listing/asset/skill validation, opt-in GitHub Action reports, and v1.59.0 release metadata. Manual portal review remains explicit. Verification: npm run release-check passed with 1,271 tests and 6 environment-gated skips, build, package audit, fresh install, and publish dry-run.