Repository navigation
fix(browser): distinguish profile write-denial from contention - #919
Conversation
Signed-off-by: Lime <22442210+L1meSn0w@users.noreply.github.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (10)
✨ 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 |
|
Thanks for the contribution. This PR has been marked as an external contribution and routed for maintainer review. Before merge, please make sure:
GitHub Copilot code review may be requested as an advisory first pass, but maintainer approval is still required. |
ffroliva
left a comment
There was a problem hiding this comment.
Thanks for this — the diagnosis is right, and the real Windows ACL reproduction is exactly the kind of evidence we want. I reproduced it independently on an English Windows with installed Chrome (Lock file can not be created: Access is denied. (0x5), generic Error, unclassified on develop, caught by this branch). Note that bundled Chromium does not fail under the same deny ACL; only the chrome channel does.
Three changes before this can merge:
1. Match the error code, not the localized OS text (src/gflow_cli/api/client.py, _PROFILE_ACCESS_DENIED_MARKERS)
Chrome formats the line as Lock file can not be created: <localized system message> (0x5) — your own payload shows the localized part (拒绝访问。). Only the English and Chinese strings match today; the same failure on a German, Japanese, Portuguese or French Windows (Zugriff verweigert (0x5), Acesso negado. (0x5), …) falls back to the old classification. The locale-invariant part is the code:
return "ProcessSingleton" in message and ("(0x5)" in message or "Error code: 5" in message)Please drop the translated markers and add one test case with a payload that is neither English nor Chinese (e.g. Acesso negado. (0x5)). This follows the project's locale-invariance rule (AGENTS.md) in spirit.
2. Narrow the scope wording in CHANGELOG.md and docs/CONFIGURATION.md
Only FlowApiClient._launch_persistent_context is classified. Other direct launch_persistent_context sites still surface the raw error — api/transports/ui_automation.py, the auth paths (auth/real_chrome.py, auth/internal_chromium.py, auth/verification.py, auth/cookies.py) and the experimental transports. The current text ("a persistent-context launch … now raises") reads as universal, and the CONFIGURATION remediation ("authenticate a profile inside a writable GFLOW_CLI_HOME") points at auth login, which is not covered. Scoping the text to the client launch (generation/download runs) and listing the uncovered paths next to the existing CDP note is enough — no need to extend the classifier in this PR.
3. One remediation text, not two
ProfileAccessError has a single raise site, and it overrides _default_remediation with a longer variant, so the default is never shown. Please move the wording into _default_remediation and drop the remediation_hint= argument at the raise site (and adjust the "filesystem permissions" assertion).
Optional
- In the test,
from gflow_cli.json_output import exit_code_for→assert exit_code_for(error) == 11replaces the inlineEXIT_CODE_MAPloop; the separateisinstance(error, ConfigurationError)then becomes redundant. - The plain-
Errortest could fold into the parametrized one with the exception class as a second parameter. - A regression case pinning the ordering: Playwright's real header (
Failed to create a ProcessSingleton for your profile directory. This usually means that the profile is already in use…) without a(0x5)line, raised asTargetClosedError, should still becomeProfileLockedError. - A one-line comment on why the access check must run before
_is_target_closed. KNOWN_ISSUES.md(the "Same profile can't be used in parallel" entry) describes ProcessSingleton failures only as contention; one sentence pointing to the new CONFIGURATION section would help.
Merge logistics (maintainer side)
CI has not run on this fork PR yet (it is awaiting workflow approval), so SonarCloud will be checked once it does. #907 touches the same _launch_persistent_context boundary; whichever lands second rebases. Please mark the PR ready for review once the above is in.
Thanks again for the careful, well-scoped fix.
…ient launch Review follow-up on ffroliva#919. - Match ProcessSingleton + Win32 code 5 ("(0x5)" / "Error code: 5") instead of localized OS text, so the classification holds in every Windows display language (pt-BR and de-DE payloads previously fell through). - One remediation text: the class default; the single raise site no longer overrides it. - Tests: parametrize over language and exception shape, use exit_code_for, and pin that a ProcessSingleton header without code 5 is still contention. - CHANGELOG/CONFIGURATION: state that only the generation client's launch is classified; auth, standalone/experimental transports and CDP are not yet. KNOWN_ISSUES points ProcessSingleton + (0x5) at the new section. Signed-off-by: Flavio Oliva <ffroliva@gmail.com>
|
@L1meSn0w thank you — rather than send this back for another round, I've pushed the review follow-ups onto your branch (82013eb), keeping your original commit as-is:
Verified end to end on Windows with installed Chrome: a profile directory with a write-deny ACL launched through |
All three must-fixes addressed in 82013eb (locale-invariant match, single remediation text, scoped docs); CI green.
|
🎉 This is live in gflow-cli v0.81.0: https://github.com/ffroliva/gflow-cli/releases/tag/v0.81.0 A write-denied profile on Windows now raises |
Summary
Report Windows Chrome profile write-denial as a structured
ProfileAccessErrorinstead of suggesting profile contention or letting a generic Playwright launch
error escape classification.
The check requires both
ProcessSingletonand one of the known Windowsaccess-denied/error-code-5 messages. It runs before the existing target-closed
classification.
ProfileAccessErrorinheritsConfigurationError, so it usesthe existing configuration exit code 11 and carries a specific remediation:
grant write access to the complete profile directory or use a writable profile.
Other launch failures retain their existing behavior. There are no changes to
browser launch arguments, authenticated profile storage, lease lifetime,
concurrency, transport selection, or generation requests. No new lock-directory
setting is included.
Real Windows reproduction
Using installed Chrome 154.0.8037.92, an entirely new writable profile launched
successfully. A second, entirely new profile with a temporary write-deny ACL
failed before any page navigation. The Playwright error was a generic
Errorwith these sanitized fragments:
Replaying that actual failure payload through the pinned upstream launch handler
preserves the generic
Error; the candidate returnsProfileAccessError.The regression test also covers this generic-error shape, in addition to the
existing
TargetClosedErrorvariants. Both temporary profiles and the deny ACLwere removed. No signed-in browser profiles, credentials, Flow pages, or
generation calls were used. This is local browser-launch evidence, not a claim
of live Google Flow verification.
Validation
srcPyright: 0 errors using the worktree interpreter.src/testsRuff lint and format: passed.website mirror, and council-memory consistency checks: passed.
The final full-suite command was:
The test environment used an isolated home/profile root and no production
credentials. The default marker filter excludes live, e2e, smoke, and container
tests. The explicit scratch root keeps concurrent test invocations separate.
Lifecycle and scope
classification, demonstrated with synthetic and actual Windows payloads.
fix; no transport, auth, DOM selector, wire schema, command, or option is added.
Positive/negative cases are in
tests/api/test_concurrency.py.continue to share the
GFlowErrorcontract. Parity/server suites passed.startup before any Flow surface is reached. The real local ACL reproduction is
described above.
review remains outstanding. This is intentionally a draft, not merge-ready.
Interaction with #907
PR #907 modifies the same
_launch_persistent_contextboundary and introducesan opt-in
launch_via_cdp()path outside the existingtry/except. This PR onlycovers the existing Playwright persistent-context branch. It does not claim
CDP coverage; reconcile the base and intended coverage with #907 before merging.
Contribution checklist
develop.Signed-off-by:trailer.and private captured material; none are included.
generated published documentation were reviewed together.
中文说明
摘要
将 Windows Chrome profile 拒绝写入报告为结构化
ProfileAccessError,避免提示 profile 被另一浏览器占用,或让普通 Playwright 启动错误绕过分类。
只有同时命中
ProcessSingleton和已列出的 Windows 拒写/error-code-5标记才采用新分类,且在原 TargetClosed 分类之前处理。新错误继承
ConfigurationError,沿用退出码 11,提示授予完整 profile 目录的写权限或改用可写 profile。
其他启动失败保持原行为;不改启动参数、认证 profile 存储、租约生命周期、
并发、传输选择或生成请求。不包含新的锁目录配置。
Windows 实机复现
Chrome 154.0.8037.92 可以启动全新的可写 profile;另一全新 profile
施加临时拒写 ACL 后,在页面导航前启动失败。Playwright 返回普通
Error,错误载荷包含 ProcessSingleton 与
Lock file can not be created: 拒绝访问。 (0x5)。将这个真实失败载荷回放到固定上游启动处理器,仍得到普通
Error;候选得到ProfileAccessError。回归用例覆盖普通错误与 TargetClosedError 两种形态。临时 profile 和拒写 ACL 均已移除;未使用已登录 profile、凭证、Flow 页面或
生成请求。这是本地浏览器启动证据,不是 Google Flow 实盘验收声明。
验证
src做 Pyright:0 errors。测试使用隔离 home/profile 根目录,没有生产凭证;默认标记排除了 live、e2e、
smoke 与容器测试。最终全量命令使用独立临时目录,避免多个 pytest 共享临时目录;
命令列在英文部分。
生命周期与边界
不适用新功能的 predict/scenario-plan;正反例位于并发测试文件。
对称性与 server 测试通过。
本地 Windows ACL 复现如上。
不宣称达到合并条件。
与 #907 的关系
#907 修改同一启动边界,并在现有 try/except 外新增 opt-in CDP 启动路径。
本 PR 只覆盖现有 Playwright persistent-context 分支,不声称覆盖 CDP;
合并前需要协调基线与覆盖范围。
提交核查
目标分支为 develop;提交使用公开 GitHub 身份与 noreply 邮箱,并带
Signed-off-by。差异已检查,不包含凭证、内部路径、机器名、账号数据或私有
抓包。源码、回归用例、错误/退出码文档、更新日志与生成的网站镜像均已自查。