Ghostex hardening pass: security remediation, macOS issue fixes, and five additive hardening features - #62
Ghostex hardening pass: security remediation, macOS issue fixes, and five additive hardening features#62pt-act wants to merge 21 commits into
Conversation
Address two verified backend security-audit findings in gxserver-rs: - Repository clone (High): replace broad ambient-environment inheritance with an explicit allowlist so env_clear() is meaningful. The clone subprocess now receives only Git-essential variables (PATH, HOME, SSH agent, locale/LC_*, proxy, temp, Windows essentials) and drops tokens, GIT_*/GIT_SSH_COMMAND overrides, LD_PRELOAD, etc. Adds tests covering the allowlist and ambient-secret non-leakage. - T3 runtime (Medium): bind the managed runtime to 127.0.0.1 instead of 0.0.0.0. It is only ever reached over loopback, so this removes off-host/container reachability with no behavior change.
Skills, hooks, and Desktop Control installed under ~/.agents/... (the path gxserver-rs agent_skills.rs writes to), but the native status checker and the bundled-skills uninstaller read ~/agents/... (no dot). Nothing ever created the no-dot directory, so every skill/Desktop Control badge stayed 'Not installed' after a successful install and Uninstall Skills silently removed nothing. Point the status checks, the uninstaller, and the shared-hooks catalog root (plus its profile attribution) at the canonical dotted ~/.agents location. The agents-hub catalog keeps its existing legacy no-dot skills group for backward compatibility. Fixes maddada#58
Remote connect runs SSH non-interactively from the GUI helper, so failures were surfaced as a generic 'SSH authentication failed'. Map stderr to specific, actionable causes: host-key changes, host-key verification, permission-denied (guides users connecting to another local account to save that machine's SSH password or add an accepted key), connection refused (Remote Login disabled), name-resolution, unreachable network, and timeout. String-only change to sanitizedProcessFailure; no auth-policy or control-flow change. The key-only BatchMode fast-fail behavior is intentional and left as-is pending a maintainer decision (see investigation notes). Refs maddada#61
…addada#38 design) Add a contributions/ folder documenting our work on this tree: - CHANGELOG.md (our contributions only; separate from root CHANGELOG.md) - security-audit-verification-and-remediation.md - issue-58 covered in changelog; issue-61 SSH investigation - issue-52 libxev kqueue upstream bug report - issue-38 menu-bar agent design decisions
Implement system health checker (Doctor) and diagnostic bundle exporter: Backend (gxserver-rs): - POST /api/doctor/run — five health checks (skills, hooks, toolchain, daemon, T3) - POST /api/doctor/fix — validated fix application with confirmation tokens - POST /api/doctor/exportDiagnostics — structured diagnostics bundle - DoctorCheck/DoctorFix structs with serde camelCase serialization - validate_check_invariants enforces Ok checks have no fix - CLI subcommand with --json support - 480 Rust tests passing Frontend (sidebar): - SupportSettingsTab in Settings modal (Support page, IconStethoscope) - Run Doctor button with loading state - Copy Diagnostics button (clipboard-first, native fallback) - Check results with status icons (ok/warn/fail) - Two-step fix confirmation flow - Type guards and state management in modal-host - RPC handlers in native-sidebar bridge - 4 TypeScript conformance tests passing Other: - Agent paths codegen (generate-agent-paths.mjs + generated TS) - Subprocess policy module for clone environment hardening - Spec document in contributions/
…ghostex-hardening Replaced implementation-detail-heavy doc with a feature-focused summary that references the canonical spec at spec/ghostex-hardening/ without exposing internal frameworks or workflow details.
Five features from spec/ghostex-hardening: doctor, shared paths, CI conformance, diagnostic bundle, subprocess policy.
Resolves 5 Dependabot alerts in tui2/website: - GHSA-j687-52p2-xcff (XSS in define:vars) - GHSA-xr5h-phrj-8vxv (server island replay) - GHSA-8hv8-536x-4wqp (reflected XSS slot name) - GHSA-jrpj-wcv7-9fh9 (XSS spread props) - GHSA-2pvr-wf23-7pc7 (host header SSRF) astro <=7.0.0-beta.6 is vulnerable; 7.0.9 fixes all five. starlight 0.41.3 requires astro ^7.0.2.
- maddada#33 (High): manage.tsx formatMarkdownQuote now escapes backslashes before double quotes, preventing backslash injection - maddada#37/maddada#35 (High/Med): session-card-content.tsx validates faviconDataUrl starts with 'data:image/' before setting img src - maddada#36/maddada#34 (High/Med): images.ts validates URL protocol (http/https/data:image) before assigning to img.src, blocking javascript: URLs Skipped: vendored code (simdutf, stb, glib), test-only regex, dev script spawnSync. Documented in codeql.txt.
…es HOOKS_ROOT
- subprocess_policy.rs: allowlisted keys (SSH_AUTH_SOCK, SSH_AGENT_PID)
now bypass the SENSITIVE_KEY_PATTERNS filter. Two-pass filter preserved
so only allowlisted+LC_* keys enter the second gate; allowlisted keys
then skip the sensitive check. Fixes SSH clone regression (Finding 1).
- native-sidebar.tsx: replace hardcoded p(".agents","hooks") with
p(HOOKS_ROOT) to match canonical .ghostex/hooks path (Finding 2).
- Add regression tests: subprocess_environment(Clone|Ssh) must yield
SSH_AUTH_SOCK when set in parent environment.
- Widen sensitive_vars_absent_from_all_profiles to exclude allowlisted
keys from the sensitivity check (they are intentionally preserved).
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ChangesGhostex hardening features
Security audit, issue investigations, and ancillary updates
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant SupportSettings
participant SidebarBridge
participant gxserver
participant doctor
SupportSettings->>SidebarBridge: runDoctor
SidebarBridge->>gxserver: /api/doctor/run
gxserver->>doctor: run_all_checks
doctor-->>gxserver: DoctorCheck list
gxserver-->>SidebarBridge: checks result
SidebarBridge-->>SupportSettings: display results
SupportSettings->>SidebarBridge: exportDiagnostics
SidebarBridge->>gxserver: /api/doctor/exportDiagnostics
gxserver-->>SidebarBridge: diagnostics bundle
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
native/sidebar/meo/helpers/images.ts (1)
251-257: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSupport uppercase schemes and render a fallback for rejected URLs.
The current check is case-sensitive, meaning valid image URLs like
HTTP://...will be rejected. Additionally, when a URL is rejected,img.srcis skipped but the widget never callsfail(), resulting in a blank space instead of the proper text fallback ().Convert the scheme check to be case-insensitive, and call
fail()when the URL is rejected.🐛 Proposed fix
- if (src.startsWith("http:") || src.startsWith("https:") || src.startsWith("data:image/")) { - img.src = src; - } - if (img.complete) { - hideLoadingPlaceholder(); - } + const lowerSrc = src.toLowerCase(); + if (lowerSrc.startsWith("http:") || lowerSrc.startsWith("https:") || lowerSrc.startsWith("data:image/")) { + img.src = src; + if (img.complete) { + hideLoadingPlaceholder(); + } + } else { + fail(); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@native/sidebar/meo/helpers/images.ts` around lines 251 - 257, Update the image source validation in the visible image-loading callback to check HTTP, HTTPS, and data-image schemes case-insensitively, while preserving accepted URLs for img.src. When validation rejects src, call the existing fail() handler so the text fallback renders instead of leaving a blank widget.gxserver-rs/src/protocol.rs (1)
169-173: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winAdd missing
/api/t3Runtime/*paths toGxserverEndpointPath.The
t3Runtimepaths (/api/t3Runtime/status,start,stop,panes) were added to theEXPECTED_ENDPOINTSarray inshared/gxserver-protocol-conformance.test.ts, which usessatisfies readonly GxserverEndpointPath[]. Because these paths are missing from theGxserverEndpointPathunion, this will cause a TypeScript compilation error.🐛 Proposed fix
| "/api/installTool" | "/api/browseFilesystem" | "/api/destructiveAdminAction" + | "/api/t3Runtime/status" + | "/api/t3Runtime/start" + | "/api/t3Runtime/stop" + | "/api/t3Runtime/panes" | "/api/capabilities" | "/api/doctor/exportDiagnostics" | "/api/doctor/fix" | "/api/doctor/run";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gxserver-rs/src/protocol.rs` around lines 169 - 173, Extend the GxserverEndpointPath union with the four missing t3Runtime endpoints: /api/t3Runtime/status, /api/t3Runtime/start, /api/t3Runtime/stop, and /api/t3Runtime/panes. Preserve all existing endpoint variants and ensure EXPECTED_ENDPOINTS satisfies the updated type.
🧹 Nitpick comments (4)
scripts/generate-agent-paths.mjs (1)
49-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify
snakeToConstantlogic.Splitting the string by
_and uppercasing each part produces the exact same result as calling.toUpperCase()on the entire string, since underscores are unchanged by case transformations.♻️ Proposed refactor
function snakeToConstant(value) { - return value - .split("_") - .map((part) => part.toUpperCase()) - .join("_"); + return value.toUpperCase(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/generate-agent-paths.mjs` around lines 49 - 54, Streamline the snakeToConstant function by removing the split/map/join pipeline and returning the input value transformed with a single toUpperCase() call. Preserve the existing output for underscore-separated strings.gxserver-rs/src/paths.rs (1)
170-170: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant string replacement.
The
replace("_ROOT", "_ROOT")operation has no effect and can be safely removed.♻️ Proposed refactor
- let ts_constant_name = name.to_ascii_uppercase().replace("_ROOT", "_ROOT"); + let ts_constant_name = name.to_ascii_uppercase();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gxserver-rs/src/paths.rs` at line 170, In the code producing ts_constant_name, remove the no-op replace("_ROOT", "_ROOT") call and retain the uppercase conversion via name.to_ascii_uppercase().shared/gxserver-protocol-conformance.test.ts (1)
123-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
satisfiesdoes not enforce exhaustiveness.The inline comment claims that the
satisfiesoperator enforces that the array covers the union type at compile time. However,satisfies readonly GxserverEndpointPath[]only ensures that the array elements are a valid subset of the union (preventing extraneous entries), not that they are exhaustive. The runtime check for> 80also does not catch specific subtraction drift.To actually enforce exhaustiveness statically and prevent drift, consider mapping the union to a
Recordwhere TypeScript requires every key to be present.🛠️ Recommended refactor for compile-time exhaustiveness
const EXPECTED_ENDPOINTS_RECORD: Record<GxserverEndpointPath, true> = { "/api/health": true, "/api/health/server": true, // ... map all paths to true }; const EXPECTED_ENDPOINTS = Object.keys(EXPECTED_ENDPOINTS_RECORD) as GxserverEndpointPath[];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shared/gxserver-protocol-conformance.test.ts` around lines 123 - 131, Replace the subset-only EXPECTED_ENDPOINTS satisfies check with an exhaustive Record<GxserverEndpointPath, true> keyed by every endpoint variant, then derive EXPECTED_ENDPOINTS from that record. Update the test comment and runtime assertion to reflect the record-based compile-time coverage, preserving the existing endpoint list and drift check.native/sidebar/native-sidebar.tsx (1)
22292-22359: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDoctor/Diagnostics feature is missing the required CDXC documentation comments. As per coding guidelines,
**/*.{ts,tsx,js,jsx,zig}changes must "Add CDX_LOG comments describing the date of the change (format: yyyy-MM-dd-hh:mm) and the requirements... Prefix all comments with CDXC:Area-of-product" and "Ensure ALL important user-facing requirements sent by the user are written as comments somewhere in the codebase." None of the new Doctor/Diagnostics code across these three files carries such a comment, unlike every other feature touched in this same PR (e.g.CDXC:AppIconPicker,CDXC:DebuggingSettings).
native/sidebar/native-sidebar.tsx#L22292-L22359: add aCDXC:Doctor <date>comment aboverunDoctorChecks/applyDoctorFix/exportDiagnosticsJsondescribing the doctor/fix/export requirement.native/sidebar/modal-host.tsx#L698-L2728: add a matchingCDXC:Doctor <date>comment near the newdoctorChecks/doctorLoading/diagnosticsJson/diagnosticsLoadingstate and thedoctorChecksResult/doctorFixResult/diagnosticsExportResulthandling.sidebar/settings-modal.tsx#L11185-L11326: add aCDXC:Doctor <date>comment aboveSupportSettingsTabdescribing the Support tab's diagnostics requirement.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@native/sidebar/native-sidebar.tsx` around lines 22292 - 22359, Add required CDXC:Doctor documentation comments with the change date in yyyy-MM-dd-hh:mm format and the relevant user-facing doctor, fix, and diagnostics-export requirements: above runDoctorChecks/applyDoctorFix/exportDiagnosticsJson in native/sidebar/native-sidebar.tsx (lines 22292-22359), near the doctorChecks/doctorLoading/diagnosticsJson/diagnosticsLoading state and result handling in native/sidebar/modal-host.tsx (lines 698-2728), and above SupportSettingsTab in sidebar/settings-modal.tsx (lines 11185-11326).Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@gxserver-rs/src/doctor.rs`:
- Around line 167-195: Update check_daemon to preserve the fetched
ServerHealthResponse.t3_runtime data and expose it to run_doctor_cli. In
run_doctor_cli, derive T3RuntimeStatusPayload from that returned t3_runtime when
available, falling back to the existing non-running placeholder only when daemon
health is unavailable; keep the current fallback behavior for fetch failures or
an absent health response.
In `@gxserver-rs/src/protocol.rs`:
- Around line 663-700: Update
every_endpoint_for_path_is_implemented_or_known_not_implemented to also verify
the reverse subset: each path in known (or separately in
ALL_IMPLEMENTED_ENDPOINTS and known_not_implemented_endpoints()) must exist in
all_ts_endpoint_paths. Preserve the existing forward checks and ensure
extraneous implemented or known-not-implemented paths cause the test to fail.
In `@gxserver-rs/src/server.rs`:
- Around line 1521-1580: Update the fix validation match in
handle_doctor_fix_http to accept the toolchain.install and install-tools
ID/token pair emitted by check_toolchain, and invoke the existing toolchain
installation routine using state.paths, returning applied: true on success.
Preserve the current skills.reinstall and hooks.reinstall behavior; centralize
identifiers only if an existing shared registry is available.
In `@gxserver-rs/src/subprocess_policy.rs`:
- Around line 181-188: Update the Unix file creation block in the subprocess
policy path to use OpenOptions with OpenOptionsExt, setting
create/write/truncate behavior and mode 0o600 atomically. Remove the separate
set_permissions call while preserving the existing write_all, sync_all, and
error propagation flow.
In `@gxserver-rs/src/t3_runtime.rs`:
- Around line 20-25: Update the macOS wrapper’s listenHost configuration in
NativeT3CodePaneReproLog.swift from 0.0.0.0 to 127.0.0.1, matching the
T3_RUNTIME_LISTEN_HOST loopback binding and keeping native-wrapper launches
inaccessible from external interfaces.
In `@native/sidebar/modal-host.tsx`:
- Around line 2566-2575: Update the successful fix handling in the
isDoctorFixResultMessage branch to dispatch the runDoctor message through the
existing vscode.postMessage SidebarToExtensionMessage channel instead of
postAppModalHostMessage. Preserve setting doctorLoading to true and ensure
handleSidebarMessage receives the message so runDoctorChecks() executes and
clears the loading state.
In `@native/sidebar/native-sidebar.tsx`:
- Around line 22598-22611: The generated Node scripts passed to
runNativeNodeScript reference SKILLS_ROOT and HOOKS_ROOT as undeclared bare
identifiers, causing runtime failures. Update the relevant String.raw script
blocks to interpolate these constants as ${SKILLS_ROOT} and ${HOOKS_ROOT},
including the skill status and catalog path checks, while preserving the
existing path construction.
- Around line 39594-39599: Update the generated Agents Hub catalog script’s
profilesFor() logic and hooksRoot reference to interpolate the HOOKS_ROOT value
rather than treating HOOKS_ROOT as a runtime variable. Ensure both generated
references use ${HOOKS_ROOT}, preventing a ReferenceError during catalog
generation while preserving the existing path checks and linkedProfiles
behavior.
In `@shared/gxserver-protocol.ts`:
- Around line 169-173: Extend the GxserverEndpointPath union with the four
t3Runtime endpoints: /api/t3Runtime/status, /api/t3Runtime/start,
/api/t3Runtime/stop, and /api/t3Runtime/panes, so EXPECTED_ENDPOINTS satisfies
the declared type.
---
Outside diff comments:
In `@gxserver-rs/src/protocol.rs`:
- Around line 169-173: Extend the GxserverEndpointPath union with the four
missing t3Runtime endpoints: /api/t3Runtime/status, /api/t3Runtime/start,
/api/t3Runtime/stop, and /api/t3Runtime/panes. Preserve all existing endpoint
variants and ensure EXPECTED_ENDPOINTS satisfies the updated type.
In `@native/sidebar/meo/helpers/images.ts`:
- Around line 251-257: Update the image source validation in the visible
image-loading callback to check HTTP, HTTPS, and data-image schemes
case-insensitively, while preserving accepted URLs for img.src. When validation
rejects src, call the existing fail() handler so the text fallback renders
instead of leaving a blank widget.
---
Nitpick comments:
In `@gxserver-rs/src/paths.rs`:
- Line 170: In the code producing ts_constant_name, remove the no-op
replace("_ROOT", "_ROOT") call and retain the uppercase conversion via
name.to_ascii_uppercase().
In `@native/sidebar/native-sidebar.tsx`:
- Around line 22292-22359: Add required CDXC:Doctor documentation comments with
the change date in yyyy-MM-dd-hh:mm format and the relevant user-facing doctor,
fix, and diagnostics-export requirements: above
runDoctorChecks/applyDoctorFix/exportDiagnosticsJson in
native/sidebar/native-sidebar.tsx (lines 22292-22359), near the
doctorChecks/doctorLoading/diagnosticsJson/diagnosticsLoading state and result
handling in native/sidebar/modal-host.tsx (lines 698-2728), and above
SupportSettingsTab in sidebar/settings-modal.tsx (lines 11185-11326).
In `@scripts/generate-agent-paths.mjs`:
- Around line 49-54: Streamline the snakeToConstant function by removing the
split/map/join pipeline and returning the input value transformed with a single
toUpperCase() call. Preserve the existing output for underscore-separated
strings.
In `@shared/gxserver-protocol-conformance.test.ts`:
- Around line 123-131: Replace the subset-only EXPECTED_ENDPOINTS satisfies
check with an exhaustive Record<GxserverEndpointPath, true> keyed by every
endpoint variant, then derive EXPECTED_ENDPOINTS from that record. Update the
test comment and runtime assertion to reflect the record-based compile-time
coverage, preserving the existing endpoint list and drift check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7f6b7522-ffc7-460f-96e0-b177bb67c260
⛔ Files ignored due to path filters (2)
shared/agent-paths.generated.tsis excluded by!**/*.generated.*tui2/website/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (33)
.gitignorecontributions/CHANGELOG.mdcontributions/Ghostex-Security-Audit-Hardening-Pass.mdcontributions/ghostex-hardening-features.mdcontributions/issue-38-menubar-agent-design-decisions.mdcontributions/issue-52-libxev-kqueue-upstream-report.mdcontributions/issue-61-ssh-investigation.mdcontributions/security-audit-verification-and-remediation.mdgxserver-rs/src/agent_hooks.rsgxserver-rs/src/agent_skills.rsgxserver-rs/src/bin/generate-agent-paths.rsgxserver-rs/src/cli.rsgxserver-rs/src/doctor.rsgxserver-rs/src/lib.rsgxserver-rs/src/paths.rsgxserver-rs/src/protocol.rsgxserver-rs/src/repository_clone.rsgxserver-rs/src/server.rsgxserver-rs/src/subprocess_policy.rsgxserver-rs/src/t3_runtime.rsnative/macos/ghostexHost/Sources/ghostexHost/RemoteGxserverClient.swiftnative/sidebar/manage.tsxnative/sidebar/meo/helpers/images.tsnative/sidebar/modal-host.tsxnative/sidebar/native-sidebar.tsxscripts/generate-agent-paths.mjsshared/ghostex-settings.tsshared/gxserver-protocol-conformance.test.tsshared/gxserver-protocol.tsshared/session-grid-contract-sidebar.tssidebar/session-card-content.tsxsidebar/settings-modal.tsxtui2/website/package.json
| fn every_endpoint_for_path_is_implemented_or_known_not_implemented() { | ||
| // Set-equality direction: Rust routing table ⊆ (implemented ∪ known-not-implemented). | ||
| // Every path in endpoint_for() must either have a real handler or | ||
| // be in known_not_implemented_endpoints(). | ||
| let not_impl: HashSet<&str> = | ||
| known_not_implemented_endpoints().iter().copied().collect(); | ||
| let implemented: HashSet<&str> = ALL_IMPLEMENTED_ENDPOINTS | ||
| .iter() | ||
| .copied() | ||
| .collect(); | ||
| let mut known: HashSet<&str> = HashSet::new(); | ||
| known.extend(¬_impl); | ||
| known.extend(&implemented); | ||
|
|
||
| for path in all_ts_endpoint_paths() { | ||
| let desc = endpoint_for(path); | ||
| assert!( | ||
| desc.is_some(), | ||
| "Path {} is in TS contract but not in endpoint_for()", | ||
| path, | ||
| ); | ||
| assert!( | ||
| known.contains(path), | ||
| "Path {} is in endpoint_for() but has neither a handler nor is in known_not_implemented_endpoints()", | ||
| path, | ||
| ); | ||
| } | ||
|
|
||
| // Also verify that known_not_implemented paths really resolve in endpoint_for(). | ||
| for path in ¬_impl { | ||
| assert!( | ||
| endpoint_for(path).is_some(), | ||
| "Known-not-implemented path {} is not in endpoint_for()", | ||
| path, | ||
| ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the reverse subset direction for true equality.
The ALL_IMPLEMENTED_ENDPOINTS doc comment claims that this test asserts this set + known_not_implemented == all_ts_endpoint_paths. However, the loop only verifies that all_ts_endpoint_paths is a subset of known. If ALL_IMPLEMENTED_ENDPOINTS contains extraneous paths that are missing from all_ts_endpoint_paths, this test will silently pass.
✅ Proposed fix to assert full equality
for path in all_ts_endpoint_paths() {
let desc = endpoint_for(path);
assert!(
desc.is_some(),
"Path {} is in TS contract but not in endpoint_for()",
path,
);
assert!(
known.contains(path),
"Path {} is in endpoint_for() but has neither a handler nor is in known_not_implemented_endpoints()",
path,
);
}
+
+ for path in known.iter() {
+ assert!(
+ all_ts_endpoint_paths().contains(path),
+ "Path {} is in known/implemented endpoints but missing from all_ts_endpoint_paths()",
+ path,
+ );
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn every_endpoint_for_path_is_implemented_or_known_not_implemented() { | |
| // Set-equality direction: Rust routing table ⊆ (implemented ∪ known-not-implemented). | |
| // Every path in endpoint_for() must either have a real handler or | |
| // be in known_not_implemented_endpoints(). | |
| let not_impl: HashSet<&str> = | |
| known_not_implemented_endpoints().iter().copied().collect(); | |
| let implemented: HashSet<&str> = ALL_IMPLEMENTED_ENDPOINTS | |
| .iter() | |
| .copied() | |
| .collect(); | |
| let mut known: HashSet<&str> = HashSet::new(); | |
| known.extend(¬_impl); | |
| known.extend(&implemented); | |
| for path in all_ts_endpoint_paths() { | |
| let desc = endpoint_for(path); | |
| assert!( | |
| desc.is_some(), | |
| "Path {} is in TS contract but not in endpoint_for()", | |
| path, | |
| ); | |
| assert!( | |
| known.contains(path), | |
| "Path {} is in endpoint_for() but has neither a handler nor is in known_not_implemented_endpoints()", | |
| path, | |
| ); | |
| } | |
| // Also verify that known_not_implemented paths really resolve in endpoint_for(). | |
| for path in ¬_impl { | |
| assert!( | |
| endpoint_for(path).is_some(), | |
| "Known-not-implemented path {} is not in endpoint_for()", | |
| path, | |
| ); | |
| } | |
| } | |
| } | |
| fn every_endpoint_for_path_is_implemented_or_known_not_implemented() { | |
| // Set-equality direction: Rust routing table ⊆ (implemented ∪ known-not-implemented). | |
| // Every path in endpoint_for() must either have a real handler or | |
| // be in known_not_implemented_endpoints(). | |
| let not_impl: HashSet<&str> = | |
| known_not_implemented_endpoints().iter().copied().collect(); | |
| let implemented: HashSet<&str> = ALL_IMPLEMENTED_ENDPOINTS | |
| .iter() | |
| .copied() | |
| .collect(); | |
| let mut known: HashSet<&str> = HashSet::new(); | |
| known.extend(¬_impl); | |
| known.extend(&implemented); | |
| for path in all_ts_endpoint_paths() { | |
| let desc = endpoint_for(path); | |
| assert!( | |
| desc.is_some(), | |
| "Path {} is in TS contract but not in endpoint_for()", | |
| path, | |
| ); | |
| assert!( | |
| known.contains(path), | |
| "Path {} is in endpoint_for() but has neither a handler nor is in known_not_implemented_endpoints()", | |
| path, | |
| ); | |
| } | |
| for path in known.iter() { | |
| assert!( | |
| all_ts_endpoint_paths().contains(path), | |
| "Path {} is in known/implemented endpoints but missing from all_ts_endpoint_paths()", | |
| path, | |
| ); | |
| } | |
| // Also verify that known_not_implemented paths really resolve in endpoint_for(). | |
| for path in ¬_impl { | |
| assert!( | |
| endpoint_for(path).is_some(), | |
| "Known-not-implemented path {} is not in endpoint_for()", | |
| path, | |
| ); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gxserver-rs/src/protocol.rs` around lines 663 - 700, Update
every_endpoint_for_path_is_implemented_or_known_not_implemented to also verify
the reverse subset: each path in known (or separately in
ALL_IMPLEMENTED_ENDPOINTS and known_not_implemented_endpoints()) must exist in
all_ts_endpoint_paths. Preserve the existing forward checks and ensure
extraneous implemented or known-not-implemented paths cause the test to fail.
| async fn handle_doctor_fix_http( | ||
| state: &AppState, | ||
| endpoint_path: String, | ||
| request_id: String, | ||
| body: &Value, | ||
| ) -> RoutedResponse { | ||
| let params = match body.get("params").and_then(|v| v.as_object()) { | ||
| Some(params) => params, | ||
| None => { | ||
| return routed_json( | ||
| Some(endpoint_path), | ||
| StatusCode::BAD_REQUEST, | ||
| rpc_error("badRequest", "Missing params object.", Some(request_id)), | ||
| ); | ||
| } | ||
| }; | ||
| let fix_id = params.get("fixId").and_then(Value::as_str).unwrap_or(""); | ||
| let confirmation_token = params | ||
| .get("confirmationToken") | ||
| .and_then(Value::as_str) | ||
| .unwrap_or(""); | ||
|
|
||
| // Validate fix ID and confirmation token. | ||
| let applied = match (fix_id, confirmation_token) { | ||
| ("skills.reinstall", "reinstall-skills") => { | ||
| crate::agent_skills::install_agent_skills( | ||
| &state.paths, | ||
| &serde_json::Map::new(), | ||
| ) | ||
| .await | ||
| .is_ok() | ||
| } | ||
| ("hooks.reinstall", "reinstall-hooks") => { | ||
| crate::agent_hooks::install_agent_hooks( | ||
| &state.paths, | ||
| &serde_json::Map::new(), | ||
| ) | ||
| .is_ok() | ||
| } | ||
| _ => false, | ||
| }; | ||
|
|
||
| if applied { | ||
| routed_json( | ||
| Some(endpoint_path), | ||
| StatusCode::OK, | ||
| rpc_success(request_id, json!({ "applied": true, "fixId": fix_id })), | ||
| ) | ||
| } else { | ||
| routed_json( | ||
| Some(endpoint_path), | ||
| StatusCode::BAD_REQUEST, | ||
| rpc_error( | ||
| "badRequest", | ||
| format!("Unknown fix or invalid confirmation token: {fix_id}"), | ||
| Some(request_id), | ||
| ), | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
printf '\n== doctor.rs matches ==\n'
rg -n 'DoctorFix|toolchain\.install|reinstall-skills|reinstall-hooks|install-tools' gxserver-rs/src/doctor.rs gxserver-rs/src/server.rs gxserver-rs/src/protocol.rs
printf '\n== handle_doctor_fix_http ==\n'
sed -n '1521,1580p' gxserver-rs/src/server.rs
printf '\n== check_toolchain / related doctor fix constructors ==\n'
sed -n '1,260p' gxserver-rs/src/doctor.rs
printf '\n== protocol auth routing snippets ==\n'
sed -n '1,260p' gxserver-rs/src/protocol.rsRepository: maddada/Ghostex
Length of output: 19807
Add the missing toolchain.install handler in handle_doctor_fix_http
check_toolchain() already emits DoctorFix { id: "toolchain.install", confirmation_token: "install-tools" }, but this match only accepts skills.reinstall and hooks.reinstall. That makes the “Install missing tools through gxserver” fix fail with BAD_REQUEST every time. Consider centralizing the fix IDs/tokens in a shared registry to avoid this drift.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gxserver-rs/src/server.rs` around lines 1521 - 1580, Update the fix
validation match in handle_doctor_fix_http to accept the toolchain.install and
install-tools ID/token pair emitted by check_toolchain, and invoke the existing
toolchain installation routine using state.paths, returning applied: true on
success. Preserve the current skills.reinstall and hooks.reinstall behavior;
centralize identifiers only if an existing shared registry is available.
| #[cfg(unix)] | ||
| { | ||
| let mut file = fs::File::create(path)?; | ||
| file.set_permissions(fs::Permissions::from_mode(0o600))?; | ||
| file.write_all(contents.as_bytes())?; | ||
| file.sync_all()?; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Prevent file permission race condition (TOCTOU).
Creating a file with default permissions and subsequently restricting them via set_permissions introduces a Time-of-Check to Time-of-Use (TOCTOU) vulnerability. An attacker with access to the parent directory could open the file between creation and the chmod operation, retaining a read-capable file descriptor to read the secrets written afterward.
Use OpenOptionsExt to atomically create the file with the 0o600 mode.
🔒️ Proposed fix
#[cfg(unix)]
{
- let mut file = fs::File::create(path)?;
- file.set_permissions(fs::Permissions::from_mode(0o600))?;
+ use std::os::unix::fs::OpenOptionsExt;
+ let mut file = fs::OpenOptions::new()
+ .write(true)
+ .create(true)
+ .truncate(true)
+ .mode(0o600)
+ .open(path)?;
file.write_all(contents.as_bytes())?;
file.sync_all()?;
Ok(())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[cfg(unix)] | |
| { | |
| let mut file = fs::File::create(path)?; | |
| file.set_permissions(fs::Permissions::from_mode(0o600))?; | |
| file.write_all(contents.as_bytes())?; | |
| file.sync_all()?; | |
| Ok(()) | |
| } | |
| #[cfg(unix)] | |
| { | |
| use std::os::unix::fs::OpenOptionsExt; | |
| let mut file = fs::OpenOptions::new() | |
| .write(true) | |
| .create(true) | |
| .truncate(true) | |
| .mode(0o600) | |
| .open(path)?; | |
| file.write_all(contents.as_bytes())?; | |
| file.sync_all()?; | |
| Ok(()) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gxserver-rs/src/subprocess_policy.rs` around lines 181 - 188, Update the Unix
file creation block in the subprocess policy path to use OpenOptions with
OpenOptionsExt, setting create/write/truncate behavior and mode 0o600
atomically. Remove the separate set_permissions call while preserving the
existing write_all, sync_all, and error propagation flow.
| // The managed T3 runtime is only ever reached over loopback (see | ||
| // `t3_http_request`, which connects to `T3_RUNTIME_HOST`). Bind the listener to | ||
| // loopback as well so the runtime is not reachable from other hosts, containers, | ||
| // or bridged interfaces. Do not widen this to `0.0.0.0` without an explicit, | ||
| // documented remote opt-in and verified auth on every T3 endpoint. | ||
| const T3_RUNTIME_LISTEN_HOST: &str = "127.0.0.1"; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if the native macOS wrapper still binds the T3 runtime to 0.0.0.0
rg 'listenHost\s*=\s*"0\.0\.0\.0"' native/macos/Repository: maddada/Ghostex
Length of output: 273
Bind the macOS wrapper to 127.0.0.1
native/macos/ghostexHost/Sources/ghostexHost/NativeT3CodePaneReproLog.swift still hardcodes listenHost = "0.0.0.0", so launches through the native wrapper can expose the T3 runtime on external interfaces. Change it to 127.0.0.1 too.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gxserver-rs/src/t3_runtime.rs` around lines 20 - 25, Update the macOS
wrapper’s listenHost configuration in NativeT3CodePaneReproLog.swift from
0.0.0.0 to 127.0.0.1, matching the T3_RUNTIME_LISTEN_HOST loopback binding and
keeping native-wrapper launches inaccessible from external interfaces.
| if (isDoctorFixResultMessage(message.message)) { | ||
| if (message.message.ok) { | ||
| toast.success("Fix applied successfully"); | ||
| setDoctorLoading(true); | ||
| postAppModalHostMessage({ type: "runDoctor" }, "AppModals:runDoctor"); | ||
| } else { | ||
| toast.error(message.message.error ?? "Fix failed"); | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Auto re-run after a successful fix is sent on the wrong message channel — stuck "Running..." state.
On a successful doctorFixResult, this calls postAppModalHostMessage({ type: "runDoctor" }, ...), which posts to the native AppModalHost lifecycle bridge (window.webkit.messageHandlers.ghostexAppModalHost), used elsewhere in this file only for ready/presented/contentHeightMeasured/toastDismissed/debugLog. "runDoctor" is a SidebarToExtensionMessage that must go through vscode.postMessage to reach handleSidebarMessage's case "runDoctor": void runDoctorChecks(); in native-sidebar.tsx.
Because native never receives this retrigger, runDoctorChecks() is never called, no doctorChecksResult ever arrives, and doctorLoading (set to true right before this call) is never cleared — the Support tab's "Run Doctor" button becomes permanently disabled after any successful fix, until the whole app-modal host remounts.
🔧 Proposed fix
if (isDoctorFixResultMessage(message.message)) {
if (message.message.ok) {
toast.success("Fix applied successfully");
setDoctorLoading(true);
- postAppModalHostMessage({ type: "runDoctor" }, "AppModals:runDoctor");
+ vscode.postMessage({ type: "runDoctor" });
} else {
toast.error(message.message.error ?? "Fix failed");
}
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (isDoctorFixResultMessage(message.message)) { | |
| if (message.message.ok) { | |
| toast.success("Fix applied successfully"); | |
| setDoctorLoading(true); | |
| postAppModalHostMessage({ type: "runDoctor" }, "AppModals:runDoctor"); | |
| } else { | |
| toast.error(message.message.error ?? "Fix failed"); | |
| } | |
| return; | |
| } | |
| if (isDoctorFixResultMessage(message.message)) { | |
| if (message.message.ok) { | |
| toast.success("Fix applied successfully"); | |
| setDoctorLoading(true); | |
| vscode.postMessage({ type: "runDoctor" }); | |
| } else { | |
| toast.error(message.message.error ?? "Fix failed"); | |
| } | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@native/sidebar/modal-host.tsx` around lines 2566 - 2575, Update the
successful fix handling in the isDoctorFixResultMessage branch to dispatch the
runDoctor message through the existing vscode.postMessage
SidebarToExtensionMessage channel instead of postAppModalHostMessage. Preserve
setting doctorLoading to true and ensure handleSidebarMessage receives the
message so runDoctorChecks() executes and clears the loading state.
| // Skills install under ~/.agents/skills (see gxserver-rs agent_skills.rs), so | ||
| // the status checks must read the dotted directory. Reading ~/agents (no dot) | ||
| // left every badge stuck on "Not installed" even after a successful install. | ||
| const browserSkillPath = path.join(home, SKILLS_ROOT, "ghostex-browser-use", "SKILL.md"); | ||
| const browserSkillInstalled = isFile(browserSkillPath); | ||
| const computerUseSkillPath = path.join(home, "agents", "skills", "ghostex-computer-use", "SKILL.md"); | ||
| const computerUseSkillPath = path.join(home, SKILLS_ROOT, "ghostex-computer-use", "SKILL.md"); | ||
| const computerUseSkillInstalled = isFile(computerUseSkillPath); | ||
| const agentOrchestrationSkillPath = path.join(home, "agents", "skills", "ghostex-agent-orchestration", "SKILL.md"); | ||
| const agentOrchestrationSkillPath = path.join(home, SKILLS_ROOT, "ghostex-agent-orchestration", "SKILL.md"); | ||
| const agentOrchestrationSkillInstalled = isFile(agentOrchestrationSkillPath); | ||
| const fable55OrchestrationSkillPath = path.join(home, "agents", "skills", "ghostex-fable-5.5-orchestration", "SKILL.md"); | ||
| const fable55OrchestrationSkillPath = path.join(home, SKILLS_ROOT, "ghostex-fable-5.5-orchestration", "SKILL.md"); | ||
| const fable55OrchestrationSkillInstalled = isFile(fable55OrchestrationSkillPath); | ||
| const generateTitleSkillPath = path.join(home, "agents", "skills", "ghostex-generate-title", "SKILL.md"); | ||
| const generateTitleSkillPath = path.join(home, SKILLS_ROOT, "ghostex-generate-title", "SKILL.md"); | ||
| const generateTitleSkillInstalled = isFile(generateTitleSkillPath); | ||
| const moveCodexSessionSkillPath = path.join(home, "agents", "skills", "ghostex-move-codex-session", "SKILL.md"); | ||
| const moveCodexSessionSkillPath = path.join(home, SKILLS_ROOT, "ghostex-move-codex-session", "SKILL.md"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the bare (non-interpolated) usages vs. the correctly-interpolated ones in the same file.
rg -n '\bSKILLS_ROOT\b|\bHOOKS_ROOT\b|\bAGENTS_ROOT\b' native/sidebar/native-sidebar.tsx
rg -n 'function runNativeNodeScript' -A 40 native/sidebar/native-sidebar.tsxRepository: maddada/Ghostex
Length of output: 2879
Bare SKILLS_ROOT / HOOKS_ROOT inside the generated Node scripts will crash at runtime.
These String.raw blocks are passed to runNativeNodeScript, so the bare names are emitted literally into the subprocess source. SKILLS_ROOT and HOOKS_ROOT are not declared there, so the first path check throws ReferenceError and aborts the whole status/catalog script. Use ${SKILLS_ROOT} / ${HOOKS_ROOT} here, like the other interpolated constants in this file.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@native/sidebar/native-sidebar.tsx` around lines 22598 - 22611, The generated
Node scripts passed to runNativeNodeScript reference SKILLS_ROOT and HOOKS_ROOT
as undeclared bare identifiers, causing runtime failures. Update the relevant
String.raw script blocks to interpolate these constants as ${SKILLS_ROOT} and
${HOOKS_ROOT}, including the skill status and catalog path checks, while
preserving the existing path construction.
| function profilesFor(candidatePath) { | ||
| if (isRelativeTo(candidatePath, p(".agents")) || isRelativeTo(candidatePath, path.join(home, "agents"))) { | ||
| if (isRelativeTo(candidatePath, path.join(home, "agents", "hooks"))) { | ||
| if (isRelativeTo(candidatePath, p(HOOKS_ROOT))) { | ||
| return linkedProfiles.concat([piAgent]); | ||
| } | ||
| return linkedProfiles; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="native/sidebar/native-sidebar.tsx"
# Show the relevant ranges around the cited lines.
sed -n '39550,39620p;39880,39920p' "$file" | cat -n
# Find the surrounding String.raw template and all HOOKS_ROOT occurrences in the file.
rg -n --context 3 'HOOKS_ROOT|getAgentsHubCatalogNodeScript|String\.raw|profilesFor|hooksRoot' "$file"Repository: maddada/Ghostex
Length of output: 12191
HOOKS_ROOT is still bare inside the generated Agents Hub catalog script. profilesFor() and hooksRoot need ${HOOKS_ROOT} here; otherwise catalog generation will throw ReferenceError: HOOKS_ROOT is not defined when it reaches those paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@native/sidebar/native-sidebar.tsx` around lines 39594 - 39599, Update the
generated Agents Hub catalog script’s profilesFor() logic and hooksRoot
reference to interpolate the HOOKS_ROOT value rather than treating HOOKS_ROOT as
a runtime variable. Ensure both generated references use ${HOOKS_ROOT},
preventing a ReferenceError during catalog generation while preserving the
existing path checks and linkedProfiles behavior.
| | "/api/destructiveAdminAction" | ||
| | "/api/capabilities" | ||
| | "/api/doctor/exportDiagnostics" | ||
| | "/api/doctor/fix" | ||
| | "/api/doctor/run"; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Add missing /api/t3Runtime/* paths to GxserverEndpointPath.
The t3Runtime paths (/api/t3Runtime/status, start, stop, panes) were added to the EXPECTED_ENDPOINTS array in shared/gxserver-protocol-conformance.test.ts, which uses satisfies readonly GxserverEndpointPath[]. Because these paths are missing from the GxserverEndpointPath union, this will cause a TypeScript compilation error.
🐛 Proposed fix
| "/api/installTool"
| "/api/browseFilesystem"
| "/api/destructiveAdminAction"
+ | "/api/t3Runtime/status"
+ | "/api/t3Runtime/start"
+ | "/api/t3Runtime/stop"
+ | "/api/t3Runtime/panes"
| "/api/capabilities"
| "/api/doctor/exportDiagnostics"
| "/api/doctor/fix"
| "/api/doctor/run";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | "/api/destructiveAdminAction" | |
| | "/api/capabilities" | |
| | "/api/doctor/exportDiagnostics" | |
| | "/api/doctor/fix" | |
| | "/api/doctor/run"; | |
| | "/api/destructiveAdminAction" | |
| | "/api/t3Runtime/status" | |
| | "/api/t3Runtime/start" | |
| | "/api/t3Runtime/stop" | |
| | "/api/t3Runtime/panes" | |
| | "/api/capabilities" | |
| | "/api/doctor/exportDiagnostics" | |
| | "/api/doctor/fix" | |
| | "/api/doctor/run"; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@shared/gxserver-protocol.ts` around lines 169 - 173, Extend the
GxserverEndpointPath union with the four t3Runtime endpoints:
/api/t3Runtime/status, /api/t3Runtime/start, /api/t3Runtime/stop, and
/api/t3Runtime/panes, so EXPECTED_ENDPOINTS satisfies the declared type.
|
Hey @pt-act, I appreciate the PR but it's A LOT in 1 go. It's even touched dependencies and a lot of risky parts that would take a lot from me to review. Can I please understand what you're trying to solve here? I see 6 issues being fixed in 1 PR which is also not a nice thing for me as the reviewer :( If you want join the discord and we can discuss better and then merge them in 1 by 1 if I agree with you that this added code is needed. Keep in mind that code is not free. I'll have to maintain and keep the stuff you're adding now for a very long into the future so hope you understand why I'm hesitant to merge this huge load of code in 1 go. For next time if you want to contribute to any project the best way is to do smaller PRs and 1 or 2 at a time. And even better would be to join discord or create an issue asking if something is needed first. All the best 🙏🏻 |
Upstream's 17-commit GPUI pivot retires the native macOS ghostexHost shell (commit 62e63b5) and makes GPUI the default local entrypoint (bb32814), winding down native macOS surfaces in favor of GPUI. Resolved conflicts: - native/macos/ghostexHost/Sources/ghostexHost/RemoteGxserverClient.swift: take upstream deletion. maddada#61 SSH fix is obsolete — the native shell is retired in favor of GPUI (the BatchMode fast-fail / SSH auth guidance can inform GPUI's remote path separately). - shared/ghostex-settings.ts: keep both 'support' and 'about' nav tabs (ours added support, upstream added about — additive). - sidebar/settings-modal.tsx: keep both Support and About TabsContent blocks, each wrapped in its own isFirstLaunchSetup conditional. - native/sidebar/native-sidebar.tsx: take upstream's fable-5.6 rename with our SKILLS_ROOT (maddada#58 dotted-dir ~/.agents/skills) path; drop the fable-5.5 lines. The rest of the file already references fable56. gxserver-rs (security hardening, doctor backend + /api/doctor/*, subprocess_policy, paths, capability discovery, agent_skills) auto- merged cleanly — frontend-agnostic, the durable core of the PR. Verified: - cargo test -p gxserver --lib: 483 passed, 0 failed, incl. paths::tests::generated_ts_matches_rust_agent_paths (AgentPaths drift) and all 12 agent_skills tests. - rg confirms the maddada#58 fix is preserved: path.join(home,"agents","skills") appears only at the intentional legacy catalog group (~line 39767); no fable-5.5/fable55 remains; all skill-status checkers use SKILLS_ROOT. - Typecheck (tsc --noEmit): 0 errors in the merged files. 7 pre-existing PR errors remain (6 in retired native doctor UI modal-host.tsx scope mismatch + 1 gxserver-client.ts switch return) — byte-identical to origin/main, NOT introduced by this merge.
The native doctor UI added in 4e3c600 is compile-broken and non-functional: its doctor useState lives in AppModalHost() while the message handlers that call the setters sit in the separate top-level useModalStateFromNative() hook — 6 TS scope errors (setDoctorChecks / setDoctorLoading / setDiagnosticsJson / setDiagnosticsLoading out of scope). The PR's frontend gate (Rust tests + TS conformance tests) never ran tsc --noEmit on the React sidebar, so the breakage passed validation. Given the GPUI pivot is retiring the native macOS surfaces, fixing the scoping is throwaway work. Remove the native doctor UI wiring and defer the surface to GPUI; the durable backend is retained untouched. Removed (frontend wiring only): - shared/ghostex-settings.ts: drop the "support" nav tab (keep "about"). - sidebar/settings-modal.tsx: drop the SupportSettingsTab component, its TabsContent block, the nav entry, the SettingsModalProps doctor fields, and now-orphaned imports (SidebarDoctorCheck, IconStethoscope). - native/sidebar/modal-host.tsx: drop the doctor useState, the <SettingsModal> doctor props, the orphaned message handlers, the three type guards, and the doctor type imports. - native/sidebar/native-sidebar.tsx: drop the runDoctorChecks / applyDoctorFix / exportDiagnosticsJson RPC handlers and the runDoctor / applyDoctorFix / exportDiagnostics switch cases. Retained (frontend-agnostic, fully tested): - gxserver-rs doctor backend: doctor.rs, /api/doctor/run|fix|exportDiagnostics routes, CLI. cargo test -p gxserver --lib: 483 passed, 0 failed. - shared message/type contract (SidebarDoctorCheck, SidebarDoctorChecksResultMessage, SidebarDoctorFixResultMessage, SidebarDiagnosticsExportResultMessage) for GPUI to reuse. Verified: tsc --noEmit drops from 7 errors to 1 (the 6 doctor scope errors gone; the remaining gxserver-client.ts:1033 TS2366 is pre-existing and unrelated). vitest: 942 passed; the 2 failures (gpui-agents-hub-scanner-parity agents/hooks gap, native-agent-prompt-text bun:test runner mismatch) are both pre-existing on the PR tip and unrelated to this change. Scope note: keep-vs-fix-vs-rebuild of the doctor UI is the maintainer's call; this surfaces the decision on the PR rather than carrying known-broken native code forward through the pivot. Trivially reversible.
|
Merged upstream 4 conflicts resolved:
One decision for you (scope call): the native doctor UI. Given the GPUI pivot is retiring the native macOS surfaces, fixing the scoping is throwaway work. So in a follow-up commit I pulled the native doctor UI wiring (the Support tab, the mis-scoped handlers/state, and the |
|
Consolidated heads-up — three pre-existing issues I found while verifying the merge. All are unrelated to this PR's changes and all are yours to triage; flagging so they're not silent. None block this PR.
|
|
Hi maddada — fair on all counts, and thanks for the directness. Let me answer your main question first, honestly. Context: it began as a security audit of gxserver-rs, which surfaced a couple of concrete fixes and from there we got ahead of ourselves and folded in several improvements we thought were useful without first checking whether you want them. That's the wrong way to contribute to a project you'll maintain long-term. Two of these solve concrete problems; the rest are improvements we proposed on our own initiative and bundling those in without asking you first was the mistake. What actually solves a reported/real problem: #58 (skills always "Not installed") a real bug: the installer writes to ~/.agents/… but the status checker read ~/agents/… (no dot), so skills/Desktop Control never showed as installed and "Uninstall" silently did nothing. Small, self-contained fix. A security audit of gxserver-rs (which we ran on our own initiative, not something you asked for) surfaced two things worth fixing: the repo-clone subprocess inherited the entire ambient environment (the env_clear() was effectively a no-op, so tokens/git config could leak into git clone), and the T3 runtime bound to 0.0.0.0 when it's only ever reached over loopback. What we were trying to solve: The unifying goal: each of the five takes a specific failure the audit or your open issues exposed and addresses the class behind it — so that class becomes self-diagnosing or can't recur instead of just patching the one instance. ghostex doctor: Achieves one command that reports skills / daemon / SSH / permissions / T3 state and offers safe fixes. Capability discovery + conformance test: Achieves a runtime endpoint listing which APIs are actually implemented, plus CI that fails if the shared TS contract and the Rust server drift. Why: the audit found endpoints declared in the contract but not implemented server-side, so clients can't tell what's live. (Honest: this is preventive, not an active user-facing bug.) Diagnostic bundle export: Achieves a "Copy Diagnostics" that emits a redacted bundle (versions, config summary, recent errors, T3/skills status). Unified subprocess policy: Achieves one auditable module defining what environment each spawned process inherits, plus a 0600 secret-file helper. That's the actual goal behind each. But I'll be equally straight: this is our read of what would help, extrapolated from a handful of instances, a single #58 bug may not, in your judgment, justify a whole shared-path system, and your existing debugging may not need a doctor. That's exactly why these belong as questions to you, not as merged code. I'll close this PR. If you're up for it and join Discord to align, then send only what you want as small single-purpose PRs. Apologies for the assumptions and unsolicited initiatives, thanks for creating and maintaining this project. Kind regards pt-act (Discord: .rialm) |
Brings in: ghostex-web workspace app, release CI workflows, GPUI CEF focus hardening, gxserver-rs polish (rustfmt, color env, web UI bootstrap), mobile remote reconnect stabilization, sidebar project collections, android/iOS submodule bumps. Preserves pt-act hardening: security allowlist, subprocess policy, doctor diagnostics, SSH actionable errors, astro/starlight bumps.
workflow_dispatch-only on macos-15-intel. Builds the GPUI macOS app for x86_64 and uploads an ad-hoc signed, unnotarized DMG as a CI artifact. Scope input (minimal default / full) to de-risk compile snags before the full build. Calls prepare-macos-runtime.sh and build-macos-app.sh directly with GHOSTEX_MACOS_ARCH=x86_64, no Sparkle/notarize/remote-packages/on-demand. No existing files modified; zero risk to the arm64 release flow. Cache budget: stable things only (skips gpui/target and gxserver-rs/target to avoid blowing the 10GB repo cache budget and evicting the ~500MB cef-cache). Shelf life: GitHub is phasing Intel macOS out (macos-13 retired, macos-14 deprecated); macos-15-intel works today but the durable fallback is cross-compiling from arm64.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/build-gpui-macos-x86_64.yml:
- Around line 119-127: Prevent shell injection in the workflow by passing inputs
and step outputs through environment variables instead of interpolating them
inside bash. In .github/workflows/build-gpui-macos-x86_64.yml lines 119-127,
expose inputs.version as INPUT_VERSION and use it in the Resolve version step;
lines 188-198, expose steps.version.outputs.version as VERSION and use it; lines
207-246, expose inputs.scope and the version output as INPUT_SCOPE and VERSION
and reference those variables throughout, including the generated DMG command.
Also correct “macos-13 runner” to “macos-15 runner” in that final section.
- Around line 57-60: Update the actions/checkout@v6 step in the macOS x86_64
workflow to set persist-credentials to false, while preserving the existing
fetch-depth and submodules settings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 949d9362-f7c9-4668-bd9a-ce8cb37956fe
📒 Files selected for processing (1)
.github/workflows/build-gpui-macos-x86_64.yml
| - uses: actions/checkout@v6 | ||
| with: | ||
| fetch-depth: 0 | ||
| submodules: false |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check the latest version of actions/checkout
gh api repos/actions/checkout/releases/latest --jq '.tag_name'Repository: maddada/Ghostex
Length of output: 160
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow snippet =="
nl -ba .github/workflows/build-gpui-macos-x86_64.yml | sed -n '45,80p'
echo
echo "== upstream checkout tags =="
gh api repos/actions/checkout/git/refs/tags/v6 --jq '.ref + " " + .object.type + " " + .object.sha' || true
gh api repos/actions/checkout/git/refs/tags/v5 --jq '.ref + " " + .object.type + " " + .object.sha' || true
gh api repos/actions/checkout/git/refs/tags/v4 --jq '.ref + " " + .object.type + " " + .object.sha' || true
echo
echo "== latest releases =="
gh api repos/actions/checkout/releases/latest --jq '.tag_name'
gh api repos/actions/checkout/releases?per_page=5 --jq '.[].tag_name'Repository: maddada/Ghostex
Length of output: 216
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow snippet =="
nl -ba .github/workflows/build-gpui-macos-x86_64.yml | sed -n '45,80p'
echo
echo "== upstream checkout tags =="
for v in v6 v5 v4; do
echo "-- $v --"
gh api "repos/actions/checkout/git/refs/tags/$v" --jq '.ref + " " + .object.type + " " + .object.sha' || true
done
echo
echo "== latest releases =="
gh api repos/actions/checkout/releases/latest --jq '.tag_name'
gh api repos/actions/checkout/releases?per_page=5 --jq '.[].tag_name'Repository: maddada/Ghostex
Length of output: 216
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for endpoint in \
repos/actions/checkout/git/refs/tags/v6 \
repos/actions/checkout/git/refs/tags/v5 \
repos/actions/checkout/git/refs/tags/v4
do
echo "== $endpoint =="
gh api "$endpoint" --jq '.ref + " " + .object.type + " " + .object.sha' || echo "missing"
done
echo "== latest release =="
gh api repos/actions/checkout/releases/latest --jq '.tag_name'Repository: maddada/Ghostex
Length of output: 502
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== git-related commands in workflow =="
rg -n '\bgit\b|push:|checkout@|persist-credentials|fetch-depth|submodules' .github/workflows/build-gpui-macos-x86_64.yml
echo
echo "== full workflow size =="
wc -l .github/workflows/build-gpui-macos-x86_64.ymlRepository: maddada/Ghostex
Length of output: 751
🌐 Web query:
actions/checkout v6 release tag GitHub
💡 Result:
The actions/checkout GitHub action has multiple release tags within the v6 series. The v6 major version was released on November 20, 2025 [1]. Subsequent patch releases for this major version included v6.0.1, v6.0.2, and v6.0.3 [2]. As of July 18, 2026, the latest major version of the actions/checkout action is v7.0.0, which was released on June 18, 2026 [3][4]. Users are generally encouraged to use the latest major version for current features and security updates [5].
Citations:
- 1: https://github.com/actions/checkout/releases/tag/v6.0.0
- 2: https://github.com/actions/checkout/releases
- 3: https://github.com/actions/checkout/releases/tag/v7.0.0
- 4: https://github.com/actions/checkout
- 5: https://github.com/actions/checkout/?tab=readme-ov-file
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow section =="
sed -n '57,80p' .github/workflows/build-gpui-macos-x86_64.yml
echo
echo "== .gitmodules =="
if [ -f .gitmodules ]; then
sed -n '1,200p' .gitmodules
else
echo "no .gitmodules at repo root"
fi
echo
echo "== submodule URLs mentioned in workflow =="
rg -n 'git submodule update|submodules:|checkout@|persist-credentials|url =' .github/workflows/build-gpui-macos-x86_64.yml .gitmodules || trueRepository: maddada/Ghostex
Length of output: 4559
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for repo in \
maddada/Ghostex \
maddada/ghostex-android \
maddada/zmx \
maddada/ghostex-ios \
maddada/zehn \
maddada/code-server \
maddada/m7code
do
echo "== $repo =="
gh api "repos/$repo" --jq '.full_name + " private=" + (.private|tostring) + " archived=" + (.archived|tostring) + " disabled=" + (.disabled|tostring) + " visibility=" + (.visibility // "n/a")'
doneRepository: maddada/Ghostex
Length of output: 869
Set persist-credentials: false on checkout.
This workflow only updates public submodules later, so keeping the GITHUB_TOKEN in .git/config is unnecessary and leaves it available to downstream steps.
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 57-60: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/build-gpui-macos-x86_64.yml around lines 57 - 60, Update
the actions/checkout@v6 step in the macOS x86_64 workflow to set
persist-credentials to false, while preserving the existing fetch-depth and
submodules settings.
Source: Linters/SAST tools
| - name: Resolve version | ||
| id: version | ||
| run: | | ||
| if [[ -n "${{ inputs.version }}" ]]; then | ||
| echo "version=${{ inputs.version }}" >> "$GITHUB_OUTPUT" | ||
| else | ||
| echo "version=$(node -p 'require("./package.json").version')" >> "$GITHUB_OUTPUT" | ||
| fi | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Prevent command injection by using environment variables instead of template interpolation.
Directly interpolating GitHub Actions context variables (such as the free-text inputs.version) into inline bash scripts exposes the runner to arbitrary command execution. Any single quotes or structural characters in the input can escape the intended shell strings.
.github/workflows/build-gpui-macos-x86_64.yml#L119-L127: Bind${{ inputs.version }}to an environment variable in theenv:block and reference it as$INPUT_VERSIONin the script..github/workflows/build-gpui-macos-x86_64.yml#L188-L198: Bind${{ steps.version.outputs.version }}to aVERSIONenvironment variable and use$VERSIONin the script..github/workflows/build-gpui-macos-x86_64.yml#L207-L246: Bind both${{ inputs.scope }}and${{ steps.version.outputs.version }}to environment variables and use them in the script. Additionally, correct the documentation typo frommacos-13 runnertomacos-15 runner.
🔒️ Proposed fixes
For L119-L127:
- name: Resolve version
id: version
+ env:
+ INPUT_VERSION: ${{ inputs.version }}
run: |
- if [[ -n "${{ inputs.version }}" ]]; then
- echo "version=${{ inputs.version }}" >> "$GITHUB_OUTPUT"
+ if [[ -n "$INPUT_VERSION" ]]; then
+ echo "version=$INPUT_VERSION" >> "$GITHUB_OUTPUT"
elseFor L188-L198:
- name: Build DMG
+ env:
+ VERSION: ${{ steps.version.outputs.version }}
run: |
- VERSION="${{ steps.version.outputs.version }}"
APP_PATH="gpui/build/macos/Ghostex.app"For L207-L246:
- name: Run summary
if: always()
+ env:
+ INPUT_SCOPE: ${{ inputs.scope }}
+ VERSION: ${{ steps.version.outputs.version }}
run: |
{
echo "## GPUI macOS x86_64 build"
echo ""
echo "| | |"
echo "|---|---|"
- echo "| Scope | \`${{ inputs.scope }}\` |"
- echo "| Version | \`${{ steps.version.outputs.version }}\` |"
- echo "| Arch | x86_64 (Intel, macos-13 runner) |"
+ echo "| Scope | \`$INPUT_SCOPE\` |"
+ echo "| Version | \`$VERSION\` |"
+ echo "| Arch | x86_64 (Intel, macos-15 runner) |"
echo "| Signing | ad-hoc (unnotarized) |"
echo ""
echo "### Download"
echo ""
- echo "Grab the \`ghostex-macos-x86_64\` artifact from this run — it contains \`ghostex-${{ steps.version.outputs.version }}-x86_64.dmg\`."
+ echo "Grab the \`ghostex-macos-x86_64\` artifact from this run — it contains \`ghostex-$VERSION-x86_64.dmg\`."
echo ""
echo "### Install on Intel Mac (ad-hoc signed, unnotarized)"
echo ""
echo "Gatekeeper will block this DMG because it is ad-hoc signed and not notarized. To open:"
echo ""
echo '```bash'
- echo "open ghostex-${{ steps.version.outputs.version }}-x86_64.dmg # drag Ghostex.app to Applications"
+ echo "open ghostex-$VERSION-x86_64.dmg # drag Ghostex.app to Applications"
echo "xattr -dr com.apple.quarantine /Applications/Ghostex.app"
echo '```'
echo ""
echo "Or right-click the app → **Open** → **Open anyway**."
echo ""
echo "### Known gaps in this dev DMG"
echo ""
echo "- **Project board (bd) is not bundled** — Beads is a separate checkout, not a submodule. Use the signed release build for Project board."
- if [[ "${{ inputs.scope }}" == "minimal" ]]; then
+ if [[ "$INPUT_SCOPE" == "minimal" ]]; then
echo "- **Zehn search and TUI are skipped** (minimal scope). Re-run with scope=full to include them."
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Resolve version | |
| id: version | |
| run: | | |
| if [[ -n "${{ inputs.version }}" ]]; then | |
| echo "version=${{ inputs.version }}" >> "$GITHUB_OUTPUT" | |
| else | |
| echo "version=$(node -p 'require("./package.json").version')" >> "$GITHUB_OUTPUT" | |
| fi | |
| - name: Resolve version | |
| id: version | |
| env: | |
| INPUT_VERSION: ${{ inputs.version }} | |
| run: | | |
| if [[ -n "$INPUT_VERSION" ]]; then | |
| echo "version=$INPUT_VERSION" >> "$GITHUB_OUTPUT" | |
| else | |
| echo "version=$(node -p 'require("./package.json").version')" >> "$GITHUB_OUTPUT" | |
| fi |
🧰 Tools
🪛 zizmor (1.26.1)
[error] 122-122: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[error] 123-123: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
📍 Affects 1 file
.github/workflows/build-gpui-macos-x86_64.yml#L119-L127(this comment).github/workflows/build-gpui-macos-x86_64.yml#L188-L198.github/workflows/build-gpui-macos-x86_64.yml#L207-L246
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/build-gpui-macos-x86_64.yml around lines 119 - 127,
Prevent shell injection in the workflow by passing inputs and step outputs
through environment variables instead of interpolating them inside bash. In
.github/workflows/build-gpui-macos-x86_64.yml lines 119-127, expose
inputs.version as INPUT_VERSION and use it in the Resolve version step; lines
188-198, expose steps.version.outputs.version as VERSION and use it; lines
207-246, expose inputs.scope and the version output as INPUT_SCOPE and VERSION
and reference those variables throughout, including the generated DMG command.
Also correct “macos-13 runner” to “macos-15 runner” in that final section.
Source: Linters/SAST tools
Review fixes before first dispatch: - tui2 is a regular directory (not a submodule); git submodule update --init tui2 would fail. Drop it from the full-scope init; tui2 is already in the checkout from actions/checkout. - Clarify GHOSTEX_GPUI_BUNDLE_ID=com.madda.ghostex.host is deliberate (matches scripts/release-gpui/macos.sh:67, not a mistake); CEF/lid-sleep helper ids derive from it consistently. - Run summary: macos-13 -> macos-15-intel (stale string from the runner-label fix). Verified non-issues: - actions/checkout@v6 is used in all 8 other release-gpui-*.yml workflows (real released tag). - All submodule URLs are public HTTPS to maddada/* (same as existing release-gpui-macos.yml which inits code-server/t3code with no auth rewriting).
First dispatch failed: gpui crate path-deps at /Users/runner/work/_references/zed (and cef-rs, gpui-component) were missing because the workflow called prepare-macos-runtime.sh + build-macos-app.sh directly, skipping scripts/release-gpui/prepare-references.sh which clones those pinned references. Add the step right after submodule init. Sets GHOSTEX_RELEASE_SKIP_SUBMODULES=1 so prepare-references.sh does ONLY the _references/* checkouts (zed@65e1c5af, cef-rs@0ddbc2ac, gpui-component@0775df39, beads@672d9420) and skips submodule init (our workflow already inits per scope). Does NOT set GHOSTEX_RELEASE_SKIP_GPUI_REFERENCES (we need zed + cef-rs + gpui-component). beads is cloned but not consumed (no BEADS_ROOT set), so bd stays absent from the dev DMG by design.
Second dispatch failed at the final link of ghostex-gpui: clang: error: no such file or directory: '.../gpui/../ghostty/macos/GhosttyKit.xcframework/macos-arm64_x86_64/ghostty-internal.a'. The whole cargo tree compiled; it died at link because gpui/build.rs hard-links the GhosttyKit universal slice at a constant path, and the xcframework was never built — scripts/release-gpui/macos.sh (which we skip because it's arm64/signing-coupled) builds it via zig build -Demit-xcframework -Dxcframework-target=universal. Add that exact invocation as a step after the references step and before Prepare macOS runtime. build.rs wants the universal (macos-arm64_x86_64) slice, so we build universal even for an x86_64 app — the linker selects the x86_64 members from the fat archive. Zig cross-compiles the arm64 slice on the Intel runner. Everything it needs is already present (ZIG=0.15.2 from the install step, xcode-select/xcrun + Metal toolchain from the runner Xcode). Also add ghostty/.zig-cache and ghostty/macos/GhosttyKit.xcframework to the cache paths (and ghostty/build.zig.zon to the key) to speed retries — the GhosttyKit build is heavy (Metal shaders for both arches).
…G env)
Third dispatch failed instantly: 'GHOSTTY_ZIG is missing:' because the GhosttyKit step was at position 3 (after references) but the Zig install step that sets ZIG was at position 7. ${{ env.ZIG }} resolved to empty. Moved GhosttyKit to position 12 — after Zig install (6) and after cache (11, which restores ghostty/.zig-cache for fast retries). Now the step has ZIG set and the zig-cache restored before it runs.
The 'Next step' section was stale after full-scope went green — it still said 'Once this is green on minimal, re-run with scope=full'. Make it scope-aware: minimal runs point at re-running with scope=full; full runs point at the durable deliverable (parameterizing scripts/release-gpui/macos.sh for the signed/notarized release flow with appcast-x86_64.xml and on-demand bd) and surface the shelf-life pressure (GitHub phasing Intel macOS out). No re-run needed — this only affects the run-summary text.
3 participants
Summary
This PR bundles a full defensive-security pass over
gxserver-rsand the native surface, fixes for reported macOS issues, and five additive hardening features that each retire a root cause of a failure this project actually hit. Everything is additive — no existing happy-path behavior changes — and ships as independent, reviewable slices.Work is grouped below by theme. All Rust changes are in
gxserver-rs; native/frontend changes are innative/sidebar,sidebar, andshared. Documentation for every item lives undercontributions/.1. Security audit remediation (
gxserver-rs)Verified all six findings plus the additional note from the initial audit against the source (every finding reproduced), and fixed the two actionable code findings.
repository_clone.rs): replaced broad ambient-environment inheritance — whereenv_clear()was effectively a no-op because everything but three color vars was re-added — with an explicit allowlist (PATH, HOME, SSH agent, locale/LC_*, proxy, temp, Windows essentials). Tokens,GIT_*/GIT_SSH_COMMANDoverrides,LD_PRELOAD, etc. are dropped. Added tests for the allowlist and ambient-secret non-leakage.t3_runtime.rs):T3_RUNTIME_LISTEN_HOSTchanged from0.0.0.0to127.0.0.1. The runtime is only ever reached over loopback, so this removes off-host/container reachability with no behavior change./api/eventsquery token), 4 (shell setup command), 5 & 6 (declarative remote/admin surface) were verified as intentional design or informational parity gaps and documented rather than changed.Detail:
contributions/security-audit-verification-and-remediation.md2. Reported macOS issue fixes
native/sidebar/native-sidebar.tsx): skills/hooks install under~/.agents/…but the native status checker and the bundled-skills uninstaller read~/agents/…(no dot), so badges never updated and Uninstall Skills removed nothing. Pointed the status checks, uninstaller, and shared-hooks catalog at the canonical dotted location. (Requires a native-sidebar rebuild; the shippednative-sidebar.jsis an untracked build artifact.)RemoteGxserverClient.swift): investigated the native remote-connect flow. Root cause: key-only machines forceBatchMode=yes, blocking the interactive password auth a terminal uses for cross-account local SSH. Made failures actionable —sanitizedProcessFailurenow maps ssh stderr to specific guidance (host-key change/verification, permission-denied → save the machine's SSH password or add an accepted key, connection-refused → enable Remote Login, DNS, unreachable, timeout). String-only change; the intentionalBatchModefast-fail is left as-is pending a maintainer decision.Detail:
contributions/issue-61-ssh-investigation.md3. Investigations documented (no code change)
backend.kqueue.Loop.tick, commit34fa5087…), not Ghostex/Ghostty source. Prepared an upstream-style bug report (versions, trace, workload, race hypothesis, repro attempt, recommended teardown-ordering audit).GxserverBar.app(reboot persistence, click-vs-menu, dev/prod isolation, preference source,ghostex://focus-session, floating-indicator split, security hardening) plus a suggested build order.Detail:
contributions/issue-52-libxev-kqueue-upstream-report.md,contributions/issue-38-menubar-agent-design-decisions.md4. Five additive hardening features
Each addresses a concrete failure from the engagement. All independently shippable.
ghostex doctor(doctor.rs,server.rs,sidebar/settings-modal.tsx,native/sidebar/modal-host.tsx): five read-only checks (skills, hooks, toolchain, daemon, T3), an invariant validator, and a CLI with--jsonthat runs in-process even when the daemon is down.POST /api/doctor/runandPOST /api/doctor/fix(FullLocal, auth-required); a Support tab with Run Doctor / Copy Diagnostics and a fix-confirmation flow.paths.rsAgentPaths,bin/generate-agent-paths.rs,scripts/generate-agent-paths.mjs,shared/agent-paths.generated.ts): one canonical Rust definition of agent paths, code-generated into checked-in TS constants consumed by the native sidebar; skills/hooks installers migrated to it. A drift test asserts the generated TS matches the Rust source. Structurally retires the Skills/Desktop Control always show "Not installed": installer writes ~/.agents/ but status checker reads ~/agents/ (no dot) #58 path-mismatch bug class.shared/gxserver-protocol-conformance.test.ts,protocol.rs,/api/capabilities): CI test asserting set-equality (both directions) between the shared TS contract and the Rust routing table; aRemoteAllowed/api/capabilitiesendpoint returning apath → implemented|notImplementedmap so clients can gate UI instead of rendering dead buttons. Directly addresses audit findings 5/6.handle_export_diagnostics_http): user-triggered "Copy Diagnostics" producing a redacted JSON bundle (version, config summary via export-time allowlist, last 50 error-level log entries, T3 status, skills summary). FullLocal, auth-required.subprocess_policy.rs):SubprocessProfilewith explicit per-use env allowlists plus awrite_secret_file(0600) helper;repository_clone.rsmigrated onto it. T3 env restriction deliberately deferred (it relies on thezsh -liclogin-shell env).Detail:
contributions/ghostex-hardening-features.md5. CodeQL alert fixes
Resolved 3 CodeQL alerts in app code (
native/sidebar/manage.tsx,native/sidebar/meo/helpers/images.ts,sidebar/session-card-content.tsx): escape backslashes before quotes in markdown quoting; validate URL protocol (http:/https:/data:image/) before assigningimg.src; require adata:image/prefix on favicon data URLs. Skips (vendored/test/dev) documented incodeql.txt.6. Follow-up hardening-pass audit fixes
A second security audit of the implemented features surfaced two defects, both fixed here:
SSH_AUTH_SOCKwas stripped from clone subprocesses: the subprocess-policy sensitive-key filter matched the substringAUTH, droppingSSH_AUTH_SOCKeven though it is in the clone allowlist — silently breaking ssh-agent auth forgit@clones. The allowlist now takes precedence over the sensitive filter, with a regression test asserting the socket survivessubprocess_environment(Clone).~/.agents/hookswhile the canonical installer path is~/.ghostex/hooks. Migrated those references to the generatedHOOKS_ROOTconstant and added a guard against hardcoded agent-path literals.Detail:
contributions/Ghostex-Security-Audit-Hardening-Pass.mdValidation
cargo buildclean;cargo test -p gxservergreen (482 tests; +2SSH_AUTH_SOCKregression tests from the follow-up fix), including theclone_environment_*,subprocess_policy,doctor, andAgentPathsdrift tests.ghostexHostchange is a string-only edit to one function;swiftc -parseclean (full Xcode build to be run by a maintainer).agent_hooks::…install_writes_notify_hook_without_payload_content) is environment-coupled (fails only whenqodercliis installed on the build machine); unrelated to these changes.Notes for reviewers
native-sidebar.jspicks up the source change.gpui(perAGENTS.md); no fallback-style patches — each fix corrects root cause.Out of scope / future work
write_secret_fileatomic-create hardening — all noted in the hardening-pass audit as low-severity follow-ups.Commits
f253650f— gxserver-rs: clone env allowlist + T3 loopback bind (security audit)6d9b46da— native/sidebar:.agentspath fix (Skills/Desktop Control always show "Not installed": installer writes ~/.agents/ but status checker reads ~/agents/ (no dot) #58)d29722c0— ghostexHost: actionable SSH connect errors (Fix SSH connections to another user account on the same Mac #61)3ebde44c— docs(contributions): changelog + Crash: null-unwrap panic in backend.kqueue.Loop.tick (io thread) — 4.21.4 / macOS 26.5.1 / M1 #52/Move macOS menu bar into a gxserver-owned agent (server control + status) #38 write-ups4e220f44— gxserver-rs: update t3_runtime test for loopback bind4e3c600a— feat(gxserver,sidebar): doctor diagnostics, shared paths, conformance/capabilities, diagnostic export, subprocess policyf8db15ba,2283a711— docs(contributions): hardening feature summariesa1ad720a— chore(deps): bump astro/starlight (docs site)024b6575— fix(security): resolve 3 CodeQL alerts in app codeb33ed397— fix(security): allowlist wins over sensitive filter (SSH_AUTH_SOCK) + hooks catalog usesHOOKS_ROOT(follow-up audit fixes)Summary by CodeRabbit
ghostex doctorhealth checks with optional automated fixes, plus doctor-related local API endpoints./api/capabilitiesto enumerate supported features.