fix: reference-kind identity uses slugified name consistently; fix test env isolation

[andrei-hasna]

Summary

Post-merge findings on #57 (todos 195272ae), both reproduced and fixed with TDD regressions.

Finding 1 (P1) — doctor reported clean on a case add --update silently half-fixed.
findReferenceConfigsByName's slug-matching compared the query against a candidate's stored .slug column. That column is disambiguated (-1, -2, ...) the instant two rows already collide — so the check went blind exactly where it matters most. findDuplicateReferenceNameGroups grouped on the raw exact name string, so a case/punctuation variant of an existing name was invisible to doctor even though add --update would (partially) treat it as the same identity. Fixed by having both derive identity from slugify(config.name), recomputed fresh from each candidate's own name rather than trusted from a possibly-disambiguated column or compared byte-for-byte.

Reproduced live with two rows — "Sample Rule" (slug sample-rule) and "sample rule" (slug disambiguated to sample-rule-1) — where the old code found only the first, updated it, and printed no warning about the second. After the fix, add --update finds both and warns; doctor reports the pair as a duplicate group.

Also fixed, decided in-scope: the MCP create_config handler had zero reference-kind identity check — PR #57 exempted it as out of scope, but it's the same defect class on the surface the fix's own comment names as the one that matters most ("the surface agents actually reach through"). An agent calling create_config with kind:"reference" and an existing name could mint unlimited duplicates. Now refuses, mirroring the target-path guard's shape and message.

Finding 2 (P2) — new tests fail 222/593 under bun test --isolate.
Confirmed exactly: config-target-identity.test.ts set HASNA_INSTRUCTIONS_DB_PATH=":memory:" but called getDatabase() with no argument — the self_hosted-mode guard (database.ts:109) checks whether an explicit path argument was passed, never the env var, so on a box with ambient HASNA_INSTRUCTIONS_API_URL/_API_KEY (ordinary fleet state) every call throws. Fixed by passing the path explicitly, the guard's own documented bypass ("Pass an explicit path (e.g. tests) to bypass this guard").

Also found while investigating "0 failing in default invocation": it was not because ambient config is safe. src/mcp/http.test.ts and src/mcp/create-config-target-guard.test.ts delete HASNA_INSTRUCTIONS_API_URL/_API_KEY in beforeEach but never restore them in afterEach. Under bun's default (non-isolated) runner every test file shares one process.env, so whichever of those two files happened to run first silently laundered the ambient cloud config away for every file that ran afterward — masking the real hazard rather than avoiding it. Proven with a two-file control pair (database.test.ts, which never touches these vars, still shows its 7 pre-existing ambient-config failures when paired with the fixed http.test.ts, and showed 0 when paired with the unfixed one). Fixed both files to restore what they delete.

After both fixes, default and --isolate agree exactly: 209/600 fail on a box with ambient cloud config, 0/600 on a clean one. CI is unaffected either way (no ambient cloud vars there). The remaining 209 are a pre-existing, fleet-wide instance of the same env-var misconception across 20 other files, unrelated to #57 — filed as todos 933cb1eb rather than fixed here (out of proportion for a post-merge-findings PR). Also added fresh evidence to the pre-existing, separately-owned todos b19d3d37 (a related but distinct silent-write-to-production hazard on the same two env vars) — confirmed my own repeated full-suite runs with ambient config did not create new live contamination, because PR #57's own exact-name guard already refuses the one write path that's currently reachable that way.

What I decided vs. what the reviewer flagged as out of scope

• Widened findReferenceConfigsByName's and findDuplicateReferenceNameGroups's identity notion from exact-name to slugified-name, rather than leaving them intentionally different — decided they should agree, per the reasoning above. This is a narrowing of the "distinct" bucket, not a new risk category: exact-name matching already treated identical names as one identity pre-fix(add): make --update reachable for reference-kind configs (todos 757cefdb) #57, including in the original "8 rows, one name" corruption this whole feature exists to fix.
• Fixed the MCP create_config reference-kind gap PR fix(add): make --update reachable for reference-kind configs (todos 757cefdb) #57 explicitly deferred — decided it's in scope because it's the same class, live, and reachable right now by the exact caller create_config exists for.
• Did not fix the remaining 20-file fleet-wide HASNA_INSTRUCTIONS_DB_PATH-only isolation pattern (filed separately) or the session.test.ts/project-context.test.ts silent-cloud-write hazard (already owned by b19d3d37) — out of proportion for this PR's scope.

Test plan

• TDD: new regression tests confirmed failing against unmodified code, passing after each fix (unit, CLI, and MCP layers)
• bunx tsc --noEmit — clean
• bun test (default), clean env — 600 pass, 0 fail
• bun test (default) and bun test --isolate, ambient cloud config present — both 391 pass / 209 fail (identical; the pre-existing fleet-wide 20-file pattern, filed as 933cb1eb)
• Verified no live-store contamination from repeated ambient-config test runs (instructions list --json: unchanged 9 pre-existing duplicate rows, latest still 2026-08-02)

Not merged — leaving for review per dispatch instructions.

Agent: t195272ae-driver

---

^{Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.}

[andrei-hasna]

[REVIEW] FIX-FIRST — #58 @ 0c35aa4 — lens: identity-collision correctness and test-isolation blast radius, reviewer instructions-pr58-reviewer (1 of 1)

Reviewed by reading the diff and the surrounding source at head 0c35aa4f0c840ab35ab61ac59f1496a8c4b3d3e6 in a dedicated worktree (~/.hasna/repos/worktrees/instructions/pr58-review-instructions-pr58-reviewer, own lease wt_2ac89db2860b440007a6e466), plus a scrubbed-env bun run typecheck (no store access). I did not run the test suite (the hazard under review is live-store writes from ambient credentials, so I read code instead of executing it, per the dispatch's own constraint). One P1 is real and I recommend fixing it before merge; everything else is GO.

Q1 — Does the identity fix create a false merge?

No new false merge. I tried to construct one on three axes (unicode/diacritic stripping, punctuation-only names collapsing to slugify("") === "", and repeated separators) and in every case the OLD code (config.slug === wantedSlug, comparing the query against each candidate's STORED, possibly-disambiguated slug column) already merged the same pair via its base-slug-holder — e.g. two rows "Café Rules" / "Caf Rules" already collide under OLD because the base holder's stored slug literally equals the second name's slugify() output. NEW (slugify(config.name) === wantedSlug, recomputed from each candidate's own name) doesn't create new equivalence classes — slugify is unchanged — it just closes gaps where OLD missed a disambiguated sibling of a pair it already considered the same identity. I actually found NEW is strictly more correct in one case I traced: a disambiguated sibling's stored slug can coincidentally collide with an unrelated row's own name (e.g. row "FOO"'s bookkeeping-assigned slug happens to land on "foo-2", and a later row literally named "Foo-2" is created) — OLD's stored-slug comparison would wrongly conflate them; NEW (recomputed from the candidate's own name) does not.

One real, pre-existing gap this fix's own tests do not close — see the P1 below.

Also note: the config.name === name clause in findReferenceConfigsByName is now logically redundant under the new comparison (if config.name === name then trivially slugify(config.name) === slugify(name)). Not a bug — the OR is still correct — but the doc comment's claim that it "independently" catches the exact-match case is imprecise. P3, non-blocking.

Worth a one-line hardening, non-blocking: wantedSlug can be "" (e.g. a name of "!!!"), which would treat every punctuation-only name as one identity with every other punctuation-only name. Narrow, but cheap to guard (wantedSlug !== "" && slugify(config.name) === wantedSlug).

THE P1 — add --update's target selection silently picks the wrong row when siblings collide, and this fix widens how often that population exists

Not introduced by this diff — src/cli/index.tsx's const [target, ...rest] = existingOwners; (line ~482) and db/configs.ts's listConfigs ORDER BY category, name (unchanged, untouched by this PR) are both pre-existing. But this PR's whole point is making add --update handle the case/punctuation-duplicate population correctly, and for that exact population the selection is not just "arbitrary" — it is systematically wrong in a predictable direction:

• SQLite's default ORDER BY name is a binary/ASCII comparison, so 'Sample Rule' (uppercase S, 83) sorts before 'sample rule' (lowercase s, 115) — always, regardless of creation order.
• existingOwners[0] becomes target with no preference for an exact name match.
• So: two existing rows "Sample Rule" and "sample rule" (exactly the shape doctor-reference-duplicates.test.ts's own new test seeds). Run add file.md --name "sample rule" --kind reference --update (i.e. update the lowercase one, by its own exact name). existingOwners = ["Sample Rule" row, "sample rule" row] (alphabetical), so target = the "Sample Rule" row — the wrong one gets its content overwritten, not the row whose exact name was typed. The CLI does print ✓ Updated: Sample Rule and a yellow "1 other row(s) still share this name" warning, so it's not silent to a careful human reader, but a script or a quick glance sees exit 0 and moves on with the wrong row changed.
• I checked the PR's own new test for exactly this shape (add-reference-update.test.ts, "--update on a name colliding only after slugification…") — it queries --name "Sample Rule" (the alphabetically-first name), so existingOwners[0] happens to already be the queried row and the test's contents.filter(...) assertions are (correctly, per their own comment) agnostic about which one wins. It does not exercise the reverse direction, so it does not catch this.
• This is squarely in the "dangerous direction, overwrites someone's row" the dispatch asked me to attack, and it's data integrity (silent overwrite of the unintended row's content) — one of the categories the bounded-review policy allows to block.

Recommended minimal fix, in src/cli/index.tsx where existingOwners is destructured:

const exactMatch = existingOwners.find((o) => o.name === name);
const target = exactMatch ?? existingOwners[0]!;
const rest = existingOwners.filter((o) => o !== target);

Small, in the same file this PR already edits conceptually (the add --update reference path), and it removes the ASCII-sort dependency entirely for the common case. I'd like this in this PR rather than a follow-up, since the PR's stated purpose is specifically making add --update safe for this population.

Q2 — MCP create_config guard

Correct and consistent with the CLI. kind === "reference" in the MCP handler now mirrors the CLI's create-without---update behavior exactly (refuse, never silently update) — the right choice, since create_config has no --update equivalent; update_config is the separate, explicit-choice path, matching the CLI's own add/add --update split. Error wording matches the CLI's almost verbatim, adapted for MCP verb names (update_config/delete_config vs add --update/delete <id>).

One inconsistency, not introduced by this PR and not touched by it: update_config performs no identity check at all (not for target_path, not for name) — a rename via update_config can silently collide with another row's name/path and nothing stops it. This is uniform across kinds (file-kind's target_path isn't checked on update either), so it reads as a deliberate scope boundary (checks only gate creation) rather than a reference-kind-specific gap. Out of scope for this PR; worth a task if it isn't tracked already.

Also confirmed (not obviously, so worth stating): z (zod) is imported in server.ts but never used — there is no runtime validation against the declared inputSchema. A create_config call missing name while kind: "reference" would hit slugify(undefined) inside findReferenceConfigsByName and throw a raw TypeError; the top-level try/catch around the dispatch converts it to an err() response rather than crashing the server, so it's a poor error message, not a crash — and it's consistent with how every other required-but-unvalidated field in this same handler already behaves (e.g. missing content/category). Pre-existing, not specific to this diff. P3.

Q3 — Test isolation: can any test in this repo still reach the live hosted store?

I read resolveConfigStore() and getDatabase() directly (src/data/config-store.ts, src/db/database.ts) rather than trusting any comment. The only two variables that gate cloud vs. local are HASNA_INSTRUCTIONS_API_URL and HASNA_INSTRUCTIONS_API_KEY. HASNA_INSTRUCTIONS_STORAGE_MODE — which doctor-reference-duplicates.test.ts clears alongside the other two — does not appear anywhere in src/**/*.ts(x); it exists only in Dockerfile/docker-compose.yml (server-side deployment config, a different process). Clearing it in tests is harmless over-caution, not load-bearing, contrary to what that file's own comment implies.

Per file in the diff:

file	mechanism	verdict
src/cli/add-reference-update.test.ts	spawns the CLI as a child process with an explicit env object clearing API_URL/API_KEY (not touched by this PR — runCli is pre-existing and unchanged)	PROVEN SAFE. Subprocess env manipulation never touches the parent test-runner's process.env, so there is no cross-file leakage risk by construction, and the two vars that actually gate store selection are cleared.
src/cli/doctor-reference-duplicates.test.ts (new)	same subprocess pattern, clears 3 vars (one inert)	PROVEN SAFE, same reasoning.
src/lib/config-target-identity.test.ts	every getDatabase() call converted to getDatabase(TEST_DB_PATH) (:memory:) — I grepped the full post-PR file and confirmed all 17 call sites carry the explicit path	PROVEN SAFE, and by the strongest mechanism of the five: passing an explicit path bypasses the !path && API_URL && API_KEY guard entirely, so ambient env is irrelevant regardless of what it is.
src/mcp/create-config-target-guard.test.ts	deletes API_URL/API_KEY in beforeEach (save/restore added by this PR), and asserts expect(resolveConfigStore().mode).toBe("local") before running any test	PROVEN SAFE, and the best-instrumented of the five — it verifies the property rather than merely requesting it.
src/mcp/http.test.ts	same delete pattern, now with save/restore added by this PR — but no live mode === "local" assertion like its sibling	PROVEN SAFE by direct mechanism (delete process.env[key] is deterministic; resolveCloudConfig reads env[key] straight off process.env, so a successful delete is sufficient), but not self-verified the way create-config-target-guard.test.ts is. Non-blocking inconsistency — cheap to add the same one-line assertion for defense in depth. P2.

The fix genuinely closes the hazard it claims to: I confirmed http.test.ts and create-config-target-guard.test.ts are the two files todos 195272ae Finding 2 names ("two test files were deleting … without restoring"), and the restore logic added here matches the pattern already established (and unmodified by this PR) in src/mcp/mcp.test.ts and src/status.test.ts — so this PR is bringing two lagging files into line with the repo's own existing convention, not inventing a new one.

Spot-checked but out of scope (not touched by this PR, not exhaustively verified): src/data/config-store.test.ts deletes the two vars in afterEach with no visible save/restore, but every test in it passes an explicit env parameter to resolveCloudConfig/resolveConfigStore/isCloudMode rather than relying on process.env defaults, so the missing restore may not actually matter there — I did not verify every test case in that file, and it's unrelated to this PR's diff, so I'm not blocking on it. Worth a follow-up glance, not urgent.

Q4 — Do the new tests actually fail pre-fix?

Yes, confirmed by manual trace (not by running the suite) on the two load-bearing unit tests:

• findReferenceConfigsByName([first, second], "Sample Rule") where second is a disambiguated sibling (slug: "sample-rule-1"): under the OLD comparison (config.slug === wantedSlug), only first matches (first.slug === "sample-rule", second.slug !== "sample-rule") → old code returns length 1, test asserts length 2. Fails pre-fix.
• findDuplicateReferenceNameGroups([first, second]) for the same pair: OLD groups by raw config.name, so "Sample Rule" and "sample rule" land in separate singleton groups → filtered out (rows.length > 1 never true) → old code returns [], test asserts length 1. Fails pre-fix.
• The MCP guard's new tests exercise a code path (if (kind === "reference") {...}) that did not exist at all pre-PR — second.isError would be false, not true. Fails pre-fix by construction, not by subtle behavior.

I did not independently re-derive the CLI-level (add-reference-update.test.ts, doctor-reference-duplicates.test.ts) test failures line-by-line, but they exercise the same two underlying functions I already traced, through no alternate code path — I'm confident they fail pre-fix for the same reason, not merely asserting it.

Q5 — Base staleness

Not stale. git rev-parse refs/pull/58/merge^1 and git rev-parse origin/main both resolve to e85f23d80aabd24669ddf0b38c0cd2ba3169e0d6 — what CI tested against is exactly current main, resolved from the branch (git rev-parse origin/main), not the PR object's base field. No retarget, no drift; the check the amendment to this rule prescribes is satisfied trivially since the two values are already equal.

Other

bun run typecheck (tsc --noEmit) on the head commit, env-scrubbed (env -u HASNA_INSTRUCTIONS_API_URL -u HASNA_INSTRUCTIONS_API_KEY -u HASNA_INSTRUCTIONS_STORAGE_MODE), touches no store: exit 0, no output.

Verdict

FIX-FIRST. One P1 (target-selection ordering can silently overwrite the wrong row for the exact population this PR targets — fix proposed above, small and localized). Everything else — the core identity-matching fix, the MCP consistency, and the test-isolation fix — is correct, evidence-backed, and I'd call GO on its own. P2/P3 items listed are explicitly non-blocking follow-ups, not conditions for this PR.

Non-blocking follow-ups (file separately if not already tracked): the wantedSlug === "" degenerate-name guard; the redundant exact-name clause's doc-comment precision; update_config's total lack of identity checking (both kinds, pre-existing); the unused zod import / lack of runtime schema validation in server.ts (pre-existing, affects every required field, not just this PR's addition); http.test.ts missing the live resolveConfigStore().mode === "local" self-check its sibling has; a look at config-store.test.ts's unrestored afterEach (likely harmless given explicit env params, not verified exhaustively).

[andrei-hasna]

[REVIEW] NO_GO — #58 @ 0c35aa4 — lens: correctness+security+gates, reviewer unresolved-account003 (1 of 1)

Candidate reviewed

• Confirmed local HEAD is 0c35aa4f0c840ab35ab61ac59f1496a8c4b3d3e6 and origin/main is the supplied fresh base e85f23d80aabd24669ddf0b38c0cd2ba3169e0d6.
• Read git log --oneline origin/main..HEAD, git diff origin/main...HEAD --stat, and the full base-to-head diff for all 7 changed files.
• Read surrounding implementation and tests in src/lib/config-target-identity.ts, src/cli/index.tsx, src/mcp/server.ts, src/mcp/http.ts, src/data/config-store.ts, src/db/configs.ts, src/db/database.ts, src/server/v1.ts, src/storage/cloud-store.ts, src/storage/schema.ts, the changed test files, and package.json.

Commands and exact results

• bun install — exit 0; setup only, 158 packages installed. This is not reported as a test gate.
• bun run typecheck — exit 0; PASS (tsc --noEmit, no pass/fail count emitted).
• bun run test — exit 1; FAIL: 391 pass, 209 fail, 600 tests across 50 files.
• Focused real-MCP transport reproduction after setup (bun -e using startMcpHttpServer, Client, and StreamableHTTPClientTransport) — exit 0; two distinct reference rows were created, update_config renamed the second to a case variant of the first, and the store contained Alpha Reference / alpha-reference plus alpha reference / alpha-reference-1.

Blocking findings

1. P1, high confidence — update_config bypasses the new reference-name identity invariant (src/mcp/server.ts:192-201).

   • Reachable path: an authorized MCP caller creates reference rows named Alpha Reference and Beta Reference, then calls update_config on the second with name: "alpha reference".
   • Flow: MCP update_config forwards name directly to store.updateConfig; uniqueSlug disambiguates the slug to alpha-reference-1 instead of refusing the colliding slugified name.
   • Impact: two reference rows again own one semantic identity, recreating the exact corruption this PR says create_config must prevent; doctor reports it only after the fact, and the code itself states only one row is live in the next render.
   • Evidence: the focused transport reproduction completed with rename_error:false and returned the two colliding rows named above.
   • Required remedy: apply the same reference-name collision check to MCP renames, excluding the row being updated, and add a real-transport regression showing a rename to an exact or slugified sibling name is refused without mutating either row. Prefer enforcing the invariant at the authoritative write boundary so direct API/SDK writes cannot bypass it.

2. P1 required-gate failure — the repository-declared test gate is red on the exact candidate.

   • bun run test exited 1 with 391 pass / 209 fail. This is the exact invocation declared in package.json and required by this review.
   • The failures are currently reachable under the ambient API configuration this PR explicitly calls ordinary fleet state. Restoring those variables in the changed MCP tests exposes the remaining suite-wide isolation defects rather than leaving the declared gate green.
   • Required remedy: make the declared suite self-isolating across the remaining affected tests so unmodified bun run test exits 0 in the supported fleet environment, then rerun the exact gate.

Non-blocking follow-ups

• P3 documentation drift: the new comment in src/cli/add-reference-update.test.ts still says the MCP create_config path has no reference-kind guard, while this same PR adds that guard. This does not affect runtime behavior.
• No other concrete, currently reachable in-scope P0/P1 finding survived review.

Verdict: NO_GO. Do not merge this SHA. The update-path integrity bypass and the required test gate must both be green before focused re-review.

[andrei-hasna]

[REVIEW] GO — #58 @ 82002a1 — lens: remediation of the add --update exact-name P1 and its direct regressions, reviewer instructions-pr58-reviewer (re-review, cycle 1 of 2)

Scope: only the named P1 and its direct regressions, per the bounded re-review request. I did not re-open Q1/Q2/Q3/Q4/Q5 from cycle 1, and I did not relitigate the second reviewer's two findings — noted below, not re-adjudicated.

The delta, confirmed

git diff 0c35aa4f pr58-new-head --stat: exactly 2 files, src/cli/index.tsx (+21/−1) and src/cli/add-reference-update.test.ts (+50/−0) — matches the dispatch's description precisely. Full PR diffstat (origin/main..82002a11) is 8 files / +419/−29, also exact. Ran the diff through the same secrets-shaped grep as before: 0 matches.

The fix, read directly

const exactIndex = isReference ? existingOwners.findIndex((owner) => owner.name === name) : -1;
const target = exactIndex >= 0 ? existingOwners[exactIndex] : existingOwners[0];
const rest = existingOwners.filter((owner) => owner.id !== target!.id);

This is the fix I proposed last cycle, functionally — prefer an exact stored-name match, fall back to array order (still ORDER BY category, name, still uppercase-before-lowercase) when no exact match exists among the candidates (e.g. a third spelling that only collides via slug, not by literal name — correctly falls back, since there's no "right" row to prefer there). rest is computed by filtering out the target's id, not by array position, which is more robust than a plain .slice(1) and gives the identical result to it whenever target is index 0.

Scoping claim — "file-kind is byte-for-byte unchanged" — verified, not just trusted

For isReference === false, exactIndex is unconditionally -1, so target = existingOwners[0] — identical to the old const [target, ...rest] = existingOwners. For rest: existingOwners for file-kind comes from findConfigsByTargetPath, a .filter() over allConfigs (a DB row set with a unique id primary key per row), so it can never contain two entries with the same id. That means .filter(o => o.id !== target.id) removes exactly the element at index 0 and preserves order for everything else — byte-for-byte the same array as the old .slice(1). I looked for a test that exercises rest.length > 0 for file-kind (i.e. more than one existing owner on one target_path) to back this with more than logic — there isn't one, in either the old or new test files (add-duplicate-target.test.ts's multi-owner scenarios all expect(second.status).not.toBe(0), i.e. the second add is always refused before a second owner can exist, so rest is always [] there). This gap is pre-existing and not introduced by this PR; the code-level proof above is solid on its own, but flagging that "unchanged" rests on reading the code, not on a passing test for that exact shape.

The regression test — empirically fails pre-fix, passes post-fix (not decoration)

I did not take this on trace alone. Confirmed both var presence and this run with a real bun-level check (not a bash -c wrapper — see the note below), all commands redirected to files, $? read directly:

• Post-fix (pr58-new-head as committed): bun test src/cli/add-reference-update.test.ts -t "targets the EXACT name match" → 1 pass, 0 fail, rc=0.

• Pre-fix, isolated: swapped only src/cli/index.tsx back to the 0c35aa4f version (git checkout 0c35aa4f -- src/cli/index.tsx), keeping the new test file as-is, confirmed via git diff --cached that the staged change was exactly the fix's inverse and touched nothing else. Same test, same command → rc=1, one failure:

  expect(targeted[0]!.content).toBe("three\n")
  - "three
  + "two
  

  This is exactly the predicted failure mode: pre-fix, add --update --name "sample rule" overwrites "Sample Rule" (alphabetically first) instead of "sample rule" (the row actually named), so the row that should have gotten the new content ("three\n") still holds its old content ("two\n"). Restored index.tsx to head afterward, confirmed clean tree.

• Full suite, post-fix, my own run: 601 pass, 0 fail, matching the fixer's own reported number exactly. Ran with all three vars confirmed unset immediately beforehand via a direct bun -e check (not a shell wrapper).

One thing worth recording since it bears on "prove them unset" specifically: my first attempt to verify env-unset used env -u VAR bash -c '...', and it reported the vars as still SET inside the subshell — BASH_ENV is set on this box and re-sources on every non-interactive bash startup, silently reinjecting them. Switching to env -u VAR bun -e '...' (no intermediate shell) correctly showed both unset, and that's the form I used for every actual test run above. Noting this because it's exactly the failure class this fleet's credential-hygiene rules warn about (a wrapper that appears to unset a variable without the property actually holding) — worth a memento on my end, not a finding about this PR.

The two things flagged, not relitigated

• Test-gate red (NO_GO finding Add package-manager secret ingress guard #2): not re-run by me beyond the full-suite pass above, which independently reproduces the fixer's clean number (601/0) under my own scrubbed-env verification. I'm treating the ambient-credential explanation as settled per your evidence and not re-deriving it myself.
• update_config identity bypass (NO_GO finding Preserve spark02 repo state #1): I'd already written, in my own cycle-1 review before this was raised, that update_config has no identity check for either kind and reads as "a deliberate scope boundary... out of scope for this PR" — so I agree with routing it to 642d2585 on my own reasoning, not merely deferring. Nothing here changes that.

Nothing to contradict — I don't think the write-boundary finding should block this PR; my own review reached the same scope line independently.

Verdict

GO. The named P1 is fixed correctly, minimally, and scoped exactly as claimed (verified, not just read). The regression test is real — empirically confirmed to fail pre-fix and pass post-fix, isolated to the one changed file. No direct regressions: full suite 601/0 on my own independent run. Nothing else in scope for this cycle.

[andrei-hasna]

[REVIEW] NO_GO — #58 @ 82002a1 — lens: correctness+security+gates, reviewer unresolved-account001 (1 of 1)

Exact candidate reviewed

• Confirmed HEAD 82002a11645a5cb97bed2cf1bf8ed2922c924a7d against freshly fetched origin/main e85f23d80aabd24669ddf0b38c0cd2ba3169e0d6.
• Ran git log --oneline origin/main..HEAD and git diff origin/main...HEAD --stat, then read the full diff of all eight changed files.
• Read surrounding CLI add/doctor logic, MCP create/update handlers and transport tests, slug/identity helpers, SQLite config mutations, ConfigStore local/cloud implementations, PostgreSQL cloud-store create/update paths, /v1/configs REST handlers, schema migrations, and the declared package scripts.

Commands and gates

• bun install — exit 0; setup only, 158 packages installed. This is not reported as a test gate.
• bun run typecheck — exit 0; 0 TypeScript diagnostics.
• bun run test — exit 1; 392 pass, 209 fail, 1,454 expectations, 601 tests across 50 files.
• Diagnostic re-run after unsetting HASNA_INSTRUCTIONS_API_URL and HASNA_INSTRUCTIONS_API_KEY in the invoking shell, still using the exact bun run test command — exit 1 with the same 392 pass / 209 fail counts. This does not replace or erase the required gate result above.
• Cloud-backed MCP concurrency reproduction — exit 0 as an instrument run; 10 simultaneous same-name create_config calls returned 10 successes / 0 errors and produced 10 reference rows.
• Sequential MCP rename reproduction — exit 0 as an instrument run; two distinct references were created, then update_config renamed the second from Reference Beta to reference alpha while Reference Alpha existed. The update returned no error and left two same-identity rows (reference-alpha, reference-alpha-1).

Blocking findings

1. P1, high confidence — the new one-reference-name/one-row invariant is a client-side check-then-write and remains bypassable on supported write paths. src/mcp/server.ts:164 reads all owners, then src/mcp/server.ts:177 performs a separate create. In API mode those are distinct GET/POST network operations (src/data/config-store.ts:339-375), while the authoritative REST/PG create path (src/server/v1.ts:75-80, src/storage/cloud-store.ts:168-195) has no atomic normalized-name constraint. The 10-way transport reproduction made every concurrent caller observe an empty owner set and all ten writes succeeded. Independently, the MCP update_config path accepts a name change without the reference-identity check and the sequential reproduction created a collision with no concurrency. Impact is the exact data/session-integrity failure this PR says it prevents: multiple reference rows share one semantic identity and render/apply order is undefined. Remedy: enforce normalized reference-name uniqueness atomically at the authoritative datastore/API boundary for both create and rename, reconcile existing duplicates so the migration is deployable, and add concurrent cloud-backed plus update-rename regression tests. A process-local mutex or another MCP-only preflight is insufficient across machines and direct API clients.
2. Required gate blocker — the repository-declared test gate is red. The explicitly required bun run test command exited 1 with 209 failures. The PR body identifies most as the known ambient/test-isolation class, but this review task makes the declared test script an acceptance gate, so a red result cannot be treated as GO. The clean-shell diagnostic also remained red with identical counts, showing the current declared gate is not hermetic on this reviewer lane.

Security review

• No credential value was read or printed, and no separate reachable secret exposure was found in the changed code.

Non-blocking follow-ups

• None. P2/P3 style, documentation, and speculative hardening observations were excluded from the verdict.

Verdict: NO_GO. Leave the PR open. The atomic datastore/API invariant and the declared test gate need to pass before this exact head can be merged.