Repository navigation
build: name the source-archive ceilings in the context check (SUP2-1094) - #388
Conversation
`insta build` warned only that a large context is slow; the 256 MiB compressed
upload cap surfaced first at `insta deploy`, after packing. The context check
now measures the context against the archive lane's limits (pack.ts
ARCHIVE_LIMITS, still offline, so labelled as defaults):
- certain: over the extracted-size or entry limit, or already-compressed files
(by extension) alone over the upload limit -> "too large for insta deploy"
- possible: uncompressed size over the upload limit -> ships only if it
compresses below it; never a verdict from uncompressed bytes alone
- next action covers intentional large assets (recompress with xz/zstd, or
fetch at start-up) besides .dockerignore
--json gains `archiveLimits` ({maxArchiveBytes, maxExtractedBytes, maxFiles,
source: "default"}).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
…nsions as a certain overflow (SUP2-1094) Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/build.test.ts">
<violation number="1" location="test/build.test.ts:151">
P3: Keep a positive integration assertion here: this test should verify that `contextCheck` surfaces the already-compressed estimate, not only that it avoids calling it definite. Otherwise the archive-risk detail can disappear while the test still passes because the context is generally oversized.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| expect(f.over).toBeUndefined() | ||
| expect(f.maybe).toMatch(/already-compressed files alone are 300\.0 MiB.*likely not fit/) | ||
| const c = contextCheck({ ...base, totalBytes: 320 * MiB, incompressibleBytes: 300 * MiB }, limits) | ||
| expect(c.detail).not.toContain('too large for') |
There was a problem hiding this comment.
P3: Keep a positive integration assertion here: this test should verify that contextCheck surfaces the already-compressed estimate, not only that it avoids calling it definite. Otherwise the archive-risk detail can disappear while the test still passes because the context is generally oversized.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At test/build.test.ts, line 151:
<comment>Keep a positive integration assertion here: this test should verify that `contextCheck` surfaces the already-compressed estimate, not only that it avoids calling it definite. Otherwise the archive-risk detail can disappear while the test still passes because the context is generally oversized.</comment>
<file context>
@@ -143,9 +143,23 @@ describe('archive ceiling (SUP2-1094)', () => {
+ expect(f.maybe).toMatch(/already-compressed files alone are 300\.0 MiB.*likely not fit/)
const c = contextCheck({ ...base, totalBytes: 320 * MiB, incompressibleBytes: 300 * MiB }, limits)
- expect(c.detail).toMatch(/too large for `insta deploy <dir>`: already-compressed files alone are 300\.0 MiB/)
+ expect(c.detail).not.toContain('too large for')
+ expect(c.status).toBe('fail')
+ })
</file context>
| expect(c.detail).not.toContain('too large for') | |
| expect(c.detail).toMatch(/already-compressed files alone are 300\.0 MiB.*likely not fit/) |
jwfing
left a comment
jwfing
left a comment
There was a problem hiding this comment.
verdict=request_changes
Reviewed full head commit: 978486979dc5db1d28d94ea8471922aa455f4d2d (base/merge-base d075ab784457fb9681be09a241fc162b3a783a25).
GitHub event: COMMENT. This is a quality verdict only; no APPROVE or REQUEST_CHANGES event is being submitted.
Scope: full PR diff (src/commands/build.ts, test/build.test.ts), build report/rendering, context walking/ignore rules, pack.ts tar construction and limit enforcement, deploy lane discovery/server limits, pinned fflate compressor, surrounding tests and repository conventions. Assessed against the SUP2-1094 requirements supplied in the review request and PR description: offline upload/extracted/entry warnings, default-sourced JSON limits, no certainty from raw size/extensions, intentional-assets advice, and parity with actual packing.
Findings
Critical (2)
src/commands/build.ts:121: Critical — The new overhead estimate still misses actual near-cap archives.pack.tsuses fflate 0.8.3gzipSync(..., {level: 9}). Its compressor flushes on symbol thresholds (including >24,576 symbols), so incompressible input can incur stored-block framing more frequently than once per 65,535 bytes. The level-0 stored-block interval is not a safe bound for this level-9 output. In a real directory containing one randomdata.binof 268,405,456 bytes,archiveOverheadBytesreturned 22,559 and the total estimate was 268,428,015, below the 268,435,456 default cap;archiveFitreturned{}. Packing that same directory produced 268,460,038 bytes, and defaultpackDirectoryrejected it as too large. Thus build still omits the upload ceiling for a context deploy rejects. Use a conservative bound for the actual pinned compressor (over the tar input), or an explicit near-limit uncertainty margin, and add a regression comparing the estimate with actual packing. This is the remaining case of the prior overhead finding, not resolved by counting tar headers alone.src/commands/build.ts:133(also entry branch at 130–131): Critical — Archive certainty is computed from the wrong ignore semantics. These limits are checked againstcontextStats, which callscontextEntries: it deliberately ignores only.dockerignore. However,packDirectoryfalls back to root and nested.gitignorewhen.dockerignoreis absent. A directory withDockerfile,.gitignorecontaininglocal.bin, and an ignored sparselocal.binof 1 GiB + 1 byte gets “too large forinsta deploy <dir>… over the 1024.0 MiB extracted-size limit” from build. Actual default packing succeeds with 2 entries, 47 extracted bytes, and a 159-byte archive. The same mismatch affects the entry cap and upload estimates. The generic Docker/Fly context scan predates this PR, but applying archive-specific certain-failure claims to it is new. Compute archive-limit statistics using the packer's ignore selection; keep any Docker/Fly context diagnostics separate. Cover the no-.dockerignore/.gitignorefallback in a pack-versus-build regression.
Suggestion (1)
src/commands/build.ts:157andrenderReport(lines 294–322): Suggestion — Label the human-readable ceilings as offline defaults.archiveLimits.source = "default"correctly labels JSON, but normalinsta buildoutput prints the ceiling and “too large forinsta deploy <dir>” with no default/offline or archive-lane qualification.renderReportnever prints the JSON metadata, and deploy actually uses the limits returned bydiscoverLane. Show a short qualifier that these are offline archive defaults and negotiated server limits win, ideally once per report; verify the rendered output as well as JSON. This would preserve the requested default-limit explanation for users who do not pass--json.
Information (0)
Previous feedback and requirement coverage
- Prior cubic extension/certain-overflow finding is addressed: upload branches return
maybe, notover. A real repetitive 300 MiBstored.zippacked successfully to 307,065 bytes and received only the qualified estimate. Large raw payloads likewise do not become certain upload failures. - Prior cubic overhead finding is only partially addressed: tar headers/padding/end blocks are included conservatively, but the compressor framing assumption still permits the reproduced near-limit miss above.
- JSON retains
{maxArchiveBytes: 268435456, maxExtractedBytes: 1073741824, maxFiles: 10000, source: "default"}. Intentional-assets guidance includes xz/zstd or fetching from storage at start-up. No network discovery was introduced in build. - Existing cubic feedback on the same head was checked before publication; its separate test-assertion suggestion is not repeated here.
Verification actually executed
- Dependency setup:
npm ci --ignore-scripts --no-audit --no-fund. npm run typecheck: passed.npm test -- test/build.test.ts test/pack.test.ts test/pack-ignore.test.ts: 3 files / 172 tests passed.npm test: 107 files passed; 2268 tests passed, 2 skipped.- Scratch-directory execution of actual
contextStats,contextCheck,buildReport,renderReport,jsonReport, andpackDirectory: reproduced ignored-file false certainty and actual near-cap gzip overflow, verified default rejection, and verified repetitive-extension content remains packable. No implementation or tests were edited.
Limits
The repository has no docs/ or docs/superpowers/ (including specs/plans). Read AGENTS.md, CONTRIBUTING.md, and the referenced development guide. The linked Linear issue was inaccessible; its requirements were taken from the supplied review request and PR description. Read the currently published companion CLI reference, but did not verify the claimed companion documentation PR. Linux/Node verification only; no Windows runtime, production resources, or live deploy/upload were used.
Head was rechecked immediately before submission; this verdict applies only to the full SHA above. A later head requires review of its changes.
| // framing (5 bytes per 64 KiB) for data that does not compress. pack.ts compresses the tar, not the | ||
| // bare files, so the upload limit applies to totalBytes plus this. | ||
| export function archiveOverheadBytes(ctx: ContextStats, payloadBytes = ctx.totalBytes): number { | ||
| return ctx.entries * 1023 + 1024 + Math.ceil(payloadBytes / 65535) * 5 + 32 |
There was a problem hiding this comment.
Critical — The new overhead estimate still misses actual near-cap archives. pack.ts uses fflate 0.8.3 gzipSync(..., {level: 9}). Its compressor flushes on symbol thresholds (including >24,576 symbols), so incompressible input can incur stored-block framing more frequently than once per 65,535 bytes. The level-0 stored-block interval is not a safe bound for this level-9 output. In a real directory containing one random data.bin of 268,405,456 bytes, archiveOverheadBytes returned 22,559 and the total estimate was 268,428,015, below the 268,435,456 default cap; archiveFit returned {}. Packing that same directory produced 268,460,038 bytes, and default packDirectory rejected it as too large. Thus build still omits the upload ceiling for a context deploy rejects. Use a conservative bound for the actual pinned compressor (over the tar input), or an explicit near-limit uncertainty margin, and add a regression comparing the estimate with actual packing. This is the remaining case of the prior overhead finding, not resolved by counting tar headers alone.
| if (ctx.entries > limits.maxFiles) { | ||
| return { over: `${ctx.entries.toLocaleString('en-US')}${ctx.truncated ? '+' : ''} entries (directories count), over the ${limits.maxFiles.toLocaleString('en-US')}-entry archive limit` } | ||
| } | ||
| if (ctx.totalBytes > limits.maxExtractedBytes) { |
There was a problem hiding this comment.
Critical — Archive certainty is computed from the wrong ignore semantics. These limits are checked against contextStats, which calls contextEntries: it deliberately ignores only .dockerignore. However, packDirectory falls back to root and nested .gitignore when .dockerignore is absent. A directory with Dockerfile, .gitignore containing local.bin, and an ignored sparse local.bin of 1 GiB + 1 byte gets “too large for insta deploy <dir> … over the 1024.0 MiB extracted-size limit” from build. Actual default packing succeeds with 2 entries, 47 extracted bytes, and a 159-byte archive. The same mismatch affects the entry cap and upload estimates. The generic Docker/Fly context scan predates this PR, but applying archive-specific certain-failure claims to it is new. Compute archive-limit statistics using the packer's ignore selection; keep any Docker/Fly context diagnostics separate. Cover the no-.dockerignore/.gitignore fallback in a pack-versus-build regression.
| : `${size}${tooBig ? ' — large contexts make remote builds slow' : ''}` | ||
| const bad = shipsNodeModules || tooBig || ctx.truncated | ||
| : `${size}${tooBig && !fit.over && !fit.maybe ? ' — large contexts make remote builds slow' : ''}` | ||
| const ceiling = fit.over ? `too large for \`insta deploy <dir>\`: ${fit.over}` : fit.maybe |
There was a problem hiding this comment.
Suggestion — Label the human-readable ceilings as offline defaults. archiveLimits.source = "default" correctly labels JSON, but normal insta build output prints the ceiling and “too large for insta deploy <dir>” with no default/offline or archive-lane qualification. renderReport never prints the JSON metadata, and deploy actually uses the limits returned by discoverLane. Show a short qualifier that these are offline archive defaults and negotiated server limits win, ideally once per report; verify the rendered output as well as JSON. This would preserve the requested default-limit explanation for users who do not pass --json.
…und gzip framing, label defaults (SUP2-1094) Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
2 issues found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/commands/build.ts">
<violation number="1" location="src/commands/build.ts:117">
P3: `contextStats` walks every file twice when `.dockerignore` exists, even though both functions use the same archive selection. Reuse the first tally for that case.</violation>
</file>
<file name="src/pack.ts">
<violation number="1" location="src/pack.ts:202">
P2: `archiveEntries` discards the packer’s symlink errors, so `insta build` can report a clean context even though the corresponding deploy always fails. Preserve the link list and surface `linkError` (or expose the condition to the context check).</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
View guided diff | Turn on auto-fix | Re-trigger cubic
| export function archiveEntries(absDir: string, cap = Infinity): Found[] { | ||
| const entries: Found[] = [] | ||
| const { ig, files, flavour } = rootIgnore(absDir) | ||
| walk(absDir, '', entries, [], ig, files, flavour, cap) |
There was a problem hiding this comment.
P2: archiveEntries discards the packer’s symlink errors, so insta build can report a clean context even though the corresponding deploy always fails. Preserve the link list and surface linkError (or expose the condition to the context check).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/pack.ts, line 202:
<comment>`archiveEntries` discards the packer’s symlink errors, so `insta build` can report a clean context even though the corresponding deploy always fails. Preserve the link list and surface `linkError` (or expose the condition to the context check).</comment>
<file context>
@@ -193,6 +193,16 @@ export function contextEntries(absDir: string, cap = Infinity): Found[] {
+export function archiveEntries(absDir: string, cap = Infinity): Found[] {
+ const entries: Found[] = []
+ const { ig, files, flavour } = rootIgnore(absDir)
+ walk(absDir, '', entries, [], ig, files, flavour, cap)
+ return entries
+}
</file context>
| walk(absDir, '', entries, [], ig, files, flavour, cap) | |
| const links: { path: string; target: string }[] = [] | |
| walk(absDir, '', entries, links, ig, files, flavour, cap) | |
| if (links.length) throw linkError(links) |
| const { stats, nodeModulesBytes } = tally(contextEntries(dir, cap + 1), cap) | ||
| return { | ||
| ...stats, | ||
| nodeModulesBytes, | ||
| hasDockerignore: existsSync(join(dir, '.dockerignore')), | ||
| archive: tally(archiveEntries(dir, cap + 1), cap).stats, | ||
| } |
There was a problem hiding this comment.
P3: contextStats walks every file twice when .dockerignore exists, even though both functions use the same archive selection. Reuse the first tally for that case.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/commands/build.ts, line 117:
<comment>`contextStats` walks every file twice when `.dockerignore` exists, even though both functions use the same archive selection. Reuse the first tally for that case.</comment>
<file context>
@@ -100,33 +110,39 @@ export function contextStats(dir: string, cap = WALK_CAP): ContextStats {
+}
+
+export function contextStats(dir: string, cap = WALK_CAP): ContextStats {
+ const { stats, nodeModulesBytes } = tally(contextEntries(dir, cap + 1), cap)
return {
- totalBytes,
</file context>
| const { stats, nodeModulesBytes } = tally(contextEntries(dir, cap + 1), cap) | |
| return { | |
| ...stats, | |
| nodeModulesBytes, | |
| hasDockerignore: existsSync(join(dir, '.dockerignore')), | |
| archive: tally(archiveEntries(dir, cap + 1), cap).stats, | |
| } | |
| const hasDockerignore = existsSync(join(dir, '.dockerignore')) | |
| const context = contextEntries(dir, cap + 1) | |
| const { stats, nodeModulesBytes } = tally(context, cap) | |
| return { | |
| ...stats, | |
| nodeModulesBytes, | |
| hasDockerignore, | |
| archive: hasDockerignore ? stats : tally(archiveEntries(dir, cap + 1), cap).stats, | |
| } |
jwfing
left a comment
jwfing
left a comment
There was a problem hiding this comment.
verdict=approved
Reviewed full head SHA: dda44f39ea8ac3a52b5f1c4954591c27c5683a0f.
Base/merge-base: d075ab784457fb9681be09a241fc162b3a783a25.
Review round: 2/10.
GitHub event: COMMENT. “approved” is this reviewer's quality judgment only; this review does not grant GitHub approval. Human approval is still required.
Scope and findings
Independently reviewed the complete PR change against SUP2-1094 and rechecked every finding from round 1. Scope includes build statistics/report/rendering/JSON, the new archive walker, packer ignore selection, tar construction, pinned fflate compression, deploy lane/server-limit discovery, and related tests and repository conventions.
- Critical: 0
- Suggestion: 0
- Information: 0
No actionable issues found in this reviewed scope. This does not guarantee the absence of defects.
Resolution of previous findings
-
Near-limit gzip overhead miss: addressed. Repeated the real-directory case with a single random file of exactly 268,405,456 bytes. Actual
packDirectoryoutput was 268,460,040 bytes, over the 268,435,456-byte default cap. The new estimate was 268,676,935 bytes andarchiveFitcorrectly returnedmaybe, without claiming certain failure.Independently checked more than the empirical random-data ratio: the installed, exactly pinned fflate 0.8.3 implementation's
gzipSyncpath callsdfltwith a 10-byte gzip header and 8-byte trailer, allocating at mostt + 5*(1 + ceil(t/7000)) + 18bytes for tar input lengtht; its final slice cannot exceed that buffer. The newceil(t/1000) + 1024compression allowance exceeds that allocation overhead for every nonnegative integralt: checked the first 7000-byte period (minimum spare margin 997 bytes), and each following period increases the spare margin by 2 bytes. The tar estimate also conservatively covers each header/padding plus the closing blocks. Thus the current allowance has a source-based upper-bound justification for this pinned implementation, beyond its measured ~0.02% random-data expansion. It is not a general guarantee for future compressors or dependency changes. -
Wrong ignore semantics for archive certainty: addressed.
archiveEntriessharesrootIgnoreandwalkwithpackDirectory;contextChecknow evaluates archive ceilings using those archive statistics. Repeated the root.gitignoreregression with an excluded sparse 1 GiB + 1-byte file: archive statistics matched the successful pack (2 entries, 47 extracted bytes, 159 compressed bytes), with no “too large for” archive claim. Also verified nested.gitignorefallback and root.dockerignoreprecedence against actual packing. The existing generic Docker-context size warning remains separate from archive certainty. -
Human-readable offline-default qualification: addressed. Ran
buildReportfollowed by actualrenderReport, not just a check-detail assertion. The rendered upload warning includes[archive limits are offline defaults — the server's limits win at deploy], and the next action includes xz/zstd and fetching intentional assets at start-up. Verified the qualifier also appears for the extracted-size warning.jsonReportretains all three default ceilings withsource: "default".
The earlier cubic extension/certain-failure issue also remains fixed: a real 300 MiB repetitive stored.zip packed successfully to 307,146 bytes and was classified only as maybe. Neither raw size nor extension-matched bytes alone causes a certain upload failure.
Verification actually executed
npm run typecheck: passed.npm test: 107 test files passed; 2271 tests passed, 2 skipped.- Independent scratch-directory assertions using actual
contextStats,archiveEntries,archiveFit,packDirectory,buildReport,renderReport, andjsonReport: all passed. - Compared actual packing with the estimate for 6 MiB random, zero-filled, low-alphabet, and mixed random/zero data. All outputs were below the estimate. A limit one byte below each actual archive produced
maybe; a limit 1% above the random archive produced no archive warning. - Tested actual 10,001-entry and >1 GiB extracted-size directories: both archive diagnostics and the packer's early rejection agreed.
- Repeated the full-size near-upload-cap regression, root/nested ignore selection, Docker-ignore precedence, rendering/default-limit JSON, and compressible
.zipbehavior described above.
Verification used Linux/Node and local scratch data; no production resources, live upload/deploy, implementation edits, or merges were used.
Review limits
The repository has no docs/ or docs/superpowers/ directory (including specs/plans). Repository instructions and development guidance were consulted. The linked Linear issue was inaccessible in the preceding round; requirements were taken from the supplied unchanged SUP2-1094 acceptance criteria and PR description. The claimed companion documentation PR was not verified. No Windows runtime was exercised.
Head was checked immediately before submission. This quality judgment applies only to the full SHA above; later commits need their own review.
Fixes SUP2-1094.
insta build --jsonsaidneeds-attentionwith only a "large context" warning at 315.3 MB. The 256 MiB compressed upload cap first showed up atinsta deploy, after packing (307.7 MiB > 256.0 MiB).What changes
The
contextcheck ininsta buildnow measures the context against the archive lane's limits (pack.tsARCHIVE_LIMITS). Build stays offline, so these are labelled as defaults; the server's negotiated figures still win at deploy.insta deploy <dir>: ….gz/.xz/.zip/images/video…) alone, or payload plus tar headers/padding, over the upload limitUncompressed bytes alone never make the call, because a large text tree can pack small. When a ceiling applies, the next action also covers intentional large assets: recompress with xz/zstd, or fetch the file at start-up. That's the reporter's case: a checkpoint they ship on purpose.
--jsongainsarchiveLimits: {maxArchiveBytes, maxExtractedBytes, maxFiles, source: "default"}.Verification
npm run typecheckpasses.test/build.test.tspasses 41/41, including new cases: 315 MB reported case, compressible vs incompressible, extracted/entry limits, custom limits, compressed-file accounting.checkpoint.gz→too large for insta deploy <dir>: already-compressed files alone are 300.0 MiB, over the 256.0 MiB compressed upload limitdata.bin→ships only if it compresses below thatnpm test:test/setup-agent.test.tsfails 12 tests withApiError: unauthorized. It fails the same way on unmodifiedmainon my machine (local login state), so this PR doesn't cause it.Docs: skills
insta/cli-reference.mdupdated in a companion PR.🤖 Generated with Claude Code
Open in [code]smith · Need help on this PR? Tag
@codesmith-botwith what you need.Summary by cubic
The
insta buildcontext check now flags contexts that exceed the deploy archive ceilings at build time, instead of surfacing only atinsta deployafter packing..dockerignore, else.gitignorefallback), not the docker context, and count tar headers and gzip framing along with the payload.insta deploy <dir>. Uncompressed size alone never triggers a certain verdict, since compressible trees can pack well below the cap..gz,.xz,.zip, images, video, etc.) alone over the upload cap → warning that it ships only if it compresses below the cap (extension-based estimate; stored files can still shrink), with guidance to recompress intentional large assets with xz/zstd or fetch them at start-up.--jsongainsarchiveLimitswith the default ceilings, labelled as such since build runs offline and the server's negotiated figures still win at deploy.Written for commit dda44f3. Summary will update on new commits.