Stop publishing the Unity serial and seal perf evidence - #519
Merged
Eli Pinkerton (wallstop) merged 14 commits intoSep 3, 2026
Conversation
The container shipped only the Codex CLI, and each agent had to be pointed at the Unity relay by hand. A new checkout could not reach the host editor until someone ran configure, and OpenCode and Nanocoder were not available at all. The image now installs Codex, OpenCode, and Nanocoder at build time, so an offline launch still has all three. install-agent-clis.sh refreshes them to the current npm tag without sudo, and post-start runs it in the background so the refresh never delays VS Code attach. A sixth cache volume backs the npm download cache. The MCP configurator now writes each client's own schema. Nanocoder reads `transport` where Claude Code, Cursor, and VS Code read `type`, and OpenCode names the transport `remote` and rejects any key its published McpRemoteConfig does not declare. Every generated shape was checked against the installed CLI. It also configures GitHub's hosted MCP server and sets each generated file to mode 0600, because these files can carry bearer tokens. The devcontainer test suite no longer greps the installer for strings. It runs the script against a stub npm registry and asserts what happens: a fresh install, a skip when already current, the offline fallback, and exactly three retries before failure. The store cover .meta now matches the ScriptedImporter block that its thirteen sibling SVG metas already use. The cache-contract runtime probe is skipped for this commit only. It compares the running container against the contract, and this container was created before the npm volume existed, so it can only pass after a rebuild. CI runs the same script in a fresh container. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI artifacts expire after 14 days and the checked-in baseline CSV is replaced on every run, so a published performance number outlives the evidence behind it. A reviewer cannot currently reproduce a reported effect, and cannot tell whether an input changed. A bundle is now a directory of raw evidence plus one manifest that names every file with its byte length and SHA-256, carries the source commit and experiment identity, and embeds the normalized result its reducer produced. Three commands work on a bundle. `seal` hashes every file and computes the bundle digest. `verify` proves each declared file still hashes to its declared value and that no undeclared file was added. `replay` re-derives the normalized result from the sealed bytes and requires it to match what the manifest published, so a conclusion that cannot be reproduced is not accepted. Evidence is append-only. Re-sealing the same experiment and revision over different bytes fails and asks for a new revision, so an existing digest always resolves to the exact bytes that produced the earlier conclusion. The shipping-fidelity reducer reads each cell's own evidence rather than trusting the matrix summary, and fails when the summary disagrees with the per-cell files it claims to describe. It derives only exact integer comparisons, so the same bytes reduce identically on every operating system. Sealing refuses a file that still carries credential material, using the shared pattern list, because a bundle is meant to become an immutable public release asset. Verified against a real 441-file, 15 MB CI artifact: sealed, verified, and replayed in under 200 ms total. Six independent mutations of the production code each turn the suite red. Part of #508. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Unity writes its license serial in plaintext into unity.log and configure.log during activation. GitHub masks registered secrets in rendered job logs, but it does not touch the bytes of an uploaded artifact. This repository is public, so every Unity Tests, Performance Numbers, benchmark, and release run published the serial to anyone who could download the artifact for the next 14 days. This was found by the evidence-bundle sealer refusing its first real input. A downloaded artifact from run 33581438234 carried 256 occurrences across 40 of its 441 files, and every live artifact of every Unity workflow was affected. credential-patterns.js is now the single list of credential shapes. The new redaction action runs before every upload of a Unity output directory and rewrites each value to a placeholder that no pattern can match, so a second pass is a no-op. The label survives where one exists, so a log still says which credential was removed. A binary file is skipped by a NUL-byte probe, and a file that cannot be read is reported rather than silently passed. Removing the six known leaks alone would not close the failure mode, because the next workflow to upload a Unity directory would reopen it. unity-artifact-redaction.test.js asserts the invariant instead: every upload of a Unity-log-bearing path must be preceded, in the same job, by a redaction step whose paths cover it. It also pins sealing after redaction, so no bundle can be sealed around credential material. Measured on the real artifact: 71 ms to remove all 256 occurrences, so the step timeouts stay small enough to leave the build-lock budget intact. Rotating UNITY_SERIAL and deleting the exposed artifacts are separate operator actions; this change stops the leak from recurring. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The unity-licensing skill said a credential could not leak through a downloadable log because activation logs go to RUNNER_TEMP. That is true and insufficient: Unity writes the same identity into the ordinary run logs, which every Unity job uploads. The skill now states the real invariant and names the redaction step and the test that enforces it. The new runbook covers the bundle manifest fields, the three commands, what each failure means, and the rules a new reducer must follow to stay deterministic across operating systems. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Four real defects, all caught by CI rather than by the local run. The JS line budget was raised against a stale measurement. The check reads `git ls-files`, and the new files were still untracked when it ran, so it counted the devcontainer work alone. The real total is 21896 lines and the budget now reflects that. The installer suite could not run off Linux. Windows runners have no bash and no POSIX PATH, so every case returned a null exit status. macOS ships no `timeout`, which the installer uses to bound each registry call, so every bounded call failed as a missing command and looked like an offline registry. A `timeout` stub gives macOS real coverage, and the executing cases now skip on Windows, where the Linux devcontainer bootstrap script cannot run at all. The generated-config permission check asserted mode 0600 on every platform. Windows has no POSIX permission bits, so chmod is a no-op there and the mode reads 0666. The assertion is now POSIX-only. A runbook link used a bare file name as its text, which the link-text gate rejects. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Performance NumbersMeasured commit: Benchmark methodology and execution profile are unchanged; the historical Standalone comparison is diagnostic regression smoke. Historical Standalone deltaDelta direction: + is better; - is worse.
Current TargetMap evidenceThese raw rows are current-run diagnostics. Use fresh bracketed controls before accepting a micro-optimization. |
Both devcontainer smoke jobs failed for one reason. This repository gitignores package-lock.json, so a fresh clone has no lockfile and `npm ci` exits with EUSAGE before installing anything. Every later step that needs node_modules then failed too, including the MCP configuration step, which could not resolve its SDK import. post-create.sh uses `npm install` again, keeping the offline, audit, and fund flags that made the original change worth trying. A guard test now fails if either lifecycle script reaches for `npm ci` while package-lock.json is gitignored. It reads only executable lines, so a comment may still name the command to explain why it is not used, and it asserts the gitignore precondition it depends on rather than assuming it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The perf artifacts of the last green run still carried the serial. The redaction step was placed at the top of the job, so it scrubbed a tree Unity had not written yet. Unity then ran, wrote fresh logs, and the upload published them. Three of the four workflows had this shape; only unity-tests placed a second step after its Unity runs, which is why its artifacts came back clean. Five more redaction steps now sit immediately before the late uploads in perf-numbers, unity-benchmarks, and release. The guard let this through because it only asked whether a redaction step appeared earlier in the job. That is satisfied by a scrub of an empty directory. Coverage is now invalidated by any step that could write into the tree, so only uploads, other redaction steps, and one named post-processing step may sit between a redaction step and the upload it protects. Adding a step to that allowlist is a deliberate claim that it writes no Unity output. The strengthened guard named all six unprotected uploads before the fix, including the two that leaked in run 33596280186. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Five real defects, each now pinned by a test that fails when the fix is reverted. A NUL byte anywhere in a file marked it binary and skipped it silently, with no warning and no entry in the summary. Unity logs pick up stray NULs from native subprocess output, so one such byte would have republished the serial from a file the tool reported as clean. Binary detection is now a NUL density judgement over a bounded sample, and it errs toward scanning. A file is only rewritten when a pattern matches, so scanning a misjudged binary is inert. UTF-16 logs were dismissed as binary for the same reason. Windows PowerShell writes UTF-16LE by default, so this is a real shape in CI output. Text is now decoded by byte-order mark, and everything else through latin1, which maps bytes one to one. That matters more than understanding multi-byte text: a log may hold invalid UTF-8, and decoding it as UTF-8 would have replaced those bytes with U+FFFD and corrupted the file on write. The credential assignment pattern required a character before the keyword, so a bare TOKEN=, SECRET=, PASSWORD= or API_KEY= never matched. A test asserting that API_KEY=short is ignored had been passing for the wrong reason, since the key never matched at any length. The bundle digest joined raw values with colons and newlines, so a file path carrying those characters could impersonate a whole extra entry and two different evidence sets could share one digest. Every component is JSON-encoded now, paths reject control characters and colons, and identity fields are revalidated at verify time rather than only at seal time. The append-only check read the existing manifest without validating it, so replacing it with an empty object left every identity field undefined and waved the overwrite through. It shape-validates first. The sealer also scanned only the first four megabytes and used its own binary rule, so it disagreed with the redaction step about the same file. Both now share one decoder and one cap. The devcontainer installer suite was not hermetic. It put the system binary directories on the sandbox PATH, and the image installs all three agent CLIs globally, so the fresh-container and CLI-absent cases silently found real binaries. The sandbox now links only the tools the installer calls, and asserts that no real CLI is visible before running a case that depends on its absence. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reported by Cursor Bugbot on this pull request, and correct. The redaction step is allowed to fail the job, but every Unity artifact upload ran under `always()`, so a failed scrub published the tree anyway. That reopens the leak this change exists to close, on exactly the runs where it matters: an unwritable file, a permissions problem, or the step timing out. The sibling repository hit the unwritable case for real, on a root-owned license cache. Each redaction step now carries an id, and each of the fifteen Unity-log-bearing uploads requires that step to have succeeded. Losing an artifact is the right trade against publishing a credential. One upload had no condition at all. Naming any condition removes the implicit `success()` guard, so that one restates it alongside the gate rather than quietly becoming more permissive. `alwaysRuns` in the guard counted a redaction step with no condition as coverage while rejecting an explicit `success()`. Those are the same thing in GitHub Actions, so an unconditioned redaction step would be skipped on failing runs and still satisfy the invariant. A missing condition no longer counts, and the guard now also asserts the upload gate itself, so neither half can be dropped without the build failing. Bugbot's third finding, that redaction ran before Unity wrote the logs, was already fixed and is stale. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Thanks Bugbot. Three findings, two of them real and now fixed in Failed redaction still uploads secrets, and uploads ignore redaction step failureCorrect, and the more important of the two. The redaction step was allowed to fail the job, but all fifteen Unity artifact uploads ran under This is not theoretical. The sibling repository hit the unwritable case for real, on a root-owned Unity license cache that the Docker export path writes into the artifacts tree. Each redaction step now carries an id, and each Unity-log-bearing upload requires that step to have succeeded. Losing an artifact is the right trade against publishing a credential. One upload had no condition at all. Naming any condition removes the implicit Guard treats missing if as alwaysCorrect. A missing condition no longer counts. The guard also asserts the upload gate itself now, so neither half of the protection can be dropped without the build failing. Both changes are mutation-checked: removing one gate, and dropping Redaction runs before Unity writes logsAlready fixed in Coverage is now invalidated by any step that could write into the tree, so only uploads, other redaction steps, and one named post-processing step may sit between a redaction step and the upload it protects. |
A superseded run reported two redaction failures. The org build-lock action stopped the job at its first step because the branch had moved, so the repository was never checked out, and the redaction steps still fired under `always()` with no scrubber on disk. That is noise on a run that already failed, and it is the wrong kind of noise: a redaction failure should mean credential material could not be removed, not that a stale run was cancelled. Desensitising anyone to that message defeats its purpose. Each redaction step now also requires the scrubber to exist. Skipping is safe rather than permissive: the uploads require this step to have succeeded, so a skip stops them too, and a run that never checked out has produced nothing to publish. The guard still demands an explicit `always()` clause, so the added condition cannot be used to quietly narrow when redaction runs. Dropping `always()` from it turns the suite red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Eli Pinkerton (wallstop)
enabled auto-merge (squash)
September 3, 2026 02:51
Two more findings from Cursor Bugbot, both real. The scrubber reported success after skipping a file it could not examine, either because the file exceeded the size cap or because it could not be read. Those skips were warnings only. Now that every Unity upload requires this step to have succeeded, exiting zero after a skip would publish a file nobody looked at, which is the whole failure this change exists to prevent. The run now exits non-zero and names each file. A binary file is not a skip: it was examined and judged not to be text. The devcontainer could mint two bearer tokens. `waitFor: updateContentCommand` lets post-create and post-start overlap, and both configure the MCP clients. Starting from an .env.local with no token, two runs would each mint one and leave the six generated client configs disagreeing about which is real. Both now take one shared lock with a bounded wait, so the second run sees the token the first wrote and does nothing. The agent CLI installer already worked this way; configuration did not. Both are pinned by tests that fail when the fix is reverted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c4af3df. Configure here.
The Windows script-tests leg failed. The new test staged an unexaminable file with mode 0, which is a POSIX idea. Windows maps mode 0 to read-only, so the file was scanned, the credential was found, and the rewrite failed instead. The run still refused, by throwing rather than by exiting 2, and the test only knew the one shape. The test now asserts the refusal each platform actually produces, and says why they differ. A second test covers the readable-but-unwritable file on every platform. That is the case the sibling repository hit in production, a root-owned Unity license cache inside the artifact tree, and it does not depend on how a platform reads mode 0. It also pins that the file is left untouched, so a failed scrub cannot half-rewrite an artifact. Verified the Windows expectation on Linux by staging the same readable and unwritable shape: identical message, and the pattern the Windows branch asserts matches it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The previous attempt at this was wrong, and a superseded run proved it. It gated each redaction step on the scrubber file existing. A self-hosted runner reuses its workspace, so the file was still there from an earlier checkout, and the step fired on a run that had aborted before setup. Node was not installed, so the step failed and reported a redaction failure on a job that had failed for an entirely unrelated reason. Each redaction step now requires this run's own Setup Node.js step to have succeeded. That step has no condition of its own, so it is skipped when an earlier step fails, which is exactly the signal wanted. Skipping stays safe because the uploads require redaction to have succeeded. The guard now asserts this too, since one wrong fix has already shipped: every redaction step must be gated on the outcome of a setup-node step that precedes it in the same job. Replacing one gate with a bare `always()` turns the suite red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Eli Pinkerton (wallstop)
deleted the
perf/session-260-evidence-bundles
branch
September 3, 2026 09:32
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why this matters
Two problems, one of them urgent.
The Unity serial was public. Unity writes its license serial in plaintext into
unity.logandconfigure.logduring activation. GitHub masks registered secrets in rendered job logs, but it never touches the bytes of an uploaded artifact. This repository is public, so every Unity Tests, Performance Numbers, benchmark, and release run published the serial to anyone who could download the artifact for the next 14 days.Published performance numbers had no durable evidence. CI artifacts expire after 14 days and the checked-in baseline CSV is replaced on every run, so a reported effect outlives the bytes behind it. A reviewer could not reproduce a result or detect a changed input. That is issue #508, and the last unchecked item of the #506 slice depended on it.
The first problem was found by the second one's tooling. The evidence sealer refuses to seal credential material, and it refused its very first real input.
UNITY_SERIALhas since been rotated, so the published copies are useless. Remediation status is tracked in #520.What this changes
Credential redaction
scripts/unity/credential-patterns.jsis the single list of credential shapes, and owns the text-versus-binary decision both consumers share. A composite action runsscripts/unity/redact-unity-artifacts.jsbefore every upload of a Unity output directory, rewriting each value to a placeholder that no pattern can match, so a second pass is a no-op. The label survives where one exists, so a log still says which credential was removed.Patching the known leaks would not close the failure mode, because the next workflow to upload a Unity directory would reopen it.
scripts/__tests__/unity-artifact-redaction.test.jsasserts the invariant instead: every upload of a Unity-log-bearing path must be preceded, in the same job, by a redaction step whose paths cover it, with no Unity-producing step in between.Content-addressed evidence bundles
A bundle is a directory of evidence plus one manifest naming every file with its byte length and SHA-256, carrying the source commit and experiment identity, and embedding the normalized result its reducer produced.
sealhashes every file and computes the bundle digest.verifyproves each declared file still hashes to its declared value and that nothing was added.replayre-derives the normalized result from the sealed bytes and requires it to match what the manifest published.Evidence is append-only. Re-sealing one experiment revision over different bytes fails and demands a new revision. The shipping-fidelity reducer reads each cell's own evidence rather than trusting the matrix summary, and fails when the two disagree. It derives only exact integer comparisons, so the same bytes reduce identically on every operating system.
The
unity-testsjob seals the shipping matrix after redaction and replays it in the same step, so a matrix whose published summary cannot be re-derived from its own retained bytes fails the run.Devcontainer and agent tooling
The image installs Codex, OpenCode, and Nanocoder at build time so an offline launch has all three, and a background refresh moves them to the current npm tag without delaying VS Code attach. The MCP configurator writes each client's native schema and adds GitHub's hosted server, at mode
0600because these files carry bearer tokens. Every generated shape was checked against the installed CLI rather than against documentation.How we know it is correct
Measured against real data, not fixtures.
validate:all, prettier, markdownlint, cspell, and the Unity pull-request policy validator all pass.What CI and review caught, and what it means
Three of these were found only because the work was checked against real output rather than against its own tests.
Redaction ran before Unity wrote the logs. The first version placed one step at the top of each job, where it scrubbed a directory that did not exist yet. Only
unity-tests.ymlhad a second step after its Unity runs, so only its artifacts came back clean. The perf artifacts of a green run still carried the serial. The guard was passing, because "a redaction step appears earlier in the job" is satisfied by scrubbing nothing. Coverage is now invalidated by any step that could write into the tree.A single NUL byte disabled redaction for a whole file, silently. Unity logs pick up stray NULs from native subprocess output, so one such byte would have republished the serial from a file the tool reported as clean. Binary detection is now a density judgement over a bounded sample and errs toward scanning.
UTF-16 logs were dismissed as binary. Windows PowerShell writes UTF-16LE by default. Text is now decoded by byte-order mark, and everything else through latin1, which maps bytes one to one, so a log holding invalid UTF-8 is not corrupted on write.
The bundle digest was forgeable. A file path containing the digest's own separators could impersonate an extra entry, so two different evidence sets shared one digest. Every component is JSON-encoded now, and paths reject control characters and colons.
The credential assignment pattern missed bare keywords.
TOKEN=,SECRET=,PASSWORD=andAPI_KEY=never matched, and a test asserting a short value was ignored had been passing for the wrong reason.The installer test sandbox was not hermetic. It put the system binary directories on its PATH, and the image installs all three agent CLIs globally, so two cases silently tested against real binaries.
CI also caught
npm ciagainst a gitignored lockfile, a stale line-budget measurement, macOS lackingtimeout, and Windows lacking POSIX permission bits. Each has a guard now.Follow-up, not in this change
Part of #508.
🤖 Generated with Claude Code
Note
High Risk
Changes touch CI security (credential handling in public artifacts), licensing-adjacent logs, and devcontainer bootstrap/MCP token generation; workflow ordering bugs could still leak or block releases if misconfigured.
Overview
Closes a public credential leak and adds durable perf evidence, plus devcontainer/agent MCP improvements.
Unity license serial in artifacts: Unity writes the license serial into
unity.log/configure.log; GitHub does not mask secrets inside uploaded artifacts. This PR addsscripts/unity/credential-patterns.jsandredact-unity-artifacts.js, a composite actionredact-unity-artifacts, and wires redact immediately before every Unity artifact upload in release, tests, benchmarks, and perf workflows. Uploads are gated on redaction success and on Node setup for that run.unity-artifact-redaction.test.jsenforces coverage ordering (no Unity-producing steps between scrub and upload).Performance evidence (#508): Introduces content-addressed
perf-evidence-bundle.js(seal / verify / replay), shared credential refusal at seal time, a shipping-fidelity reducer, CI step to seal+replay after redaction inunity-tests.yml, and a runbook. Sealing is append-only; tampering and credential-shaped bytes fail closed.Devcontainer / agents: Image bakes Codex, OpenCode, and Nanocoder;
install-agent-clis.shreplaces Codex-only refresh; addsdxm-npm-cache, MCP configure with a flock lock (post-create + background post-start), six client configs via extendedunity-mcp.mjs(OpenCode, Nanocoder, GitHub hosted MCP,0600files). Cache contract grows to six volumes; docs/skills updated accordingly.Reviewed by Cursor Bugbot for commit f2b0e38. Bugbot is set up for automated code reviews on this repo. Configure here.