feat(run-links): add per-project run links in agent comments#790
Conversation
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Well-structured feature with clean layering from DB through config to runtime. One correctness issue in the GitHub ack path and a maintainability note on duplicated code.
Code Issues
Should Fix
-
src/router/adapters/github.ts:279-300 — Run link missing from actual GitHub PR ack comment
The GitHub PR ack comment is posted without the run link (via
postGitHubPRAck→postGitHubAckon line 279-288), then the returnedmessagefield is updated with the run link after the comment is already on GitHub (lines 289-300). The actual comment the user sees on the PR will not contain the run link — only the internal metadata is updated.Compare with:
- The PM-focused path in the same file (lines 274-276) which correctly appends the link before calling
postPMAck - The Trello adapter which appends the link before calling
postTrelloAck - The JIRA adapter which appends the link before calling
postJiraAck
Fix: Pass the run link config into
postGitHubPRAckso it can include the footer in the message before posting, or generate the message with the footer before callingpostGitHubPRAck. - The PM-focused path in the same file (lines 274-276) which correctly appends the link before calling
-
Gadget files —
buildRunLinkFooter()duplicated 4 timesThe
buildRunLinkFooter()function is copy-pasted in:src/gadgets/github/core/createPR.tssrc/gadgets/github/core/createPRReview.tssrc/gadgets/github/core/postPRComment.tssrc/gadgets/pm/core/postComment.ts(with minorworkItemIdparam variation)
All 4 copies read the same set of env vars and call the same
buildRunLink/buildWorkItemRunsLinkfunctions. Consider extracting a sharedbuildRunLinkFooterFromEnv(workItemId?: string)insrc/utils/runLink.tsto keep the env-var-reading logic in one place and prevent drift during future changes.
Everything Else Looks Good
- DB migration, schema, Drizzle model, and journal entry are all consistent
- Config mapper, projects repository, and Zod schema are properly extended
- Progress monitor's
setRunId()and footer-building logic are correct - Env-var bridge for subprocess agents (claude-code/codex) is well-designed
- CLI flag uses
allowNo: truefor clean--run-links-enabled/--no-run-links-enabledUX - Dashboard checkbox is properly wired
- Unit tests cover all exported functions with good edge cases
- All CI checks pass
…cate buildRunLinkFooter - Fix correctness bug where GitHub PR ack comment was posted without the run link footer. Previously the link was appended to the internal AckResult.message *after* the comment was already on GitHub, so users never saw it. Now the message is built with the run link before calling postGitHubPRAck, matching the Trello/JIRA/PM-focused paths. - Extract shared buildRunLinkFooterFromEnv(workItemId?) to src/utils/runLink.ts and replace the 4 identical copies in createPR.ts, createPRReview.ts, postPRComment.ts, and postComment.ts, preventing future drift. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Thanks for the detailed review @nhopeatall! Both issues are addressed in the latest commit. Issue 1 (GitHub PR ack missing run link): Fixed the correctness bug. The message is now built with the run link footer before calling Issue 2 (duplicated |
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Clean, well-structured feature addition. The run links implementation follows existing patterns consistently across all layers (DB schema → config mapper → Zod schema → API → CLI → Dashboard UI → adapter/gadget runtime). The feature is properly gated (off by default, graceful no-op when CASCADE_DASHBOARD_URL is unset), and the env-var bridge for subprocess agents is well thought out.
All 5 CI checks pass. No blocking or should-fix issues found.
Minor Observations (non-blocking)
-
Test coverage gap for
buildRunLinkFooterFromEnv: This is the primary integration function used by all 4 gadgets (createPR, createPRReview, postPRComment, postComment), but the test file only covers the pure utility functions it delegates to. Adding tests for the env-var-driven orchestration logic would improve confidence in the subprocess integration path. -
Redundant
extractGitHubContextcall in GitHub adapter: WhenrunLinksEnabled && event.workItemIdis true,postAckcallsextractGitHubContext+generateAckMessageto buildgithubAckMessage, then passes it asmessageOverridetopostGitHubPRAck— which also callsextractGitHubContext(though the result is discarded sincemessageOverridetakes precedence). Not a bug, just a minor inefficiency.
Summary
Adds an optional per-project feature (
run_links_enabled) that appends subtle dashboard links to all agent comments — progress updates, ack messages, PR bodies, PR reviews, and PM/GitHub comments — making it easy to navigate directly to the relevant run for debugging.Card: https://trello.com/c/uWTDE4mD/312-lets-add-an-optional-flag-on-per-project-basis-off-by-default-that-will-make-all-out-agent-comments-in-both-scm-and-pm-tools-cod
What was implemented
run_links_enabled BOOLEAN DEFAULT falsetoprojectstable (migration0037_add_run_links.sql)runLinksEnabledsurfaces throughconfigMapper, ZodProjectConfigSchema,projectsRepository, and API router (create/update endpoints)src/utils/runLink.tswithbuildRunLink(),buildWorkItemRunsLink(),shortenModelName(), andgetDashboardUrl()(readsCASCADE_DASHBOARD_URLenv var)ProgressMonitorConfiggets arunLinkoptional field; monitor appends footer on each tick;setRunId()method lets adapter inject the run ID once it's createdbuildProgressMonitorConfigpassesrunLinkconfig when enabled; injectsCASCADE_RUN_LINKS_ENABLED,CASCADE_DASHBOARD_URL,CASCADE_ENGINE_LABEL,CASCADE_MODEL,CASCADE_PROJECT_ID,CASCADE_WORK_ITEM_ID,CASCADE_RUN_IDenv vars into subprocess secrets for native-tool backendsbuildWorkItemRunsLink()footer to ack comments whenrunLinksEnabledis truepostComment,postPRComment,createPR,createPRReviewread env vars and append run link footer whenCASCADE_RUN_LINKS_ENABLED=true--run-links-enabled/--no-run-links-enabledflag tocascade projects updatesrc/utils/runLink.tscovering all exported functionsLink format
At ack time (before runId exists), links point to the work-item-runs page instead.
Configuration
CASCADE_DASHBOARD_URL=https://your-dashboard.comin the deployment environmentcascade projects update <id> --run-links-enabledKey decisions
run_links_enabledmust be explicitly enabled per projectCASCADE_DASHBOARD_URLis unset orrun_links_enabledis false, no links are generated and no errors occurCASCADE_RUN_LINKS_ENABLEDetc.)Test plan
buildRunLink,buildWorkItemRunsLink,shortenModelName,getDashboardUrl— all 20 passingCASCADE_DASHBOARD_URL, enable on project, trigger a run, verify links in ack/progress/PR comments🤖 Generated with Claude Code