Skip to content

Extend tag field mapping to comma-strings and object arrays - #234

Open
grimicorn-agent wants to merge 4 commits into
mainfrom
agent/tag-fieldmap
Open

Extend tag field mapping to comma-strings and object arrays#234
grimicorn-agent wants to merge 4 commits into
mainfrom
agent/tag-fieldmap

Conversation

@grimicorn-agent

Copy link
Copy Markdown
Collaborator

What & why

server/utils/fieldMapper.ts pickTagsField only accepted an array of plain strings, so the two most common real webhook tag shapes silently produced no tags: a comma-delimited string, and an array of objects like GitHub's labels[].name. This made tags mapping unusable for those providers (#228).

pickTagsField now accepts, via a shared coerceTagsValue helper:

  • Array of strings — unchanged (each item trimmed, blanks dropped).
  • Comma-separated string — split on ,, trimmed, empties dropped.
  • Array of objects — extracts the first present, non-blank own string property from name, title, label, value (GitHub labels → name).

Anything else (number, boolean, plain object, etc.) still yields undefined.

Decisions

  • One coercion path for both entry points. buildRawWebhookPayload (the raw, unmapped passthrough) now routes through the same coerceTagsValue, so an identical payload produces identical tags whether or not a fieldMapping is configured. Previously the two paths diverged (raw path only filtered non-strings).
  • Blank/whitespace tags are dropped everywhere via a single toNonEmptyTag helper (rule of three: comma-split, object-key, and bare-string paths all share it).
  • Object key lookups use the module's own-property guard (hasOwnProperty), matching the existing getNestedValue prototype-safety posture, so an inherited name/title on Object.prototype can't fabricate a tag.
  • Commas are NOT split inside array items — an array already delimits tags, and a tag may legitimately contain a comma (["a,b"] stays one tag). Pinned by a test.
  • No dedupe / no max-tag cap — deliberately left out (see follow-up suggestions); both change tag semantics for existing inputs and are product decisions beyond this issue.

Tests

31 tests in tests/server/utils/fieldMapper.test.ts covering: array of strings, comma string (top-level and nested path), array of objects (name and fallback keys, priority order, label key), empty array, blank/whitespace items, non-string priority key fall-through, garbage input, and the raw-path (buildRawWebhookPayload) coercion.

Viewable

Webhook ingest path — server/api/hooks/[slug].post.tsapplyFieldMapping. No UI surface; behavior is verified by the unit tests above.

Closes #228

Follow-up suggestions

  • Cap tag count and per-tag length — A string field mapped (or misconfigured) to tags expands an arbitrarily large comma body into unbounded tags that flow straight into records.tags jsonb and one YAML frontmatter line; add named MAX_TAGS/MAX_TAG_LENGTH limits in coerceTagsValue (suggested: P3, effort: S, evidence: server/utils/fieldMapper.ts coerceTagsValue)
  • Dedupe extracted tags — Comma strings and object arrays can now easily yield duplicate tags ("bug, bug", [{name:"bug"},{name:"bug"}]) that persist and render as tags: [bug, bug]; consider order-preserving de-duplication in coerceTagsValue (suggested: P4, effort: S, evidence: server/utils/fieldMapper.ts coerceTagsValue)

@grimicorn-agent

Copy link
Copy Markdown
Collaborator Author

Independent code review trail

Ran the independent reviewer (Opus) over git diff origin/main...HEAD, 3 rounds.

Round 1 — flagged:

  • Bare string array items skipped the trim/drop-blank rule the comma-split and object paths applied — fixed: extracted a shared toNonEmptyTag and routed all three paths through it (rule of three).
  • Missing test for untrimmed/blank bare strings in an array — fixed: added.
  • Comma-split-inside-array behavior undefined — fixed: decided arrays are not re-split (a tag may contain a comma) and pinned it with a test.
  • extractTagFromObject used a raw item[key] lookup, unlike the module's own-property guard — fixed: added hasOwnProperty guard via readOwnStringProperty.
  • Tag count/length cap — deferred (see below).

Round 2 — flagged:

  • buildRawWebhookPayload still diverged from the mapped path (same payload, different tags) — fixed: extracted coerceTagsValue and wired both entry points to it.
  • .map((tag) => toNonEmptyTag(tag)) vs .map(coerceTagItem) inconsistency — fixed: .map(toNonEmptyTag).
  • Missing edge tests (nested comma-string path, empty array, non-string priority key fall-through) — fixed: added.
  • Dedupe and MAX_TAGS cap — deferred (see below).

Round 3 — flagged:

  • Raw-path widening lacked coverage — fixed: added buildRawWebhookPayload tests for comma string, object array, and non-array/non-string → undefined.
  • label key and both-keys-present priority untested — fixed: added.
  • Cap and dedupe re-raised — deferred (see below).
  • Nested if in for inside extractTagFromObjectskipped: reviewer marked it non-blocking; the shape matches the file's existing isFieldMappingConfig convention.

Deferred / not actioned (with reason):

  • Max tag count / per-tag length cap and de-duplication: intentionally out of scope. Both change tag semantics for existing array-of-strings inputs too (a cap silently drops tags; dedupe collapses intentional duplicates) and are product decisions the issue doesn't call for. Recorded as follow-up suggestions in the PR body for the improvement digest.

Final: npm run lint:ci clean, 31/31 unit tests pass.

@grimicorn-agent grimicorn-agent added the has-suggestions PR carries follow-up suggestions for the improvement digest label Aug 27, 2026
@grimicorn-agent

Copy link
Copy Markdown
Collaborator Author

Agent code review trail

Merged origin/main into this branch — no conflicts (clean fast-forward-style automatic merge; net branch diff vs main is the original two files). Ran the independent Opus review loop on the branch diff (3 rounds).

Round 1 — flagged:

  • JSON-encoded array string (tags: "[\"a\",\"b\"]") was shredded by comma-splitting into garbage tags. Fixed — added tryParseJson to parse a JSON value before falling back to comma splitting.
  • Missing tests for mapped-path-absent and forbidden-key tag paths. Fixed — added both.
  • No tag count/length cap or dedupe (reviewer noted this is pre-existing, not introduced). Skipped — out of scope for a merge task; it changes existing output semantics. Recorded as a follow-up suggestion.

Round 2 — flagged:

  • JSON array of numbers ("[1,2,3]") yields []. Kept as-is + documented with a test — this is consistent with the existing "filter non-string tags" rule already covered by the array-path test; the reviewer's fall-through alternative would reintroduce garbage tags.
  • Malformed bracketed string ("[infra, urgent") keeps a [infra artifact. Kept — honest degradation to comma-split; stripping brackets guesses at intent on an unlikely input.
  • Magic "[" literal. Fixed — extracted to a named prefix constant (later generalized to JSON_VALUE_PREFIXES).
  • Raw-path coverage gaps. Fixed — added buildRawWebhookPayload tests for a JSON-array string and a null tags value.

Round 3 — flagged:

  • JSON-object string (tags: '{"name":"bug"}') fell through to comma splitting and produced a garbage tag, while the array path already handles objects via coerceTagItem. Fixed — generalized parsing to accept [/{ and route the result through coerceParsedJsonTags, so a stringified object resolves to a single tag. Added tests for the object-string and mixed-array cases.

Unresolved after 3 rounds (follow-ups, not blockers):

  • No tag count/length cap or dedupe on the tag pipeline (pre-existing amplification; recorded as a follow-up suggestion on this PR).
  • quoteYamlScalar in server/utils/markdown.ts does not quote/escape \r and other control chars — pre-existing and reachable independently of this change; out of scope here.

Verification: npm run lint:ci clean, full suite green (1866 tests).

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

Labels

has-suggestions PR carries follow-up suggestions for the improvement digest

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Field mapping can't extract tags that arrive as a comma-string or array of objects

2 participants