test(desktop): lint the POSIX-path-literal defect instead of hunting it - #183
Merged
Merged
Conversation
The same defect turned up three times across #177 and #180: a hard-coded absolute POSIX path meeting a path the code builds with path.join. On Windows path.join emits backslashes and path.resolve prepends a drive, so the two sides can never be equal and the assertion is dead on that platform — silently, because the file had only ever run on Linux. Two were found by the Windows lane going red; the third by reading a file before adding it. One at a time, after the fact, on a required check. The rule does NOT ban leading-slash literals. A blanket ban is unusable: the seven lane files hold 71 absolute POSIX literals, and all but the three above are inputs — a fake execPath, a ControlPath handed to ssh, a path the stub filesystem is asked about. Coping with those is the job of the code under test. The defect is where a literal meets a computed path, so the rule keys on that meeting: path.join onto a POSIX root (fix: wrap in path.resolve, a no-op on POSIX), a comparison against a POSIX literal, and a regex that opens with a path separator. "Opens with" rather than "contains" keeps the shebang check and the remote-shell-command assertion — both legitimately POSIX, both in lane files today — quiet. Scoped to the suites in test:desktop:win-install, and that list is parsed from package.json rather than restated in the config. A second copy fails silently: a suite added to the lane but missing from the config gets no cross-platform linting at exactly the moment it starts needing it. Verified by observation, not assertion. Run over the pre-fix files (1770376^, 50e2b2b^) the rule reports exactly the three known defects and nothing else, and none of them on the current files. The self-maintenance claim was proved the same way: a probe file with a known defect drew no report until its name alone was added to the lane script — no config edit — after which it did. Two false positives are kept and annotated rather than engineered away. windows-hermes-path compares against /root/venv/Scripts/python.exe twice, and both are safe because every path helper that resolver touches is injected by makeDeps and joins with '/'. The disables carry that reason, which is the rule earning its keep — "why is this POSIX literal safe on Windows?" is what the next person adding a suite needs answered. The rule's own tests caught two bugs in it first: expect(x).toBe(v) puts the expected value at argument 0, not 1, so expect-style assertions were never checked; and the lane parser consumed the token after every flag, so a boolean -u swallowed the suite name behind it. It now consumes values only for known value-taking flags, erring towards a glob that matches nothing rather than silently dropping a suite. Separately, the first disable comments did not work at all — eslint-disable-next-line followed by more comment lines applies to the next comment. Found by running lint, not by reading it. lint now covers eslint-rules/ and scripts/*.test.mjs, because three lane suites live in scripts/ and were outside `eslint src/ electron/`. The rest of scripts/ stays unlinted; it carries 10 pre-existing errors that are not this change. eslint becomes a declared root devDependency — it was only ever an auto-installed peer, and the rule's tests import RuleTester from it. The lockfile delta is mechanical peer-marker removal with no version movement. check:lint 0 errors (89 warnings, unchanged), test:desktop:platforms 976 passed, lane script 117 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012689txgT12g2hjRczcUZi8
dizhaky
marked this pull request as ready for review
August 11, 2026 22:23
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
9 tasks
github-actions Bot
pushed a commit
that referenced
this pull request
Aug 11, 2026
Every workspace declares react and react-dom at exactly 19.2.7, but the root
node_modules held react 19.2.8 against react-dom 19.2.7. Dozens of packages ask
for react as a peer at ^19, npm hoisted that to the root, and the four
workspaces kept nested 19.2.7 copies. react-dom then resolved from the root and
found the root react, so every renderer test died at collection with:
Incompatible React versions: The "react" and "react-dom" packages must have
the exact same version. Instead got:
- react: 19.2.8
- react-dom: 19.2.7
This is not theoretical. On #183's run, JS & TS checks / web / check failed with
exactly that error (4 suites at collection), and apps/desktop / check:test:ui
failed the same way — the desktop case reproduced here on a pristine main
checkout with a clean npm ci, changes stashed.
Fixed with a root override, the instrument this repo already uses for exactly
this class of problem (lodash, undici, brace-expansion). The lockfile result is
one react in the tree instead of five: the four nested 19.2.7 copies collapse
and the root drops to 19.2.7.
Verified: apps/desktop --project ui now runs 396 files / 3489 tests, where
before every file failed at collection. web check 26 files / 165 tests, ui-tui
check 138 files / 1530 tests, both 0 lint errors. apps/desktop check:lint 0
errors and test:desktop:platforms 976 passed, unchanged — the override does not
disturb the electron side.
Not fixed here: ui-tui / check also fails on CI, but for an unrelated reason —
packages/hermes-ink/src/ink/ink-resize.test.ts asserts on terminal geometry and
reports "expected 128 to be less than -1" on the runner while passing locally.
That looks environment-dependent and needs its own diagnosis.
Claude-Session: https://claude.ai/code/session_012689txgT12g2hjRczcUZi8
Co-authored-by: Claude <noreply@anthropic.com>
9 tasks
github-actions Bot
pushed a commit
that referenced
this pull request
Aug 12, 2026
ink-resize.test.ts failed once on CI (on #183's run, in two of its three tests) with: AssertionError: expected 128 to be less than -1 meaning the erase landed at byte 128 and "hello" was never repainted. It is intermittent, not a standing red — it passed on #184's run along with every other JS lane. I could not reproduce it: 12 isolated runs, 8 under saturating CPU contention, 3 runs of the whole hermes-ink package, and the full ui-tui suite are all green. Two mechanisms were probed and ruled out locally. The repaint is not racing a flush — measured after onRender(), after one microtask, and after a 50ms macrotask, "hello" sits at index 39 in all three, so the `await tick()` is not load-bearing. And no deferred write from the initial render lands after the test clears its buffer, which would have explained the erase moving from byte 32 locally to 128 on CI: across five runs, zero bytes arrive after the clear. So the mechanism is unknown, and the remaining hypothesis — the erase landing in a frame whose repaint went down a diff path with nothing to write — is exactly the drift bug these tests exist to catch. That makes it possibly a real intermittent renderer race rather than a bad test. The assertion is therefore NOT loosened. Loosening it is the obvious way to stop a flake and the wrong move here: "erase written, content never repainted" is the precise condition being guarded, so a weaker check could hide a live bug. What changes is the diagnostics. The three duplicated assertions now go through expectErasedThenRepainted(), which asserts the same invariant in two steps — the text was repainted at all, then that it came after the erase — and carries the frame bytes into the failure message. Verified by negative control: fed the helper the exact CI failure shape and read the message it produces rather than assuming it renders. ui-tui check: 138 files / 1530 tests, 0 lint errors. Claude-Session: https://claude.ai/code/session_012689txgT12g2hjRczcUZi8 Co-authored-by: Claude <noreply@anthropic.com>
9 tasks
dizhaky
pushed a commit
that referenced
this pull request
Aug 12, 2026
auto-merge-prs.yml triggered on pull_request [opened, ready_for_review] and called `gh pr merge --auto` immediately. `--auto` does not wait for CI — it waits for whatever branch protection requires, and this repo has no required status checks configured, so "wait for the required checks" resolved to "wait for nothing" and PRs merged seconds after being opened. Observed, not theorised: #183 merged with `All required checks pass` RED, while web/check and apps/desktop/check:test:ui were both failing on the React version mismatch. #185 merged 11 seconds after it was created, before CI finished. The repo had already half-learned this one level down — ci.yml's all-checks-pass job carries a comment that desktop-install-windows "merged red three times before being listed here, because nothing was waiting on it." Nothing was waiting on the aggregator either. The trigger is now the completion of the CI workflow, and the decision is read from the `All required checks pass` check run on the head commit. Keyed on that check run rather than workflow_run.conclusion, and the difference is load-bearing: CI as a whole can conclude failure because of a lane nobody requires — the Docker build is explicitly outside the gate's needs list — so gating on the overall conclusion would block merges on lanes the repo has already decided are non-blocking. The aggregator is the contract, and it already counts `skipped` as passing, so Python-only PRs aren't held up by frontend lanes. One re-run, then stop. A red gate blocks the merge; if the run hasn't been retried, its failed jobs re-run once, which fires a fresh workflow_run completion that re-enters at attempt 2 where no further retry is offered. A genuinely broken PR fails twice and stays blocked. This is what keeps the known ui-tui/check flake (ink-resize.test.ts) from wedging a good PR permanently. Fork PRs never reach the merge path, since a workflow_run job holds a write token in the base repo's context. Drafts are skipped, a `do-not-merge` label is an escape hatch, and a PR whose head moved since CI ran is left to the newer run. Actions can't be executed here, so the step's shell was extracted and run against a mock gh that logs every merge/rerun requested. Nine cases: success merges; failure/cancelled at attempt 1 re-runs without merging; failure at attempt 2 neither re-runs nor merges; a missing gate, a draft, a do-not-merge label, a moved head and an already-merged PR all decline. The only path that merges is success. Separately verified that the jq name filter picks the aggregator out of a noisy check-run list, and that a renamed gate yields an empty conclusion — it fails closed, not open. This is not branch protection: it stops this automation from merging red, not a human merging by hand. Requiring the aggregator in the branch rules is still the real fence and composes with this. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012689txgT12g2hjRczcUZi8
dizhaky
pushed a commit
that referenced
this pull request
Aug 12, 2026
Owner's explicit instruction, once the gate in the previous commit made the aggregator actually block merges. The consequence, stated rather than left to be discovered: a PR that touches CI-sensitive files (workflows, actions, eslint config), changes the MCP catalog, or trips a critical supply-chain finding can now merge with no human having looked at it. That gate asks for the `ci-reviewed` label, and a label is by definition something a person adds, so requiring it would mean every such PR waits for a human — the opposite of what this repo's automation is for. To be precise about what changed: review-labels never blocked anything before this PR either, because nothing was waiting on the aggregator at all — #183 merged with the label gate red. So this is a change in intent, not in effective behaviour. The previous commit would have started enforcing it for the first time; that enforcement is declined up front rather than discovered as friction later. The job still runs and still reports red on the PR, so the signal is intact; it just doesn't block. Restoring it is one line. ci.yml parses, the review-labels job is still defined, and comment-live still lists it in needs — the review comment reads its status, so dropping it from the gate must not drop it from the comment. The gate's evaluate step iterates toJSON(needs) generically, so no other edit is required. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012689txgT12g2hjRczcUZi8
dizhaky
added a commit
that referenced
this pull request
Aug 12, 2026
auto-merge-prs.yml triggered on `pull_request: [opened, ready_for_review]` and called `gh pr merge --auto` immediately. `--auto` does not wait for CI — it waits for whatever *branch protection* requires, and this repo has no required status checks, so "wait for the required checks" resolved to "wait for nothing". Observed, not theorised: #183 merged with the aggregate gate RED, and #185 merged 11 seconds after it was created. The trigger is now the completion of the CI workflow, and the merge decision is read from the `All required checks pass` check run on the head commit. Keyed on that check run rather than workflow_run.conclusion, because CI as a whole can conclude failure over a lane nobody requires (the Docker build is deliberately outside the gate's needs) — the aggregator is the contract, and it already counts skipped as passing. A red gate blocks the merge and re-runs the failed jobs once; the fresh workflow_run re-enters at attempt 2 where no further retry is offered, so a genuinely broken PR fails twice and stays blocked. Fork PRs never reach the merge path, drafts are skipped, `do-not-merge` is an escape hatch, and a PR whose head moved since CI ran is left for the newer run. Verified by extracting the step's shell and running it against a mock gh across nine cases: the only path that merges is `success`, and a renamed gate fails closed. Also: review-labels dropped from the aggregator's needs (owner's instruction) — it still runs and still reports, it just no longer blocks. And the gitleaks "What to do on a hit" advice is now scoped to the scan step, so a setup 503 stops printing "gitleaks flagged a secret in this diff" over an infrastructure blip. This is not branch protection: it stops this automation merging red, not a human merging by hand. Requiring `All required checks pass` in the branch rules is the real fence and composes with it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.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
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.
What does this PR do?
The same defect turned up three times in three files across #177 and #180: a hard-coded absolute POSIX path meeting a path the code builds with
path.join. On Windowspath.joinemits backslashes andpath.resolveprepends a drive letter, so the two sides can never be equal — the assertion is dead on that platform, silently, because the file had only ever run on Linux.update-relaunch.test.tsconst ROOT = '/home/u/.hermes/hermes-agent'windows-hermes-path.test.tsp => p === '/venv/lib/python3.12/site-packages'ssh-connection.test.tsassert.match(a, /\/[0-9a-f]{16}\.sock$/)Two were found by the Windows lane going red; the third by reading a file before adding it. One at a time, after the fact, on a required check. This makes it a lint error instead.
It does not ban leading-slash literals. A blanket ban is unusable: the seven lane files hold 71 absolute POSIX string literals, and all but the three above are inputs — a fake
execPath, a ControlPath handed to ssh, a path the stub filesystem is asked about. Coping with those is the job of the code under test. The defect is where a literal meets a computed path, so the rule keys on that meeting:path.join(X, …)where X is, or is aconstbound to, an absolute POSIX literal. Fix:path.resolve('/…'), a no-op on POSIX.x === '/a/b', plus the expected slot of an assertion, which is the same comparison through a helper./^#!\/bin\/bash/(a shebang) and/^cd '\/home\/me\/project'…/(a remote shell command, and remotes are always POSIX) quiet — both live in lane files today.Single-segment roots (
'/tmp') are ignored, andpath.posix.join/path.win32.joinare exempt because a stated flavour makes the literal deliberate.Scope is self-maintaining. The rule applies only to the suites named in
test:desktop:win-install, and that list is parsed from package.json rather than restated in the ESLint config. A second copy fails silently: a suite added to the lane but missing from the config would get no cross-platform linting at exactly the moment it starts needing it. Files that only ever run on Linux keep their POSIX literals — this is a portability rule, not a style rule.Related Issue
No issue — this follows directly from the defect class found in #177 and #180.
Type of Change
Changes Made
apps/desktop/eslint-rules/no-posix-path-literals.mjs— the rule (+.test.mjs, 26 cases).apps/desktop/eslint-rules/windows-lane.mjs— parses the lane suite list out of thetest:desktop:win-installscript (+.test.mjs, 5 cases).apps/desktop/eslint.config.mjs— registers it ashermes/no-posix-path-literals,filesderived from the lane list.apps/desktop/electron/windows-hermes-path.test.ts— two annotated disables (see below).apps/desktop/package.json—lintnow also coverseslint-rules/andscripts/*.test.mjs; three lane suites live inscripts/and were outsideeslint src/ electron/. The rest ofscripts/stays unlinted — it carries 10 pre-existing errors that are not this change.apps/desktop/vitest.config.ts—eslint-rules/**.test.mjsjoins theelectronproject.package.json/package-lock.json—eslintbecomes a declared root devDependency. It was only ever an auto-installed peer, and the rule's testsimport { RuleTester } from 'eslint'— a source import, not a CLI invocation. The lock delta is mechanical"peer": trueremoval with no version movement..github/workflows/desktop-install-windows.yml,docs/system-log/2026-08-11.md— docs.How to Test
npm ci && npm run --prefix apps/desktop check:lint— 0 errors.npm run --prefix apps/desktop test:desktop:platforms— 976 passed, including the 31 new rule tests.apps/desktop/electron/update-relaunch.test.tsat1770376^(orssh-connection.test.tsat50e2b2b^) and re-run lint — the pre-fix defects are reported.Checklist
Code
pytest tests/ -q— N/A, no Python touchedDocumentation & Housekeeping
docs/system-log/2026-08-11.md, workflow header)cli-config.yaml.exampleCONTRIBUTING.md/AGENTS.mdScreenshots / Logs
Positive control — the rule over the pre-fix files reports exactly the three known defects and nothing else:
Self-maintenance, proved rather than claimed. A probe file with a known defect drew no report; adding only its name to
test:desktop:win-install— no config edit — made eslint reportjoinedRoot. Probe removed.Two false positives, kept and annotated.
windows-hermes-path.test.tscompares against/root/venv/Scripts/python.exetwice, and both are safe: every path helper that resolver touches is injected bymakeDepsand joins with/. They now carryeslint-disable-next-linewith that reason — which is the rule earning its keep, since "why is this POSIX literal safe on Windows?" is exactly what the next person adding a suite needs answered.The rule's own tests caught two bugs in it before it ran anywhere:
expect(x).toBe(v)puts the expected value at argument 0, not 1, so expect-style assertions were never checked; and the lane parser consumed the token after every flag, so a boolean-uswallowed the suite name behind it. It now consumes values only for known value-taking flags — erring towards an extra glob that matches nothing rather than silently dropping a suite. Separately, the first disable comments did not work at all:eslint-disable-next-linefollowed by more comment lines applies to the next comment. Found by running lint, not by reading it.Unrelated finding, not fixed here
vitest --project uifails at collection on every file withIncompatible React versions: react 19.2.8 / react-dom 19.2.7. Confirmed pre-existing: I stashed all of my changes, rannpm cifrom main's lockfile on a pristine tree, and reproduced the identical failure. Rootreactis pinned at19.2.8inpackage-lock.jsonwhilereact-domis19.2.7, though both workspaces declare19.2.7. I can't date it — this clone is shallow, andgit log -Sattributes the line to the graft boundary rather than a real commit. Left alone deliberately: a dependency-resolution fix does not belong in a lint-rule PR.Generated by Claude Code