The layered-env reader gained an unreadable-layer path, a default
reporter, and two absent-layer arms with no cases; the credential store
gained two error paths that must not be mistaken for an absent file.
The platform arms and the `linePos` guard cannot be reached from a POSIX
test run — the first is covered by the native Windows job, the second
only satisfies an optional type that `prettyErrors` always fills — so
both carry a v8 ignore naming why.
Two had real security consequences:
The bootstrap rejection ran on npm dotenv's parser while process.loadEnvFile
applied the file with Node's own. Two independently maintained dialects meant
the check and the thing it guards could disagree: a name Node accepts but the
checker misses would reach process.env unchecked, and BASH_ENV there runs a
file of the project's choosing on every `bash -c` the bash tool issues. Parse
once with node:util's parseEnv — the same engine loadEnvFile uses — and assign
the entries already checked, which also drops the dotenv dependency.
llm-pi-ai still returned a literal profile.apiKey ahead of everything, and it
registers a settings namespace, so the defect removed from llm-deepseek
survived intact in its design twin. The field is gone from the profile schema,
the resolution path, and the tests.
The rest are consistency and documentation defects the review named:
- verify-config-source-ownership did not scan the Python runtime's bundled
cordis.yml, which still inlined apiKey and baseURL. Both are covered now, and
the line-anchored INLINE_DENY documents that it is a tripwire, not a parser.
- The deny list missed NODE_TLS_REJECT_UNAUTHORIZED, the askpass hooks, the
GIT_CONFIG_* redirections, and PYTHONHOME — all implied by its own stated
rule about what a variable does.
- Snapshot lookups folded case on Windows, where environment names are
case-insensitive and an exact-match Map could miss a higher-ranked layer.
- The credentials note claimed a read-time permission check was "not taken"
while this PR implemented it; the credentials-local README still described
two layers, live process.env reads, dotenv-era limitations, and a renamed
anchor; the llm-deepseek README still advertised the removed literal apiKey;
and web.ts and base.cordis.yml kept personal-overlay wording.
- The ownership note's literal-apiKey claim now names its scope: the
web-search providers keep a literal field but register no settings
namespace, so nothing can shadow a stored credential through them.
Master removed the TUI package, the `meta` and `upgrade` subcommands, and
`--config-replace`, and made raw `dsh` require a `--config` overlay. Resolved
onto that shape:
- Dropped this branch's TUI edits with the surface itself, including
`tui.cordis.yml`, `runTui`, and the TUI keyless PTY smoke.
- Dropped the `--config-replace` plumbing rather than reintroducing a flag
master deliberately removed. The gap this branch fixed remains: `dsh -p`
still could not name its composition, so it keeps `--config`.
- Kept this branch's deletion of the personal `$DSH_HOME/config.yaml` layer,
which master still carried, and provided the environment snapshot in the new
raw `runConfig` surface alongside web and headless.
- Ported the headless shutdown PTY test off the personal overlay onto a named
`--config` file, which is what proves that flag now exists on `-p`.
$DSH_HOME/.env had just become an ordinary environment layer, which left the
harness resolving user-facing values from a flattened process.env that could
no longer say where a value came from. A key stored through the web page
stayed shadowed by an older key in the user's own .env. An endpoint could be
redirected by the project: the invoking directory's .env is materialized like
every other layer, and a base URL decides where a resolved API key is sent, so
a DEEPSEEK_BASE_URL written into a model-editable workspace would send the
user's credential — and the prompts carrying their code — to whatever host
that file named.
Give every user-facing value one ordering, with four kinds of source:
explicit for this run per-operation override, CLI argument
> authored by deployment --config / --config-replace
> this launch's shell inherited process environment
> product-managed store settings.yaml, .credentials.yaml
> discovered file $DSH_HOME/.env
> defaults schema default, shipped base, public default
The domains differ only in which tiers exist. The earlier split — credentials
ranking the environment over the managed file while settings ranked over the
environment — was inconsistent: the distinguishing fact is who authored the
source, not the domain.
packages/util/environment owns an immutable snapshot with per-layer
provenance. getFrom(name, sources) searches only the layers a caller names,
and omitting one is a refusal rather than a demotion: the adapters ask for
['process', 'user-env'], so no reordering can let a project file back into a
decision it was excluded from.
isBootstrapOnly rejects, before anything is materialized, any .env setting a
variable that governs how a process launches (PATH, SHELL, NODE_OPTIONS,
LD_PRELOAD), where code or model-visible instructions load from (the whole
DSH_* namespace, HOME, XDG_*), or how the network is reached (proxy and CA
variables). The namespace is denied wholesale so a switch added later cannot
become settable by being forgotten, and there is no opt-out.
verify-config-source-ownership keeps both rules: no unregistered process.env
read under packages/*/*/src (26 allowlisted with reasons), and no apiKey,
baseURL, or headers inlined from the environment in shipped Cordis config —
removing those inlines is what makes the deployment tier meaningful.
$DSH_HOME/config.yaml was an implicit composition layer: if the file existed,
every launch applied an arbitrary Loader patch graph over the shipped tree,
kept live by a dedicated HMR watcher. Three costs came from the implicitness,
not the capability. A patch replaces its target row's whole config, so a file
written months ago pins that row to the field set it knew and every default
the shipped tree later adds silently stops applying. It competed with the
typed settings namespaces llm-deepseek and llm-pi-ai already register, so
which one wins was a function of layer order rather than meaning. And the
explicit escape hatch it was supposedly redundant with did not exist on every
surface: dsh -p, dsh meta, and dsh upgrade all rejected --config, so for them
the implicit file was the only composition route at all.
Complete the explicit layer first: --config and --config-replace now work on
every booting surface. A headless --config-replace tree must still mount a
webserver row, because that surface reaches its own agent over the same HTTP
gateway the browser uses; AppCLIEntry names that contract in the failure
instead of reporting a bare missing service.
Then delete the implicit one. PERSONAL_CONFIG_FILENAME, loadPersonalPatches,
watchPersonalPatches, and the config-only HMR row mounted for it are gone; a
file left at that path is inert, and --dump-config no longer reads the Harness
home. --config therefore stops *replacing* the personal overlay and simply
*is* the user overlay.
No migration: a user who wants the old behavior names the same file
(dsh --config ~/.dsh/config.yaml), which a shell alias makes permanent.
$DSH_HOME/.env carried two incompatible jobs. As credentials-local's writable
secret store it could not be hoisted into process.env — hoisting makes every
stored key read as a read-only launch override and blocks rotation from the
TUI and the web page. But its name and dotenv format promise an environment
file, so a DEEPSEEK_BASE_URL sitting beside a working DEEPSEEK_API_KEY in the
same file was silently ignored: only the credential provider read the
document, and it addresses credential references alone.
Split the two jobs into two files.
.credentials.yaml is the provider-managed store: a strict YAML mapping of
CredentialRef to non-empty string, no version field, no wrapper level. Because
it holds credentials and nothing else, a non-mapping root, a non-identifier
key, a non-string value, an empty string, a duplicate key, and malformed YAML
are all rejections rather than skipped entries — loud at boot and at a write,
warn-and-keep-last-good on a live reload. The dotenv physical-line editor
gives way to a patch of the parsed document, so comments and untouched entries
keep their formatting and any string value round-trips, multi-line included.
Writer lock, read-modify-write, atomic 0600 write under a 0700 directory,
watcher, self-write suppression, and quiescent disposal are unchanged.
$DSH_HOME/.env becomes the user's ordinary environment layer. app-boot's new
loadLayeredEnv loads the invoking directory's .env then the Harness home's,
giving user < project < inherited; the home resolves from the inherited
environment first, so a project .env cannot redirect it.
Credential precedence is unchanged: the live environment still wins read-only
over the file, and shadowed writes still reject. Whether a provider-managed
store should instead win over the environment is a separate decision.
No migration: a key already in $DSH_HOME/.env keeps resolving through the new
environment layer, as a read-only env source that shadows the stored one.
Resuming a long session (196k events, 2.2k steps, 1.8k tool cards) took
~12s to render and ~800ms to echo one keystroke:
- Every step's timing footer called stepTimingAt, which replayed the whole
event log per footer - O(steps x events) on the initial render.
- pi-tui re-renders every component each frame and relies on per-component
line caches, but ToolCardComponent/ContextCardComponent built throwaway
Text/Markdown instances inside render(width), re-wrapping every settled
card's output on every keystroke.
Replace the per-footer replay with one shared StepTimingTracker per chat
mount (single O(events) cursor over the append-only log), and cache card
rows by width via CardLineCache, dropped by every state mutator and
invalidate().
Measured (tmux 200x50, 196k-event session): resume prompt-ready ~12s -> ~7.6s;
per-keystroke echo ~800ms median -> ~11ms.
session-title already registers a title projection unit; /resume now
reads it instead of scanning logs: live rows from the registry
snapshot, persisted rows from the durable checkpoint row
(cachedSnapshot, zero I/O), and only rows without a usable checkpoint
pay a coldSnapshot — checkpoint plus readFrom tail, written back so the
next scan is metadata-only. Cold reads are bounded by the new
resumeScanConcurrency config; compositions without the cache fall back
to the bounded readTitleSnapshots batch. The TUI overlay mounts the
projection registry, storage, and projection-cache rows over the same
storages root the web surface uses, so checkpoints serve both.
Rows no longer read logs for anything but the batch title fold: the
activity timestamp is a live session's last in-memory event time or the
artifact mtime via the optional sessionPersistence.locate(), falling
back to creation time; the last-turn, route, and goal columns are gone.
Route availability moves to the Enter-time preflight, which already
fully reads and replay-validates the one chosen log. The projectSessions
public API this PR had added to session-query is reverted — the change
is now confined to the TUI package.
Master's transactional loader made the invalid-provider PTY case regress:
the HMR main watcher's initial scan refreshed the include mid-initial-apply,
the concurrent group updates stranded the include fiber, and once serialized
the failing apply's rollback deadlocked on HMR's refresh drain — dsh exited
13 with no diagnostic and the terminal stranded, the exact symptom this
branch fixes. Serialize every include child-tree mutation through one queue
and pass ignoreInitial to the HMR main watcher; the failing boot now settles
through boot()'s labelled rejection with the tree disposed and exit 1. The
PTY case asserts the settled diagnostic; the fail-loud release remains the
guard for rejections boot cannot see.
Conflicts: apps/cli/src/tui.ts (keep the release install over master's comment
rewording), packages/ui/app-boot/README* (master's new installFailLoud row
wording plus this branch's release and timeout rows).
CI caught a startup race the PTY smoke stages naturally: a TUI /exit typed
while watchPersonalPatches is still opening its watcher disposes the tree,
and the HMR effect registration rejects with INACTIVE_EFFECT, crashing an
app that exited exactly as asked. Return a no-op disposer for that exact
code; every other registration failure still propagates (covered both ways).
ds-review-bot round 1 on the DSH-home integration:
- generated wrappers now inject the services their manifest needs (skills/
tools beside loader), and loadPreparedRepository rejects a wrapper fiber
that settles anything but ACTIVE — a composition missing a required
service fails the repository transaction instead of committing an ACTIVE
row over a silently PENDING child (critical finding)
- the github: source ref segment excludes '#', so 'a#b' refs fail at the
config parser with the promised syntax instead of inside pnpm
- watchPersonalPatches re-reads the include's non-patch options per refresh
instead of a registration-time snapshot
- the TUI smoke's cache-seeded wrapper is produced by the real
prepareDshPlugin (cache LAYOUT stays a deliberate external pin)
- new Loader integration test drives a live repositories update through
entry.update: generation swap, old skills removed, failed candidate
rolled back to the previous generation
ds-review-bot round 1: boot() now throws `host preparation failed` when
prepare() rejects before any config-tree entry mounts (the plugin-tree label
overstated), and the new hygiene gate verify-vendored-links pins the
linkWorkspacePackages fix — every vendored package name in pnpm-lock.yaml
must resolve to a workspace link with no registry copy alongside.
The timeout promise's executor runs synchronously while the race is
constructed, so the timer is always assigned; the undefined check was a
dead branch the per-file coverage gate rejected.
The PTY capture does continue past the terminal-takeover bytes with the
fatal diagnostic; only the reset never follows. State that precisely in
both notes.
Document on FailLoudProcess.exit that callers treat it as the end of the
run, matching how the release path already relies on it.
Review of the previous commit found two defects in the release path, both
reproduced against the implementation:
- The timeout guarding a never-settling release was unref'ed. An
unhandledRejection listener suppresses Node's default fatal exit, so with
nothing else referenced the process reached an empty event loop and exited
0 on the very failure it was reporting. Keep the timer referenced and clear
it once the race settles.
- The handler uninstalled itself before awaiting the release. A second
concurrent rejection then became uncaught and killed the process
mid-teardown, stranding exactly the terminal state this restores. Replace
the uninstall with a latch: the first rejection is the reported one, and
later rejections (teardown's own included) fall through to the pending exit.
Add the PTY regression the fake-process tests cannot express: boot the shipped
tree over a fixture whose llm-pi-ai providers value is list-shaped, expect exit
1, and assert the captured bytes carry both the diagnostic and ESC[?2004l.
Against the pre-fix source the stream ends at ESC[?2004h ESC[>7u ESC[?u ESC[c
with no reset and the case fails, so it pins the actual bug.
Split the two-shape formatting test into one install per case; a latched
handler reports once by design.
A dsh launch whose config failed validation returned the user to a broken
shell: typing was invisible and the next command was mangled by a stray
Device Attributes reply (1;2;4cecho ...).
The Loader mounts entries concurrently, so ui-tui can already hold the
terminal (raw mode, bracketed paste, keyboard protocol, plus an in-flight
DA query) when a sibling entry rejects on its own config. installFailLoud
wrote its diagnostic and exited immediately, so nothing disposed the tree
and ProcessTerminal.stop() never ran.
Give installFailLoud an optional release teardown, awaited between the
diagnostic and the exit and bounded by FAIL_LOUD_RELEASE_TIMEOUT_MS. The
TUI launcher passes one that disposes the root context, reaching the same
shutdown() the /exit path already uses (drainInput() + ui.stop()). The
context is captured in boot()'s prepare hook because the rejection arrives
while boot() is still in flight.
Bins that pass no release keep the previous behavior exactly.
