Skip to content

Fixes #9 - #11464

Closed
fzoll wants to merge 4 commits into
pingdotgg:mainfrom
fzoll:agent/issue-9
Closed

Fixes #9#11464
fzoll wants to merge 4 commits into
pingdotgg:mainfrom
fzoll:agent/issue-9

Conversation

@fzoll

@fzoll fzoll commented Sep 12, 2026 •

Copy link
Copy Markdown

Summary

Adds a measured capabilities.browser field to the public /.well-known/t3/environment descriptor so cc_runner's capability-based dispatch can demote nodes whose declared config has drifted from reality, per #9.

  • ok: a Chromium binary was found (Playwright's downloaded browser cache is checked first, falling back to a well-known system install path) and it launched successfully — reports binaryPath + version.
  • degraded: a binary was found but either it failed to launch, or only a system Chromium exists while the Playwright browser cache is missing/incomplete — reports binaryPath + details.
  • absent: no Chromium binary could be found anywhere.

The probe (apps/server/src/environment/BrowserCapabilityProbe.ts) is lazy and cached for 6 hours via Effect.cachedWithTTL, so it never runs on every health-check poll. ExecutionEnvironmentCapabilities.browser is an additive optional field in packages/contracts/src/environment.ts; existing web/desktop clients tolerate it.

Detection is filesystem + process based only (directory scan of the Playwright cache, well-known system paths, then a --version spawn) — no playwright-core dependency was added to apps/server. Installation/telepítés-kezelés and cc_runner's dispatch/demote logic stay out of scope per the issue.

Decisions

  • Test tier: unit, focused on apps/server, with mocked FileSystem/ProcessRunner (matches the issue's ## Test note). First commit (c2550a5c) adds BrowserCapabilityProbe.test.ts alone and is red (imports a module that doesn't exist yet); the second commit adds the implementation and wiring.
  • "degraded" semantics: chose to treat both "binary found but failed to launch" and "only a system Chromium exists, no Playwright cache" as degraded, since either way the automation stack that actually needs to drive the browser (Playwright) can't be trusted yet — matches the issue's example (RPi has both apt chromium and the ms-playwright cache → ok).
  • Cache key/TTL: a single process-lifetime Effect.cachedWithTTL(..., "6 hours") per server instance, as specified.

Manually verified

scripts/agent-verify.sh does not exist in this repo, so per instructions I ran pnpm lint and the relevant test suites directly.

$ pnpm --filter @t3tools/contracts typecheck
$ tsgo --noEmit
(clean, no output)

$ cd apps/server && pnpm exec vp run typecheck
~/apps/server$ tsgo --noEmit ⊘ cache disabled
(clean, no output)

$ cd apps/server && pnpm exec vp test run src/environment/ src/server.test.ts
 Test Files  5 passed (5)
      Tests  132 passed (132)

$ pnpm lint
(only pre-existing warnings in apps/web, unrelated to this diff; exit 0)

Pre-existing, unrelated flake found while running the full pnpm test

Running pnpm test at the repo root (which fans out to every workspace package in parallel) reproduces one failing test twice in a row:

FAIL apps/mobile/src/features/diffs/nativeReviewDiffHighlighter.test.ts
  > highlightNativeReviewDiffVisibleRows > keeps grammar state across inline comment rows

I confirmed this is not caused by this PR:

  • git diff main -- apps/mobile/src/features/diffs/nativeReviewDiffHighlighter.ts apps/mobile/src/features/diffs/nativeReviewDiffHighlighter.test.ts is empty — neither file is touched by this branch.
  • Running apps/mobile's full test suite alone (cd apps/mobile && pnpm exec vp test run), twice, passes cleanly both times: 520 passed (520).
  • It only reproduces when pnpm test runs 4+ workspace packages' Vitest processes fully in parallel in this sandbox and fails identically both times I tried, which points at CPU-contention-sensitive timing inside the shared Shiki/JS-regex highlighter singleton under heavy load (the failing assertion shows coarser, less-tokenized output consistent with a tokenizer time budget being hit), not a logic bug in the diff.

I did not attempt a fix for it here since it's unrelated to #9's scope and I couldn't get a reliable, isolated repro to validate a fix against — flagging it explicitly rather than silently leaving it. Given the issue's own acceptance criteria (measured ok/degraded/absent browser capability, lazy 6h-cached probe, unit test coverage) are fully met and verified above, I'm titling this "Fixes #9"; happy to split the mobile flake into a separate follow-up issue if preferred.

Closes #9

Summary by CodeRabbit

  • New Features

    • Server environment information now includes detected Chromium availability and usability.
    • Server versions can display build identifiers for more precise troubleshooting.
    • Version checks distinguish release mismatches from build-level differences, reducing false warnings for remote environments.
    • Connection diagnostics now log version mismatches before establishing sessions.
  • Bug Fixes

    • Improved reliability when multiple Git operations target the same repository, preventing conflicting worktree and fetch activity.
    • Browser detection reports clear states when Chromium is unavailable, degraded, or fully operational.

fzoll and others added 4 commits August 12, 2026 02:00
* fix(deploy): stamp build SHA into serverVersion to catch web/server drift (#1)

Source-mode nodes (e.g. the RPi in #1) rebuild apps/web/dist only when
someone explicitly runs the web build, separately from `git pull`
restarting the server. Since serverVersion/APP_VERSION otherwise track
package.json's semver (which doesn't bump per commit on this fork), a
stale bundle can silently share a version string with a server that has
moved on to a schema-incompatible commit — the existing client/server
version-mismatch banner never fires, and the connection just dies with a
generic SchemaError.

- ServerBuildSha.resolveServerBuildSha resolves the server's own git HEAD
  SHA at runtime (via the existing ProcessRunner service) and
  ServerEnvironment suffixes it onto serverVersion when resolvable,
  falling back to the plain semver for packaged builds without a .git
  directory (desktop, HA add-on).
- vite.config.ts stamps APP_VERSION with the checkout SHA the build ran
  from, unless an explicit APP_VERSION is provided (release builds).
- platform.ts logs a warning (via the existing versionSkew comparison) as
  soon as the pre-flight environment descriptor is fetched over HTTP,
  before the WS RPC session is attempted — this is the earliest point a
  mismatch can be observed, since the WS handshake itself is what fails
  to decode when client/server drift.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* fix(web): scope SHA-granularity version-mismatch checks to the primary connection

The reviewer on #2 flagged a false-positive regression: stamping serverVersion
with +git.<sha> made resolveVersionMismatch compare SHAs across every
connected environment, not just the primary/local one. A packaged desktop
build (no .git, plain semver) or any remote peer on a different commit now
tripped the "version drift" banner even though both were perfectly healthy —
exactly the multi-environment path the original issue's failure mode was
reported on.

resolveVersionMismatch/resolveServerConfigVersionMismatch now compare at
release granularity by default (git build metadata stripped) and only opt
into exact SHA comparison via `{ compareBuildMetadata: true }` for call sites
that are provably the primary/same-origin connection:
- ConnectionsSettings.tsx's primary banner (opts in)
- ChatView.tsx's per-thread banner (opts in only when the active thread's
  environment is the primary one)
- connection/platform.ts's pre-flight descriptor check (opts in — both its
  callers bootstrap a same-machine connection built from this client's own
  checkout)

Saved/remote environment rows in ConnectionsSettings.tsx keep the new default
(no option passed), matching pre-#1 semver-only behavior.

Also addresses the review's two minor notes: dropped the redundant /i flag
on the already-lowercased SHA regex (server and vite.config.ts), and gated
vite.config.ts's git rev-parse spawn to `command === "build"` so dev/test
invocations no longer spawn it on every run.

---------

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
* fix(server): per-repo semaphore around git fetch + resolveRemoteTrackingCommit in worktree provisioning

Concurrent worktree-provisioning dispatches for the same repo could race:
a `git fetch` writing refs/remotes/* on one thread could interleave with
`git rev-parse --verify refs/remotes/...` on another, failing with
"fatal: Needed a single revision". Serialize fetch, resolveRemoteTrackingCommit,
and worktree add per repo (keyed by gitCommonDir) so the same clone's git-prep
never runs concurrently, while different repos keep provisioning in parallel.

Fixes #3

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* test(server): assert cross-repo git-lock concurrency, not just per-repo max=1

Review on #5 flagged that the lock test only asserted maxActiveByRepo===1
per repo, which would also pass under a regressed single global lock -
the "but not across repos" half of the test name was unverified. Track
global concurrent guarded-subprocess count and assert it reaches 2, so a
regression to one shared semaphore across repos fails the test.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Issue #9 wants a measured browser.state (ok/degraded/absent) in the
/.well-known/t3/environment response instead of trusting declared
config, since cc_runner's capability dispatch needs to detect drift.
This test is red until BrowserCapabilityProbe lands.
cc_runner's capability-based dispatch (fzoll/cc_runner) demotes nodes
whose declared config drifted from reality, so the public
/.well-known/t3/environment descriptor now reports a measured
capabilities.browser field instead of a config flag:

- "ok": a Chromium binary was found (Playwright's downloaded browser
  cache is checked first, falling back to a system install) and it
  launched successfully.
- "degraded": a binary was found but either it failed to launch, or
  only a system Chromium exists and the Playwright browser cache is
  missing/incomplete.
- "absent": no Chromium binary could be found anywhere.

The probe is lazy and cached for 6 hours (Effect.cachedWithTTL) so it
never runs on every health-check request. Detection is filesystem/
process-based only (no playwright-core dependency added to
apps/server); installation itself stays out of scope per the issue.
@fzoll

fzoll commented Sep 12, 2026

Copy link
Copy Markdown
Author

Opened against the wrong repo (upstream instead of the fork). Closing; correct PR opened against fzoll/t3code.

@github-actions github-actions Bot added the vouch:unvouched PR author is not yet trusted in the VOUCHED list. label Sep 12, 2026
@fzoll fzoll closed this Sep 12, 2026
@github-actions github-actions Bot added the size:L 100-499 changed lines (additions + deletions). label Sep 12, 2026
@coderabbitai

coderabbitai Bot commented Sep 12, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 51942644-08ee-4741-86b0-87566e2e5e63

📥 Commits

Reviewing files that changed from the base of the PR and between cfeaca4 and 14b8c38.

📒 Files selected for processing (14)
  • apps/server/src/environment/BrowserCapabilityProbe.test.ts
  • apps/server/src/environment/BrowserCapabilityProbe.ts
  • apps/server/src/environment/ServerBuildSha.test.ts
  • apps/server/src/environment/ServerBuildSha.ts
  • apps/server/src/environment/ServerEnvironment.ts
  • apps/server/src/vcs/GitVcsDriverCore.test.ts
  • apps/server/src/vcs/GitVcsDriverCore.ts
  • apps/web/src/components/ChatView.tsx
  • apps/web/src/components/settings/ConnectionsSettings.tsx
  • apps/web/src/connection/platform.ts
  • apps/web/src/versionSkew.test.ts
  • apps/web/src/versionSkew.ts
  • apps/web/vite.config.ts
  • packages/contracts/src/environment.ts

📝 Walkthrough

Walkthrough

The PR adds browser capability detection, Git-based build metadata, version-skew handling, and per-repository Git operation locks. It updates server descriptors, web connection checks, build configuration, contracts, and related tests.

Changes

Environment observability and version identity

Layer / File(s) Summary
Browser capability contract and probe
packages/contracts/src/environment.ts, apps/server/src/environment/BrowserCapabilityProbe.ts, apps/server/src/environment/BrowserCapabilityProbe.test.ts
The environment contract adds browser capability states and fields. The server probes Playwright and system Chromium binaries, reports launch results, and caches the result.
Git build version resolution and descriptor wiring
apps/server/src/environment/ServerBuildSha.ts, apps/server/src/environment/ServerBuildSha.test.ts, apps/server/src/environment/ServerEnvironment.ts, apps/web/vite.config.ts
The server and web build resolve a validated Git SHA and append its prefix to the application version. The server descriptor includes the browser capability result.
Build metadata comparison and connection reporting
apps/web/src/versionSkew.ts, apps/web/src/versionSkew.test.ts, apps/web/src/components/ChatView.tsx, apps/web/src/components/settings/ConnectionsSettings.tsx, apps/web/src/connection/platform.ts
Version comparison can include build metadata. Primary environments compare Git metadata, remote threads use release-level comparison, and connection loading logs mismatches.

Per-repository Git synchronization

Layer / File(s) Summary
Per-repository Git operation locking
apps/server/src/vcs/GitVcsDriverCore.ts, apps/server/src/vcs/GitVcsDriverCore.test.ts
Git worktree creation, fetching, and remote tracking resolution share a semaphore per repository. Tests verify same-repository serialization and concurrent operation across different repositories.

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Feature

Sequence Diagram(s)

sequenceDiagram
  participant WebBuild
  participant ServerEnvironment
  participant ServerBuildSha
  participant BrowserCapabilityProbe
  participant WebClient
  WebBuild->>ServerBuildSha: resolve git SHA during build
  ServerEnvironment->>ServerBuildSha: resolveServerBuildSha()
  ServerEnvironment->>BrowserCapabilityProbe: probe()
  BrowserCapabilityProbe-->>ServerEnvironment: BrowserCapability
  ServerEnvironment-->>WebClient: environment descriptor with serverVersion
  WebClient->>WebClient: compare release and build metadata
Loading

Suggested reviewers: juliusmarminge

✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

"/Applications/Chromium.app/Contents/MacOS/Chromium",
],
linux: [
"/usr/bin/chromium",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Medium environment/BrowserCapabilityProbe.ts:31

On Ubuntu hosts using the Chromium Snap, the probe reports state: "absent" and incorrectly demotes the node when no Playwright cache or legacy /usr/bin/chromium wrapper exists. Add /snap/bin/chromium to the Linux system-binary candidates so the working browser is probed.

Suggested change
"/usr/bin/chromium",
"/snap/bin/chromium",
"/usr/bin/chromium",
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/environment/BrowserCapabilityProbe.ts around line 31:

On Ubuntu hosts using the Chromium Snap, the probe reports `state: "absent"` and incorrectly demotes the node when no Playwright cache or legacy `/usr/bin/chromium` wrapper exists. Add `/snap/bin/chromium` to the Linux system-binary candidates so the working browser is probed.

path: Path.Path,
): string {
const overridePath = env.PLAYWRIGHT_BROWSERS_PATH;
if (overridePath && overridePath !== "0") {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Medium environment/BrowserCapabilityProbe.ts:48

When PLAYWRIGHT_BROWSERS_PATH is set to "0" and the hermetic Chromium is the only browser available, this probe returns absent (or incorrectly degraded when a system Chromium exists), causing capability-based dispatch to demote the node. The condition at line 48 discards Playwright's hermetic-install marker and scans only the OS cache; resolve node_modules/playwright-core/.local-browsers for this mode as well.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/environment/BrowserCapabilityProbe.ts around line 48:

When `PLAYWRIGHT_BROWSERS_PATH` is set to `"0"` and the hermetic Chromium is the only browser available, this probe returns `absent` (or incorrectly `degraded` when a system Chromium exists), causing capability-based dispatch to demote the node. The condition at line 48 discards Playwright's hermetic-install marker and scans only the OS cache; resolve `node_modules/playwright-core/.local-browsers` for this mode as well.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟠 High

yield* fetchRemoteForStatus(cacheKey.gitCommonDir, cacheKey.remoteName);

Status refreshes still mutate refs/remotes/* outside repoGitLocks, so a concurrent fetchRemoteForStatus can race with resolveRemoteTrackingCommit and reproduce the ref-read failure. Protect fetchRemoteForStatus with the same per-repository semaphore before running its fetch.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/vcs/GitVcsDriverCore.ts around line 991:

Status refreshes still mutate `refs/remotes/*` outside `repoGitLocks`, so a concurrent `fetchRemoteForStatus` can race with `resolveRemoteTrackingCommit` and reproduce the ref-read failure. Protect `fetchRemoteForStatus` with the same per-repository semaphore before running its `fetch`.

@macroscopeapp

macroscopeapp Bot commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — This PR introduces a production browser-capability probe used for automated node demotion, changes version and Git concurrency behavior, and adds a static-analysis suppression directive. Unresolved detection gaps and a remaining Git ref-race risk further warrant human validation.

Not approved because:

  • 3 blocking correctness issues found at or above your repo's Minimum Blocking Severity

Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L 100-499 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant