Skip to content

docs(architecture): audit Sandbox and privacy group against implementation - #4800

Merged
Astro-Han merged 5 commits into
apache:mainfrom
ggbdpq:docs/audit-sandbox-privacy-a6
Sep 7, 2026
Merged

docs(architecture): audit Sandbox and privacy group against implementation#4800
Astro-Han merged 5 commits into
apache:mainfrom
ggbdpq:docs/audit-sandbox-privacy-a6

Conversation

@ggbdpq

@ggbdpq ggbdpq commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Summary

Audits the Sandbox and privacy group (4 documents) from #3522 against the current implementation (22715e8).

  • docs/workspace-privacy-context.mdone drift fixed: the authority section named getWorkspacePrivacyContext() / defaultWorkspacePrivacyContext() in apps/desktop/src/main/main.ts. Neither function exists any more and main.ts carries no privacy logic. The effective state is now the Runtime Host runtime policy: packages/core/src/runtime-policy.ts declares the readonly privacy.incognitoActive (default false, line 255) with patches validated by the policy codec's privacy patch rule; desktop surfaces resolve it via queryRuntimePolicy() (runtime-host-search-ipc-main.ts:55); core boundaries validate the projection with the unchanged validateWorkspacePrivacyContext() (incognito.ts:34, consumed by thread-search.ts:206). The interface shape and validator contract were already accurate and are untouched.
  • docs/architecture/windows-sandbox-rfc-v1.md + .zh-CN.md — no drift. The pair is in lockstep (twelve sections, five guarantee subsections each, 1:1). Status claims hold: feat(windows): add brokered AppContainer sandbox support #2961 merged 2026-08-17 as documented, the AppContainer enforcement lives in the filesystem-worker launch specs, and the contract constants (SANDBOX_BOUNDARY_*, MAX_EXECUTION_BOUNDARY_SERIALIZED_BYTES) exist in packages/core/src/sandbox-boundary.ts.
  • docs/permission-onboarding-plan.md — no drift. Stage 1 files exist as named (apps/desktop/src/main/permission-overlay/permission-overlay-controller.ts, the src/overlay/permission-overlay.* shell), permissions-actions.ts still deep-links into System Settings, and tracking issue tracking(permissions): finish macOS Stage 2 #4328 is open.

All four documents gained the standard frontmatter block with last_verified. The RFC pair keeps translation_status: synced; the other two are source-only.

Verification

Claim Command / route Result
Removed privacy functions repo-wide grep WorkspacePrivacy only the type + validator remain (incognito.ts); nothing defines the two named functions
New authority chain grep runtime-policy.ts, policy-codec.ts, runtime-host-search-ipc-main.ts default false, readonly policy field, queryRuntimePolicy() projection all present
RFC/zh lockstep heading diff of both files 12 sections + 5 subsections, 1:1
#2961 status PR API merged 2026-08-17
Sandbox contract symbols packages/core/src/sandbox-boundary.ts exports present
Onboarding paths ls the two directories, issue #4328 API all as documented; issue open
Format / headers npm run format:check / npm run check:asf-headers exit 0 / changed files pass

AI use

Prepared with ZCode (GLM-5.3-Flash): read all four documents, traced each checkable claim to commits, files, and line numbers, and drafted the authority-section correction. The commit carries the Generated-by trailer.

Checklist

  • One subsystem group only (Sandbox and privacy, 4 documents)
  • Refs #3522 in the commit message
  • Paired translations move together (RFC pair both touched, translation_status: synced)
  • last_verified set to the audit date (2026-09-04)
  • Docs only, no code changes

@github-actions github-actions Bot added the effort/S Under 100 readable lines label Sep 4, 2026
…ation

Audit the four Sandbox and privacy documents from the apache#3522
documentation audit against the current implementation (22715e8).

workspace-privacy-context.md: one drift fixed — the authority section
still named `getWorkspacePrivacyContext()` / `defaultWorkspacePrivacyContext()`
in `apps/desktop/src/main/main.ts`; neither function exists any more
and `main.ts` carries no privacy logic. The effective state is the
Runtime Host runtime policy (`runtime-policy.ts` declares the readonly
`privacy.incognitoActive` and defaults it to false, patched through the
policy codec), desktop surfaces resolve it via `queryRuntimePolicy()`,
and core boundaries validate the projection with the unchanged
`validateWorkspacePrivacyContext()`. The section and the consumer
inventory sentence now describe that chain.

windows-sandbox-rfc-v1.md + zh-CN: no drift. The pair stays in lockstep
(twelve sections and five guarantee subsections 1:1); the status claims
hold (apache#2961 merged 2026-08-17, the AppContainer filesystem-worker
enforcement on the tree, the contract constants in
`packages/core/src/sandbox-boundary.ts`). Frontmatter added to both.

permission-onboarding-plan.md: no drift. Stage 1 files exist as named
(`permission-overlay-controller.ts` and the `src/overlay/` shell),
`permissions-actions.ts` still deep-links, and tracking issue apache#4328 is
open. Frontmatter added.

All four documents gained the standard frontmatter block with
`last_verified` (the RFC pair keeps `translation_status: synced`).

Refs apache#3522

Generated-by: GLM-5.3-Flash (ZCode)
@ggbdpq
ggbdpq force-pushed the docs/audit-sandbox-privacy-a6 branch from 648779a to 835b727 Compare September 4, 2026 22:48
@ggbdpq ggbdpq changed the title docs: audit Sandbox and privacy group against implementation docs(architecture): audit Sandbox and privacy group against implementation Sep 4, 2026

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed exact head 835b727856cf193c165416e869626a41bc9d8f8f.

Actual change: adds current-status metadata to the Windows sandbox RFC pair and permission-onboarding plan, and rewrites the workspace-privacy authority section from the removed Desktop helpers to Runtime Host policy. I checked the four documents against the exact-head code paths, the translated RFC structure, current main, and the hosted check.

Validation: hosted exact-head test, ASF header audit, focused Biome formatting, and git diff --check pass; the current-main synthetic merge tree is clean. The Windows sandbox symbols/preview scope and Stage 1 permission-overlay paths are present as documented. I found one live privacy-authority split inline.

I did not run native Windows AppContainer or macOS permission-overlay smoke tests, and I did not independently verify the open tracker state beyond the repository implementation.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

Comment thread docs/workspace-privacy-context.md

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-reviewed exact head 4c9a102c639f42ca44b86ebb4f92fb2b86872d53 after the merge from main.

The existing P1 privacy-authority finding remains. docs/workspace-privacy-context.md:42-58 declares Runtime Host policy as the effective authority and says Desktop consumers resolve that policy. The settings path does send privacy only to Runtime Host policy (runtime-host-settings-ipc-main.ts:373-374), and clientOwnedSettingsPatch() still omits privacy (settings-ownership.ts:55-64). However, notifications-ipc-main.ts:57-76 still reads settingsStore.get().privacy.incognitoActive and may render the session title and reply preview using that stale local value. A direct projection probe confirmed a patch containing both privacy and notifications retains only notifications in the local settings patch.

The merge commit did not change the PR documents or any cited authority/consumer file relative to the previously reviewed commit. The existing inline thread remains current and is not duplicated here. I found no additional P0-P3 issue in the Windows sandbox or permission-onboarding documents.

Validation: hosted test passed on this exact head; repository format, ASF headers, and git diff --check passed; notification policy and settings ownership tests passed; Runtime Host settings tests passed 12/12 after building the byte-identical Runtime Host dependency; and current main produced a clean synthetic merge tree identical to the reviewed tree. I did not run native Windows AppContainer or macOS permission-overlay smoke tests.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

… authority

Review (hqhq1025) found the authority migration claim incomplete:
notifications-ipc-main.ts still reads privacy.incognitoActive from the local
settings store, which never receives privacy patches (settings-ownership.ts
excludes them), so the run-ended notification can raise the session title and
reply preview under a just-enabled incognito setting. Record this consumer as
the one documented deviation instead of claiming the migration is complete;
migrating the gate to the policy authority is a named follow-up issue.

Generated-by: GLM-5.3-Flash (ZCode)

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-reviewed exact head 012ca953b52b0bc56403ac7aa6d6dcc853472d7b.

The documentation issue from the previous review is fixed. docs/workspace-privacy-context.md now explicitly records the run-ended notification gate as the current exception to the Runtime Host privacy authority, including the stale local-settings path and the risk of exposing the session title and reply preview after incognito is enabled. The document no longer presents that migration as complete.

I found no new P0-P3 issue in this docs-only correction. The underlying product defect remains: runtime-host-settings-ipc-main.ts writes privacy to Runtime Host policy, settings-ownership.ts excludes privacy from the local settings patch, and notifications-ipc-main.ts still gates content-bearing notifications on the stale local copy. That implementation risk should remain tracked independently, but this PR now describes it accurately.

Hosted exact-head test, ASF headers, and git diff --check passed. A synthetic merge onto current main (12ebc45e6fe43b2eebd7dfe61f5c1fa92c0c873a) was clean with the PR patch preserved. The incremental commit changes only two documentation lines; the cited implementation is unchanged. I did not run native Windows AppContainer or macOS permission-overlay smoke tests.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

@ggbdpq

ggbdpq commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the re-review and for confirming the fix on head 012ca953b52b0bc56403ac7aa6d6dcc853472d7b.

On the underlying implementation risk (content-bearing notifications still gated on the stale local privacy copy while the authority lives with Runtime Host policy): agreed it should be tracked independently from this docs audit. I'll open a tracking issue referencing notifications-ipc-main.ts and settings-ownership.ts so it doesn't get lost, and link it back here.


中文:多谢复核确认。底层实现缺陷(通知仍按旧的本地隐私开关放行内容)不在本审计 PR 里修,我会单开 issue 跟踪并回链到这里。

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the update. Additional independent review of 012ca953b52b0bc56403ac7aa6d6dcc853472d7b: the privacy-authority exception is correctly documented. The Windows RFC pair still needs a small correction to its release-evidence references.

AI disclosure: Codex agents performed this review; the coordinating agent checked the cited evidence. This is not an independent human review.

中文说明

隐私权威例外已准确记录;Windows RFC 中英文版本的发布证据引用仍需小幅修正。

Comment thread docs/architecture/windows-sandbox-rfc-v1.md
@ggbdpq

ggbdpq commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Tracking issue for the underlying implementation risk described in the review: #4981


中文:底层实现缺陷(通知仍按旧的本地隐私开关放行内容)已单开 issue 跟踪:#4981

The P3 review on apache#4800 flagged three stale evidence references in the
Windows sandbox RFC pair: the Phase 4 tracker links pointed at the
pre-migration `maka-agent/maka-agent` repository (404); section 6.5
attributed the owner-handle, 64-launch soak, and malicious-child matrix
guarantees to apache#2961, but they landed in apache#3722; and the readiness entries
cited apache#3161 as their implementation PR, which closed unmerged — the
implementation landed through apache#3174. Update the repository URLs and the
implementation annotations in both language versions.

Generated-by: GLM-5.3-Flash (ZCode)

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the change. Approving exact head 301873babdb90f5d2d4a03749f2fbdde7b4d4971 following the completed review and Astro-Han’s explicit acceptance of this merge batch. Current checks pass and no review threads remain unresolved.

Align Windows sandbox rollout evidence with the actual implementation PRs and repository URLs. Document the remaining notification privacy-state exception instead of claiming every consumer uses Host policy. Documentation only; the notification runtime defect remains follow-up work.

AI assistance: Codex performed the review and final-state verification; Astro-Han authorized approval and merge.

中文

感谢改动。基于已完成的审查和 Astro-Han 对本批次的明确认可,批准当前精确 head;检查通过,讨论已结清。此前说明的验证边界与后续事项保持不变。本次由 Codex 执行审查和状态核对,Astro-Han 授权批准与合并。

@Astro-Han
Astro-Han merged commit 2f5f04e into apache:main Sep 7, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/S Under 100 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants