Dashboard usability sweep (#219–#236, #238) and v4.8.2 preparation - #239
Merged
Conversation
Refs #235 The first cut labelled any failure message that MENTIONED a `memesh …` command. Two of those messages tell the user to reload or retry in the browser and only escalate to a command if that fails, so the label announced a prerequisite that does not exist: "Requires Terminal: Reload the page. If this keeps happening, run `memesh doctor`." "Requires Terminal: The server reported an error (HTTP 502). Try again — if it keeps happening, run `memesh doctor`." Neither requires a terminal to act on. The unreachable load keeps its label — "Check that `memesh serve` is still running" has nothing to do in the browser first — as do the two auth errors in api.ts, whose only action is reading what `memesh serve` printed. This also restores tests/dashboard/ux-batch-b.test.tsx, which asserted the unlabelled sentence and was left red: the focused set that cleared the original change did not include it, and the Chrome journey covered the banner and feedback surfaces, never a failure message.
Refs #238 A rejected key comes back inside the provider's own prose — "Incorrect API key provided: sk-proj-…" — and `POST /v1/config/test` forwarded that sentence to the Dashboard, which renders it in an alert. The fragment then lives wherever the alert is captured: QA screenshots, evidence artifacts, a pasted bug report. Both public egresses (`/v1/doctor` and `memesh feedback`) run doctor with `probeCapabilities: false`, so this never reached a GitHub issue body — but the browser surface is enough. Three of the four paths that surface upstream prose built their result by hand and returned `err.message` raw; only the inference probe redacted. A funnel a call site can skip is not a boundary, so failures are now constructed by one `fail()` and there is no unredacted spelling: the module has exactly one `valid: false` literal. Redaction runs BEFORE the 300-char cap. Truncating first leaves a fragment no pattern matches, and the fragment is what gets published. Also adds `settings.testError.inference_failed` across the 11 locales. The code existed and is documented, but had no translation, so that path showed bare prose with none of the actionable category the other five carry. Non-sensitive diagnostics are unchanged: model name, organisation rate-limit prose and HTTP status all survive, and a test pins that.
Refs #238 The shared egress redactor missed the shape providers actually emit. A rejected key comes back masked — `sk-proj-**********ZfQ9` — and `sk-[A-Za-z0-9_-]{16,}` stops dead at the first `*`, so the prefix and the trailing characters were published. Which glyph a provider masks with is not knowable in advance, so the three `sk-`/`sk_` patterns collapse into one anchored on the prefix and closed on an alphanumeric, which leaves the sentence's own punctuation outside the match. Bullets, asterisks and bare truncation are all covered, and the list is one pattern shorter. Second gap: a credential in a query string. An upstream error that echoes the request URL (`GET /v1/models?api_key=…`) carried the key through every egress, including the pre-filled GitHub issue body. The new pattern matches the parameter NAME, so `?limit=200` and prose containing the word "token" are untouched. The bare high-entropy token stays UNCOVERED on purpose. Matching it needs an entropy heuristic, and this function runs over the whole `JSON.stringify(doctorResult)` payload — commit SHAs, sha256 digests, installation ids, hook marker hashes. Two patterns that provably damage nothing beat three where one silently corrupts diagnostics. That is what the new fixture is for: real `runDoctor` output captured against a throwaway MEMESH_DIR, asserted byte-identical after redaction. The precedent is concrete — a pattern once compiled from a relative path and rewrote every literal dot in the payload, publishing `4.5.0` as `4~5~0` with nothing saying redaction had done it.
Refs #238 `dist/` and `scripts/hooks/_generated/core-paths.js` are committed artifacts, and a plugin-marketplace install runs `dist/` as committed — it never builds. Without this the redaction fix existed only in `src/` and reached nobody. `core-paths.js` is the Stop hook's own copy of the redactor, so the two new patterns had to propagate there as well; `dist/skills-manifest.json` records its new digest and nothing else moved.
Refs #238 C5 reported 15 new hits and 15 entries to prune — one pair per file, a few lines apart. The baseline keys on file:line, so any edit above a triaged hit moves it and the audit reads one entry as two events. Each entry was carried to its new line with its classification and reason intact, and the source line at the destination was printed and checked against the reason it already had; nothing was re-triaged. C3 is different: seven `test:issue-2NN` scripts with zero references. Not a line shift — new scripts, so they get their own classification. They exist for the completion-gate review contract, which accepts only an exact declared `npm run test:*` argv, and every test file they name is already inside the default `tests/` run, so they add a bounded replay entry point rather than coverage. All nine referenced files were confirmed present.
Nine version anchors and the skills manifest move together; the coherence check treats any disagreement as a release blocker because a lagging package-lock.json has shipped past four releases before. CHANGELOG describes what a user will notice, issue by issue. It does not claim the sweep is verified: every item is implementer-produced candidate evidence, recorded on its own GitHub issue, and none of those issues is closed.
The acceptance that proves a real npm-global install survives an auto-update pins both versions as literals: the published baseline it upgrades from, and the candidate it upgrades to. That is deliberate — `tests/release-scripts-safety.test.ts` asserts the literal appears in the source, so a release that forgets this script goes red instead of testing the previous version's upgrade path. Baseline 4.8.0 → 4.8.1 (the currently published latest), candidate 4.8.1 → 4.8.2. Thirteen and twelve occurrences moved, counts carried over exactly, no 4.8.0 left. This file is a version anchor that `check-version-coherence.mjs` does not know about; the release-scripts test is what actually guards it. Every other `4.8.1` still in the tree was checked and is not an anchor: a `tsx` dependency range, a comment about release history, and synthetic health payloads inside tests.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| expect(toggle.getAttribute('aria-pressed')).toBe('false'); | ||
|
|
||
| localStorage.setItem('memesh.signalMode', 'true'); | ||
| window.dispatchEvent(new StorageEvent('storage', { key: 'memesh.signalMode' })); |
Refs #220, #221 The tests for "promote the draft only after an authoritative GET readback" asserted the ORDER of calls and a hardcoded readback body. Three mutations survived all 475 dashboard and HTTP tests: 1. POST a model with `-WRONG` appended → 475 passed 2. POST no model at all → 475 passed 3. delete the readback reconciliation → 475 passed The third is the one that matters: the guard those two issues exist to add could be removed entirely and nothing went red. A fake whose answer does not depend on what was written cannot see any of them — it proves a request was made, not that the request carried the user's choice or that the answer was checked. The fake is now a store: it keeps the posted `llm` and serves it back, which closes the loop on the write side. A second test breaks the loop on purpose — the server answers the readback with a model the user did not choose — and requires the UI to report failure rather than success, which is what pins the guard itself. All three mutations now KILLED (2, 2 and 1 failing assertions respectively); source sha256 restored identical after each.
Refs #226 Same defect as the save path, one function further down. Deleting the remove-readback reconciliation left the whole dashboard and HTTP set green: the fake answered the GET from a `removed` flag it set itself, so it could not represent the case the guard exists for — a POST the server accepts while keeping the provider. The new test is that case. The server answers OK and still holds `ollama/llama3.2`; the UI must report the readback failure and must not say the provider was removed. [Verified-By: npx vitest run tests/dashboard/SettingsTab-primary-llm.test.tsx -> exit=0, Test Files 1 passed (1), Tests 13 passed (13)] [Verified-By: mutation, delete the remove-readback reconciliation and rerun the same file -> exit=1, Tests 1 failed | 12 passed (KILLED); before the new test the same mutation ran tests/dashboard + tests/transports/http.test.ts -> exit=0, Tests 477 passed (SURVIVED); source sha256 restored identical after each] [Verified-By: npm run typecheck -> exit=0; npm run lint -> exit=0]
Refs #238 The pattern I widened to catch masked keys, `sk[-_]\S{4,}[A-Za-z0-9]`, also matches inside ordinary words that happen to contain `sk-`: task-runner, disk-usage, risk-level, desk-setup, ask-first. Six plain English phrases came back as `ta***REDACTED***`, `di***REDACTED***`, `ri***REDACTED***`. At the egress that is corruption. It is worse one module over: the same list backs `containsSecret()` in transcript-extractor, which DROPS a mined memory rather than staging it, so a false positive discards real content with nothing saying why. The impact map found this, not the tests. The negative corpus asserted only `redactSecrets` and contained no word with `sk-` inside it, so both the over-redaction and the drop were invisible. Both gaps are now closed: the six phrases are in the corpus, and a second describe block runs the whole corpus through `containsSecret` and `scrubSecrets` as well. [Verified-By: npx vitest run tests/core/redact-secrets.test.ts -> exit=0, Test Files 1 passed (1), Tests 72 passed (72)] [Verified-By: mutation, remove the leading \b and rerun -> exit=1, Tests 13 failed | 59 passed (KILLED); source sha256 restored identical] [Verified-By: before the fix, containsSecret() returned true for all six phrases and redactSecrets mangled them; after, 0 false positives while sk-proj / sk-ant / masked-key shapes still match] [Verified-By: npm run typecheck -> exit=0; npm run lint -> exit=0; npm run build -> exit=0, 6/6 smoke tests]
Found by the independent review of this branch. Pre-existing, not introduced
here, but it sits on the surface this release touches.
The guard read:
const requestedBase = host || envBase || 'http://localhost:11434';
if (!envBase && host && !isSafeOllamaHost(host)) reject
Setting `OLLAMA_HOST` is ordinary operator configuration for a remote Ollama.
Doing so made `!envBase` false and turned the loopback check off entirely,
while `host || envBase` gave the caller's value precedence — so with the env
set, `POST /v1/config/test {"provider":"ollama","host":"..."}` fetched
whatever URL the request named. The comment above it claimed the operator's
env wins and non-loopback callers are rejected; the code did neither.
The env stays the privileged escape hatch and stays unvalidated, because the
operator sets it server-side. A host that arrives in a request is now always
validated, whatever the env says. No caller sends one today — neither the
Dashboard's two config/test calls nor doctor pass a host — so the only source
is the HTTP request itself.
[Verified-By: reproduced end to end before the fix on an isolated server and
DB. Same body, host http://127.0.0.2:3198, two runs: without OLLAMA_HOST ->
errorCode "bad_host" (blocked); with OLLAMA_HOST set -> errorCode "network"
with an aborted fetch, i.e. the request was issued]
[Verified-By: npx vitest run tests/core/llm-validator.test.ts -> exit=0,
Test Files 1 passed (1), Tests 32 passed (32)]
[Verified-By: mutation, restore the original `!envBase &&` condition and
rerun -> exit=1, Tests 1 failed | 31 passed (KILLED); source sha256 restored
identical]
[Verified-By: npm run typecheck -> exit=0; npm run lint -> exit=0;
npm run build -> exit=0]
The independent review suggested bounding the run (\S{4,200}) to cap how much
a single match can swallow. Tried it, measured it, and did not keep it: with
the word boundary in place over-matching is no longer the failure mode, and a
cap introduces the opposite one — `sk-` followed by a 400-character token
redacts its first 204 characters and publishes the remaining 200.
Partially leaking a credential is worse than over-matching a long unbroken
run, and the run can no longer start mid-identifier anyway. The reasoning is
now in the source next to the pattern, and a test holds the line so the
suggestion is not silently re-applied later.
[Verified-By: bounded at 200, redactSecrets('before sk-' + 'a'.repeat(400) +
'Z after') left a 200-character tail (result length 227); unbounded it
returns exactly 'before ***REDACTED*** after']
[Verified-By: npx vitest run tests/core/redact-secrets.test.ts -> exit=0,
Tests 73 passed (73)]
[Verified-By: npm run typecheck -> exit=0; npm run lint -> exit=0;
npm run build -> exit=0]
One C5 entry moved 303 -> 310 in src/core/llm-validator.ts when the Ollama host guard grew its explanation. Same statement at the destination — `(data.models ?? []).map(...)`, still followed by the `models.length === 0` rejection its SAFE-GUARDED classification cites. Carried over unchanged; not re-triaged. [Verified-By: node scripts/audit/verification-audit.mjs -> exit=0, "Every hit is triaged; every detector saw a non-empty candidate set"]
Both landed after the CHANGELOG entry was written and neither was in it: the unconditional Ollama host validation, and the word boundary that stopped the key pattern from matching inside ordinary identifiers. A release note that omits a security fix is the kind of drift `check-doc-claims` exists to catch, and it would not have caught this one. [Verified-By: node scripts/check-doc-claims.mjs -> exit=0] [Verified-By: npm run verify:release -> exit=0]
Three findings from a second independent review of this branch, all
reproduced before fixing.
F1 — the pattern swallowed sibling JSON fields. redactSecrets runs over
JSON.stringify(doctorResult), which has no whitespace between fields. With
`\S{4,}`, a repo named `sk-widgets` anchored the match and ran through the
closing quote, the comma and the next key, stopping at the first space
inside a LATER string — the sibling `fix` field was deleted from the public
issue body. The class is now `[^\s"\\]`: no real key contains a quote or a
backslash, so this costs no coverage and makes a quote a hard stop. This
also overturns the previous commit's reasoning that "over-matching is no
longer the failure mode" — it was, in the one context the function is
documented for. The comment now says so.
F2 — the transcript consumer compiled the shared list case-SENSITIVELY
while the egress used 'gi'. `DB_PASSWORD=…` and `export OPENAI_API_KEY=…`,
the dominant credential shape in a shell transcript, passed the drop gate
and reached the LLM prompt while the same bytes were masked on the way out.
One list, two consumers, two answers. Both now use 'i'.
F3 — the name=value pattern had no left boundary and no minimum value
length, so `is_secret=false`, `signature=valid` and `token=bucket` were
redacted. A name may now be preceded only by `_` or `-` (compound env names
like DB_PASSWORD must still match) and the value must be 8+ characters.
[Verified-By: each finding reproduced on the pre-fix code with exact inputs
from the review: sk-widgets JSON lost its `fix` field; DB_PASSWORD=… passed
containsSecret; is_secret=false became is_***REDACTED***]
[Verified-By: npx vitest run tests/core/redact-secrets.test.ts
tests/core/transcript-extractor.test.ts tests/core/llm-validator.test.ts
-> exit=0, Test Files 3 passed, Tests 141 passed (141)]
[Verified-By: 3 mutations, 3 KILLED — restore \S (1 failed), restore
case-sensitive compile (1 failed), drop the {8,} floor (1 failed); source
sha256 restored identical after each]
[Verified-By: npm run typecheck -> exit=0; npm run lint -> exit=0;
npm run build -> exit=0]
Refs #234 Found by an independent review of this branch. chooseNextAction has seven states; the existing test named "all five requested states" exercised exactly five, and the two it skipped are adjacent branches with opposite meanings: `reindex === null` is "the status fetch FAILED", `undefined` is "not answered yet". Nothing pinned the line that tells them apart, so it could be loosened to `== null` (every ordinary page load flashes "reload") or deleted (a dead /v1/reindex spins as "loading" forever) with the suite green. The new function-level test covers both states and their priority; the component-level test drives /v1/reindex to a 502 and requires the "unavailable" heading with a working retry, not a recommendation. [Verified-By: npx vitest run tests/dashboard/home-next-action.test.tsx -> exit=0, Tests 7 passed (7)] [Verified-By: 2 mutations, 2 KILLED — `=== null` loosened to `== null` -> 1 failed; guard line deleted -> 2 failed; source sha256 restored identical after each] [Verified-By: npm run typecheck -> exit=0; npm run lint -> exit=0]
…made Refs #238 The independent review caught a misleading note: "Redaction no longer mangles ordinary text" described a defect introduced at 5ff0d5e and fixed at d24973d — both inside this branch. No 4.8.1 user ever saw it, and the old pattern could not have matched those words. Presenting it as a shipped bug fixed is not true. Replaced with the finding that IS a 4.8.1 defect: the transcript drop gate compiled the shared list case-sensitively, so uppercase env-assignment credentials passed it. [Verified-By: node scripts/check-doc-claims.mjs -> exit=0] [Verified-By: git log -S'sk[-_]\\S{4,}' confirms the widened pattern first appeared at 5ff0d5e on this branch, not in any released tag]
The comment said eighteen; the list has held seventeen since three sk- variants collapsed into one. The test pins the real number, so the comment now describes the behaviour without repeating a count that will drift again.
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
2 participants
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 this is
The v4.8.1 Dashboard was walked as a non-engineer would walk it. Nineteen
issues came out of that pass (#219–#236, #238); this branch carries the fixes
for all of them and prepares the 4.8.2 release.
#237 is deliberately not here. It is a product redesign — remove the
Knowledge Graph, replace it with project context, plan mind map, roadmap,
architecture and data-flow views — and it is larger than the other nineteen
combined. Mixing it in would hold nineteen real fixes behind one new product
direction. It stays open for a later release, and its owner-confirmed data
rules (issue comment of 2026-08-28) remain in force.
What a user will notice
The full list is in
CHANGELOG.mdunder[4.8.2]. The shape of it: settingsthat did not save now save and say so, errors that were swallowed now surface,
controls that did nothing now do something, and labels that were untrue are
now true.
Two are worth naming here:
A rejected API key is no longer echoed back into the page. Providers
quote the submitted credential in their rejection prose, and that sentence
was rendered verbatim in the Dashboard — and so into any screenshot of it.
Every failing provider probe is now redacted at the module boundary, and the
shared egress redactor learned the two shapes it was missing: a partially
masked key (
sk-proj-****ZfQ9, which the old charset-based pattern walkedstraight past) and a credential in a URL query parameter.
The memory timeline is called Project History, not Roadmap. It derives
its phases from when memories were captured. That measures capture activity;
it is not a plan and not proof of progress. The name now says what the data
is.
Verification
At
9d25df39:Guards added in this branch were break-tested: each fix was reverted and the
test confirmed red before being restored. The redaction change was mutated in
both directions — under-redaction and over-redaction — because the redactor
runs over the whole
doctorpayload, and a pattern that is too broad corruptsdiagnostics as surely as one that is too narrow lets a credential through. A
committed fixture of real
runDoctoroutput is asserted byte-identical afterredaction to hold that line.
What is NOT claimed
candidate evidence and its unresolved gaps.
COMPLETE_CANDIDATEisimplementer-produced; under this repository's rules it cannot be self-raised.
between this branch and merge.
fixes are in the branch and the tree is green, but nothing binds a
claim-level check to a revision for them. They are
UNVERIFIED, and theirissue comments say so.
export, and the real popup-denial path was never replayed in a browser.
of opening this PR.