Repository navigation
Fix v0.3.0 pre-release review findings in agent group writes #171
Description
Goal
Fix the findings of the v0.3.0 pre-release review (#169) before the release: an existence oracle for agent-invisible group names, a lost-update race between App group edits and agent group writes, two cancellation defects, and a stale ADR sentence.
Context
Independent read-only review of v0.2.0..3d63999 (Codex gpt-6.1-sol), each finding confirmed by the planner against the code. Builds on #161 (PR #163) and #162 (PR #166); ADR 0010.
Executor: Codex · gpt-6.1-sol · xhigh. risk:security.
Decisions
F1 — Agent-invisible group names behave as absent before approval. Today create_group with a name, or rename_group onto a name, that only Hidden or recycled credentials use is rejected at freeze, while a truly absent name gets an approval ticket. An agent can tell the two apart without any approval. Change #162's "renaming onto any occupied name rejects" rule:
- A name used only by agent-invisible members (Hidden or recycled) freezes exactly like an absent name. Empty stored groups are already listed in the catalog, so they count as visible. The agent-observable result (ticket, response fields, errors) must be identical in both cases.
- The approval card shows the truth to the maintainer. For
create_group, it shows that the group already exists, with its not-visible member count. Forrename_grouponto such a name, it shows that the source merges into the existing group, with both member counts. Commit then merges: renamed members take the existing spelling. Creating an existing group is a no-op. - A name the agent can see (in
list_credentials.groups, including empty stored groups) is still rejected for create and rename, since there is nothing to hide. - The digest binds the resolved target state, and commit re-checks it. If the hidden state changes after freeze, the commit fails closed, as it does today.
create_credential/modify_credentialwith such a group name already reuse it silently, which is consistent; keep it.
F2 — App group edits cannot overwrite agent group writes. Vault.createCredentialGroup reads and writes the stored group list in separate statements without the agent-access gate, so an agent commit landing between them is lost (or an empty group it removed comes back). Make every stored-group writer (App create/delete, #161 metadata commit, #162 organize commit) read, merge and write the list inside one SQL transaction. Take the exclusive gate where the other App mutations do.
F3 — Expiry rejections in write commit use expiry cancellation. In Vault+AgentWriteCommit.swift, both the frozen-deadline check and the storage expiry rejection call cancelAgentWrites, which uses ordinary cancelPending and so cancels a valid rename/delete-only batch that includes the credential. Use cancelPendingForExpiry for these expiry paths and keep ordinary cancellation for real mutations.
F4 — Cancellation cannot race a commit. The operation-bound BrokerApprovalStateMachine.cancel(requestID:capability:operationRequest:) ignores entry.committing, and organization cancellation is not serialized with organization commit. Refuse cancellation once consumption is reserved, as the generic overload does, and serialize organization cancel with its commit. No response may say cancelled for a write that then commits, and an operation must not get both denied and allowed access records.
F5 — ADR 0010. The Decision section still says batch rename and delete are deferred to #162. Rewrite it in past tense: #161 added the metadata fields, and #162 added batch organization.
Scope
May change: Sources/AskKeyVault/Vault+AgentOrganizationFreeze.swift, Vault+AgentOrganization.swift, VaultStore+AgentOrganization.swift, FrozenAgentOrganization*.swift, Vault+CredentialGroups.swift, VaultStore+AgentWrites.swift, Vault+AgentWriteCommit.swift, Vault+AgentWriteRequest.swift, related store/config queries; Sources/AskKeyBroker/BrokerApprovalStateMachine+Requests.swift, BrokerOrganizationSummary.swift; Sources/AskKeyAppKit/App/FrozenOrganizationApprovalContent.swift and its Localizable.xcstrings entries (English and Simplified Chinese, using 凭证 and Agent); docs/adr/0010-agent-credential-metadata-writes.md; docs/features.md; tests in Tests/AskKeyVaultTests/, Tests/AskKeyBrokerTests/, Tests/AskKeyAppTests/; the CI organization screenshot fixture in Tests/AskKeyTestSupport/E2EBrokerScenario.swift and its assertions in Tests/AskKeyE2ETests/ScreenshotE2ETests.swift (CI only), plus its entry in the scripts/e2e-report.py screenshot allowlist.
Must not touch: runtime delivery and run; permissions; the catalog's visibility rules; BrokerProtocolVersion; product identifiers; the App's group UI beyond the transaction fix.
Acceptance
swift build
swift test --filter <changed suites>
python3 scripts/check_hygiene.py && python3 scripts/check_module_deps.py- F1: a test sends identical
create_groupandrename_groupbatches against a hidden-only name, a recycled-only name, a stored name whose only members are agent-invisible, and an absent name, and asserts identical agent-observable outcomes. Merge and no-op commits are correct, the card copy covers both languages, and a visible name is still rejected. - F2: a deterministic interleaving test, in which an App create or delete overlaps an agent organize commit and a Let agents set usage instructions and groups when they write credentials #161 metadata commit, loses neither side's groups and resurrects no removed group.
- F3: a synthetic-clock test with an already frozen single write and a rename/delete-only batch keeps the batch when the single write's commit is rejected for expiry, including expiry found at transaction validation.
- F4: a blocked-transaction test shows that cancellation during a reserved commit does not report
cancelled, and that the operation ends with exactly one outcome and consistent access records. - CI
build-and-testandbasic-ui-flowsgreen; if the organization card changes, the screenshot test itself passes and the image is inspected.
Blocked by
None. Blocks #169.
Activity
Fix agent organization privacy, transactions and cancellation (#172)