feat(people): add provider-neutral sweep profiles - #694
Open
salmonumbrella wants to merge 33 commits into
Open
Conversation
roborev: Combined Review (
|
TestCredentialStoreDeleteConsumesGuardWhenProfileNameIsInvalid deadlocked CI: it asserted the surviving credential with store.Load while the PreflightDelete guard still pinned the credential namespace flock. The production contract is deliberate (openExistingCredentialDelete holds LOCK_EX until Close so concurrent setup processes serialize), and flock is bound to the open file description, so the same process blocks on its own open guard. Every other test already closes guards before ordinary store operations. Close the consumed guard first, then load. The load runs bounded in a goroutine so a future Close that leaks the namespace lock fails this test in seconds with a pointed message instead of hanging the package until the go test timeout fires.
With [remote].url configured, person provider add published the provider profile to the local config file and the credential to the local token store, then proxied the mandatory check to the remote daemon. That daemon reads its own host's config and credential store, so the validation could never see the records the CLI just wrote and the whole setup rolled back after touching local state. Reject the operation up front with an actionable message: run setup on the daemon host, or target a local daemon with --local. No credential transport to the remote daemon is introduced; people provider secrets deliberately never cross the daemon request boundary. The new test pins that remote setup performs no catalog fetch, config read, credential publication, or proxy call before refusing.
Generic person provider add demands endpoint and auth values, codex validation forbids exactly those fields, and capability negotiation deliberately excludes the Codex process transport, so the generic path could only fail with misleading errors (required-field complaints or 'codex_app_server does not accept HTTP or credential fields'). Reject --protocol codex_app_server up front with the supported manual path, drop the unreachable codex case from catalog auth resolution, and document the manual configuration: a [people.sweep.providers.<name>] table with model and reasoning_effort but no endpoint/auth/credential keys, followed by person provider login, check, and consent. Note that the Codex transport stays fail-closed until its isolation gate releases a verified build.
The new final identity check in editConfigWithMatch rejected every config creation on Windows with 'committed config identity changed before return' (CI run 32995343496: the CardDAV account-save test in internal/api and eight person-provider tests in the cli shard). Root cause: creation pins the expected mode to 0600, but Go derives Windows permission bits from the read-only attribute only (0666 writable, 0444 read-only) and can never observe 0600. capturePublishedConfigVersion then refused to adopt the published identity and the final comparison failed on both mode and identity, deterministically, for each missing-file publication. Introduce sameConfigModePerm: exact permission equality on Unix-family platforms, where stat observes real bits, and writable/read-only equivalence on Windows, where that distinction is the only observable mode information. The owner-only DACL that actually protects Windows configs is already verified separately by validateOpenedConfigSecurity, and a read-only flip is still detected. Windows regression coverage (edit_mode_windows_test.go) runs the real missing-file creation path and the comparator equivalence classes; it executes on the Windows CI matrix, which is the only place the Windows stat behavior exists. A unix guard test keeps equality strict on platforms that observe real permission bits.
…ential-store test gating codex_app_server validation on the named-profile path requires auth = "none" and credential = "none" explicitly (only the legacy table decode defaults them), so the documented manual example omitted values that Validate rejects. Update docs/configuration.md to include both keys and reword the guidance so only endpoint is forbidden while auth/credential must equal "none"; align the person provider add rejection message and its focused test expectation with the same guidance. Config defaulting behavior is unchanged. Gate the stored people provider credential lifecycle tests on linux/darwin via requireStoredCredentialStorePlatform: the file-backed store needs secure no-follow atomic filesystem operations and fails closed elsewhere (covered by the peoplesweep fail-closed test), so its happy-path transactions can only be proven where the store is implemented. The frontend remove proxying test keeps an env credential and runs on every platform, preserving Windows coverage there; the stored-credential sweep subtest skips only for the stored case.
roborev: Combined Review (
|
Resolve the five content conflicts between the provider-neutral sweep registry and main's person enrichment (kenn-io#686) plus recent fixes: - internal/peoplesweep/config.go: drop peoplesweep.PeopleConfig (moved to internal/config as the Sweep+Enrichment sibling container on main) while keeping the PR's ProviderSelection legacy-table decoding. - cmd/msgvault/cmd/person_provider_daemon_test.go: keep the PR's registry and credential-store daemon harness, reference config.PeopleConfig. - internal/api/cli_allowlist_person_provider_test.go: keep both imports; registry-based provider tests plus main's enrichment allowlist tests. - internal/api/cli_handlers.go: union of both sides' strict command validators and env-forwarding rules. person sweep run still forwards only the configured provider key; person provider check keeps rejecting request-carried credentials (daemon resolves from its own environment and credential store); person enrichment run/suppress forward the suppression key and the named enabled provider key per main. - internal/config/people_sweep_test.go: keep the PR's named-profile, save/reload, legacy-migration, and rejection tests plus main's enrichment sibling test; adapt main's legacy-compat test to resolve the active provider through the registry API.
The default providerStoreOwnedByDaemon probe is compatibility-sensitive, so a live daemon left running across a CLI upgrade reports no runtime and provider use/remove omitted the daemon restart guidance even though that daemon's scheduled sweeps still serve the startup config. Keep the shared ownership dependency unchanged (check/revoke routing must never proxy to an incompatible daemon) and add a separate, smallest liveness signal daemonAliveForRestartNotice built on the established findAnyDaemonRuntime semantics (new context-aware findAnyDaemonRuntimeContext). personProviderMutationScope consults it only when ownership found no compatible daemon, so routing decisions are unchanged while the restart notice now covers the incompatible-live case. Regression test fakes a responding incompatible daemon (real ping endpoint plus runtime record with a mismatched API version, the same pattern as the restore-into-live-home guard tests) and proves use/remove keep the mutation local, proxy nothing, and still print the restart notice.
roborev: Combined Review (
|
Codex app-server profiles configure the bare driver family (codex-app-server-v2) while the driver attests its identity as codex-app-server-v2:<attestation-digest>. The provider check gate compared the two with plain equality, so every codex_app_server profile failed with a mismatched driver version before consent or sweeps could ever succeed. Add peoplesweep.DriverVersionMatches: the configured version is satisfied by an exact match, or for the codex family only by an attested identity whose family prefix matches exactly and whose suffix is a canonical lowercase SHA-256 digest. Digest-suffixed identities stay rejected for every other driver family. The check command now uses it, keeping unsafe attested values failing closed with no recorded check.
Person provider add let the models.dev catalog choose the endpoint that onboarding transmits a credential to: when the operator omitted --endpoint, the catalog suggestion filled it, the key was then read and sent there during capability negotiation, and only later policy validation could fail. A compromised catalog could therefore redirect API keys to itself. Onboarding now pairs a credential only with an endpoint the operator explicitly supplied via --endpoint or with a first-party API host compiled into the binary (IndependentlyTrustedEndpoint: api.openai.com, api.anthropic.com, generativelanguage.googleapis.com over HTTPS with the default port, for their own protocols). The gate runs while the candidate is resolved, before any credential is read, contacted, or published, and credentialless profiles are unaffected. Catalog-driven transport resolution keeps working against first-party defaults, and the same catalog-listed endpoint stays eligible when passed explicitly.
…tests The Windows CI run for this branch failed on five exact points; fix each without skipping Windows coverage: - peoplesweep runner tests now use a local credential constant: the shared credentialCanary lives only in the linux/darwin-tagged credential store tests, so the untagged runner_test.go failed to build on Windows. - retireExactConfigForMissingRestore reopened the rollback target by name with openConfigNoFollow and never compared the opened file against the pinned published identity, so an operator's byte-identical replacement substituted between the initial read and the rollback was quarantined as if it were the published config. Retain through retainWindowsConfigArtifact(current.Path, current.identity), which both pins with attribute-only access and refuses a substituted identity with ErrConfigConflict. - The final-boundary symlink swap surfaces as ErrUnsafeConfigTarget on Windows (the reparse point is rejected as a non-regular file before any identity comparison) while remaining ErrConfigConflict elsewhere; the boundary test now expects the platform-correct sentinel. - The table-edit mode assertion uses sameConfigModePerm so Unix keeps exact 0640 equality while Windows asserts the writable/read-only distinction its stat reporting can actually observe. - The final-read replacement race uses a build-tagged swap helper: Windows publishes the operator's byte-identical replacement through the established ReplaceFileW primitive (os.Rename's MoveFileExW fails with Access denied against the retained live target), other platforms keep os.Rename.
roborev: Combined Review (
|
SaveNew published the credential file and only then pinned its cleanup guard. When pinCleanup failed, the publication was already durable but no guard existed, so the file stranded on disk with no way to retire it through the cleanup path, and a retry saw the record as present and could not create. Add retirePublished to the pinned credential-store root: while the namespace lock and directory FD are still held, it opens the published entry with no-follow semantics, verifies it still carries the exact identity recorded by save, wipes it to a durable empty record (never unlinking a pathname), revalidates the identity, and fsyncs the directory. SaveNew invokes it immediately when pinCleanup fails and joins any rollback failure into the returned error. A narrow failedCleanupPin test hook (nil in production) injects the pin failure for the regression test, which proves SaveNew errors, Load reports ErrCredentialNotFound, and a retry creates successfully.
recordCompletedCall appended a completed usage record before validating the provider response. When the response was untrustworthy (unsafe identity metadata, negative usage, diverged call identities) or unaccountable (overflow-scale token usage), the appended record was handed to failure finalization, which then either wrote untrusted values into durable history as a succeeded provider call or failed outright on the unaccountable actuals — leaving the attempt running and the lease held until expiry, with nothing charged. Validate and account before appending: a rejected response now contributes no completed record, so FinalizePersonSweepFailure marks the started batch failed and conservatively charges its reservation, terminates the attempt, and releases the lease for requeueing. Regression coverage: - worker-level table test proving untrusted responses (unsafe request ID, negative usage, diverged identities) finalize without a completed record while still carrying every reservation, and retain records from earlier trustworthy calls; - end-to-end worker + store + provider test proving an unaccountable usage response leaves no pending lease or attempt and charges exactly the reserved request, tokens, and cost, leaving the person reclaimable.
…ep responses The missing-response-model and mixed-model-version end-to-end tests still expected the provider-reported token usage of rejected responses to reach durable attempt history. Since 74f53cb a rejected response contributes no completed usage record, and failure finalization conservatively charges the call's reservation instead. Derive the expected charge from the fixture: the test provider now captures the exact wire bytes it served, and the expected reservation is computed with EstimateWireTokenReservation against the extraction output-token cap (mirrored as a documented test constant). The mixed-version test adds the rejected call's reservation to the first, trustworthy call's provider- reported usage.
roborev: Combined Review (
|
Person provider add always rewrote people.sweep.provider to the new profile before anyone consented to it, so an enabled scheduled sweep silently switched to the unconsented profile after the next daemon restart and started failing. Add now publishes only the named profile (plus any accepted catalog prices): the operator's active selection and enabled state survive untouched, and selection and enablement stay with `person provider use`, which requires its own exact successful check. An unselected config cannot remain valid once a named profile exists — people.sweep.provider must name a defined profile even while the sweep is disabled — so in that case add still publishes the required selector, which stays inert because the sweep remains disabled; only `provider use` enables it. The success message now points at `provider use`, and add keeps printing no daemon restart advice since a running daemon's scheduled sweeps observe no selection change. Document the boundary in the people usage guide, including the inert first-profile selector.
roborev: Combined Review (
|
The legacy [people.sweep.provider] migration failed on two valid legacy
layouts. A parentless provider header beside [people.sweep.budgets] left
people.sweep defined only implicitly by sub-table headers, and the
selector edit refused to add the explicit parent header; a root dotted
people.sweep.provider = { ... } assignment defined the table through
dotted keys, which an appended header could never replace.
Extend the targeted table editor with a representation-aware insertion:
exact dotted assignments are replaced in place (preserving operator
comments and position), missing keys join the adjacent dotted family,
and only otherwise is the explicit header appended, which stays valid
because TOML permits defining a super-table after its sub-tables. A
table path assigned as a value keeps refusing edits. ETag/atomic
editing, the named default profile fingerprint, enabled/selection
semantics, and operator content are unchanged, pinned by focused
regressions for add and use across both layouts.
InsertOnly semantics were enforced only for explicit table headers. A preexisting table encoded as dotted assignments (for example people.sweep.providers.alpha.model = "...") fell into the insertion path and was silently overwritten instead of returning ErrAmbiguousConfigTarget, defeating the concurrent-add guard for named records. Refuse InsertOnly edits whenever the target table semantically exists (dotted assignment family or inline-assigned value); non-InsertOnly dotted-family editing and migration remain unchanged.
roborev: Combined Review (
|
Convert the internal/store test scopes flagged by testifyhelpercheck to use package-bound helpers (require := require.New(t) / assert := assert.New(t)) instead of repeated direct package calls, matching the style already used across the package. Assertion arguments, messages, ordering, and subtest structure are unchanged; unflagged scopes and the existing checks/must/requirements helpers are left as-is.
roborev: Combined Review (
|
… lease MarkPersonSweepBudgetStarted authenticated the reservation and attempt but never verified the caller still owned a live fenced lease, so a stale worker could flip a reserved batch to running and send it after another worker had reclaimed the person. The store method now receives the caller's peoplesweep.Lease alongside the reservation (WorkStore interface, worker call site, and fakes updated). In the same transaction that transitions the batch from reserved to running it locks the person_sweep_work row (PostgreSQL FOR UPDATE, SQLite writer slot) and requires matching lease_owner and lease_fence with lease_until strictly after the database clock, returning peoplesweep.ErrLeaseLost on missing, mismatched, reclaimed, or expired leases. The lock keeps the established usage -> batch -> work ordering shared with Apply and Finalize, and the idempotent running-batch replay path requires the same live lease. Regression tests prove stale owner, stale fence, expired lease, reclaimed successor, missing work row, and foreign-person leases cannot start a batch (batch stays reserved, accounting untouched), and pin the live-lease success path plus the fenced running replay. The worker fake records the lease it was handed so a test asserts the pre-IO callback passes the owned lease identity.
roborev: Combined Review (
|
roborev: Combined Review (
|
roborev: Combined Review (
|
roborev: Combined Review (
|
roborev: Combined Review (
|
roborev: Combined Review (
|
roborev: Combined Review (
|
roborev: Combined Review (
|
roborev: Combined Review (
|
Contributor
Author
|
@wesm ready |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
provider add,check,consent,use,status, andremoveflows, with credentials kept outsideconfig.toml.Why
#685 established the durable people-sweep worker but left provider setup tied to one transport shape. Named profiles make that boundary reusable across protocols while preserving explicit consent, hard budget fences, and fail-closed credential handling.
Usage
Closes #693