Skip to content

chore: salvage the unique work from embeddedos-org before archiving it - #159

Open
srpatcha wants to merge 1 commit into
masterfrom
salvage/port-unique-work-from-embeddedos-org
Open

chore: salvage the unique work from embeddedos-org before archiving it#159
srpatcha wants to merge 1 commit into
masterfrom
salvage/port-unique-work-from-embeddedos-org

Conversation

@srpatcha

@srpatcha srpatcha commented Sep 2, 2026

Copy link
Copy Markdown
Member

Step 2 of #158. Makes embeddedos-org safe to archive.

Why

Two near-identical website repositories, no common git ancestor. This one
serves the live site. The other serves a 404:

$ gh api repos/embeddedos-org/embeddedos-org/pages
status: 404   url: None   source: None

$ curl https://embeddedos-org.github.io/embeddedos-org/
HTTP 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:

file
scripts/build-deploy.py HTML/CSS/JS minification
.github/workflows/health-check.yml weekly uptime check
.github/workflows/lint-readme.yml README lint on push
.github/dependabot.yml dependency updates
tests/simulation/test_mobile_full_simulation.py mobile coverage
package-lock.json pinned dependency tree

Plus removal of style.css.bak, an orphan the dead repo had already cleaned and
this 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:

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 the direct way: I ran it, and it took .github/ and tests/ with
it. 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 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    correctly excluded
  .github  correctly excluded
  scripts  correctly excluded

index.html  96,362 -> 92,410 bytes (4.1% smaller)
  <title>          retained
  rel="canonical"  retained
  </html>          retained

32 files minified, 32,179 bytes saved

dist/ was already in .gitignore.

After this merges

embeddedos-org can 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.

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 srpatcha left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/ and scripts/ all survive the
    run, SOURCE_ONLY excludes them from dist/, and dist/ contains 97 files with none
    of those three present. The stage_tree() top-level-only pruning is correct — a nested
    content directory named scripts would survive, as the comment claims.
  • index.html 96,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.bak is a genuine orphan: git grep style.css.bak origin/master returns
    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:

  1. Drop minify_js from scripts/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.
  2. rm package-lock.json && npm install && git add package-lock.json, then confirm
    npm ci succeeds.
  3. Resolve finding 5 with #7 before either merges.
  4. Findings 3, 4, 6 and 7 can follow, or test_mobile_full_simulation.py can be dropped
    from the salvage set and left in embeddedos-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 test check is failing. It ran 7m24s and I could not
    reproduce it here — pr-check.yml needs a Chromium download and a live server. Note
    that it uses npm install, not npm ci, so finding 2 does not explain it directly;
    but master has no package-lock.json at all, and adding one changes what
    npm install resolves, 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.html in a browser or run the Playwright suite against the
    minified tree. Finding 1 rests on node --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 that lint-readme.yml is
    the same job that is red on embeddedos-org#7 with 144 pre-existing violations on
    that repo's master; whether it passes here is untested.
  • ci.yml:27 and :30 carry 2>/dev/null || true on 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 api and curl output 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.

Sign up for free to 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.

1 participant