Skip to content

fix(browser): distinguish profile write-denial from contention - #919

Merged
ffroliva merged 2 commits into
ffroliva:developfrom
L1meSn0w:bugfix/profile-access-denied
Sep 30, 2026
Merged

ffroliva merged 2 commits into
ffroliva:developfrom
L1meSn0w:bugfix/profile-access-denied

Conversation

@L1meSn0w

Copy link
Copy Markdown
Contributor

Summary

Report Windows Chrome profile write-denial as a structured ProfileAccessError
instead of suggesting profile contention or letting a generic Playwright launch
error escape classification.

The check requires both ProcessSingleton and one of the known Windows
access-denied/error-code-5 messages. It runs before the existing target-closed
classification. ProfileAccessError inherits ConfigurationError, so it uses
the 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 Error
with these sanitized fragments:

BrowserType.launch_persistent_context: Failed to create a ProcessSingleton for your profile directory.
Lock file can not be created: 拒绝访问。 (0x5)

Replaying that actual failure payload through the pinned upstream launch handler
preserves the generic Error; the candidate returns ProfileAccessError.
The regression test also covers this generic-error shape, in addition to the
existing TargetClosedError variants. Both temporary profiles and the deny ACL
were 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

  • Full offline pytest suite: 4655 passed, 30 skipped, coverage 92.49%.
  • Focused launch/error/MCP-parity/documentation suite: 211 passed.
  • Full src Pyright: 0 errors using the worktree interpreter.
  • Full src/tests Ruff lint and format: passed.
  • Repository hygiene, documentation links, published-doc privacy, generated
    website mirror, and council-memory consistency checks: passed.

The final full-suite command was:

COLUMNS=320 python -m pytest -q -n 4 --basetemp=tmp/pytest-publication-full \
  --cov=gflow_cli --cov-fail-under=80

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

  • Issue triage: no separate GitHub issue; root cause is the launch-error
    classification, demonstrated with synthetic and actual Windows payloads.
  • Predict / new-feature scenario-plan: not applicable to this narrow diagnostic
    fix; no transport, auth, DOM selector, wire schema, command, or option is added.
    Positive/negative cases are in tests/api/test_concurrency.py.
  • Local quality checks and the CLI/MCP surface-mirror sweep completed.
  • MCP parity: no new tool or request field; existing CLI and MCP error handling
    continue to share the GFlowError contract. Parity/server suites passed.
  • Flow E2E / live generation: not applicable; this error occurs at local browser
    startup before any Flow surface is reached. The real local ACL reproduction is
    described above.
  • Independent multi-reviewer council review has not been run; maintainer
    review remains outstanding. This is intentionally a draft, not merge-ready.

Interaction with #907

PR #907 modifies the same _launch_persistent_context boundary and introduces
an opt-in launch_via_cdp() path outside the existing try/except. This PR only
covers the existing Playwright persistent-context branch. It does not claim
CDP coverage; reconcile the base and intended coverage with #907 before merging.

Contribution checklist

  • Targets develop.
  • Commit uses the contributor's public GitHub identity and noreply email.
  • Commit includes a Signed-off-by: trailer.
  • Diff reviewed for secrets, internal paths, machine names, account data,
    and private captured material; none are included.
  • Source, regression cases, error/exit-code documentation, changelog, and
    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 实盘验收声明。

验证

  • 全量离线 pytest:4655 passed,30 skipped,覆盖率 92.49%。
  • 启动/错误/MCP 对称性/文档聚焦回归:211 passed。
  • 使用 worktree 解释器,对完整 src 做 Pyright:0 errors。
  • 全仓源码/测试 Ruff lint、format 通过。
  • 仓库卫生、文档链接、网站私有标识、生成镜像与 council-memory 一致性检查通过。

测试使用隔离 home/profile 根目录,没有生产凭证;默认标记排除了 live、e2e、
smoke 与容器测试。最终全量命令使用独立临时目录,避免多个 pytest 共享临时目录;
命令列在英文部分。

生命周期与边界

  • 没有另建 GitHub issue;根因是启动错误分类,已有模拟和真实 Windows 载荷证据。
  • 这是诊断修正,不新增传输、认证、DOM 选择器、wire schema、命令或选项,
    不适用新功能的 predict/scenario-plan;正反例位于并发测试文件。
  • 已完成本地质量门与 CLI/MCP 镜像范围核查。
  • 没有新 MCP 工具或请求字段;CLI/MCP 沿用共同的 GFlowError 契约,
    对称性与 server 测试通过。
  • 未跑 Flow E2E/生成实盘:该错误发生于本地启动阶段,尚未触及任何 Flow 页面。
    本地 Windows ACL 复现如上。
  • 未运行独立多审查者 council,仍需维护者审查;本 PR 保持 Draft,
    不宣称达到合并条件。

与 #907 的关系

#907 修改同一启动边界,并在现有 try/except 外新增 opt-in CDP 启动路径。
本 PR 只覆盖现有 Playwright persistent-context 分支,不声称覆盖 CDP;
合并前需要协调基线与覆盖范围。

提交核查

目标分支为 develop;提交使用公开 GitHub 身份与 noreply 邮箱,并带
Signed-off-by。差异已检查,不包含凭证、内部路径、机器名、账号数据或私有
抓包。源码、回归用例、错误/退出码文档、更新日志与生成的网站镜像均已自查。

Signed-off-by: Lime <22442210+L1meSn0w@users.noreply.github.com>
@coderabbitai

coderabbitai Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: d3620e5f-f82d-4da0-b5c2-65c129172813

📥 Commits

Reviewing files that changed from the base of the PR and between 488dcc1 and 82013eb.

📒 Files selected for processing (10)
  • CHANGELOG.md
  • KNOWN_ISSUES.md
  • docs/CONFIGURATION.md
  • docs/USAGE.md
  • src/gflow_cli/api/client.py
  • src/gflow_cli/errors.py
  • tests/api/test_concurrency.py
  • website/docs/CONFIGURATION.md
  • website/docs/KNOWN_ISSUES.md
  • website/docs/USAGE.md
 _________________________________________________
< Undefined behavior is my favorite horror genre. >
 -------------------------------------------------
  \
   \   \
        \ /\
        ( )
      .( o ).
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


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.

❤️ Share

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

@github-actions github-actions Bot added external-contribution Pull request from a non-maintainer or fork needs-copilot-review Ask GitHub Copilot for advisory code review needs-maintainer-review Requires maintainer review before merge labels Sep 30, 2026
@github-actions
github-actions Bot requested a review from ffroliva September 30, 2026 06:24
@github-actions

Copy link
Copy Markdown

Thanks for the contribution. This PR has been marked as an external contribution and routed for maintainer review.

Before merge, please make sure:

  • The PR targets develop.
  • Commits use a real Git identity or GitHub noreply email.
  • External commits include a DCO sign-off: git commit -s.
  • The PR does not include secrets, cookies, account tokens, signed URLs, or private captured data.
  • Focused tests/docs are included for behavior changes.

GitHub Copilot code review may be requested as an advisory first pass, but maintainer approval is still required.

@ffroliva ffroliva left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

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) == 11 replaces the inline EXIT_CODE_MAP loop; the separate isinstance(error, ConfigurationError) then becomes redundant.
  • The plain-Error test 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 as TargetClosedError, should still become ProfileLockedError.
  • 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>
@ffroliva

Copy link
Copy Markdown
Owner

@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:

  • Locale-invariant match: ProcessSingleton + (0x5) / Error code: 5, no localized text. Tests now cover English, Chinese, Portuguese and German payloads, each as both TargetClosedError and a generic Error.
  • Single remediation text: moved into ProfileAccessError._default_remediation; the raise site no longer overrides it.
  • Ordering pinned: Playwright's real "already in use" header without a code-5 line still becomes ProfileLockedError.
  • Docs scoped: CHANGELOG and CONFIGURATION now say only the generation client's launch is classified (auth, standalone/experimental transports and CDP are not yet), and KNOWN_ISSUES points ProcessSingleton + (0x5) at the new section. You're credited in the CHANGELOG entry.

Verified end to end on Windows with installed Chrome: a profile directory with a write-deny ACL launched through FlowApiClient now raises ProfileAccessError (exit 11) from Playwright's generic Error. Local gates are green (tests/api + tests/mcp: 2064 passed; ruff, format, pyright, doc gates clean).

@github-actions
github-actions Bot requested a review from ffroliva September 30, 2026 13:34
@ffroliva
ffroliva dismissed their stale review September 30, 2026 13:46

All three must-fixes addressed in 82013eb (locale-invariant match, single remediation text, scoped docs); CI green.

@ffroliva
ffroliva marked this pull request as ready for review September 30, 2026 15:18
@ffroliva
ffroliva merged commit 6200339 into ffroliva:develop Sep 30, 2026
17 checks passed
@ffroliva

Copy link
Copy Markdown
Owner

🎉 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 ProfileAccessError (exit 11) with a permissions remedy, instead of posing as lock contention. It covers the generation client's launch, as scoped in the changelog, where you're credited. Thanks, @L1meSn0w!

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

Labels

external-contribution Pull request from a non-maintainer or fork needs-copilot-review Ask GitHub Copilot for advisory code review needs-maintainer-review Requires maintainer review before merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants