fix(linkedin-studio): S13 — close S12 WARN ($-scalar + false-green test) + $-safety lint guard

Closes the 2 grep/Read-verified findings from the S12 cold full-brief re-review
(docs/remediation/review.md, WARN 0/1/1/0, 0 dropped) and closes the $-injection
CLASS — not the line — across the whole state-updater.mjs mutation surface.

See docs/remediation/review.md (S13 ALLOW, 0/0/0/0) for the full closure record:
replaceField -> replacement function; the 3 additive-insert sites -> functions
(m === $1, behavior-preserving); a scalar assert.match pins last_post_topic; and a
behavioral, coverage-complete, self-testing Section 12 guard (check-replace-safety.mjs)
that is mutation-proven. Docs three-doc + residuals updated. test-runner.sh 71/0/0,
node --test 98/98.
This commit is contained in:
Kjell Tore Guttormsen 2026-05-30 19:12:45 +02:00
commit 431a893f7c
10 changed files with 665 additions and 9 deletions

View file

@ -296,7 +296,7 @@ Before testing, ensure:
1. First create a test plan (Test 18) and manually create a test file with sample data
2. Run `/linkedin:ab-test` and select "Analyze test results"
3. Select the test to analyze
**Expected:** Results comparison table, significance assessment (20% rule), verdict, recommended next steps
**Expected:** Results comparison table, directional assessment (≥20% gap), verdict, recommended next steps
**Validates:** File scanning, data analysis, result formatting
### Test 20: Enhanced Report — Trends & Alerts

View file

@ -279,6 +279,45 @@ gaps) → Phase 3 (long-form earn / redundancy measurement).
- The plugin's own lint (`scripts/test-runner.sh`, rebuilt) and any `node --test`
suites pass on the final state.
## Amendment (2026-05-30) — Finish scope (S13S17)
After the v4.0.0 remediation pushed (Phase 03 done; S12 review WARN, 2 findings open),
the operator commissioned a **finish pass** to close every remaining hole to a clean
ALLOW. Plan: `docs/remediation/finish-plan.md`. This amendment folds the new scope into
the contract so the review gate measures it as in-scope (not Non-Goal violation).
**Re-opened Non-Goals (superseded by operator decision 2026-05-30):**
- *"Not changing the /linkedin:* command invocation surface"* — **re-opened** for S14
command rationalization (a deliberate keep/develop/merge/cut pass; merges/cuts only on
explicit per-command operator approval).
- *"Not adding a manual-entry feature for saves/dwell"* — **re-opened for saves only**
(S16). Dwell stays unmeasurable (internal-only); no dwell surface is built.
**New Success Criteria (Finish):**
- **S13:** `replaceField` (`state-updater.mjs:14-18`) inserts untrusted `last_post_topic`
literally (replacement function); a `$`-bearing test pins the `last_post_topic` scalar;
a structural `$`-safety lint (Section 12) fails on any string-replacement whose value
derives from an untrusted parameter; `/trekreview` → ALLOW.
- **S14:** `docs/remediation/command-rationalization.md` records a per-command
keep/develop/merge/cut recommendation for all commands; every merge/cut is operator-
approved; the lint count-guard + all rosters + three-doc agree with `ls commands/*.md`.
- **S15:** `/linkedin:onboarding` produces a draft post inline (no `Run /linkedin:first-post`
dead-end); `/linkedin` router is tiered (≤4 primary, ~1K-gated commands flagged
"locked"); `/linkedin:carousel` copies the full deck (all slides + caption), not just
the caption.
- **S16:** a manual saves value can be entered and surfaces in `/linkedin:report` without
crashing; CSV-only data still works (backward-compatible); dwell remains explicitly
unexportable.
- **S17:** every uncalibrated audit finding C13C46 has a recorded disposition
(`docs/remediation/c13-c46-triage.md`: still-real / already-fixed / outdated-drop); every
still-real one is grep-verified closed.
**Pending / out-of-band (not yet sequenced — operator will time it):** two additional
briefs the operator flagged — `docs/linkedin-studio-persona-brief.md` and
`docs/linkedin-studio-ui-brief.md`. If their scope conflicts with S13S17 (esp. the UI
brief vs S15 router/onboarding/carousel UX), reconcile before executing the overlapping
session; do not let the finish pass pre-empt a decision the operator hasn't made.
## Research Plan
*The internal/file-level fixes (analytics-CLI crash, dead lint, voice-leak,

View file

@ -0,0 +1,126 @@
# LinkedIn Studio — Finish Plan (S13S17)
> Closes every remaining hole: the open S12 review findings + the gaps surfaced in the
> 2026-05-30 verified assessment. Brief-amendment to `docs/remediation/brief.md` (same
> project). Each session is one STATE.md-driven Voyage session, gated by `test-runner.sh`
> + `node --test` + `/trekreview` → push **only on ALLOW** (no more WARN-overrides).
>
> **Operator decisions (2026-05-30):** build manual saves entry (S16); triage C13C46
> (S17); fold into this project as a brief-amendment. **Dependency:** S14 (command set)
> precedes S15 (router tiering) — the router can't be tiered before the command set is final.
## Sequence & dependency
```
S13 (close S12 WARN + $-class) → ALLOW [finishes the ORIGINAL brief]
S14 (command rationalization) → ALLOW [sets the final command set]
S15 (UX §6c — router on final set) → ALLOW
S16 (saves manual entry) → ALLOW
S17 (C13C46 triage) → ALLOW [process complete]
```
---
## S13 — Close the open S12 findings + the `$`-replacement class
*Finishes the original brief; brings the existing scope to a clean ALLOW.*
- **A1 (MINOR):** `hooks/scripts/state-updater.mjs:14-18` — convert `replaceField` to a
replacement **function** (`(_m) => \`${field}: ${value}\``) so the untrusted
`last_post_topic` at `:58` is inserted literally.
- **A2 (MAJOR):** add `assert.match(result.content, /^last_post_topic: "\$100 budget — \$& and \$1 rule"$/m)`
to the existing `$`-bearing test in `state-updater.test.mjs` (fails today, passes after A1).
- **A3 (systemic):** audit every `String.replace` in `hooks/scripts/*.mjs` whose replacement
is a **string** built from a function parameter that can carry user content
(`grep -nE '\.replace\([^,]+, *\`'`). Confirm `replaceField` was the last such site.
Add a **structural `$`-safety lint** (`test-runner.sh` Section 12) that flags
string-replacement sites whose value derives from an untrusted parameter — non-vacuity
self-test + e2e mutation-proof, mirroring Sections 8/10/11.
- **Engine:** inline (small, surgical).
- **Verify:** A2 assert FAILS pre-A1, PASSES post-A1 · `node --test` green · `test-runner.sh`
green (incl. new Section 12 self-test) · audit-grep returns 0 unsafe sites ·
`/trekreview`**ALLOW** → commit (review.md + S13) → push.
## S14 — Command rationalization (re-opens the original command-surface Non-Goal)
*Analysis → operator decision → execute. Nothing deleted without explicit per-command yes.*
- **14a Analysis (no edits):** cold per-command review of all 27 → `docs/remediation/command-rationalization.md`.
Per command: purpose · overlap with siblings · invocation leverage (algorithmic + likely use) ·
recommendation **keep / develop / merge→X / cut** + rationale. Delegate the cold read to an
Agent (Opus) for independence.
- **14b Operator decision:** present the doc; operator decides per command (`AskUserQuestion`
batched). No mechanical deletion until approved.
- **14c Execute approved:** apply merges/cuts; for a merge, fold the source command's unique
surface into the target and delete the source; update `EXPECT_COMMANDS` in `test-runner.sh`,
all rosters (CLAUDE.md/README/SKILL.md/router), CHANGELOG, version bump if the surface count
changes (breaking → minor/major per SemVer judgment).
- **Engine:** Agent (14a) → inline (14c).
- **Verify:** `ls commands/*.md | wc -l` == every declared count · lint count-guard green ·
three-doc synced · `grep` old count → 0 stale · `/trekreview`**ALLOW** → push.
## S15 — UX finish (§6c), on the FINAL command set
- **B1 Onboarding inline:** `commands/onboarding.md` — replace the
`"Run /linkedin:first-post"` hand-off with the first-post steps embedded in the wizard, so
the flow produces a draft post inline (no dead-end). *Verify:* a walkthrough yields a draft
inside onboarding; 0 `Run /linkedin:first-post` dead-end strings. **Scope guard (UI brief
§12b):** fix the dead-end ONLY — do NOT add extensibility/provider "seams" or progressive-
disclosure config to onboarding; those are unresolved UI-brief decisions (keep onboarding
lean per the persona "first value without forking").
- **B2 Router tiering:** `commands/linkedin.md` — restructure into **Primary** (34:
post/quick/newsletter/firsthour), **Secondary** (the rest of the final set), **Locked ~1K**
(monetize/outreach/competitive, marked "unlocks later"). *Verify:* tier sections present;
primary ≤4; locked commands flagged, not inline with primaries.
- **B3 Carousel full-deck clipboard:** `commands/carousel.md` — assemble the **entire deck**
(every slide's copy + the caption) into the clipboard payload, not just the caption.
*Verify:* clipboard payload contains slide text; grep shows full-deck assembly before the
`clipboard-helper.mjs` call.
- **Engine:** inline.
- **Verify:** all three grep/observation checks pass · `test-runner.sh` green ·
`/trekreview`**ALLOW** → push.
## S16 — Saves manual-entry surface (operator-requested; lifts the original Non-Goal)
> ⚠️ **CONFLICT — reconcile before building (UI brief §9b/M0).** The UI brief makes it
> **binding** that all mutable personal data (`assets/analytics/*`, `queue.json`, `*.local.md`)
> moves OUT of the plugin tree into a stable per-user data dir, "in the v4.0.0 remediation or
> immediately after." S16 extends the analytics data model — if built against the current
> in-tree `assets/analytics/` it gets **reworked by M0**. Decision needed: (a) do **M0 first**
> (insert as S15.5), then build S16 in the final location; or (b) **defer S16** to ride along
> with M0/the UI build. Do NOT build S16 blind to M0.
- Add a manual-entry path for **saves** (visible in native LinkedIn post analytics, count-only,
~Sept 2025) to the analytics data model (`scripts/analytics/` types + import/report path),
additive and backward-compatible. Re-rank the actionable-signal output to include saves where
the CSV/manual data now contains it. **Dwell stays explicitly unmeasurable** (internal-only) —
do not fabricate a dwell surface.
- **Engine:** inline (+ analytics CLI knowledge; may need `tsx` types touch).
- **Verify:** a report run with a manual saves value surfaces it without crashing; existing
CSV-only data still works (backward-compat); honesty wording retained for dwell ·
`node --test` / analytics tests green · `/trekreview`**ALLOW** → push.
## S17 — Triage the uncalibrated audit findings (C13C46)
- Read the ~34 findings the audit never put through a second hostile pass (`§10`); for each:
classify **still-real / already-fixed / outdated-drop**; close every still-real one; record the
disposition in `docs/remediation/c13-c46-triage.md`. Delegate the cold read to an Agent (Opus).
- **Engine:** Agent (triage) → inline (fixes).
- **Verify:** every C13C46 finding has a recorded disposition; still-real ones grep-verify closed ·
`test-runner.sh` + `node --test` green · `/trekreview`**ALLOW** → push. **Process complete.**
---
## Verification (whole plan)
- **Per session:** `bash scripts/test-runner.sh` exit 0 · `node --test hooks/scripts/__tests__/*.test.mjs`
all pass · `/trekreview --project docs/remediation/`**ALLOW** (not WARN) before push.
- **Plan-complete signal:** all of S13S17 pushed on ALLOW; `command-rationalization.md` +
`c13-c46-triage.md` committed; no open `/trekreview` finding; STATE.md "Aktiv oppgave" reads
"remediering FERDIG — ren ALLOW".
- **Counts contract stays live:** the lint's count-guard (19/27/25/6 today; 27 may change in S14)
is updated in lockstep with any command merge/cut; `grep` for the prior count returns 0 stale hits.
## Locked constraints (inherited from brief)
- Opus on everything · no hidden costs (cost-warn `/trekcontinue`·`/trekreview`, standing yes) ·
three-doc rule · version-sync · bash 3.2 + Node-only hooks · push only to Forgejo · stage own
files only · fix-in-next-session for any review finding.

View file

@ -0,0 +1,166 @@
---
type: trekreview
review_version: "1.0"
task: "Remediate linkedin-studio from the baseline audit — correctness, honesty, generalization, and the highest-leverage 2026 coverage gaps (full Phase 03 roadmap, phased)"
slug: remediation
project_dir: docs/remediation/
brief_path: docs/remediation/brief.md
scope_sha_start: c5b4c58f4f390aca83c8937880c5fd0bcc983e44
scope_sha_end: 36f79dd702b9315a0cd9100c3a8dd6dd81b3797f
reviewed_files_count: 50
verdict: ALLOW
mode: default
effort: high
profile: premium
findings: []
---
# Review — linkedin-studio audit-remediation (S13 re-review: `$`-class closure + scalar-test fix)
## Executive Summary
**Verdict: ALLOW** — 0 BLOCKER, 0 MAJOR, 0 MINOR, 0 SUGGESTION.
This is the **S13 re-review** — the **seventh** full-brief sweep
(`c5b4c58..36f79dd` + the uncommitted S13 working-tree delta), run COLD and
high-effort. S13 was commissioned to close the two findings the S12 re-review
left open (verdict WARN, 0/1/1/0): the MAJOR `MISSING_TEST` (the S12 `$`-bearing
test asserted the Recent Posts section but never the `last_post_topic` scalar, so
the corruption shipped green) and the MINOR `MISSING_ERROR_HANDLING` (the
`last_post_topic` scalar was still `$`-unsafe because `replaceField` used a
replacement *string* for untrusted content). Two independent reviewers
(brief-conformance, code-correctness) ran without cross-feeding; the coordinator
applied bounded dedup + the HubSpot Judge filters + verdict (high-effort →
Cloudflare reasonableness filter skipped; the operator weighs borderline findings).
**Both reviewers returned empty finding sets.**
**Both S12 findings are CLOSED at the reviewed (uncommitted) state:**
- `replaceField` (`hooks/scripts/state-updater.mjs:14-24`) now uses a replacement
**function** (`() => \`${field}: ${value}\``), so the untrusted `last_post_topic`
at the `:64`-equivalent call site is inserted verbatim — no `$&`/`$1`/`` $` ``
expansion. The MINOR is closed.
- The existing `$`-bearing test (`hooks/scripts/__tests__/state-updater.test.mjs`)
now carries `assert.match(result.content, /^last_post_topic: "\$100 budget — \$&
and \$1 rule"$/m, …)`, distinct from the section-entry `includes()` it already
had. This assertion **fails on the old string-`replaceField` and passes on the
function form** (orchestrator-verified by reverting the fix: the test went
40 pass / 1 fail). The false-green MAJOR is closed.
**The class — not just the line — is closed.** The recurring S9→S12 lesson is
"close the class, not the line"; the class here is "untrusted user content reaching
ANY `String.replace` replacement *string*". Beyond the `replaceField` scalar, S13
also converted the three remaining additive-insert sites (`recordFirstHourPlan`
`:246`-equiv; `recordOutreachContact` `:305/:308`-equiv) from a string replacement
carrying an intentional `$1` backref + interpolated date to a replacement function
(`(m) => \`${m}\n…\``). The code-correctness reviewer verified rigorously that this
is **behavior-preserving**: each regex's capture group spans the *entire* match (the
only chars outside the group are the zero-width `^`/`$` anchors), so the full match
`m` is character-identical to the old `$1`. After S13, **every `.replace()` in
`state-updater.mjs` uses a replacement function or a `$`-free literal** — the class
is closed by construction, not by per-line patch.
**A structural guard replaces the per-line proof.** New
`scripts/check-replace-safety.mjs` (wired as `test-runner.sh` Section 12) proves the
property behaviorally: it drives every exported mutator with an adversarial payload
of every special replacement token (`$&`, `` $` ``, `$'`, `$$`, `$n`) in every
free-text *and* date field and asserts the payload survives verbatim. Two structural
backstops run on every invocation — **coverage-completeness** (a newly-exported
mutator without `$`-coverage fails the guard) and a **non-vacuity self-test** (a
naive string-replace MUST corrupt the payload and a function MUST preserve it, else a
PASS is meaningless), mirroring the Section 8/10/11 self-tests. The orchestrator
mutation-proved it end-to-end: reverting `replaceField` to a string makes the guard
exit 1 with two findings; restoring it returns exit 0.
**No Phase-03 Success Criterion regressed.** The brief-conformance reviewer traced
each S13 clause to delivered code and confirmed the counts (19 agents / 27 commands /
25 references / 6 skills), the version (4.0.0), the single-source algorithm-signal,
the model-consistency guard, and the render-chain-propagation guard all still hold;
S13 touched no command/agent/reference file. The two Non-Goals the brief amendment
re-opens (the command invocation surface for S14; saves manual-entry for S16) trace
to **explicit operator decisions** in the brief amendment, and S13 itself did not
touch either surface — no `SCOPE_CREEP_BUILT`.
**Push decision: ALLOW.** The two S12 findings are closed, the class is closed
structurally, the lint is non-vacuous and mutation-proven, all suites are green
(`scripts/test-runner.sh` → 71/0/0; `node --test` → 98/98), and no SC regressed. The
ORIGINAL remediation brief now closes clean. Per `feedback_trekreview_always_last` +
Handover 6, this review is the gate; with ALLOW, S13 may push.
## Coverage
Scope SHA range: `c5b4c58` (= `origin/main`, parent of remediation Steg 1) →
`36f79dd` (HEAD, the S12 commit) **plus the uncommitted S13 working-tree delta**
(annotated `[uncommitted]` — a brief-level contract; the brief's Assumptions allow
uncommitted review). The committed range (47 files) was already deep-reviewed and
cleared at S12 except the 2 WARN findings; the active S13 delta is the 9 working-tree
files below. **No silent skips.**
| Treatment | Count | Notes |
|-----------|-------|-------|
| `deep-review` (hooks/** + the new guard) | 4 | `state-updater.mjs`, `state-updater.test.mjs` `[uncommitted]`; `scripts/check-replace-safety.mjs` `[uncommitted, new]`; `scripts/test-runner.sh` `[uncommitted]` |
| `summary-only` | 46 | the committed `c5b4c58..36f79dd` range (already cleared at S12) + the S13 doc edits `CLAUDE.md`/`README.md`/`docs/integration-test-guide.md` `[uncommitted]` + `docs/remediation/{brief.md (amendment), finish-plan.md}` `[uncommitted]` |
| `skip` | 0 | no lockfiles / svg / generated / dist |
**Cross-cutting execution criteria (run by orchestrator):**
`scripts/test-runner.sh` → 71 passed / 0 failed / 0 warnings, exit 0 (was 70;
+1 — Section 12 `$`-safety guard).
`node --test hooks/scripts/__tests__/*.test.mjs` → 98 tests, 98 pass, 0 fail (the
S13 scalar assertion was added to an existing test, not a new test).
`node scripts/check-replace-safety.mjs` → exit 0 (8 adversarial cases / 5 mutators,
coverage-complete, self-test non-vacuous); mutation-proven (reverting the fix → exit 1).
**S12 findings — both confirmed CLOSED:**
`replaceField` (`state-updater.mjs:14-24`) → replacement function; the `$`-bearing
test now pins the `last_post_topic` scalar (`state-updater.test.mjs`); the three
remaining additive-insert string-replacements (`:246/:305/:308`) → functions; the
class is closed and guarded structurally (Section 12).
## Findings
None. Both independent reviewers returned empty finding sets; the coordinator's
bounded passes (dedup, HubSpot Judge, verdict) on an empty set yield ALLOW.
## Remediation Summary
**Gate: ALLOW.** The S12 WARN is fully resolved: the `replaceField` scalar
`$`-corruption (MINOR) and the false-green test (MAJOR) are both closed; the
`$`-injection class is closed across the whole `state-updater.mjs` mutation surface;
and a behavioral, coverage-complete, self-testing Section-12 lint guards it
structurally against future regressions. All suites green; no Phase-03 SC regressed;
the two re-opened Non-Goals trace to explicit operator decisions and S13 stayed in its
lane.
**Two non-blocking observations, recorded by both reviewers, neither rising to a
catalogue finding:**
1. The S13 dead binding both reviewers named (`check-replace-safety.mjs` `HERE` +
its now-unused `node:url`/`node:path` imports) was removed during this review
pass; the guard remains green (exit 0) after removal.
2. The behavioral guard catches a new *unguarded exported mutator* (coverage
backstop) but not a new unsafe `String.replace` added *inside* an existing
battery-covered mutator on a field the battery does not fuzz. This is a documented,
deliberate limit of the behavioral proxy (vs a per-`.replace()` AST enumeration)
and is **moot today** — every `.replace()` in `state-updater.mjs` is already a
function. Recorded as a known boundary, not a defect; closing it further is out of
S13 scope (no real `$`-unsafe site exists to catch).
The two adjacent machine-value `.replace()` sites the correctness reviewer probed —
`session-start.mjs:396` (`${actualWeek}`, a computed ISO week) and `week-rollover.mjs`
(computed week / literal int) — carry no untrusted content and are therefore not
members of the defect class, consistent with how the S12 review itself classified the
date/integer `replaceField` call sites. No finding.
Per Handover 6, this `review.md` is consumable by
`/trekplan --brief docs/remediation/review.md`; the trailing JSON block is the machine
contract for that handover. With an ALLOW verdict and no BLOCKER/MAJOR findings, no
follow-up remediation plan is required — the ORIGINAL brief is closed clean and the
finish-plan continues at S14.
```json
{
"verdict": "ALLOW",
"scope": { "sha_start": "c5b4c58f4f390aca83c8937880c5fd0bcc983e44", "sha_end": "36f79dd702b9315a0cd9100c3a8dd6dd81b3797f", "reviewed_files_count": 50, "uncommitted_delta": true },
"counts": { "BLOCKER": 0, "MAJOR": 0, "MINOR": 0, "SUGGESTION": 0 },
"findings": [],
"dropped_findings": []
}
```