Uh oh!
There was an error while loading. Please reload this page.
Expose launcher tag primitives - #385
Merged
Merged
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:9d3905f766
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
📝 WalkthroughWalkthroughThis PR adds repeatable folded-enrichment tag filtering ( ChangesTag filtering, pending stamps, and summary grouping
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
willwashburnforce-pushed
the
issue-373-launcher-tags
branch
from
May 8, 2026 07:03
9d3905f to
65a490bCompareThere was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/relayburn-cli/src/commands/summary.rs`:
- Around line 481-492: The parser parse_tag_filters currently overwrites
duplicate --tag keys by inserting into a BTreeMap; change it to detect
duplicates and return an error instead of silently keeping the last value: while
iterating in parse_tag_filters, after splitting raw into (key, value) check if
out.contains_key(key) and if so use anyhow::bail! to report the duplicate
(include the key and the conflicting raw entries or values), otherwise insert
the pair; keep other validation (empty key) as-is so callers like build_query()
will receive explicit failure for repeated --tag flags.
In `@crates/relayburn-sdk-node/src/lib.rs`:
- Around line 495-541: The parse_iso_system_time function currently accepts
invalid calendar dates (e.g. 2026-02-31) because it only bounds day to 1..=31
and relies on days_from_civil to normalize; update parse_iso_system_time to
reject impossible month/day combinations by validating the day against the
actual days-in-month for the parsed year and month (accounting for leap years)
before calling days_from_civil. Locate parse_iso_system_time and the
date-parsing helpers (parse_i64_part, parse_u32_part) and either compute
days_in_month(year, month) or use a validated date construction (e.g., try
building a chrono::NaiveDate or perform a days_from_civil reverse-check) and
return Err(invalid_arg(...)) when the day is out of range so
normalized/normalized-to-different-instant inputs are rejected.
In `@crates/relayburn-sdk/src/ingest/ingest.rs`:
- Around line 460-470: The call to resolve_pending_stamps_for_session_in
currently discards both errors and the applied count (using let _ = ...); change
this to capture the Result, log any Err via eprintln! or process logger
including the candidate info (PendingStampSessionCandidate, ledger,
ledger_home), and on Ok(applied) add the applied count to the ingest report
(e.g., increment your ingest_report or report.applied_pending_stamps field) so
resolution failures are visible and the number applied is tracked; do the same
replacement for the other occurrences of resolve_pending_stamps_for_session_in
in this file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e92b981c-1819-4f42-ac64-fc89d496f136
📒 Files selected for processing (22)
CHANGELOG.mdREADME.mdcrates/relayburn-cli/src/cli.rscrates/relayburn-cli/src/commands/run.rscrates/relayburn-cli/src/commands/summary.rscrates/relayburn-sdk-node/src/lib.rscrates/relayburn-sdk/src/ingest/ingest.rscrates/relayburn-sdk/src/ingest/orchestration_tests.rscrates/relayburn-sdk/src/ingest/pending_stamps.rscrates/relayburn-sdk/src/ingest/pending_stamps_compat_tests.rscrates/relayburn-sdk/src/query_verbs.rscrates/relayburn-sdk/tests/integration.rspackages/sdk-node/CHANGELOG.mdpackages/sdk-node/README.mdpackages/sdk-node/src/binding.d.tspackages/sdk-node/src/index.cjspackages/sdk-node/src/index.d.tspackages/sdk-node/src/index.jspackages/sdk-node/test/conformance.test.jspackages/sdk-node/test/esbuild-smoke.test.jstests/fixtures/cli-golden/snapshots/run-help.stdout.txttests/fixtures/cli-golden/snapshots/top-level-help.stdout.txt
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
willwashburnforce-pushed
the
issue-373-launcher-tags
branch
from
May 8, 2026 07:15
65a490b to
c09a408CompareThere was a problem hiding this comment.
♻️ Duplicate comments (2)
crates/relayburn-cli/src/commands/summary.rs (1)
481-490:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject conflicting repeated
--tagfilters instead of silently keeping the last one.Line 490 still overwrites earlier entries in the
BTreeMap, so--tag persona=a --tag persona=bgets collapsed beforebuild_query()can detect the conflict. That changes the requested slice instead of failing fast.Suggested fix
fn parse_tag_filters(tags: &[String]) -> anyhow::Result<BTreeMap<String, String>> { let mut out = BTreeMap::new(); for raw in tags { let (key, value) = raw .split_once('=') .ok_or_else(|| anyhow::anyhow!("burn: --tag expects k=v, got \"{raw}\""))?; if key.is_empty() { anyhow::bail!("burn: --tag key must be non-empty (got \"{raw}\")"); } - out.insert(key.to_string(), value.to_string());+ if let Some(existing) = out.insert(key.to_string(), value.to_string()) {+ if existing != value {+ anyhow::bail!(+ "burn: conflicting filters for tag \"{key}\" ({existing:?} vs {value:?})"+ );+ }+ } } Ok(out) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/relayburn-cli/src/commands/summary.rs` around lines 481 - 490, The parse_tag_filters function currently inserts tags into a BTreeMap and silently overwrites duplicate keys (e.g., --tag persona=a then --tag persona=b); change parse_tag_filters to detect duplicates before inserting (use out.contains_key(key) or the BTreeMap::entry API) and return an error (anyhow::bail!) when the same key is specified more than once, including the offending key/raw pair so callers like build_query() can fail fast instead of seeing the last value only.crates/relayburn-sdk-node/src/lib.rs (1)
528-540:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject impossible calendar dates in
spawnStartTs.This still only bounds
dayto1..=31, so values like2026-02-31T00:00:00Zare accepted and normalized to a different instant bydays_from_civil(). The caller can end up stamping a launch time other than the one they passed.Suggested fix
- if !(1..=12).contains(&month)- || !(1..=31).contains(&day)+ let max_day = days_in_month(year, month);+ if max_day == 0+ || day == 0+ || day > max_day || hour > 23 || minute > 59 || second > 60 { return Err(invalid_arg("spawnStartTs is outside the supported range")); @@ fn days_from_civil(year: i64, month: u32, day: u32) -> i64 { let y = year - i64::from(month <= 2); let era = if y >= 0 { y } else { y - 399 } / 400; let yoe = y - era * 400; let mp = month as i64 + if month > 2 { -3 } else { 9 }; let doy = (153 * mp + 2) / 5 + day as i64 - 1; let doe = yoe * 365 + yoe / 4 - yoe / 100 + doy; era * 146_097 + doe - 719_468 } ++fn days_in_month(year: i64, month: u32) -> u32 {+ match month {+ 1 | 3 | 5 | 7 | 8 | 10 | 12 => 31,+ 4 | 6 | 9 | 11 => 30,+ 2 if is_leap_year(year) => 29,+ 2 => 28,+ _ => 0,+ }+}++fn is_leap_year(year: i64) -> bool {+ (year % 4 == 0 && year % 100 != 0) || year % 400 == 0+}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/relayburn-sdk-node/src/lib.rs` around lines 528 - 540, The current validation for spawnStartTs only checks day ∈ 1..=31 which allows impossible dates (e.g., Feb 31) that days_from_civil silently normalizes; update the validation before calling days_from_civil by computing the correct maximum day for the given month and year (handle February with leap-year logic: leap if (year%4==0 && year%100!=0) || year%400==0) and reject any day > max_day, returning the same invalid_arg error; keep the other checks (month, hour, minute, second) and then proceed to call days_from_civil and compute secs as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@crates/relayburn-cli/src/commands/summary.rs`:
- Around line 481-490: The parse_tag_filters function currently inserts tags
into a BTreeMap and silently overwrites duplicate keys (e.g., --tag persona=a
then --tag persona=b); change parse_tag_filters to detect duplicates before
inserting (use out.contains_key(key) or the BTreeMap::entry API) and return an
error (anyhow::bail!) when the same key is specified more than once, including
the offending key/raw pair so callers like build_query() can fail fast instead
of seeing the last value only.
In `@crates/relayburn-sdk-node/src/lib.rs`:
- Around line 528-540: The current validation for spawnStartTs only checks day ∈
1..=31 which allows impossible dates (e.g., Feb 31) that days_from_civil
silently normalizes; update the validation before calling days_from_civil by
computing the correct maximum day for the given month and year (handle February
with leap-year logic: leap if (year%4==0 && year%100!=0) || year%400==0) and
reject any day > max_day, returning the same invalid_arg error; keep the other
checks (month, hour, minute, second) and then proceed to call days_from_civil
and compute secs as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8bafcc08-9abe-407d-8819-fba54ea7b229
📒 Files selected for processing (34)
CHANGELOG.mdREADME.mdcrates/relayburn-cli/Cargo.tomlcrates/relayburn-cli/src/cli.rscrates/relayburn-cli/src/commands/mod.rscrates/relayburn-cli/src/commands/run.rscrates/relayburn-cli/src/commands/summary.rscrates/relayburn-cli/src/harnesses/mod.rscrates/relayburn-cli/src/harnesses/registry.rscrates/relayburn-cli/src/lib.rscrates/relayburn-cli/src/main.rscrates/relayburn-cli/src/util/time.rscrates/relayburn-cli/tests/smoke.rscrates/relayburn-sdk-node/src/lib.rscrates/relayburn-sdk/src/ingest/gap.rscrates/relayburn-sdk/src/ingest/ingest.rscrates/relayburn-sdk/src/ingest/orchestration_tests.rscrates/relayburn-sdk/src/ingest/pending_stamps.rscrates/relayburn-sdk/src/ingest/pending_stamps_compat_tests.rscrates/relayburn-sdk/src/query_verbs.rscrates/relayburn-sdk/tests/integration.rspackages/relayburn/CHANGELOG.mdpackages/sdk-node/CHANGELOG.mdpackages/sdk-node/README.mdpackages/sdk-node/src/binding.d.tspackages/sdk-node/src/index.cjspackages/sdk-node/src/index.d.tspackages/sdk-node/src/index.jspackages/sdk-node/test/conformance.test.jspackages/sdk-node/test/esbuild-smoke.test.jstests/fixtures/cli-golden/README.mdtests/fixtures/cli-golden/invocations.jsontests/fixtures/cli-golden/snapshots/run-help.stdout.txttests/fixtures/cli-golden/snapshots/top-level-help.stdout.txt
💤 Files with no reviewable changes (6)
- tests/fixtures/cli-golden/snapshots/run-help.stdout.txt
- crates/relayburn-cli/src/commands/run.rs
- tests/fixtures/cli-golden/invocations.json
- crates/relayburn-cli/src/main.rs
- crates/relayburn-cli/src/cli.rs
- crates/relayburn-cli/src/commands/mod.rs
✅ Files skipped from review due to trivial changes (13)
- packages/sdk-node/src/binding.d.ts
- packages/sdk-node/README.md
- tests/fixtures/cli-golden/README.md
- crates/relayburn-cli/src/util/time.rs
- crates/relayburn-cli/src/lib.rs
- crates/relayburn-cli/src/harnesses/registry.rs
- crates/relayburn-sdk/src/ingest/gap.rs
- packages/sdk-node/test/esbuild-smoke.test.js
- packages/relayburn/CHANGELOG.md
- crates/relayburn-cli/src/harnesses/mod.rs
- packages/sdk-node/src/index.d.ts
- README.md
- packages/sdk-node/CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/relayburn-sdk/src/ingest/pending_stamps.rs
- CHANGELOG.md
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
crates/relayburn-cli/src/commands/summary.rs (1)
481-495:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAllow idempotent repeated
--tagfilters.This now rejects
--tag persona=qa --tag persona=qathe same way it rejects conflicting duplicates. Since--tagis repeatable, only conflicting values for the same key should error; identical repeats should be a no-op.Suggested fix
fn parse_tag_filters(tags: &[String]) -> anyhow::Result<BTreeMap<String, String>> { let mut out = BTreeMap::new(); for raw in tags { let (key, value) = raw .split_once('=') .ok_or_else(|| anyhow::anyhow!("burn: --tag expects k=v, got \"{raw}\""))?; if key.is_empty() { anyhow::bail!("burn: --tag key must be non-empty (got \"{raw}\")"); } - if let Some(existing) = out.get(key) {- anyhow::bail!(- "burn: duplicate --tag filter for key \"{key}\" ({existing:?} vs {value:?})"- );+ if let Some(existing) = out.get(key) {+ if existing != value {+ anyhow::bail!(+ "burn: duplicate --tag filter for key \"{key}\" ({existing:?} vs {value:?})"+ );+ }+ continue; } out.insert(key.to_string(), value.to_string()); } Ok(out) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/relayburn-cli/src/commands/summary.rs` around lines 481 - 495, In parse_tag_filters, change duplicate handling so identical repeats are a no-op: inside the existing match for out.get(key) (in function parse_tag_filters) compare the found existing value to the new value and only call anyhow::bail! when they differ; if they are equal simply continue (skip insertion) instead of treating it as an error. This preserves erroring on conflicting duplicates while allowing idempotent repeated --tag entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/relayburn-sdk-node/src/lib.rs`:
- Around line 538-543: The current timestamp assembly (using days_from_civil ->
days, then computing secs and doing UNIX_EPOCH + Duration::from_secs(...)+
Duration::from_nanos(...)) can overflow for extreme years; change the arithmetic
to use checked operations and SystemTime::checked_add: compute seconds with
checked_mul/checked_add on days/hour/minute/second (check each step and return
Err if any returns None), build Durations only from validated u64 values, then
use UNIX_EPOCH.checked_add(total_seconds_duration).and_then(|t|
t.checked_add(nanos_duration)) and return an Err(invalid_arg(...)) if any check
fails; reference the variables/functions days_from_civil, days, secs, nanos,
UNIX_EPOCH, and the construction that currently uses + so you replace it with
the checked_add chain and early error returns on overflow.
In `@crates/relayburn-sdk/tests/integration.rs`:
- Around line 127-142: The test currently only has one stamped turn so
SummaryOptions.tags could be ignored and the assertions still pass; update the
test around handle.summary(SummaryOptions { tags: Some(Enrichment::from([...]))
, group_by_tag: Some("role".to_string()), ... }) to exercise filtering by adding
a second turn that does NOT have the "role":"integration-test" enrichment (or
add a negative query for a non-matching tag) before calling handle.summary, then
assert that tagged.turn_count and by_tag contain only the matching stamped turn
while the total turn_count reflects both turns; this uses the existing
handle.summary, SummaryOptions, Enrichment and group_by_tag symbols to locate
where to change the test.
---
Duplicate comments:
In `@crates/relayburn-cli/src/commands/summary.rs`:
- Around line 481-495: In parse_tag_filters, change duplicate handling so
identical repeats are a no-op: inside the existing match for out.get(key) (in
function parse_tag_filters) compare the found existing value to the new value
and only call anyhow::bail! when they differ; if they are equal simply continue
(skip insertion) instead of treating it as an error. This preserves erroring on
conflicting duplicates while allowing idempotent repeated --tag entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5c132d60-75ac-4b98-a8cd-9ca9d9fa49d6
📒 Files selected for processing (36)
CHANGELOG.mdREADME.mdcrates/relayburn-cli/Cargo.tomlcrates/relayburn-cli/src/cli.rscrates/relayburn-cli/src/commands/ingest.rscrates/relayburn-cli/src/commands/mod.rscrates/relayburn-cli/src/commands/run.rscrates/relayburn-cli/src/commands/summary.rscrates/relayburn-cli/src/harnesses/mod.rscrates/relayburn-cli/src/harnesses/registry.rscrates/relayburn-cli/src/lib.rscrates/relayburn-cli/src/main.rscrates/relayburn-cli/src/util/time.rscrates/relayburn-cli/tests/smoke.rscrates/relayburn-sdk-node/src/lib.rscrates/relayburn-sdk/src/ingest/gap.rscrates/relayburn-sdk/src/ingest/ingest.rscrates/relayburn-sdk/src/ingest/orchestration_tests.rscrates/relayburn-sdk/src/ingest/pending_stamps.rscrates/relayburn-sdk/src/ingest/pending_stamps_compat_tests.rscrates/relayburn-sdk/src/ingest/watch_loop_tests.rscrates/relayburn-sdk/src/query_verbs.rscrates/relayburn-sdk/tests/integration.rspackages/relayburn/CHANGELOG.mdpackages/sdk-node/CHANGELOG.mdpackages/sdk-node/README.mdpackages/sdk-node/src/binding.d.tspackages/sdk-node/src/index.cjspackages/sdk-node/src/index.d.tspackages/sdk-node/src/index.jspackages/sdk-node/test/conformance.test.jspackages/sdk-node/test/esbuild-smoke.test.jstests/fixtures/cli-golden/README.mdtests/fixtures/cli-golden/invocations.jsontests/fixtures/cli-golden/snapshots/run-help.stdout.txttests/fixtures/cli-golden/snapshots/top-level-help.stdout.txt
💤 Files with no reviewable changes (6)
- crates/relayburn-cli/src/commands/mod.rs
- crates/relayburn-cli/src/cli.rs
- crates/relayburn-cli/src/main.rs
- tests/fixtures/cli-golden/snapshots/run-help.stdout.txt
- tests/fixtures/cli-golden/invocations.json
- crates/relayburn-cli/src/commands/run.rs
✅ Files skipped from review due to trivial changes (12)
- packages/sdk-node/src/binding.d.ts
- packages/sdk-node/README.md
- tests/fixtures/cli-golden/README.md
- crates/relayburn-cli/src/lib.rs
- crates/relayburn-sdk/src/ingest/gap.rs
- packages/relayburn/CHANGELOG.md
- crates/relayburn-cli/src/util/time.rs
- crates/relayburn-cli/src/harnesses/mod.rs
- packages/sdk-node/CHANGELOG.md
- tests/fixtures/cli-golden/snapshots/top-level-help.stdout.txt
- crates/relayburn-cli/src/harnesses/registry.rs
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/sdk-node/test/esbuild-smoke.test.js
- packages/sdk-node/src/index.d.ts
Comment on lines
+538
to
+543
| let days = days_from_civil(year, month, day); | ||
| if days < 0 { | ||
| return Err(invalid_arg("spawnStartTs must be at or after 1970-01-01")); | ||
| } | ||
| let secs = days as u64 * 86_400 + hour as u64 * 3_600 + minute as u64 * 60 + second as u64; | ||
| Ok(UNIX_EPOCH + Duration::from_secs(secs) + Duration::from_nanos(nanos as u64)) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Rust std::time, does using UNIX_EPOCH + Durationpanic on overflow, and isSystemTime::checked_add the recommended overflow-safe alternative?
💡 Result:
Yes. Using UNIX_EPOCH + duration via the Add implementation can panic on overflow. In the standard library docs for SystemTime, the impl Add for SystemTime is documented as: “This function may panic if the resulting point in time cannot be represented by the underlying data structure. See SystemTime::checked_add for a version without panic.” [1] SystemTime::checked_add is the recommended overflow-safe alternative. Its docs specify it returns Some(t) if self + duration can be represented as SystemTime and None otherwise (no panic). [1] So, if you want overflow safety, prefer: - SystemTime::UNIX_EPOCH.checked_add(duration) (and handle the None), rather than UNIX_EPOCH + duration. [1] Example pattern: - let t = SystemTime::UNIX_EPOCH.checked_add(dur).unwrap_or(SystemTime::MAX); (pattern shown in the docs). [1]
Citations:
🏁 Script executed:
sed -n '535,550p' crates/relayburn-sdk-node/src/lib.rsRepository: AgentWorkforce/burn
Length of output: 756
🏁 Script executed:
sed -n '500,545p' crates/relayburn-sdk-node/src/lib.rsRepository: AgentWorkforce/burn
Length of output: 1845
🏁 Script executed:
rg -n "fn days_from_civil|fn days_in_month" crates/relayburn-sdk-node/src/lib.rs -A 8Repository: AgentWorkforce/burn
Length of output: 744
Guard timestamp arithmetic against overflow for extreme input years.
Line 542 builds epoch seconds using unchecked arithmetic from an unvalidated year field, then constructs SystemTime via the + operator which panics on overflow. A year value like 999999 would overflow the intermediate arithmetic or exceed SystemTime's representable range.
Proposed hardening patch
fn parse_iso_system_time(s: &str) -> std::result::Result<SystemTime, BurnError> {
@@
let year: i64 = parse_i64_part(date_parts.next(), "year")?;
@@
+ if !(1970..=9999).contains(&year) {+ return Err(invalid_arg("spawnStartTs year must be between 1970 and 9999"));+ }+
@@
- let secs = days as u64 * 86_400 + hour as u64 * 3_600 + minute as u64 * 60 + second as u64;- Ok(UNIX_EPOCH + Duration::from_secs(secs) + Duration::from_nanos(nanos as u64))+ let day_secs = (days as u64)+ .checked_mul(86_400)+ .ok_or_else(|| invalid_arg("spawnStartTs is outside the supported range"))?;+ let hour_secs = u64::from(hour)+ .checked_mul(3_600)+ .ok_or_else(|| invalid_arg("spawnStartTs is outside the supported range"))?;+ let minute_secs = u64::from(minute)+ .checked_mul(60)+ .ok_or_else(|| invalid_arg("spawnStartTs is outside the supported range"))?;+ let secs = day_secs+ .checked_add(hour_secs)+ .and_then(|v| v.checked_add(minute_secs))+ .and_then(|v| v.checked_add(u64::from(second)))+ .ok_or_else(|| invalid_arg("spawnStartTs is outside the supported range"))?;++ UNIX_EPOCH+ .checked_add(Duration::from_secs(secs))+ .and_then(|t| t.checked_add(Duration::from_nanos(u64::from(nanos))))+ .ok_or_else(|| invalid_arg("spawnStartTs is outside the supported range"))
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/relayburn-sdk-node/src/lib.rs` around lines 538 - 543, The current
timestamp assembly (using days_from_civil -> days, then computing secs and doing
UNIX_EPOCH + Duration::from_secs(...)+ Duration::from_nanos(...)) can overflow
for extreme years; change the arithmetic to use checked operations and
SystemTime::checked_add: compute seconds with checked_mul/checked_add on
days/hour/minute/second (check each step and return Err if any returns None),
build Durations only from validated u64 values, then use
UNIX_EPOCH.checked_add(total_seconds_duration).and_then(|t|
t.checked_add(nanos_duration)) and return an Err(invalid_arg(...)) if any check
fails; reference the variables/functions days_from_civil, days, secs, nanos,
UNIX_EPOCH, and the construction that currently uses + so you replace it with
the checked_add chain and early error returns on overflow.
Comment on lines
+127
to
+142
| let tagged = handle | ||
| .summary(SummaryOptions { | ||
| tags: Some(Enrichment::from([( | ||
| "role".to_string(), | ||
| "integration-test".to_string(), | ||
| )])), | ||
| group_by_tag: Some("role".to_string()), | ||
| ..Default::default() | ||
| }) | ||
| .expect("tagged summary"); | ||
| assert_eq!(tagged.turn_count, 1); | ||
| let by_tag = tagged.by_tag.expect("byTag rows"); | ||
| assert_eq!(by_tag.len(), 1); | ||
| assert_eq!(by_tag[0].tag, "role"); | ||
| assert_eq!(by_tag[0].value.as_deref(), Some("integration-test")); | ||
| assert_eq!(by_tag[0].turn_count, 1); |
There was a problem hiding this comment.
Make this actually prove tags filtering works.
With only one stamped turn in the fixture, these assertions still pass if SummaryOptions.tags is ignored entirely. Add a negative case or a second non-matching turn so the new filter semantics are exercised, not just the grouped output shape.
Suggested test hardening
assert_eq!(by_tag[0].tag, "role");
assert_eq!(by_tag[0].value.as_deref(), Some("integration-test"));
assert_eq!(by_tag[0].turn_count, 1);
++ let missing = handle+ .summary(SummaryOptions {+ tags: Some(Enrichment::from([(+ "role".to_string(),+ "missing".to_string(),+ )])),+ ..Default::default()+ })+ .expect("missing tagged summary");+ assert_eq!(missing.turn_count, 0);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let tagged = handle | |
| .summary(SummaryOptions{ | |
| tags:Some(Enrichment::from([( | |
| "role".to_string(), | |
| "integration-test".to_string(), | |
| )])), | |
| group_by_tag:Some("role".to_string()), | |
| ..Default::default() | |
| }) | |
| .expect("tagged summary"); | |
| assert_eq!(tagged.turn_count,1); | |
| let by_tag = tagged.by_tag.expect("byTag rows"); | |
| assert_eq!(by_tag.len(),1); | |
| assert_eq!(by_tag[0].tag,"role"); | |
| assert_eq!(by_tag[0].value.as_deref(),Some("integration-test")); | |
| assert_eq!(by_tag[0].turn_count,1); | |
| let tagged = handle | |
| .summary(SummaryOptions{ | |
| tags:Some(Enrichment::from([( | |
| "role".to_string(), | |
| "integration-test".to_string(), | |
| )])), | |
| group_by_tag:Some("role".to_string()), | |
| ..Default::default() | |
| }) | |
| .expect("tagged summary"); | |
| assert_eq!(tagged.turn_count,1); | |
| let by_tag = tagged.by_tag.expect("byTag rows"); | |
| assert_eq!(by_tag.len(),1); | |
| assert_eq!(by_tag[0].tag,"role"); | |
| assert_eq!(by_tag[0].value.as_deref(),Some("integration-test")); | |
| assert_eq!(by_tag[0].turn_count,1); | |
| let missing = handle | |
| .summary(SummaryOptions{ | |
| tags:Some(Enrichment::from([( | |
| "role".to_string(), | |
| "missing".to_string(), | |
| )])), | |
| ..Default::default() | |
| }) | |
| .expect("missing tagged summary"); | |
| assert_eq!(missing.turn_count,0); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/relayburn-sdk/tests/integration.rs` around lines 127 - 142, The test
currently only has one stamped turn so SummaryOptions.tags could be ignored and
the assertions still pass; update the test around handle.summary(SummaryOptions
{ tags: Some(Enrichment::from([...])) , group_by_tag: Some("role".to_string()),
... }) to exercise filtering by adding a second turn that does NOT have the
"role":"integration-test" enrichment (or add a negative query for a non-matching
tag) before calling handle.summary, then assert that tagged.turn_count and
by_tag contain only the matching stamped turn while the total turn_count
reflects both turns; this uses the existing handle.summary, SummaryOptions,
Enrichment and group_by_tag symbols to locate where to change the test.
Uh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
writePendingStamp()through the Rust NAPI binding and@relayburn/sdk, including typed Node options/results--group-by-tagreporting forburn summary/ SDK summaryburn runlauncher wrapper from the CLI surface and docs; launchers now use SDK primitives plus ingestCloses#373
Tests
cargo test --workspacepnpm run test(Node SDK native-binding tests skipped because no local.nodeartifact was built)cargo test -p relayburn-cli --test smoke