Fix #216: extension-retirement pack hook no longer packs a 27MB tarball - #218

Merged
pseudoseed merged 2 commits into
mainfrom
builder/bugfix-216-pack-hook-timeout
Aug 29, 2026
Merged

Fix #216: extension-retirement pack hook no longer packs a 27MB tarball#218
pseudoseed merged 2 commits into
mainfrom
builder/bugfix-216-pack-hook-timeout

Conversation

@pseudoseed

Copy link
Copy Markdown
Owner

Fixes#216.

main went red at 4ebaf36b6 on one suite: extension-retirement.test.ts, whose beforeAll hook timed out at 10000ms after taking 10544ms. 341 files and 6,779 tests passed, and the other five CI jobs were green. Nothing was wrong with the shipped code.

Root Cause

#215 added a Build codev package step to the unit job, so dist/, dashboard-dist/ and v2-dist/ exist when the unit suite runs. They did not before. The root package.json is files: ["*", "!apps/streamdeck", "!apps/vscode"], and that test packs the workspace root — so the pack now sweeps the built output too.

Measured with npm pack --dry-run --json rather than estimated: the build step adds 805 files and 12.4 MB to a root tarball that is now 4,395 files and 27.4 MB packed. A hook already sitting near its ceiling went 544ms over it.

The build step stays. Without it the publish-scrub guard narrows to committed files and passes vacuously, which its own comment says. The test is what changes.

Fix — all three defects in #216

1. The pack was far more work than the assertion needed. It wrote a real 27.4 MB tarball and shelled out to tar -tzf purely to read a list of names. npm pack --dry-run --json returns that list with no tarball written and no tar process — the same idiom #214's publish-scrub guard already uses. Paths come back without the package/ prefix, so the four assertions match on bare repo-relative paths.

Raising a timeout on work that does not need doing would have been the weakest fix available: the tarball grew 12.4 MB this week alone, and the next 12 MB lands right back here.

2. The expensive fixture was in beforeAll, but only 1 of 5 tests used it. The other four read files or run pnpm list and need nothing from the pack. When the hook died, all five were reported skipped — one slow fixture took down four tests that never depended on it. The pack is now a lazy call inside the single test that needs it.

3. There was no explicit timeout. It ran on vitest's 10s default. Now 60s, with the reason beside it: ~4.7s warm locally, roughly 12x headroom for a cold runner and ordinary repo growth, still short enough that a genuine hang fails rather than hanging the job.

The helper also throws on an empty file list, since an empty list satisfies both not.toContain assertions without proving anything.

The 12 unexplained skips, since a number I gave was wrong

Reporting the CI failure, I wrote that "17 tests in that file went to skipped." That file has 5 tests, and CI's own line says (5 tests | 5 skipped). The totals reconciled — local 6,796 + 51 and CI 6,779 + 68 both make 6,847 — but only 5 of the 17 were extension-retirement's. Twelve tests changed state between machine and runner and I attributed them to a file that could not account for them. That is the same defect as reporting a value you are not positioned to observe.

They are found, and they reconcile exactly. CI's per-file skip lines:

FileSkipped in CI
session-manager.test.ts12
tunnel-integration.test.ts21
tunnel-e2e.test.ts13
non-main-default-branch.test.ts8
extension-retirement.test.ts5
default-branch.test.ts4
team-cli.test.ts2
spec-146-t3-contract.test.ts1
spec-146-phase-9-porch-engine.test.ts1
spec-146-phase-9-live-harness.test.ts1
Total68

All 12 are session-manager, and they are CI-gated rather than environmental drift: one describe.skipIf(!!process.env.CI) block holding 5 tests, plus 7 individual it.skipIf(!!process.env.CI) at lines 1218, 1269, 1331, 2077, 2115, 2149, 2187 — counted from the source by brace-matching the block. They spawn real shellper processes and need the node-pty native binding, which a CI child process cannot resolve. They run locally, where CI is unset, and skip in CI. That is exactly why they moved state between the two.

The other 51 are unconditional describe.skip or env-gated and skip in both places — which is the local count exactly.

So 51 + 12 + 5 = 68 in CI, and 51 locally with those 12 and 5 passing gives 6,796. Every number closes, and there is no second failure hiding in the remainder.

One off-subject commit rides along, deliberately

a725af68f — chore(porch): bugfix-214 protocol complete is on this branch and is not about #216. It touches one file, codev/projects/bugfix-214-pre-publish-scrub-a-home-direc/status.yaml, 2 lines.

Porch writes its state commits after a PR merges, onto whatever branch is checked out — and by the time porch done bugfix-214 ran, this branch was already cut from 4ebaf36b6 to fix the red. The bookkeeping is true: #214's protocol is complete, its leak is fixed and on main, and #216 is a separate defect with its own issue rather than unfinished #214 work. It is also porch's own commit, which is the only way status.yaml is allowed to change.

Carrying it here rather than spending a whole PR cycle on 2 lines of bookkeeping. Recorded so nobody loses time next month asking why a #214 commit sits on a #216 branch.

Test Plan

  • Rewritten suite: 5 passed, 4.91s (was a 10s-default beforeAll that took 10.5s in CI).
  • Assertions verified to have teeth, not just to pass: the resolved list is 4,395 entries, carries apps/web/package.json and apps/v2/package.json, has 0 entries under apps/vscode/ or apps/streamdeck/, and no path carries the package/ prefix.
  • Full packages/codev unit suite on a stable HEAD (bc7772121, clean tree): 342 files passed, 3 skipped; 6,796 tests passed, 51 skipped, 0 failed.

An earlier full run showed 1 failure and is not quoted here, because its preconditions were violated rather than because it was inconvenient: spec-1280-measurement-instrument's T12 — determinism compares two instrument invocations byte-for-byte, and porch done bugfix-214 wrote a state commit during that run, so the two invocations stamped different commits (a725af68f vs 4ebaf36b6). Self-inflicted concurrency, not the code and not the test. Both affected files were then re-run with HEAD stable — 2 files, 29 tests, all passing — before the clean full run above.

That hazard is filed separately as #217 and deliberately not touched here: a determinism test whose title states a precondition it never checks reports "I could not tell" with the same signal as "the instrument is non-deterministic".

pseudoseedand others added 2 commits August 29, 2026 14:57
`main` went red at 4ebaf36 on this one suite: the `beforeAll` hook timed
out at 10000ms after taking 10544ms.
#215's `Build codev package` step means `dist/`, `dashboard-dist/` and
`v2-dist/` now exist when the unit suite runs. The root package.json is
`files: ["*", …]` and this test packs the WORKSPACE ROOT, so the pack sweeps
the built output too — measured at 805 files and 12.4 MB added to a root
tarball that is now 4,395 files and 27.4 MB packed. A hook already near its
ceiling went 544ms over.
The build step stays. Without it the publish-scrub guard narrows to committed
files and passes vacuously, which its own comment says. The test changes.
All three defects, not just the one that was red:
The pack was far more work than the assertion needed — it wrote a real
tarball and shelled out to `tar -tzf` purely to read a list of names.
`npm pack --dry-run --json` returns that list with no tarball and no `tar`
process, the same idiom #214's publish-scrub guard uses. Paths come back
without the `package/` prefix, so the assertions match bare repo-relative
paths. Raising a timeout on work that does not need doing would have been the
weakest fix available: the tarball grew 12.4 MB this week alone.
The fixture was in `beforeAll` but only 1 of 5 tests used it, so when the hook
died all five were reported skipped — one slow fixture took down four tests
that never depended on it. The pack is now lazy, inside the test that needs it.
There was no explicit timeout. Now 60s, with the reason beside it.
The helper throws on an empty file list: an empty list satisfies both
`not.toContain` assertions without proving anything.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@pseudoseed

Copy link
Copy Markdown
OwnerAuthor

Architect review — approved on my read

Risk: low. 2 files, 51 additions / 29 deletions, one test file plus 2 lines of porch
bookkeeping. No shipped code. Read by me directly rather than sent to consultation; the triage
table puts this tier at architect-read, and spending three reviewer lanes on a test-timeout fix
while main is red would cost more than it returns.

I pre-reviewed this diff in the worktree before it was pushed, so this is not a rubber stamp on
the description.

Verified independently

  • --ignore-scripts does not change what the pack contains here. That flag would alter the
    file list if the root package had a prepack. It has no prepack, prepare,
    prepublishOnly or postpack. Checked the root package.json directly. The builder checked
    the same thing from its own direction and we agree — two derivations, one answer.
  • The path-shape change is correct.npm pack --dry-run --json returns files[].path
    relative to the package root, with no package/ prefix, so dropping it from all four
    assertions is right and not an oversight.
  • The skip reconciliation. I brace-matched the describe.skipIf(!!process.env.CI) at
    session-manager.test.ts:671: it spans lines 671-855 and holds exactly 5 tests, plus the 7
    it.skipIf at the lines named in the body. 12 exactly. 51 + 12 + 5 = 68 closes on both
    sides.
  • The root cause. Confirmed from the merge diff of Fix #214: no home-directory path reaches a published tarball #215 and the root files: ["*", …], not
    from the summary.

The part that matters most

The helper throws on an empty file list, because an empty list satisfies both not.toContain
assertions without proving anything. That is the same property as #214's publish-scrub guard: a
check that cannot pass vacuously. It was not in the three items I asked for, and it is the
difference between a test that verifies something and a test that reads green.

The 60s timeout also carries what it was measured against and why 12x, so whoever has to raise
it next knows what they are trading. A bare number would have been a guess with a suit on.

On the fix that was not chosen

Raising the hook timeout alone would have gone green today. The tarball grew 12.4 MB this week;
the next 12 MB lands back here. Removing work that never needed doing is the fix, and the tool
that measured the regression — npm pack --dry-run --json — was already the answer.

Correction folded in

The builder's first CI report attributed 17 skipped tests to a file containing 5. It found the
remaining 12, named the mechanism (node-pty native binding unresolvable in a CI child), and put
the full accounting in the PR body rather than leaving a number that did not add up. Recorded
here because the correction is part of the record, not a footnote to it.

Out of scope, correctly

The spec-1280 determinism failure hit during this work is not in this PR. It was a moved
HEAD — a porch done committed state mid-suite — and it is filed as #217, where the real
defect is that the test reports "not deterministic" for a case where it could not measure
determinism at all.

Approved. Merge on green CI, --merge, never squash.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

main is red: extension-retirement pack hook exceeds its 10s timeout after #215's build step

1 participant

@pseudoseed
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Fix #216: extension-retirement pack hook no longer packs a 27MB tarball - #218

Merged
pseudoseed merged 2 commits into
mainfrom
builder/bugfix-216-pack-hook-timeout
Aug 29, 2026
Merged

Fix #216: extension-retirement pack hook no longer packs a 27MB tarball#218
pseudoseed merged 2 commits into
mainfrom
builder/bugfix-216-pack-hook-timeout

Conversation

@pseudoseed

Copy link
Copy Markdown
Owner

Fixes#216.

main went red at 4ebaf36b6 on one suite: extension-retirement.test.ts, whose beforeAll hook timed out at 10000ms after taking 10544ms. 341 files and 6,779 tests passed, and the other five CI jobs were green. Nothing was wrong with the shipped code.

Root Cause

#215 added a Build codev package step to the unit job, so dist/, dashboard-dist/ and v2-dist/ exist when the unit suite runs. They did not before. The root package.json is files: ["*", "!apps/streamdeck", "!apps/vscode"], and that test packs the workspace root — so the pack now sweeps the built output too.

Measured with npm pack --dry-run --json rather than estimated: the build step adds 805 files and 12.4 MB to a root tarball that is now 4,395 files and 27.4 MB packed. A hook already sitting near its ceiling went 544ms over it.

The build step stays. Without it the publish-scrub guard narrows to committed files and passes vacuously, which its own comment says. The test is what changes.

Fix — all three defects in #216

1. The pack was far more work than the assertion needed. It wrote a real 27.4 MB tarball and shelled out to tar -tzf purely to read a list of names. npm pack --dry-run --json returns that list with no tarball written and no tar process — the same idiom #214's publish-scrub guard already uses. Paths come back without the package/ prefix, so the four assertions match on bare repo-relative paths.

Raising a timeout on work that does not need doing would have been the weakest fix available: the tarball grew 12.4 MB this week alone, and the next 12 MB lands right back here.

2. The expensive fixture was in beforeAll, but only 1 of 5 tests used it. The other four read files or run pnpm list and need nothing from the pack. When the hook died, all five were reported skipped — one slow fixture took down four tests that never depended on it. The pack is now a lazy call inside the single test that needs it.

3. There was no explicit timeout. It ran on vitest's 10s default. Now 60s, with the reason beside it: ~4.7s warm locally, roughly 12x headroom for a cold runner and ordinary repo growth, still short enough that a genuine hang fails rather than hanging the job.

The helper also throws on an empty file list, since an empty list satisfies both not.toContain assertions without proving anything.

The 12 unexplained skips, since a number I gave was wrong

Reporting the CI failure, I wrote that "17 tests in that file went to skipped." That file has 5 tests, and CI's own line says (5 tests | 5 skipped). The totals reconciled — local 6,796 + 51 and CI 6,779 + 68 both make 6,847 — but only 5 of the 17 were extension-retirement's. Twelve tests changed state between machine and runner and I attributed them to a file that could not account for them. That is the same defect as reporting a value you are not positioned to observe.

They are found, and they reconcile exactly. CI's per-file skip lines:

FileSkipped in CI
session-manager.test.ts12
tunnel-integration.test.ts21
tunnel-e2e.test.ts13
non-main-default-branch.test.ts8
extension-retirement.test.ts5
default-branch.test.ts4
team-cli.test.ts2
spec-146-t3-contract.test.ts1
spec-146-phase-9-porch-engine.test.ts1
spec-146-phase-9-live-harness.test.ts1
Total68

All 12 are session-manager, and they are CI-gated rather than environmental drift: one describe.skipIf(!!process.env.CI) block holding 5 tests, plus 7 individual it.skipIf(!!process.env.CI) at lines 1218, 1269, 1331, 2077, 2115, 2149, 2187 — counted from the source by brace-matching the block. They spawn real shellper processes and need the node-pty native binding, which a CI child process cannot resolve. They run locally, where CI is unset, and skip in CI. That is exactly why they moved state between the two.

The other 51 are unconditional describe.skip or env-gated and skip in both places — which is the local count exactly.

So 51 + 12 + 5 = 68 in CI, and 51 locally with those 12 and 5 passing gives 6,796. Every number closes, and there is no second failure hiding in the remainder.

One off-subject commit rides along, deliberately

a725af68f — chore(porch): bugfix-214 protocol complete is on this branch and is not about #216. It touches one file, codev/projects/bugfix-214-pre-publish-scrub-a-home-direc/status.yaml, 2 lines.

Porch writes its state commits after a PR merges, onto whatever branch is checked out — and by the time porch done bugfix-214 ran, this branch was already cut from 4ebaf36b6 to fix the red. The bookkeeping is true: #214's protocol is complete, its leak is fixed and on main, and #216 is a separate defect with its own issue rather than unfinished #214 work. It is also porch's own commit, which is the only way status.yaml is allowed to change.

Carrying it here rather than spending a whole PR cycle on 2 lines of bookkeeping. Recorded so nobody loses time next month asking why a #214 commit sits on a #216 branch.

Test Plan

  • Rewritten suite: 5 passed, 4.91s (was a 10s-default beforeAll that took 10.5s in CI).
  • Assertions verified to have teeth, not just to pass: the resolved list is 4,395 entries, carries apps/web/package.json and apps/v2/package.json, has 0 entries under apps/vscode/ or apps/streamdeck/, and no path carries the package/ prefix.
  • Full packages/codev unit suite on a stable HEAD (bc7772121, clean tree): 342 files passed, 3 skipped; 6,796 tests passed, 51 skipped, 0 failed.

An earlier full run showed 1 failure and is not quoted here, because its preconditions were violated rather than because it was inconvenient: spec-1280-measurement-instrument's T12 — determinism compares two instrument invocations byte-for-byte, and porch done bugfix-214 wrote a state commit during that run, so the two invocations stamped different commits (a725af68f vs 4ebaf36b6). Self-inflicted concurrency, not the code and not the test. Both affected files were then re-run with HEAD stable — 2 files, 29 tests, all passing — before the clean full run above.

That hazard is filed separately as #217 and deliberately not touched here: a determinism test whose title states a precondition it never checks reports "I could not tell" with the same signal as "the instrument is non-deterministic".

pseudoseedand others added 2 commits August 29, 2026 14:57
`main` went red at 4ebaf36 on this one suite: the `beforeAll` hook timed
out at 10000ms after taking 10544ms.
#215's `Build codev package` step means `dist/`, `dashboard-dist/` and
`v2-dist/` now exist when the unit suite runs. The root package.json is
`files: ["*", …]` and this test packs the WORKSPACE ROOT, so the pack sweeps
the built output too — measured at 805 files and 12.4 MB added to a root
tarball that is now 4,395 files and 27.4 MB packed. A hook already near its
ceiling went 544ms over.
The build step stays. Without it the publish-scrub guard narrows to committed
files and passes vacuously, which its own comment says. The test changes.
All three defects, not just the one that was red:
The pack was far more work than the assertion needed — it wrote a real
tarball and shelled out to `tar -tzf` purely to read a list of names.
`npm pack --dry-run --json` returns that list with no tarball and no `tar`
process, the same idiom #214's publish-scrub guard uses. Paths come back
without the `package/` prefix, so the assertions match bare repo-relative
paths. Raising a timeout on work that does not need doing would have been the
weakest fix available: the tarball grew 12.4 MB this week alone.
The fixture was in `beforeAll` but only 1 of 5 tests used it, so when the hook
died all five were reported skipped — one slow fixture took down four tests
that never depended on it. The pack is now lazy, inside the test that needs it.
There was no explicit timeout. Now 60s, with the reason beside it.
The helper throws on an empty file list: an empty list satisfies both
`not.toContain` assertions without proving anything.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@pseudoseed

Copy link
Copy Markdown
OwnerAuthor

Architect review — approved on my read

Risk: low. 2 files, 51 additions / 29 deletions, one test file plus 2 lines of porch
bookkeeping. No shipped code. Read by me directly rather than sent to consultation; the triage
table puts this tier at architect-read, and spending three reviewer lanes on a test-timeout fix
while main is red would cost more than it returns.

I pre-reviewed this diff in the worktree before it was pushed, so this is not a rubber stamp on
the description.

Verified independently

  • --ignore-scripts does not change what the pack contains here. That flag would alter the
    file list if the root package had a prepack. It has no prepack, prepare,
    prepublishOnly or postpack. Checked the root package.json directly. The builder checked
    the same thing from its own direction and we agree — two derivations, one answer.
  • The path-shape change is correct.npm pack --dry-run --json returns files[].path
    relative to the package root, with no package/ prefix, so dropping it from all four
    assertions is right and not an oversight.
  • The skip reconciliation. I brace-matched the describe.skipIf(!!process.env.CI) at
    session-manager.test.ts:671: it spans lines 671-855 and holds exactly 5 tests, plus the 7
    it.skipIf at the lines named in the body. 12 exactly. 51 + 12 + 5 = 68 closes on both
    sides.
  • The root cause. Confirmed from the merge diff of Fix #214: no home-directory path reaches a published tarball #215 and the root files: ["*", …], not
    from the summary.

The part that matters most

The helper throws on an empty file list, because an empty list satisfies both not.toContain
assertions without proving anything. That is the same property as #214's publish-scrub guard: a
check that cannot pass vacuously. It was not in the three items I asked for, and it is the
difference between a test that verifies something and a test that reads green.

The 60s timeout also carries what it was measured against and why 12x, so whoever has to raise
it next knows what they are trading. A bare number would have been a guess with a suit on.

On the fix that was not chosen

Raising the hook timeout alone would have gone green today. The tarball grew 12.4 MB this week;
the next 12 MB lands back here. Removing work that never needed doing is the fix, and the tool
that measured the regression — npm pack --dry-run --json — was already the answer.

Correction folded in

The builder's first CI report attributed 17 skipped tests to a file containing 5. It found the
remaining 12, named the mechanism (node-pty native binding unresolvable in a CI child), and put
the full accounting in the PR body rather than leaving a number that did not add up. Recorded
here because the correction is part of the record, not a footnote to it.

Out of scope, correctly

The spec-1280 determinism failure hit during this work is not in this PR. It was a moved
HEAD — a porch done committed state mid-suite — and it is filed as #217, where the real
defect is that the test reports "not deterministic" for a case where it could not measure
determinism at all.

Approved. Merge on green CI, --merge, never squash.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

main is red: extension-retirement pack hook exceeds its 10s timeout after #215's build step

1 participant

@pseudoseed
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix #216: extension-retirement pack hook no longer packs a 27MB tarball - #218

Merged
pseudoseed merged 2 commits into
mainfrom
builder/bugfix-216-pack-hook-timeout
Aug 29, 2026
Merged

Fix #216: extension-retirement pack hook no longer packs a 27MB tarball#218
pseudoseed merged 2 commits into
mainfrom
builder/bugfix-216-pack-hook-timeout

Conversation

@pseudoseed

Copy link
Copy Markdown
Owner

Fixes#216.

main went red at 4ebaf36b6 on one suite: extension-retirement.test.ts, whose beforeAll hook timed out at 10000ms after taking 10544ms. 341 files and 6,779 tests passed, and the other five CI jobs were green. Nothing was wrong with the shipped code.

Root Cause

#215 added a Build codev package step to the unit job, so dist/, dashboard-dist/ and v2-dist/ exist when the unit suite runs. They did not before. The root package.json is files: ["*", "!apps/streamdeck", "!apps/vscode"], and that test packs the workspace root — so the pack now sweeps the built output too.

Measured with npm pack --dry-run --json rather than estimated: the build step adds 805 files and 12.4 MB to a root tarball that is now 4,395 files and 27.4 MB packed. A hook already sitting near its ceiling went 544ms over it.

The build step stays. Without it the publish-scrub guard narrows to committed files and passes vacuously, which its own comment says. The test is what changes.

Fix — all three defects in #216

1. The pack was far more work than the assertion needed. It wrote a real 27.4 MB tarball and shelled out to tar -tzf purely to read a list of names. npm pack --dry-run --json returns that list with no tarball written and no tar process — the same idiom #214's publish-scrub guard already uses. Paths come back without the package/ prefix, so the four assertions match on bare repo-relative paths.

Raising a timeout on work that does not need doing would have been the weakest fix available: the tarball grew 12.4 MB this week alone, and the next 12 MB lands right back here.

2. The expensive fixture was in beforeAll, but only 1 of 5 tests used it. The other four read files or run pnpm list and need nothing from the pack. When the hook died, all five were reported skipped — one slow fixture took down four tests that never depended on it. The pack is now a lazy call inside the single test that needs it.

3. There was no explicit timeout. It ran on vitest's 10s default. Now 60s, with the reason beside it: ~4.7s warm locally, roughly 12x headroom for a cold runner and ordinary repo growth, still short enough that a genuine hang fails rather than hanging the job.

The helper also throws on an empty file list, since an empty list satisfies both not.toContain assertions without proving anything.

The 12 unexplained skips, since a number I gave was wrong

Reporting the CI failure, I wrote that "17 tests in that file went to skipped." That file has 5 tests, and CI's own line says (5 tests | 5 skipped). The totals reconciled — local 6,796 + 51 and CI 6,779 + 68 both make 6,847 — but only 5 of the 17 were extension-retirement's. Twelve tests changed state between machine and runner and I attributed them to a file that could not account for them. That is the same defect as reporting a value you are not positioned to observe.

They are found, and they reconcile exactly. CI's per-file skip lines:

FileSkipped in CI
session-manager.test.ts12
tunnel-integration.test.ts21
tunnel-e2e.test.ts13
non-main-default-branch.test.ts8
extension-retirement.test.ts5
default-branch.test.ts4
team-cli.test.ts2
spec-146-t3-contract.test.ts1
spec-146-phase-9-porch-engine.test.ts1
spec-146-phase-9-live-harness.test.ts1
Total68

All 12 are session-manager, and they are CI-gated rather than environmental drift: one describe.skipIf(!!process.env.CI) block holding 5 tests, plus 7 individual it.skipIf(!!process.env.CI) at lines 1218, 1269, 1331, 2077, 2115, 2149, 2187 — counted from the source by brace-matching the block. They spawn real shellper processes and need the node-pty native binding, which a CI child process cannot resolve. They run locally, where CI is unset, and skip in CI. That is exactly why they moved state between the two.

The other 51 are unconditional describe.skip or env-gated and skip in both places — which is the local count exactly.

So 51 + 12 + 5 = 68 in CI, and 51 locally with those 12 and 5 passing gives 6,796. Every number closes, and there is no second failure hiding in the remainder.

One off-subject commit rides along, deliberately

a725af68f — chore(porch): bugfix-214 protocol complete is on this branch and is not about #216. It touches one file, codev/projects/bugfix-214-pre-publish-scrub-a-home-direc/status.yaml, 2 lines.

Porch writes its state commits after a PR merges, onto whatever branch is checked out — and by the time porch done bugfix-214 ran, this branch was already cut from 4ebaf36b6 to fix the red. The bookkeeping is true: #214's protocol is complete, its leak is fixed and on main, and #216 is a separate defect with its own issue rather than unfinished #214 work. It is also porch's own commit, which is the only way status.yaml is allowed to change.

Carrying it here rather than spending a whole PR cycle on 2 lines of bookkeeping. Recorded so nobody loses time next month asking why a #214 commit sits on a #216 branch.

Test Plan

  • Rewritten suite: 5 passed, 4.91s (was a 10s-default beforeAll that took 10.5s in CI).
  • Assertions verified to have teeth, not just to pass: the resolved list is 4,395 entries, carries apps/web/package.json and apps/v2/package.json, has 0 entries under apps/vscode/ or apps/streamdeck/, and no path carries the package/ prefix.
  • Full packages/codev unit suite on a stable HEAD (bc7772121, clean tree): 342 files passed, 3 skipped; 6,796 tests passed, 51 skipped, 0 failed.

An earlier full run showed 1 failure and is not quoted here, because its preconditions were violated rather than because it was inconvenient: spec-1280-measurement-instrument's T12 — determinism compares two instrument invocations byte-for-byte, and porch done bugfix-214 wrote a state commit during that run, so the two invocations stamped different commits (a725af68f vs 4ebaf36b6). Self-inflicted concurrency, not the code and not the test. Both affected files were then re-run with HEAD stable — 2 files, 29 tests, all passing — before the clean full run above.

That hazard is filed separately as #217 and deliberately not touched here: a determinism test whose title states a precondition it never checks reports "I could not tell" with the same signal as "the instrument is non-deterministic".

pseudoseedand others added 2 commits August 29, 2026 14:57
`main` went red at 4ebaf36 on this one suite: the `beforeAll` hook timed
out at 10000ms after taking 10544ms.
#215's `Build codev package` step means `dist/`, `dashboard-dist/` and
`v2-dist/` now exist when the unit suite runs. The root package.json is
`files: ["*", …]` and this test packs the WORKSPACE ROOT, so the pack sweeps
the built output too — measured at 805 files and 12.4 MB added to a root
tarball that is now 4,395 files and 27.4 MB packed. A hook already near its
ceiling went 544ms over.
The build step stays. Without it the publish-scrub guard narrows to committed
files and passes vacuously, which its own comment says. The test changes.
All three defects, not just the one that was red:
The pack was far more work than the assertion needed — it wrote a real
tarball and shelled out to `tar -tzf` purely to read a list of names.
`npm pack --dry-run --json` returns that list with no tarball and no `tar`
process, the same idiom #214's publish-scrub guard uses. Paths come back
without the `package/` prefix, so the assertions match bare repo-relative
paths. Raising a timeout on work that does not need doing would have been the
weakest fix available: the tarball grew 12.4 MB this week alone.
The fixture was in `beforeAll` but only 1 of 5 tests used it, so when the hook
died all five were reported skipped — one slow fixture took down four tests
that never depended on it. The pack is now lazy, inside the test that needs it.
There was no explicit timeout. Now 60s, with the reason beside it.
The helper throws on an empty file list: an empty list satisfies both
`not.toContain` assertions without proving anything.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@pseudoseed

Copy link
Copy Markdown
OwnerAuthor

Architect review — approved on my read

Risk: low. 2 files, 51 additions / 29 deletions, one test file plus 2 lines of porch
bookkeeping. No shipped code. Read by me directly rather than sent to consultation; the triage
table puts this tier at architect-read, and spending three reviewer lanes on a test-timeout fix
while main is red would cost more than it returns.

I pre-reviewed this diff in the worktree before it was pushed, so this is not a rubber stamp on
the description.

Verified independently

  • --ignore-scripts does not change what the pack contains here. That flag would alter the
    file list if the root package had a prepack. It has no prepack, prepare,
    prepublishOnly or postpack. Checked the root package.json directly. The builder checked
    the same thing from its own direction and we agree — two derivations, one answer.
  • The path-shape change is correct.npm pack --dry-run --json returns files[].path
    relative to the package root, with no package/ prefix, so dropping it from all four
    assertions is right and not an oversight.
  • The skip reconciliation. I brace-matched the describe.skipIf(!!process.env.CI) at
    session-manager.test.ts:671: it spans lines 671-855 and holds exactly 5 tests, plus the 7
    it.skipIf at the lines named in the body. 12 exactly. 51 + 12 + 5 = 68 closes on both
    sides.
  • The root cause. Confirmed from the merge diff of Fix #214: no home-directory path reaches a published tarball #215 and the root files: ["*", …], not
    from the summary.

The part that matters most

The helper throws on an empty file list, because an empty list satisfies both not.toContain
assertions without proving anything. That is the same property as #214's publish-scrub guard: a
check that cannot pass vacuously. It was not in the three items I asked for, and it is the
difference between a test that verifies something and a test that reads green.

The 60s timeout also carries what it was measured against and why 12x, so whoever has to raise
it next knows what they are trading. A bare number would have been a guess with a suit on.

On the fix that was not chosen

Raising the hook timeout alone would have gone green today. The tarball grew 12.4 MB this week;
the next 12 MB lands back here. Removing work that never needed doing is the fix, and the tool
that measured the regression — npm pack --dry-run --json — was already the answer.

Correction folded in

The builder's first CI report attributed 17 skipped tests to a file containing 5. It found the
remaining 12, named the mechanism (node-pty native binding unresolvable in a CI child), and put
the full accounting in the PR body rather than leaving a number that did not add up. Recorded
here because the correction is part of the record, not a footnote to it.

Out of scope, correctly

The spec-1280 determinism failure hit during this work is not in this PR. It was a moved
HEAD — a porch done committed state mid-suite — and it is filed as #217, where the real
defect is that the test reports "not deterministic" for a case where it could not measure
determinism at all.

Approved. Merge on green CI, --merge, never squash.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

main is red: extension-retirement pack hook exceeds its 10s timeout after #215's build step

1 participant

@pseudoseed
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix #216: extension-retirement pack hook no longer packs a 27MB tarball - #218

Merged
pseudoseed merged 2 commits into
mainfrom
builder/bugfix-216-pack-hook-timeout
Aug 29, 2026
Merged

Fix #216: extension-retirement pack hook no longer packs a 27MB tarball#218
pseudoseed merged 2 commits into
mainfrom
builder/bugfix-216-pack-hook-timeout

Conversation

@pseudoseed

Copy link
Copy Markdown
Owner

Fixes#216.

main went red at 4ebaf36b6 on one suite: extension-retirement.test.ts, whose beforeAll hook timed out at 10000ms after taking 10544ms. 341 files and 6,779 tests passed, and the other five CI jobs were green. Nothing was wrong with the shipped code.

Root Cause

#215 added a Build codev package step to the unit job, so dist/, dashboard-dist/ and v2-dist/ exist when the unit suite runs. They did not before. The root package.json is files: ["*", "!apps/streamdeck", "!apps/vscode"], and that test packs the workspace root — so the pack now sweeps the built output too.

Measured with npm pack --dry-run --json rather than estimated: the build step adds 805 files and 12.4 MB to a root tarball that is now 4,395 files and 27.4 MB packed. A hook already sitting near its ceiling went 544ms over it.

The build step stays. Without it the publish-scrub guard narrows to committed files and passes vacuously, which its own comment says. The test is what changes.

Fix — all three defects in #216

1. The pack was far more work than the assertion needed. It wrote a real 27.4 MB tarball and shelled out to tar -tzf purely to read a list of names. npm pack --dry-run --json returns that list with no tarball written and no tar process — the same idiom #214's publish-scrub guard already uses. Paths come back without the package/ prefix, so the four assertions match on bare repo-relative paths.

Raising a timeout on work that does not need doing would have been the weakest fix available: the tarball grew 12.4 MB this week alone, and the next 12 MB lands right back here.

2. The expensive fixture was in beforeAll, but only 1 of 5 tests used it. The other four read files or run pnpm list and need nothing from the pack. When the hook died, all five were reported skipped — one slow fixture took down four tests that never depended on it. The pack is now a lazy call inside the single test that needs it.

3. There was no explicit timeout. It ran on vitest's 10s default. Now 60s, with the reason beside it: ~4.7s warm locally, roughly 12x headroom for a cold runner and ordinary repo growth, still short enough that a genuine hang fails rather than hanging the job.

The helper also throws on an empty file list, since an empty list satisfies both not.toContain assertions without proving anything.

The 12 unexplained skips, since a number I gave was wrong

Reporting the CI failure, I wrote that "17 tests in that file went to skipped." That file has 5 tests, and CI's own line says (5 tests | 5 skipped). The totals reconciled — local 6,796 + 51 and CI 6,779 + 68 both make 6,847 — but only 5 of the 17 were extension-retirement's. Twelve tests changed state between machine and runner and I attributed them to a file that could not account for them. That is the same defect as reporting a value you are not positioned to observe.

They are found, and they reconcile exactly. CI's per-file skip lines:

FileSkipped in CI
session-manager.test.ts12
tunnel-integration.test.ts21
tunnel-e2e.test.ts13
non-main-default-branch.test.ts8
extension-retirement.test.ts5
default-branch.test.ts4
team-cli.test.ts2
spec-146-t3-contract.test.ts1
spec-146-phase-9-porch-engine.test.ts1
spec-146-phase-9-live-harness.test.ts1
Total68

All 12 are session-manager, and they are CI-gated rather than environmental drift: one describe.skipIf(!!process.env.CI) block holding 5 tests, plus 7 individual it.skipIf(!!process.env.CI) at lines 1218, 1269, 1331, 2077, 2115, 2149, 2187 — counted from the source by brace-matching the block. They spawn real shellper processes and need the node-pty native binding, which a CI child process cannot resolve. They run locally, where CI is unset, and skip in CI. That is exactly why they moved state between the two.

The other 51 are unconditional describe.skip or env-gated and skip in both places — which is the local count exactly.

So 51 + 12 + 5 = 68 in CI, and 51 locally with those 12 and 5 passing gives 6,796. Every number closes, and there is no second failure hiding in the remainder.

One off-subject commit rides along, deliberately

a725af68f — chore(porch): bugfix-214 protocol complete is on this branch and is not about #216. It touches one file, codev/projects/bugfix-214-pre-publish-scrub-a-home-direc/status.yaml, 2 lines.

Porch writes its state commits after a PR merges, onto whatever branch is checked out — and by the time porch done bugfix-214 ran, this branch was already cut from 4ebaf36b6 to fix the red. The bookkeeping is true: #214's protocol is complete, its leak is fixed and on main, and #216 is a separate defect with its own issue rather than unfinished #214 work. It is also porch's own commit, which is the only way status.yaml is allowed to change.

Carrying it here rather than spending a whole PR cycle on 2 lines of bookkeeping. Recorded so nobody loses time next month asking why a #214 commit sits on a #216 branch.

Test Plan

  • Rewritten suite: 5 passed, 4.91s (was a 10s-default beforeAll that took 10.5s in CI).
  • Assertions verified to have teeth, not just to pass: the resolved list is 4,395 entries, carries apps/web/package.json and apps/v2/package.json, has 0 entries under apps/vscode/ or apps/streamdeck/, and no path carries the package/ prefix.
  • Full packages/codev unit suite on a stable HEAD (bc7772121, clean tree): 342 files passed, 3 skipped; 6,796 tests passed, 51 skipped, 0 failed.

An earlier full run showed 1 failure and is not quoted here, because its preconditions were violated rather than because it was inconvenient: spec-1280-measurement-instrument's T12 — determinism compares two instrument invocations byte-for-byte, and porch done bugfix-214 wrote a state commit during that run, so the two invocations stamped different commits (a725af68f vs 4ebaf36b6). Self-inflicted concurrency, not the code and not the test. Both affected files were then re-run with HEAD stable — 2 files, 29 tests, all passing — before the clean full run above.

That hazard is filed separately as #217 and deliberately not touched here: a determinism test whose title states a precondition it never checks reports "I could not tell" with the same signal as "the instrument is non-deterministic".

pseudoseedand others added 2 commits August 29, 2026 14:57
`main` went red at 4ebaf36 on this one suite: the `beforeAll` hook timed
out at 10000ms after taking 10544ms.
#215's `Build codev package` step means `dist/`, `dashboard-dist/` and
`v2-dist/` now exist when the unit suite runs. The root package.json is
`files: ["*", …]` and this test packs the WORKSPACE ROOT, so the pack sweeps
the built output too — measured at 805 files and 12.4 MB added to a root
tarball that is now 4,395 files and 27.4 MB packed. A hook already near its
ceiling went 544ms over.
The build step stays. Without it the publish-scrub guard narrows to committed
files and passes vacuously, which its own comment says. The test changes.
All three defects, not just the one that was red:
The pack was far more work than the assertion needed — it wrote a real
tarball and shelled out to `tar -tzf` purely to read a list of names.
`npm pack --dry-run --json` returns that list with no tarball and no `tar`
process, the same idiom #214's publish-scrub guard uses. Paths come back
without the `package/` prefix, so the assertions match bare repo-relative
paths. Raising a timeout on work that does not need doing would have been the
weakest fix available: the tarball grew 12.4 MB this week alone.
The fixture was in `beforeAll` but only 1 of 5 tests used it, so when the hook
died all five were reported skipped — one slow fixture took down four tests
that never depended on it. The pack is now lazy, inside the test that needs it.
There was no explicit timeout. Now 60s, with the reason beside it.
The helper throws on an empty file list: an empty list satisfies both
`not.toContain` assertions without proving anything.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@pseudoseed

Copy link
Copy Markdown
OwnerAuthor

Architect review — approved on my read

Risk: low. 2 files, 51 additions / 29 deletions, one test file plus 2 lines of porch
bookkeeping. No shipped code. Read by me directly rather than sent to consultation; the triage
table puts this tier at architect-read, and spending three reviewer lanes on a test-timeout fix
while main is red would cost more than it returns.

I pre-reviewed this diff in the worktree before it was pushed, so this is not a rubber stamp on
the description.

Verified independently

  • --ignore-scripts does not change what the pack contains here. That flag would alter the
    file list if the root package had a prepack. It has no prepack, prepare,
    prepublishOnly or postpack. Checked the root package.json directly. The builder checked
    the same thing from its own direction and we agree — two derivations, one answer.
  • The path-shape change is correct.npm pack --dry-run --json returns files[].path
    relative to the package root, with no package/ prefix, so dropping it from all four
    assertions is right and not an oversight.
  • The skip reconciliation. I brace-matched the describe.skipIf(!!process.env.CI) at
    session-manager.test.ts:671: it spans lines 671-855 and holds exactly 5 tests, plus the 7
    it.skipIf at the lines named in the body. 12 exactly. 51 + 12 + 5 = 68 closes on both
    sides.
  • The root cause. Confirmed from the merge diff of Fix #214: no home-directory path reaches a published tarball #215 and the root files: ["*", …], not
    from the summary.

The part that matters most

The helper throws on an empty file list, because an empty list satisfies both not.toContain
assertions without proving anything. That is the same property as #214's publish-scrub guard: a
check that cannot pass vacuously. It was not in the three items I asked for, and it is the
difference between a test that verifies something and a test that reads green.

The 60s timeout also carries what it was measured against and why 12x, so whoever has to raise
it next knows what they are trading. A bare number would have been a guess with a suit on.

On the fix that was not chosen

Raising the hook timeout alone would have gone green today. The tarball grew 12.4 MB this week;
the next 12 MB lands back here. Removing work that never needed doing is the fix, and the tool
that measured the regression — npm pack --dry-run --json — was already the answer.

Correction folded in

The builder's first CI report attributed 17 skipped tests to a file containing 5. It found the
remaining 12, named the mechanism (node-pty native binding unresolvable in a CI child), and put
the full accounting in the PR body rather than leaving a number that did not add up. Recorded
here because the correction is part of the record, not a footnote to it.

Out of scope, correctly

The spec-1280 determinism failure hit during this work is not in this PR. It was a moved
HEAD — a porch done committed state mid-suite — and it is filed as #217, where the real
defect is that the test reports "not deterministic" for a case where it could not measure
determinism at all.

Approved. Merge on green CI, --merge, never squash.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

main is red: extension-retirement pack hook exceeds its 10s timeout after #215's build step

1 participant

@pseudoseed
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Fix #216: extension-retirement pack hook no longer packs a 27MB tarball - #218

Merged
pseudoseed merged 2 commits into
mainfrom
builder/bugfix-216-pack-hook-timeout
Aug 29, 2026
Merged

Fix #216: extension-retirement pack hook no longer packs a 27MB tarball#218
pseudoseed merged 2 commits into
mainfrom
builder/bugfix-216-pack-hook-timeout

Conversation

@pseudoseed

Copy link
Copy Markdown
Owner

Fixes#216.

main went red at 4ebaf36b6 on one suite: extension-retirement.test.ts, whose beforeAll hook timed out at 10000ms after taking 10544ms. 341 files and 6,779 tests passed, and the other five CI jobs were green. Nothing was wrong with the shipped code.

Root Cause

#215 added a Build codev package step to the unit job, so dist/, dashboard-dist/ and v2-dist/ exist when the unit suite runs. They did not before. The root package.json is files: ["*", "!apps/streamdeck", "!apps/vscode"], and that test packs the workspace root — so the pack now sweeps the built output too.

Measured with npm pack --dry-run --json rather than estimated: the build step adds 805 files and 12.4 MB to a root tarball that is now 4,395 files and 27.4 MB packed. A hook already sitting near its ceiling went 544ms over it.

The build step stays. Without it the publish-scrub guard narrows to committed files and passes vacuously, which its own comment says. The test is what changes.

Fix — all three defects in #216

1. The pack was far more work than the assertion needed. It wrote a real 27.4 MB tarball and shelled out to tar -tzf purely to read a list of names. npm pack --dry-run --json returns that list with no tarball written and no tar process — the same idiom #214's publish-scrub guard already uses. Paths come back without the package/ prefix, so the four assertions match on bare repo-relative paths.

Raising a timeout on work that does not need doing would have been the weakest fix available: the tarball grew 12.4 MB this week alone, and the next 12 MB lands right back here.

2. The expensive fixture was in beforeAll, but only 1 of 5 tests used it. The other four read files or run pnpm list and need nothing from the pack. When the hook died, all five were reported skipped — one slow fixture took down four tests that never depended on it. The pack is now a lazy call inside the single test that needs it.

3. There was no explicit timeout. It ran on vitest's 10s default. Now 60s, with the reason beside it: ~4.7s warm locally, roughly 12x headroom for a cold runner and ordinary repo growth, still short enough that a genuine hang fails rather than hanging the job.

The helper also throws on an empty file list, since an empty list satisfies both not.toContain assertions without proving anything.

The 12 unexplained skips, since a number I gave was wrong

Reporting the CI failure, I wrote that "17 tests in that file went to skipped." That file has 5 tests, and CI's own line says (5 tests | 5 skipped). The totals reconciled — local 6,796 + 51 and CI 6,779 + 68 both make 6,847 — but only 5 of the 17 were extension-retirement's. Twelve tests changed state between machine and runner and I attributed them to a file that could not account for them. That is the same defect as reporting a value you are not positioned to observe.

They are found, and they reconcile exactly. CI's per-file skip lines:

FileSkipped in CI
session-manager.test.ts12
tunnel-integration.test.ts21
tunnel-e2e.test.ts13
non-main-default-branch.test.ts8
extension-retirement.test.ts5
default-branch.test.ts4
team-cli.test.ts2
spec-146-t3-contract.test.ts1
spec-146-phase-9-porch-engine.test.ts1
spec-146-phase-9-live-harness.test.ts1
Total68

All 12 are session-manager, and they are CI-gated rather than environmental drift: one describe.skipIf(!!process.env.CI) block holding 5 tests, plus 7 individual it.skipIf(!!process.env.CI) at lines 1218, 1269, 1331, 2077, 2115, 2149, 2187 — counted from the source by brace-matching the block. They spawn real shellper processes and need the node-pty native binding, which a CI child process cannot resolve. They run locally, where CI is unset, and skip in CI. That is exactly why they moved state between the two.

The other 51 are unconditional describe.skip or env-gated and skip in both places — which is the local count exactly.

So 51 + 12 + 5 = 68 in CI, and 51 locally with those 12 and 5 passing gives 6,796. Every number closes, and there is no second failure hiding in the remainder.

One off-subject commit rides along, deliberately

a725af68f — chore(porch): bugfix-214 protocol complete is on this branch and is not about #216. It touches one file, codev/projects/bugfix-214-pre-publish-scrub-a-home-direc/status.yaml, 2 lines.

Porch writes its state commits after a PR merges, onto whatever branch is checked out — and by the time porch done bugfix-214 ran, this branch was already cut from 4ebaf36b6 to fix the red. The bookkeeping is true: #214's protocol is complete, its leak is fixed and on main, and #216 is a separate defect with its own issue rather than unfinished #214 work. It is also porch's own commit, which is the only way status.yaml is allowed to change.

Carrying it here rather than spending a whole PR cycle on 2 lines of bookkeeping. Recorded so nobody loses time next month asking why a #214 commit sits on a #216 branch.

Test Plan

  • Rewritten suite: 5 passed, 4.91s (was a 10s-default beforeAll that took 10.5s in CI).
  • Assertions verified to have teeth, not just to pass: the resolved list is 4,395 entries, carries apps/web/package.json and apps/v2/package.json, has 0 entries under apps/vscode/ or apps/streamdeck/, and no path carries the package/ prefix.
  • Full packages/codev unit suite on a stable HEAD (bc7772121, clean tree): 342 files passed, 3 skipped; 6,796 tests passed, 51 skipped, 0 failed.

An earlier full run showed 1 failure and is not quoted here, because its preconditions were violated rather than because it was inconvenient: spec-1280-measurement-instrument's T12 — determinism compares two instrument invocations byte-for-byte, and porch done bugfix-214 wrote a state commit during that run, so the two invocations stamped different commits (a725af68f vs 4ebaf36b6). Self-inflicted concurrency, not the code and not the test. Both affected files were then re-run with HEAD stable — 2 files, 29 tests, all passing — before the clean full run above.

That hazard is filed separately as #217 and deliberately not touched here: a determinism test whose title states a precondition it never checks reports "I could not tell" with the same signal as "the instrument is non-deterministic".

pseudoseedand others added 2 commits August 29, 2026 14:57
`main` went red at 4ebaf36 on this one suite: the `beforeAll` hook timed
out at 10000ms after taking 10544ms.
#215's `Build codev package` step means `dist/`, `dashboard-dist/` and
`v2-dist/` now exist when the unit suite runs. The root package.json is
`files: ["*", …]` and this test packs the WORKSPACE ROOT, so the pack sweeps
the built output too — measured at 805 files and 12.4 MB added to a root
tarball that is now 4,395 files and 27.4 MB packed. A hook already near its
ceiling went 544ms over.
The build step stays. Without it the publish-scrub guard narrows to committed
files and passes vacuously, which its own comment says. The test changes.
All three defects, not just the one that was red:
The pack was far more work than the assertion needed — it wrote a real
tarball and shelled out to `tar -tzf` purely to read a list of names.
`npm pack --dry-run --json` returns that list with no tarball and no `tar`
process, the same idiom #214's publish-scrub guard uses. Paths come back
without the `package/` prefix, so the assertions match bare repo-relative
paths. Raising a timeout on work that does not need doing would have been the
weakest fix available: the tarball grew 12.4 MB this week alone.
The fixture was in `beforeAll` but only 1 of 5 tests used it, so when the hook
died all five were reported skipped — one slow fixture took down four tests
that never depended on it. The pack is now lazy, inside the test that needs it.
There was no explicit timeout. Now 60s, with the reason beside it.
The helper throws on an empty file list: an empty list satisfies both
`not.toContain` assertions without proving anything.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@pseudoseed

Copy link
Copy Markdown
OwnerAuthor

Architect review — approved on my read

Risk: low. 2 files, 51 additions / 29 deletions, one test file plus 2 lines of porch
bookkeeping. No shipped code. Read by me directly rather than sent to consultation; the triage
table puts this tier at architect-read, and spending three reviewer lanes on a test-timeout fix
while main is red would cost more than it returns.

I pre-reviewed this diff in the worktree before it was pushed, so this is not a rubber stamp on
the description.

Verified independently

  • --ignore-scripts does not change what the pack contains here. That flag would alter the
    file list if the root package had a prepack. It has no prepack, prepare,
    prepublishOnly or postpack. Checked the root package.json directly. The builder checked
    the same thing from its own direction and we agree — two derivations, one answer.
  • The path-shape change is correct.npm pack --dry-run --json returns files[].path
    relative to the package root, with no package/ prefix, so dropping it from all four
    assertions is right and not an oversight.
  • The skip reconciliation. I brace-matched the describe.skipIf(!!process.env.CI) at
    session-manager.test.ts:671: it spans lines 671-855 and holds exactly 5 tests, plus the 7
    it.skipIf at the lines named in the body. 12 exactly. 51 + 12 + 5 = 68 closes on both
    sides.
  • The root cause. Confirmed from the merge diff of Fix #214: no home-directory path reaches a published tarball #215 and the root files: ["*", …], not
    from the summary.

The part that matters most

The helper throws on an empty file list, because an empty list satisfies both not.toContain
assertions without proving anything. That is the same property as #214's publish-scrub guard: a
check that cannot pass vacuously. It was not in the three items I asked for, and it is the
difference between a test that verifies something and a test that reads green.

The 60s timeout also carries what it was measured against and why 12x, so whoever has to raise
it next knows what they are trading. A bare number would have been a guess with a suit on.

On the fix that was not chosen

Raising the hook timeout alone would have gone green today. The tarball grew 12.4 MB this week;
the next 12 MB lands back here. Removing work that never needed doing is the fix, and the tool
that measured the regression — npm pack --dry-run --json — was already the answer.

Correction folded in

The builder's first CI report attributed 17 skipped tests to a file containing 5. It found the
remaining 12, named the mechanism (node-pty native binding unresolvable in a CI child), and put
the full accounting in the PR body rather than leaving a number that did not add up. Recorded
here because the correction is part of the record, not a footnote to it.

Out of scope, correctly

The spec-1280 determinism failure hit during this work is not in this PR. It was a moved
HEAD — a porch done committed state mid-suite — and it is filed as #217, where the real
defect is that the test reports "not deterministic" for a case where it could not measure
determinism at all.

Approved. Merge on green CI, --merge, never squash.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

main is red: extension-retirement pack hook exceeds its 10s timeout after #215's build step

1 participant

@pseudoseed
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix #216: extension-retirement pack hook no longer packs a 27MB tarball - #218

Merged
pseudoseed merged 2 commits into
mainfrom
builder/bugfix-216-pack-hook-timeout
Aug 29, 2026
Merged

Fix #216: extension-retirement pack hook no longer packs a 27MB tarball#218
pseudoseed merged 2 commits into
mainfrom
builder/bugfix-216-pack-hook-timeout

Conversation

@pseudoseed

Copy link
Copy Markdown
Owner

Fixes#216.

main went red at 4ebaf36b6 on one suite: extension-retirement.test.ts, whose beforeAll hook timed out at 10000ms after taking 10544ms. 341 files and 6,779 tests passed, and the other five CI jobs were green. Nothing was wrong with the shipped code.

Root Cause

#215 added a Build codev package step to the unit job, so dist/, dashboard-dist/ and v2-dist/ exist when the unit suite runs. They did not before. The root package.json is files: ["*", "!apps/streamdeck", "!apps/vscode"], and that test packs the workspace root — so the pack now sweeps the built output too.

Measured with npm pack --dry-run --json rather than estimated: the build step adds 805 files and 12.4 MB to a root tarball that is now 4,395 files and 27.4 MB packed. A hook already sitting near its ceiling went 544ms over it.

The build step stays. Without it the publish-scrub guard narrows to committed files and passes vacuously, which its own comment says. The test is what changes.

Fix — all three defects in #216

1. The pack was far more work than the assertion needed. It wrote a real 27.4 MB tarball and shelled out to tar -tzf purely to read a list of names. npm pack --dry-run --json returns that list with no tarball written and no tar process — the same idiom #214's publish-scrub guard already uses. Paths come back without the package/ prefix, so the four assertions match on bare repo-relative paths.

Raising a timeout on work that does not need doing would have been the weakest fix available: the tarball grew 12.4 MB this week alone, and the next 12 MB lands right back here.

2. The expensive fixture was in beforeAll, but only 1 of 5 tests used it. The other four read files or run pnpm list and need nothing from the pack. When the hook died, all five were reported skipped — one slow fixture took down four tests that never depended on it. The pack is now a lazy call inside the single test that needs it.

3. There was no explicit timeout. It ran on vitest's 10s default. Now 60s, with the reason beside it: ~4.7s warm locally, roughly 12x headroom for a cold runner and ordinary repo growth, still short enough that a genuine hang fails rather than hanging the job.

The helper also throws on an empty file list, since an empty list satisfies both not.toContain assertions without proving anything.

The 12 unexplained skips, since a number I gave was wrong

Reporting the CI failure, I wrote that "17 tests in that file went to skipped." That file has 5 tests, and CI's own line says (5 tests | 5 skipped). The totals reconciled — local 6,796 + 51 and CI 6,779 + 68 both make 6,847 — but only 5 of the 17 were extension-retirement's. Twelve tests changed state between machine and runner and I attributed them to a file that could not account for them. That is the same defect as reporting a value you are not positioned to observe.

They are found, and they reconcile exactly. CI's per-file skip lines:

FileSkipped in CI
session-manager.test.ts12
tunnel-integration.test.ts21
tunnel-e2e.test.ts13
non-main-default-branch.test.ts8
extension-retirement.test.ts5
default-branch.test.ts4
team-cli.test.ts2
spec-146-t3-contract.test.ts1
spec-146-phase-9-porch-engine.test.ts1
spec-146-phase-9-live-harness.test.ts1
Total68

All 12 are session-manager, and they are CI-gated rather than environmental drift: one describe.skipIf(!!process.env.CI) block holding 5 tests, plus 7 individual it.skipIf(!!process.env.CI) at lines 1218, 1269, 1331, 2077, 2115, 2149, 2187 — counted from the source by brace-matching the block. They spawn real shellper processes and need the node-pty native binding, which a CI child process cannot resolve. They run locally, where CI is unset, and skip in CI. That is exactly why they moved state between the two.

The other 51 are unconditional describe.skip or env-gated and skip in both places — which is the local count exactly.

So 51 + 12 + 5 = 68 in CI, and 51 locally with those 12 and 5 passing gives 6,796. Every number closes, and there is no second failure hiding in the remainder.

One off-subject commit rides along, deliberately

a725af68f — chore(porch): bugfix-214 protocol complete is on this branch and is not about #216. It touches one file, codev/projects/bugfix-214-pre-publish-scrub-a-home-direc/status.yaml, 2 lines.

Porch writes its state commits after a PR merges, onto whatever branch is checked out — and by the time porch done bugfix-214 ran, this branch was already cut from 4ebaf36b6 to fix the red. The bookkeeping is true: #214's protocol is complete, its leak is fixed and on main, and #216 is a separate defect with its own issue rather than unfinished #214 work. It is also porch's own commit, which is the only way status.yaml is allowed to change.

Carrying it here rather than spending a whole PR cycle on 2 lines of bookkeeping. Recorded so nobody loses time next month asking why a #214 commit sits on a #216 branch.

Test Plan

  • Rewritten suite: 5 passed, 4.91s (was a 10s-default beforeAll that took 10.5s in CI).
  • Assertions verified to have teeth, not just to pass: the resolved list is 4,395 entries, carries apps/web/package.json and apps/v2/package.json, has 0 entries under apps/vscode/ or apps/streamdeck/, and no path carries the package/ prefix.
  • Full packages/codev unit suite on a stable HEAD (bc7772121, clean tree): 342 files passed, 3 skipped; 6,796 tests passed, 51 skipped, 0 failed.

An earlier full run showed 1 failure and is not quoted here, because its preconditions were violated rather than because it was inconvenient: spec-1280-measurement-instrument's T12 — determinism compares two instrument invocations byte-for-byte, and porch done bugfix-214 wrote a state commit during that run, so the two invocations stamped different commits (a725af68f vs 4ebaf36b6). Self-inflicted concurrency, not the code and not the test. Both affected files were then re-run with HEAD stable — 2 files, 29 tests, all passing — before the clean full run above.

That hazard is filed separately as #217 and deliberately not touched here: a determinism test whose title states a precondition it never checks reports "I could not tell" with the same signal as "the instrument is non-deterministic".

pseudoseedand others added 2 commits August 29, 2026 14:57
`main` went red at 4ebaf36 on this one suite: the `beforeAll` hook timed
out at 10000ms after taking 10544ms.
#215's `Build codev package` step means `dist/`, `dashboard-dist/` and
`v2-dist/` now exist when the unit suite runs. The root package.json is
`files: ["*", …]` and this test packs the WORKSPACE ROOT, so the pack sweeps
the built output too — measured at 805 files and 12.4 MB added to a root
tarball that is now 4,395 files and 27.4 MB packed. A hook already near its
ceiling went 544ms over.
The build step stays. Without it the publish-scrub guard narrows to committed
files and passes vacuously, which its own comment says. The test changes.
All three defects, not just the one that was red:
The pack was far more work than the assertion needed — it wrote a real
tarball and shelled out to `tar -tzf` purely to read a list of names.
`npm pack --dry-run --json` returns that list with no tarball and no `tar`
process, the same idiom #214's publish-scrub guard uses. Paths come back
without the `package/` prefix, so the assertions match bare repo-relative
paths. Raising a timeout on work that does not need doing would have been the
weakest fix available: the tarball grew 12.4 MB this week alone.
The fixture was in `beforeAll` but only 1 of 5 tests used it, so when the hook
died all five were reported skipped — one slow fixture took down four tests
that never depended on it. The pack is now lazy, inside the test that needs it.
There was no explicit timeout. Now 60s, with the reason beside it.
The helper throws on an empty file list: an empty list satisfies both
`not.toContain` assertions without proving anything.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@pseudoseed

Copy link
Copy Markdown
OwnerAuthor

Architect review — approved on my read

Risk: low. 2 files, 51 additions / 29 deletions, one test file plus 2 lines of porch
bookkeeping. No shipped code. Read by me directly rather than sent to consultation; the triage
table puts this tier at architect-read, and spending three reviewer lanes on a test-timeout fix
while main is red would cost more than it returns.

I pre-reviewed this diff in the worktree before it was pushed, so this is not a rubber stamp on
the description.

Verified independently

  • --ignore-scripts does not change what the pack contains here. That flag would alter the
    file list if the root package had a prepack. It has no prepack, prepare,
    prepublishOnly or postpack. Checked the root package.json directly. The builder checked
    the same thing from its own direction and we agree — two derivations, one answer.
  • The path-shape change is correct.npm pack --dry-run --json returns files[].path
    relative to the package root, with no package/ prefix, so dropping it from all four
    assertions is right and not an oversight.
  • The skip reconciliation. I brace-matched the describe.skipIf(!!process.env.CI) at
    session-manager.test.ts:671: it spans lines 671-855 and holds exactly 5 tests, plus the 7
    it.skipIf at the lines named in the body. 12 exactly. 51 + 12 + 5 = 68 closes on both
    sides.
  • The root cause. Confirmed from the merge diff of Fix #214: no home-directory path reaches a published tarball #215 and the root files: ["*", …], not
    from the summary.

The part that matters most

The helper throws on an empty file list, because an empty list satisfies both not.toContain
assertions without proving anything. That is the same property as #214's publish-scrub guard: a
check that cannot pass vacuously. It was not in the three items I asked for, and it is the
difference between a test that verifies something and a test that reads green.

The 60s timeout also carries what it was measured against and why 12x, so whoever has to raise
it next knows what they are trading. A bare number would have been a guess with a suit on.

On the fix that was not chosen

Raising the hook timeout alone would have gone green today. The tarball grew 12.4 MB this week;
the next 12 MB lands back here. Removing work that never needed doing is the fix, and the tool
that measured the regression — npm pack --dry-run --json — was already the answer.

Correction folded in

The builder's first CI report attributed 17 skipped tests to a file containing 5. It found the
remaining 12, named the mechanism (node-pty native binding unresolvable in a CI child), and put
the full accounting in the PR body rather than leaving a number that did not add up. Recorded
here because the correction is part of the record, not a footnote to it.

Out of scope, correctly

The spec-1280 determinism failure hit during this work is not in this PR. It was a moved
HEAD — a porch done committed state mid-suite — and it is filed as #217, where the real
defect is that the test reports "not deterministic" for a case where it could not measure
determinism at all.

Approved. Merge on green CI, --merge, never squash.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

main is red: extension-retirement pack hook exceeds its 10s timeout after #215's build step

1 participant

@pseudoseed
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix #216: extension-retirement pack hook no longer packs a 27MB tarball - #218

Merged
pseudoseed merged 2 commits into
mainfrom
builder/bugfix-216-pack-hook-timeout
Aug 29, 2026
Merged

Fix #216: extension-retirement pack hook no longer packs a 27MB tarball#218
pseudoseed merged 2 commits into
mainfrom
builder/bugfix-216-pack-hook-timeout

Conversation

@pseudoseed

Copy link
Copy Markdown
Owner

Fixes#216.

main went red at 4ebaf36b6 on one suite: extension-retirement.test.ts, whose beforeAll hook timed out at 10000ms after taking 10544ms. 341 files and 6,779 tests passed, and the other five CI jobs were green. Nothing was wrong with the shipped code.

Root Cause

#215 added a Build codev package step to the unit job, so dist/, dashboard-dist/ and v2-dist/ exist when the unit suite runs. They did not before. The root package.json is files: ["*", "!apps/streamdeck", "!apps/vscode"], and that test packs the workspace root — so the pack now sweeps the built output too.

Measured with npm pack --dry-run --json rather than estimated: the build step adds 805 files and 12.4 MB to a root tarball that is now 4,395 files and 27.4 MB packed. A hook already sitting near its ceiling went 544ms over it.

The build step stays. Without it the publish-scrub guard narrows to committed files and passes vacuously, which its own comment says. The test is what changes.

Fix — all three defects in #216

1. The pack was far more work than the assertion needed. It wrote a real 27.4 MB tarball and shelled out to tar -tzf purely to read a list of names. npm pack --dry-run --json returns that list with no tarball written and no tar process — the same idiom #214's publish-scrub guard already uses. Paths come back without the package/ prefix, so the four assertions match on bare repo-relative paths.

Raising a timeout on work that does not need doing would have been the weakest fix available: the tarball grew 12.4 MB this week alone, and the next 12 MB lands right back here.

2. The expensive fixture was in beforeAll, but only 1 of 5 tests used it. The other four read files or run pnpm list and need nothing from the pack. When the hook died, all five were reported skipped — one slow fixture took down four tests that never depended on it. The pack is now a lazy call inside the single test that needs it.

3. There was no explicit timeout. It ran on vitest's 10s default. Now 60s, with the reason beside it: ~4.7s warm locally, roughly 12x headroom for a cold runner and ordinary repo growth, still short enough that a genuine hang fails rather than hanging the job.

The helper also throws on an empty file list, since an empty list satisfies both not.toContain assertions without proving anything.

The 12 unexplained skips, since a number I gave was wrong

Reporting the CI failure, I wrote that "17 tests in that file went to skipped." That file has 5 tests, and CI's own line says (5 tests | 5 skipped). The totals reconciled — local 6,796 + 51 and CI 6,779 + 68 both make 6,847 — but only 5 of the 17 were extension-retirement's. Twelve tests changed state between machine and runner and I attributed them to a file that could not account for them. That is the same defect as reporting a value you are not positioned to observe.

They are found, and they reconcile exactly. CI's per-file skip lines:

FileSkipped in CI
session-manager.test.ts12
tunnel-integration.test.ts21
tunnel-e2e.test.ts13
non-main-default-branch.test.ts8
extension-retirement.test.ts5
default-branch.test.ts4
team-cli.test.ts2
spec-146-t3-contract.test.ts1
spec-146-phase-9-porch-engine.test.ts1
spec-146-phase-9-live-harness.test.ts1
Total68

All 12 are session-manager, and they are CI-gated rather than environmental drift: one describe.skipIf(!!process.env.CI) block holding 5 tests, plus 7 individual it.skipIf(!!process.env.CI) at lines 1218, 1269, 1331, 2077, 2115, 2149, 2187 — counted from the source by brace-matching the block. They spawn real shellper processes and need the node-pty native binding, which a CI child process cannot resolve. They run locally, where CI is unset, and skip in CI. That is exactly why they moved state between the two.

The other 51 are unconditional describe.skip or env-gated and skip in both places — which is the local count exactly.

So 51 + 12 + 5 = 68 in CI, and 51 locally with those 12 and 5 passing gives 6,796. Every number closes, and there is no second failure hiding in the remainder.

One off-subject commit rides along, deliberately

a725af68f — chore(porch): bugfix-214 protocol complete is on this branch and is not about #216. It touches one file, codev/projects/bugfix-214-pre-publish-scrub-a-home-direc/status.yaml, 2 lines.

Porch writes its state commits after a PR merges, onto whatever branch is checked out — and by the time porch done bugfix-214 ran, this branch was already cut from 4ebaf36b6 to fix the red. The bookkeeping is true: #214's protocol is complete, its leak is fixed and on main, and #216 is a separate defect with its own issue rather than unfinished #214 work. It is also porch's own commit, which is the only way status.yaml is allowed to change.

Carrying it here rather than spending a whole PR cycle on 2 lines of bookkeeping. Recorded so nobody loses time next month asking why a #214 commit sits on a #216 branch.

Test Plan

  • Rewritten suite: 5 passed, 4.91s (was a 10s-default beforeAll that took 10.5s in CI).
  • Assertions verified to have teeth, not just to pass: the resolved list is 4,395 entries, carries apps/web/package.json and apps/v2/package.json, has 0 entries under apps/vscode/ or apps/streamdeck/, and no path carries the package/ prefix.
  • Full packages/codev unit suite on a stable HEAD (bc7772121, clean tree): 342 files passed, 3 skipped; 6,796 tests passed, 51 skipped, 0 failed.

An earlier full run showed 1 failure and is not quoted here, because its preconditions were violated rather than because it was inconvenient: spec-1280-measurement-instrument's T12 — determinism compares two instrument invocations byte-for-byte, and porch done bugfix-214 wrote a state commit during that run, so the two invocations stamped different commits (a725af68f vs 4ebaf36b6). Self-inflicted concurrency, not the code and not the test. Both affected files were then re-run with HEAD stable — 2 files, 29 tests, all passing — before the clean full run above.

That hazard is filed separately as #217 and deliberately not touched here: a determinism test whose title states a precondition it never checks reports "I could not tell" with the same signal as "the instrument is non-deterministic".

pseudoseedand others added 2 commits August 29, 2026 14:57
`main` went red at 4ebaf36 on this one suite: the `beforeAll` hook timed
out at 10000ms after taking 10544ms.
#215's `Build codev package` step means `dist/`, `dashboard-dist/` and
`v2-dist/` now exist when the unit suite runs. The root package.json is
`files: ["*", …]` and this test packs the WORKSPACE ROOT, so the pack sweeps
the built output too — measured at 805 files and 12.4 MB added to a root
tarball that is now 4,395 files and 27.4 MB packed. A hook already near its
ceiling went 544ms over.
The build step stays. Without it the publish-scrub guard narrows to committed
files and passes vacuously, which its own comment says. The test changes.
All three defects, not just the one that was red:
The pack was far more work than the assertion needed — it wrote a real
tarball and shelled out to `tar -tzf` purely to read a list of names.
`npm pack --dry-run --json` returns that list with no tarball and no `tar`
process, the same idiom #214's publish-scrub guard uses. Paths come back
without the `package/` prefix, so the assertions match bare repo-relative
paths. Raising a timeout on work that does not need doing would have been the
weakest fix available: the tarball grew 12.4 MB this week alone.
The fixture was in `beforeAll` but only 1 of 5 tests used it, so when the hook
died all five were reported skipped — one slow fixture took down four tests
that never depended on it. The pack is now lazy, inside the test that needs it.
There was no explicit timeout. Now 60s, with the reason beside it.
The helper throws on an empty file list: an empty list satisfies both
`not.toContain` assertions without proving anything.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@pseudoseed

Copy link
Copy Markdown
OwnerAuthor

Architect review — approved on my read

Risk: low. 2 files, 51 additions / 29 deletions, one test file plus 2 lines of porch
bookkeeping. No shipped code. Read by me directly rather than sent to consultation; the triage
table puts this tier at architect-read, and spending three reviewer lanes on a test-timeout fix
while main is red would cost more than it returns.

I pre-reviewed this diff in the worktree before it was pushed, so this is not a rubber stamp on
the description.

Verified independently

  • --ignore-scripts does not change what the pack contains here. That flag would alter the
    file list if the root package had a prepack. It has no prepack, prepare,
    prepublishOnly or postpack. Checked the root package.json directly. The builder checked
    the same thing from its own direction and we agree — two derivations, one answer.
  • The path-shape change is correct.npm pack --dry-run --json returns files[].path
    relative to the package root, with no package/ prefix, so dropping it from all four
    assertions is right and not an oversight.
  • The skip reconciliation. I brace-matched the describe.skipIf(!!process.env.CI) at
    session-manager.test.ts:671: it spans lines 671-855 and holds exactly 5 tests, plus the 7
    it.skipIf at the lines named in the body. 12 exactly. 51 + 12 + 5 = 68 closes on both
    sides.
  • The root cause. Confirmed from the merge diff of Fix #214: no home-directory path reaches a published tarball #215 and the root files: ["*", …], not
    from the summary.

The part that matters most

The helper throws on an empty file list, because an empty list satisfies both not.toContain
assertions without proving anything. That is the same property as #214's publish-scrub guard: a
check that cannot pass vacuously. It was not in the three items I asked for, and it is the
difference between a test that verifies something and a test that reads green.

The 60s timeout also carries what it was measured against and why 12x, so whoever has to raise
it next knows what they are trading. A bare number would have been a guess with a suit on.

On the fix that was not chosen

Raising the hook timeout alone would have gone green today. The tarball grew 12.4 MB this week;
the next 12 MB lands back here. Removing work that never needed doing is the fix, and the tool
that measured the regression — npm pack --dry-run --json — was already the answer.

Correction folded in

The builder's first CI report attributed 17 skipped tests to a file containing 5. It found the
remaining 12, named the mechanism (node-pty native binding unresolvable in a CI child), and put
the full accounting in the PR body rather than leaving a number that did not add up. Recorded
here because the correction is part of the record, not a footnote to it.

Out of scope, correctly

The spec-1280 determinism failure hit during this work is not in this PR. It was a moved
HEAD — a porch done committed state mid-suite — and it is filed as #217, where the real
defect is that the test reports "not deterministic" for a case where it could not measure
determinism at all.

Approved. Merge on green CI, --merge, never squash.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

main is red: extension-retirement pack hook exceeds its 10s timeout after #215's build step

1 participant

@pseudoseed
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Fix #216: extension-retirement pack hook no longer packs a 27MB tarball - #218

Merged
pseudoseed merged 2 commits into
mainfrom
builder/bugfix-216-pack-hook-timeout
Aug 29, 2026
Merged

Fix #216: extension-retirement pack hook no longer packs a 27MB tarball#218
pseudoseed merged 2 commits into
mainfrom
builder/bugfix-216-pack-hook-timeout

Conversation

@pseudoseed

Copy link
Copy Markdown
Owner

Fixes#216.

main went red at 4ebaf36b6 on one suite: extension-retirement.test.ts, whose beforeAll hook timed out at 10000ms after taking 10544ms. 341 files and 6,779 tests passed, and the other five CI jobs were green. Nothing was wrong with the shipped code.

Root Cause

#215 added a Build codev package step to the unit job, so dist/, dashboard-dist/ and v2-dist/ exist when the unit suite runs. They did not before. The root package.json is files: ["*", "!apps/streamdeck", "!apps/vscode"], and that test packs the workspace root — so the pack now sweeps the built output too.

Measured with npm pack --dry-run --json rather than estimated: the build step adds 805 files and 12.4 MB to a root tarball that is now 4,395 files and 27.4 MB packed. A hook already sitting near its ceiling went 544ms over it.

The build step stays. Without it the publish-scrub guard narrows to committed files and passes vacuously, which its own comment says. The test is what changes.

Fix — all three defects in #216

1. The pack was far more work than the assertion needed. It wrote a real 27.4 MB tarball and shelled out to tar -tzf purely to read a list of names. npm pack --dry-run --json returns that list with no tarball written and no tar process — the same idiom #214's publish-scrub guard already uses. Paths come back without the package/ prefix, so the four assertions match on bare repo-relative paths.

Raising a timeout on work that does not need doing would have been the weakest fix available: the tarball grew 12.4 MB this week alone, and the next 12 MB lands right back here.

2. The expensive fixture was in beforeAll, but only 1 of 5 tests used it. The other four read files or run pnpm list and need nothing from the pack. When the hook died, all five were reported skipped — one slow fixture took down four tests that never depended on it. The pack is now a lazy call inside the single test that needs it.

3. There was no explicit timeout. It ran on vitest's 10s default. Now 60s, with the reason beside it: ~4.7s warm locally, roughly 12x headroom for a cold runner and ordinary repo growth, still short enough that a genuine hang fails rather than hanging the job.

The helper also throws on an empty file list, since an empty list satisfies both not.toContain assertions without proving anything.

The 12 unexplained skips, since a number I gave was wrong

Reporting the CI failure, I wrote that "17 tests in that file went to skipped." That file has 5 tests, and CI's own line says (5 tests | 5 skipped). The totals reconciled — local 6,796 + 51 and CI 6,779 + 68 both make 6,847 — but only 5 of the 17 were extension-retirement's. Twelve tests changed state between machine and runner and I attributed them to a file that could not account for them. That is the same defect as reporting a value you are not positioned to observe.

They are found, and they reconcile exactly. CI's per-file skip lines:

FileSkipped in CI
session-manager.test.ts12
tunnel-integration.test.ts21
tunnel-e2e.test.ts13
non-main-default-branch.test.ts8
extension-retirement.test.ts5
default-branch.test.ts4
team-cli.test.ts2
spec-146-t3-contract.test.ts1
spec-146-phase-9-porch-engine.test.ts1
spec-146-phase-9-live-harness.test.ts1
Total68

All 12 are session-manager, and they are CI-gated rather than environmental drift: one describe.skipIf(!!process.env.CI) block holding 5 tests, plus 7 individual it.skipIf(!!process.env.CI) at lines 1218, 1269, 1331, 2077, 2115, 2149, 2187 — counted from the source by brace-matching the block. They spawn real shellper processes and need the node-pty native binding, which a CI child process cannot resolve. They run locally, where CI is unset, and skip in CI. That is exactly why they moved state between the two.

The other 51 are unconditional describe.skip or env-gated and skip in both places — which is the local count exactly.

So 51 + 12 + 5 = 68 in CI, and 51 locally with those 12 and 5 passing gives 6,796. Every number closes, and there is no second failure hiding in the remainder.

One off-subject commit rides along, deliberately

a725af68f — chore(porch): bugfix-214 protocol complete is on this branch and is not about #216. It touches one file, codev/projects/bugfix-214-pre-publish-scrub-a-home-direc/status.yaml, 2 lines.

Porch writes its state commits after a PR merges, onto whatever branch is checked out — and by the time porch done bugfix-214 ran, this branch was already cut from 4ebaf36b6 to fix the red. The bookkeeping is true: #214's protocol is complete, its leak is fixed and on main, and #216 is a separate defect with its own issue rather than unfinished #214 work. It is also porch's own commit, which is the only way status.yaml is allowed to change.

Carrying it here rather than spending a whole PR cycle on 2 lines of bookkeeping. Recorded so nobody loses time next month asking why a #214 commit sits on a #216 branch.

Test Plan

  • Rewritten suite: 5 passed, 4.91s (was a 10s-default beforeAll that took 10.5s in CI).
  • Assertions verified to have teeth, not just to pass: the resolved list is 4,395 entries, carries apps/web/package.json and apps/v2/package.json, has 0 entries under apps/vscode/ or apps/streamdeck/, and no path carries the package/ prefix.
  • Full packages/codev unit suite on a stable HEAD (bc7772121, clean tree): 342 files passed, 3 skipped; 6,796 tests passed, 51 skipped, 0 failed.

An earlier full run showed 1 failure and is not quoted here, because its preconditions were violated rather than because it was inconvenient: spec-1280-measurement-instrument's T12 — determinism compares two instrument invocations byte-for-byte, and porch done bugfix-214 wrote a state commit during that run, so the two invocations stamped different commits (a725af68f vs 4ebaf36b6). Self-inflicted concurrency, not the code and not the test. Both affected files were then re-run with HEAD stable — 2 files, 29 tests, all passing — before the clean full run above.

That hazard is filed separately as #217 and deliberately not touched here: a determinism test whose title states a precondition it never checks reports "I could not tell" with the same signal as "the instrument is non-deterministic".

pseudoseedand others added 2 commits August 29, 2026 14:57
`main` went red at 4ebaf36 on this one suite: the `beforeAll` hook timed
out at 10000ms after taking 10544ms.
#215's `Build codev package` step means `dist/`, `dashboard-dist/` and
`v2-dist/` now exist when the unit suite runs. The root package.json is
`files: ["*", …]` and this test packs the WORKSPACE ROOT, so the pack sweeps
the built output too — measured at 805 files and 12.4 MB added to a root
tarball that is now 4,395 files and 27.4 MB packed. A hook already near its
ceiling went 544ms over.
The build step stays. Without it the publish-scrub guard narrows to committed
files and passes vacuously, which its own comment says. The test changes.
All three defects, not just the one that was red:
The pack was far more work than the assertion needed — it wrote a real
tarball and shelled out to `tar -tzf` purely to read a list of names.
`npm pack --dry-run --json` returns that list with no tarball and no `tar`
process, the same idiom #214's publish-scrub guard uses. Paths come back
without the `package/` prefix, so the assertions match bare repo-relative
paths. Raising a timeout on work that does not need doing would have been the
weakest fix available: the tarball grew 12.4 MB this week alone.
The fixture was in `beforeAll` but only 1 of 5 tests used it, so when the hook
died all five were reported skipped — one slow fixture took down four tests
that never depended on it. The pack is now lazy, inside the test that needs it.
There was no explicit timeout. Now 60s, with the reason beside it.
The helper throws on an empty file list: an empty list satisfies both
`not.toContain` assertions without proving anything.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@pseudoseed

Copy link
Copy Markdown
OwnerAuthor

Architect review — approved on my read

Risk: low. 2 files, 51 additions / 29 deletions, one test file plus 2 lines of porch
bookkeeping. No shipped code. Read by me directly rather than sent to consultation; the triage
table puts this tier at architect-read, and spending three reviewer lanes on a test-timeout fix
while main is red would cost more than it returns.

I pre-reviewed this diff in the worktree before it was pushed, so this is not a rubber stamp on
the description.

Verified independently

  • --ignore-scripts does not change what the pack contains here. That flag would alter the
    file list if the root package had a prepack. It has no prepack, prepare,
    prepublishOnly or postpack. Checked the root package.json directly. The builder checked
    the same thing from its own direction and we agree — two derivations, one answer.
  • The path-shape change is correct.npm pack --dry-run --json returns files[].path
    relative to the package root, with no package/ prefix, so dropping it from all four
    assertions is right and not an oversight.
  • The skip reconciliation. I brace-matched the describe.skipIf(!!process.env.CI) at
    session-manager.test.ts:671: it spans lines 671-855 and holds exactly 5 tests, plus the 7
    it.skipIf at the lines named in the body. 12 exactly. 51 + 12 + 5 = 68 closes on both
    sides.
  • The root cause. Confirmed from the merge diff of Fix #214: no home-directory path reaches a published tarball #215 and the root files: ["*", …], not
    from the summary.

The part that matters most

The helper throws on an empty file list, because an empty list satisfies both not.toContain
assertions without proving anything. That is the same property as #214's publish-scrub guard: a
check that cannot pass vacuously. It was not in the three items I asked for, and it is the
difference between a test that verifies something and a test that reads green.

The 60s timeout also carries what it was measured against and why 12x, so whoever has to raise
it next knows what they are trading. A bare number would have been a guess with a suit on.

On the fix that was not chosen

Raising the hook timeout alone would have gone green today. The tarball grew 12.4 MB this week;
the next 12 MB lands back here. Removing work that never needed doing is the fix, and the tool
that measured the regression — npm pack --dry-run --json — was already the answer.

Correction folded in

The builder's first CI report attributed 17 skipped tests to a file containing 5. It found the
remaining 12, named the mechanism (node-pty native binding unresolvable in a CI child), and put
the full accounting in the PR body rather than leaving a number that did not add up. Recorded
here because the correction is part of the record, not a footnote to it.

Out of scope, correctly

The spec-1280 determinism failure hit during this work is not in this PR. It was a moved
HEAD — a porch done committed state mid-suite — and it is filed as #217, where the real
defect is that the test reports "not deterministic" for a case where it could not measure
determinism at all.

Approved. Merge on green CI, --merge, never squash.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

main is red: extension-retirement pack hook exceeds its 10s timeout after #215's build step

1 participant

@pseudoseed