fix(review): add policy-file fallback so comment-only policy reaches cascade-tools

[zbigniewsobiecki]

Summary

• CASCADE_REVIEW_EVENT_POLICY is injected into the Claude SDK env dict but filtered by claude.exe (bun v2.1.185) before bash subprocesses see it, so cascade-tools scm create-pr-review never received the comment-only policy
• Root cause: GITHUB_TOKEN and other credentials survive because they're also in the Docker container env (via buildWorkerEnvWithProjectId); CASCADE_REVIEW_EVENT_POLICY has no such fallback
• Fix: write the policy to /tmp/cascade-review-event-policy before the agent starts; cascade-tools reads this file as a fallback when the env var is absent

Changes

• src/config/reviewEventPolicy.ts — add REVIEW_EVENT_POLICY_FILE constant
• src/backends/secretOrchestrator.ts — write policy file when comment-only before agent starts
• src/cli/scm/create-pr-review.ts — read policy file as fallback in resolveEventPolicyFromEnv()
• tests/unit/cli/scm/create-pr-review.test.ts — 5 new tests covering file fallback resolution

Test plan

• All 269 unit test files pass
• Typecheck clean
• Deployed to production, custom worker image rebuilt (sha256:9ee8c767)
• Verify review run on target PR submits as COMMENT with advisory preamble

🤖 Generated with Claude Code

https://claude.ai/code/session_011ZaMpedeE8PXsix84cWsq1

[codecov]

Codecov Report

❌ Patch coverage is 92.59259% with 2 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/backends/secretOrchestrator.ts	75.00%	2 Missing ⚠️

📢 Thoughts on this report? Let us know!

[nhopeatall]

Summary

Verdict: COMMENT (no blocking issues). This is a dev → main promotion bundling three independent changes — the review-event-policy file fallback (the titled fix), a forward-looking JIRA authType schema field (MNG-1736), and the self-directed review_requested exemption. All 48 affected unit tests pass locally and tsc --noEmit is clean. The changes are sound; I have one maintainability gap worth addressing before the JIRA field is consumed, plus one optional hardening note.

Verified correct

• Self-directed review exemption — traced all four cases: self-directed persona (sender === requested_reviewer) → dispatch; cross-persona → skip (loop prevention preserved); non-persona self-request → skip with the not a cascade persona reason; human→persona → dispatch. The loop-safety claim holds — I grepped src/ for requestReviewers / request_reviewers / createReviewRequest and there are zero call sites, so every review_requested event is human-initiated by construction. The release-before-claim dedup at review-requested.ts:147 is pre-existing and correctly re-exercised by the new self-request path. Docs (CLAUDE.md, 03-trigger-system.md) match the code.
• Policy-file fallback — precedence is correct (env → file → all default); the file is written only for comment-only, and the CLI defaults to all when the file is absent, so the two halves are consistent.

Code Issues

Should Fix

• src/pm/config.ts / JIRA authType runtime plumbing gap — the field is added to jiraConfigSchema, JiraConfig, JiraCredentials, and the manifest fixture, but the runtime config-mapper drops it. buildJiraConfig in src/db/repositories/configMapper.ts:263 hand-picks fields (no authType), its local JiraIntegrationConfig interface (line 24) and ProjectConfigRaw['jira'] (line 139) omit it, and mapProjectRow returns ProjectConfigRaw directly (line 421) without re-parsing through ProjectConfigSchema. Net effect: ProjectConfig.jira.authType loaded from the DB is always undefined, regardless of what's persisted. The new ProjectConfig-level authType flow test only exercises ProjectConfigSchema.parse(...) — not the runtime mapper path — so it stays green while the real load path silently omits the field. Since the field is explicitly forward-looking ("later stories consume this"), nothing breaks today, but the future consumer will read undefined. Recommend threading authType through configMapper now, or documenting the deferral. (configMapper.ts isn't in this diff — inline note left on src/pm/config.ts.)

Nitpicks (non-blocking)

• src/backends/secretOrchestrator.ts — the policy file is written only for comment-only and never cleaned up. Safe under the one-job-per-ephemeral-container guarantee, but writing the resolved policy unconditionally would make it self-correcting and remove the reliance on /tmp never being reused. See inline note.
• tests/unit/cli/scm/create-pr-review.test.ts — the new tests write to the real fixed path /tmp/cascade-review-event-policy (unlike the sibling tests, which use tmpdir() + a unique name). This is inherent to the hardcoded-path design (it must survive env-stripping, so it can't be per-run), and the tests clean up in before/afterEach, but two concurrent vitest invocations on the same host would collide. Acceptable, just noting the smell.

🕵️ claude-code · claude-opus-4-8 · run details

[nhopeatall]

authType is added here (and to jiraConfigSchema, JiraCredentials, and the manifest fixture), but the runtime config-mapper drops it. buildJiraConfig in src/db/repositories/configMapper.ts:263 hand-picks fields and doesn't copy authType (its local JiraIntegrationConfig interface at line 24 and ProjectConfigRaw['jira'] at line 139 also omit it), and mapProjectRow returns ProjectConfigRaw directly (line 421) without re-parsing through ProjectConfigSchema. So ProjectConfig.jira.authType loaded from the DB is always undefined, regardless of what's persisted.

The new ProjectConfig-level authType flow test only exercises ProjectConfigSchema.parse(...), which isn't the runtime path — it passes green while the mapper silently omits the field. Since this is forward-looking ("later stories consume this"), nothing breaks today, but the later consumer will read undefined. Suggest adding authType to configMapper's JiraIntegrationConfig, ProjectConfigRaw['jira'], and buildJiraConfig now, or documenting the deferral so it isn't a surprise. Also note the src/jira/types.ts comment says it's "carried through withJiraCredentials" — no call site populates it yet, which is fine but slightly overstates current behavior.

[aaight]

Fixed — threaded authType through the runtime config-mapper so it survives the DB-load path.

src/db/repositories/configMapper.ts now carries it in the local JiraIntegrationConfig interface, in ProjectConfigRaw['jira'], and — critically — copies it in buildJiraConfig, which previously hand-picked fields and dropped authType before jiraConfigSchema re-parsed via validateConfig. So ProjectConfig.jira.authType now round-trips from the DB instead of always loading as undefined.

Added two mapper-level regression tests in configMapper.test.ts (preserves jira authType through the mapper / leaves jira authType undefined when not configured) that exercise the real mapProjectRow path, not just ProjectConfigSchema.parse.

Also reworded the src/jira/types.ts authType JSDoc so it no longer overstates: the field is reserved on JiraCredentials but no call site populates it yet (JiraIntegration.withCredentials still builds { email, apiToken, baseUrl } only).

[nhopeatall]

Optional hardening: the file is written only for comment-only and never removed. Safe under one-job-per-ephemeral-container, but if /tmp is ever reused across runs (non-container execution or a future change), an all run would read a stale comment-only file and incorrectly downgrade the review. Writing the resolved policy unconditionally (both all and comment-only) makes the file self-correcting and removes the reliance on container ephemerality — the CLI's env>file>default precedence still holds.

[aaight]

Done — the policy file is now written unconditionally with the run's resolved policy (both all and comment-only) instead of only for comment-only.

It's self-correcting: an all run overwrites any stale comment-only file rather than inheriting it, removing the reliance on /tmp never being reused. The CLI's env > file > default precedence in resolveEventPolicyFromEnv() is unchanged.

Added tests in secretOrchestrator.test.ts asserting the write for the default (all), comment-only, and non-review-agent-on-a-comment-only-project (self-correcting) cases — mocking only writeFileSync so no real /tmp I/O happens during the run.