Skip to content

Add createdAt for chat.Message - #3

Closed
dgageot wants to merge 3 commits into
mainfrom
add-createdAt-chat-message
Closed

Add createdAt for chat.Message#3
dgageot wants to merge 3 commits into
mainfrom
add-createdAt-chat-message

Conversation

@dgageot

@dgageot dgageot commented Sep 1, 2025

Copy link
Copy Markdown
Member

No description provided.

Signed-off-by: Trung Nguyen <trungutt@users.noreply.github.com>
Signed-off-by: Trung Nguyen <trungutt@users.noreply.github.com>
Signed-off-by: Trung Nguyen <trungutt@users.noreply.github.com>
@dgageot

dgageot commented Sep 2, 2025

Copy link
Copy Markdown
Member Author

Merged with 6df068d

@dgageot dgageot closed this Sep 2, 2025
a-dubs pushed a commit to a-dubs/cagent that referenced this pull request Sep 3, 2025
a-dubs pushed a commit to a-dubs/cagent that referenced this pull request Sep 3, 2025
@rumpl
rumpl deleted the add-createdAt-chat-message branch September 16, 2025 18:31
dgageot added a commit to dgageot/cagent that referenced this pull request Apr 15, 2026
This is useful for computing baselines by running the same eval(s) N times.
Each repeated eval gets a '#N' suffix in its title (e.g. 'my-eval docker#3').

Assisted-By: docker-agent
dgageot added a commit that referenced this pull request Apr 28, 2026
…nctionCall scrub

Five fixes from a self-review pass:

1. Bug: pre_tool_use builtin returned the entire ToolInput map in

   UpdatedInput. Hooks for one event run concurrently and aggregate

   via shallow maps.Copy in config order, so emitting unchanged keys

   would clobber a sibling hook's modifications. Fix: only emit keys

   whose value actually changed (new redactToolInput helper). This

   also removes the unsafe top-level type assertion on redactAny's

   any-typed return value.

2. Security gap: the runtime transform missed two text-bearing

   surfaces that round-trip to providers:

     - chat.Message.ReasoningContent (sent back to Anthropic, Bedrock,

       and DeepSeek as a thinking block)

     - the legacy singular chat.Message.FunctionCall.Arguments

       (still emitted by the OpenAI provider when set)

   A secret in either of those fields would round-trip to the next

   LLM call. Now both are scrubbed; the FunctionCall pointer is

   deep-copied so the caller's history stays untouched.

3. Performance: secretsscan.Redact called strings.ToLower(out) and

   compiledRules() once per rule (~85 iterations), which on long

   inputs dominated the work. Hoisted both out of the loop. Sound

   because RedactionMarker contains no rule keyword (locked in by

   the new TestRedactionMarkerIsNotASecret) so a stale lower-case

   only ever produces extra-cautious regex runs, not false negatives.

   Allocations on a 9KB clean message: ~85 -> 2.

4. Test gap: added TestRedactionMarkerIsNotASecret to lock in the

   safety property the idempotence claim relies on (the marker must

   not match any rule, even when surrounded by arbitrary text).

5. Added BenchmarkRedactCleanInput / BenchmarkRedactWithSecret as a

   regression guard for #3.

Tests + lint: green. Behavior on clean inputs and previously-tested

secrets is unchanged.

Assisted-By: docker-agent
@docker-agent docker-agent Bot mentioned this pull request May 6, 2026
@aheritier aheritier added area/api For features/issues/fixes related to the usage of the cagent API kind/feature status/needs-rebase PR has merge conflicts or is out of date with main labels Jun 17, 2026
@aheritier aheritier added kind/feat PR adds a new feature (maps to feat:). Use on PRs only. and removed kind/feature labels Jun 26, 2026
aheritier added a commit that referenced this pull request Aug 27, 2026
…rt, align bounds

Addresses all blocking and should-fix findings from the aheritier review
on PR #4062:

[blocking #1 + #2] Generic classifier with HTTP-status precedence:
- Add modelerrors.RetryableHTTPStatus(err) — catches any error carrying a
  retryable HTTP status (429/408/5xx) via *StatusError or message regex,
  without string-pattern heuristics ('connection refused' stays non-retryable).
- startBackoffRetryable becomes: return err != nil && RetryableHTTPStatus(err).
  A StatusError{429} coexisting with context.DeadlineExceeded now arms the
  gate (HTTP wins), fixing the deadline-masks-rate-limit race.

[blocking #3] Bounds aligned with remediation plan:
- base = 15s, cap = 5min (was 1s/30s).
- Additive jitter [d, 1.2d] (was equal jitter [d/2, d]), guaranteeing the
  full nominal wait is always respected.

[blocking #4] Gate enforced only in the TryStart path:
- Move gate check from startLocked into new tryStartLocked (called by
  TryStart/TryStartWithTimeout only).
- Start() calls startLocked directly — mcpcatalog enable and skill
  sub-session startup are never delayed.

[should-fix #5] External recovery via StartReporter:
- tryStartLocked checks reporter.IsStarted() when started==false; a live
  reporter (e.g. after /toolset-restart) clears the gate and latches the
  wrapper without calling the underlying Start.
- New test: TestStartableToolSet_ExternalRecoveryClearsBackoffGate.

[should-fix #6] Exported constructor options for cross-package tests:
- NewStartable(ts, opts...) with StartableOption, WithStartRetryJitter,
  WithStartRetryClock.
- nowFn() clock seam; zero-value StartableToolSet still usable.

[should-fix #7] Concurrent and at-boundary tests:
- TestStartableToolSet_BackoffNoDoubleStartWithinWindow: 20 goroutines
  calling TryStart, assert underlying Start invoked exactly once.
- TestStartableToolSet_BackoffAtBoundary: fake clock, gate open at expiry.

[optional] Stale comment name in BackoffDormantForPlainErrors fixed.

Also:
- RetryableHTTPStatus test cases include plain-text regex fallback.
- ExportedSetClock removed (unused; WithStartRetryClock preferred).
- All gating tests converted from s.Start() to s.TryStart().
- Jitter-bounds assertions updated to [nominal, 1.2×nominal].

PR2 (#4065) will need rebasing and test updates after this lands.
aheritier added a commit that referenced this pull request Aug 27, 2026
…rt, align bounds

Addresses all blocking and should-fix findings from the aheritier review
on PR #4062:

[blocking #1 + #2] Generic classifier with HTTP-status precedence:
- Add modelerrors.RetryableHTTPStatus(err) — catches any error carrying a
  retryable HTTP status (429/408/5xx) via *StatusError or message regex,
  without string-pattern heuristics ('connection refused' stays non-retryable).
- startBackoffRetryable becomes: return err != nil && RetryableHTTPStatus(err).
  A StatusError{429} coexisting with context.DeadlineExceeded now arms the
  gate (HTTP wins), fixing the deadline-masks-rate-limit race.

[blocking #3] Bounds aligned with remediation plan:
- base = 15s, cap = 5min (was 1s/30s).
- Additive jitter [d, 1.2d] (was equal jitter [d/2, d]), guaranteeing the
  full nominal wait is always respected.

[blocking #4] Gate enforced only in the TryStart path:
- Move gate check from startLocked into new tryStartLocked (called by
  TryStart/TryStartWithTimeout only).
- Start() calls startLocked directly — mcpcatalog enable and skill
  sub-session startup are never delayed.

[should-fix #5] External recovery via StartReporter:
- tryStartLocked checks reporter.IsStarted() when started==false; a live
  reporter (e.g. after /toolset-restart) clears the gate and latches the
  wrapper without calling the underlying Start.
- New test: TestStartableToolSet_ExternalRecoveryClearsBackoffGate.

[should-fix #6] Exported constructor options for cross-package tests:
- NewStartable(ts, opts...) with StartableOption, WithStartRetryJitter,
  WithStartRetryClock.
- nowFn() clock seam; zero-value StartableToolSet still usable.

[should-fix #7] Concurrent and at-boundary tests:
- TestStartableToolSet_BackoffNoDoubleStartWithinWindow: 20 goroutines
  calling TryStart, assert underlying Start invoked exactly once.
- TestStartableToolSet_BackoffAtBoundary: fake clock, gate open at expiry.

[optional] Stale comment name in BackoffDormantForPlainErrors fixed.

Also:
- RetryableHTTPStatus test cases include plain-text regex fallback.
- ExportedSetClock removed (unused; WithStartRetryClock preferred).
- All gating tests converted from s.Start() to s.TryStart().
- Jitter-bounds assertions updated to [nominal, 1.2×nominal].

PR2 (#4065) will need rebasing and test updates after this lands.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/api For features/issues/fixes related to the usage of the cagent API kind/feat PR adds a new feature (maps to feat:). Use on PRs only. status/needs-rebase PR has merge conflicts or is out of date with main

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants