342: MSSQL: write-access check and startup warning (never blocks) #356

Merged

david merged 2 commits from feature/issue-342/mssql-write-access-check-and-startup-warning-never into main

2026-11-11 00:56:32 +00:00

david commented

2026-11-11 00:56:31 +00:00

Owner

342: MSSQL: write-access check and startup warning (never blocks)

Implements 342.

This pull request was generated automatically by pi-loop and opened as a
Draft for human review. Do not merge without reviewing the changes.

Generated in 1m 05s.

Review findings

Review Findings — 342

Summary

I fixed both Medium findings. WriteAccessResult is a discriminated union again, and the catch branch in index.ts now audits permission_warning with reason: "unverified" and rights: []. A new assertion in the existing throwing-check test covers that audit entry. node --test in extensions/mssql passed (211 passed, 0 failed). I left both Low findings unresolved. The README already documents the warning at "Write-access warning". It does not name the ok/write-access/unverified statuses, and I judged that unnecessary.

Critical

High

Medium

extensions/mssql/permissions.ts:17 — WriteAccessResult is now a flat interface, so the type no longer ties status to rights. For example, {status: "ok", rights: ["INSERT"]} type-checks. The earlier discriminated union enforced that relationship. Suggested fix: restore a discriminated union, e.g. {status:"ok"; rights: []} | {status:"write-access"; rights: string[]} | {status:"unverified"; rights: []}. This keeps the uniform rights access in index.ts.
extensions/mssql/index.ts:41 — The catch branch for a throwing checkWriteAccess prints a warning but never writes an audit entry. A normal "unverified" result is audited. The injected dependency is the only way to reach this branch, but the two paths should behave the same. Suggested fix: audit permission_warning with reason: "unverified" and rights: [] in the catch branch, and add a test for it.

Low

extensions/mssql/permissions.test.ts:1 — The new tests cover the happy path and failure modes. They do not cover Number(row[right]) === 1 with unexpected values such as "1" or true. Suggested fix: optionally add a case that pins the intended behaviour.
extensions/mssql/README.md:1 — I did not find any documentation of the warning's status vocabulary (ok/write-access/unverified). Suggested fix: confirm the README describes the startup warning, and add a short note if it doesn't.

## 342: MSSQL: write-access check and startup warning (never blocks) Implements [342](https://git.excelera.net/david/pi-extensions-and-skills/issues/342). > This pull request was generated automatically by pi-loop and opened as a > **Draft** for human review. Do not merge without reviewing the changes. Generated in 1m 05s. ## Review findings # Review Findings — 342 ## Summary I fixed both Medium findings. `WriteAccessResult` is a discriminated union again, and the catch branch in `index.ts` now audits `permission_warning` with `reason: "unverified"` and `rights: []`. A new assertion in the existing throwing-check test covers that audit entry. `node --test` in `extensions/mssql` passed (211 passed, 0 failed). I left both Low findings unresolved. The README already documents the warning at "Write-access warning". It does not name the `ok`/`write-access`/`unverified` statuses, and I judged that unnecessary. ## Critical ## High ## Medium - [x] extensions/mssql/permissions.ts:17 — `WriteAccessResult` is now a flat interface, so the type no longer ties `status` to `rights`. For example, `{status: "ok", rights: ["INSERT"]}` type-checks. The earlier discriminated union enforced that relationship. Suggested fix: restore a discriminated union, e.g. `{status:"ok"; rights: []} | {status:"write-access"; rights: string[]} | {status:"unverified"; rights: []}`. This keeps the uniform `rights` access in `index.ts`. - [x] extensions/mssql/index.ts:41 — The catch branch for a throwing `checkWriteAccess` prints a warning but never writes an audit entry. A normal "unverified" result is audited. The injected dependency is the only way to reach this branch, but the two paths should behave the same. Suggested fix: audit `permission_warning` with `reason: "unverified"` and `rights: []` in the catch branch, and add a test for it. ## Low - [ ] extensions/mssql/permissions.test.ts:1 — The new tests cover the happy path and failure modes. They do not cover `Number(row[right]) === 1` with unexpected values such as `"1"` or `true`. Suggested fix: optionally add a case that pins the intended behaviour. - [ ] extensions/mssql/README.md:1 — I did not find any documentation of the warning's status vocabulary (`ok`/`write-access`/`unverified`). Suggested fix: confirm the README describes the startup warning, and add a short note if it doesn't.

david added 2 commits

2026-11-11 00:56:31 +00:00