mosaicstack/stack
1
0
Fork 0
Code Issues 42 Pull Requests Actions Packages Projects Releases 15 Wiki Activity

fix(#707): scope session GC retention #720

Merged
jason.woltje merged 3 commits from fix/tess-session-gc-scope into main 2026-07-13 00:44:20 +00:00
Conversation 2 Commits 3 Files Changed 10 +163 -175
jason.woltje commented 2026-07-12 21:35:20 +00:00
Owner
Copy Link

Refs #707

Task: TESS-M1-SEC-006

Scopes session cleanup and log retention to a single session, disables global GC paths, and retires legacy global schedules.

Refs #707 Task: TESS-M1-SEC-006 Scopes session cleanup and log retention to a single session, disables global GC paths, and retires legacy global schedules.
jason.woltje added 1 commit 2026-07-12 21:35:20 +00:00
fix(#707): scope session GC retention
All checks were successful
ci/woodpecker/push/ci Pipeline was successful
Details
ci/woodpecker/pr/ci Pipeline was successful
Details
5ad29926d9
jason.woltje force-pushed fix/tess-session-gc-scope from 5ad29926d9 to f17e70c145 2026-07-12 23:21:35 +00:00 Compare
jason.woltje added 1 commit 2026-07-12 23:48:40 +00:00
test(#707): preserve approval boundary during session GC
All checks were successful
ci/woodpecker/pr/ci Pipeline was successful
Details
a4b6665bd7
jason.woltje commented 2026-07-13 00:17:15 +00:00
Author
Owner
Copy Link

REQUEST CHANGES reviewer-of-record [W-jarvis:reviewer] head a4b6665bd7

CI is green on exact head (ci/woodpecker/pr/ci pipeline 1713), and the direction is right: global /gc, cold-start full GC, and scheduled global GC are disabled; log demotion now uses promoteSessionToWarm(sessionId, cutoff); SEC-001 command approval tests are present and pass in CI.

Blockers before I can attest SEC-006:

  1. SessionGCService.collect() still builds a Redis SCAN MATCH glob from the raw session id: mosaic:session:${sessionId}:*. WebSocket ingress accepts any string conversationId, and that value can become the session id. A session id containing Redis glob metacharacters (for example *) broadens the scan/delete outside the single session namespace, violating the per-session retention boundary. Please escape/canonicalize session ids before using them in Redis glob patterns, or avoid glob matching for session cleanup, and add a regression test with glob metacharacters proving only the requested session's keys are deleted.

  2. The requested durable-approval regression is not proven yet. The current test asserts the scan pattern is mosaic:session:abc:* and that del was not called with a command-approval string, but it does not create a valid SEC-001 approval, run a GC pass, and prove the approval still authorizes afterward. Please add that regression so a valid tess:command-approval:<id> survives session GC.

Once those are covered, I can re-review the new head.

REQUEST CHANGES reviewer-of-record [W-jarvis:reviewer] head a4b6665bd76a23b75fbebee5535a7cc1a420dd9f CI is green on exact head (`ci/woodpecker/pr/ci` pipeline 1713), and the direction is right: global `/gc`, cold-start full GC, and scheduled global GC are disabled; log demotion now uses `promoteSessionToWarm(sessionId, cutoff)`; SEC-001 command approval tests are present and pass in CI. Blockers before I can attest SEC-006: 1. `SessionGCService.collect()` still builds a Redis `SCAN MATCH` glob from the raw session id: `mosaic:session:${sessionId}:*`. WebSocket ingress accepts any string `conversationId`, and that value can become the session id. A session id containing Redis glob metacharacters (for example `*`) broadens the scan/delete outside the single session namespace, violating the per-session retention boundary. Please escape/canonicalize session ids before using them in Redis glob patterns, or avoid glob matching for session cleanup, and add a regression test with glob metacharacters proving only the requested session's keys are deleted. 2. The requested durable-approval regression is not proven yet. The current test asserts the scan pattern is `mosaic:session:abc:*` and that `del` was not called with a `command-approval` string, but it does not create a valid SEC-001 approval, run a GC pass, and prove the approval still authorizes afterward. Please add that regression so a valid `tess:command-approval:<id>` survives session GC. Once those are covered, I can re-review the new head.
jason.woltje added 1 commit 2026-07-13 00:20:53 +00:00
fix(#707): escape session GC scan scope
All checks were successful
ci/woodpecker/pr/ci Pipeline was successful
Details
37be090e7b
jason.woltje commented 2026-07-13 00:31:02 +00:00
Author
Owner
Copy Link

VERIFIED APPROVE reviewer-of-record [W-jarvis:reviewer] head 37be090e7b

Evidence:

  • Exact PR/branch head verified: 37be090e7bcbe2666535f1b3f2a53658535d599e.
  • PR CI green: ci/woodpecker/pr/ci pipeline 1715 success.
  • Re-reviewed remediation for prior blockers: SessionGCService.collect() now escapes Redis glob metacharacters before building the SCAN MATCH pattern, with a regression for abc*?[tenant]\\escape; this would fail if the raw session id were used.
  • Durable approval survival is covered by creating a real CommandAuthorizationService approval, running a session GC pass, confirming the tess:command-approval:<id> entry remains, then authorizing with that approval successfully.
  • SEC-001 abuse coverage remains: valid durable approval is still required/consumed, but /gc no longer executes an unaudited global sweep; it returns disabled and does not call sweepOrphans.
  • SEC-006 retention boundary remains scoped: session GC deletes only the requested session namespace, demotes logs through promoteSessionToWarm(sessionId, cutoff), exposes no fullCollect/sweepOrphans, and startup removes legacy repeatable global GC schedules.
  • git diff --check clean; no secrets in diff.
VERIFIED APPROVE reviewer-of-record [W-jarvis:reviewer] head 37be090e7bcbe2666535f1b3f2a53658535d599e Evidence: - Exact PR/branch head verified: `37be090e7bcbe2666535f1b3f2a53658535d599e`. - PR CI green: `ci/woodpecker/pr/ci` pipeline 1715 success. - Re-reviewed remediation for prior blockers: `SessionGCService.collect()` now escapes Redis glob metacharacters before building the `SCAN MATCH` pattern, with a regression for `abc*?[tenant]\\escape`; this would fail if the raw session id were used. - Durable approval survival is covered by creating a real `CommandAuthorizationService` approval, running a session GC pass, confirming the `tess:command-approval:<id>` entry remains, then authorizing with that approval successfully. - SEC-001 abuse coverage remains: valid durable approval is still required/consumed, but `/gc` no longer executes an unaudited global sweep; it returns disabled and does not call `sweepOrphans`. - SEC-006 retention boundary remains scoped: session GC deletes only the requested session namespace, demotes logs through `promoteSessionToWarm(sessionId, cutoff)`, exposes no `fullCollect`/`sweepOrphans`, and startup removes legacy repeatable global GC schedules. - `git diff --check` clean; no secrets in diff.
jason.woltje merged commit 753a360517 into main 2026-07-13 00:44:20 +00:00
jason.woltje deleted branch fix/tess-session-gc-scope 2026-07-13 00:44:20 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
fleet-enhancement
Fleet-wide reliability/speed/cost improvements surfaced by the nightly enhance pass
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
jason.woltje
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: mosaicstack/stack#720
Delete Branch "fix/tess-session-gc-scope"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?