Repository navigation
Cleanup foundations: dead code and cold-path performance - #249
Merged
Merged
Conversation
oddharsh
force-pushed
the
codex/site-cleanup-foundations
branch
from
August 7, 2026 16:06
554cbb0 to
2d5d1bc
Compare
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
aadhar-sh | 3b6fe4a | Aug 07 2026, 07:57 PM |
oddharsh
marked this pull request as ready for review
August 7, 2026 19:56
oddharsh
enabled auto-merge (squash)
August 7, 2026 19:56
oddharsh
added a commit
that referenced
this pull request
Aug 7, 2026
#249 landed the behavioural fix for this while this branch was open: the report-only twin no longer carries `upgrade-insecure-requests`, which a browser ignores there and files a security issue about on every page load. So this is no longer the fix. What is left is the trap in how that fix is derived, plus the half of the assertion the test cannot currently make. CSP_REPORT_ONLY_TAIL was `CSP_TAIL.replace(/; upgrade-insecure-requests$/, "")`, anchored to the end of the string. Append one more directive after upgrade-insecure-requests and the replace matches nothing, the twin silently gets the directive back, and no test fails. Invert it: CSP_TAIL_REPORTABLE is the short tail, CSP_TAIL composes the long one from it. There is no pattern to miss, so the failure mode does not exist rather than being guarded against. The test asserted only that the twin OMITS the directive, which passes just as happily if it falls out of both policies. Added the enforcing half, and an equality that pins the two tails to one policy minus one inert directive rather than two policies free to drift. Verified both new assertions bite: putting the directive back in the twin fails 1 test, dropping it from the enforcing policy fails 2. 161 tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
oddharsh
added a commit
that referenced
this pull request
Aug 7, 2026
…#257) #249 landed the behavioural fix for this while this branch was open: the report-only twin no longer carries `upgrade-insecure-requests`, which a browser ignores there and files a security issue about on every page load. So this is no longer the fix. What is left is the trap in how that fix is derived, plus the half of the assertion the test cannot currently make. CSP_REPORT_ONLY_TAIL was `CSP_TAIL.replace(/; upgrade-insecure-requests$/, "")`, anchored to the end of the string. Append one more directive after upgrade-insecure-requests and the replace matches nothing, the twin silently gets the directive back, and no test fails. Invert it: CSP_TAIL_REPORTABLE is the short tail, CSP_TAIL composes the long one from it. There is no pattern to miss, so the failure mode does not exist rather than being guarded against. The test asserted only that the twin OMITS the directive, which passes just as happily if it falls out of both policies. Added the enforcing half, and an equality that pins the two tails to one policy minus one inert directive rather than two policies free to drift. Verified both new assertions bite: putting the directive back in the twin fails 1 test, dropping it from the enforcing policy fails 2. 161 tests pass. Co-authored-by: Aadharsh Pannirselvam <19518661+oddharsh@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
oddharsh
added a commit
that referenced
this pull request
Aug 24, 2026
…uarded ENFORCE_PAGE_HASHES was pinned true on 2026-08-16 and nothing has flipped it in either direction since, so its false arm has been dead code for a week. It was not one line: it cost a second header name, a second tail constant, a `tail` parameter threaded through both policy builders to feed that constant, an entries loop in withSecurityHeaders whose only job was carrying a second header past a bail, and two branches apiece in the contract test and the twin-fact tripwire. The emitted policy is byte-identical, checked both ways. CSP_TAIL now holds the string CSP_TAIL_REPORTABLE plus "; upgrade-insecure-requests" used to compose, and a build of each side produces the same _headers line. cspHeadersFor returns exactly ONE header now. The contract test counts the keys rather than probing one name, since a twin under any other spelling is the same regression, and the upgrade-insecure-requests assertion inverts with the change: it used to check the twin OMITTED the directive, and with one policy left the only way to get it wrong is to lose it, so it now checks presence on both arms. gen-md-twins no longer reads the flag. It reads the header set cspHeadersFor actually emits, which is the better check for the reason the flag era proved: a flag states an intention, and #249's bug was the enforcing half never being applied to a single document while the flag said it was. /security claims 'unsafe-inline' left script-src, and that claim goes false the moment a report-only twin returns, so the deploy fails until one of the two is put right. Controls, both run: reintroducing a report-only twin fails the contract test by name (it reports the two keys it found) and fails the twin-fact tripwire by name. Reverting makes both green again. The rollout story stays. What the DevTools sweep found on /garage/horizon, and why the enforcing half had never been applied, are gotcha 17 in CLAUDE.md, which is where it belongs now that the mechanism is gone. Rolling back is `git revert` rather than a flag, and `bun run csp:sweep` is the signal that would call for one; its header says so instead of describing a flip that already happened. 353 tests, typecheck, lint, build, pages, photos and the 142-route oracle green. The build's taste warnings on this branch are the ones #525 fixes, not new.
oddharsh
added a commit
that referenced
this pull request
Aug 24, 2026
…uarded ENFORCE_PAGE_HASHES was pinned true on 2026-08-16 and nothing has flipped it in either direction since, so its false arm has been dead code for a week. It was not one line: it cost a second header name, a second tail constant, a `tail` parameter threaded through both policy builders to feed that constant, an entries loop in withSecurityHeaders whose only job was carrying a second header past a bail, and two branches apiece in the contract test and the twin-fact tripwire. The emitted policy is byte-identical, checked both ways. CSP_TAIL now holds the string CSP_TAIL_REPORTABLE plus "; upgrade-insecure-requests" used to compose, and a build of each side produces the same _headers line. cspHeadersFor returns exactly ONE header now. The contract test counts the keys rather than probing one name, since a twin under any other spelling is the same regression, and the upgrade-insecure-requests assertion inverts with the change: it used to check the twin OMITTED the directive, and with one policy left the only way to get it wrong is to lose it, so it now checks presence on both arms. gen-md-twins no longer reads the flag. It reads the header set cspHeadersFor actually emits, which is the better check for the reason the flag era proved: a flag states an intention, and #249's bug was the enforcing half never being applied to a single document while the flag said it was. /security claims 'unsafe-inline' left script-src, and that claim goes false the moment a report-only twin returns, so the deploy fails until one of the two is put right. Controls, both run: reintroducing a report-only twin fails the contract test by name (it reports the two keys it found) and fails the twin-fact tripwire by name. Reverting makes both green again. The rollout story stays. What the DevTools sweep found on /garage/horizon, and why the enforcing half had never been applied, are gotcha 17 in CLAUDE.md, which is where it belongs now that the mechanism is gone. Rolling back is `git revert` rather than a flag, and `bun run csp:sweep` is the signal that would call for one; its header says so instead of describing a flip that already happened. 353 tests, typecheck, lint, build, pages, photos and the 142-route oracle green. The build's taste warnings on this branch are the ones #525 fixes, not new.
oddharsh
added a commit
that referenced
this pull request
Aug 24, 2026
…uarded (#527) ENFORCE_PAGE_HASHES was pinned true on 2026-08-16 and nothing has flipped it in either direction since, so its false arm has been dead code for a week. It was not one line: it cost a second header name, a second tail constant, a `tail` parameter threaded through both policy builders to feed that constant, an entries loop in withSecurityHeaders whose only job was carrying a second header past a bail, and two branches apiece in the contract test and the twin-fact tripwire. The emitted policy is byte-identical, checked both ways. CSP_TAIL now holds the string CSP_TAIL_REPORTABLE plus "; upgrade-insecure-requests" used to compose, and a build of each side produces the same _headers line. cspHeadersFor returns exactly ONE header now. The contract test counts the keys rather than probing one name, since a twin under any other spelling is the same regression, and the upgrade-insecure-requests assertion inverts with the change: it used to check the twin OMITTED the directive, and with one policy left the only way to get it wrong is to lose it, so it now checks presence on both arms. gen-md-twins no longer reads the flag. It reads the header set cspHeadersFor actually emits, which is the better check for the reason the flag era proved: a flag states an intention, and #249's bug was the enforcing half never being applied to a single document while the flag said it was. /security claims 'unsafe-inline' left script-src, and that claim goes false the moment a report-only twin returns, so the deploy fails until one of the two is put right. Controls, both run: reintroducing a report-only twin fails the contract test by name (it reports the two keys it found) and fails the twin-fact tripwire by name. Reverting makes both green again. The rollout story stays. What the DevTools sweep found on /garage/horizon, and why the enforcing half had never been applied, are gotcha 17 in CLAUDE.md, which is where it belongs now that the mechanism is gone. Rolling back is `git revert` rather than a flag, and `bun run csp:sweep` is the signal that would call for one; its header says so instead of describing a flip that already happened. 353 tests, typecheck, lint, build, pages, photos and the 142-route oracle green. The build's taste warnings on this branch are the ones #525 fixes, not new. Co-authored-by: Aadharsh Pannirselvam <19518661+oddharsh@users.noreply.github.com>
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.
Summary
Audit scope
Reviewed the current origin/main checkout across 1,164 tracked files, 59 registered public surfaces, all Worker modules, generated/static page boundaries, route/config parity, dependencies, exports, duplicate code, photo artifacts, auxiliary Workers, and every public subpage in a live browser sweep.
Notable restraint:
Validation
No deployment performed.