Skip to content

feat(dashboard): configurable text link patterns in transcripts - #8302

Open
jingchaodev wants to merge 1 commit into
kirodotdev:mainfrom
jingchaodev:feat/link-patterns
Open

feat(dashboard): configurable text link patterns in transcripts#8302
jingchaodev wants to merge 1 commit into
kirodotdev:mainfrom
jingchaodev:feat/link-patterns

Conversation

@jingchaodev

@jingchaodev jingchaodev commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Problem / Motivation

Chat transcripts are full of work-item identifiers -- ticket ids, issue keys, change-request numbers -- that render as plain text or as copy-only inline-code chips. The renderer has no way to know PROJ-123 maps to a tracker URL, so none of them are clickable: the user copies the id, opens a browser tab, and pastes it into their tracker by hand, for every id, in every message.

Why it matters

For anyone whose agent works with an issue tracker (Jira, YouTrack, Bugzilla, an internal system), every mention is a navigation dead end. The fix cannot be hardcoded upstream because the id-to-URL mapping is operator-specific -- which is exactly what made dashboard.jira_hosts a config key.

What changed (motivation → approach → change)

Goal: let operators declare their own text-to-link mappings once, and have every transcript -- including already-stored ones -- render them as links.

Approach: a new dashboard.link_patterns config key (list of {pattern, url} rules: JavaScript regex + http(s) URL template in which {match} receives the matched text, percent-encoded), applied at render time only. Rules register into the EXISTING autolink rule engine (utils/autolinkRules.ts + the remarkAutolinkRules masking plugin) that editions already register vocabulary on — no parallel transform. This follows dashboard.jira_hosts for operator link config flowing config → GET → React context. Rendering-only means stored messages never change and old transcripts linkify retroactively.

What was built:

  • Backend: LinkPatternRule dataclass + _coerce_link_patterns (skip-invalid, capped at 50 rules, http(s)-only templates) in config/sections.py; loader wiring; PUT validation (400 with code: invalid_link_patterns) and GET exposure in the dashboard config handler; config-baseline.json regenerated.
  • Frontend registry (utils/autolinkRules.ts): setConfigAutolinkRules compiles operator rules into the shared autolink registry with registration-time safety gates — structural ReDoS refusal (hasCatastrophicShape, polynomial shapes, a pump budget capping quantifier products at 16, a 100-atom cap on width-fixed brace demand) plus a bounded execution-time ladder — and link_pattern_url_ok-equivalent template checks (two-canary origin stability, no userinfo, {match} outside the authority) mirrored server-side. Config rules scan at most 2000 chars per text node; rendering caps at 200 links per node. Rules that fail a gate are skipped, and the editor shows an inline warning for the refused class (at most one wide quantifier). The inline-code link chip is scoped to operator-config rules: edition-registered vocabulary keeps its shipped copy-only chip.
  • Renderer: the remarkAutolinkRules plugin masks fenced + inline code, existing links, autolinks and raw HTML, then rewrites prose matches to markdown links; InlineCode renders a whole-match span as a link chip (anchor wrapping the code element, target disclosed in the native title) instead of the copy-only chip.
  • Settings → Chat: a LinkPatternsEditor row editor (composite primitive, registered in the settings registry with configKey="dashboard.link_patterns"), committing on blur/add/remove, stacking narrow-first (sm breakpoint). Inline flags: invalid regex, scan-unsafe pattern, duplicate pattern, and incomplete URL template — the four mistakes that would otherwise fail silently or only at the server. Local rows are guarded by settlement-aware watermarks so a rejected save's rollback never erases typed rows and a confirmed save still adopts later external changes. 12 i18n keys across all 12 catalogs + pseudolocale.
  • Test-helper fix: test_config_schema.py's annotation resolution now evals in the defining module's namespace (sections), since the loader facade's re-export list is frozen and a post-split dataclass type could never resolve there.

Tests

  • website/src/test/autolinkRules.test.ts: registration skips (invalid regex, bad scheme, missing {match}, placeholder-in-authority, userinfo, empty-match), the structural ReDoS rejection suite (canonical (a+)+$ shapes, ambiguous alternations, [^] class-lexing bypass variants, over-budget pump chains) with safe expressive shapes preserved, subject/hit caps, and {match} percent-encoded substitution.
  • website/src/test/MarkdownRenderer.linkPatterns.test.tsx: prose match renders an anchor at the resolved template; whole-match inline code becomes a link chip (target=_blank, rel=noopener noreferrer); partial-match code spans keep the copy chip; fenced code and existing links are untouched; per-node hit and subject caps hold; no rules = unchanged render.
  • website/src/test/LinkPatternsEditor.test.tsx: a rejected save's rollback preserves typed rows; a genuine external change is adopted; an external restore of the pre-save value after a CONFIRMED save is adopted (settlement watermark); rejection clears only the echo mark.
  • test/test_dashboard_files_coverage.py: PUT/GET round-trip; nine malformed-body rejects (non-list, non-dict entry, empty pattern, javascript: template, >50 rules, >300-char pattern, duplicate patterns, {match} in host, userinfo) all 400 with invalid_link_patterns and persist nothing.

Manual verification

website/scripts/capture-link-patterns.mjs (committed) drives the real built SPA with the rules served through the real /api/dashboard/config shape and asserts from the live DOM before shooting: exactly 5 tracker anchors across prose and both message roles, the inline-code chip is an anchor wrapping <code>, and the fenced code block contains zero injected anchors -- in dark and light. Local gates: full MarkdownRenderer vitest family (487), i18n suite incl. all 12 style gates (666), settings/ChatPanel suites, tsc -b, eslint on changed files, jscpd (0 clones), pytest for the config/dashboard surface, isort/flake8/black-gate/mypy (1282 files), scrub-lint, brand, harness-parity, vendor-manifest, docs-lint.

Screenshots / video

Prose ids link, the backticked id renders as a link chip, and the fenced code block stays plain (dark):

transcript dark

The Settings → Chat rule editor:

settings editor

Light theme

transcript light

Related Issues

N/A -- no open issue tracks this; closest neighbors are #5729 (bare-URL linkify, merged) and #6574 (chip config knob), neither of which covers operator-defined patterns.

Checklist

  • At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable)
  • No secrets, credentials, or internal references in the diff

Contribution License Agreement

@jingchaodev
jingchaodev requested a review from a team September 3, 2026 22:54
@jingchaodev
jingchaodev requested a review from a team as a code owner September 3, 2026 22:54
@jingchaodev
jingchaodev requested a review from Zedmor September 3, 2026 22:54
@github-actions github-actions Bot added fork Pull request from a fork (external contributor) readiness: checking Automated validation is still running labels Sep 3, 2026
@jingchaodev

Copy link
Copy Markdown
Contributor Author

Attribution for the two red checks on head 17b9cc1 — neither originates in this PR's diff:

Backend Tests (Windows) (1) — five failures, all in test/test_autonudge_stop_auth.py (test_applier_*). That file is byte-identical to upstream/main in this branch (git diff upstream/main...HEAD -- test/test_autonudge_stop_auth.py is empty; this PR touches no autonudge/monitor code). The assertion messages quote the new "monitor_update cannot apply legacy fields to a structured monitor" behavior introduced by e4f5e13 (#8201, this branch's base commit), and PR #8307 fails the identical five tests on the same shard — so this is a main-branch regression from #8201, not something this PR can fix.

Dependency Audit / Audit Production DependenciesERROR: production dependency audit failed closed: npm audit timed out after 120s for website/package-lock.json. Registry timeout (fail-closed by design); this diff changes zero dependency files.

Plan: once the #8201 regression is fixed on main, I'll re-roll this head over a byte-identical tree to re-run both lanes.

@jingchaodev

Copy link
Copy Markdown
Contributor Author

E2E (stub ACP backend, offline) — base-mismatch artifact, not a defect in this branch (same class as the #1364 round-8 finding).

The failing step is the i18n render vs-base gate. This run resolved I18N_BASE_REF: b2320e02 — but this branch's merge-base is e4f5e132a; main advanced 10+ commits past the branch point between PR creation and the run, so the base tree it rendered is not this branch's parent. The flagged surface (src/apps/file-explorer/TabStrip.tsx:68, en-XA) has zero churn in this diff, and the en-XA catalog diff against the branch point is exactly this PR's 9 new keys.

Verified by running the identical gate locally against the real branch point:

I18N_BASE_REF=e4f5e132aa88ee2507166d6b40e3267e42d3f443 npm run i18n:render
  [i18n-render] PASS [vs-base] — no surface got worse

Plan unchanged: when the test_autonudge_stop_auth.py regression from #8201 has a fix on main, I'll rebase onto that tip in one force-push — that simultaneously aligns this gate's base, re-rolls the timed-out npm audit, and picks up the Windows fix.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 4, 2026
@jingchaodev

Copy link
Copy Markdown
Contributor Author

Audit update for head 17e9cde: Dependency Audit / Audit Production Dependencies failed again with the same npm-audit timeout, and the same job is red on unrelated open PRs #8310 and #8316 today — a repo-wide registry/audit outage rather than anything in these diffs (this one still changes zero dependency files). Holding until the audit lane recovers; will re-roll only if it needs a fresh head after that.

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 4, 2026
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5, fork) — 🟡 CONCERNS

UX-level review of 856002ccd96475f8df6b195be8878565e2f71c8a via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

UX-Verdict: CONCERNS

The whole-match code chip silently trades its copy action for open-in-new-tab, and no artifact in this lane shows any of the new controls.

Watch

  • Copy is withdrawn from the inline-code chip: wholeMatchAutolinkHref(raw) replaces CopyableCode with an <a target="_blank"> wrapping a plain <code>, so a backticked PROJ-42 a user could previously click to copy now only opens a tab — and title={patternHref} shows the bare URL where the sibling chips in the same file state the action ("Click to copy", and SessionChip explicitly keeps "Ctrl/Cmd+click to copy"). Frequency high (every whole-match id, every transcript) × friction impact × every time. Smallest fix: make only the ExternalLink glyph the anchor and leave the <code> as CopyableCode, or add the copy path to a localized title.
  • The one editor-typable error the code already detects locally is the only one it sends anyway: duplicatePattern renders its inline flag, but commit's withhold predicate checks only halfEdited, so the PUT goes out, 400s, and surfaces as the generic failed_to_save_dashboard_config notice — while invalid-regex, unsafe-pattern and incomplete rows are held back with an explanation. Low frequency, moderate confusion, persists until the row is fixed. Fix: add duplicatePattern to the halfEdited guard.

Evidence gaps

  • Settings → Chat "Text Link Patterns" row (pattern input, URL-template input, "Add pattern", icon-only Trash remove): temp-screenshots/link-patterns/settings-editor.png is added by the fork and unreadable here; no blind reader has seen it.
  • The four inline hint states (invalid regex, unsafe pattern, duplicate, incomplete row) and the zero-rule empty state appear in no screenshot at all.
  • Transcript link chip vs copy chip at rest — whether the ExternalLink glyph alone reads as "opens a tab" — needs transcript-dark.png/transcript-light.png pushed to this repo for a blind read.

Suggestions

  • Cut mechanism from the always-visible link_patterns_desc: drop "percent-encoded" and "Rendering only:" and keep "Turn matching text such as ticket ids into links. Code blocks and existing links are left alone."

[UX-REVIEWED] 856002c

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5, fork) — ✅ PASS

Premise-level review of 856002ccd96475f8df6b195be8878565e2f71c8a via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All claims verified against the base: the autolink rule engine (website/src/utils/autolinkRules.ts + remarkAutolinkRules) exists and this change registers into it rather than building a parallel transform; jira_hosts serves a different job (known-host Jira chips, not generic pattern→URL rewrite); temp-screenshots/ is a tracked-deliverables convention (~280 sibling dirs, gitignore explicitly exempts it); the loader's re-export list is documented FROZEN at loader.py:58, which makes the test-helper namespace change necessary; TagListEditor is the existing composite-primitive precedent in settingsExtract.ts.

First-Principles-Verdict: PASS

Operator regex-to-link rules land on the existing autolink engine; every guard names the boundary it protects, and nothing duplicates an existing mechanism.

What this change ships

Intent: let operators turn work-item ids in chat transcripts into clickable tracker links via their own regex-to-URL rules — an ADDITION.

  1. New dashboard.link_patterns config key, max 50 rules — justified
  2. Prose ticket ids in transcripts (stored ones included) render as links — justified, reuses utils/autolinkRules.ts
  3. Inline-code span that IS a work item becomes a link chip with a glyph — declared, justified
  4. Settings → Chat gains a rule row editor flagging silent-failure mistakes inline — justified
  5. Pathological regexes refused at registration; scan cost capped per node and per message — justified, transcript is untrusted input
  6. PUT rejects malformed rule lists with code: invalid_link_patterns; hand-edited config drops bad entries — justified, matches host-allowlist discipline
  7. Editor strings in all 12 locales plus pseudolocale — mandated by the i18n invariant
  8. Screenshot harness script plus three committed PNGs — conventional deliverable (temp-screenshots/ is gitignore-exempted)
  9. Config-schema test helper resolves annotations in sections.py — rides along, required by the frozen re-export list (loader.py:58)
  10. SettingsField becomes importable outside components/settings — one consumer (LinkPatternsEditor), the declared composite contract

The Python link_pattern_url_ok deliberately mirrors the TS normaliseHref acceptance rule; a cross-language mirror cannot be collapsed to one symbol, the http(s) floor is boundary-derived, and the origin-stability half removes a named silent failure (rule saves, never linkifies), so it stands.

Subtractions

  • Shrink AutolinkRule.maxSubject?: number (autolinkRules.ts): one value is ever assigned — CONFIG_RULE_MAX_SUBJECT at the single setConfigAutolinkRules site (grepped maxSubject, 1 assigning site, 2 read sites both comparing against that constant). A boolean config-rule marker plus the existing constant is the singular form.

[FIRST-PRINCIPLES-REVIEWED] 856002c

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5, fork) — 🟡 CONCERNS

Design-level review of 856002ccd96475f8df6b195be8878565e2f71c8a via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

The doc isn't touched in the patch (no extension-seams.md hunk). I have what I need for the verdict. The design reuses the existing autolink seam correctly, validation layers are coherent, and the failure mode of the hard part (operator regexes meeting attacker-shaped transcript text) is bounded to cosmetic degradation. The notable risk is the bespoke ReDoS static analyzer itself.

Design-Verdict: CONCERNS

Sound feature on the right seam, but it ships a hand-rolled regex safety analyzer whose own history shows it's a bypass treadmill.

Watch

  • The ~400 lines of bespoke regex-source lexing (hasCatastrophicShape, pumpBudgetExceeded, fixed-width cap, backreference ban, probe ladder, per-document budget) exist because the design accepts full JS regex as operator input; the code's own comments record "round-20" and "round-21" bypasses found during development, and a single non-interruptible exec() can still blow past the 50ms budget on any shape not yet enumerated. Residual harm is bounded (main-thread jank on attacker-shaped transcript text, links degrade to plain text), but this analyzer is a permanent adversarial-maintenance surface someone must own. The user docs already describe a restricted grammar ("at most one wide quantifier"); declaring that grammar as the accepted input language would shrink the analyzer to a validator and end the treadmill.
  • website/docs/extension-seams.md documents the autolink registry contract this PR changes (second rule source, maxSubject, silent-drop registration, scan metering) and is not updated in the diff; the repo's same-commit doc rule applies.

Suggestions

  • Rules are registered only when ChatPage renders, so any other surface using MarkdownRenderer linkifies inconsistently depending on whether chat mounted first; hoisting the config-to-registry write to the dashboard-config query layer would make the behavior surface-independent.

[DESIGN-REVIEWED] 856002c

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed 856002ccd96475f8df6b195be8878565e2f71c8a via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 856002c

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed 856002ccd96475f8df6b195be8878565e2f71c8a via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 856002c

@jingchaodev

Copy link
Copy Markdown
Contributor Author

Dispositions for the two blocking reviews, addressed on head b2ff8fd:

First Principles — core finding ACCEPTED and applied. #7270's autolink seam landed the day before this branch and my duplicate-check missed it; the parallel implementation is gone. website/src/lib/linkPatterns.ts, the prepare-chain call, LinkPatternsCtx, and their tests are deleted. dashboard.link_patterns now feeds setConfigAutolinkRules(...) — a swap-in config rule set on the existing registry that reuses the seam's own validation verbatim (normalisePattern + normaliseHref), so the https://$1.evil.example/ class the review named is refused here exactly as it is for registered rules (origin-stable canaries, {match} outside the authority, no userinfo). Template syntax is now the seam's {match} (capture groups dropped); prose rewriting is remarkAutolinkRules' tree walk, so the double-masking is gone too. Config rules sit AFTER edition rules in registration order, keeping edition vocabulary authoritative on overlaps. The two dead exports the review flagged went with the module.

First Principles — InlineCode subtraction DECLINED, with the named user. This feature's originating report is precisely a transcript full of backticked ticket ids that were copy-only (PR body, first paragraph). Agents backtick work-item ids constantly; inlineCode staying opaque to the remark plugin is correct for partial mentions, and the chip converts ONLY a whole-string match (PROJ-123, never npm PROJ-123 run), now via wholeMatchAutolinkHref on the same registry. Copy stays one selection away and the native title discloses the destination. Happy to split it out if maintainers prefer, but it is the half that answers the original complaint.

GPT — feature map: FIXED. The chat row in docs/feature-map/README.md now names handlers/files.py and GET,PUT /api/dashboard/config.

GPT — blur deletes a stored rule: FIXED. A half-edited row (cleared URL, kept pattern) now blocks the save on blur instead of persisting the filtered list; deletion only happens through the remove button.

GPT — zero-width regex freeze and tilde fences: MOOT by the rewire. Both findings target the deleted lib/linkPatterns.ts. The seam refuses empty-matching patterns at registration, remarkAutolinkRules abandons a rule on a zero-width match against real content, and its tree walk never enters code nodes regardless of fence style.

@jingchaodev

Copy link
Copy Markdown
Contributor Author

GPT round-2 dispositions, addressed on head b4eef3f:

Operator-regex ReDoS — FIXED. setConfigAutolinkRules now runs a registration-time execution probe before accepting a rule: a length ladder of tiny almost-matching subjects (8→24 chars over several alphabets), wall-clock-checked between every exec. Exponential backtracking is unmistakable at those sizes ((a+)+$ costs ~2^n steps) while the subjects stay small enough that a single exec() is always bounded — exec is not interruptible, so no check inside a call can save you; the ladder is what makes the budget enforceable. A refused rule is dropped; linear rules clear the probe in under a millisecond. Pinned by a test that registers (a+)+$ and asserts refusal within the budget.

$0 templates pass backend, dropped by frontend — FIXED. Backend and frontend now enforce the same contract: the config coercer, the PUT validator (400 invalid_link_patterns), the field metadata, the docs row, and the Settings editor all require an absolute http(s) template containing {match}.

Function-local import — FIXED. The three caps import at module level from kiro_crew.config.sections (the defining module; the loader facade's re-export list is frozen by test_config_module_boundaries).

@bolichen97

Copy link
Copy Markdown
Collaborator

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

  • PR #7308 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7308: CONTINUE_DEVELOPMENT. Adjacent edits in one handler with unrelated goals; a mechanical rebase for whichever lands second. Files: src/kiro_crew/dashboard/handlers/files.py.
  • This PR is PARTIALLY_COVERED with PR #7270. Coverage is explicitly incomplete; this finding is not a completion or closure claim. Recommended action for PR #8302: MERGE_DISCUSSION. Main already owns the transcript bare-token linkify engine from PR #7270; the genuinely new part of this PR is the operator config path, the settings editor and the code-chip. Whether the config rules should drive the existing registerAutolinkRules/remarkAutolinkRules seam rather than a second source-text pass is a maintainer design call, and the two engines' differing safety properties (math masking, placeholder-in-authority) need reconciling either way. Files: website/src/components/MarkdownRenderer.tsx, website/src/utils/autolinkRules.ts.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.

@jingchaodev

Copy link
Copy Markdown
Contributor Author

Status on head ee34794 — review-ready:

  • CI: fully green (all backend shards on 3.10/3.12/Windows, frontend suites, E2E, builds, every gate — and the Dependency Audit passed this round after the earlier repo-wide npm-audit timeouts).
  • Reviews: Design, UX, First Principles, and Opus 4.8 all green on this head with zero findings. First Principles' earlier BLOCK (duplicate of the feat(markdown): seam for autolinking bare organisational tokens #7270 autolink seam) was accepted and the feature rewired onto setConfigAutolinkRules; GPT 5.6's six findings across two rounds are each fixed or dispositioned above (ReDoS registration probe, {match} enforced end-to-end, feature-map row, blur data-loss, plus two mooted by the rewire).
  • One caveat: the GPT 5.6 lane errored on this final head ("review incomplete" — no verdict, no findings; see the job logs). Its two completed prior rounds are fully addressed. A maintainer re-run of that one lane is welcome if wanted; I've kept the head stable rather than force-pushing a re-roll that would restart all of CI.

Remaining gate is human review — over to the maintainers.

@jingchaodev

Copy link
Copy Markdown
Contributor Author

Round-13 fixes on 9a320a5f3 (GPT blocking finding + 3 CI reds from round-12's own changes, + rebase past #8565).

GPT security-class blocking (external updates erase an in-progress rule draft) — legitimate, FIXED. The adopt effect replaced local rows wholesale, so a rule being typed when another client changed the config was silently discarded. Now the adopt diffs the outgoing baseline (parsed from the adopted watermark, which every assignment stores as serialized rules): rows typed since the last sync are carried across the adopt and rendered beside the adopted rules — that visible coexistence is the conflict surface — while a clean row the external change deleted is in the baseline and still goes. Two tests (carries a typed-but-unsaved draft across an external adopt, does not resurrect a clean row the external change deleted), mutation-proven: reverting the merge to plain replacement fails the first.

CI reds — all three were mine from round 12, fixed: hand-written pseudolocale entries regenerated via gen-pseudolocale.mjs (4 values); ko 조사 style — {{placeholder}}가{{placeholder}}이(가), {{placeholder}}를{{placeholder}}을(를), and URL이어야URL 형식이어야 per style/ko.md §2/§2.1; settingsRegistry.gen.ts regenerated (npm run gen:settings) after the desc call-site change.

Rebased onto 5f5e3dec4; the files.py GET-dict conflict with #8565's social_share_enabled resolved additively (both entries). Gates: backend 97/97, tsc 0, vitest 162/162 (editor + rules + renderer + i18n + registry), i18n gates 0 FAIL, eslint, black gate.

@github-actions github-actions Bot removed the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Sep 5, 2026
@jingchaodev

Copy link
Copy Markdown
Contributor Author

Round-14 fix on f095766bb — GPT security-class blocking finding (backreferences bypass the ReDoS gates): legitimate, FIXED.

Live-probed before patching: (a+)\1b and (?<g>a+)\k<g>b passed every gate (configPatternUnsafe false; registration's shape + pump + wall-clock chain all green) yet each measures ~125ms against a 2000-char subject — capture-split backtracking has no quantifier nesting for the shape gates to see, and the budget probe's synthesized subjects miss it. The mutation run confirms the bypass: with the new gate disabled, both patterns REGISTER.

Fix: hasBackreference — class-aware lex (same discipline as the shape gates: no leading-] skip, escapes walked; \1 inside a class is never a backreference in ECMAScript) flagging numeric \1\9 and named \k<…> outside classes — wired FIRST into both the registration chain in setConfigAutolinkRules and configPatternUnsafe (the editor's inline warning). Nothing is lost by refusing: substitution is {match}-only, so a capture group can only ever serve grouping, never reuse. Both bypass shapes added to the rejection test (existing surviving-rule-count assertion covers them; mutation-proven). link_patterns_unsafe_pattern extended with the backreference clause across the 12 catalogs + regenerated pseudolocale.

Gates: tsc 0, vitest autolinkRules 51/51 + editor/renderer 13/13 + i18n 93/93, i18n-check 0 FAIL, eslint, added-strings gate OK.

@jingchaodev

Copy link
Copy Markdown
Contributor Author

Round-15 fix on 0877f568a — GPT security-class blocking finding (per-rule caps permit aggregate transcript-driven ReDoS): legitimate, FIXED.

Live-probed the exact scenario first: a+a?a?a?a?bN passes every per-rule gate (~16ms/scan, under the 20ms ladder), all 50 register, and 50 rules × ten 2KB paragraphs measured 7841ms of synchronous scanning — the finding's arithmetic holds.

Fix: a per-document aggregate wall-clock budget (CONFIG_SCAN_BUDGET_MS = 50) metering OPERATOR-CONFIG rule execution in hitsIn. The plugin resets the budget once per document; each config-rule scan is timed and drains it; once spent, remaining config rules stop matching for the rest of the document — linkification is cosmetic, so degrading to plain text is the entire failure mode. Edition rules (reviewed first-party vocabulary, distinguished by maxSubject unset) are not metered. The budget is overridable via plugin options (configScanBudgetMs) for deterministic tests; rendering uses the default.

Re-ran the attack scenario through the real pipeline with the fix live: 73ms (from 7841ms). Two new tests — zero budget stops config rules while edition rules still link; the budget resets per document — mutation-proven (bypassing the budget check fails the first).

Gates: tsc 0, vitest 66/66 across the editor/rules/renderer suites, eslint clean.

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 5, 2026
@jingchaodev

Copy link
Copy Markdown
Contributor Author

Round-16 on d42e1ce1e — GPT + Opus clean; the three advisory lanes' fresh CONCERNS addressed: 6 taken, 3 declined with reasons.

UX (all three Watch items fixed):

  • Blocked save with zero feedback — a half-filled row (pattern without URL, or URL without pattern) now flags inline with link_patterns_row_incomplete ("Incomplete row: fill both fields or remove it. No changes in this editor save while a row is half-filled"), and a test pins flag-shown + commit-withheld together.
  • Unsafe message jargon — rewritten outcome-first with a working example: "…too complex to run safely, so this rule is ignored. Keep one repeating part, e.g. \b[A-Z]{2,5}-\d+\b…". link_patterns_url_invalid also rewritten with a path example. All × 13 catalogs, pseudolocale regenerated.
  • Hand-rolled error rows — all five inline flags now render via ErrorNotice variant="inline"; raw <input>s replaced with the shared Input (dead inputCls dropped). Chip suggestion also taken: the link chip carries a lucide ExternalLink glyph so it is distinguishable from a copy chip at rest.

First Principles (both taken): wholeMatchAutolinkHref now skips edition rules (maxSubject === undefined), scoping the chip change to the config feature that motivated it — test flipped to pin edition-null/config-resolves, PR body updated. configScanBudgetMs plugin option deleted; replaced with setConfigScanBudgetForTest per the module's existing …ForTest seam convention (0 non-test consumers, as noted).

Design (declined, with reasons):

  • Server-side pattern gates — declined. Unlike link_pattern_url_ok (a total check, portable byte-for-byte), pattern safety is only half-structural: the wall-clock ladder and the per-document aggregate budget are execution-dependent and cannot run server-side, so "a rule that saves is a rule that linkifies" cannot be made true for patterns by any PUT validator. Porting ~250 lines of ECMAScript-regex-source lexing to Python buys permanent two-implementation drift risk (each future JS regex feature must land twice, identically) without delivering the invariant. The failure mode for a CLI-written unsafe pattern is visible in-product: the rule renders with the unsafe explanation on the next Settings → Chat visit.
  • Regex as the config contract — acknowledged as a one-way door, knowingly accepted: regex-to-URL is the established operator idiom for autolink rules (GitLab/Gitea ship the same shape), the analyzer is ~250 lines, structurally tested, mutation-proven, and scoped to registration; a token-template language would be a second bespoke syntax operators must learn, with its own parser to own forever.
  • Registration placement — declined as-is: there is no shared dashboardConfig resolution point to move it to (each surface queries independently), and ChatPage's render-phase ref-guarded write exists specifically so the first paint after a config change carries the rules (an effect paints stale once). Cross-surface parity is real but needs a provider refactor beyond this PR's scope; noted for follow-up.

Gates: frontend 165/165 (editor/rules/renderer/i18n/registry), backend 97/97, tsc 0, i18n gates 0 FAIL, eslint, black.

@jingchaodev

Copy link
Copy Markdown
Contributor Author

Round-17 on f17b60a95 — GPT security-class blocking finding (inline-code scans bypass the aggregate budget) legitimate, FIXED; plus the Frontend Tests (1) red from round 16.

GPT finding. Probed first: ten 2000-char code spans × the same 50 gate-passing rules measured 7764ms through wholeMatchAutolinkHref — the round-15 budget metered only the remark prose pass, and chips scan per inline-code node outside it. Fix: the per-document budget pool moved into autolinkRules.ts and BOTH consumers drain it — hitsIn (prose) and wholeMatchAutolinkHref (chips), the latter metered per RULE, not per call (a per-call drain still let one call run all 50 rules: measured 968ms; per-rule metering: 61ms). The remark plugin re-arms the pool once per document parse; an exhausted pool degrades the chip to its copy-only form. Chip-budget test added (exhausted ⇒ null, restored ⇒ resolves), mutation-proven — bypassing the check fails 2 tests. The setConfigScanBudgetForTest seam moved with the pool.

Frontend Tests (1)settingsCoverage accounting drift from round 16's own <input><Input> swap in the editor; waiver updated in kind (input ×2Input ×2, same composite-indexing reason).

Gates: tsc 0, vitest 173/173 (editor/rules/renderer/i18n/registry/settings-coverage), backend 97/97, eslint, black.

@jingchaodev

Copy link
Copy Markdown
Contributor Author

Round-18 disposition (head d208f1cbd) — GPT's blocking finding + non-blocking finding, both taken:

  • F1 (blocking, errors-use-error-notice): fixed. All six client-side validation hints in LinkPatternsEditor (invalid regex, unsafe pattern, duplicate, 2x incomplete row, invalid URL template) no longer render as ErrorNotice — a local FieldHint renders them as role="status" + text-warn, the AboutPanel status idiom. Nothing has failed when these show, so error dress (role="alert", danger styling) was the wrong register; the adjudicator's read of the AUTOSDE rule is correct. The save-failure notice at the panel level keeps ErrorNotice — that one is an error. Editor tests locate hints by message text, so the round-16 flag-shown + commit-withheld pin still holds unchanged.
  • F2 (non-blocking, netloc whitespace): fixed. Confirmed the divergence: Python's urlsplit keeps a space in the authority (hostname='exa mple.com') where the browser's URL parser refuses the URL outright, so https://exa mple.com/{match} saved-but-never-linkified — the exact class link_pattern_url_ok exists to block. The function now rejects any whitespace surviving in netloc. Tab/newline are non-divergent (both parsers strip them, WHATWG-style, so those templates save and linkify identically) and need no special case. New rejection case in the malformed-PUT test.

Windows shard (3) red on f17b60a95 was external: both failing files (test_push_branch_gate.py, test_security.py) are byte-identical to upstream/main and absent from this diff. Re-rolled by this push; will cross-check another PR if it recurs.

Gates: backend 97/97, frontend 75+93+7, tsc, eslint, flake8/isort/mypy (1289 files), black gate, i18n string gates. Screenshots re-pinned.

@github-actions github-actions Bot added the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Sep 5, 2026
@jingchaodev

Copy link
Copy Markdown
Contributor Author

Rebased onto 834e1ea42 (was CONFLICTING: upstream's ErrorNotice-routing batch #8729 touched the same ChatPanel import block — resolved as the union; my side already carried upstream's Btn). This also picks up #8712, which fixes the TestUnrecognisedOptionsReadProtectively failures that reddened Backend (Windows) (3) and (3.12, 3) on the merge ref — that test class doesn't exist in this branch's base and passes 36/36 locally after the rebase. No feature changes; all local gates green (backend 97/97, frontend 172/172, tsc, black gate, both i18n gates).

@github-actions github-actions Bot removed the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Sep 5, 2026
@jingchaodev

Copy link
Copy Markdown
Contributor Author

Frontend Tests (1) red on c37667820 is upstream's, not this PR's. The four failing RemoteCrewPanel.test.tsx diagnosis-note tests fail identically on a pristine main checkout (834e1ea42) with zero PR code — reproduced locally before attributing. Both the test file and the component are byte-identical to upstream in this branch (this PR's only shared-module change is exporting SettingsField, a no-op for rendering). Root cause traced to #8729's ErrorNotice routing (role="alert" vs the test's [role="status"] selector); filed as #8825. A rebase past the upstream fix clears the lane once it lands — nothing to change here.

@jingchaodev

Copy link
Copy Markdown
Contributor Author

Round 20 (GPT) — fixed on 267661a15.

F1 (blocking, security-class): legitimate — fixed. The single-pump exemption let a+a{100}z through: {100} is width-fixed (no pump factor) and within the fixed-width cap, but beside a pump it multiplies the verify cost of every split that pump opens. Measured: 554ms for ONE synchronous exec on a 2000-char subject — spent before the per-document budget can drain (the drain is post-exec).

Fix follows the gate's established syntactic lineage (no overlap analysis to get wrong): fixed-width {n} (n ≥ 2) runs now pay into the same ≤16 pump product — only the single widest pump ever rides free. Zero-pump patterns stay exempt (width-deterministic: [a-f0-9]{40}:[a-f0-9]{40} SHA pairs survive), and every prior survivor still registers (\d{8,} ticket IDs, {40}/{100} fixed runs, [A-Z]{1,10}-\d+, ID\d{2,}x?). Refused now: a+a{100}z, \d{50}\d+x (order-independent), [a-z]*[a-z0-9]{20}.

Mutation-proven: disabling the fixed-width inclusion fails the new test (55/56), restored 56/56. The editor's inline warning ("keep one repeating part") already describes this refusal class — no message change. Note the adjudicator's own severity flag (operator-only, polynomial, self-recovering) is on record; fixing anyway was cheaper than a human override and strictly strengthens the gate.

Gates: autolink 56/56, frontend suites 118/118, backend 97/97, tsc 0, both i18n gates, screenshots re-pinned.

@jingchaodev

Copy link
Copy Markdown
Contributor Author

Rebased onto upstream b80fdc3e4 as 0c09bdccd (conflict: docs/feature-map chat/display rows — #8699 moved the plain-diffs toggle into the chat row; resolved as the union with this PR's files.py handler + endpoint additions). No code conflicts; all local gates green (backend 97/97, frontend 174/174 across 8 files, tsc 0, both i18n gates). Screenshots re-pinned.

@jingchaodev

Copy link
Copy Markdown
Contributor Author

Round 21 (GPT) — all three findings fixed on 92f239523.

F1 (blocking, security-class): legitimate — fixed. The budget's "one parse = one document" assumption broke against useBlockAssembler: one message mounts one remark tree PER fence-separated block, so the plugin's tree-entry rearm handed every block a fresh 50ms pool — fence-heavy message × 50 accepted rules = multi-second freeze. Per the suggested shape: the rearm now lives in the top-level MarkdownRenderer body (render-phase, parent-then-children order guarantees the pool is full before the first block's synchronous remark pass), and the plugin only drains. Both halves mutation-proven: re-adding the tree-entry rearm fails the new block-sharing test; removing the renderer rearm fails the per-message rearm test (a spent pool must not starve the NEXT message either).

F2 (editor URL check weaker than registration): fixed, one layer deeper than asked. The inline check now IS the registry's acceptance rule — new configUrlTemplateOk export wraps normaliseHref (two-canary origin check + safeHttpUrl userinfo rejection) — so userinfo and {match}-in-authority templates flag where they are typed. The test for it caught a second gap in the same class: the commit guard carried its own copy of the weak regex, so an invalid-URL row was silently FILTERED from the PUT — deleting the stored rule on blur (the round-6 vanish class). The guard now uses the same predicate: invalid rows are withheld exactly like half-filled ones. Editor test pins flag-shown + commit-withheld + clears-on-correction.

F3 (stale comment): fixed — the whole-match chip comment now says operator-configured only.

Gates: autolink 57/57 (incl. new pins), editor + renderer suites green, frontend 177/177 across 8 files, backend 97/97, tsc 0, both i18n gates, screenshots re-pinned. The link_patterns_url_invalid message already states the accepted shape, so no catalog churn.

@jingchaodev

Copy link
Copy Markdown
Contributor Author

Round 22 (GPT) — fixed on 2c9ecd385.

F1 (blocking, security-class): legitimate — fixed exactly as suggested. Both server sites (_coerce_link_patterns in sections.py, the PUT validator in files.py) and all three editor sites (persistable, the duplicate flag, the adopt-time row comparison) stripped/trimmed pattern text, silently broadening whitespace-significant regexes. Pattern text is now preserved byte-exactly end to end; strip()/trim() decide only blankness. URLs keep their edge-trim (templates are expanded, never matched, and the validator refuses authority whitespace — trimming cannot change meaning).

Consequence handled: patterns differing only in edge whitespace are DIFFERENT regexes, so the dedup (server 400 + client flag) now compares exact text — the twins PROJ-\d+ / PROJ-\d+ both save and both round-trip byte-exactly (new backend test PUT → disk → coercer → GET, mutation-proven: re-adding the strip fails it via the dedup collision). New editor test pins the exact pattern on the wire (AA-\d+ saves with its trailing space).

Gates: backend 98/98 (+ flake8, mypy, black), frontend 178/178 across 8 files, tsc 0, both i18n gates, screenshots re-pinned.

For the record, the adjudicator's LOW-harm flag (rendering-only, self-correcting, visible) is noted — fixed anyway, same reasoning as rounds 20/21: a code fix is cheaper than a human fence override and the preservation semantics are simply correct.

@jingchaodev

Copy link
Copy Markdown
Contributor Author

Fast Gate red on 2c9ecd385 was self-inflicted and is fixed on c45a5240c: a whole-file black run during round 22 reformatted two baselined files (files.py +550 lines), which moved upstream's pre-existing ".aim" strings onto ADDED diff lines — the diff-scoped scrub lint correctly attributed them to this PR. Both files are restored to upstream formatting with only the round-22 semantic hunks re-applied (diff back to +141/−1 from +723/−318); scrub lint passes locally in CI mode, backend 98/98, zero behavior change vs 2c9ecd385. Await Fast Gate and Coverage Gate were downstream of the same run. Note: test/test_ai_review_workflows.py (added by #8718) is not black-formatted and not in .github/black-baseline.txt — byte-identical to upstream here, so if the black gate flags it, that is upstream-owned.

Operator-defined regex -> URL template rules (dashboard.link_patterns)
rewrite matching plain text into links at render time: prose matches
become markdown anchors, and an inline-code span whose whole text
matches renders as a link chip instead of the copy-only chip. Masking
keeps code blocks, existing links, autolinks and bare URLs untouched;
templates are http(s)-only and matched text is URL-encoded. Rules are
edited in Settings -> Chat and served through the dashboard config API,
so old transcripts linkify retroactively at display time.
@jingchaodev

Copy link
Copy Markdown
Contributor Author

All five lanes went green on c45a5240c — GPT's first ✅ since the security series began. The two backend shard reds there were upstream's: #8608's corpus gate (test_members_dm_thread.py) requires direct SEL enqueues in members.py, and the merge ref ran the new test against this branch's 55-commit-old base, which still carried the to_thread-wrapped audits (same class as the earlier #7808 episode — the test rides the merge ref, the fix doesn't). Rebased onto current main as 856002ccd; the gate passes post-rebase (102 backend tests locally). Coverage Gate was downstream.

Also took GPT's one remaining non-blocking finding in the same push: the PUT's scheme check was case-sensitive while the editor validates with the browser's case-insensitive URL parser, so HTTPS://…/{match} passed inline and died at save. link_pattern_url_ok now compares the scheme case-insensitively (urlsplit already lowercases it for the origin comparison); new round-trip test stores the mixed-case template byte-exactly. All gates green (backend 102, frontend 178/178, tsc 0, both i18n gates, scrub clean); screenshots re-pinned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fork Pull request from a fork (external contributor) merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: action required A blocking check or review needs attention

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants