chore: salvage the unique work from embeddedos-org before archiving it - #159
chore: salvage the unique work from embeddedos-org before archiving it#159srpatcha wants to merge 1 commit into
Conversation
The organisation has two near-identical website repositories with no common
git ancestor. This one serves the live site; embeddedos-org serves a 404:
$ gh api repos/embeddedos-org/embeddedos-org/pages
status: 404 url: None source: None
and it has kept receiving real work regardless — an SEO canonical fix, a
duplicate-element bug fix and a null-dereference guard, all as recently as
2026-08-30, applied to a site nobody can load.
Six files exist there and not here. Ported so that repository can be
archived without losing them:
.github/dependabot.yml
.github/workflows/health-check.yml weekly uptime check
.github/workflows/lint-readme.yml README lint on push
tests/simulation/test_mobile_full_simulation.py
package-lock.json
scripts/build-deploy.py HTML/CSS/JS minification
Also removes style.css.bak, an orphan that the dead repository had already
cleaned up and this one still carried.
build-deploy.py needed fixing before it could be taken. It operated in
place: it minified the tracked sources over themselves and then
for d in ['tests', 'test-screenshots', '.github']:
p = ROOT / d
if p.exists(): shutil.rmtree(p)
deleted the CI configuration and the test suite from the working tree. It
is named "build for the deploy branch" and never made a branch — it mutated
whatever checkout it was run in. I found this by running it, which cost me
the .github and tests directories.
It now stages a copy into dist/ and minifies that, so the source tree is
never written to. A build step must not be able to damage the thing it is
building from.
Verified:
working tree hash before=84e3fd378a71 after=84e3fd378a71 UNCHANGED
.github and tests survive the run
dist/ 97 files; tests, .github and scripts correctly excluded
index.html 96,362 -> 92,410 bytes, title/canonical/</html> intact
32 files minified, 32,179 bytes saved
dist/ was already in .gitignore.
Refs #158
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
srpatcha
left a comment
There was a problem hiding this comment.
Review — embeddedos-org.github.io#159 "chore: salvage the unique work from embeddedos-org before archiving it"
head: a6a538b author: srpatcha ci: fail
Verdict: The build-deploy.py rewrite is the right call and the in-place destruction
is genuinely gone — I ran it and the source tree is untouched. But the script it becomes
emits JavaScript that does not parse: 4 of the 5 JS files in dist/ fail node --check,
including all three of the site's own. Two of the other five ported files do not work in
this repo either — the lockfile is out of sync with this package.json, and the Python
test is executed by nothing.
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | High | scripts/build-deploy.py:29 |
minify_js strips comments with re.sub(r'//[^\n]*', '', text), which is not comment-aware: it truncates every line at the first //, including inside string literals. Any line containing a URL loses everything from https: onward, leaving an unterminated string. dist/js/site-chrome.js becomes { href: 'https:. 4 of 5 emitted JS files fail node --check; the same 4 sources pass. The site's nav bar, search and chat widget would all be dead in a deployed dist/. |
Do not hand-roll this. terser and esbuild are both single dependencies and both already correct. If a dependency is unacceptable, drop minify_js entirely and ship the JS unminified — 32 KB saved across the whole tree is not worth shipping a broken bundle. |
| 2 | High | package-lock.json |
The lockfile was ported from a repo whose package.json declared html-validate: ^8.0.0; this repo declares ^11.10.0. npm ci fails outright. The three workflows that run npm install instead (pr-check.yml:22, deploy.yml:25, nightly.yml:24) will silently rewrite it, so the "pinned dependency tree" the PR adds is not pinned anywhere it is used and is a hard failure in ci.yml:24/:56, which do use npm ci. |
Regenerate it in this repo: rm package-lock.json && npm install, commit the result. Do not port a lockfile between repos with different manifests. |
| 3 | Medium | tests/simulation/test_mobile_full_simulation.py |
Nothing runs it. The only Python in CI is check-canon.yml:37 (python scripts/check-product-canon.py); no workflow invokes pytest, and pr-check.yml:35's npx playwright test tests/ collects *.spec.js only. The PR table lists this file as "mobile coverage" — it is a file, not coverage. Eight other Python suites already under tests/ are dark for the same reason. |
Either add a job that runs it (with a python -m pytest tests/ step and the server on the right port), or say in the PR that it is ported for preservation and not yet wired up. §28 does not let a file stand in for a test report. |
| 4 | Medium | tests/simulation/test_mobile_full_simulation.py:14-20, 23 |
Two import-time side effects. The except ImportError block runs pip install playwright and playwright install chromium --with-deps — the latter shells out to the system package manager — so merely importing this module mutates the machine. And line 23 does SCREENSHOTS_DIR.mkdir(parents=True, exist_ok=True) at module level, creating test-screenshots/mobile/ on collection alone. It also hardcodes BASE_URL = "http://localhost:8777" while every other harness in this repo uses 8080 (package.json:7, pr-check.yml:33), so if it were wired up it would target a port nothing serves. |
Move the playwright import failure to a pytest.importorskip, move the mkdir into the fixture or function that writes screenshots, and read the base URL from BASE_URL with 8080 as the default. |
| 5 | Medium | .github/dependabot.yml |
This adds Dependabot to a repo that has never had it, while embeddedos-org#7 — same author, open right now — deletes .github/dependabot.yml and reports in its body that Dependabot alerts and automated security fixes were "turned off via the API on all 26 repositories" with "90 open Dependabot PRs closed org-wide". Both cannot be the intent. As things stand the ported config either produces nothing, because the repo-level toggle is off, or re-opens the noise #7 was closing. |
Decide once, org-wide, and make the two PRs agree. If #7's position holds, drop this file from the salvage set; the config is preserved in embeddedos-org/.github/.github/dependabot-template.yml regardless, which the file's own header points at. |
| 6 | Low | scripts/build-deploy.py:24 |
minify_css includes + in r'\s*([{};:,>~+])\s*', so calc(100% + 10px) would become calc(100%+10px), which is invalid CSS. This repo's two calc() uses are both subtraction (style.css:215-216) and survive, so nothing is broken today — it is a latent trap for the next stylesheet. |
Drop + and ~ from that class, or skip the contents of calc(). |
| 7 | Low | scripts/build-deploy.py:36 |
minify_html's comment pattern <!--(?!.*\[if).*?--> runs under re.DOTALL, so the negative lookahead scans the rest of the whole document. One conditional comment anywhere in a file disables comment stripping for every comment before it. Also, [ \t]+ → ' ' on line 37 is applied to the entire document including <pre>, <code>, <script> and <textarea>. index.html has 7 <pre> blocks; I diffed them and they survive because the code inside is written flush-left, so this is latent rather than live. |
Anchor the lookahead to the comment body (<!--(?!\[if)(?:(?!-->).)*?-->), and exclude preformatted regions from whitespace collapsing. |
Finding 1, run against this PR's head:
$ python3 scripts/build-deploy.py
✓ Minified 32 files, saved 32,179 bytes
✓ Deploy tree written to dist/ (source tree untouched)
$ for f in $(find dist -name '*.js'); do node --check "$f" || echo "SYNTAX ERROR: $f"; done
SYNTAX ERROR: dist/playwright.config.js
SYNTAX ERROR: dist/js/ebot-chat.js
SYNTAX ERROR: dist/js/search.js
SYNTAX ERROR: dist/js/site-chrome.js
# clean=1 broken=4
$ for f in js/*.js; do node --check "$f"; done
# src clean=4 broken=0
What it does to the nav bar:
js/site-chrome.js:13 { href: 'https://embeddedos-org.github.io/eApps/', label: '\u{1F3EA} App Store', ... }
dist/js/site-chrome.js:8 { href: 'https:
Finding 2:
$ npm ci --ignore-scripts
npm error code EUSAGE
npm error `npm ci` can only install packages when your package.json and
npm error package-lock.json ... are in sync.
npm error Invalid: lock file's html-validate@8.29.0 does not satisfy html-validate@11.12.0
npm error Invalid: lock file's @html-validate/stylish@4.3.0 does not satisfy @html-validate/stylish@6.0.0
npm error Invalid: lock file's @sidvind/better-ajv-errors@3.0.1 does not satisfy @sidvind/better-ajv-errors@7.0.0
npm error Invalid: lock file's fast-uri@3.1.6 does not satisfy fast-uri@3.1.7
The parts that hold up, verified:
- The in-place destruction is gone.
.github/,tests/andscripts/all survive the
run,SOURCE_ONLYexcludes them fromdist/, anddist/contains 97 files with none
of those three present. Thestage_tree()top-level-only pruning is correct — a nested
content directory namedscriptswould survive, as the comment claims. index.html96,374 → 92,422 bytes (4.1%), matching the body's figures to within 12
bytes, with<title>,rel="canonical"and</html>all retained.style.css.bakis a genuine orphan:git grep style.css.bak origin/masterreturns
nothing. Removing it is right.- All six ported paths are genuinely absent from
master, so nothing here overwrites
existing work.
Architecture conformance
Conforms. Master design §21 places website and CI templates under Infrastructure, and
every file in this PR is one of those. Nothing crosses a tier boundary, so §5.1 is not
engaged; §21.1's split policy is what the whole exercise serves — two website
repositories with no common ancestor is the "separate repository for a branded name"
outcome §21.1 warns against, and consolidating to the one that actually serves the site
is the right direction.
§28 is where findings 1, 2 and 3 land. The PR's "Verified" block is careful and its
claims are true, but they cover only what was checked: the working-tree hash, the dist/
exclusions, and three HTML markers. Nothing in it speaks to whether the minified
JavaScript parses, whether the lockfile resolves in this repo, or whether the Python
suite runs — and all three fail. That is the §28 distinction between Implemented and
Validated, and it is why the brief treats an unverified "verified" as the finding.
I am not appending a design-doc proposal for this PR. The consolidation it serves is
already the right reading of §21.1, and finding 5 is an org policy decision for a human,
not a gap in the document's wording.
Proposed changes
Smallest sequence that keeps the salvage while dropping what does not work:
- Drop
minify_jsfromscripts/build-deploy.py, or replace the whole minifier with
esbuild. Then re-run and re-check:for f in $(find dist -name '*.js'); do node --check "$f"; done
must be silent. This is the one item that must not merge as written — the script's
stated purpose is to produce a deployable tree, and it does not. rm package-lock.json && npm install && git add package-lock.json, then confirm
npm cisucceeds.- Resolve finding 5 with #7 before either merges.
- Findings 3, 4, 6 and 7 can follow, or
test_mobile_full_simulation.pycan be dropped
from the salvage set and left inembeddedos-org's history, which stays readable
after archiving — the PR body already makes that point about the 29 commits.
Nothing here argues against archiving embeddedos-org. The evidence in the body that it
serves a 404 while still receiving commits is the strongest part of the PR, and steps 1
and 4 of #158 are, as the body says, the more urgent work.
Not checked
- I did not determine why the
testcheck is failing. It ran 7m24s and I could not
reproduce it here —pr-check.ymlneeds a Chromium download and a live server. Note
that it usesnpm install, notnpm ci, so finding 2 does not explain it directly;
butmasterhas nopackage-lock.jsonat all, and adding one changes what
npm installresolves, so this PR could be implicated. That connection is a
hypothesis, not a result. The failure needs its log read. - I did not run
tests/simulation/test_mobile_full_simulation.py. Finding 4 is a read of
its import block and constants; whether its 799 lines of assertions are correct is
unassessed. - I did not open
dist/index.htmlin a browser or run the Playwright suite against the
minified tree. Finding 1 rests onnode --check, which is sufficient for "does not
parse" and says nothing about the CSS or HTML output beyond findings 6 and 7. - I did not review the two ported workflows (
health-check.yml,lint-readme.yml) in
any depth beyond confirming they are new to this repo. Note thatlint-readme.ymlis
the same job that is red onembeddedos-org#7with 144 pre-existing violations on
that repo'smaster; whether it passes here is untested. ci.yml:27and:30carry2>/dev/null || trueon lint and typecheck, so those two
steps cannot fail. Pre-existing and not touched by this PR, but it is the same
swallowed-failure pattern I reported on eNI#30 in this run.- I did not verify the
gh apiandcurloutput quoted in the PR body showing the other
repo serving a 404. I took it as given; it is consistent with the repo having no Pages
configuration.
Automated architecture review of a6a538b7492a — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.
Step 2 of #158. Makes
embeddedos-orgsafe to archive.Why
Two near-identical website repositories, no common git ancestor. This one
serves the live site. The other serves a 404:
and it has kept receiving real work regardless — an SEO canonical fix, a
duplicate-element bug fix, a null-dereference guard, all as recently as
2026-08-30, applied to a site nobody can load.
What is ported
Six files that exist there and not here:
scripts/build-deploy.py.github/workflows/health-check.yml.github/workflows/lint-readme.yml.github/dependabot.ymltests/simulation/test_mobile_full_simulation.pypackage-lock.jsonPlus removal of
style.css.bak, an orphan the dead repo had already cleaned andthis one still carried.
build-deploy.py had to be fixed before it could be taken
It operated in place. It minified the tracked sources over themselves, then:
deleted the CI configuration and the test suite from the working tree. It is
named "build for the deploy branch" and never made a branch — it mutated whatever
checkout it was run in.
I found this the direct way: I ran it, and it took
.github/andtests/withit. Had it been merged as-is and run by anyone in a clone, the same thing would
have happened to them.
It now stages a copy into
dist/and minifies that. A build step must not beable to damage the thing it is building from.
Verified
dist/was already in.gitignore.After this merges
embeddedos-orgcan be archived — not deleted, so its 29 commits stay readable.That is step 3 of #158; steps 1 (get the production deploy into version control)
and 4 (set the CNAMEs) are independent and, in the case of step 1, more urgent
than either.