Skip to content

fix(web/chat): wrap long unbreakable strings in chat message bubbles (fixes #1738)#1742

Merged
Wirasm merged 1 commit into
coleam00:devfrom
truffle-dev:fix/chat-message-overflow-1738
May 25, 2026
Merged

fix(web/chat): wrap long unbreakable strings in chat message bubbles (fixes #1738)#1742
Wirasm merged 1 commit into
coleam00:devfrom
truffle-dev:fix/chat-message-overflow-1738

Conversation

@truffle-dev

@truffle-dev truffle-dev commented May 21, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Problem: Long unbreakable strings (URLs, hashes, tokens) in chat messages overflow the message-bubble container. User bubbles use max-w-[70%] and assistant content lives inside a chat-markdown block, but neither path sets overflow-wrap: break-word, so a single long word punches through the layout.
  • Why it matters: Issue Chat message text overflows outside the message container/card. #1738 reports this as a UI break — long content escapes the bubble and disrupts surrounding layout for both user and assistant messages.
  • What changed: Two surgical CSS fixes. User-bubble <p> gets break-words + min-w-0 (so the flex child can shrink below intrinsic width). The hand-rolled .chat-markdown typography rules now declare overflow-wrap: break-word on p, li, td, and a.
  • What did NOT change: Code blocks (<pre>) still use overflow-x-auto and are deliberately excluded — horizontal scroll is the right behavior for code. No layout, color, spacing, or component structure changes.

UX Journey

Before

User pastes long URL ──▶  message bubble        ──▶ <p> renders with no break rule
                          max-w-[70%]               ↓
                                                    ❌ URL extends past bubble edge,
                                                       pushes layout sideways

After

User pastes long URL ──▶  message bubble        ──▶ <p> with break-words + min-w-0
                          max-w-[70%]               ↓
                                                    ✅ URL wraps at character boundary
                                                       inside the bubble

Same fix applied to assistant markdown content via .chat-markdown rules.

Architecture Diagram

Before

MessageBubble.tsx
  ├─ user branch:  <p className="text-sm ... flex-1">  (no break rule)
  └─ assistant:    <div className="chat-markdown ...">
                     └─ ReactMarkdown → <p>, <li>, <a>, <td>  (no break rule)

After

MessageBubble.tsx
  ├─ user branch:  <p className="... break-words min-w-0 flex-1">  [~]
  └─ assistant:    <div className="chat-markdown ...">
                     └─ ReactMarkdown → <p>, <li>, <a>, <td>
                                        ↑ now inherit overflow-wrap: break-word [~]
                                        via index.css .chat-markdown rules

Connection inventory:

From To Status Notes
MessageBubble user <p> flex layout [~] adds break-words min-w-0 so flex child can wrap
.chat-markdown p typography [~] adds overflow-wrap: break-word
.chat-markdown li, td, a typography [+] new rule, same property
.chat-markdown pre code blocks unchanged — keeps overflow-x-auto (intentional)

Label Snapshot

  • Risk: risk: low
  • Size: size: XS
  • Scope: web
  • Module: web:chat

Change Metadata

  • Change type: bug
  • Primary scope: web

Linked Issue

Validation Evidence

bun run type-check    # clean (tsc --noEmit)
# lint + prettier ran via lint-staged on commit, both green

Manual verification: pasted a 200-char URL (https://example.com/foo/bar/baz? + 150 random chars) into a chat message and confirmed it now wraps inside the user bubble instead of overflowing. Repeated with a long markdown link in an assistant response — wraps inside .chat-markdown. Code blocks still scroll horizontally as before.

Security Impact

  • New permissions/capabilities? No
  • New external network calls? No
  • Secrets/tokens handling changed? No
  • File system access scope changed? No

Compatibility / Migration

  • Backward compatible? Yes — pure CSS, no API or data shape changes.
  • Config/env changes? No
  • Database migration needed? No

Human Verification

  • Verified scenarios: long URL in user bubble wraps; long URL in assistant markdown wraps; code block still scrolls horizontally; existing short messages unchanged.
  • Edge cases checked: long token without spaces, long markdown link [text](very-long-url), mixed text + long URL, table cell with long token.
  • What was not verified: cross-browser beyond Chromium 130. overflow-wrap: break-word is supported in all evergreen browsers per MDN.

Side Effects / Blast Radius

  • Affected subsystems/workflows: chat UI only (packages/web/src/components/chat/MessageBubble.tsx + .chat-markdown rules in index.css).
  • Potential unintended effects: text that previously rendered on one line might now wrap if the container is narrower than the content. This is the intended fix; no other layout reads from these rules.
  • Guardrails/monitoring: visual regression caught at build/dev time.

Rollback Plan

  • Fast rollback: git revert <commit> — pure CSS revert, no state or schema impact.
  • Feature flags: none required.
  • Observable failure symptoms: text would resume escaping the bubble.

Risks and Mitigations

  • Risk: A consumer of .chat-markdown outside the chat surface expected text not to wrap.
    • Mitigation: grepped the codebase for chat-markdown — only used in MessageBubble.tsx. No other call sites.

Summary by CodeRabbit

  • Bug Fixes
    • Improved text wrapping in chat messages to properly handle long unbroken content (such as URLs) for better readability and consistent display across all message types.

Review Change Stack

…ixes coleam00#1738)

User-bubble <p> and the .chat-markdown typography rules had no
overflow-wrap, so long URLs and tokens broke out of the max-w-[70%]
container.

- MessageBubble: add break-words + min-w-0 to the flex-1 paragraph so
  it can shrink below intrinsic content width.
- index.css: add overflow-wrap: break-word to .chat-markdown p, li, td,
  and a. Code blocks already use overflow-x-auto and are excluded.
@coderabbitai

coderabbitai Bot commented May 21, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e90a99a5-b51b-4f1b-a454-634b4e55446b

📥 Commits

Reviewing files that changed from the base of the PR and between aa71520 and dd9da23.

📒 Files selected for processing (2)
  • packages/web/src/components/chat/MessageBubble.tsx
  • packages/web/src/index.css

📝 Walkthrough

Walkthrough

This PR fixes chat message text overflow by adding word-breaking rules at both the component and global CSS levels. MessageBubble's message paragraph adds break-words and min-w-0 classes, while the chat markdown CSS applies overflow-wrap: break-word; to paragraph, list item, table cell, and link elements.

Changes

Chat message text wrapping fix

Layer / File(s) Summary
Message text wrapping fix
packages/web/src/components/chat/MessageBubble.tsx, packages/web/src/index.css
MessageBubble message content styling adds break-words and min-w-0 classes to existing flex layout; chat markdown CSS applies overflow-wrap: break-word; to multiple elements (paragraph, list item, table cell, anchor) to ensure long unbroken text wraps correctly within message boundaries.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 Long words that stretched beyond the bubble's bounds,
Now wrap and break with CSS rules profound!
min-w-0 and break-words play their part,
While overflow-wrap fixes every chart—
Long text no more breaks the UI art! 📝

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% 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
Title check ✅ Passed The title clearly and specifically summarizes the primary change: wrapping long unbreakable strings in chat message bubbles, directly addressing the linked issue #1738.
Description check ✅ Passed The description comprehensively follows the template with all major sections complete: Summary with problem/impact/changes/scope, UX Journey before/after flows, Architecture Diagram with connection inventory, Label Snapshot, Change Metadata, Linked Issue, Validation Evidence, Security Impact, Compatibility, Human Verification, Side Effects, Rollback Plan, and Risks/Mitigations.
Linked Issues check ✅ Passed The PR fully addresses issue #1738's objective: long unbreakable strings (URLs, tokens) now wrap within message bubbles via CSS overflow-wrap: break-word and flex layout fixes, preventing text overflow while preserving code-block scrolling.
Out of Scope Changes check ✅ Passed All changes are directly scoped to fixing text wrapping in chat messages: MessageBubble.tsx and index.css modifications target only the chat UI with no extraneous refactors, dependencies, or unrelated alterations.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@Wirasm

Wirasm commented May 22, 2026

Copy link
Copy Markdown
Collaborator

@truffle-dev this PR appears to fully address #1738. Consider adding Closes #1738 to the PR body so the issue auto-closes on merge.

@truffle-dev

Copy link
Copy Markdown
Contributor Author

It is in the body under ## Linked Issue (- Closes #1738), and the auto-link is active — closingIssuesReferences returns #1738. Happy to move it higher in the body if that helps the read.

@Wirasm

Wirasm commented May 25, 2026

Copy link
Copy Markdown
Collaborator

Review Summary

Verdict: ready-to-merge

Clean CSS fix for issue #1738. Long unbreakable strings (URLs, tokens) in chat bubbles now wrap properly without horizontal overflow. Two targeted changes: break-words min-w-0 on user-bubble p, and overflow-wrap: break-word on .chat-markdown elements. No regressions expected.

Blocking issues

None.

Suggested fixes

None.

Minor / nice-to-have

None.

Compliments

  • Correct use of Tailwind min-w-0 on the flex child to enable shrinking — this is the right pattern for breaking words inside a flex container.
  • Intentionally excluded <pre> blocks from overflow-wrap to preserve horizontal scrolling in code blocks — appropriate design choice.
  • PR description is clear and explains both the problem and the solution concisely.

Reviewed via maintainer-review-pr workflow (Pi/Minimax). Aspects run: code-review.

@Wirasm
Wirasm merged commit da31993 into coleam00:dev May 25, 2026
4 checks passed
@Wirasm Wirasm mentioned this pull request May 28, 2026
truffle-dev added a commit to truffle-dev/truffleagent-site that referenced this pull request May 28, 2026
Three new external merges since last refresh:
- alash3al/stash#1 (docs alignment, 2026-05-28)
- HKUDS/DeepTutor#485 (sync→async require_auth, 2026-05-28)
- coleam00/Archon#1742 (chat bubble overflow wrap, 2026-05-25)

Totals: 46 → 49 PRs, 25 → 26 orgs.
Wirasm added a commit that referenced this pull request Jul 20, 2026
* chore: update Homebrew formula for v0.3.11

* chore: fix broken docs links (#1633)

* fix(orchestrator): move system context to cacheable systemPrompt.append (fixes #1591) (#1634)

* fix(orchestrator): move system context to cacheable systemPrompt.append (fixes #1591)

Prompt caching was broken because the orchestrator embedded its static
system context (project list, workflows, routing rules) in the prompt
parameter, which changes every turn. This caused the Anthropic API to
rebuild the cache prefix on each request (high cache_creation_input_tokens,
zero cache_read_input_tokens).

Move the static orchestrator context into systemPrompt.append, which
extends the Claude Code preset and is part of the cacheable system prompt
prefix. The prompt parameter now contains only per-turn dynamic content
(workflow results, thread context, user message, issue context, files).

Changes:
- Add buildOrchestratorSystemAppend() to prompt-builder.ts
- Simplify buildFullPrompt() to user-facing content only
- Set requestOptions.systemPrompt with preset + append in handleMessage()
- Widen systemPrompt type in AgentRequestOptions and NodeConfig to accept
  the SDK's full union type (string | string[] | preset object)
- Update tests for new prompt construction path

* fix(orchestrator): move system context to cacheable systemPrompt.append

Fixes coleam00/Archon#1591

* refactor: extract SystemPromptInput type alias to prevent drift

Address CodeRabbit review: consolidate the duplicated systemPrompt
union type into a named type (SystemPromptPreset + SystemPromptInput)
so both AgentRequestOptions and NodeConfig reference a single definition.

* fix: restore file modes (0755 → 0644) from rebase churn

Restore 812 files that had their mode bits changed from 100644 to
100755 during a rebase. No content changes — mode-only fix.

Addresses S4 from review feedback.

* fix(providers,workflows): treat Claude SDK stop_sequence success as success (#1425) (#1662)

* fix(providers,workflows): treat Claude SDK stop_sequence success as success (#1425)

The Claude Agent SDK's SDKResultSuccess type declares is_error as boolean
(not literal false). When a model terminates via a configured stop sequence
the SDK sets is_error: true while keeping subtype: 'success' — its encoding
of "non-default termination, but not a failure".

The claude provider forwarded is_error verbatim into MessageChunk.isError,
so three downstream consumers (dag-executor main path, dag-executor loop
branch, orchestrator-agent direct chat) misclassified clean stop_sequence
terminations as node failures and produced the contradictory user-hostile
error "Node '<X>' failed: SDK returned success" — even though the AI had
already completed correctly and written its output. Multi-user impact on
v0.3.9 / v0.3.11 across review-classify and synthesis pipelines.

Changes:
- claude/provider.ts: normalise is_error + subtype: 'success' as a clean
  result. Don't propagate isError downstream; log a debug-level
  claude.result_success_stop_sequence event for observability instead.
- dag-executor.ts (main path + loop branch): defense-in-depth guard so
  third-party IAgentProvider implementations that forward the SDK pair raw
  can't reintroduce the same false-failure.
- provider/dag-executor/orchestrator-agent tests: regression tests covering
  the stop_sequence success path; guard test ensures real error subtypes
  (error_max_turns) still propagate.

Fixes #1425

* fix(orchestrator,providers,workflows): address review feedback for #1425

- Apply errorSubtype !== 'success' guard at orchestrator-agent.ts handleStreamMode
  and handleBatchMode result branches (defense-in-depth, mirrors dag-executor).
  Without this, a third-party IAgentProvider that forwards the SDK pair raw
  would surface a spurious error on direct chat and drop conversation output.
  Adds matching regression test.
- Rename log event claude.result_success_stop_sequence -> claude.result_success_validated
  per CodeRabbit; aligns with {domain}.{action}_{state} pino convention.
- Lock the new debug log into the provider regression test.
- Drop (#1425) issue refs from production comments (rot risk per CLAUDE.md).
- Rewrite loop-branch guard comment to be self-contained instead of cross-
  referencing the main-path guard 1000+ lines away.
- Add CHANGELOG entry under [Unreleased] -> Fixed.

* fix(providers): preserve native tools when skills are set without allowed_tools (#1605) (#1661)

When a DAG node has `skills:` but no `allowed_tools:`, the
AgentDefinition wrapper defaulted tools to `['Skill']` only, stripping
all native Claude Code tools (Read, Bash, Write, etc.).

Fix: omit the `tools` field on AgentDefinition when `options.tools` is
undefined, letting the SDK provide its full default tool set. When
`allowed_tools` is explicitly set, Skill is still appended to the list.

* fix(core): match SSH URL host generically, not just github.com (#1656)

The SSH-to-HTTPS converter in normalizeRepoUrl() and registerRepository()
only matched `[email protected]:` literally. Custom SSH host aliases, GitHub
Enterprise, Gitea, GitLab, and Bitbucket SSH URLs were left unchanged,
which produced workspace paths containing literal `git@<host>:` segments.
On Windows the colon makes mkdir fail with ENOTDIR; on Unix the owner
extraction is malformed.

Replace the literal-host check with an SCP-style regex
`/^git@([^:]+):(.+)$/` at both call sites. The github.com case still
converts identically; new hosts (custom aliases, GHE, GitLab, Bitbucket)
now produce path-safe HTTPS URLs.

Closes #1614.

* fix(workflows): persist structuredOutput on NodeOutput so $node.output.field works for Pi (#1654)

* fix(workflows): persist structuredOutput on NodeOutput so $node.output.field works for Pi

When a provider parses fence-wrapped or preamble-prefixed JSON onto the
result chunk (Pi/Minimax via tryParseStructuredOutput), the executor
captured it locally but never persisted it onto NodeOutput. Downstream
consumers (substituteNodeOutputRefs, condition-evaluator) then
JSON.parse(output)'d the original prose-prefixed text, which threw, and
$node.output.field resolved to empty.

This persists structuredOutput on NodeOutput (single-shot and
loop-terminal-iteration success paths) and teaches both consumers to
prefer the parsed object over re-parsing prose. Falls back to
JSON.parse(output) when structuredOutput is absent so Claude/Codex
output_format-encoded NodeOutput rows (and older rows written before
this field existed) keep working.

Cross-resume rehydration of structuredOutput from event_data is
out of scope here; resumed runs that re-execute downstream nodes
will fall through to the JSON.parse path, which matches existing
behavior.

Closes #1571

* test(workflows): docstring the structuredOutput makeOutput fixtures

* chore(workflows): drop direction/scope gate from maintainer-review-pr (#1675)

The gate node was the most fragile part of the workflow. It used Pi/Minimax
to return a structured JSON verdict that the DAG branched on, but Pi
intermittently wrapped the JSON in markdown fences or prefixed reasoning
prose, breaking the condition-evaluator's field extraction. When that
happened, every downstream review aspect was silently skipped and the
workflow exited 0 with no review posted — indistinguishable from a
successful run where the gate legitimately declined.

In practice the gate added no signal for hand-picked PRs from the morning
standup brief: across two full days of usage (~13 runs) the gate returned
"review" every single time. The decline/needs_split/unclear branches were
never exercised. Removing the gate eliminates the failure mode without
losing any verdict the workflow has actually produced.

Changes:
- Remove gate, approve-decline, post-decline, approve-unclear nodes from
  maintainer-review-pr.yaml.
- Rewire review-classify to depend on [fetch-pr, fetch-diff].
- Drop read-context (only the gate command consumed it).
- Simplify record-review: hardcode gate_verdict to "review" for back-compat
  with the standup brief's marker logic; drop the one_success join.
- Drop interactive: true (no more approval gate).
- Update synthesize and report commands to remove gate-decision references.
- Delete the now-orphan maintainer-review-gate.md command.

If we later wire up automated review on every open PR (where the
direction/scope decline would matter), reintroduce the gate then — and
this time with a hardened parser or Claude provider on the gate node.

* fix(server,workflows,web): surface bundled defaults on /api/workflows when no project context (#1618)

* fix(server,workflows,web): surface bundled defaults on /api/workflows when no project context (#1173)

GET /api/workflows short-circuits to an empty array when there is no
`cwd` query param and no registered codebases. The handler never
reaches discovery, so bundled defaults are not surfaced and the UI
renders a misleading "Add workflow definitions to .archon/workflows/"
empty state on first run — even though the bundled YAML files are
present on disk.

This change:

- Threads `cwd: string | null` through `discoverWorkflows` and
  `discoverWorkflowsWithConfig`. When `cwd` is `null` the discovery
  function loads bundled + home scopes and skips the project step
  cleanly (no path-join with an empty cwd, no read-error noise).
- Removes the early-return in the GET handler. When no project
  context exists, it now calls `discoverWorkflowsWithConfig(null, ...)`
  so the response carries the bundled set instead of `[]`.
- Distinguishes the empty-state copy in `WorkflowList` so the rare
  case where the list is genuinely empty reads correctly. With a
  project selected: "No workflows found in this project. Add
  workflow definitions to .archon/workflows/ in the project root."
  Without a project: "No workflows are available. Bundled defaults
  should appear here automatically; if they do not, check that
  `defaults.loadDefaultWorkflows` is enabled in your config."

Tests cover both the API (new `falls back to null cwd when no cwd
query and no codebases registered` case in `api.workflows.test.ts`)
and the discovery layer (new `discoverWorkflows with null cwd` block
in `loader.test.ts` asserting no project-source entries and no
project-step read errors).

* test(workflows): assert bundled defaults surface when cwd is null

The second test in the null-cwd discovery group only verified that
project-source workflows are absent. That assertion would still pass
if the bundled-defaults loader silently regressed.

Add an explicit `bundled` source-label assertion so the test catches
that regression directly.

* fix(workflows): address review on #1618

- api.md: document cwd-omitted behavior so the empty-state case is discoverable
- workflow-discovery.ts: docstring explains loadDefaults default rather than just naming the skipped branch
- api.workflows.test.ts: mockDiscoverWorkflows accepts string | null to match the wider signature
- api.ts: drop trailing period inside the multi-line inline comment
- CHANGELOG.md: add the #1173 Fixed entry under [Unreleased]
- loader.test.ts: add a regression test that asserts loadConfig is not invoked when cwd is null

* test(workflows): tighten null-cwd assertions + drop rot-prone refs

Address Wirasm's polish review on #1618:

- loader.test.ts: assert result.workflows.length === 0 in the
  loadDefaults:false case so the test no longer passes if bundled
  defaults are accidentally loaded.
- loader.test.ts: drop the inline (issue #1173) reference and the
  workflow-discovery.ts file+line pointer from the two comments
  that risk rotting on future refactor.
- api.workflows.test.ts: drop the inline (issue #1173) reference.
- CHANGELOG: append positive framing to the #1173 line so the
  entry reads as "what works now" not just "what no longer breaks".

* fix(marketplace): trigger auto-review on ready_for_review

The pull_request_target event doesn't fire on type=ready_for_review
unless explicitly listed. Add it so flipping a draft PR to ready
triggers the marketplace auto-review.

Co-Authored-By: Claude Opus 4.7 <[email protected]>

* feat(marketplace): add archon-idea-to-wo

* feat(marketplace): add archon-idea-to-wo workflow

Adds one entry to the marketplace registry for archon-idea-to-wo —
an interactive 8-node workflow that turns a raw idea into BKM-format
Work Orders through four AI phases with approval gates between each.

Originally authored by @lamachine in PR #1647, where it was proposed
as a bundled default. Repackaged as a standalone SHA-pinned external
repo (coleam00/archon-idea-to-wo) so it can be published through the
community marketplace without waiting for an Archon release.

- Author: lamachine
- Tags: planning, development
- Source: coleam00/archon-idea-to-wo @ 3b0d5d82 (directory format)
- archonVersionCompat: >=0.3.0

Closes #1647

* chore: re-trigger marketplace auto-review after ready-for-review

PR was flipped to ready-for-review before the action's trigger list
included ready_for_review (fixed in d8d5a35b on dev). Empty commit
fires the synchronize event so the auto-review runs now that the
draft gate is cleared.

Co-Authored-By: Claude Opus 4.7 <[email protected]>

---------

Co-authored-by: Claude Opus 4.7 <[email protected]>

* fix(marketplace-auto-review): always post PR comment on auto_merge/auto_approve

The 'gh pr review --approve' call fails with 'GitHub Actions is not
permitted to approve pull requests' unless the repo has 'Allow GitHub
Actions to create and approve pull requests' enabled in Settings.
When the approve call fails, fall back to 'gh pr comment' so the PR
author still gets a notification with the auto-review reasoning. The
merge step still runs either way.

Co-Authored-By: Claude Opus 4.7 <[email protected]>

* Release 0.3.12

* feat(marketplace): add archon-smart-mr-review

GitLab counterpart to archon-smart-pr-review. Adaptive code review of a
GitLab MR — Haiku classifies which review agents are relevant, runs them
in parallel, posts resolvable Discussion threads, auto-approves on 0
critical findings.

Source: lraphael/archon-gitlab-workflows@55ca7349

Co-authored-by: Raphael Lechner <[email protected]>

* fix(marketplace-auto-review): feed actual workflow source to AI reviewer

Before this change the ai-review node only saw four things in its
prompt: PR metadata, the entry diff, the schema validator's pass/fail
summary, and the security scanner's severity+findings summary. The
fetched workflow YAML and command files at the pinned SHA were saved
to $ARTIFACTS_DIR/source/ but never surfaced to the AI, so Haiku ended
up 'reviewing' the registry diff instead of the actual workflow.

Add a bundle-source node that emits every fetched file (capped at 12k
chars each to keep the prompt sane) and reference it in the ai-review
prompt as the artifact under review. Rewrite the prompt instructions
to make Haiku explicitly read the YAML + commands and name what it
concluded about the workflow's behavior in 'reasoning'. Update the
auto_merge comment template so it names what was actually reviewed.

Co-Authored-By: Claude Opus 4.7 <[email protected]>

* feat(marketplace): add archon-comprehensive-mr-review

GitLab counterpart to archon-comprehensive-pr-review. Full code review of
a GitLab MR — all 5 review agents (code-review, error-handling,
test-coverage, comment-quality, docs-impact) run in parallel, posts
resolvable Discussion threads, auto-approves on 0 critical findings.

Source: lraphael/archon-gitlab-workflows@6e39b359

Co-authored-by: Raphael Lechner <[email protected]>

* fix(workflows): surface condition_json_parse_failed as workflow error instead of silent skip (#1673) (#1694)

When a when: condition references $nodeId.output.field and the node's
output text is not valid JSON, the condition evaluator now returns
parsed:false so the DAG executor treats it as an error (instead of
silently skipping downstream nodes and exiting 0).

Additionally, strip common markdown fences (e.g. ```json blocks)
from output text before attempting JSON.parse, handling the common
Pi/Minimax pattern of wrapping JSON in fences.

* fix(scripts): use line-anchored regex to extract ARCHON_STATE_JSON markers (#1695)

* Fix: extract ARCHON_STATE_JSON markers as standalone lines (#1674)

The persist script extracted Claude's state-JSON block by substring-matching
the BEGIN/END markers, which gave a false match whenever the marker string
appeared inside the brief's prose or inside a JSON string value (e.g. a PR
title narrating this very bug). PR #1676's switch to lastIndexOf made the
self-referential case worse, since the last substring occurrence may now sit
inside a JSON value.

Match the markers only when they occupy an entire line (line-anchored
^...$ regex with the m flag) and pick the last END plus the last BEGIN
before it, so duplicate-emission and substring-in-content both resolve
correctly.

Changes:
- Replace indexOf/lastIndexOf substring matching in Tier 1 with
  line-anchored matchAll regex over BEGIN/END markers
- Add tests for marker substring in brief prose, marker substring inside
  state JSON value, and combined duplicate-BEGIN + marker-in-prose case
- Keep PR #1676's existing test cases (single block, duplicate BEGINs,
  JSON-wrapper fallback, no-valid-format exit 1)

Fixes #1674

* fix(scripts): address review findings in maintainer-standup-persist

- Add WARN diagnostic when all BEGIN markers appear after last END
  (previously silent fallthrough produced misleading terminal error)
- Include 200-char candidate preview in JSON parse error message
  (error was unactionable in headless workflow without raw output)
- Wrap mkdirSync/writeFileSync in try-catch with structured PERSIST FAILED
  message instead of raw Bun stack trace on disk errors
- Fix brief assignment comment to explain WHY [0] vs last-pair asymmetry
- Add test: BEGIN present but END absent (truncated output) exits 1
- Add test: prose preamble before first heading is stripped from brief
- Add clarifying comment to Test 6 noting it is defence-in-depth, not
  a case that failed under the old indexOf approach

* simplify: collapse multi-line comment blocks to single lines

* fix(web): add error handling for copy message button (#1564)

* fix(web): add error handling for copy message button

Handle navigator.clipboard.writeText() failures gracefully:
- Add copyError state to track clipboard API errors
- Show X icon with error color when copy fails
- Reset error state after 2 seconds
- Closes #1540

* docs(MessageBubble): add JSDoc comments for CodeRabbit coverage

Add docstrings to MessageBubbleRaw component and copyMessage function
to satisfy 80% docstring coverage requirement.

Closes #1540

* refactor(MessageBubble): remove JSDoc comments per policy, add debug logging

Per Wirasm review - remove redundant JSDoc blocks that restate code.
The function names and types already convey what the code does.
Add console.debug for clipboard errors for debugging support.

Closes #1540

* fix(workflows): support editing global workflows (#1557)

* fix(workflows): support editing global workflows

* fix: address review feedback on global workflow editing

- Document the `source` query parameter on PUT and DELETE in the API
  reference, including the 400 error for invalid values.
- Document the new `source: "global"` response value for GET, with the
  three-tier auto-discovery order spelled out.
- Add a DELETE test for `?source=global` to confirm the home-scoped file
  is removed (PUT already had coverage; DELETE was missing).
- Add 400 "Invalid workflow source" validation tests for both PUT and
  DELETE so the enum rejection path is locked down.

The blocking issue from the prior review — PUT/DELETE routes using
cwdQuerySchema instead of workflowTargetQuerySchema — was already
resolved on the branch via the rebase against current dev; both routes
now correctly use workflowTargetQuerySchema. The comment-rot reference
(`source:global` string in a comment) was dropped as part of the rebase
conflict resolution where dev's home-scoped resolver block was kept.

---------

Co-authored-by: Rasmus Widing <[email protected]>

* feat(workflows): support Codex MCP nodes (#1459)

* feat(workflows): support Codex MCP nodes

* fix: address MCP workflow review feedback

* fix: address review feedback on Codex MCP support

- Drop the broken `inferProviderFromModel` import from
  packages/cli/src/commands/workflow.ts. The module it referenced
  (@archon/workflows/model-validation) does not exist, which caused the
  CLI package's type-check to fail. Per CLAUDE.md, provider is resolved
  via an explicit chain (`node.provider ?? workflow.provider ?? config.assistant`)
  and model never influences provider selection — vendor SDKs add new
  model names faster than we can keep a mapping in sync, so the
  model-based inference fallback was the wrong primitive anyway. Removed
  the use site and added a comment anchoring the explicit-chain rule.
- Drop "for Claude workflows" from the
  `dag.mcp_plugin_connection_suppressed` documentation in mcp-servers.md.
  The log event is provider-agnostic and the qualifier was misleading
  once Codex got MCP support too.
- Add CHANGELOG.md entry under [Unreleased] for the MCP-for-Codex
  feature with a short note on the system-chunk surfacing behavior.

---------

Co-authored-by: Kirill <[email protected]>
Co-authored-by: Rasmus Widing <[email protected]>

* fix(workflows): pass user-controlled variables via env vars in bash nodes (#1651)

* fix(workflows): pass user-controlled variables via env vars in bash nodes (#1585)

Stop substituting user-controlled variables ($USER_MESSAGE, $ARGUMENTS,
$LOOP_USER_INPUT, $REJECTION_REASON, $LOOP_PREV_OUTPUT, and context
variables) into bash node script bodies before passing to `bash -c`.
Instead, pass them as environment variables on the subprocess.

This prevents shell injection when user messages contain metacharacters
(backticks, $(), unbalanced quotes, etc.) that would otherwise be
interpreted as shell syntax.

The fix adds a `shellSafe` option to `substituteWorkflowVariables()`
that skips user-controlled variable replacement. System-controlled
variables ($WORKFLOW_ID, $ARTIFACTS_DIR, $BASE_BRANCH, $DOCS_DIR)
are still substituted normally since they contain safe, known values.

`substituteNodeOutputRefs()` with `escapedForBash=true` was already
safe via `shellQuote()` — only the Pass 1 variables needed fixing.

Closes #1585. Subsumes #1377.

* fix: address CodeRabbit review — correct loop env semantics and harden test cleanup

- LOOP_PREV_OUTPUT in until_bash now correctly references the previous
  iteration's output (not the current one)
- Test uses try/finally for spy cleanup and asserts call count before
  dereferencing mock.calls

* fix(clone): authenticate non-GitHub forge URLs via GITLAB_TOKEN / GITEA_TOKEN (fixes #1655)

The clone handler only injected auth tokens for github.com URLs.
Private repos on GitLab, Gitea, or Forgejo failed to clone via the
Web UI URL form.

Replace the github.com-specific string substitution with a
forge-aware resolver that:

- Looks up the correct env var per hostname (GH_TOKEN, GITLAB_TOKEN,
  GITEA_TOKEN)
- Applies the correct auth URL scheme (oauth2: prefix for GitLab,
  bare token for GitHub/Gitea)
- Handles self-hosted instances (gitlab.mycompany.com, gitea.myorg.com)

Add 11 new tests: 5 integration (GitLab, self-hosted GitLab, Gitea,
Forgejo, unknown forge) + 4 resolveForgeAuth unit tests + 2 updated
existing tests.

* fix: match forge auth on parsed hostname only (CodeRabbit security review)

Replace substring matching on the full URL with hostname-based
matching. This prevents token leakage when forge names appear in
URL paths (e.g. https://evil.example.com/gitlab/mirror).

- Exact hostname match for known forges (github.com, gitlab.com, gitea.com)
- Label-based match for self-hosted instances (gitlab.mycompany.com)
- Add 2 security regression tests for path-based injection vectors

* fix: add timeout and error logging to until_bash execution

Address review feedback from @Wirasm:
- Add SUBPROCESS_DEFAULT_TIMEOUT to until_bash execFileAsync to prevent
  indefinite hangs (matching other bash invocations in this file)
- Log non-ENOENT errors in until_bash catch block so syntax errors and
  permission issues are visible instead of silently swallowed

* fix: address review feedback — loop env semantics, test cleanup, docs

- Fix LOOP_USER_INPUT in until_bash: only populate on first iteration
  (consistent with LLM prompt substitution at line 1874)
- Change afterAll → afterEach in multi-forge test cleanup (throw-safe)
- Add GITLAB_TOKEN/GITEA_TOKEN cleanup to parent beforeEach
- Add auth env vars section to variables.md reference
- Add shell injection prevention note to script-nodes.md

* fix(clone): authenticate non-GitHub forge URLs via GITLAB_TOKEN / GITEA_TOKEN (fixes #1655) (#1658)

* fix: address review feedback — hostname bug, docs, SSH support, test cleanup

- Fix parsed.host → parsed.hostname to avoid port duplication in clone URLs
- Generalize SSH→HTTPS conversion from GitHub-only to all forges (git@host:path)
- Fix ForgeAuthEntry docstring (remove non-existent exact: true/false refs)
- Add GITLAB_TOKEN/GITEA_TOKEN docs to quick-start, configuration, adapter pages
- Add SSH URL + port duplication regression tests
- Harden test env cleanup with beforeEach/afterEach, import afterEach from bun:test
- Remove redundant per-test env cleanup (beforeEach/afterEach handles it)

Addresses review by @Wirasm on #1658.

* fix(clone): hostname boundary check, remove dead code, add bare URL test

- Only match first hostname label for self-hosted forge detection,
  preventing token leakage to crafted hostnames like
  gitlab.example.com.gitlab.attacker.com
- Remove dead gitea.com entry from FORGE_AUTH (handled by SELF_HOSTED_FORGE)
- Add test for bare host/path URL form (github.com/owner/repo)
- Move orphaned JSDoc comment above resolveForgeAuth function

* Feat/pi deferred extension model resolution (#1509)

* feat(pi): support extension-registered provider models via deferred resolution

Extension providers (e.g. pi-provider-kiro) register their models on
the ModelRegistry during session.bindExtensions(), not during the
initial modelRegistry.find() call. The previous approach threw
immediately when find() returned undefined, blocking extension models.

New flow:
1. modelRegistry.find() checks static catalog + models.json (step 3)
2. If found: pass to createAgentSession, fail-fast auth as before
3. If not found: log info, skip auth (extension providers manage their
   own credentials), create session without model
4. After bindExtensions() (step 4g): retry modelRegistry.find() — now
   extension-registered models are discoverable — and call
   session.setModel() to switch to the resolved model
5. If still not found after bindExtensions(): throw with a hint about
   installing the provider extension with enableExtensions: true

This preserves all existing behavior for built-in providers and
models.json custom models while adding support for extension-registered
providers like pi-provider-kiro (Kiro API — 20 free models including
Claude, DeepSeek, Qwen, etc.).

Changes:
- provider.ts: deferred model resolution after bindExtensions();
  conditional model arg to createAgentSession; conditional auth
  fail-fast; session.setModel() for extension-resolved models
- provider.test.ts: add setModel mock; update model-not-found tests
  for two-phase find() behavior

* test: add archon-test-pi example workflow for Kiro extension validation

Simple two-node workflow that verifies the Pi provider works with
extension-registered models (e.g. pi-provider-kiro). Node 1 asks a
factual question, node 2 validates the answer.

Tested with kiro/claude-sonnet-4-6 and kiro/minimax-m2-5.

* docs: add Pi/Kiro prerequisites and setup to examples README

* fix(pi): load models.json custom providers and surface SDK error messages

- Switch ModelRegistry.inMemory() to ModelRegistry.create() so custom
  providers from ~/.pi/agent/models.json (ollama, LM Studio, etc.)
  resolve correctly. Both return the same mutable class supporting
  registerProvider() for extensions.
- Surface Pi's AssistantMessage.errorMessage in the result chunk errors
  array so dag-executor shows actual errors instead of generic
  'SDK returned error'.
- Add ollama/qwen3.5 node to archon-test-pi workflow to validate both
  extension (kiro) and models.json (ollama) provider paths.

* fix(pi): address Wirasm PR review — comment clarity, session cleanup, test mock alignment

- event-bridge: document intentional design of yielding error chunks (isError:true
  propagates in chunk, not via throw); fix event name to pi.result_chunk_error;
  remove noisy model field from error log
- provider: replace verbose multi-paragraph step-numbered comments with concise
  WHY explanations; fix stale ModelRegistry.inMemory() reference (code uses create());
  add session.dispose() on model-not-found throw and setModel() failure to close
  the session cleanup gap
- provider.test: align mock from ModelRegistry.inMemory → ModelRegistry.create to
  match actual provider code (was already broken on the branch); update test
  description and comments to describe behavior rather than step numbers

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>

* test(pi): add happy-path test for deferred extension model resolution

Verifies that when find() returns undefined on the first call (static catalog miss)
and a model on the second call (after bindExtensions()), session.setModel() is
invoked with the resolved model and no error is returned.

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>

---------

Co-authored-by: jhegeman-ds <[email protected]>
Co-authored-by: Claude Sonnet 4.6 <[email protected]>

* fix(workflows): preserve completed node state across DAG multi-resume cycles (#1530)

* fix(workflows): preserve completed node state across DAG multi-resume cycles (#1520)

Bug: getCompletedDagNodeOutputs only queried 'node_completed' events, but
resumed runs emit 'node_skipped_prior_success' for already-completed nodes.
On a second resume, previously completed nodes were re-executed because
their skip events were invisible to the query.

Fix:
- Query both 'node_completed' and 'node_skipped_prior_success' event types
  in getCompletedDagNodeOutputs so multi-resume correctly identifies all
  previously completed nodes
- Store original node_output in node_skipped_prior_success event data so
  the output is available for $nodeId.output substitution on next resume
- Improve error messaging when all nodes fail: name the specific failed
  nodes and count skipped downstream nodes instead of the generic
  'no successful nodes' message

Closes #1520

* test: address review feedback — assert node_output, test failure message format

- Assert node_output field in node_skipped_prior_success event test
- Add test for empty-string fallback when node ID has undefined output
- Add dedicated test verifying failure message names failing nodes
- Update stale comment from 'no successful nodes' to new behavior
- Add edge-case test for only node_skipped_prior_success rows (no node_completed)

* fix(adapters): place webhook clones in workspace source/ subdirectory (#1554)

The Gitea, GitHub, and GitLab adapters constructed the canonical clone
path manually as `getArchonWorkspacesPath() + owner + repo`, which
nested the cloned repo at the project root. The CLI clone handler
uses `getProjectSourcePath(owner, repo)` (returning
`workspaces/owner/repo/source/`) and `ensureProjectStructure(owner, repo)`
so that `worktrees/`, `artifacts/`, and `logs/` live as siblings of the
cloned repo. The webhook adapters bypassed this, so the first webhook
clone left the workspace in a layout the rest of the system did not
expect, breaking worktree creation and command discovery.

Replace the manual path construction with `getProjectSourcePath` and
call `ensureProjectStructure` immediately before the clone in all three
adapters. For GitLab nested namespaces (group/subgroup/repo), the leaf
segment becomes the repo and the remaining path becomes the owner,
which matches how the workspace tree treats nested groups.

Closes #1547

* refactor(workflows): consolidate duplicated safeSendMessage into executor-shared (#1496)

* refactor(workflows): consolidate safeSendMessage into executor-shared

executor.ts and dag-executor.ts each defined a near-identical copy of
safeSendMessage and SendMessageContext. Move both into executor-shared.ts
as exports and import from there. The UnknownErrorTracker guard is also
relocated and exported, but remains dormant infrastructure: no callsite
in either file instantiates a tracker today, so this PR delivers pure
deduplication with no behavioral change. Activating the guard in DAG
execution is appropriate follow-up work.

Also addresses two issues surfaced during code review (#1496):
- Reset UnknownErrorTracker.count on any non-UNKNOWN error (was only
  resetting on success), so a sequence like UNKNOWN -> TRANSIENT ->
  UNKNOWN -> UNKNOWN no longer trips the threshold incorrectly.
- Use the structured 'platform.critical_message_send_failed' event name
  in the inlined log call instead of the prose 'Critical message send
  failed' string.

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

* test(workflows): add unit tests for safeSendMessage in executor-shared

Covers the six cases requested in the PR review:
- Success resets UnknownErrorTracker to 0 and returns true
- TRANSIENT error returns false without throwing
- FATAL error rethrows with 'Platform authentication/permission error'
- UNKNOWN error increments tracker and returns false below threshold
- Three consecutive UNKNOWN errors trip the threshold and throw
- TRANSIENT resets tracker so a subsequent UNKNOWN does not trip threshold
- Calls without unknownErrorTracker (DAG executor path) never throw

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <[email protected]>

* fix(web): surface workflow-def fetch error in execution graph (#1683) (#1698)

* fix(web): surface workflow-def fetch error in execution graph

WorkflowExecution dropped error/pending state from the workflowDefinition
useQuery, so the right-pane graph rendered "Loading graph..." indefinitely
when the fetch failed (issue #1683 example: graph build never finishes,
spinner loops forever).

Capture error and isPending, mirror the existing run-query message
conversion, and split the fallback into three branches:

  graph ready   -> WorkflowDagViewer
  fetch error   -> Failed to load workflow graph + message
  pending       -> spinner
  neither       -> "Workflow graph unavailable for this run."

The empty branch covers runs where workflowName resolves but the server
has no nodes (older runs, deleted workflow defs).

* fix(web): add retry button to workflow-def graph error state

With staleTime: Infinity the cached error stays put until the page
reloads. Reuses the existing queryClient and the same queryKey the
useQuery declares, so invalidateQueries triggers a fresh fetch.

* fix(web): use resetQueries with catch on retry, comment empty fallback

Address review on #1698:
- Retry button now calls queryClient.resetQueries() instead of
  invalidateQueries(). resetQueries clears the cached error state
  before refetching, which is the idiomatic React Query pattern
  for a user-triggered retry.
- Wrap the returned Promise in .catch() so a queryClient failure
  is surfaced via console.error rather than silently swallowed.
- Add a brief comment above the final 'unavailable' fallback so
  future maintainers don't conflate it with the pending branch.

* fix(adapters): bump telegramify-markdown to 1.3.3 for blockquote escaping (#1340)

* fix(adapters): bump telegramify-markdown to 1.3.3 for blockquote escaping

telegramify-markdown 1.3.2 escapes the `>` blockquote marker (which
Telegram MarkdownV2 supports natively) and double-escapes any other
special character on the same line, so any blockquote ending in a
period (or containing `.`, `!`, `-`, etc.) is rejected by the Bot
API with "Character '.' is reserved and must be escaped". The bot
falls back to plain text and logs telegram.markdownv2_failed.

Upstream fixed both bugs in 1.3.3. Bumping the floor to ^1.3.3 and
adding a regression test on the canonical "> hi." case so the
behaviour can't silently regress if the dependency loosens later.

Fixes #1102

* test(adapters): broaden blockquote regression with multi-char and multi-line cases

Addresses review nit on #1340. 1.3.2's bug regressed both on multiple
special characters on one line and on multi-line blockquotes, so pinning
those shapes down adds real confidence against a future re-regression.

Expected outputs verified against telegramify-markdown 1.3.3:
- `> a.b-c!` escapes `.`, `-`, `!` exactly once, `>` marker unescaped.
- `> first.\n> second?` escapes `.` once, `?` passes through (not in
  Telegram MarkdownV2's reserved set), `>` marker unescaped on both lines.

* test(adapters): reframe blockquote regression comment around bug behavior

Address review on #1340: drop version-number framing in favor of
describing the bug behavior. Version numbers rot as the floor moves;
the bug behavior plus the issue link stays stable.

* fix(clone): resolve forge auth via configured *_URL env vars (fixes #1704) (#1706)

resolveForgeAuth() only matched forges by exact hostname or hostname
label. Self-hosted instances with non-standard hostnames (e.g.
git.example.com) were silently ignored even when GITEA_URL pointed to
the same host.

Add a third fallback step that compares the clone URL hostname against
configured GITEA_URL, GITLAB_URL, and FORGEJO_URL env vars. This uses
strict hostname equality so tokens are never leaked to unrelated hosts.

* chore(workflows): drop CHANGELOG.md from maintainer-review docs-impact scope (#1724)

The maintainer-review-pr workflow's docs-impact reviewer has been flagging
"missing CHANGELOG entry" at MEDIUM (and HIGH) on multiple PRs since we
started using it. The project doesn't follow per-PR CHANGELOG maintenance —
the `/release` skill generates entries from squash-commit history when
cutting a release, so contributors writing per-PR entries would create
duplicate work and merge conflicts.

Removes:
- the "Migration → CHANGELOG.md" bullet from the per-change analysis list
- `CHANGELOG.md` from the "specific places to check" enumeration
- "changelog entry" from the MEDIUM severity bucket heading

Adds an explicit callout that CHANGELOG.md is out of scope for this
review, with a one-line explanation of where it gets generated. Keeps the
rest of docs-impact behavior unchanged — public APIs, CLI flags, env vars,
and user-facing behavior changes are still in scope across the docs site
and CLAUDE.md.

Project-local command file (`.archon/commands/`), loaded from disk per
run, so the change takes effect on the next maintainer-review-pr
invocation.

* fix(workflows): write large node outputs to temp file to prevent bash substitution corruption (fixes #1717) (#1718)

* fix(workflows): write large node outputs to temp file to prevent bash substitution corruption (#1717)

When a bash node references $nodeId.output from an upstream node whose
output exceeds ~32KB, inlining the full value as a bash -c argument
causes silent data corruption. This adds a size threshold
(NODE_OUTPUT_FILE_THRESHOLD = 32KB): outputs below it are still
shell-quoted inline; outputs at or above it are written to a temp file
in logDir and substituted with $(cat '<path>') so bash reads the value
at runtime without argv size issues.

Affected paths: executeBashNode and loop-node until_bash.

Closes #1717

* fix(workflows): wrap shellQuoteOrFile writeFileSync in try/catch with fallback

Address review feedback from @Wirasm:
- Wrap writeFileSync in try/catch so disk-full or permission errors
  produce a structured log instead of an unhandled exception
- Fall back to inline shell-quoting on failure (pre-file-spill behavior)
- Add test for fallback path using a non-existent directory

Signed-off-by: kagura-agent <[email protected]>

---------

Signed-off-by: kagura-agent <[email protected]>

* fix(providers/codex): fresh AbortController per retry attempt (#1266) (#1371)

* fix(providers/codex): create a fresh AbortController per retry attempt

Fixes #1266. The codex provider's retry loop reused the caller's
AbortSignal across every attempt. When attempt N's Codex subprocess
crashes, Node's `spawn({ signal })` linkage aborts the shared signal
as part of SIGTERM'ing the dying child. On attempt N+1, `runStreamed`
passes that same (already-aborted) signal into the next `spawn`,
which SIGTERMs the freshly-spawned child before it reads any input.
The "Reading prompt from stdin..." line in the resulting error is
Codex CLI's normal startup banner, not a crash locus.

The fix: signal assignment moves out of buildTurnOptions and into
the retry loop. Each attempt gets a brand-new AbortController; the
caller's signal (if provided) is chained in via a once-listener so
cancellation still propagates. A try/finally removes the listener
and aborts the per-attempt controller once the attempt terminates.

Regression tests:

- `retry after crash receives a fresh (non-aborted) AbortSignal`
  captures the signal passed to `runStreamed` at call-time (not by
  mock.calls reference, which would see the mutated .signal after
  reassignment) and asserts attempt 1 got a distinct, non-aborted
  signal.
- `caller abort forwards into the active per-attempt signal` aborts
  the caller mid-attempt and asserts the per-attempt signal observes
  it.
- Two existing tests updated: `buildTurnOptions` no longer attaches
  the caller's signal, so both "passes signal in TurnOptions" tests
  now assert presence of an AbortSignal without identity-equality
  against the caller's.

Without the fix, these four tests fail and the rest pass (47/51).
With the fix, all 51 pass.

Out of scope: the binary HTTP timeout class-B path in the issue.

* fix(providers/codex): synchronous abort check at stream entry, plus review-pass fixes

`streamCodexEvents` now checks `abortSignal?.aborted` before entering
the `for await` so a caller abort that lands between attempt setup and
the first event surfaces immediately instead of waiting on the next
event or end-of-stream. The existing between-events check is retained.

Also from the same review pass:
- Pi shim: wrap `mkdirSync`/`writeFileSync` in try/catch so EACCES/
  ENOSPC surfaces as a classified "Pi shim setup failed at <dir>"
  instead of a raw node:fs error.
- Codex retry path: `getLog().debug` before throwing the
  model-access error from a retry-attempt `startThread`; the outer
  query_error log only runs for retryable errors.
- Docs: ai-assistants.md and configuration.md updated for Claude
  `~/.local/bin/claude` autodetect; ai-assistants.md gains a Codex
  autodetect bullet listing the five probed paths.
- Tests: `homedir()` instead of `process.env.HOME ?? '/Users/test'`
  to match the implementation; Windows autodetect probe covered;
  config-over-autodetect precedence covered.

* fix(core): resolve default assistant via config + folder detection on every codebase registration (#1729)

* fix(core): resolve default assistant via config + folder detection on every codebase registration

## Summary
- Extract `resolveDefaultAssistant(repoPath)` helper into `packages/core/src/config/resolve-assistant.ts` with precedence: `.codex` / `.claude` folder → `loadConfig().assistant` → first built-in provider → `'claude'`.
- Call the helper from `clone.ts` (replacing the inline block) and from the three forge adapters (`github`, `gitlab`, `gitea`) which previously passed no `ai_assistant_type` and silently defaulted to `'claude'` regardless of the configured assistant.
- `createCodebase` stays a thin DB function with the simple `?? 'claude'` fallback. No dynamic-import config-loading inside the DB layer.
- Lazy-load `@archon/providers` inside the helper so the resolve module doesn't pull provider SDK chains at every adapter import site (which previously broke adapter tests that mock `@archon/paths` without `BUNDLED_IS_BINARY`).
- New `resolve-assistant.test.ts` uses `spyOn` (not `mock.module`) for `loadConfig` and `getRegisteredProviders` so the spies cleanly `mockRestore()` and do not pollute `config-loader.test.ts` running in the same batch.
- `config-loader.test.ts` switches its file I/O mocks from `mock.module('./config-loader', ...)` to `mock.module('fs/promises', ...)` for cross-Bun-version compatibility.
- New `clone.test.ts` cases verify the configured-provider and loadConfig-failure fallbacks via the integration path.

Fixes #1580.

## Test plan
- [x] `bun test src/db/adapters/sqlite.test.ts src/db/codebases.test.ts ... src/config/ src/state/` — exact CI batch, 366 pass / 0 fail
- [x] `bun --filter @archon/core --filter @archon/adapters --filter @archon/server test` — only pre-existing macOS-only telegram-markdown failures (unrelated, present on dev)
- [x] `bun run type-check`, `bun run lint`, `bun run format:check` all clean

* fix(core): also lazy-load loadConfig in resolve-assistant

config-loader.ts eagerly imports @archon/providers (for runtime validation
of the configured assistant ID against the registry), which transitively
loads claude/codex binary-resolvers and their BUNDLED_IS_BINARY dependency
on @archon/paths. Static-importing loadConfig at module top therefore
forces every caller of resolve-assistant — including the three forge
adapters — to pull that chain too. Adapter tests that mock @archon/paths
without BUNDLED_IS_BINARY then break on Linux.

Move the loadConfig import to a dynamic import inside the function body
alongside the getRegisteredProviders one. Nothing in this module is
loaded eagerly anymore.

* fix(core): pass repoPath to loadConfig in resolve-assistant

loadConfig() without a path only merges the global config; the repo's
own .archon/config.yaml (which can set assistant: pi at the project
level) is silently skipped. Pass repoPath so repo-level config is
honored during registration. Add a call-contract assertion in
resolve-assistant.test.ts so a future regression that drops the path
is caught.

Surfaced by CodeRabbit review on #1729.

* fix(adapters): import resolve-assistant via deep subpath to avoid config-loader chain

@archon/core/config/index.ts does `export * from './config-loader'`, which
forces eager loading of config-loader.ts (and its top-level @archon/providers
import) at every site that imports from @archon/core/config. The three forge
adapters were importing the helper through that barrel, which pulled in the
binary-resolver chain and broke @archon/adapters tests on Linux (mocked
@archon/paths without BUNDLED_IS_BINARY).

Add a dedicated './config/resolve-assistant' subpath export and switch the
three forge adapters to it. Only resolve-assistant.ts is loaded — no
transitive @archon/providers at module-load time.

* fix(providers/claude): reject directory paths and expand npm package dirs (#1723) (#1737)

* fix(providers/claude): reject directory paths and expand npm package dirs

The Claude binary resolver validated configured paths with existsSync, which
returns true for directories. Users on Windows who installed Claude Code via
npm and configured claudeBinaryPath to the npm platform-package directory
(e.g. ...\@anthropic-ai\claude-code-win32-x64) hit a confusing SDK-side
ReferenceError ("Claude Code native binary not found at <path>") because the
SDK's child_process.spawn(directory) failed with ENOENT.

Replace the existence-only check with a pathKind() helper that distinguishes
file / directory / missing, and transparently expand a configured directory
to the platform-appropriate child executable (claude.exe on Windows, claude
on Unix) when present. A directory without the expected binary now produces
a directory-specific error that tells the user what to fix.

The autodetect branch already targets a file path directly and is unchanged.

Fixes #1723

* fix(providers/claude): address self-review — broken-symlink test + codex TODO

- Add a regression test for pathKind() returning 'missing' on a broken
  symlink (uses a real tmp symlink so the statSync ENOENT path is actually
  exercised, not mocked).
- Add a TODO marker in the Codex resolver pointing at #1723. The Codex
  resolver has the identical existsSync-on-directory gap; left unfixed in
  this PR to avoid scope creep but now discoverable from the file itself
  when a Codex bug report lands or someone does a deliberate parity pass.

* fix(providers/claude): address review — autodetect parity, EACCES breadcrumb, doc updates

Extends #1723 fix per multi-agent PR review:

- Autodetect branch now uses pathKind === 'file' instead of fileExists so
  a directory at ~/.local/bin/claude no longer slips past validation and
  crashes the SDK as ENOENT (matches the env/config branches).
- pathKind catches now distinguish ENOENT/ENOTDIR from other stat errors
  (EACCES, ELOOP, etc.) and emit a WARN log line with the error code so
  operators have a triage breadcrumb for permission issues that would
  otherwise surface as the misleading "file does not exist".
- Extract CLAUDE_BINARY_NAME constant (was duplicated 7 times across
  source + tests) and export PathKind type so test mockReturnValue calls
  are type-checked against the union rather than being unknown strings.
- Inline expandDirectoryToExecutable into validateAndExpand — single
  caller, body shorter than its JSDoc. Drop the WHAT-restating first
  sentence of validateAndExpand's docstring.
- Strip the "Wrapped for spyOn parity" clause from pathKind's JSDoc —
  contradicted the accurate first sentence and implied the design was
  testability-driven rather than classification-driven.
- Align spy declarations to `| undefined` in binary-resolver.test.ts to
  match the dev-mode file. Drop the now-unused fileExistsSpy.
- Add pathKind happy-path tests (real file → 'file', real dir →
  'directory'). Without these, a typo like isFile() → isDirectory()
  would pass every existing test because all resolver tests spy through
  pathKind and never exercise the real statSync logic.
- Add two dev-mode tests for CLAUDE_BIN_PATH-as-directory. The env
  branch runs validateAndExpand before the BUNDLED_IS_BINARY guard, so
  dev users get expansion too; pin the contract.
- Add a Windows autodetect-rejects-directory regression test.

Docs: surface the new directory-accepting behavior so Windows users who
install via npm can discover it without re-reading the source.

* docs(direction): add community-providers policy section (#1736)

Capture the acceptance criteria and maintenance policy for community
providers in direction.md so PR triage stops devolving into ad-hoc
'should this match Pi or not' debates.

Policy in brief:
- Coding-agent SDK required (no raw chat.completions wrappers — Pi
  already covers ~20 LLM backends via one harness)
- Match the Pi pattern: provider class + options translator + event
  bridge + capability matrix, registered with builtIn: false, tests
  at parity with the Pi suite, docs page in ai-assistants.md
- No cap on acceptance
- Contributor + community maintain; non-functional providers get
  deprecated and removed in the next minor unless someone fixes them

Cite as direction.md §community-providers when triaging.

* fix(providers/codex): remove stale attemptController.abort() that crashes after SDK cleanup (#1735) (#1739)

The codex-sdk's own finally calls child.removeAllListeners() + child.kill()
before Archon's retry-loop finally runs. The subsequent attemptController.abort()
fires Node's internal spawn-signal abort listener on the now-listenerless child,
surfacing an uncaught AbortError that bypasses try/catch.

The per-attempt AbortController is short-lived and goes out of scope at iteration
end — no explicit abort() cleanup is needed. Caller signal cancellation is
unaffected (removed via removeEventListener in the same finally block).

Closes #1735

* feat(workflows): add always_run node opt-out for resume caching (closes #1391) (#1730)

* feat(workflows): add always_run node opt-out for resume caching

Closes #1391.

Adds an optional `always_run: boolean` field on every DAG node. When
`true`, the node re-executes on resume even if it completed in the
prior run. The resume pre-populate filters out always_run node IDs,
and the per-node skip-check is gated by `!node.always_run`.

Use case: producers whose exit code does not validate their output
(bash that writes a file the consumer parses, code generators, fetch
scripts). Today a successful-but-garbage producer stays cached across
every resume; the only escape is renaming the node.

Default is unchanged. Normal cached nodes in the same run still skip.
Emits a new `dag.node_always_run_resume_forced` log event so operators
can see the flag firing.

* workflows: emit node_always_run_reset event on resume opt-out

The always_run resume-forced path only wrote a structured log line.
The prior_success skip path writes a DB workflow_event, so resume
forensics could see skipped nodes but not nodes that were reset from
the skip list. Add a symmetric node_always_run_reset event with the
prior output so operators can reconstruct resume decisions from the
workflow_events table.

Drop the trailing PR reference from the comment — surrounding text
explains intent.

* fix(workflows): ensure workflow-builder injects $ARGUMENTS in generated YAMLs (#1733)

Fixes #1535

The workflow-builder's generate-yaml node did not explicitly require
generated workflows to reference $ARGUMENTS (or $USER_MESSAGE). When
the AI generated single-node workflows that accept user input, it
described the input in prose but omitted the $ARGUMENTS substitution
variable. The harness captured the user's invocation message but never
injected it into the node's conversation.

Changes:
- Add rule 13 to generate-yaml prompt: every workflow that accepts user
  input MUST reference $ARGUMENTS in at least one node prompt
- Add validation warning in validate-yaml when neither $ARGUMENTS nor
  $USER_MESSAGE appears in the generated YAML
- Regenerate bundled defaults

* fix(workflows): persist read-only node outputs via bash bridges in archon-refactor-safely (#1734)

The analyze-impact and plan-refactor nodes are intentionally read-only
(denied_tools: [Write, Edit, Bash]) but their prompts instructed the AI
to write files. This caused the AI to waste turns searching for
unavailable tools, and the plan/analysis was never persisted to disk.

The execute-refactor node then failed to read the plan file, resulting
in zero work done despite the workflow reporting completed.

Changes:
- Update prompts to output analysis/plan directly (captured as node
  output) instead of attempting file writes
- Add persist-impact and persist-plan bash nodes to bridge the context
  boundary by writing node outputs to $ARTIFACTS_DIR files
- Update dependency chain: plan-refactor depends on persist-impact,
  execute-refactor depends on persist-plan

Closes #1477

* fix(providers): expand ${VAR_NAME} brace syntax in MCP config env vars (#1728)

* fix(providers): expand ${VAR_NAME} brace syntax in MCP config env vars (fixes #1612)

Add two-group regex alternation to expandEnvVarsInRecord so both $VAR and
${VAR} forms are expanded in env/headers values. Add 5 tests for the new
brace-form behavior and update MCP servers docs.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

* chore(ai-layer): evolve AI Layer from PIV run

---------

Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]>

* fix(cli): use source checkout cwd for workflow discovery on resume/approve/reject (#1743)

* Fix: workflow approve/resume discovery for worktree runs (#1663)

When a workflow paused at an approval gate is resumed via `workflow approve` or
`workflow resume`, the CLI re-invoked `workflowRunCommand` with `run.working_path`
as the discovery cwd. If `working_path` is a worktree or workspace clone that
does not contain the user's local (often untracked) workflow YAML, discovery
failed with "Workflow 'foo' not found" before execution could begin.

Separate the discovery path from the execution path by adding an optional
`discoveryCwd` to `WorkflowRunOptions`. Resume, approve, and reject now look up
the codebase and pass `codebase.default_cwd` as `discoveryCwd`, so the source
repo is searched even when `working_path` lives elsewhere. The execution cwd
and the existing `findResumableRun` keying are unchanged.

Changes:
- Add `WorkflowRunOptions.discoveryCwd`; use it for `loadWorkflows` in
  `workflowRunCommand`
- `workflowResumeCommand`, `workflowApproveCommand`, and `workflowRejectCommand`
  resolve `codebase.default_cwd` (with graceful fallback) and pass it through
- Tests covering discovery from `codebase.default_cwd` and fallback to
  `working_path` when no codebase is available

Fixes #1663

* chore(workflows): regenerate bundled defaults after default YAML updates

* fix: address review findings from PR #1743

- C1: Remove Write from denied_tools on analyze-impact and plan-refactor nodes
  in archon-refactor-safely.yaml — prompts write to $ARTIFACTS_DIR/*.md
- H1: Add else branch with warn log when codebase record not found (null return)
  at all three discoveryCwd sites (resume/approve/reject)
- H2: Log discovery path when discoveryCwd is set so the searched path is
  visible to users debugging workflow-not-found errors
- I1: Add two regression tests for workflowRejectCommand discoveryCwd path
  (codebase found and fallback-when-null), mirroring approve/resume parity
- Fix mock pollution: remove duplicate getWorkflowRun mockResolvedValueOnce
  in "throws when on_reject configured but working_path is null" test whose
  extra queued value leaked into subsequent tests
- L3: Drop caller enumeration from discoveryCwd JSDoc; keep only the why
- L4: Update codebaseId inline comment to include reject as a caller
- L6: Fix workflowRejectCommand JSDoc to describe the auto-resume branch
- M1: Add CHANGELOG entry for the #1663 fix under [Unreleased]
- M2: Rename stale test name "fall through to auto-registration" to accurately
  describe the warn-and-fallback behavior on getCodebase failure
- Regenerate bundled-defaults.generated.ts after YAML changes

* simplify: merge redundant priorCompletedNodes checks into single if/else

* feat(marketplace): add piv-system-evolution

* fix(workflows): harden marketplace decide node against non-JSON ai-review output

* fix(web/chat): wrap long unbreakable strings in chat message bubbles (fixes #1738) (#1742)

User-bubble <p> and the .chat-markdown typography rules had no
overflow-wrap, so long URLs and tokens broke out of the max-w-[70%]
container.

- MessageBubble: add break-words + min-w-0 to the flex-1 paragraph so
  it can shrink below intrinsic content width.
- index.css: add overflow-wrap: break-word to .chat-markdown p, li, td,
  and a. Code blocks already use overflow-x-auto and are excluded.

* docs(brand): add brand foundation page on archon.diy (#1745)

* docs(brand): add brand foundation page on archon.diy

- Mount the canonical Archon brand sheet at `/brand/` in the docs site
  (Penpot-exported standalone HTML, top-right "Console →" cross-link
  surgically removed via a re-runnable patch script).
- Add a Starlight overview page with a Quick reference (gradient,
  surface) and an embedded full brand sheet.
- Sidebar gains a "🎨 Brand" entry between Roadmap and The Book of Archon.
- Fix the dark-mode active sidebar link being unreadable
  (`color: var(--sl-color-white)`).
- Require future UI changes to align with the brand foundation
  (new "UI and Visual Design" section in root CLAUDE.md).

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

* docs(brand): switch foundation.html to plain source files, drop decoder scripts

The brand sheet now ships as plain Penpot-exported source (Brand.html shell,
brand-app.jsx, logo.jsx, tweaks-panel.jsx, standalone-tweaks-toggle.jsx,
app.css, archon-logo.png) and is edited like any other code in the repo:
open the JSX, change it, refresh the page.

- public/brand/foundation.html now loads React + Babel from unpkg (with
  integrity hashes) and compiles the JSX in the browser. Adds one local
  override: hide the Penpot Tweaks toggle on the public site.
- brand-app.jsx carries our single local delta: the top-right "Console →"
  cross-link is removed (the sibling Archon Console doc isn't published).
- public/brand/README.md documents what each file owns and the local delta.
- The 1.5 MB self-extracting bundle and the scripts/brand/ decoder pipeline
  (_find-console.ts, _dump.ts, _patch.ts) are deleted. Net: the repo loses
  ~1.5 MB of opaque base64 + 4 maintenance scripts; gains ~85 KB of editable
  source.

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <[email protected]>

* fix(web): recognize loop and approval node types in DAG builder (#1744)

* fix(web): recognize loop and approval node types in DAG builder

resolveNodeDisplay() fell through to the 'prompt' fallback for loop and
approval nodes, giving them nodeType='prompt' with no promptText.
useBuilderValidation then raised false-positive "prompt cannot be empty"
errors for both node types.

Changes:
- dag-layout.ts: add loop and approval cases to resolveNodeDisplay()
- DagNodeComponent.tsx: extend nodeType union; add TYPE_CONFIG entries
  and getContentPreview cases for loop and approval
- index.css: add --node-loop (teal) and --node-approval (amber) tokens

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>

* test(web): add unit and integration tests for loop/approval DAG node types

Tests requested by Wirasm for PR #1722:
- resolveNodeDisplay(): loop node → { label, nodeType, promptText }, approval → { label, nodeType }
- dagNodesToReactFlow() integration: asserts loop and approval nodes have correct nodeType in output
- getContentPreview(): loop multi-line prompt returns first line; approval returns empty string
- Exports getContentPreview from DagNodeComponent.tsx to make it testable
- Extends test script to cover src/components/

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>

---------

Co-authored-by: robby_kei <[email protected]>
Co-authored-by: Claude Sonnet 4.6 <[email protected]>

* feat(providers): add GitHub Copilot community provider  (#1505)

* feat(providers): add GitHub Copilot provider configuration types
Define CopilotProviderDefaults with model, reasoning effort, and auth options
Include system message injection and CLI path configuration support

* feat(providers): add GitHub Copilot community provider integration
Implement full provider with session management, streaming, and binary resolution
Include comprehensive test coverage and lazy-load SDK pattern

* feat(providers): add Copilot provider registration and exports
Export CopilotProvider, config parser, and binary resolver utilities
Register Copilot provider in community providers initialization

* test(e2e): add GitHub Copilot provider smoke and abort tests
Include streaming verification, token validation, and interrupt handling
Verify connectivity, output plumbing, and session management

* feat(copilot): add reasoning effort alias and session timeout improvements
Map Archon `max` effort to SDK `xhigh` and extend sendAndWait timeout to 60min
Handle fork-session requests with fresh session creation fallback

* feat(copilot): add environment variable override support and auto model default
Add COPILOT_MODEL env var with envOverrides tracking across config system
Update provider to default model to 'auto' and enhance settings UI

* docs(copilot): clarify session option handling comment

* feat(copilot): add MCP, skills, agents, and structured output support

Implement full Copilot SDK feature translation including tool restrictions,
session config assembly, and best-effort JSON parsing for structured output

* feat(copilot): respect useLoggedInUser to override env token
test(copilot): cover env token precedence and override behavior

* refactor(copilot): remove isCopilotModelCompatible and model-ref
delete model-ref.ts and model-ref.test.ts
update copilot index and registration to drop isCopilotModelCompatible export

* fix(struct-out): enforce object requirement for structured output parsing
return undefined if parsed JSON is not an object
add tests covering non-object JSON in structured output parsing

* feat(copilot): add isExecutableFile check for Copilot binary
implement isExecutableFile using stat/access and use it in path resolution
update errors to reference executable file and chmod guidance

* feat(copilot): add PATH lookup for copilot binary resolution
export resolveFromPath and prefer PATH result when executable

* ci(workflows): migrate and add Copilot CI workflows
- rename e2e-copilot-abort.yaml to test-workflows/e2e-copilot-abort.yaml
- add e2e-copilot-all-features.yaml and relocate smoke workflow to test-workflows

* refactor(shared): centralize structured-output parsing and skills
update providers to re-export shared implementations
expose shared utilities: tryParseStructuredOutput, augmentPr…
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.

Chat message text overflows outside the message container/card.

2 participants