From b63792fb7b149f08096ac00958192dd56fe0c425 Mon Sep 17 00:00:00 2001 From: Chris Troutner Date: Tue, 15 Sep 2026 19:33:46 -0700 Subject: [PATCH] Review post-link-formatting: kill link-parser boundary mutants Harden the post link parser after the refactorer handoff: - add start-of-text and leading-token boundary unit tests and drop a redundant range guard, killing the isBareDomainBoundary survivors - replace the mutable React key counter with React.Children.toArray and assert the embedded iframe is fullscreen, killing both PostContent mutation survivors - record the client verification pass and the review summary By architect. --- docs/architect-process-notes.md | 10 ++ docs/reviews/post-link-formatting-summary.md | 95 +++++++++++++++++++ .../post-link-formatting-verification.json | 46 +++++++++ .../specs/post-link-formatting.feature | 4 + .../src/components/post-feed/post-content.js | 13 ++- psf-memo-client/src/services/post-links.js | 7 +- .../test/unit/post-content.test.js | 1 + psf-memo-client/test/unit/post-links.test.js | 11 +++ 8 files changed, 181 insertions(+), 6 deletions(-) create mode 100644 docs/reviews/post-link-formatting-summary.md create mode 100644 docs/reviews/post-link-formatting-verification.json diff --git a/docs/architect-process-notes.md b/docs/architect-process-notes.md index 6d5bdb6..7c0aca5 100644 --- a/docs/architect-process-notes.md +++ b/docs/architect-process-notes.md @@ -100,6 +100,16 @@ distinct from per-task verification results, which live in ## Workflow observations +- **`architect-startup.sh` checks `tmp/aps` relative to the worktree, but + `ensure-aps.sh` resolves the single canonical APS checkout to the git common + root's `tmp/aps` (the main checkout). In a role worktree the two disagree, so + the startup script reports `[FAIL] tmp/aps missing` even though the tools run. + Local workaround: symlink the worktree's ignored `tmp/aps` to the canonical + checkout, `ln -sfn /tmp/aps tmp/aps`. Proper fix (for a task that + owns tooling): have `architect-startup.sh` capture + `APS_DIR="$(ensure-aps.sh --update)"` and check/run against `$APS_DIR` + instead of the relative path. + - **Review summaries must be force-added.** `docs/` is in the root `.gitignore`, so `git add -A` silently skips `docs/reviews/-summary.md`. The role requires the summary to be committed with the byline in the same commit as the diff --git a/docs/reviews/post-link-formatting-summary.md b/docs/reviews/post-link-formatting-summary.md new file mode 100644 index 0000000..2733875 --- /dev/null +++ b/docs/reviews/post-link-formatting-summary.md @@ -0,0 +1,95 @@ +# Review summary: post-link-formatting + +**Architect review of the refactorer handoff for task `post-link-formatting`.** + +## Commits reviewed +- `968970b` (specifier): Spec post link formatting + (`psf-memo-client/specs/post-link-formatting.feature`). +- `6c7952f` (coder): Format post URLs and bare domains as links — pure + `parsePostLinks` service (`src/services/post-links.js`), link rendering in + `src/components/post-feed/post-content.js`, acceptance handlers/adapter, unit + tests. +- `e17c515` (refactorer): Add property tests for post link parsing + (`test/property/post-links.property.test.js`). + +Merged onto the architect worktree (`swarmforge-architect`) by fast-forwarding +to `e17c515`. + +## Architectural findings and fixes applied +The structure is sound and matches the codebase's layered layout. UI/Core +separation, the dependency rule, and information hiding all hold: + +- **UI/Core separation:** `src/services/post-links.js` is a pure text parser + (`parsePostLinks`) with no React or network dependencies, so it is unit- and + property-testable directly. `src/components/post-feed/post-content.js` is a + pure presentational component written in plain `React.createElement` style + (no JSX, no I/O), so the same module drives the browser build and the Node + acceptance adapter. +- **Dependency rule:** the component depends inward on the two pure services + (`post-links`, `youtube-embed`); neither service depends outward on the + component, the DOM, or the acceptance layer. +- **Information hiding:** the parser exposes only the segment shape + (`{type:'text'}` / `{type:'link', href, text}`); the renderer hides the anchor + and iframe markup (target/rel/class/allow/referrerPolicy). + +Three issues left by the handoff were fixed: + +1. **Unkilled boundary mutants (information hiding / edge cases).** The first + mutation run left 3 survivors in `isBareDomainBoundary`. I added two unit + tests — a bare domain at the *start* of the text (must link) and a bare + domain glued to a leading token character such as `-example.com` (must stay + text) — and removed the redundant `end < input.length` guard before + `input[end] === '@'` (an out-of-range index is `undefined`, so the guard only + created an equivalent mutant). `post-links.js` is now 15 killed / 0 survived. +2. **Unkillable React key mutant (cohesion).** `post-content.js` incremented a + mutable `key` counter whose starting value (`0 -> 1`) is unobservable in + rendered output. I now build the children without keys and let + `React.Children.toArray` assign stable positional keys; the rendered markup + is unchanged and the mutant site is gone. +3. **Boolean-attribute survivor.** Added an `allowfullscreen` assertion to the + embedded-YouTube render test, killing the `allowFullScreen: true -> false` + mutant. + +Noted but intentionally left as-is: `post-links.js` repeats a +trailing-punctuation regex and a `pushText` helper from `youtube-embed.js`. +`dry4javascript` reports no duplicate candidates, and single-sourcing a shared +URL tokenizer would be a cross-module refactor beyond this task. + +## Verification results + +### Language mutation (`mutate4javascript`, differential, `--max-workers 8`) +- **`src/services/post-links.js`**: 15 killed, **0 survived**, 0 uncovered. +- **`src/components/post-feed/post-content.js`**: 1 killed, **0 survived**, 0 uncovered. + +### DRY (`dry4javascript`) +- Changed files (`post-links.js`, `post-content.js`): **no duplicate candidates**. + +### CRAP / cyclomatic complexity (`crap4javascript`) +All changed functions are below the 8.0 threshold and 100% covered: +`parsePostLinks` (CC 6, CRAP 6.0), `PostContent` (CC 5, CRAP 5.0), +`isBareDomainBoundary` (CC 4, CRAP 4.0), `pushText` (CC 2, CRAP 2.0). + +### Soft Gherkin acceptance mutation (`gherkin-mutator --level soft`) +- **`post-link-formatting.feature`**: 21 executed, **18 killed**, **3 survived**. + All three survivors are single-character case mutations of unasserted + trailing words in the `text` example (`herE`, `rePly`, `livE`). Each scenario + asserts the link `href` (and, for bare domains, the visible link text); the + trailing prose is setup-only and never asserted, so mutating its case cannot + change the outcome. These are weak example-to-assertion connections, not + implementation gaps; no change warranted. The tool wrote a per-scenario + acceptance-mutation manifest stamp for the fully-killed YouTube scenario and + correctly withheld a full-feature stamp because survivors remain. + +## Suite status +`swarmforge/scripts/verify.sh client --record …` → **pass (5/5)**: +- unit **329 passing**, property **50 passing**, acceptance **all 26 suites + passing**, lint **pass**, build **pass**. + +## Handoffs sent +- End-of-chain `git_handoff` to the specifier (`priority: 50`, task + `post-link-formatting`) with the review commit so it can merge + `swarmforge-architect` into `master`. +- No coder/refactorer handoff: the review commit is mutation/test hardening plus + a behavior-preserving renderer refactor, with no follow-up work for them. + +By architect. diff --git a/docs/reviews/post-link-formatting-verification.json b/docs/reviews/post-link-formatting-verification.json new file mode 100644 index 0000000..801dcb0 --- /dev/null +++ b/docs/reviews/post-link-formatting-verification.json @@ -0,0 +1,46 @@ +{ + "schema_version": 1, + "task": "post-link-formatting", + "component": "psf-memo-client", + "git_sha": "e17c5150653fd0f0822e6e125b5ea08c87801f0f", + "branch": "swarmforge-architect", + "timestamp": "2026-09-16T02:32:50.247Z", + "commands": [ + { + "name": "unit", + "command": "npm test", + "exit": 0, + "duration_ms": 10807, + "summary": "329 pass / 0 fail" + }, + { + "name": "property", + "command": "npm run test:property", + "exit": 0, + "duration_ms": 8523, + "summary": "50 pass / 0 fail" + }, + { + "name": "acceptance", + "command": "npm run test:acceptance", + "exit": 0, + "duration_ms": 12495, + "summary": "all 26 acceptance suites passed" + }, + { + "name": "lint", + "command": "npm run lint", + "exit": 0, + "duration_ms": 6392, + "summary": "ok" + }, + { + "name": "build", + "command": "npm run build", + "exit": 0, + "duration_ms": 71628, + "summary": "ok" + } + ], + "result": "pass" +} diff --git a/psf-memo-client/specs/post-link-formatting.feature b/psf-memo-client/specs/post-link-formatting.feature index 2d96210..0fcdd02 100644 --- a/psf-memo-client/specs/post-link-formatting.feature +++ b/psf-memo-client/specs/post-link-formatting.feature @@ -1,3 +1,7 @@ +# acceptance-mutation-manifest-begin +# {"version":1,"tested_at":"2026-09-16T02:30:54.209408268Z","feature_name":"Post Link Formatting","feature_path":"/home/trout/work/psf-memo/.worktrees/architect/psf-memo-client/specs/post-link-formatting.feature","background_hash":"0d66780cb1b8e277f0ada40a8ffe336dec7a8eaf658f19d2ea344815fb9bf26c","implementation_hash":"unknown","scenarios":[{"index":3,"name":"Post Link Formatting - 4 an embeddable YouTube link is embedded instead of rendered as a plain link","scenario_hash":"909b9c6f64851c05dec405b81a9fdb8946f83d2d8445aa60e75dc7f7c845f539","mutation_count":3,"result":{"Total":3,"Killed":3,"Survived":0,"Errors":0},"tested_at":"2026-09-16T02:30:54.209408268Z"}]} +# acceptance-mutation-manifest-end + # Scenarios: Post Link Formatting - 1, Post Link Formatting - 2, Post Link Formatting - 3, Post Link Formatting - 4 # # When a post's text contains a URL, the client renders it as an anchor that diff --git a/psf-memo-client/src/components/post-feed/post-content.js b/psf-memo-client/src/components/post-feed/post-content.js index 2ff30e4..dcfcf04 100644 --- a/psf-memo-client/src/components/post-feed/post-content.js +++ b/psf-memo-client/src/components/post-feed/post-content.js @@ -14,7 +14,6 @@ const { const { parsePostLinks } = require('../../services/post-links') function PostContent ({ text = '' }) { - let key = 0 const children = [] for (const segment of parsePostText(text)) { @@ -23,7 +22,6 @@ function PostContent ({ text = '' }) { React.createElement( 'div', { - key: key++, className: 'posts-feed-item-youtube' }, React.createElement('iframe', { @@ -45,7 +43,6 @@ function PostContent ({ text = '' }) { React.createElement( 'a', { - key: key++, href: link.href, target: '_blank', rel: 'noopener noreferrer', @@ -55,12 +52,18 @@ function PostContent ({ text = '' }) { ) ) } else { - children.push(React.createElement('span', { key: key++ }, link.text)) + children.push(React.createElement('span', null, link.text)) } } } - return React.createElement(React.Fragment, null, ...children) + // React.Children.toArray assigns stable positional keys without a mutable + // counter, so the rendered output does not depend on key values. + return React.createElement(React.Fragment, null, ...React.Children.toArray(children)) } module.exports = PostContent + +// mutate4javascript-manifest-begin +// {"version":1,"tested_at":"2026-09-16T02:29:59.905Z","module_hash":"bbc07a4d5e0d9bdf95e1d9014603fbe4ad38611469d51e006b082d32b5bd03c6","functions":[{"id":"func/PostContent","name":"PostContent","line":16,"end_line":63,"hash":"d2202910528f0974b6eaa1892d770e1a46a11e02b5b953c1a8490be59f349895"}]} +// mutate4javascript-manifest-end diff --git a/psf-memo-client/src/services/post-links.js b/psf-memo-client/src/services/post-links.js index c0a2df4..5f4ed9f 100644 --- a/psf-memo-client/src/services/post-links.js +++ b/psf-memo-client/src/services/post-links.js @@ -20,7 +20,8 @@ const LINK_RE = /(https?:\/\/[^\s]+)|((?:[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\.)+[a-z // such as an email address. function isBareDomainBoundary (input, start, end) { if (start > 0 && /[A-Za-z0-9_.@-]/.test(input[start - 1])) return false - if (end < input.length && input[end] === '@') return false + // input[end] is undefined when end === input.length, so no range guard is needed. + if (input[end] === '@') return false return true } @@ -74,3 +75,7 @@ function parsePostLinks (text) { } module.exports = { parsePostLinks } + +// mutate4javascript-manifest-begin +// {"version":1,"tested_at":"2026-09-16T02:28:07.979Z","module_hash":"9d4045719f423239c99405aadb91cd21944e7d2deaa670b15eb53aee5ee66639","functions":[{"id":"func/isBareDomainBoundary","name":"isBareDomainBoundary","line":21,"end_line":26,"hash":"6cbcecc22428ecc54a2598d4401a350b3d07a849b651e5c76198b9defb927aef"},{"id":"func/pushText","name":"pushText","line":29,"end_line":31,"hash":"abda2060349c814451b88fe350ca235069afdc769eaa3ffa90e9d214673c71b9"},{"id":"func/parsePostLinks","name":"parsePostLinks","line":40,"end_line":75,"hash":"7a51da000274bf77614f288f9db2ba8490069202919a00cda9c3c474d7bf48d8"}]} +// mutate4javascript-manifest-end diff --git a/psf-memo-client/test/unit/post-content.test.js b/psf-memo-client/test/unit/post-content.test.js index 189fa3c..c5f33e6 100644 --- a/psf-memo-client/test/unit/post-content.test.js +++ b/psf-memo-client/test/unit/post-content.test.js @@ -44,6 +44,7 @@ test('renders no anchor for plain text', () => { test('renders an embedded YouTube player and a separate link together', () => { const html = render('watch https://youtu.be/dQw4w9WgXcQ then read https://memo.fullstackcash.net') assert.match(html, /]+src="https:\/\/www\.youtube\.com\/embed\/dQw4w9WgXcQ"/) + assert.match(html, /]+allowfullscreen/i) assert.match(html, /]+href="https:\/\/memo\.fullstackcash\.net"/) assert.doesNotMatch(html, /]+href="https:\/\/youtu\.be\/dQw4w9WgXcQ"/) }) diff --git a/psf-memo-client/test/unit/post-links.test.js b/psf-memo-client/test/unit/post-links.test.js index 6c14c6a..5cf4e0b 100644 --- a/psf-memo-client/test/unit/post-links.test.js +++ b/psf-memo-client/test/unit/post-links.test.js @@ -101,6 +101,17 @@ test('parsePostLinks does not link a dotted user name before an @ sign', () => { assert.deepEqual(parsePostLinks(text), [{ type: 'text', text }]) }) +test('parsePostLinks links a bare domain at the start of the text', () => { + assert.deepEqual(parsePostLinks('memo.fullstackcash.net is live'), [ + { type: 'link', href: 'https://memo.fullstackcash.net', text: 'memo.fullstackcash.net' }, + { type: 'text', text: ' is live' } + ]) +}) + +test('parsePostLinks does not link a bare domain glued to a leading token character', () => { + assert.deepEqual(parsePostLinks('-example.com'), [{ type: 'text', text: '-example.com' }]) +}) + test('parsePostLinks round-trips: segments reconstruct the original text', () => { const samples = [ 'just a normal memo',