The `readDenyPaths` policy field shipped in the previous commit broke Linux confinement outright. bwrap has to create the `/dev/null` bind's mount point inside a tree its own profile has already made read-only, so it refused the entire confinement whenever the parent directory was absent — every host that has not stored a credential yet, including a fresh install: bwrap: Can't mkdir parents for /home/runner/.dsh/.env: Read-only file system which the executor correctly classifies as SANDBOX_UNAVAILABLE, so every confined bash call failed closed. Landlock cannot subtract from its own `/` read grant, so it reported `partial` enforcement on every confined call for a file it never hid, with no way to switch the denial off (schemastery fills an omitted array with `[]`, so empty and omitted were indistinguishable). A protection that breaks confinement where it works and misreports it where it does not is worse than a documented absence. Revert the field, both expressible backends, the enforcement downgrade, and the policy default; state the residue plainly in the credentials-local READMEs — file mode stops other OS users, not the model — and keep the OS-keychain provider recorded as the real answer. The narrower discipline stands: no surface hoists the credential document into `process.env`, and the model is never handed a resolved path to it.
8.0 KiB
Agent Note: credential boundaries, whole-snapshot requests, and atomic route registration
Status: implemented
English | 中文
Scope: the third review round over the request-level LLM configuration seam — where a stored credential lives and who can read it, how one request's facts stay one generation, and how a route set changes without a window. Companion to the settings write-path note, whose provider fixes this round applies to
credentials-localand whose writer lock it promotes intodsh-atomic-write.
Problem
Review found the credential path leaking across boundaries it had drawn. The shipped surfaces hoisted $DSH_HOME/.env into process.env before cordis booted, so on the next run credentials-local classified every key it had stored itself as a read-only ambient launch override: describe() reported source: 'env' with writable: false, set/unset rejected as shadowed, and a key stored from the web page or TUI became unrotatable and undeletable while the adapter kept using the value captured at launch. The store's own write path repeated the settings-local defects that same review round fixed (two independent chains, whole-file render from a stale cache), plus editor bugs of its own: a physical line inside another key's quoted multi-line value read as an assignment, CRLF endings degraded to LF, a multi-line entry reported writable: true while set always threw, and credentials/updated was emitted bare after the commit, so one broken observer made a durable write look failed. On the read side, the file's 0600 mode stops other OS users but not the model, whose bash and filesystem tools run as the same user.
Two request-path defects sat beside them. DeepSeek's per-request resolution kept connection facts in a last-good snapshot but re-read the literal apiKey from the raw configuration, so a settings generation the resolver rejected could still put its key on the previous generation's endpoint. pi-ai handed the SDK undefined when a configured apiKeyEnv resolved to nothing, letting pi-ai's own environment discovery authenticate with an unrelated provider key — another tenant, silently billed. And its route swap disposed the old registration before creating the new one: a route another adapter owned dropped every existing route, after which the facts cache could equal the registry's, so restoring the working configuration never re-applied.
Decision
$DSH_HOME/.env belongs to the credential provider alone. No surface loads it into process.env. The genuine launch environment and the invoking directory's .env (loaded by the bin) stay the read-only ambient layer, so a composition without the provider resolves keys exactly as before, while a stored key stays file-sourced and writable across restarts — proven by a real restart in the loader composition rather than by a unit assertion about describe().
The stored credential has no boundary against the model, and the READMEs say so. 0600 under a 0700 directory stops other OS users; the model's bash and filesystem tools run as that same user, and the shipped default confines nothing. What the harness does hold to is narrower and stated as exactly that: no surface hoists the document into process.env, and the model is never handed a resolved path to it, so reaching the value takes a deliberate read of a path it was not given. An OS-keychain provider — a store the model's processes cannot read at all — is recorded as the real answer rather than implied by a partial one.
One request, one generation. DeepSeek's resolved snapshot carries the credential facts (literal key and reference) beside the endpoint, and resolveApiKey receives that snapshot instead of re-reading configuration. A rejected generation now contributes nothing at all. pi-ai defers to provider-native discovery only for a profile naming no credential; a configured reference that misses fails with MISSING_CREDENTIAL naming the route and the reference. The boot-time credential probe is deleted: it could run before the credentials service mounted and reported every failure as a missing key, while the first request already gives the accurate error.
Route replacement is a registry operation, not a caller sequence. registerAdapter returns a handle carrying replace(providers): the candidate set is validated in full first (conflicts, names, provider metadata), then swapped in one synchronous section. A refused replacement leaves the previous routes registered and serving, and the caller's facts cache only advances after the registry actually holds the new set, so reverting to a working configuration re-applies. pi-ai's registration facts are sorted by provider, so a settings document that merely reorders its keys is no longer a route change.
Contained publication for committed credential writes. Credentials.notifyUpdated fans credentials/updated out one listener at a time; sync throws and async rejections are logged without changing the committed operation's outcome, and INVARIANT-coded failures rethrow after every listener ran — the same shape the settings seam uses for settings/updated. installSettingsSection's cleanup now distinguishes its two triggers: a provider detaching still falls back to the composition entry and re-derives, while the consumer's own unload returns immediately instead of re-registering routes during teardown.
Alternatives considered
- A sandbox read-denial naming
$DSH_HOME/.env— implemented as areadDenyPathspolicy field (a trailing SBPLdeny file-read* file-write*, a/dev/nullbwrap bind) and withdrawn on its own evidence. bwrap must create that bind's mount point inside a tree its profile has already made read-only, so it refuses the entire confinement whenever the parent directory is absent — every host that has not stored a credential yet, including a fresh install; Landlock cannot subtract from its own/read grant, so every confined call would reportpartialfor a file it never hid. A protection that breaks confinement where it works and misreports it where it does not is worse than a documented absence. Denying the whole harness home was rejected earlier for a separate reason: it also coverssessions/, andDSH_SESSION_JSONLis a documented model-visible capability. - Removing
DSH_HOMEfrom the model's bash environment — considered as defense in depth and rejected as theater with a real cost: the default home is a documented convention the agent can reconstruct, while the variable is how legitimate tooling finds harness state. There is no boundary here for it to complement; hiding the pointer would only make the absence harder to see. - Shipping the OS-keychain provider in this round — it is the only design where the model's processes genuinely cannot read the secret, and it is a sibling package with three platform backends. Sizing it against the rest of this review round would have delayed every other fix; it is recorded as the deferred answer, not as a maybe.
- A
replaceRegistration(previous, next)service method — the review's shape, but it makes the caller carry the previous handle and lets it pass a mismatched one. Hangingreplaceon the registration handle makes ownership structural: only the registration that holds routes can replace them.
Consequences
update()-adjacent behavior gained documented failure modes: a credential write can now fail on the lock deadline or on an unparsable on-disk document, and describe() reports writable: false for multi-line entries it will not rewrite. LlmAdapter registrants keep working unchanged (the handle is still callable as the disposer), and DeepSeekConnectionOptions gained credential fields, so a programmatic constructor of the adapter must supply apiKeyEnv. Deferred: the OS-keychain credential provider, and per-value revision checks for two writers editing one reference (last-write-wins remains the documented resolution).