feat: bootstrap Psyche Rust workspace, daemon, CLI, and distribution - #1
Conversation
Follow-up to the workspace skeleton, applying accepted review feedback. resolver 2 -> 3: resolver 3 is MSRV-aware, honoring the rust-version field when selecting dependency versions. clap, assert_cmd, and toml all declare rust-version 1.85 -- exactly our pin, so we have zero headroom. Under resolver 2 a routine `cargo update` would happily select a release requiring a newer compiler and break the build against our pinned toolchain, with a failure that points at a dependency rather than at the update that caused it. Verified empirically on the pinned cargo 1.85.0: resolution reports "Locking N packages to latest Rust 1.85.0 compatible versions". toml 0.8 -> 1: the 0.8 line is dead. toml 1.x is the maintained release series, and pinning a pre-1.0 range means the caret constraint stops at 0.8.x and silently misses every fix landing upstream. Resolves to 1.1.4. strip = true -> "debuginfo": `strip = true` is an alias for stripping symbols, which removes the symbol table in addition to debug info. This daemon ships over npm to machines we do not control, so the only diagnostic we get back from a field panic is the backtrace the user pastes -- and without a symbol table that backtrace is unresolved hex addresses. Keeping symbols costs a modest amount of binary size and buys readable crash reports. Debug info, the actually large part, is still stripped. rust-src component: rust-analyzer needs the stdlib sources to offer completion and go-to-definition through std. Without it every contributor hits the same silent degradation and has to discover the fix themselves. .gitignore: `*.log` was unanchored, so it matched log files at any depth -- including any a crate might legitimately want to commit as a test fixture. Anchored to `/*.log` for the repo root, where stray logs actually land. Added `.env*` and `*.tgz` for the npm-side packaging work. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The root manifest pre-declared all four planned crates in `members`, but only `psyche-core` exists. Cargo loads every declared member's manifest on ANY command, so the absent three made the entire workspace uninvokable: `cargo test`, `cargo clippy`, and even `cargo metadata --no-deps --manifest-path crates/psyche-core/Cargo.toml` all aborted with "failed to load manifest for workspace member". Neither `--no-deps` nor `--manifest-path` avoids it — both still walk up to the workspace root and resolve the full member list. `members` now names exactly the crates that exist; each later task appends its own crate as it creates it. Also commits the lockfile, which could not be generated before this fix and which belongs in VCS for a workspace that produces binaries.
Edition 2024 is stable as of Rust 1.85, which is already our pin, so
adopting it costs nothing today; deferring would mean migrating four
crates of real code later, including the 2024 `if let` temporary-scope
change that alters when guards drop across awaits.
Workspace manifest:
- `publish = false`; this is distributed via npm and must never reach
crates.io.
- Dropped the `psyche-config` and `psyche-runtime` path entries. Same
latent-break class as the pre-declared `members` list: a path that
names a nonexistent crate breaks the workspace the moment anything
references it. Each task adds its own entry.
- Added `missing_docs`, `unreachable_pub`, `unused_qualifications`, and
`rust_2018_idioms` to the rust lints, and `unwrap_used` /
`expect_used` as clippy denials — the highest-value pair for a
long-running daemon. New `clippy.toml` permits both in tests.
Deliberately not `clippy::pedantic`.
`SchemaError::UnsupportedVersion` drops its `expected` field: it was
always the const, and a public field let callers construct a state that
cannot occur. The format string now renders `{found:?}`, so a
hand-edited `" psyche.config.v1"` is visually distinguishable from the
accepted value and untrusted text cannot inject newlines or ANSI
escapes into a log line.
`lib.rs` drops the flat re-exports and the glob. One public path per
item: flat re-exports would give every type two spellings to drift
between, and the glob would silently promote anything later added to
`secret.rs` into the public API. This also retires the `unused import`
warning the empty stub was producing.
Tests now pin the version literal (not the const, so a typo in the const
fails rather than redefining the on-disk format), the operator-facing
error text, a table of near-misses that would break G2 denial if anyone
added `.trim()` or case-insensitive matching, and a compile-time
assertion that the error is Send + Sync + 'static for tokio task
boundaries.
`serde` is dropped from psyche-core; nothing used it. Task 3 adds it
back if `SecretRef` needs it.
Satisfies the newly adopted `missing_docs` policy for SchemaError, its UnsupportedVersion variant, and the variant's `found` field.
The previous check accepted any value containing `://`, which blocks a bare credential but not one carried inside a URI — `https://host/bot<token>/send` and `https://user:pass@host/path` both passed. That is the likelier paste, since it is the form API docs show. Validation now matches against an allowlist of secret-store schemes (`op://` today) and rejects a scheme with no path, so a bare `op://` fails at config load rather than at resolution time. Both error variants stay payload-free: the rejection path is where a real secret is most likely present, so the rejected value is dropped rather than echoed.
The scheme allowlist accepted any path after `op://`, so `op://VAULT/ITEM` with no field, `op://VAULT//field` with an empty segment, and references carrying surrounding whitespace or control characters all passed. Control characters in particular would reach a resolver's log line as injection. Validation now requires three non-empty `/`-separated segments, rejects surrounding whitespace rather than trimming it — storing something other than what the operator wrote is worse than telling them — and caps length to bound the blast radius of any future redaction bug. More importantly, `#[serde(try_from = "String")]` had no test coverage. Deleting that line left every test passing while `SecretRef` silently accepted any string through serde, which is the crate's actual consumption path. `deserialising_goes_through_validation` now pins it, and the no-echo test checks Debug as well as Display, since Debug is what panics and `tracing`'s `?err` print.
SecretRefError's payload-free variants guarantee that a rejected value is never echoed, but that guarantee ends at this type and the doc did not say so. The configuration contract uses TOML, and `toml::de::Error` is the opposite of payload-free: its `Display` echoes the offending source line verbatim, and its `Debug` carries `input: Some(<the whole config file>)`. A config loader that logs one with `tracing::error!(?err)` would emit every secret in the file, not merely the value that failed. The prior serde test proved the property against JSON, which is not the format that will run. The doc now scopes the claim and tells the loader author what not to do, which is the durable artifact of this finding — Task 4 writes that loader. Also records why `Serialize` is absent, so a later `config show` does not reach for a derive. Separately, `char::is_control` is Unicode `Cc` only, so the `Cf` format characters passed: `op://VAULT/ITEM/fi<U+202E>eld` and `op://VA<U+200B>ULT/...` both constructed. These cannot inject ANSI or break log framing the way `\n` and `\x1b` can, but a bidi override makes a path render as something other than what it resolves to, and no legitimate vault path contains one.
MalformedPath's doc comment and its operator-facing message both listed control characters only, but the validator also rejects the Cf format characters that enable visual spoofing. The doc under-claimed rather than over-claimed, so nobody was misled into trusting an absent guarantee, but a doc disagreeing with its code is the defect class this file has already been reviewed for twice.
`reduce_toml_error` guarded the failure path, but not the success path. `extensions` is an untyped `toml::Table`, so a derived `Debug` printed whatever it held — including a secret placed there by a future extension — on `tracing::debug!(?config)` after a config loaded successfully. Replaced with a manual impl that reports only the key count. Also qualifies the `ConfigError::Parse` detail doc (file-free, but serde's `invalid type` diagnostic embeds the offending scalar) and corrects docs/CONFIGURATION.md, which asserted a secret-literal rejection no field in this release is typed to enforce.
`Config` was `pub`, derived `Deserialize`, and had all-`pub` fields, so the
version gate was bypassable: a consumer writing `#[derive(Deserialize)]
struct Daemon { config: Config }` produced a `Config` that never called
`ensure_schema_version`, with no compile error and no failing test. The
derive now lives on a private `ConfigRepr`, so `load_str`/`load_path` are the
only routes to a `Config`, and `schema_version` becomes an accessor returning
the one accepted constant rather than a writable field — leaving no
unvalidated state a struct literal could forge.
`rejects_an_unknown_top_level_field` was broken: it appended its key after
the `[coven]` table, so it exercised `CovenConfig`'s `deny_unknown_fields`,
never `Config`'s, and passed only because the assertion matched either way.
The fixture is split so keys inject at document scope, and the test now
asserts on the expected-field list.
Also: an `Extensions` newtype keeps `toml` out of the public API and moves
redaction to where the untyped data lives; extension keys must now match
`<namespace>.<name>.v<N>`, which the docs claimed but nothing enforced;
parse errors carry line, column, and originating path; `load_path` refuses
files over MAX_CONFIG_BYTES.
`Extensions::get` called `err.message().to_string()` inline rather than going through `reduce_toml_error`, so `grep reduce_toml_error` no longer found every place a `toml::de::Error` crossed the boundary — the greppable invariant that motivated the function in the first place. The reduction is extracted into `detail_from`, which both sites now call. `reduce_toml_error` keeps its job of adding line, column, and originating path, but is the file-loading path only; `detail_from` is the exhaustive one, and the `ConfigError` doc comment now names it as such. Also notes that the reported column is a byte offset within the line, not a character offset, so it can skew on lines containing multibyte text.
`MAX_CONFIG_BYTES` enforced nothing. `std::fs::metadata(path).len()` returns 0 for any non-regular file, so the check compared `0 > 1048576`, passed, and let `read_to_string` run unbounded. Demonstrated with a FIFO: 2 MiB written past a 1 MiB cap. `/dev/zero` is worse — it also reports size 0 and its NUL bytes are valid UTF-8, so the read never terminates. That is precisely the out-of-memory case the comment claimed to prevent, and there was a TOCTOU window between the stat and the read besides. The metadata call is dropped. `File::open` then `take(MAX + 1)` bounds the read itself, so the cap holds for streams with no stated size and there is nothing to race. `bytes` in `TooLarge` is now a lower bound rather than a true size, which the field doc and the error wording say. Also: `Extensions::get` names the offending key; `ConfigRepr.schema_version` uses `#[expect(dead_code)]` rather than a second `ensure_schema_version` the reviewer showed to be unreachable; the span slice uses `raw.get(..)` so an off-boundary index degrades to no-position instead of panicking a daemon; and three doc corrections, including the versioned-key rule, which is "one or more dotted segments then `.v<digits>`" rather than the three-segment form both the doc comment and CONFIGURATION.md implied.
`psyche-runtime` is the composition root: it owns the `Config` and the only shutdown path. `Running -> Draining -> Stopped` is recorded as an ordered log rather than a current-state field alone, because the guarantee graceful shutdown owes an operator is that draining happened *between* running and stopped — a final-state assertion cannot tell that apart from skipping the drain entirely. Three departures from the drafted shape, all in the internals; the public surface and its contract are unchanged. The transition log is bounded by construction. `Lifecycle::advance` refuses any move that is not strictly forward, so with three states the log holds at most three entries for the life of the process. An unguarded `push` grew it on every rejected shutdown, which in a daemon that is signalled repeatedly is a slow leak in the one path that must not fail. State and log live under one mutex, and `shutdown` claims the transition with a single acquisition. Reading the state and then transitioning in a second acquisition let two concurrent callers both observe `Running` and both drive the machine — harmless while the drain seam is empty, and a double drain the moment it is not. Whichever caller `advance` returns `true` to owns the shutdown; the rest get `AlreadyStopped`. Lock poisoning is recovered with `PoisonError::into_inner` rather than `expect`, which the workspace denies outside tests. Both critical sections are a field write plus a bounded `push`, so the guarded value cannot be left half-written, and a daemon that panics on the way down leaves its socket behind. `Draining` is kept despite being instantaneous in this slice. It is the state `psyche status` reports and the seam the store and lease work attaches to; removing it would make that a breaking enum change later. Tests cover the ordering, the log bound under a thousand rejected shutdowns, single-winner election across sixteen concurrent callers, the forward-only invariant, survival of a poisoned lock, and that `Runtime`'s derived `Debug` cannot print an extension secret — the last asserted here rather than assumed, since the derive is sound only on the strength of `Config` redacting its untyped extensions table. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`concurrent_shutdowns_elect_exactly_one_winner` did not detect the regression it exists to prevent. Against a `shutdown` that tests the state and then transitions in a second lock acquisition — the exact race the single-acquisition claim was written to close — the test passed 0 times out of 60. It proved sixteen *sequential* shutdowns yield one `Ok`, which a racy implementation satisfies just as well. The cause is that `shutdown` has no await inside its critical region. Tokio tasks therefore run to completion before the next is polled, on any worker count, so the callers never overlapped. Worker threads were not the missing ingredient; suspension points were, and there are none to add. Replaced with OS threads released together by a `std::sync::Barrier`, which is the right primitive for contending on a synchronous critical section. Each thread builds its future before the barrier so the released work is the contended part, and `Handle::block_on` drives it on the calling thread rather than handing it to a worker. Mutation-tested rather than argued. The new form detects the two-acquisition race 60 times in 60 runs, and the wider drafted `== Stopped` variant 60 in 60 — the latter reporting up to three simultaneous winners. Detection is sensitive to the constants: at 8 threads and 200 rounds it falls to 42/60, so both figures and the instruction to re-run the mutation before lowering them are recorded at the test. 16 threads times 1,000 rounds costs 0.26s, and the correct implementation ran it 20 times with no flake. The implementation is untouched; this commit is the test body only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A mutation pass over the security-critical tests broke each property in turn and checked that a test went red. Thirteen of seventeen mutations were caught. These four tests cover what was not. parse_errors_never_echo_the_offending_source_line pins that detail_from never renders a TOML source line. Swapping err.message() for err.to_string() left the whole suite green while the error printed the offending line verbatim, secret and all — detail_from is the invariant this crate's docs tell review to grep for, and it had no test. Every existing parse test asserts what the message contains, never what it must omit. Covers all three call sites: load_str, load_path, and Extensions::get. refuses_a_stream_whose_metadata_understates_its_size pins that the size cap trips on bytes read rather than stated size. Both existing size tests use regular files, whose metadata is accurate, so neither can tell a bounded read from a metadata check — reverting take(MAX + 1) to metadata().len() left both green. A FIFO reports len() == 0 and yields 2 MiB. Asserting bytes == MAX_CONFIG_BYTES + 1 is what makes this a bounded-read test rather than another size test. The compile_fail doctest on Config pins that a consumer cannot deserialise one, and so cannot obtain a Config that never ran ensure_schema_version. This is a compile-time property with no runtime witness: restoring the derive compiles clean and leaves every test green. Verified against the full two-step, since the barrier is two derives deep — Config alone does not compile without Extensions. rejects_an_over_long_but_otherwise_valid_reference pins that the length cap is what refuses a long reference. The existing test's sample is also malformed — one segment, not three — so removing MAX_REFERENCE_LEN still rejects it, as MalformedPath; that test fails on the variant mismatch, never on the reference being accepted. Each test was confirmed to fail against the mutation it guards and to pass once reverted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`doctor` prints named config fields, never a `Config` struct dump, and reports only the extension table count — never a value. Both are pinned by tests in tests/cli.rs that were shown to fail against a `doctor` violating them, rather than assumed. Deviations from the drafted plan, each forced or justified: - `config.schema_version()` is a method, not a field. - `predicates`' `.and(..)` needs `PredicateBooleanExt` in scope. - Test helpers return `io::Result` instead of unwrapping: clippy's `allow-unwrap-in-tests` only recognises frames under a `#[test]` fn, so an `unwrap` in a free helper is a hard `-D clippy::unwrap-used` error. - `psyched` includes `logging.rs` with `#[path]` rather than carrying a second copy of the subscriber setup; two independently configured subscribers would eventually disagree about the writer, and a daemon logging to stdout corrupts a `status --json` pipeline. - Both crate roots carry `//!` docs: on a binary-only crate `missing_docs` fires for the crate root and nothing else, and CI runs `-D warnings`. - Module items are `pub(crate)`, not `pub`: `unreachable_pub` does fire in a binary crate. - No `psyche-core` dependency. Everything the CLI needs from it arrives through `psyche_config::Config`, so the entry would be a false edge in the dependency graph.
…ugh in the CLI
Two defects motivate this, both of the same kind: code that answers a
question it cannot actually answer.
A second SIGTERM defeated graceful shutdown. A caller that lost the
shutdown election was handed `AlreadyStopped` immediately while the
winner was still sitting in `Draining`, so `shutdown().await?` returned,
`main` returned, and the process exited mid-drain. Operators send a
second signal precisely when the first appears not to have worked, so
the routine operator response to a slow drain was to abort it. A losing
caller now waits on a `tokio::sync::watch` fed from inside the
transition, under the same guard that decided it, and returns only once
`Stopped` has been published. The mutex remains the election — `send`
has no compare-and-swap, and the 24,000-attempt concurrency test exists
to protect that single acquisition.
`psyche status` asserted a fact it could not know. It printed
`{"state":"stopped"}` on any host, including one running `psyched`,
because it is a separate process with no IPC. It now emits
`observed: false` alongside, and says "not observed" in the human form.
Adding the caveat after consumers have learned to trust a bare `state`
is the weaker fix.
Also in this change:
- `Runtime::start` returns `Result`; `RuntimeError` is `#[non_exhaustive]`
and its variant is renamed `ShutdownInProgress`. `LifecycleState` is
deliberately left exhaustive — a new state should break every renderer
rather than fall through a `_` arm.
- `LifecycleState: Display` with the wire spellings, used for every
lifecycle log field and by `psyche status`. Transitions previously
logged a `Debug`-rendered `Draining` under the same key `start` used
for a lowercase `running`, which broke any filter on that key.
- `Runtime::subscribe` and `Runtime::config`.
- `psyche start` runs the daemon instead of printing a suggestion and
exiting 0. Its run path is now one shared function that `psyched` also
calls, so its help text cannot go stale again.
- The `!Send` claim on the drain seam was false — it named an assertion
that constrains the type, not the future. Replaced with one that fails
at the seam.
Two decisions worth review, both reported rather than buried:
- A losing caller returns `Ok(())`. An error would be false by the time
it was returned — the shutdown is finished, not in progress — and
every caller would have to translate it back into success. The
consequence is that no path now constructs `ShutdownInProgress`.
- `send_replace`, not `send`. `send` fails when no receiver exists and
does not store the value it failed to deliver, which left the
published state at `Running` and hung the next losing caller forever.
Verified by mutation: that swap wedges the suite rather than failing
it.
`ShutdownInProgress` had no construction site. Making a losing shutdown caller wait for the winner removed the only one: there is no longer a moment at which a caller can be told a shutdown is in progress, because by the time it would be told, the shutdown is finished. Keeping it would have been an artifact asserting something the code does not do. Worse, it asserted the specific thing that was just deleted — a reader meeting a `ShutdownInProgress` variant infers that `shutdown` refuses a second caller, which is exactly the behaviour the waiting loser replaced, and exactly the behaviour that let a second SIGTERM abort a drain. `RuntimeError` is now an empty `#[non_exhaustive]` enum. `Result` on `start` and `shutdown` is provably always `Ok`, `?` still compiles at every call site, and the first real failure — opening `data_dir`, binding the Coven socket, acquiring a lease — is an added variant rather than a breaking signature change. The type documents that, and says not to add a variant speculatively. `thiserror` derives cleanly on an empty enum on 1.85, so no hand-written `Display`/`Error` was needed. `clippy::empty_enum` is allow-by-default and belongs to `pedantic`, not the `all` this workspace enables; it also stays silent when force-enabled.
`daemon::run` awaited `tokio::signal::ctrl_c()`, which on Unix is SIGINT alone. SIGTERM kept its default disposition, so `systemctl stop`, `docker stop`, a Kubernetes eviction and a bare `kill` all terminated the process with no drain: verified at exit 143 with stderr stopping at "psyche daemon ready". The entire lifecycle machinery was unreachable in production. Two further defects in the same three lines: - `ctrl_c()` installs lazily at first await, which happened only after `Runtime::start` returned. Harmless while `start` does no I/O, but the G2 follow-on opens `data_dir`, binds the Coven socket and acquires a lease inside it — from then on a signal during startup would leak exactly the socket and lease the design says must never leak. - When installation failed, the old code returned failure while a `Running` runtime went out of scope undrained. A `Signals` value now installs SIGTERM and SIGINT handlers eagerly, before `Runtime::start`, and is awaited after. Installing before starting also makes the failure path honest: there is no runtime yet, so exiting without one is the whole of the correct behaviour rather than a skipped shutdown. A `#[cfg(not(unix))]` shim keeps the Windows npm build compiling and documents that it cannot close the same startup window. The new test drives real signals through `/bin/kill` — `unsafe_code` is forbidden workspace-wide, so `libc::kill` is not available — and covers (psyched, TERM), (psyched, INT) and (psyche start, TERM), asserting exit 0 and `draining` before `stopped`. Against the previous code it failed with `expected a graceful exit, got signal: 15 (SIGTERM)` and stderr ending at "psyche daemon ready". Bounded at 30s so a daemon that ignores the signal fails CI rather than wedging it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tested `daemon.rs`, `doctor.rs` and `logging.rs` were reachable only through two binary crate roots via `#[path]` includes. Each file compiled once per binary — twice per build, four times under test — none of them could be reached from `tests/`, and a binary target has no doc-tests at all, so every rustdoc example in them was dead text. They are now modules of a `psyche_cli` library that both binaries link. The rationale for sharing is unchanged and the comments carrying it are kept: `psyche start` and `psyched` must be one daemon, and one log subscriber, or they drift. Only the mechanism is replaced. `publish = false` is already set workspace-wide, so the nominal public surface never reaches a registry. Adds the first unit test over `doctor::run`, calling it directly instead of spawning a process. Note for the record: the review held that no unit test could call `doctor::run` at all. That is not quite right — `cargo test` does build a test harness for a binary target, so a `#[cfg(test)] mod tests` inside `doctor.rs` would have run under the `psyche` binary. The compile-count, doc-test and `tests/` reachability costs above are real; that one is not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Before this, `2` was clap's usage error, `1` covered "config unreadable",
"config invalid" and "a doctor check failed" alike, and `0` covered
everything else — including `psyche stop`, which printed
"no running daemon to stop" and exited 0 while its own help text promised
to "ask a running daemon to shut down gracefully". Nothing was asked and
nothing was stopped; `psyche stop && deploy` read that as success.
The space is now defined and documented in one place:
0 ok · 2 usage (clap) · 3 configuration · 4 daemon unreachable
5 check failed · 1 left meaning unexpected
Applied across both binaries, so a unit file does not have to know which
one it invoked. `doctor` separates the two failure kinds an operator
scripting it actually needs to tell apart: `3` means the configuration
file is wrong, `5` means the file was fine and the environment it
describes is not. `stop` returns `4` and says
"stop is not implemented in this build (no daemon IPC)" on stderr, and
its `about` now says the same thing rather than describing a feature this
build does not have.
Exit codes are the most expensive contract to change after ship — they
end up in `SuccessExitStatus=`, health probes and shell `&&` chains — so
this lands before the first release rather than after.
Tests assert the specific codes for a missing file, an unsupported schema
version and malformed TOML across all five entry points, and for `stop`.
Against the previous code they failed with `code=1` where `3` was
expected and `code=0` where `4` was.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…fault The default `psyche.toml` is relative to the working directory, and a systemd system unit defaults to `WorkingDirectory=/` — so a `psyched.service` without an explicit `--config` was resolving `/psyche.toml`. There was also no environment variable, which is how a container image parameterises a path it cannot put on argv. Resolution order is now `--config` -> `$PSYCHE_CONFIG` -> `./psyche.toml`, stated in `--help` on both binaries, and clap's own precedence gives it for free. `psyche-config` already names the path it tried in the not-found error, so an operator whose service resolved `/psyche.toml` finds out which file was missing. Deliberately no XDG or `/etc` chain — deferred to the packaging work, not rejected. Noted in the argument's own documentation so the next reader does not have to guess whether it was forgotten. `--config` also becomes `global`, which collapses four identical declarations and the four-arm match that existed to pull the value back out of them, and makes `psyche --config X status` parse. That reads as the natural order and was previously a usage error; the trailing form still works, and both are asserted. Requires clap's "env" feature, added at the workspace root. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three defects, all the same shape: the command reported things it had not
established.
`data_dir` used `create_dir_all`, which returns `Ok(())` for a directory
that already exists at any mode — so the word "writable" was never
verified, and a mode-500 directory reported
`data_dir: ok (... writable)` and exited 0. It now writes and removes a
probe file, which is the only way to learn that a directory is writable.
Verified by mutation: restoring the old body turns the new unit test red
with `left: Ok, right: Fail` and the detail
"/.../blocked exists and is writable".
On a typo'd path `doctor` silently *created* the directory and blessed
it, hiding the exact misconfiguration it exists to surface. It still
creates it — failing a fresh host for a directory it can prepare would be
worse — but reports `warn` and says it did not exist.
`doctor` also refused to run against a configuration that would not load,
which is the one case it most exists for: the load happened before
dispatch, so `psyche doctor --config /nope.toml` printed one raw
`ConfigError` and exited with zero checks run. That also left the
`config` check vacuous — it could only ever report `ok`, because an
invalid configuration never reached it. `doctor` is now dispatched before
the load and takes `Result<&Config, &ConfigError>`: the `config` check
fails naming the path and the reason, every dependent check reports
`skipped`, and the command exits 3 with a full report. The error is
rendered with `Display`, never `{:?}`.
`Check.ok: bool` becomes `Status { Ok, Warn, Fail, Info, Skipped }`, with
the exit code derived from `Fail` alone. `coven_socket_path` and
`extensions` are `info`: neither can fail, and both were sitting in a
list where any non-`ok` entry failed the whole command, which is a claim
of verification neither performed. Adds `doctor --json`, emitting a
versioned `psyche.doctor.v1` document, before the ad-hoc line format gets
grepped into a contract nobody chose to make.
One spelling per status, shared by the text and JSON renderings — the
text form's old uppercase `FAIL` is gone, because two vocabularies for
one word is how the two outputs drift apart.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`status --json` emitted `{"state":"stopped","observed":false}`. On a host
where a daemon *is* running that first field is simply false, and
`jq -r .state` is what a consumer actually writes — an `observed` flag
sitting next to a populated `state` is an invitation to read the state and
ignore the flag.
The document is now:
{"schema":"psyche.status.v1","observed":false,"state":null,
"reason":"no-ipc"}
`state` is populated only when something was observed, and the caveat is
structural rather than advisory: `Observation` holds either a state or a
reason, so no rendering can emit a state it was not given. `reason` is a
closed Rust enum, not a free string, because a consumer branching on it
needs the set to be enumerable; `no-ipc` is the only variant this build
can produce, and `socket-absent` / `connect-refused` /
`permission-denied` are documented as the shape the IPC work extends it
with rather than added now as variants nothing constructs.
The envelope matters as much as the field. This repository versions its
configuration (`psyche.config.v1`) and the Coven API (`coven.daemon.v1`)
and had nothing on its own machine-readable output.
The human rendering drops `state: stopped` too — it now reads
`state: not observed (no-ipc: ...)`. The two modes previously disagreed
about how much the command knew, with the text form making the stronger
claim.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
M1: the `Send + Sync + 'static` assertions in psyche-runtime were introduced by a comment claiming "psyche-cli holds a `Runtime` across tokio task boundaries and awaits these two futures inside `tokio::spawn`". Verified false — the CLI contains no `tokio::spawn`, no `Arc` and no `Runtime` sharing; it awaits both futures inline on one task. The assertions stay, and so does the genuinely good explanation of why a `!Send` future would otherwise surface as an error in the wrong crate; only the false premise is rewritten. M5: `try_from_env(..).unwrap_or_else(|_| info)` treated a malformed directive exactly like an absent one. `PSYCHE_LOG` is now read directly, so a parse failure warns on stderr — `eprintln!`, because the subscriber is not up yet — and an absent variable still falls back silently. The review's example for M5 does not hold, and the truth is worse. `PSYCHE_LOG=trce` is *valid* EnvFilter syntax: a bare word is a target directive, so it enables a target named `trce` and the process runs in total silence rather than at info. Measured against the built binary. Nothing can distinguish that from a deliberate target filter, so it is documented in `logging.rs` and in docs/CLI.md instead of guessed at. M7: `psyched`'s `about` now says it is equivalent to `psyche start`, and its `--config` has a help description. Both binaries' `--config` help states the resolution order, and `status`'s `about` says it cannot observe a state. Implementation rationale moved out of the doc comments into ordinary comments — clap prints doc comments verbatim, and an operator running `--help` does not need to read why an argument was made global. Test gaps closed: `psyche start` now also asserts stdout is empty (the "logs to stderr, never stdout" invariant was pinned only for `status --json`, and the daemon is the path that logs volumes), and a new test asserts `psyche start --help` and `psyched --help` expose the same long options, so one cannot gain a flag the other lacks. docs/CLI.md documents the exit-code space, the `--config` resolution order, the `psyche.status.v1` and `psyche.doctor.v1` schemas, the `psyche`/`psyched` relationship and `PSYCHE_LOG`. Every claim in it was run against the built binaries. It states plainly that `stop` does nothing and that `status` never observes a state in this build. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The root manifest has carried `publish = false # distributed via npm,
never crates.io` since bootstrap. It has never done anything.
`[workspace.package]` is not applied implicitly; a member inherits a key
only where it writes `<key>.workspace = true`. All four members inherit
`version`, `edition`, `rust-version`, `license` and `repository` that
way, and none of them inherit `publish`. So every member fell back to
the default, and `cargo metadata` reported `publish: null` — publishable
to crates.io — for all four:
psyche-core publish=None
psyche-config publish=None
psyche-runtime publish=None
psyche-cli publish=None
With `publish.workspace = true` added, the same query reports
`publish=[]` for all four, which is the "no registry" form.
Found by the dependency audit added alongside this commit, not by
reading the manifests. `deny.toml` sets `wildcards = "deny"`, and the
intra-workspace `{ workspace = true }` path dependencies are wildcards —
they carry no version requirement. cargo-deny has `allow-wildcard-paths`
for exactly that case, but it applies only to private crates, on the
grounds that crates.io rejects path dependencies anyway. It read these
four as public and rejected them:
error[wildcard]: found 2 wildcard dependencies for crate 'psyche-cli'.
allow-wildcard-paths is enabled, but does not apply to public crates
as crates.io disallows path dependencies.
The audit was therefore reporting a real defect rather than a policy
that was too strict: a `cargo publish` from any member directory would
have attempted a genuine crates.io upload of a crate whose manifest says
it is never published there.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Nothing has ever been pushed, so no CI has ever run against this repository. This is that CI. Three jobs. `rust` runs fmt, clippy and the test suite across Linux, macOS and Windows with `fail-fast: false`, so one platform's failure still reports the other two. `supply-chain` runs cargo-deny over licences, advisories, bans and sources. `secrets` runs gitleaks over every commit on every ref. `RUSTFLAGS: -D warnings` is set at workflow scope rather than per step. The workspace sets `missing_docs`, `unreachable_pub`, `missing_debug_implementations`, `unused_qualifications` and `rust_2018_idioms` to `warn`; without the promotion, CI would let all five rot. It composes with clippy's own `-- -D warnings` rather than conflicting: `RUSTFLAGS` reaches rustc, the trailing args reach clippy-driver, and registry dependencies are unaffected either way because cargo passes them `--cap-lints allow`. Workflow scope is also what keeps `Swatinem/rust-cache` correct — it hashes every environment variable whose name begins with CARGO, CC, CFLAGS, CXX, CMAKE or RUST, so `RUSTFLAGS` is part of the cache key. Constant value, stable key, warm cache; a per-step assignment would have been invisible to the action and the restored cache would have been built under other flags. The MSRV pin, and why only one job carries it: `rust` pins 1.85.0 explicitly instead of tracking `@stable`. `rust-toolchain.toml` already pins 1.85.0 and outranks `rustup default`, which is all `dtolnay/rust-toolchain` sets — it does not export `RUSTUP_TOOLCHAIN` — so `@stable` would download a toolchain the build then never used: a slower job testing nothing about the version we ship. The pin here is documentation of an invariant the toolchain file enforces. The two must agree, and a disagreement shows up as a job that installs one compiler and runs another. `supply-chain` deliberately does not carry it. cargo-deny is a tool run against the tree, not an artifact we ship, so its build toolchain says nothing about what we support — and it cannot use the pin regardless: cargo-deny 0.19.8 declares `rust-version = 1.88.0`, and `cargo install` under 1.85.0 refuses rather than falling back. Because the toolchain file outranks `rustup default`, installing stable is not by itself enough; the command uses `cargo +stable`, an explicit `+toolchain` being the one thing that outranks `rust-toolchain.toml`. The version is pinned to 0.19.8 so that a cargo-deny release cannot turn CI red with no change to this repository, and so CI runs the binary the policy was validated against. `deny.toml` adds `allow-wildcard-paths = true` under `[bans]`. The intra-workspace path dependencies carry no version requirement and are therefore wildcards; this is the option that exempts them without weakening `wildcards = "deny"` for real registry dependencies. It only takes effect for crates that are actually private, which the preceding commit is what made true. `multiple-versions` is `warn`, not `deny`. The tree has one duplicate today — syn 2.0.119 via tracing-attributes against syn 3.0.3 via clap_derive, serde_derive, thiserror-impl and tokio-macros. Neither side is ours to move, and a proc-macro dependency appearing twice costs build time and nothing else. The secret guard runs the gitleaks CLI rather than `gitleaks/gitleaks-action`. The action resolves the repository owner and exits 1 when that owner is an Organization unless a `GITLEAKS_LICENSE` secret is set; `OpenCoven` is an Organization, so the action form would fail on every run until a key was bought. The CLI it wraps is MIT and has no such gate. Same reasoning as cargo-deny above: run the real binary with the arguments an engineer runs locally, so there is no CI-only path. Version and SHA-256 are pinned, because a job whose whole purpose is supply-chain hygiene should not curl an unverified tarball. No npm job. Task 8 creates `packages/psyche-npm`; naming it here would leave CI red for a whole task, for the same reason `Cargo.toml` cannot declare a workspace member before the crate exists. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`EXIT_CHECK_FAILED` is used at exactly one site, inside the `#[cfg(unix)]`
test `doctor_exits_with_the_check_failed_code_on_an_unwritable_data_dir` —
the mode-500 case has no meaning where mode bits do not restrict. The import
was unconditional, so on Windows it is dead.
That is a warning, not an error, which is why it survived: every local run
so far has been on macOS, where the import is live. The CI added in the
previous commit sets `RUSTFLAGS: -D warnings` workflow-wide, promoting it to
a hard error and failing the `windows-latest` leg at both the Clippy step
and the Tests step.
Reproduced against the real target rather than reasoned about:
RUSTFLAGS="-D warnings" cargo clippy --workspace --all-targets \
--target x86_64-pc-windows-msvc
error: unused import: `EXIT_CHECK_FAILED`
error: could not compile `psyche-cli` (test "cli") due to 1 previous error
Clean after this change, as are the host clippy, fmt, and the 88-test suite
under the same promotion.
This is the first defect the CI gate caught, and it was caught before the
gate ever ran remotely.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The wrapper used `spawnSync`, which orphans `psyched` on every directed
signal. A synchronous spawn blocks Node's event loop inside waitpid, so a
`process.on('SIGTERM')` handler is JavaScript that cannot run until the child
has already exited. There is no level of effort that fixes it in place: the
wrapper dies, the daemon is reparented to init, and it never drains.
That defeats the whole of the SIGTERM work one layer up. `psyched` grew
proper SIGTERM handling three commits ago precisely so `docker stop`,
`systemctl stop`, supervisord and a Kubernetes preStop hook would get a
graceful drain — and every one of those signals the wrapper PID alone.
Measured before changing anything:
wrapper alive? NO
orphaned child still running? YES
log: UP <- the TERM trap never fired
The failure is invisible interactively, which is what makes it dangerous.
Ctrl-C keeps working under `spawnSync` because a tty signals the entire
foreground process group, so the child hears the terminal directly and the
wrapper plays no part. Only a directed `kill` reproduces it, and manual
testing does not do directed kills.
Now: async `spawn`, an explicit forwarding loop over SIGINT/SIGTERM/SIGHUP/
SIGQUIT (SIGINT/SIGTERM/SIGBREAK on Windows, which emulates no others), and
exit on `close` so inherited stdio is flushed before this process leaves.
`exitCodeFor` is untouched — `close` yields the same (status, signal) pair
`spawnSync` returned, so its contract survived the move.
Two further defects fixed alongside, both reachable past a passing checksum:
`spawn` reports exec failure through an event rather than by throwing, so a
companion package built for the wrong architecture matched its digest and
then died with exit 1 and no output at all. It now says why.
`require.resolve('<pkg>/package.json')` fails with
ERR_PACKAGE_PATH_NOT_EXPORTED for a package that declares `exports` without
a `"./package.json"` entry. That package is installed. Reporting "not
installed" sent the operator into a reinstall loop that could never
terminate, so absence and misconfiguration no longer collapse into one
message.
Three new tests, each shown to fail against the reverted `spawnSync` form
with "timed out waiting for the child to receive SIGTERM" and to pass after
restoring it. Two signals rather than one, so the handler is proven to
forward what it received instead of hardcoding a constant.
The README records what the G12 release job must do for the messages this
package ships to stay true: companion packages must declare `os`/`cpu` (npm
only skips a mismatched optional dependency that declares them) and must not
hide package.json behind `exports`. It also notes that the repository
declares MIT in two manifests while containing no LICENSE file.
15 tests pass; the pack is still five files.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Not ready to approve
The npm signal-forwarding tests mutate the checked-in wrapper package.json during execution, making the suite non-hermetic and potentially flaky (and leaving a dirty tree if interrupted).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Bootstraps the initial Psyche Rust workspace and operator tooling, establishing strict configuration loading, a shutdown-capable runtime lifecycle, a CLI/daemon pair, and a gated npm wrapper distribution, with CI gates added to enforce formatting, linting, testing, dependency auditing, and secret scanning.
Changes:
- Added
psyche-core,psyche-config,psyche-runtime, andpsyche-clicrates with schema/version enforcement, redaction-focused types, lifecycle management, and operator commands (psyche/psyched). - Introduced an npm wrapper package (
@opencoven/psyche) that resolves a platform companion package, verifies SHA-256, forwards signals, and maps exit codes, with node:test coverage. - Added CI workflows and supply-chain tooling (
cargo-deny,gitleaks) plus repo-level toolchain and lint configuration.
File summaries
| File | Description |
|---|---|
rust-toolchain.toml |
Pins the Rust toolchain and components for consistent local/CI builds. |
Cargo.toml |
Defines the workspace, shared deps, MSRV, lint policy, and release profile. |
Cargo.lock |
Locks Rust dependency graph for reproducible builds and auditing. |
deny.toml |
Configures cargo-deny license/advisory/source policy. |
clippy.toml |
Tunes clippy allowances for test-only unwrap/expect usage. |
.gitignore |
Establishes baseline ignore rules for Rust/Node artifacts and logs. |
.github/workflows/ci.yml |
Adds multi-OS Rust checks, supply-chain audit, gitleaks scan, and npm wrapper tests/pack dry-run. |
docs/CONFIGURATION.md |
Documents the strict psyche.config.v1 configuration contract and extension rules. |
docs/CLI.md |
Documents CLI behavior, exit codes, config resolution, JSON output schemas, and caveats (no IPC). |
crates/psyche-core/Cargo.toml |
Introduces the psyche-core crate manifest with workspace-inherited metadata and deps. |
crates/psyche-core/src/lib.rs |
Exposes schema and secret modules as the core public API surface. |
crates/psyche-core/src/schema.rs |
Implements strict schema version acceptance/denial with stable error text. |
crates/psyche-core/src/secret.rs |
Adds SecretRef with allowlisted scheme validation and redacting Debug/Display. |
crates/psyche-config/Cargo.toml |
Introduces the psyche-config crate manifest and test deps. |
crates/psyche-config/src/lib.rs |
Implements strict TOML config loading with version-first denial and payload-reduced parse errors. |
crates/psyche-runtime/Cargo.toml |
Introduces the psyche-runtime crate manifest and async runtime dependencies. |
crates/psyche-runtime/src/lib.rs |
Adds lifecycle state machine, shutdown election, observability, and concurrency-focused tests. |
crates/psyche-cli/Cargo.toml |
Defines a shared library plus psyche and psyched binaries from one crate. |
crates/psyche-cli/src/lib.rs |
Centralizes shared CLI/daemon modules and defines the exit-code space. |
crates/psyche-cli/src/main.rs |
Implements the psyche entrypoint: parsing, config handling, and command dispatch. |
crates/psyche-cli/src/bin/psyched.rs |
Implements the psyched daemon entrypoint using the shared run path. |
crates/psyche-cli/src/daemon.rs |
Shared daemon lifecycle run path with signal handling and graceful shutdown behavior. |
crates/psyche-cli/src/status.rs |
Defines status observation model and JSON/text renderings that avoid unobserved state claims. |
crates/psyche-cli/src/doctor.rs |
Implements credential-free environment checks with disciplined stdout/stderr output. |
crates/psyche-cli/src/logging.rs |
Adds JSON-on-stderr tracing subscriber with PSYCHE_LOG parsing behavior. |
crates/psyche-cli/tests/cli.rs |
Adds end-to-end tests over both binaries, focusing on output discipline and exit-code contracts. |
packages/psyche-npm/package.json |
Adds npm wrapper manifest, optional platform companion deps, and checksum placeholders. |
packages/psyche-npm/README.md |
Documents wrapper behavior: resolution, checksum enforcement, signals, and packaging constraints. |
packages/psyche-npm/bin/psyche.js |
Implements the npm bin shim: resolve binary, verify checksum, spawn, forward signals, map exit codes. |
packages/psyche-npm/scripts/verify-checksum.js |
Adds checksum verification and supported-platform mapping logic. |
packages/psyche-npm/scripts/resolve-binary.js |
Resolves companion package binaries, distinguishes install vs exports errors, forwards signals, maps exit codes. |
packages/psyche-npm/test/verify-checksum.test.js |
Adds unit tests for checksum verification and manifest/platform list consistency. |
packages/psyche-npm/test/signal-forwarding.test.js |
Adds integration-style tests ensuring wrapper forwards signals and reports spawn failures. |
Review details
- Files reviewed: 30/33 changed files
- Comments generated: 3
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
The first CI run failed the `windows-latest` leg: five `doctor` unit tests
panicked with
Parse { detail: "line 3, column 17: too few unicode value digits,
expected unicode hexadecimal value" }
A Windows temp directory is `C:\Users\RUNNER~1\AppData\Local\Temp\...`, and
three fixtures interpolated one straight into a TOML basic string. TOML then
reads `\U` as the start of a unicode escape. The error names the
configuration loader, which is not where the defect was — the fixture was
malformed before the loader ever saw it, and the loader was right to reject
it.
Nothing here is platform-specific: writing a path into TOML has always
required escaping, and every one of these fixtures was wrong on every
platform. Only Windows produces a path that exposes it.
Fixed with a `toml_str` helper at all three sites, and pinned by a test that
runs everywhere — it feeds a literal Windows path through the same fixture
path, so the defect is reproducible from macOS rather than only from a CI
runner. Shown to fail: dropping the backslash replacement gives
assertion `left == right` failed: every backslash must be escaped or
TOML reads \U as a unicode escape
This is the gap the PR body predicted. Format and Clippy already passed on
Windows — the cross-compile check caught what it could, and runtime
behaviour is exactly what it could not.
89 tests pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@BunsDev I can’t apply these changes because the pull request’s branch is the repository’s default branch, which I’m not allowed to push to directly. You can ask me to create a new pull request with the requested changes instead and I'll open a separate PR on a new branch. |
Implements the approved W2 bootstrap child plan for
coven-psy1.Scope:
psyche-core,psyche-config,psyche-runtime,psyche-cli, the CI gate, and npm dry-run distribution. No Telegram, Coven execution, store, graph, or identity behaviour. No capability flag is set. Publication remains gated at G12; production child dispatch remains unauthorized pending G6.Merge with squash. Review fixes were applied as follow-up commits rather than amends, so each reviewed state stays inspectable — which means intermediate commits are individually non-building. Squashing collapses that into one buildable commit and keeps
mainbisectable. Rebase-merging would preserve the broken states.What is here
psyche-coreSecretRef, which cannot hold or print a secretpsyche-configpsyche.config.v1loading, version-first denial, payload-free errorspsyche-runtimepsyche-clipsycheandpsyched, credential-freedoctor, versioned machine-readable outputPlus
deny.toml,.github/workflows/ci.yml, andpackages/psyche-npm.Local gate
All green on macOS:
cargo fmt --all -- --check— cleancargo clippy --workspace --all-targets -- -D warnings— cleancargo test --workspace --locked— 88 passed, 0 failedcargo deny check licenses advisories bans sources—advisories ok, bans ok, licenses ok, sources okgitleaks detect --log-opts="--all"— no leaksnpm --prefix packages/psyche-npm test— 15 passednpm pack ./packages/psyche-npm --dry-run— 5 files, no binarycargo clippy --target x86_64-pc-windows-msvc— clean (cross-check only)This PR is the first time CI has ever run against this code. Every result above is local.
Design properties worth reviewing
ConfigErrordeliberately holds notoml::de::Errorand has no#[from]for one: that type'sDisplayrenders the offending source line and itsDebugcarries the entire file. Reduction happens at exactly one greppable place.SecretRefis the only type permitted to carry a secret reference, redacts through bothDebugandDisplay, and exposes one accessor named so review can grep it.Drainingis observable, transitions are ordered, and the shutdown election is proven concurrent by test.statusreportsstate: nullwithobserved: falserather than guessingstopped;stopexits 4 rather than 0;doctorearns the word "writable" with an actual write.Known gaps, stated rather than discovered later
tests/cli.rsanddoctor.rsmake path and permission assertions that have never run there. Expect findings from thewindows-latestleg.psyche stopis unimplemented — there is no daemon IPC in this build. It exits 4 and says so.data_dirboth start. That belongs with the lease work inRuntime::start.LICENSEfile, whileCargo.tomlandpackage.jsonboth declare MIT. Blocks G12.os/cpuand must not hidepackage.jsonbehindexports, or two of this wrapper's operator-facing messages become false. Recorded inpackages/psyche-npm/README.md.cargo install cargo-denyis uncached and rebuilds ~250 crates per run (~5-10 min).Defects the gate caught before it ever ran remotely
publish = falsehad been inert since bootstrap —[workspace.package]keys are not inherited implicitly, so all four crates were publishable to crates.io despite a manifest comment saying otherwise.gitleaks/gitleaks-actionexits 1 for Organization-owned repos without a paid licence;OpenCovenis an Organization. Replaced with the pinned MIT CLI.cargo install cargo-denycannot run under the 1.85.0 MSRV pin — cargo-deny 0.19.8 requires 1.88.0, andrust-toolchain.tomloutranksrustup default, so an explicit+stableis required.#[cfg(unix)]test would have failed the Windows leg under-D warnings.spawnSync, which orphans the daemon on every directed signal — defeating the SIGTERM handling one layer up. Interactive Ctrl-C masked it entirely.Note on history
mainwas rooted at an empty commit so this work could arrive through review rather than a direct push to the default branch. The branch was rebased onto it to establish common ancestry — GitHub refuses a pull request between unrelated histories. All 35 commits are signed; the tree is byte-identical to the pre-rebase state.🤖 Generated with Claude Code