mirror of
https://github.com/Permissionless-Software-Foundation/psf-memo.git
synced 2026-09-21 16:52:01 -07:00
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.
This commit is contained in:
Chris Troutner
@@ -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 <common-root>/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/<task>-summary.md`. The role
|
||||
requires the summary to be committed with the byline in the same commit as the
|
||||
|
||||
@@ -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.
|
||||
@@ -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"
|
||||
}
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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, /<iframe[^>]+src="https:\/\/www\.youtube\.com\/embed\/dQw4w9WgXcQ"/)
|
||||
assert.match(html, /<iframe[^>]+allowfullscreen/i)
|
||||
assert.match(html, /<a[^>]+href="https:\/\/memo\.fullstackcash\.net"/)
|
||||
assert.doesNotMatch(html, /<a[^>]+href="https:\/\/youtu\.be\/dQw4w9WgXcQ"/)
|
||||
})
|
||||
|
||||
@@ -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',
|
||||
|
||||
Reference in New Issue
Block a user