From 8691aa0e5d4a175f3ad39b8fb718ff2da2a70138 Mon Sep 17 00:00:00 2001 From: vczh Date: Mon, 3 Aug 2026 19:00:09 -0700 Subject: [PATCH] Sync coding agent context --- .github/Guidelines/Coding.md | 19 +++++++++++++++++- .github/Guidelines/Running-GacUI.md | 20 ++++++++++++++----- .github/KnowledgeBase/Index_GacUI.md | 2 +- ..._RemoteProtocolRendererAndSerialization.md | 20 +++++++++---------- .github/KnowledgeBase/Learning.md | 4 +++- 5 files changed, 47 insertions(+), 18 deletions(-) diff --git a/.github/Guidelines/Coding.md b/.github/Guidelines/Coding.md index b4386c59..a477bba0 100644 --- a/.github/Guidelines/Coding.md +++ b/.github/Guidelines/Coding.md @@ -16,13 +16,23 @@ In general, here is my preference for any languages: ## Be Brave Enough to Fix Upstream Code and Make Breaking Change +This section is a high level philosophy of trade-offs during making decision of where to fix the code. +It looks amgibuous, that actually means you are expected to consider the context of each issues you are facing to. + - Always fix the bug at its root cause. - If you find any API that doesn't work, fix instead of making a replacement. Even when the API is in an upstream repo, prefer fixing in the upstream repo and releasing it to the current repo. - DO NOT concern about making breaking change. - The project is well covered by unit test, any unexpecting breaking change is highly possibly to be cought. - If such breaking change is intented, unit test could help you perform complete refactoring. - If an API design does not fit the requirement or contract, just change it. -- Interface design could have been wrong, DO NOT implement twisted logic just to finish the current task while fitting the wrong design. +- Interface design could have been wrong, DO NOT implement twisted logic just to finish the current task while fitting the wrong design. +- But DO NOT leaks information from downstream to upstream repos. + - Each upstream repo has its own scope, contract, policy, strategy, etc. + - Each upstream repo releases libraries that is supposed to serve broader purposes. + - Keep interfaces between repos clean. + - If downstream repos need to need to do something depending on upstream repos' non-public information: + - If the issue is the upstream repo itself, fix the upstream repo. + - Otherwise, expose informations elegantly so that downstream repos could process and react to them properly. ## C++ Thread Safety and Multi-Threading Synchronization @@ -30,6 +40,13 @@ Check out [Coding_MultiThreading.md](./Coding_MultiThreading.md). ## C++ Coding Convention +- Anonymouse namespace `namespace{}` is not welcomed: + - DO NOT generate such construction. + - When you edit any existing code and see this, remove that anonymouse namespace and fix the indentation of the content. + - The reason is that, moust of cpp files will be merged into one single file apon release, all benefits are gone meanwhile the code looks messy. +- Impl classes like `class Something { class Impl; Ptr impl; }` is not always welcomed: + - Hiding information or improve building performance with this pattern is considered incorrect here. + - The only exception is forcing to hide platform dependend constructions, like `vl::Mutex`. - Although C++ does not require this but we want to have `extern` on all function forward declarations. - In general we don't use `inline` in header files unless such function is performance critical, e.g. very simple comparison operators. - Rules for C++ header files: diff --git a/.github/Guidelines/Running-GacUI.md b/.github/Guidelines/Running-GacUI.md index 026ad87a..1c8630ee 100644 --- a/.github/Guidelines/Running-GacUI.md +++ b/.github/Guidelines/Running-GacUI.md @@ -11,14 +11,12 @@ When the model `gpt-5.3-codex-spark` is available: - If the target application has bugs, it is normal that steps can't be performed to the end. The sub agent should summarize what is going wrong to prevent steps to be done. - This model is fast, it significantly improves the performance of operating the GUI. -## Windows Specific - - GacUI applications could end up in dead loop or dead locks, so DO NOT JUST wait for the process to exit. - When it is crashed, sometimes (but not always) a native dialog would show and block the process. - Native dialogs could be proactivately called from a GacUI application, even when `FakeDialogService` is not used. - If you believe the processing is blocked or is running too long, you are going to check out `Running-ComputerUse.md` and deal with it. -### Automation Service via HTTP +## Automation Service via HTTP This is a very useful way for coding agent to debug GacUI applications. Computer use via UI Automation may not work when the computer screen is locked. @@ -36,6 +34,7 @@ When `StartWindowsHttpAutomationService` is used during startup up a GacUI appli - GET `.../Dom`, for remote protocol renderer, exposing the DOM tree. - Read comment for DumpRemoteProtocolRenderingDom` for the schema. - POST `.../IO` or `IO/` + - Set `Content-Type` to exactly `application/json; charset=utf8`. The Windows HTTP implementation validates this value before reading the UTF-8 command body. - IRead comment for `RunIOCommandOnNativeWindow` for the schema. - `` is the window id returning from `.../Controls`. - The window ID can be comitted for the main window. @@ -58,10 +57,21 @@ GacUI does not support UI Automation so far, but this situation will be changed UI Automation does not work when the screen is locked. Calling any UIA tools in this case will just fail. +## Windows Specific + +- While polling automation endpoints or waiting for application processes, repeatedly inspect the target processes for a top-level window titled exactly `Microsoft Visual C++ Runtime Library`. + - Treat this window as a blocking crash signal immediately. Do not keep retrying the application-level endpoint, because the modal dialog can block the UI thread and make a crash look like an ordinary timeout. + - Capture the dialog text and buttons with the Win32 procedure in `Running-ComputerUse.md`, dismiss it deliberately, and record the process exit code. + - Check again after every automation timeout and before declaring a run successful. + ## Linux Specific -(to be editing...) +When maintaining the `vczh-libraries` github organization: +- Only `wGac` repo runs actual GacUI application on Linux. +- `GacUI` test apps only work when they are unit test, CLI or GacUI remote protocol core application (which is also CLI but with automation service enabled). ## macOS Specific -(to be editing...) +When maintaining the `vczh-libraries` github organization: +- Only `iGac` repo runs actual GacUI application on macOS. +- `GacUI` test apps only work when they are unit test, CLI or GacUI remote protocol core application (which is also CLI but with automation service enabled). diff --git a/.github/KnowledgeBase/Index_GacUI.md b/.github/KnowledgeBase/Index_GacUI.md index 6ffb04db..81920e3e 100644 --- a/.github/KnowledgeBase/Index_GacUI.md +++ b/.github/KnowledgeBase/Index_GacUI.md @@ -147,6 +147,6 @@ Testing GacUI applications without real OS windows or rendering, using the remot - Layered channel architecture for protocol serialization: `IGuiRemoteProtocol` is bridged by `GuiRemoteProtocolCoreChannel`/`GuiRemoteProtocolRendererChannel` over `IJsonChannel` packages, with network clients/servers using `glr::json::JsonNodeListSerializer`. - JSON envelope format with `semantic`, `id`, `name`, `arguments` fields; protocol types code-generated from `Protocol/*.txt` with `JsonHelper` specializations. - `GuiRemoteProtocolAsyncJsonChannel` and `GuiRemoteProtocolAsyncJsonChannelRenderer` provide async channel separation with queued events/responses, connection-safe request matching, renderer main-thread dispatch, and startup message caching. -- Demo project pair (`RemotingTest_Core` and `RemotingTest_Rendering_Win32`) demonstrates full protocol stack assembly for both core and renderer sides with named-pipe/HTTP transport. +- Demo project pair (`RemotingTest_Core` and `RemotingTest_Rendering_Win32`) demonstrates full protocol stack assembly for both core and renderer sides with named-pipe, Windows HTTP, and portable MiniHTTP transport. [Design Explanation](./KB_GacUI_Design_RemoteProtocolRendererAndSerialization.md) diff --git a/.github/KnowledgeBase/KB_GacUI_Design_RemoteProtocolRendererAndSerialization.md b/.github/KnowledgeBase/KB_GacUI_Design_RemoteProtocolRendererAndSerialization.md index 2d94103a..9d1d15cb 100644 --- a/.github/KnowledgeBase/KB_GacUI_Design_RemoteProtocolRendererAndSerialization.md +++ b/.github/KnowledgeBase/KB_GacUI_Design_RemoteProtocolRendererAndSerialization.md @@ -127,29 +127,29 @@ Two projects in `Test/GacUISrc/` demonstrate a full remote protocol deployment. Located at `Test/GacUISrc/RemotingTest_Core/`. Accepts `/Pipe` or `/Http` arguments to start either a named-pipe server or HTTP server. -**Protocol stack setup** (`StartServer` in `GuiMain.cpp`): -1. Creates `NamedPipeRemotingChannelServer` or `HttpRemotingChannelServer`, both derived from `RemotingChannelServerBase` and ultimately `GuiRemoteProtocolNetworkChannelServer`. -2. Starts the server, creates a local core client with `GuiRemoteProtocolLocalChannelClient`, and connects it to the server. -3. Waits for the first non-local renderer client with a manual-reset `EventObject` signaled from `OnClientConnected`. -4. Wraps the core client's protocol channel in `GuiRemoteProtocolAsyncJsonChannel`. -5. Creates `GuiRemoteProtocolCoreChannel`, then builds `GuiRemoteProtocolFilter` -> `GuiRemoteProtocolDomDiffConverter` -> `SetupRemoteNativeController`. -6. On shutdown, disconnects the named-pipe renderer client when needed, clears core channel/server pointers, and stops the channel server before stack-owned wrappers are destroyed. +**Protocol stack setup** (`StartServer` in `GuiMain.cpp`): +1. `StartNamedPipeServer`, `StartHttpServer`, or `StartMiniHttpServer` constructs the matching `RemotingChannelServer`. `/RVMT` constructs `RemoteViewModelChannelServer`, which adds exact RVM-host admission while preserving the base renderer behavior. +2. The shared run function starts the server, creates a local core client with `GuiRemoteProtocolLocalChannelClient`, and connects it to the server. Local clients are accepted directly because only the owning process can call `ConnectLocalClient`. +3. It wraps the core client's protocol channel in `GuiRemoteProtocolAsyncJsonChannel` and creates `SwitchableRenderersCoreChannel`. This `GuiRemoteProtocolCoreChannel` subclass verifies at submission time that its protocol renderer is still the transport server's current renderer. `Submit` evaluates this virtual guard before applying the base channel's default no-renderer behavior, so the switchable channel can report disconnection for both a stale renderer and renderer id `-1` while the default channel still treats `-1` as an idle state. +4. It builds `GuiRemoteProtocolFilter` -> `GuiRemoteProtocolDomDiffConverter` -> `SetupRemoteNativeController`. The Core remains usable before a renderer connects. +5. In `/RVMT`, the shared server owns the requester session. The exact RVM host is admitted before service acquisition, while renderer admission remains closed until the requester enters its running phase after the window is constructed. +6. On shutdown, Core clears the server's stored JSON/protocol channel pointers, finalizes the optional requester session, and stops the channel server before stack-owned wrappers are destroyed. -`RemotingChannelServerBase::OnClientConnected` accepts replacement renderers. If a different renderer is already current, it calls `GuiRemoteProtocolCoreChannel::DetachRenderer(oldClientId)`, tries to send a raw `ControllerConnectionStopped` package to the old renderer, and disconnects the old transport only when notification fails. Fatal exceptions from `GuiMain()` are sent to connected clients via `BroadcastError(...)` before the error is printed. +`RemotingChannelServer::OnRemoteClientConnected` accepts replacement renderers. If a different renderer is already current, it calls `GuiRemoteProtocolCoreChannel::DetachRenderer(oldClientId)`, tries to send a raw `ControllerConnectionStopped` package to the old renderer, and disconnects the old transport only when notification fails. `RemoteViewModelChannelServer` overrides this single remote-admission operation to accept the RVM host and to check the concrete requester phase before delegating renderer admission. If the RVM host disconnects, it broadcasts the business error when renderers are enabled and then terminates the requester. ### RemotingTest_Rendering_Win32 (Windows Application) Located at `Test/GacUISrc/RemotingTest_Rendering_Win32/`. Accepts `/Pipe` or `/Http` arguments to start as a named-pipe or HTTP client. **Protocol stack setup** (`StartClient` in `GuiMain.cpp`; this function is not a template): -1. Receives a named-pipe or HTTP `INetworkProtocolClient` and creates `RemotingTestChannelClient`, derived from `GuiRemoteProtocolChannelClient`, over it. +1. Receives a named-pipe, Windows HTTP, or MiniHTTP `INetworkProtocolClient` and creates the shared `RemotingChannelClient`, derived from `GuiRemoteProtocolChannelClient`, over it. 2. Creates `GuiRemoteProtocolAsyncJsonChannelRenderer` over the client's protocol channel. 3. Creates `GuiRemoteRendererSingle` and `GuiRemoteProtocolRendererChannel(&asyncRendererChannel, &remoteRenderer)`. 4. Waits for the server, then calls `SetupRawWindowsDirect2DRenderer()` to run the native window event loop. 5. In `GuiMain()`, creates the native window, registers it with `GuiRemoteRendererSingle`, creates a retained `Ptr`, installs it through `asyncChannel->SetInvokeInMainThread(invoker)`, drains startup work with `ProcessPendingMessages()`, and runs the window service. 6. On exit, clears the invoker, unregisters the main window, stops the network connection, and clears stack-owned renderer/channel pointers. -`RemotingTestChannelClient` queues both protocol packages and terminal actions through the async renderer's ordered main-thread FIFO. A Core-authored `!Error` arrives through `OnReadError`, claims the first fatal error, and opens the native Yes/No prompt. Choosing Yes calls `ForceExitByFatelError()`; choosing No calls `RetainByFatalError(message)`, keeps the native renderer window open with a `[STOPPED]` title and fatal overlay, and exposes the error through renderer automation. A fatal local channel error has different UI semantics: after VlppOS's `IChannelClient` promotes a post-connection protocol error, `OnLocalError(..., true)` queues the ordinary disconnected transition directly, without showing a fatal prompt and without waiting for `OnDisconnected`. `OnDisconnected` queues the same idempotent transition when it is delivered. FIFO ordering lets an earlier `ControllerConnectionStopped` or Core `!Error` win before detach. +`RemotingChannelClient` queues both protocol packages and terminal actions through the async renderer's ordered main-thread FIFO. A Core-authored `!Error` arrives through `OnReadError`, claims the first fatal error, and opens the native Yes/No prompt. Choosing Yes calls `ForceExitByFatelError()`; choosing No calls `RetainByFatalError(message)`, keeps the native renderer window open with a `[STOPPED]` title and fatal overlay, and exposes the error through renderer automation. A fatal local channel error has different UI semantics: after VlppOS's `IChannelClient` promotes a post-connection protocol error, `OnLocalError(..., true)` queues the ordinary disconnected transition directly, without showing a fatal prompt and without waiting for `OnDisconnected`. `OnDisconnected` queues the same idempotent transition when it is delivered. FIFO ordering lets an earlier `ControllerConnectionStopped` or Core `!Error` win before detach. ### Protocol Stack Direction diff --git a/.github/KnowledgeBase/Learning.md b/.github/KnowledgeBase/Learning.md index 46451cdd..f6a178e3 100644 --- a/.github/KnowledgeBase/Learning.md +++ b/.github/KnowledgeBase/Learning.md @@ -9,8 +9,8 @@ - Proactively remove code made redundant by refactoring [12] - Keep design documentation aligned with code after refactoring [10] - Fix behavior at the owning state instead of patching symptoms [10] +- Extract abstractions only for real shared behavior [8] - Verify and localize portability on every target OS [7] -- Extract abstractions only for real shared behavior [7] - Make `Stop()` drain asynchronous work before returning [6] - Validate expectations against implementation and existing tests [5] - Use `WString::IndexOf` with `wchar_t` (not `const wchar_t*`) [4] @@ -102,6 +102,8 @@ For role-specific implementations, extract a base that contains only truly share When several platform implementations must satisfy the same behavioral contract, define and register each scenario once in a platform-neutral runner and parameterize only the concrete types and genuine platform seams. Duplicating test registrations per platform invites feature drift. +Do not turn fixed owner decisions or concrete state observations into callbacks or virtual policy. If the owning process controls whether an operation is called, enforce that choice at the call site; if a predicate only reports internal state, keep it nonvirtual. Reserve virtual functions for behavior that derived implementations genuinely need to change. + ## Make `Stop()` drain asynchronous work before returning If an API exposes `Stop()`, callers should be able to rely on it as the shutdown boundary: after it returns, no pending action, wait callback, overlapped I/O, or async completion should still touch the object. Do not paper over a broken `Stop()` with sleeps in tests; fix the stop path.