Round 2 findings — Reviewer 1 (P1)

The changes compile and all tests pass, but fresh deployments replace configured requester names with raw transport identifiers in user-facing messages. This directly breaks the requester identity/display-name requirement.

Review comment:

• [P1] Preserve the requester's display name when creating profiles — C:\Users\fives\source\repos\founder-intake-wt-c456\FounderIntake.Host\Adapters.cs:21-23 On a fresh data directory, ResolveSlack(sourceActorId) and ResolveEmail(sourceActorId) create the requester profile without a display name, so RequesterDirectory.Resolve defaults it to the raw Slack user ID or email address. The returned identity then overrides map.Name, causing acknowledgements and responder messages to greet users as values such as U0BJECF7L4T rather than Allyson. Pass the mapped name into the directory lookup, or obtain the authenticated transport profile display name.

Round 2 findings — Reviewer 2 (18 findings, priority order at end)

I'll do an adversarial security review of the trust-hardening diff. Loading the full prompt and the actual changed sources so findings can be pinned to real file:line locations.Reading the security-critical new types and the send/confirm/idempotency paths next.# Trust-hardening review (FounderIntake / FLT-75)

Reviewer: Gronktayvius Jones-Gaffney (adversarial / red-team seat)

Overall: Round-1 criticals (optional confirm gate, missing envelope at send time, no follower reacquire, sanitizer optional) look addressed. Remaining risk is concentrated in Jira egress, lease dual-writer windows, idempotency poison pills, and thread/envelope never recycling after Done.

---

Security-critical answers

Question	Answer
Can any send path bypass the sanitizer?	Slack/AgentMail user replies: no, if they go through ReplyOutbox → OutboxPump (inspect on enqueue and before SendAsync). Jira create/comment: yes — different egress path with weaker redaction.
Can a confirm token be redeemed by someone else?	No among allowlisted actors: Confirm requires verifiedIdentity == RequesterIdentity. Non-allowlisted users never become jobs. Token is still visible in-channel.
Can internal fleet strings still leak outbound?	Mostly blocked on chat; can still reach Jira (title/transcript/writeup). Some chat edge cases remain (unicode/ZWSP, scheme-less hosts).
Is the idempotency key race-safe?	Yes on a shared SQLite file (INSERT OR IGNORE). Not under split-brain separate DBs. Incomplete reservation (ticket_key NULL) is a worse reliability hazard than races.
Can the leader lease split-brain?	Yes, briefly after expiry (~one heartbeat) while the old leader still runs; and hard if hosts do not share the same intake-leader.db. No per-job fencing token.

---

Critical / High

1. Jira egress bypasses EgressSanitizer (OWASP A01/A04 — sensitive data exposure)

Where: ConversationResponder.cs:187-217, ConversationResponder.cs:241-256, ConversationResponder.cs:280-288
Severity: High (security)

Why: Chat is dual-gated. Ticket creation is not. BuildBrief puts the first requester line into the Jira summary with no sanitizer. RedactSensitive is a thin regex (CONFIRM tokens, all URLs, a few names/BOARD) and misses WT-…, gov:, Vikunja/localhost, private IPs, harness paths, etc. Transcript comment uses that same weak redactor. LLM writeup is only RedactSensitive after Sanitize (length trim).

Concrete fix: Run the same fail-closed policy used for chat (or a shared Inspect/RedactForExternal API) on title, description, and comment before CreateAsync/AddCommentAsync. Prefer hold/operator alert over shipping raw text; at minimum apply full internal-pattern + URL-host allowlist rules.

---

2. Failed Jira create poisons the thread forever

Where: ConversationResponder.cs:175-229, IdempotencyStore.cs:47-65
Severity: High (bug + availability)

Why: Flow is: redeem token → SetThreadTicket(PENDING) → Reserve (row with ticket_key NULL) → create. If create fails/returns null:

• Thread stays PENDING
• Idempotency row stays ticket_key NULL
• State may move to Done anyway (:228-229)
• Next confirm hits “already reserved / already being filed” paths and never retries create

Concrete fix: On create failure: clear or mark reservation retryable (DELETE incomplete idempotency row, or status=failed with reclaim), reset thread ticket out of PENDING, keep state Executing (not Done), re-queue or leave job failed for retry. Only Complete + Done after a real key exists.

---

3. Active envelope + state never recycle after Done

Where: ConversationStore.cs:215-234, IntakePipeline.cs:100-106, ConversationResponder.cs:79-89, RequestStateMachine.cs:57-63
Severity: High (bug)

Why: GetOrCreateActiveEnvelope is first-writer-wins forever. After first ticket, state is Done. A new ask in the same Slack thread reuses the old RequestId. Transitions from Done are illegal; readyToFile never issues a new token. Second request in-thread is broken.

Concrete fix: On Done (or after successful ticket), clear active_thread_requests for that thread or key active request by (thread_key, open_request_id) and open a new envelope when prior is terminal. Allow Done → Received only via explicit new-request open, not silent reuse.

---

4. Leader lease allows dual writers after expiry

Where: IntakeLeaderLease.cs:35-73, IntakeLeaderCoordinator.cs:113-138
Severity: High (OWASP A04 — insecure design / multi-writer)

Why:

1. Lifetime 90s, renew every 30s. If leader freezes >90s, follower acquires while old leader still runs until its next Renew fails → overlap up to ~HeartbeatInterval.
2. Work units (OutboxPump, ConversationResponder) do not check lease token/epoch per batch — no fencing.
3. If two hosts use different DataDirectory, each has its own intake-leader.db → permanent dual leaders → dual Jira / dual sends (SQLite guards only apply per DB file).

SQLite BEGIN IMMEDIATE on a shared file is fine for mutual exclusion of TryAcquire; that does not fence in-flight work.

Concrete fix: Pass fence token into pump/responder; every lease batch checks Renew() or IsOwner(token,epoch) and aborts if lost. Prefer shorter lifetime or renew-before-work. Document that DataDirectory must be a single shared path (or use a real distributed lock). On stop, release before stopping services only after drain, or stop services first then release.

---

5. Idempotency “in flight” is not crash-safe

Where: IdempotencyStore.cs:47-65, ConversationResponder.cs:193-203
Severity: High (bug)

Why: Reserve inserts ticket_key NULL. Concurrent second worker correctly bails. After crash mid-create: orphaned Jira issue possible and permanent “already being filed” with no key. Race-safe against double insert on one DB; not safe against lost completion.

Concrete fix: Store reservation state explicitly (pending|complete|failed), include owner/lease expiry on pending rows, allow reclaim after TTL, and/or use compare-and-set Complete with Jira key discovery (search by idempotency label custom field).

---

Medium

6. Confirm / unavailable enqueue ignores Held

Where: ConversationResponder.cs:111-113, ConversationResponder.cs:232-236
Severity: Medium (bug)

Why: Normal path marks job held on OutboxEnqueueResult.Held (:95-99). EnqueueConfirmReply and the “unavailable” notice ignore the result and still MarkJobDone. User gets silence; operators get a held JSON file only if sanitizer held; job looks successful.

Concrete fix: Same as drafting path — if Held, MarkJobHeld + critical log; do not mark done.

---

7. RequestStateMachine lock is process-local only

Where: RequestStateMachine.cs:11-52, JsonFileStore.cs:16-29
Severity: Medium (OWASP A04)

Why: _gate does not coordinate two processes. Dual confirm can both pass token checks (last writer wins on file). Ticket double-create is still mitigated by SQLite SetThreadTicket if DB is shared; if not, both file Jira.

Concrete fix: Put request state in SQLite next to jobs (same transactional store as tickets/idempotency), or take a file lock around read-modify-write. Prefer one durable store for state + ticket reservation.

---

8. Sanitizer gaps / bypass shapes for chat

Where: EgressSanitizer.cs:31-54
Severity: Medium (OWASP A01)

Still fail-closed on many cases (good tests for evil hosts, ::1, link-local, BOARD, names). Residual:

Gap	Why it matters	Fix
Zero-width / homoglyph hosts (local\u200bhost)	Regex may miss; content still ships	Normalize NFKC + strip Cf category before inspect
Scheme-less evil.example/path	Not parsed as URL	Optional host-like token detection or fail on bare FQDNs outside allowlist
ftp://, file:// bare	Only https? extracted	Hold non-allowlisted absolute URIs of any scheme
InspectAndHold skips thread-binding when envelope is null (:64)	Legacy/tampered rows skip binding	Fail closed: hold if envelope missing on send for non-dry-run

Chat path still does not re-sanitize after writer transform — AgentMail templates wrap body (Adapters.cs:119-123); brand/support URL must stay allowlisted (currently slack host in defaults — OK if SupportUrl always approved).

---

9. Confirm token is a bearer secret posted in-thread

Where: ConversationResponder.cs:87-88, RequestStateMachine.cs:31
Severity: Medium (OWASP A07 — auth)

Why: Identity binding works for who may redeem, but any allowlisted peer who can spoof the same Slack user id (stolen session) can redeem. Token in channel also expands shoulder-surfing / log exfil surface. Fixed-time compare is good; tokens are 256-bit — good.

Concrete fix: Prefer Slack interactivity button bound to user_id + request id (no transferable secret), or short-lived token (e.g. 30–60 min) + single-use already present. Redact tokens from any Jira/log path (partially done).

---

10. Link tokens never expire; directory not in auth path

Where: RequesterDirectory.cs:16-34, Adapters.cs:15-32
Severity: Medium (OWASP A07) / design

Why: RequestLink tokens are unscoped in time. ConfirmLink only checks surface+identifier match. Auth still uses config allowlist StableId, not directory ProfileId — directory is mostly display/link bookkeeping, which is safer for now but easy to misuse later.

Concrete fix: TTL + single-use (already deleted on success). Keep authorization exclusively on transport-verified Slack/email allowlist; never authorize by DisplayName.

---

11. Transition(..., AwaitingConfirm) always mints a new token

Where: RequestStateMachine.cs:31
Severity: Medium (bug if path re-enters)

Why: Any legal transition into AwaitingConfirm regenerates the token. Today re-entry is blocked by the transition table, so OK — but fragile if someone adds AwaitingConfirm → AwaitingConfirm or reset paths.

Concrete fix: Mint only when ConfirmationToken is null; never rotate without invalidating outstanding user-visible tokens deliberately.

---

Low / Performance / Quality

12. Regex thrash on every inspect

Where: EgressSanitizer.cs:34-46
Severity: Low (performance)

Why: Many Regex.IsMatch / Matches with fresh patterns per call on every enqueue and every send.

Fix: static readonly Regex with RegexOptions.Compiled or [GeneratedRegex].

---

13. RequesterDirectory.Resolve O(n) full scan

Where: RequesterDirectory.cs:42-46
Severity: Low (performance)

Fix: Index surface+identifier → profileId file or SQLite unique index.

---

14. ALTER TABLE swallows all SQLite error code 1

Where: ConversationStore.cs:68-71, ReplyOutbox.cs:52-53
Severity: Low (maintainability)

Why: Code 1 is generic “SQL error”; duplicate-column is not distinguished. Real migration failures can be silent.

Fix: Match message for “duplicate column” or use PRAGMA table_info before alter.

---

15. Held outbound writes full body to disk

Where: EgressSanitizer.cs:67-68
Severity: Low (secrets / ops)

Why: Held JSON includes full reply text (possible tokens/PII) under held-outbound/.

Fix: Store hash + matched rules + truncated preview; restrict ACLs on data dir.

---

16. Dead code / wiring smell in OllamaHttpBackend

Where: AgentBackends.cs diff (_logger field added, likely unused)
Severity: Low (quality)

Fix: Use it or remove.

---

17. State machine cannot express “clarify after ready”

Where: RequestStateMachine.cs:57-63, ConversationResponder.cs:83-88
Severity: Low (product)

Why: Once AwaitingConfirm, further LLM turns cannot move back to drafting; token stays valid. Acceptable for pilot if documented.

---

18. IdempotencyStore ctor path rewrite is surprising

Where: IdempotencyStore.cs:16-18
Severity: Low

Why: Non-Data Source= strings become Path.ChangeExtension(..., ".db") — easy to open a different file than callers expect.

Fix: Require a full connection string only.

---

What looks solid (do not “fix” these)

1. Dual-gate chat egress — ReplyOutbox.Enqueue + OutboxPump re-inspect (ReplyOutbox.cs:73, OutboxPump.cs:62-67).
2. Confirm requires token shape + envelope + state + identity — bare confirm|approve|go ahead held (ConversationResponder.cs:156-161, tests).
3. Requester-bound token with length check before FixedTimeEquals (RequestStateMachine.cs:45-49).
4. Thread-first ticket reservation before Jira (SetThreadTicket then create).
5. Idempotency key from original ask frozen on active envelope (IntakePipeline.cs:100-105).
6. HTTPS fail-closed on Jira/AgentMail endpoints.
7. Follower reacquire loop in IntakeLeaderCoordinator (R1 gap closed).

---

Priority fix order

1. Jira path through full egress policy (or shared redactor with same rules as chat).
2. Reclaim path for failed create / NULL idempotency / PENDING ticket.
3. Envelope lifecycle after Done (second request in thread).
4. Lease fencing on every batch + shared data dir requirement.
5. Held handling on confirm/unavailable enqueues.
6. Normalize/expand sanitizer edge cases; compile regexes.

---

Suggested regression tests (missing today)

• Jira title/description containing docs/BOARD.md, WT-ab12, 127.0.0.1, harness names → create blocked or fully redacted.
• Jira create throws after Reserve → second confirm still can create exactly one ticket.
• Second independent ask in same Slack thread after Done → new token + new ticket.
• Leader freeze past lifetime with overlapping work → at most one successful Jira create (shared DB) and no dual sends if fencing added.
• Confirm enqueue that fails sanitizer → job held, not done.

---

Jira FLT-75 · Vikunja · BOARD.md