Skip to content

HT-7: mail-behavior acceptance fixtures - #3

Merged
zaridan merged 7 commits into
mainfrom
feat/ht-7-blackbox-fixture-harness
Jul 10, 2026
Merged

HT-7: mail-behavior acceptance fixtures#3
zaridan merged 7 commits into
mainfrom
feat/ht-7-blackbox-fixture-harness

Conversation

@zaridan

@zaridan zaridan commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

The mail engine's acceptance criteria — the threading/conversation behaviors Helpthread must exhibit, captured as sanitized fixtures and reframed as our own acceptance suite (see fixtures/mail/README.md), cited by the threading spec (HT-8).

The FreeScout-observation harness that generated these was relocated to the private resonant-help repo (tools/mail-behavior-harness/, PR ResonantIQ/resonant-help#1) — it observes FreeScout specifically and serves no Helpthread deployer, so it doesn't ship in the distributed product. Helpthread's own e2e mail harness (pointing at a Helpthread instance) is future work.

Provenance: black-box observation only (facts about behavior via API), token values + PII redacted to deterministic placeholders. No FreeScout source read/copied/derived.

Jira: https://resonantiq.atlassian.net/browse/HT-7

Summary by CodeRabbit

  • Documentation

    • Added guidance for mail behavior acceptance scenarios, including conversation creation, reply threading, forged-token rejection, subject-only replies, auto-submitted messages, and HTML handling.
  • Tests

    • Added observed mail scenarios covering new conversations, valid and forged reply tokens, subject-based replies, differing customers, auto-replies, and HTML message content.
    • Documented expected conversation and thread outcomes for each scenario.

zaridan and others added 4 commits July 9, 2026 21:03
Sends crafted emails at a live helpdesk mailbox and records the
resulting conversations (via REST, embed=threads) as sanitized JSON
fixtures — the future acceptance suite for Helpthread's mail engine.

Five scenarios: new-conversation, reply-with-reference (captures the
outbound agent reply's Message-ID over IMAP and replies against it),
reply-subject-only, auto-submitted, same-subject-different-customer.

Safety: every observation and the single mutation path (agent reply)
are gated on a per-run subject marker — pre-existing conversations are
untouchable. Deps (first in repo): nodemailer (MIT-0), imapflow (MIT),
licenses verified at adoption per charter. Dry-run validated; live run
+ recorded fixtures follow in this PR.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TqG66PPZreBrj17VbAqe3b
Live run 23ad1ec4 against the production helpdesk recorded:
- new-conversation: fresh email -> conversation with one customer thread
- reply-subject-only: 'Re:' subject with no reference headers -> a SECOND
  conversation (no subject-based threading, confirmed by observation)
- auto-submitted: Auto-Submitted mail -> conversation still created
- same-subject-different-customer: identical subject, different sender
  -> separate conversation (no cross-customer merge)

reply-with-reference timed out: the API-created agent reply thread
exists in the helpdesk but no outbound email ever left it (confirmed
absent from the customer mailbox 25+ min later) — investigation open;
fixture intentionally not committed until the send path works.

Harness fix: the error path wrote err.message unredacted (real
addresses in timeout text); failures now pass through redact() like
successes. The leaked local fixture was deleted before ever being
committed.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TqG66PPZreBrj17VbAqe3b
The scenario's fixture records the crown-jewel behavior end to end:
outbound helpdesk reply carries Message-ID
<FS_reply-{threadId}-{token}@{domain}>, and a customer reply whose
In-Reply-To/References target it threads into the same conversation
(threads 3 -> 4, observed live).

Recorded deviations, kept as observations:
- REST-API-created 'message' threads exist in the DB but dispatch no
  email; UI-sent replies do. (Production-relevant helpdesk finding.)
- Gmail SMTP rewrites plus-addressed From to the canonical account
  address, so run 23ad1ec4 had ONE customer identity; scenario 5's
  fixture relabeled honestly (same-customer variant, correction note),
  and multi-customer runs need distinct sending identities.

Harness fix: waitForMessage matches on the unique subject marker only —
a plus-tagged To filter can never match what Gmail actually delivers.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TqG66PPZreBrj17VbAqe3b
Three more black-box fixtures completing the threading picture:
- forged-reply-token: a tampered FS_reply token does NOT thread into
  the real conversation — it creates a new one. FreeScout validates the
  token, it doesn't just pattern-match the format.
- token-authority: a VALID token with an unrelated subject threads into
  the token's conversation anyway. The signed token is the sole
  threading authority; subject is irrelevant.
- html-body: an inbound <script> tag is stored VERBATIM in the thread
  body and returned raw by the API. Sanitization is entirely the
  reader's responsibility — a stored-XSS surface our engine must own.

Redaction hardened (real leaks found in the first pass, now fixed and
prevented going forward): domain scrub (reply-token Message-ID domain,
avatar URL host, bare domain), structural person-identity redaction
(name/avatar on any person-shaped object), and explicit display-name
scrubbing for names embedded in audit-log thread bodies. All eight
committed fixtures verified free of real names, emails, domains, and
avatar hashes. send.mjs gained html support for the sanitization probe.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TqG66PPZreBrj17VbAqe3b
@zaridan

zaridan commented Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 10, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai

coderabbitai Bot commented Jul 10, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Adds observed mail fixtures documenting conversation creation, reply threading, token authority and forgery handling, automated messages, subject-only behavior, same-subject messages, and HTML body capture.

Changes

Observed mail behavior fixtures

Layer / File(s) Summary
Conversation creation and threading observations
fixtures/mail/observed/new-conversation.json, fixtures/mail/observed/reply-with-reference.json, fixtures/mail/observed/reply-subject-only.json, fixtures/mail/observed/same-subject-different-customer.json, fixtures/mail/observed/token-authority.json, fixtures/mail/observed/forged-reply-token.json
Records expected conversation outcomes for fresh messages, referenced replies, subject-only replies, same-subject messages, valid tokens, and forged tokens.
Message ingestion observations and fixture guidance
fixtures/mail/observed/auto-submitted.json, fixtures/mail/observed/html-body.json, fixtures/mail/README.md
Captures auto-submitted and HTML-body observations and documents fixture scope, provenance, and redaction rules.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is concise and accurately summarizes the main change: retaining mail-behavior acceptance fixtures.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/ht-7-blackbox-fixture-harness

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 13

🧹 Nitpick comments (5)
fixtures/mail/observed/html-body.json (1)

75-75: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Do not turn raw script persistence into a passing security contract.

If acceptance tests consume this fixture as expected output, the preserved <script> can normalize stored-XSS behavior. Keep it as an observation, but add a structured assertion that rendering escapes/removes scripts, or explicitly mark this fixture as a known security regression.

Also applies to: 112-112

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@fixtures/mail/observed/html-body.json` at line 75, Keep the raw script tag in
the observed fixture as input evidence, but update the acceptance tests
consuming this fixture to assert that rendered output escapes or removes script
content. Alternatively, explicitly flag the fixture through the relevant test
metadata as a known security regression; use the fixture’s HTML-body observation
and its associated rendering/assertion helpers to implement this.
fixtures/mail/observed/reply-with-reference.json (1)

269-273: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Represent outbound-delivery observations as structured data.

The PR objective includes that API-created replies did not dispatch email, but this fact is currently only prose. Add a machine-readable field such as apiReplyEmailDelivered: false with the observation window, while retaining the note for context.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@fixtures/mail/observed/reply-with-reference.json` around lines 269 - 273, Add
a structured observation field alongside the prose entries in the notes fixture,
such as apiReplyEmailDelivered: false and an observation window indicating the
25+ minute delay; retain the existing explanatory note for context.
fixtures/harness/send.mjs (1)

14-27: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Add SMTP connection/socket timeouts.

No timeout options on the transporter — a stalled TLS handshake or slow server can hang indefinitely, well past the 240s scenario deadlines the harness otherwise enforces.

♻️ Proposed fix
   transporter = nodemailer.createTransport({
     host: 'smtp.gmail.com',
     port: 465,
     secure: true,
+    connectionTimeout: 15000,
+    greetingTimeout: 15000,
+    socketTimeout: 30000,
     auth: {
       user: env.smtpUser,
       pass: env.smtpPass,
     },
   });
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@fixtures/harness/send.mjs` around lines 14 - 27, Configure connection and
socket timeouts in the nodemailer transporter created by getTransporter, using
bounded values shorter than the harness’s 240-second scenario deadline so
stalled TLS handshakes or SMTP activity fail promptly.
fixtures/harness/api.mjs (1)

18-31: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Add a timeout to apiFetch.

No timeout on fetch() — a stalled connection to FreeScout would hang indefinitely, bypassing the deadline checks in every poll loop that calls this.

♻️ Proposed fix
 async function apiFetch(path, init = {}) {
   const env = loadEnv();
   const url = `${env.fsBaseUrl}${path}`;
-  const res = await fetch(url, {
-    ...init,
-    headers: { ...authHeaders(), ...(init.headers ?? {}) },
-  });
+  const res = await fetch(url, {
+    ...init,
+    headers: { ...authHeaders(), ...(init.headers ?? {}) },
+    signal: AbortSignal.timeout(15000),
+  });
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@fixtures/harness/api.mjs` around lines 18 - 31, Add an AbortController-based
timeout inside apiFetch, using a bounded duration from configuration or a
sensible default, and pass its signal to fetch alongside the existing request
options. Ensure the timeout is cleared after completion and timeout failures
surface as descriptive errors while preserving existing response handling.
fixtures/harness/inbox.mjs (1)

100-117: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Dead toAddress parameter in searchMailbox.

toAddress is threaded through both call sites but never used inside searchMailbox — only subjectContains filters the search. The comment above explains why recipient filtering was dropped, but the plumbing was left behind, which is confusing for future readers.

Also applies to: 141-141, 148-148, 152-152

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@fixtures/harness/inbox.mjs` around lines 100 - 117, Remove the unused
toAddress parameter from searchMailbox and update every call site, including the
references around the noted lines, to pass only the client, path, and
subjectContains arguments. Preserve the existing subject-only search behavior
and adjust any related comments or documentation that mention the removed
parameter.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@fixtures/harness/env.mjs`:
- Around line 64-71: Validate HARNESS_FS_USER_ID in the config construction
before assigning it to fsUserId; reject non-numeric or otherwise invalid values
with a clear configuration error instead of allowing NaN. Update the fsUserId
handling near OPTIONAL_DEFAULTS.HARNESS_FS_USER_ID while preserving the numeric
default behavior.

In `@fixtures/harness/inbox.mjs`:
- Around line 145-161: Move the openClient() call inside the retry loop’s
try/catch structure, using a client variable initialized before the block so
logout only runs when a connection was successfully established. Catch transient
connection or search errors, allow the loop to continue until the deadline, and
retain the existing finally-based logout cleanup for connected clients.

In `@fixtures/harness/README.md`:
- Around line 80-89: Update the “Safety rules” section in README.md to state
that isolation is enforced by the run-scoped [HT7-<runId>-...] marker, not
always by each scenarioId. Explicitly document that
same-subject-different-customer.json intentionally reuses the prior
new-conversation subject and may observe both same-run matching conversations,
while cross-run conversations remain excluded; revise the marker-gated mutation
wording consistently.

In `@fixtures/harness/redact.mjs`:
- Around line 125-137: The walk function’s person-object handling still passes
email values through generic string redaction, allowing unrelated domains to
remain unchanged. Update the isPerson branch to replace every non-null email
using the stable role-based IDENTITY_FIELD_REPLACERS placeholder logic, while
preserving null values and the existing object structure.
- Around line 82-97: The buildDomainRules function only redacts the domain of
FS_reply-* Message-IDs, leaving reusable capability tokens exposed. Add a rule
that detects and replaces the complete FS_reply-* token with a deterministic
fake while preserving the expected format, applying it before the generic domain
rule; alternatively invalidate or rotate the token in the fixture.

In `@fixtures/harness/run.mjs`:
- Around line 175-215: Scenario failures are recorded but do not affect the
process exit status. In the scenario loop within main(), set process.exitCode =
1 whenever outcome is 'timeout-or-error', while continuing to write the failure
fixture and process subsequent scenarios.
- Around line 195-209: Use the sanitized error text from the redacted `payload`
when reporting failures in the `run` harness flow. Update the `console.error`
call in the timeout-or-error handling block to log `payload.error` rather than
the raw `err.message`, while preserving the existing scenario ID and duration
context.
- Around line 179-208: Update the harness redaction flow in the success and
failure paths to collect the known identity names before calling redact. Pass
those names through the identityNames option in both redaction calls, including
the calls around the recorded payload and timeout-or-error payload, so names in
notes and error text are sanitized.
- Around line 33-43: Update parseArgs to require a non-empty value after --only,
throwing a clear error when it is missing, and reject any unrecognized options
instead of silently ignoring them. Preserve valid --only and --dry-run handling,
and ensure callers handle argument-parse errors without falling back to running
all scenarios.

In `@fixtures/harness/scenarios.mjs`:
- Around line 96-104: Guard the reply construction in the scenario using
outboundAgentReply.messageId before assigning the In-Reply-To header. Omit
In-Reply-To or use a valid fallback when the value is null, while preserving the
existing References behavior and reply send flow.
- Around line 218-266: Make runSameSubjectDifferentCustomer explicitly require
the new-conversation scenario’s prior conversation before sending. When before
is empty or no priorConversationId is found, return a clear skipped/invalid
result (or fail the scenario) explaining that it must run after
new-conversation, and do not classify the new message as own-conversation.
Update the observed data and notes accordingly so --only execution is
unambiguous.

In `@fixtures/mail/observed/same-subject-different-customer.json`:
- Around line 3-4: The fixture incorrectly presents same-customer messages as
cross-customer coverage. Update the fixture data and expectations to use a
genuinely different customer/sending identity, or rename the scenario and
objective to explicitly describe same-customer identical-subject behavior;
ensure the related cases around the corresponding expectation entries are
consistent.

In `@fixtures/mail/observed/token-authority.json`:
- Around line 7-10: The token-authority fixture contains an invalid,
non-reproducible run ID. Regenerate fixtures/observations using the current
runner so all related subject and header markers use a valid eight-character
hexadecimal ID, or explicitly mark the artifact as hand-curated if it must
retain its current values.

---

Nitpick comments:
In `@fixtures/harness/api.mjs`:
- Around line 18-31: Add an AbortController-based timeout inside apiFetch, using
a bounded duration from configuration or a sensible default, and pass its signal
to fetch alongside the existing request options. Ensure the timeout is cleared
after completion and timeout failures surface as descriptive errors while
preserving existing response handling.

In `@fixtures/harness/inbox.mjs`:
- Around line 100-117: Remove the unused toAddress parameter from searchMailbox
and update every call site, including the references around the noted lines, to
pass only the client, path, and subjectContains arguments. Preserve the existing
subject-only search behavior and adjust any related comments or documentation
that mention the removed parameter.

In `@fixtures/harness/send.mjs`:
- Around line 14-27: Configure connection and socket timeouts in the nodemailer
transporter created by getTransporter, using bounded values shorter than the
harness’s 240-second scenario deadline so stalled TLS handshakes or SMTP
activity fail promptly.

In `@fixtures/mail/observed/html-body.json`:
- Line 75: Keep the raw script tag in the observed fixture as input evidence,
but update the acceptance tests consuming this fixture to assert that rendered
output escapes or removes script content. Alternatively, explicitly flag the
fixture through the relevant test metadata as a known security regression; use
the fixture’s HTML-body observation and its associated rendering/assertion
helpers to implement this.

In `@fixtures/mail/observed/reply-with-reference.json`:
- Around line 269-273: Add a structured observation field alongside the prose
entries in the notes fixture, such as apiReplyEmailDelivered: false and an
observation window indicating the 25+ minute delay; retain the existing
explanatory note for context.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: f8eb8678-f66d-496b-85ca-d69ee1a977fc

📥 Commits

Reviewing files that changed from the base of the PR and between 8cc62d1 and 1fe728e.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (17)
  • fixtures/harness/README.md
  • fixtures/harness/api.mjs
  • fixtures/harness/env.mjs
  • fixtures/harness/inbox.mjs
  • fixtures/harness/redact.mjs
  • fixtures/harness/run.mjs
  • fixtures/harness/scenarios.mjs
  • fixtures/harness/send.mjs
  • fixtures/mail/observed/auto-submitted.json
  • fixtures/mail/observed/forged-reply-token.json
  • fixtures/mail/observed/html-body.json
  • fixtures/mail/observed/new-conversation.json
  • fixtures/mail/observed/reply-subject-only.json
  • fixtures/mail/observed/reply-with-reference.json
  • fixtures/mail/observed/same-subject-different-customer.json
  • fixtures/mail/observed/token-authority.json
  • package.json

Comment thread fixtures/harness/env.mjs Outdated
Comment thread fixtures/harness/inbox.mjs Outdated
Comment thread fixtures/harness/README.md Outdated
Comment thread fixtures/harness/redact.mjs Outdated
Comment thread fixtures/harness/redact.mjs Outdated
Comment thread fixtures/harness/run.mjs Outdated
Comment on lines +195 to +209
// Error messages can embed real addresses (e.g. waitForMessage timeouts) —
// the failure path gets redacted exactly like the success path.
payload = redact(
{
scenario: scenario.id,
title: scenario.title,
expectation: scenario.expectation,
runId,
recordedAt: new Date().toISOString(),
outcome: 'timeout-or-error',
error: err.message,
},
{ smtpUser: envConfig.smtpUser, helpdeskAddr: envConfig.helpdeskAddr },
);
console.error(`[harness] ${scenario.id} FAILED after ${Date.now() - startedAt}ms: ${err.message}`);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Do not log the raw failure message.

The code correctly redacts payload.error, but Line 209 prints the original err.message; the preceding comment already acknowledges that timeout errors may contain real addresses. This leaks PII into CI or terminal logs even when the fixture is sanitized.

Log the sanitized error text instead.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@fixtures/harness/run.mjs` around lines 195 - 209, Use the sanitized error
text from the redacted `payload` when reporting failures in the `run` harness
flow. Update the `console.error` call in the timeout-or-error handling block to
log `payload.error` rather than the raw `err.message`, while preserving the
existing scenario ID and duration context.

Comment thread fixtures/harness/scenarios.mjs Outdated
Comment thread fixtures/harness/scenarios.mjs Outdated
Comment on lines +3 to +4
"title": "Same customer, identical subject, no reference headers (intended different-customer — see correction)",
"expectation": "A second email reusing the exact subject of an existing conversation, with no reference headers, may merge or create its own conversation — observed, not assumed.",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Do not claim cross-customer coverage from this fixture.

Both conversations belong to customer id 3, and the correction explicitly says Gmail normalized the plus-address to the same canonical sender. This proves same-customer identical-subject behavior, not the PR objective’s cross-customer collision behavior.

Use a real second sending identity, or narrow the scenario title and objective to same-customer subject collisions.

Also applies to: 368-370

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@fixtures/mail/observed/same-subject-different-customer.json` around lines 3 -
4, The fixture incorrectly presents same-customer messages as cross-customer
coverage. Update the fixture data and expectations to use a genuinely different
customer/sending identity, or rename the scenario and objective to explicitly
describe same-customer identical-subject behavior; ensure the related cases
around the corresponding expectation entries are consistent.

Comment thread fixtures/mail/observed/token-authority.json Outdated
Security (CodeRabbit 🔒 Major): reply tokens are a routing capability,
not just an id — redact.mjs now replaces the whole FS_reply-* token with
a deterministic fake (was scrubbing only the domain), and person-email
fields on unexpected domains get a role placeholder instead of passing
through. Committed fixtures re-scrubbed: real/forged/reference tokens are
now distinct, consistent fakes (1111…/3333…/2222…); no real token hex
remains anywhere.

Robustness (CodeRabbit 🩺/🎯): waitForMessage now retries transient
IMAP connect/auth failures instead of aborting the whole poll (openClient
moved inside try, optional-chained logout); HARNESS_FS_USER_ID is
validated as a positive integer instead of silently becoming NaN.

Docs: README safety rule reworded to describe run-scoped marker isolation
accurately, including the intended same-run subject-collision exception.

Dry-run re-verified; all fixtures valid JSON.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TqG66PPZreBrj17VbAqe3b

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@fixtures/harness/redact.mjs`:
- Around line 153-158: The person email validation in the redaction branch
should accept both bare example.test addresses and subdomain forms. Update the
suffix check in the isPerson email handling to recognize values ending with
“@example.test” or “.example.test”, preserving the scrubbed deterministic
address instead of replacing it with person@example.test.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 660f1e43-c3f4-417f-8e6b-04f6657e2658

📥 Commits

Reviewing files that changed from the base of the PR and between 1fe728e and 97ba595.

📒 Files selected for processing (7)
  • fixtures/harness/README.md
  • fixtures/harness/env.mjs
  • fixtures/harness/inbox.mjs
  • fixtures/harness/redact.mjs
  • fixtures/mail/observed/forged-reply-token.json
  • fixtures/mail/observed/reply-with-reference.json
  • fixtures/mail/observed/token-authority.json
✅ Files skipped from review due to trivial changes (2)
  • fixtures/mail/observed/forged-reply-token.json
  • fixtures/harness/README.md
🚧 Files skipped from review as they are similar to previous changes (3)
  • fixtures/mail/observed/token-authority.json
  • fixtures/harness/env.mjs
  • fixtures/harness/inbox.mjs

Comment thread fixtures/harness/redact.mjs Outdated
zaridan and others added 2 commits July 10, 2026 07:42
…ssion

Fixes the incremental findings CodeRabbit surfaced on run.mjs/scenarios.mjs
(files it reached in round 2):

- redact.mjs: fix a regression from the prior fix — the person-email guard
  used .endsWith('.example.test'), which misses bare 'support@example.test'
  forms and would over-scrub good customer tags on future runs. Now matches
  any *example.test address. (Committed fixtures were never affected — they
  were reset+token-sed'd, not re-run through the buggy guard.)
- run.mjs: log the REDACTED error, not raw err.message (PII could reach
  terminal/CI logs); set process.exitCode=1 on any scenario timeout/error so
  CI can't read a failed run as success; validate --only's value and reject
  unknown options (a typo in live mode could otherwise send every probe).
- run.mjs + env.mjs: thread identityNames through both redact calls via a new
  optional HARNESS_IDENTITY_NAMES env var, so free-text names in notes/audit
  bodies are scrubbed on future runs (documented in README).
- scenarios.mjs: guard a null outbound messageId from becoming the literal
  'null' In-Reply-To header; fail fast if same-subject-different-customer is
  run standalone via --only (depends on new-conversation running first).

All verified: syntax, dry-run, suffix-fix, name-scrub, and both arg guards.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TqG66PPZreBrj17VbAqe3b
…istributed repo

The FreeScout-observation harness served its purpose (generated these
fixtures, surfaced the behavioral facts, found a prod send-bug) but
doesn't belong in a distributed product: it observes FreeScout
specifically and serves no Helpthread deployer. It now lives in the
private resonant-help repo (tools/mail-behavior-harness/).

What ships here: the mail-behavior acceptance fixtures, reframed as
Helpthread's own acceptance criteria (fixtures/mail/README.md) — the
behaviors the engine must exhibit, cited by the threading spec. The
harness-only deps (nodemailer, imapflow) and package.json go with the
harness; the engine's package.json comes with the engine.

Helpthread's own e2e mail harness (pointing at a Helpthread instance)
is future work.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TqG66PPZreBrj17VbAqe3b
@zaridan zaridan changed the title HT-7: black-box mail-behavior fixture harness HT-7: mail-behavior acceptance fixtures Jul 10, 2026
@zaridan
zaridan merged commit 901ffb8 into main Jul 10, 2026
1 check passed
@zaridan
zaridan deleted the feat/ht-7-blackbox-fixture-harness branch August 2, 2026 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant