Repository navigation
fix: generate portals, SDKs and plugins from projects with a large static/ folder - #457
Conversation
No server step reads it, and an image-heavy static/ pushed the upload past the size limit. portal generate now zips src/ in the action, as sdk and plugin generate do, so a failed zip names its reason instead of the generic error. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sonar counted the separate test as duplicated lines, over the 3% gate for new code. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
mehnoorsiddiqui
left a comment
mehnoorsiddiqui
left a comment
There was a problem hiding this comment.
Tested this end to end and reviewed it for code quality. The fix works, and the zip-failure message is a real improvement. Inline comments are mostly design and maintainability points. None of them block the fix.
What I verified
pnpm build,pnpm test(1970 passing) and the end-to-end tests withAPIMATIC_E2E=1(74 passing) all pass.- Production, with 26 MB of random data in
static/: ondev,portal generateandsdk generatefail with "An unexpected error occurred". On this branch both succeed, and the GIFs and logo land in the portal byte for byte.portal serveserves them too. - Same small project on
devand on this branch:- The
portal generateoutput is identical apart from build timestamps. - The SDK zip inside the portal and the SDK from
sdk generateare identical.
- The
- Branded fixture (logo and favicon in
static/): builds, and the server doesn't need them uploaded. - Locked file in
src/:devshows the generic message; this branch names the folder and theEBUSYreason. plugin generate: not fully verified. My account isn't entitled to plugin generation. This branch reached the entitlement 403 in 10 s, wheredevfailed on the upload after about 2 min, so the upload goes through, but I couldn't compare plugin output.
General points
- Plugin scope. The issue (apimatic-io#2316) proposed excluding
static/from the/portal-artifactscall only, and listed context-plugin generation as an open question. This PR also excludes it fromplugin generate. SDK output checks out (see above), but nobody has confirmed plugin output yet, so "plugins are unchanged" in the description is ahead of the evidence. Could someone with plugin access diff a plugin built with and withoutstatic/? static/is still copied, just not uploaded.srcDirZip(project-context.ts:120) andstageUpload(plugin-config-context.ts:218) copy all ofsrc/into temp before zipping. A largestatic/still costs a full disk copy. A locked file instatic/still crashessdk generatewith a rawError: EBUSY ... copyfile; I reproduced this, anddevdoes the same. Skippingstatic/at copy time would fix both. That fits naturally with the first inline comment.- Upload part changed. The portal build now goes up as
filename="<uuid>"withapplication/octet-streaminstead ofbuild.zip/application/zip. Production accepts it. The description calls it an "unnamed file"; it actually carries a UUID name. - Test gaps.
sdk generatehas no action-level test thatstatic/is left out; it relies on thesrcDirZipunit test.- Nothing pins case sensitivity or how nested paths in
excludingare matched. new AdmZip(...).getEntries().map((e) => e.entryName)now appears in three test files and could be a shared helper.
The 413 mapping (part 2 of the issue) is understood to follow separately.
🤖 Generated with Claude Code
| return await this.zipWithout(directory, []); | ||
| } | ||
|
|
||
| public async zipBuild(directory: DirectoryPath): Promise<Result<FilePath, FileProblem>> { |
There was a problem hiding this comment.
The rule for what a build upload contains is now spread across three call sites:
PreparePortalProjectActionbuilds aTempContextand zips the raw source path itself.sdk generategoes throughProjectContext.srcDirZip.plugin generatecallsstageUploadand thenzipBuild.
TempContext used to be a generic temp-file helper. With this it also knows the src/ layout, via project-layout.
The risk: the next upload site calls zip() instead of zipBuild(), and static/ quietly goes up again. Could the build be one concept on ProjectContext, say buildZip(tempDirectory, …)? All three callers would use it, static/ could be skipped at copy time (general point 2), and TempContext would stay layout-agnostic. That matches the "one domain concept per context" rule in .ai/skills/context.md.
There was a problem hiding this comment.
Done. ProjectContext.buildZip (portal, sdk) and pluginBuildZip (plugin) now make every build upload. Both copy src/ without static/ through one private step, and TempContext and ZipService are back to what they are on dev.
| // The artifacts live in this directory for as long as the caller needs them, so it wraps | ||
| // everything that reads them rather than being opened and closed around the call. | ||
| return await withDirPath(async (artifactsDirectory) => { | ||
| const build = await new TempContext(artifactsDirectory).zipBuild(sourceDirectory); |
There was a problem hiding this comment.
Nice: checking this early and reporting the real reason is much better than the old ServiceError.InvalidResponse. Small design note: the action now builds a TempContext and zips project.sourceDirectory() directly. If the build zip moved onto ProjectContext (see the comment on temp-context.ts), this would become project.buildZip(artifactsDirectory), and the action wouldn't handle the raw path.
There was a problem hiding this comment.
Done: the action calls project.buildZip(artifactsDirectory) and no longer handles the source path.
| return fs.readdirSync(dir.toString(), { withFileTypes: true }).flatMap((entry) => { | ||
| // Always use forward slashes as metadataPath — zip format requires it | ||
| const metadataPath = relativePrefix ? `${relativePrefix}/${entry.name}` : entry.name; | ||
| if (excluding.includes(metadataPath)) { |
There was a problem hiding this comment.
excluding is a list of plain strings matched against the forward-slash relative path. That has two consequences:
- Not really "top-level only": the doc comment says top-level entries, but
'content/static'would also match, and a path with backslashes never would. - Case-sensitive: a
Static/folder is still uploaded. On Windows and macOS the local build may still pick it up asstatic/(I haven't verified that), so those users could hit the old size failure.
Could the parameter say what it means, e.g. excludingTopLevel: readonly FileName[], compared against entry.name only when relativePrefix === ''? It would then match its doc comment, and the "no raw string paths" rule would be respected.
There was a problem hiding this comment.
The rule moved to the copy, where it matches top-level names only and ignores case (FileName.is), so a Static/ folder is left out too. On Windows, a project with 22 MB in Static/ now generates, and its files are still published.
| import { DirectoryPath } from '../types/file/directoryPath.js'; | ||
| import { format as f } from './format.js'; | ||
|
|
||
| export const notZippedForUpload = (directory: DirectoryPath, reason: string): string => |
There was a problem hiding this comment.
.ai/skills/prompt.md (location table) puts shared prompt utilities in src/prompts/prompt.ts. Should this live there instead of in a new one-function module?
There was a problem hiding this comment.
Moved notZippedForUpload into src/prompts/prompt.ts, and upload.ts is gone.
|
|
||
| public sdkNotZipped(sdkDirectory: DirectoryPath, problem: FileProblem) { | ||
| log.error(`The SDK in ${f.path(sdkDirectory)} could not be zipped for upload: ${problem.reason}`); | ||
| log.error(`The SDK in ${notZippedForUpload(sdkDirectory, problem.reason)}`); |
There was a problem hiding this comment.
Putting The SDK in in front of the helper's output ties this message to the helper's sentence shape: if notZippedForUpload ever starts with something other than the path, this reads wrong. Option: let the helper take a subject, or keep this one message inline.
There was a problem hiding this comment.
Back to its own inline text, as on dev.
| reportAuthorizationFailure(failure); | ||
| } | ||
|
|
||
| public sourceNotZipped(problem: FileProblem, sourceDirectory: DirectoryPath) { |
There was a problem hiding this comment.
Nit: this takes (problem, sourceDirectory), while the sibling messages srcDirNotZipped(sourceDirectory, problem) and sdkNotZipped(sdkDirectory, problem) take the directory first. I see it matches sourceProblem in this class, so take it or leave it.
There was a problem hiding this comment.
Leaving it as is: (problem, sourceDirectory) follows sourceProblem in the same class.
| const problem: FileProblem = { kind: 'zipFailed', reason: 'EACCES: permission denied' }; | ||
| const zip = sinon.stub(TempContext.prototype, 'zip').callThrough(); | ||
| zip.onSecondCall().resolves(err(problem)); | ||
| sinon.stub(TempContext.prototype, 'zip').resolves(err(problem)); |
There was a problem hiding this comment.
This now stubs every zip() call. It works only because the source zip moved to zipBuild(), and nothing here says so. If the build ever went back through zip(), this test would start failing at the wrong step and still pass for the wrong reason. Consider stubbing SdkContext/TempContext closer to the SDK zip, or add a one-line why-comment.
There was a problem hiding this comment.
Moot now: the build zip goes through zip() again, so this test is back to its dev form, which lets the first call through and fails the second one, the SDK zip.
| .asyncAndThen(() => new ResultAsync(configContext.stageUpload(tempDirectory, selection))) | ||
| .mapErr((failure): PluginGenerationProblem => ({ kind: 'configNotPrepared', failure })) | ||
| .andThen((staged) => new ResultAsync(tempContext.zip(staged))) | ||
| .andThen((staged) => new ResultAsync(tempContext.zipBuild(staged))) |
There was a problem hiding this comment.
This extends the exclusion to /plugin generation, which the issue left open ("I did not verify whether context-plugin generation reads content/"; static/ wasn't checked for plugins either). Has anyone diffed a plugin built with and without static/? I couldn't: my account isn't entitled to plugin generation. Until then I'd soften "plugins are unchanged" in the description.
There was a problem hiding this comment.
I generated a plugin from the same small project with 2.0.0, which uploads static/, and with this branch, which doesn't. The two outputs are byte-identical (69 files), and a 23 MB static/ also generates on the branch, so the description keeps "unchanged".
ProjectContext.buildZip and pluginBuildZip now make the upload for the portal, sdk and plugin commands. They copy src/ without static/ (in any letter case), following links, and report a failed copy like a failed zip, once the copy has stopped. TempContext and ZipService are back to what they are on dev, and the shared zip-failure sentence lives in prompts/prompt.ts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for the thorough pass. All of it is addressed in 55fbae3; the inline threads have the details. On the general points:
|
A test from #448 places code samples on the source directory's spec/, so the artifacts-service test keeps that directory beside the zipped build it uploads. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
portal generate,portal serve(and soquickstart),sdk generate(and sosdk publish) andplugin generateno longer uploadsrc/static/with the build. Nothing on the server reads it, and the portal build still copies it from disk.content/is still uploaded.A project whose
static/folder made the upload too large, such as one with a few large GIFs, used to fail with "An unexpected error occurred". It now generates.What changed
ProjectContextmakes it:buildZipfor the portal and the SDK, andpluginBuildZipfor the plugin, which also narrows the copy'slanguages. Each copiessrc/to a temporary folder withoutstatic/, then zips the copy. So a largestatic/is no longer copied to disk either.static/is matched in any letter case. On Windows and macOS the portal build reads aStatic/folder asstatic/, so that folder is left out too.src/used to fail every upload, and a relative file link broke in the copy thatsdk generateandplugin generatemade. Both are now uploaded as what they point to, the way the portal build reads them.portal generateandportal serveshowed "An unexpected error occurred, please try again later", andsdk generateandplugin generatecrashed with a rawEBUSYerror. No partial copy is left behind in the temporary folder.application/octet-stream, rather thanbuild.zip/application/zip. Production accepts it.How it was verified
Tests pin each part: the folders left out (any case, top level only), the links followed, a failed copy reported only once the copy has stopped, and the upload contents of each command. Each was checked by breaking the code and seeing a test fail.
The full suite passes with
APIMATIC_E2E=1.Real runs against production, against 2.0.0:
static/:portal generate,sdk generate,plugin generateportal serveserves the filesStatic/:portal generatesrc/EBUSYcrashstatic/:plugin generateKnown edge
A link inside
src/that points back atsrc/or a folder above it is followed about 63 levels deep before the copy fails withELOOP. That failure is reported with the same message. Before, such a link failed straight away.A clearer message for an upload that is still too large follows separately.
Refs apimatic/apimatic-io#2316
🤖 Generated with Claude Code