feat(slack): add message reactions to track conversation status#1475
Open
KiranChilledOut wants to merge 6 commits intocoleam00:devfrom
Open
feat(slack): add message reactions to track conversation status#1475KiranChilledOut wants to merge 6 commits intocoleam00:devfrom
KiranChilledOut wants to merge 6 commits intocoleam00:devfrom
Conversation
Add emoji reactions to Slack messages to give users visual feedback on conversation state: - 👀 (eyes) when message is acknowledged - 🔄 (arrows_counterclockwise) when work begins - Clean up intermediate reactions before final status - ✅ (white_check_mark) on success - ❌ (x) on failure The addReaction/removeReaction methods are optional in IPlatformAdapter to maintain backwards compatibility with other platforms. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds platform reaction APIs and Slack adapter reaction methods; integrates reaction lifecycle into the orchestrator with safe wrappers; extends adapter types; enables clearing persisted assistant session IDs; plus broad workflow/tooling updates, tests, docs, and multiple package/version bumps. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Orchestrator
participant SlackAdapter
participant SlackAPI as Slack API
User->>Orchestrator: posts message (channel:timestamp)
Orchestrator->>SlackAdapter: safeAddReaction("channel:ts","eyes")
SlackAdapter->>SlackAPI: reactions.add(channel, timestamp, reaction)
SlackAPI-->>SlackAdapter: success / error
SlackAdapter-->>Orchestrator: resolved
Orchestrator->>Orchestrator: start AI work
Orchestrator->>SlackAdapter: safeAddReaction("channel:ts","arrows_counterclockwise")
SlackAdapter->>SlackAPI: reactions.add
SlackAPI-->>SlackAdapter: success
Note over Orchestrator: on success or failure
Orchestrator->>SlackAdapter: safeRemoveReaction("channel:ts","arrows_counterclockwise")
SlackAdapter->>SlackAPI: reactions.remove
SlackAPI-->>SlackAdapter: success
Orchestrator->>SlackAdapter: safeAddReaction("channel:ts","white_check_mark" or "x")
SlackAdapter->>SlackAPI: reactions.add
SlackAPI-->>SlackAdapter: success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/adapters/src/chat/slack/adapter.ts (1)
210-250: Use guideline-compliant log event names for reaction lifecycle events.The new event keys (
slack.reaction_added,slack.reaction_failed, etc.) don’t follow the required{domain}.{action}_{state}convention with standard states.♻️ Suggested log key adjustments
- getLog().warn({ conversationId, reaction }, 'slack.reaction_invalid_conversation_id'); + getLog().warn({ conversationId, reaction }, 'slack.reaction_add_rejected'); ... - getLog().debug({ channelId, timestamp, reaction }, 'slack.reaction_added'); + getLog().debug({ channelId, timestamp, reaction }, 'slack.reaction_add_completed'); ... - getLog().warn({ err, channelId, timestamp, reaction }, 'slack.reaction_failed'); + getLog().warn({ err, channelId, timestamp, reaction }, 'slack.reaction_add_failed'); ... - getLog().warn({ conversationId, reaction }, 'slack.remove_reaction_invalid_conversation_id'); + getLog().warn({ conversationId, reaction }, 'slack.reaction_remove_rejected'); ... - getLog().debug({ channelId, timestamp, reaction }, 'slack.reaction_removed'); + getLog().debug({ channelId, timestamp, reaction }, 'slack.reaction_remove_completed'); ... - getLog().warn({ err, channelId, timestamp, reaction }, 'slack.remove_reaction_failed'); + getLog().warn({ err, channelId, timestamp, reaction }, 'slack.reaction_remove_failed');As per coding guidelines "Structured logging with Pino: use event naming format
{domain}.{action}_{state}(standard states: _started, _completed, _failed, _validated, _rejected)".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/adapters/src/chat/slack/adapter.ts` around lines 210 - 250, The log event names for the reaction lifecycle don't follow the {domain}.{action}_{state} convention; update all getLog() calls in addReaction and removeReaction to use standardized keys: replace slack.reaction_invalid_conversation_id -> slack.reaction_add_rejected, slack.reaction_added -> slack.reaction_add_completed, slack.reaction_failed -> slack.reaction_add_failed; and for removal replace slack.remove_reaction_invalid_conversation_id -> slack.reaction_remove_rejected, slack.reaction_removed -> slack.reaction_remove_completed, slack.remove_reaction_failed -> slack.reaction_remove_failed; keep the same metadata objects and places (inside async addReaction and removeReaction, and the catch blocks) so only the event key strings change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/orchestrator/orchestrator-agent.ts`:
- Around line 547-549: The call to platform.addReaction? (e.g., await
platform.addReaction?.(conversationId, 'eyes')) should not be awaited directly
because adapter errors can bubble up and break core message handling; make
reaction updates fire-and-forget and ensure they run (or are attempted) in a
finally block so intermediate reactions aren't left behind—wrap calls to
platform.addReaction? and any other optional reaction hooks used in the file in
try/catch that logs errors but does not throw, or invoke them without await
(e.g., start a Promise and ignore its rejection) and move these invocations into
a finally section of the surrounding handler (references: platform.addReaction?,
conversationId, any reaction hook calls around the message handling functions)
so reactions are resilient and non-blocking.
---
Nitpick comments:
In `@packages/adapters/src/chat/slack/adapter.ts`:
- Around line 210-250: The log event names for the reaction lifecycle don't
follow the {domain}.{action}_{state} convention; update all getLog() calls in
addReaction and removeReaction to use standardized keys: replace
slack.reaction_invalid_conversation_id -> slack.reaction_add_rejected,
slack.reaction_added -> slack.reaction_add_completed, slack.reaction_failed ->
slack.reaction_add_failed; and for removal replace
slack.remove_reaction_invalid_conversation_id -> slack.reaction_remove_rejected,
slack.reaction_removed -> slack.reaction_remove_completed,
slack.remove_reaction_failed -> slack.reaction_remove_failed; keep the same
metadata objects and places (inside async addReaction and removeReaction, and
the catch blocks) so only the event key strings change.
🪄 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
Run ID: 83541177-42a1-4dad-a558-33ec2a4a9882
📒 Files selected for processing (6)
packages/adapters/src/chat/slack/adapter.test.tspackages/adapters/src/chat/slack/adapter.tspackages/cli/src/commands/setup.test.tspackages/cli/src/commands/setup.tspackages/core/src/orchestrator/orchestrator-agent.tspackages/core/src/types/index.ts
Collaborator
Review SummaryVerdict: minor-fixes-needed Your PR adds Slack emoji reaction feedback (👀 on message receive, 🔄 during AI work, ✅/❌ on completion) — a nice UX enhancement that's correctly isolated from core message processing. The adapter implementation and error handling are solid. Two items need attention before merge: Blocking issuesNone. Suggested fixes
Minor / nice-to-have
Compliments
Reviewed via maintainer-review-pr workflow (Pi/Minimax). Aspects run: code-review, error-handling, test-coverage, comment-quality, docs-impact. |
There was a problem hiding this comment.
Actionable comments posted: 15
♻️ Duplicate comments (1)
packages/core/src/orchestrator/orchestrator-agent.ts (1)
949-974:⚠️ Potential issue | 🟠 MajorMove reaction teardown/final status into a real
finally.This block is still inside the main
try, so any early successreturnabove it skips cleanup entirely. In this function that includes the approval/command paths at Line 631, Line 722, Line 747, Line 754, Line 761, and Line 778, which meanseyes/arrows_counterclockwisecan still be left on the message even though the comment says otherwise.♻️ Minimal structure that preserves cleanup on every exit path
export async function handleMessage( platform: IPlatformAdapter, conversationId: string, message: string, context?: HandleMessageContext ): Promise<void> { + let finalReaction: 'white_check_mark' | 'x' | null = 'white_check_mark'; const { issueContext, threadContext, parentConversationId, isolationHints, attachedFiles } = context ?? {}; try { getLog().debug({ conversationId }, 'orchestrator_message_received'); await safeAddReaction(platform, conversationId, 'eyes'); @@ getLog().debug({ conversationId }, 'orchestrator_message_completed'); - - // Reaction cleanup and final status (ensures even on early returns) - try { - await safeRemoveReaction(platform, conversationId, 'eyes'); - await safeRemoveReaction(platform, conversationId, 'arrows_counterclockwise'); - await safeAddReaction(platform, conversationId, 'white_check_mark'); - } finally { - // No additional cleanup needed - } } catch (error) { + finalReaction = 'x'; const err = toError(error); getLog().error({ err, conversationId }, 'orchestrator_message_failed'); - - // Reaction cleanup and final status (ensures even on exceptions) - try { - await safeRemoveReaction(platform, conversationId, 'eyes'); - await safeRemoveReaction(platform, conversationId, 'arrows_counterclockwise'); - await safeAddReaction(platform, conversationId, 'x'); - } finally { - // No additional cleanup needed - } - const userMessage = classifyAndFormatError(err); try { await platform.sendMessage(conversationId, userMessage); } catch (sendError) { getLog().error({ err: toError(sendError), conversationId }, 'error_notification_failed'); } + } finally { + await safeRemoveReaction(platform, conversationId, 'eyes'); + await safeRemoveReaction(platform, conversationId, 'arrows_counterclockwise'); + if (finalReaction) { + await safeAddReaction(platform, conversationId, finalReaction); + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/orchestrator/orchestrator-agent.ts` around lines 949 - 974, The reaction teardown currently lives inside the main try block and is skipped by early returns; move the cleanup/final-status logic into a real finally that always runs after the main try/catch. Specifically, remove the safeRemoveReaction(platform, conversationId, 'eyes') / safeRemoveReaction(platform, conversationId, 'arrows_counterclockwise') and the safeAddReaction(platform, conversationId, 'white_check_mark'|'x') calls from inside the try/catch bodies and place them in a single finally block after the catch so that safeRemoveReaction and safeAddReaction are always invoked (use the existing safeRemoveReaction and safeAddReaction calls and conversationId/platform variables). Ensure the catch still logs the error with getLog().error({ err, conversationId }, 'orchestrator_message_failed') before the finally runs.
🧹 Nitpick comments (4)
docker-entrypoint.sh (1)
22-31: Constrainsafe.directorydiscovery and make additions idempotent.This loop appends on every container start, so
safe.directoryduplicates accumulate in~/.gitconfig. Also, scanning all of/.archonis broader than necessary; Git repositories only reside under/.archon/workspaces/and/.archon/worktrees/, while other paths contain non-repo artifacts.Proposed patch
-find /.archon -name ".git" -prune -print 2>/dev/null | while IFS= read -r git_dir; do - $RUNNER git config --global --add safe.directory "$(dirname "$git_dir")" -done +find /.archon/workspaces /.archon/worktrees -name ".git" -prune -print 2>/dev/null | while IFS= read -r git_dir; do + repo_dir="$(dirname "$git_dir")" + if ! $RUNNER git config --global --get-all safe.directory 2>/dev/null | grep -Fqx -- "$repo_dir"; then + $RUNNER git config --global --add safe.directory "$repo_dir" + fi +done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-entrypoint.sh` around lines 22 - 31, Limit discovery to only /.archon/workspaces and /.archon/worktrees and make additions idempotent: change the find invocation to search those two directories (instead of /.archon) and, inside the loop that uses $RUNNER and git_dir, check whether the directory is already registered (use git config --global --get-all safe.directory and test with a fixed-string match) before calling $RUNNER git config --global --add safe.directory "$(dirname "$git_dir")"; this prevents duplicate entries in ~/.gitconfig while only targeting actual repo locations..archon/workflows/defaults/archon-feature-development.yaml (1)
19-29: Makeverify-pr-basebest-effort to avoid false workflow failures.With
set -euo pipefail, transientghfailures (or missingBASE_BRANCH) can fail the whole workflow after PR creation. Consider degrading this node to warn-and-continue.Resiliency-focused diff
- id: verify-pr-base bash: | - set -euo pipefail - EXPECTED="$BASE_BRANCH" - ACTUAL=$(gh pr view --json baseRefName -q '.baseRefName') + set -uo pipefail + EXPECTED="${BASE_BRANCH:-}" + if [ -z "$EXPECTED" ]; then + echo "Skipping PR base verification: BASE_BRANCH is not set" >&2 + exit 0 + fi + ACTUAL=$(gh pr view --json baseRefName -q '.baseRefName' 2>/dev/null || true) + if [ -z "$ACTUAL" ]; then + echo "Skipping PR base verification: unable to read PR base" >&2 + exit 0 + fi if [ "$ACTUAL" != "$EXPECTED" ]; then - PR_NUMBER=$(gh pr view --json number -q '.number') + PR_NUMBER=$(gh pr view --json number -q '.number' 2>/dev/null || true) + if [ -z "$PR_NUMBER" ]; then + echo "Could not resolve PR number; skipping re-target" >&2 + exit 0 + fi echo "Base mismatch on PR #$PR_NUMBER: expected=$EXPECTED actual=$ACTUAL — re-targeting" >&2 - gh pr edit "$PR_NUMBER" --base "$EXPECTED" + gh pr edit "$PR_NUMBER" --base "$EXPECTED" || echo "Retarget failed; continuing" >&2 else echo "PR base verified: $EXPECTED" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.archon/workflows/defaults/archon-feature-development.yaml around lines 19 - 29, The current bash step using set -euo pipefail can cause the workflow to fail on transient gh failures or missing BASE_BRANCH; change the script to be best-effort by guarding gh calls and empty variables: check if EXPECTED is non-empty before proceeding, capture ACTUAL with a tolerant command like ACTUAL=$(gh pr view --json baseRefName -q '.baseRefName' 2>/dev/null || true), treat missing ACTUAL as a warning (echo to stderr) and skip re-targeting, and make PR_NUMBER and gh pr edit calls similarly tolerant (e.g., PR_NUMBER=$(gh pr view --json number -q '.number' 2>/dev/null || true) and only run gh pr edit if both PR_NUMBER and EXPECTED/ACTUAL are present), so the step warns on errors instead of failing the workflow.packages/workflows/src/loader.ts (1)
408-425: Normalize case before deduping tags, or relax the comment.The comment says values like
GitLab/GitLab/gitlabshould not survive as distinct tags, but this implementation only trims and dedupes exact strings. Right nowGitLabandgitlabstill come back as two tags.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workflows/src/loader.ts` around lines 408 - 425, The code trims and dedupes raw.tags but doesn't normalize case, so values like "GitLab" and "gitlab" remain distinct; update the tag normalization in the block that sets tags (the raw.tags -> tags transformation) to normalize case before deduping (e.g., .map(t => t.trim().toLowerCase()) before passing to new Set) so case-insensitive duplicates are removed, then, if you need to preserve original casing choose a deterministic canonicalization step after dedupe; keep the invalid-block warning via getLog().warn({ filename, value: raw.tags }, 'invalid_tags_block_ignored') unchanged.packages/workflows/src/dag-executor.test.ts (1)
6177-6229: Relax the Bun syntax error location assertion to prevent brittle test failures.The
expect(errorMsg).toContain('[eval]')check depends on Bun's current error format, which is not guaranteed across versions. Bun's syntax error reporting uses formats like "at loadAndEvaluateModule (2:1)" rather than explicitly including[eval]in stderr location tokens—this can vary without any change to Archon's sanitizer. Keep the "no command body leak + bounded diagnostics" assertions (prefix strip, length cap) and either remove the location check or replace it with a more stable marker like checking for "SyntaxError" or an actual line/column number.This aligns with the principle: "Prefer reproducible commands and locked dependency behavior in CI-sensitive paths; keep tests deterministic — no flaky timing or network dependence without guardrails."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workflows/src/dag-executor.test.ts` around lines 6177 - 6229, The brittle assertion in the test "failure message strips the \"Command failed: bun -e <body>\" prefix and stays small" checks for "[eval]" in errorMsg; replace that check with a more stable assertion (e.g., assert errorMsg contains "SyntaxError" or a line/column pattern like /\d+:\d+/) or remove the location assertion entirely so the test only verifies prefix stripping and length bounds. Locate the failing assertion on errorMsg (the expect(errorMsg).toContain('[eval]') line) and update it to something like expect(errorMsg).toMatch(/SyntaxError|\d+:\d+/) or simply delete that expect to avoid Bun-version-dependent failures. Ensure the other assertions (no "Command failed:" prefix, no leaked script body, length < 2100) remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.archon/scripts/maintainer-standup-persist.ts:
- Around line 96-102: The filename is currently derived from new
Date().toLocaleDateString('sv-SE') which can drift; instead extract the standup
date from the generated content and use that when building briefPath/statePath.
Locate the code that constructs date/briefPath/statePath (variables date,
briefPath, statePath) and replace the wall-clock-derived date with the parsed
standup date (e.g., parse the "Maintainer Standup — YYYY-MM-DD" heading or use
the date field from the parsed payload) and format it as 'YYYY-MM-DD' before
resolving briefsDir/briefPath; ensure state.json uses the same derived date
consistently.
- Around line 38-105: Validate parsed outputs and abort before any writes: after
parsing into state/brief (the branches that set state, brief, and source from
the delimiter or JSON-wrapper fallback) add strict checks that reject malformed
data and call process.exit(1) without writing. Specifically, for the delimiter
branch verify raw contains nothing after ARCHON_STATE_JSON_END (ensure endIdx
=== raw.indexOf(END, beginIdx)), confirm the extracted state is an object
(typeof === 'object' && state !== null && !Array.isArray(state)) and that
required state keys exist with correct types; for the JSON-wrapper branch ensure
parsed.next_state is a non-array object and parsed.brief_markdown is a non-empty
string and that brief contains the heading '# Maintainer Standup' (or fail if
missing). Only proceed to mkdirSync/writeFileSync when all validations pass;
otherwise emit descriptive stderr messages and exit without touching files.
In @.archon/workflows/defaults/archon-architect.yaml:
- Around line 315-316: Replace the ad-hoc "--body \"...\"" usage in the gh pr
create step with logic that loads the repository PR template and supplies that
as the PR body (ensuring every section is filled); specifically update the step
that runs `gh pr create --base $BASE_BRANCH --title "..." --body "..."` to read
the repository's PULL_REQUEST_TEMPLATE.md into the body, programmatically
populate any placeholders, and pass that content to `gh pr create` so the repo
template is always used.
In @.archon/workflows/defaults/archon-piv-loop.yaml:
- Around line 700-704: The pipeline currently masks git push failures by using
"git push -u origin HEAD 2>&1 || echo ..." in the "Step 1: Push Changes" step,
which allows the workflow to continue to the PR creation that uses "gh pr create
--draft --base $BASE_BRANCH"; change this so a failed push stops the job: remove
the "|| echo ..." masking and instead fail the step when git push returns
non‑zero (for example, run git push and if it fails echo a clear error and exit
1, or enable errexit for the step) so the pipeline does not attempt PR creation
if the branch was not pushed.
- Around line 398-403: The TASK_COUNT assignment currently appends the fallback
"0" via "|| echo '0'", producing two lines when grep exits non-zero; change it
to capture grep -c output only and then apply parameter expansion to default to
zero if empty — e.g., assign TASK_COUNT to the result of grep -c (suppressing
errors to /dev/null) and then set TASK_COUNT=${TASK_COUNT:-0} before the numeric
comparison; update the code referencing TASK_COUNT and keep PLAN_FILE and the
grep command intact.
In @.archon/workflows/defaults/archon-ralph-dag.yaml:
- Around line 651-663: The verify-pr-base step should early-exit when no PR was
created: add a preliminary check using gh pr view --json number (or similar) to
detect if a PR exists and if it returns empty or non-existent, echo a "no PR to
verify, skipping" message and exit 0; otherwise proceed with the existing ACTUAL
vs EXPECTED base verification and retarget logic. Update the verify-pr-base job
(referenced by id: verify-pr-base and its use of ACTUAL/EXPECTED and gh pr view)
to perform this existence check before fetching baseRefName so the workflow
doesn't fail when the run ended without creating a PR.
In @.archon/workflows/defaults/archon-refactor-safely.yaml:
- Around line 449-451: The gh PR creation step currently constructs its own
body; update the `gh pr create --base $BASE_BRANCH --title "..." --body "..."`
invocation so it explicitly copies the repository PR template into the PR body
(e.g., read the repo's PR template file and pass it to gh via --body-file or
substitute the file contents into --body) to ensure every required section from
the official template is present when creating PRs.
In `@CHANGELOG.md`:
- Around line 46-56: The file contains a second set of "### Added", "###
Changed", and "### Fixed" headings for the same 0.3.10 entry which violates Keep
a Changelog/MD024; merge the duplicate blocks into the initial "### Added", "###
Changed", and "### Fixed" sections (or convert the duplicate headings into plain
bullets/subheadings under the same version) so each heading appears only once
per version; ensure the detailed bullets about $LOOP_PREV_OUTPUT, provider/model
resolution (referencing the text that mentions
inferProviderFromModel/isModelCompatible and the provider resolution chain
node.provider ?? workflow.provider ?? config.assistant), and the bash/script
node fix (referencing formatSubprocessFailure and dag.node_empty_output) are
preserved under their respective single headings.
In `@package.json`:
- Line 3: Revert the manual version bump in package.json: undo the change that
sets the root "version" key to "0.3.10" so this branch does not modify the
package.json version; leave the version at whatever value was in dev/main before
this PR (i.e., remove the "0.3.10" edit) and let the release workflow own the
version bump and changelog generation. Ensure only non-release changes remain in
this commit and do not add any other release-related edits.
In `@packages/core/src/orchestrator/orchestrator-agent.ts`:
- Around line 72-104: Add orchestrator-level tests that exercise safeAddReaction
and safeRemoveReaction and the lifecycle in handleMessage: create a fake
IPlatformAdapter instance with (a) no addReaction/removeReaction to assert
no-ops, (b) addReaction/removeReaction that throw to assert errors are swallowed
and getLog().warn is called, and (c) a normal adapter to assert handleMessage
triggers the reaction sequence (eyes → spinner → success/fail) in order; use
spies/mocks on platform methods and on getLog().warn to verify calls and
ordering.
In `@packages/docs-web/src/content/docs/getting-started/ai-assistants.md`:
- Line 63: Reword the sentence about claudeBinaryPath to clarify its scope:
replace the phrase "config-file `claudeBinaryPath` is intentionally
binary-mode-only (per-repo, not per-machine)" with text that explains
claudeBinaryPath can be set either repo-locally in .archon/config.yaml
(repo-scoped) or globally in ~/.archon/config.yaml (machine-scoped), and note
that CLAUDE_BIN_PATH environment variable still overrides both; update
references to `claudeBinaryPath`, `CLAUDE_BIN_PATH`, and the two config files to
make the repo vs machine distinction explicit.
In `@packages/providers/src/community/pi/event-bridge.ts`:
- Around line 186-196: The fallback recovery currently finds only the first "{"
(variable firstBrace) in cleaned and thus misses array-root JSON; update the
logic in event-bridge.ts around cleaned, firstBrace, and JSON.parse so it scans
for the earliest occurrence of either "{" or "[" (e.g., compute indices for both
characters, ignore -1s, pick the smallest positive index) and then
JSON.parse(cleaned.slice(firstOpenIndex)) when that index > 0; keep the same
try/catch behavior so structuredOutput recovery works for top-level arrays as
well as objects.
In `@packages/workflows/src/dag-executor.test.ts`:
- Around line 6858-6895: The test "trigger_rule: none_failed skips dependent
node + anyFailed still marks run failed" doesn't assert that node 'c' was
skipped; update the test that calls executeDagWorkflow to explicitly assert skip
behavior by checking mockStore.nodeSkipped was called for node id 'c' (or, if
nodeSkipped helper isn't available, assert mockStore.nodeCompleted/nodeExecuted
was NOT called with 'c'). Locate the test block using the nodes array and
executeDagWorkflow invocation and add a final expectation that verifies
mockStore.nodeSkipped (or absence of node_completed/ node execution call) for
'c' so the test actually proves 'c' was skipped.
In `@packages/workflows/src/loader.test.ts`:
- Around line 31-34: The calls to clearRegistry() and registerBuiltinProviders()
run at module import and mutate global provider state; move this bootstrap into
test setup/teardown so registry is deterministic: remove the top-level
clearRegistry()/registerBuiltinProviders() and instead call clearRegistry()
before each test (or in a beforeEach/beforeAll hook) and call
registerBuiltinProviders() inside that setup, then restore/clear the registry in
an afterEach/afterAll hook; reference the clearRegistry and
registerBuiltinProviders symbols and ensure any tests that rely on a
pre-populated registry explicitly invoke the setup helper so state is not shared
across test files.
In `@packages/workflows/src/loader.ts`:
- Around line 155-165: The current stripMarkdownCode function removes both
fenced code blocks and inline backtick segments, which prevents load-time
validation (substituteNodeOutputRefs) from catching placeholders inside inline
code; change stripMarkdownCode to only strip fenced code blocks (```...```) and
leave inline `...` segments intact, then keep using stripMarkdownCode when
collecting sources for node.prompt and (for loops) node.loop.prompt so inline
placeholders remain present for substituteNodeOutputRefs and load-time checks;
reference stripMarkdownCode, substituteNodeOutputRefs, node.prompt,
node.loop.prompt, and isLoopNode when making this change.
---
Duplicate comments:
In `@packages/core/src/orchestrator/orchestrator-agent.ts`:
- Around line 949-974: The reaction teardown currently lives inside the main try
block and is skipped by early returns; move the cleanup/final-status logic into
a real finally that always runs after the main try/catch. Specifically, remove
the safeRemoveReaction(platform, conversationId, 'eyes') /
safeRemoveReaction(platform, conversationId, 'arrows_counterclockwise') and the
safeAddReaction(platform, conversationId, 'white_check_mark'|'x') calls from
inside the try/catch bodies and place them in a single finally block after the
catch so that safeRemoveReaction and safeAddReaction are always invoked (use the
existing safeRemoveReaction and safeAddReaction calls and
conversationId/platform variables). Ensure the catch still logs the error with
getLog().error({ err, conversationId }, 'orchestrator_message_failed') before
the finally runs.
---
Nitpick comments:
In @.archon/workflows/defaults/archon-feature-development.yaml:
- Around line 19-29: The current bash step using set -euo pipefail can cause the
workflow to fail on transient gh failures or missing BASE_BRANCH; change the
script to be best-effort by guarding gh calls and empty variables: check if
EXPECTED is non-empty before proceeding, capture ACTUAL with a tolerant command
like ACTUAL=$(gh pr view --json baseRefName -q '.baseRefName' 2>/dev/null ||
true), treat missing ACTUAL as a warning (echo to stderr) and skip re-targeting,
and make PR_NUMBER and gh pr edit calls similarly tolerant (e.g., PR_NUMBER=$(gh
pr view --json number -q '.number' 2>/dev/null || true) and only run gh pr edit
if both PR_NUMBER and EXPECTED/ACTUAL are present), so the step warns on errors
instead of failing the workflow.
In `@docker-entrypoint.sh`:
- Around line 22-31: Limit discovery to only /.archon/workspaces and
/.archon/worktrees and make additions idempotent: change the find invocation to
search those two directories (instead of /.archon) and, inside the loop that
uses $RUNNER and git_dir, check whether the directory is already registered (use
git config --global --get-all safe.directory and test with a fixed-string match)
before calling $RUNNER git config --global --add safe.directory "$(dirname
"$git_dir")"; this prevents duplicate entries in ~/.gitconfig while only
targeting actual repo locations.
In `@packages/workflows/src/dag-executor.test.ts`:
- Around line 6177-6229: The brittle assertion in the test "failure message
strips the \"Command failed: bun -e <body>\" prefix and stays small" checks for
"[eval]" in errorMsg; replace that check with a more stable assertion (e.g.,
assert errorMsg contains "SyntaxError" or a line/column pattern like /\d+:\d+/)
or remove the location assertion entirely so the test only verifies prefix
stripping and length bounds. Locate the failing assertion on errorMsg (the
expect(errorMsg).toContain('[eval]') line) and update it to something like
expect(errorMsg).toMatch(/SyntaxError|\d+:\d+/) or simply delete that expect to
avoid Bun-version-dependent failures. Ensure the other assertions (no "Command
failed:" prefix, no leaked script body, length < 2100) remain unchanged.
In `@packages/workflows/src/loader.ts`:
- Around line 408-425: The code trims and dedupes raw.tags but doesn't normalize
case, so values like "GitLab" and "gitlab" remain distinct; update the tag
normalization in the block that sets tags (the raw.tags -> tags transformation)
to normalize case before deduping (e.g., .map(t => t.trim().toLowerCase())
before passing to new Set) so case-insensitive duplicates are removed, then, if
you need to preserve original casing choose a deterministic canonicalization
step after dedupe; keep the invalid-block warning via getLog().warn({ filename,
value: raw.tags }, 'invalid_tags_block_ignored') unchanged.
🪄 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
Run ID: cce52ffc-ce1b-4e11-af09-802e6eca8d3a
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (48)
.archon/commands/defaults/archon-implement-issue.md.archon/commands/maintainer-standup.md.archon/scripts/maintainer-standup-persist.ts.archon/workflows/defaults/archon-architect.yaml.archon/workflows/defaults/archon-feature-development.yaml.archon/workflows/defaults/archon-fix-github-issue.yaml.archon/workflows/defaults/archon-idea-to-pr.yaml.archon/workflows/defaults/archon-issue-review-full.yaml.archon/workflows/defaults/archon-piv-loop.yaml.archon/workflows/defaults/archon-plan-to-pr.yaml.archon/workflows/defaults/archon-ralph-dag.yaml.archon/workflows/defaults/archon-refactor-safely.yaml.archon/workflows/defaults/archon-workflow-builder.yaml.archon/workflows/maintainer/maintainer-standup-minimax.yaml.archon/workflows/maintainer/maintainer-standup.yaml.github/workflows/e2e-smoke.yml.gitignoreCHANGELOG.mddocker-entrypoint.shpackage.jsonpackages/adapters/package.jsonpackages/cli/package.jsonpackages/core/package.jsonpackages/core/src/db/sessions.test.tspackages/core/src/db/sessions.tspackages/core/src/orchestrator/orchestrator-agent.test.tspackages/core/src/orchestrator/orchestrator-agent.tspackages/docs-web/package.jsonpackages/docs-web/src/content/docs/getting-started/ai-assistants.mdpackages/docs-web/src/content/docs/guides/script-nodes.mdpackages/git/package.jsonpackages/isolation/package.jsonpackages/paths/package.jsonpackages/providers/package.jsonpackages/providers/src/claude/binary-resolver-dev.test.tspackages/providers/src/claude/binary-resolver.tspackages/providers/src/community/pi/event-bridge.test.tspackages/providers/src/community/pi/event-bridge.tspackages/server/package.jsonpackages/web/package.jsonpackages/workflows/package.jsonpackages/workflows/src/dag-executor.test.tspackages/workflows/src/dag-executor.tspackages/workflows/src/defaults/bundled-defaults.generated.tspackages/workflows/src/executor-shared.test.tspackages/workflows/src/executor-shared.tspackages/workflows/src/loader.test.tspackages/workflows/src/loader.ts
✅ Files skipped from review due to trivial changes (14)
- packages/docs-web/package.json
- packages/workflows/package.json
- packages/adapters/package.json
- packages/web/package.json
- packages/paths/package.json
- packages/core/package.json
- .archon/commands/defaults/archon-implement-issue.md
- packages/isolation/package.json
- .gitignore
- packages/server/package.json
- packages/docs-web/src/content/docs/guides/script-nodes.md
- packages/providers/package.json
- packages/cli/package.json
- packages/git/package.json
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/core/src/orchestrator/orchestrator-agent.ts (2)
948-959: Redundant try/catch — the safe* helpers already swallow errors.
safeAddReactionandsafeRemoveReactionare designed to never throw (they catch internally and log atwarn). The outertry/catchblock will never catch anything and adds unnecessary nesting.♻️ Suggested simplification
getLog().debug({ conversationId }, 'orchestrator_message_completed'); - // Reaction cleanup and final status (ensures even on early returns) - try { - // Remove intermediate reactions before adding final status - await safeRemoveReaction(platform, conversationId, 'eyes'); - await safeRemoveReaction(platform, conversationId, 'arrows_counterclockwise'); - - // React with ✅ on success - await safeAddReaction(platform, conversationId, 'white_check_mark'); - } catch { - // Silently ignore reaction errors - } + // Remove intermediate reactions before adding final status + await safeRemoveReaction(platform, conversationId, 'eyes'); + await safeRemoveReaction(platform, conversationId, 'arrows_counterclockwise'); + // React with ✅ on success + await safeAddReaction(platform, conversationId, 'white_check_mark'); } catch (error) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/orchestrator/orchestrator-agent.ts` around lines 948 - 959, The outer try/catch around the reaction cleanup is redundant because safeAddReaction and safeRemoveReaction already swallow and log errors; remove the try/catch block and directly await the calls to safeRemoveReaction(platform, conversationId, 'eyes'), safeRemoveReaction(platform, conversationId, 'arrows_counterclockwise'), and safeAddReaction(platform, conversationId, 'white_check_mark') so the code is simpler and not nested in an unused catch.
963-974: Same redundant try/catch pattern in error path.The same simplification applies here — the outer
try/catcharound the safe* helpers is unnecessary.♻️ Suggested simplification
- // Reaction cleanup and final status (ensures even on exceptions) - try { - // Remove intermediate reactions before adding final status - await safeRemoveReaction(platform, conversationId, 'eyes'); - await safeRemoveReaction(platform, conversationId, 'arrows_counterclockwise'); - - // React with ❌ on failure - await safeAddReaction(platform, conversationId, 'x'); - } catch { - // Silently ignore reaction errors - } + // Remove intermediate reactions before adding final status + await safeRemoveReaction(platform, conversationId, 'eyes'); + await safeRemoveReaction(platform, conversationId, 'arrows_counterclockwise'); + // React with ❌ on failure + await safeAddReaction(platform, conversationId, 'x'); const userMessage = classifyAndFormatError(err);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/orchestrator/orchestrator-agent.ts` around lines 963 - 974, The outer try/catch around the final reaction cleanup is redundant because the safe* helpers already handle their own errors; remove the surrounding try { ... } catch { } block and simply call safeRemoveReaction(platform, conversationId, 'eyes'), safeRemoveReaction(platform, conversationId, 'arrows_counterclockwise'), and safeAddReaction(platform, conversationId, 'x') directly so errors are managed by the safe* helpers (refer to safeRemoveReaction and safeAddReaction and the conversationId/platform variables in the same scope).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/core/src/orchestrator/orchestrator-agent.ts`:
- Around line 948-959: The outer try/catch around the reaction cleanup is
redundant because safeAddReaction and safeRemoveReaction already swallow and log
errors; remove the try/catch block and directly await the calls to
safeRemoveReaction(platform, conversationId, 'eyes'),
safeRemoveReaction(platform, conversationId, 'arrows_counterclockwise'), and
safeAddReaction(platform, conversationId, 'white_check_mark') so the code is
simpler and not nested in an unused catch.
- Around line 963-974: The outer try/catch around the final reaction cleanup is
redundant because the safe* helpers already handle their own errors; remove the
surrounding try { ... } catch { } block and simply call
safeRemoveReaction(platform, conversationId, 'eyes'),
safeRemoveReaction(platform, conversationId, 'arrows_counterclockwise'), and
safeAddReaction(platform, conversationId, 'x') directly so errors are managed by
the safe* helpers (refer to safeRemoveReaction and safeAddReaction and the
conversationId/platform variables in the same scope).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 95b3bcd0-e23e-412b-a433-daf036f35ca9
📒 Files selected for processing (6)
CHANGELOG.mdpackages/adapters/src/chat/slack/adapter.tspackages/core/package.jsonpackages/core/src/orchestrator/orchestrator-agent.tspackages/core/src/orchestrator/safe-reaction.test.tspackages/docs-web/src/content/docs/adapters/slack.md
✅ Files skipped from review due to trivial changes (1)
- packages/docs-web/src/content/docs/adapters/slack.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/package.json
- packages/adapters/src/chat/slack/adapter.ts
Author
|
@Wirasm thank you for the feedback and review . I've added your suggestion. (Archon to the moon 🚀🚀🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Describe this PR in 2-5 bullets:
UX Journey
Before
After
Architecture Diagram
Before
After
Connection inventory (list every module-to-module edge, mark changes):
Label Snapshot
risk: lowsize: Sadapters:slackadapters:slackChange Metadata
featureadaptersLinked Issue
Validation Evidence (required)
Commands and result summary:
packages/adapters/src/chat/slack/adapter.test.ts) covers reaction functions, all tests passSecurity Impact (required)
No)No- uses existing Slack SDK, no new endpoints)No)No)Yes, describe risk and mitigation: N/ACompatibility / Migration
Yes)No)No)Human Verification (required)
What was personally validated beyond CI:
Side Effects / Blast Radius (required)
Rollback Plan (required)
git revert cee0fd01 79da658cRisks and Mitigations
List real risks in this PR (or write
None).Risk: Slack API rate limits
Risk: Reaction methods not implemented on other platforms
Summary by CodeRabbit
New Features
Bug Fixes
Documentation