build(preflight): install CommonMark runtime - #312
Conversation
Punchcard-Session: brisk-workshop-valley-n7
|
Codex review: blocked before merge. Reviewed September 4, 2026, 11:00 AM ET / 15:00 UTC. ClawSweeper reviewWhat this changesThe PR pins CommonMark, installs it in three GitHub workflows, and adds a contract test for parser availability and Markdown text extraction. Merge readiness⛔ Blocked before merge - 8 items remain Keep open: this member-authored prerequisite remains distinct from current main, but its newly added test has the same unresolved P1 failure identified in the previous review cycle. Priority: P2 Review scores
Verification
How this fits togetherClownfish workflows validate and preflight repository automation before guarded external merges. This change provides a locked Markdown parser runtime intended for review-text interpretation and installs it before those workflow commands run. flowchart LR
A[Workflow checkout] --> B[Install locked dependencies]
B --> C[Validation and preflight commands]
C --> D[CommonMark parser runtime]
D --> E[Review-text interpretation]
E --> F[Guarded external merge decision]
Decision needed
Why: This introduces a lasting third-party runtime and CI installation boundary, while the intended parser consumer remains a separate open PR. Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Land the parser runtime only with a corrected visible-text regression fixture and a current-main rebase, ideally coordinated with the paired consumer at #311 so the permanent dependency has a real production use. Do we have a high-confidence way to reproduce the issue? Yes, from source: the introduced AST walk cannot emit the reference-definition title used by the positive assertion, so the added test has a high-confidence failing path. No target code was executed during this read-only review. Is this the best way to solve the issue? No: the intended parser setup is reasonable, but the positive fixture does not test rendered text and the standalone dependency needs an explicit decision about landing it separately from its consumer. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against d3cb84d34b7e. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (51 earlier review cycles; latest 8 shown)
|
Summary
0.31.2with an npm lockfileWhy
Clownfish PR #311 needs authoritative CommonMark parsing in the external-merge
preflight. Repeated review found that a handwritten Markdown state machine was
reimplementing parser semantics and accumulating bypasses. This prerequisite PR
adds only the maintained parser runtime and its workflow installation boundary;
it does not change preflight authorization behavior.
Validation
node --test test/commonmark-runtime.test.mjspassesnpm run validatevalidates 6,707 jobsnpm testpasses all 557 testsnpm audit --omit=devreports zero vulnerabilities31f2feffac0d58ca1beded6ab883957acb7d5ae0Risk
The workflows gain an npm install step. The runtime version and transitive graph
are locked, lifecycle scripts are disabled, and audit/funding network calls are
disabled during CI installation. Exact-head hosted CI must still exercise the
clean-runner installation and cache ordering before merge.
Maintainer decision
Adopt
commonmark@0.31.2as the locked parser runtime for external-mergepreflight review text. The reference parser owns Markdown syntax; Clownfish owns
the authorization projection over its AST. The paired parser PR must load it
only in the review-text parsing path so apply-only execution remains independent.
Any incompatible parser or contract change requires a separately reviewed
dependency update.
Gate disposition
installation before all 557 tests