Kjell Tore Guttormsen
55c94ee964
Lifts the original v4.0.0 Non-Goal: an optional, manually-entered `saves`
metric through the analytics layer, built location-agnostic (option c) so
UI-brief §9b/M0 relocates the data dir in one place later.
- types: PostMetrics.saves? + Weekly/Monthly summary.totalSaves? (optional);
new RankableMetric type for the always-numeric index-access whitelist
- parser: dedicated parseOptionalCount() — blank/non-numeric/negative -> undefined
("unknown != 0"), genuine 0 kept; saves NOT folded into engagementRate
- reports: totalSaves set only when >=1 post carries saves (backward-compat)
- cli: saves surfaced in import summary + weekly/monthly totals + per-post
- S16-pre: onboarding.md allowed-tools gains Write (closes S15-deferred MAJOR)
- docs (three-doc rule): plugin README boundary + analytics README + root README
+ plugin CLAUDE.md + CHANGELOG; dwell stays explicitly unmeasurable
Independent /trekreview: brief-conformance 0 findings; code-correctness 2 MAJOR
(own lockstep misses) FIXED in-session (parseOptionalCount + edge tests). Gate:
tsc clean, analytics 116/116, lint 74/0/0, hooks 98/98. Within-v4.1.0 refinement
(no surface/count/version change).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
154 lines
8.3 KiB
Markdown
154 lines
8.3 KiB
Markdown
---
|
||
type: trekreview
|
||
review_version: "1.0"
|
||
task: "S16 — saves manual-entry surface (lifts the original Non-Goal): optional saves in the analytics data model (types + parser + weekly/monthly + CLI), built location-agnostic for M0 (option c); + S16-pre: close the deferred onboarding Write MAJOR"
|
||
slug: remediation
|
||
project_dir: docs/remediation/
|
||
brief_path: docs/remediation/brief.md
|
||
scope_sha_start: 8c52bdb
|
||
scope_sha_end: 8c52bdb
|
||
reviewed_files_count: 15
|
||
verdict: ALLOW
|
||
mode: default
|
||
effort: standard
|
||
profile: premium
|
||
findings: []
|
||
---
|
||
|
||
# Review — linkedin-studio S16 (saves manual-entry + S16-pre onboarding Write)
|
||
|
||
## Executive Summary
|
||
|
||
**Verdict: ALLOW** for S16's delivered scope — 0 BLOCKER, 0 MAJOR, 0 MINOR, 0 SUGGESTION
|
||
**open** against the diff. Two independent reviewers (brief-conformance, code-correctness)
|
||
ran COLD, without cross-feeding, on the as-delivered uncommitted working tree (HEAD
|
||
`8c52bdb` + the S16 delta).
|
||
|
||
- **brief-conformance-reviewer:** **0 findings.** Every Success Criterion traces to delivered
|
||
code (SC1 saves in types + parser + import/report; SC2 saves in the actionable-signal output;
|
||
SC3 dwell still explicitly unmeasurable, no fabricated field; SC4 backward-compat + no-crash;
|
||
SC5 onboarding `Write`). Every Non-Goal honored: **M0 not built** (I/O still routes through the
|
||
unmodified `getAnalyticsRoot()` seam — option (c)), dwell not fabricated, surface counts
|
||
(29/19/6/9) and version (v4.1.0) unchanged, no not-mine file touched.
|
||
- **code-correctness-reviewer:** **2 MAJOR — both in S16's own new code, both FIXED in-session**
|
||
(see Findings). Neither is pre-existing; both are lockstep misses in the saves parser delivered
|
||
this session, so per the operator rule ("in-session fix of the session's *own* misses =
|
||
completion") they were corrected here, not deferred.
|
||
|
||
The M0 conflict (UI-brief §9b) was reconciled **before** building, by operator decision =
|
||
**option (c): build now, location-agnostic**. The conformance reviewer independently confirmed
|
||
the build honors it (no new hardcoded `assets/analytics` path; `storage.ts` untouched).
|
||
|
||
## Coverage
|
||
|
||
Scope: HEAD `8c52bdb` (S15's commit) + the **uncommitted S16 working-tree delta** (annotated
|
||
`[uncommitted]` — a brief-level contract; the brief's Assumptions allow uncommitted review).
|
||
The 3 untracked not-mine files (`docs/linkedin-studio-persona-brief.md`, `…-ui-brief.md`,
|
||
`docs/voyage-build/progress.json`) are explicitly excluded from scope and from the commit.
|
||
**No silent skips.**
|
||
|
||
| Treatment | Count | Notes |
|
||
|-----------|-------|-------|
|
||
| `deep-review` | 0 | nothing under `hooks/**` / `auth/**` / `crypto/**` / `**/security/**` (analytics CLI + one command frontmatter) |
|
||
| `summary-only` | 15 | analytics src (5) + analytics tests (3) + 2 fixtures + assets/analytics/README.md + README.md + CHANGELOG.md + commands/onboarding.md |
|
||
| `skip` | 0 | no lockfiles / svg / generated / dist |
|
||
|
||
**Execution criteria (orchestrator-run):**
|
||
- `npx tsc --noEmit` (analytics) → **clean** (the new `RankableMetric` type closes the
|
||
`metrics[keyof]` widening the optional `saves` would otherwise introduce in `alerts.ts` + `cli.ts`).
|
||
- `node --import tsx --test tests/*.test.ts` (analytics) → **116/116**.
|
||
- `bash scripts/test-runner.sh` → **74 passed / 0 failed / 0 warnings**, exit 0.
|
||
- `node --test hooks/scripts/__tests__/*.test.mjs` → **98/98** (no hook logic changed).
|
||
- `ls commands/*.md` → **29** (no surface-count change; no version bump — within-v4.1.0 refinement, S11–S13 precedent).
|
||
- **E2E:** import + weekly + monthly with a saves fixture surfaces saves without crashing;
|
||
saves-free fixtures round-trip byte-identical; explicit `"0"` → `0`, blank/`"n/a"` → undefined.
|
||
|
||
## Findings
|
||
|
||
**0 open findings.** Code-correctness raised 2 MAJOR; both are **S16's own** new code (not
|
||
pre-existing — the saves parser is new this session), so both were **fixed in-session** as
|
||
completion of the delivered work, then re-verified green. Recorded below — not dropped, not
|
||
silenced.
|
||
|
||
### [MAJOR — FIXED in-session] Non-numeric Saves cell silently coerced to 0 (`unknown != 0` contract violated)
|
||
|
||
*Raised by code-correctness (`PLAN_EXECUTE_DRIFT`), `scripts/analytics/src/parsers/csv-parser.ts`.*
|
||
|
||
The original guard `if (savesRaw && savesRaw.trim() !== "")` then `metrics.saves =
|
||
parseMetric(savesRaw)` gated only on emptiness; `parseMetric` ends in `parseFloat(...) || 0`
|
||
+ `Math.max(0, …)`, so a non-empty non-numeric cell (`"n/a"`, `"~40"`) or a negative value
|
||
flattened to **0** — surfaced as a confident "top engagement signal". This contradicts the
|
||
`unknown != 0` contract stated in the same diff (`types.ts` saves NOTE + the parser comment).
|
||
|
||
**Fix (this session):** added a dedicated `parseOptionalCount()` helper returning
|
||
`number | undefined` — blank / non-numeric / negative → `undefined`; a genuine number
|
||
(including explicit `"0"`) → that number; reuses the EU/US separator normalization. The saves
|
||
parse now routes through it. The blank-cell path is unchanged; the garbage/negative path now
|
||
honors the contract.
|
||
|
||
### [MAJOR — FIXED in-session] No test for the explicit-`0` vs non-numeric Saves boundary
|
||
|
||
*Raised by code-correctness (`MISSING_TEST`), `scripts/analytics/tests/csv-parser.test.ts`.*
|
||
|
||
The original saves tests covered `42`, blank→undefined, no-column→undefined, and
|
||
engagement-exclusion — but not the exact boundary where the parser's behavior diverged from
|
||
its intent: a literal `"0"` (genuine zero) vs a non-numeric cell (unknown).
|
||
|
||
**Fix (this session):** added `tests/fixtures/saves-edge-export.csv` (a `"0"` row + an `"n/a"`
|
||
row) and two cases pinning `saves === 0` for `"0"` and `saves === undefined` for non-numeric.
|
||
The non-numeric case was **red** before the parser fix above and **green** after (TDD), so the
|
||
contract boundary is now regression-guarded.
|
||
|
||
### [MAJOR — CLOSED] (carried from S15) onboarding Phase 2 file-saves need `Write`
|
||
|
||
*S15's deferred finding.* `commands/onboarding.md` Phase 2 saves voice/user-profile files but
|
||
the frontmatter omitted `Write`. **Closed in S16-pre:** `Write` added to `allowed-tools`
|
||
(matching `first-post.md:9-14`).
|
||
|
||
## Remediation Summary
|
||
|
||
**Gate: ALLOW** for S16's delivered scope. brief-conformance is clean; code-correctness's two
|
||
MAJORs were S16's own lockstep misses (not pre-existing design findings), so both were fixed
|
||
in-session and re-verified (tsc clean, analytics 116/116, lint 74/0/0, hooks 98/98) — this is a
|
||
genuine ALLOW with **no open finding**, not a WARN-override. The M0 conflict was reconciled
|
||
before building (option c, location-agnostic); the conformance reviewer confirmed M0 was not
|
||
built and the data-dir seam (`getAnalyticsRoot()`) is untouched, so the planned migration
|
||
relocates the root in one place. The S15-deferred onboarding `Write` MAJOR is closed.
|
||
|
||
Per Handover 6, this `review.md` is consumable by `/trekplan --brief …`. ALLOW → S16 commits +
|
||
pushes (own files only); the finish-plan continues at **S17** (C13–C46 triage).
|
||
|
||
```json
|
||
{
|
||
"verdict": "ALLOW",
|
||
"verdict_scope": "S16 delivered changes (saves manual-entry + S16-pre onboarding Write); 15 files",
|
||
"scope": { "sha_start": "8c52bdb", "sha_end": "8c52bdb", "reviewed_files_count": 15, "uncommitted_delta": true },
|
||
"counts": { "BLOCKER": 0, "MAJOR": 0, "MINOR": 0, "SUGGESTION": 0 },
|
||
"findings": [],
|
||
"fixed_in_session": [
|
||
{
|
||
"severity": "MAJOR",
|
||
"title": "Non-numeric Saves cell silently coerced to 0 (unknown != 0 contract)",
|
||
"file": "scripts/analytics/src/parsers/csv-parser.ts",
|
||
"rule_key": "PLAN_EXECUTE_DRIFT",
|
||
"resolution": "added parseOptionalCount() returning number|undefined (blank/non-numeric/negative -> undefined; genuine 0 kept); saves parse routes through it"
|
||
},
|
||
{
|
||
"severity": "MAJOR",
|
||
"title": "No test for explicit-0 vs non-numeric Saves boundary",
|
||
"file": "scripts/analytics/tests/csv-parser.test.ts",
|
||
"rule_key": "MISSING_TEST",
|
||
"resolution": "added saves-edge-export.csv fixture + 2 cases (0 -> 0, n/a -> undefined); TDD red-before/green-after the parser fix"
|
||
},
|
||
{
|
||
"severity": "MAJOR",
|
||
"title": "onboarding Phase 2 file-saves need Write (carried from S15)",
|
||
"file": "commands/onboarding.md",
|
||
"rule_key": "PLAN_EXECUTE_DRIFT",
|
||
"resolution": "S16-pre: added Write to allowed-tools, matching first-post.md"
|
||
}
|
||
],
|
||
"deferred_findings": [],
|
||
"dropped_findings": []
|
||
}
|
||
```
|