fix(security): redact raw error messages from public PR comments - #90
Conversation
📝 WalkthroughWalkthroughSeparates public GitHub tracking comments from internal operator error details, adds ChangesError Handling and Transient Quota Recovery
CI Cleanup
Sequence DiagramsequenceDiagram
participant CW as Child Workflow
participant O as Orchestrator
participant DB as workflow_runs DB
participant V as Valkey (tickle queue)
CW->>DB: mark run failed with<br/>state.failedReason: "Anthropic ... resets 6pm (UTC)"
O->>DB: onStepComplete reads child run state
O->>O: extractFailedReason(state)
O->>O: detectTransientQuotaError(reason) → {retryAtMs, resetPhrase}
alt Quota detected
O->>V: ZADD ship:tickle retryAtMs intent_id
V-->>O: scheduled
O-->>O: return early (defer retry)
else Not quota
O->>O: continue skip_failed_child path
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
bbf1871 to
aa21ee3
Compare
There was a problem hiding this comment.
Pull request overview
Sanitizes bot-authored public GitHub comments to avoid leaking raw upstream error strings (including Octokit token-bearing URLs), while preserving detailed errors for operator-only surfaces and adding orchestrator support to auto-defer ship retries when an Anthropic quota-reset time is detected.
Changes:
- Add
ExecutionResult.errorMessageand propagate raw failure detail through executor/pipeline and workflow handlers for internal storage/logging. - Redact raw error strings from all public comment write paths (tracking comments, ship halt messages, ephemeral spawn rejection, open-pr scoped comments).
- Add orchestrator helpers to detect transient quota errors and schedule a deferred
ship:tickleretry at the reset boundary (with unit tests).
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types.ts | Extends ExecutionResult with errorMessage to carry internal failure detail. |
| src/core/executor.ts | Populates errorMessage for thrown and non-throw SDK terminal outcomes. |
| src/core/pipeline.ts | Ensures public tracking comment doesn’t receive raw error text while returning errorMessage to callers. |
| src/workflows/handlers/review.ts | Uses errorMessage for internal reason and a safe humanMessage for public comments. |
| src/workflows/handlers/implement.ts | Same internal/public split on failure as review handler. |
| src/workflows/handlers/resolve.ts | Same internal/public split on failure as review handler. |
| src/daemon/workflow-executor.ts | Defense-in-depth: prevents defaulting public humanMessage to raw reason (incl. uncaught throws). |
| src/workflows/orchestrator.ts | Stops inlining raw failure reasons into ship halt messages; adds quota-reset detection + deferred tickle scheduling. |
| src/webhook/router.ts | Removes spawnError interpolation from the public infra-unavailable rejection comment. |
| src/workflows/ship/scoped/open-pr.ts | Removes raw error_message interpolation from public open-pr scoped comments. |
| test/workflows/orchestrator.test.ts | Adds unit tests for pure quota-detection helpers (extractFailedReason, detectTransientQuotaError). |
| test/webhook/router.test.ts | Updates assertion to ensure the public comment does not include raw spawnError. |
9d85147 to
1a15c13
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/webhook/router.ts (1)
35-36:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate stale JSDoc on
spawnError— it now incorrectly says the field is "surfaced in the tracking comment"The comment reads:
/** Set when `reason === "ephemeral-spawn-failed"` — surfaced in the tracking comment. */This PR explicitly removes
spawnErrorfrom the public tracking comment. A developer reading this JSDoc later may assume surfacing the raw error text in a public comment is the intended behaviour and unintentionally reintroduce the token-leak vector.📝 Proposed fix
- /** Set when `reason === "ephemeral-spawn-failed"` — surfaced in the tracking comment. */ + /** Set when `reason === "ephemeral-spawn-failed"`. Retained for operator-side surfaces + * (structured logs, executions row) only — never interpolated into public GitHub comments. */ spawnError?: string;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/webhook/router.ts` around lines 35 - 36, The JSDoc for the spawnError field is stale and incorrectly claims the error text is "surfaced in the tracking comment"; update the comment on the spawnError property (the spawnError field in the router type/interface) to reflect that this value is not exposed publicly and is intended for internal logging/troubleshooting only (remove any mention of being surfaced in tracking comments or public output) so future contributors won't reintroduce token-leak behavior.src/workflows/handlers/implement.ts (1)
156-160:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSame outer
catchgap asreview.ts— raw Octokit error text inreasonwithouthumanMessage.
octokit.rest.issues.get(line 43) andoctokit.rest.repos.get(line 55) can throw OctokitRequestErrors whose.messageembedshttps://x-access-token:GHS_xxx@…. That string ends up inreasonwith nohumanMessage. Apply the same fix as therunPipelinefailure path above.🔒 Proposed fix
} catch (err) { const message = err instanceof Error ? err.message : String(err); log.warn({ err }, "implement handler caught error"); - return { status: "failed", reason: `implement failed: ${message}` }; + return { + status: "failed", + reason: `implement failed: ${message}`, + humanMessage: "implement pipeline execution failed — see server logs for details.", + }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/workflows/handlers/implement.ts` around lines 156 - 160, The catch block in implement.ts returns raw err.message (which can include sensitive Octokit token URLs); change the returned reason to prefer a sanitized human message when available: use the same pattern as the runPipeline failure path by extracting a humanMessage from the error (e.g., const humanMessage = (err as any)?.humanMessage ?? (err instanceof Error ? err.message : String(err))) and return { status: "failed", reason: `implement failed: ${humanMessage}` } while keeping the existing log.warn({ err }, "implement handler caught error") for diagnostics.src/workflows/handlers/resolve.ts (1)
206-210: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueCatch block still exposes raw error message in the public
reasonfield.The
runPipelinefailure path (lines 152-160) correctly separates internal/public messages, but the outer catch block at line 209 returnsreason: \resolve failed: ${message}`without ahumanMessageoverride. If this exception path is reached, the daemon's default fallback will use a safe message, but thereasonfield (which gets persisted tostate.failedReason) will contain raw error text — which is correct. However, for consistency and defense-in-depth, consider adding an explicithumanMessage` here too.🛡️ Optional: add explicit humanMessage to catch block
} catch (err) { const message = err instanceof Error ? err.message : String(err); log.warn({ err }, "resolve handler caught error"); - return { status: "failed", reason: `resolve failed: ${message}` }; + return { + status: "failed", + reason: `resolve failed: ${message}`, + humanMessage: "resolve failed — see server logs for details.", + }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/workflows/handlers/resolve.ts` around lines 206 - 210, The outer catch in the resolve handler currently returns { status: "failed", reason: `resolve failed: ${message}` } exposing raw error text; update the catch to include an explicit humanMessage (e.g., "Resolve failed, please try again" or similar safe user-facing string) alongside the existing reason so internal details remain in reason while humanMessage provides a sanitized message for consumers; modify the catch block that defines message and calls log.warn (the block catching err in the resolve handler) to return both reason and humanMessage consistent with runPipeline's failure return shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/webhook/router.ts`:
- Around line 35-36: The JSDoc for the spawnError field is stale and incorrectly
claims the error text is "surfaced in the tracking comment"; update the comment
on the spawnError property (the spawnError field in the router type/interface)
to reflect that this value is not exposed publicly and is intended for internal
logging/troubleshooting only (remove any mention of being surfaced in tracking
comments or public output) so future contributors won't reintroduce token-leak
behavior.
In `@src/workflows/handlers/implement.ts`:
- Around line 156-160: The catch block in implement.ts returns raw err.message
(which can include sensitive Octokit token URLs); change the returned reason to
prefer a sanitized human message when available: use the same pattern as the
runPipeline failure path by extracting a humanMessage from the error (e.g.,
const humanMessage = (err as any)?.humanMessage ?? (err instanceof Error ?
err.message : String(err))) and return { status: "failed", reason: `implement
failed: ${humanMessage}` } while keeping the existing log.warn({ err },
"implement handler caught error") for diagnostics.
In `@src/workflows/handlers/resolve.ts`:
- Around line 206-210: The outer catch in the resolve handler currently returns
{ status: "failed", reason: `resolve failed: ${message}` } exposing raw error
text; update the catch to include an explicit humanMessage (e.g., "Resolve
failed, please try again" or similar safe user-facing string) alongside the
existing reason so internal details remain in reason while humanMessage provides
a sanitized message for consumers; modify the catch block that defines message
and calls log.warn (the block catching err in the resolve handler) to return
both reason and humanMessage consistent with runPipeline's failure return shape.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 56b0dfe9-443a-46fd-90ef-60bdf9b206e7
📒 Files selected for processing (18)
.github/workflows/dev-release.ymldocs/use/workflows/implement.mddocs/use/workflows/resolve.mddocs/use/workflows/review.mddocs/use/workflows/ship.mdsrc/core/executor.tssrc/core/pipeline.tssrc/daemon/workflow-executor.tssrc/types.tssrc/webhook/router.tssrc/workflows/handlers/implement.tssrc/workflows/handlers/resolve.tssrc/workflows/handlers/review.tssrc/workflows/orchestrator.tssrc/workflows/ship/scoped/open-pr.tstest/core/executor.test.tstest/webhook/router.test.tstest/workflows/orchestrator.test.ts
Tracking comments and bot-authored replies were inlining raw error strings (Agent SDK throws, octokit error stacks with the installation token in the request URL, K8s spawn exceptions) into bodies that GitHub renders publicly — a real token-leak vector. Sanitize every public sink while keeping the raw error on operator-only channels (pino logs, DB state.failedReason, executions.error_message, internal WS payloads). Also wires the orchestrator to auto-defer the ship iteration to the quota reset time when the SDK returns a transient usage-limit error (parses "resets 6pm UTC" via detectTransientQuotaError and ZADDs ship:tickle with score = reset unix ms), so the loop self-recovers instead of stalling until manual re-arming. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1a15c13 to
f82910c
Compare
|
Outside-diff findings from review 4213790770 addressed in f82910c:
Docs in |
|
bot workflow 🔍 Code review complete — 18 files, +461/-21. Review — PR #90: fix(security): redact raw error messages from public PR commentsSummaryLGTM with two non-blocking observations posted inline. The PR delivers on its stated security goal (token redaction from public GitHub comments) and adds a useful self-healing behaviour for transient Anthropic usage-limit errors. The two-channel separation — public What was checked
Findings[minor] Deferred-quota wake has no Postgres durability backstopPosted inline at The transient-quota path ZADDs This is the same single-storage failure mode as the existing immediate-tickle ZADD a few lines down, but the operator-visibility framing of this PR makes the gap more conspicuous: the parent workflow_run is already marked Suggested follow-up: persist Not a blocker for this PR's primary goal. [nit]
|
![chrisleekr-bot[bot]](https://avatars.githubusercontent.com/in/2890262?s=60&v=4){kind=small}
|
bot workflow 🔎 Resolve iteration complete — 0 failing checks, 5 open comment threads (some may already be resolved). Resolve — PR #90 (
|
| Check | Status |
|---|---|
| CI / Lint & Test | SUCCESS |
| CodeQL (actions, javascript-typescript) | SUCCESS |
| Dev Release / CI / Lint & Test | SUCCESS |
| Dev Release / Semantic Release (dev) | SUCCESS |
| Docker / Build orchestrator (amd64, arm64) | SUCCESS |
| Docker / Build daemon (amd64, arm64) | SUCCESS |
| Docker / Merge manifest (orchestrator, daemon) | SUCCESS |
| Docker / Trivy scan (×4 matrix entries) | SUCCESS |
| Docs / build | SUCCESS |
| Generate Labels | SUCCESS |
| Secrets Scan / Gitleaks (×2) | SUCCESS |
| CodeRabbit (status context) | SUCCESS |
| Trivy (action) | NEUTRAL (always neutral on this repo) |
No failing checks → no diagnose-and-fix attempt was required.
Review comments
| # | DB id | Path:line | Classification | Action |
|---|---|---|---|---|
| 1 | 3175509720 | src/workflows/orchestrator.ts (detectTransientQuotaError) |
Already resolved | Pre-existing reply on 1a15c13 — thread isResolved: true. No-op. |
| 2 | 3175509732 | src/workflows/orchestrator.ts:517 (ZADD test) |
Already resolved | Pre-existing reply on 1a15c13 — thread isResolved: true. No-op. |
| 3 | 3175509749 | src/core/executor.ts:247 (errorMessage in tests) |
Already resolved | Pre-existing reply on 1a15c13 — thread isResolved: true. No-op. |
| 4 | 3175628643 | src/workflows/orchestrator.ts:493 ([minor] Postgres durability backstop) |
Valid — out of scope for this PR | Reply 3175643543: rationale (bot itself flagged [minor] "Not a blocker"; persistContinuation has no callers yet so wiring it requires a state_blob.phase design call; same gap exists on the immediate-tickle path so deserves a dedicated follow-up issue covering both writes). |
| 5 | 3175628942 | src/workflows/orchestrator.ts:415 ([nit] regex parses resets 6 UTC as 06:00) |
Valid — fixed | Commit b3cccda adds a guard if (match[2] === undefined && match[3] === undefined) return null; so ambiguous bare-hour forms fall through to the +1h fallback instead of next-day 06:00:30. Regression test added in test/workflows/orchestrator.test.ts. Reply 3175643205 confirms the fix and links the new test. |
Commits pushed
b3cccda·fix(quota): reject ambiguous bare-hour resets clock as unparseable
Validation
bun run typecheck✅ (clean)bun run lint✅ (0 errors, 288 pre-existing warnings unchanged)bun run format✅ (prettier --check .all match)bun test test/workflows/orchestrator.test.ts✅ — 8 pass, 11 skip (DB-bound tests skip withoutDATABASE_URL), 0 fail. The new "treats ambiguous 'resets 6 UTC' (no am/pm, no minute) as fallback, not 06:00" passes alongside all existingdetectTransientQuotaErrorcases.
Outstanding
- Approval. PR has
reviewDecision: ""— the prior bot review posts inline findings but cannot submit an APPROVE decision. A human reviewer's APPROVE is the remaining merge gate. - Follow-up issue (not a blocker for this PR). Persist
wake_attoship_continuationson both the deferred-quota path (src/workflows/orchestrator.ts:493) and the immediate-tickle path (a few lines below) so the boot reconciler insrc/workflows/ship/tickle-scheduler.ts:51-64can self-heal Valkey loss. Requires a design call onstate_blob.phasefor a quota-wait state — flagged in the reply to comment 4.
cost: $3.1461 · turns: 45 · duration: 417s
![chrisleekr-bot[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
`parseResetsClock` matched "resets 6 UTC" (no am/pm, no minute) and parsed it as 06:00. If now was past 06:00, the next-day rollover at line 437 pushed the wake out by ~24h — strictly worse than the +1h fallback the unparseable branch produces. Anthropic's current copy always carries am/pm so this is unlikely in practice, but defends against future format drift. Addresses [nit] review comment on PR #90. Co-authored-by: chrisleekr-bot[bot] <chrisleekr-bot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/core/executor.test.ts (1)
98-198: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd test coverage for
buildExecutionResult's non-success SDK terminal path.The new
!successblock inbuildExecutionResult(executor.ts lines 290-300) has two branches —errors.length > 0→"SDK ${subtype}: …"and the fallback"SDK terminal subtype: …"— neither of which is asserted. TheemptyIteratorimplicitly hits the fallback (no result message →result = undefined→ subtype"unknown"), but the test doesn't assertresult.errorMessage. Theerrors-populated branch is never reached at all.Per the 90% per-file coverage guideline, these need explicit assertions. A minimal addition to the suite:
🧪 Suggested test additions
+ it("surfaces SDK terminal subtype when SDK returns non-success without errors", async () => { + nextIterator = (): AsyncIterableIterator<unknown> => { + let done = false; + return { + [Symbol.asyncIterator]() { return this; }, + next: () => { + if (done) return Promise.resolve({ value: undefined, done: true as const }); + done = true; + return Promise.resolve({ + value: { type: "result", subtype: "error_max_turns" }, + done: false, + }); + }, + return: () => Promise.resolve({ value: undefined, done: true as const }), + } as AsyncIterableIterator<unknown>; + }; + + const result = await executeAgent(baseParams()); + + expect(result.success).toBe(false); + expect(result.errorMessage).toBe("SDK terminal subtype: error_max_turns"); + }); + + it("includes SDK errors array in errorMessage when non-empty", async () => { + nextIterator = (): AsyncIterableIterator<unknown> => { + let done = false; + return { + [Symbol.asyncIterator]() { return this; }, + next: () => { + if (done) return Promise.resolve({ value: undefined, done: true as const }); + done = true; + return Promise.resolve({ + value: { + type: "result", + subtype: "error_usage_limit", + errors: ["You've hit your limit · resets 6pm (UTC)"], + }, + done: false, + }); + }, + return: () => Promise.resolve({ value: undefined, done: true as const }), + } as AsyncIterableIterator<unknown>; + }; + + const result = await executeAgent(baseParams()); + + expect(result.success).toBe(false); + expect(result.errorMessage).toBe( + "SDK error_usage_limit: You've hit your limit · resets 6pm (UTC)", + ); + });As per coding guidelines:
**/*.test.{ts,tsx}— maintain minimum 90% coverage threshold per-file (lines + functions).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/core/executor.test.ts` around lines 98 - 198, Add two unit tests in executor.test.ts to cover buildExecutionResult's non-success SDK terminal branches: (1) create a nextIterator that returns a terminal SDK result object with success=false and a non-empty errors array (and subtype like "response_error"), call executeAgent(baseParams()) and assert result.success is false and result.errorMessage starts with `SDK response_error:` and contains the first error message; (2) create a nextIterator that returns the empty/undefined-message terminal path (reuse emptyIterator or produce a terminal result with no message/errors so subtype becomes "unknown"), call executeAgent and assert result.errorMessage matches the fallback pattern `SDK terminal subtype: unknown` (or the exact fallback string used by buildExecutionResult). Reference the buildExecutionResult function and the nextIterator/emptyIterator helpers to locate where to add these assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/dev-release.yml:
- Around line 95-97: The current PATTERN uses a shell glob fed to git tag -l
which can backtrack and over-match unrelated tags; replace the glob-based lookup
with a pipeline that lists all tags and filters via an anchored extended regex
that includes BRANCH_SLUG literally. Concretely, change the STALE_TAGS
assignment to call git tag -l and pipe to grep -E using an anchored pattern like
"^v[[:alnum:].-]*-${BRANCH_SLUG}\." (escape the dot) so you only match tags that
end with "-${BRANCH_SLUG}." followed by the suffix; keep the fallback "|| true"
and update the variables PATTERN/STALE_TAGS usages accordingly.
In `@src/workflows/orchestrator.ts`:
- Around line 492-521: The deferred-quota branch currently schedules a ZADD via
requireValkeyClient/TICKLE_KEY before verifying the intent's terminal state; to
fix, run the same terminal-intent guard used on the success path (call
getIntentById(...) and isSessionTerminalState(...) for intentId) before calling
detectTransientQuotaError/requireValkeyClient and ZADD so you skip scheduling
for intents already in terminal states (use intentId and childRunId to
log/return early); keep the existing log.warn/ZADD error handling but ensure
ZADD only executes after the terminal-state check passes.
In `@test/workflows/orchestrator.test.ts`:
- Around line 686-737: The test only asserts the ZADD score is "in the future"
which doesn't ensure the parsed reset time logic is used; update the test around
onStepComplete/run creation to freeze the clock (e.g., mock Date.now or use
jest.useFakeTimers with a fixed base time) before calling
insertIntent/markFailed/onStepComplete, compute the exact expected next 18:00:30
UTC timestamp from that frozen time, and assert the ZADD score (mockValkeySend
call where c[0]==="ZADD" and c[1][0]==="ship:tickle") equals that computed
timestamp instead of just being > Date.now(); keep references to onStepComplete,
markFailed, insertIntent and mockValkeySend so the change locates the right
calls.
---
Outside diff comments:
In `@test/core/executor.test.ts`:
- Around line 98-198: Add two unit tests in executor.test.ts to cover
buildExecutionResult's non-success SDK terminal branches: (1) create a
nextIterator that returns a terminal SDK result object with success=false and a
non-empty errors array (and subtype like "response_error"), call
executeAgent(baseParams()) and assert result.success is false and
result.errorMessage starts with `SDK response_error:` and contains the first
error message; (2) create a nextIterator that returns the
empty/undefined-message terminal path (reuse emptyIterator or produce a terminal
result with no message/errors so subtype becomes "unknown"), call executeAgent
and assert result.errorMessage matches the fallback pattern `SDK terminal
subtype: unknown` (or the exact fallback string used by buildExecutionResult).
Reference the buildExecutionResult function and the nextIterator/emptyIterator
helpers to locate where to add these assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 47ef9b95-2dcb-421b-9b58-3c21af31da47
📒 Files selected for processing (18)
.github/workflows/dev-release.ymldocs/use/workflows/implement.mddocs/use/workflows/resolve.mddocs/use/workflows/review.mddocs/use/workflows/ship.mdsrc/core/executor.tssrc/core/pipeline.tssrc/daemon/workflow-executor.tssrc/types.tssrc/webhook/router.tssrc/workflows/handlers/implement.tssrc/workflows/handlers/resolve.tssrc/workflows/handlers/review.tssrc/workflows/orchestrator.tssrc/workflows/ship/scoped/open-pr.tstest/core/executor.test.tstest/webhook/router.test.tstest/workflows/orchestrator.test.ts
Three independent fixes from CodeRabbit's outside-diff review on b3cccda: 1. dev-release.yml: replace `git tag -l "v*-${SLUG}.*"` glob with an anchored ERE grep. fnmatch backtracking let the glob over-match sibling-branch tags (e.g. slug `fix-test` would also match `v0.4.0-fix-some-other-fix-test.1`), risking deletion of another branch's dev tags during semrel-dev cleanup. 2. orchestrator.ts: hoist the `getIntentById` / `isSessionTerminalState` guard above the failed-child branch. Previously the deferred-quota ZADD ran without checking terminal state, so a late quota-failed child could re-arm an already-aborted/merged intent and reprocess a session that should stay dead. Both the immediate cascade and the deferred retry now share the same guard. 3. orchestrator.test.ts (H2): freeze `Date.now` and assert the exact `18:00:30 UTC` deferred score instead of "any future timestamp". The looser check passed even if the parsed-reset path silently regressed to the +1h fallback or any other future score, defeating the test's stated intent.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/dev-release.yml:
- Around line 105-119: The current STALE_TAGS selection grabs all tags for the
branch slug and then deletes them, which removes tags that are still reachable
from HEAD; change the selection so STALE_TAGS contains only tags that match the
regex for ${BRANCH_SLUG} AND are not reachable from HEAD. Concretely, filter the
tag list returned by the regex against the set of tags reachable from HEAD
(e.g., using git tag --merged HEAD or git merge-base checks) and only pass those
unreachable tags into the existing deletion loop that uses git tag -d and git
push origin --delete; keep the rest of the deletion logic (echo, xargs, loop)
the same but operate on the filtered STALE_TAGS variable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 78281980-b013-4355-82c2-96206eb00513
📒 Files selected for processing (3)
.github/workflows/dev-release.ymlsrc/workflows/orchestrator.tstest/workflows/orchestrator.test.ts
# [1.8.0](v1.7.0...v1.8.0) (2026-05-02) ### Bug Fixes * **logger:** redact paths and scrub err.* before pino emits (closes [#52](#52)) ([#89](#89)) ([641f138](641f138)) * **security:** redact raw error messages from public PR comments ([#90](#90)) ([cc70949](cc70949)) ### Features * **workflows:** unify bot reply format and harden research/resolve guards ([#91](#91)) ([7d39fb4](7d39fb4))
|
🎉 This PR is included in version 1.8.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Summary
The bot's tracking comments and bot-authored PR/issue comments were inlining raw error strings (e.g.
Claude Code returned an error result: You've hit your limit · resets 6pm (UTC), octokit error stacks, K8s spawn exceptions) into bodies that GitHub renders publicly. Octokit error messages embed the request URL with the installation token (https://x-access-token:GHS_xxx@api.github.com/...), so this surface was a token-leak vector.This PR sanitizes every public-comment write path while still surfacing the raw error to operator-only channels (logs, DB
state.failedReason, internal WS payloads), and wires the orchestrator to auto-defer the ship iteration to the quota reset time when the SDK returns a transient usage-limit error (instead of stalling the intent until an operator re-arms it).Diagram
flowchart TD classDef public fill:#fde2e1,stroke:#c40000,color:#000 classDef internal fill:#dcefdc,stroke:#1b5e20,color:#000 classDef bad fill:#ffd6d6,stroke:#a40000,color:#000 SDK["Agent SDK throws<br/>You've hit your limit · resets 6pm UTC"]:::internal SDK --> Before["BEFORE — leaks raw error"]:::bad Before --> BeforeOps["operator log"]:::internal Before --> BeforePub["public PR comment<br/>review failed: review pipeline execution failed"]:::public SDK --> After["AFTER — split surfaces"]:::internal After --> AfterOps["state.failedReason DB column<br/>+ pino log<br/>+ ExecutionResult.errorMessage"]:::internal After --> AfterPub["public PR comment<br/>review pipeline execution failed — see server logs for details."]:::public AfterOps --> Tickle["orchestrator parses reset clock<br/>ZADD ship:tickle score=resetMs<br/>iteration auto-resumes at boundary"]:::internalChanges
Visibility — surface SDK errors to operator-only channels
src/types.ts—ExecutionResultcarrieserrorMessage?: string.src/core/executor.ts— both throw-path catch and non-throw SDK terminal subtypes (error_max_turns,error_max_budget_usd, etc.) populateerrorMessage.src/core/pipeline.tsouter-catch returnserrorMessage; the public tracking-comment finalize keeps the existing safe constant.src/workflows/handlers/{review,implement,resolve}.ts— propagateresult.errorMessageinto the failurereason(DBstate.failedReason) but explicitly set a safehumanMessageso the public comment never carries the raw text.Security — redact raw errors from every public sink
src/daemon/workflow-executor.ts— failure-branch fallbackhumanMessageno longer interpolatesresult.reason; uncaught-throw branch no longer interpolateserr.message(the highest-stakes leak — uncaught octokit errors carry the installation token in the URL).src/workflows/orchestrator.ts— failed-child cascadehumanMessageno longer interpolatesresult.reason.src/webhook/router.ts— ephemeral-spawn rejection comment no longer interpolatesdecision.spawnError(raw K8s API errors).src/workflows/ship/scoped/open-pr.ts— both classifier-failure and PR-create-failure replies drop the inlinederror_message.In every case the raw text is preserved in operator surfaces (pino logs,
workflow_runs.state.failedReason,executions.error_message, internal WS payloads, scoped-job-completion handler logs).Auto-recover on transient quota error
src/workflows/orchestrator.ts— adds two pure helpers:extractFailedReason(state)— readsstate.failedReasonwritten bymarkFailed.detectTransientQuotaError(reason, nowMs)— matches the Anthropic usage-limit signature and parsesresets <time> UTC. Falls back to+1hwhen the clock cannot be parsed.maybeEarlyWakeShipIntent— when a child failed with a transient quota signature, ZADDsship:ticklewith score = unix-ms of the reset boundary (instead of the existing skip-failed-child path). The periodic tickle scanner re-fires the intent once the quota resets.Tests
test/workflows/orchestrator.test.ts— 6 new pure-function tests forextractFailedReason+detectTransientQuotaError(parses6pm (UTC)and18:30 UTCforms, rolls past-boundary to next day, falls back to+1h, ignores unrelated reasons, ignores empty/undefined).test/webhook/router.test.ts— flipped the assertion frombody.toContain("api-unavailable: boom")(which was pinning the leak) tobody.not.toContain(...)so the test now validates the security property.Related Issues
Test plan
Summary by CodeRabbit
Bug Fixes
Documentation
Chores