Skip to content

feat(sessions): add teamai save-session — monthly session logs + digest feed#192

Open
Eyre921 wants to merge 1 commit into
Tencent:mainfrom
Eyre921:feat/save-session
Open

feat(sessions): add teamai save-session — monthly session logs + digest feed#192
Eyre921 wants to merge 1 commit into
Tencent:mainfrom
Eyre921:feat/save-session

Conversation

@Eyre921

@Eyre921 Eyre921 commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Summary

Implements Features 1 & 5 from docs/designs/team-intelligence-platform.md — accepted ("ENG LOCKED") in that design but never built. digest.ts's getRecentSessions() has been reading an always-empty sessions/ directory; this PR gives it data.

teamai save-session folds the dashboard's existing per-session event stream (tool sequence, prompt turns, interventions) into a compact, privacy-scrubbed markdown summary, appends it to a local monthly log, and — with --push — commits a valuable session to the team repo at the exact path digest already reads, so "Session Highlights" lights up.

⚠️ Stacked on #191 (secret-redaction utility). The first commit in this branch is that PR; this PR's own change is the save-session commit. Please merge #191 first — the diff will then reduce to just this change.

Type of Change

  • New feature (non-breaking change that adds functionality)

What it does

teamai save-session                 # record most-recent session locally
teamai save-session --session-id X  # record a specific session
teamai save-session --push          # also push a "valuable" session to the team repo
teamai save-session --push --force  # push even trivial sessions
  • Local: ~/.teamai/session-logs/<year-month>.md (dedicated dir; not the shared ~/.teamai/sessions/ which holds contribute-state .json). Idempotent per session; pruned after 90 days (Feature 5 retention).
  • Team (--push, opt-in): sessions/<user>/<year-month>.md, direct commit (no MR), like contribute. This is the path digest's getRecentSessions() already globs — no change to digest.ts needed.
  • "Valuable" (Decision 8) = the session shows friction (interrupt / tool-reject / correction) or substantial tool use (≥3 distinct tools). Trivial sessions stay local unless --force.

Design decisions honored

  • Collect-then-summarize, no LLM call (Decision 1).
  • Monthly aggregated markdown (Decision 3).
  • Surfaces AI tool-misuse / retry signal (Decisions 7/8).
  • Privacy: team payload is counts + tool names + a single first-prompt line, and that line is run through redact() (from feat(privacy): add secret-redaction utility, scrub dashboard stopped-output #191). Consistent with the "counts only, no prompt text" posture.

Reuses existing infrastructure (no new collection path)

  • dashboard-collector readEvents() + aggregateSessionMetrics() for the data
  • utils/git pushRepoDirectly() for the direct team commit
  • read-only assertNotReadOnly() so HTTP-mode teams can't push
  • redact() for the only free text included

Test Plan

  • npx tsc --noEmit
  • npx vitest run — 140 files, 1741 tests (12 new in session-collector.test.ts)
  • npm run build
  • Real-CLI E2E: seeded ~/.teamai/dashboard/events.jsonl, ran node dist/index.js save-session --session-id <sid>. Result: ~/.teamai/session-logs/2026-07.md written with folded stats; a ghp_… token in the prompt came out <REDACTED:gh_tok> (0 raw leaks); a second run was idempotent ("already recorded"); --push without init failed gracefully while keeping the local log.

Notes for Reviewers

  • save-session is a command only (manual / agent-invoked). Auto-saving on the Stop hook is a natural follow-up but intentionally left out here to keep the injected-hook surface unchanged.
  • Local storage uses ~/.teamai/session-logs/ rather than the design's literal ~/.teamai/sessions/<year-month>.md, to avoid mixing with the contribute-state .json files already in sessions/. Happy to change if you prefer the literal path.
  • digest.ts is deliberately untouched — the feature works by supplying the data its existing reader expects.

@jeff-r2026

Copy link
Copy Markdown
Collaborator

Thanks for this — the feature is well-scoped and the reuse of the existing event stream (readEvents / aggregateSessionMetrics) instead of a new collection path is the right call. Three pieces of feedback: the CI failure, command placement, and commit hygiene.

1. Why the pipeline is red (root cause found)

It is not the fake API tokens in the test fixtures, as one might assume for a redaction PR. The internal open-source scan (CodeCC / semgrep) flags a single rule:

  • inner-mdb-normal-client — "found mdb normal client string leak" (severity: 严重)
  • at redact.test.ts:97-98:
    const out = redact('postgres://admin:s3cr3tPassword@db.example.com:5432/app');
    expect(out).toBe('postgres://admin:<REDACTED:conn>@db.example.com:5432/app');

The user:password@host:port/db DSN shape matches an internal rule that looks for leaked DB client connection strings. The scanner does not distinguish "test fixture" from "real credential" — it matches the shape and blocks.

Fix: change the connection-string fixture so it no longer matches the mdb-client DSN pattern, while still exercising the CONNECTION_STRING_PATTERN redaction. Options:

  • Use an obviously-synthetic scheme/host that breaks the DSN heuristic (e.g. avoid the postgres://.../app DB-name tail), or
  • Assemble the string from fragments at runtime so the literal never appears in source, then assert on the redacted output.

One thing to double-check: this exact line is anchored to commit 9c277bee, which is the #191 commit carried in this branch — and it now also lives on main (merged via #191), even though #191's own scan was green. So please rebase this branch onto the current main first: that drops the #191 commit from this PR's diff. If the scan is diff-scoped, the finding clears immediately; if it's whole-repo, the fixture will also need adjusting on main (a tiny follow-up), since it would otherwise flag on the next PR too.

2. save-session as a top-level command is too heavy

A new top-level verb crowds an already-long command list, and this feature is squarely in the analytics family that sits next to it (stats, digest, dashboard — all reading the same events.jsonl). Suggest folding it into a session subcommand group, matching how skill / roles / source are organized (group = noun, subcommand = verb):

teamai session save
teamai session save --push
teamai session save --push --force

This also leaves room for session list / session show later without adding more top-level commands.

3. Please don't stack unrelated commits in one PR

This PR contains two commits — the #191 redaction utility and the save-session change. Reviewing (and the scan) then sees both diffs mixed together, and the CI finding above is actually attributed to the #191 commit rather than this PR's own change. Once #191 is merged, please rebase so this PR reduces to a single, self-contained commit for the save-session change. One PR = one logical change = one commit keeps review scope and CI attribution clean.


Minor, worth a look while you're in here: the --push path uploads a redacted First ask: prompt line to the team repo. redact() is explicitly best-effort ("not a guarantee" per its own docstring), so this is a slight relaxation of the "counts only, no prompt text" posture. Might be worth defaulting to no prompt text and gating it behind an explicit --include-prompt, but I'll defer to the maintainers on the policy call.

@jeff-r2026 jeff-r2026 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes — see my detailed comment above. Blocking items:

  1. CI red — internal scan rule inner-mdb-normal-client flags the connection-string fixture at redact.test.ts:97-98. Rebase onto current main (drops the #191 commit) and reshape the DSN fixture so it no longer matches.
  2. Command placement — fold the top-level save-session into a teamai session save subcommand group.
  3. Commit hygiene — after #191 is merged, rebase so this PR is a single self-contained commit for the save-session change.

…est feed

Implements Features 1 & 5 from docs/designs/team-intelligence-platform.md,
which were accepted ("ENG LOCKED") but never built — digest.ts's
getRecentSessions() has been reading an always-empty sessions/ directory.

`teamai session save` folds the dashboard's existing per-session event stream
(tool sequence, prompt turns, interventions) into a compact markdown summary and
appends it to a local monthly log (~/.teamai/session-logs/<year-month>.md). With
`--push`, a "valuable" session (interventions or substantial tool use, per
Decision 8) is committed to the team repo at sessions/<user>/<month>.md — the
path digest already reads, so "Session Highlights" lights up with no change to
digest.ts.

Reuses existing infrastructure rather than a new collection path:
- dashboard-collector readEvents() + aggregateSessionMetrics() for the data
- utils/git pushRepoDirectly() for the direct team commit (no MR), like contribute
- redact() (from Tencent#191) on the only free text included (the first-prompt line)

Review feedback addressed:
- Placed under a `session` subcommand group (`teamai session save`) instead of a
  top-level verb, matching how `roles` / `source` are organized, leaving room
  for `session list` / `session show` later.
- Team upload defaults to counts + tools only; the redacted first-prompt line is
  opt-in via `--include-prompt`, since redact() is best-effort. Local logs keep it.

Team upload is opt-in; local logs are pruned after 90 days (Feature 5 retention).

Test Plan:
- npx tsc --noEmit
- npx vitest run (142 files, 1759 tests; 14 in session-collector.test.ts)
- npm run build
- real-CLI E2E: seeded ~/.teamai/dashboard/events.jsonl, ran
  `node dist/index.js session save --session-id <sid>`; wrote
  ~/.teamai/session-logs/2026-07.md with folded stats, a `ghp_` token in the
  prompt came out `<REDACTED:gh_tok>`; old `save-session` now errors as unknown;
  `--include-prompt` appears in `session save --help`.

Claude-Session: https://claude.ai/code/session_01GEV81Xj6mhPzSPsyBrDPSd
@Eyre921
Eyre921 force-pushed the feat/save-session branch from 2ae6b18 to 647551c Compare July 17, 2026 16:51
@Eyre921

Eyre921 commented Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review — all three blocking items and the privacy note are addressed. The branch is now a single self-contained commit rebased onto current main (the #191 commit is gone).

1. CI red (DSN scanner). Root cause confirmed — the inner-mdb-normal-client rule matches the connection-string shape in redact.test.ts, which now lives on main via #191. Since the scan is effectively whole-repo, I opened #207 to fix it at the source: the fixture is now assembled from fragments at runtime so no literal user:pass@host appears in source, while still exercising CONNECTION_STRING_PATTERN. This PR no longer carries that file at all. Merging #207 should clear the finding for this PR (and every future one).

2. Command placement. save-session is now teamai session save under a session subcommand group, matching roles / source. Leaves room for session list / session show later.

3. Commit hygiene. Rebased onto main → one logical change, one commit, 7 files, none of them from #191.

Privacy note (well taken). The team upload now defaults to counts + tools only; the redacted first-prompt line is opt-in behind --include-prompt. Local logs keep it (they never leave the machine).

Re-verified: tsc --noEmit, vitest run (142 files / 1759 tests), npm run build, and a real-CLI E2E of teamai session save (folded stats written, ghp_ token → <REDACTED:gh_tok>, old save-session now errors as unknown, --include-prompt shown in help).

jeff-r2026 pushed a commit that referenced this pull request Jul 20, 2026
…DSN scanner (#207)

The internal CodeCC/semgrep scan rule `inner-mdb-normal-client` ("mdb normal
client string leak", severity 严重) flags the connection-string test fixture in
redact.test.ts — the `user:pass@host` DSN shape matches even though it's an
obviously-synthetic test value, blocking CI on every PR that touches this tree
(surfaced on #192).

Assemble the DSN from fragments at runtime and compute the expected output, so
no contiguous `user:password@host` literal appears in source while the test
still exercises CONNECTION_STRING_PATTERN redaction. No production code change.

Test Plan:
- npx vitest run src/__tests__/redact.test.ts (21 pass)
- grep confirms no literal DSN shape remains in the file

Claude-Session: https://claude.ai/code/session_01GEV81Xj6mhPzSPsyBrDPSd

Co-authored-by: Eyre921 <Eyre921@users.noreply.github.com>
@jeff-r2026

Copy link
Copy Markdown
Collaborator

Review notes

Clean design overall — reuses the dashboard's existing event stream plus pushRepoDirectly / assertNotReadOnly / redactWithEnv instead of adding a new collection path, and the write path lines up exactly with digest.ts's getRecentSessions() (sessions/*/*.md), so the "no digest.ts change needed" claim holds. Privacy posture is correct too: team payload defaults to counts + tool names, the first-prompt line is opt-in, and it's redacted before truncation.

A few suggestions:

1. Command-name mismatch (suggest fixing before merge)

The command registers as session save (README table also says teamai session save), but several user-facing strings still say the old teamai save-session, which isn't a valid command — copy-pasting it yields "unknown command":

  • save-session.ts:426 — error hint: Retry later with: teamai save-session --push
  • save-session.ts:388assertNotReadOnly(localConfig, 'teamai save-session --push') (surfaces in the read-only error)

The docstring references (L272 / L444 / L676) are worth aligning too.

2. assertNotReadOnly throws uncaught (minor)

assertNotReadOnly (read-only.ts:10) throws, and the call at save-session.ts:388 is outside any try/catch — there's no top-level action wrapper (index.ts:935 calls program.parse() directly). On an HTTP-mode team, --push prints an unhandled stack trace instead of the intended "local log was still saved" message (the local log is in fact already written by that point). Suggest wrapping the push section in try/catch.

3. Idempotency marker can collide across sessions (minor)

appendMonthlyLog dedups by · <shortId> · (first 8 UUID chars, session-collector.ts:562). Two same-month sessions sharing an 8-char prefix (~1 in 4B) would silently drop the second. Acceptable given the odds; embedding the full sessionId in an HTML comment would be a zero-cost stronger key.

4. Promise.race doesn't clearTimeout (nit, matches existing pattern)

save-session.ts:417-421 never clears the 10s timer on success, so the process can stay alive up to 10s longer. Mirrors contribute.ts:173-183 exactly, so it's consistent with convention; a shared helper with finally { clearTimeout } would fix both.

Misc

Verdict: well-scoped and reuses infrastructure nicely. Suggest fixing #1 before merge (a copy-paste instruction that misleads users); #2 for polish; #3/#4 optional.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants