feat(progress): replace file-based comment ID with CASCADE_PROGRESS_COMMENT_ID env var#638
Conversation
…OMMENT_ID env var
The file-based progress comment ID mechanism has been replaced with the CASCADE_PROGRESS_COMMENT_ID env var. Remove the previously tracked state file and its .gitignore entry. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Clean, well-scoped migration from file-based to env-var-based progress comment state. The implementation is correct, all tests pass, and the known limitation (dynamic case cross-process visibility) is properly documented and was already present in the previous design.
Should Fix
-
src/backends/secretBuilder.ts:79 — The env var name
CASCADE_PROGRESS_COMMENT_IDis hardcoded as a string literal instead of importingENV_VAR_NAMEfromprogressState.ts. Same insrc/backends/claude-code/env.ts:31. If the name ever changes, it would need updating in three places. Consider importing the constant. -
Missing tests for
injectProgressCommentId— The existingsecretBuilder.test.tscoversaugmentProjectSecretsandresolveGitHubTokenbut the newinjectProgressCommentIdfunction has no unit tests. It would be good to add tests for the three cases: (1) string ackCommentId with cardId → injects, (2) numeric ackCommentId → skips, (3) missing cardId → skips.
Nitpick
ProgressMonitorConfig.repoDiris now dead —repoDiris still defined in the interface (line 39 ofprogressMonitor.ts) and still passed bybuildProgressMonitorConfiginadapter.ts(line 174), butthis.config.repoDiris never read in theProgressMonitorclass. Consider removing it from the interface and the config builder in a follow-up.
| ackCommentId: string | number | undefined, | ||
| ): void { | ||
| if (cardId && typeof ackCommentId === 'string' && ackCommentId) { | ||
| projectSecrets.CASCADE_PROGRESS_COMMENT_ID = `${cardId}:${ackCommentId}`; |
There was a problem hiding this comment.
Nit: This hardcodes the env var name as a string literal. Consider importing ENV_VAR_NAME from progressState.ts to keep a single source of truth. Same applies to env.ts line 31.
There was a problem hiding this comment.
Fixed — both secretBuilder.ts and env.ts now import ENV_VAR_NAME from progressState.ts instead of hardcoding the string literal. Single source of truth for the env var name.
| if (cardId && typeof ackCommentId === 'string' && ackCommentId) { | ||
| projectSecrets.CASCADE_PROGRESS_COMMENT_ID = `${cardId}:${ackCommentId}`; | ||
| } | ||
| } |
There was a problem hiding this comment.
No unit tests for injectProgressCommentId. The existing secretBuilder.test.ts has thorough coverage for the other two exports — adding a few cases (string ackCommentId with cardId injects, numeric ackCommentId skips, missing cardId skips) would maintain the same standard.
There was a problem hiding this comment.
Added 5 unit tests for injectProgressCommentId in secretBuilder.test.ts, covering all branches: string ackCommentId with cardId (injects), numeric ackCommentId (skips), missing cardId (skips), undefined ackCommentId (skips), and empty string ackCommentId (skips).
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Clean, well-scoped refactor that replaces file-based state with an env var, reducing filesystem side-effects and simplifying the API. The implementation correctly covers both the pre-seeded path (via projectSecrets injection before subprocess launch) and the dynamic path (in-process process.env mutation). CI is green.
Should Fix (non-blocking)
- Dead
repoDirproperty:ProgressMonitorConfig.repoDir(progressMonitor.ts:39) is still declared and still passed frombuildProgressMonitorConfig(adapter.ts:174), but nothing insideProgressMonitorreadsthis.config.repoDiranymore. This is harmless dead code, but removing it would complete the cleanup and prevent confusion about whether it's still needed. Minor enough to leave for a follow-up.
|
@aaight address code review concerns |
- Import ENV_VAR_NAME from progressState.ts in secretBuilder.ts and env.ts instead of hardcoding 'CASCADE_PROGRESS_COMMENT_ID' string literal - Add 5 unit tests for injectProgressCommentId covering: string ackCommentId with cardId injects, numeric ackCommentId skips, missing cardId skips, undefined ackCommentId skips, empty string ackCommentId skips Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…state Tests from dev branch (PR #637) used old file-based signatures for writeProgressCommentId and clearProgressCommentId. Updated to match the new env-var-based API (no repoDir parameter). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The file may still be created by running cascade processes (e.g. the orchestrator session itself). Keeping the gitignore entry prevents it from appearing as an uncommitted change and triggering the stop hook. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Clean, well-executed refactoring. The file-based → env-var migration is correct and simplifies the codebase significantly (removes all fs operations, .gitignore entries, and stop-hook filtering for the state file). All 148 related tests pass.
Minor Cleanup (not blocking)
repoDir in ProgressMonitorConfig (progressMonitor.ts:39) is now entirely unused — it's still set in buildProgressMonitorConfig() (adapter.ts:174) but never read by ProgressMonitor or any delegate. Consider removing it in a follow-up to avoid confusion about whether it's still needed.
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Clean, well-structured migration from file-based state to env-var-based state for progress comment IDs. The change correctly addresses the cross-process communication need for the pre-seeded case (~90% of runs) via projectSecrets injection, and honestly documents the accepted gap for the dynamic case. All 139 relevant tests pass, CI is green, and the prior review feedback has been addressed.
Code Issues
Should Fix
-
ProgressMonitorConfig.repoDiris now dead — it's still declared inprogressMonitor.ts:39and still set inadapter.ts:174(buildProgressMonitorConfig), but is never read byProgressMonitorafter this PR. Consider removing the field from the interface and the assignment inbuildProgressMonitorConfigto avoid confusing future readers into thinking it's still relevant to progress state. -
secretBuilder.ts:80duplicates the${cardId}:${ackCommentId}format string fromprogressState.ts:26. SinceinjectProgressCommentIdwrites to aRecord<string, string>(notprocess.env), it can't reusewriteProgressCommentIddirectly — but extracting aformatProgressCommentValue(workItemId, commentId)helper intoprogressState.tswould keep the format as a single source of truth. Minor, since the format is trivial.
Summary
Replaces the file-based progress comment ID mechanism (
.cascade-progress-comment-id) with an env var (CASCADE_PROGRESS_COMMENT_ID), following the existingCASCADE_*naming pattern.progressState.ts— Fully rewrites all functions to useprocess.env.CASCADE_PROGRESS_COMMENT_IDinstead of reading/writing a file. Removes allfsimports,STATE_FILE_NAMEconstant, andrepoDirparameters.pmPoster.ts— RemovesrepoDirfromPMProgressPosterConfig, removesmaybeWriteStateFile()helper, and callswriteProgressCommentId(cardId, commentId)directly (no repoDir).progressMonitor.ts— Updatesstart()to callwriteProgressCommentId(cardId, commentId)withoutrepoDirguard, and updatesstop()to callclearProgressCommentId()with no args.env.ts— AddsCASCADE_PROGRESS_COMMENT_IDtoALLOWED_ENV_EXACTso it passes through to Claude Code subprocesses.secretBuilder.ts— AddsinjectProgressCommentId()helper to inject the pre-seeded ack comment ID intoprojectSecretsfor the subprocess.adapter.ts— CallsinjectProgressCommentId()after building project secrets so the subprocess gets the env var from startup (pre-seeded case, ~90% of runs).hooks.ts— RemovesSTATE_FILE_NAMEimport and the filter that excluded the state file from uncommitted-changes detection..gitignore— Removes.cascade-progress-comment-identry (no longer written to disk).tempDirsetup removed.Test plan
progressState.test.ts— new env-var read/write/clear testspmPoster.test.ts— updated to removerepoDirfrom fixturesprogress.test.ts— updated integration tests use new no-arg signaturespostComment.test.ts— no changes needed (already used no-arg signatures)claude-code-hooks.test.ts— removed.cascade-progress-comment-idfilter testsKnown limitation
Env vars set after subprocess start aren't visible to it. For the pre-seeded case (~90% of runs), the env var is injected into
projectSecretsbefore subprocess launch — works perfectly. For the dynamic case (postInitial()fires after subprocess start), the env var is set in the parent process only — this is an accepted gap (same behavior as before since the file was also only visible due to shared filesystem, not process injection).Card: https://trello.com/c/69abd4249587f02974b0c3d6
🤖 Generated with Claude Code