docs(mac-access): freeze standalone connector architecture#707
docs(mac-access): freeze standalone connector architecture#707100yenadmin wants to merge 6 commits into
Conversation
📝 WalkthroughWalkthroughAdds the Mac Access v0.1 architecture, threat model, migration plan, shared connector-core contracts, valid/invalid fixtures, cryptographic vectors, and TypeScript, Swift, Python, and Vitest validation coverage. ChangesMac Access contract foundation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
100yenadmin
force-pushed
the
codex/699-mac-access-architecture
branch
from
e33cd7c to
5ca1f76
Compare
100yenadmin
changed the base branch from
codex/696-vendored-workbench-bridge
to
evaos/beta-rc-20260612
|
@coderabbitai review |
✅ Action performedReview finished.
|
evaOS review status: completedPR: #707 - docs(mac-access): freeze standalone connector architecture evaOS review completed for this PR head. Automation note: agents should wait for this comment to reach PR URL: #707 Review URL: #707 (review) |
![evaos-code-review-bot[bot]](https://avatars.githubusercontent.com/in/4184532?s=60&v=4){kind=small}
There was a problem hiding this comment.
Walkthrough
PR: #707 - docs(mac-access): freeze standalone connector architecture
Head: 5ca1f7688a81c80d2e008ac7b4cf46698f3a0050 into evaos/beta-rc-20260612. Review event: COMMENT.
Provider: GLM/Z.ai through ZCode (zcode-glm, zcode, model GLM-5.2).
Estimated review effort: 3/5 (~36 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
tests/contract/macAccessContracts.test.ts |
added | +354/-0 | Test coverage | Moderate: validated P2 finding |
tests/contract/macAccessCryptography.test.ts |
added | +159/-0 | Test coverage | Moderate: validated P2 finding |
vitest.config.ts |
modified | +1/-0 | Configuration | Low |
Review Signal
Validated inline findings: 3 (P0: 0, P1: 0, P2: 2, P3: 1).
Dropped findings before posting: 0. High-severity findings: 0.
Risk Taxonomy
- API compatibility: 2
- Auth: 1
Validation and Proof
1 required validation/proof recommendation(s) selected from changed files.
- required: TypeScript/web build or CI proof - Runtime TypeScript/web files or package/config files changed. Proof: npm run build; typecheck; focused Vitest; green GitHub check.
Proof status: sufficient - PR metadata mentions acceptable proof for each required validation recommendation.
Profile validation hints: Do not ask for broad local suites when remote CI or fast-smoke proof is the right gate.
Profile proof expectations: Look for focused app smoke, packaged resource checks, or CI artifact proof when relevant.
Related Context
Related issues/PRs: #697, #708, #709, #699, #698, #669, #73, #704.
Suggested labels: tests.
Suggested reviewers: none from current metadata.
Review Settings Preview
- Profile: assertive
- Enabled sections: Review summary (inline_review); Walkthrough (inline_review); Changed-files table (walkthrough); Effort estimate (walkthrough); Related issues/PRs (walkthrough); Suggested labels (suggestion_only); Review status comment (sticky_status)
- Path instructions:
apps/eva-desktop-mac/**- Check macOS identity, helper path, TCC identity, and packaged resource shape risk. - Path instructions:
scripts/**- Treat release, packaging, and artifact-shape changes as high risk. - Label suggestions: workbench, macos, regression-hardening
- Reviewer suggestions: none
- Suggestion behavior: suggestions only; labels and reviewers are not auto-applied.
- Roadmap-only settings: auto-apply labels; auto-request reviewers; required status checks
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
- Labels and reviewers are suggestions only; the bot did not auto-apply them.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ca1f7688a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
evaOS review status: stale headPR: #707 - docs(mac-access): freeze standalone connector architecture evaOS review stopped because this queued head is no longer the live PR head. Automation note: agents should wait for this comment to reach PR URL: #707 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@codex review |
evaOS review status: completedPR: #707 - docs(mac-access): freeze standalone connector architecture evaOS review completed for this PR head. Automation note: agents should wait for this comment to reach PR URL: #707 Review URL: #707 (review) |
There was a problem hiding this comment.
Walkthrough
PR: #707 - docs(mac-access): freeze standalone connector architecture
Head: 60d0997286b10dce262855c5ea8dfd863dbe60ea into evaos/beta-rc-20260612. Review event: COMMENT.
Provider: GLM/Z.ai through ZCode (zcode-glm, zcode, model GLM-5.2).
Estimated review effort: 4/5 (~46 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
tests/contract/macAccessContracts.test.ts |
added | +550/-0 | Test coverage | Elevated: large change |
tests/contract/macAccessCryptography.test.ts |
added | +194/-0 | Test coverage | Low |
vitest.config.ts |
modified | +6/-1 | Configuration | Low |
Review Signal
No validated inline findings.
Dropped findings before posting: 0. High-severity findings: 0.
Risk Taxonomy
No finding categories.
Validation and Proof
1 required validation/proof recommendation(s) selected from changed files.
- required: TypeScript/web build or CI proof - Runtime TypeScript/web files or package/config files changed. Proof: npm run build; typecheck; focused Vitest; green GitHub check.
Proof status: sufficient - PR metadata mentions acceptable proof for each required validation recommendation.
Profile validation hints: Do not ask for broad local suites when remote CI or fast-smoke proof is the right gate.
Profile proof expectations: Look for focused app smoke, packaged resource checks, or CI artifact proof when relevant.
Related Context
Related issues/PRs: #697, #708, #709, #699, #698, #669, #73, #700.
Suggested labels: tests.
Suggested reviewers: none from current metadata.
Review Settings Preview
- Profile: assertive
- Enabled sections: Review summary (inline_review); Walkthrough (inline_review); Changed-files table (walkthrough); Effort estimate (walkthrough); Related issues/PRs (walkthrough); Suggested labels (suggestion_only); Review status comment (sticky_status)
- Path instructions:
apps/eva-desktop-mac/**- Check macOS identity, helper path, TCC identity, and packaged resource shape risk. - Path instructions:
scripts/**- Treat release, packaging, and artifact-shape changes as high risk. - Label suggestions: workbench, macos, regression-hardening
- Reviewer suggestions: none
- Suggestion behavior: suggestions only; labels and reviewers are not auto-applied.
- Roadmap-only settings: auto-apply labels; auto-request reviewers; required status checks
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
- Labels and reviewers are suggestions only; the bot did not auto-apply them.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60d0997286
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
evaOS review status: completedPR: #707 - docs(mac-access): freeze standalone connector architecture evaOS review completed for this PR head. Automation note: agents should wait for this comment to reach PR URL: #707 Review URL: #707 (review) |
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
coderabbitai
Bot
added
evaos
There was a problem hiding this comment.
Walkthrough
PR: #707 - docs(mac-access): freeze standalone connector architecture
Head: b0e26adaea15714ded504e1b690653ea543b9f25 into evaos/beta-rc-20260612. Review event: COMMENT.
Provider: GLM/Z.ai through ZCode (zcode-glm, zcode, model GLM-5.2).
Estimated review effort: 5/5 (~58 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
tests/contract/macAccessContracts.test.ts |
added | +668/-0 | Test coverage | Elevated: large change |
tests/contract/macAccessCryptography.test.ts |
added | +194/-0 | Test coverage | Low |
tsconfig.json |
modified | +2/-0 | Configuration | Low |
vitest.config.ts |
modified | +6/-1 | Configuration | Low |
Review Signal
No validated inline findings.
Dropped findings before posting: 0. High-severity findings: 0.
Risk Taxonomy
No finding categories.
Validation and Proof
1 required validation/proof recommendation(s) selected from changed files.
- required: TypeScript/web build or CI proof - Runtime TypeScript/web files or package/config files changed. Proof: npm run build; typecheck; focused Vitest; green GitHub check.
Proof status: sufficient - PR metadata mentions acceptable proof for each required validation recommendation.
Profile validation hints: Do not ask for broad local suites when remote CI or fast-smoke proof is the right gate.
Profile proof expectations: Look for focused app smoke, packaged resource checks, or CI artifact proof when relevant.
Related Context
Related issues/PRs: #697, #708, #709, #699, #698, #669, #73, #700.
Suggested labels: tests.
Suggested reviewers: none from current metadata.
Review Settings Preview
- Profile: assertive
- Enabled sections: Review summary (inline_review); Walkthrough (inline_review); Changed-files table (walkthrough); Effort estimate (walkthrough); Related issues/PRs (walkthrough); Suggested labels (suggestion_only); Review status comment (sticky_status)
- Path instructions:
apps/eva-desktop-mac/**- Check macOS identity, helper path, TCC identity, and packaged resource shape risk. - Path instructions:
scripts/**- Treat release, packaging, and artifact-shape changes as high risk. - Label suggestions: workbench, macos, regression-hardening
- Reviewer suggestions: none
- Suggestion behavior: suggestions only; labels and reviewers are not auto-applied.
- Roadmap-only settings: auto-apply labels; auto-request reviewers; required status checks
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
- Labels and reviewers are suggestions only; the bot did not auto-apply them.
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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 `@docs/evaos/mac-access/architecture.md`:
- Around line 282-288: The audit journal specification in
docs/evaos/mac-access/architecture.md (lines 282-288) must define a separately
protected committed sequence and digest, and require verifying that external
anchor before actuation. In docs/evaos/mac-access/threat-model.md (line 87), add
runtime tests covering valid-tail truncation and whole-journal replacement
against that external anchor.
- Line 134: The architecture must align the outbound TLS WebSocket owner with
the App Sandbox network entitlement. Update the transport ownership described
near the artifact relationship and the referenced sections so either the helper
receives com.apple.security.network.client, or the connector is explicitly made
the authenticated relay client; keep the entitlement assignment and component
responsibilities consistent throughout.
In
`@packages/mac-connector-core/contracts/v1/fixtures/valid/audit/audit-event.json`:
- Around line 13-15: Update the audit fixture’s binding_fingerprint_sha256
associated with command_id "command-01" to match the selected "1111..." binding
fingerprint used by the corresponding authority fixture, while preserving the
request digest; alternatively, assign the event a distinct command ID if it is
intended to represent an independent binding.
In
`@packages/mac-connector-core/contracts/v1/fixtures/valid/state/access-transition-stop.json`:
- Around line 24-26: Update the stop fixture’s from-state confirmation
fields—confirmed_runtime_instance_id, confirmed_policy_epoch, and
confirmed_binding_fingerprint_sha256—to valid non-null confirmation values,
while leaving the corresponding to-state fields null so the fixture verifies
stop clears existing confirmation state.
In `@packages/mac-connector-core/contracts/v1/index.ts`:
- Around line 1107-1127: Update auditEvidenceSchema to replace the broad
safeEvidenceIdentifier validation with field-specific allowlists for audit
evidence values. Validate target_path_hash using the existing sha256 schema, and
apply owned enums or constrained schemas to capability, state_from/state_to,
transport_state, and detail_code. Remove reliance on safeEvidenceIdentifier for
these fields while preserving the schema’s strict object behavior.
- Around line 1386-1394: Update the mutations schema to use a discriminated
union on operation: require value for set mutations and disallow value for
remove mutations, while preserving the existing pointer validation and strict
object behavior.
- Around line 1129-1160: Update auditRecordPayloadSchema so command_decision and
command_result events require non-null command_id and request_digest_sha256,
while preserving nullable values for non-command event types. Implement the
conditional validation within the schema’s existing validation flow and keep the
current field formats and strict object behavior unchanged.
- Around line 812-845: Extend localActionRequestSchema to carry the one-time
pairing code for begin_pairing, making it required for that action and rejecting
it for all other local actions. Add the corresponding validation in the existing
superRefine logic while preserving the current expected_policy_epoch and
target_mode rules.
- Around line 390-434: Update the status schema’s superRefine validation to
require access.effective_mode to be 'off' whenever transport.state is
'disconnected', 'connecting', 'revoked', or 'blocked'. Preserve the existing
behavior for 'connected' transport states and add a validation issue on the
transport state when this constraint is violated.
In `@tests/contract/macAccessCryptography.test.ts`:
- Around line 27-35: Replace the hand-rolled canonicalizeJcs function with the
established RFC 8785/JCS library used by the project, such as canonicalize, and
update the cryptographic golden-vector verifier to call that library directly.
Remove the custom sorting and serialization logic while preserving the
verifier’s existing inputs and outputs.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1aef7789-798d-4ade-9e73-06c6e909fd67
📒 Files selected for processing (30)
docs/evaos/mac-access/adr-0001-independent-product-shared-repository.mddocs/evaos/mac-access/architecture.mddocs/evaos/mac-access/migration-map.mddocs/evaos/mac-access/threat-model.mdpackages/mac-connector-core/README.mdpackages/mac-connector-core/contracts/v1/fixtures/host/host-request.jsonpackages/mac-connector-core/contracts/v1/fixtures/host/host-response.jsonpackages/mac-connector-core/contracts/v1/fixtures/invalid/binding.jsonpackages/mac-connector-core/contracts/v1/fixtures/invalid/identity.jsonpackages/mac-connector-core/contracts/v1/fixtures/invalid/policy.jsonpackages/mac-connector-core/contracts/v1/fixtures/invalid/transport.jsonpackages/mac-connector-core/contracts/v1/fixtures/valid/audit/audit-chain-golden.jsonpackages/mac-connector-core/contracts/v1/fixtures/valid/audit/audit-event.jsonpackages/mac-connector-core/contracts/v1/fixtures/valid/authority/broker-control.jsonpackages/mac-connector-core/contracts/v1/fixtures/valid/authority/command-authority-golden.jsonpackages/mac-connector-core/contracts/v1/fixtures/valid/authority/local-action.jsonpackages/mac-connector-core/contracts/v1/fixtures/valid/state/access-state.jsonpackages/mac-connector-core/contracts/v1/fixtures/valid/state/access-transition-grant-expired.jsonpackages/mac-connector-core/contracts/v1/fixtures/valid/state/access-transition-stop.jsonpackages/mac-connector-core/contracts/v1/fixtures/valid/state/access-transition.jsonpackages/mac-connector-core/contracts/v1/fixtures/valid/state/full-access-state.jsonpackages/mac-connector-core/contracts/v1/fixtures/valid/state/local-status.jsonpackages/mac-connector-core/contracts/v1/golden/rollback-authorization-golden.jsonpackages/mac-connector-core/contracts/v1/index.tspackages/mac-connector-core/tests/FixtureSmoke.swiftpackages/mac-connector-core/tests/fixture_smoke.pytests/contract/macAccessContracts.test.tstests/contract/macAccessCryptography.test.tstsconfig.jsonvitest.config.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: Build Test (macos-arm64)
- GitHub Check: Coverage Test
- GitHub Check: Unit Tests (macos-14)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,jsx,ts,tsx}: Name utility files using camelCase, such as formatDate.ts.
Prefix unused parameters with_.
Format code with Oxfmt using Prettier-compatible rules: inline single-element arrays that fit on one line, require trailing commas in multiline arrays and objects, and use single quotes for strings.
Files:
vitest.config.tstests/contract/macAccessCryptography.test.tspackages/mac-connector-core/contracts/v1/index.tstests/contract/macAccessContracts.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use strict TypeScript; do not useanyand do not leave implicit returns.
Use the path aliases@/*,@process/*, and@renderer/*.
Prefertypeoverinterfaceaccording to the Oxlint configuration.
Write code comments in English and use JSDoc for public functions.
Files:
vitest.config.tstests/contract/macAccessCryptography.test.tspackages/mac-connector-core/contracts/v1/index.tstests/contract/macAccessContracts.test.ts
**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Vitest 4 for tests and maintain at least 80% coverage.
Files:
tests/contract/macAccessCryptography.test.tstests/contract/macAccessContracts.test.ts
🪛 ast-grep (0.44.1)
tests/contract/macAccessCryptography.test.ts
[warning] 23-23: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(filePath, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
tests/contract/macAccessContracts.test.ts
[warning] 38-38: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(filePath, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🪛 LanguageTool
docs/evaos/mac-access/adr-0001-independent-product-shared-repository.md
[style] ~23-~23: Since ownership is already implied, this phrasing may be redundant.
Context: ...rozen bundle/helper/service identities, its own signing/notarization job, updater/appca...
(PRP_OWN)
[style] ~24-~24: Since ownership is already implied, this phrasing may be redundant.
Context: ....36 and later Workbench releases retain their own independent release truth. A Mac Access...
(PRP_OWN)
[style] ~40-~40: Since ownership is already implied, this phrasing may be redundant.
Context: ... Mac Access release workflows must name their own paths and artifacts. A root version bum...
(PRP_OWN)
docs/evaos/mac-access/threat-model.md
[style] ~58-~58: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...eychain and append-only audit files. 5. Helper to macOS TCC/CUA frameworks. 6. Helper ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~59-~59: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... Helper to macOS TCC/CUA frameworks. 6. Helper outbound WebSocket to broker relay. 7. ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[grammar] ~64-~64: Use a hyphen to join words.
Context: ...ted, length-bounded, versioned, and fail closed. Unknown fields are rejected at a...
(QB_NEW_EN_HYPHEN)
[grammar] ~109-~109: Use a hyphen to join words.
Context: ...s supplemental evidence only. Production designated requirements bind Team ID `TC...
(QB_NEW_EN_HYPHEN)
[style] ~110-~110: This sentence is over 40 words long. Consider splitting it up, as shorter sentences make the text easier to read.
Context: ...connect to production relay authority. Production Keychain items use an epoch group derived from the com.evaos.mac-access.credentials base and the com.evaos.mac-access.connector-credential service, are non-synchronizing and ThisDeviceOnly/WhenUnlocked, and are granted only to the production helper target accepted for that security epoch. Development uses the disjoint `com.evao...
(TOO_LONG_SENTENCE)
[grammar] ~115-~115: Use a hyphen to join words.
Context: ... app main executable is the expected TCC responsible executable shown to the user...
(QB_NEW_EN_HYPHEN)
docs/evaos/mac-access/architecture.md
[grammar] ~14-~14: Use a hyphen to join words.
Context: ...ues/73). Implementation must remain fail closed until that contract exists; the c...
(QB_NEW_EN_HYPHEN)
[uncategorized] ~134-~134: The operating system from Apple is written “macOS”.
Context: ...lementation: the app main executable is Contents/MacOS/evaOS Mac Access; the persistent conne...
(MAC_OS)
[grammar] ~136-~136: Use a hyphen to join words.
Context: ...vaOS Mac Access.app` is the intended TCC responsible executable shown to the user...
(QB_NEW_EN_HYPHEN)
[style] ~136-~136: Since ownership is already implied, this phrasing may be redundant.
Context: ...on audit token. A request cannot assert its own identity. Keychain custody is frozen a...
(PRP_OWN)
[style] ~140-~140: This sentence is over 40 words long. Consider splitting it up, as shorter sentences make the text easier to read.
Context: ...heir old broker credential is rejected. Exceptional rollback uses evaos.mac_access.rollback_authorization_payload.v1, signed over RFC 8785/JCS bytes, to name the authorization ID, exact source and target version/commit/lineage/security epoch, schema reader/writer versions, both credential epochs, resulting reader/writer security and schema floors, and issue/expiry interval. The persisted verified pre-rollback bui...
(TOO_LONG_SENTENCE)
[style] ~217-~217: This sentence is over 40 words long. Consider splitting it up, as shorter sentences make the text easier to read.
Context: ...cess.broker_control.v1and carries: - the selected tuple; - the server-owned Ed25519evaos.mac_control_execution_context.v1produced by ws-proxy#69; - a separate evaos.mac_access.command_authority_payload.v1signed authorization that exhaustively includes session, channel generation, the full selected tuple and grant expiry, current policy epoch, execution-context digest, command ID, capability, exact request digest, random nonce, sequence, and issue/expiry times; - a maximum 60-second command authority fully contained inside the signed#69` execution-context interval and ending before grant expiry; - no reusable connector URL/token. The command authorization signs the UT...
(TOO_LONG_SENTENCE)
[style] ~302-~302: Consider using a different verb to strengthen your wording.
Context: ...the exact verified legacy connector; 7. remove Workbench access to connector tokens, U...
(REMOVE_REVOKE)
🪛 OpenGrep (1.25.0)
packages/mac-connector-core/contracts/v1/index.ts
[WARNING] 88-88: Sequelize.literal() with dynamic input can lead to SQL injection. Use parameterized queries or model methods instead.
(coderabbit.sql-injection.sequelize-literal)
[WARNING] 91-91: Sequelize.literal() with dynamic input can lead to SQL injection. Use parameterized queries or model methods instead.
(coderabbit.sql-injection.sequelize-literal)
[WARNING] 93-93: Sequelize.literal() with dynamic input can lead to SQL injection. Use parameterized queries or model methods instead.
(coderabbit.sql-injection.sequelize-literal)
[WARNING] 94-94: Sequelize.literal() with dynamic input can lead to SQL injection. Use parameterized queries or model methods instead.
(coderabbit.sql-injection.sequelize-literal)
[WARNING] 200-200: Sequelize.literal() with dynamic input can lead to SQL injection. Use parameterized queries or model methods instead.
(coderabbit.sql-injection.sequelize-literal)
[WARNING] 368-368: Sequelize.literal() with dynamic input can lead to SQL injection. Use parameterized queries or model methods instead.
(coderabbit.sql-injection.sequelize-literal)
[WARNING] 369-369: Sequelize.literal() with dynamic input can lead to SQL injection. Use parameterized queries or model methods instead.
(coderabbit.sql-injection.sequelize-literal)
[WARNING] 370-370: Sequelize.literal() with dynamic input can lead to SQL injection. Use parameterized queries or model methods instead.
(coderabbit.sql-injection.sequelize-literal)
[WARNING] 371-371: Sequelize.literal() with dynamic input can lead to SQL injection. Use parameterized queries or model methods instead.
(coderabbit.sql-injection.sequelize-literal)
[WARNING] 372-372: Sequelize.literal() with dynamic input can lead to SQL injection. Use parameterized queries or model methods instead.
(coderabbit.sql-injection.sequelize-literal)
[WARNING] 373-373: Sequelize.literal() with dynamic input can lead to SQL injection. Use parameterized queries or model methods instead.
(coderabbit.sql-injection.sequelize-literal)
[WARNING] 374-374: Sequelize.literal() with dynamic input can lead to SQL injection. Use parameterized queries or model methods instead.
(coderabbit.sql-injection.sequelize-literal)
[WARNING] 399-399: Sequelize.literal() with dynamic input can lead to SQL injection. Use parameterized queries or model methods instead.
(coderabbit.sql-injection.sequelize-literal)
[WARNING] 1031-1031: Sequelize.literal() with dynamic input can lead to SQL injection. Use parameterized queries or model methods instead.
(coderabbit.sql-injection.sequelize-literal)
🪛 Ruff (0.15.21)
packages/mac-connector-core/tests/fixture_smoke.py
[warning] 11-11: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (33)
docs/evaos/mac-access/adr-0001-independent-product-shared-repository.md (1)
1-128: LGTM!docs/evaos/mac-access/architecture.md (1)
1-133: LGTM!Also applies to: 135-281, 289-367
docs/evaos/mac-access/migration-map.md (1)
1-220: LGTM!docs/evaos/mac-access/threat-model.md (1)
1-86: LGTM!Also applies to: 88-155
packages/mac-connector-core/README.md (1)
1-9: LGTM!packages/mac-connector-core/contracts/v1/index.ts (1)
1-389: LGTM!Also applies to: 435-811, 846-1106, 1128-1128, 1161-1385, 1395-1427
packages/mac-connector-core/contracts/v1/fixtures/valid/state/local-status.json (1)
1-132: LGTM!packages/mac-connector-core/contracts/v1/golden/rollback-authorization-golden.json (1)
1-38: LGTM!packages/mac-connector-core/contracts/v1/fixtures/host/host-request.json (1)
1-9: LGTM!packages/mac-connector-core/contracts/v1/fixtures/host/host-response.json (1)
1-16: LGTM!packages/mac-connector-core/contracts/v1/fixtures/invalid/binding.json (1)
1-117: LGTM!packages/mac-connector-core/contracts/v1/fixtures/invalid/identity.json (1)
1-257: LGTM!packages/mac-connector-core/contracts/v1/fixtures/invalid/policy.json (1)
1-398: LGTM!packages/mac-connector-core/contracts/v1/fixtures/invalid/transport.json (1)
1-84: LGTM!packages/mac-connector-core/contracts/v1/fixtures/valid/audit/audit-chain-golden.json (1)
1-55: LGTM!packages/mac-connector-core/contracts/v1/fixtures/valid/authority/broker-control.json (1)
1-87: LGTM!packages/mac-connector-core/contracts/v1/fixtures/valid/authority/command-authority-golden.json (1)
1-36: LGTM!packages/mac-connector-core/contracts/v1/fixtures/valid/authority/local-action.json (1)
1-20: LGTM!packages/mac-connector-core/contracts/v1/fixtures/valid/state/access-state.json (1)
1-34: LGTM!packages/mac-connector-core/contracts/v1/fixtures/valid/state/access-transition-grant-expired.json (1)
1-65: LGTM!tests/contract/macAccessContracts.test.ts (3)
607-667: LGTM! Both sections concretely resolve the previously-flagged duplicate-threat dedup gap and theit.todoruntime-placeholder gap (exact ID-set pinning at line 654, concrete ledger with noit.todoat 657-667).
1-220: LGTM! Fixture-mutation helpers and manifest loading are correctly implemented; thefs.readFileSyncpath-traversal static-analysis hint (line 38) is a false positive since all paths derive from fixed constants and directory listings, not external input.
221-606: LGTM! Verified the negative-test expectations against the upstream schema invariants (accessTransitionSchema,localStatusSchema,coreHostRequestSchema) provided as context — assertions are internally consistent.tests/contract/macAccessCryptography.test.ts (3)
163-193: LGTM! These four negative cases concretely resolve the previously-flagged "audit-chain tampering/rollback forgery assertions are it.todo" gap — each mutation now assertsverifiesAuditChainGolden(...)returnsfalse.
37-95: LGTM!verifiesAuditChainGoldencorrectly enforces sequence, chaining, canonical-match, and digest checks; the command-authority and audit-chain golden verification tests (76-95) look correct, including propernull-algorithm EdDSAverify()usage.
97-161: LGTM!packages/mac-connector-core/contracts/v1/fixtures/valid/state/access-transition.json (2)
1-65: LGTM!
60-60: 🗄️ Data Integrity & IntegrationNo change needed — these placeholders are valid sha256 values. The
binding_fingerprint_sha256andconfirmed_binding_fingerprint_sha256literals are 64 hex characters and satisfy the schema.> Likely an incorrect or invalid review comment.packages/mac-connector-core/tests/FixtureSmoke.swift (1)
1-31: LGTM!packages/mac-connector-core/tests/fixture_smoke.py (1)
1-20: LGTM! The Ruff TRY003 hint on line 11 is a low-value nit for an idiomaticSystemExitmessage in a CLI smoke script — not flagging it.tsconfig.json (1)
26-27: LGTM!vitest.config.ts (1)
24-35: LGTM!Also applies to: 62-66
packages/mac-connector-core/contracts/v1/fixtures/valid/state/full-access-state.json (1)
18-18: 🗄️ Data Integrity & IntegrationNo issue here — both
confirmed_binding_fingerprint_sha256andbinding_fingerprint_sha256are 64-character SHA-256 values, so the fixture shape is consistent.> Likely an incorrect or invalid review comment.
|
|
||
| The contract SHA-256 is over the exact UTF-8 requirement text above: app `da635352f249b4213aa1a96c41d7979d8b25d86b056b9f0929c1b414e35896fb`, helper `222107bb855cfc463805777c76ca8cfdac0d1145957c5f190c234e52bfd277aa`, connector `0c3de778270de5b4a1992d0e13d4f27e41929c7ace94ae143bcba92a555be422`. Authenticated Workbench client allowlists use the same anchor/Team ID with identifier `com.evaos.workbench` (digest `ff4fc126bb70bbf7fcc3cc0957377d67185124b5e31b19760357333a8a0ae329`) or the shipped legacy identifier `com.electricsheephq.EvaDesktop` (digest `c6038eaf8a20c83a1aabfd1bf8eb4053877b7af5627e570eb1de37721e76b776`). The native verifier selects the frozen requirement for the expected role, obtains the connection process from the audit token, evaluates that `SecCode` against the requirement, and records the digest of the frozen requirement used. Wire callers cannot supply a trusted role or digest. | ||
|
|
||
| The artifact relationship is frozen before implementation: the app main executable is `Contents/MacOS/evaOS Mac Access`; the persistent connector is a nested signed login item at `Contents/Library/LoginItems/evaOS Mac Access Connector.app`; and the CUA helper is a nested signed XPC service at `Contents/XPCServices/evaOS Mac Access Helper.xpc`. The helper is launched only by the signed app/connector and accepts only frozen app or Workbench client requirements. The app and connector receive no production credential access group. Only the helper receives `TC6MS3T6NN.com.evaos.mac-access.credentials.epoch-N`; no target receives `get-task-allow`, JIT, unsigned-executable-memory, disable-library-validation, inbound network-server, or Apple-events automation entitlements. If App Sandbox is enabled, outbound `com.apple.security.network.client` belongs only to the connector. Development entitlements and access groups are disjoint. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
file='docs/evaos/mac-access/architecture.md'
wc -l "$file"
printf '\n--- lines 1-40 ---\n'
sed -n '1,40p' "$file" | cat -n
printf '\n--- lines 120-170 ---\n'
sed -n '120,170p' "$file" | cat -n
printf '\n--- lines 270-300 ---\n'
sed -n '270,300p' "$file" | cat -nRepository: 100yenadmin/evaOS-GUI
Length of output: 18892
🏁 Script executed:
set -euo pipefail
file='docs/evaos/mac-access/architecture.md'
printf '%s\n' '--- audit/head-related matches ---'
rg -n -i 'head|tip|anchor|chain|digest|hash' "$file"Repository: 100yenadmin/evaOS-GUI
Length of output: 12100
Assign network.client to the relay owner
docs/evaos/mac-access/architecture.md:15,198says the helper opens the outbound TLS WebSocket, butnetwork.clientis granted only to the connector. If App Sandbox is enabled, the declared transport owner cannot reach the relay.- Move
com.apple.security.network.clientto the helper, or make the connector the authenticated relay client.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~134-~134: The operating system from Apple is written “macOS”.
Context: ...lementation: the app main executable is Contents/MacOS/evaOS Mac Access; the persistent conne...
(MAC_OS)
🤖 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 `@docs/evaos/mac-access/architecture.md` at line 134, The architecture must
align the outbound TLS WebSocket owner with the App Sandbox network entitlement.
Update the transport ownership described near the artifact relationship and the
referenced sections so either the helper receives
com.apple.security.network.client, or the connector is explicitly made the
authenticated relay client; keep the entitlement assignment and component
responsibilities consistent throughout.
| ## Audit and redaction | ||
|
|
||
| The helper writes an append-only, owner-only local journal with a monotonic sequence, previous-record digest, and record digest. `record_sha256` is SHA-256 over the exact UTF-8 RFC 8785/JCS serialization of the versioned `evaos.mac_access.audit_event.v1` payload with `record_sha256` excluded. The next payload's `previous_record_sha256` must equal that digest. `audit-chain-golden.json` freezes a two-record chain, canonical bytes, and both digests; edit/delete/reorder/previous-digest checks are required runtime tests. The payload records non-secret identity kind, binding fingerprint, command/request IDs and digests, access mode, outcome, reason code, and an explicit bounded allowlist of evidence metadata. Arbitrary evidence keys/values are not valid contract data. | ||
|
|
||
| Default audit and support evidence forbids raw screenshots, Accessibility trees, typed text, clipboard content, cookies, auth headers, passwords, tokens, connector URLs, private addresses, Keychain references, environment dumps, and unredacted native exception text. Optional artifacts require a separate exact-scope user decision, encrypted storage, retention deadline, access audit, and explicit purge; that feature is outside v0.1 unless separately approved. | ||
|
|
||
| If the audit cannot durably record the decision before execution, execution is denied. Result-write failure activates effective `Off` and emits only a local minimal failure marker if possible. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Anchor the audit journal before claiming deletion detection.
The internal hash chain cannot detect valid-tail truncation or whole-journal replacement by a same-user process.
docs/evaos/mac-access/architecture.md#L282-L288: define a separately protected committed sequence/digest and verify it before actuation.docs/evaos/mac-access/threat-model.md#L87-L87: require truncation and replacement tests against that external anchor.
📍 Affects 2 files
docs/evaos/mac-access/architecture.md#L282-L288(this comment)docs/evaos/mac-access/threat-model.md#L87-L87
🤖 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 `@docs/evaos/mac-access/architecture.md` around lines 282 - 288, The audit
journal specification in docs/evaos/mac-access/architecture.md (lines 282-288)
must define a separately protected committed sequence and digest, and require
verifying that external anchor before actuation. In
docs/evaos/mac-access/threat-model.md (line 87), add runtime tests covering
valid-tail truncation and whole-journal replacement against that external
anchor.
| "binding_fingerprint_sha256": "ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff", | ||
| "command_id": "command-01", | ||
| "request_digest_sha256": "dddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddd", |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep the audit binding fingerprint consistent with command-01.
This event reuses command-01 and the same request digest as packages/mac-connector-core/contracts/v1/fixtures/valid/authority/broker-control.json, but records an ffff... binding fingerprint while the authority fixtures select 1111.... That makes the audit record attribute one command to a different binding. Use the selected binding fingerprint here, or assign this independent event a distinct command ID.
🤖 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
`@packages/mac-connector-core/contracts/v1/fixtures/valid/audit/audit-event.json`
around lines 13 - 15, Update the audit fixture’s binding_fingerprint_sha256
associated with command_id "command-01" to match the selected "1111..." binding
fingerprint used by the corresponding authority fixture, while preserving the
request digest; alternatively, assign the event a distinct command ID if it is
intended to represent an independent binding.
| "confirmed_runtime_instance_id": null, | ||
| "confirmed_policy_epoch": null, | ||
| "confirmed_binding_fingerprint_sha256": null, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the stop fixture exercise confirmation invalidation.
from.confirmed_runtime_instance_id, from.confirmed_policy_epoch, and from.confirmed_binding_fingerprint_sha256 are already null, so this case passes even if stop fails to clear existing confirmation state. Populate them with a valid confirmed context, while keeping the corresponding to fields null.
🤖 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
`@packages/mac-connector-core/contracts/v1/fixtures/valid/state/access-transition-stop.json`
around lines 24 - 26, Update the stop fixture’s from-state confirmation
fields—confirmed_runtime_instance_id, confirmed_policy_epoch, and
confirmed_binding_fingerprint_sha256—to valid non-null confirmation values,
while leaving the corresponding to-state fields null so the fixture verifies
stop clears existing confirmation state.
| transport: z | ||
| .object({ | ||
| state: z.enum(['disconnected', 'connecting', 'connected', 'revoked', 'blocked']), | ||
| channel_id: identifier.nullable(), | ||
| last_error_code: identifier.nullable(), | ||
| }) | ||
| .strict(), | ||
| tcc: z | ||
| .object({ | ||
| responsible_identity: z.literal(MAC_ACCESS_IDENTITIES.appBundleId), | ||
| accessibility: z.enum(['unknown', 'missing', 'granted', 'denied']), | ||
| screen_recording: z.enum(['unknown', 'missing', 'granted', 'denied']), | ||
| }) | ||
| .strict(), | ||
| audit: z | ||
| .object({ | ||
| writable: z.boolean(), | ||
| last_audit_id: identifier.nullable(), | ||
| }) | ||
| .strict(), | ||
| }) | ||
| .strict() | ||
| .superRefine((status, context) => { | ||
| if (status.leader.runtime_instance_id !== status.access.runtime_instance_id) { | ||
| context.addIssue({ | ||
| code: z.ZodIssueCode.custom, | ||
| message: 'leader and access state must describe the same runtime instance', | ||
| path: ['leader', 'runtime_instance_id'], | ||
| }); | ||
| } | ||
| if (!status.audit.writable && status.access.effective_mode !== 'off') { | ||
| context.addIssue({ | ||
| code: z.ZodIssueCode.custom, | ||
| message: 'audit failure must force effective access off', | ||
| path: ['audit', 'writable'], | ||
| }); | ||
| } | ||
| const tccGranted = status.tcc.accessibility === 'granted' && status.tcc.screen_recording === 'granted'; | ||
| if (!tccGranted && status.access.effective_mode !== 'off') { | ||
| context.addIssue({ | ||
| code: z.ZodIssueCode.custom, | ||
| message: 'missing, denied, or unknown TCC state must force effective access off', | ||
| path: ['tcc'], | ||
| }); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Force effective access Off when transport is unavailable.
The architecture defines effective mode as authority after transport constraints, but disconnected, connecting, revoked, or blocked status can currently report ask_every_time or full_access.
Proposed validation
+ if (status.transport.state !== 'connected' && status.access.effective_mode !== 'off') {
+ context.addIssue({
+ code: z.ZodIssueCode.custom,
+ message: 'unavailable transport must force effective access off',
+ path: ['access', 'effective_mode'],
+ });
+ }📝 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.
| transport: z | |
| .object({ | |
| state: z.enum(['disconnected', 'connecting', 'connected', 'revoked', 'blocked']), | |
| channel_id: identifier.nullable(), | |
| last_error_code: identifier.nullable(), | |
| }) | |
| .strict(), | |
| tcc: z | |
| .object({ | |
| responsible_identity: z.literal(MAC_ACCESS_IDENTITIES.appBundleId), | |
| accessibility: z.enum(['unknown', 'missing', 'granted', 'denied']), | |
| screen_recording: z.enum(['unknown', 'missing', 'granted', 'denied']), | |
| }) | |
| .strict(), | |
| audit: z | |
| .object({ | |
| writable: z.boolean(), | |
| last_audit_id: identifier.nullable(), | |
| }) | |
| .strict(), | |
| }) | |
| .strict() | |
| .superRefine((status, context) => { | |
| if (status.leader.runtime_instance_id !== status.access.runtime_instance_id) { | |
| context.addIssue({ | |
| code: z.ZodIssueCode.custom, | |
| message: 'leader and access state must describe the same runtime instance', | |
| path: ['leader', 'runtime_instance_id'], | |
| }); | |
| } | |
| if (!status.audit.writable && status.access.effective_mode !== 'off') { | |
| context.addIssue({ | |
| code: z.ZodIssueCode.custom, | |
| message: 'audit failure must force effective access off', | |
| path: ['audit', 'writable'], | |
| }); | |
| } | |
| const tccGranted = status.tcc.accessibility === 'granted' && status.tcc.screen_recording === 'granted'; | |
| if (!tccGranted && status.access.effective_mode !== 'off') { | |
| context.addIssue({ | |
| code: z.ZodIssueCode.custom, | |
| message: 'missing, denied, or unknown TCC state must force effective access off', | |
| path: ['tcc'], | |
| }); | |
| } | |
| transport: z | |
| .object({ | |
| state: z.enum(['disconnected', 'connecting', 'connected', 'revoked', 'blocked']), | |
| channel_id: identifier.nullable(), | |
| last_error_code: identifier.nullable(), | |
| }) | |
| .strict(), | |
| tcc: z | |
| .object({ | |
| responsible_identity: z.literal(MAC_ACCESS_IDENTITIES.appBundleId), | |
| accessibility: z.enum(['unknown', 'missing', 'granted', 'denied']), | |
| screen_recording: z.enum(['unknown', 'missing', 'granted', 'denied']), | |
| }) | |
| .strict(), | |
| audit: z | |
| .object({ | |
| writable: z.boolean(), | |
| last_audit_id: identifier.nullable(), | |
| }) | |
| .strict(), | |
| }) | |
| .strict() | |
| .superRefine((status, context) => { | |
| if (status.leader.runtime_instance_id !== status.access.runtime_instance_id) { | |
| context.addIssue({ | |
| code: z.ZodIssueCode.custom, | |
| message: 'leader and access state must describe the same runtime instance', | |
| path: ['leader', 'runtime_instance_id'], | |
| }); | |
| } | |
| if (!status.audit.writable && status.access.effective_mode !== 'off') { | |
| context.addIssue({ | |
| code: z.ZodIssueCode.custom, | |
| message: 'audit failure must force effective access off', | |
| path: ['audit', 'writable'], | |
| }); | |
| } | |
| const tccGranted = status.tcc.accessibility === 'granted' && status.tcc.screen_recording === 'granted'; | |
| if (!tccGranted && status.access.effective_mode !== 'off') { | |
| context.addIssue({ | |
| code: z.ZodIssueCode.custom, | |
| message: 'missing, denied, or unknown TCC state must force effective access off', | |
| path: ['tcc'], | |
| }); | |
| } | |
| if (status.transport.state !== 'connected' && status.access.effective_mode !== 'off') { | |
| context.addIssue({ | |
| code: z.ZodIssueCode.custom, | |
| message: 'unavailable transport must force effective access off', | |
| path: ['access', 'effective_mode'], | |
| }); | |
| } |
🧰 Tools
🪛 OpenGrep (1.25.0)
[WARNING] 399-399: Sequelize.literal() with dynamic input can lead to SQL injection. Use parameterized queries or model methods instead.
(coderabbit.sql-injection.sequelize-literal)
🤖 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 `@packages/mac-connector-core/contracts/v1/index.ts` around lines 390 - 434,
Update the status schema’s superRefine validation to require
access.effective_mode to be 'off' whenever transport.state is 'disconnected',
'connecting', 'revoked', or 'blocked'. Preserve the existing behavior for
'connected' transport states and add a validation issue on the transport state
when this constraint is violated.
| export const localActionRequestSchema = z | ||
| .object({ | ||
| schema_version: z.literal('evaos.mac_access.local_action.v1'), | ||
| request_id: identifier, | ||
| action: localActionName, | ||
| client_nonce: base64Url, | ||
| expected_policy_epoch: safeNonnegativeCounter.nullable(), | ||
| target_mode: z.enum(['off', 'ask_every_time', 'full_access']).nullable(), | ||
| }) | ||
| .strict() | ||
| .superRefine((request, context) => { | ||
| const mutation = !['get_status', 'open_permissions'].includes(request.action); | ||
| if (mutation && request.expected_policy_epoch === null) { | ||
| context.addIssue({ | ||
| code: z.ZodIssueCode.custom, | ||
| message: 'mutating local actions require the expected policy epoch', | ||
| path: ['expected_policy_epoch'], | ||
| }); | ||
| } | ||
| if (request.action === 'set_access_mode' && request.target_mode === null) { | ||
| context.addIssue({ | ||
| code: z.ZodIssueCode.custom, | ||
| message: 'set_access_mode requires target_mode', | ||
| path: ['target_mode'], | ||
| }); | ||
| } | ||
| if (request.action !== 'set_access_mode' && request.target_mode !== null) { | ||
| context.addIssue({ | ||
| code: z.ZodIssueCode.custom, | ||
| message: 'target_mode is only valid for set_access_mode', | ||
| path: ['target_mode'], | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Include the one-time code in begin_pairing.
This client contract cannot carry the code required by the private pair host operation, leaving no defined menu/Workbench-to-helper pairing path.
Proposed contract change
target_mode: z.enum(['off', 'ask_every_time', 'full_access']).nullable(),
+ pairing_code: z.string().regex(/^[A-Z0-9]{6,12}$/).nullable(),
@@
+ if ((request.action === 'begin_pairing') !== (request.pairing_code !== null)) {
+ context.addIssue({
+ code: z.ZodIssueCode.custom,
+ message: 'pairing_code is required only for begin_pairing',
+ path: ['pairing_code'],
+ });
+ }📝 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.
| export const localActionRequestSchema = z | |
| .object({ | |
| schema_version: z.literal('evaos.mac_access.local_action.v1'), | |
| request_id: identifier, | |
| action: localActionName, | |
| client_nonce: base64Url, | |
| expected_policy_epoch: safeNonnegativeCounter.nullable(), | |
| target_mode: z.enum(['off', 'ask_every_time', 'full_access']).nullable(), | |
| }) | |
| .strict() | |
| .superRefine((request, context) => { | |
| const mutation = !['get_status', 'open_permissions'].includes(request.action); | |
| if (mutation && request.expected_policy_epoch === null) { | |
| context.addIssue({ | |
| code: z.ZodIssueCode.custom, | |
| message: 'mutating local actions require the expected policy epoch', | |
| path: ['expected_policy_epoch'], | |
| }); | |
| } | |
| if (request.action === 'set_access_mode' && request.target_mode === null) { | |
| context.addIssue({ | |
| code: z.ZodIssueCode.custom, | |
| message: 'set_access_mode requires target_mode', | |
| path: ['target_mode'], | |
| }); | |
| } | |
| if (request.action !== 'set_access_mode' && request.target_mode !== null) { | |
| context.addIssue({ | |
| code: z.ZodIssueCode.custom, | |
| message: 'target_mode is only valid for set_access_mode', | |
| path: ['target_mode'], | |
| }); | |
| } | |
| }); | |
| export const localActionRequestSchema = z | |
| .object({ | |
| schema_version: z.literal('evaos.mac_access.local_action.v1'), | |
| request_id: identifier, | |
| action: localActionName, | |
| client_nonce: base64Url, | |
| expected_policy_epoch: safeNonnegativeCounter.nullable(), | |
| target_mode: z.enum(['off', 'ask_every_time', 'full_access']).nullable(), | |
| pairing_code: z.string().regex(/^[A-Z0-9]{6,12}$/).nullable(), | |
| }) | |
| .strict() | |
| .superRefine((request, context) => { | |
| const mutation = !['get_status', 'open_permissions'].includes(request.action); | |
| if (mutation && request.expected_policy_epoch === null) { | |
| context.addIssue({ | |
| code: z.ZodIssueCode.custom, | |
| message: 'mutating local actions require the expected policy epoch', | |
| path: ['expected_policy_epoch'], | |
| }); | |
| } | |
| if (request.action === 'set_access_mode' && request.target_mode === null) { | |
| context.addIssue({ | |
| code: z.ZodIssueCode.custom, | |
| message: 'set_access_mode requires target_mode', | |
| path: ['target_mode'], | |
| }); | |
| } | |
| if (request.action !== 'set_access_mode' && request.target_mode !== null) { | |
| context.addIssue({ | |
| code: z.ZodIssueCode.custom, | |
| message: 'target_mode is only valid for set_access_mode', | |
| path: ['target_mode'], | |
| }); | |
| } | |
| if ((request.action === 'begin_pairing') !== (request.pairing_code !== null)) { | |
| context.addIssue({ | |
| code: z.ZodIssueCode.custom, | |
| message: 'pairing_code is required only for begin_pairing', | |
| path: ['pairing_code'], | |
| }); | |
| } | |
| }); |
🤖 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 `@packages/mac-connector-core/contracts/v1/index.ts` around lines 812 - 845,
Extend localActionRequestSchema to carry the one-time pairing code for
begin_pairing, making it required for that action and rejecting it for all other
local actions. Add the corresponding validation in the existing superRefine
logic while preserving the current expected_policy_epoch and target_mode rules.
| const safeEvidenceIdentifier = identifier.refine( | ||
| (value) => !/(?:authorization|bearer|cookie|password|secret|token|eyJ[A-Za-z0-9_-]{8})/i.test(value), | ||
| 'audit evidence identifier resembles secret-bearing content' | ||
| ); | ||
|
|
||
| export const auditEvidenceSchema = z | ||
| .object({ | ||
| capability: safeEvidenceIdentifier.optional(), | ||
| target_path_hash: safeEvidenceIdentifier.optional(), | ||
| target_fingerprint_sha256: sha256.optional(), | ||
| state_from: safeEvidenceIdentifier.optional(), | ||
| state_to: safeEvidenceIdentifier.optional(), | ||
| transport_state: safeEvidenceIdentifier.optional(), | ||
| detail_code: safeEvidenceIdentifier.optional(), | ||
| build_version: safeEvidenceIdentifier.optional(), | ||
| schema_version: safeEvidenceIdentifier.optional(), | ||
| artifact_count: z.number().int().min(0).max(16).optional(), | ||
| record_count: z.number().int().min(0).max(10_000).optional(), | ||
| redaction_policy: z.literal('default_v1'), | ||
| }) | ||
| .strict(); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Replace the audit-value denylist with field-specific allowlists.
Values such as private addresses, raw identifiers, or API keys can pass safeEvidenceIdentifier and become persistent support evidence. At minimum, validate target_path_hash with sha256; use owned enums or constrained schemas for capability, state, transport, and detail codes.
🤖 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 `@packages/mac-connector-core/contracts/v1/index.ts` around lines 1107 - 1127,
Update auditEvidenceSchema to replace the broad safeEvidenceIdentifier
validation with field-specific allowlists for audit evidence values. Validate
target_path_hash using the existing sha256 schema, and apply owned enums or
constrained schemas to capability, state_from/state_to, transport_state, and
detail_code. Remove reliance on safeEvidenceIdentifier for these fields while
preserving the schema’s strict object behavior.
| export const auditRecordPayloadSchema = z | ||
| .object({ | ||
| schema_version: z.literal('evaos.mac_access.audit_event.v1'), | ||
| audit_id: identifier, | ||
| sequence: safePositiveCounter, | ||
| previous_record_sha256: sha256.nullable(), | ||
| occurred_at: instant, | ||
| event_type: z.enum([ | ||
| 'pairing', | ||
| 'policy_transition', | ||
| 'command_decision', | ||
| 'command_result', | ||
| 'pause', | ||
| 'revoke', | ||
| 'kill_switch', | ||
| 'lifecycle', | ||
| ]), | ||
| actor: z | ||
| .object({ | ||
| kind: z.enum(['local_user', 'workbench', 'broker_runtime', 'system']), | ||
| identity: identifier, | ||
| }) | ||
| .strict(), | ||
| binding_fingerprint_sha256: sha256.nullable(), | ||
| command_id: identifier.nullable(), | ||
| request_digest_sha256: sha256.nullable(), | ||
| access_mode: z.enum(['off', 'ask_every_time', 'full_access']), | ||
| outcome: z.enum(['allowed', 'denied', 'executed', 'failed', 'revoked', 'stopped']), | ||
| reason_code: identifier, | ||
| evidence: auditEvidenceSchema, | ||
| }) | ||
| .strict(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Require command correlation fields for command audit events.
command_decision and command_result currently accept null command IDs and request digests, allowing a valid chained record that cannot prove which command was decided or executed.
Proposed validation
- .strict();
+ .strict()
+ .superRefine((event, context) => {
+ if (
+ ['command_decision', 'command_result'].includes(event.event_type) &&
+ (event.command_id === null || event.request_digest_sha256 === null)
+ ) {
+ context.addIssue({
+ code: z.ZodIssueCode.custom,
+ message: 'command audit events require command and request-digest correlation',
+ path: ['command_id'],
+ });
+ }
+ });📝 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.
| export const auditRecordPayloadSchema = z | |
| .object({ | |
| schema_version: z.literal('evaos.mac_access.audit_event.v1'), | |
| audit_id: identifier, | |
| sequence: safePositiveCounter, | |
| previous_record_sha256: sha256.nullable(), | |
| occurred_at: instant, | |
| event_type: z.enum([ | |
| 'pairing', | |
| 'policy_transition', | |
| 'command_decision', | |
| 'command_result', | |
| 'pause', | |
| 'revoke', | |
| 'kill_switch', | |
| 'lifecycle', | |
| ]), | |
| actor: z | |
| .object({ | |
| kind: z.enum(['local_user', 'workbench', 'broker_runtime', 'system']), | |
| identity: identifier, | |
| }) | |
| .strict(), | |
| binding_fingerprint_sha256: sha256.nullable(), | |
| command_id: identifier.nullable(), | |
| request_digest_sha256: sha256.nullable(), | |
| access_mode: z.enum(['off', 'ask_every_time', 'full_access']), | |
| outcome: z.enum(['allowed', 'denied', 'executed', 'failed', 'revoked', 'stopped']), | |
| reason_code: identifier, | |
| evidence: auditEvidenceSchema, | |
| }) | |
| .strict(); | |
| export const auditRecordPayloadSchema = z | |
| .object({ | |
| schema_version: z.literal('evaos.mac_access.audit_event.v1'), | |
| audit_id: identifier, | |
| sequence: safePositiveCounter, | |
| previous_record_sha256: sha256.nullable(), | |
| occurred_at: instant, | |
| event_type: z.enum([ | |
| 'pairing', | |
| 'policy_transition', | |
| 'command_decision', | |
| 'command_result', | |
| 'pause', | |
| 'revoke', | |
| 'kill_switch', | |
| 'lifecycle', | |
| ]), | |
| actor: z | |
| .object({ | |
| kind: z.enum(['local_user', 'workbench', 'broker_runtime', 'system']), | |
| identity: identifier, | |
| }) | |
| .strict(), | |
| binding_fingerprint_sha256: sha256.nullable(), | |
| command_id: identifier.nullable(), | |
| request_digest_sha256: sha256.nullable(), | |
| access_mode: z.enum(['off', 'ask_every_time', 'full_access']), | |
| outcome: z.enum(['allowed', 'denied', 'executed', 'failed', 'revoked', 'stopped']), | |
| reason_code: identifier, | |
| evidence: auditEvidenceSchema, | |
| }) | |
| .strict() | |
| .superRefine((event, context) => { | |
| if ( | |
| ['command_decision', 'command_result'].includes(event.event_type) && | |
| (event.command_id === null || event.request_digest_sha256 === null) | |
| ) { | |
| context.addIssue({ | |
| code: z.ZodIssueCode.custom, | |
| message: 'command audit events require command and request-digest correlation', | |
| path: ['command_id'], | |
| }); | |
| } | |
| }); |
🤖 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 `@packages/mac-connector-core/contracts/v1/index.ts` around lines 1129 - 1160,
Update auditRecordPayloadSchema so command_decision and command_result events
require non-null command_id and request_digest_sha256, while preserving nullable
values for non-command event types. Implement the conditional validation within
the schema’s existing validation flow and keep the current field formats and
strict object behavior unchanged.
| mutations: z.array( | ||
| z | ||
| .object({ | ||
| operation: z.enum(['set', 'remove']), | ||
| pointer: z.string().startsWith('/'), | ||
| value: z.unknown().optional(), | ||
| }) | ||
| .strict() | ||
| ), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make fixture mutations a discriminated union.
A set without value and a remove with value both parse, leaving mutation behavior ambiguous.
Proposed fix
- mutations: z.array(
- z.object({
- operation: z.enum(['set', 'remove']),
- pointer: z.string().startsWith('/'),
- value: z.unknown().optional(),
- }).strict()
- ),
+ mutations: z.array(
+ z.discriminatedUnion('operation', [
+ z.object({
+ operation: z.literal('set'),
+ pointer: z.string().startsWith('/'),
+ value: z.unknown(),
+ }).strict(),
+ z.object({
+ operation: z.literal('remove'),
+ pointer: z.string().startsWith('/'),
+ }).strict(),
+ ]),
+ ),📝 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.
| mutations: z.array( | |
| z | |
| .object({ | |
| operation: z.enum(['set', 'remove']), | |
| pointer: z.string().startsWith('/'), | |
| value: z.unknown().optional(), | |
| }) | |
| .strict() | |
| ), | |
| mutations: z.array( | |
| z.discriminatedUnion('operation', [ | |
| z | |
| .object({ | |
| operation: z.literal('set'), | |
| pointer: z.string().startsWith('/'), | |
| value: z.unknown(), | |
| }) | |
| .strict(), | |
| z | |
| .object({ | |
| operation: z.literal('remove'), | |
| pointer: z.string().startsWith('/'), | |
| }) | |
| .strict(), | |
| ]), | |
| ), |
🤖 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 `@packages/mac-connector-core/contracts/v1/index.ts` around lines 1386 - 1394,
Update the mutations schema to use a discriminated union on operation: require
value for set mutations and disallow value for remove mutations, while
preserving the existing pointer validation and strict object behavior.
| function canonicalizeJcs(value: unknown): string { | ||
| if (value === null || typeof value !== 'object') return JSON.stringify(value); | ||
| if (Array.isArray(value)) return `[${value.map(canonicalizeJcs).join(',')}]`; | ||
| return `{${Object.entries(value) | ||
| // oxlint-disable-next-line unicorn/no-array-sort -- Object.entries returns a fresh array required for JCS key order. | ||
| .sort(([left], [right]) => (left < right ? -1 : left > right ? 1 : 0)) | ||
| .map(([key, item]) => `${JSON.stringify(key)}:${canonicalizeJcs(item)}`) | ||
| .join(',')}}`; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Prefer an established RFC 8785 (JCS) library over a hand-rolled canonicalizer.
This function is the independent verifier for cryptographic golden vectors (audit chain, command authority, rollback authorization) — if it has a subtle deviation from RFC 8785 (e.g. IEEE-754 number formatting, non-ASCII key sorting), the test could pass or fail for the wrong reason, undermining confidence in the golden-vector proofs. Well-established, RFC-8785-test-vector-verified packages exist for this (e.g. canonicalize).
♻️ Suggested direction
-function canonicalizeJcs(value: unknown): string {
- if (value === null || typeof value !== 'object') return JSON.stringify(value);
- if (Array.isArray(value)) return `[${value.map(canonicalizeJcs).join(',')}]`;
- return `{${Object.entries(value)
- // oxlint-disable-next-line unicorn/no-array-sort -- Object.entries returns a fresh array required for JCS key order.
- .sort(([left], [right]) => (left < right ? -1 : left > right ? 1 : 0))
- .map(([key, item]) => `${JSON.stringify(key)}:${canonicalizeJcs(item)}`)
- .join(',')}}`;
-}
+import { canonicalize } from 'canonicalize';
+const canonicalizeJcs = canonicalize;🤖 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 `@tests/contract/macAccessCryptography.test.ts` around lines 27 - 35, Replace
the hand-rolled canonicalizeJcs function with the established RFC 8785/JCS
library used by the project, such as canonicalize, and update the cryptographic
golden-vector verifier to call that library directly. Remove the custom sorting
and serialization logic while preserving the verifier’s existing inputs and
outputs.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b0e26adaea
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .object({ | ||
| kind: z.literal('lifecycle'), | ||
| effective_mode: z.enum(['off', 'ask_every_time', 'full_access']), | ||
| pairing_state: z.enum(['unpaired', 'paired', 'revoked']), | ||
| transport_state: z.enum(['disconnected', 'connecting', 'connected', 'revoked', 'blocked']), |
There was a problem hiding this comment.
The remaining lifecycle shape for set_access_mode is target-less: coreHostResponseSchema validates a successful response using only operation: 'set_access_mode' plus this generic effective_mode. As a result, a response to a request whose target_mode was off can still validate with effective_mode: 'full_access', so clients can accept a mode change that did not take effect unless they implement a separate request/response validator outside the frozen contract. Include the requested/configured mode in this result or validate the response together with its request target.
Useful? React with 👍 / 👎.
| }); | ||
| } | ||
| } | ||
| if (transition.event === 'restart') { |
There was a problem hiding this comment.
This validation only checks restart semantics when event === 'restart', so another event such as set_mode can change from.runtime_instance_id to a new to.runtime_instance_id while keeping effective_mode: 'full_access' and updated confirmation fields. That lets a helper restart/crash be represented as a non-restart transition and bypass the required Full Access downgrade/reconfirmation path; reject runtime-instance changes unless the event is restart.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| if ( | ||
| ['pause', 'stop', 'revoke', 'activate_kill_switch', 'shutdown'].includes(response.operation) && |
There was a problem hiding this comment.
Because resume is omitted from this lifecycle guard and has no dedicated check, a successful resume response with effective_mode: 'full_access' still validates. In the paused/full-access case that lets the host report resume completion while restoring remote authority without the reconfirmation required by the access-state machine; add a resume-specific assertion or include enough confirmation state in the response to prove it is safe.
Useful? React with 👍 / 👎.
| request_digest_sha256: sha256.nullable(), | ||
| access_mode: z.enum(['off', 'ask_every_time', 'full_access']), | ||
| outcome: z.enum(['allowed', 'denied', 'executed', 'failed', 'revoked', 'stopped']), | ||
| reason_code: identifier, |
There was a problem hiding this comment.
reason_code is written into the durable audit payload but uses the plain identifier validator, unlike evidence fields that use safeEvidenceIdentifier. If an error mapper or runtime path supplies a secret-shaped identifier such as Bearer-secret-material, the audit event validates and persists it despite the redaction contract; use the same secret-bearing denylist or a closed enum for audit reason codes.
Useful? React with 👍 / 👎.
| command_id: identifier.nullable(), | ||
| request_digest_sha256: sha256.nullable(), |
There was a problem hiding this comment.
For command_decision or command_result audit records, these nullable fields can both be null and the event still validates. That produces a durable command audit entry with no command ID or request digest to correlate with approvals, receipts, replay decisions, or tamper checks; add a cross-field check that command events carry the command identifiers/digests required by the audit contract.
Useful? React with 👍 / 👎.
| events: z.array(auditEventSchema).max(100), | ||
| next_sequence: safePositiveCounter.nullable(), |
There was a problem hiding this comment.
The audit summary response accepts any array of individually valid audit events, so a page containing out-of-order or gapped records such as sequences 5 then 2 still validates. Since this is the host API's bounded audit surface, clients consuming only the DTO can miss deletion/reordering evidence; add a summary-level check that returned events are in ascending contiguous sequence with matching previous-record links and a consistent cursor.
Useful? React with 👍 / 👎.
1 participant
Pull Request
Description
Freezes the v0.1 architecture, identity, authority, migration, and proof contracts for standalone evaOS Mac Access on the exact canonical Workbench baseline:
fff813ef1da6b766ae09344b20021b4a4b0672c4, merge0ac9742cc8c42d777da627adb9cf4179567d1373;5b1308fadc481f83116c54de2b9713ab2363bed2, merge5c86e8e91660772da5b1b6f49b43f2de3afee737;f92d45f984db29c132e65f458df85567f04186ca, merge/base27b28cd234d537a491028e9024070cf8d33b9611;b0e26adaea15714ded504e1b690653ea543b9f25.This PR adds:
This is architecture and source-contract proof only. It is not a native app, signed/notarized Mac Access artifact, TCC attribution result, pristine-Mac proof, deployed relay proof, VM-to-Mac CUA proof, customer-readiness claim, publication, or rollout.
Related Issues
Type of Change
fix- Bug fix (non-breaking change which fixes an issue)feat- New feature (non-breaking change which adds functionality)perf- Performance improvementrefactor- Code restructuring (no behavior change)docs- Documentation and executable architecture-contract updateAtomic PR Checklist (Rule 1)
Local Checks (Rule 2)
27b28cd234d537a491028e9024070cf8d33b9611is an ancestor of this headjust test-contract: 31 passed; zero todos; 10 runtime cases frozen as an explicit Mac Access: extract the owned connector into reusable mac-connector-core #700-Mac Access: make Workbench use the shared runtime without dual ownership #704 proof ledger without claiming A0 executionstopparsedindex.ts83.33% statements, 80.57% branches, 100% functionsbunx tsc --noEmitpassesVITEST_MAX_WORKERS=2 just push: lint/format/typecheck/i18n passed; 303 test files passed, 1 skipped; 2,843 tests passed, 3 skipped; zero todosgit diff --checkCanonical GitHub Actions on
b0e26adaea15714ded504e1b690653ea543b9f25remain the broad remote source of truth and are re-running after review fixes. Local source checks do not prove a built product or live runtime.Runtime Verification
Agent Handoff
100yenadmin/evaOS-GUI; archivedelectricsheephq/evaos-desktop-bridgeremains provenance only.packages/mac-access; single connector source:packages/mac-connector-core; Workbench becomes an authenticated client in Mac Access: make Workbench use the shared runtime without dual ownership #704.Screenshots
N/A — no product UI or built native app exists in this PR.
Additional Context
Independent identity/security, selected-binding/receipt/policy, and migration/coexistence reviews plus CodeRabbit, evaOS review, and Codex review have exercised the architecture. The first current-head review round found nine contract-proof/layout/coverage findings; the later Codex pass found seven operation-outcome/authority/typecheck findings. All sixteen are fixed, answered with evidence, and resolved at
b0e26adaea15714ded504e1b690653ea543b9f25. The final migration inventory also corrected the installed pre/QA canary and production receipt-verifier ownership from test-only to packagedproof/. Runtime rejection cases remain an explicit downstream proof ledger and are not claimed as executed by A0.Thank you for contributing.
Summary by CodeRabbit
New Features
Documentation
Tests