fix(reconcile): validate registry keys as path components - #587
fix(reconcile): validate registry keys as path components#587chazmaniandinkle wants to merge 1 commit into
Conversation
…egisterProvider The reconcile registry key is a filesystem path component: StatePath joins it into `<root>/.cog/config/<key>/.state.json`. Nothing validated it. This was not hypothetical. WorktreeReconciler.Type() returned "worktree-reconciler:" + repoRoot, which materialised a real nested `.cog/config/worktree-reconciler:/Users/.../workspaces/cog/.state.json` tree in a live workspace. The same join would let a key containing ".." write outside the config directory entirely — which matters more as keys start coming from operator config and, later, from declarations that sync between nodes. - ValidateInstanceName: a key is one or two "/"-separated segments (type, optional instance discriminator, e.g. "lms-model-state/lmstudio-eclipse"). Each segment must be a portable filename. ':' is rejected despite being legal on Unix because this substrate runs on Windows nodes too, and a state path that cannot be created on one node is a defect wherever minted. - RegisterProvider panics on an invalid key (boot-time programming error, consistent with its existing duplicate-name panic). UpsertProvider refuses and logs instead: it runs at runtime with names partly derived from providers.yaml, and a malformed config entry should cost that one provider, loudly, not the whole daemon. - WorktreeReconciler.Type() now derives a safe, stable, collision-resistant discriminator (basename + short digest of the repo root) rather than interpolating the path. Legible in `ls .cog/config`; distinct for two repos sharing a basename; stable across boots so state is not orphaned each time. - UnregisterProvider: the missing half of UpsertProvider. Without it, a provider whose config entry has been removed keeps being swept by the reconcile daemon — and keeps actuating against a target the operator has already retracted — until the kernel restarts. Existing state under the old malformed path is orphaned by the Type() change and can be removed by hand; the reconciler recomputes from observed state.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 cog-review — approve (head cc359d4)
The premise checks out: StatePath (pkg/substrate/reconcile/state.go:45-47) does filepath.Join(root, ".cog", "config", resourceType, ".state.json") on the raw registry key, and the pre-PR WorktreeReconciler.Type() (confirmed against the parent commit) returned "worktree-reconciler:"+r.RepoRoot, so every absolute repo root's '/' characters were already being interpolated as literal path separators into the state tree — this was live, not hypothetical, for all deployments, not just colon-bearing ones. ValidateInstanceName correctly gates on absolute paths, '..'-segments, >2 segments, empty segments, and Windows-illegal characters, with filepath.IsLocal as a lexical backstop; RegisterProvider now panics (boot-time programming error) and UpsertProvider logs+refuses (runtime operator-config error), matching the stated rationale that config-derived names (mlx-supervised/, lms-model-state/ in router.go) must not crash the daemon. I audited every Reconcilable.Type() implementation in the tree (site, marginbridge, vitalsretention, component, daemon providers, projection reconcilers, cogdoc_review, mlx/lms) and every static RegisterProvider/UpsertProvider call site — all use fixed, portable, single- or double-segment literals, so the new panic-at-boot path does not break any existing registration. The new instanceSlugForRoot correctly sanitizes and lowercases the basename, falls back to "repo" for degenerate roots (e.g. "/"), and the provided tests (including a named pin on the exact live-corruption key and a StatePath-locality property test) match the described defect and fix. Test file placement, imports, and package boundaries are all consistent with the surrounding code.
Confirmed findings (0):
none
Unverified notes:
- UnregisterProvider is added and tested but has no caller anywhere in the tree yet, so the stale-provider-keeps-being-swept problem its doc comment motivates is not actually fixed by this PR — only the primitive exists. Not a defect (the description only claims to add the function), but worth confirming a follow-up wires it into config-reload/provider-removal.
- ValidateInstanceName does not reject Windows-reserved device basenames (con, nul, aux, prn, lpt1-9, com1-9) as segments; since the PR's own stated rationale is that this substrate runs on Windows constellation nodes, a future provider or operator-config name equal to one of these would pass validation on a non-Windows build host and only fail when a state file is actually created on a Windows node. No current provider name triggers this, so it's not reachable today.
- instanceSlugForRoot hashes the raw, uncleaned repoRoot (sha256.Sum256([]byte(repoRoot))) while deriving the basename from filepath.Clean(repoRoot); two equivalent repo-root strings differing only in formatting (e.g. a trailing slash) would hash to different suffixes and thus different registry keys/state files. This mirrors a pre-existing fragility (the old code embedded the raw string wholesale too) rather than a regression introduced here, and I found no evidence in this codebase that workspaceRoot actually varies in formatting across boots (resolveWorkspaceUncached is cached once per process), so it's unconfirmed as a live issue.
This review was generated by an AI reviewer with review-only authority — it can approve or block, it cannot merge or close. Verdict basis: pr-review-rubric.md. The merge decision belongs to a human or their operator workflow.
chazmaniandinkle
changed the title
There was a problem hiding this comment.
🤖 cog-review — approve (head cc359d4)
The PR's premise is verified: pre-PR, WorktreeReconciler.Type() literally returned "worktree-reconciler:" + r.RepoRoot (confirmed via git show on the pre-PR blob), which StatePath then joined into a filesystem path, so a colon-and-absolute-path key really could and did corrupt state paths. The fix replaces this with instanceSlugForRoot, which sanitizes the basename to [a-z0-9_-] and appends a stable 4-byte sha256 discriminator, always yielding a single safe segment (verified the empty/"."/".."/"-"-only fallback path is unreachable-safe and collision-resistant enough for a discriminator, not a security boundary). The new ValidateInstanceName is wired at the two actual registry write points (RegisterProvider panics at boot, UpsertProvider logs+refuses at runtime), and I traced every downstream consumer of registry keys (StatePath/StateLockPath via reconcile_daemon.go and meta.go's reconcileResource, which resolves providers exclusively through GetProvider keyed by the same map) to confirm the registry boundary is the sole choke point — there is no bypass path that reaches StatePath with an unvalidated name. I checked every existing RegisterProvider/UpsertProvider call site and every other Type() implementation in the repo (all literals or enum-derived, e.g. "lineage-projection-"+kind) and found no sibling instance of the raw-path-interpolation defect class, so whole-class coverage is satisfied both for the reported instance and prophylactically via the generic gate. Tests are strong, including a pinned regression on the exact corrupted key shape observed in production and a property test that accepted keys stay inside the config dir. No confirmed defects.
Confirmed findings (0):
none
Unverified notes:
- router.go's dynamic UpsertProvider(mlxSupervisedType+"/"+name, ...) and (lmsModelStateType+"/"+name, ...) will now silently (log-only) skip reconcile-registry registration if an operator-configured provider name contains '/', ':', or other disallowed characters — the chat-routing provider itself still works, only the reconcile/self-heal companion registration is dropped. This matches the PR's stated UpsertProvider contract and isn't a new class of bug, but it's a runtime behavior change for malformed operator config that isn't explicitly called out in the description beyond the generic UpsertProvider wiring note.
This review was generated by an AI reviewer with review-only authority — it can approve or block, it cannot merge or close. Verdict basis: pr-review-rubric.md. The merge decision belongs to a human or their operator workflow.
1 participant
Registry keys in pkg/substrate/reconcile are used as filesystem path components via StatePath. Before this change, a provider could register with a key containing separators or Windows-hostile characters, silently corrupting state paths — a live instance of this existed: WorktreeReconciler's Type() embedded an absolute repo root after a colon.
.., absolute paths, >2 segments, empty segments, and\:*?"<>|(colon rejected because constellation nodes include Windows).worktree-reconciler/<basename>-<sha256[:4]>instead of embedding the raw path.Note: ADR three-surface-memory-projection (PR to follow) declares this branch as a dependency — its registry-key citations describe this branch's behavior.