feat(slack): structured scoping-question modal for spec loop#1318
Closed
ibuildthings-instrumentl wants to merge 21 commits intocoleam00:devfrom
Closed
feat(slack): structured scoping-question modal for spec loop#1318ibuildthings-instrumentl wants to merge 21 commits intocoleam00:devfrom
ibuildthings-instrumentl wants to merge 21 commits intocoleam00:devfrom
Conversation
- Added a new configuration option to copy local .env files into isolated worktrees for Scout MCP compatibility. - Removed outdated note regarding Claude Code binary path from setup documentation. - Renumbered subsequent steps in the setup guide for clarity.
Update Archon configuration and setup documentation
Add design for a Slack @archie bot flow that takes a natural-language feature request end-to-end: interactive spec with bounded revision loop, plan, implement in worktree, open PR, bounded review loop, wait for CI, trigger review-app deploy, post URL back to the thread. Composes existing Archon commands plus three small helper scripts; no adapter changes required. Made-with: Cursor
Bite-sized tasks covering 3 helper scripts, the workflow YAML, bundled- defaults registration, pre-PR validation, and a manual smoke-test checklist. Noted divergences from the design doc: code-review rounds are unrolled explicitly, reviewApp config is hardcoded for v1, and per-script unit tests are dropped in favor of workflow-level parsing plus manual smoke test. Made-with: Cursor
Wraps gh workflow run for review-app deployment; exits non-zero with a clear message on dispatch failure. Used by archon-slack-feature-to-review-app. Note: written as .js (not .ts as originally planned) to match the existing .archon/scripts/echo-args.js pattern and avoid the typed-linting scope gap for .archon/scripts/**/*.ts. Made-with: Cursor
Wraps gh pr checks --watch --fail-fast with a wall-clock timeout so the workflow can't hang indefinitely. Exit codes distinguish pass/fail/timeout. Note: written as .js (not .ts as originally planned) for the same reason as dispatch-review-app.js. Made-with: Cursor
Polls gh pr view --json comments for a URL matching a caller-supplied regex; prints the URL on stdout, errors on stderr so the workflow engine captures only the URL via \$nodeId.output. Note: written as .js (not .ts as originally planned) for the same reason as dispatch-review-app.js. Made-with: Cursor
End-to-end workflow for Slack @archie feature requests: interactive spec creation (bounded 3-iteration revision loop), plan + implement + PR using existing commands, two-round code review with conditional second pass, CI wait, review-app dispatch, URL fetch from PR comments, and final post back to the Slack thread. Composes existing commands; adds no new adapter or orchestrator code. Script invocations use .js extensions per the Tasks 1-3 divergence. Made-with: Cursor
Adds the text import + map entry so binary builds include the workflow. Bumps the bundled-workflow count assertion from 13 to 14 and adds the workflow to the expected-names list. Made-with: Cursor
Insert an interactive refine-plan loop between create-plan and plan-setup, mirroring the existing spec-approval gate and the pattern used by archon-scout-perf-roadmap. The loop posts a condensed plan summary in-thread, accepts feedback that edits $ARTIFACTS_DIR/plan.md in place, and only proceeds to plan-setup on explicit "approved" / "looks good" / "ship it" / "go". Bounded at max_iterations: 5. Rationale: previously the workflow jumped straight from plan creation into implementation, giving the user no chance to reshape scope, ordering, or task list before code gets written. This symmetrizes the gating with Phase A and matches how other plan-driven workflows behave. Made-with: Cursor
Interactive-loop gate messages now render Approve (primary) and Request
changes buttons in Slack; clicking Approve resumes the paused workflow,
while Request changes opens a modal with a feedback textarea whose
submission is synthesized into the gate thread.
- packages/core: add optional `interactiveGate` to MessageMetadata.
- packages/workflows: dag-executor gate-send passes runId + nodeId via
the new metadata field so adapters can bind actions per run.
- packages/adapters/slack: sendMessage renders an actions block on the
final chunk when the gate metadata is present; Bolt action + view
handlers synthesize message events that reuse the existing
natural-language approval path in handleMessage, so no new server
wiring is required.
- Fallback path: adapters without rich input ignore the metadata; the
text body still includes the `/workflow approve <uuid>` instructions.
Tests:
- 3 new Slack adapter tests asserting the actions block, action_ids,
and that buttons attach only to the final chunk of long messages.
- 1 extra assertion on the dag-executor interactive-loop test verifying
the gate-send carries { runId, nodeId } metadata.
Made-with: Cursor
feat: @archie Slack feature-to-review-app workflow
feat(slack): Block Kit approve + request-changes UI for workflow loop gates
When a user @mentions or DMs Archon, the bot now posts an 👀 reaction on the incoming message the moment it's received -- before thread-history fetch, lock acquisition, planner warm-up, or first LLM token. This eliminates the awkward silent gap between "user hit send" and "bot responds" for long-running workflows. - SlackAdapter.acknowledgeReceipt(event) calls reactions.add; swallows errors so a missing reactions:write scope just skips the reaction instead of blocking the conversation. - Server onMessage handler fires the ack right after auth/stripBotMention with void (fire-and-forget) so the reaction round-trip never delays orchestration. - reactions:write added to the Starlight docs, skill guide, and CLI setup prompt as an optional scope. - Three adapter tests cover the happy path, missing-scope failure, and already_reacted replay case. Made-with: Cursor
feat(slack): ack incoming messages with 👀 reaction
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
fix(deps): bump flatted to >=3.4.2 (Dependabot #19)
Fixes SSRF via NO_PROXY Hostname Normalization Bypass and Cloud Metadata Exfiltration via Header Injection Chain. Transitive from @slack/bolt and @slack/web-api. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
fix(deps): bump axios to ^1.15.0 (coleam00#29, coleam00#30)
…nput Add a new implementation plan for converting scoping questions into a Slack modal form. This includes modifications to the workflow prompt contract, Slack adapter for parsing and rendering the question schema, and handling modal submissions. The changes ensure that the workflow remains intact while enhancing user interaction through structured inputs. Related files updated include the workflow YAML, Slack adapter, and corresponding tests.
Convert free-text scoping questions in archon-slack-feature-to-review-app into a Slack modal form with typed inputs (yes/no, checkboxes, select, text). The workflow prompt emits an `archon-questions` fenced YAML schema on the first spec iteration. The Slack adapter parses it, renders an "Answer questions" button, opens a modal on click, and synthesizes formatted answers back into $LOOP_USER_INPUT. Falls back to existing Approve/Request changes gate when schema is absent or malformed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds Scout APM performance route discovery and consolidation workflows with multi-parallel profiling, implements Slack interactive gates (Approve/Reject buttons and modal forms), introduces CI-wait and review-app deployment helper scripts, and extends type definitions to propagate interactive-gate metadata through message flows. Changes
Sequence Diagram(s)sequenceDiagram
actor User as Slack User
participant Slack as Slack App
participant Workflow as Archon Workflow
participant GH as GitHub / CI
participant ReviewApp as Review App Svc
participant PRComments as PR Comments
User->>Slack: Request feature in thread
Slack->>Workflow: Trigger archon-slack-feature-to-review-app
Note over Workflow: Phase 1: Spec Approval Loop (≤3 iterations)
Workflow->>Slack: Send clarifying questions
Slack->>User: Display questions + "Answer" modal button
User->>Slack: Answer & submit
Slack->>Workflow: Feedback message
Workflow->>Slack: Updated spec summary
Slack->>User: Show spec + approve/request button
User->>Slack: Approve (emit SPEC_APPROVED)
Note over Workflow: Phase 2: Plan & Implementation
Workflow->>Workflow: Generate plan, implement, validate
Workflow->>GH: Create draft PR
Note over Workflow: Phase 3: Code Review (≤2 rounds + gate)
Workflow->>Workflow: Run parallel review agents (scope, code, tests, docs)
Workflow->>Workflow: Synthesize findings
alt Blocking findings detected
Workflow->>Workflow: Apply fixes, re-implement
Workflow->>Workflow: Run review round 2
end
Workflow->>Workflow: review-gate: Converge or fail
Note over Workflow: Phase 4: CI + Review App Deploy
Workflow->>GH: Wait for PR CI with timeout
GH-->>Workflow: CI checks complete
Workflow->>GH: Dispatch review-app workflow for PR branch
GH->>ReviewApp: Deploy review app
ReviewApp-->>GH: Post review-app URL in PR comment
Note over Workflow: Phase 5: Poll & Post Result
Workflow->>PRComments: Poll comments for URL regex match
PRComments-->>Workflow: Review-app URL found
Workflow->>Slack: Post final message (PR URL + review-app URL)
Slack->>User: Show PR link and review-app link in thread
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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. 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: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.claude/skills/archon/guides/setup.md (1)
161-173:⚠️ Potential issue | 🟡 MinorSection numbering is inconsistent with the surrounding step flow.
5c/5dare nested under Step 4 content and appear before## Step 5, so these should remain4c/4dto avoid operator confusion during setup.Suggested doc fix
-### 5c: Verify Configuration +### 4c: Verify Configuration ... -### 5d: Run Database Migrations (PostgreSQL only) +### 4d: Run Database Migrations (PostgreSQL only)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/archon/guides/setup.md around lines 161 - 173, Section headings "### 5c: Verify Configuration" and "### 5d: Run Database Migrations (PostgreSQL only)" are misnumbered and should be "4c" and "4d" because they live under Step 4; update the two headings to "### 4c: Verify Configuration" and "### 4d: Run Database Migrations (PostgreSQL only)" and verify their surrounding context remains nested under Step 4 (before "## Step 5") so the flow and references (e.g., the verification command `archon version` and the PostgreSQL migration note) remain correct.packages/workflows/src/defaults/bundled-defaults.ts (1)
40-56:⚠️ Potential issue | 🟡 MinorUpdate the workflow count comment.
Line 40 still says 13 default workflows, but this import brings the total to 14.
Proposed fix
-// Default Workflows (13 total) +// Default Workflows (14 total)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workflows/src/defaults/bundled-defaults.ts` around lines 40 - 56, The top comment "Default Workflows (13 total)" in defaults/bundled-defaults.ts is out of date because an additional import (archonSlackFeatureToReviewAppWf) increases the total to 14; update that comment to read "Default Workflows (14 total)" (look for the exact string "Default Workflows (13 total)" at the top of the file and change it to 14).
🟡 Minor comments (10)
.cursor/mcp.json-5-9 (1)
5-9:⚠️ Potential issue | 🟡 MinorRedundant
SCOUT_API_KEYwiring — pick one source.
SCOUT_API_KEYis injected three ways:
args: ["--env", "SCOUT_API_KEY"]tells docker to pass through the variable from the container launcher's environment.env.SCOUT_API_KEY: "${env:SCOUT_API_KEY}"sets it for thedockerprocess.envFile: ${workspaceFolder}/.envloads it again from.env.If
SCOUT_API_KEYis not set in the shell but present in.env,${env:SCOUT_API_KEY}expands to empty andenvoverridesenvFile, so docker receives an empty string and the MCP server will silently fail to authenticate. Consider dropping the explicitenventry and relying solely onenvFile, or dropenvFileand rely on the shell export.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.cursor/mcp.json around lines 5 - 9, The SCOUT_API_KEY is being provided three ways (args, env.SCOUT_API_KEY, envFile) which causes envFile to be overridden by an empty ${env:SCOUT_API_KEY}; remove the explicit env entry and let envFile supply the value: delete the "SCOUT_API_KEY": "${env:SCOUT_API_KEY}" entry from the env block so that envFile (envFile) provides SCOUT_API_KEY while keeping the args ["run", "--rm", "-i", "--env", "SCOUT_API_KEY", ...] behavior; alternatively, if you prefer shell-provided values, remove envFile instead—pick one source and remove the conflicting env mapping to prevent empty overrides..archon/scripts/fetch-review-app-url.js-60-68 (1)
60-68:⚠️ Potential issue | 🟡 MinorUser-supplied regex is unanchored and untrusted — guard against ReDoS / accidental over-matching.
new RegExp(regexStr)compiles whatever the workflow passes in with no flags and no length/complexity cap. Given this script is driven by workflow config (not end-user input) the security risk is low, but a pathological pattern (e.g.(a+)+$) against a large comment body will wedge the poll loop for the full timeout window. Consider at minimum cappingc.body.lengthbefore matching, or documenting that patterns must be linear-time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.archon/scripts/fetch-review-app-url.js around lines 60 - 68, The user-supplied regex (regex created from regexStr) can be pathological and block the poll loop when run against large comment bodies; to fix, cap the input before executing the regex by truncating c.body to a safe MAX_COMMENT_LENGTH (use a named constant) and run regex.test or regex.exec against that truncated haystack instead of the full c.body, and optionally document the max length and that regexes should be linear-time; locate uses of regex and references to c.body in the poll loop and replace them to use the truncated haystack variable..archon/scripts/dispatch-review-app.js-34-38 (1)
34-38:⚠️ Potential issue | 🟡 Minor
stderroutput is being written to stdout.Line 35 forwards
gh's stderr viaconsole.log, which puts it on fd 1 alongside the final JSON payload. Downstream callers that parse stdout as JSON (or grep for the{"dispatched":true,...}line) will seegh's warnings mixed in and break. Useconsole.errorfor stderr pass-through.Proposed fix
- if (stdout.trim()) console.log(stdout.trim()); - if (stderr.trim()) console.log(stderr.trim()); + if (stdout.trim()) console.log(stdout.trim()); + if (stderr.trim()) console.error(stderr.trim());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.archon/scripts/dispatch-review-app.js around lines 34 - 38, The script currently forwards gh's stderr to stdout by calling console.log(stderr.trim()), which mixes warnings with the JSON payload; update the stderr passthrough to use console.error(stderr.trim()) instead so that stderr goes to fd 2 while leaving the JSON line (JSON.stringify({ dispatched: true, workflow: workflowFile, ref })) on stdout; locate the block handling stdout and stderr (variables stdout and stderr) in dispatch-review-app.js and replace the console.log call for stderr with console.error..archon/scripts/ci-wait.js-37-41 (1)
37-41:⚠️ Potential issue | 🟡 MinorHandle unknown
--fail-fastflag gracefully for olderghversions.The
--fail-fastflag was added togh pr checksin March 2023 and is available in current versions. However, users with olderghversions will get an unknown-flag error (exit code 1), which the script cannot distinguish from an actual CI failure. The current error handler only catches spawn errors, not exit code errors. Either document a minimumghversion requirement (≥2.49.2 or similar), or inspect the error output to detect and handle unknown-flag errors explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.archon/scripts/ci-wait.js around lines 37 - 41, The spawn of the GitHub CLI ("gh") for "pr checks" using the '--fail-fast' flag can fail on older gh versions and currently only spawn errors are handled; update the logic around the child process created by spawn (the variable child running ['pr','checks',pr,'--watch','--fail-fast','--interval','30']) to capture the child's stderr/exit code, detect the "unknown flag" or "flag provided but not defined" message (or exit code 1 with that stderr), and in that case retry or fallback by re-spawning "gh pr checks" without the '--fail-fast' flag (or alternatively run 'gh --version' and enforce a minimum version like 2.49.2), ensuring you handle both the stderr message and non-zero exit codes so the script distinguishes older gh from real CI failures.docs/plans/2026-04-20-slack-scoping-questions-form-plan.md-24-24 (1)
24-24:⚠️ Potential issue | 🟡 MinorUpdate the dependency note.
This says “No new packages,” but the PR summary says
js-yamlwas added for YAML parsing. Keeping the plan accurate helps future audits.Suggested documentation fix
-No new packages, no DB/schema changes, no workflow-engine API changes. +Adds `js-yaml` for parsing `archon-questions` YAML. No DB/schema changes, no workflow-engine API changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/plans/2026-04-20-slack-scoping-questions-form-plan.md` at line 24, The plan's dependency note ("No new packages, no DB/schema changes, no workflow-engine API changes.") is inaccurate because the PR adds the js-yaml package; update that sentence to reflect the new dependency (for example, change "No new packages" to "Adds js-yaml for YAML parsing") and ensure the PR summary reference to js-yaml is mentioned so the plan matches the actual changes.docs/plans/2026-04-20-slack-scoping-questions-form-plan.md-348-356 (1)
348-356:⚠️ Potential issue | 🟡 MinorDon’t mark the manual Slack smoke test complete yet.
The PR objectives say manual end-to-end modal render/submit still needs verification, but this checklist marks it
[x]. Please leave it unchecked until the Slack workspace test is actually done.Suggested checklist fix
-- [x] **Step 3: Manual Slack smoke test** +- [ ] **Step 3: Manual Slack smoke test**🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/plans/2026-04-20-slack-scoping-questions-form-plan.md` around lines 348 - 356, The checklist incorrectly marks "Step 3: Manual Slack smoke test" as completed; update the checklist item by changing the checked box marker for that step from "[x]" to an unchecked "[ ]" so it remains pending until the Slack modal render/submit verification is actually performed (look for the header "Step 3: Manual Slack smoke test" and the manual script bullet list immediately below it to locate the exact line to edit).docs/plans/2026-04-17-slack-archie-feature-to-review-app-plan.md-901-903 (1)
901-903:⚠️ Potential issue | 🟡 MinorAdd a language to the Slack example fence.
The markdownlint hint is valid here; use
textfor the sample Slack message.📝 Suggested docs fix
-``` +```text `@archie` add a README badge linking to the docs site</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/plans/2026-04-17-slack-archie-feature-to-review-app-plan.mdaround
lines 901 - 903, The markdown code fence for the Slack example is missing a
language tag which triggers markdownlint; update the fenced block that contains
"@archie add a README badge linking to the docs site" to use a text language
specifier (i.e., replacewithtext) so the block becomes a plain-text
fenced code block and satisfies the lint rule.</details> </blockquote></details> <details> <summary>docs/specs/2026-04-17-slack-archie-feature-to-review-app-design.md-181-191 (1)</summary><blockquote> `181-191`: _⚠️ Potential issue_ | _🟡 Minor_ **Add a language to the progress-example fence.** The example is plain text; specifying `text` resolves the markdownlint warning. <details> <summary>📝 Suggested docs fix</summary> ```diff -``` +```text 🧠 Spec approved. Creating implementation plan... 🏗️ Plan ready. Spinning up worktree <branch> and implementing... ✅ Implementation passed local validation. Opening PR... ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/specs/2026-04-17-slack-archie-feature-to-review-app-design.mdaround
lines 181 - 191, The fenced example block containing the progress emojis (the
triple-backtick block showing "🧠 Spec approved..." through "🎉 Done. PR:
Review app: ") should specify a language to silence markdownlint; update
the opening fence fromtotext so it reads ```text and leave the content
unchanged.</details> </blockquote></details> <details> <summary>docs/plans/2026-04-17-slack-archie-feature-to-review-app-plan.md-7-9 (1)</summary><blockquote> `7-9`: _⚠️ Potential issue_ | _🟡 Minor_ **Update the plan to match the implemented architecture.** This plan still says there are no Slack adapter changes and lists `.ts` helper scripts, but this PR changes `packages/adapters/src/chat/slack/adapter.ts` and the workflow invokes `.js` scripts. Either update the plan or mark it as superseded so future agents don’t follow stale instructions. Also applies to: 17-27 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@docs/plans/2026-04-17-slack-archie-feature-to-review-app-plan.md` around lines 7 - 9, The plan text is out of date: it claims "no Slack adapter changes" and lists helper scripts as `.ts` while the PR modifies packages/adapters/src/chat/slack/adapter.ts and the workflow invokes `.archon/scripts/*.js`; update the plan (or mark it superseded) to reflect that the Slack adapter was changed (reference packages/adapters/src/chat/slack/adapter.ts) and that the workflow uses compiled `.js` helpers in `.archon/scripts/` (reference the workflow node that dispatches those scripts), ensuring the Architecture/Tech Stack section matches the actual implementation. ``` </details> </blockquote></details> <details> <summary>docs/specs/2026-04-17-slack-archie-feature-to-review-app-design.md-60-68 (1)</summary><blockquote> `60-68`: _⚠️ Potential issue_ | _🟡 Minor_ **Align the design doc with the implemented PR.** This doc still describes “no custom Slack adapter work,” config-driven `reviewApp` keys, the old workflow location, and script unit tests. The implementation now includes adapter changes, hardcoded review-app parameters, `.archon/workflows/defaults/...`, and different test coverage. Update these sections or add a clear “superseded by implementation” note. Also applies to: 72-89, 253-258, 284-300 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@docs/specs/2026-04-17-slack-archie-feature-to-review-app-design.md` around lines 60 - 68, Update the spec to reflect the actual implemented PR instead of the outdated “no custom Slack adapter” and config-driven details: change the Slack adapter section to describe the implemented adapter changes (referencing SlackAdapter.start and any adapter-specific behavior), replace mentions of config-driven reviewApp keys with the current hardcoded review-app parameters and point to the actual defaults location (.archon/workflows/defaults/...) instead of the old workflow location, and revise the tests section to reflect the actual test coverage and types (script unit tests vs. implemented test files); if you prefer not to fully sync the prose, add a clear “superseded by implementation” note in the affected paragraphs (including the ranges noted) stating which parts were changed in the implementation and where to find the current source of truth. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (4)</summary><blockquote> <details> <summary>.archon/scripts/ci-wait.js (2)</summary><blockquote> `7-11`: **Exit-code list ordering is confusing.** The doc block lists codes `0, 1, 3, 2` — please reorder to `0, 1, 2, 3` for readability. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.archon/scripts/ci-wait.js around lines 7 - 11, The exit-code documentation block is out of numeric order (currently lists 0, 1, 3, 2); update the comment so the exit codes read in ascending order 0, 1, 2, 3 — specifically reorder the lines in the doc block that list the codes so the "2 — bad args / missing gh" entry appears before the "3 — timeout reached before CI finished" entry to improve readability. ``` </details> --- `37-50`: **Consider a SIGKILL escalation if `gh` ignores SIGTERM.** On timeout you send `SIGTERM` and schedule `process.exit(3)` 2s later via an unref'd timer. If `gh` (or a child it spawned) ignores SIGTERM, the unref'd timer means the event loop can exit with the child still running in the background, or conversely the child keeps the loop alive past your intended deadline. A belt-and-suspenders approach: after 2s send `SIGKILL`, then `process.exit(3)` on a non-unref'd timer. <details> <summary>Proposed hardening</summary> ```diff - const timer = setTimeout(() => { - timedOut = true; - console.error(`\nCI wait timed out after ${Math.round(timeoutMs / 1000)}s`); - child.kill('SIGTERM'); - setTimeout(() => process.exit(3), 2000).unref(); - }, timeoutMs); - timer.unref(); + const timer = setTimeout(() => { + timedOut = true; + console.error(`\nCI wait timed out after ${Math.round(timeoutMs / 1000)}s`); + child.kill('SIGTERM'); + setTimeout(() => { + if (!child.killed) child.kill('SIGKILL'); + process.exit(3); + }, 2000); + }, timeoutMs); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.archon/scripts/ci-wait.js around lines 37 - 50, On timeout in the CI wait block (where spawn creates `child` and the `timer` sets `timedOut`), after sending `child.kill('SIGTERM')` add a second escalation timer that after ~2s sends `child.kill('SIGKILL')` if the process is still running, and use a non-unref'd timer to call `process.exit(3)` after the SIGKILL; also ensure you clear the SIGKILL/escalation timers if the `child` exits normally so you don't kill unrelated processes. Use the existing symbols (`child`, `timer`, `timedOut`, `timeoutMs`) to locate where to add the escalation and to wire up clearing logic. ``` </details> </blockquote></details> <details> <summary>package.json (1)</summary><blockquote> `48-52`: **Document rationale for the new `flatted` and `axios` overrides.** These overrides aren't mentioned in the PR summary (which focuses on the Slack scoping modal). Since transitive overrides can silently break subdependencies, please add a short comment or PR note clarifying why these versions are pinned (CVE mitigation? transitive conflict?) so the next maintainer doesn't have to archaeologize. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 48 - 52, Add a brief rationale for pinning "flatted" and "axios" in the package.json "overrides" block: explain whether these pins address a CVE, resolve a transitive-version conflict, or prevent breaking changes in a subdependency, and note the affected transitive package(s) and the minimum version that fixes the issue; place this explanation as a one- or two-line JSON comment-style note in the PR description and as a short adjacent comment in the commit/PR (since JSON doesn't support comments, add the rationale to the PR body and to the package.json change summary in the commit message), referencing the exact override keys "flatted" and "axios" so future maintainers can see why those specific versions were chosen. ``` </details> </blockquote></details> <details> <summary>packages/adapters/src/chat/slack/adapter.test.ts (1)</summary><blockquote> `34-51`: **Mock `views.open` and cover the button-click path.** The new tests register `action`/`view`, but the mock Slack client still lacks `client.views.open`, so the “Answer questions” click → modal open path is not exercised. Add a `views.open` mock and a test that invokes the captured `gate_answer_questions` action handler. <details> <summary>Suggested mock extension</summary> ```diff const mockAction = mock(() => {}); const mockView = mock(() => {}); +const mockViewsOpen = mock(() => Promise.resolve({ ok: true })); const mockApp = { client: { chat: { postMessage: mockPostMessage, }, + views: { + open: mockViewsOpen, + }, ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@packages/adapters/src/chat/slack/adapter.test.ts` around lines 34 - 51, The mock Slack client lacks a views.open stub so the “Answer questions” button → modal path isn't tested; add a mockViewsOpen (e.g., mockViewsOpen = mock(() => {})) and attach it at mockApp.client.views.open, then add a test that invokes the registered action handler for "gate_answer_questions" (the same captured action handler used for other actions in the test harness) and assert that mockViewsOpen was called, verifying the modal-open code path is executed; update existing mocks (mockAction/mockView/mockApp) to include client.views.open and add the new unit test that triggers gate_answer_questions. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.archon/commands/defaults/scout-consolidate-perf-plan.md:
- Around line 83-96: The consolidation step currently allows creating
$ARTIFACTS_DIR/plan.md with 0 tasks; add a hard guard in the consolidation logic
that reads routes.json and the collected profile-XX.md inputs (and any
plan-context.md produced by plan-setup) to detect the "zero usable profiles"
case (routes.json == [] OR all profiles are SKIPPED/empty). If that condition is
true, do NOT write $ARTIFACTS_DIR/plan.md, exit with a non-zero status, and emit
a clear failure message on stdout/stderr such as "No usable profiles found —
aborting plan generation" instead of "Plan written to $ARTIFACTS_DIR/plan.md
with {N} tasks." Ensure this check runs before any file write and references the
same artifacts (routes.json, profile-XX.md, plan-context.md,
$ARTIFACTS_DIR/plan.md) used elsewhere so callers get a deterministic failure
rather than an empty plan file.In @.archon/commands/defaults/scout-discover-routes.md:
- Around line 24-29: The current flow allows continuing without Scout data;
change the logic so the command first checks for
$ARTIFACTS_DIR/scout-endpoints-export.json and uses it if present, but if it is
not present then create an empty routes.json, write a clear error summary
(mention missing SCOUT_API_KEY and absence of scout endpoints export) into the
artifacts, and explicitly stop the workflow path (fail-closed) rather than
proceeding to call list_apps/get_app_endpoints; update any branching that
previously asked the user to “ask the user to set SCOUT_API_KEY” to this
fail-closed behavior and reference variables/commands like
$ARTIFACTS_DIR/scout-endpoints-export.json, routes.json, SCOUT_API_KEY,
$ARGUMENTS, list_apps, get_app_endpoints to locate where to change the logic.In @.archon/config.yaml:
- Around line 3-5: The config currently copies the entire .env via the
copyFiles: - .env setting which exposes all secrets into each worktree; change
this to only propagate SCOUT_API_KEY (either by creating and copying a minimal
.env.scout containing just SCOUT_API_KEY or by injecting SCOUT_API_KEY into the
worktree environment instead of copying the file), and if you keep copying
ensure the comment near copyFiles documents the blast radius and add a reliable
cleanup step to remove copied .env files from worktrees; focus your edits on the
copyFiles setting and surrounding comment in .archon/config.yaml and any
worktree creation/cleanup logic that handles copied files.In @.archon/workflows/defaults/archon-slack-feature-to-review-app.yaml:
- Around line 65-67: The rule that treats a reply containing "go" as approval is
too permissive and must be tightened: update the approval-matching logic in the
block that emits SPEC_APPROVED so it only matches explicit
phrases like "approved", "looks good", "ship it", or the full phrase "go ahead"
(use exact-string checks or a regex with word boundaries for these phrases)
instead of the bare token "go"; ensure the same change is applied to the
corresponding logic later referenced around lines 126-130 so only explicit
approvals trigger SPEC_APPROVED.- Around line 393-411: The announce-done node currently uses model: haiku with a
prompt that runsgh pr view(id: announce-done, depends_on:
[fetch-review-url]) which won't execute shell commands; instead create a
separate bash node (e.g., id: resolve-pr-url) that runsgh pr view --json url --jq .urland emits the URL as an output, then update announce-done to remove
the shell command from its prompt and reference the new node's output (e.g.,
$resolve-pr-url.output) and keep the original message template; ensure
announce-done still depends_on the new resolve-pr-url node so substitution
works.- Around line 384-387: The regex in fetch-review-app-url.js uses POSIX character
classes ([[:space:]]) which break when compiled with new RegExp(regexStr);
replace those classes with JavaScript-compatible whitespace tokens and properly
escape backslashes in the string. Update the pattern assigned to regexStr (and
any call sites) from something like
https://[^[:space:])]+\.review\.instrumentl\.com[^[:space:])]*to a
JS-compatible form such as
https://[^\\s)]+\\.review\\.instrumentl\\.com[^\\s)]*so new RegExp(regexStr)
compiles without SyntaxError.In @.cursor/hooks/state/continual-learning-index.json:
- Around line 1-11: The file .cursor/hooks/state/continual-learning-index.json
is runtime hook state and should not be committed; remove it from the PR
(unstage/remove the file from the repository) and add the containing directory
.cursor/hooks/state/ (or that specific filename) to .gitignore so future runtime
state isn’t tracked; ensure the commit only contains the .gitignore update and
the removal of continual-learning-index.json from the index to avoid
reintroducing the file.In
@packages/adapters/package.json:
- Line 22: Add the missing TypeScript types for js-yaml by adding
"@types/js-yaml" to the devDependencies in packages/adapters/package.json (next
to "js-yaml"), then run your package manager to update the lockfile; this
ensures imports of js-yaml in adapter.ts have proper typings instead of relying
on skipLibCheck.In
@packages/adapters/src/chat/slack/adapter.test.ts:
- Around line 641-660: The test suite should cover malformed option entries
(e.g. options: [{}] or entries missing stringvalue/label) and the adapter
should reject those entries so it falls back to the default actions; add the
regression test shown (a new test that calls adapter.sendMessage with a select
whose options list contains an object missingvalue) and modify the validation
in the Slack adapter (in adapter.ts, where sendMessage parses/question schema
and constructs select/checkbox options) to treat an options array as invalid
unless every entry has both label and value as strings, causing the same
fallback path used when options is absent.In
@packages/adapters/src/chat/slack/adapter.ts:
- Around line 774-777: The logs in the Slack adapter are recording raw Slack
user IDs (see uses of getLog().info with ids?.runId, ids?.nodeId, ctx.userId),
which is PII; before logging, replace ctx.userId with a masked/obfuscated form
(for example a deterministic hash or truncated/peg-and-hash like
slackUserMask(userId)) so the user-visible mention can remain unchanged but logs
never contain the raw ID. Update the logging calls around the gate approval flow
(the getLog().info calls that include ctx.userId, e.g., the
'slack.gate_approve_clicked' and the other similar log sites) to pass the masked
value instead of ctx.userId, and add/ reuse a small helper (e.g.,
maskSlackUserId or slackUserMask) to centralize the masking logic.- Around line 713-752: Button click and modal handlers currently bypass the
SLACK_ALLOWED_USER_IDS guard; add the same authorization check (use a shared
helper, e.g., authorizeSlackUser or isSlackUserAllowed) at the start of the
action handlers wired to app.action and the view handlers wired to app.view so
that handleGateClick, handleRequestChangesClick, handleAnswerQuestionsClick,
handleGateModalSubmit, and handleQuestionsModalSubmit are only invoked for
allowed users; when rejecting, ack the event, log the attempt via the adapter
logger with the user ID masked (do not emit the raw ID), and return without
dispatching synthetic input.- Around line 246-274: The isValidQuestionDefArray validator needs to be
hardened: inside isValidQuestionDefArray, besides current checks, ensure
question ids are unique (track seen ids), enforce that for 'select' and
'checkboxes' the options property is a non-empty array and each option is a
non-null object with string 'value' and 'label' properties (reject primitives,
nulls, missing fields, or empty option arrays), and treat unknown/malformed
option entries as validation failures; update the function (referencing
isValidQuestionDefArray, QuestionDef, QuestionType, and q.options) to log and
return false on these cases so malformed payloads never reach modal rendering.- Around line 785-800: The current webClient.chat.update call overwrites all
blocks, removing the original prompt/spec; change the update to preserve
existing message blocks by reading the original blocks from the message context
(e.g., ctx.originalBlocks or the message payload) and build a new blocks array
that filters out only the actions block (identify by block.type === 'actions' or
specific block_id if used) then append a context status block like the current
approval context; pass that reconstructed blocks array to webClient.chat.update
(keep channel, ts, and text as-is). Use webClient.chat.update and
ctx.messageTs/ctx.channel to locate and update the message.
Outside diff comments:
In @.claude/skills/archon/guides/setup.md:
- Around line 161-173: Section headings "### 5c: Verify Configuration" and "###
5d: Run Database Migrations (PostgreSQL only)" are misnumbered and should be
"4c" and "4d" because they live under Step 4; update the two headings to "###
4c: Verify Configuration" and "### 4d: Run Database Migrations (PostgreSQL
only)" and verify their surrounding context remains nested under Step 4 (before
"## Step 5") so the flow and references (e.g., the verification commandarchon versionand the PostgreSQL migration note) remain correct.In
@packages/workflows/src/defaults/bundled-defaults.ts:
- Around line 40-56: The top comment "Default Workflows (13 total)" in
defaults/bundled-defaults.ts is out of date because an additional import
(archonSlackFeatureToReviewAppWf) increases the total to 14; update that comment
to read "Default Workflows (14 total)" (look for the exact string "Default
Workflows (13 total)" at the top of the file and change it to 14).
Minor comments:
In @.archon/scripts/ci-wait.js:
- Around line 37-41: The spawn of the GitHub CLI ("gh") for "pr checks" using
the '--fail-fast' flag can fail on older gh versions and currently only spawn
errors are handled; update the logic around the child process created by spawn
(the variable child running
['pr','checks',pr,'--watch','--fail-fast','--interval','30']) to capture the
child's stderr/exit code, detect the "unknown flag" or "flag provided but not
defined" message (or exit code 1 with that stderr), and in that case retry or
fallback by re-spawning "gh pr checks" without the '--fail-fast' flag (or
alternatively run 'gh --version' and enforce a minimum version like 2.49.2),
ensuring you handle both the stderr message and non-zero exit codes so the
script distinguishes older gh from real CI failures.In @.archon/scripts/dispatch-review-app.js:
- Around line 34-38: The script currently forwards gh's stderr to stdout by
calling console.log(stderr.trim()), which mixes warnings with the JSON payload;
update the stderr passthrough to use console.error(stderr.trim()) instead so
that stderr goes to fd 2 while leaving the JSON line (JSON.stringify({
dispatched: true, workflow: workflowFile, ref })) on stdout; locate the block
handling stdout and stderr (variables stdout and stderr) in
dispatch-review-app.js and replace the console.log call for stderr with
console.error.In @.archon/scripts/fetch-review-app-url.js:
- Around line 60-68: The user-supplied regex (regex created from regexStr) can
be pathological and block the poll loop when run against large comment bodies;
to fix, cap the input before executing the regex by truncating c.body to a safe
MAX_COMMENT_LENGTH (use a named constant) and run regex.test or regex.exec
against that truncated haystack instead of the full c.body, and optionally
document the max length and that regexes should be linear-time; locate uses of
regex and references to c.body in the poll loop and replace them to use the
truncated haystack variable.In @.cursor/mcp.json:
- Around line 5-9: The SCOUT_API_KEY is being provided three ways (args,
env.SCOUT_API_KEY, envFile) which causes envFile to be overridden by an empty
${env:SCOUT_API_KEY}; remove the explicit env entry and let envFile supply the
value: delete the "SCOUT_API_KEY": "${env:SCOUT_API_KEY}" entry from the env
block so that envFile (envFile) provides SCOUT_API_KEY while keeping the args
["run", "--rm", "-i", "--env", "SCOUT_API_KEY", ...] behavior; alternatively, if
you prefer shell-provided values, remove envFile instead—pick one source and
remove the conflicting env mapping to prevent empty overrides.In
@docs/plans/2026-04-17-slack-archie-feature-to-review-app-plan.md:
- Around line 901-903: The markdown code fence for the Slack example is missing
a language tag which triggers markdownlint; update the fenced block that
contains "@archie add a README badge linking to the docs site" to use a text
language specifier (i.e., replacewithtext) so the block becomes a
plain-text fenced code block and satisfies the lint rule.- Around line 7-9: The plan text is out of date: it claims "no Slack adapter
changes" and lists helper scripts as.tswhile the PR modifies
packages/adapters/src/chat/slack/adapter.ts and the workflow invokes
.archon/scripts/*.js; update the plan (or mark it superseded) to reflect that
the Slack adapter was changed (reference
packages/adapters/src/chat/slack/adapter.ts) and that the workflow uses compiled
.jshelpers in.archon/scripts/(reference the workflow node that dispatches
those scripts), ensuring the Architecture/Tech Stack section matches the actual
implementation.In
@docs/plans/2026-04-20-slack-scoping-questions-form-plan.md:
- Line 24: The plan's dependency note ("No new packages, no DB/schema changes,
no workflow-engine API changes.") is inaccurate because the PR adds the js-yaml
package; update that sentence to reflect the new dependency (for example, change
"No new packages" to "Adds js-yaml for YAML parsing") and ensure the PR summary
reference to js-yaml is mentioned so the plan matches the actual changes.- Around line 348-356: The checklist incorrectly marks "Step 3: Manual Slack
smoke test" as completed; update the checklist item by changing the checked box
marker for that step from "[x]" to an unchecked "[ ]" so it remains pending
until the Slack modal render/submit verification is actually performed (look for
the header "Step 3: Manual Slack smoke test" and the manual script bullet list
immediately below it to locate the exact line to edit).In
@docs/specs/2026-04-17-slack-archie-feature-to-review-app-design.md:
- Around line 181-191: The fenced example block containing the progress emojis
(the triple-backtick block showing "🧠 Spec approved..." through "🎉 Done. PR:
Review app: ") should specify a language to silence markdownlint;
update the opening fence fromtotext so it reads ```text and leave the
content unchanged.- Around line 60-68: Update the spec to reflect the actual implemented PR
instead of the outdated “no custom Slack adapter” and config-driven details:
change the Slack adapter section to describe the implemented adapter changes
(referencing SlackAdapter.start and any adapter-specific behavior), replace
mentions of config-driven reviewApp keys with the current hardcoded review-app
parameters and point to the actual defaults location
(.archon/workflows/defaults/...) instead of the old workflow location, and
revise the tests section to reflect the actual test coverage and types (script
unit tests vs. implemented test files); if you prefer not to fully sync the
prose, add a clear “superseded by implementation” note in the affected
paragraphs (including the ranges noted) stating which parts were changed in the
implementation and where to find the current source of truth.
Nitpick comments:
In @.archon/scripts/ci-wait.js:
- Around line 7-11: The exit-code documentation block is out of numeric order
(currently lists 0, 1, 3, 2); update the comment so the exit codes read in
ascending order 0, 1, 2, 3 — specifically reorder the lines in the doc block
that list the codes so the "2 — bad args / missing gh" entry appears before the
"3 — timeout reached before CI finished" entry to improve readability.- Around line 37-50: On timeout in the CI wait block (where spawn creates
childand thetimersetstimedOut), after sendingchild.kill('SIGTERM')
add a second escalation timer that after ~2s sendschild.kill('SIGKILL')if
the process is still running, and use a non-unref'd timer to call
process.exit(3)after the SIGKILL; also ensure you clear the
SIGKILL/escalation timers if thechildexits normally so you don't kill
unrelated processes. Use the existing symbols (child,timer,timedOut,
timeoutMs) to locate where to add the escalation and to wire up clearing
logic.In
@package.json:
- Around line 48-52: Add a brief rationale for pinning "flatted" and "axios" in
the package.json "overrides" block: explain whether these pins address a CVE,
resolve a transitive-version conflict, or prevent breaking changes in a
subdependency, and note the affected transitive package(s) and the minimum
version that fixes the issue; place this explanation as a one- or two-line JSON
comment-style note in the PR description and as a short adjacent comment in the
commit/PR (since JSON doesn't support comments, add the rationale to the PR body
and to the package.json change summary in the commit message), referencing the
exact override keys "flatted" and "axios" so future maintainers can see why
those specific versions were chosen.In
@packages/adapters/src/chat/slack/adapter.test.ts:
- Around line 34-51: The mock Slack client lacks a views.open stub so the
“Answer questions” button → modal path isn't tested; add a mockViewsOpen (e.g.,
mockViewsOpen = mock(() => {})) and attach it at mockApp.client.views.open, then
add a test that invokes the registered action handler for
"gate_answer_questions" (the same captured action handler used for other actions
in the test harness) and assert that mockViewsOpen was called, verifying the
modal-open code path is executed; update existing mocks
(mockAction/mockView/mockApp) to include client.views.open and add the new unit
test that triggers gate_answer_questions.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `93525061-caf5-465d-91aa-a495290080d4` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 5ed38dc76571f2553253d1bd9b083fbeced1e331 and 721b8bb1fe294878cc3979081a42bb8ac89cc34d. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `bun.lock` is excluded by `!**/*.lock` </details> <details> <summary>📒 Files selected for processing (30)</summary> * `.archon/commands/defaults/scout-consolidate-perf-plan.md` * `.archon/commands/defaults/scout-discover-routes.md` * `.archon/config.yaml` * `.archon/scripts/ci-wait.js` * `.archon/scripts/dispatch-review-app.js` * `.archon/scripts/fetch-review-app-url.js` * `.archon/workflows/defaults/archon-scout-perf-roadmap.yaml` * `.archon/workflows/defaults/archon-slack-feature-to-review-app.yaml` * `.claude/skills/archon/guides/setup.md` * `.claude/skills/archon/guides/slack.md` * `.cursor/hooks/state/continual-learning-index.json` * `.cursor/hooks/state/continual-learning.json` * `.cursor/mcp.json` * `AGENTS.md` * `docs/plans/2026-04-17-slack-archie-feature-to-review-app-plan.md` * `docs/plans/2026-04-20-slack-scoping-questions-form-plan.md` * `docs/specs/2026-04-17-slack-archie-feature-to-review-app-design.md` * `package.json` * `packages/adapters/package.json` * `packages/adapters/src/chat/slack/adapter.test.ts` * `packages/adapters/src/chat/slack/adapter.ts` * `packages/cli/src/commands/setup.ts` * `packages/core/src/types/index.ts` * `packages/docs-web/src/content/docs/adapters/slack.md` * `packages/server/src/index.ts` * `packages/workflows/src/dag-executor.test.ts` * `packages/workflows/src/dag-executor.ts` * `packages/workflows/src/defaults/bundled-defaults.test.ts` * `packages/workflows/src/defaults/bundled-defaults.ts` * `packages/workflows/src/deps.ts` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+83
to
+96
| ## Rules | ||
|
|
||
| 1. **Deduplicate** overlapping tasks if multiple profiles touch the same file. | ||
| 2. **Order** tasks by dependency (models before handlers, shared utils first). | ||
| 3. **Reference** actual symbols/files from the profile markdown files. | ||
| 4. If profiles disagree, prefer the most evidence-backed recommendation and note the conflict in **Risks**. | ||
| 5. Ignore profiles that are SKIPPED or empty. | ||
|
|
||
| --- | ||
|
|
||
| ## Output | ||
|
|
||
| - Write **`$ARTIFACTS_DIR/plan.md`** only (plan-setup will create `plan-context.md`). | ||
| - Stdout: `Plan written to $ARTIFACTS_DIR/plan.md with {N} tasks.` |
There was a problem hiding this comment.
Add a hard guard for zero usable profiles.
If routes.json is [] or every profile-XX.md is SKIPPED/empty, this can still write a valid-looking plan.md with 0 tasks, allowing the implementation pipeline to continue on an empty plan. Make that case explicit: do not produce plan.md; emit a clear failure message instead.
Suggested command contract addition
## Rules
1. **Deduplicate** overlapping tasks if multiple profiles touch the same file.
2. **Order** tasks by dependency (models before handlers, shared utils first).
3. **Reference** actual symbols/files from the profile markdown files.
4. If profiles disagree, prefer the most evidence-backed recommendation and note the conflict in **Risks**.
5. Ignore profiles that are SKIPPED or empty.
+6. If there are **zero** usable profiles or `routes.json` is empty, do not write `plan.md`; print `ERROR: no Scout routes/profiles available to consolidate` and stop.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ## Rules | |
| 1. **Deduplicate** overlapping tasks if multiple profiles touch the same file. | |
| 2. **Order** tasks by dependency (models before handlers, shared utils first). | |
| 3. **Reference** actual symbols/files from the profile markdown files. | |
| 4. If profiles disagree, prefer the most evidence-backed recommendation and note the conflict in **Risks**. | |
| 5. Ignore profiles that are SKIPPED or empty. | |
| --- | |
| ## Output | |
| - Write **`$ARTIFACTS_DIR/plan.md`** only (plan-setup will create `plan-context.md`). | |
| - Stdout: `Plan written to $ARTIFACTS_DIR/plan.md with {N} tasks.` | |
| ## Rules | |
| 1. **Deduplicate** overlapping tasks if multiple profiles touch the same file. | |
| 2. **Order** tasks by dependency (models before handlers, shared utils first). | |
| 3. **Reference** actual symbols/files from the profile markdown files. | |
| 4. If profiles disagree, prefer the most evidence-backed recommendation and note the conflict in **Risks**. | |
| 5. Ignore profiles that are SKIPPED or empty. | |
| 6. If there are **zero** usable profiles or `routes.json` is empty, do not write `plan.md`; print `ERROR: no Scout routes/profiles available to consolidate` and stop. | |
| --- | |
| ## Output | |
| - Write **`$ARTIFACTS_DIR/plan.md`** only (plan-setup will create `plan-context.md`). | |
| - Stdout: `Plan written to $ARTIFACTS_DIR/plan.md with {N} tasks.` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.archon/commands/defaults/scout-consolidate-perf-plan.md around lines 83 -
96, The consolidation step currently allows creating $ARTIFACTS_DIR/plan.md with
0 tasks; add a hard guard in the consolidation logic that reads routes.json and
the collected profile-XX.md inputs (and any plan-context.md produced by
plan-setup) to detect the "zero usable profiles" case (routes.json == [] OR all
profiles are SKIPPED/empty). If that condition is true, do NOT write
$ARTIFACTS_DIR/plan.md, exit with a non-zero status, and emit a clear failure
message on stdout/stderr such as "No usable profiles found — aborting plan
generation" instead of "Plan written to $ARTIFACTS_DIR/plan.md with {N} tasks."
Ensure this check runs before any file write and references the same artifacts
(routes.json, profile-XX.md, plan-context.md, $ARTIFACTS_DIR/plan.md) used
elsewhere so callers get a deterministic failure rather than an empty plan file.
Comment on lines
+24
to
+29
| ## Prerequisites | ||
|
|
||
| 1. **MCP**: Scout tools should be available (`list_apps`, `get_app_endpoints`, `get_endpoint_metrics`, etc.). If MCP is unavailable, ask the user to set `SCOUT_API_KEY`, ensure Docker can run `scoutapp/scout-mcp-local`, or paste a Scout endpoints export into `$ARTIFACTS_DIR/scout-endpoints-export.json` (array of endpoint objects) and continue from that file. | ||
|
|
||
| 2. **App selection**: If `$ARGUMENTS` names an app or numeric id, use that. Otherwise call `list_apps` and pick the production app that matches this repo (name/hostname) or the most recently active app. State which app you chose. | ||
|
|
There was a problem hiding this comment.
Fail closed when Scout input is unavailable.
This runs before the interactive review gate, so “ask the user to set SCOUT_API_KEY or paste an export” can complete without producing routes.json. Prefer: use $ARTIFACTS_DIR/scout-endpoints-export.json if it already exists; otherwise write an empty routes.json plus an error summary and stop the workflow path explicitly.
Suggested prompt contract
-1. **MCP**: Scout tools should be available (`list_apps`, `get_app_endpoints`, `get_endpoint_metrics`, etc.). If MCP is unavailable, ask the user to set `SCOUT_API_KEY`, ensure Docker can run `scoutapp/scout-mcp-local`, or paste a Scout endpoints export into `$ARTIFACTS_DIR/scout-endpoints-export.json` (array of endpoint objects) and continue from that file.
+1. **MCP**: Scout tools should be available (`list_apps`, `get_app_endpoints`, `get_endpoint_metrics`, etc.). If MCP is unavailable, first check whether `$ARTIFACTS_DIR/scout-endpoints-export.json` already exists and continue from that file. If it does not exist, write `routes.json` as `[]`, write `routes-summary.md` with setup instructions, print `ERROR: Scout MCP unavailable and no endpoints export found`, and stop without inventing routes.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ## Prerequisites | |
| 1. **MCP**: Scout tools should be available (`list_apps`, `get_app_endpoints`, `get_endpoint_metrics`, etc.). If MCP is unavailable, ask the user to set `SCOUT_API_KEY`, ensure Docker can run `scoutapp/scout-mcp-local`, or paste a Scout endpoints export into `$ARTIFACTS_DIR/scout-endpoints-export.json` (array of endpoint objects) and continue from that file. | |
| 2. **App selection**: If `$ARGUMENTS` names an app or numeric id, use that. Otherwise call `list_apps` and pick the production app that matches this repo (name/hostname) or the most recently active app. State which app you chose. | |
| ## Prerequisites | |
| 1. **MCP**: Scout tools should be available (`list_apps`, `get_app_endpoints`, `get_endpoint_metrics`, etc.). If MCP is unavailable, first check whether `$ARTIFACTS_DIR/scout-endpoints-export.json` already exists and continue from that file. If it does not exist, write `routes.json` as `[]`, write `routes-summary.md` with setup instructions, print `ERROR: Scout MCP unavailable and no endpoints export found`, and stop without inventing routes. | |
| 2. **App selection**: If `$ARGUMENTS` names an app or numeric id, use that. Otherwise call `list_apps` and pick the production app that matches this repo (name/hostname) or the most recently active app. State which app you chose. | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.archon/commands/defaults/scout-discover-routes.md around lines 24 - 29, The
current flow allows continuing without Scout data; change the logic so the
command first checks for $ARTIFACTS_DIR/scout-endpoints-export.json and uses it
if present, but if it is not present then create an empty routes.json, write a
clear error summary (mention missing SCOUT_API_KEY and absence of scout
endpoints export) into the artifacts, and explicitly stop the workflow path
(fail-closed) rather than proceeding to call list_apps/get_app_endpoints; update
any branching that previously asked the user to “ask the user to set
SCOUT_API_KEY” to this fail-closed behavior and reference variables/commands
like $ARTIFACTS_DIR/scout-endpoints-export.json, routes.json, SCOUT_API_KEY,
$ARGUMENTS, list_apps, get_app_endpoints to locate where to change the logic.
Comment on lines
+3
to
+5
| # Copy local .env into isolated worktrees so Scout MCP sees SCOUT_API_KEY when cwd is ~/.archon/workspaces/... | ||
| copyFiles: | ||
| - .env |
There was a problem hiding this comment.
Copying .env into every worktree propagates all secrets, not just SCOUT_API_KEY.
The comment explains this is needed so Scout MCP sees SCOUT_API_KEY when Archon runs from ~/.archon/workspaces/.... But copyFiles: [.env] copies the whole file — SLACK_BOT_TOKEN, DB credentials, API keys for every provider — into each worktree directory, which then lives under ~/.archon/workspaces/... and may be picked up by unrelated subprocesses, backed up, or leaked via logs/artifacts if a workflow later archives the worktree. Consider:
- Using a narrower mechanism (e.g. a
.env.scoutfile with onlySCOUT_API_KEY, or passing the var through the worktree env rather than copying the file), or - Documenting the blast radius in this comment and ensuring worktree cleanup reliably removes
.envcopies.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.archon/config.yaml around lines 3 - 5, The config currently copies the
entire .env via the copyFiles: - .env setting which exposes all secrets into
each worktree; change this to only propagate SCOUT_API_KEY (either by creating
and copying a minimal .env.scout containing just SCOUT_API_KEY or by injecting
SCOUT_API_KEY into the worktree environment instead of copying the file), and if
you keep copying ensure the comment near copyFiles documents the blast radius
and add a reliable cleanup step to remove copied .env files from worktrees;
focus your edits on the copyFiles setting and surrounding comment in
.archon/config.yaml and any worktree creation/cleanup logic that handles copied
files.
Comment on lines
+65
to
+67
| 4. If the user's latest reply EXPLICITLY approves (contains | ||
| "approved", "looks good", "ship it", "go"), emit | ||
| <promise>SPEC_APPROVED</promise> and stop. Otherwise, revise the |
There was a problem hiding this comment.
Avoid treating bare go as approval.
contains "go" can match non-approval feedback like “where does this go?” or “don’t go ahead yet,” which could prematurely start implementation. Require explicit phrases such as approved, looks good, ship it, or go ahead.
✅ Suggested prompt tightening
- 4. If the user's latest reply EXPLICITLY approves (contains
- "approved", "looks good", "ship it", "go"), emit
+ 4. If the user's latest reply EXPLICITLY approves (contains
+ "approved", "looks good", "ship it", "go ahead", or equivalent
+ affirmative approval), emit- (contains "approved", "looks good", "ship it", "go"):
+ (contains "approved", "looks good", "ship it", "go ahead", or
+ equivalent affirmative approval):Also applies to: 126-130
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.archon/workflows/defaults/archon-slack-feature-to-review-app.yaml around
lines 65 - 67, The rule that treats a reply containing "go" as approval is too
permissive and must be tightened: update the approval-matching logic in the
block that emits <promise>SPEC_APPROVED</promise> so it only matches explicit
phrases like "approved", "looks good", "ship it", or the full phrase "go ahead"
(use exact-string checks or a regex with word boundaries for these phrases)
instead of the bare token "go"; ensure the same change is applied to the
corresponding logic later referenced around lines 126-130 so only explicit
approvals trigger SPEC_APPROVED.
Comment on lines
+384
to
+387
| bun .archon/scripts/fetch-review-app-url.js \ | ||
| "$extract-pr-number.output" \ | ||
| 'https://[^[:space:])]+\.review\.instrumentl\.com[^[:space:])]*' \ | ||
| 900000 20000 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
node <<'NODE'
const url = 'https://pr-123.review.instrumentl.com/path';
const current = new RegExp('https://[^[:space:])]+\\.review\\.instrumentl\\.com[^[:space:])]*');
const fixed = new RegExp('https://[^\\s)]+\\.review\\.instrumentl\\.com[^\\s)]*');
console.log({ currentMatches: current.test(url), fixedMatches: fixed.test(url) });
NODERepository: coleam00/Archon
Length of output: 802
🏁 Script executed:
fd -t f fetch-review-app-url.jsRepository: coleam00/Archon
Length of output: 41
🏁 Script executed:
fd -t f "archon-slack-feature-to-review-app.yaml"Repository: coleam00/Archon
Length of output: 41
🏁 Script executed:
fd -t f -H . .archon/ 2>/dev/null | head -30Repository: coleam00/Archon
Length of output: 1645
🏁 Script executed:
fd -t f "\.yaml$" .archon/ 2>/dev/null | head -30Repository: coleam00/Archon
Length of output: 1439
🏁 Script executed:
wc -l .archon/workflows/defaults/archon-slack-feature-to-review-app.yamlRepository: coleam00/Archon
Length of output: 129
🏁 Script executed:
sed -n '384,387p' .archon/workflows/defaults/archon-slack-feature-to-review-app.yamlRepository: coleam00/Archon
Length of output: 244
🏁 Script executed:
cat -n .archon/scripts/fetch-review-app-url.jsRepository: coleam00/Archon
Length of output: 3729
🏁 Script executed:
node <<'NODE'
const url = 'https://pr-123.review.instrumentl.com/path';
const fixed = new RegExp('https://[^\\s)]+\\.review\\.instrumentl\\.com[^\\s)]*');
console.log({ urlInput: url, fixedMatches: fixed.test(url) });
NODERepository: coleam00/Archon
Length of output: 141
Fix JavaScript-incompatible whitespace syntax in the review-app URL regex.
The pattern https://[^[:space:])]+\.review\.instrumentl\.com[^[:space:])]* contains POSIX character classes ([[:space:]]), which JavaScript's RegExp does not support. In line 62 of fetch-review-app-url.js, the regex string is compiled with new RegExp(regexStr), which throws a SyntaxError: Unmatched ')' because [^[:space:])] is parsed as a complete character class followed by an unmatched ). This breaks the workflow entirely.
Replace with JavaScript-compatible syntax:
Regex fix
- 'https://[^[:space:])]+\.review\.instrumentl\.com[^[:space:])]*' \
+ 'https://[^\s)]+\.review\.instrumentl\.com[^\s)]*' \📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| bun .archon/scripts/fetch-review-app-url.js \ | |
| "$extract-pr-number.output" \ | |
| 'https://[^[:space:])]+\.review\.instrumentl\.com[^[:space:])]*' \ | |
| 900000 20000 | |
| bun .archon/scripts/fetch-review-app-url.js \ | |
| "$extract-pr-number.output" \ | |
| 'https://[^\s)]+\.review\.instrumentl\.com[^\s)]*' \ | |
| 900000 20000 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.archon/workflows/defaults/archon-slack-feature-to-review-app.yaml around
lines 384 - 387, The regex in fetch-review-app-url.js uses POSIX character
classes (`[[:space:]]`) which break when compiled with new RegExp(regexStr);
replace those classes with JavaScript-compatible whitespace tokens and properly
escape backslashes in the string. Update the pattern assigned to regexStr (and
any call sites) from something like
`https://[^[:space:])]+\.review\.instrumentl\.com[^[:space:])]*` to a
JS-compatible form such as
`https://[^\\s)]+\\.review\\.instrumentl\\.com[^\\s)]*` so new RegExp(regexStr)
compiles without SyntaxError.
Comment on lines
+641
to
+660
| test('falls back when select type is missing options', async () => { | ||
| const noOptions = [ | ||
| '```archon-questions', | ||
| '- id: choose', | ||
| ' type: select', | ||
| ' label: "Pick"', | ||
| '```', | ||
| ].join('\n'); | ||
| await adapter.sendMessage('C123:1234.5678', noOptions, { | ||
| interactiveGate: { runId: 'run-no-opt', nodeId: 'spec' }, | ||
| }); | ||
|
|
||
| const call = (mockPostMessage as Mock<typeof mockPostMessage>).mock.calls[0][0] as { | ||
| blocks: unknown[]; | ||
| }; | ||
| const actionsBlock = call.blocks[1] as { | ||
| elements: Array<{ text: { text: string } }>; | ||
| }; | ||
| expect(actionsBlock.elements[0].text.text).toBe('Approve'); | ||
| }); |
There was a problem hiding this comment.
Cover malformed option entries, not just missing options.
The fallback tests only check absent options. A schema like options: [{}] still passes the current “array exists” shape check from adapter.ts, but it cannot produce valid Slack select/checkbox options. Add a regression test and reject option entries without string value and label.
Suggested test case
test('falls back when select type is missing options', async () => {
const noOptions = [
'```archon-questions',
'- id: choose',
' type: select',
' label: "Pick"',
'```',
].join('\n');
await adapter.sendMessage('C123:1234.5678', noOptions, {
interactiveGate: { runId: 'run-no-opt', nodeId: 'spec' },
});
const call = (mockPostMessage as Mock<typeof mockPostMessage>).mock.calls[0][0] as {
blocks: unknown[];
};
const actionsBlock = call.blocks[1] as {
elements: Array<{ text: { text: string } }>;
};
expect(actionsBlock.elements[0].text.text).toBe('Approve');
});
+
+ test('falls back when option entries are missing value or label', async () => {
+ const malformedOptions = [
+ '```archon-questions',
+ '- id: choose',
+ ' type: select',
+ ' label: "Pick"',
+ ' options:',
+ ' - { label: "Missing value" }',
+ '```',
+ ].join('\n');
+ await adapter.sendMessage('C123:1234.5678', malformedOptions, {
+ interactiveGate: { runId: 'run-bad-opt', nodeId: 'spec' },
+ });
+
+ const call = (mockPostMessage as Mock<typeof mockPostMessage>).mock.calls.at(-1)?.[0] as {
+ blocks: unknown[];
+ };
+ const actionsBlock = call.blocks[1] as {
+ elements: Array<{ text: { text: string } }>;
+ };
+ expect(actionsBlock.elements[0].text.text).toBe('Approve');
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/adapters/src/chat/slack/adapter.test.ts` around lines 641 - 660, The
test suite should cover malformed option entries (e.g. options: [{}] or entries
missing string `value`/`label`) and the adapter should reject those entries so
it falls back to the default actions; add the regression test shown (a new test
that calls adapter.sendMessage with a select whose options list contains an
object missing `value`) and modify the validation in the Slack adapter (in
adapter.ts, where sendMessage parses/question schema and constructs
select/checkbox options) to treat an options array as invalid unless every entry
has both label and value as strings, causing the same fallback path used when
options is absent.
Comment on lines
+246
to
+274
| private isValidQuestionDefArray(value: unknown): value is QuestionDef[] { | ||
| if (!Array.isArray(value) || value.length === 0) { | ||
| getLog().warn({ reason: 'not a non-empty array' }, 'slack.questions_schema_invalid'); | ||
| return false; | ||
| } | ||
| const validTypes: QuestionType[] = ['yes_no', 'yes_no_text', 'select', 'checkboxes', 'text']; | ||
| for (const item of value) { | ||
| if (typeof item !== 'object' || item === null) { | ||
| getLog().warn({ reason: 'item is not an object' }, 'slack.questions_schema_invalid'); | ||
| return false; | ||
| } | ||
| const q = item as Record<string, unknown>; | ||
| if (typeof q.id !== 'string' || typeof q.label !== 'string') { | ||
| getLog().warn({ reason: 'missing id or label' }, 'slack.questions_schema_invalid'); | ||
| return false; | ||
| } | ||
| if (!validTypes.includes(q.type as QuestionType)) { | ||
| getLog().warn( | ||
| { reason: `unknown type: ${String(q.type)}` }, | ||
| 'slack.questions_schema_invalid' | ||
| ); | ||
| return false; | ||
| } | ||
| if ((q.type === 'select' || q.type === 'checkboxes') && !Array.isArray(q.options)) { | ||
| getLog().warn({ reason: `${q.type} missing options` }, 'slack.questions_schema_invalid'); | ||
| return false; | ||
| } | ||
| } | ||
| return true; |
There was a problem hiding this comment.
Reject malformed option payloads before rendering the modal.
select/checkboxes only verify that options is an array. Entries like null, strings, missing value, missing label, duplicate IDs, or empty option arrays pass validation and then render invalid modal blocks instead of falling back to the safe approve/request-changes gate.
✅ Suggested validation hardening
+ const seenIds = new Set<string>();
for (const item of value) {
if (typeof item !== 'object' || item === null) {
getLog().warn({ reason: 'item is not an object' }, 'slack.questions_schema_invalid');
return false;
}
const q = item as Record<string, unknown>;
- if (typeof q.id !== 'string' || typeof q.label !== 'string') {
+ if (typeof q.id !== 'string' || q.id.length === 0 || typeof q.label !== 'string') {
getLog().warn({ reason: 'missing id or label' }, 'slack.questions_schema_invalid');
return false;
}
+ if (seenIds.has(q.id)) {
+ getLog().warn({ reason: `duplicate id: ${q.id}` }, 'slack.questions_schema_invalid');
+ return false;
+ }
+ seenIds.add(q.id);
if (!validTypes.includes(q.type as QuestionType)) {
getLog().warn(
{ reason: `unknown type: ${String(q.type)}` },
'slack.questions_schema_invalid'
);
return false;
}
- if ((q.type === 'select' || q.type === 'checkboxes') && !Array.isArray(q.options)) {
- getLog().warn({ reason: `${q.type} missing options` }, 'slack.questions_schema_invalid');
- return false;
+ if (q.type === 'select' || q.type === 'checkboxes') {
+ if (!Array.isArray(q.options) || q.options.length === 0) {
+ getLog().warn({ reason: `${q.type} missing options` }, 'slack.questions_schema_invalid');
+ return false;
+ }
+ for (const option of q.options) {
+ if (
+ typeof option !== 'object' ||
+ option === null ||
+ typeof (option as Record<string, unknown>).value !== 'string' ||
+ typeof (option as Record<string, unknown>).label !== 'string'
+ ) {
+ getLog().warn({ reason: `${q.type} has invalid option` }, 'slack.questions_schema_invalid');
+ return false;
+ }
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private isValidQuestionDefArray(value: unknown): value is QuestionDef[] { | |
| if (!Array.isArray(value) || value.length === 0) { | |
| getLog().warn({ reason: 'not a non-empty array' }, 'slack.questions_schema_invalid'); | |
| return false; | |
| } | |
| const validTypes: QuestionType[] = ['yes_no', 'yes_no_text', 'select', 'checkboxes', 'text']; | |
| for (const item of value) { | |
| if (typeof item !== 'object' || item === null) { | |
| getLog().warn({ reason: 'item is not an object' }, 'slack.questions_schema_invalid'); | |
| return false; | |
| } | |
| const q = item as Record<string, unknown>; | |
| if (typeof q.id !== 'string' || typeof q.label !== 'string') { | |
| getLog().warn({ reason: 'missing id or label' }, 'slack.questions_schema_invalid'); | |
| return false; | |
| } | |
| if (!validTypes.includes(q.type as QuestionType)) { | |
| getLog().warn( | |
| { reason: `unknown type: ${String(q.type)}` }, | |
| 'slack.questions_schema_invalid' | |
| ); | |
| return false; | |
| } | |
| if ((q.type === 'select' || q.type === 'checkboxes') && !Array.isArray(q.options)) { | |
| getLog().warn({ reason: `${q.type} missing options` }, 'slack.questions_schema_invalid'); | |
| return false; | |
| } | |
| } | |
| return true; | |
| private isValidQuestionDefArray(value: unknown): value is QuestionDef[] { | |
| if (!Array.isArray(value) || value.length === 0) { | |
| getLog().warn({ reason: 'not a non-empty array' }, 'slack.questions_schema_invalid'); | |
| return false; | |
| } | |
| const validTypes: QuestionType[] = ['yes_no', 'yes_no_text', 'select', 'checkboxes', 'text']; | |
| const seenIds = new Set<string>(); | |
| for (const item of value) { | |
| if (typeof item !== 'object' || item === null) { | |
| getLog().warn({ reason: 'item is not an object' }, 'slack.questions_schema_invalid'); | |
| return false; | |
| } | |
| const q = item as Record<string, unknown>; | |
| if (typeof q.id !== 'string' || q.id.length === 0 || typeof q.label !== 'string') { | |
| getLog().warn({ reason: 'missing id or label' }, 'slack.questions_schema_invalid'); | |
| return false; | |
| } | |
| if (seenIds.has(q.id)) { | |
| getLog().warn({ reason: `duplicate id: ${q.id}` }, 'slack.questions_schema_invalid'); | |
| return false; | |
| } | |
| seenIds.add(q.id); | |
| if (!validTypes.includes(q.type as QuestionType)) { | |
| getLog().warn( | |
| { reason: `unknown type: ${String(q.type)}` }, | |
| 'slack.questions_schema_invalid' | |
| ); | |
| return false; | |
| } | |
| if (q.type === 'select' || q.type === 'checkboxes') { | |
| if (!Array.isArray(q.options) || q.options.length === 0) { | |
| getLog().warn({ reason: `${q.type} missing options` }, 'slack.questions_schema_invalid'); | |
| return false; | |
| } | |
| for (const option of q.options) { | |
| if ( | |
| typeof option !== 'object' || | |
| option === null || | |
| typeof (option as Record<string, unknown>).value !== 'string' || | |
| typeof (option as Record<string, unknown>).label !== 'string' | |
| ) { | |
| getLog().warn({ reason: `${q.type} has invalid option` }, 'slack.questions_schema_invalid'); | |
| return false; | |
| } | |
| } | |
| } | |
| } | |
| return true; | |
| } |
🤖 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 246 - 274, The
isValidQuestionDefArray validator needs to be hardened: inside
isValidQuestionDefArray, besides current checks, ensure question ids are unique
(track seen ids), enforce that for 'select' and 'checkboxes' the options
property is a non-empty array and each option is a non-null object with string
'value' and 'label' properties (reject primitives, nulls, missing fields, or
empty option arrays), and treat unknown/malformed option entries as validation
failures; update the function (referencing isValidQuestionDefArray, QuestionDef,
QuestionType, and q.options) to log and return false on these cases so malformed
payloads never reach modal rendering.
Comment on lines
+713
to
+752
| async ({ ack, body, action, client }) => { | ||
| await ack(); | ||
| await this.handleGateClick({ | ||
| body, | ||
| action, | ||
| client, | ||
| verb: 'approve', | ||
| }); | ||
| } | ||
| ); | ||
|
|
||
| // Request changes button — opens a modal to collect feedback text. | ||
| this.app.action( | ||
| { type: 'block_actions', action_id: new RegExp(`^${GATE_ACTION_REQUEST_CHANGES}\\|`) }, | ||
| async ({ ack, body, action, client }) => { | ||
| await ack(); | ||
| await this.handleRequestChangesClick({ body, action, client }); | ||
| } | ||
| ); | ||
|
|
||
| // Answer questions button — opens a modal with typed inputs. | ||
| this.app.action( | ||
| { type: 'block_actions', action_id: new RegExp(`^${GATE_ACTION_ANSWER_QUESTIONS}\\|`) }, | ||
| async ({ ack, body, action, client }) => { | ||
| await ack(); | ||
| await this.handleAnswerQuestionsClick({ body, action, client }); | ||
| } | ||
| ); | ||
|
|
||
| // Modal submission — feedback text is synthesized as a thread message. | ||
| this.app.view(GATE_MODAL_CALLBACK, async ({ ack, view, body }) => { | ||
| await ack(); | ||
| await this.handleGateModalSubmit({ view, body }); | ||
| }); | ||
|
|
||
| // Questions modal submission — answers formatted and synthesized as thread message. | ||
| this.app.view(QUESTIONS_MODAL_CALLBACK, async ({ ack, view, body }) => { | ||
| await ack(); | ||
| await this.handleQuestionsModalSubmit({ view, body }); | ||
| }); |
There was a problem hiding this comment.
Authorize interactive gate clicks and modal submits before dispatching synthetic input.
The app_mention and DM paths enforce SLACK_ALLOWED_USER_IDS, but button clicks and modal submissions currently bypass that check. Anyone who can see the Slack message can approve, request changes, or submit answers into the workflow.
🛡️ Suggested authorization helper
+ private getAuthorizedInteractionUser(body: unknown, eventName: string): string | null {
+ const userId = (body as { user?: { id?: string } }).user?.id;
+ if (!isSlackUserAuthorized(userId, this.allowedUserIds)) {
+ const maskedId = userId ? `${userId.slice(0, 4)}***` : 'unknown';
+ getLog().info({ maskedUserId: maskedId }, eventName);
+ return null;
+ }
+ return userId ?? null;
+ }
+
this.app.action(
{ type: 'block_actions', action_id: new RegExp(`^${GATE_ACTION_APPROVE}\\|`) },
async ({ ack, body, action, client }) => {
await ack();
+ if (!this.getAuthorizedInteractionUser(body, 'slack.unauthorized_gate_action')) return;
await this.handleGateClick({
body,
action,Apply the same guard to request-changes, answer-questions, and both modal submit handlers.
As per coding guidelines, packages/adapters/src/**/*.{ts,tsx} adapters must perform authorization checks inside adapters and log unauthorized auth attempts with masked user IDs.
Also applies to: 886-914, 985-1028
🧰 Tools
🪛 ast-grep (0.42.1)
[warning] 725-725: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${GATE_ACTION_REQUEST_CHANGES}\\|)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 734-734: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${GATE_ACTION_ANSWER_QUESTIONS}\\|)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🤖 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 713 - 752, Button
click and modal handlers currently bypass the SLACK_ALLOWED_USER_IDS guard; add
the same authorization check (use a shared helper, e.g., authorizeSlackUser or
isSlackUserAllowed) at the start of the action handlers wired to app.action and
the view handlers wired to app.view so that handleGateClick,
handleRequestChangesClick, handleAnswerQuestionsClick, handleGateModalSubmit,
and handleQuestionsModalSubmit are only invoked for allowed users; when
rejecting, ack the event, log the attempt via the adapter logger with the user
ID masked (do not emit the raw ID), and return without dispatching synthetic
input.
Comment on lines
+774
to
+777
| getLog().info( | ||
| { runId: ids?.runId, nodeId: ids?.nodeId, userId: ctx.userId }, | ||
| 'slack.gate_approve_clicked' | ||
| ); |
There was a problem hiding this comment.
Mask Slack user IDs in gate logs.
These info logs include raw Slack user IDs. Keep the Slack mention in the user-visible message if needed, but log only a masked identifier.
🔐 Suggested log masking
+ const maskedUserId = `${ctx.userId.slice(0, 4)}***`;
getLog().info(
- { runId: ids?.runId, nodeId: ids?.nodeId, userId: ctx.userId },
+ { runId: ids?.runId, nodeId: ids?.nodeId, maskedUserId },
'slack.gate_approve_clicked'
);As per coding guidelines, never log API keys, tokens, user message content, or PII.
Also applies to: 832-835, 943-951
🤖 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 774 - 777, The logs
in the Slack adapter are recording raw Slack user IDs (see uses of getLog().info
with ids?.runId, ids?.nodeId, ctx.userId), which is PII; before logging, replace
ctx.userId with a masked/obfuscated form (for example a deterministic hash or
truncated/peg-and-hash like slackUserMask(userId)) so the user-visible mention
can remain unchanged but logs never contain the raw ID. Update the logging calls
around the gate approval flow (the getLog().info calls that include ctx.userId,
e.g., the 'slack.gate_approve_clicked' and the other similar log sites) to pass
the masked value instead of ctx.userId, and add/ reuse a small helper (e.g.,
maskSlackUserId or slackUserMask) to centralize the masking logic.
Comment on lines
+785
to
+800
| await webClient.chat.update({ | ||
| channel: ctx.channel, | ||
| ts: ctx.messageTs, | ||
| text: `Approved by <@${ctx.userId}>`, | ||
| blocks: [ | ||
| { | ||
| type: 'context', | ||
| elements: [ | ||
| { | ||
| type: 'mrkdwn', | ||
| text: `:white_check_mark: Approved by <@${ctx.userId}>`, | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| }); |
There was a problem hiding this comment.
Preserve the original message blocks when disabling the approve button.
chat.update replaces the full blocks array, so approving a gate will erase the original prompt/spec and leave only the approval context. Filter out just the actions block and append the status context instead.
💬 Suggested block-preserving update
+ const existingBlocks =
+ (body as { message?: { blocks?: SlackBlock[] } }).message?.blocks ?? [];
+ const blocksWithoutGateActions = existingBlocks.filter(block => {
+ const blockId = typeof block.block_id === 'string' ? block.block_id : '';
+ return !blockId.startsWith('gate_block|') && !blockId.startsWith('gate_questions_block|');
+ });
await webClient.chat.update({
channel: ctx.channel,
ts: ctx.messageTs,
text: `Approved by <@${ctx.userId}>`,
- blocks: [
+ blocks: [
+ ...blocksWithoutGateActions,
{
type: 'context',
elements: [🤖 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 785 - 800, The
current webClient.chat.update call overwrites all blocks, removing the original
prompt/spec; change the update to preserve existing message blocks by reading
the original blocks from the message context (e.g., ctx.originalBlocks or the
message payload) and build a new blocks array that filters out only the actions
block (identify by block.type === 'actions' or specific block_id if used) then
append a context status block like the current approval context; pass that
reconstructed blocks array to webClient.chat.update (keep channel, ts, and text
as-is). Use webClient.chat.update and ctx.messageTs/ctx.channel to locate and
update the message.
Author
|
Closing - reopening against instrumentl/Archon fork |
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
1 participant
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
archon-slack-feature-to-review-appinto a Slack modal form with typed inputs (yes/no, checkboxes, select, text)archon-questionsfenced YAML schema on first spec iteration; Slack adapter parses it, renders "Answer questions" button, opens modal on click, and synthesizes formatted answers back into$LOOP_USER_INPUTChanges
.archon/workflows/defaults/archon-slack-feature-to-review-app.yaml— Updated spec loop first-iteration instructions to emit structured question schemapackages/adapters/src/chat/slack/adapter.ts— Added question parsing, modal rendering, answer formatting, and click/submit handlerspackages/adapters/src/chat/slack/adapter.test.ts— 8 new tests covering valid schema rendering, fallback paths, regression, and answer formattingpackages/adapters/package.json— Addedjs-yamldependencyTest plan
bun run cli validate workflows archon-slack-feature-to-review-app)🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests